From 829c039f5ee707003be13b0a51863ce4b849f0f0 Mon Sep 17 00:00:00 2001 From: DJAngel973 <189556164+DJAngel973@users.noreply.github.com> Date: Sat, 31 Oct 2026 18:14:40 -0500 Subject: [PATCH 1/3] fix(auth): enforce failed-login lockout --- database/03-tables.sql | 5 +- database/08-migrations.sql | 14 +++ docs/IMPLEMENTATION-PLAN.md | 38 +++--- .../secure/auth/service/AuthService.java | 29 ++--- .../auth/service/LoginAttemptService.java | 58 +++++++++ .../com/wallet/secure/user/entity/User.java | 4 +- .../user/repository/UserRepository.java | 11 ++ .../secure/auth/service/AuthServiceTest.java | 29 ++++- .../auth/service/LoginAttemptServiceTest.java | 110 ++++++++++++++++++ 9 files changed, 261 insertions(+), 37 deletions(-) create mode 100644 src/main/java/com/wallet/secure/auth/service/LoginAttemptService.java create mode 100644 src/test/java/com/wallet/secure/auth/service/LoginAttemptServiceTest.java diff --git a/database/03-tables.sql b/database/03-tables.sql index 7352afc..d741f66 100644 --- a/database/03-tables.sql +++ b/database/03-tables.sql @@ -6,7 +6,8 @@ CREATE TABLE users ( password_hash TEXT NOT NULL, role user_role NOT NULL DEFAULT 'USER', is_active BOOLEAN DEFAULT TRUE, - failed_login_attempts INT DEFAULT 0 CHECK (failed_login_attempts >= 0 AND failed_login_attempts <= 3), + failed_login_attempts INT DEFAULT 0 CONSTRAINT users_failed_login_attempts_range + CHECK (failed_login_attempts >= 0 AND failed_login_attempts <= 5), locked_until TIMESTAMP, email_verified BOOLEAN DEFAULT FALSE, two_factor_enabled BOOLEAN DEFAULT FALSE, @@ -120,4 +121,4 @@ CREATE TABLE transaction_history ( COMMENT ON TABLE transaction_history IS 'Complete history of transaction status changes'; -CREATE INDEX idx_transaction_history_transaction ON transaction_history(transaction_id); \ No newline at end of file +CREATE INDEX idx_transaction_history_transaction ON transaction_history(transaction_id); diff --git a/database/08-migrations.sql b/database/08-migrations.sql index 633bb10..d450954 100644 --- a/database/08-migrations.sql +++ b/database/08-migrations.sql @@ -21,3 +21,17 @@ ALTER TYPE audit_action ADD VALUE IF NOT EXISTS 'WALLET_RESTORE'; -- Date: 2026-10-27 -- OWASP A07/A09: a consumed refresh token presented again is a security signal. ALTER TYPE audit_action ADD VALUE IF NOT EXISTS 'TOKEN_REFRESH'; + +-- Migration: align failed-login counter with the application lockout policy +-- Date: 2026-10-31 +-- OWASP A07: the database must accept every state used by the authoritative +-- five-attempt policy; otherwise a valid lockout update fails. +ALTER TABLE users + DROP CONSTRAINT IF EXISTS users_failed_login_attempts_check; + +ALTER TABLE users + DROP CONSTRAINT IF EXISTS users_failed_login_attempts_range; + +ALTER TABLE users + ADD CONSTRAINT users_failed_login_attempts_range + CHECK (failed_login_attempts >= 0 AND failed_login_attempts <= 5); diff --git a/docs/IMPLEMENTATION-PLAN.md b/docs/IMPLEMENTATION-PLAN.md index 5bb4122..b318074 100644 --- a/docs/IMPLEMENTATION-PLAN.md +++ b/docs/IMPLEMENTATION-PLAN.md @@ -163,7 +163,7 @@ The former public seed is no longer used and a controlled bootstrap now exists. **Current problem** -The refresh endpoint issues only a new access token and keeps the same refresh token. Reuse detection and atomic rotation are not implemented. +Resolved. The refresh endpoint now consumes the presented session under a pessimistic row lock and creates a replacement session in the same transaction. **Expected behavior** @@ -184,11 +184,17 @@ The refresh endpoint issues only a new access token and keeps the same refresh t **Acceptance criteria** -- [ ] A successful refresh returns two new tokens. -- [ ] The previous refresh token fails immediately afterward. -- [ ] Exactly one of two concurrent refresh requests with the same token succeeds. -- [ ] Rotation and reuse failures create audit evidence without logging token values. -- [ ] Logout revokes the current rotated session. +- [x] A successful refresh returns two new tokens. +- [x] The previous refresh token fails immediately afterward. +- [x] Exactly one of two concurrent refresh requests with the same token succeeds through `PESSIMISTIC_WRITE` session locking. +- [x] Rotation and reuse failures create audit evidence without logging token values. +- [x] Logout revokes the current rotated session. + +**Verification note — 2026-10-27** + +- `SessionService.rotateSession()` locks the active session row, revokes it and creates its replacement in one transaction; a `jti` makes refresh JWTs unique even when issued in the same second. +- A real Docker/PostgreSQL flow confirmed refresh rotation, rejection of the old token, and logout revocation of the rotated token. +- `./scripts/test.sh` passed: 112 tests, 0 failures and 0 errors. ### SEC-005 — Correct failed-login lockout @@ -198,7 +204,7 @@ The refresh endpoint issues only a new access token and keeps the same refresh t **Current problem** -Failed login events are audited, but the authentication flow does not demonstrably call repository operations that increment attempts and set `lockedUntil`. Values also conflict: application configuration states five attempts while the database constrains the counter to three. +Resolved in code. Failed attempts are serialized with a pessimistic user-row lock and persisted in a `REQUIRES_NEW` transaction, so rethrowing the authentication failure cannot roll the counter back. The database and configuration now both use five attempts and a 30-minute lock. **Expected behavior** @@ -222,12 +228,18 @@ Failed login events are audited, but the authentication flow does not demonstrab **Acceptance criteria** -- [ ] The configured number of consecutive failures locks the account. -- [ ] PostgreSQL accepts every valid counter state under the selected policy. -- [ ] A correct password during the lock period cannot authenticate. -- [ ] Authentication succeeds after expiry according to the chosen unlock policy. -- [ ] Success resets failed attempts to zero. -- [ ] Unit and PostgreSQL-backed tests cover boundary and concurrency cases. +- [x] The configured number of consecutive failures locks the account. +- [x] PostgreSQL accepts every valid counter state under the selected policy through the migration and schema constraint. +- [x] A correct password during the lock period cannot authenticate through Spring Security's account-lock status. +- [x] Authentication succeeds after expiry through the existing time-based `isAccountLocked()` policy, then resets the counter. +- [x] Success resets failed attempts to zero. +- [ ] A Docker/PostgreSQL concurrency check remains to prove that simultaneous failed attempts serialize as intended. Unit tests cover boundary behavior, unknown-email non-enumeration, locked-account state and the failure handoff. + +**Verification note — 2026-10-31** + +- `LoginAttemptService` uses `REQUIRES_NEW` and `findByEmailWithLock()` because `AuthService.login()` rethrows `BadCredentialsException`; without the independent transaction, the counter would roll back with the failed login. +- The fifth consecutive failed attempt caps the counter at five, sets `locked_until` for 30 minutes, writes a failure audit event and writes a CRITICAL lockout event. No raw credentials are logged. +- `./scripts/test.sh` passed: 117 tests, 0 failures and 0 errors. ### SEC-006 — Reject JWT authentication for disabled or locked accounts diff --git a/src/main/java/com/wallet/secure/auth/service/AuthService.java b/src/main/java/com/wallet/secure/auth/service/AuthService.java index 4dc2f19..3c6ae7c 100644 --- a/src/main/java/com/wallet/secure/auth/service/AuthService.java +++ b/src/main/java/com/wallet/secure/auth/service/AuthService.java @@ -50,6 +50,7 @@ public class AuthService { private final UserService userService; private final AuditService auditService; private final SessionService sessionService; + private final LoginAttemptService loginAttemptService; // --- Register @@ -187,29 +188,23 @@ public ApiResponse login(LoginRequest request, HttpServletRequest "Account locked", extractIp(httpRequest), extractUserAgent(httpRequest))); throw e; } catch (BadCredentialsException e) { - // Log failure - check if brute force threshold is reached - userRepository.findByEmail(request.getEmail()).ifPresent(user -> { - auditService.logLoginFailure(user.getId(), - request.getEmail(), - "Bad credentials", extractIp(httpRequest), extractUserAgent(httpRequest)); - // OWASP A07: brute force detection - // If >= 5 failures in 15 minutes -> CRITICAL alert - long recentFailures = auditService.countRecentFailedLogins(user.getId(), 15); - if (recentFailures >= 5) { - auditService.logCriticalSecurityEvent( - user.getId(), - String.format("Brute force detected: %s failed logins in 15 minutes", recentFailures), - - extractIp(httpRequest), extractUserAgent(httpRequest)); - } - }); + // OWASP A07: persist the counter in an independent transaction because + // this method rethrows and its transaction will roll back. + loginAttemptService.recordFailure(request.getEmail(), ip, userAgent); throw e; } // Only reached if authentication succeeded - User user = userRepository.findByEmail(request.getEmail()) + User user = userRepository.findByEmailWithLock(request.getEmail()) .orElseThrow(() -> new InvalidCredentialsException("Invalid credentials")); + // OWASP A07: authentication may have started just before another request + // reached the threshold. Recheck after obtaining the same row lock. + if (user.isAccountLocked()) { + auditService.logLoginFailure(user.getId(), user.getEmail(), "Account locked", ip, userAgent); + throw new LockedException("Account temporarily locked"); + } + // Reset failed attempts counter + update lastLoginAt // OWASP A07: counter reset prevents permanent lockout after legitime login user.resetFailedLoginAttempts(); diff --git a/src/main/java/com/wallet/secure/auth/service/LoginAttemptService.java b/src/main/java/com/wallet/secure/auth/service/LoginAttemptService.java new file mode 100644 index 0000000..beea8cd --- /dev/null +++ b/src/main/java/com/wallet/secure/auth/service/LoginAttemptService.java @@ -0,0 +1,58 @@ +package com.wallet.secure.auth.service; + +import com.wallet.secure.audit.service.AuditService; +import com.wallet.secure.user.entity.User; +import com.wallet.secure.user.repository.UserRepository; +import lombok.RequiredArgsConstructor; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Propagation; +import org.springframework.transaction.annotation.Transactional; + +import java.time.Instant; + +/** + * Persists failed-login state independently from the failed authentication flow. + * The caller rethrows BadCredentialsException, which rolls back its transaction; + * REQUIRES_NEW ensures the counter and forensic record still survive. + */ +@Service +@RequiredArgsConstructor +public class LoginAttemptService { + + private final UserRepository userRepository; + private final AuditService auditService; + + @Value("${app.security.max-login-attempts}") + private int maxLoginAttempts; + + @Value("${app.security.lockout-duration-minutes}") + private long lockoutDurationMinutes; + + @Transactional(propagation = Propagation.REQUIRES_NEW) + public void recordFailure(String email, String ipAddress, String userAgent) { + userRepository.findByEmailWithLock(email).ifPresent(user -> recordFailure(user, email, ipAddress, userAgent)); + } + + private void recordFailure(User user, String email, String ipAddress, String userAgent) { + // OWASP A07: a row lock makes each increment observe the latest count. + if (user.isAccountLocked()) { + auditService.logLoginFailure(user.getId(), email, "Account locked", ipAddress, userAgent); + return; + } + + user.incrementFailedLoginAttempts(); + boolean locked = user.getFailedLoginAttempts() >= maxLoginAttempts; + if (locked) { + user.setFailedLoginAttempts(maxLoginAttempts); + user.setLockedUntil(Instant.now().plusSeconds(lockoutDurationMinutes * 60)); + } + + auditService.logLoginFailure(user.getId(), email, + locked ? "Account locked after failed attempts" : "Bad credentials", ipAddress, userAgent); + if (locked) { + auditService.logCriticalSecurityEvent(user.getId(), + "Account locked after " + maxLoginAttempts + " failed login attempts", ipAddress, userAgent); + } + } +} diff --git a/src/main/java/com/wallet/secure/user/entity/User.java b/src/main/java/com/wallet/secure/user/entity/User.java index 3f9aacc..d1c848d 100644 --- a/src/main/java/com/wallet/secure/user/entity/User.java +++ b/src/main/java/com/wallet/secure/user/entity/User.java @@ -93,8 +93,8 @@ public class User { /** * Failed login counter — reset to 0 on successful login. - * DB constraint: CHECK (>= 0 AND <= 3). - * When reaches 3 → lockUntil is set. + * DB constraint: CHECK (>= 0 AND <= 5). + * When reaches the configured threshold → lockUntil is set. * OWASP A07: protection against brute force attacks. */ @Column(name = "failed_login_attempts", nullable = false) diff --git a/src/main/java/com/wallet/secure/user/repository/UserRepository.java b/src/main/java/com/wallet/secure/user/repository/UserRepository.java index c4607c4..e95a6a6 100644 --- a/src/main/java/com/wallet/secure/user/repository/UserRepository.java +++ b/src/main/java/com/wallet/secure/user/repository/UserRepository.java @@ -7,6 +7,9 @@ import org.springframework.data.repository.query.Param; import org.springframework.stereotype.Repository; +import jakarta.persistence.LockModeType; +import org.springframework.data.jpa.repository.Lock; + import java.time.Instant; import java.util.Optional; import java.util.UUID; @@ -39,6 +42,14 @@ public interface UserRepository extends JpaRepository { */ Optional findByEmail(String email); + /** + * Serializes failed-login accounting for one account. + * OWASP A07: concurrent guesses must not overwrite each other's counters. + */ + @Lock(LockModeType.PESSIMISTIC_WRITE) + @Query("SELECT u FROM User u WHERE u.email = :email") + Optional findByEmailWithLock(@Param("email") String email); + /** * Check if an email is already registered. * More efficient than findByEmail() — only checks existence, no data transfer. diff --git a/src/test/java/com/wallet/secure/auth/service/AuthServiceTest.java b/src/test/java/com/wallet/secure/auth/service/AuthServiceTest.java index 4a67ef9..02d5785 100644 --- a/src/test/java/com/wallet/secure/auth/service/AuthServiceTest.java +++ b/src/test/java/com/wallet/secure/auth/service/AuthServiceTest.java @@ -65,6 +65,7 @@ class AuthServiceTest { @Mock private AuditService auditService; @Mock private HttpServletRequest httpRequest; @Mock private SessionService sessionService; + @Mock private LoginAttemptService loginAttemptService; @InjectMocks private AuthService authService; @@ -194,7 +195,7 @@ void login_validCredentials_returnsTokens() { when(authenticationManager.authenticate(any(UsernamePasswordAuthenticationToken.class))) .thenReturn(mock(Authentication.class)); - when(userRepository.findByEmail(TEST_EMAIL)) + when(userRepository.findByEmailWithLock(TEST_EMAIL)) .thenReturn(Optional.of(testUser)); // WHEN @@ -218,7 +219,7 @@ void login_success_resetsFailedAttempts() { when(authenticationManager.authenticate(any(UsernamePasswordAuthenticationToken.class))) .thenReturn(mock(Authentication.class)); - when(userRepository.findByEmail(TEST_EMAIL)) + when(userRepository.findByEmailWithLock(TEST_EMAIL)) .thenReturn(Optional.of(testUser)); // WHEN @@ -241,7 +242,7 @@ void login_success_updatesLastLoginAt() { when(authenticationManager.authenticate(any(UsernamePasswordAuthenticationToken.class))) .thenReturn(mock(Authentication.class)); - when(userRepository.findByEmail(TEST_EMAIL)) + when(userRepository.findByEmailWithLock(TEST_EMAIL)) .thenReturn(Optional.of(testUser)); // WHEN @@ -251,6 +252,26 @@ void login_success_updatesLastLoginAt() { assertThat(testUser.getLastLoginAt()).isNotNull(); } + @Test + @DisplayName("rejects a user locked while authentication was in progress — OWASP A07") + void login_accountLockedAfterAuthentication_throwsLockedException() { + // GIVEN + LoginRequest request = mock(LoginRequest.class); + when(request.getEmail()).thenReturn(TEST_EMAIL); + when(request.getPassword()).thenReturn("SecurePass12!"); + when(authenticationManager.authenticate(any(UsernamePasswordAuthenticationToken.class))) + .thenReturn(mock(Authentication.class)); + testUser.setLockedUntil(Instant.now().plusSeconds(60)); + when(userRepository.findByEmailWithLock(TEST_EMAIL)).thenReturn(Optional.of(testUser)); + + // WHEN / THEN + assertThatThrownBy(() -> authService.login(request, httpRequest)) + .isInstanceOf(org.springframework.security.authentication.LockedException.class); + verify(auditService).logLoginFailure(testUser.getId(), TEST_EMAIL, + "Account locked", "127.0.0.1", "JUnit-Test-Agent"); + verify(jwtService, never()).generateAccessToken(anyString()); + } + @Test @DisplayName("propagates BadCredentialsException on wrong password — OWASP A07") void login_wrongPassword_throwsBadCredentials() { @@ -270,6 +291,8 @@ void login_wrongPassword_throwsBadCredentials() { assertThatThrownBy(() -> authService.login(request, httpRequest)) .isInstanceOf(BadCredentialsException.class); + verify(loginAttemptService).recordFailure(TEST_EMAIL, "127.0.0.1", "JUnit-Test-Agent"); + // No tokens were generated — login failed before reaching token generation verify(jwtService, never()).generateAccessToken(any()); verify(jwtService, never()).generateRefreshToken(any()); diff --git a/src/test/java/com/wallet/secure/auth/service/LoginAttemptServiceTest.java b/src/test/java/com/wallet/secure/auth/service/LoginAttemptServiceTest.java new file mode 100644 index 0000000..843aa83 --- /dev/null +++ b/src/test/java/com/wallet/secure/auth/service/LoginAttemptServiceTest.java @@ -0,0 +1,110 @@ +package com.wallet.secure.auth.service; + +import com.wallet.secure.audit.service.AuditService; +import com.wallet.secure.user.entity.User; +import com.wallet.secure.user.repository.UserRepository; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.test.util.ReflectionTestUtils; + +import java.time.Instant; +import java.util.Optional; +import java.util.UUID; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.*; + +@ExtendWith(MockitoExtension.class) +@DisplayName("LoginAttemptService") +class LoginAttemptServiceTest { + + @Mock private UserRepository userRepository; + @Mock private AuditService auditService; + @InjectMocks private LoginAttemptService loginAttemptService; + + private User user; + private final String email = "angel@test.com"; + + @BeforeEach + void setUp() { + user = User.builder().id(UUID.randomUUID()).email(email).failedLoginAttempts(0).build(); + ReflectionTestUtils.setField(loginAttemptService, "maxLoginAttempts", 5); + ReflectionTestUtils.setField(loginAttemptService, "lockoutDurationMinutes", 30L); + } + + @Nested + @DisplayName("recordFailure()") + class RecordFailureTests { + + @Test + @DisplayName("increments the known user's counter below the configured threshold") + void recordFailure_belowThreshold_incrementsCounter() { + // GIVEN + when(userRepository.findByEmailWithLock(email)).thenReturn(Optional.of(user)); + + // WHEN + loginAttemptService.recordFailure(email, "127.0.0.1", "JUnit"); + + // THEN + assertThat(user.getFailedLoginAttempts()).isEqualTo(1); + assertThat(user.getLockedUntil()).isNull(); + verify(auditService).logLoginFailure(user.getId(), email, "Bad credentials", "127.0.0.1", "JUnit"); + verify(auditService, never()).logCriticalSecurityEvent(any(), anyString(), anyString(), anyString()); + } + + @Test + @DisplayName("locks and caps the counter when the configured threshold is reached") + void recordFailure_thresholdReached_locksAccount() { + // GIVEN + user.setFailedLoginAttempts(4); + when(userRepository.findByEmailWithLock(email)).thenReturn(Optional.of(user)); + Instant before = Instant.now(); + + // WHEN + loginAttemptService.recordFailure(email, "127.0.0.1", "JUnit"); + + // THEN + assertThat(user.getFailedLoginAttempts()).isEqualTo(5); + assertThat(user.getLockedUntil()).isAfter(before.plusSeconds(1790)); + verify(auditService).logLoginFailure(user.getId(), email, + "Account locked after failed attempts", "127.0.0.1", "JUnit"); + verify(auditService).logCriticalSecurityEvent(user.getId(), + "Account locked after 5 failed login attempts", "127.0.0.1", "JUnit"); + } + + @Test + @DisplayName("does not enumerate or mutate an unknown email") + void recordFailure_unknownEmail_doesNothing() { + // GIVEN + when(userRepository.findByEmailWithLock(email)).thenReturn(Optional.empty()); + + // WHEN + loginAttemptService.recordFailure(email, "127.0.0.1", "JUnit"); + + // THEN + verifyNoInteractions(auditService); + } + + @Test + @DisplayName("does not increase a counter while an account is still locked") + void recordFailure_accountLocked_keepsCounter() { + // GIVEN + user.setFailedLoginAttempts(5); + user.setLockedUntil(Instant.now().plusSeconds(60)); + when(userRepository.findByEmailWithLock(email)).thenReturn(Optional.of(user)); + + // WHEN + loginAttemptService.recordFailure(email, "127.0.0.1", "JUnit"); + + // THEN + assertThat(user.getFailedLoginAttempts()).isEqualTo(5); + verify(auditService).logLoginFailure(user.getId(), email, "Account locked", "127.0.0.1", "JUnit"); + } + } +} From 10165cbc916ba5c105aa5828734d4473fa50845d Mon Sep 17 00:00:00 2001 From: DJAngel973 <189556164+DJAngel973@users.noreply.github.com> Date: Sat, 31 Oct 2026 19:08:24 -0500 Subject: [PATCH 2/3] fix(auth): reset expired lockout counters --- database/03-tables.sql | 2 +- docs/IMPLEMENTATION-PLAN.md | 6 +++--- .../secure/auth/service/AuthService.java | 12 +++++++----- .../auth/service/LoginAttemptService.java | 18 +++++++++++++++--- .../com/wallet/secure/user/entity/User.java | 10 ++++++---- .../secure/user/repository/UserRepository.java | 12 ++++++++---- .../auth/service/LoginAttemptServiceTest.java | 18 ++++++++++++++++++ 7 files changed, 58 insertions(+), 20 deletions(-) diff --git a/database/03-tables.sql b/database/03-tables.sql index d741f66..8c26963 100644 --- a/database/03-tables.sql +++ b/database/03-tables.sql @@ -21,7 +21,7 @@ CREATE TABLE users ( COMMENT ON TABLE users IS 'System users with 2FA authentication and lock control'; COMMENT ON COLUMN users.password_hash IS 'BCrypt hash - NEVER store passwords in plain text'; -COMMENT ON COLUMN users.locked_until IS 'NULL = unlocked, TIMESTAMP = unlocked until that date'; +COMMENT ON COLUMN users.locked_until IS 'NULL = unlocked, TIMESTAMP = locked until that date'; COMMENT ON COLUMN users.two_factor_secret IS 'Base32 secret for TOTP (Google authenticator)'; -- Table: wallets diff --git a/docs/IMPLEMENTATION-PLAN.md b/docs/IMPLEMENTATION-PLAN.md index b318074..15a93f1 100644 --- a/docs/IMPLEMENTATION-PLAN.md +++ b/docs/IMPLEMENTATION-PLAN.md @@ -231,15 +231,15 @@ Resolved in code. Failed attempts are serialized with a pessimistic user-row loc - [x] The configured number of consecutive failures locks the account. - [x] PostgreSQL accepts every valid counter state under the selected policy through the migration and schema constraint. - [x] A correct password during the lock period cannot authenticate through Spring Security's account-lock status. -- [x] Authentication succeeds after expiry through the existing time-based `isAccountLocked()` policy, then resets the counter. +- [x] Authentication succeeds after expiry through the time-based `isAccountLocked()` policy; an expired lock starts a new counter sequence and a successful login resets it to zero. - [x] Success resets failed attempts to zero. - [ ] A Docker/PostgreSQL concurrency check remains to prove that simultaneous failed attempts serialize as intended. Unit tests cover boundary behavior, unknown-email non-enumeration, locked-account state and the failure handoff. **Verification note — 2026-10-31** - `LoginAttemptService` uses `REQUIRES_NEW` and `findByEmailWithLock()` because `AuthService.login()` rethrows `BadCredentialsException`; without the independent transaction, the counter would roll back with the failed login. -- The fifth consecutive failed attempt caps the counter at five, sets `locked_until` for 30 minutes, writes a failure audit event and writes a CRITICAL lockout event. No raw credentials are logged. -- `./scripts/test.sh` passed: 117 tests, 0 failures and 0 errors. +- The fifth consecutive failed attempt caps the counter at five, sets `locked_until` for 30 minutes, writes a failure audit event and writes a CRITICAL lockout event. When that lock expires, the next failure starts again at one. No raw credentials are logged. +- `./scripts/test.sh` passed: 118 tests, 0 failures and 0 errors. ### SEC-006 — Reject JWT authentication for disabled or locked accounts diff --git a/src/main/java/com/wallet/secure/auth/service/AuthService.java b/src/main/java/com/wallet/secure/auth/service/AuthService.java index 3c6ae7c..e1336b3 100644 --- a/src/main/java/com/wallet/secure/auth/service/AuthService.java +++ b/src/main/java/com/wallet/secure/auth/service/AuthService.java @@ -163,7 +163,8 @@ private String extractUserAgent(HttpServletRequest request) { *

* OWASP A07: * - BadCredentialsException -> same message as "User not found" (prevents user enumeration) - * - Failed attempts tracked -> account lockout after 3 failures + * - Failed attempts are persisted independently -> lockout after the + * configured threshold (five by default), even though login itself fails * - LastLoginAt updated on success -> suspicious activity detection * * @param request DTO with email and password @@ -181,7 +182,8 @@ public ApiResponse login(LoginRequest request, HttpServletRequest request.getEmail(), request.getPassword())); } catch (LockedException e) { - // Account is locked - log as WARNING, not CRITICAL (already handled) + // OWASP A09: a blocked attempt is security-relevant, but the CRITICAL + // event was already recorded when the threshold originally was reached. userRepository.findByEmail(request.getEmail()).ifPresent(user -> auditService.logLoginFailure(user.getId(), request.getEmail(), @@ -205,8 +207,8 @@ public ApiResponse login(LoginRequest request, HttpServletRequest throw new LockedException("Account temporarily locked"); } - // Reset failed attempts counter + update lastLoginAt - // OWASP A07: counter reset prevents permanent lockout after legitime login + // A legitimate login closes the current failure sequence. + // OWASP A07: reset removes stale failures and clears an expired lock marker. user.resetFailedLoginAttempts(); user.setLastLoginAt(Instant.now()); @@ -238,7 +240,7 @@ public ApiResponse login(LoginRequest request, HttpServletRequest * 4. Return both replacements; the old token is now invalid * * @param request DTO with refreshToken - * @return ApiResponse with new access token (refresh token not rotated here) + * @return ApiResponse with a new access token and a rotated refresh token */ @Transactional public ApiResponse refresh(RefreshTokenRequest request, HttpServletRequest httpRequest) { diff --git a/src/main/java/com/wallet/secure/auth/service/LoginAttemptService.java b/src/main/java/com/wallet/secure/auth/service/LoginAttemptService.java index beea8cd..194d791 100644 --- a/src/main/java/com/wallet/secure/auth/service/LoginAttemptService.java +++ b/src/main/java/com/wallet/secure/auth/service/LoginAttemptService.java @@ -12,9 +12,14 @@ import java.time.Instant; /** - * Persists failed-login state independently from the failed authentication flow. - * The caller rethrows BadCredentialsException, which rolls back its transaction; - * REQUIRES_NEW ensures the counter and forensic record still survive. + * Persists the security state for a failed login independently from authentication. + * + * WHY REQUIRES_NEW: AuthService rethrows BadCredentialsException, so its + * transaction rolls back. The counter, temporary lock and audit evidence must + * survive that rollback (OWASP A07/A09). + * + * WHY the row lock: simultaneous guesses for the same account must observe the + * latest counter instead of overwriting one another. */ @Service @RequiredArgsConstructor @@ -41,6 +46,13 @@ private void recordFailure(User user, String email, String ipAddress, String use return; } + // The lock has expired: begin a new failure sequence. Without this reset, + // one wrong password after expiry would immediately lock the user again. + // OWASP A07: temporary lockouts must recover predictably for legitimate users. + if (user.getLockedUntil() != null) { + user.resetFailedLoginAttempts(); + } + user.incrementFailedLoginAttempts(); boolean locked = user.getFailedLoginAttempts() >= maxLoginAttempts; if (locked) { diff --git a/src/main/java/com/wallet/secure/user/entity/User.java b/src/main/java/com/wallet/secure/user/entity/User.java index d1c848d..605e53e 100644 --- a/src/main/java/com/wallet/secure/user/entity/User.java +++ b/src/main/java/com/wallet/secure/user/entity/User.java @@ -175,8 +175,9 @@ public class User { // ─── Business Methods ───────────────────────────────────────────────────── /** - * Checks if the account is currently locked. - * Returns true if lockedUntil is set and is in the future. + * Checks whether the temporary lock is still active. + * A past lockedUntil value no longer blocks authentication; LoginAttemptService + * clears it before starting any new failed-attempt sequence. */ public boolean isAccountLocked() { return lockedUntil != null && Instant.now().isBefore(lockedUntil); @@ -197,14 +198,15 @@ public boolean isFullyActive() { /** * Increments failed login counter. - * Called by AuthService on each failed login attempt. + * Called by LoginAttemptService while it holds the user's row lock. */ public void incrementFailedLoginAttempts() { this.failedLoginAttempts = (this.failedLoginAttempts == null ? 0 : this.failedLoginAttempts) + 1; } /** - * Resets failed login counter after successful login. + * Resets the current failed-login sequence after a successful login or after + * an expired lock is observed before a new sequence starts. */ public void resetFailedLoginAttempts() { this.failedLoginAttempts = 0; diff --git a/src/main/java/com/wallet/secure/user/repository/UserRepository.java b/src/main/java/com/wallet/secure/user/repository/UserRepository.java index e95a6a6..b3b7849 100644 --- a/src/main/java/com/wallet/secure/user/repository/UserRepository.java +++ b/src/main/java/com/wallet/secure/user/repository/UserRepository.java @@ -113,7 +113,9 @@ Optional findActiveUserByEmail( * Atomic operation — avoids race conditions on concurrent login attempts. * OWASP A07: brute force protection counter. * - * Used by: AuthService on each failed login attempt. + * Retained for future batch or administrative operations. SEC-005 uses the + * locked entity returned by findByEmailWithLock() so the threshold decision + * and update are serialized in one transaction. */ @Modifying @Query("UPDATE User u SET u.failedLoginAttempts = u.failedLoginAttempts + 1 WHERE u.id = :id") @@ -121,10 +123,11 @@ Optional findActiveUserByEmail( /** * Lock account until a specific timestamp. - * Called when failedLoginAttempts reaches MAX_FAILED_ATTEMPTS (3). + * Retained for future batch or administrative operations. SEC-005 applies + * the configured threshold while holding the user's pessimistic row lock. * OWASP A07: temporary lockout after repeated failures. * - * Used by: AuthService after 3rd consecutive failed login. + * The configured threshold is five by default (application.yml). */ @Modifying @Query("UPDATE User u SET u.lockedUntil = :until, u.failedLoginAttempts = :attempts WHERE u.id = :id") @@ -138,7 +141,8 @@ void lockAccount( * Reset failed attempts and remove lock — after successful login. * OWASP A07: clean state after legitimate access. * - * Used by: AuthService after successful authentication. + * Retained for future bulk operations; the SEC-005 entity flow resets the + * managed user after successful authentication. */ @Modifying @Query("UPDATE User u SET u.failedLoginAttempts = 0, u.lockedUntil = NULL WHERE u.id = :id") diff --git a/src/test/java/com/wallet/secure/auth/service/LoginAttemptServiceTest.java b/src/test/java/com/wallet/secure/auth/service/LoginAttemptServiceTest.java index 843aa83..d929979 100644 --- a/src/test/java/com/wallet/secure/auth/service/LoginAttemptServiceTest.java +++ b/src/test/java/com/wallet/secure/auth/service/LoginAttemptServiceTest.java @@ -106,5 +106,23 @@ void recordFailure_accountLocked_keepsCounter() { assertThat(user.getFailedLoginAttempts()).isEqualTo(5); verify(auditService).logLoginFailure(user.getId(), email, "Account locked", "127.0.0.1", "JUnit"); } + + @Test + @DisplayName("starts a new sequence after an expired lock") + void recordFailure_expiredLock_resetsCounterBeforeIncrementing() { + // GIVEN — the lock elapsed, but no successful login has occurred yet. + user.setFailedLoginAttempts(5); + user.setLockedUntil(Instant.now().minusSeconds(1)); + when(userRepository.findByEmailWithLock(email)).thenReturn(Optional.of(user)); + + // WHEN — this must be failure one in a new sequence, not failure six. + loginAttemptService.recordFailure(email, "127.0.0.1", "JUnit"); + + // THEN + assertThat(user.getFailedLoginAttempts()).isEqualTo(1); + assertThat(user.getLockedUntil()).isNull(); + verify(auditService).logLoginFailure(user.getId(), email, "Bad credentials", "127.0.0.1", "JUnit"); + verify(auditService, never()).logCriticalSecurityEvent(any(), anyString(), anyString(), anyString()); + } } } From da7fbcb9e9efc4db27769b8d06033e1442ec9402 Mon Sep 17 00:00:00 2001 From: DJAngel973 <189556164+DJAngel973@users.noreply.github.com> Date: Sat, 31 Oct 2026 19:13:56 -0500 Subject: [PATCH 3/3] docs: update PLAN --- docs/IMPLEMENTATION-PLAN.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/IMPLEMENTATION-PLAN.md b/docs/IMPLEMENTATION-PLAN.md index 15a93f1..779dc72 100644 --- a/docs/IMPLEMENTATION-PLAN.md +++ b/docs/IMPLEMENTATION-PLAN.md @@ -233,13 +233,14 @@ Resolved in code. Failed attempts are serialized with a pessimistic user-row loc - [x] A correct password during the lock period cannot authenticate through Spring Security's account-lock status. - [x] Authentication succeeds after expiry through the time-based `isAccountLocked()` policy; an expired lock starts a new counter sequence and a successful login resets it to zero. - [x] Success resets failed attempts to zero. -- [ ] A Docker/PostgreSQL concurrency check remains to prove that simultaneous failed attempts serialize as intended. Unit tests cover boundary behavior, unknown-email non-enumeration, locked-account state and the failure handoff. +- [x] Docker/PostgreSQL concurrency verification proves that simultaneous failed attempts serialize correctly. Unit tests cover boundary behavior, unknown-email non-enumeration, locked-account state and the failure handoff. **Verification note — 2026-10-31** - `LoginAttemptService` uses `REQUIRES_NEW` and `findByEmailWithLock()` because `AuthService.login()` rethrows `BadCredentialsException`; without the independent transaction, the counter would roll back with the failed login. - The fifth consecutive failed attempt caps the counter at five, sets `locked_until` for 30 minutes, writes a failure audit event and writes a CRITICAL lockout event. When that lock expires, the next failure starts again at one. No raw credentials are logged. - `./scripts/test.sh` passed: 118 tests, 0 failures and 0 errors. +- Docker/PostgreSQL verification: five simultaneous incorrect requests all returned `401`; the resulting row had `failed_login_attempts = 5` and an active `locked_until`; a following correct-password request returned `423`. ### SEC-006 — Reject JWT authentication for disabled or locked accounts