Skip to content

Fix storage and containment validation for registerView #4963

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ayushtkn wants to merge 1 commit into from
+141 −26
Closed

Fix storage and containment validation for registerView #4963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ...service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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);

@adutra adutra Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you also need to call validateLocationForTableLike – the metadata.location() value must be valid wrt to the storage configuration from the table hierarchy, AND wrt to the table directory, if escaping the directory is not allowed:

Suggested change
validateMetadataFileInTableDir(identifier, metadata.location(), metadataFileLocation);
validateLocationForTableLike(identifier, metadata.location(), resolvedParent);
validateMetadataFileInTableDir(identifier, metadata.location(), metadataFileLocation);

ops.commit(null, metadata);
Comment on lines 1084 to 1098

@ayushtkn ayushtkn Jul 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this while writing the tests—validateMetadataFileInTableDir strictly reads from the realm-level configuration and ignores the catalog-level overrides, meaning a catalog configured with ALLOW_EXTERNAL_TABLE_LOCATION=true will still be blocked by this check.

Since this exact same split-brain configuration bug currently exists for registerTable, I opted to keep the implementation as-is to bring registerView into strict behavioral parity with tables.

I plan to chase that separately if people agree that ain't deliberate rather than expanding the scope here

@adutra adutra Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ALLOW_EXTERNAL_METADATA_FILE_LOCATION is not overridable at catalog level, but ALLOW_EXTERNAL_TABLE_LOCATION is.

This looks like a bug to me. Do you mind opening an issue for this?


return new BaseView(ops, ViewUtil.fullViewName(name(), identifier));
Expand Down
162 changes: 136 additions & 26 deletions .../java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogViewTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -122,6 +135,7 @@ public abstract class AbstractLocalIcebergCatalogViewTest
@Inject FileIOFactory fileIOFactory;

private LocalIcebergCatalog catalog;
private PolarisEntity catalogEntity;

private String realmName;
private PolarisCallContext polarisContext;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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<String, String> 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();
}
}