Reject duplicate granular scopes within one request

What does this MR do and why?

Part of #631824 (closed).

This MR makes Authz::GranularScopeService#add_granular_scopes reject duplicate granular scopes inside one request. The rule is "one granular scope per boundary" on a personal access token.

The old code validated each incoming scope against scopes the token already had. It looked in two places: saved scopes in granular_scopes, and join rows built earlier but not saved in personal_access_token_granular_scopes. All incoming scopes were validated first, and the join rows were built afterwards. So incoming scopes were never compared with each other.

incoming: [scope for project X, scope for project X]

check scope 1 against the token  ->  saved: none, built: none  ->  pass
check scope 2 against the token  ->  saved: none, built: none  ->  pass   (scope 1 is not built yet)
build row for scope 1
build row for scope 2
save  ->  two scopes for project X on one token

Every production caller makes exactly one call per token. These are Authn::PersonalAccessTokens::CreateGranularService on creation and PersonalAccessTokens::RotateService on rotation. So the rule never fired where it mattered.

A user could hit this in these ways:

  • Two separate granular_scopes entries in one REST request naming the same project.
  • Two instance entries in one REST request.
  • The same global ID listed twice over GraphQL, because that path loads each ID separately.

The result was redundant rows, not a security hole. Authorization unions the permissions of all matching scopes. It was still a violated invariant.

The old check had a second flaw. It compared on namespace_id alone. Standalone scopes (user, instance, all_memberships) all have a nil namespace. Across two calls the check was too strict. A token that already held an instance scope rejected a later user scope with "The token cannot have multiple instance-level granular scopes". That pair is valid, and the GraphQL request specs create exactly that pair. Within one call the same pair slipped through because of the first flaw.

The new code

This MR adds the pure class Authz::GranularScopes::UniquenessCheck in app/models/authz/granular_scopes/uniqueness_check.rb.

first_duplicate walks the incoming list in order. It remembers each key it has seen, seeded with the keys of the existing scopes. It returns the first incoming scope whose key was already seen.

The key is [access, namespace_id]. A namespace may appear at most once. Standalone scopes are keyed by access level instead. So one user and one instance scope coexist, but two instance scopes collide.

existing: []                      seen = {}
incoming: [X, X]

X  -> key [selected_memberships, X]  not seen  -> remember
X  -> key [selected_memberships, X]  seen      -> duplicate, stop

The service now collects the token's existing scopes once. It reads them through the join rows it builds on, personal_access_token_granular_scopes.to_a. It preloads the scope of each saved row with ActiveRecord::Associations::Preloader, so saved rows do not cost one query each.

It deliberately does not load the granular_scopes association. Loading a has_many :through inside the service left it cached as empty after the save. Callers that read token.granular_scopes afterwards would have seen nothing. The token creation service's event tracking does this.

Performance

Old: one SELECT ... EXISTS per incoming scope, plus one load of the join rows.

New: one load of the join rows, plus one preload query only when the token already has saved scopes. On token creation and rotation the token is new, so there are no saved rows and no preload.

Query counts

Measured with ActiveRecord::QueryRecorder around add_granular_scopes, with the old service loaded side by side from the base commit. The SELECT column isolates reads. Totals also include the inserts for the scopes and join rows, which are the same in both versions.

Scenario Old queries (of which SELECT) New queries (of which SELECT)
New token, 1 scope 7 (3) 6 (2)
New token, 3 scopes 15 (7) 12 (4)
New token, 5 scopes 23 (11) 18 (6)
Token with 3 saved scopes, add 1 more 7 (3) 7 (3)

The uniqueness check went from one EXISTS query per incoming scope to none, so creation saves one query per scope. The SELECTs that remain are the same in both versions: one load of the join rows, and one namespace lookup per scope from the model's organization validation on save. When the token already has saved scopes, the new preload query replaces the single EXISTS, so that count is unchanged.

Behaviour change

Intended and user-visible: duplicate entries in one request now return an error instead of creating two rows. This covers the same project named by two entries, and two instance entries. The first commit carries Changelog: fixed.

Not reachable in production: a token that already holds an instance scope can now be given a user scope in a later call. No production path adds scopes to an existing token in separate calls.

Rotation merges existing duplicates: production has 7 tokens with duplicate scopes, 4 of them still active. All are standalone duplicates, such as two instance scopes. Rotation would now fail on them, so PersonalAccessTokens::RotateService merges scopes that share an access level and namespace into one scope with their combined permissions. The rotated token keeps the same access, because authorization already combines every matching scope. GroupAccessTokens::RotateService and ProjectAccessTokens::RotateService inherit this. Duplicates on tokens that are never rotated are left as they are.

Reworded error: the standalone message said "The token cannot have multiple instance-level granular scopes". That is wrong for two user or two all_memberships scopes. It now says "The token cannot have multiple granular scopes with the same access level". The namespace message is unchanged.

References

How to set up and validate locally

Use a GDK instance. <token> is a personal access token with the api scope. <project_id> is a project the user is a member of. The granular_personal_access_tokens flag is on by default.

  1. Send two entries naming the same project. Expect HTTP 422 with "The token cannot have multiple granular scopes for the same namespace".
    curl --request POST --header "PRIVATE-TOKEN: <token>" --header "Content-Type: application/json" \
      --url "http://gdk.test:3000/api/v4/user/personal_access_tokens" \
      --data '{"name":"dup","granular_scopes":[{"access":"selected_memberships","permissions":["read_project"],"project_ids":[<project_id>]},{"access":"selected_memberships","permissions":["read_project"],"project_ids":[<project_id>]}]}'
  2. Send two instance entries. Expect HTTP 422 with "The token cannot have multiple granular scopes with the same access level".
    curl --request POST --header "PRIVATE-TOKEN: <token>" --header "Content-Type: application/json" \
      --url "http://gdk.test:3000/api/v4/user/personal_access_tokens" \
      --data '{"name":"dup-instance","granular_scopes":[{"access":"instance","permissions":["read_organization"]},{"access":"instance","permissions":["read_organization"]}]}'
  3. Send one instance entry and one user entry. Expect HTTP 201 with both scopes.
  4. Send one entry with two different projects. Expect HTTP 201 with two scopes.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Eugie Limpin

Merge request reports

Loading
Loading