This is an automated email from the ASF dual-hosted git repository.
jerryshao 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 a8e1e5ccc6 [#13980] fix(iceberg): Authorize table metrics reports on
branch-1.3 (#14043)
a8e1e5ccc6 is described below
commit a8e1e5ccc619b43a0a9b4f5d4ea0ee51763db848
Author: github-actions[bot]
<41898282+github-actions[bot]@users.noreply.github.com>
AuthorDate: Wed Oct 21 19:12:34 2026 +0800
[#13980] fix(iceberg): Authorize table metrics reports on branch-1.3
(#14043)
### What changes were proposed in this pull request?
Backport #13981 to `branch-1.3`: authorize Iceberg REST table metrics
reports, check that the table exists, and reject reports that name a
different table. Keep local authorization for metrics submitted through
a REST-backed catalog. The cherry-pick conflicts are resolved;
request-cache code unavailable on `branch-1.3` was omitted.
### Why are the changes needed?
An authenticated caller without table access could submit fabricated
metrics because the report endpoint lacked an authorization expression.
Fix: #13980
### Does this PR introduce _any_ user-facing change?
Unauthorized reports return 403, reports for missing tables return 404,
and reports naming another table return 400.
### How was this patch tested?
- Ran Spotless for `server-common` and `iceberg-rest-server`.
- Passed `TestBaseMetadataAuthorizationMethodInterceptor`,
`TestIcebergMetadataAuthorizationMethodInterceptor`, and
`TestIcebergTableOperations.testReportTableMetrics`.
---------
Co-authored-by: roryqi <[email protected]>
Co-authored-by: roryqi <[email protected]>
---
.../service/rest/IcebergTableOperations.java | 40 +++++++++++++++---
...bergMetadataAuthorizationMethodInterceptor.java | 12 ++++++
.../service/rest/TestIcebergTableOperations.java | 49 ++++++++++++++++++++++
...bergMetadataAuthorizationMethodInterceptor.java | 27 ++++++++++++
...BaseMetadataAuthorizationMethodInterceptor.java | 23 +++++++++-
...BaseMetadataAuthorizationMethodInterceptor.java | 32 +++++++++++++-
6 files changed, 176 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 9f04543fa8..e6a9333692 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.PlanTableScanRequest;
@@ -403,10 +407,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 =
@@ -421,8 +429,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 5f1618099b..8e8f2efb16 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
@@ -437,6 +437,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 cd0be14d34..abad7e0dd3 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
@@ -43,6 +43,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.annotations.AuthorizationExpression;
import
org.apache.gravitino.server.authorization.annotations.AuthorizationMetadata;
import org.apache.gravitino.utils.NameIdentifierUtil;
@@ -51,6 +52,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;
@@ -295,6 +297,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 88b49b957a..21a1fca4a0 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,11 +19,13 @@
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 javax.ws.rs.HttpMethod;
import org.apache.gravitino.Entity;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.authorization.AuthorizationRequestContext;
@@ -192,6 +194,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 but do not have a
metadata object on
* which to evaluate an expression. A protocol root-list request is a
typical example: its
@@ -228,7 +242,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();
@@ -291,6 +305,13 @@ public abstract class
BaseMetadataAuthorizationMethodInterceptor {
throw new ForbiddenException(notAuthzMessage);
}
}
+ } 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 6644970fe7..6954f69731 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
@@ -36,6 +36,7 @@ import java.lang.reflect.Parameter;
import java.util.EnumMap;
import java.util.Map;
import java.util.Optional;
+import javax.ws.rs.GET;
import org.apache.gravitino.Entity;
import org.apache.gravitino.MetadataObject;
import org.apache.gravitino.NameIdentifier;
@@ -253,12 +254,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();
+ }
+
private static TestInvocation invocation(String methodName, Object result)
throws Throwable {
Method method = TestOperations.class.getDeclaredMethod(methodName);
TestInvocation invocation = mock(TestInvocation.class);
@@ -361,8 +381,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";
+ }
}
}
