This is an automated email from the ASF dual-hosted git repository.
roryqi pushed a commit to branch branch-1.3
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/branch-1.3 by this push:
new 14706f1f9b [Cherry-pick to branch-1.3] [#14476] fix(iceberg-rest):
hide view names from callers who cannot see them, and reject null namespace
property values (#14478)
14706f1f9b is described below
commit 14706f1f9b91db6d0a5981f4709217f2d865724e
Author: Mark Hoerth <[email protected]>
AuthorDate: Mon Nov 2 01:20:57 2026 -0800
[Cherry-pick to branch-1.3] [#14476] fix(iceberg-rest): hide view names
from callers who cannot see them, and reject null namespace property values
(#14478)
Backport to branch-1.3 of the fix for #14476. Same change as the PR to
main.
### What changes were proposed in this pull request?
- `LoadTableAuthzHandler`: a view name on `/tables/` gets 404 only for a
caller who may load the view or check that it exists
(`ICEBERG_LOAD_VIEW_AUTHORIZATION_EXPRESSION` or
`ICEBERG_LOAD_VIEW_EXISTENCE_CHECK_AUTHORIZATION_EXPRESSION` on the
view). That 404 is what lets clients fall back to `/views/`. Anyone else
goes through the normal table authorization with the name treated as
missing, so they get the same answer as for a table name that does not
exist. `IcebergLoadAuthzHandlerHelper` gets an `isAllowed` helper, which
`authorizeLoadEntity` now also uses.
- `IcebergNamespaceOperationExecutor.updateNamespace` rejects null
property values and null removal keys with 400 before the catalog is
called.
### Why are the changes needed?
A member with no privileges could enumerate view names. A null namespace
property value made the namespace unusable through every API.
Fixes #14476
### Does this PR introduce _any_ user-facing change?
Yes. A caller who cannot see a view gets 403 instead of 404 for its name
on `/tables/`. A namespace property update with a null value gets 400. A
namespace that already holds a NULL value is not repaired by this
change.
### How was this patch tested?
- `TestLoadTableAuthzHandler`: a view name is 403 for a caller who
cannot see the view, and 404 for one who can.
-
`TestIcebergNamespaceOperations#testUpdateNamespaceRejectsNullPropertyValue`:
the raw JSON request gets 400, and the namespace still loads and updates
afterwards.
`./gradlew :iceberg:iceberg-rest-server:test -PskipITs`
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01Eu6fb9pLfmrSJzGQDVLi1g
Co-authored-by: Claude Opus 5.5 <[email protected]>
Co-authored-by: roryqi <[email protected]>
---
.../IcebergNamespaceOperationExecutor.java | 26 +++++++++
.../web/filter/IcebergLoadAuthzHandlerHelper.java | 25 +++++----
.../server/web/filter/LoadTableAuthzHandler.java | 30 ++++++++---
.../rest/TestIcebergNamespaceOperations.java | 15 ++++++
.../web/filter/TestLoadTableAuthzHandler.java | 61 ++++++++++++++++++++++
5 files changed, 140 insertions(+), 17 deletions(-)
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergNamespaceOperationExecutor.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergNamespaceOperationExecutor.java
index 94cd918b34..353d46b68e 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergNamespaceOperationExecutor.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergNamespaceOperationExecutor.java
@@ -23,6 +23,9 @@ import com.google.common.annotations.VisibleForTesting;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
+import java.util.Set;
+import java.util.TreeSet;
+import java.util.stream.Collectors;
import org.apache.gravitino.Entity;
import org.apache.gravitino.auth.AuthConstants;
import org.apache.gravitino.catalog.lakehouse.iceberg.IcebergConstants;
@@ -98,6 +101,7 @@ public class IcebergNamespaceOperationExecutor implements
IcebergNamespaceOperat
IcebergRequestContext context,
Namespace namespace,
UpdateNamespacePropertiesRequest updateNamespacePropertiesRequest) {
+ validatePropertyUpdate(updateNamespacePropertiesRequest);
return icebergCatalogWrapperManager
.getCatalogWrapper(context.catalogName())
.updateNamespaceProperties(namespace,
updateNamespacePropertiesRequest);
@@ -148,6 +152,28 @@ public class IcebergNamespaceOperationExecutor implements
IcebergNamespaceOperat
namespace, registerTableRequest, context.requestCredentialVending());
}
+ /**
+ * Rejects a property update that sets a null value or removes a null key.
Iceberg's request
+ * builder refuses these, but a request read from JSON skips the builder,
and the JDBC catalog
+ * would store a null value that makes the namespace unreadable.
+ */
+ @VisibleForTesting
+ static void validatePropertyUpdate(UpdateNamespacePropertiesRequest request)
{
+ Set<String> nullValues =
+ request.updates().entrySet().stream()
+ .filter(entry -> entry.getKey() == null || entry.getValue() ==
null)
+ .map(entry -> String.valueOf(entry.getKey()))
+ .collect(Collectors.toCollection(TreeSet::new));
+ if (!nullValues.isEmpty()) {
+ throw new IllegalArgumentException(
+ String.format(
+ "Invalid value to update for properties %s: null. Use remove
instead", nullValues));
+ }
+ if (request.removals().contains(null)) {
+ throw new IllegalArgumentException("Invalid property to remove: null");
+ }
+ }
+
/**
* With authorization enabled, a namespace is governed as the Gravitino
schema named by its levels
* joined with the schema separator. A level that contains the separator
would give two different
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/IcebergLoadAuthzHandlerHelper.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/IcebergLoadAuthzHandlerHelper.java
index fc03d8aa65..df426feb68 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/IcebergLoadAuthzHandlerHelper.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/IcebergLoadAuthzHandlerHelper.java
@@ -111,6 +111,18 @@ final class IcebergLoadAuthzHandlerHelper {
return defaultExpression;
}
+ static boolean isAllowed(Map<EntityType, NameIdentifier> nameIdentifierMap,
String expression) {
+ return isAllowed(nameIdentifierMap, expression, new
AuthorizationRequestContext());
+ }
+
+ static boolean isAllowed(
+ Map<EntityType, NameIdentifier> nameIdentifierMap,
+ String expression,
+ AuthorizationRequestContext requestContext) {
+ return new AuthorizationExpressionEvaluator(expression)
+ .evaluate(nameIdentifierMap, Collections.emptyMap(), requestContext,
Optional.empty());
+ }
+
static void authorizeLoadEntity(
Map<EntityType, NameIdentifier> nameIdentifierMap,
String primaryExpression,
@@ -119,21 +131,12 @@ final class IcebergLoadAuthzHandlerHelper {
Supplier<RuntimeException> notFoundException,
String entityType,
NameIdentifier entityId) {
- Map<String, Object> emptyPathParams = Collections.emptyMap();
AuthorizationRequestContext requestContext = new
AuthorizationRequestContext();
- Optional<String> emptyEntityType = Optional.empty();
-
- AuthorizationExpressionEvaluator primaryEvaluator =
- new AuthorizationExpressionEvaluator(primaryExpression);
- if (primaryEvaluator.evaluate(
- nameIdentifierMap, emptyPathParams, requestContext, emptyEntityType)) {
+ if (isAllowed(nameIdentifierMap, primaryExpression, requestContext)) {
return;
}
- AuthorizationExpressionEvaluator allowExistenceEvaluator =
- new AuthorizationExpressionEvaluator(allowCheckExistenceExpression);
- if (allowExistenceEvaluator.evaluate(
- nameIdentifierMap, emptyPathParams, requestContext,
emptyEntityType)
+ if (isAllowed(nameIdentifierMap, allowCheckExistenceExpression,
requestContext)
&& !exists.getAsBoolean()) {
throw notFoundException.get();
}
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/LoadTableAuthzHandler.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/LoadTableAuthzHandler.java
index fe24ca53cb..ecbb2188ad 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/LoadTableAuthzHandler.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/LoadTableAuthzHandler.java
@@ -20,6 +20,7 @@
package org.apache.gravitino.server.web.filter;
import java.lang.reflect.Parameter;
+import java.util.HashMap;
import java.util.Map;
import org.apache.gravitino.Entity.EntityType;
import org.apache.gravitino.NameIdentifier;
@@ -113,17 +114,21 @@ public class LoadTableAuthzHandler implements
AuthorizationHandler {
* Perform TABLE-level authorization aligned with {@link
*
org.apache.gravitino.iceberg.service.rest.IcebergTableOperations#loadTable}:
evaluate the
* primary expression first; if it denies, evaluate {@link
- * AuthorizationExpression#allowCheckExistence()} when non-blank. Before
primary expression
- * evaluation, if the catalog supports views and the identifier resolves to
a view, throws {@link
- * NoSuchTableException} so clients can fall back to {@code /views/} (see
Iceberg REST spec for
- * {@code /tables/}).
+ * AuthorizationExpression#allowCheckExistence()} when non-blank.
+ *
+ * <p>If the identifier resolves to a view, a caller who may load the view
or check that it exists
+ * gets {@link NoSuchTableException}, so clients can fall back to {@code
/views/} (see Iceberg
+ * REST spec for {@code /tables/}). Anyone else is answered as for a table
name that does not
+ * exist, so the response does not reveal that a view has that name.
*/
private void performTableAuthorization(
Map<EntityType, NameIdentifier> nameIdentifierMap,
IcebergCatalogWrapper catalogWrapper,
TableIdentifier tableIdentifier,
NameIdentifier tableId) {
- if (catalogWrapper.supportsViewOperations() &&
catalogWrapper.viewExists(tableIdentifier)) {
+ boolean isView =
+ catalogWrapper.supportsViewOperations() &&
catalogWrapper.viewExists(tableIdentifier);
+ if (isView && maySeeView(nameIdentifierMap, tableId)) {
throw new NoSuchTableException("Table %s not found",
tableIdentifier.name());
}
@@ -136,12 +141,25 @@ public class LoadTableAuthzHandler implements
AuthorizationHandler {
authorizationExpression,
AuthorizationExpressionConstants
.ICEBERG_TABLE_EXISTS_SECONDARY_AUTHORIZATION_EXPRESSION),
- () -> catalogWrapper.tableExists(tableIdentifier),
+ () -> !isView && catalogWrapper.tableExists(tableIdentifier),
() -> new NoSuchTableException("Table %s not found",
tableIdentifier.name()),
"table",
tableId);
}
+ private static boolean maySeeView(
+ Map<EntityType, NameIdentifier> nameIdentifierMap, NameIdentifier
tableId) {
+ Map<EntityType, NameIdentifier> viewMap = new HashMap<>(nameIdentifierMap);
+ viewMap.remove(EntityType.TABLE);
+ viewMap.put(EntityType.VIEW, NameIdentifier.of(tableId.namespace(),
tableId.name()));
+ return IcebergLoadAuthzHandlerHelper.isAllowed(
+ viewMap,
AuthorizationExpressionConstants.ICEBERG_LOAD_VIEW_AUTHORIZATION_EXPRESSION)
+ || IcebergLoadAuthzHandlerHelper.isAllowed(
+ viewMap,
+ AuthorizationExpressionConstants
+ .ICEBERG_LOAD_VIEW_EXISTENCE_CHECK_AUTHORIZATION_EXPRESSION);
+ }
+
/**
* Check if the table is a metadata table. Metadata tables have special
names like
* `table$snapshots`, `table$files`, etc., and are accessed with longer
namespace paths
diff --git
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergNamespaceOperations.java
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergNamespaceOperations.java
index 06f7e9c792..156539ffa0 100644
---
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergNamespaceOperations.java
+++
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergNamespaceOperations.java
@@ -384,6 +384,21 @@ public class TestIcebergNamespaceOperations extends
IcebergNamespaceTestBase {
verifyUpdateNamespaceSucc(Namespace.of("update_foo3", "a"));
}
+ @Test
+ void testUpdateNamespaceRejectsNullPropertyValue() {
+ verifyCreateNamespaceSucc("update_null_value");
+ Response response =
+ getUpdateNamespaceClientBuilder(Namespace.of("update_null_value"))
+ .post(
+ Entity.entity(
+ "{\"removals\":[],\"updates\":{\"k\":null}}",
MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(400, response.getStatus());
+
+ // Nothing was stored, so the namespace can still be loaded and changed.
+ verifyLoadNamespaceSucc(Namespace.of("update_null_value"));
+ verifyUpdateNamespaceSucc(Namespace.of("update_null_value"));
+ }
+
@Test
void testRegisterTableWithCredentialVending() {
// register without credential vending -- no credentials in response
diff --git
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/server/web/filter/TestLoadTableAuthzHandler.java
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/server/web/filter/TestLoadTableAuthzHandler.java
index 976fc7fcda..ce94c9c250 100644
---
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/server/web/filter/TestLoadTableAuthzHandler.java
+++
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/server/web/filter/TestLoadTableAuthzHandler.java
@@ -22,6 +22,8 @@ package org.apache.gravitino.server.web.filter;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
@@ -47,12 +49,14 @@ import org.apache.gravitino.utils.NameIdentifierUtil;
import org.apache.iceberg.Schema;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.ForbiddenException;
import org.apache.iceberg.exceptions.NoSuchTableException;
import org.apache.iceberg.exceptions.ServiceUnavailableException;
import org.apache.iceberg.inmemory.InMemoryCatalog;
import org.apache.iceberg.types.Types;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
+import org.mockito.MockedStatic;
import org.mockito.Mockito;
/** Test for {@link LoadTableAuthzHandler}. */
@@ -155,6 +159,63 @@ public class TestLoadTableAuthzHandler {
assertDoesNotThrow(() -> handler.process(baseIdentifierMap()));
}
+ @Test
+ public void testViewNameIsForbiddenToCallersWhoCannotSeeTheView() throws
Exception {
+ InMemoryCatalog catalog = newCatalog();
+ resetContextWithView(catalog);
+ LoadTableAuthzHandler handler = createHandler(new Object[] {TEST_CATALOG +
"/", "db", "v"});
+
+ try (MockedStatic<IcebergLoadAuthzHandlerHelper> helper =
+ Mockito.mockStatic(IcebergLoadAuthzHandlerHelper.class,
Mockito.CALLS_REAL_METHODS)) {
+ helper
+ .when(() -> IcebergLoadAuthzHandlerHelper.isAllowed(any(),
anyString()))
+ .thenReturn(false);
+ helper
+ .when(() -> IcebergLoadAuthzHandlerHelper.isAllowed(any(),
anyString(), any()))
+ .thenReturn(false);
+
+ // Same answer as for a table the caller may not load, so the view's
name is not revealed.
+ assertThrows(ForbiddenException.class, () ->
handler.process(baseIdentifierMap()));
+ }
+ }
+
+ @Test
+ public void testViewNameIs404ToCallersWhoCanSeeTheView() throws Exception {
+ InMemoryCatalog catalog = newCatalog();
+ resetContextWithView(catalog);
+ LoadTableAuthzHandler handler = createHandler(new Object[] {TEST_CATALOG +
"/", "db", "v"});
+
+ try (MockedStatic<IcebergLoadAuthzHandlerHelper> helper =
+ Mockito.mockStatic(IcebergLoadAuthzHandlerHelper.class,
Mockito.CALLS_REAL_METHODS)) {
+ helper
+ .when(
+ () ->
+ IcebergLoadAuthzHandlerHelper.isAllowed(
+ any(),
+ eq(
+ AuthorizationExpressionConstants
+ .ICEBERG_LOAD_VIEW_AUTHORIZATION_EXPRESSION)))
+ .thenReturn(true);
+ helper
+ .when(() -> IcebergLoadAuthzHandlerHelper.isAllowed(any(),
anyString(), any()))
+ .thenReturn(false);
+
+ // The client falls back to /views/ on 404.
+ assertThrows(NoSuchTableException.class, () ->
handler.process(baseIdentifierMap()));
+ }
+ }
+
+ private static void resetContextWithView(InMemoryCatalog catalog) {
+ IcebergCatalogWrapperManager wrapperManager =
Mockito.mock(IcebergCatalogWrapperManager.class);
+ CatalogWrapperForREST catalogWrapper =
Mockito.mock(CatalogWrapperForREST.class);
+
when(wrapperManager.getCatalogWrapper(TEST_CATALOG)).thenReturn(catalogWrapper);
+ when(catalogWrapper.getCatalog()).thenReturn(catalog);
+ when(catalogWrapper.supportsViewOperations()).thenReturn(true);
+ when(catalogWrapper.viewExists(TableIdentifier.of("db",
"v"))).thenReturn(true);
+
when(catalogWrapper.tableExists(any(TableIdentifier.class))).thenReturn(false);
+ resetContext(wrapperManager);
+ }
+
private static LoadTableAuthzHandler createHandler(Object[] args) throws
Exception {
Method method =
TestOperations.class.getMethod("loadTable", String.class,
String.class, String.class);
