AMM-118: Add time-based account lockout with auto-unlock #332
Conversation
📝 WalkthroughWalkthroughAdds time-based account locking with a lock timestamp, automatic unlock after a configurable duration, admin endpoints to unlock accounts and retrieve lock status, login-failure handling updates tracking attempts/timestamps, and a new config property for lock duration. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Controller as IEMRAdminController
participant Service as IEMRAdminUserServiceImpl
participant Repo as UserRepository
participant DB as Database
rect rgb(220,240,255)
note over Client,Service: Failed login -> lock flow
Client->>Controller: Login (wrong password)
Controller->>Service: validateLogin / checkUserAccountStatus
Service->>Repo: find user by id
Repo->>DB: SELECT user
DB-->>Repo: user row (attempts, lockTimestamp)
Repo-->>Service: user entity
Service->>Service: increment attempts (null-safe)
alt threshold reached
Service->>Service: set lockTimestamp = now
Service->>Repo: save user (locked)
Repo->>DB: UPDATE user
Service-->>Controller: return lockout message (remaining time)
else below threshold
Service-->>Controller: return auth failure
end
end
rect rgb(240,255,240)
note over Client,Service: Auto-unlock on login attempt
Client->>Controller: Login while locked
Controller->>Service: checkUserAccountStatus
Service->>Repo: find user
Repo->>DB: SELECT user
DB-->>Repo: user row (lockTimestamp)
Repo-->>Service: user entity
alt lock duration elapsed
Service->>Service: clear lockTimestamp, reset attempts
Service->>Repo: save user (unlocked)
Repo->>DB: UPDATE user
Service-->>Controller: proceed with login
else still locked
Service-->>Controller: return remaining lock time
end
end
rect rgb(255,240,220)
note over Client,Controller: Admin unlock / status check
Client->>Controller: POST /admin/unlockUserAccount or /admin/getUserLockStatus
Controller->>Service: unlockUserAccount(userId) / getUserLockStatusJson(userId)
Service->>Repo: find user
Repo->>DB: SELECT user
DB-->>Repo: user row
Repo-->>Service: user entity
alt unlock
Service->>Service: clear lockTimestamp, reset attempts
Service->>Repo: save user
Repo->>DB: UPDATE user
Service-->>Controller: return success JSON
else status
Service-->>Controller: return status JSON (timestamp, remainingTime, attempts)
end
Controller-->>Client: JSON response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java:
- Around line 1263-1294: In unlockUserAccount, remove the redundant
iEMRUserRepositoryCustom.findByUserID(userId) call and use the same single
lookup pattern as getUserLockStatus by calling
iEMRUserRepositoryCustom.findById(userId).orElse(null) to fetch the User once;
then adjust the subsequent null check and logic to operate on that single user
instance and delete the fallback branch so no duplicate DB query occurs.
🧹 Nitpick comments (1)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (1)
1215-1237: Replace deprecated JsonParser constructor.The
new JsonParser()constructor is deprecated. UseJsonParser.parseString(request)instead for better compatibility with newer Gson versions.♻️ Recommended refactor
- JsonObject requestObj = new JsonParser().parse(request).getAsJsonObject(); + JsonObject requestObj = JsonParser.parseString(request).getAsJsonObject();
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
binsrc/main/java/com/iemr/common/controller/users/IEMRAdminController.javasrc/main/java/com/iemr/common/data/users/User.javasrc/main/java/com/iemr/common/repository/users/IEMRUserRepositoryCustom.javasrc/main/java/com/iemr/common/service/users/IEMRAdminUserService.javasrc/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.javasrc/main/resources/application.properties
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-18T08:53:22.725Z
Learnt from: helenKaryamsetty
Repo: PSMRI/Common-API PR: 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.
Applied to files:
src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java
🔇 Additional comments (9)
src/main/resources/application.properties (1)
172-173: Configuration property looks good.The new property follows Spring Boot conventions (kebab-case), is logically grouped with authentication settings, uses a reasonable default (24 hours), and includes a clear explanatory comment. Well done.
bin (1)
1-1: Submodule pointer—unable to review functional changes.This file is a git submodule reference and contains no executable code to review. The PR description indicates that functional changes exist in files like
IEMRAdminController.javaandIEMRAdminUserServiceImpl.java, but those files were not provided for review.Please confirm whether the source code files containing the account lockout implementation should be included in this review. A review of the submodule pointer alone cannot validate the PR objectives (login flow modifications, auto-unlock logic, new endpoints, configuration, and error handling).
src/main/java/com/iemr/common/data/users/User.java (1)
211-213: LGTM! New lock timestamp field properly added.The
lockTimestampfield is correctly annotated with JPA and JSON serialization annotations, and Lombok's@Datawill automatically generate the necessary getters and setters for lock management functionality.src/main/java/com/iemr/common/service/users/IEMRAdminUserService.java (1)
124-126: LGTM! Interface methods for account lock management.The new method signatures are well-defined and follow the existing code patterns in this interface. The return types and exception declarations are appropriate for their respective use cases.
src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java (5)
132-133: LGTM! Configuration property for lock duration.The
accountLockDurationHoursconfiguration field is properly defined with a sensible default value of 24 hours, allowing flexibility for different deployment environments.
225-249: LGTM! Auto-unlock logic correctly implemented.The account status check properly implements auto-unlock behavior:
- Correctly calculates lock duration using the configured hours
- Resets all lock-related fields when unlocking
- Provides clear error messages for different lock states
- Includes appropriate logging for audit trails
288-303: LGTM! Improved login failure handling with null-safety.The updated failure handling properly:
- Uses null-safe checks for
failedAttemptwith a sensible default- Captures lock timestamp when the threshold is reached
- Leverages the helper method for consistent error messaging
- Maintains existing attempt tracking behavior
331-360: LGTM! Well-structured error message generation.The helper method correctly:
- Handles null timestamps with a fallback message
- Calculates remaining lockout time accurately
- Formats the duration in a user-friendly format
- Provides actionable messages for both active locks and expired locks
1296-1312: LGTM! Clean implementation of lock status retrieval.The method correctly fetches the user entity and handles the not-found case appropriately. The implementation is straightforward and follows good error handling practices.
varundeepsaini
force-pushed
the
AMM-118-time-based-account-lockout
branch
from
edbe4a5 to
9c6b5c8
Compare
varundeepsaini
force-pushed
the
AMM-118-time-based-account-lockout
branch
from
9c6b5c8 to
ffdf91b
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java:
- Around line 1268-1296: The unlock endpoint lacks an admin-only check: add
role-based authorization to the controller method that exposes unlockUserAccount
by annotating the endpoint method in IEMRAdminController with
@PreAuthorize("hasRole('ADMIN')") (or equivalent), ensure the necessary import
for PreAuthorize is added, and confirm method security is enabled (e.g.,
@EnableGlobalMethodSecurity(prePostEnabled = true) or the appropriate
configuration) so only users with the ADMIN role can call the controller method
that invokes IEMRAdminUserServiceImpl.unlockUserAccount.
🧹 Nitpick comments (2)
src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java (2)
132-133: Add validation for the account lock duration configuration.The
accountLockDurationHoursfield lacks validation. If configured with zero or negative values, it would cause incorrect lock duration calculations throughout the code.✅ Recommended validation approach
Add a
@PostConstructmethod to validate the configuration:@PostConstruct public void validateConfiguration() { if (accountLockDurationHours <= 0) { throw new IllegalStateException("account.lock.duration.hours must be a positive integer, got: " + accountLockDurationHours); } }
225-249: Consider extracting the lock duration calculation to reduce duplication.The calculation
(long) accountLockDurationHours * 60 * 60 * 1000appears in multiple methods (lines 230, 338, 1319). Extracting this to a helper method or constant would improve maintainability.♻️ Suggested refactor
Add a helper method at the class level:
private long getLockDurationMillis() { return (long) accountLockDurationHours * 60 * 60 * 1000; }Then use it consistently:
- long lockDurationMillis = (long) accountLockDurationHours * 60 * 60 * 1000; + long lockDurationMillis = getLockDurationMillis();
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
binsrc/main/java/com/iemr/common/controller/users/IEMRAdminController.javasrc/main/java/com/iemr/common/data/users/User.javasrc/main/java/com/iemr/common/repository/users/IEMRUserRepositoryCustom.javasrc/main/java/com/iemr/common/service/users/IEMRAdminUserService.javasrc/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.javasrc/main/resources/application.properties
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
- bin
- src/main/java/com/iemr/common/data/users/User.java
- src/main/resources/application.properties
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-18T08:53:22.725Z
Learnt from: helenKaryamsetty
Repo: PSMRI/Common-API PR: 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.
Applied to files:
src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java
🔇 Additional comments (6)
src/main/java/com/iemr/common/repository/users/IEMRUserRepositoryCustom.java (1)
78-78: LGTM!This formatting change improves readability by adding visual separation between the method declarations. No functional impact.
src/main/java/com/iemr/common/service/users/IEMRAdminUserService.java (1)
126-128: LGTM!The new method signatures are well-defined and follow the existing conventions in the interface. The return types and exception declarations are appropriate for admin account management operations.
src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java (4)
288-303: LGTM!The null-safe handling of
failedAttemptprevents potential NPEs, and the lock logic correctly sets the timestamp when the threshold is reached. The use ofgenerateLockoutErrorMessageprovides clear feedback to users about when they can retry.
306-310: LGTM!The reset logic properly clears both the failed attempt counter and lock timestamp after successful authentication, ensuring a clean state.
331-360: LGTM!The helper method comprehensively handles lockout message generation with proper null checks, time calculations, and user-friendly formatting for various time remaining scenarios.
400-421: LGTM!The changes to
superUserAuthenticatemirror the updates inuserAuthenticate, maintaining consistency in lock handling across both authentication flows.
| @Override | ||
| public boolean unlockUserAccount(Long userId) throws IEMRException { | ||
| try { | ||
| User user = iEMRUserRepositoryCustom.findById(userId).orElse(null); | ||
|
|
||
| if (user == null) { | ||
| throw new IEMRException("User not found with ID: " + userId); | ||
| } | ||
|
|
||
| if (user.getDeleted() != null && user.getDeleted() && user.getLockTimestamp() != null) { | ||
| user.setDeleted(false); | ||
| user.setFailedAttempt(0); | ||
| user.setLockTimestamp(null); | ||
| iEMRUserRepositoryCustom.save(user); | ||
| logger.info("Admin manually unlocked user account for userID: {}", userId); | ||
| return true; | ||
| } else if (user.getDeleted() != null && user.getDeleted() && user.getLockTimestamp() == null) { | ||
| throw new IEMRException("User account is deactivated by administrator. Use user management to reactivate."); | ||
| } else { | ||
| logger.info("User account is not locked for userID: {}", userId); | ||
| return false; | ||
| } | ||
| } catch (IEMRException e) { | ||
| throw e; | ||
| } catch (Exception e) { | ||
| logger.error("Error unlocking user account with ID: " + userId, e); | ||
| throw new IEMRException("Error unlocking user account: " + e.getMessage(), e); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find controller class and unlockUserAccount endpoint mapping
rg -t java "unlockUserAccount" --context=5 | head -100Repository: PSMRI/Common-API
Length of output: 4109
🏁 Script executed:
#!/bin/bash
# Search for controller files and classes that might expose this endpoint
find . -type f -name "*Controller*.java" | xargs grep -l "unlockUserAccount" 2>/dev/nullRepository: PSMRI/Common-API
Length of output: 133
🏁 Script executed:
#!/bin/bash
# Search for authorization/security annotations on any method named unlockUserAccount
rg -t java -B10 "unlockUserAccount\(" | grep -E "(@PreAuthorize|@Secured|@RolesAllowed|@RequestMapping|@PostMapping|@GetMapping)" | head -50Repository: PSMRI/Common-API
Length of output: 273
🏁 Script executed:
#!/bin/bash
# Get the full IEMRAdminController class definition, especially class-level annotations
head -100 ./src/main/java/com/iemr/common/controller/users/IEMRAdminController.javaRepository: PSMRI/Common-API
Length of output: 3862
🏁 Script executed:
#!/bin/bash
# Search for the unlockUserAccount method with more context to see all annotations
rg -t java -B20 "public String unlockUserAccount" ./src/main/java/com/iemr/common/controller/users/IEMRAdminController.java | head -60Repository: PSMRI/Common-API
Length of output: 1099
🏁 Script executed:
#!/bin/bash
# Search for Spring Security configuration files
find . -type f -name "*Security*Config*.java" -o -name "*WebSecurity*.java" 2>/dev/null | head -20Repository: PSMRI/Common-API
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for authorization patterns on /user paths
rg -t java "(/user|unlockUserAccount)" --context=3 | grep -E "(PreAuthorize|Secured|RolesAllowed|hasRole|hasAuthority)" | head -30Repository: PSMRI/Common-API
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if the unlockUserAccount method or controller does any role verification internally
rg -t java -A30 "public String unlockUserAccount" ./src/main/java/com/iemr/common/controller/users/IEMRAdminController.javaRepository: PSMRI/Common-API
Length of output: 1454
Add admin role authorization to the /unlockUserAccount endpoint.
The endpoint currently only checks for an Authorization header presence but lacks explicit admin role restriction. This is a sensitive operation that should be restricted to admin users only. Add @PreAuthorize("hasRole('ADMIN')") or equivalent role-based access control to the unlockUserAccount method in IEMRAdminController to prevent unauthorized access.
🤖 Prompt for AI Agents
In @src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java
around lines 1268 - 1296, The unlock endpoint lacks an admin-only check: add
role-based authorization to the controller method that exposes unlockUserAccount
by annotating the endpoint method in IEMRAdminController with
@PreAuthorize("hasRole('ADMIN')") (or equivalent), ensure the necessary import
for PreAuthorize is added, and confirm method security is enabled (e.g.,
@EnableGlobalMethodSecurity(prePostEnabled = true) or the appropriate
configuration) so only users with the ADMIN role can call the controller method
that invokes IEMRAdminUserServiceImpl.unlockUserAccount.
| @Override | ||
| public String getUserLockStatusJson(Long userId) throws IEMRException { | ||
| try { | ||
| User user = iEMRUserRepositoryCustom.findById(userId).orElse(null); | ||
| if (user == null) { | ||
| throw new IEMRException("User not found with ID: " + userId); | ||
| } | ||
|
|
||
| org.json.JSONObject status = new org.json.JSONObject(); | ||
| status.put("userId", user.getUserID()); | ||
| status.put("userName", user.getUserName()); | ||
| status.put("failedAttempts", user.getFailedAttempt() != null ? user.getFailedAttempt() : 0); | ||
| status.put("statusID", user.getStatusID()); | ||
|
|
||
| boolean isDeleted = user.getDeleted() != null && user.getDeleted(); | ||
| boolean isLockedDueToFailedAttempts = isDeleted && user.getLockTimestamp() != null; | ||
|
|
||
| status.put("isLocked", isDeleted); | ||
| status.put("isLockedDueToFailedAttempts", isLockedDueToFailedAttempts); | ||
|
|
||
| if (isLockedDueToFailedAttempts) { | ||
| long lockDurationMillis = (long) accountLockDurationHours * 60 * 60 * 1000; | ||
| long remainingMillis = (user.getLockTimestamp().getTime() + lockDurationMillis) - System.currentTimeMillis(); | ||
| boolean lockExpired = remainingMillis <= 0; | ||
|
|
||
| status.put("lockExpired", lockExpired); | ||
| status.put("lockTimestamp", user.getLockTimestamp().toString()); | ||
| status.put("remainingTime", lockExpired ? "Lock expired - will unlock on next login" : formatRemainingTime(remainingMillis)); | ||
| if (!lockExpired) { | ||
| status.put("unlockTime", new java.sql.Timestamp(user.getLockTimestamp().getTime() + lockDurationMillis).toString()); | ||
| } | ||
| } else { | ||
| status.put("lockExpired", false); | ||
| status.put("lockTimestamp", org.json.JSONObject.NULL); | ||
| status.put("remainingTime", org.json.JSONObject.NULL); | ||
| } | ||
|
|
||
| return status.toString(); | ||
| } catch (IEMRException e) { | ||
| throw e; | ||
| } catch (Exception e) { | ||
| logger.error("Error fetching user lock status with ID: " + userId, e); | ||
| throw new IEMRException("Error fetching user lock status: " + e.getMessage(), e); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for controller methods calling getUserLockStatusJson
rg -n 'getUserLockStatusJson' --type=java -B5 -A10Repository: PSMRI/Common-API
Length of output: 4703
🏁 Script executed:
# Search for controller files that might contain lock-related endpoints
fd -t f -name '*Controller*.java' | xargs rg -l 'lock|Lock' --type=javaRepository: PSMRI/Common-API
Length of output: 10259
🏁 Script executed:
# Check for authorization annotations like @Secured, @PreAuthorize, @RolesAllowed on endpoints
rg -n '@Secured|@PreAuthorize|@RolesAllowed|@PermitAll|@DenyAll' --type=java | head -50Repository: PSMRI/Common-API
Length of output: 42
🏁 Script executed:
# Get the full controller class definition to see if there are class-level security annotations
rg -n 'class IEMRAdminController' --type=java -A 20Repository: PSMRI/Common-API
Length of output: 4320
🏁 Script executed:
# Look for the exact method with any annotations
rg -B 15 'public String getUserLockStatus\(@RequestBody String request\)' --type=javaRepository: PSMRI/Common-API
Length of output: 1983
🏁 Script executed:
# Check if there's any role-based authorization in place
rg -n 'hasRole|ROLE_ADMIN|@Secured|@PreAuthorize' --type=java | grep -i 'admin\|lock'Repository: PSMRI/Common-API
Length of output: 42
🏁 Script executed:
# Check what the Authorization header validation actually does
rg -n 'JwtUserIdValidationFilter|HTTPRequestInterceptor' --type=java -A 5 | head -100Repository: PSMRI/Common-API
Length of output: 7860
Add role-based access control to the lock status endpoint.
The getUserLockStatus endpoint at /getUserLockStatus requires an Authorization header but does not enforce admin-only access. Currently, any authenticated user with a valid JWT token can access sensitive user lock status information including timestamps and remaining lock durations. Add @PreAuthorize("hasRole('ADMIN')") to the controller method or implement equivalent role validation to restrict access to admin users only.
1 participant
📋 Description
Implements 24-hour time-based auto-unlock for user accounts locked due to failed login attempts. Previously, locked accounts required manual admin intervention. Now accounts automatically unlock after 24 hours, with clear error messages showing remaining lockout time. Also adds admin endpoints to manually unlock accounts and check lock status.
✅ Type of Change
ℹ️ Additional Information
Changes:
lock_timestampfield to User entity/unlockUserAccountendpoint for manual admin unlock/getUserLockStatusendpoint to check account statusaccount.lock.duration.hoursconfig property (default 24)Testing:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.