Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
255f14e
Introduce new method overloads and repository implentation, such that…
AndyButland Jan 20, 2026
5106da9
Use non-obsolete method overloads throughout.
AndyButland Jan 20, 2026
47abde9
Add unit tests to verify property value retrieval.
AndyButland Jan 20, 2026
9e9acc9
Don't load templates for collection view content retrieval.
AndyButland Jan 21, 2026
1c74f6a
Optimize access checks by verifying the full collection rather than o…
AndyButland Jan 21, 2026
5b414d0
Added obsoletion messages and aligned behaviour of content and media …
AndyButland Jan 21, 2026
bb098f2
Apply suggestions from code review
AndyButland Jan 21, 2026
88e0463
Return key in TreeEntityPath collection response, avoiding a later lo…
AndyButland Jan 21, 2026
38a502d
Merge branch 'main' into v17/improvement/optimize-collection-view-pro…
AndyButland Jan 21, 2026
7d7e0f3
Resolve breaking changes to interfaces.
AndyButland Jan 21, 2026
1680b96
Fix further breaking change.
AndyButland Jan 21, 2026
dcfc548
Merge branch 'main' into v17/improvement/optimize-collection-view-pro…
AndyButland Jan 23, 2026
cb569b3
Additional assert for test verifying property loading for a non-exist…
AndyButland Jan 23, 2026
eb43149
Refactored repositories to avoid having method parameters related to …
AndyButland Jan 23, 2026
5e18dad
Remove check that verifies all provided keys are found when doing per…
AndyButland Jan 23, 2026
6cd8bac
Introduce variable for permission set permissions.
AndyButland Jan 23, 2026
9ce4328
Provide functional default implementation on FilterAuthorizedAsync.
AndyButland Jan 23, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/Umbraco.Core/Models/Entities/TreeEntityPath.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,9 @@ public class TreeEntityPath
/// Gets or sets the path of the entity.
/// </summary>
public string Path { get; set; } = null!;

/// <summary>
/// Gets or sets the unique key of the entity.
/// </summary>
public Guid Key { get; set; }
}
28 changes: 28 additions & 0 deletions src/Umbraco.Core/Persistence/Repositories/IContentRepository.cs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
/// Gets paged content items.
/// </summary>
/// <remarks>Here, <paramref name="filter" /> can be null but <paramref name="ordering" /> cannot.</remarks>
[Obsolete("Please use the method overload with all parameters. Scheduled for removal in Umbraco 19.")]
IEnumerable<TEntity> GetPage(
IQuery<TEntity>? query,
long pageIndex,
Expand All @@ -81,5 +82,32 @@
IQuery<TEntity>? filter,
Ordering? ordering);

/// <summary>
/// Gets paged content items.
/// </summary>
/// <param name="query">The base query for content items.</param>
/// <param name="pageIndex">The page index (zero-based).</param>
/// <param name="pageSize">The number of items per page.</param>
/// <param name="totalRecords">Output parameter with total record count.</param>
/// <param name="propertyAliases">
/// Optional array of property aliases to load. If null, all properties are loaded.
/// If empty array, no custom properties are loaded (only system properties).
/// </param>
/// <param name="filter">Optional filter query.</param>
/// <param name="ordering">The ordering specification.</param>
/// <returns>A collection of content items for the specified page.</returns>
/// <remarks>Here, <paramref name="filter" /> can be null but <paramref name="ordering" /> cannot.</remarks>
#pragma warning disable CS0618 // Type or member is obsolete
IEnumerable<TEntity> GetPage(
IQuery<TEntity>? query,
long pageIndex,
int pageSize,
out long totalRecords,
string[]? propertyAliases,
IQuery<TEntity>? filter,
Ordering? ordering)
=> GetPage(query, pageIndex, pageSize, out totalRecords, filter, ordering);

Check warning on line 109 in src/Umbraco.Core/Persistence/Repositories/IContentRepository.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

❌ New issue: Excess Number of Function Arguments

GetPage has 7 arguments, max arguments = 4. This function has too many arguments, indicating a lack of encapsulation. Avoid adding more arguments.
#pragma warning restore CS0618 // Type or member is obsolete

