Revert "HADOOP-15836. Review of AccessControlList. Contributed by BELUGA BEHR."

This reverts commit 00254d7b8c.

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/authorize/AccessControlList.java

@@ -20,13 +20,11 @@ package org.apache.hadoop.security.authorize;
 import java.io.DataInput;
 import java.io.DataOutput;
 import java.io.IOException;
-import java.util.ArrayList;
 import java.util.Collection;
-import java.util.Collections;
+import java.util.HashSet;
-import java.util.Set;
+import java.util.LinkedList;
-import java.util.TreeSet;
+import java.util.List;
 
-import org.apache.commons.lang3.StringUtils;
 import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.classification.InterfaceStability;
 import org.apache.hadoop.conf.Configuration;
@@ -36,6 +34,7 @@ import org.apache.hadoop.io.WritableFactories;
 import org.apache.hadoop.io.WritableFactory;
 import org.apache.hadoop.security.Groups;
 import org.apache.hadoop.security.UserGroupInformation;
+import org.apache.hadoop.util.StringUtils;
 
 /**
  * Class representing a configured access control list.
@@ -44,7 +43,7 @@ import org.apache.hadoop.security.UserGroupInformation;
 @InterfaceStability.Evolving
 public class AccessControlList implements Writable {
 
-  static { // register a ctor
+  static {                                      // register a ctor
     WritableFactories.setFactory
     (AccessControlList.class,
       new WritableFactory() {
@@ -58,9 +57,9 @@ public class AccessControlList implements Writable {
   private static final int INITIAL_CAPACITY = 256;
 
   // Set of users who are granted access.
-  private final Set<String> users = new TreeSet<>();
+  private Collection<String> users;
   // Set of groups which are granted access
-  private final Set<String> groups = new TreeSet<>();
+  private Collection<String> groups;
   // Whether all users are granted access.
   private boolean allAllowed;
 
@@ -74,22 +73,22 @@ public class AccessControlList implements Writable {
 
   /**
    * Construct a new ACL from a String representation of the same.
-   *
+   * 
    * The String is a a comma separated list of users and groups.
-   * The user list comes first and is separated by a space followed
+   * The user list comes first and is separated by a space followed 
    * by the group list. For e.g. "user1,user2 group1,group2"
-   *
+   * 
    * @param aclString String representation of the ACL
    */
   public AccessControlList(String aclString) {
     buildACL(aclString.split(" ", 2));
   }
-
+  
   /**
-   * Construct a new ACL from String representation of users and groups.
+   * Construct a new ACL from String representation of users and groups
-   *
+   * 
    * The arguments are comma separated lists
-   *
+   * 
    * @param users comma separated list of users
    * @param groups comma separated list of groups
    */
