[VibeSWE]

Summary

Ports upstream parseablehq/parseable commit d1f478b ("add query param to list api keys (parseablehq#1833)"), cherry-picked with -x (it applied cleanly). It adds an optional privilege filter to GET /api/prism/v1/apikeys.

A key matches when any of its roles, direct or inherited from a user group, grants a privilege of the given kind.

• Resource-scoped privileges also match: reader on stream logs matches ?privilege=reader.
• Matching is by kind, not hierarchy: an admin-only key does not match reader.
• Without the parameter the list is unchanged.

Follow-up commits make these changes on top of upstream. The last round was written test-first: failing tests were committed before each fix (b53ac5c8).

• Param name. Upstream only accepts the misspelled ?priviledge=. privilege is now the canonical field, and priviledge is kept as a serde alias so upstream clients still work.

• Invalid values return 400. That covers unknown or differently cased values (owner, Reader, empty), which get an "unknown variant" message listing the accepted values. A repeated parameter, or both spellings at once, returns "duplicate field". None of these returns an empty list or a 500. No custom QueryConfig is registered, so actix's default 400 handler applies.

• Unknown parameter names are ignored. For example Privilege=reader or _=123 returns the unfiltered list. Rejecting them with deny_unknown_fields would break cache-busters, so this is documented instead.

• Group-inherited roles. API keys can be members of user groups (GroupUser has an apikey case). Upstream's filter only checked direct roles, so a key whose privilege came from a group disappeared from filtered results even though it was authorized. The filter now resolves the same direct and group roles that authorization does.

• superadmin. ?privilege=superadmin is accepted and matches keys whose roles (direct or group) hold SuperAdmin. Matching is still by kind, so admin does not match super-admin keys. Without this, super-admin keys, the most powerful kind, matched no filter value at all.

• Locks. Only when a filter is given, and following the documented lock order (user groups before roles), the handler works out the set of role names that grant the privilege and the set of groups that hold one of them. It releases both guards before going through the keys. A large key list or role therefore can't keep the global maps locked while it is scanned, which would block authorization checks for every tenant behind a queued writer.

• Refactor and tests. The handler's tenant lookup and filtering now live in tenant_entries, which the tests call directly using per-test tenants in the global RBAC maps. The tests cover:

  • no filter (every key returned, masked);
  • each privilege kind;
  • scoped privileges;
  • group-only roles, plus groups that reference a deleted role or don't exist;
  • two tenants with the same role names but different privileges;
  • a tenant with no role map;
  • query parsing under both spellings and invalid values (status code and message).

  I mutation-checked them: looking up roles in the wrong tenant, dropping group roles, or emptying the unfiltered list each make tests fail.

• HTTP-level test. One test goes through Server::get_apikeys_webscope() and the real auth middleware. It checks: no filter, ?privilege=admin, a bogus value (400), no session (401) and a non-admin (403). If the handler stops passing the filter through, the test fails (checked by breaking it on purpose).

• Test fixes.

  • The middleware api_key_auth_tests only added their admin role inside ROLES.get_or_init, so they relied on initialising ROLES before any other test. That race already existed on main, where the middleware and rbac tests together fail 2 of 5 runs. They now add the role whether or not the map already exists.
  • Every test that writes the shared RBAC maps (rbac::lock_order_tests, middleware::api_key_auth_tests, and the apikeys tests) now takes one #[cfg(test)] lock, rbac::map::TEST_SERIAL. The lock-order tests detect progress by watching for a queued writer on USERS, so an outside writer could make them fail spuriously or pass without testing anything.

Ramifications checked

• Routing and auth. The endpoint is still GET /api/prism/v1/apikeys, registered in Server::get_apikeys_webscope() and also used by QueryServer. It still requires Action::All, so a bad parameter returns 401 to anonymous callers, 403 to non-admins and 400 only to admins; nobody can probe it before auth. The handler still calls verify_admin, and the filter only narrows the admin's own tenant's keys.
• Frontend. client.ts#listApiKeys and the Team page's ApiKeysTab don't filter by privilege, so no code change is needed. frontend/docs/api-contracts.md now documents the parameter, and notes that each entry's roles lists direct roles only, so a key matched through a group may not show that role.
• Pre-existing escalation bugs found in this review are fixed separately in Reject protected role assignments for API keys and querier RBAC #67: API keys could be given internal or SuperAdmin roles, and so could querier users, and rejected role updates still changed live roles on Query nodes. Reject protected role assignments for API keys and querier RBAC #67 and this PR both touch apikeys.rs and the middleware test setup, so whichever merges second needs a small conflict resolution.

Verification

