From 6261c372c653e4c1792e0afab35a3f41974173f2 Mon Sep 17 00:00:00 2001 From: evasques Date: Mon, 15 Mar 2021 11:08:18 +0000 Subject: [PATCH] [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) --- .../permissions/ADMAccessControlListDAO.java | 4 +- .../permissions/FixedAclUpdaterTest.java | 84 ++++++++++++++++++- 2 files changed, 83 insertions(+), 5 deletions(-) diff --git a/repository/src/main/java/org/alfresco/repo/domain/permissions/ADMAccessControlListDAO.java b/repository/src/main/java/org/alfresco/repo/domain/permissions/ADMAccessControlListDAO.java index 774cccc9bd..8d7b54d07d 100644 --- a/repository/src/main/java/org/alfresco/repo/domain/permissions/ADMAccessControlListDAO.java +++ b/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 - if(acl.equals(sharedAclToReplace)) + // Still has old shared ACL or already replaced + if(acl.equals(sharedAclToReplace) || acl.equals(mergeFrom)) { propagateOnChildren = setFixAclPending(child.getId(), inheritFrom, mergeFrom, sharedAclToReplace, changes, false, asyncCall, propagateOnChildren); } diff --git a/repository/src/test/java/org/alfresco/repo/domain/permissions/FixedAclUpdaterTest.java b/repository/src/test/java/org/alfresco/repo/domain/permissions/FixedAclUpdaterTest.java index 16b787afb2..70566e8d0f 100644 --- a/repository/src/test/java/org/alfresco/repo/domain/permissions/FixedAclUpdaterTest.java +++ b/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 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) () -> { + 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) () -> { @@ -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 - // may create more to fix up) + private void triggerFixedACLJob(NodeRef folder) + { + // 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) () -> { 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) () -> { 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 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) () -> { + Set aspect = new HashSet<>(); + aspect.add(ContentModel.ASPECT_TEMPORARY); + nodeDAO.addNodeAspects(nodeDAO.getNodePair(folder).getFirst(), aspect); + fileFolderService.delete(folder); return null; }, false, true); }