From 1dec51bf5affedc68d3ad6f98a1f0236eaaa60d8 Mon Sep 17 00:00:00 2001 From: Jared McCannon Date: Mon, 11 Nov 2024 09:52:42 -0600 Subject: [PATCH] [PM-13014] - Add CanToggleStatus property to PolicyRepsonseModel based on Policy Validators (#4940) * Adding CanToggleState to PoliciesControllers (api/public) endpoints. Added mappings wrapped in feature flag. * Updated logic for determining CanToggle. Removed setting of toggle from List endpoint. Added new details model for single policy response. Validator now returns after first error. --- .../Controllers/PoliciesController.cs | 30 +++++--- .../Response/Helpers/PolicyDetailResponses.cs | 19 +++++ .../PolicyDetailResponseModel.cs | 20 ++++++ .../Organizations}/PolicyResponseModel.cs | 2 +- .../Public/Controllers/PoliciesController.cs | 12 ++-- .../Controllers/EmergencyAccessController.cs | 2 +- .../Models/Response/SyncResponseModel.cs | 4 +- .../VerifyOrganizationDomainCommand.cs | 3 - .../Implementations/SavePolicyCommand.cs | 3 +- .../SingleOrgPolicyValidator.cs | 25 ++++++- .../Services/Implementations/PolicyService.cs | 2 +- .../Helpers/PolicyDetailResponsesTests.cs | 69 +++++++++++++++++++ .../Controllers/PoliciesControllerTests.cs | 6 +- .../Services/PolicyServiceTests.cs | 2 +- 14 files changed, 167 insertions(+), 32 deletions(-) create mode 100644 src/Api/AdminConsole/Models/Response/Helpers/PolicyDetailResponses.cs create mode 100644 src/Api/AdminConsole/Models/Response/Organizations/PolicyDetailResponseModel.cs rename src/{Core/AdminConsole/Models/Api/Response => Api/AdminConsole/Models/Response/Organizations}/PolicyResponseModel.cs (93%) create mode 100644 test/Api.Test/AdminConsole/Models/Response/Helpers/PolicyDetailResponsesTests.cs diff --git a/src/Api/AdminConsole/Controllers/PoliciesController.cs b/src/Api/AdminConsole/Controllers/PoliciesController.cs index 4a1becc0b..ee48cdd5d 100644 --- a/src/Api/AdminConsole/Controllers/PoliciesController.cs +++ b/src/Api/AdminConsole/Controllers/PoliciesController.cs @@ -1,7 +1,11 @@ using Bit.Api.AdminConsole.Models.Request; +using Bit.Api.AdminConsole.Models.Response.Helpers; +using Bit.Api.AdminConsole.Models.Response.Organizations; using Bit.Api.Models.Response; +using Bit.Core; +using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.Enums; -using Bit.Core.AdminConsole.Models.Api.Response; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationDomains.Interfaces; using Bit.Core.AdminConsole.Repositories; using Bit.Core.AdminConsole.Services; using Bit.Core.Auth.Models.Business.Tokenables; @@ -16,7 +20,6 @@ using Bit.Core.Utilities; using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.DataProtection; using Microsoft.AspNetCore.Mvc; -using AdminConsoleEntities = Bit.Core.AdminConsole.Entities; namespace Bit.Api.AdminConsole.Controllers; @@ -32,6 +35,8 @@ public class PoliciesController : Controller private readonly GlobalSettings _globalSettings; private readonly IDataProtector _organizationServiceDataProtector; private readonly IDataProtectorTokenFactory _orgUserInviteTokenDataFactory; + private readonly IFeatureService _featureService; + private readonly IOrganizationHasVerifiedDomainsQuery _organizationHasVerifiedDomainsQuery; public PoliciesController( IPolicyRepository policyRepository, @@ -41,7 +46,9 @@ public class PoliciesController : Controller ICurrentContext currentContext, GlobalSettings globalSettings, IDataProtectionProvider dataProtectionProvider, - IDataProtectorTokenFactory orgUserInviteTokenDataFactory) + IDataProtectorTokenFactory orgUserInviteTokenDataFactory, + IFeatureService featureService, + IOrganizationHasVerifiedDomainsQuery organizationHasVerifiedDomainsQuery) { _policyRepository = policyRepository; _policyService = policyService; @@ -53,10 +60,12 @@ public class PoliciesController : Controller "OrganizationServiceDataProtector"); _orgUserInviteTokenDataFactory = orgUserInviteTokenDataFactory; + _featureService = featureService; + _organizationHasVerifiedDomainsQuery = organizationHasVerifiedDomainsQuery; } [HttpGet("{type}")] - public async Task Get(Guid orgId, int type) + public async Task Get(Guid orgId, int type) { if (!await _currentContext.ManagePolicies(orgId)) { @@ -65,10 +74,15 @@ public class PoliciesController : Controller var policy = await _policyRepository.GetByOrganizationIdTypeAsync(orgId, (PolicyType)type); if (policy == null) { - return new PolicyResponseModel(new AdminConsoleEntities.Policy() { Type = (PolicyType)type, Enabled = false }); + return new PolicyDetailResponseModel(new Policy { Type = (PolicyType)type }); } - return new PolicyResponseModel(policy); + if (_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning) && policy.Type is PolicyType.SingleOrg) + { + return await policy.GetSingleOrgPolicyDetailResponseAsync(_organizationHasVerifiedDomainsQuery); + } + + return new PolicyDetailResponseModel(policy); } [HttpGet("")] @@ -81,8 +95,8 @@ public class PoliciesController : Controller } var policies = await _policyRepository.GetManyByOrganizationIdAsync(orgIdGuid); - var responses = policies.Select(p => new PolicyResponseModel(p)); - return new ListResponseModel(responses); + + return new ListResponseModel(policies.Select(p => new PolicyResponseModel(p))); } [AllowAnonymous] diff --git a/src/Api/AdminConsole/Models/Response/Helpers/PolicyDetailResponses.cs b/src/Api/AdminConsole/Models/Response/Helpers/PolicyDetailResponses.cs new file mode 100644 index 000000000..14b9642f6 --- /dev/null +++ b/src/Api/AdminConsole/Models/Response/Helpers/PolicyDetailResponses.cs @@ -0,0 +1,19 @@ +using Bit.Api.AdminConsole.Models.Response.Organizations; +using Bit.Core.AdminConsole.Entities; +using Bit.Core.AdminConsole.Enums; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationDomains.Interfaces; + +namespace Bit.Api.AdminConsole.Models.Response.Helpers; + +public static class PolicyDetailResponses +{ + public static async Task GetSingleOrgPolicyDetailResponseAsync(this Policy policy, IOrganizationHasVerifiedDomainsQuery hasVerifiedDomainsQuery) + { + if (policy.Type is not PolicyType.SingleOrg) + { + throw new ArgumentException($"'{nameof(policy)}' must be of type '{nameof(PolicyType.SingleOrg)}'.", nameof(policy)); + } + + return new PolicyDetailResponseModel(policy, !await hasVerifiedDomainsQuery.HasVerifiedDomainsAsync(policy.OrganizationId)); + } +} diff --git a/src/Api/AdminConsole/Models/Response/Organizations/PolicyDetailResponseModel.cs b/src/Api/AdminConsole/Models/Response/Organizations/PolicyDetailResponseModel.cs new file mode 100644 index 000000000..cb5560e68 --- /dev/null +++ b/src/Api/AdminConsole/Models/Response/Organizations/PolicyDetailResponseModel.cs @@ -0,0 +1,20 @@ +using Bit.Core.AdminConsole.Entities; + +namespace Bit.Api.AdminConsole.Models.Response.Organizations; + +public class PolicyDetailResponseModel : PolicyResponseModel +{ + public PolicyDetailResponseModel(Policy policy, string obj = "policy") : base(policy, obj) + { + } + + public PolicyDetailResponseModel(Policy policy, bool canToggleState) : base(policy) + { + CanToggleState = canToggleState; + } + + /// + /// Indicates whether the Policy can be enabled/disabled + /// + public bool CanToggleState { get; set; } = true; +} diff --git a/src/Core/AdminConsole/Models/Api/Response/PolicyResponseModel.cs b/src/Api/AdminConsole/Models/Response/Organizations/PolicyResponseModel.cs similarity index 93% rename from src/Core/AdminConsole/Models/Api/Response/PolicyResponseModel.cs rename to src/Api/AdminConsole/Models/Response/Organizations/PolicyResponseModel.cs index 7ef6b1573..86e62a419 100644 --- a/src/Core/AdminConsole/Models/Api/Response/PolicyResponseModel.cs +++ b/src/Api/AdminConsole/Models/Response/Organizations/PolicyResponseModel.cs @@ -3,7 +3,7 @@ using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.Enums; using Bit.Core.Models.Api; -namespace Bit.Core.AdminConsole.Models.Api.Response; +namespace Bit.Api.AdminConsole.Models.Response.Organizations; public class PolicyResponseModel : ResponseModel { diff --git a/src/Api/AdminConsole/Public/Controllers/PoliciesController.cs b/src/Api/AdminConsole/Public/Controllers/PoliciesController.cs index 71e03a547..f2e7c35d2 100644 --- a/src/Api/AdminConsole/Public/Controllers/PoliciesController.cs +++ b/src/Api/AdminConsole/Public/Controllers/PoliciesController.cs @@ -41,14 +41,13 @@ public class PoliciesController : Controller [ProducesResponseType((int)HttpStatusCode.NotFound)] public async Task Get(PolicyType type) { - var policy = await _policyRepository.GetByOrganizationIdTypeAsync( - _currentContext.OrganizationId.Value, type); + var policy = await _policyRepository.GetByOrganizationIdTypeAsync(_currentContext.OrganizationId.Value, type); if (policy == null) { return new NotFoundResult(); } - var response = new PolicyResponseModel(policy); - return new JsonResult(response); + + return new JsonResult(new PolicyResponseModel(policy)); } /// @@ -62,9 +61,8 @@ public class PoliciesController : Controller public async Task List() { var policies = await _policyRepository.GetManyByOrganizationIdAsync(_currentContext.OrganizationId.Value); - var policyResponses = policies.Select(p => new PolicyResponseModel(p)); - var response = new ListResponseModel(policyResponses); - return new JsonResult(response); + + return new JsonResult(new ListResponseModel(policies.Select(p => new PolicyResponseModel(p)))); } /// diff --git a/src/Api/Auth/Controllers/EmergencyAccessController.cs b/src/Api/Auth/Controllers/EmergencyAccessController.cs index 95fac234c..9f8ea3df0 100644 --- a/src/Api/Auth/Controllers/EmergencyAccessController.cs +++ b/src/Api/Auth/Controllers/EmergencyAccessController.cs @@ -1,10 +1,10 @@ using Bit.Api.AdminConsole.Models.Request.Organizations; +using Bit.Api.AdminConsole.Models.Response.Organizations; using Bit.Api.Auth.Models.Request; using Bit.Api.Auth.Models.Response; using Bit.Api.Models.Response; using Bit.Api.Vault.Models.Response; using Bit.Core.AdminConsole.Entities; -using Bit.Core.AdminConsole.Models.Api.Response; using Bit.Core.Auth.Services; using Bit.Core.Exceptions; using Bit.Core.Repositories; diff --git a/src/Api/Vault/Models/Response/SyncResponseModel.cs b/src/Api/Vault/Models/Response/SyncResponseModel.cs index ce5f4562d..a9b87ac31 100644 --- a/src/Api/Vault/Models/Response/SyncResponseModel.cs +++ b/src/Api/Vault/Models/Response/SyncResponseModel.cs @@ -1,7 +1,7 @@ -using Bit.Api.Models.Response; +using Bit.Api.AdminConsole.Models.Response.Organizations; +using Bit.Api.Models.Response; using Bit.Api.Tools.Models.Response; using Bit.Core.AdminConsole.Entities; -using Bit.Core.AdminConsole.Models.Api.Response; using Bit.Core.AdminConsole.Models.Data.Provider; using Bit.Core.Entities; using Bit.Core.Models.Api; diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs index 4a597a290..870fa72aa 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs @@ -20,7 +20,6 @@ public class VerifyOrganizationDomainCommand : IVerifyOrganizationDomainCommand private readonly IGlobalSettings _globalSettings; private readonly IPolicyService _policyService; private readonly IFeatureService _featureService; - private readonly IOrganizationService _organizationService; private readonly ILogger _logger; public VerifyOrganizationDomainCommand( @@ -30,7 +29,6 @@ public class VerifyOrganizationDomainCommand : IVerifyOrganizationDomainCommand IGlobalSettings globalSettings, IPolicyService policyService, IFeatureService featureService, - IOrganizationService organizationService, ILogger logger) { _organizationDomainRepository = organizationDomainRepository; @@ -39,7 +37,6 @@ public class VerifyOrganizationDomainCommand : IVerifyOrganizationDomainCommand _globalSettings = globalSettings; _policyService = policyService; _featureService = featureService; - _organizationService = organizationService; _logger = logger; } diff --git a/src/Core/AdminConsole/OrganizationFeatures/Policies/Implementations/SavePolicyCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/Policies/Implementations/SavePolicyCommand.cs index 01ffce2cc..f193aeabd 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Policies/Implementations/SavePolicyCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Policies/Implementations/SavePolicyCommand.cs @@ -87,8 +87,7 @@ public class SavePolicyCommand : ISavePolicyCommand if (currentPolicy is not { Enabled: true } && policyUpdate.Enabled) { var missingRequiredPolicyTypes = validator.RequiredPolicies - .Where(requiredPolicyType => - savedPoliciesDict.GetValueOrDefault(requiredPolicyType) is not { Enabled: true }) + .Where(requiredPolicyType => savedPoliciesDict.GetValueOrDefault(requiredPolicyType) is not { Enabled: true }) .ToList(); if (missingRequiredPolicyTypes.Count != 0) diff --git a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs index 3e1f8d26c..cc6971f94 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs @@ -2,6 +2,7 @@ using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.Enums; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationDomains.Interfaces; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; using Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models; using Bit.Core.Auth.Enums; @@ -23,7 +24,9 @@ public class SingleOrgPolicyValidator : IPolicyValidator private readonly IOrganizationRepository _organizationRepository; private readonly ISsoConfigRepository _ssoConfigRepository; private readonly ICurrentContext _currentContext; + private readonly IFeatureService _featureService; private readonly IRemoveOrganizationUserCommand _removeOrganizationUserCommand; + private readonly IOrganizationHasVerifiedDomainsQuery _organizationHasVerifiedDomainsQuery; public SingleOrgPolicyValidator( IOrganizationUserRepository organizationUserRepository, @@ -31,14 +34,18 @@ public class SingleOrgPolicyValidator : IPolicyValidator IOrganizationRepository organizationRepository, ISsoConfigRepository ssoConfigRepository, ICurrentContext currentContext, - IRemoveOrganizationUserCommand removeOrganizationUserCommand) + IFeatureService featureService, + IRemoveOrganizationUserCommand removeOrganizationUserCommand, + IOrganizationHasVerifiedDomainsQuery organizationHasVerifiedDomainsQuery) { _organizationUserRepository = organizationUserRepository; _mailService = mailService; _organizationRepository = organizationRepository; _ssoConfigRepository = ssoConfigRepository; _currentContext = currentContext; + _featureService = featureService; _removeOrganizationUserCommand = removeOrganizationUserCommand; + _organizationHasVerifiedDomainsQuery = organizationHasVerifiedDomainsQuery; } public IEnumerable RequiredPolicies => []; @@ -93,9 +100,21 @@ public class SingleOrgPolicyValidator : IPolicyValidator if (policyUpdate is not { Enabled: true }) { var ssoConfig = await _ssoConfigRepository.GetByOrganizationIdAsync(policyUpdate.OrganizationId); - return ssoConfig.ValidateDecryptionOptionsNotEnabled([MemberDecryptionType.KeyConnector]); + + var validateDecryptionErrorMessage = ssoConfig.ValidateDecryptionOptionsNotEnabled([MemberDecryptionType.KeyConnector]); + + if (!string.IsNullOrWhiteSpace(validateDecryptionErrorMessage)) + { + return validateDecryptionErrorMessage; + } + + 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 ""; + return string.Empty; } } diff --git a/src/Core/AdminConsole/Services/Implementations/PolicyService.cs b/src/Core/AdminConsole/Services/Implementations/PolicyService.cs index 072aa8283..42655040a 100644 --- a/src/Core/AdminConsole/Services/Implementations/PolicyService.cs +++ b/src/Core/AdminConsole/Services/Implementations/PolicyService.cs @@ -289,7 +289,7 @@ public class PolicyService : IPolicyService if (_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning) && await _organizationHasVerifiedDomainsQuery.HasVerifiedDomainsAsync(org.Id)) { - throw new BadRequestException("Organization has verified domains."); + throw new BadRequestException("The Single organization policy is required for organizations that have enabled domain verification."); } } diff --git a/test/Api.Test/AdminConsole/Models/Response/Helpers/PolicyDetailResponsesTests.cs b/test/Api.Test/AdminConsole/Models/Response/Helpers/PolicyDetailResponsesTests.cs new file mode 100644 index 000000000..c380185a7 --- /dev/null +++ b/test/Api.Test/AdminConsole/Models/Response/Helpers/PolicyDetailResponsesTests.cs @@ -0,0 +1,69 @@ +using AutoFixture; +using Bit.Api.AdminConsole.Models.Response.Helpers; +using Bit.Core.AdminConsole.Entities; +using Bit.Core.AdminConsole.Enums; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationDomains.Interfaces; +using NSubstitute; +using Xunit; + +namespace Bit.Api.Test.AdminConsole.Models.Response.Helpers; + +public class PolicyDetailResponsesTests +{ + [Fact] + public async Task GetSingleOrgPolicyDetailResponseAsync_GivenPolicyEntity_WhenIsSingleOrgTypeAndHasVerifiedDomains_ThenShouldNotBeAbleToToggle() + { + var fixture = new Fixture(); + + var policy = fixture.Build() + .Without(p => p.Data) + .With(p => p.Type, PolicyType.SingleOrg) + .Create(); + + var querySub = Substitute.For(); + querySub.HasVerifiedDomainsAsync(policy.OrganizationId) + .Returns(true); + + var result = await policy.GetSingleOrgPolicyDetailResponseAsync(querySub); + + Assert.False(result.CanToggleState); + } + + [Fact] + public async Task GetSingleOrgPolicyDetailResponseAsync_GivenPolicyEntity_WhenIsNotSingleOrgType_ThenShouldThrowArgumentException() + { + var fixture = new Fixture(); + + var policy = fixture.Build() + .Without(p => p.Data) + .With(p => p.Type, PolicyType.TwoFactorAuthentication) + .Create(); + + var querySub = Substitute.For(); + querySub.HasVerifiedDomainsAsync(policy.OrganizationId) + .Returns(true); + + var action = async () => await policy.GetSingleOrgPolicyDetailResponseAsync(querySub); + + await Assert.ThrowsAsync("policy", action); + } + + [Fact] + public async Task GetSingleOrgPolicyDetailResponseAsync_GivenPolicyEntity_WhenIsSingleOrgTypeAndDoesNotHaveVerifiedDomains_ThenShouldBeAbleToToggle() + { + var fixture = new Fixture(); + + var policy = fixture.Build() + .Without(p => p.Data) + .With(p => p.Type, PolicyType.SingleOrg) + .Create(); + + var querySub = Substitute.For(); + querySub.HasVerifiedDomainsAsync(policy.OrganizationId) + .Returns(false); + + var result = await policy.GetSingleOrgPolicyDetailResponseAsync(querySub); + + Assert.True(result.CanToggleState); + } +} diff --git a/test/Api.Test/Controllers/PoliciesControllerTests.cs b/test/Api.Test/Controllers/PoliciesControllerTests.cs index 77cc5ea02..1b96ace5d 100644 --- a/test/Api.Test/Controllers/PoliciesControllerTests.cs +++ b/test/Api.Test/Controllers/PoliciesControllerTests.cs @@ -1,9 +1,9 @@ using System.Security.Claims; using System.Text.Json; using Bit.Api.AdminConsole.Controllers; +using Bit.Api.AdminConsole.Models.Response.Organizations; using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.Enums; -using Bit.Core.AdminConsole.Models.Api.Response; using Bit.Core.AdminConsole.Models.Data.Organizations.Policies; using Bit.Core.AdminConsole.Repositories; using Bit.Core.Context; @@ -157,7 +157,7 @@ public class PoliciesControllerTests var result = await sutProvider.Sut.Get(orgId, type); // Assert - Assert.IsType(result); + Assert.IsType(result); Assert.Equal(policy.Id, result.Id); Assert.Equal(policy.Type, result.Type); Assert.Equal(policy.Enabled, result.Enabled); @@ -182,7 +182,7 @@ public class PoliciesControllerTests var result = await sutProvider.Sut.Get(orgId, type); // Assert - Assert.IsType(result); + Assert.IsType(result); Assert.Equal(result.Type, (PolicyType)type); Assert.False(result.Enabled); } diff --git a/test/Core.Test/AdminConsole/Services/PolicyServiceTests.cs b/test/Core.Test/AdminConsole/Services/PolicyServiceTests.cs index da3f2b267..68f36e37c 100644 --- a/test/Core.Test/AdminConsole/Services/PolicyServiceTests.cs +++ b/test/Core.Test/AdminConsole/Services/PolicyServiceTests.cs @@ -842,6 +842,6 @@ public class PolicyServiceTests var badRequestException = await Assert.ThrowsAsync( () => sutProvider.Sut.SaveAsync(policy, null)); - Assert.Equal("Organization has verified domains.", badRequestException.Message); + Assert.Equal("The Single organization policy is required for organizations that have enabled domain verification.", badRequestException.Message); } }