-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[PM-18718] Refactor Bulk Revoke Users #6601
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
14f3e88
4645222
6a29377
89b8d59
f60f0f5
0ba770d
e5f07de
5527ca6
27ce4b8
2069c9f
cb81d29
24a39d8
a1b2bac
7307454
1303b22
4bb8734
68afb17
aee2734
031c080
805eba9
ce596b2
6e5188f
d6a5813
9d47bf4
929ac41
f648953
f800119
2e80a4d
9b009b2
e1eeaf9
3af7ba4
cd1eca4
34a05f4
72fc706
781d6e6
e5ec8d7
eb5f98e
7fb8910
3ee2bf4
44a5813
a35c789
a9d4dc3
290f391
f02f761
fa18f3b
0487fb4
084849a
46b7bfd
b4d5d50
8dd84e2
b91def1
6d5cb55
0f60466
f27d91c
032b1f8
cdcff93
7a5fd87
c301185
4551891
6187cf0
2b0b1ee
80872b8
6b0af95
64d3b1e
3fb9810
0a75cc0
420b63e
d0fdf44
448dd1a
540c843
eda8486
b5ba1ad
6810137
138f483
3029322
bbd6bbb
bac5026
7ffd23f
0cdc076
69a5fde
6e050ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| ๏ปฟusing Bit.Core.AdminConsole.Utilities.v2; | ||
|
|
||
| namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.RevokeUser.v2; | ||
|
|
||
| public record UserAlreadyRevoked() : BadRequestError("Already revoked."); | ||
| public record CannotRevokeYourself() : BadRequestError("You cannot revoke yourself."); | ||
| public record OnlyOwnersCanRevokeOwners() : BadRequestError("Only owners can revoke other owners."); | ||
| public record MustHaveConfirmedOwner() : BadRequestError("Organization must have at least one confirmed owner."); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| ๏ปฟusing Bit.Core.AdminConsole.Utilities.v2.Results; | ||
|
|
||
| namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.RevokeUser.v2; | ||
|
|
||
| public interface IRevokeOrganizationUserCommand | ||
| { | ||
| Task<IEnumerable<BulkCommandResult>> RevokeUsersAsync(RevokeOrganizationUsersRequest request); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| ๏ปฟusing Bit.Core.AdminConsole.Utilities.v2.Validation; | ||
| using Bit.Core.Entities; | ||
|
|
||
| namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.RevokeUser.v2; | ||
|
|
||
| public interface IRevokeOrganizationUserValidator | ||
| { | ||
| Task<ICollection<ValidationResult<OrganizationUser>>> ValidateAsync(RevokeOrganizationUsersValidationRequest request); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| ๏ปฟusing Bit.Core.AdminConsole.Models.Data; | ||
| using Bit.Core.AdminConsole.Utilities.v2.Results; | ||
| using Bit.Core.Entities; | ||
| using Bit.Core.Enums; | ||
| using Bit.Core.Platform.Push; | ||
| using Bit.Core.Repositories; | ||
| using Bit.Core.Services; | ||
| using Microsoft.Extensions.Logging; | ||
| using OneOf.Types; | ||
|
|
||
| namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.RevokeUser.v2; | ||
|
|
||
| public class RevokeOrganizationUserCommand( | ||
| IOrganizationUserRepository organizationUserRepository, | ||
| IEventService eventService, | ||
| IPushNotificationService pushNotificationService, | ||
| IRevokeOrganizationUserValidator validator, | ||
| TimeProvider timeProvider, | ||
| ILogger<RevokeOrganizationUserCommand> logger) | ||
| : IRevokeOrganizationUserCommand | ||
| { | ||
| public async Task<IEnumerable<BulkCommandResult>> RevokeUsersAsync(RevokeOrganizationUsersRequest request) | ||
| { | ||
| var validationRequest = await CreateValidationRequestsAsync(request); | ||
|
|
||
| var results = await validator.ValidateAsync(validationRequest); | ||
|
|
||
| var validUsers = results.Where(r => r.IsValid).Select(r => r.Request).ToList(); | ||
|
|
||
| await RevokeValidUsersAsync(validUsers); | ||
|
|
||
| await Task.WhenAll( | ||
jrmccannon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| LogRevokedOrganizationUsersAsync(validUsers, request.PerformedBy), | ||
| SendPushNotificationsAsync(validUsers) | ||
| ); | ||
|
|
||
| return results.Select(r => r.Match( | ||
| error => new BulkCommandResult(r.Request.Id, error), | ||
| _ => new BulkCommandResult(r.Request.Id, new None()) | ||
| )); | ||
| } | ||
|
|
||
| private async Task<RevokeOrganizationUsersValidationRequest> CreateValidationRequestsAsync( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ๐จ Method can be made static or simplified The
This would improve code clarity and reduce unnecessary method complexity. |
||
| RevokeOrganizationUsersRequest request) | ||
| { | ||
| var organizationUserToRevoke = await organizationUserRepository | ||
| .GetManyAsync(request.OrganizationUserIdsToRevoke); | ||
|
|
||
| return new RevokeOrganizationUsersValidationRequest( | ||
| request.OrganizationId, | ||
| request.OrganizationUserIdsToRevoke, | ||
| request.PerformedBy, | ||
| organizationUserToRevoke); | ||
| } | ||
|
|
||
| private async Task RevokeValidUsersAsync(ICollection<OrganizationUser> validUsers) | ||
| { | ||
| if (validUsers.Count == 0) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| await organizationUserRepository.RevokeManyByIdAsync(validUsers.Select(u => u.Id)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The v1 implementation called Issue: The v1 approach:
Recommendation:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would not be marked as a success because the repository would throw the exception and the transaction would be rolled back. This would be an "all-or-nothing" batch and the user would get a 500 error. |
||
| } | ||
|
|
||
| private async Task LogRevokedOrganizationUsersAsync( | ||
| ICollection<OrganizationUser> revokedUsers, | ||
| IActingUser actingUser) | ||
| { | ||
| if (revokedUsers.Count == 0) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var eventDate = timeProvider.GetUtcNow().UtcDateTime; | ||
|
|
||
| if (actingUser is SystemUser { SystemUserType: not null }) | ||
| { | ||
| var revokeEventsWithSystem = revokedUsers | ||
| .Select(user => (user, EventType.OrganizationUser_Revoked, actingUser.SystemUserType!.Value, | ||
| (DateTime?)eventDate)) | ||
| .ToList(); | ||
| await eventService.LogOrganizationUserEventsAsync(revokeEventsWithSystem); | ||
| } | ||
| else | ||
| { | ||
| var revokeEvents = revokedUsers | ||
| .Select(user => (user, EventType.OrganizationUser_Revoked, (DateTime?)eventDate)) | ||
| .ToList(); | ||
| await eventService.LogOrganizationUserEventsAsync(revokeEvents); | ||
| } | ||
| } | ||
|
|
||
| private async Task SendPushNotificationsAsync(ICollection<OrganizationUser> revokedUsers) | ||
| { | ||
| var userIdsToNotify = revokedUsers | ||
| .Where(user => user.UserId.HasValue) | ||
| .Select(user => user.UserId!.Value) | ||
| .Distinct() | ||
| .ToList(); | ||
|
|
||
| foreach (var userId in userIdsToNotify) | ||
| { | ||
| try | ||
| { | ||
| await pushNotificationService.PushSyncOrgKeysAsync(userId); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| logger.LogWarning(ex, "Failed to send push notification for user {UserId}.", userId); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| ๏ปฟusing Bit.Core.AdminConsole.Models.Data; | ||
| using Bit.Core.Entities; | ||
|
|
||
| namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.RevokeUser.v2; | ||
|
|
||
| public record RevokeOrganizationUsersRequest( | ||
| Guid OrganizationId, | ||
| ICollection<Guid> OrganizationUserIdsToRevoke, | ||
| IActingUser PerformedBy | ||
| ); | ||
|
|
||
| public record RevokeOrganizationUsersValidationRequest( | ||
| Guid OrganizationId, | ||
| ICollection<Guid> OrganizationUserIdsToRevoke, | ||
| IActingUser PerformedBy, | ||
| ICollection<OrganizationUser> OrganizationUsersToRevoke | ||
| ) : RevokeOrganizationUsersRequest(OrganizationId, OrganizationUserIdsToRevoke, PerformedBy); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| ๏ปฟusing Bit.Core.AdminConsole.Models.Data; | ||
| using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; | ||
| using Bit.Core.AdminConsole.Utilities.v2.Validation; | ||
| using Bit.Core.Entities; | ||
| using Bit.Core.Enums; | ||
| using static Bit.Core.AdminConsole.Utilities.v2.Validation.ValidationResultHelpers; | ||
|
|
||
| namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.RevokeUser.v2; | ||
|
|
||
| public class RevokeOrganizationUsersValidator(IHasConfirmedOwnersExceptQuery hasConfirmedOwnersExceptQuery) | ||
| : IRevokeOrganizationUserValidator | ||
| { | ||
| public async Task<ICollection<ValidationResult<OrganizationUser>>> ValidateAsync( | ||
| RevokeOrganizationUsersValidationRequest request) | ||
| { | ||
| var hasRemainingOwner = await hasConfirmedOwnersExceptQuery.HasConfirmedOwnersExceptAsync(request.OrganizationId, | ||
| request.OrganizationUsersToRevoke.Select(x => x.Id) // users excluded because they are going to be revoked | ||
| ); | ||
|
|
||
| return request.OrganizationUsersToRevoke.Select(x => | ||
| { | ||
| return x switch | ||
| { | ||
| _ when request.PerformedBy is not SystemUser | ||
| && x.UserId is not null | ||
| && x.UserId == request.PerformedBy.UserId => | ||
| Invalid(x, new CannotRevokeYourself()), | ||
| { Status: OrganizationUserStatusType.Revoked } => | ||
| Invalid(x, new UserAlreadyRevoked()), | ||
| { Type: OrganizationUserType.Owner } when !hasRemainingOwner => | ||
| Invalid(x, new MustHaveConfirmedOwner()), | ||
| { Type: OrganizationUserType.Owner } when !request.PerformedBy.IsOrganizationOwnerOrProvider => | ||
| Invalid(x, new OnlyOwnersCanRevokeOwners()), | ||
|
|
||
| _ => Valid(x) | ||
| }; | ||
| }).ToList(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
๐ญ The
MustHaveConfirmedOwnererror is defined but never used in the codebase. The v1 implementation checked for this condition withHasConfirmedOwnersExceptAsync, but the v2 implementation removed this check entirely.Why this matters:
This could allow revoking the last owner of an organization, which would leave the organization in an unmanageable state. The v1 implementation prevented this at lines 62-66 and 84-87.
Recommendation:
Either implement the check in the validator and use this error, or remove the unused error definition. Based on the v1 implementation and business logic, the check should be implemented.