From a73e6bfab93ab6989245c4bb5301b47513d96369 Mon Sep 17 00:00:00 2001 From: Tigran Babloyan Date: Thu, 24 Sep 2026 19:45:23 +0400 Subject: [PATCH] fix: enforce the table allow-list on request-group query members DefaultRequestGroupService.validatePermission checked a QUERY member's capability and denied columns but never allowed_schemas/allowed_tables, so a group member could reference a table outside the submitter's allow-list. Check it through core.api.AllowedTables with DatasourcePermissionChecker.rejectedTables semantics, on the break-glass path too (standalone break-glass enforces it). --- .../internal/DefaultRequestGroupService.java | 48 ++++++-- .../DefaultRequestGroupServiceCrudTest.java | 107 ++++++++++++++++++ docs/05-backend.md | 10 ++ docs/07-security.md | 2 + 4 files changed, 156 insertions(+), 11 deletions(-) diff --git a/backend/src/main/java/com/bablsoft/accessflow/requestgroups/internal/DefaultRequestGroupService.java b/backend/src/main/java/com/bablsoft/accessflow/requestgroups/internal/DefaultRequestGroupService.java index 4f823687..0b6c6391 100644 --- a/backend/src/main/java/com/bablsoft/accessflow/requestgroups/internal/DefaultRequestGroupService.java +++ b/backend/src/main/java/com/bablsoft/accessflow/requestgroups/internal/DefaultRequestGroupService.java @@ -8,6 +8,7 @@ import com.bablsoft.accessflow.audit.api.AuditLogService; import com.bablsoft.accessflow.audit.api.AuditResourceType; import com.bablsoft.accessflow.core.api.AiAnalysisLookupService; +import com.bablsoft.accessflow.core.api.AllowedTables; import com.bablsoft.accessflow.core.api.DatasourceLookupService; import com.bablsoft.accessflow.core.api.DatasourceRef; import com.bablsoft.accessflow.core.api.DatasourceUserPermissionLookupService; @@ -58,6 +59,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.TreeSet; import java.util.UUID; @Service @@ -302,17 +304,41 @@ private SqlParseResult parseQuery(UUID datasourceId, String sql) { return queryParser.parse(sql, dbType); } - /** A member may not reach a column its submitter is denied (#935), break-glass included. */ - private void verifyDeniedColumns(RequestGroupItemEntity item, - DatasourceUserPermissionView permission) { - if (DeniedColumns.normalize(permission.deniedColumns()).isEmpty()) { + /** + * A member may not reach a table outside its submitter's allow-list, nor a column they are + * denied (#935) — break-glass included, as for a standalone break-glass query. The allow-list + * rule is {@code DatasourcePermissionChecker.rejectedTables}: both lists empty means no + * restriction, and a bare entry covers only an unqualified reference. + */ + private void verifyTableAndColumnScope(RequestGroupItemEntity item, + DatasourceUserPermissionView permission) { + var allowedSchemas = AllowedTables.normalize(permission.allowedSchemas()); + var allowedTables = AllowedTables.normalize(permission.allowedTables()); + var restrictsTables = !allowedSchemas.isEmpty() || !allowedTables.isEmpty(); + var deniesColumns = !DeniedColumns.normalize(permission.deniedColumns()).isEmpty(); + if (!restrictsTables && !deniesColumns) { return; } - var rejected = DeniedColumns.rejected(permission.deniedColumns(), - parseQuery(item.getDatasourceId(), item.getSqlText())); - if (!rejected.isEmpty()) { - throw new RequestGroupPermissionException( - "Denied columns referenced: " + String.join(", ", rejected)); + var parsed = parseQuery(item.getDatasourceId(), item.getSqlText()); + if (restrictsTables) { + var rejectedTables = new TreeSet(); + for (String table : parsed.referencedTables()) { + if (AllowedTables.coveringEntry(allowedSchemas, allowedTables, table) == null) { + rejectedTables.add(table); + } + } + if (!rejectedTables.isEmpty()) { + throw new RequestGroupPermissionException( + "Tables outside the allow-list referenced: " + + String.join(", ", rejectedTables)); + } + } + if (deniesColumns) { + var rejected = DeniedColumns.rejected(permission.deniedColumns(), parsed); + if (!rejected.isEmpty()) { + throw new RequestGroupPermissionException( + "Denied columns referenced: " + String.join(", ", rejected)); + } } } @@ -325,7 +351,7 @@ private void validatePermission(RequestGroupItemEntity item, UUID submitterId, b throw new RequestGroupPermissionException( "Break-glass requires can_break_glass on every member target"); } - verifyDeniedColumns(item, perm.get()); + verifyTableAndColumnScope(item, perm.get()); return; } if (admin) { @@ -338,7 +364,7 @@ private void validatePermission(RequestGroupItemEntity item, UUID submitterId, b throw new RequestGroupPermissionException( "You are not permitted to run this query on the selected datasource"); } - verifyDeniedColumns(item, perm.get()); + verifyTableAndColumnScope(item, perm.get()); } else { var perm = apiConnectorPermissionLookupService.findFor(item.getApiConnectorId(), submitterId); if (breakGlass) { diff --git a/backend/src/test/java/com/bablsoft/accessflow/requestgroups/internal/DefaultRequestGroupServiceCrudTest.java b/backend/src/test/java/com/bablsoft/accessflow/requestgroups/internal/DefaultRequestGroupServiceCrudTest.java index 5c520f83..c312b942 100644 --- a/backend/src/test/java/com/bablsoft/accessflow/requestgroups/internal/DefaultRequestGroupServiceCrudTest.java +++ b/backend/src/test/java/com/bablsoft/accessflow/requestgroups/internal/DefaultRequestGroupServiceCrudTest.java @@ -273,6 +273,113 @@ private void stubDeniedColumnParse() { java.util.Set.of("customer"), "ssn")), true)); } + @Test + void submitRejectsAMemberThatReferencesATableOutsideTheAllowList() { + var group = stubAllowListSubmit(List.of(), List.of("public.customers"), false, + "public.orders"); + + assertThatThrownBy(() -> submit(group, false)) + .isInstanceOf(RequestGroupPermissionException.class) + .hasMessageContaining("public.orders"); + verify(stateService, org.mockito.Mockito.never()).apply(any(), any()); + } + + @Test + void submitAcceptsAMemberWhoseTablesAreCoveredByTableOrSchemaEntries() { + var group = stubAllowListSubmit(List.of("sales"), List.of("public.customers"), false, + "public.customers", "sales.orders"); + + submit(group, false); + + verify(stateService).apply(group, RequestGroupStatus.PENDING_AI); + } + + @Test + void aBareAllowListEntryCoversOnlyAnUnqualifiedReference() { + var covered = stubAllowListSubmit(List.of(), List.of("orders"), false, "orders"); + submit(covered, false); + verify(stateService).apply(covered, RequestGroupStatus.PENDING_AI); + + var qualified = stubAllowListSubmit(List.of(), List.of("orders"), false, "public.orders"); + assertThatThrownBy(() -> submit(qualified, false)) + .isInstanceOf(RequestGroupPermissionException.class); + } + + @Test + void emptyAllowListsAreUnrestrictedAndSkipTheParse() { + var group = draftGroup(); + when(groupRepository.findByIdAndOrganizationId(group.getId(), orgId)).thenReturn(Optional.of(group)); + when(itemRepository.findByGroupIdOrderBySequenceOrderAsc(group.getId())) + .thenReturn(List.of(allowListItem())); + when(datasourcePermissionLookupService.findFor(userId, datasourceId)) + .thenReturn(Optional.of(dsPerm(true, false, false))); + + submit(group, false); + + verify(queryParser, org.mockito.Mockito.never()).parse(any(), any()); + verify(stateService).apply(group, RequestGroupStatus.PENDING_AI); + } + + @Test + void breakGlassSubmitAlsoRejectsATableOutsideTheAllowList() { + var group = stubAllowListSubmit(List.of("sales"), List.of(), true, "hr.salaries"); + + assertThatThrownBy(() -> submit(group, true)) + .isInstanceOf(RequestGroupPermissionException.class) + .hasMessageContaining("hr.salaries"); + verify(executionService, org.mockito.Mockito.never()).execute(any(), any(), any()); + } + + @Test + void adminSubmitIsNotBoundByTheAllowList() { + var group = draftGroup(); + when(groupRepository.findByIdAndOrganizationId(group.getId(), orgId)).thenReturn(Optional.of(group)); + when(itemRepository.findByGroupIdOrderBySequenceOrderAsc(group.getId())) + .thenReturn(List.of(allowListItem())); + when(datasourcePermissionLookupService.findFor(userId, datasourceId)) + .thenReturn(Optional.of(allowListPerm(List.of(), List.of("public.customers"), false))); + + service.submit(new SubmitRequestGroupCommand(group.getId(), orgId, userId, true, false, null, + "1.2.3.4", "ua")); + + verify(queryParser, org.mockito.Mockito.never()).parse(any(), any()); + verify(stateService).apply(group, RequestGroupStatus.PENDING_AI); + } + + private RequestGroupEntity stubAllowListSubmit(List schemas, List tables, + boolean breakGlass, String... referenced) { + var group = draftGroup(); + when(groupRepository.findByIdAndOrganizationId(group.getId(), orgId)).thenReturn(Optional.of(group)); + when(itemRepository.findByGroupIdOrderBySequenceOrderAsc(group.getId())) + .thenReturn(List.of(allowListItem())); + when(datasourcePermissionLookupService.findFor(userId, datasourceId)) + .thenReturn(Optional.of(allowListPerm(schemas, tables, breakGlass))); + lenient().when(datasourceLookupService.findById(datasourceId)).thenReturn(Optional.empty()); + when(queryParser.parse(any(), any())).thenReturn(new SqlParseResult(QueryType.SELECT, false, + List.of("SELECT 1"), java.util.Set.of(referenced))); + return group; + } + + private void submit(RequestGroupEntity group, boolean breakGlass) { + service.submit(new SubmitRequestGroupCommand(group.getId(), orgId, userId, false, breakGlass, + null, "1.2.3.4", "ua")); + } + + private RequestGroupItemEntity allowListItem() { + var item = new RequestGroupItemEntity(); + item.setTargetKind(com.bablsoft.accessflow.requestgroups.api.RequestGroupTargetKind.QUERY); + item.setDatasourceId(datasourceId); + item.setQueryType(QueryType.SELECT); + item.setSqlText("SELECT 1"); + return item; + } + + private DatasourceUserPermissionView allowListPerm(List schemas, List tables, + boolean breakGlass) { + return new DatasourceUserPermissionView(UUID.randomUUID(), userId, datasourceId, true, false, + false, breakGlass, schemas, tables, List.of(), List.of(), null, null); + } + @Test void submitRecordsSqlReviewFindingsForEveryQueryMemberAndSkipsApiMembers() { var group = draftGroup(); diff --git a/docs/05-backend.md b/docs/05-backend.md index 44c89430..fb4ffd32 100644 --- a/docs/05-backend.md +++ b/docs/05-backend.md @@ -4612,6 +4612,16 @@ rows) and publishes `RequestGroupItemExecutedEvent`; the group publishes permission for its target — `core.api.DatasourceUserPermissionLookupService` for query members, a new `apigov.api` connector-permission lookup for API members. A **break-glass group** (`submission_reason = EMERGENCY_ACCESS`) requires `can_break_glass` on **every** member target. +A `QUERY` member is also held to the permission's table scope, exactly as a standalone query is: +every referenced table must be covered by `allowed_schemas` / `allowed_tables` (matched through +`core.api.AllowedTables`, with `DatasourcePermissionChecker.rejectedTables` semantics — both lists +empty means unrestricted, a schema entry covers any table qualified with it, and a bare table entry +covers only an unqualified reference), and no denied column may be referenced (#935). Both checks +bind break-glass groups too — standalone break-glass enforces the allow-list, and break-glass waives +approval, never data-protection controls — and neither binds `QUERY_ADMIN` holders, who skip the +per-datasource gate on this path as on standard submission. A miss is `RequestGroupPermissionException` +(403) and the group stays `DRAFT`. The query is parsed only when the permission carries an allow-list +or a deny list. **Audit & realtime.** New `AuditResourceType.REQUEST_GROUP` and `AuditAction.REQUEST_GROUP_*` values record the group lifecycle alongside each member's own query/API audit row. New diff --git a/docs/07-security.md b/docs/07-security.md index fc95da7a..0dd91a87 100644 --- a/docs/07-security.md +++ b/docs/07-security.md @@ -744,6 +744,8 @@ Are allowed_schemas / allowed_tables set? intersect with allow-list; reject (403, `error.permission.table_not_allowed`) on any miss. Unqualified references match `allowed_tables` only when the bare name is listed — a schemas-only allow-list does NOT cover them. + The same check binds every `QUERY` member of a request group, break-glass groups + included (`DefaultRequestGroupService.validatePermission`). Violation → 403 ↓ Are denied_columns set? (#935, relational engines only)