Add API for bulk removal of org users (#1320)

* Add API for bulk removal of org users

* Refactor OrganizationService, extract some common code.

* Add tests for DeleteUserAsync

* Add tests for DeleteUsers

* Formating

* Update test/Core.Test/Services/OrganizationServiceTests.cs

added a space

Co-authored-by: Addison Beck <abeck@bitwarden.com>

src/Api/Controllers/OrganizationUsersController.cs

@@ -131,7 +131,7 @@ namespace Bit.Api.Controllers
         }
         
         [HttpPost("reinvite")]
-        public async Task BulkReinvite(string orgId, [FromBody]OrganizationUserBulkReinviteRequestModel model)
+        public async Task BulkReinvite(string orgId, [FromBody]OrganizationUserBulkRequestModel model)
         {
             var orgGuidId = new Guid(orgId);
             if (!_currentContext.ManageUsers(orgGuidId))
@@ -283,5 +283,19 @@ namespace Bit.Api.Controllers
             var userId = _userService.GetProperUserId(User);
             await _organizationService.DeleteUserAsync(orgGuidId, new Guid(id), userId.Value);
         }
+
+        [HttpDelete("")]
+        [HttpPost("delete")]
+        public async Task BulkDelete(string orgId, [FromBody]OrganizationUserBulkRequestModel model)
+        {
+            var orgGuidId = new Guid(orgId);
+            if (!_currentContext.ManageUsers(orgGuidId))
+            {
+                throw new NotFoundException();
+            }
+
+            var userId = _userService.GetProperUserId(User);
+            await _organizationService.DeleteUsersAsync(orgGuidId, model.Ids, userId.Value);
+        }
     }
 }

src/Core/Models/Api/Request/Organizations/OrganizationUserRequestModels.cs

@@ -91,7 +91,7 @@ namespace Bit.Core.Models.Api
         public string ResetPasswordKey { get; set; }
     }
 
-    public class OrganizationUserBulkReinviteRequestModel
+    public class OrganizationUserBulkRequestModel
     {
         [Required]
         public IEnumerable<Guid> Ids { get; set; }

src/Core/Services/IOrganizationService.cs

@@ -43,6 +43,7 @@ namespace Bit.Core.Services
         Task SaveUserAsync(OrganizationUser user, Guid? savingUserId, IEnumerable<SelectionReadOnly> collections);
         Task DeleteUserAsync(Guid organizationId, Guid organizationUserId, Guid? deletingUserId);
         Task DeleteUserAsync(Guid organizationId, Guid userId);
+        Task DeleteUsersAsync(Guid organizationId, IEnumerable<Guid> organizationUserIds, Guid? deleteingUserId);
         Task UpdateUserGroupsAsync(OrganizationUser organizationUser, IEnumerable<Guid> groupIds, Guid? loggedInUserId);
         Task UpdateUserResetPasswordEnrollmentAsync(Guid organizationId, Guid organizationUserId, string resetPasswordKey, Guid? callingUserId);
         Task<OrganizationLicense> GenerateLicenseAsync(Guid organizationId, Guid installationId);

src/Core/Services/Implementations/OrganizationService.cs

@@ -1308,10 +1308,7 @@ namespace Bit.Core.Services
             await _eventService.LogOrganizationUserEventAsync(orgUser, EventType.OrganizationUser_Confirmed);
             await _mailService.SendOrganizationConfirmedEmailAsync(org.Name, user.Email);
 
-            // push
+            await DeleteAndPushUserRegistrationAsync(organizationId, orgUser.UserId.Value);
-            var deviceIds = await GetUserDeviceIdsAsync(orgUser.UserId.Value);
-            await _pushRegistrationService.AddUserRegistrationOrganizationAsync(deviceIds, organizationId.ToString());
-            await _pushNotificationService.PushSyncOrgKeysAsync(orgUser.UserId.Value);
 
             return orgUser;
         }
@@ -1334,9 +1331,8 @@ namespace Bit.Core.Services
                 await ValidateOrganizationUserUpdatePermissions(savingUserId.Value, user.OrganizationId, user.Type, originalUser.Type);
             }
 
-            var confirmedOwners = (await GetConfirmedOwnersAsync(user.OrganizationId)).ToList();
             if (user.Type != OrganizationUserType.Owner &&
-                confirmedOwners.Count == 1 && confirmedOwners[0].Id == user.Id)
+                !await HasConfirmedOwnersExceptAsync(user.OrganizationId, Enumerable.Empty<Guid>()))
             {
                 throw new BadRequestException("Organization must have at least one confirmed owner.");
             }
