Address PR Feedback

bitwarden · Jan 27, 2024 · f92532c · f92532c
1 parent 1462cff
commit f92532c
Show file tree

Hide file tree

Showing 6 changed files with 227 additions and 8 deletions.
diff --git a/...rcial.Core/SecretsManager/AuthorizationHandlers/Secrets/BulkSecretAuthorizationHandler.cs b/...rcial.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<bool> CanBulkUpdateSecretsAsync(

diff --git a/....Test/SecretsManager/AuthorizationHandlers/Secrets/BulkSecretAuthorizationHandlerTests.cs b/....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<BulkSecretAuthorizationHandler> sutProvider,
+      ClaimsPrincipal claimsPrincipal,
+      Secret secret1,
+      Secret secret2)
+    {
+        var secrets = new List<Secret> { secret1, secret2 };
+
+        var authorizationContext = new AuthorizationHandlerContext(
+          new List<IAuthorizationRequirement> { BulkSecretOperations.Update },
+          claimsPrincipal,
+          secrets);
+
+        await sutProvider.Sut.HandleAsync(authorizationContext);
+
+        Assert.False(authorizationContext.HasSucceeded);
+    }
+
+    [Theory]
+    [BitAutoData]
+    public async Task HandleAsync_DoesNotHaveAccessToSm_DoesNotSucceed(
+      SutProvider<BulkSecretAuthorizationHandler> sutProvider,
+      ClaimsPrincipal claimsPrincipal,
+      Secret secret1,
+      Secret secret2)
+    {
+        secret2.OrganizationId = secret1.OrganizationId;
+
+        sutProvider.GetDependency<ICurrentContext>()
+          .AccessSecretsManager(secret1.OrganizationId)
+          .Returns(false);
+
+        var secrets = new List<Secret> { secret1, secret2 };
+
+        var authorizationContext = new AuthorizationHandlerContext(
+          new List<IAuthorizationRequirement> { BulkSecretOperations.Update },
+          claimsPrincipal,
+          secrets);
+
+        await sutProvider.Sut.HandleAsync(authorizationContext);
+
+        Assert.False(authorizationContext.HasSucceeded);
+    }
+
+    [Theory]
+    [BitAutoData]
+    public async Task HandleAsync_UnknownRequirement_DoesNotSucceed(
+      SutProvider<BulkSecretAuthorizationHandler> sutProvider,
+      ClaimsPrincipal claimsPrincipal,
+      Secret secret1,
+      Secret secret2,
+      Guid userId)
+    {
+        secret2.OrganizationId = secret1.OrganizationId;
+
+        sutProvider.GetDependency<ICurrentContext>()
+          .AccessSecretsManager(secret1.OrganizationId)
+          .Returns(true);
+
+        sutProvider.GetDependency<IAccessClientQuery>()
+          .GetAccessClientAsync(claimsPrincipal, secret1.OrganizationId)
+          .Returns((AccessClientType.User, userId));
+
+        var secrets = new List<Secret> { secret1, secret2 };
+
+        var authorizationContext = new AuthorizationHandlerContext(
+          new List<IAuthorizationRequirement> { new BulkSecretOperationRequirement { Name = "Something" } },
+          claimsPrincipal,
+          secrets);
+
+        await sutProvider.Sut.HandleAsync(authorizationContext);
+
+        Assert.False(authorizationContext.HasSucceeded);
+    }
+
+    [Theory]
+    [BitAutoData]
+    public async Task HandleAsync_DoesNotHaveWriteAccessToAllSecrets_DoesNotSucceed(
+      SutProvider<BulkSecretAuthorizationHandler> sutProvider,
+      ClaimsPrincipal claimsPrincipal,
+      Secret secret1,
+      Secret secret2,
+      Guid userId)
+    {
+        secret2.OrganizationId = secret1.OrganizationId;
+
+        sutProvider.GetDependency<ICurrentContext>()
+          .AccessSecretsManager(secret1.OrganizationId)
+          .Returns(true);
+
+        sutProvider.GetDependency<IAccessClientQuery>()
+          .GetAccessClientAsync(claimsPrincipal, secret1.OrganizationId)
+          .Returns((AccessClientType.User, userId));
+
+        sutProvider.GetDependency<ISecretRepository>()
+          .AccessToSecretsAsync(
+            Arg.Is<Guid[]>(arr => arr.Length == 2 && arr[0] == secret1.Id && arr[1] == secret2.Id),
+            userId,
+            AccessClientType.User
+          )
+          .Returns(new Dictionary<Guid, (bool Read, bool Write)>
+          {
+              [secret1.Id] = (true, false),
+              [secret2.Id] = (true, true),
+          });
+
+        var secrets = new List<Secret> { secret1, secret2 };
+
+        var authorizationContext = new AuthorizationHandlerContext(
+          new List<IAuthorizationRequirement> { BulkSecretOperations.Update },
+          claimsPrincipal,
+          secrets);
+
+        await sutProvider.Sut.HandleAsync(authorizationContext);
+
+        Assert.False(authorizationContext.HasSucceeded);
+    }
+
+    [Theory]
+    [BitAutoData]
+    public async Task HandleAsync_HasWriteAccessToAllSecrets_Succeeds(
+      SutProvider<BulkSecretAuthorizationHandler> sutProvider,
+      ClaimsPrincipal claimsPrincipal,
+      Secret secret1,
+      Secret secret2,
+      Guid userId)
+    {
+        secret2.OrganizationId = secret1.OrganizationId;
+
+        sutProvider.GetDependency<ICurrentContext>()
+          .AccessSecretsManager(secret1.OrganizationId)
+          .Returns(true);
+
+        sutProvider.GetDependency<IAccessClientQuery>()
+          .GetAccessClientAsync(claimsPrincipal, secret1.OrganizationId)
+          .Returns((AccessClientType.User, userId));
+
+        sutProvider.GetDependency<ISecretRepository>()
+          .AccessToSecretsAsync(
+            Arg.Is<Guid[]>(arr => arr.Length == 2 && arr[0] == secret1.Id && arr[1] == secret2.Id),
+            userId,
+            AccessClientType.User
+          )
+          .Returns(new Dictionary<Guid, (bool Read, bool Write)>
+          {
+              [secret1.Id] = (true, true),
+              [secret2.Id] = (true, true),
+          });
+
+        var secrets = new List<Secret> { secret1, secret2 };
+
+        var authorizationContext = new AuthorizationHandlerContext(
+          new List<IAuthorizationRequirement> { 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
@@ -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
@@ -148,8 +148,6 @@ protected override void OnModelCreating(ModelBuilder builder)
         eOrganizationDomain.ToTable(nameof(OrganizationDomain));
         aWebAuthnCredential.ToTable(nameof(WebAuthnCredential));
 
-        builder.Entity<ProjectSecret>(b => b.ToTable(nameof(ProjectSecret)));
-
         ConfigureDateTimeUtcQueries(builder);
     }
 

diff --git a/...ure.EntityFramework/SecretsManager/Configurations/ProjectSecretEntityTypeConfiguration.cs b/...ure.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<ProjectSecret>
+{
+    public void Configure(EntityTypeBuilder<ProjectSecret> builder)
+    {
+        builder.ToTable(nameof(ProjectSecret));
+    }
+}
diff --git 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<Guid> secretIds)> CreateSecretsAsync(Guid orgId, int numberToCreate = 3)
     {
         var project = await _projectRepository.CreateAsync(new Project
-Original file line number
+Diff line change
@@ Expand Up / @@ -52,9 +52,9 @@ protected override async Task HandleRequirementAsync( @@
                 {
                     return;
                 }
-            }
-            context.Succeed(requirement);
+                context.Succeed(requirement);
+            }
         }
         private async Task<bool> CanBulkUpdateSecretsAsync(
@@ Expand Down @@