lasdf1234 commented on code in PR #14482: URL: https://github.com/apache/gravitino/pull/14482#discussion_r4464894325
########## 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: Thanks — fixed in 896076b24. `RegisterTableAuthzHandler` now evaluates the destination create or modify expression first, then completes authorization (`authorizationCompleted()` always returns true). Metadata FileIO and the catalog scan run only after that check. Added `testCreateDenialChecksDestinationBeforeReadingMetadata` to assert a plain-register destination denial never calls `loadTableMetadataFromLocation` or `listNamespace`. ########## 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: Thanks — fixed in 896076b24. The handler now throws `org.apache.gravitino.exceptions.ForbiddenException`, same as the Lance overwrite handler; `IcebergExceptionMapper` still maps it to the Iceberg REST 403. Added audit coverage on the real register interceptor path: overwrite denials attribute to the named TABLE, and source denials on a plain register attribute to the method's SCHEMA access target. ########## 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: Thanks. Left the fail-closed source matching as-is (no broad catch restored). Documented the per-register catalog traversal / metadata-load cost and the double metadata read in the PR description. Reusing the parsed metadata between authorization and execution looks like a larger cross-layer change; happy to follow up separately if we want that optimization. -- 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]
