[SM-789] Extract authorization from service account delete command (#2999)

* Extract authorization from SA delete command

* swap to IEnumerable

---------

Co-authored-by: Matt Bishop <mbishop@bitwarden.com>

bitwarden_license/src/Commercial.Core/SecretsManager/AuthorizationHandlers/ServiceAccounts/ServiceAccountAuthorizationHandler.cs

@@ -44,6 +44,9 @@ public class
             case not null when requirement == ServiceAccountOperations.Update:
                 await CanUpdateServiceAccountAsync(context, requirement, resource);
                 break;
+            case not null when requirement == ServiceAccountOperations.Delete:
+                await CanDeleteServiceAccountAsync(context, requirement, resource);
+                break;
             case not null when requirement == ServiceAccountOperations.CreateAccessToken:
                 await CanCreateAccessTokenAsync(context, requirement, resource);
                 break;
@@ -107,6 +110,21 @@ public class
         }
     }
 
+    private async Task CanDeleteServiceAccountAsync(AuthorizationHandlerContext context,
+        ServiceAccountOperationRequirement requirement, ServiceAccount resource)
+    {
+        var (accessClient, userId) =
+            await _accessClientQuery.GetAccessClientAsync(context.User, resource.OrganizationId);
+        var access =
+            await _serviceAccountRepository.AccessToServiceAccountAsync(resource.Id, userId,
+                accessClient);
+
+        if (access.Write)
+        {
+            context.Succeed(requirement);
+        }
+    }
+
     private async Task CanCreateAccessTokenAsync(AuthorizationHandlerContext context,
         ServiceAccountOperationRequirement requirement, ServiceAccount resource)
     {

bitwarden_license/src/Commercial.Core/SecretsManager/Commands/ServiceAccounts/DeleteServiceAccountsCommand.cs

@@ -1,7 +1,4 @@
-using Bit.Core.Context;
+using Bit.Core.SecretsManager.Commands.ServiceAccounts.Interfaces;
-using Bit.Core.Enums;
-using Bit.Core.Exceptions;
-using Bit.Core.SecretsManager.Commands.ServiceAccounts.Interfaces;
 using Bit.Core.SecretsManager.Entities;
 using Bit.Core.SecretsManager.Repositories;
 
@@ -10,74 +7,15 @@ namespace Bit.Commercial.Core.SecretsManager.Commands.ServiceAccounts;
 public class DeleteServiceAccountsCommand : IDeleteServiceAccountsCommand
 {
     private readonly IServiceAccountRepository _serviceAccountRepository;
-    private readonly ICurrentContext _currentContext;
 
     public DeleteServiceAccountsCommand(
-        IServiceAccountRepository serviceAccountRepository,
+        IServiceAccountRepository serviceAccountRepository)
-        ICurrentContext currentContext)
     {
         _serviceAccountRepository = serviceAccountRepository;
-        _currentContext = currentContext;
     }
 
-    public async Task<List<Tuple<ServiceAccount, string>>> DeleteServiceAccounts(List<Guid> ids, Guid userId)
+    public async Task DeleteServiceAccounts(IEnumerable<ServiceAccount> serviceAccounts)
     {
-        if (ids.Any() != true || userId == new Guid())
+        await _serviceAccountRepository.DeleteManyByIdAsync(serviceAccounts.Select(sa => sa.Id));
-        {
-            throw new ArgumentNullException();
-        }
-
-        var serviceAccounts = (await _serviceAccountRepository.GetManyByIds(ids))?.ToList();
-
-        if (serviceAccounts?.Any() != true || serviceAccounts.Count != ids.Count)
-        {
-            throw new NotFoundException();
-        }
-
-        // Ensure all service accounts belongs to the same organization
-        var organizationId = serviceAccounts.First().OrganizationId;
-        if (serviceAccounts.Any(p => p.OrganizationId != organizationId))
-        {
-            throw new BadRequestException();
-        }
-
-        if (!_currentContext.AccessSecretsManager(organizationId))
-        {
-            throw new NotFoundException();
-        }
-
-        var orgAdmin = await _currentContext.OrganizationAdmin(organizationId);
-        var accessClient = AccessClientHelper.ToAccessClient(_currentContext.ClientType, orgAdmin);
-
-        var results = new List<Tuple<ServiceAccount, String>>(serviceAccounts.Count);
-        var deleteIds = new List<Guid>();
-
-        foreach (var sa in serviceAccounts)
-        {
-            var hasAccess = accessClient switch
-            {
-                AccessClientType.NoAccessCheck => true,
-                AccessClientType.User => await _serviceAccountRepository.UserHasWriteAccessToServiceAccount(sa.Id, userId),
-                _ => false,
-            };
-
-            if (!hasAccess)
-            {
-                results.Add(new Tuple<ServiceAccount, string>(sa, "access denied"));
-            }
-            else
-            {
-                results.Add(new Tuple<ServiceAccount, string>(sa, ""));
-                deleteIds.Add(sa.Id);
-            }
-        }
-
-        if (deleteIds.Count > 0)
-        {
-            await _serviceAccountRepository.DeleteManyByIdAsync(deleteIds);
-        }
-
-        return results;
     }
 }
