From eb73cad12c225d6285d64684fcb0e6ea0f74c320 Mon Sep 17 00:00:00 2001 From: Rory Date: Wed, 10 Jul 2024 15:24:21 +0800 Subject: [PATCH 1/2] [#4105] improvement: Remove the logic of getValidRoles --- .../authorization/AccessControlManager.java | 2 +- .../authorization/PermissionManager.java | 35 +++++++++++----- .../gravitino/authorization/RoleManager.java | 25 ------------ .../authorization/UserGroupManager.java | 40 +++---------------- ...estAccessControlManagerForPermissions.java | 36 +---------------- .../service/TestGroupMetaService.java | 6 +++ .../service/TestUserMetaService.java | 6 +++ 7 files changed, 44 insertions(+), 106 deletions(-) diff --git a/core/src/main/java/com/datastrato/gravitino/authorization/AccessControlManager.java b/core/src/main/java/com/datastrato/gravitino/authorization/AccessControlManager.java index eb7dbfb04ef..26ec14a7e7b 100644 --- a/core/src/main/java/com/datastrato/gravitino/authorization/AccessControlManager.java +++ b/core/src/main/java/com/datastrato/gravitino/authorization/AccessControlManager.java @@ -53,7 +53,7 @@ public class AccessControlManager { public AccessControlManager(EntityStore store, IdGenerator idGenerator, Config config) { this.adminManager = new AdminManager(store, idGenerator, config); this.roleManager = new RoleManager(store, idGenerator, config); - this.userGroupManager = new UserGroupManager(store, idGenerator, roleManager); + this.userGroupManager = new UserGroupManager(store, idGenerator); this.permissionManager = new PermissionManager(store, roleManager); } diff --git a/core/src/main/java/com/datastrato/gravitino/authorization/PermissionManager.java b/core/src/main/java/com/datastrato/gravitino/authorization/PermissionManager.java index 3b24e8cde5c..9c5c91b46b1 100644 --- a/core/src/main/java/com/datastrato/gravitino/authorization/PermissionManager.java +++ b/core/src/main/java/com/datastrato/gravitino/authorization/PermissionManager.java @@ -42,7 +42,7 @@ /** * PermissionManager is used for managing the logic the granting and revoking roles. Role is used - * for manging permissions. PermissionManager will filter the invalid roles, too. + * for manging permissions. */ class PermissionManager { private static final Logger LOG = LoggerFactory.getLogger(PermissionManager.class); @@ -67,9 +67,12 @@ User grantRolesToUser(String metalake, List roles, String user) { UserEntity.class, Entity.EntityType.USER, userEntity -> { - List roleEntities = - roleManager.getValidRoles(metalake, userEntity.roleNames(), userEntity.roleIds()); - + List roleEntities = Lists.newArrayList(); + if (userEntity.roleNames() != null) { + for (String role : userEntity.roleNames()) { + roleEntities.add(roleManager.getRole(metalake, role)); + } + } List roleNames = Lists.newArrayList(toRoleNames(roleEntities)); List roleIds = Lists.newArrayList(toRoleIds(roleEntities)); @@ -129,8 +132,12 @@ Group grantRolesToGroup(String metalake, List roles, String group) { GroupEntity.class, Entity.EntityType.GROUP, groupEntity -> { - List roleEntities = - roleManager.getValidRoles(metalake, groupEntity.roleNames(), groupEntity.roleIds()); + List roleEntities = Lists.newArrayList(); + if (groupEntity.roleNames() != null) { + for (String role : groupEntity.roleNames()) { + roleEntities.add(roleManager.getRole(metalake, role)); + } + } List roleNames = Lists.newArrayList(toRoleNames(roleEntities)); List roleIds = Lists.newArrayList(toRoleIds(roleEntities)); @@ -190,8 +197,12 @@ Group revokeRolesFromGroup(String metalake, List roles, String group) { GroupEntity.class, Entity.EntityType.GROUP, groupEntity -> { - List roleEntities = - roleManager.getValidRoles(metalake, groupEntity.roleNames(), groupEntity.roleIds()); + List roleEntities = Lists.newArrayList(); + if (groupEntity.roleNames() != null) { + for (String role : groupEntity.roleNames()) { + roleEntities.add(roleManager.getRole(metalake, role)); + } + } List roleNames = Lists.newArrayList(toRoleNames(roleEntities)); List roleIds = Lists.newArrayList(toRoleIds(roleEntities)); @@ -252,8 +263,12 @@ User revokeRolesFromUser(String metalake, List roles, String user) { UserEntity.class, Entity.EntityType.USER, userEntity -> { - List roleEntities = - roleManager.getValidRoles(metalake, userEntity.roleNames(), userEntity.roleIds()); + List roleEntities = Lists.newArrayList(); + if (userEntity.roleNames() != null) { + for (String role : userEntity.roleNames()) { + roleEntities.add(roleManager.getRole(metalake, role)); + } + } List roleNames = Lists.newArrayList(toRoleNames(roleEntities)); List roleIds = Lists.newArrayList(toRoleIds(roleEntities)); diff --git a/core/src/main/java/com/datastrato/gravitino/authorization/RoleManager.java b/core/src/main/java/com/datastrato/gravitino/authorization/RoleManager.java index 96fc7170250..9d717b4eb9c 100644 --- a/core/src/main/java/com/datastrato/gravitino/authorization/RoleManager.java +++ b/core/src/main/java/com/datastrato/gravitino/authorization/RoleManager.java @@ -36,7 +36,6 @@ import com.github.benmanes.caffeine.cache.Caffeine; import com.github.benmanes.caffeine.cache.Scheduler; import com.google.common.annotations.VisibleForTesting; -import com.google.common.collect.Lists; import com.google.common.util.concurrent.ThreadFactoryBuilder; import java.io.IOException; import java.time.Instant; @@ -161,28 +160,4 @@ private RoleEntity getRoleEntity(NameIdentifier identifier) { Cache getCache() { return cache; } - - List getValidRoles(String metalake, List roleNames, List roleIds) { - List roleEntities = Lists.newArrayList(); - if (roleNames == null || roleNames.isEmpty()) { - return roleEntities; - } - - int index = 0; - for (String role : roleNames) { - try { - - RoleEntity roleEntity = getRoleEntity(AuthorizationUtils.ofRole(metalake, role)); - - if (roleEntity.id().equals(roleIds.get(index))) { - roleEntities.add(roleEntity); - } - index++; - - } catch (NoSuchEntityException nse) { - // ignore this entity - } - } - return roleEntities; - } } diff --git a/core/src/main/java/com/datastrato/gravitino/authorization/UserGroupManager.java b/core/src/main/java/com/datastrato/gravitino/authorization/UserGroupManager.java index c21b060a304..b92ab7d5a1d 100644 --- a/core/src/main/java/com/datastrato/gravitino/authorization/UserGroupManager.java +++ b/core/src/main/java/com/datastrato/gravitino/authorization/UserGroupManager.java @@ -28,7 +28,6 @@ import com.datastrato.gravitino.exceptions.UserAlreadyExistsException; import com.datastrato.gravitino.meta.AuditInfo; import com.datastrato.gravitino.meta.GroupEntity; -import com.datastrato.gravitino.meta.RoleEntity; import com.datastrato.gravitino.meta.UserEntity; import com.datastrato.gravitino.storage.IdGenerator; import com.datastrato.gravitino.utils.PrincipalUtils; @@ -36,8 +35,6 @@ import java.io.IOException; import java.time.Instant; import java.util.Collections; -import java.util.List; -import java.util.stream.Collectors; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -52,12 +49,10 @@ class UserGroupManager { private final EntityStore store; private final IdGenerator idGenerator; - private final RoleManager roleManager; - UserGroupManager(EntityStore store, IdGenerator idGenerator, RoleManager roleManager) { + UserGroupManager(EntityStore store, IdGenerator idGenerator) { this.store = store; this.idGenerator = idGenerator; - this.roleManager = roleManager; } User addUser(String metalake, String name) throws UserAlreadyExistsException { @@ -102,20 +97,9 @@ boolean removeUser(String metalake, String user) { User getUser(String metalake, String user) throws NoSuchUserException { try { AuthorizationUtils.checkMetalakeExists(metalake); - UserEntity entity = - store.get( - AuthorizationUtils.ofUser(metalake, user), Entity.EntityType.USER, UserEntity.class); + return store.get( + AuthorizationUtils.ofUser(metalake, user), Entity.EntityType.USER, UserEntity.class); - List roleEntities = - roleManager.getValidRoles(metalake, entity.roles(), entity.roleIds()); - - return UserEntity.builder() - .withId(entity.id()) - .withName(entity.name()) - .withAuditInfo(entity.auditInfo()) - .withNamespace(entity.namespace()) - .withRoleNames(roleEntities.stream().map(RoleEntity::name).collect(Collectors.toList())) - .build(); } catch (NoSuchEntityException e) { LOG.warn("User {} does not exist in the metalake {}", user, metalake, e); throw new NoSuchUserException(AuthorizationUtils.USER_DOES_NOT_EXIST_MSG, user, metalake); @@ -171,22 +155,8 @@ Group getGroup(String metalake, String group) { try { AuthorizationUtils.checkMetalakeExists(metalake); - GroupEntity entity = - store.get( - AuthorizationUtils.ofGroup(metalake, group), - Entity.EntityType.GROUP, - GroupEntity.class); - - List roleEntities = - roleManager.getValidRoles(metalake, entity.roles(), entity.roleIds()); - - return GroupEntity.builder() - .withId(entity.id()) - .withName(entity.name()) - .withAuditInfo(entity.auditInfo()) - .withNamespace(entity.namespace()) - .withRoleNames(roleEntities.stream().map(RoleEntity::name).collect(Collectors.toList())) - .build(); + return store.get( + AuthorizationUtils.ofGroup(metalake, group), Entity.EntityType.GROUP, GroupEntity.class); } catch (NoSuchEntityException e) { LOG.warn("Group {} does not exist in the metalake {}", group, metalake, e); throw new NoSuchGroupException(AuthorizationUtils.GROUP_DOES_NOT_EXIST_MSG, group, metalake); diff --git a/core/src/test/java/com/datastrato/gravitino/authorization/TestAccessControlManagerForPermissions.java b/core/src/test/java/com/datastrato/gravitino/authorization/TestAccessControlManagerForPermissions.java index aaa46c78bda..27c27395c63 100644 --- a/core/src/test/java/com/datastrato/gravitino/authorization/TestAccessControlManagerForPermissions.java +++ b/core/src/test/java/com/datastrato/gravitino/authorization/TestAccessControlManagerForPermissions.java @@ -141,7 +141,7 @@ public void testGrantRoleToUser() { String notExist = "not-exist"; User user = accessControlManager.getUser(METALAKE, USER); - Assertions.assertTrue(user.roles().isEmpty()); + Assertions.assertNull(user.roles()); user = accessControlManager.grantRolesToUser(METALAKE, ROLE, USER); Assertions.assertFalse(user.roles().isEmpty()); @@ -275,38 +275,4 @@ public void testRevokeRoleFormGroup() { NoSuchGroupException.class, () -> accessControlManager.revokeRolesFromGroup(METALAKE, ROLE, notExist)); } - - @Test - public void testDropRole() throws IOException { - String anotherRole = "anotherRole"; - - RoleEntity roleEntity = - RoleEntity.builder() - .withNamespace( - Namespace.of( - METALAKE, Entity.SYSTEM_CATALOG_RESERVED_NAME, Entity.ROLE_SCHEMA_NAME)) - .withId(1L) - .withName(anotherRole) - .withProperties(Maps.newHashMap()) - .withSecurableObjects( - Lists.newArrayList( - SecurableObjects.ofCatalog( - CATALOG, Lists.newArrayList(Privileges.UseCatalog.allow())))) - .withAuditInfo(auditInfo) - .build(); - - entityStore.put(roleEntity, true); - - User user = - accessControlManager.grantRolesToUser(METALAKE, Lists.newArrayList(anotherRole), USER); - Assertions.assertFalse(user.roles().isEmpty()); - - Group group = - accessControlManager.grantRolesToGroup(METALAKE, Lists.newArrayList(anotherRole), GROUP); - Assertions.assertFalse(group.roles().isEmpty()); - - Assertions.assertTrue(accessControlManager.deleteRole(METALAKE, anotherRole)); - group = accessControlManager.getGroup(METALAKE, GROUP); - Assertions.assertTrue(group.roles().isEmpty()); - } } diff --git a/core/src/test/java/com/datastrato/gravitino/storage/relational/service/TestGroupMetaService.java b/core/src/test/java/com/datastrato/gravitino/storage/relational/service/TestGroupMetaService.java index 3d55f8002e9..4d98103e25d 100644 --- a/core/src/test/java/com/datastrato/gravitino/storage/relational/service/TestGroupMetaService.java +++ b/core/src/test/java/com/datastrato/gravitino/storage/relational/service/TestGroupMetaService.java @@ -559,6 +559,12 @@ void updateGroup() throws IOException { Sets.newHashSet(role1.id(), role4.id()), Sets.newHashSet(noUpdaterGroup.roleIds())); Assertions.assertEquals("creator", noUpdaterGroup.auditInfo().creator()); Assertions.assertEquals("grantRevokeUser", noUpdaterGroup.auditInfo().lastModifier()); + + // Delete a role, the group entity won't contain this role. + RoleMetaService.getInstance().deleteRole(role1.nameIdentifier()); + GroupEntity groupEntity = + GroupMetaService.getInstance().getGroupByIdentifier(group1.nameIdentifier()); + Assertions.assertEquals(Sets.newHashSet("role4"), Sets.newHashSet(groupEntity.roleNames())); } @Test diff --git a/core/src/test/java/com/datastrato/gravitino/storage/relational/service/TestUserMetaService.java b/core/src/test/java/com/datastrato/gravitino/storage/relational/service/TestUserMetaService.java index 008b1eea483..fb5216801f1 100644 --- a/core/src/test/java/com/datastrato/gravitino/storage/relational/service/TestUserMetaService.java +++ b/core/src/test/java/com/datastrato/gravitino/storage/relational/service/TestUserMetaService.java @@ -557,6 +557,12 @@ void updateUser() throws IOException { Sets.newHashSet(role1.id(), role4.id()), Sets.newHashSet(noUpdaterUser.roleIds())); Assertions.assertEquals("creator", noUpdaterUser.auditInfo().creator()); Assertions.assertEquals("grantRevokeUser", noUpdaterUser.auditInfo().lastModifier()); + + // Delete a role, the user entity won't contain this role. + RoleMetaService.getInstance().deleteRole(role1.nameIdentifier()); + UserEntity userEntity = + UserMetaService.getInstance().getUserByIdentifier(user1.nameIdentifier()); + Assertions.assertEquals(Sets.newHashSet("role4"), Sets.newHashSet(userEntity.roleNames())); } @Test From 58762ecd6fab35c35ad53f736be774c89b7e013a Mon Sep 17 00:00:00 2001 From: Rory Date: Thu, 11 Jul 2024 10:17:17 +0800 Subject: [PATCH 2/2] Use name instead of id --- .../datastrato/gravitino/authorization/PermissionManager.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/com/datastrato/gravitino/authorization/PermissionManager.java b/core/src/main/java/com/datastrato/gravitino/authorization/PermissionManager.java index 9c5c91b46b1..95a59c18c9c 100644 --- a/core/src/main/java/com/datastrato/gravitino/authorization/PermissionManager.java +++ b/core/src/main/java/com/datastrato/gravitino/authorization/PermissionManager.java @@ -77,7 +77,7 @@ User grantRolesToUser(String metalake, List roles, String user) { List roleIds = Lists.newArrayList(toRoleIds(roleEntities)); for (RoleEntity roleEntityToGrant : roleEntitiesToGrant) { - if (roleNames.contains(roleEntityToGrant.name())) { + if (roleIds.contains(roleEntityToGrant.id())) { LOG.warn( "Failed to grant, role {} already exists in the user {} of metalake {}", roleEntityToGrant.name(), @@ -142,7 +142,7 @@ Group grantRolesToGroup(String metalake, List roles, String group) { List roleIds = Lists.newArrayList(toRoleIds(roleEntities)); for (RoleEntity roleEntityToGrant : roleEntitiesToGrant) { - if (roleNames.contains(roleEntityToGrant.name())) { + if (roleIds.contains(roleEntityToGrant.id())) { LOG.warn( "Failed to grant, role {} already exists in the group {} of metalake {}", roleEntityToGrant.name(),