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

[AC-2971] Draft: Policy strategy pattern #47

Open
wants to merge 2 commits 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
๏ปฟusing Bit.Core.AdminConsole.Entities;
using Bit.Core.AdminConsole.Enums;

namespace Bit.Core.AdminConsole.OrganizationFeatures.Policies;

public interface IPolicyStrategy
{
/// <summary>
/// The PolicyType that the strategy is responsible for handling.
/// </summary>
public PolicyType Type { get; }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Consider making this property read-only to ensure immutability


/// <summary>
/// A method that is called when the policy state changes from disabled to enabled, before
/// it is saved to the database.
/// For example, this can be used for validation before saving or for side effects.
/// </summary>
/// <param name="policy">The updated policy object.</param>
/// <param name="savingUserId">The current user who is updating the policy.</param>
public Task HandleEnable(Policy policy, Guid? savingUserId);

/// <summary>
/// A method that is called when the policy state changes from enabled to disabled, before
/// it is saved to the database.
/// For example, this can be used for validation before saving or for side effects.
/// </summary>
/// <param name="policy">The updated policy object.</param>
/// <param name="savingUserId">The current user who is updating the policy.</param>
public Task HandleDisable(Policy policy, Guid? savingUserId);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
๏ปฟ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.Enums;
using Bit.Core.Exceptions;
using Bit.Core.Repositories;
using Bit.Core.Services;

namespace Bit.Core.AdminConsole.OrganizationFeatures.Policies.Implementations;

public class SingleOrgPolicyStrategy : IPolicyStrategy
{
public PolicyType Type => PolicyType.SingleOrg;

private readonly IOrganizationUserRepository _organizationUserRepository;
private readonly IMailService _mailService;
private readonly IOrganizationRepository _organizationRepository;
private readonly IPolicyRepository _policyRepository;
private readonly ISsoConfigRepository _ssoConfigRepository;

public SingleOrgPolicyStrategy(
IOrganizationUserRepository organizationUserRepository,
IMailService mailService,
IOrganizationRepository organizationRepository,
IPolicyRepository policyRepository,
ISsoConfigRepository ssoConfigRepository)
{
_organizationUserRepository = organizationUserRepository;
_mailService = mailService;
_organizationRepository = organizationRepository;
_policyRepository = policyRepository;
_ssoConfigRepository = ssoConfigRepository;
}

public async Task HandleEnable(Policy policy, Guid? savingUserId)
{
// Remove non-compliant users
var orgUsers = await _organizationUserRepository.GetManyDetailsByOrganizationAsync(policy.OrganizationId);
var org = await _organizationRepository.GetByIdAsync(policy.OrganizationId);
if (org == null)
{
throw new NotFoundException("Organization not found.");
}

var removableOrgUsers = orgUsers.Where(ou =>
ou.Status != OrganizationUserStatusType.Invited &&
ou.Status != OrganizationUserStatusType.Revoked &&
ou.Type != OrganizationUserType.Owner &&
ou.Type != OrganizationUserType.Admin &&
ou.UserId != savingUserId
).ToList();

var userOrgs = await _organizationUserRepository.GetManyByManyUsersAsync(
removableOrgUsers.Select(ou => ou.UserId!.Value));
Comment on lines +55 to +56
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: This query might be inefficient for large numbers of users. Consider optimizing or paginating

foreach (var orgUser in removableOrgUsers)
{
if (userOrgs.Any(ou => ou.UserId == orgUser.UserId
&& ou.OrganizationId != org.Id
&& ou.Status != OrganizationUserStatusType.Invited))
{
// TODO: OrganizationService causes a circular dependency here, we either pass it into all handlers
// TODO: like we do with PolicyService at the moment, or we investigate and break that circle
// await _organizationService.DeleteUserAsync(policy.OrganizationId, orgUser.Id,
// savingUserId);
Comment on lines +63 to +66
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Circular dependency issue needs to be resolved before this code can be fully implemented

await _mailService.SendOrganizationUserRemovedForPolicySingleOrgEmailAsync(
org.DisplayName(), orgUser.Email);
}
}
}

public async Task HandleDisable(Policy policy, Guid? savingUserId)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: savingUserId parameter is unused in this method. Consider removing if not needed

{
// Do not allow this policy to be disabled if a dependent policy is still enabled
var policies = await _policyRepository.GetManyByOrganizationIdAsync(policy.OrganizationId);
if (policies.Any(p => p.Type == PolicyType.RequireSso && p.Enabled))
{
throw new BadRequestException("Single Sign-On Authentication policy is enabled.");
}

if (policies.Any(p => p.Type == PolicyType.MaximumVaultTimeout && p.Enabled))
{
throw new BadRequestException("Maximum Vault Timeout policy is enabled.");
}

if (policies.Any(p => p.Type == PolicyType.ResetPassword && p.Enabled))
{
throw new BadRequestException("Account Recovery policy is enabled.");
}
Comment on lines +76 to +90
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Consider refactoring repeated policy checks into a separate method for better maintainability


// Do not allow this policy to be disabled if Key Connector is being used
var ssoConfig = await _ssoConfigRepository.GetByOrganizationIdAsync(policy.OrganizationId);
if (ssoConfig?.GetData()?.MemberDecryptionType == MemberDecryptionType.KeyConnector)
{
throw new BadRequestException("Key Connector is enabled.");
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
๏ปฟusing Bit.Core.AdminConsole.OrganizationFeatures.Policies.Implementations;
using Bit.Core.AdminConsole.Services;
using Bit.Core.AdminConsole.Services.Implementations;
using Microsoft.Extensions.DependencyInjection;

namespace Bit.Core.AdminConsole.OrganizationFeatures.Policies;

public static class PolicyServiceCollectionExtensions
{
public static void AddPolicyServices(this IServiceCollection services)
{
services.AddScoped<IPolicyService, PolicyService>();
services.AddScoped<IPolicyStrategy, SingleOrgPolicyStrategy>();
}
}
Loading