Skip to content

Commit 75f4bff

Browse files
jerryshaojerryshao
authored
[#9239] fix(authz): Return 403 instead of 404 for list operations when metalake doesn't exist (#9271)
### 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
1 parent ad41c37 commit 75f4bff

File tree

12 files changed

+465
-54
lines changed

12 files changed

+465
-54
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/TagOperationsAuthorizationIT.java‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import com.google.common.collect.ImmutableSet;
2626
import com.google.common.collect.Maps;
2727
import java.io.IOException;
28+
import java.lang.reflect.Method;
2829
import java.util.ArrayList;
2930
import java.util.Arrays;
3031
import java.util.HashMap;
@@ -38,6 +39,7 @@
3839
import org.apache.gravitino.authorization.SecurableObject;
3940
import org.apache.gravitino.authorization.SecurableObjects;
4041
import org.apache.gravitino.client.GravitinoMetalake;
42+
import org.apache.gravitino.dto.MetalakeDTO;
4143
import org.apache.gravitino.exceptions.ForbiddenException;
4244
import org.apache.gravitino.integration.test.container.ContainerSuite;
4345
import org.apache.gravitino.integration.test.container.HiveContainer;
@@ -294,6 +296,82 @@ public void testDropTag() {
294296
gravitinoMetalake.deleteTag("tag1");
295297
}
296298

299+
@Test
300+
@Order(9)
301+
public void testTagOperationsWithNonExistentMetalake() throws Exception {
302+
// Test that all tag operations with @AuthorizationExpression return 403 Forbidden
303+
// when the metalake doesn't exist, instead of inconsistent 404 responses
304+
String nonExistentMetalake = "nonExistentMetalake";
305+
306+
// Access the restClient from normalUserClient using reflection
307+
Method restClientMethod =
308+
normalUserClient.getClass().getSuperclass().getDeclaredMethod("restClient");
309+
restClientMethod.setAccessible(true);
310+
Object restClient = restClientMethod.invoke(normalUserClient);
311+
312+
// Create a MetalakeDTO for the non-existent metalake
313+
MetalakeDTO metalakeDTO =
314+
MetalakeDTO.builder()
315+
.withName(nonExistentMetalake)
316+
.withComment("test")
317+
.withProperties(Maps.newHashMap())
318+
.withAudit(
319+
org.apache.gravitino.dto.AuditDTO.builder()
320+
.withCreator("test")
321+
.withCreateTime(java.time.Instant.now())
322+
.build())
323+
.build();
324+
325+
// Use DTOConverters.toMetaLake() via reflection to create GravitinoMetalake
326+
Class<?> dtoConvertersClass = Class.forName("org.apache.gravitino.client.DTOConverters");
327+
Method toMetaLakeMethod =
328+
dtoConvertersClass.getDeclaredMethod(
329+
"toMetaLake",
330+
MetalakeDTO.class,
331+
Class.forName("org.apache.gravitino.client.RESTClient"));
332+
toMetaLakeMethod.setAccessible(true);
333+
GravitinoMetalake nonExistentMetalakeObj =
334+
(GravitinoMetalake) toMetaLakeMethod.invoke(null, metalakeDTO, restClient);
335+
336+
// Test createTag - should return 403 ForbiddenException
337+
assertThrows(
338+
ForbiddenException.class,
339+
() -> nonExistentMetalakeObj.createTag("testTag", "comment", Maps.newHashMap()));
340+
341+
// Test getTag - should return 403 ForbiddenException
342+
assertThrows(ForbiddenException.class, () -> nonExistentMetalakeObj.getTag("testTag"));
343+
344+
// Test alterTag - should return 403 ForbiddenException
345+
assertThrows(
346+
ForbiddenException.class,
347+
() -> nonExistentMetalakeObj.alterTag("testTag", TagChange.rename("newName")));
348+
349+
// Test deleteTag - should return 403 ForbiddenException
350+
assertThrows(
351+
ForbiddenException.class,
352+
() -> {
353+
nonExistentMetalakeObj.deleteTag("testTag");
354+
});
355+
356+
// Test listTags - now has @AuthorizationExpression with empty expression,
357+
// should return 403 ForbiddenException
358+
assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listTags);
359+
360+
// Test listTagsInfo - now has @AuthorizationExpression with empty expression,
361+
// should return 403 ForbiddenException
362+
assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listTagsInfo);
363+
364+
// Test associateTags for metadata object - should return 403 ForbiddenException
365+
// when trying to associate tags to a catalog in a non-existent metalake
366+
assertThrows(
367+
ForbiddenException.class,
368+
() ->
369+
nonExistentMetalakeObj
370+
.loadCatalog(CATALOG)
371+
.supportsTags()
372+
.associateTags(new String[] {"testTag"}, null));
373+
}
374+
297375
private Column[] createColumns() {
298376
return new Column[] {Column.of("col1", Types.StringType.get())};
299377
}

‎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));

0 commit comments

Comments
 (0)