Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pm-15621] Refactor DeleteManagedOrganizationUserAccountCommand #5345

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

[pm-15621] Refactor DeleteManagedOrganizationUserAccountCommand #5345

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
bf5f8f7
[pm-15621] Refactor DeleteManagedOrganizationUserAccountCommand
JimmyVo16 Jan 29, 2025
720a692
[PM-15621] Address code review comments.
JimmyVo16 Feb 4, 2025
48ced72
Merge branch 'main' into ac/pm-15621/refactor-delete-command
JimmyVo16 Feb 4, 2025
adf3c1f
[PM-15621] Address code quality issues.
JimmyVo16 Feb 4, 2025
9e95d87
Merge branch 'main' into ac/pm-15621/refactor-delete-command
JimmyVo16 Feb 10, 2025
3f9db4e
Update src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/D…
JimmyVo16 Feb 10, 2025
c1d810c
Update src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/D…
JimmyVo16 Feb 10, 2025
5b2e5b3
[PM-15621] Fix test setups
JimmyVo16 Feb 10, 2025
c47f85c
[PM-15621] Add test coverage for edge cases.
JimmyVo16 Feb 10, 2025
0f2356a
Merge branch 'main' into ac/pm-15621/refactor-delete-command
JimmyVo16 Feb 10, 2025
ed7a3ef
[PM-15621] Fix single-user deletion bug.
JimmyVo16 Feb 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 29 additions & 52 deletions ...ole/OrganizationFeatures/OrganizationUsers/DeleteManagedOrganizationUserAccountCommand.cs
View file
Open in desktop
eliykat marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
private readonly IHasConfirmedOwnersExceptQuery _hasConfirmedOwnersExceptQuery;
private readonly IReferenceEventService _referenceEventService;
private readonly IPushNotificationService _pushService;
private readonly IOrganizationRepository _organizationRepository;
private readonly IProviderUserRepository _providerUserRepository;
public DeleteManagedOrganizationUserAccountCommand(
IUserService userService,
Expand All @@ -38,8 +37,8 @@
IHasConfirmedOwnersExceptQuery hasConfirmedOwnersExceptQuery,
IReferenceEventService referenceEventService,
IPushNotificationService pushService,
IOrganizationRepository organizationRepository,
IProviderUserRepository providerUserRepository)
IProviderUserRepository providerUserRepository
)
{
_userService = userService;
_eventService = eventService;
Expand All @@ -50,31 +49,12 @@
_hasConfirmedOwnersExceptQuery = hasConfirmedOwnersExceptQuery;
_referenceEventService = referenceEventService;
_pushService = pushService;
_organizationRepository = organizationRepository;
_providerUserRepository = providerUserRepository;
}

public async Task DeleteUserAsync(Guid organizationId, Guid organizationUserId, Guid? deletingUserId)
{
var organizationUser = await _organizationUserRepository.GetByIdAsync(organizationUserId);
if (organizationUser == null || organizationUser.OrganizationId != organizationId)
{
throw new NotFoundException("Member not found.");
}

var managementStatus = await _getOrganizationUsersManagementStatusQuery.GetUsersOrganizationManagementStatusAsync(organizationId, new[] { organizationUserId });
var hasOtherConfirmedOwners = await _hasConfirmedOwnersExceptQuery.HasConfirmedOwnersExceptAsync(organizationId, new[] { organizationUserId }, includeProvider: true);

await ValidateDeleteUserAsync(organizationId, organizationUser, deletingUserId, managementStatus, hasOtherConfirmedOwners);

var user = await _userRepository.GetByIdAsync(organizationUser.UserId!.Value);
if (user == null)
{
throw new NotFoundException("Member not found.");
}

await _userService.DeleteAsync(user);
await _eventService.LogOrganizationUserEventAsync(organizationUser, EventType.OrganizationUser_Deleted);
await DeleteManyUsersAsync(organizationId, new[] { organizationUserId }, deletingUserId);

Check warning on line 57 in src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/DeleteManagedOrganizationUserAccountCommand.cs

View check run for this annotation

Codecov / codecov/patch

src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/DeleteManagedOrganizationUserAccountCommand.cs#L57 

Added line #L57 was not covered by tests
JimmyVo16 marked this conversation as resolved.
Show resolved Hide resolved
}

public async Task<IEnumerable<(Guid OrganizationUserId, string? ErrorMessage)>> DeleteManyUsersAsync(Guid organizationId, IEnumerable<Guid> orgUserIds, Guid? deletingUserId)
Expand Down Expand Up @@ -105,7 +85,8 @@
throw new NotFoundException("Member not found.");
}

await ValidateUserMembershipAndPremiumAsync(user);
await ValidateUserAsync(user);
await CancelPremiumAsync(user);
eliykat marked this conversation as resolved.
Show resolved Hide resolved

results.Add((orgUserId, string.Empty));
}
Expand Down Expand Up @@ -197,46 +178,42 @@

}

private async Task ValidateUserMembershipAndPremiumAsync(User user)
private async Task ValidateUserAsync(User user)
{
await EnsureUserIsNotSoleOrganizationOwnerAsync(user);
await EnsureUserIsNotSoleProviderOwnerAsync(user);
eliykat marked this conversation as resolved.
Show resolved Hide resolved
}

private async Task CancelPremiumAsync(User user)
{
if (string.IsNullOrWhiteSpace(user.GatewaySubscriptionId))
{
return;

Check warning on line 191 in src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/DeleteManagedOrganizationUserAccountCommand.cs

View check run for this annotation

Codecov / codecov/patch

src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/DeleteManagedOrganizationUserAccountCommand.cs#L190-L191 

Added lines #L190 - L191 were not covered by tests
}
try
{
await _userService.CancelPremiumAsync(user);
}
catch (GatewayException) { }

Check warning on line 197 in src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/DeleteManagedOrganizationUserAccountCommand.cs

View check run for this annotation

Codecov / codecov/patch

src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/DeleteManagedOrganizationUserAccountCommand.cs#L197 

Added line #L197 was not covered by tests
}

private async Task EnsureUserIsNotSoleOrganizationOwnerAsync(User user)
{
// Check if user is the only owner of any organizations.
var onlyOwnerCount = await _organizationUserRepository.GetCountByOnlyOwnerAsync(user.Id);
if (onlyOwnerCount > 0)
{
throw new BadRequestException("Cannot delete this user because it is the sole owner of at least one organization. Please delete these organizations or upgrade another user.");
}

var orgs = await _organizationUserRepository.GetManyDetailsByUserAsync(user.Id, OrganizationUserStatusType.Confirmed);
if (orgs.Count == 1)
{
var org = await _organizationRepository.GetByIdAsync(orgs.First().OrganizationId);
if (org != null && (!org.Enabled || string.IsNullOrWhiteSpace(org.GatewaySubscriptionId)))
{
var orgCount = await _organizationUserRepository.GetCountByOrganizationIdAsync(org.Id);
if (orgCount <= 1)
{
await _organizationRepository.DeleteAsync(org);
}
else
{
throw new BadRequestException("Cannot delete this user because it is the sole owner of at least one organization. Please delete these organizations or upgrade another user.");
}
}
}
}


