fix: implement 24-hour auto-unlock for account lockout after failed l…#408
fix: implement 24-hour auto-unlock for account lockout after failed l…#408Aarti-panchal01 wants to merge 2 commits into
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces time-bound account lockout with automatic unlock after 24 hours. The ChangesAccount Lockout and Auto-Unlock Feature
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
Hi @drtechie, I've implemented the 24-hour auto-unlock mechanism for the account lockout issue referenced in PSMRI/AMRIT#118. I'm applying for C4GT DMP 2026 with Piramal Swasthya — specifically interested in the MMU-UI migration (#130), Helpline104-UI (#129), and FLW Gamification (#132) projects. This PR demonstrates my interest in contributing to AMRIT. Happy to make any changes based on your review feedback. Thank you! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java (2)
223-229:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate error message per requirements.
The PR objectives specify updating the user-facing error message to: "Your account has been locked. You can try tomorrow or connect to the administrator."
However, the current error message at line 225 remains:
"Your account is locked or de-activated. Please contact administrator"This message should be updated to inform users about the time-bound nature of the lockout and provide clearer guidance.
📝 Proposed fix for error message
private void checkUserAccountStatus(User user) throws IEMRException { if (user.getDeleted()) { - throw new IEMRException("Your account is locked or de-activated. Please contact administrator"); + throw new IEMRException("Your account has been locked. You can try tomorrow or connect to the administrator."); } else if (user.getStatusID() > 2) { throw new IEMRException("Your account is not active. Please contact administrator"); } }🤖 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/common/service/users/IEMRAdminUserServiceImpl.java` around lines 223 - 229, In checkUserAccountStatus(User user) update the thrown IEMRException message for the deleted/account-locked case: replace the current message thrown in the if (user.getDeleted()) branch with "Your account has been locked. You can try tomorrow or connect to the administrator." Leave the other branch (user.getStatusID() > 2) unchanged.
232-302:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftInconsistent and incomplete lockout logic across authentication methods creates security vulnerability.
The three authentication methods have conflicting lockout implementations:
userAuthenticate(lines 232-302): Uses old inline lockout logic without 24-hour auto-unlocksuperUserAuthenticate(lines 357-409): Uses refactored helper methods (checkUserLoginFailedAttempt,resetUserLoginFailedAttempt,updateUserLoginFailedAttempt) with 24-hour auto-unlockuserAuthenticateByEncryption(lines 1090-1108): Has no lockout logic at allSince all three methods are actively used in production (verified in IEMRAdminController), this creates:
- Account lockout bypass: Attackers can authenticate via
userAuthenticateByEncryptioneven if the account is locked via another method- Inconsistent security behavior: Only super users get auto-unlock protection
- Maintenance risk: Lockout logic duplicated across methods without synchronization
Update
userAuthenticateanduserAuthenticateByEncryptionto use the same helper methods assuperUserAuthenticateto ensure consistent lockout behavior across all authentication paths.🤖 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/common/service/users/IEMRAdminUserServiceImpl.java` around lines 232 - 302, The userAuthenticate and userAuthenticateByEncryption methods must be refactored to use the same lockout helpers as superUserAuthenticate to remove duplicated/inconsistent inline logic: replace the inline failed-attempt handling in userAuthenticate (the validatePassword == 0 branch and the surrounding failedAttempt calculations) with calls to checkUserLoginFailedAttempt(user), updateUserLoginFailedAttempt(user) and resetUserLoginFailedAttempt(user) as appropriate around successful/failed validation, and do the same for userAuthenticateByEncryption; retain checkUserAccountStatus(user) where present, remove the ad-hoc manipulation of user.setFailedAttempt()/user.setDeleted() and ConfigProperties references in these two methods, and ensure after successful authentication you call resetUserLoginFailedAttempt(user) and persist via iEMRUserRepositoryCustom.save(user) as superUserAuthenticate does.
🤖 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/common/service/users/IEMRAdminUserServiceImpl.java`:
- Around line 304-320: Add a manual admin unlock: implement a new service method
adminUnlockAccount(Long userId) in IEMRAdminUserServiceImpl that looks up the
user via iEMRUserRepositoryCustom.findUserByUserID(userId), returns/throws if
not found, sets user.setDeleted(false), user.setFailedAttempt(0),
user.setLockedAt(null), saves with iEMRUserRepositoryCustom.save(user) and logs
via logger.info("Admin unlocked user account. User ID: {}", userId);
declare/throw IEMRException as needed. Expose a controller endpoint in
IEMRAdminController (e.g., `@PostMapping`("/unlockUserAccount")) which accepts a
request containing userID, validates input, calls
iemrAdminUserServiceImpl.adminUnlockAccount(userId), handles exceptions and
returns an appropriate success/failure response. Ensure null checks, proper
exception handling, and permission (admin) guard where applicable.
---
Outside diff comments:
In `@src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java`:
- Around line 223-229: In checkUserAccountStatus(User user) update the thrown
IEMRException message for the deleted/account-locked case: replace the current
message thrown in the if (user.getDeleted()) branch with "Your account has been
locked. You can try tomorrow or connect to the administrator." Leave the other
branch (user.getStatusID() > 2) unchanged.
- Around line 232-302: The userAuthenticate and userAuthenticateByEncryption
methods must be refactored to use the same lockout helpers as
superUserAuthenticate to remove duplicated/inconsistent inline logic: replace
the inline failed-attempt handling in userAuthenticate (the validatePassword ==
0 branch and the surrounding failedAttempt calculations) with calls to
checkUserLoginFailedAttempt(user), updateUserLoginFailedAttempt(user) and
resetUserLoginFailedAttempt(user) as appropriate around successful/failed
validation, and do the same for userAuthenticateByEncryption; retain
checkUserAccountStatus(user) where present, remove the ad-hoc manipulation of
user.setFailedAttempt()/user.setDeleted() and ConfigProperties references in
these two methods, and ensure after successful authentication you call
resetUserLoginFailedAttempt(user) and persist via
iEMRUserRepositoryCustom.save(user) as superUserAuthenticate does.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6affa3ed-d9fa-4489-aab3-61db4380f5a3
📒 Files selected for processing (2)
src/main/java/com/iemr/common/data/users/User.javasrc/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java
| private void checkUserLoginFailedAttempt(User user) throws IEMRException { | ||
|
|
||
| if (user.getDeleted() != null && user.getDeleted() && user.getLockedAt() != null) { | ||
| long lockedTime = user.getLockedAt().getTime(); | ||
| long currentTime = System.currentTimeMillis(); | ||
| long diffInMillis = currentTime - lockedTime; | ||
| long diffInHours = diffInMillis / (1000 * 60 * 60); | ||
|
|
||
| // If 24 hours have passed, auto-unlock the account | ||
| if (diffInHours >= 24) { | ||
| user.setDeleted(false); | ||
| user.setFailedAttempt(0); | ||
| user.setLockedAt(null); | ||
| iEMRUserRepositoryCustom.save(user); | ||
| logger.info("User account auto-unlocked after 24 hours. User ID: {}", user.getUserID()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for admin unlock or account unlock endpoints
echo "=== Searching for unlock-related endpoints ==="
rg -n -i 'unlock|unlockaccount' --type=java -C3
echo -e "\n=== Searching for admin user management endpoints ==="
rg -n -i '@PostMapping.*admin.*user|@PutMapping.*admin.*user' --type=java -C3
echo -e "\n=== Checking if deleted field is updated via admin endpoints ==="
rg -n 'setDeleted\(false\)' --type=java -C3Repository: PSMRI/Common-API
Length of output: 27199
🏁 Script executed:
# Search for admin controller files
echo "=== Looking for admin controller files ==="
fd -type f -name "*Admin*Controller*" src/
echo -e "\n=== Searching for updateUser or similar admin endpoints ==="
rg -n 'public.*User.*update|@PutMapping|@PostMapping' src/main/java/com/iemr/common/controller/users/ -A3 --type=java
echo -e "\n=== Checking for methods that set deleted=false for users (admin context) ==="
rg -n 'public.*setDeleted|unlockAccount|unlockUser' src/main/java/com/iemr/common/service/users/ --type=java -A5
echo -e "\n=== Search for any endpoint mapping patterns for user unlock/update ==="
rg -n '@RequestMapping|@GetMapping|@PostMapping|@PutMapping|@DeleteMapping' src/main/java/com/iemr/common/controller/users/IEMRAdminController.java --type=java | head -30Repository: PSMRI/Common-API
Length of output: 4397
🏁 Script executed:
# Find the actual admin controller file
echo "=== Finding admin controller file ==="
find src -name "*IEMRAdminController*" -type f
echo -e "\n=== Check the service methods available in IEMRAdminUserServiceImpl ==="
rg -n 'public.*User|public.*save|public.*update' src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java --type=java | head -40
echo -e "\n=== Check if there's an admin update user method ==="
cat src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java | head -100
echo -e "\n=== List all public methods in IEMRAdminUserServiceImpl ==="
rg -n '^[[:space:]]*public' src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java --type=javaRepository: PSMRI/Common-API
Length of output: 11309
🏁 Script executed:
# Get all endpoints in IEMRAdminController
echo "=== All RequestMapping methods in IEMRAdminController ==="
rg -n '@RequestMapping|@PostMapping|@PutMapping|@GetMapping|@DeleteMapping' src/main/java/com/iemr/common/controller/users/IEMRAdminController.java --type=java -A2 | head -80
echo -e "\n=== Search for any user update/save/modify endpoint in the controller ==="
rg -n 'updateUser|saveUser|modifyUser|unlockAccount|adminUnlock' src/main/java/com/iemr/common/controller/users/IEMRAdminController.java --type=java -C3
echo -e "\n=== Check entire controller file size ==="
wc -l src/main/java/com/iemr/common/controller/users/IEMRAdminController.javaRepository: PSMRI/Common-API
Length of output: 7196
🏁 Script executed:
# Check the rest of the controller file for update endpoints
echo "=== Search in entire controller for any endpoint that might update user ==="
rg -n 'method = RequestMethod.PUT|method = RequestMethod.PATCH|@PutMapping|@PatchMapping' src/main/java/com/iemr/common/controller/users/IEMRAdminController.java --type=java
echo -e "\n=== Check all RequestMapping values (full list) ==="
rg -n '@RequestMapping\(value = "' src/main/java/com/iemr/common/controller/users/IEMRAdminController.java --type=java | tail -20
echo -e "\n=== Check if there's any user admin API/endpoint file ==="
find src -name "*AdminAPI*" -o -name "*UserAdmin*" -o -name "*AdminUser*" | grep -i controller
echo -e "\n=== Final comprehensive search for admin-related endpoints ==="
rg -rn 'class.*Admin.*Controller' src/main/java/com/iemr/common/controller/ --type=javaRepository: PSMRI/Common-API
Length of output: 3272
🏁 Script executed:
# One final comprehensive check for any user update capability
echo "=== Search all Java files for any unlock or admin user update methods ==="
rg -n 'adminUnlock|manualUnlock|unlockUser|unlockAccount' src/main/java --type=java
echo -e "\n=== Check if User model has any unlock-related methods ==="
rg -n 'public.*unlock|setDeleted.*admin|unlock.*method' src/main/java/com/iemr/common/data/users/User.java --type=java
echo -e "\n=== Verify the User.java has `@Expose` annotation on lockedAt field ==="
rg -n '@Expose.*lockedAt|lockedAt.*@Expose|private.*lockedAt' src/main/java/com/iemr/common/data/users/User.java --type=java -B2 -A2Repository: PSMRI/Common-API
Length of output: 362
Add endpoint for manual admin account unlock.
The PR exposes account lock status via the @Expose annotation on the lockedAt field in User.java, fulfilling one requirement. However, no admin endpoint exists for manually unlocking accounts—only automatic unlock after 24 hours is implemented in checkUserLoginFailedAttempt().
Add a service method and controller endpoint to allow admins to manually unlock accounts:
Example implementation
// In IEMRAdminUserServiceImpl
public void adminUnlockAccount(Long userId) throws IEMRException {
User user = iEMRUserRepositoryCustom.findUserByUserID(userId);
if (user != null) {
user.setDeleted(false);
user.setFailedAttempt(0);
user.setLockedAt(null);
iEMRUserRepositoryCustom.save(user);
logger.info("Admin unlocked user account. User ID: {}", userId);
}
}
// In IEMRAdminController
`@PostMapping`("/unlockUserAccount")
public String unlockUserAccount(`@RequestBody` Map<String, Long> request) {
Long userId = request.get("userID");
iemrAdminUserServiceImpl.adminUnlockAccount(userId);
// Return response
}🤖 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/common/service/users/IEMRAdminUserServiceImpl.java`
around lines 304 - 320, Add a manual admin unlock: implement a new service
method adminUnlockAccount(Long userId) in IEMRAdminUserServiceImpl that looks up
the user via iEMRUserRepositoryCustom.findUserByUserID(userId), returns/throws
if not found, sets user.setDeleted(false), user.setFailedAttempt(0),
user.setLockedAt(null), saves with iEMRUserRepositoryCustom.save(user) and logs
via logger.info("Admin unlocked user account. User ID: {}", userId);
declare/throw IEMRException as needed. Expose a controller endpoint in
IEMRAdminController (e.g., `@PostMapping`("/unlockUserAccount")) which accepts a
request containing userID, validates input, calls
iemrAdminUserServiceImpl.adminUnlockAccount(userId), handles exceptions and
returns an appropriate success/failure response. Ensure null checks, proper
exception handling, and permission (admin) guard where applicable.
|
|
Hi @vishwab1, @vanitha1822 could you please review this PR and let me know if it looks good or if any changes are needed? |
1 participant
📋 Description
Fixes: PSMRI/AMRIT#118
Currently, when a user enters an incorrect password 5 times, the account is permanently blocked with no automatic recovery mechanism. This forces manual admin intervention for every lockout.
This PR implements a time-bound 24-hour account lockout mechanism as described in the issue.
✅ Type of Change
🔧 Changes Made
User.java
lockedAtTimestamp field to track when account was lockedIEMRAdminUserServiceImpl.java
checkUserLoginFailedAttempt(): checks if 24 hours elapsed since lock, auto-unlocks if yesupdateUserLoginFailedAttempt(): recordslockedAttimestamp when 5th failed attempt occursresetUserLoginFailedAttempt(): clearslockedAton successful loginsuperUserAuthenticate(): integrated auto-unlock check before each login attemptℹ️ Additional Information
How it works:
lockedAttimestamp recordedlockedAtNote: A DB migration adding the
locked_atcolumn to the users table will be required before deployment.Summary by CodeRabbit