-

bitwarden_license/test/Commercial.Core.Test/SecretsManager/AuthorizationHandlers/ServiceAccounts/ServiceAccountAuthorizationHandlerTests.cs

@@ -438,4 +438,63 @@ public class ServiceAccountAuthorizationHandlerTests
 
         Assert.Equal(expected, authzContext.HasSucceeded);
     }
+
+    [Theory]
+    [BitAutoData]
+    public async Task CanDeleteServiceAccount_AccessToSecretsManagerFalse_DoesNotSucceed(
+        SutProvider<ServiceAccountAuthorizationHandler> sutProvider, ServiceAccount serviceAccount,
+        ClaimsPrincipal claimsPrincipal)
+    {
+        var requirement = ServiceAccountOperations.Delete;
+        sutProvider.GetDependency<ICurrentContext>().AccessSecretsManager(serviceAccount.OrganizationId)
+            .Returns(false);
+        var authzContext = new AuthorizationHandlerContext(new List<IAuthorizationRequirement> { requirement },
+            claimsPrincipal, serviceAccount);
+
+        await sutProvider.Sut.HandleAsync(authzContext);
+
+        Assert.False(authzContext.HasSucceeded);
+    }
+
+    [Theory]
+    [BitAutoData]
+    public async Task CanDeleteServiceAccount_NullResource_DoesNotSucceed(
+        SutProvider<ServiceAccountAuthorizationHandler> sutProvider, ServiceAccount serviceAccount,
+        ClaimsPrincipal claimsPrincipal,
+        Guid userId)
+    {
+        var requirement = ServiceAccountOperations.Delete;
+        SetupPermission(sutProvider, PermissionType.RunAsAdmin, serviceAccount.OrganizationId, userId);
+        var authzContext = new AuthorizationHandlerContext(new List<IAuthorizationRequirement> { requirement },
+            claimsPrincipal, null);
+
+        await sutProvider.Sut.HandleAsync(authzContext);
+
+        Assert.False(authzContext.HasSucceeded);
+    }
+
+    [Theory]
+    [BitAutoData(PermissionType.RunAsAdmin, true, true, true)]
+    [BitAutoData(PermissionType.RunAsUserWithPermission, false, false, false)]
+    [BitAutoData(PermissionType.RunAsUserWithPermission, false, true, true)]
+    [BitAutoData(PermissionType.RunAsUserWithPermission, true, false, false)]
+    [BitAutoData(PermissionType.RunAsUserWithPermission, true, true, true)]
+    public async Task CanDeleteProject_AccessCheck(PermissionType permissionType, bool read, bool write,
+        bool expected,
+        SutProvider<ServiceAccountAuthorizationHandler> sutProvider, ServiceAccount serviceAccount,
+        ClaimsPrincipal claimsPrincipal,
+        Guid userId)
+    {
+        var requirement = ServiceAccountOperations.Delete;
+        SetupPermission(sutProvider, permissionType, serviceAccount.OrganizationId, userId);
+        sutProvider.GetDependency<IServiceAccountRepository>()
+            .AccessToServiceAccountAsync(serviceAccount.Id, userId, Arg.Any<AccessClientType>())
+            .Returns((read, write));
+        var authzContext = new AuthorizationHandlerContext(new List<IAuthorizationRequirement> { requirement },
+            claimsPrincipal, serviceAccount);
+
+        await sutProvider.Sut.HandleAsync(authzContext);
+
+        Assert.Equal(expected, authzContext.HasSucceeded);
+    }
 }

