fix(rbac): add unique constraints for role/user_role race (#4482)

[rakdutta]

closes #4482 - Adds database-level unique constraints to prevent duplicate RBAC roles and role assignments during concurrent bootstrap.

Problem

Multiple workers can create duplicate system roles (platform_admin, etc.) and role assignments when bootstrapping concurrently, causing MultipleResultsFound exceptions → 500 errors.

Solution

Two-layer defense:

1. Database constraints — Partial unique indexes on roles(name, scope) and user_roles (split for NULL/NOT NULL scope_id)
2. App handling — Savepoint pattern catches IntegrityError, refetches existing row, returns transparently

Behavior Change: Re-granting an expired assignment no longer raises an error

Previously, assign_role_to_user() raised ValueError("User already has this role assignment") even when the existing assignment had already expired, because the active-assignment check did not distinguish expired rows from valid ones.

New behavior: if an active assignment exists but is_expired() returns True, it is automatically soft-deleted (is_active = False) in-place before the new grant is created. This allows the re-grant to proceed cleanly without any manual revocation step.

A concurrent-race variant is also handled: if two processes race to soft-delete the expired row simultaneously, the process that loses the race refetches the current state and returns the winner's fresh assignment rather than raising.

Impact: callers that previously caught ValueError on re-grant of an expired role will no longer see that error — the call now succeeds and returns the new UserRole row. Any code that relied on that error to detect "assignment already exists" should instead check the returned object's expires_at field or use list_user_roles().

Changes

• Migration d21698ae4a19 — Deduplicates existing duplicate active rows (remapping user_roles.role_id to the kept role before deactivating duplicates, and preferring unexpired/most-recently-granted assignments), then adds partial unique indexes (PostgreSQL + SQLite)
• role_service.py — Updated create_role() and assign_role_to_user() with savepoint + IntegrityError retry handling; fixed W1203 logging violations (lazy %s formatting throughout)
• Tests — New tests/unit/mcpgateway/db/test_rbac_unique_constraints_migration.py covering dedup logic, role_id remapping, expiry-preference ordering, index idempotency, and downgrade
• CHANGELOG — ### Fixed entry added under [Unreleased]

Related

• Issue [BUG] RBAC role/user_role seeder race when fast-path skips advisory lock #4482
• PR fix(bootstrap): improve startup reliability for multi-replica deploys #4444 (bootstrap fast-path)

[rakdutta]

@gandhipratik203 can you please review this PR.

[gandhipratik203]

Thanks for the previous changes. Two smaller follow-ups I noticed:

• mcpgateway/alembic/versions/d21698ae4a19_add_rbac_unique_constraints_race_fix.py:62: Could we also remap user_roles.role_id from duplicate roles to the kept role before deactivating them? Otherwise active assignments tied to the deactivated role may stop showing up, since list_user_roles() filters on active joined roles.

• mcpgateway/alembic/versions/d21698ae4a19_add_rbac_unique_constraints_race_fix.py:112 / :147: Could the user-role dedupe prefer unexpired assignments over expired ones? Keeping only the oldest granted_at row may preserve an expired assignment and deactivate a still-valid duplicate.

[msureshkumar88]

Thanks for tackling this — the two-layer defense (DB partial unique indexes + savepoint/retry) is the right shape for this bug, and the migration correctly dedupes before constraining. I ran the migration SQL and the concurrency-handling path independently (including a real multi-threaded SQLite stress test) and the core mechanism holds up well.

Blocking

• Lint gate fails: pylint reports 6 new W1203 (logging-fstring-interpolation) violations in role_service.py at lines 239, 245, 253, 696, 705, 713 — the new log calls use f-strings where the rest of the file (and this PR's own unmodified lines) use lazy %s formatting, e.g. logger.info(f"Created role: {role.name}...") vs. the pre-existing logger.info("Created role: %s...", role.name, ...). This trips make pylint, part of the required make ruff bandit interrogate pylint verify gate. Straightforward fix — revert those calls to lazy formatting.

Suggestions (non-blocking)

• Migration has no dedicated test. d21698ae4a19...py is the largest new file here (285 lines, dialect-branching SQL + idempotency checks) but isn't exercised by any test. The repo has a clear convention for this (e.g. tests/unit/mcpgateway/db/test_token_uniqueness_migration.py) — importing the module and running upgrade()/downgrade() against a scratch schema. Worth adding, given this file has the most novel logic in the PR.
• Race tests are all single-threaded mocks. The 8 new "concurrent" tests simulate the race via patch.object + call counters rather than real concurrent threads/connections against the actual constraint. A single true multi-threaded (or multiprocess) test hammering create_role/assign_role_to_user against a real SQLite/Postgres unique index would give much stronger confidence this stays fixed under refactors.
• CHANGELOG.md isn't updated — this fixes a real production 500 under concurrent bootstrap, which seems worth a line given the repo tracks fixes of similar severity there.
• Minor: the PR description's "Solution" section doesn't mention that expired-but-active assignments are now auto soft-deleted and replaced on re-grant (previously blocked with ValueError). Small but real behavior change worth calling out explicitly.

Nothing security-sensitive stood out — checked the new ValueError messages that embed raw IntegrityError text and confirmed they're routed through safe_error_detail() in rbac.py, so no leak to non-debug callers.

[rakdutta]

@msureshkumar88 @gandhipratik203 — all review comments addressed, ready for re-review.

Blocking fix (msureshkumar88)

• Fixed 6 W1203 logging-fstring-interpolation violations in role_service.py (lines 239, 245, 253, 696, 705, 713) — converted to lazy %s formatting. pylint gate now passes at 10.00/10.

Review comments (gandhipratik203)

• d21698ae4a19: Added Step 1a — remaps user_roles.role_id to the kept role before deactivating duplicate roles, so list_user_roles() JOIN on roles.is_active stays intact.
• d21698ae4a19: Changed user_roles dedupe ordering from ORDER BY granted_at ASC to prefer unexpired rows first (expires_at IS NULL OR expires_at > NOW()), then newest granted_at DESC — avoids keeping an expired assignment over a still-valid one.

Non-blocking suggestions (msureshkumar88)

• Added tests/unit/mcpgateway/db/test_rbac_unique_constraints_migration.py — 22 tests covering dedup logic, role_id remapping, expiry-preference ordering, index idempotency, and downgrade (22/22 pass).
• Updated CHANGELOG.md [Unreleased] with a ### Fixed entry.
• PR description updated to call out the behavior change on expired-assignment re-grant.

Also fixed a down_revision pointing to the wrong ancestor (was e198602c3c1e, should be b6c7d8e9f0a1) which caused a duplicate-heads error in the upgrade validation CI job.

[msureshkumar88]

Thanks for addressing all the prior feedback — the remap-before-dedupe ordering, the unexpired-preference dedupe, the W1203 lint fixes, and the new migration test suite all check out. Ran the full test surface locally (migration tests, duplicate-handling integration tests, race-condition tests, role_service unit tests) and everything is green, and pylint on role_service.py is 10.00/10.

One thing to clean up before merge:

Required

CHANGELOG.md re-adds two entries that are already documented under the released [1.0.5] section.

The [Unreleased] section this PR adds to (commit e0f7f751a) includes:

• ### Added — POST /v1/tools/generate-schemas-from-openapi (#5142)
• ### Security — cross-environment JWT fix (GHSA-vgf8-3685-66j9)

Both are already fully documented under ## [1.0.5] - 2026-07-07 further down the same file (lines ~51 and ~74) — that release shipped 10 days ago. Re-adding them to [Unreleased] will misleadingly re-announce already-shipped features as still-unreleased on the next release cut, and they're unrelated to the RBAC fix this PR is about. Likely a stale-branch/rebase artifact.

Please trim [Unreleased] down to just the RBAC ### Fixed entry this PR actually adds.

Non-blocking notes (no action required)

• assign_role_to_user's IntegrityError refetch path: if the refetched "winner" row is itself already expired (both racers create near-simultaneously-expiring assignments), the code soft-deletes it and raises ValueError rather than retrying — surfaces as an HTTP 400 for a caller who did nothing wrong. Very low likelihood, just flagging for awareness.
• The new PostgreSQL-specific dedupe/remap SQL in the migration (UPDATE ... FROM ... JOIN ... with window functions) has no test coverage — all 22 new tests run against SQLite only. Matches existing repo convention (other migrations aren't PG-tested either), so not a PR-specific gap, but worth knowing since it's a data-mutating migration.
• AsyncMock imported but unused in two test functions in test_role_service_duplicate_handling.py.

Nothing else blocking — the core fix (partial unique indexes + savepoint/retry) is solid and well-tested.

[gandhipratik203]

Thanks @rakdutta, the pr is good to go. We have all the needed approvals. There is a merge conflict. Once it's rebased, we are good to merge.

[rakdutta]

@gandhipratik203 rebased

[gandhipratik203]

Thanks @rakdutta. The PR is in good shape now. @msureshkumar88, @Lang-Akshay We can prioratise this pr for the merge once the merge queue opens up after the release.