Fix pagination for users restricted by start nodes (#18907)

* Fix pagination for users restricted by start nodes

* Default implementation to avoid breakage

* Review comments

* Fix failing test

* Add media start node tests

Author: kjac

src/Umbraco.Cms.Api.Management/Controllers/Tree/UserStartNodeTreeControllerBase.cs

@@ -42,11 +42,21 @@ protected override IEntitySlim[] GetPagedRootEntities(int skip, int take, out lo
 
     protected override IEntitySlim[] GetPagedChildEntities(Guid parentKey, int skip, int take, out long totalItems)
     {
-        IEntitySlim[] children = base.GetPagedChildEntities(parentKey, skip, take, out totalItems);
-        return UserHasRootAccess() || IgnoreUserStartNodes()
-            ? children
-            // Keeping the correct totalItems amount from GetPagedChildEntities
-            : CalculateAccessMap(() => _userStartNodeEntitiesService.ChildUserAccessEntities(children, UserStartNodePaths), out _);
+        if (UserHasRootAccess() || IgnoreUserStartNodes())
+        {
+            return base.GetPagedChildEntities(parentKey, skip, take, out totalItems);
+        }
+
+        IEnumerable<UserAccessEntity> userAccessEntities = _userStartNodeEntitiesService.ChildUserAccessEntities(
+            ItemObjectType,
+            UserStartNodePaths,
+            parentKey,
+            skip,
+            take,
+            ItemOrdering,
+            out totalItems);
+
+        return CalculateAccessMap(() => userAccessEntities, out _);
     }
 
     protected override TItem[] MapTreeItemViewModels(Guid? parentKey, IEntitySlim[] entities)

src/Umbraco.Cms.Api.Management/Services/Entities/IUserStartNodeEntitiesService.cs

@@ -16,9 +16,41 @@ public interface IUserStartNodeEntitiesService
     /// <remarks>
     /// The returned entities may include entities that outside of the user start node scope, but are needed to
     /// for browsing to the actual user start nodes. These entities will be marked as "no access" entities.
+    ///
+    /// This method does not support pagination, because it must load all entities explicitly in order to calculate
+    /// the correct result, given that user start nodes can be descendants of root nodes. Consumers need to apply
+    /// pagination to the result if applicable.
     /// </remarks>
     IEnumerable<UserAccessEntity> RootUserAccessEntities(UmbracoObjectTypes umbracoObjectType, int[] userStartNodeIds);
 
+    /// <summary>
+    /// Calculates the applicable child entities for a given object type for users without root access.
+    /// </summary>
+    /// <param name="umbracoObjectType">The object type.</param>
+    /// <param name="userStartNodePaths">The calculated start node paths for the user.</param>
+    /// <param name="parentKey">The key of the parent.</param>
+    /// <param name="skip">The number of applicable children to skip.</param>
+    /// <param name="take">The number of applicable children to take.</param>
+    /// <param name="ordering">The ordering to apply when fetching and paginating the children.</param>
+    /// <param name="totalItems">The total number of applicable children available.</param>
+    /// <returns>A list of child entities applicable for the user.</returns>
+    /// <remarks>
+    /// The returned entities may include entities that outside of the user start node scope, but are needed to
+    /// for browsing to the actual user start nodes. These entities will be marked as "no access" entities.
+    /// </remarks>
+    IEnumerable<UserAccessEntity> ChildUserAccessEntities(
+        UmbracoObjectTypes umbracoObjectType,
+        string[] userStartNodePaths,
+        Guid parentKey,
+        int skip,
+        int take,
+        Ordering ordering,
+        out long totalItems)
+    {
+        totalItems = 0;
+        return [];
+    }
+
     /// <summary>
     /// Calculates the applicable child entities from a list of candidate child entities for users without root access.
     /// </summary>

src/Umbraco.Cms.Api.Management/Services/Entities/UserStartNodeEntitiesService.cs

@@ -1,17 +1,37 @@
-using Umbraco.Cms.Core;
+using Microsoft.Extensions.DependencyInjection;
+using Umbraco.Cms.Core;
 using Umbraco.Cms.Core.Models;
 using Umbraco.Cms.Core.Models.Entities;
 using Umbraco.Cms.Core.Services;
 using Umbraco.Cms.Api.Management.Models.Entities;
+using Umbraco.Cms.Core.DependencyInjection;
+using Umbraco.Cms.Core.Persistence.Querying;
+using Umbraco.Cms.Core.Scoping;
 using Umbraco.Extensions;
 
 namespace Umbraco.Cms.Api.Management.Services.Entities;
 
 public class UserStartNodeEntitiesService : IUserStartNodeEntitiesService
 {
     private readonly IEntityService _entityService;
+    private readonly ICoreScopeProvider _scopeProvider;
+    private readonly IIdKeyMap _idKeyMap;
 
-    public UserStartNodeEntitiesService(IEntityService entityService) => _entityService = entityService;
+    [Obsolete("Please use the non-obsolete constructor. Scheduled for removal in V17.")]
+    public UserStartNodeEntitiesService(IEntityService entityService)
+        : this(
+            entityService,
+            StaticServiceProvider.Instance.GetRequiredService<ICoreScopeProvider>(),
+            StaticServiceProvider.Instance.GetRequiredService<IIdKeyMap>())
+    {
+    }
+
+    public UserStartNodeEntitiesService(IEntityService entityService, ICoreScopeProvider scopeProvider, IIdKeyMap idKeyMap)
+    {
+        _entityService = entityService;
+        _scopeProvider = scopeProvider;
+        _idKeyMap = idKeyMap;
+    }
 
     /// <inheritdoc />
     public IEnumerable<UserAccessEntity> RootUserAccessEntities(UmbracoObjectTypes umbracoObjectType, int[] userStartNodeIds)
@@ -43,6 +63,54 @@ public IEnumerable<UserAccessEntity> RootUserAccessEntities(UmbracoObjectTypes u
             .ToArray();
     }
 
+    public IEnumerable<UserAccessEntity> ChildUserAccessEntities(UmbracoObjectTypes umbracoObjectType, string[] userStartNodePaths, Guid parentKey, int skip, int take, Ordering ordering, out long totalItems)
+    {
+        Attempt<int> parentIdAttempt = _idKeyMap.GetIdForKey(parentKey, umbracoObjectType);
+        if (parentIdAttempt.Success is false)
+        {
+            totalItems = 0;
+            return [];
+        }
+
+        var parentId = parentIdAttempt.Result;
+        IEntitySlim? parent = _entityService.Get(parentId);
+        if (parent is null)
+        {
+            totalItems = 0;
+            return [];
+        }
+
+        IEntitySlim[] children;
+        if (userStartNodePaths.Any(path => $"{parent.Path},".StartsWith($"{path},")))
+        {
+            // the requested parent is one of the user start nodes (or a descendant of one), all children are by definition allowed
+            children = _entityService.GetPagedChildren(parentKey, umbracoObjectType, skip, take, out totalItems, ordering: ordering).ToArray();
+            return ChildUserAccessEntities(children, userStartNodePaths);
+        }
+
+        // if one or more of the user start nodes are descendants of the requested parent, find the "next child IDs" in those user start node paths
+        // - e.g. given the user start node path "-1,2,3,4,5", if the requested parent ID is 3, the "next child ID" is 4.
+        var userStartNodePathIds = userStartNodePaths.Select(path => path.Split(Constants.CharArrays.Comma).Select(int.Parse).ToArray()).ToArray();
+        var allowedChildIds = userStartNodePathIds
+            .Where(ids => ids.Contains(parentId))
+            // given the previous checks, the parent ID can never be the last in the user start node path, so this is safe
+            .Select(ids => ids[ids.IndexOf(parentId) + 1])
+            .Distinct()
+            .ToArray();
+
+        totalItems = allowedChildIds.Length;
+        if (allowedChildIds.Length == 0)
+        {
+            // the requested parent is outside the scope of any user start nodes
+            return [];
+        }
+
+        // even though we know the IDs of the allowed child entities to fetch, we still use a Query to yield correctly sorted children
+        IQuery<IUmbracoEntity> query = _scopeProvider.CreateQuery<IUmbracoEntity>().Where(x => allowedChildIds.Contains(x.Id));
+        children = _entityService.GetPagedChildren(parentKey, umbracoObjectType, skip, take, out totalItems, query, ordering).ToArray();
+        return ChildUserAccessEntities(children, userStartNodePaths);
+    }
+
     /// <inheritdoc />
     public IEnumerable<UserAccessEntity> ChildUserAccessEntities(IEnumerable<IEntitySlim> candidateChildren, string[] userStartNodePaths)
         // child entities for users without root access should include:

tests/Umbraco.Tests.Integration/ManagementApi/Services/UserStartNodeEntitiesServiceMediaTests.RootUserAccessEntities.cs

@@ -0,0 +1,85 @@
+using NUnit.Framework;
+using Umbraco.Cms.Core.Models;
+
+namespace Umbraco.Cms.Tests.Integration.ManagementApi.Services;
+
+public partial class UserStartNodeEntitiesServiceMediaTests
+{
+    [Test]
+    public async Task RootUserAccessEntities_FirstAndLastRoot_YieldsBoth_AsAllowed()
+    {
+        var contentStartNodeIds = await CreateUserAndGetStartNodeIds(_mediaByName["1"].Id, _mediaByName["5"].Id);
+
+        var roots = UserStartNodeEntitiesService
+            .RootUserAccessEntities(
+                UmbracoObjectTypes.Media,
+                contentStartNodeIds)
+            .ToArray();
+
+        // expected total is 2, because only two items at root ("1" amd "10") are allowed
+        Assert.AreEqual(2, roots.Length);
+        Assert.Multiple(() =>
+        {
+            // first and last content items are the ones allowed
+            Assert.AreEqual(_mediaByName["1"].Key, roots[0].Entity.Key);
+            Assert.AreEqual(_mediaByName["5"].Key, roots[1].Entity.Key);
+
+            // explicitly verify the entity sort order, both so we know sorting works,
+            // and so we know it's actually the first and last item at root
+            Assert.AreEqual(0, roots[0].Entity.SortOrder);
+            Assert.AreEqual(4, roots[1].Entity.SortOrder);
+
+            // both are allowed (they are the actual start nodes)
+            Assert.IsTrue(roots[0].HasAccess);
+            Assert.IsTrue(roots[1].HasAccess);
+        });
+    }
+
+    [Test]
+    public async Task RootUserAccessEntities_ChildrenAsStartNode_YieldsChildRoots_AsNotAllowed()
+    {
+        var contentStartNodeIds = await CreateUserAndGetStartNodeIds(_mediaByName["1-3"].Id, _mediaByName["3-3"].Id, _mediaByName["5-3"].Id);
+
+        var roots = UserStartNodeEntitiesService
+            .RootUserAccessEntities(
+                UmbracoObjectTypes.Media,
+                contentStartNodeIds)
+            .ToArray();
+
+        Assert.AreEqual(3, roots.Length);
+        Assert.Multiple(() =>
+        {
+            // the three start nodes are the children of the "1", "3" and "5" roots, respectively, so these are expected as roots
+            Assert.AreEqual(_mediaByName["1"].Key, roots[0].Entity.Key);
+            Assert.AreEqual(_mediaByName["3"].Key, roots[1].Entity.Key);
+            Assert.AreEqual(_mediaByName["5"].Key, roots[2].Entity.Key);
+
+            // all are disallowed - only the children (the actual start nodes) are allowed
+            Assert.IsTrue(roots.All(r => r.HasAccess is false));
+        });
+    }
+
+    [Test]
+    public async Task RootUserAccessEntities_GrandchildrenAsStartNode_YieldsGrandchildRoots_AsNotAllowed()
+    {
+        var contentStartNodeIds = await CreateUserAndGetStartNodeIds(_mediaByName["1-2-3"].Id, _mediaByName["2-3-4"].Id, _mediaByName["3-4-5"].Id);
+
+        var roots = UserStartNodeEntitiesService
+            .RootUserAccessEntities(
+                UmbracoObjectTypes.Media,
+                contentStartNodeIds)
+            .ToArray();
+
+        Assert.AreEqual(3, roots.Length);
+        Assert.Multiple(() =>
+        {
+            // the three start nodes are the grandchildren of the "1", "2" and "3" roots, respectively, so these are expected as roots
+            Assert.AreEqual(_mediaByName["1"].Key, roots[0].Entity.Key);
+            Assert.AreEqual(_mediaByName["2"].Key, roots[1].Entity.Key);
+            Assert.AreEqual(_mediaByName["3"].Key, roots[2].Entity.Key);
+
+            // all are disallowed - only the grandchildren (the actual start nodes) are allowed
+            Assert.IsTrue(roots.All(r => r.HasAccess is false));
+        });
+    }
+}