Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโ€™ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PM-13836] Refactor IPolicyService to remove unnecessary IOrganizationService dependency #3

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions src/Api/AdminConsole/Controllers/PoliciesController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -36,7 +35,6 @@ public class PoliciesController : Controller
public PoliciesController(
IPolicyRepository policyRepository,
IPolicyService policyService,
IOrganizationService organizationService,
IOrganizationUserRepository organizationUserRepository,
IUserService userService,
ICurrentContext currentContext,
Expand All @@ -46,7 +44,6 @@ public PoliciesController(
{
_policyRepository = policyRepository;
_policyService = policyService;
_organizationService = organizationService;
_organizationUserRepository = organizationUserRepository;
_userService = userService;
_currentContext = currentContext;
Expand Down Expand Up @@ -185,7 +182,7 @@ public async Task<PolicyResponseModel> Put(string orgId, int type, [FromBody] Po
}

var userId = _userService.GetProperUserId(User);
await _policyService.SaveAsync(policy, _organizationService, userId);
await _policyService.SaveAsync(policy, userId);
return new PolicyResponseModel(policy);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
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;

Expand All @@ -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;
}

Expand Down Expand Up @@ -96,7 +92,7 @@ public async Task<IActionResult> Put(PolicyType type, [FromBody] PolicyUpdateReq
{
policy = model.ToPolicy(policy);
}
await _policyService.SaveAsync(policy, _organizationService, null);
await _policyService.SaveAsync(policy, null);
var response = new PolicyResponseModel(policy);
return new JsonResult(response);
}
Expand Down
3 changes: 1 addition & 2 deletions src/Core/AdminConsole/Services/IPolicyService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,12 @@
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);

/// <summary>
/// Get the combined master password policy options for the specified user.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public PolicyService(
_removeOrganizationUserCommand = removeOrganizationUserCommand;
}

public async Task SaveAsync(Policy policy, IOrganizationService organizationService, Guid? savingUserId)
public async Task SaveAsync(Policy policy, Guid? savingUserId)
{
var org = await _organizationRepository.GetByIdAsync(policy.OrganizationId);
if (org == null)
Expand Down Expand Up @@ -88,7 +88,7 @@ public async Task SaveAsync(Policy policy, IOrganizationService organizationServ
return;
}

await EnablePolicyAsync(policy, org, organizationService, savingUserId);
await EnablePolicyAsync(policy, org, savingUserId);
}

public async Task<MasterPasswordPolicyData> GetMasterPasswordPolicyForUserAsync(User user)
Expand Down Expand Up @@ -262,7 +262,7 @@ private async Task SetPolicyConfiguration(Policy policy)
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)
Expand Down
9 changes: 3 additions & 6 deletions src/Core/Auth/Services/Implementations/SsoConfigService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -29,15 +28,13 @@ public SsoConfigService(
IPolicyService policyService,
IOrganizationRepository organizationRepository,
IOrganizationUserRepository organizationUserRepository,
IOrganizationService organizationService,
IEventService eventService)
{
_ssoConfigRepository = ssoConfigRepository;
_policyRepository = policyRepository;
_policyService = policyService;
_organizationRepository = organizationRepository;
_organizationUserRepository = organizationUserRepository;
_organizationService = organizationService;
_eventService = eventService;
}

