yv01p/Umbraco-CMS
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Permissions: Fix removal of check on removal the final admin user (closes #19917) (#19921)

Browse Source 
Reworks update of user groups on a user by updating in place rather than deleting and re-adding.
Ensure user groups affected by the update are invalidated in the repository cache.

Co-authored-by: Kenn Jacobsen <kja@umbraco.dk>
This commit is contained in:
Andy Butland
2025-09-22 12:51:27 +02:00
committed by GitHub
parent 79de4e3871
commit d7d39658e0
3 changed files with 138 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files

85
src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs
View File

@@ -671,6 +671,15 @@ SELECT 4 AS [Key], COUNT(id) AS [Value] FROM umbracoUser WHERE userDisabled = 0
protected override void PersistDeletedItem(IUser entity)
{
// Clear user group caches for any user groups associated with the deleted user.
// We need to do this because the count of the number of users in the user group is cached
// along with the user group, and if we've made changes to the user groups assigned to the user,
// the count for the groups need to be refreshed.
foreach (IReadOnlyUserGroup group in entity.Groups)
{
ClearRepositoryCacheForUserGroup(group.Id);
}
IEnumerable<string> deletes = GetDeleteClauses();
foreach (var delete in deletes)
{
@@ -713,8 +722,8 @@ SELECT 4 AS [Key], COUNT(id) AS [Value] FROM umbracoUser WHERE userDisabled = 0
if (entity.IsPropertyDirty("Groups"))
{
// lookup all assigned
List<UserGroupDto>? assigned = entity.Groups == null || entity.Groups.Any() == false
? new List<UserGroupDto>()
List<UserGroupDto>? assigned = entity.Groups.Any() is false
? []
: Database.Fetch<UserGroupDto>(
"SELECT * FROM umbracoUserGroup WHERE userGroupAlias IN (@aliases)",
new { aliases = entity.Groups.Select(x => x.Alias) });
@@ -724,6 +733,15 @@ SELECT 4 AS [Key], COUNT(id) AS [Value] FROM umbracoUser WHERE userDisabled = 0
var dto = new User2UserGroupDto { UserGroupId = groupDto.Id, UserId = entity.Id };
Database.Insert(dto);
}
// Clear user group caches for the user groups associated with the new user.
// We need to do this because the count of the number of users in the user group is cached
// along with the user group, and if we've made changes to the user groups assigned to the user,
// the count for the groups need to be refreshed.
foreach (IReadOnlyUserGroup group in entity.Groups)
{
ClearRepositoryCacheForUserGroup(group.Id);
}
}
entity.ResetDirtyProperties();
@@ -836,27 +854,66 @@ SELECT 4 AS [Key], COUNT(id) AS [Value] FROM umbracoUser WHERE userDisabled = 0
if (entity.IsPropertyDirty("Groups"))
{
//lookup all assigned
List<UserGroupDto>? assigned = entity.Groups == null || entity.Groups.Any() == false
? new List<UserGroupDto>()
: Database.Fetch<UserGroupDto>(
"SELECT * FROM umbracoUserGroup WHERE userGroupAlias IN (@aliases)",
new { aliases = entity.Groups.Select(x => x.Alias) });
// Get all user groups Ids currently assigned to the user.
var existingUserGroupIds = Database.Fetch<User2UserGroupDto>(
"WHERE UserId = @UserId",
new { UserId = entity.Id })
.Select(x => x.UserGroupId)
.ToList();
//first delete all
// TODO: We could do this a nicer way instead of "Nuke and Pave"
Database.Delete<User2UserGroupDto>("WHERE UserId = @UserId", new { UserId = entity.Id });
// Get the user groups Ids that need to be removed and added.
var userGroupsIdsToRemove = existingUserGroupIds
.Except(entity.Groups.Select(x => x.Id))
.ToList();
var userGroupIdsToAdd = entity.Groups
.Select(x => x.Id)
.Except(existingUserGroupIds)
.ToList();
foreach (UserGroupDto? groupDto in assigned)
// Remove user groups that are no longer assigned to the user.
if (userGroupsIdsToRemove.Count > 0)
{
var dto = new User2UserGroupDto { UserGroupId = groupDto.Id, UserId = entity.Id };
Database.Insert(dto);
Database.Delete<User2UserGroupDto>(
"WHERE UserId = @UserId AND UserGroupId IN (@userGroupIds)",
new { UserId = entity.Id, userGroupIds = userGroupsIdsToRemove });
}
// Add user groups that are newly assigned to the user.
if (userGroupIdsToAdd.Count > 0)
{
IEnumerable<User2UserGroupDto> user2UserGroupDtos = userGroupIdsToAdd
.Select(userGroupId => new User2UserGroupDto
{
UserGroupId = userGroupId,
UserId = entity.Id,
});
Database.InsertBulk(user2UserGroupDtos);
}
// Clear user group caches for any user group that have been removed or added.
// We need to do this because the count of the number of users in the user group is cached
// along with the user group, and if we've made changes to the user groups assigned to the user,
// the count for the groups need to be refreshed.
var userGroupIdsToRefresh = userGroupsIdsToRemove
.Union(userGroupIdsToAdd)
.ToList();
foreach (int userGroupIdToRefresh in userGroupIdsToRefresh)
{
ClearRepositoryCacheForUserGroup(userGroupIdToRefresh);
}
}
entity.ResetDirtyProperties();
}
private void ClearRepositoryCacheForUserGroup(int id)
{
IAppPolicyCache userGroupCache = AppCaches.IsolatedCaches.GetOrCreate<IUserGroup>();
string cacheKey = RepositoryCacheKeys.GetKey<IUserGroup, int>(id);
userGroupCache.Clear(cacheKey);
}
private void AddingOrUpdateStartNodes(IEntity entity, IEnumerable<UserStartNodeDto> current,
UserStartNodeDto.StartNodeTypeValue startNodeType, int[]? entityStartIds)
{

31
tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserServiceCrudTests.Update.cs
View File

@@ -398,4 +398,35 @@ internal sealed partial class UserServiceCrudTests
Assert.IsNotNull(updatedUser.StartMediaIds);
Assert.IsEmpty(updatedUser.StartMediaIds);
}
[TestCase(false, false)]
[TestCase(true, true)]
public async Task Cannot_Remove_Admin_Group_From_Only_Admin_User(bool createAdditionalAdminUser, bool expectSuccess)
{
var userService = CreateUserService(securitySettings: new SecuritySettings { UsernameIsEmail = false });
if (createAdditionalAdminUser)
{
var (updateModel, _) = await CreateUserForUpdate(userService);
updateModel.UserGroupKeys = new HashSet<Guid> { Constants.Security.AdminGroupKey };
var updateResult = await userService.UpdateAsync(Constants.Security.SuperUserKey, updateModel);
Assert.IsTrue(updateResult.Success);
}
var adminUser = await userService.GetAsync(Constants.Security.SuperUserKey);
var adminUserUpdateModel = await MapUserToUpdateModel(adminUser);
adminUserUpdateModel.Email = "admin@test.com";
adminUserUpdateModel.UserGroupKeys = new HashSet<Guid> { Constants.Security.EditorGroupKey };
var adminUserUpdateResult = await userService.UpdateAsync(Constants.Security.SuperUserKey, adminUserUpdateModel);
if (expectSuccess)
{
Assert.IsTrue(adminUserUpdateResult.Success);
}
else
{
Assert.IsFalse(adminUserUpdateResult.Success);
Assert.AreEqual(UserOperationStatus.AdminUserGroupMustNotBeEmpty, adminUserUpdateResult.Status);
}
}
}

