AMM-1507 | Logic changes in token denylisting in forceLogout API #211
Conversation
Merge pull request PSMRI#205 from srishtigrp78/feature/version/upgrade
* null check and logger added * Redis issue * Jwttoken added for superUserAuthenticate * Null check * If mobile returned refresh token * Force logout jwttoken expiry * Method corrected * coderabbit changes
Feature/force logout
|
Warning Rate limit exceeded@srishtigrp78 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
""" WalkthroughThis update introduces a token denylisting mechanism for JWTs during force logout, replacing the previous cookie invalidation approach. It adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant IEMRAdminController
participant JwtUtil
participant TokenDenylist
participant Redis
User->>IEMRAdminController: POST /forceLogout (with JWT cookie)
IEMRAdminController->>IEMRAdminController: getJwtTokenFromCookies()
IEMRAdminController->>JwtUtil: validateToken(token)
JwtUtil->>TokenDenylist: isTokenDenylisted(jti)
TokenDenylist->>Redis: Check if jti exists
Redis-->>TokenDenylist: Exists/Not Exists
TokenDenylist-->>JwtUtil: true/false
JwtUtil-->>IEMRAdminController: Claims/null
alt Token valid
IEMRAdminController->>TokenDenylist: addTokenToDenylist(jti, expiration)
TokenDenylist->>Redis: Set jti with expiration
Redis-->>TokenDenylist: OK
TokenDenylist-->>IEMRAdminController: Done
IEMRAdminController-->>User: 200 OK (Logout successful)
else Token invalid or denylisted
IEMRAdminController-->>User: 401 Unauthorized or 400 Bad Request
end
Possibly related PRs
Suggested reviewers
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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| private CookieUtil cookieUtil; | ||
| @Autowired | ||
| private RedisTemplate<String, Object> redisTemplate; | ||
| @Value("${jwt.blacklist.expiration}") |
Check notice
Code scanning / SonarCloud
Injecting data into static fields is not supported by Spring Low
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
src/main/java/com/iemr/common/constant/Constants.java (1)
14-14: Consider using consistent casing for the token value.The constant addition looks good. However, the value "Jwttoken" uses mixed casing which differs from typical header naming conventions (usually "JWT-Token" or "JWTToken").
If backward compatibility is not a concern, consider using a more standard casing:
- public static final String JWT_TOKEN = "Jwttoken"; + public static final String JWT_TOKEN = "JWT-Token";src/main/java/com/iemr/common/utils/TokenDenylist.java (1)
10-38: Consider using Redis Sets for better semantics.Since you're only checking for existence and not using the stored value "denied", consider using Redis Sets which are more appropriate for this use case.
Instead of storing key-value pairs where the value is always "denied", you could use a single Redis Set with a key like "denylisted_tokens" and add JTIs as members with expiration handled differently. This would be more memory efficient and semantically clearer.
src/main/java/com/iemr/common/utils/JwtUtil.java (2)
116-119: Remove unused method after refactoring validateToken.After the suggested refactoring of
validateToken, this method becomes unused and can be removed.-/** - * Check if the JWT token is expired - * @param token the JWT token - * @return true if expired, false otherwise - */ -private boolean isTokenExpired(String token) { - Date expirationDate = getAllClaimsFromToken(token).getExpiration(); - return expirationDate.before(new Date()); -}
101-107: Implement the suggested logging for better debugging.The comments suggest logging JWT validation failures, which would be valuable for debugging issues in production.
Add a logger field and implement the suggested logging:
private static final Logger logger = LoggerFactory.getLogger(JwtUtil.class);Then log the exceptions as suggested in the comments.
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (1)
942-962: Consider the security implications of the token denylisting approach.The current implementation correctly denylists tokens, but there are some considerations for the expiration time calculation.
The expiration time passed to
denylistTokenshould be the remaining time until token expiration, not the absolute expiration timestamp. The current code passes the absolute timestamp which could cause tokens to be denylisted for much longer than necessary. Consider calculating the remaining time as shown in the optimization suggestion above.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/iemr/common/constant/Constants.java(1 hunks)src/main/java/com/iemr/common/controller/users/IEMRAdminController.java(6 hunks)src/main/java/com/iemr/common/utils/JwtUtil.java(1 hunks)src/main/java/com/iemr/common/utils/TokenDenylist.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/main/java/com/iemr/common/utils/TokenDenylist.java (1)
src/main/java/com/iemr/common/utils/JwtUtil.java (1)
Component(15-181)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (1)
src/main/java/com/iemr/common/constant/Constants.java (1)
Constants(3-16)
🪛 GitHub Check: SonarCloud
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
[notice] 91-91: Injecting data into static fields is not supported by Spring
Remove this injection annotation targeting the static field.See more on SonarQube Cloud
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: style-check / checkstyle
- GitHub Check: Package-test
- GitHub Check: Analyze (java)
- GitHub Check: Build
| public Claims validateToken(String token) { | ||
| // Check if the token is blacklisted (invalidated by force logout) | ||
| if (tokenDenylist.isTokenDenylisted(getJtiFromToken(token))) { | ||
| return null; // Token is denylisted, so return null | ||
| } | ||
|
|
||
| // Check if the token is expired | ||
| if (isTokenExpired(token)) { | ||
| return null; // Token is expired, so return null | ||
| } | ||
|
|
||
| // If token is not blacklisted and not expired, verify the token signature and return claims | ||
| try { | ||
| return Jwts.parser().verifyWith(getSigningKey()).build().parseSignedClaims(token).getPayload(); | ||
| } catch (ExpiredJwtException ex) { | ||
| // Handle expired token specifically | ||
| // Optionally, log the exception or return a custom message if required | ||
| return null; // Token is expired, so return null | ||
| } catch (UnsupportedJwtException | MalformedJwtException | SignatureException | IllegalArgumentException ex) { | ||
| // Log specific error types for debugging | ||
| // Optionally, you can log each exception for specific handling or debugging | ||
| // Example: logger.error("JWT validation failed: " + ex.getMessage()); | ||
| return null; // Return null for any other JWT-related issue (invalid format, bad signature, etc.) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix token validation order to prevent exceptions.
The current implementation calls getJtiFromToken before validating the token format, which will throw exceptions for malformed tokens. The token should be parsed once and reused.
public Claims validateToken(String token) {
+ try {
+ // Parse token once and reuse claims
+ Claims claims = Jwts.parser()
+ .verifyWith(getSigningKey())
+ .build()
+ .parseSignedClaims(token)
+ .getPayload();
+
+ // Check if the token is denylisted
+ String jti = claims.getId();
+ if (tokenDenylist.isTokenDenylisted(jti)) {
+ return null;
+ }
+
+ // Check if the token is expired
+ if (claims.getExpiration().before(new Date())) {
+ return null;
+ }
+
+ return claims;
+ } catch (ExpiredJwtException ex) {
+ // Token is expired
+ return null;
+ } catch (UnsupportedJwtException | MalformedJwtException | SignatureException | IllegalArgumentException ex) {
+ // Invalid token format or signature
+ return null;
+ }
- // Check if the token is blacklisted (invalidated by force logout)
- if (tokenDenylist.isTokenDenylisted(getJtiFromToken(token))) {
- return null; // Token is denylisted, so return null
- }
-
- // Check if the token is expired
- if (isTokenExpired(token)) {
- return null; // Token is expired, so return null
- }
-
- // If token is not blacklisted and not expired, verify the token signature and return claims
- try {
- return Jwts.parser().verifyWith(getSigningKey()).build().parseSignedClaims(token).getPayload();
- } catch (ExpiredJwtException ex) {
- // Handle expired token specifically
- // Optionally, log the exception or return a custom message if required
- return null; // Token is expired, so return null
- } catch (UnsupportedJwtException | MalformedJwtException | SignatureException | IllegalArgumentException ex) {
- // Log specific error types for debugging
- // Optionally, you can log each exception for specific handling or debugging
- // Example: logger.error("JWT validation failed: " + ex.getMessage());
- return null; // Return null for any other JWT-related issue (invalid format, bad signature, etc.)
- }
}📝 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 Claims validateToken(String token) { | |
| // Check if the token is blacklisted (invalidated by force logout) | |
| if (tokenDenylist.isTokenDenylisted(getJtiFromToken(token))) { | |
| return null; // Token is denylisted, so return null | |
| } | |
| // Check if the token is expired | |
| if (isTokenExpired(token)) { | |
| return null; // Token is expired, so return null | |
| } | |
| // If token is not blacklisted and not expired, verify the token signature and return claims | |
| try { | |
| return Jwts.parser().verifyWith(getSigningKey()).build().parseSignedClaims(token).getPayload(); | |
| } catch (ExpiredJwtException ex) { | |
| // Handle expired token specifically | |
| // Optionally, log the exception or return a custom message if required | |
| return null; // Token is expired, so return null | |
| } catch (UnsupportedJwtException | MalformedJwtException | SignatureException | IllegalArgumentException ex) { | |
| // Log specific error types for debugging | |
| // Optionally, you can log each exception for specific handling or debugging | |
| // Example: logger.error("JWT validation failed: " + ex.getMessage()); | |
| return null; // Return null for any other JWT-related issue (invalid format, bad signature, etc.) | |
| } | |
| } | |
| public Claims validateToken(String token) { | |
| try { | |
| // Parse token once and reuse claims | |
| Claims claims = Jwts.parser() | |
| .verifyWith(getSigningKey()) | |
| .build() | |
| .parseSignedClaims(token) | |
| .getPayload(); | |
| // Check if the token is denylisted | |
| String jti = claims.getId(); | |
| if (tokenDenylist.isTokenDenylisted(jti)) { | |
| return null; | |
| } | |
| // Check if the token is expired | |
| if (claims.getExpiration().before(new Date())) { | |
| return null; | |
| } | |
| return claims; | |
| } catch (ExpiredJwtException ex) { | |
| // Token is expired | |
| return null; | |
| } catch (UnsupportedJwtException | MalformedJwtException | SignatureException | IllegalArgumentException ex) { | |
| // Invalid token format or signature | |
| return null; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/utils/JwtUtil.java between lines 85 and 109,
the method validateToken calls getJtiFromToken before validating the token
format, which can throw exceptions for malformed tokens. Refactor the method to
parse the token once at the start inside a try-catch block, then check if the
token is denylisted or expired using the parsed claims. This avoids multiple
parsing and prevents exceptions from malformed tokens by handling them in the
catch block.
There was a problem hiding this comment.
@srishtigrp78 Great! Thank you for fixing the token validation order issue. The refactored method will now handle malformed tokens gracefully and avoid unnecessary parsing operations.
🐰✨
🎉
╭─────╮
│ ◕ ◕ │
│ ω │
╰─────╯
│ │
│ │
╭─╯ ╰─╮
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
Outdated
Show resolved
Hide resolved
| // Extract token from cookies or headers | ||
| String token = getJwtTokenFromCookies(httpRequest); | ||
| if (token == null) { | ||
| token = httpRequest.getHeader(Constants.JWT_TOKEN); | ||
| } | ||
|
|
||
| // Validate the token: Check if it is expired or in the deny list | ||
| Claims claims = jwtUtil.validateToken(token); | ||
| if (claims == null) { | ||
| // If token is either expired or in the deny list, return 401 Unauthorized | ||
| response.setStatus(HttpServletResponse.SC_UNAUTHORIZED); | ||
| outputResponse.setError(new RuntimeException("Token is expired or has been logged out")); | ||
| return outputResponse.toString(); | ||
| } | ||
|
|
||
| // Extract the jti (JWT ID) and expiration time from the JWT token | ||
| String jti = jwtUtil.getJtiFromToken(token); | ||
| long expirationTime = jwtUtil.getAllClaimsFromToken(token).getExpiration().getTime(); | ||
|
|
||
| // Denylist the token's jti in Redis with its expiration time | ||
| tokenDenylist.denylistToken(jti, expirationTime); |
There was a problem hiding this comment.
Optimize JWT token parsing and improve error handling.
The current implementation parses the JWT token multiple times and has potential issues with null handling.
// Extract token from cookies or headers
String token = getJwtTokenFromCookies(httpRequest);
if (token == null) {
token = httpRequest.getHeader(Constants.JWT_TOKEN);
}
+
+if (token == null) {
+ response.setStatus(HttpServletResponse.SC_BAD_REQUEST);
+ outputResponse.setError(new RuntimeException("No JWT token found in request"));
+ return outputResponse.toString();
+}
// Validate the token: Check if it is expired or in the deny list
Claims claims = jwtUtil.validateToken(token);
if (claims == null) {
// If token is either expired or in the deny list, return 401 Unauthorized
response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
outputResponse.setError(new RuntimeException("Token is expired or has been logged out"));
return outputResponse.toString();
}
// Extract the jti (JWT ID) and expiration time from the JWT token
-String jti = jwtUtil.getJtiFromToken(token);
-long expirationTime = jwtUtil.getAllClaimsFromToken(token).getExpiration().getTime();
+String jti = claims.getId();
+long expirationTime = claims.getExpiration().getTime() - System.currentTimeMillis();
// Denylist the token's jti in Redis with its expiration time
tokenDenylist.denylistToken(jti, expirationTime);📝 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.
| // Extract token from cookies or headers | |
| String token = getJwtTokenFromCookies(httpRequest); | |
| if (token == null) { | |
| token = httpRequest.getHeader(Constants.JWT_TOKEN); | |
| } | |
| // Validate the token: Check if it is expired or in the deny list | |
| Claims claims = jwtUtil.validateToken(token); | |
| if (claims == null) { | |
| // If token is either expired or in the deny list, return 401 Unauthorized | |
| response.setStatus(HttpServletResponse.SC_UNAUTHORIZED); | |
| outputResponse.setError(new RuntimeException("Token is expired or has been logged out")); | |
| return outputResponse.toString(); | |
| } | |
| // Extract the jti (JWT ID) and expiration time from the JWT token | |
| String jti = jwtUtil.getJtiFromToken(token); | |
| long expirationTime = jwtUtil.getAllClaimsFromToken(token).getExpiration().getTime(); | |
| // Denylist the token's jti in Redis with its expiration time | |
| tokenDenylist.denylistToken(jti, expirationTime); | |
| // Extract token from cookies or headers | |
| String token = getJwtTokenFromCookies(httpRequest); | |
| if (token == null) { | |
| token = httpRequest.getHeader(Constants.JWT_TOKEN); | |
| } | |
| if (token == null) { | |
| response.setStatus(HttpServletResponse.SC_BAD_REQUEST); | |
| outputResponse.setError(new RuntimeException("No JWT token found in request")); | |
| return outputResponse.toString(); | |
| } | |
| // Validate the token: Check if it is expired or in the deny list | |
| Claims claims = jwtUtil.validateToken(token); | |
| if (claims == null) { | |
| // If token is either expired or in the deny list, return 401 Unauthorized | |
| response.setStatus(HttpServletResponse.SC_UNAUTHORIZED); | |
| outputResponse.setError(new RuntimeException("Token is expired or has been logged out")); | |
| return outputResponse.toString(); | |
| } | |
| // Extract the jti (JWT ID) and remaining TTL from the JWT token | |
| String jti = claims.getId(); | |
| long expirationTime = claims.getExpiration().getTime() - System.currentTimeMillis(); | |
| // Denylist the token's jti in Redis with its expiration time | |
| tokenDenylist.denylistToken(jti, expirationTime); |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
around lines 942 to 962, the JWT token is parsed multiple times which is
inefficient and error-prone. Refactor the code to parse the token once, store
the Claims object, and reuse it for validation, extracting the jti, and
expiration time. Also, add proper null checks after token extraction to handle
missing tokens gracefully before proceeding with validation.
There was a problem hiding this comment.
@srishtigrp78 Great! I can see you've added the null check for the token extraction, which addresses the main concern about handling missing tokens gracefully. Thanks for implementing that fix!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
|
|
||
| // Extract the jti (JWT ID) and expiration time from the JWT token | ||
| String jti = jwtUtil.getJtiFromToken(token); | ||
| long expirationTime = jwtUtil.getAllClaimsFromToken(token).getExpiration().getTime(); |
There was a problem hiding this comment.
didn't you get claims from L949 already? is called helper required.
There was a problem hiding this comment.
Yeah Mithun, thanks for pointing it out. You're right — I was extracting claims from the token twice. I’ll refactor the code to avoid redundant calls.
| long expirationTime = jwtUtil.getAllClaimsFromToken(token).getExpiration().getTime(); | ||
|
|
||
| // Denylist the token's jti in Redis with its expiration time | ||
| tokenDenylist.denylistToken(jti, expirationTime); |
There was a problem hiding this comment.
addTokenToDenyList?
The method doesn't sound right.
There was a problem hiding this comment.
fixed it as per suggestion Mithun
| } | ||
| if (expirationTime == null || expirationTime <= 0) { | ||
| throw new IllegalArgumentException("Expiration time must be positive"); | ||
| } |
There was a problem hiding this comment.
fixing it Mithun.
| } | ||
| // Store the jti in Redis with expiration time set to the token's exp time (in milliseconds) | ||
| try { | ||
| redisTemplate.opsForValue().set(jti, "denied", expirationTime, TimeUnit.MILLISECONDS); |
| } | ||
| // Store the jti in Redis with expiration time set to the token's exp time (in milliseconds) | ||
| try { | ||
| redisTemplate.opsForValue().set(jti, "denied", expirationTime, TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
What will the expiry time? What you get from token is the unix timestamp.
Are you sure about the timeunit?
There was a problem hiding this comment.
mm..yeah..You're correct . I will handle that in the code after analysing the required change to ensure that the expiration time is handled properly.
Thanks for catching that! I’ll fix it .
There was a problem hiding this comment.
A Unix timestamp is usually represented in seconds. Will handle its conversion properly for Redis.
There was a problem hiding this comment.
I didn't follow where the jti is saved in Redis.
Is it inside a 'set'?
But your checks are against redistemplate.hasKey()
There was a problem hiding this comment.
No Mithun, we’re not saving the jti inside a Redis set. The jti is being stored as a key with a value of "denied". in this case, we are using the key itself as the identifier for the denylist,since each jti is being stored as a unique key with the value "denied", i think the hasKey() method is appropriate for checking the presence of a denylisted token here.
There was a problem hiding this comment.
Okay it's a good practice to namespace keys in Redis.
If you put the jti itself as key in redis, it's going to be confusing and might lead to collisions.
add denied: or denied_ as the key maybe.
Also, a value is not need. Just the presence is key is more than enough.
There was a problem hiding this comment.
ok Mithun, i have added a prefix "denied_" and i am not adding any value now, just keeping it empty
|
@srishtigrp78 indent is files is off in many places. How have you tested this? |
|
@srishtigrp78 check GitHub action failures too |
| if (expirationTime == null || expirationTime <= 0) { | ||
| throw new IllegalArgumentException("Expiration time must be positive"); | ||
| } | ||
| // Store the jti in Redis with expiration time set to the token's exp time (in milliseconds) |
There was a problem hiding this comment.
Token's expiry is unix timestamp, not milliseconds.
There was a problem hiding this comment.
yah Mithun, just read about it and fixed.
There was a problem hiding this comment.
@srishtigrp78 let's fix this comment/ remove it.
@drtechie indent check is not available in IDE as of now. |
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/common/controller/users/IEMRAdminController.java(6 hunks)src/main/java/com/iemr/common/utils/TokenDenylist.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/iemr/common/utils/TokenDenylist.java (1)
src/main/java/com/iemr/common/utils/JwtUtil.java (1)
Component(15-177)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (java)
- GitHub Check: Build
- GitHub Check: Package-test
🔇 Additional comments (1)
src/main/java/com/iemr/common/utils/TokenDenylist.java (1)
21-37: Method implementation looks good with proper error handling.The addTokenToDenylist method correctly implements input validation, error handling, and uses the PREFIX for Redis key namespacing as discussed in previous reviews.
| // Store the jti in Redis with expiration time set to the token's exp time (in milliseconds) | ||
| try { | ||
| String key = PREFIX + jti; | ||
| redisTemplate.opsForValue().set(key, " ", expirationTime * 1000, TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the expiration time conversion is correct.
The code multiplies expirationTime * 1000 to convert from seconds to milliseconds. Based on past review comments, this appears to handle Unix timestamp conversion correctly, but please verify that the expirationTime parameter passed from JwtUtil is indeed in seconds.
🏁 Script executed:
#!/bin/bash
# Check how expirationTime is calculated in JwtUtil when calling addTokenToDenylist
ast-grep --pattern $'tokenDenylist.addTokenToDenylist($_, $_)'Length of output: 203
🏁 Script executed:
#!/bin/bash
# Show declaration and usage context of expirationTime in the controller
rg -n "expirationTime" -C3 src/main/java/com/iemr/common/controller/users/IEMRAdminController.javaLength of output: 616
Fix TTL Calculation for Redis Denylist
The expirationTime passed from the controller is the absolute JWT expiration timestamp in milliseconds (via claims.getExpiration().getTime()), not a duration in seconds. Multiplying that by 1,000 produces an enormous TTL. Instead, compute the time-to-live relative to now and pass that directly to Redis.
• In IEMRAdminController.java (around L959–962):
// Old
long expirationTime = claims.getExpiration().getTime();
tokenDenylist.addTokenToDenylist(jti, expirationTime);
// New: compute TTL in milliseconds
long expirationTimestamp = claims.getExpiration().getTime();
long ttlMillis = expirationTimestamp - System.currentTimeMillis();
tokenDenylist.addTokenToDenylist(jti, ttlMillis);• In TokenDenylist.java (around L32):
- redisTemplate.opsForValue().set(key, " ", expirationTime * 1000, TimeUnit.MILLISECONDS);
+ // expirationTime (better named ttlMillis) is already a relative duration in ms
+ redisTemplate.opsForValue().set(key, " ", expirationTime, TimeUnit.MILLISECONDS);Renaming the parameter to ttlMillis or similar will make its purpose clear and avoid future confusion.
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/utils/TokenDenylist.java at line 32, the
expirationTime parameter is currently treated as a duration in seconds and
multiplied by 1000 to convert to milliseconds, but it actually represents an
absolute expiration timestamp in milliseconds. To fix this, rename the parameter
to ttlMillis to clarify its meaning and remove the multiplication by 1000,
passing the TTL directly as milliseconds to redisTemplate.opsForValue().set.
Also, ensure that the caller computes the TTL as the difference between the
expiration timestamp and the current time before passing it in.
There was a problem hiding this comment.
🧩 Analysis chain
@srishtigrp78 Let me verify that the fix has been applied correctly.
🏁 Script executed:
#!/bin/bash
# Check the current implementation in TokenDenylist.java to see if the multiplication by 1000 has been removed
rg -n "expirationTime \* 1000" src/main/java/com/iemr/common/utils/TokenDenylist.java
# Check the addTokenToDenylist method implementation
rg -n -A 5 -B 2 "redisTemplate\.opsForValue\(\)\.set" src/main/java/com/iemr/common/utils/TokenDenylist.java
# Check how the controller calls addTokenToDenylist to see if TTL calculation has been fixed
rg -n -A 3 -B 3 "addTokenToDenylist" src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
Length of output: 1054
@srishtigrp78 I can see that the fix hasn't been applied yet. The code still has both issues:
In TokenDenylist.java line 32:
redisTemplate.opsForValue().set(key, " ", expirationTime * 1000, TimeUnit.MILLISECONDS);The * 1000 multiplication is still there.
In IEMRAdminController.java lines 958-961:
long expirationTime = claims.getExpiration().getTime(); // Use expiration from claims
tokenDenylist.addTokenToDenylist(jti, expirationTime);This is still passing the absolute expiration timestamp instead of computing the TTL.
To fix this, you need to make these changes:
- In
IEMRAdminController.java:
long expirationTimestamp = claims.getExpiration().getTime();
long ttlMillis = expirationTimestamp - System.currentTimeMillis();
tokenDenylist.addTokenToDenylist(jti, ttlMillis);- In
TokenDenylist.java:
redisTemplate.opsForValue().set(key, " ", expirationTime, TimeUnit.MILLISECONDS);(Remove the * 1000)
This will ensure that Redis receives the correct TTL duration in milliseconds.
🧬 Code Graph Analysis Results
Relevant snippet from src/main/java/com/iemr/common/utils/JwtUtil.java lines 15-177
@Component
public class JwtUtil {
@Value("${jwt.secret}")
private String SECRET_KEY;
@Value("${jwt.access.expiration}")
private long ACCESS_EXPIRATION_TIME;
@Value("${jwt.refresh.expiration}")
private long REFRESH_EXPIRATION_TIME;
@Autowired
private TokenDenylist tokenDenylist;
private SecretKey getSigningKey() {
if (SECRET_KEY == null || SECRET_KEY.isEmpty()) {
throw new IllegalStateException("JWT secret key is not set in application.properties");
}
return Keys.hmacShaKeyFor(SECRET_KEY.getBytes());
}
/**
* Generate an access token.
*
* @param username the username of the user
* @param userId the user ID
* @return the generated JWT access token
*/
public String generateToken(String username, String userId) {
return buildToken(username, userId, "access", ACCESS_EXPIRATION_TIME);
}
/**
* Generate a refresh token.
*
* @param username the username of the user
* @param userId the user ID
* @return the generated JWT refresh token
*/
public String generateRefreshToken(String username, String userId) {
return buildToken(username, userId, "refresh", REFRESH_EXPIRATION_TIME);
}
/**
* Build a JWT token with the specified parameters.
*
* @param username the username of the user
* @param userId the user ID
* @param tokenType the type of the token (access or refresh)
* @param expiration the expiration time of the token in milliseconds
* @return the generated JWT token
*/
private String buildToken(String username, String userId, String tokenType, long expiration) {
return Jwts.builder()
.subject(username)
.claim("userId", userId)
.claim("token_type", tokenType)
.id(UUID.randomUUID().toString())
.issuedAt(new Date())
.expiration(new Date(System.currentTimeMillis() + expiration))
.signWith(getSigningKey())
.compact();
}
/**
* Validate the JWT token, checking if it is expired and if it's blacklisted
* @param token the JWT token
* @return Claims if valid, null if invalid (expired or denylisted)
*/
public Claims validateToken(String token) {
// Check if the token is blacklisted (invalidated by force logout)
if (tokenDenylist.isTokenDenylisted(getJtiFromToken(token))) {
return null; // Token is denylisted, so return null
}
// Check if the token is expired
if (isTokenExpired(token)) {
return null; // Token is expired, so return null
}
// If token is not blacklisted and not expired, verify the token signature and return claims
try {
return Jwts.parser().verifyWith(getSigningKey()).build().parseSignedClaims(token).getPayload();
} catch (ExpiredJwtException ex) {
return null; // Token is expired, so return null
} catch (UnsupportedJwtException | MalformedJwtException | SignatureException | IllegalArgumentException ex) {
return null; // Return null for any other JWT-related issue (invalid format, bad signature, etc.)
}
}
/**
* Check if the JWT token is expired
* @param token the JWT token
* @return true if expired, false otherwise
*/
private boolean isTokenExpired(String token) {
Date expirationDate = getAllClaimsFromToken(token).getExpiration();
return expirationDate.before(new Date());
}
/**
* Extract claims from the token
* @param token the JWT token
* @return all claims from the token
*/
public Claims getAllClaimsFromToken(String token) {
return Jwts.parser()
.verifyWith(getSigningKey())
.build()
.parseSignedClaims(token)
.getPayload();
}
/**
* Extract a specific claim from the token using a function
* @param token the JWT token
* @param claimsResolver the function to extract the claim
* @param <T> the type of the claim
* @return the extracted claim
*/
public <T> T getClaimFromToken(String token, Function<Claims, T> claimsResolver) {
final Claims claims = getAllClaimsFromToken(token);
return claimsResolver.apply(claims);
}
/**
* Get the JWT ID (JTI) from the token
* @param token the JWT token
* @return the JWT ID
*/
public String getJtiFromToken(String token) {
return getAllClaimsFromToken(token).getId();
}
/**
* Get the username from the token
* @param token the JWT token
* @return the username
*/
public String getUsernameFromToken(String token) {
return getAllClaimsFromToken(token).getSubject();
}
/**
* Get the user ID from the token
* @param token the JWT token
* @return the user ID
*/
public String getUserIdFromToken(String token) {
return getAllClaimsFromToken(token).get("userId", String.class);
}
/**
* Get the expiration time of the refresh token
* @return the expiration time in milliseconds
*/
public long getRefreshTokenExpiration() {
return REFRESH_EXPIRATION_TIME;
}
}Summary:
This class JwtUtil is responsible for generating, validating, and extracting information from JWT tokens. It uses the TokenDenylist component to check if a token's JTI is denylisted (blacklisted). The denylist check is integrated in the validateToken method, which returns null if the token is denylisted or expired. The class also provides methods to generate access and refresh tokens, extract claims, and get token metadata such as JTI, username, and user ID.
This snippet is relevant because it directly uses the TokenDenylist class from the provided file and shows how tokens are checked against the denylist. It helps understand the context in which TokenDenylist is used and how the denylist affects token validation.
| // Store the jti in Redis with expiration time set to the token's exp time (in milliseconds) | ||
| try { | ||
| String key = PREFIX + jti; | ||
| redisTemplate.opsForValue().set(key, " ", expirationTime * 1000, TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
Sorry what is this expirationTime?
Why is this multiplied by 1000?
There was a problem hiding this comment.
earlier it was multuplied by 1000 bcoz we were converting the expiration time (earlier in UNIX timestamp) in miliseconds but as we are getting it through Claims now, so need of conversion, also logic i have fixed now as pr discussion.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/com/iemr/common/utils/TokenDenylist.java (1)
21-36:⚠️ Potential issueCritical Issue: TTL calculation may be incorrect for Redis.
Based on past review comments, the
expirationTimeparameter appears to be an absolute timestamp (fromclaims.getExpiration().getTime()) rather than a relative TTL duration. If this is the case, passing it directly to Redis will result in an incorrect TTL.The method should calculate the TTL as the difference between the expiration timestamp and current 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 = PREFIX + jti; - redisTemplate.opsForValue().set(key, " ", expirationTime, TimeUnit.MILLISECONDS); + long ttlMillis = expirationTime - System.currentTimeMillis(); + if (ttlMillis <= 0) { + return; // Token already expired, no need to denylist + } + redisTemplate.opsForValue().set(key, " ", ttlMillis, TimeUnit.MILLISECONDS); } catch (Exception e) { throw new RuntimeException("Failed to denylist token", e); } }Alternatively, if you want to be more explicit, consider renaming the parameter to
expirationTimestampand adding a separate parameter for TTL.
🧹 Nitpick comments (1)
src/main/java/com/iemr/common/utils/TokenDenylist.java (1)
32-32: Consider using an empty string instead of space for Redis value.The use of a space character
" "as the Redis value is unusual. Since you're only checking for key existence, consider using an empty string""for clarity.- redisTemplate.opsForValue().set(key, " ", ttlMillis, TimeUnit.MILLISECONDS); + redisTemplate.opsForValue().set(key, "", ttlMillis, TimeUnit.MILLISECONDS);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java(6 hunks)src/main/java/com/iemr/common/utils/TokenDenylist.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/iemr/common/utils/TokenDenylist.java (1)
src/main/java/com/iemr/common/utils/JwtUtil.java (1)
Component(15-177)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (java)
- GitHub Check: Build
- GitHub Check: Package-test
🔇 Additional comments (1)
src/main/java/com/iemr/common/utils/TokenDenylist.java (1)
39-51: Excellent error handling and fail-safe approach.The method correctly handles Redis failures by returning
falseto avoid blocking all requests when Redis is unavailable. The use ofBoolean.TRUE.equals()provides null safety, and the logging is appropriate for debugging.
| public void removeTokenFromDenylist(String jti) { | ||
| if (jti != null && !jti.trim().isEmpty()) { | ||
| String key = PREFIX + jti; | ||
| redisTemplate.delete(key); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for consistency with other methods.
The method lacks error handling for Redis operations, which is inconsistent with the other methods in the class. Consider adding try-catch to handle potential Redis exceptions.
public void removeTokenFromDenylist(String jti) {
if (jti != null && !jti.trim().isEmpty()) {
- String key = PREFIX + jti;
- redisTemplate.delete(key);
+ try {
+ String key = PREFIX + jti;
+ redisTemplate.delete(key);
+ } catch (Exception e) {
+ logger.error("Failed to remove token from denylist for jti: " + jti, e);
+ // Consider whether to throw or just log based on your error handling strategy
+ }
}
}🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/utils/TokenDenylist.java around lines 54 to 59,
the removeTokenFromDenylist method lacks error handling for Redis operations,
unlike other methods in the class. Add a try-catch block around the
redisTemplate.delete call to catch any exceptions thrown during the Redis
operation and handle them appropriately, such as logging the error, to maintain
consistency and robustness.
| tokenDenylist.addTokenToDenylist(jti, ttlMillis); | ||
|
|
||
| // Denylist the token's jti in Redis with its expiration time | ||
| tokenDenylist.addTokenToDenylist(jti, expirationTime); |
| } | ||
|
|
||
| // Check if the token is expired | ||
| if (isTokenExpired(token)) { |
There was a problem hiding this comment.
This check is not needed again, but okay
There was a problem hiding this comment.
okay Mithun, keeping it as it is as of now.
|
📋 Description
JIRA ID:
AMM-1507 | Logic changes in token denylisting in forceLogout API(includes changes recommended by Dr. Mithun James)
Please provide a summary of the change and the motivation behind it. Include relevant context and details.
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation