Skip to content

Commit 30b93ec

Browse files
jerryshaojerryshao
committed
Fix 404 issue for list operations with non-existent metalake
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.
1 parent 51bc4d9 commit 30b93ec

File tree

8 files changed

+199
-4
lines changed

8 files changed

+199
-4
lines changed

‎clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/CatalogAuthorizationIT.java‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,11 @@
2222
import static org.junit.jupiter.api.Assertions.assertEquals;
2323

2424
import com.google.common.collect.Maps;
25+
import java.lang.reflect.Method;
2526
import java.util.Map;
2627
import org.apache.gravitino.Catalog;
28+
import org.apache.gravitino.client.GravitinoMetalake;
29+
import org.apache.gravitino.dto.MetalakeDTO;
2730
import org.apache.gravitino.exceptions.ForbiddenException;
2831
import org.apache.gravitino.integration.test.container.ContainerSuite;
2932
import org.apache.gravitino.integration.test.container.HiveContainer;
@@ -146,4 +149,48 @@ public void testDeleteCatalog() {
146149
catalogs = client.loadMetalake(METALAKE).listCatalogs();
147150
assertEquals(0, catalogs.length);
148151
}
152+
153+
@Test
154+
@Order(4)
155+
public void testListCatalogsWithNonExistentMetalake() throws Exception {
156+
// Test that listCatalogs with @AuthorizationExpression returns 403 Forbidden
157+
// when the metalake doesn't exist, instead of 404 response
158+
String nonExistentMetalake = "nonExistentMetalake";
159+
160+
// Access the restClient from normalUserClient using reflection
161+
Method restClientMethod =
162+
normalUserClient.getClass().getSuperclass().getDeclaredMethod("restClient");
163+
restClientMethod.setAccessible(true);
164+
Object restClient = restClientMethod.invoke(normalUserClient);
165+
166+
// Create a MetalakeDTO for the non-existent metalake
167+
MetalakeDTO metalakeDTO =
168+
MetalakeDTO.builder()
169+
.withName(nonExistentMetalake)
170+
.withComment("test")
171+
.withProperties(Maps.newHashMap())
172+
.withAudit(
173+
org.apache.gravitino.dto.AuditDTO.builder()
174+
.withCreator("test")
175+
.withCreateTime(java.time.Instant.now())
176+
.build())
177+
.build();
178+
179+
// Use DTOConverters.toMetaLake() via reflection to create GravitinoMetalake
180+
Class<?> dtoConvertersClass = Class.forName("org.apache.gravitino.client.DTOConverters");
181+
Method toMetaLakeMethod =
182+
dtoConvertersClass.getDeclaredMethod(
183+
"toMetaLake",
184+
MetalakeDTO.class,
185+
Class.forName("org.apache.gravitino.client.RESTClient"));
186+
toMetaLakeMethod.setAccessible(true);
187+
GravitinoMetalake nonExistentMetalakeObj =
188+
(GravitinoMetalake) toMetaLakeMethod.invoke(null, metalakeDTO, restClient);
189+
190+
// Test listCatalogs - should return 403 ForbiddenException
191+
assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listCatalogs);
192+
193+
// Test listCatalogsInfo - should return 403 ForbiddenException
194+
assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listCatalogsInfo);
195+
}
149196
}

‎clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/GroupAuthorizationIT.java‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,15 @@
2020
import static org.junit.Assert.assertThrows;
2121

