From 00254d7b8c714ae2000d0934d260b23458033529 Mon Sep 17 00:00:00 2001 From: Inigo Goiri Date: Fri, 19 Oct 2018 13:57:43 -0700 Subject: [PATCH] HADOOP-15836. Review of AccessControlList. Contributed by BELUGA BEHR. --- .../security/authorize/AccessControlList.java | 138 ++++++++---------- .../authorize/TestAccessControlList.java | 135 ++++++++--------- 2 files changed, 123 insertions(+), 150 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 a32a5fe74b..f1ad2877b3 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,11 +20,13 @@ import java.io.DataInput; import java.io.DataOutput; import java.io.IOException; +import java.util.ArrayList; import java.util.Collection; -import java.util.HashSet; -import java.util.LinkedList; -import java.util.List; +import java.util.Collections; +import java.util.Set; +import java.util.TreeSet; +import org.apache.commons.lang3.StringUtils; import org.apache.hadoop.classification.InterfaceAudience; import org.apache.hadoop.classification.InterfaceStability; import org.apache.hadoop.conf.Configuration; @@ -34,7 +36,6 @@ 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. @@ -43,7 +44,7 @@ @InterfaceStability.Evolving public class AccessControlList implements Writable { - static { // register a ctor + static { // register a ctor WritableFactories.setFactory (AccessControlList.class, new WritableFactory() { @@ -57,9 +58,9 @@ public class AccessControlList implements Writable { private static final int INITIAL_CAPACITY = 256; // Set of users who are granted access. - private Collection users; + private final Set users = new TreeSet<>(); // Set of groups which are granted access - private Collection groups; + private final Set groups = new TreeSet<>(); // Whether all users are granted access. private boolean allAllowed; @@ -73,22 +74,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 */ @@ -103,49 +104,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; - break; + return; } } - 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 >= 1 && userGroupStrings[0] != null) { + String[] userList = userGroupStrings[0].split(","); + for (String user : userList) { + if (StringUtils.isNotBlank(user)) { + users.add(user.trim()); + } } } + 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) { - if (aclString.contains(WILDCARD_ACL_VALUE) && - aclString.trim().equals(WILDCARD_ACL_VALUE)) { - return true; - } - return false; + return WILDCARD_ACL_VALUE.equals(aclString.trim()); } 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)) { @@ -158,27 +159,24 @@ 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()) { - List groupsList = new LinkedList(); - groupsList.add(group); - groupsMapping.cacheGroupsAdd(groupsList); + groupsMapping.cacheGroupsAdd(Collections.singletonList(group)); 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)) { @@ -191,14 +189,13 @@ 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); @@ -207,18 +204,20 @@ public void removeGroup(String group) { /** * 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 getUsers() { - return users; + return Collections.unmodifiableSet(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 getGroups() { - return groups; + return Collections.unmodifiableSet(groups); } /** @@ -230,7 +229,8 @@ public Collection getGroups() { 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 void readFields(DataInput in) throws IOException { * @return comma separated list of users */ private String getUsersString() { - return getString(users); + return String.join(",", users); } /** @@ -335,26 +335,6 @@ private String getUsersString() { * @return comma separated list of groups */ private String getGroupsString() { - 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(); + return String.join(",", groups); } } 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 7039001b8f..b8e86feb04 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,9 +21,11 @@ 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; @@ -37,9 +39,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.verify; +import com.google.common.collect.Iterables; @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"); - assertTrue(acl.toString().equals( - "Members of the groups [group1, group2] are allowed")); + assertEquals("Members of the groups [group1, group2] are allowed", + acl.toString()); validateGetAclString(acl); acl = new AccessControlList("user1,user2 group1,group2"); - assertTrue(acl.toString().equals( - "Users [user1, user2] and " + - "members of the groups [group1, group2] are allowed")); + assertEquals("Users [user1, user2] and members of the groups " + + "[group1, group2] are allowed", acl.toString()); + validateGetAclString(acl); } @@ -225,48 +225,45 @@ public void testAccessControlList() throws Exception { AccessControlList acl; Collection users; Collection groups; - + acl = new AccessControlList("drwho tardis"); users = acl.getUsers(); - assertEquals(users.size(), 1); - assertEquals(users.iterator().next(), "drwho"); + assertEquals(1, users.size()); + assertEquals("drwho", Iterables.getOnlyElement(users)); groups = acl.getGroups(); - assertEquals(groups.size(), 1); - assertEquals(groups.iterator().next(), "tardis"); - + assertEquals(1, groups.size()); + assertEquals("tardis", Iterables.getOnlyElement(groups)); + acl = new AccessControlList("drwho"); users = acl.getUsers(); - assertEquals(users.size(), 1); - assertEquals(users.iterator().next(), "drwho"); + assertEquals(1, users.size()); + assertEquals("drwho", Iterables.getOnlyElement(users)); groups = acl.getGroups(); - assertEquals(groups.size(), 0); - + assertEquals(0, groups.size()); + acl = new AccessControlList("drwho "); users = acl.getUsers(); - assertEquals(users.size(), 1); - assertEquals(users.iterator().next(), "drwho"); + assertEquals(1, users.size()); + assertEquals("drwho", Iterables.getOnlyElement(users)); groups = acl.getGroups(); - assertEquals(groups.size(), 0); - + assertEquals(0, groups.size()); + acl = new AccessControlList(" tardis"); users = acl.getUsers(); - assertEquals(users.size(), 0); + assertEquals(0, users.size()); groups = acl.getGroups(); - assertEquals(groups.size(), 1); - assertEquals(groups.iterator().next(), "tardis"); + assertEquals(1, groups.size()); + assertEquals("tardis", Iterables.getOnlyElement(groups)); - Iterator iter; acl = new AccessControlList("drwho,joe tardis, users"); users = acl.getUsers(); - assertEquals(users.size(), 2); - iter = users.iterator(); - assertEquals(iter.next(), "drwho"); - assertEquals(iter.next(), "joe"); + assertEquals(2, users.size()); + assertTrue(users.contains("drwho")); + assertTrue(users.contains("joe")); groups = acl.getGroups(); - assertEquals(groups.size(), 2); - iter = groups.iterator(); - assertEquals(iter.next(), "tardis"); - assertEquals(iter.next(), "users"); + assertEquals(2, groups.size()); + assertTrue(groups.contains("tardis")); + assertTrue(groups.contains("users")); } /** @@ -281,64 +278,60 @@ public void testAddRemoveAPI() { assertEquals(0, acl.getUsers().size()); assertEquals(0, acl.getGroups().size()); assertEquals(" ", acl.getAclString()); - + acl.addUser("drwho"); users = acl.getUsers(); - assertEquals(users.size(), 1); - assertEquals(users.iterator().next(), "drwho"); + assertEquals(1, users.size()); + assertEquals("drwho", Iterables.getOnlyElement(users)); assertEquals("drwho ", acl.getAclString()); - + acl.addGroup("tardis"); groups = acl.getGroups(); - assertEquals(groups.size(), 1); - assertEquals(groups.iterator().next(), "tardis"); + assertEquals(1, groups.size()); + assertEquals("tardis", Iterables.getOnlyElement(groups)); assertEquals("drwho tardis", acl.getAclString()); - + acl.addUser("joe"); acl.addGroup("users"); users = acl.getUsers(); - assertEquals(users.size(), 2); - Iterator iter = users.iterator(); - assertEquals(iter.next(), "drwho"); - assertEquals(iter.next(), "joe"); + assertEquals(2, users.size()); + assertTrue(users.contains("drwho")); + assertTrue(users.contains("joe")); + groups = acl.getGroups(); - assertEquals(groups.size(), 2); - iter = groups.iterator(); - assertEquals(iter.next(), "tardis"); - assertEquals(iter.next(), "users"); - assertEquals("drwho,joe tardis,users", acl.getAclString()); + assertEquals(2, groups.size()); + assertTrue(groups.contains("tardis")); + assertTrue(groups.contains("users")); acl.removeUser("joe"); acl.removeGroup("users"); users = acl.getUsers(); - assertEquals(users.size(), 1); - assertFalse(users.contains("joe")); + assertEquals(1, users.size()); + assertEquals("drwho", Iterables.getOnlyElement(users)); groups = acl.getGroups(); - assertEquals(groups.size(), 1); - assertFalse(groups.contains("users")); + assertEquals(1, groups.size()); + assertEquals("tardis", Iterables.getOnlyElement(groups)); 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(" * "); @@ -347,7 +340,7 @@ public void testAddRemoveWildCard() { } assertNotNull(th); assertTrue(th instanceof IllegalArgumentException); - + th = null; try { acl.addGroup(" * "); @@ -373,7 +366,7 @@ public void testAddRemoveWildCard() { assertNotNull(th); assertTrue(th instanceof IllegalArgumentException); } - + /** * Tests adding user/group to an wild card acl. */ @@ -395,7 +388,7 @@ public void testAddRemoveToWildCardACL() { acl.addGroup("tardis"); assertTrue(acl.isAllAllowed()); assertFalse(acl.getAclString().contains("tardis")); - + acl.removeUser("drwho"); assertTrue(acl.isAllAllowed()); assertUserAllowed(drwho, acl);