From cae96dfe6e7c57f927bd825711977ae9199776a9 Mon Sep 17 00:00:00 2001 From: Chris Nauroth Date: Mon, 20 Jan 2014 18:00:06 +0000 Subject: [PATCH] HADOOP-10213. Fix bugs parsing ACL spec in FsShell setfacl. Contributed by Vinay. git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/branches/HDFS-4685@1559793 13f79535-47bb-0310-9956-ffa450edef68 --- .../apache/hadoop/fs/permission/AclEntry.java | 76 ++++++++++++++++++- .../apache/hadoop/fs/shell/AclCommands.java | 63 +-------------- .../hadoop/fs/shell/TestAclCommands.java | 39 +++++++++- .../hadoop-hdfs/CHANGES-HDFS-4685.txt | 3 + 4 files changed, 118 insertions(+), 63 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/permission/AclEntry.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/permission/AclEntry.java index 3d57e06d471..7de61159558 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/permission/AclEntry.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/permission/AclEntry.java @@ -17,12 +17,16 @@ */ package org.apache.hadoop.fs.permission; -import static org.apache.hadoop.fs.permission.AclEntryScope.*; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; import com.google.common.base.Objects; +import org.apache.hadoop.HadoopIllegalArgumentException; import org.apache.hadoop.classification.InterfaceAudience; import org.apache.hadoop.classification.InterfaceStability; +import org.apache.hadoop.util.StringUtils; /** * Defines a single entry in an ACL. An ACL entry has a type (user, group, @@ -193,4 +197,74 @@ public class AclEntry { this.permission = permission; this.scope = scope; } + + /** + * Parses a string representation of an ACL spec into a list of AclEntry + * objects. Example: "user::rwx,user:foo:rw-,group::r--,other::---" + * + * @param aclSpec + * String representation of an ACL spec. + * @param includePermission + * for setAcl operations this will be true. i.e. AclSpec should + * include permissions.
+ * But for removeAcl operation it will be false. i.e. AclSpec should + * not contain permissions.
+ * Example: "user:foo,group:bar" + * @return Returns list of AclEntries parsed + */ + public static List parseAclSpec(String aclSpec, + boolean includePermission) { + List aclEntries = new ArrayList(); + Collection aclStrings = StringUtils.getStringCollection(aclSpec, + ","); + for (String aclStr : aclStrings) { + AclEntry.Builder builder = new AclEntry.Builder(); + // Here "::" represent one empty string. + // StringUtils.getStringCollection() will ignore this. + String[] split = aclStr.split(":"); + int expectedAclSpecLength = 2; + if (includePermission) { + expectedAclSpecLength = 3; + } + if (split.length != expectedAclSpecLength + && !(split.length == expectedAclSpecLength + 1 && "default" + .equals(split[0]))) { + throw new HadoopIllegalArgumentException("Invalid : " + + aclStr); + } + int index = 0; + if (split.length == expectedAclSpecLength + 1) { + assert "default".equals(split[0]); + // default entry + index++; + builder.setScope(AclEntryScope.DEFAULT); + } + String type = split[index++]; + AclEntryType aclType = null; + try { + aclType = Enum.valueOf(AclEntryType.class, type.toUpperCase()); + builder.setType(aclType); + } catch (IllegalArgumentException iae) { + throw new HadoopIllegalArgumentException( + "Invalid type of acl in :" + aclStr); + } + + String name = split[index++]; + if (!name.isEmpty()) { + builder.setName(name); + } + + if (expectedAclSpecLength == 3) { + String permission = split[index++]; + FsAction fsAction = FsAction.getFsAction(permission); + if (null == fsAction) { + throw new HadoopIllegalArgumentException( + "Invalid permission in : " + aclStr); + } + builder.setPermission(fsAction); + } + aclEntries.add(builder.build()); + } + return aclEntries; + } } diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/AclCommands.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/AclCommands.java index cb83027f517..67f5cc0d0da 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/AclCommands.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/AclCommands.java @@ -18,8 +18,6 @@ package org.apache.hadoop.fs.shell; import java.io.IOException; -import java.util.ArrayList; -import java.util.Collection; import java.util.Iterator; import java.util.LinkedList; import java.util.List; @@ -27,14 +25,12 @@ import java.util.List; import org.apache.hadoop.HadoopIllegalArgumentException; import org.apache.hadoop.classification.InterfaceAudience; import org.apache.hadoop.classification.InterfaceStability; -import org.apache.hadoop.fs.Path; import org.apache.hadoop.fs.permission.AclEntry; import org.apache.hadoop.fs.permission.AclEntryScope; import org.apache.hadoop.fs.permission.AclEntryType; import org.apache.hadoop.fs.permission.AclStatus; import org.apache.hadoop.fs.permission.FsAction; import org.apache.hadoop.fs.permission.FsPermission; -import org.apache.hadoop.util.StringUtils; /** * Acl related operations @@ -201,9 +197,6 @@ class AclCommands extends FsCommand { + ": Comma separated list of ACL entries.\n" + ": File or directory to modify.\n"; - private static final String DEFAULT = "default"; - - Path path = null; CommandFormat cf = new CommandFormat(0, Integer.MAX_VALUE, "b", "k", "R", "m", "x", "-set"); List aclEntries = null; @@ -230,7 +223,7 @@ class AclCommands extends FsCommand { if (args.size() < 2) { throw new HadoopIllegalArgumentException(" is missing"); } - aclEntries = parseAclSpec(args.removeFirst()); + aclEntries = AclEntry.parseAclSpec(args.removeFirst(), !cf.getOpt("x")); } if (args.isEmpty()) { @@ -239,7 +232,6 @@ class AclCommands extends FsCommand { if (args.size() > 1) { throw new HadoopIllegalArgumentException("Too many arguments"); } - path = new Path(args.removeFirst()); } @Override @@ -253,59 +245,8 @@ class AclCommands extends FsCommand { } else if (cf.getOpt("x")) { item.fs.removeAclEntries(item.path, aclEntries); } else if (cf.getOpt("-set")) { - item.fs.setAcl(path, aclEntries); + item.fs.setAcl(item.path, aclEntries); } } - - /** - * Parse the aclSpec and returns the list of AclEntry objects. - * - * @param aclSpec - * @return - */ - private List parseAclSpec(String aclSpec) { - List aclEntries = new ArrayList(); - Collection aclStrings = StringUtils.getStringCollection(aclSpec, - ","); - for (String aclStr : aclStrings) { - AclEntry.Builder builder = new AclEntry.Builder(); - // Here "::" represent one empty string. - // StringUtils.getStringCollection() will ignore this. - String[] split = aclSpec.split(":"); - if (split.length != 3 - && !(split.length == 4 && DEFAULT.equals(split[0]))) { - throw new HadoopIllegalArgumentException("Invalid : " - + aclStr); - } - int index = 0; - if (split.length == 4) { - assert DEFAULT.equals(split[0]); - // default entry - index++; - builder.setScope(AclEntryScope.DEFAULT); - } - String type = split[index++]; - AclEntryType aclType = null; - try { - aclType = Enum.valueOf(AclEntryType.class, type.toUpperCase()); - builder.setType(aclType); - } catch (IllegalArgumentException iae) { - throw new HadoopIllegalArgumentException( - "Invalid type of acl in :" + aclStr); - } - - builder.setName(split[index++]); - - String permission = split[index++]; - FsAction fsAction = FsAction.getFsAction(permission); - if (null == fsAction) { - throw new HadoopIllegalArgumentException( - "Invalid permission in : " + aclStr); - } - builder.setPermission(fsAction); - aclEntries.add(builder.build()); - } - return aclEntries; - } } } diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestAclCommands.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestAclCommands.java index d0137e24729..6de73511873 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestAclCommands.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestAclCommands.java @@ -17,12 +17,18 @@ */ package org.apache.hadoop.fs.shell; -import static org.junit.Assert.assertFalse; +import static org.junit.Assert.*; import java.io.IOException; +import java.util.ArrayList; +import java.util.List; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.FsShell; +import org.apache.hadoop.fs.permission.AclEntry; +import org.apache.hadoop.fs.permission.AclEntryScope; +import org.apache.hadoop.fs.permission.AclEntryType; +import org.apache.hadoop.fs.permission.FsAction; import org.apache.hadoop.util.ToolRunner; import org.junit.Before; import org.junit.Test; @@ -57,6 +63,37 @@ public class TestAclCommands { assertFalse("setfacl should fail with extra arguments", 0 == runCommand(new String[] { "-setfacl", "--set", "default:user::rwx", "/path", "extra" })); + assertFalse("setfacl should fail with permissions for -x", + 0 == runCommand(new String[] { "-setfacl", "-x", "user:user1:rwx", + "/path" })); + assertFalse("setfacl should fail with permissions for -x", + 0 == runCommand(new String[] { "-setfacl", "-m", + "", "/path" })); + } + + @Test + public void testMultipleAclSpecParsing() throws Exception { + List parsedList = AclEntry.parseAclSpec( + "group::rwx,user:user1:rwx,user:user2:rw-,group:group1:rw-,default:group:group1:rw-", true); + + AclEntry basicAcl = new AclEntry.Builder().setType(AclEntryType.GROUP) + .setPermission(FsAction.ALL).build(); + AclEntry user1Acl = new AclEntry.Builder().setType(AclEntryType.USER) + .setPermission(FsAction.ALL).setName("user1").build(); + AclEntry user2Acl = new AclEntry.Builder().setType(AclEntryType.USER) + .setPermission(FsAction.READ_WRITE).setName("user2").build(); + AclEntry group1Acl = new AclEntry.Builder().setType(AclEntryType.GROUP) + .setPermission(FsAction.READ_WRITE).setName("group1").build(); + AclEntry defaultAcl = new AclEntry.Builder().setType(AclEntryType.GROUP) + .setPermission(FsAction.READ_WRITE).setName("group1") + .setScope(AclEntryScope.DEFAULT).build(); + List expectedList = new ArrayList(); + expectedList.add(basicAcl); + expectedList.add(user1Acl); + expectedList.add(user2Acl); + expectedList.add(group1Acl); + expectedList.add(defaultAcl); + assertEquals("Parsed Acl not correct", expectedList, parsedList); } private int runCommand(String[] commands) throws Exception { diff --git a/hadoop-hdfs-project/hadoop-hdfs/CHANGES-HDFS-4685.txt b/hadoop-hdfs-project/hadoop-hdfs/CHANGES-HDFS-4685.txt index 747c5404571..ddbf93346e2 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/CHANGES-HDFS-4685.txt +++ b/hadoop-hdfs-project/hadoop-hdfs/CHANGES-HDFS-4685.txt @@ -45,3 +45,6 @@ HDFS-4685 (Unreleased) HDFS-5739. ACL RPC must allow null name or unspecified permissions in ACL entries. (cnauroth) + + HADOOP-10213. Fix bugs parsing ACL spec in FsShell setfacl. + (Vinay via cnauroth)