Skip to content

Commit

Permalink
[PM-13026] Refactor remove and bulkremove methods to throw error if u…
Browse files Browse the repository at this point in the history 
…ser is managed by an organization (#5034)

* Enhance RemoveOrganizationUserCommand to block removing managed users when account deprovisioning is enabled

* Refactor RemoveUsersAsync method to return just the OrgUserId and update related logic.

* Refactor RemoveOrganizationUserCommand to improve variable naming and remove unused logging method

* Add support for event system user in RemoveUsersAsync method. Refactor unit tests.

* Add xmldoc to IRemoveOrganizationUserCommand methods

* Refactor RemoveOrganizationUserCommand to use TimeProvider for event date retrieval and update unit tests accordingly

* Refactor RemoveOrganizationUserCommand to use constants for error messages

* Refactor unit tests to separate feature flag tests

* refactor: Update parameter names for clarity in RemoveOrganizationUserCommand

* refactor: Rename validation and repository methods for user removal clarity
  • Loading branch information
@r-tome
r-tome authored Nov 27, 2024
1 parent 1b75e35 commit 674bd1e
Show file tree
Hide file tree
Showing 4 changed files with 757 additions and 178 deletions.
2 changes: 1 addition & 1 deletion src/Api/AdminConsole/Controllers/OrganizationUsersController.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,7 @@ public async Task<ListResponseModel<OrganizationUserBulkResponseModel>> BulkRemo
var userId = _userService.GetProperUserId(User);
var result = await _removeOrganizationUserCommand.RemoveUsersAsync(orgId, model.Ids, userId.Value);
return new ListResponseModel<OrganizationUserBulkResponseModel>(result.Select(r =>
new OrganizationUserBulkResponseModel(r.Item1.Id, r.Item2)));
new OrganizationUserBulkResponseModel(r.OrganizationUserId, r.ErrorMessage)));
}

[RequireFeature(FeatureFlagKeys.AccountDeprovisioning)]
Expand Down
49 changes: 44 additions & 5 deletions ...nsole/OrganizationFeatures/OrganizationUsers/Interfaces/IRemoveOrganizationUserCommand.cs
View file
Original file line number Diff line number Diff line change
@@ -1,14 +1,53 @@
using Bit.Core.Entities;
using Bit.Core.Enums;
using Bit.Core.Enums;

namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces;

public interface IRemoveOrganizationUserCommand
{
/// <summary>
/// Removes a user from an organization.
/// </summary>
/// <param name="organizationId">The ID of the organization.</param>
/// <param name="userId">The ID of the user to remove.</param>
Task RemoveUserAsync(Guid organizationId, Guid userId);

/// <summary>
/// Removes a user from an organization with a specified deleting user.
/// </summary>
/// <param name="organizationId">The ID of the organization.</param>
/// <param name="organizationUserId">The ID of the organization user to remove.</param>
/// <param name="deletingUserId">The ID of the user performing the removal operation.</param>
Task RemoveUserAsync(Guid organizationId, Guid organizationUserId, Guid? deletingUserId);

/// <summary>
/// Removes a user from an organization using a system user.
/// </summary>
/// <param name="organizationId">The ID of the organization.</param>
/// <param name="organizationUserId">The ID of the organization user to remove.</param>
/// <param name="eventSystemUser">The system user performing the removal operation.</param>
Task RemoveUserAsync(Guid organizationId, Guid organizationUserId, EventSystemUser eventSystemUser);
Task RemoveUserAsync(Guid organizationId, Guid userId);
Task<List<Tuple<OrganizationUser, string>>> RemoveUsersAsync(Guid organizationId,
IEnumerable<Guid> organizationUserIds, Guid? deletingUserId);

/// <summary>
/// Removes multiple users from an organization with a specified deleting user.
/// </summary>
/// <param name="organizationId">The ID of the organization.</param>
/// <param name="organizationUserIds">The collection of organization user IDs to remove.</param>
/// <param name="deletingUserId">The ID of the user performing the removal operation.</param>
/// <returns>
/// A list of tuples containing the organization user id and the error message for each user that could not be removed, otherwise empty.
/// </returns>
Task<IEnumerable<(Guid OrganizationUserId, string ErrorMessage)>> RemoveUsersAsync(
Guid organizationId, IEnumerable<Guid> organizationUserIds, Guid? deletingUserId);

/// <summary>
/// Removes multiple users from an organization using a system user.
/// </summary>
/// <param name="organizationId">The ID of the organization.</param>
/// <param name="organizationUserIds">The collection of organization user IDs to remove.</param>
/// <param name="eventSystemUser">The system user performing the removal operation.</param>
/// <returns>
/// A list of tuples containing the organization user id and the error message for each user that could not be removed, otherwise empty.
/// </returns>
Task<IEnumerable<(Guid OrganizationUserId, string ErrorMessage)>> RemoveUsersAsync(
Guid organizationId, IEnumerable<Guid> organizationUserIds, EventSystemUser eventSystemUser);
}
199 changes: 131 additions & 68 deletions ...Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RemoveOrganizationUserCommand.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,16 @@ public class RemoveOrganizationUserCommand : IRemoveOrganizationUserCommand
private readonly IPushRegistrationService _pushRegistrationService;
private readonly ICurrentContext _currentContext;
private readonly IHasConfirmedOwnersExceptQuery _hasConfirmedOwnersExceptQuery;
private readonly IGetOrganizationUsersManagementStatusQuery _getOrganizationUsersManagementStatusQuery;
private readonly IFeatureService _featureService;
private readonly TimeProvider _timeProvider;

