MNT-22040 Copying nodes that contain sys:pendingFixAcl aspect (#188)

* MNT-22040 - Copying nodes that contain sys:pendingFixAcl aspect
* Increased coverage of unit tests
* Added public method removePendingAclAspect
* Changed the job to use the removePendingAclAspect method instead of
repeating the same set of operations twice
* Changed method setFixedAcls to use sharedAclToReplace property of node
if it has the pendingFixAcl aspect applied instead of using the current
shared ACL to be replaced

* Added more tests and validations

* Fixed cuncurrency issues when updating parent and child at the same time, permissions triggered by a move on a node with a pending acl and permissions needeing to be reapplied on nodes that already have the pending acl aspect

* Increase the tree strcuture as sometimes we don't reach the timeout in tests

* code cleanup and formatting

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

@@ -23,7 +23,7 @@
  * along with Alfresco. If not, see <http://www.gnu.org/licenses/>.
  * #L%
  */
-package org.alfresco.repo.domain.permissions;
+package org.alfresco.repo.domain.permissions;
 
 import java.io.Serializable;
 import java.util.ArrayList;
@@ -392,8 +392,20 @@ public class ADMAccessControlListDAO implements AccessControlListDAO
         {
             return;
         }
-        else
-        {
+        else
+        {
+            // When node is copied when the aspect is applied, the sharedACLtoReplace will not match the children's ACLS
+            // to replace, we need to use the current one.
+            Long currentAcl = nodeDAO.getNodeAclId(nodeId);
+
+            if (nodeDAO.hasNodeAspect(nodeId, ContentModel.ASPECT_PENDING_FIX_ACL))
+            {
+                // If node has a pending acl, retrieve the sharedAclToReplace from node property. When the job calls
+                // this, it already does it but on move and copy operations, it uses the new parents old ACL.
+                sharedAclToReplace = (Long) nodeDAO.getNodeProperty(nodeId, ContentModel.PROP_SHARED_ACL_TO_REPLACE);
+                
+            }
+
             // Lazily retrieve/create the shared ACL
             if (mergeFrom == null)
             {
@@ -405,33 +417,26 @@ public class ADMAccessControlListDAO implements AccessControlListDAO
                 nodeDAO.setNodeAclId(nodeId, mergeFrom);
             }
             
-            List<NodeIdAndAclId> children = nodeDAO.getPrimaryChildrenAcls(nodeId);
-            
-            if(children.size() > 0)
-            {
-                nodeDAO.setPrimaryChildrenSharedAclId(nodeId, sharedAclToReplace, mergeFrom);
-            }
+            List<NodeIdAndAclId> children = nodeDAO.getPrimaryChildrenAcls(nodeId);
             
             if (!propagateOnChildren)
             {
                 return;
-            }
+            }
+            
             for (NodeIdAndAclId child : children)
-            {
-                Long acl = child.getAclId();
-                
+            {
+                //Use the current ACL instead of the stored value, it could've been changed meanwhile
+                Long acl = nodeDAO.getNodeAclId(child.getId());
+               
                 if (acl == null)
                 {
                     propagateOnChildren = setFixAclPending(child.getId(), inheritFrom, mergeFrom, sharedAclToReplace, changes, false, asyncCall, propagateOnChildren);
                 }
                 else
                 {
-//                    if(acl.equals(mergeFrom))
-//                    {
-//                        setFixedAcls(child.getId(), inheritFrom, mergeFrom, sharedAclToReplace, changes, false);
-//                    }
                     // Still has old shared ACL or already replaced
-                    if(acl.equals(sharedAclToReplace) || acl.equals(mergeFrom))
+                    if(acl.equals(sharedAclToReplace) || acl.equals(mergeFrom) || acl.equals(currentAcl))
                     {
                         propagateOnChildren = setFixAclPending(child.getId(), inheritFrom, mergeFrom, sharedAclToReplace, changes, false, asyncCall, propagateOnChildren);
                     }
@@ -456,7 +461,22 @@ public class ADMAccessControlListDAO implements AccessControlListDAO
                         }
                     }
                 }
-            }
+            }
+
+            // By doing an eager update of the direct children we canot see if another thread has changed the ACL
+            // between the time we get the child nodes and we update them. By updating the direct children last it is
+            // possible to verify if any child has changed meanwhile.
+            if(children.size() > 0)
+            {
+                nodeDAO.setPrimaryChildrenSharedAclId(nodeId, sharedAclToReplace, mergeFrom);
+            }
+            
+            // When this is not executed triggered by the job, but a move or copy operation occures on a pending
+            // node, we don't want to apply the OLD ACL that was pending
+            if(nodeDAO.hasNodeAspect(nodeId, ContentModel.ASPECT_PENDING_FIX_ACL))
+            {
+                removePendingAclAspect(nodeId);
+            }
         }
     }
     
