Repository navigation
[#13085] improvement(server): Allow catalog users to test existing catalog connections with stored configuration - #13086
Merged
Conversation
…ing catalog connections with stored configuration Authorize testing an existing catalog with its stored configuration the same as loading the catalog. Testing with proposed changes still requires metalake or catalog ownership, and a denial now returns HTTP 403 instead of 500. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The OpenAPI operation still lacks an explicit 403 response matching the documented and implemented behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates catalog connection testing authorization: stored configurations follow USE_CATALOG, while proposed changes require ownership.
Changes:
- Added conditional authorization and HTTP 403 handling.
- Added unit and integration coverage.
- Updated access-control and OpenAPI documentation.
File summaries
| File | Description |
|---|---|
server/src/test/java/org/apache/gravitino/server/web/rest/TestCatalogOperations.java |
Tests authorization behavior. |
server/src/test/java/org/apache/gravitino/server/web/rest/authorization/TestCatalogAuthorizationExpression.java |
Tests authorization expressions. |
server/src/main/java/org/apache/gravitino/server/web/rest/ExceptionHandlers.java |
Maps forbidden errors to HTTP 403. |
server/src/main/java/org/apache/gravitino/server/web/rest/CatalogOperations.java |
Applies conditional authorization. |
server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionConstants.java |
Adds the ownership authorization expression. |
docs/security/access-control.md |
Documents access rules. |
docs/open-api/catalogs.yaml |
Documents endpoint authorization behavior. |
clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/CatalogAuthorizationIT.java |
Adds end-to-end authorization coverage. |
Review details
- Files reviewed: 8/8 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.
roryqi
reviewed
Sep 11, 2026
…ting catalog connection tests Replace the in-method ownership check with CatalogConnectionTestAuthorizationExecutor, which switches to the annotation's secondary expression when the request carries proposed changes. Revert the test-connection 403 mapping that is no longer needed, and declare the 403 response in the OpenAPI spec. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…uthorization through the interceptor Add interceptor tests that use the real CatalogOperations annotations to verify USE_CATALOG can test the stored configuration, proposed changes are denied without ownership, and the catalog owner can test proposed changes. Also cover the executor factory and a request without updates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…n authorization denial events An authorization executor can evaluate a request-specific expression, such as a secondary expression. Report the expression it recorded in the request context in the denial event and log, and fall back to the annotation expression otherwise. Also cover the forbidden error mapping of the Python catalog error handler. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Code Coverage Report
Files
|
jerryshao
added a commit
that referenced
this pull request
Sep 11, 2026
…og users to test existing catalog connections with stored configuration (#13086) (#13088) **Cherry-pick Information:** - Original PR: #13086 (still open; this backport is raised ahead of its merge, and must be kept in sync with any review changes there) - Original issue: #13085 - Target branch: `branch-1.3` - Status: ✅ Conflicts resolved manually **Resolution:** - Applied the final #13086 change as one commit instead of replaying its individual review commits. - `TestGravitinoInterceptionService`: `branch-1.3` does not have the table existence-probe tests or their helpers, so only the new catalog connection tests, their helpers, and their imports are added. - `TestCatalogConnectionTestAuthorizationExecutor`: `AuthorizeExecutorFactory.create` on `branch-1.3` has no `allowCheckExistenceExpression` parameter, so the factory test passes nine arguments. - Production code, docs, the integration test, and the Python test are the same as #13086. **Validation:** - Server unit tests on `branch-1.3`: all `org.apache.gravitino.server.web.filter.*` tests (including the new interceptor and executor tests), `TestCatalogAuthorizationExpression`, and `TestCatalogOperations`. - Compiled the Java client test sources, including `CatalogAuthorizationIT`. The Docker integration test and the Python unit test were not run locally. - `./gradlew spotlessApply`, `:server-common:javadoc`, `:docs:build` - Conflict-marker scan and `git diff --check` 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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?
testExistingConnectionwith the load-catalog expression.CatalogConnectionTestAuthorizationExecutor(request typeTEST_CATALOG_CONNECTION). When the request carries non-emptyupdates, it switches to the annotation's secondary expression, which requires metalake or catalog ownership (TEST_CATALOG_CONNECTION_WITH_CHANGES_AUTHORIZATION_EXPRESSION, conditionHAS_PROPOSED_CHANGES).GravitinoInterceptionService, report the expression the executor evaluated (recorded inAuthorizationRequestContext) in the authorization denial event and log, falling back to the annotation expression. Before this change, a denial by a secondary expression was reported with the primary expression. This applies to every executor-based endpoint; endpoints that evaluate only the annotation expression report the same value as before.docs/security/access-control.mdand declare the 403 response indocs/open-api/catalogs.yaml.Why are the changes needed?
A user who can load a catalog already reaches the backing system with its stored configuration. Owner-only for that test adds no protection. Proposed changes still require ownership because the caller chooses what the server connects to.
Fix: #13085
Does this PR introduce any user-facing change?
Yes.
USE_CATALOGcan now test an existing catalog with its stored configuration.How was this patch tested?
TestCatalogConnectionTestAuthorizationExecutor: expression selection for no body, empty or nullupdates, proposed changes, and theNEVERcondition, plus creation throughAuthorizeExecutorFactory.TestGravitinoInterceptionService: the realCatalogOperationsannotations through the interceptor. AUSE_CATALOGuser can test the stored configuration and is denied for proposed changes (the denial event reports the owner expression); the catalog owner can test proposed changes.TestCatalogAuthorizationExpression: the primary and secondary expressions ontestExistingConnection.CatalogAuthorizationIT#testTestExistingCatalogConnection: a user withUSE_CATALOGand the owner end to end.test_error_handler: the catalog error handler raisesForbiddenExceptionfor a forbidden response../gradlew :server:test --tests 'org.apache.gravitino.server.web.filter.*' --tests '*TestCatalogAuthorizationExpression' --tests '*TestCatalogOperations' :docs:build -PskipITs🤖 Generated with Claude Code