[PM-14243] Free organization limit is not enforced when editing user (#5155)

* Enforce free organization limit when updating user

* Add test for throwing error on accepting admin user joining multiple free organizations

* Add test for throwing BadRequest when free organization admin attempts to sign up for another free organization

* Fix user ID handling in UpdateOrganizationUserCommand for free organizations

* Rename parameter 'user' to 'organizationUser' in UpdateUserAsync method for clarity

src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IUpdateOrganizationUserCommand.cs

@@ -6,6 +6,6 @@ namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interface
 
 public interface IUpdateOrganizationUserCommand
 {
-    Task UpdateUserAsync(OrganizationUser user, Guid? savingUserId,
+    Task UpdateUserAsync(OrganizationUser organizationUser, Guid? savingUserId,
         List<CollectionAccessSelection>? collectionAccess, IEnumerable<Guid>? groupAccess);
 }

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

@@ -1,6 +1,7 @@
 #nullable enable
 using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces;
 using Bit.Core.AdminConsole.Repositories;
+using Bit.Core.Billing.Enums;
 using Bit.Core.Entities;
 using Bit.Core.Enums;
 using Bit.Core.Exceptions;
@@ -49,48 +50,64 @@ public class UpdateOrganizationUserCommand : IUpdateOrganizationUserCommand
     /// <summary>
     /// Update an organization user.
     /// </summary>
-    /// <param name="user">The modified user to save.</param>
+    /// <param name="organizationUser">The modified organization user to save.</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 user, Guid? savingUserId,
+    public async Task UpdateUserAsync(OrganizationUser organizationUser, Guid? savingUserId,
         List<CollectionAccessSelection>? collectionAccess, IEnumerable<Guid>? groupAccess)
     {
         // Avoid multiple enumeration
         collectionAccess = collectionAccess?.ToList();
         groupAccess = groupAccess?.ToList();
 
-        if (user.Id.Equals(default(Guid)))
+        if (organizationUser.Id.Equals(default(Guid)))
         {
             throw new BadRequestException("Invite the user first.");
         }
 
-        var originalUser = await _organizationUserRepository.GetByIdAsync(user.Id);
+        var originalOrganizationUser = await _organizationUserRepository.GetByIdAsync(organizationUser.Id);
-        if (originalUser == null || user.OrganizationId != originalUser.OrganizationId)
+        if (originalOrganizationUser == null || organizationUser.OrganizationId != originalOrganizationUser.OrganizationId)
         {
             throw new NotFoundException();
         }
 
+        var organization = await _organizationRepository.GetByIdAsync(organizationUser.OrganizationId);
+        if (organization == null)
+        {
+            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.");
+            }
+        }
+
         if (collectionAccess?.Any() == true)
         {
-            await ValidateCollectionAccessAsync(originalUser, collectionAccess.ToList());
+            await ValidateCollectionAccessAsync(originalOrganizationUser, collectionAccess.ToList());
         }
 
         if (groupAccess?.Any() == true)
         {
-            await ValidateGroupAccessAsync(originalUser, groupAccess.ToList());
+            await ValidateGroupAccessAsync(originalOrganizationUser, groupAccess.ToList());
         }
 
         if (savingUserId.HasValue)
         {
-            await _organizationService.ValidateOrganizationUserUpdatePermissions(user.OrganizationId, user.Type, originalUser.Type, user.GetPermissions());
+            await _organizationService.ValidateOrganizationUserUpdatePermissions(organizationUser.OrganizationId, organizationUser.Type, originalOrganizationUser.Type, organizationUser.GetPermissions());
         }
 
-        await _organizationService.ValidateOrganizationCustomPermissionsEnabledAsync(user.OrganizationId, user.Type);
+        await _organizationService.ValidateOrganizationCustomPermissionsEnabledAsync(organizationUser.OrganizationId, organizationUser.Type);
 
-        if (user.Type != OrganizationUserType.Owner &&
+        if (organizationUser.Type != OrganizationUserType.Owner &&
-            !await _hasConfirmedOwnersExceptQuery.HasConfirmedOwnersExceptAsync(user.OrganizationId, new[] { user.Id }))
+            !await _hasConfirmedOwnersExceptQuery.HasConfirmedOwnersExceptAsync(organizationUser.OrganizationId, new[] { organizationUser.Id }))
         {
             throw new BadRequestException("Organization must have at least one confirmed owner.");
         }
@@ -106,26 +123,25 @@ public class UpdateOrganizationUserCommand : IUpdateOrganizationUserCommand
 
         // Only autoscale (if required) after all validation has passed so that we know it's a valid request before
         // updating Stripe
-        if (!originalUser.AccessSecretsManager && user.AccessSecretsManager)
+        if (!originalOrganizationUser.AccessSecretsManager && organizationUser.AccessSecretsManager)
         {
-            var additionalSmSeatsRequired = await _countNewSmSeatsRequiredQuery.CountNewSmSeatsRequiredAsync(user.OrganizationId, 1);
+            var additionalSmSeatsRequired = await _countNewSmSeatsRequiredQuery.CountNewSmSeatsRequiredAsync(organizationUser.OrganizationId, 1);
             if (additionalSmSeatsRequired > 0)
             {
-                var organization = await _organizationRepository.GetByIdAsync(user.OrganizationId);
                 var update = new SecretsManagerSubscriptionUpdate(organization, true)
                    .AdjustSeats(additionalSmSeatsRequired);
                 await _updateSecretsManagerSubscriptionCommand.UpdateSubscriptionAsync(update);
             }
         }
 
-        await _organizationUserRepository.ReplaceAsync(user, collectionAccess);
+        await _organizationUserRepository.ReplaceAsync(organizationUser, collectionAccess);
 
         if (groupAccess != null)
         {
-            await _organizationUserRepository.UpdateGroupsAsync(user.Id, groupAccess);
+            await _organizationUserRepository.UpdateGroupsAsync(organizationUser.Id, groupAccess);
         }
 
-        await _eventService.LogOrganizationUserEventAsync(user, EventType.OrganizationUser_Updated);
+        await _eventService.LogOrganizationUserEventAsync(organizationUser, EventType.OrganizationUser_Updated);
     }
 
     private async Task ValidateCollectionAccessAsync(OrganizationUser originalUser,

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

@@ -2,6 +2,7 @@
 using Bit.Core.AdminConsole.Enums;
 using Bit.Core.AdminConsole.Services;
 using Bit.Core.Auth.Models.Business.Tokenables;
+using Bit.Core.Billing.Enums;
 using Bit.Core.Entities;
 using Bit.Core.Enums;
 using Bit.Core.Exceptions;
@@ -182,6 +183,29 @@ public class AcceptOrgUserCommandTests
             exception.Message);
     }
 
