Merge branch 'feature/RM-6930_DeleteHold' into 'master'

RM-6930 adding permissions check for active content in holds defore deletion

Closes RM-6930

See merge request records-management/records-management!1286

rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/messages/hold-service.properties

@@ -5,4 +5,6 @@ rm.hold.add-to-hold-locked-node=Locked content can't be added to a hold.
 rm.hold.delete-frozen-node=Frozen content can't be deleted.
 rm.hold.delete-node-frozen-children=Can't delete folder because it contains frozen content.
 rm.hold.move-frozen-node=Frozen content can't be moved.
-rm.hold.update-frozen-node=Frozen content can't be updated.
+rm.hold.update-frozen-node=Frozen content can't be updated.
+rm.hold.generic-permission-error=Can't delete hold, because you don't have the correct permissions for all the items within the hold.
+rm.hold.detailed-permission-error=Can't delete hold, because filing permissions for the following items are needed: 

rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java

@@ -101,6 +101,8 @@ public class HoldServiceImpl extends ServiceBaseImpl
 
     /** I18N */
     private static final String MSG_ERR_ACCESS_DENIED = "permissions.err_access_denied";
+    private static final String MSG_ERR_HOLD_PERMISSION_GENERIC_ERROR = "rm.hold.generic-permission-error";
+    private static final String MSG_ERR_HOLD_PERMISSION_DETAILED_ERROR = "rm.hold.detailed-permission-error";
 
     /** File Plan Service */
     private FilePlanService filePlanService;
@@ -526,7 +528,7 @@ public class HoldServiceImpl extends ServiceBaseImpl
 
         if (!isHold(hold))
         {
-            throw new AlfrescoRuntimeException("Can't delete hold, becuase passed node is not a hold. (hold=" + hold.toString() + ")");
+            throw new AlfrescoRuntimeException("Can't delete hold, because passed node is not a hold. (hold=" + hold.toString() + ")");
         }
 
         List<NodeRef> held = AuthenticationUtil.runAsSystem(new RunAsWork<List<NodeRef>>()
@@ -543,14 +545,25 @@ public class HoldServiceImpl extends ServiceBaseImpl
         {
             try
             {
-                if (permissionService.hasPermission(nodeRef, RMPermissionModel.FILING) == AccessStatus.DENIED)
+                String permission;
+
+                if (recordService.isRecord(nodeRef) || recordFolderService.isRecordFolder(nodeRef))
+                {
+                    permission = RMPermissionModel.FILING;
+                }
+                else
+                {
+                    permission =  PermissionService.READ;
+                }
+
+                if (permissionService.hasPermission(nodeRef, permission) == AccessStatus.DENIED)
                 {
                     heldNames.add((String) nodeService.getProperty(nodeRef, ContentModel.PROP_NAME));
                 }
             }
             catch (AccessDeniedException ade)
             {
-                throw new AlfrescoRuntimeException("Can't delete hold, because you don't have filling permissions on all the items held within the hold.", ade);
+                throw new AccessDeniedException(I18NUtil.getMessage(MSG_ERR_HOLD_PERMISSION_GENERIC_ERROR), ade);
             }
         }
 
@@ -564,7 +577,7 @@ public class HoldServiceImpl extends ServiceBaseImpl
                 sb.append(name);
                 sb.append("'");
             }
-            throw new AlfrescoRuntimeException("Can't delete hold, because filing permissions for the following items are needed: " + sb.toString());
+            throw new AccessDeniedException(I18NUtil.getMessage(MSG_ERR_HOLD_PERMISSION_DETAILED_ERROR) + sb.toString());
         }
 
         invokeBeforeDeleteHold(hold);

rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImplUnitTest.java

@@ -96,6 +96,7 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
 
     @Mock
     private CapabilityService mockedCapabilityService;
+
     @Spy @InjectMocks HoldServiceImpl holdService;
 
     @Before
@@ -221,9 +222,7 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
         when(mockedNodeService.createNode(eq(holdContainer), eq(ContentModel.ASSOC_CONTAINS), any(QName.class) , eq(TYPE_HOLD), any(Map.class)))
             .thenReturn(new ChildAssociationRef(ContentModel.ASSOC_CONTAINS, holdContainer, generateQName(), hold));
 
-        // mocks for policies
+        mockPoliciesForCreateHold();
-        doNothing().when(holdService).invokeBeforeCreateHold(any(), anyString(), anyString());
-        doNothing().when(holdService).invokeOnCreateHold(any());
 
         // create hold
         NodeRef newHold = holdService.createHold(filePlan, HOLD_NAME, HOLD_REASON, HOLD_DESCRIPTION);
@@ -311,9 +310,7 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
     @Test
     public void deleteHold()
     {
-        // mocks for policies
+        mockPoliciesForDeleteHold();
-        doNothing().when(holdService).invokeBeforeDeleteHold(any());
-        doNothing().when(holdService).invokeOnDeleteHold(any(), any());
 
         // delete hold
         holdService.deleteHold(hold);
@@ -579,6 +576,103 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
         holdService.removeFromHolds(holds, activeContent);
     }
 
