[MNT-21847] - Aync permissions fail when new nodes are created (#1188) (#345)

Fix:
*Changed method setFixedAcls on class ADMAccessControlListDAO to continue to propagate through children to apply the correct acl not only when the current child acl matches the shared acl to replace but also when the current child acl matches the new shared acl
Unit Test:
*Refactored the unit test FixedAclUpdaterTest to be able to add in a new test without repeating code: separating the operations that set the permissions from the one that triggers the job into separate methods
*As it was if one test failed, leaving aspects to be processed, the test would run indefinitely (it was programmed to keep running the job while there where nodes with the aspect). Added a verification to stop triggering the job if the number of nodes with the pendingFixAcl did not change between executions.
*Also, if one test failed, it would leave nodes with pendingFixAcl aspect in the database, and the other tests that ran after would also fail, not completing the goal of processing all nodes with the aspect. If a test fails, the folder structure it ran is now deleted so no nodes with the aspect from that structure are processed by the other tests.
*Added a test to find the first folder in a tree where permissions where set async that has the pendingFixAcl aspect and that creates a new node in it to verify the issue

(cherry picked from commit 443e5e226430a2760492fb82214ad520e7e1cb75)

repository/src/main/java/org/alfresco/repo/domain/permissions/ADMAccessControlListDAO.java

@@ -430,8 +430,8 @@ public class ADMAccessControlListDAO implements AccessControlListDAO
 //                    {
 //                        setFixedAcls(child.getId(), inheritFrom, mergeFrom, sharedAclToReplace, changes, false);
 //                    }
-                    // Already replaced
+                    // Still has old shared ACL or already replaced
-                    if(acl.equals(sharedAclToReplace))
+                    if(acl.equals(sharedAclToReplace) || acl.equals(mergeFrom))
                     {
                         propagateOnChildren = setFixAclPending(child.getId(), inheritFrom, mergeFrom, sharedAclToReplace, changes, false, asyncCall, propagateOnChildren);
                     }

repository/src/test/java/org/alfresco/repo/domain/permissions/FixedAclUpdaterTest.java

@@ -25,7 +25,10 @@
  */
 package org.alfresco.repo.domain.permissions;
 
+import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Set;
 
 import org.alfresco.model.ContentModel;
@@ -41,6 +44,7 @@ import org.alfresco.repo.transaction.RetryingTransactionHelper.RetryingTransacti
 import org.alfresco.repo.transaction.TransactionListenerAdapter;
 import org.alfresco.service.ServiceRegistry;
 import org.alfresco.service.cmr.model.FileFolderService;
+import org.alfresco.service.cmr.model.FileInfo;
 import org.alfresco.service.cmr.repository.NodeRef;
 import org.alfresco.service.cmr.security.PermissionService;
 import org.alfresco.service.namespace.QName;
@@ -69,6 +73,7 @@ public class FixedAclUpdaterTest extends TestCase
     private Repository repository;
     private FixedAclUpdater fixedAclUpdater;
     private NodeRef folderAsyncCallNodeRef;
+    private NodeRef folderAsyncCallWithCreateNodeRef;
     private NodeRef folderSyncCallNodeRef;
     private PermissionsDaoComponent permissionsDaoComponent;
     private PermissionService permissionService;
@@ -100,6 +105,10 @@ public class FixedAclUpdaterTest extends TestCase
                 filesPerLevel);
         folderSyncCallNodeRef = txnHelper.doInTransaction(cb2);
 
+        RetryingTransactionCallback<NodeRef> cb3 = createFolderHierchyCallback(home, fileFolderService,
+                "rootFolderAsyncWithCreateCall", filesPerLevel);
+        folderAsyncCallWithCreateNodeRef = txnHelper.doInTransaction(cb3);
+
         // change setFixedAclMaxTransactionTime to lower value so setInheritParentPermissions on created folder
         // hierarchy require async call
         setFixedAclMaxTransactionTime(permissionsDaoComponent, home, 50);
@@ -147,8 +156,10 @@ public class FixedAclUpdaterTest extends TestCase
                 aspect.add(ContentModel.ASPECT_TEMPORARY);
                 nodeDAO.addNodeAspects(nodeDAO.getNodePair(folderAsyncCallNodeRef).getFirst(), aspect);
                 nodeDAO.addNodeAspects(nodeDAO.getNodePair(folderSyncCallNodeRef).getFirst(), aspect);
+                nodeDAO.addNodeAspects(nodeDAO.getNodePair(folderAsyncCallWithCreateNodeRef).getFirst(), aspect);
                 fileFolderService.delete(folderAsyncCallNodeRef);
                 fileFolderService.delete(folderSyncCallNodeRef);
