From f92532cd244a1ec1bb3d232f6644e03cab35e545 Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Sat, 27 Jan 2024 13:23:57 +0000 Subject: [PATCH] Address PR Feedback --- .../Secrets/BulkSecretAuthorizationHandler.cs | 4 +- .../BulkSecretAuthorizationHandlerTests.cs | 184 ++++++++++++++++++ .../Controllers/SecretsController.cs | 8 +- .../Repositories/DatabaseContext.cs | 2 - .../ProjectSecretEntityTypeConfiguration.cs | 13 ++ .../Controllers/SecretsControllerTests.cs | 24 +++ 6 files changed, 227 insertions(+), 8 deletions(-) create mode 100644 bitwarden_license/test/Commercial.Core.Test/SecretsManager/AuthorizationHandlers/Secrets/BulkSecretAuthorizationHandlerTests.cs create mode 100644 src/Infrastructure.EntityFramework/SecretsManager/Configurations/ProjectSecretEntityTypeConfiguration.cs diff --git a/bitwarden_license/src/Commercial.Core/SecretsManager/AuthorizationHandlers/Secrets/BulkSecretAuthorizationHandler.cs b/bitwarden_license/src/Commercial.Core/SecretsManager/AuthorizationHandlers/Secrets/BulkSecretAuthorizationHandler.cs index 6f271ed15381..851f0dc50d5b 100644 --- a/bitwarden_license/src/Commercial.Core/SecretsManager/AuthorizationHandlers/Secrets/BulkSecretAuthorizationHandler.cs +++ b/bitwarden_license/src/Commercial.Core/SecretsManager/AuthorizationHandlers/Secrets/BulkSecretAuthorizationHandler.cs @@ -52,9 +52,9 @@ protected override async Task HandleRequirementAsync( { return; } - } - context.Succeed(requirement); + context.Succeed(requirement); + } } private async Task CanBulkUpdateSecretsAsync( diff --git a/bitwarden_license/test/Commercial.Core.Test/SecretsManager/AuthorizationHandlers/Secrets/BulkSecretAuthorizationHandlerTests.cs b/bitwarden_license/test/Commercial.Core.Test/SecretsManager/AuthorizationHandlers/Secrets/BulkSecretAuthorizationHandlerTests.cs new file mode 100644 index 000000000000..473b476da91f --- /dev/null +++ b/bitwarden_license/test/Commercial.Core.Test/SecretsManager/AuthorizationHandlers/Secrets/BulkSecretAuthorizationHandlerTests.cs @@ -0,0 +1,184 @@ +using System.Security.Claims; +using Bit.Commercial.Core.SecretsManager.AuthorizationHandlers.Secrets; +using Bit.Core.Context; +using Bit.Core.Enums; +using Bit.Core.SecretsManager.AuthorizationRequirements; +using Bit.Core.SecretsManager.Entities; +using Bit.Core.SecretsManager.Queries.Interfaces; +using Bit.Core.SecretsManager.Repositories; +using Bit.Core.Test.SecretsManager.AutoFixture.ProjectsFixture; +using Bit.Test.Common.AutoFixture; +using Bit.Test.Common.AutoFixture.Attributes; +using Microsoft.AspNetCore.Authorization; +using NSubstitute; +using Xunit; + +namespace Bit.Commercial.Core.Test.SecretsManager.AuthorizationHandlers.Secrets; + +[SutProviderCustomize] +[ProjectCustomize] +public class BulkSecretAuthorizationHandlerTests +{ + [Theory] + [BitAutoData] + public async Task HandleAsync_DifferentOrganizations_DoesNotSucceed( + SutProvider sutProvider, + ClaimsPrincipal claimsPrincipal, + Secret secret1, + Secret secret2) + { + var secrets = new List { secret1, secret2 }; + + var authorizationContext = new AuthorizationHandlerContext( + new List { BulkSecretOperations.Update }, + claimsPrincipal, + secrets); + + await sutProvider.Sut.HandleAsync(authorizationContext); + + Assert.False(authorizationContext.HasSucceeded); + } + + [Theory] + [BitAutoData] + public async Task HandleAsync_DoesNotHaveAccessToSm_DoesNotSucceed( + SutProvider sutProvider, + ClaimsPrincipal claimsPrincipal, + Secret secret1, + Secret secret2) + { + secret2.OrganizationId = secret1.OrganizationId; + + sutProvider.GetDependency() + .AccessSecretsManager(secret1.OrganizationId) + .Returns(false); + + var secrets = new List { secret1, secret2 }; + + var authorizationContext = new AuthorizationHandlerContext( + new List { BulkSecretOperations.Update }, + claimsPrincipal, + secrets); + + await sutProvider.Sut.HandleAsync(authorizationContext); + + Assert.False(authorizationContext.HasSucceeded); + } + + [Theory] + [BitAutoData] + public async Task HandleAsync_UnknownRequirement_DoesNotSucceed( + SutProvider sutProvider, + ClaimsPrincipal claimsPrincipal, + Secret secret1, + Secret secret2, + Guid userId) + { + secret2.OrganizationId = secret1.OrganizationId; + + sutProvider.GetDependency() + .AccessSecretsManager(secret1.OrganizationId) + .Returns(true); + + sutProvider.GetDependency() + .GetAccessClientAsync(claimsPrincipal, secret1.OrganizationId) + .Returns((AccessClientType.User, userId)); + + var secrets = new List { secret1, secret2 }; + + var authorizationContext = new AuthorizationHandlerContext( + new List { new BulkSecretOperationRequirement { Name = "Something" } }, + claimsPrincipal, + secrets); + + await sutProvider.Sut.HandleAsync(authorizationContext); + + Assert.False(authorizationContext.HasSucceeded); + } + + [Theory] + [BitAutoData] + public async Task HandleAsync_DoesNotHaveWriteAccessToAllSecrets_DoesNotSucceed( + SutProvider sutProvider, + ClaimsPrincipal claimsPrincipal, + Secret secret1, + Secret secret2, + Guid userId) + { + secret2.OrganizationId = secret1.OrganizationId; + + sutProvider.GetDependency() + .AccessSecretsManager(secret1.OrganizationId) + .Returns(true); + + sutProvider.GetDependency() + .GetAccessClientAsync(claimsPrincipal, secret1.OrganizationId) + .Returns((AccessClientType.User, userId)); + + sutProvider.GetDependency() + .AccessToSecretsAsync( + Arg.Is(arr => arr.Length == 2 && arr[0] == secret1.Id && arr[1] == secret2.Id), + userId, + AccessClientType.User + ) + .Returns(new Dictionary + { + [secret1.Id] = (true, false), + [secret2.Id] = (true, true), + }); + + var secrets = new List { secret1, secret2 }; + + var authorizationContext = new AuthorizationHandlerContext( + new List { BulkSecretOperations.Update }, + claimsPrincipal, + secrets); + + await sutProvider.Sut.HandleAsync(authorizationContext); + + Assert.False(authorizationContext.HasSucceeded); + } + + [Theory] + [BitAutoData] + public async Task HandleAsync_HasWriteAccessToAllSecrets_Succeeds( + SutProvider sutProvider, + ClaimsPrincipal claimsPrincipal, + Secret secret1, + Secret secret2, + Guid userId) + { + secret2.OrganizationId = secret1.OrganizationId; + + sutProvider.GetDependency() + .AccessSecretsManager(secret1.OrganizationId) + .Returns(true); + + sutProvider.GetDependency() + .GetAccessClientAsync(claimsPrincipal, secret1.OrganizationId) + .Returns((AccessClientType.User, userId)); + + sutProvider.GetDependency() + .AccessToSecretsAsync( + Arg.Is(arr => arr.Length == 2 && arr[0] == secret1.Id && arr[1] == secret2.Id), + userId, + AccessClientType.User + ) + .Returns(new Dictionary + { + [secret1.Id] = (true, true), + [secret2.Id] = (true, true), + }); + + var secrets = new List { secret1, secret2 }; + + var authorizationContext = new AuthorizationHandlerContext( + new List { BulkSecretOperations.Update }, + claimsPrincipal, + secrets); + + await sutProvider.Sut.HandleAsync(authorizationContext); + + Assert.True(authorizationContext.HasSucceeded); + } +} diff --git a/src/Api/SecretsManager/Controllers/SecretsController.cs b/src/Api/SecretsManager/Controllers/SecretsController.cs index e78eed0bd81a..d0d4af1f1ae2 100644 --- a/src/Api/SecretsManager/Controllers/SecretsController.cs +++ b/src/Api/SecretsManager/Controllers/SecretsController.cs @@ -264,16 +264,16 @@ public async Task BulkMoveToProjectAsync(Guid organizationId, [FromBody] BulkMov throw new NotFoundException(); } - var authorizationResult = await _authorizationService.AuthorizeAsync( - User, secrets.AsReadOnly(), BulkSecretOperations.Update); + var project = await _projectRepository.GetByIdAsync(request.Project); + var authorizationResult = await _authorizationService.AuthorizeAsync(User, project, ProjectOperations.Update); if (!authorizationResult.Succeeded) { throw new NotFoundException(); } - var project = await _projectRepository.GetByIdAsync(request.Project); - authorizationResult = await _authorizationService.AuthorizeAsync(User, project, ProjectOperations.Update); + authorizationResult = await _authorizationService.AuthorizeAsync( + User, secrets.AsReadOnly(), BulkSecretOperations.Update); if (!authorizationResult.Succeeded) { diff --git a/src/Infrastructure.EntityFramework/Repositories/DatabaseContext.cs b/src/Infrastructure.EntityFramework/Repositories/DatabaseContext.cs index d315b028a5a6..621916f0905d 100644 --- a/src/Infrastructure.EntityFramework/Repositories/DatabaseContext.cs +++ b/src/Infrastructure.EntityFramework/Repositories/DatabaseContext.cs @@ -148,8 +148,6 @@ protected override void OnModelCreating(ModelBuilder builder) eOrganizationDomain.ToTable(nameof(OrganizationDomain)); aWebAuthnCredential.ToTable(nameof(WebAuthnCredential)); - builder.Entity(b => b.ToTable(nameof(ProjectSecret))); - ConfigureDateTimeUtcQueries(builder); } diff --git a/src/Infrastructure.EntityFramework/SecretsManager/Configurations/ProjectSecretEntityTypeConfiguration.cs b/src/Infrastructure.EntityFramework/SecretsManager/Configurations/ProjectSecretEntityTypeConfiguration.cs new file mode 100644 index 000000000000..1dc36794edc8 --- /dev/null +++ b/src/Infrastructure.EntityFramework/SecretsManager/Configurations/ProjectSecretEntityTypeConfiguration.cs @@ -0,0 +1,13 @@ +using Bit.Infrastructure.EntityFramework.SecretsManager.Models; +using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Metadata.Builders; + +namespace Bit.Infrastructure.EntityFramework.SecretsManager.Configurations; + +public class ProjectSecretEntityTypeConfiguration : IEntityTypeConfiguration +{ + public void Configure(EntityTypeBuilder builder) + { + builder.ToTable(nameof(ProjectSecret)); + } +} diff --git a/test/Api.IntegrationTest/SecretsManager/Controllers/SecretsControllerTests.cs b/test/Api.IntegrationTest/SecretsManager/Controllers/SecretsControllerTests.cs index 0643b92b1e99..3c702e16d2ec 100644 --- a/test/Api.IntegrationTest/SecretsManager/Controllers/SecretsControllerTests.cs +++ b/test/Api.IntegrationTest/SecretsManager/Controllers/SecretsControllerTests.cs @@ -865,6 +865,30 @@ public async Task BulkMoveToProjectAsync_CannotEditSecret_Fails() Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); } + [Theory] + [InlineData(false, false, false)] + [InlineData(false, false, true)] + [InlineData(false, true, false)] + [InlineData(false, true, true)] + [InlineData(true, false, false)] + [InlineData(true, false, true)] + [InlineData(true, true, false)] + public async Task BulkMoveToProjectAsync_SmAccessDenied_Fails(bool useSecrets, bool accessSecrets, bool organizationEnabled) + { + var (org, _) = await _organizationHelper.Initialize(useSecrets, accessSecrets, organizationEnabled); + await LoginAsync(_email); + var (_, secretIds) = await CreateSecretsAsync(org.Id); + var (newProject, _) = await CreateSecretsAsync(org.Id); + + var response = await _client.PostAsJsonAsync($"/organizations/{org.Id}/secrets/move", new BulkMoveToProjectsRequestModel + { + Secrets = secretIds, + Project = newProject.Id, + }); + + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + } + private async Task<(Project Project, List secretIds)> CreateSecretsAsync(Guid orgId, int numberToCreate = 3) { var project = await _projectRepository.CreateAsync(new Project