#448 Move authorization logic to new layer #520
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7c1a08f to
5eb405a
Compare
5eb405a to
0cba420
Compare
0cba420 to
dfb5b8e
Compare
dfb5b8e to
37e3eb9
Compare
6506e68 to
90a82e0
Compare
90a82e0 to
c5bbf9f
Compare
c5bbf9f to
3144445
Compare
3144445 to
51c9757
Compare
51c9757 to
27c6b01
Compare
27c6b01 to
30a113d
Compare
30a113d to
b18da74
Compare
josecelano
reviewed
Mar 21, 2024
Member
josecelano
left a comment
There was a problem hiding this comment.
Alternative draft implementation where the service knows the needed role (not the handler).
src/services/authorization.rs
Outdated
Comment on lines
26
to
53
| pub async fn authorize_user(&self, user_id: UserId, admin_required: bool) -> Result<(), ServiceError> { | ||
| // Checks if the user exists in the database | ||
| let authorization_info = self | ||
| .user_authorization_repository | ||
| .get_user_authorization_from_id(&user_id) | ||
| .await?; | ||
|
|
||
| //If admin privilages are required, it checks if the user is an admin | ||
| if admin_required { | ||
| Self::authorize_admin_user(&authorization_info) | ||
| } else { | ||
| Ok(()) | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
Hi @mario-nt I think we should remove the admin_required parameter otherwise the handler has to know if the action must be performed by an admin. The main idea was removing the responsibility of knowing which role can do the task from the axum handlers.
This implementation extracts only part of the responsibility.
I have created a PR to show you what I had in mind for this issue:
It's a lot of changes but only because I want to test the service and I wanted to create mocks for the user repository. But the service itself it's very simple. The main problem is all contexts are coupled with the authorization service for the time being. But we were discussing that. I think it's OK for now.
45bc6a3 to
66e6686
Compare
1dfcbab to
9d8069d
Compare
565ab57 to
9d8069d
Compare
josecelano
added a commit
that referenced
this pull request
Jun 4, 2024
478e234 feat: [#448] new authorization service implemented in the other services (Mario) 9ebd7b5 feat: [#448] new authorization service (Mario) b1d5536 feat: [#448] new user repository trait (Mario) a2ce990 feat: [#448] added mockall dependency (Mario) Pull request description: Resolves #448. Replaces #520. ACKs for top commit: josecelano: ACK478e2349f159da7d08b60382fd80a3f236067637 Tree-SHA512: ec93e9d95ff8b934397bd1bb13cc76077b33d2abd962bad0b667b134768066ed33aced0b174830eea7947a0e9f53273fe062147ee15342eaf1db8a5dfaaa3cfd
Contributor
Author
|
Superseded by #614 |
Member
|
Hi @mario-nt can we close this? It looks pretty outdated. |
Contributor
Author
Member
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #448