Skip to content

Commit

Permalink
[PM-15621] Fix single-user deletion bug.
Browse files Browse the repository at this point in the history
  • Loading branch information
@JimmyVo16
JimmyVo16 committed Feb 13, 2025
1 parent 0f2356a commit ed7a3ef
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 14 deletions.
39 changes: 27 additions & 12 deletions ...ole/OrganizationFeatures/OrganizationUsers/DeleteManagedOrganizationUserAccountCommand.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,31 @@ IProviderUserRepository providerUserRepository

public async Task DeleteUserAsync(Guid organizationId, Guid organizationUserId, Guid? deletingUserId)
{
await DeleteManyUsersAsync(organizationId, new[] { organizationUserId }, deletingUserId);
var result = await InternalDeleteManyUsersAsync(organizationId, new[] { organizationUserId }, deletingUserId);

var exception = result.Single().exception;

if (exception != null)
{
throw exception;
}
}

public async Task<IEnumerable<(Guid OrganizationUserId, string? ErrorMessage)>> DeleteManyUsersAsync(Guid organizationId, IEnumerable<Guid> orgUserIds, Guid? deletingUserId)
{
var userDeletionResults = await InternalDeleteManyUsersAsync(organizationId, orgUserIds, deletingUserId);

return userDeletionResults
.Select(result => (result.OrganizationUserId, result.exception?.Message))
.ToList();
}

private async Task<IEnumerable<(Guid OrganizationUserId, OrganizationUser? orgUser, User? user, Exception? exception)>> InternalDeleteManyUsersAsync(Guid organizationId, IEnumerable<Guid> orgUserIds, Guid? deletingUserId)
{
var orgUsers = await _organizationUserRepository.GetManyAsync(orgUserIds);
var users = await GetUsersAsync(orgUsers);
var managementStatus = await _getOrganizationUsersManagementStatusQuery.GetUsersOrganizationManagementStatusAsync(organizationId, orgUserIds);
var userDeletionResults = new List<(Guid OrganizationUserId, OrganizationUser? orgUser, User? user, string? ErrorMessage)>();
var userDeletionResults = new List<(Guid OrganizationUserId, OrganizationUser? orgUser, User? user, Exception? exception)>();

foreach (var orgUserId in orgUserIds)
{
Expand All @@ -89,21 +105,19 @@ public async Task DeleteUserAsync(Guid organizationId, Guid organizationUserId,

await CancelPremiumAsync(user);

userDeletionResults.Add((orgUserId, orgUser, user, string.Empty));
userDeletionResults.Add((orgUserId, orgUser, user, null));
}
catch (Exception ex)
catch (Exception exception)
{
userDeletionResults.Add((orgUserId, orgUser, user, ex.Message));
userDeletionResults.Add((orgUserId, orgUser, user, exception));
}
}

await HandleUserDeletionsAsync(userDeletionResults);

await LogDeletedOrganizationUsersAsync(userDeletionResults);

return userDeletionResults
.Select(i => (i.OrganizationUserId, i.ErrorMessage))
.ToList();
return userDeletionResults;
}

private async Task<IEnumerable<User>> GetUsersAsync(ICollection<OrganizationUser> orgUsers)
Expand Down Expand Up @@ -185,12 +199,13 @@ private async Task EnsureUserIsNotSoleProviderOwnerAsync(User user)
}

