From 4a1212095053bb93e936038a71f44b01ebbb3c36 Mon Sep 17 00:00:00 2001 From: Jimmy Vo Date: Fri, 13 Jun 2025 16:27:48 -0400 Subject: [PATCH] [PM-19703] Fix admin count logic to exclude current organization (#5918) --- .../OrganizationUsersController.cs | 4 +- .../Public/Controllers/MembersController.cs | 3 +- .../IUpdateOrganizationUserCommand.cs | 3 +- .../UpdateOrganizationUserCommand.cs | 48 +++++++++++---- .../OrganizationUserControllerPutTests.cs | 48 ++++++++++----- .../UpdateOrganizationUserCommandTests.cs | 60 +++++++++++++++---- 6 files changed, 126 insertions(+), 40 deletions(-) diff --git a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs index 6b23edf347..1b58fcfebe 100644 --- a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs +++ b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs @@ -521,7 +521,9 @@ public class OrganizationUsersController : Controller .Concat(readonlyCollectionAccess) .ToList(); - await _updateOrganizationUserCommand.UpdateUserAsync(model.ToOrganizationUser(organizationUser), userId, + var existingUserType = organizationUser.Type; + + await _updateOrganizationUserCommand.UpdateUserAsync(model.ToOrganizationUser(organizationUser), existingUserType, userId, collectionsToSave, groupsToSave); } diff --git a/src/Api/AdminConsole/Public/Controllers/MembersController.cs b/src/Api/AdminConsole/Public/Controllers/MembersController.cs index 6552684ca3..90c78c9eb7 100644 --- a/src/Api/AdminConsole/Public/Controllers/MembersController.cs +++ b/src/Api/AdminConsole/Public/Controllers/MembersController.cs @@ -177,9 +177,10 @@ public class MembersController : Controller { return new NotFoundResult(); } + var existingUserType = existingUser.Type; var updatedUser = model.ToOrganizationUser(existingUser); var associations = model.Collections?.Select(c => c.ToCollectionAccessSelection()).ToList(); - await _updateOrganizationUserCommand.UpdateUserAsync(updatedUser, null, associations, model.Groups); + await _updateOrganizationUserCommand.UpdateUserAsync(updatedUser, existingUserType, null, associations, model.Groups); MemberResponseModel response = null; if (existingUser.UserId.HasValue) { diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IUpdateOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IUpdateOrganizationUserCommand.cs index 0cd5a3295f..d1fc9fbbec 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IUpdateOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IUpdateOrganizationUserCommand.cs @@ -1,11 +1,12 @@ #nullable enable using Bit.Core.Entities; +using Bit.Core.Enums; using Bit.Core.Models.Data; namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; public interface IUpdateOrganizationUserCommand { - Task UpdateUserAsync(OrganizationUser organizationUser, Guid? savingUserId, + Task UpdateUserAsync(OrganizationUser organizationUser, OrganizationUserType existingUserType, Guid? savingUserId, List? collectionAccess, IEnumerable? groupAccess); } diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs index bad7b14b87..8d1e693e8b 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs @@ -55,11 +55,13 @@ public class UpdateOrganizationUserCommand : IUpdateOrganizationUserCommand /// Update an organization user. /// /// The modified organization user to save. + /// The current type (member role) of the user. /// The userId of the currently logged in user who is making the change. /// The user's updated collection access. If set to null, this removes all collection access. /// The user's updated group access. If set to null, groups are not updated. /// - public async Task UpdateUserAsync(OrganizationUser organizationUser, Guid? savingUserId, + public async Task UpdateUserAsync(OrganizationUser organizationUser, OrganizationUserType existingUserType, + Guid? savingUserId, List? collectionAccess, IEnumerable? groupAccess) { // Avoid multiple enumeration @@ -83,15 +85,7 @@ public class UpdateOrganizationUserCommand : IUpdateOrganizationUserCommand throw new NotFoundException(); } - if (organizationUser.UserId.HasValue && organization.PlanType == PlanType.Free && organizationUser.Type is OrganizationUserType.Admin or OrganizationUserType.Owner) - { - // Since free organizations only supports a few users there is not much point in avoiding N+1 queries for this. - var adminCount = await _organizationUserRepository.GetCountByFreeOrganizationAdminUserAsync(organizationUser.UserId.Value); - if (adminCount > 0) - { - throw new BadRequestException("User can only be an admin of one free organization."); - } - } + await EnsureUserCannotBeAdminOrOwnerForMultipleFreeOrganizationAsync(organizationUser, existingUserType, organization); if (collectionAccessList.Count != 0) { @@ -151,6 +145,40 @@ public class UpdateOrganizationUserCommand : IUpdateOrganizationUserCommand await _eventService.LogOrganizationUserEventAsync(organizationUser, EventType.OrganizationUser_Updated); } + private async Task EnsureUserCannotBeAdminOrOwnerForMultipleFreeOrganizationAsync(OrganizationUser updatedOrgUser, OrganizationUserType existingUserType, Entities.Organization organization) + { + + if (organization.PlanType != PlanType.Free) + { + return; + } + if (!updatedOrgUser.UserId.HasValue) + { + return; + } + if (updatedOrgUser.Type is not (OrganizationUserType.Admin or OrganizationUserType.Owner)) + { + return; + } + + // Since free organizations only supports a few users there is not much point in avoiding N+1 queries for this. + var adminCount = await _organizationUserRepository.GetCountByFreeOrganizationAdminUserAsync(updatedOrgUser.UserId!.Value); + + var isCurrentAdminOrOwner = existingUserType is OrganizationUserType.Admin or OrganizationUserType.Owner; + + if (isCurrentAdminOrOwner && adminCount <= 1) + { + return; + } + + if (!isCurrentAdminOrOwner && adminCount == 0) + { + return; + } + + throw new BadRequestException("User can only be an admin of one free organization."); + } + private async Task ValidateCollectionAccessAsync(OrganizationUser originalUser, ICollection collectionAccess) { diff --git a/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs b/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs index 542a76090c..e5fb3fe541 100644 --- a/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs @@ -30,6 +30,7 @@ public class OrganizationUserControllerPutTests OrganizationUser organizationUser, OrganizationAbility organizationAbility, SutProvider sutProvider, Guid savingUserId) { + // Arrange Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, currentCollectionAccess: []); // Authorize all changes for basic happy path test @@ -41,15 +42,18 @@ public class OrganizationUserControllerPutTests // Save these for later - organizationUser object will be mutated var orgUserId = organizationUser.Id; var orgUserEmail = organizationUser.Email; + var existingUserType = organizationUser.Type; + // Act await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); + // Assert await sutProvider.GetDependency().Received(1).UpdateUserAsync(Arg.Is(ou => ou.Type == model.Type && ou.Permissions == CoreHelpers.ClassToJsonData(model.Permissions) && ou.AccessSecretsManager == model.AccessSecretsManager && ou.Id == orgUserId && - ou.Email == orgUserEmail), + ou.Email == orgUserEmail), existingUserType, savingUserId, Arg.Is>(cas => cas.All(c => model.Collections.Any(m => m.Id == c.Id))), @@ -77,6 +81,7 @@ public class OrganizationUserControllerPutTests OrganizationUser organizationUser, OrganizationAbility organizationAbility, SutProvider sutProvider, Guid savingUserId) { + // Arrange // Updating self organizationUser.UserId = savingUserId; organizationAbility.AllowAdminAccessToAllCollectionItems = false; @@ -88,15 +93,18 @@ public class OrganizationUserControllerPutTests var orgUserId = organizationUser.Id; var orgUserEmail = organizationUser.Email; + var existingUserType = organizationUser.Type; + // Act await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); + // Assert await sutProvider.GetDependency().Received(1).UpdateUserAsync(Arg.Is(ou => - ou.Type == model.Type && - ou.Permissions == CoreHelpers.ClassToJsonData(model.Permissions) && - ou.AccessSecretsManager == model.AccessSecretsManager && - ou.Id == orgUserId && - ou.Email == orgUserEmail), + ou.Type == model.Type && + ou.Permissions == CoreHelpers.ClassToJsonData(model.Permissions) && + ou.AccessSecretsManager == model.AccessSecretsManager && + ou.Id == orgUserId && + ou.Email == orgUserEmail), existingUserType, savingUserId, Arg.Is>(cas => cas.All(c => model.Collections.Any(m => m.Id == c.Id))), @@ -110,6 +118,7 @@ public class OrganizationUserControllerPutTests OrganizationUser organizationUser, OrganizationAbility organizationAbility, SutProvider sutProvider, Guid savingUserId) { + // Arrange // Updating self organizationUser.UserId = savingUserId; organizationAbility.AllowAdminAccessToAllCollectionItems = true; @@ -121,15 +130,18 @@ public class OrganizationUserControllerPutTests var orgUserId = organizationUser.Id; var orgUserEmail = organizationUser.Email; + var existingUserType = organizationUser.Type; + // Act await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); + // Assert await sutProvider.GetDependency().Received(1).UpdateUserAsync(Arg.Is(ou => - ou.Type == model.Type && - ou.Permissions == CoreHelpers.ClassToJsonData(model.Permissions) && - ou.AccessSecretsManager == model.AccessSecretsManager && - ou.Id == orgUserId && - ou.Email == orgUserEmail), + ou.Type == model.Type && + ou.Permissions == CoreHelpers.ClassToJsonData(model.Permissions) && + ou.AccessSecretsManager == model.AccessSecretsManager && + ou.Id == orgUserId && + ou.Email == orgUserEmail), existingUserType, savingUserId, Arg.Is>(cas => cas.All(c => model.Collections.Any(m => m.Id == c.Id))), @@ -142,6 +154,7 @@ public class OrganizationUserControllerPutTests OrganizationUser organizationUser, OrganizationAbility organizationAbility, SutProvider sutProvider, Guid savingUserId) { + // Arrange var editedCollectionId = CoreHelpers.GenerateComb(); var readonlyCollectionId1 = CoreHelpers.GenerateComb(); var readonlyCollectionId2 = CoreHelpers.GenerateComb(); @@ -194,16 +207,19 @@ public class OrganizationUserControllerPutTests .AuthorizeAsync(Arg.Any(), Arg.Is(c => c.Id == readonlyCollectionId1 || c.Id == readonlyCollectionId2), Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) .Returns(AuthorizationResult.Failed()); + var existingUserType = organizationUser.Type; + // Act await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); + // Assert // Expect all collection access (modified and unmodified) to be saved await sutProvider.GetDependency().Received(1).UpdateUserAsync(Arg.Is(ou => - ou.Type == model.Type && - ou.Permissions == CoreHelpers.ClassToJsonData(model.Permissions) && - ou.AccessSecretsManager == model.AccessSecretsManager && - ou.Id == orgUserId && - ou.Email == orgUserEmail), + ou.Type == model.Type && + ou.Permissions == CoreHelpers.ClassToJsonData(model.Permissions) && + ou.AccessSecretsManager == model.AccessSecretsManager && + ou.Id == orgUserId && + ou.Email == orgUserEmail), existingUserType, savingUserId, Arg.Is>(cas => cas.Select(c => c.Id).SequenceEqual(currentCollectionAccess.Select(c => c.Id)) && diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs index cd03f9583b..5c07ce0347 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs @@ -27,8 +27,10 @@ public class UpdateOrganizationUserCommandTests List collections, List groups, SutProvider sutProvider) { user.Id = default(Guid); + var existingUserType = OrganizationUserType.User; + var exception = await Assert.ThrowsAsync( - () => sutProvider.Sut.UpdateUserAsync(user, savingUserId, collections, groups)); + () => sutProvider.Sut.UpdateUserAsync(user, existingUserType, savingUserId, collections, groups)); Assert.Contains("invite the user first", exception.Message.ToLowerInvariant()); } @@ -37,9 +39,10 @@ public class UpdateOrganizationUserCommandTests Guid? savingUserId, SutProvider sutProvider) { sutProvider.GetDependency().GetByIdAsync(user.Id).Returns(originalUser); + var existingUserType = OrganizationUserType.User; await Assert.ThrowsAsync( - () => sutProvider.Sut.UpdateUserAsync(user, savingUserId, null, null)); + () => sutProvider.Sut.UpdateUserAsync(user, existingUserType, savingUserId, null, null)); } [Theory, BitAutoData] @@ -55,8 +58,10 @@ public class UpdateOrganizationUserCommandTests .Returns(callInfo => callInfo.Arg>() .Select(guid => new Collection { Id = guid, OrganizationId = CoreHelpers.GenerateComb() }).ToList()); + var existingUserType = OrganizationUserType.User; + await Assert.ThrowsAsync( - () => sutProvider.Sut.UpdateUserAsync(user, savingUserId, collectionAccess, null)); + () => sutProvider.Sut.UpdateUserAsync(user, existingUserType, savingUserId, collectionAccess, null)); } [Theory, BitAutoData] @@ -76,9 +81,9 @@ public class UpdateOrganizationUserCommandTests result.RemoveAt(0); return result; }); - + var existingUserType = OrganizationUserType.User; await Assert.ThrowsAsync( - () => sutProvider.Sut.UpdateUserAsync(user, savingUserId, collectionAccess, null)); + () => sutProvider.Sut.UpdateUserAsync(user, existingUserType, savingUserId, collectionAccess, null)); } [Theory, BitAutoData] @@ -94,8 +99,10 @@ public class UpdateOrganizationUserCommandTests .Returns(callInfo => callInfo.Arg>() .Select(guid => new Group { Id = guid, OrganizationId = CoreHelpers.GenerateComb() }).ToList()); + var existingUserType = OrganizationUserType.User; + await Assert.ThrowsAsync( - () => sutProvider.Sut.UpdateUserAsync(user, savingUserId, null, groupAccess)); + () => sutProvider.Sut.UpdateUserAsync(user, existingUserType, savingUserId, null, groupAccess)); } [Theory, BitAutoData] @@ -115,9 +122,9 @@ public class UpdateOrganizationUserCommandTests result.RemoveAt(0); return result; }); - + var existingUserType = OrganizationUserType.User; await Assert.ThrowsAsync( - () => sutProvider.Sut.UpdateUserAsync(user, savingUserId, null, groupAccess)); + () => sutProvider.Sut.UpdateUserAsync(user, existingUserType, savingUserId, null, groupAccess)); } [Theory, BitAutoData] @@ -165,7 +172,9 @@ public class UpdateOrganizationUserCommandTests .GetCountByFreeOrganizationAdminUserAsync(newUserData.Id) .Returns(0); - await sutProvider.Sut.UpdateUserAsync(newUserData, savingUser.UserId, collections, groups); + var existingUserType = OrganizationUserType.User; + + await sutProvider.Sut.UpdateUserAsync(newUserData, existingUserType, savingUser.UserId, collections, groups); var organizationService = sutProvider.GetDependency(); await organizationService.Received(1).ValidateOrganizationUserUpdatePermissions( @@ -184,7 +193,7 @@ public class UpdateOrganizationUserCommandTests [Theory] [BitAutoData(OrganizationUserType.Admin)] [BitAutoData(OrganizationUserType.Owner)] - public async Task UpdateUserAsync_WhenUpdatingUserToAdminOrOwner_WithUserAlreadyAdminOfAnotherFreeOrganization_Throws( + public async Task UpdateUserAsync_WhenUpdatingUserToAdminOrOwner_AndExistingUserTypeIsNotAdminOrOwner_WithUserAlreadyAdminOfAnotherFreeOrganization_Throws( OrganizationUserType userType, OrganizationUser oldUserData, OrganizationUser newUserData, @@ -199,10 +208,39 @@ public class UpdateOrganizationUserCommandTests sutProvider.GetDependency() .GetCountByFreeOrganizationAdminUserAsync(newUserData.UserId!.Value) .Returns(1); + var existingUserType = OrganizationUserType.User; // Assert var exception = await Assert.ThrowsAsync( - () => sutProvider.Sut.UpdateUserAsync(newUserData, null, null, null)); + () => sutProvider.Sut.UpdateUserAsync(newUserData, existingUserType, null, null, null)); + Assert.Contains("User can only be an admin of one free organization.", exception.Message); + } + + [Theory] + [BitAutoData(OrganizationUserType.Admin, OrganizationUserType.Admin)] + [BitAutoData(OrganizationUserType.Admin, OrganizationUserType.Owner)] + [BitAutoData(OrganizationUserType.Owner, OrganizationUserType.Admin)] + [BitAutoData(OrganizationUserType.Owner, OrganizationUserType.Owner)] + public async Task UpdateUserAsync_WhenUpdatingUserToAdminOrOwner_AndExistingUserTypeIsAdminOrOwner_WithUserAlreadyAdminOfAnotherFreeOrganization_Throws( + OrganizationUserType newUserType, + OrganizationUserType existingUserType, + OrganizationUser oldUserData, + OrganizationUser newUserData, + Organization organization, + SutProvider sutProvider) + { + organization.PlanType = PlanType.Free; + newUserData.Type = newUserType; + + Setup(sutProvider, organization, newUserData, oldUserData); + + sutProvider.GetDependency() + .GetCountByFreeOrganizationAdminUserAsync(newUserData.UserId!.Value) + .Returns(2); + + // Assert + var exception = await Assert.ThrowsAsync( + () => sutProvider.Sut.UpdateUserAsync(newUserData, existingUserType, null, null, null)); Assert.Contains("User can only be an admin of one free organization.", exception.Message); }