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-13014] - Add CanToggleStatus property to PolicyRepsonseModel based on Policy Validators #4940

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
64 changes: 12 additions & 52 deletions src/Api/AdminConsole/Controllers/PoliciesController.cs
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
๏ปฟusing Bit.Api.AdminConsole.Models.Request;
using Bit.Api.AdminConsole.Models.Response.Helpers;
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.Policies;
using Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models;
using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationDomains.Interfaces;
using Bit.Core.AdminConsole.Repositories;
using Bit.Core.AdminConsole.Services;
using Bit.Core.Auth.Models.Business.Tokenables;
Expand All @@ -19,7 +20,6 @@
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.DataProtection;
using Microsoft.AspNetCore.Mvc;
using AdminConsoleEntities = Bit.Core.AdminConsole.Entities;

namespace Bit.Api.AdminConsole.Controllers;

Expand All @@ -36,7 +36,7 @@
private readonly IDataProtector _organizationServiceDataProtector;
private readonly IDataProtectorTokenFactory<OrgUserInviteTokenable> _orgUserInviteTokenDataFactory;
private readonly IFeatureService _featureService;
private readonly IReadOnlyDictionary<PolicyType, IPolicyValidator> _policyValidators;
private readonly IOrganizationHasVerifiedDomainsQuery _organizationHasVerifiedDomainsQuery;

public PoliciesController(
IPolicyRepository policyRepository,
Expand All @@ -48,7 +48,7 @@
IDataProtectionProvider dataProtectionProvider,
IDataProtectorTokenFactory<OrgUserInviteTokenable> orgUserInviteTokenDataFactory,
IFeatureService featureService,
IEnumerable<IPolicyValidator> validators)
IOrganizationHasVerifiedDomainsQuery organizationHasVerifiedDomainsQuery)
{
_policyRepository = policyRepository;
_policyService = policyService;
Expand All @@ -61,17 +61,11 @@

_orgUserInviteTokenDataFactory = orgUserInviteTokenDataFactory;
_featureService = featureService;

var dictionary = new Dictionary<PolicyType, IPolicyValidator>();
foreach (var validator in validators)
{
dictionary.TryAdd(validator.Type, validator);
}
_policyValidators = dictionary;
_organizationHasVerifiedDomainsQuery = organizationHasVerifiedDomainsQuery;
}

[HttpGet("{type}")]
public async Task<PolicyResponseModel> Get(Guid orgId, int type)
public async Task<PolicyDetailResponseModel> Get(Guid orgId, int type)
{
if (!await _currentContext.ManagePolicies(orgId))
{
Expand All @@ -80,26 +74,15 @@
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 });
}

if (_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning))
if (_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning) && policy.Type is PolicyType.SingleOrg)
{
var canToggle = _policyValidators.ContainsKey(policy.Type) && string.IsNullOrWhiteSpace(
await _policyValidators[policy.Type]
.ValidateAsync(
new PolicyUpdate
{
Data = policy.Data,
Enabled = !policy.Enabled,
OrganizationId = policy.OrganizationId,
Type = policy.Type
}, policy));

return new PolicyResponseModel(policy, canToggle);
return await PolicyDetailResponses.GetSingleOrgPolicyDetailResponseAsync(policy, _organizationHasVerifiedDomainsQuery);

Check warning on line 82 in src/Api/AdminConsole/Controllers/PoliciesController.cs

View check run for this annotation

Codecov / codecov/patch

src/Api/AdminConsole/Controllers/PoliciesController.cs#L81-L82

Added lines #L81 - L82 were not covered by tests
}

return new PolicyResponseModel(policy);
return new PolicyDetailResponseModel(policy);
}

[HttpGet("")]
Expand All @@ -113,30 +96,7 @@

var policies = await _policyRepository.GetManyByOrganizationIdAsync(orgIdGuid);

if (!_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning))
{
return new ListResponseModel<PolicyResponseModel>(policies.Select(p => new PolicyResponseModel(p)));
}

var responses = new List<PolicyResponseModel>();

