Do not allow addition of locked content to holds

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

@@ -1,4 +1,5 @@
 rm.hold.not-hold=The node {0} is not a hold.
 rm.hold.add-to-hold-invalid-type={0} is neither a record nor a record folder nor active content. Only records, record \
   folders or active content can be added to a hold.
-rm.hold.add-to-hold-archived-node=Archived nodes can't be added to hold.
+rm.hold.add-to-hold-archived-node=Archived nodes can't be added to hold.
+rm.hold.add-to-hold-locked-node=Locked nodes can't be added to hold.

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

@@ -27,6 +27,8 @@
 
 package org.alfresco.module.org_alfresco_module_rm.hold;
 
+import static org.alfresco.model.ContentModel.ASPECT_LOCKABLE;
+
 import java.io.Serializable;
 import java.util.ArrayList;
 import java.util.Collections;
@@ -59,7 +61,6 @@ import org.alfresco.repo.security.authentication.AuthenticationUtil.RunAsWork;
 import org.alfresco.repo.security.permissions.AccessDeniedException;
 import org.alfresco.service.cmr.repository.ChildAssociationRef;
 import org.alfresco.service.cmr.repository.NodeRef;
-import org.alfresco.service.cmr.repository.NodeService;
 import org.alfresco.service.cmr.security.AccessStatus;
 import org.alfresco.service.cmr.security.PermissionService;
 import org.alfresco.service.namespace.NamespaceService;
@@ -122,16 +123,6 @@ public class HoldServiceImpl extends ServiceBaseImpl
         this.filePlanService = filePlanService;
     }
 