2222
import com.google.common.collect.ImmutableList;
23+
import com.google.common.collect.Maps;
24+
import java.lang.reflect.Method;
2325
import java.util.Collections;
2426
import java.util.HashMap;
2527
import org.apache.gravitino.MetadataObject;
2628
import org.apache.gravitino.MetadataObjects;
2729
import org.apache.gravitino.authorization.Privileges;
2830
import org.apache.gravitino.client.GravitinoMetalake;
31+
import org.apache.gravitino.dto.MetalakeDTO;
2932
import org.apache.gravitino.exceptions.ForbiddenException;
3033
import org.junit.jupiter.api.MethodOrderer;
3134
import org.junit.jupiter.api.Order;
@@ -71,4 +74,48 @@ public void testRemoveGroup() {
7174
gravitinoMetalake.grantRolesToUser(ImmutableList.of("role"), NORMAL_USER);
7275
normalUserClient.loadMetalake(METALAKE).removeGroup("group2");
7376
}
77+
78+
@Test
79+
@Order(3)
80+
public void testListGroupsWithNonExistentMetalake() throws Exception {
81+
// Test that listGroups with @AuthorizationExpression returns 403 Forbidden
82+
// when the metalake doesn't exist, instead of 404 response
83+
String nonExistentMetalake = "nonExistentMetalake";
84+
85+
// Access the restClient from normalUserClient using reflection
86+
Method restClientMethod =
87+
normalUserClient.getClass().getSuperclass().getDeclaredMethod("restClient");
88+
restClientMethod.setAccessible(true);
89+
Object restClient = restClientMethod.invoke(normalUserClient);
90+
91+
// Create a MetalakeDTO for the non-existent metalake
92+
MetalakeDTO metalakeDTO =
93+
MetalakeDTO.builder()
94+
.withName(nonExistentMetalake)
95+
.withComment("test")
96+
.withProperties(Maps.newHashMap())
97+
.withAudit(
98+
org.apache.gravitino.dto.AuditDTO.builder()
99+
.withCreator("test")
100+
.withCreateTime(java.time.Instant.now())
101+
.build())
102+
.build();
103+
104+
// Use DTOConverters.toMetaLake() via reflection to create GravitinoMetalake
105+
Class<?> dtoConvertersClass = Class.forName("org.apache.gravitino.client.DTOConverters");
106+
Method toMetaLakeMethod =
107+
dtoConvertersClass.getDeclaredMethod(
108+
"toMetaLake",
109+
MetalakeDTO.class,
110+
Class.forName("org.apache.gravitino.client.RESTClient"));
111+
toMetaLakeMethod.setAccessible(true);
112+
GravitinoMetalake nonExistentMetalakeObj =
113+
(GravitinoMetalake) toMetaLakeMethod.invoke(null, metalakeDTO, restClient);
114+
115+
// Test listGroups - should return 403 ForbiddenException
116+
assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listGroups);
117+
118+
// Test listGroupNames - should return 403 ForbiddenException
119+
assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listGroupNames);
120+
}
74121
}

‎clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/RoleAuthorizationIT.java‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,16 @@
2020
import static org.junit.Assert.assertArrayEquals;
2121
import static org.junit.Assert.assertThrows;
2222

