laserninja commented on code in PR #13512:
URL: https://github.com/apache/gravitino/pull/13512#discussion_r4167276280
##########
server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java:
##########
@@ -109,6 +110,7 @@ public Filter getDescriptorFilter() {
ViewOperations.class.getName(),
ModelOperations.class.getName(),
FunctionOperations.class.getName(),
+ SemanticModelOperations.class.getName(),
Review Comment:
Fixed in d5d337d13. List now has the schema authorization gate and filters
identifiers with FILTER_SEMANTIC_MODEL_AUTHORIZATION_EXPRESSION; alter/drop
have their existing expressions and metadata annotations. After integrating
#13571, import/export also use create/load permissions, and REST import shares
native source validation. Interceptor regressions cover denied access without
invoking the endpoint and owner access for all seven methods; list filtering
has a separate regression test. Full server suite: 627 tests passed.
##########
server/src/test/java/org/apache/gravitino/server/web/rest/TestSemanticModelSourceValidator.java:
##########
@@ -0,0 +1,247 @@
+/*
+ * 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.server.web.rest;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoInteractions;
+import static org.mockito.Mockito.when;
+
+import org.apache.gravitino.MetadataObject;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.authorization.GravitinoAuthorizer;
+import org.apache.gravitino.authorization.Privilege;
+import org.apache.gravitino.catalog.TableDispatcher;
+import org.apache.gravitino.catalog.ViewDispatcher;
+import org.apache.gravitino.exceptions.ConnectionFailedException;
+import org.apache.gravitino.exceptions.ForbiddenException;
+import org.apache.gravitino.exceptions.IllegalSemanticModelException;
+import org.apache.gravitino.exceptions.NoSuchTableException;
+import org.apache.gravitino.exceptions.NoSuchViewException;
+import org.apache.gravitino.rel.Column;
+import org.apache.gravitino.rel.Table;
+import org.apache.gravitino.rel.View;
+import org.apache.gravitino.semantic.Dataset;
+import org.apache.gravitino.semantic.Relationship;
+import org.apache.gravitino.semantic.SemanticModelDefinition;
+import org.apache.gravitino.server.authorization.PassThroughAuthorizer;
+import org.junit.jupiter.api.Test;
+
+/** Tests catalog-backed validation independently of model persistence. */
+public class TestSemanticModelSourceValidator {
+ private final TableDispatcher tables = mock(TableDispatcher.class);
+ private final ViewDispatcher views = mock(ViewDispatcher.class);
+ private GravitinoAuthorizer authorizer = new PassThroughAuthorizer();
+ private final SemanticModelSourceValidator validator =
+ new SemanticModelSourceValidator(tables, views, () -> authorizer);
+ private static final NameIdentifier SOURCE =
+ NameIdentifier.of("other_catalog", "schema", "orders");
+ private static final NameIdentifier FULL_SOURCE =
+ NameIdentifier.of("metalake", "other_catalog", "schema", "orders");
Review Comment:
Fixed in d5d337d13: SOURCE and FULL_SOURCE now precede the instance fields.
Formatting checks passed.
##########
server/src/main/java/org/apache/gravitino/server/web/rest/SemanticModelSourceValidator.java:
##########
@@ -0,0 +1,187 @@
+/*
+ * 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.server.web.rest;
+
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.function.Supplier;
+import javax.inject.Inject;
+import org.apache.gravitino.Entity;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.authorization.AuthorizationRequestContext;
+import org.apache.gravitino.authorization.GravitinoAuthorizer;
+import org.apache.gravitino.catalog.TableDispatcher;
+import org.apache.gravitino.catalog.ViewDispatcher;
+import org.apache.gravitino.exceptions.ForbiddenException;
+import org.apache.gravitino.exceptions.IllegalSemanticModelException;
+import org.apache.gravitino.exceptions.NotFoundException;
+import org.apache.gravitino.rel.Column;
+import org.apache.gravitino.semantic.Dataset;
+import org.apache.gravitino.semantic.Relationship;
+import org.apache.gravitino.semantic.SemanticModelDefinition;
+import org.apache.gravitino.server.authorization.GravitinoAuthorizerProvider;
+import
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants;
+import
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionEvaluator;
+import org.apache.gravitino.utils.NameIdentifierUtil;
+
+/** Validates source visibility, existence, and explicit column references
before model writes. */
+public class SemanticModelSourceValidator {
+ private final TableDispatcher tables;
+ private final ViewDispatcher views;
+ private final Supplier<GravitinoAuthorizer> authorizer;
+
+ /**
+ * Creates a validator using the server's authorizer, including its
disabled-mode pass-through.
+ *
+ * @param tables table metadata dispatcher
+ * @param views view metadata dispatcher
+ */
+ @Inject
+ public SemanticModelSourceValidator(TableDispatcher tables, ViewDispatcher
views) {
+ this(tables, views, () ->
GravitinoAuthorizerProvider.getInstance().getGravitinoAuthorizer());
+ }
+
+ SemanticModelSourceValidator(
+ TableDispatcher tables, ViewDispatcher views,
Supplier<GravitinoAuthorizer> authorizer) {
+ this.tables = tables;
+ this.views = views;
+ this.authorizer = authorizer;
+ }
+
+ /**
+ * Validates every dataset source and key or relationship column under the
current caller.
+ *
+ * <p>This method must run even when authorization is disabled. It does not
interpret SQL
+ * expressions or validate transitive view dependencies. Connection failures
propagate unchanged.
+ *
+ * @param metalake the request's metalake, also used for cross-catalog
source references
+ * @param definition the complete definition to validate before persistence
+ */
+ public void validate(String metalake, SemanticModelDefinition definition) {
+ AuthorizationRequestContext context = new AuthorizationRequestContext();
+ GravitinoAuthorizer currentAuthorizer = authorizer.get();
+ Map<String, Set<String>> datasetColumns = new HashMap<>();
+ Map<NameIdentifier, Set<String>> sourceColumns = new HashMap<>();
+ for (Dataset dataset : definition.datasets()) {
+ NameIdentifier source = dataset.source();
+ if (source.namespace().length() != 2) {
+ throw new IllegalSemanticModelException("Source must contain
catalog.schema.name");
+ }
+ NameIdentifier ident =
+ NameIdentifier.of(
+ metalake, source.namespace().level(0),
source.namespace().level(1), source.name());
+ Set<String> columns =
+ sourceColumns.computeIfAbsent(ident, key -> loadColumns(key,
currentAuthorizer, context));
+ datasetColumns.put(dataset.name(), columns);
+ checkColumns(dataset.name(), dataset.primaryKey(), columns);
+ if (dataset.uniqueKeys() != null) {
+ for (String[] key : dataset.uniqueKeys()) {
+ checkColumns(dataset.name(), key, columns);
+ }
+ }
+ }
+ if (definition.relationships() != null) {
+ for (Relationship relationship : definition.relationships()) {
+ checkColumns(
+ relationship.from(),
+ relationship.fromColumns(),
+ datasetColumns.get(relationship.from()));
+ checkColumns(
+ relationship.to(), relationship.toColumns(),
datasetColumns.get(relationship.to()));
+ }
+ }
+ }
+
+ private Set<String> loadColumns(
+ NameIdentifier ident,
+ GravitinoAuthorizer currentAuthorizer,
+ AuthorizationRequestContext context) {
+ boolean tableAllowed =
+ canLoad(
+ ident,
+ Entity.EntityType.TABLE,
+
AuthorizationExpressionConstants.LOAD_TABLE_AUTHORIZATION_EXPRESSION,
+ currentAuthorizer,
+ context);
+ if (tableAllowed) {
+ try {
+ return columnNames(ident, tables.loadTable(ident).columns());
+ } catch (NotFoundException missing) {
+ // The untyped source can also refer to a logical view. Authorize that
lookup separately.
+ }
+ }
+ boolean viewAllowed =
+ canLoad(
+ ident,
+ Entity.EntityType.VIEW,
+
AuthorizationExpressionConstants.LOAD_VIEW_AUTHORIZATION_EXPRESSION,
+ currentAuthorizer,
+ context);
+ if (viewAllowed) {
+ try {
+ return columnNames(ident, views.loadView(ident).columns());
+ } catch (NotFoundException missing) {
Review Comment:
Fixed in d5d337d13. After authorizing the View lookup, the validator checks
whether the catalog operations implement ViewCatalog using a leased
CatalogManager callback. Unsupported View lookup is skipped, so a missing
source returns 400. HTTP regressions reproduced 501 before the fix and now
cover create/replacement with authorization enabled and disabled. Additional
tests exercise capability detection and ensure UnsupportedOperationException
from a supported View operation is not swallowed. All 627 server tests and 90
focused core Semantic Model/Ossie tests passed.
--
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]
