Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 27, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fa06004
Fix the 404 issue
jerryshao Nov 26, 2025
e3751eb
Fix the 404 issue
jerryshao Nov 26, 2025
63e6cee
Fix the 404 issue
jerryshao Nov 26, 2025
51bc4d9
Fix the 404 issue
jerryshao Nov 26, 2025
30b93ec
Fix 404 issue for list operations with non-existent metalake
jerryshao Nov 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions ...va/org/apache/gravitino/client/integration/test/authorization/CatalogAuthorizationIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,11 @@
import static org.junit.jupiter.api.Assertions.assertEquals;

import com.google.common.collect.Maps;
import java.lang.reflect.Method;
import java.util.Map;
import org.apache.gravitino.Catalog;
import org.apache.gravitino.client.GravitinoMetalake;
import org.apache.gravitino.dto.MetalakeDTO;
import org.apache.gravitino.exceptions.ForbiddenException;
import org.apache.gravitino.integration.test.container.ContainerSuite;
import org.apache.gravitino.integration.test.container.HiveContainer;
Expand Down Expand Up @@ -146,4 +149,48 @@ public void testDeleteCatalog() {
catalogs = client.loadMetalake(METALAKE).listCatalogs();
assertEquals(0, catalogs.length);
}

@Test
@Order(4)
public void testListCatalogsWithNonExistentMetalake() throws Exception {
// Test that listCatalogs with @AuthorizationExpression returns 403 Forbidden
// when the metalake doesn't exist, instead of 404 response
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);

// Test listCatalogs - should return 403 ForbiddenException
assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listCatalogs);

// Test listCatalogsInfo - should return 403 ForbiddenException
assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listCatalogsInfo);
}
}
47 changes: 47 additions & 0 deletions ...java/org/apache/gravitino/client/integration/test/authorization/GroupAuthorizationIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,15 @@
import static org.junit.Assert.assertThrows;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.Maps;
import java.lang.reflect.Method;
import java.util.Collections;
import java.util.HashMap;
import org.apache.gravitino.MetadataObject;
import org.apache.gravitino.MetadataObjects;
import org.apache.gravitino.authorization.Privileges;
import org.apache.gravitino.client.GravitinoMetalake;
import org.apache.gravitino.dto.MetalakeDTO;
import org.apache.gravitino.exceptions.ForbiddenException;
import org.junit.jupiter.api.MethodOrderer;
import org.junit.jupiter.api.Order;
Expand Down Expand Up @@ -71,4 +74,48 @@ public void testRemoveGroup() {
gravitinoMetalake.grantRolesToUser(ImmutableList.of("role"), NORMAL_USER);
normalUserClient.loadMetalake(METALAKE).removeGroup("group2");
}

@Test
@Order(3)
public void testListGroupsWithNonExistentMetalake() throws Exception {
// Test that listGroups with @AuthorizationExpression returns 403 Forbidden
// when the metalake doesn't exist, instead of 404 response
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);

// Test listGroups - should return 403 ForbiddenException
assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listGroups);

// Test listGroupNames - should return 403 ForbiddenException
assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listGroupNames);
}
}
45 changes: 45 additions & 0 deletions .../java/org/apache/gravitino/client/integration/test/authorization/RoleAuthorizationIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,16 @@
import static org.junit.Assert.assertArrayEquals;
import static org.junit.Assert.assertThrows;

import com.google.common.collect.Maps;
import java.lang.reflect.Method;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import org.apache.gravitino.MetadataObject;
import org.apache.gravitino.MetadataObjects;
import org.apache.gravitino.authorization.Privileges;
import org.apache.gravitino.client.GravitinoMetalake;
import org.apache.gravitino.dto.MetalakeDTO;
import org.apache.gravitino.exceptions.ForbiddenException;
import org.junit.jupiter.api.MethodOrderer;
import org.junit.jupiter.api.Order;
Expand Down Expand Up @@ -151,4 +155,45 @@ public void testDeleteRole() {
.createRole("role2", new HashMap<>(), Collections.emptyList());
});
}