ContentDataIntegrityReport CheckDataIntegrity(ContentDataIntegrityReportOptions options);
}
32 changes: 32 additions & 0 deletions src/Umbraco.Core/Persistence/Repositories/IDocumentRepository.cs
Original file line number Diff line number Diff line change
@@ -1,11 +1,43 @@
using System.Collections.Immutable;
using Umbraco.Cms.Core.Models;
using Umbraco.Cms.Core.Models.Membership;
using Umbraco.Cms.Core.Persistence.Querying;
using Umbraco.Cms.Core.Services;

namespace Umbraco.Cms.Core.Persistence.Repositories;

public interface IDocumentRepository : IContentRepository<int, IContent>, IReadRepository<Guid, IContent>
{
/// <summary>
/// Gets paged documents.
/// </summary>
/// <param name="query">The base query for documents.</param>
/// <param name="pageIndex">The page index (zero-based).</param>
/// <param name="pageSize">The number of items per page.</param>
/// <param name="totalRecords">Output parameter with total record count.</param>
/// <param name="propertyAliases">
/// Optional array of property aliases to load. If null, all properties are loaded.
/// If empty array, no custom properties are loaded (only system properties).
/// </param>
/// <param name="filter">Optional filter query.</param>
/// <param name="ordering">The ordering specification.</param>
/// <param name="loadTemplates">
/// Whether to load templates. Set to false for performance optimization when templates are not needed
/// (e.g., collection views). Default is true.
/// </param>
/// <returns>A collection of documents for the specified page.</returns>
/// <remarks>Here, <paramref name="filter" /> can be null but <paramref name="ordering" /> cannot.</remarks>
IEnumerable<IContent> GetPage(
IQuery<IContent>? query,
long pageIndex,
int pageSize,
out long totalRecords,
string[]? propertyAliases,
IQuery<IContent>? filter,
Ordering? ordering,
bool loadTemplates)
=> GetPage(query, pageIndex, pageSize, out totalRecords, propertyAliases, filter, ordering);

Check warning on line 39 in src/Umbraco.Core/Persistence/Repositories/IDocumentRepository.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

❌ New issue: Excess Number of Function Arguments

GetPage has 8 arguments, max arguments = 4. This function has too many arguments, indicating a lack of encapsulation. Avoid adding more arguments.

/// <summary>
/// Gets publish/unpublish schedule for a content node.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,4 +73,11 @@ public async Task<bool> IsDeniedForCultures(IUser currentUser, ISet<string> cult
// If we can't find the content item(s) then we can't determine whether you are denied access.
return result is not (ContentAuthorizationStatus.Success or ContentAuthorizationStatus.NotFound);
}

