Skip to content

Add failed login attempt tracking and basic lock after 5 attempts #366

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
adititiwari0810 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add failed login attempt tracking and basic lock after 5 attempts #366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/main/java/com/iemr/common/data/users/User.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.sql.Timestamp;
import java.util.List;
import java.util.Set;
import java.time.LocalDateTime;

import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
Expand Down Expand Up @@ -82,7 +83,7 @@ public class User implements Serializable {
@JoinColumn(updatable = false, insertable = false, name = "userID", referencedColumnName = "userID")
private Set<UserLangMapping> m_UserLangMappings;

@OneToMany(mappedBy = "mUser", fetch = FetchType.EAGER)
@OneToMany(mappedBy = "m_User", fetch = FetchType.EAGER)
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
fd 'FeedbackDetails.java' -t f | while read -r f; do
  echo "== File: $f =="
  cat -n "$f"
done

Repository: 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.

@Transient
@Expose
private Set<FeedbackDetails> feedbackDetails;
Expand Down Expand Up @@ -209,6 +210,10 @@ public class User implements Serializable {
@Column(name = "failed_attempt")
private Integer failedAttempt;

/* @Expose
@Column(name="lock_until")
private LocalDateTime lockUntil; */

@Expose
@Column(name = "dhistoken")
private String dhistoken;
Expand Down
59 changes: 46 additions & 13 deletions src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,29 +386,62 @@ public User superUserAuthenticate(String userName, String password) throws Excep
return users.get(0);
}


@Override
public LoginResponseModel userAuthenticateV1(LoginRequestModel loginRequest, String ipAddress, String hostName)
throws Exception {


public LoginResponseModel userAuthenticateV1(LoginRequestModel loginRequest,
String ipAddress, String hostName) throws Exception {

LoginResponseModel loginResponseModel = null;
List<User> users = iEMRUserRepositoryCustom.findByUserName(loginRequest.getUserName());

List<User> users = iEMRUserRepositoryCustom
.findByUserName(loginRequest.getUserName());

if (users.size() == 1) {
User user = users.get(0);

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."
);
}
Comment on lines +408 to +412
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


try {
if (!securePassword.validatePasswordExisting(loginRequest.getPassword(), user.getPassword())) {
if (!securePassword.validatePasswordExisting(
loginRequest.getPassword(),
user.getPassword())) {
Comment on lines +403 to +417
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.


int currentAttempt =
user.getFailedAttempt() == null ? 0 : user.getFailedAttempt();

currentAttempt++;
user.setFailedAttempt(currentAttempt);
iEMRUserRepositoryCustom.save(user);

Comment on lines +419 to +425
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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).

throw new IEMRException("Invalid username or password");
}

} catch (Exception e) {
throw new IEMRException("Invalid username or password");
}

// ✅ reset on success
user.setFailedAttempt(0);
iEMRUserRepositoryCustom.save(user);

loginResponseModel = userMapper.userDataToLoginResponse(user);
logger.info("Login response is " + loginResponseModel.toString());
List<UserServiceRoleMapping> userServiceRoleMappings = getUserServiceRoleMapping(
loginResponseModel.getUserID());
loginResponseModel
.setUserServiceRoleMappings(userServiceRoleMapper.userRoleToLoginUserRole(userServiceRoleMappings));

// loginResponseModel.setHostName(hostName);
// loginResponseModel.setIpAddress(ipAddress);

List<UserServiceRoleMapping> userServiceRoleMappings =
getUserServiceRoleMapping(loginResponseModel.getUserID());

loginResponseModel.setUserServiceRoleMappings(
userServiceRoleMapper.userRoleToLoginUserRole(userServiceRoleMappings));

} else {
throw new IEMRException("Invalid username or password");
}
Expand Down