AMM-1456 : JWT validation #84
Conversation
WalkthroughThis update introduces new utility classes for HTTP request handling and user agent context, refactors JWT validation logic in the filter, and centralizes HTTP entity creation for REST calls. It adjusts token extraction methods, improves request header management, and enhances logging and error responses for unauthorized requests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant JwtUserIdValidationFilter
participant AuthorizationHeaderRequestWrapper
participant UserAgentContext
Client->>JwtUserIdValidationFilter: Sends HTTP request
JwtUserIdValidationFilter->>JwtUserIdValidationFilter: shouldSkipPath(path)
alt Path should be skipped
JwtUserIdValidationFilter-->>Client: Passes request through
else
JwtUserIdValidationFilter->>JwtUserIdValidationFilter: Check JWT in cookies
alt JWT valid in cookies
JwtUserIdValidationFilter->>AuthorizationHeaderRequestWrapper: Wrap request (empty Authorization)
AuthorizationHeaderRequestWrapper-->>JwtUserIdValidationFilter: Wrapped request
JwtUserIdValidationFilter-->>Client: Passes wrapped request
else
JwtUserIdValidationFilter->>JwtUserIdValidationFilter: Check JWT in header
alt JWT valid in header
JwtUserIdValidationFilter->>AuthorizationHeaderRequestWrapper: Wrap request (empty Authorization)
AuthorizationHeaderRequestWrapper-->>JwtUserIdValidationFilter: Wrapped request
JwtUserIdValidationFilter-->>Client: Passes wrapped request
else
JwtUserIdValidationFilter->>JwtUserIdValidationFilter: isMobileClient(User-Agent)
alt Mobile client with Authorization header
JwtUserIdValidationFilter->>UserAgentContext: setUserAgent
JwtUserIdValidationFilter-->>Client: Passes request
JwtUserIdValidationFilter->>UserAgentContext: clear
else
JwtUserIdValidationFilter-->>Client: Responds 401 Unauthorized
end
end
end
end
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (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: 2
🧹 Nitpick comments (6)
src/main/environment/common_ci.properties (1)
116-118: Confirm env-substitution & provide a safe default for Swagger toggleThe two new flags look correct for Springdoc, but because you rely on an environment placeholder (
@env.SWAGGER_DOC_ENABLED@) the build will fail if that variable is missing.
Consider adding a profile-specific.propertiesfile (e.g.application-local.properties) with an explicit default (trueorfalse) so that local runs and CI don’t unexpectedly crash.src/main/java/com/wipro/fhir/utils/http/AuthorizationHeaderRequestWrapper.java (3)
9-15: Rename the field to follow Java conventions
Authorizationlooks like a constant but is actually per-request state. A lower-camel-case name (e.g.authorizationValue) prevents confusion and avoids the sonar smell that “Variables should not start with an upper-case letter”.-private final String Authorization; +private final String authorizationValue;Remember to update the three usages below.
25-31: Guard againstnullheader values when building the enumerationIf the wrapper is instantiated with
null(unlikely but possible),Collections.singletonList(null)will throw an NPE. A tiny guard keeps things safe:- if ("Authorization".equalsIgnoreCase(name)) { - return Collections.enumeration(Collections.singletonList(Authorization)); + if ("Authorization".equalsIgnoreCase(name)) { + return Authorization == null + ? Collections.emptyEnumeration() + : Collections.enumeration(Collections.singletonList(Authorization));
34-40: Potential duplication in header names
getHeaderNames()unconditionally adds"Authorization"even when the custom value isnull.
That can report a header name whose value subsequently resolves tonull, which may surprise downstream code.
Consider adding it only whenauthorizationValueis non-null/ non-empty.src/main/java/com/wipro/fhir/utils/JwtUserIdValidationFilter.java (2)
65-71: Avoid duplicate validation effortThe cookie and header branches are almost identical except for the token source. You can reduce duplication (and future bugs) by extracting a helper method that attempts validation and, upon success, forwards the wrapped request.
109-114: Hard-coded allow-list of paths is prone to driftEvery new public endpoint requires touching this filter. A property-driven list or annotation-based approach (
@PermitAll,@SecurityRequirement) is more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
src/main/environment/common_ci.properties(1 hunks)src/main/java/com/wipro/fhir/utils/JwtUserIdValidationFilter.java(3 hunks)src/main/java/com/wipro/fhir/utils/http/AuthorizationHeaderRequestWrapper.java(1 hunks)src/main/java/com/wipro/fhir/utils/http/HTTPRequestInterceptor.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/wipro/fhir/utils/JwtUserIdValidationFilter.java (1)
src/main/java/com/wipro/fhir/utils/http/AuthorizationHeaderRequestWrapper.java (1)
AuthorizationHeaderRequestWrapper(9-41)
| if (authorization.equals("")) { | ||
| logger.info("Authorization header is null or empty. Skipping HTTPRequestInterceptor."); | ||
| return true; // Allow the request to proceed without validation | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Null-pointer risk when Authorization header is absent
authorization.equals("") throws an NPE if the header is missing.
A safe check protects all callers and keeps the log message aligned with reality.
-if (authorization.equals("")) {
+if (authorization == null || authorization.trim().isEmpty()) {📝 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.
| if (authorization.equals("")) { | |
| logger.info("Authorization header is null or empty. Skipping HTTPRequestInterceptor."); | |
| return true; // Allow the request to proceed without validation | |
| } | |
| // src/main/java/com/wipro/fhir/utils/http/HTTPRequestInterceptor.java | |
| // Lines: ~62-65 | |
| if (authorization == null || authorization.trim().isEmpty()) { | |
| logger.info("Authorization header is null or empty. Skipping HTTPRequestInterceptor."); | |
| return true; // Allow the request to proceed without validation | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/wipro/fhir/utils/http/HTTPRequestInterceptor.java around
lines 62 to 65, the code calls authorization.equals("") without checking if
authorization is null, which can cause a NullPointerException if the
Authorization header is absent. Fix this by first checking if authorization is
null or empty using a safe method like authorization == null ||
authorization.isEmpty(), and update the log message to reflect that the header
may be null or empty.
| if (userAgent != null && isMobileClient(userAgent) && authHeader != null) { | ||
| filterChain.doFilter(servletRequest, servletResponse); | ||
| } else { | ||
| response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Invalid JWT token"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Mobile-client bypass may open an authentication hole
Right now any request whose User-Agent contains “okhttp” AND merely has an Authorization header (regardless of its content) skips JWT validation.
An attacker can spoof the UA string and send an empty or bogus header to gain access.
Consider tightening the rule:
- Check that the header starts with
Bearerand validate it, or - Restrict the bypass to very specific paths/methods, or
- Remove the bypass and rely solely on proper token validation.
🤖 Prompt for AI Agents
In src/main/java/com/wipro/fhir/utils/JwtUserIdValidationFilter.java around
lines 85 to 88, the current logic allows requests with a User-Agent containing
"okhttp" and any Authorization header to bypass JWT validation, which is a
security risk. To fix this, modify the condition to check that the Authorization
header starts with "Bearer " and validate the token properly before allowing the
bypass, or alternatively restrict the bypass to specific paths or methods, or
remove the bypass entirely to enforce JWT validation on all requests.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/main/java/com/wipro/fhir/utils/CookieUtil.java (1)
27-33: Static method with null check is a good improvementThe static implementation with a null check prevents potential NullPointerExceptions when cookies are not present. The stream-based approach is also more concise than the traditional loop.
For consistency and better documentation, consider:
+ /** + * Retrieves the JWT token from the request cookies. + * @param request The HTTP request containing cookies + * @return The JWT token value or null if not found + */ public static String getJwtTokenFromCookie(HttpServletRequest request) { if (request.getCookies() == null) { return null; // If cookies are null, return null safely. } return Arrays.stream(request.getCookies()).filter(cookie -> "Jwttoken".equals(cookie.getName())) .map(Cookie::getValue).findFirst().orElse(null); }Also, consider extracting "Jwttoken" as a constant to avoid duplication and make future changes easier.
src/main/java/com/wipro/fhir/service/api_channel/APIChannelImpl.java (1)
109-110: Good refactoring to use centralized request entity creationSimilar to the previous refactoring, this change utilizes
RestTemplateUtil.createRequestEntityfor consistent request entity creation.Consider removing the unused
getHttpHeadermethod (lines 128-139) since it's no longer being used after these refactorings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/com/wipro/fhir/service/api_channel/APIChannelImpl.java(3 hunks)src/main/java/com/wipro/fhir/utils/CookieUtil.java(1 hunks)src/main/java/com/wipro/fhir/utils/JwtUserIdValidationFilter.java(3 hunks)src/main/java/com/wipro/fhir/utils/RestTemplateUtil.java(1 hunks)src/main/java/com/wipro/fhir/utils/UserAgentContext.java(1 hunks)src/main/java/com/wipro/fhir/utils/http/HTTPRequestInterceptor.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/wipro/fhir/utils/http/HTTPRequestInterceptor.java
- src/main/java/com/wipro/fhir/utils/JwtUserIdValidationFilter.java
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/main/java/com/wipro/fhir/service/api_channel/APIChannelImpl.java (1)
src/main/java/com/wipro/fhir/utils/RestTemplateUtil.java (1)
RestTemplateUtil(13-42)
src/main/java/com/wipro/fhir/utils/RestTemplateUtil.java (1)
src/main/java/com/wipro/fhir/utils/UserAgentContext.java (1)
UserAgentContext(3-18)
🔇 Additional comments (2)
src/main/java/com/wipro/fhir/service/api_channel/APIChannelImpl.java (2)
42-42: Good addition of RestTemplateUtil importThe import for the new utility class is correctly added.
76-79: Clean refactoring to use centralized request entity creationThe code now uses the
RestTemplateUtil.createRequestEntitymethod instead of manually creating the HTTP entity, which reduces code duplication and centralizes header management.
| 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.
🛠️ Refactor suggestion
The ThreadLocal implementation looks good but needs documentation
The implementation correctly uses ThreadLocal for thread-safe user agent storage, which is essential in a multi-threaded web application environment. The class provides appropriate methods for setting, getting, and clearing the user agent value.
Add JavaDoc comments to explain the purpose of this class and its methods, especially highlighting the importance of calling clear() to prevent memory leaks in thread pools:
package com.wipro.fhir.utils;
+/**
+ * Utility class to store and retrieve User-Agent information in a thread-local context.
+ * This allows the User-Agent string to be propagated across the request processing chain.
+ */
public class UserAgentContext {
private static final ThreadLocal<String> userAgentHolder = new ThreadLocal<>();
+ /**
+ * Sets the User-Agent string for the current thread.
+ * @param userAgent The User-Agent string to store
+ */
public static void setUserAgent(String userAgent) {
userAgentHolder.set(userAgent);
}
+ /**
+ * Gets the User-Agent string for the current thread.
+ * @return The User-Agent string or null if not set
+ */
public static String getUserAgent() {
return userAgentHolder.get();
}
+ /**
+ * Clears the User-Agent string for the current thread.
+ * Important: This should be called after request processing is complete
+ * to prevent memory leaks in thread pools.
+ */
public static void clear() {
userAgentHolder.remove();
}
}📝 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 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(); | |
| } | |
| } | |
| package com.wipro.fhir.utils; | |
| /** | |
| * Utility class to store and retrieve User-Agent information in a thread-local context. | |
| * This allows the User-Agent string to be propagated across the request processing chain. | |
| */ | |
| public class UserAgentContext { | |
| private static final ThreadLocal<String> userAgentHolder = new ThreadLocal<>(); | |
| /** | |
| * Sets the User-Agent string for the current thread. | |
| * @param userAgent The User-Agent string to store | |
| */ | |
| public static void setUserAgent(String userAgent) { | |
| userAgentHolder.set(userAgent); | |
| } | |
| /** | |
| * Gets the User-Agent string for the current thread. | |
| * @return The User-Agent string or null if not set | |
| */ | |
| public static String getUserAgent() { | |
| return userAgentHolder.get(); | |
| } | |
| /** | |
| * Clears the User-Agent string for the current thread. | |
| * Important: This should be called after request processing is complete | |
| * to prevent memory leaks in thread pools. | |
| */ | |
| public static void clear() { | |
| userAgentHolder.remove(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/wipro/fhir/utils/UserAgentContext.java around lines 3 to
18, add JavaDoc comments to the class and each method. Document the class
purpose as a thread-safe holder for user agent strings using ThreadLocal. For
each method, explain its role, especially emphasize in the clear() method's
JavaDoc the importance of calling it to prevent memory leaks in thread pool
environments by removing the ThreadLocal value after use.
|
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
Bug Fixes
Refactor