jerryshao commented on code in PR #11618:
URL: https://github.com/apache/gravitino/pull/11618#discussion_r3405430082
##########
server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java:
##########
@@ -250,6 +267,180 @@ public void testEmptyExpressionSkipsAuthorization()
throws Throwable {
}
}
+ /**
+ * When the authorization executor returns {@code false}, {@code
buildNoAuthResponse} is called
+ * with {@code emitEvent=true}. Verify that:
+ *
+ * <ul>
+ * <li>An {@link AuthorizationDenialFailureEvent} is dispatched via the
EventBus.
+ * <li>{@code RequestContext.operationFailureFired} is set so {@code
HttpAuditFilter} skips
+ * emitting a duplicate failure event for the same request.
+ * </ul>
+ */
+ @Test
+ public void testExecutorDenialDispatchesEventAndSetsFlag() throws Throwable {
+ try (MockedStatic<PrincipalUtils> principalUtilsMocked =
mockStatic(PrincipalUtils.class);
+ MockedStatic<GravitinoAuthorizerProvider> authorizerMocked =
+ mockStatic(GravitinoAuthorizerProvider.class);
+ MockedStatic<GravitinoEnv> envMocked = mockStatic(GravitinoEnv.class);
+ MockedStatic<MetalakeManager> metalakeManagerMocked =
mockStatic(MetalakeManager.class)) {
+
+ principalUtilsMocked
+ .when(PrincipalUtils::getCurrentPrincipal)
+ .thenReturn(new UserPrincipal("tester"));
+
principalUtilsMocked.when(PrincipalUtils::getCurrentUserName).thenReturn("tester");
+
+ GravitinoAuthorizerProvider mockedProvider =
mock(GravitinoAuthorizerProvider.class);
+
authorizerMocked.when(GravitinoAuthorizerProvider::getInstance).thenReturn(mockedProvider);
+ when(mockedProvider.getGravitinoAuthorizer()).thenReturn(new
MockGravitinoAuthorizer());
+
+ GravitinoEnv mockEnv = mock(GravitinoEnv.class);
+ EntityStore mockStore = mock(EntityStore.class);
+ EventBus mockEventBus = spy(new EventBus(Collections.emptyList()));
+ envMocked.when(GravitinoEnv::getInstance).thenReturn(mockEnv);
+ when(mockEnv.entityStore()).thenReturn(mockStore);
+ when(mockEnv.eventBus()).thenReturn(mockEventBus);
+
+ metalakeManagerMocked
+ .when(() -> MetalakeManager.checkMetalake(ArgumentMatchers.any(),
ArgumentMatchers.any()))
+ .thenAnswer(invocation -> null);
+
+ GravitinoInterceptionService service = new
GravitinoInterceptionService();
+ Method testMethod = TestOperations.class.getMethods()[0];
+ MethodInterceptor interceptor =
service.getMethodInterceptors(testMethod).get(0);
+
+ MethodInvocation invocation = mock(MethodInvocation.class);
+ when(invocation.getMethod()).thenReturn(testMethod);
+ // testMetalake2 triggers denial in MockGravitinoAuthorizer
+ when(invocation.getArguments()).thenReturn(new Object[]
{"testMetalake2"});
+
+ Assertions.assertFalse(RequestContext.isOperationFailureFired());
+ Response response = (Response) interceptor.invoke(invocation);
+
+ assertEquals(Response.Status.FORBIDDEN.getStatusCode(),
response.getStatus());
+ // Event dispatched
+ ArgumentCaptor<org.apache.gravitino.listener.api.event.Event> captor =
+
ArgumentCaptor.forClass(org.apache.gravitino.listener.api.event.Event.class);
+ verify(mockEventBus).dispatchEvent(captor.capture());
+ Assertions.assertInstanceOf(AuthorizationDenialFailureEvent.class,
captor.getValue());
+ AuthorizationDenialFailureEvent event =
(AuthorizationDenialFailureEvent) captor.getValue();
+ assertEquals("tester", event.user());
Review Comment:
Fixed — both captors are now typed directly to
`ArgumentCaptor<AuthorizationDenialFailureEvent>`, removing the FQN and the
redundant `assertInstanceOf` + cast.
##########
server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java:
##########
@@ -250,6 +267,180 @@ public void testEmptyExpressionSkipsAuthorization()
throws Throwable {
}
}
+ /**
+ * When the authorization executor returns {@code false}, {@code
buildNoAuthResponse} is called
+ * with {@code emitEvent=true}. Verify that:
+ *
+ * <ul>
+ * <li>An {@link AuthorizationDenialFailureEvent} is dispatched via the
EventBus.
+ * <li>{@code RequestContext.operationFailureFired} is set so {@code
HttpAuditFilter} skips
+ * emitting a duplicate failure event for the same request.
+ * </ul>
+ */
+ @Test
+ public void testExecutorDenialDispatchesEventAndSetsFlag() throws Throwable {
+ try (MockedStatic<PrincipalUtils> principalUtilsMocked =
mockStatic(PrincipalUtils.class);
+ MockedStatic<GravitinoAuthorizerProvider> authorizerMocked =
+ mockStatic(GravitinoAuthorizerProvider.class);
+ MockedStatic<GravitinoEnv> envMocked = mockStatic(GravitinoEnv.class);
+ MockedStatic<MetalakeManager> metalakeManagerMocked =
mockStatic(MetalakeManager.class)) {
+
+ principalUtilsMocked
+ .when(PrincipalUtils::getCurrentPrincipal)
+ .thenReturn(new UserPrincipal("tester"));
+
principalUtilsMocked.when(PrincipalUtils::getCurrentUserName).thenReturn("tester");
+
+ GravitinoAuthorizerProvider mockedProvider =
mock(GravitinoAuthorizerProvider.class);
+
authorizerMocked.when(GravitinoAuthorizerProvider::getInstance).thenReturn(mockedProvider);
+ when(mockedProvider.getGravitinoAuthorizer()).thenReturn(new
MockGravitinoAuthorizer());
+
+ GravitinoEnv mockEnv = mock(GravitinoEnv.class);
+ EntityStore mockStore = mock(EntityStore.class);
+ EventBus mockEventBus = spy(new EventBus(Collections.emptyList()));
+ envMocked.when(GravitinoEnv::getInstance).thenReturn(mockEnv);
+ when(mockEnv.entityStore()).thenReturn(mockStore);
+ when(mockEnv.eventBus()).thenReturn(mockEventBus);
+
+ metalakeManagerMocked
+ .when(() -> MetalakeManager.checkMetalake(ArgumentMatchers.any(),
ArgumentMatchers.any()))
+ .thenAnswer(invocation -> null);
+
+ GravitinoInterceptionService service = new
GravitinoInterceptionService();
+ Method testMethod = TestOperations.class.getMethods()[0];
+ MethodInterceptor interceptor =
service.getMethodInterceptors(testMethod).get(0);
+
+ MethodInvocation invocation = mock(MethodInvocation.class);
+ when(invocation.getMethod()).thenReturn(testMethod);
+ // testMetalake2 triggers denial in MockGravitinoAuthorizer
+ when(invocation.getArguments()).thenReturn(new Object[]
{"testMetalake2"});
+
+ Assertions.assertFalse(RequestContext.isOperationFailureFired());
+ Response response = (Response) interceptor.invoke(invocation);
+
+ assertEquals(Response.Status.FORBIDDEN.getStatusCode(),
response.getStatus());
+ // Event dispatched
+ ArgumentCaptor<org.apache.gravitino.listener.api.event.Event> captor =
+
ArgumentCaptor.forClass(org.apache.gravitino.listener.api.event.Event.class);
+ verify(mockEventBus).dispatchEvent(captor.capture());
+ Assertions.assertInstanceOf(AuthorizationDenialFailureEvent.class,
captor.getValue());
+ AuthorizationDenialFailureEvent event =
(AuthorizationDenialFailureEvent) captor.getValue();
+ assertEquals("tester", event.user());
+ assertEquals("testMethod", event.methodName());
+ // Flag set so HttpAuditFilter skips duplicate
+ Assertions.assertTrue(RequestContext.isOperationFailureFired());
+ }
+ }
+
+ /**
+ * When {@code checkCurrentUser} throws {@link ForbiddenException} (user is
not a metalake
+ * member), the interceptor must dispatch an {@link
AuthorizationDenialFailureEvent} and set
+ * {@code operationFailureFired}.
+ */
+ @Test
+ public void testForbiddenExceptionDispatchesEventAndSetsFlag() throws
Throwable {
+ try (MockedStatic<PrincipalUtils> principalUtilsMocked =
mockStatic(PrincipalUtils.class);
+ MockedStatic<GravitinoAuthorizerProvider> authorizerMocked =
+ mockStatic(GravitinoAuthorizerProvider.class);
+ MockedStatic<AuthorizationUtils> authUtilsMocked =
mockStatic(AuthorizationUtils.class);
+ MockedStatic<GravitinoEnv> envMocked = mockStatic(GravitinoEnv.class))
{
+
+ principalUtilsMocked
+ .when(PrincipalUtils::getCurrentPrincipal)
+ .thenReturn(new UserPrincipal("outsider"));
+
principalUtilsMocked.when(PrincipalUtils::getCurrentUserName).thenReturn("outsider");
+
+ GravitinoAuthorizerProvider mockedProvider =
mock(GravitinoAuthorizerProvider.class);
+
authorizerMocked.when(GravitinoAuthorizerProvider::getInstance).thenReturn(mockedProvider);
+ when(mockedProvider.getGravitinoAuthorizer()).thenReturn(new
MockGravitinoAuthorizer());
+
+ authUtilsMocked
+ .when(
+ () ->
+ AuthorizationUtils.checkCurrentUser(
+ ArgumentMatchers.any(), ArgumentMatchers.any(),
ArgumentMatchers.any()))
+ .thenThrow(new ForbiddenException("User outsider is not a member"));
+
+ GravitinoEnv mockEnv = mock(GravitinoEnv.class);
+ EventBus mockEventBus = spy(new EventBus(Collections.emptyList()));
+ envMocked.when(GravitinoEnv::getInstance).thenReturn(mockEnv);
+ when(mockEnv.eventBus()).thenReturn(mockEventBus);
+
+ GravitinoInterceptionService service = new
GravitinoInterceptionService();
+ Method testMethod = TestOperations.class.getMethods()[0];
+ MethodInterceptor interceptor =
service.getMethodInterceptors(testMethod).get(0);
+
+ MethodInvocation invocation = mock(MethodInvocation.class);
+ when(invocation.getMethod()).thenReturn(testMethod);
+ when(invocation.getArguments()).thenReturn(new Object[]
{"testMetalake"});
+
+ Assertions.assertFalse(RequestContext.isOperationFailureFired());
+ Response response = (Response) interceptor.invoke(invocation);
+
+ assertEquals(Response.Status.FORBIDDEN.getStatusCode(),
response.getStatus());
+ ArgumentCaptor<org.apache.gravitino.listener.api.event.Event> captor =
+
ArgumentCaptor.forClass(org.apache.gravitino.listener.api.event.Event.class);
+ verify(mockEventBus).dispatchEvent(captor.capture());
+ Assertions.assertInstanceOf(AuthorizationDenialFailureEvent.class,
captor.getValue());
+ AuthorizationDenialFailureEvent event =
(AuthorizationDenialFailureEvent) captor.getValue();
+ assertEquals("outsider", event.user());
Review Comment:
Fixed — same as above.
##########
core/src/main/java/org/apache/gravitino/listener/api/event/server/AuthorizationDenialFailureEvent.java:
##########
@@ -0,0 +1,142 @@
+/*
+ * 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.listener.api.event.server;
+
+import com.google.common.collect.ImmutableMap;
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.annotation.DeveloperApi;
+import org.apache.gravitino.exceptions.ForbiddenException;
+import org.apache.gravitino.listener.api.event.EventSource;
+import org.apache.gravitino.listener.api.event.FailureEvent;
+import org.apache.gravitino.listener.api.event.OperationType;
+
+/**
+ * Represents an authorization denial that occurred inside {@code
+ * MetadataAuthorizationMethodInterceptor}: the request was authenticated but
the authorization
+ * executor returned {@code false} for the requested operation, or user
validation raised a {@link
+ * ForbiddenException}.
+ *
+ * <p>This event carries rich context about the denial — the authenticated
user, the resource
+ * identifier, the Java method name, and the authorization expression — so
auditors can reconstruct
+ * exactly what was attempted and why it was refused.
+ *
+ * <p>Compared to a generic {@link HttpRequestFailureEvent} with {@code
http.status=403}, this event
+ * is distinguished by {@link OperationType#AUTHORIZATION_DENIAL} and by the
non-null {@link
+ * #identifier()} and {@link #customInfo()} fields. Dispatching this event
before returning the 403
+ * response also sets the {@code RequestContext.operationFailureFired} flag,
preventing {@code
+ * HttpAuditFilter} from emitting a duplicate HTTP-level event for the same
request.
+ */
+@DeveloperApi
+public final class AuthorizationDenialFailureEvent extends FailureEvent {
+
+ private final String methodName;
+ private final String expression;
+ private final EventSource explicitEventSource;
+
+ /**
+ * Constructs an {@code AuthorizationDenialFailureEvent} attributed to the
main Gravitino server.
+ *
+ * @param user the authenticated user whose request was denied.
+ * @param accessMetadataName the {@link NameIdentifier} of the resource the
user attempted to
+ * access. May be {@code null} when the denial occurs at the
metalake-user validation level
+ * before a specific resource is resolved.
+ * @param methodName the name of the intercepted Java method (e.g. {@code
"loadTable"}).
+ * @param expression the authorization expression that was evaluated (e.g.
{@code "TABLE:LOAD"}).
+ * May be empty when the denial occurs before the expression is
extracted.
+ */
+ public AuthorizationDenialFailureEvent(
+ String user,
+ @Nullable NameIdentifier accessMetadataName,
+ String methodName,
+ String expression) {
+ this(user, accessMetadataName, methodName, expression,
EventSource.GRAVITINO_SERVER);
+ }
+
+ /**
+ * Constructs an {@code AuthorizationDenialFailureEvent} with an explicit
{@link EventSource}. Use
+ * this overload when the denial originates from a server other than the
main Gravitino server
+ * (e.g. Iceberg REST, Lance REST).
+ *
+ * @param user the authenticated user whose request was denied.
+ * @param accessMetadataName the {@link NameIdentifier} of the resource the
user attempted to
+ * access. May be {@code null}.
+ * @param methodName the name of the intercepted Java method.
+ * @param expression the authorization expression that was evaluated.
+ * @param eventSource identifies which server produced this denial event.
+ */
+ public AuthorizationDenialFailureEvent(
+ String user,
+ @Nullable NameIdentifier accessMetadataName,
+ String methodName,
+ String expression,
+ EventSource eventSource) {
+ super(
+ user,
+ accessMetadataName,
+ new ForbiddenException(
+ "Authorization denied for user '%s' on operation '%s'", user,
methodName));
+ this.methodName = methodName;
+ this.expression = expression != null ? expression : "";
+ this.explicitEventSource = eventSource;
+ }
+
+ /** Returns {@link OperationType#AUTHORIZATION_DENIAL}. */
Review Comment:
Fixed — replaced `ForbiddenException` with a lightweight
`AuthorizationDenialException` inner class (modelled after
`HttpRequestException` in `HttpRequestFailureEvent`) that suppresses stack
trace collection via the 4-arg `RuntimeException` constructor. Authorization
denials can be high-volume so the overhead saving is meaningful.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
