This is an automated email from the ASF dual-hosted git repository.
roryqi 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 f8cbcdd3f4 [#13980] fix(iceberg): Authorize table metrics reports
(#13981)
f8cbcdd3f4 is described below
commit f8cbcdd3f444c783774df1d4c2cfc5f3d9dfc9bf
Author: roryqi <[email protected]>
AuthorDate: Wed Oct 21 09:36:28 2026 +0800
[#13980] fix(iceberg): Authorize table metrics reports (#13981)
### What changes were proposed in this pull request?
- Require table read authorization before accepting Iceberg REST metrics
reports.
- Verify that the table exists and that the report names the addressed
table, including supported qualified names.
- Deny intercepted methods that lack an authorization expression, and
keep local authorization for metrics stored by a REST-backed catalog.
### Why are the changes needed?
An authenticated caller without table access could submit fabricated
metrics because the report endpoint had no authorization annotation.
Fix: #13980
### Does this PR introduce _any_ user-facing change?
Unauthorized reports return 403. Reports for nonexistent tables return
404. Reports naming another table return 400.
### How was this patch tested?
- Ran Spotless for `server-common` and `iceberg-rest-server`.
- Ran `TestBaseMetadataAuthorizationMethodInterceptor`,
`TestIcebergMetadataAuthorizationMethodInterceptor`, and
`TestIcebergTableOperations.testReportTableMetrics` with Gradle; all
passed.
---
.../service/rest/IcebergTableOperations.java | 40 +++++++++++++++---
...bergMetadataAuthorizationMethodInterceptor.java | 12 ++++++
.../service/rest/TestIcebergTableOperations.java | 49 ++++++++++++++++++++++
...bergMetadataAuthorizationMethodInterceptor.java | 27 ++++++++++++
...BaseMetadataAuthorizationMethodInterceptor.java | 23 +++++++++-
...BaseMetadataAuthorizationMethodInterceptor.java | 31 +++++++++++++-
6 files changed, 175 insertions(+), 7 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 552eb2726e..26bb0ca47f 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
@@ -77,6 +77,10 @@ import org.apache.iceberg.SnapshotRef;
import org.apache.iceberg.TableMetadata;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.metrics.CommitReport;
+import org.apache.iceberg.metrics.MetricsReport;
+import org.apache.iceberg.metrics.ScanReport;
import org.apache.iceberg.rest.RESTUtil;
import org.apache.iceberg.rest.requests.CreateTableRequest;
import org.apache.iceberg.rest.requests.FetchScanTasksRequest;
@@ -405,10 +409,14 @@ public class IcebergTableOperations {
@Produces(MediaType.APPLICATION_JSON)
@Timed(name = "report-table-metrics." + MetricNames.HTTP_PROCESS_DURATION,
absolute = true)
@ResponseMetered(name = "report-table-metrics", absolute = true)
+ @AuthorizationExpression(
+ expression = LOAD_TABLE_AUTHORIZATION_EXPRESSION,
+ accessMetadataType = MetadataObject.Type.TABLE)
public Response reportTableMetrics(
- @PathParam("prefix") String prefix,
- @Encoded() @PathParam("namespace") String namespace,
- @Encoded() @PathParam("table") String table,
+ @AuthorizationMetadata(type = EntityType.CATALOG) @PathParam("prefix")
String prefix,
+ @AuthorizationMetadata(type = EntityType.SCHEMA) @Encoded()
@PathParam("namespace")
+ String namespace,
+ @AuthorizationMetadata(type = EntityType.TABLE) @Encoded()
@PathParam("table") String table,
ReportMetricsRequest request) {
String catalogName = IcebergRESTUtils.getCatalogName(prefix);
Namespace icebergNS =
@@ -423,8 +431,30 @@ public class IcebergTableOperations {
return Utils.doAs(
httpRequest,
() -> {
- boolean accepted =
- icebergMetricsManager.recordMetric(catalogName, icebergNS,
request.report());
+ IcebergRequestContext context =
+ new IcebergRequestContext(httpServletRequest(), catalogName);
+ TableIdentifier tableIdentifier = TableIdentifier.of(icebergNS,
tableName);
+ if (!tableOperationDispatcher.tableExists(context,
tableIdentifier)) {
+ throw new NoSuchTableException("Table %s not found",
tableIdentifier);
+ }
+ MetricsReport report = request.report();
+ String reportedTableName;
+ if (report instanceof CommitReport) {
+ reportedTableName = ((CommitReport) report).tableName();
+ } else if (report instanceof ScanReport) {
+ reportedTableName = ((ScanReport) report).tableName();
+ } else {
+ throw new IllegalArgumentException("Unsupported metrics report
type");
+ }
+ String qualifiedTableName = tableIdentifier.toString();
+ if (reportedTableName == null
+ || (!tableName.equals(reportedTableName)
+ && !qualifiedTableName.equals(reportedTableName)
+ && !reportedTableName.endsWith("." + qualifiedTableName)))
{
+ throw new IllegalArgumentException(
+ "Metrics report table name does not match the requested
table");
+ }
+ boolean accepted = icebergMetricsManager.recordMetric(catalogName,
icebergNS, report);
if (accepted) {
return IcebergRESTUtils.noContent();
} else {
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/IcebergMetadataAuthorizationMethodInterceptor.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/IcebergMetadataAuthorizationMethodInterceptor.java
index 31275cc75e..c6c67f24b5 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/IcebergMetadataAuthorizationMethodInterceptor.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/IcebergMetadataAuthorizationMethodInterceptor.java
@@ -35,6 +35,7 @@ import
org.apache.gravitino.iceberg.service.IcebergCatalogWrapperManager;
import org.apache.gravitino.iceberg.service.IcebergExceptionMapper;
import org.apache.gravitino.iceberg.service.IcebergRESTUtils;
import
org.apache.gravitino.iceberg.service.authorization.IcebergRESTServerContext;
+import org.apache.gravitino.iceberg.service.rest.IcebergTableOperations;
import
org.apache.gravitino.server.authorization.annotations.AuthorizationExpression;
import
org.apache.gravitino.server.authorization.annotations.AuthorizationMetadata;
import
org.apache.gravitino.server.authorization.annotations.IcebergAuthorizationMetadata;
@@ -206,4 +207,15 @@ public class IcebergMetadataAuthorizationMethodInterceptor
// authorization. IRC2 handles authorization.
return catalogWrapper.isRESTCatalog();
}
+
+ /** Metrics are stored locally, so a downstream REST catalog cannot
authorize the report. */
+ @Override
+ protected boolean shouldSkipAuthorization(
+ Method method, Map<EntityType, NameIdentifier> nameIdentifierMap) {
+ if (method.getDeclaringClass() == IcebergTableOperations.class
+ && method.getName().equals("reportTableMetrics")) {
+ return false;
+ }
+ return super.shouldSkipAuthorization(method, nameIdentifierMap);
+ }
}
diff --git
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergTableOperations.java
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergTableOperations.java
index d11ed1e1af..3d67e2c8f0 100644
---
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergTableOperations.java
+++
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergTableOperations.java
@@ -511,6 +511,55 @@ public class TestIcebergTableOperations extends
IcebergNamespaceTestBase {
.post(Entity.entity(request, MediaType.APPLICATION_JSON_TYPE));
Assertions.assertEquals(Status.NO_CONTENT.getStatusCode(),
response.getStatus());
+
+ ReportMetricsRequest qualifiedRequest =
+ ReportMetricsRequest.of(
+ ImmutableCommitReport.builder()
+ .tableName("client_catalog." + namespace + ".metrics_foo1")
+ .snapshotId(-1)
+ .sequenceNumber(-1)
+ .operation("append")
+ .commitMetrics(commitMetrics)
+ .build());
+ Response qualifiedResponse =
+ getReportMetricsClientBuilder("metrics_foo1", namespace)
+ .post(Entity.entity(qualifiedRequest,
MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(Status.NO_CONTENT.getStatusCode(),
qualifiedResponse.getStatus());
+
+ Response missingTableResponse =
+ getReportMetricsClientBuilder("missing_metrics_table", namespace)
+ .post(Entity.entity(request, MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(Status.NOT_FOUND.getStatusCode(),
missingTableResponse.getStatus());
+
+ ReportMetricsRequest mismatchedRequest =
+ ReportMetricsRequest.of(
+ ImmutableCommitReport.builder()
+ .tableName("another_table")
+ .snapshotId(-1)
+ .sequenceNumber(-1)
+ .operation("append")
+ .commitMetrics(commitMetrics)
+ .build());
+ Response mismatchedTableResponse =
+ getReportMetricsClientBuilder("metrics_foo1", namespace)
+ .post(Entity.entity(mismatchedRequest,
MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(
+ Status.BAD_REQUEST.getStatusCode(),
mismatchedTableResponse.getStatus());
+
+ ReportMetricsRequest mismatchedNamespaceRequest =
+ ReportMetricsRequest.of(
+ ImmutableCommitReport.builder()
+ .tableName("client_catalog.other_namespace.metrics_foo1")
+ .snapshotId(-1)
+ .sequenceNumber(-1)
+ .operation("append")
+ .commitMetrics(commitMetrics)
+ .build());
+ Response mismatchedNamespaceResponse =
+ getReportMetricsClientBuilder("metrics_foo1", namespace)
+ .post(Entity.entity(mismatchedNamespaceRequest,
MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(
+ Status.BAD_REQUEST.getStatusCode(),
mismatchedNamespaceResponse.getStatus());
}
@ParameterizedTest
diff --git
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/server/web/filter/TestIcebergMetadataAuthorizationMethodInterceptor.java
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/server/web/filter/TestIcebergMetadataAuthorizationMethodInterceptor.java
index c51052b3c3..990783110f 100644
---
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/server/web/filter/TestIcebergMetadataAuthorizationMethodInterceptor.java
+++
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/server/web/filter/TestIcebergMetadataAuthorizationMethodInterceptor.java
@@ -47,6 +47,7 @@ import
org.apache.gravitino.iceberg.service.IcebergCatalogWrapperManager;
import org.apache.gravitino.iceberg.service.IcebergRESTUtils;
import
org.apache.gravitino.iceberg.service.authorization.IcebergRESTServerContext;
import org.apache.gravitino.iceberg.service.provider.IcebergConfigProvider;
+import org.apache.gravitino.iceberg.service.rest.IcebergTableOperations;
import org.apache.gravitino.server.authorization.GravitinoAuthorizerProvider;
import
org.apache.gravitino.server.authorization.annotations.AuthorizationExpression;
import
org.apache.gravitino.server.authorization.annotations.AuthorizationMetadata;
@@ -57,6 +58,7 @@ import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.exceptions.ForbiddenException;
import org.apache.iceberg.rest.RESTCatalog;
import org.apache.iceberg.rest.RESTUtil;
+import org.apache.iceberg.rest.requests.ReportMetricsRequest;
import org.apache.iceberg.rest.responses.ErrorResponse;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
@@ -302,6 +304,31 @@ public class
TestIcebergMetadataAuthorizationMethodInterceptor {
Mockito.verify(invocation, Mockito.times(1)).proceed();
}
+ @Test
+ public void testMetricsReportDoesNotSkipAuthorizationForRestCatalog() throws
Exception {
+ IcebergCatalogWrapperManager wrapperManager =
Mockito.mock(IcebergCatalogWrapperManager.class);
+ CatalogWrapperForREST wrapper = Mockito.mock(CatalogWrapperForREST.class);
+
Mockito.when(wrapperManager.getCatalogWrapper(TEST_CATALOG)).thenReturn(wrapper);
+ Mockito.when(wrapper.isRESTCatalog()).thenReturn(true);
+ resetContext(wrapperManager, true);
+
+ Method method =
+ IcebergTableOperations.class.getMethod(
+ "reportTableMetrics",
+ String.class,
+ String.class,
+ String.class,
+ ReportMetricsRequest.class);
+ IcebergMetadataAuthorizationMethodInterceptor interceptor =
+ new IcebergMetadataAuthorizationMethodInterceptor();
+
+ assertFalse(
+ interceptor.shouldSkipAuthorization(
+ method,
+ Map.of(Entity.EntityType.CATALOG, NameIdentifier.of(TEST_METALAKE,
TEST_CATALOG))));
+ Mockito.verifyNoInteractions(wrapperManager);
+ }
+
@Test
public void testInvokeDoesNotSkipAuthorizationForNonRestCatalog() throws
Throwable {
IcebergCatalogWrapperManager wrapperManager =
Mockito.mock(IcebergCatalogWrapperManager.class);
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/web/filter/BaseMetadataAuthorizationMethodInterceptor.java
b/server-common/src/main/java/org/apache/gravitino/server/web/filter/BaseMetadataAuthorizationMethodInterceptor.java
index 3d76832d86..d7de2531dd 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/web/filter/BaseMetadataAuthorizationMethodInterceptor.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/web/filter/BaseMetadataAuthorizationMethodInterceptor.java
@@ -19,12 +19,14 @@
package org.apache.gravitino.server.web.filter;
+import java.lang.annotation.Annotation;
import java.lang.reflect.Method;
import java.lang.reflect.Parameter;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
+import javax.ws.rs.HttpMethod;
import org.apache.gravitino.Entity;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.auth.ActiveRoles;
@@ -196,6 +198,18 @@ public abstract class
BaseMetadataAuthorizationMethodInterceptor {
return false;
}
+ /**
+ * Returns whether authorization may be delegated for this operation.
+ *
+ * @param method invoked protocol method
+ * @param nameIdentifierMap identifiers resolved from the request
+ * @return {@code true} if the local authorization pipeline may be skipped
+ */
+ protected boolean shouldSkipAuthorization(
+ Method method, Map<Entity.EntityType, NameIdentifier> nameIdentifierMap)
{
+ return shouldSkipAuthorization(nameIdentifierMap);
+ }
+
/**
* Hook for requests that must validate the current user and active roles
but do not have a
* metadata object on which to evaluate an expression. A protocol root-list
request is a typical
@@ -240,7 +254,7 @@ public abstract class
BaseMetadataAuthorizationMethodInterceptor {
AuthorizationTarget target =
resolveAuthorizationTarget(method, expressionAnnotation,
parameters, args);
Map<Entity.EntityType, NameIdentifier> nameIdentifierMap =
target.nameIdentifiers();
- boolean skipStandardCheck = shouldSkipAuthorization(nameIdentifierMap);
+ boolean skipStandardCheck = shouldSkipAuthorization(method,
nameIdentifierMap);
NameIdentifier metalakeIdent =
nameIdentifierMap.get(Entity.EntityType.METALAKE);
AuthorizationRequestContext authorizationRequestContext = new
AuthorizationRequestContext();
@@ -328,6 +342,13 @@ public abstract class
BaseMetadataAuthorizationMethodInterceptor {
// A skipped standard check authorized nothing that list filtering
could reuse.
scope.bindIfRead(
method, skipStandardCheck ? null : metalakeIdent,
authorizationRequestContext);
+ } else {
+ for (Annotation annotation : method.getAnnotations()) {
+ if
(annotation.annotationType().isAnnotationPresent(HttpMethod.class)) {
+ throw new ForbiddenException(
+ "Authorization expression is missing for operation '%s'",
method.getName());
+ }
+ }
}
} catch (Exception ex) {
if (ex instanceof ForbiddenException || isExceptionPropagate(ex)) {
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/web/filter/TestBaseMetadataAuthorizationMethodInterceptor.java
b/server-common/src/test/java/org/apache/gravitino/server/web/filter/TestBaseMetadataAuthorizationMethodInterceptor.java
index d100e1d405..7a4c5e4a33 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/web/filter/TestBaseMetadataAuthorizationMethodInterceptor.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/web/filter/TestBaseMetadataAuthorizationMethodInterceptor.java
@@ -307,12 +307,31 @@ public class
TestBaseMetadataAuthorizationMethodInterceptor {
public void testOperationFailureUsesProtocolMapper() throws Throwable {
IllegalStateException failure = new IllegalStateException("operation
failed");
TestInterceptor interceptor = new
TestInterceptor(Entity.EntityType.SCHEMA);
- TestInvocation invocation = invocation("unannotated", null);
+ TestInvocation invocation = invocation("operationFails", null);
when(invocation.proceed()).thenThrow(failure);
assertSame(failure, interceptor.invoke(invocation));
}
+ @Test
+ public void testMissingAuthorizationExpressionIsDenied() throws Throwable {
+ TestInterceptor interceptor = new
TestInterceptor(Entity.EntityType.SCHEMA);
+ TestInvocation invocation = invocation("unannotated", "authorized");
+
+ ForbiddenException response =
+ assertInstanceOf(ForbiddenException.class,
interceptor.invoke(invocation));
+ assertTrue(response.getMessage().contains("unannotated"));
+ verify(invocation, never()).proceed();
+ }
+
+ @Test
+ public void testUnannotatedHelperProceeds() throws Throwable {
+ TestInvocation invocation = invocation("helper", "helper result");
+
+ assertEquals("helper result", new
TestInterceptor(Entity.EntityType.SCHEMA).invoke(invocation));
+ verify(invocation).proceed();
+ }
+
/** Entry authorization, parent checks and worker filtering share one user
lookup. */
@ParameterizedTest
@ValueSource(booleans = {false, true})
@@ -540,8 +559,18 @@ public class
TestBaseMetadataAuthorizationMethodInterceptor {
return "unused";
}
+ @GET
private String unannotated() {
return "unused";
}
+
+ private String helper() {
+ return "unused";
+ }
+
+ @AuthorizationExpression(expression = "true")
+ private String operationFails() {
+ return "unused";
+ }
}
}