@Test
@Order(5)
public void testListRolesWithNonExistentMetalake() throws Exception {
// Test that listRoles with @AuthorizationExpression returns 403 Forbidden
// when the metalake doesn't exist, instead of 404 response
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);

// Test listRoleNames - should return 403 ForbiddenException
assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listRoleNames);
}
}
78 changes: 78 additions & 0 deletions .../apache/gravitino/client/integration/test/authorization/TagOperationsAuthorizationIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Maps;
import java.io.IOException;
import java.lang.reflect.Method;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
Expand All @@ -38,6 +39,7 @@
import org.apache.gravitino.authorization.SecurableObject;
import org.apache.gravitino.authorization.SecurableObjects;
import org.apache.gravitino.client.GravitinoMetalake;
import org.apache.gravitino.dto.MetalakeDTO;
import org.apache.gravitino.exceptions.ForbiddenException;
import org.apache.gravitino.integration.test.container.ContainerSuite;
import org.apache.gravitino.integration.test.container.HiveContainer;
Expand Down Expand Up @@ -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);
Comment on lines +306 to +334
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@jerryshao jerryshao Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Test createTag - should return 403 ForbiddenException
assertThrows(
ForbiddenException.class,
() -> nonExistentMetalakeObj.createTag("testTag", "comment", Maps.newHashMap()));

// Test getTag - should return 403 ForbiddenException
assertThrows(ForbiddenException.class, () -> nonExistentMetalakeObj.getTag("testTag"));

// Test alterTag - should return 403 ForbiddenException
assertThrows(
ForbiddenException.class,
() -> nonExistentMetalakeObj.alterTag("testTag", TagChange.rename("newName")));

// Test deleteTag - should return 403 ForbiddenException
assertThrows(
ForbiddenException.class,
() -> {
nonExistentMetalakeObj.deleteTag("testTag");
});

// Test listTags - now has @AuthorizationExpression with empty expression,
// should return 403 ForbiddenException
assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listTags);

// Test listTagsInfo - now has @AuthorizationExpression with empty expression,
// should return 403 ForbiddenException
assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listTagsInfo);

// Test associateTags for metadata object - should return 403 ForbiddenException
// when trying to associate tags to a catalog in a non-existent metalake
assertThrows(
ForbiddenException.class,
() ->
nonExistentMetalakeObj
.loadCatalog(CATALOG)
.supportsTags()
.associateTags(new String[] {"testTag"}, null));
}

private Column[] createColumns() {
return new Column[] {Column.of("col1", Types.StringType.get())};
}
Expand Down
47 changes: 47 additions & 0 deletions .../java/org/apache/gravitino/client/integration/test/authorization/UserAuthorizationIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
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.Arrays;
import java.util.Collections;
import java.util.Comparator;
Expand All @@ -31,6 +33,7 @@
import org.apache.gravitino.authorization.User;
import org.apache.gravitino.client.GravitinoAdminClient;
import org.apache.gravitino.client.GravitinoMetalake;
import org.apache.gravitino.dto.MetalakeDTO;
import org.apache.gravitino.exceptions.ForbiddenException;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.MethodOrderer;
Expand Down Expand Up @@ -126,6 +129,50 @@ public void testRemoveUser() {
assertUserEquals(new String[] {USER, NORMAL_USER, "user1"}, users);
}

@Test
@Order(4)
public void testListUsersWithNonExistentMetalake() throws Exception {
// Test that listUsers with @AuthorizationExpression returns 403 Forbidden
// when the metalake doesn't exist, instead of 404 response
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);

// Test listUsers - should return 403 ForbiddenException
assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listUsers);

// Test listUserNames - should return 403 ForbiddenException
assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listUserNames);
}

private void assertUserEquals(String[] exceptUsers, User[] actualUsers) {
Arrays.sort(exceptUsers);
Arrays.sort(actualUsers, Comparator.comparing(User::name));
Expand Down
Loading