From bcffef57d820f70a2e80b90bde007865179b2384 Mon Sep 17 00:00:00 2001 From: Jimmy Vo Date: Mon, 18 Nov 2024 15:52:11 -0500 Subject: [PATCH] [PM-14826] Add UsePolicies check to GET endpoints GetByToken and GetMasterPasswordPolicy endpoints provide policy information, so if the organization is not using policies, then we avoid the rest of the logic. --- .../Controllers/PoliciesController.cs | 91 ++++++----------- .../Controllers/PoliciesControllerTests.cs | 97 ++++++++++++++++--- 2 files changed, 116 insertions(+), 72 deletions(-) diff --git a/src/Api/AdminConsole/Controllers/PoliciesController.cs b/src/Api/AdminConsole/Controllers/PoliciesController.cs index ee48cdd5d..a0743e703 100644 --- a/src/Api/AdminConsole/Controllers/PoliciesController.cs +++ b/src/Api/AdminConsole/Controllers/PoliciesController.cs @@ -27,19 +27,19 @@ namespace Bit.Api.AdminConsole.Controllers; [Authorize("Application")] public class PoliciesController : Controller { + private readonly ICurrentContext _currentContext; + private readonly IFeatureService _featureService; + private readonly GlobalSettings _globalSettings; + private readonly IOrganizationHasVerifiedDomainsQuery _organizationHasVerifiedDomainsQuery; + private readonly IOrganizationRepository _organizationRepository; + private readonly IDataProtector _organizationServiceDataProtector; + private readonly IOrganizationUserRepository _organizationUserRepository; + private readonly IDataProtectorTokenFactory _orgUserInviteTokenDataFactory; private readonly IPolicyRepository _policyRepository; private readonly IPolicyService _policyService; - private readonly IOrganizationUserRepository _organizationUserRepository; private readonly IUserService _userService; - private readonly ICurrentContext _currentContext; - private readonly GlobalSettings _globalSettings; - private readonly IDataProtector _organizationServiceDataProtector; - private readonly IDataProtectorTokenFactory _orgUserInviteTokenDataFactory; - private readonly IFeatureService _featureService; - private readonly IOrganizationHasVerifiedDomainsQuery _organizationHasVerifiedDomainsQuery; - public PoliciesController( - IPolicyRepository policyRepository, + public PoliciesController(IPolicyRepository policyRepository, IPolicyService policyService, IOrganizationUserRepository organizationUserRepository, IUserService userService, @@ -48,7 +48,8 @@ public class PoliciesController : Controller IDataProtectionProvider dataProtectionProvider, IDataProtectorTokenFactory orgUserInviteTokenDataFactory, IFeatureService featureService, - IOrganizationHasVerifiedDomainsQuery organizationHasVerifiedDomainsQuery) + IOrganizationHasVerifiedDomainsQuery organizationHasVerifiedDomainsQuery, + IOrganizationRepository organizationRepository) { _policyRepository = policyRepository; _policyService = policyService; @@ -62,25 +63,18 @@ public class PoliciesController : Controller _orgUserInviteTokenDataFactory = orgUserInviteTokenDataFactory; _featureService = featureService; _organizationHasVerifiedDomainsQuery = organizationHasVerifiedDomainsQuery; + _organizationRepository = organizationRepository; } [HttpGet("{type}")] public async Task Get(Guid orgId, int type) { - if (!await _currentContext.ManagePolicies(orgId)) - { - throw new NotFoundException(); - } + if (!await _currentContext.ManagePolicies(orgId)) throw new NotFoundException(); var policy = await _policyRepository.GetByOrganizationIdTypeAsync(orgId, (PolicyType)type); - if (policy == null) - { - return new PolicyDetailResponseModel(new Policy { Type = (PolicyType)type }); - } + if (policy == null) return new PolicyDetailResponseModel(new Policy { Type = (PolicyType)type }); if (_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning) && policy.Type is PolicyType.SingleOrg) - { return await policy.GetSingleOrgPolicyDetailResponseAsync(_organizationHasVerifiedDomainsQuery); - } return new PolicyDetailResponseModel(policy); } @@ -89,10 +83,7 @@ public class PoliciesController : Controller public async Task> Get(string orgId) { var orgIdGuid = new Guid(orgId); - if (!await _currentContext.ManagePolicies(orgIdGuid)) - { - throw new NotFoundException(); - } + if (!await _currentContext.ManagePolicies(orgIdGuid)) throw new NotFoundException(); var policies = await _policyRepository.GetManyByOrganizationIdAsync(orgIdGuid); @@ -104,6 +95,10 @@ public class PoliciesController : Controller public async Task> GetByToken(Guid orgId, [FromQuery] string email, [FromQuery] string token, [FromQuery] Guid organizationUserId) { + var organization = await _organizationRepository.GetByIdAsync(orgId); + + if (organization is not { UsePolicies: true }) throw new NotFoundException(); + // TODO: PM-4142 - remove old token validation logic once 3 releases of backwards compatibility are complete var newTokenValid = OrgUserInviteTokenable.ValidateOrgUserInviteStringToken( _orgUserInviteTokenDataFactory, token, organizationUserId, email); @@ -112,16 +107,10 @@ public class PoliciesController : Controller _organizationServiceDataProtector, token, email, organizationUserId, _globalSettings ); - if (!tokenValid) - { - throw new NotFoundException(); - } + if (!tokenValid) throw new NotFoundException(); var orgUser = await _organizationUserRepository.GetByIdAsync(organizationUserId); - if (orgUser == null || orgUser.OrganizationId != orgId) - { - throw new NotFoundException(); - } + if (orgUser == null || orgUser.OrganizationId != orgId) throw new NotFoundException(); var policies = await _policyRepository.GetManyByOrganizationIdAsync(orgId); var responses = policies.Where(p => p.Enabled).Select(p => new PolicyResponseModel(p)); @@ -135,20 +124,11 @@ public class PoliciesController : Controller public async Task> GetByInvitedUser(Guid orgId, [FromQuery] Guid userId) { var user = await _userService.GetUserByIdAsync(userId); - if (user == null) - { - throw new UnauthorizedAccessException(); - } + if (user == null) throw new UnauthorizedAccessException(); var orgUsersByUserId = await _organizationUserRepository.GetManyByUserAsync(user.Id); var orgUser = orgUsersByUserId.SingleOrDefault(u => u.OrganizationId == orgId); - if (orgUser == null) - { - throw new NotFoundException(); - } - if (orgUser.Status != OrganizationUserStatusType.Invited) - { - throw new UnauthorizedAccessException(); - } + if (orgUser == null) throw new NotFoundException(); + if (orgUser.Status != OrganizationUserStatusType.Invited) throw new UnauthorizedAccessException(); var policies = await _policyRepository.GetManyByOrganizationIdAsync(orgId); var responses = policies.Where(p => p.Enabled).Select(p => new PolicyResponseModel(p)); @@ -158,21 +138,19 @@ public class PoliciesController : Controller [HttpGet("master-password")] public async Task GetMasterPasswordPolicy(Guid orgId) { + var organization = await _organizationRepository.GetByIdAsync(orgId); + + if (organization is not { UsePolicies: true }) throw new NotFoundException(); + var userId = _userService.GetProperUserId(User).Value; var orgUser = await _organizationUserRepository.GetByOrganizationAsync(orgId, userId); - if (orgUser == null) - { - throw new NotFoundException(); - } + if (orgUser == null) throw new NotFoundException(); var policy = await _policyRepository.GetByOrganizationIdTypeAsync(orgId, PolicyType.MasterPassword); - if (policy == null || !policy.Enabled) - { - throw new NotFoundException(); - } + if (policy == null || !policy.Enabled) throw new NotFoundException(); return new PolicyResponseModel(policy); } @@ -181,19 +159,12 @@ public class PoliciesController : Controller public async Task Put(string orgId, int type, [FromBody] PolicyRequestModel model) { var orgIdGuid = new Guid(orgId); - if (!await _currentContext.ManagePolicies(orgIdGuid)) - { - throw new NotFoundException(); - } + if (!await _currentContext.ManagePolicies(orgIdGuid)) throw new NotFoundException(); var policy = await _policyRepository.GetByOrganizationIdTypeAsync(new Guid(orgId), (PolicyType)type); if (policy == null) - { policy = model.ToPolicy(orgIdGuid); - } else - { policy = model.ToPolicy(policy); - } var userId = _userService.GetProperUserId(User); await _policyService.SaveAsync(policy, userId); diff --git a/test/Api.Test/Controllers/PoliciesControllerTests.cs b/test/Api.Test/Controllers/PoliciesControllerTests.cs index 1b96ace5d..08316a98c 100644 --- a/test/Api.Test/Controllers/PoliciesControllerTests.cs +++ b/test/Api.Test/Controllers/PoliciesControllerTests.cs @@ -28,10 +28,19 @@ public class PoliciesControllerTests [Theory] [BitAutoData] public async Task GetMasterPasswordPolicy_WhenCalled_ReturnsMasterPasswordPolicy( - SutProvider sutProvider, Guid orgId, Guid userId, OrganizationUser orgUser, - Policy policy, MasterPasswordPolicyData mpPolicyData) + SutProvider sutProvider, + Guid orgId, Guid userId, + OrganizationUser orgUser, + Policy policy, + MasterPasswordPolicyData mpPolicyData, + Organization organization) { // Arrange + organization.UsePolicies = true; + + var organizationRepository = sutProvider.GetDependency(); + organizationRepository.GetByIdAsync(orgId).Returns(organization); + sutProvider.GetDependency() .GetProperUserId(Arg.Any()) .Returns((Guid?)userId); @@ -135,6 +144,39 @@ public class PoliciesControllerTests await Assert.ThrowsAsync(() => sutProvider.Sut.GetMasterPasswordPolicy(orgId)); } + [Theory] + [BitAutoData] + public async Task GetMasterPasswordPolicy_WhenUsePoliciesIsFalse_ThrowsNotFoundException( + SutProvider sutProvider, + Guid orgId) + { + // Arrange + var organizationRepository = sutProvider.GetDependency(); + organizationRepository.GetByIdAsync(orgId).Returns((Organization)null); + + + // Act & Assert + await Assert.ThrowsAsync(() => sutProvider.Sut.GetMasterPasswordPolicy(orgId)); + } + + [Theory] + [BitAutoData] + public async Task GetMasterPasswordPolicy_WhenOrgIsNull_ThrowsNotFoundException( + SutProvider sutProvider, + Guid orgId, + Organization organization) + { + // Arrange + organization.UsePolicies = false; + + var organizationRepository = sutProvider.GetDependency(); + organizationRepository.GetByIdAsync(orgId).Returns(organization); + + + // Act & Assert + await Assert.ThrowsAsync(() => sutProvider.Sut.GetMasterPasswordPolicy(orgId)); + } + [Theory] [BitAutoData] public async Task Get_WhenUserCanManagePolicies_WithExistingType_ReturnsExistingPolicy( @@ -142,16 +184,16 @@ public class PoliciesControllerTests { // Arrange sutProvider.GetDependency() - .ManagePolicies(orgId) - .Returns(true); + .ManagePolicies(orgId) + .Returns(true); policy.Type = (PolicyType)type; policy.Enabled = true; policy.Data = null; sutProvider.GetDependency() - .GetByOrganizationIdTypeAsync(orgId, (PolicyType)type) - .Returns(policy); + .GetByOrganizationIdTypeAsync(orgId, (PolicyType)type) + .Returns(policy); // Act var result = await sutProvider.Sut.Get(orgId, type); @@ -171,12 +213,12 @@ public class PoliciesControllerTests { // Arrange sutProvider.GetDependency() - .ManagePolicies(orgId) - .Returns(true); + .ManagePolicies(orgId) + .Returns(true); sutProvider.GetDependency() - .GetByOrganizationIdTypeAsync(orgId, (PolicyType)type) - .Returns((Policy)null); + .GetByOrganizationIdTypeAsync(orgId, (PolicyType)type) + .Returns((Policy)null); // Act var result = await sutProvider.Sut.Get(orgId, type); @@ -194,11 +236,42 @@ public class PoliciesControllerTests { // Arrange sutProvider.GetDependency() - .ManagePolicies(orgId) - .Returns(false); + .ManagePolicies(orgId) + .Returns(false); // Act & Assert await Assert.ThrowsAsync(() => sutProvider.Sut.Get(orgId, type)); } + [Theory] + [BitAutoData] + public async Task GetByTokenAsync_WhenOrganizationUseUsePoliciesIsFalse_Jimmy( + SutProvider sutProvider, Guid orgId, Guid organizationUserId, string token, string email, + Organization organization) + { + // Arrange + organization.UsePolicies = false; + + var organizationRepository = sutProvider.GetDependency(); + organizationRepository.GetByIdAsync(orgId).Returns(organization); + + + // Act & Assert + await Assert.ThrowsAsync(() => + sutProvider.Sut.GetByToken(orgId, email, token, organizationUserId)); + } + + [Theory] + [BitAutoData] + public async Task GetByTokenAsync_WhenOrganizationIsNull_ThrowsNotFoundException( + SutProvider sutProvider, Guid orgId, Guid organizationUserId, string token, string email) + { + // Arrange + var organizationRepository = sutProvider.GetDependency(); + organizationRepository.GetByIdAsync(orgId).Returns((Organization)null); + + // Act & Assert + await Assert.ThrowsAsync(() => + sutProvider.Sut.GetByToken(orgId, email, token, organizationUserId)); + } }