This is an automated email from the ASF dual-hosted git repository.
yuqi1129 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 3a0bace80e [Cherry-pick to branch-1.3] [#11403] improvement(authz):
use per-request cache for group owner check in JcasbinAuthorizer (#11404)
(#11435)
3a0bace80e is described below
commit 3a0bace80e514c4ff3430ccda912a597a4b23da7
Author: github-actions[bot]
<41898282+github-actions[bot]@users.noreply.github.com>
AuthorDate: Fri Jun 5 09:14:31 2026 +0800
[Cherry-pick to branch-1.3] [#11403] improvement(authz): use per-request
cache for group owner check in JcasbinAuthorizer (#11404) (#11435)
**Cherry-pick Information:**
- Original commit: 651ceea8fcf9c31f122d6010de06b667883616ef
- Target branch: `branch-1.3`
- Status: ✅ Clean cherry-pick (no conflicts)
Co-authored-by: Qi Yu <[email protected]>
---
.../main/java/org/apache/gravitino/UserGroup.java | 22 +++---
.../gravitino/auth/TestGroupMapperFactory.java | 8 +--
.../gravitino/idp/auth/TestBasicAuthenticator.java | 4 +-
.../authorization/jcasbin/JcasbinAuthorizer.java | 33 +++------
.../authentication/TestJwksTokenValidator.java | 4 +-
.../authentication/TestStaticSignKeyValidator.java | 8 +--
.../jcasbin/TestJcasbinAuthorizer.java | 84 ++++++++++++++++------
7 files changed, 94 insertions(+), 69 deletions(-)
diff --git a/core/src/main/java/org/apache/gravitino/UserGroup.java
b/core/src/main/java/org/apache/gravitino/UserGroup.java
index 30e6109cd8..1bdccd5b73 100644
--- a/core/src/main/java/org/apache/gravitino/UserGroup.java
+++ b/core/src/main/java/org/apache/gravitino/UserGroup.java
@@ -27,18 +27,18 @@ import java.util.Optional;
public class UserGroup {
private final Optional<String> groupExternalUID;
- private final String groupname;
+ private final String groupName;
/**
* Constructs a UserGroup instance.
*
* @param groupExternalUID The external UID of the group.
- * @param groupname The name of the group.
+ * @param groupName The name of the group.
*/
- public UserGroup(Optional<String> groupExternalUID, String groupname) {
- Preconditions.checkArgument(groupname != null, "groupname cannot be null");
+ public UserGroup(Optional<String> groupExternalUID, String groupName) {
+ Preconditions.checkArgument(groupName != null, "groupName cannot be null");
this.groupExternalUID = groupExternalUID;
- this.groupname = groupname;
+ this.groupName = groupName;
}
/**
@@ -55,8 +55,8 @@ public class UserGroup {
*
* @return The group name.
*/
- public String getGroupname() {
- return groupname;
+ public String getGroupName() {
+ return groupName;
}
@Override
@@ -69,12 +69,12 @@ public class UserGroup {
}
UserGroup userGroup = (UserGroup) o;
return Objects.equals(groupExternalUID, userGroup.groupExternalUID)
- && Objects.equals(groupname, userGroup.groupname);
+ && Objects.equals(groupName, userGroup.groupName);
}
@Override
public int hashCode() {
- return Objects.hash(groupExternalUID, groupname);
+ return Objects.hash(groupExternalUID, groupName);
}
@Override
@@ -82,8 +82,8 @@ public class UserGroup {
return "UserGroup{"
+ "groupExternalUID="
+ groupExternalUID
- + ", groupname='"
- + groupname
+ + ", groupName='"
+ + groupName
+ '\''
+ '}';
}
diff --git
a/core/src/test/java/org/apache/gravitino/auth/TestGroupMapperFactory.java
b/core/src/test/java/org/apache/gravitino/auth/TestGroupMapperFactory.java
index 6b7dbc6b29..e6f1847308 100644
--- a/core/src/test/java/org/apache/gravitino/auth/TestGroupMapperFactory.java
+++ b/core/src/test/java/org/apache/gravitino/auth/TestGroupMapperFactory.java
@@ -49,7 +49,7 @@ public class TestGroupMapperFactory {
assertEquals(2, mappedGroups.size());
List<String> groupNames =
-
mappedGroups.stream().map(UserGroup::getGroupname).collect(Collectors.toList());
+
mappedGroups.stream().map(UserGroup::getGroupName).collect(Collectors.toList());
assertTrue(groupNames.contains("admin"));
assertTrue(groupNames.contains("user"));
}
@@ -66,7 +66,7 @@ public class TestGroupMapperFactory {
assertEquals(2, mappedGroups.size());
List<String> groupNames =
-
mappedGroups.stream().map(UserGroup::getGroupname).collect(Collectors.toList());
+
mappedGroups.stream().map(UserGroup::getGroupName).collect(Collectors.toList());
assertTrue(groupNames.contains("admin"));
assertTrue(groupNames.contains("user"));
}
@@ -83,7 +83,7 @@ public class TestGroupMapperFactory {
assertEquals(2, mappedGroups.size());
List<String> groupNames =
-
mappedGroups.stream().map(UserGroup::getGroupname).collect(Collectors.toList());
+
mappedGroups.stream().map(UserGroup::getGroupName).collect(Collectors.toList());
assertTrue(groupNames.contains("admin"));
assertTrue(groupNames.contains("user"));
}
@@ -136,7 +136,7 @@ public class TestGroupMapperFactory {
List<UserGroup> mappedGroups = mapper.map(groups);
assertEquals(1, mappedGroups.size());
- assertEquals("custom:foo", mappedGroups.get(0).getGroupname());
+ assertEquals("custom:foo", mappedGroups.get(0).getGroupName());
}
@Test
diff --git
a/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/auth/TestBasicAuthenticator.java
b/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/auth/TestBasicAuthenticator.java
index 661ea88bda..e31c9c8bb7 100644
---
a/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/auth/TestBasicAuthenticator.java
+++
b/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/auth/TestBasicAuthenticator.java
@@ -96,8 +96,8 @@ class TestBasicAuthenticator {
assertEquals("alice", principal.getName());
assertEquals(authHeader, principal.getAccessToken().orElse(null));
assertEquals(2, principal.getGroups().size());
- assertEquals("group-a", principal.getGroups().get(0).getGroupname());
- assertEquals("group-b", principal.getGroups().get(1).getGroupname());
+ assertEquals("group-a", principal.getGroups().get(0).getGroupName());
+ assertEquals("group-b", principal.getGroups().get(1).getGroupName());
}
@Test
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 f22e6a39f6..347900e606 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
@@ -55,7 +55,6 @@ import org.apache.gravitino.authorization.Privilege;
import org.apache.gravitino.authorization.SecurableObject;
import org.apache.gravitino.cache.CaffeineGravitinoCache;
import org.apache.gravitino.cache.GravitinoCache;
-import org.apache.gravitino.meta.GroupEntity;
import org.apache.gravitino.meta.RoleEntity;
import org.apache.gravitino.server.authorization.MetadataIdConverter;
import org.apache.gravitino.storage.relational.SupportsEntityChangeLog;
@@ -871,6 +870,8 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
* Returns true when the cached owner type and ID match the given principal
or one of the
* principal's groups. The user id is resolved via the version-validated
{@link #loadUserInfo}
* cache so back-to-back ownership checks in the same request do not
re-query {@code user_meta}.
+ * Group ids are resolved via {@link #loadGroupInfo}, which deduplicates
within the request via
+ * {@code requestContext.groupInfoCache} and avoids loading full group
entity objects.
*/
private boolean ownerMatchesUserOrGroups(
Optional<OwnerInfo> owner,
@@ -889,9 +890,9 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
if
(!Entity.EntityType.GROUP.name().equalsIgnoreCase(ownerInfo.getOwnerType())) {
return false;
}
- EntityStore entityStore = GravitinoEnv.getInstance().entityStore();
- for (GroupEntity groupEntity : resolveCurrentUserGroups(metalake,
entityStore)) {
- if (Objects.equals(groupEntity.id(), ownerInfo.getOwnerId())) {
+ for (String groupName : principalGroupNames(principal)) {
+ Optional<GroupUpdatedAt> groupInfo = loadGroupInfo(metalake, groupName,
requestContext);
+ if (groupInfo.isPresent() && groupInfo.get().getGroupId() ==
ownerInfo.getOwnerId()) {
return true;
}
}
@@ -1033,24 +1034,10 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
* or has no groups.
*/
private List<String> currentPrincipalGroupNames() {
- Principal principal = PrincipalUtils.getCurrentPrincipal();
- if (!(principal instanceof UserPrincipal)) {
- return new ArrayList<>();
- }
- List<UserGroup> groups = ((UserPrincipal) principal).getGroups();
- if (groups.isEmpty()) {
- return new ArrayList<>();
- }
- return
groups.stream().map(UserGroup::getGroupname).collect(Collectors.toList());
+ return principalGroupNames(PrincipalUtils.getCurrentPrincipal());
}
- /**
- * Resolves GroupEntity objects for the current principal's groups, skipping
any that are stale or
- * not found in the store. Used by owner checks that need full group
entities instead of only
- * group names.
- */
- private List<GroupEntity> resolveCurrentUserGroups(String metalake,
EntityStore entityStore) {
- Principal principal = PrincipalUtils.getCurrentPrincipal();
+ private List<String> principalGroupNames(Principal principal) {
if (!(principal instanceof UserPrincipal)) {
return new ArrayList<>();
}
@@ -1058,11 +1045,7 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
if (groups.isEmpty()) {
return new ArrayList<>();
}
- List<NameIdentifier> groupIdents =
- groups.stream()
- .map(g -> NameIdentifierUtil.ofGroup(metalake, g.getGroupname()))
- .collect(Collectors.toList());
- return entityStore.batchGet(groupIdents, Entity.EntityType.GROUP,
GroupEntity.class);
+ return
groups.stream().map(UserGroup::getGroupName).collect(Collectors.toList());
}
private void versionCheckAndLoadRoles(
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/authentication/TestJwksTokenValidator.java
b/server-common/src/test/java/org/apache/gravitino/server/authentication/TestJwksTokenValidator.java
index 3acbac918e..2fb1c1f2f7 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/authentication/TestJwksTokenValidator.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/authentication/TestJwksTokenValidator.java
@@ -606,9 +606,9 @@ public class TestJwksTokenValidator {
assertEquals("test-subject", userPrincipal.getName());
assertEquals(2, userPrincipal.getGroups().size());
assertTrue(
- userPrincipal.getGroups().stream().anyMatch(g ->
g.getGroupname().equals("group1")));
+ userPrincipal.getGroups().stream().anyMatch(g ->
g.getGroupName().equals("group1")));
assertTrue(
- userPrincipal.getGroups().stream().anyMatch(g ->
g.getGroupname().equals("group2")));
+ userPrincipal.getGroups().stream().anyMatch(g ->
g.getGroupName().equals("group2")));
}
}
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/authentication/TestStaticSignKeyValidator.java
b/server-common/src/test/java/org/apache/gravitino/server/authentication/TestStaticSignKeyValidator.java
index 6cf6836169..c6f603ff47 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/authentication/TestStaticSignKeyValidator.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/authentication/TestStaticSignKeyValidator.java
@@ -514,8 +514,8 @@ public class TestStaticSignKeyValidator {
// Check if principal is UserPrincipal and has groups
UserPrincipal userPrincipal = assertInstanceOf(UserPrincipal.class,
principal);
assertEquals(2, userPrincipal.getGroups().size());
- assertTrue(userPrincipal.getGroups().stream().anyMatch(g ->
g.getGroupname().equals("group1")));
- assertTrue(userPrincipal.getGroups().stream().anyMatch(g ->
g.getGroupname().equals("group2")));
+ assertTrue(userPrincipal.getGroups().stream().anyMatch(g ->
g.getGroupName().equals("group1")));
+ assertTrue(userPrincipal.getGroups().stream().anyMatch(g ->
g.getGroupName().equals("group2")));
}
@Test
@@ -584,7 +584,7 @@ public class TestStaticSignKeyValidator {
// Check if principal is UserPrincipal and has groups
UserPrincipal userPrincipal = assertInstanceOf(UserPrincipal.class,
principal);
assertEquals(2, userPrincipal.getGroups().size());
- assertTrue(userPrincipal.getGroups().stream().anyMatch(g ->
g.getGroupname().equals("groupa")));
- assertTrue(userPrincipal.getGroups().stream().anyMatch(g ->
g.getGroupname().equals("groupb")));
+ assertTrue(userPrincipal.getGroups().stream().anyMatch(g ->
g.getGroupName().equals("groupa")));
+ assertTrue(userPrincipal.getGroups().stream().anyMatch(g ->
g.getGroupName().equals("groupb")));
}
}
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 4b3292027f..67f84a6f99 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
@@ -618,27 +618,12 @@ public class TestJcasbinAuthorizer {
NameIdentifier catalogIdent = NameIdentifierUtil.ofCatalog(METALAKE,
"testCatalog");
- // Mock entityStore.batchGet for group entity lookup (needed for ID-based
ownership
- // verification)
- when(entityStore.batchGet(
- eq(ImmutableList.of(NameIdentifierUtil.ofGroup(METALAKE,
GROUP_NAME))),
- eq(Entity.EntityType.GROUP),
- eq(GroupEntity.class)))
- .thenReturn(ImmutableList.of(getGroupEntity()));
-
- // For non-member principal, mock batchGet for "otherGroup"
- when(entityStore.batchGet(
- eq(ImmutableList.of(NameIdentifierUtil.ofGroup(METALAKE,
"otherGroup"))),
- eq(Entity.EntityType.GROUP),
- eq(GroupEntity.class)))
- .thenReturn(
- ImmutableList.of(
- GroupEntity.builder()
- .withId(99L)
- .withName("otherGroup")
- .withNamespace(Namespace.of(METALAKE, "group"))
- .withAuditInfo(AuditInfo.EMPTY)
- .build()));
+ // Group identity is now resolved via groupMetaMapper.getGroupUpdatedAt
(per-request cache path)
+ // instead of entityStore.batchGet(GROUP); verify the new path is wired
correctly.
+ when(groupMetaMapper.getGroupUpdatedAt(eq(METALAKE), eq(GROUP_NAME)))
+ .thenReturn(new GroupUpdatedAt(GROUP_ID,
groupVersionCounter.incrementAndGet()));
+ when(groupMetaMapper.getGroupUpdatedAt(eq(METALAKE), eq("otherGroup")))
+ .thenReturn(new GroupUpdatedAt(99L,
groupVersionCounter.incrementAndGet()));
// Mock owner_meta lookup returning a GROUP-typed OwnerInfo (the owner is
GROUP_ID).
OwnerInfo groupOwnerInfo = new OwnerInfo(GROUP_ID, "GROUP");
@@ -649,6 +634,10 @@ public class TestJcasbinAuthorizer {
// The principal belongs to the owning group, so isOwner should return true
assertTrue(doAuthorizeOwner(groupPrincipal));
+ // entityStore.batchGet must NOT be called for GROUP entity lookups in the
owner-check path
+ Mockito.verify(entityStore, Mockito.never())
+ .batchGet(anyList(), eq(Entity.EntityType.GROUP),
eq(GroupEntity.class));
+
// Clear owner and verify it returns false
when(ownerMetaMapper.selectOwnerByMetadataObjectIdAndType(eq(CATALOG_ID),
eq("CATALOG")))
.thenReturn(null);
@@ -675,6 +664,59 @@ public class TestJcasbinAuthorizer {
.thenReturn(new UserPrincipal(USERNAME));
}
+ @Test
+ public void testGroupOwnerCheckDeduplicatesGroupInfoWithinRequest() throws
Exception {
+ // Verify that repeated isOwner calls within the same
AuthorizationRequestContext
+ // do not re-query group_meta; groupMetaMapper.getGroupUpdatedAt should be
called at most once
+ // per (metalake, groupName) per request thanks to
requestContext.groupInfoCache.
+ Mockito.clearInvocations(groupMetaMapper);
+
+ UserPrincipal groupPrincipal =
+ new UserPrincipal(USERNAME, ImmutableList.of(new
UserGroup(Optional.empty(), GROUP_NAME)));
+
principalUtilsMockedStatic.when(PrincipalUtils::getCurrentPrincipal).thenReturn(groupPrincipal);
+
+ when(groupMetaMapper.getGroupUpdatedAt(eq(METALAKE), eq(GROUP_NAME)))
+ .thenReturn(new GroupUpdatedAt(GROUP_ID,
groupVersionCounter.incrementAndGet()));
+
+ OwnerInfo groupOwnerInfo = new OwnerInfo(GROUP_ID, "GROUP");
+ when(ownerMetaMapper.selectOwnerByMetadataObjectIdAndType(eq(CATALOG_ID),
eq("CATALOG")))
+ .thenReturn(groupOwnerInfo);
+ getOwnerRelCache(jcasbinAuthorizer).invalidateAll();
+
+ MetadataObject catalog = MetadataObjects.of(null, "testCatalog",
MetadataObject.Type.CATALOG);
+
+ // Call isOwner twice within the same request context
+ AuthorizationRequestContext sharedContext = new
AuthorizationRequestContext();
+ assertTrue(jcasbinAuthorizer.isOwner(groupPrincipal, METALAKE, catalog,
sharedContext));
+ assertTrue(jcasbinAuthorizer.isOwner(groupPrincipal, METALAKE, catalog,
sharedContext));
+
+ // group_meta should have been queried exactly once despite two isOwner
calls
+ Mockito.verify(groupMetaMapper, Mockito.times(1))
+ .getGroupUpdatedAt(eq(METALAKE), eq(GROUP_NAME));
+ }
+
+ @Test
+ public void testGroupOwnerCheckUsesProvidedPrincipalGroups() throws
Exception {
+ UserPrincipal ownerGroupPrincipal =
+ new UserPrincipal(USERNAME, ImmutableList.of(new
UserGroup(Optional.empty(), GROUP_NAME)));
+ UserPrincipal currentPrincipalWithoutOwnerGroup =
+ new UserPrincipal(
+ USERNAME, ImmutableList.of(new UserGroup(Optional.empty(),
"otherGroup")));
+ principalUtilsMockedStatic
+ .when(PrincipalUtils::getCurrentPrincipal)
+ .thenReturn(currentPrincipalWithoutOwnerGroup);
+
+ when(groupMetaMapper.getGroupUpdatedAt(eq(METALAKE), eq(GROUP_NAME)))
+ .thenReturn(new GroupUpdatedAt(GROUP_ID,
groupVersionCounter.incrementAndGet()));
+
+ OwnerInfo groupOwnerInfo = new OwnerInfo(GROUP_ID, "GROUP");
+ when(ownerMetaMapper.selectOwnerByMetadataObjectIdAndType(eq(CATALOG_ID),
eq("CATALOG")))
+ .thenReturn(groupOwnerInfo);
+ getOwnerRelCache(jcasbinAuthorizer).invalidateAll();
+
+ assertTrue(doAuthorizeOwner(ownerGroupPrincipal));
+ }
+
@Test
public void testAuthorizeByGroupRole() throws Exception {
makeCompletableFutureUseCurrentThread(jcasbinAuthorizer);
