Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Refactor auth code #11379

wants to merge 1 commit into from

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented Jun 18, 2025

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.

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.
@Turbo87 Turbo87 added C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear A-backend ⚙️ labels Jun 18, 2025
@Turbo87
Copy link
Member Author

Turbo87 commented Jun 18, 2025

@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 free to 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant