From 7b5e0e4a64730fdfb1ae1e2d220033c8ac66dc26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rui=20Tom=C3=A9?= <108268980+r-tome@users.noreply.github.com> Date: Tue, 22 Oct 2024 10:38:01 +0100 Subject: [PATCH] [PM-13836] Refactor IPolicyService to remove unnecessary IOrganizationService dependency (#4914) --- .../Controllers/PoliciesController.cs | 5 +--- .../Public/Controllers/PoliciesController.cs | 6 +---- .../AdminConsole/Services/IPolicyService.cs | 3 +-- .../Services/Implementations/PolicyService.cs | 6 ++--- .../Implementations/SsoConfigService.cs | 9 +++----- .../Services/PolicyServiceTests.cs | 23 ++++--------------- .../Auth/Services/SsoConfigServiceTests.cs | 3 --- 7 files changed, 13 insertions(+), 42 deletions(-) diff --git a/src/Api/AdminConsole/Controllers/PoliciesController.cs b/src/Api/AdminConsole/Controllers/PoliciesController.cs index b3be852db..7bfd13c40 100644 --- a/src/Api/AdminConsole/Controllers/PoliciesController.cs +++ b/src/Api/AdminConsole/Controllers/PoliciesController.cs @@ -25,7 +25,6 @@ public class PoliciesController : Controller { private readonly IPolicyRepository _policyRepository; private readonly IPolicyService _policyService; - private readonly IOrganizationService _organizationService; private readonly IOrganizationUserRepository _organizationUserRepository; private readonly IUserService _userService; private readonly ICurrentContext _currentContext; @@ -36,7 +35,6 @@ public class PoliciesController : Controller public PoliciesController( IPolicyRepository policyRepository, IPolicyService policyService, - IOrganizationService organizationService, IOrganizationUserRepository organizationUserRepository, IUserService userService, ICurrentContext currentContext, @@ -46,7 +44,6 @@ public class PoliciesController : Controller { _policyRepository = policyRepository; _policyService = policyService; - _organizationService = organizationService; _organizationUserRepository = organizationUserRepository; _userService = userService; _currentContext = currentContext; @@ -185,7 +182,7 @@ public class PoliciesController : Controller } var userId = _userService.GetProperUserId(User); - await _policyService.SaveAsync(policy, _organizationService, userId); + await _policyService.SaveAsync(policy, userId); return new PolicyResponseModel(policy); } } diff --git a/src/Api/AdminConsole/Public/Controllers/PoliciesController.cs b/src/Api/AdminConsole/Public/Controllers/PoliciesController.cs index 2d83bd705..71e03a547 100644 --- a/src/Api/AdminConsole/Public/Controllers/PoliciesController.cs +++ b/src/Api/AdminConsole/Public/Controllers/PoliciesController.cs @@ -6,7 +6,6 @@ using Bit.Core.AdminConsole.Enums; using Bit.Core.AdminConsole.Repositories; using Bit.Core.AdminConsole.Services; using Bit.Core.Context; -using Bit.Core.Services; using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; @@ -18,18 +17,15 @@ public class PoliciesController : Controller { private readonly IPolicyRepository _policyRepository; private readonly IPolicyService _policyService; - private readonly IOrganizationService _organizationService; private readonly ICurrentContext _currentContext; public PoliciesController( IPolicyRepository policyRepository, IPolicyService policyService, - IOrganizationService organizationService, ICurrentContext currentContext) { _policyRepository = policyRepository; _policyService = policyService; - _organizationService = organizationService; _currentContext = currentContext; } @@ -96,7 +92,7 @@ public class PoliciesController : Controller { policy = model.ToPolicy(policy); } - await _policyService.SaveAsync(policy, _organizationService, null); + await _policyService.SaveAsync(policy, null); var response = new PolicyResponseModel(policy); return new JsonResult(response); } diff --git a/src/Core/AdminConsole/Services/IPolicyService.cs b/src/Core/AdminConsole/Services/IPolicyService.cs index 6d92a3a4f..16ff2f4fa 100644 --- a/src/Core/AdminConsole/Services/IPolicyService.cs +++ b/src/Core/AdminConsole/Services/IPolicyService.cs @@ -4,13 +4,12 @@ using Bit.Core.AdminConsole.Models.Data.Organizations.Policies; using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Models.Data.Organizations.OrganizationUsers; -using Bit.Core.Services; namespace Bit.Core.AdminConsole.Services; public interface IPolicyService { - Task SaveAsync(Policy policy, IOrganizationService organizationService, Guid? savingUserId); + Task SaveAsync(Policy policy, Guid? savingUserId); /// /// Get the combined master password policy options for the specified user. diff --git a/src/Core/AdminConsole/Services/Implementations/PolicyService.cs b/src/Core/AdminConsole/Services/Implementations/PolicyService.cs index 4e3a7bb89..6ab90afe0 100644 --- a/src/Core/AdminConsole/Services/Implementations/PolicyService.cs +++ b/src/Core/AdminConsole/Services/Implementations/PolicyService.cs @@ -61,7 +61,7 @@ public class PolicyService : IPolicyService _removeOrganizationUserCommand = removeOrganizationUserCommand; } - public async Task SaveAsync(Policy policy, IOrganizationService organizationService, Guid? savingUserId) + public async Task SaveAsync(Policy policy, Guid? savingUserId) { if (_featureService.IsEnabled(FeatureFlagKeys.Pm13322AddPolicyDefinitions)) { @@ -111,7 +111,7 @@ public class PolicyService : IPolicyService return; } - await EnablePolicyAsync(policy, org, organizationService, savingUserId); + await EnablePolicyAsync(policy, org, savingUserId); } public async Task GetMasterPasswordPolicyForUserAsync(User user) @@ -285,7 +285,7 @@ public class PolicyService : IPolicyService await _eventService.LogPolicyEventAsync(policy, EventType.Policy_Updated); } - private async Task EnablePolicyAsync(Policy policy, Organization org, IOrganizationService organizationService, Guid? savingUserId) + private async Task EnablePolicyAsync(Policy policy, Organization org, Guid? savingUserId) { var currentPolicy = await _policyRepository.GetByIdAsync(policy.Id); if (!currentPolicy?.Enabled ?? true) diff --git a/src/Core/Auth/Services/Implementations/SsoConfigService.cs b/src/Core/Auth/Services/Implementations/SsoConfigService.cs index fdf7e278e..532f00039 100644 --- a/src/Core/Auth/Services/Implementations/SsoConfigService.cs +++ b/src/Core/Auth/Services/Implementations/SsoConfigService.cs @@ -20,7 +20,6 @@ public class SsoConfigService : ISsoConfigService private readonly IPolicyService _policyService; private readonly IOrganizationRepository _organizationRepository; private readonly IOrganizationUserRepository _organizationUserRepository; - private readonly IOrganizationService _organizationService; private readonly IEventService _eventService; public SsoConfigService( @@ -29,7 +28,6 @@ public class SsoConfigService : ISsoConfigService IPolicyService policyService, IOrganizationRepository organizationRepository, IOrganizationUserRepository organizationUserRepository, - IOrganizationService organizationService, IEventService eventService) { _ssoConfigRepository = ssoConfigRepository; @@ -37,7 +35,6 @@ public class SsoConfigService : ISsoConfigService _policyService = policyService; _organizationRepository = organizationRepository; _organizationUserRepository = organizationUserRepository; - _organizationService = organizationService; _eventService = eventService; } @@ -71,20 +68,20 @@ public class SsoConfigService : ISsoConfigService singleOrgPolicy.Enabled = true; - await _policyService.SaveAsync(singleOrgPolicy, _organizationService, null); + await _policyService.SaveAsync(singleOrgPolicy, null); var resetPolicy = await _policyRepository.GetByOrganizationIdTypeAsync(config.OrganizationId, PolicyType.ResetPassword) ?? new Policy { OrganizationId = config.OrganizationId, Type = PolicyType.ResetPassword, }; resetPolicy.Enabled = true; resetPolicy.SetDataModel(new ResetPasswordDataModel { AutoEnrollEnabled = true }); - await _policyService.SaveAsync(resetPolicy, _organizationService, null); + await _policyService.SaveAsync(resetPolicy, null); var ssoRequiredPolicy = await _policyRepository.GetByOrganizationIdTypeAsync(config.OrganizationId, PolicyType.RequireSso) ?? new Policy { OrganizationId = config.OrganizationId, Type = PolicyType.RequireSso, }; ssoRequiredPolicy.Enabled = true; - await _policyService.SaveAsync(ssoRequiredPolicy, _organizationService, null); + await _policyService.SaveAsync(ssoRequiredPolicy, null); } await LogEventsAsync(config, oldConfig); diff --git a/test/Core.Test/AdminConsole/Services/PolicyServiceTests.cs b/test/Core.Test/AdminConsole/Services/PolicyServiceTests.cs index fb08a32f2..f9bc49bbe 100644 --- a/test/Core.Test/AdminConsole/Services/PolicyServiceTests.cs +++ b/test/Core.Test/AdminConsole/Services/PolicyServiceTests.cs @@ -34,7 +34,6 @@ public class PolicyServiceTests var badRequestException = await Assert.ThrowsAsync( () => sutProvider.Sut.SaveAsync(policy, - Substitute.For(), Guid.NewGuid())); Assert.Contains("Organization not found", badRequestException.Message, StringComparison.OrdinalIgnoreCase); @@ -61,7 +60,6 @@ public class PolicyServiceTests var badRequestException = await Assert.ThrowsAsync( () => sutProvider.Sut.SaveAsync(policy, - Substitute.For(), Guid.NewGuid())); Assert.Contains("cannot use policies", badRequestException.Message, StringComparison.OrdinalIgnoreCase); @@ -93,7 +91,6 @@ public class PolicyServiceTests var badRequestException = await Assert.ThrowsAsync( () => sutProvider.Sut.SaveAsync(policy, - Substitute.For(), Guid.NewGuid())); Assert.Contains("Single Sign-On Authentication policy is enabled.", badRequestException.Message, StringComparison.OrdinalIgnoreCase); @@ -124,7 +121,6 @@ public class PolicyServiceTests var badRequestException = await Assert.ThrowsAsync( () => sutProvider.Sut.SaveAsync(policy, - Substitute.For(), Guid.NewGuid())); Assert.Contains("Maximum Vault Timeout policy is enabled.", badRequestException.Message, StringComparison.OrdinalIgnoreCase); @@ -161,7 +157,6 @@ public class PolicyServiceTests var badRequestException = await Assert.ThrowsAsync( () => sutProvider.Sut.SaveAsync(policy, - Substitute.For(), Guid.NewGuid())); Assert.Contains("Key Connector is enabled.", badRequestException.Message, StringComparison.OrdinalIgnoreCase); @@ -189,7 +184,6 @@ public class PolicyServiceTests var badRequestException = await Assert.ThrowsAsync( () => sutProvider.Sut.SaveAsync(policy, - Substitute.For(), Guid.NewGuid())); Assert.Contains("Single Organization policy not enabled.", badRequestException.Message, StringComparison.OrdinalIgnoreCase); @@ -222,7 +216,7 @@ public class PolicyServiceTests var utcNow = DateTime.UtcNow; - await sutProvider.Sut.SaveAsync(policy, Substitute.For(), Guid.NewGuid()); + await sutProvider.Sut.SaveAsync(policy, Guid.NewGuid()); await sutProvider.GetDependency().Received() .LogPolicyEventAsync(policy, EventType.Policy_Updated); @@ -252,7 +246,6 @@ public class PolicyServiceTests var badRequestException = await Assert.ThrowsAsync( () => sutProvider.Sut.SaveAsync(policy, - Substitute.For(), Guid.NewGuid())); Assert.Contains("Single Organization policy not enabled.", badRequestException.Message, StringComparison.OrdinalIgnoreCase); @@ -353,14 +346,13 @@ public class PolicyServiceTests (orgUserDetailAdmin, false), }); - var organizationService = Substitute.For(); var removeOrganizationUserCommand = sutProvider.GetDependency(); var utcNow = DateTime.UtcNow; var savingUserId = Guid.NewGuid(); - await sutProvider.Sut.SaveAsync(policy, organizationService, savingUserId); + await sutProvider.Sut.SaveAsync(policy, savingUserId); await removeOrganizationUserCommand.Received() .RemoveUserAsync(policy.OrganizationId, orgUserDetailUserAcceptedWithout2FA.Id, savingUserId); @@ -468,13 +460,12 @@ public class PolicyServiceTests (orgUserDetailAdmin.UserId.Value, false), }); - var organizationService = Substitute.For(); var removeOrganizationUserCommand = sutProvider.GetDependency(); var savingUserId = Guid.NewGuid(); var badRequestException = await Assert.ThrowsAsync( - () => sutProvider.Sut.SaveAsync(policy, organizationService, savingUserId)); + () => sutProvider.Sut.SaveAsync(policy, 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); @@ -541,13 +532,11 @@ public class PolicyServiceTests (orgUserDetail.UserId.Value, false), }); - var organizationService = Substitute.For(); - var utcNow = DateTime.UtcNow; var savingUserId = Guid.NewGuid(); - await sutProvider.Sut.SaveAsync(policy, organizationService, savingUserId); + await sutProvider.Sut.SaveAsync(policy, savingUserId); await sutProvider.GetDependency().Received() .LogPolicyEventAsync(policy, EventType.Policy_Updated); @@ -590,7 +579,6 @@ public class PolicyServiceTests var badRequestException = await Assert.ThrowsAsync( () => sutProvider.Sut.SaveAsync(policy, - Substitute.For(), Guid.NewGuid())); Assert.Contains("Trusted device encryption is on and requires this policy.", badRequestException.Message, StringComparison.OrdinalIgnoreCase); @@ -626,7 +614,6 @@ public class PolicyServiceTests var badRequestException = await Assert.ThrowsAsync( () => sutProvider.Sut.SaveAsync(policy, - Substitute.For(), Guid.NewGuid())); Assert.Contains("Trusted device encryption is on and requires this policy.", badRequestException.Message, StringComparison.OrdinalIgnoreCase); @@ -659,7 +646,6 @@ public class PolicyServiceTests var badRequestException = await Assert.ThrowsAsync( () => sutProvider.Sut.SaveAsync(policy, - Substitute.For(), Guid.NewGuid())); Assert.Contains("Single Organization policy not enabled.", badRequestException.Message, StringComparison.OrdinalIgnoreCase); @@ -692,7 +678,6 @@ public class PolicyServiceTests var badRequestException = await Assert.ThrowsAsync( () => sutProvider.Sut.SaveAsync(policy, - Substitute.For(), Guid.NewGuid())); Assert.Contains("Account recovery policy is enabled.", badRequestException.Message, StringComparison.OrdinalIgnoreCase); diff --git a/test/Core.Test/Auth/Services/SsoConfigServiceTests.cs b/test/Core.Test/Auth/Services/SsoConfigServiceTests.cs index fb566537a..e397c838c 100644 --- a/test/Core.Test/Auth/Services/SsoConfigServiceTests.cs +++ b/test/Core.Test/Auth/Services/SsoConfigServiceTests.cs @@ -11,7 +11,6 @@ using Bit.Core.Auth.Services; using Bit.Core.Exceptions; using Bit.Core.Models.Data.Organizations.OrganizationUsers; using Bit.Core.Repositories; -using Bit.Core.Services; using Bit.Test.Common.AutoFixture; using Bit.Test.Common.AutoFixture.Attributes; using NSubstitute; @@ -342,14 +341,12 @@ public class SsoConfigServiceTests await sutProvider.GetDependency().Received(1) .SaveAsync( Arg.Is(t => t.Type == PolicyType.SingleOrg), - Arg.Any(), null ); await sutProvider.GetDependency().Received(1) .SaveAsync( Arg.Is(t => t.Type == PolicyType.ResetPassword && t.GetDataModel().AutoEnrollEnabled), - Arg.Any(), null );