foreach (var policy in policies)
{
var canToggle = _policyValidators.ContainsKey(policy.Type) && string.IsNullOrWhiteSpace(
await _policyValidators[policy.Type]
.ValidateAsync(
new PolicyUpdate
{
Data = policy.Data,
Enabled = !policy.Enabled,
OrganizationId = policy.OrganizationId,
Type = policy.Type
}, policy));

responses.Add(new PolicyResponseModel(policy, canToggle));
}

return new ListResponseModel<PolicyResponseModel>(responses);
return new ListResponseModel<PolicyResponseModel>(policies.Select(p => new PolicyResponseModel(p)));

Check warning on line 99 in src/Api/AdminConsole/Controllers/PoliciesController.cs

View check run for this annotation

Codecov / codecov/patch

src/Api/AdminConsole/Controllers/PoliciesController.cs#L99

Added line #L99 was not covered by tests
}

[AllowAnonymous]
Expand Down
Copy link
Member

Choose a reason for hiding this comment

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

No changes required, but I am interested in the decision to use an extension method here vs. a static method on the class itself. I can make some guesses but would like to hear your thoughts.

Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
๏ปฟusing Bit.Core.AdminConsole.Entities;
using Bit.Core.AdminConsole.Enums;
using Bit.Core.AdminConsole.Models.Api.Response;
using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationDomains.Interfaces;

namespace Bit.Api.AdminConsole.Models.Response.Helpers;

public static class PolicyDetailResponses
{
public static async Task<PolicyDetailResponseModel> GetSingleOrgPolicyDetailResponseAsync(Policy policy, IOrganizationHasVerifiedDomainsQuery hasVerifiedDomainsQuery)
{

Check warning on line 11 in src/Api/AdminConsole/Models/Response/Helpers/PolicyDetailResponses.cs

View check run for this annotation

Codecov / codecov/patch

src/Api/AdminConsole/Models/Response/Helpers/PolicyDetailResponses.cs#L11

Added line #L11 was not covered by tests
if (policy.Type is not PolicyType.SingleOrg)
{
throw new ArgumentException($"'{nameof(policy)}' must be of type '{nameof(PolicyType.SingleOrg)}'.", nameof(policy));

Check warning on line 14 in src/Api/AdminConsole/Models/Response/Helpers/PolicyDetailResponses.cs

View check run for this annotation

Codecov / codecov/patch

src/Api/AdminConsole/Models/Response/Helpers/PolicyDetailResponses.cs#L13-L14

Added lines #L13 - L14 were not covered by tests
}

return new PolicyDetailResponseModel(policy, await hasVerifiedDomainsQuery.HasVerifiedDomainsAsync(policy.OrganizationId));
Copy link
Member

@eliykat eliykat Nov 1, 2024

Choose a reason for hiding this comment

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

I think the bool needs to be inverted:

  • has verified domains => can't toggle state
  • does not have verified domains => can toggle state

(Probably a drawback of a bool that defaults to true, I didn't realise that when I suggested the naming. But no need to spend more time on it.)

}

Check warning on line 18 in src/Api/AdminConsole/Models/Response/Helpers/PolicyDetailResponses.cs

View check run for this annotation

Codecov / codecov/patch

src/Api/AdminConsole/Models/Response/Helpers/PolicyDetailResponses.cs#L17-L18

Added lines #L17 - L18 were not covered by tests
}
67 changes: 2 additions & 65 deletions src/Api/AdminConsole/Public/Controllers/PoliciesController.cs
jrmccannon marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,10 @@
using Bit.Api.AdminConsole.Public.Models.Request;
using Bit.Api.AdminConsole.Public.Models.Response;
using Bit.Api.Models.Public.Response;
using Bit.Core;
using Bit.Core.AdminConsole.Enums;
using Bit.Core.AdminConsole.OrganizationFeatures.Policies;
using Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models;
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 @@ -22,27 +18,15 @@
private readonly IPolicyRepository _policyRepository;
private readonly IPolicyService _policyService;
private readonly ICurrentContext _currentContext;
private readonly IFeatureService _featureService;
private readonly IReadOnlyDictionary<PolicyType, IPolicyValidator> _policyValidators;

public PoliciesController(
IPolicyRepository policyRepository,
IPolicyService policyService,
ICurrentContext currentContext,
IFeatureService featureService,
IEnumerable<IPolicyValidator> policyValidators)
ICurrentContext currentContext)
{
_policyRepository = policyRepository;
_policyService = policyService;
_currentContext = currentContext;
_featureService = featureService;

var dictionary = new Dictionary<PolicyType, IPolicyValidator>();
foreach (var validator in policyValidators)
{
dictionary.TryAdd(validator.Type, validator);
}
_policyValidators = dictionary;
}

