Skip to content

Commit

Permalink
[apache#4105] improvement: Remove the logic of getValidRoles
Browse files Browse the repository at this point in the history
  • Loading branch information
@jerqi
jerqi committed Jul 10, 2024
1 parent 9e80cbc commit be9cea6
Show file tree
Hide file tree
Showing 7 changed files with 43 additions and 105 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
33 changes: 24 additions & 9 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 @@ -67,9 +67,12 @@ 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));

Expand Down Expand Up @@ -129,8 +132,12 @@ 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));

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 be9cea6

Please sign in to comment.