Skip to content

Conversation

@varundeepsaini
Copy link

@varundeepsaini varundeepsaini commented Jan 7, 2026

Part of PSMRI/AMRIT#118

📋 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

  • New feature (non-breaking change which adds functionality)

ℹ️ Additional Information

Changes:

  • Added lock_timestamp field to User entity
  • Modified login flow to auto-unlock after 24 hours
  • Added /unlockUserAccount endpoint for manual admin unlock
  • Added /getUserLockStatus endpoint to check account status
  • Added account.lock.duration.hours config property (default 24)
  • Error messages now show remaining lockout time

Testing:

  • Tested login with locked account shows remaining time
  • Tested auto-unlock after lock period expires
  • Tested admin unlock endpoint
  • Tested lock status endpoint returns correct info

Summary by CodeRabbit

  • New Features

    • Admins can manually unlock user accounts.
    • Admins can view detailed account lock status (timestamp, remaining lock time).
  • Improvements

    • Locked accounts auto-unlock after a configurable duration (default: 24 hours).
    • Authentication and account-locking flows improved for clearer messages and more reliable behavior.
    • Admin privilege checks added for lock-related actions.
  • Documentation

    • Added configuration setting for account lock duration.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 7, 2026

📝 Walkthrough

Walkthrough

Adds account lock timestamp to User, implements configurable time-based auto-unlock and centralized authentication/lock logic, and exposes admin endpoints to query lock status and manually unlock user accounts.

Changes

Cohort / File(s) Summary
Controller Endpoints
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
Added unlockUserAccount and getUserLockStatus POST endpoints; added helpers to parse userId, extract authenticated userId from Authorization/session, validate admin privileges, and return structured success/error responses.
Entity Model
src/main/java/com/iemr/common/data/users/User.java
Added lockTimestamp (Timestamp) mapped to lock_timestamp with @Expose and @Column.
Service Interface
src/main/java/com/iemr/common/service/users/IEMRAdminUserService.java
Added method signatures: unlockUserAccount(Long), getUserLockStatusJson(Long), and hasAdminPrivileges(Long).
Service Implementation
src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java
Introduced accountLockDurationHours config; centralized authentication/password/lock handling; auto-unlocks after configured duration; added unlockUserAccount, getUserLockStatusJson, hasAdminPrivileges, and multiple helpers for failed-attempt handling, lock messaging, and time calculations.
Configuration
src/main/resources/application.properties
Added account.lock.duration.hours=24 and a descriptive comment.
Repository (whitespace)
src/main/java/com/iemr/common/repository/users/IEMRUserRepositoryCustom.java
Minor whitespace/formatting normalization.

Sequence Diagram(s)

sequenceDiagram
    participant Admin as Admin/Client
    participant Controller as IEMRAdminController
    participant Service as IEMRAdminUserServiceImpl
    participant DB as User Repository/Database

    Admin->>Controller: POST /unlockUserAccount { userId }
    Controller->>Controller: parseUserIdFromRequest(), getAuthenticatedUserId()
    Controller->>Service: hasAdminPrivileges(authUserId)
    alt admin
        Controller->>Service: unlockUserAccount(userId)
        Service->>DB: find user by id
        DB-->>Service: User
        Service->>Service: clear lockTimestamp, reset attempts
        Service->>DB: save(user)
        Service-->>Controller: success
    else not admin
        Service-->>Controller: permission error
    end
    Controller-->>Admin: success/failure response
Loading
sequenceDiagram
    participant Client as User/Client
    participant AuthCtrl as Auth Controller
    participant Service as IEMRAdminUserServiceImpl
    participant DB as User Repository/Database

    Client->>AuthCtrl: Login(credentials)
    AuthCtrl->>Service: userAuthenticate(credentials)
    Service->>DB: findByUserId(userId)
    DB-->>Service: User
    Service->>Service: checkUserAccountStatus()
    alt Locked and within lock duration
        Service-->>AuthCtrl: lockout message (remaining time)
    else Locked but duration expired
        Service->>Service: auto-unlock (clear lockTimestamp, reset attempts)
        Service->>Service: validate password
        alt Password valid
            Service->>Service: reset failed attempts
            Service-->>AuthCtrl: auth success
        else Password invalid
            Service->>Service: handle failed attempt (increment/lock)
            Service-->>AuthCtrl: invalid/locked response
        end
    else Not locked
        Service->>Service: validate password
        alt Password valid
            Service->>Service: reset failed attempts
            Service-->>AuthCtrl: auth success
        else Password invalid
            Service->>Service: handle failed attempt (increment/lock)
            Service-->>AuthCtrl: invalid/locked response
        end
    end
Loading

Estimated Code Review Effort

🎯 4 (Complex) | ⏱️ ~40 minutes

