fix: ensure jwt is not in deny list before further authentication #57
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 jti
JwtUtil->>TokenDenylist: isTokenDenylisted(jti)
TokenDenylist->>Redis: get(denied_jti)
Redis-->>TokenDenylist: return value
TokenDenylist-->>JwtUtil: true/false
alt Token is denylisted
JwtUtil-->>Client: return null
else Token is valid
JwtUtil-->>Client: return Claims
end
sequenceDiagram
participant Client
participant JwtUtil
participant TokenDenylist
participant Redis
Client->>JwtUtil: invalidateToken(token)
JwtUtil->>JwtUtil: parse token, extract jti, expiration
JwtUtil->>TokenDenylist: addTokenToDenylist(jti, expirationTime)
TokenDenylist->>Redis: set(denied_jti, true, expirationTime)
Redis-->>TokenDenylist: OK
TokenDenylist-->>JwtUtil: (void)
JwtUtil-->>Client: (void)
✨ 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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/iemr/tm/utils/JwtUtil.java(2 hunks)src/main/java/com/iemr/tm/utils/TokenDenylist.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/iemr/tm/utils/TokenDenylist.java (1)
src/main/java/com/iemr/tm/utils/JwtUtil.java (1)
Component(15-87)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (2)
src/main/java/com/iemr/tm/utils/TokenDenylist.java (1)
27-38: Good resilience pattern for Redis failuresThe method gracefully handles Redis failures by returning
false, which prevents service disruption. This fail-open approach prioritizes availability over strict token denial enforcement.src/main/java/com/iemr/tm/utils/JwtUtil.java (1)
35-48: LGTM! Proper integration with denylistThe token validation correctly checks the denylist after successful parsing, with appropriate null checks for the JTI.
| public void addTokenToDenylist(String jti, Long expirationTime) { | ||
| if (jti != null && expirationTime != null && expirationTime > 0) { | ||
| try { | ||
| String key = PREFIX + jti; | ||
| redisTemplate.opsForValue().set(key, true); | ||
| redisTemplate.expire(key, expirationTime, TimeUnit.MILLISECONDS); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException("Failed to add token to denylist", e); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Use atomic operation for setting key with expiration
The current implementation sets the value and expiration in two separate operations. If the expire call fails, the key will remain in Redis indefinitely without expiration.
Apply this diff to use an atomic operation:
public void addTokenToDenylist(String jti, Long expirationTime) {
if (jti != null && expirationTime != null && expirationTime > 0) {
try {
String key = PREFIX + jti;
- redisTemplate.opsForValue().set(key, true);
- redisTemplate.expire(key, expirationTime, TimeUnit.MILLISECONDS);
+ redisTemplate.opsForValue().set(key, true, expirationTime, TimeUnit.MILLISECONDS);
} catch (Exception e) {
throw new RuntimeException("Failed to add token to denylist", e);
}
}
}📝 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.
| public void addTokenToDenylist(String jti, Long expirationTime) { | |
| if (jti != null && expirationTime != null && expirationTime > 0) { | |
| try { | |
| String key = PREFIX + jti; | |
| redisTemplate.opsForValue().set(key, true); | |
| redisTemplate.expire(key, expirationTime, TimeUnit.MILLISECONDS); | |
| } catch (Exception e) { | |
| throw new RuntimeException("Failed to add token to denylist", e); | |
| } | |
| } | |
| } | |
| public void addTokenToDenylist(String jti, Long expirationTime) { | |
| if (jti != null && expirationTime != null && expirationTime > 0) { | |
| try { | |
| String key = PREFIX + jti; | |
| redisTemplate.opsForValue().set(key, true, expirationTime, TimeUnit.MILLISECONDS); | |
| } catch (Exception e) { | |
| throw new RuntimeException("Failed to add token to denylist", e); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/tm/utils/TokenDenylist.java between lines 15 and 25,
the method addTokenToDenylist sets the Redis key and expiration in two separate
calls, risking keys without expiration if expire fails. Fix this by using a
single atomic Redis operation that sets the key with its expiration time in one
call, such as redisTemplate.opsForValue().set(key, true, expirationTime,
TimeUnit.MILLISECONDS), to ensure the key always has the correct TTL.
| } catch (Exception e) { | ||
| // Log error but don't throw - token might be invalid already | ||
| throw new RuntimeException("Failed to invalidate token", e); | ||
| } |
There was a problem hiding this comment.
Fix inconsistency between comment and implementation
The comment states "Log error but don't throw" but the code throws a RuntimeException.
Either update the comment or change the implementation:
} catch (Exception e) {
- // Log error but don't throw - token might be invalid already
+ // Propagate error - token invalidation should be reliable
throw new RuntimeException("Failed to invalidate token", e);
}Or if you want to follow the comment's intent:
} catch (Exception e) {
- // Log error but don't throw - token might be invalid already
- throw new RuntimeException("Failed to invalidate token", e);
+ // Log error but don't throw - token might be invalid already
+ // logger.error("Failed to invalidate token", e);
}📝 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.
| } catch (Exception e) { | |
| // Log error but don't throw - token might be invalid already | |
| throw new RuntimeException("Failed to invalidate token", e); | |
| } | |
| } catch (Exception e) { | |
| // Propagate error - token invalidation should be reliable | |
| throw new RuntimeException("Failed to invalidate token", e); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/tm/utils/JwtUtil.java around lines 65 to 68, the
comment says to log the error without throwing, but the code throws a
RuntimeException. To fix this inconsistency, either update the comment to
reflect that an exception is thrown or modify the catch block to log the error
without throwing any exception, following the comment's intent.
| 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.
🛠️ Refactor suggestion
Consider adding exception handling for invalid tokens
The extractAllClaims method will throw exceptions for invalid tokens. While validateToken handles this properly, invalidateToken expects this method to return null for invalid tokens based on the null check at line 58.
Consider wrapping in try-catch to align with the usage pattern:
private Claims extractAllClaims(String token) {
+ try {
return Jwts.parser()
.verifyWith(getSigningKey())
.build()
.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(); | |
| 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; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/tm/utils/JwtUtil.java around lines 80 to 86, the
extractAllClaims method currently throws exceptions for invalid tokens, but
invalidateToken expects it to return null on invalid tokens. To fix this, wrap
the method's logic in a try-catch block that catches relevant exceptions (e.g.,
JwtException) and returns null when an exception occurs, ensuring consistency
with the null check usage.
📋 Description
JIRA ID: AMM-1507
Check if jti is present in deny list before authenticating.
✅ Type of Change
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores