HADOOP-10352. Recursive setfacl erroneously attempts to apply default ACL to files. Contributed by Chris Nauroth.

git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/trunk@1570466 13f79535-47bb-0310-9956-ffa450edef68

hadoop-common-project/hadoop-common/CHANGES.txt

@@ -326,6 +326,9 @@ Trunk (Unreleased)
     HADOOP-10344. Fix TestAclCommands after merging HADOOP-10338 patch.
     (cnauroth)
 
+    HADOOP-10352. Recursive setfacl erroneously attempts to apply default ACL to
+    files. (cnauroth)
+
   OPTIMIZATIONS
 
     HADOOP-7761. Improve the performance of raw comparisons. (todd)

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/AclCommands.java

@@ -22,6 +22,8 @@ import java.util.Iterator;
 import java.util.LinkedList;
 import java.util.List;
 
+import com.google.common.collect.Lists;
+
 import org.apache.hadoop.HadoopIllegalArgumentException;
 import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.classification.InterfaceStability;
@@ -231,6 +233,7 @@ class AclCommands extends FsCommand {
     CommandFormat cf = new CommandFormat(0, Integer.MAX_VALUE, "b", "k", "R",
         "m", "x", "-set");
     List<AclEntry> aclEntries = null;
+    List<AclEntry> accessAclEntries = null;
 
     @Override
     protected void processOptions(LinkedList<String> args) throws IOException {
@@ -263,6 +266,19 @@ class AclCommands extends FsCommand {
       if (args.size() > 1) {
         throw new HadoopIllegalArgumentException("Too many arguments");
       }
+
+      // In recursive mode, save a separate list of just the access ACL entries.
+      // Only directories may have a default ACL.  When a recursive operation
+      // encounters a file under the specified path, it must pass only the
+      // access ACL entries.
+      if (isRecursive() && (oneModifyOption || setOption)) {
+        accessAclEntries = Lists.newArrayList();
+        for (AclEntry entry: aclEntries) {
+          if (entry.getScope() == AclEntryScope.ACCESS) {
+            accessAclEntries.add(entry);
+          }
+        }
+      }
     }
 
     @Override
@@ -272,11 +288,37 @@ class AclCommands extends FsCommand {
       } else if (cf.getOpt("k")) {
         item.fs.removeDefaultAcl(item.path);
       } else if (cf.getOpt("m")) {
-        item.fs.modifyAclEntries(item.path, aclEntries);
+        List<AclEntry> entries = getAclEntries(item);
+        if (!entries.isEmpty()) {
+          item.fs.modifyAclEntries(item.path, entries);
+        }
       } else if (cf.getOpt("x")) {
-        item.fs.removeAclEntries(item.path, aclEntries);
+        List<AclEntry> entries = getAclEntries(item);
+        if (!entries.isEmpty()) {
+          item.fs.removeAclEntries(item.path, entries);
+        }
       } else if (cf.getOpt("-set")) {
-        item.fs.setAcl(item.path, aclEntries);
+        List<AclEntry> entries = getAclEntries(item);
+        if (!entries.isEmpty()) {
+          item.fs.setAcl(item.path, entries);
+        }
+      }
+    }
+
+    /**
+     * Returns the ACL entries to use in the API call for the given path.  For a
+     * recursive operation, returns all specified ACL entries if the item is a
+     * directory or just the access ACL entries if the item is a file.  For a
+     * non-recursive operation, returns all specified ACL entries.
+     *
+     * @param item PathData path to check
+     * @return List<AclEntry> ACL entries to use in the API call
+     */
+    private List<AclEntry> getAclEntries(PathData item) {
+      if (isRecursive()) {
+        return item.stat.isDirectory() ? aclEntries : accessAclEntries;
+      } else {
+        return aclEntries;
       }
     }
   }

hadoop-hdfs-project/hadoop-hdfs/src/test/resources/testAclCLI.xml

@@ -911,5 +911,66 @@
         </comparator>
       </comparators>
     </test>