bitwarden_license/test/Commercial.Core.Test/SecretsManager/Commands/ServiceAccounts/DeleteServiceAccountsCommandTests.cs

@@ -0,0 +1,25 @@
+using Bit.Commercial.Core.SecretsManager.Commands.ServiceAccounts;
+using Bit.Core.SecretsManager.Entities;
+using Bit.Core.SecretsManager.Repositories;
+using Bit.Test.Common.AutoFixture;
+using Bit.Test.Common.AutoFixture.Attributes;
+using Bit.Test.Common.Helpers;
+using NSubstitute;
+using Xunit;
+
+namespace Bit.Commercial.Core.Test.SecretsManager.Commands.ServiceAccounts;
+
+[SutProviderCustomize]
+public class DeleteServiceAccountsCommandTests
+{
+    [Theory]
+    [BitAutoData]
+    public async Task DeleteServiceAccounts_Success(SutProvider<DeleteServiceAccountsCommand> sutProvider,
+        List<ServiceAccount> data)
+    {
+        await sutProvider.Sut.DeleteServiceAccounts(data);
+        await sutProvider.GetDependency<IServiceAccountRepository>()
+            .Received(1)
+            .DeleteManyByIdAsync(Arg.Is(AssertHelper.AssertPropertyEqual(data.Select(d => d.Id))));
+    }
+}

src/Api/SecretsManager/Controllers/ServiceAccountsController.cs

@@ -7,6 +7,7 @@ using Bit.Core.Exceptions;
 using Bit.Core.SecretsManager.AuthorizationRequirements;
 using Bit.Core.SecretsManager.Commands.AccessTokens.Interfaces;
 using Bit.Core.SecretsManager.Commands.ServiceAccounts.Interfaces;
+using Bit.Core.SecretsManager.Entities;
 using Bit.Core.SecretsManager.Repositories;
 using Bit.Core.Services;
 using Bit.Core.Utilities;
@@ -130,10 +131,40 @@ public class ServiceAccountsController : Controller
     [HttpPost("delete")]
     public async Task<ListResponseModel<BulkDeleteResponseModel>> BulkDeleteAsync([FromBody] List<Guid> ids)
     {
-        var userId = _userService.GetProperUserId(User).Value;
+        var serviceAccounts = (await _serviceAccountRepository.GetManyByIds(ids)).ToList();
+        if (!serviceAccounts.Any() || serviceAccounts.Count != ids.Count)
+        {
+            throw new NotFoundException();
+        }
 
-        var results = await _deleteServiceAccountsCommand.DeleteServiceAccounts(ids, userId);
+        // Ensure all service accounts belong to the same organization
-        var responses = results.Select(r => new BulkDeleteResponseModel(r.Item1.Id, r.Item2));
+        var organizationId = serviceAccounts.First().OrganizationId;
+        if (serviceAccounts.Any(sa => sa.OrganizationId != organizationId) ||
+            !_currentContext.AccessSecretsManager(organizationId))
+        {
+            throw new NotFoundException();
+        }
+
+        var serviceAccountsToDelete = new List<ServiceAccount>();
+        var results = new List<(ServiceAccount ServiceAccount, string Error)>();
+
+        foreach (var serviceAccount in serviceAccounts)
+        {
+            var authorizationResult =
+                await _authorizationService.AuthorizeAsync(User, serviceAccount, ServiceAccountOperations.Delete);
+            if (authorizationResult.Succeeded)
+            {
+                serviceAccountsToDelete.Add(serviceAccount);
+                results.Add((serviceAccount, ""));
+            }
+            else
+            {
+                results.Add((serviceAccount, "access denied"));
+            }
+        }
+
+        await _deleteServiceAccountsCommand.DeleteServiceAccounts(serviceAccountsToDelete);
+        var responses = results.Select(r => new BulkDeleteResponseModel(r.ServiceAccount.Id, r.Error));
         return new ListResponseModel<BulkDeleteResponseModel>(responses);
     }
 

src/Core/SecretsManager/AuthorizationRequirements/ServiceAccountOperationRequirement.cs

