[#6116] fix(authz): Fix the check of duplicated privileges (#6117)

### What changes were proposed in this pull request? Fix the check of duplicated privileges ### Why are the changes needed? Fix: #6116 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Added new UT.
apache · Jan 7, 2025 · 688c1c9 · 688c1c9
1 parent b66ed1f
commit 688c1c9
Show file tree

Hide file tree

Showing 15 changed files with 166 additions and 53 deletions.
diff --git a/...test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerBaseE2EIT.java b/...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/...test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerFilesetIT.java b/...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
@@ -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<Privilege> privilegesList = new ArrayList<>();
+      Set<Privilege> 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
@@ -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<Privilege> privilegesList = new ArrayList<>();
+      Set<Privilege> 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
@@ -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<PrivilegeDTO> toPrivileges(List<Privilege> privileges) {
+  static List<PrivilegeDTO> toPrivileges(Collection<Privilege> 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
@@ -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<Privilege> 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<Privilege> 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<Privi
    * @throws IllegalPrivilegeException If any privilege can't be bind to the metadata object.
    * @throws RuntimeException If revoking privileges from a role encounters storage issues.
    */
+  @Deprecated
   public Role revokePrivilegesFromRole(
       String role, MetadataObject object, List<Privilege> 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<Privilege> 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
@@ -20,13 +20,15 @@
 
 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;
 import java.util.List;
 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<String> roles, String group)
   public Role grantPrivilegesToRole(String role, MetadataObject object, List<Privilege> privileges)
       throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException,
           IllegalPrivilegeException {
+    Set<Privilege> 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<Privilege> 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<Privi
    * @throws IllegalPrivilegeException If any privilege can't be bind to the metadata object.
    * @throws RuntimeException If revoking privileges from a role encounters storage issues.
    */
+  @Deprecated
   public Role revokePrivilegesFromRole(
       String role, MetadataObject object, List<Privilege> privileges)
       throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException,
           IllegalPrivilegeException {
+    Set<Privilege> 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<Privilege> 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
@@ -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())));
   }
 }