diff --git a/src/Core/AdminConsole/Services/Implementations/PolicyService.cs b/src/Core/AdminConsole/Services/Implementations/PolicyService.cs index 246b4d56d..cd254f615 100644 --- a/src/Core/AdminConsole/Services/Implementations/PolicyService.cs +++ b/src/Core/AdminConsole/Services/Implementations/PolicyService.cs @@ -124,10 +124,17 @@ public class PolicyService : IPolicyService switch (policy.Type) { case PolicyType.TwoFactorAuthentication: - foreach (var orgUser in removableOrgUsers) + // Reorder by HasMasterPassword to prioritize checking users without a master if they have 2FA enabled + foreach (var orgUser in removableOrgUsers.OrderBy(ou => ou.HasMasterPassword)) { if (!await userService.TwoFactorIsEnabledAsync(orgUser)) { + if (!orgUser.HasMasterPassword) + { + 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 organizationService.DeleteUserAsync(policy.OrganizationId, orgUser.Id, savingUserId); await _mailService.SendOrganizationUserRemovedForPolicyTwoStepEmailAsync( diff --git a/test/Core.Test/AdminConsole/Services/PolicyServiceTests.cs b/test/Core.Test/AdminConsole/Services/PolicyServiceTests.cs index 2ae02376b..989aa9067 100644 --- a/test/Core.Test/AdminConsole/Services/PolicyServiceTests.cs +++ b/test/Core.Test/AdminConsole/Services/PolicyServiceTests.cs @@ -273,18 +273,16 @@ public class PolicyServiceTests [Theory, BitAutoData] public async Task SaveAsync_ExistingPolicy_UpdateTwoFactor( - [AdminConsoleFixtures.Policy(PolicyType.TwoFactorAuthentication)] Policy policy, SutProvider sutProvider) + Organization organization, + [AdminConsoleFixtures.Policy(PolicyType.TwoFactorAuthentication)] Policy policy, + SutProvider sutProvider) { // If the policy that this is updating isn't enabled then do some work now that the current one is enabled - var org = new Organization - { - Id = policy.OrganizationId, - UsePolicies = true, - Name = "TEST", - }; + organization.UsePolicies = true; + policy.OrganizationId = organization.Id; - SetupOrg(sutProvider, policy.OrganizationId, org); + SetupOrg(sutProvider, organization.Id, organization); sutProvider.GetDependency() .GetByIdAsync(policy.Id) @@ -292,32 +290,71 @@ public class PolicyServiceTests { Id = policy.Id, Type = PolicyType.TwoFactorAuthentication, - Enabled = false, + Enabled = false }); - var orgUserDetail = new Core.Models.Data.Organizations.OrganizationUsers.OrganizationUserUserDetails + var orgUserDetailUserInvited = new OrganizationUserUserDetails + { + Id = Guid.NewGuid(), + Status = OrganizationUserStatusType.Invited, + Type = OrganizationUserType.User, + // Needs to be different from what is passed in as the savingUserId to Sut.SaveAsync + Email = "user1@test.com", + Name = "TEST", + UserId = Guid.NewGuid(), + HasMasterPassword = false + }; + var orgUserDetailUserAcceptedWith2FA = new OrganizationUserUserDetails { Id = Guid.NewGuid(), Status = OrganizationUserStatusType.Accepted, Type = OrganizationUserType.User, // Needs to be different from what is passed in as the savingUserId to Sut.SaveAsync - Email = "test@bitwarden.com", + Email = "user2@test.com", Name = "TEST", UserId = Guid.NewGuid(), + HasMasterPassword = true + }; + var orgUserDetailUserAcceptedWithout2FA = new OrganizationUserUserDetails + { + Id = Guid.NewGuid(), + Status = OrganizationUserStatusType.Accepted, + 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 = true + }; + var orgUserDetailAdmin = new OrganizationUserUserDetails + { + Id = Guid.NewGuid(), + Status = OrganizationUserStatusType.Confirmed, + Type = OrganizationUserType.Admin, + // Needs to be different from what is passed in as the savingUserId to Sut.SaveAsync + Email = "admin@test.com", + Name = "ADMIN", + UserId = Guid.NewGuid(), + HasMasterPassword = false }; sutProvider.GetDependency() .GetManyDetailsByOrganizationAsync(policy.OrganizationId) - .Returns(new List + .Returns(new List { - orgUserDetail, + orgUserDetailUserInvited, + orgUserDetailUserAcceptedWith2FA, + orgUserDetailUserAcceptedWithout2FA, + orgUserDetailAdmin }); var userService = Substitute.For(); var organizationService = Substitute.For(); - userService.TwoFactorIsEnabledAsync(orgUserDetail) - .Returns(false); + userService.TwoFactorIsEnabledAsync(orgUserDetailUserInvited).Returns(false); + userService.TwoFactorIsEnabledAsync(orgUserDetailUserAcceptedWith2FA).Returns(true); + userService.TwoFactorIsEnabledAsync(orgUserDetailUserAcceptedWithout2FA).Returns(false); + userService.TwoFactorIsEnabledAsync(orgUserDetailAdmin).Returns(false); var utcNow = DateTime.UtcNow; @@ -326,10 +363,22 @@ public class PolicyServiceTests await sutProvider.Sut.SaveAsync(policy, userService, organizationService, savingUserId); await organizationService.Received() - .DeleteUserAsync(policy.OrganizationId, orgUserDetail.Id, savingUserId); - + .DeleteUserAsync(policy.OrganizationId, orgUserDetailUserAcceptedWithout2FA.Id, savingUserId); await sutProvider.GetDependency().Received() - .SendOrganizationUserRemovedForPolicyTwoStepEmailAsync(org.DisplayName(), orgUserDetail.Email); + .SendOrganizationUserRemovedForPolicyTwoStepEmailAsync(organization.DisplayName(), orgUserDetailUserAcceptedWithout2FA.Email); + + await organizationService.DidNotReceive() + .DeleteUserAsync(policy.OrganizationId, orgUserDetailUserInvited.Id, savingUserId); + await sutProvider.GetDependency().DidNotReceive() + .SendOrganizationUserRemovedForPolicyTwoStepEmailAsync(organization.DisplayName(), orgUserDetailUserInvited.Email); + await organizationService.DidNotReceive() + .DeleteUserAsync(policy.OrganizationId, orgUserDetailUserAcceptedWith2FA.Id, savingUserId); + await sutProvider.GetDependency().DidNotReceive() + .SendOrganizationUserRemovedForPolicyTwoStepEmailAsync(organization.DisplayName(), orgUserDetailUserAcceptedWith2FA.Email); + await organizationService.DidNotReceive() + .DeleteUserAsync(policy.OrganizationId, orgUserDetailAdmin.Id, savingUserId); + await sutProvider.GetDependency().DidNotReceive() + .SendOrganizationUserRemovedForPolicyTwoStepEmailAsync(organization.DisplayName(), orgUserDetailAdmin.Email); await sutProvider.GetDependency().Received() .LogPolicyEventAsync(policy, EventType.Policy_Updated); @@ -341,6 +390,99 @@ public class PolicyServiceTests Assert.True(policy.RevisionDate - utcNow < TimeSpan.FromSeconds(1)); } + [Theory, BitAutoData] + public async Task SaveAsync_EnableTwoFactor_WithoutMasterPasswordOr2FA_ThrowsBadRequest( + Organization organization, + [AdminConsoleFixtures.Policy(PolicyType.TwoFactorAuthentication)] Policy policy, + SutProvider sutProvider) + { + organization.UsePolicies = true; + policy.OrganizationId = organization.Id; + + SetupOrg(sutProvider, organization.Id, organization); + + var orgUserDetailUserWith2FAAndMP = 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 = "user1@test.com", + Name = "TEST", + UserId = Guid.NewGuid(), + HasMasterPassword = true + }; + var orgUserDetailUserWith2FANoMP = 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 = "user2@test.com", + Name = "TEST", + UserId = Guid.NewGuid(), + HasMasterPassword = false + }; + 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 + }; + var orgUserDetailAdmin = new OrganizationUserUserDetails + { + Id = Guid.NewGuid(), + Status = OrganizationUserStatusType.Confirmed, + Type = OrganizationUserType.Admin, + // Needs to be different from what is passed in as the savingUserId to Sut.SaveAsync + Email = "admin@test.com", + Name = "ADMIN", + UserId = Guid.NewGuid(), + HasMasterPassword = false + }; + + sutProvider.GetDependency() + .GetManyDetailsByOrganizationAsync(policy.OrganizationId) + .Returns(new List + { + orgUserDetailUserWith2FAAndMP, + orgUserDetailUserWith2FANoMP, + orgUserDetailUserWithout2FA, + orgUserDetailAdmin + }); + + var userService = Substitute.For(); + var organizationService = Substitute.For(); + + userService.TwoFactorIsEnabledAsync(orgUserDetailUserWith2FANoMP).Returns(true); + userService.TwoFactorIsEnabledAsync(orgUserDetailUserWithout2FA).Returns(false); + userService.TwoFactorIsEnabledAsync(orgUserDetailAdmin).Returns(false); + + var savingUserId = Guid.NewGuid(); + + var badRequestException = await Assert.ThrowsAsync( + () => sutProvider.Sut.SaveAsync(policy, userService, organizationService, savingUserId)); + + 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); + + await organizationService.DidNotReceiveWithAnyArgs() + .DeleteUserAsync(organizationId: default, organizationUserId: default, deletingUserId: default); + + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .SendOrganizationUserRemovedForPolicyTwoStepEmailAsync(default, default); + + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .LogPolicyEventAsync(default, default); + + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .UpsertAsync(default); + } + [Theory, BitAutoData] public async Task SaveAsync_ExistingPolicy_UpdateSingleOrg( [AdminConsoleFixtures.Policy(PolicyType.TwoFactorAuthentication)] Policy policy, SutProvider sutProvider) @@ -374,6 +516,7 @@ public class PolicyServiceTests Email = "test@bitwarden.com", Name = "TEST", UserId = Guid.NewGuid(), + HasMasterPassword = true }; sutProvider.GetDependency()