Enhance JWT Handling: Return Token in Response for Mobile, Add Secure Refresh API, and Update Middleware #181
Conversation
… setting a cookie, Implement refresh tokens with a long expiry for session management, Create a new API to refresh JWT tokens securely, Modify middleware to handle both cookies (for web) and Authorization headers (for mobile).
WalkthroughThe changes update the authentication system by refactoring token handling and enhancing security checks. In the controller, mobile device detection is implemented using a new utility to decide between JWT cookie responses and issuing a refresh token stored in Redis. A new refresh token endpoint is added. The JWT utility now manages access and refresh tokens with distinct expiration properties and improved claim parsing, while the user service gains a user lookup method. Additionally, logging and authentication skipping have been refined in supporting filters and interceptors, and necessary properties were added to the configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant A as IEMRAdminController
participant UA as UserAgentUtil
participant R as Redis/Token Store
participant Jwt as JwtUtil
C->>A: Login Request (with User-Agent header)
A->>UA: isMobileDevice(userAgent)
UA-->>A: true / false
alt Mobile Device Detected
A->>Jwt: generateRefreshToken(username, userId)
Jwt-->>A: Refresh Token
A->>R: Store refresh token with unique JTI
A->>C: Return JWT and Refresh Token
else Non-Mobile Device
A->>Jwt: generateToken(username, userId)
Jwt-->>A: JWT Token
A->>C: Return JWT as cookie
end
sequenceDiagram
participant C as Client
participant A as IEMRAdminController
participant R as Redis/Token Store
participant Jwt as JwtUtil
C->>A: RefreshToken Request (with refresh token)
A->>R: Validate refresh token (check type & revocation)
R-->>A: Valid / Invalid token status
alt Token Valid
A->>Jwt: generateToken(username, userId)
Jwt-->>A: New JWT Token
A->>C: Return new JWT Token
else Token Invalid/Expired
A->>C: Respond with error status/message
end
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
🧹 Nitpick comments (8)
src/main/java/com/iemr/common/utils/JwtUserIdValidationFilter.java (1)
90-98: Well-structured authentication bypass method with refresh token supportThe
shouldSkipAuthenticationmethod properly includes the new refresh token endpoint alongside existing bypass paths. The method has a clear purpose and is well-implemented.However, I recommend considering using a collection (like a Set) for path patterns to improve maintainability if the number of excluded paths continues to grow.
- private boolean shouldSkipAuthentication(String path, String contextPath) { - return path.equals(contextPath + "/user/userAuthenticate") - || path.equalsIgnoreCase(contextPath + "/user/logOutUserFromConcurrentSession") - || path.startsWith(contextPath + "/swagger-ui") - || path.startsWith(contextPath + "/v3/api-docs") - || path.startsWith(contextPath + "/public") - || path.equals(contextPath + "/user/refreshToken") - ; - } + private boolean shouldSkipAuthentication(String path, String contextPath) { + // Exact path matches + Set<String> exactPaths = Set.of( + contextPath + "/user/userAuthenticate", + contextPath + "/user/logOutUserFromConcurrentSession", + contextPath + "/user/refreshToken" + ); + + // Prefix matches + Set<String> prefixPaths = Set.of( + contextPath + "/swagger-ui", + contextPath + "/v3/api-docs", + contextPath + "/public" + ); + + return exactPaths.contains(path) || + prefixPaths.stream().anyMatch(prefix -> path.startsWith(prefix)); + }src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (5)
24-24: Avoid wildcard import fromjava.util
Usingimport java.util.*can lead to namespace clutter and potential class conflicts. It's generally better to import only the specific classes needed.-import java.util.*; +import java.util.List; +import java.util.Map; +import java.util.concurrent.TimeUnit;
40-40: Avoid wildcard import fromorg.springframework.web.bind.annotation
Consider importing only the required annotations to keep the namespace clean.-import org.springframework.web.bind.annotation.*; +import org.springframework.web.bind.annotation.CrossOrigin; +import org.springframework.web.bind.annotation.Operation; +import org.springframework.web.bind.annotation.RequestBody; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RequestMethod; +import org.springframework.web.bind.annotation.PathVariable; +import org.springframework.web.bind.annotation.RestController;
164-166: Handle null or empty User-Agent
Ifrequest.getHeader("User-Agent")is null or empty,UserAgentUtil.isMobileDevice()might not behave as expected. Consider providing a fallback or explicit check.String userAgent = request.getHeader("User-Agent"); +if (userAgent == null) { + userAgent = ""; +} isMobile = UserAgentUtil.isMobileDevice(userAgent);
255-261: Token rotation consideration
Only a new access token is returned. For stronger security, consider rotating the refresh token by issuing a new one and revoking the old.
806-806: Incomplete Javadoc
The@param keyannotation lacks a description. Either remove it if unused or provide details on the parameter.src/main/java/com/iemr/common/utils/JwtUtil.java (2)
32-32: Key generation
getSigningKey()uses the secret key from the configuration. In a production environment, consider rotating secrets or storing them in a secure vault.
80-86: Potential redundancy withvalidateToken
getAllClaimsFromTokenalso parses the token. If you callvalidateTokenbeforehand, you may be parsing twice. Consider combining them for efficiency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java(6 hunks)src/main/java/com/iemr/common/service/users/IEMRAdminUserService.java(1 hunks)src/main/java/com/iemr/common/utils/JwtUserIdValidationFilter.java(3 hunks)src/main/java/com/iemr/common/utils/JwtUtil.java(1 hunks)src/main/java/com/iemr/common/utils/UserAgentUtil.java(1 hunks)src/main/java/com/iemr/common/utils/http/HTTPRequestInterceptor.java(1 hunks)
🔇 Additional comments (28)
src/main/java/com/iemr/common/service/users/IEMRAdminUserService.java (1)
119-119: Interface enhancement aligned with refresh token implementationThe new
getUserByIdmethod is a necessary addition to support the refresh token functionality, allowing user data retrieval during token validation and refresh processes.src/main/java/com/iemr/common/utils/UserAgentUtil.java (1)
1-9: Well-structured utility for mobile device detectionThis utility class properly implements mobile device detection by checking for common mobile identifiers in the user agent string. The implementation includes proper null handling and case normalization.
One suggestion: Consider adding more comprehensive mobile detection patterns to catch less common devices or update the detection logic as user agents evolve.
src/main/java/com/iemr/common/utils/http/HTTPRequestInterceptor.java (1)
109-109: Appropriate authentication bypass for refresh token endpointAdding the "refreshToken" case is necessary to allow unauthenticated access to the refresh token endpoint, which aligns with standard token refresh patterns.
src/main/java/com/iemr/common/utils/JwtUserIdValidationFilter.java (3)
55-60: Improved code organization with extracted methodExtracting the authentication skip logic to a separate method enhances readability and maintainability. The updated logging format using parameterized message is also a good practice.
70-73: Enhanced error logging for missing JWT tokenThe improved error logging provides better context for troubleshooting authentication issues.
76-83: Simplified token validation logicDirect use of the validation method result in the conditional improves code clarity by removing the unnecessary intermediate boolean variable.
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (11)
29-31: Imports look good
These imports (UserAgentUtil, Claims, ExpiredJwtException) are relevant to the new JWT and user-agent detection logic.
153-156: Local variables introduced for JWT and refresh flow
DefiningjwtToken,refreshToken, andisMobileclearly emphasizes the different tokens and usage contexts.
158-158: Consistent use of JWT utility
The call tojwtUtil.generateToken()aligns with the new token generation structure.
168-178: Storing refresh token in Redis
Using a unique JTI as the key for refresh tokens is a solid approach. This design supports token revocation checks.
182-182: Clear Redis key naming
Using"user_" + mUser.get(0).getUserID()to store user details ensures a unique key for each user session.
184-185: Storing user object in Redis
Persisting user data with a 30-minute TTL helps manage session-limited info.
207-212: Return tokens for mobile
ExposingjwtTokenandrefreshTokenin the response body fulfills the PR’s objective to support mobile authentication flows.
222-227: Refresh endpoint definition
The new/refreshTokenendpoint provides a clear entry point for the refresh flow. Ensure you validate that"refreshToken"is not null in the request body if applicable.
242-247: Valid “refresh” token type
Ensuring the token has"token_type": "refresh"adds a crucial safeguard against misuse of access tokens in the refresh endpoint.
249-254: Revoked check using Redis
Checkingif (!redisTemplate.hasKey("refresh:" + jti))is a straightforward way to handle refresh token revocation.
266-272: Exception handling
The granular catch forExpiredJwtExceptionplus a general fallback for other exceptions is a solid approach.src/main/java/com/iemr/common/utils/JwtUtil.java (11)
4-18: Added imports for new exceptions, UUID, andSecretKey
These additions are consistent with the updated JWT logic and the new approach for keyed signing.
26-30: Separate access and refresh expirations
Storing separate expiration values for access vs. refresh tokens aligns with best practices.
40-41: Refactored token generation
Delegating tobuildToken(...)keepsgenerateToken(...)concise and maintainable.
43-45: Dedicated method for refresh token
IntroducinggenerateRefreshToken(...)provides better clarity and avoids confusion with the regular access token generation flow.
47-57:buildTokenmethod
Centralizing claim assignment inbuildTokenhelps reduce duplicate code and clarifies token differences viatokenType.
60-66: Improved validation logic
TheverifyWith(...).build().parseSignedClaims(token)approach, combined with the subsequent catch blocks, is a clean pattern for validating token integrity.
67-70: Explicit error handling
CatchingExpiredJwtExceptionseparately, plusUnsupportedJwtException,MalformedJwtException, etc., provides more granular error reporting.
75-77: Generic getter for claims
getClaimFromToken(...)elegantly defers togetAllClaimsFromToken(...)without duplicating logic.
88-90: Accessor for refresh token expiration
getRefreshTokenExpiration()simplifies retrieval of the configured refresh expiration.
92-93: Extracting the JWT ID
getJtiFromTokenreturns the token’sjticlaim. Very useful for token revocation checks.
97-98: Extracting the username
getUsernameFromTokenclarifies the subject claim extraction.
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (4)
173-177: Redis key format for refresh tokensConsider using a constant for the Redis key prefix "refresh:" to improve maintainability.
+ // Use a constant for Redis key prefixes redisTemplate.opsForValue().set( - "refresh:" + jti, + RedisKeyPrefixes.REFRESH_TOKEN + jti, user.getUserID().toString(), jwtUtil.getRefreshTokenExpiration(), TimeUnit.MILLISECONDS );
182-185: User caching in RedisGood implementation of user caching in Redis with appropriate TTL. Consider making the TTL configurable rather than hardcoded.
- redisTemplate.opsForValue().set(redisKey, user, 30, TimeUnit.MINUTES); + redisTemplate.opsForValue().set(redisKey, user, userCacheTTLMinutes, TimeUnit.MINUTES);
242-244: Refresh token revocation checkGood implementation of token revocation checking using Redis. Consider also invalidating the old refresh token when a new one is issued to prevent potential misuse.
// Check revocation using JTI String jti = claims.getId(); if (!redisTemplate.hasKey("refresh:" + jti)) { return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body("Token revoked"); } + + // Optional: Add token rotation by deleting the used refresh token + // This enhances security by ensuring each refresh token can only be used once + redisTemplate.delete("refresh:" + jti);
257-262: Error handling for token refreshGood implementation of specific error handling for token refresh. Consider adding more detailed logs for troubleshooting.
} catch (ExpiredJwtException ex) { + logger.info("Refresh token expired for user attempt: {}", ex.getClaims().getSubject()); return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body("Token expired"); } catch (Exception e) { - logger.error("Refresh failed: ", e); + logger.error("Token refresh failed with exception: {}", e.getMessage(), e); return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body("Token refresh failed"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java(6 hunks)
🔇 Additional comments (8)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (8)
24-41: New imports added for mobile authentication and JWT handlingThe imports have been updated to support the new JWT handling features for mobile authentication, including
UserAgentUtil,Claims, and exception handling for expired tokens.
154-156: Variables added for JWT token handlingAppropriate variables introduced to track JWT token, refresh token, and mobile device status.
158-158: JWT token generation callThe token generation call is now using the appropriate method to generate the standard JWT token.
164-166: Mobile device detection implementationGood implementation of device type detection via User-Agent header parsing.
168-180: Conditional token delivery based on device typeThe implementation correctly handles different authentication methods for mobile vs. web clients. For mobile, it generates a refresh token and stores it in Redis with appropriate expiration. For web, it maintains backward compatibility by setting a cookie.
207-211: Add tokens to response for mobile clientsThis implementation correctly addresses the PR objective of returning tokens in the response body for mobile clients.
222-263: New endpoint for token refresh implementedThe refresh token endpoint implementation is well-structured with appropriate validation steps and error handling. It checks token validity, type, and revocation status before generating a new token.
229-231: Double-parsing the refresh tokenThe code calls
validateToken(refreshToken)and latergetAllClaimsFromToken(refreshToken), causing the token to be parsed twice. This is inefficient.- if (jwtUtil.validateToken(refreshToken) == null) { - return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body("Invalid token"); - } - - Claims claims = jwtUtil.getAllClaimsFromToken(refreshToken); + Claims claims = jwtUtil.getAllClaimsFromToken(refreshToken); + if (claims == null) { + return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body("Invalid token"); + }Also applies to: 233-233
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
Outdated
Show resolved
Hide resolved
| refreshToken = jwtUtil.generateRefreshToken(m_User.getUserName(), user.getUserID().toString()); | ||
| logger.info("Generated refresh token: {}", refreshToken); | ||
| String jti = jwtUtil.getJtiFromToken(refreshToken); | ||
| redisTemplate.opsForValue().set( |
There was a problem hiding this comment.
What's the expiry of refresh token? Are we storing it in Redis?
There was a problem hiding this comment.
jwt.access.expiration=900000 (in ms)
jwt.refresh.expiration=604800000(in ms)
can be modified from .properties files
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Component | ||
| public class JwtUtil { | ||
|
|
||
| @Value("${jwt.secret}") | ||
| private String SECRET_KEY; | ||
|
|
||
| private static final long EXPIRATION_TIME = 24L * 60 * 60 * 1000; // 1 day in milliseconds | ||
| @Value("${jwt.access.expiration}") |
There was a problem hiding this comment.
Are these new env variabes. If yes, please add them to application properties
There was a problem hiding this comment.
currently its configured to change from environment specific .properties files. bcz jwt-secret was listed in these files . so i put just below of it to easily find out
|
@ravishanigarapu can you review this once? |
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
Outdated
Show resolved
Hide resolved
…ller.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (1)
256-256: 🛠️ Refactor suggestionConsider implementing refresh token rotation
Security best practice is to issue a new refresh token along with the new JWT for each refresh operation. This is known as refresh token rotation and improves security by limiting the lifetime of any single refresh token.
Map<String, String> tokens = new HashMap<>(); tokens.put("jwtToken", newJwt); + + // Generate and store a new refresh token (token rotation) + String newRefreshToken = jwtUtil.generateRefreshToken(user.getUserName(), userId); + String newJti = jwtUtil.getJtiFromToken(newRefreshToken); + redisTemplate.opsForValue().set( + "refresh:" + newJti, + userId, + jwtUtil.getRefreshTokenExpiration(), + TimeUnit.MILLISECONDS + ); + tokens.put("refreshToken", newRefreshToken); + + // Invalidate the old refresh token + redisTemplate.delete("refresh:" + jti); return ResponseEntity.ok(tokens);
🧹 Nitpick comments (3)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (3)
229-233: Token is parsed twice unnecessarilyThe code calls
validateToken(refreshToken)and thengetAllClaimsFromToken(refreshToken), causing the token to be parsed twice. This is inefficient.- if (jwtUtil.validateToken(refreshToken) == null) { - return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body("Invalid token"); - } - - Claims claims = jwtUtil.getAllClaimsFromToken(refreshToken); + Claims claims = jwtUtil.getAllClaimsFromToken(refreshToken); + if (claims == null) { + return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body("Invalid token"); + }
241-244: Consider adding a token blacklist checkCurrently, you're checking if the token exists in Redis. For added security, consider implementing a token blacklist where explicitly revoked tokens are stored.
// Check revocation using JTI String jti = claims.getId(); - if (!redisTemplate.hasKey("refresh:" + jti)) { + // Check if token exists in Redis and not in blacklist + if (!redisTemplate.hasKey("refresh:" + jti) || redisTemplate.hasKey("blacklist:" + jti)) { return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body("Token revoked"); }
223-223: Update operation summary to match functionThe operation summary should reflect that this endpoint refreshes the JWT token using a refresh token, not just "generates a refresh token."
- @Operation(summary = "generates a refresh token") + @Operation(summary = "Refresh JWT token using a valid refresh token")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java(6 hunks)
🔇 Additional comments (4)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (4)
154-186: Auth logic now handles mobile clients differentlyThe updated authentication logic now distinguishes between mobile and web clients, generating JWT tokens and refresh tokens as needed. This implementation aligns with the PR objectives.
170-170: Good security practice using debug level for token generationUsing
logger.debuginstead oflogger.infofor refresh token generation avoids exposing sensitive information in production logs.
207-212: Appropriately returning tokens in the response for mobile clientsThe code successfully implements the requirement to return JWT and refresh tokens in the response body for mobile clients, enabling mobile apps to handle authentication properly.
222-222: Remove unnecessary @crossorigin annotationThe @crossorigin annotation isn't needed as noted in a previous comment. There's already a ticket to fix this: PSMRI/AMRIT#47
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
Outdated
Show resolved
Hide resolved
| @CrossOrigin() | ||
| @Operation(summary = "generates a refresh token") | ||
| @RequestMapping(value = "/refreshToken", method = RequestMethod.POST, produces = MediaType.APPLICATION_JSON) | ||
| public ResponseEntity<?> refreshToken(@RequestBody Map<String, String> request) { | ||
| String refreshToken = request.get("refreshToken"); | ||
|
|
||
| try { | ||
| if (jwtUtil.validateToken(refreshToken) == null) { | ||
| return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body("Invalid token"); | ||
| } | ||
|
|
||
| Claims claims = jwtUtil.getAllClaimsFromToken(refreshToken); | ||
|
|
||
| // Verify token type | ||
| if (!"refresh".equals(claims.get("token_type", String.class))) { | ||
| return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body("Invalid token type"); | ||
| } | ||
|
|
||
| // Check revocation using JTI | ||
| String jti = claims.getId(); | ||
| if (!redisTemplate.hasKey("refresh:" + jti)) { | ||
| return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body("Token revoked"); | ||
| } | ||
|
|
||
| // Get user details | ||
| String userId = claims.get("userId", String.class); | ||
| User user = iemrAdminUserServiceImpl.getUserById(Long.parseLong(userId)); | ||
|
|
||
| // Generate new tokens | ||
| String newJwt = jwtUtil.generateToken(user.getUserName(), userId); | ||
|
|
||
| Map<String, String> tokens = new HashMap<>(); | ||
| tokens.put("jwtToken", newJwt); | ||
|
|
||
| return ResponseEntity.ok(tokens); | ||
| } catch (ExpiredJwtException ex) { | ||
| return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body("Token expired"); | ||
| } catch (Exception e) { | ||
| logger.error("Refresh failed: ", e); | ||
| return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body("Token refresh failed"); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Implement token revocation on logout
The new refresh token functionality requires corresponding revocation when users log out. Ensure that both the /userLogout and /userForceLogout endpoints invalidate refresh tokens in Redis.
🏁 Script executed:
#!/bin/bash
# Check if the userLogout or userForceLogout methods handle refresh token revocation
# First, search for Redis operations in logout methods
echo "Checking if logout methods revoke refresh tokens in Redis:"
rg -A 5 "userLogout|userForceLogout|forceLogout" --type java | rg "redisTemplate.delete|opsForValue\(\).remove"
# Second, check if JTI extraction happens during logout
echo "Checking if JTI extraction happens during logout:"
rg -A 10 "userLogout|userForceLogout|forceLogout" --type java | rg "getJtiFromToken|getAllClaimsFromToken"Length of output: 458
Revise Token Revocation Handling on Logout Endpoints
The current implementation lacks explicit invalidation of refresh tokens during logout. Neither the /userLogout nor /userForceLogout endpoints appear to perform the necessary Redis operations (e.g., deleting the token entry using its JTI) to revoke the refresh token, which may leave stale tokens active.
Action items:
- Implement Token Revocation: Ensure that both
/userLogoutand/userForceLogoutendpoints retrieve the refresh token's JTI (using, for example,jwtUtil.getAllClaimsFromToken) and subsequently delete the corresponding Redis key (e.g., by invokingredisTemplate.delete("refresh:" + jti)). - Verify Functionality: Add tests to confirm that upon logout, the refresh token is indeed revoked and cannot be used to generate new JWT tokens.
…ller.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (2)
788-802:⚠️ Potential issueImplement refresh token revocation on logout.
The current logout endpoint doesn't invalidate refresh tokens stored in Redis, which could allow continued use of refresh tokens even after logout.
Update the logout endpoint to also revoke refresh tokens:
@RequestMapping(value = "/userLogout", method = RequestMethod.POST, produces = MediaType.APPLICATION_JSON, headers = "Authorization") public String userLogout(HttpServletRequest request) { OutputResponse response = new OutputResponse(); logger.info("userLogout"); try { + // Get user info from the JWT token + String token = request.getHeader("Authorization"); + try { + Claims claims = jwtUtil.getAllClaimsFromToken(token); + String userId = claims.get("userId", String.class); + + // Remove all refresh tokens for this user + Set<String> keys = redisTemplate.keys("refresh:user:" + userId + ":token:*"); + if (keys != null && !keys.isEmpty()) { + redisTemplate.delete(keys); + } + } catch (Exception e) { + logger.warn("Failed to revoke refresh tokens during logout", e); + } + deleteSessionObjectByGettingSessionDetails(request.getHeader("Authorization")); sessionObject.deleteSessionObject(request.getHeader("Authorization")); response.setResponse("Success"); } catch (Exception e) { response.setError(e); } return response.toString(); }
851-864:⚠️ Potential issueImplement refresh token revocation on force logout.
Similar to the regular logout endpoint, the force logout endpoint should also invalidate refresh tokens.
Update the force logout endpoint to revoke refresh tokens:
@RequestMapping(value = "/userForceLogout", method = RequestMethod.POST, produces = MediaType.APPLICATION_JSON, headers = "Authorization") public String userForceLogout( @Param(value = "\"{\\\"userName\\\":\\\"String\\\"}\"") @RequestBody ForceLogoutRequestModel request) { OutputResponse response = new OutputResponse(); try { + // Get user ID for the username + List<User> users = iemrAdminUserServiceImpl.userExitsCheck(request.getUserName()); + if (users != null && users.size() == 1) { + Long userId = users.get(0).getUserID(); + + // Remove all refresh tokens for this user + Set<String> keys = redisTemplate.keys("refresh:user:" + userId + ":token:*"); + if (keys != null && !keys.isEmpty()) { + redisTemplate.delete(keys); + } + } + iemrAdminUserServiceImpl.userForceLogout(request); response.setResponse("Success"); } catch (Exception e) { response.setError(e); } return response.toString(); }
♻️ Duplicate comments (2)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (2)
259-265: 🛠️ Refactor suggestionImplement refresh token rotation for better security.
The current implementation does not rotate refresh tokens, which is a security best practice. When a refresh token is used, a new refresh token should be issued along with the JWT token to limit the lifetime of any single refresh token.
// Generate new tokens String newJwt = jwtUtil.generateToken(user.getUserName(), userId); Map<String, String> tokens = new HashMap<>(); tokens.put("jwtToken", newJwt); + + // Generate and store a new refresh token (token rotation) + String newRefreshToken = jwtUtil.generateRefreshToken(user.getUserName(), userId); + String newJti = jwtUtil.getJtiFromToken(newRefreshToken); + + // Delete old token + String oldRedisKey = String.format("refresh:user:%s:token:%s", userId, jti); + redisTemplate.delete(oldRedisKey); + + // Store new token + String newRedisKey = String.format("refresh:user:%s:token:%s", userId, newJti); + Map<String, String> tokenData = new HashMap<>(); + tokenData.put("userId", userId); + redisTemplate.opsForValue().set( + newRedisKey, + tokenData, + jwtUtil.getRefreshTokenExpiration(), + TimeUnit.MILLISECONDS + ); + tokens.put("refreshToken", newRefreshToken); return ResponseEntity.ok(tokens);
226-245: 🛠️ Refactor suggestionToken validation logic has security gaps.
The current implementation validates tokens correctly but has some room for improvement:
- The token revocation check needs to be updated to match the modified key structure if you implement the earlier suggestion
- The code makes two calls to the JWT utility (validate and getAllClaims), which is inefficient
String refreshToken = request.get("refreshToken"); try { - if (jwtUtil.validateToken(refreshToken) == null) { + Claims claims; + try { + claims = jwtUtil.getAllClaimsFromToken(refreshToken); + } catch (Exception e) { return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body("Invalid token"); + } - Claims claims = jwtUtil.getAllClaimsFromToken(refreshToken); // Verify token type if (!"refresh".equals(claims.get("token_type", String.class))) { return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body("Invalid token type"); } // Check revocation using JTI String jti = claims.getId(); - if (!redisTemplate.hasKey("refresh:" + jti)) { + String redisKey = String.format("refresh:user:%s:token:%s", claims.get("userId", String.class), jti); + if (!redisTemplate.hasKey(redisKey)) { return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body("Token revoked"); }
🧹 Nitpick comments (2)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (2)
168-181: Consider securing the refresh token storage more explicitly.While storing refresh tokens in Redis with expiration is a good practice, additional security measures could be implemented:
- The Redis key contains only the JTI, but consider adding a namespace or prefix that includes the user ID for better isolation
- Consider implementing token binding to the device or client fingerprint
- redisTemplate.opsForValue().set( - "refresh:" + jti, - user.getUserID().toString(), - jwtUtil.getRefreshTokenExpiration(), - TimeUnit.MILLISECONDS - ); + // Store refresh token with user context for better security + String redisKey = String.format("refresh:user:%s:token:%s", user.getUserID(), jti); + Map<String, String> tokenData = new HashMap<>(); + tokenData.put("userId", user.getUserID().toString()); + tokenData.put("userAgent", userAgent); // Bind token to device + redisTemplate.opsForValue().set( + redisKey, + tokenData, + jwtUtil.getRefreshTokenExpiration(), + TimeUnit.MILLISECONDS + );
222-225: Remove unnecessary CrossOrigin annotation.The CrossOrigin annotation without any parameters doesn't provide additional security, and as noted in a previous review, there's a ticket (#47) to fix this across the application.
- @CrossOrigin() @Operation(summary = "generates a refresh token") @RequestMapping(value = "/refreshToken", method = RequestMethod.POST, produces = MediaType.APPLICATION_JSON) public ResponseEntity<?> refreshToken(@RequestBody Map<String, String> request) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java(6 hunks)
🔇 Additional comments (6)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (6)
24-30: Imports have been updated to support new functionality.The imports now include necessary JWT and utility classes to support the new token handling functionality for mobile devices.
154-166: Good implementation of device detection for authentication flow.The authentication flow now differentiates between mobile and web clients using the User-Agent header, which is a good approach for providing platform-specific authentication mechanisms.
207-211: Appropriate token response for mobile clients.The implementation now sends both JWT and refresh tokens in the response for mobile clients, which is appropriate for mobile applications that typically use local storage for token persistence.
246-258: Good user validation implementation.The validation of user existence and status is a critical security check that prevents token misuse if a user account is deactivated or deleted.
266-271: Good error handling for token-related exceptions.The error handling is thorough, with specific responses for expired tokens and general exceptions, which helps clients understand and respond appropriately to different failure scenarios.
182-185:✅ Verification successful
Review Redis usage for user storage.
The code stores the user in Redis with a 30-minute TTL. This is good for caching purposes, but ensure this complements rather than duplicates session management.
Verify if this user storage is for performance optimization or for session tracking:
🏁 Script executed:
#!/bin/bash # Check how the Redis user storage is used elsewhere in the codebase echo "Searching for references to the Redis key pattern 'user_'" rg "user_.*" --type java -A 3 -B 3Length of output: 5086
Redis Caching Clarification
The search output confirms that the Redis key
"user_"is consistently used for caching user objects, as seen in bothIEMRAdminController.javaandJwtAuthenticationUtil.java. This caching mechanism avoids unnecessary database calls and is independent of the session management (evidenced by the separate JWT token handling via cookies). Please ensure that the 30-minute TTL aligns with your overall cache and session strategies.
…ss.expiration and jwt.refresh.expiration in common_example.properties file.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (1)
790-801:⚠️ Potential issueImplement refresh token revocation on logout
The current logout implementation is missing revocation of refresh tokens.
When users log out, their refresh tokens should be invalidated to prevent unauthorized access:
@RequestMapping(value = "/userLogout", method = RequestMethod.POST, produces = MediaType.APPLICATION_JSON, headers = "Authorization") public String userLogout(HttpServletRequest request) { OutputResponse response = new OutputResponse(); logger.info("userLogout"); try { + // Get the refresh token from the Authorization header if it exists + String authHeader = request.getHeader("Authorization"); + if (authHeader != null && authHeader.startsWith("Bearer ")) { + String refreshToken = authHeader.substring(7); + try { + Claims claims = jwtUtil.getAllClaimsFromToken(refreshToken); + String jti = claims.getId(); + redisTemplate.delete("refresh:" + jti); + logger.debug("Refresh token revoked during logout"); + } catch (Exception e) { + logger.debug("Error revoking refresh token: " + e.getMessage()); + } + } + deleteSessionObjectByGettingSessionDetails(request.getHeader("Authorization")); sessionObject.deleteSessionObject(request.getHeader("Authorization")); response.setResponse("Success"); } catch (Exception e) { response.setError(e); } return response.toString(); }Similar changes should be made to
userForceLogoutandforceLogoutmethods.
♻️ Duplicate comments (1)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (1)
164-180:⚠️ Potential issueMobile detection and token handling looks good, but avoid logging sensitive information
The mobile device detection and token handling logic is well-implemented, with proper Redis storage for refresh tokens.
Avoid logging sensitive information like refresh tokens:
- logger.debug("Refresh token generated successfully for user: {}", user.getUserName()); + logger.debug("Refresh token generated successfully for user: {}", user.getUserName());Consider separating the mobile device detection into a helper method for better testability.
🧹 Nitpick comments (4)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (2)
222-271: Implement refresh token rotation for improved securityThe refresh token endpoint works correctly but lacks token rotation, which is a security best practice.
Token rotation reduces the risk from leaked refresh tokens. Instead of just generating a new access token, also generate a new refresh token and invalidate the old one:
// Generate new tokens String newJwt = jwtUtil.generateToken(user.getUserName(), userId); Map<String, String> tokens = new HashMap<>(); tokens.put("jwtToken", newJwt); + + // Generate new refresh token (token rotation) + String newRefreshToken = jwtUtil.generateRefreshToken(user.getUserName(), userId); + String newJti = jwtUtil.getJtiFromToken(newRefreshToken); + + // Store new refresh token + redisTemplate.opsForValue().set( + "refresh:" + newJti, + userId, + jwtUtil.getRefreshTokenExpiration(), + TimeUnit.MILLISECONDS + ); + + // Delete old refresh token + String oldJti = claims.getId(); + redisTemplate.delete("refresh:" + oldJti); + + tokens.put("refreshToken", newRefreshToken);
223-224: Add proper Cross-Origin configuration for the refresh endpointThe refresh token endpoint is missing Cross-Origin annotations that are present on other endpoints.
For consistency with other API endpoints, add appropriate Cross-Origin configuration:
@Operation(summary = "generating a auth token with the refreshToken.") +@CrossOrigin() @RequestMapping(value = "/refreshToken", method = RequestMethod.POST, produces = MediaType.APPLICATION_JSON) public ResponseEntity<?> refreshToken(@RequestBody Map<String, String> request) {src/main/java/com/iemr/common/utils/JwtUtil.java (2)
59-73: Improve exception handling in validateToken methodWhile the exception handling is more specific now, it would benefit from proper logging.
Add logging for different exception types to help with troubleshooting:
public Claims validateToken(String token) { try { return Jwts.parser() .verifyWith(getSigningKey()) .build() .parseSignedClaims(token) .getPayload(); } catch (ExpiredJwtException ex) { - // Handle expired token specifically if needed + logger.debug("JWT token expired", ex); } catch (UnsupportedJwtException | MalformedJwtException | SignatureException | IllegalArgumentException ex) { - // Log specific error types + logger.warn("Invalid JWT token: {}", ex.getMessage(), ex); } return null; }
75-78: Add null check to getClaimFromToken methodThe
getClaimFromTokenmethod might throw a NullPointerException ifgetAllClaimsFromTokenreturns null.Add a null check to improve robustness:
public <T> T getClaimFromToken(String token, Function<Claims, T> claimsResolver) { final Claims claims = getAllClaimsFromToken(token); + if (claims == null) { + return null; + } return claimsResolver.apply(claims); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/environment/common_example.properties(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/resources/application.properties(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/main/java/com/iemr/common/utils/JwtUtil.java (2)
Learnt from: indraniBan
PR: PSMRI/Common-API#138
File: src/main/environment/common_uat.properties:0-0
Timestamp: 2025-03-31T13:57:03.202Z
Learning: In the Common-API project, JWT secret configuration should use environment variables (e.g., `[email protected]_SECRET_KEY@`) across all environments for better security.
Learnt from: indraniBan
PR: PSMRI/Common-API#138
File: src/main/environment/common_test.properties:0-0
Timestamp: 2025-03-31T13:56:54.733Z
Learning: In Common-API, JWT secrets should be configured using placeholders (e.g., `<Enter_Your_Secret_Key>`) in properties files, similar to other sensitive credentials, to prevent accidental commit of actual secrets.
🔇 Additional comments (3)
src/main/java/com/iemr/common/utils/JwtUtil.java (3)
26-31: Configurable token expiration times enhance flexibilityThe use of configurable properties for token expiration times is a good practice, allowing for environment-specific customization.
39-45: Well-structured token generation methodsThe refactoring to use a common
buildToken()method reduces code duplication and improves maintainability.
88-103: Well-documented getRefreshTokenExpiration methodThe documentation for this method is excellent, explaining the rationale, security considerations, and configurability.
| jwt.secret= | ||
| jwt.access.expiration=900000 | ||
| jwt.refresh.expiration=604800000 |
There was a problem hiding this comment.
JWT secret key configuration needs attention
The JWT expiration times look appropriate (15 minutes for access tokens, 7 days for refresh tokens). However, the jwt.secret property is empty, which is concerning for security.
Based on project practices, JWT secrets should be configured using either:
- Environment variable placeholders:
[email protected]_SECRET_KEY@ - Clear placeholder text:
jwt.secret=<Enter_Your_Secret_Key>
This helps prevent accidental commits of actual secrets while making it clear to developers that they need to set this value.
| @@ -169,6 +169,8 @@ eausadhaAuthorization=<Enter eausadha authorization key> | |||
| spring.main.allow-bean-definition-overriding=true | |||
| spring.main.allow-circular-references=true | |||
| jwt.secret= | |||
| jwt.access.expiration=900000 | |||
There was a problem hiding this comment.
@kevalkanp1011 please add this to add common_* files in the same folder.
There was a problem hiding this comment.
@kevalkanp1011 this isn't done. Also, as a good practice, allow the reviewer to resolve comments.
There was a problem hiding this comment.
it is done . okay I will not resolve comments myself.
…ller.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (1)
222-285: 🛠️ Refactor suggestionImplement token revocation on logout
The new refresh token endpoint has been implemented correctly, but the existing logout endpoints don't revoke refresh tokens.
Please update the logout endpoints to invalidate refresh tokens:
@RequestMapping(value = "/userLogout", method = RequestMethod.POST, produces = MediaType.APPLICATION_JSON, headers = "Authorization") public String userLogout(HttpServletRequest request) { OutputResponse response = new OutputResponse(); logger.info("userLogout"); try { + // Extract refresh token from request if available + Cookie[] cookies = request.getCookies(); + if (cookies != null) { + for (Cookie cookie : cookie) { + if ("refreshToken".equals(cookie.getName())) { + String refreshToken = cookie.getValue(); + Claims claims = jwtUtil.getAllClaimsFromToken(refreshToken); + if (claims != null) { + String jti = claims.getId(); + redisTemplate.delete("refresh:" + jti); + logger.debug("Refresh token revoked for user logout"); + } + } + } + } deleteSessionObjectByGettingSessionDetails(request.getHeader("Authorization")); sessionObject.deleteSessionObject(request.getHeader("Authorization")); response.setResponse("Success"); } catch (Exception e) { response.setError(e); } return response.toString(); }Similar changes should be made to the
userForceLogoutmethod.
🧹 Nitpick comments (1)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (1)
168-177: Document refresh token expiration strategyThe code uses
jwtUtil.getRefreshTokenExpiration()for setting the expiration time, but there's limited documentation about this value.Consider adding a comment that explains:
- The chosen refresh token lifespan
- Security considerations for this expiration period
- Whether this duration is configurable
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java(6 hunks)
🔇 Additional comments (6)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (6)
154-211: JWT authentication flow now supports mobile devicesThe updated authentication flow now detects device type and handles token delivery appropriately:
- For mobile clients: Returns JWT and refresh tokens in the response body
- For web clients: Sets the JWT token in a cookie
This is a good improvement that supports both types of clients with their preferred authentication mechanisms.
246-257: Good user validation before token refreshThe implementation now properly validates that the user exists and is active before generating a new token. This is a security best practice that prevents token refresh for deleted or deactivated accounts.
267-276: Excellent implementation of refresh token rotationThe implementation now includes refresh token rotation, which is a security best practice. Each time a token is refreshed:
- A new refresh token is generated
- The new token is stored in Redis with appropriate expiration
- Both the new JWT and refresh tokens are returned to the client
This approach improves security by limiting the lifetime of any single refresh token.
279-284: Proper error handling for token refresh operationsThe implementation provides clear, specific error messages for different failure scenarios:
- Invalid tokens
- Expired tokens
- Revoked tokens
- User not found/inactive
This approach helps with debugging and provides appropriate feedback to clients.
239-243: Good token revocation check using JTIThe implementation uses the JWT ID (JTI) to check if a refresh token has been revoked by looking for its presence in Redis. This is a common and effective approach for managing token revocation.
170-170: Good security practice: reduced logging of sensitive informationPrevious reviews noted that logging refresh tokens poses a security risk. The implementation now correctly logs only the fact that a token was generated, not the token itself.
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/resources/application.properties (1)
182-184: JWT Token Configuration Properties AddedThe new properties for JWT token management have been correctly introduced:
jwt.access.expiration=900000sets the access token lifetime to 15 minutes.jwt.refresh.expiration=604800000sets the refresh token lifetime to one week.These additions align with the PR objectives of enhancing JWT handling and support secure session management across mobile and web platforms. For improved clarity, consider updating the comment from
#Jwt Token configurationto# JWT Token configurationto maintain consistency in capitalization.Proposed diff for comment clarity:
| .getPayload(); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
This block of comment is unnecessary.
|
@kevalkanp1011 Please check some checks are failed |
|
@ravishanigarapu fixed |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (2)
800-812: 🛠️ Refactor suggestionImplement token revocation on logout.
The userLogout method doesn't invalidate refresh tokens in Redis, which could leave tokens valid after logout.
@RequestMapping(value = "/userLogout", method = RequestMethod.POST, produces = MediaType.APPLICATION_JSON, headers = "Authorization") public String userLogout(HttpServletRequest request) { OutputResponse response = new OutputResponse(); logger.info("userLogout"); try { + // Extract JWT from Authorization header + String token = request.getHeader("Authorization"); + if (token != null && token.startsWith("Bearer ")) { + token = token.substring(7); + } + + // Get claims and check if it's a refresh token in the Authorization header + try { + Claims claims = jwtUtil.getAllClaimsFromToken(token); + if (claims != null && "refresh".equals(claims.get("token_type", String.class))) { + // Invalidate refresh token in Redis + String jti = claims.getId(); + redisTemplate.delete("refresh:" + jti); + } + } catch (Exception e) { + logger.warn("Could not invalidate refresh token during logout", e); + } + deleteSessionObjectByGettingSessionDetails(request.getHeader("Authorization")); sessionObject.deleteSessionObject(request.getHeader("Authorization")); response.setResponse("Success"); } catch (Exception e) { response.setError(e); } return response.toString(); }
863-874: 🛠️ Refactor suggestionImplement token revocation on force logout.
Similar to the regular logout endpoint, the userForceLogout method should also invalidate refresh tokens.
@RequestMapping(value = "/userForceLogout", method = RequestMethod.POST, produces = MediaType.APPLICATION_JSON, headers = "Authorization") public String userForceLogout( @Param(value = "\"{\\\"userName\\\":\\\"String\\\"}\"") @RequestBody ForceLogoutRequestModel request) { OutputResponse response = new OutputResponse(); try { + // Find any refresh tokens associated with this user in Redis and invalidate them + String pattern = "refresh:*"; + Set<String> keys = redisTemplate.keys(pattern); + if (keys != null) { + for (String key : keys) { + Object userId = redisTemplate.opsForValue().get(key); + if (userId != null) { + User user = iemrAdminUserServiceImpl.getUserById(Long.parseLong(userId.toString())); + if (user != null && user.getUserName().equals(request.getUserName())) { + redisTemplate.delete(key); + logger.debug("Invalidated refresh token for forced logout user: {}", request.getUserName()); + } + } + } + } iemrAdminUserServiceImpl.userForceLogout(request); response.setResponse("Success"); } catch (Exception e) { response.setError(e); } return response.toString(); }
🧹 Nitpick comments (1)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (1)
232-232: Consider unifying token parsing and validation.The code calls
validateToken(refreshToken)and latergetAllClaimsFromToken(refreshToken), causing two parsing operations.- if (jwtUtil.validateToken(refreshToken) == null) { - return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body("Invalid token"); - } - - Claims claims = jwtUtil.getAllClaimsFromToken(refreshToken); + Claims claims = jwtUtil.getAllClaimsFromToken(refreshToken); + if (claims == null) { + return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body("Invalid token"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pom.xml(1 hunks)src/main/java/com/iemr/common/controller/users/IEMRAdminController.java(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- pom.xml
🔇 Additional comments (12)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (12)
24-29: Good imports organization adding UserAgentUtil.The introduction of the UserAgentUtil class and necessary JWT-related imports support the new mobile device detection and token handling functionality.
154-157: Clear initialization of token-related variables.Good practice to initialize these variables at the beginning of the method to make the token handling logic more readable.
164-166: Proper mobile device detection implementation.The code correctly extracts the User-Agent header and uses the UserAgentUtil to determine if the request is from a mobile device.
168-180: Well-implemented conditional token handling for different client types.The conditional logic correctly handles JWT distribution based on client type:
- For mobile clients: Generates refresh tokens and stores them in Redis
- For web clients: Uses cookies for the JWT token
This approach enhances security while maintaining compatibility with both client types.
170-170: Security improvement in token logging.Good use of debug level for sensitive token information, which addresses a previous security concern about logging refresh tokens.
207-211: Correct token inclusion in response for mobile clients.The implementation properly includes both the JWT and refresh tokens in the response body for mobile clients, which is necessary since mobile clients cannot easily use cookies.
222-282: Well-implemented refresh token endpoint with proper validation and security checks.The refresh token endpoint implementation is robust with:
- Token validation
- Token type verification
- Revocation checking
- User existence and status validation
- Token rotation (generating new refresh token)
- Proper error handling with appropriate HTTP status codes
This implementation follows security best practices for refresh token handling.
240-243: Good implementation of token revocation checking.The code correctly uses the JTI (JWT ID) to check if the token has been revoked by checking for the existence of the corresponding key in Redis.
250-257: Thorough user validation before generating new tokens.The implementation properly validates that:
- The user exists
- The user's account is active
This prevents token refresh for deleted or deactivated accounts.
264-273: Proper implementation of refresh token rotation.Token rotation is correctly implemented:
- A new refresh token is generated
- The new JTI is extracted
- The new token is stored in Redis with proper expiration
- The new refresh token is included in the response
This enhances security by limiting the lifetime of each refresh token.
276-280: Good error handling with specific error messages.The implementation includes proper exception handling:
- Specific handling for expired tokens
- General exception handling with appropriate logging
- Returning appropriate HTTP status codes for different error conditions
171-177:❓ Verification inconclusive
Refresh token storage using Redis with proper expiration.
The implementation correctly:
- Extracts the JTI (JWT ID) from the refresh token
- Uses it as part of the Redis key for storage
- Sets an appropriate expiration time
However, revocation during logout should also be implemented.
🏁 Script executed:
#!/bin/bash # Check if logout methods handle refresh token revocation echo "Checking if userLogout method includes token revocation:" rg -A 10 "userLogout" src/main/java/com/iemr/common/controller/users/IEMRAdminController.java | rg "redisTemplate.delete|opsForValue\(\).remove" echo "Checking if userForceLogout method includes token revocation:" rg -A 10 "userForceLogout" src/main/java/com/iemr/common/controller/users/IEMRAdminController.java | rg "redisTemplate.delete|opsForValue\(\).remove"Length of output: 550
Action Required: Verify and Implement Token Revocation on Logout
The refresh token storage is implemented correctly by:
- Extracting the JTI (JWT ID) from the refresh token.
- Constructing the Redis key with the JTI.
- Setting an appropriate expiration.
However, using the available search, no explicit token revocation (e.g., via
redisTemplate.deleteor similar removal operations) was detected in theuserLogoutanduserForceLogoutmethods. This suggests that the refresh token is not being revoked during logout, which could lead to potential security vulnerabilities if tokens remain valid after a user logs out.Next Steps:
- Verify Manually: Please double-check if token revocation is implemented elsewhere or intentionally omitted.
- If missing, add logic in your logout methods to delete the corresponding refresh token from Redis (for example, by invoking
redisTemplate.delete("refresh:" + jti)).
| redisTemplate.opsForValue().set( | ||
| "refresh:" + jti, | ||
| user.getUserID().toString(), | ||
| jwtUtil.getRefreshTokenExpiration(), | ||
| TimeUnit.MILLISECONDS | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Document the refresh token expiration strategy.
The code uses jwtUtil.getRefreshTokenExpiration() without any inline documentation explaining its value or the reasoning behind it.
🌐 Web query:
What is a typical expiration time for JWT refresh tokens?
💡 Result:
JWT refresh token expiration times vary based on security requirements and system design, but common configurations range from 7 days to 90 days, with some systems allowing configurations up to 1 year. Here's a breakdown of typical practices:
Common Expiration Periods
- **Short
Citations:
- 1: https://fusionauth.io/articles/tokens/revoking-jwts
- 2: https://www.descope.com/blog/post/refresh-token-rotation
- 3: https://devforum.zoom.us/t/how-can-i-know-when-my-jwt-token-will-expire/83881
- 4: https://auth0.com/docs/secure/tokens/refresh-tokens/configure-refresh-token-expiration
- 5: https://www.descope.com/learn/post/refresh-token
- 6: https://community.auth0.com/t/refresh-token-absolute-expiration-clarification-needed/107945
- 7: https://www.loginradius.com/blog/identity/refresh-tokens-jwt-interaction/
- 8: https://auth0.com/blog/refresh-tokens-what-are-they-and-when-to-use-them/
- 9: https://learn.microsoft.com/en-us/answers/questions/845791/jwt-token-expiration-and-authorization
- 10: https://auth0.com/docs/secure/tokens/token-best-practices
Document JWT Refresh Token Expiration Strategy Inline
At src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (lines 172-177), please add inline documentation explaining the refresh token expiration strategy. For example, clarify that jwtUtil.getRefreshTokenExpiration() returns the token's lifetime in milliseconds and mention that typical refresh token expirations usually range from 7 to 90 days (or more, depending on security requirements). This added context will help future maintainers understand the rationale behind selecting this expiration period.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Examining JwtUtil class for documentation about refresh token expiration
cat src/main/java/com/iemr/common/utils/JwtUtil.java | grep -A 15 "getRefreshTokenExpiration"
# Also check application.properties for any expiration configuration
cat src/main/resources/application.properties | grep -i "jwt\|expiration\|token"
Length of output: 581
@kevalkanp1011 I see that you've configured the refresh token expiration in application.properties (jwt.refresh.expiration=604800000, which is 7 days). While the configuration value exists, adding a brief comment in the code where it's used would help future developers understand your token strategy without having to search through configuration files. This is especially helpful for explaining why you chose 7 days and any security considerations that went into that decision.
🧬 Code Graph Analysis Results
Relevant Code Snippets
1. UserAgentUtil.java
Filename: src/main/java/com/iemr/common/utils/UserAgentUtil.java
Line Range: 3-9
public class UserAgentUtil {
public static boolean isMobileDevice(String userAgent) {
if (userAgent == null) return false;
String lowerUA = userAgent.toLowerCase();
return lowerUA.contains("mobile") || lowerUA.contains("android") || lowerUA.contains("iphone");
}
}Description: This utility class contains a method to check if the user agent string corresponds to a mobile device. It returns true if the user agent indicates a mobile device, otherwise false.
2. IEMRAdminController.java (userAuthenticate method)
Filename: src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
Line Range: 70-130
@CrossOrigin()
@Operation(summary = "User authentication")
@RequestMapping(value = "/userAuthenticate", method = RequestMethod.POST, produces = MediaType.APPLICATION_JSON)
public String userAuthenticate(
@Param(value = "\"{\\\"userName\\\":\\\"String\\\",\\\"password\\\":\\\"String\\\"}\"") @RequestBody LoginRequestModel m_User,
HttpServletRequest request, HttpServletResponse httpResponse) {
OutputResponse response = new OutputResponse();
logger.info("userAuthenticate request - " + m_User + " " + m_User.getUserName() + " " + m_User.getPassword());
try {
String decryptPassword = aesUtil.decrypt("Piramal12Piramal", m_User.getPassword());
logger.info("decryptPassword : " + decryptPassword);
List<User> mUser = iemrAdminUserServiceImpl.userAuthenticate(m_User.getUserName(), decryptPassword);
// Additional logic for authentication...
} catch (Exception e) {
logger.error("userAuthenticate failed with error " + e.getMessage(), e);
response.setError(e);
}
logger.info("userAuthenticate response " + response.toString());
return response.toString();
}Description: This method handles user authentication. It decrypts the provided password, checks the user credentials, and logs the authentication process. It returns a response indicating the success or failure of the authentication.
3. IEMRAdminController.java (userAuthenticateNew method)
Filename: src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
Line Range: 50-68
@CrossOrigin()
@Operation(summary = "New user authentication")
@RequestMapping(value = "/userAuthenticateNew", method = RequestMethod.POST, produces = MediaType.APPLICATION_JSON)
public String userAuthenticateNew(
@Param(value = "\"{\\\"userName\\\":\\\"String\\\",\\\"password\\\":\\\"String\\\"}\"") @RequestBody String jsonRequest) {
OutputResponse response = new OutputResponse();
try {
User m_user = inputMapper.gson().fromJson(jsonRequest, User.class);
response.setResponse("hello.....");
} catch (Exception e) {
response.setError(e);
}
return response.toString();
}Description: This method is designed for new user authentication. It parses the incoming JSON request to extract user credentials and sets a response. The implementation is basic and primarily serves as a placeholder.
4. IEMRAdminController.java (logOutUserFromConcurrentSession method)
Filename: src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
Line Range: 140-180
@CrossOrigin()
@Operation(summary = "Log out user from concurrent session")
@RequestMapping(value = "/logOutUserFromConcurrentSession", method = RequestMethod.POST, produces = MediaType.APPLICATION_JSON)
public String logOutUserFromConcurrentSession(
@Param(value = "\"{\\\"userName\\\":\\\"String\\\"}\"") @RequestBody LoginRequestModel m_User,
HttpServletRequest request) {
OutputResponse response = new OutputResponse();
logger.info("logOutUserFromConcurrentSession request - " + m_User);
try {
if (m_User != null && m_User.getUserName() != null) {
List<User> mUsers = iemrAdminUserServiceImpl.userExitsCheck(m_User.getUserName());
// Additional logic for logging out...
}
} catch (Exception e) {
logger.error("logOutUserFromConcurrentSession failed with error " + e.getMessage(), e);
response.setError(e);
}
logger.info("logOutUserFromConcurrentSession response " + response.toString());
return response.toString();
}Description: This method logs out a user from a concurrent session. It checks if the user exists and performs the necessary actions to log them out, handling any exceptions that may occur during the process.
5. IEMRAdminController.java (refreshToken method)
Filename: src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
Line Range: 130-140
@Operation(summary = "generating a auth token with the refreshToken.")
@RequestMapping(value = "/refreshToken", method = RequestMethod.POST, produces = MediaType.APPLICATION_JSON)
public ResponseEntity<?> refreshToken(@RequestBody Map<String, String> request) {
String refreshToken = request.get("refreshToken");
// Logic for refreshing token...
}Description: This method handles the generation of a new authentication token using a provided refresh token. It validates the refresh token and generates a new JWT if valid.
These snippets provide context on user authentication and session management, which may relate to the user's comment about something being "already added."
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/main/java/com/iemr/common/utils/JwtUtil.java (1)
89-91: Consider adding documentation for the refresh token expirationThe getter method for refresh token expiration is straightforward, but adding Javadoc would help explain its purpose and expected usage.
+ /** + * Returns the configured refresh token expiration time in milliseconds. + * This value is used when generating refresh tokens. + * + * @return The refresh token expiration time in milliseconds + */ public long getRefreshTokenExpiration() { return REFRESH_EXPIRATION_TIME; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/common/utils/JwtUtil.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/main/java/com/iemr/common/utils/JwtUtil.java (2)
Learnt from: indraniBan
PR: PSMRI/Common-API#138
File: src/main/environment/common_uat.properties:0-0
Timestamp: 2025-04-10T05:07:03.450Z
Learning: In the Common-API project, JWT secret configuration should use environment variables (e.g., `[email protected]_SECRET_KEY@`) across all environments for better security.
Learnt from: indraniBan
PR: PSMRI/Common-API#138
File: src/main/environment/common_test.properties:0-0
Timestamp: 2025-04-10T05:07:00.876Z
Learning: In Common-API, JWT secrets should be configured using placeholders (e.g., `<Enter_Your_Secret_Key>`) in properties files, similar to other sensitive credentials, to prevent accidental commit of actual secrets.
🔇 Additional comments (4)
src/main/java/com/iemr/common/utils/JwtUtil.java (4)
4-4: Good improvements to imports for better exception handlingThe added imports enhance error handling capabilities with specific JWT exceptions and add UUID support for generating unique token identifiers. These are good improvements that will help with more detailed error reporting and debugging.
Also applies to: 11-14, 16-18
32-37: Improved signing key handling with proper type safetyThe method signature change to return
SecretKeyinstead of a genericKeyimproves type safety. The null check with a clear exception message is also a good practice.
39-41: Well-designed token generation with clear separation of concernsThe refactoring of token generation into
buildTokenwith specialized methods for access and refresh tokens is excellent. The inclusion of token type, UUID-based ID, and configurable expiration times increases security and flexibility.Also applies to: 43-45, 47-57
26-30:❓ Verification inconclusive
Ensure environment-specific properties are properly configured
The new JWT expiration time configuration properties are well-structured, separating access and refresh token lifetimes.
Based on previous discussions and project standards, please ensure these values are properly configured in all environment-specific property files. Consider using environment variables for these sensitive configurations similar to how the JWT secret should be handled.
🏁 Script executed:
#!/bin/bash # Check if the new properties are configured in all environment files echo "Checking environment properties files for JWT expiration settings:" find src/main/environment -name "*.properties" -type f -exec grep -l "jwt.access.expiration\|jwt.refresh.expiration" {} \; | sortLength of output: 277
Action Required: Verify JWT Properties in Environment Files
The new JWT expiration time properties for access and refresh tokens look well-structured. However, the environment check script did not locate any environment-specific property files containing the keys (
jwt.access.expirationandjwt.refresh.expiration). Please verify that these values are defined consistently across all environment property files (typically within thesrc/main/environmentdirectory). Additionally, consider using environment variables for these sensitive configurations, similar to the JWT secret configuration.
- Confirm and update all relevant property files to include the
jwt.access.expirationandjwt.refresh.expirationkeys.- Consider aligning with best practices by potentially using environment variables for sensitive settings.
| public <T> T getClaimFromToken(String token, Function<Claims, T> claimsResolver) { | ||
| final Claims claims = getAllClaimsFromToken(token); | ||
| return claimsResolver.apply(claims); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add exception handling to token claim extraction methods
The claim extraction methods are well-structured, but they lack exception handling.
When these methods are called with invalid tokens, they will throw uncaught exceptions that could crash the application. Add proper exception handling:
public Claims getAllClaimsFromToken(String token) {
+ try {
return Jwts.parser()
.verifyWith(getSigningKey())
.build()
.parseSignedClaims(token)
.getPayload();
+ } catch (ExpiredJwtException | UnsupportedJwtException | MalformedJwtException |
+ SignatureException | IllegalArgumentException ex) {
+ // Log the specific error
+ logger.error("Error extracting claims from token: {}", ex.getMessage());
+ return null;
+ }
}The same exception handling should be applied to getClaimFromToken or it should handle null returns from getAllClaimsFromToken.
Also applies to: 80-86
| public String getJtiFromToken(String token) { | ||
| return getAllClaimsFromToken(token).getId(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add exception handling to token extraction helper methods
The helper methods for extracting specific claims from tokens should include exception handling.
These methods directly call getAllClaimsFromToken() which currently doesn't handle exceptions. If a method is called with an invalid token, it will propagate exceptions to the caller. Add try-catch blocks or ensure getAllClaimsFromToken() handles exceptions properly.
public String getJtiFromToken(String token) {
+ Claims claims = getAllClaimsFromToken(token);
+ return claims != null ? claims.getId() : null;
- return getAllClaimsFromToken(token).getId();
}
public String getUsernameFromToken(String token) {
+ Claims claims = getAllClaimsFromToken(token);
+ return claims != null ? claims.getSubject() : null;
- return getAllClaimsFromToken(token).getSubject();
}Also applies to: 98-100
| public Claims validateToken(String token) { | ||
| try { | ||
| // Use the JwtParserBuilder correctly in version 0.12.6 | ||
| return Jwts.parser() // Correct method in 0.12.6 to get JwtParserBuilder | ||
| .setSigningKey(getSigningKey()) // Set the signing key | ||
| .build() // Build the JwtParser | ||
| .parseClaimsJws(token) // Parse and validate the token | ||
| .getBody(); | ||
| } catch (Exception e) { | ||
| return null; // Handle token parsing/validation errors | ||
| return Jwts.parser() | ||
| .verifyWith(getSigningKey()) | ||
| .build() | ||
| .parseSignedClaims(token) | ||
| .getPayload(); | ||
|
|
||
| } catch (ExpiredJwtException ex) { | ||
| // Handle expired token specifically if needed | ||
| } catch (UnsupportedJwtException | MalformedJwtException | SignatureException | IllegalArgumentException ex) { | ||
| // Log specific error types | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve exception handling in validateToken method
While the updated validation logic using the new JWT API is good, the exception handling needs improvement.
The current implementation has empty catch blocks that silently ignore errors. This will make debugging difficult and hide potential security issues.
public Claims validateToken(String token) {
try {
return Jwts.parser()
.verifyWith(getSigningKey())
.build()
.parseSignedClaims(token)
.getPayload();
} catch (ExpiredJwtException ex) {
- // Handle expired token specifically if needed
+ // Log the expired token with specific error code
+ logger.warn("JWT token has expired: {}", ex.getMessage());
} catch (UnsupportedJwtException | MalformedJwtException | SignatureException | IllegalArgumentException ex) {
- // Log specific error types
+ // Log the specific error type for security monitoring
+ logger.error("JWT validation error: {} - {}", ex.getClass().getName(), ex.getMessage());
}
return null;
}Consider adding a logging mechanism (e.g., SLF4J) to record these exceptions for security monitoring and debugging purposes.
📝 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) { | |
| try { | |
| // Use the JwtParserBuilder correctly in version 0.12.6 | |
| return Jwts.parser() // Correct method in 0.12.6 to get JwtParserBuilder | |
| .setSigningKey(getSigningKey()) // Set the signing key | |
| .build() // Build the JwtParser | |
| .parseClaimsJws(token) // Parse and validate the token | |
| .getBody(); | |
| } catch (Exception e) { | |
| return null; // Handle token parsing/validation errors | |
| return Jwts.parser() | |
| .verifyWith(getSigningKey()) | |
| .build() | |
| .parseSignedClaims(token) | |
| .getPayload(); | |
| } catch (ExpiredJwtException ex) { | |
| // Handle expired token specifically if needed | |
| } catch (UnsupportedJwtException | MalformedJwtException | SignatureException | IllegalArgumentException ex) { | |
| // Log specific error types | |
| } | |
| return null; | |
| } | |
| public Claims validateToken(String token) { | |
| try { | |
| return Jwts.parser() | |
| .verifyWith(getSigningKey()) | |
| .build() | |
| .parseSignedClaims(token) | |
| .getPayload(); | |
| } catch (ExpiredJwtException ex) { | |
| // Log the expired token with specific error code | |
| logger.warn("JWT token has expired: {}", ex.getMessage()); | |
| } catch (UnsupportedJwtException | MalformedJwtException | SignatureException | IllegalArgumentException ex) { | |
| // Log the specific error type for security monitoring | |
| logger.error("JWT validation error: {} - {}", ex.getClass().getName(), ex.getMessage()); | |
| } | |
| return null; | |
| } |
Enhance JWT Handling: Return Token in Response for Mobile, Add Secure Refresh API, and Update Middleware
Description
Please provide a summary of the change and the motivation behind it. Include relevant context and details.
This PR improves JWT authentication handling by implementing the following changes:
These changes ensure better authentication flexibility across web and mobile platforms while improving security.
✅ Type of Change
ℹ️ Additional Information
The changes were tested using:
Summary by CodeRabbit
New Features
Refactor