ACS-3964: fix for non-inheritable rules still appearing in inherited rule sets (#1565)

* ACS-3964: fix for non-inheritable rules still appearing in inherited rule sets

* ACS-3964: fix failing E2Es and add negative test

packaging/tests/tas-restapi/src/test/java/org/alfresco/rest/rules/GetInheritedRulesTests.java

@@ -25,6 +25,7 @@
  */
 package org.alfresco.rest.rules;
 
+import static org.alfresco.rest.requests.RuleSettings.IS_INHERITANCE_ENABLED;
 import static org.alfresco.utility.report.log.Step.STEP;
 import static org.testng.Assert.assertEquals;
 
@@ -38,10 +39,12 @@ import org.alfresco.rest.model.RestRuleModelsCollection;
 import org.alfresco.rest.model.RestRuleSetLinkModel;
 import org.alfresco.rest.model.RestRuleSetModel;
 import org.alfresco.rest.model.RestRuleSetModelsCollection;
+import org.alfresco.rest.model.RestRuleSettingsModel;
 import org.alfresco.utility.model.FolderModel;
 import org.alfresco.utility.model.SiteModel;
 import org.alfresco.utility.model.TestGroup;
 import org.alfresco.utility.model.UserModel;
+import org.springframework.http.HttpStatus;
 import org.testng.annotations.BeforeClass;
 import org.testng.annotations.Test;
 
@@ -71,26 +74,96 @@ public class GetInheritedRulesTests extends RestTest
         STEP("Create a parent and child folder, each with inheriting rules");
         FolderModel parent = dataContent.usingUser(user).usingSite(site).createFolder();
         FolderModel child = dataContent.usingUser(user).usingResource(parent).createFolder();
-        RestRuleModel parentRule = rulesUtils.createRuleModelWithDefaultValues();
+        RestRuleModel parentRule = rulesUtils.createInheritableRuleModel();
         parentRule = restClient.authenticateUser(user).withPrivateAPI().usingNode(parent).usingDefaultRuleSet().createSingleRule(parentRule);
+        restClient.assertStatusCodeIs(HttpStatus.CREATED);
+
+        RestRuleSettingsModel enabled = new RestRuleSettingsModel();
+        enabled.setValue(true);
+        restClient.authenticateUser(user).withPrivateAPI().usingNode(child).usingRuleSetting(IS_INHERITANCE_ENABLED).updateSetting(enabled);
+        restClient.assertStatusCodeIs(HttpStatus.OK);
+
         RestRuleModel childRule = rulesUtils.createRuleModelWithDefaultValues();
         childRule = restClient.authenticateUser(user).withPrivateAPI().usingNode(child).usingDefaultRuleSet().createSingleRule(childRule);
+        restClient.assertStatusCodeIs(HttpStatus.CREATED);
 
         STEP("Get the rules in the default rule set for the child folder");
         RestRuleModelsCollection rules = restClient.authenticateUser(user).withPrivateAPI().usingNode(child).usingDefaultRuleSet().getListOfRules();
+        restClient.assertStatusCodeIs(HttpStatus.OK);
         rules.assertThat().entriesListContains("id", childRule.getId())
              .and().entriesListCountIs(1);
 
         STEP("Get the rules in the inherited rule set for the child folder");
         RestRuleSetModelsCollection ruleSets = restClient.authenticateUser(user).withPrivateAPI().usingNode(child).include("inclusionType").getListOfRuleSets();
+        restClient.assertStatusCodeIs(HttpStatus.OK);
         String inheritedRuleSetId = ruleSets.getEntries().stream()
                                             .filter(ruleSet -> ruleSet.onModel().getInclusionType().equals("inherited"))
                                             .findFirst().get().onModel().getId();
+
         RestRuleModelsCollection inheritedRules = restClient.authenticateUser(user).withPrivateAPI().usingNode(child).usingRuleSet(inheritedRuleSetId).getListOfRules();
+        restClient.assertStatusCodeIs(HttpStatus.OK);
         inheritedRules.assertThat().entriesListContains("id", parentRule.getId())
                       .and().entriesListCountIs(1);
     }
 
