Add failed login attempt tracking and basic lock after 5 attempts#366
Open
adititiwari0810 wants to merge 1 commit intoPSMRI:mainfrom
Open
Add failed login attempt tracking and basic lock after 5 attempts#366adititiwari0810 wants to merge 1 commit intoPSMRI:mainfrom
adititiwari0810 wants to merge 1 commit intoPSMRI:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThe changes introduce account locking mechanisms to the authentication service by tracking failed login attempts and locking accounts after threshold breach. A new method retrieves users by ID, ORM mappings are updated, and authentication flows are enhanced with role mapping population and failed attempt management. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant AuthService as IEMRAdminUserServiceImpl
participant UserRepo as iEMRUserRepository
participant DB as Database
Client->>AuthService: userAuthenticateV1(userId, password)
AuthService->>UserRepo: findByUserID(userId)
UserRepo->>DB: Query User
DB-->>UserRepo: User record
UserRepo-->>AuthService: User object
alt User locked (failedAttempt >= 5)
AuthService-->>Client: IEMRException (Account locked)
else User not locked
alt Password valid
AuthService->>DB: Reset failedAttempt to 0
AuthService->>UserRepo: Save updated User
UserRepo->>DB: Persist changes
DB-->>UserRepo: Success
UserRepo-->>AuthService: Confirm save
AuthService->>AuthService: Populate loginResponseModel<br/>with service roles
AuthService-->>Client: Login success + roles
else Password invalid
AuthService->>AuthService: Increment failedAttempt
AuthService->>UserRepo: Save updated User
UserRepo->>DB: Persist failedAttempt
DB-->>UserRepo: Success
UserRepo-->>AuthService: Confirm save
AuthService-->>Client: IEMRException (Invalid credentials)
end
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java (1)
1238-1252:⚠️ Potential issue | 🟠 Major
getUserByIdcurrently masks “not found” as a generic fetch error.The
IEMRExceptionthrown for missing user is caught bycatch (Exception)and wrapped, losing semantic error handling.🛠️ Suggested fix
public User getUserById(Long userId) throws IEMRException { try { // Fetch user from custom repository by userId User user = iEMRUserRepositoryCustom.findByUserID(userId); // Check if user is found if (user == null) { throw new IEMRException("User not found with ID: " + userId); } return user; + } catch (IEMRException e) { + throw e; } catch (Exception e) { // Log and throw custom exception in case of errors logger.error("Error fetching user with ID: " + userId, e); throw new IEMRException("Error fetching user with ID: " + userId, e); } }🤖 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 1238 - 1252, The getUserById method in IEMRAdminUserServiceImpl masks the "user not found" IEMRException because the catch(Exception) block wraps everything; fix by doing the null check and throwing IEMRException before the try-catch (or rethrow the original IEMRException inside the catch) so a missing user is surfaced distinctly; specifically locate the call to iEMRUserRepositoryCustom.findByUserID and ensure that if user==null you throw new IEMRException("User not found with ID: "+userId) outside the generic catch, and change the catch to handle only unexpected exceptions (or rethrow existing IEMRException) so callers can distinguish not-found from other errors.
🧹 Nitpick comments (1)
src/main/java/com/iemr/common/data/users/User.java (1)
28-28: Remove the commentedlockUntilentity field or implement it fully.This placeholder adds dead code and leaves lock behavior unclear. If timed unlock is not being shipped now, remove it (and its import) to keep the model clean.
♻️ Suggested cleanup
-import java.time.LocalDateTime; @@ - /* `@Expose` - `@Column`(name="lock_until") - private LocalDateTime lockUntil; */Also applies to: 213-216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/data/users/User.java` at line 28, The User model contains a commented-out timed-lock field and an unused import—remove the dead code by deleting the commented `lockUntil` field and its related commented lines (and also remove the unused `import java.time.LocalDateTime;`) from the User class, or alternatively fully implement the feature by adding an actual `private LocalDateTime lockUntil;` field with its getter/setter and any required persistence annotations in the User class (ensure consistency with existing lock-related methods); pick one approach and apply it consistently across the class where the commented lines at 213-216 appear.
🤖 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/data/users/User.java`:
- Line 86: The OneToMany mapping in User.java uses mappedBy = "m_User", which
doesn't match the actual field name in FeedbackDetails (mUser); update the
mapping annotation on the collection in the User entity (the `@OneToMany` mappedBy
attribute) to mappedBy = "mUser" so it references the FeedbackDetails field
correctly and avoids JPA initialization errors.
In `@src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java`:
- Around line 419-425: In IEMRAdminUserServiceImpl the read-modify-write using
user.getFailedAttempt(), user.setFailedAttempt(...), and
iEMRUserRepositoryCustom.save(user) must be replaced with an atomic DB-side
increment to avoid lost updates under concurrent logins: add/use a repository
method like incrementFailedAttemptById(Long userId) that executes a single
UPDATE ... SET failed_attempt = failed_attempt + 1 (ideally with RETURNING or a
follow-up SELECT in the same transaction) and returns the new failedAttempt
count, call that from the login failure path instead of reading/modifying the
User entity, and use the returned value for lockout decisions (ensure the call
runs within a transactional context).
- Around line 408-412: The code in IEMRAdminUserServiceImpl checks failedAttempt
>= 5 with a hard-coded threshold; instead use the configured failedLoginAttempt
value so environments respect config. Replace the literal 5 with the configured
property/field (e.g., failedLoginAttempt or the getter that exposes the
configured threshold used elsewhere) in the failedAttempt check and ensure the
same configured value is used when deciding to throw the IEMRException.
- Around line 403-417: The userAuthenticateV1 flow omits account status
validation allowing deleted/deactivated accounts to log in; reintroduce a call
to checkUserAccountStatus(user) after retrieving the User object and before
performing password validation and failed-attempt checks in userAuthenticateV1
(use the existing checkUserAccountStatus method so suspended/disabled/deleted
users are rejected), ensuring the call happens right after User user =
users.get(0) and before securePassword.validatePasswordExisting(...) so status
is validated early in the method.
---
Outside diff comments:
In `@src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java`:
- Around line 1238-1252: The getUserById method in IEMRAdminUserServiceImpl
masks the "user not found" IEMRException because the catch(Exception) block
wraps everything; fix by doing the null check and throwing IEMRException before
the try-catch (or rethrow the original IEMRException inside the catch) so a
missing user is surfaced distinctly; specifically locate the call to
iEMRUserRepositoryCustom.findByUserID and ensure that if user==null you throw
new IEMRException("User not found with ID: "+userId) outside the generic catch,
and change the catch to handle only unexpected exceptions (or rethrow existing
IEMRException) so callers can distinguish not-found from other errors.
---
Nitpick comments:
In `@src/main/java/com/iemr/common/data/users/User.java`:
- Line 28: The User model contains a commented-out timed-lock field and an
unused import—remove the dead code by deleting the commented `lockUntil` field
and its related commented lines (and also remove the unused `import
java.time.LocalDateTime;`) from the User class, or alternatively fully implement
the feature by adding an actual `private LocalDateTime lockUntil;` field with
its getter/setter and any required persistence annotations in the User class
(ensure consistency with existing lock-related methods); pick one approach and
apply it consistently across the class where the commented lines at 213-216
appear.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 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 Set<UserLangMapping> m_UserLangMappings; | ||
|
|
||
| @OneToMany(mappedBy = "mUser", fetch = FetchType.EAGER) | ||
| @OneToMany(mappedBy = "m_User", fetch = FetchType.EAGER) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd 'FeedbackDetails.java' -t f | while read -r f; do
echo "== File: $f =="
cat -n "$f"
doneRepository: PSMRI/Common-API
Length of output: 25201
Correct the mappedBy reference to match the actual field name in FeedbackDetails.
The mappedBy = "m_User" in User.java references a non-existent field. FeedbackDetails defines the User field as mUser (camelCase), not m_User. This mismatch will cause JPA to fail during entity initialization at runtime.
Change mappedBy = "m_User" to mappedBy = "mUser".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/iemr/common/data/users/User.java` at line 86, The OneToMany
mapping in User.java uses mappedBy = "m_User", which doesn't match the actual
field name in FeedbackDetails (mUser); update the mapping annotation on the
collection in the User entity (the `@OneToMany` mappedBy attribute) to mappedBy =
"mUser" so it references the FeedbackDetails field correctly and avoids JPA
initialization errors.
Comment on lines
+403
to
+417
| User user = users.get(0); // ✅ FIRST declare user | ||
|
|
||
| // ✅ THEN check failed attempts | ||
| Integer failedAttempt = user.getFailedAttempt(); | ||
|
|
||
| if (failedAttempt != null && failedAttempt >= 5) { | ||
| throw new IEMRException( | ||
| "Your account has been locked. You can try tomorrow or connect to the administrator." | ||
| ); | ||
| } | ||
|
|
||
| try { | ||
| if (!securePassword.validatePasswordExisting(loginRequest.getPassword(), user.getPassword())) { | ||
| if (!securePassword.validatePasswordExisting( | ||
| loginRequest.getPassword(), | ||
| user.getPassword())) { |
Contributor
There was a problem hiding this comment.
Reintroduce account status validation in userAuthenticateV1.
This flow currently skips checkUserAccountStatus(user), so deleted/deactivated accounts can authenticate if credentials are correct.
🔒 Suggested fix
if (users.size() == 1) {
User user = users.get(0); // ✅ FIRST declare user
+ checkUserAccountStatus(user);
// ✅ THEN check failed attempts
Integer failedAttempt = user.getFailedAttempt();📝 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.
Suggested change
| User user = users.get(0); // ✅ FIRST declare user | |
| // ✅ THEN check failed attempts | |
| Integer failedAttempt = user.getFailedAttempt(); | |
| if (failedAttempt != null && failedAttempt >= 5) { | |
| throw new IEMRException( | |
| "Your account has been locked. You can try tomorrow or connect to the administrator." | |
| ); | |
| } | |
| try { | |
| if (!securePassword.validatePasswordExisting(loginRequest.getPassword(), user.getPassword())) { | |
| if (!securePassword.validatePasswordExisting( | |
| loginRequest.getPassword(), | |
| user.getPassword())) { | |
| User user = users.get(0); // ✅ FIRST declare user | |
| checkUserAccountStatus(user); | |
| // ✅ THEN check failed attempts | |
| Integer failedAttempt = user.getFailedAttempt(); | |
| if (failedAttempt != null && failedAttempt >= 5) { | |
| throw new IEMRException( | |
| "Your account has been locked. You can try tomorrow or connect to the administrator." | |
| ); | |
| } | |
| try { | |
| if (!securePassword.validatePasswordExisting( | |
| loginRequest.getPassword(), | |
| user.getPassword())) { |
🤖 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 403 - 417, The userAuthenticateV1 flow omits account status
validation allowing deleted/deactivated accounts to log in; reintroduce a call
to checkUserAccountStatus(user) after retrieving the User object and before
performing password validation and failed-attempt checks in userAuthenticateV1
(use the existing checkUserAccountStatus method so suspended/disabled/deleted
users are rejected), ensuring the call happens right after User user =
users.get(0) and before securePassword.validatePasswordExisting(...) so status
is validated early in the method.
Comment on lines
+408
to
+412
| if (failedAttempt != null && failedAttempt >= 5) { | ||
| throw new IEMRException( | ||
| "Your account has been locked. You can try tomorrow or connect to the administrator." | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid hard-coding the lock threshold to 5.
This bypasses the configured failedLoginAttempt value and can diverge behavior across environments.
⚙️ Suggested fix
- Integer failedAttempt = user.getFailedAttempt();
+ Integer failedAttempt = user.getFailedAttempt();
+ int maxFailedAttempts = 5;
+ if (failedLoginAttempt != null) {
+ maxFailedAttempts = Integer.parseInt(failedLoginAttempt);
+ }
- if (failedAttempt != null && failedAttempt >= 5) {
+ if (failedAttempt != null && failedAttempt >= maxFailedAttempts) {
throw new IEMRException(
"Your account has been locked. You can try tomorrow or connect to the 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 408 - 412, The code in IEMRAdminUserServiceImpl checks
failedAttempt >= 5 with a hard-coded threshold; instead use the configured
failedLoginAttempt value so environments respect config. Replace the literal 5
with the configured property/field (e.g., failedLoginAttempt or the getter that
exposes the configured threshold used elsewhere) in the failedAttempt check and
ensure the same configured value is used when deciding to throw the
IEMRException.
Comment on lines
+419
to
+425
| int currentAttempt = | ||
| user.getFailedAttempt() == null ? 0 : user.getFailedAttempt(); | ||
|
|
||
| currentAttempt++; | ||
| user.setFailedAttempt(currentAttempt); | ||
| iEMRUserRepositoryCustom.save(user); | ||
|
|
Contributor
There was a problem hiding this comment.
Failed-attempt update is non-atomic and can lose increments under parallel logins.
Concurrent invalid attempts can read the same value and overwrite each other, weakening lockout enforcement.
A safer pattern is an atomic DB-side increment (single statement) and using the returned value for lock decisions.
🤖 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 419 - 425, In IEMRAdminUserServiceImpl the read-modify-write using
user.getFailedAttempt(), user.setFailedAttempt(...), and
iEMRUserRepositoryCustom.save(user) must be replaced with an atomic DB-side
increment to avoid lost updates under concurrent logins: add/use a repository
method like incrementFailedAttemptById(Long userId) that executes a single
UPDATE ... SET failed_attempt = failed_attempt + 1 (ideally with RETURNING or a
follow-up SELECT in the same transaction) and returns the new failedAttempt
count, call that from the login failure path instead of reading/modifying the
User entity, and use the returned value for lockout decisions (ensure the call
runs within a transactional context).
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.
📋 Description
JIRA ID: N/A
✅ Type of Change
ℹ️ Additional Information
Testing
Verified compilation using:
mvn clean install -DskipTests
Manually tested login scenarios:
Wrong password increments failed attempt.
After 5 consecutive failures, login is blocked.
Successful login resets failed attempt counter.
No changes were made to existing test cases..
Summary by CodeRabbit
New Features
Bug Fixes