This is an automated email from the ASF dual-hosted git repository.
jerryshao 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 bb9a7e5e42 [#13085] improvement(server): Allow catalog users to test
existing catalog connections with stored configuration (#13086)
bb9a7e5e42 is described below
commit bb9a7e5e4230546f225bea80184d7bd382ecdd33
Author: Jerry Shao <[email protected]>
AuthorDate: Fri Sep 11 16:24:26 2026 +0800
[#13085] improvement(server): Allow catalog users to test existing catalog
connections with stored configuration (#13086)
### What changes were proposed in this pull request?
- Authorize `testExistingConnection` with the load-catalog expression.
- Add `CatalogConnectionTestAuthorizationExecutor` (request type
`TEST_CATALOG_CONNECTION`). When the request carries non-empty
`updates`, it switches to the annotation's secondary expression, which
requires metalake or catalog ownership
(`TEST_CATALOG_CONNECTION_WITH_CHANGES_AUTHORIZATION_EXPRESSION`,
condition `HAS_PROPOSED_CHANGES`).
- In `GravitinoInterceptionService`, report the expression the executor
evaluated (recorded in `AuthorizationRequestContext`) 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.
- Document the rule in `docs/security/access-control.md` and declare the
403 response in `docs/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.
- Users with `USE_CATALOG` can now test an existing catalog with its
stored configuration.
- Testing with proposed changes still requires metalake or catalog
ownership.
- Authorization denial audit events report the expression that was
actually evaluated.
### How was this patch tested?
- `TestCatalogConnectionTestAuthorizationExecutor`: expression selection
for no body, empty or null `updates`, proposed changes, and the `NEVER`
condition, plus creation through `AuthorizeExecutorFactory`.
- `TestGravitinoInterceptionService`: the real `CatalogOperations`
annotations through the interceptor. A `USE_CATALOG` user 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 on `testExistingConnection`.
- `CatalogAuthorizationIT#testTestExistingCatalogConnection`: a user
with `USE_CATALOG` and the owner end to end.
- Python `test_error_handler`: the catalog error handler raises
`ForbiddenException` for a forbidden response.
- `./gradlew :server:test --tests
'org.apache.gravitino.server.web.filter.*' --tests
'*TestCatalogAuthorizationExpression' --tests '*TestCatalogOperations'
:docs:build -PskipITs`
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5 <[email protected]>
---
.../test/authorization/CatalogAuthorizationIT.java | 44 ++++++-
.../tests/unittests/test_error_handler.py | 5 +
docs/open-api/catalogs.yaml | 14 ++-
docs/security/access-control.md | 5 +
.../annotations/AuthorizationRequest.java | 3 +-
.../annotations/ExpressionCondition.java | 4 +-
.../AuthorizationExpressionConstants.java | 9 ++
.../web/filter/GravitinoInterceptionService.java | 10 +-
.../authorization/AuthorizeExecutorFactory.java | 9 ++
...CatalogConnectionTestAuthorizationExecutor.java | 80 ++++++++++++
.../server/web/rest/CatalogOperations.java | 11 +-
.../filter/TestGravitinoInterceptionService.java | 134 +++++++++++++++++++++
...CatalogConnectionTestAuthorizationExecutor.java | 132 ++++++++++++++++++++
.../TestCatalogAuthorizationExpression.java | 46 +++++++
14 files changed, 498 insertions(+), 8 deletions(-)
diff --git
a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/CatalogAuthorizationIT.java
b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/CatalogAuthorizationIT.java
index f67f1168fc..004b20fdc9 100644
---
a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/CatalogAuthorizationIT.java
+++
b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/CatalogAuthorizationIT.java
@@ -21,10 +21,16 @@ import static org.junit.Assert.assertThrows;
import static org.junit.jupiter.api.Assertions.assertArrayEquals;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.Maps;
import java.lang.reflect.Method;
+import java.util.HashMap;
import java.util.Map;
import org.apache.gravitino.Catalog;
+import org.apache.gravitino.CatalogChange;
+import org.apache.gravitino.authorization.Privilege;
+import org.apache.gravitino.authorization.Privileges;
+import org.apache.gravitino.authorization.SecurableObjects;
import org.apache.gravitino.client.GravitinoMetalake;
import org.apache.gravitino.dto.MetalakeDTO;
import org.apache.gravitino.exceptions.ForbiddenException;
@@ -131,6 +137,42 @@ public class CatalogAuthorizationIT extends
BaseRestApiAuthorizationIT {
@Test
@Order(3)
+ public void testTestExistingCatalogConnection() throws Exception {
+ GravitinoMetalake adminMetalake = client.loadMetalake(METALAKE);
+ CatalogChange proposedChange = CatalogChange.updateComment("proposed
comment");
+ assertThrows(
+ "Can not access metadata {" + catalog1 + "}.",
+ ForbiddenException.class,
+ () ->
normalUserClient.loadMetalake(METALAKE).testConnection(catalog1));
+
+ // USE_CATALOG allows testing the stored configuration, but not proposed
changes.
+ String role = "testConnectionRole";
+ adminMetalake.createRole(
+ role,
+ new HashMap<>(),
+ ImmutableList.of(
+ SecurableObjects.ofCatalog(
+ catalog1,
ImmutableList.<Privilege>of(Privileges.UseCatalog.allow()))));
+ adminMetalake.grantRolesToUser(ImmutableList.of(role), NORMAL_USER);
+ try {
+ GravitinoMetalake normalUserMetalake =
normalUserClient.loadMetalake(METALAKE);
+ normalUserMetalake.testConnection(catalog1);
+ assertThrows(
+ "Can not access metadata {" + catalog1 + "}.",
+ ForbiddenException.class,
+ () -> normalUserMetalake.testConnection(catalog1, proposedChange));
+ } finally {
+ adminMetalake.revokeRolesFromUser(ImmutableList.of(role), NORMAL_USER);
+ adminMetalake.deleteRole(role);
+ }
+
+ // The owner can test both the stored configuration and proposed changes.
+ adminMetalake.testConnection(catalog1);
+ adminMetalake.testConnection(catalog1, proposedChange);
+ }
+
+ @Test
+ @Order(4)
public void testDeleteCatalog() {
String[] catalogs = client.loadMetalake(METALAKE).listCatalogs();
assertEquals(2, catalogs.length);
@@ -151,7 +193,7 @@ public class CatalogAuthorizationIT extends
BaseRestApiAuthorizationIT {
}
@Test
- @Order(4)
+ @Order(5)
public void testListCatalogsWithNonExistentMetalake() throws Exception {
// Test that listCatalogs with @AuthorizationExpression returns 403
Forbidden
// when the metalake doesn't exist, instead of 404 response
diff --git a/clients/client-python/tests/unittests/test_error_handler.py
b/clients/client-python/tests/unittests/test_error_handler.py
index 64c190e43d..778581450e 100644
--- a/clients/client-python/tests/unittests/test_error_handler.py
+++ b/clients/client-python/tests/unittests/test_error_handler.py
@@ -230,6 +230,11 @@ class TestErrorHandler(unittest.TestCase):
)
)
+ with self.assertRaises(ForbiddenException):
+ CATALOG_ERROR_HANDLER.handle(
+ ErrorResponse.generate_error_response(ForbiddenException,
"mock error")
+ )
+
with self.assertRaises(InternalError):
CATALOG_ERROR_HANDLER.handle(
ErrorResponse.generate_error_response(InternalError, "mock
error")
diff --git a/docs/open-api/catalogs.yaml b/docs/open-api/catalogs.yaml
index 973be684dd..cc290f39db 100644
--- a/docs/open-api/catalogs.yaml
+++ b/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.
operationId: testExistingCatalogConnection
requestBody:
required: false
@@ -225,6 +229,14 @@ paths:
message: Catalog my_metalake.my_catalog is not in use
"400":
$ref: "./openapi.yaml#/components/responses/BadRequestErrorResponse"
+ "403":
+ description: >-
+ Forbidden - The caller cannot load the catalog, or tests proposed
changes without
+ owning the metalake or the catalog
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "./openapi.yaml#/components/schemas/ErrorModel"
"5xx":
$ref: "./openapi.yaml#/components/responses/ServerErrorResponse"
diff --git a/docs/security/access-control.md b/docs/security/access-control.md
index 69b1827651..397a923a9b 100755
--- a/docs/security/access-control.md
+++ b/docs/security/access-control.md
@@ -254,6 +254,11 @@ return only the entries the caller is entitled to see,
which for a metalake owne
| Model | `REGISTER_MODEL` | `USE_MODEL` |
Owner | Owner |
| Function | `REGISTER_FUNCTION` | `EXECUTE_FUNCTION` or `MODIFY_FUNCTION` |
`MODIFY_FUNCTION` | Owner |
+Testing a catalog connection follows the catalog row. Testing a catalog before
it is created takes
+`CREATE_CATALOG`. Testing an existing catalog with its stored configuration
takes `USE_CATALOG`, the
+same as loading it. Testing an existing catalog with proposed changes that are
not saved takes
+ownership, the same as altering it, because the caller chooses what the server
connects to.
+
Table statistics follow the table itself: reading them takes `SELECT_TABLE` or
`MODIFY_TABLE`,
writing them takes `MODIFY_TABLE`. Model versions follow the model:
`USE_MODEL` to read, owner to
alter or delete. Fetching plaintext secrets (`getSecrets`) or vend credentials
(`getCredentials`)
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/authorization/annotations/AuthorizationRequest.java
b/server-common/src/main/java/org/apache/gravitino/server/authorization/annotations/AuthorizationRequest.java
index 2492fc9976..f90679324f 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/authorization/annotations/AuthorizationRequest.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/authorization/annotations/AuthorizationRequest.java
@@ -35,6 +35,7 @@ public @interface AuthorizationRequest {
RUN_JOB,
LINEAGE,
LOAD_TABLE,
- CREATE_SCHEMA
+ CREATE_SCHEMA,
+ TEST_CATALOG_CONNECTION
}
}
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/authorization/annotations/ExpressionCondition.java
b/server-common/src/main/java/org/apache/gravitino/server/authorization/annotations/ExpressionCondition.java
index e811871217..f1bd58c1c6 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/authorization/annotations/ExpressionCondition.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/authorization/annotations/ExpressionCondition.java
@@ -20,5 +20,7 @@ package org.apache.gravitino.server.authorization.annotations;
public enum ExpressionCondition {
NEVER,
- REQUIRED_MODIFY_PRIVILEGES
+ REQUIRED_MODIFY_PRIVILEGES,
+ /** The request carries proposed changes that are not saved, for example in
its request body. */
+ HAS_PROPOSED_CHANGES
}
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionConstants.java
b/server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionConstants.java
index ac7a073daf..705025153e 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionConstants.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionConstants.java
@@ -20,6 +20,15 @@ public class AuthorizationExpressionConstants {
public static final String LOAD_CATALOG_AUTHORIZATION_EXPRESSION =
"ANY_USE_CATALOG || ANY(OWNER, METALAKE, CATALOG)";
+ /**
+ * Authorizes testing an existing catalog connection with proposed changes.
The caller chooses the
+ * configuration the server connects to, so this matches the authorization
for altering the
+ * catalog. Testing with the stored configuration uses {@link
+ * #LOAD_CATALOG_AUTHORIZATION_EXPRESSION} instead.
+ */
+ public static final String
TEST_CATALOG_CONNECTION_WITH_CHANGES_AUTHORIZATION_EXPRESSION =
+ "ANY(OWNER, METALAKE, CATALOG)";
+
public static final String LOAD_SCHEMA_AUTHORIZATION_EXPRESSION =
"""
ANY(OWNER, METALAKE, CATALOG) ||
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java
b/server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java
index cb82a46798..af16c2efd1 100644
---
a/server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java
+++
b/server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java
@@ -256,12 +256,18 @@ public class GravitinoInterceptionService implements
InterceptionService {
MetadataObject.Type type =
expressionAnnotation.accessMetadataType();
NameIdentifier accessMetadataName =
metadataContext.get(Entity.EntityType.valueOf(type.name()));
+ // An executor can evaluate a request-specific expression, such
as the secondary
+ // expression, so report the expression it evaluated when it
recorded one.
+ String evaluatedExpression =
+ StringUtils.defaultIfBlank(
+
authorizationRequestContext.getOriginalAuthorizationExpression(), expression);
dispatchAuthzDenialEvent(
PrincipalUtils.getCurrentUserName(),
accessMetadataName,
method.getName(),
- expression);
- return buildNoAuthResponse(expressionAnnotation,
metadataContext, method, expression);
+ evaluatedExpression);
+ return buildNoAuthResponse(
+ expressionAnnotation, metadataContext, method,
evaluatedExpression);
}
}
}
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/filter/authorization/AuthorizeExecutorFactory.java
b/server/src/main/java/org/apache/gravitino/server/web/filter/authorization/AuthorizeExecutorFactory.java
index aae03326ae..12a526cb2b 100644
---
a/server/src/main/java/org/apache/gravitino/server/web/filter/authorization/AuthorizeExecutorFactory.java
+++
b/server/src/main/java/org/apache/gravitino/server/web/filter/authorization/AuthorizeExecutorFactory.java
@@ -60,6 +60,15 @@ public class AuthorizeExecutorFactory {
allowCheckExistenceExpression);
case CREATE_SCHEMA -> new CreateSchemaAuthorizationExecutor(
parameters, args, expression, metadataContext, pathParams,
entityType);
+ case TEST_CATALOG_CONNECTION -> new
CatalogConnectionTestAuthorizationExecutor(
+ parameters,
+ args,
+ expression,
+ metadataContext,
+ pathParams,
+ entityType,
+ secondaryExpression,
+ secondaryExpressionCondition);
};
}
}
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/filter/authorization/CatalogConnectionTestAuthorizationExecutor.java
b/server/src/main/java/org/apache/gravitino/server/web/filter/authorization/CatalogConnectionTestAuthorizationExecutor.java
new file mode 100644
index 0000000000..47380d4218
--- /dev/null
+++
b/server/src/main/java/org/apache/gravitino/server/web/filter/authorization/CatalogConnectionTestAuthorizationExecutor.java
@@ -0,0 +1,80 @@
+/*
+ * 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.server.web.filter.authorization;
+
+import java.lang.reflect.Parameter;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.gravitino.Entity;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.dto.requests.CatalogUpdatesRequest;
+import
org.apache.gravitino.server.authorization.annotations.ExpressionCondition;
+import
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionEvaluator;
+import org.apache.gravitino.server.web.filter.ParameterUtil;
+
+/**
+ * Authorization executor for testing the connection of an existing catalog.
+ *
+ * <p>Testing with the stored configuration uses the default expression. When
the request carries
+ * proposed changes and the condition is {@link
ExpressionCondition#HAS_PROPOSED_CHANGES}, the
+ * secondary expression is used instead, because the caller chooses the
configuration the server
+ * connects to.
+ */
+public class CatalogConnectionTestAuthorizationExecutor extends
CommonAuthorizerExecutor {
+
+ /**
+ * Creates an authorization executor for an existing catalog connection test.
+ *
+ * @param parameters the parameters of the intercepted method
+ * @param args the arguments passed to the intercepted method
+ * @param expression the expression for testing with the stored configuration
+ * @param metadataContext the metadata context bound to the authorization
expression
+ * @param pathParams the path parameters of the request
+ * @param entityType the optional entity type of the request
+ * @param secondaryExpression the expression for testing with proposed
changes
+ * @param secondaryExpressionCondition the condition for using the secondary
expression
+ */
+ public CatalogConnectionTestAuthorizationExecutor(
+ Parameter[] parameters,
+ Object[] args,
+ String expression,
+ Map<Entity.EntityType, NameIdentifier> metadataContext,
+ Map<String, Object> pathParams,
+ Optional<String> entityType,
+ String secondaryExpression,
+ ExpressionCondition secondaryExpressionCondition) {
+ super(expression, metadataContext, pathParams, entityType);
+ if (StringUtils.isBlank(secondaryExpression)
+ || secondaryExpressionCondition !=
ExpressionCondition.HAS_PROPOSED_CHANGES) {
+ return;
+ }
+
+ // An empty change list is the same as testing the stored configuration.
+ Object request = ParameterUtil.extractFromParameters(parameters, args);
+ if (request instanceof CatalogUpdatesRequest updatesRequest
+ && updatesRequest.getUpdates() != null
+ && !updatesRequest.getUpdates().isEmpty()) {
+ this.expression = secondaryExpression;
+ this.authorizationExpressionEvaluator =
+ new AuthorizationExpressionEvaluator(secondaryExpression);
+ }
+ }
+}
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/rest/CatalogOperations.java
b/server/src/main/java/org/apache/gravitino/server/web/rest/CatalogOperations.java
index 60f17c0239..09679b7515 100644
---
a/server/src/main/java/org/apache/gravitino/server/web/rest/CatalogOperations.java
+++
b/server/src/main/java/org/apache/gravitino/server/web/rest/CatalogOperations.java
@@ -59,6 +59,8 @@ import org.apache.gravitino.metrics.MetricNames;
import org.apache.gravitino.server.authorization.MetadataAuthzHelper;
import
org.apache.gravitino.server.authorization.annotations.AuthorizationExpression;
import
org.apache.gravitino.server.authorization.annotations.AuthorizationMetadata;
+import
org.apache.gravitino.server.authorization.annotations.AuthorizationRequest;
+import
org.apache.gravitino.server.authorization.annotations.ExpressionCondition;
import
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants;
import org.apache.gravitino.server.web.Utils;
import org.apache.gravitino.utils.NameIdentifierUtil;
@@ -230,7 +232,11 @@ public class CatalogOperations {
@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,
+ secondaryExpression =
+ AuthorizationExpressionConstants
+ .TEST_CATALOG_CONNECTION_WITH_CHANGES_AUTHORIZATION_EXPRESSION,
+ secondaryExpressionCondition = ExpressionCondition.HAS_PROPOSED_CHANGES,
accessMetadataType = MetadataObject.Type.CATALOG)
@ResponseMetered(name = "test-existing-connection", absolute = true)
public Response testExistingConnection(
@@ -238,7 +244,8 @@ public class CatalogOperations {
String metalake,
@PathParam("catalog") @AuthorizationMetadata(type =
Entity.EntityType.CATALOG)
String catalogName,
- CatalogUpdatesRequest request) {
+ @AuthorizationRequest(type =
AuthorizationRequest.RequestType.TEST_CATALOG_CONNECTION)
+ CatalogUpdatesRequest request) {
LOG.info("Received test connection request for existing catalog: {}.{}",
metalake, catalogName);
try {
return Utils.doAs(
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java
b/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java
index c82726272a..572c492d68 100644
---
a/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java
+++
b/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java
@@ -20,6 +20,7 @@ package org.apache.gravitino.server.web.filter;
import static
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants.CAN_ACCESS_METADATA_AND_TAG;
import static
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants.LOAD_TABLE_AUTHORIZATION_EXPRESSION;
import static
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants.PROBE_TABLE_LIKE_AUTHORIZATION_EXPRESSION;
+import static
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants.TEST_CATALOG_CONNECTION_WITH_CHANGES_AUTHORIZATION_EXPRESSION;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
@@ -29,6 +30,7 @@ import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
+import com.google.common.collect.ImmutableList;
import java.io.IOException;
import java.lang.reflect.Method;
import java.security.Principal;
@@ -52,6 +54,8 @@ import org.apache.gravitino.authorization.GravitinoAuthorizer;
import org.apache.gravitino.authorization.Privilege;
import org.apache.gravitino.catalog.TableDispatcher;
import org.apache.gravitino.catalog.ViewDispatcher;
+import org.apache.gravitino.dto.requests.CatalogUpdateRequest;
+import org.apache.gravitino.dto.requests.CatalogUpdatesRequest;
import org.apache.gravitino.dto.requests.SchemaCreateRequest;
import org.apache.gravitino.dto.requests.TagValuesAssociateRequest;
import org.apache.gravitino.dto.responses.ErrorConstants;
@@ -69,6 +73,7 @@ import
org.apache.gravitino.server.authorization.annotations.AuthorizationMetada
import
org.apache.gravitino.server.authorization.annotations.AuthorizationObjectType;
import
org.apache.gravitino.server.authorization.annotations.AuthorizationRequest;
import org.apache.gravitino.server.web.Utils;
+import org.apache.gravitino.server.web.rest.CatalogOperations;
import org.apache.gravitino.server.web.rest.MetadataObjectTagOperations;
import org.apache.gravitino.server.web.rest.SchemaOperations;
import org.apache.gravitino.server.web.rest.SecretsProviderOperations;
@@ -913,6 +918,58 @@ public class TestGravitinoInterceptionService {
}
}
+ @Test
+ public void
testExistingCatalogConnectionAllowsUseCatalogWithoutProposedChanges()
+ throws Throwable {
+ GravitinoAuthorizer authorizer = catalogConnectionAuthorizer(true, false);
+
+ MethodInvocation noBody = testExistingConnectionInvocation(null);
+ Response noBodyResponse =
+ invokeTestExistingConnection(authorizer, mock(EventBus.class), noBody);
+ assertEquals(Response.Status.OK.getStatusCode(),
noBodyResponse.getStatus());
+ verify(noBody).proceed();
+
+ MethodInvocation emptyChanges =
+ testExistingConnectionInvocation(new
CatalogUpdatesRequest(Collections.emptyList()));
+ Response emptyChangesResponse =
+ invokeTestExistingConnection(authorizer, mock(EventBus.class),
emptyChanges);
+ assertEquals(Response.Status.OK.getStatusCode(),
emptyChangesResponse.getStatus());
+ verify(emptyChanges).proceed();
+ }
+
+ @Test
+ public void
testExistingCatalogConnectionWithProposedChangesDeniesUseCatalog() throws
Throwable {
+ EventBus eventBus = spy(new EventBus(Collections.emptyList()));
+ MethodInvocation invocation =
testExistingConnectionInvocation(proposedCatalogChanges());
+
+ Response response =
+ invokeTestExistingConnection(
+ catalogConnectionAuthorizer(true, false), eventBus, invocation);
+
+ assertEquals(Response.Status.FORBIDDEN.getStatusCode(),
response.getStatus());
+ verify(invocation, never()).proceed();
+ // The denial event reports the owner expression the executor evaluated,
not the default one.
+ ArgumentCaptor<AuthorizationDenialFailureEvent> captor =
+ ArgumentCaptor.forClass(AuthorizationDenialFailureEvent.class);
+ verify(eventBus).dispatchEvent(captor.capture());
+ assertEquals(
+ TEST_CATALOG_CONNECTION_WITH_CHANGES_AUTHORIZATION_EXPRESSION,
+ captor.getValue().expression());
+ }
+
+ @Test
+ public void
testExistingCatalogConnectionWithProposedChangesAllowsCatalogOwner()
+ throws Throwable {
+ MethodInvocation invocation =
testExistingConnectionInvocation(proposedCatalogChanges());
+
+ Response response =
+ invokeTestExistingConnection(
+ catalogConnectionAuthorizer(false, true), mock(EventBus.class),
invocation);
+
+ assertEquals(Response.Status.OK.getStatusCode(), response.getStatus());
+ verify(invocation).proceed();
+ }
+
public static class TestMetadataObjectTagAssociationOperations {
@AuthorizationExpression(expression = CAN_ACCESS_METADATA_AND_TAG)
@@ -1017,6 +1074,83 @@ public class TestGravitinoInterceptionService {
return authorizer;
}
+ private Response invokeTestExistingConnection(
+ GravitinoAuthorizer authorizer, EventBus eventBus, MethodInvocation
invocation)
+ throws Throwable {
+ try (MockedStatic<PrincipalUtils> principalUtilsMocked =
mockStatic(PrincipalUtils.class);
+ MockedStatic<GravitinoAuthorizerProvider> authorizerMocked =
+ mockStatic(GravitinoAuthorizerProvider.class);
+ MockedStatic<AuthorizationUtils> authUtilsMocked =
mockStatic(AuthorizationUtils.class);
+ MockedStatic<GravitinoEnv> envMocked = mockStatic(GravitinoEnv.class))
{
+ principalUtilsMocked
+ .when(PrincipalUtils::getCurrentPrincipal)
+ .thenReturn(new UserPrincipal("tester"));
+
principalUtilsMocked.when(PrincipalUtils::getCurrentUserName).thenReturn("tester");
+
+ authUtilsMocked
+ .when(
+ () ->
+ AuthorizationUtils.checkCurrentUser(
+ ArgumentMatchers.any(), ArgumentMatchers.any(),
ArgumentMatchers.any()))
+ .thenAnswer(ignored -> null);
+
+ GravitinoAuthorizerProvider mockedProvider =
mock(GravitinoAuthorizerProvider.class);
+
authorizerMocked.when(GravitinoAuthorizerProvider::getInstance).thenReturn(mockedProvider);
+ when(mockedProvider.getGravitinoAuthorizer()).thenReturn(authorizer);
+
+ GravitinoEnv mockEnv = mock(GravitinoEnv.class);
+ envMocked.when(GravitinoEnv::getInstance).thenReturn(mockEnv);
+ when(mockEnv.eventBus()).thenReturn(eventBus);
+
+ // Use the real resource method so its annotations drive the
authorization executor.
+ MethodInterceptor interceptor =
+ new
GravitinoInterceptionService().getMethodInterceptors(invocation.getMethod()).get(0);
+ return (Response) interceptor.invoke(invocation);
+ }
+ }
+
+ private MethodInvocation
testExistingConnectionInvocation(CatalogUpdatesRequest request)
+ throws Throwable {
+ Method method =
+ CatalogOperations.class.getMethod(
+ "testExistingConnection", String.class, String.class,
CatalogUpdatesRequest.class);
+ MethodInvocation invocation = mock(MethodInvocation.class);
+ when(invocation.getMethod()).thenReturn(method);
+ when(invocation.getArguments())
+ .thenReturn(new Object[] {"testMetalake", "testCatalog", request});
+ when(invocation.proceed()).thenReturn(Utils.ok("ok"));
+ return invocation;
+ }
+
+ private CatalogUpdatesRequest proposedCatalogChanges() {
+ return new CatalogUpdatesRequest(
+ ImmutableList.of(new
CatalogUpdateRequest.SetCatalogPropertyRequest("key", "value")));
+ }
+
+ private GravitinoAuthorizer catalogConnectionAuthorizer(boolean useCatalog,
boolean owner) {
+ GravitinoAuthorizer authorizer = mock(GravitinoAuthorizer.class);
+ when(authorizer.authorize(
+ ArgumentMatchers.any(),
+ ArgumentMatchers.eq("testMetalake"),
+ ArgumentMatchers.argThat(
+ metadataObject ->
+ metadataObject.type() == MetadataObject.Type.CATALOG
+ && "testCatalog".equals(metadataObject.name())),
+ ArgumentMatchers.eq(Privilege.Name.USE_CATALOG),
+ ArgumentMatchers.any()))
+ .thenReturn(useCatalog);
+ when(authorizer.isOwner(
+ ArgumentMatchers.any(),
+ ArgumentMatchers.eq("testMetalake"),
+ ArgumentMatchers.argThat(
+ metadataObject ->
+ metadataObject.type() == MetadataObject.Type.CATALOG
+ && "testCatalog".equals(metadataObject.name())),
+ ArgumentMatchers.any()))
+ .thenReturn(owner);
+ return authorizer;
+ }
+
private static class MockGravitinoAuthorizer implements GravitinoAuthorizer {
@Override
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/filter/authorization/TestCatalogConnectionTestAuthorizationExecutor.java
b/server/src/test/java/org/apache/gravitino/server/web/filter/authorization/TestCatalogConnectionTestAuthorizationExecutor.java
new file mode 100644
index 0000000000..26cd29987b
--- /dev/null
+++
b/server/src/test/java/org/apache/gravitino/server/web/filter/authorization/TestCatalogConnectionTestAuthorizationExecutor.java
@@ -0,0 +1,132 @@
+/*
+ * 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.server.web.filter.authorization;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import com.google.common.collect.ImmutableList;
+import java.lang.reflect.Method;
+import java.util.Collections;
+import java.util.Optional;
+import org.apache.gravitino.Entity;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.dto.requests.CatalogUpdateRequest;
+import org.apache.gravitino.dto.requests.CatalogUpdatesRequest;
+import
org.apache.gravitino.server.authorization.annotations.AuthorizationRequest;
+import
org.apache.gravitino.server.authorization.annotations.ExpressionCondition;
+import
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants;
+import org.junit.jupiter.api.Test;
+
+public class TestCatalogConnectionTestAuthorizationExecutor {
+ private static final String PRIMARY_EXPRESSION =
+ AuthorizationExpressionConstants.LOAD_CATALOG_AUTHORIZATION_EXPRESSION;
+ private static final String SECONDARY_EXPRESSION =
+ AuthorizationExpressionConstants
+ .TEST_CATALOG_CONNECTION_WITH_CHANGES_AUTHORIZATION_EXPRESSION;
+
+ @Test
+ public void testUsesPrimaryExpressionWithoutRequestBody() throws Exception {
+ CatalogConnectionTestAuthorizationExecutor executor =
+ createExecutor(null, ExpressionCondition.HAS_PROPOSED_CHANGES);
+
+ assertEquals(PRIMARY_EXPRESSION, executor.expression);
+ }
+
+ @Test
+ public void testUsesPrimaryExpressionForEmptyChanges() throws Exception {
+ CatalogConnectionTestAuthorizationExecutor executor =
+ createExecutor(
+ new CatalogUpdatesRequest(Collections.emptyList()),
+ ExpressionCondition.HAS_PROPOSED_CHANGES);
+
+ assertEquals(PRIMARY_EXPRESSION, executor.expression);
+ }
+
+ @Test
+ public void testUsesSecondaryExpressionForProposedChanges() throws Exception
{
+ CatalogConnectionTestAuthorizationExecutor executor =
+ createExecutor(proposedChanges(),
ExpressionCondition.HAS_PROPOSED_CHANGES);
+
+ assertEquals(SECONDARY_EXPRESSION, executor.expression);
+ }
+
+ @Test
+ public void testUsesPrimaryExpressionWhenConditionNever() throws Exception {
+ CatalogConnectionTestAuthorizationExecutor executor =
+ createExecutor(proposedChanges(), ExpressionCondition.NEVER);
+
+ assertEquals(PRIMARY_EXPRESSION, executor.expression);
+ }
+
+ @Test
+ public void testUsesPrimaryExpressionForNullChanges() throws Exception {
+ CatalogConnectionTestAuthorizationExecutor executor =
+ createExecutor(new CatalogUpdatesRequest(),
ExpressionCondition.HAS_PROPOSED_CHANGES);
+
+ assertEquals(PRIMARY_EXPRESSION, executor.expression);
+ }
+
+ @Test
+ public void testFactoryCreatesExecutorForTestCatalogConnection() throws
Exception {
+ Method method = TestOperations.class.getMethod("testConnection",
CatalogUpdatesRequest.class);
+ AuthorizationExecutor executor =
+ AuthorizeExecutorFactory.create(
+ PRIMARY_EXPRESSION,
+ AuthorizationRequest.RequestType.TEST_CATALOG_CONNECTION,
+ Collections.emptyMap(),
+ Collections.emptyMap(),
+ Optional.empty(),
+ method.getParameters(),
+ new Object[] {proposedChanges()},
+ SECONDARY_EXPRESSION,
+ ExpressionCondition.HAS_PROPOSED_CHANGES,
+ "");
+
+ assertTrue(executor instanceof CatalogConnectionTestAuthorizationExecutor);
+ assertEquals(
+ SECONDARY_EXPRESSION, ((CatalogConnectionTestAuthorizationExecutor)
executor).expression);
+ }
+
+ private static CatalogUpdatesRequest proposedChanges() {
+ return new CatalogUpdatesRequest(
+ ImmutableList.of(new
CatalogUpdateRequest.SetCatalogPropertyRequest("key", "value")));
+ }
+
+ private static CatalogConnectionTestAuthorizationExecutor createExecutor(
+ CatalogUpdatesRequest request, ExpressionCondition condition) throws
Exception {
+ Method method = TestOperations.class.getMethod("testConnection",
CatalogUpdatesRequest.class);
+ return new CatalogConnectionTestAuthorizationExecutor(
+ method.getParameters(),
+ new Object[] {request},
+ PRIMARY_EXPRESSION,
+ Collections.<Entity.EntityType, NameIdentifier>emptyMap(),
+ Collections.emptyMap(),
+ Optional.empty(),
+ SECONDARY_EXPRESSION,
+ condition);
+ }
+
+ public static class TestOperations {
+ public void testConnection(
+ @AuthorizationRequest(type =
AuthorizationRequest.RequestType.TEST_CATALOG_CONNECTION)
+ CatalogUpdatesRequest request) {}
+ }
+}
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/rest/authorization/TestCatalogAuthorizationExpression.java
b/server/src/test/java/org/apache/gravitino/server/web/rest/authorization/TestCatalogAuthorizationExpression.java
index 124ce01e6a..3879f6693d 100644
---
a/server/src/test/java/org/apache/gravitino/server/web/rest/authorization/TestCatalogAuthorizationExpression.java
+++
b/server/src/test/java/org/apache/gravitino/server/web/rest/authorization/TestCatalogAuthorizationExpression.java
@@ -16,6 +16,7 @@
*/
package org.apache.gravitino.server.web.rest.authorization;
+import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -26,6 +27,7 @@ import ognl.OgnlException;
import org.apache.gravitino.dto.requests.CatalogCreateRequest;
import org.apache.gravitino.dto.requests.CatalogUpdatesRequest;
import
org.apache.gravitino.server.authorization.annotations.AuthorizationExpression;
+import
org.apache.gravitino.server.authorization.annotations.ExpressionCondition;
import
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants;
import org.apache.gravitino.server.web.rest.CatalogOperations;
import org.junit.jupiter.api.Test;
@@ -115,6 +117,50 @@ public class TestCatalogAuthorizationExpression {
assertFalse(mockEvaluator.getResult(ImmutableSet.of("CATALOG::USE_CATALOG")));
}
+ @Test
+ public void testTestExistingConnection() throws NoSuchMethodException,
OgnlException {
+ Method method =
+ CatalogOperations.class.getMethod(
+ "testExistingConnection", String.class, String.class,
CatalogUpdatesRequest.class);
+ AuthorizationExpression authorizationExpressionAnnotation =
+ method.getAnnotation(AuthorizationExpression.class);
+ String expression = authorizationExpressionAnnotation.expression();
+ MockAuthorizationExpressionEvaluator mockEvaluator =
+ new MockAuthorizationExpressionEvaluator(expression);
+ assertFalse(mockEvaluator.getResult(ImmutableSet.of()));
+
assertFalse(mockEvaluator.getResult(ImmutableSet.of("METALAKE::USE_SCHEMA")));
+
assertTrue(mockEvaluator.getResult(ImmutableSet.of("METALAKE::USE_CATALOG")));
+ assertTrue(mockEvaluator.getResult(ImmutableSet.of("METALAKE::OWNER")));
+ assertTrue(mockEvaluator.getResult(ImmutableSet.of("CATALOG::OWNER")));
+
assertFalse(mockEvaluator.getResult(ImmutableSet.of("METALAKE::CREATE_CATALOG")));
+
assertTrue(mockEvaluator.getResult(ImmutableSet.of("CATALOG::USE_CATALOG")));
+ assertFalse(
+ mockEvaluator.getResult(
+ ImmutableSet.of("METALAKE::USE_CATALOG",
"CATALOG::DENY_USE_CATALOG")));
+ }
+
+ @Test
+ public void testTestExistingConnectionWithChanges() throws
NoSuchMethodException, OgnlException {
+ Method method =
+ CatalogOperations.class.getMethod(
+ "testExistingConnection", String.class, String.class,
CatalogUpdatesRequest.class);
+ AuthorizationExpression authorizationExpressionAnnotation =
+ method.getAnnotation(AuthorizationExpression.class);
+ assertEquals(
+ ExpressionCondition.HAS_PROPOSED_CHANGES,
+ authorizationExpressionAnnotation.secondaryExpressionCondition());
+ MockAuthorizationExpressionEvaluator mockEvaluator =
+ new MockAuthorizationExpressionEvaluator(
+ authorizationExpressionAnnotation.secondaryExpression());
+ assertFalse(mockEvaluator.getResult(ImmutableSet.of()));
+
assertFalse(mockEvaluator.getResult(ImmutableSet.of("METALAKE::USE_SCHEMA")));
+
assertFalse(mockEvaluator.getResult(ImmutableSet.of("METALAKE::USE_CATALOG")));
+ assertTrue(mockEvaluator.getResult(ImmutableSet.of("METALAKE::OWNER")));
+ assertTrue(mockEvaluator.getResult(ImmutableSet.of("CATALOG::OWNER")));
+
assertFalse(mockEvaluator.getResult(ImmutableSet.of("METALAKE::CREATE_CATALOG")));
+
assertFalse(mockEvaluator.getResult(ImmutableSet.of("CATALOG::USE_CATALOG")));
+ }
+
@Test
public void testDropCatalog() throws NoSuchMethodException, OgnlException {
Method method =
