Role Based Broken Access Control Implementation : WASA #139
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughComprehensive Spring Security integration introducing role-based access control across the application. Adds a security configuration with JWT authentication filter, custom exception handlers, and method-level authorization via Changes
Sequence DiagramsequenceDiagram
participant Client
participant RoleAuthFilter as RoleAuthenticationFilter
participant JwtUtil
participant Redis
participant UserService as UserLoginRepo
participant Controller
participant SecurityContext
Client->>RoleAuthFilter: HTTP Request (JWT in cookie/header)
alt JWT Present
RoleAuthFilter->>JwtUtil: validateToken(jwt)
JwtUtil-->>RoleAuthFilter: Claims (if valid)
RoleAuthFilter->>RoleAuthFilter: Extract userId from claims
RoleAuthFilter->>Redis: getUserRoleFromCache(userId)
alt Roles in Cache
Redis-->>RoleAuthFilter: List<String> roles
else Roles Not Cached
RoleAuthFilter->>UserService: getRoleNamebyUserId(userId)
UserService-->>RoleAuthFilter: List<String> roles
RoleAuthFilter->>Redis: cacheUserRoles(userId, roles)
Redis-->>RoleAuthFilter: Done
end
RoleAuthFilter->>RoleAuthFilter: Build GrantedAuthorities from roles
RoleAuthFilter->>SecurityContext: Set UsernamePasswordAuthenticationToken
SecurityContext-->>RoleAuthFilter: Stored
else JWT Missing/Empty
RoleAuthFilter-->>Client: Continue (unauthenticated)
end
RoleAuthFilter->>Controller: doFilter → `@PreAuthorize` check
alt Authorization Passed
Controller-->>Client: 200 Response
else Authorization Failed
Controller->>CustomAccessDeniedHandler: handle()
CustomAccessDeniedHandler-->>Client: 403 Forbidden JSON
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas requiring attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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. Comment |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@vanitha1822, implementation looks fine. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (12)
src/main/java/com/iemr/mmu/controller/labtechnician/LabTechnicianController.java (1)
27-28: Lab technician controller correctly locked down to lab rolesApplying a class-level
@PreAuthorize("hasRole('LABTECHNICIAN') || hasRole('LAB_TECHNICIAN') ")is consistent with the controller’s purpose and keeps the endpoints reserved for lab users.If you want to keep expressions tidy and consistent, you can drop the trailing space inside the string and consider extracting a shared constant/annotation for the repeated lab-tech role expression used in other controllers.
Also applies to: 46-49
src/main/java/com/iemr/mmu/utils/redis/RedisStorage.java (1)
24-25: Harden Redis role caching: handle empty lists, logging, and injection styleThe new Redis-backed role cache is a good addition but can be made more robust with a few small tweaks:
- Avoid pushing empty role lists:
rightPushAll(key, roles)will throw for an empty collection. Guard it and just clear/delete the key when there are no roles.- Return empty lists instead of
null:getUserRoleFromCachecurrently returnsnullon error; returningCollections.emptyList()simplifies caller logic and matches Sonar’s suggestion.- Fix log formatting:
logger.info("get user role from cache", userId);ignoresuserIdbecause there’s no{}placeholder.You could adjust along these lines:
-import java.util.List; +import java.util.List; +import java.util.Collections; @@ - @Autowired - private RedisTemplate<String, String> redisTemplate; + @Autowired + private RedisTemplate<String, String> redisTemplate; // consider constructor injection/qualifier if multiple templates exist @@ - public void cacheUserRoles(Long userId, List<String> roles) { + public void cacheUserRoles(Long userId, List<String> roles) { try { - logger.info("Caching roles for user {} : {} ", userId, roles); - String key = "roles:" + userId; - redisTemplate.delete(key); // Clear previous cache - redisTemplate.opsForList().rightPushAll(key, roles); + logger.info("Caching roles for user {} : {}", userId, roles); + String key = "roles:" + userId; + redisTemplate.delete(key); // Clear previous cache + if (roles != null && !roles.isEmpty()) { + redisTemplate.opsForList().rightPushAll(key, roles); + } } catch (Exception e) { logger.warn("Failed to cache role for user {} : {} ", userId, e.getMessage()); } @@ - public List<String> getUserRoleFromCache(Long userId) { + public List<String> getUserRoleFromCache(Long userId) { try { - logger.info("get user role from cache",userId); - return redisTemplate.opsForList().range("roles:" + userId, 0, -1); + logger.info("Get user roles from cache for user {}", userId); + List<String> roles = redisTemplate.opsForList().range("roles:" + userId, 0, -1); + return roles != null ? roles : Collections.emptyList(); } catch (Exception e) { logger.warn("Failed to retrieve cached role for user {} : {} ", userId, e.getMessage()); - return null; + return Collections.emptyList(); } }[snake_case tags: reason + external lib]
Because this relies on Spring Data Redis and Boot’s Redis auto-configuration, please also verify:
- That there isn’t an ambiguity between multiple
RedisTemplatebeans (e.g., defaultredisTemplatevs.stringRedisTemplate). If you see aNoUniqueBeanDefinitionException, injectStringRedisTemplateor use@Qualifierto pick the intended bean.- That callers of
getUserRoleFromCachecorrectly treat an empty list as “no cached roles” rather than an error.Also applies to: 32-33, 112-135
src/main/java/com/iemr/mmu/controller/common/master/CommonMasterController.java (1)
27-28: Master-data RBAC mapping is sensible; consider DRYing shared role expressions and confirm authority namingThe added
@PreAuthorizeconstraints align well with endpoint intent:
- Visit reasons & nurse master data →
NURSEorDOCTOR- Doctor master data →
DOCTOR- ECG abnormalities → lab-tech roles
This is consistent with the new RBAC model.
Two follow-ups to consider:
- DRY up repeated role expressions:
hasRole('NURSE') || hasRole('DOCTOR')and similar constructs appear in multiple controllers. You could centralize these as:
- A custom meta-annotation (e.g.,
@NurseOrDoctorOnly) or- A SpEL helper, e.g.,
@PreAuthorize("@roleChecker.isNurseOrDoctor()")
to improve maintainability.- Verify authority naming vs
hasRole: Ensure yourGrantedAuthoritystrings actually use theROLE_prefix expected byhasRole(e.g.,ROLE_NURSE,ROLE_DOCTOR,ROLE_LAB_TECHNICIAN). If your stored names are bare (e.g.,"NURSE"), you may want to switch these checks tohasAuthority('NURSE')instead.Also applies to: 57-60, 74-77, 95-98, 110-113
src/main/java/com/iemr/mmu/utils/mapper/RoleAuthenticationFilter.java (2)
17-31: Remove unused imports and fix incorrect IOException import.Several imports are unused, and
io.jsonwebtoken.io.IOExceptionis imported instead of using onlyjava.io.IOException.-import com.iemr.mmu.service.common.transaction.CommonServiceImpl; -import com.iemr.mmu.service.login.IemrMmuLoginServiceImpl; import com.iemr.mmu.utils.CookieUtil; import com.iemr.mmu.utils.JwtAuthenticationUtil; -import com.iemr.mmu.utils.JwtUserIdValidationFilter; import com.iemr.mmu.utils.JwtUtil; import com.iemr.mmu.utils.redis.RedisStorage; import io.jsonwebtoken.Claims; -import io.jsonwebtoken.io.IOException; import jakarta.servlet.FilterChain; import jakarta.servlet.ServletException; -import jakarta.servlet.http.Cookie; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse;Then update the method signature:
protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) - throws ServletException, IOException, java.io.IOException { + throws ServletException, java.io.IOException {
37-44: Prefer constructor injection over field injection.Field injection makes testing harder and hides dependencies. Constructor injection is recommended by Spring best practices.
- @Autowired - private JwtUtil jwtUtil; - - @Autowired - private RedisStorage redisService; - - @Autowired - private JwtAuthenticationUtil userService; + private final JwtUtil jwtUtil; + private final RedisStorage redisService; + private final JwtAuthenticationUtil userService; + + public RoleAuthenticationFilter(JwtUtil jwtUtil, RedisStorage redisService, JwtAuthenticationUtil userService) { + this.jwtUtil = jwtUtil; + this.redisService = redisService; + this.userService = userService; + }src/main/java/com/iemr/mmu/repo/login/UserLoginRepo.java (1)
18-20: LGTM - Native query for role retrieval is correctly implemented.The parameterized query using
@Paramis safe from SQL injection. Consider renaming togetRoleNamesByUserId(plural) since it returnsList<String>for better clarity.src/main/java/com/iemr/mmu/controller/location/LocationController.java (1)
28-28: LGTM! Broad role access is appropriate for location master data.Location endpoints serve as master data lookups needed across different workflows, so the wide role coverage is justified.
Optional: Consider extracting the common role expressions into constants or a custom meta-annotation to reduce duplication across controllers and improve maintainability.
Also applies to: 46-47
src/main/java/com/iemr/mmu/utils/exception/CustomAccessDeniedHandler.java (1)
14-27: Consider including the exception message for debugging purposes.The
CustomAuthenticationEntryPointincludesauthException.getMessage()in its response, but this handler uses a static "Access denied" message. While this is acceptable for security (avoiding information leakage), consider logging the exception for debugging:public void handle(HttpServletRequest request, HttpServletResponse response, AccessDeniedException accessDeniedException) throws IOException { + // Log the access denied attempt for debugging/audit + // logger.warn("Access denied: {}", accessDeniedException.getMessage()); response.setStatus(HttpServletResponse.SC_FORBIDDEN); // 403 response.setContentType("application/json");Otherwise, the implementation looks correct.
src/main/java/com/iemr/mmu/controller/generalOPD/GeneralOPDController.java (1)
68-68: Minor: Remove trailing spaces in @PreAuthorize annotations.The
@PreAuthorizeannotations in this file have trailing spaces (e.g.,"hasRole('NURSE') "instead of"hasRole('NURSE')"). While this doesn't affect functionality, it's inconsistent with other controllers in this PR.- @PreAuthorize("hasRole('NURSE') ") + @PreAuthorize("hasRole('NURSE')")This applies to lines 68, 101, 128, 160, 191, 223, 255, 282, 313, 344, 375, and 405.
src/main/java/com/iemr/mmu/controller/anc/ANCController.java (1)
96-96: Minor: Remove leading space in @PreAuthorize annotation.There's a leading space in the annotation value:
" hasRole('DOCTOR')"should be"hasRole('DOCTOR')".- @PreAuthorize(" hasRole('DOCTOR')") + @PreAuthorize("hasRole('DOCTOR')")src/main/java/com/iemr/mmu/controller/common/main/CommonController.java (2)
486-488: Fix formatting: Method declaration should be on a separate line.The
public Stringdeclaration appears to be on the same line as the@PreAuthorizeannotation, which is unusual formatting.@PostMapping(value = { "/getBenPastObstetricHistory" }) @PreAuthorize("hasRole('NURSE') || hasRole('DOCTOR')") - public String getBenPastObstetricHistory( + public String getBenPastObstetricHistory(
1011-1011: Consider consolidating role checks for maintainability.The
extendRedisSessionendpoint has a very long role expression with 9 role checks, including duplicate formats (TC_SPECIALISTandTCSPECIALIST,LAB_TECHNICIANimplicitly expected). Consider extracting this to a custom annotation or constant for maintainability.For example, define a constant or create a custom security expression:
// Option 1: Extract to a constant private static final String ALL_AUTHENTICATED_ROLES = "hasRole('NURSE') || hasRole('DOCTOR') || hasRole('PHARMACIST') || " + "hasRole('LAB_TECHNICIAN') || hasRole('LABTECHNICIAN') || hasRole('RADIOLOGIST') || " + "hasRole('ONCOLOGIST') || hasRole('TC_SPECIALIST') || hasRole('TCSPECIALIST') || hasRole('REGISTRAR')"; // Option 2: Use hasAnyRole (cleaner but needs role names without ROLE_ prefix handling) @PreAuthorize("hasAnyRole('NURSE', 'DOCTOR', 'PHARMACIST', 'LAB_TECHNICIAN', 'LABTECHNICIAN', 'RADIOLOGIST', 'ONCOLOGIST', 'TC_SPECIALIST', 'TCSPECIALIST', 'REGISTRAR')")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
pom.xml(1 hunks)src/main/java/com/iemr/mmu/controller/anc/ANCController.java(15 hunks)src/main/java/com/iemr/mmu/controller/cancerscreening/CancerScreeningController.java(17 hunks)src/main/java/com/iemr/mmu/controller/common/main/CommonController.java(39 hunks)src/main/java/com/iemr/mmu/controller/common/main/InsertCommonController.java(2 hunks)src/main/java/com/iemr/mmu/controller/common/master/CommonMasterController.java(5 hunks)src/main/java/com/iemr/mmu/controller/covid19/CovidController.java(10 hunks)src/main/java/com/iemr/mmu/controller/dataSyncActivity/StartSyncActivity.java(2 hunks)src/main/java/com/iemr/mmu/controller/dataSyncLayerCentral/MMUDataSyncVanToServer.java(2 hunks)src/main/java/com/iemr/mmu/controller/fileSync/FileSyncController.java(2 hunks)src/main/java/com/iemr/mmu/controller/generalOPD/GeneralOPDController.java(13 hunks)src/main/java/com/iemr/mmu/controller/labtechnician/LabTechnicianController.java(2 hunks)src/main/java/com/iemr/mmu/controller/location/LocationController.java(2 hunks)src/main/java/com/iemr/mmu/controller/login/IemrMmuLoginController.java(2 hunks)src/main/java/com/iemr/mmu/controller/ncdCare/NCDCareController.java(10 hunks)src/main/java/com/iemr/mmu/controller/ncdscreening/NCDController.java(15 hunks)src/main/java/com/iemr/mmu/controller/nurse/vitals/AnthropometryVitalsController.java(2 hunks)src/main/java/com/iemr/mmu/controller/pnc/PostnatalCareController.java(14 hunks)src/main/java/com/iemr/mmu/controller/quickconsult/QuickConsultController.java(7 hunks)src/main/java/com/iemr/mmu/controller/registrar/main/RegistrarController.java(17 hunks)src/main/java/com/iemr/mmu/controller/reports/ReportGateway.java(2 hunks)src/main/java/com/iemr/mmu/controller/snomedct/SnomedController.java(2 hunks)src/main/java/com/iemr/mmu/controller/teleconsultation/TeleConsultationController.java(2 hunks)src/main/java/com/iemr/mmu/repo/login/UserLoginRepo.java(2 hunks)src/main/java/com/iemr/mmu/utils/JwtAuthenticationUtil.java(3 hunks)src/main/java/com/iemr/mmu/utils/JwtUtil.java(3 hunks)src/main/java/com/iemr/mmu/utils/exception/CustomAccessDeniedHandler.java(1 hunks)src/main/java/com/iemr/mmu/utils/exception/CustomAuthenticationEntryPoint.java(1 hunks)src/main/java/com/iemr/mmu/utils/mapper/RoleAuthenticationFilter.java(1 hunks)src/main/java/com/iemr/mmu/utils/mapper/SecurityConfig.java(1 hunks)src/main/java/com/iemr/mmu/utils/redis/RedisStorage.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/main/java/com/iemr/mmu/utils/mapper/RoleAuthenticationFilter.java (6)
src/main/java/com/iemr/mmu/utils/JwtAuthenticationUtil.java (1)
Component(22-151)src/main/java/com/iemr/mmu/utils/JwtUtil.java (1)
Component(20-80)src/main/java/com/iemr/mmu/utils/exception/CustomAccessDeniedHandler.java (1)
Component(14-28)src/main/java/com/iemr/mmu/utils/exception/CustomAuthenticationEntryPoint.java (1)
Component(12-23)src/main/java/com/iemr/mmu/utils/redis/RedisStorage.java (1)
Component(36-137)src/main/java/com/iemr/mmu/utils/JwtUserIdValidationFilter.java (1)
JwtUserIdValidationFilter(21-177)
src/main/java/com/iemr/mmu/utils/exception/CustomAccessDeniedHandler.java (1)
src/main/java/com/iemr/mmu/utils/exception/CustomAuthenticationEntryPoint.java (1)
Component(12-23)
src/main/java/com/iemr/mmu/utils/JwtUtil.java (2)
src/main/java/com/iemr/mmu/utils/JwtAuthenticationUtil.java (1)
Component(22-151)src/main/java/com/iemr/mmu/utils/mapper/RoleAuthenticationFilter.java (1)
Component(33-110)
🪛 GitHub Check: SonarCloud Code Analysis
src/main/java/com/iemr/mmu/utils/mapper/RoleAuthenticationFilter.java
[warning] 50-50: Replace this use of System.out by a logger.
[warning] 43-43: Remove this field injection and use constructor injection instead.
[warning] 69-69: No need to call "toString()" method as formatting and string conversion is done by the Formatter.
[warning] 17-17: Remove this unused import 'com.iemr.mmu.service.common.transaction.CommonServiceImpl'.
[warning] 69-69: "userId" is already a string, there's no need to call "toString()" on it.
[warning] 40-40: Remove this field injection and use constructor injection instead.
[failure] 35-35: "logger" is the name of a field in "GenericFilterBean".
[warning] 29-29: Remove this unused import 'jakarta.servlet.http.Cookie'.
[warning] 70-70: Replace this use of System.out by a logger.
[warning] 84-84: Replace this use of System.out by a logger.
[warning] 76-76: Extract this nested try block into a separate method.
[warning] 21-21: Remove this unused import 'com.iemr.mmu.utils.JwtUserIdValidationFilter'.
[warning] 37-37: Remove this field injection and use constructor injection instead.
[warning] 70-70: "userId" is already a string, there's no need to call "toString()" on it.
[warning] 69-69: Invoke method(s) only conditionally.
[warning] 18-18: Remove this unused import 'com.iemr.mmu.service.login.IemrMmuLoginServiceImpl'.
[warning] 92-92: Replace this usage of 'Stream.collect(Collectors.toList())' with 'Stream.toList()' and ensure that the list is unmodified.
src/main/java/com/iemr/mmu/utils/mapper/SecurityConfig.java
[warning] 35-35: Replace this use of System.out by a logger.
src/main/java/com/iemr/mmu/utils/redis/RedisStorage.java
[warning] 133-133: Return an empty collection instead of null.
[warning] 112-112: Remove this field injection and use constructor injection instead.
[warning] 129-129: String contains no format specifiers.
src/main/java/com/iemr/mmu/utils/JwtAuthenticationUtil.java
[warning] 81-81: Remove this unused "roles" local variable.
[warning] 144-144: Replace generic exceptions with specific library exceptions or a custom exception.
[warning] 148-148: Replace generic exceptions with specific library exceptions or a custom exception.
[warning] 137-137: Replace generic exceptions with specific library exceptions or a custom exception.
src/main/java/com/iemr/mmu/utils/JwtUtil.java
[warning] 3-3: Remove this unused import 'java.util.List'.
[warning] 31-31: Remove this field injection and use constructor injection instead.
🔇 Additional comments (51)
src/main/java/com/iemr/mmu/controller/login/IemrMmuLoginController.java (1)
28-28: Class-level RBAC annotation looks consistent with stated rolesThe
@PreAuthorizeexpression covers all clinical and sync roles mentioned in the PR and cleanly enforces RBAC on all/userendpoints in this controller. No functional issues from this change as long as yourGrantedAuthorityvalues match thehasRole(...)expectations (i.e.,ROLE_*).Please double-check that the authorities created from your JWT/user store are of the form
ROLE_NURSE,ROLE_DOCTOR, etc., so thathasRole('...')evaluates as intended under Spring Security 6.Also applies to: 45-47
src/main/java/com/iemr/mmu/controller/nurse/vitals/AnthropometryVitalsController.java (1)
8-8: Anthropometry vitals restricted to NURSE/DOCTORClass-level
@PreAuthorize("hasRole('NURSE') || hasRole('DOCTOR')")is a reasonable gate for these nurse/doctor-facing vitals APIs and is consistent with the controller’s responsibility.Confirm that no additional roles (e.g., TC specialist or other doctors) need access to this endpoint set, to avoid unintentionally blocking legitimate flows.
Also applies to: 20-23
pom.xml (1)
67-70: Security starter addition is appropriate; ensure method security is enabledAdding
spring-boot-starter-securityis required for the new authentication/authorization setup and looks correct for Spring Boot 3.2.2.Verify that your security configuration enables method security (e.g.,
@EnableMethodSecurity(prePostEnabled = true)) so that the@PreAuthorizeannotations across controllers are actually enforced.src/main/java/com/iemr/mmu/controller/covid19/CovidController.java (1)
31-32: COVID endpoints now guarded by NURSE/DOCTOR roles; revisit doctor-only flows if neededThe added
@PreAuthorizeannotations correctly constrain:
- Nurse save →
hasRole('NURSE')- Doctor save →
hasRole('DOCTOR')- Shared read/update flows →
hasRole('NURSE') || hasRole('DOCTOR')This is a sensible first pass and aligns the controller with the RBAC model introduced in the PR.
Several methods that conceptually represent “doctor screen” or “doctor data” (e.g.,
updateCovid19DoctorData,getBenCaseRecordFromDoctorCovid19) are currently open to both NURSE and DOCTOR. Please confirm with product/BA whether those should in fact beDOCTOR-only; if yes, you can tighten the relevant@PreAuthorizeexpressions.Also applies to: 67-70, 104-106, 131-133, 164-166, 195-197, 224-226, 252-253, 285-286, 315-316
src/main/java/com/iemr/mmu/controller/reports/ReportGateway.java (1)
27-28: Report gateway RBAC is coherent with clinical-roles requirementThe class-level
@PreAuthorizecleanly limits reporting APIs to the listed clinical roles, which fits the sensitive nature of report data.Confirm whether registrar or data-sync roles intentionally do not need access to reports; if they should, consider adding the corresponding
hasRole(...)checks here to keep behavior consistent with other controllers.Also applies to: 43-46
src/main/java/com/iemr/mmu/controller/ncdCare/NCDCareController.java (10)
31-31: Import addition for PreAuthorize looks good.The import is correctly added to support method-level security annotations.
70-70: Role-based access control correctly applied to nurse endpoint.The
@PreAuthorize("hasRole('NURSE') ")annotation correctly restricts this data-saving endpoint to users with the NURSE role. Note: there's a trailing space inside the annotation string which is harmless but could be cleaned up.
104-104: Doctor-only access correctly applied.
131-131: Combined role access correctly applied for read operations.Allowing both NURSE and DOCTOR roles to access visit details is appropriate for this workflow.
164-164: LGTM - Appropriate role combination for history details.
195-195: LGTM - Vitals access for both roles.
228-228: LGTM - Case record access for both roles.
255-255: LGTM - History update access for both roles.
288-288: LGTM - Vital update access for both roles.
318-318: Verify: Should doctor data update allow NURSE role?The endpoint
/update/doctorDatais annotated withhasRole('NURSE') || hasRole('DOCTOR'), but the method nameupdateNCDCareDoctorDataand summary suggest this is for updating doctor-entered data. Verify if nurses should have permission to modify doctor case records.src/main/java/com/iemr/mmu/controller/pnc/PostnatalCareController.java (5)
28-28: PreAuthorize import correctly added.
67-67: LGTM - Nurse data save restricted to NURSE role.
101-101: LGTM - Doctor data save restricted to DOCTOR role.
127-127: LGTM - Read operations allow both roles.The pattern of allowing both NURSE and DOCTOR roles for read operations and nurse-entered data updates is consistently applied across endpoints (Lines 127, 161, 193, 224, 256, 288, 315, 346, 377, 408).
432-432: LGTM - Doctor data update restricted to DOCTOR role.This is correctly more restrictive than the NCDCareController's
/update/doctorDataendpoint, which allows both roles. This appears to be the intended behavior for PNC.src/main/java/com/iemr/mmu/controller/quickconsult/QuickConsultController.java (7)
28-28: PreAuthorize import correctly added.
74-74: LGTM - Nurse data save restricted to NURSE role.
107-107: LGTM - Doctor data save restricted to DOCTOR role.
145-145: LGTM - Read operations allow both roles.
176-176: LGTM - Vitals access for both roles.
209-209: LGTM - Case record access for both roles.
236-236: LGTM - Doctor data update restricted to DOCTOR role.Consistent restriction pattern for doctor data updates.
src/main/java/com/iemr/mmu/controller/ncdscreening/NCDController.java (15)
28-28: PreAuthorize import correctly added.
78-78: LGTM - Nurse data save restricted to NURSE role.
113-113: LGTM - Doctor data save restricted to DOCTOR role.
141-141: Verify: NURSE-only restriction on get/nurseData endpoint.This endpoint is restricted to NURSE role only, unlike similar read endpoints in other controllers which allow both NURSE and DOCTOR. If doctors need to view NCD screening details entered by nurses, this restriction would block them. Verify this is the intended access policy.
167-167: LGTM - Visit count accessible by both roles.
191-191: LGTM - Visit details accessible by both roles.
218-218: LGTM - History details accessible by both roles.
244-244: LGTM - Vital details accessible by both roles.
271-271: LGTM - IDRS details accessible by both roles.
303-303: LGTM - Doctor case record accessible by both roles.
331-331: LGTM - Nurse data update accessible by both roles.
359-359: LGTM - History update accessible by both roles.
384-384: LGTM - Vital update accessible by both roles.
408-408: LGTM - IDRS update accessible by both roles.
432-432: LGTM - Doctor data update restricted to DOCTOR role.src/main/java/com/iemr/mmu/controller/common/main/InsertCommonController.java (1)
46-47: LGTM - Role-based access control correctly applied.Restricting document file operations to NURSE and DOCTOR roles is appropriate for this controller's functionality.
src/main/java/com/iemr/mmu/controller/dataSyncLayerCentral/MMUDataSyncVanToServer.java (1)
51-52: LGTM - Data sync operations appropriately restricted.The DATASYNC role restriction is appropriate for these sensitive data synchronization endpoints. The dual role naming pattern (
DATASYNC||DATA_SYNC) maintains consistency with other controllers in this PR.src/main/java/com/iemr/mmu/controller/snomedct/SnomedController.java (1)
45-46: LGTM - SNOMED CT lookup appropriately restricted to clinical roles.NURSE and DOCTOR role restrictions are suitable for medical terminology lookup operations.
src/main/java/com/iemr/mmu/controller/teleconsultation/TeleConsultationController.java (1)
47-48: Class-level authorization correctly restricts all endpoints to TC specialists using established codebase pattern.The dual role check (
TCSPECIALIST || TC_SPECIALIST) matches the pattern already established inCommonController(lines 689, 715, 1011). Verify these role names match exactly what is stored in your authentication/authorization system's database, as role definitions are not managed within this repository.src/main/java/com/iemr/mmu/controller/dataSyncActivity/StartSyncActivity.java (1)
28-28: LGTM! Class-level security annotation correctly restricts data sync operations.The
@PreAuthorizeannotation appropriately gates all endpoints in this controller to users withDATASYNCorDATA_SYNCroles, which aligns with the PR objective for role-based access control.Also applies to: 51-52
src/main/java/com/iemr/mmu/controller/fileSync/FileSyncController.java (1)
27-27: LGTM! Consistent security policy with other data sync controllers.The
@PreAuthorizeannotation is consistent withStartSyncActivity, appropriately restricting file sync operations toDATASYNC/DATA_SYNCroles.Also applies to: 42-43
src/main/java/com/iemr/mmu/utils/mapper/SecurityConfig.java (1)
39-54: CSRF disabling is appropriate for this stateless JWT-based API; RoleAuthenticationFilter correctly validates JWT tokens and sets SecurityContext with user roles.The
RoleAuthenticationFilterproperly:
- Extracts JWT from both cookie and header sources
- Validates token presence and claims integrity
- Extracts userId with null/empty checks
- Converts userId to Long with error handling
- Fetches roles from Redis cache with database fallback
- Creates
UsernamePasswordAuthenticationTokenwith proper authorities- Sets
SecurityContextfor authenticated requests- Clears context on authentication errors
- Continues filter chain appropriately
Combined with the stateless session policy and role-based authorization configuration, this implementation correctly secures all authenticated endpoints.
src/main/java/com/iemr/mmu/controller/anc/ANCController.java (1)
459-461: Verify if NURSE should have access to update doctor data.The
updateANCDoctorDataendpoint allows bothNURSEandDOCTORroles. While this might be intentional (e.g., nurses assisting with data entry), typically doctor data updates would be restricted to doctors only. Please confirm this is the intended behavior.src/main/java/com/iemr/mmu/controller/registrar/main/RegistrarController.java (1)
175-175: Verify database role names and standardize @PreAuthorize annotations for lab technician role.The codebase contains systematic inconsistency with both
hasRole('LABTECHNICIAN')andhasRole('LAB_TECHNICIAN')appearing together across multiple endpoints. Similar patterns exist withDATASYNC/DATA_SYNCandTCSPECIALIST/TC_SPECIALIST.Roles are retrieved from the database via
userLoginRepo.getRoleNamebyUserId()and normalized inRoleAuthenticationFilterusing.toUpperCase().replace(" ", "_"). However, this normalization does not reconcile the two lab technician formats—they remain distinct role names in Spring Security context.Action required: Query the database to confirm which role name(s) are actually stored (
LABTECHNICIAN,LAB_TECHNICIAN, or both). Then standardize all@PreAuthorizeannotations to use the canonical format. If both formats must be supported for backward compatibility, document this explicitly and consider centralizing the role definitions as constants.
src/main/java/com/iemr/mmu/controller/cancerscreening/CancerScreeningController.java
Show resolved
Hide resolved
src/main/java/com/iemr/mmu/utils/exception/CustomAuthenticationEntryPoint.java
Show resolved
Hide resolved
* fix: wasa-IDOR Vulnerability * fix: coderabbit comments * fix: remove userid from payload
vanitha1822
force-pushed
the
nd/vs/wasa_1921
branch
from
bdcd9f5 to
2f558b0
Compare
@drtechie fixed the comments. |
|
📋 Description
JIRA ID:
AMM-1921
Implement Role-based access control for the API's with the roles: Nurse, Registrar, DataSync, Doctor, LabTechnician, Pharmacist, Oncologist, Radiologist and TCSpecialist
✅ Type of Change
Summary by CodeRabbit
Release Notes
New Features
Security
✏️ Tip: You can customize this high-level summary in your review settings.