/// <inheritdoc/>
public async Task<ISet<Guid>> FilterAuthorizedAsync(
IUser currentUser,
IEnumerable<Guid> contentKeys,
ISet<string> permissionsToCheck) =>
await _contentPermissionService.FilterAuthorizedAccessAsync(currentUser, contentKeys, permissionsToCheck);
}
Original file line number Diff line number Diff line change
Expand Up @@ -81,4 +81,30 @@ Task<bool> IsDeniedAtRecycleBinLevelAsync(IUser currentUser, string permissionTo
Task<bool> IsDeniedAtRecycleBinLevelAsync(IUser currentUser, ISet<string> permissionsToCheck);

Task<bool> IsDeniedForCultures(IUser currentUser, ISet<string> culturesToCheck);

/// <summary>
/// Filters the specified content keys to only those the user has access to.
/// </summary>
/// <param name="currentUser">The current user.</param>
/// <param name="contentKeys">The keys of the content items to filter.</param>
/// <param name="permissionsToCheck">The collection of permissions to authorize.</param>
/// <returns>Returns the keys of content items the user has access to.</returns>
/// <remarks>
/// The default implementation falls back to calling <see cref="IsDeniedAsync(IUser, IEnumerable{Guid}, ISet{string})"/>
/// for each key individually. Override this method for better performance with batch authorization.
/// </remarks>
// TODO (V18): Remove default implementation.
async Task<ISet<Guid>> FilterAuthorizedAsync(IUser currentUser, IEnumerable<Guid> contentKeys, ISet<string> permissionsToCheck)
{
var authorizedKeys = new HashSet<Guid>();
foreach (Guid key in contentKeys)
{
if (await IsDeniedAsync(currentUser, [key], permissionsToCheck) == false)
{
authorizedKeys.Add(key);
}
}

return authorizedKeys;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,23 @@ Task<bool> IsDeniedAsync(IUser currentUser, Guid mediaKey)
/// <param name="currentUser">The current user.</param>
/// <returns>Returns <c>true</c> if authorization is successful, otherwise <c>false</c>.</returns>
Task<bool> IsDeniedAtRecycleBinLevelAsync(IUser currentUser);

/// <summary>
/// Filters the specified media keys to only those the user has access to.
/// </summary>
/// <param name="currentUser">The current user.</param>
/// <param name="mediaKeys">The keys of the media items to filter.</param>
/// <returns>Returns the keys of media items the user has access to.</returns>
/// <remarks>
/// The default implementation falls back to calling <see cref="IsDeniedAsync(IUser, IEnumerable{Guid})"/>
/// for each key individually. Override this method for better performance with batch authorization.
/// </remarks>
// TODO (V18): Remove default implementation and make this method required.
async Task<ISet<Guid>> FilterAuthorizedAsync(IUser currentUser, IEnumerable<Guid> mediaKeys)
{
var results = await Task.WhenAll(mediaKeys.Select(async key =>
(key, isAuthorized: await IsDeniedAsync(currentUser, [key]) == false)));

return results.Where(r => r.isAuthorized).Select(r => r.key).ToHashSet();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,8 @@ public async Task<bool> IsDeniedAtRecycleBinLevelAsync(IUser currentUser)
// If we can't find the media item(s) then we can't determine whether you are denied access.
return result is not (MediaAuthorizationStatus.Success or MediaAuthorizationStatus.NotFound);
}

/// <inheritdoc/>
public async Task<ISet<Guid>> FilterAuthorizedAsync(IUser currentUser, IEnumerable<Guid> mediaKeys) =>
await _mediaPermissionService.FilterAuthorizedAccessAsync(currentUser, mediaKeys);
}
2 changes: 1 addition & 1 deletion src/Umbraco.Core/Services/ContentEditingService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ protected override IContent New(string? name, int parentId, IContentType content
protected override OperationResult? Delete(IContent content, int userId) => ContentService.Delete(content, userId);

protected override IEnumerable<IContent> GetPagedChildren(int parentId, int pageIndex, int pageSize, out long total)
=> ContentService.GetPagedChildren(parentId, pageIndex, pageSize, out total);
=> ContentService.GetPagedChildren(parentId, pageIndex, pageSize, out total, propertyAliases: null, filter: null, ordering: null);

protected override ContentEditingOperationStatus Sort(IEnumerable<IContent> items, int userId)
{
Expand Down
69 changes: 63 additions & 6 deletions src/Umbraco.Core/Services/ContentPermissionService.cs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
using System.Globalization;
using Umbraco.Cms.Core.Cache;

Check warning on line 1 in src/Umbraco.Core/Services/ContentPermissionService.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

❌ New issue: Overall Code Complexity

This module has a mean cyclomatic complexity of 4.38 across 8 functions. The mean complexity threshold is 4. This file has many conditional statements (e.g. if, for, while) across its implementation, leading to lower code health. Avoid adding more conditionals.
using Umbraco.Cms.Core.Models;
using Umbraco.Cms.Core.Models.Entities;
using Umbraco.Cms.Core.Models.Membership;
using Umbraco.Cms.Core.Security;
using Umbraco.Cms.Core.Services.AuthorizationStatus;
using Umbraco.Extensions;

Expand Down Expand Up @@ -37,19 +37,32 @@
IEnumerable<Guid> contentKeys,
ISet<string> permissionsToCheck)
{
var contentItems = _contentService.GetByIds(contentKeys).ToArray();
Guid[] keysArray = contentKeys.ToArray();

if (contentItems.Length == 0)
if (keysArray.Length == 0)
{
return Task.FromResult(ContentAuthorizationStatus.Success);
}

// Use GetAllPaths instead of loading full content items - we only need paths for authorization
TreeEntityPath[] entityPaths = _entityService.GetAllPaths(UmbracoObjectTypes.Document, keysArray).ToArray();

if (entityPaths.Length == 0)
{
return Task.FromResult(ContentAuthorizationStatus.NotFound);
}

if (contentItems.Any(contentItem => user.HasPathAccess(contentItem, _entityService, _appCaches) == false))
// Check path access using the paths directly
int[]? startNodeIds = user.CalculateContentStartNodeIds(_entityService, _appCaches);
foreach (TreeEntityPath entityPath in entityPaths)
{
return Task.FromResult(ContentAuthorizationStatus.UnauthorizedMissingPathAccess);
if (ContentPermissions.HasPathAccess(entityPath.Path, startNodeIds, Constants.System.RecycleBinContent) == false)
{
return Task.FromResult(ContentAuthorizationStatus.UnauthorizedMissingPathAccess);
}
}

return Task.FromResult(HasPermissionAccess(user, contentItems.Select(c => c.Path), permissionsToCheck)
return Task.FromResult(HasPermissionAccess(user, entityPaths.Select(p => p.Path), permissionsToCheck)
? ContentAuthorizationStatus.Success
: ContentAuthorizationStatus.UnauthorizedMissingPermissionAccess);
}
Expand Down Expand Up @@ -150,6 +163,50 @@
: ContentAuthorizationStatus.UnauthorizedMissingCulture;
}

/// <inheritdoc/>
public Task<ISet<Guid>> FilterAuthorizedAccessAsync(
IUser user,
IEnumerable<Guid> contentKeys,
ISet<string> permissionsToCheck)
{
Guid[] keysArray = [.. contentKeys];

if (keysArray.Length == 0)
{
return Task.FromResult<ISet<Guid>>(new HashSet<Guid>());
}

// Retrieve paths in a single database query for all keys.
TreeEntityPath[] entityPaths = [.. _entityService.GetAllPaths(UmbracoObjectTypes.Document, keysArray)];

if (entityPaths.Length == 0)
{
return Task.FromResult<ISet<Guid>>(new HashSet<Guid>());
}

var authorizedKeys = new HashSet<Guid>();
int[]? startNodeIds = user.CalculateContentStartNodeIds(_entityService, _appCaches);

foreach (TreeEntityPath entityPath in entityPaths)
{
// Check path access
if (ContentPermissions.HasPathAccess(entityPath.Path, startNodeIds, Constants.System.RecycleBinContent) == false)
{
continue;
}

// Check permission access
EntityPermissionSet permissionSet = _userService.GetPermissionsForPath(user, entityPath.Path);
ISet<string> permissionSetPermissions = permissionSet.GetAllPermissions();
if (permissionsToCheck.All(p => permissionSetPermissions.Contains(p)))
{
authorizedKeys.Add(entityPath.Key);
}
}

return Task.FromResult<ISet<Guid>>(authorizedKeys);
}

/// <summary>
/// Check the implicit/inherited permissions of a user for given content items.
/// </summary>
Expand Down
13 changes: 10 additions & 3 deletions src/Umbraco.Core/Services/ContentService.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System.Collections.Immutable;

Check notice on line 1 in src/Umbraco.Core/Services/ContentService.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

✅ Getting better: Code Duplication

reduced similar code in: GetPagedChildren,GetPagedOfType. Avoid duplicated, aka copy-pasted, code inside the module. More duplication lowers the code health.

Check notice on line 1 in src/Umbraco.Core/Services/ContentService.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

✅ Getting better: Overall Code Complexity

The mean cyclomatic complexity decreases from 4.07 to 4.05, threshold = 4. This file has many conditional statements (e.g. if, for, while) across its implementation, leading to lower code health. Avoid adding more conditionals.
using System.Diagnostics.CodeAnalysis;
using System.Runtime.InteropServices;
using Microsoft.Extensions.DependencyInjection;
Expand Down Expand Up @@ -673,6 +673,7 @@
pageIndex,
pageSize,
out totalRecords,
null,
filter,
ordering);
}
Expand Down Expand Up @@ -705,6 +706,7 @@
pageIndex,
pageSize,
out totalRecords,
null,
filter,
ordering);
}
Expand Down Expand Up @@ -840,7 +842,12 @@
}

