From 4748b5b3fc53aae32b7d42d960d582cef91bf352 Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Mon, 28 Aug 2023 08:05:23 +1000 Subject: [PATCH] [AC-1568] Finish refactor of update subscription interface, minor fixes (#3189) * Remove UpdateSecretsManagerSubscriptionCommand.AdjustServiceAccounts interface * Rewrite tests and use autofixture customizations * Reduce method nesting in the command, simplify parameters * Fix capitalization and wording of error messages * Add checks for existing SM beta enrolment etc --- .../OrganizationUsersController.cs | 4 +- ...tsManagerSubscriptionUpdateRequestModel.cs | 13 +- .../Controllers/ServiceAccountsController.cs | 6 +- .../SecretsManagerSubscriptionUpdate.cs | 31 ++- .../AddSecretsManagerSubscriptionCommand.cs | 11 ++ ...UpdateSecretsManagerSubscriptionCommand.cs | 4 +- ...UpdateSecretsManagerSubscriptionCommand.cs | 187 +++++++----------- .../Implementations/OrganizationService.cs | 8 +- .../ServiceAccountsControllerTests.cs | 10 +- .../AutoFixture/OrganizationFixtures.cs | 1 + ...dSecretsManagerSubscriptionCommandTests.cs | 32 +++ ...eSecretsManagerSubscriptionCommandTests.cs | 65 +++--- 12 files changed, 190 insertions(+), 182 deletions(-) diff --git a/src/Api/Controllers/OrganizationUsersController.cs b/src/Api/Controllers/OrganizationUsersController.cs index 6d1424f01..3dbaea3ff 100644 --- a/src/Api/Controllers/OrganizationUsersController.cs +++ b/src/Api/Controllers/OrganizationUsersController.cs @@ -447,8 +447,8 @@ public class OrganizationUsersController : Controller if (additionalSmSeatsRequired > 0) { var organization = await _organizationRepository.GetByIdAsync(orgId); - var update = new SecretsManagerSubscriptionUpdate(organization, true); - update.AdjustSeats(additionalSmSeatsRequired); + var update = new SecretsManagerSubscriptionUpdate(organization, true) + .AdjustSeats(additionalSmSeatsRequired); await _updateSecretsManagerSubscriptionCommand.UpdateSubscriptionAsync(update); } diff --git a/src/Api/Models/Request/Organizations/SecretsManagerSubscriptionUpdateRequestModel.cs b/src/Api/Models/Request/Organizations/SecretsManagerSubscriptionUpdateRequestModel.cs index a8adbe92e..96e9603fc 100644 --- a/src/Api/Models/Request/Organizations/SecretsManagerSubscriptionUpdateRequestModel.cs +++ b/src/Api/Models/Request/Organizations/SecretsManagerSubscriptionUpdateRequestModel.cs @@ -14,11 +14,12 @@ public class SecretsManagerSubscriptionUpdateRequestModel public virtual SecretsManagerSubscriptionUpdate ToSecretsManagerSubscriptionUpdate(Organization organization) { - var orgUpdate = new SecretsManagerSubscriptionUpdate( - organization, - seatAdjustment: SeatAdjustment, maxAutoscaleSeats: MaxAutoscaleSeats, - serviceAccountAdjustment: ServiceAccountAdjustment, maxAutoscaleServiceAccounts: MaxAutoscaleServiceAccounts); - - return orgUpdate; + return new SecretsManagerSubscriptionUpdate(organization, false) + { + MaxAutoscaleSmSeats = MaxAutoscaleSeats, + MaxAutoscaleSmServiceAccounts = MaxAutoscaleServiceAccounts + } + .AdjustSeats(SeatAdjustment) + .AdjustServiceAccounts(ServiceAccountAdjustment); } } diff --git a/src/Api/SecretsManager/Controllers/ServiceAccountsController.cs b/src/Api/SecretsManager/Controllers/ServiceAccountsController.cs index 0e2d59da2..bb28276b0 100644 --- a/src/Api/SecretsManager/Controllers/ServiceAccountsController.cs +++ b/src/Api/SecretsManager/Controllers/ServiceAccountsController.cs @@ -4,6 +4,7 @@ using Bit.Api.SecretsManager.Models.Response; using Bit.Core.Context; using Bit.Core.Enums; using Bit.Core.Exceptions; +using Bit.Core.Models.Business; using Bit.Core.OrganizationFeatures.OrganizationSubscriptions.Interface; using Bit.Core.Repositories; using Bit.Core.SecretsManager.AuthorizationRequirements; @@ -125,8 +126,9 @@ public class ServiceAccountsController : Controller if (newServiceAccountSlotsRequired > 0) { var org = await _organizationRepository.GetByIdAsync(organizationId); - await _updateSecretsManagerSubscriptionCommand.AdjustServiceAccountsAsync(org, - newServiceAccountSlotsRequired); + var update = new SecretsManagerSubscriptionUpdate(org, true) + .AdjustServiceAccounts(newServiceAccountSlotsRequired); + await _updateSecretsManagerSubscriptionCommand.UpdateSubscriptionAsync(update); } var userId = _userService.GetProperUserId(User).Value; diff --git a/src/Core/Models/Business/SecretsManagerSubscriptionUpdate.cs b/src/Core/Models/Business/SecretsManagerSubscriptionUpdate.cs index a2e687a92..1cb7a41df 100644 --- a/src/Core/Models/Business/SecretsManagerSubscriptionUpdate.cs +++ b/src/Core/Models/Business/SecretsManagerSubscriptionUpdate.cs @@ -6,7 +6,7 @@ namespace Bit.Core.Models.Business; public class SecretsManagerSubscriptionUpdate { - public Organization Organization { get; set; } + public Organization Organization { get; } /// /// The total seats the organization will have after the update, including any base seats included in the plan @@ -14,8 +14,7 @@ public class SecretsManagerSubscriptionUpdate public int? SmSeats { get; set; } /// - /// The new autoscale limit for seats, expressed as a total (not an adjustment). - /// This may or may not be the same as the current autoscale limit. + /// The new autoscale limit for seats after the update /// public int? MaxAutoscaleSmSeats { get; set; } @@ -26,8 +25,7 @@ public class SecretsManagerSubscriptionUpdate public int? SmServiceAccounts { get; set; } /// - /// The new autoscale limit for service accounts, expressed as a total (not an adjustment). - /// This may or may not be the same as the current autoscale limit. + /// The new autoscale limit for service accounts after the update /// public int? MaxAutoscaleSmServiceAccounts { get; set; } @@ -39,7 +37,7 @@ public class SecretsManagerSubscriptionUpdate /// /// Whether the subscription update is a result of autoscaling /// - public bool Autoscaling { get; init; } + public bool Autoscaling { get; } /// /// The seats the organization will have after the update, excluding the base seats included in the plan @@ -57,18 +55,11 @@ public class SecretsManagerSubscriptionUpdate public bool MaxAutoscaleSmServiceAccountsChanged => MaxAutoscaleSmServiceAccounts != Organization.MaxAutoscaleSmServiceAccounts; public Plan Plan => Utilities.StaticStore.GetSecretsManagerPlan(Organization.PlanType); + public bool SmSeatAutoscaleLimitReached => SmSeats.HasValue && MaxAutoscaleSmSeats.HasValue && SmSeats == MaxAutoscaleSmSeats; - public SecretsManagerSubscriptionUpdate( - Organization organization, - int seatAdjustment, int? maxAutoscaleSeats, - int serviceAccountAdjustment, int? maxAutoscaleServiceAccounts) : this(organization, false) - { - AdjustSeats(seatAdjustment); - AdjustServiceAccounts(serviceAccountAdjustment); - - MaxAutoscaleSmSeats = maxAutoscaleSeats; - MaxAutoscaleSmServiceAccounts = maxAutoscaleServiceAccounts; - } + public bool SmServiceAccountAutoscaleLimitReached => SmServiceAccounts.HasValue && + MaxAutoscaleSmServiceAccounts.HasValue && + SmServiceAccounts == MaxAutoscaleSmServiceAccounts; public SecretsManagerSubscriptionUpdate(Organization organization, bool autoscaling) { @@ -91,13 +82,15 @@ public class SecretsManagerSubscriptionUpdate Autoscaling = autoscaling; } - public void AdjustSeats(int adjustment) + public SecretsManagerSubscriptionUpdate AdjustSeats(int adjustment) { SmSeats = SmSeats.GetValueOrDefault() + adjustment; + return this; } - public void AdjustServiceAccounts(int adjustment) + public SecretsManagerSubscriptionUpdate AdjustServiceAccounts(int adjustment) { SmServiceAccounts = SmServiceAccounts.GetValueOrDefault() + adjustment; + return this; } } diff --git a/src/Core/OrganizationFeatures/OrganizationSubscriptions/AddSecretsManagerSubscriptionCommand.cs b/src/Core/OrganizationFeatures/OrganizationSubscriptions/AddSecretsManagerSubscriptionCommand.cs index 6b514089e..3741148af 100644 --- a/src/Core/OrganizationFeatures/OrganizationSubscriptions/AddSecretsManagerSubscriptionCommand.cs +++ b/src/Core/OrganizationFeatures/OrganizationSubscriptions/AddSecretsManagerSubscriptionCommand.cs @@ -62,6 +62,17 @@ public class AddSecretsManagerSubscriptionCommand : IAddSecretsManagerSubscripti throw new NotFoundException(); } + if (organization.SecretsManagerBeta) + { + throw new BadRequestException("Organization is enrolled in Secrets Manager Beta. " + + "Please contact Customer Success to add Secrets Manager to your subscription."); + } + + if (organization.UseSecretsManager) + { + throw new BadRequestException("Organization already uses Secrets Manager."); + } + var plan = StaticStore.GetSecretsManagerPlan(organization.PlanType); if (string.IsNullOrWhiteSpace(organization.GatewayCustomerId) && plan.Product != ProductType.Free) { diff --git a/src/Core/OrganizationFeatures/OrganizationSubscriptions/Interface/IUpdateSecretsManagerSubscriptionCommand.cs b/src/Core/OrganizationFeatures/OrganizationSubscriptions/Interface/IUpdateSecretsManagerSubscriptionCommand.cs index 38126d17c..036e4a179 100644 --- a/src/Core/OrganizationFeatures/OrganizationSubscriptions/Interface/IUpdateSecretsManagerSubscriptionCommand.cs +++ b/src/Core/OrganizationFeatures/OrganizationSubscriptions/Interface/IUpdateSecretsManagerSubscriptionCommand.cs @@ -1,11 +1,9 @@ -using Bit.Core.Entities; -using Bit.Core.Models.Business; +using Bit.Core.Models.Business; namespace Bit.Core.OrganizationFeatures.OrganizationSubscriptions.Interface; public interface IUpdateSecretsManagerSubscriptionCommand { Task UpdateSubscriptionAsync(SecretsManagerSubscriptionUpdate update); - Task AdjustServiceAccountsAsync(Organization organization, int smServiceAccountsAdjustment); Task ValidateUpdate(SecretsManagerSubscriptionUpdate update); } diff --git a/src/Core/OrganizationFeatures/OrganizationSubscriptions/UpdateSecretsManagerSubscriptionCommand.cs b/src/Core/OrganizationFeatures/OrganizationSubscriptions/UpdateSecretsManagerSubscriptionCommand.cs index ec4b482e4..ab96e55f7 100644 --- a/src/Core/OrganizationFeatures/OrganizationSubscriptions/UpdateSecretsManagerSubscriptionCommand.cs +++ b/src/Core/OrganizationFeatures/OrganizationSubscriptions/UpdateSecretsManagerSubscriptionCommand.cs @@ -2,13 +2,11 @@ using Bit.Core.Enums; using Bit.Core.Exceptions; using Bit.Core.Models.Business; -using Bit.Core.Models.StaticStore; using Bit.Core.OrganizationFeatures.OrganizationSubscriptions.Interface; using Bit.Core.Repositories; using Bit.Core.SecretsManager.Repositories; using Bit.Core.Services; using Bit.Core.Settings; -using Bit.Core.Utilities; using Microsoft.Extensions.Logging; namespace Bit.Core.OrganizationFeatures.OrganizationSubscriptions; @@ -51,82 +49,46 @@ public class UpdateSecretsManagerSubscriptionCommand : IUpdateSecretsManagerSubs { await ValidateUpdate(update); - await FinalizeSubscriptionAdjustmentAsync(update.Organization, update.Plan, update); + await FinalizeSubscriptionAdjustmentAsync(update); - await SendEmailIfAutoscaleLimitReached(update.Organization); - } - - public async Task AdjustServiceAccountsAsync(Organization organization, int smServiceAccountsAdjustment) - { - var update = new SecretsManagerSubscriptionUpdate( - organization, seatAdjustment: 0, maxAutoscaleSeats: organization?.MaxAutoscaleSmSeats, - serviceAccountAdjustment: smServiceAccountsAdjustment, maxAutoscaleServiceAccounts: organization?.MaxAutoscaleSmServiceAccounts) + if (update.SmSeatAutoscaleLimitReached) { - Autoscaling = true - }; + await SendSeatLimitEmailAsync(update.Organization); + } - await UpdateSubscriptionAsync(update); + if (update.SmServiceAccountAutoscaleLimitReached) + { + await SendServiceAccountLimitEmailAsync(update.Organization); + } } - private async Task FinalizeSubscriptionAdjustmentAsync(Organization organization, - Plan plan, SecretsManagerSubscriptionUpdate update) + private async Task FinalizeSubscriptionAdjustmentAsync(SecretsManagerSubscriptionUpdate update) { if (update.SmSeatsChanged) { - await ProcessChargesAndRaiseEventsForAdjustSeatsAsync(organization, plan, update); - organization.SmSeats = update.SmSeats; + await _paymentService.AdjustSeatsAsync(update.Organization, update.Plan, update.SmSeatsExcludingBase, update.ProrationDate); + + // TODO: call ReferenceEventService - see AC-1481 } if (update.SmServiceAccountsChanged) { - await ProcessChargesAndRaiseEventsForAdjustServiceAccountsAsync(organization, plan, update); - organization.SmServiceAccounts = update.SmServiceAccounts; + await _paymentService.AdjustServiceAccountsAsync(update.Organization, update.Plan, + update.SmServiceAccountsExcludingBase, update.ProrationDate); + + // TODO: call ReferenceEventService - see AC-1481 } - if (update.MaxAutoscaleSmSeatsChanged) - { - organization.MaxAutoscaleSmSeats = update.MaxAutoscaleSmSeats; - } - - if (update.MaxAutoscaleSmServiceAccountsChanged) - { - organization.MaxAutoscaleSmServiceAccounts = update.MaxAutoscaleSmServiceAccounts; - } + var organization = update.Organization; + organization.SmSeats = update.SmSeats; + organization.SmServiceAccounts = update.SmServiceAccounts; + organization.MaxAutoscaleSmSeats = update.MaxAutoscaleSmSeats; + organization.MaxAutoscaleSmServiceAccounts = update.MaxAutoscaleSmServiceAccounts; await ReplaceAndUpdateCacheAsync(organization); } - private async Task ProcessChargesAndRaiseEventsForAdjustSeatsAsync(Organization organization, Plan plan, - SecretsManagerSubscriptionUpdate update) - { - await _paymentService.AdjustSeatsAsync(organization, plan, update.SmSeatsExcludingBase, update.ProrationDate); - - // TODO: call ReferenceEventService - see AC-1481 - } - - private async Task ProcessChargesAndRaiseEventsForAdjustServiceAccountsAsync(Organization organization, Plan plan, - SecretsManagerSubscriptionUpdate update) - { - await _paymentService.AdjustServiceAccountsAsync(organization, plan, - update.SmServiceAccountsExcludingBase, update.ProrationDate); - - // TODO: call ReferenceEventService - see AC-1481 - } - - private async Task SendEmailIfAutoscaleLimitReached(Organization organization) - { - if (organization.SmSeats.HasValue && organization.MaxAutoscaleSmSeats.HasValue && organization.SmSeats == organization.MaxAutoscaleSmSeats) - { - await SendSeatLimitEmailAsync(organization, organization.MaxAutoscaleSmSeats.Value); - } - - if (organization.SmServiceAccounts.HasValue && organization.MaxAutoscaleSmServiceAccounts.HasValue && organization.SmServiceAccounts == organization.MaxAutoscaleSmServiceAccounts) - { - await SendServiceAccountLimitEmailAsync(organization, organization.MaxAutoscaleSmServiceAccounts.Value); - } - } - - private async Task SendSeatLimitEmailAsync(Organization organization, int MaxAutoscaleValue) + private async Task SendSeatLimitEmailAsync(Organization organization) { try { @@ -134,16 +96,16 @@ public class UpdateSecretsManagerSubscriptionCommand : IUpdateSecretsManagerSubs OrganizationUserType.Owner)) .Select(u => u.Email).Distinct(); - await _mailService.SendSecretsManagerMaxSeatLimitReachedEmailAsync(organization, MaxAutoscaleValue, ownerEmails); + await _mailService.SendSecretsManagerMaxSeatLimitReachedEmailAsync(organization, organization.MaxAutoscaleSmSeats.Value, ownerEmails); } catch (Exception e) { - _logger.LogError(e, $"Error encountered notifying organization owners of Seats limit reached."); + _logger.LogError(e, $"Error encountered notifying organization owners of seats limit reached."); } } - private async Task SendServiceAccountLimitEmailAsync(Organization organization, int MaxAutoscaleValue) + private async Task SendServiceAccountLimitEmailAsync(Organization organization) { try { @@ -151,12 +113,12 @@ public class UpdateSecretsManagerSubscriptionCommand : IUpdateSecretsManagerSubs OrganizationUserType.Owner)) .Select(u => u.Email).Distinct(); - await _mailService.SendSecretsManagerMaxServiceAccountLimitReachedEmailAsync(organization, MaxAutoscaleValue, ownerEmails); + await _mailService.SendSecretsManagerMaxServiceAccountLimitReachedEmailAsync(organization, organization.MaxAutoscaleSmServiceAccounts.Value, ownerEmails); } catch (Exception e) { - _logger.LogError(e, $"Error encountered notifying organization owners of Service Accounts limit reached."); + _logger.LogError(e, $"Error encountered notifying organization owners of service accounts limit reached."); } } @@ -171,46 +133,45 @@ public class UpdateSecretsManagerSubscriptionCommand : IUpdateSecretsManagerSubs throw new BadRequestException(message); } - var organization = update.Organization; - ValidateOrganization(organization); - - var plan = GetPlanForOrganization(organization); + ValidateOrganization(update); if (update.SmSeatsChanged) { - await ValidateSmSeatsUpdateAsync(organization, update, plan); + await ValidateSmSeatsUpdateAsync(update); } if (update.SmServiceAccountsChanged) { - await ValidateSmServiceAccountsUpdateAsync(organization, update, plan); + await ValidateSmServiceAccountsUpdateAsync(update); } if (update.MaxAutoscaleSmSeatsChanged) { - ValidateMaxAutoscaleSmSeatsUpdateAsync(organization, update.MaxAutoscaleSmSeats, plan); + ValidateMaxAutoscaleSmSeatsUpdateAsync(update); } if (update.MaxAutoscaleSmServiceAccountsChanged) { - ValidateMaxAutoscaleSmServiceAccountUpdate(organization, update.MaxAutoscaleSmServiceAccounts, plan); + ValidateMaxAutoscaleSmServiceAccountUpdate(update); } } - private void ValidateOrganization(Organization organization) + private void ValidateOrganization(SecretsManagerSubscriptionUpdate update) { - if (organization == null) - { - throw new NotFoundException("Organization is not found."); - } + var organization = update.Organization; if (!organization.UseSecretsManager) { throw new BadRequestException("Organization has no access to Secrets Manager."); } - var plan = GetPlanForOrganization(organization); - if (plan.Product == ProductType.Free) + if (organization.SecretsManagerBeta) + { + throw new BadRequestException("Organization is enrolled in Secrets Manager Beta. " + + "Please contact Customer Success to add Secrets Manager to your subscription."); + } + + if (update.Plan.Product == ProductType.Free) { // No need to check the organization is set up with Stripe return; @@ -227,18 +188,11 @@ public class UpdateSecretsManagerSubscriptionCommand : IUpdateSecretsManagerSubs } } - private Plan GetPlanForOrganization(Organization organization) + private async Task ValidateSmSeatsUpdateAsync(SecretsManagerSubscriptionUpdate update) { - var plan = StaticStore.SecretManagerPlans.FirstOrDefault(p => p.Type == organization.PlanType); - if (plan == null) - { - throw new BadRequestException("Existing plan not found."); - } - return plan; - } + var organization = update.Organization; + var plan = update.Plan; - private async Task ValidateSmSeatsUpdateAsync(Organization organization, SecretsManagerSubscriptionUpdate update, Plan plan) - { // Check if the organization has unlimited seats if (organization.SmSeats == null) { @@ -282,21 +236,24 @@ public class UpdateSecretsManagerSubscriptionCommand : IUpdateSecretsManagerSubs // Check minimum seats currently in use by the organization if (organization.SmSeats.Value > update.SmSeats.Value) { - var currentSeats = await _organizationUserRepository.GetOccupiedSmSeatCountByOrganizationIdAsync(organization.Id); - if (currentSeats > update.SmSeats.Value) + var occupiedSeats = await _organizationUserRepository.GetOccupiedSmSeatCountByOrganizationIdAsync(organization.Id); + if (occupiedSeats > update.SmSeats.Value) { - throw new BadRequestException($"Your organization currently has {currentSeats} Secrets Manager seats. " + - $"Your plan only allows {update.SmSeats} Secrets Manager seats. Remove some Secrets Manager users."); + throw new BadRequestException($"{occupiedSeats} users are currently occupying Secrets Manager seats. " + + "You cannot decrease your subscription below your current occupied seat count."); } } } - private async Task ValidateSmServiceAccountsUpdateAsync(Organization organization, SecretsManagerSubscriptionUpdate update, Plan plan) + private async Task ValidateSmServiceAccountsUpdateAsync(SecretsManagerSubscriptionUpdate update) { + var organization = update.Organization; + var plan = update.Plan; + // Check if the organization has unlimited service accounts if (organization.SmServiceAccounts == null) { - throw new BadRequestException("Organization has no Service Accounts limit, no need to adjust Service Accounts"); + throw new BadRequestException("Organization has no service accounts limit, no need to adjust service accounts"); } if (update.Autoscaling && update.SmServiceAccounts.Value < organization.SmServiceAccounts.Value) @@ -326,13 +283,13 @@ public class UpdateSecretsManagerSubscriptionCommand : IUpdateSecretsManagerSubs // Check minimum service accounts included with plan if (plan.BaseServiceAccount.HasValue && plan.BaseServiceAccount.Value > update.SmServiceAccounts.Value) { - throw new BadRequestException($"Plan has a minimum of {plan.BaseServiceAccount} Service Accounts."); + throw new BadRequestException($"Plan has a minimum of {plan.BaseServiceAccount} service accounts."); } // Check minimum service accounts required by business logic if (update.SmServiceAccounts.Value <= 0) { - throw new BadRequestException("You must have at least 1 Service Account."); + throw new BadRequestException("You must have at least 1 service account."); } // Check minimum service accounts currently in use by the organization @@ -341,30 +298,32 @@ public class UpdateSecretsManagerSubscriptionCommand : IUpdateSecretsManagerSubs var currentServiceAccounts = await _serviceAccountRepository.GetServiceAccountCountByOrganizationIdAsync(organization.Id); if (currentServiceAccounts > update.SmServiceAccounts) { - throw new BadRequestException($"Your organization currently has {currentServiceAccounts} Service Accounts. " + - $"Your plan only allows {update.SmServiceAccounts} Service Accounts. Remove some Service Accounts."); + throw new BadRequestException($"Your organization currently has {currentServiceAccounts} service accounts. " + + $"You cannot decrease your subscription below your current service account usage."); } } } - private void ValidateMaxAutoscaleSmSeatsUpdateAsync(Organization organization, int? maxAutoscaleSeats, Plan plan) + private void ValidateMaxAutoscaleSmSeatsUpdateAsync(SecretsManagerSubscriptionUpdate update) { - if (!maxAutoscaleSeats.HasValue) + var plan = update.Plan; + + if (!update.MaxAutoscaleSmSeats.HasValue) { // autoscale limit has been turned off, no validation required return; } - if (organization.SmSeats.HasValue && maxAutoscaleSeats.Value < organization.SmSeats.Value) + if (update.SmSeats.HasValue && update.MaxAutoscaleSmSeats.Value < update.SmSeats.Value) { throw new BadRequestException($"Cannot set max Secrets Manager seat autoscaling below current Secrets Manager seat count."); } - if (plan.MaxUsers.HasValue && maxAutoscaleSeats.Value > plan.MaxUsers) + if (plan.MaxUsers.HasValue && update.MaxAutoscaleSmSeats.Value > plan.MaxUsers) { throw new BadRequestException(string.Concat( $"Your plan has a Secrets Manager seat limit of {plan.MaxUsers}, ", - $"but you have specified a max autoscale count of {maxAutoscaleSeats}.", + $"but you have specified a max autoscale count of {update.MaxAutoscaleSmSeats}.", "Reduce your max autoscale count.")); } @@ -374,30 +333,32 @@ public class UpdateSecretsManagerSubscriptionCommand : IUpdateSecretsManagerSubs } } - private void ValidateMaxAutoscaleSmServiceAccountUpdate(Organization organization, int? maxAutoscaleServiceAccounts, Plan plan) + private void ValidateMaxAutoscaleSmServiceAccountUpdate(SecretsManagerSubscriptionUpdate update) { - if (!maxAutoscaleServiceAccounts.HasValue) + var plan = update.Plan; + + if (!update.MaxAutoscaleSmServiceAccounts.HasValue) { // autoscale limit has been turned off, no validation required return; } - if (organization.SmServiceAccounts.HasValue && maxAutoscaleServiceAccounts.Value < organization.SmServiceAccounts.Value) + if (update.SmServiceAccounts.HasValue && update.MaxAutoscaleSmServiceAccounts.Value < update.SmServiceAccounts.Value) { throw new BadRequestException( - $"Cannot set max Service Accounts autoscaling below current Service Accounts count."); + $"Cannot set max service accounts autoscaling below current service accounts count."); } if (!plan.AllowServiceAccountsAutoscale) { - throw new BadRequestException("Your plan does not allow Service Accounts autoscaling."); + throw new BadRequestException("Your plan does not allow service accounts autoscaling."); } - if (plan.MaxServiceAccounts.HasValue && maxAutoscaleServiceAccounts.Value > plan.MaxServiceAccounts) + if (plan.MaxServiceAccounts.HasValue && update.MaxAutoscaleSmServiceAccounts.Value > plan.MaxServiceAccounts) { throw new BadRequestException(string.Concat( - $"Your plan has a Service Accounts limit of {plan.MaxServiceAccounts}, ", - $"but you have specified a max autoscale count of {maxAutoscaleServiceAccounts}.", + $"Your plan has a service account limit of {plan.MaxServiceAccounts}, ", + $"but you have specified a max autoscale count of {update.MaxAutoscaleSmServiceAccounts}.", "Reduce your max autoscale count.")); } } diff --git a/src/Core/Services/Implementations/OrganizationService.cs b/src/Core/Services/Implementations/OrganizationService.cs index b27e395b0..b7424a53e 100644 --- a/src/Core/Services/Implementations/OrganizationService.cs +++ b/src/Core/Services/Implementations/OrganizationService.cs @@ -861,8 +861,8 @@ public class OrganizationService : IOrganizationService var additionalSmSeatsRequired = await _countNewSmSeatsRequiredQuery.CountNewSmSeatsRequiredAsync(organization.Id, inviteWithSmAccessCount); if (additionalSmSeatsRequired > 0) { - smSubscriptionUpdate = new SecretsManagerSubscriptionUpdate(organization, true); - smSubscriptionUpdate.AdjustSeats(additionalSmSeatsRequired); + smSubscriptionUpdate = new SecretsManagerSubscriptionUpdate(organization, true) + .AdjustSeats(additionalSmSeatsRequired); await _updateSecretsManagerSubscriptionCommand.ValidateUpdate(smSubscriptionUpdate); } @@ -1418,8 +1418,8 @@ public class OrganizationService : IOrganizationService if (additionalSmSeatsRequired > 0) { var organization = await _organizationRepository.GetByIdAsync(user.OrganizationId); - var update = new SecretsManagerSubscriptionUpdate(organization, true); - update.AdjustSeats(additionalSmSeatsRequired); + var update = new SecretsManagerSubscriptionUpdate(organization, true) + .AdjustSeats(additionalSmSeatsRequired); await _updateSecretsManagerSubscriptionCommand.UpdateSubscriptionAsync(update); } } diff --git a/test/Api.Test/SecretsManager/Controllers/ServiceAccountsControllerTests.cs b/test/Api.Test/SecretsManager/Controllers/ServiceAccountsControllerTests.cs index 6a311a482..29dcd117c 100644 --- a/test/Api.Test/SecretsManager/Controllers/ServiceAccountsControllerTests.cs +++ b/test/Api.Test/SecretsManager/Controllers/ServiceAccountsControllerTests.cs @@ -5,6 +5,7 @@ using Bit.Core.Context; using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Exceptions; +using Bit.Core.Models.Business; using Bit.Core.OrganizationFeatures.OrganizationSubscriptions.Interface; using Bit.Core.Repositories; using Bit.Core.SecretsManager.Commands.AccessTokens.Interfaces; @@ -106,7 +107,7 @@ public class ServiceAccountsControllerTests .CreateAsync(Arg.Is(sa => sa.Name == data.Name), Arg.Any()); await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() - .AdjustServiceAccountsAsync(Arg.Any(), Arg.Any()); + .UpdateSubscriptionAsync(Arg.Any()); } [Theory] @@ -124,7 +125,12 @@ public class ServiceAccountsControllerTests .CreateAsync(Arg.Is(sa => sa.Name == data.Name), Arg.Any()); await sutProvider.GetDependency().Received(1) - .AdjustServiceAccountsAsync(Arg.Is(organization), Arg.Is(newSlotsRequired)); + .UpdateSubscriptionAsync(Arg.Is(update => + update.Autoscaling == true && + update.SmServiceAccounts == organization.SmServiceAccounts + newSlotsRequired && + !update.SmSeatsChanged && + !update.MaxAutoscaleSmSeatsChanged && + !update.MaxAutoscaleSmServiceAccountsChanged)); } [Theory] diff --git a/test/Core.Test/AutoFixture/OrganizationFixtures.cs b/test/Core.Test/AutoFixture/OrganizationFixtures.cs index 8e6cfd060..0116296d3 100644 --- a/test/Core.Test/AutoFixture/OrganizationFixtures.cs +++ b/test/Core.Test/AutoFixture/OrganizationFixtures.cs @@ -137,6 +137,7 @@ public class SecretsManagerOrganizationCustomization : ICustomization fixture.Customize(composer => composer .With(o => o.Id, organizationId) .With(o => o.UseSecretsManager, true) + .With(o => o.SecretsManagerBeta, false) .With(o => o.PlanType, planType) .With(o => o.Plan, StaticStore.GetPasswordManagerPlan(planType).Name) .With(o => o.MaxAutoscaleSmSeats, (int?)null) diff --git a/test/Core.Test/OrganizationFeatures/OrganizationSubscriptionUpdate/AddSecretsManagerSubscriptionCommandTests.cs b/test/Core.Test/OrganizationFeatures/OrganizationSubscriptionUpdate/AddSecretsManagerSubscriptionCommandTests.cs index 9d635a5af..a09500cf6 100644 --- a/test/Core.Test/OrganizationFeatures/OrganizationSubscriptionUpdate/AddSecretsManagerSubscriptionCommandTests.cs +++ b/test/Core.Test/OrganizationFeatures/OrganizationSubscriptionUpdate/AddSecretsManagerSubscriptionCommandTests.cs @@ -95,6 +95,38 @@ public class AddSecretsManagerSubscriptionCommandTests await VerifyDependencyNotCalledAsync(sutProvider); } + [Theory] + [BitAutoData] + public async Task SignUpAsync_ThrowsException_WhenOrganizationEnrolledInSmBeta( + SutProvider sutProvider, + Organization organization) + { + organization.UseSecretsManager = true; + organization.SecretsManagerBeta = true; + + var exception = await Assert.ThrowsAsync( + () => sutProvider.Sut.SignUpAsync(organization, 10, 10)); + + Assert.Contains("Organization is enrolled in Secrets Manager Beta", exception.Message); + await VerifyDependencyNotCalledAsync(sutProvider); + } + + [Theory] + [BitAutoData] + public async Task SignUpAsync_ThrowsException_WhenOrganizationAlreadyHasSecretsManager( + SutProvider sutProvider, + Organization organization) + { + organization.UseSecretsManager = true; + organization.SecretsManagerBeta = false; + + var exception = await Assert.ThrowsAsync( + () => sutProvider.Sut.SignUpAsync(organization, 10, 10)); + + Assert.Contains("Organization already uses Secrets Manager", exception.Message); + await VerifyDependencyNotCalledAsync(sutProvider); + } + private static async Task VerifyDependencyNotCalledAsync(SutProvider sutProvider) { await sutProvider.GetDependency().DidNotReceive() diff --git a/test/Core.Test/OrganizationFeatures/OrganizationSubscriptionUpdate/UpdateSecretsManagerSubscriptionCommandTests.cs b/test/Core.Test/OrganizationFeatures/OrganizationSubscriptionUpdate/UpdateSecretsManagerSubscriptionCommandTests.cs index f40011561..2834db335 100644 --- a/test/Core.Test/OrganizationFeatures/OrganizationSubscriptionUpdate/UpdateSecretsManagerSubscriptionCommandTests.cs +++ b/test/Core.Test/OrganizationFeatures/OrganizationSubscriptionUpdate/UpdateSecretsManagerSubscriptionCommandTests.cs @@ -125,8 +125,7 @@ public class UpdateSecretsManagerSubscriptionCommandTests Organization organization, SutProvider sutProvider) { - var update = new SecretsManagerSubscriptionUpdate(organization, autoscaling); - update.AdjustSeats(2); + var update = new SecretsManagerSubscriptionUpdate(organization, autoscaling).AdjustSeats(2); sutProvider.GetDependency().SelfHosted.Returns(true); @@ -151,6 +150,23 @@ public class UpdateSecretsManagerSubscriptionCommandTests await VerifyDependencyNotCalledAsync(sutProvider); } + [Theory] + [BitAutoData] + public async Task UpdateSubscriptionAsync_OrganizationEnrolledInSmBeta_ThrowsException( + SutProvider sutProvider, + Organization organization) + { + organization.UseSecretsManager = true; + organization.SecretsManagerBeta = true; + var update = new SecretsManagerSubscriptionUpdate(organization, false); + + var exception = await Assert.ThrowsAsync( + () => sutProvider.Sut.UpdateSubscriptionAsync(update)); + + Assert.Contains("Organization is enrolled in Secrets Manager Beta", exception.Message); + await VerifyDependencyNotCalledAsync(sutProvider); + } + [Theory] [BitAutoData(PlanType.EnterpriseAnnually)] [BitAutoData(PlanType.EnterpriseMonthly)] @@ -163,8 +179,7 @@ public class UpdateSecretsManagerSubscriptionCommandTests { organization.PlanType = planType; organization.GatewayCustomerId = null; - var update = new SecretsManagerSubscriptionUpdate(organization, false); - update.AdjustSeats(1); + var update = new SecretsManagerSubscriptionUpdate(organization, false).AdjustSeats(1); var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.UpdateSubscriptionAsync(update)); Assert.Contains("No payment method found.", exception.Message); @@ -183,8 +198,7 @@ public class UpdateSecretsManagerSubscriptionCommandTests { organization.PlanType = planType; organization.GatewaySubscriptionId = null; - var update = new SecretsManagerSubscriptionUpdate(organization, false); - update.AdjustSeats(1); + var update = new SecretsManagerSubscriptionUpdate(organization, false).AdjustSeats(1); var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.UpdateSubscriptionAsync(update)); Assert.Contains("No subscription found.", exception.Message); @@ -223,8 +237,7 @@ public class UpdateSecretsManagerSubscriptionCommandTests var expectedSmServiceAccounts = organizationServiceAccounts + smServiceAccountsAdjustment; var expectedSmServiceAccountsExcludingBase = expectedSmServiceAccounts - plan.BaseServiceAccount.GetValueOrDefault(); - var update = new SecretsManagerSubscriptionUpdate(organization, false); - update.AdjustServiceAccounts(10); + var update = new SecretsManagerSubscriptionUpdate(organization, false).AdjustServiceAccounts(10); await sutProvider.Sut.UpdateSubscriptionAsync(update); @@ -247,7 +260,6 @@ public class UpdateSecretsManagerSubscriptionCommandTests Organization organization, SutProvider sutProvider) { - organization.SmSeats = 9; var update = new SecretsManagerSubscriptionUpdate(organization, false) { SmSeats = 10, @@ -267,8 +279,7 @@ public class UpdateSecretsManagerSubscriptionCommandTests SutProvider sutProvider) { organization.SmSeats = null; - var update = new SecretsManagerSubscriptionUpdate(organization, false); - update.AdjustSeats(1); + var update = new SecretsManagerSubscriptionUpdate(organization, false).AdjustSeats(1); var exception = await Assert.ThrowsAsync( () => sutProvider.Sut.UpdateSubscriptionAsync(update)); @@ -283,8 +294,7 @@ public class UpdateSecretsManagerSubscriptionCommandTests Organization organization, SutProvider sutProvider) { - var update = new SecretsManagerSubscriptionUpdate(organization, true); - update.AdjustSeats(-2); + var update = new SecretsManagerSubscriptionUpdate(organization, true).AdjustSeats(-2); var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.UpdateSubscriptionAsync(update)); Assert.Contains("Cannot use autoscaling to subtract seats.", exception.Message); @@ -299,8 +309,7 @@ public class UpdateSecretsManagerSubscriptionCommandTests SutProvider sutProvider) { organization.PlanType = planType; - var update = new SecretsManagerSubscriptionUpdate(organization, false); - update.AdjustSeats(1); + var update = new SecretsManagerSubscriptionUpdate(organization, false).AdjustSeats(1); var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.UpdateSubscriptionAsync(update)); Assert.Contains("You have reached the maximum number of Secrets Manager seats (2) for this plan", @@ -317,8 +326,7 @@ public class UpdateSecretsManagerSubscriptionCommandTests organization.SmSeats = 9; organization.MaxAutoscaleSmSeats = 10; - var update = new SecretsManagerSubscriptionUpdate(organization, true); - update.AdjustSeats(2); + var update = new SecretsManagerSubscriptionUpdate(organization, true).AdjustSeats(2); var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.UpdateSubscriptionAsync(update)); Assert.Contains("Secrets Manager seat limit has been reached.", exception.Message); @@ -375,7 +383,7 @@ public class UpdateSecretsManagerSubscriptionCommandTests sutProvider.GetDependency().GetOccupiedSmSeatCountByOrganizationIdAsync(organization.Id).Returns(8); var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.UpdateSubscriptionAsync(update)); - Assert.Contains("Your organization currently has 8 Secrets Manager seats. Your plan only allows 7 Secrets Manager seats. Remove some Secrets Manager users", exception.Message); + Assert.Contains("8 users are currently occupying Secrets Manager seats. You cannot decrease your subscription below your current occupied seat count", exception.Message); await VerifyDependencyNotCalledAsync(sutProvider); } @@ -385,7 +393,6 @@ public class UpdateSecretsManagerSubscriptionCommandTests Organization organization, SutProvider sutProvider) { - organization.SmServiceAccounts = 250; var update = new SecretsManagerSubscriptionUpdate(organization, false) { SmServiceAccounts = 300, @@ -405,11 +412,10 @@ public class UpdateSecretsManagerSubscriptionCommandTests SutProvider sutProvider) { organization.SmServiceAccounts = null; - var update = new SecretsManagerSubscriptionUpdate(organization, false); - update.AdjustServiceAccounts(1); + var update = new SecretsManagerSubscriptionUpdate(organization, false).AdjustServiceAccounts(1); var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.UpdateSubscriptionAsync(update)); - Assert.Contains("Organization has no Service Accounts limit, no need to adjust Service Accounts", exception.Message); + Assert.Contains("Organization has no service accounts limit, no need to adjust service accounts", exception.Message); await VerifyDependencyNotCalledAsync(sutProvider); } @@ -419,8 +425,7 @@ public class UpdateSecretsManagerSubscriptionCommandTests Organization organization, SutProvider sutProvider) { - var update = new SecretsManagerSubscriptionUpdate(organization, true); - update.AdjustServiceAccounts(-2); + var update = new SecretsManagerSubscriptionUpdate(organization, true).AdjustServiceAccounts(-2); var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.UpdateSubscriptionAsync(update)); Assert.Contains("Cannot use autoscaling to subtract service accounts.", exception.Message); @@ -435,8 +440,7 @@ public class UpdateSecretsManagerSubscriptionCommandTests SutProvider sutProvider) { organization.PlanType = planType; - var update = new SecretsManagerSubscriptionUpdate(organization, false); - update.AdjustServiceAccounts(1); + var update = new SecretsManagerSubscriptionUpdate(organization, false).AdjustServiceAccounts(1); var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.UpdateSubscriptionAsync(update)); Assert.Contains("You have reached the maximum number of service accounts (3) for this plan", @@ -453,8 +457,7 @@ public class UpdateSecretsManagerSubscriptionCommandTests organization.SmServiceAccounts = 9; organization.MaxAutoscaleSmServiceAccounts = 10; - var update = new SecretsManagerSubscriptionUpdate(organization, true); - update.AdjustServiceAccounts(2); + var update = new SecretsManagerSubscriptionUpdate(organization, true).AdjustServiceAccounts(2); var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.UpdateSubscriptionAsync(update)); Assert.Contains("Secrets Manager service account limit has been reached.", exception.Message); @@ -492,7 +495,7 @@ public class UpdateSecretsManagerSubscriptionCommandTests var exception = await Assert.ThrowsAsync( () => sutProvider.Sut.UpdateSubscriptionAsync(update)); - Assert.Contains("Plan has a minimum of 200 Service Accounts", exception.Message); + Assert.Contains("Plan has a minimum of 200 service accounts", exception.Message); await VerifyDependencyNotCalledAsync(sutProvider); } @@ -516,7 +519,7 @@ public class UpdateSecretsManagerSubscriptionCommandTests .Returns(currentServiceAccounts); var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.UpdateSubscriptionAsync(update)); - Assert.Contains("Your organization currently has 301 Service Accounts. Your plan only allows 201 Service Accounts. Remove some Service Accounts", exception.Message); + Assert.Contains("Your organization currently has 301 service accounts. You cannot decrease your subscription below your current service account usage", exception.Message); await VerifyDependencyNotCalledAsync(sutProvider); } @@ -588,7 +591,7 @@ public class UpdateSecretsManagerSubscriptionCommandTests var update = new SecretsManagerSubscriptionUpdate(organization, false) { MaxAutoscaleSmServiceAccounts = 3 }; var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.UpdateSubscriptionAsync(update)); - Assert.Contains("Your plan does not allow Service Accounts autoscaling.", exception.Message); + Assert.Contains("Your plan does not allow service accounts autoscaling.", exception.Message); await VerifyDependencyNotCalledAsync(sutProvider); }