mchades commented on code in PR #12867: URL: https://github.com/apache/gravitino/pull/12867#discussion_r4122734591
########## 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); + } + return semanticModel; + } + + @Override + public SemanticModel alterSemanticModel(NameIdentifier ident, SemanticModelChange... changes) + throws NoSuchSemanticModelException, SemanticModelAlreadyExistsException, + IllegalSemanticModelException { + return dispatcher.alterSemanticModel(ident, changes); + } + + @Override + public boolean dropSemanticModel(NameIdentifier ident) { + return dispatcher.dropSemanticModel(ident); Review Comment: [P2] Creation adds an owner relation, but the relational orphan-cleanup registry has no `SEMANTIC_MODEL` entity table. Once a model is soft-deleted, its `owner_meta` and `role_meta_securable_object` rows are never collected. This is part of #12594's deletion-cleanup scope and also preserves stale authorization state. Register `semantic_model_meta` / `semantic_model_id` with the cleanup path and add relational-backend coverage. ########## core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java: ########## @@ -97,7 +97,10 @@ public class AuthorizationUtils { MetadataObject.Type.JOB_TEMPLATE, MetadataObject.Type.TAG, MetadataObject.Type.POLICY, - MetadataObject.Type.VIEW); + MetadataObject.Type.VIEW, + // Semantic models live only in Gravitino, underlying connectors know nothing about + // them, so there is no privilege to push down to an authorization plugin. + MetadataObject.Type.SEMANTIC_MODEL); Review Comment: [P1] `SKIP_APPLY_TYPES` only checks the securable object's type. The three Semantic Model privileges can also bind to `METALAKE`, `CATALOG`, or `SCHEMA`, so those grants still traverse connector authorization plugins and pass Semantic Model privileges downstream. This contradicts the stated Gravitino-only behavior and can make role creation depend on or mutate an external authorizer. Filter these privilege names before dispatch, skip the plugin call when no connector-visible privilege remains, and cover parent-scope and mixed-privilege roles. ########## 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: [P1] Adding the forward name-to-ID entry lets a `SEMANTIC_MODEL` securable object be persisted, but `RoleMetaService` reconstructs persisted securables through `MetadataObjectService.TYPE_TO_FULLNAME_FUNCTION_MAP`, which has no `SEMANTIC_MODEL` entry. A role containing object-level `SELECT_SEMANTIC_MODEL`, `MODIFY_SEMANTIC_MODEL`, or `MANAGE_GRANTS` will therefore fail on the next get, update, or revoke with `Unsupported metadata object type: SEMANTIC_MODEL`. Please add the reverse lookup and a persist-read-update/revoke round-trip test in this PR; deferring it leaves the new public grants unusable. ########## server-common/src/test/java/org/apache/gravitino/server/authorization/TestMetadataIdConverter.java: ########## @@ -153,6 +156,12 @@ void testConvert() throws IllegalAccessException { MetadataIdConverter.normalizeCaseSensitive( eq(ident8), eq(null), eq(mockCatalogManager))) .thenReturn(ident8); + mockedStatic + .when( + () -> + MetadataIdConverter.normalizeCaseSensitive( + eq(ident9), eq(Capability.Scope.SEMANTIC_MODEL), eq(mockCatalogManager))) Review Comment: [P2] This stub bypasses the behavior that needs coverage. The generic `SEMANTIC_MODEL` scope normalization applies the catalog capability to the complete identifier, while `SemanticModelNormalizeDispatcher` intentionally applies catalog rules only to the parent schema and stable/default rules to the model leaf. A capability that lowercases this scope can therefore turn a stored `SalesModel` into a lookup for `salesmodel`, causing owner and role authorization to miss it. Exercise the real normalization path with a capability that changes leaf case, and use Semantic Model-specific normalization that preserves the leaf contract. ########## 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); + } + return semanticModel; + } + + @Override + public SemanticModel alterSemanticModel(NameIdentifier ident, SemanticModelChange... changes) + throws NoSuchSemanticModelException, SemanticModelAlreadyExistsException, + IllegalSemanticModelException { + return dispatcher.alterSemanticModel(ident, changes); Review Comment: [P1] Rename and drop currently leave the authorization name-to-ID cache populated. After a drop followed by name reuse, or a rename to a recently used name, the local node can authorize the new object with a stale ID and the old owner or role relations until asynchronous invalidation catches up. Notify `AuthorizationUtils` after successful rename and drop, including the relevant old and new identifiers, and add a deterministic rename/drop/name-reuse cache test. -- 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]