/// <summary>
Expand All @@ -57,32 +41,13 @@
[ProducesResponseType((int)HttpStatusCode.NotFound)]
public async Task<IActionResult> Get(PolicyType type)
{
var policy = await _policyRepository.GetByOrganizationIdTypeAsync(_currentContext.OrganizationId.Value, type);

Check warning on line 44 in src/Api/AdminConsole/Public/Controllers/PoliciesController.cs

View check run for this annotation

Codecov / codecov/patch

src/Api/AdminConsole/Public/Controllers/PoliciesController.cs#L44

Added line #L44 was not covered by tests
if (policy == null)
{
return new NotFoundResult();
}

if (_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning))
{
if (policy.Type == PolicyType.SingleOrg)
{
var canToggle = _policyValidators.ContainsKey(policy.Type) && string.IsNullOrWhiteSpace(
await _policyValidators[policy.Type]
.ValidateAsync(
new PolicyUpdate
{
Data = policy.Data,
Enabled = !policy.Enabled,
OrganizationId = policy.OrganizationId,
Type = policy.Type
}, policy));

return new JsonResult(new PolicyResponseModel(policy, canToggle));
}
}

return new JsonResult(new PolicyResponseModel(policy));

Check warning on line 50 in src/Api/AdminConsole/Public/Controllers/PoliciesController.cs

View check run for this annotation

Codecov / codecov/patch

src/Api/AdminConsole/Public/Controllers/PoliciesController.cs#L50

Added line #L50 was not covered by tests
}

/// <summary>
Expand All @@ -97,35 +62,7 @@
{
var policies = await _policyRepository.GetManyByOrganizationIdAsync(_currentContext.OrganizationId.Value);

if (!_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning))
{
return new JsonResult(new ListResponseModel<PolicyResponseModel>(policies.Select(p => new PolicyResponseModel(p))));
}

var responses = new List<PolicyResponseModel>();

foreach (var policy in policies)
{
if (policy.Type == PolicyType.SingleOrg)
{
var canToggle = _policyValidators.ContainsKey(policy.Type) && string.IsNullOrWhiteSpace(
await _policyValidators[policy.Type]
.ValidateAsync(
new PolicyUpdate
{
Data = policy.Data,
Enabled = !policy.Enabled,
OrganizationId = policy.OrganizationId,
Type = policy.Type
}, policy));

responses.Add(new PolicyResponseModel(policy, canToggle));
}

responses.Add(new PolicyResponseModel(policy));
}

return new JsonResult(new ListResponseModel<PolicyResponseModel>(responses));
return new JsonResult(new ListResponseModel<PolicyResponseModel>(policies.Select(p => new PolicyResponseModel(p))));

Check warning on line 65 in src/Api/AdminConsole/Public/Controllers/PoliciesController.cs

View check run for this annotation

Codecov / codecov/patch

src/Api/AdminConsole/Public/Controllers/PoliciesController.cs#L65

Added line #L65 was not covered by tests
}

/// <summary>
Expand Down
11 changes: 0 additions & 11 deletions src/Api/AdminConsole/Public/Models/Response/PolicyResponseModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,6 @@ public PolicyResponseModel(Policy policy)
}
}

public PolicyResponseModel(Policy policy, bool canToggleState) : this(policy)
{
CanToggleState = canToggleState;
}

