HADOOP-15846. ABFS: fix mask related bugs in setAcl, modifyAclEntries and removeAclEntries.
Contributed by Junhua Gu. (cherry picked from commit 66715005f9e8f4f25faa352a06d142b75a029f0e) Project: http://git-wip-us.apache.org/repos/asf/hadoop/repo Commit: http://git-wip-us.apache.org/repos/asf/hadoop/commit/724c1500 Tree: http://git-wip-us.apache.org/repos/asf/hadoop/tree/724c1500 Diff: http://git-wip-us.apache.org/repos/asf/hadoop/diff/724c1500 Branch: refs/heads/trunk Commit: 724c15007beb9ca36f2f37c829f1daa366d95bbf Parents: 9317a61 Author: Junhua Gu <junhua...@microsoft.com> Authored: Thu Nov 8 17:21:40 2018 +0000 Committer: Steve Loughran <ste...@apache.org> Committed: Thu Nov 8 17:21:40 2018 +0000 ---------------------------------------------------------------------- .../fs/azurebfs/AzureBlobFileSystemStore.java | 20 +-- .../fs/azurebfs/services/AbfsAclHelper.java | 89 +++++++++-- .../azurebfs/ITestAzureBlobFilesystemAcl.java | 147 ++++++++++++++++++- 3 files changed, 225 insertions(+), 31 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/hadoop/blob/724c1500/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java ---------------------------------------------------------------------- diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java index 1ac1761..bfdbba8 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java @@ -626,17 +626,7 @@ public class AzureBlobFileSystemStore { final Map<String, String> aclEntries = AbfsAclHelper.deserializeAclSpec(op.getResult().getResponseHeader(HttpHeaderConfigurations.X_MS_ACL)); - for (Map.Entry<String, String> modifyAclEntry : modifyAclEntries.entrySet()) { - aclEntries.put(modifyAclEntry.getKey(), modifyAclEntry.getValue()); - } - - if (!modifyAclEntries.containsKey(AbfsHttpConstants.ACCESS_MASK)) { - aclEntries.remove(AbfsHttpConstants.ACCESS_MASK); - } - - if (!modifyAclEntries.containsKey(AbfsHttpConstants.DEFAULT_MASK)) { - aclEntries.remove(AbfsHttpConstants.DEFAULT_MASK); - } + AbfsAclHelper.modifyAclEntriesInternal(aclEntries, modifyAclEntries); client.setAcl(AbfsHttpConstants.FORWARD_SLASH + getRelativePath(path, true), AbfsAclHelper.serializeAclSpec(aclEntries), eTag); @@ -736,12 +726,8 @@ public class AzureBlobFileSystemStore { final String eTag = op.getResult().getResponseHeader(HttpHeaderConfigurations.ETAG); final Map<String, String> getAclEntries = AbfsAclHelper.deserializeAclSpec(op.getResult().getResponseHeader(HttpHeaderConfigurations.X_MS_ACL)); - for (Map.Entry<String, String> ace : getAclEntries.entrySet()) { - if (ace.getKey().startsWith("default:") && (ace.getKey() != AbfsHttpConstants.DEFAULT_MASK) - && !aclEntries.containsKey(ace.getKey())) { - aclEntries.put(ace.getKey(), ace.getValue()); - } - } + + AbfsAclHelper.setAclEntriesInternal(aclEntries, getAclEntries); client.setAcl(AbfsHttpConstants.FORWARD_SLASH + getRelativePath(path, true), AbfsAclHelper.serializeAclSpec(aclEntries), eTag); http://git-wip-us.apache.org/repos/asf/hadoop/blob/724c1500/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsAclHelper.java ---------------------------------------------------------------------- diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsAclHelper.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsAclHelper.java index c28da2c..34959a6 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsAclHelper.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsAclHelper.java @@ -44,12 +44,17 @@ public final class AbfsAclHelper { // not called } - public static Map<String, String> deserializeAclSpec(final String aclSpecString) { + public static Map<String, String> deserializeAclSpec(final String aclSpecString) throws AzureBlobFileSystemException { final Map<String, String> aclEntries = new HashMap<>(); - final String[] aclArray = aclSpecString.split(AbfsHttpConstants.COMMA); - for (String acl : aclArray) { - int idx = acl.lastIndexOf(AbfsHttpConstants.COLON); - aclEntries.put(acl.substring(0, idx), acl.substring(idx + 1)); + final String[] aceArray = aclSpecString.split(AbfsHttpConstants.COMMA); + for (String ace : aceArray) { + int idx = ace.lastIndexOf(AbfsHttpConstants.COLON); + final String key = ace.substring(0, idx); + final String val = ace.substring(idx + 1); + if (aclEntries.containsKey(key)) { + throw new InvalidAclOperationException("Duplicate acl entries are not allowed."); + } + aclEntries.put(key, val); } return aclEntries; } @@ -104,12 +109,22 @@ public final class AbfsAclHelper { } } } + + if (removeIndicationSet.contains(AbfsHttpConstants.ACCESS_MASK) && containsNamedAce(aclEntries, false)) { + throw new InvalidAclOperationException("Access mask is required when a named access acl is present."); + } + if (accessAclTouched) { if (removeIndicationSet.contains(AbfsHttpConstants.ACCESS_MASK)) { aclEntries.remove(AbfsHttpConstants.ACCESS_MASK); } recalculateMask(aclEntries, false); } + + if (removeIndicationSet.contains(AbfsHttpConstants.DEFAULT_MASK) && containsNamedAce(aclEntries, true)) { + throw new InvalidAclOperationException("Default mask is required when a named default acl is present."); + } + if (defaultAclTouched) { if (removeIndicationSet.contains(AbfsHttpConstants.DEFAULT_MASK)) { aclEntries.remove(AbfsHttpConstants.DEFAULT_MASK); @@ -127,6 +142,50 @@ public final class AbfsAclHelper { } } + public static void modifyAclEntriesInternal(Map<String, String> aclEntries, Map<String, String> toModifyEntries) + throws AzureBlobFileSystemException { + boolean namedAccessAclTouched = false; + boolean namedDefaultAclTouched = false; + + for (Map.Entry<String, String> toModifyEntry : toModifyEntries.entrySet()) { + aclEntries.put(toModifyEntry.getKey(), toModifyEntry.getValue()); + if (isNamedAce(toModifyEntry.getKey())) { + if (isDefaultAce(toModifyEntry.getKey())) { + namedDefaultAclTouched = true; + } else { + namedAccessAclTouched = true; + } + } + } + + if (!toModifyEntries.containsKey(AbfsHttpConstants.ACCESS_MASK) && namedAccessAclTouched) { + aclEntries.remove(AbfsHttpConstants.ACCESS_MASK); + } + + if (!toModifyEntries.containsKey(AbfsHttpConstants.DEFAULT_MASK) && namedDefaultAclTouched) { + aclEntries.remove(AbfsHttpConstants.DEFAULT_MASK); + } + } + + public static void setAclEntriesInternal(Map<String, String> aclEntries, Map<String, String> getAclEntries) + throws AzureBlobFileSystemException { + boolean defaultAclTouched = false; + + for (String entryKey : aclEntries.keySet()) { + if (isDefaultAce(entryKey)) { + defaultAclTouched = true; + break; + } + } + + for (Map.Entry<String, String> ace : getAclEntries.entrySet()) { + if (AbfsAclHelper.isDefaultAce(ace.getKey()) && (ace.getKey() != AbfsHttpConstants.DEFAULT_MASK || !defaultAclTouched) + && !aclEntries.containsKey(ace.getKey())) { + aclEntries.put(ace.getKey(), ace.getValue()); + } + } + } + private static boolean removeNamedAceAndUpdateSet(String entry, boolean isDefaultAcl, Set<String> removeIndicationSet, Map<String, String> aclEntries) throws AzureBlobFileSystemException { @@ -136,8 +195,7 @@ public final class AbfsAclHelper { : entryParts[startIndex] + AbfsHttpConstants.COLON; if ((entry.equals(AbfsHttpConstants.ACCESS_USER) || entry.equals(AbfsHttpConstants.ACCESS_GROUP) - || entry.equals(AbfsHttpConstants.ACCESS_OTHER)) - && !isNamedAce(entry)) { + || entry.equals(AbfsHttpConstants.ACCESS_OTHER))) { throw new InvalidAclOperationException("Cannot remove user, group or other entry from access ACL."); } @@ -154,7 +212,7 @@ public final class AbfsAclHelper { } private static void recalculateMask(Map<String, String> aclEntries, boolean isDefaultMask) { - FsAction umask = FsAction.NONE; + FsAction mask = FsAction.NONE; if (!isExtendAcl(aclEntries, isDefaultMask)) { return; } @@ -163,17 +221,17 @@ public final class AbfsAclHelper { if (isDefaultMask) { if ((isDefaultAce(aclEntry.getKey()) && isNamedAce(aclEntry.getKey())) || aclEntry.getKey().equals(AbfsHttpConstants.DEFAULT_GROUP)) { - umask = umask.or(FsAction.getFsAction(aclEntry.getValue())); + mask = mask.or(FsAction.getFsAction(aclEntry.getValue())); } } else { if ((!isDefaultAce(aclEntry.getKey()) && isNamedAce(aclEntry.getKey())) || aclEntry.getKey().equals(AbfsHttpConstants.ACCESS_GROUP)) { - umask = umask.or(FsAction.getFsAction(aclEntry.getValue())); + mask = mask.or(FsAction.getFsAction(aclEntry.getValue())); } } } - aclEntries.put(isDefaultMask ? AbfsHttpConstants.DEFAULT_MASK : AbfsHttpConstants.ACCESS_MASK, umask.SYMBOL); + aclEntries.put(isDefaultMask ? AbfsHttpConstants.DEFAULT_MASK : AbfsHttpConstants.ACCESS_MASK, mask.SYMBOL); } private static boolean isExtendAcl(Map<String, String> aclEntries, boolean checkDefault) { @@ -192,6 +250,15 @@ public final class AbfsAclHelper { return false; } + private static boolean containsNamedAce(Map<String, String> aclEntries, boolean checkDefault) { + for (String entryKey : aclEntries.keySet()) { + if (isNamedAce(entryKey) && (checkDefault == isDefaultAce(entryKey))) { + return true; + } + } + return false; + } + private static boolean isDefaultAce(String entry) { return entry.startsWith(AbfsHttpConstants.DEFAULT_SCOPE); } http://git-wip-us.apache.org/repos/asf/hadoop/blob/724c1500/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFilesystemAcl.java ---------------------------------------------------------------------- diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFilesystemAcl.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFilesystemAcl.java index 7377132..05f9856 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFilesystemAcl.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFilesystemAcl.java @@ -30,6 +30,7 @@ import org.junit.Test; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.PathIOException; import org.apache.hadoop.fs.FileStatus; import org.apache.hadoop.fs.azurebfs.utils.AclTestHelpers; import org.apache.hadoop.fs.permission.AclEntry; @@ -54,6 +55,7 @@ public class ITestAzureBlobFilesystemAcl extends AbstractAbfsIntegrationTest { private static final FsAction ALL = FsAction.ALL; private static final FsAction NONE = FsAction.NONE; private static final FsAction READ = FsAction.READ; + private static final FsAction EXECUTE = FsAction.EXECUTE; private static final FsAction READ_EXECUTE = FsAction.READ_EXECUTE; private static final FsAction READ_WRITE = FsAction.READ_WRITE; @@ -65,6 +67,7 @@ public class ITestAzureBlobFilesystemAcl extends AbstractAbfsIntegrationTest { private static final short RWX_RX = 0750; private static final short RWX_RX_RX = 0755; private static final short RW_R = 0640; + private static final short RW_X = 0610; private static final short RW_RW = 0660; private static final short RW_RWX = 0670; private static final short RW_R_R = 0644; @@ -271,6 +274,67 @@ public class ITestAzureBlobFilesystemAcl extends AbstractAbfsIntegrationTest { } @Test + public void testModifyAclEntriesWithDefaultMask() throws Exception { + final AzureBlobFileSystem fs = this.getFileSystem(); + assumeTrue(fs.getIsNamespaceEnabled()); + path = new Path(testRoot, UUID.randomUUID().toString()); + FileSystem.mkdirs(fs, path, FsPermission.createImmutable((short) RWX_RX)); + List<AclEntry> aclSpec = Lists.newArrayList( + aclEntry(DEFAULT, MASK, EXECUTE)); + fs.setAcl(path, aclSpec); + + List<AclEntry> modifyAclSpec = Lists.newArrayList( + aclEntry(DEFAULT, USER, READ_WRITE)); + fs.modifyAclEntries(path, modifyAclSpec); + + AclStatus s = fs.getAclStatus(path); + AclEntry[] returned = s.getEntries().toArray(new AclEntry[0]); + assertArrayEquals(new AclEntry[] { + aclEntry(DEFAULT, USER, READ_WRITE), + aclEntry(DEFAULT, GROUP, READ_EXECUTE), + aclEntry(DEFAULT, MASK, EXECUTE), + aclEntry(DEFAULT, OTHER, NONE)}, returned); + assertPermission(fs, (short) RWX_RX); + } + + @Test + public void testModifyAclEntriesWithAccessMask() throws Exception { + final AzureBlobFileSystem fs = this.getFileSystem(); + assumeTrue(fs.getIsNamespaceEnabled()); + path = new Path(testRoot, UUID.randomUUID().toString()); + FileSystem.mkdirs(fs, path, FsPermission.createImmutable((short) RWX_RX)); + List<AclEntry> aclSpec = Lists.newArrayList( + aclEntry(ACCESS, MASK, EXECUTE)); + fs.setAcl(path, aclSpec); + + List<AclEntry> modifyAclSpec = Lists.newArrayList( + aclEntry(ACCESS, USER, READ_WRITE)); + fs.modifyAclEntries(path, modifyAclSpec); + + AclStatus s = fs.getAclStatus(path); + AclEntry[] returned = s.getEntries().toArray(new AclEntry[0]); + assertArrayEquals(new AclEntry[] { + aclEntry(ACCESS, GROUP, READ_EXECUTE)}, returned); + assertPermission(fs, (short) RW_X); + } + + @Test(expected=PathIOException.class) + public void testModifyAclEntriesWithDuplicateEntries() throws Exception { + final AzureBlobFileSystem fs = this.getFileSystem(); + assumeTrue(fs.getIsNamespaceEnabled()); + path = new Path(testRoot, UUID.randomUUID().toString()); + FileSystem.mkdirs(fs, path, FsPermission.createImmutable((short) RWX_RX)); + List<AclEntry> aclSpec = Lists.newArrayList( + aclEntry(ACCESS, MASK, EXECUTE)); + fs.setAcl(path, aclSpec); + + List<AclEntry> modifyAclSpec = Lists.newArrayList( + aclEntry(ACCESS, USER, READ_WRITE), + aclEntry(ACCESS, USER, READ_WRITE)); + fs.modifyAclEntries(path, modifyAclSpec); + } + + @Test public void testRemoveAclEntries() throws Exception { final AzureBlobFileSystem fs = this.getFileSystem(); assumeTrue(fs.getIsNamespaceEnabled()); @@ -440,6 +504,50 @@ public class ITestAzureBlobFilesystemAcl extends AbstractAbfsIntegrationTest { fs.removeAclEntries(path, aclSpec); } + @Test(expected=PathIOException.class) + public void testRemoveAclEntriesAccessMask() throws Exception { + final AzureBlobFileSystem fs = this.getFileSystem(); + assumeTrue(fs.getIsNamespaceEnabled()); + path = new Path(testRoot, UUID.randomUUID().toString()); + FileSystem.mkdirs(fs, path, FsPermission.createImmutable((short) RWX_RX)); + List<AclEntry> aclSpec = Lists.newArrayList( + aclEntry(ACCESS, MASK, EXECUTE), + aclEntry(ACCESS, USER, "foo", ALL)); + fs.setAcl(path, aclSpec); + + fs.removeAclEntries(path, Lists.newArrayList(aclEntry(ACCESS, MASK, NONE))); + } + + @Test(expected=PathIOException.class) + public void testRemoveAclEntriesDefaultMask() throws Exception { + final AzureBlobFileSystem fs = this.getFileSystem(); + assumeTrue(fs.getIsNamespaceEnabled()); + path = new Path(testRoot, UUID.randomUUID().toString()); + FileSystem.mkdirs(fs, path, FsPermission.createImmutable((short) RWX_RX)); + List<AclEntry> aclSpec = Lists.newArrayList( + aclEntry(DEFAULT, MASK, EXECUTE), + aclEntry(DEFAULT, USER, "foo", ALL)); + fs.setAcl(path, aclSpec); + + fs.removeAclEntries(path, Lists.newArrayList(aclEntry(DEFAULT, MASK, NONE))); + } + + @Test(expected=PathIOException.class) + public void testRemoveAclEntriesWithDuplicateEntries() throws Exception { + final AzureBlobFileSystem fs = this.getFileSystem(); + assumeTrue(fs.getIsNamespaceEnabled()); + path = new Path(testRoot, UUID.randomUUID().toString()); + FileSystem.mkdirs(fs, path, FsPermission.createImmutable((short) RWX_RX)); + List<AclEntry> aclSpec = Lists.newArrayList( + aclEntry(DEFAULT, MASK, EXECUTE)); + fs.setAcl(path, aclSpec); + + List<AclEntry> removeAclSpec = Lists.newArrayList( + aclEntry(DEFAULT, USER, READ_WRITE), + aclEntry(DEFAULT, USER, READ_WRITE)); + fs.removeAclEntries(path, removeAclSpec); + } + @Test public void testRemoveDefaultAcl() throws Exception { final AzureBlobFileSystem fs = this.getFileSystem(); @@ -814,6 +922,42 @@ public class ITestAzureBlobFilesystemAcl extends AbstractAbfsIntegrationTest { } @Test + public void testSetAclDoesNotChangeDefaultMask() throws Exception { + final AzureBlobFileSystem fs = this.getFileSystem(); + assumeTrue(fs.getIsNamespaceEnabled()); + path = new Path(testRoot, UUID.randomUUID().toString()); + FileSystem.mkdirs(fs, path, FsPermission.createImmutable((short) RWX_RX)); + List<AclEntry> aclSpec = Lists.newArrayList( + aclEntry(DEFAULT, MASK, EXECUTE)); + fs.setAcl(path, aclSpec); + // only change access acl, and default mask should not change. + List<AclEntry> aclSpec2 = Lists.newArrayList( + aclEntry(ACCESS, OTHER, READ_EXECUTE)); + fs.setAcl(path, aclSpec2); + // get acl status and check result. + AclStatus s = fs.getAclStatus(path); + AclEntry[] returned = s.getEntries().toArray(new AclEntry[0]); + assertArrayEquals(new AclEntry[] { + aclEntry(DEFAULT, USER, ALL), + aclEntry(DEFAULT, GROUP, READ_EXECUTE), + aclEntry(DEFAULT, MASK, EXECUTE), + aclEntry(DEFAULT, OTHER, NONE) }, returned); + assertPermission(fs, (short) RWX_RX_RX); + } + + @Test(expected=PathIOException.class) + public void testSetAclWithDuplicateEntries() throws Exception { + final AzureBlobFileSystem fs = this.getFileSystem(); + assumeTrue(fs.getIsNamespaceEnabled()); + path = new Path(testRoot, UUID.randomUUID().toString()); + FileSystem.mkdirs(fs, path, FsPermission.createImmutable((short) RWX_RX)); + List<AclEntry> aclSpec = Lists.newArrayList( + aclEntry(ACCESS, MASK, EXECUTE), + aclEntry(ACCESS, MASK, EXECUTE)); + fs.setAcl(path, aclSpec); + } + + @Test public void testSetPermission() throws Exception { final AzureBlobFileSystem fs = this.getFileSystem(); assumeTrue(fs.getIsNamespaceEnabled()); @@ -924,7 +1068,6 @@ public class ITestAzureBlobFilesystemAcl extends AbstractAbfsIntegrationTest { } @Test - @Ignore // wait investigation in service public void testDefaultMinimalAclNewFile() throws Exception { final AzureBlobFileSystem fs = this.getFileSystem(); assumeTrue(fs.getIsNamespaceEnabled()); @@ -970,7 +1113,6 @@ public class ITestAzureBlobFilesystemAcl extends AbstractAbfsIntegrationTest { } @Test - @Ignore // wait umask fix to be deployed public void testOnlyAccessAclNewDir() throws Exception { final AzureBlobFileSystem fs = this.getFileSystem(); assumeTrue(fs.getIsNamespaceEnabled()); @@ -988,7 +1130,6 @@ public class ITestAzureBlobFilesystemAcl extends AbstractAbfsIntegrationTest { } @Test - @Ignore // wait investigation in service public void testDefaultMinimalAclNewDir() throws Exception { final AzureBlobFileSystem fs = this.getFileSystem(); assumeTrue(fs.getIsNamespaceEnabled()); --------------------------------------------------------------------- To unsubscribe, e-mail: common-commits-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-commits-h...@hadoop.apache.org
