Force logout #207
Force logout #207
Conversation
WalkthroughThis change introduces a JWT token blacklist mechanism. It adds a new constant for the JWT token name, implements an in-memory blacklist with expiration, updates the force logout flow to blacklist tokens instead of invalidating cookies, and modifies token validation to reject blacklisted tokens. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Controller
participant TokenBlacklist
participant JwtUtil
User->>Controller: Force Logout Request
Controller->>TokenBlacklist: blacklistToken(token)
Note right of Controller: (Extract token from cookies/header)
Controller-->>User: Respond (logout complete)
User->>JwtUtil: validateToken(token)
JwtUtil->>TokenBlacklist: isTokenBlacklisted(token)
alt Token is blacklisted
JwtUtil-->>User: Return null (invalid token)
else Token not blacklisted
JwtUtil-->>User: Validate JWT, return claims if valid
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}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 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(4 hunks)src/main/java/com/iemr/common/utils/JwtUtil.java(1 hunks)src/main/java/com/iemr/common/utils/TokenBlacklist.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/iemr/common/utils/JwtUtil.java (1)
src/main/java/com/iemr/common/utils/TokenBlacklist.java (1)
TokenBlacklist(10-36)
🪛 GitHub Check: SonarCloud
src/main/java/com/iemr/common/utils/TokenBlacklist.java
[notice] 11-11: 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: Build
- GitHub Check: Analyze (java)
🔇 Additional comments (6)
src/main/java/com/iemr/common/constant/Constants.java (1)
14-14: LGTM! Clean constant addition.The JWT token constant is properly defined and will improve code maintainability by centralizing the token name reference.
src/main/java/com/iemr/common/utils/JwtUtil.java (2)
60-62: LGTM! Proper blacklist integration.The blacklist check is correctly placed before token parsing, providing an efficient early exit for blacklisted tokens.
64-64: LGTM! Clean code refactoring.The chained method call improves readability while maintaining the same functionality.
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (3)
47-47: LGTM! Proper imports added.The necessary imports for Constants and TokenBlacklist are correctly added.
Also applies to: 60-60
954-964: LGTM! Well-implemented helper method.The helper method correctly extracts JWT tokens from cookies with proper null checks and case-insensitive comparison.
945-945: Good practice: commenting out instead of removing.Keeping the cookie invalidation code commented out allows for easy rollback if needed while transitioning to the token blacklist approach.
| @Value("${jwt.blacklist.expiration}") | ||
| private static long BLACK_LIST_EXPIRATION_TIME; |
There was a problem hiding this comment.
Fix Spring injection into static field.
Spring cannot inject values into static fields using @Value. This injection will fail and BLACK_LIST_EXPIRATION_TIME will remain 0.
Convert to instance-based approach:
-@Value("${jwt.blacklist.expiration}")
-private static long BLACK_LIST_EXPIRATION_TIME;
+@Component
+public class TokenBlacklist {
+ @Value("${jwt.blacklist.expiration}")
+ private long blackListExpirationTime;
+
+ private static TokenBlacklist instance;
+
+ @PostConstruct
+ public void init() {
+ instance = this;
+ }
+
+ public static void blacklistToken(String token) {
+ instance.blacklistTokenInternal(token);
+ }Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: SonarCloud
[notice] 11-11: Injecting data into static fields is not supported by Spring
Remove this injection annotation targeting the static field.See more on SonarQube Cloud
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/utils/TokenBlacklist.java around lines 11 to
12, the @Value annotation is used on a static field BLACK_LIST_EXPIRATION_TIME,
which Spring cannot inject. To fix this, remove the static modifier from
BLACK_LIST_EXPIRATION_TIME and make it an instance field. Then ensure the class
is a Spring-managed bean so that Spring can inject the value properly at
runtime.
| String token = null; | ||
| token = getJwtTokenFromCookies(httpRequest); | ||
| if(null == token) { | ||
| token = httpRequest.getHeader(Constants.JWT_TOKEN); | ||
| } | ||
| TokenBlacklist.blacklistToken(token); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and null check for token blacklisting.
The token blacklisting should handle cases where no token is found and should occur even if the service call fails to ensure security.
// Perform the force logout logic
-iemrAdminUserServiceImpl.forceLogout(request);
String token = null;
token = getJwtTokenFromCookies(httpRequest);
if(null == token) {
token = httpRequest.getHeader(Constants.JWT_TOKEN);
}
-TokenBlacklist.blacklistToken(token);
+if (token != null) {
+ TokenBlacklist.blacklistToken(token);
+}
+
+// Perform the force logout logic
+iemrAdminUserServiceImpl.forceLogout(request);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
around lines 938 to 943, add a null check before calling
TokenBlacklist.blacklistToken to avoid null pointer exceptions if no token is
found. Also, wrap the blacklisting call in a try-catch block to handle any
exceptions gracefully, ensuring the blacklisting attempt occurs regardless of
service call failures for security purposes.
|
| 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
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (4)
89-90:⚠️ Potential issueFix Spring injection into static field.
Spring cannot inject values into static fields using
@Value. This injection will fail andBLACK_LIST_EXPIRATION_TIMEwill remain 0, causing incorrect blacklist expiration times.Convert to instance field approach:
-@Value("${jwt.blacklist.expiration}") -private static long BLACK_LIST_EXPIRATION_TIME; +@Value("${jwt.blacklist.expiration}") +private long blackListExpirationTime;Then update the usage in the
forceLogoutmethod:-TokenBlacklist.blacklistToken(token,BLACK_LIST_EXPIRATION_TIME); +TokenBlacklist.blacklistToken(token, blackListExpirationTime);🧰 Tools
🪛 GitHub Check: SonarCloud
[notice] 89-89: Injecting data into static fields is not supported by Spring
Remove this injection annotation targeting the static field.See more on SonarQube Cloud
941-946: 🛠️ Refactor suggestionAdd null check before token blacklisting.
The token extraction could return null if no JWT token is found in cookies or headers. Blacklisting a null token should be avoided.
String token = null; token = getJwtTokenFromCookies(httpRequest); if(null == token) { token = httpRequest.getHeader(Constants.JWT_TOKEN); } -TokenBlacklist.blacklistToken(token,BLACK_LIST_EXPIRATION_TIME); +if (token != null) { + TokenBlacklist.blacklistToken(token, BLACK_LIST_EXPIRATION_TIME); +}
89-90:⚠️ Potential issueFix Spring injection into static field.
Spring cannot inject values into static fields using
@Value. This will causeBLACK_LIST_EXPIRATION_TIMEto remain 0, making the blacklist ineffective.Convert to instance field and ensure proper injection:
-@Value("${jwt.blacklist.expiration}") -private static long BLACK_LIST_EXPIRATION_TIME; +@Value("${jwt.blacklist.expiration}") +private long blackListExpirationTime;Then update the usage in the
forceLogoutmethod to use the instance field.🧰 Tools
🪛 GitHub Check: SonarCloud
[notice] 89-89: Injecting data into static fields is not supported by Spring
Remove this injection annotation targeting the static field.See more on SonarQube Cloud
941-946:⚠️ Potential issueAdd null check and improve error handling for token blacklisting.
The token blacklisting should include proper null checking and error handling. Additionally, for security reasons, the blacklisting should occur regardless of the service call outcome.
Apply this improvement:
+// Blacklist token first for security +String token = getJwtTokenFromCookies(httpRequest); +if (token == null) { + token = httpRequest.getHeader(Constants.JWT_TOKEN); +} +if (token != null && !token.trim().isEmpty()) { + try { + TokenBlacklist.blacklistToken(token, blackListExpirationTime); + } catch (Exception e) { + logger.warn("Failed to blacklist token during force logout", e); + } +} + // Perform the force logout logic iemrAdminUserServiceImpl.forceLogout(request); -String token = null; -token = getJwtTokenFromCookies(httpRequest); -if(null == token) { - token = httpRequest.getHeader(Constants.JWT_TOKEN); -} -TokenBlacklist.blacklistToken(token,BLACK_LIST_EXPIRATION_TIME);
🧹 Nitpick comments (1)
src/main/java/com/iemr/common/utils/TokenBlacklist.java (1)
3-6: Clean up unused imports.Remove the unused import statements to improve code cleanliness.
-import java.util.HashMap; -import java.util.HashSet; -import java.util.Set; import java.util.concurrent.ConcurrentHashMap;
📜 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/TokenBlacklist.java(1 hunks)
🧰 Additional context used
🪛 GitHub Check: SonarCloud
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
[notice] 89-89: 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 (3)
- GitHub Check: Package-test
- GitHub Check: Analyze (java)
- GitHub Check: Build
🔇 Additional comments (5)
src/main/java/com/iemr/common/utils/TokenBlacklist.java (3)
15-24: LGTM! Thread-safe implementation with proper validation.The implementation correctly addresses the previous concerns:
- Uses
ConcurrentHashMapfor thread safety- Includes proper null/empty validation
- Calculates expiration timestamp correctly
28-40: LGTM! Proper token validation with cleanup logic.The method correctly:
- Validates input parameters
- Handles expired token cleanup automatically
- Returns appropriate boolean values for different scenarios
1-42: LGTM! Well-implemented token blacklist utility.The implementation correctly addresses all the concerns raised in previous reviews:
- Uses
ConcurrentHashMapfor thread safety- Includes proper null/empty token validation
- Correctly calculates expiration timestamps (
System.currentTimeMillis() + blackListExpirationTime)- Implements automatic cleanup of expired tokens
The utility provides a clean and secure in-memory token blacklist mechanism.
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (2)
957-967: LGTM! Clean helper method implementation.The method properly extracts JWT tokens from cookies with case-insensitive matching, which aligns with the existing codebase pattern.
957-967: LGTM! Clean helper method implementation.The
getJwtTokenFromCookiesmethod is well-implemented with proper null checking and case-insensitive cookie name matching.
| @Autowired | ||
| private RedisTemplate<String, Object> redisTemplate; | ||
| @Value("${jwt.blacklist.expiration}") | ||
| private static long BLACK_LIST_EXPIRATION_TIME; |
There was a problem hiding this comment.
Each token will have it's own expiration time. Why would this be a constant?
| return outputResponse.toString(); | ||
| } | ||
|
|
||
| private String getJwtTokenFromCookies(HttpServletRequest request) { |
There was a problem hiding this comment.
Look up for current token seems to be getting repeated in so many places. Can we make this a reusable method?
| if(null == token) { | ||
| token = httpRequest.getHeader(Constants.JWT_TOKEN); | ||
| } | ||
| TokenBlacklist.blacklistToken(token,BLACK_LIST_EXPIRATION_TIME); |
| // invalidateJwtCookie(httpRequest, response); | ||
|
|
There was a problem hiding this comment.
If this method is not going to be used, let's remove it from code.
Setting cookie to null with Set-cookie header is still valid though.
|
|
||
| import org.springframework.beans.factory.annotation.Value; | ||
|
|
||
| public class TokenBlacklist { |
There was a problem hiding this comment.
Please use "Deny" instead of Black
| public class TokenBlacklist { | ||
|
|
||
|
|
||
| // Store blacklisted tokens (in-memory) |
There was a problem hiding this comment.
What does in memory mean? We should store this in Redis.
| Long expiry = blacklistedTokens.get(token); | ||
| if (expiry == null) return false; | ||
| // If token is expired, remove it from blacklist and treat as not blacklisted | ||
| if (System.currentTimeMillis() > expiry) { |
There was a problem hiding this comment.
This looks like application memory.
Not the best place to keep.
Also it looks like token gets removed only in subsequent request.
📋 Description
JIRA ID: AMM-1507
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