This is an automated email from the ASF dual-hosted git repository.
jerryshao 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 b6b0f24f9a [#12727] fix(server): Return 400 for invalid metadata
object types (#13059)
b6b0f24f9a is described below
commit b6b0f24f9af095950f7d2bd6ebdadd688032d249
Author: Nevin Zheng <[email protected]>
AuthorDate: Wed Sep 9 23:39:19 2026 -0700
[#12727] fix(server): Return 400 for invalid metadata object types (#13059)
### What changes were proposed in this pull request?
Treat an unsupported metadata-object `{type}` parsed by the
authorization interceptor as malformed client input. Affected object
endpoints now return the standard HTTP 400 illegal-argument response
instead of an authorization-related HTTP 500.
The change uses `IllegalMetadataObjectException` as a narrow internal
signal at the enum-parsing boundary. The interceptor handles only that
typed exception as client input, while unrelated authorization failures
continue to return HTTP 500. Valid metadata-object types continue
through the existing authorization flow unchanged.
This pull request adds focused interceptor tests and a raw HTTP
integration test with authorization enabled.
### Why are the changes needed?
With authorization enabled,
`ParameterUtil.extractNameIdentifierFromParameters` calls
`MetadataObject.Type.valueOf` before the resource method runs. An
unknown type throws `IllegalArgumentException`, which the interceptor's
generic handler currently reports as a system authorization failure.
This misclassifies client input and affects the endpoint families
documented in #12727.
The earlier diagnosis in #10626 and implementation attempts #10634 and
#10635 identified the same failure mode. Those pull requests conflict
with current `main`; this pull request carries a narrowly typed
correction forward against current `main` and acknowledges that prior
work.
Fix: #12727
### Does this PR introduce _any_ user-facing change?
Yes. With authorization enabled, an unsupported metadata-object type now
returns HTTP 400 with error code `1001` and a message identifying the
rejected value, instead of HTTP 500 with error code `1002` and a generic
internal-authorization message.
Valid requests, public APIs, and configuration remain unchanged.
### How was this patch tested?
- `./gradlew :server:test --tests
'org.apache.gravitino.server.web.filter.TestGravitinoInterceptionService'
-PskipITs -PskipWeb=true` — passed 19 tests with no failures or errors.
- `./gradlew :clients:client-java:test --tests
'org.apache.gravitino.client.integration.test.authorization.InvalidMetadataObjectTypeAuthorizationIT'
-PskipWeb=true` — passed the raw HTTP authorization integration test.
- `./gradlew :server:spotlessApply :clients:client-java:spotlessApply
-PskipWeb=true` — passed.
- `git diff --check` — passed.
---
.../InvalidMetadataObjectTypeAuthorizationIT.java | 67 ++++++++++++++++++++++
.../web/filter/GravitinoInterceptionService.java | 5 ++
.../gravitino/server/web/filter/ParameterUtil.java | 10 +++-
.../filter/TestGravitinoInterceptionService.java | 63 ++++++++++++++++++++
4 files changed, 143 insertions(+), 2 deletions(-)
diff --git
a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/InvalidMetadataObjectTypeAuthorizationIT.java
b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/InvalidMetadataObjectTypeAuthorizationIT.java
new file mode 100644
index 0000000000..a8530caba3
--- /dev/null
+++
b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/InvalidMetadataObjectTypeAuthorizationIT.java
@@ -0,0 +1,67 @@
+/*
+ * 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.client.integration.test.authorization;
+
+import java.net.URI;
+import java.net.http.HttpClient;
+import java.net.http.HttpRequest;
+import java.net.http.HttpResponse;
+import java.nio.charset.StandardCharsets;
+import java.util.Base64;
+import org.apache.gravitino.auth.AuthConstants;
+import org.apache.gravitino.dto.responses.ErrorConstants;
+import org.apache.gravitino.dto.responses.ErrorResponse;
+import org.apache.gravitino.server.web.ObjectMapperProvider;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+/** Integration tests for invalid metadata object types in
authorization-protected REST paths. */
+public class InvalidMetadataObjectTypeAuthorizationIT extends
BaseRestApiAuthorizationIT {
+
+ /** Verifies that an invalid metadata object type is reported as malformed
client input. */
+ @Test
+ public void testInvalidMetadataObjectTypeReturnsBadRequest() throws
Exception {
+ String authorization =
+ AuthConstants.AUTHORIZATION_BASIC_HEADER
+ + Base64.getEncoder()
+ .encodeToString((USER +
":dummy").getBytes(StandardCharsets.UTF_8));
+ HttpRequest request =
+ HttpRequest.newBuilder()
+ .uri(new URI(serverUri +
"/api/metalakes/zz/objects/bogusType/a.b.c/tags"))
+ .header(AuthConstants.HTTP_HEADER_AUTHORIZATION, authorization)
+ .GET()
+ .build();
+
+ HttpResponse<String> response =
+ HttpClient.newHttpClient().send(request,
HttpResponse.BodyHandlers.ofString());
+
+ Assertions.assertEquals(400, response.statusCode(), "Unexpected body: " +
response.body());
+ ErrorResponse errorResponse =
+ ObjectMapperProvider.objectMapper().readValue(response.body(),
ErrorResponse.class);
+ Assertions.assertEquals(ErrorConstants.ILLEGAL_ARGUMENTS_CODE,
errorResponse.getCode());
+ Assertions.assertEquals(
+ IllegalArgumentException.class.getSimpleName(),
errorResponse.getType());
+ Assertions.assertTrue(
+ errorResponse.getMessage().contains("bogusType"),
+ "Unexpected message: " + errorResponse.getMessage());
+ Assertions.assertFalse(
+ errorResponse.getMessage().contains("Authorization failed due to
system internal error"),
+ "Unexpected message: " + errorResponse.getMessage());
+ }
+}
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 acd9499fa8..cb82a46798 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
@@ -46,6 +46,7 @@ import
org.apache.gravitino.authorization.AuthorizationRequestContext;
import org.apache.gravitino.authorization.AuthorizationUtils;
import org.apache.gravitino.exceptions.BadRequestException;
import org.apache.gravitino.exceptions.ForbiddenException;
+import org.apache.gravitino.exceptions.IllegalMetadataObjectException;
import org.apache.gravitino.exceptions.IllegalNameIdentifierException;
import org.apache.gravitino.exceptions.NoSuchMetalakeException;
import org.apache.gravitino.lineage.source.rest.LineageOperations;
@@ -265,6 +266,10 @@ public class GravitinoInterceptionService implements
InterceptionService {
}
}
return methodInvocation.proceed();
+ } catch (IllegalMetadataObjectException ex) {
+ LOG.warn("Invalid metadata object type during authorization", ex);
+ return Utils.illegalArguments(
+ IllegalArgumentException.class.getSimpleName(), ex.getMessage(),
ex);
} catch (IllegalNameIdentifierException ex) {
LOG.warn("Invalid metadata object identifier during authorization",
ex);
return Utils.illegalArguments(ex.getMessage(), ex);
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/filter/ParameterUtil.java
b/server/src/main/java/org/apache/gravitino/server/web/filter/ParameterUtil.java
index 70c0731d64..026a5a47f3 100644
---
a/server/src/main/java/org/apache/gravitino/server/web/filter/ParameterUtil.java
+++
b/server/src/main/java/org/apache/gravitino/server/web/filter/ParameterUtil.java
@@ -28,6 +28,7 @@ import org.apache.gravitino.Entity;
import org.apache.gravitino.MetadataObject;
import org.apache.gravitino.MetadataObjects;
import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.exceptions.IllegalMetadataObjectException;
import
org.apache.gravitino.server.authorization.annotations.AuthorizationFullName;
import
org.apache.gravitino.server.authorization.annotations.AuthorizationMetadata;
import
org.apache.gravitino.server.authorization.annotations.AuthorizationObjectType;
@@ -82,8 +83,13 @@ public class ParameterUtil {
if (fullName.isPresent() && metadataObjectType.isPresent()) {
String metalake = entities.get(Entity.EntityType.METALAKE);
if (metalake != null) {
- MetadataObject.Type type =
-
MetadataObject.Type.valueOf(metadataObjectType.get().toUpperCase(Locale.ROOT));
+ String rawType = metadataObjectType.get();
+ MetadataObject.Type type;
+ try {
+ type = MetadataObject.Type.valueOf(rawType.toUpperCase(Locale.ROOT));
+ } catch (IllegalArgumentException e) {
+ throw new IllegalMetadataObjectException(e, "Invalid metadata object
type: %s", rawType);
+ }
NameIdentifier nameIdentifier =
MetadataObjectUtil.toEntityIdent(metalake,
MetadataObjects.parse(fullName.get(), type));
nameIdentifierMap.putAll(
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 bedec592e1..c82726272a 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
@@ -54,6 +54,7 @@ import org.apache.gravitino.catalog.TableDispatcher;
import org.apache.gravitino.catalog.ViewDispatcher;
import org.apache.gravitino.dto.requests.SchemaCreateRequest;
import org.apache.gravitino.dto.requests.TagValuesAssociateRequest;
+import org.apache.gravitino.dto.responses.ErrorConstants;
import org.apache.gravitino.dto.responses.ErrorResponse;
import org.apache.gravitino.exceptions.ForbiddenException;
import org.apache.gravitino.exceptions.NoSuchMetalakeException;
@@ -325,6 +326,34 @@ public class TestGravitinoInterceptionService {
}
}
+ @Test
+ public void testInvalidMetadataObjectTypeReturnsBadRequest() throws
Throwable {
+ Method method =
+ TestMetadataObjectTagAssociationOperations.class.getMethod(
+ "associateTagValuesForObject",
+ String.class,
+ String.class,
+ String.class,
+ TagValuesAssociateRequest.class);
+ MethodInvocation invocation = mock(MethodInvocation.class);
+ when(invocation.getMethod()).thenReturn(method);
+ when(invocation.getArguments())
+ .thenReturn(new Object[] {"testMetalake", "bogusType", "a.b.c", null});
+
+ MethodInterceptor interceptor =
+ new
GravitinoInterceptionService().getMethodInterceptors(method).get(0);
+ Response response = (Response) interceptor.invoke(invocation);
+
+ assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
response.getStatus());
+ ErrorResponse errorResponse = (ErrorResponse) response.getEntity();
+ assertEquals(ErrorConstants.ILLEGAL_ARGUMENTS_CODE,
errorResponse.getCode());
+ assertEquals(IllegalArgumentException.class.getSimpleName(),
errorResponse.getType());
+ Assertions.assertTrue(errorResponse.getMessage().contains("bogusType"));
+ Assertions.assertFalse(
+ errorResponse.getMessage().contains("Authorization failed due to
system internal error"));
+ verify(invocation, never()).proceed();
+ }
+
@Test
public void testSystemInternalErrorHandling() throws Throwable {
try (MockedStatic<PrincipalUtils> principalUtilsMocked =
mockStatic(PrincipalUtils.class);
@@ -368,6 +397,40 @@ public class TestGravitinoInterceptionService {
}
}
+ @Test
+ public void testUnexpectedIllegalArgumentExceptionRemainsInternalError()
throws Throwable {
+ try (MockedStatic<PrincipalUtils> principalUtilsMocked =
mockStatic(PrincipalUtils.class);
+ MockedStatic<GravitinoAuthorizerProvider> mockStatic =
+ mockStatic(GravitinoAuthorizerProvider.class)) {
+ principalUtilsMocked
+ .when(PrincipalUtils::getCurrentPrincipal)
+ .thenReturn(new UserPrincipal("tester"));
+
principalUtilsMocked.when(PrincipalUtils::getCurrentUserName).thenReturn("tester");
+
+ MethodInvocation methodInvocation = mock(MethodInvocation.class);
+ GravitinoAuthorizerProvider mockedProvider =
mock(GravitinoAuthorizerProvider.class);
+
mockStatic.when(GravitinoAuthorizerProvider::getInstance).thenReturn(mockedProvider);
+ when(mockedProvider.getGravitinoAuthorizer())
+ .thenThrow(new IllegalArgumentException("Invalid authorizer
configuration"));
+
+ GravitinoInterceptionService gravitinoInterceptionService =
+ new GravitinoInterceptionService();
+ Method testMethod = TestOperations.class.getMethods()[0];
+ MethodInterceptor methodInterceptor =
+
gravitinoInterceptionService.getMethodInterceptors(testMethod).get(0);
+ when(methodInvocation.getMethod()).thenReturn(testMethod);
+ when(methodInvocation.getArguments()).thenReturn(new Object[]
{"testMetalake"});
+
+ Response response = (Response)
methodInterceptor.invoke(methodInvocation);
+
+ ErrorResponse errorResponse = (ErrorResponse) response.getEntity();
+ assertEquals(
+ "Authorization failed due to system internal error. Please contact
administrator.",
+ errorResponse.getMessage());
+ assertEquals(Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(),
response.getStatus());
+ }
+ }
+
@Test
public void testDottedMetadataNameReturnsBadRequest() throws Throwable {
try (MockedStatic<PrincipalUtils> principalUtilsMocked =
mockStatic(PrincipalUtils.class);
