From 33552b664b6e14cc3ebced78ab3b03e0582a830a Mon Sep 17 00:00:00 2001 From: Chris Nauroth Date: Tue, 30 Jul 2013 05:28:43 +0000 Subject: [PATCH] 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/CHANGES.txt | 3 + .../apache/hadoop/fs/FsShellPermissions.java | 10 +- .../hadoop/fs/TestFsShellReturnCode.java | 95 +++++++++++++++++++ .../src/test/resources/testConf.xml | 8 +- 4 files changed, 105 insertions(+), 11 deletions(-) diff --git a/hadoop-common-project/hadoop-common/CHANGES.txt b/hadoop-common-project/hadoop-common/CHANGES.txt index db9efb69445..83d44a07de3 100644 --- a/hadoop-common-project/hadoop-common/CHANGES.txt +++ b/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 diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FsShellPermissions.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FsShellPermissions.java index 41c9f4b3670..5ac10ceda7b 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FsShellPermissions.java +++ b/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" + diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFsShellReturnCode.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFsShellReturnCode.java index d64292b39df..dcc19df3d4e 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFsShellReturnCode.java +++ b/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); + } + } } diff --git a/hadoop-common-project/hadoop-common/src/test/resources/testConf.xml b/hadoop-common-project/hadoop-common/src/test/resources/testConf.xml index 13459094d65..69886abb84b 100644 --- a/hadoop-common-project/hadoop-common/src/test/resources/testConf.xml +++ b/hadoop-common-project/hadoop-common/src/test/resources/testConf.xml @@ -779,15 +779,11 @@ RegexpComparator - ^( |\t)*The owner and group names may only cosists of digits, alphabet,( )* + ^( |\t)*The owner and group names may only consist of digits, alphabet,( )* RegexpComparator - ^( |\t)*and any of '-_.@/' i.e. \[-_.@/a-zA-Z0-9\]. The names are case( )* - - - RegexpComparator - ^( |\t)*sensitive.( )* + ^( |\t)*and any of .+?. The names are case sensitive.( )* RegexpComparator