[PM-19703] Fix admin count logic to exclude current organization (#5918)

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

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

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<CollectionAccessSelection>? collectionAccess, IEnumerable<Guid>? groupAccess);
 }

src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs

@@ -55,11 +55,13 @@ public class UpdateOrganizationUserCommand : IUpdateOrganizationUserCommand
     /// Update an organization user.
     /// </summary>
     /// <param name="organizationUser">The modified organization user to save.</param>
+    /// <param name="existingUserType">The current type (member role) of the user.</param>
     /// <param name="savingUserId">The userId of the currently logged in user who is making the change.</param>
     /// <param name="collectionAccess">The user's updated collection access. If set to null, this removes all collection access.</param>
     /// <param name="groupAccess">The user's updated group access. If set to null, groups are not updated.</param>
     /// <exception cref="BadRequestException"></exception>
-    public async Task UpdateUserAsync(OrganizationUser organizationUser, Guid? savingUserId,
+    public async Task UpdateUserAsync(OrganizationUser organizationUser, OrganizationUserType existingUserType,
+        Guid? savingUserId,
         List<CollectionAccessSelection>? collectionAccess, IEnumerable<Guid>? 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)
+        await EnsureUserCannotBeAdminOrOwnerForMultipleFreeOrganizationAsync(organizationUser, existingUserType, organization);
-        {
-            // 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.");
-            }
-        }
 
         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<CollectionAccessSelection> collectionAccess)
     {

test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs

@@ -30,6 +30,7 @@ public class OrganizationUserControllerPutTests
         OrganizationUser organizationUser, OrganizationAbility organizationAbility,
         SutProvider<OrganizationUsersController> 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<IUpdateOrganizationUserCommand>().Received(1).UpdateUserAsync(Arg.Is<OrganizationUser>(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<List<CollectionAccessSelection>>(cas =>
                 cas.All(c => model.Collections.Any(m => m.Id == c.Id))),
@@ -77,6 +81,7 @@ public class OrganizationUserControllerPutTests
         OrganizationUser organizationUser, OrganizationAbility organizationAbility,
         SutProvider<OrganizationUsersController> 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<IUpdateOrganizationUserCommand>().Received(1).UpdateUserAsync(Arg.Is<OrganizationUser>(ou =>
-            ou.Type == model.Type &&
+                ou.Type == model.Type &&
-            ou.Permissions == CoreHelpers.ClassToJsonData(model.Permissions) &&
+                ou.Permissions == CoreHelpers.ClassToJsonData(model.Permissions) &&
-            ou.AccessSecretsManager == model.AccessSecretsManager &&
+                ou.AccessSecretsManager == model.AccessSecretsManager &&
-            ou.Id == orgUserId &&
+                ou.Id == orgUserId &&
-            ou.Email == orgUserEmail),
+                ou.Email == orgUserEmail), existingUserType,
             savingUserId,
             Arg.Is<List<CollectionAccessSelection>>(cas =>
                 cas.All(c => model.Collections.Any(m => m.Id == c.Id))),
@@ -110,6 +118,7 @@ public class OrganizationUserControllerPutTests
         OrganizationUser organizationUser, OrganizationAbility organizationAbility,
         SutProvider<OrganizationUsersController> 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<IUpdateOrganizationUserCommand>().Received(1).UpdateUserAsync(Arg.Is<OrganizationUser>(ou =>
-            ou.Type == model.Type &&
+                ou.Type == model.Type &&
-            ou.Permissions == CoreHelpers.ClassToJsonData(model.Permissions) &&
+                ou.Permissions == CoreHelpers.ClassToJsonData(model.Permissions) &&
-            ou.AccessSecretsManager == model.AccessSecretsManager &&
+                ou.AccessSecretsManager == model.AccessSecretsManager &&
-            ou.Id == orgUserId &&
+                ou.Id == orgUserId &&
-            ou.Email == orgUserEmail),
+                ou.Email == orgUserEmail), existingUserType,
             savingUserId,
             Arg.Is<List<CollectionAccessSelection>>(cas =>
                 cas.All(c => model.Collections.Any(m => m.Id == c.Id))),
@@ -142,6 +154,7 @@ public class OrganizationUserControllerPutTests
         OrganizationUser organizationUser, OrganizationAbility organizationAbility,
         SutProvider<OrganizationUsersController> 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<ClaimsPrincipal>(), Arg.Is<Collection>(c => c.Id == readonlyCollectionId1 || c.Id == readonlyCollectionId2),
                 Arg.Is<IEnumerable<IAuthorizationRequirement>>(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<IUpdateOrganizationUserCommand>().Received(1).UpdateUserAsync(Arg.Is<OrganizationUser>(ou =>
-            ou.Type == model.Type &&
+                ou.Type == model.Type &&
-            ou.Permissions == CoreHelpers.ClassToJsonData(model.Permissions) &&
+                ou.Permissions == CoreHelpers.ClassToJsonData(model.Permissions) &&
-            ou.AccessSecretsManager == model.AccessSecretsManager &&
+                ou.AccessSecretsManager == model.AccessSecretsManager &&
-            ou.Id == orgUserId &&
+                ou.Id == orgUserId &&
-            ou.Email == orgUserEmail),
+                ou.Email == orgUserEmail), existingUserType,
             savingUserId,
             Arg.Is<List<CollectionAccessSelection>>(cas =>
                 cas.Select(c => c.Id).SequenceEqual(currentCollectionAccess.Select(c => c.Id)) &&

test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs

@@ -27,8 +27,10 @@ public class UpdateOrganizationUserCommandTests
         List<CollectionAccessSelection> collections, List<Guid> groups, SutProvider<UpdateOrganizationUserCommand> sutProvider)
     {
         user.Id = default(Guid);
+        var existingUserType = OrganizationUserType.User;
+
         var exception = await Assert.ThrowsAsync<BadRequestException>(
-            () => 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<UpdateOrganizationUserCommand> sutProvider)
     {
         sutProvider.GetDependency<IOrganizationUserRepository>().GetByIdAsync(user.Id).Returns(originalUser);
+        var existingUserType = OrganizationUserType.User;
 
         await Assert.ThrowsAsync<NotFoundException>(
-            () => 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<IEnumerable<Guid>>()
                 .Select(guid => new Collection { Id = guid, OrganizationId = CoreHelpers.GenerateComb() }).ToList());
 
+        var existingUserType = OrganizationUserType.User;
+
         await Assert.ThrowsAsync<NotFoundException>(
-            () => 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<NotFoundException>(
-            () => 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<IEnumerable<Guid>>()
                 .Select(guid => new Group { Id = guid, OrganizationId = CoreHelpers.GenerateComb() }).ToList());
 
+        var existingUserType = OrganizationUserType.User;
+
         await Assert.ThrowsAsync<NotFoundException>(
-            () => 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<NotFoundException>(
-            () => 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<IOrganizationService>();
         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<IOrganizationUserRepository>()
             .GetCountByFreeOrganizationAdminUserAsync(newUserData.UserId!.Value)
             .Returns(1);
+        var existingUserType = OrganizationUserType.User;
 
         // Assert
         var exception = await Assert.ThrowsAsync<BadRequestException>(
-            () => 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<UpdateOrganizationUserCommand> sutProvider)
+    {
+        organization.PlanType = PlanType.Free;
+        newUserData.Type = newUserType;
+
+        Setup(sutProvider, organization, newUserData, oldUserData);
+
+        sutProvider.GetDependency<IOrganizationUserRepository>()
+            .GetCountByFreeOrganizationAdminUserAsync(newUserData.UserId!.Value)
+            .Returns(2);
+
+        // Assert
+        var exception = await Assert.ThrowsAsync<BadRequestException>(
+            () => sutProvider.Sut.UpdateUserAsync(newUserData, existingUserType, null, null, null));
         Assert.Contains("User can only be an admin of one free organization.", exception.Message);
     }