[MNT-24282] Obtain site manager authority as system and keep site manager permissions when inheritance flag is disabled (#2914)

* [MNT-24282] Obtain site manager authority as system

* [MNT-24282] Add unit test

* [MNT-24282] Do not remove site manager permissions from local set when inheritance flag is disabled

* [MNT-24282] PMD scan changes

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

@@ -2401,6 +2401,8 @@ public class NodesImpl implements Nodes
         NodePermissions nodePerms = nodeInfo.getPermissions();
         if (nodePerms != null)
         {
+            String siteManagerAuthority = getSiteManagerAuthority(nodeRef);
+
             // Cannot set inherited permissions, only direct (locally set) permissions can be set
             if ((nodePerms.getInherited() != null) && (nodePerms.getInherited().size() > 0))
             {
@@ -2411,7 +2413,7 @@ public class NodesImpl implements Nodes
             if (nodePerms.getIsInheritanceEnabled() != null)
             {
                 // If inheritance flag is being disabled, the site manager needs to have permission
-                setSiteManagerPermission(nodeRef, nodePerms);
+                setSiteManagerPermission(nodeRef, nodePerms, siteManagerAuthority);
 
                 if (nodePerms.getIsInheritanceEnabled() != permissionService.getInheritParentPermissions(nodeRef))
                 {
@@ -2510,8 +2512,19 @@ public class NodesImpl implements Nodes
                 }
 
                 // remove any remaining direct perms
+                boolean isInheritanceEnabled = permissionService.getInheritParentPermissions(nodeRef);
                 for (AccessPermission accessPerm : directPerms)
                 {
+                    // prevents deletion of site manager permissions when inheritance is disabled
+                    boolean isSiteManagerAuthority = siteManagerAuthority != null && siteManagerAuthority.equals(accessPerm.getAuthority());
+                    if (!isInheritanceEnabled && isSiteManagerAuthority)
+                    {
+                        if (logger.isDebugEnabled())
+                        {
+                            logger.debug("Site manager permissions will be kept since inheritance flag is disabled: " + nodeRef.getId());
+                        }
+                        continue;
+                    }
                     permissionService.deletePermission(nodeRef, accessPerm.getAuthority(), accessPerm.getPermission());
                 }
             }
@@ -2776,25 +2789,29 @@ public class NodesImpl implements Nodes
         return updateExistingFile(null, nodeRef, fileName, contentInfo, stream, parameters, versionMajor, versionComment);
     }
 
-    private void setSiteManagerPermission(NodeRef nodeRef, NodePermissions nodePerms)
+    private String getSiteManagerAuthority(NodeRef nodeRef) {
+        return AuthenticationUtil.runAsSystem(() -> {
+            SiteInfo containingSite = siteService.getSite(nodeRef);
+
+            if (containingSite != null)
+            {
+                String thisSiteGroupPrefix = siteService.getSiteGroup(containingSite.getShortName());
+                return thisSiteGroupPrefix + "_" + SiteModel.SITE_MANAGER;
+            }
+
+            return null;
+        });
+    }
+
+    private void setSiteManagerPermission(NodeRef nodeRef, NodePermissions nodePerms, String siteManagerAuthority)
     {
-        if (nodeRef != null && nodePerms != null)
+        if (nodeRef != null && nodePerms != null && nodePerms.getIsInheritanceEnabled() != null && siteManagerAuthority != null)
         {
             try
             {
-                if (nodePerms.getIsInheritanceEnabled() != null && !nodePerms.getIsInheritanceEnabled())
+                if (!nodePerms.getIsInheritanceEnabled())
                 {
-                    SiteInfo containingSite = siteService.getSite(nodeRef);
+                    permissionService.setPermission(nodeRef, siteManagerAuthority, SiteModel.SITE_MANAGER, true);
-
-                    if (containingSite != null)
-                    {
-                        String thisSiteGroupPrefix = siteService.getSiteGroup(containingSite.getShortName());
-                        final String siteManagerAuthority = thisSiteGroupPrefix + "_" + SiteModel.SITE_MANAGER;
-                        AuthenticationUtil.runAsSystem(() -> {
-                            permissionService.setPermission(nodeRef, siteManagerAuthority, SiteModel.SITE_MANAGER, true);
-                            return null;
-                        });
-                    }
                 }
             }
             catch (Exception e)

remote-api/src/test/java/org/alfresco/rest/api/tests/NodeApiTest.java

@@ -35,6 +35,7 @@ import static org.junit.Assert.assertTrue;
 
 import java.io.ByteArrayInputStream;
 import java.io.File;
+import java.io.IOException;
 import java.io.RandomAccessFile;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -62,6 +63,7 @@ import org.alfresco.rest.api.model.ClassDefinition;
 import org.alfresco.rest.api.model.ConstraintDefinition;
 import org.alfresco.rest.api.model.LockInfo;
 import org.alfresco.rest.api.model.NodePermissions;
+import org.alfresco.rest.api.model.NodePermissions.NodePermission;
 import org.alfresco.rest.api.model.NodeTarget;
 import org.alfresco.rest.api.model.PropertyDefinition;
 import org.alfresco.rest.api.model.Site;
@@ -6368,7 +6370,7 @@ public class NodeApiTest extends AbstractSingleNetworkSiteTest
      * Tests if site manager permissions are kept after inheritance flag is disabled
      */
     @Test
-    public void testSiteManagerPermission() throws Exception
+    public void testSiteManagerPermission_MNT23379() throws Exception
     {
         // Change to User1 context
         setRequestContext(user1);
@@ -6394,6 +6396,10 @@ public class NodeApiTest extends AbstractSingleNetworkSiteTest
         Node nodeUpdate = new Node();
         NodePermissions nodePerms = new NodePermissions();
         nodePerms.setIsInheritanceEnabled(false);
+        NodePermission permission = new NodePermission("GROUP_site_" + site1Id + "_SiteConsumer", SiteRole.SiteConsumer.toString(), AccessStatus.ALLOWED.toString());
+        List<NodePermission> locallySet = new ArrayList<>();
+        locallySet.add(permission);
+        nodePerms.setLocallySet(locallySet);
         nodeUpdate.setPermissions(nodePerms);
         put(URL_NODES, content1_Id, toJsonAsStringNonNull(nodeUpdate), null, 200);
 
@@ -6409,6 +6415,76 @@ public class NodeApiTest extends AbstractSingleNetworkSiteTest
         deleteSite(site1Id, true, 204);
     }
 
+    /**
+     * Test if inheritance flag is correctly set on a node placed inside a site
+     */
+    @Test
+    public void testSiteNodeInheritance_MNT24282() throws Exception
+    {
+        // Change to User1 context
+        setRequestContext(user1);
+
+        // user1 creates a site
+        String site1Title = "site-testSiteInheritanceFlag_DocLib-" + RUNID;
+        String site1Id = createSite(site1Title, SiteVisibility.PUBLIC).getId();
+        String site1DocLibNodeId = getSiteContainerNodeId(site1Id, "documentLibrary");
+
+        // user1 adds user2 as a site consumer to the created site
+        addSiteMember(site1Id, user2, SiteRole.SiteConsumer);
+
+        // user1 creates folder hierarchy: folderA/folderB/folderC
+        Folder folderA = createFolder(site1DocLibNodeId, "folderA");
+        Folder folderB = createFolder(folderA.getId(), "folderB");
+        Folder folderC = createFolder(folderB.getId(), "folderC");
+
+        // user1 disables inheritance flag on folder B
+        disableSiteNodeInheritanceAsShare(site1Id, folderB.getId());
+
+        // user1 disables inheritance flag on folder C
+        disableSiteNodeInheritanceAsShare(site1Id, folderC.getId());
+
+        // user1 gives SiteManager permissions to user2 in folderC
+        Node nodeC = new Node();
+        NodePermissions permsC = new NodePermissions();
+        List<NodePermission> locallySet = new ArrayList<>();
+        locallySet.add(new NodePermission(user2, SiteRole.SiteManager.toString(), AccessStatus.ALLOWED.toString()));
+        permsC.setLocallySet(locallySet);
+        nodeC.setPermissions(permsC);
+        put(URL_NODES, folderC.getId(), toJsonAsStringNonNull(nodeC), null, 200);
+
+        // Change to User2 context
+        setRequestContext(user2);
+
+        // user2 creates folderD inside folderC
+        Folder folderD = createFolder(folderC.getId(), "folderD");
+
+        // user2 disables inheritance flag on folder D
+        disableSiteNodeInheritanceAsShare(site1Id, folderD.getId());
+
+        // user2 enables inheritance flag on folder D
+        setSiteNodeInheritance(site1Id, folderD.getId(), true, false);
+
+        // user2 disables inheritance flag on folder D
+        disableSiteNodeInheritanceAsShare(site1Id, folderD.getId());
+
+        // user2 should be able to re-enable and disable the inheritance flag without causing a permission denied error
+        setSiteNodeInheritance(site1Id, folderD.getId(), true, false);
+        setSiteNodeInheritance(site1Id, folderD.getId(), false, false);
+
+        // user2 checks if has access to the folder
+        Map<String, String> params = new HashMap<>();
+        params.put("include", "permissions");
+        HttpResponse response = getSingle(NodesEntityResource.class, folderD.getId(), params, 200);
+        Document node = jacksonUtil.parseEntry(response.getJsonResponse(), Document.class);
+        assertNotNull(node);
+        assertEquals(node.getId(), folderD.getId());
+        assertFalse("Folder D inheritance flag should be false", node.getPermissions().getIsInheritanceEnabled());
+
+        // cleanup
+        setRequestContext(user1);
+        deleteSite(site1Id, true, 204);
+    }
+
     @Test
     public void testRuleAspectAfterUpdate() throws Exception
     {
@@ -6445,5 +6521,28 @@ public class NodeApiTest extends AbstractSingleNetworkSiteTest
         // Cleanup
         delete(URL_NODES, folderId, 204);
     }
+
+    private void disableSiteNodeInheritanceAsShare(String siteId, String nodeId) throws IOException
+    {
+        setSiteNodeInheritance(siteId, nodeId, false, true);
+    }
+
+    private void setSiteNodeInheritance(String siteId, String nodeId, boolean inheritanceFlag, boolean addSiteManager) throws IOException
+    {
+        Node node = new Node();
+        NodePermissions perms = new NodePermissions();
+        perms.setIsInheritanceEnabled(inheritanceFlag);
+        if (!inheritanceFlag && addSiteManager) {
+            List<NodePermission> locallySet = new ArrayList<>();
+            locallySet.add(new NodePermission("GROUP_site_" + siteId + "_SiteManager", SiteRole.SiteManager.toString(), AccessStatus.ALLOWED.toString()));
+            perms.setLocallySet(locallySet);
+        }
+        node.setPermissions(perms);
+        try {
+            put(URL_NODES, nodeId, toJsonAsStringNonNull(node), null, 200);
+        } catch (Exception e) {
+            throw new IOException("Failed to update permissions: " + e.getMessage(), e);
+        }
+    }
 }