From 2529c5b36fe2a8821ba5c852196c5bd132e12abf Mon Sep 17 00:00:00 2001 From: Colton Hurst Date: Fri, 14 Apr 2023 09:48:11 -0400 Subject: [PATCH] SM-695: Block Create & Update for Admins on Secrets Outside of the Org (#2844) * SM-695: Block create or update for admins on secrets outside of the org * SM-695: Update test, org is required on project * SM-695: Update tests to set matching org id in project * SM-695: Ensure there is no more than 1 project connected to a secret, plus remove org admin check in the CreateSecretCommand. * SM-695: Add integration tests for create and update secrets security fixes * SM-695: Update Create and Update secret tests, a secret can only be in one project at a time --- .../Commands/Secrets/CreateSecretCommand.cs | 5 + .../Commands/Secrets/UpdateSecretCommand.cs | 5 + .../Repositories/ProjectRepository.cs | 9 ++ .../Secrets/CreateSecretCommandTests.cs | 3 + .../Secrets/UpdateSecretCommandTests.cs | 3 + .../Controllers/SecretsController.cs | 10 ++ .../Repositories/IProjectRepository.cs | 1 + .../Controllers/SecretsControllerTests.cs | 100 +++++++++++++++++- .../Controllers/SecretsControllerTests.cs | 12 +++ 9 files changed, 147 insertions(+), 1 deletion(-) diff --git a/bitwarden_license/src/Commercial.Core/SecretsManager/Commands/Secrets/CreateSecretCommand.cs b/bitwarden_license/src/Commercial.Core/SecretsManager/Commands/Secrets/CreateSecretCommand.cs index d71f741b0..8f0ce365e 100644 --- a/bitwarden_license/src/Commercial.Core/SecretsManager/Commands/Secrets/CreateSecretCommand.cs +++ b/bitwarden_license/src/Commercial.Core/SecretsManager/Commands/Secrets/CreateSecretCommand.cs @@ -31,6 +31,11 @@ public class CreateSecretCommand : ICreateSecretCommand throw new NotFoundException(); } + if (secret.Projects != null && secret.Projects.Any() && !(await _projectRepository.ProjectsAreInOrganization(secret.Projects.Select(p => p.Id).ToList(), secret.OrganizationId))) + { + throw new NotFoundException(); + } + var hasAccess = accessClient switch { AccessClientType.NoAccessCheck => true, diff --git a/bitwarden_license/src/Commercial.Core/SecretsManager/Commands/Secrets/UpdateSecretCommand.cs b/bitwarden_license/src/Commercial.Core/SecretsManager/Commands/Secrets/UpdateSecretCommand.cs index 1d5511444..350ad65fd 100644 --- a/bitwarden_license/src/Commercial.Core/SecretsManager/Commands/Secrets/UpdateSecretCommand.cs +++ b/bitwarden_license/src/Commercial.Core/SecretsManager/Commands/Secrets/UpdateSecretCommand.cs @@ -28,6 +28,11 @@ public class UpdateSecretCommand : IUpdateSecretCommand throw new NotFoundException(); } + if (updatedSecret.Projects != null && updatedSecret.Projects.Any() && !(await _projectRepository.ProjectsAreInOrganization(updatedSecret.Projects.Select(p => p.Id).ToList(), secret.OrganizationId))) + { + throw new NotFoundException(); + } + var orgAdmin = await _currentContext.OrganizationAdmin(secret.OrganizationId); var accessClient = AccessClientHelper.ToAccessClient(_currentContext.ClientType, orgAdmin); diff --git a/bitwarden_license/src/Commercial.Infrastructure.EntityFramework/SecretsManager/Repositories/ProjectRepository.cs b/bitwarden_license/src/Commercial.Infrastructure.EntityFramework/SecretsManager/Repositories/ProjectRepository.cs index fe5936cf1..676b76966 100644 --- a/bitwarden_license/src/Commercial.Infrastructure.EntityFramework/SecretsManager/Repositories/ProjectRepository.cs +++ b/bitwarden_license/src/Commercial.Infrastructure.EntityFramework/SecretsManager/Repositories/ProjectRepository.cs @@ -190,4 +190,13 @@ public class ProjectRepository : Repository ProjectsAreInOrganization(List projectIds, Guid organizationId) + { + using var scope = ServiceScopeFactory.CreateScope(); + var dbContext = GetDatabaseContext(scope); + var results = await dbContext.Project.Where(p => p.OrganizationId == organizationId && projectIds.Contains(p.Id)).ToListAsync(); + + return projectIds.Count == results.Count; + } } diff --git a/bitwarden_license/test/Commercial.Core.Test/SecretsManager/Secrets/CreateSecretCommandTests.cs b/bitwarden_license/test/Commercial.Core.Test/SecretsManager/Secrets/CreateSecretCommandTests.cs index 78b8c2a02..9ae71ee8d 100644 --- a/bitwarden_license/test/Commercial.Core.Test/SecretsManager/Secrets/CreateSecretCommandTests.cs +++ b/bitwarden_license/test/Commercial.Core.Test/SecretsManager/Secrets/CreateSecretCommandTests.cs @@ -22,6 +22,9 @@ public class CreateSecretCommandTests public async Task CreateAsync_Success(PermissionType permissionType, Secret data, SutProvider sutProvider, Guid userId, Project mockProject) { + sutProvider.GetDependency().ProjectsAreInOrganization(default, default).ReturnsForAnyArgs(true); + + mockProject.OrganizationId = data.OrganizationId; data.Projects = new List() { mockProject }; if (permissionType == PermissionType.RunAsAdmin) diff --git a/bitwarden_license/test/Commercial.Core.Test/SecretsManager/Secrets/UpdateSecretCommandTests.cs b/bitwarden_license/test/Commercial.Core.Test/SecretsManager/Secrets/UpdateSecretCommandTests.cs index 5fe37c697..c348acf13 100644 --- a/bitwarden_license/test/Commercial.Core.Test/SecretsManager/Secrets/UpdateSecretCommandTests.cs +++ b/bitwarden_license/test/Commercial.Core.Test/SecretsManager/Secrets/UpdateSecretCommandTests.cs @@ -34,6 +34,9 @@ public class UpdateSecretCommandTests public async Task UpdateAsync_Success(PermissionType permissionType, Secret data, SutProvider sutProvider, Guid userId, Project mockProject) { sutProvider.GetDependency().AccessSecretsManager(data.OrganizationId).Returns(true); + sutProvider.GetDependency().ProjectsAreInOrganization(default, default).ReturnsForAnyArgs(true); + + mockProject.OrganizationId = data.OrganizationId; data.Projects = new List() { mockProject }; if (permissionType == PermissionType.RunAsAdmin) diff --git a/src/Api/SecretsManager/Controllers/SecretsController.cs b/src/Api/SecretsManager/Controllers/SecretsController.cs index fd60c15fb..5a8ace4f9 100644 --- a/src/Api/SecretsManager/Controllers/SecretsController.cs +++ b/src/Api/SecretsManager/Controllers/SecretsController.cs @@ -80,6 +80,11 @@ public class SecretsController : Controller throw new NotFoundException(); } + if (createRequest.ProjectIds != null && createRequest.ProjectIds.Length > 1) + { + throw new BadRequestException(); + } + var userId = _userService.GetProperUserId(User).Value; var result = await _createSecretCommand.CreateAsync(createRequest.ToSecret(organizationId), userId); @@ -140,6 +145,11 @@ public class SecretsController : Controller [HttpPut("secrets/{id}")] public async Task UpdateSecretAsync([FromRoute] Guid id, [FromBody] SecretUpdateRequestModel updateRequest) { + if (updateRequest.ProjectIds != null && updateRequest.ProjectIds.Length > 1) + { + throw new BadRequestException(); + } + var userId = _userService.GetProperUserId(User).Value; var secret = updateRequest.ToSecret(id); var result = await _updateSecretCommand.UpdateAsync(secret, userId); diff --git a/src/Core/SecretsManager/Repositories/IProjectRepository.cs b/src/Core/SecretsManager/Repositories/IProjectRepository.cs index 9bb234cd5..7ad76077f 100644 --- a/src/Core/SecretsManager/Repositories/IProjectRepository.cs +++ b/src/Core/SecretsManager/Repositories/IProjectRepository.cs @@ -18,4 +18,5 @@ public interface IProjectRepository Task ServiceAccountHasWriteAccessToProject(Guid id, Guid userId); Task ServiceAccountHasReadAccessToProject(Guid id, Guid userId); Task<(bool Read, bool Write)> AccessToProjectAsync(Guid id, Guid userId, AccessClientType accessType); + Task ProjectsAreInOrganization(List projectIds, Guid organizationId); } diff --git a/test/Api.IntegrationTest/SecretsManager/Controllers/SecretsControllerTests.cs b/test/Api.IntegrationTest/SecretsManager/Controllers/SecretsControllerTests.cs index 1ea7c4049..6a8b4f829 100644 --- a/test/Api.IntegrationTest/SecretsManager/Controllers/SecretsControllerTests.cs +++ b/test/Api.IntegrationTest/SecretsManager/Controllers/SecretsControllerTests.cs @@ -148,7 +148,7 @@ public class SecretsControllerTests : IClassFixture, IAsy var (org, _) = await _organizationHelper.Initialize(true, true); await LoginAsync(_email); - var project = await _projectRepository.CreateAsync(new Project { Name = "123" }); + var project = await _projectRepository.CreateAsync(new Project { OrganizationId = org.Id, Name = "123" }); var request = new SecretCreateRequestModel { @@ -179,6 +179,47 @@ public class SecretsControllerTests : IClassFixture, IAsy Assert.Null(createdSecret.DeletedDate); } + [Fact] + public async Task CreateWithDifferentProjectOrgId_RunAsAdmin_NotFound() + { + var (org, _) = await _organizationHelper.Initialize(true, true); + await LoginAsync(_email); + + var project = await _projectRepository.CreateAsync(new Project { Name = "123" }); + + var request = new SecretCreateRequestModel + { + ProjectIds = new Guid[] { project.Id }, + Key = _mockEncryptedString, + Value = _mockEncryptedString, + Note = _mockEncryptedString, + }; + + var response = await _client.PostAsJsonAsync($"/organizations/{org.Id}/secrets", request); + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + } + + [Fact] + public async Task CreateWithMultipleProjects_RunAsAdmin_BadRequest() + { + var (org, _) = await _organizationHelper.Initialize(true, true); + await LoginAsync(_email); + + var projectA = await _projectRepository.CreateAsync(new Project { OrganizationId = org.Id, Name = "123A" }); + var projectB = await _projectRepository.CreateAsync(new Project { OrganizationId = org.Id, Name = "123B" }); + + var request = new SecretCreateRequestModel + { + ProjectIds = new Guid[] { projectA.Id, projectB.Id }, + Key = _mockEncryptedString, + Value = _mockEncryptedString, + Note = _mockEncryptedString, + }; + + var response = await _client.PostAsJsonAsync($"/organizations/{org.Id}/secrets", request); + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + } + [Fact] public async Task CreateWithoutProject_RunAsUser_NotFound() { @@ -531,6 +572,63 @@ public class SecretsControllerTests : IClassFixture, IAsy Assert.NotEqual(secret.RevisionDate, updatedSecret.RevisionDate); } + [Fact] + public async Task UpdateWithDifferentProjectOrgId_RunAsAdmin_NotFound() + { + var (org, _) = await _organizationHelper.Initialize(true, true); + await LoginAsync(_email); + + var project = await _projectRepository.CreateAsync(new Project { Name = "123" }); + + var secret = await _secretRepository.CreateAsync(new Secret + { + OrganizationId = org.Id, + Key = _mockEncryptedString, + Value = _mockEncryptedString, + Note = _mockEncryptedString + }); + + var request = new SecretUpdateRequestModel + { + Key = _mockEncryptedString, + Value = "2.3Uk+WNBIoU5xzmVFNcoWzz==|1MsPIYuRfdOHfu/0uY6H2Q==|/98xy4wb6pHP1VTZ9JcNCYgQjEUMFPlqJgCwRk1YXKg=", + Note = _mockEncryptedString, + ProjectIds = new Guid[] { project.Id }, + }; + + var response = await _client.PutAsJsonAsync($"/secrets/{secret.Id}", request); + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + } + + [Fact] + public async Task UpdateWithMultipleProjects_BadRequest() + { + var (org, _) = await _organizationHelper.Initialize(true, true); + await LoginAsync(_email); + + var projectA = await _projectRepository.CreateAsync(new Project { OrganizationId = org.Id, Name = "123A" }); + var projectB = await _projectRepository.CreateAsync(new Project { OrganizationId = org.Id, Name = "123B" }); + + var secret = await _secretRepository.CreateAsync(new Secret + { + OrganizationId = org.Id, + Key = _mockEncryptedString, + Value = _mockEncryptedString, + Note = _mockEncryptedString + }); + + var request = new SecretUpdateRequestModel + { + Key = _mockEncryptedString, + Value = "2.3Uk+WNBIoU5xzmVFNcoWzz==|1MsPIYuRfdOHfu/0uY6H2Q==|/98xy4wb6pHP1VTZ9JcNCYgQjEUMFPlqJgCwRk1YXKg=", + Note = _mockEncryptedString, + ProjectIds = new Guid[] { projectA.Id, projectB.Id }, + }; + + var response = await _client.PutAsJsonAsync($"/secrets/{secret.Id}", request); + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + } + [Theory] [InlineData(false, false)] [InlineData(true, false)] diff --git a/test/Api.Test/SecretsManager/Controllers/SecretsControllerTests.cs b/test/Api.Test/SecretsManager/Controllers/SecretsControllerTests.cs index 6877c2871..54e7bcf90 100644 --- a/test/Api.Test/SecretsManager/Controllers/SecretsControllerTests.cs +++ b/test/Api.Test/SecretsManager/Controllers/SecretsControllerTests.cs @@ -125,6 +125,12 @@ public class SecretsControllerTests [BitAutoData(PermissionType.RunAsUserWithPermission)] public async void CreateSecret_Success(PermissionType permissionType, SutProvider sutProvider, SecretCreateRequestModel data, Guid organizationId, Project mockProject, Guid userId) { + // We currently only allow a secret to be in one project at a time + if (data.ProjectIds != null && data.ProjectIds.Length > 1) + { + data.ProjectIds = new Guid[] { data.ProjectIds.ElementAt(0) }; + } + var resultSecret = data.ToSecret(organizationId); sutProvider.GetDependency().GetProperUserId(default).ReturnsForAnyArgs(userId); @@ -152,6 +158,12 @@ public class SecretsControllerTests [BitAutoData(PermissionType.RunAsUserWithPermission)] public async void UpdateSecret_Success(PermissionType permissionType, SutProvider sutProvider, SecretUpdateRequestModel data, Guid secretId, Guid organizationId, Guid userId, Project mockProject) { + // We currently only allow a secret to be in one project at a time + if (data.ProjectIds != null && data.ProjectIds.Length > 1) + { + data.ProjectIds = new Guid[] { data.ProjectIds.ElementAt(0) }; + } + sutProvider.GetDependency().GetProperUserId(default).ReturnsForAnyArgs(userId); if (permissionType == PermissionType.RunAsAdmin)