jerryshao commented on code in PR #13361:
URL: https://github.com/apache/gravitino/pull/13361#discussion_r4059169045
##########
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:
[Nit] Only the `dynamicMetalake = false` path is covered here.
The two stubs replay `NoSuchMetalakeException` then `ForbiddenException`
through `TestOperations.testMethod`, which has an
`@AuthorizationMetadata(METALAKE)` parameter (line 1034-1036) and therefore
always takes the non-dynamic branch. The dynamic branch is the one where the
two responses still differ (see the comment on
GravitinoInterceptionService.java:307-309), so it is the one most worth pinning
once that is settled.
A third case using an operation without a METALAKE-annotated parameter — so
`metalakeIdent` is null and `getAuthorizationMetalake()` supplies the name —
would assert whatever behavior you decide on there, and would fail if someone
later re-introduces a distinguishing response on either path.
Verified by: read the test at lines 493-528 and `TestOperations` at lines
1029-1038; read the two call sites of `validateCurrentUserAndActiveRoles` at
GravitinoInterceptionService.java:180-187 and 224-235 to confirm which one this
test reaches.
##########
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:
[Question] Is the Iceberg/Lance REST surface meant to be in scope? It still
returns the old message this issue is about.
`BaseMetadataAuthorizationMethodInterceptor` runs the identical membership
gate but was not touched: it calls the same
`AuthorizationUtils.checkCurrentUser` and rethrows the `ForbiddenException`
unchanged
(server-common/.../BaseMetadataAuthorizationMethodInterceptor.java:243-252), so
callers there still get `Current user %s doesn't exist in the metalake %s, you
should add the user to the metalake first` (AuthorizationUtils.java:131-133) —
the wording #13360 calls out as telling callers to add a user to a metalake
that may not exist. It is used by
`IcebergMetadataAuthorizationMethodInterceptor` and
`LanceMetadataAuthorizationMethodInterceptor`, both on live REST paths.
There is no existence oracle on that path under JCasbin (`isMetalakeUser`
returns false for a missing metalake, so both cases land on the same
`ForbiddenException`), so this is not the same severity as the lineage gap —
but the misleading message survives, and
`TrinoIcebergRESTAuthorizationIT.java:44` notes Trino surfaces that message
verbatim to end users. If the intent is to fix this at the source, moving the
neutral wording into `AuthorizationUtils.checkCurrentUser` would cover both
interceptors and make `metalakeMembershipFailure` unnecessary. If it is
deliberately out of scope, a line in the PR description saying so would help.
Verified by: read BaseMetadataAuthorizationMethodInterceptor.java:225-320;
`grep -rn "checkCurrentUser(" --include=*.java` outside tests returns exactly
two call sites, this file and GravitinoInterceptionService.java:296; `grep -rn
"extends BaseMetadataAuthorizationMethodInterceptor"` returns the Iceberg and
Lance interceptors.
##########
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:
[Nit] `getMethods()[0]` does not have a defined order.
`Class.getMethods()` returns public methods "in no particular order" per its
javadoc, and the array includes the inherited `Object` methods (`equals`,
`hashCode`, `toString`, `wait`, ...), not just `testMethod`. If the order ever
differs, `getMethodInterceptors(method)` is handed a method with no
`@AuthorizationExpression` and `.get(0)` throws `IndexOutOfBoundsException`
rather than failing with a useful message.
This was carried over from the old `testMetalakeNotExist`, so it is not a
regression — but the line is being rewritten anyway, and
`TestOperations.class.getMethod("testMethod", String.class)` is the same length
and deterministic. The file already uses that form elsewhere (e.g.
`tableLoadInterceptor()` at lines 1049-1054).
Verified by: read the rewritten test at lines 493-528, `TestOperations` at
lines 1029-1038, and the existing `getMethod(...)` helpers at lines 1049-1065.
##########
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:
[Important] The existence oracle this PR closes on path-metalake endpoints
is still open on `POST /api/lineage`.
Both catch branches now return the same neutral 403 — except when
`dynamicMetalake` is true. Lines 300-305, just above, still return a 400
`IllegalArguments` whose message is `job.namespace must identify an existing
metalake: <name>`, while a caller who is simply not a member of an existing
metalake falls through to the `ForbiddenException` branch and gets the neutral
403. That is exactly the two-different-responses probe the PR description says
it is removing: send `{"job": {"namespace": "<guess>"}}` and read 400 vs 403 to
learn whether `<guess>` exists.
This is reachable by any authenticated user in a default deployment:
- `LineageOperations.postLineage` carries
`@AuthorizationExpression(CAN_ACCESS_METADATA)` and has no
`@AuthorizationMetadata(METALAKE)` parameter, so `metalakeIdent` is null at
line 180 and the dynamic path at lines 224-232 is the one taken.
- The metalake name is caller-controlled:
`LineageAuthorizationExecutor.getAuthorizationMetalake()` returns
`event.getJob().getNamespace()` verbatim
(LineageAuthorizationExecutor.java:71-78).
- `LineageConfig.SOURCE_NAME` defaults to `http` (LineageConfig.java:56-61),
which resolves to `HTTPLineageSource`, whose REST package `GravitinoServer`
registers at startup (GravitinoServer.java:154) — so the endpoint is on by
default, not behind a flag.
Either route the `dynamicMetalake` case through `metalakeMembershipFailure`
as well, or, if the 400 is wanted for request-shape reasons, make it name-free
(`job.namespace does not identify a metalake you can access`) so it carries no
more information than the 403. Worth stating either way in the PR description,
since "the same error code, type, and neutral message for both cases" is not
currently true for every annotated endpoint.
Verified by: read `validateCurrentUserAndActiveRoles` at
GravitinoInterceptionService.java:288-330 and both of its call sites (lines
180-187, 224-235); read `LineageOperations.postLineage` and confirmed it has no
METALAKE-annotated parameter; read
`LineageAuthorizationExecutor.getAuthorizationMetalake` (:71-78); traced
`LineageConfig.sourceClass()` (:90-97) to `HTTPLineageSource` and its
registration via `LineageService.getRESTPackages()` (LineageService.java:80-85)
into `GravitinoServer.java:154`. `grep -rn "getAuthorizationMetalake"` shows
`LineageAuthorizationExecutor` is the only non-default implementation, so
lineage is the only endpoint affected.
--
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]