/// <inheritdoc />
[Obsolete("Please use the method overload with all parameters. Scheduled for removal in Umbraco 19.")]
public IEnumerable<IContent> GetPagedChildren(int id, long pageIndex, int pageSize, out long totalChildren, IQuery<IContent>? filter = null, Ordering? ordering = null)
=> GetPagedChildren(id, pageIndex, pageSize, out totalChildren, propertyAliases: null, filter: filter, ordering: ordering);

/// <inheritdoc />
public IEnumerable<IContent> GetPagedChildren(int id, long pageIndex, int pageSize, out long totalChildren, string[]? propertyAliases, IQuery<IContent>? filter, Ordering? ordering, bool loadTemplates = true)
{
if (pageIndex < 0)
{
Expand All @@ -859,7 +866,7 @@
scope.ReadLock(Constants.Locks.ContentTree);

IQuery<IContent>? query = Query<IContent>()?.Where(x => x.ParentId == id);
return _documentRepository.GetPage(query, pageIndex, pageSize, out totalChildren, filter, ordering);
return _documentRepository.GetPage(query, pageIndex, pageSize, out totalChildren, propertyAliases, filter, ordering, loadTemplates);
}
}

Expand Down Expand Up @@ -918,7 +925,7 @@
throw new ArgumentNullException(nameof(ordering));
}

