[#9239] fix(authz): Return 403 instead of 404 for list operations when metalake doesn't exist #9271

jerryshao · 2025-11-26T09:03:26Z

What changes were proposed in this pull request?

This PR fixes the inconsistent HTTP response codes for tag list operations when the metalake doesn't exist or is not in use. Previously, listTags() and listMetadataObjectsForTag() returned 404 (Not Found) while other tag operations like createTag(), getTag(), alterTag(), and deleteTag() returned 403 (Forbidden).

The fix adds @AuthorizationExpression annotations with empty expressions to the list tag methods, which triggers the authorization interceptor to check metalake existence and return 403 consistently across all tag operations.

The user, group, role, catalog have similar issues. This PR fixes them together.

Why are the changes needed?

Fix: #9239

When authorization is enabled, all tag operations should return consistent error responses when the metalake doesn't exist or is not in use. The current inconsistency confuses users and makes error handling more complex for client applications.

Does this PR introduce any user-facing change?

Yes. The HTTP response code for listTags() and listMetadataObjectsForTag() operations changes from 404 to 403 when the metalake doesn't exist or is not in use. This makes the behavior consistent with other tag operations.

How was this patch tested?

Added unit tests in TestGravitinoInterceptionService:
- testMetalakeNotExistOrNotInUse(): Verifies 403 response when metalake doesn't exist
- testEmptyExpressionSkipsAuthorization(): Verifies empty expressions work correctly
Updated integration test TagOperationsAuthorizationIT.testTagOperationsWithNonExistentMetalake() to verify all tag operations return 403 consistently
All tests pass successfully

Copilot

Pull request overview

This PR addresses inconsistent HTTP response codes for tag list operations when a metalake doesn't exist or is not in use. Previously, listTags() and listMetadataObjectsForTag() returned 404 (Not Found), while other tag operations returned 403 (Forbidden). The fix adds @AuthorizationExpression annotations with empty expressions to trigger metalake existence checks before proceeding, ensuring consistent 403 responses across all tag operations.

Key Changes:

Modified authorization interceptor to check metalake existence and user validation before authorization expression evaluation
Added empty @AuthorizationExpression annotations to listTags() and listMetadataObjectsForTag() methods
Enhanced test coverage with unit tests for metalake non-existence and empty expression scenarios

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
`server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java`	Refactored authorization flow to check metalake existence before authorization, and skip authorization expression evaluation for empty expressions while still performing metalake and user validation
`server/src/main/java/org/apache/gravitino/server/web/rest/TagOperations.java`	Added `@AuthorizationExpression` with empty expressions to `listTags()` and `listMetadataObjectsForTag()` methods to trigger metalake existence checks
`server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java`	Added unit tests verifying 403 responses for non-existent metalakes and proper handling of empty authorization expressions
`clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/TagOperationsAuthorizationIT.java`	Added integration test to verify all tag operations consistently return 403 when metalake doesn't exist

server/src/main/java/org/apache/gravitino/server/web/rest/TagOperations.java

Copilot · 2025-11-26T10:13:00Z

...r/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java

+  @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());
+    }
+  }


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.

Copilot · 2025-11-26T10:13:00Z

server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java

+          // If expression is empty, skip authorization check (method handles its own filtering)
+          if (StringUtils.isNotBlank(expression)) {


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:

Metalake to exist and be in use

Current user to exist in the metalake

The method will only skip the authorization expression evaluation, not the pre-checks.

Copilot · 2025-11-26T10:13:00Z

...org/apache/gravitino/client/integration/test/authorization/TagOperationsAuthorizationIT.java

+    // 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);


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.

It will break the current class, so using reflection is acceptable.

jerqi · 2025-11-27T02:18:13Z

Look good to me. Could u address the conflicts first?

Add @AuthorizationExpression annotation to list operations in Catalog, User, Group, and Role operations to ensure consistent 403 Forbidden response when metalake doesn't exist, instead of 404. Changes: - Add @AuthorizationExpression(expression = "") to listCatalogs(), listUsers(), listGroups(), and listRoles() methods - Add @AuthorizationMetadata annotation to metalake parameters - Add integration tests to verify 403 response for non-existent metalakes This ensures authorization checks happen before existence checks, preventing information leakage about metalake existence through different error codes.

jerryshao force-pushed the issue-9239 branch from 4c6c228 to c793284 Compare November 26, 2025 09:52

jerryshao requested review from Copilot, hdygxsj and jerqi November 26, 2025 10:02

Copilot started reviewing on behalf of jerryshao November 26, 2025 10:02 View session

Copilot finished reviewing on behalf of jerryshao November 26, 2025 10:06

Copilot AI reviewed Nov 26, 2025

View reviewed changes

jerryshao added 4 commits November 27, 2025 10:53

Fix the 404 issue

fa06004

Fix the 404 issue

e3751eb

Fix the 404 issue

63e6cee

Fix the 404 issue

51bc4d9

jerryshao changed the title ~~[#9239] fix(authz): Return 403 instead of 404 for list tag operations when metalake doesn't exist~~ [#9239] fix(authz): Return 403 instead of 404 for list operations when metalake doesn't exist Nov 27, 2025

jerryshao force-pushed the issue-9239 branch from b96658b to 30b93ec Compare November 27, 2025 03:30

jerqi mentioned this pull request Nov 27, 2025

[#8943] feat(authz): support policy access control #9252

Open

jerqi approved these changes Nov 27, 2025

View reviewed changes

jerqi merged commit 75f4bff into apache:main Nov 27, 2025
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[#9239] fix(authz): Return 403 instead of 404 for list operations when metalake doesn't exist #9271

[#9239] fix(authz): Return 403 instead of 404 for list operations when metalake doesn't exist #9271

Uh oh!

jerryshao commented Nov 26, 2025 •

edited by jerqi

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

jerryshao Nov 26, 2025

Uh oh!

jerqi commented Nov 27, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		// If expression is empty, skip authorization check (method handles its own filtering)
		if (StringUtils.isNotBlank(expression)) {

[#9239] fix(authz): Return 403 instead of 404 for list operations when metalake doesn't exist #9271

[#9239] fix(authz): Return 403 instead of 404 for list operations when metalake doesn't exist #9271

Uh oh!

Conversation

jerryshao commented Nov 26, 2025 • edited by jerqi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

jerryshao Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

jerqi commented Nov 27, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jerryshao commented Nov 26, 2025 •

edited by jerqi

Loading