ACS-3346 Require CHANGE permission for POST/DELETE rules. (#1251)

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

@@ -27,6 +27,7 @@ package org.alfresco.rest.rules;
 
 import static java.util.stream.Collectors.toList;
 
+import static org.alfresco.utility.constants.UserRole.SiteCollaborator;
 import static org.alfresco.utility.model.FileModel.getRandomFileModel;
 import static org.alfresco.utility.model.FileType.TEXT_PLAIN;
 import static org.alfresco.utility.report.log.Step.STEP;
@@ -145,7 +146,7 @@ public class CreateRulesTests extends RestTest
     }
 
     /** Check that a user without permission to view the folder cannot create a rule in it. */
-    public void requirePermissionToCreateRule()
+    public void requireReadPermissionToCreateRule()
     {
         STEP("Create a user and use them to create a private site containing a folder");
         UserModel privateUser = dataUser.createRandomTestUser();
@@ -159,7 +160,27 @@ public class CreateRulesTests extends RestTest
         restClient.authenticateUser(user).withCoreAPI().usingNode(privateFolder).usingDefaultRuleSet().createSingleRule(ruleModel);
 
         restClient.assertStatusCodeIs(FORBIDDEN);
-        restClient.assertLastError().containsSummary("Cannot read from this node");
+        restClient.assertLastError().containsSummary("Insufficient permissions to manage rules");
+    }
+
+    /** Check that a user without write permission for the folder cannot create a rule in it. */
+    public void requireWritePermissionToCreateRule()
+    {
+        STEP("Create a user and use them to create a private site containing a folder");
+        UserModel privateUser = dataUser.createRandomTestUser();
+        SiteModel privateSite = dataSite.usingUser(privateUser).createPrivateRandomSite();
+        FolderModel privateFolder = dataContent.usingUser(privateUser).usingSite(privateSite).createFolder();
+
+        STEP("Create a collaborator and check they cannot create a rule in the private folder");
+        UserModel collaborator = dataUser.createRandomTestUser();
+        dataUser.addUserToSite(collaborator, privateSite, SiteCollaborator);
+        RestRuleModel ruleModel = new RestRuleModel();
+        ruleModel.setName("ruleName");
+
+        restClient.authenticateUser(collaborator).withCoreAPI().usingNode(privateFolder).usingDefaultRuleSet().createSingleRule(ruleModel);
+
+        restClient.assertStatusCodeIs(FORBIDDEN);
+        restClient.assertLastError().containsSummary("Insufficient permissions to manage rules");
     }
 
     /** Check we can't create a rule under a document node. */

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

@@ -26,6 +26,9 @@
 
 package org.alfresco.rest.api.impl;
 
+import static org.alfresco.service.cmr.security.AccessStatus.ALLOWED;
+import static org.alfresco.service.cmr.security.PermissionService.CHANGE_PERMISSIONS;
+
 import java.util.List;
 import java.util.Set;
 import java.util.stream.Collectors;
@@ -44,7 +47,6 @@ import org.alfresco.rest.framework.resource.parameters.Paging;
 import org.alfresco.service.Experimental;
 import org.alfresco.service.cmr.repository.NodeRef;
 import org.alfresco.service.cmr.rule.RuleService;
-import org.alfresco.service.cmr.security.AccessStatus;
 import org.alfresco.service.cmr.security.PermissionService;
 import org.alfresco.service.namespace.QName;
 
