Refactor: Extract JWT validation logic to infrastructure layer - #827
Open
YKDBontekoe wants to merge 4 commits into
Open
YKDBontekoe wants to merge 4 commits into
YKDBontekoe wants to merge 4 commits into
Conversation
Moves the Supabase JWT validation logic out of the domain-layer `AuthService` into a dedicated `SupabaseJWTVerifier` inside the `infrastructure/external` module. This adheres to Clean Architecture principles by isolating the external `PyJWKClient` and network IO dependencies from the application's business logic. - Implements `JWTVerifierProtocol` in the domain layer. - Refactors FastAPI dependencies to expose `get_jwt_verifier()`. - Updates authentication utilities and `websocket.py` to use the injected interface. - Adapts unit tests to test the new external verifier class directly. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Contributor
|
Warning Review limit reached
More reviews will be available in 41 minutes and 12 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughJWT verification logic is extracted from ChangesJWT Verifier Extraction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| async def verify_token(self, token: str) -> JWTPayload: | ||
| """Decode and verify a JWT.""" | ||
| ... |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Moves the Supabase JWT validation logic out of the domain-layer `AuthService` into a dedicated `SupabaseJWTVerifier` inside the `infrastructure/external` module. This adheres to Clean Architecture principles by isolating the external `PyJWKClient` and network IO dependencies from the application's business logic. - Implements `JWTVerifierProtocol` in the domain layer. - Refactors FastAPI dependencies to expose `get_jwt_verifier()`. - Updates authentication utilities and `websocket.py` to use the injected interface. - Adapts unit tests to test the new external verifier class directly. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@backend/app/api/dependencies.py`:
- Around line 74-76: The public dependency provider get_jwt_verifier() is
missing a Google-style Returns section in its docstring. Update the docstring
for get_jwt_verifier to explicitly document that it returns the singleton
JWTVerifierProtocol instance, keeping the docstring in Google format used by
other public functions and classes.
In `@backend/app/api/v1/websocket.py`:
- Around line 53-54: The `_verify_token` flow in `websocket_chat_hub` is still
calling `get_jwt_verifier()` directly, which bypasses FastAPI dependency
overrides and breaks the DI migration. Inject the JWT verifier into
`websocket_chat_hub` using FastAPI `Depends`, then pass that verifier into
`_verify_token` instead of fetching it inside the helper. Keep the token
verification logic in `_verify_token` the same, but make it use the injected
verifier instance.
In `@backend/app/domain/auth/interfaces.py`:
- Around line 12-17: The public protocol API in JWTVerifierProtocol and its
verify_token method needs Google-style docstrings instead of the current simple
docstring. Update the class and method documentation to include clear Args and
Returns sections so the contract for the token parameter and JWTPayload result
is explicit for implementers.
In `@backend/app/infrastructure/external/jwt_verifier.py`:
- Around line 15-27: Add Google-style docstrings to the public verifier API by
documenting the SupabaseJWTVerifier class and its verify_token() method; update
the class-level docstring to describe its purpose, and add a method docstring
for verify_token() that includes Args for token and Returns for JWTPayload,
matching the project’s Python docstring standard. Also ensure any other public
methods in SupabaseJWTVerifier follow the same Google-style format if needed.
- Around line 45-46: The synchronous JWKS lookup in JwtVerifier.verify_token
blocks the event loop because PyJWKClient.get_signing_key_from_jwt() may refresh
JWKS on cache miss or rotation. Move the key resolution in
JwtVerifier._get_jwks_client/verify_token off the loop by running the
get_signing_key_from_jwt call with asyncio.to_thread(...) or by replacing it
with an async JWKS fetch/cache implementation, while keeping the rest of the
token verification flow unchanged.
- Around line 30-56: Narrow the broad exception handling in JWTVerifier’s token
verification flow so expected JWT/JWKS errors are handled separately from
unexpected failures. In jwt_verifier.py, update the try/except around
get_unverified_header, jwt.decode, and JWKS lookup to catch jwt.PyJWTError for
validation-related issues, and use logger.exception in the unexpected error path
to preserve tracebacks. Keep JWTPayload construction and the existing
verification logic intact, but avoid a blanket except Exception that downgrades
all failures to warnings.
In `@backend/tests/test_auth.py`:
- Around line 24-28: Add a test that exercises the JWKS verification path in
SupabaseJWTVerifier.verify_token by using a non-HS256 token so execution reaches
the PyJWKClient branch and resolves a signing key. In
backend/tests/test_auth.py, mock PyJWKClient and the signing-key lookup with
unittest.mock.patch, then assert the verified payload is returned (and add an
error/edge-case assertion if key resolution fails). Keep the existing
HS256/malformed-token tests, but ensure this new case covers the else branch in
jwt_verifier.py.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 28e7134a-2ffa-4d43-b240-a85f16c64619
📒 Files selected for processing (7)
backend/app/api/dependencies.pybackend/app/api/v1/websocket.pybackend/app/core/security/auth.pybackend/app/domain/auth/interfaces.pybackend/app/domain/auth/service.pybackend/app/infrastructure/external/jwt_verifier.pybackend/tests/test_auth.py
💤 Files with no reviewable changes (1)
- backend/app/domain/auth/service.py
Owner
Author
Jules-CodeRabbit Bridge (Attempt 1/3)Verify each finding against current code. Fix only still-valid issues, skip the Inline comments:
In
In
In
In
Project context (follow these exactly):
|
Owner
Author
Jules-CodeRabbit Bridge (Attempt 2/3)Verify each finding against current code. Fix only still-valid issues, skip the Inline comments:
In
In
In
In
Project context (follow these exactly):
|
Contributor
I have implemented the requested changes:
|
Contributor
I have completed all the requested fixes as mentioned in my previous reply. The changes are fully tested and committed to the branch. |
- Add Google-style Returns docstring to get_jwt_verifier - Inject JWTVerifierProtocol into websocket_chat_hub using Depends - Update JWTVerifierProtocol and SupabaseJWTVerifier with full Google-style docstrings - Move blocking PyJWKClient lookup to asyncio.to_thread - Narrow token validation exception handling to jwt.PyJWTError - Add test coverage for RS256/JWKS execution path in tests/test_auth.py Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Provides the injected `jwt_verifier` parameter to `websocket_chat_hub` calls in unit tests to match the updated dependency injection signature. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
This branch had an error being deployed
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
2 participants
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.
Extracted
SupabaseJWTVerifierimplementation out of theAuthServiceto cleanly encapsulate infrastructure concerns and resolve architectural layering violations.PR created automatically by Jules for task 4476076807082277045 started by @YKDBontekoe
Summary by CodeRabbit
New Features
Bug Fixes
Tests