@@ -1363,19 +1359,13 @@ namespace Bit.Core.Services
                 throw new BadRequestException("You cannot remove yourself.");
             }
 
-            if (orgUser.Type == OrganizationUserType.Owner && deletingUserId.HasValue)
+            if (orgUser.Type == OrganizationUserType.Owner && deletingUserId.HasValue &&
+                !await UserIsOwnerAsync(organizationId, deletingUserId.Value))
             {
-                var deletingUserOrgs = await _organizationUserRepository.GetManyByUserAsync(deletingUserId.Value);
+                throw new BadRequestException("Only owners can delete other owners.");
-                var anyOwners = deletingUserOrgs.Any(
-                    u => u.OrganizationId == organizationId && u.Type == OrganizationUserType.Owner);
-                if (!anyOwners)
-                {
-                    throw new BadRequestException("Only owners can delete other owners.");
-                }
             }
 
-            var confirmedOwners = (await GetConfirmedOwnersAsync(organizationId)).ToList();
+            if (!await HasConfirmedOwnersExceptAsync(organizationId, new[] {organizationUserId}))
-            if (confirmedOwners.Count == 1 && confirmedOwners[0].Id == organizationUserId)
             {
                 throw new BadRequestException("Organization must have at least one confirmed owner.");
             }
@@ -1385,11 +1375,7 @@ namespace Bit.Core.Services
 
             if (orgUser.UserId.HasValue)
             {
-                // push
+                await DeleteAndPushUserRegistrationAsync(organizationId, orgUser.UserId.Value);
-                var deviceIds = await GetUserDeviceIdsAsync(orgUser.UserId.Value);
-                await _pushRegistrationService.DeleteUserRegistrationOrganizationAsync(deviceIds,
-                    organizationId.ToString());
-                await _pushNotificationService.PushSyncOrgKeysAsync(orgUser.UserId.Value);
             }
         }
 
@@ -1401,8 +1387,7 @@ namespace Bit.Core.Services
                 throw new NotFoundException();
             }
 
-            var confirmedOwners = (await GetConfirmedOwnersAsync(organizationId)).ToList();
+            if (!await HasConfirmedOwnersExceptAsync(organizationId, new[] {orgUser.Id}))
-            if (confirmedOwners.Count == 1 && confirmedOwners[0].Id == orgUser.Id)
             {
                 throw new BadRequestException("Organization must have at least one confirmed owner.");
             }
@@ -1412,14 +1397,64 @@ namespace Bit.Core.Services
 
             if (orgUser.UserId.HasValue)
             {
-                // push
+                await DeleteAndPushUserRegistrationAsync(organizationId, orgUser.UserId.Value);
-                var deviceIds = await GetUserDeviceIdsAsync(orgUser.UserId.Value);
-                await _pushRegistrationService.DeleteUserRegistrationOrganizationAsync(deviceIds,
-                    organizationId.ToString());
-                await _pushNotificationService.PushSyncOrgKeysAsync(orgUser.UserId.Value);
             }
         }
 
