Copilot commented on code in PR #10874:
URL: https://github.com/apache/gravitino/pull/10874#discussion_r3166934031
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -446,18 +481,42 @@ private boolean loadPrivilegeAndAuthorize(
MetadataObject metadataObject,
String privilege,
AuthorizationRequestContext requestContext) {
- Long metadataId;
Long userId;
try {
UserEntity userEntity = getUserEntity(username, metalake);
userId = userEntity.id();
- metadataId = MetadataIdConverter.getID(metadataObject, metalake);
} catch (Exception e) {
LOG.debug("Can not get entity id", e);
return false;
}
loadRolePrivilege(metalake, username, userId, requestContext);
- return authorizeByJcasbin(userId, metalake, metadataObject, metadataId,
privilege);
+
+ // when checking CREATE SCHEMA, the metadata object may be null, we can
just
+ // skip the check
+ if (metadataObject == null) {
+ return false;
+ }
+ // For SCHEMA objects with hierarchical names (e.g. "catalog.A:B:C"),
walk the logical
+ // parent chain so that a privilege granted on an ancestor schema is
inherited by all
+ // descendant schemas.
Review Comment:
This inline comment uses `"catalog.A:B:C"` as the SCHEMA name example, but
`MetadataObject` for SCHEMA generally has `parent=catalog` and `name=A:B:C`.
Please adjust the example/comment wording to reflect that the hierarchy walk is
over the schema *name* segments, not a `catalog.`-prefixed string.
```suggestion
// For SCHEMA objects with hierarchical schema names (for example,
parent=catalog and
// name="A:B:C"), walk the logical parent chain over the schema name
segments so that a
// privilege granted on an ancestor schema is inherited by all
descendant schemas.
```
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -422,6 +440,23 @@ public void close() throws IOException {
}
}
+ /**
+ * Builds the logical schema inheritance chain for a SCHEMA MetadataObject.
For a schema named
+ * {@code "catalog.A:B:C"} this returns MetadataObjects for {@code
catalog.A:B:C}, {@code
+ * catalog.A:B}, and {@code catalog.A} in that order.
Review Comment:
The Javadoc example here implies `schemaObject.name()` includes the catalog
prefix (e.g. `catalog.A:B:C`), but for SCHEMA `MetadataObject`s the catalog is
typically represented as the parent and `name()` is just the schema name (e.g.
`A:B:C`). Please update the Javadoc/example to match the actual
`MetadataObject` structure to avoid confusion for future maintainers.
```suggestion
* Builds the logical schema inheritance chain for a SCHEMA
MetadataObject. For a schema object
* whose parent is {@code catalog} and whose name is {@code "A:B:C"}, this
returns
* MetadataObjects for parent {@code catalog} with schema names {@code
A:B:C}, {@code A:B}, and
* {@code A} in that order.
```
##########
core/src/test/java/org/apache/gravitino/hook/TestSchemaHookDispatcher.java:
##########
@@ -0,0 +1,256 @@
+/*
+ * 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.hook;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.ArgumentMatchers.isNull;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.util.Collections;
+import java.util.Optional;
+import org.apache.commons.lang3.reflect.FieldUtils;
+import org.apache.gravitino.Config;
+import org.apache.gravitino.Configs;
+import org.apache.gravitino.Entity;
+import org.apache.gravitino.GravitinoEnv;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.Schema;
+import org.apache.gravitino.authorization.Owner;
+import org.apache.gravitino.authorization.OwnerDispatcher;
+import org.apache.gravitino.catalog.SchemaDispatcher;
+import org.apache.gravitino.utils.NameIdentifierUtil;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+public class TestSchemaHookDispatcher {
+
+ private static final String METALAKE = "test_metalake";
+ private static final String CATALOG = "test_catalog";
+ private static final String SEPARATOR = ":";
+
+ private SchemaDispatcher mockDispatcher;
+ private OwnerDispatcher mockOwnerDispatcher;
+ private SchemaHookDispatcher hookDispatcher;
+
+ @BeforeEach
+ public void setUp() throws IllegalAccessException {
+ mockDispatcher = mock(SchemaDispatcher.class);
+ mockOwnerDispatcher = mock(OwnerDispatcher.class);
+
+ Config mockConfig = mock(Config.class);
+ when(mockConfig.get(Configs.SCHEMA_SEPARATOR)).thenReturn(SEPARATOR);
+
+ FieldUtils.writeField(GravitinoEnv.getInstance(), "config", mockConfig,
true);
+ FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher",
mockOwnerDispatcher, true);
+
+ hookDispatcher = new SchemaHookDispatcher(mockDispatcher);
+ when(mockOwnerDispatcher.getOwner(any(),
any())).thenReturn(Optional.empty());
+ }
+
+ @AfterEach
+ public void tearDown() throws IllegalAccessException {
+ FieldUtils.writeField(GravitinoEnv.getInstance(), "config", null, true);
+ FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", null,
true);
+ }
Review Comment:
`tearDown()` resets `GravitinoEnv` singleton fields (`config`,
`ownerDispatcher`) to null rather than restoring their original values. This
can leak state across tests and make the suite order-dependent. Please save the
original field values in `setUp()` and restore them in `tearDown()`/`finally`
(similar to `TestCatalogHookDispatcher`).
##########
authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerAuthorizationPluginIT.java:
##########
@@ -556,7 +556,7 @@ public void testInvalidAuthorizationOperation() {
String.format("catalog.schema"),
MetadataObject.Type.SCHEMA,
Lists.newArrayList(Privileges.CreateSchema.allow()));
- Assertions.assertFalse(
+ Assertions.assertTrue(
rangerAuthPlugin.validAuthorizationOperation(Arrays.asList(createSchema1)));
Review Comment:
`testInvalidAuthorizationOperation` now asserts that `CreateSchema` on a
SCHEMA object is valid. This makes the test method name/structure misleading
(it’s mixing valid and invalid cases). Please either move this assertion into
`testValidAuthorizationOperation` or split/rename the test so “invalid” cases
only contain expected-false assertions.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/MetadataObjectService.java:
##########
@@ -572,7 +573,11 @@ public static Map<Long, String>
getSchemaObjectsFullName(List<Long> schemaIds) {
return;
}
- String fullName = DOT_JOINER.join(catalogName,
schemaPO.getSchemaName());
+ String fullName =
+ DOT_JOINER.join(
+ catalogName,
+ HierarchicalSchemaUtil.physicalToLogical(
+ schemaPO.getSchemaName(),
HierarchicalSchemaUtil.schemaSeparator()));
schemaIdAndNameMap.put(schemaPO.getSchemaId(), fullName);
});
Review Comment:
`getSchemaObjectsFullName` now converts schema names from physical
(\u0001-separated) back to logical using the configured schema separator, but
there’s still no unit test covering this behavior (especially for nested schema
names). Please add a test that persists a nested schema name in the relational
store and asserts the returned full name uses the logical separator and does
not expose the physical separator.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -446,18 +481,42 @@ private boolean loadPrivilegeAndAuthorize(
MetadataObject metadataObject,
String privilege,
AuthorizationRequestContext requestContext) {
- Long metadataId;
Long userId;
try {
UserEntity userEntity = getUserEntity(username, metalake);
userId = userEntity.id();
- metadataId = MetadataIdConverter.getID(metadataObject, metalake);
} catch (Exception e) {
LOG.debug("Can not get entity id", e);
return false;
}
loadRolePrivilege(metalake, username, userId, requestContext);
- return authorizeByJcasbin(userId, metalake, metadataObject, metadataId,
privilege);
+
+ // when checking CREATE SCHEMA, the metadata object may be null, we can
just
+ // skip the check
Review Comment:
The comment says “skip the check” when `metadataObject` is null, but the
code returns `false` (deny). Please clarify the comment to reflect the actual
behavior/intent (e.g., “cannot authorize without a metadata object, so deny”),
or adjust the logic if the intent truly is to bypass object-scoped checks while
still allowing higher-scope evaluation.
```suggestion
// For requests such as CREATE SCHEMA, the metadata object may be
null. This method
// performs object-scoped authorization, so without a metadata object
it cannot evaluate
// the request and must deny authorization here.
```
--
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]
