From 28e406bf2e42296114800ef45bd25d527cda2b1f Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 15 Aug 2026 14:47:42 +0000 Subject: [PATCH] fix(security): gate saved-dashboard CRUD on connection ACL Mirror SavedQueryController: manage on create/update/delete/favorite (persisted connectionId for mutations), read on GETs. Closes #54. Co-authored-by: Venkat SF --- .../controller/SavedDashboardController.java | 24 ++ .../SavedDashboardControllerAccessTest.java | 217 ++++++++++++++++++ 2 files changed, 241 insertions(+) create mode 100644 backend/src/test/java/com/dbaagent/controller/SavedDashboardControllerAccessTest.java diff --git a/backend/src/main/java/com/dbaagent/controller/SavedDashboardController.java b/backend/src/main/java/com/dbaagent/controller/SavedDashboardController.java index 2dde08a..32ec855 100644 --- a/backend/src/main/java/com/dbaagent/controller/SavedDashboardController.java +++ b/backend/src/main/java/com/dbaagent/controller/SavedDashboardController.java @@ -113,6 +113,7 @@ public ResponseEntity> disableShare(@PathVariable UUID id) { public ResponseEntity> createDashboard(@RequestBody SavedDashboard savedDashboard) { try { log.info("Creating saved dashboard: {} for connection: {}", savedDashboard.getName(), savedDashboard.getConnectionId()); + accessControlService.assertCanManageConnectionContent(savedDashboard.getConnectionId()); SavedDashboard created = savedDashboardService.saveDashboard(savedDashboard); @@ -140,6 +141,7 @@ public ResponseEntity> createDashboard(@RequestBody SavedDas public ResponseEntity> getDashboardsByConnection(@PathVariable String connectionId) { try { log.info("Fetching saved dashboards for connection: {}", connectionId); + accessControlService.assertCanReadConnectionContent(connectionId); List dashboards = savedDashboardService.getDashboardsByConnection(connectionId); @@ -170,6 +172,7 @@ public ResponseEntity> getDashboardById(@PathVariable UUID i return savedDashboardService.getDashboardById(id) .map(dashboard -> { + accessControlService.assertCanReadConnectionContent(dashboard.getConnectionId()); Map response = new HashMap<>(); response.put("success", true); response.put("savedDashboard", dashboard); @@ -199,6 +202,11 @@ public ResponseEntity> getDashboardById(@PathVariable UUID i public ResponseEntity> updateDashboard(@PathVariable UUID id, @RequestBody SavedDashboard updates) { try { log.info("Updating saved dashboard: {}", id); + SavedDashboard existing = savedDashboardService.getDashboardById(id) + .orElseThrow(() -> new IllegalArgumentException("Dashboard not found: " + id)); + // Authorize against the persisted connection — never trust a body + // connectionId that could re-attach the row to a different connection. + accessControlService.assertCanManageConnectionContent(existing.getConnectionId()); SavedDashboard updated = savedDashboardService.updateDashboard(id, updates); @@ -234,6 +242,9 @@ public ResponseEntity> updateDashboard(@PathVariable UUID id public ResponseEntity> deleteDashboard(@PathVariable UUID id) { try { log.info("Deleting saved dashboard: {}", id); + SavedDashboard existing = savedDashboardService.getDashboardById(id) + .orElseThrow(() -> new IllegalArgumentException("Dashboard not found: " + id)); + accessControlService.assertCanManageConnectionContent(existing.getConnectionId()); savedDashboardService.deleteDashboard(id); @@ -242,6 +253,12 @@ public ResponseEntity> deleteDashboard(@PathVariable UUID id response.put("message", "Dashboard deleted successfully"); return ResponseEntity.ok(response); + } catch (IllegalArgumentException e) { + log.error("Dashboard not found: {}", id); + Map errorResponse = new HashMap<>(); + errorResponse.put("success", false); + errorResponse.put("message", e.getMessage()); + return ResponseEntity.status(HttpStatus.NOT_FOUND).body(errorResponse); } catch (org.springframework.web.server.ResponseStatusException e) { throw e; } catch (Exception e) { @@ -260,6 +277,9 @@ public ResponseEntity> deleteDashboard(@PathVariable UUID id public ResponseEntity> toggleFavorite(@PathVariable UUID id) { try { log.info("Toggling favorite for dashboard: {}", id); + SavedDashboard existing = savedDashboardService.getDashboardById(id) + .orElseThrow(() -> new IllegalArgumentException("Dashboard not found: " + id)); + accessControlService.assertCanManageConnectionContent(existing.getConnectionId()); SavedDashboard updated = savedDashboardService.toggleFavorite(id); @@ -295,6 +315,7 @@ public ResponseEntity> toggleFavorite(@PathVariable UUID id) public ResponseEntity> getFavoriteDashboards(@PathVariable String connectionId) { try { log.info("Fetching favorite dashboards for connection: {}", connectionId); + accessControlService.assertCanReadConnectionContent(connectionId); List dashboards = savedDashboardService.getFavoriteDashboards(connectionId); @@ -324,6 +345,7 @@ public ResponseEntity> getDashboardsByFolder( @PathVariable String folder) { try { log.info("Fetching dashboards in folder: {} for connection: {}", folder, connectionId); + accessControlService.assertCanReadConnectionContent(connectionId); List dashboards = savedDashboardService.getDashboardsByFolder(connectionId, folder); @@ -353,6 +375,7 @@ public ResponseEntity> searchDashboards( @RequestParam String q) { try { log.info("Searching dashboards for connection: {} with term: {}", connectionId, q); + accessControlService.assertCanReadConnectionContent(connectionId); List dashboards = savedDashboardService.searchDashboards(connectionId, q); @@ -380,6 +403,7 @@ public ResponseEntity> searchDashboards( public ResponseEntity> getFolders(@PathVariable String connectionId) { try { log.info("Fetching folders for connection: {}", connectionId); + accessControlService.assertCanReadConnectionContent(connectionId); List folders = savedDashboardService.getFolders(connectionId); diff --git a/backend/src/test/java/com/dbaagent/controller/SavedDashboardControllerAccessTest.java b/backend/src/test/java/com/dbaagent/controller/SavedDashboardControllerAccessTest.java new file mode 100644 index 0000000..2341a10 --- /dev/null +++ b/backend/src/test/java/com/dbaagent/controller/SavedDashboardControllerAccessTest.java @@ -0,0 +1,217 @@ +package com.dbaagent.controller; + +import com.dbaagent.model.SavedDashboard; +import com.dbaagent.service.SavedDashboardService; +import com.dbaagent.service.security.AccessControlService; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; +import org.springframework.web.server.ResponseStatusException; + +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.UUID; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Issue #54: create/update/delete (and GETs) must check connection ACL. + * Write paths authorize against the persisted connectionId, never a body + * connectionId that could re-home a dashboard onto another connection. + */ +@ExtendWith(MockitoExtension.class) +class SavedDashboardControllerAccessTest { + + @Mock private SavedDashboardService savedDashboardService; + @Mock private AccessControlService accessControlService; + + private SavedDashboardController controller; + + @BeforeEach + void setUp() { + controller = new SavedDashboardController(); + // Field injection mirrors production @Autowired wiring. + setField(controller, "savedDashboardService", savedDashboardService); + setField(controller, "accessControlService", accessControlService); + } + + @Test + void create_assertsManageOnBodyConnectionId() { + SavedDashboard incoming = dashboard("conn-allowed", "Ops"); + SavedDashboard saved = dashboard("conn-allowed", "Ops"); + saved.setId(UUID.randomUUID()); + when(savedDashboardService.saveDashboard(incoming)).thenReturn(saved); + + ResponseEntity> response = controller.createDashboard(incoming); + + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.CREATED); + verify(accessControlService).assertCanManageConnectionContent("conn-allowed"); + verify(savedDashboardService).saveDashboard(incoming); + } + + @Test + void create_denied_neverPersists() { + SavedDashboard incoming = dashboard("conn-denied", "Leak"); + doThrow(new ResponseStatusException(HttpStatus.FORBIDDEN, "Content access denied for this connection")) + .when(accessControlService).assertCanManageConnectionContent("conn-denied"); + + assertThatThrownBy(() -> controller.createDashboard(incoming)) + .isInstanceOf(ResponseStatusException.class) + .extracting(ex -> ((ResponseStatusException) ex).getStatusCode()) + .isEqualTo(HttpStatus.FORBIDDEN); + + verify(savedDashboardService, never()).saveDashboard(any()); + } + + @Test + void update_assertsManageOnPersistedConnection_notBody() { + UUID id = UUID.randomUUID(); + SavedDashboard existing = dashboard("conn-real", "Existing"); + existing.setId(id); + SavedDashboard body = dashboard("conn-spoofed", "Hijack"); + SavedDashboard updated = dashboard("conn-real", "Existing"); + updated.setId(id); + + when(savedDashboardService.getDashboardById(id)).thenReturn(Optional.of(existing)); + when(savedDashboardService.updateDashboard(id, body)).thenReturn(updated); + + ResponseEntity> response = controller.updateDashboard(id, body); + + assertThat(response.getStatusCode().is2xxSuccessful()).isTrue(); + verify(accessControlService).assertCanManageConnectionContent("conn-real"); + verify(accessControlService, never()).assertCanManageConnectionContent("conn-spoofed"); + verify(savedDashboardService).updateDashboard(id, body); + } + + @Test + void update_denied_neverMutates() { + UUID id = UUID.randomUUID(); + SavedDashboard existing = dashboard("conn-real", "Existing"); + existing.setId(id); + when(savedDashboardService.getDashboardById(id)).thenReturn(Optional.of(existing)); + doThrow(new ResponseStatusException(HttpStatus.FORBIDDEN, "Content access denied for this connection")) + .when(accessControlService).assertCanManageConnectionContent("conn-real"); + + assertThatThrownBy(() -> controller.updateDashboard(id, dashboard("conn-spoofed", "x"))) + .isInstanceOf(ResponseStatusException.class) + .extracting(ex -> ((ResponseStatusException) ex).getStatusCode()) + .isEqualTo(HttpStatus.FORBIDDEN); + + verify(savedDashboardService, never()).updateDashboard(any(), any()); + } + + @Test + void delete_assertsManageOnPersistedConnection() { + UUID id = UUID.randomUUID(); + SavedDashboard existing = dashboard("conn-real", "Existing"); + existing.setId(id); + when(savedDashboardService.getDashboardById(id)).thenReturn(Optional.of(existing)); + + ResponseEntity> response = controller.deleteDashboard(id); + + assertThat(response.getStatusCode().is2xxSuccessful()).isTrue(); + verify(accessControlService).assertCanManageConnectionContent("conn-real"); + verify(savedDashboardService).deleteDashboard(id); + } + + @Test + void delete_denied_neverDeletes() { + UUID id = UUID.randomUUID(); + SavedDashboard existing = dashboard("conn-real", "Existing"); + existing.setId(id); + when(savedDashboardService.getDashboardById(id)).thenReturn(Optional.of(existing)); + doThrow(new ResponseStatusException(HttpStatus.FORBIDDEN, "Content access denied for this connection")) + .when(accessControlService).assertCanManageConnectionContent("conn-real"); + + assertThatThrownBy(() -> controller.deleteDashboard(id)) + .isInstanceOf(ResponseStatusException.class) + .extracting(ex -> ((ResponseStatusException) ex).getStatusCode()) + .isEqualTo(HttpStatus.FORBIDDEN); + + verify(savedDashboardService, never()).deleteDashboard(any()); + } + + @Test + void delete_missing_returns404() { + UUID id = UUID.randomUUID(); + when(savedDashboardService.getDashboardById(id)).thenReturn(Optional.empty()); + + ResponseEntity> response = controller.deleteDashboard(id); + + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.NOT_FOUND); + verify(accessControlService, never()).assertCanManageConnectionContent(any()); + verify(savedDashboardService, never()).deleteDashboard(any()); + } + + @Test + void favorite_assertsManageOnPersistedConnection() { + UUID id = UUID.randomUUID(); + SavedDashboard existing = dashboard("conn-real", "Existing"); + existing.setId(id); + SavedDashboard toggled = dashboard("conn-real", "Existing"); + toggled.setId(id); + toggled.setIsFavorite(true); + when(savedDashboardService.getDashboardById(id)).thenReturn(Optional.of(existing)); + when(savedDashboardService.toggleFavorite(id)).thenReturn(toggled); + + ResponseEntity> response = controller.toggleFavorite(id); + + assertThat(response.getStatusCode().is2xxSuccessful()).isTrue(); + verify(accessControlService).assertCanManageConnectionContent("conn-real"); + verify(savedDashboardService).toggleFavorite(id); + } + + @Test + void getById_assertsReadOnPersistedConnection() { + UUID id = UUID.randomUUID(); + SavedDashboard existing = dashboard("conn-real", "Existing"); + existing.setId(id); + when(savedDashboardService.getDashboardById(id)).thenReturn(Optional.of(existing)); + + ResponseEntity> response = controller.getDashboardById(id); + + assertThat(response.getStatusCode().is2xxSuccessful()).isTrue(); + verify(accessControlService).assertCanReadConnectionContent("conn-real"); + } + + @Test + void listByConnection_assertsRead() { + when(savedDashboardService.getDashboardsByConnection("conn-1")).thenReturn(List.of()); + + ResponseEntity> response = controller.getDashboardsByConnection("conn-1"); + + assertThat(response.getStatusCode().is2xxSuccessful()).isTrue(); + verify(accessControlService).assertCanReadConnectionContent("conn-1"); + } + + private static SavedDashboard dashboard(String connectionId, String name) { + SavedDashboard d = new SavedDashboard(); + d.setConnectionId(connectionId); + d.setName(name); + d.setDashboardConfig("{}"); + d.setIsFavorite(false); + d.setIsPublic(false); + return d; + } + + private static void setField(Object target, String name, Object value) { + try { + var field = SavedDashboardController.class.getDeclaredField(name); + field.setAccessible(true); + field.set(target, value); + } catch (ReflectiveOperationException e) { + throw new IllegalStateException(e); + } + } +}