Merge branch 'master' into feature/RM-7028_Add_Add_To_Hold_ToAudit

rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-action-context.xml

@@ -1025,7 +1025,6 @@
 
    <bean id="deleteHold" class="org.alfresco.module.org_alfresco_module_rm.action.impl.DeleteHoldAction" parent="rmAction">
       <property name="auditable" value="false"/>
-      <property name="auditedImmediately" value="true"/>
    </bean>
 
    <bean id="deleteHold_proxy" parent="rmProxyAction" >

rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-model-context.xml

@@ -177,6 +177,11 @@
    </bean>
 
    <util:constant id="propThumbnailModification" static-field="org.alfresco.model.ContentModel.PROP_LAST_THUMBNAIL_MODIFICATION_DATA" />
+   
+   <!-- Defines a list of namespace URIs for properties, which should be always editable for a frozen node-->
+   <util:list id="frozen_alwaysEditURIs" value-type="java.lang.String">
+      <value>http://www.alfresco.org/model/system/1.0</value>
+   </util:list>
 
    <bean name="updateFrozenPropertyCheck"
          class="org.alfresco.module.org_alfresco_module_rm.util.PropertyModificationAllowedCheck">
@@ -185,6 +190,7 @@
             <ref bean="propThumbnailModification" />
          </list>
       </property>
+      <property name="editableURIs" ref="frozen_alwaysEditURIs" />
    </bean>
 
    <bean id="rma.vitalRecordDefinition" class="org.alfresco.module.org_alfresco_module_rm.model.rma.aspect.VitalRecordDefinitionAspect" parent="rm.baseBehaviour">

rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/audit/event/CreateHoldAuditEvent.java

@@ -30,10 +30,11 @@ package org.alfresco.module.org_alfresco_module_rm.audit.event;
 import java.io.Serializable;
 import java.util.Map;
 
+import org.alfresco.repo.node.NodeServicePolicies;
+import org.alfresco.repo.policy.Behaviour.NotificationFrequency;
 import org.alfresco.repo.policy.annotation.Behaviour;
 import org.alfresco.repo.policy.annotation.BehaviourBean;
 import org.alfresco.repo.policy.annotation.BehaviourKind;
-import org.alfresco.repo.policy.Behaviour.NotificationFrequency;
 import org.alfresco.service.cmr.repository.ChildAssociationRef;
 import org.alfresco.service.cmr.repository.NodeRef;
 import org.alfresco.service.cmr.repository.NodeService;
@@ -41,14 +42,18 @@ import org.alfresco.service.namespace.QName;
 
 /**
  * Create hold audit event.
+ * This listens to the NodeServicePolicies.OnCreateNodePolicy in order to cover the create hold action from Share
+ * since that does not call the createHold from HoldService
  *
  * @author Sara Aspery
  * @since 3.3
  */
 @BehaviourBean