+    /**
+     * test delete hold throws exception for failed read permission check for content
+     */
+    @Test (expected = AccessDeniedException.class)
+    public void testDeleteHoldThrowsExceptionForActiveContentWithoutReadPermission()
+    {
+        NodeRef heldContent = generateNodeRef(TYPE_CONTENT);
+        List<ChildAssociationRef> holds = createListOfHoldAssociations(heldContent);
+
+        when(mockedNodeService.getChildAssocs(hold, ASSOC_FROZEN_CONTENT, RegexQNamePattern.MATCH_ALL)).thenReturn(holds);
+        when(mockedPermissionService.hasPermission(heldContent, PermissionService.READ)).thenReturn(AccessStatus.DENIED);
+        when(mockedNodeService.getProperty(heldContent, ContentModel.PROP_NAME)).thenReturn("foo");
+
+        holdService.deleteHold(hold);
+    }
+
+    /**
+     * test delete hold throws exception for failed read permission check for records
+     */
+    @Test (expected = AccessDeniedException.class)
+    public void testDeleteHoldThrowsExceptionForARecordWithoutReadPermission()
+    {
+        NodeRef heldContent = generateNodeRef();
+        List<ChildAssociationRef> holds = createListOfHoldAssociations(heldContent);
+
+        when(mockedNodeService.getChildAssocs(hold, ASSOC_FROZEN_CONTENT, RegexQNamePattern.MATCH_ALL)).thenReturn(holds);
+        when(mockedRecordService.isRecord(heldContent)).thenThrow(new AccessDeniedException(""));
+
+        holdService.deleteHold(hold);
+    }
+
+    /**
+     * test delete hold throws exception for failed file permission check for records
+     */
+    @Test (expected = AccessDeniedException.class)
+    public void testDeleteHoldThrowsExceptionForARecordWithoutFilePermission()
+    {
+        NodeRef heldContent = generateNodeRef();
+        List<ChildAssociationRef> holds = createListOfHoldAssociations(heldContent);
+
+        when(mockedNodeService.getChildAssocs(hold, ASSOC_FROZEN_CONTENT, RegexQNamePattern.MATCH_ALL)).thenReturn(holds);
+        when(mockedRecordService.isRecord(heldContent)).thenReturn(true);
+        when(mockedPermissionService.hasPermission(heldContent, RMPermissionModel.FILING)).thenReturn(AccessStatus.DENIED);
+        when(mockedNodeService.getProperty(heldContent, ContentModel.PROP_NAME)).thenReturn("foo");
+
+        holdService.deleteHold(hold);
+    }
+
+    /**
+     * Test hold deleted for active content with read permission
+     */
+    @Test
+    public void testDeleteHoldChecksReadPermissionForActiveContent()
+    {
+        NodeRef heldContent = generateNodeRef(TYPE_CONTENT);
+        List<ChildAssociationRef> holds = createListOfHoldAssociations(heldContent);
+
+        mockPoliciesForDeleteHold();
+        when(mockedNodeService.getChildAssocs(hold, ASSOC_FROZEN_CONTENT, RegexQNamePattern.MATCH_ALL)).thenReturn(holds);
+        when(mockedRecordService.isRecord(heldContent)).thenReturn(false);
+        when(mockedRecordFolderService.isRecordFolder(heldContent)).thenReturn(false);
+        when(mockedPermissionService.hasPermission(heldContent, PermissionService.READ)).thenReturn(AccessStatus.ALLOWED);
+        when(mockedNodeService.getProperty(heldContent, ContentModel.PROP_NAME)).thenReturn("foo");
+
+        holdService.deleteHold(hold);
+
+        verify(mockedNodeService, times(1)).deleteNode(hold);
+    }
+
+    /**
+     * Helper method to create hold and associations with given content
+     */
+    private List<ChildAssociationRef> createListOfHoldAssociations(NodeRef heldContent)
+    {
+        List<ChildAssociationRef> holds = new ArrayList<>(2);
+        holds.add(new ChildAssociationRef(ASSOC_FROZEN_CONTENT, hold, ASSOC_FROZEN_CONTENT, heldContent, true, 1));
+        return holds;
+    }
+
+    /**
+     * mocks policies for create hold
+     */
+    private void mockPoliciesForCreateHold()
+    {
+        doNothing().when(holdService).invokeBeforeCreateHold(any(), anyString(), anyString());
+        doNothing().when(holdService).invokeOnCreateHold(any());
+    }
+
+    /**
+     * mocks policies for delete hold
+     */
+    private void mockPoliciesForDeleteHold()
+    {
+        doNothing().when(holdService).invokeBeforeDeleteHold(any());
+        doNothing().when(holdService).invokeOnDeleteHold(any(), any());
+    }
+
     /**
      * mocks policies for add to hold
      */