private async Task EnsureUserIsNotSoleProviderOwnerAsync(User user)
{
var onlyOwnerProviderCount = await _providerUserRepository.GetCountByOnlyOwnerAsync(user.Id);
if (onlyOwnerProviderCount > 0)
{
throw new BadRequestException("Cannot delete this user because it is the sole owner of at least one provider. Please delete these providers or upgrade another user.");
}

if (!string.IsNullOrWhiteSpace(user.GatewaySubscriptionId))
{
try
{
await _userService.CancelPremiumAsync(user);
}
catch (GatewayException) { }
}
}
}
217 changes: 0 additions & 217 deletions ...rganizationFeatures/OrganizationUsers/DeleteManagedOrganizationUserAccountCommandTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
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,222 +16,6 @@ namespace Bit.Core.Test.AdminConsole.OrganizationFeatures.OrganizationUsers;
[SutProviderCustomize]
public class DeleteManagedOrganizationUserAccountCommandTests
{
[Theory]
[BitAutoData]
public async Task DeleteUserAsync_WithValidUser_DeletesUserAndLogsEvent(
SutProvider<DeleteManagedOrganizationUserAccountCommand> sutProvider, User user, Guid deletingUserId,
[OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser organizationUser)
{
// Arrange
organizationUser.UserId = user.Id;

sutProvider.GetDependency<IUserRepository>()
.GetByIdAsync(user.Id)
.Returns(user);

sutProvider.GetDependency<IOrganizationUserRepository>()
.GetByIdAsync(organizationUser.Id)
.Returns(organizationUser);

sutProvider.GetDependency<IGetOrganizationUsersManagementStatusQuery>()
.GetUsersOrganizationManagementStatusAsync(
organizationUser.OrganizationId,
Arg.Is<IEnumerable<Guid>>(ids => ids.Contains(organizationUser.Id)))
.Returns(new Dictionary<Guid, bool> { { organizationUser.Id, true } });

sutProvider.GetDependency<IHasConfirmedOwnersExceptQuery>()
.HasConfirmedOwnersExceptAsync(
organizationUser.OrganizationId,
Arg.Is<IEnumerable<Guid>>(ids => ids.Contains(organizationUser.Id)),
includeProvider: Arg.Any<bool>())
.Returns(true);

// Act
await sutProvider.Sut.DeleteUserAsync(organizationUser.OrganizationId, organizationUser.Id, deletingUserId);

// Assert
await sutProvider.GetDependency<IUserService>().Received(1).DeleteAsync(user);
await sutProvider.GetDependency<IEventService>().Received(1)
.LogOrganizationUserEventAsync(organizationUser, EventType.OrganizationUser_Deleted);
}

[Theory]
[BitAutoData]
public async Task DeleteUserAsync_WithUserNotFound_ThrowsException(
SutProvider<DeleteManagedOrganizationUserAccountCommand> sutProvider,
Guid organizationId, Guid organizationUserId)
{
// Arrange
sutProvider.GetDependency<IOrganizationUserRepository>()
.GetByIdAsync(organizationUserId)
.Returns((OrganizationUser?)null);

// Act
var exception = await Assert.ThrowsAsync<NotFoundException>(() =>
sutProvider.Sut.DeleteUserAsync(organizationId, organizationUserId, null));

// Assert
Assert.Equal("Member not found.", exception.Message);
await sutProvider.GetDependency<IUserService>().Received(0).DeleteAsync(Arg.Any<User>());
await sutProvider.GetDependency<IEventService>().Received(0)
.LogOrganizationUserEventAsync(Arg.Any<OrganizationUser>(), Arg.Any<EventType>(), Arg.Any<DateTime?>());
}

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

sutProvider.GetDependency<IOrganizationUserRepository>()
.GetByIdAsync(organizationUser.Id)
.Returns(organizationUser);

sutProvider.GetDependency<IUserRepository>().GetByIdAsync(user.Id)
.Returns(user);

// Act
var exception = await Assert.ThrowsAsync<BadRequestException>(() =>
sutProvider.Sut.DeleteUserAsync(organizationUser.OrganizationId, organizationUser.Id, deletingUserId));

// Assert
Assert.Equal("You cannot delete yourself.", exception.Message);
await sutProvider.GetDependency<IUserService>().Received(0).DeleteAsync(Arg.Any<User>());
await sutProvider.GetDependency<IEventService>().Received(0)
.LogOrganizationUserEventAsync(Arg.Any<OrganizationUser>(), Arg.Any<EventType>(), Arg.Any<DateTime?>());
}

[Theory]
[BitAutoData]
public async Task DeleteUserAsync_WhenUserIsInvited_ThrowsException(
SutProvider<DeleteManagedOrganizationUserAccountCommand> sutProvider,
[OrganizationUser(OrganizationUserStatusType.Invited, OrganizationUserType.User)] OrganizationUser organizationUser)
{
// Arrange
organizationUser.UserId = null;

sutProvider.GetDependency<IOrganizationUserRepository>()
.GetByIdAsync(organizationUser.Id)
.Returns(organizationUser);

// Act
var exception = await Assert.ThrowsAsync<BadRequestException>(() =>
sutProvider.Sut.DeleteUserAsync(organizationUser.OrganizationId, organizationUser.Id, null));

// Assert
Assert.Equal("You cannot delete a member with Invited status.", exception.Message);
await sutProvider.GetDependency<IUserService>().Received(0).DeleteAsync(Arg.Any<User>());
await sutProvider.GetDependency<IEventService>().Received(0)
.LogOrganizationUserEventAsync(Arg.Any<OrganizationUser>(), Arg.Any<EventType>(), Arg.Any<DateTime?>());
}

[Theory]
[BitAutoData]
public async Task DeleteUserAsync_DeletingOwnerWhenNotOwner_ThrowsException(
SutProvider<DeleteManagedOrganizationUserAccountCommand> sutProvider, User user,
[OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.Owner)] OrganizationUser organizationUser,
Guid deletingUserId)
{
// Arrange
organizationUser.UserId = user.Id;

sutProvider.GetDependency<IOrganizationUserRepository>()
.GetByIdAsync(organizationUser.Id)
.Returns(organizationUser);

sutProvider.GetDependency<IUserRepository>().GetByIdAsync(user.Id)
.Returns(user);

sutProvider.GetDependency<ICurrentContext>()
.OrganizationOwner(organizationUser.OrganizationId)
.Returns(false);

// Act
var exception = await Assert.ThrowsAsync<BadRequestException>(() =>
sutProvider.Sut.DeleteUserAsync(organizationUser.OrganizationId, organizationUser.Id, deletingUserId));

// Assert
Assert.Equal("Only owners can delete other owners.", exception.Message);
await sutProvider.GetDependency<IUserService>().Received(0).DeleteAsync(Arg.Any<User>());
await sutProvider.GetDependency<IEventService>().Received(0)
.LogOrganizationUserEventAsync(Arg.Any<OrganizationUser>(), Arg.Any<EventType>(), Arg.Any<DateTime?>());
}

