implement time-bound account lock with auto-unlock after 24h#394
Open
neha222222 wants to merge 2 commits intoPSMRI:mainfrom
Open
implement time-bound account lock with auto-unlock after 24h#394neha222222 wants to merge 2 commits intoPSMRI:mainfrom
neha222222 wants to merge 2 commits intoPSMRI:mainfrom
Conversation
- added lock_timestamp column to m_user table - on 5th failed login, account gets locked with timestamp instead of permanent block - on next login attempt after 24h, account auto-unlocks (resets deleted flag, failed attempts, and lock timestamp) - updated error message: "Your account has been locked. You can try tomorrow or connect to the administrator." - failed attempt counter resets on successful login fixes PSMRI/AMRIT#118
Contributor
📝 WalkthroughWalkthroughA time-bound account locking mechanism was added: a persisted Changes
Sequence DiagramsequenceDiagram
participant User
participant AuthService as Auth Service
participant UserDB as User DB
participant Clock as System Clock
User->>AuthService: Login (username + password)
AuthService->>UserDB: Fetch user
UserDB-->>AuthService: User record (may include lockTimestamp)
alt user locked (lockTimestamp present)
AuthService->>Clock: Get current time
Clock-->>AuthService: Current timestamp
alt current >= lockTimestamp + 24h
AuthService->>AuthService: Auto-unlock (clear deleted/failedAttempt/lockTimestamp)
AuthService->>UserDB: Save unlocked user
UserDB-->>AuthService: Saved
AuthService->>AuthService: Continue authentication
else
AuthService-->>User: "Account locked...try tomorrow"
end
end
alt password invalid
AuthService->>AuthService: Increment failedAttempt
alt failedAttempt >= threshold
AuthService->>Clock: Get current time
Clock-->>AuthService: Current timestamp
AuthService->>AuthService: Set lockTimestamp
AuthService->>UserDB: Save locked user
UserDB-->>AuthService: Saved
AuthService-->>User: "Account locked...try tomorrow"
else
AuthService->>UserDB: Save updated failedAttempt
UserDB-->>AuthService: Saved
AuthService-->>User: "Invalid username or password"
end
else password valid
AuthService->>AuthService: Reset failedAttempt and clear lockTimestamp
AuthService->>UserDB: Save user
UserDB-->>AuthService: Saved
AuthService-->>User: Success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
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)
287-297:⚠️ Potential issue | 🔴 CriticalDon’t rewrite
lockTimestampfor users who are already deleted.
userAuthenticate()loads deleted rows viafindByUserNameNew(), but this branch runs before any deleted/lock guard. If a locked user submits a wrong password, Line 296 overwrites the original lock time and effectively extends the lock window on every attempt. The same path can also stamp alockTimestamponto a manually deactivated account, making it auto-unlockable later.🔒 Minimal guard to stop mutating already-deleted accounts
} else if (validatePassword == 0) { + if (Boolean.TRUE.equals(user.getDeleted())) { + checkUserAccountStatus(user); + } if (user.getFailedAttempt() + 1 < failedAttempt) { user.setFailedAttempt(user.getFailedAttempt() + 1); user = iEMRUserRepositoryCustom.save(user); logger.warn("User Password Wrong"); throw new IEMRException("Invalid username or password");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java` around lines 287 - 297, The code in userAuthenticate() increments failed attempts and unconditionally sets user.setDeleted(true) and user.setLockTimestamp(...) when failedAttempt threshold is reached, which overwrites existing lockTimestamp or stamps deactivated accounts; guard this by checking user.isDeleted() (or user.getDeleted()) and only setLockTimestamp and setDeleted when the user is not already deleted/locked. Specifically, in the branch handling validatePassword == 0 where you call user.setFailedAttempt(...), add a conditional around the block that sets user.setDeleted(true) and user.setLockTimestamp(...) (and saves) so those calls only run if user.getDeleted() == false (and/or existing lockTimestamp is null), leaving existing deleted/locked users' timestamps untouched; keep updating failedAttempt for audit but avoid mutating lockTimestamp/deleted for already-deleted users.
342-390:⚠️ Potential issue | 🟠 MajorLocked superusers can’t auto-unlock with this repository call.
superUserAuthenticate()still usesfindByUserName(), and that query filters ondeleted=false. A temporarily locked superuser is therefore never loaded,checkUserAccountStatus()never runs, and the flow falls out as “Invalid username or password” instead of auto-unlocking after the lock window. Cross-file evidence:src/main/java/com/iemr/common/repository/users/IEMRUserRepositoryCustom.java:39-46.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java` around lines 342 - 390, superUserAuthenticate loads users via iEMRUserRepositoryCustom.findByUserName which filters out deleted=true, so temporarily locked (deleted=true) superusers are never fetched and cannot be auto-unlocked by checkUserAccountStatus; update the data access to fetch the user regardless of deleted flag (either add/use a new repository method like findByUserNameIncludeDeleted or change the query behind findByUserName to not filter deleted for admin flow), then call checkUserAccountStatus(user) as before so locked accounts within the unlock window are un-locked and processed normally; ensure any save() calls still set deleted/lockTimestamp appropriately and that other code paths that rely on filtered queries are not broken.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java`:
- Around line 227-241: The current check in IEMRAdminUserServiceImpl (using
user.getLockTimestamp(), lockTime, now, twentyFourHoursMs) enforces a rolling
24-hour lock; replace it with a calendar-day-based check: convert
user.getLockTimestamp() to a LocalDate (using the application's ZoneId) and
compare it to LocalDate.now(); if the lock date is before today then auto-unlock
(reset deleted, failedAttempt, lockTimestamp and call
iEMRUserRepositoryCustom.save and log), otherwise throw the IEMRException. This
ensures any lock that occurred on a prior calendar day is unlocked after
midnight rather than waiting a full 24 hours.
---
Outside diff comments:
In `@src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java`:
- Around line 287-297: The code in userAuthenticate() increments failed attempts
and unconditionally sets user.setDeleted(true) and user.setLockTimestamp(...)
when failedAttempt threshold is reached, which overwrites existing lockTimestamp
or stamps deactivated accounts; guard this by checking user.isDeleted() (or
user.getDeleted()) and only setLockTimestamp and setDeleted when the user is not
already deleted/locked. Specifically, in the branch handling validatePassword ==
0 where you call user.setFailedAttempt(...), add a conditional around the block
that sets user.setDeleted(true) and user.setLockTimestamp(...) (and saves) so
those calls only run if user.getDeleted() == false (and/or existing
lockTimestamp is null), leaving existing deleted/locked users' timestamps
untouched; keep updating failedAttempt for audit but avoid mutating
lockTimestamp/deleted for already-deleted users.
- Around line 342-390: superUserAuthenticate loads users via
iEMRUserRepositoryCustom.findByUserName which filters out deleted=true, so
temporarily locked (deleted=true) superusers are never fetched and cannot be
auto-unlocked by checkUserAccountStatus; update the data access to fetch the
user regardless of deleted flag (either add/use a new repository method like
findByUserNameIncludeDeleted or change the query behind findByUserName to not
filter deleted for admin flow), then call checkUserAccountStatus(user) as before
so locked accounts within the unlock window are un-locked and processed
normally; ensure any save() calls still set deleted/lockTimestamp appropriately
and that other code paths that rely on filtered queries are not broken.
🪄 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: b771db1e-54c8-450e-8b8b-cb66fdb62e6e
📒 Files selected for processing (3)
src/main/java/com/iemr/common/data/users/User.javasrc/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.javasrc/main/resources/db/migration/V1__add_lock_timestamp_to_m_user.sql
src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java
Outdated
Show resolved
Hide resolved
…er query - use LocalDate comparison instead of rolling 24h so accounts unlock at midnight as the error message promises - guard against overwriting lockTimestamp when an already-locked user enters wrong password again - switch superUserAuthenticate to findByUserNameNew so temporarily locked superusers can still be loaded and auto-unlocked - clear lockTimestamp on successful superuser login too
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java`:
- Around line 319-320: In userAuthenticate() and superUserAuthenticate(), the
successful-password branches (where SecurePassword.validatePassword(...) == 1,
== 2, or == 3) must clear the account failure state; currently failedAttempt and
lockTimestamp are only reset in the final else. Update each success branch to
set user.setFailedAttempt(0) and user.setLockTimestamp(null) and persist the
change via iEMRUserRepositoryCustom.save(user) (same persistence call used
elsewhere) so a previously-failed user who now authenticates successfully does
not retain stale failure counts that could re-lock them on a subsequent failure.
- Around line 230-238: The auto-unlock block in IEMRAdminUserServiceImpl
currently clears deleted/failedAttempt/lockTimestamp then returns, which
bypasses the subsequent inactive-account status check; remove the premature
return after the iEMRUserRepositoryCustom.save(user) and logger.info(...) so the
method continues into the existing status check logic (so deactivated accounts
with statusID > 2 are handled properly) while keeping the auto-unlock field
resets and save call intact.
🪄 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: b1effd2f-1a3c-413d-99da-9e6f598367e1
📒 Files selected for processing (1)
src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java
Comment on lines
+230
to
+238
| if (today.isAfter(lockDate)) { | ||
| // auto-unlock: it's a new day after the lock date | ||
| user.setDeleted(false); | ||
| user.setFailedAttempt(0); | ||
| user.setLockTimestamp(null); | ||
| iEMRUserRepositoryCustom.save(user); | ||
| logger.info("Account auto-unlocked for user {} (locked on {}, unlocked on {})", | ||
| user.getUserName(), lockDate, today); | ||
| return; |
Contributor
There was a problem hiding this comment.
Don't auto-unlock past the inactive-status guard.
If an account is deactivated (statusID > 2) while it still has a historical lockTimestamp, this branch clears deleted and returns before Line 246 runs. The rest of the login flow then proceeds with an inactive account. Let the method continue into the status check after clearing the temporary lock, instead of returning here.
Suggested fix
if (user.getDeleted()) {
if (user.getLockTimestamp() != null) {
java.time.LocalDate lockDate = user.getLockTimestamp().toLocalDateTime().toLocalDate();
java.time.LocalDate today = java.time.LocalDate.now();
if (today.isAfter(lockDate)) {
user.setDeleted(false);
user.setFailedAttempt(0);
user.setLockTimestamp(null);
iEMRUserRepositoryCustom.save(user);
logger.info("Account auto-unlocked for user {} (locked on {}, unlocked on {})",
user.getUserName(), lockDate, today);
- return;
} else {
throw new IEMRException(
"Your account has been locked. You can try tomorrow or connect to the administrator.");
}
}
- throw new IEMRException(
- "Your account is locked or de-activated. Please contact administrator");
- } else if (user.getStatusID() > 2) {
+ if (user.getDeleted()) {
+ throw new IEMRException(
+ "Your account is locked or de-activated. Please contact administrator");
+ }
+ }
+ if (user.getStatusID() > 2) {
throw new IEMRException("Your account is not active. Please contact administrator");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java`
around lines 230 - 238, The auto-unlock block in IEMRAdminUserServiceImpl
currently clears deleted/failedAttempt/lockTimestamp then returns, which
bypasses the subsequent inactive-account status check; remove the premature
return after the iEMRUserRepositoryCustom.save(user) and logger.info(...) so the
method continues into the existing status check logic (so deactivated accounts
with statusID > 2 are handled properly) while keeping the auto-unlock field
resets and save call intact.
Comment on lines
+319
to
320
| user.setLockTimestamp(null); | ||
| user = iEMRUserRepositoryCustom.save(user); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== SecurePassword.validatePassword contract =="
fd 'SecurePassword.java' src --exec sed -n '1,260p' {}
echo
echo "== validatePassword call sites in IEMRAdminUserServiceImpl =="
rg -n -C3 '\bvalidatePassword\s*\(' --type=java src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.javaRepository: PSMRI/Common-API
Length of output: 6138
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full code around first reset (lines 250-330) =="
sed -n '250,330p' src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java
echo
echo "== Full code around second reset (lines 340-420) =="
sed -n '340,420p' src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.javaRepository: PSMRI/Common-API
Length of output: 6292
Reset the failure state in every successful-auth branch.
failedAttempt and lockTimestamp are only cleared in the final else block of each method. Since SecurePassword.validatePassword(...) returns 1, 2, or 3 for successful password matches—and the code handles each distinctly—users who log in successfully via any of these paths retain stale failure counts. A user with prior failures can authenticate successfully but still be re-locked on the next incorrect password attempt.
Move the reset into all three success branches (validatePassword == 1, 2, 3) in both userAuthenticate() and superUserAuthenticate().
This applies to both methods around the locations indicated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java`
around lines 319 - 320, In userAuthenticate() and superUserAuthenticate(), the
successful-password branches (where SecurePassword.validatePassword(...) == 1,
== 2, or == 3) must clear the account failure state; currently failedAttempt and
lockTimestamp are only reset in the final else. Update each success branch to
set user.setFailedAttempt(0) and user.setLockTimestamp(null) and persist the
change via iEMRUserRepositoryCustom.save(user) (same persistence call used
elsewhere) so a previously-failed user who now authenticates successfully does
not retain stale failure counts that could re-lock them on a subsequent failure.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes PSMRI/AMRIT#118
📋 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