public const string UserNotFoundErrorMessage = "User not found.";
public const string UsersInvalidErrorMessage = "Users invalid.";
public const string RemoveYourselfErrorMessage = "You cannot remove yourself.";
public const string RemoveOwnerByNonOwnerErrorMessage = "Only owners can delete other owners.";
public const string RemoveLastConfirmedOwnerErrorMessage = "Organization must have at least one confirmed owner.";
public const string RemoveClaimedAccountErrorMessage = "Cannot remove member accounts claimed by the organization. To offboard a member, revoke or delete the account.";

public RemoveOrganizationUserCommand(
IDeviceRepository deviceRepository,
Expand All @@ -25,7 +35,10 @@ public RemoveOrganizationUserCommand(
IPushNotificationService pushNotificationService,
IPushRegistrationService pushRegistrationService,
ICurrentContext currentContext,
IHasConfirmedOwnersExceptQuery hasConfirmedOwnersExceptQuery)
IHasConfirmedOwnersExceptQuery hasConfirmedOwnersExceptQuery,
IGetOrganizationUsersManagementStatusQuery getOrganizationUsersManagementStatusQuery,
IFeatureService featureService,
TimeProvider timeProvider)
{
_deviceRepository = deviceRepository;
_organizationUserRepository = organizationUserRepository;
Expand All @@ -34,123 +47,107 @@ public RemoveOrganizationUserCommand(
_pushRegistrationService = pushRegistrationService;
_currentContext = currentContext;
_hasConfirmedOwnersExceptQuery = hasConfirmedOwnersExceptQuery;
_getOrganizationUsersManagementStatusQuery = getOrganizationUsersManagementStatusQuery;
_featureService = featureService;
_timeProvider = timeProvider;
}

public async Task RemoveUserAsync(Guid organizationId, Guid organizationUserId, Guid? deletingUserId)
public async Task RemoveUserAsync(Guid organizationId, Guid userId)
{
var organizationUser = await _organizationUserRepository.GetByIdAsync(organizationUserId);
ValidateDeleteUser(organizationId, organizationUser);
var organizationUser = await _organizationUserRepository.GetByOrganizationAsync(organizationId, userId);
ValidateRemoveUser(organizationId, organizationUser);

await RepositoryDeleteUserAsync(organizationUser, deletingUserId);
await RepositoryRemoveUserAsync(organizationUser, deletingUserId: null, eventSystemUser: null);

await _eventService.LogOrganizationUserEventAsync(organizationUser, EventType.OrganizationUser_Removed);
}

public async Task RemoveUserAsync(Guid organizationId, Guid organizationUserId, EventSystemUser eventSystemUser)
public async Task RemoveUserAsync(Guid organizationId, Guid organizationUserId, Guid? deletingUserId)
{
var organizationUser = await _organizationUserRepository.GetByIdAsync(organizationUserId);
ValidateDeleteUser(organizationId, organizationUser);
ValidateRemoveUser(organizationId, organizationUser);

await RepositoryDeleteUserAsync(organizationUser, null);
await RepositoryRemoveUserAsync(organizationUser, deletingUserId, eventSystemUser: null);

await _eventService.LogOrganizationUserEventAsync(organizationUser, EventType.OrganizationUser_Removed, eventSystemUser);
await _eventService.LogOrganizationUserEventAsync(organizationUser, EventType.OrganizationUser_Removed);
}

public async Task RemoveUserAsync(Guid organizationId, Guid userId)
public async Task RemoveUserAsync(Guid organizationId, Guid organizationUserId, EventSystemUser eventSystemUser)
{
var organizationUser = await _organizationUserRepository.GetByOrganizationAsync(organizationId, userId);
ValidateDeleteUser(organizationId, organizationUser);
var organizationUser = await _organizationUserRepository.GetByIdAsync(organizationUserId);
ValidateRemoveUser(organizationId, organizationUser);

await RepositoryDeleteUserAsync(organizationUser, null);
await RepositoryRemoveUserAsync(organizationUser, deletingUserId: null, eventSystemUser);

await _eventService.LogOrganizationUserEventAsync(organizationUser, EventType.OrganizationUser_Removed);
await _eventService.LogOrganizationUserEventAsync(organizationUser, EventType.OrganizationUser_Removed, eventSystemUser);
}

public async Task<List<Tuple<OrganizationUser, string>>> RemoveUsersAsync(Guid organizationId,
IEnumerable<Guid> organizationUsersId,
Guid? deletingUserId)
public async Task<IEnumerable<(Guid OrganizationUserId, string ErrorMessage)>> RemoveUsersAsync(
Guid organizationId, IEnumerable<Guid> organizationUserIds, Guid? deletingUserId)
{
var orgUsers = await _organizationUserRepository.GetManyAsync(organizationUsersId);
var filteredUsers = orgUsers.Where(u => u.OrganizationId == organizationId)
.ToList();
var result = await RemoveUsersInternalAsync(organizationId, organizationUserIds, deletingUserId, eventSystemUser: null);

if (!filteredUsers.Any())
var removedUsers = result.Where(r => string.IsNullOrEmpty(r.ErrorMessage)).Select(r => r.OrganizationUser).ToList();
if (removedUsers.Any())
{
throw new BadRequestException("Users invalid.");
DateTime? eventDate = _timeProvider.GetUtcNow().UtcDateTime;
await _eventService.LogOrganizationUserEventsAsync(
removedUsers.Select(ou => (ou, EventType.OrganizationUser_Removed, eventDate)));
}

if (!await _hasConfirmedOwnersExceptQuery.HasConfirmedOwnersExceptAsync(organizationId, organizationUsersId))
{
throw new BadRequestException("Organization must have at least one confirmed owner.");
}
return result.Select(r => (r.OrganizationUser.Id, r.ErrorMessage));
}

var deletingUserIsOwner = false;
if (deletingUserId.HasValue)
{
deletingUserIsOwner = await _currentContext.OrganizationOwner(organizationId);
}
public async Task<IEnumerable<(Guid OrganizationUserId, string ErrorMessage)>> RemoveUsersAsync(
Guid organizationId, IEnumerable<Guid> organizationUserIds, EventSystemUser eventSystemUser)
{
var result = await RemoveUsersInternalAsync(organizationId, organizationUserIds, deletingUserId: null, eventSystemUser);

var result = new List<Tuple<OrganizationUser, string>>();
var deletedUserIds = new List<Guid>();
foreach (var orgUser in filteredUsers)
var removedUsers = result.Where(r => string.IsNullOrEmpty(r.ErrorMessage)).Select(r => r.OrganizationUser).ToList();
if (removedUsers.Any())
{
try
{
if (deletingUserId.HasValue && orgUser.UserId == deletingUserId)
{
throw new BadRequestException("You cannot remove yourself.");
}

if (orgUser.Type == OrganizationUserType.Owner && deletingUserId.HasValue && !deletingUserIsOwner)
{
throw new BadRequestException("Only owners can delete other owners.");
}

await _eventService.LogOrganizationUserEventAsync(orgUser, EventType.OrganizationUser_Removed);

if (orgUser.UserId.HasValue)
{
await DeleteAndPushUserRegistrationAsync(organizationId, orgUser.UserId.Value);
}
result.Add(Tuple.Create(orgUser, ""));
deletedUserIds.Add(orgUser.Id);
}
catch (BadRequestException e)
{
result.Add(Tuple.Create(orgUser, e.Message));
}

await _organizationUserRepository.DeleteManyAsync(deletedUserIds);
DateTime? eventDate = _timeProvider.GetUtcNow().UtcDateTime;
await _eventService.LogOrganizationUserEventsAsync(
removedUsers.Select(ou => (ou, EventType.OrganizationUser_Removed, eventSystemUser, eventDate)));
}

return result;
return result.Select(r => (r.OrganizationUser.Id, r.ErrorMessage));
}

private void ValidateDeleteUser(Guid organizationId, OrganizationUser orgUser)
private void ValidateRemoveUser(Guid organizationId, OrganizationUser orgUser)
{
if (orgUser == null || orgUser.OrganizationId != organizationId)
{
throw new NotFoundException("User not found.");
throw new NotFoundException(UserNotFoundErrorMessage);
}
}

private async Task RepositoryDeleteUserAsync(OrganizationUser orgUser, Guid? deletingUserId)
private async Task RepositoryRemoveUserAsync(OrganizationUser orgUser, Guid? deletingUserId, EventSystemUser? eventSystemUser)
{
if (deletingUserId.HasValue && orgUser.UserId == deletingUserId.Value)
{
throw new BadRequestException("You cannot remove yourself.");
throw new BadRequestException(RemoveYourselfErrorMessage);
}

if (orgUser.Type == OrganizationUserType.Owner)
{
if (deletingUserId.HasValue && !await _currentContext.OrganizationOwner(orgUser.OrganizationId))
{
throw new BadRequestException("Only owners can delete other owners.");
throw new BadRequestException(RemoveOwnerByNonOwnerErrorMessage);
}

if (!await _hasConfirmedOwnersExceptQuery.HasConfirmedOwnersExceptAsync(orgUser.OrganizationId, new[] { orgUser.Id }, includeProvider: true))
{
throw new BadRequestException("Organization must have at least one confirmed owner.");
throw new BadRequestException(RemoveLastConfirmedOwnerErrorMessage);
}
}

if (_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning) && deletingUserId.HasValue && eventSystemUser == null)
{
var managementStatus = await _getOrganizationUsersManagementStatusQuery.GetUsersOrganizationManagementStatusAsync(orgUser.OrganizationId, new[] { orgUser.Id });
if (managementStatus.TryGetValue(orgUser.Id, out var isManaged) && isManaged)
{
throw new BadRequestException(RemoveClaimedAccountErrorMessage);
}
}

Expand All @@ -177,4 +174,70 @@ await _pushRegistrationService.DeleteUserRegistrationOrganizationAsync(devices,
organizationId.ToString());
await _pushNotificationService.PushSyncOrgKeysAsync(userId);
}

