jerryshao commented on code in PR #13361:
URL: https://github.com/apache/gravitino/pull/13361#discussion_r4059061404
##########
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:
[Nit] Prefer `internalMetalakeDispatcher()` for this existence probe.
`GravitinoEnv.metalakeDispatcher()` is the public chain
`MetalakeEventDispatcher -> MetalakeNormalizeDispatcher ->
MetalakeHookDispatcher -> MetalakeManager` (GravitinoEnv.java:853-861).
`internalMetalakeDispatcher()` exists for exactly this case and its javadoc
says so: "The internal dispatcher preserves normalization but skips hooks and
event emission" (GravitinoEnv.java:479-488). `MetadataObjectUtil.java:239`
already uses it for an existence check of the same kind.
No behavior difference today — `MetalakeEventDispatcher.metalakeExists`
(MetalakeEventDispatcher.java:108-111) and the hook dispatcher
(MetalakeHookDispatcher.java:128-131) both delegate without emitting anything,
so this call is currently event-free. The point is that it is event-free by
accident: if that override is ever dropped, `SupportsMetalakes.metalakeExists`
falls back to `loadMetalake`, and then every denied request on these endpoints
would emit a `LoadMetalakeEvent`/`LoadMetalakeFailureEvent` from inside the
authorization filter. Given that event fidelity is what this PR is about, using
the internal dispatcher makes that impossible rather than merely unlikely.
Verified by: followed the dispatcher chain from GravitinoEnv.java:853-861
through
MetalakeEventDispatcher/MetalakeNormalizeDispatcher/MetalakeHookDispatcher to
`SupportsMetalakes.metalakeExists` (SupportsMetalakes.java:61-68).
##########
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:
[Nit] This branch is missing the `dynamicMetalake` guard that the sibling
branch has.
In the `NoSuchMetalakeException` branch the `dynamicMetalake` case returns a
400 (`job.namespace must identify an existing metalake`) at lines 321-327,
before the service-admin bypass is considered. Here the bypass is applied
regardless of `dynamicMetalake`, so a service admin on a job-namespace request
would be sent into the resource method instead of getting that 400.
Dormant today: `allowServiceAdminOnMissingMetalake` is only set on
`MetalakeOperations` endpoints, which always take the non-dynamic path
(`dynamicMetalake = false`, line 192). But the asymmetry is easy to trip over
the first time the flag is put on an endpoint that resolves its metalake
dynamically. Adding `&& !dynamicMetalake` to the condition, or moving the guard
above both branches, keeps the two paths saying the same thing.
Verified by: read both catch branches at lines 318-347 and both call sites
of `validateCurrentUserAndActiveRoles` (lines 182-195, 233-246); grepped for
`allowServiceAdminOnMissingMetalake` and found it only on the four
`MetalakeOperations` methods.
##########
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:
[Important] This bypass is a check-then-act race that can execute a mutating
metalake operation with no authorization at all.
`proceed()` here returns straight out of `invoke()` (lines 192-195), so
`executor.execute(...)` and the `METALAKE::OWNER` evaluation never run. The
gate is the `metalakeExists` probe on line 341, but the resource method runs
afterwards: if the metalake is created between the probe and
`metalakeDispatcher.dropMetalake/alterMetalake/enableMetalake`, the operation
succeeds against a metalake the caller does not own.
That matters because service admin is not a superset of metalake owner in
this codebase: `createMetalake` requires only `SERVICE_ADMIN`
(MetalakeOperations.java:113-118), while
`setMetalake`/`alterMetalake`/`dropMetalake` require `METALAKE::OWNER`
(MetalakeOperations.java:180-182, 225-227, 267-269). So service admin B, who
owns nothing, can poll `DELETE /api/metalakes/foo` while admin A creates `foo`,
and drop A's metalake without the owner check. Before this PR that request was
a 403.
`loadMetalake` does not have this problem — losing the race there just
returns a metalake the caller could not otherwise read, which is the
existence-disclosure the flag deliberately grants.
Two ways to close it, either is fine:
- Scope the bypass so the mutating endpoints do not call `proceed()`: return
the canonical absent-metalake result directly (404 for set/alter,
`dropped=false` for drop). This gives up the "the resource method emits its own
events" property for those three, so it is a trade-off worth stating.
- Or keep `proceed()` and re-assert ownership if the entity turns out to
exist, so a lost race degrades to 403 rather than an unauthorized mutation.
Verified by: read `invoke()` at GravitinoInterceptionService.java:179-195
and 276-280 to confirm the early return skips expression evaluation entirely;
read all four annotations in MetalakeOperations.java:150-296; confirmed no
ownership check exists below the interceptor by reading
`MetalakeManager.dropMetalake`/`alterMetalake` and the
`MetalakeNormalizeDispatcher`/`MetalakeHookDispatcher` delegates.
##########
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:
[Question] Is this `NoSuchMetalakeException` branch reachable in any shipped
configuration?
The only authorizer whose `isMetalakeUser` can raise
`NoSuchMetalakeException` is `PassThroughAuthorizer`, via
`dispatcher.addUser(metalake, user)` (PassThroughAuthorizer.java:85-96). But
`PassThroughAuthorizer` is only installed when `gravitino.authorization.enable`
is false (GravitinoAuthorizerProvider.java:50-63), and in that case
`GravitinoInterceptionService` is never bound at all
(GravitinoServer.java:157-166). With authorization on, the authorizer comes
from `Configs.AUTHORIZATION_IMPL`, which defaults to `JcasbinAuthorizer`
(Configs.java:338-343), and its membership check is a plain SQL lookup that
returns empty rather than throwing (JcasbinAuthorizer.java:408-417 ->
:1015-1024). I found no non-test config setting `authorization.impl` to
`PassThroughAuthorizer`.
So unless a custom `authorization.impl` is expected to throw it, the live
path is the `ForbiddenException` one below, and this is dead defensive code.
Worth confirming, because the unit test comment "PassThroughAuthorizer path:
the membership check itself reports the missing metalake" reads as if this is
the disabled-authorization path — which cannot reach the interceptor. Happy to
be wrong if a custom authorizer is the intended trigger; a one-line comment
saying so would help the next reader.
Verified by: read PassThroughAuthorizer.java:85-96,
GravitinoAuthorizerProvider.java:46-67, GravitinoServer.java:157-166,
Configs.java:331-343, JcasbinAuthorizer.java:408-417 and :1015-1024; grepped
the tree for non-test references to `PassThroughAuthorizer` and found none.
##########
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:
[Nit] This test only drives `loadMetalake`, leaving the mutating endpoints
uncovered on the live path.
`testServiceAdminProceedsOnMissingMetalake` above loops over all four
methods, but that is the `NoSuchMetalakeException` branch. This test covers the
`ForbiddenException` + `metalakeExists` branch — the one that actually runs
under JCasbin — and pins only `loadMetalake`. `setMetalake`, `alterMetalake`
and `dropMetalake` are the three that reach a mutating dispatcher call after
`proceed()`, so they are the ones most worth asserting here.
`missingMetalakeAwareMethods()` and `missingMetalakeArguments()` already
exist, so this is a matter of wrapping the body in the same loop the other test
uses, with `metalakeExists` stubbed false then true per method.
Verified by: read both new tests (lines 548-672) and confirmed
`missingMetalakeAwareMethods()` returns all four methods while this test
hardcodes `MetalakeOperations.class.getMethod("loadMetalake", String.class)`.
--
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]
