User-Agent validation #79
Conversation
WalkthroughThis update introduces a new utility class, Changes
Sequence Diagram(s)sequenceDiagram
participant ServiceImpl as Service Implementation
participant RestTemplateUtil
participant CookieUtil
participant UserAgentContext
participant RestTemplate
ServiceImpl->>RestTemplateUtil: createRequestEntity(body, authorization)
RestTemplateUtil->>UserAgentContext: getUserAgent()
RestTemplateUtil->>CookieUtil: getJwtTokenFromCookie(request)
RestTemplateUtil-->>ServiceImpl: HttpEntity with headers/body
ServiceImpl->>RestTemplate: exchange(..., HttpEntity, ...)
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (1)src/main/java/com/iemr/tm/utils/http/HTTPRequestInterceptor.java (1)⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java (1)
110-118:⚠️ Potential issueCookie name is case-sensitive
cookie.getName().equals("Jwttoken")will not match"JwtToken"set by many auth libraries.
Use a case-insensitive comparison or normalise the name:-if (cookie.getName().equals("Jwttoken")) { +if ("jwttoken".equalsIgnoreCase(cookie.getName())) {
🧹 Nitpick comments (10)
src/main/java/com/iemr/tm/utils/http/AuthorizationHeaderRequestWrapper.java (4)
10-16: Inconsistent variable naming convention.The instance variable
Authorizationdoesn't follow Java naming conventions for non-constant fields. Variables should start with lowercase letters.-private final String Authorization; +private final String authorization; public AuthorizationHeaderRequestWrapper(HttpServletRequest request, String authHeaderValue) { super(request); - this.Authorization = authHeaderValue; + this.authorization = authHeaderValue; }
19-24: Update variable reference in getHeader method.If you rename the variable as suggested above, update the reference here.
@Override public String getHeader(String name) { if ("Authorization".equalsIgnoreCase(name)) { - return Authorization; + return authorization; } return super.getHeader(name); }
26-32: Update variable reference in getHeaders method.If you rename the variable as suggested above, update the reference here.
@Override public Enumeration<String> getHeaders(String name) { if ("Authorization".equalsIgnoreCase(name)) { - return Collections.enumeration(Collections.singletonList(Authorization)); + return Collections.enumeration(Collections.singletonList(authorization)); } return super.getHeaders(name); }
34-41: Consider using case-insensitive check for consistency.For consistency with other methods, consider using case-insensitive check for "Authorization" in the getHeaderNames method.
@Override public Enumeration<String> getHeaderNames() { List<String> names = Collections.list(super.getHeaderNames()); - if (!names.contains("Authorization")) { + boolean hasAuthHeader = names.stream() + .anyMatch(name -> "Authorization".equalsIgnoreCase(name)); + if (!hasAuthHeader) { names.add("Authorization"); } return Collections.enumeration(names); }src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java (6)
56-58: Consider using a consolidated path-matching strategyThe list of “public” paths has grown long and is matched using a mixture of
equals,equalsIgnoreCase, andstartsWith.
A typo (e.g. trailing slash) will silently skip the filter and weaken security. Spring already offersAntPathMatcherand MVC configuration for this purpose; delegating to those utilities will be safer and more maintainable.
64-74: Minor: use the imported class directly
AuthorizationHeaderRequestWrapperis already imported (line 9), but the code still uses the fully-qualified name.
Using the simple name keeps the code shorter and consistent with the import.-AuthorizationHeaderRequestWrapper authorizationHeaderRequestWrapper = new com.iemr.tm.utils.http.AuthorizationHeaderRequestWrapper(request, ""); +AuthorizationHeaderRequestWrapper authorizationHeaderRequestWrapper = + new AuthorizationHeaderRequestWrapper(request, "");
77-84: Duplicate wrapper creation logic – extract to a helperThe cookie and header branches are identical except for the token source.
Extracting the common logic into a small private method eliminates duplication and makes future changes (e.g. adding audit logging) less error-prone.
94-96: Consistency: log at INFO and include request idThe warning here will fire for every unauthenticated request, potentially spamming logs.
Consider downgrading toINFO(or adding a request-id/MDC key) so repeated 401s from the same client are easier to correlate.
103-108: Broaden mobile-client detection & avoid false negativesMatching only
"okhttp"will miss many iOS and Android user agents (e.g. defaultNSURLSession,Dio, etc.).
Either document the limitation or expand the pattern, perhaps:return userAgent.contains("okhttp") || userAgent.contains("ios") || userAgent.contains("android");Alternatively, allow the list to be externalised (configuration property).
110-118: Magic string – promote to a constant
"Jwttoken"appears here and (indirectly) in other branches.
Define a private static finalCOOKIE_JWT_TOKEN = "JwtToken"constant to avoid typos in future edits.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/com/iemr/tm/controller/registrar/main/RegistrarController.java(0 hunks)src/main/java/com/iemr/tm/service/common/transaction/CommonNurseServiceImpl.java(1 hunks)src/main/java/com/iemr/tm/service/registrar/RegistrarServiceImpl.java(2 hunks)src/main/java/com/iemr/tm/utils/CookieUtil.java(1 hunks)src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java(2 hunks)src/main/java/com/iemr/tm/utils/UserAgentContext.java(1 hunks)src/main/java/com/iemr/tm/utils/http/AuthorizationHeaderRequestWrapper.java(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/com/iemr/tm/controller/registrar/main/RegistrarController.java
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/main/java/com/iemr/tm/service/registrar/RegistrarServiceImpl.java (1)
src/main/java/com/iemr/tm/utils/UserAgentContext.java (1)
UserAgentContext(3-18)
src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java (2)
src/main/java/com/iemr/tm/utils/http/AuthorizationHeaderRequestWrapper.java (1)
AuthorizationHeaderRequestWrapper(10-42)src/main/java/com/iemr/tm/utils/UserAgentContext.java (1)
UserAgentContext(3-18)
🔇 Additional comments (3)
src/main/java/com/iemr/tm/service/common/transaction/CommonNurseServiceImpl.java (1)
1897-1916: Good addition of null check to prevent NullPointerExceptionThe added null check for
menstrualHistoryDetailsbefore accessing its properties is a good defensive programming practice. This ensures the application won't throw a NullPointerException when processing menstrual history data that might be null.src/main/java/com/iemr/tm/utils/CookieUtil.java (1)
28-37: Good defensive programming with null check.The addition of the null check for
request.getCookies()prevents potentialNullPointerExceptionwhen no cookies are present in the request. The stream-based implementation is clean and efficient.src/main/java/com/iemr/tm/service/registrar/RegistrarServiceImpl.java (1)
76-76: LGTM - Import for new UserAgentContext utility.The import statement correctly references the newly added UserAgentContext class.
| MultiValueMap<String, String> headers = new LinkedMultiValueMap<String, String>(); | ||
| headers.add("Content-Type", MediaType.APPLICATION_JSON + ";charset=utf-8"); | ||
| // headers.add("Content-Type", MediaType.APPLICATION_JSON); | ||
| headers.add("User-Agent",UserAgentContext.getUserAgent()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider null check when retrieving UserAgent.
The code adds the User-Agent header using the thread-local value, but doesn't handle the case where UserAgentContext.getUserAgent() might return null.
-headers.add("User-Agent",UserAgentContext.getUserAgent());
+String userAgent = UserAgentContext.getUserAgent();
+if (userAgent != null) {
+ headers.add("User-Agent", userAgent);
+}📝 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.
| headers.add("User-Agent",UserAgentContext.getUserAgent()); | |
| String userAgent = UserAgentContext.getUserAgent(); | |
| if (userAgent != null) { | |
| headers.add("User-Agent", userAgent); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/tm/service/registrar/RegistrarServiceImpl.java at line
666, the code adds the User-Agent header without checking if
UserAgentContext.getUserAgent() returns null. To fix this, add a null check
before adding the header; only add the User-Agent header if the retrieved value
is not null to avoid potential NullPointerExceptions or invalid headers.
| public class UserAgentContext { | ||
| private static final ThreadLocal<String> userAgentHolder = new ThreadLocal<>(); | ||
|
|
||
| public static void setUserAgent(String userAgent) { | ||
| userAgentHolder.set(userAgent); | ||
| } | ||
|
|
||
| public static String getUserAgent() { | ||
| return userAgentHolder.get(); | ||
| } | ||
|
|
||
| public static void clear() { | ||
| userAgentHolder.remove(); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Thread-local implementation looks good but ensure proper cleanup.
The implementation of UserAgentContext using ThreadLocal is clean and appropriate for maintaining user agent data per request thread. However, ensure that the clear() method is called at the end of request processing to prevent memory leaks in application servers with thread pools.
Consider adding a try-with-resources pattern or filter that ensures cleanup:
// Example in a filter's doFilter method
try {
UserAgentContext.setUserAgent(request.getHeader("User-Agent"));
chain.doFilter(request, response);
} finally {
UserAgentContext.clear();
}🏁 Script executed:
#!/bin/bash
# Check if clear() is called in filter or interceptor classes
rg -A 5 -B 5 "UserAgentContext.clear\(\)" --type javaLength of output: 55
🏁 Script executed:
#!/bin/bash
# Verify where UserAgentContext.setUserAgent is invoked and inspect surrounding code for cleanup
rg -A5 -B5 "UserAgentContext.setUserAgent" --type javaLength of output: 1109
Ensure UserAgentContext.clear() Is Always Invoked in Your Filter
I didn’t find any calls to UserAgentContext.clear() in your filter, leaving threads at risk of retaining stale data. Wrap your setUserAgent / doFilter call in a try…finally to guarantee cleanup.
• File to update:
src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java
• Replace this block:
if (userAgent != null && isMobileClient(userAgent) && authHeader != null) {
UserAgentContext.setUserAgent(userAgent);
filterChain.doFilter(servletRequest, servletResponse);
return;
}with:
if (userAgent != null && isMobileClient(userAgent) && authHeader != null) {
try {
UserAgentContext.setUserAgent(userAgent);
filterChain.doFilter(servletRequest, servletResponse);
} finally {
UserAgentContext.clear();
}
return;
}This guarantees that clear() runs even if an exception occurs during doFilter.
🤖 Prompt for AI Agents
In src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java around the
block where UserAgentContext.setUserAgent is called before filterChain.doFilter,
wrap the setUserAgent and doFilter calls inside a try-finally block.
Specifically, replace the existing code that sets the user agent and calls
doFilter with a try block that sets the user agent and calls doFilter, and in
the finally block call UserAgentContext.clear(). This ensures that clear() is
always invoked to prevent memory leaks even if doFilter throws an exception.
There was a problem hiding this comment.
@coderabbitai why its showing again i already added try and finally block to clear UserAgentContect
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check JwtUserIdValidationFilter.java for UserAgentContext usage with try-finally blocks
cat src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java
Length of output: 4848
@ravishanigarapu You're absolutely right. I can see that you've already implemented the try-finally block correctly in JwtUserIdValidationFilter.java:
if (userAgent != null && isMobileClient(userAgent) && authHeader != null) {
try {
UserAgentContext.setUserAgent(userAgent);
filterChain.doFilter(servletRequest, servletResponse);
} finally {
UserAgentContext.clear();
}
return;
}This implementation matches exactly what was suggested. The previous comment might have been based on an older version of your code that didn't include these changes yet. Your implementation is correct and ensures proper cleanup of the ThreadLocal resources.
✏️ Learnings added
Learnt from: ravishanigarapu
PR: PSMRI/TM-API#79
File: src/main/java/com/iemr/tm/utils/UserAgentContext.java:3-18
Timestamp: 2025-05-22T09:27:49.400Z
Learning: JwtUserIdValidationFilter.java already includes a try-finally block to properly clear UserAgentContext's ThreadLocal resources after use to prevent memory leaks.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/com/iemr/tm/service/dataSyncActivity/DownloadDataFromServerImpl.java (1)
142-143: Excellent refactoring to use the centralized RestTemplateUtil.This change replaces the manual creation of HTTP headers and entity with a centralized utility method, improving code maintainability and consistency.
Consider updating or removing the comment on line 141 "Multivalue map for headers with content-type and auth key" as it's now obsolete with the new implementation.
- // Multivalue map for headers with content-type and auth key + // Create HTTP entity with headers and body using utility methodsrc/main/java/com/iemr/tm/utils/RestTemplateUtil.java (1)
19-24: Improve exception handling for JWT token extraction.The current implementation catches exceptions but only prints the stack trace and continues execution, which could hide issues with token extraction.
Consider adding logging with an appropriate log level instead of using
e.printStackTrace()and possibly adding a comment explaining why execution continues despite exceptions.try { jwtTokenFromCookie = CookieUtil.getJwtTokenFromCookie(requestHeader); } catch (Exception e) { - e.printStackTrace(); + // Log at appropriate level but continue, as JWT token might be optional + // or provided through other means (e.g., Authorization header) + logger.warn("Failed to extract JWT token from cookie: " + e.getMessage()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/com/iemr/tm/service/cancerScreening/CSCarestreamServiceImpl.java(2 hunks)src/main/java/com/iemr/tm/service/common/master/RegistrarServiceMasterDataImpl.java(2 hunks)src/main/java/com/iemr/tm/service/common/transaction/CommonDoctorServiceImpl.java(2 hunks)src/main/java/com/iemr/tm/service/common/transaction/CommonServiceImpl.java(2 hunks)src/main/java/com/iemr/tm/service/dataSyncActivity/DownloadDataFromServerImpl.java(2 hunks)src/main/java/com/iemr/tm/service/dataSyncActivity/UploadDataToServerImpl.java(2 hunks)src/main/java/com/iemr/tm/service/registrar/RegistrarServiceImpl.java(5 hunks)src/main/java/com/iemr/tm/service/tele_consultation/SMSGatewayServiceImpl.java(2 hunks)src/main/java/com/iemr/tm/service/tele_consultation/TeleConsultationServiceImpl.java(2 hunks)src/main/java/com/iemr/tm/utils/CookieUtil.java(1 hunks)src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java(2 hunks)src/main/java/com/iemr/tm/utils/RestTemplateUtil.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/com/iemr/tm/service/common/transaction/CommonDoctorServiceImpl.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/com/iemr/tm/service/registrar/RegistrarServiceImpl.java
- src/main/java/com/iemr/tm/utils/CookieUtil.java
- src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java
🧰 Additional context used
🧬 Code Graph Analysis (8)
src/main/java/com/iemr/tm/service/tele_consultation/SMSGatewayServiceImpl.java (1)
src/main/java/com/iemr/tm/utils/RestTemplateUtil.java (1)
RestTemplateUtil(13-36)
src/main/java/com/iemr/tm/service/cancerScreening/CSCarestreamServiceImpl.java (1)
src/main/java/com/iemr/tm/utils/RestTemplateUtil.java (1)
RestTemplateUtil(13-36)
src/main/java/com/iemr/tm/service/common/master/RegistrarServiceMasterDataImpl.java (1)
src/main/java/com/iemr/tm/utils/RestTemplateUtil.java (1)
RestTemplateUtil(13-36)
src/main/java/com/iemr/tm/utils/RestTemplateUtil.java (1)
src/main/java/com/iemr/tm/utils/UserAgentContext.java (1)
UserAgentContext(3-18)
src/main/java/com/iemr/tm/service/dataSyncActivity/UploadDataToServerImpl.java (1)
src/main/java/com/iemr/tm/utils/RestTemplateUtil.java (1)
RestTemplateUtil(13-36)
src/main/java/com/iemr/tm/service/tele_consultation/TeleConsultationServiceImpl.java (1)
src/main/java/com/iemr/tm/utils/RestTemplateUtil.java (1)
RestTemplateUtil(13-36)
src/main/java/com/iemr/tm/service/dataSyncActivity/DownloadDataFromServerImpl.java (1)
src/main/java/com/iemr/tm/utils/RestTemplateUtil.java (1)
RestTemplateUtil(13-36)
src/main/java/com/iemr/tm/service/common/transaction/CommonServiceImpl.java (1)
src/main/java/com/iemr/tm/utils/RestTemplateUtil.java (1)
RestTemplateUtil(13-36)
🔇 Additional comments (15)
src/main/java/com/iemr/tm/service/common/transaction/CommonServiceImpl.java (2)
68-68: Good addition of the RestTemplateUtil import.This import supports the HTTP request entity creation refactoring.
566-566: Excellent refactoring to use centralized RestTemplateUtil.Replacing manual HTTP entity creation with the centralized utility method improves code maintainability and consistency across API calls. The utility handles all necessary headers including Content-Type, Authorization, User-Agent, and JWT tokens from both request headers and cookies.
src/main/java/com/iemr/tm/service/cancerScreening/CSCarestreamServiceImpl.java (2)
47-47: Good addition of the RestTemplateUtil import.This import supports the HTTP request entity creation refactoring.
71-71: Improved HTTP entity creation with centralized utility.Using RestTemplateUtil.createRequestEntity eliminates manual header construction and ensures consistent header management including User-Agent and JWT tokens across API calls.
src/main/java/com/iemr/tm/service/dataSyncActivity/UploadDataToServerImpl.java (2)
49-49: Good addition of the RestTemplateUtil import.This import enables the use of the centralized HTTP entity creation utility.
216-216: Effective refactoring to use centralized HTTP entity creation.Replacing custom header setup with RestTemplateUtil.createRequestEntity improves code consistency and maintainability. The utility handles proper content type, authorization, user-agent headers, and JWT token management.
src/main/java/com/iemr/tm/service/tele_consultation/TeleConsultationServiceImpl.java (2)
63-63: Good addition of the RestTemplateUtil import.This import supports the HTTP request entity creation refactoring.
222-222: Improved HTTP entity creation with centralized utility.Replacing manual header construction with RestTemplateUtil.createRequestEntity ensures consistent header management including User-Agent and JWT tokens. This aligns with the broader refactoring pattern across the codebase.
src/main/java/com/iemr/tm/service/common/master/RegistrarServiceMasterDataImpl.java (2)
65-65: Good addition of RestTemplateUtil import.The import added here supports the refactoring to centralize HTTP entity creation.
221-221: Excellent refactoring to use the centralized RestTemplateUtil.This change replaces the manual creation of HTTP headers and entity with a centralized utility method, improving code maintainability and consistency across the application.
src/main/java/com/iemr/tm/service/dataSyncActivity/DownloadDataFromServerImpl.java (1)
52-52: Good addition of RestTemplateUtil import.The import supports the code refactoring for centralizing HTTP entity creation.
src/main/java/com/iemr/tm/service/tele_consultation/SMSGatewayServiceImpl.java (2)
50-50: Good addition of RestTemplateUtil import.The import supports the refactoring to centralize HTTP entity creation.
199-200: Effective use of RestTemplateUtil for HTTP entity creation.The refactoring simplifies the code by delegating HTTP entity creation to the centralized utility method.
src/main/java/com/iemr/tm/utils/RestTemplateUtil.java (2)
1-12: Good organization of imports and package structure.The imports are well-organized and include all necessary dependencies for HTTP entity creation.
26-32: Good implementation of header construction.The headers are comprehensively set with appropriate values for content type, user agent, authorization, JWT token, and cookies.
|
2 participants
📋 Description
JIRA ID: AMM-1456
Please provide a summary of the change and the motivation behind it. Include relevant context and details.
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores