refresh token unauthorize issue #296
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe refreshToken endpoint now allows users with status Active or New to refresh tokens. The authentication filter was updated to skip authentication for /user/refreshToken, similar to /user/logOutUserFromConcurrentSession. No public API signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant F as JwtUserIdValidationFilter
participant A as IEMRAdminController
participant U as UserService
participant T as TokenService
rect rgba(200,220,255,0.25)
note over F: Updated skip condition
C->>F: HTTP POST /user/refreshToken
alt Path is /user/refreshToken
F-->>C: Skip auth (proceed)
else Other paths
F->>F: Validate JWT
end
end
C->>A: refreshToken request
A->>U: Load user by token/userId
U-->>A: User(status)
alt status in {Active, New}
A->>T: Generate new token
T-->>A: Token
A-->>C: 200 OK (token)
else
A-->>C: 401 Unauthorized
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (1)
270-276: Refresh token rotation is incomplete: the used refresh token (old JTI) is not revoked.Without deleting the used JTI, a stolen refresh token can be reused until expiry. Revoke it upon successful rotation.
Apply this diff:
String jti = claims.getId(); if (!redisTemplate.hasKey("refresh:" + jti)) { logger.warn("Token validation failed: refresh token is revoked or not found in store."); return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body("Unauthorized."); } + // Rotate: revoke the used refresh token (one-time use) + redisTemplate.delete("refresh:" + jti); @@ String newRefreshToken = jwtUtil.generateRefreshToken(user.getUserName(), userId); String newJti = jwtUtil.getJtiFromToken(newRefreshToken); redisTemplate.opsForValue().set( "refresh:" + newJti, userId, jwtUtil.getRefreshTokenExpiration(), TimeUnit.MILLISECONDS );Optional hardening (future): store a short-lived “used:” marker to detect/token-ban refresh token reuse.
Also applies to: 299-307
🧹 Nitpick comments (2)
src/main/java/com/iemr/common/utils/JwtUserIdValidationFilter.java (2)
172-172: Remove duplicate skip condition for logOutUserFromConcurrentSession.This endpoint is already covered earlier with equalsIgnoreCase. The duplicate startsWith adds noise without benefit.
Apply this diff:
- || path.startsWith(contextPath + "/user/logOutUserFromConcurrentSession")
173-174: Avoid redundant skip for refreshToken (equals + startsWith).Keep one form (prefer equals for exact match) to reduce confusion.
If you want to retain only the exact match (already present above), remove these lines:
- || path.startsWith(contextPath + "/user/refreshToken"); + ;Alternatively, if you want startsWith behavior, drop the earlier equals(...) check instead.
📜 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(1 hunks)src/main/java/com/iemr/common/utils/JwtUserIdValidationFilter.java(1 hunks)
🔇 Additional comments (1)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (1)
288-289: Allowing 'New' status for refresh looks correct.Matches the PR objective to unblock refresh for Active or New users.
| if (user.getM_status() == null || !"Active".equalsIgnoreCase(user.getM_status().getStatus())) { | ||
| if (user.getM_status() == null || !("Active".equalsIgnoreCase(user.getM_status().getStatus()) | ||
| || "New".equalsIgnoreCase(user.getM_status().getStatus()))) { | ||
| logger.warn("Token validation failed: user account is inactive or not in 'Active' status."); |
There was a problem hiding this comment.
Fix log message to reflect the broadened rule.
It still mentions only 'Active'.
Apply this diff:
- logger.warn("Token validation failed: user account is inactive or not in 'Active' status.");
+ logger.warn("Token validation failed: user account is neither 'Active' nor 'New'.");📝 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.
| logger.warn("Token validation failed: user account is inactive or not in 'Active' status."); | |
| logger.warn("Token validation failed: user account is neither 'Active' nor 'New'."); |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
around line 290, the logger.warn message still references only 'Active' even
though the rule was broadened; update the warning text to reflect the broader
rule (e.g., state that token validation failed because the user account is
inactive or not in an allowed/valid status) so the log accurately describes the
condition.
|
📋 Description
JIRA ID: AMM-1845
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