HADOOP-15846. ABFS: fix mask related bugs in setAcl, modifyAclEntries and removeAclEntries.

Contributed by Junhua Gu.

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()) {
+    AbfsAclHelper.modifyAclEntriesInternal(aclEntries, modifyAclEntries);
-      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);
-    }
 
     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)
+    AbfsAclHelper.setAclEntriesInternal(aclEntries, getAclEntries);
-              && !aclEntries.containsKey(ace.getKey())) {
-        aclEntries.put(ace.getKey(), ace.getValue());
-      }
-    }
 
     client.setAcl(AbfsHttpConstants.FORWARD_SLASH + getRelativePath(path, true),
         AbfsAclHelper.serializeAclSpec(aclEntries), eTag);

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);
+    final String[] aceArray = aclSpecString.split(AbfsHttpConstants.COMMA);
-    for (String acl : aclArray) {
+    for (String ace : aceArray) {
-      int idx = acl.lastIndexOf(AbfsHttpConstants.COLON);
+      int idx = ace.lastIndexOf(AbfsHttpConstants.COLON);
-      aclEntries.put(acl.substring(0, idx), acl.substring(idx + 1));
+      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))
+        || entry.equals(AbfsHttpConstants.ACCESS_OTHER))) {
-        && !isNamedAce(entry)) {
       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);
   }

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;
@@ -270,6 +273,67 @@ public class ITestAzureBlobFilesystemAcl extends AbstractAbfsIntegrationTest {
     fs.modifyAclEntries(path, aclSpec);
   }
 
+  @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();
@@ -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();
@@ -813,6 +921,42 @@ public class ITestAzureBlobFilesystemAcl extends AbstractAbfsIntegrationTest {
     fs.setAcl(path, aclSpec);
   }
 
+  @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();
@@ -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());