From 0ce95ec1473cf1047fdfef6720e4ef7fb4ef8c86 Mon Sep 17 00:00:00 2001 From: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com> Date: Thu, 2 Feb 2023 12:25:14 -0600 Subject: [PATCH] [SM-465] Add access policy on service account creation (#2649) * Add access policy on service account creation --- .../CreateServiceAccountCommand.cs | 28 ++++++++++++++++--- .../Repositories/AccessPolicyRepository.cs | 21 ++++++++++++-- .../CreateServiceAccountCommandTests.cs | 15 ++++++++-- .../Controllers/AccessPoliciesController.cs | 2 +- .../Controllers/ServiceAccountsController.cs | 4 +-- .../ProjectAccessPoliciesResponseModel.cs | 5 ---- .../ICreateServiceAccountCommand.cs | 2 +- .../Repositories/IAccessPolicyRepository.cs | 3 +- .../AccessPoliciesControllerTest.cs | 20 +++++++++++++ .../ServiceAccountsControllerTests.cs | 9 ++++++ .../AccessPoliciesControllerTests.cs | 6 ++-- .../ServiceAccountsControllerTests.cs | 11 +++++--- 12 files changed, 101 insertions(+), 25 deletions(-) diff --git a/bitwarden_license/src/Commercial.Core/SecretsManager/Commands/ServiceAccounts/CreateServiceAccountCommand.cs b/bitwarden_license/src/Commercial.Core/SecretsManager/Commands/ServiceAccounts/CreateServiceAccountCommand.cs index f3d3799472..687291d75a 100644 --- a/bitwarden_license/src/Commercial.Core/SecretsManager/Commands/ServiceAccounts/CreateServiceAccountCommand.cs +++ b/bitwarden_license/src/Commercial.Core/SecretsManager/Commands/ServiceAccounts/CreateServiceAccountCommand.cs @@ -1,4 +1,5 @@ -using Bit.Core.SecretsManager.Commands.ServiceAccounts.Interfaces; +using Bit.Core.Repositories; +using Bit.Core.SecretsManager.Commands.ServiceAccounts.Interfaces; using Bit.Core.SecretsManager.Entities; using Bit.Core.SecretsManager.Repositories; @@ -6,15 +7,34 @@ namespace Bit.Commercial.Core.SecretsManager.Commands.ServiceAccounts; public class CreateServiceAccountCommand : ICreateServiceAccountCommand { + private readonly IAccessPolicyRepository _accessPolicyRepository; + private readonly IOrganizationUserRepository _organizationUserRepository; private readonly IServiceAccountRepository _serviceAccountRepository; - public CreateServiceAccountCommand(IServiceAccountRepository serviceAccountRepository) + public CreateServiceAccountCommand( + IAccessPolicyRepository accessPolicyRepository, + IOrganizationUserRepository organizationUserRepository, + IServiceAccountRepository serviceAccountRepository) { + _accessPolicyRepository = accessPolicyRepository; + _organizationUserRepository = organizationUserRepository; _serviceAccountRepository = serviceAccountRepository; } - public async Task CreateAsync(ServiceAccount serviceAccount) + public async Task CreateAsync(ServiceAccount serviceAccount, Guid userId) { - return await _serviceAccountRepository.CreateAsync(serviceAccount); + var createdServiceAccount = await _serviceAccountRepository.CreateAsync(serviceAccount); + + var user = await _organizationUserRepository.GetByOrganizationAsync(createdServiceAccount.OrganizationId, + userId); + var accessPolicy = new UserServiceAccountAccessPolicy + { + OrganizationUserId = user.Id, + GrantedServiceAccountId = createdServiceAccount.Id, + Read = true, + Write = true, + }; + await _accessPolicyRepository.CreateManyAsync(new List { accessPolicy }); + return createdServiceAccount; } } diff --git a/bitwarden_license/src/Commercial.Infrastructure.EntityFramework/SecretsManager/Repositories/AccessPolicyRepository.cs b/bitwarden_license/src/Commercial.Infrastructure.EntityFramework/SecretsManager/Repositories/AccessPolicyRepository.cs index dd56e08fcd..1cf066190e 100644 --- a/bitwarden_license/src/Commercial.Infrastructure.EntityFramework/SecretsManager/Repositories/AccessPolicyRepository.cs +++ b/bitwarden_license/src/Commercial.Infrastructure.EntityFramework/SecretsManager/Repositories/AccessPolicyRepository.cs @@ -138,7 +138,7 @@ public class AccessPolicyRepository : BaseEntityFrameworkRepository, IAccessPoli } } - public async Task?> GetManyByProjectId(Guid id) + public async Task> GetManyByGrantedProjectIdAsync(Guid id) { using (var scope = ServiceScopeFactory.CreateScope()) { @@ -153,10 +153,25 @@ public class AccessPolicyRepository : BaseEntityFrameworkRepository, IAccessPoli .Include(ap => ((ServiceAccountProjectAccessPolicy)ap).ServiceAccount) .ToListAsync(); - return !entities.Any() ? null : entities.Select(MapToCore); + return entities.Select(MapToCore); } } + public async Task> GetManyByGrantedServiceAccountIdAsync(Guid id) + { + using var scope = ServiceScopeFactory.CreateScope(); + var dbContext = GetDatabaseContext(scope); + + var entities = await dbContext.AccessPolicies.Where(ap => + ((UserServiceAccountAccessPolicy)ap).GrantedServiceAccountId == id || + ((GroupServiceAccountAccessPolicy)ap).GrantedServiceAccountId == id) + .Include(ap => ((UserServiceAccountAccessPolicy)ap).OrganizationUser.User) + .Include(ap => ((GroupServiceAccountAccessPolicy)ap).Group) + .ToListAsync(); + + return entities.Select(MapToCore); + } + public async Task DeleteAsync(Guid id) { using (var scope = ServiceScopeFactory.CreateScope()) @@ -178,6 +193,8 @@ public class AccessPolicyRepository : BaseEntityFrameworkRepository, IAccessPoli UserProjectAccessPolicy ap => Mapper.Map(ap), GroupProjectAccessPolicy ap => Mapper.Map(ap), ServiceAccountProjectAccessPolicy ap => Mapper.Map(ap), + UserServiceAccountAccessPolicy ap => Mapper.Map(ap), + GroupServiceAccountAccessPolicy ap => Mapper.Map(ap), _ => throw new ArgumentException("Unsupported access policy type") }; } diff --git a/bitwarden_license/test/Commercial.Core.Test/SecretsManager/ServiceAccounts/CreateServiceAccountCommandTests.cs b/bitwarden_license/test/Commercial.Core.Test/SecretsManager/ServiceAccounts/CreateServiceAccountCommandTests.cs index 3c3e11a6a0..7acd315651 100644 --- a/bitwarden_license/test/Commercial.Core.Test/SecretsManager/ServiceAccounts/CreateServiceAccountCommandTests.cs +++ b/bitwarden_license/test/Commercial.Core.Test/SecretsManager/ServiceAccounts/CreateServiceAccountCommandTests.cs @@ -1,4 +1,6 @@ using Bit.Commercial.Core.SecretsManager.Commands.ServiceAccounts; +using Bit.Core.Entities; +using Bit.Core.Repositories; using Bit.Core.SecretsManager.Entities; using Bit.Core.SecretsManager.Repositories; using Bit.Test.Common.AutoFixture; @@ -15,9 +17,18 @@ public class CreateServiceAccountCommandTests [Theory] [BitAutoData] public async Task CreateAsync_CallsCreate(ServiceAccount data, - SutProvider sutProvider) + Guid userId, + SutProvider sutProvider) { - await sutProvider.Sut.CreateAsync(data); + sutProvider.GetDependency() + .GetByOrganizationAsync(Arg.Any(), Arg.Any()) + .Returns(new OrganizationUser() { Id = userId }); + + sutProvider.GetDependency() + .CreateAsync(Arg.Any()) + .Returns(data); + + await sutProvider.Sut.CreateAsync(data, userId); await sutProvider.GetDependency().Received(1) .CreateAsync(Arg.Is(AssertHelper.AssertPropertyEqual(data))); diff --git a/src/Api/SecretsManager/Controllers/AccessPoliciesController.cs b/src/Api/SecretsManager/Controllers/AccessPoliciesController.cs index 493fbf5602..d2d3a4f28b 100644 --- a/src/Api/SecretsManager/Controllers/AccessPoliciesController.cs +++ b/src/Api/SecretsManager/Controllers/AccessPoliciesController.cs @@ -40,7 +40,7 @@ public class AccessPoliciesController : Controller [HttpGet("/projects/{id}/access-policies")] public async Task GetProjectAccessPoliciesAsync([FromRoute] Guid id) { - var results = await _accessPolicyRepository.GetManyByProjectId(id); + var results = await _accessPolicyRepository.GetManyByGrantedProjectIdAsync(id); return new ProjectAccessPoliciesResponseModel(results); } diff --git a/src/Api/SecretsManager/Controllers/ServiceAccountsController.cs b/src/Api/SecretsManager/Controllers/ServiceAccountsController.cs index f70db9d813..175db2a1f2 100644 --- a/src/Api/SecretsManager/Controllers/ServiceAccountsController.cs +++ b/src/Api/SecretsManager/Controllers/ServiceAccountsController.cs @@ -71,8 +71,8 @@ public class ServiceAccountsController : Controller { throw new NotFoundException(); } - - var result = await _createServiceAccountCommand.CreateAsync(createRequest.ToServiceAccount(organizationId)); + var userId = _userService.GetProperUserId(User).Value; + var result = await _createServiceAccountCommand.CreateAsync(createRequest.ToServiceAccount(organizationId), userId); return new ServiceAccountResponseModel(result); } diff --git a/src/Api/SecretsManager/Models/Response/ProjectAccessPoliciesResponseModel.cs b/src/Api/SecretsManager/Models/Response/ProjectAccessPoliciesResponseModel.cs index c7170e1367..f03411cd39 100644 --- a/src/Api/SecretsManager/Models/Response/ProjectAccessPoliciesResponseModel.cs +++ b/src/Api/SecretsManager/Models/Response/ProjectAccessPoliciesResponseModel.cs @@ -10,11 +10,6 @@ public class ProjectAccessPoliciesResponseModel : ResponseModel public ProjectAccessPoliciesResponseModel(IEnumerable baseAccessPolicies) : base(_objectName) { - if (baseAccessPolicies == null) - { - return; - } - foreach (var baseAccessPolicy in baseAccessPolicies) switch (baseAccessPolicy) { diff --git a/src/Core/SecretsManager/Commands/ServiceAccounts/Interfaces/ICreateServiceAccountCommand.cs b/src/Core/SecretsManager/Commands/ServiceAccounts/Interfaces/ICreateServiceAccountCommand.cs index 42eaf6824f..48e368b842 100644 --- a/src/Core/SecretsManager/Commands/ServiceAccounts/Interfaces/ICreateServiceAccountCommand.cs +++ b/src/Core/SecretsManager/Commands/ServiceAccounts/Interfaces/ICreateServiceAccountCommand.cs @@ -4,5 +4,5 @@ namespace Bit.Core.SecretsManager.Commands.ServiceAccounts.Interfaces; public interface ICreateServiceAccountCommand { - Task CreateAsync(ServiceAccount serviceAccount); + Task CreateAsync(ServiceAccount serviceAccount, Guid userId); } diff --git a/src/Core/SecretsManager/Repositories/IAccessPolicyRepository.cs b/src/Core/SecretsManager/Repositories/IAccessPolicyRepository.cs index 3450fdc800..dbe05074fa 100644 --- a/src/Core/SecretsManager/Repositories/IAccessPolicyRepository.cs +++ b/src/Core/SecretsManager/Repositories/IAccessPolicyRepository.cs @@ -8,7 +8,8 @@ public interface IAccessPolicyRepository Task> CreateManyAsync(List baseAccessPolicies); Task AccessPolicyExists(BaseAccessPolicy baseAccessPolicy); Task GetByIdAsync(Guid id); - Task?> GetManyByProjectId(Guid id); + Task> GetManyByGrantedProjectIdAsync(Guid id); + Task> GetManyByGrantedServiceAccountIdAsync(Guid id); Task ReplaceAsync(BaseAccessPolicy baseAccessPolicy); Task DeleteAsync(Guid id); } diff --git a/test/Api.IntegrationTest/SecretsManager/Controllers/AccessPoliciesControllerTest.cs b/test/Api.IntegrationTest/SecretsManager/Controllers/AccessPoliciesControllerTest.cs index 74b0e062b9..9d42b67e8b 100644 --- a/test/Api.IntegrationTest/SecretsManager/Controllers/AccessPoliciesControllerTest.cs +++ b/test/Api.IntegrationTest/SecretsManager/Controllers/AccessPoliciesControllerTest.cs @@ -129,6 +129,26 @@ public class AccessPoliciesControllerTest : IClassFixture Assert.Null(test); } + [Fact] + public async Task GetProjectAccessPolicies_ReturnsEmpty() + { + var initialProject = await _projectRepository.CreateAsync(new Project + { + OrganizationId = _organization.Id, + Name = _mockEncryptedString, + }); + + var response = await _client.GetAsync($"/projects/{initialProject.Id}/access-policies"); + response.EnsureSuccessStatusCode(); + + var result = await response.Content.ReadFromJsonAsync(); + + Assert.NotNull(result); + Assert.Empty(result!.UserAccessPolicies); + Assert.Empty(result!.GroupAccessPolicies); + Assert.Empty(result!.ServiceAccountAccessPolicies); + } + [Fact] public async Task GetProjectAccessPolicies() { diff --git a/test/Api.IntegrationTest/SecretsManager/Controllers/ServiceAccountsControllerTests.cs b/test/Api.IntegrationTest/SecretsManager/Controllers/ServiceAccountsControllerTests.cs index 48b1c433cf..97373422d8 100644 --- a/test/Api.IntegrationTest/SecretsManager/Controllers/ServiceAccountsControllerTests.cs +++ b/test/Api.IntegrationTest/SecretsManager/Controllers/ServiceAccountsControllerTests.cs @@ -153,6 +153,15 @@ public class ServiceAccountsControllerTest : IClassFixture().Received(1) - .GetManyByProjectId(Arg.Is(AssertHelper.AssertPropertyEqual(id))); + .GetManyByGrantedProjectIdAsync(Arg.Is(AssertHelper.AssertPropertyEqual(id))); Assert.Empty(result.GroupAccessPolicies); Assert.Empty(result.UserAccessPolicies); @@ -37,13 +37,13 @@ public class AccessPoliciesControllerTests public async void GetAccessPoliciesByProject_Success(SutProvider sutProvider, Guid id, UserProjectAccessPolicy resultAccessPolicy) { - sutProvider.GetDependency().GetManyByProjectId(default) + sutProvider.GetDependency().GetManyByGrantedProjectIdAsync(default) .ReturnsForAnyArgs(new List { resultAccessPolicy }); var result = await sutProvider.Sut.GetProjectAccessPoliciesAsync(id); await sutProvider.GetDependency().Received(1) - .GetManyByProjectId(Arg.Is(AssertHelper.AssertPropertyEqual(id))); + .GetManyByGrantedProjectIdAsync(Arg.Is(AssertHelper.AssertPropertyEqual(id))); Assert.Empty(result.GroupAccessPolicies); Assert.NotEmpty(result.UserAccessPolicies); diff --git a/test/Api.Test/SecretsManager/Controllers/ServiceAccountsControllerTests.cs b/test/Api.Test/SecretsManager/Controllers/ServiceAccountsControllerTests.cs index 7a574cb169..c6b80b1832 100644 --- a/test/Api.Test/SecretsManager/Controllers/ServiceAccountsControllerTests.cs +++ b/test/Api.Test/SecretsManager/Controllers/ServiceAccountsControllerTests.cs @@ -66,14 +66,15 @@ public class ServiceAccountsControllerTests [BitAutoData] public async void CreateServiceAccount_Success(SutProvider sutProvider, ServiceAccountCreateRequestModel data, Guid organizationId) { + sutProvider.GetDependency().GetProperUserId(default).ReturnsForAnyArgs(Guid.NewGuid()); sutProvider.GetDependency().AccessSecretsManager(organizationId).Returns(true); sutProvider.GetDependency().OrganizationUser(default).ReturnsForAnyArgs(true); var resultServiceAccount = data.ToServiceAccount(organizationId); - sutProvider.GetDependency().CreateAsync(default).ReturnsForAnyArgs(resultServiceAccount); + sutProvider.GetDependency().CreateAsync(default, default).ReturnsForAnyArgs(resultServiceAccount); await sutProvider.Sut.CreateAsync(organizationId, data); await sutProvider.GetDependency().Received(1) - .CreateAsync(Arg.Any()); + .CreateAsync(Arg.Any(), Arg.Any()); } [Theory] @@ -82,11 +83,13 @@ public class ServiceAccountsControllerTests { sutProvider.GetDependency().OrganizationUser(default).ReturnsForAnyArgs(false); var resultServiceAccount = data.ToServiceAccount(organizationId); - sutProvider.GetDependency().CreateAsync(default).ReturnsForAnyArgs(resultServiceAccount); + sutProvider.GetDependency().CreateAsync(default, default).ReturnsForAnyArgs(resultServiceAccount); await Assert.ThrowsAsync(() => sutProvider.Sut.CreateAsync(organizationId, data)); - await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().CreateAsync(Arg.Any()); + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .CreateAsync(Arg.Any(), Arg.Any()); } [Theory]