This is an automated email from the ASF dual-hosted git repository.
jshao 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 d6135447a [#4105] improvement(core): Remove the logic of getValidRoles
(#4121)
d6135447a is described below
commit d6135447af900e15954b21d6ccf7637d66237237
Author: roryqi <[email protected]>
AuthorDate: Fri Jul 12 16:27:05 2024 +0800
[#4105] improvement(core): Remove the logic of getValidRoles (#4121)
### What changes were proposed in this pull request?
Remove the logic of getValidRoles.
### Why are the changes needed?
Fix: #4105
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Modify some test cases.
---
.../authorization/AccessControlManager.java | 2 +-
.../gravitino/authorization/PermissionManager.java | 39 ++++++++++++++-------
.../gravitino/authorization/RoleManager.java | 25 --------------
.../gravitino/authorization/UserGroupManager.java | 40 +++-------------------
.../TestAccessControlManagerForPermissions.java | 36 +------------------
.../relational/service/TestGroupMetaService.java | 6 ++++
.../relational/service/TestUserMetaService.java | 6 ++++
7 files changed, 46 insertions(+), 108 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 eb7dbfb04..26ec14a7e 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 3b24e8cde..95a59c18c 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 @@ import org.slf4j.LoggerFactory;
/**
* 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,14 +67,17 @@ class PermissionManager {
UserEntity.class,
Entity.EntityType.USER,
userEntity -> {
- List<RoleEntity> roleEntities =
- roleManager.getValidRoles(metalake, userEntity.roleNames(),
userEntity.roleIds());
-
+ List<RoleEntity> roleEntities = Lists.newArrayList();
+ if (userEntity.roleNames() != null) {
+ for (String role : userEntity.roleNames()) {
+ roleEntities.add(roleManager.getRole(metalake, role));
+ }
+ }
List<String> roleNames =
Lists.newArrayList(toRoleNames(roleEntities));
List<Long> 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(),
@@ -129,13 +132,17 @@ class PermissionManager {
GroupEntity.class,
Entity.EntityType.GROUP,
groupEntity -> {
- List<RoleEntity> roleEntities =
- roleManager.getValidRoles(metalake, groupEntity.roleNames(),
groupEntity.roleIds());
+ List<RoleEntity> roleEntities = Lists.newArrayList();
+ if (groupEntity.roleNames() != null) {
+ for (String role : groupEntity.roleNames()) {
+ roleEntities.add(roleManager.getRole(metalake, role));
+ }
+ }
List<String> roleNames =
Lists.newArrayList(toRoleNames(roleEntities));
List<Long> 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(),
@@ -190,8 +197,12 @@ class PermissionManager {
GroupEntity.class,
Entity.EntityType.GROUP,
groupEntity -> {
- List<RoleEntity> roleEntities =
- roleManager.getValidRoles(metalake, groupEntity.roleNames(),
groupEntity.roleIds());
+ List<RoleEntity> roleEntities = Lists.newArrayList();
+ if (groupEntity.roleNames() != null) {
+ for (String role : groupEntity.roleNames()) {
+ roleEntities.add(roleManager.getRole(metalake, role));
+ }
+ }
List<String> roleNames =
Lists.newArrayList(toRoleNames(roleEntities));
List<Long> roleIds = Lists.newArrayList(toRoleIds(roleEntities));
@@ -252,8 +263,12 @@ class PermissionManager {
UserEntity.class,
Entity.EntityType.USER,
userEntity -> {
- List<RoleEntity> roleEntities =
- roleManager.getValidRoles(metalake, userEntity.roleNames(),
userEntity.roleIds());
+ List<RoleEntity> roleEntities = Lists.newArrayList();
+ if (userEntity.roleNames() != null) {
+ for (String role : userEntity.roleNames()) {
+ roleEntities.add(roleManager.getRole(metalake, role));
+ }
+ }
List<String> roleNames =
Lists.newArrayList(toRoleNames(roleEntities));
List<Long> 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 96fc71702..9d717b4eb 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.Cache;
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 @@ class RoleManager {
Cache<NameIdentifier, RoleEntity> getCache() {
return cache;
}
-
- List<RoleEntity> getValidRoles(String metalake, List<String> roleNames,
List<Long> roleIds) {
- List<RoleEntity> 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 c21b060a3..b92ab7d5a 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.NoSuchUserException;
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 com.google.common.collect.Lists;
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 @@ class UserGroupManager {
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<RoleEntity> 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 @@ class UserGroupManager {
try {
AuthorizationUtils.checkMetalakeExists(metalake);
- GroupEntity entity =
- store.get(
- AuthorizationUtils.ofGroup(metalake, group),
- Entity.EntityType.GROUP,
- GroupEntity.class);
-
- List<RoleEntity> 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 aaa46c78b..27c27395c 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 class TestAccessControlManagerForPermissions {
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 class TestAccessControlManagerForPermissions {
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 3d55f8002..4d98103e2 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 @@ class TestGroupMetaService extends TestJDBCBackend {
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 008b1eea4..fb5216801 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 @@ class TestUserMetaService extends TestJDBCBackend {
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
