Skip to content

feat(api-key): add allowed_models/allowed_providers and enforce in routing (#85, #86) - #90

Merged
yacosta738 merged 5 commits into
feat/api-key-scope-enforcementfrom
feat/allowed-models-providers
Jun 2, 2026
Merged

yacosta738 merged 5 commits into
feat/api-key-scope-enforcementfrom
feat/allowed-models-providers

Conversation

@yacosta738

Copy link
Copy Markdown
Contributor

Summary

Implements two issues from Phase 2: Auth & Provider Management:

Changes

Data layer (#85)

  • ApiKeyRecord gains allowed_models: Vec<ModelId> and allowed_providers: Vec<ProviderId> (empty = unrestricted)
  • Migration V1__allowed_models_providers.sql adds two TEXT columns to api_keys (default '[]')
  • CreateApiKeyRequest / UpdateApiKeyRequest accept these fields
  • POST /api/api-keys, PUT /api/api-keys/:id, GET /api/api-keys, GET /api/api-keys/:id pass them through

Enforcement layer (#86)

  • CortexError::forbidden() + forbidden_code() distinguish model_not_allowed vs provider_not_allowed
  • RouteRequest checks model restriction before any provider interaction, and provider restriction after router.select()
  • Same checks apply to execute_stream
  • HTTP 403 returned for both error codes (OpenAI handler returns JSON with code; Anthropic handler returns plain text matching its existing style)
  • Authz middleware stamps new x-authz-allowed-models / x-authz-allowed-providers headers from ApiKeySubject so handlers can hydrate CompletionRequest.restrictions

Testing

  • 343 tests passing, 0 failed (full workspace)
  • 7 new unit tests in route_request.rs covering model/provider restrictions for both execute and execute_stream
  • 3 new tests in auth-sqlite for create/update with restrictions
  • clippy --workspace --all-targets -- -D warnings clean

Architecture

  • Restrictions live on ApiKeyRecord (domain) → DTOs (transport) → wire format via ApiKeySubject
  • Enforcement happens in the use case (RouteRequest) so it applies regardless of which handler invokes it (OpenAI, Anthropic, etc.)
  • Headers-based transport from authz to handlers keeps the use case signature unchanged (the existing CompletionRequest already had restrictions field with #[serde(default)] so internal callers are unaffected)

Notes

Closes #85, closes #86

dependabot Bot and others added 5 commits June 2, 2026 09:03
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>
@coderabbitai

coderabbitai Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@yacosta738, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 14 minutes and 38 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5fea54fd-e69b-4f21-859d-73e7f3735627

📥 Commits

Reviewing files that changed from the base of the PR and between 76bbb96 and d4740cb.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (29)
  • crates/application/rook-usecases/Cargo.toml
  • crates/application/rook-usecases/src/auth/bootstrap_status.rs
  • crates/application/rook-usecases/src/auth/login.rs
  • crates/application/rook-usecases/src/auth/validate_session.rs
  • crates/application/rook-usecases/src/authenticate_client_api.rs
  • crates/application/rook-usecases/src/manage_api_keys.rs
  • crates/application/rook-usecases/src/route_request.rs
  • crates/application/rook-usecases/src/router_impl.rs
  • crates/domain/rook-core/src/api_key.rs
  • crates/domain/rook-core/src/lib.rs
  • crates/domain/rook-core/src/model.rs
  • crates/domain/shared-kernel/src/error.rs
  • crates/infrastructure/auth-sqlite/src/lib.rs
  • crates/infrastructure/db-migration/src/migrations/V1__allowed_models_providers.sql
  • crates/infrastructure/providers-anthropic/tests/provider.rs
  • crates/infrastructure/providers-gemini/tests/provider.rs
  • crates/infrastructure/providers-groq/tests/provider.rs
  • crates/infrastructure/providers-ollama/tests/provider.rs
  • crates/infrastructure/providers-openai/tests/provider.rs
  • crates/infrastructure/transport-axum/Cargo.toml
  • crates/infrastructure/transport-axum/src/anthropic_adapter.rs
  • crates/infrastructure/transport-axum/src/authz.rs
  • crates/infrastructure/transport-axum/src/format_registry.rs
  • crates/infrastructure/transport-axum/src/handlers/api_key.rs
  • crates/infrastructure/transport-axum/src/handlers/auth.rs
  • crates/infrastructure/transport-axum/src/openai_adapter.rs
  • crates/infrastructure/transport-axum/src/routes.rs
  • crates/infrastructure/transport-axum/tests/api_key_routes.rs
  • crates/infrastructure/transport-axum/tests/format_translation_integration.rs
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/allowed-models-providers

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added the area/testing Tests and testing infrastructure label Jun 2, 2026
@socket-security

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedcargo/​hex@​0.4.310010093100100

View full report

@yacosta738
yacosta738 changed the base branch from main to feat/api-key-scope-enforcement June 2, 2026 08:12
@yacosta738
yacosta738 merged commit 424994b into feat/api-key-scope-enforcement Jun 2, 2026
12 checks passed
@yacosta738
yacosta738 deleted the feat/allowed-models-providers branch June 2, 2026 08:12
yacosta738 added a commit that referenced this pull request Jun 2, 2026
…nt (#89)

* feat(api-key): typed ApiKeyScope with canonical scope values (#83)

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.

* feat(authz): scope enforcement per route class in client API policy (#84)

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.

* fix: apply CodeRabbit auto-fixes

Fixed 8 file(s) based on 3 unresolved review comments.

Co-authored-by: CodeRabbit <noreply@coderabbit.ai>

* feat(api-key): add allowed_models/allowed_providers and enforce in routing (#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>

* feat(deps): add hex crate version 0.4.3 to Cargo.lock

* fix(dashboard): update API key scope options to canonical chat:read/chat: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

* fix(authz): address code review findings (#1-#4)

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.

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/testing Tests and testing infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enforce allowed_models and allowed_providers in the routing pipeline Add allowed_models and allowed_providers to ApiKeyRecord — schema + CRUD

1 participant