This is an automated email from the ASF dual-hosted git repository.
jerryshao pushed a commit to branch branch-1.3
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/branch-1.3 by this push:
new eb2cd9ce43 [#13360] fix(server): Avoid exposing missing metalakes in
authorization errors (#13361) (#13362)
eb2cd9ce43 is described below
commit eb2cd9ce43a5237b6cc4e331542020b2df8745d9
Author: Qi Yu <[email protected]>
AuthorDate: Mon Sep 21 16:47:30 2026 +0800
[#13360] fix(server): Avoid exposing missing metalakes in authorization
errors (#13361) (#13362)
### What changes were proposed in this pull request?
Backport of #13361 to branch-1.3.
- Return the same 403 status, error code, type, and neutral message when
a metalake is missing or the caller is not a member, including the
dynamic metalake path used by lineage.
- Use one membership message source so Iceberg and Lance REST callers no
longer receive the misleading advice to add a user to a metalake that
might not exist.
- Remove the earlier service-admin bypass and existence probe; service
admins retain the documented metalake permissions.
- Test both failure paths for path and dynamic metalakes, and the shared
membership message.
### Why are the changes needed?
The old error directed callers to add themselves to a metalake that
might not exist. Distinct responses for a missing metalake and an
inaccessible one would also reveal metalake names. A custom authorizer
may throw `NoSuchMetalakeException` directly; JCasbin reports both cases
as failed membership. Both paths now have the same client-visible
response.
This addresses the misleading error in #13360. Returning `dropped=false`
for a repeated delete requires a separate permission to probe metalake
existence and is outside this change.
### Does this PR introduce _any_ user-facing change?
Yes. Missing metalakes and failed membership checks return the same
neutral 403 response. The dynamic lineage path now returns 403 for a
missing metalake instead of 400. Iceberg and Lance REST callers also
receive the neutral membership message.
### How was this patch tested?
- `./gradlew :core:test --tests
org.apache.gravitino.authorization.TestAuthorizationUtils :server:test
--tests
org.apache.gravitino.server.web.filter.TestGravitinoInterceptionService
--tests
org.apache.gravitino.server.web.filter.authorization.TestLineageAuthorizationExecutor
-PskipITs -PskipDockerTests=true`
- `./gradlew :core:spotlessApply :server:spotlessApply`
---
.../authorization/AuthorizationUtils.java | 17 +++++--
.../authorization/TestAuthorizationUtils.java | 19 ++++++++
.../web/filter/GravitinoInterceptionService.java | 42 ++++++----------
.../filter/TestGravitinoInterceptionService.java | 57 +++++++++-------------
.../TestLineageAuthorizationExecutor.java | 48 +++++++-----------
5 files changed, 88 insertions(+), 95 deletions(-)
diff --git
a/core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java
b/core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java
index 0545baf3fa..ec1ea7fe7c 100644
---
a/core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java
+++
b/core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java
@@ -125,12 +125,23 @@ public class AuthorizationUtils {
String metalake, String user, AuthorizationRequestContext
requestContext) {
GravitinoAuthorizer authorizer =
GravitinoEnv.getInstance().gravitinoAuthorizer();
if (authorizer != null && !authorizer.isMetalakeUser(metalake,
requestContext)) {
- throw new ForbiddenException(
- "Current user %s doesn't exist in the metalake %s, you should add
the user to the metalake first",
- user, metalake);
+ throw new ForbiddenException("%s",
metalakeMembershipFailureMessage(metalake, user));
}
}
+ /**
+ * Returns a membership error that does not disclose whether the metalake
exists.
+ *
+ * @param metalake The metalake name.
+ * @param user The current user name.
+ * @return A neutral membership error message.
+ */
+ public static String metalakeMembershipFailureMessage(String metalake,
String user) {
+ return String.format(
+ "Current user %s is not a member of metalake %s, or the metalake does
not exist",
+ user, metalake);
+ }
+
public static NameIdentifier ofRole(String metalake, String role) {
return NameIdentifier.of(
metalake, Entity.SYSTEM_CATALOG_RESERVED_NAME,
Entity.ROLE_SCHEMA_NAME, role);
diff --git
a/core/src/test/java/org/apache/gravitino/authorization/TestAuthorizationUtils.java
b/core/src/test/java/org/apache/gravitino/authorization/TestAuthorizationUtils.java
index 0049a36ec2..ddaf6ae97f 100644
---
a/core/src/test/java/org/apache/gravitino/authorization/TestAuthorizationUtils.java
+++
b/core/src/test/java/org/apache/gravitino/authorization/TestAuthorizationUtils.java
@@ -40,6 +40,7 @@ import org.apache.gravitino.catalog.SchemaDispatcher;
import org.apache.gravitino.catalog.TableDispatcher;
import org.apache.gravitino.connector.BaseCatalog;
import org.apache.gravitino.connector.authorization.AuthorizationPlugin;
+import org.apache.gravitino.exceptions.ForbiddenException;
import org.apache.gravitino.exceptions.IllegalNameIdentifierException;
import org.apache.gravitino.exceptions.IllegalNamespaceException;
import org.apache.gravitino.meta.AuditInfo;
@@ -56,6 +57,24 @@ class TestAuthorizationUtils {
String metalake = "metalake";
+ @Test
+ void testCheckCurrentUserUsesNeutralMembershipMessage() {
+ try (MockedStatic<GravitinoEnv> envMock =
Mockito.mockStatic(GravitinoEnv.class)) {
+ GravitinoEnv env = Mockito.mock(GravitinoEnv.class);
+ GravitinoAuthorizer authorizer = Mockito.mock(GravitinoAuthorizer.class);
+ envMock.when(GravitinoEnv::getInstance).thenReturn(env);
+ Mockito.when(env.gravitinoAuthorizer()).thenReturn(authorizer);
+
+ ForbiddenException exception =
+ Assertions.assertThrows(
+ ForbiddenException.class,
+ () -> AuthorizationUtils.checkCurrentUser(metalake, "tester"));
+ Assertions.assertEquals(
+ "Current user tester is not a member of metalake metalake, or the
metalake does not exist",
+ exception.getMessage());
+ }
+ }
+
@Test
void testCreateNameIdentifier() {
NameIdentifier user = AuthorizationUtils.ofUser(metalake, "user");
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java
b/server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java
index a48e5e9d3e..215073d6e3 100644
---
a/server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java
+++
b/server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java
@@ -172,14 +172,7 @@ public class GravitinoInterceptionService implements
InterceptionService {
if (metalakeIdent != null) {
authorizationMetalake = Optional.of(metalakeIdent.name());
Optional<Response> validationFailure =
- validateCurrentUser(
- metalakeIdent,
- authorizationRequestContext,
- expressionAnnotation,
- metadataContext,
- method,
- expression,
- false);
+ validateCurrentUser(metalakeIdent,
authorizationRequestContext, method, expression);
if (validationFailure.isPresent()) {
return validationFailure.get();
}
@@ -224,11 +217,8 @@ public class GravitinoInterceptionService implements
InterceptionService {
validateCurrentUser(
NameIdentifier.of(dynamicMetalake.get()),
authorizationRequestContext,
- expressionAnnotation,
- metadataContext,
method,
- expression,
- true);
+ expression);
if (validationFailure.isPresent()) {
return validationFailure.get();
}
@@ -287,28 +277,19 @@ public class GravitinoInterceptionService implements
InterceptionService {
private Optional<Response> validateCurrentUser(
NameIdentifier metalakeIdent,
AuthorizationRequestContext authorizationRequestContext,
- AuthorizationExpression expressionAnnotation,
- Map<Entity.EntityType, NameIdentifier> metadataContext,
Method method,
- String expression,
- boolean dynamicMetalake) {
+ String expression) {
String currentUser = PrincipalUtils.getCurrentUserName();
try {
AuthorizationUtils.checkCurrentUser(
metalakeIdent.name(), currentUser, authorizationRequestContext);
} catch (NoSuchMetalakeException e) {
+ // A custom authorizer may report a missing metalake directly; JCasbin
reports it as
+ // non-membership instead.
LOG.warn("Metalake {} does not exist when validating user {}",
metalakeIdent, currentUser);
- if (dynamicMetalake) {
- return Optional.of(
- Utils.illegalArguments(
- String.format(
- "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()));
} catch (ForbiddenException ex) {
LOG.warn(
"User validation failed - User: {}, Metalake: {}, Reason: {}",
@@ -316,7 +297,7 @@ public class GravitinoInterceptionService implements
InterceptionService {
metalakeIdent.name(),
ex.getMessage());
dispatchAuthzDenialEvent(currentUser, metalakeIdent, method.getName(),
expression);
- return Optional.of(Utils.forbidden(ex.getMessage(), ex));
+ return Optional.of(metalakeMembershipFailure(currentUser,
metalakeIdent.name()));
} catch (Exception ex) {
LOG.error(
"Unexpected error during user validation - User: {}, Metalake: {}",
@@ -329,6 +310,11 @@ public class GravitinoInterceptionService implements
InterceptionService {
return Optional.empty();
}
+ private Response metalakeMembershipFailure(String user, String metalake) {
+ return Utils.forbidden(
+ AuthorizationUtils.metalakeMembershipFailureMessage(metalake, user),
null);
+ }
+
private Response buildNoAuthResponse(
AuthorizationExpression expressionAnnotation,
Map<Entity.EntityType, NameIdentifier> metadataContext,
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java
b/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java
index daa2521910..92d4617bfd 100644
---
a/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java
+++
b/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java
@@ -418,52 +418,43 @@ public class TestGravitinoInterceptionService {
}
@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"));
+ authorizationUtilsMocked
+ .when(() ->
AuthorizationUtils.metalakeMembershipFailureMessage("target", "tester"))
+ .thenCallRealMethod();
- 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];
+ MethodInvocation invocation = mock(MethodInvocation.class);
+ when(invocation.getMethod()).thenReturn(method);
+ when(invocation.getArguments()).thenReturn(new Object[] {"target"});
+ MethodInterceptor interceptor =
+ new
GravitinoInterceptionService().getMethodInterceptors(method).get(0);
- // Test with non-existent metalake
- when(methodInvocation.getMethod()).thenReturn(testMethod);
- when(methodInvocation.getArguments()).thenReturn(new Object[]
{"nonExistentMetalake"});
- Response response = (Response)
methodInterceptor.invoke(methodInvocation);
+ Response missingResponse = (Response) interceptor.invoke(invocation);
+ Response inaccessibleResponse = (Response)
interceptor.invoke(invocation);
- // Verify that a 403 Forbidden response is returned
- assertEquals(Response.Status.FORBIDDEN.getStatusCode(),
response.getStatus());
- ErrorResponse errorResponse = (ErrorResponse) response.getEntity();
+ assertEquals(Response.Status.FORBIDDEN.getStatusCode(),
missingResponse.getStatus());
+ assertEquals(missingResponse.getStatus(),
inaccessibleResponse.getStatus());
+ ErrorResponse missingError = (ErrorResponse) missingResponse.getEntity();
+ ErrorResponse inaccessibleError = (ErrorResponse)
inaccessibleResponse.getEntity();
+ assertEquals(missingError.getCode(), inaccessibleError.getCode());
+ assertEquals(missingError.getType(), inaccessibleError.getType());
assertEquals(
- "User 'tester' is not authorized to perform operation 'testMethod'
on "
- + "metadata 'nonExistentMetalake'",
- errorResponse.getMessage());
+ "Current user tester is not a member of metalake target, or the
metalake does not exist",
+ missingError.getMessage());
+ assertEquals(missingError.getMessage(), inaccessibleError.getMessage());
+ verify(invocation, never()).proceed();
}
}
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/filter/authorization/TestLineageAuthorizationExecutor.java
b/server/src/test/java/org/apache/gravitino/server/web/filter/authorization/TestLineageAuthorizationExecutor.java
index 2829baf2ca..518e09d6d1 100644
---
a/server/src/test/java/org/apache/gravitino/server/web/filter/authorization/TestLineageAuthorizationExecutor.java
+++
b/server/src/test/java/org/apache/gravitino/server/web/filter/authorization/TestLineageAuthorizationExecutor.java
@@ -110,7 +110,7 @@ class TestLineageAuthorizationExecutor {
}
@Test
- void testInterceptorRejectsUserOutsideDynamicMetalake() throws Throwable {
+ void testMissingAndInaccessibleDynamicMetalakeHaveSameResponse() throws
Throwable {
MethodInvocation invocation = invocation(event());
try (MockedStatic<PrincipalUtils> principalUtils =
mockStatic(PrincipalUtils.class);
@@ -123,38 +123,24 @@ class TestLineageAuthorizationExecutor {
() ->
AuthorizationUtils.checkCurrentUser(
eq(METALAKE), eq("tester"),
any(AuthorizationRequestContext.class)))
+ .thenThrow(new NoSuchMetalakeException("Metalake does not exist"))
.thenThrow(new ForbiddenException("User tester is not a member"));
-
- Response response = (Response) lineageInterceptor().invoke(invocation);
-
- Assertions.assertEquals(Response.Status.FORBIDDEN.getStatusCode(),
response.getStatus());
- verify(invocation, never()).proceed();
- }
- }
-
- @Test
- void testRejectNonexistentDynamicMetalakeAsBadRequest() throws Throwable {
- MethodInvocation invocation = invocation(event());
-
- try (MockedStatic<PrincipalUtils> principalUtils =
mockStatic(PrincipalUtils.class);
- MockedStatic<AuthorizationUtils> authorizationUtils =
- mockStatic(AuthorizationUtils.class)) {
-
principalUtils.when(PrincipalUtils::getCurrentPrincipal).thenReturn(principal());
-
principalUtils.when(PrincipalUtils::getCurrentUserName).thenReturn("tester");
authorizationUtils
- .when(
- () ->
- AuthorizationUtils.checkCurrentUser(
- eq(METALAKE), eq("tester"),
any(AuthorizationRequestContext.class)))
- .thenThrow(new NoSuchMetalakeException("Metalake does not exist"));
-
- Response response = (Response) lineageInterceptor().invoke(invocation);
-
- Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
response.getStatus());
- ErrorResponse errorResponse = (ErrorResponse) response.getEntity();
- Assertions.assertTrue(
- errorResponse.getMessage().contains("job.namespace"),
- "The response should identify the invalid field");
+ .when(() ->
AuthorizationUtils.metalakeMembershipFailureMessage("metalake", "tester"))
+ .thenCallRealMethod();
+
+ MethodInterceptor interceptor = lineageInterceptor();
+ Response missingResponse = (Response) interceptor.invoke(invocation);
+ Response inaccessibleResponse = (Response)
interceptor.invoke(invocation);
+
+ Assertions.assertEquals(
+ Response.Status.FORBIDDEN.getStatusCode(),
missingResponse.getStatus());
+ Assertions.assertEquals(missingResponse.getStatus(),
inaccessibleResponse.getStatus());
+ ErrorResponse missingError = (ErrorResponse) missingResponse.getEntity();
+ ErrorResponse inaccessibleError = (ErrorResponse)
inaccessibleResponse.getEntity();
+ Assertions.assertEquals(missingError.getCode(),
inaccessibleError.getCode());
+ Assertions.assertEquals(missingError.getType(),
inaccessibleError.getType());
+ Assertions.assertEquals(missingError.getMessage(),
inaccessibleError.getMessage());
verify(invocation, never()).proceed();
}
}
