Copilot commented on code in PR #9271:
URL: https://github.com/apache/gravitino/pull/9271#discussion_r2564381579
##########
server/src/main/java/org/apache/gravitino/server/web/rest/TagOperations.java:
##########
@@ -261,8 +263,11 @@ public Response deleteTag(
@Produces("application/vnd.gravitino.v1+json")
@Timed(name = "list-objects-for-tag." + MetricNames.HTTP_PROCESS_DURATION,
absolute = true)
@ResponseMetered(name = "list-objects-for-tag", absolute = true)
+ @AuthorizationExpression(expression = "", accessMetadataType =
MetadataObject.Type.TAG)
Review Comment:
In `listMetadataObjectsForTag()`, the `@AuthorizationExpression` specifies
`accessMetadataType = MetadataObject.Type.TAG`, but when metalake doesn't exist
and we reach line 158, the error message will try to extract the TAG identifier
from `metadataContext`. If the metalake doesn't exist, there may not be a valid
tag identifier in the context either.
The error message built at line 223-224 will try to get
`Entity.EntityType.TAG` from the metadata context. This could result in a
confusing error message like "User 'X' is not authorized to perform operation
'listMetadataObjectsForTag' on metadata 'testTag'" when the real issue is that
the metalake doesn't exist.
Consider using `MetadataObject.Type.METALAKE` as the `accessMetadataType`
for this method (similar to `listTags()`), so that the error message correctly
identifies that the metalake is the issue, not the tag.
```suggestion
@AuthorizationExpression(expression = "", accessMetadataType =
MetadataObject.Type.METALAKE)
```
##########
server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java:
##########
@@ -172,22 +146,57 @@ public Object invoke(MethodInvocation methodInvocation)
throws Throwable {
extractNameIdentifierFromParameters(parameters, args);
Map<String, Object> pathParams =
Utils.extractPathParamsFromParameters(parameters, args);
- AuthorizationExpressionEvaluator authorizationExpressionEvaluator =
- new AuthorizationExpressionEvaluator(expression);
- AuthorizationRequest.RequestType requestType =
- extractAuthorizationRequestTypeFromParameters(parameters);
- executor =
- AuthorizeExecutorFactory.create(
- requestType,
- metadataContext,
- authorizationExpressionEvaluator,
- pathParams,
- entityType,
- parameters,
- args);
- boolean authorizeResult = executor.execute();
- if (!authorizeResult) {
- return buildNoAuthResponse(expressionAnnotation, metadataContext,
method, expression);
+
+ // Check metalake and user existence before authorization
+ NameIdentifier metalakeIdent =
metadataContext.get(Entity.EntityType.METALAKE);
+ if (metalakeIdent != null) {
+ try {
+ MetalakeManager.checkMetalake(
+ metalakeIdent, GravitinoEnv.getInstance().entityStore());
+ } catch (NoSuchMetalakeException | MetalakeNotInUseException ex) {
+ // If metalake doesn't exist or is not in use, return no auth
response
+ return buildNoAuthResponse(expressionAnnotation,
metadataContext, method, expression);
+ }
+
+ String currentUser = PrincipalUtils.getCurrentUserName();
+ try {
+ AuthorizationUtils.checkCurrentUser(metalakeIdent.name(),
currentUser);
+ } catch (ForbiddenException ex) {
+ LOG.warn(
+ "User validation failed - User: {}, Metalake: {}, Reason:
{}",
+ currentUser,
+ metalakeIdent.name(),
+ ex.getMessage());
+ return Utils.forbidden(ex.getMessage(), ex);
+ } catch (Exception ex) {
+ LOG.error(
+ "Unexpected error during user validation - User: {},
Metalake: {}",
+ currentUser,
+ metalakeIdent.name(),
+ ex);
+ return Utils.internalError("Failed to validate user", ex);
+ }
+ }
+
+ // If expression is empty, skip authorization check (method handles
its own filtering)
+ if (StringUtils.isNotBlank(expression)) {
Review Comment:
The logic now checks for metalake existence and user validation even when
the expression is empty. This means that for `listTags()` and
`listMetadataObjectsForTag()` which use empty expressions, users will still be
validated and need to exist in the metalake. However, these methods internally
perform their own filtering (as seen with
`MetadataAuthzHelper.filterByExpression` in `listTags()`).
Consider documenting this behavior - methods with empty expressions still
require:
1. Metalake to exist and be in use
2. Current user to exist in the metalake
The method will only skip the authorization expression evaluation, not the
pre-checks.
##########
clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/TagOperationsAuthorizationIT.java:
##########
@@ -294,6 +296,82 @@ public void testDropTag() {
gravitinoMetalake.deleteTag("tag1");
}
+ @Test
+ @Order(9)
+ public void testTagOperationsWithNonExistentMetalake() throws Exception {
+ // Test that all tag operations with @AuthorizationExpression return 403
Forbidden
+ // when the metalake doesn't exist, instead of inconsistent 404 responses
+ String nonExistentMetalake = "nonExistentMetalake";
+
+ // Access the restClient from normalUserClient using reflection
+ Method restClientMethod =
+
normalUserClient.getClass().getSuperclass().getDeclaredMethod("restClient");
+ restClientMethod.setAccessible(true);
+ Object restClient = restClientMethod.invoke(normalUserClient);
+
+ // Create a MetalakeDTO for the non-existent metalake
+ MetalakeDTO metalakeDTO =
+ MetalakeDTO.builder()
+ .withName(nonExistentMetalake)
+ .withComment("test")
+ .withProperties(Maps.newHashMap())
+ .withAudit(
+ org.apache.gravitino.dto.AuditDTO.builder()
+ .withCreator("test")
+ .withCreateTime(java.time.Instant.now())
+ .build())
+ .build();
+
+ // Use DTOConverters.toMetaLake() via reflection to create
GravitinoMetalake
+ Class<?> dtoConvertersClass =
Class.forName("org.apache.gravitino.client.DTOConverters");
+ Method toMetaLakeMethod =
+ dtoConvertersClass.getDeclaredMethod(
+ "toMetaLake",
+ MetalakeDTO.class,
+ Class.forName("org.apache.gravitino.client.RESTClient"));
+ toMetaLakeMethod.setAccessible(true);
+ GravitinoMetalake nonExistentMetalakeObj =
+ (GravitinoMetalake) toMetaLakeMethod.invoke(null, metalakeDTO,
restClient);
Review Comment:
The test uses Java reflection to access private/internal APIs
(`restClient()` method and `DTOConverters.toMetaLake()`). While this works for
testing, it creates a brittle test that could break if these internal APIs
change.
Consider adding a public test helper method in the client library or test
utilities that allows creating a metalake client without server-side
validation, specifically for testing authorization scenarios. This would make
the test more maintainable and less dependent on implementation details.
##########
server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java:
##########
@@ -127,6 +146,113 @@ public void testSystemInternalErrorHandling() throws
Throwable {
}
}
+ @Test
+ public void testMetalakeNotExistOrNotInUse() throws Throwable {
+ try (MockedStatic<PrincipalUtils> principalUtilsMocked =
mockStatic(PrincipalUtils.class);
+ MockedStatic<GravitinoAuthorizerProvider> authorizerMocked =
+ mockStatic(GravitinoAuthorizerProvider.class);
+ MockedStatic<org.apache.gravitino.GravitinoEnv> envMocked =
+ mockStatic(org.apache.gravitino.GravitinoEnv.class);
+ MockedStatic<org.apache.gravitino.metalake.MetalakeManager>
metalakeManagerMocked =
+ mockStatic(org.apache.gravitino.metalake.MetalakeManager.class)) {
+
+ principalUtilsMocked
+ .when(PrincipalUtils::getCurrentPrincipal)
+ .thenReturn(new UserPrincipal("tester"));
+
principalUtilsMocked.when(PrincipalUtils::getCurrentUserName).thenReturn("tester");
+
+ MethodInvocation methodInvocation = mock(MethodInvocation.class);
+ GravitinoAuthorizerProvider mockedProvider =
mock(GravitinoAuthorizerProvider.class);
+
authorizerMocked.when(GravitinoAuthorizerProvider::getInstance).thenReturn(mockedProvider);
+ when(mockedProvider.getGravitinoAuthorizer()).thenReturn(new
MockGravitinoAuthorizer());
+
+ GravitinoEnv mockEnv = mock(GravitinoEnv.class);
+ EntityStore mockStore = mock(EntityStore.class);
+ envMocked.when(GravitinoEnv::getInstance).thenReturn(mockEnv);
+ when(mockEnv.entityStore()).thenReturn(mockStore);
+
+ // Mock MetalakeManager.checkMetalake to throw NoSuchMetalakeException
+ metalakeManagerMocked
+ .when(() -> MetalakeManager.checkMetalake(ArgumentMatchers.any(),
ArgumentMatchers.any()))
+ .thenThrow(new NoSuchMetalakeException("Metalake nonExistentMetalake
does not exist"));
+
+ GravitinoInterceptionService gravitinoInterceptionService =
+ new GravitinoInterceptionService();
+ Class<TestOperations> testOperationsClass = TestOperations.class;
+ Method[] methods = testOperationsClass.getMethods();
+ Method testMethod = methods[0];
+ List<MethodInterceptor> methodInterceptors =
+ gravitinoInterceptionService.getMethodInterceptors(testMethod);
+ MethodInterceptor methodInterceptor = methodInterceptors.get(0);
+
+ // Test with non-existent metalake
+ when(methodInvocation.getMethod()).thenReturn(testMethod);
+ when(methodInvocation.getArguments()).thenReturn(new Object[]
{"nonExistentMetalake"});
+ Response response = (Response)
methodInterceptor.invoke(methodInvocation);
+
+ // Verify that a 403 Forbidden response is returned
+ assertEquals(Response.Status.FORBIDDEN.getStatusCode(),
response.getStatus());
+ ErrorResponse errorResponse = (ErrorResponse) response.getEntity();
+ assertEquals(
+ "User 'tester' is not authorized to perform operation 'testMethod'
on "
+ + "metadata 'nonExistentMetalake'",
+ errorResponse.getMessage());
+ }
+ }
+
+ @Test
+ public void testEmptyExpressionSkipsAuthorization() throws Throwable {
+ try (MockedStatic<PrincipalUtils> principalUtilsMocked =
mockStatic(PrincipalUtils.class);
+ MockedStatic<GravitinoAuthorizerProvider> authorizerMocked =
+ mockStatic(GravitinoAuthorizerProvider.class);
+ MockedStatic<org.apache.gravitino.GravitinoEnv> envMocked =
+ mockStatic(org.apache.gravitino.GravitinoEnv.class);
+ MockedStatic<org.apache.gravitino.metalake.MetalakeManager>
metalakeManagerMocked =
+ mockStatic(org.apache.gravitino.metalake.MetalakeManager.class)) {
+
+ principalUtilsMocked
+ .when(PrincipalUtils::getCurrentPrincipal)
+ .thenReturn(new UserPrincipal("tester"));
+
principalUtilsMocked.when(PrincipalUtils::getCurrentUserName).thenReturn("tester");
+
+ GravitinoAuthorizerProvider mockedProvider =
mock(GravitinoAuthorizerProvider.class);
+
authorizerMocked.when(GravitinoAuthorizerProvider::getInstance).thenReturn(mockedProvider);
+ when(mockedProvider.getGravitinoAuthorizer()).thenReturn(new
MockGravitinoAuthorizer());
+
+ GravitinoEnv mockEnv = mock(GravitinoEnv.class);
+ EntityStore mockStore = mock(EntityStore.class);
+ envMocked.when(GravitinoEnv::getInstance).thenReturn(mockEnv);
+ when(mockEnv.entityStore()).thenReturn(mockStore);
+
+ // Mock MetalakeManager.checkMetalake to do nothing (metalake exists)
+ metalakeManagerMocked
+ .when(() -> MetalakeManager.checkMetalake(ArgumentMatchers.any(),
ArgumentMatchers.any()))
+ .thenAnswer(invocation -> null);
+
+ GravitinoInterceptionService gravitinoInterceptionService =
+ new GravitinoInterceptionService();
+ Class<TestOperationsWithEmptyExpression> testOperationsClass =
+ TestOperationsWithEmptyExpression.class;
+ Method[] methods = testOperationsClass.getMethods();
+ Method testMethod = methods[0];
+ List<MethodInterceptor> methodInterceptors =
+ gravitinoInterceptionService.getMethodInterceptors(testMethod);
+ MethodInterceptor methodInterceptor = methodInterceptors.get(0);
+
+ MethodInvocation methodInvocation = mock(MethodInvocation.class);
+ when(methodInvocation.getMethod()).thenReturn(testMethod);
+ when(methodInvocation.getArguments()).thenReturn(new Object[]
{"testMetalake"});
+ when(methodInvocation.proceed()).thenReturn(Utils.ok("success"));
+
+ // Test with empty expression - should skip authorization and proceed
+ Response response = (Response)
methodInterceptor.invoke(methodInvocation);
+
+ // Verify that the method was allowed to proceed without authorization
check
+ assertEquals(Response.Status.OK.getStatusCode(), response.getStatus());
+ assertEquals("success", response.getEntity());
+ }
+ }
Review Comment:
The test `testEmptyExpressionSkipsAuthorization()` mocks
`MetalakeManager.checkMetalake()` to do nothing (succeed), but doesn't verify
that the check is actually performed when using empty expressions.
Consider adding a test case that verifies `MetalakeManager.checkMetalake()`
is still called even with empty expressions, and that it correctly returns 403
when the metalake doesn't exist. This would complement the existing test and
ensure the metalake check happens before skipping authorization.
--
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]
