This is an automated email from the ASF dual-hosted git repository. github-actions[bot] pushed a commit to branch cherry-pick-6004dfac-to-branch-1.3 in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit 0f21cba5d3bd4ebe6f4029e527ad20abbc2d62a0 Author: MaSai <[email protected]> AuthorDate: Tue Nov 3 16:13:34 2026 +0800 [#14423] fix(authz): Return 400 for invalid run-job requests in authz (#14526) ### What changes were proposed in this pull request? - In `RunJobAuthorizationExecutor`, treat a missing request body as a client error (`BadRequestException("Request body cannot be null")`) instead of `return false` (which became 403). - Wrap `JobRunRequest.validate()` failures as `BadRequestException` so the existing interceptor `catch (BadRequestException)` path returns **400**, instead of letting `IllegalArgumentException` fall into the catch-all and become **500**. - Add `TestRunJobAuthorizationExecutor` covering the executor and the interception path (null body / blank `jobTemplateName` → 400). ### Why are the changes needed? Fixes #14423 `POST .../jobs/runs` with no body or a missing/blank `jobTemplateName` was answered as 403 or 500 during authorization, before `JobOperations.runJob` could return the usual 400. Same pattern as `LineageAuthorizationExecutor` (map invalid input to `BadRequestException`). Related: #10171, #14304. ### Does this PR introduce _any_ user-facing change? - Invalid run-job requests that previously returned 403/500 during authorization now return **400** with the validation message (aligned with the endpoint). ### How was this patch tested? - `./gradlew :server:test --tests org.apache.gravitino.server.web.filter.authorization.TestRunJobAuthorizationExecutor -PskipITs` Made with [Cursor](https://cursor.com) Co-authored-by: Cursor <[email protected]> --- .../authorization/RunJobAuthorizationExecutor.java | 14 +- .../TestRunJobAuthorizationExecutor.java | 198 +++++++++++++++++++++ 2 files changed, 210 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/apache/gravitino/server/web/filter/authorization/RunJobAuthorizationExecutor.java b/server/src/main/java/org/apache/gravitino/server/web/filter/authorization/RunJobAuthorizationExecutor.java index 7e592408ed..aaadc129b9 100644 --- a/server/src/main/java/org/apache/gravitino/server/web/filter/authorization/RunJobAuthorizationExecutor.java +++ b/server/src/main/java/org/apache/gravitino/server/web/filter/authorization/RunJobAuthorizationExecutor.java @@ -6,7 +6,9 @@ * 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 @@ -26,6 +28,7 @@ import org.apache.gravitino.Entity; import org.apache.gravitino.NameIdentifier; import org.apache.gravitino.authorization.AuthorizationRequestContext; import org.apache.gravitino.dto.requests.JobRunRequest; +import org.apache.gravitino.exceptions.BadRequestException; import org.apache.gravitino.server.authorization.expression.AuthorizationExpressionEvaluator; import org.apache.gravitino.utils.NameIdentifierUtil; @@ -58,7 +61,8 @@ public class RunJobAuthorizationExecutor implements AuthorizationExecutor { public boolean execute(AuthorizationRequestContext context) throws Exception { Object request = extractFromParameters(parameters, args); if (request == null) { - return false; + // Match JobOperations.runJob: a missing body is a client error, not an auth denial. + throw new BadRequestException("Request body cannot be null"); } context.setOriginalAuthorizationExpression(expression); @@ -67,7 +71,13 @@ public class RunJobAuthorizationExecutor implements AuthorizationExecutor { "Expected JobRunRequest but found %s", request.getClass().getSimpleName()); JobRunRequest jobRunRequest = (JobRunRequest) request; - jobRunRequest.validate(); + try { + jobRunRequest.validate(); + } catch (IllegalArgumentException e) { + // validate() throws IllegalArgumentException; map to BadRequest so the interceptor returns + // 400 instead of the catch-all 500 authorization internal error. + throw new BadRequestException(e, "%s", e.getMessage()); + } String jobTemplateName = jobRunRequest.getJobTemplateName(); NameIdentifier metalake = metadataContext.get(Entity.EntityType.METALAKE); NameIdentifier jobTemplateNameIdentifier = diff --git a/server/src/test/java/org/apache/gravitino/server/web/filter/authorization/TestRunJobAuthorizationExecutor.java b/server/src/test/java/org/apache/gravitino/server/web/filter/authorization/TestRunJobAuthorizationExecutor.java new file mode 100644 index 0000000000..ef337928eb --- /dev/null +++ b/server/src/test/java/org/apache/gravitino/server/web/filter/authorization/TestRunJobAuthorizationExecutor.java @@ -0,0 +1,198 @@ +/* + * 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.filter.authorization; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyMap; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +import java.lang.reflect.Method; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; +import javax.ws.rs.core.Response; +import org.aopalliance.intercept.MethodInterceptor; +import org.aopalliance.intercept.MethodInvocation; +import org.apache.commons.lang3.reflect.FieldUtils; +import org.apache.gravitino.Entity; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.UserPrincipal; +import org.apache.gravitino.authorization.AuthorizationRequestContext; +import org.apache.gravitino.authorization.AuthorizationUtils; +import org.apache.gravitino.authorization.GravitinoAuthorizer; +import org.apache.gravitino.dto.requests.JobRunRequest; +import org.apache.gravitino.dto.responses.ErrorResponse; +import org.apache.gravitino.exceptions.BadRequestException; +import org.apache.gravitino.server.authorization.GravitinoAuthorizerProvider; +import org.apache.gravitino.server.authorization.annotations.AuthorizationRequest; +import org.apache.gravitino.server.authorization.expression.AuthorizationExpressionEvaluator; +import org.apache.gravitino.server.web.filter.GravitinoInterceptionService; +import org.apache.gravitino.server.web.rest.JobOperations; +import org.apache.gravitino.utils.NameIdentifierUtil; +import org.apache.gravitino.utils.PrincipalUtils; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.NullAndEmptySource; +import org.junit.jupiter.params.provider.ValueSource; +import org.mockito.MockedStatic; + +public class TestRunJobAuthorizationExecutor { + + @Test + public void testNullRequestBodyThrowsBadRequest() throws Exception { + RunJobAuthorizationExecutor executor = createExecutor(null); + AuthorizationExpressionEvaluator evaluator = mock(AuthorizationExpressionEvaluator.class); + FieldUtils.writeField(executor, "authorizationExpressionEvaluator", evaluator, true); + + BadRequestException thrown = + assertThrows( + BadRequestException.class, () -> executor.execute(new AuthorizationRequestContext())); + assertEquals("Request body cannot be null", thrown.getMessage()); + verifyNoInteractions(evaluator); + } + + @ParameterizedTest + @NullAndEmptySource + @ValueSource(strings = {" "}) + public void testMissingOrBlankJobTemplateNameThrowsBadRequest(String jobTemplateName) + throws Exception { + RunJobAuthorizationExecutor executor = + createExecutor(new JobRunRequest(jobTemplateName, Collections.emptyMap())); + AuthorizationExpressionEvaluator evaluator = mock(AuthorizationExpressionEvaluator.class); + FieldUtils.writeField(executor, "authorizationExpressionEvaluator", evaluator, true); + + BadRequestException thrown = + assertThrows( + BadRequestException.class, () -> executor.execute(new AuthorizationRequestContext())); + assertTrue(thrown.getMessage().contains("jobTemplateName")); + verifyNoInteractions(evaluator); + } + + @Test + public void testValidRequestEvaluatesAuthorization() throws Exception { + JobRunRequest request = new JobRunRequest("tpl", Collections.emptyMap()); + Map<Entity.EntityType, NameIdentifier> metadataContext = new HashMap<>(); + metadataContext.put(Entity.EntityType.METALAKE, NameIdentifier.of("ml")); + RunJobAuthorizationExecutor executor = createExecutor(request, metadataContext); + AuthorizationExpressionEvaluator evaluator = mock(AuthorizationExpressionEvaluator.class); + when(evaluator.evaluate(anyMap(), anyMap(), any(AuthorizationRequestContext.class), any())) + .thenReturn(true); + FieldUtils.writeField(executor, "authorizationExpressionEvaluator", evaluator, true); + + AuthorizationRequestContext context = new AuthorizationRequestContext(); + assertTrue(executor.execute(context)); + assertEquals( + NameIdentifierUtil.ofJobTemplate("ml", "tpl"), + metadataContext.get(Entity.EntityType.JOB_TEMPLATE)); + verify(evaluator).evaluate(anyMap(), anyMap(), any(AuthorizationRequestContext.class), any()); + } + + @Test + public void testInterceptorReturns400ForNullBody() throws Throwable { + Response response = invokeRunJobThroughInterceptor(null); + assertBadRequest(response, "Request body cannot be null"); + } + + @ParameterizedTest + @NullAndEmptySource + @ValueSource(strings = {" "}) + public void testInterceptorReturns400ForMissingOrBlankJobTemplateName(String jobTemplateName) + throws Throwable { + Response response = + invokeRunJobThroughInterceptor(new JobRunRequest(jobTemplateName, Collections.emptyMap())); + assertBadRequest(response, "jobTemplateName"); + } + + private static void assertBadRequest(Response response, String messageFragment) { + assertEquals(Response.Status.BAD_REQUEST.getStatusCode(), response.getStatus()); + ErrorResponse errorResponse = (ErrorResponse) response.getEntity(); + assertTrue(errorResponse.getMessage().contains(messageFragment)); + assertFalse( + errorResponse.getMessage().contains("Authorization failed due to system internal error")); + } + + private static Response invokeRunJobThroughInterceptor(JobRunRequest request) throws Throwable { + Method method = JobOperations.class.getMethod("runJob", String.class, JobRunRequest.class); + MethodInvocation invocation = mock(MethodInvocation.class); + when(invocation.getMethod()).thenReturn(method); + when(invocation.getArguments()).thenReturn(new Object[] {"ml", request}); + + try (MockedStatic<PrincipalUtils> principalUtils = mockStatic(PrincipalUtils.class); + MockedStatic<GravitinoAuthorizerProvider> authorizerProvider = + mockStatic(GravitinoAuthorizerProvider.class); + MockedStatic<AuthorizationUtils> authorizationUtils = + mockStatic(AuthorizationUtils.class)) { + principalUtils + .when(PrincipalUtils::getCurrentPrincipal) + .thenReturn(new UserPrincipal("admin")); + principalUtils.when(PrincipalUtils::getCurrentUserName).thenReturn("admin"); + authorizationUtils + .when(() -> AuthorizationUtils.checkCurrentUser(any(), any(), any())) + .thenAnswer(ignored -> null); + + GravitinoAuthorizerProvider provider = mock(GravitinoAuthorizerProvider.class); + authorizerProvider.when(GravitinoAuthorizerProvider::getInstance).thenReturn(provider); + when(provider.getGravitinoAuthorizer()).thenReturn(mock(GravitinoAuthorizer.class)); + + MethodInterceptor interceptor = + new GravitinoInterceptionService().getMethodInterceptors(method).get(0); + Response response = (Response) interceptor.invoke(invocation); + verify(invocation, never()).proceed(); + return response; + } + } + + private static RunJobAuthorizationExecutor createExecutor(JobRunRequest request) + throws Exception { + Map<Entity.EntityType, NameIdentifier> metadataContext = new HashMap<>(); + metadataContext.put(Entity.EntityType.METALAKE, NameIdentifier.of("ml")); + return createExecutor(request, metadataContext); + } + + private static RunJobAuthorizationExecutor createExecutor( + JobRunRequest request, Map<Entity.EntityType, NameIdentifier> metadataContext) + throws Exception { + Method method = TestOperations.class.getMethod("runJob", JobRunRequest.class); + return new RunJobAuthorizationExecutor( + method.getParameters(), + new Object[] {request}, + "METALAKE::OWNER", + metadataContext, + Collections.emptyMap(), + Optional.empty()); + } + + @SuppressWarnings("unused") + public static class TestOperations { + public void runJob( + @AuthorizationRequest(type = AuthorizationRequest.RequestType.RUN_JOB) + JobRunRequest request) {} + } +}
