From e9105f0f0c76a101b7f6f804164ca6941b8a2b18 Mon Sep 17 00:00:00 2001 From: Tom Page Date: Thu, 29 Sep 2022 16:31:20 +0100 Subject: [PATCH 1/2] ACS-3377 Update rule order. --- .../org/alfresco/rest/rules/ReorderRules.java | 132 +++++++++++++++++- .../java/org/alfresco/rest/api/RuleSets.java | 10 ++ .../rest/api/impl/rules/RuleSetLoader.java | 2 +- .../rest/api/impl/rules/RuleSetsImpl.java | 44 +++++- .../rest/api/nodes/NodeRuleSetsRelation.java | 20 ++- .../rest/api/impl/rules/RuleSetsImplTest.java | 100 ++++++++++++- 6 files changed, 298 insertions(+), 10 deletions(-) diff --git a/packaging/tests/tas-restapi/src/test/java/org/alfresco/rest/rules/ReorderRules.java b/packaging/tests/tas-restapi/src/test/java/org/alfresco/rest/rules/ReorderRules.java index 3d270e756c..dcf9c9230e 100644 --- a/packaging/tests/tas-restapi/src/test/java/org/alfresco/rest/rules/ReorderRules.java +++ b/packaging/tests/tas-restapi/src/test/java/org/alfresco/rest/rules/ReorderRules.java @@ -29,14 +29,18 @@ import static java.util.stream.Collectors.toList; import static org.alfresco.rest.rules.RulesTestsUtils.createRuleModel; import static org.alfresco.utility.report.log.Step.STEP; +import static org.springframework.http.HttpStatus.FORBIDDEN; import static org.springframework.http.HttpStatus.OK; import java.util.List; import java.util.stream.IntStream; +import com.google.common.collect.Lists; + import org.alfresco.rest.RestTest; import org.alfresco.rest.model.RestRuleModel; import org.alfresco.rest.model.RestRuleSetModel; +import org.alfresco.utility.constants.UserRole; import org.alfresco.utility.model.FolderModel; import org.alfresco.utility.model.SiteModel; import org.alfresco.utility.model.TestGroup; @@ -64,10 +68,7 @@ public class ReorderRules extends RestTest { STEP("Create a folder containing three rules in the existing site"); FolderModel folder = dataContent.usingUser(user).usingSite(site).createFolder(); - List rules = IntStream.range(0, 3).mapToObj(index -> { - RestRuleModel ruleModel = createRuleModel("ruleName"); - return restClient.authenticateUser(user).withCoreAPI().usingNode(folder).usingDefaultRuleSet().createSingleRule(ruleModel); - }).collect(toList()); + List rules = createRulesInFolder(folder, user); STEP("Get the default rule set for the folder including the ordered rule ids"); RestRuleSetModel ruleSet = restClient.authenticateUser(user).withCoreAPI().usingNode(folder) @@ -77,4 +78,127 @@ public class ReorderRules extends RestTest restClient.assertStatusCodeIs(OK); ruleSet.assertThat().field("ruleIds").is(expectedRuleIds); } + + /** Check that a user can view the order of the rules in a rule set if they only have read permission. */ + @Test + public void getRuleSetAndRuleIdsWithReadOnlyPermission() + { + STEP("Create a site owned by admin and add user as a consumer"); + SiteModel siteModel = dataSite.usingAdmin().createPrivateRandomSite(); + dataUser.addUserToSite(user, siteModel, UserRole.SiteConsumer); + + STEP("Use admin to create a folder with a rule set and three rules in it"); + FolderModel ruleFolder = dataContent.usingAdmin().usingSite(siteModel).createFolder(); + dataContent.usingAdmin().usingResource(ruleFolder).createFolder(); + List rules = createRulesInFolder(ruleFolder, dataUser.getAdminUser()); + + STEP("Get the rule set with the ordered list of rules"); + RestRuleSetModel ruleSet = restClient.authenticateUser(user).withCoreAPI().usingNode(ruleFolder) + .include("ruleIds").getDefaultRuleSet(); + + restClient.assertStatusCodeIs(OK); + List ruleIds = rules.stream().map(RestRuleModel::getId).collect(toList()); + ruleSet.assertThat().field("ruleIds").is(ruleIds); + } + + /** Check we can reorder the rules in a rule set. */ + @Test (groups = { TestGroup.REST_API, TestGroup.RULES }) + public void reorderRules() + { + STEP("Create a folder containing three rules in the existing site"); + FolderModel folder = dataContent.usingUser(user).usingSite(site).createFolder(); + List rules = createRulesInFolder(folder, user); + + STEP("Reverse the order of the rules within the rule set"); + List reversedRuleIds = Lists.reverse(rules.stream().map(RestRuleModel::getId).collect(toList())); + RestRuleSetModel ruleSetBody = new RestRuleSetModel(); + ruleSetBody.setId("-default-"); + ruleSetBody.setRuleIds(reversedRuleIds); + RestRuleSetModel ruleSet = restClient.authenticateUser(user).withCoreAPI().usingNode(folder) + .include("ruleIds").updateRuleSet(ruleSetBody); + + restClient.assertStatusCodeIs(OK); + ruleSet.assertThat().field("ruleIds").is(reversedRuleIds); + } + + /** Check we can reorder the rules in a rule set by editing the response from the GET. */ + @Test (groups = { TestGroup.REST_API, TestGroup.RULES }) + public void reorderRulesUsingResponseFromGET() + { + STEP("Create a folder containing three rules in the existing site"); + FolderModel folder = dataContent.usingUser(user).usingSite(site).createFolder(); + List rules = createRulesInFolder(folder, user); + + STEP("Get the rule set with its id."); + RestRuleSetModel ruleSetResponse = restClient.authenticateUser(user).withCoreAPI().usingNode(folder) + .include("ruleIds").getDefaultRuleSet(); + + STEP("Reverse the order of the rules within the rule set"); + ruleSetResponse.setRuleIds(Lists.reverse(ruleSetResponse.getRuleIds())); + RestRuleSetModel ruleSet = restClient.authenticateUser(user).withCoreAPI().usingNode(folder) + .include("ruleIds").updateRuleSet(ruleSetResponse); + + restClient.assertStatusCodeIs(OK); + List reversedRuleIds = Lists.reverse(rules.stream().map(RestRuleModel::getId).collect(toList())); + ruleSet.assertThat().field("ruleIds").is(reversedRuleIds); + } + + /** Check that a user cannot reorder the rules in a rule set if they only have read permission. */ + @Test + public void reorderRulesWithoutPermission() + { + STEP("Create a site owned by admin and add user as a consumer"); + SiteModel siteModel = dataSite.usingAdmin().createPrivateRandomSite(); + dataUser.addUserToSite(user, siteModel, UserRole.SiteContributor); + + STEP("Use admin to create a folder with a rule set and three rules in it"); + FolderModel ruleFolder = dataContent.usingAdmin().usingSite(siteModel).createFolder(); + dataContent.usingAdmin().usingResource(ruleFolder).createFolder(); + List rules = createRulesInFolder(ruleFolder, dataUser.getAdminUser()); + + STEP("Try to reorder the rules as the contributor"); + List reversedRuleIds = Lists.reverse(rules.stream().map(RestRuleModel::getId).collect(toList())); + RestRuleSetModel ruleSetBody = new RestRuleSetModel(); + ruleSetBody.setId("-default-"); + ruleSetBody.setRuleIds(reversedRuleIds); + RestRuleSetModel ruleSet = restClient.authenticateUser(user).withCoreAPI().usingNode(ruleFolder) + .include("ruleIds").updateRuleSet(ruleSetBody); + + restClient.assertStatusCodeIs(FORBIDDEN); + } + + /** Check that a user cannot reorder the rules in a rule set if they only have read permission. */ + @Test + public void reorderRulesWithPermission() + { + STEP("Create a site owned by admin and add user as a collaborator"); + SiteModel siteModel = dataSite.usingAdmin().createPrivateRandomSite(); + dataUser.addUserToSite(user, siteModel, UserRole.SiteCollaborator); + + STEP("Use admin to create a folder with a rule set and three rules in it"); + FolderModel ruleFolder = dataContent.usingAdmin().usingSite(siteModel).createFolder(); + dataContent.usingAdmin().usingResource(ruleFolder).createFolder(); + List rules = createRulesInFolder(ruleFolder, dataUser.getAdminUser()); + + STEP("Try to reorder the rules as the contributor"); + List reversedRuleIds = Lists.reverse(rules.stream().map(RestRuleModel::getId).collect(toList())); + RestRuleSetModel ruleSetBody = new RestRuleSetModel(); + ruleSetBody.setId("-default-"); + ruleSetBody.setRuleIds(reversedRuleIds); + RestRuleSetModel ruleSet = restClient.authenticateUser(user).withCoreAPI().usingNode(ruleFolder) + .include("ruleIds").updateRuleSet(ruleSetBody); + + restClient.assertStatusCodeIs(OK); + ruleSet.assertThat().field("ruleIds").is(reversedRuleIds); + } + + /** Create three rules in the given folder. */ + private List createRulesInFolder(FolderModel folder, UserModel user) + { + return IntStream.range(0, 3).mapToObj(index -> + { + RestRuleModel ruleModel = createRuleModel("ruleName"); + return restClient.authenticateUser(user).withCoreAPI().usingNode(folder).usingDefaultRuleSet().createSingleRule(ruleModel); + }).collect(toList()); + } } diff --git a/remote-api/src/main/java/org/alfresco/rest/api/RuleSets.java b/remote-api/src/main/java/org/alfresco/rest/api/RuleSets.java index e77766b403..a5bcfa2455 100644 --- a/remote-api/src/main/java/org/alfresco/rest/api/RuleSets.java +++ b/remote-api/src/main/java/org/alfresco/rest/api/RuleSets.java @@ -59,6 +59,16 @@ public interface RuleSets */ RuleSet getRuleSetById(String folderNodeId, String ruleSetId, List includes); + /** + * Update a rule set - for example to reorder the rules within it. + * + * @param folderNodeId Folder node ID + * @param ruleSet The updated rule set. + * @param includes List of fields to include in the response. + * @return The updated rule set from the server. + */ + RuleSet updateRuleSet(String folderNodeId, RuleSet ruleSet, List includes); + /** * Link a rule set to a folder */ diff --git a/remote-api/src/main/java/org/alfresco/rest/api/impl/rules/RuleSetLoader.java b/remote-api/src/main/java/org/alfresco/rest/api/impl/rules/RuleSetLoader.java index 2d50a387f9..3f4acec8ff 100644 --- a/remote-api/src/main/java/org/alfresco/rest/api/impl/rules/RuleSetLoader.java +++ b/remote-api/src/main/java/org/alfresco/rest/api/impl/rules/RuleSetLoader.java @@ -149,7 +149,7 @@ public class RuleSetLoader ); } - private List loadRuleIds(NodeRef folderNodeRef) + public List loadRuleIds(NodeRef folderNodeRef) { return ruleService.getRules(folderNodeRef, false).stream() .map(restRuleModelMapper::toRestModel) diff --git a/remote-api/src/main/java/org/alfresco/rest/api/impl/rules/RuleSetsImpl.java b/remote-api/src/main/java/org/alfresco/rest/api/impl/rules/RuleSetsImpl.java index 0a55148fc6..0f4ea936c9 100644 --- a/remote-api/src/main/java/org/alfresco/rest/api/impl/rules/RuleSetsImpl.java +++ b/remote-api/src/main/java/org/alfresco/rest/api/impl/rules/RuleSetsImpl.java @@ -28,8 +28,14 @@ package org.alfresco.rest.api.impl.rules; import static java.util.stream.Collectors.toList; +import static org.alfresco.service.cmr.repository.StoreRef.STORE_REF_WORKSPACE_SPACESSTORE; +import static org.alfresco.util.collections.CollectionUtils.isEmpty; + +import java.util.HashSet; import java.util.List; import java.util.Objects; +import java.util.Set; +import java.util.stream.IntStream; import org.alfresco.repo.rule.RuleModel; import org.alfresco.repo.rule.RuntimeRuleService; @@ -41,7 +47,6 @@ import org.alfresco.rest.framework.resource.parameters.CollectionWithPagingInfo; import org.alfresco.rest.framework.resource.parameters.ListPage; import org.alfresco.rest.framework.resource.parameters.Paging; import org.alfresco.service.Experimental; -import org.alfresco.service.cmr.repository.AspectMissingException; import org.alfresco.service.cmr.repository.NodeRef; import org.alfresco.service.cmr.repository.NodeService; import org.alfresco.service.cmr.rule.RuleService; @@ -80,6 +85,43 @@ public class RuleSetsImpl implements RuleSets return ruleSetLoader.loadRuleSet(ruleSetNode, folderNode, includes); } + @Override + public RuleSet updateRuleSet(String folderNodeId, RuleSet ruleSet, List includes) + { + // Editing the order of the rules doesn't require permission to edit the rule set itself. + NodeRef folderNode = validator.validateFolderNode(folderNodeId, false); + NodeRef ruleSetNode = validator.validateRuleSetNode(ruleSet.getId(), folderNode); + + RuleSet returnedRuleSet = ruleSetLoader.loadRuleSet(ruleSetNode, folderNode, includes); + + // Currently the only field that can be updated is ruleIds to reorder the rules. + List suppliedRuleIds = ruleSet.getRuleIds(); + if (!isEmpty(suppliedRuleIds)) + { + // Check there are no duplicate rule ids in the request. + Set suppliedRuleIdSet = new HashSet<>(suppliedRuleIds); + + // Check that the set of rule ids hasn't changed. + Set existingRuleIds = new HashSet<>(ruleSetLoader.loadRuleIds(folderNode)); + if (!suppliedRuleIdSet.equals(existingRuleIds)) + { + throw new InvalidArgumentException("Unexpected set of rule ids - received " + suppliedRuleIds + " but expected " + existingRuleIds); + } + + IntStream.range(0, suppliedRuleIds.size()).forEach(index -> + { + NodeRef ruleNode = new NodeRef(STORE_REF_WORKSPACE_SPACESSTORE, suppliedRuleIds.get(index)); + ruleService.setRulePosition(folderNode, ruleNode, index); + }); + if (includes.contains(RuleSetLoader.RULE_IDS)) + { + returnedRuleSet.setRuleIds(suppliedRuleIds); + } + } + + return returnedRuleSet; + } + @Override public RuleSetLink linkToRuleSet(String folderNodeId, String linkToNodeId) { diff --git a/remote-api/src/main/java/org/alfresco/rest/api/nodes/NodeRuleSetsRelation.java b/remote-api/src/main/java/org/alfresco/rest/api/nodes/NodeRuleSetsRelation.java index ac6963a45c..6a0cf0bd60 100644 --- a/remote-api/src/main/java/org/alfresco/rest/api/nodes/NodeRuleSetsRelation.java +++ b/remote-api/src/main/java/org/alfresco/rest/api/nodes/NodeRuleSetsRelation.java @@ -28,8 +28,6 @@ package org.alfresco.rest.api.nodes; import javax.servlet.http.HttpServletResponse; -import java.util.List; - import org.alfresco.rest.api.RuleSets; import org.alfresco.rest.api.model.rules.RuleSet; import org.alfresco.rest.framework.WebApiDescription; @@ -37,7 +35,6 @@ import org.alfresco.rest.framework.core.exceptions.RelationshipResourceNotFoundE import org.alfresco.rest.framework.resource.RelationshipResource; import org.alfresco.rest.framework.resource.actions.interfaces.RelationshipResourceAction; import org.alfresco.rest.framework.resource.parameters.CollectionWithPagingInfo; -import org.alfresco.rest.framework.resource.parameters.Paging; import org.alfresco.rest.framework.resource.parameters.Parameters; import org.alfresco.service.Experimental; import org.alfresco.util.PropertyCheck; @@ -50,6 +47,7 @@ import org.springframework.beans.factory.InitializingBean; @RelationshipResource(name = "rule-sets", entityResource = NodesEntityResource.class, title = "Folder node rule sets") public class NodeRuleSetsRelation implements RelationshipResourceAction.Read, RelationshipResourceAction.ReadById, + RelationshipResourceAction.Update, InitializingBean { private RuleSets ruleSets; @@ -106,4 +104,20 @@ public class NodeRuleSetsRelation implements RelationshipResourceAction.Read + * - PUT /nodes/{folderNodeId}/rule-sets/{ruleSetId} + * + * @param folderNodeId The id for the folder. + * @param ruleSet The updated rule set. + * @param parameters Contains information about which fields to include in the response. + * @return The updated rule set. + */ + @Override + public RuleSet update(String folderNodeId, RuleSet ruleSet, Parameters parameters) + { + return ruleSets.updateRuleSet(folderNodeId, ruleSet, parameters.getInclude()); + } } diff --git a/remote-api/src/test/java/org/alfresco/rest/api/impl/rules/RuleSetsImplTest.java b/remote-api/src/test/java/org/alfresco/rest/api/impl/rules/RuleSetsImplTest.java index 86951183d5..904a312be3 100644 --- a/remote-api/src/test/java/org/alfresco/rest/api/impl/rules/RuleSetsImplTest.java +++ b/remote-api/src/test/java/org/alfresco/rest/api/impl/rules/RuleSetsImplTest.java @@ -27,6 +27,7 @@ package org.alfresco.rest.api.impl.rules; import static java.util.Collections.emptyList; +import static org.alfresco.rest.api.impl.rules.RuleSetLoader.RULE_IDS; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; @@ -46,7 +47,6 @@ import org.alfresco.rest.framework.core.exceptions.InvalidArgumentException; import org.alfresco.rest.framework.resource.parameters.CollectionWithPagingInfo; import org.alfresco.rest.framework.resource.parameters.Paging; import org.alfresco.service.Experimental; -import org.alfresco.service.cmr.repository.AspectMissingException; import org.alfresco.service.cmr.repository.ChildAssociationRef; import org.alfresco.service.cmr.repository.NodeRef; import org.alfresco.service.cmr.repository.NodeService; @@ -329,4 +329,102 @@ public class RuleSetsImplTest extends TestCase then(ruleServiceMock).shouldHaveNoMoreInteractions(); then(nodeServiceMock).shouldHaveNoInteractions(); } + + @Test + public void testUpdateRuleSet() + { + given(ruleSetMock.getId()).willReturn(RULE_SET_ID); + given(nodeValidatorMock.validateFolderNode(FOLDER_ID, false)).willReturn(FOLDER_NODE); + given(nodeValidatorMock.validateRuleSetNode(RULE_SET_ID, FOLDER_NODE)).willReturn(RULE_SET_NODE); + RuleSet ruleSetResponse = mock(RuleSet.class); + given(ruleSetLoaderMock.loadRuleSet(RULE_SET_NODE, FOLDER_NODE, emptyList())).willReturn(ruleSetResponse); + + //when + RuleSet ruleSet = ruleSets.updateRuleSet(FOLDER_ID, ruleSetMock, emptyList()); + + assertEquals("Unexpected rule set returned.", ruleSetResponse, ruleSet); + + then(nodeValidatorMock).should().validateFolderNode(FOLDER_ID, false); + then(nodeValidatorMock).should().validateRuleSetNode(RULE_SET_ID, FOLDER_NODE); + then(ruleSetLoaderMock).should().loadRuleSet(RULE_SET_NODE, FOLDER_NODE, emptyList()); + } + + /** Simulate rules being reordered from [RuleA, RuleB] to [RuleB, RuleA]. */ + @Test + public void testUpdateRuleSet_reorderRules() + { + List dbOrder = List.of("RuleA", "RuleB"); + List newOrder = List.of("RuleB", "RuleA"); + List includes = List.of(RULE_IDS); + + RuleSet dbRuleSet = mock(RuleSet.class); + RuleSet requestRuleSet = mock(RuleSet.class); + given(requestRuleSet.getId()).willReturn(RULE_SET_ID); + given(requestRuleSet.getRuleIds()).willReturn(newOrder); + + given(ruleSetLoaderMock.loadRuleSet(RULE_SET_NODE, FOLDER_NODE, includes)).willReturn(dbRuleSet); + given(ruleSetLoaderMock.loadRuleIds(FOLDER_NODE)).willReturn(dbOrder); + given(nodeValidatorMock.validateFolderNode(FOLDER_ID, false)).willReturn(FOLDER_NODE); + given(nodeValidatorMock.validateRuleSetNode(RULE_SET_ID, FOLDER_NODE)).willReturn(RULE_SET_NODE); + + //when + RuleSet ruleSet = ruleSets.updateRuleSet(FOLDER_ID, requestRuleSet, includes); + + assertEquals("Unexpected rule set returned.", dbRuleSet, ruleSet); + + then(nodeValidatorMock).should().validateFolderNode(FOLDER_ID, false); + then(nodeValidatorMock).should().validateRuleSetNode(RULE_SET_ID, FOLDER_NODE); + then(ruleSetLoaderMock).should().loadRuleSet(RULE_SET_NODE, FOLDER_NODE, includes); + then(dbRuleSet).should().setRuleIds(newOrder); + } + + /** Check that we can't remove a rule by updating the rule set. */ + @Test + public void testUpdateRuleSet_tryToChangeSetOfRuleIds() + { + List dbOrder = List.of("RuleA", "RuleB"); + List newOrder = List.of("RuleA"); + List includes = List.of(RULE_IDS); + + RuleSet dbRuleSet = mock(RuleSet.class); + RuleSet requestRuleSet = mock(RuleSet.class); + given(requestRuleSet.getId()).willReturn(RULE_SET_ID); + given(requestRuleSet.getRuleIds()).willReturn(newOrder); + + given(ruleSetLoaderMock.loadRuleSet(RULE_SET_NODE, FOLDER_NODE, includes)).willReturn(dbRuleSet); + given(ruleSetLoaderMock.loadRuleIds(FOLDER_NODE)).willReturn(dbOrder); + given(nodeValidatorMock.validateFolderNode(FOLDER_ID, false)).willReturn(FOLDER_NODE); + given(nodeValidatorMock.validateRuleSetNode(RULE_SET_ID, FOLDER_NODE)).willReturn(RULE_SET_NODE); + + //when + assertThatExceptionOfType(InvalidArgumentException.class).isThrownBy( + () -> ruleSets.updateRuleSet(FOLDER_ID, requestRuleSet, includes) + ); + } + + /** Check that we can update the rule ids without returning them. */ + @Test + public void testUpdateRuleSet_dontIncludeRuleIds() + { + List dbOrder = List.of("RuleA", "RuleB"); + List newOrder = List.of("RuleB", "RuleA"); + List includes = emptyList(); + + RuleSet dbRuleSet = mock(RuleSet.class); + RuleSet requestRuleSet = mock(RuleSet.class); + given(requestRuleSet.getId()).willReturn(RULE_SET_ID); + given(requestRuleSet.getRuleIds()).willReturn(newOrder); + + given(ruleSetLoaderMock.loadRuleSet(RULE_SET_NODE, FOLDER_NODE, includes)).willReturn(dbRuleSet); + given(ruleSetLoaderMock.loadRuleIds(FOLDER_NODE)).willReturn(dbOrder); + given(nodeValidatorMock.validateFolderNode(FOLDER_ID, false)).willReturn(FOLDER_NODE); + given(nodeValidatorMock.validateRuleSetNode(RULE_SET_ID, FOLDER_NODE)).willReturn(RULE_SET_NODE); + + //when + RuleSet ruleSet = ruleSets.updateRuleSet(FOLDER_ID, requestRuleSet, includes); + + // Expect the DB rule set to be returned, but no extra fields to be populated. + assertEquals("Unexpected rule set returned.", dbRuleSet, ruleSet); + then(dbRuleSet).shouldHaveNoInteractions(); + } } From f707906943d6d7e784cbd88e6723902215bba8a1 Mon Sep 17 00:00:00 2001 From: Tom Page Date: Mon, 3 Oct 2022 15:40:52 +0100 Subject: [PATCH 2/2] ACS-3377 Fix review comments. --- .../org/alfresco/rest/rules/ReorderRules.java | 10 ++++---- .../rest/api/impl/rules/RuleSetsImpl.java | 2 +- .../rest/api/impl/rules/RuleSetsImplTest.java | 24 +++++++++++++++++++ 3 files changed, 30 insertions(+), 6 deletions(-) diff --git a/packaging/tests/tas-restapi/src/test/java/org/alfresco/rest/rules/ReorderRules.java b/packaging/tests/tas-restapi/src/test/java/org/alfresco/rest/rules/ReorderRules.java index dcf9c9230e..b531da50da 100644 --- a/packaging/tests/tas-restapi/src/test/java/org/alfresco/rest/rules/ReorderRules.java +++ b/packaging/tests/tas-restapi/src/test/java/org/alfresco/rest/rules/ReorderRules.java @@ -27,7 +27,7 @@ package org.alfresco.rest.rules; import static java.util.stream.Collectors.toList; -import static org.alfresco.rest.rules.RulesTestsUtils.createRuleModel; +import static org.alfresco.rest.rules.RulesTestsUtils.createRuleModelWithDefaultValues; import static org.alfresco.utility.report.log.Step.STEP; import static org.springframework.http.HttpStatus.FORBIDDEN; import static org.springframework.http.HttpStatus.OK; @@ -161,13 +161,13 @@ public class ReorderRules extends RestTest RestRuleSetModel ruleSetBody = new RestRuleSetModel(); ruleSetBody.setId("-default-"); ruleSetBody.setRuleIds(reversedRuleIds); - RestRuleSetModel ruleSet = restClient.authenticateUser(user).withCoreAPI().usingNode(ruleFolder) - .include("ruleIds").updateRuleSet(ruleSetBody); + restClient.authenticateUser(user).withCoreAPI().usingNode(ruleFolder) + .include("ruleIds").updateRuleSet(ruleSetBody); restClient.assertStatusCodeIs(FORBIDDEN); } - /** Check that a user cannot reorder the rules in a rule set if they only have read permission. */ + /** Check that a user can reorder the rules in a rule set if they have write permission. */ @Test public void reorderRulesWithPermission() { @@ -197,7 +197,7 @@ public class ReorderRules extends RestTest { return IntStream.range(0, 3).mapToObj(index -> { - RestRuleModel ruleModel = createRuleModel("ruleName"); + RestRuleModel ruleModel = createRuleModelWithDefaultValues(); return restClient.authenticateUser(user).withCoreAPI().usingNode(folder).usingDefaultRuleSet().createSingleRule(ruleModel); }).collect(toList()); } diff --git a/remote-api/src/main/java/org/alfresco/rest/api/impl/rules/RuleSetsImpl.java b/remote-api/src/main/java/org/alfresco/rest/api/impl/rules/RuleSetsImpl.java index 0f4ea936c9..8b162290ca 100644 --- a/remote-api/src/main/java/org/alfresco/rest/api/impl/rules/RuleSetsImpl.java +++ b/remote-api/src/main/java/org/alfresco/rest/api/impl/rules/RuleSetsImpl.java @@ -103,7 +103,7 @@ public class RuleSetsImpl implements RuleSets // Check that the set of rule ids hasn't changed. Set existingRuleIds = new HashSet<>(ruleSetLoader.loadRuleIds(folderNode)); - if (!suppliedRuleIdSet.equals(existingRuleIds)) + if (suppliedRuleIdSet.size() != suppliedRuleIds.size() || !suppliedRuleIdSet.equals(existingRuleIds)) { throw new InvalidArgumentException("Unexpected set of rule ids - received " + suppliedRuleIds + " but expected " + existingRuleIds); } diff --git a/remote-api/src/test/java/org/alfresco/rest/api/impl/rules/RuleSetsImplTest.java b/remote-api/src/test/java/org/alfresco/rest/api/impl/rules/RuleSetsImplTest.java index 904a312be3..a53dac2e87 100644 --- a/remote-api/src/test/java/org/alfresco/rest/api/impl/rules/RuleSetsImplTest.java +++ b/remote-api/src/test/java/org/alfresco/rest/api/impl/rules/RuleSetsImplTest.java @@ -402,6 +402,30 @@ public class RuleSetsImplTest extends TestCase ); } + /** Check that we can't include a rule twice in a rule set. */ + @Test + public void testUpdateRuleSet_DuplicateRuleId() + { + List dbOrder = List.of("RuleA", "RuleB"); + List newOrder = List.of("RuleA", "RuleB", "RuleA"); + List includes = List.of(RULE_IDS); + + RuleSet dbRuleSet = mock(RuleSet.class); + RuleSet requestRuleSet = mock(RuleSet.class); + given(requestRuleSet.getId()).willReturn(RULE_SET_ID); + given(requestRuleSet.getRuleIds()).willReturn(newOrder); + + given(ruleSetLoaderMock.loadRuleSet(RULE_SET_NODE, FOLDER_NODE, includes)).willReturn(dbRuleSet); + given(ruleSetLoaderMock.loadRuleIds(FOLDER_NODE)).willReturn(dbOrder); + given(nodeValidatorMock.validateFolderNode(FOLDER_ID, false)).willReturn(FOLDER_NODE); + given(nodeValidatorMock.validateRuleSetNode(RULE_SET_ID, FOLDER_NODE)).willReturn(RULE_SET_NODE); + + //when + assertThatExceptionOfType(InvalidArgumentException.class).isThrownBy( + () -> ruleSets.updateRuleSet(FOLDER_ID, requestRuleSet, includes) + ); + } + /** Check that we can update the rule ids without returning them. */ @Test public void testUpdateRuleSet_dontIncludeRuleIds()