• cargo fmt --check ✅, and prettier --check on the docs file ✅
• cargo clippy -- -D warnings (the lib target CI gates on) ✅
• cargo clippy --all-targets: no warnings in the files this PR touches. It still reports warnings in test code elsewhere on main.
• cargo test --lib: 857 passed, 0 failed (P_BUILD_NO_UI=1). It was run repeatedly, including with --test-threads=1 and --test-threads=64.
• Reviewed adversarially by Sol 6.1 (correctness, security/API) and Opus 5.5 (code and test quality), before and after the fixes.

🤖 Generated with Claude Code

[VibeSWE]

Adversarial review of PR #60 (round 2, at a0b47035)

Six independent reviewers looked at this PR, each with one focus: three Opus 5.5 (concurrency and test state, RBAC parity, code and test quality) and three Sol 6.1 (HTTP contract end to end, security and multi-tenancy, regressions and integration). Every finding below was checked against the code at the PR head before it was posted.

Verdict: OK to merge after the medium finding is decided. No regressions, no auth bypass, no cross-tenant exposure.

Findings in this PR

Severity	Finding
Medium	Keys with a SuperAdmin role match no filter value (thread on the deserializer)
Low	A filtered list holds the global USER_GROUPS and ROLES read locks for the whole per-key scan (thread)
Low	The handler's wiring to the filter has no test (thread)
Low	The new test helpers write the shared RBAC maps without the serialisation rbac::lock_order_tests relies on (thread)
Nit	The lookup order in the tuple doesn't match the lock-order comment; one test name overstates what it tests (threads)
Nit	Expired keys still match a filter. That's consistent with the unfiltered list, which keeps them with expiresAt, but worth one line in the docs.
Nit	Nothing in the fork currently puts an API key into a user group (new_api_key starts with empty user_groups, and there is no membership endpoint), so the group-role branch is correct but unused in practice. It still keeps the filter in line with authorization if that changes.

Problems that predate this PR (worth their own issues, not blocking here)

• High: tenant admins can create super-admin API keys. validate_roles (src/handlers/http/apikeys.rs, unchanged here) only checks that each role exists. The native-user path rejects RoleType::Internal and deny_super_admin() roles (src/handlers/http/rbac.rs:170-178); the API key path skips that check. A default-tenant admin can POST /apikeys with roles: ["super-admin"] and get a key that carries Action::SuperAdmin. This is also what makes the medium finding reachable.
• High (from reading the code, not reproduced): rejected role updates still reach live roles on Query nodes. In src/handlers/http/modal/query/querier_role.rs:59-75, *role = body is written into the live role map before the deny_super_admin() check. A rejected PUT that adds SuperAdmin still changes the in-memory role. Standalone mode (role.rs) checks first.

Checked and fine

• Auth order: checked with 126 requests through Server::get_apikeys_webscope() and the real auth middleware. Callers get 401 when unauthenticated and 403 as non-admins, even with a malformed query. Admins get 400 for bad values and 200 otherwise. Bad values covered: mixed case, empty, +/space, repeated, invalid UTF-8 and a 60 KB value. The 400 body matches the endpoint's other validation errors and leaks nothing internal.
• Unfiltered behaviour: the JSON with no filter is identical to the pre-PR handler. Unknown parameter names, including priviledge%3Dreader, return the unfiltered list, as documented.
• Tenants: keys, groups and roles all come from the caller's credential-derived tenant. A tenant header can't redirect the listing, and super-admins get no cross-tenant reach.
• Authz parity: the filter matches authorization's role resolution: direct roles, then User.user_groups, then the tenant's groups, then the tenant's roles. Missing roles and groups are skipped the same way, protected makes no difference for API keys, and keys are not cached in sessions. The route is only served on Standalone and Query nodes, both of which load the same metadata.
• Lock order: USERS is released, then USER_GROUPS, then ROLES are each taken once. No production writer takes them in the reverse order.
• Merge onto current main (fd6216c8): clean. The merged tree passes 868 lib tests, fmt, clippy -D warnings and Prettier, and the frontend unit tests pass (95). upstream/main has no later changes to apikeys.rs.
• Repeated runs: the full cargo test --lib passed at --test-threads 1, 2, 4, 16 and 64 and by default ×3. Deliberately broken copies (role check always true, wrong tenant lookup, group roles dropped, unfiltered list emptied) are caught by the filter tests.
• Middleware test change: it is correct, and it fixes a flaky test that predates this PR. On main, the middleware and rbac tests together fail 2 of 5 runs, because rbac::lock_order_tests can create ROLES empty first. It's worth saying so in the PR description.
• Docs: the docs line and the PR description were checked against the code and are accurate.