-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#615: Authorization for public handlers #662
#615: Authorization for public handlers #662
Conversation
fa88449
to
0f72b2b
Compare
71a368e
to
27fa27d
Compare
29232e0
to
70f3c2a
Compare
51316d1
to
e527867
Compare
2df5448
to
dbcd0e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mario-nt, the PR is good, however I'm missing something we had discussed some days ago.
When the user is not authorized because is not authenticated I would keep returning a 401, instead of changing the API.
I would add a new service error for the case when guest user is not authorized. The error name could be one of these:
ServiceError::UnauthorizedActiognForGuests
ServiceError::GuestUnauthorized
ServiceError::MissingCredentials
ServiceError::Unauthorized
ServiceError::AuthenticationRequired
ServiceError::UserCredentialsRequired
We already have a similar error for that:
ServiceError::LoggedInUserNotFound => StatusCode::UNAUTHORIZED, // 401
But I suppose we don't need the ExtractLoggedInUser
extractor anymore. I would use:
ServiceError::UnauthorizedActiognForGuests => StatusCode::UNAUTHORIZED, // 401
In general, I prefer to be explicit about the error that has happened instead of how to solve it. The error message can contain instructions to fix it. For example:
"Unauthorized actions for guest. Try logging in to check if you have permission to perform the action"
And I would change this method:
pub async fn authorize(&self, action: ACTION, maybe_user_id: Option<UserId>) -> std::result::Result<(), ServiceError> {
let role = self.get_role(maybe_user_id).await;
let enforcer = self.casbin_enforcer.enforcer.read().await;
let authorize = enforcer
.enforce((role, action))
.map_err(|_| ServiceError::UnauthorizedAction)?;
if authorize {
Ok(())
} else {
Err(ServiceError::UnauthorizedAction)
}
}
To:
pub async fn authorize(&self, action: ACTION, maybe_user_id: Option<UserId>) -> std::result::Result<(), ServiceError> {
let role = self.get_role(maybe_user_id).await;
let enforcer = self.casbin_enforcer.enforcer.read().await;
let authorized = enforcer
.enforce((role, action))
.map_err(|_| ServiceError::UnauthorizedAction)?;
if authorized {
Ok(())
} else {
if role == Role::guest {
Err(ApiError::UserCredentialsRequired) // Guest users are not authorized for this action
} else {
Err(ServiceError::UnauthorizedAction) // Authenticated user lacks necessary permissions
}
}
}
8e85f51
to
f38b628
Compare
ACK f38b628 |
Parent issue: #615