From eca13ea011ebb8140364d06d548c53ff80501436 Mon Sep 17 00:00:00 2001 From: Mole Date: Thu, 7 Mar 2024 13:21:48 +0100 Subject: [PATCH] V14: Add additional validation around manipulating groups (#15834) * Add additional validation when removing a user from usergroup * Add additional validation to UpdateUserGroups * Don't re-implement operation results * Add additional validation to the update user endpoint * Complete scopes where it's safe to do so --- .../User/UpdateUserGroupsUserController.cs | 19 ++- .../User/UserOrCurrentUserControllerBase.cs | 4 + .../RemoveUsersFromUserGroupController.cs | 7 +- .../UserGroup/UserGroupsControllerBase.cs | 7 + ...esolvedUserToUserGroupManipulationModel.cs | 10 ++ .../UsersToUserGroupManipulationModel.cs | 3 +- .../Services/IUserGroupService.cs | 2 +- .../UserGroupOperationStatus.cs | 4 +- .../OperationStatus/UserOperationStatus.cs | 1 + src/Umbraco.Core/Services/UserGroupService.cs | 126 ++++++++++++++---- src/Umbraco.Core/Services/UserService.cs | 22 ++- 11 files changed, 159 insertions(+), 46 deletions(-) create mode 100644 src/Umbraco.Core/Models/ResolvedUserToUserGroupManipulationModel.cs diff --git a/src/Umbraco.Cms.Api.Management/Controllers/User/UpdateUserGroupsUserController.cs b/src/Umbraco.Cms.Api.Management/Controllers/User/UpdateUserGroupsUserController.cs index a9227d28e3..41035f1d23 100644 --- a/src/Umbraco.Cms.Api.Management/Controllers/User/UpdateUserGroupsUserController.cs +++ b/src/Umbraco.Cms.Api.Management/Controllers/User/UpdateUserGroupsUserController.cs @@ -2,17 +2,25 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; -using Umbraco.Cms.Api.Management.Security.Authorization.User; +using Umbraco.Cms.Api.Management.Controllers.UserGroup; +using Umbraco.Cms.Api.Management.Routing; using Umbraco.Cms.Api.Management.ViewModels.User; using Umbraco.Cms.Core.Security.Authorization; using Umbraco.Cms.Core.Services; +using Umbraco.Cms.Core.Services.OperationStatus; using Umbraco.Cms.Web.Common.Authorization; using Umbraco.Extensions; namespace Umbraco.Cms.Api.Management.Controllers.User; +// This controller is a bit of a weird case, for all intents and purposes this should be a UserGroupController +// It uses the UserGroupService to manipulate the members of a user group, however, from the frontend perspective it is a user(s) operation +// In order to not have to re-implement all the UserGroupOperationStatusResults this controller inherits from UserGroupControllerBase +// But manually specifies its route and APIExplorerSettings to be under users. [ApiVersion("1.0")] -public class UpdateUserGroupsUserController : UserControllerBase +[VersionedApiBackOfficeRoute("user")] +[ApiExplorerSettings(GroupName = "User")] +public class UpdateUserGroupsUserController : UserGroupControllerBase { private readonly IAuthorizationService _authorizationService; private readonly IUserGroupService _userGroupService; @@ -33,12 +41,13 @@ public class UpdateUserGroupsUserController : UserControllerBase UserPermissionResource.WithKeys(requestModel.UserIds), AuthorizationPolicies.UserPermissionByResource); - if (!authorizationResult.Succeeded) + if (authorizationResult.Succeeded is false) { return Forbidden(); } - await _userGroupService.UpdateUserGroupsOnUsers(requestModel.UserGroupIds, requestModel.UserIds); - return Ok(); + UserGroupOperationStatus status = await _userGroupService.UpdateUserGroupsOnUsersAsync(requestModel.UserGroupIds, requestModel.UserIds); + + return UserGroupOperationStatusResult(status); } } diff --git a/src/Umbraco.Cms.Api.Management/Controllers/User/UserOrCurrentUserControllerBase.cs b/src/Umbraco.Cms.Api.Management/Controllers/User/UserOrCurrentUserControllerBase.cs index 6b9f20ea1e..18eb49d4fe 100644 --- a/src/Umbraco.Cms.Api.Management/Controllers/User/UserOrCurrentUserControllerBase.cs +++ b/src/Umbraco.Cms.Api.Management/Controllers/User/UserOrCurrentUserControllerBase.cs @@ -19,6 +19,10 @@ public abstract class UserOrCurrentUserControllerBase : ManagementApiControllerB .WithTitle("Missing User Group") .WithDetail("The specified user group was not found.") .Build()), + UserOperationStatus.AdminUserGroupMustNotBeEmpty => BadRequest(problemDetailsBuilder + .WithTitle("Admin User Group Must Not Be Empty") + .WithDetail("The admin user group must not be empty.") + .Build()), UserOperationStatus.NoUserGroup => BadRequest(problemDetailsBuilder .WithTitle("No User Group Specified") .WithDetail("A user group must be specified to create a user") diff --git a/src/Umbraco.Cms.Api.Management/Controllers/UserGroup/RemoveUsersFromUserGroupController.cs b/src/Umbraco.Cms.Api.Management/Controllers/UserGroup/RemoveUsersFromUserGroupController.cs index fa4748b412..16fc75e150 100644 --- a/src/Umbraco.Cms.Api.Management/Controllers/UserGroup/RemoveUsersFromUserGroupController.cs +++ b/src/Umbraco.Cms.Api.Management/Controllers/UserGroup/RemoveUsersFromUserGroupController.cs @@ -2,12 +2,7 @@ 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; using Umbraco.Cms.Core.Models; -using Umbraco.Cms.Core.Models.Membership; using Umbraco.Cms.Core.Security; using Umbraco.Cms.Core.Security.Authorization; using Umbraco.Cms.Core.Services; @@ -45,7 +40,7 @@ public class RemoveUsersFromUserGroupController : UserGroupControllerBase UserGroupPermissionResource.WithKeys(id), AuthorizationPolicies.UserBelongsToUserGroupInRequest); - if (!authorizationResult.Succeeded) + if (authorizationResult.Succeeded is false) { return Forbidden(); } diff --git a/src/Umbraco.Cms.Api.Management/Controllers/UserGroup/UserGroupsControllerBase.cs b/src/Umbraco.Cms.Api.Management/Controllers/UserGroup/UserGroupsControllerBase.cs index 93daff8c3b..023435ea8b 100644 --- a/src/Umbraco.Cms.Api.Management/Controllers/UserGroup/UserGroupsControllerBase.cs +++ b/src/Umbraco.Cms.Api.Management/Controllers/UserGroup/UserGroupsControllerBase.cs @@ -69,6 +69,13 @@ public class UserGroupControllerBase : ManagementApiControllerBase .WithTitle("Missing user group name.") .WithDetail("The user group name is required, and cannot be an empty string.") .Build()), + UserGroupOperationStatus.AdminGroupCannotBeEmpty => BadRequest(problemDetailsBuilder + .WithTitle("Admin group cannot be empty") + .WithDetail("The admin group cannot be empty.") + .Build()), + UserGroupOperationStatus.UserNotInGroup => BadRequest(problemDetailsBuilder + .WithTitle("User not in group") + .WithDetail("The user is not in the group.")), UserGroupOperationStatus.UnauthorizedMissingAllowedSectionAccess => Unauthorized(problemDetailsBuilder .WithTitle("Unauthorized section access") .WithDetail("The performing user does not have access to all sections specified as allowed for this user group.") diff --git a/src/Umbraco.Core/Models/ResolvedUserToUserGroupManipulationModel.cs b/src/Umbraco.Core/Models/ResolvedUserToUserGroupManipulationModel.cs new file mode 100644 index 0000000000..33eda14f0e --- /dev/null +++ b/src/Umbraco.Core/Models/ResolvedUserToUserGroupManipulationModel.cs @@ -0,0 +1,10 @@ +using Umbraco.Cms.Core.Models.Membership; + +namespace Umbraco.Cms.Core.Models; + +public class ResolvedUserToUserGroupManipulationModel +{ + public required IUser[] Users { get; init; } + + public required IUserGroup UserGroup { get; init; } +} diff --git a/src/Umbraco.Core/Models/UsersToUserGroupManipulationModel.cs b/src/Umbraco.Core/Models/UsersToUserGroupManipulationModel.cs index 370d598d73..375d6fcdb1 100644 --- a/src/Umbraco.Core/Models/UsersToUserGroupManipulationModel.cs +++ b/src/Umbraco.Core/Models/UsersToUserGroupManipulationModel.cs @@ -1,10 +1,9 @@ -using Umbraco.Cms.Core.Models.Membership; - namespace Umbraco.Cms.Core.Models; public class UsersToUserGroupManipulationModel { public Guid UserGroupKey { get; init; } + public Guid[] UserKeys { get; init; } public UsersToUserGroupManipulationModel(Guid userGroupKey, Guid[] userKeys) diff --git a/src/Umbraco.Core/Services/IUserGroupService.cs b/src/Umbraco.Core/Services/IUserGroupService.cs index 4a2255ea79..c597b9ead3 100644 --- a/src/Umbraco.Core/Services/IUserGroupService.cs +++ b/src/Umbraco.Core/Services/IUserGroupService.cs @@ -94,7 +94,7 @@ public interface IUserGroupService /// The user groups the users should be part of. /// The user whose groups we want to alter. /// An attempt indicating if the operation was a success as well as a more detailed . - Task UpdateUserGroupsOnUsers(ISet userGroupKeys, ISet userKeys); + Task UpdateUserGroupsOnUsersAsync(ISet userGroupKeys, ISet userKeys); Task AddUsersToUserGroupAsync(UsersToUserGroupManipulationModel addUsersModel, Guid performingUserKey); Task RemoveUsersFromUserGroupAsync(UsersToUserGroupManipulationModel removeUsersModel, Guid performingUserKey); diff --git a/src/Umbraco.Core/Services/OperationStatus/UserGroupOperationStatus.cs b/src/Umbraco.Core/Services/OperationStatus/UserGroupOperationStatus.cs index 0cbfcf2292..b086b36e49 100644 --- a/src/Umbraco.Core/Services/OperationStatus/UserGroupOperationStatus.cs +++ b/src/Umbraco.Core/Services/OperationStatus/UserGroupOperationStatus.cs @@ -22,5 +22,7 @@ public enum UserGroupOperationStatus UnauthorizedMissingContentStartNodeAccess, UnauthorizedMissingMediaStartNodeAccess, UnauthorizedMissingUserGroupAccess, - UnauthorizedMissingUsersSectionAccess + UnauthorizedMissingUsersSectionAccess, + AdminGroupCannotBeEmpty, + UserNotInGroup, } diff --git a/src/Umbraco.Core/Services/OperationStatus/UserOperationStatus.cs b/src/Umbraco.Core/Services/OperationStatus/UserOperationStatus.cs index 122f7fa792..c26398b8c3 100644 --- a/src/Umbraco.Core/Services/OperationStatus/UserOperationStatus.cs +++ b/src/Umbraco.Core/Services/OperationStatus/UserOperationStatus.cs @@ -11,6 +11,7 @@ public enum UserOperationStatus UserNameIsNotEmail, EmailCannotBeChanged, NoUserGroup, + AdminUserGroupMustNotBeEmpty, DuplicateUserName, InvalidEmail, DuplicateEmail, diff --git a/src/Umbraco.Core/Services/UserGroupService.cs b/src/Umbraco.Core/Services/UserGroupService.cs index 03431f755e..54001d7e57 100644 --- a/src/Umbraco.Core/Services/UserGroupService.cs +++ b/src/Umbraco.Core/Services/UserGroupService.cs @@ -178,16 +178,34 @@ internal sealed class UserGroupService : RepositoryService, IUserGroupService return Attempt.Succeed(UserGroupOperationStatus.Success); } - public async Task UpdateUserGroupsOnUsers( + public async Task UpdateUserGroupsOnUsersAsync( ISet userGroupKeys, ISet userKeys) { + using ICoreScope scope = ScopeProvider.CreateCoreScope(); + IUser[] users = (await _userService.GetAsync(userKeys)).ToArray(); IReadOnlyUserGroup[] userGroups = (await GetAsync(userGroupKeys)) .Select(x => x.ToReadOnlyGroup()) .ToArray(); + // This means that we're potentially de-admining a user, which might cause the admin group to be empty. + if (userGroupKeys.Contains(Constants.Security.AdminGroupKey) is false) + { + IUser[] usersToDeAdmin = users.Where(x => x.IsAdmin()).ToArray(); + if (usersToDeAdmin.Length > 0) + { + // Unfortunately we have to resolve the admin group to ensure that it would not be left empty. + IUserGroup? adminGroup = await GetAsync(Constants.Security.AdminGroupKey); + if (adminGroup is not null && adminGroup.UserCount <= usersToDeAdmin.Length) + { + scope.Complete(); + return UserGroupOperationStatus.AdminGroupCannotBeEmpty; + } + } + } + foreach (IUser user in users) { user.ClearGroups(); @@ -198,6 +216,10 @@ internal sealed class UserGroupService : RepositoryService, IUserGroupService } _userService.Save(users); + + scope.Complete(); + + return UserGroupOperationStatus.Success; } private Attempt ValidateUserGroupDeletion(IUserGroup? userGroup) @@ -345,68 +367,114 @@ internal sealed class UserGroupService : RepositoryService, IUserGroupService { using ICoreScope scope = ScopeProvider.CreateCoreScope(); - UserGroupOperationStatus result = await SafelyManipulateUsersBasedOnGroupAsync(addUsersModel, performingUserKey, (users, group) => - { - IReadOnlyUserGroup readOnlyGroup = group.ToReadOnlyGroup(); + Attempt resolveAttempt = await ResolveUserGroupManipulationModel(addUsersModel, performingUserKey); - foreach (IUser user in users) - { - user.AddGroup(readOnlyGroup); - } - }); + if (resolveAttempt.Success is false) + { + return resolveAttempt.Status; + } + + ResolvedUserToUserGroupManipulationModel? resolvedModel = resolveAttempt.Result; + + // This should never happen, but we need to check it to avoid null reference exceptions + if (resolvedModel is null) + { + throw new InvalidOperationException("The resolved model should not be null."); + } + + IReadOnlyUserGroup readOnlyGroup = resolvedModel.UserGroup.ToReadOnlyGroup(); + + foreach (IUser user in resolvedModel.Users) + { + user.AddGroup(readOnlyGroup); + } + + _userService.Save(resolvedModel.Users); scope.Complete(); - return result; + + return UserGroupOperationStatus.Success; } public async Task RemoveUsersFromUserGroupAsync(UsersToUserGroupManipulationModel removeUsersModel, Guid performingUserKey) { using ICoreScope scope = ScopeProvider.CreateCoreScope(); - UserGroupOperationStatus result = await SafelyManipulateUsersBasedOnGroupAsync(removeUsersModel, performingUserKey, (users, group) => + Attempt resolveAttempt = await ResolveUserGroupManipulationModel(removeUsersModel, performingUserKey); + + if (resolveAttempt.Success is false) { - foreach (IUser user in users) + return resolveAttempt.Status; + } + + ResolvedUserToUserGroupManipulationModel? resolvedModel = resolveAttempt.Result; + + // This should never happen, but we need to check it to avoid null reference exceptions + if (resolvedModel is null) + { + throw new InvalidOperationException("The resolved model should not be null."); + } + + foreach (IUser user in resolvedModel.Users) + { + // We can't remove a user from a group they're not part of. + if (user.Groups.Select(x => x.Key).Contains(resolvedModel.UserGroup.Key) is false) { - user.RemoveGroup(group.Alias); + return UserGroupOperationStatus.UserNotInGroup; } - }); + + user.RemoveGroup(resolvedModel.UserGroup.Alias); + } + + // Ensure that that the admin group is never empty. + // This would mean that you could never add a user to the admin group again, since you need to be part of the admin group to do so. + if (resolvedModel.UserGroup.Key == Constants.Security.AdminGroupKey + && resolvedModel.UserGroup.UserCount <= resolvedModel.Users.Length) + { + return UserGroupOperationStatus.AdminGroupCannotBeEmpty; + } + + _userService.Save(resolvedModel.Users); scope.Complete(); - return result; + + return UserGroupOperationStatus.Success; } /// - /// Checks whether all users that are part of the manipulation exist, - /// performs the manipulation, - /// saves the users + /// Resolves the user group manipulation model keys into actual entities. + /// Checks whether the performing user exists. + /// Checks whether all users that are part of the manipulation exist. /// - private async Task SafelyManipulateUsersBasedOnGroupAsync(UsersToUserGroupManipulationModel assignModel, Guid performingUserKey, Action manipulation) + private async Task> ResolveUserGroupManipulationModel(UsersToUserGroupManipulationModel model, Guid performingUserKey) { IUser? performingUser = await _userService.GetAsync(performingUserKey); if (performingUser is null) { - return UserGroupOperationStatus.MissingUser; + return Attempt.FailWithStatus(UserGroupOperationStatus.MissingUser, null); } - IUserGroup? existingUserGroup = await GetAsync(assignModel.UserGroupKey); + IUserGroup? existingUserGroup = await GetAsync(model.UserGroupKey); if (existingUserGroup is null) { - return UserGroupOperationStatus.NotFound; + return Attempt.FailWithStatus(UserGroupOperationStatus.NotFound, null); } - IUser[] users = (await _userService.GetAsync(assignModel.UserKeys)).ToArray(); + IUser[] users = (await _userService.GetAsync(model.UserKeys)).ToArray(); - if (users.Length != assignModel.UserKeys.Length) + if (users.Length != model.UserKeys.Length) { - return UserGroupOperationStatus.UserNotFound; + return Attempt.FailWithStatus(UserGroupOperationStatus.UserNotFound, null); } - manipulation(users, existingUserGroup); + var resolvedModel = new ResolvedUserToUserGroupManipulationModel + { + UserGroup = existingUserGroup, + Users = users, + }; - _userService.Save(users); - - return UserGroupOperationStatus.Success; + return Attempt.SucceedWithStatus(UserGroupOperationStatus.Success, resolvedModel); } private async Task ValidateUserGroupUpdateAsync(IUserGroup userGroup) diff --git a/src/Umbraco.Core/Services/UserService.cs b/src/Umbraco.Core/Services/UserService.cs index b0e0828b58..00d158590a 100644 --- a/src/Umbraco.Core/Services/UserService.cs +++ b/src/Umbraco.Core/Services/UserService.cs @@ -909,22 +909,37 @@ internal class UserService : RepositoryService, IUserService if (performingUser is null) { + scope.Complete(); return Attempt.FailWithStatus(UserOperationStatus.MissingUser, existingUser); } - var userGroups = _userGroupRepository.GetMany().Where(x=>model.UserGroupKeys.Contains(x.Key)).ToHashSet(); + IEnumerable allUserGroups = _userGroupRepository.GetMany().ToArray(); + var userGroups = allUserGroups.Where(x => model.UserGroupKeys.Contains(x.Key)).ToHashSet(); if (userGroups.Count != model.UserGroupKeys.Count) { + scope.Complete(); return Attempt.FailWithStatus(UserOperationStatus.MissingUserGroup, existingUser); } + // We're de-admining a user, we need to ensure that this would not leave the admin group empty. + if (existingUser.IsAdmin() && model.UserGroupKeys.Contains(Constants.Security.AdminGroupKey) is false) + { + IUserGroup? adminGroup = allUserGroups.FirstOrDefault(x => x.Key == Constants.Security.AdminGroupKey); + if (adminGroup?.UserCount == 1) + { + scope.Complete(); + return Attempt.FailWithStatus(UserOperationStatus.AdminUserGroupMustNotBeEmpty, existingUser); + } + } + // 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); if (startContentIds is null || startContentIds.Length != model.ContentStartNodeKeys.Count) { + scope.Complete(); return Attempt.FailWithStatus(UserOperationStatus.ContentStartNodeNotFound, existingUser); } @@ -932,6 +947,7 @@ internal class UserService : RepositoryService, IUserService if (startMediaIds is null || startMediaIds.Length != model.MediaStartNodeKeys.Count) { + scope.Complete(); return Attempt.FailWithStatus(UserOperationStatus.MediaStartNodeNotFound, existingUser); } @@ -944,12 +960,14 @@ internal class UserService : RepositoryService, IUserService if (isAuthorized.Success is false) { + scope.Complete(); return Attempt.FailWithStatus(UserOperationStatus.Unauthorized, existingUser); } UserOperationStatus validationStatus = ValidateUserUpdateModel(existingUser, model); if (validationStatus is not UserOperationStatus.Success) { + scope.Complete(); return Attempt.FailWithStatus(validationStatus, existingUser); } @@ -1055,7 +1073,7 @@ internal class UserService : RepositoryService, IUserService return UserOperationStatus.UserNameIsNotEmail; } - if (!IsEmailValid(model.Email)) + if (IsEmailValid(model.Email) is false) { return UserOperationStatus.InvalidEmail; }