From febc696c804b58e8e9ac690e9a139451238d590b Mon Sep 17 00:00:00 2001 From: Vincent Salucci <26154748+vincentsalucci@users.noreply.github.com> Date: Fri, 17 May 2024 14:28:51 -0500 Subject: [PATCH] [AC-240] - BUG - Confirm Admin/Owners to org when excluded from Single Org Policy (#4087) * fix: align policy checks for excluded types, update tests, create fixture, refs AC-240 * fix: update final policy check against other orgs (not including the current), refs AC-240 --- .../Implementations/OrganizationService.cs | 28 +++-- .../OrganizationUserPolicyDetailsFixtures.cs | 40 +++++++ .../Services/OrganizationServiceTests.cs | 108 ++++++++++++++---- 3 files changed, 145 insertions(+), 31 deletions(-) create mode 100644 test/Core.Test/AdminConsole/AutoFixture/OrganizationUserPolicyDetailsFixtures.cs diff --git a/src/Core/AdminConsole/Services/Implementations/OrganizationService.cs b/src/Core/AdminConsole/Services/Implementations/OrganizationService.cs index dca45ceba..b4a243d70 100644 --- a/src/Core/AdminConsole/Services/Implementations/OrganizationService.cs +++ b/src/Core/AdminConsole/Services/Implementations/OrganizationService.cs @@ -1323,7 +1323,6 @@ public class OrganizationService : IOrganizationService var validOrganizationUserIds = validOrganizationUsers.Select(u => u.UserId.Value).ToList(); var organization = await GetOrgById(organizationId); - var policies = await _policyRepository.GetManyByOrganizationIdAsync(organizationId); var usersOrgs = await _organizationUserRepository.GetManyByManyUsersAsync(validOrganizationUserIds); var users = await _userRepository.GetManyAsync(validOrganizationUserIds); @@ -1355,7 +1354,7 @@ public class OrganizationService : IOrganizationService } } - await CheckPolicies(policies, organizationId, user, orgUsers, userService); + await CheckPolicies(organizationId, user, orgUsers, userService); orgUser.Status = OrganizationUserStatusType.Confirmed; orgUser.Key = keys[orgUser.Id]; orgUser.Email = null; @@ -1449,22 +1448,29 @@ public class OrganizationService : IOrganizationService } } - private async Task CheckPolicies(ICollection policies, Guid organizationId, User user, + private async Task CheckPolicies(Guid organizationId, User user, ICollection userOrgs, IUserService userService) { - var usingTwoFactorPolicy = policies.Any(p => p.Type == PolicyType.TwoFactorAuthentication && p.Enabled); - if (usingTwoFactorPolicy && !await userService.TwoFactorIsEnabledAsync(user)) + // Enforce Two Factor Authentication Policy for this organization + var orgRequiresTwoFactor = (await _policyService.GetPoliciesApplicableToUserAsync(user.Id, PolicyType.TwoFactorAuthentication)).Any(p => p.OrganizationId == organizationId); + if (orgRequiresTwoFactor && !await userService.TwoFactorIsEnabledAsync(user)) { throw new BadRequestException("User does not have two-step login enabled."); } - var usingSingleOrgPolicy = policies.Any(p => p.Type == PolicyType.SingleOrg && p.Enabled); - if (usingSingleOrgPolicy) + var hasOtherOrgs = userOrgs.Any(ou => ou.OrganizationId != organizationId); + var singleOrgPolicies = await _policyService.GetPoliciesApplicableToUserAsync(user.Id, PolicyType.SingleOrg); + var otherSingleOrgPolicies = + singleOrgPolicies.Where(p => p.OrganizationId != organizationId); + // Enforce Single Organization Policy for this organization + if (hasOtherOrgs && singleOrgPolicies.Any(p => p.OrganizationId == organizationId)) { - if (userOrgs.Any(ou => ou.OrganizationId != organizationId && ou.Status != OrganizationUserStatusType.Invited)) - { - throw new BadRequestException("User is a member of another organization."); - } + throw new BadRequestException("Cannot confirm this member to the organization until they leave or remove all other organizations."); + } + // Enforce Single Organization Policy of other organizations user is a member of + if (otherSingleOrgPolicies.Any()) + { + throw new BadRequestException("Cannot confirm this member to the organization because they are in another organization which forbids it."); } } diff --git a/test/Core.Test/AdminConsole/AutoFixture/OrganizationUserPolicyDetailsFixtures.cs b/test/Core.Test/AdminConsole/AutoFixture/OrganizationUserPolicyDetailsFixtures.cs new file mode 100644 index 000000000..634b234e7 --- /dev/null +++ b/test/Core.Test/AdminConsole/AutoFixture/OrganizationUserPolicyDetailsFixtures.cs @@ -0,0 +1,40 @@ +using System.Reflection; +using AutoFixture; +using AutoFixture.Xunit2; +using Bit.Core.AdminConsole.Enums; +using Bit.Core.Models.Data.Organizations.OrganizationUsers; + +namespace Bit.Core.Test.AdminConsole.AutoFixture; + +internal class OrganizationUserPolicyDetailsCustomization : ICustomization +{ + public PolicyType Type { get; set; } + + public OrganizationUserPolicyDetailsCustomization(PolicyType type) + { + Type = type; + } + + public void Customize(IFixture fixture) + { + fixture.Customize(composer => composer + .With(o => o.OrganizationId, Guid.NewGuid()) + .With(o => o.PolicyType, Type) + .With(o => o.PolicyEnabled, true)); + } +} + +public class OrganizationUserPolicyDetailsAttribute : CustomizeAttribute +{ + private readonly PolicyType _type; + + public OrganizationUserPolicyDetailsAttribute(PolicyType type) + { + _type = type; + } + + public override ICustomization GetCustomization(ParameterInfo parameter) + { + return new OrganizationUserPolicyDetailsCustomization(_type); + } +} diff --git a/test/Core.Test/AdminConsole/Services/OrganizationServiceTests.cs b/test/Core.Test/AdminConsole/Services/OrganizationServiceTests.cs index fa2090d37..db6805c09 100644 --- a/test/Core.Test/AdminConsole/Services/OrganizationServiceTests.cs +++ b/test/Core.Test/AdminConsole/Services/OrganizationServiceTests.cs @@ -4,6 +4,7 @@ using Bit.Core.AdminConsole.Enums; using Bit.Core.AdminConsole.Enums.Provider; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; using Bit.Core.AdminConsole.Repositories; +using Bit.Core.AdminConsole.Services; using Bit.Core.Auth.Entities; using Bit.Core.Auth.Enums; using Bit.Core.Auth.Models.Business.Tokenables; @@ -39,7 +40,6 @@ using NSubstitute.ReturnsExtensions; using Xunit; using Organization = Bit.Core.AdminConsole.Entities.Organization; using OrganizationUser = Bit.Core.Entities.OrganizationUser; -using Policy = Bit.Core.AdminConsole.Entities.Policy; namespace Bit.Core.Test.Services; @@ -1441,15 +1441,15 @@ OrganizationUserInvite invite, SutProvider sutProvider) [Theory, BitAutoData] - public async Task ConfirmUser_SingleOrgPolicy(Organization org, OrganizationUser confirmingUser, + public async Task ConfirmUser_AsUser_SingleOrgPolicy_AppliedFromConfirmingOrg_Throws(Organization org, OrganizationUser confirmingUser, [OrganizationUser(OrganizationUserStatusType.Accepted)] OrganizationUser orgUser, User user, - OrganizationUser orgUserAnotherOrg, [Policy(PolicyType.SingleOrg)] Policy singleOrgPolicy, + OrganizationUser orgUserAnotherOrg, [OrganizationUserPolicyDetails(PolicyType.SingleOrg)] OrganizationUserPolicyDetails singleOrgPolicy, string key, SutProvider sutProvider) { var organizationUserRepository = sutProvider.GetDependency(); var organizationRepository = sutProvider.GetDependency(); - var policyRepository = sutProvider.GetDependency(); var userRepository = sutProvider.GetDependency(); + var policyService = sutProvider.GetDependency(); var userService = Substitute.For(); org.PlanType = PlanType.EnterpriseAnnually; @@ -1460,23 +1460,84 @@ OrganizationUserInvite invite, SutProvider sutProvider) organizationUserRepository.GetManyByManyUsersAsync(default).ReturnsForAnyArgs(new[] { orgUserAnotherOrg }); organizationRepository.GetByIdAsync(org.Id).Returns(org); userRepository.GetManyAsync(default).ReturnsForAnyArgs(new[] { user }); - policyRepository.GetManyByOrganizationIdAsync(org.Id).Returns(new[] { singleOrgPolicy }); + singleOrgPolicy.OrganizationId = org.Id; + policyService.GetPoliciesApplicableToUserAsync(user.Id, PolicyType.SingleOrg).Returns(new[] { singleOrgPolicy }); var exception = await Assert.ThrowsAsync( () => sutProvider.Sut.ConfirmUserAsync(orgUser.OrganizationId, orgUser.Id, key, confirmingUser.Id, userService)); - Assert.Contains("User is a member of another organization.", exception.Message); + Assert.Contains("Cannot confirm this member to the organization until they leave or remove all other organizations.", exception.Message); } [Theory, BitAutoData] - public async Task ConfirmUser_TwoFactorPolicy(Organization org, OrganizationUser confirmingUser, + public async Task ConfirmUser_AsUser_SingleOrgPolicy_AppliedFromOtherOrg_Throws(Organization org, OrganizationUser confirmingUser, [OrganizationUser(OrganizationUserStatusType.Accepted)] OrganizationUser orgUser, User user, - OrganizationUser orgUserAnotherOrg, [Policy(PolicyType.TwoFactorAuthentication)] Policy twoFactorPolicy, + OrganizationUser orgUserAnotherOrg, [OrganizationUserPolicyDetails(PolicyType.SingleOrg)] OrganizationUserPolicyDetails singleOrgPolicy, string key, SutProvider sutProvider) { var organizationUserRepository = sutProvider.GetDependency(); var organizationRepository = sutProvider.GetDependency(); - var policyRepository = sutProvider.GetDependency(); var userRepository = sutProvider.GetDependency(); + var policyService = sutProvider.GetDependency(); + var userService = Substitute.For(); + + org.PlanType = PlanType.EnterpriseAnnually; + orgUser.Status = OrganizationUserStatusType.Accepted; + orgUser.OrganizationId = confirmingUser.OrganizationId = org.Id; + orgUser.UserId = orgUserAnotherOrg.UserId = user.Id; + organizationUserRepository.GetManyAsync(default).ReturnsForAnyArgs(new[] { orgUser }); + organizationUserRepository.GetManyByManyUsersAsync(default).ReturnsForAnyArgs(new[] { orgUserAnotherOrg }); + organizationRepository.GetByIdAsync(org.Id).Returns(org); + userRepository.GetManyAsync(default).ReturnsForAnyArgs(new[] { user }); + singleOrgPolicy.OrganizationId = orgUserAnotherOrg.Id; + policyService.GetPoliciesApplicableToUserAsync(user.Id, PolicyType.SingleOrg).Returns(new[] { singleOrgPolicy }); + + var exception = await Assert.ThrowsAsync( + () => sutProvider.Sut.ConfirmUserAsync(orgUser.OrganizationId, orgUser.Id, key, confirmingUser.Id, userService)); + Assert.Contains("Cannot confirm this member to the organization because they are in another organization which forbids it.", exception.Message); + } + + [Theory] + [BitAutoData(OrganizationUserType.Admin)] + [BitAutoData(OrganizationUserType.Owner)] + public async Task ConfirmUser_AsOwnerOrAdmin_SingleOrgPolicy_ExcludedViaUserType_Success( + OrganizationUserType userType, Organization org, OrganizationUser confirmingUser, + [OrganizationUser(OrganizationUserStatusType.Accepted)] OrganizationUser orgUser, User user, + OrganizationUser orgUserAnotherOrg, + string key, SutProvider sutProvider) + { + var organizationUserRepository = sutProvider.GetDependency(); + var organizationRepository = sutProvider.GetDependency(); + var userRepository = sutProvider.GetDependency(); + var userService = Substitute.For(); + + org.PlanType = PlanType.EnterpriseAnnually; + orgUser.Type = userType; + orgUser.Status = OrganizationUserStatusType.Accepted; + orgUser.OrganizationId = confirmingUser.OrganizationId = org.Id; + orgUser.UserId = orgUserAnotherOrg.UserId = user.Id; + organizationUserRepository.GetManyAsync(default).ReturnsForAnyArgs(new[] { orgUser }); + organizationUserRepository.GetManyByManyUsersAsync(default).ReturnsForAnyArgs(new[] { orgUserAnotherOrg }); + organizationRepository.GetByIdAsync(org.Id).Returns(org); + userRepository.GetManyAsync(default).ReturnsForAnyArgs(new[] { user }); + + await sutProvider.Sut.ConfirmUserAsync(orgUser.OrganizationId, orgUser.Id, key, confirmingUser.Id, userService); + + await sutProvider.GetDependency().Received(1).LogOrganizationUserEventAsync(orgUser, EventType.OrganizationUser_Confirmed); + await sutProvider.GetDependency().Received(1).SendOrganizationConfirmedEmailAsync(org.DisplayName(), user.Email); + await organizationUserRepository.Received(1).ReplaceManyAsync(Arg.Is>(users => users.Contains(orgUser) && users.Count == 1)); + } + + [Theory, BitAutoData] + public async Task ConfirmUser_TwoFactorPolicy_NotEnabled_Throws(Organization org, OrganizationUser confirmingUser, + [OrganizationUser(OrganizationUserStatusType.Accepted)] OrganizationUser orgUser, User user, + OrganizationUser orgUserAnotherOrg, + [OrganizationUserPolicyDetails(PolicyType.TwoFactorAuthentication)] OrganizationUserPolicyDetails twoFactorPolicy, + string key, SutProvider sutProvider) + { + var organizationUserRepository = sutProvider.GetDependency(); + var organizationRepository = sutProvider.GetDependency(); + var userRepository = sutProvider.GetDependency(); + var policyService = sutProvider.GetDependency(); var userService = Substitute.For(); org.PlanType = PlanType.EnterpriseAnnually; @@ -1486,7 +1547,8 @@ OrganizationUserInvite invite, SutProvider sutProvider) organizationUserRepository.GetManyByManyUsersAsync(default).ReturnsForAnyArgs(new[] { orgUserAnotherOrg }); organizationRepository.GetByIdAsync(org.Id).Returns(org); userRepository.GetManyAsync(default).ReturnsForAnyArgs(new[] { user }); - policyRepository.GetManyByOrganizationIdAsync(org.Id).Returns(new[] { twoFactorPolicy }); + twoFactorPolicy.OrganizationId = org.Id; + policyService.GetPoliciesApplicableToUserAsync(user.Id, PolicyType.TwoFactorAuthentication).Returns(new[] { twoFactorPolicy }); var exception = await Assert.ThrowsAsync( () => sutProvider.Sut.ConfirmUserAsync(orgUser.OrganizationId, orgUser.Id, key, confirmingUser.Id, userService)); @@ -1494,15 +1556,15 @@ OrganizationUserInvite invite, SutProvider sutProvider) } [Theory, BitAutoData] - public async Task ConfirmUser_Success(Organization org, OrganizationUser confirmingUser, + public async Task ConfirmUser_TwoFactorPolicy_Enabled_Success(Organization org, OrganizationUser confirmingUser, [OrganizationUser(OrganizationUserStatusType.Accepted)] OrganizationUser orgUser, User user, - [Policy(PolicyType.TwoFactorAuthentication)] Policy twoFactorPolicy, - [Policy(PolicyType.SingleOrg)] Policy singleOrgPolicy, string key, SutProvider sutProvider) + [OrganizationUserPolicyDetails(PolicyType.TwoFactorAuthentication)] OrganizationUserPolicyDetails twoFactorPolicy, + string key, SutProvider sutProvider) { var organizationUserRepository = sutProvider.GetDependency(); var organizationRepository = sutProvider.GetDependency(); - var policyRepository = sutProvider.GetDependency(); var userRepository = sutProvider.GetDependency(); + var policyService = sutProvider.GetDependency(); var userService = Substitute.For(); org.PlanType = PlanType.EnterpriseAnnually; @@ -1511,7 +1573,8 @@ OrganizationUserInvite invite, SutProvider sutProvider) organizationUserRepository.GetManyAsync(default).ReturnsForAnyArgs(new[] { orgUser }); organizationRepository.GetByIdAsync(org.Id).Returns(org); userRepository.GetManyAsync(default).ReturnsForAnyArgs(new[] { user }); - policyRepository.GetManyByOrganizationIdAsync(org.Id).Returns(new[] { twoFactorPolicy, singleOrgPolicy }); + twoFactorPolicy.OrganizationId = org.Id; + policyService.GetPoliciesApplicableToUserAsync(user.Id, PolicyType.TwoFactorAuthentication).Returns(new[] { twoFactorPolicy }); userService.TwoFactorIsEnabledAsync(user).Returns(true); await sutProvider.Sut.ConfirmUserAsync(orgUser.OrganizationId, orgUser.Id, key, confirmingUser.Id, userService); @@ -1524,13 +1587,14 @@ OrganizationUserInvite invite, SutProvider sutProvider) [OrganizationUser(OrganizationUserStatusType.Accepted)] OrganizationUser orgUser2, [OrganizationUser(OrganizationUserStatusType.Accepted)] OrganizationUser orgUser3, OrganizationUser anotherOrgUser, User user1, User user2, User user3, - [Policy(PolicyType.TwoFactorAuthentication)] Policy twoFactorPolicy, - [Policy(PolicyType.SingleOrg)] Policy singleOrgPolicy, string key, SutProvider sutProvider) + [OrganizationUserPolicyDetails(PolicyType.TwoFactorAuthentication)] OrganizationUserPolicyDetails twoFactorPolicy, + [OrganizationUserPolicyDetails(PolicyType.SingleOrg)] OrganizationUserPolicyDetails singleOrgPolicy, + string key, SutProvider sutProvider) { var organizationUserRepository = sutProvider.GetDependency(); var organizationRepository = sutProvider.GetDependency(); - var policyRepository = sutProvider.GetDependency(); var userRepository = sutProvider.GetDependency(); + var policyService = sutProvider.GetDependency(); var userService = Substitute.For(); org.PlanType = PlanType.EnterpriseAnnually; @@ -1543,10 +1607,14 @@ OrganizationUserInvite invite, SutProvider sutProvider) organizationUserRepository.GetManyAsync(default).ReturnsForAnyArgs(orgUsers); organizationRepository.GetByIdAsync(org.Id).Returns(org); userRepository.GetManyAsync(default).ReturnsForAnyArgs(new[] { user1, user2, user3 }); - policyRepository.GetManyByOrganizationIdAsync(org.Id).Returns(new[] { twoFactorPolicy, singleOrgPolicy }); + twoFactorPolicy.OrganizationId = org.Id; + policyService.GetPoliciesApplicableToUserAsync(Arg.Any(), PolicyType.TwoFactorAuthentication).Returns(new[] { twoFactorPolicy }); userService.TwoFactorIsEnabledAsync(user1).Returns(true); userService.TwoFactorIsEnabledAsync(user2).Returns(false); userService.TwoFactorIsEnabledAsync(user3).Returns(true); + singleOrgPolicy.OrganizationId = org.Id; + policyService.GetPoliciesApplicableToUserAsync(user3.Id, PolicyType.SingleOrg) + .Returns(new[] { singleOrgPolicy }); organizationUserRepository.GetManyByManyUsersAsync(default) .ReturnsForAnyArgs(new[] { orgUser1, orgUser2, orgUser3, anotherOrgUser }); @@ -1554,7 +1622,7 @@ OrganizationUserInvite invite, SutProvider sutProvider) var result = await sutProvider.Sut.ConfirmUsersAsync(confirmingUser.OrganizationId, keys, confirmingUser.Id, userService); Assert.Contains("", result[0].Item2); Assert.Contains("User does not have two-step login enabled.", result[1].Item2); - Assert.Contains("User is a member of another organization.", result[2].Item2); + Assert.Contains("Cannot confirm this member to the organization until they leave or remove all other organizations.", result[2].Item2); } [Theory, BitAutoData]