This is an automated email from the ASF dual-hosted git repository.
roryqi pushed a commit to branch branch-1.3
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/branch-1.3 by this push:
new cfe3e0a296 [Cherry-pick to branch-1.3] [#12977] fix(server): Report
dotted metadata names clearly (#12980) (#12995)
cfe3e0a296 is described below
commit cfe3e0a296164ea832e1821b28ebacf85df936cc
Author: github-actions[bot]
<41898282+github-actions[bot]@users.noreply.github.com>
AuthorDate: Wed Sep 9 10:00:46 2026 +0800
[Cherry-pick to branch-1.3] [#12977] fix(server): Report dotted metadata
names clearly (#12980) (#12995)
**Cherry-pick Information:**
- Original commit: 5aea36da5d7a9460a9b2e35e94e938c1161de4d9
- Target branch: `branch-1.3`
- Status: ✅ Conflicts resolved manually
**Resolution:**
- Preserved the `branch-1.3` list-authorization short-circuit and
applied the dotted metadata-name validation before it.
- Adapted the interception regression test to use the `branch-1.3`
`TableOperations` fixture instead of main-only test helpers.
- Removed all committed conflict markers.
**Validation:**
- `./gradlew :core:test --tests
org.apache.gravitino.utils.TestNameIdentifierUtil :server-common:test
--tests
org.apache.gravitino.server.authorization.TestMetadataAuthzHelper
:server:test --tests
org.apache.gravitino.server.web.filter.TestGravitinoInterceptionService
-PskipITs -PskipWeb=true`
- Conflict-marker scan and `git diff --check`
---------
Co-authored-by: roryqi <[email protected]>
Co-authored-by: roryqi <[email protected]>
---
.../apache/gravitino/utils/NameIdentifierUtil.java | 20 +++++++++
.../gravitino/utils/TestNameIdentifierUtil.java | 24 +++++++++++
.../server/authorization/MetadataAuthzHelper.java | 26 ++++++++----
.../authorization/TestMetadataAuthzHelper.java | 44 ++++++++++++++++++++
.../web/filter/GravitinoInterceptionService.java | 4 ++
.../filter/TestGravitinoInterceptionService.java | 47 ++++++++++++++++++++++
6 files changed, 158 insertions(+), 7 deletions(-)
diff --git
a/core/src/main/java/org/apache/gravitino/utils/NameIdentifierUtil.java
b/core/src/main/java/org/apache/gravitino/utils/NameIdentifierUtil.java
index b7bc9b742d..33747d8b44 100644
--- a/core/src/main/java/org/apache/gravitino/utils/NameIdentifierUtil.java
+++ b/core/src/main/java/org/apache/gravitino/utils/NameIdentifierUtil.java
@@ -593,6 +593,25 @@ public class NameIdentifierUtil {
NamespaceUtil.checkJobTemplate(ident.namespace());
}
+ /**
+ * Check whether the metadata object name can be represented in a qualified
metadata object name.
+ *
+ * @param ident The metadata object identifier to check
+ * @param entityType The metadata object entity type
+ * @throws IllegalNameIdentifierException If the object name contains the
qualified-name separator
+ */
+ public static void checkMetadataObjectName(NameIdentifier ident,
Entity.EntityType entityType) {
+ Preconditions.checkArgument(
+ ident != null && entityType != null, "The identifier and entity type
must not be null");
+
+ if (ident.name().contains(".")) {
+ throw new IllegalNameIdentifierException(
+ "The %s name '%s' is unsupported because '.' is reserved as the
qualified-name "
+ + "separator.",
+ entityType, ident.name());
+ }
+ }
+
/**
* Convert the given {@link NameIdentifier} and {@link Entity.EntityType} to
{@link
* MetadataObject}.
@@ -605,6 +624,7 @@ public class NameIdentifierUtil {
NameIdentifier ident, Entity.EntityType entityType) {
Preconditions.checkArgument(
ident != null && entityType != null, "The identifier and entity type
must not be null");
+ checkMetadataObjectName(ident, entityType);
Joiner dot = Joiner.on(".");
diff --git
a/core/src/test/java/org/apache/gravitino/utils/TestNameIdentifierUtil.java
b/core/src/test/java/org/apache/gravitino/utils/TestNameIdentifierUtil.java
index 99967d2ad3..8ff6f37e2b 100644
--- a/core/src/test/java/org/apache/gravitino/utils/TestNameIdentifierUtil.java
+++ b/core/src/test/java/org/apache/gravitino/utils/TestNameIdentifierUtil.java
@@ -163,6 +163,30 @@ public class TestNameIdentifierUtil {
assertTrue(e3.getMessage().contains("Entity type MODEL_VERSION is not
supported"));
}
+ @Test
+ public void testRejectDottedMetadataObjectName() {
+ NameIdentifier table = NameIdentifier.of("metalake1", "catalog1",
"schema1", "sales.2024");
+ IllegalNameIdentifierException tableException =
+ assertThrows(
+ IllegalNameIdentifierException.class,
+ () -> NameIdentifierUtil.toMetadataObject(table,
Entity.EntityType.TABLE));
+ assertEquals(
+ "The TABLE name 'sales.2024' is unsupported because '.' is reserved as
the "
+ + "qualified-name separator.",
+ tableException.getMessage());
+
+ NameIdentifier topic =
+ NameIdentifier.of("metalake1", "catalog1", "schema1",
"orders.created.v1");
+ IllegalNameIdentifierException topicException =
+ assertThrows(
+ IllegalNameIdentifierException.class,
+ () -> NameIdentifierUtil.toMetadataObject(topic,
Entity.EntityType.TOPIC));
+ assertEquals(
+ "The TOPIC name 'orders.created.v1' is unsupported because '.' is
reserved as the "
+ + "qualified-name separator.",
+ topicException.getMessage());
+ }
+
@Test
void testOfUser() {
String userName = "userA";
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java
b/server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java
index 744fdcb181..fcb4f2cf5e 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java
@@ -31,6 +31,7 @@ import java.util.concurrent.CompletableFuture;
import java.util.concurrent.Executor;
import java.util.concurrent.Executors;
import java.util.function.Function;
+import java.util.stream.Collectors;
import org.apache.gravitino.Config;
import org.apache.gravitino.Configs;
import org.apache.gravitino.Entity;
@@ -89,6 +90,11 @@ public class MetadataAuthzHelper {
private static final List<Entity.EntityType> REQUIRE_SCHEMA_EXISTS =
Arrays.asList(Entity.EntityType.TABLE, Entity.EntityType.TOPIC);
+ private static final Set<Entity.EntityType> METADATA_OBJECT_ENTITY_TYPES =
+ Arrays.stream(MetadataObject.Type.values())
+ .map(type -> Entity.EntityType.valueOf(type.name()))
+ .collect(Collectors.toUnmodifiableSet());
+
private static final String TABLE_PARENT_SCOPES = "METALAKE, CATALOG,
SCHEMA";
private static final String SCHEMA_PARENT_SCOPES = "METALAKE, CATALOG";
private static final String CATALOG_PARENT_SCOPES = "METALAKE";
@@ -326,13 +332,19 @@ public class MetadataAuthzHelper {
// per-object loop over every catalog in the metalake.
NameIdentifier[] nameIdentifiers =
Arrays.stream(entities).map(toNameIdentifier).toArray(NameIdentifier[]::new);
- if (enableAuthorization()
- && nameIdentifiers.length > 0
- && allVisibleViaParentScope(metalake, expression, entityType,
nameIdentifiers)) {
- // A privilege granted at a parent scope (metalake/catalog/schema) makes
every object in the
- // list visible, and no object-level deny exists, so the per-object
authorization loop is
- // skipped entirely. See
AuthorizationExpressionConstants.*_LIST_PARENT_SCOPE_*.
- return entities;
+ if (enableAuthorization() && nameIdentifiers.length > 0) {
+ if (METADATA_OBJECT_ENTITY_TYPES.contains(entityType)) {
+ Arrays.stream(nameIdentifiers)
+ .forEach(
+ identifier ->
NameIdentifierUtil.checkMetadataObjectName(identifier, entityType));
+ }
+
+ if (allVisibleViaParentScope(metalake, expression, entityType,
nameIdentifiers)) {
+ // A privilege granted at a parent scope (metalake/catalog/schema)
makes every object in
+ // the list visible, and no object-level deny exists, so the
per-object authorization loop
+ // is skipped entirely. See
AuthorizationExpressionConstants.*_LIST_PARENT_SCOPE_*.
+ return entities;
+ }
}
preloadToCache(entityType, nameIdentifiers);
preloadOwner(entityType, nameIdentifiers);
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/authorization/TestMetadataAuthzHelper.java
b/server-common/src/test/java/org/apache/gravitino/server/authorization/TestMetadataAuthzHelper.java
index 83e575c28a..0b40a63c31 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/authorization/TestMetadataAuthzHelper.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/authorization/TestMetadataAuthzHelper.java
@@ -44,6 +44,7 @@ import org.apache.gravitino.authorization.GravitinoAuthorizer;
import org.apache.gravitino.authorization.Privilege;
import org.apache.gravitino.catalog.SchemaDispatcher;
import org.apache.gravitino.dto.tag.MetadataObjectDTO;
+import org.apache.gravitino.exceptions.IllegalNameIdentifierException;
import
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants;
import org.apache.gravitino.utils.NameIdentifierUtil;
import org.apache.gravitino.utils.PrincipalUtils;
@@ -115,6 +116,49 @@ public class TestMetadataAuthzHelper {
}
}
+ @ParameterizedTest
+ @EnumSource(
+ value = Entity.EntityType.class,
+ names = {"TABLE", "TOPIC"})
+ public void testFilterRejectsDottedExternalObjectName(Entity.EntityType
entityType) {
+ NameIdentifier[] identifiers = {
+ NameIdentifier.of("testMetalake", "testCatalog", "testSchema",
"object.with.dot")
+ };
+
+ IllegalNameIdentifierException exception =
+ Assertions.assertThrows(
+ IllegalNameIdentifierException.class,
+ () ->
+ MetadataAuthzHelper.filterByExpression(
+ "testMetalake", "", entityType, identifiers));
+
+ Assertions.assertEquals(
+ "The "
+ + entityType
+ + " name 'object.with.dot' is unsupported because '.' is reserved
as the "
+ + "qualified-name separator.",
+ exception.getMessage());
+ }
+
+ @Test
+ public void
testFilterPreservesDottedExternalObjectNameWithoutAuthorization() {
+ Config config = gravitinoEnv.config();
+ when(config.get(eq(Configs.ENABLE_AUTHORIZATION))).thenReturn(false);
+ NameIdentifier[] identifiers = {
+ NameIdentifier.of("testMetalake", "testCatalog", "testSchema",
"object.with.dot")
+ };
+
+ try {
+ NameIdentifier[] filtered =
+ MetadataAuthzHelper.filterByExpression(
+ "testMetalake", "", Entity.EntityType.TABLE, identifiers);
+
+ Assertions.assertSame(identifiers, filtered);
+ } finally {
+ when(config.get(eq(Configs.ENABLE_AUTHORIZATION))).thenReturn(true);
+ }
+ }
+
@Test
public void testPreloadUsesInternalDispatchers() throws Exception {
AccessControlDispatcher accessControlDispatcher =
mock(AccessControlDispatcher.class);
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 d515cec330..202e3f6d87 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
@@ -45,6 +45,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.IllegalNameIdentifierException;
import org.apache.gravitino.exceptions.NoSuchMetalakeException;
import org.apache.gravitino.lineage.source.rest.LineageOperations;
import
org.apache.gravitino.listener.api.event.server.AuthorizationDenialFailureEvent;
@@ -255,6 +256,9 @@ public class GravitinoInterceptionService implements
InterceptionService {
}
}
return methodInvocation.proceed();
+ } catch (IllegalNameIdentifierException ex) {
+ LOG.warn("Invalid metadata object identifier during authorization",
ex);
+ return Utils.illegalArguments(ex.getMessage(), ex);
} catch (Exception ex) {
String currentUser = PrincipalUtils.getCurrentUserName();
String methodName = methodInvocation.getMethod().getName();
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 93f3681170..6ff61d736b 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
@@ -59,6 +59,7 @@ import
org.apache.gravitino.server.authorization.annotations.AuthorizationExpres
import
org.apache.gravitino.server.authorization.annotations.AuthorizationMetadata;
import org.apache.gravitino.server.web.Utils;
import org.apache.gravitino.server.web.rest.SchemaOperations;
+import org.apache.gravitino.server.web.rest.TableOperations;
import org.apache.gravitino.server.web.rest.ViewOperations;
import org.apache.gravitino.utils.PrincipalUtils;
import org.apache.gravitino.utils.RequestContext;
@@ -297,6 +298,52 @@ public class TestGravitinoInterceptionService {
}
}
+ @Test
+ public void testDottedMetadataNameReturnsBadRequest() throws Throwable {
+ try (MockedStatic<PrincipalUtils> principalUtilsMocked =
mockStatic(PrincipalUtils.class);
+ MockedStatic<GravitinoAuthorizerProvider> authorizerMocked =
+ mockStatic(GravitinoAuthorizerProvider.class);
+ MockedStatic<AuthorizationUtils> authorizationUtilsMocked =
+ mockStatic(AuthorizationUtils.class)) {
+ principalUtilsMocked
+ .when(PrincipalUtils::getCurrentPrincipal)
+ .thenReturn(new UserPrincipal("tester"));
+
principalUtilsMocked.when(PrincipalUtils::getCurrentUserName).thenReturn("tester");
+ authorizationUtilsMocked
+ .when(
+ () ->
+ AuthorizationUtils.checkCurrentUser(
+ ArgumentMatchers.any(), ArgumentMatchers.any(),
ArgumentMatchers.any()))
+ .thenAnswer(invocation -> null);
+
+ GravitinoAuthorizerProvider provider =
mock(GravitinoAuthorizerProvider.class);
+ GravitinoAuthorizer authorizer = mock(GravitinoAuthorizer.class);
+ when(authorizer.authorize(any(), any(), any(), any(),
any())).thenReturn(true);
+
authorizerMocked.when(GravitinoAuthorizerProvider::getInstance).thenReturn(provider);
+ when(provider.getGravitinoAuthorizer()).thenReturn(authorizer);
+
+ Method method =
+ TableOperations.class.getMethod(
+ "loadTable", String.class, String.class, String.class,
String.class, String.class);
+ MethodInvocation invocation = mock(MethodInvocation.class);
+ when(invocation.getMethod()).thenReturn(method);
+ when(invocation.getArguments())
+ .thenReturn(
+ new Object[] {"testMetalake", "testCatalog", "testSchema",
"sales.2024", null});
+
+ MethodInterceptor interceptor =
+ new
GravitinoInterceptionService().getMethodInterceptors(method).get(0);
+ Response response = (Response) interceptor.invoke(invocation);
+
+ assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
response.getStatus());
+ assertEquals(
+ "The TABLE name 'sales.2024' is unsupported because '.' is reserved
as the "
+ + "qualified-name separator.",
+ ((ErrorResponse) response.getEntity()).getMessage());
+ verify(invocation, never()).proceed();
+ }
+ }
+
@Test
public void testMetalakeNotExist() throws Throwable {
try (MockedStatic<PrincipalUtils> principalUtilsMocked =
mockStatic(PrincipalUtils.class);
