fix: ensure jwt is not in deny list before further authentication #86
Conversation
|
""" WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant JwtUtil
participant TokenDenylist
participant Redis
Client->>JwtUtil: validateToken(token)
JwtUtil->>JwtUtil: parse JWT, extract jti
JwtUtil->>TokenDenylist: isTokenDenylisted(jti)
TokenDenylist->>Redis: check "denied_{jti}" key
Redis-->>TokenDenylist: return exists/not exists
TokenDenylist-->>JwtUtil: return denylist status
alt jti is denylisted
JwtUtil-->>Client: return null
else jti is not denylisted
JwtUtil-->>Client: return claims
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ 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
🧹 Nitpick comments (1)
src/main/java/com/iemr/tm/utils/TokenDenylist.java (1)
35-35: Consider using a more meaningful Redis value.Using a space character
" "as the Redis value works but could be more explicit.Consider using a more descriptive value:
- redisTemplate.opsForValue().set(key, " ", expirationTime, TimeUnit.MILLISECONDS); + redisTemplate.opsForValue().set(key, "DENIED", expirationTime, TimeUnit.MILLISECONDS);
📜 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(4 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(16-71)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (9)
src/main/java/com/iemr/tm/utils/TokenDenylist.java (3)
11-18: LGTM! Well-structured component setup.The class is properly annotated as a Spring component with appropriate logger and Redis template injection. The constant prefix approach for Redis keys is a good practice for namespace isolation.
20-22: LGTM! Clean helper method.The key generation method properly encapsulates the prefix logic for Redis keys.
42-54: LGTM! Excellent fail-safe strategy.The method correctly handles Redis failures by returning false to avoid blocking valid requests. The error logging and defensive approach are well-implemented.
src/main/java/com/iemr/tm/utils/JwtUtil.java (6)
6-6: LGTM! Appropriate import for improved type safety.The import of
SecretKeyprovides better type safety for JWT operations.
22-23: LGTM! Clean dependency injection.The TokenDenylist component is properly injected for use in token validation.
26-26: LGTM! Improved type safety with SecretKey.Changing the return type to
SecretKeyprovides better type safety and aligns with JWT library best practices.
61-61: LGTM! Excellent defensive programming.The null check prevents NullPointerException when claims extraction fails, improving robustness.
65-69: LGTM! Consistent use of new signing key method.The updated JWT parsing correctly uses the new
getSigningKey()method for consistency.
34-53:Details
❓ Verification inconclusive
Verify JWT token structure requirements.
Ensure that all JWTs in your system include the
jticlaim, as the denylist functionality depends on it.Run the following script to check if existing JWT generation code includes
jticlaims:
🏁 Script executed:
#!/bin/bash # Description: Search for JWT generation code to verify jti claim usage # Expected: Find JWT creation code that includes jti claims # Search for JWT creation/generation patterns echo "Searching for JWT generation code..." rg -A 10 -B 5 "Jwts\.builder|jwt.*build|createToken|generateToken" # Search for jti claim usage echo -e "\nSearching for jti claim usage..." rg -A 5 -B 5 "setId\(|\.id\(|jti"Length of output: 13117
Verify inclusion of
jtiin your JWT issuance logicI couldn’t find any
Jwts.builder()orgenerateTokencalls in the repo, so it’s unclear whether your token‐creation code ever sets a JTI. SinceJwtUtil.validateToken(...)usesclaims.getId()to perform denylist checks, you must ensure every JWT your system issues includes a non-nulljti.Please review your token‐generation routine (e.g. in AuthService, SecurityConfig, or any external provider) and confirm that you’re doing something like:
String jti = UUID.randomUUID().toString(); String token = Jwts.builder() .setId(jti) // … other claims, signWith, etc. .compact();– If tokens come from an external service, verify they ship a
jti.
– If you already set the JTI elsewhere, point me to that code.
| public void removeTokenFromDenylist(String jti) { | ||
| if (jti != null && !jti.trim().isEmpty()) { | ||
| String key = getKey(jti); // Use helper method to get the key | ||
| redisTemplate.delete(key); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add exception handling for consistency.
The remove method lacks exception handling unlike other Redis operations in this class.
Apply this diff to add consistent exception handling:
public void removeTokenFromDenylist(String jti) {
if (jti != null && !jti.trim().isEmpty()) {
- String key = getKey(jti); // Use helper method to get the key
- redisTemplate.delete(key);
+ try {
+ String key = getKey(jti); // Use helper method to get the key
+ redisTemplate.delete(key);
+ } catch (Exception e) {
+ logger.error("Failed to remove token from denylist for jti: " + jti, e);
+ // Don't throw exception to avoid breaking logout/token cleanup flows
+ }
}
}📝 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 removeTokenFromDenylist(String jti) { | |
| if (jti != null && !jti.trim().isEmpty()) { | |
| String key = getKey(jti); // Use helper method to get the key | |
| redisTemplate.delete(key); | |
| } | |
| } | |
| public void removeTokenFromDenylist(String jti) { | |
| if (jti != null && !jti.trim().isEmpty()) { | |
| try { | |
| String key = getKey(jti); // Use helper method to get the key | |
| redisTemplate.delete(key); | |
| } catch (Exception e) { | |
| logger.error("Failed to remove token from denylist for jti: " + jti, e); | |
| // Don't throw exception to avoid breaking logout/token cleanup flows | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/tm/utils/TokenDenylist.java around lines 57 to 62, the
removeTokenFromDenylist method lacks exception handling unlike other Redis
operations in this class. Wrap the Redis delete operation inside a try-catch
block to catch any exceptions, log the error appropriately, and ensure
consistent exception handling across all Redis interactions in this class.
| 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
Improve exception handling consistency.
The method has good input validation but lacks error logging consistency compared to other methods in the class.
Apply this diff to add consistent error logging:
} catch (Exception e) {
+ logger.error("Failed to add token to denylist for jti: " + jti, e);
throw new RuntimeException("Failed to denylist 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.
| 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); | |
| } | |
| } | |
| 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) { | |
| logger.error("Failed to add token to denylist for jti: " + jti, e); | |
| throw new RuntimeException("Failed to denylist token", e); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/tm/utils/TokenDenylist.java between lines 25 and 39,
the addTokenToDenylist method catches exceptions but does not log errors
consistently with other methods. Modify the catch block to log the exception
details using the class logger before throwing the RuntimeException, ensuring
error logging is consistent across the class.
|
📋 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