refactor(auth): clean up OIDC service architecture - #398
Merged
hedhoud merged 1 commit intoMay 19, 2026
Merged
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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 (14)
📝 WalkthroughWalkthroughThis PR refactors OIDC and authentication logic from the component layer ( ChangesOIDC Auth Service Layer Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
openrag/services/auth/deps.py (1)
71-83: 💤 Low valueConsider using
asyncio.get_running_loop()for better Python 3.10+ compatibility.
asyncio.get_event_loop_policy().get_event_loop()can raiseDeprecationWarningin Python 3.10+ when called outside an async context. A safer pattern would catchRuntimeErrorfromget_running_loop()to distinguish the two cases.Suggested alternative
try: import asyncio - loop = asyncio.get_event_loop_policy().get_event_loop() - if loop.is_running(): - # Schedule close on the running loop without awaiting — caller - # doesn't need to be async. + try: + loop = asyncio.get_running_loop() loop.create_task(old.aclose()) - else: + except RuntimeError: + # No running loop — run synchronously. + asyncio.run(old.aclose()) - loop.run_until_complete(old.aclose()) except Exception: # Closing is best-effort; never let a reset blow up the caller. pass🤖 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 `@openrag/services/auth/deps.py` around lines 71 - 83, Replace the current event-loop detection with asyncio.get_running_loop() and explicitly handle RuntimeError to detect "no running loop": call asyncio.get_running_loop() in a try/except, and if it returns a loop use loop.create_task(old.aclose()), otherwise (RuntimeError) obtain a non-running loop (e.g. via get_event_loop_policy().get_event_loop() or by creating a new event loop) and call loop.run_until_complete(old.aclose()) — keep the operation best-effort and preserve the existing swallow-on-exception behavior; reference the existing old.aclose() call and use asyncio.get_running_loop(), loop.create_task, and loop.run_until_complete in the fix.openrag/routers/test_auth_router.py (1)
287-295: ⚡ Quick winAdd
bind()to the logger stub interface.This stub can fail if router code uses contextual logging (
get_logger().bind(...).info(...)). Add a no-opbindreturning the logger object.Proposed patch
logger_stub = types.ModuleType("utils.logger") - logger_stub.get_logger = lambda: types.SimpleNamespace( - debug=lambda *args, **kwargs: None, - info=lambda *args, **kwargs: None, - warning=lambda *args, **kwargs: None, - error=lambda *args, **kwargs: None, - exception=lambda *args, **kwargs: None, - ) + class _NoopLogger: + def bind(self, **kwargs): + return self + def debug(self, *args, **kwargs): ... + def info(self, *args, **kwargs): ... + def warning(self, *args, **kwargs): ... + def error(self, *args, **kwargs): ... + def exception(self, *args, **kwargs): ... + + logger_stub.get_logger = lambda: _NoopLogger()As per coding guidelines, "Use Loguru for structured logging with the
get_logger()utility fromopenrag/utils/logger, and bind contextual information (e.g., file_id, partition) to log messages".🤖 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 `@openrag/routers/test_auth_router.py` around lines 287 - 295, The logger stub returned by get_logger in the test (logger_stub.get_logger -> types.SimpleNamespace) lacks a bind method, so any router code calling get_logger().bind(...).info(...) will fail; add a no-op bind on the returned logger object (a method that accepts *args, **kwargs and returns the same logger instance) so contextual chaining works; update the SimpleNamespace created in logger_stub.get_logger to include this bind method while keeping existing debug/info/warning/error/exception no-ops.tests/api_tests/test_oidc_lifecycle.py (1)
252-260: ⚡ Quick winMirror Loguru’s
bind()in the test logger stub.This stub should expose
bind()to avoid AttributeError when code paths use contextual logging.Proposed patch
logger_stub = types.ModuleType("utils.logger") - logger_stub.get_logger = lambda: types.SimpleNamespace( - debug=lambda *args, **kwargs: None, - info=lambda *args, **kwargs: None, - warning=lambda *args, **kwargs: None, - error=lambda *args, **kwargs: None, - exception=lambda *args, **kwargs: None, - ) + class _NoopLogger: + def bind(self, **kwargs): + return self + def debug(self, *args, **kwargs): ... + def info(self, *args, **kwargs): ... + def warning(self, *args, **kwargs): ... + def error(self, *args, **kwargs): ... + def exception(self, *args, **kwargs): ... + + logger_stub.get_logger = lambda: _NoopLogger()As per coding guidelines, "Use Loguru for structured logging with the
get_logger()utility fromopenrag/utils/logger, and bind contextual information (e.g., file_id, partition) to log messages".🤖 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/api_tests/test_oidc_lifecycle.py` around lines 252 - 260, The test logger stub lacks a bind() method, causing AttributeError when code expects Loguru-style contextual logging; update the logger_stub.get_logger to return a logger-like object that includes a bind(self, **kwargs) method which returns the same (or a shallow copy of the) SimpleNamespace logger so callers can chain bind(...). Ensure the returned object still exposes debug/info/warning/error/exception and that bind simply returns an object with those same methods (preserving the existing lambdas) so tests using bind(file_id=..., partition=...) work without 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.
Inline comments:
In `@openrag/services/auth/oidc_client.py`:
- Around line 355-356: The logout-token nonce check uses decoded.get("nonce")
which treats empty/falsy values as missing; change the check to test key
presence (e.g., if "nonce" in decoded) so any presence of the "nonce" claim
(even empty string) triggers the same ValueError; update the conditional at the
decoded nonce check and keep the existing ValueError("logout_token must not
contain nonce") behavior.
In `@openrag/services/auth/refresh.py`:
- Line 109: Replace direct awaits of Ray actor method calls with the centralized
timeout wrapper: import call_ray_actor_with_timeout from
services.workers.ray_utils and change lines that call
vectordb.get_oidc_session_by_id.remote(...), and the two other
vectordb.<method>.remote(...) calls in this function to await
call_ray_actor_with_timeout(<actor_call>, timeout=<appropriate seconds>,
task_description="Re-read OIDC session for stampede recovery" or a short
descriptive string for the other two calls). Use the same timeout policy used
elsewhere in this module and ensure the task_description clearly identifies the
operation; keep the remote(...) call as the first argument to
call_ray_actor_with_timeout.
---
Nitpick comments:
In `@openrag/routers/test_auth_router.py`:
- Around line 287-295: The logger stub returned by get_logger in the test
(logger_stub.get_logger -> types.SimpleNamespace) lacks a bind method, so any
router code calling get_logger().bind(...).info(...) will fail; add a no-op bind
on the returned logger object (a method that accepts *args, **kwargs and returns
the same logger instance) so contextual chaining works; update the
SimpleNamespace created in logger_stub.get_logger to include this bind method
while keeping existing debug/info/warning/error/exception no-ops.
In `@openrag/services/auth/deps.py`:
- Around line 71-83: Replace the current event-loop detection with
asyncio.get_running_loop() and explicitly handle RuntimeError to detect "no
running loop": call asyncio.get_running_loop() in a try/except, and if it
returns a loop use loop.create_task(old.aclose()), otherwise (RuntimeError)
obtain a non-running loop (e.g. via get_event_loop_policy().get_event_loop() or
by creating a new event loop) and call loop.run_until_complete(old.aclose()) —
keep the operation best-effort and preserve the existing swallow-on-exception
behavior; reference the existing old.aclose() call and use
asyncio.get_running_loop(), loop.create_task, and loop.run_until_complete in the
fix.
In `@tests/api_tests/test_oidc_lifecycle.py`:
- Around line 252-260: The test logger stub lacks a bind() method, causing
AttributeError when code expects Loguru-style contextual logging; update the
logger_stub.get_logger to return a logger-like object that includes a bind(self,
**kwargs) method which returns the same (or a shallow copy of the)
SimpleNamespace logger so callers can chain bind(...). Ensure the returned
object still exposes debug/info/warning/error/exception and that bind simply
returns an object with those same methods (preserving the existing lambdas) so
tests using bind(file_id=..., partition=...) work without errors.
🪄 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: a89eb4b3-97d4-4e99-bac1-d373ab8b261b
📒 Files selected for processing (14)
openrag/components/auth/deps.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/routers/test_auth_router.pyopenrag/services/auth/__init__.pyopenrag/services/auth/deps.pyopenrag/services/auth/oidc_client.pyopenrag/services/auth/refresh.pyopenrag/services/auth/session_tokens.pyopenrag/services/auth/state_cookie.pytests/api_tests/test_oidc_lifecycle.py
hedhoud
force-pushed
the
refactor/cleaner-auth-architecture
branch
from
May 19, 2026 13:59
94fe029 to
aa4aefb
Compare
EnjoyBacon7
pushed a commit
that referenced
this pull request
May 26, 2026
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
1 participant
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.
Context
This continues the hexagonal refactor by moving OIDC auth behavior out of the component layer and into the service layer. The goal is to make auth easier to test and evolve without keeping business logic tied to legacy component imports.
Problem
The OIDC client, session token handling, state cookie handling, and refresh flow still lived under
components. That made the auth boundary blurry and forced newer code to depend on legacy locations.Expected behavior
Existing imports continue to work through compatibility shims, while new code can depend on the service-layer auth package directly. The behavior should remain unchanged; this PR is about ownership and cleaner boundaries, not a product change.
Validation
Layer import guard passes. Focused Python syntax checks pass using an isolated pycache path. The focused pytest command could not start in this environment because
psycopg2requirespg_configto build.Summary by CodeRabbit
Release Notes