This is an automated email from the ASF dual-hosted git repository. github-actions[bot] pushed a commit to branch cherry-pick-b3593b86-to-branch-1.3 in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit ba4438276be821fea51bcdd6bcc1af235ca74773 Author: Qi Yu <[email protected]> AuthorDate: Wed Sep 2 16:43:46 2026 +0800 [#12837] fix(authz): Evaluate list denies by access path (#12838) ### What changes were proposed in this pull request? - Model list-authorization short-circuits as independent parent-scope access paths. - Check deny policies only for the privilege used by each access path. - Support both table filter and table-like list authorization expressions. - Preserve the `USE_CATALOG` and `USE_SCHEMA` requirements for table-like listing. ### Why are the changes needed? The existing short-circuit checks `SELECT_TABLE` and `MODIFY_TABLE` denies together. A deny on one privilege therefore disables an independent, deny-free access path and falls back to per-object authorization. For large schemas, this causes thousands of unnecessary authorization checks even when all tables are visible through another parent-scope privilege. Fix: #12837 ### Does this PR introduce _any_ user-facing change? No API or authorization-result changes. Eligible list operations avoid unnecessary per-object authorization checks. ### How was this patch tested? - `./gradlew :server-common:spotlessApply` - `./gradlew :server-common:test --tests org.apache.gravitino.server.authorization.TestMetadataAuthzHelper -PskipITs -PskipDockerTests=false` - `./gradlew :server-common:check -PskipITs -PskipDockerTests=false` - `git diff --check` # Conflicts: # server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java --- .../server/authorization/MetadataAuthzHelper.java | 145 +++++++++++++---- .../authorization/TestMetadataAuthzHelper.java | 176 +++++++++++++++++++++ 2 files changed, 292 insertions(+), 29 deletions(-) diff --git a/server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java b/server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java index 65a149ce16..6dd5159feb 100644 --- a/server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java +++ b/server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java @@ -89,45 +89,77 @@ public class MetadataAuthzHelper { private static final List<Entity.EntityType> REQUIRE_SCHEMA_EXISTS = Arrays.asList(Entity.EntityType.TABLE, Entity.EntityType.TOPIC); + private static final String TABLE_PARENT_SCOPES = "METALAKE, CATALOG, SCHEMA"; + private static final String SCHEMA_PARENT_SCOPES = "METALAKE, CATALOG"; + private static final String CATALOG_PARENT_SCOPES = "METALAKE"; + /** * Registry of list-authorization short-circuits keyed by the listed object's entity type. Each - * entry pairs the per-object filter expression it applies to with the parent-scope expression to - * evaluate once and the privileges whose object-level denies would defeat the short-circuit. + * entry pairs a per-object filter expression with the alternative parent-scope access paths that + * can make every listed object visible. Each path tracks only the deny privileges that can + * invalidate that path, so a deny on one path does not disable an independent path. */ - private static final Map<Entity.EntityType, ListShortCircuit> LIST_SHORT_CIRCUITS = - Map.of( - Entity.EntityType.TABLE, - new ListShortCircuit( - AuthorizationExpressionConstants.FILTER_TABLE_AUTHORIZATION_EXPRESSION, - AuthorizationExpressionConstants.TABLE_LIST_PARENT_SCOPE_AUTHORIZATION_EXPRESSION, - Set.of(Privilege.Name.SELECT_TABLE, Privilege.Name.MODIFY_TABLE)), - Entity.EntityType.SCHEMA, - new ListShortCircuit( - AuthorizationExpressionConstants.FILTER_SCHEMA_AUTHORIZATION_EXPRESSION, - AuthorizationExpressionConstants.SCHEMA_LIST_PARENT_SCOPE_AUTHORIZATION_EXPRESSION, - Set.of(Privilege.Name.USE_SCHEMA)), - Entity.EntityType.CATALOG, - new ListShortCircuit( - AuthorizationExpressionConstants.LOAD_CATALOG_AUTHORIZATION_EXPRESSION, - AuthorizationExpressionConstants.CATALOG_LIST_PARENT_SCOPE_AUTHORIZATION_EXPRESSION, - Set.of(Privilege.Name.USE_CATALOG))); - - /** Immutable description of a single list-authorization short-circuit. */ - private static final class ListShortCircuit { - private final String filterExpression; - private final String parentScopeExpression; + private static final Map<Entity.EntityType, Map<String, List<ParentScopeAccessPath>>> + LIST_SHORT_CIRCUITS = + Map.of( + Entity.EntityType.TABLE, + Map.of( + AuthorizationExpressionConstants.FILTER_TABLE_AUTHORIZATION_EXPRESSION, + List.of( + parentOwnerPath(TABLE_PARENT_SCOPES), + parentPrivilegePath(Privilege.Name.SELECT_TABLE, TABLE_PARENT_SCOPES), + parentPrivilegePath(Privilege.Name.MODIFY_TABLE, TABLE_PARENT_SCOPES)), + AuthorizationExpressionConstants.LIST_TABLE_LIKE_AUTHORIZATION_EXPRESSION, + List.of( + parentOwnerPath(TABLE_PARENT_SCOPES), + tableLikeParentPrivilegePath(Privilege.Name.PROBE_TABLE_LIKE), + tableLikeParentPrivilegePath(Privilege.Name.SELECT_TABLE), + tableLikeParentPrivilegePath(Privilege.Name.MODIFY_TABLE), + tableLikeParentPrivilegePath(Privilege.Name.CREATE_TABLE), + tableLikeParentPrivilegePath(Privilege.Name.CREATE_VIEW))), + Entity.EntityType.SCHEMA, + Map.of( + AuthorizationExpressionConstants.FILTER_SCHEMA_AUTHORIZATION_EXPRESSION, + List.of( + parentOwnerPath(SCHEMA_PARENT_SCOPES), + parentPrivilegePath(Privilege.Name.USE_SCHEMA, SCHEMA_PARENT_SCOPES))), + Entity.EntityType.CATALOG, + Map.of( + AuthorizationExpressionConstants.LOAD_CATALOG_AUTHORIZATION_EXPRESSION, + List.of( + parentOwnerPath(CATALOG_PARENT_SCOPES), + parentPrivilegePath(Privilege.Name.USE_CATALOG, CATALOG_PARENT_SCOPES)))); + + /** A sufficient parent-scope access path and the deny privileges that can invalidate it. */ + private static final class ParentScopeAccessPath { + private final String expression; private final Set<Privilege.Name> denyPrivileges; - private ListShortCircuit( - String filterExpression, String parentScopeExpression, Set<Privilege.Name> denyPrivileges) { - this.filterExpression = filterExpression; - this.parentScopeExpression = parentScopeExpression; + private ParentScopeAccessPath(String expression, Set<Privilege.Name> denyPrivileges) { + this.expression = expression; this.denyPrivileges = denyPrivileges; } } private MetadataAuthzHelper() {} + private static ParentScopeAccessPath parentOwnerPath(String parentScopes) { + return new ParentScopeAccessPath("ANY(OWNER, " + parentScopes + ")", Set.of()); + } + + private static ParentScopeAccessPath parentPrivilegePath( + Privilege.Name privilege, String parentScopes) { + return new ParentScopeAccessPath( + String.format("ANY(%s, %s)", privilege.name(), parentScopes), Set.of(privilege)); + } + + private static ParentScopeAccessPath tableLikeParentPrivilegePath(Privilege.Name privilege) { + ParentScopeAccessPath privilegePath = parentPrivilegePath(privilege, TABLE_PARENT_SCOPES); + return new ParentScopeAccessPath( + "ANY_USE_CATALOG && ANY_USE_SCHEMA && (" + privilegePath.expression + ")", + privilegePath.denyPrivileges); + } + public static Metalake[] filterMetalakes(Metalake[] metalakes, String expression) { AuthorizationRequestContext authorizationRequestContext = new AuthorizationRequestContext(); return doFilter( @@ -220,8 +252,27 @@ public class MetadataAuthzHelper { String expression, Entity.EntityType entityType, NameIdentifier[] nameIdentifiers) { +<<<<<<< HEAD ListShortCircuit spec = LIST_SHORT_CIRCUITS.get(entityType); if (spec == null || !spec.filterExpression.equals(expression)) { +======= + Principal principal = PrincipalUtils.getCurrentPrincipal(); + Map<String, List<ParentScopeAccessPath>> entityShortCircuits = + LIST_SHORT_CIRCUITS.get(entityType); + List<ParentScopeAccessPath> accessPaths = + entityShortCircuits == null ? null : entityShortCircuits.get(expression); + if (accessPaths == null) { + LOG.debug( + "Parent-scope short-circuit unavailable for principal {}, entity type {} under metalake " + + "{}: {}.", + principal.getName(), + entityType, + metalake, + entityShortCircuits == null + ? "no short-circuit spec is registered for this entity type" + : "the requested filter expression does not match a registered short-circuit " + + "expression"); +>>>>>>> b3593b869 ([#12837] fix(authz): Evaluate list denies by access path (#12838)) return false; } @@ -237,10 +288,10 @@ public class MetadataAuthzHelper { GravitinoAuthorizer authorizer = GravitinoAuthorizerProvider.getInstance().getGravitinoAuthorizer(); AuthorizationRequestContext requestContext = new AuthorizationRequestContext(); - requestContext.setOriginalAuthorizationExpression(spec.parentScopeExpression); Map<Entity.EntityType, NameIdentifier> metadataNames = NameIdentifierUtil.splitNameIdentifier(metalake, entityType, nameIdentifiers[0]); +<<<<<<< HEAD boolean parentGrantsAccess = new AuthorizationExpressionEvaluator(spec.parentScopeExpression, authorizer) .evaluate(metadataNames, requestContext, principal, Optional.empty()); @@ -252,6 +303,42 @@ public class MetadataAuthzHelper { // deny on these privileges (at the parent scope or on an individual object), so the // short-circuit is only safe when no such deny may exist. return !authorizer.hasDenyPolicy(principal, metalake, spec.denyPrivileges, requestContext); +======= + for (ParentScopeAccessPath accessPath : accessPaths) { + requestContext.setOriginalAuthorizationExpression(accessPath.expression); + boolean parentGrantsAccess = + new AuthorizationExpressionEvaluator(accessPath.expression, authorizer) + .evaluate(metadataNames, requestContext, principal, Optional.empty()); + if (!parentGrantsAccess) { + continue; + } + + boolean hasDeny = + !accessPath.denyPrivileges.isEmpty() + && authorizer.hasDenyPolicy( + principal, metalake, accessPath.denyPrivileges, requestContext); + if (!hasDeny) { + return true; + } + + LOG.debug( + "Parent-scope access path {} disabled for entity type {} under metalake {}: principal " + + "{} holds a deny policy on {}.", + accessPath.expression, + entityType, + metalake, + principal.getName(), + accessPath.denyPrivileges); + } + + LOG.debug( + "Parent-scope short-circuit skipped for entity type {} under metalake {}: principal {} " + + "has no deny-free parent access path, so per-object authorization is required.", + entityType, + metalake, + principal.getName()); + return false; +>>>>>>> b3593b869 ([#12837] fix(authz): Evaluate list denies by access path (#12838)) } /** diff --git a/server-common/src/test/java/org/apache/gravitino/server/authorization/TestMetadataAuthzHelper.java b/server-common/src/test/java/org/apache/gravitino/server/authorization/TestMetadataAuthzHelper.java index 15783d818a..024443f568 100644 --- a/server-common/src/test/java/org/apache/gravitino/server/authorization/TestMetadataAuthzHelper.java +++ b/server-common/src/test/java/org/apache/gravitino/server/authorization/TestMetadataAuthzHelper.java @@ -29,6 +29,7 @@ import static org.mockito.Mockito.when; import java.lang.reflect.Field; import java.util.Arrays; +import java.util.Set; import java.util.concurrent.Executor; import org.apache.gravitino.Config; import org.apache.gravitino.Configs; @@ -47,6 +48,8 @@ import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; import org.mockito.MockedStatic; /** Test of {@link MetadataAuthzHelper} */ @@ -233,6 +236,61 @@ public class TestMetadataAuthzHelper { return authorizer; } + /** + * Builds an authorizer with table privileges granted at the schema scope. Optional use grants + * satisfy the catalog and schema gates in the table-like list expression, while deny discovery + * reports a deny only when the short-circuit asks about one of {@code deniedPrivileges}. + */ + private GravitinoAuthorizer mockTableListRouteAuthorizer( + Set<Privilege.Name> grantedPrivileges, + Set<Privilege.Name> deniedPrivileges, + boolean grantUsePrivileges) { + GravitinoAuthorizer authorizer = mock(GravitinoAuthorizer.class); + lenient() + .when(authorizer.authorize(any(), eq("testMetalake"), any(), any(), any())) + .thenAnswer( + invocation -> { + MetadataObject object = invocation.getArgument(2); + Privilege.Name privilege = invocation.getArgument(3); + if (object.type() == MetadataObject.Type.CATALOG) { + return grantUsePrivileges && privilege == Privilege.Name.USE_CATALOG; + } + if (object.type() == MetadataObject.Type.SCHEMA) { + return grantedPrivileges.contains(privilege) + || grantUsePrivileges && privilege == Privilege.Name.USE_SCHEMA; + } + return false; + }); + lenient() + .when(authorizer.deny(any(), eq("testMetalake"), any(), any(), any())) + .thenReturn(false); + lenient().when(authorizer.isOwner(any(), eq("testMetalake"), any(), any())).thenReturn(false); + lenient() + .when(authorizer.hasDenyPolicy(any(), eq("testMetalake"), anySet(), any())) + .thenAnswer( + invocation -> { + Set<Privilege.Name> privileges = invocation.getArgument(2); + return privileges.stream().anyMatch(deniedPrivileges::contains); + }); + return authorizer; + } + + private void withAuthorizer(GravitinoAuthorizer authorizer, Runnable assertions) { + makeCompletableFutureUseCurrentThread(); + try (MockedStatic<PrincipalUtils> principalUtilsMocked = mockStatic(PrincipalUtils.class); + MockedStatic<GravitinoAuthorizerProvider> mockStatic = + mockStatic(GravitinoAuthorizerProvider.class)) { + principalUtilsMocked + .when(PrincipalUtils::getCurrentPrincipal) + .thenReturn(new UserPrincipal("tester")); + principalUtilsMocked.when(() -> PrincipalUtils.doAs(any(), any())).thenCallRealMethod(); + GravitinoAuthorizerProvider mockedProvider = mock(GravitinoAuthorizerProvider.class); + mockStatic.when(GravitinoAuthorizerProvider::getInstance).thenReturn(mockedProvider); + when(mockedProvider.getGravitinoAuthorizer()).thenReturn(authorizer); + assertions.run(); + } + } + /** * Builds an authorizer that grants {@code grantPrivilege} at a single ancestor scope ({@code * grantType}) and reports no object-level deny, so a list of children under that ancestor is @@ -559,6 +617,124 @@ public class TestMetadataAuthzHelper { } } + @ParameterizedTest + @EnumSource( + value = Privilege.Name.class, + names = {"PROBE_TABLE_LIKE", "SELECT_TABLE", "MODIFY_TABLE", "CREATE_TABLE", "CREATE_VIEW"}) + public void testListTableLikeShortCircuitSupportsEveryParentPrivilege(Privilege.Name privilege) { + GravitinoAuthorizer authorizer = + mockTableListRouteAuthorizer(Set.of(privilege), Set.of(), true); + withAuthorizer( + authorizer, + () -> { + NameIdentifier[] tables = threeTables(); + NameIdentifier[] filtered = filterTableLike(tables); + + Assertions.assertSame(tables, filtered); + verify(authorizer, times(1)) + .hasDenyPolicy(any(), eq("testMetalake"), eq(Set.of(privilege)), any()); + }); + } + + @Test + public void testListTableLikeShortCircuitIgnoresDenyOnIndependentPath() { + GravitinoAuthorizer authorizer = + mockTableListRouteAuthorizer( + Set.of(Privilege.Name.SELECT_TABLE, Privilege.Name.MODIFY_TABLE), + Set.of(Privilege.Name.MODIFY_TABLE), + true); + withAuthorizer( + authorizer, + () -> { + NameIdentifier[] tables = threeTables(); + NameIdentifier[] filtered = filterTableLike(tables); + + Assertions.assertSame( + tables, + filtered, + "A MODIFY_TABLE deny must not disable the independent SELECT_TABLE access path"); + verify(authorizer, times(1)) + .hasDenyPolicy( + any(), eq("testMetalake"), eq(Set.of(Privilege.Name.SELECT_TABLE)), any()); + verify(authorizer, times(0)) + .hasDenyPolicy( + any(), eq("testMetalake"), eq(Set.of(Privilege.Name.MODIFY_TABLE)), any()); + }); + } + + @Test + public void testListTableLikeShortCircuitTriesNextPathAfterDeny() { + GravitinoAuthorizer authorizer = + mockTableListRouteAuthorizer( + Set.of(Privilege.Name.SELECT_TABLE, Privilege.Name.MODIFY_TABLE), + Set.of(Privilege.Name.SELECT_TABLE), + true); + withAuthorizer( + authorizer, + () -> { + NameIdentifier[] tables = threeTables(); + NameIdentifier[] filtered = filterTableLike(tables); + + Assertions.assertSame( + tables, + filtered, + "A denied SELECT_TABLE path must not hide a deny-free MODIFY_TABLE path"); + verify(authorizer, times(1)) + .hasDenyPolicy( + any(), eq("testMetalake"), eq(Set.of(Privilege.Name.SELECT_TABLE)), any()); + verify(authorizer, times(1)) + .hasDenyPolicy( + any(), eq("testMetalake"), eq(Set.of(Privilege.Name.MODIFY_TABLE)), any()); + }); + } + + @Test + public void testListTableLikeShortCircuitRequiresUsePrivileges() { + GravitinoAuthorizer authorizer = + mockTableListRouteAuthorizer(Set.of(Privilege.Name.SELECT_TABLE), Set.of(), false); + withAuthorizer( + authorizer, + () -> { + NameIdentifier[] filtered = filterTableLike(threeTables()); + + verify(authorizer, times(0)).hasDenyPolicy(any(), eq("testMetalake"), anySet(), any()); + Assertions.assertEquals( + 0, + filtered.length, + "The short-circuit must not bypass the table-like expression's use privileges"); + }); + } + + @Test + public void testListShortCircuitOwnerIgnoresPrivilegeDenies() { + GravitinoAuthorizer authorizer = mockTableListAuthorizer(true, true); + withAuthorizer( + authorizer, + () -> { + NameIdentifier[] tables = threeTables(); + NameIdentifier[] filtered = + MetadataAuthzHelper.filterByExpression( + "testMetalake", + AuthorizationExpressionConstants.FILTER_TABLE_AUTHORIZATION_EXPRESSION, + Entity.EntityType.TABLE, + tables); + + Assertions.assertSame( + tables, + filtered, + "Privilege denies do not invalidate an independent ancestor-owner access path"); + verify(authorizer, times(0)).hasDenyPolicy(any(), eq("testMetalake"), anySet(), any()); + }); + } + + private static NameIdentifier[] filterTableLike(NameIdentifier[] tables) { + return MetadataAuthzHelper.filterByExpression( + "testMetalake", + AuthorizationExpressionConstants.LIST_TABLE_LIKE_AUTHORIZATION_EXPRESSION, + Entity.EntityType.TABLE, + tables); + } + private static void makeCompletableFutureUseCurrentThread() { try { Executor currentThread = Runnable::run;