private async Task<IEnumerable<(OrganizationUser OrganizationUser, string ErrorMessage)>> RemoveUsersInternalAsync(
Guid organizationId, IEnumerable<Guid> organizationUsersId, Guid? deletingUserId, EventSystemUser? eventSystemUser)
{
var orgUsers = await _organizationUserRepository.GetManyAsync(organizationUsersId);
var filteredUsers = orgUsers.Where(u => u.OrganizationId == organizationId).ToList();

if (!filteredUsers.Any())
{
throw new BadRequestException(UsersInvalidErrorMessage);
}

if (!await _hasConfirmedOwnersExceptQuery.HasConfirmedOwnersExceptAsync(organizationId, organizationUsersId))
{
throw new BadRequestException(RemoveLastConfirmedOwnerErrorMessage);
}

var deletingUserIsOwner = false;
if (deletingUserId.HasValue)
{
deletingUserIsOwner = await _currentContext.OrganizationOwner(organizationId);
}

var managementStatus = _featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning) && deletingUserId.HasValue && eventSystemUser == null
? await _getOrganizationUsersManagementStatusQuery.GetUsersOrganizationManagementStatusAsync(organizationId, filteredUsers.Select(u => u.Id))
: filteredUsers.ToDictionary(u => u.Id, u => false);
var result = new List<(OrganizationUser OrganizationUser, string ErrorMessage)>();
foreach (var orgUser in filteredUsers)
{
try
{
if (deletingUserId.HasValue && orgUser.UserId == deletingUserId)
{
throw new BadRequestException(RemoveYourselfErrorMessage);
}

if (orgUser.Type == OrganizationUserType.Owner && deletingUserId.HasValue && !deletingUserIsOwner)
{
throw new BadRequestException(RemoveOwnerByNonOwnerErrorMessage);
}

if (managementStatus.TryGetValue(orgUser.Id, out var isManaged) && isManaged)
{
throw new BadRequestException(RemoveClaimedAccountErrorMessage);
}

result.Add((orgUser, string.Empty));
}
catch (BadRequestException e)
{
result.Add((orgUser, e.Message));
}
}

var organizationUsersToRemove = result.Where(r => string.IsNullOrEmpty(r.ErrorMessage)).Select(r => r.OrganizationUser).ToList();
if (organizationUsersToRemove.Any())
{
await _organizationUserRepository.DeleteManyAsync(organizationUsersToRemove.Select(ou => ou.Id));
foreach (var orgUser in organizationUsersToRemove.Where(ou => ou.UserId.HasValue))
{
await DeleteAndPushUserRegistrationAsync(organizationId, orgUser.UserId.Value);
}
}

return result;
}
}
Loading

0 comments on commit 674bd1e

Please sign in to comment.