-
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
Changes from 29 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
d0dd776
273754a
2f3da19
1e7c17f
66d5831
772b057
0253c6f
8669e90
d485305
6c214b1
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 IsOrganizationOwner { 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; | ||
IsOrganizationOwner = isOrganizationOwner; | ||
} | ||
|
||
public Guid? UserId { get; } | ||
public bool IsOrganizationOwner { 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 IsOrganizationOwner => 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) { } | ||
} |
eliykat marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,115 @@ | ||
๏ปฟ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.SetOrganizationUsersStatusAsync(request.OrganizationUsers.Select(x => x.Id), | ||
OrganizationUserStatusType.Revoked); | ||
|
||
if (request.ActionPerformedBy.GetType() == typeof(StandardUser)) | ||
jrmccannon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
await eventService.LogOrganizationUserEventsAsync( | ||
request.OrganizationUsers.Select(x => GetRevokedUserEventTuple(x, timeProvider.GetUtcNow()))); | ||
} | ||
else if (request.ActionPerformedBy is SystemUser { SystemUserType: not null } loggableSystem) | ||
{ | ||
await eventService.LogOrganizationUserEventsAsync( | ||
request.OrganizationUsers.Select(x => | ||
GetRevokedUserEventBySystemUserTuple(x, loggableSystem.SystemUserType.Value, | ||
timeProvider.GetUtcNow()))); | ||
} | ||
Check warning on line 47 in src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs
|
||
jrmccannon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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 54 in src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs
|
||
|
||
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 58 in src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs
|
||
eliykat marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
private async Task<CommandResult> ValidateAsync(RevokeOrganizationUsersRequest request) | ||
{ | ||
if (!PerformedByIsAnExpectedType(request.ActionPerformedBy)) | ||
{ | ||
return new CommandResult([ErrorRequestedByWasNotValid]); | ||
jrmccannon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
if (request.ActionPerformedBy is StandardUser loggableUser | ||
&& request.OrganizationUsers.Any(x => x.UserId == loggableUser.UserId)) | ||
jrmccannon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
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 { IsOrganizationOwner: false } && organizationUser.Type == OrganizationUserType.Owner; | ||
|
||
private static bool IsActingUserAllowedToRevokeOwner(OrganizationUserUserDetails organizationUser, | ||
StandardUser requestingOrganizationUser) => organizationUser is { Type: OrganizationUserType.Owner } | ||
&& requestingOrganizationUser.IsOrganizationOwner; | ||
Check warning on line 114 in src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs
|
||
jrmccannon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} |
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?
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'm not sure about this method specifically, but generally:
CurrentContext.UserId
is null because it's the organization that's authenticated, not the userA regular call from a client through the private api will always have UserId populated (to my knowledge) because CurrentContext is always initialized in the middleware and the private api always requires user authentication.
What this means for you here I'm not sure. This method seems specifically designed to be called by a user, so throwing if there is no UserId available seems reasonable. And if the UserId is null for some reason, then
_currentContext.OrganizationOwner
is probably not going to be accurate either (as this is also user specific). Really it seems like an invalid state.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.
Added exception throw redirecting caller to use the other method. Not sure which exception to throw, so just going with InvalidOperation for now.