[Theory]
[BitAutoData]
public async Task DeleteUserAsync_DeletingLastConfirmedOwner_ThrowsException(
SutProvider<DeleteManagedOrganizationUserAccountCommand> sutProvider, User user,
[OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.Owner)] OrganizationUser organizationUser,
Guid deletingUserId)
{
// Arrange
organizationUser.UserId = user.Id;

sutProvider.GetDependency<IOrganizationUserRepository>()
.GetByIdAsync(organizationUser.Id)
.Returns(organizationUser);

sutProvider.GetDependency<IUserRepository>().GetByIdAsync(user.Id)
.Returns(user);

sutProvider.GetDependency<ICurrentContext>()
.OrganizationOwner(organizationUser.OrganizationId)
.Returns(true);

sutProvider.GetDependency<IHasConfirmedOwnersExceptQuery>()
.HasConfirmedOwnersExceptAsync(
organizationUser.OrganizationId,
Arg.Is<IEnumerable<Guid>>(ids => ids.Contains(organizationUser.Id)),
includeProvider: Arg.Any<bool>())
.Returns(false);

// Act
var exception = await Assert.ThrowsAsync<BadRequestException>(() =>
sutProvider.Sut.DeleteUserAsync(organizationUser.OrganizationId, organizationUser.Id, deletingUserId));

// Assert
Assert.Equal("Organization must have at least one confirmed owner.", exception.Message);
await sutProvider.GetDependency<IUserService>().Received(0).DeleteAsync(Arg.Any<User>());
await sutProvider.GetDependency<IEventService>().Received(0)
.LogOrganizationUserEventAsync(Arg.Any<OrganizationUser>(), Arg.Any<EventType>(), Arg.Any<DateTime?>());
}

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

sutProvider.GetDependency<IOrganizationUserRepository>()
.GetByIdAsync(organizationUser.Id)
.Returns(organizationUser);

sutProvider.GetDependency<IUserRepository>().GetByIdAsync(user.Id)
.Returns(user);

sutProvider.GetDependency<IGetOrganizationUsersManagementStatusQuery>()
.GetUsersOrganizationManagementStatusAsync(organizationUser.OrganizationId, Arg.Any<IEnumerable<Guid>>())
.Returns(new Dictionary<Guid, bool> { { organizationUser.Id, false } });

// Act
var exception = await Assert.ThrowsAsync<BadRequestException>(() =>
sutProvider.Sut.DeleteUserAsync(organizationUser.OrganizationId, organizationUser.Id, null));

// Assert
Assert.Equal("Member is not managed by the organization.", exception.Message);
await sutProvider.GetDependency<IUserService>().Received(0).DeleteAsync(Arg.Any<User>());
await sutProvider.GetDependency<IEventService>().Received(0)
.LogOrganizationUserEventAsync(Arg.Any<OrganizationUser>(), Arg.Any<EventType>(), Arg.Any<DateTime?>());
}

[Theory]
[BitAutoData]
public async Task DeleteManyUsersAsync_WithValidUsers_DeletesUsersAndLogsEvents(
Expand Down
Loading