From 7b66fff98c3d4045391e8b06cf79c986cd45009b Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Mon, 7 Oct 2024 09:45:45 +1000 Subject: [PATCH] Revert "Use delegates to simplify mocking" This reverts commit 4983e413e468675506f9189ae9aa5e7826da91bf. --- .../Policies/IPolicyDefinition.cs | 5 +-- .../SingleOrgPolicyDefinition.cs | 34 ++++++++++--------- .../Implementations/PolicyServicevNext.cs | 8 ++--- .../PolicyServicevNextCustomization.cs | 2 ++ .../Services/PolicyDefinitionFixtures.cs | 28 ++++++++------- .../Services/PolicyServicevNextTests.cs | 6 ++-- 6 files changed, 45 insertions(+), 38 deletions(-) diff --git a/src/Core/AdminConsole/OrganizationFeatures/Policies/IPolicyDefinition.cs b/src/Core/AdminConsole/OrganizationFeatures/Policies/IPolicyDefinition.cs index 82cf3d64a..c5c1b1257 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Policies/IPolicyDefinition.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Policies/IPolicyDefinition.cs @@ -2,6 +2,7 @@ using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.Enums; +using Bit.Core.Entities; namespace Bit.Core.AdminConsole.OrganizationFeatures.Policies; @@ -25,7 +26,7 @@ public interface IPolicyDefinition /// The current policy, if any /// The modified policy to be saved /// A sequence of validation errors if validation was unsuccessful - public Func> Validate { get; } + public Task ValidateAsync(Policy? currentPolicy, Policy modifiedPolicy); /// /// Optionally performs side effects after a policy is validated but before it is saved. @@ -33,5 +34,5 @@ public interface IPolicyDefinition /// /// The current policy, if any /// The modified policy to be saved - public Func OnSaveSideEffects { get; } + public Task OnSaveSideEffectsAsync(Policy? currentPolicy, Policy modifiedPolicy); } diff --git a/src/Core/AdminConsole/OrganizationFeatures/Policies/Implementations/SingleOrgPolicyDefinition.cs b/src/Core/AdminConsole/OrganizationFeatures/Policies/Implementations/SingleOrgPolicyDefinition.cs index 46a6b1389..9b59d2790 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Policies/Implementations/SingleOrgPolicyDefinition.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Policies/Implementations/SingleOrgPolicyDefinition.cs @@ -1,10 +1,13 @@ #nullable enable +using System.ComponentModel; using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.Enums; +using Bit.Core.AdminConsole.Repositories; using Bit.Core.Auth.Enums; using Bit.Core.Auth.Repositories; using Bit.Core.Context; +using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Exceptions; using Bit.Core.Repositories; @@ -37,27 +40,13 @@ public class SingleOrgPolicyDefinition : IPolicyDefinition _currentContext = currentContext; } - public Func> Validate => async (currentPolicy, modifiedPolicy) => - { - var organizationId = modifiedPolicy.OrganizationId; - - // Do not allow this policy to be disabled if Key Connector is being used - var ssoConfig = await _ssoConfigRepository.GetByOrganizationIdAsync(organizationId); - if (ssoConfig?.GetData()?.MemberDecryptionType == MemberDecryptionType.KeyConnector) - { - return "Key Connector is enabled."; - } - - return null; - }; - - public Func OnSaveSideEffects => async (currentPolicy, modifiedPolicy) => + public async Task OnSaveSideEffectsAsync(Policy? currentPolicy, Policy modifiedPolicy) { if (currentPolicy is null or { Enabled: false } && modifiedPolicy is { Enabled: true }) { await RemoveNonCompliantUsersAsync(modifiedPolicy.OrganizationId); } - }; + } private async Task RemoveNonCompliantUsersAsync(Guid organizationId) { @@ -96,4 +85,17 @@ public class SingleOrgPolicyDefinition : IPolicyDefinition } } + public async Task ValidateAsync(Policy? currentPolicy, Policy modifiedPolicy) + { + var organizationId = modifiedPolicy.OrganizationId; + + // Do not allow this policy to be disabled if Key Connector is being used + var ssoConfig = await _ssoConfigRepository.GetByOrganizationIdAsync(organizationId); + if (ssoConfig?.GetData()?.MemberDecryptionType == MemberDecryptionType.KeyConnector) + { + return "Key Connector is enabled."; + } + + return null; + } } diff --git a/src/Core/AdminConsole/Services/Implementations/PolicyServicevNext.cs b/src/Core/AdminConsole/Services/Implementations/PolicyServicevNext.cs index a13884c1e..63c1cdafe 100644 --- a/src/Core/AdminConsole/Services/Implementations/PolicyServicevNext.cs +++ b/src/Core/AdminConsole/Services/Implementations/PolicyServicevNext.cs @@ -82,21 +82,21 @@ public class PolicyServicevNext : IPolicyServicevNext .Where(otherPolicy => otherPolicy is { Enabled: true }) .ToList(); - if (dependentPolicies is { Count: > 0 }) + if (dependentPolicies is { Count: > 0}) { - throw new BadRequestException("This policy is required by " + dependentPolicies.First() + ". Try disabling that policy first."); + throw new BadRequestException("This policy is required by " + dependentPolicies.First() + ". Try disabling that policy first." ); } } // Run other validation - var validationError = await policyDefinition.Validate(currentPolicy, policy); + var validationError = await policyDefinition.ValidateAsync(currentPolicy, policy); if (validationError != null) { throw new BadRequestException(validationError); } // Run side effects - await policyDefinition.OnSaveSideEffects(currentPolicy, policy); + await policyDefinition.OnSaveSideEffectsAsync(currentPolicy, policy); var now = DateTime.UtcNow; if (policy.Id == default) diff --git a/test/Core.Test/AdminConsole/AutoFixture/PolicyServicevNextCustomization.cs b/test/Core.Test/AdminConsole/AutoFixture/PolicyServicevNextCustomization.cs index 10154a82f..797f4ec60 100644 --- a/test/Core.Test/AdminConsole/AutoFixture/PolicyServicevNextCustomization.cs +++ b/test/Core.Test/AdminConsole/AutoFixture/PolicyServicevNextCustomization.cs @@ -1,9 +1,11 @@ #nullable enable using System.Reflection; +using AutoFixture; using AutoFixture.Kernel; using Bit.Core.AdminConsole.OrganizationFeatures.Policies; using Bit.Core.AdminConsole.Services.Implementations; +using Bit.Test.Common.AutoFixture.Attributes; namespace Bit.Core.Test.AdminConsole.AutoFixture; diff --git a/test/Core.Test/AdminConsole/Services/PolicyDefinitionFixtures.cs b/test/Core.Test/AdminConsole/Services/PolicyDefinitionFixtures.cs index 4e5dce9f9..f0047c83b 100644 --- a/test/Core.Test/AdminConsole/Services/PolicyDefinitionFixtures.cs +++ b/test/Core.Test/AdminConsole/Services/PolicyDefinitionFixtures.cs @@ -1,9 +1,8 @@ -#nullable enable - -using Bit.Core.AdminConsole.Entities; +using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.Enums; using Bit.Core.AdminConsole.OrganizationFeatures.Policies; using NSubstitute; +using NSubstitute.ExceptionExtensions; namespace Bit.Core.Test.AdminConsole.Services; @@ -11,15 +10,18 @@ public class FakeSingleOrgPolicyDefinition : IPolicyDefinition { public PolicyType Type => PolicyType.SingleOrg; public IEnumerable RequiredPolicies => Array.Empty(); - public Func> Validate => Substitute.For>>(); - public Func OnSaveSideEffects => Substitute.For>(); -} -public class FakeRequireSsoPolicyDefinition : IPolicyDefinition -{ - public PolicyType Type => PolicyType.RequireSso; - public IEnumerable RequiredPolicies => [PolicyType.SingleOrg]; - public Func> Validate => Substitute.For>>(); - public Func OnSaveSideEffects => Substitute.For>(); -} + public readonly Func> ValidateAsyncMock = Substitute.For>>(); + public readonly Action OnSaveSideEffectsAsyncMock = Substitute.For>(); + public Task? ValidateAsync(Policy? currentPolicy, Policy modifiedPolicy) + { + return ValidateAsyncMock(currentPolicy, modifiedPolicy); + } + + public Task OnSaveSideEffectsAsync(Policy? currentPolicy, Policy modifiedPolicy) + { + OnSaveSideEffectsAsyncMock(currentPolicy, modifiedPolicy); + return Task.FromResult(0); + } +} diff --git a/test/Core.Test/AdminConsole/Services/PolicyServicevNextTests.cs b/test/Core.Test/AdminConsole/Services/PolicyServicevNextTests.cs index ea994045f..4ad8828ad 100644 --- a/test/Core.Test/AdminConsole/Services/PolicyServicevNextTests.cs +++ b/test/Core.Test/AdminConsole/Services/PolicyServicevNextTests.cs @@ -10,9 +10,9 @@ using Bit.Core.Services; using Bit.Core.Test.AdminConsole.AutoFixture; using Bit.Test.Common.AutoFixture; using Bit.Test.Common.AutoFixture.Attributes; +using AdminConsoleFixtures = Bit.Core.Test.AdminConsole.AutoFixture; using NSubstitute; using Xunit; -using AdminConsoleFixtures = Bit.Core.Test.AdminConsole.AutoFixture; namespace Bit.Core.Test.AdminConsole.Services; @@ -72,7 +72,7 @@ public class PolicyServicevNextTests } [Theory, BitAutoData] - public async Task SaveAsync_PolicyDefinitionNotFound_Throws([Policy(PolicyType.SingleOrg)] Policy policy) + public async Task SaveAsync_PolicyDefinitionNotFound_Throws([Policy(PolicyType.SingleOrg)]Policy policy) { var sutProvider = SutProviderFactory(); ArrangeOrganization(sutProvider, policy); @@ -91,7 +91,7 @@ public class PolicyServicevNextTests public async Task SaveAsync_ThrowsOnValidationError([AdminConsoleFixtures.Policy(PolicyType.SingleOrg)] Policy policy) { var fakePolicyDefinition = new FakeSingleOrgPolicyDefinition(); - fakePolicyDefinition.Validate(null, policy).Returns("Validation error!"); + fakePolicyDefinition.ValidateAsyncMock(null, policy).Returns("Validation error!"); var sutProvider = SutProviderFactory([fakePolicyDefinition]); ArrangeOrganization(sutProvider, policy);