+                fileFolderService.delete(folderAsyncCallWithCreateNodeRef);
                 return null;
             }, false, true);
         }
@@ -189,7 +200,34 @@ public class FixedAclUpdaterTest extends TestCase
         testWork(folderAsyncCallNodeRef, true);
     }
 
+    @Test
+    public void testAsyncWithNodeCreation()
+    {
+        testWorkWithNodeCreation(folderAsyncCallWithCreateNodeRef, true);
+    }
+
     private void testWork(NodeRef folderRef, boolean asyncCall)
+    {
+        setPermissionsOnTree(folderRef, asyncCall);
+        triggerFixedACLJob(folderRef);
+    }
+
+    private void testWorkWithNodeCreation(NodeRef folderRef, boolean asyncCall)
+    {
+        setPermissionsOnTree(folderRef, asyncCall);
+
+        // MNT-21847 - Create a new content in folder that has the aspect applied
+        txnHelper.doInTransaction((RetryingTransactionCallback<Void>) () -> {
+            NodeRef folderWithPendingAcl = getFirstFolderWithAclPending(folderRef);
+            assertNotNull("No children folders were found with pendingFixACl aspect", folderWithPendingAcl);
+            createFile(fileFolderService, folderWithPendingAcl, "NewFile", ContentModel.TYPE_CONTENT);
+            return null;
+        }, false, true);
+
+        triggerFixedACLJob(folderRef);
+    }
+
+    private void setPermissionsOnTree(NodeRef folderRef, boolean asyncCall)
     {
         // kick it off by setting inherit parent permissions == false
         txnHelper.doInTransaction((RetryingTransactionCallback<Void>) () -> {
@@ -203,21 +241,61 @@ public class FixedAclUpdaterTest extends TestCase
             assertTrue("There are no nodes to process", getNodesCountWithPendingFixedAclAspect() > 0);
             return null;
         }, false, true);
+    }
 
-        // run the fixedAclUpdater until there is nothing more to fix (running the updater
+    private void triggerFixedACLJob(NodeRef folder)
-        // may create more to fix up)
+    {
+        // run the fixedAclUpdater until there is nothing more to fix (running the updater may create more to fix up) or
+        // the count doesn't change, meaning we have a problem.
         txnHelper.doInTransaction((RetryingTransactionCallback<Void>) () -> {
             int count = 0;
+            int previousCount = 0;
             do
             {
+                previousCount = count;
                 count = fixedAclUpdater.execute();
-            } while (count > 0);
+            } while (count > 0 && previousCount != count);
             return null;
         }, false, true);
 
         // check if nodes with ASPECT_PENDING_FIX_ACL are processed
         txnHelper.doInTransaction((RetryingTransactionCallback<Void>) () -> {
             assertEquals("Not all nodes were processed", 0, getNodesCountWithPendingFixedAclAspect());
+            //Remove the tree that failed so it does not influence the other test results
+            removeNodesWithPendingAcl(folder);
+            return null;
+        }, false, true);
+    }
+
+    private NodeRef getFirstFolderWithAclPending(NodeRef parentNodeRef)
+    {
+        NodeRef folderWithPendingFixedAcl = null;
+        List<FileInfo> primaryChildFolders = fileFolderService.listFolders(parentNodeRef);
+        for (int i = 0; i < primaryChildFolders.size(); i++)
+        {
+            NodeRef thisChildFolder = primaryChildFolders.get(i).getNodeRef();
+            Long thisChildNodeId = nodeDAO.getNodePair(thisChildFolder).getFirst();
+            if (nodeDAO.hasNodeAspect(thisChildNodeId, ContentModel.ASPECT_PENDING_FIX_ACL))
+            {
+                folderWithPendingFixedAcl = thisChildFolder;
+                break;
+            }
+
+            if (folderWithPendingFixedAcl == null)
+            {
+                folderWithPendingFixedAcl = getFirstFolderWithAclPending(thisChildFolder);
+            }
+        }
+        return folderWithPendingFixedAcl;
+    }
+
+    private void removeNodesWithPendingAcl(NodeRef folder)
+    {
+        txnHelper.doInTransaction((RetryingTransactionCallback<Void>) () -> {
+            Set<QName> aspect = new HashSet<>();
+            aspect.add(ContentModel.ASPECT_TEMPORARY);
+            nodeDAO.addNodeAspects(nodeDAO.getNodePair(folder).getFirst(), aspect);
+            fileFolderService.delete(folder);
             return null;
         }, false, true);
     }