yuqi1129 commented on code in PR #13361:
URL: https://github.com/apache/gravitino/pull/13361#discussion_r4059790762
##########
server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java:
##########
@@ -361,6 +350,14 @@ private Optional<Response>
validateCurrentUserAndActiveRoles(
return Optional.empty();
}
+ private Response metalakeMembershipFailure(String user, String metalake) {
+ return Utils.forbidden(
+ String.format(
+ "Current user %s is not a member of metalake %s, or the metalake
does not exist",
+ user, metalake),
+ null);
Review Comment:
Fixed at the source: AuthorizationUtils.checkCurrentUser now uses the shared
neutral message, covering Iceberg and Lance REST as well as the server
interceptor. I added a core unit test and updated both PR descriptions.
##########
server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java:
##########
@@ -491,52 +491,40 @@ public void testDottedMetadataNameReturnsBadRequest()
throws Throwable {
}
@Test
- public void testMetalakeNotExist() throws Throwable {
+ public void testMissingAndInaccessibleMetalakeHaveSameResponse() throws
Throwable {
try (MockedStatic<PrincipalUtils> principalUtilsMocked =
mockStatic(PrincipalUtils.class);
- MockedStatic<GravitinoAuthorizerProvider> authorizerMocked =
- mockStatic(GravitinoAuthorizerProvider.class);
MockedStatic<AuthorizationUtils> authorizationUtilsMocked =
mockStatic(AuthorizationUtils.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());
-
- // Mock AuthorizationUtils.checkCurrentUser to throw
NoSuchMetalakeException
authorizationUtilsMocked
.when(
() ->
AuthorizationUtils.checkCurrentUser(
ArgumentMatchers.any(), ArgumentMatchers.any(),
ArgumentMatchers.any()))
- .thenThrow(new NoSuchMetalakeException("Metalake nonExistentMetalake
does not exist"));
+ .thenThrow(new NoSuchMetalakeException("Metalake target does not
exist"))
+ .thenThrow(new ForbiddenException("User is not a member of target"));
Review Comment:
Added a lineage interceptor test using an operation without a
METALAKE-annotated parameter. It compares both failure responses and verifies
that the operation does not proceed.
##########
server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java:
##########
@@ -491,52 +491,40 @@ public void testDottedMetadataNameReturnsBadRequest()
throws Throwable {
}
@Test
- public void testMetalakeNotExist() throws Throwable {
+ public void testMissingAndInaccessibleMetalakeHaveSameResponse() throws
Throwable {
try (MockedStatic<PrincipalUtils> principalUtilsMocked =
mockStatic(PrincipalUtils.class);
- MockedStatic<GravitinoAuthorizerProvider> authorizerMocked =
- mockStatic(GravitinoAuthorizerProvider.class);
MockedStatic<AuthorizationUtils> authorizationUtilsMocked =
mockStatic(AuthorizationUtils.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());
-
- // Mock AuthorizationUtils.checkCurrentUser to throw
NoSuchMetalakeException
authorizationUtilsMocked
.when(
() ->
AuthorizationUtils.checkCurrentUser(
ArgumentMatchers.any(), ArgumentMatchers.any(),
ArgumentMatchers.any()))
- .thenThrow(new NoSuchMetalakeException("Metalake nonExistentMetalake
does not exist"));
+ .thenThrow(new NoSuchMetalakeException("Metalake target does not
exist"))
+ .thenThrow(new ForbiddenException("User is not a member of target"));
- 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);
+ Method method = TestOperations.class.getMethods()[0];
Review Comment:
Fixed: the test now uses TestOperations.class.getMethod with the explicit
method name and parameter type.
--
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]
