[VibeSWE]

Summary

Fixes privilege-escalation paths found in the review of #60. All of them existed before #60 and were fixed test-first: each has a failing test committed before its fix.

1. API keys could be given internal or SuperAdmin roles. POST /api/prism/v1/apikeys → validate_roles only checked that each role exists. The native-user path in rbac.rs rejects roles that are RoleType::Internal or hold SuperAdmin, and roles held by a protected user; this path didn't. A tenant admin could create a key with roles: ["super-admin"], which carries Action::SuperAdmin. validate_roles now returns RBACError::ProtectedRole (400) for those roles. This runs after the unknown-role check and before the duplicate-name check and any storage access.
2. The querier user endpoints had the same gaps. In modal/query/querier_rbac.rs:
   • post_user and add_roles_to_user now reject internal, SuperAdmin and protected-user-held roles with ProtectedRole (400).
   • add_roles_to_user, remove_roles_from_user and delete_user reject a protected target user with ProtectedUser (400).
   • add_roles_to_user runs these checks before the unknown-role check, in the same order as standalone, so both servers answer a mixed request the same way.
   • In post_user the update lock is now taken before validation.
3. Rejected role updates still changed live roles on Query nodes. querier_role::put wrote *role = body into the live role map before the deny_super_admin() check, so a rejected PUT that added SuperAdmin still changed the in-memory role that authorization reads. It also never rejected a body marked internal. It now works like standalone role::put:
   • it rejects an internal or SuperAdmin body before touching anything;
   • it checks whether the existing role is internal with a read-only lookup;
   • it only updates the live map after put_metadata succeeds;
   • it takes UPDATE_LOCK, which Standalone already did, and releases it before the cluster sync.

The protected-user checks use a new Users::any_role_held_by_protected_user helper (rbac/mod.rs). It takes the USERS lock, so the API key path releases its ROLES guard first, per the lock order.

Behaviour change: a real super-admin creating a key with roles: ["super-admin"] now gets 400, just as standalone post_user already does. Existing keys holding these roles keep working.

Tests

• API keys: validate_roles rejects internal roles, SuperAdmin roles and roles held by a protected user as separate cases, and still accepts ordinary roles. A handler-level test has a tenant admin try to create a super-admin key.
• Querier user endpoints: each new rejection has a test, including tests that mix in an unknown role to pin the check order. Users' roles stay unchanged.
• Querier role update: a rejected SuperAdmin body, a rejected internal body, and an update to an existing internal role each leave the live role unchanged.
• Helper: unprotected holders, disjoint roles, other tenants, and no-tenant meaning the default tenant.
• Tests that write the RBAC maps hold rbac::map::TEST_SERIAL.
• Some querier tests can only catch a removed check by the test binary exiting with code 2 (the handler reaches storage, which initialises PARSEABLE from the test argv). The module comments explain this.

Verification (at 8988e955)

• cargo fmt --check ✅
• cargo clippy -- -D warnings ✅
• cargo test --lib (P_BUILD_NO_UI=1): 1018 passed, 0 failed, 2 ignored
• Mutation testing: every protected check in validate_roles, add_roles_to_user and the helper is caught by an assertion when removed or reordered.

Notes

• Where SuperAdmin roles exist: the built-in super-admin role only exists in the default tenant. In other tenants, the API key escalation needs a role holding SuperAdmin to exist there, and the querier role-update bug was how such a role could get into memory.
• Follow-ups not in this PR: ingestor sync endpoints accept protected roles when CLUSTER_SECRET is unset; querier role DELETE doesn't take UPDATE_LOCK; role syncs can apply out of order; a staging-write failure in put_metadata leaves memory stale; querier post_gen_password lacks the protected-user check; demo mode (frontend/src/lib/demo.ts) is fixed in a separate PR.

🤖 Generated with Claude Code

---

^{Need help on this PR? Tag @codesmith-bot with what you need.}

[VibeSWE]

Adversarial review of PR #67 (at 36af50e5)

Three independent reviewers: two Sol 6.1 (authorization completeness; concurrency and state consistency) and one Opus 5.5 (regressions, HTTP contract, merge correctness). The Opus test-quality reviewer failed twice for infrastructure reasons, so its mutation testing was run directly instead (results below). Every finding was checked against the code at the PR head before posting.

Verdict: no regressions and no bypass of the new checks. Four small in-scope items (threads below) plus a description update. The bigger problems found are all pre-existing and are listed separately.

Findings in this PR

Severity	Finding
Low	The "existing role is internal" check in querier_role::put has no test; removing it passes every test (thread)
Low	The querier user endpoints and API key creation still skip standalone's protected-user checks (thread)
Low	add_roles_to_user on Query nodes checks unknown roles before protected roles, the reverse of standalone (thread)
Nit	The querier tests catch a regression by crashing the test binary, not by failing an assertion (thread)
Low	The PR description is out of date after the merge with #60, and doesn't say that super-admin callers are now also rejected

Description details:

