Improve User Authentication to Prevent Enumeration #264
Conversation
WalkthroughThe changes introduce improved error handling in user controllers, extend the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CommonService
participant KMFileManagerRepo
Client->>CommonService: getSubCategories(request)
CommonService->>KMFileManagerRepo: findSubCategoriesByCategoryID(categoryID)
loop For each subcategory
CommonService->>KMFileManagerRepo: getFilesBySubCategoryID(subCategoryID)
KMFileManagerRepo-->>CommonService: List<KMFileManager>
CommonService->>CommonService: Attach file list to SubCategoryDetails
end
CommonService-->>Client: List<SubCategoryDetails> (with file lists)
sequenceDiagram
participant User
participant IEMRAdminController
participant IEMRAdminUserService
User->>IEMRAdminController: Authenticate/Password/Role request
IEMRAdminController->>IEMRAdminUserService: Call relevant method
IEMRAdminUserService-->>IEMRAdminController: Success or Exception
alt Exception thrown
IEMRAdminController->>User: response.setError(e)
else Success
IEMRAdminController->>User: Return success response
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (1)
1107-1107: Inconsistent error handling pattern.This method still uses
response.setError(5000, e.getMessage())while other methods have been changed toresponse.setError(e). Consider using a consistent approach across all methods.For consistency with security objectives:
- response.setError(5000, e.getMessage()); + response.setError(5000, "Unable to validate security answers. Please try again.");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java(9 hunks)src/main/java/com/iemr/common/data/kmfilemanager/KMFileManager.java(1 hunks)src/main/java/com/iemr/common/dto/grivance/GrievanceWorklistDTO.java(2 hunks)src/main/java/com/iemr/common/repository/kmfilemanager/KMFileManagerRepository.java(1 hunks)src/main/java/com/iemr/common/service/grievance/GrievanceDataSyncImpl.java(1 hunks)src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java(2 hunks)src/main/java/com/iemr/common/service/kmfilemanager/KMFileManagerServiceImpl.java(3 hunks)src/main/java/com/iemr/common/service/services/CommonServiceImpl.java(2 hunks)src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java(6 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: vishwab1
PR: PSMRI/Common-API#214
File: src/main/java/com/iemr/common/service/recaptcha/CaptchaValidationService.java:28-31
Timestamp: 2025-06-10T10:53:24.380Z
Learning: The user vishwab1 confirmed that their CAPTCHA implementation handles null token checking at the controller level before calling the service, which mitigates the direct security vulnerability of the service returning true for null tokens.
Learnt from: srishtigrp78
PR: PSMRI/Common-API#166
File: src/main/java/com/iemr/common/repository/grievance/GrievanceDataRepo.java:139-140
Timestamp: 2025-03-03T05:09:33.578Z
Learning: The query method `fetchGrievanceWorklistByComplaintID` in GrievanceDataRepo does not require filtering for deleted records (g.deleted = false) as per developer's preference.
Learnt from: helenKaryamsetty
PR: PSMRI/Common-API#145
File: src/main/java/com/iemr/common/service/abdmfacility/AbdmFacilityServiceImpl.java:16-20
Timestamp: 2024-12-18T08:53:22.725Z
Learning: In AbdmFacilityServiceImpl, no exceptions are thrown because the UI layer takes responsibility for handling all error scenarios and directly uses the raw response returned by the repository.
Learnt from: srishtigrp78
PR: PSMRI/Common-API#163
File: src/main/java/com/iemr/common/data/grievance/GrievanceDetails.java:111-118
Timestamp: 2025-02-21T09:49:46.528Z
Learning: The `complaintResolution` and `remarks` fields in GrievanceDetails class are intentionally kept without validation constraints (@NotBlank, @Size) as per the team's decision.
Learnt from: helenKaryamsetty
PR: PSMRI/Common-API#123
File: src/main/java/com/iemr/common/controller/abdmfacility/AbdmFacilityController.java:41-45
Timestamp: 2024-11-20T07:23:22.514Z
Learning: In the `Common-API` codebase, exception handling within controller classes like `AbdmFacilityController.java` is managed using the `OutputResponse` class, and it is not required to catch specific exceptions separately. General exception handling is sufficient in this context.
Learnt from: srishtigrp78
PR: PSMRI/Common-API#166
File: src/main/java/com/iemr/common/repository/grievance/GrievanceDataRepo.java:128-136
Timestamp: 2025-03-03T05:09:09.520Z
Learning: For the GrievanceDataRepo's fetchGrievanceDetailsBasedOnParams method, filtering out deleted records with "AND g.deleted = false" is not required as per the developer's decision.
src/main/java/com/iemr/common/dto/grivance/GrievanceWorklistDTO.java (3)
Learnt from: srishtigrp78
PR: PSMRI/Common-API#163
File: src/main/java/com/iemr/common/data/grievance/GrievanceDetails.java:111-118
Timestamp: 2025-02-21T09:49:46.528Z
Learning: The `complaintResolution` and `remarks` fields in GrievanceDetails class are intentionally kept without validation constraints (@NotBlank, @Size) as per the team's decision.
Learnt from: srishtigrp78
PR: PSMRI/Common-API#166
File: src/main/java/com/iemr/common/repository/grievance/GrievanceDataRepo.java:139-140
Timestamp: 2025-03-03T05:09:33.578Z
Learning: The query method `fetchGrievanceWorklistByComplaintID` in GrievanceDataRepo does not require filtering for deleted records (g.deleted = false) as per developer's preference.
Learnt from: srishtigrp78
PR: PSMRI/Common-API#166
File: src/main/java/com/iemr/common/repository/grievance/GrievanceDataRepo.java:128-136
Timestamp: 2025-03-03T05:09:09.520Z
Learning: For the GrievanceDataRepo's fetchGrievanceDetailsBasedOnParams method, filtering out deleted records with "AND g.deleted = false" is not required as per the developer's decision.
src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java (3)
Learnt from: srishtigrp78
PR: PSMRI/Common-API#166
File: src/main/java/com/iemr/common/repository/grievance/GrievanceDataRepo.java:139-140
Timestamp: 2025-03-03T05:09:33.578Z
Learning: The query method `fetchGrievanceWorklistByComplaintID` in GrievanceDataRepo does not require filtering for deleted records (g.deleted = false) as per developer's preference.
Learnt from: srishtigrp78
PR: PSMRI/Common-API#166
File: src/main/java/com/iemr/common/repository/grievance/GrievanceDataRepo.java:128-136
Timestamp: 2025-03-03T05:09:09.520Z
Learning: For the GrievanceDataRepo's fetchGrievanceDetailsBasedOnParams method, filtering out deleted records with "AND g.deleted = false" is not required as per the developer's decision.
Learnt from: srishtigrp78
PR: PSMRI/Common-API#163
File: src/main/java/com/iemr/common/data/grievance/GrievanceDetails.java:111-118
Timestamp: 2025-02-21T09:49:46.528Z
Learning: The `complaintResolution` and `remarks` fields in GrievanceDetails class are intentionally kept without validation constraints (@NotBlank, @Size) as per the team's decision.
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (3)
Learnt from: helenKaryamsetty
PR: PSMRI/Common-API#123
File: src/main/java/com/iemr/common/controller/abdmfacility/AbdmFacilityController.java:41-45
Timestamp: 2024-11-20T07:23:22.514Z
Learning: In the `Common-API` codebase, exception handling within controller classes like `AbdmFacilityController.java` is managed using the `OutputResponse` class, and it is not required to catch specific exceptions separately. General exception handling is sufficient in this context.
Learnt from: helenKaryamsetty
PR: PSMRI/Common-API#145
File: src/main/java/com/iemr/common/service/abdmfacility/AbdmFacilityServiceImpl.java:16-20
Timestamp: 2024-12-18T08:53:22.725Z
Learning: In AbdmFacilityServiceImpl, no exceptions are thrown because the UI layer takes responsibility for handling all error scenarios and directly uses the raw response returned by the repository.
Learnt from: indraniBan
PR: PSMRI/Common-API#138
File: src/main/java/com/iemr/common/utils/JwtAuthenticationUtil.java:0-0
Timestamp: 2025-02-03T12:42:38.278Z
Learning: In the JwtAuthenticationUtil class, catching a generic Exception in the validateUserIdAndJwtToken method is acceptable as per the team's preference.
src/main/java/com/iemr/common/service/grievance/GrievanceDataSyncImpl.java (4)
Learnt from: ravishanigarapu
PR: PSMRI/Common-API#175
File: src/main/java/com/iemr/common/service/grievance/GrievanceDataSyncImpl.java:541-637
Timestamp: 2025-03-20T07:16:43.865Z
Learning: In the PSMRI/Common-API grievance system, the scheduler runs only once per day, so concurrent thread execution is not a concern for methods like `completeGrievanceCall`.
Learnt from: srishtigrp78
PR: PSMRI/Common-API#166
File: src/main/java/com/iemr/common/repository/grievance/GrievanceDataRepo.java:128-136
Timestamp: 2025-03-03T05:09:09.520Z
Learning: For the GrievanceDataRepo's fetchGrievanceDetailsBasedOnParams method, filtering out deleted records with "AND g.deleted = false" is not required as per the developer's decision.
Learnt from: srishtigrp78
PR: PSMRI/Common-API#166
File: src/main/java/com/iemr/common/repository/grievance/GrievanceDataRepo.java:139-140
Timestamp: 2025-03-03T05:09:33.578Z
Learning: The query method `fetchGrievanceWorklistByComplaintID` in GrievanceDataRepo does not require filtering for deleted records (g.deleted = false) as per developer's preference.
Learnt from: srishtigrp78
PR: PSMRI/Common-API#163
File: src/main/java/com/iemr/common/data/grievance/GrievanceDetails.java:111-118
Timestamp: 2025-02-21T09:49:46.528Z
Learning: The `complaintResolution` and `remarks` fields in GrievanceDetails class are intentionally kept without validation constraints (@NotBlank, @Size) as per the team's decision.
src/main/java/com/iemr/common/service/services/CommonServiceImpl.java (1)
Learnt from: helenKaryamsetty
PR: PSMRI/Common-API#145
File: src/main/java/com/iemr/common/service/abdmfacility/AbdmFacilityServiceImpl.java:16-20
Timestamp: 2024-12-18T08:53:22.725Z
Learning: In AbdmFacilityServiceImpl, no exceptions are thrown because the UI layer takes responsibility for handling all error scenarios and directly uses the raw response returned by the repository.
src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java (1)
Learnt from: helenKaryamsetty
PR: PSMRI/Common-API#145
File: src/main/java/com/iemr/common/service/abdmfacility/AbdmFacilityServiceImpl.java:16-20
Timestamp: 2024-12-18T08:53:22.725Z
Learning: In AbdmFacilityServiceImpl, no exceptions are thrown because the UI layer takes responsibility for handling all error scenarios and directly uses the raw response returned by the repository.
🔇 Additional comments (13)
src/main/java/com/iemr/common/service/kmfilemanager/KMFileManagerServiceImpl.java (1)
137-173: Inconsistent summary: Code changes don't match PR objectives.The PR objectives describe authentication improvements to prevent user enumeration, but the actual code changes focus on file management enhancements for subcategories. This suggests either incorrect PR objectives or these files belong to a different changeset.
Likely an incorrect or invalid review comment.
src/main/java/com/iemr/common/repository/kmfilemanager/KMFileManagerRepository.java (1)
79-80: LGTM! Well-structured repository method.The new
getFilesBySubCategoryIDmethod correctly implements JPA query syntax and properly filters out deleted records. This enhancement supports the multi-file subcategory functionality.src/main/java/com/iemr/common/data/kmfilemanager/KMFileManager.java (1)
114-116: LGTM! Proper field persistence change.Changing
subCategoryIDfrom@Transientto@Column(name = "SubCategoryID")correctly enables database persistence for this field, which is necessary for the new repository query methodgetFilesBySubCategoryID.src/main/java/com/iemr/common/service/services/CommonServiceImpl.java (2)
60-60: LGTM! Necessary import addition.The import for
KMFileManageris required for the refactoredgetSubCategoriesmethod.
140-173: LGTM! Well-implemented enhancement for multi-file subcategory support.The refactored
getSubCategoriesmethod effectively enhances the functionality to support multiple files per subcategory. The implementation:
- Correctly uses the new repository method
getFilesBySubCategoryID- Properly handles the case where no files exist for a subcategory
- Sets representative file URL and name from the first file for backwards compatibility
- Maintains proper error handling and follows established patterns
This enhancement aligns well with the changes in other files and supports the overall file management improvement.
src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java (1)
271-293: Failed login attempt handling is correctly implemented.The logic properly increments failed attempts, locks the account when threshold is reached, and returns appropriate generic error messages to prevent user enumeration.
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (1)
623-626: Good implementation of enumeration prevention.The generic error message "If the username is registered, you will be asked a security question" correctly prevents attackers from determining whether a username exists in the system.
src/main/java/com/iemr/common/dto/grivance/GrievanceWorklistDTO.java (1)
43-43: LGTM! Clean DTO enhancement.The addition of the
beneficiaryConsentBoolean field follows proper Java conventions with correct field declaration, constructor parameter addition, and assignment implementation.Also applies to: 49-49, 75-75
src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java (2)
299-299: LGTM! Appropriate row length validation update.The minimum row length check correctly increased from 22 to 24 to accommodate the new
beneficiaryConsentfield.
338-338: LGTM! Correct beneficiaryConsent field assignment.The new
beneficiaryConsentfield is correctly populated fromrow[23], properly extending the DTO constructor call.src/main/java/com/iemr/common/service/grievance/GrievanceDataSyncImpl.java (3)
582-608: Excellent refactoring for improved maintainability.The introduction of explicit boolean flags (
isResolved,isValidGroup,isInvalidGroup) significantly improves code readability by replacing compound conditionals with clear, self-documenting logic. The four distinct case handlers make the business rules explicit and easier to maintain.
610-625: LGTM! Clear call counter logic.The refactored call counter update logic explicitly handles retry scenarios vs max attempts reached, making the flow much clearer than the previous implementation.
538-632: Flag inconsistency with PR objectives.The changes in this file focus on grievance call completion logic improvements, but the PR objectives specifically mention "User Authentication to Prevent Enumeration" with details about password validation, failed attempts, and account lockout. There appears to be a mismatch between the stated PR objectives and the actual code changes.
Likely an incorrect or invalid review comment.
| kmFileManager.setKmUploadStatus(KM_UPLOADSTATUS_COMPLETED); | ||
| kmFileManager.setFileUID(uuid); | ||
|
|
||
| kmFileManager.setSubCategoryID(kmFileManager.getSubCategoryID()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant assignment.
The line kmFileManager.setSubCategoryID(kmFileManager.getSubCategoryID()); is redundant as it sets a field to its own current value, serving no functional purpose.
- kmFileManager.setSubCategoryID(kmFileManager.getSubCategoryID());📝 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.
| kmFileManager.setSubCategoryID(kmFileManager.getSubCategoryID()); |
🤖 Prompt for AI Agents
In
src/main/java/com/iemr/common/service/kmfilemanager/KMFileManagerServiceImpl.java
at line 180, remove the redundant assignment where kmFileManager sets its
SubCategoryID to its current value, as this line has no functional effect and
can be safely deleted.
| if (users.get(0).getDeleted()) | ||
| throw new IEMRException("Your account is locked or de-activated. Please contact administrator"); | ||
| else if (users.get(0).getStatusID() > 2) | ||
| throw new IEMRException("Your account is not active. Please contact administrator"); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply the same refactoring to superUserAuthenticate method.
The superUserAuthenticate method has the same code duplication issue. Use the suggested validateAccountStatus method here as well to maintain consistency.
Also applies to: 362-366, 393-397
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java
around lines 344-348, 362-366, and 393-397, the superUserAuthenticate method
duplicates account status validation code. Refactor these sections to replace
the duplicated logic with calls to the existing validateAccountStatus method to
ensure consistency and reduce code duplication.
| if (users.get(0).getDeleted()) | ||
| throw new IEMRException("Your account is locked or de-activated. Please contact administrator"); | ||
| else if (users.get(0).getStatusID() > 2) | ||
| throw new IEMRException("Your account is not active. Please contact administrator"); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate account status checks to reduce code duplication.
The account status validation logic is repeated identically in all password validation branches. This violates the DRY principle and increases maintenance burden.
Extract the account status checks into a private method:
+private void validateAccountStatus(User user) throws IEMRException {
+ if (user.getDeleted())
+ throw new IEMRException("Your account is locked or de-activated. Please contact administrator");
+ else if (user.getStatusID() > 2)
+ throw new IEMRException("Your account is not active. Please contact administrator");
+}Then replace all duplicate checks with a single method call:
if (validatePassword == 1) {
- if (users.get(0).getDeleted())
- throw new IEMRException("Your account is locked or de-activated. Please contact administrator");
- else if (users.get(0).getStatusID() > 2)
- throw new IEMRException("Your account is not active. Please contact administrator");
+ validateAccountStatus(user);Also applies to: 257-261, 265-269, 295-299
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java
around lines 239-243, 257-261, 265-269, and 295-299, the account status
validation logic is duplicated across multiple password validation branches. To
fix this, extract the repeated account status checks into a private method that
accepts the user object and throws the appropriate exceptions. Then replace all
the duplicated checks in those line ranges with calls to this new method to
adhere to the DRY principle and simplify maintenance.
| } catch (Exception e) { | ||
| logger.error("userAuthenticate failed with error " + e.getMessage(), e); | ||
| response.setError(5000, "Authentication failed. Please try again later."); // Generic fallback | ||
| response.setError(e); |
There was a problem hiding this comment.
Security concern: Direct exception propagation may leak sensitive information.
Propagating raw exceptions directly to the response could expose internal system details, stack traces, or database information that attackers could exploit. This contradicts the PR's objective of preventing information leakage.
Consider maintaining generic error messages for client responses while logging detailed exceptions server-side:
} catch (Exception e) {
logger.error("userAuthenticate failed with error " + e.getMessage(), e);
- response.setError(e);
+ response.setError(5000, "Authentication failed. Please try again.");
}Also applies to: 648-648, 737-737, 755-755, 1061-1061, 1079-1079, 1163-1163
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/controller/users/IEMRAdminController.java at
line 522 and similarly at lines 648, 737, 755, 1061, 1079, and 1163, avoid
setting raw exceptions directly in the response to prevent leaking sensitive
internal details. Instead, log the full exception details on the server side for
debugging, and set a generic, non-sensitive error message in the response object
to safely inform the client of an error without exposing system internals.
| (Timestamp) row[13], // lastCall | ||
| (Boolean) row[23] //beneficiaryConsent |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify lastCall field assignment.
The lastCall field is being assigned row[13] which is the same as lastModDate. This seems incorrect - lastCall should likely be assigned a separate timestamp field from the database query result.
🏁 Script executed:
#!/bin/bash
# Description: Check the database query or stored procedure that populates the worklistData to understand the correct field mapping
# Expected: Find the query definition to verify what row[13] actually represents and if there's a separate lastCall field
# Search for the stored procedure or query definition that provides worklistData
rg -A 10 -B 5 "getGrievanceWorklistData|grievance.*worklist.*data" --type java
# Look for any SQL queries or native queries that might show the field order
ast-grep --pattern 'nativeQuery = $_'Length of output: 3892
To verify the field ordering, let’s pull in the full mapping block around those row[...] assignments:
🏁 Script executed:
#!/bin/bash
# Extract the array-to-DTO mapping in GrievanceHandlingServiceImpl
sed -n '300,380p' src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.javaLength of output: 4023
Fix incorrect lastCall mapping in GrievanceWorklistDTO constructor
The lastCall field is currently pulling from row[13] (which is already used for lastModDate), so it’s wrong. You’ll need to update it to the correct index from your stored-procedure result set and shift the beneficiaryConsent index accordingly.
• File: src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java
• Around lines 337–338 in the new GrievanceWorklistDTO(…) mapping
Suggested diff (adjust indices to match your procedure’s column order):
- (Timestamp) row[13], // lastCall
- (Boolean) row[23] // beneficiaryConsent
+ (Timestamp) row[23], // lastCall
+ (Boolean) row[24] // beneficiaryConsentPlease verify the actual column positions in Pr_Grievanceworklist and update the indices accordingly to prevent data mix-ups.
📝 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.
| (Timestamp) row[13], // lastCall | |
| (Boolean) row[23] //beneficiaryConsent | |
| (Timestamp) row[23], // lastCall | |
| (Boolean) row[24] // beneficiaryConsent |
🤖 Prompt for AI Agents
In
src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java
around lines 337 to 338, the lastCall field is incorrectly mapped from row[13],
which is already used for lastModDate. Review the stored procedure
Pr_Grievanceworklist to identify the correct column index for lastCall, update
the mapping to use that correct index, and adjust the beneficiaryConsent index
accordingly to maintain correct data alignment.
📋 Description
JIRA ID: AMM-1249
Please provide a summary of the change and the motivation behind it. Include relevant context and details.
✅ Type of Change
ℹ️ Additional Information
Key Changes:
validatePassword == 0to:failedAttempton each incorrect password.setDeleted(true)) if the failed attempts reach the configured threshold.Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Other Changes