This is an automated email from the ASF dual-hosted git repository.
yuqi1129 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 5bf0d8b113 [#12562] feat(lance): Add table mutation and deletion
authorization (#12696)
5bf0d8b113 is described below
commit 5bf0d8b1136110e4b5dcc5bb3a2a8b7298dc2985
Author: Qi Yu <[email protected]>
AuthorDate: Thu Sep 3 21:54:24 2026 +0800
[#12562] feat(lance): Add table mutation and deletion authorization (#12696)
### What changes were proposed in this pull request?
Authorizes the four Lance REST table endpoints that were still served
unchecked after #12559, #12560 and #12561: `deregister`, `drop`,
`drop_columns` and `alter_columns`.
- `drop_columns` and `alter_columns` require MODIFY_TABLE or ownership.
- `deregister` and `drop` require ownership of the table or of one of
its ancestors. MODIFY_TABLE alters a table but never removes it.
- Both expressions are guarded on the addressed entity type, so an
identifier that resolves to a catalog or a schema is denied instead of
being authorized as if it addressed a table.
`deregister` (which removes only the Gravitino metadata) and `drop`
(which also deletes the data) deliberately require the same privilege.
Neither the Gravitino core server nor Iceberg REST splits removal into
two levels, and inventing a third rule here would be a difference
callers have to learn for no benefit.
This is the last endpoint group of #9087, so with this PR the Lance REST
surface is fully authorized. The remaining sub-task is the documentation
one, #12574.
Fix: #12562
### Why are the changes needed?
With authorization enabled, any authenticated caller could drop any
table or rewrite any table's columns through Lance REST, regardless of
the privileges they hold on it.
### Does this PR introduce _any_ user-facing change?
Yes. With authorization enabled, column changes, deregister and drop are
authorized and denied requests return 403 before any metadata or storage
mutation. A table the caller may not remove is reported as forbidden
whether or not it exists, so these endpoints cannot be used to probe for
existence.
### How was this patch tested?
`TestLanceMetadataAuthorizationMethodInterceptor` and
`LanceTableAuthorizationIT` cover the two properties this change is
about: a caller without the privilege is rejected *before* the operation
runs, and a table the caller cannot see is indistinguishable from one
that does not exist.
- MODIFY_TABLE authorizes column changes and SELECT_TABLE does not; an
explicit deny overrides MODIFY_TABLE; MODIFY_TABLE and CREATE_TABLE
together are still not enough to remove a table, while ownership is.
- The ancestor-ownership branches are covered per level rather than by
making the caller own everything: owning the schema removes tables the
owner never created, owning a catalog or the metalake does too, and
schema ownership without USE_CATALOG removes nothing.
- Denied paths assert `verify(invocation, never()).proceed()`, which is
the direct form of "no mutation on denial". In the integration test the
allowed column changes are checked against the described columns (`[id,
value]` -> `[id, renamed]` -> `[id]`), so they are shown to reach the
metadata store rather than merely passing the interceptor.
- A denied response is checked not to contain the table location, and an
inaccessible table and a missing table both return 403 to the
unprivileged caller while the admin sees 200 and 404.
New `TestLanceRESTEndpointAuthorizationCoverage` guards the completeness
of the whole surface. The shared pipeline authorizes a method only when
it carries `@AuthorizationExpression`, so adding an endpoint without one
serves it unchecked and no behavioral test would notice, because the new
endpoint simply has no test of its own. The test scans the REST resource
package instead of listing endpoints, and asserts that every JAX-RS
endpoint declares an expression and that every resource declaring one is
actually intercepted. Health operations are the one exception and are
listed explicitly. I verified the test fails as intended by removing the
two new annotations:
```
Lance REST endpoints without @AuthorizationExpression are served unchecked:
[LanceTableOperations#deregisterTable, LanceTableOperations#dropTable]
```
`./gradlew :lance:lance-rest-server:build :lance:lance-common:build` —
202 tests, all passing.
---
.../LanceAuthorizationExpressions.java | 11 ++
.../lance/service/rest/LanceTableOperations.java | 6 +
.../test/LanceTableAuthorizationIT.java | 219 ++++++++++++++++++++-
...anceMetadataAuthorizationMethodInterceptor.java | 160 +++++++++++++++
...TestLanceRESTEndpointAuthorizationCoverage.java | 152 ++++++++++++++
5 files changed, 539 insertions(+), 9 deletions(-)
diff --git
a/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/authorization/LanceAuthorizationExpressions.java
b/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/authorization/LanceAuthorizationExpressions.java
index a3d72bbb4a..0d40c6fab8 100644
---
a/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/authorization/LanceAuthorizationExpressions.java
+++
b/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/authorization/LanceAuthorizationExpressions.java
@@ -129,5 +129,16 @@ public final class LanceAuthorizationExpressions {
+
AuthorizationExpressionConstants.MODIFY_TABLE_AUTHORIZATION_EXPRESSION
+ ")";
+ /**
+ * Authorizes removing a table, whether the storage is deleted with it or
only the Gravitino
+ * metadata is. Both require ownership of the table or of one of its
ancestors, matching the
+ * Gravitino and Iceberg REST surfaces: MODIFY_TABLE alters a table but
never removes it.
+ */
+ public static final String DROP_TABLE_AUTHORIZATION_EXPRESSION =
+ """
+ entityType == 'TABLE' && (ANY(OWNER, METALAKE, CATALOG) ||
SCHEMA_OWNER_WITH_USE_CATALOG ||
+ ANY_USE_CATALOG && ANY_USE_SCHEMA && TABLE::OWNER)
+ """;
+
private LanceAuthorizationExpressions() {}
}
diff --git
a/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java
b/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java
index 1d51a65dbf..5eb2f731ed 100644
---
a/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java
+++
b/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java
@@ -23,6 +23,8 @@ import static
org.apache.gravitino.lance.common.utils.LanceConstants.LANCE_LOCAT
import static
org.apache.gravitino.lance.common.utils.LanceConstants.LANCE_TABLE_LOCATION_HEADER;
import static
org.apache.gravitino.lance.common.utils.LanceConstants.LANCE_TABLE_PROPERTIES_PREFIX_HEADER;
import static
org.apache.gravitino.lance.service.authorization.LanceAuthorizationExpressions.CREATE_TABLE_AUTHORIZATION_EXPRESSION;
+import static
org.apache.gravitino.lance.service.authorization.LanceAuthorizationExpressions.DROP_TABLE_AUTHORIZATION_EXPRESSION;
+import static
org.apache.gravitino.lance.service.authorization.LanceAuthorizationExpressions.MODIFY_TABLE_AUTHORIZATION_EXPRESSION;
import static
org.apache.gravitino.lance.service.authorization.LanceAuthorizationExpressions.PROBE_TABLE_AUTHORIZATION_EXPRESSION;
import static
org.apache.gravitino.lance.service.authorization.LanceAuthorizationExpressions.READ_TABLE_AUTHORIZATION_EXPRESSION;
@@ -221,6 +223,7 @@ public class LanceTableOperations {
@Path("/deregister")
@Timed(name = "deregister-table." + MetricNames.HTTP_PROCESS_DURATION,
absolute = true)
@ResponseMetered(name = "deregister-table", absolute = true)
+ @AuthorizationExpression(expression = DROP_TABLE_AUTHORIZATION_EXPRESSION)
public Response deregisterTable(
@PathParam("id") String tableId,
@QueryParam("delimiter") @DefaultValue("$") String delimiter,
@@ -263,6 +266,7 @@ public class LanceTableOperations {
@Path("/drop")
@Timed(name = "drop-table." + MetricNames.HTTP_PROCESS_DURATION, absolute =
true)
@ResponseMetered(name = "drop-table", absolute = true)
+ @AuthorizationExpression(expression = DROP_TABLE_AUTHORIZATION_EXPRESSION)
public Response dropTable(
@PathParam("id") String tableId,
@QueryParam("delimiter") @DefaultValue("$") String delimiter,
@@ -281,6 +285,7 @@ public class LanceTableOperations {
@Path("/drop_columns")
@Timed(name = "drop-columns." + MetricNames.HTTP_PROCESS_DURATION, absolute
= true)
@ResponseMetered(name = "drop-columns", absolute = true)
+ @AuthorizationExpression(expression = MODIFY_TABLE_AUTHORIZATION_EXPRESSION)
public Response dropColumns(
@PathParam("id") String tableId,
@QueryParam("delimiter") @DefaultValue("$") String delimiter,
@@ -304,6 +309,7 @@ public class LanceTableOperations {
@Path("/alter_columns")
@Timed(name = "alter-columns." + MetricNames.HTTP_PROCESS_DURATION, absolute
= true)
@ResponseMetered(name = "alter-columns", absolute = true)
+ @AuthorizationExpression(expression = MODIFY_TABLE_AUTHORIZATION_EXPRESSION)
public Response alterColumns(
@PathParam("id") String tableId,
@QueryParam("delimiter") @DefaultValue("$") String delimiter,
diff --git
a/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceTableAuthorizationIT.java
b/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceTableAuthorizationIT.java
index 1036465ae8..c7722b0523 100644
---
a/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceTableAuthorizationIT.java
+++
b/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceTableAuthorizationIT.java
@@ -28,36 +28,51 @@ import java.util.ArrayList;
import java.util.Base64;
import java.util.HashMap;
import java.util.List;
+import org.apache.arrow.vector.types.pojo.ArrowType;
+import org.apache.arrow.vector.types.pojo.Field;
+import org.apache.arrow.vector.types.pojo.Schema;
import org.apache.gravitino.Configs;
+import org.apache.gravitino.MetadataObject;
+import org.apache.gravitino.MetadataObjects;
import org.apache.gravitino.auth.AuthConstants;
+import org.apache.gravitino.authorization.Owner;
import org.apache.gravitino.authorization.Privileges;
import org.apache.gravitino.authorization.SecurableObject;
import org.apache.gravitino.authorization.SecurableObjects;
import org.apache.gravitino.client.GravitinoMetalake;
import org.apache.gravitino.integration.test.util.BaseIT;
+import org.apache.gravitino.lance.common.utils.ArrowUtils;
+import org.apache.gravitino.lance.common.utils.LanceConstants;
import org.apache.gravitino.server.web.ObjectMapperProvider;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;
+import org.lance.namespace.model.AlterTableDropColumnsRequest;
import org.lance.namespace.model.CreateNamespaceRequest;
import org.lance.namespace.model.DeclareTableRequest;
import org.lance.namespace.model.DescribeTableResponse;
import org.lance.namespace.model.ListTablesResponse;
import org.lance.namespace.model.RegisterTableRequest;
-/** Verifies that read-only Lance REST table operations are authorized and
filtered. */
+/** Verifies that Lance REST table operations are authorized and filtered. */
public class LanceTableAuthorizationIT extends BaseIT {
private static final String ADMIN = "lance_table_authz_admin";
private static final String READER = "lance_table_authz_reader";
private static final String PROBER = "lance_table_authz_prober";
+ private static final String MUTATOR = "lance_table_authz_mutator";
+ private static final String SCHEMA_OWNER = "lance_table_authz_schema_owner";
+ private static final String LONE_OWNER = "lance_table_authz_lone_owner";
private static final String CATALOG = "lance_table_authz_catalog";
private static final String SCHEMA = "lance_table_authz_schema";
// Tables created by the write tests live in their own schema, so the
listing tests keep asserting
// the exact contents of the read schema whatever order the tests run in.
private static final String WRITE_SCHEMA = "lance_table_authz_write_schema";
+ // Two schemas owned by users who created nothing in them, to exercise
ancestor ownership.
+ private static final String OWNED_SCHEMA = "lance_table_authz_owned_schema";
+ private static final String LONE_SCHEMA = "lance_table_authz_lone_schema";
// The hidden table sorts first so that a listing filtered after pagination
rather than before it
// would return an empty first page instead of the visible table.
@@ -65,6 +80,11 @@ public class LanceTableAuthorizationIT extends BaseIT {
private static final String VISIBLE_TABLE = "b_visible_table";
private static final String MISSING_TABLE = "c_missing_table";
private static final String PROBER_TABLE = "d_prober_table";
+ private static final String MUTABLE_TABLE = "e_mutable_table";
+ private static final String DEREGISTER_TABLE = "f_deregister_table";
+ private static final String DROP_TABLE = "g_drop_table";
+ private static final String OWNED_SCHEMA_TABLE = "h_owned_schema_table";
+ private static final String LONE_SCHEMA_TABLE = "i_lone_schema_table";
private static final String DELIMITER = ".";
@TempDir private static Path tempDir;
@@ -85,16 +105,31 @@ public class LanceTableAuthorizationIT extends BaseIT {
GravitinoMetalake metalake = client.loadMetalake(metalakeName);
metalake.addUser(READER);
metalake.addUser(PROBER);
+ metalake.addUser(MUTATOR);
+ metalake.addUser(SCHEMA_OWNER);
+ metalake.addUser(LONE_OWNER);
createNamespace(CATALOG);
createNamespace(id(CATALOG, SCHEMA));
createNamespace(id(CATALOG, WRITE_SCHEMA));
+ createNamespace(id(CATALOG, OWNED_SCHEMA));
+ createNamespace(id(CATALOG, LONE_SCHEMA));
registerTable(VISIBLE_TABLE);
registerTable(HIDDEN_TABLE);
+ createTable(WRITE_SCHEMA, MUTABLE_TABLE);
+ // Registered by the admin, so neither owner below owns the table itself.
+ assertStatus(
+ 200, register(ADMIN, OWNED_SCHEMA, OWNED_SCHEMA_TABLE, null,
location(OWNED_SCHEMA_TABLE)));
+ assertStatus(
+ 200, register(ADMIN, LONE_SCHEMA, LONE_SCHEMA_TABLE, null,
location(LONE_SCHEMA_TABLE)));
+ setSchemaOwner(metalake, OWNED_SCHEMA, SCHEMA_OWNER);
+ setSchemaOwner(metalake, LONE_SCHEMA, LONE_OWNER);
SecurableObject catalogScope = SecurableObjects.ofCatalog(CATALOG, new
ArrayList<>());
SecurableObject schemaScope =
SecurableObjects.ofSchema(catalogScope, SCHEMA, new ArrayList<>());
+ SecurableObject writeSchemaScope =
+ SecurableObjects.ofSchema(catalogScope, WRITE_SCHEMA, new
ArrayList<>());
// The reader may select exactly one table, so the other one must stay
invisible even though
// the reader can reach the schema holding it.
@@ -110,6 +145,15 @@ public class LanceTableAuthorizationIT extends BaseIT {
SecurableObjects.ofTable(
schemaScope,
VISIBLE_TABLE,
+ new ArrayList<>(List.of(Privileges.SelectTable.allow()))),
+ // Read access to the mutable table as well, so that denying a
column change to the
+ // reader can only be explained by the missing MODIFY_TABLE and
not by the reader
+ // being unable to reach the table at all.
+ SecurableObjects.ofSchema(
+ catalogScope, WRITE_SCHEMA, new
ArrayList<>(List.of(Privileges.UseSchema.allow()))),
+ SecurableObjects.ofTable(
+ writeSchemaScope,
+ MUTABLE_TABLE,
new ArrayList<>(List.of(Privileges.SelectTable.allow())))));
grant(
@@ -129,6 +173,36 @@ public class LanceTableAuthorizationIT extends BaseIT {
WRITE_SCHEMA,
new ArrayList<>(
List.of(Privileges.UseSchema.allow(),
Privileges.CreateTable.allow())))));
+
+ // The schema owner holds USE_CATALOG and nothing else: everything below
the catalog has to
+ // come from owning the schema. The lone owner holds no role at all, so it
owns a schema it
+ // cannot reach.
+ grant(
+ metalake,
+ "lance_table_authz_schema_owner_role",
+ SCHEMA_OWNER,
+ List.of(
+ SecurableObjects.ofCatalog(
+ CATALOG, new
ArrayList<>(List.of(Privileges.UseCatalog.allow())))));
+
+ // The mutator may change tables it does not own, which must not let it
remove them.
+ grant(
+ metalake,
+ "lance_table_authz_mutator_role",
+ MUTATOR,
+ List.of(
+ SecurableObjects.ofCatalog(
+ CATALOG, new
ArrayList<>(List.of(Privileges.UseCatalog.allow()))),
+ SecurableObjects.ofSchema(
+ catalogScope,
+ SCHEMA,
+ new ArrayList<>(
+ List.of(Privileges.UseSchema.allow(),
Privileges.ModifyTable.allow()))),
+ SecurableObjects.ofSchema(
+ catalogScope,
+ WRITE_SCHEMA,
+ new ArrayList<>(
+ List.of(Privileges.UseSchema.allow(),
Privileges.ModifyTable.allow())))));
}
@AfterAll
@@ -202,6 +276,98 @@ public class LanceTableAuthorizationIT extends BaseIT {
Assertions.assertEquals(location(HIDDEN_TABLE), describedLocation(ADMIN,
HIDDEN_TABLE));
}
+ @Test
+ public void testColumnMutationRequiresModifyTable() throws Exception {
+ // The reader holds USE_CATALOG, USE_SCHEMA and SELECT_TABLE on this exact
table, so it can
+ // read the table. Asserting that first is what makes the denials below
evidence that
+ // SELECT_TABLE does not authorize a column change, rather than evidence
that the reader
+ // cannot reach the table.
+ assertStatus(200, table(READER, WRITE_SCHEMA, MUTABLE_TABLE, "describe"));
+ assertStatus(403, dropColumns(READER, MUTABLE_TABLE, "value"));
+ assertStatus(403, alterColumns(READER, MUTABLE_TABLE, "value", "renamed"));
+ Assertions.assertEquals(List.of("id", "value"),
describedColumns(MUTABLE_TABLE));
+
+ // An inherited MODIFY_TABLE grant authorizes both mutations, and each
reaches the metadata
+ // store rather than merely passing the authorization interceptor.
+ assertStatus(200, alterColumns(MUTATOR, MUTABLE_TABLE, "value",
"renamed"));
+ Assertions.assertEquals(List.of("id", "renamed"),
describedColumns(MUTABLE_TABLE));
+ assertStatus(200, dropColumns(MUTATOR, MUTABLE_TABLE, "renamed"));
+ Assertions.assertEquals(List.of("id"), describedColumns(MUTABLE_TABLE));
+
+ assertStatus(404, dropColumns(MUTATOR, MISSING_TABLE, "value"));
+ }
+
+ @Test
+ public void testRemovingATableRequiresOwnership() throws Exception {
+ // MODIFY_TABLE alters a table but never removes it.
+ assertStatus(403, table(MUTATOR, VISIBLE_TABLE, "deregister"));
+ assertStatus(403, table(MUTATOR, VISIBLE_TABLE, "drop"));
+ // The denied requests left the table in place.
+ assertStatus(200, table(ADMIN, VISIBLE_TABLE, "describe"));
+
+ // Creating assigns ownership, so the creator may use either removal
operation.
+ assertStatus(
+ 200, register(PROBER, WRITE_SCHEMA, DEREGISTER_TABLE, null,
location(DEREGISTER_TABLE)));
+ assertStatus(200, register(PROBER, WRITE_SCHEMA, DROP_TABLE, null,
location(DROP_TABLE)));
+ assertStatus(200, table(PROBER, WRITE_SCHEMA, DEREGISTER_TABLE,
"deregister"));
+ assertStatus(200, table(PROBER, WRITE_SCHEMA, DROP_TABLE, "drop"));
+ assertStatus(404, table(ADMIN, WRITE_SCHEMA, DEREGISTER_TABLE,
"describe"));
+ assertStatus(404, table(ADMIN, WRITE_SCHEMA, DROP_TABLE, "describe"));
+ }
+
+ @Test
+ public void testSchemaOwnershipRemovesTablesTheOwnerDidNotCreate() throws
Exception {
+ // SCHEMA_OWNER_WITH_USE_CATALOG: the admin registered this table, so the
only thing the caller
+ // has is ownership of the schema holding it, plus USE_CATALOG.
+ assertStatus(200, table(SCHEMA_OWNER, OWNED_SCHEMA, OWNED_SCHEMA_TABLE,
"drop"));
+ assertStatus(404, table(ADMIN, OWNED_SCHEMA, OWNED_SCHEMA_TABLE,
"describe"));
+ }
+
+ @Test
+ public void testSchemaOwnershipWithoutUseCatalogRemovesNothing() throws
Exception {
+ // Ownership of the schema alone is not enough, the expression also
requires USE_CATALOG.
+ assertStatus(403, table(LONE_OWNER, LONE_SCHEMA, LONE_SCHEMA_TABLE,
"drop"));
+ assertStatus(403, table(LONE_OWNER, LONE_SCHEMA, LONE_SCHEMA_TABLE,
"deregister"));
+ assertStatus(403, dropColumns(LONE_OWNER, LONE_SCHEMA, LONE_SCHEMA_TABLE,
"value"));
+ assertStatus(200, table(ADMIN, LONE_SCHEMA, LONE_SCHEMA_TABLE,
"describe"));
+ }
+
+ @Test
+ public void testMutationConcealsTablesTheCallerMayNotSee() throws Exception {
+ // Both are forbidden for the reader, so the endpoint cannot be used to
probe for existence.
+ HttpResponse<String> denied = table(READER, HIDDEN_TABLE, "deregister");
+ assertStatus(403, denied);
+ Assertions.assertFalse(denied.body().contains(location(HIDDEN_TABLE)),
denied.body());
+ assertStatus(403, table(READER, MISSING_TABLE, "deregister"));
+ assertStatus(200, table(ADMIN, HIDDEN_TABLE, "describe"));
+ assertStatus(404, table(ADMIN, MISSING_TABLE, "deregister"));
+ }
+
+ private HttpResponse<String> dropColumns(String user, String tableName,
String column)
+ throws Exception {
+ return dropColumns(user, WRITE_SCHEMA, tableName, column);
+ }
+
+ private HttpResponse<String> dropColumns(
+ String user, String schema, String tableName, String column) throws
Exception {
+ AlterTableDropColumnsRequest body = new AlterTableDropColumnsRequest();
+ body.setColumns(List.of(column));
+ return send(user, "/v1/table/" + id(CATALOG, schema, tableName) +
"/drop_columns", body);
+ }
+
+ private HttpResponse<String> alterColumns(
+ String user, String tableName, String column, String renamedColumn)
throws Exception {
+ // Sent as raw JSON because the generated request model serializes the
rename field in a shape
+ // the server does not accept, which would fail with a 400 before
authorization runs.
+ // TODO: switch back to AlterTableAlterColumnsRequest once the
lance-namespace models
+ // serialize `rename` as a plain string, otherwise this test silently
stops exercising the
+ // request shape the models produce.
+ return sendRaw(
+ user,
+ "/v1/table/" + id(CATALOG, WRITE_SCHEMA, tableName) + "/alter_columns",
+
"{\"alterations\":[{\"path\":\"%s\",\"rename\":\"%s\"}]}".formatted(column,
renamedColumn));
+ }
+
private List<String> listTables(String user, int limit) throws Exception {
HttpRequest request =
request(user, "/v1/namespace/" + id(CATALOG, SCHEMA) + "/table/list",
"&limit=" + limit)
@@ -218,6 +384,12 @@ public class LanceTableAuthorizationIT extends BaseIT {
return tables;
}
+ private void setSchemaOwner(GravitinoMetalake metalake, String schemaName,
String user) {
+ MetadataObject schemaObject =
+ MetadataObjects.of(List.of(CATALOG, schemaName),
MetadataObject.Type.SCHEMA);
+ metalake.setOwner(schemaObject, user, Owner.Type.USER);
+ }
+
private void grant(
GravitinoMetalake metalake, String role, String user,
List<SecurableObject> objects) {
metalake.createRole(role, new HashMap<>(), objects);
@@ -244,6 +416,21 @@ public class LanceTableAuthorizationIT extends BaseIT {
assertStatus(200, register(ADMIN, SCHEMA, tableName, null,
location(tableName)));
}
+ private void createTable(String schemaName, String tableName) throws
Exception {
+ Schema schema =
+ new Schema(
+ List.of(
+ Field.nullable("id", new ArrowType.Int(32, true)),
+ Field.nullable("value", new ArrowType.Utf8())));
+ HttpRequest request =
+ request(ADMIN, "/v1/table/" + id(CATALOG, schemaName, tableName) +
"/create", "")
+ .setHeader("Content-Type", "application/vnd.apache.arrow.stream")
+ .setHeader(LanceConstants.LANCE_TABLE_LOCATION_HEADER,
location(tableName))
+
.POST(HttpRequest.BodyPublishers.ofByteArray(ArrowUtils.generateIpcStream(schema)))
+ .build();
+ assertStatus(200, httpClient.send(request,
HttpResponse.BodyHandlers.ofString()));
+ }
+
private HttpResponse<String> register(
String user, String schema, String tableName, String mode, String
location) throws Exception {
RegisterTableRequest body = new RegisterTableRequest();
@@ -262,11 +449,25 @@ public class LanceTableAuthorizationIT extends BaseIT {
}
private String describedLocation(String user, String tableName) throws
Exception {
- HttpResponse<String> response = table(user, tableName, "describe");
+ return describe(user, tableName).getLocation();
+ }
+
+ private List<String> describedColumns(String tableName) throws Exception {
+ return describe(ADMIN, WRITE_SCHEMA,
tableName).getSchema().getFields().stream()
+ .map(field -> field.getName())
+ .toList();
+ }
+
+ private DescribeTableResponse describe(String user, String tableName) throws
Exception {
+ return describe(user, SCHEMA, tableName);
+ }
+
+ private DescribeTableResponse describe(String user, String schemaName,
String tableName)
+ throws Exception {
+ HttpResponse<String> response = table(user, schemaName, tableName,
"describe");
assertStatus(200, response);
return ObjectMapperProvider.objectMapper()
- .readValue(response.body(), DescribeTableResponse.class)
- .getLocation();
+ .readValue(response.body(), DescribeTableResponse.class);
}
private HttpResponse<String> table(String user, String tableName, String
operation)
@@ -284,12 +485,12 @@ public class LanceTableAuthorizationIT extends BaseIT {
}
private HttpResponse<String> send(String user, String path, Object body)
throws Exception {
+ return sendRaw(user, path,
ObjectMapperProvider.objectMapper().writeValueAsString(body));
+ }
+
+ private HttpResponse<String> sendRaw(String user, String path, String body)
throws Exception {
HttpRequest request =
- request(user, path, "")
- .POST(
- HttpRequest.BodyPublishers.ofString(
-
ObjectMapperProvider.objectMapper().writeValueAsString(body)))
- .build();
+ request(user, path,
"").POST(HttpRequest.BodyPublishers.ofString(body)).build();
return httpClient.send(request, HttpResponse.BodyHandlers.ofString());
}
diff --git
a/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/authorization/TestLanceMetadataAuthorizationMethodInterceptor.java
b/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/authorization/TestLanceMetadataAuthorizationMethodInterceptor.java
index 7b5ef686fd..517d74d770 100644
---
a/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/authorization/TestLanceMetadataAuthorizationMethodInterceptor.java
+++
b/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/authorization/TestLanceMetadataAuthorizationMethodInterceptor.java
@@ -35,6 +35,7 @@ import javax.ws.rs.QueryParam;
import javax.ws.rs.core.HttpHeaders;
import javax.ws.rs.core.Response;
import org.aopalliance.intercept.MethodInvocation;
+import org.apache.gravitino.MetadataObject;
import org.apache.gravitino.UserPrincipal;
import org.apache.gravitino.authorization.AuthorizationUtils;
import org.apache.gravitino.authorization.GravitinoAuthorizer;
@@ -48,10 +49,14 @@ import org.apache.gravitino.utils.PrincipalUtils;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
+import org.lance.namespace.model.AlterTableAlterColumnsRequest;
+import org.lance.namespace.model.AlterTableDropColumnsRequest;
import org.lance.namespace.model.CreateNamespaceRequest;
import org.lance.namespace.model.DeclareTableRequest;
+import org.lance.namespace.model.DeregisterTableRequest;
import org.lance.namespace.model.DescribeTableRequest;
import org.lance.namespace.model.DropNamespaceRequest;
+import org.lance.namespace.model.DropTableRequest;
import org.lance.namespace.model.ErrorResponse;
import org.lance.namespace.model.RegisterTableRequest;
import org.lance.namespace.model.TableExistsRequest;
@@ -381,6 +386,149 @@ class TestLanceMetadataAuthorizationMethodInterceptor {
assertEquals(PROCEEDED,
interceptor.invoke(createTableInvocation(tableId(), "overwrite")));
}
+ @Test
+ void testColumnMutationRequiresModifyTable() throws Throwable {
+ allow(Privilege.Name.USE_CATALOG, Privilege.Name.USE_SCHEMA,
Privilege.Name.MODIFY_TABLE);
+ assertEquals(PROCEEDED,
interceptor.invoke(dropColumnsInvocation(tableId())));
+ assertEquals(PROCEEDED,
interceptor.invoke(alterColumnsInvocation(tableId())));
+
+ // Reading a table does not authorize changing its columns.
+ allow(Privilege.Name.USE_CATALOG, Privilege.Name.USE_SCHEMA,
Privilege.Name.SELECT_TABLE);
+ MethodInvocation dropColumns = dropColumnsInvocation(tableId());
+ assertErrorResponse(interceptor.invoke(dropColumns),
Response.Status.FORBIDDEN);
+ verify(dropColumns, never()).proceed();
+ MethodInvocation alterColumns = alterColumnsInvocation(tableId());
+ assertErrorResponse(interceptor.invoke(alterColumns),
Response.Status.FORBIDDEN);
+ verify(alterColumns, never()).proceed();
+ }
+
+ @Test
+ void testExplicitDenyOverridesModifyTable() throws Throwable {
+ allow(Privilege.Name.USE_CATALOG, Privilege.Name.USE_SCHEMA,
Privilege.Name.MODIFY_TABLE);
+ doAnswer(
+ invocation ->
+
Privilege.Name.MODIFY_TABLE.equals(invocation.<Privilege.Name>getArgument(3)))
+ .when(authorizer)
+ .deny(any(), any(), any(), any(), any());
+
+ MethodInvocation dropColumns = dropColumnsInvocation(tableId());
+ assertErrorResponse(interceptor.invoke(dropColumns),
Response.Status.FORBIDDEN);
+ verify(dropColumns, never()).proceed();
+ MethodInvocation alterColumns = alterColumnsInvocation(tableId());
+ assertErrorResponse(interceptor.invoke(alterColumns),
Response.Status.FORBIDDEN);
+ verify(alterColumns, never()).proceed();
+ }
+
+ @Test
+ void testRemovingATableRequiresOwnership() throws Throwable {
+ // MODIFY_TABLE alters a table but never removes it.
+ allow(
+ Privilege.Name.USE_CATALOG,
+ Privilege.Name.USE_SCHEMA,
+ Privilege.Name.MODIFY_TABLE,
+ Privilege.Name.CREATE_TABLE);
+ MethodInvocation drop = dropTableInvocation(tableId());
+ assertErrorResponse(interceptor.invoke(drop), Response.Status.FORBIDDEN);
+ verify(drop, never()).proceed();
+ MethodInvocation deregister = deregisterTableInvocation(tableId());
+ assertErrorResponse(interceptor.invoke(deregister),
Response.Status.FORBIDDEN);
+ verify(deregister, never()).proceed();
+
+ when(authorizer.isOwner(any(), any(), any(), any())).thenReturn(true);
+ assertEquals(PROCEEDED,
interceptor.invoke(dropTableInvocation(tableId())));
+ assertEquals(PROCEEDED,
interceptor.invoke(deregisterTableInvocation(tableId())));
+ }
+
+ @Test
+ void testAncestorOwnershipAuthorizesRemoval() throws Throwable {
+ allow(Privilege.Name.USE_CATALOG, Privilege.Name.USE_SCHEMA);
+
+ // SCHEMA_OWNER_WITH_USE_CATALOG: owning the schema removes tables inside
it.
+ ownerOf(MetadataObject.Type.SCHEMA);
+ assertEquals(PROCEEDED,
interceptor.invoke(dropTableInvocation(tableId())));
+ assertEquals(PROCEEDED,
interceptor.invoke(deregisterTableInvocation(tableId())));
+
+ // ANY(OWNER, METALAKE, CATALOG): owning an ancestor above the schema is
enough on its own.
+ ownerOf(MetadataObject.Type.CATALOG);
+ assertEquals(PROCEEDED,
interceptor.invoke(dropTableInvocation(tableId())));
+ ownerOf(MetadataObject.Type.METALAKE);
+ assertEquals(PROCEEDED,
interceptor.invoke(deregisterTableInvocation(tableId())));
+ }
+
+ @Test
+ void testSchemaOwnershipWithoutUseCatalogCannotRemoveATable() throws
Throwable {
+ // SCHEMA::OWNER only counts through SCHEMA_OWNER_WITH_USE_CATALOG, which
also requires
+ // USE_CATALOG. Losing access to the catalog therefore withdraws the
removal too.
+ allow(Privilege.Name.USE_SCHEMA);
+ ownerOf(MetadataObject.Type.SCHEMA);
+
+ MethodInvocation drop = dropTableInvocation(tableId());
+ assertErrorResponse(interceptor.invoke(drop), Response.Status.FORBIDDEN);
+ verify(drop, never()).proceed();
+ }
+
+ @Test
+ void testAncestorOwnershipAuthorizesColumnMutation() throws Throwable {
+ allow(Privilege.Name.USE_CATALOG, Privilege.Name.USE_SCHEMA);
+
+ ownerOf(MetadataObject.Type.SCHEMA);
+ assertEquals(PROCEEDED,
interceptor.invoke(dropColumnsInvocation(tableId())));
+ ownerOf(MetadataObject.Type.CATALOG);
+ assertEquals(PROCEEDED,
interceptor.invoke(alterColumnsInvocation(tableId())));
+ }
+
+ @Test
+ void testMutationExpressionsRejectIdentifiersOfTheWrongDepth() throws
Throwable {
+ when(authorizer.isOwner(any(), any(), any(), any())).thenReturn(true);
+ allow(Privilege.Name.values());
+
+ // A schema identifier must not be authorized as if it addressed a table.
+ assertErrorResponse(
+ interceptor.invoke(dropTableInvocation(CATALOG + "$" + SCHEMA)),
Response.Status.FORBIDDEN);
+ assertErrorResponse(
+ interceptor.invoke(dropColumnsInvocation(CATALOG)),
Response.Status.FORBIDDEN);
+ }
+
+ private MethodInvocation dropTableInvocation(String tableId) throws
Throwable {
+ Method method =
+ LanceTableOperations.class.getMethod(
+ "dropTable", String.class, String.class, HttpHeaders.class,
DropTableRequest.class);
+ return invocation(method, tableId, "$", null, new DropTableRequest());
+ }
+
+ private MethodInvocation deregisterTableInvocation(String tableId) throws
Throwable {
+ Method method =
+ LanceTableOperations.class.getMethod(
+ "deregisterTable",
+ String.class,
+ String.class,
+ HttpHeaders.class,
+ DeregisterTableRequest.class);
+ return invocation(method, tableId, "$", null, new
DeregisterTableRequest());
+ }
+
+ private MethodInvocation dropColumnsInvocation(String tableId) throws
Throwable {
+ Method method =
+ LanceTableOperations.class.getMethod(
+ "dropColumns",
+ String.class,
+ String.class,
+ HttpHeaders.class,
+ AlterTableDropColumnsRequest.class);
+ return invocation(method, tableId, "$", null, new
AlterTableDropColumnsRequest());
+ }
+
+ private MethodInvocation alterColumnsInvocation(String tableId) throws
Throwable {
+ Method method =
+ LanceTableOperations.class.getMethod(
+ "alterColumns",
+ String.class,
+ String.class,
+ HttpHeaders.class,
+ AlterTableAlterColumnsRequest.class);
+ return invocation(method, tableId, "$", null, new
AlterTableAlterColumnsRequest());
+ }
+
private MethodInvocation createTableInvocation(String tableId, String mode)
throws Throwable {
Method method =
LanceTableOperations.class.getMethod(
@@ -461,6 +609,18 @@ class TestLanceMetadataAuthorizationMethodInterceptor {
return invocation(method, namespaceId, delimiter, new
DropNamespaceRequest());
}
+ /** Makes the caller the owner of exactly the given metadata levels, and of
nothing else. */
+ private void ownerOf(MetadataObject.Type... types) {
+ Set<MetadataObject.Type> owned = Set.of(types);
+ doAnswer(
+ invocation -> {
+ MetadataObject metadataObject = invocation.getArgument(2);
+ return metadataObject != null &&
owned.contains(metadataObject.type());
+ })
+ .when(authorizer)
+ .isOwner(any(), any(), any(), any());
+ }
+
private void allow(Privilege.Name... privileges) {
Set<Privilege.Name> allowed = Set.of(privileges);
// doAnswer, not when(...): a test that narrows the allowed privileges
calls this twice, and
diff --git
a/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/authorization/TestLanceRESTEndpointAuthorizationCoverage.java
b/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/authorization/TestLanceRESTEndpointAuthorizationCoverage.java
new file mode 100644
index 0000000000..3c8a64ee60
--- /dev/null
+++
b/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/authorization/TestLanceRESTEndpointAuthorizationCoverage.java
@@ -0,0 +1,152 @@
+/*
+ * 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.lance.service.authorization;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.io.File;
+import java.lang.reflect.Method;
+import java.net.URL;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Enumeration;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+import javax.ws.rs.HttpMethod;
+import javax.ws.rs.Path;
+import org.apache.gravitino.lance.service.rest.LanceHealthOperations;
+import
org.apache.gravitino.server.authorization.annotations.AuthorizationExpression;
+import org.glassfish.hk2.api.Descriptor;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Guards the completeness of Lance REST authorization.
+ *
+ * <p>The shared interception pipeline authorizes a method only when it
carries {@link
+ * AuthorizationExpression}; a method without one is invoked unchecked. Adding
an endpoint therefore
+ * opens a hole that no behavioral test would notice, because the new endpoint
simply has no
+ * authorization test of its own. These tests scan the REST resource package
instead of listing the
+ * endpoints, so a new endpoint or a new resource class fails here until it is
authorized.
+ */
+class TestLanceRESTEndpointAuthorizationCoverage {
+
+ private static final String RESOURCE_PACKAGE =
"org.apache.gravitino.lance.service.rest";
+
+ /**
+ * Health checks answer before any metalake is known and are served to
unauthenticated liveness
+ * probes, so they deliberately carry no authorization annotation.
+ */
+ private static final Set<Class<?>> UNAUTHORIZED_RESOURCES =
Set.of(LanceHealthOperations.class);
+
+ @Test
+ void testEveryRESTEndpointDeclaresAnAuthorizationExpression() throws
Exception {
+ List<Class<?>> resources = authorizedResourceClasses();
+
+ List<String> unauthorized =
+ resources.stream()
+ .flatMap(resource -> Arrays.stream(resource.getDeclaredMethods()))
+ .filter(TestLanceRESTEndpointAuthorizationCoverage::isRestEndpoint)
+ .filter(method ->
!method.isAnnotationPresent(AuthorizationExpression.class))
+ .map(method -> method.getDeclaringClass().getSimpleName() + "#" +
method.getName())
+ .sorted()
+ .collect(Collectors.toList());
+
+ assertTrue(
+ unauthorized.isEmpty(),
+ "Lance REST endpoints without @AuthorizationExpression are served
unchecked: "
+ + unauthorized);
+ }
+
+ @Test
+ void testEveryAuthorizedResourceIsIntercepted() throws Exception {
+ LanceRESTAuthInterceptionService interceptionService =
+ new LanceRESTAuthInterceptionService("test_metalake");
+
+ for (Class<?> resource : authorizedResourceClasses()) {
+ assertTrue(
+
interceptionService.getDescriptorFilter().matches(descriptorOf(resource)),
+ resource.getName()
+ + " declares authorization expressions but is not intercepted,
so they are never "
+ + "evaluated");
+ }
+
+ // The exception is explicit rather than incidental: a resource is
unauthorized only because it
+ // is listed here, and listing it also keeps it out of the interception
service.
+ for (Class<?> resource : UNAUTHORIZED_RESOURCES) {
+ assertFalse(
+
interceptionService.getDescriptorFilter().matches(descriptorOf(resource)),
+ resource.getName() + " is intercepted but declares no authorization
expression");
+ }
+ }
+
+ private static boolean isRestEndpoint(Method method) {
+ // A JAX-RS HTTP method is any annotation that is itself meta-annotated
with @HttpMethod, which
+ // covers @GET, @POST, @PUT, @DELETE, @HEAD and @PATCH without listing
them.
+ return Arrays.stream(method.getAnnotations())
+ .anyMatch(annotation ->
annotation.annotationType().isAnnotationPresent(HttpMethod.class));
+ }
+
+ private static Descriptor descriptorOf(Class<?> resource) {
+ Descriptor descriptor = mock(Descriptor.class);
+ when(descriptor.getImplementation()).thenReturn(resource.getName());
+ return descriptor;
+ }
+
+ /** Returns the JAX-RS resource classes that must be authorized. */
+ private static List<Class<?>> authorizedResourceClasses() throws Exception {
+ List<Class<?>> resources =
+ scanPackage().stream()
+ .filter(clazz -> clazz.isAnnotationPresent(Path.class))
+ .filter(clazz -> !UNAUTHORIZED_RESOURCES.contains(clazz))
+ .collect(Collectors.toList());
+
+ // Without this the scan silently passing would make both tests vacuous.
+ assertFalse(resources.isEmpty(), "No JAX-RS resource found in " +
RESOURCE_PACKAGE);
+ return resources;
+ }
+
+ private static List<Class<?>> scanPackage() throws Exception {
+ String resourcePath = RESOURCE_PACKAGE.replace('.', '/');
+ List<Class<?>> classes = new ArrayList<>();
+ Enumeration<URL> roots =
+
Thread.currentThread().getContextClassLoader().getResources(resourcePath);
+ while (roots.hasMoreElements()) {
+ URL root = roots.nextElement();
+ if (!"file".equals(root.getProtocol())) {
+ continue;
+ }
+ File[] files =
+ new File(root.toURI())
+ .listFiles((directory, name) -> name.endsWith(".class") &&
!name.contains("$"));
+ if (files == null) {
+ continue;
+ }
+ for (File file : files) {
+ String simpleName =
+ file.getName().substring(0, file.getName().length() -
".class".length());
+ classes.add(Class.forName(RESOURCE_PACKAGE + "." + simpleName));
+ }
+ }
+ return classes;
+ }
+}
