Skip to content

Commit

Permalink
[#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 3d9229d
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 70 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
25 changes: 16 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,10 @@ 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();
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 +130,10 @@ 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();
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 +193,10 @@ 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();
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 +257,10 @@ 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();
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

0 comments on commit 3d9229d

Please sign in to comment.