+    /**
+     * Check we get no (inherited) rules when inheritance is disabled in the child folder.
+     */
+    @Test (groups = { TestGroup.REST_API, TestGroup.RULES, TestGroup.SANITY })
+    public void getInheritedRules_childFolderInheritanceDisabled()
+    {
+        STEP("Create a parent and child folder, with inheritable parent rule");
+        FolderModel parent = dataContent.usingUser(user).usingSite(site).createFolder();
+        FolderModel child = dataContent.usingUser(user).usingResource(parent).createFolder();
+        RestRuleModel parentRule = rulesUtils.createInheritableRuleModel();
+        restClient.authenticateUser(user).withPrivateAPI().usingNode(parent).usingDefaultRuleSet().createSingleRule(parentRule);
+        restClient.assertStatusCodeIs(HttpStatus.CREATED);
+
+        STEP("Disable inheritance in the child folder");
+        RestRuleSettingsModel enabledInheritance = new RestRuleSettingsModel();
+        enabledInheritance.setValue(false);
+        restClient.authenticateUser(user).withPrivateAPI().usingNode(child).usingRuleSetting(IS_INHERITANCE_ENABLED).updateSetting(enabledInheritance);
+        restClient.assertStatusCodeIs(HttpStatus.OK);
+
+        STEP("The child folder should have no rule sets");
+        RestRuleSetModelsCollection ruleSets = restClient.authenticateUser(user).withPrivateAPI().usingNode(child).getListOfRuleSets();
+        restClient.assertStatusCodeIs(HttpStatus.OK);
+        ruleSets.assertThat().entriesListIsEmpty();
+    }
+
+    /**
+     * Check that non-inheritable rules owned by the parent folder are not found inside the child folder.
+     */
+    @Test (groups = { TestGroup.REST_API, TestGroup.RULES, TestGroup.SANITY })
+    public void inheritance_test()
+    {
+        STEP("Create a parent and child folder, with an inheritable and a non-inheritable parent rule");
+        FolderModel parent = dataContent.usingUser(user).usingSite(site).createFolder();
+        FolderModel child = dataContent.usingUser(user).usingResource(parent).createFolder();
+
+        RestRuleModel inheritableRule = rulesUtils.createInheritableRuleModel();
+        inheritableRule = restClient.authenticateUser(user).withPrivateAPI().usingNode(parent).usingDefaultRuleSet().createSingleRule(inheritableRule);
+        restClient.assertStatusCodeIs(HttpStatus.CREATED);
+
+        RestRuleModel nonInheritableRule = rulesUtils.createRuleModelWithDefaultValues();
+        nonInheritableRule = restClient.authenticateUser(user).withPrivateAPI().usingNode(parent).usingDefaultRuleSet().createSingleRule(nonInheritableRule);
+        restClient.assertStatusCodeIs(HttpStatus.CREATED);
+
+        STEP("The inherited rule set for the child folder should only return the inheritable rule");
+        RestRuleSetModelsCollection ruleSets = restClient.authenticateUser(user).withPrivateAPI().usingNode(child).include("inclusionType").getListOfRuleSets();
+        restClient.assertStatusCodeIs(HttpStatus.OK);
+
+        String inheritedRuleSetId = ruleSets.getEntries().stream()
+                .filter(ruleSet -> ruleSet.onModel().getInclusionType().equals("inherited"))
+                .findFirst().get().onModel().getId();
+
+        RestRuleModelsCollection inheritedRules = restClient.authenticateUser(user).withPrivateAPI().usingNode(child).usingRuleSet(inheritedRuleSetId).getListOfRules();
+        restClient.assertStatusCodeIs(HttpStatus.OK);
+        inheritedRules.assertThat().entriesListContains("id", inheritableRule.getId())
+                             .and().entriesListDoesNotContain("id",nonInheritableRule.getId())
+                             .and().entriesListCountIs(1);
+    }
+
     /**
      * Check that we only get each rule once with linking and inheritance, and the order is correct.
      * <p>
@@ -110,9 +183,14 @@ public class GetInheritedRulesTests extends RestTest
         FolderModel folderB = dataContent.usingUser(user).usingResource(folderA).createFolder();
         FolderModel folderC = dataContent.usingUser(user).usingResource(folderB).createFolder();
         FolderModel folderD = dataContent.usingUser(user).usingResource(folderC).createFolder();
-        RestRuleModel ruleB = restClient.authenticateUser(user).withPrivateAPI().usingNode(folderB).usingDefaultRuleSet().createSingleRule(rulesUtils.createRuleModelWithDefaultValues());
+        RestRuleModel ruleB = restClient.authenticateUser(user).withPrivateAPI().usingNode(folderB).usingDefaultRuleSet().createSingleRule(rulesUtils.createInheritableRuleModel());
-        RestRuleModel ruleC = restClient.authenticateUser(user).withPrivateAPI().usingNode(folderC).usingDefaultRuleSet().createSingleRule(rulesUtils.createRuleModelWithDefaultValues());
+        RestRuleModel ruleC = restClient.authenticateUser(user).withPrivateAPI().usingNode(folderC).usingDefaultRuleSet().createSingleRule(rulesUtils.createInheritableRuleModel());
         RestRuleModel ruleD = restClient.authenticateUser(user).withPrivateAPI().usingNode(folderD).usingDefaultRuleSet().createSingleRule(rulesUtils.createRuleModelWithDefaultValues());
+        RestRuleSettingsModel enabled = new RestRuleSettingsModel();
+        enabled.setValue(true);
+        restClient.authenticateUser(user).withPrivateAPI().usingNode(folderC).usingRuleSetting(IS_INHERITANCE_ENABLED).updateSetting(enabled);
+        restClient.authenticateUser(user).withPrivateAPI().usingNode(folderD).usingRuleSetting(IS_INHERITANCE_ENABLED).updateSetting(enabled);
+
         STEP("Link folderA to ruleSetD");
         RestRuleSetLinkModel linkModel = new RestRuleSetLinkModel();
         linkModel.setId(folderD.getNodeRef());

packaging/tests/tas-restapi/src/test/java/org/alfresco/rest/rules/RulesTestsUtils.java

@@ -213,6 +213,13 @@ public class RulesTestsUtils
         return createRuleModel(RULE_NAME_DEFAULT);
     }
 
+    public RestRuleModel createInheritableRuleModel()
+    {
+        RestRuleModel ruleModel = createRuleModel(RULE_NAME_DEFAULT);
+        ruleModel.setIsInheritable(true);
+        return ruleModel;
+    }
+
     public RestRuleModel createRuleModel(String name)
     {
         return createRuleModel(name, List.of(createAddAudioAspectAction()));

remote-api/src/main/java/org/alfresco/rest/api/impl/rules/RulesImpl.java

@@ -26,17 +26,12 @@
 
 package org.alfresco.rest.api.impl.rules;
 
-import java.io.Serializable;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.stream.Collectors;
-
 import org.alfresco.repo.action.ActionImpl;
 import org.alfresco.repo.action.access.ActionAccessRestriction;
 import org.alfresco.repo.action.executer.ExecuteAllRulesActionExecuter;
 import org.alfresco.rest.api.Rules;
 import org.alfresco.rest.api.model.mapper.RestModelMapper;
+import org.alfresco.rest.api.model.rules.InclusionType;
 import org.alfresco.rest.api.model.rules.Rule;
 import org.alfresco.rest.api.model.rules.RuleExecution;
 import org.alfresco.rest.api.model.rules.RuleSet;
@@ -53,6 +48,14 @@ import org.apache.commons.collections.CollectionUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import java.io.Serializable;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+import static org.alfresco.rest.api.impl.rules.RuleSetLoader.INCLUSION_TYPE;
+
 @Experimental
 public class RulesImpl implements Rules
 {
@@ -63,6 +66,7 @@ public class RulesImpl implements Rules
     private RuleService ruleService;
     private NodeValidator validator;
     private RuleLoader ruleLoader;
+    private RuleSetLoader ruleSetLoader;
     private ActionPermissionValidator actionPermissionValidator;
     private RestModelMapper<Rule, org.alfresco.service.cmr.rule.Rule> ruleMapper;
 
@@ -75,8 +79,10 @@ public class RulesImpl implements Rules
         final NodeRef folderNodeRef = validator.validateFolderNode(folderNodeId, false);
         NodeRef ruleSetNode = validator.validateRuleSetNode(ruleSetId, folderNodeRef);
         NodeRef owningFolder = ruleService.getOwningNodeRef(ruleSetNode);
+        RuleSet ruleSet = ruleSetLoader.loadRuleSet(ruleSetNode, folderNodeRef, List.of(INCLUSION_TYPE));
 
         final List<Rule> rules = ruleService.getRules(owningFolder, false).stream()
+                .filter(ruleModel -> ruleSet.getInclusionType() != InclusionType.INHERITED || ruleModel.isAppliedToChildren())
                 .map(ruleModel -> ruleLoader.loadRule(ruleModel, includes))
                 .collect(Collectors.toList());
 
@@ -182,6 +188,11 @@ public class RulesImpl implements Rules
         this.ruleLoader = ruleLoader;
     }
 
+    public void setRuleSetLoader(RuleSetLoader ruleSetLoader)
+    {
+        this.ruleSetLoader = ruleSetLoader;
+    }
+
     public void setActionPermissionValidator(ActionPermissionValidator actionPermissionValidator)
     {
         this.actionPermissionValidator = actionPermissionValidator;

remote-api/src/main/resources/alfresco/public-rest-context.xml

@@ -929,6 +929,7 @@
         <property name="validator" ref="nodeValidator"/>
         <property name="ruleService" ref="RuleService" />
         <property name="ruleLoader" ref="ruleLoader"/>
+        <property name="ruleSetLoader" ref="ruleSetLoader"/>
         <property name="actionPermissionValidator" ref="actionPermissionValidator"/>
         <property name="ruleMapper" ref="ruleMapper"/>
     </bean>

remote-api/src/test/java/org/alfresco/rest/api/impl/rules/RulesImplTest.java

@@ -28,6 +28,7 @@ package org.alfresco.rest.api.impl.rules;
 
 import static java.util.Collections.emptyList;
 
+import static org.alfresco.rest.api.impl.rules.RuleSetLoader.INCLUSION_TYPE;
 import static org.alfresco.rest.api.model.rules.RuleSet.DEFAULT_ID;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
@@ -51,8 +52,10 @@ import org.alfresco.repo.action.executer.ExecuteAllRulesActionExecuter;
 import org.alfresco.rest.api.Nodes;
 import org.alfresco.rest.api.model.mapper.RestModelMapper;
 import org.alfresco.rest.api.model.rules.Action;
+import org.alfresco.rest.api.model.rules.InclusionType;
 import org.alfresco.rest.api.model.rules.Rule;
 import org.alfresco.rest.api.model.rules.RuleExecution;
+import org.alfresco.rest.api.model.rules.RuleSet;
 import org.alfresco.rest.framework.core.exceptions.EntityNotFoundException;
 import org.alfresco.rest.framework.core.exceptions.InvalidArgumentException;
 import org.alfresco.rest.framework.core.exceptions.PermissionDeniedException;
@@ -80,6 +83,7 @@ public class RulesImplTest extends TestCase
     private static final String FOLDER_NODE_ID = "dummy-folder-node-id";
     private static final String RULE_SET_ID = "dummy-rule-set-id";
     private static final String RULE_ID = "dummy-rule-id";
+    private static final String RULE_ID_INHERITED = "dummy-rule-id-inherited";
     private static final NodeRef FOLDER_NODE_REF = new NodeRef(StoreRef.STORE_REF_WORKSPACE_SPACESSTORE, FOLDER_NODE_ID);
     private static final NodeRef RULE_SET_NODE_REF = new NodeRef(StoreRef.STORE_REF_WORKSPACE_SPACESSTORE, RULE_SET_ID);
     private static final NodeRef RULE_NODE_REF = new NodeRef(StoreRef.STORE_REF_WORKSPACE_SPACESSTORE, RULE_ID);
@@ -101,15 +105,20 @@ public class RulesImplTest extends TestCase
     @Mock
     private RuleLoader ruleLoaderMock;
     @Mock
+    private RuleSetLoader ruleSetLoaderMock;
+    @Mock
     private ActionPermissionValidator actionPermissionValidatorMock;
     @Mock
     private org.alfresco.service.cmr.rule.Rule serviceRuleMock;
     @Mock
     private Rule ruleMock;
     @Mock
+    private RuleSet ruleSetMock;
+    @Mock
     private Action actionMock;
 
     private org.alfresco.service.cmr.rule.Rule ruleModel = createRule(RULE_ID);
+    private org.alfresco.service.cmr.rule.Rule ruleModelInherited = createRule(RULE_ID_INHERITED);
 
     @InjectMocks
     private RulesImpl rules;
@@ -118,6 +127,9 @@ public class RulesImplTest extends TestCase
     @Override
     public void setUp() throws Exception
     {
+        ruleModel.applyToChildren(true);
+        ruleModelInherited.applyToChildren(true);
+
         given(nodeValidatorMock.validateFolderNode(any(), anyBoolean())).willReturn(FOLDER_NODE_REF);
         given(nodeValidatorMock.validateRuleSetNode(any(), any())).willReturn(RULE_SET_NODE_REF);
         given(nodeValidatorMock.validateRuleNode(any(), any())).willReturn(RULE_NODE_REF);
@@ -126,13 +138,15 @@ public class RulesImplTest extends TestCase
         given(ruleServiceMock.getRules(FOLDER_NODE_REF, false)).willReturn(List.of(ruleModel));
         given(ruleServiceMock.getOwningNodeRef(RULE_SET_NODE_REF)).willReturn(FOLDER_NODE_REF);
 
+        given(ruleSetMock.getInclusionType()).willReturn(InclusionType.INHERITED);
+
         given(ruleLoaderMock.loadRule(ruleModel, INCLUDE)).willReturn(ruleMock);
+        given(ruleSetLoaderMock.loadRuleSet(RULE_SET_NODE_REF, FOLDER_NODE_REF, List.of(INCLUSION_TYPE))).willReturn(ruleSetMock);
     }
 
     @Test
     public void testGetRules()
     {
-        given(ruleLoaderMock.loadRule(ruleModel, emptyList())).willReturn(ruleMock);
 
         // when
         final CollectionWithPagingInfo<Rule> rulesPage = rules.getRules(FOLDER_NODE_ID, RULE_SET_ID, INCLUDE, PAGING);
@@ -141,6 +155,66 @@ public class RulesImplTest extends TestCase
         then(nodeValidatorMock).should().validateRuleSetNode(RULE_SET_ID, FOLDER_NODE_REF);
         then(nodeValidatorMock).shouldHaveNoMoreInteractions();
         then(ruleServiceMock).should().getOwningNodeRef(RULE_SET_NODE_REF);
+        then(ruleSetLoaderMock).should().loadRuleSet(RULE_SET_NODE_REF, FOLDER_NODE_REF, List.of(INCLUSION_TYPE));
+        then(ruleSetLoaderMock).shouldHaveNoMoreInteractions();
+        then(ruleServiceMock).should().getRules(FOLDER_NODE_REF, false);
+        then(ruleServiceMock).shouldHaveNoMoreInteractions();
+        then(ruleLoaderMock).should().loadRule(ruleModel, emptyList());
+        then(ruleLoaderMock).shouldHaveNoMoreInteractions();
+        assertThat(rulesPage)
+                .isNotNull()
+                .extracting(CollectionWithPagingInfo::getCollection)
+                .isNotNull()
+                .extracting(Collection::size)
+                .isEqualTo(1);
+        assertThat(rulesPage.getCollection().stream().findFirst().get()).isEqualTo(ruleMock);
+    }
+
+    @Test
+    public void testGetRules_ruleNotAppliedToChildren()
+    {
+        given(ruleSetMock.getInclusionType()).willReturn(InclusionType.INHERITED);
+        given(ruleServiceMock.getRules(FOLDER_NODE_REF, false)).willReturn(List.of(ruleModel, ruleModelInherited));
+        ruleModelInherited.applyToChildren(false);
+
+        // when
+        final CollectionWithPagingInfo<Rule> rulesPage = rules.getRules(FOLDER_NODE_ID, RULE_SET_ID, INCLUDE, PAGING);
+
+        then(nodeValidatorMock).should().validateFolderNode(FOLDER_NODE_ID, false);
+        then(nodeValidatorMock).should().validateRuleSetNode(RULE_SET_ID, FOLDER_NODE_REF);
+        then(nodeValidatorMock).shouldHaveNoMoreInteractions();
+        then(ruleServiceMock).should().getOwningNodeRef(RULE_SET_NODE_REF);
+        then(ruleSetLoaderMock).should().loadRuleSet(RULE_SET_NODE_REF, FOLDER_NODE_REF, List.of(INCLUSION_TYPE));
+        then(ruleSetLoaderMock).shouldHaveNoMoreInteractions();
+        then(ruleServiceMock).should().getRules(FOLDER_NODE_REF, false);
+        then(ruleServiceMock).shouldHaveNoMoreInteractions();
+        then(ruleLoaderMock).should().loadRule(ruleModel, emptyList());
+        then(ruleLoaderMock).shouldHaveNoMoreInteractions();
+        assertThat(rulesPage)
+                .isNotNull()
+                .extracting(CollectionWithPagingInfo::getCollection)
+                .isNotNull()
+                .extracting(Collection::size)
+                .isEqualTo(1);
+        assertThat(rulesPage.getCollection().stream().findFirst().get()).isEqualTo(ruleMock);
+    }
+
+    @Test
+    public void testGetRules_inheritedRuleSet()
+    {
+        given(ruleSetMock.getInclusionType()).willReturn(InclusionType.INHERITED);
+        ruleModelInherited.applyToChildren(false);
+        given(ruleServiceMock.getRules(FOLDER_NODE_REF, false)).willReturn(List.of(ruleModel, ruleModelInherited));
+
+        // when
+        final CollectionWithPagingInfo<Rule> rulesPage = rules.getRules(FOLDER_NODE_ID, RULE_SET_ID, INCLUDE, PAGING);
+
+        then(nodeValidatorMock).should().validateFolderNode(FOLDER_NODE_ID, false);
+        then(nodeValidatorMock).should().validateRuleSetNode(RULE_SET_ID, FOLDER_NODE_REF);
+        then(nodeValidatorMock).shouldHaveNoMoreInteractions();
+        then(ruleServiceMock).should().getOwningNodeRef(RULE_SET_NODE_REF);
+        then(ruleSetLoaderMock).should().loadRuleSet(RULE_SET_NODE_REF, FOLDER_NODE_REF, List.of(INCLUSION_TYPE));
+        then(ruleSetLoaderMock).shouldHaveNoMoreInteractions();
         then(ruleServiceMock).should().getRules(FOLDER_NODE_REF, false);
         then(ruleServiceMock).shouldHaveNoMoreInteractions();
         then(ruleLoaderMock).should().loadRule(ruleModel, emptyList());
@@ -163,6 +237,8 @@ public class RulesImplTest extends TestCase
         final CollectionWithPagingInfo<Rule> rulesPage = rules.getRules(FOLDER_NODE_ID, RULE_SET_ID, INCLUDE, PAGING);
 
         then(ruleServiceMock).should().getOwningNodeRef(RULE_SET_NODE_REF);
+        then(ruleSetLoaderMock).should().loadRuleSet(RULE_SET_NODE_REF, FOLDER_NODE_REF, List.of(INCLUSION_TYPE));
+        then(ruleSetLoaderMock).shouldHaveNoMoreInteractions();
         then(ruleServiceMock).should().getRules(FOLDER_NODE_REF, false);
         then(ruleServiceMock).shouldHaveNoMoreInteractions();
         assertThat(rulesPage)