@@ -104,49 +103,49 @@ public class AccessControlList implements Writable {
    * @param aclString build ACL from array of Strings
    */
   private void buildACL(String[] userGroupStrings) {
+    users = new HashSet<String>();
+    groups = new HashSet<String>();
     for (String aclPart : userGroupStrings) {
       if (aclPart != null && isWildCardACLValue(aclPart)) {
         allAllowed = true;
-        return;
+        break;
       }
     }
-    if (userGroupStrings.length >= 1 && userGroupStrings[0] != null) {
+    if (!allAllowed) {      
-      String[] userList = userGroupStrings[0].split(",");
+      if (userGroupStrings.length >= 1 && userGroupStrings[0] != null) {
-      for (String user : userList) {
+        users = StringUtils.getTrimmedStringCollection(userGroupStrings[0]);
-        if (StringUtils.isNotBlank(user)) {
+      } 
-          users.add(user.trim());
+      
-        }
+      if (userGroupStrings.length == 2 && userGroupStrings[1] != null) {
+        groups = StringUtils.getTrimmedStringCollection(userGroupStrings[1]);
+        groupsMapping.cacheGroupsAdd(new LinkedList<String>(groups));
       }
     }
-    if (userGroupStrings.length == 2 && userGroupStrings[1] != null) {
-      String[] groupList = userGroupStrings[1].split(",");
-      for (String group : groupList) {
-        if (StringUtils.isNotBlank(group)) {
-          groups.add(group.trim());
-        }
-      }
-      groupsMapping.cacheGroupsAdd(new ArrayList<>(groups));
-    }
   }
-
+  
   /**
-   * Checks whether ACL string contains wildcard.
+   * Checks whether ACL string contains wildcard
    *
    * @param aclString check this ACL string for wildcard
    * @return true if ACL string contains wildcard false otherwise
    */
   private boolean isWildCardACLValue(String aclString) {
-    return WILDCARD_ACL_VALUE.equals(aclString.trim());
+    if (aclString.contains(WILDCARD_ACL_VALUE) && 
+        aclString.trim().equals(WILDCARD_ACL_VALUE)) {
+      return true;
+    }
+    return false;
   }
 
   public boolean isAllAllowed() {
     return allAllowed;
   }
-
+  
   /**
    * Add user to the names of users allowed for this service.
-   *
+   * 
-   * @param user The user name
+   * @param user
+   *          The user name
    */
   public void addUser(String user) {
     if (isWildCardACLValue(user)) {
@@ -159,24 +158,27 @@ public class AccessControlList implements Writable {
 
   /**
    * Add group to the names of groups allowed for this service.
-   *
+   * 
-   * @param group The group name
+   * @param group
+   *          The group name
    */
   public void addGroup(String group) {
     if (isWildCardACLValue(group)) {
-      throw new IllegalArgumentException(
+      throw new IllegalArgumentException("Group " + group + " can not be added");
-          "Group " + group + " can not be added");
     }
     if (!isAllAllowed()) {
-      groupsMapping.cacheGroupsAdd(Collections.singletonList(group));
+      List<String> groupsList = new LinkedList<String>();
+      groupsList.add(group);
+      groupsMapping.cacheGroupsAdd(groupsList);
       groups.add(group);
     }
   }
 
   /**
    * Remove user from the names of users allowed for this service.
-   *
+   * 
-   * @param user The user name
+   * @param user
+   *          The user name
    */
   public void removeUser(String user) {
     if (isWildCardACLValue(user)) {
@@ -189,13 +191,14 @@ public class AccessControlList implements Writable {
 
   /**
    * Remove group from the names of groups allowed for this service.
-   *
+   * 
-   * @param group The group name
+   * @param group
+   *          The group name
    */
   public void removeGroup(String group) {
     if (isWildCardACLValue(group)) {
-      throw new IllegalArgumentException(
+      throw new IllegalArgumentException("Group " + group
-          "Group " + group + " can not be removed");
+          + " can not be removed");
     }
     if (!isAllAllowed()) {
       groups.remove(group);
@@ -204,20 +207,18 @@ public class AccessControlList implements Writable {
 
   /**
    * Get the names of users allowed for this service.
-   *
+   * @return the set of user names. the set must not be modified.
-   * @return an unmodifiable set of user names in alphabetic order.
    */
   public Collection<String> getUsers() {
-    return Collections.unmodifiableSet(users);
+    return users;
   }
-
+  
   /**
    * Get the names of user groups allowed for this service.
-   *
+   * @return the set of group names. the set must not be modified.
-   * @return an unmodifiable set of group names in alphabetic order.
    */
   public Collection<String> getGroups() {
-    return Collections.unmodifiableSet(groups);
+    return groups;
   }
 
   /**
@@ -229,8 +230,7 @@ public class AccessControlList implements Writable {
   public final boolean isUserInList(UserGroupInformation ugi) {
     if (allAllowed || users.contains(ugi.getShortUserName())) {
       return true;
-    }
+    } else if (!groups.isEmpty()) {
-    if (!groups.isEmpty()) {
       for (String group : ugi.getGroups()) {
         if (groups.contains(group)) {
           return true;
@@ -326,7 +326,7 @@ public class AccessControlList implements Writable {
    * @return comma separated list of users
    */
   private String getUsersString() {
-    return String.join(",", users);
+    return getString(users);
   }
 
   /**
@@ -335,6 +335,26 @@ public class AccessControlList implements Writable {
    * @return comma separated list of groups
    */
   private String getGroupsString() {
-    return String.join(",", groups);
+    return getString(groups);
+  }
+
+  /**
+   * Returns comma-separated concatenated single String of all strings of
+   * the given set
+   *
+   * @param strings set of strings to concatenate
+   */
+  private String getString(Collection<String> strings) {
+    StringBuilder sb = new StringBuilder(INITIAL_CAPACITY);
+    boolean first = true;
+    for(String str: strings) {
+      if (!first) {
+        sb.append(",");
+      } else {
+        first = false;
+      }
+      sb.append(str);
+    }
+    return sb.toString();
   }
 }

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/authorize/TestAccessControlList.java

@@ -21,11 +21,9 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
-import static org.mockito.Mockito.never;
-import static org.mockito.Mockito.spy;
-import static org.mockito.Mockito.verify;
 
 import java.util.Collection;
+import java.util.Iterator;
 import java.util.List;
 
 import org.apache.hadoop.classification.InterfaceAudience;
@@ -39,7 +37,9 @@ import org.junit.Test;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import com.google.common.collect.Iterables;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
 
 @InterfaceAudience.LimitedPrivate({"HDFS", "MapReduce"})
 @InterfaceStability.Evolving
@@ -148,17 +148,17 @@ public class TestAccessControlList {
     assertTrue(jerryLeeLewisGroups.contains("@memphis"));
 
     // allowed because his netgroup is in ACL
-    UserGroupInformation elvis =
+    UserGroupInformation elvis = 
       UserGroupInformation.createRemoteUser("elvis");
     assertUserAllowed(elvis, acl);
 
     // allowed because he's in ACL
-    UserGroupInformation carlPerkins =
+    UserGroupInformation carlPerkins = 
       UserGroupInformation.createRemoteUser("carlPerkins");
     assertUserAllowed(carlPerkins, acl);
 
     // not allowed because he's not in ACL and has no netgroups
-    UserGroupInformation littleRichard =
+    UserGroupInformation littleRichard = 
       UserGroupInformation.createRemoteUser("littleRichard");
     assertUserNotAllowed(littleRichard, acl);
   }
@@ -166,16 +166,16 @@ public class TestAccessControlList {
   @Test
   public void testWildCardAccessControlList() throws Exception {
     AccessControlList acl;
-
+    
     acl = new AccessControlList("*");
     assertTrue(acl.isAllAllowed());
-
+    
     acl = new AccessControlList("  * ");
     assertTrue(acl.isAllAllowed());
-
+    
     acl = new AccessControlList(" *");
     assertTrue(acl.isAllAllowed());
-
+    
     acl = new AccessControlList("*  ");
     assertTrue(acl.isAllAllowed());
   }
@@ -202,14 +202,14 @@ public class TestAccessControlList {
     validateGetAclString(acl);
 
     acl = new AccessControlList(" group1,group2");
-    assertEquals("Members of the groups [group1, group2] are allowed",
+    assertTrue(acl.toString().equals(
-        acl.toString());
+        "Members of the groups [group1, group2] are allowed"));
     validateGetAclString(acl);
 
     acl = new AccessControlList("user1,user2 group1,group2");
-    assertEquals("Users [user1, user2] and members of the groups "
+    assertTrue(acl.toString().equals(
-        + "[group1, group2] are allowed", acl.toString());
+        "Users [user1, user2] and " +
-
+        "members of the groups [group1, group2] are allowed"));
     validateGetAclString(acl);
   }
 
@@ -225,45 +225,48 @@ public class TestAccessControlList {
     AccessControlList acl;
     Collection<String> users;
     Collection<String> groups;
-
+    
     acl = new AccessControlList("drwho tardis");
     users = acl.getUsers();
-    assertEquals(1, users.size());
+    assertEquals(users.size(), 1);
-    assertEquals("drwho", Iterables.getOnlyElement(users));
+    assertEquals(users.iterator().next(), "drwho");
     groups = acl.getGroups();
-    assertEquals(1, groups.size());
+    assertEquals(groups.size(), 1);
-    assertEquals("tardis", Iterables.getOnlyElement(groups));
+    assertEquals(groups.iterator().next(), "tardis");
-
+    
     acl = new AccessControlList("drwho");
     users = acl.getUsers();
-    assertEquals(1, users.size());
+    assertEquals(users.size(), 1);
-    assertEquals("drwho", Iterables.getOnlyElement(users));
+    assertEquals(users.iterator().next(), "drwho");
     groups = acl.getGroups();
-    assertEquals(0, groups.size());
+    assertEquals(groups.size(), 0);
-
+    
     acl = new AccessControlList("drwho ");
     users = acl.getUsers();
-    assertEquals(1, users.size());
+    assertEquals(users.size(), 1);
-    assertEquals("drwho", Iterables.getOnlyElement(users));
+    assertEquals(users.iterator().next(), "drwho");
     groups = acl.getGroups();
-    assertEquals(0, groups.size());
+    assertEquals(groups.size(), 0);
-
+    
     acl = new AccessControlList(" tardis");
     users = acl.getUsers();
-    assertEquals(0, users.size());
+    assertEquals(users.size(), 0);
     groups = acl.getGroups();
-    assertEquals(1, groups.size());
+    assertEquals(groups.size(), 1);
-    assertEquals("tardis", Iterables.getOnlyElement(groups));
+    assertEquals(groups.iterator().next(), "tardis");
 
+    Iterator<String> iter;    
     acl = new AccessControlList("drwho,joe tardis, users");
     users = acl.getUsers();
-    assertEquals(2, users.size());
+    assertEquals(users.size(), 2);
-    assertTrue(users.contains("drwho"));
+    iter = users.iterator();
-    assertTrue(users.contains("joe"));
+    assertEquals(iter.next(), "drwho");
+    assertEquals(iter.next(), "joe");
     groups = acl.getGroups();
-    assertEquals(2, groups.size());
+    assertEquals(groups.size(), 2);
-    assertTrue(groups.contains("tardis"));
+    iter = groups.iterator();
-    assertTrue(groups.contains("users"));
+    assertEquals(iter.next(), "tardis");
+    assertEquals(iter.next(), "users");
   }
 
   /**
@@ -278,60 +281,64 @@ public class TestAccessControlList {
     assertEquals(0, acl.getUsers().size());
     assertEquals(0, acl.getGroups().size());
     assertEquals(" ", acl.getAclString());
-
+    
     acl.addUser("drwho");
     users = acl.getUsers();
-    assertEquals(1, users.size());
+    assertEquals(users.size(), 1);
-    assertEquals("drwho", Iterables.getOnlyElement(users));
+    assertEquals(users.iterator().next(), "drwho");
     assertEquals("drwho ", acl.getAclString());
-
+    
     acl.addGroup("tardis");
     groups = acl.getGroups();
-    assertEquals(1, groups.size());
+    assertEquals(groups.size(), 1);
-    assertEquals("tardis", Iterables.getOnlyElement(groups));
+    assertEquals(groups.iterator().next(), "tardis");
     assertEquals("drwho tardis", acl.getAclString());
-
+    
     acl.addUser("joe");
     acl.addGroup("users");
     users = acl.getUsers();
-    assertEquals(2, users.size());
+    assertEquals(users.size(), 2);
-    assertTrue(users.contains("drwho"));
+    Iterator<String> iter = users.iterator();
-    assertTrue(users.contains("joe"));
+    assertEquals(iter.next(), "drwho");
-
+    assertEquals(iter.next(), "joe");
     groups = acl.getGroups();
-    assertEquals(2, groups.size());
+    assertEquals(groups.size(), 2);
-    assertTrue(groups.contains("tardis"));
+    iter = groups.iterator();
-    assertTrue(groups.contains("users"));
+    assertEquals(iter.next(), "tardis");
+    assertEquals(iter.next(), "users");
+    assertEquals("drwho,joe tardis,users", acl.getAclString());
 
     acl.removeUser("joe");
     acl.removeGroup("users");
     users = acl.getUsers();
-    assertEquals(1, users.size());
+    assertEquals(users.size(), 1);
-    assertEquals("drwho", Iterables.getOnlyElement(users));
+    assertFalse(users.contains("joe"));
     groups = acl.getGroups();
-    assertEquals(1, groups.size());
+    assertEquals(groups.size(), 1);
-    assertEquals("tardis", Iterables.getOnlyElement(groups));
+    assertFalse(groups.contains("users"));
     assertEquals("drwho tardis", acl.getAclString());
-
+    
     acl.removeGroup("tardis");
     groups = acl.getGroups();
     assertEquals(0, groups.size());
+    assertFalse(groups.contains("tardis"));
     assertEquals("drwho ", acl.getAclString());
-
+    
     acl.removeUser("drwho");
     assertEquals(0, users.size());
+    assertFalse(users.contains("drwho"));
     assertEquals(0, acl.getGroups().size());
     assertEquals(0, acl.getUsers().size());
     assertEquals(" ", acl.getAclString());
   }
-
+  
   /**
    * Tests adding/removing wild card as the user/group.
    */
   @Test
   public void testAddRemoveWildCard() {
     AccessControlList acl = new AccessControlList("drwho tardis");
-
+    
     Throwable th = null;
     try {
       acl.addUser(" * ");
@@ -340,7 +347,7 @@ public class TestAccessControlList {
     }
     assertNotNull(th);
     assertTrue(th instanceof IllegalArgumentException);
-
+    
     th = null;
     try {
       acl.addGroup(" * ");
@@ -366,7 +373,7 @@ public class TestAccessControlList {
     assertNotNull(th);
     assertTrue(th instanceof IllegalArgumentException);
   }
-
+  
   /**
    * Tests adding user/group to an wild card acl.
    */
@@ -388,7 +395,7 @@ public class TestAccessControlList {
     acl.addGroup("tardis");
     assertTrue(acl.isAllAllowed());
     assertFalse(acl.getAclString().contains("tardis"));
-
+   
     acl.removeUser("drwho");
     assertTrue(acl.isAllAllowed());
     assertUserAllowed(drwho, acl);