+        public async Task DeleteUsersAsync(Guid organizationId, IEnumerable<Guid> organizationUsersId,
+            Guid? deletingUserId)
+        {
+            var orgUsers = await _organizationUserRepository.GetManyAsync(organizationUsersId);
+            var filteredUsers = orgUsers.Where(u => u.OrganizationId == organizationId)
+                .ToList();
+
+            if (!filteredUsers.Any())
+            {
+                throw new BadRequestException("Users invalid.");
+            }
+            
+            if (deletingUserId.HasValue && filteredUsers.Exists(u => u.UserId == deletingUserId.Value))
+            {
+                throw new BadRequestException("You cannot remove yourself.");
+            }
+
+            var owners = filteredUsers.Where(u => u.Type == OrganizationUserType.Owner);
+            if (!owners.Any() && deletingUserId.HasValue && !await UserIsOwnerAsync(organizationId, deletingUserId.Value))
+            {
+                throw new BadRequestException("Only owners can delete other owners.");
+            }
+
+            if (!await HasConfirmedOwnersExceptAsync(organizationId, organizationUsersId))
+            {
+                throw new BadRequestException("Organization must have at least one confirmed owner.");
+            }
+
+            foreach (var orgUser in filteredUsers)
+            {
+                // TODO: We should replace this call with `DeleteManyAsync`.
+                await _organizationUserRepository.DeleteAsync(orgUser);
+                await _eventService.LogOrganizationUserEventAsync(orgUser, EventType.OrganizationUser_Removed);
+
+                if (orgUser.UserId.HasValue)
+                {
+                    await DeleteAndPushUserRegistrationAsync(organizationId, orgUser.UserId.Value);
+                }
+            }
+        }
+
+        private async Task<bool> HasConfirmedOwnersExceptAsync(Guid organizationId, IEnumerable<Guid> organizationUsersId)
+        {
+            var confirmedOwners = await GetConfirmedOwnersAsync(organizationId);
+            var confirmedOwnersIds = confirmedOwners.Select(u => u.Id);
+            return confirmedOwnersIds.Except(organizationUsersId).Any();
+        }
+
+        private async Task<bool> UserIsOwnerAsync(Guid organizationId, Guid deletingUserId)
+        {
+            var deletingUserOrgs = await _organizationUserRepository.GetManyByUserAsync(deletingUserId);
+            return deletingUserOrgs.Any(u => u.OrganizationId == organizationId && u.Type == OrganizationUserType.Owner);
+        }
+
         public async Task UpdateUserGroupsAsync(OrganizationUser organizationUser, IEnumerable<Guid> groupIds, Guid? loggedInUserId)
         {
             if (loggedInUserId.HasValue)
@@ -1716,6 +1751,15 @@ namespace Bit.Core.Services
             return owners.Where(o => o.Status == OrganizationUserStatusType.Confirmed);
         }
 
+        private async Task DeleteAndPushUserRegistrationAsync(Guid organizationId, Guid userId)
+        {
+            var deviceIds = await GetUserDeviceIdsAsync(userId);
+            await _pushRegistrationService.DeleteUserRegistrationOrganizationAsync(deviceIds,
+                organizationId.ToString());
+            await _pushNotificationService.PushSyncOrgKeysAsync(userId);
+        }
+
+
         private async Task<IEnumerable<string>> GetUserDeviceIdsAsync(Guid userId)
         {
             var devices = await _deviceRepository.GetManyByUserIdAsync(userId);

test/Core.Test/Services/OrganizationServiceTests.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Linq;
 using System.Threading.Tasks;
 using Bit.Core.Models.Data;
 using Bit.Core.Models.Table;
@@ -372,9 +373,169 @@ namespace Bit.Core.Test.Services
             newUserData.OrganizationId = savingUser.OrganizationId = oldUserData.OrganizationId;
             savingUser.Type = OrganizationUserType.Owner;
             organizationUserRepository.GetByIdAsync(oldUserData.Id).Returns(oldUserData);
+            organizationUserRepository.GetManyByOrganizationAsync(savingUser.OrganizationId, OrganizationUserType.Owner)
+                .Returns(new List<OrganizationUser> { savingUser });
             organizationUserRepository.GetManyByUserAsync(savingUser.UserId.Value).Returns(new List<OrganizationUser> { savingUser });
 
             await sutProvider.Sut.SaveUserAsync(newUserData, savingUser.UserId, collections);
         }