@@ -11,6 +11,7 @@ public static class ServiceAccountOperations
     public static readonly ServiceAccountOperationRequirement Create = new() { Name = nameof(Create) };
     public static readonly ServiceAccountOperationRequirement Read = new() { Name = nameof(Read) };
     public static readonly ServiceAccountOperationRequirement Update = new() { Name = nameof(Update) };
+    public static readonly ServiceAccountOperationRequirement Delete = new() { Name = nameof(Delete) };
     public static readonly ServiceAccountOperationRequirement ReadAccessTokens = new() { Name = nameof(ReadAccessTokens) };
     public static readonly ServiceAccountOperationRequirement CreateAccessToken = new() { Name = nameof(CreateAccessToken) };
     public static readonly ServiceAccountOperationRequirement RevokeAccessTokens = new() { Name = nameof(RevokeAccessTokens) };

src/Core/SecretsManager/Commands/ServiceAccounts/Interfaces/IDeleteServiceAccountsCommand.cs

@@ -4,6 +4,6 @@ namespace Bit.Core.SecretsManager.Commands.ServiceAccounts.Interfaces;
 
 public interface IDeleteServiceAccountsCommand
 {
-    Task<List<Tuple<ServiceAccount, string>>> DeleteServiceAccounts(List<Guid> ids, Guid userId);
+    Task DeleteServiceAccounts(IEnumerable<ServiceAccount> serviceAccounts);
 }
 

test/Api.Test/SecretsManager/Controllers/ServiceAccountsControllerTests.cs

@@ -260,4 +260,108 @@ public class ServiceAccountsControllerTests
         await sutProvider.GetDependency<IRevokeAccessTokensCommand>().Received(1)
             .RevokeAsync(Arg.Any<ServiceAccount>(), Arg.Any<Guid[]>());
     }