-    /**
-     * Set the node service
-     *
-     * @param nodeService the node service
-     */
-    public void setNodeService(NodeService nodeService)
-    {
-        this.nodeService = nodeService;
-    }
-
     /**
      * Set the record service
      *
@@ -575,8 +566,7 @@ public class HoldServiceImpl extends ServiceBaseImpl
                 throw new IntegrityException(I18NUtil.getMessage("rm.hold.not-hold", holdName), null);
             }
 
-            if (permissionService.hasPermission(hold, RMPermissionModel.FILING) == AccessStatus.DENIED ||
+            if (!AccessStatus.ALLOWED.equals(
-                    !AccessStatus.ALLOWED.equals(
                             capabilityService.getCapabilityAccessState(hold, RMPermissionModel.ADD_TO_HOLD)))
             {
                 throw new AccessDeniedException(I18NUtil.getMessage(MSG_ERR_ACCESS_DENIED));
@@ -638,6 +628,11 @@ public class HoldServiceImpl extends ServiceBaseImpl
         {
             throw new IntegrityException(I18NUtil.getMessage("rm.hold.add-to-hold-archived-node"), null);
         }
+
+        if (nodeService.hasAspect(nodeRef, ASPECT_LOCKABLE))
+        {
+            throw new IntegrityException(I18NUtil.getMessage("rm.hold.add-to-hold-locked-node"), null);
+        }
     }
 
     /**

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

@@ -131,9 +131,9 @@ public abstract class BaseHold extends DeclarativeWebScript
     @Override
     protected Map<String, Object> executeImpl(WebScriptRequest req, Status status, Cache cache)
     {
-        JSONObject json = getJSONFromContent(req);
+        final JSONObject json = getJSONFromContent(req);
-        List<NodeRef> holds = getHolds(json);
+        final List<NodeRef> holds = getHolds(json);
-        List<NodeRef> nodeRefs = getItemNodeRefs(json);
+        final List<NodeRef> nodeRefs = getItemNodeRefs(json);
         doAction(holds, nodeRefs);
         return new HashMap<>();
     }
@@ -159,7 +159,7 @@ public abstract class BaseHold extends DeclarativeWebScript
         JSONObject json = null;
         try
         {
-            String content = req.getContent().getContent();
+            final String content = req.getContent().getContent();
             json = new JSONObject(new JSONTokener(content));
         }
         catch (IOException iox)
@@ -185,10 +185,10 @@ public abstract class BaseHold extends DeclarativeWebScript
      */
     protected List<NodeRef> getItemNodeRefs(JSONObject json)
     {
-        List<NodeRef> nodeRefs = new ArrayList<>();
+        final List<NodeRef> nodeRefs = new ArrayList<>();
         try
         {
-            JSONArray nodeRefsArray = json.getJSONArray("nodeRefs");
+            final JSONArray nodeRefsArray = json.getJSONArray("nodeRefs");
             for (int i = 0; i < nodeRefsArray.length(); i++)
             {
                 NodeRef nodeReference = new NodeRef(nodeRefsArray.getString(i));
@@ -234,13 +234,13 @@ public abstract class BaseHold extends DeclarativeWebScript
      */
     protected List<NodeRef> getHolds(JSONObject json)
     {
-        List<NodeRef> holds = new ArrayList<>();
+        final List<NodeRef> holds = new ArrayList<>();
         try
         {
-            JSONArray holdsArray = json.getJSONArray("holds");
+            final JSONArray holdsArray = json.getJSONArray("holds");
             for (int i = 0; i < holdsArray.length(); i++)
             {
-                NodeRef nodeRef = new NodeRef(holdsArray.getString(i));
+                final NodeRef nodeRef = new NodeRef(holdsArray.getString(i));
                 checkHoldNodeRef(nodeRef);
                 holds.add(nodeRef);
             }

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

@@ -116,33 +116,33 @@ public class HoldsGet extends DeclarativeWebScript
     @Override
     protected Map<String, Object> executeImpl(WebScriptRequest req, Status status, Cache cache)
     {
-        boolean fileOnly = getFileOnly(req);
+        final boolean fileOnly = getFileOnly(req);
-        NodeRef itemNodeRef = getItemNodeRef(req);
+        final NodeRef itemNodeRef = getItemNodeRef(req);
-        List<NodeRef> holds = new ArrayList<>();
+        final List<NodeRef> holds = new ArrayList<>();
 
         if (itemNodeRef == null)
         {
-            NodeRef filePlan = getFilePlan(req);
+            final NodeRef filePlan = getFilePlan(req);
             holds.addAll(holdService.getHolds(filePlan));
         }
         else
         {
-            boolean includedInHold = getIncludedInHold(req);
+            final boolean includedInHold = getIncludedInHold(req);
             holds.addAll(holdService.heldBy(itemNodeRef, includedInHold));
         }
 
-        List<Hold> holdObjects = new ArrayList<>(holds.size());
+        final List<Hold> holdObjects = new ArrayList<>(holds.size());
         for (NodeRef nodeRef : holds)
         {
             // only add if user has filling permission on the hold
             if (!fileOnly || permissionService.hasPermission(nodeRef, RMPermissionModel.FILING) == AccessStatus.ALLOWED)
             {
-                String name = (String) nodeService.getProperty(nodeRef, ContentModel.PROP_NAME);
+                final String name = (String) nodeService.getProperty(nodeRef, ContentModel.PROP_NAME);
                 holdObjects.add(new Hold(name, nodeRef));
             }
         }
 
-        Map<String, Object> model = new HashMap<>(1);
+        final Map<String, Object> model = new HashMap<>(1);
         sortHoldByName(holdObjects);
         model.put("holds", holdObjects);
 
@@ -159,10 +159,10 @@ public class HoldsGet extends DeclarativeWebScript
     {
         NodeRef filePlan = null;
 
-        Map<String, String> templateVars = req.getServiceMatch().getTemplateVars();
+        final Map<String, String> templateVars = req.getServiceMatch().getTemplateVars();
-        String storeType = templateVars.get("store_type");
+        final String storeType = templateVars.get("store_type");
-        String storeId = templateVars.get("store_id");
+        final String storeId = templateVars.get("store_id");
-        String id = templateVars.get("id");
+        final String id = templateVars.get("id");
 
         if (StringUtils.isNotBlank(storeType) && StringUtils.isNotBlank(storeId) && StringUtils.isNotBlank(id))
         {
@@ -194,7 +194,7 @@ public class HoldsGet extends DeclarativeWebScript
      */
     private NodeRef getItemNodeRef(WebScriptRequest req)
     {
-        String nodeRef = req.getParameter("itemNodeRef");
+        final String nodeRef = req.getParameter("itemNodeRef");
         NodeRef itemNodeRef = null;
         if (StringUtils.isNotBlank(nodeRef))
         {
@@ -212,7 +212,7 @@ public class HoldsGet extends DeclarativeWebScript
     private boolean getIncludedInHold(WebScriptRequest req)
     {
         boolean result = true;
-        String includedInHold = req.getParameter("includedInHold");
+        final String includedInHold = req.getParameter("includedInHold");
         if (StringUtils.isNotBlank(includedInHold))
         {
             result = Boolean.parseBoolean(includedInHold);
@@ -223,7 +223,7 @@ public class HoldsGet extends DeclarativeWebScript
     private boolean getFileOnly(WebScriptRequest req)
     {
         boolean result = false;
-        String fillingOnly = req.getParameter("fileOnly");
+        final String fillingOnly = req.getParameter("fileOnly");
         if (StringUtils.isNotBlank(fillingOnly))
         {
             result = Boolean.parseBoolean(fillingOnly);

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

@@ -93,8 +93,8 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
     protected NodeRef hold2;
     protected NodeRef activeContent;
 
-    @Mock (name="capabilityService")
+    @Mock
-    protected CapabilityService mockedCapabilityService;
+    private CapabilityService mockedCapabilityService;
     @Spy @InjectMocks HoldServiceImpl holdService;
 
     @Before
@@ -389,10 +389,14 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
     }
 
     @Test (expected = AccessDeniedException.class)
-    public void addActiveContentToHoldNoPermissionsOnHold()
+    public void addActiveContentToHoldsNoPermissionsOnHold()
     {
-        when(mockedPermissionService.hasPermission(hold, RMPermissionModel.FILING)).thenReturn(AccessStatus.DENIED);
+        when(mockedCapabilityService.getCapabilityAccessState(hold, RMPermissionModel.ADD_TO_HOLD)).thenReturn(AccessStatus.DENIED);
-        holdService.addToHold(hold, activeContent);
+        // build a list of holds
+        List<NodeRef> holds = new ArrayList<>(2);
+        holds.add(hold);
+        holds.add(hold2);
+        holdService.addToHolds(holds, activeContent);
     }
 
     @Test (expected = AccessDeniedException.class)
@@ -409,6 +413,13 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
         holdService.addToHold(hold, activeContent);
     }
 
+    @Test (expected = IntegrityException.class)
+    public void addLockedContentToHold()
+    {
+        when(mockedNodeService.hasAspect(activeContent, ContentModel.ASPECT_LOCKABLE)).thenReturn(true);
+        holdService.addToHold(hold, activeContent);
+    }
+
     @SuppressWarnings("unchecked")
     @Test
     public void addToHolds()