- Notifications
You must be signed in to change notification settings - Fork655
Refactor auth code#11379
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Draft
Turbo87 wants to merge1 commit intorust-lang:mainChoose a base branch fromTurbo87:auth-refactor
base:main
Could not load branches
Branch not found:{{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline, and old review comments may become outdated.
Draft
Refactor auth code#11379
Uh oh!
There was an error while loading.Please reload this page.
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
Up until this point our "auth" code was scattered all over the place and mixed up authentication (who are you?) and authorization (what are you allowed to do?) in multiple places:- Whether a user account was locked was checked as part of authentication, although it is more of an authorization concern.- Whether a user has a verified email address was checked in the individual endpoints, if at all.- Whether a user is an admin was also checked in the individual endpoints that were adjusted to support admin users.- Whether a user is an owner (or team member) of a crate was also checked in the individual endpoints, in various different ways.- Whether an API token has the right scopes was also checked as part of authentication, but it is an authorization concern.This commit centralizes these checks with a three step approach:1. In the first step the endpoint uses one of the `Credentials` structs as an axum request extractor to extract the raw credentials from the request metadata (user ID from session cookie, API token, Trusted Publishing token).2. The credentials are used to look up a corresponding user (or Trusted Publishing token) from the database.3. The authenticated entity is used to check for authorization of the requested operation.The second and third step are folded into one fn call (`.validate(permission)`) to make it harder to use an authenticated entity without knowing if they are authorized for the operation.While the auth code and the endpoints have changed quite a bit due to this refactoring, the test code is mostly the same except for a few minor details caused by using the request extractors now.
@rust-lang/crates-io I've opened this as a draft since I'm mostly looking for feedback on this idea. Please take a look and let me know if this is actually an improvement or not. I'm a little worried that I've over-complicated it, but I'm not quite sure if it can be simplified in a significant way. |
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Labels
A-backend ⚙️C-internal 🔧Category: Nonessential work that would make the codebase more consistent or clear
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.
Up until this point our "auth" code was scattered all over the place and mixed up authentication (who are you?) and authorization (what are you allowed to do?) in multiple places:
This commit centralizes these checks with a three step approach:
Credentials
structs as an axum request extractor to extract the raw credentials from the request metadata (user ID from session cookie, API token, Trusted Publishing token).The second and third step are folded into one fn call (
.validate(permission)
) to make it harder to use an authenticated entity without knowing if they are authorized for the operation.While the auth code and the endpoints have changed quite a bit due to this refactoring, the test code is mostly the same except for a few minor details caused by using the request extractors now.