feat(api-key): typed scopes with canonical values and route enforcement - #89
Conversation
Add KnownScope enum with five canonical values: chat:read, chat:write, providers:read, providers:write, admin. ApiKeyScope::parse now rejects unknown values with UnknownScope error. ApiKeyScope::parse_lenient is introduced for DB reads — accepts any non-empty value and emits a tracing::warn for unrecognised scopes. ManageApiKeys::create and update validate all scopes before touching the repository. Fixes pre-existing sha2 0.11 compilation breakage in login.rs, validate_session.rs, and auth.rs.
) Add required_scope(method, path) mapping routes under /v1/* to their canonical scope requirement. Add check_scope helper that allows requests when the subject holds the required scope or the admin superset scope, and rejects with HTTP 403 INSUFFICIENT_SCOPE otherwise. Thread method and path through evaluate_policy and client_api_policy. Update env-fallback credentials to use canonical scope names. Update all affected tests to use canonical scope values.
|
Warning Review limit reached
More reviews will be available in 31 minutes and 56 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds canonical API key scopes and strict parsing, allow-list fields (models/providers) throughout domain, persistence, use-cases, adapters, and middleware; enforces route-required scopes by HTTP method/path; adds request-level restriction checks; refactors token-hash hex encoding; updates tests and UI to use namespaced scopes. ChangesTyped API Key Scopes with Validation, Restrictions, and Enforcement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/infrastructure/auth-sqlite/src/lib.rs (1)
384-390:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: database reads use strict
parseinstead ofparse_lenient, breaking backward compatibility.Line 388 calls
ApiKeyScope::parse(scope), which rejects unknown scopes withUnknownScopeerror. This function is invoked byrow_to_subject(line 346) androw_to_record(line 374) when loading API keys from the database.Per the domain model contract,
ApiKeyScope::parse_lenientwas added specifically for "reading from the database" to preserve unknown/legacy scope values and emit warnings instead of failing. Using strictparsehere violates that contract and defeats the PR's backward compatibility goal (PR objective: "Preserve (but warn about) unknown existing DB scope values on read").Impact:
- Databases containing API keys with unknown scopes (e.g., legacy
"read","write", or custom values from before this PR) will fail to load with parse errors.- The
tracing::warn!inparse_lenientwill never fire.- Migration/upgrade path is broken for existing deployments.
🐛 Proposed fix: use lenient parsing for database reads
fn scopes_from_json(value: &str) -> Result<Vec<ApiKeyScope>, String> { let values = serde_json::from_str::<Vec<String>>(value).map_err(|error| error.to_string())?; - values - .iter() - .map(|scope| ApiKeyScope::parse(scope).map_err(|error| error.to_string())) - .collect() + Ok(values + .iter() + .map(|scope| ApiKeyScope::parse_lenient(scope)) + .collect()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/infrastructure/auth-sqlite/src/lib.rs` around lines 384 - 390, The database read helper scopes_from_json currently uses strict ApiKeyScope::parse which rejects unknown/legacy scopes; change it to call ApiKeyScope::parse_lenient so DB reads preserve unknown scope values and emit warnings instead of failing (this affects places that call scopes_from_json such as row_to_subject and row_to_record); update the mapping in scopes_from_json to invoke ApiKeyScope::parse_lenient(scope) and propagate errors as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/application/rook-usecases/src/auth/login.rs`:
- Around line 90-97: The manual byte-to-hex folding in the token creation (the
token_hash block using hasher.finalize and write!(s, "{b:02x}")) should be
extracted into a shared helper (e.g., digest_to_hex) and replaced everywhere
it’s duplicated (crates/application/rook-usecases/src/auth/login.rs,
validate_session.rs, and
crates/infrastructure/transport-axum/src/handlers/auth.rs); implement
digest_to_hex to take the finalized digest bytes and return a hex string using
the hex crate (hex::encode) — add hex to Cargo.toml, update imports to remove
the local write::Use, and call digest_to_hex(token_bytes) (or similar) in place
of the fold to centralize encoding.
In `@crates/application/rook-usecases/src/manage_api_keys.rs`:
- Around line 186-203: The match in validate_scopes handling ApiKeyScope::parse
includes an unreachable ApiKeyValidationError::InvalidTier arm; remove that arm
so the match only handles ApiKeyValidationError::UnknownScope and
ApiKeyValidationError::EmptyScope (or replace the unreachable arm with a
wildcard that forwards to ManageApiKeysError::Validation if you prefer
future-proofing), keeping the rest of validate_scopes and the ApiKeyScope::parse
call unchanged.
In `@crates/domain/rook-core/src/api_key.rs`:
- Around line 83-89: parse_lenient currently silently accepts an empty trimmed
value; update it so that after trimming you check for value.is_empty() and emit
a clear warning (e.g., tracing::warn with scope = "<empty>" or similar) before
returning Self so empty scopes are no longer silently accepted; modify the
function parse_lenient (and reference ApiKeyScope/ApiKeyScope::parse behavior in
comments if helpful) to log when value.is_empty() and still return
Self(value.into()) to preserve lenient behavior but avoid silent acceptance.
---
Outside diff comments:
In `@crates/infrastructure/auth-sqlite/src/lib.rs`:
- Around line 384-390: The database read helper scopes_from_json currently uses
strict ApiKeyScope::parse which rejects unknown/legacy scopes; change it to call
ApiKeyScope::parse_lenient so DB reads preserve unknown scope values and emit
warnings instead of failing (this affects places that call scopes_from_json such
as row_to_subject and row_to_record); update the mapping in scopes_from_json to
invoke ApiKeyScope::parse_lenient(scope) and propagate errors as before.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ce895e53-2328-435f-a1de-8aec418ed510
📒 Files selected for processing (10)
crates/application/rook-usecases/src/auth/login.rscrates/application/rook-usecases/src/auth/validate_session.rscrates/application/rook-usecases/src/authenticate_client_api.rscrates/application/rook-usecases/src/manage_api_keys.rscrates/domain/rook-core/src/api_key.rscrates/domain/rook-core/src/lib.rscrates/infrastructure/auth-sqlite/src/lib.rscrates/infrastructure/transport-axum/src/authz.rscrates/infrastructure/transport-axum/src/handlers/auth.rscrates/infrastructure/transport-axum/tests/api_key_routes.rs
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 8 file(s) based on 3 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 8 file(s) based on 3 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
…uting (#85, #86) (#90) * chore(deps-rust)(deps): bump axum-test from 15.7.4 to 20.1.0 (#78) Bumps [axum-test](https://github.com/JosephLenton/axum-test) from 15.7.4 to 20.1.0. - [Release notes](https://github.com/JosephLenton/axum-test/releases) - [Commits](https://github.com/JosephLenton/axum-test/commits) --- updated-dependencies: - dependency-name: axum-test dependency-version: 20.1.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * feat(api-key): add allowed_models and allowed_providers restriction fields (#85) * feat(routing): enforce allowed_models and allowed_providers restrictions (#86) --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
# Conflicts: # Cargo.lock
…hat:write The dashboard was still emitting pre-#83 scope values ('read', 'write') in the create/edit modals, which the backend now rejects with 400 'unknown API key scope'. This was surfaced by 'just ci-local' failing the Playwright e2e suite after #83/#84 landed. Without this fix, every dashboard user creating an API key via the UI would have hit the same 400 error. - ApiKeysView.vue: dropdown values updated to canonical scopes - api-keys.spec.ts: helper defaults + 2 call sites updated; UI selector tightened to '^chat read$' to avoid accidental matches
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/infrastructure/auth-sqlite/src/lib.rs (1)
405-410:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
parse_lenienton the SQLite read path.
scopes_from_jsonstill calls strictApiKeyScope::parse, so any preexisting row with a legacy scope value likereadnow becomes unreadable and will failfind,list, andfind_active_by_hashinstead of being preserved with a warning. That breaks the compatibility contract introduced byApiKeyScope::parse_lenient.Suggested fix
fn scopes_from_json(value: &str) -> Result<Vec<ApiKeyScope>, String> { let values = serde_json::from_str::<Vec<String>>(value).map_err(|error| error.to_string())?; - values - .iter() - .map(|scope| ApiKeyScope::parse(scope).map_err(|error| error.to_string())) - .collect() + Ok(values + .iter() + .map(|scope| ApiKeyScope::parse_lenient(scope)) + .collect()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/infrastructure/auth-sqlite/src/lib.rs` around lines 405 - 410, scopes_from_json currently uses the strict ApiKeyScope::parse which will error on legacy values; change it to call ApiKeyScope::parse_lenient when mapping each scope string (in the scopes_from_json function) so legacy/unknown scope strings are preserved/normalized rather than failing reads—replace ApiKeyScope::parse with ApiKeyScope::parse_lenient and keep the existing error-to-string mapping behavior.crates/infrastructure/transport-axum/src/authz.rs (2)
620-639:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFallback
/v1/*scope mapping under-enforces write routes.The default branch always returns
chat:read, so any non-GET endpoint added under/v1/*outside/v1/chat/*and/v1/providers*becomes callable by a read-only key. That breaks the new route-level authz contract. Default the fallback by method instead:GET => chat:read, everything else =>chat:write.Suggested fix
fn required_scope(method: &Method, path: &str) -> Option<&'static str> { if !path.starts_with("/v1/") { return None; } if path.starts_with("/v1/providers/") || path.starts_with("/v1/providers") { return if *method == Method::GET { Some("providers:read") } else { Some("providers:write") }; } if path.starts_with("/v1/chat/") { return match *method { Method::GET => Some("chat:read"), _ => Some("chat:write"), }; } - // GET /v1/models* and all other /v1/* default to chat:read - Some("chat:read") + match *method { + Method::GET => Some("chat:read"), + _ => Some("chat:write"), + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/infrastructure/transport-axum/src/authz.rs` around lines 620 - 639, The fallback in required_scope incorrectly returns Some("chat:read") for all /v1/* paths, letting non-GET routes be authorized with read-only keys; update required_scope (the function handling Method and path) so the default branch inspects the HTTP method: return Some("chat:read") for Method::GET and Some("chat:write") for all other methods (i.e., non-GET), preserving existing special-cases for /v1/providers* and /v1/chat/*.
742-775:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDashboard redirects weren't updated for session-era auth failures.
management_policy()now rejects stale or revoked sessions withSESSION_NOT_FOUND, but the dashboard redirect path still only handles the old token codes. As written,/dashboard/*with an expired session returns a 401 JSON body instead of redirecting back to/login.Suggested fix
if route_class == AuthTier::Management && path.starts_with("/dashboard/") && matches!( outcome.code, - Some("MISSING_AUTH_TOKEN" | "INVALID_TOKEN" | "TOKEN_EXPIRED") + Some( + "MISSING_AUTH_TOKEN" + | "INVALID_TOKEN" + | "TOKEN_EXPIRED" + | "SESSION_NOT_FOUND" + ) ) {Also applies to: 860-870
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/infrastructure/transport-axum/src/authz.rs` around lines 742 - 775, management_policy currently rejects stale/revoked sessions with "SESSION_NOT_FOUND", but the dashboard redirect logic only recognizes the older token-era rejection codes and thus returns a 401 JSON instead of redirecting; change behavior so dashboard access triggers a redirect to /login by either (A) returning the same rejection code/string that the dashboard redirect logic already handles (i.e., replace "SESSION_NOT_FOUND" in management_policy's AuthOutcome::reject call with the token-era rejection identifier), or (B) update the dashboard redirect mapping to treat "SESSION_NOT_FOUND" the same as the missing/expired token codes; apply the same fix to the other analogous session-policy function noted in the diff (the other policy that handles session/token validation) so both code paths redirect to /login.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/infrastructure/transport-axum/src/routes.rs`:
- Around line 179-180: Streamed request paths (e.g., chat_completions_stream and
anthropic_messages_stream) currently bypass the new forbidden/rate-limit HTTP
error mapping because they return into SSE helpers; change the startup flow so
route_request.execute_stream_with_format (and similar calls) are matched first
and map Err variants to the same HTTP responses as the non-stream branches: on
Ok(stream) continue to the SSE helper, on Err(e) if e.is_forbidden() return the
same 403 response shape, on Err(e) if e.is_rate_limited() return the same
429/headers, and on other Err(e) fall back to the existing SSE error handling;
apply this pattern for execute_stream_with_format invocations referenced in
chat_completions_stream, anthropic_messages_stream and the other listed stream
call sites so streaming and non-streaming requests share identical
auth/rate-limit behavior.
- Around line 136-168: The function restrictions_from_headers currently treats
missing or non-UTF8 x-authz-allowed-models / x-authz-allowed-providers as an
empty Vec (unrestricted); change it to fail closed by returning a Result (or
Option) instead of ApiKeyRestrictions so that missing or to_str().ok()==None is
treated as an error (authz context missing) rather than defaulting to
unrestricted; update restrictions_from_headers to validate presence and UTF-8 of
both headers ("x-authz-allowed-models" and "x-authz-allowed-providers"), parse
them into ApiKeyRestrictions only on success, and propagate the error type to
callers (adjust call sites that expect restrictions_from_headers to handle the
error and reject the request).
---
Outside diff comments:
In `@crates/infrastructure/auth-sqlite/src/lib.rs`:
- Around line 405-410: scopes_from_json currently uses the strict
ApiKeyScope::parse which will error on legacy values; change it to call
ApiKeyScope::parse_lenient when mapping each scope string (in the
scopes_from_json function) so legacy/unknown scope strings are
preserved/normalized rather than failing reads—replace ApiKeyScope::parse with
ApiKeyScope::parse_lenient and keep the existing error-to-string mapping
behavior.
In `@crates/infrastructure/transport-axum/src/authz.rs`:
- Around line 620-639: The fallback in required_scope incorrectly returns
Some("chat:read") for all /v1/* paths, letting non-GET routes be authorized with
read-only keys; update required_scope (the function handling Method and path) so
the default branch inspects the HTTP method: return Some("chat:read") for
Method::GET and Some("chat:write") for all other methods (i.e., non-GET),
preserving existing special-cases for /v1/providers* and /v1/chat/*.
- Around line 742-775: management_policy currently rejects stale/revoked
sessions with "SESSION_NOT_FOUND", but the dashboard redirect logic only
recognizes the older token-era rejection codes and thus returns a 401 JSON
instead of redirecting; change behavior so dashboard access triggers a redirect
to /login by either (A) returning the same rejection code/string that the
dashboard redirect logic already handles (i.e., replace "SESSION_NOT_FOUND" in
management_policy's AuthOutcome::reject call with the token-era rejection
identifier), or (B) update the dashboard redirect mapping to treat
"SESSION_NOT_FOUND" the same as the missing/expired token codes; apply the same
fix to the other analogous session-policy function noted in the diff (the other
policy that handles session/token validation) so both code paths redirect to
/login.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: be255ae3-becf-4301-97c2-0729e9a6467e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
apps/rook/dashboard/e2e/api-keys.spec.tsapps/rook/dashboard/src/views/ApiKeysView.vuecrates/application/rook-usecases/Cargo.tomlcrates/application/rook-usecases/src/auth/bootstrap_status.rscrates/application/rook-usecases/src/auth/login.rscrates/application/rook-usecases/src/auth/validate_session.rscrates/application/rook-usecases/src/authenticate_client_api.rscrates/application/rook-usecases/src/manage_api_keys.rscrates/application/rook-usecases/src/route_request.rscrates/application/rook-usecases/src/router_impl.rscrates/domain/rook-core/src/api_key.rscrates/domain/rook-core/src/model.rscrates/domain/shared-kernel/src/error.rscrates/infrastructure/auth-sqlite/src/lib.rscrates/infrastructure/db-migration/src/migrations/V1__allowed_models_providers.sqlcrates/infrastructure/providers-anthropic/tests/provider.rscrates/infrastructure/providers-gemini/tests/provider.rscrates/infrastructure/providers-groq/tests/provider.rscrates/infrastructure/providers-ollama/tests/provider.rscrates/infrastructure/providers-openai/tests/provider.rscrates/infrastructure/transport-axum/Cargo.tomlcrates/infrastructure/transport-axum/src/anthropic_adapter.rscrates/infrastructure/transport-axum/src/authz.rscrates/infrastructure/transport-axum/src/format_registry.rscrates/infrastructure/transport-axum/src/handlers/api_key.rscrates/infrastructure/transport-axum/src/handlers/auth.rscrates/infrastructure/transport-axum/src/openai_adapter.rscrates/infrastructure/transport-axum/src/routes.rscrates/infrastructure/transport-axum/tests/api_key_routes.rscrates/infrastructure/transport-axum/tests/format_translation_integration.rs
Four pre-existing issues caught by inline review of the PR-A stack. Finding 5 (SESSION_NOT_FOUND redirect) was verified invalid and skipped — the dashboard redirect is driven by currentUser, not by rejection codes. ## Finding 1: stream paths bypassed forbidden/rate-limit HTTP mapping chat_completions_stream and anthropic_messages_stream were returning SSE 200 with a generic internal_error event when the upstream execute_stream_with_format returned a forbidden or rate_limited error. This means a model-restricted key streaming chat completions got 200 + a confusing SSE error event instead of a clean HTTP 403. Added Err-arms for is_forbidden() and is_rate_limited() in both stream handlers so streaming and non-streaming requests share identical auth/rate-limit behavior. New helpers map_forbidden_openai and map_rate_limited return typed HttpError for the IntoResponse path. ## Finding 2: restrictions_from_headers failed open on missing headers The function used unwrap_or_default() on header lookups, so a missing x-authz-allowed-models or x-authz-allowed-providers header was silently treated as 'unrestricted'. The authz middleware must always stamp these headers, so a missing header indicates either a routing bug or a middleware bypass — both should be loud, not silent. Restructured into a parse_csv_header helper that returns Result<Vec<String>, HttpError> and propagates AUTHZ_HEADER_MISSING or AUTHZ_HEADER_INVALID 500 responses. Empty header value (public subject) still maps to empty Vec, which the domain treats as unrestricted. ## Finding 3: scopes_from_json rejected pre-#83 legacy scope strings auth-sqlite used ApiKeyScope::parse (strict) in scopes_from_json, which rejects any unknown scope string. Existing API keys created before #83 with legacy values ('read', 'write') would fail to load. Switched to ApiKeyScope::parse_lenient, which is the documented method for reading from the database (accepts unknowns, logs warning). Added regression test read_key_with_legacy_scope_string_is_preserved. ## Finding 4: required_scope fallback allowed POST with read-only key required_scope returned Some("chat:read") for ANY /v1/* path that wasn't /v1/providers/* or /v1/chat/* — regardless of HTTP method. This meant a key with only the chat:read scope could hit POST /v1/messages (Anthropic) and pass the authz check, then rely on downstream luck. Updated the fallback to inspect the method: GET → chat:read, all others → chat:write. The special-cases for /v1/providers* and /v1/chat/* are preserved. Added regression test client_api_with_chat_read_scope_rejected_on_post_to_messages.
Summary
Implements issues #83 and #84 — the first two slices of the API key scope system.
Changes
#83 — Typed
ApiKeyScopewith canonical scope valuesKnownScopeenum:ChatRead,ChatWrite,ProvidersRead,ProvidersWrite,AdminApiKeyScope::parsenow rejects unknown values withUnknownScopeerrorApiKeyScope::parse_lenientfor DB reads — preserves unknown values, emitstracing::warnManageApiKeys::createandupdatevalidate all scopes before writinglogin.rs,validate_session.rs,auth.rs#84 — Scope enforcement per route class
required_scope(method, path)mapping/v1/*routes to canonical scope requirementscheck_scopehelper: allowsadmin(superset), required scope, rejects with HTTP 403INSUFFICIENT_SCOPEotherwisemethodandpaththroughevaluate_policyandclient_api_policychat:read,chat:write)Test results
Closes
Closes #83
Closes #84