Skip to content

Commit

Permalink
[#4105] improvement(core): Remove the logic of getValidRoles (#4121)
Browse files Browse the repository at this point in the history 
### 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.
  • Loading branch information
@jerqi
jerqi authored Jul 12, 2024
1 parent b80be6e commit d613544
Show file tree
Hide file tree
Showing 7 changed files with 46 additions and 108 deletions.
2 changes: 1 addition & 1 deletion core/src/main/java/com/datastrato/gravitino/authorization/AccessControlManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
39 changes: 27 additions & 12 deletions core/src/main/java/com/datastrato/gravitino/authorization/PermissionManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -67,14 +67,17 @@ User grantRolesToUser(String metalake, List<String> roles, String user) {
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(),
Expand Down Expand Up @@ -129,13 +132,17 @@ Group grantRolesToGroup(String metalake, List<String> roles, String group) {
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(),
Expand Down Expand Up @@ -190,8 +197,12 @@ Group revokeRolesFromGroup(String metalake, List<String> roles, String group) {
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));

Expand Down Expand Up @@ -252,8 +263,12 @@ User revokeRolesFromUser(String metalake, List<String> roles, String user) {
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));
Expand Down
25 changes: 0 additions & 25 deletions core/src/main/java/com/datastrato/gravitino/authorization/RoleManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -161,28 +160,4 @@ private RoleEntity getRoleEntity(NameIdentifier identifier) {
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;
}
}
40 changes: 5 additions & 35 deletions core/src/main/java/com/datastrato/gravitino/authorization/UserGroupManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,13 @@
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;
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;

Expand All @@ -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 {
Expand Down Expand Up @@ -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<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);
Expand Down Expand Up @@ -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<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);
Expand Down
36 changes: 1 addition & 35 deletions ...t/java/com/datastrato/gravitino/authorization/TestAccessControlManagerForPermissions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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());
}
}
6 changes: 6 additions & 0 deletions ...c/test/java/com/datastrato/gravitino/storage/relational/service/TestGroupMetaService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 6 additions & 0 deletions ...rc/test/java/com/datastrato/gravitino/storage/relational/service/TestUserMetaService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit d613544

Please sign in to comment.