From 0a4c88a295add61a82926db8f46d73aae7c69642 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Thu, 7 Nov 2024 15:50:19 -0600 Subject: [PATCH 01/49] Revoking users when enabling single org and 2fa policies. Fixing tests. --- .../Models/Api/Response/CommandResult.cs | 10 ++ .../AdminConsole/Models/Data/IActingUser.cs | 10 ++ .../AdminConsole/Models/Data/StandardUser.cs | 16 +++ .../AdminConsole/Models/Data/SystemUser.cs | 16 +++ .../Requests/RevokeOrganizationUser.cs | 13 ++ ...vokeNonCompliantOrganizationUserCommand.cs | 120 ++++++++++++++++++ .../Policies/Models/PolicyUpdate.cs | 2 + .../SingleOrgPolicyValidator.cs | 51 ++++++-- .../TwoFactorAuthenticationPolicyValidator.cs | 84 +++++++++--- .../IOrganizationUserRepository.cs | 2 + .../Services/Implementations/PolicyService.cs | 13 +- ...OrganizationServiceCollectionExtensions.cs | 1 + .../OrganizationUserRepository.cs | 10 ++ .../OrganizationUserRepository.cs | 10 ++ ...OrganizationUser_SetStatusForUsersById.sql | 13 ++ .../AutoFixture/PolicyUpdateFixtures.cs | 4 +- ...onCompliantOrganizationUserCommandTests.cs | 24 ++++ .../SingleOrgPolicyValidatorTests.cs | 5 + ...actorAuthenticationPolicyValidatorTests.cs | 11 ++ 19 files changed, 383 insertions(+), 32 deletions(-) create mode 100644 src/Core/AdminConsole/Models/Api/Response/CommandResult.cs create mode 100644 src/Core/AdminConsole/Models/Data/IActingUser.cs create mode 100644 src/Core/AdminConsole/Models/Data/StandardUser.cs create mode 100644 src/Core/AdminConsole/Models/Data/SystemUser.cs create mode 100644 src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Requests/RevokeOrganizationUser.cs create mode 100644 src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs create mode 100644 src/Sql/dbo/Stored Procedures/OrganizationUser_SetStatusForUsersById.sql create mode 100644 test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs diff --git a/src/Core/AdminConsole/Models/Api/Response/CommandResult.cs b/src/Core/AdminConsole/Models/Api/Response/CommandResult.cs new file mode 100644 index 000000000000..e98a18d5ca99 --- /dev/null +++ b/src/Core/AdminConsole/Models/Api/Response/CommandResult.cs @@ -0,0 +1,10 @@ +namespace Bit.Core.AdminConsole.Models.Api.Response; + +public class CommandResult(IEnumerable errors) +{ + public bool Success => ErrorMessages.Count == 0; + public bool HasErrors => ErrorMessages.Count > 0; + public List ErrorMessages { get; } = errors.ToList(); + + public CommandResult() : this([]) { } +} diff --git a/src/Core/AdminConsole/Models/Data/IActingUser.cs b/src/Core/AdminConsole/Models/Data/IActingUser.cs new file mode 100644 index 000000000000..4e678bb1413d --- /dev/null +++ b/src/Core/AdminConsole/Models/Data/IActingUser.cs @@ -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; } +} diff --git a/src/Core/AdminConsole/Models/Data/StandardUser.cs b/src/Core/AdminConsole/Models/Data/StandardUser.cs new file mode 100644 index 000000000000..7b536741f5bb --- /dev/null +++ b/src/Core/AdminConsole/Models/Data/StandardUser.cs @@ -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)}"); +} diff --git a/src/Core/AdminConsole/Models/Data/SystemUser.cs b/src/Core/AdminConsole/Models/Data/SystemUser.cs new file mode 100644 index 000000000000..e73c74c9caee --- /dev/null +++ b/src/Core/AdminConsole/Models/Data/SystemUser.cs @@ -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; } +} diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Requests/RevokeOrganizationUser.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Requests/RevokeOrganizationUser.cs new file mode 100644 index 000000000000..7dbb21dd854d --- /dev/null +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Requests/RevokeOrganizationUser.cs @@ -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 RevokeOrganizationUsers( + Guid OrganizationId, + IEnumerable OrganizationUsers, + IActingUser ActionPerformedBy) +{ + public RevokeOrganizationUsers(Guid organizationId, OrganizationUserUserDetails organizationUser, IActingUser actionPerformedBy) + : this(organizationId, [organizationUser], actionPerformedBy) { } +} diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs new file mode 100644 index 000000000000..e7c1943f944a --- /dev/null +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs @@ -0,0 +1,120 @@ +using Bit.Core.AdminConsole.Models.Api.Response; +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.Data.Organizations.OrganizationUsers; +using Bit.Core.Repositories; +using Bit.Core.Services; + +namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers; + +public interface IRevokeNonCompliantOrganizationUserCommand +{ + Task RevokeNonCompliantOrganizationUsersAsync(RevokeOrganizationUsers request); +} + +public class RevokeNonCompliantOrganizationUserCommand(IOrganizationUserRepository organizationUserRepository, + IEventService eventService, + IHasConfirmedOwnersExceptQuery confirmedOwnersExceptQuery, + TimeProvider timeProvider) : IRevokeNonCompliantOrganizationUserCommand +{ + private const string _cannotRevokeSelfMessage = "You cannot revoke yourself."; + private const string _onlyOwnersCanRevokeOtherOwners = "Only owners can revoke other owners."; + private const string _userAlreadyRevoked = "User is already revoked."; + private const string _orgMustHaveAtLeastOneOwner = "Organization must have at least one confirmed owner."; + private const string _invalidUsers = "Invalid users."; + private const string _requestedByWasNotValid = "Action was performed by an unexpected type."; + + public async Task RevokeNonCompliantOrganizationUsersAsync(RevokeOrganizationUsers 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)) + { + 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()))); + } + + return validationResult; + } + + private static (OrganizationUserUserDetails organizationUser, EventType eventType, DateTime? time) GetRevokedUserEventTuple( + OrganizationUserUserDetails organizationUser, DateTimeOffset dateTimeOffset) => new(organizationUser, + EventType.OrganizationUser_Revoked, dateTimeOffset.UtcDateTime); + + 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); + + private async Task ValidateAsync(RevokeOrganizationUsers request) + { + if (PerformedByIsAnExpectedType(request.ActionPerformedBy)) + { + return new CommandResult([_requestedByWasNotValid]); + } + + if (request.ActionPerformedBy is StandardUser loggableUser + && request.OrganizationUsers.Any(x => x.UserId == loggableUser.UserId)) + { + return new CommandResult([_cannotRevokeSelfMessage]); + } + + if (request.OrganizationUsers.Any(x => x.OrganizationId != request.OrganizationId)) + { + return new CommandResult([_invalidUsers]); + } + + if (!await confirmedOwnersExceptQuery.HasConfirmedOwnersExceptAsync( + request.OrganizationId, + request.OrganizationUsers.Select(x => x.Id))) + { + return new CommandResult([_orgMustHaveAtLeastOneOwner]); + } + + return request.OrganizationUsers.Aggregate(new CommandResult(), (result, userToRevoke) => + { + if (IsAlreadyRevoked(userToRevoke)) + { + result.ErrorMessages.Add($"{_userAlreadyRevoked} Id: {userToRevoke.Id}"); + return result; + } + + if (IsNonOwnerRevokingAnOwner(userToRevoke, request.ActionPerformedBy)) + { + result.ErrorMessages.Add($"{_onlyOwnersCanRevokeOtherOwners}"); + 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 IsNonOwnerRevokingAnOwner(OrganizationUserUserDetails organizationUser, + IActingUser requestingUser) => requestingUser is StandardUser standardUser && + IsActingUserAllowedToRevokeOwner(organizationUser, standardUser); + + private static bool IsActingUserAllowedToRevokeOwner(OrganizationUserUserDetails organizationUser, + StandardUser requestingOrganizationUser) => organizationUser is { Type: OrganizationUserType.Owner } + && requestingOrganizationUser.IsOrganizationOwner; +} diff --git a/src/Core/AdminConsole/OrganizationFeatures/Policies/Models/PolicyUpdate.cs b/src/Core/AdminConsole/OrganizationFeatures/Policies/Models/PolicyUpdate.cs index 117a7ec73396..d1a52f008011 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Policies/Models/PolicyUpdate.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Policies/Models/PolicyUpdate.cs @@ -1,6 +1,7 @@ #nullable enable using Bit.Core.AdminConsole.Enums; +using Bit.Core.AdminConsole.Models.Data; using Bit.Core.AdminConsole.Models.Data.Organizations.Policies; using Bit.Core.Utilities; @@ -15,6 +16,7 @@ public record PolicyUpdate public PolicyType Type { get; set; } public string? Data { get; set; } public bool Enabled { get; set; } + public IActingUser? PerformedBy { get; set; } public T GetDataModel() where T : IPolicyDataModel, new() { diff --git a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs index 3e1f8d26c81d..883bf1bb63a4 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs @@ -2,7 +2,10 @@ using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.Enums; +using Bit.Core.AdminConsole.Models.Data; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Requests; using Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models; using Bit.Core.Auth.Enums; using Bit.Core.Auth.Repositories; @@ -24,6 +27,8 @@ public class SingleOrgPolicyValidator : IPolicyValidator private readonly ISsoConfigRepository _ssoConfigRepository; private readonly ICurrentContext _currentContext; private readonly IRemoveOrganizationUserCommand _removeOrganizationUserCommand; + private readonly IRevokeNonCompliantOrganizationUserCommand _revokeNonCompliantOrganizationUserCommand; + private readonly IFeatureService _featureService; public SingleOrgPolicyValidator( IOrganizationUserRepository organizationUserRepository, @@ -31,7 +36,9 @@ public SingleOrgPolicyValidator( IOrganizationRepository organizationRepository, ISsoConfigRepository ssoConfigRepository, ICurrentContext currentContext, - IRemoveOrganizationUserCommand removeOrganizationUserCommand) + IRemoveOrganizationUserCommand removeOrganizationUserCommand, + IRevokeNonCompliantOrganizationUserCommand revokeNonCompliantOrganizationUserCommand, + IFeatureService featureService) { _organizationUserRepository = organizationUserRepository; _mailService = mailService; @@ -39,6 +46,8 @@ public SingleOrgPolicyValidator( _ssoConfigRepository = ssoConfigRepository; _currentContext = currentContext; _removeOrganizationUserCommand = removeOrganizationUserCommand; + _revokeNonCompliantOrganizationUserCommand = revokeNonCompliantOrganizationUserCommand; + _featureService = featureService; } public IEnumerable RequiredPolicies => []; @@ -73,17 +82,41 @@ private async Task RemoveNonCompliantUsersAsync(Guid organizationId) var userOrgs = await _organizationUserRepository.GetManyByManyUsersAsync( removableOrgUsers.Select(ou => ou.UserId!.Value)); - foreach (var orgUser in removableOrgUsers) + + if (_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning)) + { + var isOwner = await _currentContext.OrganizationOwner(organizationId); + + var revocableUsers = removableOrgUsers.Where(nonCompliantUser => orgUsers.Any(orgUser => + nonCompliantUser.UserId == orgUser.UserId + && nonCompliantUser.OrganizationId != org.Id + && nonCompliantUser.Status != OrganizationUserStatusType.Invited)); + + var commandResult = await _revokeNonCompliantOrganizationUserCommand.RevokeNonCompliantOrganizationUsersAsync( + new RevokeOrganizationUsers(organizationId, revocableUsers, new StandardUser(savingUserId ?? Guid.Empty, isOwner))); + + // might need to throw if list of errors returned + if (commandResult.HasErrors) + { + throw new BadRequestException(string.Join(", ", commandResult.ErrorMessages)); + } + + // TODO send email + } + else { - if (userOrgs.Any(ou => ou.UserId == orgUser.UserId - && ou.OrganizationId != org.Id - && ou.Status != OrganizationUserStatusType.Invited)) + foreach (var orgUser in removableOrgUsers) { - await _removeOrganizationUserCommand.RemoveUserAsync(organizationId, orgUser.Id, - savingUserId); + if (userOrgs.Any(ou => ou.UserId == orgUser.UserId + && ou.OrganizationId != org.Id + && ou.Status != OrganizationUserStatusType.Invited)) + { + await _removeOrganizationUserCommand.RemoveUserAsync(organizationId, orgUser.Id, + savingUserId); - await _mailService.SendOrganizationUserRemovedForPolicySingleOrgEmailAsync( - org.DisplayName(), orgUser.Email); + await _mailService.SendOrganizationUserRemovedForPolicySingleOrgEmailAsync( + org.DisplayName(), orgUser.Email); + } } } } diff --git a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs index ef896bbb9b57..3d0b2c7ba87b 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs @@ -2,12 +2,16 @@ using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.Enums; +using Bit.Core.AdminConsole.Models.Data; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Requests; using Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models; using Bit.Core.Auth.UserFeatures.TwoFactorAuth.Interfaces; using Bit.Core.Context; using Bit.Core.Enums; using Bit.Core.Exceptions; +using Bit.Core.Models.Data.Organizations.OrganizationUsers; using Bit.Core.Repositories; using Bit.Core.Services; @@ -21,6 +25,10 @@ public class TwoFactorAuthenticationPolicyValidator : IPolicyValidator private readonly ICurrentContext _currentContext; private readonly ITwoFactorIsEnabledQuery _twoFactorIsEnabledQuery; private readonly IRemoveOrganizationUserCommand _removeOrganizationUserCommand; + private readonly IFeatureService _featureService; + private readonly IRevokeNonCompliantOrganizationUserCommand _revokeNonCompliantOrganizationUserCommand; + + private const string _nonCompliantMembersWillLoseAccess = "Policy could not be enabled. Non-compliant members will lose access to their accounts. Identify members without two-step login from the policies column in the members page."; public PolicyType Type => PolicyType.TwoFactorAuthentication; public IEnumerable RequiredPolicies => []; @@ -31,7 +39,9 @@ public TwoFactorAuthenticationPolicyValidator( IOrganizationRepository organizationRepository, ICurrentContext currentContext, ITwoFactorIsEnabledQuery twoFactorIsEnabledQuery, - IRemoveOrganizationUserCommand removeOrganizationUserCommand) + IRemoveOrganizationUserCommand removeOrganizationUserCommand, + IFeatureService featureService, + IRevokeNonCompliantOrganizationUserCommand revokeNonCompliantOrganizationUserCommand) { _organizationUserRepository = organizationUserRepository; _mailService = mailService; @@ -39,6 +49,8 @@ public TwoFactorAuthenticationPolicyValidator( _currentContext = currentContext; _twoFactorIsEnabledQuery = twoFactorIsEnabledQuery; _removeOrganizationUserCommand = removeOrganizationUserCommand; + _featureService = featureService; + _revokeNonCompliantOrganizationUserCommand = revokeNonCompliantOrganizationUserCommand; } public async Task OnSaveSideEffectsAsync(PolicyUpdate policyUpdate, Policy? currentPolicy) @@ -55,33 +67,65 @@ private async Task RemoveNonCompliantUsersAsync(Guid organizationId) var savingUserId = _currentContext.UserId; var orgUsers = await _organizationUserRepository.GetManyDetailsByOrganizationAsync(organizationId); - var organizationUsersTwoFactorEnabled = await _twoFactorIsEnabledQuery.TwoFactorIsEnabledAsync(orgUsers); - var removableOrgUsers = orgUsers.Where(ou => - ou.Status != OrganizationUserStatusType.Invited && ou.Status != OrganizationUserStatusType.Revoked && - ou.Type != OrganizationUserType.Owner && ou.Type != OrganizationUserType.Admin && - ou.UserId != savingUserId); - - // Reorder by HasMasterPassword to prioritize checking users without a master if they have 2FA enabled - foreach (var orgUser in removableOrgUsers.OrderBy(ou => ou.HasMasterPassword)) + + // this can get moved into the private method after AccountDeprovisiong flag check is removed. + var organizationUsersTwoFactorEnabled = (await _twoFactorIsEnabledQuery.TwoFactorIsEnabledAsync(orgUsers)).ToList(); + + var revocableOrgUsers = orgUsers.Where(ou => + ou.Status != OrganizationUserStatusType.Invited && ou.Status != OrganizationUserStatusType.Revoked && + ou.Type != OrganizationUserType.Owner && ou.Type != OrganizationUserType.Admin && + ou.UserId != savingUserId) + .ToList(); + + if (_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning)) + { + if (DoMembersNotHaveAPasswordWithTwoFactorAuthenticationEnabled(revocableOrgUsers, organizationUsersTwoFactorEnabled)) + { + throw new BadRequestException(_nonCompliantMembersWillLoseAccess); + } + + var result = await _revokeNonCompliantOrganizationUserCommand.RevokeNonCompliantOrganizationUsersAsync( + new RevokeOrganizationUsers(organizationId, revocableOrgUsers, + new StandardUser(savingUserId ?? Guid.Empty, + await _currentContext.OrganizationOwner(organizationId)))); + + if (result.HasErrors) + { + throw new BadRequestException(string.Join(", ", result.ErrorMessages)); + } + + // TODO Send out User Revoked Email + } + else { - var userTwoFactorEnabled = organizationUsersTwoFactorEnabled.FirstOrDefault(u => u.user.Id == orgUser.Id) - .twoFactorIsEnabled; - if (!userTwoFactorEnabled) + // Reorder by HasMasterPassword to prioritize checking users without a master if they have 2FA enabled + foreach (var orgUser in revocableOrgUsers.OrderBy(ou => ou.HasMasterPassword)) { - if (!orgUser.HasMasterPassword) + var userTwoFactorEnabled = organizationUsersTwoFactorEnabled + .FirstOrDefault(u => u.user.Id == orgUser.Id).twoFactorIsEnabled; + if (!userTwoFactorEnabled) { - throw new BadRequestException( - "Policy could not be enabled. Non-compliant members will lose access to their accounts. Identify members without two-step login from the policies column in the members page."); - } + if (!orgUser.HasMasterPassword) + { + throw new BadRequestException(_nonCompliantMembersWillLoseAccess); + } - await _removeOrganizationUserCommand.RemoveUserAsync(organizationId, orgUser.Id, - savingUserId); + await _removeOrganizationUserCommand.RemoveUserAsync(organizationId, orgUser.Id, + savingUserId); - await _mailService.SendOrganizationUserRemovedForPolicyTwoStepEmailAsync( - org!.DisplayName(), orgUser.Email); + await _mailService.SendOrganizationUserRemovedForPolicyTwoStepEmailAsync( + org!.DisplayName(), orgUser.Email); + } } } } + private static bool DoMembersNotHaveAPasswordWithTwoFactorAuthenticationEnabled( + IEnumerable orgUserDetails, + IEnumerable<(OrganizationUserUserDetails user, bool isTwoFactorEnabled)> organizationUsersTwoFactorEnabled) => + orgUserDetails.Any(x => + !x.HasMasterPassword && !organizationUsersTwoFactorEnabled.FirstOrDefault(u => u.user.Id == x.Id) + .isTwoFactorEnabled); + public Task ValidateAsync(PolicyUpdate policyUpdate, Policy? currentPolicy) => Task.FromResult(""); } diff --git a/src/Core/AdminConsole/Repositories/IOrganizationUserRepository.cs b/src/Core/AdminConsole/Repositories/IOrganizationUserRepository.cs index a3a68b5de264..f8007b648eba 100644 --- a/src/Core/AdminConsole/Repositories/IOrganizationUserRepository.cs +++ b/src/Core/AdminConsole/Repositories/IOrganizationUserRepository.cs @@ -58,4 +58,6 @@ UpdateEncryptedDataForKeyRotation UpdateForKeyRotation(Guid userId, /// Returns a list of OrganizationUsers with email domains that match one of the Organization's claimed domains. /// Task> GetManyByOrganizationWithClaimedDomainsAsync(Guid organizationId); + + Task SetOrganizationUsersStatusAsync(IEnumerable userIds, OrganizationUserStatusType status); } diff --git a/src/Core/AdminConsole/Services/Implementations/PolicyService.cs b/src/Core/AdminConsole/Services/Implementations/PolicyService.cs index 072aa8283489..7f56664b93fd 100644 --- a/src/Core/AdminConsole/Services/Implementations/PolicyService.cs +++ b/src/Core/AdminConsole/Services/Implementations/PolicyService.cs @@ -1,5 +1,6 @@ using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.Enums; +using Bit.Core.AdminConsole.Models.Data; using Bit.Core.AdminConsole.Models.Data.Organizations.Policies; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationDomains.Interfaces; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; @@ -9,6 +10,7 @@ using Bit.Core.Auth.Enums; using Bit.Core.Auth.Repositories; using Bit.Core.Auth.UserFeatures.TwoFactorAuth.Interfaces; +using Bit.Core.Context; using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Exceptions; @@ -34,6 +36,7 @@ public class PolicyService : IPolicyService private readonly ISavePolicyCommand _savePolicyCommand; private readonly IRemoveOrganizationUserCommand _removeOrganizationUserCommand; private readonly IOrganizationHasVerifiedDomainsQuery _organizationHasVerifiedDomainsQuery; + private readonly ICurrentContext _currentContext; public PolicyService( IApplicationCacheService applicationCacheService, @@ -48,7 +51,8 @@ public PolicyService( IFeatureService featureService, ISavePolicyCommand savePolicyCommand, IRemoveOrganizationUserCommand removeOrganizationUserCommand, - IOrganizationHasVerifiedDomainsQuery organizationHasVerifiedDomainsQuery) + IOrganizationHasVerifiedDomainsQuery organizationHasVerifiedDomainsQuery, + ICurrentContext currentContext) { _applicationCacheService = applicationCacheService; _eventService = eventService; @@ -63,6 +67,7 @@ public PolicyService( _savePolicyCommand = savePolicyCommand; _removeOrganizationUserCommand = removeOrganizationUserCommand; _organizationHasVerifiedDomainsQuery = organizationHasVerifiedDomainsQuery; + _currentContext = currentContext; } public async Task SaveAsync(Policy policy, Guid? savingUserId) @@ -70,12 +75,16 @@ public async Task SaveAsync(Policy policy, Guid? savingUserId) if (_featureService.IsEnabled(FeatureFlagKeys.Pm13322AddPolicyDefinitions)) { // Transitional mapping - this will be moved to callers once the feature flag is removed + // TODO make sure to populate with SystemUser if not an actual user var policyUpdate = new PolicyUpdate { OrganizationId = policy.OrganizationId, Type = policy.Type, Enabled = policy.Enabled, - Data = policy.Data + Data = policy.Data, + PerformedBy = savingUserId.HasValue + ? new StandardUser(savingUserId.Value, await _currentContext.OrganizationOwner(policy.OrganizationId)) + : null }; await _savePolicyCommand.SaveAsync(policyUpdate); diff --git a/src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs b/src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs index d11da2119aa5..96fdefcfad44 100644 --- a/src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs +++ b/src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs @@ -91,6 +91,7 @@ private static void AddOrganizationSponsorshipCommands(this IServiceCollection s private static void AddOrganizationUserCommands(this IServiceCollection services) { services.AddScoped(); + services.AddScoped(); services.AddScoped(); services.AddScoped(); services.AddScoped(); diff --git a/src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationUserRepository.cs b/src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationUserRepository.cs index 361b1f058c73..e9722c102a60 100644 --- a/src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationUserRepository.cs +++ b/src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationUserRepository.cs @@ -557,4 +557,14 @@ public async Task> GetManyByOrganizationWithClaime return results.ToList(); } } + + public async Task SetOrganizationUsersStatusAsync(IEnumerable organizationUserIds, OrganizationUserStatusType status) + { + await using var connection = new SqlConnection(ConnectionString); + + await connection.ExecuteAsync( + "[dbo].[OrganizationUser_SetStatusForUsersById]", + new { OrganizationUserIds = organizationUserIds.ToGuidIdArrayTVP(), Status = status }, + commandType: CommandType.StoredProcedure); + } } diff --git a/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs b/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs index 0c9f1d0b9d8d..7e13b56e4e23 100644 --- a/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs +++ b/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs @@ -721,4 +721,14 @@ public UpdateEncryptedDataForKeyRotation UpdateForKeyRotation( return data; } } + + public async Task SetOrganizationUsersStatusAsync(IEnumerable userIds, OrganizationUserStatusType status) + { + using var scope = ServiceScopeFactory.CreateScope(); + + var dbContext = GetDatabaseContext(scope); + + await dbContext.OrganizationUsers.Where(x => userIds.Contains(x.OrganizationId)) + .ExecuteUpdateAsync(s => s.SetProperty(x => x.Status, status)); + } } diff --git a/src/Sql/dbo/Stored Procedures/OrganizationUser_SetStatusForUsersById.sql b/src/Sql/dbo/Stored Procedures/OrganizationUser_SetStatusForUsersById.sql new file mode 100644 index 000000000000..6476af4aa665 --- /dev/null +++ b/src/Sql/dbo/Stored Procedures/OrganizationUser_SetStatusForUsersById.sql @@ -0,0 +1,13 @@ +CREATE PROCEDURE [dbo].[OrganizationUser_SetStatusForUsersById] + @OrganizationUserIds AS [dbo].[GuidIdArray] READONLY, + @Status SMALLINT +AS +BEGIN + SET NOCOUNT ON + UPDATE + [dbo].[OrganizationUser] + SET + [Status] = @Status + WHERE + [Id] IN (SELECT Id from @OrganizationUserIds) +END diff --git a/test/Core.Test/AdminConsole/AutoFixture/PolicyUpdateFixtures.cs b/test/Core.Test/AdminConsole/AutoFixture/PolicyUpdateFixtures.cs index dff9b571782c..794f6fddf351 100644 --- a/test/Core.Test/AdminConsole/AutoFixture/PolicyUpdateFixtures.cs +++ b/test/Core.Test/AdminConsole/AutoFixture/PolicyUpdateFixtures.cs @@ -2,6 +2,7 @@ using AutoFixture; using AutoFixture.Xunit2; using Bit.Core.AdminConsole.Enums; +using Bit.Core.AdminConsole.Models.Data; using Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models; namespace Bit.Core.Test.AdminConsole.AutoFixture; @@ -12,7 +13,8 @@ public void Customize(IFixture fixture) { fixture.Customize(composer => composer .With(o => o.Type, type) - .With(o => o.Enabled, enabled)); + .With(o => o.Enabled, enabled) + .With(o => o.PerformedBy, new StandardUser(Guid.NewGuid(), false))); } } diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs new file mode 100644 index 000000000000..03c916167d88 --- /dev/null +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs @@ -0,0 +1,24 @@ +namespace Bit.Core.Test.AdminConsole.OrganizationFeatures.OrganizationUsers; + +public class RevokeNonCompliantOrganizationUserCommandTests +{ + // TODO Test if performed by is who we expect it to be + + // TODO Test if user is attempting to revoke themselves + + // TODO Test if user is trying to remove org users from another org + + // TODO test if user will remove all owners + + // TODO test if user is revoking an owner and they are not an owner + + // TODO test if the user being revoked is already revoked + + // TODO test if validation fails, then return the result list + + // TODO if all is valid, then revoke all the users + + // TODO if all is valid and is a standard user, call event service as such + + // TODO if all is valid and is a system user, call event service as such +} diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs index 76ee5748403f..aeb2b74769d7 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs @@ -126,4 +126,9 @@ await sutProvider.GetDependency() .SendOrganizationUserRemovedForPolicySingleOrgEmailAsync(organization.DisplayName(), "user3@example.com"); } + + // TODO feature flag is enabled, we call the revoke command + // TODO feature flag is enabled and revoke returns errors, we throw + // TODO feature flag is enabled and revoke returns no errors, no throw + // TODO feature flag is disabled we don't call command } diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs index 4dce13174934..bfd126ef9850 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs @@ -176,6 +176,10 @@ public async Task OnSaveSideEffectsAsync_UsersToBeRemovedDontHaveMasterPasswords HasMasterPassword = false }; + sutProvider.GetDependency() + .IsEnabled(FeatureFlagKeys.AccountDeprovisioning) + .Returns(false); + sutProvider.GetDependency() .GetManyDetailsByOrganizationAsync(policy.OrganizationId) .Returns(new List @@ -206,4 +210,11 @@ public async Task OnSaveSideEffectsAsync_UsersToBeRemovedDontHaveMasterPasswords await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() .RemoveUserAsync(organizationId: default, organizationUserId: default, deletingUserId: default); } + + + // TODO feature flag is disabled we don't call command + // TODO the feature flag is enabled, make sure it calls command + // TODO the feature flag is enabled and revocable members aren't both missing 2FA and MP, throw error + // TODO the feature flag is enabled and command returns errors, make sure we throw + // TODO the feature flag is enabled and command returns no errors, make sure we don't throw } From ff61b83675095954a8c67af15b5f850bc68db51d Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Thu, 7 Nov 2024 15:51:18 -0600 Subject: [PATCH 02/49] Added migration. --- .../DbScripts/2024-11-06-00_BulkOrgUserRevoke.sql | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 util/Migrator/DbScripts/2024-11-06-00_BulkOrgUserRevoke.sql diff --git a/util/Migrator/DbScripts/2024-11-06-00_BulkOrgUserRevoke.sql b/util/Migrator/DbScripts/2024-11-06-00_BulkOrgUserRevoke.sql new file mode 100644 index 000000000000..dec6edf3b1a8 --- /dev/null +++ b/util/Migrator/DbScripts/2024-11-06-00_BulkOrgUserRevoke.sql @@ -0,0 +1,15 @@ +CREATE OR ALTER PROCEDURE [dbo].[OrganizationUser_SetStatusForUsersById] + @OrganizationUserIds AS [dbo].[GuidIdArray] READONLY, + @Status SMALLINT +AS +BEGIN + SET NOCOUNT ON + UPDATE + [dbo].[OrganizationUser] + SET + [Status] = @Status + WHERE + [Id] IN (SELECT Id from @OrganizationUserIds) +END + +EXEC [dbo].[User_BumpAccountRevisionDateByOrganizationUserIds] @OrganizationUserIds From 49620c76ca2a1db152c1269366ca9141593bc012 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Mon, 11 Nov 2024 13:12:07 -0600 Subject: [PATCH 03/49] Wrote tests and fixed bugs found. --- ...vokeNonCompliantOrganizationUserCommand.cs | 28 +-- ...onCompliantOrganizationUserCommandTests.cs | 172 ++++++++++++++++-- 2 files changed, 175 insertions(+), 25 deletions(-) diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs index e7c1943f944a..d91071b566e7 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs @@ -19,12 +19,12 @@ public class RevokeNonCompliantOrganizationUserCommand(IOrganizationUserReposito IHasConfirmedOwnersExceptQuery confirmedOwnersExceptQuery, TimeProvider timeProvider) : IRevokeNonCompliantOrganizationUserCommand { - private const string _cannotRevokeSelfMessage = "You cannot revoke yourself."; - private const string _onlyOwnersCanRevokeOtherOwners = "Only owners can revoke other owners."; - private const string _userAlreadyRevoked = "User is already revoked."; - private const string _orgMustHaveAtLeastOneOwner = "Organization must have at least one confirmed owner."; - private const string _invalidUsers = "Invalid users."; - private const string _requestedByWasNotValid = "Action was performed by an unexpected type."; + public const string CannotRevokeSelfMessage = "You cannot revoke yourself."; + public const string OnlyOwnersCanRevokeOtherOwners = "Only owners can revoke other owners."; + public const string UserAlreadyRevoked = "User is already revoked."; + public const string OrgMustHaveAtLeastOneOwner = "Organization must have at least one confirmed owner."; + public const string InvalidUsers = "Invalid users."; + public const string RequestedByWasNotValid = "Action was performed by an unexpected type."; public async Task RevokeNonCompliantOrganizationUsersAsync(RevokeOrganizationUsers request) { @@ -64,40 +64,40 @@ private static (OrganizationUserUserDetails organizationUser, EventType eventTyp private async Task ValidateAsync(RevokeOrganizationUsers request) { - if (PerformedByIsAnExpectedType(request.ActionPerformedBy)) + if (!PerformedByIsAnExpectedType(request.ActionPerformedBy)) { - return new CommandResult([_requestedByWasNotValid]); + return new CommandResult([RequestedByWasNotValid]); } if (request.ActionPerformedBy is StandardUser loggableUser && request.OrganizationUsers.Any(x => x.UserId == loggableUser.UserId)) { - return new CommandResult([_cannotRevokeSelfMessage]); + return new CommandResult([CannotRevokeSelfMessage]); } if (request.OrganizationUsers.Any(x => x.OrganizationId != request.OrganizationId)) { - return new CommandResult([_invalidUsers]); + return new CommandResult([InvalidUsers]); } if (!await confirmedOwnersExceptQuery.HasConfirmedOwnersExceptAsync( request.OrganizationId, request.OrganizationUsers.Select(x => x.Id))) { - return new CommandResult([_orgMustHaveAtLeastOneOwner]); + return new CommandResult([OrgMustHaveAtLeastOneOwner]); } return request.OrganizationUsers.Aggregate(new CommandResult(), (result, userToRevoke) => { if (IsAlreadyRevoked(userToRevoke)) { - result.ErrorMessages.Add($"{_userAlreadyRevoked} Id: {userToRevoke.Id}"); + result.ErrorMessages.Add($"{UserAlreadyRevoked} Id: {userToRevoke.Id}"); return result; } if (IsNonOwnerRevokingAnOwner(userToRevoke, request.ActionPerformedBy)) { - result.ErrorMessages.Add($"{_onlyOwnersCanRevokeOtherOwners}"); + result.ErrorMessages.Add($"{OnlyOwnersCanRevokeOtherOwners}"); return result; } @@ -112,7 +112,7 @@ private static bool IsAlreadyRevoked(OrganizationUserUserDetails organizationUse private static bool IsNonOwnerRevokingAnOwner(OrganizationUserUserDetails organizationUser, IActingUser requestingUser) => requestingUser is StandardUser standardUser && - IsActingUserAllowedToRevokeOwner(organizationUser, standardUser); + !IsActingUserAllowedToRevokeOwner(organizationUser, standardUser); private static bool IsActingUserAllowedToRevokeOwner(OrganizationUserUserDetails organizationUser, StandardUser requestingOrganizationUser) => organizationUser is { Type: OrganizationUserType.Owner } diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs index 03c916167d88..88b11429a73f 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs @@ -1,24 +1,174 @@ -namespace Bit.Core.Test.AdminConsole.OrganizationFeatures.OrganizationUsers; +using Bit.Core.AdminConsole.Models.Data; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Requests; +using Bit.Core.Enums; +using Bit.Core.Models.Data.Organizations.OrganizationUsers; +using Bit.Core.Repositories; +using Bit.Test.Common.AutoFixture; +using Bit.Test.Common.AutoFixture.Attributes; +using NSubstitute; +using Xunit; +namespace Bit.Core.Test.AdminConsole.OrganizationFeatures.OrganizationUsers; + +[SutProviderCustomize] public class RevokeNonCompliantOrganizationUserCommandTests { - // TODO Test if performed by is who we expect it to be + [Theory, BitAutoData] + public async Task RevokeNonCompliantOrganizationUsersAsync_GivenUnrecognizedUserType_WhenAttemptingToRevoke_ThenErrorShouldBeReturned( + Guid organizationId, SutProvider sutProvider) + { + var command = new RevokeOrganizationUsers(organizationId, [], new InvalidUser()); + + var result = await sutProvider.Sut.RevokeNonCompliantOrganizationUsersAsync(command); + + Assert.True(result.HasErrors); + Assert.Contains(RevokeNonCompliantOrganizationUserCommand.RequestedByWasNotValid, result.ErrorMessages); + } + + [Theory, BitAutoData] + public async Task RevokeNonCompliantOrganizationUsersAsync_GivenPopulatedRequest_WhenUserAttemptsToRevokeThemselves_ThenErrorShouldBeReturned( + Guid organizationId, OrganizationUserUserDetails revokingUser, + SutProvider sutProvider) + { + var command = new RevokeOrganizationUsers(organizationId, revokingUser, + new StandardUser(revokingUser?.UserId ?? Guid.NewGuid(), true)); + + var result = await sutProvider.Sut.RevokeNonCompliantOrganizationUsersAsync(command); + + Assert.True(result.HasErrors); + Assert.Contains(RevokeNonCompliantOrganizationUserCommand.CannotRevokeSelfMessage, result.ErrorMessages); + } + + [Theory, BitAutoData] + public async Task RevokeNonCompliantOrganizationUsersAsync_GivenPopulatedRequest_WhenUserAttemptsToRevokeOrgUsersFromAnotherOrg_ThenErrorShouldBeReturned( + Guid organizationId, OrganizationUserUserDetails userFromAnotherOrg, + SutProvider sutProvider) + { + userFromAnotherOrg.OrganizationId = Guid.NewGuid(); + + var command = new RevokeOrganizationUsers(organizationId, userFromAnotherOrg, + new StandardUser(Guid.NewGuid(), true)); + + var result = await sutProvider.Sut.RevokeNonCompliantOrganizationUsersAsync(command); + + Assert.True(result.HasErrors); + Assert.Contains(RevokeNonCompliantOrganizationUserCommand.InvalidUsers, result.ErrorMessages); + } + + [Theory, BitAutoData] + public async Task RevokeNonCompliantOrganizationUsersAsync_GivenPopulatedRequest_WhenUserAttemptsToRevokeAllOwnersFromOrg_ThenErrorShouldBeReturned( + Guid organizationId, OrganizationUserUserDetails userToRevoke, + SutProvider sutProvider) + { + userToRevoke.OrganizationId = organizationId; + + var command = new RevokeOrganizationUsers(organizationId, userToRevoke, + new StandardUser(Guid.NewGuid(), true)); + + sutProvider.GetDependency() + .HasConfirmedOwnersExceptAsync(organizationId, Arg.Any>()) + .Returns(false); + + var result = await sutProvider.Sut.RevokeNonCompliantOrganizationUsersAsync(command); + + Assert.True(result.HasErrors); + Assert.Contains(RevokeNonCompliantOrganizationUserCommand.OrgMustHaveAtLeastOneOwner, result.ErrorMessages); + } + + [Theory, BitAutoData] + public async Task RevokeNonCompliantOrganizationUsersAsync_GivenPopulatedRequest_WhenUserAttemptsToRevokeOwnerWhenNotAnOwner_ThenErrorShouldBeReturned( + Guid organizationId, OrganizationUserUserDetails userToRevoke, + SutProvider sutProvider) + { + userToRevoke.OrganizationId = organizationId; + userToRevoke.Type = OrganizationUserType.Owner; + + var command = new RevokeOrganizationUsers(organizationId, userToRevoke, + new StandardUser(Guid.NewGuid(), false)); + + sutProvider.GetDependency() + .HasConfirmedOwnersExceptAsync(organizationId, Arg.Any>()) + .Returns(true); + + var result = await sutProvider.Sut.RevokeNonCompliantOrganizationUsersAsync(command); + + Assert.True(result.HasErrors); + Assert.Contains(RevokeNonCompliantOrganizationUserCommand.OnlyOwnersCanRevokeOtherOwners, result.ErrorMessages); + } + + [Theory, BitAutoData] + public async Task RevokeNonCompliantOrganizationUsersAsync_GivenPopulatedRequest_WhenUserAttemptsToRevokeUserWhoIsAlreadyRevoked_ThenErrorShouldBeReturned( + Guid organizationId, OrganizationUserUserDetails userToRevoke, + SutProvider sutProvider) + { + userToRevoke.OrganizationId = organizationId; + userToRevoke.Status = OrganizationUserStatusType.Revoked; + + var command = new RevokeOrganizationUsers(organizationId, userToRevoke, + new StandardUser(Guid.NewGuid(), true)); + + sutProvider.GetDependency() + .HasConfirmedOwnersExceptAsync(organizationId, Arg.Any>()) + .Returns(true); + + var result = await sutProvider.Sut.RevokeNonCompliantOrganizationUsersAsync(command); + + Assert.True(result.HasErrors); + Assert.Contains($"{RevokeNonCompliantOrganizationUserCommand.UserAlreadyRevoked} Id: {userToRevoke.Id}", result.ErrorMessages); + } + + [Theory, BitAutoData] + public async Task RevokeNonCompliantOrganizationUsersAsync_GivenPopulatedRequest_WhenUserHasMultipleInvalidUsers_ThenErrorShouldBeReturned( + Guid organizationId, IEnumerable usersToRevoke, + SutProvider sutProvider) + { + var revocableUsers = usersToRevoke.ToList(); + revocableUsers.ForEach(user => user.OrganizationId = organizationId); + revocableUsers[0].Type = OrganizationUserType.Owner; + revocableUsers[1].Status = OrganizationUserStatusType.Revoked; + + var command = new RevokeOrganizationUsers(organizationId, revocableUsers, + new StandardUser(Guid.NewGuid(), false)); + + sutProvider.GetDependency() + .HasConfirmedOwnersExceptAsync(organizationId, Arg.Any>()) + .Returns(true); - // TODO Test if user is attempting to revoke themselves + var result = await sutProvider.Sut.RevokeNonCompliantOrganizationUsersAsync(command); - // TODO Test if user is trying to remove org users from another org + Assert.True(result.HasErrors); + Assert.True(result.ErrorMessages.Count > 1); + } - // TODO test if user will remove all owners + [Theory, BitAutoData] + public async Task RevokeNonCompliantOrganizationUsersAsync_GivenValidPopulatedRequest_WhenUserAttemptsToRevokeAUser_ThenUserShouldBeRevoked( + Guid organizationId, OrganizationUserUserDetails userToRevoke, + SutProvider sutProvider) + { + userToRevoke.OrganizationId = organizationId; - // TODO test if user is revoking an owner and they are not an owner + var command = new RevokeOrganizationUsers(organizationId, userToRevoke, + new StandardUser(Guid.NewGuid(), true)); - // TODO test if the user being revoked is already revoked + sutProvider.GetDependency() + .HasConfirmedOwnersExceptAsync(organizationId, Arg.Any>()) + .Returns(true); - // TODO test if validation fails, then return the result list + var result = await sutProvider.Sut.RevokeNonCompliantOrganizationUsersAsync(command); - // TODO if all is valid, then revoke all the users + await sutProvider.GetDependency() + .Received(1) + .SetOrganizationUsersStatusAsync(Arg.Any>(), OrganizationUserStatusType.Revoked); - // TODO if all is valid and is a standard user, call event service as such + Assert.True(result.Success); + } - // TODO if all is valid and is a system user, call event service as such + public class InvalidUser : IActingUser + { + public Guid? UserId => Guid.Empty; + public bool IsOrganizationOwner => false; + public EventSystemUser? SystemUserType => null; + } } From e7a9a4ff327fa7e4ba687ba194baf199d85d19b9 Mon Sep 17 00:00:00 2001 From: Matt Bishop Date: Mon, 11 Nov 2024 16:29:47 -0500 Subject: [PATCH 04/49] Patch build process --- .github/workflows/build.yml | 58 ++++++++++++++++++++++++------------- 1 file changed, 38 insertions(+), 20 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index cc305eb91327..1d092d8b4dcb 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -21,6 +21,8 @@ jobs: lint: name: Lint runs-on: ubuntu-22.04 + needs: + - check-run steps: - name: Check out repo uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 @@ -36,7 +38,8 @@ jobs: build-artifacts: name: Build artifacts runs-on: ubuntu-22.04 - needs: lint + needs: + - lint strategy: fail-fast: false matrix: @@ -130,7 +133,6 @@ jobs: security-events: write needs: - build-artifacts - - check-run strategy: fail-fast: false matrix: @@ -224,7 +226,7 @@ jobs: - name: Generate Docker image tag id: tag run: | - if [[ $(grep "pull" <<< "${GITHUB_REF}") ]]; then + if [[ "${GITHUB_EVENT_NAME}" == "pull_request_target" ]]; then IMAGE_TAG=$(echo "${GITHUB_HEAD_REF}" | sed "s#/#-#g") else IMAGE_TAG=$(echo "${GITHUB_REF:11}" | sed "s#/#-#g") @@ -319,9 +321,9 @@ jobs: run: az acr login -n $_AZ_REGISTRY --only-show-errors - name: Make Docker stubs - if: github.ref == 'refs/heads/main' || - github.ref == 'refs/heads/rc' || - github.ref == 'refs/heads/hotfix-rc' + if: | + github.event_name != 'pull_request_target' + && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/rc' || github.ref == 'refs/heads/hotfix-rc') run: | # Set proper setup image based on branch case "$GITHUB_REF" in @@ -361,13 +363,17 @@ jobs: cd docker-stub/EU; zip -r ../../docker-stub-EU.zip *; cd ../.. - name: Make Docker stub checksums - if: github.ref == 'refs/heads/main' || github.ref == 'refs/heads/rc' || github.ref == 'refs/heads/hotfix-rc' + if: | + github.event_name != 'pull_request_target' + && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/rc' || github.ref == 'refs/heads/hotfix-rc') run: | sha256sum docker-stub-US.zip > docker-stub-US-sha256.txt sha256sum docker-stub-EU.zip > docker-stub-EU-sha256.txt - name: Upload Docker stub US artifact - if: github.ref == 'refs/heads/main' || github.ref == 'refs/heads/rc' || github.ref == 'refs/heads/hotfix-rc' + if: | + github.event_name != 'pull_request_target' + && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/rc' || github.ref == 'refs/heads/hotfix-rc') uses: actions/upload-artifact@b4b15b8c7c6ac21ea08fcf65892d2ee8f75cf882 # v4.4.3 with: name: docker-stub-US.zip @@ -375,7 +381,9 @@ jobs: if-no-files-found: error - name: Upload Docker stub EU artifact - if: github.ref == 'refs/heads/main' || github.ref == 'refs/heads/rc' || github.ref == 'refs/heads/hotfix-rc' + if: | + github.event_name != 'pull_request_target' + && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/rc' || github.ref == 'refs/heads/hotfix-rc') uses: actions/upload-artifact@b4b15b8c7c6ac21ea08fcf65892d2ee8f75cf882 # v4.4.3 with: name: docker-stub-EU.zip @@ -383,7 +391,9 @@ jobs: if-no-files-found: error - name: Upload Docker stub US checksum artifact - if: github.ref == 'refs/heads/main' || github.ref == 'refs/heads/rc' || github.ref == 'refs/heads/hotfix-rc' + if: | + github.event_name != 'pull_request_target' + && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/rc' || github.ref == 'refs/heads/hotfix-rc') uses: actions/upload-artifact@b4b15b8c7c6ac21ea08fcf65892d2ee8f75cf882 # v4.4.3 with: name: docker-stub-US-sha256.txt @@ -391,7 +401,9 @@ jobs: if-no-files-found: error - name: Upload Docker stub EU checksum artifact - if: github.ref == 'refs/heads/main' || github.ref == 'refs/heads/rc' || github.ref == 'refs/heads/hotfix-rc' + if: | + github.event_name != 'pull_request_target' + && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/rc' || github.ref == 'refs/heads/hotfix-rc') uses: actions/upload-artifact@b4b15b8c7c6ac21ea08fcf65892d2ee8f75cf882 # v4.4.3 with: name: docker-stub-EU-sha256.txt @@ -466,7 +478,8 @@ jobs: build-mssqlmigratorutility: name: Build MSSQL migrator utility runs-on: ubuntu-22.04 - needs: lint + needs: + - lint defaults: run: shell: bash @@ -517,8 +530,10 @@ jobs: self-host-build: name: Trigger self-host build + if: github.event_name != 'pull_request_target' && github.ref == 'refs/heads/main' runs-on: ubuntu-22.04 - needs: build-docker + needs: + - build-docker steps: - name: Log in to Azure - CI subscription uses: Azure/login@e15b166166a8746d1a47596803bd8c1b595455cf # v1.6.0 @@ -549,9 +564,10 @@ jobs: trigger-k8s-deploy: name: Trigger k8s deploy - if: github.ref == 'refs/heads/main' + if: github.event_name != 'pull_request_target' && github.ref == 'refs/heads/main' runs-on: ubuntu-22.04 - needs: build-docker + needs: + - build-docker steps: - name: Log in to Azure - CI subscription uses: Azure/login@e15b166166a8746d1a47596803bd8c1b595455cf # v1.6.0 @@ -583,9 +599,12 @@ jobs: trigger-ee-updates: name: Trigger Ephemeral Environment updates - if: github.ref != 'refs/heads/main' && contains(github.event.pull_request.labels.*.name, 'ephemeral-environment') + if: | + github.event_name == 'pull_request_target' + && contains(github.event.pull_request.labels.*.name, 'ephemeral-environment') runs-on: ubuntu-24.04 - needs: build-docker + needs: + - build-docker steps: - name: Log in to Azure - CI subscription uses: Azure/login@e15b166166a8746d1a47596803bd8c1b595455cf # v1.6.0 @@ -629,9 +648,8 @@ jobs: steps: - name: Check if any job failed if: | - (github.ref == 'refs/heads/main' - || github.ref == 'refs/heads/rc' - || github.ref == 'refs/heads/hotfix-rc') + github.event_name != 'pull_request_target' + && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/rc' || github.ref == 'refs/heads/hotfix-rc') && contains(needs.*.result, 'failure') run: exit 1 From 8b534b03d451cc5026164b08b7e8de05b2d6db51 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Mon, 11 Nov 2024 15:32:16 -0600 Subject: [PATCH 05/49] Fixing tests. --- .../OrganizationFeatures/Policies/SavePolicyCommandTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/SavePolicyCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/SavePolicyCommandTests.cs index 342ede9c829d..3ca7004e70ba 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/SavePolicyCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/SavePolicyCommandTests.cs @@ -100,7 +100,7 @@ public void Constructor_DuplicatePolicyValidators_Throws() } [Theory, BitAutoData] - public async Task SaveAsync_OrganizationDoesNotExist_ThrowsBadRequest(PolicyUpdate policyUpdate) + public async Task SaveAsync_OrganizationDoesNotExist_ThrowsBadRequest([PolicyUpdate(PolicyType.ActivateAutofill)] PolicyUpdate policyUpdate) { var sutProvider = SutProviderFactory(); sutProvider.GetDependency() @@ -115,7 +115,7 @@ public async Task SaveAsync_OrganizationDoesNotExist_ThrowsBadRequest(PolicyUpda } [Theory, BitAutoData] - public async Task SaveAsync_OrganizationCannotUsePolicies_ThrowsBadRequest(PolicyUpdate policyUpdate) + public async Task SaveAsync_OrganizationCannotUsePolicies_ThrowsBadRequest([PolicyUpdate(PolicyType.ActivateAutofill)] PolicyUpdate policyUpdate) { var sutProvider = SutProviderFactory(); sutProvider.GetDependency() From 6a367679faf9688d38bd560bac51374308785471 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Tue, 12 Nov 2024 09:33:49 -0600 Subject: [PATCH 06/49] Added unit test around disabling the feature flag. --- ...actorAuthenticationPolicyValidatorTests.cs | 48 +++++++++++++++++-- 1 file changed, 43 insertions(+), 5 deletions(-) diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs index bfd126ef9850..c72985efaed9 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs @@ -1,6 +1,8 @@ using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.Enums; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Requests; using Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models; using Bit.Core.AdminConsole.OrganizationFeatures.Policies.PolicyValidators; using Bit.Core.Auth.UserFeatures.TwoFactorAuth.Interfaces; @@ -211,10 +213,46 @@ await sutProvider.GetDependency().DidNotReceiveW .RemoveUserAsync(organizationId: default, organizationUserId: default, deletingUserId: default); } + [Theory, BitAutoData] + public async Task OnSaveSideEffectsAsync_GivenPolicyUpdate_WhenAccountProvisioningIsDisabled_ThenRevokeUserCommandShouldNotBeCalled( + Organization organization, + [PolicyUpdate(PolicyType.TwoFactorAuthentication)] PolicyUpdate policyUpdate, + [Policy(PolicyType.TwoFactorAuthentication, false)] Policy policy, + SutProvider sutProvider) + { + policy.OrganizationId = organization.Id = policyUpdate.OrganizationId; + sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); + + sutProvider.GetDependency() + .IsEnabled(FeatureFlagKeys.AccountDeprovisioning) + .Returns(false); + + sutProvider.GetDependency() + .GetManyDetailsByOrganizationAsync(policyUpdate.OrganizationId) + .Returns([]); + + var orgUserDetailUserAcceptedWithout2Fa = new OrganizationUserUserDetails + { + Id = Guid.NewGuid(), + Status = OrganizationUserStatusType.Accepted, + Type = OrganizationUserType.User, + Email = "user3@test.com", + Name = "TEST", + UserId = Guid.NewGuid(), + HasMasterPassword = true + }; - // TODO feature flag is disabled we don't call command - // TODO the feature flag is enabled, make sure it calls command - // TODO the feature flag is enabled and revocable members aren't both missing 2FA and MP, throw error - // TODO the feature flag is enabled and command returns errors, make sure we throw - // TODO the feature flag is enabled and command returns no errors, make sure we don't throw + sutProvider.GetDependency() + .TwoFactorIsEnabledAsync(Arg.Any>()) + .Returns(new List<(OrganizationUserUserDetails user, bool hasTwoFactor)>() + { + (orgUserDetailUserAcceptedWithout2Fa, false), + }); + + await sutProvider.Sut.OnSaveSideEffectsAsync(policyUpdate, policy); + + await sutProvider.GetDependency() + .DidNotReceive() + .RevokeNonCompliantOrganizationUsersAsync(Arg.Any()); + } } From f2f2a626706bb7541fb506c9c18ff2695a57216d Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Tue, 12 Nov 2024 09:46:45 -0600 Subject: [PATCH 07/49] Updated error message to be public and added test for validating the request. --- .../TwoFactorAuthenticationPolicyValidator.cs | 6 +- ...actorAuthenticationPolicyValidatorTests.cs | 69 ++++++++++++++++--- 2 files changed, 62 insertions(+), 13 deletions(-) diff --git a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs index 3d0b2c7ba87b..5d2c83eab195 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs @@ -28,7 +28,7 @@ public class TwoFactorAuthenticationPolicyValidator : IPolicyValidator private readonly IFeatureService _featureService; private readonly IRevokeNonCompliantOrganizationUserCommand _revokeNonCompliantOrganizationUserCommand; - private const string _nonCompliantMembersWillLoseAccess = "Policy could not be enabled. Non-compliant members will lose access to their accounts. Identify members without two-step login from the policies column in the members page."; + public const string NonCompliantMembersWillLoseAccess = "Policy could not be enabled. Non-compliant members will lose access to their accounts. Identify members without two-step login from the policies column in the members page."; public PolicyType Type => PolicyType.TwoFactorAuthentication; public IEnumerable RequiredPolicies => []; @@ -81,7 +81,7 @@ private async Task RemoveNonCompliantUsersAsync(Guid organizationId) { if (DoMembersNotHaveAPasswordWithTwoFactorAuthenticationEnabled(revocableOrgUsers, organizationUsersTwoFactorEnabled)) { - throw new BadRequestException(_nonCompliantMembersWillLoseAccess); + throw new BadRequestException(NonCompliantMembersWillLoseAccess); } var result = await _revokeNonCompliantOrganizationUserCommand.RevokeNonCompliantOrganizationUsersAsync( @@ -107,7 +107,7 @@ private async Task RemoveNonCompliantUsersAsync(Guid organizationId) { if (!orgUser.HasMasterPassword) { - throw new BadRequestException(_nonCompliantMembersWillLoseAccess); + throw new BadRequestException(NonCompliantMembersWillLoseAccess); } await _removeOrganizationUserCommand.RemoveUserAsync(organizationId, orgUser.Id, diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs index c72985efaed9..8640be29a3b5 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs @@ -207,18 +207,20 @@ public async Task OnSaveSideEffectsAsync_UsersToBeRemovedDontHaveMasterPasswords var badRequestException = await Assert.ThrowsAsync( () => sutProvider.Sut.OnSaveSideEffectsAsync(policyUpdate, policy)); - Assert.Contains("Policy could not be enabled. Non-compliant members will lose access to their accounts. Identify members without two-step login from the policies column in the members page.", badRequestException.Message, StringComparison.OrdinalIgnoreCase); + Assert.Equal(TwoFactorAuthenticationPolicyValidator.NonCompliantMembersWillLoseAccess, badRequestException.Message); await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() .RemoveUserAsync(organizationId: default, organizationUserId: default, deletingUserId: default); } [Theory, BitAutoData] - public async Task OnSaveSideEffectsAsync_GivenPolicyUpdate_WhenAccountProvisioningIsDisabled_ThenRevokeUserCommandShouldNotBeCalled( - Organization organization, - [PolicyUpdate(PolicyType.TwoFactorAuthentication)] PolicyUpdate policyUpdate, - [Policy(PolicyType.TwoFactorAuthentication, false)] Policy policy, - SutProvider sutProvider) + public async Task OnSaveSideEffectsAsync_GivenUpdateTo2faPolicy_WhenAccountProvisioningIsDisabled_ThenRevokeUserCommandShouldNotBeCalled( + Organization organization, + [PolicyUpdate(PolicyType.TwoFactorAuthentication)] + PolicyUpdate policyUpdate, + [Policy(PolicyType.TwoFactorAuthentication, false)] + Policy policy, + SutProvider sutProvider) { policy.OrganizationId = organization.Id = policyUpdate.OrganizationId; sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); @@ -227,10 +229,6 @@ public async Task OnSaveSideEffectsAsync_GivenPolicyUpdate_WhenAccountProvisioni .IsEnabled(FeatureFlagKeys.AccountDeprovisioning) .Returns(false); - sutProvider.GetDependency() - .GetManyDetailsByOrganizationAsync(policyUpdate.OrganizationId) - .Returns([]); - var orgUserDetailUserAcceptedWithout2Fa = new OrganizationUserUserDetails { Id = Guid.NewGuid(), @@ -242,6 +240,14 @@ public async Task OnSaveSideEffectsAsync_GivenPolicyUpdate_WhenAccountProvisioni HasMasterPassword = true }; + sutProvider.GetDependency() + .GetManyDetailsByOrganizationAsync(policyUpdate.OrganizationId) + .Returns(new List + { + orgUserDetailUserAcceptedWithout2Fa + }); + + sutProvider.GetDependency() .TwoFactorIsEnabledAsync(Arg.Any>()) .Returns(new List<(OrganizationUserUserDetails user, bool hasTwoFactor)>() @@ -255,4 +261,47 @@ await sutProvider.GetDependency() .DidNotReceive() .RevokeNonCompliantOrganizationUsersAsync(Arg.Any()); } + + [Theory, BitAutoData] + public async Task OnSaveSideEffectsAsync_GivenUpdateTo2faPolicy_WhenAccountProvisioningIsEnabled_ThenRevokeUserCommandShouldNotBeCalled( + Organization organization, + [PolicyUpdate(PolicyType.TwoFactorAuthentication)] PolicyUpdate policyUpdate, + [Policy(PolicyType.TwoFactorAuthentication, false)] Policy policy, + SutProvider sutProvider) + { + policy.OrganizationId = organization.Id = policyUpdate.OrganizationId; + sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); + + sutProvider.GetDependency() + .IsEnabled(FeatureFlagKeys.AccountDeprovisioning) + .Returns(true); + + var orgUserDetailUserWithout2Fa = new OrganizationUserUserDetails + { + Id = Guid.NewGuid(), + Status = OrganizationUserStatusType.Confirmed, + Type = OrganizationUserType.User, + // Needs to be different from what is passed in as the savingUserId to Sut.SaveAsync + Email = "user3@test.com", + Name = "TEST", + UserId = Guid.NewGuid(), + HasMasterPassword = false + }; + + sutProvider.GetDependency() + .GetManyDetailsByOrganizationAsync(policyUpdate.OrganizationId) + .Returns([orgUserDetailUserWithout2Fa]); + + sutProvider.GetDependency() + .TwoFactorIsEnabledAsync(Arg.Any>()) + .Returns(new List<(OrganizationUserUserDetails user, bool hasTwoFactor)>() + { + (orgUserDetailUserWithout2Fa, false), + }); + + var action = () => sutProvider.Sut.OnSaveSideEffectsAsync(policyUpdate, policy); + + var exception = await Assert.ThrowsAsync(action); + Assert.Equal(TwoFactorAuthenticationPolicyValidator.NonCompliantMembersWillLoseAccess, exception.Message); + } } From f009db0f059dc68ef1d0abda9045d73b83a8325f Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Tue, 12 Nov 2024 09:47:49 -0600 Subject: [PATCH 08/49] formatting --- .../TwoFactorAuthenticationPolicyValidatorTests.cs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs index 8640be29a3b5..0f4ad2160ea6 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs @@ -281,7 +281,6 @@ public async Task OnSaveSideEffectsAsync_GivenUpdateTo2faPolicy_WhenAccountProvi Id = Guid.NewGuid(), Status = OrganizationUserStatusType.Confirmed, Type = OrganizationUserType.User, - // Needs to be different from what is passed in as the savingUserId to Sut.SaveAsync Email = "user3@test.com", Name = "TEST", UserId = Guid.NewGuid(), @@ -299,9 +298,8 @@ public async Task OnSaveSideEffectsAsync_GivenUpdateTo2faPolicy_WhenAccountProvi (orgUserDetailUserWithout2Fa, false), }); - var action = () => sutProvider.Sut.OnSaveSideEffectsAsync(policyUpdate, policy); + var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.OnSaveSideEffectsAsync(policyUpdate, policy)); - var exception = await Assert.ThrowsAsync(action); Assert.Equal(TwoFactorAuthenticationPolicyValidator.NonCompliantMembersWillLoseAccess, exception.Message); } } From 595e4b900404f23817eb09515371f66bb33b747f Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Tue, 12 Nov 2024 10:37:25 -0600 Subject: [PATCH 09/49] Added some tests for single org policy validator. --- .../SingleOrgPolicyValidator.cs | 1 - .../SingleOrgPolicyValidatorTests.cs | 67 ++++++++++++++++++- 2 files changed, 66 insertions(+), 2 deletions(-) diff --git a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs index 883bf1bb63a4..6615865f7341 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs @@ -95,7 +95,6 @@ private async Task RemoveNonCompliantUsersAsync(Guid organizationId) var commandResult = await _revokeNonCompliantOrganizationUserCommand.RevokeNonCompliantOrganizationUsersAsync( new RevokeOrganizationUsers(organizationId, revocableUsers, new StandardUser(savingUserId ?? Guid.Empty, isOwner))); - // might need to throw if list of errors returned if (commandResult.HasErrors) { throw new BadRequestException(string.Join(", ", commandResult.ErrorMessages)); diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs index aeb2b74769d7..bd392eb20e65 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs @@ -1,6 +1,8 @@ using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.Enums; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Requests; using Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models; using Bit.Core.AdminConsole.OrganizationFeatures.Policies.PolicyValidators; using Bit.Core.Auth.Entities; @@ -127,7 +129,70 @@ await sutProvider.GetDependency() "user3@example.com"); } - // TODO feature flag is enabled, we call the revoke command + [Theory, BitAutoData] + public async Task OnSaveSideEffectsAsync_GivenUpdatedSingleOrgPolicy_WhenAccountDeprovisioningIsDisabled_Then( + [PolicyUpdate(PolicyType.SingleOrg)] PolicyUpdate policyUpdate, + [Policy(PolicyType.SingleOrg, false)] Policy policy, + Guid savingUserId, + Guid nonCompliantUserId, + Organization organization, SutProvider sutProvider) + { + policy.OrganizationId = organization.Id = policyUpdate.OrganizationId; + + var compliantUser1 = new OrganizationUserUserDetails + { + OrganizationId = organization.Id, + Type = OrganizationUserType.User, + Status = OrganizationUserStatusType.Confirmed, + UserId = new Guid(), + Email = "user1@example.com" + }; + + var compliantUser2 = new OrganizationUserUserDetails + { + OrganizationId = organization.Id, + Type = OrganizationUserType.User, + Status = OrganizationUserStatusType.Confirmed, + UserId = new Guid(), + Email = "user2@example.com" + }; + + var nonCompliantUser = new OrganizationUserUserDetails + { + OrganizationId = organization.Id, + Type = OrganizationUserType.User, + Status = OrganizationUserStatusType.Confirmed, + UserId = nonCompliantUserId, + Email = "user3@example.com" + }; + + sutProvider.GetDependency() + .GetManyDetailsByOrganizationAsync(policyUpdate.OrganizationId) + .Returns([compliantUser1, compliantUser2, nonCompliantUser]); + + var otherOrganizationUser = new OrganizationUser + { + OrganizationId = new Guid(), + UserId = nonCompliantUserId, + Status = OrganizationUserStatusType.Confirmed + }; + + sutProvider.GetDependency() + .GetManyByManyUsersAsync(Arg.Is>(ids => ids.Contains(nonCompliantUserId))) + .Returns([otherOrganizationUser]); + + sutProvider.GetDependency().UserId.Returns(savingUserId); + sutProvider.GetDependency().GetByIdAsync(policyUpdate.OrganizationId) + .Returns(organization); + + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.AccountDeprovisioning).Returns(false); + + await sutProvider.Sut.OnSaveSideEffectsAsync(policyUpdate, policy); + + await sutProvider.GetDependency() + .DidNotReceive() + .RevokeNonCompliantOrganizationUsersAsync(Arg.Any()); + } // TODO feature flag is enabled and revoke returns errors, we throw // TODO feature flag is enabled and revoke returns no errors, no throw // TODO feature flag is disabled we don't call command From d64032c4658240c7f4df30334b0dda028b25ee84 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Tue, 12 Nov 2024 11:59:15 -0600 Subject: [PATCH 10/49] Fix issues from merge. --- .../RevokeNonCompliantOrganizationUserCommand.cs | 4 ++-- .../OrganizationFeatures/Shared}/CommandResult.cs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) rename src/{Api/AdminConsole/Models/Response/Organizations => Core/AdminConsole/OrganizationFeatures/Shared}/CommandResult.cs (81%) diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs index d91071b566e7..cdaca476447f 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs @@ -1,7 +1,7 @@ -using Bit.Core.AdminConsole.Models.Api.Response; -using Bit.Core.AdminConsole.Models.Data; +using Bit.Core.AdminConsole.Models.Data; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Requests; +using Bit.Core.AdminConsole.OrganizationFeatures.Shared; using Bit.Core.Enums; using Bit.Core.Models.Data.Organizations.OrganizationUsers; using Bit.Core.Repositories; diff --git a/src/Api/AdminConsole/Models/Response/Organizations/CommandResult.cs b/src/Core/AdminConsole/OrganizationFeatures/Shared/CommandResult.cs similarity index 81% rename from src/Api/AdminConsole/Models/Response/Organizations/CommandResult.cs rename to src/Core/AdminConsole/OrganizationFeatures/Shared/CommandResult.cs index e98a18d5ca99..73d9c6fc2511 100644 --- a/src/Api/AdminConsole/Models/Response/Organizations/CommandResult.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Shared/CommandResult.cs @@ -1,4 +1,4 @@ -namespace Bit.Core.AdminConsole.Models.Api.Response; +namespace Bit.Core.AdminConsole.OrganizationFeatures.Shared; public class CommandResult(IEnumerable errors) { From abbd4f582512ed211ae1c63ebb65161d384f6e16 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Tue, 12 Nov 2024 15:33:00 -0600 Subject: [PATCH 11/49] Added sending emails to revoked non-compliant users. --- .../PolicyValidators/SingleOrgPolicyValidator.cs | 6 ++++-- ...anizationUserRevokedForSinglePolicyOrg.html.hbs | 14 ++++++++++++++ ...anizationUserRevokedForSinglePolicyOrg.text.hbs | 5 +++++ ...zationUserRevokedForPolicySingleOrgViewModel.cs | 6 ++++++ src/Core/Services/IMailService.cs | 1 + .../Implementations/HandlebarsMailService.cs | 14 ++++++++++++++ .../NoopImplementations/NoopMailService.cs | 3 +++ 7 files changed, 47 insertions(+), 2 deletions(-) create mode 100644 src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForSinglePolicyOrg.html.hbs create mode 100644 src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForSinglePolicyOrg.text.hbs create mode 100644 src/Core/Models/Mail/OrganizationUserRevokedForPolicySingleOrgViewModel.cs diff --git a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs index 37247dd28018..b339de73bff4 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs @@ -94,7 +94,8 @@ private async Task RemoveNonCompliantUsersAsync(Guid organizationId) var revocableUsers = removableOrgUsers.Where(nonCompliantUser => orgUsers.Any(orgUser => nonCompliantUser.UserId == orgUser.UserId && nonCompliantUser.OrganizationId != org.Id - && nonCompliantUser.Status != OrganizationUserStatusType.Invited)); + && nonCompliantUser.Status != OrganizationUserStatusType.Invited)) + .ToList(); var commandResult = await _revokeNonCompliantOrganizationUserCommand.RevokeNonCompliantOrganizationUsersAsync( new RevokeOrganizationUsers(organizationId, revocableUsers, new StandardUser(savingUserId ?? Guid.Empty, isOwner))); @@ -104,7 +105,8 @@ private async Task RemoveNonCompliantUsersAsync(Guid organizationId) throw new BadRequestException(string.Join(", ", commandResult.ErrorMessages)); } - // TODO send email + await Task.WhenAll(revocableUsers.Select(x => + _mailService.SendOrganizationUserRevokedForPolicySingleOrgEmailAsync(org.DisplayName(), x.Email))); } else { diff --git a/src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForSinglePolicyOrg.html.hbs b/src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForSinglePolicyOrg.html.hbs new file mode 100644 index 000000000000..d04abe86c935 --- /dev/null +++ b/src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForSinglePolicyOrg.html.hbs @@ -0,0 +1,14 @@ +{{#>FullHtmlLayout}} + + + + + + + +
+ Your user account has been revoked from the {{OrganizationName}} organization because your account is part of multiple organizations. Before you can re-join {{OrganizationName}}, you must first leave all other organizations. +
+ To leave an organization, first log into the web app, select the three dot menu next to the organization name, and select Leave. +
+{{/FullHtmlLayout}} diff --git a/src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForSinglePolicyOrg.text.hbs b/src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForSinglePolicyOrg.text.hbs new file mode 100644 index 000000000000..0a720a7f869c --- /dev/null +++ b/src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForSinglePolicyOrg.text.hbs @@ -0,0 +1,5 @@ +{{#>BasicTextLayout}} +Your user account has been revoked from the {{OrganizationName}} organization because your account is part of multiple organizations. Before you can rejoin {{OrganizationName}}, you must first leave all other organizations. + +To leave an organization, first log in the {{link "web app" "https://vault.bitwarden.com/#/login"}}, select the three dot menu next to the organization name, and select Leave. +{{/BasicTextLayout}} diff --git a/src/Core/Models/Mail/OrganizationUserRevokedForPolicySingleOrgViewModel.cs b/src/Core/Models/Mail/OrganizationUserRevokedForPolicySingleOrgViewModel.cs new file mode 100644 index 000000000000..27c784bd15dd --- /dev/null +++ b/src/Core/Models/Mail/OrganizationUserRevokedForPolicySingleOrgViewModel.cs @@ -0,0 +1,6 @@ +namespace Bit.Core.Models.Mail; + +public class OrganizationUserRevokedForPolicySingleOrgViewModel : BaseMailModel +{ + public string OrganizationName { get; set; } +} diff --git a/src/Core/Services/IMailService.cs b/src/Core/Services/IMailService.cs index 15ed9e2ea4ee..294139dfa24c 100644 --- a/src/Core/Services/IMailService.cs +++ b/src/Core/Services/IMailService.cs @@ -35,6 +35,7 @@ Task SendTrialInitiationSignupEmailAsync( Task SendOrganizationAcceptedEmailAsync(Organization organization, string userIdentifier, IEnumerable adminEmails, bool hasAccessSecretsManager = false); Task SendOrganizationConfirmedEmailAsync(string organizationName, string email, bool hasAccessSecretsManager = false); Task SendOrganizationUserRemovedForPolicyTwoStepEmailAsync(string organizationName, string email); + Task SendOrganizationUserRevokedForPolicySingleOrgEmailAsync(string organizationName, string email); Task SendPasswordlessSignInAsync(string returnUrl, string token, string email); Task SendInvoiceUpcoming( string email, diff --git a/src/Core/Services/Implementations/HandlebarsMailService.cs b/src/Core/Services/Implementations/HandlebarsMailService.cs index dbf056c026b0..d6bb9a23c5d0 100644 --- a/src/Core/Services/Implementations/HandlebarsMailService.cs +++ b/src/Core/Services/Implementations/HandlebarsMailService.cs @@ -496,6 +496,20 @@ public async Task SendOrganizationUserRemovedForPolicySingleOrgEmailAsync(string await _mailDeliveryService.SendEmailAsync(message); } + public async Task SendOrganizationUserRevokedForPolicySingleOrgEmailAsync(string organizationName, string email) + { + var message = CreateDefaultMessage($"You have been revoked from {organizationName}", email); + var model = new OrganizationUserRevokedForPolicySingleOrgViewModel + { + OrganizationName = CoreHelpers.SanitizeForEmail(organizationName, false), + WebVaultUrl = _globalSettings.BaseServiceUri.VaultWithHash, + SiteName = _globalSettings.SiteName + }; + await AddMessageContentAsync(message, "OrganizationUserRevokedForPolicySingleOrg", model); + message.Category = "OrganizationUserRevokedForPolicySingleOrg"; + await _mailDeliveryService.SendEmailAsync(message); + } + public async Task SendEnqueuedMailMessageAsync(IMailQueueMessage queueMessage) { var message = CreateDefaultMessage(queueMessage.Subject, queueMessage.ToEmails); diff --git a/src/Core/Services/NoopImplementations/NoopMailService.cs b/src/Core/Services/NoopImplementations/NoopMailService.cs index 9b8a9abeea29..f489a0b9b52f 100644 --- a/src/Core/Services/NoopImplementations/NoopMailService.cs +++ b/src/Core/Services/NoopImplementations/NoopMailService.cs @@ -79,6 +79,9 @@ public Task SendOrganizationUserRemovedForPolicyTwoStepEmailAsync(string organiz return Task.FromResult(0); } + public Task SendOrganizationUserRevokedForPolicySingleOrgEmailAsync(string organizationName, string email) => + Task.CompletedTask; + public Task SendTwoFactorEmailAsync(string email, string token) { return Task.FromResult(0); From 39171142d6a98289f48f0c42b5a49e3692a01cf0 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Wed, 13 Nov 2024 09:31:06 -0600 Subject: [PATCH 12/49] Fixing name. Adding two factor policy email. --- ...ionUserRevokedForSingleOrgPolicy.html.hbs} | 0 ...ionUserRevokedForSingleOrgPolicy.text.hbs} | 0 ...tionUserRevokedForTwoFactorPolicy.html.hbs | 15 +++++++++++++ ...tionUserRevokedForTwoFactorPolicy.text.hbs | 7 +++++++ ...nUserRevokedForPolicyTwoFactorViewModel.cs | 6 ++++++ src/Core/Services/IMailService.cs | 1 + .../Implementations/HandlebarsMailService.cs | 21 +++++++++++++++---- .../NoopImplementations/NoopMailService.cs | 3 +++ 8 files changed, 49 insertions(+), 4 deletions(-) rename src/Core/MailTemplates/Handlebars/AdminConsole/{OrganizationUserRevokedForSinglePolicyOrg.html.hbs => OrganizationUserRevokedForSingleOrgPolicy.html.hbs} (100%) rename src/Core/MailTemplates/Handlebars/AdminConsole/{OrganizationUserRevokedForSinglePolicyOrg.text.hbs => OrganizationUserRevokedForSingleOrgPolicy.text.hbs} (100%) create mode 100644 src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForTwoFactorPolicy.html.hbs create mode 100644 src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForTwoFactorPolicy.text.hbs create mode 100644 src/Core/Models/Mail/OrganizationUserRevokedForPolicyTwoFactorViewModel.cs diff --git a/src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForSinglePolicyOrg.html.hbs b/src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForSingleOrgPolicy.html.hbs similarity index 100% rename from src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForSinglePolicyOrg.html.hbs rename to src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForSingleOrgPolicy.html.hbs diff --git a/src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForSinglePolicyOrg.text.hbs b/src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForSingleOrgPolicy.text.hbs similarity index 100% rename from src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForSinglePolicyOrg.text.hbs rename to src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForSingleOrgPolicy.text.hbs diff --git a/src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForTwoFactorPolicy.html.hbs b/src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForTwoFactorPolicy.html.hbs new file mode 100644 index 000000000000..cf38632a9e1f --- /dev/null +++ b/src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForTwoFactorPolicy.html.hbs @@ -0,0 +1,15 @@ +{{#>FullHtmlLayout}} + + + + + + + +
+ Your user account has been revoked from the {{OrganizationName}} organization because you do not have two-step login configured. Before you can re-join {{OrganizationName}}, you need to set up two-step login on your user account. +
+ Learn how to enable two-step login on your user account at + https://help.bitwarden.com/article/setup-two-step-login/ +
+{{/FullHtmlLayout}} diff --git a/src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForTwoFactorPolicy.text.hbs b/src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForTwoFactorPolicy.text.hbs new file mode 100644 index 000000000000..ef43713766ee --- /dev/null +++ b/src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForTwoFactorPolicy.text.hbs @@ -0,0 +1,7 @@ +{{#>BasicTextLayout}} + Your user account has been removed from the {{OrganizationName}} organization because you do not have two-step login + configured. Before you can re-join this organization you need to set up two-step login on your user account. + + Learn how to enable two-step login on your user account at + {{link "https://help.bitwarden.com/article/setup-two-step-login/"}} +{{/BasicTextLayout}} diff --git a/src/Core/Models/Mail/OrganizationUserRevokedForPolicyTwoFactorViewModel.cs b/src/Core/Models/Mail/OrganizationUserRevokedForPolicyTwoFactorViewModel.cs new file mode 100644 index 000000000000..9286ee74b3d2 --- /dev/null +++ b/src/Core/Models/Mail/OrganizationUserRevokedForPolicyTwoFactorViewModel.cs @@ -0,0 +1,6 @@ +namespace Bit.Core.Models.Mail; + +public class OrganizationUserRevokedForPolicyTwoFactorViewModel : BaseMailModel +{ + public string OrganizationName { get; set; } +} diff --git a/src/Core/Services/IMailService.cs b/src/Core/Services/IMailService.cs index 294139dfa24c..3f054428c5fe 100644 --- a/src/Core/Services/IMailService.cs +++ b/src/Core/Services/IMailService.cs @@ -35,6 +35,7 @@ Task SendTrialInitiationSignupEmailAsync( Task SendOrganizationAcceptedEmailAsync(Organization organization, string userIdentifier, IEnumerable adminEmails, bool hasAccessSecretsManager = false); Task SendOrganizationConfirmedEmailAsync(string organizationName, string email, bool hasAccessSecretsManager = false); Task SendOrganizationUserRemovedForPolicyTwoStepEmailAsync(string organizationName, string email); + Task SendOrganizationUserRevokedForTwoFactoryPolicyEmailAsync(string organizationName, string email); Task SendOrganizationUserRevokedForPolicySingleOrgEmailAsync(string organizationName, string email); Task SendPasswordlessSignInAsync(string returnUrl, string token, string email); Task SendInvoiceUpcoming( diff --git a/src/Core/Services/Implementations/HandlebarsMailService.cs b/src/Core/Services/Implementations/HandlebarsMailService.cs index d6bb9a23c5d0..f61a7e30a7e0 100644 --- a/src/Core/Services/Implementations/HandlebarsMailService.cs +++ b/src/Core/Services/Implementations/HandlebarsMailService.cs @@ -25,8 +25,7 @@ public class HandlebarsMailService : IMailService private readonly GlobalSettings _globalSettings; private readonly IMailDeliveryService _mailDeliveryService; private readonly IMailEnqueuingService _mailEnqueuingService; - private readonly Dictionary> _templateCache = - new Dictionary>(); + private readonly Dictionary> _templateCache = new(); private bool _registeredHelpersAndPartials = false; @@ -295,6 +294,20 @@ public async Task SendOrganizationUserRemovedForPolicyTwoStepEmailAsync(string o await _mailDeliveryService.SendEmailAsync(message); } + public async Task SendOrganizationUserRevokedForTwoFactoryPolicyEmailAsync(string organizationName, string email) + { + var message = CreateDefaultMessage($"You have been revoked from {organizationName}", email); + var model = new OrganizationUserRevokedForPolicyTwoFactorViewModel + { + OrganizationName = CoreHelpers.SanitizeForEmail(organizationName, false), + WebVaultUrl = _globalSettings.BaseServiceUri.VaultWithHash, + SiteName = _globalSettings.SiteName + }; + await AddMessageContentAsync(message, "OrganizationUserRevokedForTwoFactorPolicy", model); + message.Category = "OrganizationUserRevokedForTwoFactorPolicy"; + await _mailDeliveryService.SendEmailAsync(message); + } + public async Task SendWelcomeEmailAsync(User user) { var message = CreateDefaultMessage("Welcome to Bitwarden!", user.Email); @@ -505,8 +518,8 @@ public async Task SendOrganizationUserRevokedForPolicySingleOrgEmailAsync(string WebVaultUrl = _globalSettings.BaseServiceUri.VaultWithHash, SiteName = _globalSettings.SiteName }; - await AddMessageContentAsync(message, "OrganizationUserRevokedForPolicySingleOrg", model); - message.Category = "OrganizationUserRevokedForPolicySingleOrg"; + await AddMessageContentAsync(message, "OrganizationUserRevokedForSingleOrgPolicy", model); + message.Category = "OrganizationUserRevokedForSingleOrgPolicy"; await _mailDeliveryService.SendEmailAsync(message); } diff --git a/src/Core/Services/NoopImplementations/NoopMailService.cs b/src/Core/Services/NoopImplementations/NoopMailService.cs index f489a0b9b52f..38cee2245626 100644 --- a/src/Core/Services/NoopImplementations/NoopMailService.cs +++ b/src/Core/Services/NoopImplementations/NoopMailService.cs @@ -79,6 +79,9 @@ public Task SendOrganizationUserRemovedForPolicyTwoStepEmailAsync(string organiz return Task.FromResult(0); } + public Task SendOrganizationUserRevokedForTwoFactoryPolicyEmailAsync(string organizationName, string email) => + Task.CompletedTask; + public Task SendOrganizationUserRevokedForPolicySingleOrgEmailAsync(string organizationName, string email) => Task.CompletedTask; From da02c894a0ed9053d9fcae55bbe295c493fb9521 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Wed, 13 Nov 2024 09:33:44 -0600 Subject: [PATCH 13/49] Send email when user has been revoked. --- .../PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs index 5d2c83eab195..b8de329b241a 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs @@ -94,7 +94,8 @@ private async Task RemoveNonCompliantUsersAsync(Guid organizationId) throw new BadRequestException(string.Join(", ", result.ErrorMessages)); } - // TODO Send out User Revoked Email + await Task.WhenAll(revocableOrgUsers.Select(x => + _mailService.SendOrganizationUserRevokedForTwoFactoryPolicyEmailAsync(org!.DisplayName(), x.Email))); } else { From 1c3e4d860131cd2cd15790cd20772644e8e98f80 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Wed, 13 Nov 2024 09:36:00 -0600 Subject: [PATCH 14/49] Correcting migration name. --- ...lkOrgUserRevoke.sql => 2024-11-06-00_OrgUserSetStatusBulk.sql} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename util/Migrator/DbScripts/{2024-11-06-00_BulkOrgUserRevoke.sql => 2024-11-06-00_OrgUserSetStatusBulk.sql} (100%) diff --git a/util/Migrator/DbScripts/2024-11-06-00_BulkOrgUserRevoke.sql b/util/Migrator/DbScripts/2024-11-06-00_OrgUserSetStatusBulk.sql similarity index 100% rename from util/Migrator/DbScripts/2024-11-06-00_BulkOrgUserRevoke.sql rename to util/Migrator/DbScripts/2024-11-06-00_OrgUserSetStatusBulk.sql From 35643c15dc2a94721d48fab9d3602e7495be7918 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Wed, 13 Nov 2024 13:26:18 -0600 Subject: [PATCH 15/49] Fixing templates and logic issue in Revoke command. --- .../RevokeNonCompliantOrganizationUserCommand.cs | 2 +- .../OrganizationUserRevokedForSingleOrgPolicy.text.hbs | 2 +- .../OrganizationUserRevokedForTwoFactorPolicy.text.hbs | 2 +- src/Core/Services/Implementations/HandlebarsMailService.cs | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs index cdaca476447f..6731b8e857a7 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs @@ -95,7 +95,7 @@ private async Task ValidateAsync(RevokeOrganizationUsers request) return result; } - if (IsNonOwnerRevokingAnOwner(userToRevoke, request.ActionPerformedBy)) + if (!IsNonOwnerRevokingAnOwner(userToRevoke, request.ActionPerformedBy)) { result.ErrorMessages.Add($"{OnlyOwnersCanRevokeOtherOwners}"); return result; diff --git a/src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForSingleOrgPolicy.text.hbs b/src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForSingleOrgPolicy.text.hbs index 0a720a7f869c..f933e8cf625e 100644 --- a/src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForSingleOrgPolicy.text.hbs +++ b/src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForSingleOrgPolicy.text.hbs @@ -1,5 +1,5 @@ {{#>BasicTextLayout}} Your user account has been revoked from the {{OrganizationName}} organization because your account is part of multiple organizations. Before you can rejoin {{OrganizationName}}, you must first leave all other organizations. -To leave an organization, first log in the {{link "web app" "https://vault.bitwarden.com/#/login"}}, select the three dot menu next to the organization name, and select Leave. +To leave an organization, first log in the web app (https://vault.bitwarden.com/#/login), select the three dot menu next to the organization name, and select Leave. {{/BasicTextLayout}} diff --git a/src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForTwoFactorPolicy.text.hbs b/src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForTwoFactorPolicy.text.hbs index ef43713766ee..f197f37f00e5 100644 --- a/src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForTwoFactorPolicy.text.hbs +++ b/src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForTwoFactorPolicy.text.hbs @@ -3,5 +3,5 @@ configured. Before you can re-join this organization you need to set up two-step login on your user account. Learn how to enable two-step login on your user account at - {{link "https://help.bitwarden.com/article/setup-two-step-login/"}} + https://help.bitwarden.com/article/setup-two-step-login/ {{/BasicTextLayout}} diff --git a/src/Core/Services/Implementations/HandlebarsMailService.cs b/src/Core/Services/Implementations/HandlebarsMailService.cs index f61a7e30a7e0..312ca5620d52 100644 --- a/src/Core/Services/Implementations/HandlebarsMailService.cs +++ b/src/Core/Services/Implementations/HandlebarsMailService.cs @@ -289,7 +289,7 @@ public async Task SendOrganizationUserRemovedForPolicyTwoStepEmailAsync(string o WebVaultUrl = _globalSettings.BaseServiceUri.VaultWithHash, SiteName = _globalSettings.SiteName }; - await AddMessageContentAsync(message, "OrganizationUserRemovedForPolicyTwoStep", model); + await AddMessageContentAsync(message, "AdminConsole.OrganizationUserRemovedForPolicyTwoStep", model); message.Category = "OrganizationUserRemovedForPolicyTwoStep"; await _mailDeliveryService.SendEmailAsync(message); } @@ -303,7 +303,7 @@ public async Task SendOrganizationUserRevokedForTwoFactoryPolicyEmailAsync(strin WebVaultUrl = _globalSettings.BaseServiceUri.VaultWithHash, SiteName = _globalSettings.SiteName }; - await AddMessageContentAsync(message, "OrganizationUserRevokedForTwoFactorPolicy", model); + await AddMessageContentAsync(message, "AdminConsole.OrganizationUserRevokedForTwoFactorPolicy", model); message.Category = "OrganizationUserRevokedForTwoFactorPolicy"; await _mailDeliveryService.SendEmailAsync(message); } From d3172c077ef2da3a1e076793ac34af3115be41a4 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Wed, 13 Nov 2024 14:30:16 -0600 Subject: [PATCH 16/49] Moving interface into its own file. --- .../IRevokeNonCompliantOrganizationUserCommand.cs | 9 +++++++++ .../RevokeNonCompliantOrganizationUserCommand.cs | 5 ----- 2 files changed, 9 insertions(+), 5 deletions(-) create mode 100644 src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IRevokeNonCompliantOrganizationUserCommand.cs diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IRevokeNonCompliantOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IRevokeNonCompliantOrganizationUserCommand.cs new file mode 100644 index 000000000000..722025cb5808 --- /dev/null +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IRevokeNonCompliantOrganizationUserCommand.cs @@ -0,0 +1,9 @@ +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Requests; +using Bit.Core.AdminConsole.OrganizationFeatures.Shared; + +namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; + +public interface IRevokeNonCompliantOrganizationUserCommand +{ + Task RevokeNonCompliantOrganizationUsersAsync(RevokeOrganizationUsers request); +} diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs index 6731b8e857a7..e3c8ed48e5a7 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs @@ -9,11 +9,6 @@ namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers; -public interface IRevokeNonCompliantOrganizationUserCommand -{ - Task RevokeNonCompliantOrganizationUsersAsync(RevokeOrganizationUsers request); -} - public class RevokeNonCompliantOrganizationUserCommand(IOrganizationUserRepository organizationUserRepository, IEventService eventService, IHasConfirmedOwnersExceptQuery confirmedOwnersExceptQuery, From fe5af90e4fa03220ea6fe7c558f877452977c7d4 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Wed, 13 Nov 2024 14:39:29 -0600 Subject: [PATCH 17/49] Correcting namespaces for email templates. --- src/Core/Services/Implementations/HandlebarsMailService.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Core/Services/Implementations/HandlebarsMailService.cs b/src/Core/Services/Implementations/HandlebarsMailService.cs index 312ca5620d52..043b4df8bfda 100644 --- a/src/Core/Services/Implementations/HandlebarsMailService.cs +++ b/src/Core/Services/Implementations/HandlebarsMailService.cs @@ -289,7 +289,7 @@ public async Task SendOrganizationUserRemovedForPolicyTwoStepEmailAsync(string o WebVaultUrl = _globalSettings.BaseServiceUri.VaultWithHash, SiteName = _globalSettings.SiteName }; - await AddMessageContentAsync(message, "AdminConsole.OrganizationUserRemovedForPolicyTwoStep", model); + await AddMessageContentAsync(message, "OrganizationUserRemovedForPolicyTwoStep", model); message.Category = "OrganizationUserRemovedForPolicyTwoStep"; await _mailDeliveryService.SendEmailAsync(message); } @@ -518,7 +518,7 @@ public async Task SendOrganizationUserRevokedForPolicySingleOrgEmailAsync(string WebVaultUrl = _globalSettings.BaseServiceUri.VaultWithHash, SiteName = _globalSettings.SiteName }; - await AddMessageContentAsync(message, "OrganizationUserRevokedForSingleOrgPolicy", model); + await AddMessageContentAsync(message, "AdminConsole.OrganizationUserRevokedForSingleOrgPolicy", model); message.Category = "OrganizationUserRevokedForSingleOrgPolicy"; await _mailDeliveryService.SendEmailAsync(message); } From 95bb1f4167cd863cf2895c5d3796125a9a22655e Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Wed, 13 Nov 2024 15:40:09 -0600 Subject: [PATCH 18/49] correcting logic that would not allow normal users to revoke non owners. --- .../RevokeNonCompliantOrganizationUserCommand.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs index e3c8ed48e5a7..0f26ca431666 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs @@ -90,7 +90,7 @@ private async Task ValidateAsync(RevokeOrganizationUsers request) return result; } - if (!IsNonOwnerRevokingAnOwner(userToRevoke, request.ActionPerformedBy)) + if (IsNonOwnerRevokingAnOwner(userToRevoke, request.ActionPerformedBy)) { result.ErrorMessages.Add($"{OnlyOwnersCanRevokeOtherOwners}"); return result; @@ -107,7 +107,11 @@ private static bool IsAlreadyRevoked(OrganizationUserUserDetails organizationUse private static bool IsNonOwnerRevokingAnOwner(OrganizationUserUserDetails organizationUser, IActingUser requestingUser) => requestingUser is StandardUser standardUser && - !IsActingUserAllowedToRevokeOwner(organizationUser, standardUser); + !IsActingUserAllowedToRevokeOwner(organizationUser, standardUser) && + !IsActingUserRevokingNonOwner(organizationUser); + + private static bool IsActingUserRevokingNonOwner(OrganizationUserUserDetails organizationUser) => + organizationUser is not { Type: OrganizationUserType.Owner }; private static bool IsActingUserAllowedToRevokeOwner(OrganizationUserUserDetails organizationUser, StandardUser requestingOrganizationUser) => organizationUser is { Type: OrganizationUserType.Owner } From eb693a8be027ab45660c0b37269eddefa147c8bf Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Wed, 13 Nov 2024 16:18:52 -0600 Subject: [PATCH 19/49] Actually correcting the test and logic. --- .../RevokeNonCompliantOrganizationUserCommand.cs | 12 ++++-------- ...RevokeNonCompliantOrganizationUserCommandTests.cs | 3 ++- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs index 0f26ca431666..51b254059059 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs @@ -90,7 +90,7 @@ private async Task ValidateAsync(RevokeOrganizationUsers request) return result; } - if (IsNonOwnerRevokingAnOwner(userToRevoke, request.ActionPerformedBy)) + if (NonOwnersCannotRevokeOwners(userToRevoke, request.ActionPerformedBy)) { result.ErrorMessages.Add($"{OnlyOwnersCanRevokeOtherOwners}"); return result; @@ -105,13 +105,9 @@ private async Task ValidateAsync(RevokeOrganizationUsers request) private static bool IsAlreadyRevoked(OrganizationUserUserDetails organizationUser) => organizationUser is { Status: OrganizationUserStatusType.Revoked }; - private static bool IsNonOwnerRevokingAnOwner(OrganizationUserUserDetails organizationUser, - IActingUser requestingUser) => requestingUser is StandardUser standardUser && - !IsActingUserAllowedToRevokeOwner(organizationUser, standardUser) && - !IsActingUserRevokingNonOwner(organizationUser); - - private static bool IsActingUserRevokingNonOwner(OrganizationUserUserDetails organizationUser) => - organizationUser is not { Type: OrganizationUserType.Owner }; + 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 } diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs index 88b11429a73f..d3efece70e90 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs @@ -148,9 +148,10 @@ public async Task RevokeNonCompliantOrganizationUsersAsync_GivenValidPopulatedRe SutProvider sutProvider) { userToRevoke.OrganizationId = organizationId; + userToRevoke.Type = OrganizationUserType.Admin; var command = new RevokeOrganizationUsers(organizationId, userToRevoke, - new StandardUser(Guid.NewGuid(), true)); + new StandardUser(Guid.NewGuid(), false)); sutProvider.GetDependency() .HasConfirmedOwnersExceptAsync(organizationId, Arg.Any>()) From 58702d2323b9f8ac06141adca71a428cad55b35f Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Wed, 13 Nov 2024 16:20:51 -0600 Subject: [PATCH 20/49] dotnet format. Added exec to bottom of bulk sproc --- .../Policies/PolicyValidators/SingleOrgPolicyValidator.cs | 1 - .../PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs | 1 - .../OrganizationUser_SetStatusForUsersById.sql | 2 ++ .../Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs | 1 - .../TwoFactorAuthenticationPolicyValidatorTests.cs | 1 - 5 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs index b339de73bff4..888f25374120 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs @@ -4,7 +4,6 @@ using Bit.Core.AdminConsole.Enums; using Bit.Core.AdminConsole.Models.Data; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationDomains.Interfaces; -using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Requests; using Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models; diff --git a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs index b8de329b241a..7efd5c1f9a84 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs @@ -3,7 +3,6 @@ using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.Enums; using Bit.Core.AdminConsole.Models.Data; -using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Requests; using Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models; diff --git a/src/Sql/dbo/Stored Procedures/OrganizationUser_SetStatusForUsersById.sql b/src/Sql/dbo/Stored Procedures/OrganizationUser_SetStatusForUsersById.sql index 6476af4aa665..571019bb663f 100644 --- a/src/Sql/dbo/Stored Procedures/OrganizationUser_SetStatusForUsersById.sql +++ b/src/Sql/dbo/Stored Procedures/OrganizationUser_SetStatusForUsersById.sql @@ -11,3 +11,5 @@ BEGIN WHERE [Id] IN (SELECT Id from @OrganizationUserIds) END + +EXEC [dbo].[User_BumpAccountRevisionDateByOrganizationUserIds] @OrganizationUserIds diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs index bd392eb20e65..592c26307dc0 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs @@ -1,6 +1,5 @@ using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.Enums; -using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Requests; using Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models; diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs index 0f4ad2160ea6..9e8445c22e7e 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs @@ -1,6 +1,5 @@ using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.Enums; -using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Requests; using Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models; From 9427b9ff4c1cc8c68a1996148f880d8d02754f9b Mon Sep 17 00:00:00 2001 From: Jared McCannon Date: Fri, 15 Nov 2024 09:45:41 -0600 Subject: [PATCH 21/49] Update src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Rui Tomé <108268980+r-tome@users.noreply.github.com> --- .../RevokeNonCompliantOrganizationUserCommand.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs index 51b254059059..06303cb66570 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs @@ -14,12 +14,12 @@ public class RevokeNonCompliantOrganizationUserCommand(IOrganizationUserReposito IHasConfirmedOwnersExceptQuery confirmedOwnersExceptQuery, TimeProvider timeProvider) : IRevokeNonCompliantOrganizationUserCommand { - public const string CannotRevokeSelfMessage = "You cannot revoke yourself."; - public const string OnlyOwnersCanRevokeOtherOwners = "Only owners can revoke other owners."; - public const string UserAlreadyRevoked = "User is already revoked."; - public const string OrgMustHaveAtLeastOneOwner = "Organization must have at least one confirmed owner."; - public const string InvalidUsers = "Invalid users."; - public const string RequestedByWasNotValid = "Action was performed by an unexpected type."; + 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 RevokeNonCompliantOrganizationUsersAsync(RevokeOrganizationUsers request) { From 99f98f25ee7738606d3ed2bfe74dd9932f56c5ed Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Fri, 15 Nov 2024 09:49:02 -0600 Subject: [PATCH 22/49] Updated OrgIds to be a json string --- ...OrganizationUser_SetStatusForUsersById.sql | 26 +++++++++++++---- .../2024-11-06-00_OrgUserSetStatusBulk.sql | 29 ++++++++++++++----- 2 files changed, 41 insertions(+), 14 deletions(-) diff --git a/src/Sql/dbo/Stored Procedures/OrganizationUser_SetStatusForUsersById.sql b/src/Sql/dbo/Stored Procedures/OrganizationUser_SetStatusForUsersById.sql index 571019bb663f..95ed5a3155d0 100644 --- a/src/Sql/dbo/Stored Procedures/OrganizationUser_SetStatusForUsersById.sql +++ b/src/Sql/dbo/Stored Procedures/OrganizationUser_SetStatusForUsersById.sql @@ -1,15 +1,29 @@ CREATE PROCEDURE [dbo].[OrganizationUser_SetStatusForUsersById] - @OrganizationUserIds AS [dbo].[GuidIdArray] READONLY, + @OrganizationUserIds AS NVARCHAR(MAX), @Status SMALLINT AS BEGIN SET NOCOUNT ON + + -- Declare a table variable to hold the parsed JSON data + DECLARE @ParsedIds TABLE (Id UNIQUEIDENTIFIER); + + -- Parse the JSON input into the table variable + INSERT INTO @ParsedIds (Id) + SELECT value + FROM OPENJSON(@OrganizationUserIds); + + -- Check if the input table is empty + IF (SELECT COUNT(1) FROM @ParsedIds) < 1 + BEGIN + RETURN(-1); + END + UPDATE [dbo].[OrganizationUser] - SET - [Status] = @Status - WHERE - [Id] IN (SELECT Id from @OrganizationUserIds) + SET [Status] = @Status + WHERE [Id] IN (SELECT Id from @ParsedIds) + + EXEC [dbo].[User_BumpAccountRevisionDateByOrganizationUserIds] @OrganizationUserIds END -EXEC [dbo].[User_BumpAccountRevisionDateByOrganizationUserIds] @OrganizationUserIds diff --git a/util/Migrator/DbScripts/2024-11-06-00_OrgUserSetStatusBulk.sql b/util/Migrator/DbScripts/2024-11-06-00_OrgUserSetStatusBulk.sql index dec6edf3b1a8..95151f90ded5 100644 --- a/util/Migrator/DbScripts/2024-11-06-00_OrgUserSetStatusBulk.sql +++ b/util/Migrator/DbScripts/2024-11-06-00_OrgUserSetStatusBulk.sql @@ -1,15 +1,28 @@ -CREATE OR ALTER PROCEDURE [dbo].[OrganizationUser_SetStatusForUsersById] - @OrganizationUserIds AS [dbo].[GuidIdArray] READONLY, +CREATE PROCEDURE [dbo].[OrganizationUser_SetStatusForUsersById] + @OrganizationUserIds AS NVARCHAR(MAX), @Status SMALLINT AS BEGIN SET NOCOUNT ON + + -- Declare a table variable to hold the parsed JSON data + DECLARE @ParsedIds TABLE (Id UNIQUEIDENTIFIER); + + -- Parse the JSON input into the table variable + INSERT INTO @ParsedIds (Id) + SELECT value + FROM OPENJSON(@OrganizationUserIds); + + -- Check if the input table is empty + IF (SELECT COUNT(1) FROM @ParsedIds) < 1 + BEGIN + RETURN(-1); + END + UPDATE [dbo].[OrganizationUser] - SET - [Status] = @Status - WHERE - [Id] IN (SELECT Id from @OrganizationUserIds) -END + SET [Status] = @Status + WHERE [Id] IN (SELECT Id from @ParsedIds) -EXEC [dbo].[User_BumpAccountRevisionDateByOrganizationUserIds] @OrganizationUserIds + EXEC [dbo].[User_BumpAccountRevisionDateByOrganizationUserIds] @OrganizationUserIds +END From 78cd03d73b55ff9768815a9671b12ab107b24983 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Fri, 15 Nov 2024 09:53:39 -0600 Subject: [PATCH 23/49] Fixing errors. --- .../RevokeNonCompliantOrganizationUserCommand.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs index 06303cb66570..c52226bb35e2 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs @@ -61,38 +61,38 @@ private async Task ValidateAsync(RevokeOrganizationUsers request) { if (!PerformedByIsAnExpectedType(request.ActionPerformedBy)) { - return new CommandResult([RequestedByWasNotValid]); + return new CommandResult([ErrorRequestedByWasNotValid]); } if (request.ActionPerformedBy is StandardUser loggableUser && request.OrganizationUsers.Any(x => x.UserId == loggableUser.UserId)) { - return new CommandResult([CannotRevokeSelfMessage]); + return new CommandResult([ErrorCannotRevokeSelf]); } if (request.OrganizationUsers.Any(x => x.OrganizationId != request.OrganizationId)) { - return new CommandResult([InvalidUsers]); + return new CommandResult([ErrorInvalidUsers]); } if (!await confirmedOwnersExceptQuery.HasConfirmedOwnersExceptAsync( request.OrganizationId, request.OrganizationUsers.Select(x => x.Id))) { - return new CommandResult([OrgMustHaveAtLeastOneOwner]); + return new CommandResult([ErrorOrgMustHaveAtLeastOneOwner]); } return request.OrganizationUsers.Aggregate(new CommandResult(), (result, userToRevoke) => { if (IsAlreadyRevoked(userToRevoke)) { - result.ErrorMessages.Add($"{UserAlreadyRevoked} Id: {userToRevoke.Id}"); + result.ErrorMessages.Add($"{ErrorUserAlreadyRevoked} Id: {userToRevoke.Id}"); return result; } if (NonOwnersCannotRevokeOwners(userToRevoke, request.ActionPerformedBy)) { - result.ErrorMessages.Add($"{OnlyOwnersCanRevokeOtherOwners}"); + result.ErrorMessages.Add($"{ErrorOnlyOwnersCanRevokeOtherOwners}"); return result; } From 436bebdbea69c859c51d777d9d93210917ccd9ed Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Fri, 15 Nov 2024 09:54:53 -0600 Subject: [PATCH 24/49] Updating test --- ...RevokeNonCompliantOrganizationUserCommandTests.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs index d3efece70e90..c3b471360e65 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs @@ -24,7 +24,7 @@ public async Task RevokeNonCompliantOrganizationUsersAsync_GivenUnrecognizedUser var result = await sutProvider.Sut.RevokeNonCompliantOrganizationUsersAsync(command); Assert.True(result.HasErrors); - Assert.Contains(RevokeNonCompliantOrganizationUserCommand.RequestedByWasNotValid, result.ErrorMessages); + Assert.Contains(RevokeNonCompliantOrganizationUserCommand.ErrorRequestedByWasNotValid, result.ErrorMessages); } [Theory, BitAutoData] @@ -38,7 +38,7 @@ public async Task RevokeNonCompliantOrganizationUsersAsync_GivenPopulatedRequest var result = await sutProvider.Sut.RevokeNonCompliantOrganizationUsersAsync(command); Assert.True(result.HasErrors); - Assert.Contains(RevokeNonCompliantOrganizationUserCommand.CannotRevokeSelfMessage, result.ErrorMessages); + Assert.Contains(RevokeNonCompliantOrganizationUserCommand.ErrorCannotRevokeSelf, result.ErrorMessages); } [Theory, BitAutoData] @@ -54,7 +54,7 @@ public async Task RevokeNonCompliantOrganizationUsersAsync_GivenPopulatedRequest var result = await sutProvider.Sut.RevokeNonCompliantOrganizationUsersAsync(command); Assert.True(result.HasErrors); - Assert.Contains(RevokeNonCompliantOrganizationUserCommand.InvalidUsers, result.ErrorMessages); + Assert.Contains(RevokeNonCompliantOrganizationUserCommand.ErrorInvalidUsers, result.ErrorMessages); } [Theory, BitAutoData] @@ -74,7 +74,7 @@ public async Task RevokeNonCompliantOrganizationUsersAsync_GivenPopulatedRequest var result = await sutProvider.Sut.RevokeNonCompliantOrganizationUsersAsync(command); Assert.True(result.HasErrors); - Assert.Contains(RevokeNonCompliantOrganizationUserCommand.OrgMustHaveAtLeastOneOwner, result.ErrorMessages); + Assert.Contains(RevokeNonCompliantOrganizationUserCommand.ErrorOrgMustHaveAtLeastOneOwner, result.ErrorMessages); } [Theory, BitAutoData] @@ -95,7 +95,7 @@ public async Task RevokeNonCompliantOrganizationUsersAsync_GivenPopulatedRequest var result = await sutProvider.Sut.RevokeNonCompliantOrganizationUsersAsync(command); Assert.True(result.HasErrors); - Assert.Contains(RevokeNonCompliantOrganizationUserCommand.OnlyOwnersCanRevokeOtherOwners, result.ErrorMessages); + Assert.Contains(RevokeNonCompliantOrganizationUserCommand.ErrorOnlyOwnersCanRevokeOtherOwners, result.ErrorMessages); } [Theory, BitAutoData] @@ -116,7 +116,7 @@ public async Task RevokeNonCompliantOrganizationUsersAsync_GivenPopulatedRequest var result = await sutProvider.Sut.RevokeNonCompliantOrganizationUsersAsync(command); Assert.True(result.HasErrors); - Assert.Contains($"{RevokeNonCompliantOrganizationUserCommand.UserAlreadyRevoked} Id: {userToRevoke.Id}", result.ErrorMessages); + Assert.Contains($"{RevokeNonCompliantOrganizationUserCommand.ErrorUserAlreadyRevoked} Id: {userToRevoke.Id}", result.ErrorMessages); } [Theory, BitAutoData] From 8b89ad6148ef9abed17e45ba2f26c5314c84f61d Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Fri, 15 Nov 2024 10:07:04 -0600 Subject: [PATCH 25/49] Moving command result. --- .../Interfaces/IRevokeNonCompliantOrganizationUserCommand.cs | 2 +- .../RevokeNonCompliantOrganizationUserCommand.cs | 2 +- .../Shared => Models/Commands}/CommandResult.cs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) rename src/Core/{AdminConsole/OrganizationFeatures/Shared => Models/Commands}/CommandResult.cs (81%) diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IRevokeNonCompliantOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IRevokeNonCompliantOrganizationUserCommand.cs index 722025cb5808..74798fd54974 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IRevokeNonCompliantOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IRevokeNonCompliantOrganizationUserCommand.cs @@ -1,5 +1,5 @@ using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Requests; -using Bit.Core.AdminConsole.OrganizationFeatures.Shared; +using Bit.Core.Models.Commands; namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs index c52226bb35e2..366e12b9b32c 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs @@ -1,8 +1,8 @@ using Bit.Core.AdminConsole.Models.Data; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Requests; -using Bit.Core.AdminConsole.OrganizationFeatures.Shared; using Bit.Core.Enums; +using Bit.Core.Models.Commands; using Bit.Core.Models.Data.Organizations.OrganizationUsers; using Bit.Core.Repositories; using Bit.Core.Services; diff --git a/src/Core/AdminConsole/OrganizationFeatures/Shared/CommandResult.cs b/src/Core/Models/Commands/CommandResult.cs similarity index 81% rename from src/Core/AdminConsole/OrganizationFeatures/Shared/CommandResult.cs rename to src/Core/Models/Commands/CommandResult.cs index 73d9c6fc2511..23e6cb529f90 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Shared/CommandResult.cs +++ b/src/Core/Models/Commands/CommandResult.cs @@ -1,4 +1,4 @@ -namespace Bit.Core.AdminConsole.OrganizationFeatures.Shared; +namespace Bit.Core.Models.Commands; public class CommandResult(IEnumerable errors) { From 0014d5c8bc51011eccd31d51132b7578b06b1de6 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Fri, 15 Nov 2024 10:15:50 -0600 Subject: [PATCH 26/49] Formatting and request rename --- ...IRevokeNonCompliantOrganizationUserCommand.cs | 2 +- .../Requests/RevokeOrganizationUser.cs | 4 ++-- .../RevokeNonCompliantOrganizationUserCommand.cs | 4 ++-- .../PolicyValidators/SingleOrgPolicyValidator.cs | 2 +- .../TwoFactorAuthenticationPolicyValidator.cs | 2 +- ...keNonCompliantOrganizationUserCommandTests.cs | 16 ++++++++-------- .../SingleOrgPolicyValidatorTests.cs | 2 +- ...woFactorAuthenticationPolicyValidatorTests.cs | 2 +- 8 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IRevokeNonCompliantOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IRevokeNonCompliantOrganizationUserCommand.cs index 74798fd54974..c9768a890576 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IRevokeNonCompliantOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IRevokeNonCompliantOrganizationUserCommand.cs @@ -5,5 +5,5 @@ namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interface public interface IRevokeNonCompliantOrganizationUserCommand { - Task RevokeNonCompliantOrganizationUsersAsync(RevokeOrganizationUsers request); + Task RevokeNonCompliantOrganizationUsersAsync(RevokeOrganizationUsersRequest request); } diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Requests/RevokeOrganizationUser.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Requests/RevokeOrganizationUser.cs index 7dbb21dd854d..88f1dc8aa1f0 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Requests/RevokeOrganizationUser.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Requests/RevokeOrganizationUser.cs @@ -3,11 +3,11 @@ namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Requests; -public record RevokeOrganizationUsers( +public record RevokeOrganizationUsersRequest( Guid OrganizationId, IEnumerable OrganizationUsers, IActingUser ActionPerformedBy) { - public RevokeOrganizationUsers(Guid organizationId, OrganizationUserUserDetails organizationUser, IActingUser actionPerformedBy) + public RevokeOrganizationUsersRequest(Guid organizationId, OrganizationUserUserDetails organizationUser, IActingUser actionPerformedBy) : this(organizationId, [organizationUser], actionPerformedBy) { } } diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs index 366e12b9b32c..b5fde39d1035 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs @@ -21,7 +21,7 @@ public class RevokeNonCompliantOrganizationUserCommand(IOrganizationUserReposito public const string ErrorInvalidUsers = "Invalid users."; public const string ErrorRequestedByWasNotValid = "Action was performed by an unexpected type."; - public async Task RevokeNonCompliantOrganizationUsersAsync(RevokeOrganizationUsers request) + public async Task RevokeNonCompliantOrganizationUsersAsync(RevokeOrganizationUsersRequest request) { var validationResult = await ValidateAsync(request); @@ -57,7 +57,7 @@ private static (OrganizationUserUserDetails organizationUser, EventType eventTyp OrganizationUserUserDetails organizationUser, EventSystemUser systemUser, DateTimeOffset dateTimeOffset) => new(organizationUser, EventType.OrganizationUser_Revoked, systemUser, dateTimeOffset.UtcDateTime); - private async Task ValidateAsync(RevokeOrganizationUsers request) + private async Task ValidateAsync(RevokeOrganizationUsersRequest request) { if (!PerformedByIsAnExpectedType(request.ActionPerformedBy)) { diff --git a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs index 888f25374120..2060c5f9026e 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs @@ -97,7 +97,7 @@ private async Task RemoveNonCompliantUsersAsync(Guid organizationId) .ToList(); var commandResult = await _revokeNonCompliantOrganizationUserCommand.RevokeNonCompliantOrganizationUsersAsync( - new RevokeOrganizationUsers(organizationId, revocableUsers, new StandardUser(savingUserId ?? Guid.Empty, isOwner))); + new RevokeOrganizationUsersRequest(organizationId, revocableUsers, new StandardUser(savingUserId ?? Guid.Empty, isOwner))); if (commandResult.HasErrors) { diff --git a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs index 7efd5c1f9a84..e1668e851817 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs @@ -84,7 +84,7 @@ private async Task RemoveNonCompliantUsersAsync(Guid organizationId) } var result = await _revokeNonCompliantOrganizationUserCommand.RevokeNonCompliantOrganizationUsersAsync( - new RevokeOrganizationUsers(organizationId, revocableOrgUsers, + new RevokeOrganizationUsersRequest(organizationId, revocableOrgUsers, new StandardUser(savingUserId ?? Guid.Empty, await _currentContext.OrganizationOwner(organizationId)))); diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs index c3b471360e65..d14af40376de 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs @@ -19,7 +19,7 @@ public class RevokeNonCompliantOrganizationUserCommandTests public async Task RevokeNonCompliantOrganizationUsersAsync_GivenUnrecognizedUserType_WhenAttemptingToRevoke_ThenErrorShouldBeReturned( Guid organizationId, SutProvider sutProvider) { - var command = new RevokeOrganizationUsers(organizationId, [], new InvalidUser()); + var command = new RevokeOrganizationUsersRequest(organizationId, [], new InvalidUser()); var result = await sutProvider.Sut.RevokeNonCompliantOrganizationUsersAsync(command); @@ -32,7 +32,7 @@ public async Task RevokeNonCompliantOrganizationUsersAsync_GivenPopulatedRequest Guid organizationId, OrganizationUserUserDetails revokingUser, SutProvider sutProvider) { - var command = new RevokeOrganizationUsers(organizationId, revokingUser, + var command = new RevokeOrganizationUsersRequest(organizationId, revokingUser, new StandardUser(revokingUser?.UserId ?? Guid.NewGuid(), true)); var result = await sutProvider.Sut.RevokeNonCompliantOrganizationUsersAsync(command); @@ -48,7 +48,7 @@ public async Task RevokeNonCompliantOrganizationUsersAsync_GivenPopulatedRequest { userFromAnotherOrg.OrganizationId = Guid.NewGuid(); - var command = new RevokeOrganizationUsers(organizationId, userFromAnotherOrg, + var command = new RevokeOrganizationUsersRequest(organizationId, userFromAnotherOrg, new StandardUser(Guid.NewGuid(), true)); var result = await sutProvider.Sut.RevokeNonCompliantOrganizationUsersAsync(command); @@ -64,7 +64,7 @@ public async Task RevokeNonCompliantOrganizationUsersAsync_GivenPopulatedRequest { userToRevoke.OrganizationId = organizationId; - var command = new RevokeOrganizationUsers(organizationId, userToRevoke, + var command = new RevokeOrganizationUsersRequest(organizationId, userToRevoke, new StandardUser(Guid.NewGuid(), true)); sutProvider.GetDependency() @@ -85,7 +85,7 @@ public async Task RevokeNonCompliantOrganizationUsersAsync_GivenPopulatedRequest userToRevoke.OrganizationId = organizationId; userToRevoke.Type = OrganizationUserType.Owner; - var command = new RevokeOrganizationUsers(organizationId, userToRevoke, + var command = new RevokeOrganizationUsersRequest(organizationId, userToRevoke, new StandardUser(Guid.NewGuid(), false)); sutProvider.GetDependency() @@ -106,7 +106,7 @@ public async Task RevokeNonCompliantOrganizationUsersAsync_GivenPopulatedRequest userToRevoke.OrganizationId = organizationId; userToRevoke.Status = OrganizationUserStatusType.Revoked; - var command = new RevokeOrganizationUsers(organizationId, userToRevoke, + var command = new RevokeOrganizationUsersRequest(organizationId, userToRevoke, new StandardUser(Guid.NewGuid(), true)); sutProvider.GetDependency() @@ -129,7 +129,7 @@ public async Task RevokeNonCompliantOrganizationUsersAsync_GivenPopulatedRequest revocableUsers[0].Type = OrganizationUserType.Owner; revocableUsers[1].Status = OrganizationUserStatusType.Revoked; - var command = new RevokeOrganizationUsers(organizationId, revocableUsers, + var command = new RevokeOrganizationUsersRequest(organizationId, revocableUsers, new StandardUser(Guid.NewGuid(), false)); sutProvider.GetDependency() @@ -150,7 +150,7 @@ public async Task RevokeNonCompliantOrganizationUsersAsync_GivenValidPopulatedRe userToRevoke.OrganizationId = organizationId; userToRevoke.Type = OrganizationUserType.Admin; - var command = new RevokeOrganizationUsers(organizationId, userToRevoke, + var command = new RevokeOrganizationUsersRequest(organizationId, userToRevoke, new StandardUser(Guid.NewGuid(), false)); sutProvider.GetDependency() diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs index 592c26307dc0..bae8a5c60401 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs @@ -190,7 +190,7 @@ public async Task OnSaveSideEffectsAsync_GivenUpdatedSingleOrgPolicy_WhenAccount await sutProvider.GetDependency() .DidNotReceive() - .RevokeNonCompliantOrganizationUsersAsync(Arg.Any()); + .RevokeNonCompliantOrganizationUsersAsync(Arg.Any()); } // TODO feature flag is enabled and revoke returns errors, we throw // TODO feature flag is enabled and revoke returns no errors, no throw diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs index 9e8445c22e7e..c99b3ece3b65 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs @@ -258,7 +258,7 @@ public async Task OnSaveSideEffectsAsync_GivenUpdateTo2faPolicy_WhenAccountProvi await sutProvider.GetDependency() .DidNotReceive() - .RevokeNonCompliantOrganizationUsersAsync(Arg.Any()); + .RevokeNonCompliantOrganizationUsersAsync(Arg.Any()); } [Theory, BitAutoData] From 82b65e03be7addeea4a4205d6775f2f94db454b1 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Fri, 15 Nov 2024 10:55:32 -0600 Subject: [PATCH 27/49] Realized this would throw a null error from the system domain verification. Adding unknown type to event system user. Adding optional parameter to SaveAsync in policy service in order to pass in event system user. --- .../AdminConsole/Enums/EventSystemUser.cs | 1 + .../VerifyOrganizationDomainCommand.cs | 86 +++++++++---------- .../AdminConsole/Services/IPolicyService.cs | 2 +- .../Services/Implementations/PolicyService.cs | 4 +- .../VerifyOrganizationDomainCommandTests.cs | 8 +- .../Services/EventServiceTests.cs | 1 - 6 files changed, 49 insertions(+), 53 deletions(-) diff --git a/src/Core/AdminConsole/Enums/EventSystemUser.cs b/src/Core/AdminConsole/Enums/EventSystemUser.cs index df9be9a350b7..c3e13705dd9f 100644 --- a/src/Core/AdminConsole/Enums/EventSystemUser.cs +++ b/src/Core/AdminConsole/Enums/EventSystemUser.cs @@ -2,6 +2,7 @@ public enum EventSystemUser : byte { + Unknown = 0, SCIM = 1, DomainVerification = 2, PublicApi = 3, diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs index 870fa72aa709..f44d1251cb6b 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs @@ -1,7 +1,9 @@ using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.Enums; +using Bit.Core.AdminConsole.Models.Data; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationDomains.Interfaces; using Bit.Core.AdminConsole.Services; +using Bit.Core.Context; using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Exceptions; @@ -12,124 +14,114 @@ namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationDomains; -public class VerifyOrganizationDomainCommand : IVerifyOrganizationDomainCommand +public class VerifyOrganizationDomainCommand( + IOrganizationDomainRepository organizationDomainRepository, + IDnsResolverService dnsResolverService, + IEventService eventService, + IGlobalSettings globalSettings, + IPolicyService policyService, + IFeatureService featureService, + ICurrentContext currentContext, + ILogger logger) + : IVerifyOrganizationDomainCommand { - private readonly IOrganizationDomainRepository _organizationDomainRepository; - private readonly IDnsResolverService _dnsResolverService; - private readonly IEventService _eventService; - private readonly IGlobalSettings _globalSettings; - private readonly IPolicyService _policyService; - private readonly IFeatureService _featureService; - private readonly ILogger _logger; - - public VerifyOrganizationDomainCommand( - IOrganizationDomainRepository organizationDomainRepository, - IDnsResolverService dnsResolverService, - IEventService eventService, - IGlobalSettings globalSettings, - IPolicyService policyService, - IFeatureService featureService, - ILogger logger) - { - _organizationDomainRepository = organizationDomainRepository; - _dnsResolverService = dnsResolverService; - _eventService = eventService; - _globalSettings = globalSettings; - _policyService = policyService; - _featureService = featureService; - _logger = logger; - } public async Task UserVerifyOrganizationDomainAsync(OrganizationDomain organizationDomain) { - var domainVerificationResult = await VerifyOrganizationDomainAsync(organizationDomain); + var actingUser = new StandardUser(currentContext.UserId ?? Guid.Empty, await currentContext.OrganizationOwner(organizationDomain.OrganizationId)); - await _eventService.LogOrganizationDomainEventAsync(domainVerificationResult, + var domainVerificationResult = await VerifyOrganizationDomainAsync(organizationDomain, actingUser); + + await eventService.LogOrganizationDomainEventAsync(domainVerificationResult, domainVerificationResult.VerifiedDate != null ? EventType.OrganizationDomain_Verified : EventType.OrganizationDomain_NotVerified); - await _organizationDomainRepository.ReplaceAsync(domainVerificationResult); + await organizationDomainRepository.ReplaceAsync(domainVerificationResult); return domainVerificationResult; } public async Task SystemVerifyOrganizationDomainAsync(OrganizationDomain organizationDomain) { + var actingUser = new SystemUser(EventSystemUser.DomainVerification); + organizationDomain.SetJobRunCount(); - var domainVerificationResult = await VerifyOrganizationDomainAsync(organizationDomain); + var domainVerificationResult = await VerifyOrganizationDomainAsync(organizationDomain, actingUser); if (domainVerificationResult.VerifiedDate is not null) { - _logger.LogInformation(Constants.BypassFiltersEventId, "Successfully validated domain"); + logger.LogInformation(Constants.BypassFiltersEventId, "Successfully validated domain"); - await _eventService.LogOrganizationDomainEventAsync(domainVerificationResult, + await eventService.LogOrganizationDomainEventAsync(domainVerificationResult, EventType.OrganizationDomain_Verified, EventSystemUser.DomainVerification); } else { - domainVerificationResult.SetNextRunDate(_globalSettings.DomainVerification.VerificationInterval); + domainVerificationResult.SetNextRunDate(globalSettings.DomainVerification.VerificationInterval); - await _eventService.LogOrganizationDomainEventAsync(domainVerificationResult, + await eventService.LogOrganizationDomainEventAsync(domainVerificationResult, EventType.OrganizationDomain_NotVerified, EventSystemUser.DomainVerification); - _logger.LogInformation(Constants.BypassFiltersEventId, + logger.LogInformation(Constants.BypassFiltersEventId, "Verification for organization {OrgId} with domain {Domain} failed", domainVerificationResult.OrganizationId, domainVerificationResult.DomainName); } - await _organizationDomainRepository.ReplaceAsync(domainVerificationResult); + await organizationDomainRepository.ReplaceAsync(domainVerificationResult); return domainVerificationResult; } - private async Task VerifyOrganizationDomainAsync(OrganizationDomain domain) + private async Task VerifyOrganizationDomainAsync(OrganizationDomain domain, IActingUser actingUser) { domain.SetLastCheckedDate(); if (domain.VerifiedDate is not null) { - await _organizationDomainRepository.ReplaceAsync(domain); + await organizationDomainRepository.ReplaceAsync(domain); throw new ConflictException("Domain has already been verified."); } var claimedDomain = - await _organizationDomainRepository.GetClaimedDomainsByDomainNameAsync(domain.DomainName); + await organizationDomainRepository.GetClaimedDomainsByDomainNameAsync(domain.DomainName); if (claimedDomain.Count > 0) { - await _organizationDomainRepository.ReplaceAsync(domain); + await organizationDomainRepository.ReplaceAsync(domain); throw new ConflictException("The domain is not available to be claimed."); } try { - if (await _dnsResolverService.ResolveAsync(domain.DomainName, domain.Txt)) + if (await dnsResolverService.ResolveAsync(domain.DomainName, domain.Txt)) { domain.SetVerifiedDate(); - await EnableSingleOrganizationPolicyAsync(domain.OrganizationId); + await EnableSingleOrganizationPolicyAsync(domain.OrganizationId, actingUser); } } catch (Exception e) { - _logger.LogError("Error verifying Organization domain: {domain}. {errorMessage}", + logger.LogError("Error verifying Organization domain: {domain}. {errorMessage}", domain.DomainName, e.Message); } return domain; } - private async Task EnableSingleOrganizationPolicyAsync(Guid organizationId) + private async Task EnableSingleOrganizationPolicyAsync(Guid organizationId, IActingUser actingUser) { - if (_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning)) + if (featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning)) { - await _policyService.SaveAsync( - new Policy { OrganizationId = organizationId, Type = PolicyType.SingleOrg, Enabled = true }, null); + await policyService.SaveAsync( + new Policy { OrganizationId = organizationId, Type = PolicyType.SingleOrg, Enabled = true }, + savingUserId: actingUser is StandardUser standardUser ? standardUser.UserId : null, + eventSystemUser: actingUser is SystemUser systemUser ? systemUser.SystemUserType : null); } } } diff --git a/src/Core/AdminConsole/Services/IPolicyService.cs b/src/Core/AdminConsole/Services/IPolicyService.cs index 16ff2f4fa11c..715c3a34d934 100644 --- a/src/Core/AdminConsole/Services/IPolicyService.cs +++ b/src/Core/AdminConsole/Services/IPolicyService.cs @@ -9,7 +9,7 @@ namespace Bit.Core.AdminConsole.Services; public interface IPolicyService { - Task SaveAsync(Policy policy, Guid? savingUserId); + Task SaveAsync(Policy policy, Guid? savingUserId, EventSystemUser? eventSystemUser = null); /// /// Get the combined master password policy options for the specified user. diff --git a/src/Core/AdminConsole/Services/Implementations/PolicyService.cs b/src/Core/AdminConsole/Services/Implementations/PolicyService.cs index 29d78910c8d2..69ec27fd8a2a 100644 --- a/src/Core/AdminConsole/Services/Implementations/PolicyService.cs +++ b/src/Core/AdminConsole/Services/Implementations/PolicyService.cs @@ -70,7 +70,7 @@ public PolicyService( _currentContext = currentContext; } - public async Task SaveAsync(Policy policy, Guid? savingUserId) + public async Task SaveAsync(Policy policy, Guid? savingUserId, EventSystemUser? eventSystemUser = null) { if (_featureService.IsEnabled(FeatureFlagKeys.Pm13322AddPolicyDefinitions)) { @@ -84,7 +84,7 @@ public async Task SaveAsync(Policy policy, Guid? savingUserId) Data = policy.Data, PerformedBy = savingUserId.HasValue ? new StandardUser(savingUserId.Value, await _currentContext.OrganizationOwner(policy.OrganizationId)) - : null + : new SystemUser(eventSystemUser ?? EventSystemUser.Unknown) }; await _savePolicyCommand.SaveAsync(policyUpdate); diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommandTests.cs index 2fcaf8134c80..e2a12c675e81 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommandTests.cs @@ -2,6 +2,7 @@ using Bit.Core.AdminConsole.Enums; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationDomains; using Bit.Core.AdminConsole.Services; +using Bit.Core.Context; using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Exceptions; @@ -143,7 +144,7 @@ await sutProvider.GetDependency().ReceivedWithAnyArgs(1) [Theory, BitAutoData] public async Task UserVerifyOrganizationDomainAsync_GivenOrganizationDomainWithAccountDeprovisioningEnabled_WhenDomainIsVerified_ThenSingleOrgPolicyShouldBeEnabled( - OrganizationDomain domain, SutProvider sutProvider) + OrganizationDomain domain, Guid userId, SutProvider sutProvider) { sutProvider.GetDependency() .GetClaimedDomainsByDomainNameAsync(domain.DomainName) @@ -157,11 +158,14 @@ public async Task UserVerifyOrganizationDomainAsync_GivenOrganizationDomainWithA .IsEnabled(FeatureFlagKeys.AccountDeprovisioning) .Returns(true); + sutProvider.GetDependency() + .UserId.Returns(userId); + _ = await sutProvider.Sut.UserVerifyOrganizationDomainAsync(domain); await sutProvider.GetDependency() .Received(1) - .SaveAsync(Arg.Is(x => x.Type == PolicyType.SingleOrg && x.OrganizationId == domain.OrganizationId && x.Enabled), null); + .SaveAsync(Arg.Is(x => x.Type == PolicyType.SingleOrg && x.OrganizationId == domain.OrganizationId && x.Enabled), userId); } [Theory, BitAutoData] diff --git a/test/Core.Test/AdminConsole/Services/EventServiceTests.cs b/test/Core.Test/AdminConsole/Services/EventServiceTests.cs index 18f5371b4943..d064fce2ecd8 100644 --- a/test/Core.Test/AdminConsole/Services/EventServiceTests.cs +++ b/test/Core.Test/AdminConsole/Services/EventServiceTests.cs @@ -169,7 +169,6 @@ public async Task LogOrganizationUserEvent_WithEventSystemUser_LogsRequiredInfo( new EventMessage() { IpAddress = ipAddress, - DeviceType = DeviceType.Server, OrganizationId = orgUser.OrganizationId, UserId = orgUser.UserId, OrganizationUserId = orgUser.Id, From 51d0b122f5b84492313ca4c17b03fb794f4ccca2 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Tue, 19 Nov 2024 13:51:17 -0600 Subject: [PATCH 28/49] Code review changes --- .../AdminConsole/Models/Data/IActingUser.cs | 2 +- .../AdminConsole/Models/Data/StandardUser.cs | 4 +- .../AdminConsole/Models/Data/SystemUser.cs | 2 +- ...vokeNonCompliantOrganizationUserCommand.cs | 47 ++++---- .../SingleOrgPolicyValidator.cs | 92 ++++++++++------ .../TwoFactorAuthenticationPolicyValidator.cs | 100 +++++++++++------- .../IOrganizationUserRepository.cs | 2 +- src/Core/Models/Commands/CommandResult.cs | 2 + .../OrganizationUserRepository.cs | 4 +- .../OrganizationUserRepository.cs | 6 +- ...onCompliantOrganizationUserCommandTests.cs | 4 +- 11 files changed, 157 insertions(+), 108 deletions(-) diff --git a/src/Core/AdminConsole/Models/Data/IActingUser.cs b/src/Core/AdminConsole/Models/Data/IActingUser.cs index 4e678bb1413d..f97235f34cc4 100644 --- a/src/Core/AdminConsole/Models/Data/IActingUser.cs +++ b/src/Core/AdminConsole/Models/Data/IActingUser.cs @@ -5,6 +5,6 @@ namespace Bit.Core.AdminConsole.Models.Data; public interface IActingUser { Guid? UserId { get; } - bool IsOrganizationOwner { get; } + bool IsOrganizationOwnerOrProvider { get; } EventSystemUser? SystemUserType { get; } } diff --git a/src/Core/AdminConsole/Models/Data/StandardUser.cs b/src/Core/AdminConsole/Models/Data/StandardUser.cs index 7b536741f5bb..f21a41db7c0e 100644 --- a/src/Core/AdminConsole/Models/Data/StandardUser.cs +++ b/src/Core/AdminConsole/Models/Data/StandardUser.cs @@ -7,10 +7,10 @@ public class StandardUser : IActingUser public StandardUser(Guid userId, bool isOrganizationOwner) { UserId = userId; - IsOrganizationOwner = isOrganizationOwner; + IsOrganizationOwnerOrProvider = isOrganizationOwner; } public Guid? UserId { get; } - public bool IsOrganizationOwner { get; } + public bool IsOrganizationOwnerOrProvider { get; } public EventSystemUser? SystemUserType => throw new Exception($"{nameof(StandardUser)} does not have a {nameof(SystemUserType)}"); } diff --git a/src/Core/AdminConsole/Models/Data/SystemUser.cs b/src/Core/AdminConsole/Models/Data/SystemUser.cs index e73c74c9caee..c4859f928fa5 100644 --- a/src/Core/AdminConsole/Models/Data/SystemUser.cs +++ b/src/Core/AdminConsole/Models/Data/SystemUser.cs @@ -11,6 +11,6 @@ public SystemUser(EventSystemUser systemUser) public Guid? UserId => throw new Exception($"{nameof(SystemUserType)} does not have a {nameof(UserId)}."); - public bool IsOrganizationOwner => false; + public bool IsOrganizationOwnerOrProvider => false; public EventSystemUser? SystemUserType { get; } } diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs index b5fde39d1035..dec44e7c17d4 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs @@ -30,28 +30,29 @@ public async Task RevokeNonCompliantOrganizationUsersAsync(Revoke return validationResult; } - await organizationUserRepository.SetOrganizationUsersStatusAsync(request.OrganizationUsers.Select(x => x.Id), - OrganizationUserStatusType.Revoked); + await organizationUserRepository.RevokeOrganizationUserAsync(request.OrganizationUsers.Select(x => x.Id)); - if (request.ActionPerformedBy.GetType() == typeof(StandardUser)) - { - await eventService.LogOrganizationUserEventsAsync( - request.OrganizationUsers.Select(x => GetRevokedUserEventTuple(x, timeProvider.GetUtcNow()))); - } - else if (request.ActionPerformedBy is SystemUser { SystemUserType: not null } loggableSystem) + var now = timeProvider.GetUtcNow(); + + switch (request.ActionPerformedBy) { - await eventService.LogOrganizationUserEventsAsync( - request.OrganizationUsers.Select(x => - GetRevokedUserEventBySystemUserTuple(x, loggableSystem.SystemUserType.Value, - timeProvider.GetUtcNow()))); + 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; } return validationResult; } private static (OrganizationUserUserDetails organizationUser, EventType eventType, DateTime? time) GetRevokedUserEventTuple( - OrganizationUserUserDetails organizationUser, DateTimeOffset dateTimeOffset) => new(organizationUser, - EventType.OrganizationUser_Revoked, dateTimeOffset.UtcDateTime); + OrganizationUserUserDetails organizationUser, DateTimeOffset dateTimeOffset) => + new(organizationUser, EventType.OrganizationUser_Revoked, dateTimeOffset.UtcDateTime); private static (OrganizationUserUserDetails organizationUser, EventType eventType, EventSystemUser eventSystemUser, DateTime? time) GetRevokedUserEventBySystemUserTuple( OrganizationUserUserDetails organizationUser, EventSystemUser systemUser, DateTimeOffset dateTimeOffset) => new(organizationUser, @@ -61,25 +62,25 @@ private async Task ValidateAsync(RevokeOrganizationUsersRequest r { if (!PerformedByIsAnExpectedType(request.ActionPerformedBy)) { - return new CommandResult([ErrorRequestedByWasNotValid]); + return new CommandResult(ErrorRequestedByWasNotValid); } - if (request.ActionPerformedBy is StandardUser loggableUser - && request.OrganizationUsers.Any(x => x.UserId == loggableUser.UserId)) + if (request.ActionPerformedBy is StandardUser user + && request.OrganizationUsers.Any(x => x.UserId == user.UserId)) { - return new CommandResult([ErrorCannotRevokeSelf]); + return new CommandResult(ErrorCannotRevokeSelf); } if (request.OrganizationUsers.Any(x => x.OrganizationId != request.OrganizationId)) { - return new CommandResult([ErrorInvalidUsers]); + return new CommandResult(ErrorInvalidUsers); } if (!await confirmedOwnersExceptQuery.HasConfirmedOwnersExceptAsync( request.OrganizationId, request.OrganizationUsers.Select(x => x.Id))) { - return new CommandResult([ErrorOrgMustHaveAtLeastOneOwner]); + return new CommandResult(ErrorOrgMustHaveAtLeastOneOwner); } return request.OrganizationUsers.Aggregate(new CommandResult(), (result, userToRevoke) => @@ -107,9 +108,5 @@ private static bool IsAlreadyRevoked(OrganizationUserUserDetails organizationUse 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; + actingUser is StandardUser { IsOrganizationOwnerOrProvider: false } && organizationUser.Type == OrganizationUserType.Owner; } diff --git a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs index 2060c5f9026e..683171012ce1 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs @@ -20,6 +20,8 @@ namespace Bit.Core.AdminConsole.OrganizationFeatures.Policies.PolicyValidators; public class SingleOrgPolicyValidator : IPolicyValidator { public PolicyType Type => PolicyType.SingleOrg; + private const string OrganizationNotFoundErrorMessage = "Organization not found."; + private const string ClaimedDomainSingleOrganizationRequiredErrorMessage = "The Single organization policy is required for organizations that have enabled domain verification."; private readonly IOrganizationUserRepository _organizationUserRepository; private readonly IMailService _mailService; @@ -59,10 +61,55 @@ public async Task OnSaveSideEffectsAsync(PolicyUpdate policyUpdate, Policy? curr { if (currentPolicy is not { Enabled: true } && policyUpdate is { Enabled: true }) { - await RemoveNonCompliantUsersAsync(policyUpdate.OrganizationId); + if (_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning)) + { + var currentUser = _currentContext.UserId ?? Guid.Empty; + var isOwnerOrProvider = await _currentContext.OrganizationOwner(policyUpdate.OrganizationId); + await RevokeNonCompliantUsersAsync(policyUpdate.OrganizationId, policyUpdate.PerformedBy ?? new StandardUser(currentUser, isOwnerOrProvider)); + } + else + { + await RemoveNonCompliantUsersAsync(policyUpdate.OrganizationId); + } } } + private async Task RevokeNonCompliantUsersAsync(Guid organizationId, IActingUser performedBy) + { + var organization = await _organizationRepository.GetByIdAsync(organizationId); + + if (organization is null) + { + throw new NotFoundException(OrganizationNotFoundErrorMessage); + } + + var currentActiveRevocableOrganizationUsers = + (await _organizationUserRepository.GetManyDetailsByOrganizationAsync(organizationId)) + .Where(ou => ou.Status != OrganizationUserStatusType.Invited && + ou.Status != OrganizationUserStatusType.Revoked && + ou.Type != OrganizationUserType.Owner && + ou.Type != OrganizationUserType.Admin && + performedBy is StandardUser stdUser && + stdUser.UserId != ou.UserId) + .ToList(); + + if (currentActiveRevocableOrganizationUsers.Count == 0) + { + return; + } + + var commandResult = await _revokeNonCompliantOrganizationUserCommand.RevokeNonCompliantOrganizationUsersAsync( + new RevokeOrganizationUsersRequest(organizationId, currentActiveRevocableOrganizationUsers, performedBy)); + + if (commandResult.HasErrors) + { + throw new BadRequestException(string.Join(", ", commandResult.ErrorMessages)); + } + + await Task.WhenAll(currentActiveRevocableOrganizationUsers.Select(x => + _mailService.SendOrganizationUserRevokedForPolicySingleOrgEmailAsync(organization.DisplayName(), x.Email))); + } + private async Task RemoveNonCompliantUsersAsync(Guid organizationId) { // Remove non-compliant users @@ -72,7 +119,7 @@ private async Task RemoveNonCompliantUsersAsync(Guid organizationId) var org = await _organizationRepository.GetByIdAsync(organizationId); if (org == null) { - throw new NotFoundException("Organization not found."); + throw new NotFoundException(OrganizationNotFoundErrorMessage); } var removableOrgUsers = orgUsers.Where(ou => @@ -86,41 +133,16 @@ private async Task RemoveNonCompliantUsersAsync(Guid organizationId) var userOrgs = await _organizationUserRepository.GetManyByManyUsersAsync( removableOrgUsers.Select(ou => ou.UserId!.Value)); - if (_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning)) + foreach (var orgUser in removableOrgUsers) { - var isOwner = await _currentContext.OrganizationOwner(organizationId); - - var revocableUsers = removableOrgUsers.Where(nonCompliantUser => orgUsers.Any(orgUser => - nonCompliantUser.UserId == orgUser.UserId - && nonCompliantUser.OrganizationId != org.Id - && nonCompliantUser.Status != OrganizationUserStatusType.Invited)) - .ToList(); - - var commandResult = await _revokeNonCompliantOrganizationUserCommand.RevokeNonCompliantOrganizationUsersAsync( - new RevokeOrganizationUsersRequest(organizationId, revocableUsers, new StandardUser(savingUserId ?? Guid.Empty, isOwner))); - - if (commandResult.HasErrors) + if (userOrgs.Any(ou => ou.UserId == orgUser.UserId + && ou.OrganizationId != org.Id + && ou.Status != OrganizationUserStatusType.Invited)) { - throw new BadRequestException(string.Join(", ", commandResult.ErrorMessages)); - } + await _removeOrganizationUserCommand.RemoveUserAsync(organizationId, orgUser.Id, savingUserId); - await Task.WhenAll(revocableUsers.Select(x => - _mailService.SendOrganizationUserRevokedForPolicySingleOrgEmailAsync(org.DisplayName(), x.Email))); - } - else - { - foreach (var orgUser in removableOrgUsers) - { - if (userOrgs.Any(ou => ou.UserId == orgUser.UserId - && ou.OrganizationId != org.Id - && ou.Status != OrganizationUserStatusType.Invited)) - { - await _removeOrganizationUserCommand.RemoveUserAsync(organizationId, orgUser.Id, - savingUserId); - - await _mailService.SendOrganizationUserRemovedForPolicySingleOrgEmailAsync( - org.DisplayName(), orgUser.Email); - } + await _mailService.SendOrganizationUserRemovedForPolicySingleOrgEmailAsync( + org.DisplayName(), orgUser.Email); } } } @@ -141,7 +163,7 @@ public async Task ValidateAsync(PolicyUpdate policyUpdate, Policy? curre if (_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning) && await _organizationHasVerifiedDomainsQuery.HasVerifiedDomainsAsync(policyUpdate.OrganizationId)) { - return "The Single organization policy is required for organizations that have enabled domain verification."; + return ClaimedDomainSingleOrganizationRequiredErrorMessage; } } diff --git a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs index e1668e851817..f8e44050d167 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs @@ -56,10 +56,58 @@ public async Task OnSaveSideEffectsAsync(PolicyUpdate policyUpdate, Policy? curr { if (currentPolicy is not { Enabled: true } && policyUpdate is { Enabled: true }) { - await RemoveNonCompliantUsersAsync(policyUpdate.OrganizationId); + if (_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning)) + { + var currentUser = _currentContext.UserId ?? Guid.Empty; + var isOwnerOrProvider = await _currentContext.OrganizationOwner(policyUpdate.OrganizationId); + await RevokeNonCompliantUsersAsync(policyUpdate.OrganizationId, policyUpdate.PerformedBy ?? new StandardUser(currentUser, isOwnerOrProvider)); + } + else + { + await RemoveNonCompliantUsersAsync(policyUpdate.OrganizationId); + } } } + private async Task RevokeNonCompliantUsersAsync(Guid organizationId, IActingUser performedBy) + { + var organization = await _organizationRepository.GetByIdAsync(organizationId); + + var currentActiveRevocableOrganizationUsers = + (await _organizationUserRepository.GetManyDetailsByOrganizationAsync(organizationId)) + .Where(ou => ou.Status != OrganizationUserStatusType.Invited && + ou.Status != OrganizationUserStatusType.Revoked && + ou.Type != OrganizationUserType.Owner && + ou.Type != OrganizationUserType.Admin && + performedBy is StandardUser stdUser && + stdUser.UserId != ou.UserId) + .ToList(); + + if (currentActiveRevocableOrganizationUsers.Count == 0) + { + return; + } + + var organizationUsersTwoFactorEnabled = + await _twoFactorIsEnabledQuery.TwoFactorIsEnabledAsync(currentActiveRevocableOrganizationUsers); + + if (DoMembersNotHaveAPasswordWithTwoFactorAuthenticationEnabled(currentActiveRevocableOrganizationUsers, organizationUsersTwoFactorEnabled)) + { + throw new BadRequestException(NonCompliantMembersWillLoseAccess); + } + + var commandResult = await _revokeNonCompliantOrganizationUserCommand.RevokeNonCompliantOrganizationUsersAsync( + new RevokeOrganizationUsersRequest(organizationId, currentActiveRevocableOrganizationUsers, performedBy)); + + if (commandResult.HasErrors) + { + throw new BadRequestException(string.Join(", ", commandResult.ErrorMessages)); + } + + await Task.WhenAll(currentActiveRevocableOrganizationUsers.Select(x => + _mailService.SendOrganizationUserRevokedForPolicySingleOrgEmailAsync(organization.DisplayName(), x.Email))); + } + private async Task RemoveNonCompliantUsersAsync(Guid organizationId) { var org = await _organizationRepository.GetByIdAsync(organizationId); @@ -68,7 +116,8 @@ private async Task RemoveNonCompliantUsersAsync(Guid organizationId) var orgUsers = await _organizationUserRepository.GetManyDetailsByOrganizationAsync(organizationId); // this can get moved into the private method after AccountDeprovisiong flag check is removed. - var organizationUsersTwoFactorEnabled = (await _twoFactorIsEnabledQuery.TwoFactorIsEnabledAsync(orgUsers)).ToList(); + var organizationUsersTwoFactorEnabled = + (await _twoFactorIsEnabledQuery.TwoFactorIsEnabledAsync(orgUsers)).ToList(); var revocableOrgUsers = orgUsers.Where(ou => ou.Status != OrganizationUserStatusType.Invited && ou.Status != OrganizationUserStatusType.Revoked && @@ -76,46 +125,23 @@ private async Task RemoveNonCompliantUsersAsync(Guid organizationId) ou.UserId != savingUserId) .ToList(); - if (_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning)) + // Reorder by HasMasterPassword to prioritize checking users without a master if they have 2FA enabled + foreach (var orgUser in revocableOrgUsers.OrderBy(ou => ou.HasMasterPassword)) { - if (DoMembersNotHaveAPasswordWithTwoFactorAuthenticationEnabled(revocableOrgUsers, organizationUsersTwoFactorEnabled)) - { - throw new BadRequestException(NonCompliantMembersWillLoseAccess); - } - - var result = await _revokeNonCompliantOrganizationUserCommand.RevokeNonCompliantOrganizationUsersAsync( - new RevokeOrganizationUsersRequest(organizationId, revocableOrgUsers, - new StandardUser(savingUserId ?? Guid.Empty, - await _currentContext.OrganizationOwner(organizationId)))); - - if (result.HasErrors) + var userTwoFactorEnabled = organizationUsersTwoFactorEnabled + .FirstOrDefault(u => u.user.Id == orgUser.Id).twoFactorIsEnabled; + if (!userTwoFactorEnabled) { - throw new BadRequestException(string.Join(", ", result.ErrorMessages)); - } - - await Task.WhenAll(revocableOrgUsers.Select(x => - _mailService.SendOrganizationUserRevokedForTwoFactoryPolicyEmailAsync(org!.DisplayName(), x.Email))); - } - else - { - // Reorder by HasMasterPassword to prioritize checking users without a master if they have 2FA enabled - foreach (var orgUser in revocableOrgUsers.OrderBy(ou => ou.HasMasterPassword)) - { - var userTwoFactorEnabled = organizationUsersTwoFactorEnabled - .FirstOrDefault(u => u.user.Id == orgUser.Id).twoFactorIsEnabled; - if (!userTwoFactorEnabled) + if (!orgUser.HasMasterPassword) { - if (!orgUser.HasMasterPassword) - { - throw new BadRequestException(NonCompliantMembersWillLoseAccess); - } + throw new BadRequestException(NonCompliantMembersWillLoseAccess); + } - await _removeOrganizationUserCommand.RemoveUserAsync(organizationId, orgUser.Id, - savingUserId); + await _removeOrganizationUserCommand.RemoveUserAsync(organizationId, orgUser.Id, + savingUserId); - await _mailService.SendOrganizationUserRemovedForPolicyTwoStepEmailAsync( - org!.DisplayName(), orgUser.Email); - } + await _mailService.SendOrganizationUserRemovedForPolicyTwoStepEmailAsync( + org!.DisplayName(), orgUser.Email); } } } diff --git a/src/Core/AdminConsole/Repositories/IOrganizationUserRepository.cs b/src/Core/AdminConsole/Repositories/IOrganizationUserRepository.cs index f8007b648eba..83dbae79dd76 100644 --- a/src/Core/AdminConsole/Repositories/IOrganizationUserRepository.cs +++ b/src/Core/AdminConsole/Repositories/IOrganizationUserRepository.cs @@ -59,5 +59,5 @@ UpdateEncryptedDataForKeyRotation UpdateForKeyRotation(Guid userId, /// Task> GetManyByOrganizationWithClaimedDomainsAsync(Guid organizationId); - Task SetOrganizationUsersStatusAsync(IEnumerable userIds, OrganizationUserStatusType status); + Task RevokeOrganizationUserAsync(IEnumerable userIds); } diff --git a/src/Core/Models/Commands/CommandResult.cs b/src/Core/Models/Commands/CommandResult.cs index 23e6cb529f90..4ac2d6249918 100644 --- a/src/Core/Models/Commands/CommandResult.cs +++ b/src/Core/Models/Commands/CommandResult.cs @@ -2,6 +2,8 @@ public class CommandResult(IEnumerable errors) { + public CommandResult(string error) : this([error]) { } + public bool Success => ErrorMessages.Count == 0; public bool HasErrors => ErrorMessages.Count > 0; public List ErrorMessages { get; } = errors.ToList(); diff --git a/src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationUserRepository.cs b/src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationUserRepository.cs index e9722c102a60..9084fa67d985 100644 --- a/src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationUserRepository.cs +++ b/src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationUserRepository.cs @@ -558,13 +558,13 @@ public async Task> GetManyByOrganizationWithClaime } } - public async Task SetOrganizationUsersStatusAsync(IEnumerable organizationUserIds, OrganizationUserStatusType status) + public async Task RevokeOrganizationUserAsync(IEnumerable organizationUserIds) { await using var connection = new SqlConnection(ConnectionString); await connection.ExecuteAsync( "[dbo].[OrganizationUser_SetStatusForUsersById]", - new { OrganizationUserIds = organizationUserIds.ToGuidIdArrayTVP(), Status = status }, + new { OrganizationUserIds = JsonSerializer.Serialize(organizationUserIds), Status = OrganizationUserStatusType.Revoked }, commandType: CommandType.StoredProcedure); } } diff --git a/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs b/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs index 7e13b56e4e23..cd7d12dcf467 100644 --- a/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs +++ b/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs @@ -722,13 +722,15 @@ public UpdateEncryptedDataForKeyRotation UpdateForKeyRotation( } } - public async Task SetOrganizationUsersStatusAsync(IEnumerable userIds, OrganizationUserStatusType status) + public async Task RevokeOrganizationUserAsync(IEnumerable userIds) { using var scope = ServiceScopeFactory.CreateScope(); var dbContext = GetDatabaseContext(scope); await dbContext.OrganizationUsers.Where(x => userIds.Contains(x.OrganizationId)) - .ExecuteUpdateAsync(s => s.SetProperty(x => x.Status, status)); + .ExecuteUpdateAsync(s => s.SetProperty(x => x.Status, OrganizationUserStatusType.Revoked)); + + await dbContext.UserBumpAccountRevisionDateByOrganizationUserIdsAsync(userIds); } } diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs index d14af40376de..8ac9b1224980 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs @@ -161,7 +161,7 @@ public async Task RevokeNonCompliantOrganizationUsersAsync_GivenValidPopulatedRe await sutProvider.GetDependency() .Received(1) - .SetOrganizationUsersStatusAsync(Arg.Any>(), OrganizationUserStatusType.Revoked); + .RevokeOrganizationUserAsync(Arg.Any>()); Assert.True(result.Success); } @@ -169,7 +169,7 @@ await sutProvider.GetDependency() public class InvalidUser : IActingUser { public Guid? UserId => Guid.Empty; - public bool IsOrganizationOwner => false; + public bool IsOrganizationOwnerOrProvider => false; public EventSystemUser? SystemUserType => null; } } From d4bd369918141c62f60b8defc49ec855b62a118a Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Tue, 19 Nov 2024 14:24:36 -0600 Subject: [PATCH 29/49] Removing todos --- .../Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs index bae8a5c60401..d5e09694b18f 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs @@ -192,7 +192,4 @@ await sutProvider.GetDependency() .DidNotReceive() .RevokeNonCompliantOrganizationUsersAsync(Arg.Any()); } - // TODO feature flag is enabled and revoke returns errors, we throw - // TODO feature flag is enabled and revoke returns no errors, no throw - // TODO feature flag is disabled we don't call command } From 1d773456f10a58b3a5a47545071fb633de57d95d Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Tue, 19 Nov 2024 14:26:43 -0600 Subject: [PATCH 30/49] Corrected test name. --- .../TwoFactorAuthenticationPolicyValidatorTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs index c99b3ece3b65..f0dad978b102 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs @@ -262,7 +262,7 @@ await sutProvider.GetDependency() } [Theory, BitAutoData] - public async Task OnSaveSideEffectsAsync_GivenUpdateTo2faPolicy_WhenAccountProvisioningIsEnabled_ThenRevokeUserCommandShouldNotBeCalled( + public async Task OnSaveSideEffectsAsync_GivenUpdateTo2faPolicy_WhenAccountProvisioningIsEnabledAndUserDoesNotHaveMasterPassword_ThenNonCompliantMembersErrorMessageWillReturn( Organization organization, [PolicyUpdate(PolicyType.TwoFactorAuthentication)] PolicyUpdate policyUpdate, [Policy(PolicyType.TwoFactorAuthentication, false)] Policy policy, From 37ca7ff6f8a182fb0a30a0991d7d6d5bcb66f8df Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Wed, 20 Nov 2024 08:36:33 -0600 Subject: [PATCH 31/49] Syncing filename to record name. --- ...RevokeOrganizationUser.cs => RevokeOrganizationUserRequest.cs} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Requests/{RevokeOrganizationUser.cs => RevokeOrganizationUserRequest.cs} (100%) diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Requests/RevokeOrganizationUser.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Requests/RevokeOrganizationUserRequest.cs similarity index 100% rename from src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Requests/RevokeOrganizationUser.cs rename to src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Requests/RevokeOrganizationUserRequest.cs From d0dd776157961a882e181b8bdae57f28cbaf734d Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Wed, 20 Nov 2024 11:49:24 -0600 Subject: [PATCH 32/49] Fixing up the tests. --- .../SingleOrgPolicyValidator.cs | 5 +- .../SingleOrgPolicyValidatorTests.cs | 51 +++++++++++++++---- 2 files changed, 42 insertions(+), 14 deletions(-) diff --git a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs index 683171012ce1..249b71b721b3 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs @@ -128,11 +128,10 @@ private async Task RemoveNonCompliantUsersAsync(Guid organizationId) ou.Type != OrganizationUserType.Owner && ou.Type != OrganizationUserType.Admin && ou.UserId != savingUserId - ).ToList(); + ).ToList(); var userOrgs = await _organizationUserRepository.GetManyByManyUsersAsync( - removableOrgUsers.Select(ou => ou.UserId!.Value)); - + removableOrgUsers.Select(ou => ou.UserId!.Value)); foreach (var orgUser in removableOrgUsers) { if (userOrgs.Any(ou => ou.UserId == orgUser.UserId diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs index d5e09694b18f..83d10a7ec980 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs @@ -11,6 +11,7 @@ using Bit.Core.Context; using Bit.Core.Entities; using Bit.Core.Enums; +using Bit.Core.Models.Commands; using Bit.Core.Models.Data.Organizations.OrganizationUsers; using Bit.Core.Repositories; using Bit.Core.Services; @@ -62,8 +63,11 @@ public async Task ValidateAsync_DisablingPolicy_KeyConnectorNotEnabled_Success( Assert.True(string.IsNullOrEmpty(result)); } - [Theory, BitAutoData] + [Theory] + [BitAutoData(true)] + [BitAutoData(false)] public async Task OnSaveSideEffectsAsync_RemovesNonCompliantUsers( + bool isAccountDeprovisioningEnabled, [PolicyUpdate(PolicyType.SingleOrg)] PolicyUpdate policyUpdate, [Policy(PolicyType.SingleOrg, false)] Policy policy, Guid savingUserId, @@ -117,19 +121,40 @@ public async Task OnSaveSideEffectsAsync_RemovesNonCompliantUsers( sutProvider.GetDependency().UserId.Returns(savingUserId); sutProvider.GetDependency().GetByIdAsync(policyUpdate.OrganizationId).Returns(organization); + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.AccountDeprovisioning) + .Returns(isAccountDeprovisioningEnabled); + + sutProvider.GetDependency() + .RevokeNonCompliantOrganizationUsersAsync(Arg.Any()) + .Returns(new CommandResult()); + await sutProvider.Sut.OnSaveSideEffectsAsync(policyUpdate, policy); - await sutProvider.GetDependency() - .Received(1) - .RemoveUserAsync(policyUpdate.OrganizationId, nonCompliantUser.Id, savingUserId); - await sutProvider.GetDependency() - .Received(1) - .SendOrganizationUserRemovedForPolicySingleOrgEmailAsync(organization.DisplayName(), - "user3@example.com"); + if (isAccountDeprovisioningEnabled) + { + await sutProvider.GetDependency() + .Received(1) + .RevokeNonCompliantOrganizationUsersAsync(Arg.Any()); + await sutProvider.GetDependency() + .Received(1) + .SendOrganizationUserRevokedForPolicySingleOrgEmailAsync(organization.DisplayName(), + "user3@example.com"); + } + else + { + await sutProvider.GetDependency() + .Received(1) + .RemoveUserAsync(policyUpdate.OrganizationId, nonCompliantUser.Id, savingUserId); + await sutProvider.GetDependency() + .Received(1) + .SendOrganizationUserRemovedForPolicySingleOrgEmailAsync(organization.DisplayName(), + "user3@example.com"); + } + } [Theory, BitAutoData] - public async Task OnSaveSideEffectsAsync_GivenUpdatedSingleOrgPolicy_WhenAccountDeprovisioningIsDisabled_Then( + public async Task OnSaveSideEffectsAsync_WhenAccountDeprovisioningIsEnabled_ThenUsersAreRevoked( [PolicyUpdate(PolicyType.SingleOrg)] PolicyUpdate policyUpdate, [Policy(PolicyType.SingleOrg, false)] Policy policy, Guid savingUserId, @@ -184,12 +209,16 @@ public async Task OnSaveSideEffectsAsync_GivenUpdatedSingleOrgPolicy_WhenAccount sutProvider.GetDependency().GetByIdAsync(policyUpdate.OrganizationId) .Returns(organization); - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.AccountDeprovisioning).Returns(false); + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.AccountDeprovisioning).Returns(true); + + sutProvider.GetDependency() + .RevokeNonCompliantOrganizationUsersAsync(Arg.Any()) + .Returns(new CommandResult()); await sutProvider.Sut.OnSaveSideEffectsAsync(policyUpdate, policy); await sutProvider.GetDependency() - .DidNotReceive() + .Received() .RevokeNonCompliantOrganizationUsersAsync(Arg.Any()); } } From 273754adc663b76f420b8fd7c496998eadbcb067 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Wed, 20 Nov 2024 12:07:17 -0600 Subject: [PATCH 33/49] Added happy path test --- ...actorAuthenticationPolicyValidatorTests.cs | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs index f0dad978b102..ae6b3f77ffd1 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs @@ -8,6 +8,7 @@ using Bit.Core.Context; using Bit.Core.Enums; using Bit.Core.Exceptions; +using Bit.Core.Models.Commands; using Bit.Core.Models.Data.Organizations.OrganizationUsers; using Bit.Core.Repositories; using Bit.Core.Services; @@ -301,4 +302,56 @@ public async Task OnSaveSideEffectsAsync_GivenUpdateTo2faPolicy_WhenAccountProvi Assert.Equal(TwoFactorAuthenticationPolicyValidator.NonCompliantMembersWillLoseAccess, exception.Message); } + + [Theory, BitAutoData] + public async Task OnSaveSideEffectsAsync_WhenAccountProvisioningIsEnabledAndUserHasMasterPassword_ThenUserWillBeRevoked( + Organization organization, + [PolicyUpdate(PolicyType.TwoFactorAuthentication)] PolicyUpdate policyUpdate, + [Policy(PolicyType.TwoFactorAuthentication, false)] Policy policy, + SutProvider sutProvider) + { + policy.OrganizationId = organization.Id = policyUpdate.OrganizationId; + sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); + + sutProvider.GetDependency() + .IsEnabled(FeatureFlagKeys.AccountDeprovisioning) + .Returns(true); + + var orgUserDetailUserWithout2Fa = new OrganizationUserUserDetails + { + Id = Guid.NewGuid(), + Status = OrganizationUserStatusType.Confirmed, + Type = OrganizationUserType.User, + Email = "user3@test.com", + Name = "TEST", + UserId = Guid.NewGuid(), + HasMasterPassword = true + }; + + sutProvider.GetDependency() + .GetManyDetailsByOrganizationAsync(policyUpdate.OrganizationId) + .Returns([orgUserDetailUserWithout2Fa]); + + sutProvider.GetDependency() + .TwoFactorIsEnabledAsync(Arg.Any>()) + .Returns(new List<(OrganizationUserUserDetails user, bool hasTwoFactor)>() + { + (orgUserDetailUserWithout2Fa, true), + }); + + sutProvider.GetDependency() + .RevokeNonCompliantOrganizationUsersAsync(Arg.Any()) + .Returns(new CommandResult()); + + await sutProvider.Sut.OnSaveSideEffectsAsync(policyUpdate, policy); + + await sutProvider.GetDependency() + .Received(1) + .RevokeNonCompliantOrganizationUsersAsync(Arg.Any()); + + await sutProvider.GetDependency() + .Received(1) + .SendOrganizationUserRevokedForPolicySingleOrgEmailAsync(organization.DisplayName(), + "user3@test.com"); + } } From 2f3da19b64a43c4a54d388333371f702e3362777 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Wed, 20 Nov 2024 13:00:15 -0600 Subject: [PATCH 34/49] Naming corrections. And corrected EF query. --- .../RevokeNonCompliantOrganizationUserCommand.cs | 2 +- .../Repositories/IOrganizationUserRepository.cs | 2 +- .../AdminConsole/Repositories/OrganizationUserRepository.cs | 2 +- .../AdminConsole/Repositories/OrganizationUserRepository.cs | 6 +++--- .../RevokeNonCompliantOrganizationUserCommandTests.cs | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs index dec44e7c17d4..971ed02b29e9 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs @@ -30,7 +30,7 @@ public async Task RevokeNonCompliantOrganizationUsersAsync(Revoke return validationResult; } - await organizationUserRepository.RevokeOrganizationUserAsync(request.OrganizationUsers.Select(x => x.Id)); + await organizationUserRepository.RevokeManyByIdAsync(request.OrganizationUsers.Select(x => x.Id)); var now = timeProvider.GetUtcNow(); diff --git a/src/Core/AdminConsole/Repositories/IOrganizationUserRepository.cs b/src/Core/AdminConsole/Repositories/IOrganizationUserRepository.cs index 83dbae79dd76..488d037733e3 100644 --- a/src/Core/AdminConsole/Repositories/IOrganizationUserRepository.cs +++ b/src/Core/AdminConsole/Repositories/IOrganizationUserRepository.cs @@ -59,5 +59,5 @@ UpdateEncryptedDataForKeyRotation UpdateForKeyRotation(Guid userId, /// Task> GetManyByOrganizationWithClaimedDomainsAsync(Guid organizationId); - Task RevokeOrganizationUserAsync(IEnumerable userIds); + Task RevokeManyByIdAsync(IEnumerable organizationUserIds); } diff --git a/src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationUserRepository.cs b/src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationUserRepository.cs index 9084fa67d985..4a205a73a8a4 100644 --- a/src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationUserRepository.cs +++ b/src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationUserRepository.cs @@ -558,7 +558,7 @@ public async Task> GetManyByOrganizationWithClaime } } - public async Task RevokeOrganizationUserAsync(IEnumerable organizationUserIds) + public async Task RevokeManyByIdAsync(IEnumerable organizationUserIds) { await using var connection = new SqlConnection(ConnectionString); diff --git a/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs b/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs index cd7d12dcf467..7c56ef799b47 100644 --- a/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs +++ b/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs @@ -722,15 +722,15 @@ public UpdateEncryptedDataForKeyRotation UpdateForKeyRotation( } } - public async Task RevokeOrganizationUserAsync(IEnumerable userIds) + public async Task RevokeManyByIdAsync(IEnumerable organizationUserIds) { using var scope = ServiceScopeFactory.CreateScope(); var dbContext = GetDatabaseContext(scope); - await dbContext.OrganizationUsers.Where(x => userIds.Contains(x.OrganizationId)) + await dbContext.OrganizationUsers.Where(x => organizationUserIds.Contains(x.Id)) .ExecuteUpdateAsync(s => s.SetProperty(x => x.Status, OrganizationUserStatusType.Revoked)); - await dbContext.UserBumpAccountRevisionDateByOrganizationUserIdsAsync(userIds); + await dbContext.UserBumpAccountRevisionDateByOrganizationUserIdsAsync(organizationUserIds); } } diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs index 8ac9b1224980..85955fd09de7 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs @@ -161,7 +161,7 @@ public async Task RevokeNonCompliantOrganizationUsersAsync_GivenValidPopulatedRe await sutProvider.GetDependency() .Received(1) - .RevokeOrganizationUserAsync(Arg.Any>()); + .RevokeManyByIdAsync(Arg.Any>()); Assert.True(result.Success); } From 1e7c17fdcdc7c66204ac9b5ceb8189f075a34a56 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Wed, 20 Nov 2024 15:18:33 -0600 Subject: [PATCH 35/49] added check against event service --- .../RevokeNonCompliantOrganizationUserCommandTests.cs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs index 85955fd09de7..3653cd27d7df 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs @@ -5,6 +5,7 @@ using Bit.Core.Enums; using Bit.Core.Models.Data.Organizations.OrganizationUsers; using Bit.Core.Repositories; +using Bit.Core.Services; using Bit.Test.Common.AutoFixture; using Bit.Test.Common.AutoFixture.Attributes; using NSubstitute; @@ -164,6 +165,15 @@ await sutProvider.GetDependency() .RevokeManyByIdAsync(Arg.Any>()); Assert.True(result.Success); + + await sutProvider.GetDependency() + .Received(1) + .LogOrganizationUserEventsAsync( + Arg.Is>( + x => x.Any(y => + y.organizationUser.Id == userToRevoke.Id && y.eventType == EventType.OrganizationUser_Revoked) + )); } public class InvalidUser : IActingUser From 772b0577bb4fe3b67d77f05f55500e80fb45c9fd Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Fri, 22 Nov 2024 15:55:52 -0600 Subject: [PATCH 36/49] Code review changes. --- .../VerifyOrganizationDomainCommand.cs | 9 ++++- .../SingleOrgPolicyValidator.cs | 3 +- .../TwoFactorAuthenticationPolicyValidator.cs | 35 ++++++++----------- ...actorAuthenticationPolicyValidatorTests.cs | 4 +-- 4 files changed, 26 insertions(+), 25 deletions(-) diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs index f44d1251cb6b..dd6add669f2d 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs @@ -29,7 +29,14 @@ public class VerifyOrganizationDomainCommand( public async Task UserVerifyOrganizationDomainAsync(OrganizationDomain organizationDomain) { - var actingUser = new StandardUser(currentContext.UserId ?? Guid.Empty, await currentContext.OrganizationOwner(organizationDomain.OrganizationId)); + if (currentContext.UserId is null) + { + throw new InvalidOperationException( + $"{nameof(UserVerifyOrganizationDomainAsync)} can only be called by a user. " + + $"Please call {nameof(SystemVerifyOrganizationDomainAsync)} for system users."); + } + + var actingUser = new StandardUser(currentContext.UserId.Value, await currentContext.OrganizationOwner(organizationDomain.OrganizationId)); var domainVerificationResult = await VerifyOrganizationDomainAsync(organizationDomain, actingUser); diff --git a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs index 249b71b721b3..050949ee7f91 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs @@ -89,8 +89,7 @@ private async Task RevokeNonCompliantUsersAsync(Guid organizationId, IActingUser ou.Status != OrganizationUserStatusType.Revoked && ou.Type != OrganizationUserType.Owner && ou.Type != OrganizationUserType.Admin && - performedBy is StandardUser stdUser && - stdUser.UserId != ou.UserId) + !(performedBy is StandardUser stdUser && stdUser.UserId == ou.UserId)) .ToList(); if (currentActiveRevocableOrganizationUsers.Count == 0) diff --git a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs index f8e44050d167..c2dd8cff91c8 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs @@ -27,7 +27,7 @@ public class TwoFactorAuthenticationPolicyValidator : IPolicyValidator private readonly IFeatureService _featureService; private readonly IRevokeNonCompliantOrganizationUserCommand _revokeNonCompliantOrganizationUserCommand; - public const string NonCompliantMembersWillLoseAccess = "Policy could not be enabled. Non-compliant members will lose access to their accounts. Identify members without two-step login from the policies column in the members page."; + public const string NonCompliantMembersWillLoseAccessMessage = "Policy could not be enabled. Non-compliant members will lose access to their accounts. Identify members without two-step login from the policies column in the members page."; public PolicyType Type => PolicyType.TwoFactorAuthentication; public IEnumerable RequiredPolicies => []; @@ -79,8 +79,7 @@ private async Task RevokeNonCompliantUsersAsync(Guid organizationId, IActingUser ou.Status != OrganizationUserStatusType.Revoked && ou.Type != OrganizationUserType.Owner && ou.Type != OrganizationUserType.Admin && - performedBy is StandardUser stdUser && - stdUser.UserId != ou.UserId) + !(performedBy is StandardUser stdUser && stdUser.UserId == ou.UserId)) .ToList(); if (currentActiveRevocableOrganizationUsers.Count == 0) @@ -91,9 +90,9 @@ performedBy is StandardUser stdUser && var organizationUsersTwoFactorEnabled = await _twoFactorIsEnabledQuery.TwoFactorIsEnabledAsync(currentActiveRevocableOrganizationUsers); - if (DoMembersNotHaveAPasswordWithTwoFactorAuthenticationEnabled(currentActiveRevocableOrganizationUsers, organizationUsersTwoFactorEnabled)) + if (NonCompliantMembersWillLoseAccess(currentActiveRevocableOrganizationUsers, organizationUsersTwoFactorEnabled)) { - throw new BadRequestException(NonCompliantMembersWillLoseAccess); + throw new BadRequestException(NonCompliantMembersWillLoseAccessMessage); } var commandResult = await _revokeNonCompliantOrganizationUserCommand.RevokeNonCompliantOrganizationUsersAsync( @@ -114,27 +113,23 @@ private async Task RemoveNonCompliantUsersAsync(Guid organizationId) var savingUserId = _currentContext.UserId; var orgUsers = await _organizationUserRepository.GetManyDetailsByOrganizationAsync(organizationId); - - // this can get moved into the private method after AccountDeprovisiong flag check is removed. - var organizationUsersTwoFactorEnabled = - (await _twoFactorIsEnabledQuery.TwoFactorIsEnabledAsync(orgUsers)).ToList(); - - var revocableOrgUsers = orgUsers.Where(ou => - ou.Status != OrganizationUserStatusType.Invited && ou.Status != OrganizationUserStatusType.Revoked && - ou.Type != OrganizationUserType.Owner && ou.Type != OrganizationUserType.Admin && - ou.UserId != savingUserId) - .ToList(); + var organizationUsersTwoFactorEnabled = await _twoFactorIsEnabledQuery.TwoFactorIsEnabledAsync(orgUsers); + var removableOrgUsers = orgUsers.Where(ou => + ou.Status != OrganizationUserStatusType.Invited && ou.Status != OrganizationUserStatusType.Revoked && + ou.Type != OrganizationUserType.Owner && ou.Type != OrganizationUserType.Admin && + ou.UserId != savingUserId); // Reorder by HasMasterPassword to prioritize checking users without a master if they have 2FA enabled - foreach (var orgUser in revocableOrgUsers.OrderBy(ou => ou.HasMasterPassword)) + foreach (var orgUser in removableOrgUsers.OrderBy(ou => ou.HasMasterPassword)) { - var userTwoFactorEnabled = organizationUsersTwoFactorEnabled - .FirstOrDefault(u => u.user.Id == orgUser.Id).twoFactorIsEnabled; + var userTwoFactorEnabled = organizationUsersTwoFactorEnabled.FirstOrDefault(u => u.user.Id == orgUser.Id) + .twoFactorIsEnabled; if (!userTwoFactorEnabled) { if (!orgUser.HasMasterPassword) { - throw new BadRequestException(NonCompliantMembersWillLoseAccess); + throw new BadRequestException( + "Policy could not be enabled. Non-compliant members will lose access to their accounts. Identify members without two-step login from the policies column in the members page."); } await _removeOrganizationUserCommand.RemoveUserAsync(organizationId, orgUser.Id, @@ -146,7 +141,7 @@ await _mailService.SendOrganizationUserRemovedForPolicyTwoStepEmailAsync( } } - private static bool DoMembersNotHaveAPasswordWithTwoFactorAuthenticationEnabled( + private static bool NonCompliantMembersWillLoseAccess( IEnumerable orgUserDetails, IEnumerable<(OrganizationUserUserDetails user, bool isTwoFactorEnabled)> organizationUsersTwoFactorEnabled) => orgUserDetails.Any(x => diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs index ae6b3f77ffd1..4e5f1816a5a4 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs @@ -207,7 +207,7 @@ public async Task OnSaveSideEffectsAsync_UsersToBeRemovedDontHaveMasterPasswords var badRequestException = await Assert.ThrowsAsync( () => sutProvider.Sut.OnSaveSideEffectsAsync(policyUpdate, policy)); - Assert.Equal(TwoFactorAuthenticationPolicyValidator.NonCompliantMembersWillLoseAccess, badRequestException.Message); + Assert.Equal(TwoFactorAuthenticationPolicyValidator.NonCompliantMembersWillLoseAccessMessage, badRequestException.Message); await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() .RemoveUserAsync(organizationId: default, organizationUserId: default, deletingUserId: default); @@ -300,7 +300,7 @@ public async Task OnSaveSideEffectsAsync_GivenUpdateTo2faPolicy_WhenAccountProvi var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.OnSaveSideEffectsAsync(policyUpdate, policy)); - Assert.Equal(TwoFactorAuthenticationPolicyValidator.NonCompliantMembersWillLoseAccess, exception.Message); + Assert.Equal(TwoFactorAuthenticationPolicyValidator.NonCompliantMembersWillLoseAccessMessage, exception.Message); } [Theory, BitAutoData] From 8669e90e183120ca2bf725a7b05cf7977cdfff88 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Fri, 22 Nov 2024 16:02:11 -0600 Subject: [PATCH 37/49] Fixing tests. --- .../VerifyOrganizationDomainCommandTests.cs | 30 ++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommandTests.cs index e2a12c675e81..8dbe5331317c 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommandTests.cs @@ -29,7 +29,12 @@ public async Task UserVerifyOrganizationDomainAsync_ShouldThrowConflict_WhenDoma DomainName = "Test Domain", Txt = "btw+test18383838383" }; + + sutProvider.GetDependency() + .UserId.Returns(Guid.NewGuid()); + expected.SetVerifiedDate(); + sutProvider.GetDependency() .GetByIdAsync(id) .Returns(expected); @@ -54,6 +59,10 @@ public async Task UserVerifyOrganizationDomainAsync_ShouldThrowConflict_WhenDoma sutProvider.GetDependency() .GetByIdAsync(id) .Returns(expected); + + sutProvider.GetDependency() + .UserId.Returns(Guid.NewGuid()); + sutProvider.GetDependency() .GetClaimedDomainsByDomainNameAsync(expected.DomainName) .Returns(new List { expected }); @@ -78,9 +87,14 @@ public async Task UserVerifyOrganizationDomainAsync_ShouldVerifyDomainUpdateAndL sutProvider.GetDependency() .GetByIdAsync(id) .Returns(expected); + + sutProvider.GetDependency() + .UserId.Returns(Guid.NewGuid()); + sutProvider.GetDependency() .GetClaimedDomainsByDomainNameAsync(expected.DomainName) .Returns(new List()); + sutProvider.GetDependency() .ResolveAsync(expected.DomainName, Arg.Any()) .Returns(true); @@ -108,9 +122,14 @@ public async Task UserVerifyOrganizationDomainAsync_ShouldNotSetVerifiedDate_Whe sutProvider.GetDependency() .GetByIdAsync(id) .Returns(expected); + + sutProvider.GetDependency() + .UserId.Returns(Guid.NewGuid()); + sutProvider.GetDependency() .GetClaimedDomainsByDomainNameAsync(expected.DomainName) .Returns(new List()); + sutProvider.GetDependency() .ResolveAsync(expected.DomainName, Arg.Any()) .Returns(false); @@ -180,6 +199,9 @@ public async Task UserVerifyOrganizationDomainAsync_GivenOrganizationDomainWithA .ResolveAsync(domain.DomainName, domain.Txt) .Returns(true); + sutProvider.GetDependency() + .UserId.Returns(Guid.NewGuid()); + sutProvider.GetDependency() .IsEnabled(FeatureFlagKeys.AccountDeprovisioning) .Returns(false); @@ -193,7 +215,7 @@ await sutProvider.GetDependency() [Theory, BitAutoData] public async Task UserVerifyOrganizationDomainAsync_GivenOrganizationDomainWithAccountDeprovisioningEnabled_WhenDomainIsNotVerified_ThenSingleOrgPolicyShouldNotBeEnabled( - OrganizationDomain domain, SutProvider sutProvider) + OrganizationDomain domain, SutProvider sutProvider) { sutProvider.GetDependency() .GetClaimedDomainsByDomainNameAsync(domain.DomainName) @@ -203,6 +225,9 @@ public async Task UserVerifyOrganizationDomainAsync_GivenOrganizationDomainWithA .ResolveAsync(domain.DomainName, domain.Txt) .Returns(false); + sutProvider.GetDependency() + .UserId.Returns(Guid.NewGuid()); + sutProvider.GetDependency() .IsEnabled(FeatureFlagKeys.AccountDeprovisioning) .Returns(true); @@ -227,6 +252,9 @@ public async Task UserVerifyOrganizationDomainAsync_GivenOrganizationDomainWithA .ResolveAsync(domain.DomainName, domain.Txt) .Returns(false); + sutProvider.GetDependency() + .UserId.Returns(Guid.NewGuid()); + sutProvider.GetDependency() .IsEnabled(FeatureFlagKeys.AccountDeprovisioning) .Returns(true); From d485305b20db1628b446e363fb3051f1130a32bc Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Tue, 26 Nov 2024 11:16:45 -0600 Subject: [PATCH 38/49] splitting up tests --- .../SingleOrgPolicyValidatorTests.cs | 104 ++++++++++++++---- 1 file changed, 80 insertions(+), 24 deletions(-) diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs index 83d10a7ec980..0731920757a6 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs @@ -63,11 +63,8 @@ public async Task ValidateAsync_DisablingPolicy_KeyConnectorNotEnabled_Success( Assert.True(string.IsNullOrEmpty(result)); } - [Theory] - [BitAutoData(true)] - [BitAutoData(false)] - public async Task OnSaveSideEffectsAsync_RemovesNonCompliantUsers( - bool isAccountDeprovisioningEnabled, + [Theory, BitAutoData] + public async Task OnSaveSideEffectsAsync_RevokesNonCompliantUsers( [PolicyUpdate(PolicyType.SingleOrg)] PolicyUpdate policyUpdate, [Policy(PolicyType.SingleOrg, false)] Policy policy, Guid savingUserId, @@ -122,7 +119,7 @@ public async Task OnSaveSideEffectsAsync_RemovesNonCompliantUsers( sutProvider.GetDependency().GetByIdAsync(policyUpdate.OrganizationId).Returns(organization); sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.AccountDeprovisioning) - .Returns(isAccountDeprovisioningEnabled); + .Returns(true); sutProvider.GetDependency() .RevokeNonCompliantOrganizationUsersAsync(Arg.Any()) @@ -130,27 +127,86 @@ public async Task OnSaveSideEffectsAsync_RemovesNonCompliantUsers( await sutProvider.Sut.OnSaveSideEffectsAsync(policyUpdate, policy); - if (isAccountDeprovisioningEnabled) + await sutProvider.GetDependency() + .Received(1) + .RevokeNonCompliantOrganizationUsersAsync(Arg.Any()); + await sutProvider.GetDependency() + .Received(1) + .SendOrganizationUserRevokedForPolicySingleOrgEmailAsync(organization.DisplayName(), + "user3@example.com"); + } + + [Theory, BitAutoData] + public async Task OnSaveSideEffectsAsync_RemovesNonCompliantUsers( + [PolicyUpdate(PolicyType.SingleOrg)] PolicyUpdate policyUpdate, + [Policy(PolicyType.SingleOrg, false)] Policy policy, + Guid savingUserId, + Guid nonCompliantUserId, + Organization organization, SutProvider sutProvider) + { + policy.OrganizationId = organization.Id = policyUpdate.OrganizationId; + + var compliantUser1 = new OrganizationUserUserDetails + { + OrganizationId = organization.Id, + Type = OrganizationUserType.User, + Status = OrganizationUserStatusType.Confirmed, + UserId = new Guid(), + Email = "user1@example.com" + }; + + var compliantUser2 = new OrganizationUserUserDetails + { + OrganizationId = organization.Id, + Type = OrganizationUserType.User, + Status = OrganizationUserStatusType.Confirmed, + UserId = new Guid(), + Email = "user2@example.com" + }; + + var nonCompliantUser = new OrganizationUserUserDetails { - await sutProvider.GetDependency() - .Received(1) - .RevokeNonCompliantOrganizationUsersAsync(Arg.Any()); - await sutProvider.GetDependency() - .Received(1) - .SendOrganizationUserRevokedForPolicySingleOrgEmailAsync(organization.DisplayName(), - "user3@example.com"); - } - else + OrganizationId = organization.Id, + Type = OrganizationUserType.User, + Status = OrganizationUserStatusType.Confirmed, + UserId = nonCompliantUserId, + Email = "user3@example.com" + }; + + sutProvider.GetDependency() + .GetManyDetailsByOrganizationAsync(policyUpdate.OrganizationId) + .Returns([compliantUser1, compliantUser2, nonCompliantUser]); + + var otherOrganizationUser = new OrganizationUser { - await sutProvider.GetDependency() - .Received(1) - .RemoveUserAsync(policyUpdate.OrganizationId, nonCompliantUser.Id, savingUserId); - await sutProvider.GetDependency() - .Received(1) - .SendOrganizationUserRemovedForPolicySingleOrgEmailAsync(organization.DisplayName(), - "user3@example.com"); - } + OrganizationId = new Guid(), + UserId = nonCompliantUserId, + Status = OrganizationUserStatusType.Confirmed + }; + + sutProvider.GetDependency() + .GetManyByManyUsersAsync(Arg.Is>(ids => ids.Contains(nonCompliantUserId))) + .Returns([otherOrganizationUser]); + + sutProvider.GetDependency().UserId.Returns(savingUserId); + sutProvider.GetDependency().GetByIdAsync(policyUpdate.OrganizationId).Returns(organization); + + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.AccountDeprovisioning) + .Returns(false); + + sutProvider.GetDependency() + .RevokeNonCompliantOrganizationUsersAsync(Arg.Any()) + .Returns(new CommandResult()); + + await sutProvider.Sut.OnSaveSideEffectsAsync(policyUpdate, policy); + await sutProvider.GetDependency() + .Received(1) + .RemoveUserAsync(policyUpdate.OrganizationId, nonCompliantUser.Id, savingUserId); + await sutProvider.GetDependency() + .Received(1) + .SendOrganizationUserRemovedForPolicySingleOrgEmailAsync(organization.DisplayName(), + "user3@example.com"); } [Theory, BitAutoData] From 68cdc343ea072a0ce3c64f0edcee4613f222bddb Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Fri, 15 Nov 2024 09:45:26 -0600 Subject: [PATCH 39/49] Added templates and email side effect for claiming a domain. --- .../VerifyOrganizationDomainCommand.cs | 29 +++++++++++++++++-- .../DomainClaimedByOrganization.html.hbs | 0 .../DomainClaimedByOrganization.text.hbs | 0 ...VerifiedDomainUserNotificationViewModel.cs | 6 ++++ src/Core/Services/IMailService.cs | 1 + .../Implementations/HandlebarsMailService.cs | 13 +++++++++ .../NoopImplementations/NoopMailService.cs | 1 + 7 files changed, 47 insertions(+), 3 deletions(-) create mode 100644 src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.html.hbs create mode 100644 src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.text.hbs create mode 100644 src/Core/Models/Mail/VerifiedDomainUserNotificationViewModel.cs diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs index dd6add669f2d..2ec2e05e1d4c 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs @@ -22,6 +22,9 @@ public class VerifyOrganizationDomainCommand( IPolicyService policyService, IFeatureService featureService, ICurrentContext currentContext, + IMailService mailService, + IOrganizationUserRepository organizationUserRepository, + IOrganizationRepository organizationRepository, ILogger logger) : IVerifyOrganizationDomainCommand { @@ -50,7 +53,7 @@ await eventService.LogOrganizationDomainEventAsync(domainVerificationResult, return domainVerificationResult; } - public async Task SystemVerifyOrganizationDomainAsync(OrganizationDomain organizationDomain) + public async Task SystemVerifyOrganizationDomainAsync(OrganizationDomain organizationDomain) // need request object to hold who is doing the action { var actingUser = new SystemUser(EventSystemUser.DomainVerification); @@ -109,7 +112,7 @@ private async Task VerifyOrganizationDomainAsync(Organizatio { domain.SetVerifiedDate(); - await EnableSingleOrganizationPolicyAsync(domain.OrganizationId, actingUser); + await DomainVerificationSideEffectsAsync(domain.OrganizationId, actingUser); } } catch (Exception e) @@ -121,7 +124,7 @@ private async Task VerifyOrganizationDomainAsync(Organizatio return domain; } - private async Task EnableSingleOrganizationPolicyAsync(Guid organizationId, IActingUser actingUser) + private async Task DomainVerificationSideEffectsAsync(Guid organizationId, IActingUser actingUser) { if (featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning)) { @@ -131,4 +134,24 @@ await policyService.SaveAsync( eventSystemUser: actingUser is SystemUser systemUser ? systemUser.SystemUserType : null); } } + + private async Task EnableSingleOrganizationPolicyAsync(Guid organizationId, IActingUser actingUser) => + await policyService.SaveAsync( + new Policy { OrganizationId = organizationId, Type = PolicyType.SingleOrg, Enabled = true }, + savingUserId: actingUser is StandardUser standardUser ? standardUser.UserId : null, + eventSystemUser: actingUser is SystemUser systemUser ? systemUser.SystemUserType : null); + + private async Task SendVerifiedDomainUserEmailAsync(Guid organizationId) + { + var orgUsers = await organizationUserRepository.GetManyByOrganizationWithClaimedDomainsAsync(organizationId); + var orgUserUsers = await organizationUserRepository.GetManyDetailsByOrganizationAsync(organizationId); + + var userEmails = orgUsers.Where(x => x.Status != OrganizationUserStatusType.Revoked) + .Select(y => orgUserUsers.FirstOrDefault(x => x.Id == y.Id)?.Email) + .Where(x => x is not null); + + var organization = await organizationRepository.GetByIdAsync(organizationId); + + await Task.WhenAll(userEmails.Select(email => mailService.SendVerifiedDomainUserEmailAsync(email, organization))); + } } diff --git a/src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.html.hbs b/src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.html.hbs new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.text.hbs b/src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.text.hbs new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/src/Core/Models/Mail/VerifiedDomainUserNotificationViewModel.cs b/src/Core/Models/Mail/VerifiedDomainUserNotificationViewModel.cs new file mode 100644 index 000000000000..41b17cbd50e1 --- /dev/null +++ b/src/Core/Models/Mail/VerifiedDomainUserNotificationViewModel.cs @@ -0,0 +1,6 @@ +namespace Bit.Core.Models.Mail; + +public class VerifiedDomainUserNotificationViewModel : BaseMailModel +{ + public string OrganizationName { get; init; } +} diff --git a/src/Core/Services/IMailService.cs b/src/Core/Services/IMailService.cs index bc8d1440f155..e1cb4c229970 100644 --- a/src/Core/Services/IMailService.cs +++ b/src/Core/Services/IMailService.cs @@ -93,5 +93,6 @@ Task SendProviderUpdatePaymentMethod( Task SendRequestSMAccessToAdminEmailAsync(IEnumerable adminEmails, string organizationName, string userRequestingAccess, string emailContent); Task SendFamiliesForEnterpriseRemoveSponsorshipsEmailAsync(string email, string offerAcceptanceDate, string organizationId, string organizationName); + Task SendVerifiedDomainUserEmailAsync(string email, Organization organization); } diff --git a/src/Core/Services/Implementations/HandlebarsMailService.cs b/src/Core/Services/Implementations/HandlebarsMailService.cs index acc729e53ce0..681321fc3bd1 100644 --- a/src/Core/Services/Implementations/HandlebarsMailService.cs +++ b/src/Core/Services/Implementations/HandlebarsMailService.cs @@ -460,6 +460,19 @@ public async Task SendRequestSMAccessToAdminEmailAsync(IEnumerable email await _mailDeliveryService.SendEmailAsync(message); } + public async Task SendVerifiedDomainUserEmailAsync(string email, Organization organization) + { + var message = CreateDefaultMessage($"Your Bitwarden account is claimed by {organization.DisplayName()}", email); + + var model = new VerifiedDomainUserNotificationViewModel + { + OrganizationName = CoreHelpers.SanitizeForEmail(organization.DisplayName(), false) + }; + await AddMessageContentAsync(message, "AdminConsole.VerifiedDomainUserNotification", model); + message.Category = "VerifiedDomainUserNotification"; + await _mailDeliveryService.SendEmailAsync(message); + } + public async Task SendNewDeviceLoggedInEmail(string email, string deviceType, DateTime timestamp, string ip) { var message = CreateDefaultMessage($"New Device Logged In From {deviceType}", email); diff --git a/src/Core/Services/NoopImplementations/NoopMailService.cs b/src/Core/Services/NoopImplementations/NoopMailService.cs index 399874eee73d..b00ade3e43d5 100644 --- a/src/Core/Services/NoopImplementations/NoopMailService.cs +++ b/src/Core/Services/NoopImplementations/NoopMailService.cs @@ -309,5 +309,6 @@ public Task SendFamiliesForEnterpriseRemoveSponsorshipsEmailAsync(string email, { return Task.FromResult(0); } + public Task SendVerifiedDomainUserEmailAsync(string email, Organization organization) => Task.CompletedTask; } From eb79696dfbc811f8f0ea855bdb3f506fe20b6bfa Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Fri, 15 Nov 2024 14:13:15 -0600 Subject: [PATCH 40/49] bringing changes from nc user changes. --- .../VerifyOrganizationDomainCommand.cs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs index 2ec2e05e1d4c..3780c18810c3 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs @@ -28,8 +28,6 @@ public class VerifyOrganizationDomainCommand( ILogger logger) : IVerifyOrganizationDomainCommand { - - public async Task UserVerifyOrganizationDomainAsync(OrganizationDomain organizationDomain) { if (currentContext.UserId is null) @@ -53,7 +51,7 @@ await eventService.LogOrganizationDomainEventAsync(domainVerificationResult, return domainVerificationResult; } - public async Task SystemVerifyOrganizationDomainAsync(OrganizationDomain organizationDomain) // need request object to hold who is doing the action + public async Task SystemVerifyOrganizationDomainAsync(OrganizationDomain organizationDomain) { var actingUser = new SystemUser(EventSystemUser.DomainVerification); @@ -128,10 +126,8 @@ private async Task DomainVerificationSideEffectsAsync(Guid organizationId, IActi { if (featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning)) { - await policyService.SaveAsync( - new Policy { OrganizationId = organizationId, Type = PolicyType.SingleOrg, Enabled = true }, - savingUserId: actingUser is StandardUser standardUser ? standardUser.UserId : null, - eventSystemUser: actingUser is SystemUser systemUser ? systemUser.SystemUserType : null); + await EnableSingleOrganizationPolicyAsync(organizationId, actingUser); + await SendVerifiedDomainUserEmailAsync(organizationId); } } From 1d321e485b3b6f91b91a819e472f220fa1f08502 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Fri, 15 Nov 2024 14:49:06 -0600 Subject: [PATCH 41/49] Switched to enqueue mail message. --- .../VerifyOrganizationDomainCommand.cs | 3 ++- .../ManagedUserDomainClaimedEmails.cs | 5 +++++ ...VerifiedDomainUserNotificationViewModel.cs | 2 +- src/Core/Services/IMailService.cs | 3 ++- .../Implementations/HandlebarsMailService.cs | 22 +++++++++++-------- .../NoopImplementations/NoopMailService.cs | 3 ++- 6 files changed, 25 insertions(+), 13 deletions(-) create mode 100644 src/Core/Models/Data/Organizations/ManagedUserDomainClaimedEmails.cs diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs index 3780c18810c3..f300ce570071 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs @@ -7,6 +7,7 @@ using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Exceptions; +using Bit.Core.Models.Data.Organizations; using Bit.Core.Repositories; using Bit.Core.Services; using Bit.Core.Settings; @@ -148,6 +149,6 @@ private async Task SendVerifiedDomainUserEmailAsync(Guid organizationId) var organization = await organizationRepository.GetByIdAsync(organizationId); - await Task.WhenAll(userEmails.Select(email => mailService.SendVerifiedDomainUserEmailAsync(email, organization))); + await mailService.SendVerifiedDomainUserEmailAsync(new ManagedUserDomainClaimedEmails(userEmails, organization)); } } diff --git a/src/Core/Models/Data/Organizations/ManagedUserDomainClaimedEmails.cs b/src/Core/Models/Data/Organizations/ManagedUserDomainClaimedEmails.cs new file mode 100644 index 000000000000..429257e26611 --- /dev/null +++ b/src/Core/Models/Data/Organizations/ManagedUserDomainClaimedEmails.cs @@ -0,0 +1,5 @@ +using Bit.Core.AdminConsole.Entities; + +namespace Bit.Core.Models.Data.Organizations; + +public record ManagedUserDomainClaimedEmails(IEnumerable EmailList, Organization Organization); diff --git a/src/Core/Models/Mail/VerifiedDomainUserNotificationViewModel.cs b/src/Core/Models/Mail/VerifiedDomainUserNotificationViewModel.cs index 41b17cbd50e1..1b11d2bcfa9f 100644 --- a/src/Core/Models/Mail/VerifiedDomainUserNotificationViewModel.cs +++ b/src/Core/Models/Mail/VerifiedDomainUserNotificationViewModel.cs @@ -1,6 +1,6 @@ namespace Bit.Core.Models.Mail; -public class VerifiedDomainUserNotificationViewModel : BaseMailModel +public class VerifiedDomainUserNotificationViewModel : BaseTitleContactUsMailModel { public string OrganizationName { get; init; } } diff --git a/src/Core/Services/IMailService.cs b/src/Core/Services/IMailService.cs index e1cb4c229970..4cabfe29c5d0 100644 --- a/src/Core/Services/IMailService.cs +++ b/src/Core/Services/IMailService.cs @@ -3,6 +3,7 @@ using Bit.Core.Auth.Entities; using Bit.Core.Billing.Enums; using Bit.Core.Entities; +using Bit.Core.Models.Data.Organizations; using Bit.Core.Models.Mail; namespace Bit.Core.Services; @@ -93,6 +94,6 @@ Task SendProviderUpdatePaymentMethod( Task SendRequestSMAccessToAdminEmailAsync(IEnumerable adminEmails, string organizationName, string userRequestingAccess, string emailContent); Task SendFamiliesForEnterpriseRemoveSponsorshipsEmailAsync(string email, string offerAcceptanceDate, string organizationId, string organizationName); - Task SendVerifiedDomainUserEmailAsync(string email, Organization organization); + Task SendVerifiedDomainUserEmailAsync(ManagedUserDomainClaimedEmails emailList); } diff --git a/src/Core/Services/Implementations/HandlebarsMailService.cs b/src/Core/Services/Implementations/HandlebarsMailService.cs index 681321fc3bd1..e32e806793d5 100644 --- a/src/Core/Services/Implementations/HandlebarsMailService.cs +++ b/src/Core/Services/Implementations/HandlebarsMailService.cs @@ -7,6 +7,7 @@ using Bit.Core.Billing.Enums; using Bit.Core.Billing.Models.Mail; using Bit.Core.Entities; +using Bit.Core.Models.Data.Organizations; using Bit.Core.Models.Mail; using Bit.Core.Models.Mail.FamiliesForEnterprise; using Bit.Core.Models.Mail.Provider; @@ -460,17 +461,20 @@ public async Task SendRequestSMAccessToAdminEmailAsync(IEnumerable email await _mailDeliveryService.SendEmailAsync(message); } - public async Task SendVerifiedDomainUserEmailAsync(string email, Organization organization) + public async Task SendVerifiedDomainUserEmailAsync(ManagedUserDomainClaimedEmails emailList) { - var message = CreateDefaultMessage($"Your Bitwarden account is claimed by {organization.DisplayName()}", email); + await EnqueueMailAsync(emailList.EmailList.Select(email => + CreateMessage(email, emailList.Organization))); + return; - var model = new VerifiedDomainUserNotificationViewModel - { - OrganizationName = CoreHelpers.SanitizeForEmail(organization.DisplayName(), false) - }; - await AddMessageContentAsync(message, "AdminConsole.VerifiedDomainUserNotification", model); - message.Category = "VerifiedDomainUserNotification"; - await _mailDeliveryService.SendEmailAsync(message); + MailQueueMessage CreateMessage(string emailAddress, Organization org) => + new(CreateDefaultMessage($"Your Bitwarden account is claimed by {org.DisplayName()}", emailAddress), + "AdminConsole.VerifiedDomainUserNotification", + new VerifiedDomainUserNotificationViewModel + { + TitleFirst = $"Hey {emailAddress}, here is a heads up on your claimed account:", + OrganizationName = CoreHelpers.SanitizeForEmail(org.DisplayName(), false) + }); } public async Task SendNewDeviceLoggedInEmail(string email, string deviceType, DateTime timestamp, string ip) diff --git a/src/Core/Services/NoopImplementations/NoopMailService.cs b/src/Core/Services/NoopImplementations/NoopMailService.cs index b00ade3e43d5..deb95aabc574 100644 --- a/src/Core/Services/NoopImplementations/NoopMailService.cs +++ b/src/Core/Services/NoopImplementations/NoopMailService.cs @@ -3,6 +3,7 @@ using Bit.Core.Auth.Entities; using Bit.Core.Billing.Enums; using Bit.Core.Entities; +using Bit.Core.Models.Data.Organizations; using Bit.Core.Models.Mail; namespace Bit.Core.Services; @@ -309,6 +310,6 @@ public Task SendFamiliesForEnterpriseRemoveSponsorshipsEmailAsync(string email, { return Task.FromResult(0); } - public Task SendVerifiedDomainUserEmailAsync(string email, Organization organization) => Task.CompletedTask; + public Task SendVerifiedDomainUserEmailAsync(ManagedUserDomainClaimedEmails emailList) => Task.CompletedTask; } From 2759afd3c10259c54b2aca97b687f3863bb9a95e Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Mon, 18 Nov 2024 09:06:49 -0600 Subject: [PATCH 42/49] Filled in DomainClaimedByOrganization.html.hbs --- .../DomainClaimedByOrganization.html.hbs | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.html.hbs b/src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.html.hbs index e69de29bb2d1..21a78a7f22b1 100644 --- a/src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.html.hbs +++ b/src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.html.hbs @@ -0,0 +1,24 @@ +{{#>TitleContactUsHtmlLayout}} + + + + + + + + + + +
+ As a member of {{OrganizationName}}, your Bitwarden account is claimed and owned by your organization. +
+ Here's what that means: +
    +
  • This account should only be used to store items related to {{OrganizationName}}
    • +
  • Your admins managing your Bitwarden organization manages your email address and other account settings
    • +
  • Your admins can also revoke or delete your account at any time
    • +
+
+ For more information, please refer to the following help article: Claimed Accounts +
+{{/TitleContactUsHtmlLayout}} From baab61774c2a1cb6e8e7b281c85bb4fbb06873af Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Tue, 19 Nov 2024 14:23:48 -0600 Subject: [PATCH 43/49] Added text document for domain claiming --- .../AdminConsole/DomainClaimedByOrganization.text.hbs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.text.hbs b/src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.text.hbs index e69de29bb2d1..c0078d389dbd 100644 --- a/src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.text.hbs +++ b/src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.text.hbs @@ -0,0 +1,8 @@ +As a member of {{OrganizationName}}, your Bitwarden account is claimed and owned by your organization. + +Here's what that means: +- This account should only be used to store items related to {{OrganizationName}} +- Your admins managing your Bitwarden organization manages your email address and other account settings +- Your admins can also revoke or delete your account at any time + +For more information, please refer to the following help article: Claimed Accounts (https://bitwarden.com/help/claimed-accounts) From 934438a7ecf10051da6a6fa08fcfdfaa5230bd48 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Wed, 27 Nov 2024 08:28:52 -0600 Subject: [PATCH 44/49] Fixing migration script. --- util/Migrator/DbScripts/2024-11-26-00_OrgUserSetStatusBulk.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/Migrator/DbScripts/2024-11-26-00_OrgUserSetStatusBulk.sql b/util/Migrator/DbScripts/2024-11-26-00_OrgUserSetStatusBulk.sql index 95151f90ded5..5c51f9da40c7 100644 --- a/util/Migrator/DbScripts/2024-11-26-00_OrgUserSetStatusBulk.sql +++ b/util/Migrator/DbScripts/2024-11-26-00_OrgUserSetStatusBulk.sql @@ -1,4 +1,4 @@ -CREATE PROCEDURE [dbo].[OrganizationUser_SetStatusForUsersById] +CREATE OR ALTER PROCEDURE[dbo].[OrganizationUser_SetStatusForUsersById] @OrganizationUserIds AS NVARCHAR(MAX), @Status SMALLINT AS From 768a83374435c0e475cabef916d65b9828d4d05e Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Wed, 27 Nov 2024 08:39:21 -0600 Subject: [PATCH 45/49] Remove old sproc --- .../2024-11-06-00_OrgUserSetStatusBulk.sql | 28 ------------------- 1 file changed, 28 deletions(-) delete mode 100644 util/Migrator/DbScripts/2024-11-06-00_OrgUserSetStatusBulk.sql diff --git a/util/Migrator/DbScripts/2024-11-06-00_OrgUserSetStatusBulk.sql b/util/Migrator/DbScripts/2024-11-06-00_OrgUserSetStatusBulk.sql deleted file mode 100644 index 95151f90ded5..000000000000 --- a/util/Migrator/DbScripts/2024-11-06-00_OrgUserSetStatusBulk.sql +++ /dev/null @@ -1,28 +0,0 @@ -CREATE PROCEDURE [dbo].[OrganizationUser_SetStatusForUsersById] - @OrganizationUserIds AS NVARCHAR(MAX), - @Status SMALLINT -AS -BEGIN - SET NOCOUNT ON - - -- Declare a table variable to hold the parsed JSON data - DECLARE @ParsedIds TABLE (Id UNIQUEIDENTIFIER); - - -- Parse the JSON input into the table variable - INSERT INTO @ParsedIds (Id) - SELECT value - FROM OPENJSON(@OrganizationUserIds); - - -- Check if the input table is empty - IF (SELECT COUNT(1) FROM @ParsedIds) < 1 - BEGIN - RETURN(-1); - END - - UPDATE - [dbo].[OrganizationUser] - SET [Status] = @Status - WHERE [Id] IN (SELECT Id from @ParsedIds) - - EXEC [dbo].[User_BumpAccountRevisionDateByOrganizationUserIds] @OrganizationUserIds -END From e0909e2487819f7523c7d3b0817d4dd73306c5e0 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Wed, 27 Nov 2024 10:36:14 -0600 Subject: [PATCH 46/49] Limiting sending of the email down to users who are a part of the domain being claimed. --- .../VerifyOrganizationDomainCommand.cs | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs index f300ce570071..85d51e7c60f5 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs @@ -111,7 +111,7 @@ private async Task VerifyOrganizationDomainAsync(Organizatio { domain.SetVerifiedDate(); - await DomainVerificationSideEffectsAsync(domain.OrganizationId, actingUser); + await DomainVerificationSideEffectsAsync(domain, actingUser); } } catch (Exception e) @@ -123,12 +123,12 @@ private async Task VerifyOrganizationDomainAsync(Organizatio return domain; } - private async Task DomainVerificationSideEffectsAsync(Guid organizationId, IActingUser actingUser) + private async Task DomainVerificationSideEffectsAsync(OrganizationDomain domain, IActingUser actingUser) { if (featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning)) { - await EnableSingleOrganizationPolicyAsync(organizationId, actingUser); - await SendVerifiedDomainUserEmailAsync(organizationId); + await EnableSingleOrganizationPolicyAsync(domain.OrganizationId, actingUser); + await SendVerifiedDomainUserEmailAsync(domain); } } @@ -138,17 +138,18 @@ await policyService.SaveAsync( savingUserId: actingUser is StandardUser standardUser ? standardUser.UserId : null, eventSystemUser: actingUser is SystemUser systemUser ? systemUser.SystemUserType : null); - private async Task SendVerifiedDomainUserEmailAsync(Guid organizationId) + private async Task SendVerifiedDomainUserEmailAsync(OrganizationDomain domain) { - var orgUsers = await organizationUserRepository.GetManyByOrganizationWithClaimedDomainsAsync(organizationId); - var orgUserUsers = await organizationUserRepository.GetManyDetailsByOrganizationAsync(organizationId); + var orgUserUsers = await organizationUserRepository.GetManyDetailsByOrganizationAsync(domain.OrganizationId); - var userEmails = orgUsers.Where(x => x.Status != OrganizationUserStatusType.Revoked) - .Select(y => orgUserUsers.FirstOrDefault(x => x.Id == y.Id)?.Email) - .Where(x => x is not null); + var domainUserEmails = orgUserUsers + .Where(ou => ou.Email.ToLower().EndsWith($"@{domain.DomainName.ToLower()}") && + ou.Status != OrganizationUserStatusType.Revoked && + ou.Status != OrganizationUserStatusType.Invited) + .Select(ou => ou.Email); - var organization = await organizationRepository.GetByIdAsync(organizationId); + var organization = await organizationRepository.GetByIdAsync(domain.OrganizationId); - await mailService.SendVerifiedDomainUserEmailAsync(new ManagedUserDomainClaimedEmails(userEmails, organization)); + await mailService.SendVerifiedDomainUserEmailAsync(new ManagedUserDomainClaimedEmails(domainUserEmails, organization)); } } From efa160435e3031287f03aa3fc6c3908359dd8084 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Wed, 27 Nov 2024 11:21:56 -0600 Subject: [PATCH 47/49] Added test for change --- .../VerifyOrganizationDomainCommandTests.cs | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommandTests.cs index 8dbe5331317c..0565f8911504 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommandTests.cs @@ -6,6 +6,8 @@ using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Exceptions; +using Bit.Core.Models.Data.Organizations; +using Bit.Core.Models.Data.Organizations.OrganizationUsers; using Bit.Core.Repositories; using Bit.Core.Services; using Bit.Test.Common.AutoFixture; @@ -265,4 +267,53 @@ await sutProvider.GetDependency() .DidNotReceive() .SaveAsync(Arg.Any(), null); } + + [Theory, BitAutoData] + public async Task UserVerifyOrganizationDomainAsync_GivenOrganizationDomainWithAccountDeprovisioningEnabled_WhenDomainIsVerified_ThenEmailShouldBeSentToUsersWhoBelongToTheDomain( + ICollection organizationUsers, + OrganizationDomain domain, + Organization organization, + SutProvider sutProvider) + { + foreach (var organizationUser in organizationUsers) + { + organizationUser.Email = $"{organizationUser.Name}@{domain.DomainName}"; + } + + var mockedUsers = organizationUsers + .Where(x => x.Status != OrganizationUserStatusType.Invited && + x.Status != OrganizationUserStatusType.Revoked).ToList(); + + organization.Id = domain.OrganizationId; + + sutProvider.GetDependency() + .GetClaimedDomainsByDomainNameAsync(domain.DomainName) + .Returns([]); + + sutProvider.GetDependency() + .GetByIdAsync(domain.OrganizationId) + .Returns(organization); + + sutProvider.GetDependency() + .ResolveAsync(domain.DomainName, domain.Txt) + .Returns(true); + + sutProvider.GetDependency() + .UserId.Returns(Guid.NewGuid()); + + sutProvider.GetDependency() + .IsEnabled(FeatureFlagKeys.AccountDeprovisioning) + .Returns(true); + + sutProvider.GetDependency() + .GetManyDetailsByOrganizationAsync(domain.OrganizationId) + .Returns(mockedUsers); + + _ = await sutProvider.Sut.UserVerifyOrganizationDomainAsync(domain); + + await sutProvider.GetDependency().Received().SendVerifiedDomainUserEmailAsync( + Arg.Is(x => + x.EmailList.Count(e => e.EndsWith(domain.DomainName)) == mockedUsers.Count && + x.Organization.Id == organization.Id)); + } } From 8db1c25aa6785362665d6fc6dad5d2e2a57214b1 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Wed, 27 Nov 2024 12:03:11 -0600 Subject: [PATCH 48/49] Renames and fixed up email. --- .../VerifyOrganizationDomainCommand.cs | 2 +- .../AdminConsole/DomainClaimedByOrganization.html.hbs | 10 +++++----- ...el.cs => ClaimedDomainUserNotificationViewModel.cs} | 2 +- src/Core/Services/IMailService.cs | 2 +- .../Services/Implementations/HandlebarsMailService.cs | 6 +++--- .../Services/NoopImplementations/NoopMailService.cs | 2 +- .../VerifyOrganizationDomainCommandTests.cs | 2 +- 7 files changed, 13 insertions(+), 13 deletions(-) rename src/Core/Models/Mail/{VerifiedDomainUserNotificationViewModel.cs => ClaimedDomainUserNotificationViewModel.cs} (52%) diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs index 85d51e7c60f5..995f8c02197c 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs @@ -150,6 +150,6 @@ private async Task SendVerifiedDomainUserEmailAsync(OrganizationDomain domain) var organization = await organizationRepository.GetByIdAsync(domain.OrganizationId); - await mailService.SendVerifiedDomainUserEmailAsync(new ManagedUserDomainClaimedEmails(domainUserEmails, organization)); + await mailService.SendClaimedDomainUserEmailAsync(new ManagedUserDomainClaimedEmails(domainUserEmails, organization)); } } diff --git a/src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.html.hbs b/src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.html.hbs index 21a78a7f22b1..779de47f4017 100644 --- a/src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.html.hbs +++ b/src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.html.hbs @@ -1,22 +1,22 @@ {{#>TitleContactUsHtmlLayout}} - +
- - diff --git a/src/Core/Models/Mail/VerifiedDomainUserNotificationViewModel.cs b/src/Core/Models/Mail/ClaimedDomainUserNotificationViewModel.cs similarity index 52% rename from src/Core/Models/Mail/VerifiedDomainUserNotificationViewModel.cs rename to src/Core/Models/Mail/ClaimedDomainUserNotificationViewModel.cs index 1b11d2bcfa9f..97591b51bc94 100644 --- a/src/Core/Models/Mail/VerifiedDomainUserNotificationViewModel.cs +++ b/src/Core/Models/Mail/ClaimedDomainUserNotificationViewModel.cs @@ -1,6 +1,6 @@ namespace Bit.Core.Models.Mail; -public class VerifiedDomainUserNotificationViewModel : BaseTitleContactUsMailModel +public class ClaimedDomainUserNotificationViewModel : BaseTitleContactUsMailModel { public string OrganizationName { get; init; } } diff --git a/src/Core/Services/IMailService.cs b/src/Core/Services/IMailService.cs index 4cabfe29c5d0..c6c9dc794820 100644 --- a/src/Core/Services/IMailService.cs +++ b/src/Core/Services/IMailService.cs @@ -94,6 +94,6 @@ Task SendProviderUpdatePaymentMethod( Task SendRequestSMAccessToAdminEmailAsync(IEnumerable adminEmails, string organizationName, string userRequestingAccess, string emailContent); Task SendFamiliesForEnterpriseRemoveSponsorshipsEmailAsync(string email, string offerAcceptanceDate, string organizationId, string organizationName); - Task SendVerifiedDomainUserEmailAsync(ManagedUserDomainClaimedEmails emailList); + Task SendClaimedDomainUserEmailAsync(ManagedUserDomainClaimedEmails emailList); } diff --git a/src/Core/Services/Implementations/HandlebarsMailService.cs b/src/Core/Services/Implementations/HandlebarsMailService.cs index e32e806793d5..c220df18a127 100644 --- a/src/Core/Services/Implementations/HandlebarsMailService.cs +++ b/src/Core/Services/Implementations/HandlebarsMailService.cs @@ -461,7 +461,7 @@ public async Task SendRequestSMAccessToAdminEmailAsync(IEnumerable email await _mailDeliveryService.SendEmailAsync(message); } - public async Task SendVerifiedDomainUserEmailAsync(ManagedUserDomainClaimedEmails emailList) + public async Task SendClaimedDomainUserEmailAsync(ManagedUserDomainClaimedEmails emailList) { await EnqueueMailAsync(emailList.EmailList.Select(email => CreateMessage(email, emailList.Organization))); @@ -469,8 +469,8 @@ await EnqueueMailAsync(emailList.EmailList.Select(email => MailQueueMessage CreateMessage(string emailAddress, Organization org) => new(CreateDefaultMessage($"Your Bitwarden account is claimed by {org.DisplayName()}", emailAddress), - "AdminConsole.VerifiedDomainUserNotification", - new VerifiedDomainUserNotificationViewModel + "AdminConsole.DomainClaimedByOrganization", + new ClaimedDomainUserNotificationViewModel { TitleFirst = $"Hey {emailAddress}, here is a heads up on your claimed account:", OrganizationName = CoreHelpers.SanitizeForEmail(org.DisplayName(), false) diff --git a/src/Core/Services/NoopImplementations/NoopMailService.cs b/src/Core/Services/NoopImplementations/NoopMailService.cs index deb95aabc574..e8ea8d9863b1 100644 --- a/src/Core/Services/NoopImplementations/NoopMailService.cs +++ b/src/Core/Services/NoopImplementations/NoopMailService.cs @@ -310,6 +310,6 @@ public Task SendFamiliesForEnterpriseRemoveSponsorshipsEmailAsync(string email, { return Task.FromResult(0); } - public Task SendVerifiedDomainUserEmailAsync(ManagedUserDomainClaimedEmails emailList) => Task.CompletedTask; + public Task SendClaimedDomainUserEmailAsync(ManagedUserDomainClaimedEmails emailList) => Task.CompletedTask; } diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommandTests.cs index 0565f8911504..b565cf9a8e94 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommandTests.cs @@ -311,7 +311,7 @@ public async Task UserVerifyOrganizationDomainAsync_GivenOrganizationDomainWithA _ = await sutProvider.Sut.UserVerifyOrganizationDomainAsync(domain); - await sutProvider.GetDependency().Received().SendVerifiedDomainUserEmailAsync( + await sutProvider.GetDependency().Received().SendClaimedDomainUserEmailAsync( Arg.Is(x => x.EmailList.Count(e => e.EndsWith(domain.DomainName)) == mockedUsers.Count && x.Organization.Id == organization.Id)); From 78f115980c8b7eb44b45fded86f5473ef76bd505 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Wed, 27 Nov 2024 12:12:47 -0600 Subject: [PATCH 49/49] Fixing up CSS --- .../AdminConsole/DomainClaimedByOrganization.html.hbs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.html.hbs b/src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.html.hbs index 779de47f4017..05ca170a502c 100644 --- a/src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.html.hbs +++ b/src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.html.hbs @@ -1,12 +1,12 @@ {{#>TitleContactUsHtmlLayout}} -
As a member of {{OrganizationName}}, your Bitwarden account is claimed and owned by your organization.
+ Here's what that means:
  • This account should only be used to store items related to {{OrganizationName}}
    • -
  • Your admins managing your Bitwarden organization manages your email address and other account settings
    • -
  • Your admins can also revoke or delete your account at any time
    • +
  • Admins managing your Bitwarden organization manage your email address and other account settings
    • +
  • Admins can also revoke or delete your account at any time
+ For more information, please refer to the following help article: Claimed Accounts
+
- -
As a member of {{OrganizationName}}, your Bitwarden account is claimed and owned by your organization.
+ Here's what that means:
  • This account should only be used to store items related to {{OrganizationName}}
    • @@ -16,7 +16,7 @@
+ For more information, please refer to the following help article: Claimed Accounts