fix: Add forwarded JWT header auth and issuer utils for api users#1697
Merged
Conversation
Add utilities to fetch and cache issuer public keys and verify JWTs (config/utils.py), including a helper to obtain OpenSearch service tokens from an internal auth server. Expose AUTH_SERVER_URL and a per-call get_jwt_auth_header accessor in config/settings.py so the forwarded-JWT header is configurable and testable. Extend dependencies.py to accept a gateway-forwarded JWT on the /v1 (API-key) surface: verify the token against the issuer, stage JWT roles for RBAC, and make the JWT the source of identity when valid (with proper 401 behavior under RBAC). Add unit tests for the header accessor and the JWT-header auth flows.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImplements permission-scoped API-key gating and gateway-JWT authentication for v1: adds require_api_key_permission, JWT public-key discovery/verification and role staging, updates v1 endpoints to use permission dependencies and PEP-604 typing, and adds unit/integration tests and a Ruff allowlist entry. ChangesAPI Key Authentication & Permission Enforcement
Sequence DiagramsequenceDiagram
participant Client
participant Gateway
participant App as OpenRAG App
participant Issuer
participant RBAC as RBACService
Client->>Gateway: request to /v1 endpoint with JWT header
Gateway->>App: forwarded JWT (get_jwt_auth_header)
App->>App: verify_jwt_from_issuer (reads kid, iss)
App->>Issuer: fetch issuer keys
Issuer->>App: JWKS/PEM
App->>App: _load_public_key_from_payload -> cache
App->>App: jwt.decode -> claims
App->>RBAC: rbac.get_user_permissions(user_id)
RBAC-->>App: permission list
App->>App: require_api_key_permission checks permission -> allow/deny
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 docstrings
🧪 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 |
github-actions
Bot
added
backend
Introduce require_api_key_permission dependency to enforce RBAC for the /v1 API-key/forwarded-JWT surface (mirrors require_permission semantics and kill-switch behavior). Wire v1 endpoints to use the new gate, update several type hints to PEP 604 unions, and tidy imports. Add unit tests (tests/unit/dependencies/test_require_api_key_permission.py) to validate permission checks and kill-switch behavior. Also register the new dependency in pyproject extend-immutable-calls.
github-actions
Bot
added
bug
github-actions
Bot
added
bug
github-actions
Bot
added
bug
Annotate chat_id_container as dict[str, str] in chat endpoint to make types explicit. Allow get_settings_endpoint to return either SettingsResponse or JSONResponse to support error responses. Change default OPENRAG_JWT_AUTH_HEADER from "X-OpenRAG-JWT" to the standard "Authorization" header so gateway-forwarded JWTs use a common header.
github-actions
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/api/v1/chat.py (1)
149-149:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd type annotation to fix mypy error.
The pipeline reports a mypy error requiring a type annotation for
chat_id_container.Proposed fix
- chat_id_container = {} + chat_id_container: dict[str, str | None] = {}🤖 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 `@src/api/v1/chat.py` at line 149, Add an explicit type annotation for chat_id_container to satisfy mypy (e.g., import Dict and Any from typing and declare chat_id_container: Dict[str, Any] = {}); ensure the typing import (from typing import Dict, Any) is added near other imports and replace the bare assignment "chat_id_container = {}" with the annotated declaration so mypy recognizes its type.src/api/v1/settings.py (1)
69-71:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix return type mismatch flagged by mypy.
The function signature declares
-> SettingsResponsebut the exception handler returnsJSONResponse, causing the pipeline mypy error. Either change the return type annotation toSettingsResponse | JSONResponseor raise anHTTPExceptioninstead.Option 1: Raise HTTPException (preferred for FastAPI)
except Exception as e: logger.error("Failed to get settings", error=str(e)) - return JSONResponse({"error": "Failed to get settings"}, status_code=500) + raise HTTPException(status_code=500, detail="Failed to get settings")Add
HTTPExceptionto imports:-from fastapi import Depends +from fastapi import Depends, HTTPExceptionOption 2: Update return type annotation
async def get_settings_endpoint( user: User = Depends(get_api_key_user_async), -) -> SettingsResponse: +) -> SettingsResponse | JSONResponse:🤖 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 `@src/api/v1/settings.py` around lines 69 - 71, The exception handler in the function that returns SettingsResponse currently returns a JSONResponse, causing a mypy return-type mismatch; update the handler to raise fastapi.HTTPException instead: add HTTPException to imports and replace `return JSONResponse({"error": "Failed to get settings"}, status_code=500)` with `raise HTTPException(status_code=500, detail="Failed to get settings")` (keep the existing logger.error call). This preserves the declared return type (SettingsResponse) and follows FastAPI conventions.
🧹 Nitpick comments (2)
src/config/utils.py (1)
89-91: ⚖️ Poor tradeoffSynchronous HTTP call may block the event loop when invoked from async context.
get_public_key_from_issueruses a synchronoushttpx.Client, but it's called fromverify_jwt_from_issuer, which is invoked insideget_api_key_user_async(an async FastAPI dependency). This blocks the event loop during the HTTP fetch, degrading concurrency under load.Consider converting to
httpx.AsyncClient(and making the caller chain async) or wrapping inasyncio.to_thread()at the call site.🤖 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 `@src/config/utils.py` around lines 89 - 91, get_public_key_from_issuer currently performs a blocking HTTP call with httpx.Client which can block the FastAPI event loop when invoked from async dependencies; change it to an async implementation using httpx.AsyncClient (await client.get(...) and await response.aread()/response.raise_for_status()) and propagate async changes by making verify_jwt_from_issuer async and awaiting get_public_key_from_issuer, and updating get_api_key_user_async to await verify_jwt_from_issuer; alternatively, if you prefer minimal changes, keep get_public_key_from_issuer sync but call it from verify_jwt_from_issuer via asyncio.to_thread(...) to offload the blocking call—use the approach that matches your project's async style and ensure all call sites await the async functions.tests/unit/dependencies/test_require_api_key_permission.py (1)
18-19: ⚡ Quick winMake the in-memory SQLite pooling explicit (intent clarity), not necessity.
SQLAlchemy’s default behavior for
sqlite+aiosqlite:///:memory:already uses StaticPool/keeps a single in-memory connection alive, sopoolclass=StaticPoolisn’t strictly required to avoid state-sharing flakes; adding it just makes the test’s intent resilient to future default changes.Proposed diff
from fastapi import Depends, FastAPI, Request from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine +from sqlalchemy.pool import StaticPool from sqlmodel import SQLModel @@ engine = create_async_engine( "sqlite+aiosqlite:///:memory:", connect_args={"check_same_thread": False}, + poolclass=StaticPool, )🤖 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 `@tests/unit/dependencies/test_require_api_key_permission.py` around lines 18 - 19, The test should make SQLite in-memory pooling explicit: import StaticPool from sqlalchemy.pool and pass poolclass=StaticPool to create_async_engine (the call that currently uses "sqlite+aiosqlite:///:memory:"), so the engine creation in this test (where create_async_engine is used alongside async_sessionmaker/SQLModel) clearly documents and enforces a single-connection in-memory pool; update the engine construction to include poolclass=StaticPool (and keep any existing connect_args) so intent is explicit and robust to default changes.
🤖 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.
Outside diff comments:
In `@src/api/v1/chat.py`:
- Line 149: Add an explicit type annotation for chat_id_container to satisfy
mypy (e.g., import Dict and Any from typing and declare chat_id_container:
Dict[str, Any] = {}); ensure the typing import (from typing import Dict, Any) is
added near other imports and replace the bare assignment "chat_id_container =
{}" with the annotated declaration so mypy recognizes its type.
In `@src/api/v1/settings.py`:
- Around line 69-71: The exception handler in the function that returns
SettingsResponse currently returns a JSONResponse, causing a mypy return-type
mismatch; update the handler to raise fastapi.HTTPException instead: add
HTTPException to imports and replace `return JSONResponse({"error": "Failed to
get settings"}, status_code=500)` with `raise HTTPException(status_code=500,
detail="Failed to get settings")` (keep the existing logger.error call). This
preserves the declared return type (SettingsResponse) and follows FastAPI
conventions.
---
Nitpick comments:
In `@src/config/utils.py`:
- Around line 89-91: get_public_key_from_issuer currently performs a blocking
HTTP call with httpx.Client which can block the FastAPI event loop when invoked
from async dependencies; change it to an async implementation using
httpx.AsyncClient (await client.get(...) and await
response.aread()/response.raise_for_status()) and propagate async changes by
making verify_jwt_from_issuer async and awaiting get_public_key_from_issuer, and
updating get_api_key_user_async to await verify_jwt_from_issuer; alternatively,
if you prefer minimal changes, keep get_public_key_from_issuer sync but call it
from verify_jwt_from_issuer via asyncio.to_thread(...) to offload the blocking
call—use the approach that matches your project's async style and ensure all
call sites await the async functions.
In `@tests/unit/dependencies/test_require_api_key_permission.py`:
- Around line 18-19: The test should make SQLite in-memory pooling explicit:
import StaticPool from sqlalchemy.pool and pass poolclass=StaticPool to
create_async_engine (the call that currently uses
"sqlite+aiosqlite:///:memory:"), so the engine creation in this test (where
create_async_engine is used alongside async_sessionmaker/SQLModel) clearly
documents and enforces a single-connection in-memory pool; update the engine
construction to include poolclass=StaticPool (and keep any existing
connect_args) so intent is explicit and robust to default changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 54f6a999-9142-4516-affe-3598c87a8b38
📒 Files selected for processing (13)
pyproject.tomlsrc/api/v1/chat.pysrc/api/v1/documents.pysrc/api/v1/knowledge_filters.pysrc/api/v1/models.pysrc/api/v1/search.pysrc/api/v1/settings.pysrc/config/settings.pysrc/config/utils.pysrc/dependencies.pytests/unit/config/test_jwt_auth_header.pytests/unit/dependencies/test_jwt_header_auth.pytests/unit/dependencies/test_require_api_key_permission.py
Contributor
|
Actionable comments posted: 0 |
github-actions
Bot
added
bug
Stop pinning JWT issuers via AUTH_SERVER_URL and remove the issuer allowlist. JWT verification now discovers signing keys from the token's own iss (JWKS/PEM/JWK) and trusts the issuer to publish its keys; related helper logic (k8s SA token reader, issuer allowlist helper, and get_opensearch_service_token) and the AUTH_SERVER_URL setting were removed. The default JWT header was changed to Authorization (tests updated and pinning added where needed). Update dependencies and unit tests to reflect the simplified verification flow and header default; callers relying on AUTH_SERVER_URL or the removed service-token helper must be updated accordingly.
github-actions
Bot
added
bug
github-actions
Bot
added
bug
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/dependencies/test_jwt_header_auth.py (1)
33-36:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix mypy
[var-annotated]in_FakeRequest.__init__by annotating untyped instance attributes (headers,state).In
tests/unit/dependencies/test_jwt_header_auth.py,self.headers = headers or {}andself.state = SimpleNamespace()are currently un-annotated (whileself.cookiesis already annotated). Add explicit annotations (and ideally narrow theheadersparameter todict[str, str] | None) to unblock strict mypy.🤖 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 `@tests/unit/dependencies/test_jwt_header_auth.py` around lines 33 - 36, The test helper class _FakeRequest.__init__ assigns un-annotated instance attributes which triggers mypy var-annotated errors; update the signature to accept headers: dict[str, str] | None and add explicit instance attribute annotations for self.headers (dict[str, str]), self.cookies (dict[str, str]) and self.state (SimpleNamespace) inside _FakeRequest.__init__ so mypy can see the types (keep self.cookies as-is and narrow headers as suggested, and ensure SimpleNamespace import/usage for self.state is annotated).
🧹 Nitpick comments (2)
src/config/utils.py (2)
66-81: 💤 Low valueProcess httpx response inside the context manager.
Accessing
response.headersandresponse.json()after thewithblock exits works only because httpx buffers the response eagerly onclient.get(). If future refactoring enables streaming or deferred reads, this will fail silently. Move all response processing inside the context for clarity and safety.♻️ Suggested refactor
with httpx.Client(verify=verify_tls, timeout=timeout) as client: response = client.get(issuer) response.raise_for_status() - - content_type = response.headers.get("content-type", "") - if "json" in content_type: - key_payload = response.json() - else: - try: + content_type = response.headers.get("content-type", "") + if "json" in content_type: key_payload = response.json() - except ValueError: - key_payload = response.text + else: + try: + key_payload = response.json() + except ValueError: + key_payload = response.text public_key = _load_public_key_from_payload(key_payload, key_id)🤖 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 `@src/config/utils.py` around lines 66 - 81, Move all response processing into the httpx.Client context to avoid relying on buffered responses: inside the with httpx.Client(verify=verify_tls, timeout=timeout) as client block, call client.get(issuer), response.raise_for_status(), read response.headers and determine content_type, attempt response.json() with ValueError fallback to response.text to set key_payload; then after the with block call _load_public_key_from_payload(key_payload, key_id) and cache into _ISSUER_PUBLIC_KEY_CACHE[cache_key]. Use the existing variable names (client, response, key_payload, issuer, verify_tls, timeout, _load_public_key_from_payload, _ISSUER_PUBLIC_KEY_CACHE, cache_key) to locate and adjust the code.
84-101: 💤 Low valueDocument the SSRF/open-redirect trust model more prominently.
The docstring correctly states there is no issuer allowlist and that the gateway must control which
issvalues reach this code. However, the downstream call independencies.pypassesverify_tls=False, which compounds the risk if the header is ever settable by untrusted clients. Consider adding a warning-level log whenverify_tls=Falseis used, or at minimum ensure operational docs highlight this configuration.🤖 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 `@src/config/utils.py` around lines 84 - 101, The docstring and call site miss a prominent runtime warning about SSRF/open-redirect risk when discoverable issuer URLs are trusted and TLS verification is disabled; update the verify_jwt_from_issuer function to (1) augment its docstring with an explicit, high-visibility warning about the SSRF/open-redirect trust model and the danger of passing verify_tls=False, and (2) emit a warning-level log when verify_tls is False (use the module logger, e.g. logger.warning) so operators see the risky configuration at runtime; also ensure the dependency call-site that passes verify_tls=False is annotated in operational docs or comments to highlight this exceptional configuration.
🤖 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.
Outside diff comments:
In `@tests/unit/dependencies/test_jwt_header_auth.py`:
- Around line 33-36: The test helper class _FakeRequest.__init__ assigns
un-annotated instance attributes which triggers mypy var-annotated errors;
update the signature to accept headers: dict[str, str] | None and add explicit
instance attribute annotations for self.headers (dict[str, str]), self.cookies
(dict[str, str]) and self.state (SimpleNamespace) inside _FakeRequest.__init__
so mypy can see the types (keep self.cookies as-is and narrow headers as
suggested, and ensure SimpleNamespace import/usage for self.state is annotated).
---
Nitpick comments:
In `@src/config/utils.py`:
- Around line 66-81: Move all response processing into the httpx.Client context
to avoid relying on buffered responses: inside the with
httpx.Client(verify=verify_tls, timeout=timeout) as client block, call
client.get(issuer), response.raise_for_status(), read response.headers and
determine content_type, attempt response.json() with ValueError fallback to
response.text to set key_payload; then after the with block call
_load_public_key_from_payload(key_payload, key_id) and cache into
_ISSUER_PUBLIC_KEY_CACHE[cache_key]. Use the existing variable names (client,
response, key_payload, issuer, verify_tls, timeout,
_load_public_key_from_payload, _ISSUER_PUBLIC_KEY_CACHE, cache_key) to locate
and adjust the code.
- Around line 84-101: The docstring and call site miss a prominent runtime
warning about SSRF/open-redirect risk when discoverable issuer URLs are trusted
and TLS verification is disabled; update the verify_jwt_from_issuer function to
(1) augment its docstring with an explicit, high-visibility warning about the
SSRF/open-redirect trust model and the danger of passing verify_tls=False, and
(2) emit a warning-level log when verify_tls is False (use the module logger,
e.g. logger.warning) so operators see the risky configuration at runtime; also
ensure the dependency call-site that passes verify_tls=False is annotated in
operational docs or comments to highlight this exceptional configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f025a862-30ed-494a-a827-3848ec638896
📒 Files selected for processing (5)
src/config/settings.pysrc/config/utils.pysrc/dependencies.pytests/unit/config/test_jwt_auth_header.pytests/unit/dependencies/test_jwt_header_auth.py
💤 Files with no reviewable changes (1)
- src/config/settings.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/dependencies.py
Contributor
|
Actionable comments posted: 0 |
github-actions
Bot
added
bug
Collaborator
Author
|
@coderabbitai generate a mermaid diagram on the workflow |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
flowchart TD
A([Incoming /v1 Request]) --> B{JWT header\npresent?\nget_jwt_auth_header}
B -- No --> K[Proceed to\nAPI Key Check]
B -- Yes --> C{AUTH_SERVER_URL\nconfigured?}
C -- No --> K
C -- Yes --> D[verify_jwt_from_issuer\nconfig/utils.py]
D --> D1[Strip Bearer prefix]
D1 --> D2[Decode unverified header\nCheck allowed alg]
D2 --> D3[get_public_key_from_issuer\nFetch & cache issuer public key]
D3 --> D4[jwt.decode\nVerify signature + claims\niss, sub, exp, iat]
D4 -- Verification\nfails --> E{RBAC\nenforced?\nOPENRAG_RBAC_ENFORCE}
E -- Yes --> F[HTTP 401\nInvalid or unverifiable JWT]
E -- No --> K
D4 -- Claims valid --> G[Build User identity\nfrom JWT claims]
G --> H[_stage_jwt_roles\nextract roles from JWT claims]
H --> I{RBAC\nenforced?}
I -- No --> J[Skip role staging\nreturn JWT User]
I -- Yes --> I2{Roles found\nin JWT?}
I2 -- No roles --> F2[HTTP 401\nNo recognized roles]
I2 -- Roles found --> I3[Store roles in\nrequest.state.jwt_roles]
I3 --> L[_attach_db_user_id\nlink JWT identity to DB]
L --> J
K --> K1{API key\npresent?}
K1 -- No --> K2[HTTP 401\nAPI key required]
K1 -- Yes --> K3[Resolve User\nfrom API key\nget_api_key_user_async]
J --> M[require_api_key_permission\nperm check]
K3 --> M
M --> N{RBAC\nenforced?}
N -- No --> O([✅ Handler invoked])
N -- Yes --> P[rbac.get_user_permissions]
P --> Q{Permission\nheld?}
Q -- Yes --> O
Q -- No --> R[HTTP 403\npermission_denied\nrequired: perm]
style F fill:rgba(220,50,47,0.5)
style F2 fill:rgba(220,50,47,0.5)
style K2 fill:rgba(220,50,47,0.5)
style R fill:rgba(220,50,47,0.5)
style O fill:rgba(42,161,152,0.5)
Key flows summarized:
|
Collaborator
Author
@coderabbitai retry again generate the sequence diagram |
Collaborator
Author
|
@coderabbitai review the PR |
Contributor
|
Also, here is the sequence diagram you requested earlier: sequenceDiagram
participant Client
participant FastAPI as FastAPI /v1 Endpoint
participant GKU as get_jwt_auth_header()<br/>(config/settings.py)
participant VJI as verify_jwt_from_issuer()<br/>(config/utils.py)
participant GPKI as get_public_key_from_issuer()<br/>(config/utils.py)
participant SJR as _stage_jwt_roles()<br/>(dependencies.py)
participant ADBU as _attach_db_user_id()<br/>(dependencies.py)
participant RAKP as require_api_key_permission()<br/>(dependencies.py)
participant RBAC as rbac.get_user_permissions()
Client->>FastAPI: Request with headers
FastAPI->>GKU: get_jwt_auth_header()
GKU-->>FastAPI: header name (e.g. "Authorization")
alt JWT header present AND AUTH_SERVER_URL set
FastAPI->>VJI: verify_jwt_from_issuer(token)
VJI->>VJI: Strip Bearer prefix
VJI->>VJI: Decode unverified header (check alg)
VJI->>GPKI: get_public_key_from_issuer(iss, kid)
GPKI-->>VJI: cached/fetched public key
VJI->>VJI: jwt.decode (verify sig + iss/sub/exp/iat)
alt Verification fails
VJI-->>FastAPI: None
alt RBAC enforced
FastAPI-->>Client: HTTP 401 Invalid or unverifiable JWT
else RBAC off
FastAPI->>FastAPI: Fall through to API key check
end
else Claims valid
VJI-->>FastAPI: JWT claims dict
FastAPI->>FastAPI: Build User identity from claims
FastAPI->>SJR: _stage_jwt_roles(request, claims, user_id)
alt RBAC enforced
SJR->>SJR: Extract roles from JWT claim
alt No recognized roles
SJR-->>Client: HTTP 401 No recognized roles
else Roles found
SJR->>SJR: Store in request.state.jwt_roles
SJR-->>FastAPI: roles staged
end
else RBAC off
SJR-->>FastAPI: noop (jwt_roles not staged)
end
FastAPI->>ADBU: _attach_db_user_id(user)
ADBU-->>FastAPI: user with DB id
end
else No JWT header or no AUTH_SERVER_URL
FastAPI->>FastAPI: Check API key header
alt No API key
FastAPI-->>Client: HTTP 401 API key required
else API key present
FastAPI->>FastAPI: Resolve User from API key
end
end
FastAPI->>RAKP: require_api_key_permission(perm)
alt RBAC off
RAKP-->>FastAPI: bypass
else RBAC enforced
RAKP->>RBAC: get_user_permissions(user)
RBAC-->>RAKP: permissions set
alt Permission held
RAKP-->>FastAPI: allowed
else Permission missing
RAKP-->>Client: HTTP 403 permission_denied (required: perm)
end
end
FastAPI-->>Client: ✅ Handler response
✅ Actions performedReview triggered.
|
Contributor
|
Tip For best results, initiate chat on the files or code changes.
sequenceDiagram
autonumber
participant C as Client
participant EP as FastAPI /v1 Endpoint
participant DEP as get_api_key_user_async
participant VFY as verify_jwt_from_issuer
participant PKI as get_public_key_from_issuer<br/>(TTL Cache)
participant SJR as _stage_jwt_roles
participant DBA as _attach_db_user_id
participant PERM as require_api_key_permission
participant RBAC as RBAC Service
C->>EP: HTTP Request (JWT header or API key)
EP->>DEP: resolve user identity
alt JWT header present AND AUTH_SERVER_URL configured
DEP->>VFY: verify_jwt_from_issuer(token)
VFY->>VFY: Strip Bearer prefix
VFY->>VFY: Decode unverified header → check alg allowlist
VFY->>VFY: Decode unverified claims → extract iss
VFY->>PKI: get_public_key_from_issuer(iss, kid)
PKI-->>VFY: cached or freshly fetched public key
VFY->>VFY: jwt.decode (verify sig + iss/sub/exp/iat)
alt Verification succeeds
VFY-->>DEP: JWT claims (sub, email, …)
DEP->>DEP: Build User from JWT claims
DEP->>SJR: _stage_jwt_roles(request, claims, user_id)
alt RBAC enforced (OPENRAG_RBAC_ENFORCE=true)
SJR->>SJR: Map JWT roles claim → OpenRAG roles
alt Roles found
SJR->>SJR: request.state.jwt_roles = [roles]
SJR-->>DEP: OK
DEP->>DBA: _attach_db_user_id(user)
DBA-->>DEP: user with DB id attached
DEP-->>EP: JWT User ✅
else No recognized roles
SJR-->>EP: HTTP 401 – No recognized roles ❌
end
else RBAC off
SJR->>SJR: skip role staging
SJR-->>DEP: OK (roles not staged)
DEP-->>EP: JWT User ✅
end
else Verification fails
VFY-->>DEP: None
alt RBAC enforced
DEP-->>EP: HTTP 401 – Invalid or unverifiable JWT ❌
else RBAC off
Note over DEP: Fall through to API key check
DEP->>DEP: Check API key header
alt API key present
DEP-->>EP: API key User ✅
else No API key
DEP-->>EP: HTTP 401 – API key required ❌
end
end
end
else No JWT header (or no AUTH_SERVER_URL)
DEP->>DEP: Check API key header
alt API key present
DEP-->>EP: API key User ✅
else No API key
DEP-->>EP: HTTP 401 – API key required ❌
end
end
EP->>PERM: require_api_key_permission(perm)
alt RBAC enforced
PERM->>RBAC: get_user_permissions(user)
RBAC-->>PERM: permission set
alt Permission held
PERM-->>EP: OK ✅
EP-->>C: 200 Response
else Permission missing
PERM-->>EP: HTTP 403 – permission_denied { required: perm } ❌
EP-->>C: 403 Forbidden
end
else RBAC off
PERM-->>EP: Bypass ✅
EP-->>C: 200 Response
end
Summary of key paths:
(¬ᵕ¬) 🐇 |
github-actions
Bot
added
bug
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/v1/settings.py (1)
47-71:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn type annotation no longer matches actual return paths.
The annotation was narrowed to
SettingsResponse, but line 71 still returnsJSONResponseon error. This breaks static type checking and can mislead consumers.Either restore the union type or convert to an
HTTPExceptionfor consistency with FastAPI error-handling patterns.Option A: Restore union type
async def get_settings_endpoint( user: User = Depends(get_api_key_user_async), -) -> SettingsResponse: +) -> SettingsResponse | JSONResponse:Option B: Raise HTTPException (preferred)
+from fastapi import Depends, HTTPException -from fastapi import Depends ... except Exception as e: logger.error("Failed to get settings", error=str(e)) - return JSONResponse({"error": "Failed to get settings"}, status_code=500) + raise HTTPException(status_code=500, detail="Failed to get settings")As per coding guidelines:
src/api/**/*.pyrequires verifying "response model typing."🤖 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 `@src/api/v1/settings.py` around lines 47 - 71, The get_settings_endpoint function currently annotates its return type as SettingsResponse but returns a JSONResponse on exception; update this to follow FastAPI error-handling by replacing the JSONResponse return with raising fastapi.HTTPException (import HTTPException) so the function consistently returns SettingsResponse or raises on error, or alternatively change the signature to return Union[SettingsResponse, JSONResponse] if you prefer Option A; locate get_settings_endpoint, the error handling block that logs via logger.error and uses JSONResponse, and modify it to raise HTTPException(status_code=500, detail="Failed to get settings") while keeping the existing log of get_openrag_config errors.
🤖 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.
Outside diff comments:
In `@src/api/v1/settings.py`:
- Around line 47-71: The get_settings_endpoint function currently annotates its
return type as SettingsResponse but returns a JSONResponse on exception; update
this to follow FastAPI error-handling by replacing the JSONResponse return with
raising fastapi.HTTPException (import HTTPException) so the function
consistently returns SettingsResponse or raises on error, or alternatively
change the signature to return Union[SettingsResponse, JSONResponse] if you
prefer Option A; locate get_settings_endpoint, the error handling block that
logs via logger.error and uses JSONResponse, and modify it to raise
HTTPException(status_code=500, detail="Failed to get settings") while keeping
the existing log of get_openrag_config errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 943bde5b-6e57-4b73-b524-eb86fcb9f131
📒 Files selected for processing (2)
pyproject.tomlsrc/api/v1/settings.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add utilities to fetch and cache issuer public keys and verify JWTs (config/utils.py), including a helper to obtain OpenSearch service tokens from an internal auth server. Expose AUTH_SERVER_URL and a per-call get_jwt_auth_header accessor in config/settings.py so the forwarded-JWT header is configurable and testable. Extend dependencies.py to accept a gateway-forwarded JWT on the /v1 (API-key) surface: verify the token against the issuer, stage JWT roles for RBAC, and make the JWT the source of identity when valid (with proper 401 behavior under RBAC). Add unit tests for the header accessor and the JWT-header auth flows.
Summary by CodeRabbit
New Features
Refactor
Tests