From c33e703be989c934770731f28c33627d67600d89 Mon Sep 17 00:00:00 2001 From: Rodica Sutu Date: Fri, 1 Nov 2019 09:51:48 +0200 Subject: [PATCH 01/10] add a list of namespace URIs for properties, which should be always editable for a frozen node --- .../rm-model-context.xml | 6 +++++ .../PropertyModificationAllowedCheck.java | 26 ++++++++++++++++++- ...pertyModificationAllowedCheckUnitTest.java | 4 +++ 3 files changed, 35 insertions(+), 1 deletion(-) diff --git a/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-model-context.xml b/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-model-context.xml index 932d24c525..d849ae16b1 100644 --- a/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-model-context.xml +++ b/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-model-context.xml @@ -177,6 +177,11 @@ + + + + http://www.alfresco.org/model/system/1.0 + @@ -185,6 +190,7 @@ + diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java index 8e98601b61..52c7f956dd 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java @@ -45,6 +45,29 @@ public class PropertyModificationAllowedCheck * List of qnames that can be modified */ private List whiteList; + + /** + * List of model URI's for which the properties can be updated + */ + private List editableURIs; + + /** + * Setter for list of model URI's + * @param editableURIs List + */ + public void setEditableURIs(List editableURIs) + { + this.editableURIs = editableURIs; + } + + /** + * Getter for list of model URI's + * @return return the list of model URI's + */ + public List getEditableURIs() + { + return editableURIs; + } /** * Setter for list of qnames @@ -93,7 +116,7 @@ public class PropertyModificationAllowedCheck if (before.get(key) != null && after.get(key) != null && !(after.get(key).equals(before.get(key)))) { //Property modified check to see if allowed - proceed = whiteList.contains(key); + proceed = whiteList.contains(key) || getEditableURIs().contains(key.getNamespaceURI()); if (!proceed) { break; @@ -103,4 +126,5 @@ public class PropertyModificationAllowedCheck return proceed; } + } diff --git a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheckUnitTest.java b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheckUnitTest.java index 1b192a62c0..3cb5e89c0d 100644 --- a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheckUnitTest.java +++ b/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 list; + private List editableURIs; @Mock private Serializable serializable, serializable2; @@ -74,6 +75,9 @@ public class PropertyModificationAllowedCheckUnitTest before.put(qName, serializable); after.put(qName, serializable2); list = new ArrayList(); + editableURIs =new ArrayList<>(); + propertyModificationAllowedCheck.setWhiteList(list); + propertyModificationAllowedCheck.setEditableURIs(editableURIs); } /** From 671d6b954c79be13a9767d773e3846e4c347b1d3 Mon Sep 17 00:00:00 2001 From: Rodica Sutu Date: Mon, 4 Nov 2019 11:39:35 +0200 Subject: [PATCH 02/10] add unit tests --- .../PropertyModificationAllowedCheck.java | 15 ++++++++++--- ...pertyModificationAllowedCheckUnitTest.java | 22 +++++++++++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java index 52c7f956dd..b1d90afa65 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java @@ -95,7 +95,7 @@ public class PropertyModificationAllowedCheck if (!before.containsKey(key) || !after.containsKey(key)) { //Property modified check to see if allowed - proceed = whiteList.contains(key); + proceed = allowPropertyUpdate(key); if (!proceed) { break; @@ -106,7 +106,7 @@ public class PropertyModificationAllowedCheck (after.get(key) == null && before.get(key) != null)) { //Property modified check to see if allowed - proceed = whiteList.contains(key); + proceed = allowPropertyUpdate(key); if (!proceed) { break; @@ -116,7 +116,7 @@ public class PropertyModificationAllowedCheck if (before.get(key) != null && after.get(key) != null && !(after.get(key).equals(before.get(key)))) { //Property modified check to see if allowed - proceed = whiteList.contains(key) || getEditableURIs().contains(key.getNamespaceURI()); + proceed = allowPropertyUpdate(key); if (!proceed) { break; @@ -126,5 +126,14 @@ public class PropertyModificationAllowedCheck return proceed; } + /** + * Determines whether the property should be allowed to be updated or not. + * @param key property + * @return true if property update us allowed + */ + private boolean allowPropertyUpdate(QName key) + { + return whiteList.contains(key) || getEditableURIs().contains(key.getNamespaceURI()); + } } diff --git a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheckUnitTest.java b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheckUnitTest.java index 3cb5e89c0d..fca0a8b9cb 100644 --- a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheckUnitTest.java +++ b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheckUnitTest.java @@ -236,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)); + } } From 9bbb0b351682a434676a8384bc2b7754d9f86723 Mon Sep 17 00:00:00 2001 From: Rodica Sutu Date: Mon, 4 Nov 2019 17:24:09 +0200 Subject: [PATCH 03/10] refactor the code to fix the sonar issue "Refactor this method to reduce its Cognitive Complexity from 21 to the 15 allowed." --- .../PropertyModificationAllowedCheck.java | 56 +++++++++---------- 1 file changed, 26 insertions(+), 30 deletions(-) diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java index b1d90afa65..49dc26b716 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java @@ -30,6 +30,7 @@ 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; @@ -80,49 +81,44 @@ public class PropertyModificationAllowedCheck /** * 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 */ public boolean check(Map before, Map after) { boolean proceed = true; - HashSet unionKeys = new HashSet<>(before.keySet()); - unionKeys.addAll(after.keySet()); - for (QName key : unionKeys) + // Initially check for changes to existing keys and values. + for (Map.Entry entry : before.entrySet()) { - //Check if property has been added or removed - if (!before.containsKey(key) || !after.containsKey(key)) + QName key = entry.getKey(); + Serializable beforeValue = entry.getValue(); + //check if property has been updated + boolean modified = after.containsKey(key) && after.get(key) != null + && !after.get(key).equals(beforeValue); + + //check if the property has been emptied or removed + boolean propertyRemovedEmptied = after.get(key) == null || !after.containsKey(key); + if (modified || propertyRemovedEmptied) { - //Property modified check to see if allowed proceed = allowPropertyUpdate(key); - if (!proceed) - { - break; - } } - //Check if property emptied or empty property filled - if ((before.get(key) == null && after.get(key) != null) || - (after.get(key) == null && before.get(key) != null)) + if (!proceed) { - //Property modified check to see if allowed - proceed = allowPropertyUpdate(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)))) - { - //Property modified check to see if allowed - proceed = allowPropertyUpdate(key); - if (!proceed) - { - break; - } + return proceed; } } + + // Check for new values. Record individual values and group as a single map. + Set newKeys = new HashSet<>(after.keySet()); + newKeys.removeAll(before.keySet()); + for (QName key : newKeys) + { + proceed = allowPropertyUpdate(key); + if (!proceed) + break; + } return proceed; } From f54ab26b88f7aeabb38bd1229e984f80114e62b4 Mon Sep 17 00:00:00 2001 From: Rodica Sutu Date: Mon, 4 Nov 2019 17:42:25 +0200 Subject: [PATCH 04/10] fix the case when before and after values are null --- .../util/PropertyModificationAllowedCheck.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java index 49dc26b716..592f704971 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java @@ -99,7 +99,8 @@ public class PropertyModificationAllowedCheck && !after.get(key).equals(beforeValue); //check if the property has been emptied or removed - boolean propertyRemovedEmptied = after.get(key) == null || !after.containsKey(key); + boolean propertyRemovedEmptied = (after.get(key) == null && beforeValue !=null) + || !after.containsKey(key); if (modified || propertyRemovedEmptied) { proceed = allowPropertyUpdate(key); From 9e62b9a8585f2a654206d5c1b214d8a111c853fa Mon Sep 17 00:00:00 2001 From: Rodica Sutu Date: Tue, 5 Nov 2019 11:43:32 +0200 Subject: [PATCH 05/10] code reviews changes and couple sonar fixes --- .../rm-model-context.xml | 2 +- .../PropertyModificationAllowedCheck.java | 46 +++++++++++-------- ...pertyModificationAllowedCheckUnitTest.java | 6 +-- 3 files changed, 30 insertions(+), 24 deletions(-) diff --git a/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-model-context.xml b/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-model-context.xml index d849ae16b1..9119c2f01c 100644 --- a/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-model-context.xml +++ b/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-model-context.xml @@ -178,7 +178,7 @@ - + http://www.alfresco.org/model/system/1.0 diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java index 592f704971..68de8e85d8 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java @@ -46,14 +46,25 @@ public class PropertyModificationAllowedCheck * List of qnames that can be modified */ private List whiteList; - - /** + + /** * List of model URI's for which the properties can be updated */ private List editableURIs; + /** + * Getter for list of model URI's + * + * @return return the list of model URI's + */ + private List getEditableURIs() + { + return editableURIs; + } + /** * Setter for list of model URI's + * * @param editableURIs List */ public void setEditableURIs(List editableURIs) @@ -61,17 +72,9 @@ public class PropertyModificationAllowedCheck this.editableURIs = editableURIs; } - /** - * Getter for list of model URI's - * @return return the list of model URI's - */ - public List getEditableURIs() - { - return editableURIs; - } - /** * Setter for list of qnames + * * @param whiteList List */ public void setWhiteList(List whiteList) @@ -90,17 +93,17 @@ public class PropertyModificationAllowedCheck { boolean proceed = true; // Initially check for changes to existing keys and values. - for (Map.Entry entry : before.entrySet()) + for (final Map.Entry entry : before.entrySet()) { - QName key = entry.getKey(); - Serializable beforeValue = entry.getValue(); + final QName key = entry.getKey(); + final Serializable beforeValue = entry.getValue(); //check if property has been updated - boolean modified = after.containsKey(key) && after.get(key) != null + final boolean modified = after.containsKey(key) && after.get(key) != null && !after.get(key).equals(beforeValue); //check if the property has been emptied or removed - boolean propertyRemovedEmptied = (after.get(key) == null && beforeValue !=null) - || !after.containsKey(key); + final boolean propertyRemovedEmptied = (after.get(key) == null && beforeValue != null) + || !after.containsKey(key); if (modified || propertyRemovedEmptied) { proceed = allowPropertyUpdate(key); @@ -112,21 +115,24 @@ public class PropertyModificationAllowedCheck } // Check for new values. Record individual values and group as a single map. - Set newKeys = new HashSet<>(after.keySet()); + final Set newKeys = new HashSet<>(after.keySet()); newKeys.removeAll(before.keySet()); - for (QName key : newKeys) + for (final QName key : newKeys) { proceed = allowPropertyUpdate(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 us allowed + * @return true if property update is allowed */ private boolean allowPropertyUpdate(QName key) { diff --git a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheckUnitTest.java b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheckUnitTest.java index fca0a8b9cb..a152a7c533 100644 --- a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheckUnitTest.java +++ b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheckUnitTest.java @@ -68,14 +68,14 @@ 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<>(); + editableURIs = new ArrayList<>(); propertyModificationAllowedCheck.setWhiteList(list); propertyModificationAllowedCheck.setEditableURIs(editableURIs); } From 02bdf0d92a523610a73c7594cd29f27f52959f10 Mon Sep 17 00:00:00 2001 From: Rodica Sutu Date: Tue, 5 Nov 2019 16:43:57 +0200 Subject: [PATCH 06/10] update java docs & fix sonar "Mutable members should not be stored or returned directly" from PropertyModificationAllowedCheck class --- .../util/PropertyModificationAllowedCheck.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java index 68de8e85d8..88544fbbd5 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java @@ -26,6 +26,8 @@ */ 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; @@ -59,7 +61,7 @@ public class PropertyModificationAllowedCheck */ private List getEditableURIs() { - return editableURIs; + return unmodifiableList(editableURIs); } /** @@ -69,7 +71,7 @@ public class PropertyModificationAllowedCheck */ public void setEditableURIs(List editableURIs) { - this.editableURIs = editableURIs; + this.editableURIs = unmodifiableList(editableURIs); } /** @@ -79,7 +81,8 @@ public class PropertyModificationAllowedCheck */ public void setWhiteList(List whiteList) { - this.whiteList = whiteList; + this.whiteList = unmodifiableList(whiteList); + } /** @@ -87,7 +90,8 @@ public class PropertyModificationAllowedCheck * * @param before current node properties * @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 before, Map after) { From 295350bb081127f70efddb646073b19b7b661aa6 Mon Sep 17 00:00:00 2001 From: Sara Aspery Date: Thu, 7 Nov 2019 12:54:15 +0000 Subject: [PATCH 07/10] RM-7028 update from review --- .../alfresco/module/org_alfresco_module_rm/rm-action-context.xml | 1 - 1 file changed, 1 deletion(-) diff --git a/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-action-context.xml b/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-action-context.xml index 88765a490b..649022988c 100644 --- a/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-action-context.xml +++ b/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-action-context.xml @@ -1025,7 +1025,6 @@ - From 65f30701d1b96de034091bd6772a29bab4420632 Mon Sep 17 00:00:00 2001 From: Roxana Lucanu Date: Fri, 8 Nov 2019 14:33:43 +0200 Subject: [PATCH 08/10] RM-7054 Changes on held content policies --- .../hold/HoldServiceImpl.java | 32 +++++++++++-------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java index 4b817cabc7..a445306e6d 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java @@ -653,7 +653,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) () -> { + authenticationUtil.runAsSystem((RunAsWork) () -> + { // gather freeze properties final Map props = new HashMap<>(2); props.put(PROP_FROZEN_AT, new Date()); @@ -676,11 +677,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); } } } @@ -796,6 +797,7 @@ public class HoldServiceImpl extends ServiceBaseImpl if (!holds.isEmpty()) { + List removedHolds = new ArrayList(); for (final NodeRef hold : holds) { if (!isHold(hold)) @@ -812,34 +814,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) () -> { - // fire before remove from hold policy - invokeBeforeRemoveFromHold(hold, nodeRef); + authenticationUtil.runAsSystem((RunAsWork) () -> + { // 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) () -> { + authenticationUtil.runAsSystem((RunAsWork) () -> + { removeFreezeAspect(nodeRef, 0); return null; }); + for (NodeRef removedHold : removedHolds) + { + // fire on remove from hold policy + invokeOnRemoveFromHold(removedHold, nodeRef); + } } } From 1278258a7451a67abb6fa4ff5a9957b803f43b1e Mon Sep 17 00:00:00 2001 From: Roxana Lucanu Date: Tue, 12 Nov 2019 10:36:42 +0200 Subject: [PATCH 09/10] RM-7054 code review changes --- .../module/org_alfresco_module_rm/hold/HoldServiceImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java index a445306e6d..d286a70d63 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java @@ -797,7 +797,7 @@ public class HoldServiceImpl extends ServiceBaseImpl if (!holds.isEmpty()) { - List removedHolds = new ArrayList(); + List removedHolds = new ArrayList<>(); for (final NodeRef hold : holds) { if (!isHold(hold)) From c386f1bbd83ab97df8aec4f2e17381f9214c0d4a Mon Sep 17 00:00:00 2001 From: Ramona Popa Date: Mon, 18 Nov 2019 08:34:54 +0000 Subject: [PATCH 10/10] RM-7057: Delete hold event isn't audited if hold is deleted using nodes api -listen to beforeDeleteNode policy rather than beforeDeleteHold one to cover the delete the hold from api as well --- .../audit/event/CreateHoldAuditEvent.java | 19 ++++++++----- .../audit/event/DeleteHoldAuditEvent.java | 28 ++++++++++--------- .../event/CreateHoldAuditEventUnitTest.java | 2 +- .../event/DeleteHoldAuditEventUnitTest.java | 2 +- 4 files changed, 29 insertions(+), 22 deletions(-) diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/audit/event/CreateHoldAuditEvent.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/audit/event/CreateHoldAuditEvent.java index 6e585cecd3..922ad61459 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/audit/event/CreateHoldAuditEvent.java +++ b/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(); diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/audit/event/DeleteHoldAuditEvent.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/audit/event/DeleteHoldAuditEvent.java index 2789df192b..a2ecc1fbc2 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/audit/event/DeleteHoldAuditEvent.java +++ b/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 - ( - kind = BehaviourKind.CLASS, - type = "rma:hold", - notificationFrequency = EVERY_EVENT - ) - public void beforeDeleteHold(NodeRef holdNodeRef) + @Behaviour ( + kind = BehaviourKind.CLASS, + type = "rma:hold", + notificationFrequency = EVERY_EVENT + ) + public void beforeDeleteNode(NodeRef holdNodeRef) { Map auditProperties = HoldUtils.makePropertiesMap(holdNodeRef, nodeService); recordsManagementAuditService.auditEvent(holdNodeRef, getName(), auditProperties, null, true, false); diff --git a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/audit/event/CreateHoldAuditEventUnitTest.java b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/audit/event/CreateHoldAuditEventUnitTest.java index a82a77c619..9c4038d6df 100644 --- a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/audit/event/CreateHoldAuditEventUnitTest.java +++ b/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)); } } diff --git a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/audit/event/DeleteHoldAuditEventUnitTest.java b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/audit/event/DeleteHoldAuditEventUnitTest.java index 5f787af6f5..73e0ccb215 100644 --- a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/audit/event/DeleteHoldAuditEventUnitTest.java +++ b/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)); }