From 327d2ceca22119a73ea05ddf4620e5253a2fded6 Mon Sep 17 00:00:00 2001 From: Chris Nauroth Date: Fri, 21 Feb 2014 06:29:52 +0000 Subject: [PATCH] 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/CHANGES.txt | 3 + .../apache/hadoop/fs/shell/AclCommands.java | 48 ++++++++++++++- .../src/test/resources/testAclCLI.xml | 61 +++++++++++++++++++ 3 files changed, 109 insertions(+), 3 deletions(-) diff --git a/hadoop-common-project/hadoop-common/CHANGES.txt b/hadoop-common-project/hadoop-common/CHANGES.txt index 7711888d8f..0ec69f9515 100644 --- a/hadoop-common-project/hadoop-common/CHANGES.txt +++ b/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) 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 9628487d17..6e10e7223e 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 @@ -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 aclEntries = null; + List accessAclEntries = null; @Override protected void processOptions(LinkedList 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 entries = getAclEntries(item); + if (!entries.isEmpty()) { + item.fs.modifyAclEntries(item.path, entries); + } } else if (cf.getOpt("x")) { - item.fs.removeAclEntries(item.path, aclEntries); + List entries = getAclEntries(item); + if (!entries.isEmpty()) { + item.fs.removeAclEntries(item.path, entries); + } } else if (cf.getOpt("-set")) { - item.fs.setAcl(item.path, aclEntries); + List 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 ACL entries to use in the API call + */ + private List getAclEntries(PathData item) { + if (isRecursive()) { + return item.stat.isDirectory() ? aclEntries : accessAclEntries; + } else { + return aclEntries; } } } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/resources/testAclCLI.xml b/hadoop-hdfs-project/hadoop-hdfs/src/test/resources/testAclCLI.xml index f4aa0bb075..a7491511be 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/resources/testAclCLI.xml +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/resources/testAclCLI.xml @@ -911,5 +911,66 @@ + + setfacl: recursive modify entries with mix of files and directories + + -fs NAMENODE -mkdir -p /dir1 + -fs NAMENODE -touchz /dir1/file1 + -fs NAMENODE -mkdir -p /dir1/dir2 + -fs NAMENODE -touchz /dir1/dir2/file2 + -fs NAMENODE -setfacl -R -m user:charlie:rwx,default:user:charlie:r-x /dir1 + -fs NAMENODE -getfacl -R /dir1 + + + -fs NAMENODE -rm -R /dir1 + + + + ExactComparator + # 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# + + + + + setfacl: recursive remove entries with mix of files and directories + + -fs NAMENODE -mkdir -p /dir1 + -fs NAMENODE -touchz /dir1/file1 + -fs NAMENODE -mkdir -p /dir1/dir2 + -fs NAMENODE -touchz /dir1/dir2/file2 + -fs NAMENODE -setfacl -R -m user:bob:rwx,user:charlie:rwx,default:user:bob:rwx,default:user:charlie:r-x /dir1 + -fs NAMENODE -setfacl -R -x user:bob,default:user:bob /dir1 + -fs NAMENODE -getfacl -R /dir1 + + + -fs NAMENODE -rm -R /dir1 + + + + ExactComparator + # 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# + + + + + setfacl: recursive set with mix of files and directories + + -fs NAMENODE -mkdir -p /dir1 + -fs NAMENODE -touchz /dir1/file1 + -fs NAMENODE -mkdir -p /dir1/dir2 + -fs NAMENODE -touchz /dir1/dir2/file2 + -fs NAMENODE -setfacl -R --set user::rwx,user:charlie:rwx,group::r-x,other::r-x,default:user:charlie:r-x /dir1 + -fs NAMENODE -getfacl -R /dir1 + + + -fs NAMENODE -rm -R /dir1 + + + + ExactComparator + # 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# + + +