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
+268 −324
Open

[pm-15621] Refactor DeleteManagedOrganizationUserAccountCommand #5345

Show file tree
Hide file tree
Changes from 8 commits
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
211 changes: 105 additions & 106 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 @@ -11,6 +11,7 @@
using Bit.Core.Tools.Models.Business;
using Bit.Core.Tools.Services;


#nullable enable

namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers;
Expand All @@ -26,7 +27,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 +38,8 @@
IHasConfirmedOwnersExceptQuery hasConfirmedOwnersExceptQuery,
IReferenceEventService referenceEventService,
IPushNotificationService pushService,
IOrganizationRepository organizationRepository,
IProviderUserRepository providerUserRepository)
IProviderUserRepository providerUserRepository
)
{
_userService = userService;
_eventService = eventService;
Expand All @@ -50,143 +50,172 @@
_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 58 in src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/DeleteManagedOrganizationUserAccountCommand.cs

View check run for this annotation

Codecov / codecov/patch

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

Added line #L58 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)
{
var orgUsers = await _organizationUserRepository.GetManyAsync(orgUserIds);
var userIds = orgUsers.Where(ou => ou.UserId.HasValue).Select(ou => ou.UserId!.Value).ToList();
var users = await _userRepository.GetManyAsync(userIds);

var users = await GetUsersAsync(orgUsers);
var managementStatus = await _getOrganizationUsersManagementStatusQuery.GetUsersOrganizationManagementStatusAsync(organizationId, orgUserIds);
var hasOtherConfirmedOwners = await _hasConfirmedOwnersExceptQuery.HasConfirmedOwnersExceptAsync(organizationId, orgUserIds, includeProvider: true);
var userDeletionResults = new List<(Guid OrganizationUserId, OrganizationUser? orgUser, User? user, string? ErrorMessage)>();

var results = new List<(Guid OrganizationUserId, string? ErrorMessage)>();
foreach (var orgUserId in orgUserIds)
{
OrganizationUser? orgUser = null;
User? user = null;

try
{
var orgUser = orgUsers.FirstOrDefault(ou => ou.Id == orgUserId);
orgUser = orgUsers.FirstOrDefault(ou => ou.Id == orgUserId);
if (orgUser == null || orgUser.OrganizationId != organizationId)
{
throw new NotFoundException("Member not found.");
}

await ValidateDeleteUserAsync(organizationId, orgUser, deletingUserId, managementStatus, hasOtherConfirmedOwners);
user = users.FirstOrDefault(u => u.Id == orgUser.UserId);

var user = users.FirstOrDefault(u => u.Id == orgUser.UserId);
if (user == null)
{
throw new NotFoundException("Member not found.");
}

await ValidateUserMembershipAndPremiumAsync(user);
await ValidateAsync(organizationId, orgUser, user, deletingUserId, managementStatus);

await CancelPremiumAsync(user);

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

var orgUserResultsToDelete = results.Where(result => string.IsNullOrEmpty(result.ErrorMessage));
var orgUsersToDelete = orgUsers.Where(orgUser => orgUserResultsToDelete.Any(result => orgUser.Id == result.OrganizationUserId));
var usersToDelete = users.Where(user => orgUsersToDelete.Any(orgUser => orgUser.UserId == user.Id));
await HandleUserDeletionsAsync(userDeletionResults);

if (usersToDelete.Any())
{
await DeleteManyAsync(usersToDelete);
}
await LogDeletedOrganizationUsersAsync(userDeletionResults);

await LogDeletedOrganizationUsersAsync(orgUsers, results);
return userDeletionResults
.Select(i => (i.OrganizationUserId, i.ErrorMessage))
.ToList();
}

return results;
private async Task<IEnumerable<User>> GetUsersAsync(ICollection<OrganizationUser> orgUsers)
{
var userIds = orgUsers
.Where(orgUser => orgUser.UserId.HasValue)
.Select(orgUser => orgUser.UserId!.Value)
.ToList();

return await _userRepository.GetManyAsync(userIds);
}

private async Task ValidateDeleteUserAsync(Guid organizationId, OrganizationUser orgUser, Guid? deletingUserId, IDictionary<Guid, bool> managementStatus, bool hasOtherConfirmedOwners)
private async Task ValidateAsync(Guid organizationId, OrganizationUser orgUser, User user, Guid? deletingUserId, IDictionary<Guid, bool> managementStatus)
{
EnsureUserStatusIsNotInvited(orgUser);
PreventSelfDeletion(orgUser, deletingUserId);
EnsureUserIsManagedByOrganization(orgUser, managementStatus);

await EnsureOnlyOwnersCanDeleteOwnersAsync(organizationId, orgUser, deletingUserId);
await EnsureUserIsNotSoleOrganizationOwnerAsync(user);
await EnsureUserIsNotSoleProviderOwnerAsync(user);
}
private static void EnsureUserStatusIsNotInvited(OrganizationUser orgUser)
{
if (!orgUser.UserId.HasValue || orgUser.Status == OrganizationUserStatusType.Invited)
{
throw new BadRequestException("You cannot delete a member with Invited status.");
}

if (deletingUserId.HasValue && orgUser.UserId.Value == deletingUserId.Value)
}
private static void PreventSelfDeletion(OrganizationUser orgUser, Guid? deletingUserId)
{
if (!orgUser.UserId.HasValue || !deletingUserId.HasValue)
{
return;
}
if (orgUser.UserId.Value == deletingUserId.Value)
{
throw new BadRequestException("You cannot delete yourself.");
}
}

if (orgUser.Type == OrganizationUserType.Owner)
private async Task EnsureOnlyOwnersCanDeleteOwnersAsync(Guid organizationId, OrganizationUser orgUser, Guid? deletingUserId)
eliykat marked this conversation as resolved.
Show resolved Hide resolved
{
if (orgUser.Type != OrganizationUserType.Owner)
{
if (deletingUserId.HasValue && !await _currentContext.OrganizationOwner(organizationId))
{
throw new BadRequestException("Only owners can delete other owners.");
}
return;
}

if (!hasOtherConfirmedOwners)
{
throw new BadRequestException("Organization must have at least one confirmed owner.");
}
if (deletingUserId.HasValue && !await _currentContext.OrganizationOwner(organizationId))
{
throw new BadRequestException("Only owners can delete other owners.");
}
}

private static void EnsureUserIsManagedByOrganization(OrganizationUser orgUser, IDictionary<Guid, bool> managementStatus)
{
if (!managementStatus.TryGetValue(orgUser.Id, out var isManaged) || !isManaged)
{
throw new BadRequestException("Member is not managed by the organization.");
}
}

private async Task LogDeletedOrganizationUsersAsync(
IEnumerable<OrganizationUser> orgUsers,
IEnumerable<(Guid OrgUserId, string? ErrorMessage)> results)
private async Task EnsureUserIsNotSoleOrganizationOwnerAsync(User user)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This overlaps with PreventOrganizationSoleOwnerDeletion (maybe what you meant to refer to in your comment above?)

  • PreventOrganizationSoleOwnerDeletion prevents you from deleting the sole owner of this organization
  • EnsureUserIsNotSoleOrganizationOwnerAsync prevents you from deleting the sole owner of any organization

I suggest we can just rely on the latter given that it obviously includes the former. Both also require database calls, and we don't want to hit the db unnecessarily.

I also question whether PreventOrganizationSoleOwnerDeletion would ever be tripped, given that you can't delete yourself and only owners can delete other owners. If you're the sole owner, who would be able to delete you?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After much thinking, yeah i think about right. EnsureUserIsNotSoleOrganizationOwnerAsync should handle all cases for use, so we can remove PreventOrganizationSoleOwnerDeletion.

(me thinking out loud) These logics are pretty complex. I wonder if there is a way to make them discoverable than deep in a validation method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(me thinking out loud) These logics are pretty complex. I wonder if there is a way to make them discoverable than deep in a validation method.

I would say validation is the primary source of complexity in our commands (and in old service code that should be commands). It's definitely a strong argument for making this code as simple and as easy to follow as possible.

In terms of discoverability/location, I believe Jared had previously mooted separate Validator classes which encapsulate the validation logic for a given command. Not sure if that makes it more discoverable, but it would provide a stronger separation than private methods.

{
var eventDate = DateTime.UtcNow;
var events = new List<(OrganizationUser OrgUser, EventType Event, DateTime? EventDate)>();

foreach (var (orgUserId, errorMessage) in results)
var onlyOwnerCount = await _organizationUserRepository.GetCountByOnlyOwnerAsync(user.Id);
if (onlyOwnerCount > 0)
{
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;
}
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.");

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

View check run for this annotation

Codecov / codecov/patch

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

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

events.Add((orgUser, EventType.OrganizationUser_Deleted, eventDate));
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.");

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

View check run for this annotation

Codecov / codecov/patch

src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/DeleteManagedOrganizationUserAccountCommand.cs#L182-L183 

Added lines #L182 - L183 were not covered by tests
}
}

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

var events = results
.Where(result => string.IsNullOrEmpty(result.ErrorMessage)
&& result.orgUser != null)
.Select(result => (result.orgUser!, (EventType)EventType.OrganizationUser_Deleted, (DateTime?)eventDate))
.ToList();

if (events.Any())
{
await _eventService.LogOrganizationUserEventsAsync(events);
}
}
private async Task DeleteManyAsync(IEnumerable<User> users)
private async Task HandleUserDeletionsAsync(List<(Guid OrganizationUserId, OrganizationUser? orgUser, User? user, string? ErrorMessage)> userDeletionResults)
{
var usersToDelete = userDeletionResults
.Where(result =>
string.IsNullOrEmpty(result.ErrorMessage)
&& result.user != null)
.Select(i => i.user!);

if (usersToDelete.Any())
{
await DeleteManyAsync(usersToDelete);
}
}

private async Task DeleteManyAsync(IEnumerable<User> users)
{
await _userRepository.DeleteManyAsync(users);
foreach (var user in users)
{
Expand All @@ -197,46 +226,16 @@

}

private async Task ValidateUserMembershipAndPremiumAsync(User user)
private async Task CancelPremiumAsync(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.");
}
}
}

var onlyOwnerProviderCount = await _providerUserRepository.GetCountByOnlyOwnerAsync(user.Id);
if (onlyOwnerProviderCount > 0)
if (string.IsNullOrWhiteSpace(user.GatewaySubscriptionId))
{
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.");
return;

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L233 was not covered by tests
}

if (!string.IsNullOrWhiteSpace(user.GatewaySubscriptionId))
try
{
try
{
await _userService.CancelPremiumAsync(user);
}
catch (GatewayException) { }
await _userService.CancelPremiumAsync(user);
}
catch (GatewayException) { }

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L239 was not covered by tests
}
}
Loading
Loading