From f1a4829e5ecedbb4bbc8dfc31a741ce09ad31d5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rui=20Tom=C3=A9?= <108268980+r-tome@users.noreply.github.com> Date: Wed, 9 Apr 2025 15:23:29 +0100 Subject: [PATCH] [PM-12485] Create OrganizationUpdateKeys command (#5600) * Add OrganizationUpdateKeysCommand * Add unit tests for OrganizationUpdateKeysCommand to validate permission checks and key updates * Register OrganizationUpdateKeysCommand for dependency injection * Refactor OrganizationsController to use IOrganizationUpdateKeysCommand for updating organization keys * Remove outdated unit tests for UpdateOrganizationKeysAsync in OrganizationServiceTests * Remove UpdateOrganizationKeysAsync method from IOrganizationService and OrganizationService implementations * Add IOrganizationUpdateKeysCommand dependency mock to OrganizationsControllerTests --- .../Controllers/OrganizationsController.cs | 9 ++- .../IOrganizationUpdateKeysCommand.cs | 13 ++++ .../OrganizationUpdateKeysCommand.cs | 47 ++++++++++++ .../Services/IOrganizationService.cs | 1 - .../Implementations/OrganizationService.cs | 22 ------ ...OrganizationServiceCollectionExtensions.cs | 6 ++ .../OrganizationsControllerTests.cs | 5 +- .../OrganizationUpdateKeysCommandTests.cs | 75 +++++++++++++++++++ .../Services/OrganizationServiceTests.cs | 42 ----------- 9 files changed, 151 insertions(+), 69 deletions(-) create mode 100644 src/Core/AdminConsole/OrganizationFeatures/Organizations/Interfaces/IOrganizationUpdateKeysCommand.cs create mode 100644 src/Core/AdminConsole/OrganizationFeatures/Organizations/OrganizationUpdateKeysCommand.cs create mode 100644 test/Core.Test/AdminConsole/OrganizationFeatures/Organizations/OrganizationUpdateKeysCommandTests.cs diff --git a/src/Api/AdminConsole/Controllers/OrganizationsController.cs b/src/Api/AdminConsole/Controllers/OrganizationsController.cs index 6b7d031a00..c856c8ab91 100644 --- a/src/Api/AdminConsole/Controllers/OrganizationsController.cs +++ b/src/Api/AdminConsole/Controllers/OrganizationsController.cs @@ -65,6 +65,7 @@ public class OrganizationsController : Controller private readonly IOrganizationDeleteCommand _organizationDeleteCommand; private readonly IPolicyRequirementQuery _policyRequirementQuery; private readonly IPricingClient _pricingClient; + private readonly IOrganizationUpdateKeysCommand _organizationUpdateKeysCommand; public OrganizationsController( IOrganizationRepository organizationRepository, @@ -88,7 +89,8 @@ public class OrganizationsController : Controller ICloudOrganizationSignUpCommand cloudOrganizationSignUpCommand, IOrganizationDeleteCommand organizationDeleteCommand, IPolicyRequirementQuery policyRequirementQuery, - IPricingClient pricingClient) + IPricingClient pricingClient, + IOrganizationUpdateKeysCommand organizationUpdateKeysCommand) { _organizationRepository = organizationRepository; _organizationUserRepository = organizationUserRepository; @@ -112,6 +114,7 @@ public class OrganizationsController : Controller _organizationDeleteCommand = organizationDeleteCommand; _policyRequirementQuery = policyRequirementQuery; _pricingClient = pricingClient; + _organizationUpdateKeysCommand = organizationUpdateKeysCommand; } [HttpGet("{id}")] @@ -490,7 +493,7 @@ public class OrganizationsController : Controller } [HttpPost("{id}/keys")] - public async Task PostKeys(string id, [FromBody] OrganizationKeysRequestModel model) + public async Task PostKeys(Guid id, [FromBody] OrganizationKeysRequestModel model) { var user = await _userService.GetUserByPrincipalAsync(User); if (user == null) @@ -498,7 +501,7 @@ public class OrganizationsController : Controller throw new UnauthorizedAccessException(); } - var org = await _organizationService.UpdateOrganizationKeysAsync(new Guid(id), model.PublicKey, + var org = await _organizationUpdateKeysCommand.UpdateOrganizationKeysAsync(id, model.PublicKey, model.EncryptedPrivateKey); return new OrganizationKeysResponseModel(org); } diff --git a/src/Core/AdminConsole/OrganizationFeatures/Organizations/Interfaces/IOrganizationUpdateKeysCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/Organizations/Interfaces/IOrganizationUpdateKeysCommand.cs new file mode 100644 index 0000000000..2d01a5e4e3 --- /dev/null +++ b/src/Core/AdminConsole/OrganizationFeatures/Organizations/Interfaces/IOrganizationUpdateKeysCommand.cs @@ -0,0 +1,13 @@ +using Bit.Core.AdminConsole.Entities; + +public interface IOrganizationUpdateKeysCommand +{ + /// + /// Update the keys for an organization. + /// + /// The ID of the organization to update. + /// The public key for the organization. + /// The private key for the organization. + /// The updated organization. + Task UpdateOrganizationKeysAsync(Guid orgId, string publicKey, string privateKey); +} diff --git a/src/Core/AdminConsole/OrganizationFeatures/Organizations/OrganizationUpdateKeysCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/Organizations/OrganizationUpdateKeysCommand.cs new file mode 100644 index 0000000000..aa85c7e2a4 --- /dev/null +++ b/src/Core/AdminConsole/OrganizationFeatures/Organizations/OrganizationUpdateKeysCommand.cs @@ -0,0 +1,47 @@ +using Bit.Core.AdminConsole.Entities; +using Bit.Core.Context; +using Bit.Core.Exceptions; +using Bit.Core.Repositories; +using Bit.Core.Services; + +public class OrganizationUpdateKeysCommand : IOrganizationUpdateKeysCommand +{ + private readonly ICurrentContext _currentContext; + private readonly IOrganizationRepository _organizationRepository; + private readonly IOrganizationService _organizationService; + + public const string OrganizationKeysAlreadyExistErrorMessage = "Organization Keys already exist."; + + public OrganizationUpdateKeysCommand( + ICurrentContext currentContext, + IOrganizationRepository organizationRepository, + IOrganizationService organizationService) + { + _currentContext = currentContext; + _organizationRepository = organizationRepository; + _organizationService = organizationService; + } + + public async Task UpdateOrganizationKeysAsync(Guid organizationId, string publicKey, string privateKey) + { + if (!await _currentContext.ManageResetPassword(organizationId)) + { + throw new UnauthorizedAccessException(); + } + + // If the keys already exist, error out + var organization = await _organizationRepository.GetByIdAsync(organizationId); + if (organization.PublicKey != null && organization.PrivateKey != null) + { + throw new BadRequestException(OrganizationKeysAlreadyExistErrorMessage); + } + + // Update org with generated public/private key + organization.PublicKey = publicKey; + organization.PrivateKey = privateKey; + + await _organizationService.UpdateAsync(organization); + + return organization; + } +} diff --git a/src/Core/AdminConsole/Services/IOrganizationService.cs b/src/Core/AdminConsole/Services/IOrganizationService.cs index e0088f1f74..8d2997bbc6 100644 --- a/src/Core/AdminConsole/Services/IOrganizationService.cs +++ b/src/Core/AdminConsole/Services/IOrganizationService.cs @@ -43,7 +43,6 @@ public interface IOrganizationService IEnumerable newUsers, IEnumerable removeUserExternalIds, bool overwriteExisting, EventSystemUser eventSystemUser); Task DeleteSsoUserAsync(Guid userId, Guid? organizationId); - Task UpdateOrganizationKeysAsync(Guid orgId, string publicKey, string privateKey); Task RevokeUserAsync(OrganizationUser organizationUser, Guid? revokingUserId); Task RevokeUserAsync(OrganizationUser organizationUser, EventSystemUser systemUser); Task>> RevokeUsersAsync(Guid organizationId, diff --git a/src/Core/AdminConsole/Services/Implementations/OrganizationService.cs b/src/Core/AdminConsole/Services/Implementations/OrganizationService.cs index c9027b8030..c9b38b3e30 100644 --- a/src/Core/AdminConsole/Services/Implementations/OrganizationService.cs +++ b/src/Core/AdminConsole/Services/Implementations/OrganizationService.cs @@ -1418,28 +1418,6 @@ public class OrganizationService : IOrganizationService } } - public async Task UpdateOrganizationKeysAsync(Guid orgId, string publicKey, string privateKey) - { - if (!await _currentContext.ManageResetPassword(orgId)) - { - throw new UnauthorizedAccessException(); - } - - // If the keys already exist, error out - var org = await _organizationRepository.GetByIdAsync(orgId); - if (org.PublicKey != null && org.PrivateKey != null) - { - throw new BadRequestException("Organization Keys already exist"); - } - - // Update org with generated public/private key - org.PublicKey = publicKey; - org.PrivateKey = privateKey; - await UpdateAsync(org); - - return org; - } - private async Task UpdateUsersAsync(Group group, HashSet groupUsers, Dictionary existingUsersIdDict, HashSet existingUsers = null) { diff --git a/src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs b/src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs index 96d9095c1a..164710d522 100644 --- a/src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs +++ b/src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs @@ -60,6 +60,7 @@ public static class OrganizationServiceCollectionExtensions services.AddOrganizationDomainCommandsQueries(); services.AddOrganizationSignUpCommands(); services.AddOrganizationDeleteCommands(); + services.AddOrganizationUpdateCommands(); services.AddOrganizationEnableCommands(); services.AddOrganizationDisableCommands(); services.AddOrganizationAuthCommands(); @@ -77,6 +78,11 @@ public static class OrganizationServiceCollectionExtensions services.AddScoped(); } + private static void AddOrganizationUpdateCommands(this IServiceCollection services) + { + services.AddScoped(); + } + private static void AddOrganizationEnableCommands(this IServiceCollection services) => services.AddScoped(); diff --git a/test/Api.Test/AdminConsole/Controllers/OrganizationsControllerTests.cs b/test/Api.Test/AdminConsole/Controllers/OrganizationsControllerTests.cs index 3c06c78392..867f8f8ec6 100644 --- a/test/Api.Test/AdminConsole/Controllers/OrganizationsControllerTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/OrganizationsControllerTests.cs @@ -60,6 +60,7 @@ public class OrganizationsControllerTests : IDisposable private readonly IOrganizationDeleteCommand _organizationDeleteCommand; private readonly IPolicyRequirementQuery _policyRequirementQuery; private readonly IPricingClient _pricingClient; + private readonly IOrganizationUpdateKeysCommand _organizationUpdateKeysCommand; private readonly OrganizationsController _sut; public OrganizationsControllerTests() @@ -86,6 +87,7 @@ public class OrganizationsControllerTests : IDisposable _organizationDeleteCommand = Substitute.For(); _policyRequirementQuery = Substitute.For(); _pricingClient = Substitute.For(); + _organizationUpdateKeysCommand = Substitute.For(); _sut = new OrganizationsController( _organizationRepository, @@ -109,7 +111,8 @@ public class OrganizationsControllerTests : IDisposable _cloudOrganizationSignUpCommand, _organizationDeleteCommand, _policyRequirementQuery, - _pricingClient); + _pricingClient, + _organizationUpdateKeysCommand); } public void Dispose() diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Organizations/OrganizationUpdateKeysCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Organizations/OrganizationUpdateKeysCommandTests.cs new file mode 100644 index 0000000000..91ab9214e1 --- /dev/null +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Organizations/OrganizationUpdateKeysCommandTests.cs @@ -0,0 +1,75 @@ +using Bit.Core.AdminConsole.Entities; +using Bit.Core.Context; +using Bit.Core.Exceptions; +using Bit.Core.Repositories; +using Bit.Core.Services; +using Bit.Test.Common.AutoFixture; +using Bit.Test.Common.AutoFixture.Attributes; +using NSubstitute; +using Xunit; + +namespace Bit.Core.Test.AdminConsole.OrganizationFeatures.Organizations; + +[SutProviderCustomize] +public class OrganizationUpdateKeysCommandTests +{ + [Theory, BitAutoData] + public async Task UpdateOrganizationKeysAsync_WithoutManageResetPasswordPermission_ThrowsUnauthorizedException( + Guid orgId, string publicKey, string privateKey, SutProvider sutProvider) + { + sutProvider.GetDependency() + .ManageResetPassword(orgId) + .Returns(false); + + await Assert.ThrowsAsync( + () => sutProvider.Sut.UpdateOrganizationKeysAsync(orgId, publicKey, privateKey)); + } + + [Theory, BitAutoData] + public async Task UpdateOrganizationKeysAsync_WhenKeysAlreadyExist_ThrowsBadRequestException( + Organization organization, string publicKey, string privateKey, + SutProvider sutProvider) + { + organization.PublicKey = "existingPublicKey"; + organization.PrivateKey = "existingPrivateKey"; + + sutProvider.GetDependency() + .ManageResetPassword(organization.Id) + .Returns(true); + + sutProvider.GetDependency() + .GetByIdAsync(organization.Id) + .Returns(organization); + + var exception = await Assert.ThrowsAsync( + () => sutProvider.Sut.UpdateOrganizationKeysAsync(organization.Id, publicKey, privateKey)); + + Assert.Equal(OrganizationUpdateKeysCommand.OrganizationKeysAlreadyExistErrorMessage, exception.Message); + } + + [Theory, BitAutoData] + public async Task UpdateOrganizationKeysAsync_WhenKeysDoNotExist_UpdatesOrganization( + Organization organization, string publicKey, string privateKey, + SutProvider sutProvider) + { + organization.PublicKey = null; + organization.PrivateKey = null; + + sutProvider.GetDependency() + .ManageResetPassword(organization.Id) + .Returns(true); + + sutProvider.GetDependency() + .GetByIdAsync(organization.Id) + .Returns(organization); + + var result = await sutProvider.Sut.UpdateOrganizationKeysAsync(organization.Id, publicKey, privateKey); + + Assert.Equal(publicKey, result.PublicKey); + Assert.Equal(privateKey, result.PrivateKey); + + await sutProvider.GetDependency() + .Received(1) + .UpdateAsync(organization); + } +} diff --git a/test/Core.Test/AdminConsole/Services/OrganizationServiceTests.cs b/test/Core.Test/AdminConsole/Services/OrganizationServiceTests.cs index e45643435d..c138cfac2e 100644 --- a/test/Core.Test/AdminConsole/Services/OrganizationServiceTests.cs +++ b/test/Core.Test/AdminConsole/Services/OrganizationServiceTests.cs @@ -814,48 +814,6 @@ public class OrganizationServiceTests sutProvider.GetDependency().ManageUsers(organization.Id).Returns(true); } - [Theory, BitAutoData] - public async Task UpdateOrganizationKeysAsync_WithoutManageResetPassword_Throws(Guid orgId, string publicKey, - string privateKey, SutProvider sutProvider) - { - var currentContext = Substitute.For(); - currentContext.ManageResetPassword(orgId).Returns(false); - - await Assert.ThrowsAsync( - () => sutProvider.Sut.UpdateOrganizationKeysAsync(orgId, publicKey, privateKey)); - } - - [Theory, BitAutoData] - public async Task UpdateOrganizationKeysAsync_KeysAlreadySet_Throws(Organization org, string publicKey, - string privateKey, SutProvider sutProvider) - { - var currentContext = sutProvider.GetDependency(); - currentContext.ManageResetPassword(org.Id).Returns(true); - - var organizationRepository = sutProvider.GetDependency(); - organizationRepository.GetByIdAsync(org.Id).Returns(org); - - var exception = await Assert.ThrowsAsync( - () => sutProvider.Sut.UpdateOrganizationKeysAsync(org.Id, publicKey, privateKey)); - Assert.Contains("Organization Keys already exist", exception.Message); - } - - [Theory, BitAutoData] - public async Task UpdateOrganizationKeysAsync_KeysAlreadySet_Success(Organization org, string publicKey, - string privateKey, SutProvider sutProvider) - { - org.PublicKey = null; - org.PrivateKey = null; - - var currentContext = sutProvider.GetDependency(); - currentContext.ManageResetPassword(org.Id).Returns(true); - - var organizationRepository = sutProvider.GetDependency(); - organizationRepository.GetByIdAsync(org.Id).Returns(org); - - await sutProvider.Sut.UpdateOrganizationKeysAsync(org.Id, publicKey, privateKey); - } - [Theory] [PaidOrganizationCustomize(CheckedPlanType = PlanType.EnterpriseAnnually)] [BitAutoData("Cannot set max seat autoscaling below seat count", 1, 0, 2, 2)]