36
tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/UserServiceTests.cs
View File

@@ -1019,6 +1019,42 @@ internal sealed class UserServiceTests : UmbracoIntegrationTest
}
}
[Test]
public async Task Can_Assign_And_Get_Groups_For_User()
{
// Arrange
var (user, userGroup1) = await CreateTestUserAndGroup();
var userGroup2 = await CreateTestUserGroup("testGroup2", "Test Group 2");
// Act & Assert
user = UserService.GetByUsername(user.Username);
Assert.IsNotNull(user);
Assert.AreEqual(1, user.Groups.Count());
Assert.AreEqual(userGroup1.Alias, user.Groups.First().Alias);
// - add second group
user.AddGroup(userGroup2);
UserService.Save(user);
user = UserService.GetByUsername(user.Username);
Assert.AreEqual(2, user.Groups.Count());
// - remove first group
user.RemoveGroup(userGroup1.Alias);
UserService.Save(user);
user = UserService.GetByUsername(user.Username);
Assert.AreEqual(1, user.Groups.Count());
Assert.AreEqual(userGroup2.Alias, user.Groups.First().Alias);
// - remove second group and add first
user.RemoveGroup(userGroup2.Alias);
user.AddGroup(userGroup1.ToReadOnlyGroup());
UserService.Save(user);
user = UserService.GetByUsername(user.Username);
Assert.AreEqual(1, user.Groups.Count());
Assert.AreEqual(userGroup1.Alias, user.Groups.First().Alias);
}
[TestCase(UserKind.Default, UserClientCredentialsOperationStatus.InvalidUser)]
[TestCase(UserKind.Api, UserClientCredentialsOperationStatus.Success)]
public async Task Can_Assign_ClientId_To_Api_User(UserKind userKind, UserClientCredentialsOperationStatus expectedResult)