23+
import com.google.common.collect.Maps;
24+
import java.lang.reflect.Method;
2325
import java.util.Arrays;
2426
import java.util.Collections;
2527
import java.util.HashMap;
2628
import org.apache.gravitino.MetadataObject;
2729
import org.apache.gravitino.MetadataObjects;
2830
import org.apache.gravitino.authorization.Privileges;
31+
import org.apache.gravitino.client.GravitinoMetalake;
32+
import org.apache.gravitino.dto.MetalakeDTO;
2933
import org.apache.gravitino.exceptions.ForbiddenException;
3034
import org.junit.jupiter.api.MethodOrderer;
3135
import org.junit.jupiter.api.Order;
@@ -151,4 +155,45 @@ public void testDeleteRole() {
151155
.createRole("role2", new HashMap<>(), Collections.emptyList());
152156
});
153157
}
158+
159+
@Test
160+
@Order(5)
161+
public void testListRolesWithNonExistentMetalake() throws Exception {
162+
// Test that listRoles with @AuthorizationExpression returns 403 Forbidden
163+
// when the metalake doesn't exist, instead of 404 response
164+
String nonExistentMetalake = "nonExistentMetalake";
165+
166+
// Access the restClient from normalUserClient using reflection
167+
Method restClientMethod =
168+
normalUserClient.getClass().getSuperclass().getDeclaredMethod("restClient");
169+
restClientMethod.setAccessible(true);
170+
Object restClient = restClientMethod.invoke(normalUserClient);
171+
172+
// Create a MetalakeDTO for the non-existent metalake
173+
MetalakeDTO metalakeDTO =
174+
MetalakeDTO.builder()
175+
.withName(nonExistentMetalake)
176+
.withComment("test")
177+
.withProperties(Maps.newHashMap())
178+
.withAudit(
179+
org.apache.gravitino.dto.AuditDTO.builder()
180+
.withCreator("test")
181+
.withCreateTime(java.time.Instant.now())
182+
.build())
183+
.build();
184+
185+
// Use DTOConverters.toMetaLake() via reflection to create GravitinoMetalake
186+
Class<?> dtoConvertersClass = Class.forName("org.apache.gravitino.client.DTOConverters");
187+
Method toMetaLakeMethod =
188+
dtoConvertersClass.getDeclaredMethod(
189+
"toMetaLake",
190+
MetalakeDTO.class,
191+
Class.forName("org.apache.gravitino.client.RESTClient"));
192+
toMetaLakeMethod.setAccessible(true);
193+
GravitinoMetalake nonExistentMetalakeObj =
194+
(GravitinoMetalake) toMetaLakeMethod.invoke(null, metalakeDTO, restClient);
195+
196+
// Test listRoleNames - should return 403 ForbiddenException
197+
assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listRoleNames);
198+
}
154199
}

‎clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/UserAuthorizationIT.java‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@
2121
import static org.junit.jupiter.api.Assertions.assertEquals;
2222

2323
import com.google.common.collect.ImmutableList;
24+
import com.google.common.collect.Maps;
25+
import java.lang.reflect.Method;
2426
import java.util.Arrays;
2527
import java.util.Collections;
2628
import java.util.Comparator;
@@ -31,6 +33,7 @@
3133
import org.apache.gravitino.authorization.User;
3234
import org.apache.gravitino.client.GravitinoAdminClient;
3335
import org.apache.gravitino.client.GravitinoMetalake;
36+
import org.apache.gravitino.dto.MetalakeDTO;
3437
import org.apache.gravitino.exceptions.ForbiddenException;
3538
import org.junit.jupiter.api.Assertions;
3639
import org.junit.jupiter.api.MethodOrderer;
@@ -126,6 +129,50 @@ public void testRemoveUser() {
126129
assertUserEquals(new String[] {USER, NORMAL_USER, "user1"}, users);
127130
}
128131

