Add support for JWT Token authentication with JWK #243
Conversation
|
""" WalkthroughThe changes introduce a new authentication module supporting JWT validation using JSON Web Keys (JWK). This includes new configuration models, constants, dependency classes, and comprehensive unit tests. The authentication configuration is extended to support JWK, and the main authentication dependency selector is updated to handle the new module. Required dependencies are added to the project. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FastAPI
participant JwkTokenAuthDependency
participant JWK_Server
Client->>FastAPI: HTTP request with Authorization: Bearer <JWT>
FastAPI->>JwkTokenAuthDependency: Call dependency
JwkTokenAuthDependency->>JWK_Server: (If cache miss) Fetch JWK set
JWK_Server-->>JwkTokenAuthDependency: Return JWK set
JwkTokenAuthDependency->>JwkTokenAuthDependency: Validate and decode JWT
JwkTokenAuthDependency-->>FastAPI: Return (user_id, username, token)
FastAPI-->>Client: Processed response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
""" Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
omertuc
force-pushed
the
jwk
branch
3 times, most recently
from
e6f573b to
14e3db7
Compare
umago
left a comment
umago
left a comment
There was a problem hiding this comment.
Thanks for the patch, comments inline
tisnik
left a comment
tisnik
left a comment
There was a problem hiding this comment.
in overall it looks ok, thank you.
pls. look at comments made by others
omertuc
force-pushed
the
jwk
branch
2 times, most recently
from
d25d34b to
ee23335
Compare
|
Resolved most comments, still didn't write tests and still testing a bit manually |
|
Tests are passing in isolation but failing when ran all together, some side effect thing. Need to debug |
omertuc
force-pushed
the
jwk
branch
3 times, most recently
from
08d8ac1 to
969c597
Compare
omertuc
changed the title
|
OK ready for review now |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/models/config.py (1)
140-143: Remove the empty validator or add validation logic.The
check_jwt_configurationvalidator doesn't perform any validation. Either remove it entirely or add actual validation logic if needed.- @model_validator(mode="after") - def check_jwt_configuration(self) -> Self: - """Validate JWT configuration.""" - return selfsrc/auth/jwk_token.py (1)
27-32: Consider adding cache invalidation mechanism for key rotation scenarios.The 1-hour TTL cache is reasonable, but consider adding a mechanism to invalidate the cache if authentication failures suggest key rotation. This would help handle scenarios where keys are rotated more frequently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
pyproject.toml(2 hunks)src/auth/__init__.py(3 hunks)src/auth/jwk_token.py(1 hunks)src/configuration.py(1 hunks)src/constants.py(1 hunks)src/models/config.py(2 hunks)tests/unit/auth/test_jwk_token.py(1 hunks)tests/unit/models/test_config.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/configuration.py (1)
src/models/config.py (1)
AuthenticationConfiguration(160-195)
src/auth/__init__.py (3)
src/configuration.py (2)
configuration(47-52)authentication_configuration(87-97)src/auth/jwk_token.py (1)
JwkTokenAuthDependency(50-149)src/models/config.py (1)
jwk_configuration(188-195)
src/auth/jwk_token.py (3)
src/auth/interface.py (1)
AuthInterface(8-13)src/auth/utils.py (1)
extract_user_token(7-26)src/models/config.py (1)
JwkConfiguration(146-157)
🔇 Additional comments (18)
pyproject.toml (2)
56-56: LGTM: Development dependency addition is appropriate.The
types-cachetoolsdependency provides proper type annotations for caching utilities used in the JWK implementation.
16-17: Dependencies verified: current stable, no known critical vulnerabilitiesBoth
aiohttp>=3.12.14andauthlib>=1.6.0are already at their latest stable releases and have no outstanding security issues in mainstream PyPI distributions:
- aiohttp 3.12.14: Latest stable as of June 4, 2025; no active vulnerabilities flagged in public databases. (CVE-2024-52304 applies only to a specific IBM product context, not the general PyPI package.)
- authlib 1.6.0: Latest stable as of May 22, 2025; no known security issues in this release.
Recommendations:
- Keep both packages up to date to receive security and bug-fix releases.
- Monitor official changelogs and vulnerability databases (e.g., CVE Details, Snyk) for future disclosures.
- If deploying in specialized environments (e.g., IBM watsonx), verify applicability of enterprise-specific advisories.
No changes required.
tests/unit/models/test_config.py (3)
439-439: LGTM: Consistent test updates for new configuration field.The addition of
"jwk_config": Noneto the expected authentication configuration correctly reflects the new field in theAuthenticationConfigurationmodel.
523-523: LGTM: Consistent test updates maintained.The test correctly includes the new
jwk_configfield in the expected serialization output.
625-625: LGTM: All configuration dump tests updated consistently.The test updates are consistent across all configuration dump scenarios, ensuring the new JWK configuration field is properly tested.
src/auth/__init__.py (3)
6-6: LGTM: Import addition follows established pattern.The import of the new
jwk_tokenmodule is correctly added alongside other authentication modules.
18-18: LGTM: Type annotation reflects configuration guarantee.The removal of
Optionalfrom the return type is consistent with the configuration changes that ensure authentication configuration is always present.
35-39: LGTM: JWK token authentication integration is well-implemented.The new case for
AUTH_MOD_JWK_TOKENfollows the established pattern and correctly passes the JWK configuration to the dependency constructor.src/constants.py (3)
36-36: LGTM: Authentication module constant follows naming convention.The
AUTH_MOD_JWK_TOKENconstant follows the established naming pattern for authentication modules.
43-43: LGTM: Module properly added to supported authentication modules.The new authentication module is correctly included in the
SUPPORTED_AUTHENTICATION_MODULESfrozenset.
47-48: LGTM: JWT claim constants are well-defined.The default JWT claim constants
DEFAULT_JWT_UID_CLAIMandDEFAULT_JWT_USER_NAME_CLAIMuse reasonable field names that align with common JWT implementations like Red Hat SSO.src/configuration.py (2)
87-87: LGTM: Type annotation improvement enhances code safety.Removing
Optionalfrom the return type correctly reflects that authentication configuration is always required and present.
93-95: LGTM: Additional assertion provides runtime validation.The assertion ensures that the authentication configuration is properly loaded before returning it, providing clear error messaging for configuration issues.
src/models/config.py (1)
167-196: LGTM!The authentication configuration changes properly integrate JWK support with appropriate validation and access patterns.
src/auth/jwk_token.py (1)
60-149: LGTM!The authentication flow is well-implemented with comprehensive error handling for various JWT validation scenarios. The HTTP status codes are appropriate for each error type.
tests/unit/auth/test_jwk_token.py (3)
233-238: Fix expected status code.Based on the implementation in
jwk_token.py, when there's no Authorization header,extract_user_tokenraises HTTPException with status 400. The test expectation is correct.
252-257: Fix expected status code.Based on the implementation, when the Authorization header doesn't contain a valid Bearer token,
extract_user_tokenraises HTTPException with status 400. The test expectation is correct.
145-386: LGTM!The test suite provides comprehensive coverage of the JWT authentication functionality, including edge cases and error scenarios. The fixtures and mocking are well-designed.
| async def get_jwk_set(url: str) -> KeySet: | ||
| """Fetch the JWK set from the cache, or fetch it from the URL if not cached.""" | ||
| async with _jwk_cache_lock: | ||
| if url not in _jwk_cache: | ||
| async with aiohttp.ClientSession() as session: | ||
| async with session.get(url) as resp: | ||
| resp.raise_for_status() | ||
| _jwk_cache[url] = JsonWebKey.import_key_set(await resp.json()) | ||
| return _jwk_cache[url] | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout and better error handling for HTTP requests.
The function should handle network timeouts and provide more specific error messages for different failure scenarios.
async def get_jwk_set(url: str) -> KeySet:
"""Fetch the JWK set from the cache, or fetch it from the URL if not cached."""
async with _jwk_cache_lock:
if url not in _jwk_cache:
- async with aiohttp.ClientSession() as session:
- async with session.get(url) as resp:
- resp.raise_for_status()
- _jwk_cache[url] = JsonWebKey.import_key_set(await resp.json())
+ timeout = aiohttp.ClientTimeout(total=10)
+ async with aiohttp.ClientSession(timeout=timeout) as session:
+ try:
+ async with session.get(url) as resp:
+ resp.raise_for_status()
+ jwk_data = await resp.json()
+ _jwk_cache[url] = JsonWebKey.import_key_set(jwk_data)
+ except aiohttp.ClientError as e:
+ logger.error("Failed to fetch JWK set from %s: %s", url, e)
+ raise
+ except ValueError as e:
+ logger.error("Invalid JSON response from JWK endpoint: %s", e)
+ raise
return _jwk_cache[url]📝 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.
| async def get_jwk_set(url: str) -> KeySet: | |
| """Fetch the JWK set from the cache, or fetch it from the URL if not cached.""" | |
| async with _jwk_cache_lock: | |
| if url not in _jwk_cache: | |
| async with aiohttp.ClientSession() as session: | |
| async with session.get(url) as resp: | |
| resp.raise_for_status() | |
| _jwk_cache[url] = JsonWebKey.import_key_set(await resp.json()) | |
| return _jwk_cache[url] | |
| async def get_jwk_set(url: str) -> KeySet: | |
| """Fetch the JWK set from the cache, or fetch it from the URL if not cached.""" | |
| async with _jwk_cache_lock: | |
| if url not in _jwk_cache: | |
| timeout = aiohttp.ClientTimeout(total=10) | |
| async with aiohttp.ClientSession(timeout=timeout) as session: | |
| try: | |
| async with session.get(url) as resp: | |
| resp.raise_for_status() | |
| jwk_data = await resp.json() | |
| _jwk_cache[url] = JsonWebKey.import_key_set(jwk_data) | |
| except aiohttp.ClientError as e: | |
| logger.error("Failed to fetch JWK set from %s: %s", url, e) | |
| raise | |
| except ValueError as e: | |
| logger.error("Invalid JSON response from JWK endpoint: %s", e) | |
| raise | |
| return _jwk_cache[url] |
🤖 Prompt for AI Agents
In src/auth/jwk_token.py around lines 35 to 44, the get_jwk_set function lacks
timeout settings and detailed error handling for the HTTP request. Add a timeout
parameter to the aiohttp.ClientSession or the session.get call to prevent
hanging requests. Wrap the HTTP request in a try-except block to catch
exceptions like asyncio.TimeoutError and aiohttp.ClientError, and raise or log
more specific error messages indicating the nature of the failure. This will
improve reliability and debuggability of the function.
|
Will handle coderabbit comments and add a few more unittests |
|
OK finally have time to get back to this. Still a few unit tests refuse to pass, looking into it |
thank you! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit/auth/test_jwk_token.py (2)
318-318: Fix incorrect comment.The comment incorrectly references "user_id claim" when it should reference "username claim" for this fixture.
- """Token without a user_id claim.""" + """Token without a username claim."""
419-419: Fix incorrect comment.The comment incorrectly references "RS256" when it should reference "RS384" for this fixture.
- """RS256 no kid.""" + """RS384 no kid."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
pyproject.toml(2 hunks)src/auth/__init__.py(3 hunks)src/auth/jwk_token.py(1 hunks)src/configuration.py(1 hunks)src/constants.py(1 hunks)src/models/config.py(2 hunks)tests/unit/auth/test_jwk_token.py(1 hunks)tests/unit/models/test_config.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- pyproject.toml
- src/configuration.py
- src/auth/init.py
- tests/unit/models/test_config.py
- src/auth/jwk_token.py
- src/constants.py
- src/models/config.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-pr
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/auth/jwk_token.py (2)
35-44: Add timeout and better error handling for HTTP requests.This function needs timeout configuration and more specific error handling for different failure scenarios to prevent hanging requests and provide better debugging information.
91-92: Add error handling for missing 'alg' field in JWK.The code assumes the key has an 'alg' field, which could raise a KeyError if missing from the JWK.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
pyproject.toml(2 hunks)src/auth/__init__.py(3 hunks)src/auth/jwk_token.py(1 hunks)src/configuration.py(1 hunks)src/constants.py(1 hunks)src/models/config.py(2 hunks)tests/unit/auth/test_jwk_token.py(1 hunks)tests/unit/models/test_config.py(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/unit/models/test_config.py
- src/constants.py
🚧 Files skipped from review as they are similar to previous changes (4)
- pyproject.toml
- src/configuration.py
- src/auth/init.py
- src/models/config.py
🔇 Additional comments (12)
src/auth/jwk_token.py (6)
46-48: LGTM!Clean custom exception class with appropriate docstring and inheritance.
53-58: LGTM!Constructor correctly initializes the configuration and virtual path with appropriate defaults.
60-64: LGTM!Proper token extraction and JWK set retrieval following the established patterns.
96-123: LGTM!Comprehensive error handling with appropriate HTTP status codes and informative error messages for different JWT decode failure scenarios.
124-140: LGTM!Proper token validation with appropriate error handling for expired tokens and other validation failures.
141-160: LGTM!Proper claim extraction with configurable claim names, appropriate error handling for missing claims, and good logging of successful authentications.
tests/unit/auth/test_jwk_token.py (6)
3-3: LGTM!Module docstring correctly references the jwk_token module being tested.
19-127: LGTM!Comprehensive and well-organized test fixtures providing proper test isolation, key generation, token creation, and mock server setup for different test scenarios.
130-180: LGTM!Well-designed helper functions that promote code reuse and provide clean abstractions for creating test requests and verifying results.
182-284: LGTM!Comprehensive basic authentication tests covering valid tokens, expired tokens, invalid signatures, and missing/malformed authorization headers with appropriate status code assertions.
285-391: LGTM!Thorough testing of claim extraction including missing claims, custom claim names, and proper error handling with appropriate status codes and error messages.
393-538: LGTM!Excellent multi-key testing including DoS prevention for tokens without kid headers, proper algorithm support, and comprehensive error handling for multi-key scenarios.
omertuc
force-pushed
the
jwk
branch
2 times, most recently
from
345a87a to
e4caac9
Compare
umago
left a comment
umago
left a comment
There was a problem hiding this comment.
Overall solid patch. Two nits inline regarding validation of the config models but, it's no harm. Can be fixed in case you need to change something else in the patch
|
Improved key selection algorithm and fixed review comments |
omertuc
force-pushed
the
jwk
branch
3 times, most recently
from
c733537 to
0d8552d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/auth/jwk_token.py (1)
35-44: Address the TODO comment for error handling and timeouts.The TODO comment indicates that connection errors and timeouts need to be handled. This is critical for production reliability.
As mentioned in previous reviews, consider implementing:
async def get_jwk_set(url: str) -> KeySet: """Fetch the JWK set from the cache, or fetch it from the URL if not cached.""" async with _jwk_cache_lock: if url not in _jwk_cache: - async with aiohttp.ClientSession() as session: - # TODO(omertuc): handle connection errors, timeouts, etc. - async with session.get(url) as resp: - resp.raise_for_status() - _jwk_cache[url] = JsonWebKey.import_key_set(await resp.json()) + timeout = aiohttp.ClientTimeout(total=10) + try: + async with aiohttp.ClientSession(timeout=timeout) as session: + async with session.get(url) as resp: + resp.raise_for_status() + jwk_data = await resp.json() + _jwk_cache[url] = JsonWebKey.import_key_set(jwk_data) + except (aiohttp.ClientError, asyncio.TimeoutError) as e: + logger.error("Failed to fetch JWK set from %s: %s", url, e) + raise HTTPException( + status_code=status.HTTP_503_SERVICE_UNAVAILABLE, + detail="Unable to fetch JWK set" + ) from e return _jwk_cache[url]
🧹 Nitpick comments (1)
src/auth/jwk_token.py (1)
27-32: Consider increasing cache size for better scalability.The TTL cache setup is solid with appropriate 1-hour expiration. However,
maxsize=3might be limiting if the application needs to support multiple JWK endpoints or environments.Consider increasing the cache size to handle more diverse deployment scenarios:
-_jwk_cache: TTLCache[str, KeySet] = TTLCache(maxsize=3, ttl=3600) +_jwk_cache: TTLCache[str, KeySet] = TTLCache(maxsize=10, ttl=3600)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
pyproject.toml(2 hunks)src/auth/__init__.py(3 hunks)src/auth/jwk_token.py(1 hunks)src/configuration.py(1 hunks)src/constants.py(1 hunks)src/models/config.py(2 hunks)tests/unit/auth/test_jwk_token.py(1 hunks)tests/unit/models/test_config.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/unit/models/test_config.py
🚧 Files skipped from review as they are similar to previous changes (5)
- src/constants.py
- src/auth/init.py
- pyproject.toml
- src/configuration.py
- src/models/config.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/auth/jwk_token.py (3)
src/auth/interface.py (1)
AuthInterface(8-13)src/auth/utils.py (1)
extract_user_token(7-26)src/models/config.py (1)
JwkConfiguration(157-161)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-pr
🔇 Additional comments (11)
src/auth/jwk_token.py (4)
1-24: LGTM! Module docstring and imports are correct.The docstring accurately describes the JWT JWK authentication functionality, and all necessary imports are present for the implementation.
47-48: LGTM! Exception is properly defined.The custom exception is appropriate for handling key resolution failures in the JWK context.
51-109: Excellent key resolution implementation with comprehensive error handling.The key resolver function properly addresses previous review concerns:
- ✅ Validates presence of 'alg' field in token header
- ✅ Handles kid-based key lookup with algorithm validation
- ✅ Provides fallback for tokens without kid
- ✅ Includes DoS protection by limiting key attempts
- ✅ Provides descriptive error messages for debugging
The logic correctly handles edge cases and provides robust key resolution.
112-192: Robust JWT authentication implementation with comprehensive error handling.The authentication dependency correctly implements the AuthInterface and provides thorough error handling:
- ✅ Proper HTTP status codes for different error scenarios
- ✅ Comprehensive exception handling for JWT decode and validation errors
- ✅ Configurable claim extraction for user ID and username
- ✅ Appropriate logging for successful authentication
- ✅ Returns expected tuple format (user_id, username, token)
The implementation addresses previous review concerns about error handling and HTTP status codes.
tests/unit/auth/test_jwk_token.py (7)
1-17: LGTM! Module docstring and imports are correct.The docstring correctly references the module being tested, and all necessary imports are present for comprehensive JWT testing.
15-73: Well-structured test setup with proper isolation.The test fixtures and utilities are well-designed:
- ✅ Clear test constants for consistent data
- ✅ Reusable fixtures for tokens and keys
- ✅ Proper RSA key generation for testing
- ✅ Cache clearing fixture ensures test isolation
- ✅ Good separation of concerns in fixture design
The
autouse=Trueon the cache clearing fixture is essential for preventing test interference.
75-151: Comprehensive mock server setup for JWK endpoint simulation.The mock infrastructure properly handles the complexity of testing async HTTP interactions:
- ✅ Correctly mocks aiohttp ClientSession and response context managers
- ✅ Properly converts private keys to public JWK format for server responses
- ✅ Flexible server configuration for different key sets and algorithms
- ✅ Helper utilities for request generation and header manipulation
This enables thorough testing of the JWK fetching and caching behavior.
182-283: Comprehensive coverage of basic authentication scenarios.The test cases thoroughly validate core authentication functionality:
- ✅ Valid token authentication with correct claim extraction
- ✅ Expired token handling with appropriate HTTP 401 response
- ✅ Invalid signature detection and rejection
- ✅ Missing Authorization header validation (HTTP 400)
- ✅ Malformed Authorization header validation (HTTP 400)
- ✅ Proper error message verification for debugging
Each test appropriately verifies both status codes and error messages.
285-345: Proper validation of required JWT claims.The claim validation tests ensure robust handling of missing required claims:
- ✅ Missing user_id claim properly rejected with HTTP 401
- ✅ Missing username claim properly rejected with HTTP 401
- ✅ Error messages include specific claim names for debugging
- ✅ Consistent error handling pattern across claim types
This validates the claim extraction and validation logic.
347-391: Excellent validation of configurable claim names.The custom claims test ensures flexibility in JWT claim configuration:
- ✅ Supports non-standard claim names for user ID and username
- ✅ Proper configuration override mechanism
- ✅ Validates successful extraction of custom claims
- ✅ Maintains consistent authentication flow with custom configuration
This flexibility is crucial for integrating with diverse identity providers.
393-541: Comprehensive multi-key testing with important security validations.The multi-key tests validate critical security and functionality aspects:
- ✅ Multiple keys with different algorithms (RS256, RS384)
- ✅ Proper kid-based key selection for all keys
- ✅ DoS protection: tokens without kid only try first matching algorithm key
- ✅ Fallback behavior when only one key exists for an algorithm
- ✅ Proper rejection of tokens that can't be verified with first key
The test design correctly validates the security consideration of preventing brute-force key attempts while maintaining functionality for legitimate use cases.
This commit introduces a new authentication module `jwk-token` that
allows the application to authenticate users using JSON Web Tokens (JWT)
signed with JSON Web Keys (JWK). The new module fetches JWKs from a
specified URL and validates incoming JWTs against these keys.
Example config:
```yaml
authentication:
module: jwk-token
jwk_config:
url: https://sso.redhat.com/auth/realms/redhat-external/protocol/openid-connect/certs
jwt_configuration:
user_id_claim: user_id
username_claim: username
```
|
Force push for rebase conflicts |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/auth/jwk_token.py (3)
35-44: Add timeout and comprehensive error handling for HTTP requests.The function lacks timeout configuration and specific error handling for network failures, which was previously flagged. The TODO comment acknowledges this limitation.
88-94: Add error handling for missing 'alg' field in JWK keys.The code still assumes JWK keys have an 'alg' field (line 90), which could raise a KeyError if missing. This was flagged in previous reviews but the issue persists.
key = keys[0] - if key["alg"] != header["alg"]: + if "alg" not in key: + raise KeyNotFoundError( + f"Key with kid '{header.get('kid')}' missing 'alg' field" + ) + + if key["alg"] != header["alg"]: raise KeyNotFoundError( "Key found by kid does not match the algorithm in the token header" )
97-107: Add error handling for missing 'alg' field in algorithm-only key selection.Similar to the kid-based selection, this code assumes keys have an 'alg' field (line 98), which could raise a KeyError.
# No kid in the token header, we will try to find a key by alg - keys = [key for key in jwk_set.keys if key["alg"] == header["alg"]] + keys = [key for key in jwk_set.keys if key.get("alg") == header["alg"]]
🧹 Nitpick comments (1)
src/auth/jwk_token.py (1)
25-32: Consider increasing the JWK cache size.The cache maxsize of 3 may be too restrictive if the application needs to authenticate against multiple JWK endpoints. Consider increasing this value based on expected usage patterns.
The design choice to use a simple Lock instead of RWLock is reasonable to avoid additional dependencies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
pyproject.toml(2 hunks)src/auth/__init__.py(3 hunks)src/auth/jwk_token.py(1 hunks)src/configuration.py(1 hunks)src/constants.py(1 hunks)src/models/config.py(2 hunks)tests/unit/auth/test_jwk_token.py(1 hunks)tests/unit/models/test_config.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- pyproject.toml
- tests/unit/models/test_config.py
- src/configuration.py
- src/auth/init.py
- src/constants.py
- src/models/config.py
- tests/unit/auth/test_jwk_token.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/auth/jwk_token.py (3)
src/auth/interface.py (1)
AuthInterface(8-13)src/auth/utils.py (1)
extract_user_token(7-26)src/models/config.py (1)
JwkConfiguration(157-161)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-pr
🔇 Additional comments (3)
src/auth/jwk_token.py (3)
1-24: LGTM: Module setup and imports are well-structured.The corrected module docstring accurately describes the JWT JWK authentication functionality, and all imports appear necessary and properly organized.
47-48: LGTM: Clear and purposeful custom exception.The
KeyNotFoundErrorexception is well-defined with a clear docstring explaining its specific use case.
112-191: LGTM: Comprehensive JWT authentication implementation.The
JwkTokenAuthDependencyclass properly implements theAuthInterfacewith robust error handling:
- Appropriate HTTP status codes for different error scenarios
- Comprehensive exception handling for JWT decoding and validation
- Clear error messages for missing claims
- Proper logging of successful authentication
- Correct return type matching the interface
The implementation follows security best practices and handles edge cases appropriately.
Update lightspeed-stack to the latest version which includes this PR: lightspeed-core/lightspeed-stack#243 Modify our configuration to use the new jwk-token authentication module, with the JWK URL pointing to the Red Hat SSO server and using the user ID / username fields that can be found in a typical JWT issued by Red Hat SSO.
Update lightspeed-stack to the latest version which includes this PR: lightspeed-core/lightspeed-stack#243 Modify our configuration to use the new jwk-token authentication module, with the JWK URL pointing to the Red Hat SSO server and using the user ID / username fields that can be found in a typical JWT issued by Red Hat SSO.
Update lightspeed-stack to the latest version which includes this PR: lightspeed-core/lightspeed-stack#243 Modify our configuration to use the new jwk-token authentication module, with the JWK URL pointing to the Red Hat SSO server and using the user ID / username fields that can be found in a typical JWT issued by Red Hat SSO.
3 participants
Description
Add support for JWT Token authentication with JWK
This commit introduces a new authentication module
jwk-tokenthat allows the application to authenticate users using JSON Web Tokens (JWT) signed with JSON Web Keys (JWK). The new module fetches JWKs from a specified URL and validates incoming JWTs against these keys.Example config:
Type of change
Related Tickets & Documents
MGMT-21125
Checklist before requesting a review
Testing
Just ran it
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores