HADOOP-10277. setfacl -x fails to parse ACL spec if trying to remove the mask entry. Contributed by Vinay.

git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/branches/HDFS-4685@1561769 13f79535-47bb-0310-9956-ffa450edef68

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/permission/AclEntry.java

@@ -210,7 +210,7 @@ private AclEntry(AclEntryType type, String name, FsAction permission, AclEntrySc
    *          But for removeAcl operation it will be false. i.e. AclSpec should
    *          not contain permissions.<br>
    *          Example: "user:foo,group:bar"
-   * @return Returns list of AclEntries parsed
+   * @return Returns list of {@link AclEntry} parsed
    */
   public static List<AclEntry> parseAclSpec(String aclSpec,
       boolean includePermission) {
@@ -218,53 +218,84 @@ public static List<AclEntry> parseAclSpec(String aclSpec,
     Collection<String> aclStrings = StringUtils.getStringCollection(aclSpec,
         ",");
     for (String aclStr : aclStrings) {
-      AclEntry.Builder builder = new AclEntry.Builder();
+      AclEntry aclEntry = parseAclEntry(aclStr, includePermission);
-      // Here "::" represent one empty string.
+      aclEntries.add(aclEntry);
-      // 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 <aclSpec> : "
-            + 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 <aclSpec> :" + 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 <aclSpec> : " + aclStr);
-        }
-        builder.setPermission(fsAction);
-      }
-      aclEntries.add(builder.build());
     }
     return aclEntries;
   }
+
+  /**
+   * Parses a string representation of an ACL into a AclEntry object.<br>
+   * 
+   * @param aclStr
+   *          String representation of an ACL.<br>
+   *          Example: "user:foo:rw-"
+   * @param includePermission
+   *          for setAcl operations this will be true. i.e. Acl should include
+   *          permissions.<br>
+   *          But for removeAcl operation it will be false. i.e. Acl should not
+   *          contain permissions.<br>
+   *          Example: "user:foo,group:bar,mask::"
+   * @return Returns an {@link AclEntry} object
+   */
+  public static AclEntry parseAclEntry(String aclStr,
+      boolean includePermission) {
+    AclEntry.Builder builder = new AclEntry.Builder();
+    // Here "::" represent one empty string.
+    // StringUtils.getStringCollection() will ignore this.
+    String[] split = aclStr.split(":");
+
+    if (split.length == 0) {
+      throw new HadoopIllegalArgumentException("Invalid <aclSpec> : " + aclStr);
+    }
+    int index = 0;
+    if ("default".equals(split[0])) {
+      // default entry
+      index++;
+      builder.setScope(AclEntryScope.DEFAULT);
+    }
+
+    if (split.length <= index) {
+      throw new HadoopIllegalArgumentException("Invalid <aclSpec> : " + aclStr);
+    }
+
+    AclEntryType aclType = null;
+    try {
+      aclType = Enum.valueOf(AclEntryType.class, split[index].toUpperCase());
+      builder.setType(aclType);
+      index++;
+    } catch (IllegalArgumentException iae) {
+      throw new HadoopIllegalArgumentException(
+          "Invalid type of acl in <aclSpec> :" + aclStr);
+    }
+
+    if (split.length > index) {
+      String name = split[index];
+      if (!name.isEmpty()) {
+        builder.setName(name);
+      }
+      index++;
+    }
+
+    if (includePermission) {
+      if (split.length < index) {
+        throw new HadoopIllegalArgumentException("Invalid <aclSpec> : "
+            + aclStr);
+      }
+      String permission = split[index];
+      FsAction fsAction = FsAction.getFsAction(permission);
+      if (null == fsAction) {
+        throw new HadoopIllegalArgumentException(
+            "Invalid permission in <aclSpec> : " + aclStr);
+      }
+      builder.setPermission(fsAction);
+      index++;
+    }
+
+    if (split.length > index) {
+      throw new HadoopIllegalArgumentException("Invalid <aclSpec> : " + aclStr);
+    }
+    AclEntry aclEntry = builder.build();
+    return aclEntry;
+  }
 }

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestAclCommands.java

@@ -66,7 +66,7 @@ public void testSetfaclValidations() throws Exception {
     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",
+    assertFalse("setfacl should fail ACL spec missing",
         0 == runCommand(new String[] { "-setfacl", "-m",
             "", "/path" }));
   }
@@ -74,7 +74,8 @@ public void testSetfaclValidations() throws Exception {
   @Test
   public void testMultipleAclSpecParsing() throws Exception {
     List<AclEntry> parsedList = AclEntry.parseAclSpec(
-        "group::rwx,user:user1:rwx,user:user2:rw-,group:group1:rw-,default:group:group1:rw-", true);
+        "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();
@@ -96,6 +97,37 @@ public void testMultipleAclSpecParsing() throws Exception {
     assertEquals("Parsed Acl not correct", expectedList, parsedList);
   }
 
+  @Test
+  public void testMultipleAclSpecParsingWithoutPermissions() throws Exception {
+    List<AclEntry> parsedList = AclEntry.parseAclSpec(
+        "user::,user:user1:,group::,group:group1:,mask::,other::,"
+            + "default:user:user1::,default:mask::", false);
+
+    AclEntry owner = new AclEntry.Builder().setType(AclEntryType.USER).build();
+    AclEntry namedUser = new AclEntry.Builder().setType(AclEntryType.USER)
+        .setName("user1").build();
+    AclEntry group = new AclEntry.Builder().setType(AclEntryType.GROUP).build();
+    AclEntry namedGroup = new AclEntry.Builder().setType(AclEntryType.GROUP)
+        .setName("group1").build();
+    AclEntry mask = new AclEntry.Builder().setType(AclEntryType.MASK).build();
+    AclEntry other = new AclEntry.Builder().setType(AclEntryType.OTHER).build();
+    AclEntry defaultUser = new AclEntry.Builder()
+        .setScope(AclEntryScope.DEFAULT).setType(AclEntryType.USER)
+        .setName("user1").build();
+    AclEntry defaultMask = new AclEntry.Builder()
+        .setScope(AclEntryScope.DEFAULT).setType(AclEntryType.MASK).build();
+    List<AclEntry> expectedList = new ArrayList<AclEntry>();
+    expectedList.add(owner);
+    expectedList.add(namedUser);
+    expectedList.add(group);
+    expectedList.add(namedGroup);
+    expectedList.add(mask);
+    expectedList.add(other);
+    expectedList.add(defaultUser);
+    expectedList.add(defaultMask);
+    assertEquals("Parsed Acl not correct", expectedList, parsedList);
+  }
+
   private int runCommand(String[] commands) throws Exception {
     return ToolRunner.run(conf, new FsShell(), commands);
   }

hadoop-hdfs-project/hadoop-hdfs/CHANGES-HDFS-4685.txt

@@ -58,3 +58,6 @@ HDFS-4685 (Unreleased)
     (Vinay via cnauroth)
 
     HDFS-5799. Make audit logging consistent across ACL APIs. (cnauroth)
+
+    HADOOP-10277. setfacl -x fails to parse ACL spec if trying to remove the
+    mask entry. (Vinay via cnauroth)