132+
@Test
133+
@Order(4)
134+
public void testListUsersWithNonExistentMetalake() throws Exception {
135+
// Test that listUsers with @AuthorizationExpression returns 403 Forbidden
136+
// when the metalake doesn't exist, instead of 404 response
137+
String nonExistentMetalake = "nonExistentMetalake";
138+
139+
// Access the restClient from normalUserClient using reflection
140+
Method restClientMethod =
141+
normalUserClient.getClass().getSuperclass().getDeclaredMethod("restClient");
142+
restClientMethod.setAccessible(true);
143+
Object restClient = restClientMethod.invoke(normalUserClient);
144+
145+
// Create a MetalakeDTO for the non-existent metalake
146+
MetalakeDTO metalakeDTO =
147+
MetalakeDTO.builder()
148+
.withName(nonExistentMetalake)
149+
.withComment("test")
150+
.withProperties(Maps.newHashMap())
151+
.withAudit(
152+
org.apache.gravitino.dto.AuditDTO.builder()
153+
.withCreator("test")
154+
.withCreateTime(java.time.Instant.now())
155+
.build())
156+
.build();
157+
158+
// Use DTOConverters.toMetaLake() via reflection to create GravitinoMetalake
159+
Class<?> dtoConvertersClass = Class.forName("org.apache.gravitino.client.DTOConverters");
160+
Method toMetaLakeMethod =
161+
dtoConvertersClass.getDeclaredMethod(
162+
"toMetaLake",
163+
MetalakeDTO.class,
164+
Class.forName("org.apache.gravitino.client.RESTClient"));
165+
toMetaLakeMethod.setAccessible(true);
166+
GravitinoMetalake nonExistentMetalakeObj =
167+
(GravitinoMetalake) toMetaLakeMethod.invoke(null, metalakeDTO, restClient);
168+
169+
// Test listUsers - should return 403 ForbiddenException
170+
assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listUsers);
171+
172+
// Test listUserNames - should return 403 ForbiddenException
173+
assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listUserNames);
174+
}
175+
129176
private void assertUserEquals(String[] exceptUsers, User[] actualUsers) {
130177
Arrays.sort(exceptUsers);
131178
Arrays.sort(actualUsers, Comparator.comparing(User::name));

‎server/src/main/java/org/apache/gravitino/server/web/rest/CatalogOperations.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,8 +84,10 @@ public CatalogOperations(CatalogDispatcher catalogDispatcher) {
8484
@Produces("application/vnd.gravitino.v1+json")
8585
@Timed(name = "list-catalog." + MetricNames.HTTP_PROCESS_DURATION, absolute = true)
8686
@ResponseMetered(name = "list-catalog", absolute = true)
87+
@AuthorizationExpression(expression = "")
8788
public Response listCatalogs(
88-
@PathParam("metalake") String metalake,
89+
@PathParam("metalake") @AuthorizationMetadata(type = Entity.EntityType.METALAKE)
90+
String metalake,
8991
@QueryParam("details") @DefaultValue("false") boolean verbose) {
9092
LOG.info(
9193
"Received list catalog {} request for metalake: {}, ",

‎server/src/main/java/org/apache/gravitino/server/web/rest/GroupOperations.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,8 +158,10 @@ public Response removeGroup(
158158
@Produces("application/vnd.gravitino.v1+json")
159159
@Timed(name = "list-group." + MetricNames.HTTP_PROCESS_DURATION, absolute = true)
160160
@ResponseMetered(name = "list-group", absolute = true)
161+
@AuthorizationExpression(expression = "")
161162
public Response listGroups(
162-
@PathParam("metalake") String metalake,
163+
@PathParam("metalake") @AuthorizationMetadata(type = Entity.EntityType.METALAKE)
164+
String metalake,
163165
@QueryParam("details") @DefaultValue("false") boolean verbose) {
164166
LOG.info("Received list groups request.");
165167
try {

‎server/src/main/java/org/apache/gravitino/server/web/rest/RoleOperations.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,10 @@ public RoleOperations() {
8282
@Produces("application/vnd.gravitino.v1+json")
8383
@Timed(name = "list-role." + MetricNames.HTTP_PROCESS_DURATION, absolute = true)
8484
@ResponseMetered(name = "list-role", absolute = true)
85-
public Response listRoles(@PathParam("metalake") String metalake) {
85+
@AuthorizationExpression(expression = "")
86+
public Response listRoles(
87+
@PathParam("metalake") @AuthorizationMetadata(type = Entity.EntityType.METALAKE)
88+
String metalake) {
8689
try {
8790
return Utils.doAs(
8891
httpRequest,

‎server/src/main/java/org/apache/gravitino/server/web/rest/UserOperations.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,8 +103,10 @@ public Response getUser(
103103
@Produces("application/vnd.gravitino.v1+json")
104104
@Timed(name = "list-user." + MetricNames.HTTP_PROCESS_DURATION, absolute = true)
105105
@ResponseMetered(name = "list-user", absolute = true)
106+
@AuthorizationExpression(expression = "")
106107
public Response listUsers(
107-
@PathParam("metalake") String metalake,
108+
@PathParam("metalake") @AuthorizationMetadata(type = Entity.EntityType.METALAKE)
109+
String metalake,
108110
@QueryParam("details") @DefaultValue("false") boolean verbose) {
109111
try {
110112
return Utils.doAs(

0 commit comments

Comments
 (0)