Possibly Related Issues

Poem

🐇 I nudged the latch and watched the hour,
A lock that mended with a gentle power,
Admin hops in, clears the way,
Failed attempts fade — bright new day,
Hop, unlock — let users play.

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 9.38% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title directly summarizes the main change: implementing time-based account lockout with auto-unlock functionality, which aligns with the primary objectives of the PR.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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. Use JsonParser.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

📥 Commits

Reviewing files that changed from the base of the PR and between a1a0027 and edbe4a5.

📒 Files selected for processing (7)
  • bin
  • src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
  • src/main/java/com/iemr/common/data/users/User.java
  • src/main/java/com/iemr/common/repository/users/IEMRUserRepositoryCustom.java
  • src/main/java/com/iemr/common/service/users/IEMRAdminUserService.java
  • src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.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 (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.java and IEMRAdminUserServiceImpl.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 lockTimestamp field is correctly annotated with JPA and JSON serialization annotations, and Lombok's @Data will 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 accountLockDurationHours configuration 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 failedAttempt with 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 varundeepsaini force-pushed the AMM-118-time-based-account-lockout branch 2 times, most recently from 9c6b5c8 to ffdf91b Compare January 7, 2026 18:00
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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 accountLockDurationHours field lacks validation. If configured with zero or negative values, it would cause incorrect lock duration calculations throughout the code.

✅ Recommended validation approach

Add a @PostConstruct method 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 * 1000 appears 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

📥 Commits

Reviewing files that changed from the base of the PR and between edbe4a5 and ffdf91b.

📒 Files selected for processing (7)
  • bin
  • src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
  • src/main/java/com/iemr/common/data/users/User.java
  • src/main/java/com/iemr/common/repository/users/IEMRUserRepositoryCustom.java
  • src/main/java/com/iemr/common/service/users/IEMRAdminUserService.java
  • src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java
  • src/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 failedAttempt prevents potential NPEs, and the lock logic correctly sets the timestamp when the threshold is reached. The use of generateLockoutErrorMessage provides 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 superUserAuthenticate mirror the updates in userAuthenticate, maintaining consistency in lock handling across both authentication flows.

@varundeepsaini varundeepsaini force-pushed the AMM-118-time-based-account-lockout branch from ffdf91b to a2b1e2a Compare January 13, 2026 16:57
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In @src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java:
- Around line 340-345: getFailedAttemptThreshold currently calls
Integer.parseInt(failedLoginAttempt) directly which can throw
NumberFormatException; wrap the parse in a try-catch inside
getFailedAttemptThreshold, catch NumberFormatException, log a warning (using the
class logger) that the configured failedLoginAttempt value is invalid, and
return the default 5 on error or when failedLoginAttempt is null/blank.
- Around line 255-281: handlePasswordValidationAndLocking currently treats
validatePassword() results 1,2,3 as successful logins but never calls
resetFailedAttemptsIfNeeded(), so prior failedAttempt counts remain; update the
method to call resetFailedAttemptsIfNeeded(user) in the successful branches
(cases 1,2,3) — ensure you still call checkUserAccountStatus(user) and
iEMRUserRepositoryCustom.save(user) after resetting, and for case 1 (old format)
keep generateUpgradedPassword(password) before save; leave
handleFailedLoginAttempt(...) for case 0 unchanged.
🧹 Nitpick comments (1)
src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java (1)

285-294: Increase PBKDF2 iteration count significantly for better security.

The iteration count of 1001 is far below current OWASP recommendations. For PBKDF2-HMAC-SHA512, OWASP recommends 210,000 iterations (or tune to achieve 50–150 ms computation time on production hardware). While this matches existing patterns in the codebase, consider planning a migration to stronger parameters.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ffdf91b and a2b1e2a.

📒 Files selected for processing (6)
  • src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
  • src/main/java/com/iemr/common/data/users/User.java
  • src/main/java/com/iemr/common/repository/users/IEMRUserRepositoryCustom.java
  • src/main/java/com/iemr/common/service/users/IEMRAdminUserService.java
  • src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java
  • src/main/resources/application.properties
✅ Files skipped from review due to trivial changes (1)
  • src/main/java/com/iemr/common/repository/users/IEMRUserRepositoryCustom.java
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/main/resources/application.properties
  • src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
  • src/main/java/com/iemr/common/data/users/User.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-12-07T19:32:42.660Z
Learnt from: helenKaryamsetty
Repo: PSMRI/Common-API PR: 123
File: src/main/java/com/iemr/common/controller/abdmfacility/AbdmFacilityController.java:31-31
Timestamp: 2024-12-07T19:32:42.660Z
Learning: In this codebase, for Java Spring Boot API controllers like `AbdmFacilityController.java`, it's a common pattern to include the `Authorization` parameter in method signatures, even if it's not used in the implementation, to maintain consistency across APIs.

Applied to files:

  • src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java
📚 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 (8)
src/main/java/com/iemr/common/service/users/IEMRAdminUserService.java (1)

126-128: LGTM!

The new method declarations for unlockUserAccount and getUserLockStatusJson are well-designed. They follow the existing interface patterns, use appropriate return types, and declare the standard IEMRException for error handling.

src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java (7)

132-133: LGTM!

The configuration field with a sensible default of 24 hours is well implemented. Externalizing the lock duration allows for environment-specific tuning without code changes.


225-249: LGTM!

The auto-unlock logic is well-implemented. The method correctly distinguishes between time-based locks (with lockTimestamp) and admin deactivations (without lockTimestamp), providing appropriate messaging for each case. The explicit cast to long on line 230 appropriately prevents potential integer overflow for larger lock durations.


296-313: LGTM!

The failed login attempt handling logic correctly increments the counter and locks the account when the threshold is reached. The null check for getFailedAttempt() prevents potential NPEs.


347-368: LGTM!

The lockout messaging utilities are well-designed with appropriate handling for null timestamps, expired locks, and active locks. The remaining time calculation is correct.


1227-1255: LGTM!

The unlockUserAccount method correctly distinguishes between time-based account locks (unlockable via this endpoint) and admin deactivations (requiring user management). The error handling with specific messages for each scenario is helpful for administrators.


1257-1301: LGTM!

The getUserLockStatusJson method provides comprehensive lock status information appropriate for administrative use. The distinction between time-based locks and other deactivations is maintained, and the calculated fields (remaining time, unlock time) are useful for support operations.


1303-1309: LGTM!

The time formatting utility handles the common cases well. For edge cases with less than a minute remaining, "0 minutes" is displayed, which is acceptable for user-facing messages.

@varundeepsaini varundeepsaini force-pushed the AMM-118-time-based-account-lockout branch from a2b1e2a to 3395eb9 Compare January 13, 2026 17:09
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java:
- Around line 225-249: checkUserAccountStatus currently calls user.getDeleted()
without null-safe handling which can NPE; update the boolean checks to mirror
unlockUserAccount by guarding against null (e.g., use user.getDeleted() != null
&& user.getDeleted()) when testing deleted state and adjust logic paths
accordingly (including the branch that resets deleted to
false/failedAttempt/lockTimestamp and the else throws) so all uses of
getDeleted() are null-safe; ensure you update any related conditional like the
initial if(user.getDeleted()) and any subsequent flows that assume non-null
Deleted.
🧹 Nitpick comments (2)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (2)

1233-1262: Consider adding request/response logging for consistency.

Other endpoints in this controller log both request and response at INFO level. These new endpoints only log errors.

Suggested enhancement
 @Operation(summary = "Unlock user account locked due to failed login attempts")
 @RequestMapping(value = "/unlockUserAccount", method = RequestMethod.POST, produces = MediaType.APPLICATION_JSON, headers = "Authorization")
 public String unlockUserAccount(@RequestBody String request) {
     OutputResponse response = new OutputResponse();
+    logger.info("unlockUserAccount request: " + request);
     try {
         Long userId = parseUserIdFromRequest(request);
         boolean unlocked = iemrAdminUserServiceImpl.unlockUserAccount(userId);
         response.setResponse(unlocked ? "User account successfully unlocked" : "User account was not locked");
     } catch (Exception e) {
         logger.error("Error unlocking user account: " + e.getMessage(), e);
         response.setError(e);
     }
+    logger.info("unlockUserAccount response: " + response.toString());
     return response.toString();
 }

 @Operation(summary = "Get user account lock status")
 @RequestMapping(value = "/getUserLockStatus", method = RequestMethod.POST, produces = MediaType.APPLICATION_JSON, headers = "Authorization")
 public String getUserLockStatus(@RequestBody String request) {
     OutputResponse response = new OutputResponse();
+    logger.info("getUserLockStatus request: " + request);
     try {
         Long userId = parseUserIdFromRequest(request);
         String lockStatusJson = iemrAdminUserServiceImpl.getUserLockStatusJson(userId);
         response.setResponse(lockStatusJson);
     } catch (Exception e) {
         logger.error("Error getting user lock status: " + e.getMessage(), e);
         response.setError(e);
     }
+    logger.info("getUserLockStatus response: " + response.toString());
     return response.toString();
 }

1264-1270: Consider handling malformed userId values.

If userId is provided as a non-numeric string (e.g., "userId": "abc"), getAsLong() will throw a NumberFormatException with a less user-friendly message. Consider adding explicit validation.

Suggested enhancement
 private Long parseUserIdFromRequest(String request) throws IEMRException {
-    JsonObject requestObj = JsonParser.parseString(request).getAsJsonObject();
-    if (!requestObj.has("userId") || requestObj.get("userId").isJsonNull()) {
-        throw new IEMRException("userId is required");
+    try {
+        JsonObject requestObj = JsonParser.parseString(request).getAsJsonObject();
+        if (!requestObj.has("userId") || requestObj.get("userId").isJsonNull()) {
+            throw new IEMRException("userId is required");
+        }
+        return requestObj.get("userId").getAsLong();
+    } catch (NumberFormatException e) {
+        throw new IEMRException("Invalid userId format");
+    } catch (IllegalStateException | com.google.gson.JsonSyntaxException e) {
+        throw new IEMRException("Invalid request format");
     }
-    return requestObj.get("userId").getAsLong();
 }
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a2b1e2a and 3395eb9.

📒 Files selected for processing (6)
  • src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
  • src/main/java/com/iemr/common/data/users/User.java
  • src/main/java/com/iemr/common/repository/users/IEMRUserRepositoryCustom.java
  • src/main/java/com/iemr/common/service/users/IEMRAdminUserService.java
  • src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java
  • src/main/resources/application.properties
✅ Files skipped from review due to trivial changes (1)
  • src/main/java/com/iemr/common/repository/users/IEMRUserRepositoryCustom.java
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/main/resources/application.properties
  • src/main/java/com/iemr/common/data/users/User.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-12-07T19:32:42.660Z
Learnt from: helenKaryamsetty
Repo: PSMRI/Common-API PR: 123
File: src/main/java/com/iemr/common/controller/abdmfacility/AbdmFacilityController.java:31-31
Timestamp: 2024-12-07T19:32:42.660Z
Learning: In this codebase, for Java Spring Boot API controllers like `AbdmFacilityController.java`, it's a common pattern to include the `Authorization` parameter in method signatures, even if it's not used in the implementation, to maintain consistency across APIs.

Applied to files:

  • src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java
📚 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
🧬 Code graph analysis (1)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (3)
src/main/java/com/iemr/common/controller/covid/CovidVaccinationController.java (1)
  • RequestMapping (47-161)
src/main/java/com/iemr/common/controller/abdmfacility/AbdmFacilityController.java (1)
  • RequestMapping (18-48)
src/main/java/com/iemr/common/controller/email/EmailController.java (1)
  • RequestMapping (43-105)
🔇 Additional comments (8)
src/main/java/com/iemr/common/service/users/IEMRAdminUserService.java (1)

126-128: LGTM!

The new interface method declarations for unlockUserAccount and getUserLockStatusJson are well-defined and consistent with the existing interface patterns.

src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java (7)

132-133: LGTM!

The configuration field with a sensible default of 24 hours is properly wired and follows Spring conventions.


251-285: LGTM!

The refactored password validation and locking flow is well-structured with clear separation of concerns. The switch-case handles all validation outcomes appropriately.


287-331: LGTM!

The helper methods are well-implemented with proper null handling in handleFailedLoginAttempt. The differentiation between 1001 iterations for upgraded passwords vs 1000 for existing passwords is noted as intentional.


333-400: LGTM!

Good refactoring to extract common authentication logic into handlePasswordValidationAndLocking and getFailedAttemptThreshold, reducing code duplication between userAuthenticate and superUserAuthenticate.


361-381: LGTM!

The lockout error message generation with remaining time provides clear user feedback. The handling of null timestamps and expired locks is appropriate.


1240-1268: LGTM!

The manual unlock implementation correctly differentiates between time-based locks (from failed login attempts) and admin deactivations, providing appropriate responses for each case.


1270-1321: LGTM!

The lock status JSON provides comprehensive information for administrators. The formatRemainingTime helper handles most cases well.

@varundeepsaini varundeepsaini force-pushed the AMM-118-time-based-account-lockout branch 2 times, most recently from 848f506 to 29a1bb0 Compare January 13, 2026 17:20
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Fix all issues with AI agents
In @src/main/java/com/iemr/common/controller/users/IEMRAdminController.java:
- Around line 1276-1301: getAuthenticatedUserId currently only reads the JWT
from cookies so header-supplied tokens are ignored; update
getAuthenticatedUserId(HttpServletRequest) to first check
HttpServletRequest.getHeader("Jwttoken") and use that token if present
(non-null/non-empty), falling back to the existing cookie iteration for
"Jwttoken" otherwise, then validate via jwtUtil.getUserIdFromToken and parse
with Long.parseLong as before, preserving the same exception handling paths
(IEMRException on missing/invalid token and NumberFormatException handling).
- Around line 1268-1274: The parseUserIdFromRequest method can throw uncaught
JsonSyntaxException or NumberFormatException for malformed JSON or non-numeric
userId; wrap the parsing and getAsLong calls in a try/catch that catches
JsonSyntaxException, IllegalStateException, and NumberFormatException (or
Exception) and rethrow a user-friendly IEMRException (e.g., "Invalid request
body" or "userId must be a number") including the original exception as the
cause; also validate that requestObj.has("userId") &&
requestObj.get("userId").isJsonPrimitive() &&
requestObj.get("userId").getAsJsonPrimitive().isNumber() before calling
getAsLong to provide a clearer error path in parseUserIdFromRequest.

In @src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java:
- Around line 225-249: checkUserAccountStatus may NPE on user.getDeleted(); make
the deleted check null-safe by using a Boolean-safe comparison (e.g.,
Boolean.TRUE.equals(user.getDeleted()) or explicit null check) before treating
it as a boolean; update the conditional in checkUserAccountStatus to handle null
deleted values consistently with unlockUserAccount and preserve existing
behavior (auto-unlock, exception paths) when deleted is true, false, or null.
🧹 Nitpick comments (1)
src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java (1)

1315-1321: Edge case: Very short remaining times display as "0 minutes".

When remainingMillis is less than 60 seconds (e.g., 30 seconds), this method returns "0 minutes", which could confuse users who still cannot log in. Consider showing seconds for sub-minute durations or displaying "less than 1 minute".

♻️ Suggested improvement
 private String formatRemainingTime(long remainingMillis) {
     long hours = remainingMillis / (60 * 60 * 1000);
     long minutes = (remainingMillis % (60 * 60 * 1000)) / (60 * 1000);
     if (hours > 0 && minutes > 0) return String.format("%d hours %d minutes", hours, minutes);
     if (hours > 0) return String.format("%d hours", hours);
+    if (minutes <= 0) return "less than 1 minute";
     return String.format("%d minutes", minutes);
 }
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3395eb9 and 848f506.

📒 Files selected for processing (6)
  • src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
  • src/main/java/com/iemr/common/data/users/User.java
  • src/main/java/com/iemr/common/repository/users/IEMRUserRepositoryCustom.java
  • src/main/java/com/iemr/common/service/users/IEMRAdminUserService.java
  • src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java
  • src/main/resources/application.properties
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/main/resources/application.properties
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-12-07T19:32:42.660Z
Learnt from: helenKaryamsetty
Repo: PSMRI/Common-API PR: 123
File: src/main/java/com/iemr/common/controller/abdmfacility/AbdmFacilityController.java:31-31
Timestamp: 2024-12-07T19:32:42.660Z
Learning: In this codebase, for Java Spring Boot API controllers like `AbdmFacilityController.java`, it's a common pattern to include the `Authorization` parameter in method signatures, even if it's not used in the implementation, to maintain consistency across APIs.

Applied to files:

  • src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java
📚 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
🧬 Code graph analysis (1)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (3)
src/main/java/com/iemr/common/controller/covid/CovidVaccinationController.java (1)
  • RequestMapping (47-161)
src/main/java/com/iemr/common/controller/abdmfacility/AbdmFacilityController.java (1)
  • RequestMapping (18-48)
src/main/java/com/iemr/common/controller/email/EmailController.java (1)
  • RequestMapping (43-105)
🔇 Additional comments (12)
src/main/java/com/iemr/common/repository/users/IEMRUserRepositoryCustom.java (1)

78-78: No concerns with this formatting change.

This is a trivial whitespace addition (blank line) between method declarations with no functional impact.

src/main/java/com/iemr/common/data/users/User.java (1)

216-218: LGTM!

The lockTimestamp field is appropriately added with correct annotations (@Expose for JSON serialization, @Column for JPA mapping). The naming and type are consistent with other timestamp fields in the entity.

src/main/java/com/iemr/common/service/users/IEMRAdminUserService.java (1)

126-130: LGTM!

The new method signatures are well-defined with appropriate return types and consistent exception handling. They complement the existing service contract for the account lockout feature.

src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java (6)

132-133: LGTM!

Good use of externalized configuration with a sensible default value. The @Value annotation correctly provides fallback to 24 hours if the property is not configured.


251-285: Good centralization of authentication and locking logic.

The helper method cleanly consolidates password validation and account locking logic used by both userAuthenticate() and superUserAuthenticate(). The switch cases handle all validation outcomes appropriately.

Minor observation: Cases 2/3 and the default case both call clearFailedAttemptState/resetFailedAttemptsIfNeeded followed by save, which is slightly redundant but ensures consistency.


307-324: LGTM!

The failed login handling correctly increments the counter, locks the account at threshold, and records the lock timestamp for accurate remaining time calculations.


1240-1268: LGTM!

The unlock logic correctly distinguishes between accounts locked due to failed attempts (unlockable) versus accounts deactivated by administrators (requires user management). Proper null-safety checks and logging are in place.


1270-1313: LGTM!

The lock status JSON response provides comprehensive information for administrators including failed attempt count, lock state, and remaining time. The distinction between login-lockout and admin-deactivation states is clearly communicated.


1323-1344: Loose role name matching could allow unintended roles to be flagged as admin.

The contains() check for "admin", "supervisor", and "provideradmin" relies on substring matching. While the three keywords are reasonably specific and no problematic role names are evident in the codebase, this pattern is inherently fragile. Consider either adding test cases for edge case role names (e.g., "Administrator", "NonAdminReviewer") to ensure they behave as expected, or refactoring to use exact matches or an enum-based approach for more maintainability.

src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (3)

1234-1249: LGTM!

The endpoint correctly implements authentication and authorization checks before allowing the admin unlock operation. Error handling and logging are properly implemented.


1251-1266: LGTM!

The lock status endpoint follows the same secure pattern as the unlock endpoint with proper authentication and authorization checks.


1303-1308: LGTM!

The authorization helper is clean and includes appropriate audit logging for unauthorized access attempts.

@varundeepsaini varundeepsaini force-pushed the AMM-118-time-based-account-lockout branch from 29a1bb0 to b657431 Compare January 13, 2026 17:28
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @src/main/java/com/iemr/common/controller/users/IEMRAdminController.java:
- Around line 1286-1315: In getAuthenticatedUserId, call
jwtUtil.validateToken(jwtToken) before jwtUtil.getUserIdFromToken(jwtToken)
(same pattern used in refreshToken) and throw an IEMRException if validateToken
returns false or throws, so denylisted/revoked tokens are rejected; then proceed
to call getUserIdFromToken and parse the userId as before, preserving existing
NumberFormatException and generic exception handling.
🧹 Nitpick comments (1)
src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java (1)

255-285: Minor inefficiency: redundant database save operations.

Cases 2 and 3 (lines 271-277) always call iEMRUserRepositoryCustom.save(user) even when no state has changed (failed attempts were already 0 and lock timestamp was already null). Consider checking whether a save is actually needed:

 			case 2:
 			case 3:
 				// Valid password
 				checkUserAccountStatus(user);
-				clearFailedAttemptState(user);
-				iEMRUserRepositoryCustom.save(user);
+				if (user.getFailedAttempt() != null && user.getFailedAttempt() != 0) {
+					clearFailedAttemptState(user);
+					iEMRUserRepositoryCustom.save(user);
+				}
 				break;

This is similar to what resetFailedAttemptsIfNeeded already does in the default case.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 848f506 and b657431.

📒 Files selected for processing (6)
  • src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
  • src/main/java/com/iemr/common/data/users/User.java
  • src/main/java/com/iemr/common/repository/users/IEMRUserRepositoryCustom.java
  • src/main/java/com/iemr/common/service/users/IEMRAdminUserService.java
  • src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java
  • src/main/resources/application.properties
✅ Files skipped from review due to trivial changes (1)
  • src/main/java/com/iemr/common/repository/users/IEMRUserRepositoryCustom.java
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/main/java/com/iemr/common/data/users/User.java
  • src/main/resources/application.properties
  • src/main/java/com/iemr/common/service/users/IEMRAdminUserService.java
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-12-07T19:32:42.660Z
Learnt from: helenKaryamsetty
Repo: PSMRI/Common-API PR: 123
File: src/main/java/com/iemr/common/controller/abdmfacility/AbdmFacilityController.java:31-31
Timestamp: 2024-12-07T19:32:42.660Z
Learning: In this codebase, for Java Spring Boot API controllers like `AbdmFacilityController.java`, it's a common pattern to include the `Authorization` parameter in method signatures, even if it's not used in the implementation, to maintain consistency across APIs.

Applied to files:

  • src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java
📚 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
📚 Learning: 2025-07-11T18:00:56.920Z
Learnt from: Zapper9982
Repo: PSMRI/Common-API PR: 258
File: src/test/java/com/iemr/common/controller/users/IEMRAdminControllerTest.java:704-704
Timestamp: 2025-07-11T18:00:56.920Z
Learning: In the IEMRAdminController (and likely other controllers in this codebase), error handling is incomplete - NullPointerExceptions are being allowed to propagate to clients instead of being caught and converted to proper error responses. Tests expect error messages like "Cannot invoke \"org.json.JSONObject.toString()\" because \"responseObj\" is null", indicating a systemic issue with input validation and null handling in controller methods.

Applied to files:

  • src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
📚 Learning: 2025-02-03T12:42:38.278Z
Learnt from: indraniBan
Repo: PSMRI/Common-API PR: 138
File: src/main/java/com/iemr/common/utils/JwtAuthenticationUtil.java:0-0
Timestamp: 2025-02-03T12:42:38.278Z
Learning: In the JwtAuthenticationUtil class, catching a generic Exception in the validateUserIdAndJwtToken method is acceptable as per the team's preference.

Applied to files:

  • src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
📚 Learning: 2025-02-21T07:43:03.828Z
Learnt from: sandipkarmakar3
Repo: PSMRI/Common-API PR: 162
File: src/main/java/com/iemr/common/utils/CookieUtil.java:52-66
Timestamp: 2025-02-21T07:43:03.828Z
Learning: In the Common-API project's CookieUtil class, the current implementation of addJwtTokenToCookie using both response.addCookie() and manual Set-Cookie header has been tested and confirmed to work as required for their use case. URL encoding of the JWT token is not necessary.

Applied to files:

  • src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
🔇 Additional comments (12)
src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java (8)

132-133: LGTM!

Good use of configuration-driven lock duration with a sensible default value. This allows flexibility without code changes.


225-249: LGTM!

The auto-unlock logic correctly distinguishes between time-based lockouts (with lockTimestamp) and admin-initiated deactivations (without lockTimestamp). The duration check and state reset are properly implemented.


292-294: LGTM!

The cast to long is correctly placed at the beginning of the expression, ensuring the entire multiplication is performed in long arithmetic to avoid overflow.


307-324: LGTM!

The failed login attempt handling correctly locks the account on the Nth failed attempt (where N equals the threshold) and properly sets the lock timestamp for time-based auto-unlock.


1315-1321: LGTM!

The time formatting logic correctly handles all cases. Callers already guard against negative values.


1240-1268: LGTM!

Good implementation that correctly distinguishes between time-based lockouts (unlockable via this endpoint) and admin-initiated deactivations (requiring user management). The error messaging is clear and appropriate.


1270-1313: LGTM!

The lock status endpoint provides comprehensive information for administrators to diagnose account states. The response correctly distinguishes between different lock scenarios and provides actionable information like remaining time and expected unlock time.


1323-1344: Verify admin role names in the m_role database table match the expected patterns.

The hasAdminPrivileges method relies on substring matching for role names containing "admin", "supervisor", or "provideradmin". Since role names are populated in the external m_role database table (not defined in this codebase), confirm that actual admin roles follow these naming conventions. If they don't, the privilege check may fail to identify legitimate admins or incorrectly identify non-admin roles.

src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (4)

1234-1249: LGTM!

The endpoint properly enforces authentication and admin authorization before allowing account unlock operations. The separation of concerns using helper methods improves maintainability.


1251-1266: LGTM!

Consistent implementation with the unlock endpoint. Both endpoints share the same authentication and authorization pattern.


1268-1284: LGTM!

Robust input validation that checks for presence, type, and format of the userId parameter. The exception handling properly wraps parsing errors.


1317-1322: LGTM!

Good security practice to log unauthorized access attempts for audit purposes. The error message is clear without revealing internal details.

@varundeepsaini varundeepsaini force-pushed the AMM-118-time-based-account-lockout branch 3 times, most recently from 710f70f to 2984d5e Compare January 14, 2026 11:47
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java`:
- Around line 1328-1335: The authorization loop in IEMRAdminUserServiceImpl uses
role.getRoleName().toLowerCase().contains(...), which can falsely match
unintended names; replace the contains checks with exact matching (e.g., use
role.getRoleName().trim().equalsIgnoreCase("Admin"),
equalsIgnoreCase("Supervisor"), equalsIgnoreCase("ProviderAdmin") or compare
against a Set of allowed role names) so only exact role names grant privileges;
update the logic in the loop that iterates UserServiceRoleMapping -> Role ->
getRoleName accordingly.
🧹 Nitpick comments (2)
src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java (2)

276-281: Default case appears to be unreachable dead code.

Based on the securePassword.validatePassword() implementation (which returns 0, 1, 2, or 3), this default case will never execute. Consider removing it or converting it to throw an exception for unexpected return values:

♻️ Proposed fix
 		default:
-			// Successful validation - reset failed attempts if needed
-			checkUserAccountStatus(user);
-			resetFailedAttemptsIfNeeded(user);
-			break;
+			// Unexpected return value - should never happen
+			throw new IEMRException("Unexpected password validation result: " + validatePassword);

1313-1319: Edge case: remaining time less than 1 minute shows "0 minutes".

When the lock is about to expire (less than 1 minute remaining), this will display "0 minutes" which may confuse users. Consider handling seconds or displaying a more informative message:

♻️ Proposed improvement
 private String formatRemainingTime(long remainingMillis) {
 	long hours = remainingMillis / (60 * 60 * 1000);
 	long minutes = (remainingMillis % (60 * 60 * 1000)) / (60 * 1000);
 	if (hours > 0 && minutes > 0) return String.format("%d hours %d minutes", hours, minutes);
 	if (hours > 0) return String.format("%d hours", hours);
+	if (minutes <= 0) return "less than a minute";
 	return String.format("%d minutes", minutes);
 }
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b657431 and 2984d5e.

📒 Files selected for processing (6)
  • src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
  • src/main/java/com/iemr/common/data/users/User.java
  • src/main/java/com/iemr/common/repository/users/IEMRUserRepositoryCustom.java
  • src/main/java/com/iemr/common/service/users/IEMRAdminUserService.java
  • src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java
  • src/main/resources/application.properties
✅ Files skipped from review due to trivial changes (1)
  • src/main/java/com/iemr/common/repository/users/IEMRUserRepositoryCustom.java
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/main/java/com/iemr/common/data/users/User.java
  • src/main/resources/application.properties
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-12-07T19:32:42.660Z
Learnt from: helenKaryamsetty
Repo: PSMRI/Common-API PR: 123
File: src/main/java/com/iemr/common/controller/abdmfacility/AbdmFacilityController.java:31-31
Timestamp: 2024-12-07T19:32:42.660Z
Learning: In this codebase, for Java Spring Boot API controllers like `AbdmFacilityController.java`, it's a common pattern to include the `Authorization` parameter in method signatures, even if it's not used in the implementation, to maintain consistency across APIs.

Applied to files:

  • src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java
📚 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
📚 Learning: 2025-07-11T18:00:56.920Z
Learnt from: Zapper9982
Repo: PSMRI/Common-API PR: 258
File: src/test/java/com/iemr/common/controller/users/IEMRAdminControllerTest.java:704-704
Timestamp: 2025-07-11T18:00:56.920Z
Learning: In the IEMRAdminController (and likely other controllers in this codebase), error handling is incomplete - NullPointerExceptions are being allowed to propagate to clients instead of being caught and converted to proper error responses. Tests expect error messages like "Cannot invoke \"org.json.JSONObject.toString()\" because \"responseObj\" is null", indicating a systemic issue with input validation and null handling in controller methods.

Applied to files:

  • src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
📚 Learning: 2025-02-03T12:42:38.278Z
Learnt from: indraniBan
Repo: PSMRI/Common-API PR: 138
File: src/main/java/com/iemr/common/utils/JwtAuthenticationUtil.java:0-0
Timestamp: 2025-02-03T12:42:38.278Z
Learning: In the JwtAuthenticationUtil class, catching a generic Exception in the validateUserIdAndJwtToken method is acceptable as per the team's preference.

Applied to files:

  • src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
🔇 Additional comments (10)
src/main/java/com/iemr/common/service/users/IEMRAdminUserService.java (1)

126-131: LGTM!

The new method declarations follow the existing interface patterns and have appropriate return types and exception handling. The interface provides a clean contract for account lock management operations.

src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java (4)

132-133: LGTM!

Configuration-driven lock duration with a sensible default of 24 hours is a good approach for flexibility.


225-248: LGTM!

The auto-unlock logic is well-implemented with proper null-safety checks and clear logging. The time-based unlock calculation correctly uses the configured lock duration.


1238-1266: LGTM!

The unlock logic correctly distinguishes between accounts locked due to failed attempts versus those deactivated by an administrator, providing appropriate handling for each scenario. The audit logging for admin unlock actions is a good practice.


1268-1311: LGTM!

The lock status JSON provides comprehensive information including lock state, timestamps, and calculated remaining time. The conditional logic properly handles the different scenarios (locked due to failed attempts vs. admin deactivation vs. not locked).

src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (5)

80-80: LGTM!

Good practice to use a constant for the field name to avoid magic strings.


587-592: LGTM!

Good addition of JWT token validation before extracting the user ID, ensuring that expired or denylisted tokens are properly rejected.


1242-1274: LGTM!

Both endpoints follow a secure and consistent pattern: authenticate the caller, validate admin privileges, then perform the operation. The error handling and logging are appropriate.


1276-1293: LGTM!

Comprehensive input validation with proper handling of malformed JSON, missing fields, and type validation. The error messages are user-friendly without leaking implementation details.


1295-1323: LGTM!

The authentication helper correctly uses the session-based approach consistent with the codebase patterns. The admin privilege validation provides clear access control with appropriate logging of unauthorized access attempts.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Signed-off-by: Varun Deep Saini <varun.23bcs10048@ms.sst.scaler.com>
@varundeepsaini varundeepsaini force-pushed the AMM-118-time-based-account-lockout branch from 2984d5e to e57bce4 Compare January 14, 2026 11:55
@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant