diff --git a/CLAUDE.md b/CLAUDE.md index c5c7cab..e4e227d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -709,6 +709,103 @@ 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 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`; `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 + 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..8400105 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,17 @@ 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 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.assertCanManageConnectionContentOrNotFound(action.getConnectionId(), "Action"); + } } 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..e5eb443 100644 --- a/backend/src/main/java/com/dbaagent/controller/ProjectController.java +++ b/backend/src/main/java/com/dbaagent/controller/ProjectController.java @@ -2,21 +2,38 @@ 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; 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: 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") @RequiredArgsConstructor public class ProjectController { private final ProjectService projectService; + private final AccessControlService accessControlService; @PostMapping public ResponseEntity createProject(@RequestBody CreateProjectRequest request) { + accessControlService.assertCanManageConnectionContent(request.getConnectionId()); Project project = projectService.createProject( request.getName(), request.getDescription(), @@ -29,17 +46,28 @@ public ResponseEntity createProject(@RequestBody CreateProjectRequest r public ResponseEntity> listProjects( @RequestParam(required = false) String connectionId ) { - List projects = connectionId != null - ? projectService.getProjectsByConnection(connectionId) - : projectService.getAllProjects(); - return ResponseEntity.ok(projects); + if (connectionId != null) { + accessControlService.assertCanReadConnectionContent(connectionId); + return ResponseEntity.ok(projectService.getProjectsByConnection(connectionId)); + } + // 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}") @@ -47,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()); @@ -54,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(); @@ -66,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 a368f32..69e0698 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,24 @@ 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 — 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.assertCanManageConnectionContentOrNotFound(connectionId, "Regression"); + } + + /** 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..38e8736 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)); } @@ -94,9 +108,18 @@ public ResponseEntity> getUnacknowledgedRegressions(@P public ResponseEntity> acknowledgeRegressions( @PathVariable String connectionId, @RequestBody List comparisonIds, - @RequestParam(required = false, defaultValue = "user") String acknowledgedBy) { - - int count = planCacheService.acknowledgeRegressions(comparisonIds, 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)) { + 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 +131,20 @@ 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 — 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.assertCanManageConnectionContentOrNotFound(connectionId, "Plan"); + } } 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..0046103 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,8 +79,15 @@ public ResponseEntity> getRecentSnapshots(@PathVariable Str 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" @@ -84,7 +102,17 @@ public ResponseEntity> compareSnapshots( @RequestParam String snapshotId1, @RequestParam String snapshotId2) { - return ResponseEntity.ok(schemaChangeService.compareSnapshots(snapshotId1, snapshotId2)); + assertCanReadSnapshot(snapshotId1); + assertCanReadSnapshot(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 ==================== @@ -94,6 +122,7 @@ public ResponseEntity> compareSnapshots( */ @GetMapping("/{connectionId}/changes") public ResponseEntity> getChanges(@PathVariable String connectionId) { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(schemaChangeService.getChanges(connectionId)); } @@ -102,6 +131,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)); } @@ -112,9 +142,14 @@ 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); - 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 @@ -127,9 +162,14 @@ 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(connectionId, acknowledgedBy); + int count = schemaChangeService.acknowledgeAllChanges( + connectionId, accessControlService.requireCurrentUsername()); return ResponseEntity.ok(Map.of( "status", "success", "acknowledgedCount", count @@ -141,6 +181,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 +192,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 +205,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 +215,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 +223,44 @@ 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 — 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.assertCanReadConnectionContentOrNotFound(connectionId, "Snapshot"); + } + + /** + * 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"); + } + } + + /** + * 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 9bb2e2f..36984ea 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,11 +227,14 @@ 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") List 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); @@ -256,10 +271,13 @@ 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"); - 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); @@ -297,6 +315,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 +361,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 +396,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 +526,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 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.assertCanManageConnectionContentOrNotFound(connectionId, "Recommendation"); + } } 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..ff04cc2 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,53 @@ 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. + // + // 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) + .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.assertCanReadConnectionContentOrNotFound( + historyConnectionId(historyId), "Analysis"); + } + + private void assertCanManageHistory(String 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.assertCanManageConnectionContentOrNotFound(connectionId, "Alert"); + } + + 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.assertCanReadConnectionContentOrNotFound( + fingerprintConnectionId(fingerprintId), "Fingerprint"); + } + + private void assertCanManageFingerprint(String fingerprintId) { + accessControlService.assertCanManageConnectionContentOrNotFound( + fingerprintConnectionId(fingerprintId), "Fingerprint"); + } } 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..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 -> { @@ -655,9 +664,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/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 new file mode 100644 index 0000000..4ad60b2 --- /dev/null +++ b/backend/src/test/java/com/dbaagent/controller/ConnectionScopedAuthorizationSafetyTest.java @@ -0,0 +1,454 @@ +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) { + 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<>(); + + 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(); + } + + /** + * {@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 + * 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()