Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#4105] improvement(core): Remove the logic of getValidRoles #4121

Merged
merged 2 commits into from
Jul 12, 2024
Merged

[#4105] improvement(core): Remove the logic of getValidRoles #4121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eb73cad
[#4105] improvement: Remove the logic of getValidRoles
jerqi Jul 10, 2024
58762ec
Use name instead of id
jerqi Jul 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/src/main/java/com/datastrato/gravitino/authorization/AccessControlManager.java
View file
Open in desktop
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
Open in desktop
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));
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest that we don't store the role name in user entity, only store the id, and we don't need to check role name, but the role id.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't store role name in the underlying relational database. It's necessary for us to assign the role names fields in the user entity. The updated user entity will be returned to end user. The users of requests need role names for human understanding.

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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading