From 66c20372b6fd6a896b290a5e6473f2211b0733b4 Mon Sep 17 00:00:00 2001 From: Ayush Saxena Date: Fri, 3 Jul 2026 15:34:54 +0530 Subject: [PATCH] Fix storage and containment validation for registerView to match registerTable --- .../catalog/iceberg/LocalIcebergCatalog.java | 5 + .../AbstractLocalIcebergCatalogViewTest.java | 162 +++++++++++++++--- 2 files changed, 141 insertions(+), 26 deletions(-) diff --git a/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java b/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java index 5b77cdeb19..16ca3ce20a 100644 --- a/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java +++ b/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java @@ -1080,6 +1080,9 @@ public View registerView(TableIdentifier identifier, String metadataFileLocation throw new IllegalStateException( String.format("Failed to fetch resolved parent for TableIdentifier '%s'", identifier)); } + + validateLocationForTableLike(identifier, metadataFileLocation, resolvedParent); + FileIO fileIO = loadFileIOForTableLike( identifier, @@ -1090,6 +1093,8 @@ public View registerView(TableIdentifier identifier, String metadataFileLocation InputFile metadataFile = fileIO.newInputFile(metadataFileLocation); ViewMetadata metadata = ViewMetadataParser.read(metadataFile); + validateLocationForTableLike(identifier, metadata.location(), resolvedParent); + validateMetadataFileInTableDir(identifier, metadata.location(), metadataFileLocation); ops.commit(null, metadata); return new BaseView(ops, ViewUtil.fullViewName(name(), identifier)); diff --git a/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogViewTest.java b/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogViewTest.java index 55bcbe65c5..443faa10ef 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogViewTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogViewTest.java @@ -18,6 +18,7 @@ */ package org.apache.polaris.service.catalog.iceberg; +import static java.nio.charset.StandardCharsets.UTF_8; import static org.awaitility.Awaitility.await; import com.google.common.collect.ImmutableMap; @@ -32,15 +33,25 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.UUID; import org.apache.iceberg.CatalogProperties; import org.apache.iceberg.catalog.Catalog; +import org.apache.iceberg.catalog.Namespace; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.exceptions.BadRequestException; import org.apache.iceberg.exceptions.CommitFailedException; +import org.apache.iceberg.exceptions.ForbiddenException; +import org.apache.iceberg.inmemory.InMemoryFileIO; import org.apache.iceberg.io.FileIO; import org.apache.iceberg.view.BaseView; +import org.apache.iceberg.view.ImmutableSQLViewRepresentation; +import org.apache.iceberg.view.ImmutableViewVersion; import org.apache.iceberg.view.View; import org.apache.iceberg.view.ViewCatalogTests; import org.apache.iceberg.view.ViewMetadata; +import org.apache.iceberg.view.ViewMetadataParser; import org.apache.iceberg.view.ViewOperations; +import org.apache.iceberg.view.ViewVersion; import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.PolarisDiagnostics; import org.apache.polaris.core.admin.model.CreateCatalogRequest; @@ -54,9 +65,11 @@ import org.apache.polaris.core.context.CallContext; import org.apache.polaris.core.context.RealmContext; import org.apache.polaris.core.entity.CatalogEntity; +import org.apache.polaris.core.entity.PolarisEntity; import org.apache.polaris.core.entity.PrincipalEntity; import org.apache.polaris.core.identity.provider.ServiceIdentityProvider; import org.apache.polaris.core.persistence.PolarisMetaStoreManager; +import org.apache.polaris.core.persistence.dao.entity.EntityResult; import org.apache.polaris.core.persistence.resolver.ResolutionManifestFactory; import org.apache.polaris.core.persistence.resolver.ResolverFactory; import org.apache.polaris.core.secrets.UserSecretsManager; @@ -122,6 +135,7 @@ public abstract class AbstractLocalIcebergCatalogViewTest @Inject FileIOFactory fileIOFactory; private LocalIcebergCatalog catalog; + private PolarisEntity catalogEntity; private String realmName; private PolarisCallContext polarisContext; @@ -172,36 +186,41 @@ public void before(TestInfo testInfo) { authorizer = new PolarisAuthorizerImpl(realmConfig); reservedProperties = ReservedProperties.NONE; - newAdminService() - .createCatalog( - new CreateCatalogRequest( - new CatalogEntity.Builder() - .setName(CATALOG_NAME) - .addProperty( - FeatureConfiguration.ALLOW_EXTERNAL_METADATA_FILE_LOCATION.catalogConfig(), - "true") - .addProperty( - FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), - "true") - .addProperty( - FeatureConfiguration.DROP_WITH_PURGE_ENABLED.catalogConfig(), "true") - .setDefaultBaseLocation("file://tmp") - .setStorageConfigurationInfo( - realmConfig, - new FileStorageConfigInfo( - StorageConfigInfo.StorageTypeEnum.FILE, - List.of("file://tmp", "*"), - null), - "file://tmp") - .build() - .asCatalog(serviceIdentityProvider))); + catalogEntity = + newAdminService() + .createCatalog( + new CreateCatalogRequest( + new CatalogEntity.Builder() + .setName(CATALOG_NAME) + .addProperty( + FeatureConfiguration.ALLOW_EXTERNAL_METADATA_FILE_LOCATION + .catalogConfig(), + "true") + .addProperty( + FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), + "true") + .addProperty( + FeatureConfiguration.DROP_WITH_PURGE_ENABLED.catalogConfig(), "true") + .setDefaultBaseLocation("file://tmp") + .setStorageConfigurationInfo( + realmConfig, + new FileStorageConfigInfo( + StorageConfigInfo.StorageTypeEnum.FILE, + List.of("file://tmp", "*"), + null), + "file://tmp") + .build() + .asCatalog(serviceIdentityProvider))); + testPolarisEventListener = (TestPolarisEventListener) polarisEventListener; + testPolarisEventListener.clear(); + buildCatalog(); + } + + private void buildCatalog() { PolarisPassthroughResolutionView passthroughView = new PolarisPassthroughResolutionView( resolutionManifestFactory, authenticatedRoot, CATALOG_NAME); - - testPolarisEventListener = (TestPolarisEventListener) polarisEventListener; - testPolarisEventListener.clear(); this.catalog = new LocalIcebergCatalog( diagServices, @@ -362,4 +381,95 @@ public void testFailedViewCommitDeletesOrphanMetadataFile() { Assertions.assertThat(orphanCandidates).isNotEmpty(); Assertions.assertThat(deletedLocations).containsAll(orphanCandidates); } + + @Test + public void testRegisterViewRejectsMetadataOutsideAllowedLocations() throws IOException { + // Update catalog to disable permissive location settings + updateCatalogProperties( + Map.of( + FeatureConfiguration.ALLOW_EXTERNAL_METADATA_FILE_LOCATION.catalogConfig(), "false", + FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "false")); + + LocalIcebergCatalog catalog = catalog(); + Namespace ns = Namespace.of("restricted_ns1"); + catalog.createNamespace(ns); + + // Metadata file at a location outside the catalog's allowed locations + String unauthorizedLocation = "s3://unauthorized-bucket/some/path"; + String metadataFileLocation = unauthorizedLocation + "/metadata/v1.metadata.json"; + + addViewMetadataFile(ns, unauthorizedLocation, metadataFileLocation); + + TableIdentifier viewIdentifier = TableIdentifier.of(ns, "unauthorized_view"); + + Assertions.assertThatThrownBy(() -> catalog.registerView(viewIdentifier, metadataFileLocation)) + .isInstanceOf(ForbiddenException.class) + .hasMessageContaining("Invalid locations") + .hasMessageContaining(metadataFileLocation); + } + + @Test + public void testRegisterViewRejectsMetadataFileOutsideViewDir() throws IOException { + // Update catalog: disallow external metadata file location so metadata-in-dir validation + // fires, but keep unstructured locations enabled so the storage validation passes + updateCatalogProperties( + Map.of( + FeatureConfiguration.ALLOW_EXTERNAL_METADATA_FILE_LOCATION.catalogConfig(), "false", + FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true")); + + LocalIcebergCatalog catalog = catalog(); + Namespace ns = Namespace.of("restricted_ns2"); + catalog.createNamespace(ns); + + // View declares its base location, but metadata file is in a different directory + String viewLocation = "file://tmp/restricted_ns2/my_view"; + String metadataFileLocation = "file://tmp/restricted_ns2/other_view/metadata/v1.metadata.json"; + + addViewMetadataFile(ns, viewLocation, metadataFileLocation); + + TableIdentifier viewIdentifier = TableIdentifier.of(ns, "my_view"); + + Assertions.assertThatThrownBy(() -> catalog.registerView(viewIdentifier, metadataFileLocation)) + .isInstanceOf(BadRequestException.class) + .hasMessageContaining("is not allowed outside of table location"); + } + + private static void addViewMetadataFile( + Namespace ns, String viewLocation, String metadataFileLocation) { + ViewVersion viewVersion = + ImmutableViewVersion.builder() + .versionId(1) + .timestampMillis(System.currentTimeMillis()) + .schemaId(0) + .defaultNamespace(ns) + .addRepresentations( + ImmutableSQLViewRepresentation.builder().sql("select 1").dialect("spark").build()) + .build(); + + ViewMetadata viewMetadata = + ViewMetadata.builder() + .assignUUID(UUID.randomUUID().toString()) + .setLocation(viewLocation) + .setCurrentVersion(viewVersion, TestData.SCHEMA) + .build(); + + InMemoryFileIO inMemoryFileIO = new InMemoryFileIO(); + inMemoryFileIO.addFile( + metadataFileLocation, ViewMetadataParser.toJson(viewMetadata).getBytes(UTF_8)); + } + + private void updateCatalogProperties(Map properties) throws IOException { + CatalogEntity.Builder builder = new CatalogEntity.Builder(CatalogEntity.of(catalogEntity)); + properties.forEach(builder::addProperty); + + EntityResult result = + metaStoreManager.updateEntityPropertiesIfNotChanged( + polarisContext, List.of(PolarisEntity.toCore(catalogEntity)), builder.build()); + Assertions.assertThat(result.isSuccess()).isTrue(); + catalogEntity = PolarisEntity.of(result.getEntity()); + // The catalog snapshots its entity (and its feature-flag properties) at construction time, + // so close and rebuild it here to pick up the properties we just wrote. + this.catalog.close(); + buildCatalog(); + } }