From 8ad6c360381ab131874cebf7385bfc3788f49ffe Mon Sep 17 00:00:00 2001 From: Kenn Jacobsen Date: Fri, 3 May 2024 08:47:10 +0200 Subject: [PATCH] Amend user start node handling (#16094) * Amend user start node handling * Add "has root access" to current user endpoint * Add document and media root access to user response model * Update OpenApi.json * Applied API suggestions --------- Co-authored-by: Sven Geusens --- .../Factories/UserPresentationFactory.cs | 16 +- .../Filters/RequireTreeRootAccessAttribute.cs | 4 - src/Umbraco.Cms.Api.Management/OpenApi.json | 24 +++ .../Entities/UserStartNodeEntitiesService.cs | 4 +- .../User/Current/CurrentUserResponseModel.cs | 6 +- .../ViewModels/User/UpdateUserRequestModel.cs | 4 + .../ViewModels/User/UserResponseModel.cs | 4 + src/Umbraco.Core/Models/UserUpdateModel.cs | 4 + src/Umbraco.Core/Services/UserService.cs | 32 ++-- .../Services/UserServiceCrudTests.Update.cs | 142 ++++++++++++++++++ .../Services/UserServiceCrudTests.cs | 2 +- 11 files changed, 220 insertions(+), 22 deletions(-) diff --git a/src/Umbraco.Cms.Api.Management/Factories/UserPresentationFactory.cs b/src/Umbraco.Cms.Api.Management/Factories/UserPresentationFactory.cs index f7a798bffa..581530cf8b 100644 --- a/src/Umbraco.Cms.Api.Management/Factories/UserPresentationFactory.cs +++ b/src/Umbraco.Cms.Api.Management/Factories/UserPresentationFactory.cs @@ -3,6 +3,7 @@ using Microsoft.Extensions.Options; using Umbraco.Cms.Api.Management.Security; using Umbraco.Cms.Api.Management.ViewModels.User; using Umbraco.Cms.Api.Management.ViewModels.User.Current; +using Umbraco.Cms.Core; using Umbraco.Cms.Api.Management.ViewModels.User.Item; using Umbraco.Cms.Core.Cache; using Umbraco.Cms.Core.Configuration.Models; @@ -70,7 +71,9 @@ public class UserPresentationFactory : IUserPresentationFactory State = user.UserState, UserGroupIds = new HashSet(user.Groups.Select(x => x.Key)), DocumentStartNodeIds = GetKeysFromIds(user.StartContentIds, UmbracoObjectTypes.Document), + HasDocumentRootAccess = HasRootAccess(user.StartContentIds), MediaStartNodeIds = GetKeysFromIds(user.StartMediaIds, UmbracoObjectTypes.Media), + HasMediaRootAccess = HasRootAccess(user.StartMediaIds), FailedLoginAttempts = user.FailedPasswordAttempts, LastLoginDate = user.LastLoginDate, LastLockoutDate = user.LastLockoutDate, @@ -159,7 +162,9 @@ public class UserPresentationFactory : IUserPresentationFactory UserName = updateModel.UserName, LanguageIsoCode = updateModel.LanguageIsoCode, ContentStartNodeKeys = updateModel.DocumentStartNodeIds, + HasContentRootAccess = updateModel.HasDocumentRootAccess, MediaStartNodeKeys = updateModel.MediaStartNodeIds, + HasMediaRootAccess = updateModel.HasMediaRootAccess }; model.UserGroupKeys = updateModel.UserGroupIds; @@ -172,8 +177,10 @@ public class UserPresentationFactory : IUserPresentationFactory var presentationUser = CreateResponseModel(user); var presentationGroups = await _userGroupPresentationFactory.CreateMultipleAsync(user.Groups); var languages = presentationGroups.SelectMany(x => x.Languages).Distinct().ToArray(); - var mediaStartNodeKeys = GetKeysFromIds(user.CalculateMediaStartNodeIds(_entityService, _appCaches), UmbracoObjectTypes.Media); - var documentStartNodeKeys = GetKeysFromIds(user.CalculateContentStartNodeIds(_entityService, _appCaches), UmbracoObjectTypes.Document); + var mediaStartNodeIds = user.CalculateMediaStartNodeIds(_entityService, _appCaches); + var mediaStartNodeKeys = GetKeysFromIds(mediaStartNodeIds, UmbracoObjectTypes.Media); + var contentStartNodeIds = user.CalculateContentStartNodeIds(_entityService, _appCaches); + var documentStartNodeKeys = GetKeysFromIds(contentStartNodeIds, UmbracoObjectTypes.Document); var permissions = presentationGroups.SelectMany(x => x.Permissions).ToHashSet(); var fallbackPermissions = presentationGroups.SelectMany(x => x.FallbackPermissions).ToHashSet(); @@ -192,7 +199,9 @@ public class UserPresentationFactory : IUserPresentationFactory AvatarUrls = presentationUser.AvatarUrls, LanguageIsoCode = presentationUser.LanguageIsoCode, MediaStartNodeIds = mediaStartNodeKeys, + HasMediaRootAccess = HasRootAccess(mediaStartNodeIds), DocumentStartNodeIds = documentStartNodeKeys, + HasDocumentRootAccess = HasRootAccess(contentStartNodeIds), Permissions = permissions, FallbackPermissions = fallbackPermissions, HasAccessToAllLanguages = hasAccessToAllLanguages, @@ -214,5 +223,6 @@ public class UserPresentationFactory : IUserPresentationFactory : new HashSet(keys); } - + private bool HasRootAccess(IEnumerable? startNodeIds) + => startNodeIds?.Contains(Constants.System.Root) is true; } diff --git a/src/Umbraco.Cms.Api.Management/Filters/RequireTreeRootAccessAttribute.cs b/src/Umbraco.Cms.Api.Management/Filters/RequireTreeRootAccessAttribute.cs index afe85fdc9c..a8b827aaf3 100644 --- a/src/Umbraco.Cms.Api.Management/Filters/RequireTreeRootAccessAttribute.cs +++ b/src/Umbraco.Cms.Api.Management/Filters/RequireTreeRootAccessAttribute.cs @@ -16,10 +16,6 @@ public abstract class RequireTreeRootAccessAttribute : ActionFilterAttribute IUser? user = backOfficeSecurityAccessor.BackOfficeSecurity?.CurrentUser; var startNodeIds = user != null ? GetUserStartNodeIds(user, context) : Array.Empty(); - - // TODO: remove this once we have backoffice auth in place - startNodeIds = new[] { Constants.System.Root }; - if (startNodeIds.Contains(Constants.System.Root)) { return; diff --git a/src/Umbraco.Cms.Api.Management/OpenApi.json b/src/Umbraco.Cms.Api.Management/OpenApi.json index 6386c94a42..595231be92 100644 --- a/src/Umbraco.Cms.Api.Management/OpenApi.json +++ b/src/Umbraco.Cms.Api.Management/OpenApi.json @@ -34357,6 +34357,8 @@ "fallbackPermissions", "hasAccessToAllLanguages", "hasAccessToSensitiveData", + "hasDocumentRootAccess", + "hasMediaRootAccess", "id", "isAdmin", "languages", @@ -34392,6 +34394,9 @@ "format": "uuid" } }, + "hasDocumentRootAccess": { + "type": "boolean" + }, "mediaStartNodeIds": { "uniqueItems": true, "type": "array", @@ -34400,6 +34405,9 @@ "format": "uuid" } }, + "hasMediaRootAccess": { + "type": "boolean" + }, "avatarUrls": { "type": "array", "items": { @@ -43206,6 +43214,8 @@ "required": [ "documentStartNodeIds", "email", + "hasDocumentRootAccess", + "hasMediaRootAccess", "languageIsoCode", "mediaStartNodeIds", "name", @@ -43242,6 +43252,9 @@ "format": "uuid" } }, + "hasDocumentRootAccess": { + "type": "boolean" + }, "mediaStartNodeIds": { "uniqueItems": true, "type": "array", @@ -43249,6 +43262,9 @@ "type": "string", "format": "uuid" } + }, + "hasMediaRootAccess": { + "type": "boolean" } }, "additionalProperties": false @@ -43626,6 +43642,8 @@ "documentStartNodeIds", "email", "failedLoginAttempts", + "hasDocumentRootAccess", + "hasMediaRootAccess", "id", "isAdmin", "mediaStartNodeIds", @@ -43670,6 +43688,9 @@ "format": "uuid" } }, + "hasDocumentRootAccess": { + "type": "boolean" + }, "mediaStartNodeIds": { "uniqueItems": true, "type": "array", @@ -43678,6 +43699,9 @@ "format": "uuid" } }, + "hasMediaRootAccess": { + "type": "boolean" + }, "avatarUrls": { "type": "array", "items": { diff --git a/src/Umbraco.Cms.Api.Management/Services/Entities/UserStartNodeEntitiesService.cs b/src/Umbraco.Cms.Api.Management/Services/Entities/UserStartNodeEntitiesService.cs index be309815d7..1a5572303b 100644 --- a/src/Umbraco.Cms.Api.Management/Services/Entities/UserStartNodeEntitiesService.cs +++ b/src/Umbraco.Cms.Api.Management/Services/Entities/UserStartNodeEntitiesService.cs @@ -19,7 +19,9 @@ public class UserStartNodeEntitiesService : IUserStartNodeEntitiesService // root entities for users without root access should include: // - the start nodes that are actual root entities (level == 1) // - the root level ancestors to the rest of the start nodes (required for browsing to the actual start nodes - will be marked as "no access") - IEntitySlim[] userStartEntities = _entityService.GetAll(umbracoObjectType, userStartNodeIds).ToArray(); + IEntitySlim[] userStartEntities = userStartNodeIds.Any() + ? _entityService.GetAll(umbracoObjectType, userStartNodeIds).ToArray() + : Array.Empty(); // find the start nodes that are at root level (level == 1) IEntitySlim[] allowedTopmostEntities = userStartEntities.Where(entity => entity.Level == 1).ToArray(); diff --git a/src/Umbraco.Cms.Api.Management/ViewModels/User/Current/CurrentUserResponseModel.cs b/src/Umbraco.Cms.Api.Management/ViewModels/User/Current/CurrentUserResponseModel.cs index 13fcd5ac2e..2e6b2cf1dc 100644 --- a/src/Umbraco.Cms.Api.Management/ViewModels/User/Current/CurrentUserResponseModel.cs +++ b/src/Umbraco.Cms.Api.Management/ViewModels/User/Current/CurrentUserResponseModel.cs @@ -1,6 +1,4 @@ -using Umbraco.Cms.Api.Management.ViewModels.UserGroup; using Umbraco.Cms.Api.Management.ViewModels.UserGroup.Permissions; -using Umbraco.Cms.Core.Models.Membership; namespace Umbraco.Cms.Api.Management.ViewModels.User.Current; @@ -18,8 +16,12 @@ public class CurrentUserResponseModel public required ISet DocumentStartNodeIds { get; init; } = new HashSet(); + public required bool HasDocumentRootAccess { get; init; } + public required ISet MediaStartNodeIds { get; init; } = new HashSet(); + public required bool HasMediaRootAccess { get; init; } + public required IEnumerable AvatarUrls { get; init; } = Enumerable.Empty(); public required IEnumerable Languages { get; init; } = Enumerable.Empty(); diff --git a/src/Umbraco.Cms.Api.Management/ViewModels/User/UpdateUserRequestModel.cs b/src/Umbraco.Cms.Api.Management/ViewModels/User/UpdateUserRequestModel.cs index da927a477b..bb85c7fb30 100644 --- a/src/Umbraco.Cms.Api.Management/ViewModels/User/UpdateUserRequestModel.cs +++ b/src/Umbraco.Cms.Api.Management/ViewModels/User/UpdateUserRequestModel.cs @@ -6,5 +6,9 @@ public class UpdateUserRequestModel : UserPresentationBase public ISet DocumentStartNodeIds { get; set; } = new HashSet(); + public bool HasDocumentRootAccess { get; init; } + public ISet MediaStartNodeIds { get; set; } = new HashSet(); + + public bool HasMediaRootAccess { get; init; } } diff --git a/src/Umbraco.Cms.Api.Management/ViewModels/User/UserResponseModel.cs b/src/Umbraco.Cms.Api.Management/ViewModels/User/UserResponseModel.cs index f886f7d69a..747110eabf 100644 --- a/src/Umbraco.Cms.Api.Management/ViewModels/User/UserResponseModel.cs +++ b/src/Umbraco.Cms.Api.Management/ViewModels/User/UserResponseModel.cs @@ -10,8 +10,12 @@ public class UserResponseModel : UserPresentationBase public ISet DocumentStartNodeIds { get; set; } = new HashSet(); + public bool HasDocumentRootAccess { get; set; } + public ISet MediaStartNodeIds { get; set; } = new HashSet(); + public bool HasMediaRootAccess { get; set; } + public IEnumerable AvatarUrls { get; set; } = Enumerable.Empty(); public UserState State { get; set; } diff --git a/src/Umbraco.Core/Models/UserUpdateModel.cs b/src/Umbraco.Core/Models/UserUpdateModel.cs index aeba002e64..684cbafda8 100644 --- a/src/Umbraco.Core/Models/UserUpdateModel.cs +++ b/src/Umbraco.Core/Models/UserUpdateModel.cs @@ -14,7 +14,11 @@ public class UserUpdateModel public ISet ContentStartNodeKeys { get; set; } = new HashSet(); + public bool HasContentRootAccess { get; set; } + public ISet MediaStartNodeKeys { get; set; } = new HashSet(); + public bool HasMediaRootAccess { get; set; } + public ISet UserGroupKeys { get; set; } = new HashSet(); } diff --git a/src/Umbraco.Core/Services/UserService.cs b/src/Umbraco.Core/Services/UserService.cs index 89791e0ad6..6992f80465 100644 --- a/src/Umbraco.Core/Services/UserService.cs +++ b/src/Umbraco.Core/Services/UserService.cs @@ -984,22 +984,32 @@ internal class UserService : RepositoryService, IUserService // We have to resolve the keys to ids to be compatible with the repository, this could be done in the factory, // but I'd rather keep the ids out of the service API as much as possible. - int[]? startContentIds = GetIdsFromKeys(model.ContentStartNodeKeys, UmbracoObjectTypes.Document); + List? startContentIds = GetIdsFromKeys(model.ContentStartNodeKeys, UmbracoObjectTypes.Document); - if (startContentIds is null || startContentIds.Length != model.ContentStartNodeKeys.Count) + if (startContentIds is null || startContentIds.Count != model.ContentStartNodeKeys.Count) { scope.Complete(); return Attempt.FailWithStatus(UserOperationStatus.ContentStartNodeNotFound, existingUser); } - int[]? startMediaIds = GetIdsFromKeys(model.MediaStartNodeKeys, UmbracoObjectTypes.Media); + List? startMediaIds = GetIdsFromKeys(model.MediaStartNodeKeys, UmbracoObjectTypes.Media); - if (startMediaIds is null || startMediaIds.Length != model.MediaStartNodeKeys.Count) + if (startMediaIds is null || startMediaIds.Count != model.MediaStartNodeKeys.Count) { scope.Complete(); return Attempt.FailWithStatus(UserOperationStatus.MediaStartNodeNotFound, existingUser); } + if (model.HasContentRootAccess) + { + startContentIds.Add(Constants.System.Root); + } + + if (model.HasMediaRootAccess) + { + startMediaIds.Add(Constants.System.Root); + } + Attempt isAuthorized = _userEditorAuthorizationHelper.IsAuthorized( performingUser, existingUser, @@ -1085,15 +1095,15 @@ internal class UserService : RepositoryService, IUserService UserUpdateModel source, ISet sourceUserGroups, IUser target, - int[]? startContentIds, - int[]? startMediaIds) + List startContentIds, + List startMediaIds) { target.Name = source.Name; target.Language = source.LanguageIsoCode; target.Email = source.Email; target.Username = source.UserName; - target.StartContentIds = startContentIds; - target.StartMediaIds = startMediaIds; + target.StartContentIds = startContentIds.ToArray(); + target.StartMediaIds = startMediaIds.ToArray(); target.ClearGroups(); foreach (IUserGroup group in sourceUserGroups) @@ -1152,13 +1162,13 @@ internal class UserService : RepositoryService, IUserService private static bool IsEmailValid(string email) => new EmailAddressAttribute().IsValid(email); - private int[]? GetIdsFromKeys(IEnumerable? guids, UmbracoObjectTypes type) + private List? GetIdsFromKeys(IEnumerable? guids, UmbracoObjectTypes type) { - int[]? keys = guids? + var keys = guids? .Select(x => _entityService.GetId(x, type)) .Where(x => x.Success) .Select(x => x.Result) - .ToArray(); + .ToList(); return keys; } diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserServiceCrudTests.Update.cs b/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserServiceCrudTests.Update.cs index 12f430cf1f..3850ada472 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserServiceCrudTests.Update.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserServiceCrudTests.Update.cs @@ -242,4 +242,146 @@ public partial class UserServiceCrudTests Assert.IsNotNull(updatedUser); Assert.AreEqual(isoCode, updatedUser.Language); } + + // todo Ideally we would test content and media separately and together (Introduce Testcases for switching permutations) + [Test] + public async Task Can_Assign_User_Start_Nodes() + { + var contentService = GetRequiredService(); + var mediaService = GetRequiredService(); + var contentStartNode = contentService.GetRootContent().First(); + var mediaStartNode = mediaService.CreateMediaWithIdentity("test", -1, "Image"); + + var userService = CreateUserService(securitySettings: new SecuritySettings { UsernameIsEmail = false }); + + var (updateModel, createdUser) = await CreateUserForUpdate(userService); + updateModel.ContentStartNodeKeys = new HashSet { contentStartNode.Key }; + updateModel.MediaStartNodeKeys = new HashSet { mediaStartNode.Key }; + + var result = await userService.UpdateAsync(Constants.Security.SuperUserKey, updateModel); + + Assert.IsTrue(result.Success); + var updatedUser = await userService.GetAsync(createdUser.Key); + + Assert.IsNotNull(updatedUser); + Assert.IsNotNull(updatedUser.StartContentIds); + Assert.AreEqual(1, updatedUser.StartContentIds.Length); + Assert.AreEqual(contentStartNode.Id, updatedUser.StartContentIds.First()); + Assert.IsNotNull(updatedUser.StartMediaIds); + Assert.AreEqual(1, updatedUser.StartMediaIds.Length); + Assert.AreEqual(mediaStartNode.Id, updatedUser.StartMediaIds.First()); + } + + [TestCase(false, false)] + [TestCase(false, true)] + [TestCase(true, false)] + [TestCase(true, true)] + public async Task Can_Assign_Root_As_User_Start_Node(bool contentRootAccess, bool mediaRootAccess) + { + var userService = CreateUserService(securitySettings: new SecuritySettings { UsernameIsEmail = false }); + + var (updateModel, createdUser) = await CreateUserForUpdate(userService); + updateModel.HasContentRootAccess = contentRootAccess; + updateModel.HasMediaRootAccess = mediaRootAccess; + + var result = await userService.UpdateAsync(Constants.Security.SuperUserKey, updateModel); + + Assert.IsTrue(result.Success); + var updatedUser = await userService.GetAsync(createdUser.Key); + Assert.IsNotNull(updatedUser); + + Assert.IsNotNull(updatedUser.StartContentIds); + if (contentRootAccess) + { + Assert.AreEqual(1, updatedUser.StartContentIds.Length); + Assert.AreEqual(Constants.System.Root, updatedUser.StartContentIds.First()); + } + else + { + Assert.IsEmpty(updatedUser.StartContentIds); + } + + Assert.IsNotNull(updatedUser.StartMediaIds); + if (mediaRootAccess) + { + Assert.AreEqual(1, updatedUser.StartMediaIds.Length); + Assert.AreEqual(Constants.System.Root, updatedUser.StartMediaIds.First()); + } + else + { + Assert.IsEmpty(updatedUser.StartMediaIds); + } + } + + // todo Ideally we would test content and media separately and together (Introduce Testcases for switching permutations) + [Test] + public async Task Can_Unassign_User_Start_Nodes() + { + var contentService = GetRequiredService(); + var mediaService = GetRequiredService(); + var contentStartNode = contentService.GetRootContent().First(); + var mediaStartNode = mediaService.CreateMediaWithIdentity("test", -1, "Image"); + + var userService = CreateUserService(securitySettings: new SecuritySettings { UsernameIsEmail = false }); + + var (updateModel, createdUser) = await CreateUserForUpdate(userService); + updateModel.ContentStartNodeKeys = new HashSet { contentStartNode.Key }; + updateModel.MediaStartNodeKeys = new HashSet { mediaStartNode.Key }; + + await userService.UpdateAsync(Constants.Security.SuperUserKey, updateModel); + + var updatedUser = await userService.GetAsync(createdUser.Key); + Assert.IsNotNull(updatedUser); + Assert.IsNotEmpty(updatedUser.StartContentIds!); + Assert.IsNotEmpty(updatedUser.StartMediaIds!); + + updateModel = await MapUserToUpdateModel(updatedUser); + updateModel.ContentStartNodeKeys = new HashSet(); + updateModel.MediaStartNodeKeys = new HashSet(); + + var result = await userService.UpdateAsync(Constants.Security.SuperUserKey, updateModel); + + Assert.IsTrue(result.Success); + updatedUser = await userService.GetAsync(createdUser.Key); + Assert.IsNotNull(updatedUser); + + Assert.IsNotNull(updatedUser.StartContentIds); + Assert.IsEmpty(updatedUser.StartContentIds); + + Assert.IsNotNull(updatedUser.StartMediaIds); + Assert.IsEmpty(updatedUser.StartMediaIds); + } + + // todo Ideally we would test content and media separately and together (Introduce Testcases for switching permutations) + [Test] + public async Task Can_Unassign_Root_As_User_Start_Node() + { + var userService = CreateUserService(securitySettings: new SecuritySettings { UsernameIsEmail = false }); + + var (updateModel, createdUser) = await CreateUserForUpdate(userService); + updateModel.HasContentRootAccess = true; + updateModel.HasMediaRootAccess = true; + + await userService.UpdateAsync(Constants.Security.SuperUserKey, updateModel); + var updatedUser = await userService.GetAsync(createdUser.Key); + Assert.IsNotNull(updatedUser); + Assert.IsNotEmpty(updatedUser.StartContentIds!); + Assert.IsNotEmpty(updatedUser.StartMediaIds!); + + updateModel = await MapUserToUpdateModel(updatedUser); + updateModel.HasContentRootAccess = false; + updateModel.HasMediaRootAccess = false; + + var result = await userService.UpdateAsync(Constants.Security.SuperUserKey, updateModel); + + Assert.IsTrue(result.Success); + updatedUser = await userService.GetAsync(createdUser.Key); + Assert.IsNotNull(updatedUser); + + Assert.IsNotNull(updatedUser.StartContentIds); + Assert.IsEmpty(updatedUser.StartContentIds); + + Assert.IsNotNull(updatedUser.StartMediaIds); + Assert.IsEmpty(updatedUser.StartMediaIds); + } } diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserServiceCrudTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserServiceCrudTests.cs index c8bcb16ca9..0dc52c0bab 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserServiceCrudTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserServiceCrudTests.cs @@ -23,7 +23,7 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Core.Services; [TestFixture] [UmbracoTest(Database = UmbracoTestOptions.Database.NewSchemaPerTest)] -public partial class UserServiceCrudTests : UmbracoIntegrationTest +public partial class UserServiceCrudTests : UmbracoIntegrationTestWithContent { private IUserGroupService UserGroupService => GetRequiredService();