@@ -509,25 +529,45 @@ public class ADMAccessControlListDAO implements AccessControlListDAO
         }
         // set ASPECT_PENDING_FIX_ACL aspect on node to be later on processed with FixedAclUpdater amd switch flag
         // FIXED_ACL_ASYNC_REQUIRED_KEY
-        addFixedAclPendingAspect(nodeId, sharedAclToReplace, inheritFrom);
+        addFixedAclPendingAspect(nodeId, sharedAclToReplace, inheritFrom, mergeFrom);
         AlfrescoTransactionSupport.bindResource(FixedAclUpdater.FIXED_ACL_ASYNC_REQUIRED_KEY, true);
         // stop propagating on children nodes
         return false;
     }
 
-    private void addFixedAclPendingAspect(Long nodeId, Long sharedAclToReplace, Long inheritFrom)
-    {
-        Set<QName> aspect = new HashSet<>();
-        aspect.add(ContentModel.ASPECT_PENDING_FIX_ACL);
-        nodeDAO.addNodeAspects(nodeId, aspect);
-        Map<QName, Serializable> pendingAclProperties = new HashMap<>();
-        pendingAclProperties.put(ContentModel.PROP_SHARED_ACL_TO_REPLACE, sharedAclToReplace);
-        pendingAclProperties.put(ContentModel.PROP_INHERIT_FROM_ACL, inheritFrom);
-        nodeDAO.addNodeProperties(nodeId, pendingAclProperties);
-        if (log.isDebugEnabled())
-        {
-            log.debug("Set Fixed Acl Pending : " + nodeId + " " + nodeDAO.getNodePair(nodeId).getSecond());
+    private void addFixedAclPendingAspect(Long nodeId, Long sharedAclToReplace, Long inheritFrom, Long mergeFrom)
+    {
+        //If the node already has the pending ACL aspect, just update the new inheritFrom value
+        if (nodeDAO.hasNodeAspect(nodeId, ContentModel.ASPECT_PENDING_FIX_ACL))
+        {
+            Map<QName, Serializable> pendingAclProperties = new HashMap<>();
+            pendingAclProperties.put(ContentModel.PROP_INHERIT_FROM_ACL, inheritFrom);
+            nodeDAO.addNodeProperties(nodeId, pendingAclProperties);
+            return;
+        }
+        
+        Set<QName> aspect = new HashSet<>();
+        aspect.add(ContentModel.ASPECT_PENDING_FIX_ACL);
+        nodeDAO.addNodeAspects(nodeId, aspect);
+        Map<QName, Serializable> pendingAclProperties = new HashMap<>();
+        pendingAclProperties.put(ContentModel.PROP_SHARED_ACL_TO_REPLACE, sharedAclToReplace);
+        pendingAclProperties.put(ContentModel.PROP_INHERIT_FROM_ACL, inheritFrom);
+        nodeDAO.addNodeProperties(nodeId, pendingAclProperties);
+        if (log.isDebugEnabled())
+        {
+            log.debug("Set Fixed Acl Pending : " + nodeId + " " + nodeDAO.getNodePair(nodeId).getSecond());
         }
+    }
+
+    public void removePendingAclAspect(Long nodeId)
+    {
+        Set<QName> aspects = new HashSet<>(1);
+        aspects.add(ContentModel.ASPECT_PENDING_FIX_ACL);
+        Set<QName> pendingFixAclProperties = new HashSet<>();
+        pendingFixAclProperties.add(ContentModel.PROP_SHARED_ACL_TO_REPLACE);
+        pendingFixAclProperties.add(ContentModel.PROP_INHERIT_FROM_ACL);
+        nodeDAO.removeNodeAspects(nodeId, aspects);
+        nodeDAO.removeNodeProperties(nodeId, pendingFixAclProperties);
     }
     
     /**

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

@@ -105,5 +105,7 @@ public interface AccessControlListDAO
     
     public void updateInheritance(Long childNodeId, Long oldParentAclId, Long newParentAclId);
     
-    public void setFixedAcls(Long nodeId, Long inheritFrom, Long mergeFrom, Long sharedAclToReplace, List<AclChange> changes, boolean set);
+    public void setFixedAcls(Long nodeId, Long inheritFrom, Long mergeFrom, Long sharedAclToReplace, List<AclChange> changes, boolean set);
+
+    public void removePendingAclAspect(Long nodeId);
 }

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

@@ -53,7 +53,6 @@ import org.alfresco.repo.transaction.AlfrescoTransactionSupport;
 import org.alfresco.repo.transaction.RetryingTransactionHelper.RetryingTransactionCallback;
 import org.alfresco.repo.transaction.TransactionListenerAdapter;
 import org.alfresco.service.cmr.repository.NodeRef;
-import org.alfresco.service.cmr.repository.NodeRef.Status;
 import org.alfresco.service.cmr.repository.StoreRef;
 import org.alfresco.service.namespace.NamespaceService;
 import org.alfresco.service.namespace.QName;
@@ -93,8 +92,8 @@ public class FixedAclUpdater extends TransactionListenerAdapter implements Appli
     private int maxItemBatchSize = 100;
     private int numThreads = 4;
 
-    private ClassPolicyDelegate<OnInheritPermissionsDisabled> onInheritPermissionsDisabledDelegate;    
-    private PolicyComponent policyComponent;    
+    private ClassPolicyDelegate<OnInheritPermissionsDisabled> onInheritPermissionsDisabledDelegate;
+    private PolicyComponent policyComponent;
     private PolicyIgnoreUtil policyIgnoreUtil;
 
     public void setNumThreads(int numThreads)
@@ -137,8 +136,8 @@ public class FixedAclUpdater extends TransactionListenerAdapter implements Appli
     {
         this.lockTimeToLive = lockTimeToLive;
         this.lockRefreshTime = lockTimeToLive / 2;
-    }    
-    
+    }
+
     public void setPolicyComponent(PolicyComponent policyComponent)
     {
         this.policyComponent = policyComponent;
@@ -151,7 +150,8 @@ public class FixedAclUpdater extends TransactionListenerAdapter implements Appli
 
     public void init()
     {
-        onInheritPermissionsDisabledDelegate = policyComponent.registerClassPolicy(PermissionServicePolicies.OnInheritPermissionsDisabled.class);
+        onInheritPermissionsDisabledDelegate = policyComponent
+                .registerClassPolicy(PermissionServicePolicies.OnInheritPermissionsDisabled.class);
     }
 
     private class GetNodesWithAspects
@@ -264,35 +264,34 @@ public class FixedAclUpdater extends TransactionListenerAdapter implements Appli
                     {
                         log.debug(String.format("Processing node %s", nodeRef));
                     }
+
                     final Long nodeId = nodeDAO.getNodePair(nodeRef).getFirst();
 
                     // MNT-22009 - If node was deleted and in archive store, remove the aspect and properties and do not
                     // process
                     if (nodeRef.getStoreRef().equals(StoreRef.STORE_REF_ARCHIVE_SPACESSTORE))
                     {
-                        nodeDAO.removeNodeAspects(nodeId, aspects);
-                        nodeDAO.removeNodeProperties(nodeId, PENDING_FIX_ACL_ASPECT_PROPS);
+                        accessControlListDAO.removePendingAclAspect(nodeId);
                         return null;
                     }
 
                     // retrieve acl properties from node
-                    Long inheritFrom = (Long) nodeDAO.getNodeProperty(nodeId,
-                            ContentModel.PROP_INHERIT_FROM_ACL);
-                    Long sharedAclToReplace = (Long) nodeDAO.getNodeProperty(nodeId,
-                            ContentModel.PROP_SHARED_ACL_TO_REPLACE);
+                    Long inheritFrom = (Long) nodeDAO.getNodeProperty(nodeId, ContentModel.PROP_INHERIT_FROM_ACL);
+                    Long sharedAclToReplace = (Long) nodeDAO.getNodeProperty(nodeId, ContentModel.PROP_SHARED_ACL_TO_REPLACE);
 
                     // set inheritance using retrieved prop
-                    accessControlListDAO.setInheritanceForChildren(nodeRef, inheritFrom, sharedAclToReplace,
-                            true);
+                    accessControlListDAO.setInheritanceForChildren(nodeRef, inheritFrom, sharedAclToReplace, true);
+
+                    // Remove aspect
+                    accessControlListDAO.removePendingAclAspect(nodeId);
 
-                    nodeDAO.removeNodeAspects(nodeId, aspects);
-                    nodeDAO.removeNodeProperties(nodeId, PENDING_FIX_ACL_ASPECT_PROPS);
-                    
                     if (!policyIgnoreUtil.ignorePolicy(nodeRef))
                     {
-                        boolean transformedToAsyncOperation = toBoolean((Boolean) AlfrescoTransactionSupport.getResource(FixedAclUpdater.FIXED_ACL_ASYNC_REQUIRED_KEY));
+                        boolean transformedToAsyncOperation = toBoolean(
+                                (Boolean) AlfrescoTransactionSupport.getResource(FixedAclUpdater.FIXED_ACL_ASYNC_REQUIRED_KEY));
 
-                        OnInheritPermissionsDisabled onInheritPermissionsDisabledPolicy = onInheritPermissionsDisabledDelegate.get(ContentModel.TYPE_BASE);
+                        OnInheritPermissionsDisabled onInheritPermissionsDisabledPolicy = onInheritPermissionsDisabledDelegate
+                                .get(ContentModel.TYPE_BASE);
                         onInheritPermissionsDisabledPolicy.onInheritPermissionsDisabled(nodeRef, transformedToAsyncOperation);
                     }
 
@@ -406,12 +405,8 @@ public class FixedAclUpdater extends TransactionListenerAdapter implements Appli
 
             AclWorkProvider provider = new AclWorkProvider();
             AclWorker worker = new AclWorker();
-            BatchProcessor<NodeRef> bp = new BatchProcessor<>(
-                    "FixedAclUpdater",
-                    transactionService.getRetryingTransactionHelper(),
-                    provider,
-                    numThreads, maxItemBatchSize,
-                    applicationContext,
+            BatchProcessor<NodeRef> bp = new BatchProcessor<>("FixedAclUpdater",
+                    transactionService.getRetryingTransactionHelper(), provider, numThreads, maxItemBatchSize, applicationContext,
                     log, 100);
             int count = bp.process(worker, true);
             return count;
@@ -424,7 +419,7 @@ public class FixedAclUpdater extends TransactionListenerAdapter implements Appli
         finally
         {
             jobLockRefreshCallback.isActive.set(false);
-            if(lockToken != null)
+            if (lockToken != null)
             {
                 jobLockService.releaseLock(lockToken, lockQName);
             }