diff --git a/backend/src/main/java/com/bablsoft/accessflow/proxy/internal/DefaultQueryDryRunService.java b/backend/src/main/java/com/bablsoft/accessflow/proxy/internal/DefaultQueryDryRunService.java index 3233fb1c..0545b068 100644 --- a/backend/src/main/java/com/bablsoft/accessflow/proxy/internal/DefaultQueryDryRunService.java +++ b/backend/src/main/java/com/bablsoft/accessflow/proxy/internal/DefaultQueryDryRunService.java @@ -1,5 +1,6 @@ package com.bablsoft.accessflow.proxy.internal; +import com.bablsoft.accessflow.core.api.AllowedTables; import com.bablsoft.accessflow.core.api.DatasourceAdminService; import com.bablsoft.accessflow.core.api.DatasourceUserPermissionLookupService; import com.bablsoft.accessflow.core.api.DatasourceUserPermissionView; @@ -21,9 +22,7 @@ import org.springframework.security.access.AccessDeniedException; import org.springframework.stereotype.Service; -import java.util.ArrayList; import java.util.List; -import java.util.Locale; import java.util.Set; import java.util.TreeSet; import java.util.UUID; @@ -106,8 +105,8 @@ private void verifyPermission(UUID userId, UUID datasourceId, SqlParseResult par private void verifyAllowedTables(DatasourceUserPermissionView permission, UUID datasourceId, Set referencedTables) { - var allowedSchemas = normalizeList(permission.allowedSchemas()); - var allowedTables = normalizeList(permission.allowedTables()); + var allowedSchemas = AllowedTables.normalize(permission.allowedSchemas()); + var allowedTables = AllowedTables.normalize(permission.allowedTables()); if (allowedSchemas.isEmpty() && allowedTables.isEmpty()) { return; } @@ -116,14 +115,9 @@ private void verifyAllowedTables(DatasourceUserPermissionView permission, UUID d } var rejected = new TreeSet(); for (String table : referencedTables) { - if (allowedTables.contains(table)) { - continue; + if (AllowedTables.coveringEntry(allowedSchemas, allowedTables, table) == null) { + rejected.add(table); } - int dotIdx = table.indexOf('.'); - if (dotIdx > 0 && allowedSchemas.contains(table.substring(0, dotIdx))) { - continue; - } - rejected.add(table); } if (!rejected.isEmpty()) { log.warn("Dry-run allow-list rejection on datasource {} for user {}: tables {}", @@ -142,31 +136,6 @@ private static boolean hasCapability(DatasourceUserPermissionView permission, Qu }; } - private static List normalizeList(List raw) { - if (raw == null || raw.isEmpty()) { - return List.of(); - } - var out = new ArrayList(raw.size()); - for (String entry : raw) { - if (entry == null) { - continue; - } - var stripped = new StringBuilder(entry.length()); - for (int i = 0; i < entry.length(); i++) { - char c = entry.charAt(i); - if (c == '"' || c == '`' || c == '[' || c == ']') { - continue; - } - stripped.append(c); - } - var normalized = stripped.toString().trim().toLowerCase(Locale.ROOT); - if (!normalized.isEmpty()) { - out.add(normalized); - } - } - return List.copyOf(out); - } - private String msg(String key, Object[] args) { return messageSource.getMessage(key, args, LocaleContextHolder.getLocale()); } diff --git a/backend/src/main/java/com/bablsoft/accessflow/proxy/internal/DefaultSampleDataService.java b/backend/src/main/java/com/bablsoft/accessflow/proxy/internal/DefaultSampleDataService.java index 56fb26c5..808dcc1d 100644 --- a/backend/src/main/java/com/bablsoft/accessflow/proxy/internal/DefaultSampleDataService.java +++ b/backend/src/main/java/com/bablsoft/accessflow/proxy/internal/DefaultSampleDataService.java @@ -1,5 +1,6 @@ package com.bablsoft.accessflow.proxy.internal; +import com.bablsoft.accessflow.core.api.AllowedTables; import com.bablsoft.accessflow.core.api.ColumnMaskDirective; import com.bablsoft.accessflow.core.api.DatabaseSchemaView; import com.bablsoft.accessflow.core.api.DatasourceAdminService; @@ -21,12 +22,12 @@ import org.springframework.security.access.AccessDeniedException; import org.springframework.stereotype.Service; -import java.util.ArrayList; import java.util.List; -import java.util.Locale; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.UUID; +import java.util.function.Supplier; @Service @RequiredArgsConstructor @@ -55,7 +56,9 @@ public SelectExecutionResult sample(UUID datasourceId, UUID organizationId, UUID if (!isAdmin) { // Non-admins additionally need read capability + the target inside their allow-list. var view = permission.orElseThrow(() -> new TableNotFoundException(datasourceId, table)); - if (!view.canRead() || !targetAllowed(view, target)) { + if (!view.canRead() || !targetAllowed(view, target, + () -> datasourceAdminService.introspectSchemaForSystem(datasourceId, + organizationId))) { throw new TableNotFoundException(datasourceId, table); } // The preview reads every column, so a denied column on the table refuses it (#935). @@ -123,59 +126,51 @@ private Optional resolveTarget(DatabaseSchemaView view, String schema, S } /** - * Mirrors the allow-list semantics of {@code DefaultQuerySubmissionService.verifyAllowedTables}: - * empty lists allow everything; otherwise the table (bare or {@code schema.table}) must be in - * {@code allowedTables}, or its schema in {@code allowedSchemas}. + * Matches through {@link AllowedTables#coveringEntry}, the query gate's matcher: the qualified + * target is covered by its own {@code schema.table} entry or by its schema. A bare + * {@code allowed_tables} entry covers only an unqualified reference in the gate — whatever table + * the database resolves that name to — which a preview of a concrete {@code schema.table} cannot + * know. It admits the target only when no other schema in the database has a table of that name + * (the fail-closed rule {@code SchemaViewPermissionFilter} applies to the view, #936), counted + * over the unfiltered catalog, fetched only for this fallback, since the caller's filtered view + * may already hide the other table. A target without a schema is covered by a bare entry only. */ - private static boolean targetAllowed(DatasourceUserPermissionView permission, Target target) { - var allowedSchemas = normalizeList(permission.allowedSchemas()); - var allowedTables = normalizeList(permission.allowedTables()); + private static boolean targetAllowed(DatasourceUserPermissionView permission, Target target, + Supplier fullCatalog) { + var allowedSchemas = AllowedTables.normalize(permission.allowedSchemas()); + var allowedTables = AllowedTables.normalize(permission.allowedTables()); if (allowedSchemas.isEmpty() && allowedTables.isEmpty()) { return true; } - var bare = normalize(target.table()); - var qualified = target.schema() == null || target.schema().isBlank() - ? bare - : normalize(target.schema()) + "." + bare; - if (allowedTables.contains(bare) || allowedTables.contains(qualified)) { + var bare = AllowedTables.normalizeEntry(target.table()); + if (bare == null) { + return false; + } + var schema = AllowedTables.normalizeEntry(target.schema()); + if (schema == null) { + return allowedTables.contains(bare); + } + if (AllowedTables.coveringEntry(allowedSchemas, allowedTables, schema + "." + bare) != null) { return true; } - return target.schema() != null && !target.schema().isBlank() - && allowedSchemas.contains(normalize(target.schema())); + return allowedTables.contains(bare) && tablesNamed(fullCatalog.get(), bare) == 1; } - private static List normalizeList(List raw) { - if (raw == null || raw.isEmpty()) { - return List.of(); - } - var out = new ArrayList(raw.size()); - for (String entry : raw) { - if (entry == null) { - continue; - } - var normalized = normalize(entry); - if (!normalized.isEmpty()) { - out.add(normalized); + private static int tablesNamed(DatabaseSchemaView view, String bare) { + var count = 0; + for (var ns : view.schemas()) { + for (var t : ns.tables()) { + if (bare.equals(AllowedTables.normalizeEntry(t.name()))) { + count++; + } } } - return List.copyOf(out); + return count; } private static String qualifiedName(Target target) { - return target.schema() == null || target.schema().isBlank() - ? normalize(target.table()) - : normalize(target.schema()) + "." + normalize(target.table()); - } - - private static String normalize(String raw) { - var stripped = new StringBuilder(raw.length()); - for (int i = 0; i < raw.length(); i++) { - char c = raw.charAt(i); - if (c == '"' || c == '`' || c == '[' || c == ']') { - continue; - } - stripped.append(c); - } - return stripped.toString().trim().toLowerCase(Locale.ROOT); + var table = Objects.requireNonNullElse(AllowedTables.normalizeEntry(target.table()), ""); + var schema = AllowedTables.normalizeEntry(target.schema()); + return schema == null ? table : schema + "." + table; } } diff --git a/backend/src/test/java/com/bablsoft/accessflow/proxy/internal/DefaultQueryDryRunServiceTest.java b/backend/src/test/java/com/bablsoft/accessflow/proxy/internal/DefaultQueryDryRunServiceTest.java index 6a65cbe6..6e39d7bb 100644 --- a/backend/src/test/java/com/bablsoft/accessflow/proxy/internal/DefaultQueryDryRunServiceTest.java +++ b/backend/src/test/java/com/bablsoft/accessflow/proxy/internal/DefaultQueryDryRunServiceTest.java @@ -124,6 +124,40 @@ void nonAdminWithNoPermissionIsDenied() { .isInstanceOf(AccessDeniedException.class); } + @Test + void nonAdminReferencingTablesCoveredBySchemaOrQualifiedEntryPasses() { + when(datasourceAdminService.getForUser(datasourceId, orgId, userId)).thenReturn(view()); + when(queryParser.parse(anyString(), any())) + .thenReturn(parse(QueryType.SELECT, Set.of("sales.orders", "public.users"))); + when(permissionLookupService.findFor(userId, datasourceId)) + .thenReturn(java.util.Optional.of(permission(true, List.of("Sales"), + List.of("\"PUBLIC\".\"USERS\"")))); + when(rowSecurityResolutionService.resolveApplicable(orgId, datasourceId, userId)) + .thenReturn(List.of()); + var expected = QueryDryRunResult.of("postgresql", QueryType.SELECT, 1L, null, null, + Set.of(), Duration.ZERO); + when(queryExecutor.dryRun(any())).thenReturn(expected); + + assertThat(service.dryRun(datasourceId, "SELECT 1", userId, orgId, false)) + .isSameAs(expected); + } + + @Test + void nonAdminWithAllowListAndNoReferencedTablesPasses() { + when(datasourceAdminService.getForUser(datasourceId, orgId, userId)).thenReturn(view()); + when(queryParser.parse(anyString(), any())).thenReturn(parse(QueryType.SELECT, Set.of())); + when(permissionLookupService.findFor(userId, datasourceId)) + .thenReturn(java.util.Optional.of(permission(true, List.of(), List.of("orders")))); + when(rowSecurityResolutionService.resolveApplicable(orgId, datasourceId, userId)) + .thenReturn(List.of()); + var expected = QueryDryRunResult.of("postgresql", QueryType.SELECT, 1L, null, null, + Set.of(), Duration.ZERO); + when(queryExecutor.dryRun(any())).thenReturn(expected); + + assertThat(service.dryRun(datasourceId, "SELECT 1", userId, orgId, false)) + .isSameAs(expected); + } + @Test void nonAdminReferencingDisallowedTableIsDenied() { when(datasourceAdminService.getForUser(datasourceId, orgId, userId)).thenReturn(view()); diff --git a/backend/src/test/java/com/bablsoft/accessflow/proxy/internal/DefaultSampleDataServiceTest.java b/backend/src/test/java/com/bablsoft/accessflow/proxy/internal/DefaultSampleDataServiceTest.java index cfca81e0..591b1914 100644 --- a/backend/src/test/java/com/bablsoft/accessflow/proxy/internal/DefaultSampleDataServiceTest.java +++ b/backend/src/test/java/com/bablsoft/accessflow/proxy/internal/DefaultSampleDataServiceTest.java @@ -284,6 +284,137 @@ void rowLimitPolicyOnTheSampledTableCapsThePreview() { assertThat(captor.getValue().maxRowsOverride()).isEqualTo(7); } + @Test + void bareEntryDoesNotAdmitASchemaQualifiedTargetWhenTheNameIsAmbiguous() { + // Defence in depth: even a view that still lists both tables must not admit either. + stubSchemas(schema("public", "orders"), schema("archive", "orders")); + stubCatalog(schema("public", "orders"), schema("archive", "orders")); + when(permissionLookupService.findFor(userId, datasourceId)) + .thenReturn(Optional.of(permission(true, List.of(), List.of(), List.of("orders")))); + + assertThatThrownBy(() -> service.sample(datasourceId, organizationId, userId, false, + "archive", "orders", 50)) + .isInstanceOf(TableNotFoundException.class); + assertThatThrownBy(() -> service.sample(datasourceId, organizationId, userId, false, + "public", "orders", 50)) + .isInstanceOf(TableNotFoundException.class); + verify(queryExecutor, never()).sampleTable(any()); + } + + @Test + void bareEntryAdmitsTheOnlyTableOfThatName() { + stubSchemas(schema("archive", "orders")); + stubCatalog(schema("archive", "orders"), schema("public", "customers")); + when(permissionLookupService.findFor(userId, datasourceId)) + .thenReturn(Optional.of(permission(true, List.of(), List.of(), List.of("orders")))); + + assertThat(service.sample(datasourceId, organizationId, userId, false, "archive", "orders", + 50)).isSameAs(result); + } + + @Test + void bareEntryCountsTheUnfilteredCatalogNotTheCallersView() { + // The filtered view shows archive.orders through a catalog-qualified grant and hides + // public.orders; the bare entry must still see the name is ambiguous in the database. + stubSchemas(schema("archive", "orders")); + stubCatalog(schema("public", "orders"), schema("archive", "orders")); + when(permissionLookupService.findFor(userId, datasourceId)) + .thenReturn(Optional.of(permission(true, List.of(), List.of(), + List.of("orders", "cat.archive.orders")))); + + assertThatThrownBy(() -> service.sample(datasourceId, organizationId, userId, false, + "archive", "orders", 50)) + .isInstanceOf(TableNotFoundException.class); + verify(queryExecutor, never()).sampleTable(any()); + } + + @Test + void coveredTargetNeverIntrospectsTheUnfilteredCatalog() { + when(permissionLookupService.findFor(userId, datasourceId)) + .thenReturn(Optional.of(permission(true, List.of(), List.of("public"), List.of()))); + + service.sample(datasourceId, organizationId, userId, false, "public", "users", 50); + + verify(datasourceAdminService, never()).introspectSchemaForSystem(any(), any()); + } + + @Test + void qualifiedEntryDoesNotCoverTheSameNameInAnotherSchema() { + stubSchemas(schema("public", "orders"), schema("archive", "orders")); + when(permissionLookupService.findFor(userId, datasourceId)) + .thenReturn(Optional.of(permission(true, List.of(), List.of(), + List.of("public.orders")))); + + assertThatThrownBy(() -> service.sample(datasourceId, organizationId, userId, false, + "archive", "orders", 50)) + .isInstanceOf(TableNotFoundException.class); + assertThat(service.sample(datasourceId, organizationId, userId, false, "public", "orders", + 50)).isSameAs(result); + } + + @Test + void allowedSchemaCoversAnAmbiguousNameInsideIt() { + stubSchemas(schema("public", "orders"), schema("archive", "orders")); + when(permissionLookupService.findFor(userId, datasourceId)) + .thenReturn(Optional.of(permission(true, List.of(), List.of("archive"), List.of()))); + + assertThat(service.sample(datasourceId, organizationId, userId, false, "archive", "orders", + 50)).isSameAs(result); + assertThatThrownBy(() -> service.sample(datasourceId, organizationId, userId, false, + "public", "orders", 50)) + .isInstanceOf(TableNotFoundException.class); + } + + @Test + void quotedMixedCaseEntriesAreNormalizedLikeTheQueryGate() { + when(permissionLookupService.findFor(userId, datasourceId)) + .thenReturn(Optional.of(permission(true, List.of(), List.of(), + List.of(" \"PUBLIC\".[Users] ", "")))); + + assertThat(service.sample(datasourceId, organizationId, userId, false, "public", "users", + 50)).isSameAs(result); + } + + @Test + void unnamedSchemaTargetIsCoveredOnlyByABareEntry() { + stubSchemas(schema("", "orders")); + when(permissionLookupService.findFor(userId, datasourceId)) + .thenReturn(Optional.of(permission(true, List.of(), List.of(), List.of("orders")))); + + assertThat(service.sample(datasourceId, organizationId, userId, false, null, "orders", 50)) + .isSameAs(result); + var captor = ArgumentCaptor.forClass(SampleTableRequest.class); + verify(queryExecutor).sampleTable(captor.capture()); + assertThat(captor.getValue().table()).isEqualTo("orders"); + } + + @Test + void unnamedSchemaTargetIsRefusedWithOnlyAQualifiedEntry() { + stubSchemas(schema("", "orders")); + when(permissionLookupService.findFor(userId, datasourceId)) + .thenReturn(Optional.of(permission(true, List.of(), List.of("public"), + List.of("public.orders")))); + + assertThatThrownBy(() -> service.sample(datasourceId, organizationId, userId, false, null, + "orders", 50)) + .isInstanceOf(TableNotFoundException.class); + } + + private void stubCatalog(DatabaseSchemaView.Schema... schemas) { + when(datasourceAdminService.introspectSchemaForSystem(datasourceId, organizationId)) + .thenReturn(new DatabaseSchemaView(List.of(schemas))); + } + + private void stubSchemas(DatabaseSchemaView.Schema... schemas) { + when(datasourceAdminService.introspectSchema(eq(datasourceId), eq(organizationId), + eq(userId), anyBoolean())).thenReturn(new DatabaseSchemaView(List.of(schemas))); + } + + private static DatabaseSchemaView.Schema schema(String name, String table) { + return new DatabaseSchemaView.Schema(name, List.of(new DatabaseSchemaView.Table(table, + List.of(new DatabaseSchemaView.Column("id", "uuid", false, true)), List.of()))); + } + private DatasourceUserPermissionView permission(boolean canRead, List restrictedColumns, List allowedSchemas, List allowedTables) { diff --git a/docs/05-backend.md b/docs/05-backend.md index bff6afec..244edfff 100644 --- a/docs/05-backend.md +++ b/docs/05-backend.md @@ -1027,7 +1027,7 @@ The result is returned via `DatabaseSchemaView` (immutable nested records: `Sche `proxy.api.SampleDataService` returns a bounded, fully-governed sample of a single table's rows for the schema-explorer UI — an **ad-hoc read that bypasses review but not governance**. It does *not* create a `query_request`; it resolves the caller's directives and runs through the executor exactly like `DefaultQueryLifecycleService.doExecute`: -1. **Authorization + allow-list.** `DefaultSampleDataService` calls `DatasourceAdminService.introspectSchema(...)` (which enforces org + permission-row access) and validates the requested `schema`/`table` against the returned `DatabaseSchemaView`. Non-ADMINs additionally need `can_read` and the target inside their `allowed_schemas`/`allowed_tables` (same normalization as `DefaultQuerySubmissionService.verifyAllowedTables`). A miss raises `TableNotFoundException` (HTTP 404) — existence is never leaked. +1. **Authorization + allow-list.** `DefaultSampleDataService` calls `DatasourceAdminService.introspectSchema(...)` (which enforces org + permission-row access) and validates the requested `schema`/`table` against the returned `DatabaseSchemaView`. Non-ADMINs additionally need `can_read` and the target inside their `allowed_schemas`/`allowed_tables`, matched by `core.api.AllowedTables` (`normalize` + `coveringEntry`) — the query gate's matcher (`DatasourcePermissionChecker.rejectedTables`), so a `schema.table` or `allowed_schemas` grant covers the preview exactly as it covers a `SELECT` (#1089). A **bare** `allowed_tables` entry is the one place the two differ: the gate applies it to an *unqualified* reference, which the database resolves, while the preview reads a concrete `schema.table`. The preview admits it only when no other schema in the database has a table of that name — counted over the unfiltered catalog (`introspectSchemaForSystem`, fetched only for this fallback), since the caller's #936-filtered view may already hide the twin — and otherwise fails closed, matching `core.internal.SchemaViewPermissionFilter`. So `allowed_tables=[orders]` with both `public.orders` and `archive.orders` previews neither until the admin qualifies the entry, and a target whose schema reports no name is covered by a bare entry only. The fallback is deliberately looser than the gate in one case: with `orders` only in `archive` and `archive` off the search path, the preview reads `archive.orders` while an unqualified `SELECT * FROM orders` fails to resolve — the same table the schema tree already shows. The view's catalog-suffix rule (`mydb.dbo.orders` showing `dbo.orders`) is not honoured here, so such a table is listed but its preview returns 404. A miss raises `TableNotFoundException` (HTTP 404) — existence is never leaked. 2. **Directive resolution.** Restricted columns (from the permission), `ColumnMaskDirective`s (`MaskingPolicyResolutionService`), and `RowSecurityDirective`s (`RowSecurityResolutionService`) are resolved for the caller. 3. **Execution.** `QueryExecutor.sampleTable(SampleTableRequest)` enforces the row cap (`maxRowsOverride` — the requested limit, lowered to the caller's effective `row_limit_override` when one applies (#933) and to any row-limit policy on the sampled table (#934) — clamped to the datasource + global `ACCESSFLOW_PROXY_EXECUTION_MAX_ROWS`) and statement timeout, then: - **Relational** datasources: builds `SELECT * FROM ` (via `IdentifierQuoter`, never raw input) and runs the existing JDBC path — `RowSecurityRewriter` injects RLS, `JdbcResultRowMapper` + `ColumnMasker` mask post-fetch, JDBC `setMaxRows` caps without a dialect-specific `LIMIT`. @@ -1039,7 +1039,7 @@ The result is a `SelectExecutionResult` mapped to `SampleRowsResponse` for `GET `proxy.api.QueryDryRunService` returns a **non-committing execution plan + best-effort estimated row impact** for a query — the playground/sandbox a user reaches for before formal submission (`POST /api/v1/queries/dry-run`). Like the sample path it is an **ad-hoc read that bypasses review but not governance**, creates no `query_request`, and never mutates data — every engine plans the statement (relational `EXPLAIN`, Mongo `explain`, …) but never executes it. -1. **Authorization + allow-list.** `DefaultQueryDryRunService` resolves the datasource via `DatasourceAdminService.getForUser`/`getForAdmin` (org + permission-row access; 404 on miss), parses the query through `QueryParser` (`InvalidSqlException` → 422) for the `QueryType` + `referencedTables`, and — for non-ADMINs — verifies the matching capability (`can_read`/`can_write`/`can_ddl`) and that every referenced table is inside the caller's allow-list (same normalization as `DefaultQuerySubmissionService.verifyAllowedTables`; a miss raises Spring Security `AccessDeniedException` → 403). +1. **Authorization + allow-list.** `DefaultQueryDryRunService` resolves the datasource via `DatasourceAdminService.getForUser`/`getForAdmin` (org + permission-row access; 404 on miss), parses the query through `QueryParser` (`InvalidSqlException` → 422) for the `QueryType` + `referencedTables`, and — for non-ADMINs — verifies the matching capability (`can_read`/`can_write`/`can_ddl`) and that every referenced table is inside the caller's allow-list (`core.api.AllowedTables.coveringEntry`, the query gate's matcher; a miss raises Spring Security `AccessDeniedException` → 403). 2. **Directive resolution.** The caller's `RowSecurityDirective`s (`RowSecurityResolutionService`) are resolved so the plan reflects the **governed** query. Column masks are irrelevant to a plan (no rows are returned) and are omitted. 3. **Planning.** `QueryExecutor.dryRun(QueryExecutionRequest)` applies the `RowSecurityRewriter`, acquires a connection via `RoutingDataSourceResolver` (SELECT dry-runs prefer the read replica; writes plan on the primary — e.g. Oracle writes its scratch `PLAN_TABLE` there), and: - **Relational** datasources: a per-`DbType` `DryRunPlanner` (`proxy/internal/dryrun/`) runs the dialect's non-executing EXPLAIN — PostgreSQL `EXPLAIN (FORMAT JSON)`, MySQL/MariaDB `EXPLAIN FORMAT=JSON`, Oracle `EXPLAIN PLAN FOR` + `PLAN_TABLE` (rows deleted in a `finally`), SQL Server `SET SHOWPLAN_ALL ON` — and maps it to a `QueryPlanNode` tree. `CUSTOM` JDBC has no planner and degrades gracefully.