This is an automated email from the ASF dual-hosted git repository.
yuqi1129 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new d54358e1bc [#12622] improvement(server): apply the list authorization
short-circuit to every list shape (#12623)
d54358e1bc is described below
commit d54358e1bc637418b6b22c64ad5321745d81e50d
Author: Qi Yu <[email protected]>
AuthorDate: Wed Aug 26 20:52:35 2026 +0800
[#12622] improvement(server): apply the list authorization short-circuit to
every list shape (#12623)
### What changes were proposed in this pull request?
Move the parent-scope short-circuit and the `preloadToCache` /
`preloadOwner` calls from `MetadataAuthzHelper.filterByExpression(...,
NameIdentifier[])` into the generic `filterByExpression(..., E[],
Function)`, and let the identifier overload delegate to the generic one.
Every list endpoint now passes the same point regardless of the shape it
holds its results in, so a caller with a parent-scope grant is filtered
with one expression evaluation and one deny check instead of one
evaluation per object.
### Why are the changes needed?
`listCatalogs?details=true` carries `Catalog[]` and went through the
generic overload, which had neither the short-circuit nor the preloads,
so it ran the per-object loop over every catalog in the metalake.
Catalogs are the only entity type with a `details=true` variant, which
is why tables and schemas were unaffected and this went unnoticed.
The list endpoints for functions, groups, jobs, policies, roles, tags
and users also use the generic overload and gain the preloads.
Fix: #12622
### Does this PR introduce _any_ user-facing change?
No. Filtering results are unchanged; only the work done to reach them
is. No API or property is added.
### How was this patch tested?
- New `testListShortCircuitAppliesToNonIdentifierResults` filters a
non-identifier array through the generic overload. It fails without this
change and passes with it. It asserts `hasDenyPolicy` is consulted
exactly once, which only the short-circuit does; asserting on the
returned elements alone would not catch the regression, because the
metalake-scope grant satisfies the per-object expression as well.
- `./gradlew :server-common:test :server:test -PskipITs` —
`TestMetadataAuthzHelper` 10, `TestCatalogOperations` 9,
`TestCatalogAuthorizationExpression` 5, no failures.
- `./gradlew :server-common:spotlessCheck :server-common:javadoc
:server:compileJava :iceberg:iceberg-rest-server:compileJava -PskipITs`.
- `TestHttpsServerAuthentication` mutual-TLS cases fail intermittently
in a full `:server-common:test` run on `main` as well, with different
cases failing across runs; unrelated to this change.
---
.../server/authorization/MetadataAuthzHelper.java | 59 ++++++++++++----------
.../authorization/TestMetadataAuthzHelper.java | 55 ++++++++++++++++++++
2 files changed, 88 insertions(+), 26 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 fc7db7b615..cd6178258f 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
@@ -206,32 +206,6 @@ public class MetadataAuthzHelper {
String expression,
Entity.EntityType entityType,
NameIdentifier[] nameIdentifiers) {
- if (enableAuthorization() && nameIdentifiers.length > 0) {
- String principalName = PrincipalUtils.getCurrentPrincipal().getName();
- if (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_*.
- LOG.debug(
- "List authorization short-circuit HIT for principal {}, entity
type {} under metalake "
- + "{}: all {} listed object(s) are visible via a parent-scope
grant; skipping the "
- + "per-object authorization loop.",
- principalName,
- entityType,
- metalake,
- nameIdentifiers.length);
- return nameIdentifiers;
- }
- LOG.debug(
- "List authorization short-circuit MISS for principal {}, entity type
{} under metalake "
- + "{} ({} object(s)); falling back to the per-object
authorization loop.",
- principalName,
- entityType,
- metalake,
- nameIdentifiers.length);
- }
- preloadToCache(entityType, nameIdentifiers);
- preloadOwner(entityType, nameIdentifiers);
return filterByExpression(metalake, expression, entityType,
nameIdentifiers, e -> e);
}
@@ -356,6 +330,39 @@ 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) {
+ String principalName = PrincipalUtils.getCurrentPrincipal().getName();
+ if (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_*.
+ LOG.debug(
+ "List authorization short-circuit HIT for principal {}, entity
type {} under metalake "
+ + "{}: all {} listed object(s) are visible via a parent-scope
grant; skipping the "
+ + "per-object authorization loop.",
+ principalName,
+ entityType,
+ metalake,
+ nameIdentifiers.length);
+ return entities;
+ }
+ LOG.debug(
+ "List authorization short-circuit MISS for principal {}, entity type
{} under metalake "
+ + "{} ({} object(s)); falling back to the per-object
authorization loop.",
+ principalName,
+ entityType,
+ metalake,
+ nameIdentifiers.length);
+ }
+ preloadToCache(entityType, nameIdentifiers);
+ preloadOwner(entityType, nameIdentifiers);
+
GravitinoAuthorizer authorizer =
GravitinoAuthorizerProvider.getInstance().getGravitinoAuthorizer();
AuthorizationRequestContext authorizationRequestContext = new
AuthorizationRequestContext();
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 a5019085cb..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
@@ -23,6 +23,8 @@ 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;
@@ -321,6 +323,59 @@ public class TestMetadataAuthzHelper {
}
}
+ /**
+ * 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
