HADOOP-9768. chown and chgrp reject users and groups with spaces on platforms where spaces are otherwise acceptable. Contributed by Chris Nauroth.

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

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

@@ -660,6 +660,9 @@ Release 2.1.0-beta - 2013-07-02
     HADOOP-9507. LocalFileSystem rename() is broken in some cases when
     destination exists. (cnauroth)
 
+    HADOOP-9768. chown and chgrp reject users and groups with spaces on platforms
+    where spaces are otherwise acceptable. (cnauroth)
+
   BREAKDOWN OF HADOOP-8562 SUBTASKS AND RELATED JIRAS
 
     HADOOP-8924. Hadoop Common creating package-info.java must not depend on

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

@@ -31,7 +31,7 @@ import org.apache.hadoop.fs.shell.CommandFactory;
 import org.apache.hadoop.fs.shell.CommandFormat;
 import org.apache.hadoop.fs.shell.FsCommand;
 import org.apache.hadoop.fs.shell.PathData;
-
+import org.apache.hadoop.util.Shell;
 
 /**
  * This class is the home for file permissions related commands.
@@ -111,7 +111,8 @@ public class FsShellPermissions extends FsCommand {
   }
   
   // used by chown/chgrp
-  static private String allowedChars = "[-_./@a-zA-Z0-9]";  
+  static private String allowedChars = Shell.WINDOWS ? "[-_./@a-zA-Z0-9 ]" :
+    "[-_./@a-zA-Z0-9]";
 
   /**
    * Used to change owner and/or group of files 
@@ -126,9 +127,8 @@ public class FsShellPermissions extends FsCommand {
       "\tcurrently supported.\n\n" +
       "\tIf only owner or group is specified then only owner or\n" +
       "\tgroup is modified.\n\n" +
-      "\tThe owner and group names may only cosists of digits, alphabet,\n"+
-      "\tand any of '-_.@/' i.e. [-_.@/a-zA-Z0-9]. The names are case\n" +
-      "\tsensitive.\n\n" +
+      "\tThe owner and group names may only consist of digits, alphabet,\n"+
+      "\tand any of " + allowedChars + ". The names are case sensitive.\n\n" +
       "\tWARNING: Avoid using '.' to separate user name and group though\n" +
       "\tLinux allows it. If user names have dots in them and you are\n" +
       "\tusing local file system, you might see surprising results since\n" +

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

@@ -20,11 +20,13 @@ package org.apache.hadoop.fs;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.InterruptedIOException;
 import java.io.PrintStream;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.LinkedList;
@@ -38,6 +40,7 @@ import org.apache.hadoop.fs.shell.FsCommand;
 import org.apache.hadoop.fs.shell.PathData;
 import org.apache.hadoop.io.IOUtils;
 import static org.apache.hadoop.fs.CommonConfigurationKeysPublic.FS_DEFAULT_NAME_KEY;
+import org.apache.hadoop.util.Shell;
 import org.junit.BeforeClass;
 import org.junit.Test;
 
@@ -407,6 +410,65 @@ public class TestFsShellReturnCode {
     assertEquals(2, InterruptCommand.processed);
     assertEquals(130, exitCode);
   }
+
+  /**
+   * Tests combinations of valid and invalid user and group arguments to chown.
+   */
+  @Test
+  public void testChownUserAndGroupValidity() {
+    // This test only covers argument parsing, so override to skip processing.
+    FsCommand chown = new FsShellPermissions.Chown() {
+      @Override
+      protected void processArgument(PathData item) {
+      }
+    };
+    chown.setConf(new Configuration());
+
+    // The following are valid (no exception expected).
+    chown.run("user", "/path");
+    chown.run("user:group", "/path");
+    chown.run(":group", "/path");
+
+    // The following are valid only on Windows.
+    assertValidArgumentsOnWindows(chown, "User With Spaces", "/path");
+    assertValidArgumentsOnWindows(chown, "User With Spaces:group", "/path");
+    assertValidArgumentsOnWindows(chown, "User With Spaces:Group With Spaces",
+      "/path");
+    assertValidArgumentsOnWindows(chown, "user:Group With Spaces", "/path");
+    assertValidArgumentsOnWindows(chown, ":Group With Spaces", "/path");
+
+    // The following are invalid (exception expected).
+    assertIllegalArguments(chown, "us!er", "/path");
+    assertIllegalArguments(chown, "us^er", "/path");
+    assertIllegalArguments(chown, "user:gr#oup", "/path");
+    assertIllegalArguments(chown, "user:gr%oup", "/path");
+    assertIllegalArguments(chown, ":gr#oup", "/path");
+    assertIllegalArguments(chown, ":gr%oup", "/path");
+  }
+
+  /**
+   * Tests valid and invalid group arguments to chgrp.
+   */
+  @Test
+  public void testChgrpGroupValidity() {
+    // This test only covers argument parsing, so override to skip processing.
+    FsCommand chgrp = new FsShellPermissions.Chgrp() {
+      @Override
+      protected void processArgument(PathData item) {
+      }
+    };
+    chgrp.setConf(new Configuration());
+
+    // The following are valid (no exception expected).
+    chgrp.run("group", "/path");
+
+    // The following are valid only on Windows.
+    assertValidArgumentsOnWindows(chgrp, "Group With Spaces", "/path");
+
+    // The following are invalid (exception expected).
+    assertIllegalArguments(chgrp, ":gr#oup", "/path");
+    assertIllegalArguments(chgrp, ":gr%oup", "/path");
+  }
   
   static class LocalFileSystemExtn extends LocalFileSystem {
     public LocalFileSystemExtn() {
@@ -480,4 +542,37 @@ public class TestFsShellReturnCode {
       }
     }
   }  
+
+  /**
+   * Asserts that for the given command, the given arguments are considered
+   * invalid.  The expectation is that the command will throw
+   * IllegalArgumentException.
+   * 
+   * @param cmd FsCommand to check
+   * @param args String... arguments to check
+   */
+  private static void assertIllegalArguments(FsCommand cmd, String... args) {
+    try {
+      cmd.run(args);
+      fail("Expected IllegalArgumentException from args: " +
+        Arrays.toString(args));
+    } catch (IllegalArgumentException e) {
+    }
+  }
+
+  /**
+   * Asserts that for the given command, the given arguments are considered valid
+   * on Windows, but invalid elsewhere.
+   * 
+   * @param cmd FsCommand to check
+   * @param args String... arguments to check
+   */
+  private static void assertValidArgumentsOnWindows(FsCommand cmd,
+      String... args) {
+    if (Shell.WINDOWS) {
+      cmd.run(args);
+    } else {
+      assertIllegalArguments(cmd, args);
+    }
+  }
 }

hadoop-common-project/hadoop-common/src/test/resources/testConf.xml

@@ -779,15 +779,11 @@
         </comparator>
         <comparator>
           <type>RegexpComparator</type>
-          <expected-output>^( |\t)*The owner and group names may only cosists of digits, alphabet,( )*</expected-output>
+          <expected-output>^( |\t)*The owner and group names may only consist of digits, alphabet,( )*</expected-output>
         </comparator>
         <comparator>
           <type>RegexpComparator</type>
-          <expected-output>^( |\t)*and any of '-_.@/' i.e. \[-_.@/a-zA-Z0-9\]. The names are case( )*</expected-output>
-        </comparator>
-        <comparator>
-          <type>RegexpComparator</type>
-          <expected-output>^( |\t)*sensitive.( )*</expected-output>
+          <expected-output>^( |\t)*and any of .+?. The names are case sensitive.( )*</expected-output>
         </comparator>
         <comparator>
           <type>RegexpComparator</type>