+    [Theory]
+    [BitAutoData(OrganizationUserType.Admin)]
+    [BitAutoData(OrganizationUserType.Owner)]
+    public async Task AcceptOrgUser_AdminOfFreePlanTryingToJoinSecondFreeOrg_ThrowsBadRequest(
+        OrganizationUserType userType,
+        SutProvider<AcceptOrgUserCommand> sutProvider,
+        User user, Organization org, OrganizationUser orgUser, OrganizationUserUserDetails adminUserDetails)
+    {
+        // Arrange
+        SetupCommonAcceptOrgUserMocks(sutProvider, user, org, orgUser, adminUserDetails);
+        org.PlanType = PlanType.Free;
+        orgUser.Type = userType;
+
+        sutProvider.GetDependency<IOrganizationUserRepository>()
+            .GetCountByFreeOrganizationAdminUserAsync(user.Id)
+            .Returns(1);
+
+        // Act & Assert
+        var exception = await Assert.ThrowsAsync<BadRequestException>(() =>
+            sutProvider.Sut.AcceptOrgUserAsync(orgUser, user, _userService));
+
+        Assert.Equal("You can only be an admin of one free organization.", exception.Message);
+    }
 
     // AcceptOrgUserByOrgIdAsync tests --------------------------------------------------------------------------------
 

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

@@ -3,6 +3,7 @@ using Bit.Core.AdminConsole.Entities;
 using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers;
 using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces;
 using Bit.Core.AdminConsole.Repositories;
