From 89f4f0babcde13cec7385c3908007306c3816448 Mon Sep 17 00:00:00 2001 From: DJAngel973 <189556164+DJAngel973@users.noreply.github.com> Date: Thu, 5 Nov 2026 21:34:17 -0500 Subject: [PATCH 1/2] fix(authz): restrict account deactivation to administrators --- database/02-types.sql | 1 + database/08-migrations.sql | 6 ++ docs/IMPLEMENTATION-PLAN.md | 27 +++++---- .../secure/audit/service/AuditService.java | 9 ++- .../secure/common/enums/AuditAction.java | 1 + .../exception/GlobalExceptionHandler.java | 17 ++++++ .../user/controller/UserController.java | 29 +++------- .../secure/user/service/UserService.java | 41 ++++++------- .../audit/service/AuditServiceTest.java | 8 ++- .../secure/config/SecurityConfigTest.java | 57 ++++++++++++++++++- .../secure/user/service/UserServiceTest.java | 29 +++++----- 11 files changed, 147 insertions(+), 78 deletions(-) diff --git a/database/02-types.sql b/database/02-types.sql index d01bce2..d8e5aad 100644 --- a/database/02-types.sql +++ b/database/02-types.sql @@ -31,6 +31,7 @@ CREATE TYPE audit_action AS ENUM ( 'USER_REGISTER', 'USER_UPDATE', 'USER_DELETE', + 'ACCOUNT_DEACTIVATE', 'WALLET_CREATE', 'WALLET_SUSPEND', 'WALLET_CLOSE', diff --git a/database/08-migrations.sql b/database/08-migrations.sql index d450954..c7bb416 100644 --- a/database/08-migrations.sql +++ b/database/08-migrations.sql @@ -35,3 +35,9 @@ ALTER TABLE users ALTER TABLE users ADD CONSTRAINT users_failed_login_attempts_range CHECK (failed_login_attempts >= 0 AND failed_login_attempts <= 5); + +-- Migration: represent an administrative account block without implying deletion +-- Date: 2026-11-03 +-- OWASP A09: financial accounts are retained; audit evidence must distinguish +-- an is_active=false state change from a hard delete. +ALTER TYPE audit_action ADD VALUE IF NOT EXISTS 'ACCOUNT_DEACTIVATE'; diff --git a/docs/IMPLEMENTATION-PLAN.md b/docs/IMPLEMENTATION-PLAN.md index 14512fe..3ad2536 100644 --- a/docs/IMPLEMENTATION-PLAN.md +++ b/docs/IMPLEMENTATION-PLAN.md @@ -291,11 +291,11 @@ Resolved. `JwtAuthFilter` now uses the reloaded `UserDetails` account state befo **Current problem** -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. +Resolved. Administrative endpoints enforce `ROLE_ADMIN`; only an ADMIN can deactivate a regular account through `PATCH /users/{id}/deactivate`. The operation changes `isActive` and revokes sessions without deleting financial data, while audit records contain immutable actor and target identifiers. **Expected behavior** -- `ADMIN` can perform explicitly approved management operations on `USER` accounts. +- `ADMIN` can deactivate (block) explicitly approved `USER` accounts without deleting them. - `USER` cannot invoke administrative operations. - No public or administrative API operation creates, promotes, or modifies another `ADMIN`. - Every administrative action is audited with actor and target identifiers. @@ -320,8 +320,8 @@ Resolved. Administrative endpoints enforce `ROLE_ADMIN`, service rules limit cro **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. +- `UserService` accepts ADMIN deactivation of a `USER`, rejects self-service and ADMIN-to-ADMIN deactivation, and registration persists the fixed `USER` role because `RegisterRequest` has no role field. +- `AuditService.logUserDeactivation()` writes `ACCOUNT_DEACTIVATE` evidence with immutable actor and target UUIDs in the same transaction as the state change. - `./scripts/test.sh` passed with 0 failures and 0 errors. ### AUTHZ-002 — Review wallet permissions and ownership @@ -330,9 +330,9 @@ Resolved. Administrative endpoints enforce `ROLE_ADMIN`, service rules limit cro **Priority:** P1 **Depends on:** AUTHZ-001 -**Current problem** +**Current problem — resolved** -Wallet controllers derive identity from the JWT and services include ownership patterns, but complete endpoint-by-endpoint authorization evidence is missing. +Wallet ownership already came from the authenticated principal and ownership lookups use non-enumerating queries. AUTHZ-002 now also proves the route matrix and preserves the administrator (rather than the wallet owner) as the actor in lifecycle audit records. **Expected behavior** @@ -352,10 +352,17 @@ Wallet controllers derive identity from the JWT and services include ownership p **Acceptance criteria** -- [ ] User A cannot read, debit, close, suspend, or restore User B's wallet. -- [ ] User-controlled input cannot assign a wallet to a different owner. -- [ ] Administrative actions require `ROLE_ADMIN` and generate audit records. -- [ ] Automated tests cover every wallet endpoint with owner, non-owner, anonymous, and admin actors. +- [x] User A cannot read, debit, close, suspend, or restore User B's wallet. +- [x] User-controlled input cannot assign a wallet to a different owner. +- [x] Administrative actions require `ROLE_ADMIN` and generate audit records. +- [x] Automated tests cover every wallet endpoint with owner, non-owner, anonymous, and admin actors. + +**Verification note — 2026-11-03** + +- Ownership-sensitive wallet reads and transaction validation use `findByIdAndUserId`, returning the same `404` for a nonexistent wallet and another user's wallet; controllers never accept an owner ID in request input. +- The create DTO exposes only `currency`; the service obtains the owner from the JWT-derived ID and forces balance/status server-side. +- `@PreAuthorize("hasRole('ADMIN')")` blocks a `USER` with `403` on suspend, restore, and close. Anonymous calls to every wallet route return `401`; an `ADMIN` passes the lifecycle authorization gate. +- Lifecycle audit events now identify the authenticated administrator as the actor and retain the affected `walletId` in immutable audit details. ### TX-001 — Verify transactions, concurrency, rollback, and audit behavior 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 cfd7dc3..a2048e3 100644 --- a/src/main/java/com/wallet/secure/audit/service/AuditService.java +++ b/src/main/java/com/wallet/secure/audit/service/AuditService.java @@ -154,16 +154,15 @@ public void logRegister(UUID userId, String email, String ipAddress, String user * 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) { + @Transactional(propagation = Propagation.MANDATORY) + public void logUserDeactivation(UUID actorUserId, UUID targetUserId) { String details = buildDetails( "\"actorUserId\":\"" + actorUserId + "\"", "\"targetUserId\":\"" + targetUserId + "\"", - "\"mode\":\"" + (selfService ? "SELF_SERVICE" : "ADMIN") + "\"", + "\"mode\":\"ADMIN\"", "\"outcome\":\"SUCCESS\"" ); - save(AuditLog.success(actorUserId, AuditAction.USER_DELETE, LogSeverity.WARNING, + save(AuditLog.success(actorUserId, AuditAction.ACCOUNT_DEACTIVATE, LogSeverity.WARNING, details, null, null)); } diff --git a/src/main/java/com/wallet/secure/common/enums/AuditAction.java b/src/main/java/com/wallet/secure/common/enums/AuditAction.java index dbef455..8d4eca9 100644 --- a/src/main/java/com/wallet/secure/common/enums/AuditAction.java +++ b/src/main/java/com/wallet/secure/common/enums/AuditAction.java @@ -21,6 +21,7 @@ public enum AuditAction { USER_REGISTER, USER_UPDATE, USER_DELETE, + ACCOUNT_DEACTIVATE, // Wallet lifecycle WALLET_CREATE, 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 c4ac8b5..8a2253a 100644 --- a/src/main/java/com/wallet/secure/common/exception/GlobalExceptionHandler.java +++ b/src/main/java/com/wallet/secure/common/exception/GlobalExceptionHandler.java @@ -9,6 +9,7 @@ import org.springframework.web.bind.MethodArgumentNotValidException; import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.RestControllerAdvice; +import org.springframework.web.HttpRequestMethodNotSupportedException; import java.util.HashMap; import java.util.Map; @@ -215,6 +216,22 @@ public ResponseEntity>> handleValidationErrors( .body(ApiResponse.error("Validation failed", errors)); } + /** + * 405 — The URL exists but does not support the requested HTTP method. + * + * WHY this is explicit: removing a destructive DELETE route must not make + * clients see a misleading 500 through the generic fallback. The API keeps + * its response contract while making the absence of deletion intentional. + */ + @ExceptionHandler(HttpRequestMethodNotSupportedException.class) + public ResponseEntity> handleMethodNotSupported( + HttpRequestMethodNotSupportedException ex) { + log.warn("Unsupported HTTP method: {}", ex.getMethod()); + return ResponseEntity + .status(HttpStatus.METHOD_NOT_ALLOWED) + .body(ApiResponse.error("HTTP method not allowed")); + } + // ─── Generic Fallback ───────────────────────────────────────────────────── /** diff --git a/src/main/java/com/wallet/secure/user/controller/UserController.java b/src/main/java/com/wallet/secure/user/controller/UserController.java index 83ce46a..a04c103 100644 --- a/src/main/java/com/wallet/secure/user/controller/UserController.java +++ b/src/main/java/com/wallet/secure/user/controller/UserController.java @@ -92,30 +92,19 @@ public ResponseEntity> updateMyProfile( return ResponseEntity.ok(userService.updateProfile(userId, request)); } - // ─── Account Deactivation ───────────────────────────────────────────────── + // ─── Administrative Account Deactivation ────────────────────────────────── /** - * DELETE /users/me - * Soft-deactivates the authenticated user's account. - * isActive = false — data preserved for audit compliance. + * PATCH /users/{id}/deactivate + * Deactivates (blocks) an account; it never deletes its financial data. * - * HTTP 200 with message — not 204, because ApiResponse carries confirmation. - * OWASP A09: data retention for financial compliance. + * A bank-like wallet must retain users, wallets, transactions and audit + * evidence. Therefore this changes only isActive to false and revokes + * sessions; only an ADMIN can execute it. PATCH expresses a state change, + * unlike DELETE, which would misleadingly imply resource removal. + * OWASP A01/A09: ADMIN-only operation with preserved forensic evidence. */ - @DeleteMapping("/me") - public ResponseEntity> deactivateMyAccount( - @AuthenticationPrincipal UserDetails userDetails) { - - UUID userId = resolveUserId(userDetails); - return ResponseEntity.ok(userService.deactivateAccount(userId, userId)); - } - - /** - * DELETE /users/{id} - * Admin deactivates any user's account. - * OWASP A01: ADMIN role required. - */ - @DeleteMapping("/{id}") + @PatchMapping("/{id}/deactivate") @PreAuthorize("hasRole('ADMIN')") public ResponseEntity> deactivateUser( @PathVariable UUID id, 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 e6b8328..24f4d4f 100644 --- a/src/main/java/com/wallet/secure/user/service/UserService.java +++ b/src/main/java/com/wallet/secure/user/service/UserService.java @@ -175,33 +175,26 @@ public ApiResponse updateProfile(UUID userId, UpdateProfileRequest * - OWASP A09: audit trail must be preserved * * @param userId ID of user to deactivate - * @param requesterId ID of who is requesting (must be same user or ADMIN) + * @param requesterId authenticated ADMIN performing the action */ @Transactional public ApiResponse deactivateAccount(UUID userId, UUID requesterId) { User user = findUserById(userId); - // OWASP A01: only the account owner OR an ADMIN can deactivate an account. - // WHY we also load the requester: comparing UUIDs alone is not enough — - // an ADMIN will always have a different UUID than the target user. - // We must verify the requester's role to allow cross-user deactivation. - boolean isSelf = userId.equals(requesterId); - if (!isSelf) { - User requester = findUserById(requesterId); - boolean isAdmin = UserRole.ADMIN.equals(requester.getRole()); - if (!isAdmin) { - log.warn("Unauthorized deactivation attempt: requesterId={} tried userId={}", - requesterId, userId); - throw new UnauthorizedOperationException("Not authorized to deactivate this account"); - } + // Defense in depth: @PreAuthorize blocks this at the HTTP boundary, and + // this service check also protects future internal callers (OWASP A01). + User requester = findUserById(requesterId); + if (!UserRole.ADMIN.equals(requester.getRole())) { + log.warn("Unauthorized account deactivation attempt: requesterId={} targetUserId={}", + 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"); - } + // Privileged accounts cannot be changed through this management route. + if (UserRole.ADMIN.equals(user.getRole())) { + log.warn("Blocked ADMIN-to-ADMIN deactivation: requesterId={} targetUserId={}", + requesterId, userId); + throw new UnauthorizedOperationException("Not authorized to deactivate this account"); } user.setIsActive(false); @@ -212,8 +205,10 @@ public ApiResponse deactivateAccount(UUID userId, UUID requesterId) { // token on its next request using the reloaded isActive state. sessionService.revokeAllSessions(userId); - // OWASP A09: immutable audit evidence records both actor and target. - auditService.logUserDeactivation(requesterId, userId, isSelf); + // OWASP A09: this audit joins the state-change transaction. Returning a + // success response without durable evidence is unacceptable for an + // administrative account block. + auditService.logUserDeactivation(requesterId, userId); 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 05a89c5..3494285 100644 --- a/src/test/java/com/wallet/secure/audit/service/AuditServiceTest.java +++ b/src/test/java/com/wallet/secure/audit/service/AuditServiceTest.java @@ -73,17 +73,17 @@ void setUp() { class LogUserDeactivationTests { @Test - @DisplayName("records actor and target IDs as immutable USER_DELETE evidence") + @DisplayName("records actor and target IDs as immutable ACCOUNT_DEACTIVATE evidence") void logUserDeactivation_recordsActorAndTarget() { UUID targetUserId = UUID.randomUUID(); - auditService.logUserDeactivation(userId, targetUserId, false); + auditService.logUserDeactivation(userId, targetUserId); 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.getAction()).isEqualTo(AuditAction.ACCOUNT_DEACTIVATE); assertThat(saved.getSeverityLevel()).isEqualTo(LogSeverity.WARNING); assertThat(saved.getDetails()) .contains("\"actorUserId\":\"" + userId + "\"") @@ -367,9 +367,11 @@ void logWalletSuspended_savesCorrectActionAndSeverity() { verify(auditLogRepository).save(captor.capture()); AuditLog saved = captor.getValue(); + assertThat(saved.getUserId()).isEqualTo(userId); assertThat(saved.getAction()).isEqualTo(AuditAction.WALLET_SUSPEND); // WARNING — suspension is a security-relevant admin action assertThat(saved.getSeverityLevel()).isEqualTo(LogSeverity.WARNING); + assertThat(saved.getDetails()).contains("\"adminId\":\"" + userId + "\""); } } diff --git a/src/test/java/com/wallet/secure/config/SecurityConfigTest.java b/src/test/java/com/wallet/secure/config/SecurityConfigTest.java index 23f6f00..8a9c08d 100644 --- a/src/test/java/com/wallet/secure/config/SecurityConfigTest.java +++ b/src/test/java/com/wallet/secure/config/SecurityConfigTest.java @@ -1,11 +1,13 @@ package com.wallet.secure.config; +import com.wallet.secure.wallet.service.WalletService; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.test.context.ActiveProfiles; +import org.springframework.test.context.bean.override.mockito.MockitoBean; import org.springframework.test.web.servlet.MockMvc; import org.springframework.security.test.context.support.WithMockUser; @@ -13,6 +15,8 @@ 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.request.MockMvcRequestBuilders.patch; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; /** @@ -33,6 +37,11 @@ class SecurityConfigTest { @Autowired private MockMvc mockMvc; + // The filter chain is the subject under test. Mocking business logic keeps + // authorization evidence independent from database state. + @MockitoBean + private WalletService walletService; + @Test @DisplayName("Protected endpoint returns 401 when no token provided") void protectedEndpoint_withoutToken_returns401() throws Exception { @@ -41,6 +50,42 @@ void protectedEndpoint_withoutToken_returns401() throws Exception { .andExpect(status().isUnauthorized()); } + @Test + @DisplayName("Every wallet route returns 401 when anonymous") + void walletEndpoints_withoutToken_return401() throws Exception { + UUID walletId = UUID.randomUUID(); + + mockMvc.perform(post("/wallets").contentType("application/json").content("{\"currency\":\"USD\"}")) + .andExpect(status().isUnauthorized()); + mockMvc.perform(get("/wallets/{id}", walletId)).andExpect(status().isUnauthorized()); + mockMvc.perform(get("/wallets/currency/USD")).andExpect(status().isUnauthorized()); + mockMvc.perform(patch("/wallets/{id}/suspend", walletId)).andExpect(status().isUnauthorized()); + mockMvc.perform(patch("/wallets/{id}/restore", walletId)).andExpect(status().isUnauthorized()); + mockMvc.perform(patch("/wallets/{id}/close", walletId)).andExpect(status().isUnauthorized()); + } + + @Test + @WithMockUser(roles = "USER") + @DisplayName("Regular user receives 403 from every administrative wallet route") + void administrativeWalletEndpoints_regularUser_return403() throws Exception { + UUID walletId = UUID.randomUUID(); + + mockMvc.perform(patch("/wallets/{id}/suspend", walletId)).andExpect(status().isForbidden()); + mockMvc.perform(patch("/wallets/{id}/restore", walletId)).andExpect(status().isForbidden()); + mockMvc.perform(patch("/wallets/{id}/close", walletId)).andExpect(status().isForbidden()); + } + + @Test + @WithMockUser(roles = "ADMIN") + @DisplayName("Administrator passes authorization for every wallet lifecycle route") + void administrativeWalletEndpoints_admin_passesAuthorization() throws Exception { + UUID walletId = UUID.randomUUID(); + + mockMvc.perform(patch("/wallets/{id}/suspend", walletId)).andExpect(status().isOk()); + mockMvc.perform(patch("/wallets/{id}/restore", walletId)).andExpect(status().isOk()); + mockMvc.perform(patch("/wallets/{id}/close", walletId)).andExpect(status().isOk()); + } + @Test @DisplayName("Health actuator is publicly accessible") void actuatorHealth_isPublic() throws Exception { @@ -58,9 +103,17 @@ void adminUserEndpoint_regularUser_returns403() throws Exception { @Test @WithMockUser(roles = "USER") - @DisplayName("Regular user receives 403 when trying to deactivate another account") + @DisplayName("Regular user receives 403 when trying to deactivate an account") void adminDeactivationEndpoint_regularUser_returns403() throws Exception { - mockMvc.perform(delete("/users/{id}", UUID.randomUUID())) + mockMvc.perform(patch("/users/{id}/deactivate", UUID.randomUUID())) .andExpect(status().isForbidden()); } + + @Test + @WithMockUser(roles = "USER") + @DisplayName("No DELETE route exists for user account removal") + void deleteAccountRoute_returnsMethodNotAllowed() throws Exception { + mockMvc.perform(delete("/users/me")) + .andExpect(status().isMethodNotAllowed()); + } } 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 70fe0b2..ed26f5b 100644 --- a/src/test/java/com/wallet/secure/user/service/UserServiceTest.java +++ b/src/test/java/com/wallet/secure/user/service/UserServiceTest.java @@ -300,20 +300,19 @@ void shouldThrowWhenNewPasswordSameAsCurrent() { class DeactivateTests { @Test - @DisplayName("Should deactivate own account successfully") - void shouldDeactivateOwnAccount() { - // GIVEN — user deactivates themselves (userId == requesterId) + @DisplayName("Should reject a regular user attempting to deactivate their own account") + void shouldRejectSelfServiceDeactivationByRegularUser() { + // GIVEN — no self-service endpoint exists, and the service enforces + // the same bank-style rule for any future internal caller. when(userRepository.findById(testUserId)).thenReturn(Optional.of(testUser)); - when(userRepository.save(any(User.class))).thenReturn(testUser); - - // WHEN - ApiResponse response = userService.deactivateAccount(testUserId, testUserId); - // THEN - assertThat(response.isSuccess()).isTrue(); - verify(userRepository).save(argThat(u -> !u.getIsActive())); - verify(sessionService).revokeAllSessions(testUserId); - verify(auditService).logUserDeactivation(testUserId, testUserId, true); + // WHEN / THEN + assertThatThrownBy(() -> userService.deactivateAccount(testUserId, testUserId)) + .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()); } @Test @@ -340,7 +339,7 @@ void shouldAllowAdminToDeactivateAnotherUser() { assertThat(response.isSuccess()).isTrue(); verify(userRepository).save(argThat(u -> !u.getIsActive())); verify(sessionService).revokeAllSessions(testUserId); - verify(auditService).logUserDeactivation(adminId, testUserId, false); + verify(auditService).logUserDeactivation(adminId, testUserId); } @Test @@ -364,7 +363,7 @@ void shouldThrowWhenNonAdminTriesToDeactivateAnotherUser() { // Account must NOT have been deactivated verify(userRepository, never()).save(any()); - verify(auditService, never()).logUserDeactivation(any(), any(), anyBoolean()); + verify(auditService, never()).logUserDeactivation(any(), any()); } @Test @@ -387,7 +386,7 @@ void shouldRejectAdminDeactivationOfAnotherAdmin() { .hasMessage("Not authorized to deactivate this account"); verify(userRepository, never()).save(any()); verify(sessionService, never()).revokeAllSessions(any()); - verify(auditService, never()).logUserDeactivation(any(), any(), anyBoolean()); + verify(auditService, never()).logUserDeactivation(any(), any()); } } From 1bc6c7cbfc8ba330c97220ae94005af667a63e2c Mon Sep 17 00:00:00 2001 From: DJAngel973 <189556164+DJAngel973@users.noreply.github.com> Date: Thu, 5 Nov 2026 21:35:05 -0500 Subject: [PATCH 2/2] fix(authz): enforce wallet ownership and lifecycle audit --- .../wallet/controller/WalletController.java | 15 ++++--- .../secure/wallet/service/WalletService.java | 20 ++++++---- .../wallet/service/WalletServiceTest.java | 40 +++++++++++++++---- 3 files changed, 55 insertions(+), 20 deletions(-) diff --git a/src/main/java/com/wallet/secure/wallet/controller/WalletController.java b/src/main/java/com/wallet/secure/wallet/controller/WalletController.java index 31aad1e..de332bc 100644 --- a/src/main/java/com/wallet/secure/wallet/controller/WalletController.java +++ b/src/main/java/com/wallet/secure/wallet/controller/WalletController.java @@ -120,9 +120,10 @@ public ResponseEntity> getMyWalletByCurrency(@PathVa */ @PatchMapping("/{id}/suspend") @PreAuthorize("hasRole('ADMIN')") - public ResponseEntity> suspendWallet(@PathVariable UUID id) { + public ResponseEntity> suspendWallet(@PathVariable UUID id, + @AuthenticationPrincipal UserDetails userDetails) { - return ResponseEntity.ok(walletService.suspendWallet(id)); + return ResponseEntity.ok(walletService.suspendWallet(id, resolveUserId(userDetails))); } /** @@ -132,9 +133,10 @@ public ResponseEntity> suspendWallet(@PathVariable U */ @PatchMapping("/{id}/restore") @PreAuthorize("hasRole('ADMIN')") - public ResponseEntity> restoreWallet(@PathVariable UUID id) { + public ResponseEntity> restoreWallet(@PathVariable UUID id, + @AuthenticationPrincipal UserDetails userDetails) { - return ResponseEntity.ok(walletService.restoreWallet(id)); + return ResponseEntity.ok(walletService.restoreWallet(id, resolveUserId(userDetails))); } /** @@ -146,9 +148,10 @@ public ResponseEntity> restoreWallet(@PathVariable U */ @PatchMapping("/{id}/close") @PreAuthorize("hasRole('ADMIN')") - public ResponseEntity> closeWallet(@PathVariable UUID id) { + public ResponseEntity> closeWallet(@PathVariable UUID id, + @AuthenticationPrincipal UserDetails userDetails) { - return ResponseEntity.ok(walletService.closeWallet(id)); + return ResponseEntity.ok(walletService.closeWallet(id, resolveUserId(userDetails))); } // --- Private Helper diff --git a/src/main/java/com/wallet/secure/wallet/service/WalletService.java b/src/main/java/com/wallet/secure/wallet/service/WalletService.java index 6c160e1..36e2968 100644 --- a/src/main/java/com/wallet/secure/wallet/service/WalletService.java +++ b/src/main/java/com/wallet/secure/wallet/service/WalletService.java @@ -164,10 +164,11 @@ public ApiResponse getMyWalletByCurrency(UUID userId, CurrencyCo * OWASP A01: @PreAuthorize("hasROle('ADMIN')") enforced at controller level. * * @param walletId wallet to suspend + * @param adminId authenticated administrator's ID, preserved as the audit actor * @return ApiResponse with updates WalletResponse */ @Transactional - public ApiResponse suspendWallet(UUID walletId) { + public ApiResponse suspendWallet(UUID walletId, UUID adminId) { Wallet wallet = walletRepository.findById(walletId) .orElseThrow(() -> new ResourceNotFoundException("Wallet not found")); @@ -178,7 +179,9 @@ public ApiResponse suspendWallet(UUID walletId) { wallet.setStatus(WalletStatus.SUSPENDED); Wallet saved = walletRepository.save(wallet); - auditService.logWalletSuspended(wallet.getUser().getId(), walletId, null, null); + // OWASP A09: record the administrator who made the state change, not + // the wallet owner who was affected by it. + auditService.logWalletSuspended(adminId, walletId, null, null); log.warn("Wallet suspended: walletId={}", walletId); @@ -194,10 +197,11 @@ public ApiResponse suspendWallet(UUID walletId) { * OWASP A01: @PreAuthorize("hasRole('ADMIN')") at controller level * * @param walletId wallet to close permanently + * @param adminId authenticated administrator's ID, preserved as the audit actor * @return ApiResponse with updated WalletResponse */ @Transactional - public ApiResponse closeWallet(UUID walletId) { + public ApiResponse closeWallet(UUID walletId, UUID adminId) { Wallet wallet = walletRepository.findById(walletId) .orElseThrow(() -> new ResourceNotFoundException("Wallet not found")); @@ -213,7 +217,8 @@ public ApiResponse closeWallet(UUID walletId) { wallet.setStatus(WalletStatus.CLOSED); Wallet saved = walletRepository.save(wallet); - auditService.logWalletClosed(wallet.getUser().getId(), walletId, null, null); + // OWASP A09: the audit actor is the administrator, never the affected owner. + auditService.logWalletClosed(adminId, walletId, null, null); log.warn("Wallet permanently closed: walletId={}", walletId); @@ -225,10 +230,11 @@ public ApiResponse closeWallet(UUID walletId) { * SUSPENDED -> ACTIVE * * @param walletId wallet to restore + * @param adminId authenticated administrator's ID, preserved as the audit actor * @return ApiResponse with updated WalletResponse */ @Transactional - public ApiResponse restoreWallet(UUID walletId) { + public ApiResponse restoreWallet(UUID walletId, UUID adminId) { Wallet wallet = walletRepository.findById(walletId) .orElseThrow(() -> new ResourceNotFoundException("Wallet not found")); @@ -241,7 +247,7 @@ public ApiResponse restoreWallet(UUID walletId) { // OWASP A09: wallet restoration re-enables all financial operations on it — // this is a security-relevant state change that must be traceable to the admin. - auditService.logWalletRestored(wallet.getUser().getId(), walletId, null, null); + auditService.logWalletRestored(adminId, walletId, null, null); log.warn("Wallet restored to ACTIVE: walletId={}", walletId); @@ -285,4 +291,4 @@ public UUID resolveUserId(String email) { return userRepository.findByEmail(email) .orElseThrow(() -> new ResourceNotFoundException("User not found")).getId(); } -} \ No newline at end of file +} diff --git a/src/test/java/com/wallet/secure/wallet/service/WalletServiceTest.java b/src/test/java/com/wallet/secure/wallet/service/WalletServiceTest.java index 0ff7d72..83f99a1 100644 --- a/src/test/java/com/wallet/secure/wallet/service/WalletServiceTest.java +++ b/src/test/java/com/wallet/secure/wallet/service/WalletServiceTest.java @@ -64,12 +64,14 @@ public class WalletServiceTest { private User testUser; private UUID userId; + private UUID adminId; private Wallet activeWallet; private UUID walletId; @BeforeEach void setUp() { userId = UUID.randomUUID(); + adminId = UUID.randomUUID(); walletId = UUID.randomUUID(); testUser = User.builder() @@ -200,11 +202,12 @@ void suspendWallet_activeWallet_success() { when(walletRepository.save(any(Wallet.class))).thenReturn(activeWallet); // WHEN - ApiResponse response = walletService.suspendWallet(walletId); + ApiResponse response = walletService.suspendWallet(walletId, adminId); // THEN assertThat(response.isSuccess()).isTrue(); assertThat(activeWallet.getStatus()).isEqualTo(WalletStatus.SUSPENDED); + verify(auditService).logWalletSuspended(adminId, walletId, null, null); } @Test @@ -215,7 +218,7 @@ void suspendWallet_alreadySuspended_throwsException() { when(walletRepository.findById(walletId)).thenReturn(Optional.of(activeWallet)); // WHEN / THEN - assertThatThrownBy(() -> walletService.suspendWallet(walletId)) + assertThatThrownBy(() -> walletService.suspendWallet(walletId, adminId)) .isInstanceOf(IllegalStateException.class) .hasMessageContaining("ACTIVE"); } @@ -228,7 +231,7 @@ void suspendWallet_closedWallet_throwsException() { when(walletRepository.findById(walletId)).thenReturn(Optional.of(activeWallet)); // WHEN / THEN - assertThatThrownBy(() -> walletService.suspendWallet(walletId)) + assertThatThrownBy(() -> walletService.suspendWallet(walletId, adminId)) .isInstanceOf(IllegalStateException.class); } } @@ -248,11 +251,12 @@ void closeWallet_zeroBalance_success() { when(walletRepository.save(any(Wallet.class))).thenReturn(activeWallet); // WHEN - ApiResponse response = walletService.closeWallet(walletId); + ApiResponse response = walletService.closeWallet(walletId, adminId); // THEN assertThat(response.isSuccess()).isTrue(); assertThat(activeWallet.getStatus()).isEqualTo(WalletStatus.CLOSED); + verify(auditService).logWalletClosed(adminId, walletId, null, null); } @Test @@ -264,7 +268,7 @@ void closeWallet_withBalance_throwsException() { // WHEN / THEN // Business rule: cannot close wallet with remaining balance - assertThatThrownBy(() -> walletService.closeWallet(walletId)) + assertThatThrownBy(() -> walletService.closeWallet(walletId, adminId)) .isInstanceOf(IllegalStateException.class) .hasMessageContaining("balance"); } @@ -278,12 +282,34 @@ void closeWallet_alreadyClosed_throwsException() { when(walletRepository.findById(walletId)).thenReturn(Optional.of(activeWallet)); // WHEN / THEN - assertThatThrownBy(() -> walletService.closeWallet(walletId)) + assertThatThrownBy(() -> walletService.closeWallet(walletId, adminId)) .isInstanceOf(IllegalStateException.class) .hasMessageContaining("already closed"); } } + @Nested + @DisplayName("restoreWallet()") + class RestoreWalletTests { + + @Test + @DisplayName("restores SUSPENDED wallet and audits the authenticated administrator") + void restoreWallet_suspendedWallet_success() { + // GIVEN + activeWallet.setStatus(WalletStatus.SUSPENDED); + when(walletRepository.findById(walletId)).thenReturn(Optional.of(activeWallet)); + when(walletRepository.save(any(Wallet.class))).thenReturn(activeWallet); + + // WHEN + ApiResponse response = walletService.restoreWallet(walletId, adminId); + + // THEN + assertThat(response.isSuccess()).isTrue(); + assertThat(activeWallet.getStatus()).isEqualTo(WalletStatus.ACTIVE); + verify(auditService).logWalletRestored(adminId, walletId, null, null); + } + } + // ─── getMyWallets @Nested @@ -330,4 +356,4 @@ void getMyWallets_noWallets_returnsEmptyList() { assertThat(response.getData()).isEmpty(); } } -} \ No newline at end of file +}