[#12562] feat(lance): Add table mutation and deletion authorization - #12696
Merged
yuqi1129 merged 3 commits intoSep 3, 2026
Conversation
Code Coverage Report
Files
|
yuqi1129
force-pushed
the
feat/12562-lance-table-mutation-authz
branch
from
September 2, 2026 15:08
7a69a61 to
f92433a
Compare
…tion Column changes require MODIFY_TABLE or ownership, while deregister-table and drop-table require ownership: MODIFY_TABLE alters a table but never removes it, matching the Gravitino and Iceberg REST surfaces. A table the caller may not remove is reported as forbidden whether or not it exists, so the endpoints cannot be used to probe for existence. Claude-Session: https://claude.ai/code/session_01EPJduJLZAREBg1bKxybe1e
yuqi1129
force-pushed
the
feat/12562-lance-table-mutation-authz
branch
from
September 3, 2026 07:52
f92433a to
d582b7a
Compare
… ownership Add a test that scans the REST resource package and fails when an endpoint carries no authorization expression, or when a resource that declares one is not intercepted. The shared pipeline authorizes a method only when it is annotated, so a new endpoint would otherwise be served unchecked and no behavioral test would notice. Cover the ancestor-ownership branches of the removal and mutation expressions directly: 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. Claude-Session: https://claude.ai/code/session_01EPJduJLZAREBg1bKxybe1e
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
One new integration test case asserts a privilege distinction but currently denies due to missing schema/table access rather than demonstrating that SELECT_TABLE doesn’t authorize mutations.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Completes authorization coverage for the remaining Lance REST table-mutation and deletion endpoints by adding explicit authorization expressions and extending unit/integration tests to verify denial-before-mutation and concealment semantics.
Changes:
- Added
@AuthorizationExpressionguards forderegister,drop,drop_columns, andalter_columnsLance REST endpoints. - Introduced
DROP_TABLE_AUTHORIZATION_EXPRESSIONand applied it to table removal operations; applied modify-table expression to column mutations. - Added test coverage, including a new “authorization coverage” test that scans REST resources and asserts all endpoints are annotated and intercepted.
File summaries
| File | Description |
|---|---|
| lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/authorization/TestLanceRESTEndpointAuthorizationCoverage.java | Adds a package-scan based guard to ensure all REST endpoints are annotated and intercepted. |
| lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/authorization/TestLanceMetadataAuthorizationMethodInterceptor.java | Extends interceptor unit tests for column mutation and table removal authorization semantics (including ancestor ownership). |
| lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceTableAuthorizationIT.java | Extends integration tests to cover mutation/removal authz, concealment, and “no mutation on denial”. |
| lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java | Adds @AuthorizationExpression annotations to the remaining unguarded table endpoints. |
| lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/authorization/LanceAuthorizationExpressions.java | Introduces DROP_TABLE_AUTHORIZATION_EXPRESSION for table removal operations. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+270
to
+273
| // Selecting a table does not authorize changing its columns. | ||
| assertStatus(403, dropColumns(READER, MUTABLE_TABLE, "value")); | ||
| assertStatus(403, alterColumns(READER, MUTABLE_TABLE, "value", "renamed")); | ||
| Assertions.assertEquals(List.of("id", "value"), describedColumns(MUTABLE_TABLE)); |
Contributor
Author
There was a problem hiding this comment.
Good catch, this was a real problem with the test. READER had no USE_SCHEMA on WRITE_SCHEMA and no SELECT_TABLE on MUTABLE_TABLE, so the 403 only showed that it could not reach the table, not that SELECT_TABLE fails to authorize a column change.
Fixed in 6fd5489: READER now holds USE_SCHEMA on WRITE_SCHEMA and SELECT_TABLE on MUTABLE_TABLE, and the test asserts describe returns 200 for it before the two denials. The only privilege it is missing now is MODIFY_TABLE, so the denials say what the test claims.
… own point The reader had no access to the write schema at all, so its denied column changes were evidence that it could not reach the table, not evidence that SELECT_TABLE fails to authorize a column change. Grant the reader USE_SCHEMA on the write schema and SELECT_TABLE on the table it is denied on, and assert it can describe that table before the denials, so the test states the property it claims to test. Claude-Session: https://claude.ai/code/session_01EPJduJLZAREBg1bKxybe1e
roryqi
approved these changes
Sep 3, 2026
jerryshao
pushed a commit
that referenced
this pull request
Sep 4, 2026
…sions (#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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_columnsandalter_columns.drop_columnsandalter_columnsrequire MODIFY_TABLE or ownership.deregisteranddroprequire ownership of the table or of one of its ancestors. MODIFY_TABLE alters a table but never removes it.deregister(which removes only the Gravitino metadata) anddrop(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?
TestLanceMetadataAuthorizationMethodInterceptorandLanceTableAuthorizationITcover 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.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.New
TestLanceRESTEndpointAuthorizationCoverageguards 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:./gradlew :lance:lance-rest-server:build :lance:lance-common:build— 202 tests, all passing.