+using Bit.Core.Billing.Enums;
 using Bit.Core.Entities;
 using Bit.Core.Enums;
 using Bit.Core.Exceptions;
@@ -144,6 +145,7 @@ public class UpdateOrganizationUserCommandTests
         newUserData.Id = oldUserData.Id;
         newUserData.UserId = oldUserData.UserId;
         newUserData.OrganizationId = savingUser.OrganizationId = oldUserData.OrganizationId = organization.Id;
+        newUserData.Type = OrganizationUserType.Admin;
         newUserData.Permissions = JsonSerializer.Serialize(permissions, new JsonSerializerOptions
         {
             PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
@@ -159,6 +161,10 @@ public class UpdateOrganizationUserCommandTests
             .Returns(callInfo => callInfo.Arg<IEnumerable<Guid>>()
                 .Select(guid => new Group { Id = guid, OrganizationId = oldUserData.OrganizationId }).ToList());
 
+        sutProvider.GetDependency<IOrganizationUserRepository>()
+            .GetCountByFreeOrganizationAdminUserAsync(newUserData.Id)
+            .Returns(0);
+
         await sutProvider.Sut.UpdateUserAsync(newUserData, savingUser.UserId, collections, groups);
 
         var organizationService = sutProvider.GetDependency<IOrganizationService>();
@@ -175,6 +181,31 @@ public class UpdateOrganizationUserCommandTests
             Arg.Is<IEnumerable<Guid>>(i => i.Contains(newUserData.Id)));
     }
 
+    [Theory]
+    [BitAutoData(OrganizationUserType.Admin)]
+    [BitAutoData(OrganizationUserType.Owner)]
+    public async Task UpdateUserAsync_WhenUpdatingUserToAdminOrOwner_WithUserAlreadyAdminOfAnotherFreeOrganization_Throws(
+        OrganizationUserType userType,
+        OrganizationUser oldUserData,
+        OrganizationUser newUserData,
+        Organization organization,
+        SutProvider<UpdateOrganizationUserCommand> sutProvider)
+    {
+        organization.PlanType = PlanType.Free;
+        newUserData.Type = userType;
+
+        Setup(sutProvider, organization, newUserData, oldUserData);
+
+        sutProvider.GetDependency<IOrganizationUserRepository>()
+            .GetCountByFreeOrganizationAdminUserAsync(newUserData.UserId!.Value)
+            .Returns(1);
+
+        // Assert
+        var exception = await Assert.ThrowsAsync<BadRequestException>(
+            () => sutProvider.Sut.UpdateUserAsync(newUserData, null, null, null));
+        Assert.Contains("User can only be an admin of one free organization.", exception.Message);
+    }
+
     private void Setup(SutProvider<UpdateOrganizationUserCommand> sutProvider, Organization organization,
         OrganizationUser newUser, OrganizationUser oldUser)
     {

test/Core.Test/AdminConsole/OrganizationFeatures/Organizations/OrganizationSignUp/CloudOrganizationSignUpCommandTests.cs

@@ -242,4 +242,27 @@ public class CloudICloudOrganizationSignUpCommandTests
             () => sutProvider.Sut.SignUpOrganizationAsync(signup));
         Assert.Contains("You can't subtract Machine Accounts!", exception.Message);
     }
+
+    [Theory]
+    [BitAutoData]
+    public async Task SignUpAsync_Free_ExistingFreeOrgAdmin_ThrowsBadRequest(
+        SutProvider<CloudOrganizationSignUpCommand> sutProvider)
+    {
+        // Arrange
+        var signup = new OrganizationSignup
+        {
+            Plan = PlanType.Free,
+            IsFromProvider = false,
+            Owner = new User { Id = Guid.NewGuid() }
+        };
+
+        sutProvider.GetDependency<IOrganizationUserRepository>()
+            .GetCountByFreeOrganizationAdminUserAsync(signup.Owner.Id)
+            .Returns(1);
+
+        // Act & Assert
+        var exception = await Assert.ThrowsAsync<BadRequestException>(
+            () => sutProvider.Sut.SignUpOrganizationAsync(signup));
+        Assert.Contains("You can only be an admin of one free organization.", exception.Message);
+    }
 }