diff --git a/src/Umbraco.Cms.Api.Management/Controllers/UserGroup/ByKeyUserGroupController.cs b/src/Umbraco.Cms.Api.Management/Controllers/UserGroup/ByKeyUserGroupController.cs index 67ce7fd76e..0574524fbb 100644 --- a/src/Umbraco.Cms.Api.Management/Controllers/UserGroup/ByKeyUserGroupController.cs +++ b/src/Umbraco.Cms.Api.Management/Controllers/UserGroup/ByKeyUserGroupController.cs @@ -3,7 +3,6 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; using Umbraco.Cms.Api.Management.Factories; -using Umbraco.Cms.Api.Management.Security.Authorization.UserGroup; using Umbraco.Cms.Api.Management.ViewModels.UserGroup; using Umbraco.Cms.Core.Models.Membership; using Umbraco.Cms.Core.Security.Authorization; diff --git a/src/Umbraco.Cms.Api.Management/Controllers/UserGroup/UserGroupControllerBase.cs b/src/Umbraco.Cms.Api.Management/Controllers/UserGroup/UserGroupControllerBase.cs index 9ad5cead4d..a5691932b1 100644 --- a/src/Umbraco.Cms.Api.Management/Controllers/UserGroup/UserGroupControllerBase.cs +++ b/src/Umbraco.Cms.Api.Management/Controllers/UserGroup/UserGroupControllerBase.cs @@ -29,11 +29,15 @@ public class UserGroupControllerBase : ManagementApiControllerBase .WithTitle("Duplicate alias") .WithDetail("A user group already exists with the attempted alias.") .Build()), + UserGroupOperationStatus.CanNotUpdateAliasIsSystemUserGroup => BadRequest(problemDetailsBuilder + .WithTitle("System user group") + .WithDetail("Changing the alias is not allowed on a system user group.") + .Build()), UserGroupOperationStatus.MissingUser => Unauthorized(problemDetailsBuilder .WithTitle("Missing user") .WithDetail("A performing user was not found when attempting the operation.") .Build()), - UserGroupOperationStatus.IsSystemUserGroup => BadRequest(problemDetailsBuilder + UserGroupOperationStatus.CanNotDeleteIsSystemUserGroup => BadRequest(problemDetailsBuilder .WithTitle("System user group") .WithDetail("The operation is not allowed on a system user group.") .Build()), diff --git a/src/Umbraco.Cms.Api.Management/Factories/UserGroupPresentationFactory.cs b/src/Umbraco.Cms.Api.Management/Factories/UserGroupPresentationFactory.cs index d05ff8aa52..0119864f5f 100644 --- a/src/Umbraco.Cms.Api.Management/Factories/UserGroupPresentationFactory.cs +++ b/src/Umbraco.Cms.Api.Management/Factories/UserGroupPresentationFactory.cs @@ -51,8 +51,9 @@ public class UserGroupPresentationFactory : IUserGroupPresentationFactory return new UserGroupResponseModel { - Name = userGroup.Name ?? string.Empty, Id = userGroup.Key, + Name = userGroup.Name ?? string.Empty, + Alias = userGroup.Alias, DocumentStartNode = ReferenceByIdModel.ReferenceOrNull(contentStartNodeKey), DocumentRootAccess = contentRootAccess, MediaStartNode = ReferenceByIdModel.ReferenceOrNull(mediaStartNodeKey), @@ -63,7 +64,8 @@ public class UserGroupPresentationFactory : IUserGroupPresentationFactory FallbackPermissions = userGroup.Permissions, Permissions = await _permissionPresentationFactory.CreateAsync(userGroup.GranularPermissions), Sections = userGroup.AllowedSections.Select(SectionMapper.GetName), - IsSystemGroup = userGroup.IsSystemUserGroup() + IsDeletable = !userGroup.IsSystemUserGroup(), + AliasCanBeChanged = !userGroup.IsSystemUserGroup(), }; } @@ -83,8 +85,9 @@ public class UserGroupPresentationFactory : IUserGroupPresentationFactory return new UserGroupResponseModel { - Name = userGroup.Name ?? string.Empty, Id = userGroup.Key, + Name = userGroup.Name ?? string.Empty, + Alias = userGroup.Alias, DocumentStartNode = ReferenceByIdModel.ReferenceOrNull(contentStartNodeKey), MediaStartNode = ReferenceByIdModel.ReferenceOrNull(mediaStartNodeKey), Icon = userGroup.Icon, @@ -93,6 +96,8 @@ public class UserGroupPresentationFactory : IUserGroupPresentationFactory FallbackPermissions = userGroup.Permissions, Permissions = await _permissionPresentationFactory.CreateAsync(userGroup.GranularPermissions), Sections = userGroup.AllowedSections.Select(SectionMapper.GetName), + IsDeletable = !userGroup.IsSystemUserGroup(), + AliasCanBeChanged = !userGroup.IsSystemUserGroup(), }; } @@ -107,6 +112,7 @@ public class UserGroupPresentationFactory : IUserGroupPresentationFactory return userGroupViewModels; } + /// public async Task> CreateMultipleAsync(IEnumerable userGroups) { @@ -122,18 +128,21 @@ public class UserGroupPresentationFactory : IUserGroupPresentationFactory /// public async Task> CreateAsync(CreateUserGroupRequestModel requestModel) { - var cleanedName = requestModel.Name.CleanForXss('[', ']', '(', ')', ':'); - var group = new UserGroup(_shortStringHelper) { - Name = cleanedName, - Alias = cleanedName, + Name = CleanUserGroupNameOrAliasForXss(requestModel.Name), + Alias = CleanUserGroupNameOrAliasForXss(requestModel.Alias), Icon = requestModel.Icon, HasAccessToAllLanguages = requestModel.HasAccessToAllLanguages, Permissions = requestModel.FallbackPermissions, - GranularPermissions = await _permissionPresentationFactory.CreatePermissionSetsAsync(requestModel.Permissions) + GranularPermissions = await _permissionPresentationFactory.CreatePermissionSetsAsync(requestModel.Permissions), }; + if (requestModel.Id.HasValue) + { + group.Key = requestModel.Id.Value; + } + Attempt assignmentAttempt = AssignStartNodesToUserGroup(requestModel, group); if (assignmentAttempt.Success is false) { @@ -186,7 +195,8 @@ public class UserGroupPresentationFactory : IUserGroupPresentationFactory current.AddAllowedSection(SectionMapper.GetAlias(sectionName)); } - current.Name = request.Name.CleanForXss('[', ']', '(', ')', ':'); + current.Name = CleanUserGroupNameOrAliasForXss(request.Name); + current.Alias = CleanUserGroupNameOrAliasForXss(request.Alias); current.Icon = request.Icon; current.HasAccessToAllLanguages = request.HasAccessToAllLanguages; @@ -196,6 +206,9 @@ public class UserGroupPresentationFactory : IUserGroupPresentationFactory return Attempt.SucceedWithStatus(UserGroupOperationStatus.Success, current); } + private static string CleanUserGroupNameOrAliasForXss(string input) + => input.CleanForXss('[', ']', '(', ')', ':'); + private async Task, UserGroupOperationStatus>> MapLanguageIdsToIsoCodeAsync(IEnumerable ids) { IEnumerable languages = await _languageService.GetAllAsync(); diff --git a/src/Umbraco.Cms.Api.Management/Mapping/Item/ItemTypeMapDefinition.cs b/src/Umbraco.Cms.Api.Management/Mapping/Item/ItemTypeMapDefinition.cs index dd236ea8f8..66020860a1 100644 --- a/src/Umbraco.Cms.Api.Management/Mapping/Item/ItemTypeMapDefinition.cs +++ b/src/Umbraco.Cms.Api.Management/Mapping/Item/ItemTypeMapDefinition.cs @@ -112,6 +112,7 @@ public class ItemTypeMapDefinition : IMapDefinition target.Id = source.Key; target.Name = source.Name ?? source.Alias; target.Icon = source.Icon; + target.Alias = source.Alias; } // Umbraco.Code.MapAll diff --git a/src/Umbraco.Cms.Api.Management/OpenApi.json b/src/Umbraco.Cms.Api.Management/OpenApi.json index 595231be92..f25b163e73 100644 --- a/src/Umbraco.Cms.Api.Management/OpenApi.json +++ b/src/Umbraco.Cms.Api.Management/OpenApi.json @@ -34131,6 +34131,7 @@ }, "CreateUserGroupRequestModel": { "required": [ + "alias", "documentRootAccess", "fallbackPermissions", "hasAccessToAllLanguages", @@ -34145,6 +34146,9 @@ "name": { "type": "string" }, + "alias": { + "type": "string" + }, "icon": { "type": "string", "nullable": true @@ -34206,6 +34210,11 @@ } ] } + }, + "id": { + "type": "string", + "format": "uuid", + "nullable": true } }, "additionalProperties": false @@ -43105,6 +43114,7 @@ }, "UpdateUserGroupRequestModel": { "required": [ + "alias", "documentRootAccess", "fallbackPermissions", "hasAccessToAllLanguages", @@ -43119,6 +43129,9 @@ "name": { "type": "string" }, + "alias": { + "type": "string" + }, "icon": { "type": "string", "nullable": true @@ -43432,12 +43445,17 @@ "icon": { "type": "string", "nullable": true + }, + "alias": { + "type": "string", + "nullable": true } }, "additionalProperties": false }, "UserGroupResponseModel": { "required": [ + "alias", "documentRootAccess", "fallbackPermissions", "hasAccessToAllLanguages", @@ -43454,6 +43472,9 @@ "name": { "type": "string" }, + "alias": { + "type": "string" + }, "icon": { "type": "string", "nullable": true diff --git a/src/Umbraco.Cms.Api.Management/ViewModels/UserGroup/CreateUserGroupRequestModel.cs b/src/Umbraco.Cms.Api.Management/ViewModels/UserGroup/CreateUserGroupRequestModel.cs index 41b11efaed..89789b3651 100644 --- a/src/Umbraco.Cms.Api.Management/ViewModels/UserGroup/CreateUserGroupRequestModel.cs +++ b/src/Umbraco.Cms.Api.Management/ViewModels/UserGroup/CreateUserGroupRequestModel.cs @@ -2,5 +2,5 @@ public class CreateUserGroupRequestModel : UserGroupBase { - + public Guid? Id { get; set; } } diff --git a/src/Umbraco.Cms.Api.Management/ViewModels/UserGroup/Item/UserGroupItemResponseModel.cs b/src/Umbraco.Cms.Api.Management/ViewModels/UserGroup/Item/UserGroupItemResponseModel.cs index 54730f1141..52167ef899 100644 --- a/src/Umbraco.Cms.Api.Management/ViewModels/UserGroup/Item/UserGroupItemResponseModel.cs +++ b/src/Umbraco.Cms.Api.Management/ViewModels/UserGroup/Item/UserGroupItemResponseModel.cs @@ -5,4 +5,6 @@ namespace Umbraco.Cms.Api.Management.ViewModels.UserGroup.Item; public class UserGroupItemResponseModel : NamedItemResponseModelBase { public string? Icon { get; set; } + + public string? Alias { get; set; } } diff --git a/src/Umbraco.Cms.Api.Management/ViewModels/UserGroup/UserGroupBase.cs b/src/Umbraco.Cms.Api.Management/ViewModels/UserGroup/UserGroupBase.cs index 2338d89f6a..d9f1695359 100644 --- a/src/Umbraco.Cms.Api.Management/ViewModels/UserGroup/UserGroupBase.cs +++ b/src/Umbraco.Cms.Api.Management/ViewModels/UserGroup/UserGroupBase.cs @@ -17,6 +17,11 @@ public class UserGroupBase /// public required string Name { get; init; } + /// + /// The alias of the user groups + /// + public required string Alias { get; init; } + /// /// The Icon for the user group /// diff --git a/src/Umbraco.Cms.Api.Management/ViewModels/UserGroup/UserGroupResponseModel.cs b/src/Umbraco.Cms.Api.Management/ViewModels/UserGroup/UserGroupResponseModel.cs index dc555484fe..f392a5c247 100644 --- a/src/Umbraco.Cms.Api.Management/ViewModels/UserGroup/UserGroupResponseModel.cs +++ b/src/Umbraco.Cms.Api.Management/ViewModels/UserGroup/UserGroupResponseModel.cs @@ -10,5 +10,10 @@ public class UserGroupResponseModel : UserGroupBase /// /// Whether this user group is required at system level (thus cannot be removed) /// - public bool IsSystemGroup { get; set; } + public bool IsDeletable { get; set; } + + /// + /// Whether this user group is required at system level (thus alias needs to be fixed) + /// + public bool AliasCanBeChanged { get; set; } } diff --git a/src/Umbraco.Core/Services/OperationStatus/UserGroupOperationStatus.cs b/src/Umbraco.Core/Services/OperationStatus/UserGroupOperationStatus.cs index f7dc1f76e0..7870429e4c 100644 --- a/src/Umbraco.Core/Services/OperationStatus/UserGroupOperationStatus.cs +++ b/src/Umbraco.Core/Services/OperationStatus/UserGroupOperationStatus.cs @@ -8,7 +8,8 @@ public enum UserGroupOperationStatus AlreadyExists, DuplicateAlias, MissingUser, - IsSystemUserGroup, + CanNotDeleteIsSystemUserGroup, + CanNotUpdateAliasIsSystemUserGroup, CancelledByNotification, MediaStartNodeKeyNotFound, DocumentStartNodeKeyNotFound, diff --git a/src/Umbraco.Core/Services/UserGroupService.cs b/src/Umbraco.Core/Services/UserGroupService.cs index c87ba2dd4b..1d9461681c 100644 --- a/src/Umbraco.Core/Services/UserGroupService.cs +++ b/src/Umbraco.Core/Services/UserGroupService.cs @@ -263,7 +263,7 @@ internal sealed class UserGroupService : RepositoryService, IUserGroupService if (userGroup.IsSystemUserGroup()) { - return Attempt.Fail(UserGroupOperationStatus.IsSystemUserGroup); + return Attempt.Fail(UserGroupOperationStatus.CanNotDeleteIsSystemUserGroup); } return Attempt.Succeed(UserGroupOperationStatus.Success); @@ -520,12 +520,18 @@ internal sealed class UserGroupService : RepositoryService, IUserGroupService return UserGroupOperationStatus.NotFound; } - IUserGroup? existing = _userGroupRepository.Get(userGroup.Alias); - if (existing is not null && existing.Key != userGroup.Key) + IUserGroup? existingByAlias = _userGroupRepository.Get(userGroup.Alias); + if (existingByAlias is not null && existingByAlias.Key != userGroup.Key) { return UserGroupOperationStatus.DuplicateAlias; } + IUserGroup? existingByKey = await GetAsync(userGroup.Key); + if (existingByKey is not null && existingByKey.IsSystemUserGroup() && existingByKey.Alias != userGroup.Alias) + { + return UserGroupOperationStatus.CanNotUpdateAliasIsSystemUserGroup; + } + return UserGroupOperationStatus.Success; } diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserGroupServiceValidationTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserGroupServiceValidationTests.cs index aeca51b3aa..9d3d8aeee5 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserGroupServiceValidationTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserGroupServiceValidationTests.cs @@ -185,7 +185,7 @@ public class UserGroupServiceValidationTests : UmbracoIntegrationTest var result = await UserGroupService.DeleteAsync(key); Assert.IsFalse(result.Success); - Assert.AreEqual(UserGroupOperationStatus.IsSystemUserGroup, result.Result); + Assert.AreEqual(UserGroupOperationStatus.CanNotDeleteIsSystemUserGroup, result.Result); } // these keys are not defined as "const" in Constants.Security but as "static readonly", so we have to hardcode diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Services/UserGroupServiceTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Services/UserGroupServiceTests.cs index 2384c72eb2..268b3f0659 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Services/UserGroupServiceTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Services/UserGroupServiceTests.cs @@ -1,12 +1,17 @@ +using System.Data; +using System.Linq.Expressions; using Microsoft.Extensions.Logging; using Moq; using NUnit.Framework; using Umbraco.Cms.Core; using Umbraco.Cms.Core.Events; using Umbraco.Cms.Core.Models.Membership; +using Umbraco.Cms.Core.Notifications; +using Umbraco.Cms.Core.Persistence.Querying; using Umbraco.Cms.Core.Persistence.Repositories; using Umbraco.Cms.Core.Scoping; using Umbraco.Cms.Core.Services; +using Umbraco.Cms.Core.Services.OperationStatus; using Umbraco.Cms.Core.Strings; namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Services; @@ -21,7 +26,7 @@ public class UserGroupServiceTests public async Task Filter_Returns_Only_User_Groups_For_Non_Admin(params string[] userGroupAliases) { var userKey = Guid.NewGuid(); - var userGroupService = SetupUserGroupService(userKey, userGroupAliases); + var userGroupService = SetupUserGroupServiceWithUserAndGetManyReturnsFourGroups(userKey, userGroupAliases); var result = await userGroupService.FilterAsync(userKey, null, 0, 10); Assert.Multiple(() => @@ -41,7 +46,7 @@ public class UserGroupServiceTests public async Task Filter_Does_Not_Return_Non_Existing_Groups(params string[] userGroupAliases) { var userKey = Guid.NewGuid(); - var userGroupService = SetupUserGroupService(userKey, userGroupAliases); + var userGroupService = SetupUserGroupServiceWithUserAndGetManyReturnsFourGroups(userKey, userGroupAliases); var result = await userGroupService.FilterAsync(userKey, null, 0, 10); Assert.Multiple(() => @@ -55,7 +60,7 @@ public class UserGroupServiceTests public async Task Filter_Returns_All_Groups_For_Admin() { var userKey = Guid.NewGuid(); - var userGroupService = SetupUserGroupService(userKey, new [] { Constants.Security.AdminGroupAlias }); + var userGroupService = SetupUserGroupServiceWithUserAndGetManyReturnsFourGroups(userKey, new [] { Constants.Security.AdminGroupAlias }); var result = await userGroupService.FilterAsync(userKey, null, 0, 10); Assert.Multiple(() => @@ -73,7 +78,7 @@ public class UserGroupServiceTests public async Task Filter_Can_Filter_By_Group_Name() { var userKey = Guid.NewGuid(); - var userGroupService = SetupUserGroupService(userKey, new [] { Constants.Security.AdminGroupAlias }); + var userGroupService = SetupUserGroupServiceWithUserAndGetManyReturnsFourGroups(userKey, new [] { Constants.Security.AdminGroupAlias }); var result = await userGroupService.FilterAsync(userKey, "e", 0, 10); Assert.Multiple(() => @@ -85,6 +90,79 @@ public class UserGroupServiceTests }); } + [TestCase(false,UserGroupOperationStatus.Success)] + [TestCase(true,UserGroupOperationStatus.CanNotUpdateAliasIsSystemUserGroup)] + public async Task Can_Not_Update_SystemGroup_Alias(bool isSystemGroup, UserGroupOperationStatus status) + { + // Arrange + var actingUserKey = Guid.NewGuid(); + var mockUser = SetupUserWithGroupAccess(actingUserKey, [Constants.Security.AdminGroupAlias]); + var userService = SetupUserServiceWithGetUserByKey(actingUserKey, mockUser); + var userGroupRepository = new Mock(); + var userGroupKey = Guid.NewGuid(); + var persistedUserGroup = + new UserGroup( + Mock.Of(), + 0, + isSystemGroup ? Constants.Security.AdminGroupAlias : "someNonSystemAlias", + "Administrators", + null) + { + Id = 10, + Key = userGroupKey, + }; + userGroupRepository + .Setup(r => r.Get(It.IsAny>())) + .Returns(new[] + { + persistedUserGroup + }); + var updatingUserGroup = new UserGroup(Mock.Of(), 0, persistedUserGroup.Alias + "updated", + persistedUserGroup.Name + "updated", null) + { + Key = persistedUserGroup.Key, + Id = persistedUserGroup.Id + }; + + var scopedNotificationPublisher = new Mock(); + scopedNotificationPublisher.Setup(p => p.PublishCancelableAsync(It.IsAny())) + .ReturnsAsync(false); + + var scope = new Mock(); + scope.SetupGet(s => s.Notifications).Returns(scopedNotificationPublisher.Object); + + var query = new Mock>(); + query.Setup(q => q.Where(It.IsAny>>())).Returns(query.Object); + + var provider = new Mock(); + provider.Setup(p => p.CreateQuery()).Returns(query.Object); + provider.Setup(p => p.CreateCoreScope( + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny())) + .Returns(scope.Object); + + var service = new UserGroupService( + provider.Object, + Mock.Of(), + Mock.Of(), + userGroupRepository.Object, + Mock.Of(), + Mock.Of(), + userService.Object, + Mock.Of>()); + + // act + var updateAttempt = await service.UpdateAsync(updatingUserGroup, actingUserKey); + + // assert + Assert.AreEqual(status, updateAttempt.Status); + } + private IEnumerable CreateGroups(params string[] aliases) => aliases.Select(alias => { @@ -93,14 +171,11 @@ public class UserGroupServiceTests return group.Object; }).ToArray(); - private IUserGroupService SetupUserGroupService(Guid userKey, string[] userGroupAliases) + private IUserGroupService SetupUserGroupServiceWithUserAndGetManyReturnsFourGroups(Guid userKey, string[] userGroupAliases) { - var user = new Mock(); - user.SetupGet(u => u.Key).Returns(userKey); - user.Setup(u => u.Groups).Returns(CreateGroups(userGroupAliases)); + var mockUser = SetupUserWithGroupAccess(userKey, userGroupAliases); - var userService = new Mock(); - userService.Setup(s => s.GetAsync(userKey)).Returns(Task.FromResult(user.Object)); + var userService = SetupUserServiceWithGetUserByKey(userKey, mockUser); var userGroupRepository = new Mock(); userGroupRepository @@ -123,4 +198,19 @@ public class UserGroupServiceTests userService.Object, Mock.Of>()); } + + private Mock SetupUserWithGroupAccess(Guid userKey, string[] userGroupAliases) + { + var user = new Mock(); + user.SetupGet(u => u.Key).Returns(userKey); + user.Setup(u => u.Groups).Returns(CreateGroups(userGroupAliases)); + return user; + } + + private Mock SetupUserServiceWithGetUserByKey(Guid userKey, Mock mockUser) + { + var userService = new Mock(); + userService.Setup(s => s.GetAsync(userKey)).Returns(Task.FromResult(mockUser.Object)); + return userService; + } }