• The "Middleware test fix" bullet now belongs to #60 (middleware.rs is no longer in this diff).
• The merge-conflict note is resolved (9187a42d).
• The verification numbers predate the merge.
• Not stated: a real super-admin creating a key with roles: ["super-admin"] now gets 400, just as standalone post_user already does. Existing keys holding these roles keep working.

Mutation testing

Mutation	Result
API keys: remove the protected-role check	❌ caught by 3 role_escalation tests
API keys: keep only the SuperAdmin half / only the Internal half	❌ caught (rejects_internal_role / rejects_super_admin_privilege_role + handler test)
Querier post_user: remove the check	❌ caught, but only by the test binary exiting with code 2 (see Nit thread)
Querier add_roles_to_user: remove the check / keep only SuperAdmin	❌ caught, same way
querier_role::put: remove the internal-body check	❌ caught (rejected_internal_update_leaves_live_role_unchanged)
querier_role::put: remove the SuperAdmin-body check	❌ caught, by the test binary exiting with code 2
querier_role::put: write the live map before the checks (the original bug)	❌ caught by both querier role tests
querier_role::put: remove the existing-internal-role check	✅ survived (thread)

Problems that predate this PR (not blocking; follow-ups)

• High: the ingestor sync endpoints accept protected roles from ordinary admins.
  • IntraClusterRequest lets every request through when CLUSTER_SECRET is unset (middleware.rs:796), and nothing outside tests ever sets it in this fork.
  • That leaves PATCH /user/{id}/role/sync/add and PUT /role/{name}/sync on ingestors behind only the ordinary PutUserRoles/PutRole authorization. Neither handler rejects internal or SuperAdmin roles (ingestor_rbac.rs:107, ingestor_role.rs:52).
  • Fixing this properly means enforcing a cluster secret, which changes deployment requirements, so it needs its own decision.
• Medium: querier role DELETE never takes UPDATE_LOCK (querier_role.rs:146); standalone does (role.rs:155). It can overwrite a concurrent post_user's metadata write.
• Medium: role syncs to ingestors can be applied out of order. put releases UPDATE_LOCK before sync_role_update, and receivers install whatever arrives, so two quick PUTs can leave ingestors on the older role. Failed syncs are only logged.
• Medium: a staging-write failure in put_metadata leaves durable metadata changed while memory and sessions keep the old state.
• Low: querier post_gen_password skips the protected-user check that standalone has (rbac.rs:243), and it reads metadata before taking the lock. It's Low because nothing in this fork creates protected users.
• Nit: frontend/src/lib/demo.ts still accepts internal and SuperAdmin roles for keys and users, so demo mode allows what the backend now rejects.

Checked and fine

• HTTP contract:
  • Every new rejection returns 400.
  • API key check order is unknown role → protected → 409 duplicate → storage.
  • 401/403 still come before any of these checks.
  • The frontend's role picker already hides internal roles.
• querier_role::put vs standalone role::put: same body checks and errors, and the live map is written only after put_metadata.
• Locks and concurrency:
  • UPDATE_LOCK is held through persistence, publication and session refresh, then dropped before the sync.
  • No re-entrant UPDATE_LOCK acquisition, and no parking_lot guard held across an .await.
  • Session refresh follows the documented SESSIONS → USERS → ROLES order.
• Tenants: validation uses the authenticated tenant, and the tenant header can't redirect it.
• Merge 9187a42d: it keeps #60's filter and tests, the role escalation tests, and main's middleware setup, with nothing dropped or duplicated. 36af50e5 only adds TEST_SERIAL guards.
• Local run: fmt and clippy are clean, and cargo test --lib passed 1005 tests in each of two runs.

[VibeSWE]

Round 2 adversarial review (at ad4b09d2, fixes now in 8988e955)

One Sol 6.1 and one Opus 5.5 reviewer read the four fix commits against the whole PR. Neither found anything blocking. Both confirmed:

• validate_roles drops its ROLES guard before the helper takes USERS;
• querier add_roles_to_user now checks in the same order as standalone;
• every new rejection is a 400, and every new map-writing test holds TEST_SERIAL.

Findings, all fixed before posting:

Severity	Finding	Fix
Low	Dropping user.protected && from the new helper, or mishandling the default tenant, passed every test	7c4630f9: helper tests for unprotected holders, disjoint roles, other tenants, and no tenant = default tenant. Both mutations now fail
Low	Querier delete_user and remove_roles_from_user still lacked standalone's protected-user check	8988e955, with red tests first
Nit	Querier post_user returns its 400s before reading storage; standalone reads metadata first	Kept: it was already so for the existing checks and is the safer order

Mutation runs, all caught by assertions: removing the protected-holder check in validate_roles; disabling the Internal/SuperAdmin, protected-holder and protected-target checks in querier add_roles_to_user; the two helper mutations above.

Local at 8988e955: fmt and clippy -- -D warnings clean; cargo test --lib 1018 passed, 0 failed.