This is an automated email from the ASF dual-hosted git repository.
roryqi pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new ab2645a799 [#12112] fix(authz): Align group load authorization (#12113)
ab2645a799 is described below
commit ab2645a799eff40dbaeb36b1381695fc8fd7a802
Author: roryqi <[email protected]>
AuthorDate: Mon Jul 27 15:49:50 2026 +0800
[#12112] fix(authz): Align group load authorization (#12113)
### What changes were proposed in this pull request?
- Add an explicit authorization expression for loading group details.
- Include metalake and group path parameters in the authorization
metadata context.
- Support GROUP::SELF in JCasbin authorization checks.
- Add regression tests for group load authorization metadata and
GROUP::SELF evaluation.
### Why are the changes needed?
The group load API should follow the same authorization interception
path as related user and group access-control APIs.
Fix: #12112
### Does this PR introduce _any_ user-facing change?
No public API change. Group detail loading now follows the expected
authorization behavior.
### How was this patch tested?
- `./gradlew :server:test --tests
org.apache.gravitino.server.web.rest.authorization.TestGroupAuthorizationExpression
--tests org.apache.gravitino.server.web.rest.TestGroupOperations`
- `./gradlew :server-common:test --tests
org.apache.gravitino.server.authorization.jcasbin.TestJcasbinAuthorizer`
- `git diff --check`
---
.../authorization/jcasbin/JcasbinAuthorizer.java | 19 ++++-
.../jcasbin/TestJcasbinAuthorizer.java | 40 +++++++++++
.../gravitino/server/web/rest/GroupOperations.java | 33 +++++++--
.../server/web/rest/TestGroupOperations.java | 80 ++++++++++++++++++++++
.../TestGroupAuthorizationExpression.java | 66 ++++++++++++++++++
5 files changed, 232 insertions(+), 6 deletions(-)
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java
b/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java
index 2535e8e336..66583526e1 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java
@@ -425,10 +425,27 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
NameIdentifier nameIdentifier,
AuthorizationRequestContext requestContext) {
String metalake = nameIdentifier.namespace().level(0);
- String currentUserName = PrincipalUtils.getCurrentUserName();
if (Entity.EntityType.USER == type) {
+ String currentUserName = PrincipalUtils.getCurrentUserName();
return Objects.equals(nameIdentifier.name(), currentUserName);
+ } else if (Entity.EntityType.GROUP == type) {
+ Principal currentPrincipal = PrincipalUtils.getCurrentPrincipal();
+ if (!(currentPrincipal instanceof UserPrincipal)) {
+ return false;
+ }
+
+ List<UserGroup> groups = ((UserPrincipal) currentPrincipal).getGroups();
+ if (groups.isEmpty()) {
+ return false;
+ }
+
+ boolean principalHasGroup =
+ groups.stream()
+ .map(UserGroup::getGroupName)
+ .anyMatch(groupName -> Objects.equals(groupName,
nameIdentifier.name()));
+ return principalHasGroup;
} else if (Entity.EntityType.ROLE == type) {
+ String currentUserName = PrincipalUtils.getCurrentUserName();
try {
Optional<Long> roleId =
MetadataIdConverter.getID(
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java
b/server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java
index 624c547908..177fda2d7b 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java
@@ -888,6 +888,46 @@ public class TestJcasbinAuthorizer {
restoreDefaultPrincipal();
}
+ @Test
+ public void testIsSelfGroupViaPrincipalGroup() throws Exception {
+ NameIdentifier groupIdent = NameIdentifierUtil.ofGroup(METALAKE,
GROUP_NAME);
+
+ setCurrentPrincipalWithGroup(GROUP_NAME);
+ Mockito.clearInvocations(groupMetaMapper);
+ assertTrue(
+ jcasbinAuthorizer.isSelf(
+ Entity.EntityType.GROUP, groupIdent, new
AuthorizationRequestContext()));
+ Mockito.verify(groupMetaMapper,
Mockito.never()).getGroupUpdatedAt(anyString(), anyString());
+
+ setCurrentPrincipalWithGroup("otherGroup");
+ assertFalse(
+ jcasbinAuthorizer.isSelf(
+ Entity.EntityType.GROUP, groupIdent, new
AuthorizationRequestContext()));
+ Mockito.verify(groupMetaMapper,
Mockito.never()).getGroupUpdatedAt(anyString(), anyString());
+
+ restoreDefaultPrincipal();
+ }
+
+ @Test
+ public void testIsSelfGroupDoesNotRequireCurrentUserName() throws Exception {
+ NameIdentifier groupIdent = NameIdentifierUtil.ofGroup(METALAKE,
GROUP_NAME);
+
+ UserPrincipal groupPrincipal = mock(UserPrincipal.class);
+ when(groupPrincipal.getGroups())
+ .thenReturn(ImmutableList.of(new UserGroup(Optional.empty(),
GROUP_NAME)));
+ when(groupPrincipal.getName())
+ .thenThrow(new AssertionError("GROUP self check should only use
principal groups"));
+
principalUtilsMockedStatic.when(PrincipalUtils::getCurrentPrincipal).thenReturn(groupPrincipal);
+
+ try {
+ assertTrue(
+ jcasbinAuthorizer.isSelf(
+ Entity.EntityType.GROUP, groupIdent, new
AuthorizationRequestContext()));
+ } finally {
+ restoreDefaultPrincipal();
+ }
+ }
+
@Test
public void testIsSelfRoleReusesCacheAcrossCalls() throws Exception {
// Acceptance criterion for #11088: repeated isSelf(ROLE) calls in the
same logical request
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/rest/GroupOperations.java
b/server/src/main/java/org/apache/gravitino/server/web/rest/GroupOperations.java
index 5cc083c7f7..ef44db299c 100644
---
a/server/src/main/java/org/apache/gravitino/server/web/rest/GroupOperations.java
+++
b/server/src/main/java/org/apache/gravitino/server/web/rest/GroupOperations.java
@@ -49,10 +49,12 @@ import org.apache.gravitino.dto.responses.RemoveResponse;
import org.apache.gravitino.dto.util.DTOConverters;
import org.apache.gravitino.metalake.MetalakeManager;
import org.apache.gravitino.metrics.MetricNames;
+import org.apache.gravitino.server.authorization.MetadataAuthzHelper;
import org.apache.gravitino.server.authorization.NameBindings;
import
org.apache.gravitino.server.authorization.annotations.AuthorizationExpression;
import
org.apache.gravitino.server.authorization.annotations.AuthorizationMetadata;
import org.apache.gravitino.server.web.Utils;
+import org.apache.gravitino.utils.NameIdentifierUtil;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -62,6 +64,9 @@ public class GroupOperations {
private static final Logger LOG =
LoggerFactory.getLogger(GroupOperations.class);
+ private static final String LOAD_GROUP_PRIVILEGE =
+ "METALAKE::OWNER || METALAKE::MANAGE_GROUPS || GROUP::SELF";
+
private final AccessControlDispatcher accessControlManager;
private final OwnerDispatcher ownerDispatcher;
@@ -80,8 +85,11 @@ public class GroupOperations {
@Produces("application/vnd.gravitino.v1+json")
@Timed(name = "get-group." + MetricNames.HTTP_PROCESS_DURATION, absolute =
true)
@ResponseMetered(name = "get-group", absolute = true)
+ @AuthorizationExpression(expression = LOAD_GROUP_PRIVILEGE)
public Response getGroup(
- @PathParam("metalake") String metalake, @PathParam("group") String
group) {
+ @PathParam("metalake") @AuthorizationMetadata(type =
Entity.EntityType.METALAKE)
+ String metalake,
+ @PathParam("group") @AuthorizationMetadata(type =
Entity.EntityType.GROUP) String group) {
try {
return Utils.doAs(
httpRequest,
@@ -179,11 +187,26 @@ public class GroupOperations {
() -> {
MetalakeManager.checkMetalakeInUse(metalake);
if (verbose) {
- return Utils.ok(
- new GroupListResponse(
-
DTOConverters.toDTOs(accessControlManager.listGroups(metalake))));
+ Group[] groups = accessControlManager.listGroups(metalake);
+ groups =
+ MetadataAuthzHelper.filterByExpression(
+ metalake,
+ LOAD_GROUP_PRIVILEGE,
+ Entity.EntityType.GROUP,
+ groups,
+ groupEntity -> NameIdentifierUtil.ofGroup(metalake,
groupEntity.name()));
+
+ return Utils.ok(new
GroupListResponse(DTOConverters.toDTOs(groups)));
} else {
- return Utils.ok(new
NameListResponse(accessControlManager.listGroupNames(metalake)));
+ String[] groups = accessControlManager.listGroupNames(metalake);
+ groups =
+ MetadataAuthzHelper.filterByExpression(
+ metalake,
+ LOAD_GROUP_PRIVILEGE,
+ Entity.EntityType.GROUP,
+ groups,
+ groupName -> NameIdentifierUtil.ofGroup(metalake,
groupName));
+ return Utils.ok(new NameListResponse(groups));
}
});
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestGroupOperations.java
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestGroupOperations.java
index 5fd1084b6c..d35faee879 100644
---
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestGroupOperations.java
+++
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestGroupOperations.java
@@ -38,6 +38,7 @@ import javax.ws.rs.core.Response;
import javax.ws.rs.core.Response.Status;
import org.apache.commons.lang3.reflect.FieldUtils;
import org.apache.gravitino.Config;
+import org.apache.gravitino.Entity.EntityType;
import org.apache.gravitino.EntityStore;
import org.apache.gravitino.GravitinoEnv;
import org.apache.gravitino.authorization.AccessControlManager;
@@ -62,12 +63,14 @@ import org.apache.gravitino.meta.AuditInfo;
import org.apache.gravitino.meta.BaseMetalake;
import org.apache.gravitino.meta.GroupEntity;
import org.apache.gravitino.rest.RESTUtils;
+import org.apache.gravitino.server.authorization.MetadataAuthzHelper;
import org.glassfish.hk2.utilities.binding.AbstractBinder;
import org.glassfish.jersey.server.ResourceConfig;
import org.glassfish.jersey.test.TestProperties;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
+import org.mockito.MockedStatic;
import org.mockito.Mockito;
public class TestGroupOperations extends BaseOperationsTest {
@@ -380,6 +383,43 @@ public class TestGroupOperations extends
BaseOperationsTest {
Assertions.assertEquals(RuntimeException.class.getSimpleName(),
errorResponse2.getType());
}
+ @Test
+ public void testListGroupNamesFiltersByAuthorizationExpression() throws
Exception {
+ Mockito.reset(manager, entityStore);
+ Mockito.doReturn(new String[] {"group",
"filtered"}).when(manager).listGroupNames(any());
+
+ // Mock metalake with in-use property
+ BaseMetalake metalake = mock(BaseMetalake.class);
+ PropertiesMetadata propertiesMetadata = mock(PropertiesMetadata.class);
+ when(propertiesMetadata.getOrDefault(any(), any())).thenReturn(true);
+ when(metalake.propertiesMetadata()).thenReturn(propertiesMetadata);
+ when(entityStore.get(any(), any(), any())).thenReturn(metalake);
+
+ try (MockedStatic<MetadataAuthzHelper> metadataAuthzHelper =
+ Mockito.mockStatic(MetadataAuthzHelper.class)) {
+ metadataAuthzHelper
+ .when(
+ () ->
+ MetadataAuthzHelper.filterByExpression(
+ Mockito.eq("metalake1"),
+ Mockito.eq("METALAKE::OWNER || METALAKE::MANAGE_GROUPS
|| GROUP::SELF"),
+ Mockito.eq(EntityType.GROUP),
+ Mockito.any(String[].class),
+ Mockito.any()))
+ .thenReturn(new String[] {"group"});
+
+ GroupOperations groupOperations = new GroupOperations();
+ HttpServletRequest request = mock(HttpServletRequest.class);
+ FieldUtils.writeField(groupOperations, "httpRequest", request, true);
+
+ Response resp = groupOperations.listGroups("metalake1", false);
+
+ Assertions.assertEquals(Response.Status.OK.getStatusCode(),
resp.getStatus());
+ NameListResponse listResponse = (NameListResponse) resp.getEntity();
+ Assertions.assertArrayEquals(new String[] {"group"},
listResponse.getNames());
+ }
+ }
+
@Test
public void testListGroups() throws IOException {
Group group = buildGroup("group");
@@ -445,6 +485,46 @@ public class TestGroupOperations extends
BaseOperationsTest {
Assertions.assertEquals(RuntimeException.class.getSimpleName(),
errorResponse2.getType());
}
+ @Test
+ public void testListGroupsFiltersByAuthorizationExpression() throws
Exception {
+ Mockito.reset(manager, entityStore);
+ Group group = buildGroup("group");
+ Group filteredGroup = buildGroup("filtered");
+ Mockito.doReturn(new Group[] {group,
filteredGroup}).when(manager).listGroups(any());
+
+ // Mock metalake with in-use property
+ BaseMetalake metalake = mock(BaseMetalake.class);
+ PropertiesMetadata propertiesMetadata = mock(PropertiesMetadata.class);
+ when(propertiesMetadata.getOrDefault(any(), any())).thenReturn(true);
+ when(metalake.propertiesMetadata()).thenReturn(propertiesMetadata);
+ when(entityStore.get(any(), any(), any())).thenReturn(metalake);
+
+ try (MockedStatic<MetadataAuthzHelper> metadataAuthzHelper =
+ Mockito.mockStatic(MetadataAuthzHelper.class)) {
+ metadataAuthzHelper
+ .when(
+ () ->
+ MetadataAuthzHelper.filterByExpression(
+ Mockito.eq("metalake1"),
+ Mockito.eq("METALAKE::OWNER || METALAKE::MANAGE_GROUPS
|| GROUP::SELF"),
+ Mockito.eq(EntityType.GROUP),
+ Mockito.any(Group[].class),
+ Mockito.any()))
+ .thenReturn(new Group[] {group});
+
+ GroupOperations groupOperations = new GroupOperations();
+ HttpServletRequest request = mock(HttpServletRequest.class);
+ FieldUtils.writeField(groupOperations, "httpRequest", request, true);
+
+ Response resp = groupOperations.listGroups("metalake1", true);
+
+ Assertions.assertEquals(Response.Status.OK.getStatusCode(),
resp.getStatus());
+ GroupListResponse listResponse = (GroupListResponse) resp.getEntity();
+ Assertions.assertEquals(1, listResponse.getGroups().length);
+ Assertions.assertEquals(group.name(),
listResponse.getGroups()[0].name());
+ }
+ }
+
@Test
public void testRemoveGroup() throws IOException {
// Mock metalake with in-use property
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/rest/authorization/TestGroupAuthorizationExpression.java
b/server/src/test/java/org/apache/gravitino/server/web/rest/authorization/TestGroupAuthorizationExpression.java
new file mode 100644
index 0000000000..7e0d39a08f
--- /dev/null
+++
b/server/src/test/java/org/apache/gravitino/server/web/rest/authorization/TestGroupAuthorizationExpression.java
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.server.web.rest.authorization;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import com.google.common.collect.ImmutableSet;
+import java.lang.reflect.Method;
+import java.lang.reflect.Parameter;
+import ognl.OgnlException;
+import org.apache.gravitino.Entity;
+import
org.apache.gravitino.server.authorization.annotations.AuthorizationExpression;
+import
org.apache.gravitino.server.authorization.annotations.AuthorizationMetadata;
+import org.apache.gravitino.server.web.rest.GroupOperations;
+import org.junit.jupiter.api.Test;
+
+public class TestGroupAuthorizationExpression {
+
+ @Test
+ public void testGetGroupAuthorizationExpression() throws
NoSuchMethodException, OgnlException {
+ Method method = GroupOperations.class.getMethod("getGroup", String.class,
String.class);
+ AuthorizationExpression authorizationExpressionAnnotation =
+ method.getAnnotation(AuthorizationExpression.class);
+ assertNotNull(authorizationExpressionAnnotation);
+
+ MockAuthorizationExpressionEvaluator mockEvaluator =
+ new
MockAuthorizationExpressionEvaluator(authorizationExpressionAnnotation.expression());
+ assertFalse(mockEvaluator.getResult(ImmutableSet.of()));
+
assertFalse(mockEvaluator.getResult(ImmutableSet.of("METALAKE::MANAGE_USERS")));
+ assertTrue(mockEvaluator.getResult(ImmutableSet.of("METALAKE::OWNER")));
+
assertTrue(mockEvaluator.getResult(ImmutableSet.of("METALAKE::MANAGE_GROUPS")));
+ assertTrue(mockEvaluator.getResult(ImmutableSet.of("GROUP::SELF")));
+ }
+
+ @Test
+ public void testGetGroupAuthorizationMetadata() throws NoSuchMethodException
{
+ Method method = GroupOperations.class.getMethod("getGroup", String.class,
String.class);
+ Parameter[] parameters = method.getParameters();
+
+ AuthorizationMetadata metalakeMetadata =
+ parameters[0].getAnnotation(AuthorizationMetadata.class);
+ assertNotNull(metalakeMetadata);
+ assertEquals(Entity.EntityType.METALAKE, metalakeMetadata.type());
+
+ AuthorizationMetadata groupMetadata =
parameters[1].getAnnotation(AuthorizationMetadata.class);
+ assertNotNull(groupMetadata);
+ assertEquals(Entity.EntityType.GROUP, groupMetadata.type());
+ }
+}