+
+    [Theory]
+    [BitAutoData]
+    public async void BulkDelete_NoServiceAccountsFound_ThrowsNotFound(SutProvider<ServiceAccountsController> sutProvider, List<ServiceAccount> data)
+    {
+        var ids = data.Select(sa => sa.Id).ToList();
+        sutProvider.GetDependency<IServiceAccountRepository>().GetManyByIds(Arg.Is(ids)).ReturnsForAnyArgs(new List<ServiceAccount>());
+        await Assert.ThrowsAsync<NotFoundException>(() => sutProvider.Sut.BulkDeleteAsync(ids));
+        await sutProvider.GetDependency<IDeleteServiceAccountsCommand>().DidNotReceiveWithAnyArgs().DeleteServiceAccounts(Arg.Any<List<ServiceAccount>>());
+    }
+
+    [Theory]
+    [BitAutoData]
+    public async void BulkDelete_ServiceAccountsFoundMisMatch_ThrowsNotFound(SutProvider<ServiceAccountsController> sutProvider, List<ServiceAccount> data, ServiceAccount mockSa)
+    {
+        data.Add(mockSa);
+        var ids = data.Select(sa => sa.Id).ToList();
+        sutProvider.GetDependency<IServiceAccountRepository>().GetManyByIds(Arg.Is(ids)).ReturnsForAnyArgs(new List<ServiceAccount> { mockSa });
+        await Assert.ThrowsAsync<NotFoundException>(() => sutProvider.Sut.BulkDeleteAsync(ids));
+        await sutProvider.GetDependency<IDeleteServiceAccountsCommand>().DidNotReceiveWithAnyArgs().DeleteServiceAccounts(Arg.Any<List<ServiceAccount>>());
+    }
+
+    [Theory]
+    [BitAutoData]
+    public async void BulkDelete_OrganizationMistMatch_ThrowsNotFound(SutProvider<ServiceAccountsController> sutProvider, List<ServiceAccount> data)
+    {
+        var ids = data.Select(sa => sa.Id).ToList();
+        sutProvider.GetDependency<IServiceAccountRepository>().GetManyByIds(Arg.Is(ids)).ReturnsForAnyArgs(data);
+        await Assert.ThrowsAsync<NotFoundException>(() => sutProvider.Sut.BulkDeleteAsync(ids));
+        await sutProvider.GetDependency<IDeleteServiceAccountsCommand>().DidNotReceiveWithAnyArgs().DeleteServiceAccounts(Arg.Any<List<ServiceAccount>>());
+    }
+
+    [Theory]
+    [BitAutoData]
+    public async void BulkDelete_NoAccessToSecretsManager_ThrowsNotFound(SutProvider<ServiceAccountsController> sutProvider, List<ServiceAccount> data)
+    {
+        var ids = data.Select(sa => sa.Id).ToList();
+        var organizationId = data.First().OrganizationId;
+        foreach (var sa in data)
+        {
+            sa.OrganizationId = organizationId;
+        }
+        sutProvider.GetDependency<ICurrentContext>().AccessSecretsManager(Arg.Is(organizationId)).ReturnsForAnyArgs(false);
+        sutProvider.GetDependency<IServiceAccountRepository>().GetManyByIds(Arg.Is(ids)).ReturnsForAnyArgs(data);
+        await Assert.ThrowsAsync<NotFoundException>(() => sutProvider.Sut.BulkDeleteAsync(ids));
+        await sutProvider.GetDependency<IDeleteServiceAccountsCommand>().DidNotReceiveWithAnyArgs().DeleteServiceAccounts(Arg.Any<List<ServiceAccount>>());
+    }
+
+    [Theory]
+    [BitAutoData]
+    public async void BulkDelete_ReturnsAccessDeniedForProjectsWithoutAccess_Success(SutProvider<ServiceAccountsController> sutProvider, List<ServiceAccount> data)
+    {
+        var ids = data.Select(sa => sa.Id).ToList();
+        var organizationId = data.First().OrganizationId;
+        foreach (var sa in data)
+        {
+            sa.OrganizationId = organizationId;
+            sutProvider.GetDependency<IAuthorizationService>()
+                .AuthorizeAsync(Arg.Any<ClaimsPrincipal>(), sa,
+                    Arg.Any<IEnumerable<IAuthorizationRequirement>>()).ReturnsForAnyArgs(AuthorizationResult.Success());
+        }
+        sutProvider.GetDependency<IAuthorizationService>()
+            .AuthorizeAsync(Arg.Any<ClaimsPrincipal>(), data.First(),
+                Arg.Any<IEnumerable<IAuthorizationRequirement>>()).Returns(AuthorizationResult.Failed());
+        sutProvider.GetDependency<ICurrentContext>().AccessSecretsManager(Arg.Is(organizationId)).ReturnsForAnyArgs(true);
+        sutProvider.GetDependency<IServiceAccountRepository>().GetManyByIds(Arg.Is(ids)).ReturnsForAnyArgs(data);
+
+        var results = await sutProvider.Sut.BulkDeleteAsync(ids);
+
+        Assert.Equal(data.Count, results.Data.Count());
+        Assert.Equal("access denied", results.Data.First().Error);
+
+        data.Remove(data.First());
+        await sutProvider.GetDependency<IDeleteServiceAccountsCommand>().Received(1)
+            .DeleteServiceAccounts(Arg.Is(AssertHelper.AssertPropertyEqual(data)));
+    }
+
+    [Theory]
+    [BitAutoData]
+    public async void BulkDelete_Success(SutProvider<ServiceAccountsController> sutProvider, List<ServiceAccount> data)
+    {
+        var ids = data.Select(sa => sa.Id).ToList();
+        var organizationId = data.First().OrganizationId;
+        foreach (var sa in data)
+        {
+            sa.OrganizationId = organizationId;
+            sutProvider.GetDependency<IAuthorizationService>()
+                .AuthorizeAsync(Arg.Any<ClaimsPrincipal>(), sa,
+                    Arg.Any<IEnumerable<IAuthorizationRequirement>>()).ReturnsForAnyArgs(AuthorizationResult.Success());
+        }
+
+        sutProvider.GetDependency<ICurrentContext>().AccessSecretsManager(Arg.Is(organizationId)).ReturnsForAnyArgs(true);
+        sutProvider.GetDependency<IServiceAccountRepository>().GetManyByIds(Arg.Is(ids)).ReturnsForAnyArgs(data);
+
+        var results = await sutProvider.Sut.BulkDeleteAsync(ids);
+
+        await sutProvider.GetDependency<IDeleteServiceAccountsCommand>().Received(1)
+            .DeleteServiceAccounts(Arg.Is(AssertHelper.AssertPropertyEqual(data)));
+        Assert.Equal(data.Count, results.Data.Count());
+        foreach (var result in results.Data)
+        {
+            Assert.Null(result.Error);
+        }
+    }
 }