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

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);
     }
 }

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")

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();
         }

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.")

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; }
+}

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)

src/Umbraco.Core/Services/IUserGroupService.cs

@@ -94,7 +94,7 @@ public interface IUserGroupService
     /// <param name="userGroupKeys">The user groups the users should be part of.</param>
     /// <param name="userKeys">The user whose groups we want to alter.</param>
     /// <returns>An attempt indicating if the operation was a success as well as a more detailed <see cref="UserGroupOperationStatus"/>.</returns>
-    Task UpdateUserGroupsOnUsers(ISet<Guid> userGroupKeys, ISet<Guid> userKeys);
+    Task<UserGroupOperationStatus> UpdateUserGroupsOnUsersAsync(ISet<Guid> userGroupKeys, ISet<Guid> userKeys);
 
     Task<UserGroupOperationStatus> AddUsersToUserGroupAsync(UsersToUserGroupManipulationModel addUsersModel, Guid performingUserKey);
     Task<UserGroupOperationStatus> RemoveUsersFromUserGroupAsync(UsersToUserGroupManipulationModel removeUsersModel, Guid performingUserKey);

src/Umbraco.Core/Services/OperationStatus/UserGroupOperationStatus.cs

@@ -22,5 +22,7 @@ public enum UserGroupOperationStatus
     UnauthorizedMissingContentStartNodeAccess,
     UnauthorizedMissingMediaStartNodeAccess,
     UnauthorizedMissingUserGroupAccess,
-    UnauthorizedMissingUsersSectionAccess
+    UnauthorizedMissingUsersSectionAccess,
+    AdminGroupCannotBeEmpty,
+    UserNotInGroup,
 }

src/Umbraco.Core/Services/OperationStatus/UserOperationStatus.cs

@@ -11,6 +11,7 @@ public enum UserOperationStatus
     UserNameIsNotEmail,
     EmailCannotBeChanged,
     NoUserGroup,
+    AdminUserGroupMustNotBeEmpty,
     DuplicateUserName,
     InvalidEmail,
     DuplicateEmail,

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<UserGroupOperationStatus> UpdateUserGroupsOnUsersAsync(
         ISet<Guid> userGroupKeys,
         ISet<Guid> 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<UserGroupOperationStatus> 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<ResolvedUserToUserGroupManipulationModel?, UserGroupOperationStatus> 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<UserGroupOperationStatus> RemoveUsersFromUserGroupAsync(UsersToUserGroupManipulationModel removeUsersModel, Guid performingUserKey)
     {
         using ICoreScope scope = ScopeProvider.CreateCoreScope();
 
-        UserGroupOperationStatus result = await SafelyManipulateUsersBasedOnGroupAsync(removeUsersModel, performingUserKey, (users, group) =>
+        Attempt<ResolvedUserToUserGroupManipulationModel?, UserGroupOperationStatus> 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;
     }
 
     /// <summary>
-    /// 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.
     /// </summary>
-    private async Task<UserGroupOperationStatus> SafelyManipulateUsersBasedOnGroupAsync(UsersToUserGroupManipulationModel assignModel, Guid performingUserKey, Action<IUser[], IUserGroup> manipulation)
+    private async Task<Attempt<ResolvedUserToUserGroupManipulationModel?, UserGroupOperationStatus>> ResolveUserGroupManipulationModel(UsersToUserGroupManipulationModel model, Guid performingUserKey)
     {
         IUser? performingUser = await _userService.GetAsync(performingUserKey);
         if (performingUser is null)
         {
-            return UserGroupOperationStatus.MissingUser;
+            return Attempt.FailWithStatus<ResolvedUserToUserGroupManipulationModel?, UserGroupOperationStatus>(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<ResolvedUserToUserGroupManipulationModel?, UserGroupOperationStatus>(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<ResolvedUserToUserGroupManipulationModel?, UserGroupOperationStatus>(UserGroupOperationStatus.UserNotFound, null);
         }
 
-        manipulation(users, existingUserGroup);
+        var resolvedModel = new ResolvedUserToUserGroupManipulationModel
+        {
+            UserGroup = existingUserGroup,
+            Users = users,
+        };
 
-        _userService.Save(users);
-
-        return UserGroupOperationStatus.Success;
+        return Attempt.SucceedWithStatus<ResolvedUserToUserGroupManipulationModel?, UserGroupOperationStatus>(UserGroupOperationStatus.Success, resolvedModel);
     }
 
     private async Task<UserGroupOperationStatus> ValidateUserGroupUpdateAsync(IUserGroup userGroup)

src/Umbraco.Core/Services/UserService.cs

@@ -909,22 +909,37 @@ internal class UserService : RepositoryService, IUserService
 
         if (performingUser is null)
         {
+            scope.Complete();
             return Attempt.FailWithStatus<IUser?, UserOperationStatus>(UserOperationStatus.MissingUser, existingUser);
         }
 
-        var userGroups = _userGroupRepository.GetMany().Where(x=>model.UserGroupKeys.Contains(x.Key)).ToHashSet();
+        IEnumerable<IUserGroup> 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<IUser?, UserOperationStatus>(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<IUser?, UserOperationStatus>(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<IUser?, UserOperationStatus>(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<IUser?, UserOperationStatus>(UserOperationStatus.MediaStartNodeNotFound, existingUser);
         }
 
@@ -944,12 +960,14 @@ internal class UserService : RepositoryService, IUserService
 
         if (isAuthorized.Success is false)
         {
+            scope.Complete();
             return Attempt.FailWithStatus<IUser?, UserOperationStatus>(UserOperationStatus.Unauthorized, existingUser);
         }
 
         UserOperationStatus validationStatus = ValidateUserUpdateModel(existingUser, model);
         if (validationStatus is not UserOperationStatus.Success)
         {
+            scope.Complete();
             return Attempt.FailWithStatus<IUser?, UserOperationStatus>(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;
         }