@@ -62,7 +64,7 @@ public class RulesImpl implements Rules
     @Override
     public CollectionWithPagingInfo<Rule> getRules(final String folderNodeId, final String ruleSetId, final Paging paging)
     {
-        final NodeRef folderNodeRef = validateFolderNode(folderNodeId);
+        final NodeRef folderNodeRef = validateFolderNode(folderNodeId, false);
         validateRuleSetNode(ruleSetId, folderNodeRef);
 
         final List<Rule> rules = ruleService.getRules(folderNodeRef).stream()
@@ -75,7 +77,7 @@ public class RulesImpl implements Rules
     @Override
     public Rule getRuleById(final String folderNodeId, final String ruleSetId, final String ruleId)
     {
-        final NodeRef folderNodeRef = validateFolderNode(folderNodeId);
+        final NodeRef folderNodeRef = validateFolderNode(folderNodeId, false);
         final NodeRef ruleSetNodeRef = validateRuleSetNode(ruleSetId, folderNodeRef);
         final NodeRef ruleNodeRef = validateRuleNode(ruleId, ruleSetNodeRef);
 
@@ -85,7 +87,7 @@ public class RulesImpl implements Rules
     @Override
     public List<Rule> createRules(final String folderNodeId, final String ruleSetId, final List<Rule> rules)
     {
-        final NodeRef folderNodeRef = validateFolderNode(folderNodeId);
+        final NodeRef folderNodeRef = validateFolderNode(folderNodeId, true);
         // Don't validate the ruleset node if -default- is passed since we may need to create it.
         if (RuleSet.isNotDefaultId(ruleSetId))
         {
@@ -102,7 +104,7 @@ public class RulesImpl implements Rules
     @Override
     public void deleteRuleById(String folderNodeId, String ruleSetId, String ruleId)
     {
-        final NodeRef folderNodeRef = validateFolderNode(folderNodeId);
+        final NodeRef folderNodeRef = validateFolderNode(folderNodeId, true);
         final NodeRef ruleSetNodeRef = validateRuleSetNode(ruleSetId, folderNodeRef);
         final NodeRef ruleNodeRef = validateRuleNode(ruleId, ruleSetNodeRef);
         final org.alfresco.service.cmr.rule.Rule rule = ruleService.getRule(ruleNodeRef);
@@ -125,20 +127,31 @@ public class RulesImpl implements Rules
     }
 
     /**
-     * Validates if folder node exists and user have permission to read from it.
+     * Validates if folder node exists and the user has permission to use it.
      *
      * @param folderNodeId - folder node ID
+     * @param requireChangePermission - Whether to require change permission or just read permission.
      * @return folder node reference
      * @throws InvalidArgumentException if node is not of an expected type
-     * @throws PermissionDeniedException if user doesn't have right to read from folder
+     * @throws PermissionDeniedException if the user doesn't have the appropriate permission for the folder.
      */
-    private NodeRef validateFolderNode(final String folderNodeId)
+    private NodeRef validateFolderNode(final String folderNodeId, boolean requireChangePermission)
     {
         final NodeRef nodeRef = nodes.validateOrLookupNode(folderNodeId, null);
-        if (permissionService.hasReadPermission(nodeRef) != AccessStatus.ALLOWED) {
+        if (requireChangePermission)
+        {
+            if (permissionService.hasPermission(nodeRef, CHANGE_PERMISSIONS) != ALLOWED)
+            {
+                throw new PermissionDeniedException("Insufficient permissions to manage rules.");
+            }
+        }
+        else
+        {
+            if (permissionService.hasReadPermission(nodeRef) != ALLOWED)
+            {
                 throw new PermissionDeniedException("Cannot read from this node!");
             }
-
+        }
         verifyNodeType(nodeRef, ContentModel.TYPE_FOLDER, null);
 
         return nodeRef;

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

@@ -29,6 +29,9 @@ package org.alfresco.rest.api.impl;
 import static java.util.Collections.emptyList;
 
 import static org.alfresco.rest.api.model.rules.RuleSet.DEFAULT_ID;
+import static org.alfresco.service.cmr.security.AccessStatus.ALLOWED;
+import static org.alfresco.service.cmr.security.AccessStatus.DENIED;
+import static org.alfresco.service.cmr.security.PermissionService.CHANGE_PERMISSIONS;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 import static org.mockito.ArgumentMatchers.any;
@@ -54,7 +57,6 @@ import org.alfresco.service.Experimental;
 import org.alfresco.service.cmr.repository.NodeRef;
 import org.alfresco.service.cmr.repository.StoreRef;
 import org.alfresco.service.cmr.rule.RuleService;
-import org.alfresco.service.cmr.security.AccessStatus;
 import org.alfresco.service.cmr.security.PermissionService;
 import org.junit.Before;
 import org.junit.Test;
@@ -98,7 +100,8 @@ public class RulesImplTest extends TestCase
         given(nodesMock.validateOrLookupNode(eq(FOLDER_NODE_ID), any())).willReturn(folderNodeRef);
         given(nodesMock.validateNode(RULE_SET_ID)).willReturn(ruleSetNodeRef);
         given(nodesMock.nodeMatches(any(), any(), any())).willReturn(true);
-        given(permissionServiceMock.hasReadPermission(any())).willReturn(AccessStatus.ALLOWED);
+        given(permissionServiceMock.hasReadPermission(any())).willReturn(ALLOWED);
+        given(permissionServiceMock.hasPermission(any(), any())).willReturn(ALLOWED);
     }
 
     @Test
@@ -201,7 +204,7 @@ public class RulesImplTest extends TestCase
     @Test
     public void testGetRulesWithoutReadPermission()
     {
-        given(permissionServiceMock.hasReadPermission(any())).willReturn(AccessStatus.DENIED);
+        given(permissionServiceMock.hasReadPermission(any())).willReturn(DENIED);
 
         // when
         assertThatExceptionOfType(PermissionDeniedException.class).isThrownBy(
@@ -392,6 +395,22 @@ public class RulesImplTest extends TestCase
         assertThat(actual).isEqualTo(expected);
     }
 
+    /** Try to create a rule without CHANGE permission and check an exception is thrown. */
+    @Test
+    public void testSaveRules_noChangePermission()
+    {
+        given(permissionServiceMock.hasPermission(folderNodeRef, CHANGE_PERMISSIONS)).willReturn(DENIED);
+
+        Rule ruleBody = mock(Rule.class);
+        List<Rule> ruleList = List.of(ruleBody);
+
+        // when
+        assertThatExceptionOfType(PermissionDeniedException.class).isThrownBy(() ->
+                rules.createRules(folderNodeRef.getId(), ruleSetNodeRef.getId(), ruleList));
+
+        then(ruleServiceMock).shouldHaveNoMoreInteractions();
+    }
+
     @Test
     public void testDeleteRuleById() {
         given(nodesMock.validateNode(RULE_SET_ID)).willReturn(ruleSetNodeRef);
@@ -411,7 +430,7 @@ public class RulesImplTest extends TestCase
         then(nodesMock).should().nodeMatches(eq(ruleSetNodeRef), any(), isNull());
         then(nodesMock).should().nodeMatches(eq(ruleNodeRef), any(), isNull());
         then(nodesMock).shouldHaveNoMoreInteractions();
-        then(permissionServiceMock).should().hasReadPermission(folderNodeRef);
+        then(permissionServiceMock).should().hasPermission(folderNodeRef, CHANGE_PERMISSIONS);
         then(permissionServiceMock).shouldHaveNoMoreInteractions();
         then(ruleServiceMock).should().isRuleSetAssociatedWithFolder(ruleSetNodeRef, folderNodeRef);
         then(ruleServiceMock).should().isRuleAssociatedWithRuleSet(ruleNodeRef, ruleSetNodeRef);
@@ -436,7 +455,7 @@ public class RulesImplTest extends TestCase
         then(nodesMock).should().nodeMatches(eq(folderNodeRef), any(), isNull());
         then(nodesMock).should().nodeMatches(eq(ruleSetNodeRef), any(), isNull());
         then(nodesMock).shouldHaveNoMoreInteractions();
-        then(permissionServiceMock).should().hasReadPermission(folderNodeRef);
+        then(permissionServiceMock).should().hasPermission(folderNodeRef, CHANGE_PERMISSIONS);
         then(permissionServiceMock).shouldHaveNoMoreInteractions();
         then(ruleServiceMock).should().isRuleSetAssociatedWithFolder(ruleSetNodeRef, folderNodeRef);
         then(ruleServiceMock).shouldHaveNoMoreInteractions();
@@ -460,7 +479,7 @@ public class RulesImplTest extends TestCase
         then(nodesMock).should().nodeMatches(eq(ruleSetNodeRef), any(), isNull());
         then(nodesMock).should().nodeMatches(eq(ruleNodeRef), any(), isNull());
         then(nodesMock).shouldHaveNoMoreInteractions();
-        then(permissionServiceMock).should().hasReadPermission(folderNodeRef);
+        then(permissionServiceMock).should().hasPermission(folderNodeRef, CHANGE_PERMISSIONS);
         then(permissionServiceMock).shouldHaveNoMoreInteractions();
         then(ruleServiceMock).should().isRuleSetAssociatedWithFolder(ruleSetNodeRef, folderNodeRef);
         then(ruleServiceMock).should().isRuleAssociatedWithRuleSet(ruleNodeRef, ruleSetNodeRef);
@@ -479,7 +498,7 @@ public class RulesImplTest extends TestCase
         then(nodesMock).should().validateNode(RULE_SET_ID);
         then(nodesMock).should().nodeMatches(eq(folderNodeRef), any(), isNull());
         then(nodesMock).shouldHaveNoMoreInteractions();
-        then(permissionServiceMock).should().hasReadPermission(folderNodeRef);
+        then(permissionServiceMock).should().hasPermission(folderNodeRef, CHANGE_PERMISSIONS);
         then(permissionServiceMock).shouldHaveNoMoreInteractions();
         then(ruleServiceMock).shouldHaveNoMoreInteractions();
     }
@@ -498,7 +517,7 @@ public class RulesImplTest extends TestCase
         then(nodesMock).should().nodeMatches(eq(folderNodeRef), any(), isNull());
         then(nodesMock).should().nodeMatches(eq(ruleSetNodeRef), any(), isNull());
         then(nodesMock).shouldHaveNoMoreInteractions();
-        then(permissionServiceMock).should().hasReadPermission(folderNodeRef);
+        then(permissionServiceMock).should().hasPermission(folderNodeRef, CHANGE_PERMISSIONS);
         then(permissionServiceMock).shouldHaveNoMoreInteractions();
         then(ruleServiceMock).should().isRuleSetAssociatedWithFolder(ruleSetNodeRef, folderNodeRef);
         then(ruleServiceMock).shouldHaveNoMoreInteractions();
@@ -518,6 +537,20 @@ public class RulesImplTest extends TestCase
         then(ruleServiceMock).shouldHaveNoMoreInteractions();
     }
 
+    /** Try to delete a rule without CHANGE permission and check an exception is thrown. */
+    @Test
+    public void testDeleteRuleById_noChangePermission()
+    {
+        given(permissionServiceMock.hasPermission(folderNodeRef, CHANGE_PERMISSIONS)).willReturn(DENIED);
+
+        // when
+        assertThatExceptionOfType(PermissionDeniedException.class).isThrownBy(() ->
+                rules.deleteRuleById(folderNodeRef.getId(), ruleSetNodeRef.getId(), RULE_ID));
+
+        then(ruleServiceMock).shouldHaveNoMoreInteractions();
+    }
+
+
     private static org.alfresco.service.cmr.rule.Rule createRule(final String id) {
         final org.alfresco.service.cmr.rule.Rule rule = new org.alfresco.service.cmr.rule.Rule();
         rule.setNodeRef(new NodeRef(StoreRef.STORE_REF_WORKSPACE_SPACESSTORE, id));