-public class CreateHoldAuditEvent extends AuditEvent
+public class CreateHoldAuditEvent extends AuditEvent implements NodeServicePolicies.OnCreateNodePolicy
 {
-    /** Node Service */
+    /**
+     * Node Service
+     */
     private NodeService nodeService;
 
     /**
@@ -62,16 +67,16 @@ public class CreateHoldAuditEvent extends AuditEvent
     }
 
     /**
-     * @param childAssociationRef child association reference
+     * @see org.alfresco.repo.node.NodeServicePolicies.OnCreateNodePolicy#onCreateNode(org.alfresco.service.cmr.repository.ChildAssociationRef)
      */
+    @Override
     @Behaviour
             (
                     kind = BehaviourKind.CLASS,
                     type = "rma:hold",
-                    policy = "alf:onCreateNode",
+                    notificationFrequency = NotificationFrequency.TRANSACTION_COMMIT
-                    notificationFrequency= NotificationFrequency.TRANSACTION_COMMIT
             )
-    public void onCreateHold(ChildAssociationRef childAssociationRef)
+    public void onCreateNode(ChildAssociationRef childAssociationRef)
     {
         NodeRef holdNodeRef = childAssociationRef.getChildRef();
 

rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/audit/event/DeleteHoldAuditEvent.java

@@ -27,10 +27,12 @@
 
 package org.alfresco.module.org_alfresco_module_rm.audit.event;
 
+import static org.alfresco.repo.policy.Behaviour.NotificationFrequency.EVERY_EVENT;
+
 import java.io.Serializable;
 import java.util.Map;
 
-import org.alfresco.module.org_alfresco_module_rm.hold.HoldServicePolicies;
+import org.alfresco.repo.node.NodeServicePolicies;
 import org.alfresco.repo.policy.annotation.Behaviour;
 import org.alfresco.repo.policy.annotation.BehaviourBean;
 import org.alfresco.repo.policy.annotation.BehaviourKind;
@@ -38,18 +40,19 @@ import org.alfresco.service.cmr.repository.NodeRef;
 import org.alfresco.service.cmr.repository.NodeService;
 import org.alfresco.service.namespace.QName;
 
-import static org.alfresco.repo.policy.Behaviour.NotificationFrequency.EVERY_EVENT;
-
 /**
  * Delete hold audit event.
+ * This listens to the NodeServicePolicies.BeforeDeleteNodePolicy in order to cover the delete hold using nodes service
  *
  * @author Sara Aspery
  * @since 3.3
  */
 @BehaviourBean
-public class DeleteHoldAuditEvent extends AuditEvent implements HoldServicePolicies.BeforeDeleteHoldPolicy
+public class DeleteHoldAuditEvent extends AuditEvent implements NodeServicePolicies.BeforeDeleteNodePolicy
 {
-    /** Node Service */
+    /**
+     * Node Service
+     */
     private NodeService nodeService;
 
     /**
@@ -63,16 +66,15 @@ public class DeleteHoldAuditEvent extends AuditEvent implements HoldServicePolic
     }
 
     /**
-     * @see org.alfresco.module.org_alfresco_module_rm.hold.HoldServicePolicies.BeforeDeleteHoldPolicy#beforeDeleteHold(org.alfresco.service.cmr.repository.NodeRef)
+     * @see org.alfresco.repo.node.NodeServicePolicies.BeforeDeleteNodePolicy#beforeDeleteNode(org.alfresco.service.cmr.repository.NodeRef)
      */
     @Override
-    @Behaviour
+    @Behaviour (
-            (
+            kind = BehaviourKind.CLASS,
-                    kind = BehaviourKind.CLASS,
+            type = "rma:hold",
-                    type = "rma:hold",
+            notificationFrequency = EVERY_EVENT
-                    notificationFrequency = EVERY_EVENT
+    )
-            )
+    public void beforeDeleteNode(NodeRef holdNodeRef)
-    public void beforeDeleteHold(NodeRef holdNodeRef)
     {
         Map<QName, Serializable> auditProperties = HoldUtils.makePropertiesMap(holdNodeRef, nodeService);
         recordsManagementAuditService.auditEvent(holdNodeRef, getName(), auditProperties, null, true, false);

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

@@ -651,7 +651,8 @@ public class HoldServiceImpl extends ServiceBaseImpl
                 // fire before add to hold policy
                 invokeBeforeAddToHold(hold, nodeRef);
                 // run as system to ensure we have all the appropriate permissions to perform the manipulations we require
-                authenticationUtil.runAsSystem((RunAsWork<Void>) () -> {
+                authenticationUtil.runAsSystem((RunAsWork<Void>) () ->
+                {
                     // gather freeze properties
                     final Map<QName, Serializable> props = new HashMap<>(2);
                     props.put(PROP_FROZEN_AT, new Date());
@@ -671,11 +672,11 @@ public class HoldServiceImpl extends ServiceBaseImpl
                         records.forEach(record -> addFrozenAspect(record, props));
                     }
 
-                    // fire on add to hold policy
-                    invokeOnAddToHold(hold, nodeRef);
-
                     return null;
                 });
+
+                // fire on add to hold policy
+                invokeOnAddToHold(hold, nodeRef);
             }
         }
     }
@@ -791,6 +792,7 @@ public class HoldServiceImpl extends ServiceBaseImpl
 
         if (!holds.isEmpty())
         {
+            List<NodeRef> removedHolds = new ArrayList<>();
             for (final NodeRef hold : holds)
             {
                 if (!isHold(hold))
@@ -807,34 +809,38 @@ public class HoldServiceImpl extends ServiceBaseImpl
 
                 if (getHeld(hold).contains(nodeRef))
                 {
+                    // fire before remove from hold policy
+                    invokeBeforeRemoveFromHold(hold, nodeRef);
                     // run as system so we don't run into further permission issues
                     // we already know we have to have the correct capability to get here
-                    authenticationUtil.runAsSystem((RunAsWork<Void>) () -> {
+                    authenticationUtil.runAsSystem((RunAsWork<Void>) () ->
-                        // fire before remove from hold policy
+                    {
-                        invokeBeforeRemoveFromHold(hold, nodeRef);
                         // remove from hold
                         //set in transaction cache in order not to trigger update policy when removing the child association
                         transactionalResourceHelper.getSet("frozen").add(nodeRef);
                         nodeService.removeChild(hold, nodeRef);
 
-                        // audit that the node has been remove from the hold
+                        // audit that the node has been removed from the hold
                         // TODO add details of the hold that the node was removed from
                         recordsManagementAuditService.auditEvent(nodeRef, AUDIT_REMOVE_FROM_HOLD);
 
-                        // fire on remove from hold policy
-                        invokeOnRemoveFromHold(hold, nodeRef);
-
                         return null;
-
                     });
+                    removedHolds.add(hold);
                 }
             }
 
             // run as system as we can't be sure if have remove aspect rights on node
-            authenticationUtil.runAsSystem((RunAsWork<Void>) () -> {
+            authenticationUtil.runAsSystem((RunAsWork<Void>) () ->
+            {
                 removeFreezeAspect(nodeRef, 0);
                 return null;
             });
+            for (NodeRef removedHold : removedHolds)
+            {
+                // fire on remove from hold policy
+                invokeOnRemoveFromHold(removedHold, nodeRef);
+            }
         }
     }
 

rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java

@@ -26,10 +26,13 @@
  */
 package org.alfresco.module.org_alfresco_module_rm.util;
 
+import static java.util.Collections.unmodifiableList;
+
 import java.io.Serializable;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import org.alfresco.service.namespace.QName;
 
@@ -46,61 +49,98 @@ public class PropertyModificationAllowedCheck
      */
     private List<QName> whiteList;
 
+    /**
+     * List of model URI's for which the properties can be updated
+     */
+    private List<String> editableURIs;
+
+    /**
+     * Getter for list of model URI's
+     *
+     * @return return the list of model URI's
+     */
+    private List<String> getEditableURIs()
+    {
+        return unmodifiableList(editableURIs);
+    }
+
+    /**
+     * Setter for list of model URI's
+     *
+     * @param editableURIs List<String>
+     */
+    public void setEditableURIs(List<String> editableURIs)
+    {
+        this.editableURIs = unmodifiableList(editableURIs);
+    }
+
     /**
      * Setter for list of qnames
+     *
      * @param whiteList List<QName>
      */
     public void setWhiteList(List<QName> whiteList)
     {
-        this.whiteList = whiteList;
+        this.whiteList = unmodifiableList(whiteList);
+
     }
 
     /**
      * Compares the node properties with the requested update to make sure all potential updates are permitted
+     *
      * @param before current node properties
-     * @param after updated properties for the node
+     * @param after  updated properties for the node
-     * @return true -  if all modified property keys are in the whitelist
+     * @return true -  if all modified property keys are in the whitelist or
+     *                    in the list of model URI's for which the properties can be modified
      */
     public boolean check(Map<QName, Serializable> before, Map<QName, Serializable> after)
     {
         boolean proceed = true;
-        HashSet<QName> unionKeys = new HashSet<>(before.keySet());
+        // Initially check for changes to existing keys and values.
-        unionKeys.addAll(after.keySet());
+        for (final Map.Entry<QName, Serializable> entry : before.entrySet())
-        for (QName key : unionKeys)
         {
-            //Check if property has been added or removed
+            final QName key = entry.getKey();
-            if (!before.containsKey(key)  || !after.containsKey(key))
+            final Serializable beforeValue = entry.getValue();
+            //check if property has been updated
+            final boolean modified = after.containsKey(key) && after.get(key) != null
+                    && !after.get(key).equals(beforeValue);
+
+            //check if the property has been emptied or removed
+            final boolean propertyRemovedEmptied = (after.get(key) == null && beforeValue != null)
+                                            || !after.containsKey(key);
+            if (modified || propertyRemovedEmptied)
             {
-                //Property modified check to see if allowed
+                proceed = allowPropertyUpdate(key);
-                proceed = whiteList.contains(key);
-                if (!proceed)
-                {
-                    break;
-                }
             }
-            //Check if property emptied or empty property filled
+            if (!proceed)
-            if  ((before.get(key) == null && after.get(key) != null) ||
-                    (after.get(key) == null && before.get(key) != null))
             {
-                //Property modified check to see if allowed
+                return proceed;
-                proceed = whiteList.contains(key);
-                if (!proceed)
-                {
-                    break;
-                }
             }
-            //If properties aren't missing or empty check equality
+        }
-            if (before.get(key) != null && after.get(key) != null && !(after.get(key).equals(before.get(key))))
+
+        // Check for new values. Record individual values and group as a single map.
+        final Set<QName> newKeys = new HashSet<>(after.keySet());
+        newKeys.removeAll(before.keySet());
+        for (final QName key : newKeys)
+        {
+            proceed = allowPropertyUpdate(key);
+            if (!proceed)
             {
-                //Property modified check to see if allowed
+                break;
-                proceed = whiteList.contains(key);
-                if (!proceed)
-                {
-                    break;
-                }
             }
         }
         return proceed;
     }
 
+    /**
+     * Determines whether the property should be allowed to be updated or not.
+     *
+     * @param key property
+     * @return true if property update is allowed
+     */
+    private boolean allowPropertyUpdate(QName key)
+    {
+        return whiteList.contains(key) || getEditableURIs().contains(key.getNamespaceURI());
+    }
+
 }

rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/audit/event/CreateHoldAuditEventUnitTest.java

@@ -87,7 +87,7 @@ public class CreateHoldAuditEventUnitTest extends BaseUnitTest
     @Test
     public void testCreateHoldCausesAuditEvent()
     {
-        createHoldAuditEvent.onCreateHold(childAssociationRef);
+        createHoldAuditEvent.onCreateNode(childAssociationRef);
         verify(mockedRecordsManagementAuditService, times(1)).auditEvent(eq(holdNodeRef), any(String.class), isNull(Map.class), any(Map.class));
     }
 }

rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/audit/event/DeleteHoldAuditEventUnitTest.java

@@ -82,7 +82,7 @@ public class DeleteHoldAuditEventUnitTest extends BaseUnitTest
     @Test
     public void testDeleteHoldCausesAuditEvent()
     {
-        deleteHoldAuditEvent.beforeDeleteHold(holdNodeRef);
+        deleteHoldAuditEvent.beforeDeleteNode(holdNodeRef);
         verify(mockedRecordsManagementAuditService, times(1))
             .auditEvent(eq(holdNodeRef), any(String.class), any(Map.class), isNull(Map.class), Matchers.eq(true), Matchers.eq(false));
     }

rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheckUnitTest.java

@@ -58,6 +58,7 @@ public class PropertyModificationAllowedCheckUnitTest
     private QName qName, qName2;
 
     private List<QName> list;
+    private List<String> editableURIs;
 
     @Mock
     private Serializable serializable, serializable2;
@@ -67,13 +68,16 @@ public class PropertyModificationAllowedCheckUnitTest
     {
         MockitoAnnotations.initMocks(this);
         propertyModificationAllowedCheck = new PropertyModificationAllowedCheck();
-        before =  new HashMap();
+        before = new HashMap();
         after = new HashMap();
-        qName = QName.createQName("foo","bar");
+        qName = QName.createQName("foo", "bar");
         qName2 = QName.createQName("bar", "foo");
         before.put(qName, serializable);
         after.put(qName, serializable2);
         list = new ArrayList();
+        editableURIs = new ArrayList<>();
+        propertyModificationAllowedCheck.setWhiteList(list);
+        propertyModificationAllowedCheck.setEditableURIs(editableURIs);
     }
 
     /**
@@ -232,4 +236,26 @@ public class PropertyModificationAllowedCheckUnitTest
         after.put(qName, null);
         assertTrue(propertyModificationAllowedCheck.check(before, after));
     }
+
+    /**
+     * Test update of a property from the model URI for which properties can be updated
+     */
+    @Test
+    public void testUpdatePropertyFromAllowedModelURI()
+    {
+        editableURIs.add("foo");
+        propertyModificationAllowedCheck.setEditableURIs(editableURIs);
+        assertTrue(propertyModificationAllowedCheck.check(before, after));
+    }
+
+    /**
+     * Test update of a property that is not in the model URI for which properties can be updated
+     */
+    @Test
+    public void testUpdatePropertyFromNotAllowedModelURI()
+    {
+        editableURIs.add("bar");
+        propertyModificationAllowedCheck.setEditableURIs(editableURIs);
+        assertFalse(propertyModificationAllowedCheck.check(before, after));
+    }
 }