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
28 changes: 21 additions & 7 deletions src/Api/AdminConsole/Controllers/PoliciesController.cs
Original file line number Diff line number Diff line change
@@ -1,7 +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.OrganizationDomains.Interfaces;
using Bit.Core.AdminConsole.Repositories;
using Bit.Core.AdminConsole.Services;
using Bit.Core.Auth.Models.Business.Tokenables;
Expand All @@ -16,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 @@ -32,6 +35,8 @@
private readonly GlobalSettings _globalSettings;
private readonly IDataProtector _organizationServiceDataProtector;
private readonly IDataProtectorTokenFactory<OrgUserInviteTokenable> _orgUserInviteTokenDataFactory;
private readonly IFeatureService _featureService;
private readonly IOrganizationHasVerifiedDomainsQuery _organizationHasVerifiedDomainsQuery;

public PoliciesController(
IPolicyRepository policyRepository,
Expand All @@ -41,7 +46,9 @@
ICurrentContext currentContext,
GlobalSettings globalSettings,
IDataProtectionProvider dataProtectionProvider,
IDataProtectorTokenFactory<OrgUserInviteTokenable> orgUserInviteTokenDataFactory)
IDataProtectorTokenFactory<OrgUserInviteTokenable> orgUserInviteTokenDataFactory,
IFeatureService featureService,
IOrganizationHasVerifiedDomainsQuery organizationHasVerifiedDomainsQuery)
{
_policyRepository = policyRepository;
_policyService = policyService;
Expand All @@ -53,10 +60,12 @@
"OrganizationServiceDataProtector");

_orgUserInviteTokenDataFactory = orgUserInviteTokenDataFactory;
_featureService = featureService;
_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 @@ -65,10 +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 });
}

return new PolicyResponseModel(policy);
if (_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning) && policy.Type is PolicyType.SingleOrg)
{
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 PolicyDetailResponseModel(policy);
}

[HttpGet("")]
Expand All @@ -81,8 +95,8 @@
}

var policies = await _policyRepository.GetManyByOrganizationIdAsync(orgIdGuid);
var responses = policies.Select(p => new PolicyResponseModel(p));
jrmccannon marked this conversation as resolved.
Show resolved Hide resolved
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
}
12 changes: 5 additions & 7 deletions src/Api/AdminConsole/Public/Controllers/PoliciesController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,13 @@
[ProducesResponseType((int)HttpStatusCode.NotFound)]
public async Task<IActionResult> Get(PolicyType type)
{
var policy = await _policyRepository.GetByOrganizationIdTypeAsync(
_currentContext.OrganizationId.Value, 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();
}
var response = new PolicyResponseModel(policy);
return new JsonResult(response);

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 @@ -62,9 +61,8 @@
public async Task<IActionResult> List()
{
var policies = await _policyRepository.GetManyByOrganizationIdAsync(_currentContext.OrganizationId.Value);
var policyResponses = policies.Select(p => new PolicyResponseModel(p));
var response = new ListResponseModel<PolicyResponseModel>(policyResponses);
return new JsonResult(response);

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
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;
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ public class VerifyOrganizationDomainCommand : IVerifyOrganizationDomainCommand
private readonly IGlobalSettings _globalSettings;
private readonly IPolicyService _policyService;
private readonly IFeatureService _featureService;
private readonly IOrganizationService _organizationService;
private readonly ILogger<VerifyOrganizationDomainCommand> _logger;

public VerifyOrganizationDomainCommand(
Expand All @@ -30,7 +29,6 @@ public VerifyOrganizationDomainCommand(
IGlobalSettings globalSettings,
IPolicyService policyService,
IFeatureService featureService,
IOrganizationService organizationService,
ILogger<VerifyOrganizationDomainCommand> logger)
{
_organizationDomainRepository = organizationDomainRepository;
Expand All @@ -39,7 +37,6 @@ public VerifyOrganizationDomainCommand(
_globalSettings = globalSettings;
_policyService = policyService;
_featureService = featureService;
_organizationService = organizationService;
_logger = logger;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,7 @@ private async Task RunValidatorAsync(IPolicyValidator validator, PolicyUpdate po
if (currentPolicy is not { Enabled: true } && policyUpdate.Enabled)
{
var missingRequiredPolicyTypes = validator.RequiredPolicies
.Where(requiredPolicyType =>
savedPoliciesDict.GetValueOrDefault(requiredPolicyType) is not { Enabled: true })
.Where(requiredPolicyType => savedPoliciesDict.GetValueOrDefault(requiredPolicyType) is not { Enabled: true })
.ToList();

if (missingRequiredPolicyTypes.Count != 0)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

using Bit.Core.AdminConsole.Entities;
using Bit.Core.AdminConsole.Enums;
using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationDomains.Interfaces;
using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces;
using Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models;
using Bit.Core.Auth.Enums;
Expand All @@ -23,22 +24,28 @@
private readonly IOrganizationRepository _organizationRepository;
private readonly ISsoConfigRepository _ssoConfigRepository;
private readonly ICurrentContext _currentContext;
private readonly IFeatureService _featureService;
private readonly IRemoveOrganizationUserCommand _removeOrganizationUserCommand;
private readonly IOrganizationHasVerifiedDomainsQuery _organizationHasVerifiedDomainsQuery;

public SingleOrgPolicyValidator(
IOrganizationUserRepository organizationUserRepository,
IMailService mailService,
IOrganizationRepository organizationRepository,
ISsoConfigRepository ssoConfigRepository,
ICurrentContext currentContext,
IRemoveOrganizationUserCommand removeOrganizationUserCommand)
IFeatureService featureService,
IRemoveOrganizationUserCommand removeOrganizationUserCommand,
IOrganizationHasVerifiedDomainsQuery organizationHasVerifiedDomainsQuery)
{
_organizationUserRepository = organizationUserRepository;
_mailService = mailService;
_organizationRepository = organizationRepository;
_ssoConfigRepository = ssoConfigRepository;
_currentContext = currentContext;
_featureService = featureService;
_removeOrganizationUserCommand = removeOrganizationUserCommand;
_organizationHasVerifiedDomainsQuery = organizationHasVerifiedDomainsQuery;
}

public IEnumerable<PolicyType> RequiredPolicies => [];
Expand Down Expand Up @@ -93,9 +100,21 @@
if (policyUpdate is not { Enabled: true })
{
var ssoConfig = await _ssoConfigRepository.GetByOrganizationIdAsync(policyUpdate.OrganizationId);
return 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))
{
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 "";
return string.Empty;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ private async Task HasVerifiedDomainsAsync(Organization org)
if (_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning)
&& await _organizationHasVerifiedDomainsQuery.HasVerifiedDomainsAsync(org.Id))
{
throw new BadRequestException("Organization has verified domains.");
throw new BadRequestException("The Single organization policy is required for organizations that have enabled domain verification.");
}
}

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
Original file line number Diff line number Diff line change
Expand Up @@ -842,6 +842,6 @@ public async Task SaveAsync_GivenOrganizationUsingPoliciesAndHasVerifiedDomains_
var badRequestException = await Assert.ThrowsAsync<BadRequestException>(
() => sutProvider.Sut.SaveAsync(policy, null));

Assert.Equal("Organization has verified domains.", badRequestException.Message);
Assert.Equal("The Single organization policy is required for organizations that have enabled domain verification.", badRequestException.Message);
}
}
Loading