yuqi1129 commented on code in PR #13361:
URL: https://github.com/apache/gravitino/pull/13361#discussion_r4059789975
##########
server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java:
##########
@@ -314,11 +325,26 @@ private Optional<Response>
validateCurrentUserAndActiveRoles(
"job.namespace must identify an existing metalake: %s",
metalakeIdent.name()),
e));
}
+ if (serviceAdminAllowedOnMissingMetalake(expressionAnnotation)) {
+ // Let the resource method report the missing metalake itself (404
or dropped=false),
+ // so its events and error shape match the pre-authorization
behavior.
+ return Optional.of(methodInvocation.proceed());
+ }
// Not a real authz denial — metalake is absent, not forbidden. Skip
event dispatch;
// HttpAuditFilter will emit a generic HttpRequestFailureEvent for
this 403.
return Optional.of(
buildNoAuthResponse(expressionAnnotation, metadataContext, method,
expression));
} catch (ForbiddenException ex) {
+ // JCasbin reports non-membership rather than a missing metalake, so
probe existence to
+ // tell the two apart. An existing metalake the service admin cannot
access stays 403.
+ if (serviceAdminAllowedOnMissingMetalake(expressionAnnotation)
+ &&
!GravitinoEnv.getInstance().metalakeDispatcher().metalakeExists(metalakeIdent))
{
+ LOG.warn(
+ "Metalake {} does not exist when validating service admin {}",
+ metalakeIdent,
+ currentUser);
+ return Optional.of(methodInvocation.proceed());
Review Comment:
Resolved by removing the service-admin bypass entirely. A failed membership
check now returns 403 before the resource method, so the check-then-act
mutation race is gone.
##########
server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java:
##########
@@ -314,11 +325,26 @@ private Optional<Response>
validateCurrentUserAndActiveRoles(
"job.namespace must identify an existing metalake: %s",
metalakeIdent.name()),
e));
}
+ if (serviceAdminAllowedOnMissingMetalake(expressionAnnotation)) {
+ // Let the resource method report the missing metalake itself (404
or dropped=false),
+ // so its events and error shape match the pre-authorization
behavior.
+ return Optional.of(methodInvocation.proceed());
+ }
// Not a real authz denial — metalake is absent, not forbidden. Skip
event dispatch;
// HttpAuditFilter will emit a generic HttpRequestFailureEvent for
this 403.
return Optional.of(
buildNoAuthResponse(expressionAnnotation, metadataContext, method,
expression));
} catch (ForbiddenException ex) {
+ // JCasbin reports non-membership rather than a missing metalake, so
probe existence to
+ // tell the two apart. An existing metalake the service admin cannot
access stays 403.
+ if (serviceAdminAllowedOnMissingMetalake(expressionAnnotation)
+ &&
!GravitinoEnv.getInstance().metalakeDispatcher().metalakeExists(metalakeIdent))
{
Review Comment:
The existence probe has been removed, so there is no dispatcher choice on
this path.
##########
server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java:
##########
@@ -314,11 +325,26 @@ private Optional<Response>
validateCurrentUserAndActiveRoles(
"job.namespace must identify an existing metalake: %s",
metalakeIdent.name()),
e));
}
+ if (serviceAdminAllowedOnMissingMetalake(expressionAnnotation)) {
+ // Let the resource method report the missing metalake itself (404
or dropped=false),
+ // so its events and error shape match the pre-authorization
behavior.
+ return Optional.of(methodInvocation.proceed());
Review Comment:
Agreed: JCasbin reports failed membership for a missing metalake. I kept the
NoSuchMetalakeException catch for custom authorizers and added a comment
explaining that case.
##########
server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java:
##########
@@ -540,6 +545,131 @@ public void testMetalakeNotExist() throws Throwable {
}
}
+ @Test
+ public void testServiceAdminProceedsOnMissingMetalake() throws Throwable {
+ try (MockedStatic<PrincipalUtils> principalUtils =
mockStatic(PrincipalUtils.class);
+ MockedStatic<GravitinoAuthorizerProvider> authorizerProvider =
+ mockStatic(GravitinoAuthorizerProvider.class);
+ MockedStatic<AuthorizationUtils> authorizationUtils =
+ mockStatic(AuthorizationUtils.class)) {
+
principalUtils.when(PrincipalUtils::getCurrentUserName).thenReturn("admin");
+ GravitinoAuthorizerProvider provider =
mock(GravitinoAuthorizerProvider.class);
+ GravitinoAuthorizer authorizer = mock(GravitinoAuthorizer.class);
+
authorizerProvider.when(GravitinoAuthorizerProvider::getInstance).thenReturn(provider);
+ when(provider.getGravitinoAuthorizer()).thenReturn(authorizer);
+ // PassThroughAuthorizer path: the membership check itself reports the
missing metalake.
+ authorizationUtils
+ .when(
+ () ->
+ AuthorizationUtils.checkCurrentUser(
+ ArgumentMatchers.eq("gone"),
+ ArgumentMatchers.eq("admin"),
+ any(AuthorizationRequestContext.class)))
+ .thenThrow(new NoSuchMetalakeException("Metalake gone does not
exist"));
+
+ GravitinoInterceptionService service = new
GravitinoInterceptionService();
+ Response resourceResult = Utils.ok(new DropResponse(false));
+
+ for (boolean serviceAdmin : new boolean[] {false, true}) {
+ when(authorizer.isServiceAdmin()).thenReturn(serviceAdmin);
+ for (Method method : missingMetalakeAwareMethods()) {
+ MethodInvocation invocation = mock(MethodInvocation.class);
+ when(invocation.getMethod()).thenReturn(method);
+
when(invocation.getArguments()).thenReturn(missingMetalakeArguments(method,
"gone"));
+ when(invocation.proceed()).thenReturn(resourceResult);
+ MethodInterceptor interceptor =
service.getMethodInterceptors(method).get(0);
+
+ Object result = interceptor.invoke(invocation);
+
+ if (serviceAdmin) {
+ // The resource method reports the missing metalake itself (404 or
dropped=false).
+ Assertions.assertSame(resourceResult, result, method.getName());
+ verify(invocation).proceed();
+ } else {
+ assertEquals(
+ Response.Status.FORBIDDEN.getStatusCode(),
+ ((Response) result).getStatus(),
+ method.getName());
+ verify(invocation, never()).proceed();
+ }
+ }
+ }
+ }
+ }
+
+ @Test
+ public void testServiceAdminDistinguishesMissingMetalakeFromNonMembership()
throws Throwable {
+ try (MockedStatic<PrincipalUtils> principalUtils =
mockStatic(PrincipalUtils.class);
+ MockedStatic<GravitinoAuthorizerProvider> authorizerProvider =
+ mockStatic(GravitinoAuthorizerProvider.class);
+ MockedStatic<AuthorizationUtils> authorizationUtils =
mockStatic(AuthorizationUtils.class);
+ MockedStatic<GravitinoEnv> envMock = mockStatic(GravitinoEnv.class)) {
+
principalUtils.when(PrincipalUtils::getCurrentUserName).thenReturn("admin");
+ GravitinoAuthorizerProvider provider =
mock(GravitinoAuthorizerProvider.class);
+ GravitinoAuthorizer authorizer = mock(GravitinoAuthorizer.class);
+
authorizerProvider.when(GravitinoAuthorizerProvider::getInstance).thenReturn(provider);
+ when(provider.getGravitinoAuthorizer()).thenReturn(authorizer);
+ when(authorizer.isServiceAdmin()).thenReturn(true);
+ // JCasbin path: non-membership is reported for missing and inaccessible
metalakes alike.
+ authorizationUtils
+ .when(
+ () ->
+ AuthorizationUtils.checkCurrentUser(
+ ArgumentMatchers.eq("metalake"),
+ ArgumentMatchers.eq("admin"),
+ any(AuthorizationRequestContext.class)))
+ .thenThrow(new ForbiddenException("User is not a member"));
+
+ GravitinoEnv env = mock(GravitinoEnv.class);
+ MetalakeDispatcher metalakeDispatcher = mock(MetalakeDispatcher.class);
+ envMock.when(GravitinoEnv::getInstance).thenReturn(env);
+ when(env.metalakeDispatcher()).thenReturn(metalakeDispatcher);
+ when(env.eventBus()).thenReturn(mock(EventBus.class));
+
+ Method method = MetalakeOperations.class.getMethod("loadMetalake",
String.class);
Review Comment:
The service-admin probe test and bypass were removed. The current tests
compare the missing and inaccessible 403 responses on both path and dynamic
metalake routes.
##########
server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java:
##########
@@ -314,11 +325,26 @@ private Optional<Response>
validateCurrentUserAndActiveRoles(
"job.namespace must identify an existing metalake: %s",
metalakeIdent.name()),
e));
}
+ if (serviceAdminAllowedOnMissingMetalake(expressionAnnotation)) {
+ // Let the resource method report the missing metalake itself (404
or dropped=false),
+ // so its events and error shape match the pre-authorization
behavior.
+ return Optional.of(methodInvocation.proceed());
+ }
// Not a real authz denial — metalake is absent, not forbidden. Skip
event dispatch;
// HttpAuditFilter will emit a generic HttpRequestFailureEvent for
this 403.
return Optional.of(
buildNoAuthResponse(expressionAnnotation, metadataContext, method,
expression));
} catch (ForbiddenException ex) {
+ // JCasbin reports non-membership rather than a missing metalake, so
probe existence to
+ // tell the two apart. An existing metalake the service admin cannot
access stays 403.
Review Comment:
The service-admin flag and bypass were removed, so the asymmetric condition
no longer exists.
##########
server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java:
##########
@@ -314,10 +304,9 @@ private Optional<Response>
validateCurrentUserAndActiveRoles(
"job.namespace must identify an existing metalake: %s",
metalakeIdent.name()),
e));
}
- // Not a real authz denial — metalake is absent, not forbidden. Skip
event dispatch;
- // HttpAuditFilter will emit a generic HttpRequestFailureEvent for
this 403.
- return Optional.of(
- buildNoAuthResponse(expressionAnnotation, metadataContext, method,
expression));
+ // A missing metalake is not an authorization denial, so skip the
denial event. Return the
+ // same client-visible response as a failed membership check to avoid
exposing existence.
+ return Optional.of(metalakeMembershipFailure(currentUser,
metalakeIdent.name()));
Review Comment:
Fixed. Dynamic metalake requests now use the same neutral 403 response for
missing and inaccessible metalakes. The lineage test compares status, code,
type, and message across both cases.
--
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]
