From d1155ee376f05884f47a2f746e66f658abfa4965 Mon Sep 17 00:00:00 2001 From: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com> Date: Wed, 31 May 2023 13:49:58 -0500 Subject: [PATCH] [SM-704] Extract Authorization For ServiceAccounts (#2869) * Move to access query for project commands * Swap to hasAccess method per action * Swap to authorization handler pattern * Move ProjectOperationRequirement to Core * Add default throw + tests * Extract authorization out of commands * Unit tests for authorization handler * Formatting * Swap to reflection for testing switch * Swap to check read & reflections in test * fix wording on exception * Refactor GetAccessClient into its own query * Use accessClientQuery in project handler --- .../Projects/ProjectAuthorizationHandler.cs | 22 +- .../ServiceAccountAuthorizationHandler.cs | 100 ++++++ .../UpdateServiceAccountCommand.cs | 23 +- .../Queries/AccessClientQuery.cs | 28 ++ .../SecretsManagerCollectionExtensions.cs | 5 + .../Repositories/ServiceAccountRepository.cs | 29 ++ .../ProjectAuthorizationHandlerTests.cs | 43 ++- ...ServiceAccountAuthorizationHandlerTests.cs | 302 ++++++++++++++++++ .../UpdateServiceAccountCommandTests.cs | 56 +--- .../Controllers/ServiceAccountsController.cs | 52 ++- .../ServiceAccountOperationRequirement.cs | 14 + .../IUpdateServiceAccountCommand.cs | 2 +- .../Queries/Interfaces/IAccessClientQuery.cs | 9 + .../Repositories/IServiceAccountRepository.cs | 1 + .../ServiceAccountsControllerTests.cs | 199 ++++++------ .../ServiceAccountsControllerTests.cs | 58 ++-- 16 files changed, 694 insertions(+), 249 deletions(-) create mode 100644 bitwarden_license/src/Commercial.Core/SecretsManager/AuthorizationHandlers/ServiceAccounts/ServiceAccountAuthorizationHandler.cs create mode 100644 bitwarden_license/src/Commercial.Core/SecretsManager/Queries/AccessClientQuery.cs create mode 100644 bitwarden_license/test/Commercial.Core.Test/SecretsManager/AuthorizationHandlers/ServiceAccounts/ServiceAccountAuthorizationHandlerTests.cs create mode 100644 src/Core/SecretsManager/AuthorizationRequirements/ServiceAccountOperationRequirement.cs create mode 100644 src/Core/SecretsManager/Queries/Interfaces/IAccessClientQuery.cs diff --git a/bitwarden_license/src/Commercial.Core/SecretsManager/AuthorizationHandlers/Projects/ProjectAuthorizationHandler.cs b/bitwarden_license/src/Commercial.Core/SecretsManager/AuthorizationHandlers/Projects/ProjectAuthorizationHandler.cs index 0d898a704..744942224 100644 --- a/bitwarden_license/src/Commercial.Core/SecretsManager/AuthorizationHandlers/Projects/ProjectAuthorizationHandler.cs +++ b/bitwarden_license/src/Commercial.Core/SecretsManager/AuthorizationHandlers/Projects/ProjectAuthorizationHandler.cs @@ -2,23 +2,23 @@ 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.Services; using Microsoft.AspNetCore.Authorization; namespace Bit.Commercial.Core.SecretsManager.AuthorizationHandlers.Projects; public class ProjectAuthorizationHandler : AuthorizationHandler { + private readonly IAccessClientQuery _accessClientQuery; private readonly ICurrentContext _currentContext; private readonly IProjectRepository _projectRepository; - private readonly IUserService _userService; - public ProjectAuthorizationHandler(ICurrentContext currentContext, IUserService userService, + public ProjectAuthorizationHandler(ICurrentContext currentContext, IAccessClientQuery accessClientQuery, IProjectRepository projectRepository) { _currentContext = currentContext; - _userService = userService; + _accessClientQuery = accessClientQuery; _projectRepository = projectRepository; } @@ -40,14 +40,14 @@ public class ProjectAuthorizationHandler : AuthorizationHandler true, @@ -65,13 +65,13 @@ public class ProjectAuthorizationHandler : AuthorizationHandler GetAccessClientAsync(Guid organizationId) - { - var orgAdmin = await _currentContext.OrganizationAdmin(organizationId); - return AccessClientHelper.ToAccessClient(_currentContext.ClientType, orgAdmin); - } } diff --git a/bitwarden_license/src/Commercial.Core/SecretsManager/AuthorizationHandlers/ServiceAccounts/ServiceAccountAuthorizationHandler.cs b/bitwarden_license/src/Commercial.Core/SecretsManager/AuthorizationHandlers/ServiceAccounts/ServiceAccountAuthorizationHandler.cs new file mode 100644 index 000000000..9a62b22c3 --- /dev/null +++ b/bitwarden_license/src/Commercial.Core/SecretsManager/AuthorizationHandlers/ServiceAccounts/ServiceAccountAuthorizationHandler.cs @@ -0,0 +1,100 @@ +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 Microsoft.AspNetCore.Authorization; + +namespace Bit.Commercial.Core.SecretsManager.AuthorizationHandlers.ServiceAccounts; + +public class + ServiceAccountAuthorizationHandler : AuthorizationHandler +{ + private readonly IAccessClientQuery _accessClientQuery; + private readonly ICurrentContext _currentContext; + private readonly IServiceAccountRepository _serviceAccountRepository; + + public ServiceAccountAuthorizationHandler(ICurrentContext currentContext, + IAccessClientQuery accessClientQuery, + IServiceAccountRepository serviceAccountRepository) + { + _currentContext = currentContext; + _accessClientQuery = accessClientQuery; + _serviceAccountRepository = serviceAccountRepository; + } + + protected override async Task HandleRequirementAsync(AuthorizationHandlerContext context, + ServiceAccountOperationRequirement requirement, + ServiceAccount resource) + { + if (!_currentContext.AccessSecretsManager(resource.OrganizationId)) + { + return; + } + + switch (requirement) + { + case not null when requirement == ServiceAccountOperations.Create: + await CanCreateServiceAccountAsync(context, requirement, resource); + break; + case not null when requirement == ServiceAccountOperations.Read: + await CanReadServiceAccountAsync(context, requirement, resource); + break; + case not null when requirement == ServiceAccountOperations.Update: + await CanUpdateServiceAccountAsync(context, requirement, resource); + break; + default: + throw new ArgumentException("Unsupported operation requirement type provided.", + nameof(requirement)); + } + } + + private async Task CanCreateServiceAccountAsync(AuthorizationHandlerContext context, + ServiceAccountOperationRequirement requirement, ServiceAccount resource) + { + var (accessClient, _) = await _accessClientQuery.GetAccessClientAsync(context.User, resource.OrganizationId); + var hasAccess = accessClient switch + { + AccessClientType.NoAccessCheck => true, + AccessClientType.User => true, + AccessClientType.ServiceAccount => false, + _ => false, + }; + + if (hasAccess) + { + context.Succeed(requirement); + } + } + + private async Task CanReadServiceAccountAsync(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.Read) + { + context.Succeed(requirement); + } + } + + private async Task CanUpdateServiceAccountAsync(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); + } + } +} diff --git a/bitwarden_license/src/Commercial.Core/SecretsManager/Commands/ServiceAccounts/UpdateServiceAccountCommand.cs b/bitwarden_license/src/Commercial.Core/SecretsManager/Commands/ServiceAccounts/UpdateServiceAccountCommand.cs index 378f3cbc3..ac1d2cc08 100644 --- a/bitwarden_license/src/Commercial.Core/SecretsManager/Commands/ServiceAccounts/UpdateServiceAccountCommand.cs +++ b/bitwarden_license/src/Commercial.Core/SecretsManager/Commands/ServiceAccounts/UpdateServiceAccountCommand.cs @@ -1,5 +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.Entities; @@ -18,7 +17,7 @@ public class UpdateServiceAccountCommand : IUpdateServiceAccountCommand _currentContext = currentContext; } - public async Task UpdateAsync(ServiceAccount updatedServiceAccount, Guid userId) + public async Task UpdateAsync(ServiceAccount updatedServiceAccount) { var serviceAccount = await _serviceAccountRepository.GetByIdAsync(updatedServiceAccount.Id); if (serviceAccount == null) @@ -26,26 +25,6 @@ public class UpdateServiceAccountCommand : IUpdateServiceAccountCommand throw new NotFoundException(); } - if (!_currentContext.AccessSecretsManager(serviceAccount.OrganizationId)) - { - throw new NotFoundException(); - } - - var orgAdmin = await _currentContext.OrganizationAdmin(serviceAccount.OrganizationId); - var accessClient = AccessClientHelper.ToAccessClient(_currentContext.ClientType, orgAdmin); - - var hasAccess = accessClient switch - { - AccessClientType.NoAccessCheck => true, - AccessClientType.User => await _serviceAccountRepository.UserHasWriteAccessToServiceAccount(updatedServiceAccount.Id, userId), - _ => false, - }; - - if (!hasAccess) - { - throw new NotFoundException(); - } - serviceAccount.Name = updatedServiceAccount.Name; serviceAccount.RevisionDate = DateTime.UtcNow; diff --git a/bitwarden_license/src/Commercial.Core/SecretsManager/Queries/AccessClientQuery.cs b/bitwarden_license/src/Commercial.Core/SecretsManager/Queries/AccessClientQuery.cs new file mode 100644 index 000000000..101734506 --- /dev/null +++ b/bitwarden_license/src/Commercial.Core/SecretsManager/Queries/AccessClientQuery.cs @@ -0,0 +1,28 @@ +using System.Security.Claims; +using Bit.Core.Context; +using Bit.Core.Enums; +using Bit.Core.SecretsManager.Queries.Interfaces; +using Bit.Core.Services; + +namespace Bit.Commercial.Core.SecretsManager.Queries; + +public class AccessClientQuery : IAccessClientQuery +{ + private readonly ICurrentContext _currentContext; + private readonly IUserService _userService; + + public AccessClientQuery(ICurrentContext currentContext, IUserService userService) + { + _currentContext = currentContext; + _userService = userService; + } + + public async Task<(AccessClientType AccessClientType, Guid UserId)> GetAccessClientAsync( + ClaimsPrincipal claimsPrincipal, Guid organizationId) + { + var orgAdmin = await _currentContext.OrganizationAdmin(organizationId); + var accessClient = AccessClientHelper.ToAccessClient(_currentContext.ClientType, orgAdmin); + var userId = _userService.GetProperUserId(claimsPrincipal).Value; + return (accessClient, userId); + } +} diff --git a/bitwarden_license/src/Commercial.Core/SecretsManager/SecretsManagerCollectionExtensions.cs b/bitwarden_license/src/Commercial.Core/SecretsManager/SecretsManagerCollectionExtensions.cs index 149bc690a..4f01f7460 100644 --- a/bitwarden_license/src/Commercial.Core/SecretsManager/SecretsManagerCollectionExtensions.cs +++ b/bitwarden_license/src/Commercial.Core/SecretsManager/SecretsManagerCollectionExtensions.cs @@ -1,4 +1,5 @@ using Bit.Commercial.Core.SecretsManager.AuthorizationHandlers.Projects; +using Bit.Commercial.Core.SecretsManager.AuthorizationHandlers.ServiceAccounts; using Bit.Commercial.Core.SecretsManager.Commands.AccessPolicies; using Bit.Commercial.Core.SecretsManager.Commands.AccessTokens; using Bit.Commercial.Core.SecretsManager.Commands.Porting; @@ -6,6 +7,7 @@ using Bit.Commercial.Core.SecretsManager.Commands.Projects; using Bit.Commercial.Core.SecretsManager.Commands.Secrets; using Bit.Commercial.Core.SecretsManager.Commands.ServiceAccounts; using Bit.Commercial.Core.SecretsManager.Commands.Trash; +using Bit.Commercial.Core.SecretsManager.Queries; using Bit.Core.SecretsManager.Commands.AccessPolicies.Interfaces; using Bit.Core.SecretsManager.Commands.AccessTokens.Interfaces; using Bit.Core.SecretsManager.Commands.Porting.Interfaces; @@ -13,6 +15,7 @@ using Bit.Core.SecretsManager.Commands.Projects.Interfaces; using Bit.Core.SecretsManager.Commands.Secrets.Interfaces; using Bit.Core.SecretsManager.Commands.ServiceAccounts.Interfaces; using Bit.Core.SecretsManager.Commands.Trash.Interfaces; +using Bit.Core.SecretsManager.Queries.Interfaces; using Microsoft.AspNetCore.Authorization; using Microsoft.Extensions.DependencyInjection; @@ -23,6 +26,8 @@ public static class SecretsManagerCollectionExtensions public static void AddSecretsManagerServices(this IServiceCollection services) { services.AddScoped(); + services.AddScoped(); + services.AddScoped(); services.AddScoped(); services.AddScoped(); services.AddScoped(); diff --git a/bitwarden_license/src/Commercial.Infrastructure.EntityFramework/SecretsManager/Repositories/ServiceAccountRepository.cs b/bitwarden_license/src/Commercial.Infrastructure.EntityFramework/SecretsManager/Repositories/ServiceAccountRepository.cs index 9b2bfd54f..492c0c0d8 100644 --- a/bitwarden_license/src/Commercial.Infrastructure.EntityFramework/SecretsManager/Repositories/ServiceAccountRepository.cs +++ b/bitwarden_license/src/Commercial.Infrastructure.EntityFramework/SecretsManager/Repositories/ServiceAccountRepository.cs @@ -101,6 +101,35 @@ public class ServiceAccountRepository : Repository AccessToServiceAccountAsync(Guid id, Guid userId, + AccessClientType accessType) + { + using var scope = ServiceScopeFactory.CreateScope(); + var dbContext = GetDatabaseContext(scope); + + var serviceAccount = dbContext.ServiceAccount.Where(sa => sa.Id == id); + + var query = accessType switch + { + AccessClientType.NoAccessCheck => serviceAccount.Select(_ => new { Read = true, Write = true }), + AccessClientType.User => serviceAccount.Select(sa => new + { + Read = sa.UserAccessPolicies.Any(ap => ap.OrganizationUser.User.Id == userId && ap.Read) || + sa.GroupAccessPolicies.Any(ap => + ap.Group.GroupUsers.Any(gu => gu.OrganizationUser.User.Id == userId && ap.Read)), + Write = sa.UserAccessPolicies.Any(ap => ap.OrganizationUser.User.Id == userId && ap.Write) || + sa.GroupAccessPolicies.Any(ap => + ap.Group.GroupUsers.Any(gu => gu.OrganizationUser.User.Id == userId && ap.Write)), + }), + AccessClientType.ServiceAccount => serviceAccount.Select(_ => new { Read = false, Write = false }), + _ => serviceAccount.Select(_ => new { Read = false, Write = false }), + }; + + var policy = await query.FirstOrDefaultAsync(); + + return policy == null ? (false, false) : (policy.Read, policy.Write); + } + private static Expression> UserHasReadAccessToServiceAccount(Guid userId) => sa => sa.UserAccessPolicies.Any(ap => ap.OrganizationUser.User.Id == userId && ap.Read) || sa.GroupAccessPolicies.Any(ap => ap.Group.GroupUsers.Any(gu => gu.OrganizationUser.User.Id == userId && ap.Read)); diff --git a/bitwarden_license/test/Commercial.Core.Test/SecretsManager/AuthorizationHandlers/Projects/ProjectAuthorizationHandlerTests.cs b/bitwarden_license/test/Commercial.Core.Test/SecretsManager/AuthorizationHandlers/Projects/ProjectAuthorizationHandlerTests.cs index 3e9b73256..07feb35b8 100644 --- a/bitwarden_license/test/Commercial.Core.Test/SecretsManager/AuthorizationHandlers/Projects/ProjectAuthorizationHandlerTests.cs +++ b/bitwarden_license/test/Commercial.Core.Test/SecretsManager/AuthorizationHandlers/Projects/ProjectAuthorizationHandlerTests.cs @@ -4,11 +4,10 @@ using Bit.Commercial.Core.SecretsManager.AuthorizationHandlers.Projects; using Bit.Commercial.Core.Test.SecretsManager.Enums; using Bit.Core.Context; using Bit.Core.Enums; -using Bit.Core.Identity; using Bit.Core.SecretsManager.AuthorizationRequirements; using Bit.Core.SecretsManager.Entities; +using Bit.Core.SecretsManager.Queries.Interfaces; using Bit.Core.SecretsManager.Repositories; -using Bit.Core.Services; using Bit.Core.Test.SecretsManager.AutoFixture.ProjectsFixture; using Bit.Test.Common.AutoFixture; using Bit.Test.Common.AutoFixture.Attributes; @@ -23,21 +22,22 @@ namespace Bit.Commercial.Core.Test.SecretsManager.AuthorizationHandlers.Projects public class ProjectAuthorizationHandlerTests { private static void SetupPermission(SutProvider sutProvider, - PermissionType permissionType, Guid organizationId) + PermissionType permissionType, Guid organizationId, Guid userId = new()) { sutProvider.GetDependency().AccessSecretsManager(organizationId) .Returns(true); - sutProvider.GetDependency().ClientType - .Returns(ClientType.User); - switch (permissionType) { case PermissionType.RunAsAdmin: - sutProvider.GetDependency().OrganizationAdmin(organizationId).Returns(true); + sutProvider.GetDependency().GetAccessClientAsync(default, organizationId) + .ReturnsForAnyArgs( + (AccessClientType.NoAccessCheck, userId)); break; case PermissionType.RunAsUserWithPermission: - sutProvider.GetDependency().OrganizationAdmin(organizationId).Returns(false); + sutProvider.GetDependency().GetAccessClientAsync(default, organizationId) + .ReturnsForAnyArgs( + (AccessClientType.User, userId)); break; default: throw new ArgumentOutOfRangeException(nameof(permissionType), permissionType, null); @@ -63,7 +63,6 @@ public class ProjectAuthorizationHandlerTests var authzContext = new AuthorizationHandlerContext(new List { requirement }, claimsPrincipal, project); - await Assert.ThrowsAsync(() => sutProvider.Sut.HandleAsync(authzContext)); } @@ -74,7 +73,6 @@ public class ProjectAuthorizationHandlerTests { sutProvider.GetDependency().AccessSecretsManager(project.OrganizationId) .Returns(true); - sutProvider.GetDependency().GetProperUserId(default).ReturnsForAnyArgs(new Guid()); var requirements = typeof(ProjectOperations).GetFields(BindingFlags.Public | BindingFlags.Static) .Select(i => (ProjectOperationRequirement)i.GetValue(null)); @@ -105,17 +103,16 @@ public class ProjectAuthorizationHandlerTests } [Theory] - [BitAutoData(ClientType.ServiceAccount)] - [BitAutoData(ClientType.Organization)] - public async Task CanCreateProject_NotSupportedClientTypes_DoesNotSucceed(ClientType clientType, + [BitAutoData(AccessClientType.ServiceAccount)] + [BitAutoData(AccessClientType.Organization)] + public async Task CanCreateProject_NotSupportedClientTypes_DoesNotSucceed(AccessClientType clientType, SutProvider sutProvider, Project project, ClaimsPrincipal claimsPrincipal) { sutProvider.GetDependency().AccessSecretsManager(project.OrganizationId) .Returns(true); - sutProvider.GetDependency().OrganizationAdmin(project.OrganizationId) - .Returns(false); - sutProvider.GetDependency().ClientType - .Returns(clientType); + sutProvider.GetDependency().GetAccessClientAsync(default, project.OrganizationId) + .ReturnsForAnyArgs( + (clientType, new Guid())); var requirement = ProjectOperations.Create; var authzContext = new AuthorizationHandlerContext(new List { requirement }, claimsPrincipal, project); @@ -167,7 +164,6 @@ public class ProjectAuthorizationHandlerTests sutProvider.GetDependency().AccessSecretsManager(project.OrganizationId) .Returns(true); SetupPermission(sutProvider, PermissionType.RunAsAdmin, project.OrganizationId); - sutProvider.GetDependency().GetProperUserId(default).ReturnsForAnyArgs(userId); sutProvider.GetDependency() .AccessToProjectAsync(project.Id, userId, Arg.Any()) .Returns((true, true)); @@ -188,8 +184,9 @@ public class ProjectAuthorizationHandlerTests sutProvider.GetDependency().AccessSecretsManager(project.OrganizationId) .Returns(true); sutProvider.GetDependency().OrganizationAdmin(project.OrganizationId).Returns(false); - sutProvider.GetDependency().ClientType - .Returns(ClientType.ServiceAccount); + sutProvider.GetDependency().GetAccessClientAsync(default, project.OrganizationId) + .ReturnsForAnyArgs( + (AccessClientType.ServiceAccount, new Guid())); var requirement = ProjectOperations.Update; var authzContext = new AuthorizationHandlerContext(new List { requirement }, claimsPrincipal, project); @@ -206,8 +203,7 @@ public class ProjectAuthorizationHandlerTests SutProvider sutProvider, Project project, ClaimsPrincipal claimsPrincipal, Guid userId) { - SetupPermission(sutProvider, permissionType, project.OrganizationId); - sutProvider.GetDependency().GetProperUserId(default).ReturnsForAnyArgs(userId); + SetupPermission(sutProvider, permissionType, project.OrganizationId, userId); sutProvider.GetDependency() .AccessToProjectAsync(project.Id, userId, Arg.Any()) .Returns((read, write)); @@ -229,8 +225,7 @@ public class ProjectAuthorizationHandlerTests SutProvider sutProvider, Project project, ClaimsPrincipal claimsPrincipal, Guid userId) { - SetupPermission(sutProvider, permissionType, project.OrganizationId); - sutProvider.GetDependency().GetProperUserId(default).ReturnsForAnyArgs(userId); + SetupPermission(sutProvider, permissionType, project.OrganizationId, userId); sutProvider.GetDependency() .AccessToProjectAsync(project.Id, userId, Arg.Any()) .Returns((read, write)); 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 new file mode 100644 index 000000000..5b9f139b7 --- /dev/null +++ b/bitwarden_license/test/Commercial.Core.Test/SecretsManager/AuthorizationHandlers/ServiceAccounts/ServiceAccountAuthorizationHandlerTests.cs @@ -0,0 +1,302 @@ +using System.Reflection; +using System.Security.Claims; +using Bit.Commercial.Core.SecretsManager.AuthorizationHandlers.ServiceAccounts; +using Bit.Commercial.Core.Test.SecretsManager.Enums; +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.Test.Common.AutoFixture; +using Bit.Test.Common.AutoFixture.Attributes; +using Microsoft.AspNetCore.Authorization; +using NSubstitute; +using Xunit; + +namespace Bit.Commercial.Core.Test.SecretsManager.AuthorizationHandlers.ServiceAccounts; + +[SutProviderCustomize] +public class ServiceAccountAuthorizationHandlerTests +{ + private static void SetupPermission(SutProvider sutProvider, + PermissionType permissionType, Guid organizationId, Guid userId = new()) + { + sutProvider.GetDependency().AccessSecretsManager(organizationId) + .Returns(true); + + switch (permissionType) + { + case PermissionType.RunAsAdmin: + sutProvider.GetDependency().GetAccessClientAsync(default, organizationId) + .ReturnsForAnyArgs( + (AccessClientType.NoAccessCheck, userId)); + break; + case PermissionType.RunAsUserWithPermission: + sutProvider.GetDependency().GetAccessClientAsync(default, organizationId) + .ReturnsForAnyArgs( + (AccessClientType.User, userId)); + break; + default: + throw new ArgumentOutOfRangeException(nameof(permissionType), permissionType, null); + } + } + + [Fact] + public void ServiceAccountOperations_OnlyPublicStatic() + { + var publicStaticFields = typeof(ServiceAccountOperations).GetFields(BindingFlags.Public | BindingFlags.Static); + var allFields = typeof(ServiceAccountOperations).GetFields(); + Assert.Equal(publicStaticFields.Length, allFields.Length); + } + + [Theory] + [BitAutoData] + public async Task Handler_UnsupportedServiceAccountOperationRequirement_Throws( + SutProvider sutProvider, ServiceAccount serviceAccount, + ClaimsPrincipal claimsPrincipal) + { + sutProvider.GetDependency().AccessSecretsManager(serviceAccount.OrganizationId) + .Returns(true); + var requirement = new ServiceAccountOperationRequirement(); + var authzContext = new AuthorizationHandlerContext(new List { requirement }, + claimsPrincipal, serviceAccount); + + await Assert.ThrowsAsync(() => sutProvider.Sut.HandleAsync(authzContext)); + } + + [Theory] + [BitAutoData] + public async Task Handler_SupportedServiceAccountOperationRequirement_DoesNotThrow( + SutProvider sutProvider, ServiceAccount serviceAccount, + ClaimsPrincipal claimsPrincipal) + { + sutProvider.GetDependency().AccessSecretsManager(serviceAccount.OrganizationId) + .Returns(true); + + var requirements = typeof(ServiceAccountOperations).GetFields(BindingFlags.Public | BindingFlags.Static) + .Select(i => (ServiceAccountOperationRequirement)i.GetValue(null)); + + foreach (var req in requirements) + { + var authzContext = new AuthorizationHandlerContext(new List { req }, + claimsPrincipal, serviceAccount); + + await sutProvider.Sut.HandleAsync(authzContext); + } + } + + [Theory] + [BitAutoData] + public async Task CanCreateServiceAccount_AccessToSecretsManagerFalse_DoesNotSucceed( + SutProvider sutProvider, ServiceAccount serviceAccount, + ClaimsPrincipal claimsPrincipal) + { + sutProvider.GetDependency().AccessSecretsManager(serviceAccount.OrganizationId) + .Returns(false); + var requirement = ServiceAccountOperations.Create; + var authzContext = new AuthorizationHandlerContext(new List { requirement }, + claimsPrincipal, serviceAccount); + + await sutProvider.Sut.HandleAsync(authzContext); + + Assert.False(authzContext.HasSucceeded); + } + + [Theory] + [BitAutoData(AccessClientType.ServiceAccount)] + [BitAutoData(AccessClientType.Organization)] + public async Task CanCreateServiceAccount_NotSupportedClientTypes_DoesNotSucceed(AccessClientType clientType, + SutProvider sutProvider, ServiceAccount serviceAccount, + ClaimsPrincipal claimsPrincipal) + { + var requirement = ServiceAccountOperations.Create; + sutProvider.GetDependency().AccessSecretsManager(serviceAccount.OrganizationId) + .Returns(true); + sutProvider.GetDependency().OrganizationAdmin(serviceAccount.OrganizationId) + .Returns(false); + sutProvider.GetDependency().GetAccessClientAsync(default, serviceAccount.OrganizationId) + .ReturnsForAnyArgs( + (clientType, new Guid())); + var authzContext = new AuthorizationHandlerContext(new List { requirement }, + claimsPrincipal, serviceAccount); + + await sutProvider.Sut.HandleAsync(authzContext); + + Assert.False(authzContext.HasSucceeded); + } + + [Theory] + [BitAutoData(PermissionType.RunAsAdmin)] + [BitAutoData(PermissionType.RunAsUserWithPermission)] + public async Task CanCreateServiceAccount_Success(PermissionType permissionType, + SutProvider sutProvider, ServiceAccount serviceAccount, + ClaimsPrincipal claimsPrincipal) + { + var requirement = ServiceAccountOperations.Create; + SetupPermission(sutProvider, permissionType, serviceAccount.OrganizationId); + var authzContext = new AuthorizationHandlerContext(new List { requirement }, + claimsPrincipal, serviceAccount); + + await sutProvider.Sut.HandleAsync(authzContext); + + Assert.True(authzContext.HasSucceeded); + } + + [Theory] + [BitAutoData] + public async Task CanUpdateServiceAccount_AccessToSecretsManagerFalse_DoesNotSucceed( + SutProvider sutProvider, ServiceAccount serviceAccount, + ClaimsPrincipal claimsPrincipal) + { + var requirement = ServiceAccountOperations.Update; + 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 CanUpdateServiceAccount_NullResource_DoesNotSucceed( + SutProvider sutProvider, ServiceAccount serviceAccount, + ClaimsPrincipal claimsPrincipal, + Guid userId) + { + var requirement = ServiceAccountOperations.Update; + 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.RunAsUserWithPermission, true, false)] + [BitAutoData(PermissionType.RunAsUserWithPermission, false, false)] + public async Task CanUpdateServiceAccount_ShouldNotSucceed(PermissionType permissionType, bool read, bool write, + SutProvider sutProvider, ServiceAccount serviceAccount, + ClaimsPrincipal claimsPrincipal, + Guid userId) + { + var requirement = ServiceAccountOperations.Update; + 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.False(authzContext.HasSucceeded); + } + + [Theory] + [BitAutoData(PermissionType.RunAsAdmin, true, true)] + [BitAutoData(PermissionType.RunAsAdmin, false, true)] + [BitAutoData(PermissionType.RunAsUserWithPermission, true, true)] + [BitAutoData(PermissionType.RunAsUserWithPermission, false, true)] + public async Task CanUpdateServiceAccount_Success(PermissionType permissionType, bool read, bool write, + SutProvider sutProvider, ServiceAccount serviceAccount, + ClaimsPrincipal claimsPrincipal, + Guid userId) + { + var requirement = ServiceAccountOperations.Update; + 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.True(authzContext.HasSucceeded); + } + + + [Theory] + [BitAutoData] + public async Task CanReadServiceAccount_AccessToSecretsManagerFalse_DoesNotSucceed( + SutProvider sutProvider, ServiceAccount serviceAccount, + ClaimsPrincipal claimsPrincipal) + { + var requirement = ServiceAccountOperations.Read; + 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 CanReadServiceAccount_NullResource_DoesNotSucceed( + SutProvider sutProvider, ServiceAccount serviceAccount, + ClaimsPrincipal claimsPrincipal, + Guid userId) + { + var requirement = ServiceAccountOperations.Read; + 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.RunAsUserWithPermission, false, false)] + [BitAutoData(PermissionType.RunAsUserWithPermission, false, true)] + public async Task CanReadServiceAccount_ShouldNotSucceed(PermissionType permissionType, bool read, bool write, + SutProvider sutProvider, ServiceAccount serviceAccount, + ClaimsPrincipal claimsPrincipal, + Guid userId) + { + var requirement = ServiceAccountOperations.Read; + 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.False(authzContext.HasSucceeded); + } + + [Theory] + [BitAutoData(PermissionType.RunAsAdmin, true, true)] + [BitAutoData(PermissionType.RunAsAdmin, true, false)] + [BitAutoData(PermissionType.RunAsUserWithPermission, true, true)] + [BitAutoData(PermissionType.RunAsUserWithPermission, true, false)] + public async Task CanReadServiceAccount_Success(PermissionType permissionType, bool read, bool write, + SutProvider sutProvider, ServiceAccount serviceAccount, + ClaimsPrincipal claimsPrincipal, + Guid userId) + { + var requirement = ServiceAccountOperations.Read; + 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.True(authzContext.HasSucceeded); + } +} diff --git a/bitwarden_license/test/Commercial.Core.Test/SecretsManager/Commands/ServiceAccounts/UpdateServiceAccountCommandTests.cs b/bitwarden_license/test/Commercial.Core.Test/SecretsManager/Commands/ServiceAccounts/UpdateServiceAccountCommandTests.cs index 488138a4e..7987c9e5d 100644 --- a/bitwarden_license/test/Commercial.Core.Test/SecretsManager/Commands/ServiceAccounts/UpdateServiceAccountCommandTests.cs +++ b/bitwarden_license/test/Commercial.Core.Test/SecretsManager/Commands/ServiceAccounts/UpdateServiceAccountCommandTests.cs @@ -1,5 +1,4 @@ using Bit.Commercial.Core.SecretsManager.Commands.ServiceAccounts; -using Bit.Core.Context; using Bit.Core.Exceptions; using Bit.Core.SecretsManager.Entities; using Bit.Core.SecretsManager.Repositories; @@ -16,49 +15,20 @@ public class UpdateServiceAccountCommandTests { [Theory] [BitAutoData] - public async Task UpdateAsync_ServiceAccountDoesNotExist_ThrowsNotFound(ServiceAccount data, Guid userId, SutProvider sutProvider) + public async Task UpdateAsync_ServiceAccountDoesNotExist_ThrowsNotFound(ServiceAccount data, SutProvider sutProvider) { - await Assert.ThrowsAsync(() => sutProvider.Sut.UpdateAsync(data, userId)); + await Assert.ThrowsAsync(() => sutProvider.Sut.UpdateAsync(data)); await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().ReplaceAsync(default); } [Theory] [BitAutoData] - public async Task UpdateAsync_User_NoAccess(ServiceAccount data, Guid userId, SutProvider sutProvider) + public async Task UpdateAsync_Success(ServiceAccount data, SutProvider sutProvider) { sutProvider.GetDependency().GetByIdAsync(data.Id).Returns(data); - sutProvider.GetDependency().UserHasWriteAccessToServiceAccount(data.Id, userId).Returns(false); - await Assert.ThrowsAsync(() => sutProvider.Sut.UpdateAsync(data, userId)); - - await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().ReplaceAsync(default); - } - - [Theory] - [BitAutoData] - public async Task UpdateAsync_User_Success(ServiceAccount data, Guid userId, SutProvider sutProvider) - { - sutProvider.GetDependency().AccessSecretsManager(data.OrganizationId).Returns(true); - sutProvider.GetDependency().GetByIdAsync(data.Id).Returns(data); - sutProvider.GetDependency().UserHasWriteAccessToServiceAccount(data.Id, userId).Returns(true); - - await sutProvider.Sut.UpdateAsync(data, userId); - - await sutProvider.GetDependency().Received(1) - .ReplaceAsync(Arg.Is(AssertHelper.AssertPropertyEqual(data))); - } - - - [Theory] - [BitAutoData] - public async Task UpdateAsync_Admin_Success(ServiceAccount data, Guid userId, SutProvider sutProvider) - { - sutProvider.GetDependency().GetByIdAsync(data.Id).Returns(data); - sutProvider.GetDependency().AccessSecretsManager(data.OrganizationId).Returns(true); - sutProvider.GetDependency().OrganizationAdmin(data.OrganizationId).Returns(true); - - await sutProvider.Sut.UpdateAsync(data, userId); + await sutProvider.Sut.UpdateAsync(data); await sutProvider.GetDependency().Received(1) .ReplaceAsync(Arg.Is(AssertHelper.AssertPropertyEqual(data))); @@ -66,11 +36,9 @@ public class UpdateServiceAccountCommandTests [Theory] [BitAutoData] - public async Task UpdateAsync_DoesNotModifyOrganizationId(ServiceAccount existingServiceAccount, Guid userId, SutProvider sutProvider) + public async Task UpdateAsync_DoesNotModifyOrganizationId(ServiceAccount existingServiceAccount, SutProvider sutProvider) { - sutProvider.GetDependency().AccessSecretsManager(existingServiceAccount.OrganizationId).Returns(true); sutProvider.GetDependency().GetByIdAsync(existingServiceAccount.Id).Returns(existingServiceAccount); - sutProvider.GetDependency().UserHasWriteAccessToServiceAccount(existingServiceAccount.Id, userId).Returns(true); var updatedOrgId = Guid.NewGuid(); var serviceAccountUpdate = new ServiceAccount() @@ -80,7 +48,7 @@ public class UpdateServiceAccountCommandTests Name = existingServiceAccount.Name, }; - var result = await sutProvider.Sut.UpdateAsync(serviceAccountUpdate, userId); + var result = await sutProvider.Sut.UpdateAsync(serviceAccountUpdate); Assert.Equal(existingServiceAccount.OrganizationId, result.OrganizationId); Assert.NotEqual(existingServiceAccount.OrganizationId, updatedOrgId); @@ -88,11 +56,9 @@ public class UpdateServiceAccountCommandTests [Theory] [BitAutoData] - public async Task UpdateAsync_DoesNotModifyCreationDate(ServiceAccount existingServiceAccount, Guid userId, SutProvider sutProvider) + public async Task UpdateAsync_DoesNotModifyCreationDate(ServiceAccount existingServiceAccount, SutProvider sutProvider) { - sutProvider.GetDependency().AccessSecretsManager(existingServiceAccount.OrganizationId).Returns(true); sutProvider.GetDependency().GetByIdAsync(existingServiceAccount.Id).Returns(existingServiceAccount); - sutProvider.GetDependency().UserHasWriteAccessToServiceAccount(existingServiceAccount.Id, userId).Returns(true); var updatedCreationDate = DateTime.UtcNow; var serviceAccountUpdate = new ServiceAccount() @@ -102,7 +68,7 @@ public class UpdateServiceAccountCommandTests Name = existingServiceAccount.Name, }; - var result = await sutProvider.Sut.UpdateAsync(serviceAccountUpdate, userId); + var result = await sutProvider.Sut.UpdateAsync(serviceAccountUpdate); Assert.Equal(existingServiceAccount.CreationDate, result.CreationDate); Assert.NotEqual(existingServiceAccount.CreationDate, updatedCreationDate); @@ -110,11 +76,9 @@ public class UpdateServiceAccountCommandTests [Theory] [BitAutoData] - public async Task UpdateAsync_RevisionDateIsUpdatedToUtcNow(ServiceAccount existingServiceAccount, Guid userId, SutProvider sutProvider) + public async Task UpdateAsync_RevisionDateIsUpdatedToUtcNow(ServiceAccount existingServiceAccount, SutProvider sutProvider) { - sutProvider.GetDependency().AccessSecretsManager(existingServiceAccount.OrganizationId).Returns(true); sutProvider.GetDependency().GetByIdAsync(existingServiceAccount.Id).Returns(existingServiceAccount); - sutProvider.GetDependency().UserHasWriteAccessToServiceAccount(existingServiceAccount.Id, userId).Returns(true); var updatedRevisionDate = DateTime.UtcNow.AddDays(10); var serviceAccountUpdate = new ServiceAccount() @@ -124,7 +88,7 @@ public class UpdateServiceAccountCommandTests Name = existingServiceAccount.Name, }; - var result = await sutProvider.Sut.UpdateAsync(serviceAccountUpdate, userId); + var result = await sutProvider.Sut.UpdateAsync(serviceAccountUpdate); Assert.NotEqual(serviceAccountUpdate.RevisionDate, result.RevisionDate); AssertHelper.AssertRecent(result.RevisionDate); diff --git a/src/Api/SecretsManager/Controllers/ServiceAccountsController.cs b/src/Api/SecretsManager/Controllers/ServiceAccountsController.cs index 620231052..561d07813 100644 --- a/src/Api/SecretsManager/Controllers/ServiceAccountsController.cs +++ b/src/Api/SecretsManager/Controllers/ServiceAccountsController.cs @@ -4,6 +4,7 @@ using Bit.Api.SecretsManager.Models.Response; using Bit.Core.Context; using Bit.Core.Enums; 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.Repositories; @@ -21,6 +22,7 @@ public class ServiceAccountsController : Controller { private readonly ICurrentContext _currentContext; private readonly IUserService _userService; + private readonly IAuthorizationService _authorizationService; private readonly IServiceAccountRepository _serviceAccountRepository; private readonly IApiKeyRepository _apiKeyRepository; private readonly ICreateAccessTokenCommand _createAccessTokenCommand; @@ -32,6 +34,7 @@ public class ServiceAccountsController : Controller public ServiceAccountsController( ICurrentContext currentContext, IUserService userService, + IAuthorizationService authorizationService, IServiceAccountRepository serviceAccountRepository, IApiKeyRepository apiKeyRepository, ICreateAccessTokenCommand createAccessTokenCommand, @@ -42,6 +45,7 @@ public class ServiceAccountsController : Controller { _currentContext = currentContext; _userService = userService; + _authorizationService = authorizationService; _serviceAccountRepository = serviceAccountRepository; _apiKeyRepository = apiKeyRepository; _createServiceAccountCommand = createServiceAccountCommand; @@ -73,32 +77,13 @@ public class ServiceAccountsController : Controller [HttpGet("{id}")] public async Task GetByServiceAccountIdAsync( - [FromRoute] Guid id) + [FromRoute] Guid id) { - var userId = _userService.GetProperUserId(User).Value; var serviceAccount = await _serviceAccountRepository.GetByIdAsync(id); + var authorizationResult = + await _authorizationService.AuthorizeAsync(User, serviceAccount, ServiceAccountOperations.Read); - if (serviceAccount == null) - { - throw new NotFoundException(); - } - - if (!_currentContext.AccessSecretsManager(serviceAccount.OrganizationId)) - { - throw new NotFoundException(); - } - - var orgAdmin = await _currentContext.OrganizationAdmin(serviceAccount.OrganizationId); - var accessClient = AccessClientHelper.ToAccessClient(_currentContext.ClientType, orgAdmin); - - var hasAccess = accessClient switch - { - AccessClientType.NoAccessCheck => true, - AccessClientType.User => await _serviceAccountRepository.UserHasWriteAccessToServiceAccount(id, userId), - _ => false, - }; - - if (!hasAccess) + if (!authorizationResult.Succeeded) { throw new NotFoundException(); } @@ -110,12 +95,18 @@ public class ServiceAccountsController : Controller public async Task CreateAsync([FromRoute] Guid organizationId, [FromBody] ServiceAccountCreateRequestModel createRequest) { - if (!_currentContext.AccessSecretsManager(organizationId)) + var serviceAccount = createRequest.ToServiceAccount(organizationId); + var authorizationResult = + await _authorizationService.AuthorizeAsync(User, serviceAccount, ServiceAccountOperations.Create); + + if (!authorizationResult.Succeeded) { throw new NotFoundException(); } + var userId = _userService.GetProperUserId(User).Value; - var result = await _createServiceAccountCommand.CreateAsync(createRequest.ToServiceAccount(organizationId), userId); + var result = + await _createServiceAccountCommand.CreateAsync(createRequest.ToServiceAccount(organizationId), userId); return new ServiceAccountResponseModel(result); } @@ -123,9 +114,16 @@ public class ServiceAccountsController : Controller public async Task UpdateAsync([FromRoute] Guid id, [FromBody] ServiceAccountUpdateRequestModel updateRequest) { - var userId = _userService.GetProperUserId(User).Value; + var serviceAccount = await _serviceAccountRepository.GetByIdAsync(id); + var authorizationResult = + await _authorizationService.AuthorizeAsync(User, serviceAccount, ServiceAccountOperations.Update); - var result = await _updateServiceAccountCommand.UpdateAsync(updateRequest.ToServiceAccount(id), userId); + if (!authorizationResult.Succeeded) + { + throw new NotFoundException(); + } + + var result = await _updateServiceAccountCommand.UpdateAsync(updateRequest.ToServiceAccount(id)); return new ServiceAccountResponseModel(result); } diff --git a/src/Core/SecretsManager/AuthorizationRequirements/ServiceAccountOperationRequirement.cs b/src/Core/SecretsManager/AuthorizationRequirements/ServiceAccountOperationRequirement.cs new file mode 100644 index 000000000..18d2f0701 --- /dev/null +++ b/src/Core/SecretsManager/AuthorizationRequirements/ServiceAccountOperationRequirement.cs @@ -0,0 +1,14 @@ +using Microsoft.AspNetCore.Authorization.Infrastructure; + +namespace Bit.Core.SecretsManager.AuthorizationRequirements; + +public class ServiceAccountOperationRequirement : OperationAuthorizationRequirement +{ +} + +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) }; +} diff --git a/src/Core/SecretsManager/Commands/ServiceAccounts/Interfaces/IUpdateServiceAccountCommand.cs b/src/Core/SecretsManager/Commands/ServiceAccounts/Interfaces/IUpdateServiceAccountCommand.cs index 2844f2903..ad3ee776c 100644 --- a/src/Core/SecretsManager/Commands/ServiceAccounts/Interfaces/IUpdateServiceAccountCommand.cs +++ b/src/Core/SecretsManager/Commands/ServiceAccounts/Interfaces/IUpdateServiceAccountCommand.cs @@ -4,5 +4,5 @@ namespace Bit.Core.SecretsManager.Commands.ServiceAccounts.Interfaces; public interface IUpdateServiceAccountCommand { - Task UpdateAsync(ServiceAccount serviceAccount, Guid userId); + Task UpdateAsync(ServiceAccount serviceAccount); } diff --git a/src/Core/SecretsManager/Queries/Interfaces/IAccessClientQuery.cs b/src/Core/SecretsManager/Queries/Interfaces/IAccessClientQuery.cs new file mode 100644 index 000000000..613e66a50 --- /dev/null +++ b/src/Core/SecretsManager/Queries/Interfaces/IAccessClientQuery.cs @@ -0,0 +1,9 @@ +using System.Security.Claims; +using Bit.Core.Enums; + +namespace Bit.Core.SecretsManager.Queries.Interfaces; + +public interface IAccessClientQuery +{ + Task<(AccessClientType AccessClientType, Guid UserId)> GetAccessClientAsync(ClaimsPrincipal claimsPrincipal, Guid organizationId); +} diff --git a/src/Core/SecretsManager/Repositories/IServiceAccountRepository.cs b/src/Core/SecretsManager/Repositories/IServiceAccountRepository.cs index 194df493f..d79ed020a 100644 --- a/src/Core/SecretsManager/Repositories/IServiceAccountRepository.cs +++ b/src/Core/SecretsManager/Repositories/IServiceAccountRepository.cs @@ -14,4 +14,5 @@ public interface IServiceAccountRepository Task UserHasReadAccessToServiceAccount(Guid id, Guid userId); Task UserHasWriteAccessToServiceAccount(Guid id, Guid userId); Task> GetManyByOrganizationIdWriteAccessAsync(Guid organizationId, Guid userId, AccessClientType accessType); + Task<(bool Read, bool Write)> AccessToServiceAccountAsync(Guid id, Guid userId, AccessClientType accessType); } diff --git a/test/Api.IntegrationTest/SecretsManager/Controllers/ServiceAccountsControllerTests.cs b/test/Api.IntegrationTest/SecretsManager/Controllers/ServiceAccountsControllerTests.cs index 2fca7ab61..910e2d373 100644 --- a/test/Api.IntegrationTest/SecretsManager/Controllers/ServiceAccountsControllerTests.cs +++ b/test/Api.IntegrationTest/SecretsManager/Controllers/ServiceAccountsControllerTests.cs @@ -139,11 +139,21 @@ public class ServiceAccountsControllerTests : IClassFixture { - new UserServiceAccountAccessPolicy - { - GrantedServiceAccountId = serviceAccount.Id, - OrganizationUserId = orgUser.Id, - Write = true, - Read = true, - }, - }); - } + var serviceAccount = await SetupServiceAccountWithAccessAsync(permissionType); var response = await _client.GetAsync($"/service-accounts/{serviceAccount.Id}"); response.EnsureSuccessStatusCode(); @@ -212,12 +199,25 @@ public class ServiceAccountsControllerTests : IClassFixture(); - Assert.NotNull(result); - Assert.Equal(request.Name, result!.Name); - Assert.NotEqual(initialServiceAccount.Name, result.Name); - AssertHelper.AssertRecent(result.RevisionDate); - Assert.NotEqual(initialServiceAccount.RevisionDate, result.RevisionDate); - - var updatedServiceAccount = await _serviceAccountRepository.GetByIdAsync(initialServiceAccount.Id); - Assert.NotNull(result); - Assert.Equal(request.Name, updatedServiceAccount.Name); - AssertHelper.AssertRecent(updatedServiceAccount.RevisionDate); - AssertHelper.AssertRecent(updatedServiceAccount.CreationDate); - Assert.NotEqual(initialServiceAccount.Name, updatedServiceAccount.Name); - Assert.NotEqual(initialServiceAccount.RevisionDate, updatedServiceAccount.RevisionDate); - } - - [Fact] - public async Task Update_User_WithPermission() - { - var (org, _) = await _organizationHelper.Initialize(true, true); - var (email, orgUser) = await _organizationHelper.CreateNewUser(OrganizationUserType.User, true); - await LoginAsync(email); - - var initialServiceAccount = await _serviceAccountRepository.CreateAsync(new ServiceAccount - { - OrganizationId = org.Id, - Name = _mockEncryptedString, - }); - - await CreateUserPolicyAsync(orgUser.Id, initialServiceAccount.Id, true, true); - - var request = new ServiceAccountUpdateRequestModel { Name = _mockNewName }; - - var response = await _client.PutAsJsonAsync($"/service-accounts/{initialServiceAccount.Id}", request); - response.EnsureSuccessStatusCode(); - var result = await response.Content.ReadFromJsonAsync(); - Assert.NotNull(result); - Assert.Equal(request.Name, result!.Name); - Assert.NotEqual(initialServiceAccount.Name, result.Name); - AssertHelper.AssertRecent(result.RevisionDate); - Assert.NotEqual(initialServiceAccount.RevisionDate, result.RevisionDate); - - var updatedServiceAccount = await _serviceAccountRepository.GetByIdAsync(initialServiceAccount.Id); - Assert.NotNull(result); - Assert.Equal(request.Name, updatedServiceAccount.Name); - AssertHelper.AssertRecent(updatedServiceAccount.RevisionDate); - AssertHelper.AssertRecent(updatedServiceAccount.CreationDate); - Assert.NotEqual(initialServiceAccount.Name, updatedServiceAccount.Name); - Assert.NotEqual(initialServiceAccount.RevisionDate, updatedServiceAccount.RevisionDate); - } - [Fact] public async Task Update_User_NoPermissions() { @@ -352,6 +287,45 @@ public class ServiceAccountsControllerTests : IClassFixture(); + Assert.NotNull(result); + Assert.Equal(request.Name, result!.Name); + Assert.NotEqual(initialServiceAccount.Name, result.Name); + AssertHelper.AssertRecent(result.RevisionDate); + Assert.NotEqual(initialServiceAccount.RevisionDate, result.RevisionDate); + + var updatedServiceAccount = await _serviceAccountRepository.GetByIdAsync(initialServiceAccount.Id); + Assert.NotNull(result); + Assert.Equal(request.Name, updatedServiceAccount.Name); + AssertHelper.AssertRecent(updatedServiceAccount.RevisionDate); + AssertHelper.AssertRecent(updatedServiceAccount.CreationDate); + Assert.NotEqual(initialServiceAccount.Name, updatedServiceAccount.Name); + Assert.NotEqual(initialServiceAccount.RevisionDate, updatedServiceAccount.RevisionDate); + } + [Theory] [InlineData(false, false)] [InlineData(true, false)] @@ -837,4 +811,35 @@ public class ServiceAccountsControllerTests : IClassFixture SetupServiceAccountWithAccessAsync(PermissionType permissionType) + { + var (org, _) = await _organizationHelper.Initialize(true, true); + await LoginAsync(_email); + + var initialServiceAccount = await _serviceAccountRepository.CreateAsync(new ServiceAccount + { + OrganizationId = org.Id, + Name = _mockEncryptedString, + }); + + if (permissionType == PermissionType.RunAsAdmin) + { + return initialServiceAccount; + } + + var (email, orgUser) = await _organizationHelper.CreateNewUser(OrganizationUserType.User, true); + await LoginAsync(email); + + var accessPolicies = new List + { + new UserServiceAccountAccessPolicy + { + GrantedServiceAccountId = initialServiceAccount.Id, OrganizationUserId = orgUser.Id, Read = true, Write = true, + }, + }; + await _accessPolicyRepository.CreateManyAsync(accessPolicies); + + return initialServiceAccount; + } } diff --git a/test/Api.Test/SecretsManager/Controllers/ServiceAccountsControllerTests.cs b/test/Api.Test/SecretsManager/Controllers/ServiceAccountsControllerTests.cs index c6b80b183..83e013973 100644 --- a/test/Api.Test/SecretsManager/Controllers/ServiceAccountsControllerTests.cs +++ b/test/Api.Test/SecretsManager/Controllers/ServiceAccountsControllerTests.cs @@ -1,4 +1,5 @@ -using Bit.Api.SecretsManager.Controllers; +using System.Security.Claims; +using Bit.Api.SecretsManager.Controllers; using Bit.Api.SecretsManager.Models.Request; using Bit.Core.Context; using Bit.Core.Enums; @@ -11,6 +12,7 @@ using Bit.Core.Services; using Bit.Test.Common.AutoFixture; using Bit.Test.Common.AutoFixture.Attributes; using Bit.Test.Common.Helpers; +using Microsoft.AspNetCore.Authorization; using NSubstitute; using NSubstitute.ReturnsExtensions; using Xunit; @@ -62,13 +64,30 @@ public class ServiceAccountsControllerTests sutProvider.Sut.ListByOrganizationAsync(orgId)); } + [Theory] + [BitAutoData] + public async void CreateServiceAccount_NoAccess_Throws(SutProvider sutProvider, ServiceAccountCreateRequestModel data, Guid organizationId) + { + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), data.ToServiceAccount(organizationId), + Arg.Any>()).ReturnsForAnyArgs(AuthorizationResult.Failed()); + sutProvider.GetDependency().GetProperUserId(default).ReturnsForAnyArgs(Guid.NewGuid()); + var resultServiceAccount = data.ToServiceAccount(organizationId); + sutProvider.GetDependency().CreateAsync(default, default).ReturnsForAnyArgs(resultServiceAccount); + + await Assert.ThrowsAsync(() => sutProvider.Sut.CreateAsync(organizationId, data)); + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .CreateAsync(Arg.Any(), Arg.Any()); + } + [Theory] [BitAutoData] public async void CreateServiceAccount_Success(SutProvider sutProvider, ServiceAccountCreateRequestModel data, Guid organizationId) { + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), data.ToServiceAccount(organizationId), + Arg.Any>()).ReturnsForAnyArgs(AuthorizationResult.Success()); sutProvider.GetDependency().GetProperUserId(default).ReturnsForAnyArgs(Guid.NewGuid()); - sutProvider.GetDependency().AccessSecretsManager(organizationId).Returns(true); - sutProvider.GetDependency().OrganizationUser(default).ReturnsForAnyArgs(true); var resultServiceAccount = data.ToServiceAccount(organizationId); sutProvider.GetDependency().CreateAsync(default, default).ReturnsForAnyArgs(resultServiceAccount); @@ -79,30 +98,33 @@ public class ServiceAccountsControllerTests [Theory] [BitAutoData] - public async void CreateServiceAccount_NotOrgUser_Throws(SutProvider sutProvider, ServiceAccountCreateRequestModel data, Guid organizationId) + public async void UpdateServiceAccount_NoAccess_Throws(SutProvider sutProvider, ServiceAccountUpdateRequestModel data, ServiceAccount existingServiceAccount) { - sutProvider.GetDependency().OrganizationUser(default).ReturnsForAnyArgs(false); - var resultServiceAccount = data.ToServiceAccount(organizationId); - sutProvider.GetDependency().CreateAsync(default, default).ReturnsForAnyArgs(resultServiceAccount); + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), data.ToServiceAccount(existingServiceAccount.Id), + Arg.Any>()).ReturnsForAnyArgs(AuthorizationResult.Failed()); + sutProvider.GetDependency().GetByIdAsync(existingServiceAccount.Id).ReturnsForAnyArgs(existingServiceAccount); + var resultServiceAccount = data.ToServiceAccount(existingServiceAccount.Id); + sutProvider.GetDependency().UpdateAsync(default).ReturnsForAnyArgs(resultServiceAccount); - await Assert.ThrowsAsync(() => sutProvider.Sut.CreateAsync(organizationId, data)); - - await sutProvider.GetDependency() - .DidNotReceiveWithAnyArgs() - .CreateAsync(Arg.Any(), Arg.Any()); + await Assert.ThrowsAsync(() => sutProvider.Sut.UpdateAsync(existingServiceAccount.Id, data)); + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .UpdateAsync(Arg.Any()); } [Theory] [BitAutoData] - public async void UpdateServiceAccount_Success(SutProvider sutProvider, ServiceAccountUpdateRequestModel data, Guid serviceAccountId) + public async void UpdateServiceAccount_Success(SutProvider sutProvider, ServiceAccountUpdateRequestModel data, ServiceAccount existingServiceAccount) { - sutProvider.GetDependency().GetProperUserId(default).ReturnsForAnyArgs(Guid.NewGuid()); - var resultServiceAccount = data.ToServiceAccount(serviceAccountId); - sutProvider.GetDependency().UpdateAsync(default, default).ReturnsForAnyArgs(resultServiceAccount); + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), data.ToServiceAccount(existingServiceAccount.Id), + Arg.Any>()).ReturnsForAnyArgs(AuthorizationResult.Success()); + var resultServiceAccount = data.ToServiceAccount(existingServiceAccount.Id); + sutProvider.GetDependency().UpdateAsync(default).ReturnsForAnyArgs(resultServiceAccount); - var result = await sutProvider.Sut.UpdateAsync(serviceAccountId, data); + var result = await sutProvider.Sut.UpdateAsync(existingServiceAccount.Id, data); await sutProvider.GetDependency().Received(1) - .UpdateAsync(Arg.Any(), Arg.Any()); + .UpdateAsync(Arg.Any()); } [Theory]