fix: ensure jwt is not in deny list before further authentication #87
Conversation
WalkthroughThe JwtUtil class was refactored to remove JWT token generation and to update token parsing and validation using a new JWT API. Token validation now integrates a denylist check via a newly introduced TokenDenylist component, which manages denylisted JWT IDs using Redis. Method signatures and internal logic were updated for null safety and modern API usage. 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: return result
TokenDenylist-->>JwtUtil: denylist status
JwtUtil-->>Client: return claims or null
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/iemr/mmu/utils/JwtUtil.java(3 hunks)src/main/java/com/iemr/mmu/utils/TokenDenylist.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/iemr/mmu/utils/TokenDenylist.java (1)
src/main/java/com/iemr/mmu/utils/JwtUtil.java (1)
Component(14-69)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (4)
src/main/java/com/iemr/mmu/utils/TokenDenylist.java (2)
1-22: Well-structured class setup with proper dependency injection.The class is properly configured as a Spring component with appropriate logger initialization and Redis template autowiring. The key prefix approach is a good practice for namespacing Redis keys.
41-54: Excellent defensive programming with graceful error handling.The method properly handles Redis failures by returning
falseto avoid blocking requests, which is a good resilience pattern. The null-safe comparison usingBoolean.TRUE.equals()is also a best practice.src/main/java/com/iemr/mmu/utils/JwtUtil.java (2)
20-29: Good integration of TokenDenylist and improved type safety.The autowiring of
TokenDenylistis properly implemented, and the change to returnSecretKeyinstead of genericKeyimproves type safety.
31-51: Well-implemented denylist integration with proper null safety.The token validation correctly checks the denylist after successful parsing, with appropriate null checking for the JWT ID. The integration aligns perfectly with the PR objective of preventing denylisted tokens from being authenticated.
| // Add a token's jti to the denylist with expiration time | ||
| public void addTokenToDenylist(String jti, Long expirationTime) { | ||
| if (jti == null || jti.trim().isEmpty()) { | ||
| 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); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException("Failed to denylist token", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consistent error handling strategy across methods.
The method throws a RuntimeException on Redis failure, while isTokenDenylisted gracefully handles failures by returning false. This inconsistency could lead to service disruption if Redis is temporarily unavailable.
Consider adopting a uniform approach:
- Either fail gracefully in both methods (recommended for resilience)
- Or throw exceptions consistently in both methods
Apply this diff to handle Redis failures gracefully:
public void addTokenToDenylist(String jti, Long expirationTime) {
if (jti == null || jti.trim().isEmpty()) {
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);
} catch (Exception e) {
- throw new RuntimeException("Failed to denylist token", e);
+ logger.error("Failed to add token to denylist for jti: " + jti, e);
+ // Optionally, you could track this failure for monitoring
}
}Also, consider using a more meaningful value instead of a space character:
- redisTemplate.opsForValue().set(key, " ", expirationTime, TimeUnit.MILLISECONDS);
+ redisTemplate.opsForValue().set(key, "true", expirationTime, TimeUnit.MILLISECONDS);🤖 Prompt for AI Agents
In src/main/java/com/iemr/mmu/utils/TokenDenylist.java between lines 24 and 39,
the addTokenToDenylist method currently throws a RuntimeException on Redis
failure, while isTokenDenylisted handles failures gracefully by returning false.
To ensure consistency and improve resilience, modify addTokenToDenylist to catch
exceptions and handle them gracefully without throwing, similar to
isTokenDenylisted. Additionally, replace the placeholder value " " stored in
Redis with a more meaningful value such as "denied" to improve clarity.
| private Claims extractAllClaims(String token) { | ||
| return Jwts.parser().setSigningKey(getSigningKey()).build().parseClaimsJws(token).getBody(); | ||
| return Jwts.parser() | ||
| .verifyWith(getSigningKey()) | ||
| .build() | ||
| .parseSignedClaims(token) | ||
| .getPayload(); | ||
| } |
There was a problem hiding this comment.
Add exception handling for consistency with validateToken.
The extractAllClaims method doesn't handle exceptions, which could cause runtime errors when called from extractClaim or extractUsername. This is inconsistent with validateToken which catches all exceptions.
Apply this diff to handle exceptions consistently:
private Claims extractAllClaims(String token) {
+ try {
return Jwts.parser()
.verifyWith(getSigningKey())
.build()
.parseSignedClaims(token)
.getPayload();
+ } catch (Exception e) {
+ return null; // Consistent with validateToken behavior
+ }
}📝 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(); | |
| return Jwts.parser() | |
| .verifyWith(getSigningKey()) | |
| .build() | |
| .parseSignedClaims(token) | |
| .getPayload(); | |
| } | |
| private Claims extractAllClaims(String token) { | |
| try { | |
| return Jwts.parser() | |
| .verifyWith(getSigningKey()) | |
| .build() | |
| .parseSignedClaims(token) | |
| .getPayload(); | |
| } catch (Exception e) { | |
| return null; // Consistent with validateToken behavior | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/mmu/utils/JwtUtil.java between lines 62 and 68, the
extractAllClaims method lacks exception handling, which can cause runtime errors
and is inconsistent with the validateToken method. Modify extractAllClaims to
catch exceptions such as JwtException or general Exception, and handle them
appropriately (e.g., return null or throw a custom exception) to ensure
consistent error handling across methods that parse tokens.
📋 Description
JIRA ID: AMM-1507
Check if jti is present in deny list before authenticating.
✅ Type of Change
Summary by CodeRabbit