Copilot commented on code in PR #12867: URL: https://github.com/apache/gravitino/pull/12867#discussion_r4122850364
########## core/src/main/java/org/apache/gravitino/hook/SemanticModelHookDispatcher.java: ########## @@ -0,0 +1,112 @@ +/* + * 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 java.util.Map; +import java.util.function.Supplier; +import javax.annotation.Nullable; +import org.apache.gravitino.Entity; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.authorization.Owner; +import org.apache.gravitino.authorization.OwnerDispatcher; +import org.apache.gravitino.catalog.SemanticModelDispatcher; +import org.apache.gravitino.exceptions.IllegalSemanticModelException; +import org.apache.gravitino.exceptions.NoSuchSchemaException; +import org.apache.gravitino.exceptions.NoSuchSemanticModelException; +import org.apache.gravitino.exceptions.SemanticModelAlreadyExistsException; +import org.apache.gravitino.semantic.SemanticModel; +import org.apache.gravitino.semantic.SemanticModelChange; +import org.apache.gravitino.semantic.SemanticModelDefinition; +import org.apache.gravitino.utils.NameIdentifierUtil; +import org.apache.gravitino.utils.PrincipalUtils; + +/** + * {@code SemanticModelHookDispatcher} is a decorator for {@link SemanticModelDispatcher} that not + * only delegates Semantic Model operations to the underlying dispatcher but also executes some hook + * operations before or after the underlying operations. + */ +public class SemanticModelHookDispatcher implements SemanticModelDispatcher { + + private final SemanticModelDispatcher dispatcher; + private final Supplier<OwnerDispatcher> ownerDispatcher; + + /** + * Creates a Semantic Model hook dispatcher. + * + * @param dispatcher The underlying dispatcher. + * @param ownerDispatcher Supplies the owner dispatcher, or null when authorization is disabled. + */ + public SemanticModelHookDispatcher( + SemanticModelDispatcher dispatcher, Supplier<OwnerDispatcher> ownerDispatcher) { + this.dispatcher = dispatcher; + this.ownerDispatcher = ownerDispatcher; + } + + @Override + public NameIdentifier[] listSemanticModels(Namespace namespace) throws NoSuchSchemaException { + return dispatcher.listSemanticModels(namespace); + } + + @Override + public SemanticModel loadSemanticModel(NameIdentifier ident) throws NoSuchSemanticModelException { + return dispatcher.loadSemanticModel(ident); + } + + @Override + public boolean semanticModelExists(NameIdentifier ident) { + return dispatcher.semanticModelExists(ident); + } + + @Override + public SemanticModel createSemanticModel( + NameIdentifier ident, + @Nullable String comment, + SemanticModelDefinition definition, + Map<String, String> properties) + throws NoSuchSchemaException, SemanticModelAlreadyExistsException, + IllegalSemanticModelException { + SemanticModel semanticModel = + dispatcher.createSemanticModel(ident, comment, definition, properties); + + // Set the creator as the owner of the Semantic Model. + OwnerDispatcher ownerManager = ownerDispatcher.get(); + if (ownerManager != null) { + ownerManager.setOwner( + ident.namespace().level(0), + NameIdentifierUtil.toMetadataObject(ident, Entity.EntityType.SEMANTIC_MODEL), + PrincipalUtils.getCurrentUserName(), + Owner.Type.USER); Review Comment: When authorization is enabled in the full server, this invokes `OwnerManager.setOwner` for every create. The relational owner path locks the owned row through `OwnerMetaSQLProviderFactory.selectMetadataObjectIdForUpdate`, whose entity-type switch has no `SEMANTIC_MODEL` case and therefore throws `Unsupported owned object type: SEMANTIC_MODEL`. As a result, authenticated Semantic Model creation fails after the model is created; add the corresponding storage lock support (and related cleanup) before enabling this hook. ########## server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataIdConverter.java: ########## @@ -50,7 +50,8 @@ public class MetadataIdConverter { MetadataObject.Type.MODEL, Capability.Scope.MODEL, MetadataObject.Type.FILESET, Capability.Scope.FILESET, MetadataObject.Type.TOPIC, Capability.Scope.TOPIC, - MetadataObject.Type.COLUMN, Capability.Scope.COLUMN); + MetadataObject.Type.COLUMN, Capability.Scope.COLUMN, + MetadataObject.Type.SEMANTIC_MODEL, Capability.Scope.SEMANTIC_MODEL); Review Comment: Adding `SEMANTIC_MODEL` to this generic scope map makes `MetadataIdConverter.normalizeCaseSensitive` normalize the leaf name as well as the parent: `CapabilityHelpers.applyCaseSensitive` applies the scope to `ident.name()` too. However, `SemanticModelNormalizeDispatcher` intentionally preserves Semantic Model names during lookup, and creation persists names with the Gravitino-owned casing. A model created as `Sales_Model` in a case-insensitive catalog will therefore be looked up as `sales_model` during authorization ID resolution, causing owner/privilege checks to miss it. Normalize only the parent namespace here while retaining the model name. ########## api/src/main/java/org/apache/gravitino/authorization/Privileges.java: ########## @@ -107,7 +115,8 @@ public class Privileges { MetadataObject.Type.TOPIC, MetadataObject.Type.FILESET, MetadataObject.Type.MODEL, - MetadataObject.Type.FUNCTION); + MetadataObject.Type.FUNCTION, + MetadataObject.Type.SEMANTIC_MODEL); Review Comment: The new `MANAGE_GRANTS_SUPPORTED_TYPES` entry is not covered by the adjacent binding test: `TestSecurableObjects.testPrivileges` checks FUNCTION as the last supported leaf but never asserts that MANAGE_GRANTS binds to SEMANTIC_MODEL. Add the positive assertion so this newly introduced per-model grant capability cannot regress unnoticed. ########## core/src/test/java/org/apache/gravitino/authorization/TestAuthorizationUtils.java: ########## @@ -516,6 +516,35 @@ void testRemoveTablePrivilegesNotifiesAuthorizationPluginWithExpectedChange() { Assertions.assertEquals(locations, removeChange.getLocations()); } + @Test + void testSemanticModelPrivilegesAreNotPushedToAuthorizationPlugin() { + // Semantic Models exist only in Gravitino, so underlying connectors have nothing to revoke or + // rename. Rename and remove must leave the authorization plugin untouched. + NameIdentifier ident = NameIdentifier.of("metalake", "catalog", "schema", "sales_model"); + + AccessControlDispatcher accessControlDispatcher = Mockito.mock(AccessControlDispatcher.class); + CatalogManager catalogManager = Mockito.mock(CatalogManager.class); + BaseCatalog<?> baseCatalog = Mockito.mock(BaseCatalog.class); + AuthorizationPlugin authorizationPlugin = Mockito.mock(AuthorizationPlugin.class); + CatalogTestUtils.mockDoWithCatalog(catalogManager, baseCatalog); + Mockito.when(baseCatalog.getAuthorizationPlugin()).thenReturn(authorizationPlugin); + + GravitinoEnv envMock = Mockito.mock(GravitinoEnv.class); + Mockito.when(envMock.accessControlDispatcher()).thenReturn(accessControlDispatcher); Review Comment: This stubs `accessControlDispatcher()`, but `authorizationPluginRenamePrivileges` and `authorizationPluginRemovePrivileges` guard on `internalAccessControlDispatcher()`. Mockito therefore returns null for the method actually used and the test exits before consulting the catalog plugin, so it would pass even if semantic-model changes were pushed down. Stub `internalAccessControlDispatcher()` instead. -- 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]
