feat(auth): add OpenID Connect authentication mode - #308
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 50 minutes and 16 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (32)
📝 WalkthroughWalkthroughThis pull request introduces OpenID Connect (OIDC) authentication as an alternative to the existing token-based authentication. It adds a complete OIDC implementation with an Authorization Code + PKCE flow, session management, encrypted token storage, database persistence, middleware integration, and comprehensive documentation and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant OpenRAG as OpenRAG API
participant IdP as Identity Provider
participant DB as Database
Browser->>OpenRAG: GET /auth/login?next=/...
OpenRAG->>OpenRAG: Generate state, nonce, PKCE
OpenRAG->>Browser: Set state cookie + Redirect to IdP
Browser->>IdP: GET /authorize?state=...&code_challenge=...
IdP->>Browser: Login form
Browser->>IdP: Submit credentials
IdP->>Browser: Redirect to callback with code
Browser->>OpenRAG: GET /auth/callback?code=...&state=...
OpenRAG->>OpenRAG: Verify state from cookie
OpenRAG->>IdP: POST /token (code + code_verifier)
IdP->>OpenRAG: Return id_token, access_token
OpenRAG->>OpenRAG: Verify id_token, extract sub
OpenRAG->>DB: Lookup user by external_user_id=sub
DB->>OpenRAG: Return user
OpenRAG->>OpenRAG: Optional: fetch userinfo & update claims
OpenRAG->>DB: Create OIDC session (encrypted tokens)
DB->>OpenRAG: Session created
OpenRAG->>Browser: Set session cookie + Redirect to next
Browser->>OpenRAG: GET /api/endpoint + Cookie: openrag_session
OpenRAG->>OpenRAG: Middleware: Validate session token
OpenRAG->>DB: Fetch session by token hash
DB->>OpenRAG: Return encrypted session
OpenRAG->>OpenRAG: Decrypt tokens, check expiry
OpenRAG->>OpenRAG: If near expiry: refresh via RefreshToken
OpenRAG->>Browser: Serve response with updated session
sequenceDiagram
participant Browser
participant OpenRAG as OpenRAG API
participant IdP as Identity Provider
participant DB as Database
Note over OpenRAG,IdP: Back-Channel Logout (IdP-Initiated)
IdP->>OpenRAG: POST /auth/backchannel-logout (logout_token)
OpenRAG->>OpenRAG: Verify logout_token JWT signature
OpenRAG->>OpenRAG: Extract sid from logout_token
OpenRAG->>DB: Revoke all sessions by sid
DB->>OpenRAG: Sessions revoked
OpenRAG->>IdP: Return 200
Note over Browser,OpenRAG: RP-Initiated Logout
Browser->>OpenRAG: GET /auth/logout
OpenRAG->>OpenRAG: Extract session cookie
OpenRAG->>DB: Revoke OIDC session
OpenRAG->>IdP: GET /end_session_endpoint
IdP->>Browser: Redirect to post_logout_redirect_uri
Browser->>Browser: Cleared
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an OpenID Connect (OIDC) authentication mode (AUTH_MODE=oidc) alongside the existing token-based auth, including session cookie handling, encrypted token storage, back-channel logout support, and supporting tests/docs.
Changes:
- Introduces an OIDC auth package (client, middleware, refresh helper, state cookie + session token utils) and mounts new
/auth/*routes. - Adds DB support for OIDC sessions (
oidc_sessions) and optionalusers.email, plus vectordb actor methods for session/user operations. - Adds extensive unit/integration/E2E-style tests plus configuration and operational documentation updates.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/api_tests/test_oidc_lifecycle.py | Adds an end-to-end OIDC lifecycle test (login → callback → authenticated request → backchannel logout → revoked session). |
| tests/api_tests/OIDC_TEST_COVERAGE.md | Adds an acceptance-criteria-to-tests coverage matrix for OIDC. |
| pyproject.toml | Adds runtime deps for OIDC (authlib/itsdangerous/cryptography) and dev dep respx. |
| openrag/scripts/migrations/alembic/versions/f5b6c918f741_add_oidc_auth.py | Adds users.email and the oidc_sessions table + indexes. |
| openrag/routers/test_auth_router.py | Adds router-level integration tests for /auth/* endpoints with IdP mocking. |
| openrag/routers/auth.py | Implements /auth/login, /auth/callback, /auth/backchannel-logout, /auth/logout, /auth/me. |
| openrag/models/user.py | Extends user models with optional email. |
| openrag/components/indexer/vectordb/vectordb.py | Exposes OIDC-related PartitionFileManager methods on the Ray actor surface. |
| openrag/components/indexer/vectordb/utils.py | Implements user lookup/update helpers and full OIDC session CRUD/revocation/cleanup in PartitionFileManager. |
| openrag/components/indexer/vectordb/test_oidc_sessions.py | Adds unit tests covering OIDC session/user DB behaviors. |
| openrag/components/indexer/vectordb/models.py | Adds User.email and new OIDCSession ORM model + relationship. |
| openrag/components/auth/test_state_cookie.py | Adds state cookie serializer tests (round-trip/tamper/expiry). |
| openrag/components/auth/test_session_tokens.py | Adds session token hashing + Fernet encrypt/decrypt tests. |
| openrag/components/auth/test_oidc_client.py | Adds OIDC client tests for discovery/JWKS/JWT verification/refresh/userinfo/logout token verification. |
| openrag/components/auth/test_middleware.py | Adds comprehensive middleware decision-tree tests for token vs OIDC mode + refresh stampede guard. |
| openrag/components/auth/state_cookie.py | Implements signed OIDC state cookie serializer/payload. |
| openrag/components/auth/session_tokens.py | Implements opaque session tokens + SHA-256 hashing + Fernet encryption helpers. |
| openrag/components/auth/refresh.py | Implements lazy access-token refresh with stampede mitigation and error recovery. |
| openrag/components/auth/oidc_client.py | Implements lightweight OIDC RP client (discovery/JWKS caches, PKCE, exchanges, refresh, verification). |
| openrag/components/auth/middleware.py | Replaces legacy AuthMiddleware with dual-mode (token + oidc) middleware and routing behavior. |
| openrag/components/auth/deps.py | Adds a process-local OIDCClient singleton with reset hook. |
| openrag/components/auth/init.py | Exposes the auth package public API. |
| openrag/app_front.py | Updates Chainlit auth integration for OIDC cookie-based sessions (header auth callback). |
| openrag/api.py | Adds OIDC config validation, mounts auth router, and swaps in the refactored AuthMiddleware. |
| docs/oidc.md | Adds a detailed OIDC configuration/operations guide. |
| docker-compose.yaml | Propagates AUTH_MODE to indexer-ui and adjusts host resolution / services. |
| README.md | Documents the two auth modes and links to OIDC guide. |
| CLAUDE.md | Adds an OIDC authentication section and operational notes. |
| .env.example | Documents new OIDC-related environment variables and claim mapping options. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Introduces AUTH_MODE=oidc alongside the existing token flow. In the new
mode, humans authenticate via an external IdP (Authorization Code + PKCE)
and receive an opaque httpOnly session cookie; programmatic clients keep
using Bearer users.token.
Backend:
- New /auth/{login,callback,backchannel-logout,logout,me} routes
- AuthMiddleware: oidc_sessions cookie lookup, users.token Bearer fallback,
lazy access_token refresh with last_refresh_at short-circuit + SELECT
FOR UPDATE, UI routes 302 to /auth/login, API routes 401 JSON
- OIDC client based on Authlib (discovery, JWKS, token exchange, refresh,
userinfo, logout_token verification)
- Fernet-encrypted IdP tokens at rest; openrag_session cookie is opaque
(SHA-256 hashed in oidc_sessions)
- Back-channel logout: revokes by sid (spec-compliant)
- User matching: external_user_id=sub (fast path) → email (with backfill
of external_user_id=sub) → 403 (no auto-provisioning)
Chainlit:
- header_auth_callback reads openrag_session cookie in oidc mode
- password_auth_callback gated behind AUTH_MODE!=oidc
DB:
- Alembic migration f5b6c918f741: users.email (unique nullable) +
oidc_sessions table (encrypted tokens, sid index, composite user_sub)
Deps: authlib>=1.3, itsdangerous>=2.2, cryptography>=42, respx (dev)
Docs:
- docs/oidc.md: full guide (flow, config, Keycloak + LemonLDAP::NG,
programmatic access, back-channel logout, troubleshooting, security)
- CLAUDE.md: Authentication section extended
- README.md + .env.example
Backward compat:
- AUTH_MODE=token (default) behavior strictly unchanged
- Legacy 403 "Missing token" / "Invalid token" responses preserved
Pairs with linagora/openrag-admin-ui#18.
Per design review: drop the email-fallback matching path and its backfill/collision machinery. Matching is now purely by external_user_id == sub (admin MUST pre-provision with the right sub). The users.email column stays as optional metadata; it is no longer used for matching. Introduce an opt-in OIDC_CLAIM_MAPPING env var to let the IdP be the source of truth for display_name / email: OIDC_CLAIM_MAPPING=display_name:name,email:email OIDC_CLAIM_SOURCE=id_token # or 'userinfo' After a successful login, claims are read from the ID token (cheap) or /userinfo (complete) and the user row is updated. Writable fields are strictly whitelisted (display_name, email) — is_admin, external_user_id, file_quota, token can never be written via OIDC. The whitelist is enforced at three layers (startup parser, request-time parser, DB method) as defence in depth. Breaking changes inside the draft PR: - OIDC_EMAIL_SOURCE → OIDC_CLAIM_SOURCE (rename, same semantics) - OIDC_ALLOWED_EMAIL_DOMAINS: removed (no longer applicable) - get_user_by_email / set_user_external_id: removed (unused) Added: - update_user_fields DB method + Ray actor delegation - Three new callback tests (mapping from id_token, from userinfo, skipped when unset); five new update_user_fields tests Also in this commit: - Fix CI lint: ruff autofix (sorted imports, typing.Callable → abc, unused imports removed, dict() literal) - Fix CI tests: respx.MockTransport → MockRouter + httpx.MockTransport (respx >= 0.22 API) - Small doc updates: CLAUDE.md, docs/oidc.md, .env.example
- JsonWebToken() → JsonWebToken(["RS256"]) in both _sign_jwt helpers (Authlib >= 1.0 requires the allowed-algorithms list as first arg). - test_auth_router.py: respx.MockTransport → MockRouter + httpx.MockTransport (same fix as test_oidc_client.py last commit — respx >= 0.22 API). - test_oidc_lifecycle.py: drop unused `Request` import + sort imports.
When a user starts the OIDC flow from http://<backend>/ (no ?next= override), the callback lands them back on "/" after authentication and previously got a 404. Add a simple root handler that forwards to the indexer-ui (if it runs on a different host/port) or to /chainlit/, otherwise returns a minimal JSON status.
Correctness / security: - middleware: cookie-session lookup now gated behind AUTH_MODE=oidc so a stray openrag_session cookie cannot authenticate requests in token mode (preserves legacy Bearer-only contract) - /auth/callback: on token-exchange failure, return a generic error message to the client; full exception now logs via logger.exception() only — no IdP URLs / stack-adjacent internals leak via the HTTP response - docker-compose: remove the hardcoded `auth.example.com:host-gateway` extra_host (would shadow a real external domain for other users) + restore vllm-gpu / vllm-cpu service definitions that an unrelated local change had commented out - deps: reset_oidc_client() now best-effort closes the underlying httpx.AsyncClient to avoid "Unclosed client session" warnings in tests Tests — CI green: - test_auth_router.py: finish the respx.MockTransport → MockRouter migration (rename the oidc_transport fixture attr + all callsites; swap _mock_token_endpoint's param; drop the stray .router.get chain) - test_oidc_lifecycle.py: same migration (MockRouter + httpx.MockTransport), add ["RS256"] to the JsonWebToken factory (Authlib >= 1.0), tighten the post-logout /users/info assertion to strict 401 - test_middleware.py: docstring says "naive local time" not "naive UTC" Docs: - docs/oidc.md CSRF section: fix cookie name (openrag_oidc_state, not idp_state) and TTL (10 min, not 5) - CLAUDE.md Session Management: correct session-token shape description (secrets.token_urlsafe(32) → URL-safe ~43 chars, not 32-byte hex)
- JWKS kid was set with setdefault() which authlib's JsonWebKey.generate_key() had already populated with a random value, so signed JWTs advertised kid="test-key-1" while the JWKS served random-kid keys → authlib raised "Key not found" during verification. Use explicit []= assignment in all three test files (test_oidc_client.py, test_auth_router.py, test_oidc_lifecycle.py) to force the expected kid. Doc: - Add docs/sso-quickstart.md — step-by-step onboarding for non-experts: what to ask the SSO admin (client registration fields, redirect URIs, back-channel logout URL), how to verify the exact issuer string via curl | jq, how to generate the Fernet key, the .env block, and how to pre-provision users.
Previously defaulted to '/' which lands the user back on OpenRag's root
and immediately re-triggers OIDC login (silent re-auth if the IdP
session survived, or a loop if it didn't). Now optional:
- If set: forwarded to the IdP's end_session_endpoint as
post_logout_redirect_uri (when available) or returned directly.
- If unset: /auth/logout redirects to the IdP's end_session endpoint
WITHOUT a post_logout_redirect (IdP shows its own 'logged out' page),
or if the IdP doesn't advertise end_session, returns a plain 200
{"detail": "Logged out"} with the session cookie deleted.
Also drop the misleading 'PKCE S256 required' row from docs/sso-quickstart
— PKCE is RP-side, no IdP admin action needed.
…override
- Run ruff format on tests/api_tests/test_oidc_lifecycle.py to fix the
remaining CI lint failure (single-line function calls where the project
style allows them).
- Add docker-compose.override.yaml{,.yml} to .gitignore so developers can
keep local-only overrides (e.g. mapping an IdP hostname to host-gateway,
skipping heavy deps) without risking an accidental commit.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (9)
openrag/components/auth/test_state_cookie.py (1)
60-67: Nit:time.sleep(1)in unit tests is wasteful.If the serializer treats
age >= max_ageas expired,max_age=0expires instantly and the sleep is unnecessary; if it requiresage > max_age, the test depends on the 1 s wait actually elapsing. Either switch tomax_age=-1/ freeze time, or add a brief comment documenting why the sleep is required. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/auth/test_state_cookie.py` around lines 60 - 67, The test TestExpiry.test_expired_cookie_raises_value_error currently uses time.sleep(1) which is unnecessary and brittle; update the test to avoid sleeping by making the token immediately considered expired (e.g., call ser.loads(token, max_age=-1)) and remove the time.sleep call, or alternatively freeze time around _serializer()/_payload() creation and loading; modify references in the test (TestExpiry, test_expired_cookie_raises_value_error, _serializer, _payload, ser.loads, time.sleep) accordingly so the test deterministically fails with a ValueError without real-time waiting.openrag/app_front.py (1)
58-68: Minor: cookie parser doesn't unquote quoted values.[RFC 6265] allows cookie-values to be wrapped in double quotes. For opaque URL-safe session tokens this is effectively unreachable, but making it robust is a one-liner and guards against odd reverse-proxy rewrites:
🧹 Suggestion
- if k.strip() == name: - return v.strip() + if k.strip() == name: + v = v.strip() + if len(v) >= 2 and v[0] == '"' and v[-1] == '"': + v = v[1:-1] + return v🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/app_front.py` around lines 58 - 68, The cookie parser _extract_cookie should handle RFC6265 quoted cookie-values: after locating v in _extract_cookie, detect if it starts and ends with a double-quote and if so strip the surrounding quotes and unescape backslash-escaped characters (at least \" and \\) before returning; update the function to return this unquoted/unescaped value so quoted cookie headers are parsed correctly.openrag/components/indexer/vectordb/models.py (1)
106-140: LGTM — OIDCSession schema matches the persistence contract.
session_token_hashsized for SHA-256 hex, cascade delete via FK + relationship, encrypted blobs asLargeBinary, and the(user_id, sub)composite index all line up with the back-channel-logout and refresh code paths. TheUser.emailaddition asunique=True, nullable=True, index=Truecorrectly allows multiple NULL rows while enforcing per-email uniqueness.One low-priority operational note (can defer): a future janitor job that purges expired/revoked sessions will want an index on
(revoked_at)or(session_expires_at)— worth adding when you wire up that sweeper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/indexer/vectordb/models.py` around lines 106 - 140, Add a time-based index to OIDCSession to support efficient janitor sweeps: update the OIDCSession model's __table_args__ (in the OIDCSession class) to include an Index on session_expires_at and/or revoked_at (e.g., add Index("ix_oidc_sessions_session_expires_at", "session_expires_at") and/or Index("ix_oidc_sessions_revoked_at", "revoked_at")) so expired or revoked session lookups used by the sweeper are indexed.openrag/api.py (1)
142-157: ValidateOIDC_TOKEN_ENCRYPTION_KEYas a real Fernet key at startup.You currently only check the var is non-empty. A typo or wrong-length value will boot successfully and then blow up on the first
encrypt_token/decrypt_tokencall mid-flow (callback, refresh, logout). Since the existing block is already fail-fast, extending it costs nothing and prevents a class of late failures.🛡️ Proposed fail-fast check
if OIDC_CLAIM_SOURCE not in ("id_token", "userinfo"): raise RuntimeError(f"Invalid OIDC_CLAIM_SOURCE={OIDC_CLAIM_SOURCE!r}. Expected 'id_token' or 'userinfo'.") + try: + from cryptography.fernet import Fernet + Fernet(OIDC_TOKEN_ENCRYPTION_KEY.encode() if isinstance(OIDC_TOKEN_ENCRYPTION_KEY, str) else OIDC_TOKEN_ENCRYPTION_KEY) + except Exception as e: + raise RuntimeError(f"OIDC_TOKEN_ENCRYPTION_KEY is not a valid Fernet key: {e}") from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/api.py` around lines 142 - 157, The current startup check only ensures OIDC_TOKEN_ENCRYPTION_KEY is non-empty; add a fail-fast validation that the value is a valid Fernet key by attempting to construct a cryptography.fernet.Fernet instance with OIDC_TOKEN_ENCRYPTION_KEY inside the AUTH_MODE == "oidc" block (near the existing missing-vars check) and raise a RuntimeError with a clear message if construction fails; this prevents later runtime failures in encrypt_token/decrypt_token flows and complements the existing OIDC_CLAIM_SOURCE validation.openrag/components/indexer/vectordb/utils.py (1)
332-332: DRY: extract an_normalize_emailhelper.The same
email.strip().lower()logic is duplicated betweencreate_userandupdate_user_fields. Small helper keeps future changes (e.g., IDN/punycode normalization, RFC 5321 mailbox parsing) in one place:♻️ Proposed helper
+ `@staticmethod` + def _normalize_email(value: object) -> str | None: + if not isinstance(value, str): + return value # type: ignore[return-value] + v = value.strip().lower() + return v or None- email=(body.email.strip().lower() if body.email else None), + email=self._normalize_email(body.email) if body.email else None,- if "email" in cleaned and isinstance(cleaned["email"], str): - cleaned["email"] = cleaned["email"].strip().lower() + if "email" in cleaned: + cleaned["email"] = self._normalize_email(cleaned["email"])Also applies to: 897-902
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/indexer/vectordb/utils.py` at line 332, Extract a small helper function named _normalize_email and replace the inline email normalization in create_user and update_user_fields with calls to this helper; implement _normalize_email to accept a possibly None value and return None or the normalized string (e.g., value.strip().lower()) so future changes (IDN/punycode, RFC parsing) are centralized, then update all occurrences (including the similar duplication around lines 897-902) to call _normalize_email(body.email) instead of repeating body.email.strip().lower().openrag/components/indexer/vectordb/test_oidc_sessions.py (1)
92-103: Nit: awkward pattern using an unrelated lookup to "silence" warnings.The
pfm.get_user_by_external_id("sub-missing")call and the trailingassert refreshed is Noneare dead weight — you're already re-reading vias.query(User). Drop the indirection:♻️ Proposed cleanup
def test_update_user_fields_updates_display_name_and_lowercases_email(pfm): user_id = _make_user(pfm, display_name="Old", email="old@example.com") pfm.update_user_fields(user_id, {"display_name": "New Name", "email": "NEW@Example.COM"}) - refreshed = pfm.get_user_by_external_id("sub-missing") # None lookup, use session - # Re-read via direct ORM since the user has no external_user_id set. with pfm.Session() as s: row = s.query(User).filter_by(id=user_id).first() assert row.display_name == "New Name" assert row.email == "new@example.com" # normalised - # `refreshed` is unrelated to the assertions — silences unused warning. - assert refreshed is None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/indexer/vectordb/test_oidc_sessions.py` around lines 92 - 103, The test test_update_user_fields_updates_display_name_and_lowercases_email contains an unnecessary pfm.get_user_by_external_id("sub-missing") call and the subsequent assert refreshed is None used only to silence an unused-variable warning; remove the pfm.get_user_by_external_id invocation and the refreshed variable and its assert, leaving the direct ORM re-read via pfm.Session() / s.query(User) and the two assertions on row.display_name and row.email intact.openrag/components/auth/oidc_client.py (2)
374-376:aclose()closes caller-injectedhttp_client.When a caller passes
http_client=(tests do, via respx),aclose()still closes it. The test fixture already reusesreset_oidc_client()and opts into this, so this is fine today — just track the coupling: if a future caller wants to share oneAsyncClientacross multiple services, they'd be surprised by unexpected close. A small_owns_httpflag would isolate ownership.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/auth/oidc_client.py` around lines 374 - 376, The aclose method currently unconditionally closes the caller-injected AsyncClient (_http), so add an ownership flag (e.g., self._owns_http) set in __init__ based on whether http_client was provided; when you create the internal AsyncClient set _owns_http = True, when a caller supplies http_client set _owns_http = False; update aclose to only call await self._http.aclose() if self._owns_http is True (leave reset_oidc_client and existing tests unchanged).
310-368: Consider adding jti replay protection for logout tokens.OIDC Back-Channel Logout §2.6 specifies (step 8) that RPs optionally verify that another logout token with the same
jtihas not been recently received. Currently, the same logout token can be re-POSTed indefinitely; whilerevoke_oidc_sessions_by_sidis idempotent, this creates unnecessary processing and DoS surface. A bounded in-memory LRU or small DB table keyed by(iss, jti)with TTL matching the token lifetime would prevent replays. Not blocking but worth tracking as a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/auth/oidc_client.py` around lines 310 - 368, Add jti replay protection to verify_logout_token by recording seen (iss, jti) values with a bounded TTL and rejecting duplicates; specifically, in the verify_logout_token flow inside verify_logout_token (before returning LogoutTokenClaims and after signature/claim validation) check if decoded.get("jti") is present, look up (self.issuer, jti) in a short-lived store (an in-memory LRU with TTL or a small DB table), and if already seen raise ValueError("logout_token replay detected") or silently ignore processing; otherwise insert (self.issuer, jti) with TTL equal to token lifetime (exp - iat or exp - now) so subsequent calls will be treated as replays; ensure the store is initialized on the OIDC client instance and used by revoke_oidc_sessions_by_sid callers as needed.openrag/routers/auth.py (1)
312-321: Consider not failing the login on a transient userinfo fetch error.A flaky IdP userinfo endpoint (or a network hiccup) will 400 the entire callback even though the code exchange and ID-token verification already succeeded. Since claim-mapping is an optional profile enrichment, log and continue without the update is more user-friendly — the user is already authenticated:
♻️ Proposed fix
if _claim_source() == "userinfo": try: claims_for_mapping: dict[str, Any] = await client.fetch_userinfo(bundle.access_token) except Exception as e: - logger.warning(f"OIDC userinfo fetch failed: {e}") - return _json_error( - status.HTTP_400_BAD_REQUEST, - "Failed to fetch userinfo from IdP.", - delete_state_cookie=True, - ) + logger.warning(f"OIDC userinfo fetch failed — skipping claim mapping: {e}") + claims_for_mapping = {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/routers/auth.py` around lines 312 - 321, The current callback treats a userinfo fetch failure as fatal; instead make userinfo enrichment optional by catching exceptions from client.fetch_userinfo (called when _claim_source() == "userinfo"), logging the failure, and continuing without returning an error—e.g., on exception assign claims_for_mapping = {} (or None) and let the flow proceed to the existing post-auth logic rather than calling _json_error or deleting state cookies; update the try/except around client.fetch_userinfo and remove the early return so claim-mapping is skipped but authentication succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Line 413: Fix the typo in CLAUDE.md by replacing the string "Programmtic
access: Bearer `users.token` accepted in both modes" with "Programmatic access:
Bearer `users.token` accepted in both modes" so the auth behavior note reads
correctly.
In `@docs/oidc.md`:
- Line 724: Update the contradictory TTL for the OIDC `state` cookie so it's
consistent: change the statement referencing a 10-minute TTL to match the
5-minute TTL used elsewhere (lines that mention `openrag_oidc_state` and the
5-minute TTL at lines ~79 and ~749). Ensure the documentation text that names
the cookie `openrag_oidc_state` explicitly states the unified 5-minute TTL.
- Line 47: Several fenced code blocks in docs/oidc.md are missing language
identifiers and trigger markdownlint MD040; update each triple-backtick block
that contains the examples (the block showing "Browser OpenRag
IdP (Keycloak)", the block with the long token
"XFlT-ZfXkdqf0v-5Z8kVt9xhU6c7Z4z0ZY8Z4Z4Z4=", the HTTP example starting with
"POST /auth/backchannel-logout", and the environment example
"OIDC_ENDPOINT=<exact string from jq output>") by adding the appropriate
language tags (e.g., text for the ASCII table, http for the POST request, dotenv
for environment vars) so markdownlint MD040 is satisfied.
In `@docs/sso-quickstart.md`:
- Around line 137-139: The example uses an email for "external_user_id", which
conflicts with guidance that external_user_id must equal the IdP sub (often an
opaque non-email value); update the provisioning example (the JSON block
containing "external_user_id", "email", "is_admin") to use a non-email
placeholder for external_user_id such as "external_user_id": "user-12345" or
"external_sub_value" and ensure the "email" field still shows the user's email
so readers see the distinction.
In `@openrag/api.py`:
- Around line 258-272: The current root_redirect function uses a brittle
substring check of INDEXERUI_URL vs APP_PORT; change root_redirect to accept a
FastAPI Request object and replace the f":{os.getenv('APP_PORT','8080')}"
substring test with a structural comparison using urllib.parse.urlparse on
INDEXERUI_URL and request.url (or request.url.hostname and request.url.port),
normalizing default ports (80 for http, 443 for https) so you compare host+port
equality exactly; if parsed INDEXERUI_URL resolves to the same host/port as the
incoming request, skip the redirect, otherwise return RedirectResponse as
before; keep existing WITH_CHAINLIT_UI and JSONResponse fallback behavior.
In `@openrag/components/auth/deps.py`:
- Around line 57-83: The helper reset_oidc_client uses deprecated event-loop
APIs and drops the task reference; update it to use asyncio.get_running_loop()
to detect a running loop and, if that raises RuntimeError, create a temporary
event loop (asyncio.new_event_loop()) to run old.aclose() synchronously; when
scheduling on a running loop keep a strong reference to the Task by adding it to
a module-level set (e.g. _pending_closes: set[asyncio.Task] = set()), and attach
a done callback to remove the task from that set once complete; keep the
best-effort semantics and the existing exception-suppressing behavior.
In `@openrag/components/auth/middleware.py`:
- Line 158: Wrap the two calls to vectordb.get_user.remote(session["user_id"])
in try/except that catches VDBUserNotFound, revoke/clear the current session
using the same session-revocation logic used in the refresh-failure handling,
and then fall through to the unauthenticated path (i.e., return/continue as the
refresh-failure branch does). Specifically, around the vectordb.get_user.remote
calls, catch VDBUserNotFound, perform the identical session cleanup used in the
refresh-failure code path, and let the middleware proceed as if no authenticated
user exists.
In `@openrag/components/auth/oidc_client.py`:
- Around line 346-349: The logout_token verification currently treats a missing
"exp" as non-failure; update the check in the verification logic (the block
using decoded and now) to require the "exp" claim like "iat" is required: first
assert "exp" in decoded and raise ValueError("logout_token missing exp claim")
if absent, then parse/convert decoded["exp"] to an int and compare it to now
(e.g., if int(decoded["exp"]) < now: raise ValueError("logout_token has
expired")). Ensure you reference the same variables used now (decoded and now)
and keep error messages consistent with the existing "iat" handling.
In `@openrag/components/auth/refresh.py`:
- Around line 53-63: The _to_dt helper can raise ValueError when
datetime.fromisoformat() receives a malformed string but callers (the
stampede-guard block that currently only catches TypeError) expect TypeError;
wrap the datetime.fromisoformat(val) call in a try/except ValueError and
re-raise a TypeError with an explanatory message so all invalid inputs produce
TypeError from _to_dt (or alternatively update the stampede-guard to also catch
ValueError) — change code in function _to_dt to catch ValueError and raise
TypeError, referencing _to_dt and the stampede-guard block that currently only
handles TypeError.
In `@openrag/routers/auth.py`:
- Around line 131-142: The _allowed_next_origins function currently always adds
localhost dev origins; change it so those entries are only added when an
explicit dev signal is present (e.g. os.getenv("ENV") in ("development","dev")
or a new os.getenv("ALLOW_LOCALHOST_REDIRECTS") == "1"); keep the INDEXERUI_URL
behavior unchanged (read indexer_ui and rstrip("/") as before) and return the
set; update the logic in _allowed_next_origins to conditionally add
"http://localhost:3042" and "http://localhost:5173" based on that environment
check.
In `@openrag/scripts/migrations/alembic/versions/f5b6c918f741_add_oidc_auth.py`:
- Around line 50-60: The migration's created_at server_default uses
sa.func.now() which yields TIMESTAMPTZ while the ORM uses naive datetime.now(),
causing semantic mismatch; to fix, add a clear inline comment next to the
"created_at" column definition in the oidc_sessions migration (and mirror it in
the ORM model) stating that the project intentionally uses naive local time
across the auth subsystem, referencing _utcnow() and refresh.py, and that all
producers use datetime.now() to avoid cross-host clock skew; alternatively, if
you prefer UTC semantics instead, replace server_default=sa.func.now() with an
explicit UTC expression such as sa.text("(now() at time zone 'utc')") and adjust
the ORM default to produce a timezone-aware datetime then strip tzinfo (e.g.,
default=lambda: datetime.now(timezone.utc).replace(tzinfo=None)).
In `@tests/api_tests/test_oidc_lifecycle.py`:
- Around line 392-398: The test currently uses a loose negative assertion on
r3.status_code which masks real middleware failures; change it to an explicit
allow-list for accepted statuses when calling client.get("/users/info",
cookies={"openrag_session": session_cookie}) — e.g. assert r3.status_code in
(200, 500) with a clear message so 200 verifies middleware resolved alice and
500 remains allowed as the known task_state_manager=None environment artifact;
update the assertion on r3 (and its failure message) accordingly.
---
Nitpick comments:
In `@openrag/api.py`:
- Around line 142-157: The current startup check only ensures
OIDC_TOKEN_ENCRYPTION_KEY is non-empty; add a fail-fast validation that the
value is a valid Fernet key by attempting to construct a
cryptography.fernet.Fernet instance with OIDC_TOKEN_ENCRYPTION_KEY inside the
AUTH_MODE == "oidc" block (near the existing missing-vars check) and raise a
RuntimeError with a clear message if construction fails; this prevents later
runtime failures in encrypt_token/decrypt_token flows and complements the
existing OIDC_CLAIM_SOURCE validation.
In `@openrag/app_front.py`:
- Around line 58-68: The cookie parser _extract_cookie should handle RFC6265
quoted cookie-values: after locating v in _extract_cookie, detect if it starts
and ends with a double-quote and if so strip the surrounding quotes and unescape
backslash-escaped characters (at least \" and \\) before returning; update the
function to return this unquoted/unescaped value so quoted cookie headers are
parsed correctly.
In `@openrag/components/auth/oidc_client.py`:
- Around line 374-376: The aclose method currently unconditionally closes the
caller-injected AsyncClient (_http), so add an ownership flag (e.g.,
self._owns_http) set in __init__ based on whether http_client was provided; when
you create the internal AsyncClient set _owns_http = True, when a caller
supplies http_client set _owns_http = False; update aclose to only call await
self._http.aclose() if self._owns_http is True (leave reset_oidc_client and
existing tests unchanged).
- Around line 310-368: Add jti replay protection to verify_logout_token by
recording seen (iss, jti) values with a bounded TTL and rejecting duplicates;
specifically, in the verify_logout_token flow inside verify_logout_token (before
returning LogoutTokenClaims and after signature/claim validation) check if
decoded.get("jti") is present, look up (self.issuer, jti) in a short-lived store
(an in-memory LRU with TTL or a small DB table), and if already seen raise
ValueError("logout_token replay detected") or silently ignore processing;
otherwise insert (self.issuer, jti) with TTL equal to token lifetime (exp - iat
or exp - now) so subsequent calls will be treated as replays; ensure the store
is initialized on the OIDC client instance and used by
revoke_oidc_sessions_by_sid callers as needed.
In `@openrag/components/auth/test_state_cookie.py`:
- Around line 60-67: The test TestExpiry.test_expired_cookie_raises_value_error
currently uses time.sleep(1) which is unnecessary and brittle; update the test
to avoid sleeping by making the token immediately considered expired (e.g., call
ser.loads(token, max_age=-1)) and remove the time.sleep call, or alternatively
freeze time around _serializer()/_payload() creation and loading; modify
references in the test (TestExpiry, test_expired_cookie_raises_value_error,
_serializer, _payload, ser.loads, time.sleep) accordingly so the test
deterministically fails with a ValueError without real-time waiting.
In `@openrag/components/indexer/vectordb/models.py`:
- Around line 106-140: Add a time-based index to OIDCSession to support
efficient janitor sweeps: update the OIDCSession model's __table_args__ (in the
OIDCSession class) to include an Index on session_expires_at and/or revoked_at
(e.g., add Index("ix_oidc_sessions_session_expires_at", "session_expires_at")
and/or Index("ix_oidc_sessions_revoked_at", "revoked_at")) so expired or revoked
session lookups used by the sweeper are indexed.
In `@openrag/components/indexer/vectordb/test_oidc_sessions.py`:
- Around line 92-103: The test
test_update_user_fields_updates_display_name_and_lowercases_email contains an
unnecessary pfm.get_user_by_external_id("sub-missing") call and the subsequent
assert refreshed is None used only to silence an unused-variable warning; remove
the pfm.get_user_by_external_id invocation and the refreshed variable and its
assert, leaving the direct ORM re-read via pfm.Session() / s.query(User) and the
two assertions on row.display_name and row.email intact.
In `@openrag/components/indexer/vectordb/utils.py`:
- Line 332: Extract a small helper function named _normalize_email and replace
the inline email normalization in create_user and update_user_fields with calls
to this helper; implement _normalize_email to accept a possibly None value and
return None or the normalized string (e.g., value.strip().lower()) so future
changes (IDN/punycode, RFC parsing) are centralized, then update all occurrences
(including the similar duplication around lines 897-902) to call
_normalize_email(body.email) instead of repeating body.email.strip().lower().
In `@openrag/routers/auth.py`:
- Around line 312-321: The current callback treats a userinfo fetch failure as
fatal; instead make userinfo enrichment optional by catching exceptions from
client.fetch_userinfo (called when _claim_source() == "userinfo"), logging the
failure, and continuing without returning an error—e.g., on exception assign
claims_for_mapping = {} (or None) and let the flow proceed to the existing
post-auth logic rather than calling _json_error or deleting state cookies;
update the try/except around client.fetch_userinfo and remove the early return
so claim-mapping is skipped but authentication succeeds.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 75d2e09f-675b-4ad2-8f53-8493c4727d4b
📒 Files selected for processing (32)
.env.example.gitignoreCLAUDE.mdREADME.mddocker-compose.yamldocs/oidc.mddocs/sso-quickstart.mdextern/indexer-uiopenrag/api.pyopenrag/app_front.pyopenrag/components/auth/__init__.pyopenrag/components/auth/deps.pyopenrag/components/auth/middleware.pyopenrag/components/auth/oidc_client.pyopenrag/components/auth/refresh.pyopenrag/components/auth/session_tokens.pyopenrag/components/auth/state_cookie.pyopenrag/components/auth/test_middleware.pyopenrag/components/auth/test_oidc_client.pyopenrag/components/auth/test_session_tokens.pyopenrag/components/auth/test_state_cookie.pyopenrag/components/indexer/vectordb/models.pyopenrag/components/indexer/vectordb/test_oidc_sessions.pyopenrag/components/indexer/vectordb/utils.pyopenrag/components/indexer/vectordb/vectordb.pyopenrag/models/user.pyopenrag/routers/auth.pyopenrag/routers/test_auth_router.pyopenrag/scripts/migrations/alembic/versions/f5b6c918f741_add_oidc_auth.pypyproject.tomltests/api_tests/OIDC_TEST_COVERAGE.mdtests/api_tests/test_oidc_lifecycle.py
|
|
||
| - UI paths (`/`, `/chainlit`, `/static`) without auth → 302 redirect to `/auth/login?next=...` | ||
| - API paths (`/v1`, `/indexer`, `/search`, etc.) without auth → 401 JSON response | ||
| - Programmtic access: Bearer `users.token` accepted in both modes |
There was a problem hiding this comment.
Fix typo in auth behavior note.
Line 413 has a typo: Programmtic → Programmatic.
Suggested doc fix
-- Programmtic access: Bearer `users.token` accepted in both modes
+- Programmatic access: Bearer `users.token` accepted in both modes📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - Programmtic access: Bearer `users.token` accepted in both modes | |
| - Programmatic access: Bearer `users.token` accepted in both modes |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` at line 413, Fix the typo in CLAUDE.md by replacing the string
"Programmtic access: Bearer `users.token` accepted in both modes" with
"Programmatic access: Bearer `users.token` accepted in both modes" so the auth
behavior note reads correctly.
|
|
||
| ### Authentication Flow Diagram | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add languages to fenced code blocks to satisfy markdownlint (MD040).
These blocks are missing language identifiers.
Suggested doc fix
-```
+```text
Browser OpenRag IdP (Keycloak)
...
-```
+```
-```
+```text
XFlT-ZfXkdqf0v-5Z8kVt9xhU6c7Z4z0ZY8Z4Z4Z4=
-```
+```
-```
+```http
POST /auth/backchannel-logout HTTP/1.1
Host: openrag.example.com
Content-Type: application/x-www-form-urlencoded
...
-```
+```
-```
+```dotenv
OIDC_ENDPOINT=<exact string from jq output>
-```
+```Also applies to: 126-126, 520-520, 623-623
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 47-47: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/oidc.md` at line 47, Several fenced code blocks in docs/oidc.md are
missing language identifiers and trigger markdownlint MD040; update each
triple-backtick block that contains the examples (the block showing "Browser
OpenRag IdP (Keycloak)", the block with the long token
"XFlT-ZfXkdqf0v-5Z8kVt9xhU6c7Z4z0ZY8Z4Z4Z4=", the HTTP example starting with
"POST /auth/backchannel-logout", and the environment example
"OIDC_ENDPOINT=<exact string from jq output>") by adding the appropriate
language tags (e.g., text for the ASCII table, http for the POST request, dotenv
for environment vars) so markdownlint MD040 is satisfied.
| ### CSRF Mitigation | ||
|
|
||
| - Authorization requests use a server-generated `state` parameter | ||
| - The `state` is stored in a temporary, itsdangerous-signed cookie (`openrag_oidc_state`, 10-minute TTL) |
There was a problem hiding this comment.
Resolve contradictory state-cookie TTL documentation.
Line 724 says 10-minute TTL, but Line 79 and Line 749 describe 5-minute TTL. Keep this value consistent across the guide.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/oidc.md` at line 724, Update the contradictory TTL for the OIDC `state`
cookie so it's consistent: change the statement referencing a 10-minute TTL to
match the 5-minute TTL used elsewhere (lines that mention `openrag_oidc_state`
and the 5-minute TTL at lines ~79 and ~749). Ensure the documentation text that
names the cookie `openrag_oidc_state` explicitly states the unified 5-minute
TTL.
Comment on lines
+137
to
+139
| "external_user_id": "alice@mycorp.com", | ||
| "email": "alice@mycorp.com", | ||
| "is_admin": false |
There was a problem hiding this comment.
Use a non-email placeholder for external_user_id in the provisioning example.
Line 137 currently implies email-as-sub. That conflicts with the surrounding guidance that external_user_id must match the IdP sub (often opaque).
Suggested doc fix
- "external_user_id": "alice@mycorp.com",
- "email": "alice@mycorp.com",
+ "external_user_id": "550e8400-e29b-41d4-a716-446655440000",
+ "email": "alice@mycorp.com",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "external_user_id": "alice@mycorp.com", | |
| "email": "alice@mycorp.com", | |
| "is_admin": false | |
| "external_user_id": "550e8400-e29b-41d4-a716-446655440000", | |
| "email": "alice@mycorp.com", | |
| "is_admin": false |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/sso-quickstart.md` around lines 137 - 139, The example uses an email for
"external_user_id", which conflicts with guidance that external_user_id must
equal the IdP sub (often an opaque non-email value); update the provisioning
example (the JSON block containing "external_user_id", "email", "is_admin") to
use a non-email placeholder for external_user_id such as "external_user_id":
"user-12345" or "external_sub_value" and ensure the "email" field still shows
the user's email so readers see the distinction.
Comment on lines
+258
to
+272
| @app.get("/", include_in_schema=False) | ||
| def root_redirect(): | ||
| """Root handler — sends authenticated users to the indexer-ui (if | ||
| configured on a separate host) or the chainlit chat mounted on this | ||
| app. Prevents a bare ``http://localhost:APP_PORT/`` from returning 404 | ||
| after an OIDC login that used ``next=/``. | ||
| """ | ||
| # INDEXERUI_URL always has a default (localhost:INDEXERUI_PORT); only | ||
| # redirect there when it points to a different host/port than us — | ||
| # otherwise we'd loop. | ||
| if INDEXERUI_URL and f":{os.getenv('APP_PORT', '8080')}" not in INDEXERUI_URL: | ||
| return RedirectResponse(url=INDEXERUI_URL, status_code=302) | ||
| if WITH_CHAINLIT_UI: | ||
| return RedirectResponse(url="/chainlit/", status_code=302) | ||
| return JSONResponse({"status": "ok", "app": "openrag", "version": app.version}) |
There was a problem hiding this comment.
Root redirect's port heuristic is brittle in realistic deployments.
The f":{os.getenv('APP_PORT', '8080')}" not in INDEXERUI_URL check misfires in several real scenarios:
- Reverse proxy / TLS termination:
INDEXERUI_URL=https://app.example.comhas no port at all → the substring check passes → you happily redirect to what is very likely this same app, producing the 404 loop this handler was meant to avoid. - Custom APP_PORT:
APP_PORTis not read anywhere else (uvicorn on line 345 hardcodesport=8080), so an operator running uvicorn on a non-default port without remembering to exportAPP_PORTgets wrong behavior. - Substring false positives: a port like
":8080"appearing inside a path/query ofINDEXERUI_URLwould incorrectly match.
Prefer a structural check based on host+port equality (via urllib.parse.urlparse) against the request URL, or drop the auto-redirect entirely and require an explicit OIDC_POST_LOGIN_REDIRECT_URI.
🔧 Sketch using urlparse
-@app.get("/", include_in_schema=False)
-def root_redirect():
+@app.get("/", include_in_schema=False)
+def root_redirect(request: Request):
...
- if INDEXERUI_URL and f":{os.getenv('APP_PORT', '8080')}" not in INDEXERUI_URL:
- return RedirectResponse(url=INDEXERUI_URL, status_code=302)
+ if INDEXERUI_URL:
+ from urllib.parse import urlparse
+ target = urlparse(INDEXERUI_URL)
+ here = request.url
+ same_origin = (target.hostname == here.hostname) and ((target.port or None) == (here.port or None))
+ if not same_origin:
+ return RedirectResponse(url=INDEXERUI_URL, status_code=302)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/api.py` around lines 258 - 272, The current root_redirect function
uses a brittle substring check of INDEXERUI_URL vs APP_PORT; change
root_redirect to accept a FastAPI Request object and replace the
f":{os.getenv('APP_PORT','8080')}" substring test with a structural comparison
using urllib.parse.urlparse on INDEXERUI_URL and request.url (or
request.url.hostname and request.url.port), normalizing default ports (80 for
http, 443 for https) so you compare host+port equality exactly; if parsed
INDEXERUI_URL resolves to the same host/port as the incoming request, skip the
redirect, otherwise return RedirectResponse as before; keep existing
WITH_CHAINLIT_UI and JSONResponse fallback behavior.
Comment on lines
+346
to
+349
| if "iat" not in decoded: | ||
| raise ValueError("logout_token missing iat claim") | ||
| if int(decoded.get("exp", now + 1)) < now: | ||
| raise ValueError("logout_token has expired") |
There was a problem hiding this comment.
exp is silently tolerated when missing.
int(decoded.get("exp", now + 1)) < now evaluates to False when exp is absent, so a logout_token with no expiry passes verification. Per OIDC Back-Channel Logout §2.4, exp is REQUIRED. Either assert presence (consistent with iat handling immediately above) or drop the check if you intentionally accept no-exp tokens.
🛡️ Proposed fix
- if "iat" not in decoded:
- raise ValueError("logout_token missing iat claim")
- if int(decoded.get("exp", now + 1)) < now:
- raise ValueError("logout_token has expired")
+ if "iat" not in decoded:
+ raise ValueError("logout_token missing iat claim")
+ if "exp" not in decoded:
+ raise ValueError("logout_token missing exp claim")
+ if int(decoded["exp"]) < now:
+ raise ValueError("logout_token has expired")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if "iat" not in decoded: | |
| raise ValueError("logout_token missing iat claim") | |
| if int(decoded.get("exp", now + 1)) < now: | |
| raise ValueError("logout_token has expired") | |
| if "iat" not in decoded: | |
| raise ValueError("logout_token missing iat claim") | |
| if "exp" not in decoded: | |
| raise ValueError("logout_token missing exp claim") | |
| if int(decoded["exp"]) < now: | |
| raise ValueError("logout_token has expired") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/components/auth/oidc_client.py` around lines 346 - 349, The
logout_token verification currently treats a missing "exp" as non-failure;
update the check in the verification logic (the block using decoded and now) to
require the "exp" claim like "iat" is required: first assert "exp" in decoded
and raise ValueError("logout_token missing exp claim") if absent, then
parse/convert decoded["exp"] to an int and compare it to now (e.g., if
int(decoded["exp"]) < now: raise ValueError("logout_token has expired")). Ensure
you reference the same variables used now (decoded and now) and keep error
messages consistent with the existing "iat" handling.
Comment on lines
+53
to
+63
| def _to_dt(val: Any) -> datetime: | ||
| """Coerce a datetime-or-ISO-string into a ``datetime``. | ||
|
|
||
| Ray occasionally ships values across actors in serialised form; accept | ||
| either shape so callers never have to care about the transport. | ||
| """ | ||
| if isinstance(val, datetime): | ||
| return val | ||
| if isinstance(val, str): | ||
| return datetime.fromisoformat(val) | ||
| raise TypeError(f"Expected datetime or ISO string, got {type(val).__name__}") |
There was a problem hiding this comment.
_to_dt exception coverage is inconsistent with its own failure modes.
_to_dt explicitly raises TypeError for unexpected types, but datetime.fromisoformat(val) on a malformed string raises ValueError — and the docstring on lines 56–57 specifically calls out string-shape inputs as a supported path (Ray serialization). The stampede-guard block at line 105 only catches TypeError, so a malformed ISO string from the DB/Ray transport would escape as an unhandled ValueError inside the middleware dispatch.
Low likelihood in practice (Postgres + SQLAlchemy returns datetime), but the fix is trivial:
try:
last_refresh_at_dt = _to_dt(last_refresh_at)
- except TypeError:
+ except (TypeError, ValueError):
last_refresh_at_dt = NoneAlso applies to: 102-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/components/auth/refresh.py` around lines 53 - 63, The _to_dt helper
can raise ValueError when datetime.fromisoformat() receives a malformed string
but callers (the stampede-guard block that currently only catches TypeError)
expect TypeError; wrap the datetime.fromisoformat(val) call in a try/except
ValueError and re-raise a TypeError with an explanatory message so all invalid
inputs produce TypeError from _to_dt (or alternatively update the stampede-guard
to also catch ValueError) — change code in function _to_dt to catch ValueError
and raise TypeError, referencing _to_dt and the stampede-guard block that
currently only handles TypeError.
Comment on lines
+131
to
+142
| def _allowed_next_origins() -> set[str]: | ||
| """Origins (scheme://host[:port]) accepted as redirect targets after login. | ||
|
|
||
| Mirrors the CORS allow_origins from ``api.py``: localhost dev ports plus | ||
| ``INDEXERUI_URL`` so that the indexer-ui (served on a different port) can | ||
| receive the user back after the OIDC flow completes. | ||
| """ | ||
| origins = {"http://localhost:3042", "http://localhost:5173"} | ||
| indexer_ui = os.getenv("INDEXERUI_URL") | ||
| if indexer_ui: | ||
| origins.add(indexer_ui.rstrip("/")) | ||
| return origins |
There was a problem hiding this comment.
localhost dev origins are always whitelisted, even in production.
http://localhost:3042 and http://localhost:5173 are unconditionally added to the allow-list. In a production deployment this means /auth/login?next=http://localhost:5173/evil will redirect authenticated users to localhost after login. Impact is limited (no cookie leakage thanks to origin binding, and only users running something on those ports are affected) but it's still an open redirect surface that shouldn't ship to prod.
Gate the dev entries on an environment signal:
🛡️ Proposed fix
def _allowed_next_origins() -> set[str]:
- origins = {"http://localhost:3042", "http://localhost:5173"}
+ origins: set[str] = set()
+ if os.getenv("OPENRAG_ENV", "").lower() in {"dev", "development", "local"}:
+ origins.update({"http://localhost:3042", "http://localhost:5173"})
indexer_ui = os.getenv("INDEXERUI_URL")
if indexer_ui:
origins.add(indexer_ui.rstrip("/"))
return origins🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/routers/auth.py` around lines 131 - 142, The _allowed_next_origins
function currently always adds localhost dev origins; change it so those entries
are only added when an explicit dev signal is present (e.g. os.getenv("ENV") in
("development","dev") or a new os.getenv("ALLOW_LOCALHOST_REDIRECTS") == "1");
keep the INDEXERUI_URL behavior unchanged (read indexer_ui and rstrip("/") as
before) and return the set; update the logic in _allowed_next_origins to
conditionally add "http://localhost:3042" and "http://localhost:5173" based on
that environment check.
Comment on lines
+392
to
+398
| # ── Step 5: GET /users/info with session cookie → 200 alice profile ─────── | ||
| r3 = client.get("/users/info", cookies={"openrag_session": session_cookie}) | ||
| # The users router depends on task_state_manager for file counts; since we | ||
| # stub it as None the endpoint may 500 on full wiring — we accept 200 or | ||
| # verify the middleware resolved alice (status != 401/302). | ||
| assert r3.status_code not in (401, 302), f"Middleware should resolve alice, got {r3.status_code}: {r3.text}" | ||
|
|
There was a problem hiding this comment.
Loose assertion will hide real middleware bugs.
assert r3.status_code not in (401, 302) silently accepts every other status, including 500, 400, 403. The helpful regression signal here is "middleware resolved the session"; a 500 from the stubbed task_state_manager=None is an environment artifact, but a 400/403 would be a genuine middleware bug and this assert would pass. Tighten to an explicit allow-list:
- assert r3.status_code not in (401, 302), f"Middleware should resolve alice, got {r3.status_code}: {r3.text}"
+ assert r3.status_code in (200, 500), (
+ f"Middleware should resolve alice (200 happy path; 500 tolerated due to stubbed "
+ f"task_state_manager). Got {r3.status_code}: {r3.text}"
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/api_tests/test_oidc_lifecycle.py` around lines 392 - 398, The test
currently uses a loose negative assertion on r3.status_code which masks real
middleware failures; change it to an explicit allow-list for accepted statuses
when calling client.get("/users/info", cookies={"openrag_session":
session_cookie}) — e.g. assert r3.status_code in (200, 500) with a clear message
so 200 verifies middleware resolved alice and 500 remains allowed as the known
task_state_manager=None environment artifact; update the assertion on r3 (and
its failure message) accordingly.
This was referenced Apr 19, 2026
Merged
3 tasks
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.
Summary
Adds a second authentication mode,
AUTH_MODE=oidc, that replaces the Chainlit token form with a full OpenID Connect Authorization Code + PKCE flow and supports spec-compliant Back-Channel Logout. Bearerusers.tokenkeeps working for programmatic access (CI, agents, tests).Pairs with linagora/openrag-admin-ui#18 (indexer-ui side).
Design decisions (validated up-front)
/auth/callback— stored SHA-256-hashed in a newoidc_sessionstable. We never try to read back the user'susers.token(which is also hashed) — the two credentials are cleanly separated.external_user_id=sub(fast, stable) →email(with backfill ofexternal_user_id=subon success) → 403, no auto-provisioning.users.tokenstill accepted in oidc mode for programmatic clients. Humans go through OIDC only.sid(OIDC spec); the same IdP-revocation cascades across indexer-ui, Chainlit, and the API surface because they all shareoidc_sessions.last_refresh_at < 5sshort-circuit andSELECT … FOR UPDATE— protects against the refresh-token-rotation stampede.OIDC_TOKEN_ENCRYPTION_KEY).What's in the box
Backend
openrag/components/auth/package (Authlib-based client, session tokens, state cookie, middleware, refresh helper)openrag/routers/auth.py:GET /auth/login,GET /auth/callback,POST /auth/backchannel-logout,GET /auth/logout,GET /auth/meAuthMiddlewarerefactor:openrag_session→oidc_sessionslookupoidc_sessionsthenusers.token(fixes Chainlit'sheader_auth_callbackwhich forwards the cookie as Bearer to/users/info)/auth/login?next=…DB
f5b6c918f741:users.email(unique nullable) +oidc_sessions(encrypted tokens,sid/user_subindexes)UserCreatemodel extended withemail;create_userpersists itPartitionFileManagerexposed as Ray actor methodsChainlit
header_auth_callbackin oidc mode (readsopenrag_sessioncookie)password_auth_callbackgated behindAUTH_MODE != "oidc"Cross-origin (indexer-ui)
_sanitize_next_urlnow accepts absolute URLs whose origin ∈{INDEXERUI_URL, localhost:3042, localhost:5173}(anti open-redirect preserved)docker-compose.yaml:AUTH_MODEpropagated to indexer-ui;extra_hostsfor host-gateway (IdP on the host)Docs
docs/oidc.md(~700 lines): flow diagram, config table, Keycloak + LemonLDAP::NG setup, programmatic access, back-channel logout, troubleshooting (incl. trailing-slash issuer-mismatch gotcha with per-IdP table), security considerationsCLAUDE.mdAuthentication section extendedREADME.md+.env.exampleTests
test_oidc_sessions.py)respx+ RSA JWT signing)/auth/mewhich requires a live middleware)tests/api_tests/test_oidc_lifecycle.py): login → callback → DB assertions →/users/infovia cookie → back-channel-logout → cookie rejectedtests/api_tests/OIDC_TEST_COVERAGE.mdConfiguration
New env vars (see
docs/oidc.mdfor details):AUTH_MODEtoken(default) |oidcOIDC_ENDPOINT.well-known/openid-configuration.issuerbyte-for-byte)OIDC_CLIENT_ID/OIDC_CLIENT_SECRETOIDC_REDIRECT_URIOIDC_TOKEN_ENCRYPTION_KEYOIDC_EMAIL_SOURCEid_token(default) |userinfoOIDC_SCOPESopenid email profile offline_accessOIDC_POST_LOGOUT_REDIRECT_URI/OIDC_ALLOWED_EMAIL_DOMAINSStartup refuses to boot if any required var is missing, with a clear error listing which ones.
Backward compatibility
AUTH_MODE=token(the default) is strictly unchanged. The existing password-auth Chainlit flow,AUTH_TOKENdev bypass, CORS config, and 403 error bodies are all preserved. Token-mode tests pass without modification.Architect review
Done (APPROVED WITH CAVEATS). The two MAJOR items (M1 refresh stampede, M2 timezone) are fixed in this PR. Remaining minors (cookie redaction in logs, startup Fernet-key validity check,
/auth/metest un-skip, open-redirect hardening onpost_logout_redirect_uri) are tracked for a follow-up.Test plan
docker compose up --build -d+docker compose exec openrag-cpu uv run pytest openrag/— unit tests greendocker compose exec openrag-cpu uv run pytest tests/api_tests/— integration green (incl.test_oidc_lifecycle.py)tests/api/users.robotstill returns 403 on bad BearerAUTH_MODE=token→ existing Chainlit login form, unchanged flowAUTH_MODE=oidc+ Keycloak local → login → cookie posed → Chainlit loadscurl -H "Authorization: Bearer <AUTH_TOKEN>" /v1/modelsin oidc mode → 200 (agent path works)Draft
Live testing ongoing. Will be flipped to ready once the full end-to-end flow is verified against a real IdP.
Summary by CodeRabbit
Release Notes
New Features
AUTH_MODEconfigurationDocumentation