This is an automated email from the ASF dual-hosted git repository.
mchades 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 942e5d6ba2 [#14668] fix(server): Return 400 for blank catalog names
(#14669)
942e5d6ba2 is described below
commit 942e5d6ba221dde177f931fb5380cfc6dd3ba2a3
Author: mchades <[email protected]>
AuthorDate: Fri Nov 6 21:13:01 2026 +0800
[#14668] fix(server): Return 400 for blank catalog names (#14669)
### What changes were proposed in this pull request?
Convert metadata-name parsing failures to
`IllegalMetadataObjectException` before evaluating authorization
expressions, preserving existing identifier-validation exceptions. Keep
unexpected authorizer configuration, executor, and resource failures on
the interceptor's existing HTTP 500 path. Resource-level handlers retain
the more specific parsing-error type, including Semantic Model
operations.
### Why are the changes needed?
Whitespace-only catalog names fail validation before the REST resource
runs and currently return a generic HTTP 500 authorization error.
Fixes #14668
### Does this PR introduce _any_ user-facing change?
Yes. Whitespace-only catalog names return HTTP 400 with code `1001` and
the validation message. Resource and bulk error responses can now report
`IllegalMetadataObjectException` instead of `IllegalArgumentException`
for metadata-name parsing failures; this type refinement is accepted for
2.0. Unexpected failures in the authorization interceptor retain generic
HTTP 500 responses and health-failure recording.
### How was this patch tested?
- All 790 server tests, 11 evaluator tests, and 5 HTTP integration cases
passed (806 total). This includes all 24 Semantic Model
whitespace-source cases and the interceptor's internal-error response
and health-recording regressions.
- Formatting checks and `git diff --check` passed. Repository-wide and
Docker tests were not run locally.
```sh
./gradlew :server:spotlessApply :server:spotlessCheck
:server-common:spotlessCheck :clients:client-java:spotlessCheck :server:test
:server-common:test --tests '*TestAuthorizationExpressionEvaluator'
:clients:client-java:test --tests '*InvalidCatalogNameAuthorizationIT' --tests
'*InvalidMetadataObjectTypeAuthorizationIT' -PskipWeb=true
-PskipDockerTests=true
```
---
.../InvalidCatalogNameAuthorizationIT.java | 72 +++++++++++++
.../AuthorizationExpressionEvaluator.java | 13 ++-
.../TestAuthorizationExpressionEvaluator.java | 54 ++++++++++
.../filter/TestGravitinoInterceptionService.java | 120 ++++++++++++++++++++-
.../web/rest/TestSemanticModelOperations.java | 3 +-
5 files changed, 256 insertions(+), 6 deletions(-)
diff --git
a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/InvalidCatalogNameAuthorizationIT.java
b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/InvalidCatalogNameAuthorizationIT.java
new file mode 100644
index 0000000000..df88a727bd
--- /dev/null
+++
b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/InvalidCatalogNameAuthorizationIT.java
@@ -0,0 +1,72 @@
+/*
+ * 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.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+/** Integration tests for invalid Catalog names with authorization enabled. */
+public class InvalidCatalogNameAuthorizationIT extends
BaseRestApiAuthorizationIT {
+
+ /**
+ * Verifies that URL-encoded whitespace in a Catalog name returns a JSON
bad-request response.
+ *
+ * @param encodedName the percent-encoded invalid Catalog name
+ */
+ @ParameterizedTest
+ @ValueSource(strings = {"%20", "%09", "%0A", "%20%09%0A"})
+ public void testBlankCatalogNameReturnsBadRequest(String encodedName) 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/" + METALAKE +
"/catalogs/" + encodedName))
+ .header(AuthConstants.HTTP_HEADER_AUTHORIZATION, authorization)
+ .header("Accept", "application/vnd.gravitino.v1+json")
+ .GET()
+ .build();
+
+ HttpResponse<String> response =
+ HttpClient.newHttpClient().send(request,
HttpResponse.BodyHandlers.ofString());
+
+ Assertions.assertEquals(400, response.statusCode(), "Unexpected body: " +
response.body());
+ Assertions.assertTrue(
+
response.headers().firstValue("Content-Type").orElse("").startsWith("application/json"));
+ 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.assertEquals(
+ "Metadata object full name cannot be blank",
errorResponse.getMessage());
+ }
+}
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionEvaluator.java
b/server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionEvaluator.java
index 0311de01f0..5fcc56bd3e 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionEvaluator.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionEvaluator.java
@@ -33,6 +33,8 @@ import org.apache.gravitino.MetadataObject;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.authorization.AuthorizationRequestContext;
import org.apache.gravitino.authorization.GravitinoAuthorizer;
+import org.apache.gravitino.exceptions.IllegalMetadataObjectException;
+import org.apache.gravitino.exceptions.IllegalNameIdentifierException;
import org.apache.gravitino.server.authorization.GravitinoAuthorizerProvider;
import org.apache.gravitino.utils.NameIdentifierUtil;
import org.apache.gravitino.utils.PrincipalUtils;
@@ -183,8 +185,15 @@ public class AuthorizationExpressionEvaluator {
metadataNames.forEach(
(type, entityNameIdent) -> {
if (isMetadataType(type)) {
- MetadataObject metadataObject =
- NameIdentifierUtil.toMetadataObject(entityNameIdent, type);
+ MetadataObject metadataObject;
+ try {
+ metadataObject =
NameIdentifierUtil.toMetadataObject(entityNameIdent, type);
+ } catch (IllegalNameIdentifierException e) {
+ throw e;
+ } catch (IllegalArgumentException e) {
+ // Only metadata-name parsing failures are client errors, not
authorizer failures.
+ throw new IllegalMetadataObjectException(e, "%s",
e.getMessage());
+ }
ognlContext.put(type.name(), metadataObject);
}
ognlContext.put(type.name() + "_NAME_IDENT", entityNameIdent);
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/authorization/expression/TestAuthorizationExpressionEvaluator.java
b/server-common/src/test/java/org/apache/gravitino/server/authorization/expression/TestAuthorizationExpressionEvaluator.java
index 88cd841cf7..df34e7850f 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/authorization/expression/TestAuthorizationExpressionEvaluator.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/authorization/expression/TestAuthorizationExpressionEvaluator.java
@@ -20,6 +20,7 @@ package org.apache.gravitino.server.authorization.expression;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.mockStatic;
+import static org.mockito.Mockito.verifyNoInteractions;
import static org.mockito.Mockito.when;
import java.util.HashMap;
@@ -41,6 +42,8 @@ import org.apache.gravitino.UserPrincipal;
import org.apache.gravitino.authorization.AuthorizationRequestContext;
import org.apache.gravitino.authorization.GravitinoAuthorizer;
import org.apache.gravitino.authorization.Privilege;
+import org.apache.gravitino.exceptions.IllegalMetadataObjectException;
+import org.apache.gravitino.exceptions.IllegalNameIdentifierException;
import org.apache.gravitino.server.authorization.GravitinoAuthorizerProvider;
import org.apache.gravitino.server.authorization.MockGravitinoAuthorizer;
import org.apache.gravitino.utils.NameIdentifierUtil;
@@ -48,6 +51,8 @@ import org.apache.gravitino.utils.PrincipalUtils;
import org.junit.jupiter.api.Assertions;
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;
/** Test for {@link AuthorizationExpressionEvaluator} */
@@ -97,6 +102,55 @@ public class TestAuthorizationExpressionEvaluator {
}
}
+ /**
+ * Verifies that blank names are classified as invalid metadata before the
authorizer is called.
+ *
+ * @param catalogName the whitespace-only catalog name
+ */
+ @ParameterizedTest
+ @ValueSource(strings = {" ", "\t", "\n", " \t\n"})
+ public void testBlankCatalogNameIsInvalidMetadata(String catalogName) {
+ GravitinoAuthorizer authorizer = mock(GravitinoAuthorizer.class);
+ AuthorizationExpressionEvaluator evaluator =
+ new AuthorizationExpressionEvaluator("CATALOG::USE_CATALOG",
authorizer);
+ Map<Entity.EntityType, NameIdentifier> names =
+ Map.of(Entity.EntityType.CATALOG,
NameIdentifierUtil.ofCatalog("metalake", catalogName));
+
+ IllegalMetadataObjectException error =
+ Assertions.assertThrows(
+ IllegalMetadataObjectException.class,
+ () ->
+ evaluator.evaluate(
+ names,
+ new AuthorizationRequestContext(),
+ new UserPrincipal("tester"),
+ Optional.empty()));
+
+ Assertions.assertEquals("Metadata object full name cannot be blank",
error.getMessage());
+ Assertions.assertInstanceOf(IllegalArgumentException.class,
error.getCause());
+ verifyNoInteractions(authorizer);
+ }
+
+ /** Verifies that existing identifier-validation errors retain their
exception type. */
+ @Test
+ public void testDottedCatalogNameRetainsIdentifierError() {
+ GravitinoAuthorizer authorizer = mock(GravitinoAuthorizer.class);
+ AuthorizationExpressionEvaluator evaluator =
+ new AuthorizationExpressionEvaluator("CATALOG::USE_CATALOG",
authorizer);
+ Map<Entity.EntityType, NameIdentifier> names =
+ Map.of(Entity.EntityType.CATALOG,
NameIdentifierUtil.ofCatalog("metalake", "sales.2024"));
+
+ Assertions.assertThrows(
+ IllegalNameIdentifierException.class,
+ () ->
+ evaluator.evaluate(
+ names,
+ new AuthorizationRequestContext(),
+ new UserPrincipal("tester"),
+ Optional.empty()));
+ verifyNoInteractions(authorizer);
+ }
+
@Test
public void testEvaluatorWithOwner() {
String expression = "METALAKE::OWNER || CATALOG::CREATE_CATALOG";
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 2e476586f4..c703470aa4 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
@@ -95,7 +95,10 @@ import
org.apache.gravitino.server.authorization.annotations.AuthorizationFullNa
import
org.apache.gravitino.server.authorization.annotations.AuthorizationMetadata;
import
org.apache.gravitino.server.authorization.annotations.AuthorizationObjectType;
import
org.apache.gravitino.server.authorization.annotations.AuthorizationRequest;
+import org.apache.gravitino.server.web.ServerHealth;
import org.apache.gravitino.server.web.Utils;
+import
org.apache.gravitino.server.web.filter.authorization.AuthorizationExecutor;
+import
org.apache.gravitino.server.web.filter.authorization.AuthorizeExecutorFactory;
import org.apache.gravitino.server.web.rest.CatalogOperations;
import org.apache.gravitino.server.web.rest.JobOperations;
import org.apache.gravitino.server.web.rest.MetadataObjectPolicyOperations;
@@ -123,6 +126,7 @@ import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;
+import org.junit.jupiter.params.provider.ValueSource;
import org.mockito.ArgumentCaptor;
import org.mockito.ArgumentMatchers;
import org.mockito.MockedStatic;
@@ -681,11 +685,13 @@ public class TestGravitinoInterceptionService {
}
}
+ /** Verifies that unexpected authorizer configuration errors retain
server-error handling. */
@Test
public void testUnexpectedIllegalArgumentExceptionRemainsInternalError()
throws Throwable {
try (MockedStatic<PrincipalUtils> principalUtilsMocked =
mockStatic(PrincipalUtils.class);
MockedStatic<GravitinoAuthorizerProvider> mockStatic =
- mockStatic(GravitinoAuthorizerProvider.class)) {
+ mockStatic(GravitinoAuthorizerProvider.class);
+ MockedStatic<ServerHealth> healths = mockStatic(ServerHealth.class)) {
principalUtilsMocked
.when(PrincipalUtils::getCurrentPrincipal)
.thenReturn(new UserPrincipal("tester"));
@@ -694,8 +700,11 @@ public class TestGravitinoInterceptionService {
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"));
+ IllegalArgumentException failure =
+ new IllegalArgumentException("Invalid authorizer configuration");
+ when(mockedProvider.getGravitinoAuthorizer()).thenThrow(failure);
+ ServerHealth health = mock(ServerHealth.class);
+ healths.when(ServerHealth::getInstance).thenReturn(health);
GravitinoInterceptionService gravitinoInterceptionService =
new GravitinoInterceptionService();
@@ -711,7 +720,112 @@ public class TestGravitinoInterceptionService {
assertEquals(
"Authorization failed due to system internal error. Please contact
administrator.",
errorResponse.getMessage());
+ assertEquals(RuntimeException.class.getSimpleName(),
errorResponse.getType());
+ assertEquals(ErrorConstants.INTERNAL_ERROR_CODE,
errorResponse.getCode());
assertEquals(Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(),
response.getStatus());
+ verify(health).recordFailure(failure);
+ verify(methodInvocation, never()).proceed();
+ }
+ }
+
+ /**
+ * Verifies that raw execution failures are not classified as malformed
client input.
+ *
+ * @param failInExecutor whether the executor fails instead of the REST
resource method
+ */
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void
testExecutionIllegalArgumentExceptionRemainsInternalError(boolean
failInExecutor)
+ throws Throwable {
+ try (MockedStatic<PrincipalUtils> principals =
mockStatic(PrincipalUtils.class);
+ MockedStatic<AuthorizationUtils> authorization =
mockStatic(AuthorizationUtils.class);
+ MockedStatic<AuthorizeExecutorFactory> factories =
+ mockStatic(AuthorizeExecutorFactory.class);
+ MockedStatic<ServerHealth> healths = mockStatic(ServerHealth.class)) {
+ principals.when(PrincipalUtils::getCurrentUserName).thenReturn("tester");
+ ServerHealth health = mock(ServerHealth.class);
+ healths.when(ServerHealth::getInstance).thenReturn(health);
+ AuthorizationExecutor executor = mock(AuthorizationExecutor.class);
+ factories
+ .when(
+ () ->
+ AuthorizeExecutorFactory.create(
+ any(), any(), any(), any(), any(), any(), any(), any(),
any(), any()))
+ .thenReturn(executor);
+ Method method = TestOperations.class.getMethod("testMethod",
String.class);
+ MethodInvocation invocation = mock(MethodInvocation.class);
+ when(invocation.getMethod()).thenReturn(method);
+ when(invocation.getArguments()).thenReturn(new Object[] {"metalake"});
+ IllegalArgumentException failure =
+ new IllegalArgumentException("Invalid backend configuration");
+ if (failInExecutor) {
+ when(executor.execute(any())).thenThrow(failure);
+ } else {
+ when(executor.execute(any())).thenReturn(true);
+ when(invocation.proceed()).thenThrow(failure);
+ }
+
+ try (Response response =
+ (Response)
+ new GravitinoInterceptionService()
+ .getMethodInterceptors(method)
+ .get(0)
+ .invoke(invocation)) {
+ assertEquals(Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(),
response.getStatus());
+ ErrorResponse error = (ErrorResponse) response.getEntity();
+ assertEquals(ErrorConstants.INTERNAL_ERROR_CODE, error.getCode());
+ assertEquals(RuntimeException.class.getSimpleName(), error.getType());
+ assertEquals(
+ "Authorization failed due to system internal error. Please contact
administrator.",
+ error.getMessage());
+ }
+ verify(health).recordFailure(failure);
+ verify(executor).execute(any());
+ if (failInExecutor) {
+ verify(invocation, never()).proceed();
+ } else {
+ verify(invocation).proceed();
+ }
+ }
+ }
+
+ /**
+ * Verifies that blank Catalog names are rejected before the resource method
is invoked.
+ *
+ * @param catalogName the whitespace-only Catalog name
+ */
+ @ParameterizedTest
+ @ValueSource(strings = {" ", "\t", "\n", " \t\n"})
+ public void testBlankCatalogNameReturnsBadRequest(String catalogName) throws
Throwable {
+ try (MockedStatic<PrincipalUtils> principals =
mockStatic(PrincipalUtils.class);
+ MockedStatic<GravitinoAuthorizerProvider> providers =
+ mockStatic(GravitinoAuthorizerProvider.class);
+ // Bypass checkCurrentUser's GravitinoEnv lookup to isolate
catalog-name validation.
+ MockedStatic<AuthorizationUtils> authUtils =
mockStatic(AuthorizationUtils.class)) {
+ principals.when(PrincipalUtils::getCurrentPrincipal).thenReturn(new
UserPrincipal("tester"));
+ principals.when(PrincipalUtils::getCurrentUserName).thenReturn("tester");
+ GravitinoAuthorizerProvider provider =
mock(GravitinoAuthorizerProvider.class);
+
providers.when(GravitinoAuthorizerProvider::getInstance).thenReturn(provider);
+
when(provider.getGravitinoAuthorizer()).thenReturn(mock(GravitinoAuthorizer.class));
+
+ Method method = CatalogOperations.class.getMethod("loadCatalog",
String.class, String.class);
+ MethodInvocation invocation = mock(MethodInvocation.class);
+ when(invocation.getMethod()).thenReturn(method);
+ when(invocation.getArguments()).thenReturn(new Object[] {"testMetalake",
catalogName});
+
+ Response response =
+ (Response)
+ new GravitinoInterceptionService()
+ .getMethodInterceptors(method)
+ .get(0)
+ .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());
+ assertEquals("Metadata object full name cannot be blank",
errorResponse.getMessage());
+ verify(invocation, never()).proceed();
}
}
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestSemanticModelOperations.java
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestSemanticModelOperations.java
index 7b321251ee..97cc2dd10a 100644
---
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestSemanticModelOperations.java
+++
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestSemanticModelOperations.java
@@ -73,6 +73,7 @@ import org.apache.gravitino.dto.semantic.DatasetDTO;
import org.apache.gravitino.dto.semantic.SemanticModelDefinitionDTO;
import org.apache.gravitino.exceptions.ConnectionFailedException;
import org.apache.gravitino.exceptions.ForbiddenException;
+import org.apache.gravitino.exceptions.IllegalMetadataObjectException;
import org.apache.gravitino.exceptions.IllegalSemanticModelException;
import org.apache.gravitino.exceptions.MetalakeNotInUseException;
import org.apache.gravitino.exceptions.NoSuchSchemaException;
@@ -950,7 +951,7 @@ public class TestSemanticModelOperations extends
BaseOperationsTest {
response,
Response.Status.BAD_REQUEST,
ErrorConstants.ILLEGAL_ARGUMENTS_CODE,
- IllegalArgumentException.class.getSimpleName(),
+ IllegalMetadataObjectException.class.getSimpleName(),
"Metadata object full name cannot be blank");
verifyNoModelWrites();
verifyNoInteractions(tables, views);
