jerryshao commented on code in PR #13086:
URL: https://github.com/apache/gravitino/pull/13086#discussion_r3986344437
##########
docs/open-api/catalogs.yaml:
##########
@@ -165,7 +165,11 @@ paths:
regular catalog operations, but does not guarantee that every
object-level or mutating
operation will succeed. Fileset catalogs test all catalog-level
`location` and `location-*`
targets. Model and Generic catalogs do not support connection testing.
Expected test
- failures are returned as application error codes in an HTTP 200
response.
+ failures are returned as application error codes in an HTTP 200
response. When
+ authorization is enabled, testing with the stored configuration
requires the same access
+ as loading the catalog, and testing with proposed changes requires
owning the metalake or
+ the catalog, the same as altering it. Callers without that access
receive an HTTP 403
+ response.
Review Comment:
Added an explicit `403` response using `ErrorModel` to
`testExistingCatalogConnection` in `catalogs.yaml` (d8714da41).
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
server/src/main/java/org/apache/gravitino/server/web/rest/CatalogOperations.java:
##########
@@ -230,7 +232,7 @@ public Response testConnection(
@Produces("application/vnd.gravitino.v1+json")
@Timed(name = "test-existing-connection." +
MetricNames.HTTP_PROCESS_DURATION, absolute = true)
@AuthorizationExpression(
- expression = "ANY(OWNER, METALAKE, CATALOG)",
+ expression =
AuthorizationExpressionConstants.LOAD_CATALOG_AUTHORIZATION_EXPRESSION,
Review Comment:
Moved the ownership check out of the method body into a new
`CatalogConnectionTestAuthorizationExecutor` (request type
`TEST_CATALOG_CONNECTION`). `AuthorizationHandler` is only wired into the
Iceberg and Lance interceptors, and the core REST interceptor selects executors
instead, so this follows `LoadTableAuthorizationExecutor`: the annotation
declares `secondaryExpression` with condition `HAS_PROPOSED_CHANGES`, and the
executor switches to it when `updates` is non-empty (d8714da41).
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
server/src/main/java/org/apache/gravitino/server/web/rest/CatalogOperations.java:
##########
@@ -230,7 +232,7 @@ public Response testConnection(
@Produces("application/vnd.gravitino.v1+json")
@Timed(name = "test-existing-connection." +
MetricNames.HTTP_PROCESS_DURATION, absolute = true)
@AuthorizationExpression(
- expression = "ANY(OWNER, METALAKE, CATALOG)",
+ expression =
AuthorizationExpressionConstants.LOAD_CATALOG_AUTHORIZATION_EXPRESSION,
Review Comment:
Followed `LoadTableAuthorizationExecutor`, which is the counterpart of those
handlers in the core REST interceptor. See the reply above for details
(d8714da41).
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
