From 6dabfcd6c272b4f75cb709e628ea98da0c0e5aeb Mon Sep 17 00:00:00 2001 From: sumit Date: Wed, 26 Aug 2026 21:58:58 +0530 Subject: [PATCH 1/2] fix: authorize every connection-scoped endpoint (116 were unguarded) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 12 controllers took a caller-supplied connectionId and never checked it. SecurityConfig only asserts .anyRequest().authenticated() and no filter, interceptor or aspect inspects a connection id, so authentication was the only barrier. Verified against a running install, not inferred: a DEVELOPER holding no grant on any connection could - read literal-bearing slow-query SQL with real customer ids and names (GET /slow-query-analytics/{id}/query/{fp}/samples returned 200 while GET /slow-log-source/{id} returned 403 in the same session), - enumerate another tenant's schema and table statistics, via two endpoints that decrypt the target connection's credentials and open a live JDBC session (/tenant-column-suggestions and /config), - and permanently delete that tenant's analysis history (DELETE /slow-queries/history/connection/{id} -> 200, row gone). Affected: SlowQueryController (43), SlowQueryAnalyticsController (13), SchemaChangeController (13), SentinelAnalyticsController (10), PerformanceActionController (9), QueryPerformanceController (8), QueryPlanController (8), IndexAdvisorController (7), PerformanceInsightsController (5), AdvisorController (3), ResourceLimitsController (3), BusinessRuleController (3). This is the same class of defect BrainController carried (93 of 116 unguarded). The safety test added then hardcodes one Path.of(...), so it could not see any of these. What changed * 127 guard calls: assertCanReadConnectionContent on reads, assertCanManageConnectionContent on writes and deletes. * An id is not a capability. For endpoints keyed on alertId, actionId, regressionId, recommendationId, fingerprintId, planId, ruleId, snapshotId or historyId, resolve the owning connection and assert on that. 15 new findConnectionIdFor* accessors where no lookup existed. These report 404, not 403, for an unknown id — a 403 confirms the row exists, turning the endpoint into an id oracle, and regressionId is a sequential Long. * Ids arriving in the request body are not constrained by a path-variable check. Four holes survived exactly that kind of fix: - schema-changes/snapshots/compare took two snapshot ids and no connectionId at all, so it would diff tenant A's schema against tenant B's; compareSnapshots now refuses a mismatch outright. - PUT /performance-actions/batch-status took an arbitrary actionIds list with no scope; it now authorizes every id before mutating any, so a mixed batch fails atomically. - changes/acknowledge and regressions/acknowledge authorized the path connection and then acted on whatever ids the body named; allChangesBelongTo / allComparisonsBelongTo verify membership, and an id that resolves to nothing fails too, so unknown ids cannot be mixed into an otherwise valid batch. * Never take the actor from the request. userId was a query parameter and acknowledgedBy/resolvedBy/updatedBy defaulted to the literal string "user", so the acknowledgement trail was unauthenticated free text that could name any colleague. 10 sites now use requireCurrentUsername(). The parameters are still accepted for wire compatibility and ignored. * ConnectionScopedAuthorizationSafetyTest replaces the per-file approach: it scans every *Controller.java, so a new controller is covered the day it is written. Six cases — connection-scoped endpoints authorized, body-supplied id collections scoped, 403 not swallowed into 500, controller advices not swallowing denials, exemptions still true, delegated service checks still present. The exemption list is itself guarded, so it cannot rot into a way of hiding a real gap. Two things found by writing and running the fix, not by reading it * The generalized test immediately found 9 more unguarded endpoints in controllers nobody was looking at: StatsController, ProjectController, DashboardController, and a destructive DELETE /sentinel/demo/cleanup/{connectionId}. Three had been in my draft exemption list on the assumption they were connection-free; they were not. * Testing the fix found a bug reading it never would. 24 endpoints returned 403 and index-advisor returned 500: IndexAdvisorExceptionHandler's @ExceptionHandler(Exception.class) swallowed the denial and reported "Index operation failed" with the 403's text in the body. The guard held, but the response blamed the index store. It now handles ResponseStatusException first, and the safety test asserts no advice with a catch-all omits that. Also drops @CrossOrigin(origins = "*") from SentinelAnalyticsController. Tested and inert — an evil-origin preflight gets 403 with no Access-Control-Allow-Origin because the SecurityConfig allowlist wins, while an allowed origin gets 200 + ACAO — but it reads like an intentional hole. Verification Real Maven compile of main and test sources, zero errors. SlowQueryControllerS3Test needed the new constructor argument and was updated rather than left red. Live, against the rebuilt image with a DEVELOPER holding no grant on the target connection: - 40/40 previously-leaking reads -> 403 - 10/10 writes and destructive endpoints -> 403, and psql confirms nothing was mutated - 7/7 orphan-id and body-scoped paths -> 404, no existence oracle - 30/30 same endpoint shapes on a granted connection -> 200, zero false denials; confirmed again from a real browser session - index-advisor now returns 403 "Read access denied for this connection" Not covered: mvn test was not executed (the image build uses -DskipTests and this host has no JDK/Maven). The six safety-test cases were validated by re-implementing their scan logic against the tree and the file compiles, but they have not been run by JUnit. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 67 +++ .../controller/AdvisorController.java | 14 + .../controller/BusinessRuleController.java | 14 + .../controller/DashboardController.java | 9 + .../controller/IndexAdvisorController.java | 15 + .../IndexAdvisorExceptionHandler.java | 21 + .../PerformanceActionController.java | 37 +- .../PerformanceInsightsController.java | 13 + .../controller/ProjectController.java | 14 + .../QueryPerformanceController.java | 43 +- .../controller/QueryPlanController.java | 35 +- .../controller/ResourceLimitsController.java | 11 + .../controller/SchemaChangeController.java | 52 ++- .../SentinelAnalyticsController.java | 34 +- .../SentinelDemoDataController.java | 10 + .../SlowQueryAnalyticsController.java | 21 + .../controller/SlowQueryController.java | 108 ++++- .../dbaagent/controller/StatsController.java | 12 + .../service/BusinessRuleMemoryService.java | 8 + .../service/QueryFingerprintService.java | 8 + .../service/QueryPerformanceService.java | 9 + .../service/QueryPlanCacheService.java | 24 ++ .../service/SchemaChangeTrackingService.java | 31 ++ .../service/SentinelAnalyticsService.java | 9 + .../service/SlowQueryAlertService.java | 8 + ...nnectionScopedAuthorizationSafetyTest.java | 383 ++++++++++++++++++ .../controller/SlowQueryControllerS3Test.java | 14 +- 27 files changed, 1009 insertions(+), 15 deletions(-) create mode 100644 backend/src/test/java/com/dbaagent/controller/ConnectionScopedAuthorizationSafetyTest.java diff --git a/CLAUDE.md b/CLAUDE.md index c5c7cab..53bc9d2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -709,6 +709,73 @@ it against a real database — not a theoretical hardening pass. `catch (ResponseStatusException e) { throw e; }` a 403 is swallowed and reported as a server error, so a client cannot tell "not yours" from "broken". The safety test asserts this too. +- **Then it happened again, on 12 more controllers — 116 endpoints, zero checks.** + `BrainControllerAuthorizationSafetyTest` hardcodes one `Path.of(...)`, so it could not + see `SlowQueryController` (43), `SlowQueryAnalyticsController` (13), + `SchemaChangeController` (13), `SentinelAnalyticsController` (10), + `PerformanceActionController` (9), `QueryPerformanceController` (8), + `QueryPlanController` (8), `IndexAdvisorController` (7), + `PerformanceInsightsController` (5), `AdvisorController` (3), + `ResourceLimitsController` (3) or `BusinessRuleController` (3). Verified live, not + inferred: a DEVELOPER holding **no grant on any connection** read literal-bearing + slow-query SQL with real customer ids and names + (`/slow-query-analytics/{id}/query/{fp}/samples` → 200 while + `/slow-log-source/{id}` → 403 in the same session), enumerated another tenant's + schema, and **deleted that tenant's analysis history** via + `DELETE /slow-queries/history/connection/{id}`. All 116 are now guarded. + `ConnectionScopedAuthorizationSafetyTest` replaces the per-file approach: it scans + **every** `*Controller.java`, so a new controller is covered the day it is written. + Writing it immediately found 9 more unguarded endpoints in controllers nobody was + looking at, including `StatsController`, `ProjectController`, `DashboardController` + and a destructive `DELETE /sentinel/demo/cleanup/{connectionId}`. +- **Two endpoints decrypted another user's credentials before anyone checked access.** + `GET /slow-query-analytics/{id}/tenant-column-suggestions` and `/config` reach + `suggestTenantColumns` → `getJdbcTemplateForBackgroundJob` → + `credentialService.getDecryptedConnection`, opening a live JDBC session to the target + database. An unguarded read is not only a data leak; it can be a credential-use + primitive. Check before the work, not after. +- **A path-variable sweep is not enough — ids in the request body need their own + check.** Four holes survived exactly that kind of fix: `snapshots/compare` (two + snapshot ids, no `connectionId` at all — it would diff tenant A's schema against + tenant B's), `PUT /performance-actions/batch-status` (an arbitrary `actionIds` list, + no scope), and `changes/acknowledge` / `regressions/acknowledge` (path connection + authorized, body ids unchecked). `allChangesBelongTo` / `allComparisonsBelongTo` + verify membership, and **an id that resolves to nothing fails too** — otherwise + unknown ids can be mixed into an otherwise valid batch. The safety test has a + dedicated case for body-supplied id collections. +- **An id is not a capability.** For `alertId`, `actionId`, `regressionId`, + `recommendationId`, `fingerprintId`, `planId`, `ruleId`, `snapshotId`, `historyId`: + resolve the owning connection and assert on that. Several services had no such + accessor, so `findConnectionIdFor*` was added to `QueryPerformanceService`, + `QueryPlanCacheService`, `SentinelAnalyticsService`, `BusinessRuleMemoryService`, + `SlowQueryAlertService`, `QueryFingerprintService` and `SchemaChangeTrackingService`. + These helpers report **404, not 403**, for an unknown id — a 403 confirms the row + exists, turning the endpoint into an id oracle. `regressionId` is a sequential + `Long`, so that mattered. +- **Never take the actor from the request.** `POST /slow-queries/alerts/{id}/acknowledge` + took `@RequestParam String userId`, and three acknowledge endpoints took + `acknowledgedBy` defaulting to the literal string `"user"` — so the audit trail was + unauthenticated free text and could name any colleague. All seven sites now use + `accessControlService.requireCurrentUsername()`. The parameters are still accepted + (wire compatibility) and ignored. +- **Guarded vs unguarded is an existence oracle.** A guarded endpoint 404s an unknown + connection id (`resolveCurrentUserAccess` wraps the lookup); an unguarded one returned + 200. That difference alone enumerated valid connection ids. +- **A `@ControllerAdvice` catch-all swallows a 403 the same way an in-method one does, + and it is easier to miss because it lives in another file.** + `IndexAdvisorExceptionHandler` has `@ExceptionHandler(Exception.class)`, so the newly + added guard on `/index-advisor/{id}/health-report` returned + `500 "Index operation failed"` with the 403's text in the body — the denial held, but + the response blamed the index store. Found by *testing the fix*, not by reading it: the + other 24 endpoints returned 403 and this one did not. It now has an + `@ExceptionHandler(ResponseStatusException.class)` that preserves the status, and + `ConnectionScopedAuthorizationSafetyTest` asserts every advice with a catch-all also + handles `ResponseStatusException`. +- **`@CrossOrigin(origins = "*")` on a controller is dead code here, and worth + deleting.** `SentinelAnalyticsController` carried it. Tested: an evil-origin preflight + gets `403` with no `Access-Control-Allow-Origin` (the `SecurityConfig` allowlist wins), + while an allowed origin gets `200` + ACAO — so the annotation never had effect. It + still reads like an intentional hole to the next person. ### MCP & CLI Release Rules diff --git a/backend/src/main/java/com/dbaagent/controller/AdvisorController.java b/backend/src/main/java/com/dbaagent/controller/AdvisorController.java index fac0ec5..632ab53 100644 --- a/backend/src/main/java/com/dbaagent/controller/AdvisorController.java +++ b/backend/src/main/java/com/dbaagent/controller/AdvisorController.java @@ -3,6 +3,7 @@ import com.dbaagent.model.IndexRecommendation; import com.dbaagent.model.PerformanceAnalysis; import com.dbaagent.service.DatabaseAdvisorService; +import com.dbaagent.service.security.AccessControlService; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.http.ResponseEntity; @@ -10,6 +11,15 @@ import java.util.List; +/** + * REST API for the performance advisor (analysis, missing indexes, health summary). + * + *

