Sync release-3.8.3 with vb/trace: role grant - #155
Conversation
Allow users with the new Volunteer role (Stop TB serviceline) to authenticate via /user login endpoints and to register new beneficiaries via /registrarBeneficaryRegistrationNew. Mirrors existing ASHA access. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Confirmed via role trace that userId 4408 (and likely other Registration Officer accounts) was hitting 403 on registrarBeneficaryRegistrationNew because REGISTRATION_OFFICER wasn't in the allowed-roles whitelist. Mirrors the existing ASHA/VOLUNTEER access. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…)" This reverts commit 8894a0e.
📝 WalkthroughWalkthroughChangesThe pull request adds stateless Spring Security authentication with role-based controller authorization, authenticated-principal validation, CORS enforcement, and custom 401/403 responses. It also adds health and version endpoints, ECG abnormal-finding support, doctor-signature flow tracking, registration validation, and medication-frequency handling updates. Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
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/service/common/transaction/CommonServiceImpl.java (1)
557-601: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftPotential IDOR on OpenKM file download.
getOpenKMDocURLresolves a caller-suppliedfileIDstraight to afileUUIDwith no beneficiary/visit ownership check, andgetKMFilehas no method-level role restriction. Any authenticated user can varyfileIDand fetch another beneficiary’s OpenKM URL. Add a record-level access check before resolving the URL, and put a@PreAuthorizeguard ongetKMFileas defense in depth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/common/transaction/CommonServiceImpl.java` around lines 557 - 601, The getOpenKMDocURL flow must enforce record-level beneficiary/visit ownership before benVisitDetailRepo.getFileUUID resolves the caller-supplied fileID; reuse the project’s existing authorization check and return or reject unauthorized requests. In WorklistController.java lines 806-824, add a method-level `@PreAuthorize` guard to getKMFile using the appropriate role expression, preserving existing behavior for authorized callers.src/main/java/com/iemr/tm/service/labtechnician/LabTechnicianServiceImpl.java (1)
417-466: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winNullPointerException vulnerability when processing partially-populated payloads.
The overarching conditional ensures execution if either
labResultsListORradiologyTestResultshas items. However, if the payload contains one but omits the other, the omitted list will benull. The code attempts to iterate over both lists unconditionally, which will trigger aNullPointerException(e.g., whenlabResultsListisnullbutradiologyTestResultshas elements, or vice versa).Safeguard both
forloops by explicitly validating that the target list is not null before iteration.🐛 Proposed fix structure
Wrap each loop block with a null check:
if ((null != labResultsList && labResultsList.size() > 0) || (null != wrapperLabResults.getRadiologyTestResults() && wrapperLabResults.getRadiologyTestResults().size() > 0)) { List<LabResultEntry> labResultsListNew = new ArrayList<LabResultEntry>(); - for (LabResultEntry labResult : labResultsList) { + if (labResultsList != null) { + for (LabResultEntry labResult : labResultsList) { List<Map<String, String>> compResult = labResult.getCompList(); if (null != compResult && compResult.size() > 0) { // ... existing lab components loop ... } } + } - for (LabResultEntry labResultEntry : wrapperLabResults.getRadiologyTestResults()) { + if (wrapperLabResults.getRadiologyTestResults() != null) { + for (LabResultEntry labResultEntry : wrapperLabResults.getRadiologyTestResults()) { labResultEntry.setBeneficiaryRegID(wrapperLabResults.getBeneficiaryRegID()); // ... existing radiology items loop ... labResultsListNew.add(labResultEntry); } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/labtechnician/LabTechnicianServiceImpl.java` around lines 417 - 466, Guard both list iterations in the result-processing block: only iterate labResultsList when it is non-null, and only iterate wrapperLabResults.getRadiologyTestResults() when that list is non-null. Preserve the existing processing logic and outer condition while preventing partially populated payloads from reaching either enhanced for loop with a null list.src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java (2)
152-169: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDuplicated
isOriginAllowedorigin-matching logic in two files, both flagged for a non-literal-regex ReDoS risk.The exact same pattern-to-regex conversion and
origin.matches(regex)call is copy-pasted intoJwtUserIdValidationFilterandHTTPRequestInterceptor(the latter's own javadoc says it mirrors the former "for consistency"). Static analysis flags both call sites since the regex is built dynamically from config and matched against attacker-influencedOrigininput, risking catastrophic backtracking if the configured allow-list pattern is complex. Extracting one shared, hardened matcher fixes both the duplication and the ReDoS exposure in a single place instead of two.
src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java#L152-L169: extract this method into a shared component (e.g.,CorsOriginMatcher), and harden it (e.g., precompile/cache patterns once at startup instead of rebuilding regex per call, and/or validate configured patterns to avoid pathological wildcard combinations).src/main/java/com/iemr/tm/utils/http/HTTPRequestInterceptor.java#L149-L163: delete this duplicate copy and delegate to the same shared matcher used byJwtUserIdValidationFilter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java` around lines 152 - 169, Extract the origin-pattern conversion and matching from isOriginAllowed in src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java lines 152-169 into one shared, hardened CorsOriginMatcher component, validating or safely precompiling configured patterns to prevent pathological regex backtracking while preserving current allow-list behavior. Remove the duplicate matching logic from src/main/java/com/iemr/tm/utils/http/HTTPRequestInterceptor.java lines 149-163 and make its origin validation delegate to the shared matcher; both callers must retain their existing null and empty-configuration handling.Source: Linters/SAST tools
40-104: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winShort-circuit allowed
OPTIONSrequests before JWT validation. This filter is registered on/*withOrdered.HIGHEST_PRECEDENCE, so preflight requests hit it first. For allowed origins, return a 2xx immediately after setting the CORS headers; otherwise browser requests to protected endpoints will fail on preflight with401and never reach the API.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java` around lines 40 - 104, Update the OPTIONS branch in JwtUserIdValidationFilter to short-circuit allowed preflight requests after CORS headers are configured, returning a successful 2xx response without continuing into JWT validation. Preserve the existing rejection behavior for missing or unauthorized origins, and ensure non-OPTIONS requests continue through the current filter flow.
🧹 Nitpick comments (8)
src/main/java/com/iemr/tm/controller/teleconsultation/TeleConsultationController.java (1)
38-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused imports and injected fields across controllers.
Both
TeleConsultationControllerandVideoConsultationControllerdeclare imports forCookieUtil,JwtUtil(andHttpServletRequest) and inject aJwtUtilbean. Since the endpoint authentication logic has been refactored to rely exclusively on theAuthenticationprincipal passed as a method parameter, these utilities are no longer used.
src/main/java/com/iemr/tm/controller/teleconsultation/TeleConsultationController.java#L38-L40: Remove theCookieUtilandJwtUtilimports.src/main/java/com/iemr/tm/controller/teleconsultation/TeleConsultationController.java#L58-L60: Remove the unused@Autowired private JwtUtil jwtUtil;field.src/main/java/com/iemr/tm/controller/videoconsultationcontroller/VideoConsultationController.java#L37-L40: Remove theHttpServletRequest,CookieUtil, andJwtUtilimports.src/main/java/com/iemr/tm/controller/videoconsultationcontroller/VideoConsultationController.java#L53-L54: Remove the unused@Autowired private JwtUtil jwtUtil;field.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/controller/teleconsultation/TeleConsultationController.java` around lines 38 - 40, Remove the unused CookieUtil and JwtUtil imports and injected JwtUtil field from TeleConsultationController at src/main/java/com/iemr/tm/controller/teleconsultation/TeleConsultationController.java lines 38-40 and 58-60. In VideoConsultationController, remove the unused HttpServletRequest, CookieUtil, and JwtUtil imports plus the injected JwtUtil field at src/main/java/com/iemr/tm/controller/videoconsultationcontroller/VideoConsultationController.java lines 37-40 and 53-54; keep authentication based on the Authentication method parameter.src/main/java/com/iemr/tm/controller/cancerscreening/CancerScreeningController.java (1)
143-143: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify
@PreAuthorizerole checks withhasAnyRole.Using Spring Security's native
hasAnyRole('ROLE1', 'ROLE2')provides a more concise and idiomatic expression than chaining multiplehasRole(...) || hasRole(...)blocks. Consider updating the authorization logic across the controller endpoints to improve readability.
src/main/java/com/iemr/tm/controller/cancerscreening/CancerScreeningController.java#L143-L143: usehasAnyRole('NURSE', 'DOCTOR').src/main/java/com/iemr/tm/controller/cancerscreening/CancerScreeningController.java#L173-L173: usehasAnyRole('NURSE', 'DOCTOR').src/main/java/com/iemr/tm/controller/cancerscreening/CancerScreeningController.java#L203-L203: usehasAnyRole('NURSE', 'DOCTOR').src/main/java/com/iemr/tm/controller/cancerscreening/CancerScreeningController.java#L233-L233: usehasAnyRole('NURSE', 'DOCTOR').src/main/java/com/iemr/tm/controller/cancerscreening/CancerScreeningController.java#L263-L263: usehasAnyRole('NURSE', 'DOCTOR').src/main/java/com/iemr/tm/controller/cancerscreening/CancerScreeningController.java#L295-L295: usehasAnyRole('NURSE', 'DOCTOR').src/main/java/com/iemr/tm/controller/cancerscreening/CancerScreeningController.java#L327-L327: usehasAnyRole('NURSE', 'DOCTOR').src/main/java/com/iemr/tm/controller/cancerscreening/CancerScreeningController.java#L359-L359: usehasAnyRole('NURSE', 'DOCTOR').src/main/java/com/iemr/tm/controller/cancerscreening/CancerScreeningController.java#L390-L390: usehasAnyRole('NURSE', 'DOCTOR').src/main/java/com/iemr/tm/controller/cancerscreening/CancerScreeningController.java#L417-L417: usehasAnyRole('NURSE', 'DOCTOR').src/main/java/com/iemr/tm/controller/cancerscreening/CancerScreeningController.java#L469-L469: usehasAnyRole('NURSE', 'DOCTOR').src/main/java/com/iemr/tm/controller/cancerscreening/CancerScreeningController.java#L508-L508: usehasAnyRole('NURSE', 'DOCTOR').src/main/java/com/iemr/tm/controller/cancerscreening/CancerScreeningController.java#L544-L544: usehasAnyRole('NURSE', 'DOCTOR', 'ONCOLOGIST').src/main/java/com/iemr/tm/controller/ncdscreening/NCDScreeningController.java#L143-L143: usehasAnyRole('NURSE', 'DOCTOR').src/main/java/com/iemr/tm/controller/ncdscreening/NCDScreeningController.java#L169-L169: usehasAnyRole('NURSE', 'DOCTOR').src/main/java/com/iemr/tm/controller/ncdscreening/NCDScreeningController.java#L198-L198: usehasAnyRole('NURSE', 'DOCTOR').src/main/java/com/iemr/tm/controller/ncdscreening/NCDScreeningController.java#L226-L226: usehasAnyRole('NURSE', 'DOCTOR').src/main/java/com/iemr/tm/controller/ncdscreening/NCDScreeningController.java#L253-L253: usehasAnyRole('NURSE', 'DOCTOR').src/main/java/com/iemr/tm/controller/ncdscreening/NCDScreeningController.java#L279-L279: usehasAnyRole('NURSE', 'DOCTOR').src/main/java/com/iemr/tm/controller/ncdscreening/NCDScreeningController.java#L306-L306: usehasAnyRole('NURSE', 'DOCTOR').src/main/java/com/iemr/tm/controller/ncdscreening/NCDScreeningController.java#L369-L369: usehasAnyRole('NURSE', 'DOCTOR').src/main/java/com/iemr/tm/controller/ncdscreening/NCDScreeningController.java#L398-L398: usehasAnyRole('NURSE', 'DOCTOR').src/main/java/com/iemr/tm/controller/ncdscreening/NCDScreeningController.java#L427-L427: usehasAnyRole('NURSE', 'DOCTOR').🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/controller/cancerscreening/CancerScreeningController.java` at line 143, Replace the chained hasRole authorization expressions on the affected endpoints with hasAnyRole expressions. In CancerScreeningController at lines 143, 173, 203, 233, 263, 295, 327, 359, 390, 417, 469, and 508, use NURSE and DOCTOR; at line 544, include ONCOLOGIST as well. Apply the same NURSE/DOCTOR hasAnyRole expression in NCDScreeningController at lines 143, 169, 198, 226, 253, 279, 306, 369, 398, and 427, preserving the existing access rules.src/main/java/com/iemr/tm/service/health/HealthService.java (1)
90-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider externalizing
ADVANCED_HEALTH_CHECKS_ENABLED.It's hardcoded
true; advanced checks add extraINFORMATION_SCHEMAqueries per (throttled) health check. Exposing this via@Valuewith a sane default would allow disabling the extra DB load without a redeploy if it ever becomes a concern in production.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/health/HealthService.java` at line 90, Externalize ADVANCED_HEALTH_CHECKS_ENABLED instead of hardcoding it to true, injecting it through the service’s configuration mechanism (such as `@Value`) with a default of true. Preserve the existing advanced health-check behavior while allowing operators to disable the extra database queries through configuration.src/main/java/com/iemr/tm/service/generalOPD/GeneralOPDServiceImpl.java (1)
757-761: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
doctorSignatureFlagparsing logic.The same 4-line null-safe boolean extraction is repeated in both
saveDoctorDataandupdateGeneralOPDDoctorData. Consider extracting a small private helper (e.g.parseDoctorSignatureFlag(JsonObject requestOBJ)) to keep the two call sites in sync if the field name or semantics change.Also applies to: 1368-1372
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/generalOPD/GeneralOPDServiceImpl.java` around lines 757 - 761, Extract the repeated null-safe doctorSignatureFlag parsing from saveDoctorData and updateGeneralOPDDoctorData into a private helper such as parseDoctorSignatureFlag(JsonObject requestOBJ). Replace both inline extraction blocks with calls to the helper, preserving the current false default and boolean conversion behavior.src/main/java/com/iemr/tm/service/common/master/CommonMasterServiceImpl.java (1)
39-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInject the interface instead of the concrete implementation class.
Injecting the concrete implementation (
LabTechnicianServiceImpl) instead of its interface (LabTechnicianService) violates the Dependency Inversion Principle. It may also lead toBeanNotOfRequiredTypeExceptionfailures if Spring utilizes JDK dynamic proxies for transaction or security interception on the target bean.Prefer injecting the
LabTechnicianServiceinterface.♻️ Proposed refactor
- private LabTechnicianServiceImpl labTechnicianServiceImpl; + private LabTechnicianService labTechnicianService; `@Autowired` public void setNcdCareMasterDataServiceImpl(NCDCareMasterDataServiceImpl ncdCareMasterDataServiceImpl) { this.ncdCareMasterDataServiceImpl = ncdCareMasterDataServiceImpl; } `@Autowired` - public void setLabTechnicianServiceImpl(LabTechnicianServiceImpl labTechnicianServiceImpl) { - this.labTechnicianServiceImpl = labTechnicianServiceImpl; + public void setLabTechnicianService(LabTechnicianService labTechnicianService) { + this.labTechnicianService = labTechnicianService; }Make sure to update the usage on line 234:
`@Override` public String getECGAbnormalFindings() { - return labTechnicianServiceImpl.getECGAbnormalFindings(); + return labTechnicianService.getECGAbnormalFindings(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/common/master/CommonMasterServiceImpl.java` around lines 39 - 49, Update CommonMasterServiceImpl to depend on the LabTechnicianService interface instead of LabTechnicianServiceImpl: change the field and setLabTechnicianServiceImpl setter parameter accordingly, while preserving the existing setter wiring. Also update the usage around the referenced call site to use the interface-typed dependency.src/main/java/com/iemr/tm/utils/IntegerListConverter.java (1)
35-51: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueCache the
TypeTokentype to prevent unnecessary object churn.Using
new TypeToken<List<Integer>>(){}.getType()recreates an anonymous inner class and executes reflection on every invocation ofconvertToEntityAttribute. Moving it to astatic finalconstant is better for performance and memory allocation.♻️ Proposed refactor
import com.google.gson.Gson; import com.google.gson.reflect.TypeToken; +import java.lang.reflect.Type; `@Converter` public class IntegerListConverter implements AttributeConverter<List<Integer>, String> { private final Gson gson = new Gson(); + private static final Type LIST_TYPE = new TypeToken<List<Integer>>(){}.getType(); `@Override` public String convertToDatabaseColumn(List<Integer> attribute) { if (attribute == null || attribute.isEmpty()) { return null; } - return gson.toJson(attribute); + return gson.toJson(attribute, LIST_TYPE); } `@Override` public List<Integer> convertToEntityAttribute(String dbData) { if (dbData == null || dbData.trim().isEmpty()) { return null; } - return gson.fromJson(dbData, new TypeToken<List<Integer>>(){}.getType()); + return gson.fromJson(dbData, LIST_TYPE); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/utils/IntegerListConverter.java` around lines 35 - 51, Cache the Gson type metadata used by IntegerListConverter in a static final Type field, initialized once from the List<Integer> TypeToken. Update convertToEntityAttribute to reuse this constant instead of creating a new TypeToken on each invocation, while preserving the existing null and blank-input behavior.src/main/java/com/iemr/tm/service/registrar/RegistrarServiceImpl.java (1)
117-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider depending on
RedisConnectionFactoryinstead of the concreteLettuceConnectionFactory.Coding to the
RedisConnectionFactoryinterface (which is all that's used here viagetConnection()) improves testability and decouples this class from the specific Redis client implementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/registrar/RegistrarServiceImpl.java` around lines 117 - 118, Change the injected field in RegistrarServiceImpl from LettuceConnectionFactory to the RedisConnectionFactory interface, retaining the existing getConnection() usage and removing the concrete-client dependency.src/main/java/com/iemr/tm/service/ncdCare/NCDCareServiceImpl.java (1)
774-778: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix indentation for readability.
The assignment statement inside the
ifblock is missing an indentation level. Correcting the indentation ensures the code remains clean and visually clear.
src/main/java/com/iemr/tm/service/ncdCare/NCDCareServiceImpl.java#L774-L778: indentdoctorSignatureFlag = ...correctly.src/main/java/com/iemr/tm/service/ncdCare/NCDCareServiceImpl.java#L1211-L1215: indentdoctorSignatureFlag = ...correctly.🧹 Proposed fixes
For lines 774-778:
Boolean doctorSignatureFlag = false; if (requestOBJ.has("doctorSignatureFlag") && !requestOBJ.get("doctorSignatureFlag").isJsonNull()) { - doctorSignatureFlag = requestOBJ.get("doctorSignatureFlag").getAsBoolean(); + doctorSignatureFlag = requestOBJ.get("doctorSignatureFlag").getAsBoolean(); }For lines 1211-1215:
Boolean doctorSignatureFlag = false; if (requestOBJ.has("doctorSignatureFlag") && !requestOBJ.get("doctorSignatureFlag").isJsonNull()) { - doctorSignatureFlag = requestOBJ.get("doctorSignatureFlag").getAsBoolean(); + doctorSignatureFlag = requestOBJ.get("doctorSignatureFlag").getAsBoolean(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/ncdCare/NCDCareServiceImpl.java` around lines 774 - 778, Correct the indentation of the doctorSignatureFlag assignment inside the if block at src/main/java/com/iemr/tm/service/ncdCare/NCDCareServiceImpl.java lines 774-778 and lines 1211-1215, without changing its behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/iemr/tm/controller/common/main/WorklistController.java`:
- Around line 710-731: Add the same `@PreAuthorize` role restriction used by
neighboring worklist handlers to getTCSpecialistWorkListNew and the other two TC
specialist worklist endpoint methods. Preserve their existing authentication and
response logic while applying the established role guard consistently to all
three endpoints.
In `@src/main/java/com/iemr/tm/controller/health/HealthController.java`:
- Around line 73-82: Update the catch-all error response in HealthController to
include a components map matching the success schema, including the mysql status
entry, while preserving the existing DOWN status, timestamp, and
SERVICE_UNAVAILABLE response.
- Around line 40-43: Update the error response construction in HealthController
to include the same top-level keys as the successful health response,
specifically adding components alongside the existing status fields. Preserve
the current failure status and error details while ensuring both response paths
share a consistent shape.
In
`@src/main/java/com/iemr/tm/service/benFlowStatus/CommonBenStatusFlowServiceImpl.java`:
- Line 30: Remove the unused org.checkerframework.checker.units.qual.s import
from CommonBenStatusFlowServiceImpl, leaving the remaining imports and class
implementation unchanged.
In `@src/main/java/com/iemr/tm/service/cancerScreening/CSServiceImpl.java`:
- Around line 814-819: Update the doctorSignatureFlag initialization block in
CSServiceImpl to guard requestOBJ before calling has or get; only read
doctorSignatureFlag when requestOBJ is non-null and the property is present and
non-null, preserving false as the default for a null requestOBJ.
In `@src/main/java/com/iemr/tm/service/common/transaction/CommonServiceImpl.java`:
- Around line 583-593: The file URL extraction logic in CommonServiceImpl must
not return dataVal.toString() when a JSONObject lacks the response key. Update
this fallback to signal that no usable URL was found, matching the existing
error or empty-result contract consumed by WorklistController.getKMFile, while
preserving response extraction and URL normalization for valid objects.
- Around line 566-595: Update the response logging in the OpenKM exchange flow
before parsing responseBody: remove raw response-body logging at INFO level, or
log only a redacted version at DEBUG level by stripping embedded credentials
before “@”. Preserve the existing response parsing and URL normalization in the
surrounding fileUUID handling.
In `@src/main/java/com/iemr/tm/utils/JwtAuthenticationUtil.java`:
- Around line 134-147: Update getUserRoles so the intentionally thrown
IEMRException for a missing role is not caught and re-wrapped by the general
exception handler. Preserve its original message, while continuing to wrap
unexpected exceptions with the original exception as the cause when constructing
the failure IEMRException.
In `@src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java`:
- Around line 68-78: Update the allowed-origin branch in
JwtUserIdValidationFilter so it restores the Vary response header with the value
Origin alongside Access-Control-Allow-Origin. Keep this header limited to
responses where isOriginAllowed(origin) succeeds.
In `@src/main/java/com/iemr/tm/utils/mapper/RoleAuthenticationFilter.java`:
- Around line 150-165: Update resolveAuthToken in RoleAuthenticationFilter to
remove the leading “Bearer ” scheme prefix before returning the token, matching
HTTPRequestInterceptor.preHandle and preserving raw-token Redis session lookup.
Apply this normalization to the resolved Authorization token while leaving other
header and cookie fallback behavior unchanged.
In `@src/main/java/com/iemr/tm/utils/mapper/SecurityConfig.java`:
- Around line 38-40: Update the SecurityConfig HTTP security chain to retain
CSRF protection for requests authenticated through the Jwttoken cookie, using
the existing JwtUserIdValidationFilter/CookieUtil authentication path as the
reference. Do not leave csrf globally disabled; configure an appropriate CSRF
token repository and cookie-based request handling while preserving stateless
session management and header-token behavior.
In `@src/main/java/com/iemr/tm/utils/redis/RedisStorage.java`:
- Around line 102-112: Update cacheUserRoles and the role-assignment mutation
flow to prevent stale authorities: shorten the current 30-minute TTL and add
explicit deletion of the user’s "roles:" cache key whenever roles are assigned,
revoked, or changed. Ensure RoleAuthenticationFilter observes the updated roles
after mutation while preserving the existing cache write behavior.
---
Outside diff comments:
In `@src/main/java/com/iemr/tm/service/common/transaction/CommonServiceImpl.java`:
- Around line 557-601: The getOpenKMDocURL flow must enforce record-level
beneficiary/visit ownership before benVisitDetailRepo.getFileUUID resolves the
caller-supplied fileID; reuse the project’s existing authorization check and
return or reject unauthorized requests. In WorklistController.java lines
806-824, add a method-level `@PreAuthorize` guard to getKMFile using the
appropriate role expression, preserving existing behavior for authorized
callers.
In
`@src/main/java/com/iemr/tm/service/labtechnician/LabTechnicianServiceImpl.java`:
- Around line 417-466: Guard both list iterations in the result-processing
block: only iterate labResultsList when it is non-null, and only iterate
wrapperLabResults.getRadiologyTestResults() when that list is non-null. Preserve
the existing processing logic and outer condition while preventing partially
populated payloads from reaching either enhanced for loop with a null list.
In `@src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java`:
- Around line 152-169: Extract the origin-pattern conversion and matching from
isOriginAllowed in
src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java lines 152-169
into one shared, hardened CorsOriginMatcher component, validating or safely
precompiling configured patterns to prevent pathological regex backtracking
while preserving current allow-list behavior. Remove the duplicate matching
logic from src/main/java/com/iemr/tm/utils/http/HTTPRequestInterceptor.java
lines 149-163 and make its origin validation delegate to the shared matcher;
both callers must retain their existing null and empty-configuration handling.
- Around line 40-104: Update the OPTIONS branch in JwtUserIdValidationFilter to
short-circuit allowed preflight requests after CORS headers are configured,
returning a successful 2xx response without continuing into JWT validation.
Preserve the existing rejection behavior for missing or unauthorized origins,
and ensure non-OPTIONS requests continue through the current filter flow.
---
Nitpick comments:
In
`@src/main/java/com/iemr/tm/controller/cancerscreening/CancerScreeningController.java`:
- Line 143: Replace the chained hasRole authorization expressions on the
affected endpoints with hasAnyRole expressions. In CancerScreeningController at
lines 143, 173, 203, 233, 263, 295, 327, 359, 390, 417, 469, and 508, use NURSE
and DOCTOR; at line 544, include ONCOLOGIST as well. Apply the same NURSE/DOCTOR
hasAnyRole expression in NCDScreeningController at lines 143, 169, 198, 226,
253, 279, 306, 369, 398, and 427, preserving the existing access rules.
In
`@src/main/java/com/iemr/tm/controller/teleconsultation/TeleConsultationController.java`:
- Around line 38-40: Remove the unused CookieUtil and JwtUtil imports and
injected JwtUtil field from TeleConsultationController at
src/main/java/com/iemr/tm/controller/teleconsultation/TeleConsultationController.java
lines 38-40 and 58-60. In VideoConsultationController, remove the unused
HttpServletRequest, CookieUtil, and JwtUtil imports plus the injected JwtUtil
field at
src/main/java/com/iemr/tm/controller/videoconsultationcontroller/VideoConsultationController.java
lines 37-40 and 53-54; keep authentication based on the Authentication method
parameter.
In
`@src/main/java/com/iemr/tm/service/common/master/CommonMasterServiceImpl.java`:
- Around line 39-49: Update CommonMasterServiceImpl to depend on the
LabTechnicianService interface instead of LabTechnicianServiceImpl: change the
field and setLabTechnicianServiceImpl setter parameter accordingly, while
preserving the existing setter wiring. Also update the usage around the
referenced call site to use the interface-typed dependency.
In `@src/main/java/com/iemr/tm/service/generalOPD/GeneralOPDServiceImpl.java`:
- Around line 757-761: Extract the repeated null-safe doctorSignatureFlag
parsing from saveDoctorData and updateGeneralOPDDoctorData into a private helper
such as parseDoctorSignatureFlag(JsonObject requestOBJ). Replace both inline
extraction blocks with calls to the helper, preserving the current false default
and boolean conversion behavior.
In `@src/main/java/com/iemr/tm/service/health/HealthService.java`:
- Line 90: Externalize ADVANCED_HEALTH_CHECKS_ENABLED instead of hardcoding it
to true, injecting it through the service’s configuration mechanism (such as
`@Value`) with a default of true. Preserve the existing advanced health-check
behavior while allowing operators to disable the extra database queries through
configuration.
In `@src/main/java/com/iemr/tm/service/ncdCare/NCDCareServiceImpl.java`:
- Around line 774-778: Correct the indentation of the doctorSignatureFlag
assignment inside the if block at
src/main/java/com/iemr/tm/service/ncdCare/NCDCareServiceImpl.java lines 774-778
and lines 1211-1215, without changing its behavior.
In `@src/main/java/com/iemr/tm/service/registrar/RegistrarServiceImpl.java`:
- Around line 117-118: Change the injected field in RegistrarServiceImpl from
LettuceConnectionFactory to the RedisConnectionFactory interface, retaining the
existing getConnection() usage and removing the concrete-client dependency.
In `@src/main/java/com/iemr/tm/utils/IntegerListConverter.java`:
- Around line 35-51: Cache the Gson type metadata used by IntegerListConverter
in a static final Type field, initialized once from the List<Integer> TypeToken.
Update convertToEntityAttribute to reuse this constant instead of creating a new
TypeToken on each invocation, while preserving the existing null and blank-input
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd8740c7-861f-4f11-867e-a403654fdd98
📒 Files selected for processing (67)
pom.xmlsrc/main/environment/common_ci.propertiessrc/main/environment/common_docker.propertiessrc/main/environment/common_example.propertiessrc/main/java/com/iemr/tm/controller/anc/AntenatalCareController.javasrc/main/java/com/iemr/tm/controller/cancerscreening/CancerScreeningController.javasrc/main/java/com/iemr/tm/controller/common/main/WorklistController.javasrc/main/java/com/iemr/tm/controller/common/master/CommonMasterController.javasrc/main/java/com/iemr/tm/controller/covid19/CovidController.javasrc/main/java/com/iemr/tm/controller/dataSyncActivity/StartSyncActivity.javasrc/main/java/com/iemr/tm/controller/dataSyncLayerCentral/MMUDataSyncVanToServer.javasrc/main/java/com/iemr/tm/controller/foetalmonitor/FoetalMonitorController.javasrc/main/java/com/iemr/tm/controller/generalOPD/GeneralOPDController.javasrc/main/java/com/iemr/tm/controller/health/HealthController.javasrc/main/java/com/iemr/tm/controller/labtechnician/LabtechnicianController.javasrc/main/java/com/iemr/tm/controller/location/LocationController.javasrc/main/java/com/iemr/tm/controller/login/IemrMmuLoginController.javasrc/main/java/com/iemr/tm/controller/ncdCare/NCDCareController.javasrc/main/java/com/iemr/tm/controller/ncdscreening/NCDScreeningController.javasrc/main/java/com/iemr/tm/controller/nurse/vitals/AnthropometryVitalsController.javasrc/main/java/com/iemr/tm/controller/patientApp/master/PatientAppCommonMasterController.javasrc/main/java/com/iemr/tm/controller/pnc/PostnatalCareController.javasrc/main/java/com/iemr/tm/controller/quickconsult/QuickConsultController.javasrc/main/java/com/iemr/tm/controller/registrar/main/RegistrarController.javasrc/main/java/com/iemr/tm/controller/report/CRMReportController.javasrc/main/java/com/iemr/tm/controller/snomedct/SnomedController.javasrc/main/java/com/iemr/tm/controller/teleconsultation/TeleConsultationController.javasrc/main/java/com/iemr/tm/controller/version/VersionController.javasrc/main/java/com/iemr/tm/controller/videoconsultationcontroller/VideoConsultationController.javasrc/main/java/com/iemr/tm/data/benFlowStatus/BeneficiaryFlowStatus.javasrc/main/java/com/iemr/tm/data/labModule/ECGAbnormalFindingMaster.javasrc/main/java/com/iemr/tm/data/labModule/LabResultEntry.javasrc/main/java/com/iemr/tm/data/ncdcare/NCDCareDiagnosis.javasrc/main/java/com/iemr/tm/repo/benFlowStatus/BeneficiaryFlowStatusRepo.javasrc/main/java/com/iemr/tm/repo/labModule/ECGAbnormalFindingMasterRepo.javasrc/main/java/com/iemr/tm/repo/login/UserLoginRepo.javasrc/main/java/com/iemr/tm/repo/nurse/ncdcare/NCDCareDiagnosisRepo.javasrc/main/java/com/iemr/tm/service/anc/ANCServiceImpl.javasrc/main/java/com/iemr/tm/service/benFlowStatus/CommonBenStatusFlowServiceImpl.javasrc/main/java/com/iemr/tm/service/cancerScreening/CSServiceImpl.javasrc/main/java/com/iemr/tm/service/common/master/CommonMasterServiceImpl.javasrc/main/java/com/iemr/tm/service/common/master/CommonMaterService.javasrc/main/java/com/iemr/tm/service/common/transaction/CommonDoctorServiceImpl.javasrc/main/java/com/iemr/tm/service/common/transaction/CommonNurseServiceImpl.javasrc/main/java/com/iemr/tm/service/common/transaction/CommonServiceImpl.javasrc/main/java/com/iemr/tm/service/covid19/Covid19ServiceImpl.javasrc/main/java/com/iemr/tm/service/generalOPD/GeneralOPDServiceImpl.javasrc/main/java/com/iemr/tm/service/health/HealthService.javasrc/main/java/com/iemr/tm/service/labtechnician/LabTechnicianServiceImpl.javasrc/main/java/com/iemr/tm/service/ncdCare/NCDCareServiceImpl.javasrc/main/java/com/iemr/tm/service/ncdscreening/NCDSCreeningDoctorServiceImpl.javasrc/main/java/com/iemr/tm/service/ncdscreening/NCDScreeningServiceImpl.javasrc/main/java/com/iemr/tm/service/pnc/PNCServiceImpl.javasrc/main/java/com/iemr/tm/service/quickConsultation/QuickConsultationServiceImpl.javasrc/main/java/com/iemr/tm/service/registrar/RegistrarServiceImpl.javasrc/main/java/com/iemr/tm/utils/CookieUtil.javasrc/main/java/com/iemr/tm/utils/IntegerListConverter.javasrc/main/java/com/iemr/tm/utils/JwtAuthenticationUtil.javasrc/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.javasrc/main/java/com/iemr/tm/utils/JwtUtil.javasrc/main/java/com/iemr/tm/utils/StringListConverter.javasrc/main/java/com/iemr/tm/utils/exception/CustomAccessDeniedHandler.javasrc/main/java/com/iemr/tm/utils/exception/CustomAuthenticationEntryPoint.javasrc/main/java/com/iemr/tm/utils/http/HTTPRequestInterceptor.javasrc/main/java/com/iemr/tm/utils/mapper/RoleAuthenticationFilter.javasrc/main/java/com/iemr/tm/utils/mapper/SecurityConfig.javasrc/main/java/com/iemr/tm/utils/redis/RedisStorage.java
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (12)
src/main/java/com/iemr/tm/controller/common/main/WorklistController.java (1)
710-731: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add the same role guard to the three TC specialist worklist endpoints.
They only checkauthentication.isAuthenticated(), while the neighboring worklist handlers require a role. Add the same@PreAuthorize(...)restriction here so access control stays consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/controller/common/main/WorklistController.java` around lines 710 - 731, Add the same `@PreAuthorize` role restriction used by neighboring worklist handlers to getTCSpecialistWorkListNew and the other two TC specialist worklist endpoint methods. Preserve their existing authentication and response logic while applying the established role guard consistently to all three endpoints.src/main/java/com/iemr/tm/controller/health/HealthController.java (2)
40-43: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
git ls-files | rg 'src/main/java/.*/(HealthController|HealthService|SecurityConfig)\.java$'Repository: PSMRI/TM-API
Length of output: 335
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== files ==\n' git ls-files | rg 'src/main/java/.*/(HealthController|HealthService|SecurityConfig)\.java$' printf '\n== controller outline ==\n' ast-grep outline src/main/java/com/iemr/tm/controller/health/HealthController.java --view expanded || true printf '\n== security config outline ==\n' ast-grep outline src/main/java/com/iemr/tm/config/SecurityConfig.java --view expanded || true printf '\n== health service outline ==\n' ast-grep outline src/main/java/com/iemr/tm/service/HealthService.java --view expanded || trueRepository: PSMRI/TM-API
Length of output: 828
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== HealthController.java ==\n' sed -n '1,220p' src/main/java/com/iemr/tm/controller/health/HealthController.java printf '\n== HealthService.java ==\n' sed -n '1,260p' src/main/java/com/iemr/tm/service/health/HealthService.java printf '\n== SecurityConfig.java ==\n' sed -n '1,260p' src/main/java/com/iemr/tm/utils/mapper/SecurityConfig.javaRepository: PSMRI/TM-API
Length of output: 17377
Keep the failure response shape consistent
HealthController.java:76-79omitscomponents, while the success path always returns it. Add the same top-level keys on the error path so callers don’t fail only when health checks break.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/controller/health/HealthController.java` around lines 40 - 43, Update the error response construction in HealthController to include the same top-level keys as the successful health response, specifically adding components alongside the existing status fields. Preserve the current failure status and error details while ensuring both response paths share a consistent shape.
73-82: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Error-path response omits
components, unlike the success schema.The catch-all fallback returns only
{status, timestamp}, while the normal path always includes acomponentsmap. A client that unconditionally readscomponents.mysql.statuswill fail specifically when the health check itself throws — the scenario where a reliable response matters most.🩹 Suggested fix
Map<String, Object> errorResponse = Map.of( "status", "DOWN", - "timestamp", Instant.now().toString() + "timestamp", Instant.now().toString(), + "components", Map.of() );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.} catch (Exception e) { logger.error("Unexpected error during health check", e); Map<String, Object> errorResponse = Map.of( "status", "DOWN", "timestamp", Instant.now().toString(), "components", Map.of() ); return new ResponseEntity<>(errorResponse, HttpStatus.SERVICE_UNAVAILABLE); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/controller/health/HealthController.java` around lines 73 - 82, Update the catch-all error response in HealthController to include a components map matching the success schema, including the mysql status entry, while preserving the existing DOWN status, timestamp, and SERVICE_UNAVAILABLE response.src/main/java/com/iemr/tm/service/benFlowStatus/CommonBenStatusFlowServiceImpl.java (1)
30-30: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the unresolved and unused import.
This import appears to be an accidental inclusion (e.g., from an IDE auto-import). It is entirely unused within the class and will cause
javaccompilation failures if thechecker-qualpackage is not available on the classpath.🧹 Proposed fix
-import org.checkerframework.checker.units.qual.s;📝 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.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/benFlowStatus/CommonBenStatusFlowServiceImpl.java` at line 30, Remove the unused org.checkerframework.checker.units.qual.s import from CommonBenStatusFlowServiceImpl, leaving the remaining imports and class implementation unchanged.src/main/java/com/iemr/tm/service/cancerScreening/CSServiceImpl.java (1)
814-819: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Add a null check for
requestOBJto prevent a potentialNullPointerException.The code unconditionally invokes
requestOBJ.has(...), but the downstream logic (e.g., the null guard at line 820) indicates thatrequestOBJcan legitimately be null. If a null object is provided, this block will crash the request execution.🛡️ Proposed fix to add the null guard and correct the indentation
Boolean doctorSignatureFlag = false; - if (requestOBJ.has("doctorSignatureFlag") && !requestOBJ.get("doctorSignatureFlag").isJsonNull()) { - doctorSignatureFlag = requestOBJ.get("doctorSignatureFlag").getAsBoolean(); - } + if (requestOBJ != null && requestOBJ.has("doctorSignatureFlag") && !requestOBJ.get("doctorSignatureFlag").isJsonNull()) { + doctorSignatureFlag = requestOBJ.get("doctorSignatureFlag").getAsBoolean(); + }📝 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.Boolean doctorSignatureFlag = false; if (requestOBJ != null && requestOBJ.has("doctorSignatureFlag") && !requestOBJ.get("doctorSignatureFlag").isJsonNull()) { doctorSignatureFlag = requestOBJ.get("doctorSignatureFlag").getAsBoolean(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/cancerScreening/CSServiceImpl.java` around lines 814 - 819, Update the doctorSignatureFlag initialization block in CSServiceImpl to guard requestOBJ before calling has or get; only read doctorSignatureFlag when requestOBJ is non-null and the property is present and non-null, preserving false as the default for a null requestOBJ.src/main/java/com/iemr/tm/service/common/transaction/CommonServiceImpl.java (2)
566-595: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Logging the raw OpenKM response can leak embedded credentials.
The method's own comment documents that the extracted URL can look like
https://user:pass@https://host, meaning the response body legitimately contains Basic-Auth-style credentials embedded in a URL.logger.info("Response=" + response.getBody())writes that entire body — credentials included — into application logs before any redaction happens.🔒 Suggested fix
- logger.info("Response=" + response.getBody()); String responseBody = response.getBody();If response-body logging is needed for debugging, redact the credentials portion (e.g. strip anything before
@) before logging, and drop it to DEBUG level.📝 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.logger.info("fileUUID for fileID " + obj.getInt("fileID") + " is " + fileUUID); logger.info("openkmDocUrl is " + openkmDocUrl); if (fileUUID != null) { Map<String, Object> requestBody = new HashMap<>(); requestBody.put("fileUID", fileUUID); HttpEntity<Object> request = RestTemplateUtil.createRequestEntity(requestBody, Authorization); ResponseEntity<String> response = restTemplate.exchange(openkmDocUrl, HttpMethod.POST, request, String.class); String responseBody = response.getBody(); if (responseBody != null) { JSONObject responseObj = new JSONObject(responseBody); if (responseObj.has("data")) { Object dataVal = responseObj.get("data"); if (dataVal instanceof JSONObject) { JSONObject dataObj = (JSONObject) dataVal; if (dataObj.has("response")) { String fileUrl = dataObj.getString("response"); // Fix malformed URL: https://user:pass@https://host -> https://user:pass@host fileUrl = fileUrl.replaceAll("`@https`?://", "@"); return fileUrl; } } return dataVal.toString(); } } return responseBody;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/common/transaction/CommonServiceImpl.java` around lines 566 - 595, Update the response logging in the OpenKM exchange flow before parsing responseBody: remove raw response-body logging at INFO level, or log only a redacted version at DEBUG level by stripping embedded credentials before “@”. Preserve the existing response parsing and URL normalization in the surrounding fileUUID handling.
583-593: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Silent fallback returns a stringified JSON object as the "file URL".
When
datais aJSONObjectwithout aresponsekey,dataVal.toString()is returned as if it were the download URL. The caller (WorklistController.getKMFile) puts this directly into the client response viaresponse.setResponse(s), so the client would receive an unusable JSON blob instead of a URL, with no error signal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/common/transaction/CommonServiceImpl.java` around lines 583 - 593, The file URL extraction logic in CommonServiceImpl must not return dataVal.toString() when a JSONObject lacks the response key. Update this fallback to signal that no usable URL was found, matching the existing error or empty-result contract consumed by WorklistController.getKMFile, while preserving response extraction and URL normalization for valid objects.src/main/java/com/iemr/tm/utils/JwtAuthenticationUtil.java (1)
134-147: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Catch block re-wraps its own thrown
IEMRException, losing message clarity and cause.The
throw new IEMRException("No role found...")at Line 141 is caught by thecatch (Exception e)below it and re-wrapped into a confusing nested message ("Failed to retrieverole... error : No role found..."). The original exception's cause is also dropped sinceeisn't passed to the newIEMRException.🐛 Proposed fix
public List<String> getUserRoles(Long userId) throws IEMRException { if (null == userId || userId <= 0) { throw new IEMRException("Invalid User ID : " + userId); } try { List<String> role = userLoginRepo.getRoleNamebyUserId(userId); if (null == role || role.isEmpty()) { throw new IEMRException("No role found for userId : " + userId); } return role; + } catch (IEMRException e) { + throw e; } catch (Exception e) { - throw new IEMRException("Failed to retrieverole for usedId : " + userId + " error : " + e.getMessage()); + throw new IEMRException("Failed to retrieve role for userId : " + userId + " error : " + e.getMessage(), e); } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.public List<String> getUserRoles(Long userId) throws IEMRException { if (null == userId || userId <= 0) { throw new IEMRException("Invalid User ID : " + userId); } try { List<String> role = userLoginRepo.getRoleNamebyUserId(userId); if (null == role || role.isEmpty()) { throw new IEMRException("No role found for userId : " + userId); } return role; } catch (IEMRException e) { throw e; } catch (Exception e) { throw new IEMRException("Failed to retrieve role for userId : " + userId + " error : " + e.getMessage(), e); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/utils/JwtAuthenticationUtil.java` around lines 134 - 147, Update getUserRoles so the intentionally thrown IEMRException for a missing role is not caught and re-wrapped by the general exception handler. Preserve its original message, while continuing to wrap unexpected exceptions with the original exception as the cause when constructing the failure IEMRException.src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java (1)
68-78: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Vary: Originheader dropped when echoing the allowed origin.Previously the allowed-origin response set
Vary: Origin; this block no longer sets it. Without it, caches/CDNs sitting in front of this service could serve one origin's CORS-enabled response to a different origin.🛡️ Proposed fix
response.setHeader("Access-Control-Allow-Credentials", "true"); response.setHeader("Access-Control-Max-Age", "3600"); + response.setHeader("Vary", "Origin"); logger.info("Origin Validated | Origin: {} | Method: {} | URI: {}", origin, method, uri);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if (origin != null && isOriginAllowed(origin)) { response.setHeader("Access-Control-Allow-Origin", origin); // Never use wildcard response.setHeader("Access-Control-Allow-Methods", "GET, POST, PUT, PATCH, DELETE, OPTIONS"); response.setHeader("Access-Control-Allow-Headers", "Authorization, Content-Type, Accept, Jwttoken, serverAuthorization, ServerAuthorization, serverauthorization, Serverauthorization"); response.setHeader("Access-Control-Allow-Credentials", "true"); response.setHeader("Access-Control-Max-Age", "3600"); response.setHeader("Vary", "Origin"); logger.info("Origin Validated | Origin: {} | Method: {} | URI: {}", origin, method, uri); } else { logger.warn("Origin [{}] is NOT allowed. CORS headers NOT added.", origin); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java` around lines 68 - 78, Update the allowed-origin branch in JwtUserIdValidationFilter so it restores the Vary response header with the value Origin alongside Access-Control-Allow-Origin. Keep this header limited to responses where isOriginAllowed(origin) succeeds.src/main/java/com/iemr/tm/utils/mapper/RoleAuthenticationFilter.java (1)
150-165: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
resolveAuthTokendoesn't strip the"Bearer "prefix, breaking the legacy Redis session lookup for standard clients.
HTTPRequestInterceptor.preHandlestrips"Bearer "before using the token as a Redis key (preAuth.replace("Bearer ", "")). This method doesn't, so a client sendingAuthorization: Bearer <token>will haveauthToken = "Bearer <token>"passed toredisService.getObject(...), which won't find the session stored under the raw token — silently failing the legacy fallback path for standard Bearer-scheme clients.🐛 Proposed fix
private String resolveAuthToken(HttpServletRequest request) { String token = request.getHeader("Authorization"); + if (token != null && token.startsWith("Bearer ")) { + token = token.substring(7); + } if (token == null || token.isBlank()) { token = request.getHeader("AuthToken");📝 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.private String resolveAuthToken(HttpServletRequest request) { String token = request.getHeader("Authorization"); if (token != null && token.startsWith("Bearer ")) { token = token.substring(7); } if (token == null || token.isBlank()) { token = request.getHeader("AuthToken"); } if (token == null || token.isBlank()) { token = request.getHeader("X-Auth-Token"); } if (token == null || token.isBlank()) { token = CookieUtil.getCookieValue(request, "Authorization") .orElse(null); } return token; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/utils/mapper/RoleAuthenticationFilter.java` around lines 150 - 165, Update resolveAuthToken in RoleAuthenticationFilter to remove the leading “Bearer ” scheme prefix before returning the token, matching HTTPRequestInterceptor.preHandle and preserving raw-token Redis session lookup. Apply this normalization to the resolved Authorization token while leaving other header and cookie fallback behavior unchanged.src/main/java/com/iemr/tm/utils/mapper/SecurityConfig.java (1)
38-40: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
CSRF protection disabled while the app also authenticates via a cookie (
Jwttoken).CSRF is safe to disable only for stateless, header-only token auth with no browser-auto-submitted cookie. This codebase's
JwtUserIdValidationFilter/CookieUtiltreat theJwttokencookie as a first-class auth path, so state-changing requests authenticated via that cookie are not protected against forged cross-site requests withcsrf().disable().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/utils/mapper/SecurityConfig.java` around lines 38 - 40, Update the SecurityConfig HTTP security chain to retain CSRF protection for requests authenticated through the Jwttoken cookie, using the existing JwtUserIdValidationFilter/CookieUtil authentication path as the reference. Do not leave csrf globally disabled; configure an appropriate CSRF token repository and cookie-based request handling while preserving stateless session management and header-token behavior.Source: Linters/SAST tools
src/main/java/com/iemr/tm/utils/redis/RedisStorage.java (1)
102-112: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
30-minute role cache creates a stale-privilege window after role changes/revocations.
cacheUserRolessets a flat 30-minute TTL with no invalidation hook tied to role mutation elsewhere. SinceRoleAuthenticationFiltertreats this cache as authoritative forSecurityContextHolderauthorities, a revoked or downgraded role can remain effectively granted for up to 30 minutes after the change.Consider a shorter TTL and/or invalidating (
redisTemplate.delete("roles:" + userId)) the cache entry whenever a user's role assignment changes, rather than relying solely on time-based expiry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/utils/redis/RedisStorage.java` around lines 102 - 112, Update cacheUserRoles and the role-assignment mutation flow to prevent stale authorities: shorten the current 30-minute TTL and add explicit deletion of the user’s "roles:" cache key whenever roles are assigned, revoked, or changed. Ensure RoleAuthenticationFilter observes the updated roles after mutation while preserving the existing cache write behavior.
2 participants
📋 Description
JIRA ID:
Please provide a summary of the change and the motivation behind it. Include relevant context and details.
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit
New Features
Bug Fixes