private async Task LogDeletedOrganizationUsersAsync(
List<(Guid OrganizationUserId, OrganizationUser? orgUser, User? user, string? ErrorMessage)> results)
List<(Guid OrganizationUserId, OrganizationUser? orgUser, User? user, Exception? exception)> results)
{
var eventDate = DateTime.UtcNow;

var events = results
.Where(result => string.IsNullOrEmpty(result.ErrorMessage)
.Where(result =>
result.exception == null
&& result.orgUser != null)
.Select(result => (result.orgUser!, (EventType)EventType.OrganizationUser_Deleted, (DateTime?)eventDate))
.ToList();
Expand All @@ -200,11 +215,11 @@ private async Task LogDeletedOrganizationUsersAsync(
await _eventService.LogOrganizationUserEventsAsync(events);
}
}
private async Task HandleUserDeletionsAsync(List<(Guid OrganizationUserId, OrganizationUser? orgUser, User? user, string? ErrorMessage)> userDeletionResults)
private async Task HandleUserDeletionsAsync(List<(Guid OrganizationUserId, OrganizationUser? orgUser, User? user, Exception? exception)> userDeletionResults)
{
var usersToDelete = userDeletionResults
.Where(result =>
string.IsNullOrEmpty(result.ErrorMessage)
result.exception == null
&& result.user != null)
.Select(i => i.user!);

Expand Down
58 changes: 56 additions & 2 deletions ...rganizationFeatures/OrganizationUsers/DeleteManagedOrganizationUserAccountCommandTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using Bit.Core.Context;
using Bit.Core.Entities;
using Bit.Core.Enums;
using Bit.Core.Exceptions;
using Bit.Core.Repositories;
using Bit.Core.Services;
using Bit.Core.Test.AutoFixture.OrganizationUserFixtures;
Expand All @@ -17,6 +18,59 @@ namespace Bit.Core.Test.AdminConsole.OrganizationFeatures.OrganizationUsers;
[SutProviderCustomize]
public class DeleteManagedOrganizationUserAccountCommandTests
{

[Theory]
[BitAutoData]
public async Task DeleteUserAsync_WithValidUser_DeletesUserAndLogsEvents(
SutProvider<DeleteManagedOrganizationUserAccountCommand> sutProvider, User user, Guid organizationId,
[OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser orgUser)
{
// Arrange
orgUser.OrganizationId = organizationId;
orgUser.UserId = user.Id;

sutProvider.GetDependency<IOrganizationUserRepository>()
.GetManyAsync(Arg.Any<IEnumerable<Guid>>())
.Returns(new List<OrganizationUser> { orgUser });

sutProvider.GetDependency<IUserRepository>()
.GetManyAsync(Arg.Is<IEnumerable<Guid>>(ids => ids.Contains(user.Id)))
.Returns(new[] { user });

sutProvider.GetDependency<IGetOrganizationUsersManagementStatusQuery>()
.GetUsersOrganizationManagementStatusAsync(organizationId, Arg.Any<IEnumerable<Guid>>())
.Returns(new Dictionary<Guid, bool> { { orgUser.Id, true } });

// Act
await sutProvider.Sut.DeleteUserAsync(organizationId, orgUser.Id, null);

// Assert
await sutProvider.GetDependency<IUserRepository>().Received(1).DeleteManyAsync(Arg.Is<IEnumerable<User>>(users => users.Any(u => u.Id == user.Id)));
await sutProvider.GetDependency<IEventService>().Received(1).LogOrganizationUserEventsAsync(
Arg.Is<IEnumerable<(OrganizationUser, EventType, DateTime?)>>(events =>
events.Count(e => e.Item1.Id == orgUser.Id && e.Item2 == EventType.OrganizationUser_Deleted) == 1));
}

[Theory]
[BitAutoData]
public async Task DeleteUserAsync_WhenError_ShouldThrowException(
SutProvider<DeleteManagedOrganizationUserAccountCommand> sutProvider, User user, Guid organizationId,
[OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser orgUser)
{
// Arrange
orgUser.OrganizationId = organizationId;
orgUser.UserId = user.Id;

sutProvider.GetDependency<IOrganizationUserRepository>()
.GetManyAsync(Arg.Any<IEnumerable<Guid>>())
.Returns(new List<OrganizationUser> { });

// Act
await Assert.ThrowsAsync<NotFoundException>(() => sutProvider.Sut.DeleteUserAsync(organizationId, orgUser.Id, null));

// Assert
}

[Theory]
[BitAutoData]
public async Task DeleteManyUsersAsync_WithValidUsers_DeletesUsersAndLogsEvents(
Expand Down Expand Up @@ -47,7 +101,7 @@ public async Task DeleteManyUsersAsync_WithValidUsers_DeletesUsersAndLogsEvents(

// Assert
Assert.Equal(2, results.Count());
Assert.All(results, r => Assert.Empty(r.Item2));
Assert.All(results, r => Assert.Null(r.Item2));

await sutProvider.GetDependency<IOrganizationUserRepository>().Received(1).GetManyAsync(userIds);
await sutProvider.GetDependency<IUserRepository>().Received(1).DeleteManyAsync(Arg.Is<IEnumerable<User>>(users => users.Any(u => u.Id == user1.Id) && users.Any(u => u.Id == user2.Id)));
Expand Down Expand Up @@ -359,7 +413,7 @@ public async Task DeleteManyUsersAsync_MixedValidAndInvalidUsers_ReturnsAppropri
var orgUser2ErrorMessage = results.First(r => r.Item1 == orgUser2.Id).Item2;
var orgUser3ErrorMessage = results.First(r => r.Item1 == orgUser3.Id).Item2;

Assert.Empty(orgUser1ErrorMessage);
Assert.Null(orgUser1ErrorMessage);
Assert.Equal("Member not found.", orgUser2ErrorMessage);
Assert.Equal("Member is not managed by the organization.", orgUser3ErrorMessage);

Expand Down

0 comments on commit ed7a3ef

Please sign in to comment.