+    <test>
+      <description>setfacl: recursive modify entries with mix of files and directories</description>
+      <test-commands>
+        <command>-fs NAMENODE -mkdir -p /dir1</command>
+        <command>-fs NAMENODE -touchz /dir1/file1</command>
+        <command>-fs NAMENODE -mkdir -p /dir1/dir2</command>
+        <command>-fs NAMENODE -touchz /dir1/dir2/file2</command>
+        <command>-fs NAMENODE -setfacl -R -m user:charlie:rwx,default:user:charlie:r-x /dir1</command>
+        <command>-fs NAMENODE -getfacl -R /dir1</command>
+      </test-commands>
+      <cleanup-commands>
+        <command>-fs NAMENODE -rm -R /dir1</command>
+      </cleanup-commands>
+      <comparators>
+        <comparator>
+          <type>ExactComparator</type>
+          <expected-output># file: /dir1#LF## owner: USERNAME#LF## group: supergroup#LF#user::rwx#LF#user:charlie:rwx#LF#group::r-x#LF#mask::rwx#LF#other::r-x#LF#default:user::rwx#LF#default:user:charlie:r-x#LF#default:group::r-x#LF#default:mask::r-x#LF#default:other::r-x#LF##LF## file: /dir1/dir2#LF## owner: USERNAME#LF## group: supergroup#LF#user::rwx#LF#user:charlie:rwx#LF#group::r-x#LF#mask::rwx#LF#other::r-x#LF#default:user::rwx#LF#default:user:charlie:r-x#LF#default:group::r-x#LF#default:mask::r-x#LF#default:other::r-x#LF##LF## file: /dir1/dir2/file2#LF## owner: USERNAME#LF## group: supergroup#LF#user::rw-#LF#user:charlie:rwx#LF#group::r--#LF#mask::rwx#LF#other::r--#LF##LF## file: /dir1/file1#LF## owner: USERNAME#LF## group: supergroup#LF#user::rw-#LF#user:charlie:rwx#LF#group::r--#LF#mask::rwx#LF#other::r--#LF##LF#</expected-output>
+        </comparator>
+      </comparators>
+    </test>
+    <test>
+      <description>setfacl: recursive remove entries with mix of files and directories</description>
+      <test-commands>
+        <command>-fs NAMENODE -mkdir -p /dir1</command>
+        <command>-fs NAMENODE -touchz /dir1/file1</command>
+        <command>-fs NAMENODE -mkdir -p /dir1/dir2</command>
+        <command>-fs NAMENODE -touchz /dir1/dir2/file2</command>
+        <command>-fs NAMENODE -setfacl -R -m user:bob:rwx,user:charlie:rwx,default:user:bob:rwx,default:user:charlie:r-x /dir1</command>
+        <command>-fs NAMENODE -setfacl -R -x user:bob,default:user:bob /dir1</command>
+        <command>-fs NAMENODE -getfacl -R /dir1</command>
+      </test-commands>
+      <cleanup-commands>
+        <command>-fs NAMENODE -rm -R /dir1</command>
+      </cleanup-commands>
+      <comparators>
+        <comparator>
+          <type>ExactComparator</type>
+          <expected-output># file: /dir1#LF## owner: USERNAME#LF## group: supergroup#LF#user::rwx#LF#user:charlie:rwx#LF#group::r-x#LF#mask::rwx#LF#other::r-x#LF#default:user::rwx#LF#default:user:charlie:r-x#LF#default:group::r-x#LF#default:mask::r-x#LF#default:other::r-x#LF##LF## file: /dir1/dir2#LF## owner: USERNAME#LF## group: supergroup#LF#user::rwx#LF#user:charlie:rwx#LF#group::r-x#LF#mask::rwx#LF#other::r-x#LF#default:user::rwx#LF#default:user:charlie:r-x#LF#default:group::r-x#LF#default:mask::r-x#LF#default:other::r-x#LF##LF## file: /dir1/dir2/file2#LF## owner: USERNAME#LF## group: supergroup#LF#user::rw-#LF#user:charlie:rwx#LF#group::r--#LF#mask::rwx#LF#other::r--#LF##LF## file: /dir1/file1#LF## owner: USERNAME#LF## group: supergroup#LF#user::rw-#LF#user:charlie:rwx#LF#group::r--#LF#mask::rwx#LF#other::r--#LF##LF#</expected-output>
+        </comparator>
+      </comparators>
+    </test>
+    <test>
+      <description>setfacl: recursive set with mix of files and directories</description>
+      <test-commands>
+        <command>-fs NAMENODE -mkdir -p /dir1</command>
+        <command>-fs NAMENODE -touchz /dir1/file1</command>
+        <command>-fs NAMENODE -mkdir -p /dir1/dir2</command>
+        <command>-fs NAMENODE -touchz /dir1/dir2/file2</command>
+        <command>-fs NAMENODE -setfacl -R --set user::rwx,user:charlie:rwx,group::r-x,other::r-x,default:user:charlie:r-x /dir1</command>
+        <command>-fs NAMENODE -getfacl -R /dir1</command>
+      </test-commands>
+      <cleanup-commands>
+        <command>-fs NAMENODE -rm -R /dir1</command>
+      </cleanup-commands>
+      <comparators>
+        <comparator>
+          <type>ExactComparator</type>
+          <expected-output># file: /dir1#LF## owner: USERNAME#LF## group: supergroup#LF#user::rwx#LF#user:charlie:rwx#LF#group::r-x#LF#mask::rwx#LF#other::r-x#LF#default:user::rwx#LF#default:user:charlie:r-x#LF#default:group::r-x#LF#default:mask::r-x#LF#default:other::r-x#LF##LF## file: /dir1/dir2#LF## owner: USERNAME#LF## group: supergroup#LF#user::rwx#LF#user:charlie:rwx#LF#group::r-x#LF#mask::rwx#LF#other::r-x#LF#default:user::rwx#LF#default:user:charlie:r-x#LF#default:group::r-x#LF#default:mask::r-x#LF#default:other::r-x#LF##LF## file: /dir1/dir2/file2#LF## owner: USERNAME#LF## group: supergroup#LF#user::rwx#LF#user:charlie:rwx#LF#group::r-x#LF#mask::rwx#LF#other::r-x#LF##LF## file: /dir1/file1#LF## owner: USERNAME#LF## group: supergroup#LF#user::rwx#LF#user:charlie:rwx#LF#group::r-x#LF#mask::rwx#LF#other::r-x#LF##LF#</expected-output>
+        </comparator>
+      </comparators>
+    </test>
   </tests>
 </configuration>