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 7856f0ee19 [#12875] improve(server): Reuse the shared table
authorization expressions (#12876)
7856f0ee19 is described below
commit 7856f0ee196d7512a127dde4534ef72c8e67a16f
Author: Qi Yu <[email protected]>
AuthorDate: Fri Sep 4 14:34:34 2026 +0800
[#12875] improve(server): Reuse the shared table authorization expressions
(#12876)
### What changes were proposed in this pull request?
Adds `DROP_TABLE_AUTHORIZATION_EXPRESSION` to
`AuthorizationExpressionConstants` and replaces 13 hand-written copies
of three table authorization expressions with references to the shared
constants:
| Sites | Constant |
| --- | --- |
| `TableOperations#dropTable`, `IcebergTableOperations#dropTable`,
`RenameTableAuthzHandler` | `DROP_TABLE_AUTHORIZATION_EXPRESSION` (new)
|
| `IcebergTableOperations#updateTable`, `IcebergTableRenameOperations`,
four `StatisticOperations` endpoints |
`MODIFY_TABLE_AUTHORIZATION_EXPRESSION` (existing) |
| `IcebergTableOperations` credentials and plan, two
`StatisticOperations` endpoints | `LOAD_TABLE_AUTHORIZATION_EXPRESSION`
(existing) |
Also removes a local variable in `RenameTableAuthzHandler` that only
aliased the expression.
This is behavior preserving. No privilege rule changes.
Lance REST has a fourth copy of the removal expression, added in #12696,
which is not merged yet. The two PRs are independent; whichever lands
second, the Lance copy will be pointed at the same shared constant in a
follow-up so all four surfaces read from one place.
Fix: #12875
### Why are the changes needed?
A privilege change to one of these rules currently has to be repeated in
every copy, and missing one is a silent authorization difference between
the Gravitino, Iceberg REST and Lance REST surfaces rather than a
visible failure. Two copies had already drifted in formatting
(`ANY_SELECT_TABLE|| ANY_MODIFY_TABLE`, missing the space), which is
evidence that they are edited independently.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
The expressions were compared before and after rather than by eye. A
throwaway test ran each removed literal and its replacement constant
through `AuthorizationExpressionConverter.convertToOgnlExpression`,
asserted the results are identical ignoring whitespace, and parsed both
with `Ognl.parseExpression` to confirm they are still valid OGNL.
Whitespace around operators turned out to be the only difference, from
the two `StatisticOperations` copies that were missing a space before
`||`.
`./gradlew :server-common:test :server:test
:iceberg:iceberg-rest-server:test` — 1119 tests, all passing.
`TestHttpsServerAuthentication` is a pre-existing flake and is
unrelated: a different method of that class fails the same way on a
branch that does not touch `server-common` at all.
---
.../service/rest/IcebergTableOperations.java | 24 ++++++----------
.../service/rest/IcebergTableRenameOperations.java | 7 ++---
.../server/web/filter/RenameTableAuthzHandler.java | 9 ++----
.../AuthorizationExpressionConstants.java | 12 ++++++++
.../server/web/rest/StatisticOperations.java | 33 ++++++----------------
.../gravitino/server/web/rest/TableOperations.java | 8 ++----
6 files changed, 37 insertions(+), 56 deletions(-)
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java
index 4bedb7c41e..898f12747e 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java
@@ -18,6 +18,10 @@
*/
package org.apache.gravitino.iceberg.service.rest;
+import static
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants.DROP_TABLE_AUTHORIZATION_EXPRESSION;
+import static
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants.LOAD_TABLE_AUTHORIZATION_EXPRESSION;
+import static
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants.MODIFY_TABLE_AUTHORIZATION_EXPRESSION;
+
import com.codahale.metrics.annotation.ResponseMetered;
import com.codahale.metrics.annotation.Timed;
import com.fasterxml.jackson.core.JsonProcessingException;
@@ -206,10 +210,7 @@ public class IcebergTableOperations {
@Timed(name = "update-table." + MetricNames.HTTP_PROCESS_DURATION, absolute
= true)
@ResponseMetered(name = "update-table", absolute = true)
@AuthorizationExpression(
- expression =
- "ANY(OWNER, METALAKE, CATALOG) || "
- + "SCHEMA_OWNER_WITH_USE_CATALOG || "
- + "ANY_USE_CATALOG && ANY_USE_SCHEMA && (TABLE::OWNER ||
ANY_MODIFY_TABLE)",
+ expression = MODIFY_TABLE_AUTHORIZATION_EXPRESSION,
accessMetadataType = MetadataObject.Type.TABLE)
public Response updateTable(
@AuthorizationMetadata(type = Entity.EntityType.CATALOG)
@PathParam("prefix") String prefix,
@@ -252,10 +253,7 @@ public class IcebergTableOperations {
@Timed(name = "drop-table." + MetricNames.HTTP_PROCESS_DURATION, absolute =
true)
@ResponseMetered(name = "drop-table", absolute = true)
@AuthorizationExpression(
- expression =
- "ANY(OWNER, METALAKE, CATALOG) || "
- + "SCHEMA_OWNER_WITH_USE_CATALOG || "
- + "ANY_USE_CATALOG && ANY_USE_SCHEMA && TABLE::OWNER ",
+ expression = DROP_TABLE_AUTHORIZATION_EXPRESSION,
accessMetadataType = MetadataObject.Type.TABLE)
public Response dropTable(
@AuthorizationMetadata(type = Entity.EntityType.CATALOG)
@PathParam("prefix") String prefix,
@@ -459,10 +457,7 @@ public class IcebergTableOperations {
@Timed(name = "get-table-credentials." + MetricNames.HTTP_PROCESS_DURATION,
absolute = true)
@ResponseMetered(name = "get-table-credentials", absolute = true)
@AuthorizationExpression(
- expression =
- "ANY(OWNER, METALAKE, CATALOG) || "
- + "SCHEMA_OWNER_WITH_USE_CATALOG || "
- + "ANY_USE_CATALOG && ANY_USE_SCHEMA && (TABLE::OWNER ||
ANY_SELECT_TABLE || ANY_MODIFY_TABLE)",
+ expression = LOAD_TABLE_AUTHORIZATION_EXPRESSION,
accessMetadataType = MetadataObject.Type.TABLE)
public Response getTableCredentials(
@AuthorizationMetadata(type = Entity.EntityType.CATALOG)
@PathParam("prefix") String prefix,
@@ -514,10 +509,7 @@ public class IcebergTableOperations {
@Timed(name = "plan-table-scan." + MetricNames.HTTP_PROCESS_DURATION,
absolute = true)
@ResponseMetered(name = "plan-table-scan", absolute = true)
@AuthorizationExpression(
- expression =
- "ANY(OWNER, METALAKE, CATALOG) || "
- + "SCHEMA_OWNER_WITH_USE_CATALOG || "
- + "ANY_USE_CATALOG && ANY_USE_SCHEMA && (TABLE::OWNER ||
ANY_SELECT_TABLE || ANY_MODIFY_TABLE)",
+ expression = LOAD_TABLE_AUTHORIZATION_EXPRESSION,
accessMetadataType = MetadataObject.Type.TABLE)
public Response planTableScan(
@PathParam("prefix") @AuthorizationMetadata(type = EntityType.CATALOG)
String prefix,
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableRenameOperations.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableRenameOperations.java
index 09c5f6e72a..95555cfe3e 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableRenameOperations.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableRenameOperations.java
@@ -18,6 +18,8 @@
*/
package org.apache.gravitino.iceberg.service.rest;
+import static
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants.MODIFY_TABLE_AUTHORIZATION_EXPRESSION;
+
import com.codahale.metrics.annotation.ResponseMetered;
import com.codahale.metrics.annotation.Timed;
import com.google.common.annotations.VisibleForTesting;
@@ -67,10 +69,7 @@ public class IcebergTableRenameOperations {
@Timed(name = "rename-table." + MetricNames.HTTP_PROCESS_DURATION, absolute
= true)
@ResponseMetered(name = "rename-table", absolute = true)
@AuthorizationExpression(
- expression =
- "ANY(OWNER, METALAKE, CATALOG) || "
- + "SCHEMA_OWNER_WITH_USE_CATALOG || "
- + "ANY_USE_CATALOG && ANY_USE_SCHEMA && (TABLE::OWNER ||
ANY_MODIFY_TABLE)",
+ expression = MODIFY_TABLE_AUTHORIZATION_EXPRESSION,
accessMetadataType = MetadataObject.Type.TABLE)
public Response renameTable(
@AuthorizationMetadata(type = Entity.EntityType.CATALOG)
@PathParam("prefix") String prefix,
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/RenameTableAuthzHandler.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/RenameTableAuthzHandler.java
index 4df2f41068..07c480bf70 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/RenameTableAuthzHandler.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/RenameTableAuthzHandler.java
@@ -19,6 +19,8 @@
package org.apache.gravitino.server.web.filter;
+import static
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants.DROP_TABLE_AUTHORIZATION_EXPRESSION;
+
import java.lang.reflect.Parameter;
import java.util.HashMap;
import java.util.Map;
@@ -134,13 +136,8 @@ public class RenameTableAuthzHandler implements
AuthorizationHandler {
EntityType.TABLE,
NameIdentifierUtil.ofTable(metalakeName, catalog, sourceSchema,
sourceTable));
- String sourceExpression =
- "ANY(OWNER, METALAKE, CATALOG) || "
- + "SCHEMA_OWNER_WITH_USE_CATALOG || "
- + "ANY_USE_CATALOG && ANY_USE_SCHEMA && TABLE::OWNER";
-
AuthorizationExpressionEvaluator sourceEvaluator =
- new AuthorizationExpressionEvaluator(sourceExpression);
+ new
AuthorizationExpressionEvaluator(DROP_TABLE_AUTHORIZATION_EXPRESSION);
boolean sourceAuthorized =
sourceEvaluator.evaluate(
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionConstants.java
b/server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionConstants.java
index 7aadba3f2c..548281ed9f 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionConstants.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionConstants.java
@@ -100,6 +100,18 @@ public class AuthorizationExpressionConstants {
ANY_USE_CATALOG && ANY_USE_SCHEMA && (TABLE::OWNER ||
ANY_MODIFY_TABLE)
""";
+ /**
+ * Authorizes removing a table, whether the stored data is deleted with it
or only the Gravitino
+ * metadata is. Removal requires ownership of the table or of one of its
ancestors: MODIFY_TABLE
+ * alters a table but never removes it.
+ */
+ public static final String DROP_TABLE_AUTHORIZATION_EXPRESSION =
+ """
+ ANY(OWNER, METALAKE, CATALOG) ||
+ SCHEMA_OWNER_WITH_USE_CATALOG ||
+ ANY_USE_CATALOG && ANY_USE_SCHEMA && TABLE::OWNER
+ """;
+
public static final String LOAD_TOPICS_AUTHORIZATION_EXPRESSION =
"""
ANY(OWNER, METALAKE, CATALOG) ||
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/rest/StatisticOperations.java
b/server/src/main/java/org/apache/gravitino/server/web/rest/StatisticOperations.java
index 4d750aa05e..1c11abb958 100644
---
a/server/src/main/java/org/apache/gravitino/server/web/rest/StatisticOperations.java
+++
b/server/src/main/java/org/apache/gravitino/server/web/rest/StatisticOperations.java
@@ -18,6 +18,9 @@
*/
package org.apache.gravitino.server.web.rest;
+import static
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants.LOAD_TABLE_AUTHORIZATION_EXPRESSION;
+import static
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants.MODIFY_TABLE_AUTHORIZATION_EXPRESSION;
+
import com.codahale.metrics.annotation.ResponseMetered;
import com.codahale.metrics.annotation.Timed;
import com.google.common.annotations.VisibleForTesting;
@@ -95,10 +98,7 @@ public class StatisticOperations {
@Timed(name = "list-stats." + MetricNames.HTTP_PROCESS_DURATION, absolute =
true)
@ResponseMetered(name = "list-stats", absolute = true)
@AuthorizationExpression(
- expression =
- "ANY(OWNER, METALAKE, CATALOG) || "
- + "SCHEMA_OWNER_WITH_USE_CATALOG || "
- + "ANY_USE_CATALOG && ANY_USE_SCHEMA && (TABLE::OWNER ||
ANY_SELECT_TABLE|| ANY_MODIFY_TABLE)",
+ expression = LOAD_TABLE_AUTHORIZATION_EXPRESSION,
accessMetadataType = MetadataObject.Type.TABLE)
public Response listStatistics(
@PathParam("metalake") @AuthorizationMetadata(type =
Entity.EntityType.METALAKE)
@@ -140,10 +140,7 @@ public class StatisticOperations {
@Timed(name = "update-stats." + MetricNames.HTTP_PROCESS_DURATION, absolute
= true)
@ResponseMetered(name = "update-stats", absolute = true)
@AuthorizationExpression(
- expression =
- "ANY(OWNER, METALAKE, CATALOG) || "
- + "SCHEMA_OWNER_WITH_USE_CATALOG || "
- + "ANY_USE_CATALOG && ANY_USE_SCHEMA && (TABLE::OWNER ||
ANY_MODIFY_TABLE)",
+ expression = MODIFY_TABLE_AUTHORIZATION_EXPRESSION,
accessMetadataType = MetadataObject.Type.TABLE)
public Response updateStatistics(
@PathParam("metalake") @AuthorizationMetadata(type =
Entity.EntityType.METALAKE)
@@ -200,10 +197,7 @@ public class StatisticOperations {
@Timed(name = "drop-stats." + MetricNames.HTTP_PROCESS_DURATION, absolute =
true)
@ResponseMetered(name = "drop-stats", absolute = true)
@AuthorizationExpression(
- expression =
- "ANY(OWNER, METALAKE, CATALOG) || "
- + "SCHEMA_OWNER_WITH_USE_CATALOG || "
- + "ANY_USE_CATALOG && ANY_USE_SCHEMA && (TABLE::OWNER ||
ANY_MODIFY_TABLE)",
+ expression = MODIFY_TABLE_AUTHORIZATION_EXPRESSION,
accessMetadataType = MetadataObject.Type.TABLE)
public Response dropStatistics(
@PathParam("metalake") @AuthorizationMetadata(type =
Entity.EntityType.METALAKE)
@@ -259,10 +253,7 @@ public class StatisticOperations {
@Timed(name = "list-partition-stats." + MetricNames.HTTP_PROCESS_DURATION,
absolute = true)
@ResponseMetered(name = "list-partition-stats", absolute = true)
@AuthorizationExpression(
- expression =
- "ANY(OWNER, METALAKE, CATALOG) || "
- + "SCHEMA_OWNER_WITH_USE_CATALOG || "
- + "ANY_USE_CATALOG && ANY_USE_SCHEMA && (TABLE::OWNER ||
ANY_SELECT_TABLE|| ANY_MODIFY_TABLE)",
+ expression = LOAD_TABLE_AUTHORIZATION_EXPRESSION,
accessMetadataType = MetadataObject.Type.TABLE)
public Response listPartitionStatistics(
@PathParam("metalake") @AuthorizationMetadata(type =
Entity.EntityType.METALAKE)
@@ -349,10 +340,7 @@ public class StatisticOperations {
@Timed(name = "update-partitions-stats." +
MetricNames.HTTP_PROCESS_DURATION, absolute = true)
@ResponseMetered(name = "update-partitions-stats", absolute = true)
@AuthorizationExpression(
- expression =
- "ANY(OWNER, METALAKE, CATALOG) || "
- + "SCHEMA_OWNER_WITH_USE_CATALOG || "
- + "ANY_USE_CATALOG && ANY_USE_SCHEMA && (TABLE::OWNER ||
ANY_MODIFY_TABLE)",
+ expression = MODIFY_TABLE_AUTHORIZATION_EXPRESSION,
accessMetadataType = MetadataObject.Type.TABLE)
public Response updatePartitionStatistics(
@PathParam("metalake") @AuthorizationMetadata(type =
Entity.EntityType.METALAKE)
@@ -420,10 +408,7 @@ public class StatisticOperations {
@Timed(name = "drop-partitions-stats." + MetricNames.HTTP_PROCESS_DURATION,
absolute = true)
@ResponseMetered(name = "drop-partitions-stats", absolute = true)
@AuthorizationExpression(
- expression =
- "ANY(OWNER, METALAKE, CATALOG) || "
- + "SCHEMA_OWNER_WITH_USE_CATALOG || "
- + "ANY_USE_CATALOG && ANY_USE_SCHEMA && (TABLE::OWNER ||
ANY_MODIFY_TABLE)",
+ expression = MODIFY_TABLE_AUTHORIZATION_EXPRESSION,
accessMetadataType = MetadataObject.Type.TABLE)
public Response dropPartitionStatistics(
@PathParam("metalake") @AuthorizationMetadata(type =
Entity.EntityType.METALAKE)
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/rest/TableOperations.java
b/server/src/main/java/org/apache/gravitino/server/web/rest/TableOperations.java
index 1b0eb4d90c..69fd0cc669 100644
---
a/server/src/main/java/org/apache/gravitino/server/web/rest/TableOperations.java
+++
b/server/src/main/java/org/apache/gravitino/server/web/rest/TableOperations.java
@@ -20,6 +20,7 @@ package org.apache.gravitino.server.web.rest;
import static org.apache.gravitino.dto.util.DTOConverters.fromDTO;
import static org.apache.gravitino.dto.util.DTOConverters.fromDTOs;
+import static
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants.DROP_TABLE_AUTHORIZATION_EXPRESSION;
import com.codahale.metrics.annotation.ResponseMetered;
import com.codahale.metrics.annotation.Timed;
@@ -257,12 +258,7 @@ public class TableOperations {
@Timed(name = "drop-table." + MetricNames.HTTP_PROCESS_DURATION, absolute =
true)
@ResponseMetered(name = "drop-table", absolute = true)
@AuthorizationExpression(
- expression =
- """
- ANY(OWNER, METALAKE, CATALOG) ||
- SCHEMA_OWNER_WITH_USE_CATALOG ||
- ANY_USE_CATALOG && ANY_USE_SCHEMA && TABLE::OWNER
- """,
+ expression = DROP_TABLE_AUTHORIZATION_EXPRESSION,
accessMetadataType = MetadataObject.Type.TABLE)
public Response dropTable(
@PathParam("metalake") @AuthorizationMetadata(type =
Entity.EntityType.METALAKE)
