diff --git a/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerBaseE2EIT.java b/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerBaseE2EIT.java index 919551bd922..d3b158fb1f1 100644 --- a/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerBaseE2EIT.java +++ b/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerBaseE2EIT.java @@ -21,6 +21,7 @@ import static org.apache.gravitino.authorization.ranger.integration.test.RangerITEnv.currentFunName; import com.google.common.collect.Lists; +import com.google.common.collect.Sets; import java.io.File; import java.io.IOException; import java.nio.charset.StandardCharsets; @@ -905,7 +906,7 @@ void testAllowUseSchemaPrivilege() throws InterruptedException { MetadataObject catalogObject = MetadataObjects.of(null, catalogName, MetadataObject.Type.CATALOG); metalake.revokePrivilegesFromRole( - roleName, catalogObject, Lists.newArrayList(Privileges.CreateSchema.allow())); + roleName, catalogObject, Sets.newHashSet(Privileges.CreateSchema.allow())); waitForUpdatingPolicies(); // Use Spark to show this databases(schema) @@ -920,7 +921,7 @@ void testAllowUseSchemaPrivilege() throws InterruptedException { MetadataObject schemaObject = MetadataObjects.of(catalogName, schemaName, MetadataObject.Type.SCHEMA); metalake.grantPrivilegesToRole( - roleName, schemaObject, Lists.newArrayList(Privileges.UseSchema.allow())); + roleName, schemaObject, Sets.newHashSet(Privileges.UseSchema.allow())); waitForUpdatingPolicies(); // Use Spark to show this databases(schema) again @@ -1020,7 +1021,7 @@ void testGrantPrivilegesForMetalake() throws InterruptedException { metalake.grantPrivilegesToRole( roleName, MetadataObjects.of(null, metalakeName, MetadataObject.Type.METALAKE), - Lists.newArrayList(Privileges.CreateSchema.allow())); + Sets.newHashSet(Privileges.CreateSchema.allow())); // Fail to create a schema Assertions.assertThrows( diff --git a/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerFilesetIT.java b/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerFilesetIT.java index ab74b0449ae..0b1112278ce 100644 --- a/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerFilesetIT.java +++ b/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerFilesetIT.java @@ -28,6 +28,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; import com.google.common.collect.Maps; +import com.google.common.collect.Sets; import java.io.IOException; import java.security.PrivilegedExceptionAction; import java.util.Arrays; @@ -262,7 +263,7 @@ void testReadWritePath() throws IOException, RangerServiceException { String.format("%s.%s", catalogName, schemaName), fileset.name(), MetadataObject.Type.FILESET), - Lists.newArrayList(Privileges.WriteFileset.allow())); + Sets.newHashSet(Privileges.WriteFileset.allow())); policies = rangerClient.getPoliciesInService(RangerITEnv.RANGER_HDFS_REPO_NAME); Assertions.assertEquals(1, policies.size()); @@ -320,7 +321,7 @@ void testReadWritePath() throws IOException, RangerServiceException { String.format("%s.%s", catalogName, schemaName), fileset.name(), MetadataObject.Type.FILESET), - Lists.newArrayList(Privileges.ReadFileset.allow(), Privileges.WriteFileset.allow())); + Sets.newHashSet(Privileges.ReadFileset.allow(), Privileges.WriteFileset.allow())); policies = rangerClient.getPoliciesInService(RangerITEnv.RANGER_HDFS_REPO_NAME); Assertions.assertEquals(1, policies.size()); Assertions.assertEquals(3, policies.get(0).getPolicyItems().size()); @@ -460,7 +461,7 @@ void testReadWritePathE2E() throws IOException, RangerServiceException, Interrup fileset.name(), MetadataObject.Type.FILESET); metalake.grantPrivilegesToRole( - filesetRole, filesetObject, Lists.newArrayList(Privileges.WriteFileset.allow())); + filesetRole, filesetObject, Sets.newHashSet(Privileges.WriteFileset.allow())); RangerBaseE2EIT.waitForUpdatingPolicies(); UserGroupInformation.createProxyUser(userName, UserGroupInformation.getCurrentUser()) .doAs( @@ -488,7 +489,7 @@ void testReadWritePathE2E() throws IOException, RangerServiceException, Interrup metalake.revokePrivilegesFromRole( filesetRole, filesetObject, - Lists.newArrayList(Privileges.ReadFileset.allow(), Privileges.WriteFileset.allow())); + Sets.newHashSet(Privileges.ReadFileset.allow(), Privileges.WriteFileset.allow())); RangerBaseE2EIT.waitForUpdatingPolicies(); UserGroupInformation.createProxyUser(userName, UserGroupInformation.getCurrentUser()) .doAs( diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/GrantPrivilegesToRole.java b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/GrantPrivilegesToRole.java index 8630282ea60..b21a74ff48c 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/GrantPrivilegesToRole.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/GrantPrivilegesToRole.java @@ -19,8 +19,8 @@ package org.apache.gravitino.cli.commands; -import java.util.ArrayList; -import java.util.List; +import java.util.HashSet; +import java.util.Set; import org.apache.gravitino.MetadataObject; import org.apache.gravitino.authorization.Privilege; import org.apache.gravitino.cli.ErrorMessages; @@ -69,7 +69,7 @@ public GrantPrivilegesToRole( public void handle() { try { GravitinoClient client = buildClient(metalake); - List privilegesList = new ArrayList<>(); + Set privilegesSet = new HashSet<>(); for (String privilege : privileges) { if (!Privileges.isValid(privilege)) { @@ -81,11 +81,11 @@ public void handle() { .withName(Privileges.toName(privilege)) .withCondition(Privilege.Condition.ALLOW) .build(); - privilegesList.add(privilegeDTO); + privilegesSet.add(privilegeDTO); } MetadataObject metadataObject = constructMetadataObject(entity, client); - client.grantPrivilegesToRole(role, metadataObject, privilegesList); + client.grantPrivilegesToRole(role, metadataObject, privilegesSet); } catch (NoSuchMetalakeException err) { System.err.println(ErrorMessages.UNKNOWN_METALAKE); return; diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RevokePrivilegesFromRole.java b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RevokePrivilegesFromRole.java index 3bfa7cd4526..fbf273ce0d8 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RevokePrivilegesFromRole.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RevokePrivilegesFromRole.java @@ -19,8 +19,8 @@ package org.apache.gravitino.cli.commands; -import java.util.ArrayList; -import java.util.List; +import java.util.HashSet; +import java.util.Set; import org.apache.gravitino.MetadataObject; import org.apache.gravitino.authorization.Privilege; import org.apache.gravitino.cli.ErrorMessages; @@ -69,7 +69,7 @@ public RevokePrivilegesFromRole( public void handle() { try { GravitinoClient client = buildClient(metalake); - List privilegesList = new ArrayList<>(); + Set privilegesSet = new HashSet<>(); for (String privilege : privileges) { if (!Privileges.isValid(privilege)) { @@ -81,11 +81,11 @@ public void handle() { .withName(Privileges.toName(privilege)) .withCondition(Privilege.Condition.DENY) .build(); - privilegesList.add(privilegeDTO); + privilegesSet.add(privilegeDTO); } MetadataObject metadataObject = constructMetadataObject(entity, client); - client.revokePrivilegesFromRole(role, metadataObject, privilegesList); + client.revokePrivilegesFromRole(role, metadataObject, privilegesSet); } catch (NoSuchMetalakeException err) { System.err.println(ErrorMessages.UNKNOWN_METALAKE); return; diff --git a/clients/client-java/src/main/java/org/apache/gravitino/client/DTOConverters.java b/clients/client-java/src/main/java/org/apache/gravitino/client/DTOConverters.java index 560dae06d1f..c9a88239f57 100644 --- a/clients/client-java/src/main/java/org/apache/gravitino/client/DTOConverters.java +++ b/clients/client-java/src/main/java/org/apache/gravitino/client/DTOConverters.java @@ -20,6 +20,7 @@ import static org.apache.gravitino.dto.util.DTOConverters.toFunctionArg; +import java.util.Collection; import java.util.List; import java.util.stream.Collectors; import org.apache.gravitino.Catalog; @@ -321,7 +322,7 @@ static SecurableObjectDTO toSecurableObject(SecurableObject securableObject) { .build(); } - static List toPrivileges(List privileges) { + static List toPrivileges(Collection privileges) { return privileges.stream() .map( privilege -> diff --git a/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoClient.java b/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoClient.java index c0310f23873..fb2a990891f 100644 --- a/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoClient.java +++ b/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoClient.java @@ -20,9 +20,11 @@ package org.apache.gravitino.client; import com.google.common.base.Preconditions; +import com.google.common.collect.Sets; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; import org.apache.gravitino.Catalog; import org.apache.gravitino.CatalogChange; import org.apache.gravitino.MetadataObject; @@ -420,12 +422,33 @@ public String[] listRoleNames() throws NoSuchMetalakeException { * @throws IllegalPrivilegeException If any privilege can't be bind to the metadata object. * @throws RuntimeException If granting roles to a role encounters storage issues. */ + @Deprecated public Role grantPrivilegesToRole(String role, MetadataObject object, List privileges) throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException, IllegalPrivilegeException { return getMetalake().grantPrivilegesToRole(role, object, privileges); } + /** + * Grant privileges to a role. + * + * @param role The name of the role. + * @param privileges The privileges to grant. + * @param object The object is associated with privileges to grant. + * @return The role after granted. + * @throws NoSuchRoleException If the role with the given name does not exist. + * @throws NoSuchMetadataObjectException If the metadata object with the given name does not + * exist. + * @throws NoSuchMetalakeException If the Metalake with the given name does not exist. + * @throws IllegalPrivilegeException If any privilege can't be bind to the metadata object. + * @throws RuntimeException If granting roles to a role encounters storage issues. + */ + public Role grantPrivilegesToRole(String role, MetadataObject object, Set privileges) + throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException, + IllegalPrivilegeException { + return getMetalake().grantPrivilegesToRole(role, object, privileges); + } + /** * Revoke privileges from a role. * @@ -440,10 +463,32 @@ public Role grantPrivilegesToRole(String role, MetadataObject object, List privileges) throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException, IllegalPrivilegeException { + return getMetalake().revokePrivilegesFromRole(role, object, Sets.newHashSet(privileges)); + } + + /** + * Revoke privileges from a role. + * + * @param role The name of the role. + * @param privileges The privileges to revoke. + * @param object The object is associated with privileges to revoke. + * @return The role after revoked. + * @throws NoSuchRoleException If the role with the given name does not exist. + * @throws NoSuchMetadataObjectException If the metadata object with the given name does not + * exist. + * @throws NoSuchMetalakeException If the Metalake with the given name does not exist. + * @throws IllegalPrivilegeException If any privilege can't be bind to the metadata object. + * @throws RuntimeException If revoking privileges from a role encounters storage issues. + */ + public Role revokePrivilegesFromRole( + String role, MetadataObject object, Set privileges) + throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException, + IllegalPrivilegeException { return getMetalake().revokePrivilegesFromRole(role, object, privileges); } diff --git a/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoMetalake.java b/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoMetalake.java index 14e17a851e1..60bb47adac7 100644 --- a/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoMetalake.java +++ b/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoMetalake.java @@ -20,6 +20,7 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Sets; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; @@ -27,6 +28,7 @@ import java.util.Locale; import java.util.Map; import java.util.Optional; +import java.util.Set; import java.util.stream.Collectors; import org.apache.commons.lang3.StringUtils; import org.apache.gravitino.Catalog; @@ -1018,6 +1020,27 @@ public Group revokeRolesFromGroup(List roles, String group) public Role grantPrivilegesToRole(String role, MetadataObject object, List privileges) throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException, IllegalPrivilegeException { + Set privilegeSet = Sets.newHashSet(privileges); + return grantPrivilegesToRole(role, object, privilegeSet); + } + + /** + * Grant privileges to a role. + * + * @param role The name of the role. + * @param privileges The privileges to grant. + * @param object The object is associated with privileges to grant. + * @return The role after granted. + * @throws NoSuchRoleException If the role with the given name does not exist. + * @throws NoSuchMetadataObjectException If the metadata object with the given name does not + * exist. + * @throws NoSuchMetalakeException If the Metalake with the given name does not exist. + * @throws IllegalPrivilegeException If any privilege can't be bind to the metadata object. + * @throws RuntimeException If granting privileges to a role encounters storage issues. + */ + public Role grantPrivilegesToRole(String role, MetadataObject object, Set privileges) + throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException, + IllegalPrivilegeException { PrivilegeGrantRequest request = new PrivilegeGrantRequest(DTOConverters.toPrivileges(privileges)); request.validate(); @@ -1056,10 +1079,33 @@ public Role grantPrivilegesToRole(String role, MetadataObject object, List privileges) throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException, IllegalPrivilegeException { + Set privilegeSet = Sets.newHashSet(privileges); + return revokePrivilegesFromRole(role, object, privilegeSet); + } + + /** + * Revoke privileges from a role. + * + * @param role The name of the role. + * @param privileges The privileges to revoke. + * @param object The object is associated with privileges to revoke. + * @return The role after revoked. + * @throws NoSuchRoleException If the role with the given name does not exist. + * @throws NoSuchMetadataObjectException If the metadata object with the given name does not + * exist. + * @throws NoSuchMetalakeException If the Metalake with the given name does not exist. + * @throws IllegalPrivilegeException If any privilege can't be bind to the metadata object. + * @throws RuntimeException If revoking privileges from a role encounters storage issues. + */ + public Role revokePrivilegesFromRole( + String role, MetadataObject object, Set privileges) + throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException, + IllegalPrivilegeException { PrivilegeRevokeRequest request = new PrivilegeRevokeRequest(DTOConverters.toPrivileges(privileges)); request.validate(); diff --git a/clients/client-java/src/test/java/org/apache/gravitino/client/TestPermission.java b/clients/client-java/src/test/java/org/apache/gravitino/client/TestPermission.java index 006a47b43ca..d1731607a80 100644 --- a/clients/client-java/src/test/java/org/apache/gravitino/client/TestPermission.java +++ b/clients/client-java/src/test/java/org/apache/gravitino/client/TestPermission.java @@ -22,6 +22,7 @@ import static org.apache.hc.core5.http.HttpStatus.SC_SERVER_ERROR; import com.google.common.collect.Lists; +import com.google.common.collect.Sets; import java.time.Instant; import java.util.List; import org.apache.gravitino.MetadataObject; @@ -231,7 +232,7 @@ public void testGrantPrivilegeToRole() throws Exception { MetadataObject object = MetadataObjects.of(null, metalakeName, MetadataObject.Type.METALAKE); Role grantedRole = gravitinoClient.grantPrivilegesToRole( - role, object, Lists.newArrayList(Privileges.CreateTable.allow())); + role, object, Sets.newHashSet(Privileges.CreateTable.allow())); Assertions.assertEquals(grantedRole.name(), role); Assertions.assertEquals(1, grantedRole.securableObjects().size()); SecurableObject securableObject = grantedRole.securableObjects().get(0); @@ -249,7 +250,7 @@ public void testGrantPrivilegeToRole() throws Exception { RuntimeException.class, () -> gravitinoClient.grantPrivilegesToRole( - role, object, Lists.newArrayList(Privileges.CreateTable.allow()))); + role, object, Sets.newHashSet(Privileges.CreateTable.allow()))); } @Test @@ -280,7 +281,7 @@ public void testRevokePrivilegeFromRole() throws Exception { MetadataObject object = MetadataObjects.of(null, metalakeName, MetadataObject.Type.METALAKE); Role revokedRole = gravitinoClient.revokePrivilegesFromRole( - role, object, Lists.newArrayList(Privileges.CreateTable.allow())); + role, object, Sets.newHashSet(Privileges.CreateTable.allow())); Assertions.assertEquals(revokedRole.name(), role); Assertions.assertTrue(revokedRole.securableObjects().isEmpty()); @@ -291,6 +292,6 @@ public void testRevokePrivilegeFromRole() throws Exception { RuntimeException.class, () -> gravitinoClient.revokePrivilegesFromRole( - role, object, Lists.newArrayList(Privileges.CreateTable.allow()))); + role, object, Sets.newHashSet(Privileges.CreateTable.allow()))); } } diff --git a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java index 268ed20f3ce..07232e8a8d4 100644 --- a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java +++ b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java @@ -20,6 +20,7 @@ import com.google.common.collect.Lists; import com.google.common.collect.Maps; +import com.google.common.collect.Sets; import java.util.Arrays; import java.util.Collections; import java.util.Comparator; @@ -468,7 +469,7 @@ void testManageRolePermissions() { // grant a privilege Role role = metalake.grantPrivilegesToRole( - roleName, metadataObject, Lists.newArrayList(Privileges.CreateCatalog.allow())); + roleName, metadataObject, Sets.newHashSet(Privileges.CreateCatalog.allow())); Assertions.assertEquals(1, role.securableObjects().size()); // grant a wrong privilege @@ -477,7 +478,7 @@ void testManageRolePermissions() { IllegalPrivilegeException.class, () -> metalake.grantPrivilegesToRole( - roleName, catalog, Lists.newArrayList(Privileges.CreateCatalog.allow()))); + roleName, catalog, Sets.newHashSet(Privileges.CreateCatalog.allow()))); // grant a wrong catalog type privilege MetadataObject wrongCatalog = @@ -486,7 +487,7 @@ void testManageRolePermissions() { IllegalPrivilegeException.class, () -> metalake.grantPrivilegesToRole( - roleName, wrongCatalog, Lists.newArrayList(Privileges.SelectTable.allow()))); + roleName, wrongCatalog, Sets.newHashSet(Privileges.SelectTable.allow()))); // grant a duplicated privilege MetadataObject duplicatedCatalog = @@ -497,12 +498,23 @@ void testManageRolePermissions() { metalake.grantPrivilegesToRole( roleName, duplicatedCatalog, - Lists.newArrayList(Privileges.SelectTable.allow(), Privileges.SelectTable.deny()))); + Sets.newHashSet(Privileges.ReadFileset.allow(), Privileges.ReadFileset.deny()))); + + // repeat to grant a privilege + metalake.grantPrivilegesToRole( + roleName, duplicatedCatalog, Sets.newHashSet(Privileges.ReadFileset.allow())); + Assertions.assertThrows( + IllegalPrivilegeException.class, + () -> + metalake.grantPrivilegesToRole( + roleName, duplicatedCatalog, Sets.newHashSet(Privileges.ReadFileset.deny()))); + metalake.revokePrivilegesFromRole( + roleName, duplicatedCatalog, Sets.newHashSet(Privileges.ReadFileset.allow())); // repeat to grant a privilege role = metalake.grantPrivilegesToRole( - roleName, metadataObject, Lists.newArrayList(Privileges.CreateCatalog.allow())); + roleName, metadataObject, Sets.newHashSet(Privileges.CreateCatalog.allow())); Assertions.assertEquals(1, role.securableObjects().size()); // grant a not-existing role @@ -510,12 +522,12 @@ void testManageRolePermissions() { NoSuchRoleException.class, () -> metalake.grantPrivilegesToRole( - "not-exist", metadataObject, Lists.newArrayList(Privileges.CreateCatalog.allow()))); + "not-exist", metadataObject, Sets.newHashSet(Privileges.CreateCatalog.allow()))); // revoke a privilege role = metalake.revokePrivilegesFromRole( - roleName, metadataObject, Lists.newArrayList(Privileges.CreateCatalog.allow())); + roleName, metadataObject, Sets.newHashSet(Privileges.CreateCatalog.allow())); Assertions.assertTrue(role.securableObjects().isEmpty()); // revoke a wrong privilege @@ -523,19 +535,19 @@ void testManageRolePermissions() { IllegalPrivilegeException.class, () -> metalake.revokePrivilegesFromRole( - roleName, catalog, Lists.newArrayList(Privileges.CreateCatalog.allow()))); + roleName, catalog, Sets.newHashSet(Privileges.CreateCatalog.allow()))); // revoke a wrong catalog type privilege Assertions.assertThrows( IllegalPrivilegeException.class, () -> metalake.revokePrivilegesFromRole( - roleName, wrongCatalog, Lists.newArrayList(Privileges.SelectTable.allow()))); + roleName, wrongCatalog, Sets.newHashSet(Privileges.SelectTable.allow()))); // repeat to revoke a privilege role = metalake.revokePrivilegesFromRole( - roleName, metadataObject, Lists.newArrayList(Privileges.CreateCatalog.allow())); + roleName, metadataObject, Sets.newHashSet(Privileges.CreateCatalog.allow())); Assertions.assertTrue(role.securableObjects().isEmpty()); // revoke a not-existing role @@ -543,7 +555,7 @@ void testManageRolePermissions() { NoSuchRoleException.class, () -> metalake.revokePrivilegesFromRole( - "not-exist", metadataObject, Lists.newArrayList(Privileges.CreateCatalog.allow()))); + "not-exist", metadataObject, Sets.newHashSet(Privileges.CreateCatalog.allow()))); // Cleanup metalake.deleteRole(roleName); diff --git a/core/src/main/java/org/apache/gravitino/authorization/AccessControlDispatcher.java b/core/src/main/java/org/apache/gravitino/authorization/AccessControlDispatcher.java index b75a67055b4..9aa2b3f52bf 100644 --- a/core/src/main/java/org/apache/gravitino/authorization/AccessControlDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/authorization/AccessControlDispatcher.java @@ -20,6 +20,7 @@ import java.util.List; import java.util.Map; +import java.util.Set; import org.apache.gravitino.MetadataObject; import org.apache.gravitino.exceptions.GroupAlreadyExistsException; import org.apache.gravitino.exceptions.IllegalRoleException; @@ -293,7 +294,7 @@ String[] listRoleNamesByObject(String metalake, MetadataObject object) * @throws RuntimeException If granting roles to a role encounters storage issues. */ Role grantPrivilegeToRole( - String metalake, String role, MetadataObject object, List privileges) + String metalake, String role, MetadataObject object, Set privileges) throws NoSuchGroupException, NoSuchRoleException; /** @@ -308,6 +309,6 @@ Role grantPrivilegeToRole( * @throws RuntimeException If revoking privileges from a role encounters storage issues. */ Role revokePrivilegesFromRole( - String metalake, String role, MetadataObject object, List privileges) + String metalake, String role, MetadataObject object, Set privileges) throws NoSuchMetalakeException, NoSuchRoleException; } diff --git a/core/src/main/java/org/apache/gravitino/authorization/AccessControlManager.java b/core/src/main/java/org/apache/gravitino/authorization/AccessControlManager.java index 798285806f5..2d756646b64 100644 --- a/core/src/main/java/org/apache/gravitino/authorization/AccessControlManager.java +++ b/core/src/main/java/org/apache/gravitino/authorization/AccessControlManager.java @@ -20,6 +20,7 @@ import java.util.List; import java.util.Map; +import java.util.Set; import org.apache.gravitino.Config; import org.apache.gravitino.Configs; import org.apache.gravitino.EntityStore; @@ -169,14 +170,14 @@ public String[] listRoleNamesByObject(String metalake, MetadataObject object) @Override public Role grantPrivilegeToRole( - String metalake, String role, MetadataObject object, List privileges) + String metalake, String role, MetadataObject object, Set privileges) throws NoSuchRoleException, NoSuchMetalakeException { return permissionManager.grantPrivilegesToRole(metalake, role, object, privileges); } @Override public Role revokePrivilegesFromRole( - String metalake, String role, MetadataObject object, List privileges) + String metalake, String role, MetadataObject object, Set privileges) throws NoSuchRoleException, NoSuchMetalakeException { return permissionManager.revokePrivilegesFromRole(metalake, role, object, privileges); } diff --git a/core/src/main/java/org/apache/gravitino/authorization/PermissionManager.java b/core/src/main/java/org/apache/gravitino/authorization/PermissionManager.java index bdaa8f6f74d..1046723d706 100644 --- a/core/src/main/java/org/apache/gravitino/authorization/PermissionManager.java +++ b/core/src/main/java/org/apache/gravitino/authorization/PermissionManager.java @@ -406,7 +406,7 @@ User revokeRolesFromUser(String metalake, List roles, String user) { } Role grantPrivilegesToRole( - String metalake, String role, MetadataObject object, List privileges) { + String metalake, String role, MetadataObject object, Set privileges) { try { AuthorizationPluginCallbackWrapper authorizationPluginCallbackWrapper = new AuthorizationPluginCallbackWrapper(); @@ -476,7 +476,7 @@ private static SecurableObject updateGrantedSecurableObject( String metalake, String role, MetadataObject object, - List privileges, + Set privileges, RoleEntity roleEntity, SecurableObject targetObject, AuthorizationPluginCallbackWrapper authorizationPluginCallbackWrapper) { @@ -489,7 +489,7 @@ private static SecurableObject updateGrantedSecurableObject( return targetObject; } else { updatePrivileges.addAll(privileges); - AuthorizationUtils.checkDuplicatedNamePrivilege(privileges); + AuthorizationUtils.checkDuplicatedNamePrivilege(updatePrivileges); SecurableObject newSecurableObject = SecurableObjects.parse( @@ -513,7 +513,7 @@ private static SecurableObject updateGrantedSecurableObject( } Role revokePrivilegesFromRole( - String metalake, String role, MetadataObject object, List privileges) { + String metalake, String role, MetadataObject object, Set privileges) { try { AuthorizationPluginCallbackWrapper authorizationCallbackWrapper = new AuthorizationPluginCallbackWrapper(); @@ -587,9 +587,11 @@ private static SecurableObject createNewSecurableObject( String metalake, String role, MetadataObject object, - List privileges, + Set privileges, RoleEntity roleEntity, AuthorizationPluginCallbackWrapper authorizationPluginCallbackWrapper) { + AuthorizationUtils.checkDuplicatedNamePrivilege(privileges); + // Add a new securable object if there doesn't exist the object in the role SecurableObject securableObject = SecurableObjects.parse(object.fullName(), object.type(), Lists.newArrayList(privileges)); @@ -613,7 +615,7 @@ private static SecurableObject updateRevokedSecurableObject( String metalake, String role, MetadataObject object, - List privileges, + Set privileges, RoleEntity roleEntity, SecurableObject targetObject, AuthorizationPluginCallbackWrapper authorizationCallbackWrapper) { diff --git a/core/src/main/java/org/apache/gravitino/hook/AccessControlHookDispatcher.java b/core/src/main/java/org/apache/gravitino/hook/AccessControlHookDispatcher.java index f5f5a27648e..dba0177ca12 100644 --- a/core/src/main/java/org/apache/gravitino/hook/AccessControlHookDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/hook/AccessControlHookDispatcher.java @@ -20,6 +20,7 @@ import java.util.List; import java.util.Map; +import java.util.Set; import org.apache.gravitino.Entity; import org.apache.gravitino.GravitinoEnv; import org.apache.gravitino.MetadataObject; @@ -188,14 +189,14 @@ public String[] listRoleNamesByObject(String metalake, MetadataObject object) @Override public Role grantPrivilegeToRole( - String metalake, String role, MetadataObject object, List privileges) + String metalake, String role, MetadataObject object, Set privileges) throws NoSuchMetalakeException, NoSuchRoleException { return dispatcher.grantPrivilegeToRole(metalake, role, object, privileges); } @Override public Role revokePrivilegesFromRole( - String metalake, String role, MetadataObject object, List privileges) + String metalake, String role, MetadataObject object, Set privileges) throws NoSuchMetalakeException, NoSuchRoleException { return dispatcher.revokePrivilegesFromRole(metalake, role, object, privileges); } diff --git a/core/src/test/java/org/apache/gravitino/authorization/TestAccessControlManagerForPermissions.java b/core/src/test/java/org/apache/gravitino/authorization/TestAccessControlManagerForPermissions.java index 30084a32e2d..e4b62567d48 100644 --- a/core/src/test/java/org/apache/gravitino/authorization/TestAccessControlManagerForPermissions.java +++ b/core/src/test/java/org/apache/gravitino/authorization/TestAccessControlManagerForPermissions.java @@ -24,6 +24,7 @@ import com.google.common.collect.Lists; import com.google.common.collect.Maps; +import com.google.common.collect.Sets; import java.io.IOException; import java.time.Instant; import java.util.List; @@ -355,7 +356,7 @@ public void testGrantPrivilegeToRole() { METALAKE, "grantedRole", MetadataObjects.of(null, METALAKE, MetadataObject.Type.METALAKE), - Lists.newArrayList(Privileges.CreateTable.allow())); + Sets.newHashSet(Privileges.CreateTable.allow())); List objects = role.securableObjects(); @@ -370,7 +371,7 @@ public void testGrantPrivilegeToRole() { METALAKE, "grantedRole", MetadataObjects.of(null, METALAKE, MetadataObject.Type.METALAKE), - Lists.newArrayList(Privileges.CreateTable.allow())); + Sets.newHashSet(Privileges.CreateTable.allow())); objects = role.securableObjects(); Assertions.assertEquals(2, objects.size()); @@ -383,7 +384,7 @@ public void testGrantPrivilegeToRole() { METALAKE, notExist, MetadataObjects.of(null, METALAKE, MetadataObject.Type.METALAKE), - Lists.newArrayList(Privileges.CreateTable.allow()))); + Sets.newHashSet(Privileges.CreateTable.allow()))); } @Test @@ -396,7 +397,7 @@ public void testRevokePrivilegeFromRole() { METALAKE, "revokedRole", MetadataObjects.of(null, CATALOG, MetadataObject.Type.CATALOG), - Lists.newArrayList(Privileges.UseCatalog.allow())); + Sets.newHashSet(Privileges.UseCatalog.allow())); // Test authorization plugin verify(authorizationPlugin).onRoleUpdated(any(), any()); @@ -411,7 +412,7 @@ public void testRevokePrivilegeFromRole() { METALAKE, "revokedRole", MetadataObjects.of(null, CATALOG, MetadataObject.Type.CATALOG), - Lists.newArrayList(Privileges.UseCatalog.allow())); + Sets.newHashSet(Privileges.UseCatalog.allow())); objects = role.securableObjects(); Assertions.assertTrue(objects.isEmpty()); @@ -423,6 +424,6 @@ public void testRevokePrivilegeFromRole() { METALAKE, notExist, MetadataObjects.of(null, METALAKE, MetadataObject.Type.METALAKE), - Lists.newArrayList(Privileges.CreateTable.allow()))); + Sets.newHashSet(Privileges.CreateTable.allow()))); } } diff --git a/server/src/main/java/org/apache/gravitino/server/web/rest/PermissionOperations.java b/server/src/main/java/org/apache/gravitino/server/web/rest/PermissionOperations.java index 3ce1517a46a..3f89cc7eeae 100644 --- a/server/src/main/java/org/apache/gravitino/server/web/rest/PermissionOperations.java +++ b/server/src/main/java/org/apache/gravitino/server/web/rest/PermissionOperations.java @@ -242,7 +242,7 @@ public Response grantPrivilegeToRole( object, privilegeGrantRequest.getPrivileges().stream() .map(DTOConverters::fromPrivilegeDTO) - .collect(Collectors.toList())))))); + .collect(Collectors.toSet())))))); }); } catch (Exception e) { return ExceptionHandlers.handleRolePermissionOperationException( @@ -289,7 +289,7 @@ public Response revokePrivilegeFromRole( object, privilegeRevokeRequest.getPrivileges().stream() .map(DTOConverters::fromPrivilegeDTO) - .collect(Collectors.toList())))))); + .collect(Collectors.toSet())))))); }); } catch (Exception e) { return ExceptionHandlers.handleRolePermissionOperationException(