Skip to content

RoleClaims has no unique constraint on (RoleId, ClaimType, ClaimValue): concurrent permission syncs insert duplicate rows #1435

Closed

Description

@marcelo-maciel

Problem

identity.RoleClaims has no uniqueness on (RoleId, ClaimType, ClaimValue). Its only index is on RoleId (IdentityDbContextModelSnapshot, entity FshRoleClaim), so nothing in the database stops the same permission from being stored twice on the same role.

Both writers that grant permissions to the built-in roles follow a read-then-insert pattern without a lock:

  • RolePermissionSyncer.SyncRoleAsync loads the role's permission claims, computes the missing ones and inserts them.
  • IdentityDbInitializer.AssignPermissionsToRoleAsync does the same during seeding.

When two API instances start at the same time after a release that registers a new permission, both read the role before either one commits, both see the permission as missing, and both insert it. The result is a duplicate row, with no exception and no log line that would point at it. The same applies when the seeder and the startup sync overlap on a fresh tenant.

Impact

Authorization is not affected today: the effective permission set is built as a set, the sync reads existing claims into a HashSet, and when a permission is later revoked the sync deletes every matching row. The cost is silent data drift in RoleClaims, which grows by one row per permission per concurrent start, and any future code that counts claims or relies on one row per grant would be wrong.

This came up in the review of #1426 (point 2, two instances starting at once). That PR handles the delete side of the race (DbUpdateConcurrencyException is caught and logged at Information); the insert side needs a schema change, so it was left out of that PR.

Proposed fix

  1. A migration that first removes existing duplicates (keeping the lowest Id per (RoleId, ClaimType, ClaimValue)) and then adds a unique index on (RoleId, ClaimType, ClaimValue). ClaimValue is nullable, and PostgreSQL treats NULLs as distinct in a unique index, so the null-valued claims the sync already removes do not block the index.
  2. The two writers treat a unique violation on that index (23505) the same way the sync treats a lost delete race: another writer already inserted the row, so the end state is correct and it is logged at Information, not as an error.
  3. An integration test that inserts the same permission twice on a role and asserts the second insert is rejected, plus a test that the sync survives the conflict.

Expand-then-contract is not needed here, because the index only rejects rows that should never have existed; but the dedupe step must run before the index is created, inside the same migration.

Activity

  1. added a commit that references this issue on Nov 3, 2026
    2c3add9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions