Copilot commented on code in PR #11618: URL: https://github.com/apache/gravitino/pull/11618#discussion_r3401223862
########## server-common/src/main/java/org/apache/gravitino/server/web/HttpAuditFilter.java: ########## @@ -0,0 +1,275 @@ +/* + * 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.web; + +import java.io.IOException; +import java.security.Principal; +import javax.annotation.Nullable; +import javax.servlet.Filter; +import javax.servlet.FilterChain; +import javax.servlet.FilterConfig; +import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import javax.servlet.http.HttpServletResponseWrapper; +import org.apache.commons.lang3.StringUtils; +import org.apache.gravitino.auth.AuthConstants; +import org.apache.gravitino.listener.EventBus; +import org.apache.gravitino.listener.api.event.EventSource; +import org.apache.gravitino.listener.api.event.server.HttpRequestFailureEvent; +import org.apache.gravitino.utils.RequestContext; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * A servlet filter that emits an {@link HttpRequestFailureEvent} for every HTTP request that + * completes with a 4xx or 5xx status code and for which no operation-layer failure event was + * already dispatched on the same request thread. + * + * <p><strong>Filter chain position:</strong> this filter must be registered <em>after</em> {@link + * RequestContextFilter} (so the remote address is already populated) but <em>before</em> {@link + * org.apache.gravitino.server.authentication.AuthenticationFilter} (so it observes 401 + * authentication failures as well as downstream 403 authorization denials). It is safe to install + * on servers that do not use {@code RequestContextFilter} (Iceberg REST, Lance REST) because it + * resolves the client address independently. + * + * <p><strong>Double-logging prevention:</strong> when an operation-layer failure event (e.g. {@code + * LoadTableFailureEvent}, {@code AuthorizationDenialFailureEvent}) has already been dispatched via + * {@link EventBus}, {@link RequestContext#markOperationFailureFired()} is set on the request + * thread. This filter checks that flag in the {@code finally} block and skips emitting its own + * {@link HttpRequestFailureEvent} if the flag is set. + * + * <p><strong>Success + 5xx edge case:</strong> if an operation dispatcher emits a {@code + * SuccessEvent} (flag not set) but the HTTP layer subsequently fails with a 5xx (e.g. JSON + * serialization error), this filter will emit an {@link HttpRequestFailureEvent} in addition to the + * success event already in the audit log. Both entries are correct — the operation itself succeeded + * but the response delivery failed — and are intentionally preserved. + * + * <p><strong>Health check exclusion:</strong> requests targeting {@code /health}, {@code + * /health/*}, {@code /health.html}, {@code /api/health}, and {@code /api/health/*} are silently + * passed through without audit logging to avoid polluting the audit log with probe traffic. + * + * <p><strong>Exception escape:</strong> if an uncaught {@link Throwable} escapes from the filter + * chain and the captured status is still 200 (i.e. no downstream component set an error code), the + * captured status is promoted to 500 before the {@code finally} block runs, ensuring that the audit + * event reflects the actual error condition. + */ +public class HttpAuditFilter implements Filter { + + private static final Logger LOG = LoggerFactory.getLogger(HttpAuditFilter.class); + private static final String X_FORWARDED_FOR = "X-Forwarded-For"; + + @Nullable private final EventBus eventBus; + private final EventSource eventSource; + + /** + * Constructs an {@code HttpAuditFilter}. + * + * @param eventBus the event bus used to dispatch {@link HttpRequestFailureEvent}s; may be {@code + * null}, in which case the filter is a pass-through no-op (useful when no audit listener is + * configured). + * @param eventSource identifies which server this filter instance is installed on; included in + * every emitted {@link HttpRequestFailureEvent}. + */ + public HttpAuditFilter(@Nullable EventBus eventBus, EventSource eventSource) { + this.eventBus = eventBus; + this.eventSource = eventSource; + } + + @Override + public void init(FilterConfig filterConfig) {} + + @Override + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + // Pass through when audit is not configured or request is not HTTP. + if (eventBus == null || !(request instanceof HttpServletRequest)) { + chain.doFilter(request, response); + return; + } + + HttpServletRequest httpRequest = (HttpServletRequest) request; + // Defensive cleanup at request entry in case a pooled thread leaked stale state. + RequestContext.resetOperationFailureFired(); + if (isHealthCheckRequest(httpRequest)) { + chain.doFilter(request, response); + return; + } Review Comment: The health-check bypass returns without clearing RequestContext.operationFailureFired. If a failure event is dispatched during `/api/health*`, the ThreadLocal flag can leak onto the Jetty thread and affect later requests. Wrap the health-check early-return in a try/finally that resets the flag (same as the main path). ########## 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: `AuthorizationDenialFailureEvent` constructs a new `ForbiddenException`, which captures a full stack trace. Authorization denials can be high-volume, and this exception is only a synthetic carrier for audit context, so the stack trace overhead is avoidable. Consider using a lightweight exception that suppresses stack trace (similar to `HttpRequestFailureEvent.HttpRequestException`). ########## 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: Same as above: avoid fully qualified class names here (AGENTS.md:26-30) and simplify the captor to the expected event type to remove the redundant `instanceof`/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()); Review Comment: Avoid using fully qualified class names in tests when there is no name conflict (AGENTS.md:26-30). Here the captor can be typed directly to `AuthorizationDenialFailureEvent`, which also removes the need for the `instanceof` + cast. ########## core/src/main/java/org/apache/gravitino/listener/EventBus.java: ########## @@ -129,17 +131,28 @@ private void dispatchPostEvent(Event postEvent) { } /** - * Dispatches a failure failureEvent to listeners, swallowing all exceptions to preserve the - * original error. + * Dispatches a failure event to listeners, swallowing all exceptions to preserve the original + * error. * * <p>When the primary operation fails, we want to notify listeners, but listener exceptions must * NOT propagate and mask the original failure. This method catches and logs all exceptions * (including {@link RuntimeException}, {@link Error}, etc.) to ensure the original exception can * be properly thrown to the caller. Review Comment: The Javadoc claims this method catches listener failures including `Error`, but the implementation only catches `Exception` (so `Error` would still propagate). Please align the comment with the actual behavior (or broaden the catch, if that is truly intended). -- 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]
