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 c5f76b2c22 [#13251] improvement(authz): Reuse request context during
list filtering (#13252)
c5f76b2c22 is described below
commit c5f76b2c22ca345261d465d514e726005ba3c78c
Author: Qi Yu <[email protected]>
AuthorDate: Mon Sep 21 09:04:39 2026 +0800
[#13251] improvement(authz): Reuse request context during list filtering
(#13252)
### What changes were proposed in this pull request?
Reuse entry authorization state for parent-scope checks and per-object
list filtering in read-only REST requests. A scoped binding is opened by
the interceptor, bound only for read methods, reused only for the same
principal instance and metalake, and cleared when the request completes.
Filter workers receive the context explicitly; mutation operations
retain independent contexts.
### Why are the changes needed?
Separate contexts repeat user and role-version lookups during one list
request. Reusing the context lets filtering use state already loaded by
entry authorization.
Fix: #13251
### Does this PR introduce _any_ user-facing change?
No API or configuration changes. Authorization semantics remain
unchanged.
### How was this patch tested?
139 targeted unit tests passed across server-common, server, and
iceberg-rest-server. Tests cover context reuse through REST
interceptors, parallel filtering with table denies, security-context
isolation, exception cleanup, and JCasbin SQL-prefetch reuse with
revalidation on the next request. Ran Spotless on the changed modules.
---
.../authorization/AuthorizationRequestContext.java | 8 +-
.../authorization/AuthorizationRequestScope.java | 117 +++++++++++++++++
.../server/authorization/MetadataAuthzHelper.java | 74 ++++++-----
...BaseMetadataAuthorizationMethodInterceptor.java | 12 ++
.../TestAuthorizationRequestScope.java | 90 +++++++++++++
.../jcasbin/TestJcasbinAuthorizer.java | 32 +++++
...BaseMetadataAuthorizationMethodInterceptor.java | 143 +++++++++++++++++++++
.../web/filter/GravitinoInterceptionService.java | 4 +-
.../filter/TestGravitinoInterceptionService.java | 50 +++++++
9 files changed, 493 insertions(+), 37 deletions(-)
diff --git
a/core/src/main/java/org/apache/gravitino/authorization/AuthorizationRequestContext.java
b/core/src/main/java/org/apache/gravitino/authorization/AuthorizationRequestContext.java
index dc30ae58d0..76c4b2fef7 100644
---
a/core/src/main/java/org/apache/gravitino/authorization/AuthorizationRequestContext.java
+++
b/core/src/main/java/org/apache/gravitino/authorization/AuthorizationRequestContext.java
@@ -51,9 +51,11 @@ import org.apache.gravitino.utils.PrincipalUtils;
* <li>per-request role loading happens at most once via {@link
#loadRole(Runnable)}.
* </ul>
*
- * <p>Instances are not intended to outlive a request and are not reusable
across threads beyond the
- * request handling thread; the internal maps are {@link ConcurrentHashMap}
purely to tolerate any
- * incidental fan-out (e.g. async listeners) within the same request scope.
+ * <p>Instances must not outlive a request or be reused across principals,
active-role selections or
+ * metalakes. Entry authorization and list filtering of one read-only request
may share an instance:
+ * list workers receive it explicitly, which is why the internal maps are
{@link ConcurrentHashMap}.
+ * Role selection must be fixed before workers start, and a mutation must not
reuse decisions made
+ * before it.
*/
public class AuthorizationRequestContext {
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/authorization/AuthorizationRequestScope.java
b/server-common/src/main/java/org/apache/gravitino/server/authorization/AuthorizationRequestScope.java
new file mode 100644
index 0000000000..c309516f52
--- /dev/null
+++
b/server-common/src/main/java/org/apache/gravitino/server/authorization/AuthorizationRequestScope.java
@@ -0,0 +1,117 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.server.authorization;
+
+import java.lang.reflect.Method;
+import java.security.Principal;
+import java.util.Objects;
+import javax.annotation.Nullable;
+import javax.ws.rs.GET;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.authorization.AuthorizationRequestContext;
+import org.apache.gravitino.utils.PrincipalUtils;
+
+/**
+ * Makes entry authorization state available to list filtering during a
synchronous read request.
+ *
+ * <p>The interceptor opens a scope around the REST method, binds the context
it built for entry
+ * authorization when the method is a read, and closes the scope when the
method returns. {@link
+ * #getOrCreate(String)} then hands that context to {@code
MetadataAuthzHelper.filterByExpression}
+ * on the same thread. Filter workers receive the context explicitly; this
thread-local scope is not
+ * inherited by them.
+ *
+ * <p>This is separate from {@link org.apache.gravitino.utils.RequestContext}
because it is bound to
+ * the intercepted method, not to the servlet request, and is closed together
with it.
+ */
+public final class AuthorizationRequestScope implements AutoCloseable {
+ private static final ThreadLocal<AuthorizationRequestScope> CURRENT = new
ThreadLocal<>();
+
+ @Nullable private Principal principal;
+ @Nullable private String metalake;
+ @Nullable private AuthorizationRequestContext context;
+
+ private AuthorizationRequestScope() {}
+
+ /**
+ * Opens the scope of one intercepted invocation, to be closed on the same
thread with
+ * try-with-resources.
+ *
+ * @return the new scope
+ */
+ public static AuthorizationRequestScope open() {
+ AuthorizationRequestScope scope = new AuthorizationRequestScope();
+ CURRENT.set(scope);
+ return scope;
+ }
+
+ /**
+ * Binds completed entry authorization to this scope when the method is a
read operation. Only
+ * reads may reuse entry decisions, because a mutation could invalidate them
before the list is
+ * filtered. Nothing is bound for other methods or when no metalake was
authorized.
+ *
+ * @param method the intercepted REST method
+ * @param metalakeIdent the authorized metalake, or null when entry
authorization had none
+ * @param context the entry authorization context
+ */
+ public void bindIfRead(
+ Method method, @Nullable NameIdentifier metalakeIdent,
AuthorizationRequestContext context) {
+ if (metalakeIdent != null && method.isAnnotationPresent(GET.class)) {
+ bind(metalakeIdent.name(), context);
+ }
+ }
+
+ /**
+ * Binds completed entry authorization for a read-only operation to this
scope.
+ *
+ * @param metalake the authorized metalake
+ * @param context the entry authorization context
+ */
+ public void bind(String metalake, AuthorizationRequestContext context) {
+ this.principal = PrincipalUtils.getCurrentPrincipal();
+ this.metalake = Objects.requireNonNull(metalake, "metalake");
+ this.context = Objects.requireNonNull(context, "context");
+ }
+
+ /**
+ * Returns the bound entry context when it was built for the current
principal instance and the
+ * given metalake, otherwise a fresh context. Principal identity is compared
on purpose: a context
+ * snapshots the principal's active roles when it is created, and principal
equality may ignore
+ * those.
+ *
+ * @param metalake the metalake being filtered
+ * @return the matching request context, or a new independent context
+ */
+ public static AuthorizationRequestContext getOrCreate(String metalake) {
+ AuthorizationRequestScope scope = CURRENT.get();
+ if (scope != null
+ && scope.context != null
+ && scope.principal == PrincipalUtils.getCurrentPrincipal()
+ && Objects.equals(scope.metalake, metalake)) {
+ return scope.context;
+ }
+ return new AuthorizationRequestContext();
+ }
+
+ /** Removes the scope when the invocation completes. */
+ @Override
+ public void close() {
+ CURRENT.remove();
+ }
+}
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 0499fed142..3fd021ba75 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
@@ -291,8 +291,10 @@ public class MetadataAuthzHelper {
String metalake,
String expression,
Entity.EntityType entityType,
- NameIdentifier[] nameIdentifiers) {
- Principal principal = PrincipalUtils.getCurrentPrincipal();
+ NameIdentifier[] nameIdentifiers,
+ Principal principal,
+ GravitinoAuthorizer authorizer,
+ AuthorizationRequestContext requestContext) {
Map<String, List<ParentScopeAccessPath>> entityShortCircuits =
LIST_SHORT_CIRCUITS.get(entityType);
List<ParentScopeAccessPath> accessPaths =
@@ -327,9 +329,6 @@ public class MetadataAuthzHelper {
}
}
- GravitinoAuthorizer authorizer =
- GravitinoAuthorizerProvider.getInstance().getGravitinoAuthorizer();
- AuthorizationRequestContext requestContext = new
AuthorizationRequestContext();
Map<Entity.EntityType, NameIdentifier> metadataNames =
NameIdentifierUtil.splitNameIdentifier(metalake, entityType,
nameIdentifiers[0]);
@@ -415,45 +414,54 @@ public class MetadataAuthzHelper {
// per-object loop over every catalog in the metalake.
NameIdentifier[] nameIdentifiers =
Arrays.stream(entities).map(toNameIdentifier).toArray(NameIdentifier[]::new);
- if (enableAuthorization() && nameIdentifiers.length > 0) {
- if (METADATA_OBJECT_ENTITY_TYPES.contains(entityType)) {
- Arrays.stream(nameIdentifiers)
- .forEach(
- identifier ->
NameIdentifierUtil.checkMetadataObjectName(identifier, entityType));
- }
+ if (!enableAuthorization() || nameIdentifiers.length == 0) {
+ return entities;
+ }
+ if (METADATA_OBJECT_ENTITY_TYPES.contains(entityType)) {
+ Arrays.stream(nameIdentifiers)
+ .forEach(
+ identifier ->
NameIdentifierUtil.checkMetadataObjectName(identifier, entityType));
+ }
- 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;
- }
+ Principal principal = PrincipalUtils.getCurrentPrincipal();
+ GravitinoAuthorizer authorizer =
+ GravitinoAuthorizerProvider.getInstance().getGravitinoAuthorizer();
+ AuthorizationRequestContext authorizationRequestContext =
+ AuthorizationRequestScope.getOrCreate(metalake);
+ if (allVisibleViaParentScope(
+ metalake,
+ expression,
+ entityType,
+ nameIdentifiers,
+ principal,
+ authorizer,
+ authorizationRequestContext)) {
+ // 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 MISS for principal {}, entity type
{} under metalake "
- + "{} ({} object(s)); falling back to the per-object
authorization loop.",
- principalName,
+ "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.",
+ principal.getName(),
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.",
+ principal.getName(),
+ entityType,
+ metalake,
+ nameIdentifiers.length);
preloadToCache(entityType, nameIdentifiers);
- GravitinoAuthorizer authorizer =
- GravitinoAuthorizerProvider.getInstance().getGravitinoAuthorizer();
- AuthorizationRequestContext authorizationRequestContext = new
AuthorizationRequestContext();
return doFilter(
expression,
entities,
- PrincipalUtils.getCurrentPrincipal(),
+ principal,
authorizer,
authorizationRequestContext,
(entity) -> {
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/web/filter/BaseMetadataAuthorizationMethodInterceptor.java
b/server-common/src/main/java/org/apache/gravitino/server/web/filter/BaseMetadataAuthorizationMethodInterceptor.java
index dd22014fb1..3d76832d86 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/web/filter/BaseMetadataAuthorizationMethodInterceptor.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/web/filter/BaseMetadataAuthorizationMethodInterceptor.java
@@ -31,6 +31,7 @@ import org.apache.gravitino.auth.ActiveRoles;
import org.apache.gravitino.authorization.AuthorizationRequestContext;
import org.apache.gravitino.authorization.AuthorizationUtils;
import org.apache.gravitino.exceptions.ForbiddenException;
+import org.apache.gravitino.server.authorization.AuthorizationRequestScope;
import org.apache.gravitino.server.authorization.GravitinoAuthorizerProvider;
import
org.apache.gravitino.server.authorization.annotations.AuthorizationExpression;
import
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionEvaluator;
@@ -222,6 +223,14 @@ public abstract class
BaseMetadataAuthorizationMethodInterceptor {
*/
protected final Object authorizeMethod(Method method, Object[] args,
MethodInvoker methodInvoker)
throws Throwable {
+ try (AuthorizationRequestScope scope = AuthorizationRequestScope.open()) {
+ return authorizeMethodInScope(method, args, methodInvoker, scope);
+ }
+ }
+
+ private Object authorizeMethodInScope(
+ Method method, Object[] args, MethodInvoker methodInvoker,
AuthorizationRequestScope scope)
+ throws Throwable {
try {
Parameter[] parameters = method.getParameters();
AuthorizationExpression expressionAnnotation =
@@ -316,6 +325,9 @@ public abstract class
BaseMetadataAuthorizationMethodInterceptor {
throw new ForbiddenException(notAuthzMessage);
}
}
+ // A skipped standard check authorized nothing that list filtering
could reuse.
+ scope.bindIfRead(
+ method, skipStandardCheck ? null : metalakeIdent,
authorizationRequestContext);
}
} catch (Exception ex) {
if (ex instanceof ForbiddenException || isExceptionPropagate(ex)) {
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/authorization/TestAuthorizationRequestScope.java
b/server-common/src/test/java/org/apache/gravitino/server/authorization/TestAuthorizationRequestScope.java
new file mode 100644
index 0000000000..5220b3485f
--- /dev/null
+++
b/server-common/src/test/java/org/apache/gravitino/server/authorization/TestAuthorizationRequestScope.java
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.server.authorization;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotSame;
+import static org.junit.jupiter.api.Assertions.assertSame;
+
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import org.apache.gravitino.UserPrincipal;
+import org.apache.gravitino.auth.ActiveRoles;
+import org.apache.gravitino.authorization.AuthorizationRequestContext;
+import org.apache.gravitino.utils.PrincipalUtils;
+import org.junit.jupiter.api.Test;
+
+/** Tests security boundaries and lifetime of read-request context reuse. */
+public class TestAuthorizationRequestScope {
+ /** A scope must not share role loading or cached decisions across security
identities. */
+ @Test
+ public void testSecurityBoundaries() throws Exception {
+ UserPrincipal principal = new UserPrincipal("tester");
+ PrincipalUtils.doAs(
+ principal,
+ () -> {
+ AuthorizationRequestContext context = new
AuthorizationRequestContext();
+ try (AuthorizationRequestScope scope =
AuthorizationRequestScope.open()) {
+ scope.bind("metalake", context);
+ assertSame(context,
AuthorizationRequestScope.getOrCreate("metalake"));
+ assertNotSame(context,
AuthorizationRequestScope.getOrCreate("other"));
+ PrincipalUtils.doAs(
+ new UserPrincipal("other"),
+ () -> {
+ assertNotSame(context,
AuthorizationRequestScope.getOrCreate("metalake"));
+ return null;
+ });
+ // The same user with other active roles is an equal but distinct
principal instance,
+ // and must get a context that carries its own roles.
+ UserPrincipal assumed =
principal.withActiveRoles(ActiveRoles.of(List.of("reader")));
+ PrincipalUtils.doAs(
+ assumed,
+ () -> {
+ AuthorizationRequestContext isolated =
+ AuthorizationRequestScope.getOrCreate("metalake");
+ assertNotSame(context, isolated);
+ assertEquals(assumed.getActiveRoles(),
isolated.getActiveRoles());
+ return null;
+ });
+ }
+ assertNotSame(context,
AuthorizationRequestScope.getOrCreate("metalake"));
+ return null;
+ });
+ }
+
+ /** Asynchronous work must not inherit the bound context, and a closed scope
leaves nothing. */
+ @Test
+ public void testWorkerIsolationAndCleanup() throws Exception {
+ PrincipalUtils.doAs(
+ new UserPrincipal("tester"),
+ () -> {
+ AuthorizationRequestContext context = new
AuthorizationRequestContext();
+ try (AuthorizationRequestScope scope =
AuthorizationRequestScope.open()) {
+ scope.bind("metalake", context);
+ assertSame(context,
AuthorizationRequestScope.getOrCreate("metalake"));
+ assertNotSame(
+ context,
+ CompletableFuture.supplyAsync(
+ () ->
AuthorizationRequestScope.getOrCreate("metalake"))
+ .join());
+ }
+ assertNotSame(context,
AuthorizationRequestScope.getOrCreate("metalake"));
+ return null;
+ });
+ }
+}
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 9316cdad38..f47e925bdd 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
@@ -23,6 +23,7 @@ import static
org.apache.gravitino.authorization.Privilege.Name.USE_SCHEMA;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
@@ -84,6 +85,7 @@ import org.apache.gravitino.meta.RoleEntity;
import org.apache.gravitino.meta.SchemaVersion;
import org.apache.gravitino.meta.UserEntity;
import org.apache.gravitino.server.ServerConfig;
+import org.apache.gravitino.server.authorization.AuthorizationRequestScope;
import org.apache.gravitino.server.authorization.MetadataIdConverter;
import org.apache.gravitino.storage.relational.mapper.EntityChangeLogMapper;
import org.apache.gravitino.storage.relational.mapper.GroupMetaMapper;
@@ -891,6 +893,36 @@ public class TestJcasbinAuthorizer {
assertFalse(doAuthorizeOwner(currentPrincipal));
}
+ /** Reusing entry state avoids another SQL prefetch even for a different
privilege check. */
+ @Test
+ public void testReadScopeReusesEntryRolePrefetch() throws Exception {
+ Principal principal = PrincipalUtils.getCurrentPrincipal();
+ RoleEntity role =
+ mockRoleInStore(ALLOW_ROLE_ID, "allowRole",
ImmutableList.of(getAllowSecurableObject()));
+ mockDirectUserRoles(role);
+ MetadataObject catalog = MetadataObjects.of(null, "testCatalog",
MetadataObject.Type.CATALOG);
+ AuthorizationRequestContext entryContext = new
AuthorizationRequestContext();
+ assertTrue(
+ jcasbinAuthorizer.authorize(principal, METALAKE, catalog, USE_CATALOG,
entryContext));
+ Mockito.clearInvocations(userMetaMapper, roleMetaMapper);
+
+ try (AuthorizationRequestScope scope = AuthorizationRequestScope.open()) {
+ scope.bind(METALAKE, entryContext);
+ AuthorizationRequestContext filterContext =
AuthorizationRequestScope.getOrCreate(METALAKE);
+ assertSame(entryContext, filterContext);
+ assertFalse(
+ jcasbinAuthorizer.authorize(principal, METALAKE, catalog,
SELECT_TABLE, filterContext));
+ verify(userMetaMapper, Mockito.never())
+ .batchGetAuthSubjectsForUser(anyString(), anyString(), anyList());
+ verify(roleMetaMapper, Mockito.never()).batchGetRoleUpdatedAt(any());
+ }
+
+ // A subsequent request must revalidate SQL versions, even with warm
shared role caches.
+ AuthorizationRequestContext nextContext =
AuthorizationRequestScope.getOrCreate(METALAKE);
+ assertTrue(jcasbinAuthorizer.authorize(principal, METALAKE, catalog,
USE_CATALOG, nextContext));
+ verify(userMetaMapper).batchGetAuthSubjectsForUser(eq(METALAKE),
eq(USERNAME), anyList());
+ }
+
@Test
public void testPrefetchRunsAfterOwnerUserInfoLookup() throws Exception {
Principal currentPrincipal = PrincipalUtils.getCurrentPrincipal();
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/web/filter/TestBaseMetadataAuthorizationMethodInterceptor.java
b/server-common/src/test/java/org/apache/gravitino/server/web/filter/TestBaseMetadataAuthorizationMethodInterceptor.java
index de3afcb917..d100e1d405 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/web/filter/TestBaseMetadataAuthorizationMethodInterceptor.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/web/filter/TestBaseMetadataAuthorizationMethodInterceptor.java
@@ -19,8 +19,10 @@
package org.apache.gravitino.server.web.filter;
import static
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants.CAN_ACCESS_METADATA;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.assertNotSame;
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
@@ -38,7 +40,17 @@ import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Function;
+import javax.ws.rs.GET;
+import javax.ws.rs.POST;
+import org.apache.gravitino.Config;
+import org.apache.gravitino.Configs;
import org.apache.gravitino.Entity;
+import org.apache.gravitino.GravitinoEnv;
import org.apache.gravitino.MetadataObject;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.UserPrincipal;
@@ -48,13 +60,19 @@ import
org.apache.gravitino.authorization.AuthorizationUtils;
import org.apache.gravitino.authorization.GravitinoAuthorizer;
import org.apache.gravitino.authorization.Privilege;
import org.apache.gravitino.exceptions.ForbiddenException;
+import org.apache.gravitino.server.authorization.AuthorizationRequestScope;
import org.apache.gravitino.server.authorization.GravitinoAuthorizerProvider;
+import org.apache.gravitino.server.authorization.MetadataAuthzHelper;
import
org.apache.gravitino.server.authorization.annotations.AuthorizationExpression;
+import
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants;
+import org.apache.gravitino.storage.relational.po.auth.UserUpdatedAt;
import org.apache.gravitino.utils.NameIdentifierUtil;
import org.apache.gravitino.utils.PrincipalUtils;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
import org.mockito.MockedStatic;
/** Tests for {@link BaseMetadataAuthorizationMethodInterceptor}. */
@@ -295,6 +313,119 @@ public class
TestBaseMetadataAuthorizationMethodInterceptor {
assertSame(failure, interceptor.invoke(invocation));
}
+ /** Entry authorization, parent checks and worker filtering share one user
lookup. */
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ public void testReadListReusesEntryContext(boolean hasDeny) throws Throwable
{
+ AtomicInteger userLoads = new AtomicInteger();
+ Set<AuthorizationRequestContext> contexts = ConcurrentHashMap.newKeySet();
+ AtomicBoolean perObjectLoopRan = new AtomicBoolean();
+ AtomicReference<AuthorizationRequestContext> entryContext = new
AtomicReference<>();
+ Function<String, Optional<UserUpdatedAt>> loadUser =
+ key -> {
+ userLoads.incrementAndGet();
+ return Optional.of(new UserUpdatedAt(1L, 1L));
+ };
+ try (MockedStatic<GravitinoEnv> envStatic =
mockStatic(GravitinoEnv.class)) {
+ GravitinoEnv env = mock(GravitinoEnv.class);
+ Config config = mock(Config.class);
+ envStatic.when(GravitinoEnv::getInstance).thenReturn(env);
+ when(env.config()).thenReturn(config);
+ when(config.get(Configs.ENABLE_AUTHORIZATION)).thenReturn(true);
+
when(config.get(Configs.GRAVITINO_AUTHORIZATION_THREAD_POOL_SIZE)).thenReturn(2);
+ principalUtils.when(() -> PrincipalUtils.doAs(any(),
any())).thenCallRealMethod();
+ authorizationUtils
+ .when(() -> AuthorizationUtils.checkCurrentUser(any(), any(), any()))
+ .thenAnswer(
+ invocation -> {
+ AuthorizationRequestContext context =
invocation.getArgument(2);
+ entryContext.set(context);
+ context.computeUserInfoIfAbsent("metalake::tester", loadUser);
+ return null;
+ });
+ when(authorizer.authorize(any(), any(), any(), any(), any()))
+ .thenAnswer(
+ invocation -> {
+ AuthorizationRequestContext context =
invocation.getArgument(4);
+ contexts.add(context);
+ context.computeUserInfoIfAbsent("metalake::tester", loadUser);
+ return true;
+ });
+ when(authorizer.hasDenyPolicy(any(), any(), any(), any()))
+ .thenAnswer(
+ invocation -> {
+ contexts.add(invocation.getArgument(3));
+ return hasDeny;
+ });
+ when(authorizer.deny(any(), any(), any(), any(), any()))
+ .thenAnswer(
+ invocation -> {
+ MetadataObject object = invocation.getArgument(2);
+ contexts.add(invocation.getArgument(4));
+ if (object.type() == MetadataObject.Type.TABLE) {
+ perObjectLoopRan.set(true);
+ }
+ return hasDeny && object.name().equals("hidden");
+ });
+ NameIdentifier visible = NameIdentifier.of("metalake", "catalog",
"schema", "visible");
+ NameIdentifier hidden = NameIdentifier.of("metalake", "catalog",
"schema", "hidden");
+ TestInvocation invocation = invocation("listTables", null);
+ when(invocation.proceed())
+ .thenAnswer(
+ unused ->
+ MetadataAuthzHelper.filterByExpression(
+ "metalake",
+
AuthorizationExpressionConstants.FILTER_TABLE_AUTHORIZATION_EXPRESSION,
+ Entity.EntityType.TABLE,
+ new NameIdentifier[] {visible, hidden}));
+
+ Object result = new
TestInterceptor(Entity.EntityType.SCHEMA).invoke(invocation);
+
+ assertArrayEquals(
+ hasDeny ? new NameIdentifier[] {visible} : new NameIdentifier[]
{visible, hidden},
+ (NameIdentifier[]) result);
+ assertEquals(Set.of(entryContext.get()), contexts);
+ assertEquals(1, userLoads.get());
+ assertEquals(hasDeny, perObjectLoopRan.get());
+ assertNotSame(entryContext.get(),
AuthorizationRequestScope.getOrCreate("metalake"));
+ }
+ }
+
+ /** An endpoint failure must not retain a request's permission cache on a
reused thread. */
+ @Test
+ public void testReadContextIsClearedAfterOperationFailure() throws Throwable
{
+ when(authorizer.authorize(any(), any(), any(), any(),
any())).thenReturn(true);
+ AtomicReference<AuthorizationRequestContext> context = new
AtomicReference<>();
+ TestInvocation invocation = invocation("listTables", null);
+ IllegalStateException failure = new IllegalStateException("list failed");
+ when(invocation.proceed())
+ .thenAnswer(
+ unused -> {
+ context.set(AuthorizationRequestScope.getOrCreate("metalake"));
+ throw failure;
+ });
+ assertSame(failure, new
TestInterceptor(Entity.EntityType.SCHEMA).invoke(invocation));
+ assertNotSame(context.get(),
AuthorizationRequestScope.getOrCreate("metalake"));
+ }
+
+ /** Write operations must not expose pre-mutation authorization decisions to
later filtering. */
+ @Test
+ public void testWriteDoesNotReuseEntryContext() throws Throwable {
+ AtomicReference<AuthorizationRequestContext> entryContext = new
AtomicReference<>();
+ when(authorizer.authorize(any(), any(), any(), any(), any()))
+ .thenAnswer(
+ invocation -> {
+ entryContext.set(invocation.getArgument(4));
+ return true;
+ });
+ TestInvocation invocation = invocation("writeTables", null);
+ when(invocation.proceed())
+ .thenAnswer(unused ->
AuthorizationRequestScope.getOrCreate("metalake"));
+ Object result = new
TestInterceptor(Entity.EntityType.SCHEMA).invoke(invocation);
+ assertInstanceOf(AuthorizationRequestContext.class, result);
+ assertNotSame(entryContext.get(), result);
+ }
+
private static TestInvocation invocation(String methodName, Object result)
throws Throwable {
Method method = TestOperations.class.getDeclaredMethod(methodName);
TestInvocation invocation = mock(TestInvocation.class);
@@ -385,6 +516,18 @@ public class
TestBaseMetadataAuthorizationMethodInterceptor {
}
private static class TestOperations {
+ @GET
+ @AuthorizationExpression(expression = "CATALOG::USE_CATALOG")
+ private String listTables() {
+ return "unused";
+ }
+
+ @POST
+ @AuthorizationExpression(expression = "CATALOG::USE_CATALOG")
+ private String writeTables() {
+ return "unused";
+ }
+
@AuthorizationExpression(
expression = CAN_ACCESS_METADATA,
accessMetadataType = MetadataObject.Type.METALAKE)
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java
b/server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java
index 1cd0306af4..1fd90c407c 100644
---
a/server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java
+++
b/server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java
@@ -51,6 +51,7 @@ import
org.apache.gravitino.exceptions.IllegalNameIdentifierException;
import org.apache.gravitino.exceptions.NoSuchMetalakeException;
import org.apache.gravitino.lineage.source.rest.LineageOperations;
import
org.apache.gravitino.listener.api.event.server.AuthorizationDenialFailureEvent;
+import org.apache.gravitino.server.authorization.AuthorizationRequestScope;
import org.apache.gravitino.server.authorization.GravitinoAuthorizerProvider;
import
org.apache.gravitino.server.authorization.annotations.AuthorizationExpression;
import
org.apache.gravitino.server.authorization.annotations.AuthorizationRequest;
@@ -162,7 +163,7 @@ public class GravitinoInterceptionService implements
InterceptionService {
AuthorizationExpression expressionAnnotation =
method.getAnnotation(AuthorizationExpression.class);
- try {
+ try (AuthorizationRequestScope scope = AuthorizationRequestScope.open())
{
AuthorizationExecutor executor = null;
if (expressionAnnotation != null) {
String expression = expressionAnnotation.expression();
@@ -270,6 +271,7 @@ public class GravitinoInterceptionService implements
InterceptionService {
expressionAnnotation, metadataContext, method,
evaluatedExpression);
}
}
+ scope.bindIfRead(method, metalakeIdent, authorizationRequestContext);
}
return methodInvocation.proceed();
} catch (IllegalMetadataObjectException ex) {
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java
b/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java
index 35efcd1cfb..0044367ab1 100644
---
a/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java
+++
b/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java
@@ -23,6 +23,7 @@ import static
org.apache.gravitino.server.authorization.expression.Authorization
import static
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants.TEST_CATALOG_CONNECTION_WITH_CHANGES_AUTHORIZATION_EXPRESSION;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.mockStatic;
import static org.mockito.Mockito.never;
@@ -37,6 +38,7 @@ import java.security.Principal;
import java.util.Collections;
import java.util.List;
import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.atomic.AtomicReference;
import javax.servlet.http.HttpServletRequest;
import javax.ws.rs.core.Response;
import org.aopalliance.intercept.MethodInterceptor;
@@ -67,6 +69,7 @@ import org.apache.gravitino.json.JsonUtils;
import org.apache.gravitino.listener.EventBus;
import
org.apache.gravitino.listener.api.event.server.AuthorizationDenialFailureEvent;
import org.apache.gravitino.metalake.MetalakeManager;
+import org.apache.gravitino.server.authorization.AuthorizationRequestScope;
import org.apache.gravitino.server.authorization.GravitinoAuthorizerProvider;
import
org.apache.gravitino.server.authorization.annotations.AuthorizationExpression;
import
org.apache.gravitino.server.authorization.annotations.AuthorizationFullName;
@@ -78,6 +81,7 @@ import org.apache.gravitino.server.web.rest.CatalogOperations;
import org.apache.gravitino.server.web.rest.MetadataObjectTagOperations;
import org.apache.gravitino.server.web.rest.SchemaOperations;
import org.apache.gravitino.server.web.rest.SecretsProviderOperations;
+import org.apache.gravitino.server.web.rest.TableOperations;
import org.apache.gravitino.server.web.rest.ViewOperations;
import org.apache.gravitino.tag.TagDispatcher;
import org.apache.gravitino.utils.PrincipalUtils;
@@ -209,6 +213,52 @@ public class TestGravitinoInterceptionService {
}
}
+ /** The Gravitino list endpoint receives entry state and never leaks it into
the next request. */
+ @Test
+ public void testListTablesReusesEntryContextAndCleansUp() throws Throwable {
+ try (MockedStatic<PrincipalUtils> principals =
mockStatic(PrincipalUtils.class);
+ MockedStatic<GravitinoAuthorizerProvider> providers =
+ mockStatic(GravitinoAuthorizerProvider.class);
+ MockedStatic<AuthorizationUtils> authorization =
mockStatic(AuthorizationUtils.class)) {
+ UserPrincipal principal = new UserPrincipal("tester");
+
principals.when(PrincipalUtils::getCurrentPrincipal).thenReturn(principal);
+
principals.when(PrincipalUtils::getCurrentUserName).thenReturn(principal.getName());
+ GravitinoAuthorizer authorizer = mock(GravitinoAuthorizer.class);
+ GravitinoAuthorizerProvider provider =
mock(GravitinoAuthorizerProvider.class);
+
providers.when(GravitinoAuthorizerProvider::getInstance).thenReturn(provider);
+ when(provider.getGravitinoAuthorizer()).thenReturn(authorizer);
+ when(authorizer.isOwner(any(), any(), any(), any())).thenReturn(true);
+ AtomicReference<AuthorizationRequestContext> entry = new
AtomicReference<>();
+ authorization
+ .when(() -> AuthorizationUtils.checkCurrentUser(any(), any(), any()))
+ .thenAnswer(
+ call -> {
+ entry.set(call.getArgument(2));
+ return null;
+ });
+ Method method =
+ TableOperations.class.getMethod("listTables", String.class,
String.class, String.class);
+ MethodInvocation invocation = mock(MethodInvocation.class);
+ when(invocation.getMethod()).thenReturn(method);
+ when(invocation.getArguments()).thenReturn(new Object[] {"metalake",
"catalog", "schema"});
+ when(invocation.proceed())
+ .thenAnswer(unused ->
AuthorizationRequestScope.getOrCreate("metalake"));
+ MethodInterceptor interceptor =
+ new
GravitinoInterceptionService().getMethodInterceptors(method).get(0);
+ Object first = interceptor.invoke(invocation);
+ Assertions.assertSame(entry.get(), first);
+ Assertions.assertNotSame(first,
AuthorizationRequestScope.getOrCreate("metalake"));
+ Object second = interceptor.invoke(invocation);
+ Assertions.assertSame(entry.get(), second);
+ Assertions.assertNotSame(first, second);
+ doThrow(new IllegalStateException("list
failed")).when(invocation).proceed();
+ try (Response failure = (Response) interceptor.invoke(invocation)) {
+ assertEquals(500, failure.getStatus());
+ }
+ Assertions.assertNotSame(entry.get(),
AuthorizationRequestScope.getOrCreate("metalake"));
+ }
+ }
+
@Test
public void testMetadataAuthorizationMethodInterceptor() throws Throwable {
try (MockedStatic<PrincipalUtils> principalUtilsMocked =
mockStatic(PrincipalUtils.class);