Authorization: every endpoint here takes a caller-supplied connection id, so + * each one asserts access itself ({@code assertCanReadConnectionContent} for reads, + * {@code assertCanManageConnectionContent} for writes). {@code SecurityConfig} only + * requires an authenticated principal — nothing upstream inspects a connection id. See + * {@code ConnectionScopedAuthorizationSafetyTest}. + */ @RestController @RequestMapping("/advisor") @RequiredArgsConstructor @@ -17,6 +27,7 @@ public class AdvisorController { private final DatabaseAdvisorService advisorService; + private final AccessControlService accessControlService; /** * Get comprehensive performance analysis @@ -25,6 +36,7 @@ public class AdvisorController { public ResponseEntity analyzePerformance( @PathVariable String connectionId ) { + accessControlService.assertCanReadConnectionContent(connectionId); try { log.info("Performance analysis requested for connection: {}", connectionId); PerformanceAnalysis analysis = advisorService.analyzePerformance(connectionId); @@ -44,6 +56,7 @@ public ResponseEntity analyzePerformance( public ResponseEntity> getMissingIndexes( @PathVariable String connectionId ) { + accessControlService.assertCanReadConnectionContent(connectionId); try { log.info("Index recommendations requested for connection: {}", connectionId); PerformanceAnalysis analysis = advisorService.analyzePerformance(connectionId); @@ -63,6 +76,7 @@ public ResponseEntity> getMissingIndexes( public ResponseEntity getHealthSummary( @PathVariable String connectionId ) { + accessControlService.assertCanReadConnectionContent(connectionId); try { log.info("Health summary requested for connection: {}", connectionId); PerformanceAnalysis analysis = advisorService.analyzePerformance(connectionId); diff --git a/backend/src/main/java/com/dbaagent/controller/BusinessRuleController.java b/backend/src/main/java/com/dbaagent/controller/BusinessRuleController.java index 16fa5f5..4b08cf7 100644 --- a/backend/src/main/java/com/dbaagent/controller/BusinessRuleController.java +++ b/backend/src/main/java/com/dbaagent/controller/BusinessRuleController.java @@ -2,6 +2,7 @@ import com.dbaagent.model.brain.BrainRule; import com.dbaagent.service.BusinessRuleMemoryService; +import com.dbaagent.service.security.AccessControlService; import lombok.RequiredArgsConstructor; import org.springframework.http.ResponseEntity; import org.springframework.web.bind.annotation.*; @@ -11,6 +12,12 @@ /** * API endpoints for connection-scoped learned SQL business rules. + * + *

Authorization: every endpoint here takes a caller-supplied connection id, so + * each one asserts access itself ({@code assertCanReadConnectionContent} for reads, + * {@code assertCanManageConnectionContent} for writes). {@code SecurityConfig} only + * requires an authenticated principal — nothing upstream inspects a connection id. See + * {@code ConnectionScopedAuthorizationSafetyTest}. */ @RestController @RequestMapping("/business-rules") @@ -18,6 +25,7 @@ public class BusinessRuleController { private final BusinessRuleMemoryService businessRuleMemoryService; + private final AccessControlService accessControlService; /** * Returns all active rules for the connection plus the subset applicable to an optional question. @@ -26,6 +34,7 @@ public class BusinessRuleController { public ResponseEntity> getRules( @PathVariable String connectionId, @RequestParam(required = false) String question) { + accessControlService.assertCanReadConnectionContent(connectionId); List activeRules = businessRuleMemoryService.getActiveRules(connectionId); List applicable = businessRuleMemoryService .resolveApplicableGuardrails(connectionId, question, null); @@ -49,6 +58,7 @@ public ResponseEntity> getRules( public ResponseEntity> learn( @PathVariable String connectionId, @RequestBody LearnRuleRequest request) { + accessControlService.assertCanManageConnectionContent(connectionId); int learned = businessRuleMemoryService.learnFromFeedback( connectionId, request.text(), @@ -70,6 +80,10 @@ public ResponseEntity> learn( */ @DeleteMapping("/rule/{ruleId}") public ResponseEntity> deactivateRule(@PathVariable String ruleId) { + String connectionId = businessRuleMemoryService.findConnectionIdForRule(ruleId) + .orElseThrow(() -> new org.springframework.web.server.ResponseStatusException( + org.springframework.http.HttpStatus.NOT_FOUND, "Rule not found")); + accessControlService.assertCanManageConnectionContent(connectionId); boolean deactivated = businessRuleMemoryService.deactivateRule(ruleId); return ResponseEntity.ok(Map.of( "ruleId", ruleId, diff --git a/backend/src/main/java/com/dbaagent/controller/DashboardController.java b/backend/src/main/java/com/dbaagent/controller/DashboardController.java index 27dcf75..7b92220 100644 --- a/backend/src/main/java/com/dbaagent/controller/DashboardController.java +++ b/backend/src/main/java/com/dbaagent/controller/DashboardController.java @@ -1,6 +1,7 @@ package com.dbaagent.controller; import com.dbaagent.service.DashboardService; +import com.dbaagent.service.security.AccessControlService; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.http.ResponseEntity; @@ -8,6 +9,12 @@ /** * REST API for performance dashboard + * + *

Authorization: every endpoint here takes a caller-supplied connection id, so + * each one asserts access itself ({@code assertCanReadConnectionContent} for reads, + * {@code assertCanManageConnectionContent} for writes). {@code SecurityConfig} only + * requires an authenticated principal — nothing upstream inspects a connection id. See + * {@code ConnectionScopedAuthorizationSafetyTest}. */ @RestController @RequestMapping("/dashboard") @@ -16,6 +23,7 @@ public class DashboardController { private final DashboardService dashboardService; + private final AccessControlService accessControlService; /** * Get performance dashboard data for a connection @@ -26,6 +34,7 @@ public ResponseEntity getPerformanceDashboard( @RequestParam(required = false, defaultValue = "30") Integer days ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); log.info("Fetching performance dashboard for connection: {}, days: {}", connectionId, days); DashboardService.DashboardData data = dashboardService.getDashboardData(connectionId, days); diff --git a/backend/src/main/java/com/dbaagent/controller/IndexAdvisorController.java b/backend/src/main/java/com/dbaagent/controller/IndexAdvisorController.java index eb729fe..3bbfc5e 100644 --- a/backend/src/main/java/com/dbaagent/controller/IndexAdvisorController.java +++ b/backend/src/main/java/com/dbaagent/controller/IndexAdvisorController.java @@ -2,6 +2,7 @@ import com.dbaagent.service.IndexAdvisorService; import com.dbaagent.service.PerformanceMonitoringService; +import com.dbaagent.service.security.AccessControlService; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.http.ResponseEntity; @@ -12,6 +13,12 @@ /** * REST API for enhanced index advisor functionality + * + *

Authorization: every endpoint here takes a caller-supplied connection id, so + * each one asserts access itself ({@code assertCanReadConnectionContent} for reads, + * {@code assertCanManageConnectionContent} for writes). {@code SecurityConfig} only + * requires an authenticated principal — nothing upstream inspects a connection id. See + * {@code ConnectionScopedAuthorizationSafetyTest}. */ @RestController @RequestMapping("/index-advisor") @@ -21,12 +28,14 @@ public class IndexAdvisorController { private final IndexAdvisorService indexAdvisorService; private final PerformanceMonitoringService performanceMonitoringService; + private final AccessControlService accessControlService; /** * Get comprehensive index health report */ @GetMapping("/{connectionId}/health-report") public ResponseEntity> getHealthReport(@PathVariable String connectionId) { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(indexAdvisorService.getIndexHealthReport(connectionId)); } @@ -35,6 +44,7 @@ public ResponseEntity> getHealthReport(@PathVariable String */ @GetMapping("/{connectionId}/unused") public ResponseEntity>> getUnusedIndexes(@PathVariable String connectionId) { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(performanceMonitoringService.getUnusedIndexes(connectionId)); } @@ -43,6 +53,7 @@ public ResponseEntity>> getUnusedIndexes(@PathVariable */ @GetMapping("/{connectionId}/duplicates") public ResponseEntity>> getDuplicateIndexes(@PathVariable String connectionId) { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(performanceMonitoringService.getDuplicateIndexes(connectionId)); } @@ -53,6 +64,7 @@ public ResponseEntity>> getDuplicateIndexes(@PathVariab public ResponseEntity> estimateIndexCreation( @PathVariable String connectionId, @RequestBody Map request) { + accessControlService.assertCanReadConnectionContent(connectionId); String tableName = (String) request.get("tableName"); @SuppressWarnings("unchecked") @@ -75,6 +87,7 @@ public ResponseEntity> estimateIndexCreation( public ResponseEntity> estimateIndexDrop( @PathVariable String connectionId, @RequestBody Map request) { + accessControlService.assertCanReadConnectionContent(connectionId); String tableName = (String) request.get("tableName"); String indexName = (String) request.get("indexName"); @@ -94,6 +107,7 @@ public ResponseEntity> estimateIndexDrop( public ResponseEntity>> getIndexUsageStats( @PathVariable String connectionId, @PathVariable String tableName) { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(performanceMonitoringService.getIndexUsageStats(connectionId, tableName)); } @@ -102,6 +116,7 @@ public ResponseEntity>> getIndexUsageStats( */ @GetMapping("/{connectionId}/cache-stats") public ResponseEntity> getCacheStats(@PathVariable String connectionId) { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(performanceMonitoringService.getCacheHitRatios(connectionId)); } } diff --git a/backend/src/main/java/com/dbaagent/controller/IndexAdvisorExceptionHandler.java b/backend/src/main/java/com/dbaagent/controller/IndexAdvisorExceptionHandler.java index d96535a..9453524 100644 --- a/backend/src/main/java/com/dbaagent/controller/IndexAdvisorExceptionHandler.java +++ b/backend/src/main/java/com/dbaagent/controller/IndexAdvisorExceptionHandler.java @@ -61,6 +61,27 @@ public ResponseEntity> handleDataAccess(Exception ex) { return ResponseEntity.status(HttpStatus.SERVICE_UNAVAILABLE).body(body); } + /** + * An authorization denial is a deliberate answer, not a failure of this feature. + * {@code handleGeneric} below matches {@code Exception}, so without this more specific + * handler a {@code ResponseStatusException} from + * {@code assertCanReadConnectionContent} was reported as + * {@code 500 "Index operation failed"} — the denial still held, but the caller could + * not tell "not yours" from "the index store is broken", and the message named the + * wrong subsystem. Verified: a non-granted user hitting + * {@code /index-advisor/{id}/health-report} got a 500 whose body carried the 403 text. + */ + @ExceptionHandler(org.springframework.web.server.ResponseStatusException.class) + public ResponseEntity> handleStatus( + org.springframework.web.server.ResponseStatusException ex) { + Map body = new LinkedHashMap<>(); + body.put("timestamp", Instant.now().toString()); + body.put("status", ex.getStatusCode().value()); + body.put("error", ex.getStatusCode().toString()); + body.put("message", ex.getReason() != null ? ex.getReason() : ex.getMessage()); + return ResponseEntity.status(ex.getStatusCode()).body(body); + } + /** Any other uncaught error from these endpoints → a clean message, not an opaque 500. */ @ExceptionHandler(Exception.class) public ResponseEntity> handleGeneric(Exception ex) { diff --git a/backend/src/main/java/com/dbaagent/controller/PerformanceActionController.java b/backend/src/main/java/com/dbaagent/controller/PerformanceActionController.java index 0e67f48..da266c3 100644 --- a/backend/src/main/java/com/dbaagent/controller/PerformanceActionController.java +++ b/backend/src/main/java/com/dbaagent/controller/PerformanceActionController.java @@ -12,11 +12,14 @@ import com.dbaagent.service.PerformanceActionAggregatorService.ActionSummary; import com.dbaagent.service.PerformanceActionAggregatorService.RefreshResult; import com.dbaagent.service.SlowQueryHistoryService; +import com.dbaagent.service.security.AccessControlService; import lombok.AllArgsConstructor; import lombok.Data; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; +import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; +import org.springframework.web.server.ResponseStatusException; import org.springframework.web.bind.annotation.*; import java.util.ArrayList; @@ -28,6 +31,12 @@ /** * REST controller for unified performance actions. * Provides endpoints for listing, filtering, refreshing, and managing performance recommendations. + * + *

Authorization: every endpoint here takes a caller-supplied connection id, so + * each one asserts access itself ({@code assertCanReadConnectionContent} for reads, + * {@code assertCanManageConnectionContent} for writes). {@code SecurityConfig} only + * requires an authenticated principal — nothing upstream inspects a connection id. See + * {@code ConnectionScopedAuthorizationSafetyTest}. */ @RestController @RequestMapping("/performance-actions") @@ -40,6 +49,7 @@ public class PerformanceActionController { private final PerformanceActionAggregatorService aggregatorService; private final SlowQueryHistoryService slowQueryHistoryService; + private final AccessControlService accessControlService; /** * Get all pending performance actions for a connection, sorted by ROI. @@ -47,6 +57,7 @@ public class PerformanceActionController { @GetMapping("/{connectionId}") public ResponseEntity> getActions( @PathVariable String connectionId) { + accessControlService.assertCanReadConnectionContent(connectionId); log.debug("Getting performance actions for connection: {}", connectionId); List actions = aggregatorService.getAggregatedActions(connectionId); return ResponseEntity.ok(actions); @@ -59,6 +70,7 @@ public ResponseEntity> getActions( public ResponseEntity> getTopActions( @PathVariable String connectionId, @RequestParam(defaultValue = "10") int limit) { + accessControlService.assertCanReadConnectionContent(connectionId); log.debug("Getting top {} actions for connection: {}", limit, connectionId); List actions = aggregatorService.getTopActions(connectionId, limit); return ResponseEntity.ok(actions); @@ -71,6 +83,7 @@ public ResponseEntity> getTopActions( public ResponseEntity> getActionsByCategory( @PathVariable String connectionId, @PathVariable ActionCategory category) { + accessControlService.assertCanReadConnectionContent(connectionId); log.debug("Getting actions for connection {} by category: {}", connectionId, category); List actions = aggregatorService.getActionsByCategory(connectionId, category); return ResponseEntity.ok(actions); @@ -83,6 +96,7 @@ public ResponseEntity> getActionsByCategory( public ResponseEntity> getActionsBySource( @PathVariable String connectionId, @PathVariable ActionSource source) { + accessControlService.assertCanReadConnectionContent(connectionId); log.debug("Getting actions for connection {} by source: {}", connectionId, source); List actions = aggregatorService.getActionsBySource(connectionId, source); return ResponseEntity.ok(actions); @@ -94,6 +108,7 @@ public ResponseEntity> getActionsBySource( @GetMapping("/{connectionId}/summary") public ResponseEntity getSummary( @PathVariable String connectionId) { + accessControlService.assertCanReadConnectionContent(connectionId); log.debug("Getting action summary for connection: {}", connectionId); ActionSummary summary = aggregatorService.getSummary(connectionId); return ResponseEntity.ok(summary); @@ -105,6 +120,7 @@ public ResponseEntity getSummary( @PostMapping("/{connectionId}/refresh") public ResponseEntity refreshActions( @PathVariable String connectionId) { + accessControlService.assertCanManageConnectionContent(connectionId); log.info("Refreshing performance actions for connection: {}", connectionId); RefreshResult result = aggregatorService.refreshActions(connectionId); return ResponseEntity.ok(result); @@ -118,10 +134,13 @@ public ResponseEntity updateStatus( @PathVariable String actionId, @RequestBody StatusUpdateRequest request) { log.info("Updating action {} status to: {}", actionId, request.getStatus()); + assertCanManageAction(actionId); + // The resolver is the authenticated caller, never request.getResolvedBy(): + // that was client-supplied, so the audit trail could name anyone. PerformanceAction updated = aggregatorService.updateStatus( actionId, request.getStatus(), - request.getResolvedBy(), + accessControlService.requireCurrentUsername(), request.getNotes()); return ResponseEntity.ok(updated); } @@ -134,12 +153,13 @@ public ResponseEntity> batchUpdateStatus( @RequestBody BatchStatusUpdateRequest request) { log.info("Batch updating {} actions to status: {}", request.getActionIds().size(), request.getStatus()); + request.getActionIds().forEach(this::assertCanManageAction); List updated = request.getActionIds().stream() .map(id -> aggregatorService.updateStatus( id, request.getStatus(), - request.getResolvedBy(), + accessControlService.requireCurrentUsername(), request.getNotes())) .toList(); @@ -158,6 +178,7 @@ public ResponseEntity getAffectedQueries(@PathVariable } PerformanceAction action = actionOpt.get(); String connectionId = action.getConnectionId(); + accessControlService.assertCanReadConnectionContent(connectionId); String tableName = action.getTargetObject(); if (tableName == null || tableName.isBlank()) { return ResponseEntity.ok(new AffectedQueriesResponse(List.of(), 0)); @@ -273,4 +294,16 @@ public static class AffectedQueryItem { private final Double avgExecutionTimeMs; private final Long callCount; } + + /** + * Authorize a write keyed only on an action id. The action carries its own + * connectionId, so resolve that first and assert against it — an action id + * is not a capability. An unknown id reports 404 rather than 403 so the + * endpoint cannot be used to probe which action ids exist. + */ + private void assertCanManageAction(String actionId) { + PerformanceAction action = aggregatorService.getActionById(actionId) + .orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND, "Action not found")); + accessControlService.assertCanManageConnectionContent(action.getConnectionId()); + } } diff --git a/backend/src/main/java/com/dbaagent/controller/PerformanceInsightsController.java b/backend/src/main/java/com/dbaagent/controller/PerformanceInsightsController.java index 568c514..ec5eccb 100644 --- a/backend/src/main/java/com/dbaagent/controller/PerformanceInsightsController.java +++ b/backend/src/main/java/com/dbaagent/controller/PerformanceInsightsController.java @@ -5,6 +5,7 @@ import com.dbaagent.model.PerformanceSnapshot; import com.dbaagent.service.CredentialService; import com.dbaagent.service.PerformanceInsightsService; +import com.dbaagent.service.security.AccessControlService; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.http.ResponseEntity; @@ -17,6 +18,12 @@ /** * Performance Insights Controller * Provides AWS RDS Performance Insights-style APIs + * + *

Authorization: every endpoint here takes a caller-supplied connection id, so + * each one asserts access itself ({@code assertCanReadConnectionContent} for reads, + * {@code assertCanManageConnectionContent} for writes). {@code SecurityConfig} only + * requires an authenticated principal — nothing upstream inspects a connection id. See + * {@code ConnectionScopedAuthorizationSafetyTest}. */ @RestController @RequestMapping("/performance-insights") @@ -27,6 +34,7 @@ public class PerformanceInsightsController { private final PerformanceInsightsService performanceInsightsService; private final CredentialService credentialService; + private final AccessControlService accessControlService; /** * GET /api/performance-insights/{connectionId} @@ -36,6 +44,7 @@ public class PerformanceInsightsController { public ResponseEntity getSnapshots( @PathVariable String connectionId, @RequestParam(defaultValue = "1") int hours) { + accessControlService.assertCanReadConnectionContent(connectionId); try { log.info("Fetching performance snapshots for connection: {}, hours: {}", connectionId, hours); @@ -75,6 +84,7 @@ public ResponseEntity getSnapshots( public ResponseEntity getRecentSnapshots( @PathVariable String connectionId, @RequestParam(defaultValue = "12") int limit) { + accessControlService.assertCanReadConnectionContent(connectionId); try { log.info("Fetching {} recent performance snapshots for connection: {}", limit, connectionId); @@ -107,6 +117,7 @@ public ResponseEntity getRecentSnapshots( */ @PostMapping("/{connectionId}/collect") public ResponseEntity collectSnapshot(@PathVariable String connectionId) { + accessControlService.assertCanManageConnectionContent(connectionId); try { log.info("Manual snapshot collection triggered for connection: {}", connectionId); @@ -145,6 +156,7 @@ public ResponseEntity collectSnapshot(@PathVariable String connectionId) { public ResponseEntity getSummary( @PathVariable String connectionId, @RequestParam(defaultValue = "1") int hours) { + accessControlService.assertCanReadConnectionContent(connectionId); try { log.info("Fetching performance summary for connection: {}, hours: {}", connectionId, hours); @@ -237,6 +249,7 @@ public ResponseEntity getSummary( */ @GetMapping("/table-usage/{connectionId}") public ResponseEntity getTableUsage(@PathVariable String connectionId) { + accessControlService.assertCanReadConnectionContent(connectionId); try { log.info("Fetching table usage for connection: {}", connectionId); diff --git a/backend/src/main/java/com/dbaagent/controller/ProjectController.java b/backend/src/main/java/com/dbaagent/controller/ProjectController.java index 7ee190b..46766d7 100644 --- a/backend/src/main/java/com/dbaagent/controller/ProjectController.java +++ b/backend/src/main/java/com/dbaagent/controller/ProjectController.java @@ -2,6 +2,7 @@ import com.dbaagent.model.Project; import com.dbaagent.service.ProjectService; +import com.dbaagent.service.security.AccessControlService; import lombok.Data; import lombok.RequiredArgsConstructor; import org.springframework.http.ResponseEntity; @@ -9,11 +10,21 @@ import java.util.List; +/** + * REST API for projects, optionally filtered by connection. + * + *

Authorization: every endpoint here takes a caller-supplied connection id, so + * each one asserts access itself ({@code assertCanReadConnectionContent} for reads, + * {@code assertCanManageConnectionContent} for writes). {@code SecurityConfig} only + * requires an authenticated principal — nothing upstream inspects a connection id. See + * {@code ConnectionScopedAuthorizationSafetyTest}. + */ @RestController @RequestMapping("/projects") @RequiredArgsConstructor public class ProjectController { private final ProjectService projectService; + private final AccessControlService accessControlService; @PostMapping public ResponseEntity createProject(@RequestBody CreateProjectRequest request) { @@ -29,6 +40,9 @@ public ResponseEntity createProject(@RequestBody CreateProjectRequest r public ResponseEntity> listProjects( @RequestParam(required = false) String connectionId ) { + if (connectionId != null) { + accessControlService.assertCanReadConnectionContent(connectionId); + } List projects = connectionId != null ? projectService.getProjectsByConnection(connectionId) : projectService.getAllProjects(); diff --git a/backend/src/main/java/com/dbaagent/controller/QueryPerformanceController.java b/backend/src/main/java/com/dbaagent/controller/QueryPerformanceController.java index a368f32..fdb14ec 100644 --- a/backend/src/main/java/com/dbaagent/controller/QueryPerformanceController.java +++ b/backend/src/main/java/com/dbaagent/controller/QueryPerformanceController.java @@ -3,6 +3,7 @@ import com.dbaagent.model.QueryPerformanceHistory; import com.dbaagent.model.QueryPerformanceRegression; import com.dbaagent.service.QueryPerformanceService; +import com.dbaagent.service.security.AccessControlService; import lombok.extern.slf4j.Slf4j; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.http.ResponseEntity; @@ -12,6 +13,15 @@ import java.util.List; import java.util.Map; +/** + * REST API for per-query performance history, trends, and regressions. + * + *

Authorization: every endpoint here takes a caller-supplied connection id, so + * each one asserts access itself ({@code assertCanReadConnectionContent} for reads, + * {@code assertCanManageConnectionContent} for writes). {@code SecurityConfig} only + * requires an authenticated principal — nothing upstream inspects a connection id. See + * {@code ConnectionScopedAuthorizationSafetyTest}. + */ @RestController @RequestMapping("/query-performance") @Slf4j @@ -20,6 +30,9 @@ public class QueryPerformanceController { @Autowired private QueryPerformanceService queryPerformanceService; + @Autowired + private AccessControlService accessControlService; + /** * Record a query execution */ @@ -27,6 +40,7 @@ public class QueryPerformanceController { public ResponseEntity> recordQueryExecution(@RequestBody Map request) { try { String connectionId = (String) request.get("connectionId"); + accessControlService.assertCanManageConnectionContent(connectionId); String queryText = (String) request.get("queryText"); Double executionTimeMs = ((Number) request.get("executionTimeMs")).doubleValue(); Long rowsExamined = request.get("rowsExamined") != null ? @@ -62,6 +76,7 @@ public ResponseEntity> recordQueryExecution(@RequestBody Map @GetMapping("/queries/{connectionId}") public ResponseEntity> getTrackedQueries(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List> queries = queryPerformanceService.getTrackedQueries(connectionId); Map response = new HashMap<>(); @@ -91,6 +106,7 @@ public ResponseEntity> getQueryHistory( @RequestParam(defaultValue = "7") int days ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List history = queryPerformanceService.getQueryHistory( connectionId, queryHash, days ); @@ -122,6 +138,7 @@ public ResponseEntity> getPerformanceTrend( @RequestParam(defaultValue = "7") int days ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); Map trendData = queryPerformanceService.getPerformanceTrend( connectionId, queryHash, days ); @@ -151,6 +168,7 @@ public ResponseEntity> getRegressions( @RequestParam(defaultValue = "false") boolean unacknowledgedOnly ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List regressions = queryPerformanceService.getRegressions( connectionId, unacknowledgedOnly ); @@ -181,8 +199,8 @@ public ResponseEntity> acknowledgeRegression( @RequestBody(required = false) Map request ) { try { - String acknowledgedBy = request != null ? request.get("acknowledgedBy") : "user"; - queryPerformanceService.acknowledgeRegression(regressionId, acknowledgedBy); + assertCanManageRegression(regressionId); + queryPerformanceService.acknowledgeRegression(regressionId, actor()); Map response = new HashMap<>(); response.put("success", true); @@ -209,6 +227,7 @@ public ResponseEntity> resolveRegression( @RequestBody Map request ) { try { + assertCanManageRegression(regressionId); String resolutionNotes = request.get("resolutionNotes"); queryPerformanceService.resolveRegression(regressionId, resolutionNotes); @@ -234,6 +253,7 @@ public ResponseEntity> resolveRegression( @PostMapping("/analyze/{connectionId}") public ResponseEntity> triggerAnalysis(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); // This will be run async, so just trigger it new Thread(() -> { log.info("Manual performance analysis triggered for connection: {}", connectionId); @@ -255,4 +275,23 @@ public ResponseEntity> triggerAnalysis(@PathVariable String return ResponseEntity.badRequest().body(error); } } + + /** + * Authorize a write keyed only on a regression id. The regression carries + * its own connectionId, so resolve that and assert against it — a + * regression id is not a capability, and these ids are sequential Longs, + * so they are trivially enumerable. An unknown id reports 404 so the + * endpoint cannot be used to probe which regressions exist. + */ + private void assertCanManageRegression(Long regressionId) { + String connectionId = queryPerformanceService.findConnectionIdForRegression(regressionId) + .orElseThrow(() -> new org.springframework.web.server.ResponseStatusException( + org.springframework.http.HttpStatus.NOT_FOUND, "Regression not found")); + accessControlService.assertCanManageConnectionContent(connectionId); + } + + /** The authenticated caller. Never trust a client-supplied actor name. */ + private String actor() { + return accessControlService.requireCurrentUsername(); + } } diff --git a/backend/src/main/java/com/dbaagent/controller/QueryPlanController.java b/backend/src/main/java/com/dbaagent/controller/QueryPlanController.java index 5e5be9f..e847471 100644 --- a/backend/src/main/java/com/dbaagent/controller/QueryPlanController.java +++ b/backend/src/main/java/com/dbaagent/controller/QueryPlanController.java @@ -3,6 +3,7 @@ import com.dbaagent.model.QueryPlanCache; import com.dbaagent.model.QueryPlanComparison; import com.dbaagent.service.QueryPlanCacheService; +import com.dbaagent.service.security.AccessControlService; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.http.ResponseEntity; @@ -13,6 +14,12 @@ /** * REST API for query plan caching and comparison + * + *

Authorization: every endpoint here takes a caller-supplied connection id, so + * each one asserts access itself ({@code assertCanReadConnectionContent} for reads, + * {@code assertCanManageConnectionContent} for writes). {@code SecurityConfig} only + * requires an authenticated principal — nothing upstream inspects a connection id. See + * {@code ConnectionScopedAuthorizationSafetyTest}. */ @RestController @RequestMapping("/query-plans") @@ -21,6 +28,7 @@ public class QueryPlanController { private final QueryPlanCacheService planCacheService; + private final AccessControlService accessControlService; /** * Capture execution plan for a query @@ -29,6 +37,7 @@ public class QueryPlanController { public ResponseEntity capturePlan( @PathVariable String connectionId, @RequestBody Map request) { + accessControlService.assertCanManageConnectionContent(connectionId); String query = (String) request.get("query"); boolean analyze = Boolean.TRUE.equals(request.get("analyze")); @@ -46,6 +55,7 @@ public ResponseEntity capturePlan( */ @GetMapping("/{connectionId}/recent") public ResponseEntity> getRecentPlans(@PathVariable String connectionId) { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(planCacheService.getRecentPlans(connectionId)); } @@ -56,6 +66,7 @@ public ResponseEntity> getRecentPlans(@PathVariable String public ResponseEntity> getPlansForQuery( @PathVariable String connectionId, @PathVariable String queryHash) { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(planCacheService.getPlansForQuery(connectionId, queryHash)); } @@ -64,6 +75,7 @@ public ResponseEntity> getPlansForQuery( */ @PostMapping("/plans/{planId}/set-baseline") public ResponseEntity> setBaseline(@PathVariable String planId) { + assertCanManagePlan(planId); planCacheService.setBaseline(planId); return ResponseEntity.ok(Map.of( "status", "success", @@ -76,6 +88,7 @@ public ResponseEntity> setBaseline(@PathVariable String plan */ @GetMapping("/{connectionId}/regressions") public ResponseEntity> getRegressions(@PathVariable String connectionId) { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(planCacheService.getRegressions(connectionId)); } @@ -84,6 +97,7 @@ public ResponseEntity> getRegressions(@PathVariable St */ @GetMapping("/{connectionId}/regressions/unacknowledged") public ResponseEntity> getUnacknowledgedRegressions(@PathVariable String connectionId) { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(planCacheService.getUnacknowledgedRegressions(connectionId)); } @@ -95,8 +109,14 @@ public ResponseEntity> acknowledgeRegressions( @PathVariable String connectionId, @RequestBody List comparisonIds, @RequestParam(required = false, defaultValue = "user") String acknowledgedBy) { + accessControlService.assertCanManageConnectionContent(connectionId); - int count = planCacheService.acknowledgeRegressions(comparisonIds, acknowledgedBy); + if (!planCacheService.allComparisonsBelongTo(connectionId, comparisonIds)) { + throw new org.springframework.web.server.ResponseStatusException( + org.springframework.http.HttpStatus.NOT_FOUND, "Regression not found for this connection"); + } + int count = planCacheService.acknowledgeRegressions( + comparisonIds, accessControlService.requireCurrentUsername()); return ResponseEntity.ok(Map.of( "status", "success", "acknowledgedCount", count @@ -108,6 +128,19 @@ public ResponseEntity> acknowledgeRegressions( */ @GetMapping("/{connectionId}/stats") public ResponseEntity> getPlanStats(@PathVariable String connectionId) { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(planCacheService.getPlanStats(connectionId)); } + + /** + * Authorize a write keyed only on a plan id. The cached plan carries its own + * connectionId, so resolve that and assert against it. An unknown id reports + * 404 so the endpoint cannot be used to probe which plan ids exist. + */ + private void assertCanManagePlan(String planId) { + String connectionId = planCacheService.findConnectionIdForPlan(planId) + .orElseThrow(() -> new org.springframework.web.server.ResponseStatusException( + org.springframework.http.HttpStatus.NOT_FOUND, "Plan not found")); + accessControlService.assertCanManageConnectionContent(connectionId); + } } diff --git a/backend/src/main/java/com/dbaagent/controller/ResourceLimitsController.java b/backend/src/main/java/com/dbaagent/controller/ResourceLimitsController.java index 4792c55..050eb09 100644 --- a/backend/src/main/java/com/dbaagent/controller/ResourceLimitsController.java +++ b/backend/src/main/java/com/dbaagent/controller/ResourceLimitsController.java @@ -2,6 +2,7 @@ import com.dbaagent.model.ResourceLimits; import com.dbaagent.repository.ResourceLimitsRepository; +import com.dbaagent.service.security.AccessControlService; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.http.ResponseEntity; @@ -14,6 +15,12 @@ /** * Resource Limits Configuration Controller * Manages capacity limits for Sentinel-DBA analytics + * + *

Authorization: every endpoint here takes a caller-supplied connection id, so + * each one asserts access itself ({@code assertCanReadConnectionContent} for reads, + * {@code assertCanManageConnectionContent} for writes). {@code SecurityConfig} only + * requires an authenticated principal — nothing upstream inspects a connection id. See + * {@code ConnectionScopedAuthorizationSafetyTest}. */ @RestController @RequestMapping("/resource-limits") @@ -23,6 +30,7 @@ public class ResourceLimitsController { private final ResourceLimitsRepository resourceLimitsRepository; + private final AccessControlService accessControlService; /** * GET /api/resource-limits/{connectionId} @@ -31,6 +39,7 @@ public class ResourceLimitsController { @GetMapping("/{connectionId}") public ResponseEntity getResourceLimits(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); log.info("Fetching resource limits for connection: {}", connectionId); var limits = resourceLimitsRepository.findByConnectionId(connectionId); @@ -61,6 +70,7 @@ public ResponseEntity getResourceLimits(@PathVariable String connectionId) { @PostMapping public ResponseEntity saveResourceLimits(@RequestBody ResourceLimitsRequest request) { try { + accessControlService.assertCanManageConnectionContent(request.getConnectionId()); log.info("Saving resource limits for connection: {}", request.getConnectionId()); // Check if limits already exist @@ -120,6 +130,7 @@ public ResponseEntity saveResourceLimits(@RequestBody ResourceLimitsRequest r @DeleteMapping("/{connectionId}") public ResponseEntity deleteResourceLimits(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); log.info("Deleting resource limits for connection: {}", connectionId); resourceLimitsRepository.findByConnectionId(connectionId) diff --git a/backend/src/main/java/com/dbaagent/controller/SchemaChangeController.java b/backend/src/main/java/com/dbaagent/controller/SchemaChangeController.java index 4fe60a9..e289c25 100644 --- a/backend/src/main/java/com/dbaagent/controller/SchemaChangeController.java +++ b/backend/src/main/java/com/dbaagent/controller/SchemaChangeController.java @@ -4,6 +4,7 @@ import com.dbaagent.model.SchemaDriftConfig; import com.dbaagent.model.SchemaSnapshot; import com.dbaagent.service.SchemaChangeTrackingService; +import com.dbaagent.service.security.AccessControlService; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.http.ResponseEntity; @@ -14,6 +15,12 @@ /** * REST API for schema change tracking and drift detection + * + *

Authorization: every endpoint here takes a caller-supplied connection id, so + * each one asserts access itself ({@code assertCanReadConnectionContent} for reads, + * {@code assertCanManageConnectionContent} for writes). {@code SecurityConfig} only + * requires an authenticated principal — nothing upstream inspects a connection id. See + * {@code ConnectionScopedAuthorizationSafetyTest}. */ @RestController @RequestMapping("/schema-changes") @@ -23,6 +30,7 @@ public class SchemaChangeController { private final SchemaChangeTrackingService schemaChangeService; private final com.dbaagent.service.SchemaSnapshotService schemaSnapshotService; + private final AccessControlService accessControlService; // ==================== Snapshot Endpoints ==================== @@ -35,6 +43,7 @@ public ResponseEntity captureSnapshot( @RequestParam(required = false) String name, @RequestParam(required = false, defaultValue = "MANUAL") String type, @RequestParam(required = false) String notes) { + accessControlService.assertCanManageConnectionContent(connectionId); SchemaSnapshot.SnapshotType snapshotType = SchemaSnapshot.SnapshotType.valueOf(type.toUpperCase()); // Was: schemaChangeService.captureSnapshot — moved into @@ -50,6 +59,7 @@ public ResponseEntity captureSnapshot( */ @GetMapping("/{connectionId}/snapshots") public ResponseEntity> getSnapshots(@PathVariable String connectionId) { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(schemaChangeService.getSnapshots(connectionId)); } @@ -58,6 +68,7 @@ public ResponseEntity> getSnapshots(@PathVariable String co */ @GetMapping("/{connectionId}/snapshots/recent") public ResponseEntity> getRecentSnapshots(@PathVariable String connectionId) { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(schemaChangeService.getRecentSnapshots(connectionId)); } @@ -68,6 +79,7 @@ public ResponseEntity> getRecentSnapshots(@PathVariable Str public ResponseEntity> setBaseline( @PathVariable String connectionId, @PathVariable String snapshotId) { + accessControlService.assertCanManageConnectionContent(connectionId); schemaChangeService.setBaseline(connectionId, snapshotId); return ResponseEntity.ok(Map.of( @@ -84,6 +96,8 @@ public ResponseEntity> compareSnapshots( @RequestParam String snapshotId1, @RequestParam String snapshotId2) { + assertCanReadSnapshot(snapshotId1); + assertCanReadSnapshot(snapshotId2); return ResponseEntity.ok(schemaChangeService.compareSnapshots(snapshotId1, snapshotId2)); } @@ -94,6 +108,7 @@ public ResponseEntity> compareSnapshots( */ @GetMapping("/{connectionId}/changes") public ResponseEntity> getChanges(@PathVariable String connectionId) { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(schemaChangeService.getChanges(connectionId)); } @@ -102,6 +117,7 @@ public ResponseEntity> getChanges(@PathVariable String connec */ @GetMapping("/{connectionId}/changes/unacknowledged") public ResponseEntity> getUnacknowledgedChanges(@PathVariable String connectionId) { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(schemaChangeService.getUnacknowledgedChanges(connectionId)); } @@ -113,8 +129,10 @@ public ResponseEntity> acknowledgeChanges( @PathVariable String connectionId, @RequestBody List changeIds, @RequestParam(required = false, defaultValue = "user") String acknowledgedBy) { + accessControlService.assertCanManageConnectionContent(connectionId); - int count = schemaChangeService.acknowledgeChanges(changeIds, acknowledgedBy); + assertChangesBelongTo(connectionId, changeIds); + int count = schemaChangeService.acknowledgeChanges(changeIds, accessControlService.requireCurrentUsername()); return ResponseEntity.ok(Map.of( "status", "success", "acknowledgedCount", count @@ -128,8 +146,10 @@ public ResponseEntity> acknowledgeChanges( public ResponseEntity> acknowledgeAllChanges( @PathVariable String connectionId, @RequestParam(required = false, defaultValue = "user") String acknowledgedBy) { + accessControlService.assertCanManageConnectionContent(connectionId); - int count = schemaChangeService.acknowledgeAllChanges(connectionId, acknowledgedBy); + int count = schemaChangeService.acknowledgeAllChanges( + connectionId, accessControlService.requireCurrentUsername()); return ResponseEntity.ok(Map.of( "status", "success", "acknowledgedCount", count @@ -141,6 +161,7 @@ public ResponseEntity> acknowledgeAllChanges( */ @GetMapping("/{connectionId}/changes/stats") public ResponseEntity> getChangeStats(@PathVariable String connectionId) { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(schemaChangeService.getChangeStats(connectionId)); } @@ -151,6 +172,7 @@ public ResponseEntity> getChangeStats(@PathVariable String c */ @GetMapping("/{connectionId}/drift-config") public ResponseEntity getDriftConfig(@PathVariable String connectionId) { + accessControlService.assertCanReadConnectionContent(connectionId); return schemaChangeService.getDriftConfig(connectionId) .map(ResponseEntity::ok) .orElse(ResponseEntity.notFound().build()); @@ -163,6 +185,7 @@ public ResponseEntity getDriftConfig(@PathVariable String con public ResponseEntity configureDriftDetection( @PathVariable String connectionId, @RequestBody SchemaDriftConfig config) { + accessControlService.assertCanManageConnectionContent(connectionId); return ResponseEntity.ok(schemaChangeService.configureDriftDetection(connectionId, config)); } @@ -172,6 +195,7 @@ public ResponseEntity configureDriftDetection( */ @PostMapping("/{connectionId}/drift-check") public ResponseEntity> triggerDriftCheck(@PathVariable String connectionId) { + accessControlService.assertCanManageConnectionContent(connectionId); List changes = schemaChangeService.checkDrift(connectionId); return ResponseEntity.ok(Map.of( "status", "success", @@ -179,4 +203,28 @@ public ResponseEntity> triggerDriftCheck(@PathVariable Strin "changes", changes )); } + + /** + * Authorize a read keyed only on a snapshot id. The snapshot carries its own + * connectionId, so resolve that and assert against it. An unknown id reports + * 404 so the endpoint cannot be used to probe which snapshots exist. + */ + private void assertCanReadSnapshot(String snapshotId) { + String connectionId = schemaChangeService.findConnectionIdForSnapshot(snapshotId) + .orElseThrow(() -> new org.springframework.web.server.ResponseStatusException( + org.springframework.http.HttpStatus.NOT_FOUND, "Snapshot not found")); + accessControlService.assertCanReadConnectionContent(connectionId); + } + + /** + * The change ids arrive in the request body, so the path-variable check alone + * does not constrain them — a caller authorized on their own connection could + * otherwise acknowledge another connection's changes. + */ + private void assertChangesBelongTo(String connectionId, List changeIds) { + if (!schemaChangeService.allChangesBelongTo(connectionId, changeIds)) { + throw new org.springframework.web.server.ResponseStatusException( + org.springframework.http.HttpStatus.NOT_FOUND, "Change not found for this connection"); + } + } } diff --git a/backend/src/main/java/com/dbaagent/controller/SentinelAnalyticsController.java b/backend/src/main/java/com/dbaagent/controller/SentinelAnalyticsController.java index 9bb2e2f..7d126df 100644 --- a/backend/src/main/java/com/dbaagent/controller/SentinelAnalyticsController.java +++ b/backend/src/main/java/com/dbaagent/controller/SentinelAnalyticsController.java @@ -5,6 +5,7 @@ import com.dbaagent.model.SentinelRecommendation; import com.dbaagent.service.EventCorrelationService; import com.dbaagent.service.SentinelAnalyticsService; +import com.dbaagent.service.security.AccessControlService; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.http.ResponseEntity; @@ -23,16 +24,22 @@ * - Contextual Attribution: Event correlation * - Velocity & Acceleration: Growth derivatives * - AI Recommendations: Actionable insights + * + *

Authorization: every endpoint here takes a caller-supplied connection id, so + * each one asserts access itself ({@code assertCanReadConnectionContent} for reads, + * {@code assertCanManageConnectionContent} for writes). {@code SecurityConfig} only + * requires an authenticated principal — nothing upstream inspects a connection id. See + * {@code ConnectionScopedAuthorizationSafetyTest}. */ @RestController @RequestMapping("/sentinel") @RequiredArgsConstructor @Slf4j -@CrossOrigin(origins = "*") public class SentinelAnalyticsController { private final SentinelAnalyticsService sentinelAnalytics; private final EventCorrelationService eventCorrelation; + private final AccessControlService accessControlService; /** * GET /api/sentinel/death-clock/{connectionId} @@ -41,6 +48,7 @@ public class SentinelAnalyticsController { */ @GetMapping("/death-clock/{connectionId}") public ResponseEntity> getDeathClock(@PathVariable String connectionId) { + accessControlService.assertCanReadConnectionContent(connectionId); try { log.info("Fetching Death Clock for connection: {}", connectionId); @@ -72,6 +80,7 @@ public ResponseEntity> getDeathClock(@PathVariable String co public ResponseEntity> getForecasts( @PathVariable String connectionId, @RequestParam(required = false) String tableName) { + accessControlService.assertCanReadConnectionContent(connectionId); try { log.info("Fetching forecasts for connection: {}, table: {}", connectionId, tableName); @@ -112,6 +121,7 @@ public ResponseEntity> getForecasts( public ResponseEntity> generateForecast( @PathVariable String connectionId, @RequestParam String tableName) { + accessControlService.assertCanManageConnectionContent(connectionId); try { log.info("Generating forecast for connection: {}, table: {}", connectionId, tableName); @@ -145,6 +155,7 @@ public ResponseEntity> getVelocityAndAcceleration( @PathVariable String connectionId, @RequestParam String tableName, @RequestParam(defaultValue = "30") int historicalDays) { + accessControlService.assertCanReadConnectionContent(connectionId); try { log.info("Calculating velocity/acceleration for table: {}", tableName); @@ -181,6 +192,7 @@ public ResponseEntity> getVelocityAndAcceleration( public ResponseEntity> getEvents( @PathVariable String connectionId, @RequestParam(required = false) Integer days) { + accessControlService.assertCanReadConnectionContent(connectionId); try { int daysToFetch = days != null ? days : 30; @@ -215,6 +227,7 @@ public ResponseEntity> getEvents( public ResponseEntity> logDeploymentEvent(@RequestBody Map eventData) { try { String connectionId = (String) eventData.get("connectionId"); + accessControlService.assertCanManageConnectionContent(connectionId); String deploymentVersion = (String) eventData.get("deploymentVersion"); String deploymentTag = (String) eventData.get("deploymentTag"); @SuppressWarnings("unchecked") @@ -256,6 +269,7 @@ public ResponseEntity> logDeploymentEvent(@RequestBody Map> logSchemaChangeEvent(@RequestBody Map eventData) { try { String connectionId = (String) eventData.get("connectionId"); + accessControlService.assertCanManageConnectionContent(connectionId); String tableName = (String) eventData.get("tableName"); String changeType = (String) eventData.get("changeType"); String description = (String) eventData.get("description"); @@ -297,6 +311,7 @@ public ResponseEntity> getRecommendations( @PathVariable String connectionId, @RequestParam(required = false) String status, @RequestParam(required = false) String priority) { + accessControlService.assertCanReadConnectionContent(connectionId); try { log.info("Fetching recommendations for connection: {} (status: {}, priority: {})", @@ -342,8 +357,9 @@ public ResponseEntity> updateRecommendationStatus( @RequestBody Map statusData) { try { + assertCanManageRecommendation(recommendationId); String status = statusData.get("status"); - String updatedBy = statusData.get("updatedBy"); + String updatedBy = accessControlService.requireCurrentUsername(); log.info("Updating recommendation {} status to: {}", recommendationId, status); @@ -376,6 +392,7 @@ public ResponseEntity> updateRecommendationStatus( */ @GetMapping("/summary/{connectionId}") public ResponseEntity> getSentinelSummary(@PathVariable String connectionId) { + accessControlService.assertCanReadConnectionContent(connectionId); try { log.info("Generating Sentinel-DBA summary for connection: {}", connectionId); @@ -505,4 +522,17 @@ private String generateExecutiveSummary( pendingRecs.size() ); } + + /** + * Authorize a write keyed only on a recommendation id. The recommendation + * carries its own connectionId, so resolve that and assert against it — a + * recommendation id is not a capability. An unknown id reports 404 so the + * endpoint cannot be used to probe which recommendations exist. + */ + private void assertCanManageRecommendation(String recommendationId) { + String connectionId = sentinelAnalytics.findConnectionIdForRecommendation(recommendationId) + .orElseThrow(() -> new org.springframework.web.server.ResponseStatusException( + org.springframework.http.HttpStatus.NOT_FOUND, "Recommendation not found")); + accessControlService.assertCanManageConnectionContent(connectionId); + } } diff --git a/backend/src/main/java/com/dbaagent/controller/SentinelDemoDataController.java b/backend/src/main/java/com/dbaagent/controller/SentinelDemoDataController.java index f71ffb6..07675d7 100644 --- a/backend/src/main/java/com/dbaagent/controller/SentinelDemoDataController.java +++ b/backend/src/main/java/com/dbaagent/controller/SentinelDemoDataController.java @@ -4,6 +4,7 @@ import com.dbaagent.repository.*; import com.dbaagent.service.EventCorrelationService; import com.dbaagent.service.SentinelAnalyticsService; +import com.dbaagent.service.security.AccessControlService; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.http.ResponseEntity; @@ -15,6 +16,12 @@ /** * Demo Data Generator for Sentinel-DBA * Creates sample resource limits, forecasts, events, and recommendations + * + *

Authorization: every endpoint here takes a caller-supplied connection id, so + * each one asserts access itself ({@code assertCanReadConnectionContent} for reads, + * {@code assertCanManageConnectionContent} for writes). {@code SecurityConfig} only + * requires an authenticated principal — nothing upstream inspects a connection id. See + * {@code ConnectionScopedAuthorizationSafetyTest}. */ @RestController @RequestMapping("/sentinel/demo") @@ -29,6 +36,7 @@ public class SentinelDemoDataController { private final SentinelRecommendationRepository recommendationRepository; private final EventCorrelationService eventCorrelation; private final SentinelAnalyticsService sentinelAnalytics; + private final AccessControlService accessControlService; /** * POST /api/sentinel/demo/generate/{connectionId} @@ -37,6 +45,7 @@ public class SentinelDemoDataController { @PostMapping("/generate/{connectionId}") public ResponseEntity> generateDemoData(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); log.info("Generating Sentinel demo data for connection: {}", connectionId); Map results = new HashMap<>(); @@ -81,6 +90,7 @@ public ResponseEntity> generateDemoData(@PathVariable String @DeleteMapping("/cleanup/{connectionId}") public ResponseEntity> cleanupDemoData(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); log.info("Cleaning up Sentinel demo data for connection: {}", connectionId); // Delete in reverse order of dependencies diff --git a/backend/src/main/java/com/dbaagent/controller/SlowQueryAnalyticsController.java b/backend/src/main/java/com/dbaagent/controller/SlowQueryAnalyticsController.java index e3e91bb..525d841 100644 --- a/backend/src/main/java/com/dbaagent/controller/SlowQueryAnalyticsController.java +++ b/backend/src/main/java/com/dbaagent/controller/SlowQueryAnalyticsController.java @@ -5,6 +5,7 @@ import com.dbaagent.repository.ConnectionAnalyticsConfigRepository; import com.dbaagent.service.SlowQueryAnalyticsService; import com.dbaagent.service.SlowQueryDailyAnalysisService; +import com.dbaagent.service.security.AccessControlService; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.format.annotation.DateTimeFormat; @@ -21,6 +22,12 @@ * Read endpoints serve the per-query timeline, regressions, and per-customer * breakdown the UI / MCP / CLI consume. Write endpoints manage the * per-connection analytics config and trigger an on-demand analysis. + * + *

Authorization: every endpoint here takes a caller-supplied connection id, so + * each one asserts access itself ({@code assertCanReadConnectionContent} for reads, + * {@code assertCanManageConnectionContent} for writes). {@code SecurityConfig} only + * requires an authenticated principal — nothing upstream inspects a connection id. See + * {@code ConnectionScopedAuthorizationSafetyTest}. */ @RestController @RequestMapping("/slow-query-analytics") @@ -31,11 +38,13 @@ public class SlowQueryAnalyticsController { private final SlowQueryAnalyticsService analyticsService; private final SlowQueryDailyAnalysisService dailyAnalysisService; private final ConnectionAnalyticsConfigRepository configRepository; + private final AccessControlService accessControlService; /** Every tracked query for a connection, as of the most recent analysis run. */ @GetMapping("/{connectionId}/queries") public ResponseEntity> queries( @PathVariable String connectionId) { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(analyticsService.listQueries(connectionId)); } @@ -44,6 +53,7 @@ public ResponseEntity> queries( public ResponseEntity> timeline( @PathVariable String connectionId, @PathVariable String fingerprint) { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(analyticsService.timeline(connectionId, fingerprint)); } @@ -56,6 +66,7 @@ public ResponseEntity> regressio @PathVariable String connectionId, @RequestParam(required = false) @DateTimeFormat(iso = DateTimeFormat.ISO.DATE) LocalDate day, @RequestParam(required = false, defaultValue = "1.5") double minFactor) { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(analyticsService.regressions(connectionId, day, minFactor)); } @@ -63,6 +74,7 @@ public ResponseEntity> regressio @GetMapping("/{connectionId}/customers") public ResponseEntity> listCustomers( @PathVariable String connectionId) { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(analyticsService.listCustomers(connectionId)); } @@ -71,6 +83,7 @@ public ResponseEntity> listCusto public ResponseEntity> queriesForCustomer( @PathVariable String connectionId, @PathVariable String customerId) { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(analyticsService.queriesForCustomer(connectionId, customerId)); } @@ -80,6 +93,7 @@ public ResponseEntity> samplesForCus @PathVariable String connectionId, @PathVariable String customerId, @PathVariable String fingerprint) { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok( analyticsService.samplesForCustomerQuery(connectionId, customerId, fingerprint)); } @@ -89,6 +103,7 @@ public ResponseEntity> samplesForCus public ResponseEntity> samples( @PathVariable String connectionId, @PathVariable String fingerprint) { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(analyticsService.querySamples(connectionId, fingerprint)); } @@ -98,6 +113,7 @@ public ResponseEntity> custome @PathVariable String connectionId, @PathVariable String fingerprint, @RequestParam(required = false) @DateTimeFormat(iso = DateTimeFormat.ISO.DATE) LocalDate day) { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(analyticsService.customerBreakdown(connectionId, fingerprint, day)); } @@ -110,6 +126,7 @@ public ResponseEntity> custome public ResponseEntity> tenantColumnSuggestions( @PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(analyticsService.suggestTenantColumns(connectionId)); } catch (org.springframework.web.server.ResponseStatusException e) { throw e; @@ -127,6 +144,7 @@ public ResponseEntity> te */ @GetMapping("/{connectionId}/config") public ResponseEntity getConfig(@PathVariable String connectionId) { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(analyticsService.effectiveConfig(connectionId)); } @@ -135,6 +153,7 @@ public ResponseEntity getConfig(@PathVariable String public ResponseEntity putConfig( @PathVariable String connectionId, @RequestBody ConnectionAnalyticsConfig body) { + accessControlService.assertCanManageConnectionContent(connectionId); ConnectionAnalyticsConfig cfg = configRepository.findById(connectionId) .orElseGet(() -> ConnectionAnalyticsConfig.builder() .connectionId(connectionId) @@ -163,6 +182,7 @@ public ResponseEntity putConfig( @PostMapping("/{connectionId}/analyze-now") public ResponseEntity> analyzeNow(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); SlowQueryHistory header = dailyAnalysisService.analyzeAndPersist(connectionId); if (header != null) { return ResponseEntity.ok(Map.of( @@ -206,6 +226,7 @@ public ResponseEntity> analyzeNow(@PathVariable String conne @DeleteMapping("/{connectionId}/reset") public ResponseEntity> reset(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); analyticsService.resetAnalytics(connectionId); return ResponseEntity.ok(Map.of("success", true, "connectionId", connectionId)); } catch (org.springframework.web.server.ResponseStatusException e) { diff --git a/backend/src/main/java/com/dbaagent/controller/SlowQueryController.java b/backend/src/main/java/com/dbaagent/controller/SlowQueryController.java index 10b143e..1631e16 100644 --- a/backend/src/main/java/com/dbaagent/controller/SlowQueryController.java +++ b/backend/src/main/java/com/dbaagent/controller/SlowQueryController.java @@ -11,6 +11,7 @@ import com.dbaagent.model.SlowQueryAnalysis; import com.dbaagent.model.SlowQueryHistory; import com.dbaagent.service.*; +import com.dbaagent.service.security.AccessControlService; import com.fasterxml.jackson.databind.ObjectMapper; import lombok.Data; import lombok.RequiredArgsConstructor; @@ -32,6 +33,12 @@ /** * REST API for slow query analysis + * + *

Authorization: every endpoint here takes a caller-supplied connection id, so + * each one asserts access itself ({@code assertCanReadConnectionContent} for reads, + * {@code assertCanManageConnectionContent} for writes). {@code SecurityConfig} only + * requires an authenticated principal — nothing upstream inspects a connection id. See + * {@code ConnectionScopedAuthorizationSafetyTest}. */ @RestController @RequestMapping("/slow-queries") @@ -54,6 +61,7 @@ public class SlowQueryController { private final KeyCustomerService keyCustomerService; private final SlowQueryInsightsService slowQueryInsightsService; private final ObjectMapper objectMapper; + private final AccessControlService accessControlService; // Thread pool for SSE streaming — keeps SSE work off the Jetty request thread private static final ExecutorService sseExecutor = @@ -70,6 +78,7 @@ public class SlowQueryController { public ResponseEntity analyzeSlowQueries( @RequestBody SlowQueryRequest request ) { + accessControlService.assertCanManageConnectionContent(request.getConnectionId()); try { log.info("Slow query analysis requested for connection: {}", request.getConnectionId()); @@ -107,6 +116,7 @@ public ResponseEntity analyzeSlowQueriesSimple( @RequestParam(required = false, defaultValue = "100") Double threshold, @RequestParam(required = false, defaultValue = "10") Integer limit ) { + accessControlService.assertCanReadConnectionContent(connectionId); try { log.info("Slow query analysis requested for connection: {}", connectionId); @@ -142,6 +152,7 @@ public ResponseEntity analyzeSlowQueriesSimple( public ResponseEntity saveHistory( @RequestBody SaveHistoryRequest request ) { + accessControlService.assertCanManageConnectionContent(request.getConnectionId()); try { log.info("Saving slow query history for connection: {}", request.getConnectionId()); @@ -169,6 +180,7 @@ public ResponseEntity saveHistory( public ResponseEntity> getHistory( @PathVariable String connectionId ) { + accessControlService.assertCanReadConnectionContent(connectionId); try { log.info("Fetching slow query history summaries for connection: {}", connectionId); @@ -195,6 +207,7 @@ public ResponseEntity> getHistory( public ResponseEntity getLatestAnalysis( @PathVariable String connectionId ) { + accessControlService.assertCanReadConnectionContent(connectionId); try { log.info("Fetching latest slow query analysis for connection: {}", connectionId); @@ -227,6 +240,7 @@ public ResponseEntity> getHistoryByTimeRange( @PathVariable String connectionId, @PathVariable String timeRange ) { + accessControlService.assertCanReadConnectionContent(connectionId); try { log.info("Fetching slow query history summaries for connection: {} and timeRange: {}", connectionId, timeRange); @@ -252,6 +266,7 @@ public ResponseEntity> getHistoryByTimeRange( public ResponseEntity getHistoryById( @PathVariable String id ) { + assertCanReadHistory(id); try { log.info("Fetching slow query history item: {}", id); @@ -280,6 +295,7 @@ public ResponseEntity getHistoryById( public ResponseEntity> deleteHistory( @PathVariable String id ) { + assertCanManageHistory(id); try { log.info("Deleting slow query history: {}", id); historyService.deleteHistory(id); @@ -301,6 +317,7 @@ public ResponseEntity> deleteHistory( public ResponseEntity> deleteAllHistory( @PathVariable String connectionId ) { + accessControlService.assertCanManageConnectionContent(connectionId); try { log.info("Deleting all slow query history for connection: {}", connectionId); historyService.deleteAllForConnection(connectionId); @@ -324,6 +341,7 @@ public ResponseEntity analyzeSlowQueryLogFile( @RequestParam("connectionId") String connectionId, @RequestParam(required = false, defaultValue = "mysql") String databaseType ) { + accessControlService.assertCanManageConnectionContent(connectionId); try { log.info("Analyzing uploaded slow query log file for connection: {}, type: {}", connectionId, databaseType); @@ -366,6 +384,7 @@ public ResponseEntity analyzeSlowQueryLogFile( public ResponseEntity analyzeSlowQueryLogFileFromS3( @RequestBody S3LogRequest request ) { + accessControlService.assertCanManageConnectionContent(request.getConnectionId()); try { log.info("Analyzing S3 slow query log file for connection: {}, url: {}", request.getConnectionId(), request.getS3Url()); @@ -410,6 +429,7 @@ public ResponseEntity analyzeSlowQueryLogFileFromS3( public ResponseEntity analyzeSlowQueryLogFromCloudWatch( @RequestBody CloudWatchLogRequest request ) { + accessControlService.assertCanManageConnectionContent(request.getConnectionId()); try { log.info("Analyzing CloudWatch slow query logs for connection: {}, log group: {}", request.getConnectionId(), request.getLogGroupName()); @@ -475,6 +495,7 @@ public ResponseEntity getKeyCustomers( @PathVariable String connectionId, @RequestParam(defaultValue = "20") int limit, @RequestParam(required = false) String tableName) { + accessControlService.assertCanReadConnectionContent(connectionId); try { return keyCustomerService.analyze(connectionId, limit, tableName) .map(ResponseEntity::ok) @@ -496,6 +517,7 @@ public ResponseEntity getInsights( @RequestParam(defaultValue = "7d") String window, @RequestParam(defaultValue = "20") int limit ) { + accessControlService.assertCanReadConnectionContent(connectionId); try { SlowQueryInsightsResponse response = slowQueryInsightsService.getInsights(connectionId, window, limit); return ResponseEntity.ok(response); @@ -516,6 +538,7 @@ public ResponseEntity getRemediat @RequestParam(defaultValue = "7d") String window, @RequestParam(defaultValue = "20") int limit ) { + accessControlService.assertCanReadConnectionContent(connectionId); try { SlowQueryInsightsResponse.RemediationInsights response = slowQueryInsightsService.getRemediationInsights(connectionId, window, limit); @@ -537,6 +560,7 @@ public ResponseEntity getHotspotInsig @RequestParam(defaultValue = "7d") String window, @RequestParam(defaultValue = "20") int limit ) { + accessControlService.assertCanReadConnectionContent(connectionId); try { SlowQueryInsightsResponse.HotspotInsights response = slowQueryInsightsService.getHotspotInsights(connectionId, window, limit); @@ -558,6 +582,7 @@ public ResponseEntity getSkewInsights( @RequestParam(defaultValue = "7d") String window, @RequestParam(defaultValue = "20") int limit ) { + accessControlService.assertCanReadConnectionContent(connectionId); try { SlowQueryInsightsResponse.SkewInsights response = slowQueryInsightsService.getSkewInsights(connectionId, window, limit); @@ -579,6 +604,7 @@ public ResponseEntity getTailRiskIns @RequestParam(defaultValue = "7d") String window, @RequestParam(defaultValue = "20") int limit ) { + accessControlService.assertCanReadConnectionContent(connectionId); try { SlowQueryInsightsResponse.TailRiskInsights response = slowQueryInsightsService.getTailRiskInsights(connectionId, window, limit); @@ -600,6 +626,7 @@ public ResponseEntity getPlanDriftI @RequestParam(defaultValue = "7d") String window, @RequestParam(defaultValue = "20") int limit ) { + accessControlService.assertCanReadConnectionContent(connectionId); try { SlowQueryInsightsResponse.PlanDriftInsights response = slowQueryInsightsService.getPlanDriftInsights(connectionId, window, limit); @@ -675,6 +702,7 @@ private HistorySummaryResponse convertSummaryToResponse(SlowQueryHistorySummary public ResponseEntity optimizeQuery( @RequestBody OptimizeQueryRequest request ) { + accessControlService.assertCanManageConnectionContent(request.getConnectionId()); try { log.info("Generating AI optimization for connection: {}", request.getConnectionId()); @@ -735,6 +763,7 @@ public SseEmitter streamOptimize( @RequestParam(required = false) String queryId, @RequestParam(defaultValue = "false") boolean forceRefresh ) { + accessControlService.assertCanReadConnectionContent(connectionId); SseEmitter emitter = new SseEmitter(300_000L); // 5-minute timeout sseExecutor.submit(() -> { @@ -913,6 +942,7 @@ public ResponseEntity> batchOp @PathVariable String connectionId, @RequestParam(defaultValue = "5") int limit ) { + accessControlService.assertCanManageConnectionContent(connectionId); try { log.info("Batch optimizing slow queries for connection: {}", connectionId); @@ -946,6 +976,7 @@ public ResponseEntity getOptimizationCandidates( @PathVariable String connectionId, @PathVariable String queryFingerprint ) { + accessControlService.assertCanReadConnectionContent(connectionId); try { List candidates = candidateService.getCandidates(connectionId, queryFingerprint); @@ -975,6 +1006,7 @@ public ResponseEntity benchmarkCandidates( @PathVariable String queryFingerprint, @RequestBody(required = false) BenchmarkCandidatesRequest request ) { + accessControlService.assertCanManageConnectionContent(connectionId); try { Integer runs = request != null ? request.getRuns() : null; Integer timeoutMs = request != null ? request.getTimeoutMs() : null; @@ -999,6 +1031,7 @@ public ResponseEntity getCachedOpti @PathVariable String connectionId, @PathVariable String queryFingerprint ) { + accessControlService.assertCanReadConnectionContent(connectionId); try { QueryOptimizationService.OptimizationResult cached = optimizationService.getCachedOptimization(connectionId, queryFingerprint); @@ -1024,6 +1057,7 @@ public ResponseEntity> @PathVariable String connectionId, @RequestBody List fingerprints ) { + accessControlService.assertCanReadConnectionContent(connectionId); try { Map cached = optimizationService.getCachedOptimizations(connectionId, fingerprints); @@ -1044,6 +1078,7 @@ public ResponseEntity> public ResponseEntity> getOptimizationCacheStats( @PathVariable String connectionId ) { + accessControlService.assertCanReadConnectionContent(connectionId); try { Map stats = optimizationService.getCacheStats(connectionId); return ResponseEntity.ok(stats); @@ -1062,6 +1097,7 @@ public ResponseEntity> getOptimizationCacheStats( public ResponseEntity clearOptimizationCache( @PathVariable String connectionId ) { + accessControlService.assertCanManageConnectionContent(connectionId); try { optimizationService.clearConnectionCache(connectionId); return ResponseEntity.ok().build(); @@ -1082,6 +1118,7 @@ public ResponseEntity clearOptimizationCache( public ResponseEntity getAlertSummary( @PathVariable String connectionId ) { + accessControlService.assertCanReadConnectionContent(connectionId); try { SlowQueryAlertService.SlowQueryAlertSummary summary = alertService.getAlertSummary(connectionId); return ResponseEntity.ok(summary); @@ -1099,10 +1136,15 @@ public ResponseEntity getAlertSumma @PostMapping("/alerts/{alertId}/acknowledge") public ResponseEntity acknowledgeAlert( @PathVariable String alertId, - @RequestParam String userId + @RequestParam(required = false) String userId ) { + assertCanManageAlert(alertId); try { - PlaybookAlert alert = alertService.acknowledgeAlert(alertId, userId); + // The actor is the authenticated caller, never the userId query parameter: + // that was client-supplied, so the acknowledgement trail could name anyone. + // The parameter is still accepted so existing callers do not break, and ignored. + PlaybookAlert alert = alertService.acknowledgeAlert( + alertId, accessControlService.requireCurrentUsername()); return ResponseEntity.ok(alert); } catch (IllegalArgumentException e) { return ResponseEntity.notFound().build(); @@ -1120,10 +1162,12 @@ public ResponseEntity acknowledgeAlert( @PostMapping("/alerts/{connectionId}/acknowledge-all") public ResponseEntity> acknowledgeAllAlerts( @PathVariable String connectionId, - @RequestParam String userId + @RequestParam(required = false) String userId ) { + accessControlService.assertCanManageConnectionContent(connectionId); try { - int count = alertService.acknowledgeAllAlerts(connectionId, userId); + int count = alertService.acknowledgeAllAlerts( + connectionId, accessControlService.requireCurrentUsername()); return ResponseEntity.ok(Map.of("acknowledged", count)); } catch (org.springframework.web.server.ResponseStatusException e) { throw e; @@ -1141,6 +1185,7 @@ public ResponseEntity> processAlertsFromAnalysis( @PathVariable String connectionId, @RequestBody(required = false) AlertConfigRequest config ) { + accessControlService.assertCanManageConnectionContent(connectionId); try { Optional latestOpt = historyService.getLatestHistory(connectionId); if (latestOpt.isEmpty()) { @@ -1181,6 +1226,8 @@ public ResponseEntity> compareAnalyses( @RequestParam String historyId1, @RequestParam String historyId2 ) { + assertCanReadHistory(historyId1); + assertCanReadHistory(historyId2); try { Optional history1Opt = historyService.getHistoryById(historyId1); Optional history2Opt = historyService.getHistoryById(historyId2); @@ -1280,6 +1327,7 @@ public ResponseEntity> compareAnalyses( public ResponseEntity getDashboardWidgets( @PathVariable String connectionId ) { + accessControlService.assertCanReadConnectionContent(connectionId); try { SlowQueryDashboardService.DashboardWidgetData data = dashboardService.getWidgetData(connectionId); return ResponseEntity.ok(data); @@ -1298,6 +1346,7 @@ public ResponseEntity getDashboar public ResponseEntity getOverviewWidget( @PathVariable String connectionId ) { + accessControlService.assertCanReadConnectionContent(connectionId); try { SlowQueryDashboardService.OverviewWidget data = dashboardService.getOverviewWidget(connectionId); return ResponseEntity.ok(data); @@ -1316,6 +1365,7 @@ public ResponseEntity getOverviewWidge public ResponseEntity getTrendWidget( @PathVariable String connectionId ) { + accessControlService.assertCanReadConnectionContent(connectionId); try { SlowQueryDashboardService.TrendWidget data = dashboardService.getTrendWidget(connectionId); return ResponseEntity.ok(data); @@ -1336,6 +1386,7 @@ public ResponseEntity getTrendWidget( public ResponseEntity getFingerprintSummary( @PathVariable String connectionId ) { + accessControlService.assertCanReadConnectionContent(connectionId); try { QueryFingerprintService.FingerprintSummary summary = fingerprintService.getSummary(connectionId); return ResponseEntity.ok(summary); @@ -1358,6 +1409,7 @@ public ResponseEntity> getFingerprints( @RequestParam(required = false) Boolean regressingOnly, @RequestParam(defaultValue = "50") int limit ) { + accessControlService.assertCanReadConnectionContent(connectionId); try { QueryFingerprint.TrendDirection direction = null; if (trendDirection != null && !trendDirection.isBlank()) { @@ -1384,6 +1436,7 @@ public ResponseEntity> getFingerprints( public ResponseEntity getFingerprintTrend( @PathVariable String fingerprintId ) { + assertCanReadFingerprint(fingerprintId); try { QueryFingerprintService.FingerprintTrend trend = fingerprintService.getTrend(fingerprintId); return ResponseEntity.ok(trend); @@ -1404,6 +1457,7 @@ public ResponseEntity getFingerprintTr public ResponseEntity resetFingerprintBaseline( @PathVariable String fingerprintId ) { + assertCanManageFingerprint(fingerprintId); try { QueryFingerprint fingerprint = fingerprintService.resetBaseline(fingerprintId); return ResponseEntity.ok(fingerprint); @@ -1424,6 +1478,7 @@ public ResponseEntity resetFingerprintBaseline( public ResponseEntity> processFingerprints( @PathVariable String connectionId ) { + accessControlService.assertCanManageConnectionContent(connectionId); try { Optional latestOpt = historyService.getLatestHistory(connectionId); if (latestOpt.isEmpty()) { @@ -1450,6 +1505,7 @@ public ResponseEntity> processFingerprints( public ResponseEntity> getExplainPlan( @RequestBody ExplainQueryRequest request ) { + accessControlService.assertCanManageConnectionContent(request.getConnectionId()); try { log.info("Running EXPLAIN for query in connection: {}", request.getConnectionId()); @@ -1482,6 +1538,7 @@ public ResponseEntity>> getCriticalQueryExplains( @PathVariable String connectionId, @RequestParam(defaultValue = "5") int limit ) { + accessControlService.assertCanReadConnectionContent(connectionId); try { Optional latestOpt = historyService.getLatestHistory(connectionId); if (latestOpt.isEmpty()) { @@ -1712,4 +1769,47 @@ public static class HistorySummaryResponse { private Double totalDatabaseTimeMs; private String timestamp; } + + // ── authorization helpers for endpoints keyed on a non-connection id ────── + // + // An id is not a capability: each of these entities carries its own + // connectionId, so resolve the owner and assert against that. An unknown id + // reports 404 rather than 403, so none of these can be used to probe which + // ids exist on connections the caller cannot see. + + private String historyConnectionId(String historyId) { + return historyService.getHistoryById(historyId) + .map(com.dbaagent.model.SlowQueryHistory::getConnectionId) + .orElseThrow(() -> new org.springframework.web.server.ResponseStatusException( + org.springframework.http.HttpStatus.NOT_FOUND, "Analysis not found")); + } + + private void assertCanReadHistory(String historyId) { + accessControlService.assertCanReadConnectionContent(historyConnectionId(historyId)); + } + + private void assertCanManageHistory(String historyId) { + accessControlService.assertCanManageConnectionContent(historyConnectionId(historyId)); + } + + private void assertCanManageAlert(String alertId) { + String connectionId = alertService.findConnectionIdForAlert(alertId) + .orElseThrow(() -> new org.springframework.web.server.ResponseStatusException( + org.springframework.http.HttpStatus.NOT_FOUND, "Alert not found")); + accessControlService.assertCanManageConnectionContent(connectionId); + } + + private String fingerprintConnectionId(String fingerprintId) { + return fingerprintService.findConnectionIdForFingerprintId(fingerprintId) + .orElseThrow(() -> new org.springframework.web.server.ResponseStatusException( + org.springframework.http.HttpStatus.NOT_FOUND, "Fingerprint not found")); + } + + private void assertCanReadFingerprint(String fingerprintId) { + accessControlService.assertCanReadConnectionContent(fingerprintConnectionId(fingerprintId)); + } + + private void assertCanManageFingerprint(String fingerprintId) { + accessControlService.assertCanManageConnectionContent(fingerprintConnectionId(fingerprintId)); + } } diff --git a/backend/src/main/java/com/dbaagent/controller/StatsController.java b/backend/src/main/java/com/dbaagent/controller/StatsController.java index a7a4aa4..3f4388a 100644 --- a/backend/src/main/java/com/dbaagent/controller/StatsController.java +++ b/backend/src/main/java/com/dbaagent/controller/StatsController.java @@ -3,6 +3,7 @@ import com.dbaagent.model.DbaStats; import com.dbaagent.service.CredentialService; import com.dbaagent.service.StatsCollectorService; +import com.dbaagent.service.security.AccessControlService; import lombok.RequiredArgsConstructor; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; @@ -12,15 +13,26 @@ import java.util.HashMap; import java.util.Map; +/** + * REST API for a connection's live database statistics. + * + *

Authorization: every endpoint here takes a caller-supplied connection id, so + * each one asserts access itself ({@code assertCanReadConnectionContent} for reads, + * {@code assertCanManageConnectionContent} for writes). {@code SecurityConfig} only + * requires an authenticated principal — nothing upstream inspects a connection id. See + * {@code ConnectionScopedAuthorizationSafetyTest}. + */ @RestController @RequestMapping("/connections/{connectionId}/stats") @RequiredArgsConstructor public class StatsController { private final StatsCollectorService statsCollectorService; private final CredentialService credentialService; + private final AccessControlService accessControlService; @GetMapping public ResponseEntity> getStats(@PathVariable String connectionId) { + accessControlService.assertCanReadConnectionContent(connectionId); Map response = new HashMap<>(); try { if (!credentialService.connectionExists(connectionId)) { diff --git a/backend/src/main/java/com/dbaagent/service/BusinessRuleMemoryService.java b/backend/src/main/java/com/dbaagent/service/BusinessRuleMemoryService.java index a7b033e..60b61bf 100644 --- a/backend/src/main/java/com/dbaagent/service/BusinessRuleMemoryService.java +++ b/backend/src/main/java/com/dbaagent/service/BusinessRuleMemoryService.java @@ -462,6 +462,14 @@ public List getActiveRules(String connectionId) { } @Transactional + /** + * The connection a rule belongs to, for authorizing an endpoint keyed only + * on a rule id. Empty when the id does not exist. + */ + public java.util.Optional findConnectionIdForRule(String ruleId) { + return brainRuleRepository.findById(ruleId).map(r -> r.getConnectionId()); + } + public boolean deactivateRule(String ruleId) { if (ruleId == null || ruleId.isBlank()) { return false; diff --git a/backend/src/main/java/com/dbaagent/service/QueryFingerprintService.java b/backend/src/main/java/com/dbaagent/service/QueryFingerprintService.java index 5334f1a..8f51a94 100644 --- a/backend/src/main/java/com/dbaagent/service/QueryFingerprintService.java +++ b/backend/src/main/java/com/dbaagent/service/QueryFingerprintService.java @@ -173,6 +173,14 @@ public FingerprintSummary getSummary(String connectionId) { /** * Get trend data for a specific fingerprint */ + /** + * The connection a fingerprint row belongs to, for authorizing an endpoint + * keyed only on a fingerprint id. Empty when the id does not exist. + */ + public java.util.Optional findConnectionIdForFingerprintId(String fingerprintId) { + return fingerprintRepository.findById(fingerprintId).map(f -> f.getConnectionId()); + } + public FingerprintTrend getTrend(String fingerprintId) { QueryFingerprint fp = fingerprintRepository.findById(fingerprintId) .orElseThrow(() -> new IllegalArgumentException("Fingerprint not found: " + fingerprintId)); diff --git a/backend/src/main/java/com/dbaagent/service/QueryPerformanceService.java b/backend/src/main/java/com/dbaagent/service/QueryPerformanceService.java index 057038a..604c7ac 100644 --- a/backend/src/main/java/com/dbaagent/service/QueryPerformanceService.java +++ b/backend/src/main/java/com/dbaagent/service/QueryPerformanceService.java @@ -208,6 +208,15 @@ public List getRegressions(String connectionId, bool return regressionRepository.findByConnectionIdOrderByDetectedAtDesc(connectionId); } + /** + * The connection a regression belongs to, for authorizing an endpoint keyed + * only on the regression id. Empty when the id does not exist. + */ + public Optional findConnectionIdForRegression(Long regressionId) { + return regressionRepository.findById(regressionId) + .map(QueryPerformanceRegression::getConnectionId); + } + /** * Acknowledge a regression */ diff --git a/backend/src/main/java/com/dbaagent/service/QueryPlanCacheService.java b/backend/src/main/java/com/dbaagent/service/QueryPlanCacheService.java index 184b48a..82678b2 100644 --- a/backend/src/main/java/com/dbaagent/service/QueryPlanCacheService.java +++ b/backend/src/main/java/com/dbaagent/service/QueryPlanCacheService.java @@ -549,6 +549,14 @@ public QueryPlanComparison comparePlans(QueryPlanCache baseline, QueryPlanCache * Set a plan as the baseline for a query */ @Transactional + /** + * The connection a cached plan belongs to, for authorizing an endpoint keyed + * only on a plan id. Empty when the id does not exist. + */ + public java.util.Optional findConnectionIdForPlan(String planId) { + return planCacheRepository.findById(planId).map(p -> p.getConnectionId()); + } + public void setBaseline(String planId) { planCacheRepository.findById(planId).ifPresent(plan -> { // Clear existing baseline @@ -636,6 +644,22 @@ public int acknowledgeRegressions(List comparisonIds, String acknowledge return comparisonRepository.acknowledgeByIds(comparisonIds, acknowledgedBy, LocalDateTime.now()); } + /** + * True when every given comparison id belongs to {@code connectionId}. The ids arrive + * in the request body, so the caller's authorization on the path connection does not + * constrain them — without this a caller could acknowledge another connection's plan + * regressions. An id that resolves to nothing fails too, so unknown ids cannot be + * mixed into an otherwise valid batch. + */ + public boolean allComparisonsBelongTo(String connectionId, List comparisonIds) { + if (comparisonIds == null || comparisonIds.isEmpty()) { + return true; + } + List found = comparisonRepository.findAllById(comparisonIds); + return found.size() == comparisonIds.stream().distinct().count() + && found.stream().allMatch(c -> java.util.Objects.equals(connectionId, c.getConnectionId())); + } + /** * Get plan statistics */ diff --git a/backend/src/main/java/com/dbaagent/service/SchemaChangeTrackingService.java b/backend/src/main/java/com/dbaagent/service/SchemaChangeTrackingService.java index 01bf76e..56eb0db 100644 --- a/backend/src/main/java/com/dbaagent/service/SchemaChangeTrackingService.java +++ b/backend/src/main/java/com/dbaagent/service/SchemaChangeTrackingService.java @@ -655,9 +655,40 @@ public List compareSnapshots(String snapshotId1, String snapshotId throw new IllegalArgumentException("One or both snapshots not found"); } + // Both snapshots must belong to the same connection. Without this a caller + // authorized on connection A could diff A's schema against connection B's + // and read B's table and column names out of the resulting change list. + if (!Objects.equals(snap1.get().getConnectionId(), snap2.get().getConnectionId())) { + throw new IllegalArgumentException("Snapshots belong to different connections"); + } + return detectChanges(snap1.get(), snap2.get()); } + /** + * The connection a snapshot belongs to, for authorizing an endpoint keyed + * only on a snapshot id. Empty when the id does not exist. + */ + public Optional findConnectionIdForSnapshot(String snapshotId) { + return snapshotRepository.findById(snapshotId).map(SchemaSnapshot::getConnectionId); + } + + /** + * True when every given change id belongs to {@code connectionId}. Guards the + * acknowledge endpoints, whose ids arrive in the body and are therefore not + * covered by a path-variable authorization check. + */ + public boolean allChangesBelongTo(String connectionId, List changeIds) { + if (changeIds == null || changeIds.isEmpty()) { + return true; + } + List found = changeRepository.findAllById(changeIds); + // An id that resolves to nothing must fail too, otherwise a caller can mix + // unknown ids in and still have the batch accepted. + return found.size() == changeIds.stream().distinct().count() + && found.stream().allMatch(c -> Objects.equals(connectionId, c.getConnectionId())); + } + /** * Get change statistics for a connection */ diff --git a/backend/src/main/java/com/dbaagent/service/SentinelAnalyticsService.java b/backend/src/main/java/com/dbaagent/service/SentinelAnalyticsService.java index 1babddf..f6d9d10 100644 --- a/backend/src/main/java/com/dbaagent/service/SentinelAnalyticsService.java +++ b/backend/src/main/java/com/dbaagent/service/SentinelAnalyticsService.java @@ -499,6 +499,15 @@ public List getRecommendationsByPriority( /** * Update recommendation status */ + /** + * The connection a recommendation belongs to, for authorizing an endpoint + * keyed only on a recommendation id. Empty when the id does not exist. + */ + public java.util.Optional findConnectionIdForRecommendation(String recommendationId) { + return recommendationRepository.findById(recommendationId) + .map(SentinelRecommendation::getConnectionId); + } + public SentinelRecommendation updateRecommendationStatus( String recommendationId, SentinelRecommendation.Status status, diff --git a/backend/src/main/java/com/dbaagent/service/SlowQueryAlertService.java b/backend/src/main/java/com/dbaagent/service/SlowQueryAlertService.java index 9fdef5f..d9a0e5c 100644 --- a/backend/src/main/java/com/dbaagent/service/SlowQueryAlertService.java +++ b/backend/src/main/java/com/dbaagent/service/SlowQueryAlertService.java @@ -243,6 +243,14 @@ public SlowQueryAlertSummary getAlertSummary(String connectionId) { /** * Acknowledge an alert */ + /** + * The connection an alert belongs to, for authorizing an endpoint keyed only + * on an alert id. Empty when the id does not exist. + */ + public java.util.Optional findConnectionIdForAlert(String alertId) { + return alertRepository.findById(alertId).map(PlaybookAlert::getConnectionId); + } + public PlaybookAlert acknowledgeAlert(String alertId, String userId) { PlaybookAlert alert = alertRepository.findById(alertId) .orElseThrow(() -> new IllegalArgumentException("Alert not found: " + alertId)); diff --git a/backend/src/test/java/com/dbaagent/controller/ConnectionScopedAuthorizationSafetyTest.java b/backend/src/test/java/com/dbaagent/controller/ConnectionScopedAuthorizationSafetyTest.java new file mode 100644 index 0000000..d61a277 --- /dev/null +++ b/backend/src/test/java/com/dbaagent/controller/ConnectionScopedAuthorizationSafetyTest.java @@ -0,0 +1,383 @@ +package com.dbaagent.controller; + +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import java.util.stream.Stream; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Every endpoint that takes a caller-supplied connection id must authorize it. + * + *

{@link BrainControllerAuthorizationSafetyTest} asserts this for one file, and that is + * exactly why the same defect shipped again: 116 endpoints across 12 other controllers + * ({@code SlowQueryController}, {@code SlowQueryAnalyticsController}, {@code SentinelAnalyticsController}, + * {@code SchemaChangeController}, the four Performance controllers, {@code IndexAdvisorController}, + * {@code AdvisorController}, {@code ResourceLimitsController}, {@code BusinessRuleController}) + * had no authorization at all. A user with no grant on any connection could read + * literal-bearing slow-query SQL with real customer ids and names, enumerate another + * tenant's schema, and permanently delete their analysis history — verified against a + * running install, not inferred. + * + *

So this test scans every controller rather than a named list. A new + * controller is covered the day it is written, which a per-file test can never promise. + */ +class ConnectionScopedAuthorizationSafetyTest { + + private static final Path CONTROLLER_DIR = Path.of("src/main/java/com/dbaagent/controller"); + + private static final Pattern MAPPING = Pattern.compile( + "^\\s*@(?:[\\w.]*\\.)?(Get|Post|Delete|Put|Patch)Mapping\\b"); + + /** + * A handler is authorized by a per-connection assert, by resolving some other id to its + * owning connection through a local helper, or by being admin-only. The helper form is + * matched by name because the resolution happens one call away — an + * {@code assertCanManageAlert(alertId)} that looks up the alert's connection and + * asserts on it is the correct shape, and inlining it in every handler would be worse. + */ + private static final Pattern AUTHORIZED = Pattern.compile( + "accessControlService\\.assertCan\\w+\\(|@PreAuthorize|assertCan(Read|Manage)\\w+\\("); + + /** + * Handlers whose authorization correctly lives one layer down, named as + * {@code Controller:line}. Two distinct reasons, and both matter: + * + *

+ * + * {@link #everyDelegatedCheckStillExists()} re-derives the first group, so removing the + * service-layer assert fails the build instead of silently widening access. + */ + private static final Set AUTHORIZED_ELSEWHERE = Set.of( + "DashboardWorkspaceController.java:47", + "DashboardWorkspaceController.java:60", + "AgentChatController.java:25", + "AgentConversationController.java:29", + "AgentConversationController.java:45" + ); + + /** Service methods that own a delegated connection check. */ + private static final List DELEGATED_CHECKS = List.of( + "src/main/java/com/dbaagent/service/DashboardWorkspaceService.java" + ); + + /** + * Controllers that legitimately have no connection to authorize against. Each is here + * for a stated reason, not because it was inconvenient — an entry is a claim that the + * endpoints hold no caller-supplied connection id, which {@link #everyExemptControllerIsActuallyConnectionFree()} + * re-checks so this list cannot rot into a way of hiding a real gap. + */ + private static final Set NOT_CONNECTION_SCOPED = Set.of( + "AuthController", // login / refresh / logout — pre-authentication by definition + "AuthCliController", // device-code pairing, same + "AuthInternalController", // nginx auth_request subrequest + "BootstrapController", // first-admin creation, gated by a shared secret + localhost + "SetupController", // install wizard + "InviteCodeController", // invite redemption, keyed on a code + "UserController", // user administration, role-gated elsewhere + "AdminController", // admin surface, @PreAuthorize at class level + "ImpersonationController", // admin surface, @PreAuthorize at class level + "McpTokenController", // per-caller tokens, scoped to the authenticated user + "SlackLinkController", // Slack workspace binding + "LlmProxyController", // OpenAI-shaped gateway, no connection in the contract + "PublicDashboardController", // permitAll by design; scoped by share token + // Provisions the agent profile for whoever is calling: the username comes from + // requireCurrentUsername() and any connectionId in the body only selects which of + // the caller's own connections to preload. + "AgentBridgeController" + ); + + private record Endpoint(String file, int line, String mapping, String body) {} + + private static List controllers() throws IOException { + try (Stream paths = Files.list(CONTROLLER_DIR)) { + return paths.filter(p -> p.getFileName().toString().endsWith("Controller.java")).sorted().toList(); + } + } + + /** + * Slices one controller into one entry per handler, mapping annotation to closing brace. + * + *

The slice starts one line above the mapping when that line is another + * annotation, because {@code @PreAuthorize} is conventionally written above + * {@code @PostMapping}. Starting at the mapping itself put the authorization outside the + * captured body and reported {@code POST /training/reindex-all} — which is admin-only — + * as unguarded. + */ + private static List endpoints(Path controller) throws IOException { + List lines = Files.readAllLines(controller); + List endpoints = new ArrayList<>(); + String name = controller.getFileName().toString(); + + for (int i = 0; i < lines.size(); i++) { + Matcher matcher = MAPPING.matcher(lines.get(i)); + if (!matcher.find()) { + continue; + } + int start = i; + while (start > 0 && lines.get(start - 1).trim().startsWith("@")) { + start--; + } + StringBuilder body = new StringBuilder(); + int end = start; + while (end < lines.size()) { + body.append(lines.get(end)).append('\n'); + if (end > i && lines.get(end).equals(" }")) { + break; + } + end++; + } + endpoints.add(new Endpoint(name, i + 1, lines.get(i).trim(), body.toString())); + } + return endpoints; + } + + /** + * True when the handler receives a connection id, or an id that identifies a + * connection-owned row. Both forms need authorization: the second is the trap, since + * {@code PUT /performance-actions/{actionId}/status} carries no {@code connectionId} + * yet mutates a row that belongs to one. + */ + private static boolean touchesAConnection(String body) { + return body.contains("connectionId") + || Pattern.compile("@PathVariable[^)]*\\)?\\s*(?:Long|String)\\s+" + + "(alertId|actionId|regressionId|recommendationId|fingerprintId|planId|ruleId" + + "|snapshotId|historyId|changeId|comparisonId)").matcher(body).find() + || body.contains("historyId1") + || body.contains("snapshotId1"); + } + + @Test + void everyConnectionScopedEndpointAuthorizesTheCaller() throws IOException { + List offenders = new ArrayList<>(); + + for (Path controller : controllers()) { + String name = controller.getFileName().toString().replace(".java", ""); + if (NOT_CONNECTION_SCOPED.contains(name)) { + continue; + } + String source = Files.readString(controller); + boolean classLevelAdminOnly = source.contains("@PreAuthorize") + && source.indexOf("@PreAuthorize") < source.indexOf("public class"); + if (classLevelAdminOnly) { + continue; + } + for (Endpoint endpoint : endpoints(controller)) { + if (!touchesAConnection(endpoint.body())) { + continue; + } + if (AUTHORIZED_ELSEWHERE.contains(endpoint.file() + ":" + endpoint.line())) { + continue; + } + if (!AUTHORIZED.matcher(endpoint.body()).find()) { + offenders.add(endpoint.file() + ":" + endpoint.line() + " " + endpoint.mapping()); + } + } + } + + assertThat(offenders) + .as("These endpoints take a caller-supplied connection id (or an id owned by a " + + "connection) and never authorize it. Authentication is not authorization: " + + "SecurityConfig only asserts .anyRequest().authenticated() and no filter, " + + "interceptor or aspect inspects a connectionId. Add " + + "accessControlService.assertCanReadConnectionContent(connectionId) to reads " + + "and assertCanManageConnectionContent(connectionId) to writes. When the path " + + "carries some other id, resolve its owning connection first and assert on " + + "that — an id is not a capability. An endpoint with no connection scope at " + + "all is admin-only (@PreAuthorize).") + .isEmpty(); + } + + /** + * Ids that arrive in the request body are not constrained by a path-variable + * check. {@code POST /schema-changes/{connectionId}/changes/acknowledge} authorizes the + * path connection and then acknowledges whatever change ids the body names, so a caller + * authorized on their own connection could acknowledge another tenant's changes. Each + * such handler must additionally verify the collection belongs to the scope. + */ + @Test + void collectionIdsFromTheRequestBodyAreCheckedAgainstTheScope() throws IOException { + List offenders = new ArrayList<>(); + + for (Path controller : controllers()) { + for (Endpoint endpoint : endpoints(controller)) { + String body = endpoint.body(); + boolean takesIdCollection = Pattern + .compile("@RequestBody[^;]*List\\s+(\\w*[Ii]ds)").matcher(body).find() + || body.contains("getActionIds()") + || body.contains("getChangeIds()"); + if (!takesIdCollection) { + continue; + } + boolean scoped = body.contains("BelongTo") + || body.contains("forEach(this::assertCan") + || body.contains("stream().forEach"); + if (!scoped) { + offenders.add(endpoint.file() + ":" + endpoint.line() + " " + endpoint.mapping()); + } + } + } + + assertThat(offenders) + .as("These endpoints accept a list of ids in the request body. A path-variable " + + "authorization check does not constrain them, so verify every id belongs " + + "to the authorized scope (or authorize each id individually) before acting.") + .isEmpty(); + } + + /** + * An assert placed inside a {@code try} whose catch-all returns 500 turns a 403 into a + * server error: the denial holds, but the client cannot tell "not yours" from "broken". + */ + @Test + void authorizationFailuresPropagateAsForbiddenRatherThanServerError() throws IOException { + List offenders = new ArrayList<>(); + + for (Path controller : controllers()) { + for (Endpoint endpoint : endpoints(controller)) { + String body = endpoint.body(); + int assertAt = body.indexOf("accessControlService.assertCan"); + if (assertAt < 0) { + continue; + } + int tryAt = body.indexOf("try {"); + boolean assertInsideTry = tryAt >= 0 && tryAt < assertAt; + boolean hasCatchAll = body.contains("catch (Exception"); + if (assertInsideTry && hasCatchAll + && !body.contains("catch (org.springframework.web.server.ResponseStatusException e)") + && !body.contains("catch (ResponseStatusException e)")) { + offenders.add(endpoint.file() + ":" + endpoint.line() + " " + endpoint.mapping()); + } + } + } + + assertThat(offenders) + .as("These endpoints assert access inside a try whose catch-all converts the 403 " + + "into a 500. Rethrow it first: catch (ResponseStatusException e) { throw e; } " + + "— or move the assert above the try.") + .isEmpty(); + } + + /** + * A {@code @ControllerAdvice} with a catch-all {@code @ExceptionHandler(Exception.class)} + * swallows authorization denials the same way an in-method catch-all does, and it is + * easier to miss because it lives in a different file from the endpoint. + * + *

{@code IndexAdvisorExceptionHandler} did exactly this: a non-granted caller hitting + * {@code /index-advisor/{id}/health-report} got {@code 500 "Index operation failed"} + * whose body carried the 403's text. The guard held, but the response blamed the index + * store. Any advice with a catch-all must also handle {@code ResponseStatusException}. + */ + @Test + void controllerAdvicesDoNotSwallowAuthorizationDenials() throws IOException { + List offenders = new ArrayList<>(); + + try (Stream paths = Files.walk(Path.of("src/main/java/com/dbaagent"))) { + for (Path file : paths.filter(p -> p.toString().endsWith(".java")).toList()) { + String source = Files.readString(file); + if (!source.contains("@RestControllerAdvice") && !source.contains("@ControllerAdvice")) { + continue; + } + if (source.contains("@ExceptionHandler(Exception.class)") + && !source.contains("ResponseStatusException.class")) { + offenders.add(file.getFileName().toString()); + } + } + } + + assertThat(offenders) + .as("These @ControllerAdvice classes catch Exception without handling " + + "ResponseStatusException first, so a 403 from an authorization check is " + + "reported as a 500 attributed to the feature. Add an " + + "@ExceptionHandler(ResponseStatusException.class) that preserves the status.") + .isEmpty(); + } + + /** + * A handler exempted because its check lives in the service layer stays exempt only + * while that check is actually there. Without this, deleting the service-layer assert + * would widen access and the exemption would quietly cover for it. + */ + @Test + void everyDelegatedCheckStillExists() throws IOException { + List missing = new ArrayList<>(); + + for (String service : DELEGATED_CHECKS) { + String source = Files.readString(Path.of(service)); + if (!source.contains("accessControlService.assertCanReadConnectionContent(") + && !source.contains("accessControlService.assertCanManageConnectionContent(")) { + missing.add(service); + } + } + + assertThat(missing) + .as("A controller endpoint is exempted from the authorization sweep because this " + + "service performs the connection check on its behalf, and that check is now " + + "gone. Either restore it or drop the controller's AUTHORIZED_ELSEWHERE entry " + + "and assert in the controller.") + .isEmpty(); + } + + /** + * Guards the exemption list. If an exempt controller grows an endpoint that does take a + * connection id, the entry is no longer true and the controller must be authorized + * rather than skipped. + */ + @Test + void everyExemptControllerIsActuallyConnectionFree() throws IOException { + List offenders = new ArrayList<>(); + + for (Path controller : controllers()) { + String name = controller.getFileName().toString().replace(".java", ""); + if (!NOT_CONNECTION_SCOPED.contains(name)) { + continue; + } + String source = Files.readString(controller); + // A class-level @PreAuthorize already authorizes every handler in the file, so a + // connectionId appearing inside one is not evidence of a gap. + if (source.contains("@PreAuthorize") + && source.indexOf("@PreAuthorize") < source.indexOf("public class")) { + continue; + } + for (Endpoint endpoint : endpoints(controller)) { + String body = endpoint.body(); + if (!body.contains("connectionId")) { + continue; + } + // Acting on the caller's own identity is its own scope: the row is selected + // by the authenticated username, so a connectionId in the body only picks + // among things that caller already owns. + boolean scopedToCaller = body.contains("requireCurrentUsername()") + || body.contains("getCurrentUsername()"); + if (!scopedToCaller && !AUTHORIZED.matcher(body).find()) { + offenders.add(endpoint.file() + ":" + endpoint.line() + " " + endpoint.mapping()); + } + } + } + + assertThat(offenders) + .as("These endpoints live in a controller exempted as 'not connection scoped', " + + "but they reference a connectionId and neither authorize it nor scope the " + + "work to the authenticated caller. Either authorize them or remove the " + + "controller from NOT_CONNECTION_SCOPED — the exemption list must stay true.") + .isEmpty(); + } +} diff --git a/backend/src/test/java/com/dbaagent/controller/SlowQueryControllerS3Test.java b/backend/src/test/java/com/dbaagent/controller/SlowQueryControllerS3Test.java index 3f797fc..6d38d5f 100644 --- a/backend/src/test/java/com/dbaagent/controller/SlowQueryControllerS3Test.java +++ b/backend/src/test/java/com/dbaagent/controller/SlowQueryControllerS3Test.java @@ -27,6 +27,8 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import com.dbaagent.service.security.AccessControlService; + import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.isNull; @@ -53,6 +55,9 @@ void analyzeS3LogFile() throws Exception { KeyCustomerService keyCustomerService = mock(KeyCustomerService.class); SlowQueryInsightsService slowQueryInsightsService = mock(SlowQueryInsightsService.class); ObjectMapper objectMapper = mock(ObjectMapper.class); + // The controller now authorizes the connection before doing any work; a plain + // mock allows it, so these tests still exercise the S3 path they were written for. + AccessControlService accessControlService = mock(AccessControlService.class); SlowQueryController controller = new SlowQueryController( slowQueryService, @@ -69,7 +74,8 @@ void analyzeS3LogFile() throws Exception { explainPlanService, keyCustomerService, slowQueryInsightsService, - objectMapper + objectMapper, + accessControlService ); SlowQueryAnalysis analysis = SlowQueryAnalysis.builder() @@ -117,6 +123,9 @@ void getInsightsReturnsPayload() { KeyCustomerService keyCustomerService = mock(KeyCustomerService.class); SlowQueryInsightsService slowQueryInsightsService = mock(SlowQueryInsightsService.class); ObjectMapper objectMapper = mock(ObjectMapper.class); + // The controller now authorizes the connection before doing any work; a plain + // mock allows it, so these tests still exercise the S3 path they were written for. + AccessControlService accessControlService = mock(AccessControlService.class); SlowQueryController controller = new SlowQueryController( slowQueryService, @@ -133,7 +142,8 @@ void getInsightsReturnsPayload() { explainPlanService, keyCustomerService, slowQueryInsightsService, - objectMapper + objectMapper, + accessControlService ); SlowQueryInsightsResponse payload = SlowQueryInsightsResponse.builder() From f723a13d945a2a13e00bf9883ae9fd8a440e4266 Mon Sep 17 00:00:00 2001 From: sumit Date: Thu, 27 Aug 2026 19:19:25 +0530 Subject: [PATCH 2/2] fix: close the three gaps cursor[bot] found on #86 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three were real. Verified each against the code before fixing, and each fix against the running backend after. 1. ProjectController was half-open, and the safety test could not see it Only `listProjects` with a non-null connectionId was guarded. `createProject` took the connection from `request.getConnectionId()`, and the three projectId-keyed endpoints had no check at all — so any authenticated user could read, rename or delete another tenant's project. Root cause was the scanner, exactly as reported: `touchesAConnection` matched the literal lowercase `connectionId`, so `getConnectionId()` (capital C) did not register, and `projectId` was absent from its hand-written id allowlist. Four unguarded endpoints were invisible while the suite reported every case green. My javadoc on that controller claimed "every endpoint here asserts access itself", which was false. Fixed both halves. The connection match is now case-insensitive (`(?i)connection_?id`), and the id rule is inverted: *any* `@PathVariable ...Id` counts as connection-owned until proven otherwise, with real exceptions listed in NOT_CONNECTION_OWNED_IDS alongside the reason. An allowlist can only catch the ids someone remembered to add; this way an omission fails the build instead of passing silently. Inverting it surfaced four PlaybookController endpoints. Those are true negatives — `Playbook` has no connectionId field, playbooks are global templates, and the endpoints in that file which *do* carry a connection are already guarded. `playbookId` is therefore exempt, and `playbookExemptionHoldsOnlyWhilePlaybooksAreConnectionFree` fails the build if a connectionId is ever added to the entity, so the exemption cannot start hiding those four endpoints later. `GET /projects` with no filter spans every connection and cannot be authorized against one grant, so it now filters to connections the caller can read, resolving access once per distinct connectionId rather than once per project (`ConnectionAccessService.resolveAccess` is uncached and hits the grant table). 2. setBaseline trusted the path connectionId while mutating an unconstrained snapshotId `POST /schema-changes/{connectionId}/snapshots/{snapshotId}/set-baseline` asserted manage on the connection, then flipped whatever snapshot id it was handed to BASELINE and pointed that connection's drift config at it. Manage access on A was enough to retarget B's snapshot and bind A's baseline to it — the same id-mismatch class this branch claimed to have closed, split across two path variables instead of hiding in a body. The controller now binds the snapshot to the path connection, and `setBaseline` enforces it again in the service so the invariant does not depend on the caller. It throws rather than skipping: silently no-op'ing the snapshot write while still writing the drift config would leave the config referencing another connection's snapshot. 3. The existence oracle was still open, and the comments claimed otherwise The id helpers 404'd an unknown id but left an existing-but-unauthorized row at 403, so the pair still confirmed which ids are real — `query_performance_regression.id` is a sequential Long, so walking 1..N would have mapped every tenant's regressions. The code comments asserting "404 so it cannot be used to probe" described only the half that was implemented. Added `assertCanRead/ManageConnectionContentOrNotFound`, which answers 404 for both cases, matching what `DashboardWorkspaceService.assertCanReadDashboard` already does for a dashboard outside the caller's workspace. Applied to all nine id-keyed helpers. Endpoints keyed on a connectionId keep 403 on purpose: the caller already knows that connection exists, so an actionable "access denied" beats a misleading 404. Comments corrected to state the property the code now has. Medium items from the same review * `compareSnapshots` and `setBaseline` threw IllegalArgumentException for a missing or cross-connection snapshot, which surfaced as 500. Both now map it to 404 — "not something you can compare" is not a server fault, and a 500 reads as a broken feature. * Remaining actor fields: Sentinel's `initiatedBy` came from the request body, and `acknowledgedBy` still defaulted to the literal string "user" in three places. All now use requireCurrentUsername(). The parameters stay accepted and ignored, noted at each site so nobody re-wires them. Verification Real Maven compile of main and test sources, zero errors. All seven safety-test cases green against the tree. Live, against the rebuilt image with a DEVELOPER who has manage on connection G and no grant on connection U: - POST /projects on U -> 403; on G -> 200 - GET/PUT/DELETE a project owned by U -> 404, 404, 404 (not 403, not 200); row intact afterwards; not leaked through the unfiltered list - set-baseline binding U's snapshot as G's baseline -> 404; snapshot stayed MANUAL; G's drift config still unbound - existing-but-unauthorized vs nonexistent history id -> 404 and 404, indistinguishable; row survived the refused DELETE - 8/8 reads on G still non-403, and the project list shows only G's project Not covered: mvn test still has not been executed (the image build uses -DskipTests and this host has no JDK/Maven), so the seven cases are compile-verified and logic-validated but not JUnit-run. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 46 ++++++++-- .../PerformanceActionController.java | 7 +- .../controller/ProjectController.java | 65 ++++++++++++--- .../QueryPerformanceController.java | 7 +- .../controller/QueryPlanController.java | 10 ++- .../controller/SchemaChangeController.java | 48 +++++++++-- .../SentinelAnalyticsController.java | 14 ++-- .../controller/SlowQueryController.java | 22 +++-- .../service/SchemaChangeTrackingService.java | 19 +++-- .../security/AccessControlService.java | 45 ++++++++++ ...nnectionScopedAuthorizationSafetyTest.java | 83 +++++++++++++++++-- 11 files changed, 307 insertions(+), 59 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 53bc9d2..e4e227d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -749,18 +749,48 @@ it against a real database — not a theoretical hardening pass. accessor, so `findConnectionIdFor*` was added to `QueryPerformanceService`, `QueryPlanCacheService`, `SentinelAnalyticsService`, `BusinessRuleMemoryService`, `SlowQueryAlertService`, `QueryFingerprintService` and `SchemaChangeTrackingService`. - These helpers report **404, not 403**, for an unknown id — a 403 confirms the row - exists, turning the endpoint into an id oracle. `regressionId` is a sequential - `Long`, so that mattered. + These helpers report **404 for both** "no such id" and "not yours", via + `assertCanRead/ManageConnectionContentOrNotFound`. The first attempt only 404'd the + *unknown* case and left an authorized-but-denied row at 403, which still confirms the + row exists — a review caught that the code comments claimed a property the code did not + have. `query_performance_regression.id` is a sequential `Long`, so walking 1..N would + have mapped every tenant's regressions. Same answer + `DashboardWorkspaceService.assertCanReadDashboard` already gives. Endpoints keyed on a + **connectionId** keep 403: the caller already knows that connection exists, so an + actionable "access denied" is better than a misleading 404. - **Never take the actor from the request.** `POST /slow-queries/alerts/{id}/acknowledge` - took `@RequestParam String userId`, and three acknowledge endpoints took - `acknowledgedBy` defaulting to the literal string `"user"` — so the audit trail was - unauthenticated free text and could name any colleague. All seven sites now use - `accessControlService.requireCurrentUsername()`. The parameters are still accepted - (wire compatibility) and ignored. + took `@RequestParam String userId`; `acknowledgedBy` defaulted to the literal string + `"user"`; `resolvedBy`, `updatedBy` and Sentinel's `initiatedBy` came from the request + body — so the audit trail was unauthenticated free text and could name any colleague. + All of them now use `accessControlService.requireCurrentUsername()`. The parameters are + still accepted (wire compatibility) and ignored, which is noted at each site so nobody + re-wires them. - **Guarded vs unguarded is an existence oracle.** A guarded endpoint 404s an unknown connection id (`resolveCurrentUserAccess` wraps the lookup); an unguarded one returned 200. That difference alone enumerated valid connection ids. +- **A scanner built on an allowlist of id names can only catch the ids someone + remembered.** `ConnectionScopedAuthorizationSafetyTest` first matched + `body.contains("connectionId")` plus a hand-written list + (`alertId|actionId|regressionId|…`). Both halves leaked: `ProjectController.createProject` + reads `request.getConnectionId()` — **capital C** — and `projectId` was not in the list, + so `POST /projects` and `GET|PUT|DELETE /projects/{projectId}` were invisible while the + suite reported every case green. Now the connection match is case-insensitive and *any* + `@PathVariable …Id` counts as connection-owned until proven otherwise, with genuine + exceptions in `NOT_CONNECTION_OWNED_IDS` carrying a reason. Inverting it immediately + surfaced four `PlaybookController` endpoints — those turned out to be true negatives + (`Playbook` has no `connectionId`; playbooks are global templates), and + `playbookExemptionHoldsOnlyWhilePlaybooksAreConnectionFree` fails the build if a + `connectionId` is ever added to that entity. **A safety test that reports green is + evidence only about what it can see.** +- **Two path variables are as dangerous as a body id.** + `POST /schema-changes/{connectionId}/snapshots/{snapshotId}/set-baseline` authorized the + connection and then flipped *whatever snapshot id it was handed* to BASELINE and pointed + that connection's drift config at it — so manage access on A could retarget B's snapshot + and bind A's baseline to it. `setBaseline` now refuses a snapshot whose `connectionId` + differs, in the service as well as the controller, and **throws rather than silently + skipping**: no-op'ing the snapshot write while still writing the drift config would leave + the config pointing at another connection's snapshot. When a handler takes an id + alongside a `connectionId`, authorizing the connection is half the check. - **A `@ControllerAdvice` catch-all swallows a 403 the same way an in-method one does, and it is easier to miss because it lives in another file.** `IndexAdvisorExceptionHandler` has `@ExceptionHandler(Exception.class)`, so the newly diff --git a/backend/src/main/java/com/dbaagent/controller/PerformanceActionController.java b/backend/src/main/java/com/dbaagent/controller/PerformanceActionController.java index da266c3..8400105 100644 --- a/backend/src/main/java/com/dbaagent/controller/PerformanceActionController.java +++ b/backend/src/main/java/com/dbaagent/controller/PerformanceActionController.java @@ -298,12 +298,13 @@ public static class AffectedQueryItem { /** * Authorize a write keyed only on an action id. The action carries its own * connectionId, so resolve that first and assert against it — an action id - * is not a capability. An unknown id reports 404 rather than 403 so the - * endpoint cannot be used to probe which action ids exist. + * is not a capability. An unknown id and one on a connection the caller cannot + * manage both report 404, so the endpoint cannot be used to probe which action + * ids exist. */ private void assertCanManageAction(String actionId) { PerformanceAction action = aggregatorService.getActionById(actionId) .orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND, "Action not found")); - accessControlService.assertCanManageConnectionContent(action.getConnectionId()); + accessControlService.assertCanManageConnectionContentOrNotFound(action.getConnectionId(), "Action"); } } diff --git a/backend/src/main/java/com/dbaagent/controller/ProjectController.java b/backend/src/main/java/com/dbaagent/controller/ProjectController.java index 46766d7..e5eb443 100644 --- a/backend/src/main/java/com/dbaagent/controller/ProjectController.java +++ b/backend/src/main/java/com/dbaagent/controller/ProjectController.java @@ -8,16 +8,21 @@ import org.springframework.http.ResponseEntity; import org.springframework.web.bind.annotation.*; +import java.util.HashMap; import java.util.List; +import java.util.Map; /** * REST API for projects, optionally filtered by connection. * - *

Authorization: every endpoint here takes a caller-supplied connection id, so - * each one asserts access itself ({@code assertCanReadConnectionContent} for reads, - * {@code assertCanManageConnectionContent} for writes). {@code SecurityConfig} only - * requires an authenticated principal — nothing upstream inspects a connection id. See - * {@code ConnectionScopedAuthorizationSafetyTest}. + *

Authorization: a project belongs to a connection, so every endpoint is gated + * on that connection's ACL — directly where the request carries a {@code connectionId}, + * and via the project's own {@code connectionId} where the path carries only a + * {@code projectId}. {@code SecurityConfig} only requires an authenticated principal; + * nothing upstream inspects a connection id. + * + *

The id-keyed endpoints report 404 rather than 403 for a project the caller may not + * touch, so the route cannot be used to test which project ids exist. */ @RestController @RequestMapping("/projects") @@ -28,6 +33,7 @@ public class ProjectController { @PostMapping public ResponseEntity createProject(@RequestBody CreateProjectRequest request) { + accessControlService.assertCanManageConnectionContent(request.getConnectionId()); Project project = projectService.createProject( request.getName(), request.getDescription(), @@ -42,18 +48,26 @@ public ResponseEntity> listProjects( ) { if (connectionId != null) { accessControlService.assertCanReadConnectionContent(connectionId); + return ResponseEntity.ok(projectService.getProjectsByConnection(connectionId)); } - List projects = connectionId != null - ? projectService.getProjectsByConnection(connectionId) - : projectService.getAllProjects(); - return ResponseEntity.ok(projects); + // No filter means "every project on every connection", which cannot be authorized + // against a single connection's grants — so it is scoped to the caller instead. + // Access is resolved once per distinct connection, not once per project: + // ConnectionAccessService.resolveAccess is uncached and hits the grant table, and + // many projects share a connection. + Map readable = new HashMap<>(); + return ResponseEntity.ok(projectService.getAllProjects().stream() + .filter(p -> readable.computeIfAbsent( + String.valueOf(p.getConnectionId()), c -> canRead(p.getConnectionId()))) + .toList()); } @GetMapping("/{projectId}") public ResponseEntity getProject(@PathVariable String projectId) { - return projectService.getProject(projectId) - .map(ResponseEntity::ok) - .orElse(ResponseEntity.notFound().build()); + Project project = requireProject(projectId); + accessControlService.assertCanReadConnectionContentOrNotFound( + project.getConnectionId(), "Project"); + return ResponseEntity.ok(project); } @PutMapping("/{projectId}") @@ -61,6 +75,7 @@ public ResponseEntity updateProject( @PathVariable String projectId, @RequestBody UpdateProjectRequest request ) { + assertCanManageProject(projectId); return projectService.updateProject(projectId, request.getName(), request.getDescription()) .map(ResponseEntity::ok) .orElse(ResponseEntity.notFound().build()); @@ -68,6 +83,7 @@ public ResponseEntity updateProject( @DeleteMapping("/{projectId}") public ResponseEntity deleteProject(@PathVariable String projectId) { + assertCanManageProject(projectId); return projectService.deleteProject(projectId) ? ResponseEntity.ok().build() : ResponseEntity.notFound().build(); @@ -80,6 +96,31 @@ public static class CreateProjectRequest { private String connectionId; } + private Project requireProject(String projectId) { + return projectService.getProject(projectId) + .orElseThrow(() -> new org.springframework.web.server.ResponseStatusException( + org.springframework.http.HttpStatus.NOT_FOUND, "Project not found")); + } + + /** A project id is not a capability: authorize the connection that owns the project. */ + private void assertCanManageProject(String projectId) { + accessControlService.assertCanManageConnectionContentOrNotFound( + requireProject(projectId).getConnectionId(), "Project"); + } + + /** Non-throwing read check, for filtering a cross-connection list. */ + private boolean canRead(String connectionId) { + if (connectionId == null) { + return false; + } + try { + accessControlService.assertCanReadConnectionContent(connectionId); + return true; + } catch (org.springframework.web.server.ResponseStatusException e) { + return false; + } + } + @Data public static class UpdateProjectRequest { private String name; diff --git a/backend/src/main/java/com/dbaagent/controller/QueryPerformanceController.java b/backend/src/main/java/com/dbaagent/controller/QueryPerformanceController.java index fdb14ec..69e0698 100644 --- a/backend/src/main/java/com/dbaagent/controller/QueryPerformanceController.java +++ b/backend/src/main/java/com/dbaagent/controller/QueryPerformanceController.java @@ -280,14 +280,15 @@ public ResponseEntity> triggerAnalysis(@PathVariable String * Authorize a write keyed only on a regression id. The regression carries * its own connectionId, so resolve that and assert against it — a * regression id is not a capability, and these ids are sequential Longs, - * so they are trivially enumerable. An unknown id reports 404 so the - * endpoint cannot be used to probe which regressions exist. + * so they are trivially enumerable — walking 1..N would otherwise map out every + * tenant's regressions. An unknown id and one the caller cannot manage both report + * 404, so the response does not distinguish them. */ private void assertCanManageRegression(Long regressionId) { String connectionId = queryPerformanceService.findConnectionIdForRegression(regressionId) .orElseThrow(() -> new org.springframework.web.server.ResponseStatusException( org.springframework.http.HttpStatus.NOT_FOUND, "Regression not found")); - accessControlService.assertCanManageConnectionContent(connectionId); + accessControlService.assertCanManageConnectionContentOrNotFound(connectionId, "Regression"); } /** The authenticated caller. Never trust a client-supplied actor name. */ diff --git a/backend/src/main/java/com/dbaagent/controller/QueryPlanController.java b/backend/src/main/java/com/dbaagent/controller/QueryPlanController.java index e847471..38e8736 100644 --- a/backend/src/main/java/com/dbaagent/controller/QueryPlanController.java +++ b/backend/src/main/java/com/dbaagent/controller/QueryPlanController.java @@ -108,7 +108,10 @@ public ResponseEntity> getUnacknowledgedRegressions(@P public ResponseEntity> acknowledgeRegressions( @PathVariable String connectionId, @RequestBody List comparisonIds, - @RequestParam(required = false, defaultValue = "user") String acknowledgedBy) { + @RequestParam(required = false) String acknowledgedBy) { + // Accepted for wire compatibility and deliberately ignored: the actor is + // taken from the security context below. It previously defaulted to the + // literal string "user", so the trail named nobody. accessControlService.assertCanManageConnectionContent(connectionId); if (!planCacheService.allComparisonsBelongTo(connectionId, comparisonIds)) { @@ -135,12 +138,13 @@ public ResponseEntity> getPlanStats(@PathVariable String con /** * Authorize a write keyed only on a plan id. The cached plan carries its own * connectionId, so resolve that and assert against it. An unknown id reports - * 404 so the endpoint cannot be used to probe which plan ids exist. + * 404 — as does a plan belonging to a connection the caller cannot manage, so the + * endpoint cannot be used to probe which plan ids exist. */ private void assertCanManagePlan(String planId) { String connectionId = planCacheService.findConnectionIdForPlan(planId) .orElseThrow(() -> new org.springframework.web.server.ResponseStatusException( org.springframework.http.HttpStatus.NOT_FOUND, "Plan not found")); - accessControlService.assertCanManageConnectionContent(connectionId); + accessControlService.assertCanManageConnectionContentOrNotFound(connectionId, "Plan"); } } diff --git a/backend/src/main/java/com/dbaagent/controller/SchemaChangeController.java b/backend/src/main/java/com/dbaagent/controller/SchemaChangeController.java index e289c25..0046103 100644 --- a/backend/src/main/java/com/dbaagent/controller/SchemaChangeController.java +++ b/backend/src/main/java/com/dbaagent/controller/SchemaChangeController.java @@ -80,8 +80,14 @@ public ResponseEntity> setBaseline( @PathVariable String connectionId, @PathVariable String snapshotId) { accessControlService.assertCanManageConnectionContent(connectionId); + assertSnapshotBelongsTo(connectionId, snapshotId); - schemaChangeService.setBaseline(connectionId, snapshotId); + try { + schemaChangeService.setBaseline(connectionId, snapshotId); + } catch (IllegalArgumentException e) { + throw new org.springframework.web.server.ResponseStatusException( + org.springframework.http.HttpStatus.NOT_FOUND, e.getMessage()); + } return ResponseEntity.ok(Map.of( "status", "success", "message", "Snapshot set as baseline" @@ -98,7 +104,15 @@ public ResponseEntity> compareSnapshots( assertCanReadSnapshot(snapshotId1); assertCanReadSnapshot(snapshotId2); - return ResponseEntity.ok(schemaChangeService.compareSnapshots(snapshotId1, snapshotId2)); + try { + return ResponseEntity.ok(schemaChangeService.compareSnapshots(snapshotId1, snapshotId2)); + } catch (IllegalArgumentException e) { + // Missing snapshot, or two snapshots from different connections. Both are + // "not something you can compare", not a server fault — a 500 here would read + // as a broken feature and hide the real reason. + throw new org.springframework.web.server.ResponseStatusException( + org.springframework.http.HttpStatus.NOT_FOUND, e.getMessage()); + } } // ==================== Change Endpoints ==================== @@ -128,7 +142,10 @@ public ResponseEntity> getUnacknowledgedChanges(@PathVariable public ResponseEntity> acknowledgeChanges( @PathVariable String connectionId, @RequestBody List changeIds, - @RequestParam(required = false, defaultValue = "user") String acknowledgedBy) { + @RequestParam(required = false) String acknowledgedBy) { + // Accepted for wire compatibility and deliberately ignored: the actor is + // taken from the security context below. It previously defaulted to the + // literal string "user", so the trail named nobody. accessControlService.assertCanManageConnectionContent(connectionId); assertChangesBelongTo(connectionId, changeIds); @@ -145,7 +162,10 @@ public ResponseEntity> acknowledgeChanges( @PostMapping("/{connectionId}/changes/acknowledge-all") public ResponseEntity> acknowledgeAllChanges( @PathVariable String connectionId, - @RequestParam(required = false, defaultValue = "user") String acknowledgedBy) { + @RequestParam(required = false) String acknowledgedBy) { + // Accepted for wire compatibility and deliberately ignored: the actor is + // taken from the security context below. It previously defaulted to the + // literal string "user", so the trail named nobody. accessControlService.assertCanManageConnectionContent(connectionId); int count = schemaChangeService.acknowledgeAllChanges( @@ -207,13 +227,14 @@ public ResponseEntity> triggerDriftCheck(@PathVariable Strin /** * Authorize a read keyed only on a snapshot id. The snapshot carries its own * connectionId, so resolve that and assert against it. An unknown id reports - * 404 so the endpoint cannot be used to probe which snapshots exist. + * 404 — as does a snapshot on a connection the caller cannot read, so the endpoint + * cannot be used to probe which snapshots exist. */ private void assertCanReadSnapshot(String snapshotId) { String connectionId = schemaChangeService.findConnectionIdForSnapshot(snapshotId) .orElseThrow(() -> new org.springframework.web.server.ResponseStatusException( org.springframework.http.HttpStatus.NOT_FOUND, "Snapshot not found")); - accessControlService.assertCanReadConnectionContent(connectionId); + accessControlService.assertCanReadConnectionContentOrNotFound(connectionId, "Snapshot"); } /** @@ -227,4 +248,19 @@ private void assertChangesBelongTo(String connectionId, List changeIds) org.springframework.http.HttpStatus.NOT_FOUND, "Change not found for this connection"); } } + + /** + * The snapshot id is a separate path variable from the connection id, so authorizing + * the connection says nothing about the snapshot. Without this a caller with manage + * access on connection A could flip connection B's snapshot to BASELINE and point A's + * drift config at it — the same body/path id-mismatch class as + * {@code changes/acknowledge}, just split across two path variables instead. + */ + private void assertSnapshotBelongsTo(String connectionId, String snapshotId) { + String owner = schemaChangeService.findConnectionIdForSnapshot(snapshotId).orElse(null); + if (!connectionId.equals(owner)) { + throw new org.springframework.web.server.ResponseStatusException( + org.springframework.http.HttpStatus.NOT_FOUND, "Snapshot not found"); + } + } } diff --git a/backend/src/main/java/com/dbaagent/controller/SentinelAnalyticsController.java b/backend/src/main/java/com/dbaagent/controller/SentinelAnalyticsController.java index 7d126df..36984ea 100644 --- a/backend/src/main/java/com/dbaagent/controller/SentinelAnalyticsController.java +++ b/backend/src/main/java/com/dbaagent/controller/SentinelAnalyticsController.java @@ -232,7 +232,9 @@ public ResponseEntity> logDeploymentEvent(@RequestBody Map affectedTables = (List) eventData.get("affectedTables"); - String initiatedBy = (String) eventData.get("initiatedBy"); + // The actor is the authenticated caller; an "initiatedBy" in the body + // would let anyone attribute a deployment event to a colleague. + String initiatedBy = accessControlService.requireCurrentUsername(); log.info("Logging deployment event: {} for connection {}", deploymentVersion, connectionId); @@ -273,7 +275,9 @@ public ResponseEntity> logSchemaChangeEvent(@RequestBody Map String tableName = (String) eventData.get("tableName"); String changeType = (String) eventData.get("changeType"); String description = (String) eventData.get("description"); - String initiatedBy = (String) eventData.get("initiatedBy"); + // The actor is the authenticated caller; an "initiatedBy" in the body + // would let anyone attribute a deployment event to a colleague. + String initiatedBy = accessControlService.requireCurrentUsername(); log.info("Logging schema change event: {} on table {}", changeType, tableName); @@ -526,13 +530,13 @@ private String generateExecutiveSummary( /** * Authorize a write keyed only on a recommendation id. The recommendation * carries its own connectionId, so resolve that and assert against it — a - * recommendation id is not a capability. An unknown id reports 404 so the - * endpoint cannot be used to probe which recommendations exist. + * recommendation id is not a capability. An unknown id and one on a connection the + * caller cannot manage both report 404, so the two are indistinguishable. */ private void assertCanManageRecommendation(String recommendationId) { String connectionId = sentinelAnalytics.findConnectionIdForRecommendation(recommendationId) .orElseThrow(() -> new org.springframework.web.server.ResponseStatusException( org.springframework.http.HttpStatus.NOT_FOUND, "Recommendation not found")); - accessControlService.assertCanManageConnectionContent(connectionId); + accessControlService.assertCanManageConnectionContentOrNotFound(connectionId, "Recommendation"); } } diff --git a/backend/src/main/java/com/dbaagent/controller/SlowQueryController.java b/backend/src/main/java/com/dbaagent/controller/SlowQueryController.java index 1631e16..ff04cc2 100644 --- a/backend/src/main/java/com/dbaagent/controller/SlowQueryController.java +++ b/backend/src/main/java/com/dbaagent/controller/SlowQueryController.java @@ -1773,9 +1773,11 @@ public static class HistorySummaryResponse { // ── authorization helpers for endpoints keyed on a non-connection id ────── // // An id is not a capability: each of these entities carries its own - // connectionId, so resolve the owner and assert against that. An unknown id - // reports 404 rather than 403, so none of these can be used to probe which - // ids exist on connections the caller cannot see. + // connectionId, so resolve the owner and assert against that. + // + // Both "no such id" and "not yours" answer 404, via the *OrNotFound guards. Splitting + // them (404 vs 403) would confirm which ids are real, which is an enumeration + // primitive — same reasoning as DashboardWorkspaceService.assertCanReadDashboard. private String historyConnectionId(String historyId) { return historyService.getHistoryById(historyId) @@ -1785,18 +1787,20 @@ private String historyConnectionId(String historyId) { } private void assertCanReadHistory(String historyId) { - accessControlService.assertCanReadConnectionContent(historyConnectionId(historyId)); + accessControlService.assertCanReadConnectionContentOrNotFound( + historyConnectionId(historyId), "Analysis"); } private void assertCanManageHistory(String historyId) { - accessControlService.assertCanManageConnectionContent(historyConnectionId(historyId)); + accessControlService.assertCanManageConnectionContentOrNotFound( + historyConnectionId(historyId), "Analysis"); } private void assertCanManageAlert(String alertId) { String connectionId = alertService.findConnectionIdForAlert(alertId) .orElseThrow(() -> new org.springframework.web.server.ResponseStatusException( org.springframework.http.HttpStatus.NOT_FOUND, "Alert not found")); - accessControlService.assertCanManageConnectionContent(connectionId); + accessControlService.assertCanManageConnectionContentOrNotFound(connectionId, "Alert"); } private String fingerprintConnectionId(String fingerprintId) { @@ -1806,10 +1810,12 @@ private String fingerprintConnectionId(String fingerprintId) { } private void assertCanReadFingerprint(String fingerprintId) { - accessControlService.assertCanReadConnectionContent(fingerprintConnectionId(fingerprintId)); + accessControlService.assertCanReadConnectionContentOrNotFound( + fingerprintConnectionId(fingerprintId), "Fingerprint"); } private void assertCanManageFingerprint(String fingerprintId) { - accessControlService.assertCanManageConnectionContent(fingerprintConnectionId(fingerprintId)); + accessControlService.assertCanManageConnectionContentOrNotFound( + fingerprintConnectionId(fingerprintId), "Fingerprint"); } } diff --git a/backend/src/main/java/com/dbaagent/service/SchemaChangeTrackingService.java b/backend/src/main/java/com/dbaagent/service/SchemaChangeTrackingService.java index 56eb0db..0c359f4 100644 --- a/backend/src/main/java/com/dbaagent/service/SchemaChangeTrackingService.java +++ b/backend/src/main/java/com/dbaagent/service/SchemaChangeTrackingService.java @@ -499,11 +499,20 @@ public SchemaDriftConfig ensureDefaultDriftConfig(String connectionId) { */ @Transactional public void setBaseline(String connectionId, String snapshotId) { - // Update the snapshot type to BASELINE - snapshotRepository.findById(snapshotId).ifPresent(snapshot -> { - snapshot.setSnapshotType(SchemaSnapshot.SnapshotType.BASELINE); - snapshotRepository.save(snapshot); - }); + // The snapshot id arrives as its own path variable, so authorizing connectionId + // upstream says nothing about it. Without this bind, a caller with manage access + // on connection A could flip connection B's snapshot to BASELINE and point A's + // drift config at it. Rejected rather than skipped: silently no-op'ing the + // snapshot write while still updating the drift config would leave the config + // referencing a snapshot from another connection. Enforced here as well as in the + // controller so the invariant does not depend on which caller reaches this method. + SchemaSnapshot snapshot = snapshotRepository.findById(snapshotId) + .filter(s -> Objects.equals(connectionId, s.getConnectionId())) + .orElseThrow(() -> new IllegalArgumentException( + "Snapshot not found for this connection")); + + snapshot.setSnapshotType(SchemaSnapshot.SnapshotType.BASELINE); + snapshotRepository.save(snapshot); // Update drift config driftConfigRepository.findByConnectionId(connectionId).ifPresent(config -> { diff --git a/backend/src/main/java/com/dbaagent/service/security/AccessControlService.java b/backend/src/main/java/com/dbaagent/service/security/AccessControlService.java index 79b8105..fc6bcbb 100644 --- a/backend/src/main/java/com/dbaagent/service/security/AccessControlService.java +++ b/backend/src/main/java/com/dbaagent/service/security/AccessControlService.java @@ -69,6 +69,51 @@ public void assertCanManageConnectionConfig(String connectionId) { assertAccess(connectionId, EffectiveConnectionAccess::canManageConfig, "Configuration access denied for this connection"); } + /** + * As {@link #assertCanReadConnectionContent}, but reports 404 instead of 403 — for + * endpoints keyed on a row id rather than a connection id. + * + *

Returning 403 for a row the caller may not touch and 404 for one that does not + * exist tells the caller which ids are real. That is an enumeration primitive, and + * `query_performance_regression.id` is a sequential {@code Long}, so walking it is + * trivial. Collapsing both to 404 means "no such row, as far as you are concerned", + * which is the same answer {@code DashboardWorkspaceService.assertCanReadDashboard} + * already gives for a dashboard outside the caller's workspace. + * + *

Use this only where the caller supplied an opaque row id. Endpoints that + * take a {@code connectionId} directly should keep 403: the caller already knows the + * connection exists (they typed its id), so hiding it buys nothing and an actionable + * "access denied" is the better answer. + * + * @param entity human-readable name for the 404 message, e.g. {@code "Alert"} + */ + public void assertCanReadConnectionContentOrNotFound(String connectionId, String entity) { + assertOrNotFound(connectionId, EffectiveConnectionAccess::canReadContent, entity); + } + + /** Write-side counterpart to {@link #assertCanReadConnectionContentOrNotFound}. */ + public void assertCanManageConnectionContentOrNotFound(String connectionId, String entity) { + assertOrNotFound(connectionId, EffectiveConnectionAccess::canManageContent, entity); + } + + private void assertOrNotFound( + String connectionId, + java.util.function.Predicate predicate, + String entity + ) { + ConnectionAccessService.ResolvedConnectionAccess access; + try { + access = resolveCurrentUserAccess(connectionId); + } catch (ResponseStatusException e) { + // An unresolvable connection, or an unauthenticated caller, must look the same + // as a row that isn't there — otherwise the distinction leaks back in here. + throw new ResponseStatusException(NOT_FOUND, entity + " not found"); + } + if (!predicate.test(access.getEffectiveAccess())) { + throw new ResponseStatusException(NOT_FOUND, entity + " not found"); + } + } + public ConnectionAccessService.ResolvedConnectionAccess resolveCurrentUserAccess(String connectionId) { if (!authEnabled && !ImpersonationContext.isActive()) { try { diff --git a/backend/src/test/java/com/dbaagent/controller/ConnectionScopedAuthorizationSafetyTest.java b/backend/src/test/java/com/dbaagent/controller/ConnectionScopedAuthorizationSafetyTest.java index d61a277..4ad60b2 100644 --- a/backend/src/test/java/com/dbaagent/controller/ConnectionScopedAuthorizationSafetyTest.java +++ b/backend/src/test/java/com/dbaagent/controller/ConnectionScopedAuthorizationSafetyTest.java @@ -157,14 +157,66 @@ private static List endpoints(Path controller) throws IOException { * yet mutates a row that belongs to one. */ private static boolean touchesAConnection(String body) { - return body.contains("connectionId") - || Pattern.compile("@PathVariable[^)]*\\)?\\s*(?:Long|String)\\s+" - + "(alertId|actionId|regressionId|recommendationId|fingerprintId|planId|ruleId" - + "|snapshotId|historyId|changeId|comparisonId)").matcher(body).find() - || body.contains("historyId1") - || body.contains("snapshotId1"); + if (CONNECTION_REF.matcher(body).find()) { + return true; + } + Matcher ids = OWNED_ID.matcher(body); + while (ids.find()) { + if (!NOT_CONNECTION_OWNED_IDS.contains(ids.group(1))) { + return true; + } + } + return false; } + /** + * Any mention of a connection id, in any casing a real signature uses. + * + *

This started as {@code body.contains("connectionId")} and that was a real hole: + * {@code ProjectController.createProject} reads the connection from + * {@code request.getConnectionId()} — capital C — so it did not match, and four + * unguarded endpoints were invisible while this test reported 6/6 green. Match the + * name case-insensitively and cover the {@code getConnectionId()} / + * {@code get("connectionId")} accessor forms explicitly. + */ + private static final Pattern CONNECTION_REF = Pattern.compile( + "(?i)connection_?id"); + + /** + * Any id-shaped path variable, allowlist-free. + * + *

The previous version enumerated the id names it knew about + * ({@code alertId|actionId|regressionId|…}), which can only catch ids someone + * remembered to add — {@code projectId} was missing, so + * {@code GET|PUT|DELETE /projects/{projectId}} were never examined. Inverted: treat + * every {@code @PathVariable ...Id} as a row that plausibly belongs to a + * connection, and require the handler to prove otherwise by authorizing it. A genuine + * exception goes in {@link #NOT_CONNECTION_OWNED_IDS} with a reason, so adding one is + * a deliberate, reviewable act rather than an omission. + */ + private static final Pattern OWNED_ID = Pattern.compile( + "@PathVariable[^)]*\\)?\\s*(?:Long|String|UUID)\\s+(\\w*[Ii]d)\\b"); + + /** + * Path-variable ids that identify something other than a connection-owned row. Each + * is scoped by its own mechanism, named here so the exemption is auditable. + */ + private static final Set NOT_CONNECTION_OWNED_IDS = Set.of( + "userId", // user administration; role-gated, not connection-gated + "id", // too generic to classify — handled per-controller + "chatId", // AccessControlService.assertCanAccessChat owns this + "workspaceId", // DashboardWorkspaceService membership owns this + "dashboardId", // SavedDashboardService owns this + "tokenId", // MCP tokens, scoped to the authenticated caller + "jobId", // resolved to its connection by SlowLogSourceController + "threadId", // agent conversation, scoped by userId + "conversationId", + // Playbooks are global templates: the Playbook entity has no connectionId at all, + // so there is no connection to authorize against. The endpoints in that controller + // which *do* carry one (execute, runs, alerts) are guarded — verified, not assumed. + "playbookId" + ); + @Test void everyConnectionScopedEndpointAuthorizesTheCaller() throws IOException { List offenders = new ArrayList<>(); @@ -276,6 +328,25 @@ void authorizationFailuresPropagateAsForbiddenRatherThanServerError() throws IOE .isEmpty(); } + /** + * {@code playbookId} is exempt because {@code Playbook} carries no {@code connectionId} + * — there is genuinely no connection to authorize against. That is a claim about the + * entity, so check it: if a {@code connectionId} is ever added to {@code Playbook}, the + * exemption silently starts hiding four unguarded endpoints + * ({@code GET|PUT|DELETE /playbooks/{id}} and {@code /toggle}). + */ + @Test + void playbookExemptionHoldsOnlyWhilePlaybooksAreConnectionFree() throws IOException { + Path entity = Path.of("src/main/java/com/dbaagent/model/Playbook.java"); + String source = Files.readString(entity); + + assertThat(source) + .as("Playbook has gained a connectionId, so playbooks are no longer global " + + "templates. Remove \"playbookId\" from NOT_CONNECTION_OWNED_IDS and " + + "authorize the id-keyed playbook endpoints against the owning connection.") + .doesNotContain("connectionId"); + } + /** * A {@code @ControllerAdvice} with a catch-all {@code @ExceptionHandler(Exception.class)} * swallows authorization denials the same way an in-method catch-all does, and it is