Skip to content

Commit 4ed1d41

Browse files
flyrainflyrain
authored
Service: Always validate allowed locations from Storage Config (#2473)
1 parent 17a359b commit 4ed1d41

File tree

9 files changed

+367
-197
lines changed

9 files changed

+367
-197
lines changed

‎integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogViewIntegrationBase.java‎

Lines changed: 5 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020

2121
import static org.apache.polaris.service.it.env.PolarisClient.polarisClient;
2222

23-
import com.google.common.collect.ImmutableMap;
2423
import java.lang.reflect.Method;
2524
import java.nio.file.Path;
2625
import java.nio.file.Paths;
@@ -88,6 +87,7 @@ public abstract class PolarisRestCatalogViewIntegrationBase extends ViewCatalogT
8887
private static ManagementApi managementApi;
8988

9089
private RESTCatalog restCatalog;
90+
private StorageConfigInfo storageConfig;
9191

9292
@BeforeAll
9393
static void setup(PolarisApiEndpoints apiEndpoints, ClientCredentials credentials) {
@@ -114,7 +114,7 @@ public void before(TestInfo testInfo) {
114114
Method method = testInfo.getTestMethod().orElseThrow();
115115
String catalogName = client.newEntityName(method.getName());
116116

117-
StorageConfigInfo storageConfig = getStorageConfigInfo();
117+
storageConfig = getStorageConfigInfo();
118118
String defaultBaseLocation =
119119
storageConfig.getAllowedLocations().getFirst()
120120
+ "/"
@@ -201,16 +201,10 @@ public void createViewWithCustomMetadataLocationUsingPolaris(@TempDir Path tempD
201201
TableIdentifier identifier = TableIdentifier.of("ns", "view");
202202

203203
String location = Paths.get(tempDir.toUri().toString()).toString();
204-
String customLocation = Paths.get(tempDir.toUri().toString(), "custom-location").toString();
205-
String customLocation2 = Paths.get(tempDir.toUri().toString(), "custom-location2").toString();
206-
String customLocationChild =
207-
Paths.get(tempDir.toUri().toString(), "custom-location/child").toString();
204+
String customLocation =
205+
Paths.get(storageConfig.getAllowedLocations().getFirst(), "/custom-location1").toString();
208206

209-
catalog()
210-
.createNamespace(
211-
identifier.namespace(),
212-
ImmutableMap.of(
213-
IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY, location));
207+
catalog().createNamespace(identifier.namespace());
214208

215209
Assertions.assertThat(catalog().viewExists(identifier)).as("View should not exist").isFalse();
216210

@@ -234,35 +228,5 @@ public void createViewWithCustomMetadataLocationUsingPolaris(@TempDir Path tempD
234228
Assertions.assertThat(((BaseView) view).operations().current().metadataFileLocation())
235229
.isNotNull()
236230
.startsWith(customLocation);
237-
238-
// CANNOT update the view with a new metadata location `baseLocation/customLocation2`,
239-
// even though the new location is still under the parent namespace's
240-
// `write.metadata.path=baseLocation`.
241-
Assertions.assertThatThrownBy(
242-
() ->
243-
catalog()
244-
.loadView(identifier)
245-
.updateProperties()
246-
.set(
247-
IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY,
248-
customLocation2)
249-
.commit())
250-
.isInstanceOf(ForbiddenException.class)
251-
.hasMessageContaining("Forbidden: Invalid locations");
252-
253-
// CANNOT update the view with a child metadata location `baseLocation/customLocation/child`,
254-
// even though it is a subpath of the original view's
255-
// `write.metadata.path=baseLocation/customLocation`.
256-
Assertions.assertThatThrownBy(
257-
() ->
258-
catalog()
259-
.loadView(identifier)
260-
.updateProperties()
261-
.set(
262-
IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY,
263-
customLocationChild)
264-
.commit())
265-
.isInstanceOf(ForbiddenException.class)
266-
.hasMessageContaining("Forbidden: Invalid locations");
267231
}
268232
}

‎polaris-core/src/main/java/org/apache/polaris/core/storage/InMemoryStorageIntegration.java‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -53,16 +53,15 @@ protected InMemoryStorageIntegration(T config, String identifierOrId) {
5353
* implementation, all actions have the same validation result, as we only verify the
5454
* locations are equal to or subdirectories of the allowed locations.
5555
*/
56-
public static Map<String, Map<PolarisStorageActions, ValidationResult>>
57-
validateSubpathsOfAllowedLocations(
58-
@Nonnull RealmConfig realmConfig,
59-
@Nonnull PolarisStorageConfigurationInfo storageConfig,
60-
@Nonnull Set<PolarisStorageActions> actions,
61-
@Nonnull Set<String> locations) {
56+
public static Map<String, Map<PolarisStorageActions, ValidationResult>> validateAllowedLocations(
57+
@Nonnull RealmConfig realmConfig,
58+
@Nonnull List<String> allowedLocationsToValid,
59+
@Nonnull Set<PolarisStorageActions> actions,
60+
@Nonnull Set<String> locations) {
6261
// trim trailing / from allowed locations so that locations missing the trailing slash still
6362
// match
6463
Set<String> allowedLocationStrings =
65-
storageConfig.getAllowedLocations().stream()
64+
allowedLocationsToValid.stream()
6665
.map(
6766
str -> {
6867
if (str.endsWith("/") && str.length() > 1) {
@@ -123,6 +122,7 @@ public Map<String, Map<PolarisStorageActions, ValidationResult>> validateAccessT
123122
@Nonnull T storageConfig,
124123
@Nonnull Set<PolarisStorageActions> actions,
125124
@Nonnull Set<String> locations) {
126-
return validateSubpathsOfAllowedLocations(realmConfig, storageConfig, actions, locations);
125+
return validateAllowedLocations(
126+
realmConfig, storageConfig.getAllowedLocations(), actions, locations);
127127
}
128128
}

‎polaris-core/src/main/java/org/apache/polaris/core/storage/LocationRestrictions.java‎

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
package org.apache.polaris.core.storage;
20+
21+
import jakarta.annotation.Nonnull;
22+
import java.util.List;
23+
import java.util.Set;
24+
import org.apache.iceberg.catalog.TableIdentifier;
25+
import org.apache.iceberg.exceptions.ForbiddenException;
26+
import org.apache.polaris.core.config.RealmConfig;
27+
import org.slf4j.Logger;
28+
import org.slf4j.LoggerFactory;
29+
30+
/** Defines storage location access restrictions for Polaris entities within a specific context. */
31+
public class LocationRestrictions {
32+
private static final Logger LOGGER = LoggerFactory.getLogger(LocationRestrictions.class);
33+
34+
/**
35+
* The complete set of storage locations that are permitted for access.
36+
*
37+
* <p>This list contains all storage URIs that entities can read from or write to, including both
38+
* catalog-level allowed locations and any additional user-specified locations when unstructured
39+
* table access is enabled.
40+
*
41+
* <p>All locations in this list have been validated to conform to the storage type's URI scheme
42+
* requirements during construction.
43+
*/
44+
private final List<String> allowedLocations;
45+
46+
/**
47+
* The parent location for structured table enforcement.
48+
*
49+
* <p>When non-null, this location represents the root under which all new tables must be created,
50+
* enforcing a structured hierarchy in addition to residing under {@code allowedLocations}. When
51+
* null, table creation is allowed anywhere within the {@code allowedLocations}.
52+
*/
53+
private final String parentLocation;
54+
55+
public LocationRestrictions(
56+
@Nonnull PolarisStorageConfigurationInfo storageConfigurationInfo, String parentLocation) {
57+
this.allowedLocations = storageConfigurationInfo.getAllowedLocations();
58+
allowedLocations.forEach(storageConfigurationInfo::validatePrefixForStorageType);
59+
this.parentLocation = parentLocation;
60+
}
61+
62+
public LocationRestrictions(@Nonnull PolarisStorageConfigurationInfo storageConfigurationInfo) {
63+
this(storageConfigurationInfo, null);
64+
}
65+
66+
/**
67+
* Validates that the requested storage locations are permitted for the given table identifier.
68+
*
69+
* <p>This method performs location validation by checking the requested locations against:
70+
*
71+
* <ul>
72+
* <li>The parent location (if configured) for structured table enforcement
73+
* <li>The allowed locations list for general access permissions
74+
* </ul>
75+
*
76+
* <p>The validation ensures that all requested locations conform to the storage access
77+
* restrictions defined for this context. If a parent location is configured, all requests must be
78+
* under that location hierarchy in addition to being within the allowed locations.
79+
*
80+
* @param realmConfig the realm configuration containing storage validation rules
81+
* @param identifier the table identifier for which locations are being validated
82+
* @param requestLocations the set of storage locations that need validation
83+
* @throws ForbiddenException if any of the requested locations violate the configured
84+
* restrictions
85+
*/
86+
public void validate(
87+
RealmConfig realmConfig, TableIdentifier identifier, Set<String> requestLocations) {
88+
if (parentLocation != null) {
89+
validateLocations(realmConfig, List.of(parentLocation), requestLocations, identifier);
90+
}
91+
92+
validateLocations(realmConfig, allowedLocations, requestLocations, identifier);
93+
}
94+
95+
private void validateLocations(
96+
RealmConfig realmConfig,
97+
List<String> allowedLocations,
98+
Set<String> requestLocations,
99+
TableIdentifier identifier) {
100+
var validationResults =
101+
InMemoryStorageIntegration.validateAllowedLocations(
102+
realmConfig, allowedLocations, Set.of(PolarisStorageActions.ALL), requestLocations);
103+
validationResults
104+
.values()
105+
.forEach(
106+
actionResult ->
107+
actionResult
108+
.values()
109+
.forEach(
110+
result -> {
111+
if (!result.isSuccess()) {
112+
throw new ForbiddenException(
113+
"Invalid locations '%s' for identifier '%s': %s",
114+
requestLocations, identifier, result.getMessage());
115+
} else {
116+
LOGGER.debug(
117+
"Validated locations '{}' for identifier '{}'",
118+
requestLocations,
119+
identifier);
120+
}
121+
}));
122+
}
123+
}

‎polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageConfigurationInfo.java‎

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -26,14 +26,12 @@
2626
import com.fasterxml.jackson.core.JsonProcessingException;
2727
import com.fasterxml.jackson.databind.DeserializationFeature;
2828
import com.fasterxml.jackson.databind.ObjectMapper;
29-
import com.google.common.collect.ImmutableList;
3029
import jakarta.annotation.Nonnull;
3130
import java.util.ArrayList;
3231
import java.util.Collections;
3332
import java.util.List;
3433
import java.util.Locale;
3534
import java.util.Optional;
36-
import java.util.Set;
3735
import org.apache.polaris.core.admin.model.Catalog;
3836
import org.apache.polaris.core.config.FeatureConfiguration;
3937
import org.apache.polaris.core.config.RealmConfig;
@@ -116,7 +114,7 @@ public static PolarisStorageConfigurationInfo deserialize(final @Nonnull String
116114
}
117115
}
118116

119-
public static Optional<PolarisStorageConfigurationInfo> forEntityPath(
117+
public static Optional<LocationRestrictions> forEntityPath(
120118
RealmConfig realmConfig, List<PolarisEntity> entityPath) {
121119
return findStorageInfoFromHierarchy(entityPath)
122120
.map(
@@ -150,22 +148,13 @@ public static Optional<PolarisStorageConfigurationInfo> forEntityPath(
150148
LOGGER.debug(
151149
"Not allowing unstructured table location for entity: {}",
152150
entityPathReversed.get(0).getName());
153-
return new StorageConfigurationOverride(configInfo, List.of(baseLocation));
151+
return new LocationRestrictions(configInfo, baseLocation);
154152
} else {
155153
LOGGER.debug(
156154
"Allowing unstructured table location for entity: {}",
157155
entityPathReversed.get(0).getName());
158156

159-
// TODO: figure out the purpose of adding `userSpecifiedWriteLocations`
160-
Set<String> locations =
161-
StorageUtil.getLocationsAllowedToBeAccessed(
162-
null, entityPathReversed.get(0).getPropertiesAsMap());
163-
return new StorageConfigurationOverride(
164-
configInfo,
165-
ImmutableList.<String>builder()
166-
.addAll(configInfo.getAllowedLocations())
167-
.addAll(locations)
168-
.build());
157+
return new LocationRestrictions(configInfo);
169158
}
170159
});
171160
}

‎polaris-core/src/main/java/org/apache/polaris/core/storage/StorageConfigurationOverride.java‎

Lines changed: 0 additions & 68 deletions
This file was deleted.

‎polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -70,12 +70,12 @@ public class StorageUtil {
7070
}
7171

7272
/** Given a TableMetadata, extracts the locations where the table's [meta]data might be found. */
73-
public static @Nonnull Set<String> getLocationsAllowedToBeAccessed(TableMetadata tableMetadata) {
74-
return getLocationsAllowedToBeAccessed(tableMetadata.location(), tableMetadata.properties());
73+
public static @Nonnull Set<String> getLocationsUsedByTable(TableMetadata tableMetadata) {
74+
return getLocationsUsedByTable(tableMetadata.location(), tableMetadata.properties());
7575
}
7676

7777
/** Given a baseLocation and entity (table?) properties, extracts the relevant locations */
78-
public static @Nonnull Set<String> getLocationsAllowedToBeAccessed(
78+
public static @Nonnull Set<String> getLocationsUsedByTable(
7979
String baseLocation, Map<String, String> properties) {
8080
Set<String> locations = new HashSet<>();
8181
locations.add(baseLocation);
@@ -87,7 +87,7 @@ public class StorageUtil {
8787
}
8888

8989
/** Given a ViewMetadata, extracts the locations where the view's [meta]data might be found. */
90-
public static @Nonnull Set<String> getLocationsAllowedToBeAccessed(ViewMetadata viewMetadata) {
90+
public static @Nonnull Set<String> getLocationsUsedByTable(ViewMetadata viewMetadata) {
9191
return Set.of(viewMetadata.location());
9292
}
9393

0 commit comments

Comments
 (0)