diff --git a/bitwarden_license/src/Commercial.Core/SecretsManager/AuthorizationHandlers/ServiceAccounts/ServiceAccountAuthorizationHandler.cs b/bitwarden_license/src/Commercial.Core/SecretsManager/AuthorizationHandlers/ServiceAccounts/ServiceAccountAuthorizationHandler.cs index dac784986..2c4e78d10 100644 --- a/bitwarden_license/src/Commercial.Core/SecretsManager/AuthorizationHandlers/ServiceAccounts/ServiceAccountAuthorizationHandler.cs +++ b/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) { diff --git a/bitwarden_license/src/Commercial.Core/SecretsManager/Commands/ServiceAccounts/DeleteServiceAccountsCommand.cs b/bitwarden_license/src/Commercial.Core/SecretsManager/Commands/ServiceAccounts/DeleteServiceAccountsCommand.cs index 39d340d75..7853b4873 100644 --- a/bitwarden_license/src/Commercial.Core/SecretsManager/Commands/ServiceAccounts/DeleteServiceAccountsCommand.cs +++ b/bitwarden_license/src/Commercial.Core/SecretsManager/Commands/ServiceAccounts/DeleteServiceAccountsCommand.cs @@ -1,7 +1,4 @@ -using Bit.Core.Context; -using Bit.Core.Enums; -using Bit.Core.Exceptions; -using Bit.Core.SecretsManager.Commands.ServiceAccounts.Interfaces; +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, - ICurrentContext currentContext) + IServiceAccountRepository serviceAccountRepository) { _serviceAccountRepository = serviceAccountRepository; - _currentContext = currentContext; } - public async Task>> DeleteServiceAccounts(List ids, Guid userId) + public async Task DeleteServiceAccounts(IEnumerable serviceAccounts) { - if (ids.Any() != true || userId == new Guid()) - { - 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>(serviceAccounts.Count); - var deleteIds = new List(); - - 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(sa, "access denied")); - } - else - { - results.Add(new Tuple(sa, "")); - deleteIds.Add(sa.Id); - } - } - - if (deleteIds.Count > 0) - { - await _serviceAccountRepository.DeleteManyByIdAsync(deleteIds); - } - - return results; + await _serviceAccountRepository.DeleteManyByIdAsync(serviceAccounts.Select(sa => sa.Id)); } } - diff --git a/bitwarden_license/test/Commercial.Core.Test/SecretsManager/AuthorizationHandlers/ServiceAccounts/ServiceAccountAuthorizationHandlerTests.cs b/bitwarden_license/test/Commercial.Core.Test/SecretsManager/AuthorizationHandlers/ServiceAccounts/ServiceAccountAuthorizationHandlerTests.cs index 2da2eafde..b7804a700 100644 --- a/bitwarden_license/test/Commercial.Core.Test/SecretsManager/AuthorizationHandlers/ServiceAccounts/ServiceAccountAuthorizationHandlerTests.cs +++ b/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 sutProvider, ServiceAccount serviceAccount, + ClaimsPrincipal claimsPrincipal) + { + var requirement = ServiceAccountOperations.Delete; + sutProvider.GetDependency().AccessSecretsManager(serviceAccount.OrganizationId) + .Returns(false); + var authzContext = new AuthorizationHandlerContext(new List { requirement }, + claimsPrincipal, serviceAccount); + + await sutProvider.Sut.HandleAsync(authzContext); + + Assert.False(authzContext.HasSucceeded); + } + + [Theory] + [BitAutoData] + public async Task CanDeleteServiceAccount_NullResource_DoesNotSucceed( + SutProvider sutProvider, ServiceAccount serviceAccount, + ClaimsPrincipal claimsPrincipal, + Guid userId) + { + var requirement = ServiceAccountOperations.Delete; + SetupPermission(sutProvider, PermissionType.RunAsAdmin, serviceAccount.OrganizationId, userId); + var authzContext = new AuthorizationHandlerContext(new List { 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 sutProvider, ServiceAccount serviceAccount, + ClaimsPrincipal claimsPrincipal, + Guid userId) + { + var requirement = ServiceAccountOperations.Delete; + SetupPermission(sutProvider, permissionType, serviceAccount.OrganizationId, userId); + sutProvider.GetDependency() + .AccessToServiceAccountAsync(serviceAccount.Id, userId, Arg.Any()) + .Returns((read, write)); + var authzContext = new AuthorizationHandlerContext(new List { requirement }, + claimsPrincipal, serviceAccount); + + await sutProvider.Sut.HandleAsync(authzContext); + + Assert.Equal(expected, authzContext.HasSucceeded); + } } diff --git a/bitwarden_license/test/Commercial.Core.Test/SecretsManager/Commands/ServiceAccounts/DeleteServiceAccountsCommandTests.cs b/bitwarden_license/test/Commercial.Core.Test/SecretsManager/Commands/ServiceAccounts/DeleteServiceAccountsCommandTests.cs new file mode 100644 index 000000000..8805c511d --- /dev/null +++ b/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 sutProvider, + List data) + { + await sutProvider.Sut.DeleteServiceAccounts(data); + await sutProvider.GetDependency() + .Received(1) + .DeleteManyByIdAsync(Arg.Is(AssertHelper.AssertPropertyEqual(data.Select(d => d.Id)))); + } +} diff --git a/src/Api/SecretsManager/Controllers/ServiceAccountsController.cs b/src/Api/SecretsManager/Controllers/ServiceAccountsController.cs index 57f0b38a7..1b7307b8a 100644 --- a/src/Api/SecretsManager/Controllers/ServiceAccountsController.cs +++ b/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> BulkDeleteAsync([FromBody] List 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); - var responses = results.Select(r => new BulkDeleteResponseModel(r.Item1.Id, r.Item2)); + // Ensure all service accounts belong to the same organization + var organizationId = serviceAccounts.First().OrganizationId; + if (serviceAccounts.Any(sa => sa.OrganizationId != organizationId) || + !_currentContext.AccessSecretsManager(organizationId)) + { + throw new NotFoundException(); + } + + var serviceAccountsToDelete = new List(); + 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(responses); } diff --git a/src/Core/SecretsManager/AuthorizationRequirements/ServiceAccountOperationRequirement.cs b/src/Core/SecretsManager/AuthorizationRequirements/ServiceAccountOperationRequirement.cs index 3a079e62e..23f312d0b 100644 --- a/src/Core/SecretsManager/AuthorizationRequirements/ServiceAccountOperationRequirement.cs +++ b/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) }; diff --git a/src/Core/SecretsManager/Commands/ServiceAccounts/Interfaces/IDeleteServiceAccountsCommand.cs b/src/Core/SecretsManager/Commands/ServiceAccounts/Interfaces/IDeleteServiceAccountsCommand.cs index 23260b06b..aaea373f8 100644 --- a/src/Core/SecretsManager/Commands/ServiceAccounts/Interfaces/IDeleteServiceAccountsCommand.cs +++ b/src/Core/SecretsManager/Commands/ServiceAccounts/Interfaces/IDeleteServiceAccountsCommand.cs @@ -4,6 +4,6 @@ namespace Bit.Core.SecretsManager.Commands.ServiceAccounts.Interfaces; public interface IDeleteServiceAccountsCommand { - Task>> DeleteServiceAccounts(List ids, Guid userId); + Task DeleteServiceAccounts(IEnumerable serviceAccounts); } diff --git a/test/Api.Test/SecretsManager/Controllers/ServiceAccountsControllerTests.cs b/test/Api.Test/SecretsManager/Controllers/ServiceAccountsControllerTests.cs index 2b5095384..f075d0ec3 100644 --- a/test/Api.Test/SecretsManager/Controllers/ServiceAccountsControllerTests.cs +++ b/test/Api.Test/SecretsManager/Controllers/ServiceAccountsControllerTests.cs @@ -260,4 +260,108 @@ public class ServiceAccountsControllerTests await sutProvider.GetDependency().Received(1) .RevokeAsync(Arg.Any(), Arg.Any()); } + + [Theory] + [BitAutoData] + public async void BulkDelete_NoServiceAccountsFound_ThrowsNotFound(SutProvider sutProvider, List data) + { + var ids = data.Select(sa => sa.Id).ToList(); + sutProvider.GetDependency().GetManyByIds(Arg.Is(ids)).ReturnsForAnyArgs(new List()); + await Assert.ThrowsAsync(() => sutProvider.Sut.BulkDeleteAsync(ids)); + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().DeleteServiceAccounts(Arg.Any>()); + } + + [Theory] + [BitAutoData] + public async void BulkDelete_ServiceAccountsFoundMisMatch_ThrowsNotFound(SutProvider sutProvider, List data, ServiceAccount mockSa) + { + data.Add(mockSa); + var ids = data.Select(sa => sa.Id).ToList(); + sutProvider.GetDependency().GetManyByIds(Arg.Is(ids)).ReturnsForAnyArgs(new List { mockSa }); + await Assert.ThrowsAsync(() => sutProvider.Sut.BulkDeleteAsync(ids)); + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().DeleteServiceAccounts(Arg.Any>()); + } + + [Theory] + [BitAutoData] + public async void BulkDelete_OrganizationMistMatch_ThrowsNotFound(SutProvider sutProvider, List data) + { + var ids = data.Select(sa => sa.Id).ToList(); + sutProvider.GetDependency().GetManyByIds(Arg.Is(ids)).ReturnsForAnyArgs(data); + await Assert.ThrowsAsync(() => sutProvider.Sut.BulkDeleteAsync(ids)); + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().DeleteServiceAccounts(Arg.Any>()); + } + + [Theory] + [BitAutoData] + public async void BulkDelete_NoAccessToSecretsManager_ThrowsNotFound(SutProvider sutProvider, List data) + { + var ids = data.Select(sa => sa.Id).ToList(); + var organizationId = data.First().OrganizationId; + foreach (var sa in data) + { + sa.OrganizationId = organizationId; + } + sutProvider.GetDependency().AccessSecretsManager(Arg.Is(organizationId)).ReturnsForAnyArgs(false); + sutProvider.GetDependency().GetManyByIds(Arg.Is(ids)).ReturnsForAnyArgs(data); + await Assert.ThrowsAsync(() => sutProvider.Sut.BulkDeleteAsync(ids)); + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().DeleteServiceAccounts(Arg.Any>()); + } + + [Theory] + [BitAutoData] + public async void BulkDelete_ReturnsAccessDeniedForProjectsWithoutAccess_Success(SutProvider sutProvider, List data) + { + var ids = data.Select(sa => sa.Id).ToList(); + var organizationId = data.First().OrganizationId; + foreach (var sa in data) + { + sa.OrganizationId = organizationId; + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), sa, + Arg.Any>()).ReturnsForAnyArgs(AuthorizationResult.Success()); + } + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), data.First(), + Arg.Any>()).Returns(AuthorizationResult.Failed()); + sutProvider.GetDependency().AccessSecretsManager(Arg.Is(organizationId)).ReturnsForAnyArgs(true); + sutProvider.GetDependency().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().Received(1) + .DeleteServiceAccounts(Arg.Is(AssertHelper.AssertPropertyEqual(data))); + } + + [Theory] + [BitAutoData] + public async void BulkDelete_Success(SutProvider sutProvider, List data) + { + var ids = data.Select(sa => sa.Id).ToList(); + var organizationId = data.First().OrganizationId; + foreach (var sa in data) + { + sa.OrganizationId = organizationId; + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), sa, + Arg.Any>()).ReturnsForAnyArgs(AuthorizationResult.Success()); + } + + sutProvider.GetDependency().AccessSecretsManager(Arg.Is(organizationId)).ReturnsForAnyArgs(true); + sutProvider.GetDependency().GetManyByIds(Arg.Is(ids)).ReturnsForAnyArgs(data); + + var results = await sutProvider.Sut.BulkDeleteAsync(ids); + + await sutProvider.GetDependency().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); + } + } }