Skip to content

Commit ba486bf

Browse files
authored
[#10412] feat(server-common): Support group ownership in JcasbinAuthorizer (#10867)
### What changes were proposed in this pull request? Extend `JcasbinAuthorizer` to recognize group-based ownership. When a metadata object is owned by a group, all members of that group are now treated as owners and granted owner privileges. Key changes: - **`OwnerInfo` inner class**: Replaces the raw `Long` owner ID in the cache with a struct that stores `id`, `type` (USER/GROUP), and `name`. - **`ownerRel` cache type**: Changed from `Cache<Long, Optional<Long>>` to `Cache<Long, Optional<OwnerInfo>>`. - **`loadOwnerPolicy()`**: Now handles `GroupEntity` alongside `UserEntity` when populating the owner cache. - **`checkOwnership()`**: New method that resolves both user and group owners — for USER owners it compares entity IDs, for GROUP owners it checks whether the principal's groups include the owning group. - **`isOwner()` and `authorizeByJcasbin()`**: Refactored to delegate to `checkOwnership()`, eliminating duplicated ownership logic. - **Documentation**: Removed "group ownership not supported" info boxes from `docs/security/access-control.md` and added group ownership bullet. ### Why are the changes needed? Currently, when a metadata object's owner is set to a group (supported since #10848), the `JcasbinAuthorizer` does not recognize group ownership — only individual user ownership is checked. This means group members are denied owner privileges even when the group is the registered owner. This PR is part 2 of a 3-PR series for #10412: 1. **#10848** — Core/API: `OwnerManager.setOwner` accepts GROUP (**merged**) 2. **This PR** — Enforcement: `JcasbinAuthorizer` recognizes group ownership 3. **Planned** — Role inheritance: `loadRolePrivilege` queries `ROLE_GROUP_REL` Fix: #10412 ### Does this PR introduce _any_ user-facing change? Yes. Users who assign group ownership to metadata objects will now have all group members recognized as owners by the JCasbin authorization plugin. ### How was this patch tested? Unit tests in `TestJcasbinAuthorizer`: - `testAuthorizeByGroupOwner`: Verifies a principal whose groups include the owning group is recognized as owner, a non-member group is denied, and clearing ownership returns false. - Existing tests (`testIsOwner`, `testOwnerCacheInvalidation`, `testCacheInitialization`) updated for `OwnerInfo` type. - All 8 tests pass: `./gradlew :server-common:test --tests "...TestJcasbinAuthorizer"`
1 parent f4d6178 commit ba486bf

3 files changed

Lines changed: 182 additions & 24 deletions

File tree

docs/security/access-control.md

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -132,10 +132,7 @@ Every securable object in Gravitino has an owner - the user with administrative
132132
- **Automatic assignment**: The creator of an object automatically becomes its owner
133133
- **Administrative privileges**: Owners have implicit management privileges (e.g., drop, alter)
134134
- **Exclusive control**: Only the owner can fully manage the object
135-
136-
:::info
137-
Group ownership is not currently supported. Only user ownership is available.
138-
:::
135+
- **Group ownership**: Ownership can be assigned to a group, granting all members of that group owner privileges
139136

140137
**Supported Objects:**
141138

@@ -173,10 +170,6 @@ A group is a collection of users that simplifies permission management by allowi
173170

174171
All users in a group inherit the roles and privileges granted to that group.
175172

176-
:::info
177-
Groups can be granted roles and privileges, but they cannot be owners of securable objects. Only users can be owners.
178-
:::
179-
180173
### Metadata Objects
181174

182175
Metadata objects are entities managed by Gravitino, such as catalogs, schemas, tables, filesets, topics, models, functions, roles, and metalakes.

server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java

Lines changed: 82 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import java.util.concurrent.Executors;
3636
import java.util.concurrent.ThreadPoolExecutor;
3737
import java.util.concurrent.TimeUnit;
38+
import java.util.stream.Collectors;
3839
import org.apache.commons.io.IOUtils;
3940
import org.apache.commons.lang3.StringUtils;
4041
import org.apache.gravitino.Configs;
@@ -45,13 +46,16 @@
4546
import org.apache.gravitino.MetadataObjects;
4647
import org.apache.gravitino.NameIdentifier;
4748
import org.apache.gravitino.SupportsRelationOperations;
49+
import org.apache.gravitino.UserGroup;
50+
import org.apache.gravitino.UserPrincipal;
4851
import org.apache.gravitino.auth.AuthConstants;
4952
import org.apache.gravitino.authorization.AuthorizationRequestContext;
5053
import org.apache.gravitino.authorization.AuthorizationUtils;
5154
import org.apache.gravitino.authorization.GravitinoAuthorizer;
5255
import org.apache.gravitino.authorization.Privilege;
5356
import org.apache.gravitino.authorization.SecurableObject;
5457
import org.apache.gravitino.exceptions.NoSuchUserException;
58+
import org.apache.gravitino.meta.GroupEntity;
5559
import org.apache.gravitino.meta.RoleEntity;
5660
import org.apache.gravitino.meta.UserEntity;
5761
import org.apache.gravitino.server.authorization.MetadataIdConverter;
@@ -87,7 +91,7 @@ public class JcasbinAuthorizer implements GravitinoAuthorizer {
8791
*/
8892
private Cache<Long, Boolean> loadedRoles;
8993

90-
private Cache<Long, Optional<Long>> ownerRel;
94+
private Cache<Long, Optional<OwnerInfo>> ownerRel;
9195

9296
private Executor executor = null;
9397

@@ -218,15 +222,11 @@ public boolean isOwner(
218222
String metalake,
219223
MetadataObject metadataObject,
220224
AuthorizationRequestContext requestContext) {
221-
Long userId;
222225
boolean result;
223226
try {
224227
Long metadataId = MetadataIdConverter.getID(metadataObject, metalake);
225228
loadOwnerPolicy(metalake, metadataObject, metadataId);
226-
UserEntity userEntity = getUserEntity(principal.getName(), metalake);
227-
userId = userEntity.id();
228-
metadataId = MetadataIdConverter.getID(metadataObject, metalake);
229-
result = Objects.equals(Optional.of(userId), ownerRel.getIfPresent(metadataId));
229+
result = checkOwnership(principal, metalake, metadataId);
230230
} catch (Exception e) {
231231
LOG.debug("Can not get entity id", e);
232232
result = false;
@@ -457,14 +457,17 @@ private boolean loadPrivilegeAndAuthorize(
457457
return false;
458458
}
459459
loadRolePrivilege(metalake, username, userId, requestContext);
460-
return authorizeByJcasbin(userId, metadataObject, metadataId, privilege);
460+
return authorizeByJcasbin(userId, metalake, metadataObject, metadataId, privilege);
461461
}
462462

463463
private boolean authorizeByJcasbin(
464-
Long userId, MetadataObject metadataObject, Long metadataId, String privilege) {
464+
Long userId,
465+
String metalake,
466+
MetadataObject metadataObject,
467+
Long metadataId,
468+
String privilege) {
465469
if (AuthConstants.OWNER.equals(privilege)) {
466-
Optional<Long> owner = ownerRel.getIfPresent(metadataId);
467-
return Objects.equals(Optional.of(userId), owner);
470+
return checkOwnership(PrincipalUtils.getCurrentPrincipal(), metalake, metadataId);
468471
}
469472
return enforcer.enforce(
470473
String.valueOf(userId),
@@ -556,7 +559,14 @@ private void loadOwnerPolicy(String metalake, MetadataObject metadataObject, Lon
556559
for (Entity ownerEntity : owners) {
557560
if (ownerEntity instanceof UserEntity) {
558561
UserEntity user = (UserEntity) ownerEntity;
559-
ownerRel.put(metadataId, Optional.of(user.id()));
562+
ownerRel.put(
563+
metadataId,
564+
Optional.of(new OwnerInfo(user.id(), Entity.EntityType.USER, user.name())));
565+
} else if (ownerEntity instanceof GroupEntity) {
566+
GroupEntity group = (GroupEntity) ownerEntity;
567+
ownerRel.put(
568+
metadataId,
569+
Optional.of(new OwnerInfo(group.id(), Entity.EntityType.GROUP, group.name())));
560570
}
561571
}
562572
}
@@ -601,4 +611,65 @@ private void loadPolicyByRoleEntity(RoleEntity roleEntity) {
601611
}
602612
}
603613
}
614+
615+
/**
616+
* Checks whether the given principal is the owner of the metadata object identified by
617+
* metadataId. Supports both user and group ownership.
618+
*/
619+
private boolean checkOwnership(Principal principal, String metalake, Long metadataId) {
620+
Optional<OwnerInfo> ownerOpt = ownerRel.getIfPresent(metadataId);
621+
if (ownerOpt == null || !ownerOpt.isPresent()) {
622+
return false;
623+
}
624+
OwnerInfo owner = ownerOpt.get();
625+
// We compare by entity ID rather than name to guard against stale cache entries.
626+
// If a user/group is deleted and recreated with the same name, the cached OwnerInfo
627+
// still holds the old ID. A name-only comparison would incorrectly grant ownership
628+
// to the new entity. The extra IO to fetch the current entity ensures correctness.
629+
if (owner.type == Entity.EntityType.USER) {
630+
try {
631+
UserEntity userEntity = getUserEntity(principal.getName(), metalake);
632+
return Objects.equals(userEntity.id(), owner.id);
633+
} catch (Exception e) {
634+
LOG.debug("Can not get user entity for ownership check", e);
635+
return false;
636+
}
637+
} else if (owner.type == Entity.EntityType.GROUP) {
638+
if (principal instanceof UserPrincipal) {
639+
List<UserGroup> groups = ((UserPrincipal) principal).getGroups();
640+
if (groups.isEmpty()) {
641+
return false;
642+
}
643+
try {
644+
List<NameIdentifier> groupIdents =
645+
groups.stream()
646+
.map(g -> NameIdentifierUtil.ofGroup(metalake, g.getGroupname()))
647+
.collect(Collectors.toList());
648+
List<GroupEntity> groupEntities =
649+
GravitinoEnv.getInstance()
650+
.entityStore()
651+
.batchGet(groupIdents, Entity.EntityType.GROUP, GroupEntity.class);
652+
return groupEntities.stream().anyMatch(ge -> Objects.equals(ge.id(), owner.id));
653+
} catch (Exception e) {
654+
LOG.debug("Can not get group entities for ownership check", e);
655+
return false;
656+
}
657+
}
658+
return false;
659+
}
660+
return false;
661+
}
662+
663+
/** Holds the owner identity for a metadata object in the owner cache. */
664+
static class OwnerInfo {
665+
final Long id;
666+
final Entity.EntityType type;
667+
final String name;
668+
669+
OwnerInfo(Long id, Entity.EntityType type, String name) {
670+
this.id = id;
671+
this.type = type;
672+
this.name = name;
673+
}
674+
}
604675
}

server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java

Lines changed: 99 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -51,17 +51,20 @@
5151
import org.apache.gravitino.NameIdentifier;
5252
import org.apache.gravitino.Namespace;
5353
import org.apache.gravitino.SupportsRelationOperations;
54+
import org.apache.gravitino.UserGroup;
5455
import org.apache.gravitino.UserPrincipal;
5556
import org.apache.gravitino.authorization.AuthorizationRequestContext;
5657
import org.apache.gravitino.authorization.Privilege;
5758
import org.apache.gravitino.authorization.SecurableObject;
5859
import org.apache.gravitino.meta.AuditInfo;
5960
import org.apache.gravitino.meta.BaseMetalake;
61+
import org.apache.gravitino.meta.GroupEntity;
6062
import org.apache.gravitino.meta.RoleEntity;
6163
import org.apache.gravitino.meta.SchemaVersion;
6264
import org.apache.gravitino.meta.UserEntity;
6365
import org.apache.gravitino.server.ServerConfig;
6466
import org.apache.gravitino.server.authorization.MetadataIdConverter;
67+
import org.apache.gravitino.server.authorization.jcasbin.JcasbinAuthorizer.OwnerInfo;
6568
import org.apache.gravitino.storage.relational.po.SecurableObjectPO;
6669
import org.apache.gravitino.storage.relational.service.OwnerMetaService;
6770
import org.apache.gravitino.storage.relational.utils.POConverters;
@@ -91,6 +94,10 @@ public class TestJcasbinAuthorizer {
9194

9295
private static final String METALAKE = "testMetalake";
9396

97+
private static final Long GROUP_ID = 6L;
98+
99+
private static final String GROUP_NAME = "testGroup";
100+
94101
private static EntityStore entityStore = mock(EntityStore.class);
95102

96103
private static GravitinoEnv gravitinoEnv = mock(GravitinoEnv.class);
@@ -259,6 +266,84 @@ public void testAuthorizeByOwner() throws Exception {
259266
assertFalse(doAuthorizeOwner(currentPrincipal));
260267
}
261268

269+
@Test
270+
public void testAuthorizeByGroupOwner() throws Exception {
271+
// Set up a UserPrincipal whose groups include GROUP_NAME
272+
UserPrincipal groupPrincipal =
273+
new UserPrincipal(USERNAME, ImmutableList.of(new UserGroup(Optional.empty(), GROUP_NAME)));
274+
principalUtilsMockedStatic.when(PrincipalUtils::getCurrentPrincipal).thenReturn(groupPrincipal);
275+
276+
NameIdentifier catalogIdent = NameIdentifierUtil.ofCatalog(METALAKE, "testCatalog");
277+
278+
// Mock entityStore.batchGet for group entity lookup (needed for ID-based ownership
279+
// verification)
280+
when(entityStore.batchGet(
281+
eq(ImmutableList.of(NameIdentifierUtil.ofGroup(METALAKE, GROUP_NAME))),
282+
eq(Entity.EntityType.GROUP),
283+
eq(GroupEntity.class)))
284+
.thenReturn(ImmutableList.of(getGroupEntity()));
285+
286+
// For non-member principal, mock batchGet for "otherGroup"
287+
when(entityStore.batchGet(
288+
eq(ImmutableList.of(NameIdentifierUtil.ofGroup(METALAKE, "otherGroup"))),
289+
eq(Entity.EntityType.GROUP),
290+
eq(GroupEntity.class)))
291+
.thenReturn(
292+
ImmutableList.of(
293+
GroupEntity.builder()
294+
.withId(99L)
295+
.withName("otherGroup")
296+
.withNamespace(Namespace.of(METALAKE, "group"))
297+
.withAuditInfo(AuditInfo.EMPTY)
298+
.build()));
299+
300+
// Mock owner relation returning a GroupEntity
301+
List<GroupEntity> owners = ImmutableList.of(getGroupEntity());
302+
doReturn(owners)
303+
.when(supportsRelationOperations)
304+
.listEntitiesByRelation(
305+
eq(SupportsRelationOperations.Type.OWNER_REL),
306+
eq(catalogIdent),
307+
eq(Entity.EntityType.CATALOG));
308+
getOwnerRelCache(jcasbinAuthorizer).invalidateAll();
309+
310+
// The principal belongs to the owning group, so isOwner should return true
311+
assertTrue(doAuthorizeOwner(groupPrincipal));
312+
313+
// Clear owner and verify it returns false
314+
doReturn(new ArrayList<>())
315+
.when(supportsRelationOperations)
316+
.listEntitiesByRelation(
317+
eq(SupportsRelationOperations.Type.OWNER_REL),
318+
eq(catalogIdent),
319+
eq(Entity.EntityType.CATALOG));
320+
jcasbinAuthorizer.handleMetadataOwnerChange(
321+
METALAKE, GROUP_ID, catalogIdent, Entity.EntityType.CATALOG);
322+
assertFalse(doAuthorizeOwner(groupPrincipal));
323+
324+
// Verify a principal whose groups do NOT include the owner group gets denied
325+
UserPrincipal nonMemberPrincipal =
326+
new UserPrincipal(
327+
USERNAME, ImmutableList.of(new UserGroup(Optional.empty(), "otherGroup")));
328+
principalUtilsMockedStatic
329+
.when(PrincipalUtils::getCurrentPrincipal)
330+
.thenReturn(nonMemberPrincipal);
331+
// Re-populate the owner cache with the group owner
332+
doReturn(ImmutableList.of(getGroupEntity()))
333+
.when(supportsRelationOperations)
334+
.listEntitiesByRelation(
335+
eq(SupportsRelationOperations.Type.OWNER_REL),
336+
eq(catalogIdent),
337+
eq(Entity.EntityType.CATALOG));
338+
getOwnerRelCache(jcasbinAuthorizer).invalidateAll();
339+
assertFalse(doAuthorizeOwner(nonMemberPrincipal));
340+
341+
// Restore the original principal mock
342+
principalUtilsMockedStatic
343+
.when(PrincipalUtils::getCurrentPrincipal)
344+
.thenReturn(new UserPrincipal(USERNAME));
345+
}
346+
262347
private Boolean doAuthorize(Principal currentPrincipal) {
263348
return jcasbinAuthorizer.authorize(
264349
currentPrincipal,
@@ -285,6 +370,15 @@ private static UserEntity getUserEntity() {
285370
.build();
286371
}
287372

373+
private static GroupEntity getGroupEntity() {
374+
return GroupEntity.builder()
375+
.withId(GROUP_ID)
376+
.withName(GROUP_NAME)
377+
.withNamespace(Namespace.of(METALAKE, "group"))
378+
.withAuditInfo(AuditInfo.EMPTY)
379+
.build();
380+
}
381+
288382
private static RoleEntity getRoleEntity(
289383
Long roleId, String roleName, List<SecurableObject> securableObjects) {
290384
Namespace namespace = NamespaceUtil.ofRole(METALAKE);
@@ -383,10 +477,10 @@ public void testRoleCacheInvalidation() throws Exception {
383477
@Test
384478
public void testOwnerCacheInvalidation() throws Exception {
385479
// Get the ownerRel cache via reflection
386-
Cache<Long, Optional<Long>> ownerRel = getOwnerRelCache(jcasbinAuthorizer);
480+
Cache<Long, Optional<OwnerInfo>> ownerRel = getOwnerRelCache(jcasbinAuthorizer);
387481

388482
// Manually add an owner relation to the cache
389-
ownerRel.put(CATALOG_ID, Optional.of(USER_ID));
483+
ownerRel.put(CATALOG_ID, Optional.of(new OwnerInfo(USER_ID, Entity.EntityType.USER, USERNAME)));
390484

391485
// Verify it's in the cache
392486
assertNotNull(ownerRel.getIfPresent(CATALOG_ID));
@@ -441,7 +535,7 @@ public void testRoleCacheSynchronousRemovalListenerDeletesPolicy() throws Except
441535
public void testCacheInitialization() throws Exception {
442536
// Verify that caches are initialized
443537
Cache<Long, Boolean> loadedRoles = getLoadedRolesCache(jcasbinAuthorizer);
444-
Cache<Long, Optional<Long>> ownerRel = getOwnerRelCache(jcasbinAuthorizer);
538+
Cache<Long, Optional<OwnerInfo>> ownerRel = getOwnerRelCache(jcasbinAuthorizer);
445539

446540
assertNotNull(loadedRoles, "loadedRoles cache should be initialized");
447541
assertNotNull(ownerRel, "ownerRel cache should be initialized");
@@ -597,11 +691,11 @@ private static Cache<Long, Boolean> getLoadedRolesCache(JcasbinAuthorizer author
597691
}
598692

599693
@SuppressWarnings("unchecked")
600-
private static Cache<Long, Optional<Long>> getOwnerRelCache(JcasbinAuthorizer authorizer)
694+
private static Cache<Long, Optional<OwnerInfo>> getOwnerRelCache(JcasbinAuthorizer authorizer)
601695
throws Exception {
602696
Field field = JcasbinAuthorizer.class.getDeclaredField("ownerRel");
603697
field.setAccessible(true);
604-
return (Cache<Long, Optional<Long>>) field.get(authorizer);
698+
return (Cache<Long, Optional<OwnerInfo>>) field.get(authorizer);
605699
}
606700

607701
private static Enforcer getAllowEnforcer(JcasbinAuthorizer authorizer) throws Exception {

0 commit comments

Comments
 (0)