Fix get login #203
Fix get login #203
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis change updates cookie name comparisons in several components to use case-insensitive matching. Specifically, checks for "Jwttoken" and "userId" cookies now use Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant CookieUtil
participant Filter
Client->>Controller: Sends request with cookies (e.g., "jwttoken", "Jwttoken")
Controller->>CookieUtil: Calls getJwtTokenFromCookie(request)
CookieUtil->>CookieUtil: Finds "Jwttoken" cookie (case-insensitive)
CookieUtil-->>Controller: Returns JWT token (if found)
Controller-->>Client: Proceeds with authentication
Note over Filter: On request processing
Filter->>CookieUtil: Calls getJwtTokenFromCookie(request)
CookieUtil->>CookieUtil: Finds "Jwttoken" cookie (case-insensitive)
Filter->>Filter: Checks/clears "userId" cookie (case-insensitive)
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
♻️ Duplicate comments (1)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (1)
1095-1095: Consistency issue: Case-insensitive cookie matching.This change mirrors the issue in CookieUtil.java. Cookie names should be case-sensitive per HTTP standards (RFC 6265). The case-insensitive matching here should be aligned with the cookie setting logic.
🧹 Nitpick comments (1)
src/main/java/com/iemr/common/utils/JwtUtil.java (1)
93-96: Consider adding error handling for robustness.The method implementation is correct and follows the existing pattern. However, consider adding explicit error handling for cases where the token is invalid or the "userId" claim is missing.
Consider this enhanced implementation:
public String getUserIdFromToken(String token) { - return getAllClaimsFromToken(token).get("userId", String.class); + try { + Claims claims = getAllClaimsFromToken(token); + if (claims == null) { + return null; + } + return claims.get("userId", String.class); + } catch (Exception e) { + // Log the exception if needed + return null; + } }
🛑 Comments failed to post (6)
src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java (1)
662-662: 💡 Verification agent
🧩 Analysis chain
Verify the necessity of exposing sensitive user data.
Changing the access modifier from private to public exposes user service role mapping functionality, which contains sensitive user data including roles and privileges. Ensure this exposure is necessary and that proper authorization checks are in place at the calling layer.
Run the following script to verify how this method is being used:
🏁 Script executed:
#!/bin/bash # Description: Check usage of getUserServiceRoleMapping method to ensure proper authorization # Search for all usages of this method rg -A 3 -B 3 "getUserServiceRoleMapping" # Look for any authorization or security checks in calling methods ast-grep --pattern 'public $_ $NAME($$$) { $$$ getUserServiceRoleMapping($_) $$$ }'Length of output: 8596
I’m going to check the controller for any Spring Security or role‐based annotations around the endpoint that calls this method:
🏁 Script executed:
#!/bin/bash # Search for any security annotations in the controller rg "@(PreAuthorize|Secured|RolesAllowed|PermitAll|DenyAll)" -n src/main/java/com/iemr/common/controller/users/IEMRAdminController.java || echo "No security annotations found" # Dump the first 100 lines to inspect class‐level annotations sed -n '1,100p' src/main/java/com/iemr/common/controller/users/IEMRAdminController.javaLength of output: 3952
I’ll look for any global Spring Security configuration to confirm whether these endpoints are protected:
🏁 Script executed:
#!/bin/bash # Check for WebSecurityConfigurerAdapter or SecurityFilterChain beans rg -l "@EnableWebSecurity" || echo "No @EnableWebSecurity annotations found" rg -l "extends WebSecurityConfigurerAdapter" || echo "No WebSecurityConfigurerAdapter subclasses found" rg -n "SecurityFilterChain" -n src/ || echo "No SecurityFilterChain definitions found" # Look for authorizeRequests/authorizeHttpRequests usage rg -n "authorizeRequests" -n src/ || echo "No authorizeRequests calls found" rg -n "authorizeHttpRequests" -n src/ || echo "No authorizeHttpRequests calls found"Length of output: 626
Ensure authorization on endpoints exposing sensitive role mappings
The IEMRAdminController exposes
getUserServiceRoleMapping(...)(viamUser.setM_UserServiceRoleMapping(...)) without any Spring Security annotations or global security configuration guarding access. By default, anyone with a valid session/request could call this and retrieve sensitive role/privilege data.Please add appropriate authorization checks:
• Apply method- or class-level annotations (e.g.
@PreAuthorize("hasRole('ADMIN')")) on the controller methods that invokegetUserServiceRoleMapping.
• Or introduce aSecurityFilterChain/WebSecurityConfigurerAdapterbean withauthorizeRequests()/authorizeHttpRequests()rules restricting access to/user/**endpoints.
• Ensure your existing JWT/session filter is correctly wired in Spring’s filter chain before controller dispatch.Locations needing fixes:
- src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (methods calling
getUserServiceRoleMapping)- No
@EnableWebSecurity,SecurityFilterChain, orauthorizeRequestsfound in the codebase🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java at line 662, the method getUserServiceRoleMapping was changed to public, exposing sensitive user role and privilege data. To fix this, do not restrict access only by method visibility; instead, add proper authorization checks on the controller methods in src/main/java/com/iemr/common/controller/users/IEMRAdminController.java that call getUserServiceRoleMapping. Apply Spring Security annotations like @PreAuthorize("hasRole('ADMIN')") on those controller methods or configure a SecurityFilterChain or WebSecurityConfigurerAdapter bean to restrict access to /user/** endpoints. Also, ensure the JWT/session filters are correctly integrated in the Spring filter chain before controller dispatch to enforce these restrictions. This will secure access to sensitive role mapping data while allowing necessary functionality.src/main/java/com/iemr/common/utils/CookieUtil.java (1)
72-72: 💡 Verification agent
🧩 Analysis chain
Security consideration: Case-insensitive cookie matching may violate HTTP standards.
Making cookie name comparison case-insensitive deviates from HTTP cookie standards (RFC 6265), where cookie names are case-sensitive. This change could potentially match unintended cookies and may introduce security vulnerabilities.
Consider reverting to case-sensitive comparison and ensuring consistent cookie naming throughout the application:
-return Arrays.stream(request.getCookies()).filter(cookie -> "Jwttoken".equalsIgnoreCase(cookie.getName())) +return Arrays.stream(request.getCookies()).filter(cookie -> "Jwttoken".equals(cookie.getName()))Please verify that all cookie setting locations use the exact same case for the cookie name "Jwttoken".
🏁 Script executed:
#!/bin/bash # Search for all cookie setting operations to verify naming consistency rg -A 3 -B 3 "Cookie.*Jwttoken|addCookie.*Jwttoken|setCookie.*Jwttoken"Length of output: 2235
Security & standards compliance: revert to case-sensitive cookie name matching
All cookie-setting operations use the exact name
"Jwttoken". UsingequalsIgnoreCaseat retrieval deviates from RFC 6265 (which treats cookie names as case-sensitive) and could allow unintended or malicious cookies to be accepted. Please revert to a strict, case-sensitive comparison and ensure no conflicting cookies can be set.Pinpoint location:
- src/main/java/com/iemr/common/utils/CookieUtil.java (line 72)
Suggested diff:
- return Arrays.stream(request.getCookies()) - .filter(cookie -> "Jwttoken".equalsIgnoreCase(cookie.getName())) + return Arrays.stream(request.getCookies()) + .filter(cookie -> "Jwttoken".equals(cookie.getName())) .map(Cookie::getValue) .findFirst() .orElse(null);📝 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.return Arrays.stream(request.getCookies()) .filter(cookie -> "Jwttoken".equals(cookie.getName())) .map(Cookie::getValue) .findFirst() .orElse(null);🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/utils/CookieUtil.java at line 72, revert the cookie name comparison from case-insensitive (equalsIgnoreCase) to case-sensitive (equals) to comply with RFC 6265 standards. Ensure the filter strictly matches the cookie name "Jwttoken" with exact casing, and verify that all cookie-setting code uses the same exact case to maintain consistency and security.src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (4)
489-490:
⚠️ Potential issueFix potential NullPointerException.
The code checks
authHeader.isEmpty()but doesn't handle the case whereauthHeaderis null, which could cause a NullPointerException.Apply this fix:
-String authHeader = request.getHeader("Authorization"); -if (authHeader.isEmpty()) { +String authHeader = request.getHeader("Authorization"); +if (authHeader == null || authHeader.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.String authHeader = request.getHeader("Authorization"); if (authHeader == null || authHeader.isEmpty()) {🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/controller/users/IEMRAdminController.java around lines 489 to 490, the code calls isEmpty() on authHeader without checking if authHeader is null, which can cause a NullPointerException. Fix this by first checking if authHeader is null or empty using a null-safe condition before calling isEmpty(), for example by using authHeader == null || authHeader.isEmpty().
491-509: 🛠️ Refactor suggestion
Clarify authentication token precedence and improve security.
The method now supports multiple JWT token sources (header, cookie) but doesn't clearly define precedence. This could lead to confusion and potential security issues if different tokens are present in different sources.
Consider implementing a clear token source precedence strategy:
-// Try JWT token from header first -String jwtToken = request.getHeader("Jwttoken"); - -// If not in header, try cookie -if (jwtToken == null) { +// Define clear precedence: Authorization header > Jwttoken header > Cookie +String jwtToken = null; + +// First priority: Jwttoken header +jwtToken = request.getHeader("Jwttoken"); + +// Second priority: Cookie (only if header not found) +if (jwtToken == null) {Also consider logging which authentication source was used for audit purposes.
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 491 to 509, clarify the precedence of JWT token sources by explicitly defining that the header token takes priority over the cookie token. After retrieving the token, add logging to record which source (header or cookie) provided the token for audit purposes. This ensures clear token source precedence and improves security by making token origin explicit.
531-533: 🛠️ Refactor suggestion
Improve exception handling specificity.
Catching all exceptions with
Exception emay hide specific authentication errors and make debugging difficult. Consider catching specific exception types for better error handling.-} catch (Exception e) { - logger.error("getLoginResponse failed with error " + e.getMessage(), e); +} catch (IEMRException e) { + logger.error("Authentication failed: " + e.getMessage(), e); + response.setError(e); +} catch (ExpiredJwtException e) { + logger.error("JWT token expired: " + e.getMessage(), e); + response.setError(new IEMRException("Token expired")); +} catch (Exception e) { + logger.error("Unexpected error in getLoginResponse: " + e.getMessage(), e);📝 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.} catch (IEMRException e) { logger.error("Authentication failed: " + e.getMessage(), e); response.setError(e); } catch (ExpiredJwtException e) { logger.error("JWT token expired: " + e.getMessage(), e); response.setError(new IEMRException("Token expired")); } catch (Exception e) { logger.error("Unexpected error in getLoginResponse: " + e.getMessage(), e); response.setError(e); }🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/controller/users/IEMRAdminController.java around lines 531 to 533, the code currently catches a generic Exception which can obscure specific authentication errors. Refactor the catch block to handle more specific exceptions related to authentication, such as AuthenticationException or other relevant subclasses, before catching a general Exception. This will improve error handling clarity and make debugging easier by distinguishing different failure cases.
499-499: 💡 Verification agent
🧩 Analysis chain
Cookie name inconsistency detected.
The code searches for a cookie named "jwtToken" (camelCase), but other parts of the codebase use "Jwttoken" (PascalCase). This inconsistency could lead to authentication failures.
Verify the correct cookie name and ensure consistency:
Consider standardizing on one naming convention throughout the application.
🏁 Script executed:
#!/bin/bash # Search for all cookie-related operations to identify the correct naming convention rg -A 2 -B 2 "Cookie.*[Jj]wt[Tt]oken|addCookie|setCookie" --type javaLength of output: 3617
Standardize JWT Cookie Name for Consistency
We’ve identified three different JWT cookie names in the codebase, which will prevent the controller from ever matching the cookie:
- In CookieUtil:
new Cookie("Jwttoken", …)- In JwtUserIdValidationFilter:
request.getHeader("JwtToken")- In IEMRAdminController (line 499):
if ("jwtToken".equals(cookie.getName())) {Please pick a single name (e.g.
"JwtToken"), expose it as a constant (e.g.CookieUtil.JWT_COOKIE_NAME), and update all occurrences accordingly:• src/main/java/com/iemr/common/utils/CookieUtil.java
– Changenew Cookie("Jwttoken", …)tonew Cookie(JWT_COOKIE_NAME, …)
– Changefilter(cookie -> "Jwttoken".equalsIgnoreCase(cookie.getName()))to use the same constant• src/main/java/com/iemr/common/utils/JwtUserIdValidationFilter.java
– UseJWT_COOKIE_NAMEwhen reading from headers or cookies• src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (around line 499)
– Replace
diff - if ("jwtToken".equals(cookie.getName())) { + if (JWT_COOKIE_NAME.equalsIgnoreCase(cookie.getName())) {This will ensure reliable cookie lookup and simplify future maintenance.
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 at line 499, the cookie name "jwtToken" is inconsistent with other parts of the codebase that use "Jwttoken" or "JwtToken". To fix this, define a constant JWT_COOKIE_NAME in src/main/java/com/iemr/common/utils/CookieUtil.java with the chosen standard name (e.g., "JwtToken") and replace all hardcoded cookie name strings in CookieUtil, JwtUserIdValidationFilter, and IEMRAdminController with this constant to ensure consistent cookie name usage across the application.
|
📋 Description
JIRA ID:
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