mirror of
https://github.com/bitwarden/server.git
synced 2024-11-21 12:05:42 +01:00
[PM-6934] Prevent enabling two step login policy if any Org member has no master password and no 2FA set up (#3915)
* [PM-6934] Prevent enabling two step login policy if any Org member has no master password and no 2FA set up * [PM-6934] PR feedback * [PM-6934] Updated policy check to only check users that will be deleted * [PM-6934] Removed unnecessary code * [PM-6934] Fixed unit tests and policy update logic * [PM-6934] Updated error message
This commit is contained in:
Rui Tomé
GitHub
parent
78ce1f8a5d
commit
366eef7e23
@ -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(
|
||||
|
||||
@ -273,18 +273,16 @@ public class PolicyServiceTests
|
||||
|
||||
[Theory, BitAutoData]
|
||||
public async Task SaveAsync_ExistingPolicy_UpdateTwoFactor(
|
||||
[AdminConsoleFixtures.Policy(PolicyType.TwoFactorAuthentication)] Policy policy, SutProvider<PolicyService> sutProvider)
|
||||
Organization organization,
|
||||
[AdminConsoleFixtures.Policy(PolicyType.TwoFactorAuthentication)] Policy policy,
|
||||
SutProvider<PolicyService> 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<IPolicyRepository>()
|
||||
.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<IOrganizationUserRepository>()
|
||||
.GetManyDetailsByOrganizationAsync(policy.OrganizationId)
|
||||
.Returns(new List<Core.Models.Data.Organizations.OrganizationUsers.OrganizationUserUserDetails>
|
||||
.Returns(new List<OrganizationUserUserDetails>
|
||||
{
|
||||
orgUserDetail,
|
||||
orgUserDetailUserInvited,
|
||||
orgUserDetailUserAcceptedWith2FA,
|
||||
orgUserDetailUserAcceptedWithout2FA,
|
||||
orgUserDetailAdmin
|
||||
});
|
||||
|
||||
var userService = Substitute.For<IUserService>();
|
||||
var organizationService = Substitute.For<IOrganizationService>();
|
||||
|
||||
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<IMailService>().Received()
|
||||
.SendOrganizationUserRemovedForPolicyTwoStepEmailAsync(org.DisplayName(), orgUserDetail.Email);
|
||||
.SendOrganizationUserRemovedForPolicyTwoStepEmailAsync(organization.DisplayName(), orgUserDetailUserAcceptedWithout2FA.Email);
|
||||
|
||||
await organizationService.DidNotReceive()
|
||||
.DeleteUserAsync(policy.OrganizationId, orgUserDetailUserInvited.Id, savingUserId);
|
||||
await sutProvider.GetDependency<IMailService>().DidNotReceive()
|
||||
.SendOrganizationUserRemovedForPolicyTwoStepEmailAsync(organization.DisplayName(), orgUserDetailUserInvited.Email);
|
||||
await organizationService.DidNotReceive()
|
||||
.DeleteUserAsync(policy.OrganizationId, orgUserDetailUserAcceptedWith2FA.Id, savingUserId);
|
||||
await sutProvider.GetDependency<IMailService>().DidNotReceive()
|
||||
.SendOrganizationUserRemovedForPolicyTwoStepEmailAsync(organization.DisplayName(), orgUserDetailUserAcceptedWith2FA.Email);
|
||||
await organizationService.DidNotReceive()
|
||||
.DeleteUserAsync(policy.OrganizationId, orgUserDetailAdmin.Id, savingUserId);
|
||||
await sutProvider.GetDependency<IMailService>().DidNotReceive()
|
||||
.SendOrganizationUserRemovedForPolicyTwoStepEmailAsync(organization.DisplayName(), orgUserDetailAdmin.Email);
|
||||
|
||||
await sutProvider.GetDependency<IEventService>().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<PolicyService> 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<IOrganizationUserRepository>()
|
||||
.GetManyDetailsByOrganizationAsync(policy.OrganizationId)
|
||||
.Returns(new List<OrganizationUserUserDetails>
|
||||
{
|
||||
orgUserDetailUserWith2FAAndMP,
|
||||
orgUserDetailUserWith2FANoMP,
|
||||
orgUserDetailUserWithout2FA,
|
||||
orgUserDetailAdmin
|
||||
});
|
||||
|
||||
var userService = Substitute.For<IUserService>();
|
||||
var organizationService = Substitute.For<IOrganizationService>();
|
||||
|
||||
userService.TwoFactorIsEnabledAsync(orgUserDetailUserWith2FANoMP).Returns(true);
|
||||
userService.TwoFactorIsEnabledAsync(orgUserDetailUserWithout2FA).Returns(false);
|
||||
userService.TwoFactorIsEnabledAsync(orgUserDetailAdmin).Returns(false);
|
||||
|
||||
var savingUserId = Guid.NewGuid();
|
||||
|
||||
var badRequestException = await Assert.ThrowsAsync<BadRequestException>(
|
||||
() => 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<IMailService>().DidNotReceiveWithAnyArgs()
|
||||
.SendOrganizationUserRemovedForPolicyTwoStepEmailAsync(default, default);
|
||||
|
||||
await sutProvider.GetDependency<IEventService>().DidNotReceiveWithAnyArgs()
|
||||
.LogPolicyEventAsync(default, default);
|
||||
|
||||
await sutProvider.GetDependency<IPolicyRepository>().DidNotReceiveWithAnyArgs()
|
||||
.UpsertAsync(default);
|
||||
}
|
||||
|
||||
[Theory, BitAutoData]
|
||||
public async Task SaveAsync_ExistingPolicy_UpdateSingleOrg(
|
||||
[AdminConsoleFixtures.Policy(PolicyType.TwoFactorAuthentication)] Policy policy, SutProvider<PolicyService> sutProvider)
|
||||
@ -374,6 +516,7 @@ public class PolicyServiceTests
|
||||
Email = "test@bitwarden.com",
|
||||
Name = "TEST",
|
||||
UserId = Guid.NewGuid(),
|
||||
HasMasterPassword = true
|
||||
};
|
||||
|
||||
sutProvider.GetDependency<IOrganizationUserRepository>()
|
||||
|
||||
Loading…
Reference in New Issue
Block a user