diff --git a/CLAUDE.md b/CLAUDE.md index 543c223..653c3ff 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -349,6 +349,42 @@ it against a real database — not a theoretical hardening pass. `POST /users/admin/reset` on every install that had run `setup-agent.sh`, since that mints an admin MCP token on each run. +### Endpoint Authorization Rules + +- **Authentication is not authorization.** `SecurityConfig` only asserts + `.anyRequest().authenticated()` and `JwtAuthenticationFilter` only resolves a + principal — neither looks at a `connectionId`. Connections are **private per user** + (`ConnectionAccessService.resolveAccess` keys on `ownerUsername` plus an explicit + grant table), so any endpoint taking a caller-supplied `connectionId` **must** call + `accessControlService.assertCanReadConnectionContent` (reads) or + `assertCanManageConnectionContent` (writes) itself. There is no filter, interceptor + or aspect that does this for you. +- **`BrainController` shipped with 93 of its 116 endpoints unguarded.** Only the first + ~15 (`/understanding`, `/notes/*`, `/tasks/*`, `/key-columns/*`, + `/inferred-relationships/*`) had the check; every later "Phase" block did not — so an + authenticated user could pass someone else's connection id to + `/brain/health-scores/{id}`, `/brain/data-sensitivity/{id}` (which names the PII + columns), `/brain/cost-attribution/{id}`, `/brain/ml-overview/{id}` and ~90 more and + read that user's database intelligence. All 116 are now guarded, and + `BrainControllerAuthorizationSafetyTest` fails the build if a new one is not. The + misses clustered by **when a section was written**, not by read/write semantics — + when adding a controller section, guard it as you write it. +- **When the path carries some other id** (`simulationId`, `experimentId`, `patternId`, + `noteId`, `taskId`), resolve the owning connection first via that service's + `getConnectionId(id)` and assert on the result. Do not skip the check because the + path has no `connectionId` in it. +- **An endpoint with no connection scope at all is admin-only.** + `POST /brain/column-values/embed-all` spans every connection, so it carries + `@PreAuthorize("hasRole('ADMIN')")` — it cannot be authorized against one + connection's grants. `@EnableMethodSecurity(prePostEnabled = true)` is on in + `SecurityConfig`, so `@PreAuthorize` is live. +- **Assert inside the `try`, and rethrow `ResponseStatusException` before the + catch-all.** Every handler in `BrainController` ends with a + `catch (Exception) -> 500`; without the earlier + `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. + ### MCP & CLI Release Rules **Whenever you add, rename, or remove an MCP tool or a CLI subcommand, you MUST update all of these in the same commit — they are agent-facing surfaces and drift silently breaks discoverability:** diff --git a/backend/src/main/java/com/dbaagent/controller/BrainController.java b/backend/src/main/java/com/dbaagent/controller/BrainController.java index 9d4560a..636cdc3 100644 --- a/backend/src/main/java/com/dbaagent/controller/BrainController.java +++ b/backend/src/main/java/com/dbaagent/controller/BrainController.java @@ -70,6 +70,7 @@ import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.http.ResponseEntity; +import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.web.bind.annotation.DeleteMapping; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PathVariable; @@ -391,7 +392,14 @@ public ResponseEntity analyzeKeyColumns( } } + /** + * Not implemented. Kept admin-only rather than guarded per connection: the body + * never loads the anti-pattern, so there is no connection to authorize against. + * Whoever implements it must resolve the owning connection from {@code patternId} + * and switch to {@code assertCanManageConnectionContent}. + */ @PostMapping("/key-columns/anti-pattern/{patternId}/acknowledge") + @PreAuthorize("hasRole('ADMIN')") public ResponseEntity> acknowledgeAntiPattern( @PathVariable Long patternId ) { @@ -600,6 +608,7 @@ public ResponseEntity> analyzeQueryAntiPatterns( @PathVariable String connectionId ) { try { + accessControlService.assertCanManageConnectionContent(connectionId); queryQualityAnalysisService.analyzeQueryQuality(connectionId); List patterns = queryQualityAnalysisService.getAllQueryAntiPatterns(connectionId); @@ -631,6 +640,7 @@ public ResponseEntity> getQueryAntiPatternsBySeverity( @PathVariable String severity ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); QueryAntiPattern.Severity severityEnum = QueryAntiPattern.Severity.valueOf(severity.toUpperCase()); // Note: Need to add this method to QueryQualityAnalysisService return ResponseEntity.ok(List.of()); // TODO: Implement in service @@ -651,6 +661,7 @@ public ResponseEntity> getScalabilitySimulations( @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(scalabilitySimulationService.getAllSimulations(connectionId)); } catch (ResponseStatusException e) { throw e; @@ -665,6 +676,7 @@ public ResponseEntity getLatestSimulation( @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return scalabilitySimulationService.getLatestSimulation(connectionId) .map(ResponseEntity::ok) .orElse(ResponseEntity.notFound().build()); @@ -682,6 +694,7 @@ public ResponseEntity> runScalabilitySimulation( @RequestParam(required = false) String scenario ) { try { + accessControlService.assertCanManageConnectionContent(connectionId); if (scenario != null && !scenario.isEmpty()) { // Run single scenario ScalabilitySimulation.GrowthScenario scenarioEnum = @@ -708,7 +721,10 @@ public ResponseEntity> getTablePredictions( @PathVariable String simulationId ) { try { + accessControlService.assertCanReadConnectionContent(scalabilitySimulationService.getConnectionId(simulationId)); return ResponseEntity.ok(scalabilitySimulationService.getTablePredictions(simulationId)); + } catch (IllegalArgumentException e) { + return ResponseEntity.notFound().build(); } catch (ResponseStatusException e) { throw e; } catch (Exception e) { @@ -722,6 +738,7 @@ public ResponseEntity> getHighRiskTables( @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(scalabilitySimulationService.getHighRiskTables(connectionId)); } catch (ResponseStatusException e) { throw e; @@ -738,6 +755,7 @@ public ResponseEntity getLatestBrainScore( @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return brainScoreService.getLatestBrainScore(connectionId) .map(ResponseEntity::ok) .orElse(ResponseEntity.notFound().build()); @@ -755,6 +773,7 @@ public ResponseEntity> getBrainScoreHistory( @RequestParam(defaultValue = "10") int limit ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(brainScoreService.getBrainScoreHistory(connectionId, limit)); } catch (ResponseStatusException e) { throw e; @@ -769,6 +788,7 @@ public ResponseEntity calculateBrainScore( @PathVariable String connectionId ) { try { + accessControlService.assertCanManageConnectionContent(connectionId); BrainScore score = brainScoreService.calculateBrainScore(connectionId); return ResponseEntity.ok(score); } catch (ResponseStatusException e) { @@ -786,6 +806,7 @@ public ResponseEntity @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(antiPatternService.detectAntiPatterns(connectionId)); } catch (ResponseStatusException e) { throw e; @@ -814,6 +836,7 @@ public ResponseEntity> get @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(healthScoreService.calculateHealthScores(connectionId)); } catch (ResponseStatusException e) { throw e; @@ -842,6 +866,7 @@ public ResponseEntity> @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(partitionService.evaluatePartitionReadiness(connectionId)); } catch (ResponseStatusException e) { throw e; @@ -884,6 +911,7 @@ public ResponseEntity> getRelationshipClas @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(relationshipClassificationService.getRelationships(connectionId)); } catch (ResponseStatusException e) { throw e; @@ -898,6 +926,7 @@ public ResponseEntity> getIntegrityIssues( @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(relationshipClassificationService.getIntegrityIssues(connectionId)); } catch (ResponseStatusException e) { throw e; @@ -912,6 +941,7 @@ public ResponseEntity> getMissingRelations @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(relationshipClassificationService.getMissingIndexes(connectionId)); } catch (ResponseStatusException e) { throw e; @@ -927,6 +957,7 @@ public ResponseEntity> getTablesBySensitivity( @PathVariable String level ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> level.equalsIgnoreCase(tc.getSensitivityLevel())) @@ -946,6 +977,7 @@ public ResponseEntity> getTablesByDomain( @PathVariable String domain ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> domain.equalsIgnoreCase(tc.getBusinessDomain())) @@ -964,6 +996,7 @@ public ResponseEntity> getPartitionCandidates( @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> tc.getPartitionReadiness() != null && @@ -984,6 +1017,7 @@ public ResponseEntity> getTablesWithAntiPatterns( @RequestParam(required = false) String severity ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> tc.getAntiPatternCount() != null && tc.getAntiPatternCount() > 0) @@ -1004,6 +1038,7 @@ public ResponseEntity> getLowHealthTables( @RequestParam(defaultValue = "60") double threshold ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> tc.getHealthScore() != null && tc.getHealthScore().doubleValue() < threshold) @@ -1025,6 +1060,7 @@ public ResponseEntity> getCo @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(costService.calculateCostAttribution(connectionId)); } catch (ResponseStatusException e) { throw e; @@ -1109,6 +1150,7 @@ public ResponseEntity> g @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(shardingService.assessShardingReadiness(connectionId)); } catch (ResponseStatusException e) { throw e; @@ -1123,6 +1165,7 @@ public ResponseEntity> get @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(dataQualityService.calculateDataQuality(connectionId)); } catch (ResponseStatusException e) { throw e; @@ -1137,6 +1180,7 @@ public ResponseEntity> get @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(growthPredictionService.predictGrowth(connectionId)); } catch (ResponseStatusException e) { throw e; @@ -1168,6 +1213,7 @@ public ResponseEntity> getTablesByLifecycle( @PathVariable String lifecycle ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> lifecycle.equalsIgnoreCase(tc.getDataLifecycle())) @@ -1186,6 +1232,7 @@ public ResponseEntity> getHighCacheValueTables( @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> "HIGH_CACHE_VALUE".equalsIgnoreCase(tc.getCacheAffinity())) @@ -1205,6 +1252,7 @@ public ResponseEntity> getTablesBySchemaRisk( @PathVariable String level ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> level.equalsIgnoreCase(tc.getSchemaEvolutionRisk())) @@ -1223,6 +1271,7 @@ public ResponseEntity> getDenormalizationCandidateTabl @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> Boolean.TRUE.equals(tc.getDenormalizationCandidate())) @@ -1241,6 +1290,7 @@ public ResponseEntity> getShardingReadyTables( @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> "READY".equalsIgnoreCase(tc.getShardingReadiness())) @@ -1260,6 +1310,7 @@ public ResponseEntity> getLowQualityTables( @RequestParam(defaultValue = "50") double threshold ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> tc.getDataQualityScore() != null && tc.getDataQualityScore().doubleValue() < threshold) @@ -1279,6 +1330,7 @@ public ResponseEntity> getHighDependencyCriticalityTab @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> "CRITICAL".equalsIgnoreCase(tc.getDependencyCriticality()) || @@ -1298,6 +1350,7 @@ public ResponseEntity> getRapidGrowthTables( @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> "EXPLOSIVE".equalsIgnoreCase(tc.getGrowthCategory()) || @@ -1318,6 +1371,7 @@ public ResponseEntity> getTablesByCostTier( @PathVariable String tier ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> tier.equalsIgnoreCase(tc.getCostTier())) @@ -1343,6 +1397,7 @@ public ResponseEntity> getColumnValues( @RequestParam(required = false) String tableName ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List values = columnValueCollectionService.getCachedColumns(connectionId, tableName); return ResponseEntity.ok(values); } catch (ResponseStatusException e) { @@ -1361,6 +1416,7 @@ public ResponseEntity> getColumnValueStats( @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); Map stats = columnValueCollectionService.getStatistics(connectionId); return ResponseEntity.ok(stats); } catch (ResponseStatusException e) { @@ -1380,6 +1436,7 @@ public ResponseEntity> refreshColumnValues( @PathVariable String connectionId ) { try { + accessControlService.assertCanManageConnectionContent(connectionId); log.info("Manually triggering column value refresh for connection: {}", connectionId); // Run async and return immediately @@ -1409,8 +1466,12 @@ public ResponseEntity> refreshColumnValues( /** * Embed all unembedded column values. * Useful for re-syncing Azure AI Search after migration or data loss. + * + *

