AMM-1239 : Role based broken access control #96
Conversation
WalkthroughThis update introduces Spring Security by adding its dependency, configuring security filters, and implementing custom exception handlers. It enforces role-based access control with JWT authentication, integrates Redis caching for user roles, and restricts access to multiple controllers based on user roles such as SUPERVISOR, QUALITY_SUPERVISOR, QUALITY_AUDITOR, ANM, MO, and ASSOCIATE. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RoleAuthenticationFilter
participant JwtUtil
participant RedisStorage
participant MasterServiceImpl
participant SecurityContext
participant Controller
Client->>RoleAuthenticationFilter: Sends HTTP request with JWT
RoleAuthenticationFilter->>JwtUtil: extractAllClaims(token)
JwtUtil-->>RoleAuthenticationFilter: Returns claims (userId)
RoleAuthenticationFilter->>RedisStorage: getUserRoleFromCache(userId)
alt Role found in cache
RedisStorage-->>RoleAuthenticationFilter: Returns roles
else Role not in cache
RoleAuthenticationFilter->>MasterServiceImpl: getUserRoles(userId)
MasterServiceImpl-->>RoleAuthenticationFilter: Returns roles
RoleAuthenticationFilter->>RedisStorage: cacheUserRoles(userId, roles)
end
RoleAuthenticationFilter->>SecurityContext: Set authentication with userId and roles
RoleAuthenticationFilter->>Controller: Continue filter chain (request proceeds)
sequenceDiagram
participant Client
participant Controller
participant SecurityContext
participant CustomAccessDeniedHandler
participant CustomAuthenticationEntryPoint
Client->>Controller: Access protected endpoint
Controller->>SecurityContext: Check authentication and role
alt Not authenticated
SecurityContext->>CustomAuthenticationEntryPoint: commence()
CustomAuthenticationEntryPoint-->>Client: 401 JSON error response
else Not authorized
SecurityContext->>CustomAccessDeniedHandler: handle()
CustomAccessDeniedHandler-->>Client: 403 JSON error response
else Authorized
Controller-->>Client: Normal response
end
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (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 (
|
![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: 16
🔭 Outside diff range comments (1)
src/main/java/com/iemr/ecd/utils/mapper/JwtUtil.java (1)
62-68: 🛠️ Refactor suggestionConsider security implications of exposing JWT parsing method.
Making
extractAllClaimspublic exposes JWT parsing without the security checks present invalidateToken(denylist check, exception handling). This could lead to security vulnerabilities if misused.Consider these alternatives:
- Add security wrapper: Create a public method that includes denylist checks
- Use validateToken: Callers should use
validateTokeninstead and extract claims from the resultpublic Claims extractAllClaims(String token) { + // Consider using validateToken() instead for security checks return Jwts.parser() .verifyWith(getSigningKey()) .build() .parseSignedClaims(token) .getPayload(); }Or better yet, add a secure public method:
public Claims extractAllClaimsSecurely(String token) { return validateToken(token); // Includes denylist and exception handling }
🧹 Nitpick comments (4)
src/main/java/com/iemr/ecd/utils/mapper/SecurityConfig.java (2)
34-34: Document CSRF disabling decision.CSRF protection is disabled without explanation. While this is common for stateless APIs using JWT tokens, it should be documented for security auditing purposes.
Add a comment explaining why CSRF is disabled:
- .csrf(csrf -> csrf.disable()) + .csrf(csrf -> csrf.disable()) // Disabled for stateless JWT authentication
1-1: Consider moving SecurityConfig to a more appropriate package.The
SecurityConfigclass is currently placed in theutils.mapperpackage, which doesn't align with its purpose. Consider moving it to a dedicated security configuration package likecom.iemr.ecd.config.security.src/main/java/com/iemr/ecd/utils/redis/RedisStorage.java (2)
100-101: Consider consolidating Redis access patterns.The class now uses both
LettuceConnectionFactory(for session management) andRedisTemplate(for role caching). Consider standardizing on one approach for consistency and maintainability.
104-104: Consider improving the cache key naming convention.The current key format
"role:" + userIdis basic. Consider using a more descriptive format like"user:role:" + userIdto avoid potential key collisions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
pom.xml(1 hunks)src/main/java/com/iemr/ecd/controller/dataupload/DataUploadController.java(2 hunks)src/main/java/com/iemr/ecd/repository/masters/RoleRepo.java(2 hunks)src/main/java/com/iemr/ecd/service/masters/MasterServiceImpl.java(1 hunks)src/main/java/com/iemr/ecd/utils/advice/exception_handler/CustomAccessDeniedHandler.java(1 hunks)src/main/java/com/iemr/ecd/utils/advice/exception_handler/CustomAuthenticationEntryPoint.java(1 hunks)src/main/java/com/iemr/ecd/utils/mapper/JwtUtil.java(1 hunks)src/main/java/com/iemr/ecd/utils/mapper/RoleAuthenticationFilter.java(1 hunks)src/main/java/com/iemr/ecd/utils/mapper/SecurityConfig.java(1 hunks)src/main/java/com/iemr/ecd/utils/redis/RedisStorage.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (10)
pom.xml (1)
248-251: LGTM! Spring Security dependency properly added.The
spring-boot-starter-securitydependency is correctly added and will inherit the appropriate version from the Spring Boot parent (3.2.2), ensuring compatibility.src/main/java/com/iemr/ecd/controller/dataupload/DataUploadController.java (2)
28-28: LGTM! Security import properly added.The
PreAuthorizeimport is correctly added to support role-based access control.
46-46: LGTM! Appropriate role-based access control implemented.The
@PreAuthorize("hasRole('SUPERVISOR')")annotation correctly restricts access to supervisor-level users, which is appropriate for data upload functionality. The class-level placement ensures all endpoints inherit this security requirement.src/main/java/com/iemr/ecd/repository/masters/RoleRepo.java (2)
26-28: LGTM! Required imports properly added.The
QueryandParamannotations are correctly imported to support the new repository method.
37-38:Details
❓ Verification inconclusive
Review native query for multiple roles scenario.
The native query is well-formed and uses proper parameter binding to prevent SQL injection. However, there's a potential issue with the return type:
Concern: The query could return multiple role names if a user has multiple role assignments, but the method returns
Stringinstead ofList<String>.Run this script to verify if users can have multiple roles:
If users can have multiple roles, consider:
- Using
GROUP_CONCATin the query to concatenate roles- Changing return type to
List<String>- Adding
LIMIT 1if only the first role is needed
🏁 Script executed:
#!/bin/bash # Description: Check if any user has multiple role assignments # Expected: If output shows users with multiple roles, the method needs adjustment ast-grep --pattern $'select $$$ from m_userservicerolemapping $$$' # Also search for any GROUP_CONCAT or similar handling rg -A 5 -B 5 "m_userservicerolemapping.*group|GROUP_CONCAT|LISTAGG"Length of output: 138
Verify role cardinality and adjust return type
The current native query can return multiple rows if the
m_userservicerolemappingtable allows multiple role assignments per user, but the repository method is defined to return a singleString. Please confirm the intended behavior and update accordingly:
- If users may have multiple roles:
- Change the method signature to return
List<String>- Rename it (e.g.) to
getRoleNamesByUserId- Add
import java.util.List;- If each user must have at most one role:
- Enforce
LIMIT 1in the SQL- Or add a uniqueness constraint on
(userid)inm_userservicerolemappingExample diff for multiple roles:
--- a/src/main/java/com/iemr/ecd/repository/masters/RoleRepo.java +++ b/src/main/java/com/iemr/ecd/repository/masters/RoleRepo.java @@ -36,7 +36,8 @@ public interface RoleRepo extends JpaRepository<Role, Long> { @Query(nativeQuery = true, value = "select rolename from m_role where roleid in (select roleid from m_userservicerolemapping where userid=:userID)") - String getRoleNameByUserId(@Param("userID") Long userID); + List<String> getRoleNamesByUserId(@Param("userID") Long userID); }src/main/java/com/iemr/ecd/utils/mapper/SecurityConfig.java (3)
37-37:Details
❓ Verification inconclusive
Consider more specific path patterns for public endpoints.
The broad
/user/**pattern permits all sub-paths under/user/. Consider if this aligns with your security requirements or if more specific endpoints should be public.Verify that all endpoints under
/user/**should be publicly accessible:
🏁 Script executed:
#!/bin/bash # Find all controller methods with /user mapping ast-grep --pattern $'@RequestMapping($$$) public $_ $$$($$$) { $$$ }' | grep -A 10 -B 5 "/user" # Also check for @GetMapping, @PostMapping etc with /user paths rg -A 5 -B 2 '@(Get|Post|Put|Delete|Patch)Mapping.*"/user'Length of output: 157
Let’s broaden the search for any occurrence of “/user” in your Java sources to catch both class- and method-level mappings:
🏁 Script executed:
#!/bin/bash # Find any hard-coded "/user" strings in Java source files rg -n '"/user' --glob '*.java' # Specifically look for @RequestMapping or other mapping annotations referencing /user rg -n '@(Request|Get|Post|Put|Delete|Patch)Mapping.*"/user' --glob '*.java'Length of output: 653
Let’s locate your User‐related controllers and inspect their mappings to see exactly which
/user/**endpoints exist:
🏁 Script executed:
#!/bin/bash # 1. Find any Java files named *User*Controller.java fd -e java -g '*User*Controller.java' # 2. Show mapping annotations inside those controller files fd -e java -g '*User*Controller.java' | xargs -r rg -Hn '@(Request|Get|Post|Put|Delete|Patch)Mapping' # 3. Also check for any class-level @RequestMapping("/user") rg -Hn '@RequestMapping.*"/user"' --glob '*.java'Length of output: 195
Lock down
/user/**permits
Your.requestMatchers("/user/**").permitAll()rule currently opens every/user/...route without authentication. Based on yourJwtUserIdValidationFilter, it looks like only these endpoints actually need to be public:
/user/userAuthenticate/user/refreshToken/user/logOutUserFromConcurrentSessionConsider replacing the blanket pattern with explicit matchers:
- .requestMatchers("/user/**").permitAll() + .requestMatchers( + "/user/userAuthenticate", + "/user/refreshToken", + "/user/logOutUserFromConcurrentSession" + ).permitAll()• Verify your controller mappings (including any path constants) to ensure no other
/user/...routes remain exposed.
• Double-checkJwtUserIdValidationFilterand all your controllers for/userpaths.
43-43: Good implementation of custom authentication filter placement.The filter is correctly positioned before
UsernamePasswordAuthenticationFilterto intercept requests for JWT authentication.
37-37:Details
❓ Verification inconclusive
Verify the security implications of permitting all requests to /user/**.
The configuration allows unrestricted access to all endpoints under
/user/**. Ensure this is intentional and that no sensitive operations are exposed through these endpoints.Run the following script to identify endpoints under the /user path:
🏁 Script executed:
#!/bin/bash # Description: Find all controller methods mapped to /user/** paths # Expected: List of endpoints that will be publicly accessible ast-grep --pattern $'@RequestMapping($$$) class $_ { $$$ }' ast-grep --pattern $'@GetMapping("$_") $_ $$$($$$) { $$$ }' ast-grep --pattern $'@PostMapping("$_") $_ $$$($$$) { $$$ }' rg -A 5 '@.*Mapping.*"/user'Length of output: 220
🏁 Script executed:
#!/bin/bash # Re-run searches to catch all /user/** mappings in the Java codebase # 1. Class‐level RequestMapping for "/user…" patterns rg -n '@RequestMapping.*"/user' --context 3 src/main/java # 2. Method‐level Get/Post/Put/Delete/Patch mappings for "/user…" patterns rg -n '@(Get|Post|Put|Delete|Patch)Mapping.*"/user' --context 3 src/main/java # 3. Any raw occurrences of "/user" in Java source (to catch string‐based usages) rg -n '"\/user' src/main/javaLength of output: 246
#!/bin/bash # Refined search for any controller mappings referencing /user # 1. Class‐level @RequestMapping with path or value attr rg -n '@RequestMapping.*(path|value)\s*=\s*".*?/user' --context 3 src/main/java # 2. Method‐level @Get|Post|Put|Delete|PatchMapping with path/value or direct string rg -n '@(Get|Post|Put|Delete|Patch)Mapping.*(path|value)\s*=\s*".*?/user' --context 3 src/main/java # 3. Generic literal search for "/user" in Java sources rg -n '"/user' src/main/java
Please manually verify all
/user/**endpoints
I couldn’t find any controller mappings under/user/**in the codebase, yet the security config at line 37 opens them to everyone. Ensure this is intentional and that none of the/user/**paths expose sensitive functionality.
- Location:
src/main/java/com/iemr/ecd/utils/mapper/SecurityConfig.java(line 37)
.requestMatchers("/user/**").permitAll()- No controller methods or class‐level mappings for
/user/**were detected—please confirm there aren’t any hidden or dynamic endpoints under this path.src/main/java/com/iemr/ecd/utils/mapper/RoleAuthenticationFilter.java (2)
61-71: JWT cookie extraction looks correct.The method properly iterates through cookies and performs case-insensitive matching for the JWT token name.
42-42:Details
❌ Incorrect review comment
Missing JWT token validation.
The filter extracts claims from the JWT token without validating the token's signature, expiration, or other security properties.
Ensure
jwtUtil.extractAllClaims()includes proper token validation:
🏁 Script executed:
#!/bin/bash # Description: Check if JwtUtil properly validates JWT tokens # Expected: Methods that validate signature, expiration, etc. ast-grep --pattern $'class JwtUtil { $$$ extractAllClaims($$$) { $$$ } $$$ }' rg -A 10 "extractAllClaims"Length of output: 2530
JWT validation is already implemented in JwtUtil.extractAllClaims
The call to
Jwts.parser() .verifyWith(getSigningKey()) .build() .parseSignedClaims(token) .getPayload();in
JwtUtil.extractAllClaims(...)validates the signature (viaverifyWith) and, by default, enforces expiration checks. No additional JWT validation is required in the filter.Likely an incorrect or invalid review comment.
src/main/java/com/iemr/ecd/service/masters/MasterServiceImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iemr/ecd/utils/advice/exception_handler/CustomAccessDeniedHandler.java
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,47 @@ | |||
| package com.iemr.ecd.utils.mapper; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move configuration class to appropriate package.
Configuration classes should typically be placed in a config package rather than utils.mapper. This improves code organization and follows Spring Boot conventions.
Move this class to com.iemr.ecd.config.SecurityConfig.
🤖 Prompt for AI Agents
In src/main/java/com/iemr/ecd/utils/mapper/SecurityConfig.java at line 1, the
SecurityConfig class is currently placed in the utils.mapper package, which is
not appropriate for configuration classes. Move the SecurityConfig.java file to
the package com.iemr.ecd.config by changing its directory location and updating
the package declaration at the top of the file to com.iemr.ecd.config. This
aligns with Spring Boot conventions and improves project organization.
src/main/java/com/iemr/ecd/utils/mapper/RoleAuthenticationFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iemr/ecd/utils/mapper/RoleAuthenticationFilter.java
Outdated
Show resolved
Hide resolved
| private RedisTemplate<String, String> redisTemplate; | ||
|
|
||
| public void cacheUserRole(Long userId, String role) { | ||
| redisTemplate.opsForValue().set("role:" + userId, role, Duration.ofHours(1)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make cache expiration time configurable.
The 1-hour expiration is hardcoded. Consider making it configurable similar to the existing sessionExpiryTimeInSec property.
+private @Value("${iemr.role.cache.expiry.hours:1}") int roleCacheExpiryHours;
-redisTemplate.opsForValue().set("role:" + userId, role, Duration.ofHours(1));
+redisTemplate.opsForValue().set("role:" + userId, role, Duration.ofHours(roleCacheExpiryHours));🤖 Prompt for AI Agents
In src/main/java/com/iemr/ecd/utils/redis/RedisStorage.java at line 104, the
cache expiration time is hardcoded to 1 hour. Modify the code to use a
configurable property for expiration time, similar to the existing
sessionExpiryTimeInSec property. Retrieve the expiration duration from
configuration and apply it in the redisTemplate.opsForValue().set method instead
of the fixed Duration.ofHours(1).
src/main/java/com/iemr/ecd/utils/mapper/RoleAuthenticationFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iemr/ecd/utils/mapper/RoleAuthenticationFilter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/iemr/ecd/service/masters/MasterServiceImpl.java(1 hunks)src/main/java/com/iemr/ecd/utils/advice/exception_handler/CustomAccessDeniedHandler.java(1 hunks)src/main/java/com/iemr/ecd/utils/mapper/RoleAuthenticationFilter.java(1 hunks)src/main/java/com/iemr/ecd/utils/redis/RedisStorage.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/com/iemr/ecd/utils/mapper/RoleAuthenticationFilter.java
- src/main/java/com/iemr/ecd/utils/advice/exception_handler/CustomAccessDeniedHandler.java
- src/main/java/com/iemr/ecd/utils/redis/RedisStorage.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
| public String getUserRole(Long userId) { | ||
| if (null == userId || userId <= 0) { | ||
| throw new IllegalArgumentException("Invalid User ID : " + userId); | ||
| } | ||
| try { | ||
| String role = roleRepo.getRoleNamebyUserId(userId); | ||
| if (null == role || role.trim().isEmpty()) { | ||
| throw new ECDException("No role found for userId : " + userId); | ||
| } | ||
| return role; | ||
| } catch (Exception e) { | ||
| throw new ECDException("Failed to retrieverole for usedId : " + userId + " error : " + e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix typos in exception message.
The implementation correctly follows the previous suggestions for input validation and error handling. However, there are typos in the exception message that need correction:
- throw new ECDException("Failed to retrieverole for usedId : " + userId + " error : " + e.getMessage());
+ throw new ECDException("Failed to retrieve role for userId : " + userId + " error : " + e.getMessage());Additionally, consider preserving the original exception as a cause for better debugging:
- throw new ECDException("Failed to retrieve role for userId : " + userId + " error : " + e.getMessage());
+ throw new ECDException("Failed to retrieve role for userId : " + userId, 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.
| public String getUserRole(Long userId) { | |
| if (null == userId || userId <= 0) { | |
| throw new IllegalArgumentException("Invalid User ID : " + userId); | |
| } | |
| try { | |
| String role = roleRepo.getRoleNamebyUserId(userId); | |
| if (null == role || role.trim().isEmpty()) { | |
| throw new ECDException("No role found for userId : " + userId); | |
| } | |
| return role; | |
| } catch (Exception e) { | |
| throw new ECDException("Failed to retrieverole for usedId : " + userId + " error : " + e.getMessage()); | |
| } | |
| } | |
| public String getUserRole(Long userId) { | |
| if (null == userId || userId <= 0) { | |
| throw new IllegalArgumentException("Invalid User ID : " + userId); | |
| } | |
| try { | |
| String role = roleRepo.getRoleNamebyUserId(userId); | |
| if (null == role || role.trim().isEmpty()) { | |
| throw new ECDException("No role found for userId : " + userId); | |
| } | |
| return role; | |
| } catch (Exception e) { | |
| throw new ECDException("Failed to retrieve role for userId : " + userId, e); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/ecd/service/masters/MasterServiceImpl.java around
lines 232 to 245, fix the typos in the exception messages by correcting
"retrieverole" to "retrieve role" and "usedId" to "userId". Also, update the
catch block to pass the caught exception as the cause when throwing the new
ECDException to preserve the original stack trace for better debugging.
| } | ||
|
|
||
| } | ||
| private String getJwtTokenFromCookies(HttpServletRequest request) { |
There was a problem hiding this comment.
this method seems to be appearing in a lot of files. Can't we reuse?
There was a problem hiding this comment.
Yes, I have updated
|
|
||
| @RestController | ||
| @CrossOrigin() | ||
| @PreAuthorize("hasRole('SUPERVISOR')") |
There was a problem hiding this comment.
Didn't follow where this hasRole is checked.
There was a problem hiding this comment.
Please verify In RoleAuthenticationFilter line no 66
| .csrf(csrf -> csrf.csrfTokenRepository(CookieCsrfTokenRepository.withHttpOnlyFalse())) | ||
| .sessionManagement(session -> session.sessionCreationPolicy(SessionCreationPolicy.STATELESS)) | ||
| .authorizeHttpRequests(auth -> auth | ||
| .requestMatchers("/user/*").permitAll() |
There was a problem hiding this comment.
If the requestURL coming like 'user/userAuhenticate' it will give the permission it will not check the role.
| public void cacheUserRole(Long userId, String role) { | ||
| try { | ||
| redisTemplate.opsForValue().set("role:" + userId, role, Duration.ofHours(1)); | ||
| }catch (Exception e) { |
| @Bean | ||
| public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception { | ||
| http | ||
| .csrf(csrf -> csrf.csrfTokenRepository(CookieCsrfTokenRepository.withHttpOnlyFalse())) |
There was a problem hiding this comment.
withHttpOnlyFalse
this is creating a security hotspot. please check.
EcdQuestionareController
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/iemr/ecd/controller/questionare/EcdQuestionareController.java (1)
61-61: Verify class-level@PreAuthorize('SUPERVISOR')on controllersBoth of the following controllers apply a blanket SUPERVISOR restriction at the class level, with no method-level overrides found:
- src/main/java/com/iemr/ecd/controller/questionare/EcdQuestionareController.java
- src/main/java/com/iemr/ecd/controller/dataupload/DataUploadController.java
This means every endpoint in these classes—including read-only GETs—now requires the SUPERVISOR role. Before rolling this out, please ensure:
- Every operation truly requires SUPERVISOR privileges (e.g., should GET endpoints be open to lower roles?).
- If not, migrate to method-level
@PreAuthorize(...)on specific handlers and remove or loosen the class-level restriction.- Coordinate any user-role updates with your team to prevent unexpected access failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/ecd/controller/questionare/EcdQuestionareController.java(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: sandipkarmakar3
PR: PSMRI/ECD-API#48
File: src/main/java/com/iemr/ecd/config/RedisConfig.java:37-37
Timestamp: 2025-02-03T12:54:04.136Z
Learning: Redis security configuration (password, SSL/TLS, timeouts) is not required for this project as per the team's decision.
🔇 Additional comments (1)
src/main/java/com/iemr/ecd/controller/questionare/EcdQuestionareController.java (1)
30-30: LGTM - Necessary import added.The import for
@PreAuthorizeis correctly added to support the security annotation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/com/iemr/ecd/utils/mapper/RoleAuthenticationFilter.java (1)
41-86: Performance concern: Filter runs for all requests.The filter processes every request, including those that don't require authentication. This causes unnecessary JWT parsing and database calls.
Override
shouldNotFiltermethod to skip processing for excluded paths:+@Override +protected boolean shouldNotFilter(HttpServletRequest request) { + String path = request.getServletPath(); + return path.startsWith("/user/") || + path.startsWith("/actuator/") || + path.startsWith("/swagger-") || + path.startsWith("/v3/api-docs"); +}
🧹 Nitpick comments (2)
src/main/java/com/iemr/ecd/controller/quality/QualityAuditController.java (1)
60-60: Role-based access control implemented with consideration needed.The
@PreAuthorize("hasRole('QUALITY_AUDITOR')")annotation restricts access to users with the QUALITY_AUDITOR role. However, consider if all endpoints in this controller require the same access level. For example, some endpoints like/getQualityAuditGradesmight be read-only operations that could be accessed by supervisors as well.Consider if some endpoints need different access levels:
// For read-only operations, you might want to allow supervisors as well @PreAuthorize("hasRole('QUALITY_AUDITOR') || hasRole('QUALITY_SUPERVISOR')") @GetMapping("/getQualityAuditGrades/{psmId}") public ResponseEntity<List<GradeConfiguration>> getQualityAuditGrades(@PathVariable Integer psmId) { // ... }src/main/java/com/iemr/ecd/utils/mapper/RoleAuthenticationFilter.java (1)
65-70: Improve role processing robustness.The role processing logic could be more robust with additional validation and error handling.
Consider adding validation for empty roles list:
List<String> roles = userService.getUserRoles(Long.valueOf(userId)); +if (roles == null || roles.isEmpty()) { + logger.warn("No roles found for user: {}", userId); + filterChain.doFilter(request, response); + return; +} authRoles = roles.stream() .filter(Objects::nonNull) .map(String::trim) + .filter(role -> !role.isEmpty()) .map(role -> "ROLE_" + role.toUpperCase().replace(" ", "_")) .collect(Collectors.toList());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/main/java/com/iemr/ecd/controller/dataupload/DataTemplateController.java(2 hunks)src/main/java/com/iemr/ecd/controller/outboundworklist/CallStatisticsController.java(2 hunks)src/main/java/com/iemr/ecd/controller/outboundworklist/OutBoundWorklistController.java(2 hunks)src/main/java/com/iemr/ecd/controller/quality/ChartsController.java(2 hunks)src/main/java/com/iemr/ecd/controller/quality/GradeConfigurationController.java(2 hunks)src/main/java/com/iemr/ecd/controller/quality/QualityAuditController.java(2 hunks)src/main/java/com/iemr/ecd/controller/quality/QualityAuditQuestionConfigurationController.java(2 hunks)src/main/java/com/iemr/ecd/controller/quality/QualityAuditSectionConfigurationController.java(2 hunks)src/main/java/com/iemr/ecd/controller/quality/SampleSelectionConfigurationController.java(2 hunks)src/main/java/com/iemr/ecd/repository/masters/RoleRepo.java(2 hunks)src/main/java/com/iemr/ecd/service/masters/MasterServiceImpl.java(1 hunks)src/main/java/com/iemr/ecd/utils/mapper/RoleAuthenticationFilter.java(1 hunks)src/main/java/com/iemr/ecd/utils/redis/RedisStorage.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/com/iemr/ecd/service/masters/MasterServiceImpl.java
- src/main/java/com/iemr/ecd/repository/masters/RoleRepo.java
- src/main/java/com/iemr/ecd/utils/redis/RedisStorage.java
🧰 Additional context used
🧠 Learnings (1)
src/main/java/com/iemr/ecd/utils/mapper/RoleAuthenticationFilter.java (1)
Learnt from: sandipkarmakar3
PR: PSMRI/ECD-API#48
File: src/main/java/com/iemr/ecd/utils/mapper/JwtUtil.java:32-40
Timestamp: 2025-02-03T12:54:54.932Z
Learning: Token replay protection using nonce claims and token blacklisting was deemed unnecessary for the JWT implementation in the ECD-API project.
🧬 Code Graph Analysis (1)
src/main/java/com/iemr/ecd/utils/mapper/RoleAuthenticationFilter.java (3)
src/main/java/com/iemr/ecd/utils/mapper/JwtUtil.java (1)
Component(14-69)src/main/java/com/iemr/ecd/utils/redis/RedisStorage.java (1)
Component(42-123)src/main/java/com/iemr/ecd/utils/constants/Constants.java (1)
Constants(5-24)
🔇 Additional comments (14)
src/main/java/com/iemr/ecd/controller/dataupload/DataTemplateController.java (2)
32-32: LGTM! Security import added correctly.The import for
@PreAuthorizeannotation is correctly added to support role-based access control.
53-53: Approve @PreAuthorize("hasRole('SUPERVISOR')"): Consistent Role Usage VerifiedThe
SUPERVISORrole is applied uniformly across all relevant controllers, including:
- src/main/java/com/iemr/ecd/controller/dataupload/DataTemplateController.java
- src/main/java/com/iemr/ecd/controller/dataupload/DataUploadController.java
- src/main/java/com/iemr/ecd/controller/questionare/EcdQuestionareController.java
- src/main/java/com/iemr/ecd/controller/quality/ChartsController.java
- src/main/java/com/iemr/ecd/controller/outboundworklist/CallStatisticsController.java
No further changes required.
src/main/java/com/iemr/ecd/controller/quality/GradeConfigurationController.java (2)
30-30: Security import correctly added.The import for
@PreAuthorizeannotation is properly added to enable role-based access control.
52-52: Role-based access control properly implemented.The
@PreAuthorize("hasRole('QUALITY_SUPERVISOR')")annotation correctly restricts access to users with the QUALITY_SUPERVISOR role, which is appropriate for grade configuration operations.src/main/java/com/iemr/ecd/controller/quality/ChartsController.java (2)
30-30: Security import correctly added.The import statement for
@PreAuthorizeannotation is properly added.
49-49: Multi-role access control properly implemented.The
@PreAuthorizeannotation correctly allows access to users with any of the three roles (SUPERVISOR, QUALITY_SUPERVISOR, or QUALITY_AUDITOR). This is appropriate for chart viewing operations where multiple user types need access to quality analytics.src/main/java/com/iemr/ecd/controller/quality/QualityAuditSectionConfigurationController.java (2)
30-30: Security import correctly added.The import for
@PreAuthorizeannotation is properly added to support role-based access control.
52-52: Role-based access control properly implemented.The
@PreAuthorize("hasRole('QUALITY_SUPERVISOR') || hasRole('QUALITY_AUDITOR')")annotation correctly allows access to users with either QUALITY_SUPERVISOR or QUALITY_AUDITOR roles, which is appropriate for quality audit section configuration operations.src/main/java/com/iemr/ecd/controller/quality/QualityAuditController.java (1)
30-30: Security import correctly added.The import for
@PreAuthorizeannotation is properly added to enable role-based access control.src/main/java/com/iemr/ecd/controller/outboundworklist/OutBoundWorklistController.java (1)
31-31: LGTM: Role-based access control properly implemented.The security annotation correctly restricts access to users with ANM, MO, or ASSOCIATE roles, which is appropriate for outbound worklist operations. The class-level annotation ensures all endpoints are secured.
Also applies to: 53-53
src/main/java/com/iemr/ecd/controller/quality/SampleSelectionConfigurationController.java (1)
30-30: LGTM: Appropriate role restrictions for quality management.The security annotation correctly restricts access to QUALITY_SUPERVISOR and QUALITY_AUDITOR roles, which is appropriate for sample selection configuration operations in quality management.
Also applies to: 52-52
src/main/java/com/iemr/ecd/controller/quality/QualityAuditQuestionConfigurationController.java (1)
30-30: LGTM: Consistent role-based security for quality operations.The security annotation correctly restricts access to QUALITY_SUPERVISOR and QUALITY_AUDITOR roles, maintaining consistency with other quality management controllers.
Also applies to: 53-53
src/main/java/com/iemr/ecd/controller/outboundworklist/CallStatisticsController.java (1)
28-28: LGTM: Appropriate multi-role access for call statistics.The security annotation correctly restricts access to supervisory and quality audit roles, which is appropriate for call statistics monitoring functionality.
Also applies to: 47-47
src/main/java/com/iemr/ecd/utils/mapper/RoleAuthenticationFilter.java (1)
41-86: Good improvements addressing previous security concerns.The filter now includes proper null checks, exception handling, and security context management that address the critical issues from previous reviews.
src/main/java/com/iemr/ecd/utils/mapper/RoleAuthenticationFilter.java
Outdated
Show resolved
Hide resolved
|
2 participants
📋 Description
JIRA ID: AMM-1239
This PR introduces role-based API-level security using the @PreAuthorize annotation from Spring Security. It allows fine-grained access control to service or controller methods based on user roles.
✅ 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
Improvements