fix: ensure jwt is not in deny list before further authentication #135
Conversation
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant JwtUtil
participant TokenDenylist
participant Redis
Client->>JwtUtil: validateToken(token)
JwtUtil->>JwtUtil: Parse token, extract claims
JwtUtil->>TokenDenylist: isTokenDenylisted(jti)
TokenDenylist->>Redis: Check "denied_{jti}" key
Redis-->>TokenDenylist: Key presence/absence
TokenDenylist-->>JwtUtil: true/false
alt jti is denylisted
JwtUtil-->>Client: Return null (token invalid)
else jti not denylisted
JwtUtil-->>Client: Return claims (token valid)
end
Poem
✨ Finishing Touches
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 (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/main/java/com/iemr/hwc/utils/JwtUtil.java (1)
15-67: 💡 Verification agent🧩 Analysis chain
Verify token generation is handled elsewhere after removal.
The AI summary indicates that the token generation method was removed from this class. Please ensure that JWT token generation is properly handled elsewhere in the system.
🏁 Script executed:
#!/bin/bash # Description: Search for JWT token generation in the codebase to ensure it's handled after removal from JwtUtil # Search for JWT token generation patterns echo "=== Searching for JWT token generation ===" rg -A 5 "Jwts\.builder\(\)" --type java # Search for token signing patterns echo -e "\n=== Searching for JWT signing patterns ===" rg -A 5 "signWith|compact\(\)" --type java # Search for any remaining token generation methods echo -e "\n=== Searching for generateToken methods ===" ast-grep --pattern 'generateToken($_) { $$$ }'Length of output: 434
Token generation logic is missing in the codebase
Our automated search found no evidence of JWT issuance anywhere:
- No
Jwts.builder()usages- No
signWith(...)orcompact()calls- No
generateToken(...)methodsPlease ensure that JWT token creation is implemented in another module or restore the token‐generation functionality so clients can obtain valid tokens.
🧹 Nitpick comments (4)
src/main/java/com/iemr/hwc/utils/TokenDenylist.java (3)
24-39: Consider consistent error handling and clarify time unit in documentation.The method has good input validation, but there are a few suggestions:
- The error handling is inconsistent - null
jtireturns silently while invalidexpirationTimethrows an exception- The comment should specify that
expirationTimeis in milliseconds- Consider whether throwing
RuntimeExceptionis appropriate, as it might cause transaction rollbacksConsider this improved implementation:
- // Add a token's jti to the denylist with expiration time - public void addTokenToDenylist(String jti, Long expirationTime) { + // Add a token's jti to the denylist with expiration time in milliseconds + public void addTokenToDenylist(String jti, Long expirationTime) { if (jti == null || jti.trim().isEmpty()) { + logger.warn("Attempted to denylist token with null or empty jti"); return; } if (expirationTime == null || expirationTime <= 0) { throw new IllegalArgumentException("Expiration time must be positive"); } try { String key = getKey(jti); // Use helper method to get the key redisTemplate.opsForValue().set(key, " ", expirationTime, TimeUnit.MILLISECONDS); + logger.debug("Successfully denylisted token with jti: {}", jti); } catch (Exception e) { - throw new RuntimeException("Failed to denylist token", e); + logger.error("Failed to denylist token with jti: {}", jti, e); + throw new RuntimeException("Failed to denylist token", e); } }
50-50: Use parameterized logging instead of string concatenation.String concatenation in logging statements should be avoided for performance reasons.
- logger.error("Failed to check denylist status for jti: " + jti, e); + logger.error("Failed to check denylist status for jti: {}", jti, e);
41-54: Excellent resilience pattern for high availability.The fail-open approach in
isTokenDenylisted(returningfalseon Redis failures) is a great architectural decision that prevents Redis outages from blocking authentication. This ensures system availability while logging the issues for monitoring.src/main/java/com/iemr/hwc/utils/JwtUtil.java (1)
33-52: Well-implemented denylist integration with proper null handling.The integration with
TokenDenylistis clean and the null check onjtiprevents issues with tokens that don't have an ID. The migration to the new JWT parser API is also correct.Consider adding debug logging to help with troubleshooting:
} catch (Exception e) { + logger.debug("Token validation failed", e); return null; // Handle token parsing/validation errors }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/iemr/hwc/utils/JwtUtil.java(3 hunks)src/main/java/com/iemr/hwc/utils/TokenDenylist.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/main/java/com/iemr/hwc/utils/TokenDenylist.java (1)
src/main/java/com/iemr/hwc/utils/JwtUtil.java (1)
Component(15-67)
src/main/java/com/iemr/hwc/utils/JwtUtil.java (3)
src/main/java/com/iemr/hwc/utils/TokenDenylist.java (1)
Component(11-55)src/main/java/com/iemr/hwc/utils/JwtUserIdValidationFilter.java (1)
Component(18-129)src/main/java/com/iemr/hwc/utils/JwtAuthenticationUtil.java (1)
Component(21-126)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (3)
src/main/java/com/iemr/hwc/utils/TokenDenylist.java (1)
11-22: Good design with proper Redis key namespacing.The component structure follows Spring best practices, and using a prefix for Redis keys helps prevent collisions with other data in Redis.
src/main/java/com/iemr/hwc/utils/JwtUtil.java (2)
4-4: Good type safety improvement.Using
SecretKeyinstead of the genericKeyinterface provides better type safety and aligns with the JWT library's recommendations.Also applies to: 25-25
58-61: Good defensive programming with null check.Adding the null check prevents potential
NullPointerExceptionwhen token parsing fails.
| private Claims extractAllClaims(String token) { | ||
| return Jwts.parser().setSigningKey(getSigningKey()).build().parseClaimsJws(token).getBody(); | ||
| JwtParser parser = Jwts.parser().verifyWith(getSigningKey()).build(); | ||
| return parser.parseSignedClaims(token).getPayload(); | ||
| } |
There was a problem hiding this comment.
Add exception handling to prevent unhandled exceptions.
The extractAllClaims method can throw JWT parsing exceptions that aren't handled, which could propagate to calling methods.
private Claims extractAllClaims(String token) {
- JwtParser parser = Jwts.parser().verifyWith(getSigningKey()).build();
- return parser.parseSignedClaims(token).getPayload();
+ try {
+ JwtParser parser = Jwts.parser().verifyWith(getSigningKey()).build();
+ return parser.parseSignedClaims(token).getPayload();
+ } catch (Exception e) {
+ return null;
+ }
}📝 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.
| private Claims extractAllClaims(String token) { | |
| return Jwts.parser().setSigningKey(getSigningKey()).build().parseClaimsJws(token).getBody(); | |
| JwtParser parser = Jwts.parser().verifyWith(getSigningKey()).build(); | |
| return parser.parseSignedClaims(token).getPayload(); | |
| } | |
| private Claims extractAllClaims(String token) { | |
| + try { | |
| + JwtParser parser = Jwts.parser().verifyWith(getSigningKey()).build(); | |
| + return parser.parseSignedClaims(token).getPayload(); | |
| + } catch (Exception e) { | |
| + return null; | |
| + } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/hwc/utils/JwtUtil.java around lines 63 to 66, the
extractAllClaims method currently does not handle exceptions that may occur
during JWT parsing. To fix this, wrap the parsing logic in a try-catch block to
catch relevant JWT exceptions, such as JwtException or its subclasses, and
handle them appropriately, for example by logging the error and returning null
or throwing a custom exception. This will prevent unhandled exceptions from
propagating to calling methods.
📋 Description
JIRA ID: AMM-1507
Check if jti is present in deny list before authenticating.
✅ Type of Change
Summary by CodeRabbit