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