From dd268a64d36d27dbeeb775b92cb3664f4ed4fbdf Mon Sep 17 00:00:00 2001 From: Inigo Goiri Date: Tue, 23 Oct 2018 09:23:03 -0700 Subject: [PATCH] Revert "HADOOP-15836. Review of AccessControlList. Contributed by BELUGA BEHR." This reverts commit 00254d7b8c714ae2000d0934d260b23458033529. --- .../security/authorize/AccessControlList.java | 138 ++++++++++-------- .../authorize/TestAccessControlList.java | 135 +++++++++-------- 2 files changed, 150 insertions(+), 123 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/authorize/AccessControlList.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/authorize/AccessControlList.java index f1ad2877b3..a32a5fe74b 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/authorize/AccessControlList.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/authorize/AccessControlList.java @@ -20,13 +20,11 @@ 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.Set; -import java.util.TreeSet; +import java.util.HashSet; +import java.util.LinkedList; +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.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 @@ @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 users = new TreeSet<>(); + private Collection users; // Set of groups which are granted access - private final Set groups = new TreeSet<>(); + private Collection groups; // Whether all users are granted access. private boolean allAllowed; @@ -74,22 +73,22 @@ public AccessControlList() { /** * 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 AccessControlList(String users, String groups) { * @param aclString build ACL from array of Strings */ private void buildACL(String[] userGroupStrings) { + users = new HashSet(); + groups = new HashSet(); for (String aclPart : userGroupStrings) { if (aclPart != null && isWildCardACLValue(aclPart)) { allAllowed = true; - return; + break; } } - if (userGroupStrings.length >= 1 && userGroupStrings[0] != null) { - String[] userList = userGroupStrings[0].split(","); - for (String user : userList) { - if (StringUtils.isNotBlank(user)) { - users.add(user.trim()); - } + if (!allAllowed) { + if (userGroupStrings.length >= 1 && userGroupStrings[0] != null) { + users = StringUtils.getTrimmedStringCollection(userGroupStrings[0]); + } + + if (userGroupStrings.length == 2 && userGroupStrings[1] != null) { + groups = StringUtils.getTrimmedStringCollection(userGroupStrings[1]); + groupsMapping.cacheGroupsAdd(new LinkedList(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 void addUser(String user) { /** * 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( - "Group " + group + " can not be added"); + throw new IllegalArgumentException("Group " + group + " can not be added"); } if (!isAllAllowed()) { - groupsMapping.cacheGroupsAdd(Collections.singletonList(group)); + List groupsList = new LinkedList(); + 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 void removeUser(String user) { /** * 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( - "Group " + group + " can not be removed"); + throw new IllegalArgumentException("Group " + group + + " can not be removed"); } if (!isAllAllowed()) { groups.remove(group); @@ -204,20 +207,18 @@ public void removeGroup(String group) { /** * Get the names of users allowed for this service. - * - * @return an unmodifiable set of user names in alphabetic order. + * @return the set of user names. the set must not be modified. */ public Collection getUsers() { - return Collections.unmodifiableSet(users); + return users; } - + /** * Get the names of user groups allowed for this service. - * - * @return an unmodifiable set of group names in alphabetic order. + * @return the set of group names. the set must not be modified. */ public Collection getGroups() { - return Collections.unmodifiableSet(groups); + return groups; } /** @@ -229,8 +230,7 @@ public Collection getGroups() { public final boolean isUserInList(UserGroupInformation ugi) { if (allAllowed || users.contains(ugi.getShortUserName())) { return true; - } - if (!groups.isEmpty()) { + } else if (!groups.isEmpty()) { for (String group : ugi.getGroups()) { if (groups.contains(group)) { return true; @@ -326,7 +326,7 @@ public void readFields(DataInput in) throws IOException { * @return comma separated list of users */ private String getUsersString() { - return String.join(",", users); + return getString(users); } /** @@ -335,6 +335,26 @@ private String getUsersString() { * @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 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(); } } diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/authorize/TestAccessControlList.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/authorize/TestAccessControlList.java index b8e86feb04..7039001b8f 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/authorize/TestAccessControlList.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/authorize/TestAccessControlList.java @@ -21,11 +21,9 @@ 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.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 @@ private void validateNetgroups(Groups groups, 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 @@ private void validateNetgroups(Groups groups, @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 void testAclString() { validateGetAclString(acl); acl = new AccessControlList(" group1,group2"); - assertEquals("Members of the groups [group1, group2] are allowed", - acl.toString()); + assertTrue(acl.toString().equals( + "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 " - + "[group1, group2] are allowed", acl.toString()); - + assertTrue(acl.toString().equals( + "Users [user1, user2] and " + + "members of the groups [group1, group2] are allowed")); validateGetAclString(acl); } @@ -225,45 +225,48 @@ public void testAccessControlList() throws Exception { AccessControlList acl; Collection users; Collection groups; - + acl = new AccessControlList("drwho tardis"); users = acl.getUsers(); - assertEquals(1, users.size()); - assertEquals("drwho", Iterables.getOnlyElement(users)); + assertEquals(users.size(), 1); + assertEquals(users.iterator().next(), "drwho"); groups = acl.getGroups(); - assertEquals(1, groups.size()); - assertEquals("tardis", Iterables.getOnlyElement(groups)); - + assertEquals(groups.size(), 1); + assertEquals(groups.iterator().next(), "tardis"); + acl = new AccessControlList("drwho"); users = acl.getUsers(); - assertEquals(1, users.size()); - assertEquals("drwho", Iterables.getOnlyElement(users)); + assertEquals(users.size(), 1); + 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("drwho", Iterables.getOnlyElement(users)); + assertEquals(users.size(), 1); + 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("tardis", Iterables.getOnlyElement(groups)); + assertEquals(groups.size(), 1); + assertEquals(groups.iterator().next(), "tardis"); + Iterator iter; acl = new AccessControlList("drwho,joe tardis, users"); users = acl.getUsers(); - assertEquals(2, users.size()); - assertTrue(users.contains("drwho")); - assertTrue(users.contains("joe")); + assertEquals(users.size(), 2); + iter = users.iterator(); + assertEquals(iter.next(), "drwho"); + assertEquals(iter.next(), "joe"); groups = acl.getGroups(); - assertEquals(2, groups.size()); - assertTrue(groups.contains("tardis")); - assertTrue(groups.contains("users")); + assertEquals(groups.size(), 2); + iter = groups.iterator(); + assertEquals(iter.next(), "tardis"); + assertEquals(iter.next(), "users"); } /** @@ -278,60 +281,64 @@ public void testAddRemoveAPI() { assertEquals(0, acl.getUsers().size()); assertEquals(0, acl.getGroups().size()); assertEquals(" ", acl.getAclString()); - + acl.addUser("drwho"); users = acl.getUsers(); - assertEquals(1, users.size()); - assertEquals("drwho", Iterables.getOnlyElement(users)); + assertEquals(users.size(), 1); + assertEquals(users.iterator().next(), "drwho"); assertEquals("drwho ", acl.getAclString()); - + acl.addGroup("tardis"); groups = acl.getGroups(); - assertEquals(1, groups.size()); - assertEquals("tardis", Iterables.getOnlyElement(groups)); + assertEquals(groups.size(), 1); + assertEquals(groups.iterator().next(), "tardis"); assertEquals("drwho tardis", acl.getAclString()); - + acl.addUser("joe"); acl.addGroup("users"); users = acl.getUsers(); - assertEquals(2, users.size()); - assertTrue(users.contains("drwho")); - assertTrue(users.contains("joe")); - + assertEquals(users.size(), 2); + Iterator iter = users.iterator(); + assertEquals(iter.next(), "drwho"); + assertEquals(iter.next(), "joe"); groups = acl.getGroups(); - assertEquals(2, groups.size()); - assertTrue(groups.contains("tardis")); - assertTrue(groups.contains("users")); + assertEquals(groups.size(), 2); + iter = groups.iterator(); + 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("drwho", Iterables.getOnlyElement(users)); + assertEquals(users.size(), 1); + assertFalse(users.contains("joe")); groups = acl.getGroups(); - assertEquals(1, groups.size()); - assertEquals("tardis", Iterables.getOnlyElement(groups)); + assertEquals(groups.size(), 1); + 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 void testAddRemoveWildCard() { } assertNotNull(th); assertTrue(th instanceof IllegalArgumentException); - + th = null; try { acl.addGroup(" * "); @@ -366,7 +373,7 @@ public void testAddRemoveWildCard() { assertNotNull(th); assertTrue(th instanceof IllegalArgumentException); } - + /** * Tests adding user/group to an wild card acl. */ @@ -388,7 +395,7 @@ public void testAddRemoveToWildCardACL() { acl.addGroup("tardis"); assertTrue(acl.isAllAllowed()); assertFalse(acl.getAclString().contains("tardis")); - + acl.removeUser("drwho"); assertTrue(acl.isAllAllowed()); assertUserAllowed(drwho, acl);