#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
josecelano
left a comment
There was a problem hiding this comment.
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::UnauthorizedActiognForGuestsServiceError::GuestUnauthorizedServiceError::MissingCredentialsServiceError::UnauthorizedServiceError::AuthenticationRequiredServiceError::UserCredentialsRequired
We already have a similar error for that:
ServiceError::LoggedInUserNotFound => StatusCode::UNAUTHORIZED, // 401But I suppose we don't need the ExtractLoggedInUser extractor anymore. I would use:
ServiceError::UnauthorizedActiognForGuests => StatusCode::UNAUTHORIZED, // 401In 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