return _documentRepository.GetPage(query, pageIndex, pageSize, out totalChildren, filter, ordering);
return _documentRepository.GetPage(query, pageIndex, pageSize, out totalChildren, propertyAliases: null, filter, ordering);
}

/// <summary>
Expand Down Expand Up @@ -1009,7 +1016,7 @@
scope.ReadLock(Constants.Locks.ContentTree);
IQuery<IContent>? query = Query<IContent>()?
.Where(x => x.Path.StartsWith(Constants.System.RecycleBinContentPathPrefix));
return _documentRepository.GetPage(query, pageIndex, pageSize, out totalRecords, filter, ordering);
return _documentRepository.GetPage(query, pageIndex, pageSize, out totalRecords, propertyAliases: null, filter, ordering);
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/Umbraco.Core/Services/EntityXmlSerializer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ public XElement Serialize(
while (page * pageSize < total)
{
IEnumerable<IContent> children =
_contentService.GetPagedChildren(content.Id, page++, pageSize, out total);
_contentService.GetPagedChildren(content.Id, page++, pageSize, out total, propertyAliases: null, filter: null, ordering: null);
SerializeChildren(children, xml, published);
}
}
Expand Down Expand Up @@ -678,7 +678,7 @@ private void SerializeChildren(IEnumerable<IContent> children, XElement xml, boo
while (page * pageSize < total)
{
IEnumerable<IContent> grandChildren =
_contentService.GetPagedChildren(child.Id, page++, pageSize, out total);
_contentService.GetPagedChildren(child.Id, page++, pageSize, out total, propertyAliases: null, filter: null, ordering: null);

// recurse
SerializeChildren(grandChildren, childXml, published);
Expand Down
11 changes: 11 additions & 0 deletions src/Umbraco.Core/Services/IContentPermissionService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -88,4 +88,15 @@ Task<ContentAuthorizationStatus> AuthorizeBinAccessAsync(IUser user, string perm
/// <param name="culturesToCheck">The collection of cultures to authorize.</param>
/// <returns>A task resolving into a <see cref="ContentAuthorizationStatus"/>.</returns>
Task<ContentAuthorizationStatus> AuthorizeCultureAccessAsync(IUser user, ISet<string> culturesToCheck);

/// <summary>
/// Filters content keys to only those the user has access to.
/// </summary>
/// <param name="user"><see cref="IUser" /> to authorize.</param>
/// <param name="contentKeys">The identifiers of the content items to filter.</param>
/// <param name="permissionsToCheck">The collection of permissions to authorize.</param>
/// <returns>A task resolving into the set of authorized content keys.</returns>
// TODO (V18): Remove default implementation.
Task<ISet<Guid>> FilterAuthorizedAccessAsync(IUser user, IEnumerable<Guid> contentKeys, ISet<string> permissionsToCheck)
=> Task.FromResult<ISet<Guid>>(new HashSet<Guid>());
}
Loading
Loading