This is an automated email from the ASF dual-hosted git repository. yuqi1129 pushed a commit to branch fix/11403-group-owner-check-cache in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit 520015fe2bb62b33f1149908073f5cf85fbcbfff Author: yuqi <[email protected]> AuthorDate: Wed Jun 3 16:24:19 2026 +0800 [#11403] improvement(authz): use per-request cache for group owner check in JcasbinAuthorizer Replace resolveCurrentUserGroups(entityStore) in ownerMatchesUserOrGroups() with currentPrincipalGroupNames() + loadGroupInfo(), routing group-id lookups through requestContext.groupInfoCache (per-request dedup) instead of issuing a direct entityStore.batchGet(GROUP) on every GROUP owner check. Remove the now-unused resolveCurrentUserGroups() method and its GroupEntity import. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> --- .../authorization/jcasbin/JcasbinAuthorizer.java | 30 ++--------- .../jcasbin/TestJcasbinAuthorizer.java | 62 ++++++++++++++-------- 2 files changed, 46 insertions(+), 46 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 f22e6a39f6..415984a7d4 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 : currentPrincipalGroupNames()) { + Optional<GroupUpdatedAt> groupInfo = loadGroupInfo(metalake, groupname, requestContext); + if (groupInfo.isPresent() && groupInfo.get().getGroupId() == ownerInfo.getOwnerId()) { return true; } } @@ -1044,27 +1045,6 @@ public class JcasbinAuthorizer implements GravitinoAuthorizer { return groups.stream().map(UserGroup::getGroupname).collect(Collectors.toList()); } - /** - * 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(); - if (!(principal instanceof UserPrincipal)) { - return new ArrayList<>(); - } - List<UserGroup> groups = ((UserPrincipal) principal).getGroups(); - 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); - } - private void versionCheckAndLoadRoles( String metalake, List<Long> roleIds, AuthorizationRequestContext requestContext) { List<Long> uniqueRoleIds = roleIds.stream().distinct().collect(Collectors.toList()); 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..8439d61479 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,37 @@ 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 testAuthorizeByGroupRole() throws Exception { makeCompletableFutureUseCurrentThread(jcasbinAuthorizer);