Spans every connection with no per-connection scope, so it cannot be + * authorized against a single connection's grants — admin only. */ @PostMapping("/column-values/embed-all") + @PreAuthorize("hasRole('ADMIN')") public ResponseEntity> embedAllColumnValues() { try { int count = columnValueCollectionService.embedAllUnembedded(); @@ -1436,6 +1497,7 @@ public ResponseEntity> embedAllColumnValues() { @PostMapping("/insights/{connectionId}/embed") public ResponseEntity> embedBrainInsights(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); log.info("Embedding Brain insights for connection: {}", connectionId); Map result = brainInsightEmbeddingService.embedAllInsights(connectionId); return ResponseEntity.ok(result); @@ -1457,6 +1519,7 @@ public ResponseEntity> embedBrainInsights(@PathVariable Stri @PostMapping("/insights/{connectionId}/embed/patterns") public ResponseEntity> embedPlanPatterns(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); int count = brainInsightEmbeddingService.embedPlanPatterns(connectionId); return ResponseEntity.ok(Map.of( "success", true, @@ -1477,6 +1540,7 @@ public ResponseEntity> embedPlanPatterns(@PathVariable Strin @PostMapping("/insights/{connectionId}/embed/workload") public ResponseEntity> embedWorkloadInsight(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); boolean success = brainInsightEmbeddingService.embedWorkloadInsight(connectionId); return ResponseEntity.ok(Map.of( "success", success, @@ -1496,6 +1560,7 @@ public ResponseEntity> embedWorkloadInsight(@PathVariable St @PostMapping("/insights/{connectionId}/embed/cardinality") public ResponseEntity> embedCardinalityInsights(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); int count = brainInsightEmbeddingService.embedCardinalityInsights(connectionId); return ResponseEntity.ok(Map.of( "success", true, @@ -1518,6 +1583,7 @@ public ResponseEntity> embedCardinalityInsights(@PathVariabl @PostMapping("/workload/collect/{connectionId}") public ResponseEntity collectWorkloadMetrics(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); log.info("Collecting workload metrics for connection: {}", connectionId); WorkloadMetricsSnapshot snapshot = metricsCollectorService.collectMetrics(connectionId); return ResponseEntity.ok(snapshot); @@ -1535,6 +1601,7 @@ public ResponseEntity collectWorkloadMetrics(@PathVaria @PostMapping("/workload/characterize/{connectionId}") public ResponseEntity characterizeWorkload(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); log.info("Characterizing workload for connection: {}", connectionId); WorkloadProfile profile = workloadCharacterizationService.characterizeWorkload(connectionId); return ResponseEntity.ok(profile); @@ -1552,6 +1619,7 @@ public ResponseEntity characterizeWorkload(@PathVariable String @GetMapping("/workload/profile/{connectionId}") public ResponseEntity getWorkloadProfile(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return workloadCharacterizationService.getProfile(connectionId) .map(ResponseEntity::ok) .orElse(ResponseEntity.notFound().build()); @@ -1570,6 +1638,7 @@ public ResponseEntity getWorkloadProfile(@PathVariable String c @GetMapping("/workload/status/{connectionId}") public ResponseEntity> getWorkloadStatus(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); Map status = new java.util.HashMap<>(); // Snapshot count @@ -1620,6 +1689,7 @@ public ResponseEntity> findSimilarWorkloads( @PathVariable String connectionId, @RequestParam(defaultValue = "5") int limit) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List similar = workloadCharacterizationService.findSimilarProfiles(connectionId, limit); return ResponseEntity.ok(similar); } catch (ResponseStatusException e) { @@ -1640,6 +1710,7 @@ public ResponseEntity> identifyKnobs( @PathVariable String connectionId, @RequestParam(defaultValue = "LATENCY") KnobRanking.TargetMetric targetMetric) { try { + accessControlService.assertCanManageConnectionContent(connectionId); log.info("Identifying knobs for connection: {} targeting: {}", connectionId, targetMetric); List rankings = knobIdentificationService.identifyKnobs(connectionId, targetMetric); return ResponseEntity.ok(rankings); @@ -1660,6 +1731,7 @@ public ResponseEntity> getTopKnobs( @RequestParam(defaultValue = "LATENCY") KnobRanking.TargetMetric targetMetric, @RequestParam(defaultValue = "5") int limit) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List topKnobs = knobIdentificationService.getTopKnobs(connectionId, targetMetric, limit); return ResponseEntity.ok(topKnobs); } catch (ResponseStatusException e) { @@ -1676,6 +1748,7 @@ public ResponseEntity> getTopKnobs( @GetMapping("/config/rankings/{connectionId}") public ResponseEntity> getAllKnobRankings(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List rankings = knobIdentificationService.getAllRankings(connectionId); return ResponseEntity.ok(rankings); } catch (ResponseStatusException e) { @@ -1693,6 +1766,7 @@ public ResponseEntity> getAllKnobRankings(@PathVariable String public ResponseEntity> generateConfigRecommendations( @PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); log.info("Generating ML-based config recommendations for: {}", connectionId); List recommendations = configTuningService.generateRecommendations(connectionId); return ResponseEntity.ok(recommendations); @@ -1712,6 +1786,7 @@ public ResponseEntity startTuningExperiment( @PathVariable String connectionId, @org.springframework.web.bind.annotation.RequestBody ExperimentRequest request) { try { + accessControlService.assertCanManageConnectionContent(connectionId); log.info("Starting tuning experiment for: {}", connectionId); TuningExperiment experiment = configTuningService.startExperiment( connectionId, request.getKnobChanges(), request.getRecommendationId()); @@ -1732,6 +1807,7 @@ public ResponseEntity startTuningExperiment( @PostMapping("/config/experiments/{experimentId}/complete") public ResponseEntity completeTuningExperiment(@PathVariable String experimentId) { try { + accessControlService.assertCanManageConnectionContent(configTuningService.getConnectionId(experimentId)); log.info("Completing experiment: {}", experimentId); TuningExperiment experiment = configTuningService.completeExperiment(experimentId); return ResponseEntity.ok(experiment); @@ -1751,6 +1827,7 @@ public ResponseEntity completeTuningExperiment(@PathVariable S @DeleteMapping("/config/experiments/{experimentId}") public ResponseEntity cancelTuningExperiment(@PathVariable String experimentId) { try { + accessControlService.assertCanManageConnectionContent(configTuningService.getConnectionId(experimentId)); log.info("Cancelling experiment: {}", experimentId); configTuningService.cancelExperiment(experimentId); return ResponseEntity.noContent().build(); @@ -1772,6 +1849,7 @@ public ResponseEntity> getExperimentHistory( @PathVariable String connectionId, @RequestParam(defaultValue = "10") int limit) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List history = configTuningService.getExperimentHistory(connectionId, limit); return ResponseEntity.ok(history); } catch (ResponseStatusException e) { @@ -1788,6 +1866,7 @@ public ResponseEntity> getExperimentHistory( @GetMapping("/config/experiments/{connectionId}/success-rate") public ResponseEntity> getExperimentSuccessRate(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); double successRate = configTuningService.getExperimentSuccessRate(connectionId); return ResponseEntity.ok(Map.of( "connectionId", connectionId, @@ -1811,6 +1890,7 @@ public ResponseEntity> collectTableStatistics( @PathVariable String connectionId, @PathVariable String tableName) { try { + accessControlService.assertCanManageConnectionContent(connectionId); log.info("Collecting statistics for table: {} in connection: {}", tableName, connectionId); List stats = cardinalityEstimationService.collectTableStatistics(connectionId, tableName); return ResponseEntity.ok(stats); @@ -1828,6 +1908,7 @@ public ResponseEntity> collectTableStatistics( @GetMapping("/statistics/{connectionId}") public ResponseEntity> getColumnStatistics(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List stats = cardinalityEstimationService.getStatistics(connectionId); return ResponseEntity.ok(stats); } catch (ResponseStatusException e) { @@ -1846,6 +1927,7 @@ public ResponseEntity> getHighCardinalityColumns( @PathVariable String connectionId, @RequestParam(defaultValue = "1000") long minDistinct) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List columns = cardinalityEstimationService.getHighCardinalityColumns(connectionId, minDistinct); return ResponseEntity.ok(columns); } catch (ResponseStatusException e) { @@ -1862,6 +1944,7 @@ public ResponseEntity> getHighCardinalityColumns( @GetMapping("/statistics/{connectionId}/skewed") public ResponseEntity> getSkewedColumns(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List columns = cardinalityEstimationService.getSkewedColumns(connectionId); return ResponseEntity.ok(columns); } catch (ResponseStatusException e) { @@ -1880,6 +1963,7 @@ public ResponseEntity> estimateCardinality( @PathVariable String connectionId, @org.springframework.web.bind.annotation.RequestBody CardinalityEstimateRequest request) { try { + accessControlService.assertCanManageConnectionContent(connectionId); long estimate; if (request.getLowBound() != null || request.getHighBound() != null) { estimate = cardinalityEstimationService.estimateRange( @@ -1909,6 +1993,7 @@ public ResponseEntity> estimateCardinality( @PostMapping("/statistics/{connectionId}/refresh") public ResponseEntity> refreshStaleStatistics(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); int refreshed = cardinalityEstimationService.refreshStaleStatistics(connectionId); return ResponseEntity.ok(Map.of("refreshedCount", refreshed)); } catch (ResponseStatusException e) { @@ -1926,6 +2011,7 @@ public ResponseEntity> refreshStaleStatistics(@PathVariable @GetMapping("/statistics/{connectionId}/accuracy") public ResponseEntity> getCardinalityAccuracy(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); Map accuracy = adaptivePlanScoringService.getCardinalityAccuracyStats(connectionId); return ResponseEntity.ok(accuracy); } catch (ResponseStatusException e) { @@ -1946,6 +2032,7 @@ public ResponseEntity recordExecution( @PathVariable String connectionId, @org.springframework.web.bind.annotation.RequestBody ExecutionRecordRequest request) { try { + accessControlService.assertCanManageConnectionContent(connectionId); PlanExecution execution = adaptivePlanScoringService.recordExecution( connectionId, request.getQuery(), request.getActualExecutionMs(), request.getActualRows()); return ResponseEntity.ok(execution); @@ -1965,6 +2052,7 @@ public ResponseEntity> getRecentExecutions( @PathVariable String connectionId, @RequestParam(defaultValue = "20") int limit) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List executions = adaptivePlanScoringService.getRecentExecutions(connectionId, limit); return ResponseEntity.ok(executions); } catch (ResponseStatusException e) { @@ -1983,6 +2071,7 @@ public ResponseEntity> getCardinalityErrors( @PathVariable String connectionId, @RequestParam(defaultValue = "10") int limit) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List errors = adaptivePlanScoringService.getSignificantCardinalityErrors(connectionId, limit); return ResponseEntity.ok(errors); } catch (ResponseStatusException e) { @@ -1999,6 +2088,7 @@ public ResponseEntity> getCardinalityErrors( @GetMapping("/calibration/{connectionId}") public ResponseEntity> getCalibrationStatus(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); Map status = adaptivePlanScoringService.getCalibrationStatus(connectionId); return ResponseEntity.ok(status); } catch (ResponseStatusException e) { @@ -2015,6 +2105,7 @@ public ResponseEntity> getCalibrationStatus(@PathVariable St @GetMapping("/executions/{connectionId}/multiple-plans") public ResponseEntity>> getQueriesWithMultiplePlans(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); Map> result = adaptivePlanScoringService.findQueriesWithMultiplePlans(connectionId); return ResponseEntity.ok(result); } catch (ResponseStatusException e) { @@ -2028,9 +2119,10 @@ public ResponseEntity>> getQueriesWithMultiplePlans(@Pa /** * Reset cost calibration. */ - @org.springframework.web.bind.annotation.DeleteMapping("/calibration/{connectionId}") + @DeleteMapping("/calibration/{connectionId}") public ResponseEntity resetCalibration(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); adaptivePlanScoringService.resetCalibration(connectionId); return ResponseEntity.ok().build(); } catch (ResponseStatusException e) { @@ -2051,6 +2143,7 @@ public ResponseEntity>> getPatternSuggestions( @PathVariable String connectionId, @org.springframework.web.bind.annotation.RequestBody QueryRequest request) { try { + accessControlService.assertCanManageConnectionContent(connectionId); List> suggestions = planPatternLibraryService.getSuggestions(connectionId, request.getQuery()); return ResponseEntity.ok(suggestions); } catch (ResponseStatusException e) { @@ -2067,6 +2160,7 @@ public ResponseEntity>> getPatternSuggestions( @GetMapping("/patterns/{connectionId}/reliable") public ResponseEntity> getReliablePatterns(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List patterns = planPatternLibraryService.getReliablePatterns(connectionId); return ResponseEntity.ok(patterns); } catch (ResponseStatusException e) { @@ -2085,6 +2179,7 @@ public ResponseEntity> getMostUsedPatterns( @PathVariable String connectionId, @RequestParam(defaultValue = "10") int limit) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List patterns = planPatternLibraryService.getMostUsedPatterns(connectionId, limit); return ResponseEntity.ok(patterns); } catch (ResponseStatusException e) { @@ -2101,6 +2196,7 @@ public ResponseEntity> getMostUsedPatterns( @GetMapping("/patterns/{connectionId}/with-optimizations") public ResponseEntity> getPatternsWithOptimizations(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List patterns = planPatternLibraryService.getPatternsWithOptimizations(connectionId); return ResponseEntity.ok(patterns); } catch (ResponseStatusException e) { @@ -2117,6 +2213,7 @@ public ResponseEntity> getPatternsWithOptimizations(@PathVaria @GetMapping("/patterns/{connectionId}/stats") public ResponseEntity> getPatternStatistics(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); Map stats = planPatternLibraryService.getPatternStatistics(connectionId); return ResponseEntity.ok(stats); } catch (ResponseStatusException e) { @@ -2135,8 +2232,11 @@ public ResponseEntity recordPatternFeedback( @PathVariable String patternId, @RequestParam boolean wasSuccessful) { try { + accessControlService.assertCanManageConnectionContent(planPatternLibraryService.getConnectionId(patternId)); planPatternLibraryService.recordFeedback(patternId, wasSuccessful); return ResponseEntity.ok().build(); + } catch (IllegalArgumentException e) { + return ResponseEntity.notFound().build(); } catch (ResponseStatusException e) { throw e; } catch (Exception e) { @@ -2151,6 +2251,7 @@ public ResponseEntity recordPatternFeedback( @PostMapping("/patterns/{connectionId}/cleanup") public ResponseEntity> cleanupPatterns(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); int deleted = planPatternLibraryService.cleanupIneffectivePatterns(connectionId); return ResponseEntity.ok(Map.of("deletedCount", deleted)); } catch (ResponseStatusException e) { @@ -2169,6 +2270,7 @@ public ResponseEntity> cleanupPatterns(@PathVariable String @GetMapping("/ml-overview/{connectionId}") public ResponseEntity> getMlOverview(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); Map overview = new java.util.HashMap<>(); // Workload Profile @@ -2211,6 +2313,7 @@ public ResponseEntity> getMlOverview(@PathVariable String co @DeleteMapping("/query-intelligence/{connectionId}/executions") public ResponseEntity> clearExecutions(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); long countBefore = adaptivePlanScoringService.getRecentExecutions(connectionId, 1).isEmpty() ? 0 : adaptivePlanScoringService.getCardinalityAccuracyStats(connectionId).get("totalExecutions") instanceof Number n ? n.longValue() : 0; adaptivePlanScoringService.clearExecutions(connectionId); @@ -2238,6 +2341,7 @@ public ResponseEntity> clearExecutions(@PathVariable String @PostMapping("/query-intelligence/{connectionId}/backfill") public ResponseEntity> backfillQueryIntelligence(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); log.info("Starting Query Intelligence backfill for connection {}", connectionId); // Get history summaries (lightweight) instead of full entities to avoid OOM @@ -2302,6 +2406,7 @@ public ResponseEntity startExplai @PathVariable String connectionId, @RequestParam(defaultValue = "500") int maxQueries) { try { + accessControlService.assertCanManageConnectionContent(connectionId); // Limit max queries to prevent runaway jobs int safeMax = Math.min(maxQueries, 2000); var progress = adaptivePlanScoringService.startExplainBackfill(connectionId, safeMax); @@ -2321,6 +2426,7 @@ public ResponseEntity startExplai public ResponseEntity getExplainProgress( @PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); var progress = adaptivePlanScoringService.getExplainProgress(connectionId); return ResponseEntity.ok(progress); } catch (ResponseStatusException e) { @@ -2337,6 +2443,7 @@ public ResponseEntity getExplainP @PostMapping("/query-intelligence/{connectionId}/explain-cancel") public ResponseEntity cancelExplainBackfill(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); adaptivePlanScoringService.cancelExplainBackfill(connectionId); return ResponseEntity.ok().build(); } catch (ResponseStatusException e) { diff --git a/backend/src/main/java/com/dbaagent/service/brain/analysis/ScalabilitySimulationService.java b/backend/src/main/java/com/dbaagent/service/brain/analysis/ScalabilitySimulationService.java index e2f541b..974ec6e 100644 --- a/backend/src/main/java/com/dbaagent/service/brain/analysis/ScalabilitySimulationService.java +++ b/backend/src/main/java/com/dbaagent/service/brain/analysis/ScalabilitySimulationService.java @@ -504,6 +504,12 @@ public List getTablePredictions(String simulationId) { return predictionRepository.findByScalabilitySimulationId(simulationId); } + public String getConnectionId(String simulationId) { + return simulationRepository.findById(simulationId) + .map(ScalabilitySimulation::getConnectionId) + .orElseThrow(() -> new IllegalArgumentException("Simulation not found")); + } + /** * Get high-risk tables. */ diff --git a/backend/src/main/java/com/dbaagent/service/brain/config/ConfigTuningService.java b/backend/src/main/java/com/dbaagent/service/brain/config/ConfigTuningService.java index a472e79..1af5306 100644 --- a/backend/src/main/java/com/dbaagent/service/brain/config/ConfigTuningService.java +++ b/backend/src/main/java/com/dbaagent/service/brain/config/ConfigTuningService.java @@ -723,6 +723,12 @@ public void cancelExperiment(String experimentId) { experimentRepository.delete(experiment); } + public String getConnectionId(String experimentId) { + return experimentRepository.findById(experimentId) + .map(TuningExperiment::getConnectionId) + .orElseThrow(() -> new IllegalArgumentException("Experiment not found: " + experimentId)); + } + /** * Get experiment history. */ diff --git a/backend/src/main/java/com/dbaagent/service/brain/query/PlanPatternLibraryService.java b/backend/src/main/java/com/dbaagent/service/brain/query/PlanPatternLibraryService.java index d60cb18..db5abb1 100644 --- a/backend/src/main/java/com/dbaagent/service/brain/query/PlanPatternLibraryService.java +++ b/backend/src/main/java/com/dbaagent/service/brain/query/PlanPatternLibraryService.java @@ -194,6 +194,12 @@ public void recordFeedback(String patternId, boolean wasSuccessful) { } } + public String getConnectionId(String patternId) { + return patternRepository.findById(patternId) + .map(PlanPattern::getConnectionId) + .orElseThrow(() -> new IllegalArgumentException("Pattern not found: " + patternId)); + } + /** * Get reliable patterns for a connection. */ diff --git a/backend/src/test/java/com/dbaagent/controller/BrainControllerAuthorizationSafetyTest.java b/backend/src/test/java/com/dbaagent/controller/BrainControllerAuthorizationSafetyTest.java new file mode 100644 index 0000000..fd76a56 --- /dev/null +++ b/backend/src/test/java/com/dbaagent/controller/BrainControllerAuthorizationSafetyTest.java @@ -0,0 +1,131 @@ +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.regex.Matcher; +import java.util.regex.Pattern; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Every {@code /brain/**} endpoint must authorize the caller against the connection it + * touches, not merely require a logged-in user. + * + *

