From 5a493c43eb251b19c4f05eeff6bf935f28959fd8 Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Wed, 13 Nov 2024 11:40:05 +0000 Subject: [PATCH] Refactor RemoveUsersAsync method to return just the OrgUserId and update related logic. --- .../OrganizationUsersController.cs | 2 +- .../IRemoveOrganizationUserCommand.cs | 14 ++++-- .../RemoveOrganizationUserCommand.cs | 43 ++++++++++++++++--- .../RemoveOrganizationUserCommandTests.cs | 8 ++-- 4 files changed, 52 insertions(+), 15 deletions(-) diff --git a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs index b81cb068f..dc11d2568 100644 --- a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs +++ b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs @@ -538,7 +538,7 @@ public class OrganizationUsersController : Controller var userId = _userService.GetProperUserId(User); var result = await _removeOrganizationUserCommand.RemoveUsersAsync(orgId, model.Ids, userId.Value); return new ListResponseModel(result.Select(r => - new OrganizationUserBulkResponseModel(r.Item1.Id, r.Item2))); + new OrganizationUserBulkResponseModel(r.OrganizationUserId, r.ErrorMessage))); } [RequireFeature(FeatureFlagKeys.AccountDeprovisioning)] diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IRemoveOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IRemoveOrganizationUserCommand.cs index 583645a89..e92f29ae1 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IRemoveOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IRemoveOrganizationUserCommand.cs @@ -1,5 +1,4 @@ -using Bit.Core.Entities; -using Bit.Core.Enums; +using Bit.Core.Enums; namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; @@ -9,6 +8,13 @@ public interface IRemoveOrganizationUserCommand Task RemoveUserAsync(Guid organizationId, Guid organizationUserId, EventSystemUser eventSystemUser); Task RemoveUserAsync(Guid organizationId, Guid userId); - Task>> RemoveUsersAsync(Guid organizationId, - IEnumerable organizationUserIds, Guid? deletingUserId); + + /// + /// Removes multiple users from an organization. + /// + /// + /// A list of tuples containing the organization user id and the error message for each user that could not be removed, otherwise empty. + /// + Task> RemoveUsersAsync( + Guid organizationId, IEnumerable organizationUserIds, Guid? deletingUserId); } diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RemoveOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RemoveOrganizationUserCommand.cs index 32932dcc4..c69ef0077 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RemoveOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RemoveOrganizationUserCommand.cs @@ -72,9 +72,8 @@ public class RemoveOrganizationUserCommand : IRemoveOrganizationUserCommand await _eventService.LogOrganizationUserEventAsync(organizationUser, EventType.OrganizationUser_Removed); } - public async Task>> RemoveUsersAsync(Guid organizationId, - IEnumerable organizationUsersId, - Guid? deletingUserId) + public async Task> RemoveUsersAsync( + Guid organizationId, IEnumerable organizationUsersId, Guid? deletingUserId) { var orgUsers = await _organizationUserRepository.GetManyAsync(organizationUsersId); var filteredUsers = orgUsers.Where(u => u.OrganizationId == organizationId) @@ -99,7 +98,7 @@ public class RemoveOrganizationUserCommand : IRemoveOrganizationUserCommand var managementStatus = _featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning) ? await _getOrganizationUsersManagementStatusQuery.GetUsersOrganizationManagementStatusAsync(organizationId, filteredUsers.Select(u => u.Id)) : filteredUsers.ToDictionary(u => u.Id, u => false); - var result = new List>(); + var result = new List<(Guid OrganizationUserId, string ErrorMessage)>(); var deletedUserIds = new List(); foreach (var orgUser in filteredUsers) { @@ -126,15 +125,22 @@ public class RemoveOrganizationUserCommand : IRemoveOrganizationUserCommand { await DeleteAndPushUserRegistrationAsync(organizationId, orgUser.UserId.Value); } - result.Add(Tuple.Create(orgUser, string.Empty)); + result.Add((orgUser.Id, string.Empty)); deletedUserIds.Add(orgUser.Id); } catch (BadRequestException e) { - result.Add(Tuple.Create(orgUser, e.Message)); + result.Add((orgUser.Id, e.Message)); } + } + if (deletedUserIds.Any()) + { + DateTime? eventDate = DateTime.UtcNow; await _organizationUserRepository.DeleteManyAsync(deletedUserIds); + await _eventService.LogOrganizationUserEventsAsync( + orgUsers.Where(u => deletedUserIds.Contains(u.Id)) + .Select(u => (u, EventType.OrganizationUser_Removed, eventDate))); } return result; @@ -200,4 +206,29 @@ public class RemoveOrganizationUserCommand : IRemoveOrganizationUserCommand organizationId.ToString()); await _pushNotificationService.PushSyncOrgKeysAsync(userId); } + + private async Task LogDeletedOrganizationUsersAsync( + IEnumerable orgUsers, + IEnumerable<(Guid OrgUserId, string ErrorMessage)> results) + { + var eventDate = DateTime.UtcNow; + var events = new List<(OrganizationUser OrgUser, EventType Event, DateTime? EventDate)>(); + + foreach (var (orgUserId, errorMessage) in results) + { + var orgUser = orgUsers.FirstOrDefault(ou => ou.Id == orgUserId); + // If the user was not found or there was an error, we skip logging the event + if (orgUser == null || !string.IsNullOrEmpty(errorMessage)) + { + continue; + } + + events.Add((orgUser, EventType.OrganizationUser_Deleted, eventDate)); + } + + if (events.Any()) + { + await _eventService.LogOrganizationUserEventsAsync(events); + } + } } diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RemoveOrganizationUserCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RemoveOrganizationUserCommandTests.cs index 8ccf319db..36945e220 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RemoveOrganizationUserCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RemoveOrganizationUserCommandTests.cs @@ -298,7 +298,7 @@ public class RemoveOrganizationUserCommandTests .Returns(true); var result = await sutProvider.Sut.RemoveUsersAsync(deletingUser.OrganizationId, organizationUserIds, deletingUser.UserId); - Assert.Contains("You cannot remove yourself.", result[0].Item2); + Assert.Contains("You cannot remove yourself.", result.First().ErrorMessage); } [Theory, BitAutoData] @@ -319,7 +319,7 @@ public class RemoveOrganizationUserCommandTests .Returns(true); var result = await sutProvider.Sut.RemoveUsersAsync(deletingUser.OrganizationId, organizationUserIds, deletingUser.UserId); - Assert.Contains("Only owners can delete other owners.", result[0].Item2); + Assert.Contains("Only owners can delete other owners.", result.First().ErrorMessage); } [Theory, BitAutoData] @@ -347,7 +347,7 @@ public class RemoveOrganizationUserCommandTests await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().DeleteAsync(default); await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().LogOrganizationUserEventAsync((OrganizationUser)default, EventType.OrganizationUser_Removed); - Assert.Contains("Managed members cannot be simply removed, their entire individual account must be deleted.", result[0].Item2); + Assert.Contains("Managed members cannot be simply removed, their entire individual account must be deleted.", result.First().ErrorMessage); } [Theory, BitAutoData] @@ -373,7 +373,7 @@ public class RemoveOrganizationUserCommandTests var result = await sutProvider.Sut.RemoveUsersAsync(orgUser.OrganizationId, new[] { orgUser.Id }, null); - Assert.True(result.Count == 1 && result[0].Item1.Id == orgUser.Id && result[0].Item2 == string.Empty); + Assert.True(result.Count() == 1 && result.First().OrganizationUserId == orgUser.Id && result.First().ErrorMessage == string.Empty); await sutProvider.GetDependency().Received(1).DeleteManyAsync(Arg.Is>(i => i.Contains(orgUser.Id))); await sutProvider.GetDependency().Received(1).LogOrganizationUserEventAsync(orgUser, EventType.OrganizationUser_Removed); }