Role Based Broken Access Control Implementation : WASA #104
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 WalkthroughThis PR implements Spring Security role-based access control across the application by adding security dependencies, creating authentication filters and configuration, annotating controller endpoints with Changes
Sequence DiagramsequenceDiagram
participant Client
participant RoleAuthFilter as RoleAuthenticationFilter
participant JwtUtil
participant JwtAuthUtil as JwtAuthenticationUtil
participant UserService
participant Redis
participant PreAuthorize
participant Controller
participant CustomHandler as CustomAccessDeniedHandler/<br/>CustomAuthenticationEntryPoint
Client->>RoleAuthFilter: Request with JWT
RoleAuthFilter->>JwtUtil: Extract token from cookie/header
alt Token valid
JwtUtil-->>RoleAuthFilter: userId extracted
RoleAuthFilter->>Redis: getUserRoleFromCache(userId)
alt Roles cached
Redis-->>RoleAuthFilter: Roles list
else Cache miss
RoleAuthFilter->>JwtAuthUtil: getUserRoles(userId)
JwtAuthUtil->>UserService: Fetch roles
UserService-->>JwtAuthUtil: Roles
JwtAuthUtil-->>RoleAuthFilter: Roles
RoleAuthFilter->>Redis: cacheUserRoles(userId, roles)
Redis-->>RoleAuthFilter: Cached
end
RoleAuthFilter->>RoleAuthFilter: Set SecurityContext with GrantedAuthorities
else Invalid/Missing token
RoleAuthFilter->>CustomHandler: AuthenticationException
CustomHandler-->>Client: 401 Unauthorized (JSON)
end
RoleAuthFilter->>PreAuthorize: Check `@PreAuthorize` role expression
alt Role matches
PreAuthorize->>Controller: Allow access
Controller-->>Client: Response 200
else Role denied
PreAuthorize->>CustomHandler: AccessDeniedException
CustomHandler-->>Client: 403 Forbidden (JSON)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/main/java/com/iemr/tm/utils/JwtUtil.java (1)
62-68: Critical: ExposingextractAllClaimsbypasses denylist validation.Making this method public creates a security vulnerability. Callers can now extract claims from denylisted tokens (e.g., tokens from logged-out users), bypassing the denylist check that exists only in
validateToken(lines 42-45). This undermines the authentication security being implemented in this PR.Recommended solution: Keep the method private and ensure all claim extraction goes through
validateToken. If external access is required, refactor to eliminate duplication:- public Claims extractAllClaims(String token) { - return Jwts.parser() - .verifyWith(getSigningKey()) - .build() - .parseSignedClaims(token) - .getPayload(); - } + private Claims extractAllClaims(String token) { + // Delegate to validateToken to ensure denylist check is performed + return validateToken(token); + }This change ensures all token parsing includes denylist validation while removing code duplication.
src/main/java/com/iemr/tm/controller/login/IemrMmuLoginController.java (2)
73-82: Fix null check ordering to preventNullPointerException.The
userIdnull check (line 79) occurs afterInteger.parseInt(userId)(line 75). IfuserIdis null, theparseIntcall will throw aNullPointerExceptionbefore the null check is evaluated.try { - String jwtToken = CookieUtil.getJwtTokenFromCookie(request); + if (jwtToken == null) { + response.setError(403, "Unauthorized access: Missing or invalid token"); + return response.toString(); + } String userId = jwtUtil.getUserIdFromToken(jwtToken); - Integer userID=Integer.parseInt(userId); - - JSONObject obj = new JSONObject(comingRequest); - logger.info("getUserServicePointVanDetails request " + comingRequest); - if (userId == null || jwtToken ==null) { + if (userId == null) { response.setError(403, "Unauthorized access: Missing or invalid token"); return response.toString(); } + Integer userID = Integer.parseInt(userId); + + JSONObject obj = new JSONObject(comingRequest); + logger.info("getUserServicePointVanDetails request " + comingRequest); String responseData = iemrMmuLoginServiceImpl.getUserServicePointVanDetails(userID); response.setResponse(responseData);
120-135: Same null check ordering issue as above.The pattern is repeated here:
Integer.parseInt(userId)(line 122) is called before checking ifuserIdorjwtTokenis null (lines 127, 131).public String getUserVanSpDetails(@RequestBody String comingRequest, HttpServletRequest request) { OutputResponse response = new OutputResponse(); try { - String jwtToken = CookieUtil.getJwtTokenFromCookie(request); - String userId = jwtUtil.getUserIdFromToken(jwtToken); - Integer userID=Integer.parseInt(userId); - + String jwtToken = CookieUtil.getJwtTokenFromCookie(request); + if (jwtToken == null) { + response.setError(403, "Unauthorized access: Missing or invalid token"); + return response.toString(); + } + String userId = jwtUtil.getUserIdFromToken(jwtToken); + if (userId == null) { + response.setError(403, "Unauthorized access: Missing or invalid token"); + return response.toString(); + } + Integer userID = Integer.parseInt(userId); + JSONObject obj = new JSONObject(comingRequest); logger.info("getServicepointVillages request " + comingRequest); - if (userId !=null && obj.has("providerServiceMapID")) { + if (obj.has("providerServiceMapID")) { String responseData = iemrMmuLoginServiceImpl.getUserVanSpDetails(userID, obj.getInt("providerServiceMapID")); response.setResponse(responseData); - } else if(userId == null || jwtToken ==null) { - response.setError(403, "Unauthorized access : Missing or invalid token"); } else { response.setError(5000, "Invalid request"); }src/main/java/com/iemr/tm/controller/common/main/WorklistController.java (1)
711-800: TC Specialist worklist endpoints are missing@PreAuthorizeannotations.The endpoints
getTCSpecialistWorkListNew,getTCSpecialistWorkListNewPatientApp, andgetTCSpecialistWorklistFutureScheduled(lines 711-800) lack security annotations while all other role-specific worklist endpoints have them. These should be restricted to TC_SPECIALIST role.@Operation(summary = "Get teleconsultation specialist worklist") @GetMapping(value = { "/getTCSpecialistWorklist/{providerServiceMapID}/{serviceID}" }) +@PreAuthorize("hasRole('TC_SPECIALIST') || hasRole('TCSPECIALIST')") public String getTCSpecialistWorkListNew(...) {Apply similar annotations to the other two TC specialist endpoints.
♻️ Duplicate comments (3)
src/main/java/com/iemr/tm/controller/dataSyncActivity/StartSyncActivity.java (1)
50-50: Duplicate role naming issue with DATASYNC vs DATA_SYNC.This controller has the same role naming inconsistency as MMUDataSyncVanToServer.java. See the comment on that file for details and verification steps.
src/main/java/com/iemr/tm/controller/teleconsultation/TeleConsultationController.java (1)
48-48: Duplicate role naming issue with TCSPECIALIST vs TC_SPECIALIST.This controller has the same role naming inconsistency noted in PatientAppCommonMasterController.java (lines 135, 157, 179, 201, 223, 245). The duplication indicates unclear canonical role names.
src/main/java/com/iemr/tm/controller/labtechnician/LabtechnicianController.java (1)
27-28: Restricting lab endpoints to lab technician roles makes sense; verify role namingThe class-level
@PreAuthorize("hasRole('LAB_TECHNICIAN') || hasRole('LABTECHNICIAN') ")correctly gates all lab endpoints to lab-tech roles. Just ensure that:
- Your
GrantedAuthorityvalues align with these role strings and Spring’shasRoleconvention, and- You really need both
LAB_TECHNICIANandLABTECHNICIANvariants long-term (or can normalize to a single canonical role).Also applies to: 48-51
🧹 Nitpick comments (13)
src/main/java/com/iemr/tm/controller/patientApp/master/PatientAppCommonMasterController.java (1)
65-65: Remove trailing spaces in authorization expression.The annotation string contains trailing spaces:
"hasRole('NURSE') || hasRole('DOCTOR') ". While this won't affect functionality, it's inconsistent with the style used elsewhere.Apply this diff:
- @PreAuthorize("hasRole('NURSE') || hasRole('DOCTOR') ") + @PreAuthorize("hasRole('NURSE') || hasRole('DOCTOR')")src/main/java/com/iemr/tm/controller/nurse/vitals/AnthropometryVitalsController.java (1)
22-22: Remove trailing whitespace in@PreAuthorizeexpression.The annotation contains a trailing space inside the string:
"hasRole('NURSE') ". While this typically doesn't affect functionality, it's inconsistent with other controllers and should be cleaned up.-@PreAuthorize("hasRole('NURSE') ") +@PreAuthorize("hasRole('NURSE')")src/main/java/com/iemr/tm/controller/registrar/main/RegistrarController.java (1)
177-180: Consider de-duplicating repeated complex role expressionsSeveral methods repeat the same long role set (e.g.
hasRole('NURSE') || hasRole('DOCTOR') || hasRole('LABTECHNICIAN') || hasRole('LAB_TECHNICIAN') || hasRole('PHARMACIST') ...). Over time this is easy to get out of sync.If this pattern stabilizes, consider introducing:
- A custom composed annotation (e.g.
@BenReadAccess) backed by a single@PreAuthorize, or- At least shared constants for the SpEL strings.
This would make future role changes less error-prone.
Also applies to: 212-215, 286-290, 318-322
src/main/java/com/iemr/tm/repo/login/UserLoginRepo.java (1)
3-4: Use DISTINCT and align role names with security layer expectationsThe new
getRoleNamebyUserIdis straightforward, but two small points:
- Consider
SELECT DISTINCT rolename ...to avoid duplicate role names if a user has multiple service-role mappings.- Ensure the returned
rolenamevalues match what your security layer expects (e.g. eitherNURSE/DOCTORthat you later convert toROLE_*, or already-prefixed names compatible withhasRole).Also applies to: 18-20
src/main/java/com/iemr/tm/utils/JwtAuthenticationUtil.java (1)
3-3:getUserRoleslogic looks fine; fix typos and naming for clarityThe
getUserRolesmethod correctly validatesuserId, delegates toUserLoginRepo, and fails fast when no roles are found. Two minor cleanups:
- Rename
roletoroles(or similar) to reflect it’s aList<String>.- Fix typos in the exception message:
"Failed to retrieverole for usedId : ..."→"Failed to retrieve roles for userId : ...".These don’t affect behavior but will make logs and errors easier to interpret.
Also applies to: 134-147
src/main/java/com/iemr/tm/utils/redis/RedisStorage.java (1)
113-120: Return an empty collection instead of null.Returning
nullon failure forces callers to handle null checks and risksNullPointerException. Per static analysis, return an empty list instead.public List<String> getUserRoleFromCache(Long userId) { try { - return redisTemplate.opsForList().range("roles:" + userId, 0, -1); + 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(); } }Add import:
import java.util.Collections;src/main/java/com/iemr/tm/utils/mapper/RoleAuthenticationFilter.java (1)
17-17: Remove unused imports.Per static analysis,
CommonMasterServiceImplandCookieare not used.-import com.iemr.tm.service.common.master.CommonMasterServiceImpl; import com.iemr.tm.utils.CookieUtil; ... -import jakarta.servlet.http.Cookie; import jakarta.servlet.http.HttpServletRequest;Also applies to: 27-27
src/main/java/com/iemr/tm/controller/common/main/WorklistController.java (1)
110-110: Inconsistent role naming patterns suggest unclear role normalization.The code uses both
TCSPECIALISTandTC_SPECIALIST, andLABTECHNICIANandLAB_TECHNICIAN. This duplication indicates uncertainty about how roles are stored and transformed.The
RoleAuthenticationFiltertransforms roles via.toUpperCase().replace(" ", "_"), so a role stored as "TC Specialist" becomes "ROLE_TC_SPECIALIST".Standardize role names across the codebase. If roles are consistently transformed to use underscores, you can simplify:
-@PreAuthorize("hasRole('TC_SPECIALIST') || hasRole('TCSPECIALIST') ") +@PreAuthorize("hasRole('TC_SPECIALIST')")Verify the actual role values in the database to determine the canonical form.
Also applies to: 157-157, 178-178, 225-225
src/main/java/com/iemr/tm/utils/mapper/SecurityConfig.java (1)
35-39: Remove dead code: CSRF token repository is configured but CSRF is disabled.Lines 35-37 configure a
CookieCsrfTokenRepositorybut line 39 disables CSRF entirely, making this configuration unused.@Bean public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception { - CookieCsrfTokenRepository csrfTokenRepository = new CookieCsrfTokenRepository(); - csrfTokenRepository.setCookieHttpOnly(true); - csrfTokenRepository.setCookiePath("/"); http .csrf(csrf -> csrf.disable())src/main/java/com/iemr/tm/controller/ncdCare/NCDCareController.java (1)
266-266: Confirm that allowing both NURSE and DOCTOR on update endpoints matches the intended access model
updateHistoryNurseandupdateVitalNursenow allow bothNURSEandDOCTOR, even though the JavaDoc/objective text focuses on nurse‑entered data. If domain rules require that only doctors can overwrite nurse data (or vice‑versa), you may want to tighten these to a single role or split endpoints accordingly.If the intent is that both can perform updates, this is fine as‑is; just confirm it matches your functional requirements and threat model.
Also applies to: 304-304
src/main/java/com/iemr/tm/controller/covid19/CovidController.java (1)
258-258: Reconfirm mixed NURSE/DOCTOR write access on COVID update endpoints
updateHistoryNurseandupdateVitalNurseallow bothNURSEandDOCTORwhile dealing with nurse‑captured data. If your policy is that only one of these roles should be able to perform these modifications, consider tightening the expression; otherwise, documenting this shared update responsibility somewhere in your security design would help future maintainers.Also applies to: 296-296
src/main/java/com/iemr/tm/controller/generalOPD/GeneralOPDController.java (1)
298-298: Validate that allowing both NURSE and DOCTOR on General OPD update flows is intentional
updateVisitNurse,updateHistoryNurse,updateVitalNurse, andupdateGeneralOPDExaminationNursenow accept bothNURSEandDOCTOR. Given that the JavaDocs emphasize nurse‑entered data being potentially replaced by doctor details, double‑check that nurses are indeed supposed to retain write access once doctors are involved. If not, you may want to move some of these to DOCTOR‑only.Also applies to: 333-333, 368-368, 403-403
src/main/java/com/iemr/tm/controller/ncdscreening/NCDScreeningController.java (1)
335-335: Double‑check that mixed NURSE/DOCTOR write access on NCD screening updates matches requirements
updateHistoryNurse,updateVitalNurse, andupdateIDRSScreenall now allow bothNURSEandDOCTOR. Given that some comments describe overwriting nurse data with doctor details, verify whether nurses should still be able to change these records post‑doctor‑review, or whether some of these endpoints should be DOCTOR‑only.If the shared access is intended (e.g., for corrections), consider documenting that explicitly in your security/requirements docs.
Also applies to: 369-369, 398-398, 427-427
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
pom.xml(2 hunks)src/main/java/com/iemr/tm/controller/anc/AntenatalCareController.java(15 hunks)src/main/java/com/iemr/tm/controller/cancerscreening/CancerScreeningController.java(17 hunks)src/main/java/com/iemr/tm/controller/common/main/WorklistController.java(38 hunks)src/main/java/com/iemr/tm/controller/common/master/CommonMasterController.java(2 hunks)src/main/java/com/iemr/tm/controller/covid19/CovidController.java(10 hunks)src/main/java/com/iemr/tm/controller/dataSyncActivity/StartSyncActivity.java(2 hunks)src/main/java/com/iemr/tm/controller/dataSyncLayerCentral/MMUDataSyncVanToServer.java(2 hunks)src/main/java/com/iemr/tm/controller/foetalmonitor/FoetalMonitorController.java(2 hunks)src/main/java/com/iemr/tm/controller/generalOPD/GeneralOPDController.java(13 hunks)src/main/java/com/iemr/tm/controller/labtechnician/LabtechnicianController.java(2 hunks)src/main/java/com/iemr/tm/controller/location/LocationController.java(2 hunks)src/main/java/com/iemr/tm/controller/login/IemrMmuLoginController.java(2 hunks)src/main/java/com/iemr/tm/controller/ncdCare/NCDCareController.java(10 hunks)src/main/java/com/iemr/tm/controller/ncdscreening/NCDScreeningController.java(15 hunks)src/main/java/com/iemr/tm/controller/nurse/vitals/AnthropometryVitalsController.java(2 hunks)src/main/java/com/iemr/tm/controller/patientApp/master/PatientAppCommonMasterController.java(11 hunks)src/main/java/com/iemr/tm/controller/pnc/PostnatalCareController.java(14 hunks)src/main/java/com/iemr/tm/controller/quickconsult/QuickConsultController.java(7 hunks)src/main/java/com/iemr/tm/controller/registrar/main/RegistrarController.java(17 hunks)src/main/java/com/iemr/tm/controller/report/CRMReportController.java(2 hunks)src/main/java/com/iemr/tm/controller/snomedct/SnomedController.java(2 hunks)src/main/java/com/iemr/tm/controller/teleconsultation/TeleConsultationController.java(2 hunks)src/main/java/com/iemr/tm/repo/login/UserLoginRepo.java(2 hunks)src/main/java/com/iemr/tm/utils/JwtAuthenticationUtil.java(2 hunks)src/main/java/com/iemr/tm/utils/JwtUtil.java(1 hunks)src/main/java/com/iemr/tm/utils/exception/CustomAccessDeniedHandler.java(1 hunks)src/main/java/com/iemr/tm/utils/exception/CustomAuthenticationEntryPoint.java(1 hunks)src/main/java/com/iemr/tm/utils/mapper/RoleAuthenticationFilter.java(1 hunks)src/main/java/com/iemr/tm/utils/mapper/SecurityConfig.java(1 hunks)src/main/java/com/iemr/tm/utils/redis/RedisStorage.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/main/java/com/iemr/tm/controller/dataSyncActivity/StartSyncActivity.java (3)
src/main/java/com/iemr/tm/controller/common/master/CommonMasterController.java (1)
RestController(42-109)src/main/java/com/iemr/tm/controller/labtechnician/LabtechnicianController.java (1)
RestController(48-154)src/main/java/com/iemr/tm/controller/snomedct/SnomedController.java (1)
RequestMapping(44-107)
src/main/java/com/iemr/tm/utils/mapper/RoleAuthenticationFilter.java (3)
src/main/java/com/iemr/tm/utils/JwtAuthenticationUtil.java (1)
Component(22-148)src/main/java/com/iemr/tm/utils/JwtUtil.java (1)
Component(14-77)src/main/java/com/iemr/tm/utils/redis/RedisStorage.java (1)
Component(37-121)
src/main/java/com/iemr/tm/utils/exception/CustomAuthenticationEntryPoint.java (3)
src/main/java/com/iemr/tm/utils/JwtAuthenticationUtil.java (1)
Component(22-148)src/main/java/com/iemr/tm/utils/JwtUtil.java (1)
Component(14-77)src/main/java/com/iemr/tm/utils/exception/CustomAccessDeniedHandler.java (1)
Component(14-28)
🪛 GitHub Check: SonarCloud Code Analysis
src/main/java/com/iemr/tm/utils/redis/RedisStorage.java
[warning] 99-99: Remove this field injection and use constructor injection instead.
[warning] 118-118: Return an empty collection instead of null.
src/main/java/com/iemr/tm/utils/mapper/RoleAuthenticationFilter.java
[failure] 33-33: "logger" is the name of a field in "GenericFilterBean".
[warning] 35-35: Remove this field injection and use constructor injection instead.
[warning] 69-69: Extract this nested try block into a separate method.
[warning] 38-38: Remove this field injection and use constructor injection instead.
[warning] 27-27: Remove this unused import 'jakarta.servlet.http.Cookie'.
[warning] 17-17: Remove this unused import 'com.iemr.tm.service.common.master.CommonMasterServiceImpl'.
[warning] 84-84: Replace this usage of 'Stream.collect(Collectors.toList())' with 'Stream.toList()' and ensure that the list is unmodified.
[warning] 41-41: Remove this field injection and use constructor injection instead.
🔇 Additional comments (15)
src/main/java/com/iemr/tm/controller/anc/AntenatalCareController.java (1)
70-70: LGTM - Appropriate role restrictions for ANC workflow.The access control logic correctly restricts:
- Nurse data saving to NURSE role only (line 70)
- Doctor data saving to DOCTOR role only (lines 103, 445)
- Read and update operations to both NURSE and DOCTOR roles
This properly enforces the ANC workflow separation of concerns.
Minor style note: Several annotations have trailing spaces in the string (lines 138, 165, 192, 218, 245, 271, 300, 329, 358, 387, 416), e.g.,
"hasRole('NURSE') || hasRole('DOCTOR') ". Consider removing for consistency.src/main/java/com/iemr/tm/controller/snomedct/SnomedController.java (1)
46-46: LGTM - Appropriate access control for clinical terminology.Restricting SNOMED CT clinical terminology access to NURSE and DOCTOR roles is appropriate for this medical records system.
Minor style note: The annotation has a trailing space:
"hasRole('NURSE') || hasRole('DOCTOR') ". Consider removing for consistency.src/main/java/com/iemr/tm/controller/cancerscreening/CancerScreeningController.java (1)
74-74: LGTM - Well-designed access control for cancer screening workflow.The role-based access control is appropriately structured:
- Nurse data entry restricted to NURSE role (line 74)
- Doctor updates restricted to DOCTOR role (lines 111, 579)
- Shared read/update operations allow NURSE and DOCTOR
- Cancer diagnosis updates appropriately include ONCOLOGIST role (line 544)
This properly enforces the cancer screening clinical workflow with appropriate specialist involvement.
src/main/java/com/iemr/tm/utils/exception/CustomAccessDeniedHandler.java (1)
14-27: LGTM!Clean implementation that follows the same pattern as
CustomAuthenticationEntryPoint. The staticObjectMapperis thread-safe, and the generic error message avoids leaking sensitive exception details.src/main/java/com/iemr/tm/controller/report/CRMReportController.java (1)
49-50: This role naming pattern is systematic across the codebase, not isolated to CRMReportController.Both role variants (
LABTECHNICIAN/LAB_TECHNICIANandTCSPECIALIST/TC_SPECIALIST) appear consistently together in@PreAuthorizeannotations across 8+ controller files (LabtechnicianController, TeleConsultationController, CRMReportController, LocationController, IemrMmuLoginController, WorklistController, and others). This suggests intentional dual support—likely for backward compatibility—rather than an inconsistency requiring immediate standardization in this specific file.If standardization is desired, it must be addressed codebase-wide. Alternatively, if this is intentional legacy support, document it centrally and verify both role variants are actually recognized by the authentication layer.
src/main/java/com/iemr/tm/controller/pnc/PostnatalCareController.java (1)
28-29: Confirm role naming vshasRoleexpectations and document access rulesThe added
@PreAuthorizeannotations on PNC endpoints look consistent with the intended nurse/doctor responsibilities, but correctness depends on how authorities are populated:
hasRole('NURSE')/hasRole('DOCTOR')expectGrantedAuthorityvalues likeROLE_NURSE,ROLE_DOCTORby default.- If your JWT/
RoleAuthenticationFiltercurrently uses bare role names (e.g.NURSE), either:
- Adjust authority creation to prefix with
ROLE_, or- Switch these to
hasAuthority('NURSE')/hasAuthority('DOCTOR').It would also help to explicitly document (in SecurityConfig or constants) which roles are allowed per screen so future additions (e.g. new clinical roles) stay consistent.
Also applies to: 68-71, 106-109, 139-142, 172-176, 205-208, 236-239, 268-271, 299-303, 327-330, 363-366, 399-402, 435-438, 464-467
src/main/java/com/iemr/tm/controller/quickconsult/QuickConsultController.java (1)
28-29: Validate that quick consult authorities line up withhasRoleusageThe new
@PreAuthorizeguards for nurse-only, doctor-only, and shared quick-consult endpoints are reasonable. As with other controllers, please verify:
- Authorities derived from JWT/DB are named to match
hasRole('NURSE')/hasRole('DOCTOR')(i.e. underlying authorities should typically beROLE_NURSE/ROLE_DOCTOR), or- If authorities are stored as plain
NURSE/DOCTOR, update these expressions tohasAuthority(...)instead.Once verified, this controller’s access control looks good.
Also applies to: 77-80, 114-117, 153-156, 184-187, 216-220, 244-247
src/main/java/com/iemr/tm/controller/registrar/main/RegistrarController.java (1)
32-33: Ensure registrar role strings align with actual authorities, especially LAB variants*You’ve added comprehensive
@PreAuthorizechecks with combinations ofNURSE,DOCTOR,REGISTRAR,LABTECHNICIAN/LAB_TECHNICIAN, andPHARMACIST. Functionally this looks right for registrar/search/beneficiary and lab/pharmacy-facing endpoints.To avoid silent denials:
- Confirm which exact role names are stored for users (e.g.
LABTECHNICIANvsLAB_TECHNICIAN), and that yourGrantedAuthorityvalues conform to Spring’shasRoleconvention (ROLE_…), or else switch tohasAuthority.- Double-check that endpoints intended for both lab & pharmacy access (e.g. beneficiary image/details) include all required roles and no unintended ones.
Also applies to: 84-88, 104-108, 124-127, 147-150, 177-180, 212-215, 238-242, 264-266, 286-290, 318-322, 336-339, 399-403, 418-421, 481-485, 504-507, 528-531
src/main/java/com/iemr/tm/controller/common/master/CommonMasterController.java (1)
29-30: Confirm that restricting/masterAPIs to NURSE/DOCTOR won’t break other role flowsAdding class-level
@PreAuthorize("hasRole('NURSE') || hasRole('DOCTOR') ")is consistent with the current method names (nurse/doctor master data), but it will deny access for any existing Registrar, LabTechnician, etc. clients calling these endpoints.Please:
- Verify no non-nurse/doctor flows depend on
/master/**.- As elsewhere, ensure your authorities are aligned with
hasRolesemantics, or switch tohasAuthorityif you don’t use theROLE_prefix convention.Also applies to: 42-46
src/main/java/com/iemr/tm/utils/exception/CustomAuthenticationEntryPoint.java (1)
15-22: I'll help you verify this review comment. Let me start by examining the relevant code files to confirm the issues and assess the recommendations.
<function_calls>
#!/bin/bashFind and read the CustomAuthenticationEntryPoint file
find . -name "CustomAuthenticationEntryPoint.java" -type f
</function_calls>
<function_calls>
#!/bin/bashAlso search for CustomAccessDeniedHandler to verify if it's a comparable pattern
find . -name "CustomAccessDeniedHandler.java" -type f
</function_calls>
<function_calls>
#!/bin/bashSearch for exception handling utilities
find . -name "Exception.java" -type f | head -20
</function_calls>Let me execute these searches:
<function_calls>
#!/bin/bashLocate the file mentioned in the review
cat src/main/java/com/iemr/tm/utils/exception/CustomAuthenticationEntryPoint.java 2>/dev/null || echo "File not found at expected path"
</function_calls>src/main/java/com/iemr/tm/utils/mapper/SecurityConfig.java (1)
41-43: I'll help you verify the security configuration concerns. Let me search the codebase for actuator, Swagger, and related configurations.
<function_calls>
#!/bin/bashSearch for actuator and swagger references in the codebase
echo "=== Searching for actuator/swagger references ==="
rg -n "actuator|swagger|springdoc" --type=java --type=properties --type=yaml -A 2 -B 2
</function_calls>
#!/bin/bashRead the full SecurityConfig.java file
echo "=== Reading SecurityConfig.java ==="
cat -n src/main/java/com/iemr/tm/utils/mapper/SecurityConfig.java
</function_calls>
#!/bin/bashSearch for application configuration files
echo "=== Looking for application config files ==="
find . -type f ( -name "application.properties" -o -name "application.yml" -o -name "application-.properties" -o -name "application-.yml" ) 2>/dev/null | head -20
</function_calls>
#!/bin/bashCheck for any web endpoints or controllers that might be public
echo "=== Searching for RestController or RequestMapping ==="
rg -n "RestController|RequestMapping|GetMapping|PostMapping" --type=java -A 3 | head -50
</function_calls>src/main/java/com/iemr/tm/controller/ncdCare/NCDCareController.java (1)
31-31: RBAC mapping across NCD Care endpoints looks coherent; please confirm authority naming and method-security config
saveBenNCDCareNurseDatarestricted tohasRole('NURSE')andsaveBenNCDCareDoctorData/updateNCDCareDoctorDatatohasRole('DOCTOR')align well with their responsibilities.- Read-style endpoints (
getBenVisitDetailsFrmNurseNCDCare, history, vitals, doctor case record) and mixed nurse/doctor update endpoints are guarded withhasRole('NURSE') || hasRole('DOCTOR'), which matches the naming/comments.Two things to double‑check:
- In Spring Security 6/Boot 3,
hasRole('X')expects aGrantedAuthorityofROLE_Xby default. Ensure your authentication setup issuesROLE_NURSE/ROLE_DOCTOR(or that you’ve customized the role prefix), otherwise these checks will silently fail.- Verify that global method security is enabled (e.g.
@EnableMethodSecurity(prePostEnabled = true)or equivalent) so these@PreAuthorizeannotations are actually enforced.Also applies to: 73-73, 110-110, 143-143, 175-175, 206-206, 239-239, 266-266, 304-304, 339-339
src/main/java/com/iemr/tm/controller/covid19/CovidController.java (1)
31-31: Consistent nurse/doctor RBAC for COVID endpoints; verify GrantedAuthority values and method security
saveBenNCDCareNurseDataguarded byhasRole('NURSE')andsaveBenCovidDoctorData/updateCovid19DoctorDatabyhasRole('DOCTOR')is consistent with the intent.- Read and detail endpoints (visit, history, vitals, case‑record) and most update flows are
hasRole('NURSE') || hasRole('DOCTOR'), which matches their names and usage.Same Spring Security points to validate:
- That your
AuthenticationcontainsROLE_NURSE/ROLE_DOCTOR(or that you’ve customized the role prefix) sohasRole('...')behaves as expected.- That method security is enabled at configuration level so
@PreAuthorizeis applied.Also applies to: 65-65, 102-102, 135-135, 167-167, 198-198, 231-231, 258-258, 296-296, 331-331
src/main/java/com/iemr/tm/controller/generalOPD/GeneralOPDController.java (1)
28-28: General OPD RBAC is aligned with endpoint responsibilities; ensure authorities and method security are wired
saveBenGenOPDNurseDatarestricted tohasRole('NURSE')andsaveBenGenOPDDoctorData/updateGeneralOPDDoctorDatatohasRole('DOCTOR')is appropriate.- All read‑style and shared nurse/doctor flows (visit, history, vitals, examination, case record) use
hasRole('NURSE') || hasRole('DOCTOR'), which matches the controller semantics.As with other controllers, please confirm:
- Your authorities use the
ROLE_prefix (e.g.,ROLE_NURSE,ROLE_DOCTOR) or that you’ve customized Spring Security’s role prefix sohasRole('...')checks succeed.- Global method security is enabled so
@PreAuthorizeconstraints on this controller are enforced.Also applies to: 74-74, 111-111, 143-143, 176-176, 207-207, 239-239, 271-271, 298-298, 333-333, 368-368, 403-403, 437-437
src/main/java/com/iemr/tm/controller/ncdscreening/NCDScreeningController.java (1)
28-28: NCD Screening RBAC rules are consistent and granular; please verify authority values and method-security setup
saveBeneficiaryNCDScreeningDetailsandupdateBeneficiaryNCDScreeningDetailsare NURSE‑only, which fits their nurse‑focused responsibilities.- Doctor‑specific flows (
saveBenNCDScreeningDoctorData,updateDoctorData) are DOCTOR‑only, which is appropriate.- Data retrieval endpoints (visit, history, vitals, IDRS, case‑records, visit count) are accessible to both
NURSEandDOCTOR, matching their usage in shared workflows.- Shared update flows for history, vitals, and IDRS also permit both roles.
As with the other controllers:
- Confirm that your
GrantedAuthorityvalues follow theROLE_NURSE/ROLE_DOCTORconvention (or that you’ve overridden the default role prefix) so thathasRole('NURSE')/hasRole('DOCTOR')function correctly under Spring Security 6 / Boot 3.2.2.- Ensure method‑level security is enabled (e.g., via
@EnableMethodSecurity) so these@PreAuthorizeannotations are active in production.Also applies to: 79-79, 111-111, 143-143, 169-169, 198-198, 226-226, 253-253, 279-279, 306-306, 335-335, 369-369, 398-398, 427-427, 456-456
src/main/java/com/iemr/tm/controller/dataSyncLayerCentral/MMUDataSyncVanToServer.java
Show resolved
Hide resolved
src/main/java/com/iemr/tm/controller/foetalmonitor/FoetalMonitorController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iemr/tm/controller/location/LocationController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iemr/tm/controller/patientApp/master/PatientAppCommonMasterController.java
Show resolved
Hide resolved
src/main/java/com/iemr/tm/utils/mapper/RoleAuthenticationFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iemr/tm/utils/mapper/RoleAuthenticationFilter.java
Outdated
Show resolved
Hide resolved
drtechie
left a comment
drtechie
left a comment
There was a problem hiding this comment.
Approving, please check the comments if relevant
|
CSRF disabled because app uses token-based REST APIs. |
|
📋 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
Chores
✏️ Tip: You can customize this high-level summary in your review settings.