yuqi1129 commented on code in PR #14482: URL: https://github.com/apache/gravitino/pull/14482#discussion_r4464526479
########## iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/RegisterTableAuthzHandler.java: ########## @@ -0,0 +1,188 @@ +/* + * 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.filter; + +import static org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants.LOAD_TABLE_AUTHORIZATION_EXPRESSION; +import static org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants.MODIFY_TABLE_AUTHORIZATION_EXPRESSION; + +import java.lang.reflect.Parameter; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.regex.Pattern; +import org.apache.gravitino.Entity.EntityType; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.authorization.AuthorizationRequestContext; +import org.apache.gravitino.iceberg.service.CatalogWrapperForREST; +import org.apache.gravitino.iceberg.service.authorization.IcebergRESTServerContext; +import org.apache.gravitino.iceberg.service.dispatcher.RegisterTableSourceResolver; +import org.apache.gravitino.server.authorization.annotations.IcebergAuthorizationMetadata; +import org.apache.gravitino.server.authorization.expression.AuthorizationExpressionEvaluator; +import org.apache.gravitino.server.web.filter.BaseMetadataAuthorizationMethodInterceptor.AuthorizationHandler; +import org.apache.gravitino.utils.HierarchicalSchemaUtil; +import org.apache.gravitino.utils.NameIdentifierUtil; +import org.apache.gravitino.utils.PrincipalUtils; +import org.apache.iceberg.TableMetadata; +import org.apache.iceberg.catalog.Namespace; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.exceptions.ForbiddenException; +import org.apache.iceberg.rest.requests.RegisterTableRequest; + +/** + * Authorizes Iceberg REST {@code registerTable} requests that the method annotation alone cannot + * express. + * + * <p>An overwrite replaces the named table, so it is a modification rather than a create: this + * handler evaluates the modify-table expression against that table itself and completes + * authorization, so {@code CREATE_TABLE} alone cannot replace a table the caller does not own. A + * plain register falls through to the create expression on the method. Either way, when the + * metadata file still belongs to another live table, the load-table expression on that table is + * required, so {@code CREATE_TABLE} alone cannot adopt an unreadable table's metadata. + */ +@SuppressWarnings("FormatStringAnnotation") +public class RegisterTableAuthzHandler implements AuthorizationHandler { + + private static final Pattern SCHEMA_SPLIT = + Pattern.compile(Pattern.quote(HierarchicalSchemaUtil.schemaSeparator())); + + private final Parameter[] parameters; + private final Object[] args; + + /** + * Creates a handler for the register-table request arguments. + * + * @param parameters invoked method parameters + * @param args invoked method arguments + */ + public RegisterTableAuthzHandler(Parameter[] parameters, Object[] args) { + this.parameters = parameters; + this.args = args; + } + + @Override + public void process(Map<EntityType, NameIdentifier> nameIdentifierMap) { + RegisterTableRequest request = registerTableRequest(); + + NameIdentifier metalakeIdent = nameIdentifierMap.get(EntityType.METALAKE); + NameIdentifier catalogIdent = nameIdentifierMap.get(EntityType.CATALOG); + NameIdentifier schemaIdent = nameIdentifierMap.get(EntityType.SCHEMA); + if (metalakeIdent == null || catalogIdent == null || schemaIdent == null) { + throw new ForbiddenException( + "Missing metalake, catalog, or schema context for register-table authorization"); + } + + String metalakeName = metalakeIdent.name(); + String catalog = catalogIdent.name(); + String schema = schemaIdent.name(); + String tableName = request.name(); + nameIdentifierMap.put( + EntityType.TABLE, NameIdentifierUtil.ofTable(metalakeName, catalog, schema, tableName)); + + if (request.overwrite()) { + requireCanOverwriteTable(nameIdentifierMap); + } + authorizeMetadataSources(metalakeName, catalog, schema, tableName, request); Review Comment: [Important] Could we check the destination create permission before reading metadata and scanning sources for a plain register, just as the overwrite branch checks its destination permission first? `BaseMetadataAuthorizationMethodInterceptor` runs `process()` before evaluating the method's create expression. Consequently, `overwrite=false` reaches this call even when the user has no CREATE_TABLE permission. It opens the user-supplied metadata file and can traverse the entire catalog before eventually returning 403; a storage/metadata failure can also replace that denial with a different error. I reproduced this through the real interceptor with USE_CATALOG + USE_SCHEMA but no CREATE_TABLE: the final response is 403, yet `loadTableMetadataFromLocation` was invoked. If that read throws IllegalArgumentException, the same unauthorized request instead returns 500. Please finish the destination authorization before any FileIO/source discovery. Choosing the create-versus-modify expression up front inside this existing handler and then completing authorization would preserve the previous review's approach without reintroducing shared expression-selection hooks. A symmetric test should verify that a plain-register destination denial performs no metadata reads or catalog scan. ########## iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/RegisterTableAuthzHandler.java: ########## @@ -0,0 +1,188 @@ +/* + * 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.filter; + +import static org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants.LOAD_TABLE_AUTHORIZATION_EXPRESSION; +import static org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants.MODIFY_TABLE_AUTHORIZATION_EXPRESSION; + +import java.lang.reflect.Parameter; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.regex.Pattern; +import org.apache.gravitino.Entity.EntityType; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.authorization.AuthorizationRequestContext; +import org.apache.gravitino.iceberg.service.CatalogWrapperForREST; +import org.apache.gravitino.iceberg.service.authorization.IcebergRESTServerContext; +import org.apache.gravitino.iceberg.service.dispatcher.RegisterTableSourceResolver; +import org.apache.gravitino.server.authorization.annotations.IcebergAuthorizationMetadata; +import org.apache.gravitino.server.authorization.expression.AuthorizationExpressionEvaluator; +import org.apache.gravitino.server.web.filter.BaseMetadataAuthorizationMethodInterceptor.AuthorizationHandler; +import org.apache.gravitino.utils.HierarchicalSchemaUtil; +import org.apache.gravitino.utils.NameIdentifierUtil; +import org.apache.gravitino.utils.PrincipalUtils; +import org.apache.iceberg.TableMetadata; +import org.apache.iceberg.catalog.Namespace; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.exceptions.ForbiddenException; +import org.apache.iceberg.rest.requests.RegisterTableRequest; + +/** + * Authorizes Iceberg REST {@code registerTable} requests that the method annotation alone cannot + * express. + * + * <p>An overwrite replaces the named table, so it is a modification rather than a create: this + * handler evaluates the modify-table expression against that table itself and completes + * authorization, so {@code CREATE_TABLE} alone cannot replace a table the caller does not own. A + * plain register falls through to the create expression on the method. Either way, when the + * metadata file still belongs to another live table, the load-table expression on that table is + * required, so {@code CREATE_TABLE} alone cannot adopt an unreadable table's metadata. + */ +@SuppressWarnings("FormatStringAnnotation") +public class RegisterTableAuthzHandler implements AuthorizationHandler { + + private static final Pattern SCHEMA_SPLIT = + Pattern.compile(Pattern.quote(HierarchicalSchemaUtil.schemaSeparator())); + + private final Parameter[] parameters; + private final Object[] args; + + /** + * Creates a handler for the register-table request arguments. + * + * @param parameters invoked method parameters + * @param args invoked method arguments + */ + public RegisterTableAuthzHandler(Parameter[] parameters, Object[] args) { + this.parameters = parameters; + this.args = args; + } + + @Override + public void process(Map<EntityType, NameIdentifier> nameIdentifierMap) { + RegisterTableRequest request = registerTableRequest(); + + NameIdentifier metalakeIdent = nameIdentifierMap.get(EntityType.METALAKE); + NameIdentifier catalogIdent = nameIdentifierMap.get(EntityType.CATALOG); + NameIdentifier schemaIdent = nameIdentifierMap.get(EntityType.SCHEMA); + if (metalakeIdent == null || catalogIdent == null || schemaIdent == null) { + throw new ForbiddenException( + "Missing metalake, catalog, or schema context for register-table authorization"); + } + + String metalakeName = metalakeIdent.name(); + String catalog = catalogIdent.name(); + String schema = schemaIdent.name(); + String tableName = request.name(); + nameIdentifierMap.put( + EntityType.TABLE, NameIdentifierUtil.ofTable(metalakeName, catalog, schema, tableName)); + + if (request.overwrite()) { + requireCanOverwriteTable(nameIdentifierMap); + } + authorizeMetadataSources(metalakeName, catalog, schema, tableName, request); + } + + @Override + public boolean authorizationCompleted() { + // An overwrite is fully authorized in process(); a plain register still needs the create + // expression on the method. + return registerTableRequest().overwrite(); + } + + @Override + public EntityType entityType(EntityType defaultType) { + return registerTableRequest().overwrite() ? EntityType.TABLE : defaultType; + } + + private static void requireCanOverwriteTable(Map<EntityType, NameIdentifier> nameIdentifierMap) { + boolean authorized = + new AuthorizationExpressionEvaluator(MODIFY_TABLE_AUTHORIZATION_EXPRESSION) + .evaluate( + nameIdentifierMap, + new HashMap<>(), + new AuthorizationRequestContext(), + Optional.of(EntityType.TABLE.name())); + if (!authorized) { + throw new ForbiddenException( Review Comment: [Important] These handler denials use `org.apache.iceberg.exceptions.ForbiddenException`, whereas the shared interceptor only dispatches an AuthorizationDenialFailureEvent for `org.apache.gravitino.exceptions.ForbiddenException`. The same mismatch affects the source-table denial below. HTTP mapping still produces 403, but neither denial reaches the audit event path, despite the new `entityType()` hook. I reproduced this with the real register interceptor and a mocked GravitinoEnv/EventBus: an overwrite denial returns 403, but the event bus has zero interactions. The new shared-interceptor test misses it because its synthetic handler throws the Gravitino exception. Could we use the Gravitino ForbiddenException here, as the Lance overwrite handler does, and leave conversion to the Iceberg exception to the protocol mapper? Please exercise the actual register handler's overwrite/source denials in the audit tests, including the expected audit attribution. ########## iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/RegisterTableAuthzHandler.java: ########## @@ -0,0 +1,188 @@ +/* + * 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.filter; + +import static org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants.LOAD_TABLE_AUTHORIZATION_EXPRESSION; +import static org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants.MODIFY_TABLE_AUTHORIZATION_EXPRESSION; + +import java.lang.reflect.Parameter; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.regex.Pattern; +import org.apache.gravitino.Entity.EntityType; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.authorization.AuthorizationRequestContext; +import org.apache.gravitino.iceberg.service.CatalogWrapperForREST; +import org.apache.gravitino.iceberg.service.authorization.IcebergRESTServerContext; +import org.apache.gravitino.iceberg.service.dispatcher.RegisterTableSourceResolver; +import org.apache.gravitino.server.authorization.annotations.IcebergAuthorizationMetadata; +import org.apache.gravitino.server.authorization.expression.AuthorizationExpressionEvaluator; +import org.apache.gravitino.server.web.filter.BaseMetadataAuthorizationMethodInterceptor.AuthorizationHandler; +import org.apache.gravitino.utils.HierarchicalSchemaUtil; +import org.apache.gravitino.utils.NameIdentifierUtil; +import org.apache.gravitino.utils.PrincipalUtils; +import org.apache.iceberg.TableMetadata; +import org.apache.iceberg.catalog.Namespace; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.exceptions.ForbiddenException; +import org.apache.iceberg.rest.requests.RegisterTableRequest; + +/** + * Authorizes Iceberg REST {@code registerTable} requests that the method annotation alone cannot + * express. + * + * <p>An overwrite replaces the named table, so it is a modification rather than a create: this + * handler evaluates the modify-table expression against that table itself and completes + * authorization, so {@code CREATE_TABLE} alone cannot replace a table the caller does not own. A + * plain register falls through to the create expression on the method. Either way, when the + * metadata file still belongs to another live table, the load-table expression on that table is + * required, so {@code CREATE_TABLE} alone cannot adopt an unreadable table's metadata. + */ +@SuppressWarnings("FormatStringAnnotation") +public class RegisterTableAuthzHandler implements AuthorizationHandler { + + private static final Pattern SCHEMA_SPLIT = + Pattern.compile(Pattern.quote(HierarchicalSchemaUtil.schemaSeparator())); + + private final Parameter[] parameters; + private final Object[] args; + + /** + * Creates a handler for the register-table request arguments. + * + * @param parameters invoked method parameters + * @param args invoked method arguments + */ + public RegisterTableAuthzHandler(Parameter[] parameters, Object[] args) { + this.parameters = parameters; + this.args = args; + } + + @Override + public void process(Map<EntityType, NameIdentifier> nameIdentifierMap) { + RegisterTableRequest request = registerTableRequest(); + + NameIdentifier metalakeIdent = nameIdentifierMap.get(EntityType.METALAKE); + NameIdentifier catalogIdent = nameIdentifierMap.get(EntityType.CATALOG); + NameIdentifier schemaIdent = nameIdentifierMap.get(EntityType.SCHEMA); + if (metalakeIdent == null || catalogIdent == null || schemaIdent == null) { + throw new ForbiddenException( + "Missing metalake, catalog, or schema context for register-table authorization"); + } + + String metalakeName = metalakeIdent.name(); + String catalog = catalogIdent.name(); + String schema = schemaIdent.name(); + String tableName = request.name(); + nameIdentifierMap.put( + EntityType.TABLE, NameIdentifierUtil.ofTable(metalakeName, catalog, schema, tableName)); + + if (request.overwrite()) { + requireCanOverwriteTable(nameIdentifierMap); + } + authorizeMetadataSources(metalakeName, catalog, schema, tableName, request); + } + + @Override + public boolean authorizationCompleted() { + // An overwrite is fully authorized in process(); a plain register still needs the create + // expression on the method. + return registerTableRequest().overwrite(); + } + + @Override + public EntityType entityType(EntityType defaultType) { + return registerTableRequest().overwrite() ? EntityType.TABLE : defaultType; + } + + private static void requireCanOverwriteTable(Map<EntityType, NameIdentifier> nameIdentifierMap) { + boolean authorized = + new AuthorizationExpressionEvaluator(MODIFY_TABLE_AUTHORIZATION_EXPRESSION) + .evaluate( + nameIdentifierMap, + new HashMap<>(), + new AuthorizationRequestContext(), + Optional.of(EntityType.TABLE.name())); + if (!authorized) { + throw new ForbiddenException( + "User '%s' is not authorized to overwrite table '%s'", + PrincipalUtils.getCurrentUserName(), nameIdentifierMap.get(EntityType.TABLE)); + } + } + + private void authorizeMetadataSources( + String metalakeName, + String catalog, + String schema, + String tableName, + RegisterTableRequest request) { + CatalogWrapperForREST catalogWrapper = + IcebergRESTServerContext.getInstance().catalogWrapperManager().getCatalogWrapper(catalog); + TableMetadata metadata = + catalogWrapper.loadTableMetadataFromLocation(request.metadataLocation()); Review Comment: [Nit] This adds another read/parse of the request metadata file before `IcebergNamespaceOperationExecutor.registerTable()` reads it again for schema validation. Could we reuse the parsed metadata between authorization and execution where practical? It would also help to state the lookup's cost and availability scope in the PR description: every authorized register traverses all namespaces/tables in this catalog and may load each unrelated table's current metadata. An unreadable unrelated table can therefore block registration too. Keeping unknown ownership fail-closed is the right security choice; please do not restore the broad exception catch to avoid that cost. Documenting the limitation would make the tradeoff explicit without requiring a global ownership index in this patch. This observation is from the call chain, not a large-catalog benchmark. -- 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]
