-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[PM-10319] - Revoke Non Complaint Users for 2FA and Single Org Policy Enablement #5037
base: main
Are you sure you want to change the base?
Changes from 32 commits
0a4c88a
ff61b83
49620c7
e7a9a4f
8b534b0
6a36767
f2f2a62
f009db0
595e4b9
d662fff
d64032c
abbd4f5
3917114
da02c89
1c3e4d8
35643c1
d3172c0
fe5af90
95bb1f4
eb693a8
58702d2
47cc545
9427b9f
99f98f2
78cd03d
436bebd
8b89ad6
0014d5c
82b65e0
51d0b12
d4bd369
1d77345
37ca7ff
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 |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
|
||
public enum EventSystemUser : byte | ||
{ | ||
Unknown = 0, | ||
SCIM = 1, | ||
DomainVerification = 2, | ||
PublicApi = 3, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
๏ปฟusing Bit.Core.Enums; | ||
|
||
namespace Bit.Core.AdminConsole.Models.Data; | ||
|
||
public interface IActingUser | ||
{ | ||
Guid? UserId { get; } | ||
bool IsOrganizationOwnerOrProvider { get; } | ||
EventSystemUser? SystemUserType { get; } | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
๏ปฟusing Bit.Core.Enums; | ||
|
||
namespace Bit.Core.AdminConsole.Models.Data; | ||
|
||
public class StandardUser : IActingUser | ||
{ | ||
public StandardUser(Guid userId, bool isOrganizationOwner) | ||
{ | ||
UserId = userId; | ||
IsOrganizationOwnerOrProvider = isOrganizationOwner; | ||
} | ||
|
||
public Guid? UserId { get; } | ||
public bool IsOrganizationOwnerOrProvider { get; } | ||
public EventSystemUser? SystemUserType => throw new Exception($"{nameof(StandardUser)} does not have a {nameof(SystemUserType)}"); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
๏ปฟusing Bit.Core.Enums; | ||
|
||
namespace Bit.Core.AdminConsole.Models.Data; | ||
|
||
public class SystemUser : IActingUser | ||
{ | ||
public SystemUser(EventSystemUser systemUser) | ||
{ | ||
SystemUserType = systemUser; | ||
} | ||
|
||
public Guid? UserId => throw new Exception($"{nameof(SystemUserType)} does not have a {nameof(UserId)}."); | ||
|
||
public bool IsOrganizationOwnerOrProvider => false; | ||
public EventSystemUser? SystemUserType { get; } | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
๏ปฟusing Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Requests; | ||
using Bit.Core.Models.Commands; | ||
|
||
namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; | ||
|
||
public interface IRevokeNonCompliantOrganizationUserCommand | ||
{ | ||
Task<CommandResult> RevokeNonCompliantOrganizationUsersAsync(RevokeOrganizationUsersRequest request); | ||
} |
jrmccannon marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
๏ปฟusing Bit.Core.AdminConsole.Models.Data; | ||
using Bit.Core.Models.Data.Organizations.OrganizationUsers; | ||
|
||
namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Requests; | ||
|
||
public record RevokeOrganizationUsersRequest( | ||
Guid OrganizationId, | ||
IEnumerable<OrganizationUserUserDetails> OrganizationUsers, | ||
r-tome marked this conversation as resolved.
Show resolved
Hide resolved
|
||
IActingUser ActionPerformedBy) | ||
{ | ||
public RevokeOrganizationUsersRequest(Guid organizationId, OrganizationUserUserDetails organizationUser, IActingUser actionPerformedBy) | ||
: this(organizationId, [organizationUser], actionPerformedBy) { } | ||
} |
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. Is there anything in here that is specific to users being revoked for non-compliance with an org policy? Users can also be revoked for other reasons (including because you looked at an admin funny). If this command is a bit more general, it can be used in all cases and can replace the other (various, confusing) OrganizationService implementations. 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. I think it could, but I would want to do that change in a separate PR. I also don't want to change the name until then so we know its safe to be the de facto RevokeOrganizationUserCommand 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. That's cool, can you please make a follow-up tech debt ticket to look into this? 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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,112 @@ | ||
๏ปฟusing Bit.Core.AdminConsole.Models.Data; | ||
using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; | ||
using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Requests; | ||
using Bit.Core.Enums; | ||
using Bit.Core.Models.Commands; | ||
using Bit.Core.Models.Data.Organizations.OrganizationUsers; | ||
using Bit.Core.Repositories; | ||
using Bit.Core.Services; | ||
|
||
namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers; | ||
|
||
public class RevokeNonCompliantOrganizationUserCommand(IOrganizationUserRepository organizationUserRepository, | ||
IEventService eventService, | ||
IHasConfirmedOwnersExceptQuery confirmedOwnersExceptQuery, | ||
TimeProvider timeProvider) : IRevokeNonCompliantOrganizationUserCommand | ||
{ | ||
public const string ErrorCannotRevokeSelf = "You cannot revoke yourself."; | ||
public const string ErrorOnlyOwnersCanRevokeOtherOwners = "Only owners can revoke other owners."; | ||
public const string ErrorUserAlreadyRevoked = "User is already revoked."; | ||
public const string ErrorOrgMustHaveAtLeastOneOwner = "Organization must have at least one confirmed owner."; | ||
public const string ErrorInvalidUsers = "Invalid users."; | ||
public const string ErrorRequestedByWasNotValid = "Action was performed by an unexpected type."; | ||
|
||
public async Task<CommandResult> RevokeNonCompliantOrganizationUsersAsync(RevokeOrganizationUsersRequest request) | ||
{ | ||
var validationResult = await ValidateAsync(request); | ||
|
||
if (validationResult.HasErrors) | ||
{ | ||
return validationResult; | ||
} | ||
|
||
await organizationUserRepository.RevokeOrganizationUserAsync(request.OrganizationUsers.Select(x => x.Id)); | ||
|
||
var now = timeProvider.GetUtcNow(); | ||
|
||
switch (request.ActionPerformedBy) | ||
{ | ||
case StandardUser: | ||
await eventService.LogOrganizationUserEventsAsync( | ||
request.OrganizationUsers.Select(x => GetRevokedUserEventTuple(x, now))); | ||
break; | ||
case SystemUser { SystemUserType: not null } loggableSystem: | ||
await eventService.LogOrganizationUserEventsAsync( | ||
request.OrganizationUsers.Select(x => | ||
GetRevokedUserEventBySystemUserTuple(x, loggableSystem.SystemUserType.Value, now))); | ||
break; | ||
Check warning on line 47 in src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs Codecov / codecov/patchsrc/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs#L44-L47
|
||
} | ||
|
||
return validationResult; | ||
} | ||
|
||
private static (OrganizationUserUserDetails organizationUser, EventType eventType, DateTime? time) GetRevokedUserEventTuple( | ||
OrganizationUserUserDetails organizationUser, DateTimeOffset dateTimeOffset) => | ||
new(organizationUser, EventType.OrganizationUser_Revoked, dateTimeOffset.UtcDateTime); | ||
Check warning on line 55 in src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs Codecov / codecov/patchsrc/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs#L55
|
||
|
||
private static (OrganizationUserUserDetails organizationUser, EventType eventType, EventSystemUser eventSystemUser, DateTime? time) GetRevokedUserEventBySystemUserTuple( | ||
OrganizationUserUserDetails organizationUser, EventSystemUser systemUser, DateTimeOffset dateTimeOffset) => new(organizationUser, | ||
EventType.OrganizationUser_Revoked, systemUser, dateTimeOffset.UtcDateTime); | ||
Check warning on line 59 in src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs Codecov / codecov/patchsrc/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs#L58-L59
|
||
|
||
private async Task<CommandResult> ValidateAsync(RevokeOrganizationUsersRequest request) | ||
{ | ||
if (!PerformedByIsAnExpectedType(request.ActionPerformedBy)) | ||
{ | ||
return new CommandResult(ErrorRequestedByWasNotValid); | ||
} | ||
|
||
if (request.ActionPerformedBy is StandardUser user | ||
&& request.OrganizationUsers.Any(x => x.UserId == user.UserId)) | ||
{ | ||
return new CommandResult(ErrorCannotRevokeSelf); | ||
} | ||
|
||
if (request.OrganizationUsers.Any(x => x.OrganizationId != request.OrganizationId)) | ||
{ | ||
return new CommandResult(ErrorInvalidUsers); | ||
} | ||
|
||
if (!await confirmedOwnersExceptQuery.HasConfirmedOwnersExceptAsync( | ||
request.OrganizationId, | ||
request.OrganizationUsers.Select(x => x.Id))) | ||
{ | ||
return new CommandResult(ErrorOrgMustHaveAtLeastOneOwner); | ||
} | ||
|
||
return request.OrganizationUsers.Aggregate(new CommandResult(), (result, userToRevoke) => | ||
{ | ||
if (IsAlreadyRevoked(userToRevoke)) | ||
{ | ||
result.ErrorMessages.Add($"{ErrorUserAlreadyRevoked} Id: {userToRevoke.Id}"); | ||
return result; | ||
} | ||
|
||
if (NonOwnersCannotRevokeOwners(userToRevoke, request.ActionPerformedBy)) | ||
{ | ||
result.ErrorMessages.Add($"{ErrorOnlyOwnersCanRevokeOtherOwners}"); | ||
return result; | ||
} | ||
|
||
return result; | ||
}); | ||
} | ||
|
||
private static bool PerformedByIsAnExpectedType(IActingUser entity) => entity is SystemUser or StandardUser; | ||
|
||
private static bool IsAlreadyRevoked(OrganizationUserUserDetails organizationUser) => | ||
organizationUser is { Status: OrganizationUserStatusType.Revoked }; | ||
|
||
private static bool NonOwnersCannotRevokeOwners(OrganizationUserUserDetails organizationUser, | ||
IActingUser actingUser) => | ||
actingUser is StandardUser { IsOrganizationOwnerOrProvider: false } && organizationUser.Type == OrganizationUserType.Owner; | ||
} |
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.
Why pass an empty Guid and not the nullable Guid in the first parameter?
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.
I'd rather not have null. I could throw at this point instead of using Guid.Empty. I was just defaulting to Guid.Empty in the event that a caller hasn't initialized the CurrentContext.
Do we know which applications would call this method without the CurrentContext populated with a user?