Expand Down Expand Up @@ -71,20 +68,20 @@ public async Task SaveAsync(SsoConfig config, Organization organization)

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);
Expand Down
23 changes: 4 additions & 19 deletions test/Core.Test/AdminConsole/Services/PolicyServiceTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ public async Task SaveAsync_OrganizationDoesNotExist_ThrowsBadRequest(

var badRequestException = await Assert.ThrowsAsync<BadRequestException>(
() => sutProvider.Sut.SaveAsync(policy,
Substitute.For<IOrganizationService>(),
Guid.NewGuid()));

Assert.Contains("Organization not found", badRequestException.Message, StringComparison.OrdinalIgnoreCase);
Expand All @@ -61,7 +60,6 @@ public async Task SaveAsync_OrganizationCannotUsePolicies_ThrowsBadRequest(

var badRequestException = await Assert.ThrowsAsync<BadRequestException>(
() => sutProvider.Sut.SaveAsync(policy,
Substitute.For<IOrganizationService>(),
Guid.NewGuid()));

Assert.Contains("cannot use policies", badRequestException.Message, StringComparison.OrdinalIgnoreCase);
Expand Down Expand Up @@ -93,7 +91,6 @@ public async Task SaveAsync_SingleOrg_RequireSsoEnabled_ThrowsBadRequest(

var badRequestException = await Assert.ThrowsAsync<BadRequestException>(
() => sutProvider.Sut.SaveAsync(policy,
Substitute.For<IOrganizationService>(),
Guid.NewGuid()));

Assert.Contains("Single Sign-On Authentication policy is enabled.", badRequestException.Message, StringComparison.OrdinalIgnoreCase);
Expand Down Expand Up @@ -124,7 +121,6 @@ public async Task SaveAsync_SingleOrg_VaultTimeoutEnabled_ThrowsBadRequest([Admi

var badRequestException = await Assert.ThrowsAsync<BadRequestException>(
() => sutProvider.Sut.SaveAsync(policy,
Substitute.For<IOrganizationService>(),
Guid.NewGuid()));

Assert.Contains("Maximum Vault Timeout policy is enabled.", badRequestException.Message, StringComparison.OrdinalIgnoreCase);
Expand Down Expand Up @@ -161,7 +157,6 @@ public async Task SaveAsync_PolicyRequiredByKeyConnector_DisablePolicy_ThrowsBad

var badRequestException = await Assert.ThrowsAsync<BadRequestException>(
() => sutProvider.Sut.SaveAsync(policy,
Substitute.For<IOrganizationService>(),
Guid.NewGuid()));

Assert.Contains("Key Connector is enabled.", badRequestException.Message, StringComparison.OrdinalIgnoreCase);
Expand Down Expand Up @@ -189,7 +184,6 @@ public async Task SaveAsync_RequireSsoPolicy_NotEnabled_ThrowsBadRequestAsync(

var badRequestException = await Assert.ThrowsAsync<BadRequestException>(
() => sutProvider.Sut.SaveAsync(policy,
Substitute.For<IOrganizationService>(),
Guid.NewGuid()));

Assert.Contains("Single Organization policy not enabled.", badRequestException.Message, StringComparison.OrdinalIgnoreCase);
Expand Down Expand Up @@ -222,7 +216,7 @@ public async Task SaveAsync_NewPolicy_Created(

var utcNow = DateTime.UtcNow;

await sutProvider.Sut.SaveAsync(policy, Substitute.For<IOrganizationService>(), Guid.NewGuid());
await sutProvider.Sut.SaveAsync(policy, Guid.NewGuid());

await sutProvider.GetDependency<IEventService>().Received()
.LogPolicyEventAsync(policy, EventType.Policy_Updated);
Expand Down Expand Up @@ -252,7 +246,6 @@ public async Task SaveAsync_VaultTimeoutPolicy_NotEnabled_ThrowsBadRequestAsync(

var badRequestException = await Assert.ThrowsAsync<BadRequestException>(
() => sutProvider.Sut.SaveAsync(policy,
Substitute.For<IOrganizationService>(),
Guid.NewGuid()));

Assert.Contains("Single Organization policy not enabled.", badRequestException.Message, StringComparison.OrdinalIgnoreCase);
Expand Down Expand Up @@ -353,14 +346,13 @@ public async Task SaveAsync_ExistingPolicy_UpdateTwoFactor(
(orgUserDetailAdmin, false),
});

var organizationService = Substitute.For<IOrganizationService>();
var removeOrganizationUserCommand = sutProvider.GetDependency<IRemoveOrganizationUserCommand>();

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);
Expand Down Expand Up @@ -468,13 +460,12 @@ public async Task SaveAsync_EnableTwoFactor_WithoutMasterPasswordOr2FA_ThrowsBad
(orgUserDetailAdmin.UserId.Value, false),
});

var organizationService = Substitute.For<IOrganizationService>();
var removeOrganizationUserCommand = sutProvider.GetDependency<IRemoveOrganizationUserCommand>();

var savingUserId = Guid.NewGuid();

var badRequestException = await Assert.ThrowsAsync<BadRequestException>(
() => 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);

Expand Down Expand Up @@ -541,13 +532,11 @@ public async Task SaveAsync_ExistingPolicy_UpdateSingleOrg(
(orgUserDetail.UserId.Value, false),
});

var organizationService = Substitute.For<IOrganizationService>();

var utcNow = DateTime.UtcNow;

var savingUserId = Guid.NewGuid();

await sutProvider.Sut.SaveAsync(policy, organizationService, savingUserId);
await sutProvider.Sut.SaveAsync(policy, savingUserId);

await sutProvider.GetDependency<IEventService>().Received()
.LogPolicyEventAsync(policy, EventType.Policy_Updated);
Expand Down Expand Up @@ -590,7 +579,6 @@ public async Task SaveAsync_ResetPasswordPolicyRequiredByTrustedDeviceEncryption

var badRequestException = await Assert.ThrowsAsync<BadRequestException>(
() => sutProvider.Sut.SaveAsync(policy,
Substitute.For<IOrganizationService>(),
Guid.NewGuid()));

Assert.Contains("Trusted device encryption is on and requires this policy.", badRequestException.Message, StringComparison.OrdinalIgnoreCase);
Expand Down Expand Up @@ -626,7 +614,6 @@ public async Task SaveAsync_RequireSsoPolicyRequiredByTrustedDeviceEncryption_Di

var badRequestException = await Assert.ThrowsAsync<BadRequestException>(
() => sutProvider.Sut.SaveAsync(policy,
Substitute.For<IOrganizationService>(),
Guid.NewGuid()));

Assert.Contains("Trusted device encryption is on and requires this policy.", badRequestException.Message, StringComparison.OrdinalIgnoreCase);
Expand Down Expand Up @@ -659,7 +646,6 @@ public async Task SaveAsync_PolicyRequiredForAccountRecovery_NotEnabled_ThrowsBa

var badRequestException = await Assert.ThrowsAsync<BadRequestException>(
() => sutProvider.Sut.SaveAsync(policy,
Substitute.For<IOrganizationService>(),
Guid.NewGuid()));

Assert.Contains("Single Organization policy not enabled.", badRequestException.Message, StringComparison.OrdinalIgnoreCase);
Expand Down Expand Up @@ -692,7 +678,6 @@ public async Task SaveAsync_SingleOrg_AccountRecoveryEnabled_ThrowsBadRequest(

var badRequestException = await Assert.ThrowsAsync<BadRequestException>(
() => sutProvider.Sut.SaveAsync(policy,
Substitute.For<IOrganizationService>(),
Guid.NewGuid()));

Assert.Contains("Account recovery policy is enabled.", badRequestException.Message, StringComparison.OrdinalIgnoreCase);
Expand Down
3 changes: 0 additions & 3 deletions test/Core.Test/Auth/Services/SsoConfigServiceTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
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;
Expand Down Expand Up @@ -342,14 +341,12 @@ public async Task SaveAsync_Tde_Enable_Required_Policies(SutProvider<SsoConfigSe
await sutProvider.GetDependency<IPolicyService>().Received(1)
.SaveAsync(
Arg.Is<Policy>(t => t.Type == PolicyType.SingleOrg),
Arg.Any<IOrganizationService>(),
null
);

await sutProvider.GetDependency<IPolicyService>().Received(1)
.SaveAsync(
Arg.Is<Policy>(t => t.Type == PolicyType.ResetPassword && t.GetDataModel<ResetPasswordDataModel>().AutoEnrollEnabled),
Arg.Any<IOrganizationService>(),
null
);

Expand Down
Loading