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

Changes from 3 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
183 changes: 112 additions & 71 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 Down Expand Up @@ -60,111 +61,177 @@ public async Task DeleteUserAsync(Guid organizationId, Guid organizationUserId,
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 results = new List<(Guid OrganizationUserId, string? ErrorMessage)>();
var userDeletionResults = new List<(Guid OrganizationUserId, OrganizationUser? orgUser, User? user, 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 ValidateUserAsync(user);
await ValidateDeleteUserAsync(organizationId, orgUser, user, deletingUserId, managementStatus, hasOtherConfirmedOwners);

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);

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

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

await LogDeletedOrganizationUsersAsync(orgUsers, results);
var users = await _userRepository.GetManyAsync(userIds);

return results;
return users;
eliykat marked this conversation as resolved.
Show resolved Hide resolved
}

private async Task ValidateDeleteUserAsync(Guid organizationId, OrganizationUser orgUser, Guid? deletingUserId, IDictionary<Guid, bool> managementStatus, bool hasOtherConfirmedOwners)
private async Task ValidateDeleteUserAsync(Guid organizationId, OrganizationUser orgUser, User user, Guid? deletingUserId, IDictionary<Guid, bool> managementStatus, bool hasOtherConfirmedOwners)
JimmyVo16 marked this conversation as resolved.
Show resolved Hide resolved
{
EnsureUserStatusIsNotInvited(orgUser);
PreventSelfDeletion(orgUser, deletingUserId);
EnsureUserIsManagedByOrganization(orgUser, managementStatus);
PreventOrganizationSoleOwnerDeletion(orgUser, hasOtherConfirmedOwners);
JimmyVo16 marked this conversation as resolved.
Show resolved Hide resolved

await EnsureOnlyOwnersCanDeleteOwnersAsync(organizationId, orgUser, deletingUserId);
await EnsureUserIsNotSoleOrganizationOwnerAsync(user);
await EnsureUserIsNotSoleProviderOwnerAsync(user);
JimmyVo16 marked this conversation as resolved.
Show resolved Hide resolved
}
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))
JimmyVo16 marked this conversation as resolved.
Show resolved Hide resolved
{
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 PreventOrganizationSoleOwnerDeletion(OrganizationUser orgUser, bool hasOtherConfirmedOwners)
{
if (orgUser.Type != OrganizationUserType.Owner)
{
return;
}

if (!hasOtherConfirmedOwners)
{
throw new BadRequestException("Organization must have at least one confirmed owner.");
}
}

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.");
}
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.");
}
}

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 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)
{

Expand All @@ -178,12 +245,6 @@ await _referenceEventService.RaiseEventAsync(

}

private async Task ValidateUserAsync(User user)
{
await EnsureUserIsNotSoleOrganizationOwnerAsync(user);
await EnsureUserIsNotSoleProviderOwnerAsync(user);
}

private async Task CancelPremiumAsync(User user)
{
if (string.IsNullOrWhiteSpace(user.GatewaySubscriptionId))
Expand All @@ -196,24 +257,4 @@ private async Task CancelPremiumAsync(User user)
}
catch (GatewayException) { }
}

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

}


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.");
}
}
}
Loading