This is an automated email from the ASF dual-hosted git repository.
yuqi1129 pushed a commit to branch branch-1.3
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/branch-1.3 by this push:
new 6a8dc954a3 [#11775][#12622] improvement(authz): Optimize list
authorization (#12648)
6a8dc954a3 is described below
commit 6a8dc954a345de72a9cdb3208756336229321787
Author: github-actions[bot]
<41898282+github-actions[bot]@users.noreply.github.com>
AuthorDate: Thu Aug 27 17:31:16 2026 +0800
[#11775][#12622] improvement(authz): Optimize list authorization (#12648)
### What changes were proposed in this pull request?
- Backport #11778 to `branch-1.3`, including the parent-scope
list-authorization short-circuit, deny-policy detection, authorization
expressions, JCasbin implementation, and tests.
- Apply #12623 on top of that prerequisite by moving the short-circuit
and cache preloads into the generic `filterByExpression` overload.
- Let the `NameIdentifier[]` overload delegate to the generic overload
so every list result shape follows the same authorization path.
The short-circuit is deliberately conservative. It returns the complete
list only when all objects share one parent, the caller has the matching
parent-scope grant, and no relevant deny policy may exist. Otherwise it
falls back to normal per-object authorization.
### Why are the changes needed?
#12623 depends on the parent-scope short-circuit introduced by #11778,
but #11778 was not present on `branch-1.3`. The automatic cherry-pick
therefore committed conflict markers and referenced APIs that did not
exist on the release branch.
Combining the prerequisite and follow-up makes the backport
self-contained. It also fixes verbose catalog listing and other
endpoints that hold list results in non-`NameIdentifier` arrays; these
endpoints now receive the same safe short-circuit and cache preloading
as identifier-based lists.
Fix: #11775, #12622
### 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?
- `:core:spotlessApply :server-common:spotlessApply`
- `:server-common:test --tests TestMetadataAuthzHelper --tests
TestJcasbinAuthorizer -PskipITs` — 59 tests, no failures.
- `:server:test --tests TestCatalogOperations --tests
TestCatalogAuthorizationExpression -PskipITs` — 13 tests, no failures.
- `:server-common:javadoc :iceberg:iceberg-rest-server:compileJava
-PskipITs`
- `git diff --check` and conflict-marker scan.
All local Gradle commands were run with proxy environment variables
disabled.
---------
Co-authored-by: Qi Yu <[email protected]>
---
.../authorization/GravitinoAuthorizer.java | 32 ++
.../server/authorization/MetadataAuthzHelper.java | 103 +++++-
.../AuthorizationExpressionConstants.java | 42 +++
.../authorization/jcasbin/JcasbinAuthorizer.java | 48 +++
.../authorization/TestMetadataAuthzHelper.java | 369 +++++++++++++++++++++
.../jcasbin/TestJcasbinAuthorizer.java | 65 ++++
6 files changed, 657 insertions(+), 2 deletions(-)
diff --git
a/core/src/main/java/org/apache/gravitino/authorization/GravitinoAuthorizer.java
b/core/src/main/java/org/apache/gravitino/authorization/GravitinoAuthorizer.java
index 6860c5f8df..5b121f5abf 100644
---
a/core/src/main/java/org/apache/gravitino/authorization/GravitinoAuthorizer.java
+++
b/core/src/main/java/org/apache/gravitino/authorization/GravitinoAuthorizer.java
@@ -19,6 +19,7 @@ package org.apache.gravitino.authorization;
import java.io.Closeable;
import java.security.Principal;
+import java.util.Set;
import javax.annotation.Nullable;
import org.apache.gravitino.Entity;
import org.apache.gravitino.GravitinoEnv;
@@ -107,6 +108,37 @@ public interface GravitinoAuthorizer extends Closeable {
*/
boolean isMetalakeUser(String metalake, AuthorizationRequestContext
requestContext);
+ /**
+ * Determines whether the given principal may have any {@code DENY} policy,
for any of the given
+ * privileges, at any scope (metalake, catalog, schema or the object itself)
within the metalake.
+ *
+ * <p>This supports list-authorization short-circuiting: when a privilege is
granted at a parent
+ * scope (metalake, catalog or schema), every child object is visible
<em>unless</em> a {@code
+ * DENY} overrides it at some scope. A {@code false} return therefore lets
the caller skip
+ * per-object authorization for the whole list and return every identifier;
a {@code true} return
+ * forces the caller to fall back to per-object authorization.
+ *
+ * <p>The query is scope-agnostic on purpose: a deny granted at a parent
scope hides the whole
+ * subtree, while a deny granted on a single object hides just that object.
Both cases must
+ * disable the short-circuit, so the privilege match is not restricted to a
metadata type.
+ *
+ * <p>The default implementation conservatively returns {@code true} so that
authorizers which
+ * cannot answer the question never enable an unsafe short-circuit.
+ *
+ * @param principal the user principal
+ * @param metalake the metalake
+ * @param privileges the privileges whose denies would affect visibility
+ * @param requestContext authorization request context
+ * @return whether a deny for any of the given privileges may exist
+ */
+ default boolean hasDenyPolicy(
+ Principal principal,
+ String metalake,
+ Set<Privilege.Name> privileges,
+ AuthorizationRequestContext requestContext) {
+ return true;
+ }
+
/**
* Determine whether the user can set owner
*
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 f1ddaa368c..65a149ce16 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
@@ -26,6 +26,7 @@ import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
+import java.util.Set;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.Executor;
import java.util.concurrent.Executors;
@@ -42,6 +43,7 @@ import org.apache.gravitino.Namespace;
import org.apache.gravitino.SupportsRelationOperations;
import org.apache.gravitino.authorization.AuthorizationRequestContext;
import org.apache.gravitino.authorization.GravitinoAuthorizer;
+import org.apache.gravitino.authorization.Privilege;
import org.apache.gravitino.dto.tag.MetadataObjectDTO;
import
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants;
import
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionEvaluator;
@@ -87,6 +89,43 @@ public class MetadataAuthzHelper {
private static final List<Entity.EntityType> REQUIRE_SCHEMA_EXISTS =
Arrays.asList(Entity.EntityType.TABLE, Entity.EntityType.TOPIC);
+ /**
+ * 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.
+ */
+ 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 final Set<Privilege.Name> denyPrivileges;
+
+ private ListShortCircuit(
+ String filterExpression, String parentScopeExpression,
Set<Privilege.Name> denyPrivileges) {
+ this.filterExpression = filterExpression;
+ this.parentScopeExpression = parentScopeExpression;
+ this.denyPrivileges = denyPrivileges;
+ }
+ }
+
private MetadataAuthzHelper() {}
public static Metalake[] filterMetalakes(Metalake[] metalakes, String
expression) {
@@ -167,11 +206,54 @@ public class MetadataAuthzHelper {
String expression,
Entity.EntityType entityType,
NameIdentifier[] nameIdentifiers) {
- preloadToCache(entityType, nameIdentifiers);
- preloadOwner(entityType, nameIdentifiers);
return filterByExpression(metalake, expression, entityType,
nameIdentifiers, e -> e);
}
+ /**
+ * Attempts the list-authorization short-circuit: when the listed objects
all share one parent and
+ * the matching filter expression is granted at a parent scope, the whole
list is visible unless
+ * an object-level deny may exist. Returns {@code true} only when it is safe
to return every
+ * identifier without per-object authorization.
+ */
+ private static boolean allVisibleViaParentScope(
+ String metalake,
+ String expression,
+ Entity.EntityType entityType,
+ NameIdentifier[] nameIdentifiers) {
+ ListShortCircuit spec = LIST_SHORT_CIRCUITS.get(entityType);
+ if (spec == null || !spec.filterExpression.equals(expression)) {
+ return false;
+ }
+
+ // The short-circuit reasons about a single parent scope, so every
identifier must share it.
+ Namespace parent = nameIdentifiers[0].namespace();
+ for (NameIdentifier ident : nameIdentifiers) {
+ if (!ident.namespace().equals(parent)) {
+ return false;
+ }
+ }
+
+ Principal principal = PrincipalUtils.getCurrentPrincipal();
+ GravitinoAuthorizer authorizer =
+ GravitinoAuthorizerProvider.getInstance().getGravitinoAuthorizer();
+ AuthorizationRequestContext requestContext = new
AuthorizationRequestContext();
+
requestContext.setOriginalAuthorizationExpression(spec.parentScopeExpression);
+ Map<Entity.EntityType, NameIdentifier> metadataNames =
+ NameIdentifierUtil.splitNameIdentifier(metalake, entityType,
nameIdentifiers[0]);
+
+ boolean parentGrantsAccess =
+ new AuthorizationExpressionEvaluator(spec.parentScopeExpression,
authorizer)
+ .evaluate(metadataNames, requestContext, principal,
Optional.empty());
+ if (!parentGrantsAccess) {
+ return false;
+ }
+
+ // Parent scope grants access to every object; the only thing that can
still hide one is a
+ // 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);
+ }
+
/**
* Call {@link AuthorizationExpressionEvaluator} to check access
*
@@ -212,6 +294,23 @@ public class MetadataAuthzHelper {
Entity.EntityType entityType,
E[] entities,
Function<E, NameIdentifier> toNameIdentifier) {
+ // Every list endpoint funnels through here, whichever shape it holds its
results in, so the
+ // short-circuit and the preloads live at this one point. Keeping them in
the NameIdentifier[]
+ // overload alone let the verbose catalog listing, which carries Catalog
objects, run the
+ // per-object loop over every catalog in the metalake.
+ NameIdentifier[] nameIdentifiers =
+
Arrays.stream(entities).map(toNameIdentifier).toArray(NameIdentifier[]::new);
+ if (enableAuthorization()
+ && nameIdentifiers.length > 0
+ && allVisibleViaParentScope(metalake, expression, entityType,
nameIdentifiers)) {
+ // A privilege granted at a parent scope (metalake/catalog/schema) makes
every object in the
+ // list visible, and no object-level deny exists, so the per-object
authorization loop is
+ // skipped entirely. See
AuthorizationExpressionConstants.*_LIST_PARENT_SCOPE_*.
+ return entities;
+ }
+ preloadToCache(entityType, nameIdentifiers);
+ preloadOwner(entityType, nameIdentifiers);
+
GravitinoAuthorizer authorizer =
GravitinoAuthorizerProvider.getInstance().getGravitinoAuthorizer();
AuthorizationRequestContext authorizationRequestContext = new
AuthorizationRequestContext();
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionConstants.java
b/server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionConstants.java
index 122bdfcd01..c8fd3d07d1 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionConstants.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionConstants.java
@@ -166,6 +166,48 @@ public class AuthorizationExpressionConstants {
ANY_MODIFY_TABLE
""";
+ /**
+ * Parent-scope expression used to short-circuit table-list authorization.
It is the ancestor-only
+ * portion of {@link #FILTER_TABLE_AUTHORIZATION_EXPRESSION}: ownership of,
or a SELECT/MODIFY
+ * grant at, the metalake/catalog/schema scope means every table in the
schema is visible.
+ * Evaluated once per list request instead of once per table.
+ *
+ * <p>This expression only checks the positive parent-scope grant; deny
coverage (at any scope) is
+ * handled separately by {@link
+ * org.apache.gravitino.authorization.GravitinoAuthorizer#hasDenyPolicy} so
the short-circuit
+ * falls back to per-object authorization whenever a relevant deny may exist.
+ */
+ public static final String TABLE_LIST_PARENT_SCOPE_AUTHORIZATION_EXPRESSION =
+ """
+ ANY(OWNER, METALAKE, CATALOG, SCHEMA) ||
+ ANY(SELECT_TABLE, METALAKE, CATALOG, SCHEMA) ||
+ ANY(MODIFY_TABLE, METALAKE, CATALOG, SCHEMA)
+ """;
+
+ /**
+ * Parent-scope expression used to short-circuit schema-list authorization.
Ancestor-only portion
+ * of {@link #FILTER_SCHEMA_AUTHORIZATION_EXPRESSION}: ownership of, or a
USE_SCHEMA grant at, the
+ * metalake/catalog scope means every schema in the catalog is visible. Deny
coverage is handled
+ * by {@link
org.apache.gravitino.authorization.GravitinoAuthorizer#hasDenyPolicy}.
+ */
+ public static final String SCHEMA_LIST_PARENT_SCOPE_AUTHORIZATION_EXPRESSION
=
+ """
+ ANY(OWNER, METALAKE, CATALOG) ||
+ ANY(USE_SCHEMA, METALAKE, CATALOG)
+ """;
+
+ /**
+ * Parent-scope expression used to short-circuit catalog-list authorization.
Ancestor-only portion
+ * of {@link #LOAD_CATALOG_AUTHORIZATION_EXPRESSION}: metalake ownership, or
a USE_CATALOG grant
+ * at the metalake scope, means every catalog is visible. Deny coverage is
handled by {@link
+ * org.apache.gravitino.authorization.GravitinoAuthorizer#hasDenyPolicy}.
+ */
+ public static final String
CATALOG_LIST_PARENT_SCOPE_AUTHORIZATION_EXPRESSION =
+ """
+ METALAKE::OWNER ||
+ ANY(USE_CATALOG, METALAKE)
+ """;
+
public static final String FILTER_VIEW_AUTHORIZATION_EXPRESSION =
"""
ANY(OWNER, METALAKE, CATALOG, SCHEMA, VIEW) ||
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java
b/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java
index 7e68172bf3..84945fc0bf 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java
@@ -122,6 +122,9 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
*/
private static final int POLICY_SUBJECT_FIELD_INDEX = 0;
+ /** Field index of {@code act} (the privilege) in a jcasbin {@code p} policy
row. */
+ private static final int POLICY_ACTION_FIELD_INDEX = 3;
+
/**
* How long to wait before retrying a role whose last policy load was
incomplete, i.e. at least
* one of its securable objects could not be resolved to a metadata id. Such
a role is
@@ -411,6 +414,51 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
return loadUserInfo(metalake, currentUserName, requestContext).isPresent();
}
+ @Override
+ public boolean hasDenyPolicy(
+ Principal principal,
+ String metalake,
+ Set<Privilege.Name> privileges,
+ AuthorizationRequestContext requestContext) {
+ Optional<UserUpdatedAt> userInfoOpt =
+ loadUserInfo(metalake, principal.getName(), requestContext);
+ if (!userInfoOpt.isPresent()) {
+ // An unknown user holds no roles and therefore no deny policies.
+ return false;
+ }
+ UserUpdatedAt userInfo = userInfoOpt.get();
+ long userId = userInfo.getUserId();
+ // Bind the user's (direct + group-inherited) roles into the enforcers so
the in-memory scan
+ // below sees every policy the user can carry. Idempotent within a request.
+ loadRolePrivilege(metalake, principal.getName(), userId, userInfo,
requestContext);
+
+ Set<String> privilegeNames =
privileges.stream().map(Enum::name).collect(Collectors.toSet());
+ String userIdStr = String.valueOf(userId);
+ // This is an existence query, not a per-object check: it answers "does
any deny on these
+ // privileges exist for the user's roles, at any scope?" The standard
enforce path needs a
+ // concrete metadataId, so reusing it would mean iterating every listed
object and defeat the
+ // short-circuit. Filtering the deny enforcer's policies by role keeps the
scan bounded by the
+ // user's role/policy count, never by the number of listed objects. The
match is intentionally
+ // scope-agnostic (no metadataType filter): a parent-scope deny hides the
whole subtree and an
+ // object-scope deny hides one object, and both must disable the
short-circuit.
+ for (String roleId : denyEnforcer.getRolesForUser(userIdStr)) {
+ // getFilteredNamedPolicy returns every "p" row (p = sub, metadataType,
metadataId, act, eft)
+ // whose field at POLICY_SUBJECT_FIELD_INDEX (sub) equals roleId, i.e.
all rules carried by
+ // this role. denyEnforcer is a dedicated enforcer that is only ever
loaded with privileges
+ // whose condition is DENY (see loadPolicyByRoleEntity), so every row
here represents a deny
+ // regardless of its stored eft string. Each returned row is the list of
those five fields,
+ // so we read field POLICY_ACTION_FIELD_INDEX (act) to compare the
denied privilege.
+ for (List<String> policy :
+ denyEnforcer.getFilteredNamedPolicy("p", POLICY_SUBJECT_FIELD_INDEX,
roleId)) {
+ if (policy.size() > POLICY_ACTION_FIELD_INDEX
+ && privilegeNames.contains(policy.get(POLICY_ACTION_FIELD_INDEX)))
{
+ return true;
+ }
+ }
+ }
+ return false;
+ }
+
@Override
public boolean isSelf(
Entity.EntityType type,
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 81e7d858ca..15783d818a 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
@@ -18,12 +18,17 @@
package org.apache.gravitino.server.authorization;
import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anySet;
import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.lenient;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.mockStatic;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import java.lang.reflect.Field;
+import java.util.Arrays;
import java.util.concurrent.Executor;
import org.apache.gravitino.Config;
import org.apache.gravitino.Configs;
@@ -32,7 +37,10 @@ import org.apache.gravitino.GravitinoEnv;
import org.apache.gravitino.MetadataObject;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.UserPrincipal;
+import org.apache.gravitino.authorization.GravitinoAuthorizer;
+import org.apache.gravitino.authorization.Privilege;
import org.apache.gravitino.dto.tag.MetadataObjectDTO;
+import
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants;
import org.apache.gravitino.utils.NameIdentifierUtil;
import org.apache.gravitino.utils.PrincipalUtils;
import org.junit.jupiter.api.AfterAll;
@@ -190,6 +198,367 @@ public class TestMetadataAuthzHelper {
}
}
+ /**
+ * Builds three table identifiers under the same schema, where a parent
(schema) level
+ * SELECT_TABLE grant exists and the middle table additionally carries a
table-level deny. The
+ * deny gate is controlled separately so each branch of the list
short-circuit can be exercised.
+ */
+ private GravitinoAuthorizer mockTableListAuthorizer(boolean owner, boolean
denyGate) {
+ GravitinoAuthorizer authorizer = mock(GravitinoAuthorizer.class);
+ // Parent-scope SELECT_TABLE grant at the schema level (applies to every
table in the schema).
+ lenient()
+ .when(authorizer.authorize(any(), eq("testMetalake"), any(), any(),
any()))
+ .thenAnswer(
+ invocation -> {
+ MetadataObject object = invocation.getArgument(2);
+ Privilege.Name privilege = invocation.getArgument(3);
+ return object.type() == MetadataObject.Type.SCHEMA
+ && privilege == Privilege.Name.SELECT_TABLE;
+ });
+ // The middle table (t2) has a table-level deny on SELECT_TABLE.
+ lenient()
+ .when(authorizer.deny(any(), eq("testMetalake"), any(), any(), any()))
+ .thenAnswer(
+ invocation -> {
+ MetadataObject object = invocation.getArgument(2);
+ Privilege.Name privilege = invocation.getArgument(3);
+ return object.type() == MetadataObject.Type.TABLE
+ && "t2".equals(object.name())
+ && privilege == Privilege.Name.SELECT_TABLE;
+ });
+ lenient().when(authorizer.isOwner(any(), eq("testMetalake"), any(),
any())).thenReturn(owner);
+ lenient()
+ .when(authorizer.hasDenyPolicy(any(), eq("testMetalake"), anySet(),
any()))
+ .thenReturn(denyGate);
+ return authorizer;
+ }
+
+ /**
+ * 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
+ * fully visible via the parent-scope short-circuit.
+ */
+ private GravitinoAuthorizer mockParentGrantAuthorizer(
+ MetadataObject.Type grantType, Privilege.Name grantPrivilege) {
+ 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);
+ return object.type() == grantType && privilege == grantPrivilege;
+ });
+ 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()))
+ .thenReturn(false);
+ return authorizer;
+ }
+
+ @Test
+ public void testListShortCircuitSchemaViaCatalogGrant() {
+ 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);
+ GravitinoAuthorizer authorizer =
+ mockParentGrantAuthorizer(MetadataObject.Type.CATALOG,
Privilege.Name.USE_SCHEMA);
+ when(mockedProvider.getGravitinoAuthorizer()).thenReturn(authorizer);
+
+ NameIdentifier[] schemas =
+ new NameIdentifier[] {
+ NameIdentifierUtil.ofSchema("testMetalake", "testCatalog", "s1"),
+ NameIdentifierUtil.ofSchema("testMetalake", "testCatalog", "s2")
+ };
+ NameIdentifier[] filtered =
+ MetadataAuthzHelper.filterByExpression(
+ "testMetalake",
+
AuthorizationExpressionConstants.FILTER_SCHEMA_AUTHORIZATION_EXPRESSION,
+ Entity.EntityType.SCHEMA,
+ schemas);
+
+ Assertions.assertEquals(2, filtered.length);
+ }
+ }
+
+ @Test
+ public void testListShortCircuitCatalogViaMetalakeGrant() {
+ 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);
+ GravitinoAuthorizer authorizer =
+ mockParentGrantAuthorizer(MetadataObject.Type.METALAKE,
Privilege.Name.USE_CATALOG);
+ when(mockedProvider.getGravitinoAuthorizer()).thenReturn(authorizer);
+
+ NameIdentifier[] catalogs =
+ new NameIdentifier[] {
+ NameIdentifierUtil.ofCatalog("testMetalake", "c1"),
+ NameIdentifierUtil.ofCatalog("testMetalake", "c2")
+ };
+ NameIdentifier[] filtered =
+ MetadataAuthzHelper.filterByExpression(
+ "testMetalake",
+
AuthorizationExpressionConstants.LOAD_CATALOG_AUTHORIZATION_EXPRESSION,
+ Entity.EntityType.CATALOG,
+ catalogs);
+
+ Assertions.assertEquals(2, filtered.length);
+ }
+ }
+
+ /**
+ * The verbose catalog listing hands this helper {@code Catalog} objects
rather than identifiers,
+ * so it goes through the generic overload. That overload used to skip the
short-circuit, which
+ * left every catalog in the metalake on the per-object path.
+ *
+ * <p>Proving which path ran needs care. Comparing the returned elements
does not work, because
+ * the metalake-scope grant satisfies the per-object expression too and
every catalog comes back
+ * either way. Counting authorizer calls does not work either: the
per-request cache in {@link
+ * org.apache.gravitino.authorization.AuthorizationRequestContext} collapses
the repeated
+ * metalake-scope check, so the count is the same for three catalogs and for
thirty.
+ *
+ * <p>What does separate them is the array itself. The short-circuit hands
back the caller's own
+ * array untouched, while the per-object path collects survivors into a new
one, so identity says
+ * which branch produced the result.
+ */
+ @Test
+ public void testListShortCircuitAppliesToNonIdentifierResults() {
+ 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);
+ GravitinoAuthorizer authorizer =
+ mockParentGrantAuthorizer(MetadataObject.Type.METALAKE,
Privilege.Name.USE_CATALOG);
+ when(mockedProvider.getGravitinoAuthorizer()).thenReturn(authorizer);
+
+ // Stands in for the Catalog objects the verbose listing carries:
anything that is not a
+ // NameIdentifier and needs a mapper to become one.
+ String[] catalogNames = new String[] {"c1", "c2", "c3"};
+
+ String[] filtered =
+ MetadataAuthzHelper.filterByExpression(
+ "testMetalake",
+
AuthorizationExpressionConstants.LOAD_CATALOG_AUTHORIZATION_EXPRESSION,
+ Entity.EntityType.CATALOG,
+ catalogNames,
+ name -> NameIdentifierUtil.ofCatalog("testMetalake", name));
+
+ Assertions.assertSame(
+ catalogNames,
+ filtered,
+ "The parent-scope short-circuit must return the caller's array as
is; a new array means "
+ + "the per-object authorization loop ran for every catalog");
+ // Only the short-circuit asks this, and it asks once, after the parent
grant is confirmed.
+ verify(authorizer, times(1)).hasDenyPolicy(any(), eq("testMetalake"),
anySet(), any());
+ }
+ }
+
+ /**
+ * Builds an authorizer that grants {@code SELECT_TABLE} at the schema
scope, but only for the
+ * schema whose simple name equals {@code grantedSchema}. Used to prove the
short-circuit never
+ * applies one parent's grant to siblings under a different parent.
+ */
+ private GravitinoAuthorizer mockSchemaScopedTableAuthorizer(String
grantedSchema) {
+ 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);
+ return object.type() == MetadataObject.Type.SCHEMA
+ && grantedSchema.equals(object.name())
+ && privilege == Privilege.Name.SELECT_TABLE;
+ });
+ 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()))
+ .thenReturn(false);
+ return authorizer;
+ }
+
+ /**
+ * Builds an authorizer that grants no parent-scope privilege at all and
only owns a single table
+ * ({@code ownedTable}). No ancestor grant means the short-circuit must not
trigger, falling back
+ * to per-object filtering that exposes only the owned table.
+ */
+ private GravitinoAuthorizer mockTableOwnerOnlyAuthorizer(String ownedTable) {
+ GravitinoAuthorizer authorizer = mock(GravitinoAuthorizer.class);
+ lenient()
+ .when(authorizer.authorize(any(), eq("testMetalake"), any(), any(),
any()))
+ .thenReturn(false);
+ lenient()
+ .when(authorizer.deny(any(), eq("testMetalake"), any(), any(), any()))
+ .thenReturn(false);
+ lenient()
+ .when(authorizer.isOwner(any(), eq("testMetalake"), any(), any()))
+ .thenAnswer(
+ invocation -> {
+ MetadataObject object = invocation.getArgument(2);
+ return object.type() == MetadataObject.Type.TABLE &&
ownedTable.equals(object.name());
+ });
+ lenient()
+ .when(authorizer.hasDenyPolicy(any(), eq("testMetalake"), anySet(),
any()))
+ .thenReturn(false);
+ return authorizer;
+ }
+
+ @Test
+ public void testListShortCircuitDoesNotLeakAcrossParents() {
+ 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);
+ // Grant SELECT_TABLE only at schema s1; t2 lives under a different
schema s2.
+ GravitinoAuthorizer authorizer = mockSchemaScopedTableAuthorizer("s1");
+ when(mockedProvider.getGravitinoAuthorizer()).thenReturn(authorizer);
+
+ NameIdentifier[] tables =
+ new NameIdentifier[] {
+ NameIdentifierUtil.ofTable("testMetalake", "testCatalog", "s1",
"t1"),
+ NameIdentifierUtil.ofTable("testMetalake", "testCatalog", "s2",
"t2")
+ };
+ NameIdentifier[] filtered =
+ MetadataAuthzHelper.filterByExpression(
+ "testMetalake",
+
AuthorizationExpressionConstants.FILTER_TABLE_AUTHORIZATION_EXPRESSION,
+ Entity.EntityType.TABLE,
+ tables);
+
+ // Identifiers span two schemas, so the single-parent short-circuit must
not fire. Per-object
+ // filtering keeps only t1 (granted via s1) and never leaks t2 from the
ungranted schema s2.
+ Assertions.assertEquals(1, filtered.length);
+ Assertions.assertEquals("t1", filtered[0].name());
+ }
+ }
+
+ @Test
+ public void testListNoParentGrantFallsBackToPerObject() {
+ 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);
+ GravitinoAuthorizer authorizer = mockTableOwnerOnlyAuthorizer("t1");
+ when(mockedProvider.getGravitinoAuthorizer()).thenReturn(authorizer);
+
+ NameIdentifier[] filtered =
+ MetadataAuthzHelper.filterByExpression(
+ "testMetalake",
+
AuthorizationExpressionConstants.FILTER_TABLE_AUTHORIZATION_EXPRESSION,
+ Entity.EntityType.TABLE,
+ threeTables());
+
+ // No ancestor grant exists, so the short-circuit is skipped and
per-object filtering returns
+ // only the single table the user owns.
+ Assertions.assertEquals(1, filtered.length);
+ Assertions.assertEquals("t1", filtered[0].name());
+ }
+ }
+
+ private static NameIdentifier[] threeTables() {
+ return new NameIdentifier[] {
+ NameIdentifierUtil.ofTable("testMetalake", "testCatalog", "testSchema",
"t1"),
+ NameIdentifierUtil.ofTable("testMetalake", "testCatalog", "testSchema",
"t2"),
+ NameIdentifierUtil.ofTable("testMetalake", "testCatalog", "testSchema",
"t3")
+ };
+ }
+
+ @Test
+ public void testListShortCircuitParentGrantWithoutDenyReturnsAll() {
+ 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);
+ GravitinoAuthorizer authorizer = mockTableListAuthorizer(false, false);
+ when(mockedProvider.getGravitinoAuthorizer()).thenReturn(authorizer);
+
+ NameIdentifier[] filtered =
+ MetadataAuthzHelper.filterByExpression(
+ "testMetalake",
+
AuthorizationExpressionConstants.FILTER_TABLE_AUTHORIZATION_EXPRESSION,
+ Entity.EntityType.TABLE,
+ threeTables());
+
+ // The schema-level grant covers every table and no object-level deny is
reported, so the
+ // whole list is returned without consulting the per-table deny on t2.
+ Assertions.assertEquals(3, filtered.length);
+ }
+ }
+
+ @Test
+ public void testListShortCircuitFallsBackWhenDenyMayExist() {
+ 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);
+ GravitinoAuthorizer authorizer = mockTableListAuthorizer(false, true);
+ when(mockedProvider.getGravitinoAuthorizer()).thenReturn(authorizer);
+
+ NameIdentifier[] filtered =
+ MetadataAuthzHelper.filterByExpression(
+ "testMetalake",
+
AuthorizationExpressionConstants.FILTER_TABLE_AUTHORIZATION_EXPRESSION,
+ Entity.EntityType.TABLE,
+ threeTables());
+
+ // A deny may exist, so the short-circuit is disabled and per-table
filtering excludes t2.
+ Assertions.assertEquals(2, filtered.length);
+ Assertions.assertTrue(
+ Arrays.stream(filtered).noneMatch(id -> "t2".equals(id.name())),
+ "t2 must be filtered out by its table-level deny");
+ }
+ }
+
private static void makeCompletableFutureUseCurrentThread() {
try {
Executor currentThread = Runnable::run;
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java
b/server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java
index 829fb99e19..b067751a2e 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java
@@ -17,6 +17,7 @@
package org.apache.gravitino.server.authorization.jcasbin;
+import static org.apache.gravitino.authorization.Privilege.Name.SELECT_TABLE;
import static org.apache.gravitino.authorization.Privilege.Name.USE_CATALOG;
import static org.apache.gravitino.authorization.Privilege.Name.USE_SCHEMA;
import static org.junit.jupiter.api.Assertions.assertEquals;
@@ -39,6 +40,7 @@ import static org.mockito.Mockito.when;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
import java.io.IOException;
import java.lang.reflect.Field;
import java.lang.reflect.Method;
@@ -467,6 +469,69 @@ public class TestJcasbinAuthorizer {
assertFalse(doAuthorize(currentPrincipal));
}
+ @Test
+ public void testHasDenyPolicy() throws Exception {
+ makeCompletableFutureUseCurrentThread(jcasbinAuthorizer);
+ Principal currentPrincipal = PrincipalUtils.getCurrentPrincipal();
+
+ // With no roles assigned, the user can hold no deny policy.
+ assertFalse(
+ jcasbinAuthorizer.hasDenyPolicy(
+ currentPrincipal,
+ METALAKE,
+ ImmutableSet.of(USE_CATALOG),
+ new AuthorizationRequestContext()));
+
+ // Assign a role that DENIES USE_CATALOG at the catalog scope.
+ mockRoleInStore(DENY_ROLE_ID, "denyRole",
ImmutableList.of(getDenySecurableObject()));
+ when(roleMetaMapper.listRolesByUserId(eq(USER_ID)))
+ .thenReturn(ImmutableList.of(buildRolePO(DENY_ROLE_ID, "denyRole")));
+ when(userMetaMapper.getUserUpdatedAt(eq(METALAKE), eq(USERNAME)))
+ .thenReturn(new UserUpdatedAt(USER_ID, nextUserVersion()));
+
+ // The deny on USE_CATALOG is detected. The match is scope-agnostic: the
deny lives on a
+ // catalog, which is a parent scope for a schema/catalog list, so it must
be reported and
+ // disable the short-circuit.
+ assertTrue(
+ jcasbinAuthorizer.hasDenyPolicy(
+ currentPrincipal,
+ METALAKE,
+ ImmutableSet.of(USE_CATALOG),
+ new AuthorizationRequestContext()));
+
+ // A deny on USE_CATALOG must not be reported when querying a different
privilege.
+ assertFalse(
+ jcasbinAuthorizer.hasDenyPolicy(
+ currentPrincipal,
+ METALAKE,
+ ImmutableSet.of(SELECT_TABLE),
+ new AuthorizationRequestContext()));
+ }
+
+ @Test
+ public void testHasDenyPolicyDetectsGroupInheritedDeny() throws Exception {
+ makeCompletableFutureUseCurrentThread(jcasbinAuthorizer);
+ getLoadedRolesCache(jcasbinAuthorizer).invalidateAll();
+
+ // The user holds no direct roles; the deny role is only reachable through
group membership.
+ UserPrincipal groupPrincipal = setCurrentPrincipalWithGroup(GROUP_NAME);
+ mockRoleInStore(DENY_ROLE_ID, "denyRole",
ImmutableList.of(getDenySecurableObject()));
+ mockNoDirectUserRoles();
+ mockGroupWithRoles(GROUP_NAME, ImmutableList.of(DENY_ROLE_ID),
ImmutableList.of("denyRole"));
+
+ // A deny inherited via a group must still be detected, otherwise the list
short-circuit would
+ // over-expose objects that a group-level deny is meant to hide.
+ assertTrue(
+ jcasbinAuthorizer.hasDenyPolicy(
+ groupPrincipal,
+ METALAKE,
+ ImmutableSet.of(USE_CATALOG),
+ new AuthorizationRequestContext()));
+
+ restoreDefaultPrincipal();
+ getLoadedRolesCache(jcasbinAuthorizer).invalidateAll();
+ }
+
@Test
public void testUserRoleCacheDoesNotReuseRolesAfterUsernameRecreate() throws
Exception {
makeCompletableFutureUseCurrentThread(jcasbinAuthorizer);