This shipped with 93 of 116 endpoints unguarded. {@code SecurityConfig} only asserts + * {@code .anyRequest().authenticated()} and {@code JwtAuthenticationFilter} only resolves a + * principal — neither inspects a {@code connectionId}. Connections are private per user + * ({@code ConnectionAccessService.resolveAccess} keys on {@code ownerUsername} plus an + * explicit grant table), so an authenticated user who passed somebody else's connection id + * to {@code /brain/health-scores/{id}}, {@code /brain/data-sensitivity/{id}} (which names + * the PII columns), {@code /brain/cost-attribution/{id}} and ~90 others got that user's + * database intelligence back. + * + *

Resource-level authorization here is opt-in per method, and the misses clustered by + * when a section was written rather than by read/write semantics — the first ~15 endpoints + * had it and every later "Phase" block did not. That is a defect a reviewer catches once + * and a test catches forever, so it is asserted structurally: scanned as text, because the + * property under test is that no endpoint method body lacks the call, whatever it does. + */ +class BrainControllerAuthorizationSafetyTest { + + private static final Path CONTROLLER = + Path.of("src/main/java/com/dbaagent/controller/BrainController.java"); + + /** + * Matches a fully-qualified annotation as well as a bare one. A + * {@code @org.springframework.web.bind.annotation.DeleteMapping} on + * {@code DELETE /calibration/{connectionId}} is exactly how the one destructive + * endpoint escaped the first sweep of this fix — a bare-name-only pattern silently + * skips it, so the endpoint reads as "not an endpoint" rather than "unguarded". + */ + private static final Pattern MAPPING = Pattern.compile( + "^\\s*@(?:[\\w.]*\\.)?(Get|Post|Delete|Put|Patch)Mapping\\b"); + + /** A method is authorized by a per-connection assert, or by being admin-only. */ + private static final Pattern AUTHORIZED = Pattern.compile( + "accessControlService\\.assertCan(Read|Manage)ConnectionContent\\(|@PreAuthorize"); + + private record Endpoint(int line, String mapping, String body) {} + + /** + * Slices the controller into one entry per handler: from its mapping annotation to the + * method's closing brace, which at this nesting level is a line that is exactly + * {@code " }"}. + */ + private static List endpoints() throws IOException { + List lines = Files.readAllLines(CONTROLLER); + List endpoints = new ArrayList<>(); + + for (int i = 0; i < lines.size(); i++) { + Matcher matcher = MAPPING.matcher(lines.get(i)); + if (!matcher.find()) { + continue; + } + StringBuilder body = new StringBuilder(); + int end = i; + while (end < lines.size()) { + body.append(lines.get(end)).append('\n'); + if (end > i && lines.get(end).equals(" }")) { + break; + } + end++; + } + endpoints.add(new Endpoint(i + 1, lines.get(i).trim(), body.toString())); + } + return endpoints; + } + + @Test + void everyBrainEndpointAuthorizesTheCallerAgainstTheConnection() throws IOException { + List offenders = new ArrayList<>(); + + for (Endpoint endpoint : endpoints()) { + if (!AUTHORIZED.matcher(endpoint.body()).find()) { + offenders.add(CONTROLLER + ":" + endpoint.line() + " " + endpoint.mapping()); + } + } + + assertThat(offenders) + .as("Each of these BrainController endpoints takes a caller-supplied id and " + + "never authorizes it. Authentication is not authorization: connections " + + "are private per user, so this hands one user another user's schema, " + + "sensitivity, cost and workload intelligence. Add " + + "accessControlService.assertCanReadConnectionContent(connectionId) to " + + "reads and assertCanManageConnectionContent(connectionId) to writes, " + + "resolving the connection id first when the path carries some other id. " + + "An endpoint with no connection scope at all is admin-only (@PreAuthorize).") + .isEmpty(); + } + + /** + * The asserts live inside each handler's {@code try}, and every handler ends with a + * {@code catch (Exception)} that returns 500. Without an earlier + * {@code catch (ResponseStatusException e) { throw e; }} the 403 would be swallowed and + * reported as a server error — the denial would still hold, but it would look like a + * bug in the feature rather than a permission boundary, and a client could not tell + * "not yours" from "broken". + */ + @Test + void authorizationFailuresPropagateAsForbiddenRatherThanServerError() throws IOException { + List offenders = new ArrayList<>(); + + for (Endpoint endpoint : endpoints()) { + String body = endpoint.body(); + boolean guardedInline = body.contains("accessControlService.assertCan"); + if (guardedInline && !body.contains("catch (ResponseStatusException e)")) { + offenders.add(CONTROLLER + ":" + 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; }") + .isEmpty(); + } +}