From c18fb630129bb2ada4ba767f5341462e07e33924 Mon Sep 17 00:00:00 2001 From: DJAngel973 <189556164+DJAngel973@users.noreply.github.com> Date: Tue, 3 Nov 2026 19:31:11 -0500 Subject: [PATCH] fix(authz): enforce regular-user administration boundary --- docs/IMPLEMENTATION-PLAN.md | 19 +++++++--- .../secure/audit/service/AuditService.java | 20 ++++++++++ .../exception/GlobalExceptionHandler.java | 18 ++++++++- .../secure/user/service/UserService.java | 19 +++++----- .../audit/service/AuditServiceTest.java | 26 ++++++++++++- .../secure/config/SecurityConfigTest.java | 20 ++++++++++ .../secure/user/service/UserServiceTest.java | 37 ++++++++++++++++--- 7 files changed, 136 insertions(+), 23 deletions(-) diff --git a/docs/IMPLEMENTATION-PLAN.md b/docs/IMPLEMENTATION-PLAN.md index df865fe..14512fe 100644 --- a/docs/IMPLEMENTATION-PLAN.md +++ b/docs/IMPLEMENTATION-PLAN.md @@ -291,7 +291,7 @@ Resolved. `JwtAuthFilter` now uses the reloaded `UserDetails` account state befo **Current problem** -Administrative read/deactivation operations exist, but there is no consolidated and verified administrative user-management contract. The confirmed model allows an administrator to manage regular users but not create or promote other administrators through the API. +Resolved. Administrative endpoints enforce `ROLE_ADMIN`, service rules limit cross-account deactivation to regular users, public registration fixes the `USER` role, and deactivation audit records contain immutable actor and target identifiers. **Expected behavior** @@ -311,11 +311,18 @@ Administrative read/deactivation operations exist, but there is no consolidated **Acceptance criteria** -- [ ] `USER` receives `403` for every administrative endpoint. -- [ ] `ADMIN` can manage only operations explicitly allowed for regular users. -- [ ] API requests containing privileged role changes are rejected or ignored safely and never persisted. -- [ ] No API route can create or promote another administrator. -- [ ] Administrative actions produce immutable audit records. +- [x] `USER` receives `403` for every administrative endpoint. +- [x] `ADMIN` can manage only operations explicitly allowed for regular users. +- [x] API requests containing privileged role changes are rejected or ignored safely and never persisted. +- [x] No API route can create or promote another administrator. +- [x] Administrative actions produce immutable audit records. + +**Verification note — 2026-11-02** + +- `@PreAuthorize("hasRole('ADMIN')")` returns `403` for a `USER` on both user-administration endpoints; `GlobalExceptionHandler` now maps Spring Security `AccessDeniedException` to the same consistent `ApiResponse` contract instead of a `500`. +- `UserService` accepts ADMIN deactivation of a `USER`, rejects attempts to deactivate another `ADMIN`, and registration persists the fixed `USER` role because `RegisterRequest` has no role field. +- `AuditService.logUserDeactivation()` writes `USER_DELETE` evidence with immutable actor and target UUIDs. +- `./scripts/test.sh` passed with 0 failures and 0 errors. ### AUTHZ-002 — Review wallet permissions and ownership diff --git a/src/main/java/com/wallet/secure/audit/service/AuditService.java b/src/main/java/com/wallet/secure/audit/service/AuditService.java index 2f1181e..cfd7dc3 100644 --- a/src/main/java/com/wallet/secure/audit/service/AuditService.java +++ b/src/main/java/com/wallet/secure/audit/service/AuditService.java @@ -147,6 +147,26 @@ public void logRegister(UUID userId, String email, String ipAddress, String user details, ipAddress, userAgent)); } + /** + * Records a successful account deactivation with both actor and target IDs. + * + * WHY userId is the actor: audit_logs.user_id identifies who performed the + * sensitive operation; targetUserId remains in immutable JSONB details for + * an administrator's forensic timeline (OWASP A01/A09). + */ + @Async + @Transactional(propagation = Propagation.REQUIRES_NEW) + public void logUserDeactivation(UUID actorUserId, UUID targetUserId, boolean selfService) { + String details = buildDetails( + "\"actorUserId\":\"" + actorUserId + "\"", + "\"targetUserId\":\"" + targetUserId + "\"", + "\"mode\":\"" + (selfService ? "SELF_SERVICE" : "ADMIN") + "\"", + "\"outcome\":\"SUCCESS\"" + ); + save(AuditLog.success(actorUserId, AuditAction.USER_DELETE, LogSeverity.WARNING, + details, null, null)); + } + // ─── Transaction Events /** diff --git a/src/main/java/com/wallet/secure/common/exception/GlobalExceptionHandler.java b/src/main/java/com/wallet/secure/common/exception/GlobalExceptionHandler.java index fb9a263..c4ac8b5 100644 --- a/src/main/java/com/wallet/secure/common/exception/GlobalExceptionHandler.java +++ b/src/main/java/com/wallet/secure/common/exception/GlobalExceptionHandler.java @@ -5,6 +5,7 @@ import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.validation.FieldError; +import org.springframework.security.access.AccessDeniedException; import org.springframework.web.bind.MethodArgumentNotValidException; import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.RestControllerAdvice; @@ -106,7 +107,7 @@ public ResponseEntity> handleInvalidCredentials( /** * 423 — Account locked after too many failed attempts. - * Triggered by: AuthService (future) when failedLoginAttempts >= 3. + * Triggered by: AuthService when failedLoginAttempts reaches the configured threshold. * OWASP A07: brute force protection. */ @ExceptionHandler(AccountLockedException.class) @@ -132,6 +133,21 @@ public ResponseEntity> handleUnauthorizedOperation( .body(ApiResponse.error(ex.getMessage())); } + /** + * 403 — Spring Security method authorization rejected an authenticated user. + * Triggered before a @PreAuthorize-protected controller method executes. + * + * OWASP A01: return a consistent denial response instead of letting the + * generic handler turn an authorization denial into a misleading 500. + */ + @ExceptionHandler(AccessDeniedException.class) + public ResponseEntity> handleAccessDenied(AccessDeniedException ex) { + log.warn("Access denied by Spring Security"); + return ResponseEntity + .status(HttpStatus.FORBIDDEN) + .body(ApiResponse.error("Access denied")); + } + /** * 423 — Account locked by Spring Security (LockedException). * Thrown by Spring Security when UserDetails.isAccountLocked() = true. diff --git a/src/main/java/com/wallet/secure/user/service/UserService.java b/src/main/java/com/wallet/secure/user/service/UserService.java index 4d13327..e6b8328 100644 --- a/src/main/java/com/wallet/secure/user/service/UserService.java +++ b/src/main/java/com/wallet/secure/user/service/UserService.java @@ -40,9 +40,6 @@ @Log4j2 public class UserService { - private static final int MAX_FAILED_ATTEMPTS = 3; - private static final int LOCK_DURATION_MINUTES = 30; - private final UserRepository userRepository; private final PasswordEncoder passwordEncoder; private final AuditService auditService; @@ -197,6 +194,14 @@ public ApiResponse deactivateAccount(UUID userId, UUID requesterId) { requesterId, userId); throw new UnauthorizedOperationException("Not authorized to deactivate this account"); } + + // OWASP A01: administrators may manage regular accounts only. An API + // cannot be used to alter another privileged account, even by ADMIN. + if (UserRole.ADMIN.equals(user.getRole())) { + log.warn("Blocked attempt to deactivate another ADMIN: requesterId={} targetUserId={}", + requesterId, userId); + throw new UnauthorizedOperationException("Not authorized to deactivate this account"); + } } user.setIsActive(false); @@ -207,12 +212,8 @@ public ApiResponse deactivateAccount(UUID userId, UUID requesterId) { // token on its next request using the reloaded isActive state. sessionService.revokeAllSessions(userId); - // OWASP A09: account deactivation is a high-sensitivity operation — always audited - auditService.logCriticalSecurityEvent( - userId, - "Account deactivated by " + (userId.equals(requesterId) ? "owner" : "ADMIN requesterId=" + requesterId), - null, null - ); + // OWASP A09: immutable audit evidence records both actor and target. + auditService.logUserDeactivation(requesterId, userId, isSelf); log.info("Account deactivated: userId={} by requesterId={}", userId, requesterId); return ApiResponse.ok("Account deactivated successfully"); diff --git a/src/test/java/com/wallet/secure/audit/service/AuditServiceTest.java b/src/test/java/com/wallet/secure/audit/service/AuditServiceTest.java index dcadd17..05a89c5 100644 --- a/src/test/java/com/wallet/secure/audit/service/AuditServiceTest.java +++ b/src/test/java/com/wallet/secure/audit/service/AuditServiceTest.java @@ -68,6 +68,30 @@ void setUp() { // ─── Authentication Events + @Nested + @DisplayName("logUserDeactivation()") + class LogUserDeactivationTests { + + @Test + @DisplayName("records actor and target IDs as immutable USER_DELETE evidence") + void logUserDeactivation_recordsActorAndTarget() { + UUID targetUserId = UUID.randomUUID(); + + auditService.logUserDeactivation(userId, targetUserId, false); + + ArgumentCaptor captor = ArgumentCaptor.forClass(AuditLog.class); + verify(auditLogRepository).save(captor.capture()); + AuditLog saved = captor.getValue(); + assertThat(saved.getUserId()).isEqualTo(userId); + assertThat(saved.getAction()).isEqualTo(AuditAction.USER_DELETE); + assertThat(saved.getSeverityLevel()).isEqualTo(LogSeverity.WARNING); + assertThat(saved.getDetails()) + .contains("\"actorUserId\":\"" + userId + "\"") + .contains("\"targetUserId\":\"" + targetUserId + "\"") + .contains("\"mode\":\"ADMIN\""); + } + } + @Nested @DisplayName("logLoginSuccess()") class LogLoginSuccessTests { @@ -500,4 +524,4 @@ void countRecentFailedLogins_passesCorrectTimeWindow() { assertThat(since).isBefore(Instant.now()); } } -} \ No newline at end of file +} diff --git a/src/test/java/com/wallet/secure/config/SecurityConfigTest.java b/src/test/java/com/wallet/secure/config/SecurityConfigTest.java index 667f66c..23f6f00 100644 --- a/src/test/java/com/wallet/secure/config/SecurityConfigTest.java +++ b/src/test/java/com/wallet/secure/config/SecurityConfigTest.java @@ -7,8 +7,12 @@ import org.springframework.boot.test.context.SpringBootTest; import org.springframework.test.context.ActiveProfiles; import org.springframework.test.web.servlet.MockMvc; +import org.springframework.security.test.context.support.WithMockUser; + +import java.util.UUID; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; /** @@ -43,4 +47,20 @@ void actuatorHealth_isPublic() throws Exception { mockMvc.perform(get("/actuator/health")) .andExpect(status().isOk()); } + + @Test + @WithMockUser(roles = "USER") + @DisplayName("Regular user receives 403 from an ADMIN user-management endpoint") + void adminUserEndpoint_regularUser_returns403() throws Exception { + mockMvc.perform(get("/users/{id}", UUID.randomUUID())) + .andExpect(status().isForbidden()); + } + + @Test + @WithMockUser(roles = "USER") + @DisplayName("Regular user receives 403 when trying to deactivate another account") + void adminDeactivationEndpoint_regularUser_returns403() throws Exception { + mockMvc.perform(delete("/users/{id}", UUID.randomUUID())) + .andExpect(status().isForbidden()); + } } diff --git a/src/test/java/com/wallet/secure/user/service/UserServiceTest.java b/src/test/java/com/wallet/secure/user/service/UserServiceTest.java index 27a8982..70fe0b2 100644 --- a/src/test/java/com/wallet/secure/user/service/UserServiceTest.java +++ b/src/test/java/com/wallet/secure/user/service/UserServiceTest.java @@ -19,6 +19,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.InjectMocks; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import org.springframework.security.crypto.password.PasswordEncoder; @@ -112,7 +113,10 @@ void shouldRegisterUserSuccessfully() { // Verify BCrypt was called — password must be hashed verify(passwordEncoder).encode("SecurePass12!"); // Verify user was saved to DB - verify(userRepository).save(any(User.class)); + ArgumentCaptor savedUser = ArgumentCaptor.forClass(User.class); + verify(userRepository).save(savedUser.capture()); + // OWASP A01: RegisterRequest has no role field and service fixes USER. + assertThat(savedUser.getValue().getRole()).isEqualTo(UserRole.USER); } /** @@ -309,8 +313,7 @@ void shouldDeactivateOwnAccount() { assertThat(response.isSuccess()).isTrue(); verify(userRepository).save(argThat(u -> !u.getIsActive())); verify(sessionService).revokeAllSessions(testUserId); - // OWASP A09: deactivation is a critical event — must be audited - verify(auditService).logCriticalSecurityEvent(eq(testUserId), anyString(), isNull(), isNull()); + verify(auditService).logUserDeactivation(testUserId, testUserId, true); } @Test @@ -337,7 +340,7 @@ void shouldAllowAdminToDeactivateAnotherUser() { assertThat(response.isSuccess()).isTrue(); verify(userRepository).save(argThat(u -> !u.getIsActive())); verify(sessionService).revokeAllSessions(testUserId); - verify(auditService).logCriticalSecurityEvent(eq(testUserId), anyString(), isNull(), isNull()); + verify(auditService).logUserDeactivation(adminId, testUserId, false); } @Test @@ -361,8 +364,30 @@ void shouldThrowWhenNonAdminTriesToDeactivateAnotherUser() { // Account must NOT have been deactivated verify(userRepository, never()).save(any()); - // No audit log for a blocked attempt - verify(auditService, never()).logCriticalSecurityEvent(any(), any(), any(), any()); + verify(auditService, never()).logUserDeactivation(any(), any(), anyBoolean()); + } + + @Test + @DisplayName("Should reject an ADMIN trying to deactivate another ADMIN") + void shouldRejectAdminDeactivationOfAnotherAdmin() { + // GIVEN + UUID requesterId = UUID.randomUUID(); + User targetAdmin = User.builder().email("target-admin@test.com") + .passwordHash("$2a$12$targetAdminHash").role(UserRole.ADMIN).build(); + targetAdmin.setId(testUserId); + User requesterAdmin = User.builder().email("requester-admin@test.com") + .passwordHash("$2a$12$requesterAdminHash").role(UserRole.ADMIN).build(); + requesterAdmin.setId(requesterId); + when(userRepository.findById(testUserId)).thenReturn(Optional.of(targetAdmin)); + when(userRepository.findById(requesterId)).thenReturn(Optional.of(requesterAdmin)); + + // WHEN / THEN + assertThatThrownBy(() -> userService.deactivateAccount(testUserId, requesterId)) + .isInstanceOf(UnauthorizedOperationException.class) + .hasMessage("Not authorized to deactivate this account"); + verify(userRepository, never()).save(any()); + verify(sessionService, never()).revokeAllSessions(any()); + verify(auditService, never()).logUserDeactivation(any(), any(), anyBoolean()); } }