+        
+        [Theory, CustomAutoData(typeof(SutProviderCustomization))]
+        public async Task DeleteUser_InvalidUser(OrganizationUser organizationUser, OrganizationUser deletingUser,
+            SutProvider<OrganizationService> sutProvider)
+        {
+            var organizationUserRepository = sutProvider.GetDependency<IOrganizationUserRepository>();
+
+            organizationUserRepository.GetByIdAsync(organizationUser.Id).Returns(organizationUser);
+
+            var exception = await Assert.ThrowsAsync<BadRequestException>(
+                () => sutProvider.Sut.DeleteUserAsync(Guid.NewGuid(), organizationUser.Id, deletingUser.UserId));
+            Assert.Contains("User not valid.", exception.Message);
+        }
+        
+        [Theory, CustomAutoData(typeof(SutProviderCustomization))]
+        public async Task DeleteUser_RemoveYourself(OrganizationUser deletingUser, SutProvider<OrganizationService> sutProvider)
+        {
+            var organizationUserRepository = sutProvider.GetDependency<IOrganizationUserRepository>();
+            
+            organizationUserRepository.GetByIdAsync(deletingUser.Id).Returns(deletingUser);
+
+            var exception = await Assert.ThrowsAsync<BadRequestException>(
+                () => sutProvider.Sut.DeleteUserAsync(deletingUser.OrganizationId, deletingUser.Id, deletingUser.UserId));
+            Assert.Contains("You cannot remove yourself.", exception.Message);
+        }
+        
+        [Theory, CustomAutoData(typeof(SutProviderCustomization))]
+        public async Task DeleteUser_NonOwnerRemoveOwner(OrganizationUser organizationUser, OrganizationUser deletingUser,
+            SutProvider<OrganizationService> sutProvider)
+        {
+            var organizationUserRepository = sutProvider.GetDependency<IOrganizationUserRepository>();
+            
+            organizationUser.OrganizationId = deletingUser.OrganizationId;
+            organizationUser.Type = OrganizationUserType.Owner;
+            organizationUserRepository.GetByIdAsync(organizationUser.Id).Returns(organizationUser);
+            organizationUserRepository.GetManyByUserAsync(deletingUser.UserId.Value).Returns(new[] { deletingUser });
+
+            var exception = await Assert.ThrowsAsync<BadRequestException>(
+                () => sutProvider.Sut.DeleteUserAsync(deletingUser.OrganizationId, organizationUser.Id, deletingUser.UserId));
+            Assert.Contains("Only owners can delete other owners.", exception.Message);
+        }
+
+        [Theory, CustomAutoData(typeof(SutProviderCustomization))]
+        public async Task DeleteUser_LastOwner(OrganizationUser organizationUser, OrganizationUser deletingUser,
+            SutProvider<OrganizationService> sutProvider)
+        {
+            var organizationUserRepository = sutProvider.GetDependency<IOrganizationUserRepository>();
+            
+            organizationUser.OrganizationId = deletingUser.OrganizationId;
+            organizationUser.Type = OrganizationUserType.Owner;
+            organizationUserRepository.GetByIdAsync(organizationUser.Id).Returns(organizationUser);
+            organizationUserRepository.GetManyByOrganizationAsync(deletingUser.OrganizationId, OrganizationUserType.Owner)
+                .Returns(new[] { organizationUser });
+
+            var exception = await Assert.ThrowsAsync<BadRequestException>(
+                () => sutProvider.Sut.DeleteUserAsync(deletingUser.OrganizationId, organizationUser.Id, null));
+            Assert.Contains("Organization must have at least one confirmed owner.", exception.Message);
+        }
+
+        [Theory, CustomAutoData(typeof(SutProviderCustomization))]
+        public async Task DeleteUser_Success(OrganizationUser organizationUser, OrganizationUser deletingUser,
+            SutProvider<OrganizationService> sutProvider)
+        {
+            var organizationUserRepository = sutProvider.GetDependency<IOrganizationUserRepository>();
+            
+            deletingUser.Type = OrganizationUserType.Owner;
+            deletingUser.Status = OrganizationUserStatusType.Confirmed;
+            organizationUser.OrganizationId = deletingUser.OrganizationId;
+            organizationUserRepository.GetByIdAsync(organizationUser.Id).Returns(organizationUser);
+            organizationUserRepository.GetByIdAsync(deletingUser.Id).Returns(deletingUser);
+            organizationUserRepository.GetManyByUserAsync(deletingUser.UserId.Value).Returns(new[] { deletingUser });
+            organizationUserRepository.GetManyByOrganizationAsync(deletingUser.OrganizationId, OrganizationUserType.Owner)
+                .Returns(new[] {deletingUser, organizationUser});
+
+            await sutProvider.Sut.DeleteUserAsync(deletingUser.OrganizationId, organizationUser.Id, deletingUser.UserId);
+        }
+        
+        [Theory, CustomAutoData(typeof(SutProviderCustomization))]
+        public async Task DeleteUsers_FilterInvalid(OrganizationUser organizationUser, OrganizationUser deletingUser,
+            SutProvider<OrganizationService> sutProvider)
+        {
+            var organizationUserRepository = sutProvider.GetDependency<IOrganizationUserRepository>();
+            var organizationUsers = new[] { organizationUser };
+            var organizationUserIds = organizationUsers.Select(u => u.Id);
+            organizationUserRepository.GetManyAsync(organizationUserIds).Returns(organizationUsers);
+
+            var exception = await Assert.ThrowsAsync<BadRequestException>(
+                () => sutProvider.Sut.DeleteUsersAsync(deletingUser.OrganizationId, organizationUserIds, deletingUser.UserId));
+            Assert.Contains("Users invalid.", exception.Message);
+        }
+        
+        [Theory, CustomAutoData(typeof(SutProviderCustomization))]
+        public async Task DeleteUsers_RemoveYourself(OrganizationUser deletingUser, SutProvider<OrganizationService> sutProvider)
+        {
+            var organizationUserRepository = sutProvider.GetDependency<IOrganizationUserRepository>();
+            var organizationUsers = new[] { deletingUser };
+            var organizationUserIds = organizationUsers.Select(u => u.Id);
+            organizationUserRepository.GetManyAsync(organizationUserIds).Returns(organizationUsers);
+
+            var exception = await Assert.ThrowsAsync<BadRequestException>(
+                () => sutProvider.Sut.DeleteUsersAsync(deletingUser.OrganizationId, organizationUserIds, deletingUser.UserId));
+            Assert.Contains("You cannot remove yourself.", exception.Message);
+        }
+        
+        [Theory, CustomAutoData(typeof(SutProviderCustomization))]
+        public async Task DeleteUsers_NonOwnerRemoveOwner(OrganizationUser deletingUser, OrganizationUser orgUser1, OrganizationUser orgUser2,
+            SutProvider<OrganizationService> sutProvider)
+        {
+            var organizationUserRepository = sutProvider.GetDependency<IOrganizationUserRepository>();
+
+            deletingUser.Type = OrganizationUserType.Admin;
+            orgUser1.OrganizationId = orgUser2.OrganizationId = deletingUser.OrganizationId;
+            var organizationUsers = new[] { orgUser1, orgUser2 };
+            var organizationUserIds = organizationUsers.Select(u => u.Id);
+            organizationUserRepository.GetManyAsync(organizationUserIds).Returns(organizationUsers);
+
+            var exception = await Assert.ThrowsAsync<BadRequestException>(
+                () => sutProvider.Sut.DeleteUsersAsync(deletingUser.OrganizationId, organizationUserIds, deletingUser.UserId));
+            Assert.Contains("Only owners can delete other owners.", exception.Message);
+        }
+        
+        [Theory, CustomAutoData(typeof(SutProviderCustomization))]
+        public async Task DeleteUsers_LastOwner(OrganizationUser orgUser, SutProvider<OrganizationService> sutProvider)
+        {
+            var organizationUserRepository = sutProvider.GetDependency<IOrganizationUserRepository>();
+
+            orgUser.Type = OrganizationUserType.Owner;
+            orgUser.Status = OrganizationUserStatusType.Confirmed;
+            var organizationUsers = new[] { orgUser };
+            var organizationUserIds = organizationUsers.Select(u => u.Id);
+            organizationUserRepository.GetManyAsync(organizationUserIds).Returns(organizationUsers);
+            organizationUserRepository.GetManyByOrganizationAsync(orgUser.OrganizationId, OrganizationUserType.Owner).Returns(organizationUsers);
+
+            var exception = await Assert.ThrowsAsync<BadRequestException>(
+                () => sutProvider.Sut.DeleteUsersAsync(orgUser.OrganizationId, organizationUserIds, null));
+            Assert.Contains("Organization must have at least one confirmed owner.", exception.Message);
+        }
+
+        [Theory, CustomAutoData(typeof(SutProviderCustomization))]
+        public async Task DeleteUsers_Success(OrganizationUser deletingUser, OrganizationUser orgUser1, OrganizationUser orgUser2,
+            SutProvider<OrganizationService> sutProvider)
+        {
+            var organizationUserRepository = sutProvider.GetDependency<IOrganizationUserRepository>();
+            
+            deletingUser.Type = OrganizationUserType.Owner;
+            deletingUser.Status = OrganizationUserStatusType.Confirmed;
+            orgUser1.OrganizationId = orgUser2.OrganizationId = deletingUser.OrganizationId;
+            orgUser1.Type = OrganizationUserType.Owner;
+            var organizationUsers = new[] { orgUser1, orgUser2 };
+            var organizationUserIds = organizationUsers.Select(u => u.Id);
+            organizationUserRepository.GetManyAsync(organizationUserIds).Returns(organizationUsers);
+            organizationUserRepository.GetByIdAsync(deletingUser.Id).Returns(deletingUser);
+            organizationUserRepository.GetManyByUserAsync(deletingUser.UserId.Value).Returns(new[] { deletingUser });
+            organizationUserRepository.GetManyByOrganizationAsync(deletingUser.OrganizationId, OrganizationUserType.Owner)
+                .Returns(new[] {deletingUser, orgUser1});
+
+            await sutProvider.Sut.DeleteUsersAsync(deletingUser.OrganizationId, organizationUserIds, deletingUser.UserId);
+        }
     }
 }