refactor(auth): move auth adapters to services/auth (phase 6F) - #372
refactor(auth): move auth adapters to services/auth (phase 6F)#372EnjoyBacon7 wants to merge 1 commit into
Conversation
Move oidc_client, refresh, session_tokens, state_cookie, and deps from components/auth/ to services/auth/. Old files become re-export shims. Middleware stays in components/auth/ (phase 10). Tests updated to patch the canonical module for stampede-guard tests.
📝 WalkthroughWalkthroughAuthentication logic and utilities are moved from ChangesAuth Implementation Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openrag/components/auth/test_session_tokens.py (1)
3-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix import ordering to resolve Ruff
I001failure.CI is currently failing on this import block ordering/format, so this needs a small reorder to unblock merge.
Proposed diff
-import pytest -from services.auth.session_tokens import ( - decrypt_token, - encrypt_token, - hash_session_token, - issue_session_token, -) from cryptography.fernet import Fernet +import pytest + +from services.auth.session_tokens import ( + decrypt_token, + encrypt_token, + hash_session_token, + issue_session_token, +)🤖 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/components/auth/test_session_tokens.py` around lines 3 - 10, The import block triggers Ruff I001 due to incorrect ordering; reorder imports into groups (stdlib, third-party, local) and alphabetize within groups: put third-party imports first in alphabetical order (cryptography.fernet.Fernet then pytest) followed by the local import from services.auth.session_tokens (decrypt_token, encrypt_token, hash_session_token, issue_session_token). Ensure there is a blank line between the third-party group and the local group so the import order and grouping conform to Ruff I001.
🧹 Nitpick comments (4)
openrag/services/auth/oidc_client.py (3)
84-88: 💤 Low valueOptional: a single 10 s timeout covers connect+read+write+pool together — split for clarity.
httpx.AsyncClient(timeout=10.0)applies the same value to all four phases. For OIDC discovery / token exchange against a remote IdP, a slightly more generous read timeout with a tighter connect timeout (e.g.httpx.Timeout(connect=5.0, read=15.0, write=10.0, pool=5.0)) tends to fail fast on dead IdPs while tolerating brief processing latency. Not a defect, just a tuning hint.🤖 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/oidc_client.py` around lines 84 - 88, The single 10s timeout on creating the AsyncClient should be replaced with a split httpx.Timeout to separate connect/read/write/pool phases; update the AsyncClient instantiation (where self._http is assigned in the OIDC client constructor, e.g. in __init__ or class OIDCClient) to use httpx.Timeout(connect=5.0, read=15.0, write=10.0, pool=5.0) (or similar tuned values) instead of timeout=10.0 so connect failures fail fast while allowing a longer read window for token/discovery responses.
263-304: 💤 Low valueConsider clock-skew tolerance for
expand lower-bound check oniat.Two small hardening tweaks for
_verify_id_token(and mirrored inverify_logout_token):
int(decoded["exp"]) < now: real-world clock drift between RP and IdP regularly trips this on short-lived tokens. A small leeway (e.g. 30 s) —int(decoded["exp"]) + leeway < now— matches whatauthlib/pyjwtprovide out of the box.iatis checked for presence but not value; rejecting aniatmore than a few minutes in the future would catch grossly mis-clocked or replayed tokens.Skip if you'd rather defer hardening to the future
joserfcmigration.🤖 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/oidc_client.py` around lines 263 - 304, In _verify_id_token (and mirror the change in verify_logout_token) add a small clock-skew leeway (e.g. leeway = 30) and use it when validating exp and iat: treat the token as expired only if int(decoded["exp"]) + leeway < now, and after confirming iat exists also reject if int(decoded["iat"]) > now + leeway (i.e. iat too far in the future); store the leeway as a local constant or configurable attribute and ensure you convert claims to int before comparisons and raise the existing ValueError messages on failure.
27-29: ⚖️ Poor tradeoffMigrate from deprecated
authlib.josetojoserfc.The deprecated
authlib.josemodule (frozen since Authlib 1.7.0, removal planned for 2.0.0) should be replaced withjoserfc. This module is newly introduced in the OIDC client, making the migration straightforward:
JsonWebKey.import_key_set()→joserfc.jwk.import_key_set()JsonWebToken()initialization and.decode()→joserfc.jwtequivalentsJoseErrorexception handling → joserfc exception typesThe migration spans 5 files (1 main + 4 tests) and avoids larger refactoring costs later when the code becomes more dependent on these primitives.
🤖 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/oidc_client.py` around lines 27 - 29, Replace deprecated authlib.jose usage with joserfc equivalents: change imports to use joserfc.jwk and joserfc.jwt, update JsonWebKey.import_key_set calls to joserfc.jwk.import_key_set, replace JsonWebToken() creation and .decode() usage with the joserfc.jwt APIs (use the appropriate joserfc.jwt.decode/verify pattern), and swap JoseError exception handling to the corresponding joserfc exception types; locate and update these references (JsonWebKey, JsonWebToken, JoseError) in oidc_client.py and mirror the same changes in the four related test files to ensure compatibility.openrag/services/auth/refresh.py (1)
153-155: 💤 Low valueMinor:
expires_in or 0swallows a0literal but that's then re-floored to 60s — fine; just confirm IdP intent.If a real IdP returns
expires_in: 0(rare but legal for short-lived tokens), this code grants the token a synthetic 60 s lifetime instead of immediately retrying. Documented behaviour, but worth a one-line comment so future maintainers don't "fix" the clamp.🤖 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/refresh.py` around lines 153 - 155, The code currently clamps bundle.expires_in to a minimum of 60s when computing new_access_exp (new_access_exp = now + timedelta(seconds=max(int(bundle.expires_in or 0), 60))); add a one-line comment immediately above this expression explaining that a literal 0 from the IdP is intentionally treated as a synthetic 60s grace period (so we don’t immediately expire tokens) and note that this is deliberate behavior to preserve short-lived tokens, so future maintainers won't "fix" the clamp; mention bundle.expires_in, new_access_exp and the max(..., 60) clamp in that comment.
🤖 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/deps.py`:
- Around line 71-80: Replace the deprecated
asyncio.get_event_loop_policy().get_event_loop() usage: attempt to obtain the
running loop with asyncio.get_running_loop() and if that raises RuntimeError use
asyncio.run(old.aclose()) as the offline fallback; when running, schedule
old.aclose() with loop.create_task but keep a strong reference by adding the
Task to a module-level set (e.g., pending_closes) and attach
task.add_done_callback(lambda t: pending_closes.discard(t)) so the task is not
garbage-collected before completion and the set is cleaned up after done.
In `@openrag/services/auth/oidc_client.py`:
- Around line 346-353: The logout token verifier (verify_logout_token) currently
accepts tokens missing the exp claim by using decoded.get("exp", now + 1);
change this to explicitly require and validate exp like _verify_id_token does:
raise ValueError if "exp" not in decoded, then parse int(decoded["exp"]) and
compare to now to detect expiration (no default), and keep the existing
iat/already-present events checks (decoded, iat, events) so exp is enforced per
OIDC Back-Channel Logout spec.
---
Outside diff comments:
In `@openrag/components/auth/test_session_tokens.py`:
- Around line 3-10: The import block triggers Ruff I001 due to incorrect
ordering; reorder imports into groups (stdlib, third-party, local) and
alphabetize within groups: put third-party imports first in alphabetical order
(cryptography.fernet.Fernet then pytest) followed by the local import from
services.auth.session_tokens (decrypt_token, encrypt_token, hash_session_token,
issue_session_token). Ensure there is a blank line between the third-party group
and the local group so the import order and grouping conform to Ruff I001.
---
Nitpick comments:
In `@openrag/services/auth/oidc_client.py`:
- Around line 84-88: The single 10s timeout on creating the AsyncClient should
be replaced with a split httpx.Timeout to separate connect/read/write/pool
phases; update the AsyncClient instantiation (where self._http is assigned in
the OIDC client constructor, e.g. in __init__ or class OIDCClient) to use
httpx.Timeout(connect=5.0, read=15.0, write=10.0, pool=5.0) (or similar tuned
values) instead of timeout=10.0 so connect failures fail fast while allowing a
longer read window for token/discovery responses.
- Around line 263-304: In _verify_id_token (and mirror the change in
verify_logout_token) add a small clock-skew leeway (e.g. leeway = 30) and use it
when validating exp and iat: treat the token as expired only if
int(decoded["exp"]) + leeway < now, and after confirming iat exists also reject
if int(decoded["iat"]) > now + leeway (i.e. iat too far in the future); store
the leeway as a local constant or configurable attribute and ensure you convert
claims to int before comparisons and raise the existing ValueError messages on
failure.
- Around line 27-29: Replace deprecated authlib.jose usage with joserfc
equivalents: change imports to use joserfc.jwk and joserfc.jwt, update
JsonWebKey.import_key_set calls to joserfc.jwk.import_key_set, replace
JsonWebToken() creation and .decode() usage with the joserfc.jwt APIs (use the
appropriate joserfc.jwt.decode/verify pattern), and swap JoseError exception
handling to the corresponding joserfc exception types; locate and update these
references (JsonWebKey, JsonWebToken, JoseError) in oidc_client.py and mirror
the same changes in the four related test files to ensure compatibility.
In `@openrag/services/auth/refresh.py`:
- Around line 153-155: The code currently clamps bundle.expires_in to a minimum
of 60s when computing new_access_exp (new_access_exp = now +
timedelta(seconds=max(int(bundle.expires_in or 0), 60))); add a one-line comment
immediately above this expression explaining that a literal 0 from the IdP is
intentionally treated as a synthetic 60s grace period (so we don’t immediately
expire tokens) and note that this is deliberate behavior to preserve short-lived
tokens, so future maintainers won't "fix" the clamp; mention bundle.expires_in,
new_access_exp and the max(..., 60) clamp in that comment.
🪄 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: bfac39c0-c307-4beb-a8e0-a1af361aa780
📒 Files selected for processing (16)
openrag/components/auth/__init__.pyopenrag/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/components/auth/test_oidc_client.pyopenrag/components/auth/test_session_tokens.pyopenrag/components/auth/test_state_cookie.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.py
| try: | ||
| import asyncio | ||
|
|
||
| loop = asyncio.get_event_loop_policy().get_event_loop() | ||
| if loop.is_running(): | ||
| loop.create_task(old.aclose()) | ||
| else: | ||
| loop.run_until_complete(old.aclose()) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is asyncio.get_event_loop_policy().get_event_loop() deprecated in Python 3.12 / 3.13?
💡 Result:
Yes, asyncio.get_event_loop_policy.get_event_loop is deprecated in Python 3.12 and remains deprecated in Python 3.13. Specifically, the get_event_loop method of the default asyncio policy (accessed via get_event_loop_policy) emits a DeprecationWarning since Python 3.12 if there is no current event loop set and it decides to create one; in future releases, this will become an error [1][2][3]. The full policy system, including get_event_loop_policy, is deprecated since Python 3.14 and slated for removal in 3.16 [4][5][6], but the specific deprecation for this usage pattern started in 3.12 [7]. In Python 3.13 docs, the policy page does not yet show the top-level deprecation notice (added in 3.14), but the method's deprecation is noted [2]. Projects are already adapting by avoiding it [8][9]. Recommended replacements: use asyncio.get_running_loop inside coroutines, asyncio.new_event_loop for manual loop creation, or asyncio.run for top-level entry [10][11].
Citations:
- 1: https://docs.python.org/3.12/library/asyncio-policy.html
- 2: https://docs.python.org/3.13/library/asyncio-policy.html
- 3: https://docs.python.org/3.13/deprecations/index.html
- 4: Deprecate asyncio policy system python/cpython#127949
- 5: https://docs.python.org/3.14/library/asyncio-policy.html
- 6: https://github.com/python/cpython/blob/main/Doc/library/asyncio-policy.rst
- 7: https://docs.python.org/3/whatsnew/3.12.html
- 8: [Python][Cleanup]
asyncio.get_event_loop_policy()will be removed in Python 3.16 grpc/grpc#39518 - 9: https://aiohttp.readthedocs.io/en/stable/changes.html
- 10: https://stackoverflow.com/questions/78577538/replacement-for-deprecated-asyncio-get-event-loop
- 11: https://docs.python.org/3.13/library/asyncio-eventloop.html
🏁 Script executed:
find . -type f -name "deps.py" | grep -E "auth|services" | head -5Repository: linagora/openrag
Length of output: 126
🏁 Script executed:
cat -n ./openrag/services/auth/deps.py | head -100Repository: linagora/openrag
Length of output: 3214
🏁 Script executed:
head -20 ./openrag/services/auth/deps.py && echo "---" && tail -5 ./openrag/services/auth/deps.pyRepository: linagora/openrag
Length of output: 858
🏁 Script executed:
# Check for Python version constraints in the project
find . -type f \( -name "pyproject.toml" -o -name "setup.py" -o -name ".python-version" \) -exec cat {} \; | grep -A 5 -B 5 "python"Repository: linagora/openrag
Length of output: 877
🏁 Script executed:
# Check if there are existing async utilities in the codebase that handle task cleanup
rg -t py "asyncio.get_running_loop\|asyncio.run\|_pending" openrag/ | head -20Repository: linagora/openrag
Length of output: 42
🏁 Script executed:
# Check if there's a pattern of async cleanup or task tracking elsewhere in the codebase
rg -t py "create_task|add_done_callback" openrag/ | head -20Repository: linagora/openrag
Length of output: 305
🏁 Script executed:
# Check the OIDCClient implementation to understand what aclose() does
rg -t py "class OIDCClient" openrag/ -A 20 | head -40Repository: linagora/openrag
Length of output: 1432
🏁 Script executed:
# Check the aclose method in OIDCClient
rg -t py "def aclose|async def aclose" openrag/services/auth/oidc_client.py -A 5Repository: linagora/openrag
Length of output: 178
🏁 Script executed:
# Check for test files that use reset_oidc_client
rg -t py "reset_oidc_client" openrag/Repository: linagora/openrag
Length of output: 839
Replace deprecated asyncio.get_event_loop_policy().get_event_loop() with asyncio.get_running_loop() + asyncio.run() fallback.
Two issues in this test hook cleanup path:
asyncio.get_event_loop_policy().get_event_loop()emits aDeprecationWarningon Python 3.12+ (the project's minimum version) and is scheduled for removal in a future release. Useasyncio.get_running_loop()(raisesRuntimeErrorif none) withasyncio.run()as the offline fallback.loop.create_task(old.aclose())is fire-and-forget without a strong reference — the task may be garbage-collected before completion. Retain a module-level set of pending closes and usetask.add_done_callback()to clean up the reference.
Suggested fix
- try:
- import asyncio
-
- loop = asyncio.get_event_loop_policy().get_event_loop()
- if loop.is_running():
- loop.create_task(old.aclose())
- else:
- loop.run_until_complete(old.aclose())
- except Exception:
- pass
+ try:
+ import asyncio
+
+ try:
+ loop = asyncio.get_running_loop()
+ except RuntimeError:
+ # No loop running -- safe to drive the close synchronously.
+ asyncio.run(old.aclose())
+ else:
+ # Keep a strong reference so the task isn't GC'd mid-flight.
+ task = loop.create_task(old.aclose())
+ _pending_closes.add(task)
+ task.add_done_callback(_pending_closes.discard)
+ except Exception:
+ passAnd at module scope:
+_pending_closes: set = set()📝 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.
| try: | |
| import asyncio | |
| loop = asyncio.get_event_loop_policy().get_event_loop() | |
| if loop.is_running(): | |
| loop.create_task(old.aclose()) | |
| else: | |
| loop.run_until_complete(old.aclose()) | |
| except Exception: | |
| pass | |
| try: | |
| import asyncio | |
| try: | |
| loop = asyncio.get_running_loop() | |
| except RuntimeError: | |
| # No loop running -- safe to drive the close synchronously. | |
| asyncio.run(old.aclose()) | |
| else: | |
| # Keep a strong reference so the task isn't GC'd mid-flight. | |
| task = loop.create_task(old.aclose()) | |
| _pending_closes.add(task) | |
| task.add_done_callback(_pending_closes.discard) | |
| except Exception: | |
| 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 - 80, Replace the deprecated
asyncio.get_event_loop_policy().get_event_loop() usage: attempt to obtain the
running loop with asyncio.get_running_loop() and if that raises RuntimeError use
asyncio.run(old.aclose()) as the offline fallback; when running, schedule
old.aclose() with loop.create_task but keep a strong reference by adding the
Task to a module-level set (e.g., pending_closes) and attach
task.add_done_callback(lambda t: pending_closes.discard(t)) so the task is not
garbage-collected before completion and the set is cleaned up after done.
| 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") | ||
|
|
||
| events = decoded.get("events") or {} | ||
| if "http://schemas.openid.net/event/backchannel-logout" not in events: | ||
| raise ValueError("logout_token missing required back-channel-logout event claim") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the oidc_client.py file to verify the claims
fd -type f -name "oidc_client.py" | head -1Repository: linagora/openrag
Length of output: 231
🏁 Script executed:
# Read the oidc_client.py file to check the _verify_id_token and verify_logout_token implementations
cat -n openrag/services/auth/oidc_client.py | sed -n '280,360p'Repository: linagora/openrag
Length of output: 3889
🏁 Script executed:
# Also check if there are any imports or error handling patterns for token validation
cat -n openrag/services/auth/oidc_client.py | head -50Repository: linagora/openrag
Length of output: 1771
🌐 Web query:
OIDC Back-Channel Logout 1.0 spec exp claim requirement section 2.6
💡 Result:
In the OpenID Connect Back-Channel Logout 1.0 specification, section 2.6 (Logout Token Validation) requires the Relying Party (RP) to validate the "exp" (expiration time) claim in the Logout Token. Specifically, step 4 states: "Validate the 'iss', 'aud', 'iat', and 'exp' Claims in the same way they are validated in ID Tokens."[1][2] The "exp" claim itself is REQUIRED in the Logout Token, as defined in section 2.5 (Logout Token), where it is listed as: "exp REQUIRED. Expiration time, as specified in Section 2 of [OpenID.Core]."[1][2] Validation follows OpenID Connect Core 1.0 rules: the current time MUST be before or equal to the expiration time, and the tolerance (allowing for clock skew) is no more than 4 minutes (per Core spec Section 2).[1][2]
Citations:
- 1: https://openid.net/specs/openid-connect-backchannel-1_0.txt
- 2: https://openid.net/specs/openid-connect-backchannel-1_0.html
Add explicit exp claim validation to logout token verifier — required by OIDC Back-Channel Logout spec.
The verify_logout_token method at line 348 silently accepts logout tokens missing the exp claim due to the default value in decoded.get("exp", now + 1). Per OIDC Back-Channel Logout 1.0 spec §2.6 step 4, RPs must "Validate the iss, aud, iat, and exp Claims in the same way they are validated in ID Tokens", and section 2.5 marks exp as REQUIRED in the Logout Token. Your _verify_id_token method (lines 292–293) correctly rejects ID tokens lacking exp, but verify_logout_token allows it.
Without requiring exp, an attacker with access to the IdP signing key could forge a never-expiring logout token to forcibly invalidate arbitrary sessions indefinitely. Requiring exp constrains the impact window.
Suggested fix
if "iat" not in decoded:
raise ValueError("logout_token missing iat claim")
- if int(decoded.get("exp", now + 1)) < now:
+ 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.
| 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") | |
| events = decoded.get("events") or {} | |
| if "http://schemas.openid.net/event/backchannel-logout" not in events: | |
| raise ValueError("logout_token missing required back-channel-logout event claim") | |
| 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") | |
| events = decoded.get("events") or {} | |
| if "http://schemas.openid.net/event/backchannel-logout" not in events: | |
| raise ValueError("logout_token missing required back-channel-logout event claim") |
🤖 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/oidc_client.py` around lines 346 - 353, The logout
token verifier (verify_logout_token) currently accepts tokens missing the exp
claim by using decoded.get("exp", now + 1); change this to explicitly require
and validate exp like _verify_id_token does: raise ValueError if "exp" not in
decoded, then parse int(decoded["exp"]) and compare to now to detect expiration
(no default), and keep the existing iat/already-present events checks (decoded,
iat, events) so exp is enforced per OIDC Back-Channel Logout spec.
Summary
components/auth/toservices/auth/components/auth/files become thin re-export shims so all downstream consumers (middleware, routers, main) keep workingcomponents/auth/(moves in phase 10)services.auth.refreshmodule for stampede-guard testsTest plan
pytest openrag/components/auth/)Summary by CodeRabbit
Refactor
New Features