/// <summary>
/// String representing the object's type. Objects of the same type share the same properties.
/// </summary>
Expand All @@ -49,10 +44,4 @@ public PolicyResponseModel(Policy policy, bool canToggleState) : this(policy)
/// </summary>
[Required]
public PolicyType? Type { get; set; }

/// <summary>
/// Indicates whether the Policy can be enabled/disabled
/// </summary>
[Required]
public bool CanToggleState { get; set; }
}
Copy link
Member

Choose a reason for hiding this comment

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

Api request/response models live in the Api project.

Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
๏ปฟusing Bit.Core.AdminConsole.Entities;

namespace Bit.Core.AdminConsole.Models.Api.Response;

public class PolicyDetailResponseModel : PolicyResponseModel
{
public PolicyDetailResponseModel(Policy policy, string obj = "policy") : base(policy, obj)
{
}

public PolicyDetailResponseModel(Policy policy, bool canToggleState) : base(policy)
{
CanToggleState = canToggleState;
}

Check warning on line 14 in src/Core/AdminConsole/Models/Api/Response/PolicyDetailResponseModel.cs

View check run for this annotation

Codecov / codecov/patch

src/Core/AdminConsole/Models/Api/Response/PolicyDetailResponseModel.cs#L11-L14

Added lines #L11 - L14 were not covered by tests

/// <summary>
/// Indicates whether the Policy can be enabled/disabled
/// </summary>
public bool CanToggleState { get; set; } = true;
}
10 changes: 0 additions & 10 deletions src/Core/AdminConsole/Models/Api/Response/PolicyResponseModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,19 +25,9 @@ public PolicyResponseModel(Policy policy, string obj = "policy")
}
}

public PolicyResponseModel(Policy policy, bool canToggleState) : this(policy)
{
CanToggleState = canToggleState;
}

public Guid Id { get; set; }
public Guid OrganizationId { get; set; }
public PolicyType Type { get; set; }
public Dictionary<string, object> Data { get; set; }
public bool Enabled { get; set; }

/// <summary>
/// Indicates whether the Policy can be enabled/disabled
/// </summary>
public bool CanToggleState { get; set; } = true;
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
๏ปฟ#nullable enable

using System.Text;
using Bit.Core.AdminConsole.Entities;
using Bit.Core.AdminConsole.Enums;
using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationDomains.Interfaces;
Expand Down Expand Up @@ -100,18 +99,20 @@
{
if (policyUpdate is not { Enabled: true })
{
var resultString = new StringBuilder();

var ssoConfig = await _ssoConfigRepository.GetByOrganizationIdAsync(policyUpdate.OrganizationId);
resultString.Append(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))
{
resultString.Append("The Single organization policy is required for organizations that have enabled domain verification.");
return "The Single organization policy is required for organizations that have enabled domain verification.";

Check warning on line 114 in src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs

View check run for this annotation

Codecov / codecov/patch

src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs#L113-L114

Added lines #L113 - L114 were not covered by tests
}

return resultString.ToString();
}

return string.Empty;
Expand Down
4 changes: 2 additions & 2 deletions test/Api.Test/Controllers/PoliciesControllerTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ public async Task Get_WhenUserCanManagePolicies_WithExistingType_ReturnsExisting
var result = await sutProvider.Sut.Get(orgId, type);

// Assert
Assert.IsType<PolicyResponseModel>(result);
Assert.IsType<PolicyDetailResponseModel>(result);
Assert.Equal(policy.Id, result.Id);
Assert.Equal(policy.Type, result.Type);
Assert.Equal(policy.Enabled, result.Enabled);
Expand All @@ -182,7 +182,7 @@ public async Task Get_WhenUserCanManagePolicies_WithNonExistingType_ReturnsDefau
var result = await sutProvider.Sut.Get(orgId, type);

// Assert
Assert.IsType<PolicyResponseModel>(result);
Assert.IsType<PolicyDetailResponseModel>(result);
Assert.Equal(result.Type, (PolicyType)type);
Assert.False(result.Enabled);
}
Expand Down
Loading