This is an automated email from the ASF dual-hosted git repository.
jerryshao pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new a274f7adba [#13222] fix(api): fail closed in TableCatalog.loadTable
default with required privileges (#13223)
a274f7adba is described below
commit a274f7adbabb31fa945435c6bbf860f3af1c6225
Author: YangJie <[email protected]>
AuthorDate: Sun Sep 20 07:29:53 2026 -0400
[#13222] fix(api): fail closed in TableCatalog.loadTable default with
required privileges (#13223)
### What changes were proposed in this pull request?
The default `loadTable(ident, requiredPrivilegeNames)` now throws
`UnsupportedOperationException` instead of delegating to
`loadTable(ident)`, matching the convention used for other unsupported
capability defaults.
### Why are the changes needed?
The default silently discarded the required privileges while the javadoc
promised privilege-aware loading, so a caller relying on it as an
access-control checkpoint got the table with no enforcement and no
signal.
Fix: #13222
### Does this PR introduce _any_ user-facing change?
Yes. The default `loadTable(ident, requiredPrivilegeNames)` now throws
`UnsupportedOperationException` instead of silently ignoring the
privileges. This is a behavior change for any out-of-tree catalog that
relied on the default; the only in-repo override, `RelationalCatalog`,
forwards the privileges to the server and is unaffected.
### How was this patch tested?
Added `TestTableCatalog`, which pins that the default `loadTable(ident,
requiredPrivilegeNames)` throws `UnsupportedOperationException`; it
fails on the pre-fix tree (the default returned the table and dropped
the privileges) and passes after the fix.
Co-authored-by: Jerry Shao <[email protected]>
---
.../org/apache/gravitino/rel/TableCatalog.java | 9 ++-
.../org/apache/gravitino/rel/TestTableCatalog.java | 88 ++++++++++++++++++++++
2 files changed, 96 insertions(+), 1 deletion(-)
diff --git a/api/src/main/java/org/apache/gravitino/rel/TableCatalog.java
b/api/src/main/java/org/apache/gravitino/rel/TableCatalog.java
index 888526ffd5..6f564760f3 100644
--- a/api/src/main/java/org/apache/gravitino/rel/TableCatalog.java
+++ b/api/src/main/java/org/apache/gravitino/rel/TableCatalog.java
@@ -65,14 +65,21 @@ public interface TableCatalog {
/**
* Load table metadata by {@link NameIdentifier} from the catalog with
required privileges.
*
+ * <p>The default implementation throws {@link
UnsupportedOperationException}: a catalog that
+ * cannot enforce the required privileges must fail closed instead of
silently returning the table
+ * metadata. Catalogs that support privilege-aware loading must override
this method.
+ *
* @param ident A table identifier.
* @param requiredPrivilegeNames The required privilege names to access the
table.
* @return The table metadata.
* @throws NoSuchTableException If the table does not exist.
+ * @throws UnsupportedOperationException If the catalog does not support
loading a table with
+ * required privileges.
*/
default Table loadTable(NameIdentifier ident, Set<Privilege.Name>
requiredPrivilegeNames)
throws NoSuchTableException {
- return loadTable(ident);
+ throw new UnsupportedOperationException(
+ "The catalog does not support loading a table with required
privileges");
}
/**
diff --git a/api/src/test/java/org/apache/gravitino/rel/TestTableCatalog.java
b/api/src/test/java/org/apache/gravitino/rel/TestTableCatalog.java
new file mode 100644
index 0000000000..50281dad13
--- /dev/null
+++ b/api/src/test/java/org/apache/gravitino/rel/TestTableCatalog.java
@@ -0,0 +1,88 @@
+/*
+ * 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.rel;
+
+import com.google.common.collect.ImmutableSet;
+import java.util.Map;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.Namespace;
+import org.apache.gravitino.authorization.Privilege;
+import org.apache.gravitino.exceptions.NoSuchSchemaException;
+import org.apache.gravitino.exceptions.NoSuchTableException;
+import org.apache.gravitino.rel.expressions.distributions.Distribution;
+import org.apache.gravitino.rel.expressions.sorts.SortOrder;
+import org.apache.gravitino.rel.expressions.transforms.Transform;
+import org.apache.gravitino.rel.indexes.Index;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class TestTableCatalog {
+
+ private static TableCatalog minimalCatalog() {
+ return new TableCatalog() {
+ @Override
+ public NameIdentifier[] listTables(Namespace namespace) throws
NoSuchSchemaException {
+ return new NameIdentifier[0];
+ }
+
+ @Override
+ public Table loadTable(NameIdentifier ident) throws NoSuchTableException
{
+ return null;
+ }
+
+ @Override
+ public Table createTable(
+ NameIdentifier ident,
+ Column[] columns,
+ String comment,
+ Map<String, String> properties,
+ Transform[] partitions,
+ Distribution distribution,
+ SortOrder[] sortOrders,
+ Index[] indexes) {
+ return null;
+ }
+
+ @Override
+ public Table alterTable(NameIdentifier ident, TableChange... changes)
+ throws NoSuchTableException {
+ return null;
+ }
+
+ @Override
+ public boolean dropTable(NameIdentifier ident) {
+ return false;
+ }
+ };
+ }
+
+ @Test
+ void testLoadTableWithRequiredPrivilegesFailsClosedByDefault() {
+ TableCatalog catalog = minimalCatalog();
+
+ // Before the fix, the default silently discarded the required privileges
and returned the
+ // table as if no privilege check was requested.
+ Assertions.assertThrows(
+ UnsupportedOperationException.class,
+ () ->
+ catalog.loadTable(
+ NameIdentifier.of("schema", "table"),
+ ImmutableSet.of(Privilege.Name.SELECT_TABLE)));
+ }
+}
