diff --git a/src/Api/Controllers/CollectionsController.cs b/src/Api/Controllers/CollectionsController.cs index 879ec077d2..b8d83da706 100644 --- a/src/Api/Controllers/CollectionsController.cs +++ b/src/Api/Controllers/CollectionsController.cs @@ -1,5 +1,6 @@ using Bit.Api.Models.Request; using Bit.Api.Models.Response; +using Bit.Api.Vault.AuthorizationHandlers.Collections; using Bit.Core.Context; using Bit.Core.Entities; using Bit.Core.Exceptions; @@ -20,19 +21,22 @@ public class CollectionsController : Controller private readonly IDeleteCollectionCommand _deleteCollectionCommand; private readonly IUserService _userService; private readonly ICurrentContext _currentContext; + private readonly IAuthorizationService _authorizationService; public CollectionsController( ICollectionRepository collectionRepository, ICollectionService collectionService, IDeleteCollectionCommand deleteCollectionCommand, IUserService userService, - ICurrentContext currentContext) + ICurrentContext currentContext, + IAuthorizationService authorizationService) { _collectionRepository = collectionRepository; _collectionService = collectionService; _deleteCollectionCommand = deleteCollectionCommand; _userService = userService; _currentContext = currentContext; + _authorizationService = authorizationService; } [HttpGet("{id}")] @@ -62,6 +66,7 @@ public class CollectionsController : Controller { throw new NotFoundException(); } + return new CollectionAccessDetailsResponseModel(collection, access.Groups, access.Users); } else @@ -72,6 +77,7 @@ public class CollectionsController : Controller { throw new NotFoundException(); } + return new CollectionAccessDetailsResponseModel(collection, access.Groups, access.Users); } } @@ -79,13 +85,15 @@ public class CollectionsController : Controller [HttpGet("details")] public async Task> GetManyWithDetails(Guid orgId) { - if (!await ViewAtLeastOneCollectionAsync(orgId) && !await _currentContext.ManageUsers(orgId) && !await _currentContext.ManageGroups(orgId)) + if (!await ViewAtLeastOneCollectionAsync(orgId) && !await _currentContext.ManageUsers(orgId) && + !await _currentContext.ManageGroups(orgId)) { throw new NotFoundException(); } // We always need to know which collections the current user is assigned to - var assignedOrgCollections = await _collectionRepository.GetManyByUserIdWithAccessAsync(_currentContext.UserId.Value, orgId); + var assignedOrgCollections = + await _collectionRepository.GetManyByUserIdWithAccessAsync(_currentContext.UserId.Value, orgId); if (await _currentContext.ViewAllCollections(orgId) || await _currentContext.ManageUsers(orgId)) { @@ -141,8 +149,8 @@ public class CollectionsController : Controller { var collection = model.ToCollection(orgId); - if (!await CanCreateCollection(orgId, collection.Id) && - !await CanEditCollectionAsync(orgId, collection.Id)) + var result = await _authorizationService.AuthorizeAsync(User, collection, CollectionOperations.Create); + if (!result.Succeeded) { throw new NotFoundException(); } @@ -150,10 +158,7 @@ public class CollectionsController : Controller var groups = model.Groups?.Select(g => g.ToSelectionReadOnly()); var users = model.Users?.Select(g => g.ToSelectionReadOnly()); - var assignUserToCollection = !(await _currentContext.EditAnyCollection(orgId)) && - await _currentContext.EditAssignedCollections(orgId); - - await _collectionService.SaveAsync(collection, groups, users, assignUserToCollection ? _currentContext.UserId : null); + await _collectionService.SaveAsync(collection, groups, users, _currentContext.UserId); return new CollectionResponseModel(collection); } @@ -189,12 +194,13 @@ public class CollectionsController : Controller [HttpPost("{id}/delete")] public async Task Delete(Guid orgId, Guid id) { - if (!await CanDeleteCollectionAsync(orgId, id)) + var collection = await GetCollectionAsync(id, orgId); + var result = await _authorizationService.AuthorizeAsync(User, collection, CollectionOperations.Delete); + if (!result.Succeeded) { throw new NotFoundException(); } - var collection = await GetCollectionAsync(id, orgId); await _deleteCollectionCommand.DeleteAsync(collection); } @@ -202,22 +208,14 @@ public class CollectionsController : Controller [HttpPost("delete")] public async Task DeleteMany([FromBody] CollectionBulkDeleteRequestModel model) { - var orgId = new Guid(model.OrganizationId); - var collectionIds = model.Ids.Select(i => new Guid(i)); - if (!await _currentContext.DeleteAssignedCollections(orgId) && !await _currentContext.DeleteAnyCollection(orgId)) + var collections = await _collectionRepository.GetManyByManyIdsAsync(model.Ids); + var result = await _authorizationService.AuthorizeAsync(User, collections, CollectionOperations.Delete); + if (!result.Succeeded) { throw new NotFoundException(); } - var userCollections = await _collectionService.GetOrganizationCollectionsAsync(orgId); - var filteredCollections = userCollections.Where(c => collectionIds.Contains(c.Id) && c.OrganizationId == orgId); - - if (!filteredCollections.Any()) - { - throw new BadRequestException("No collections found."); - } - - await _deleteCollectionCommand.DeleteManyAsync(filteredCollections); + await _deleteCollectionCommand.DeleteManyAsync(collections); } [HttpDelete("{id}/user/{orgUserId}")] @@ -248,17 +246,6 @@ public class CollectionsController : Controller return collection; } - - private async Task CanCreateCollection(Guid orgId, Guid collectionId) - { - if (collectionId != default) - { - return false; - } - - return await _currentContext.CreateNewCollections(orgId); - } - private async Task CanEditCollectionAsync(Guid orgId, Guid collectionId) { if (collectionId == default) @@ -273,28 +260,8 @@ public class CollectionsController : Controller if (await _currentContext.EditAssignedCollections(orgId)) { - var collectionDetails = await _collectionRepository.GetByIdAsync(collectionId, _currentContext.UserId.Value); - return collectionDetails != null; - } - - return false; - } - - private async Task CanDeleteCollectionAsync(Guid orgId, Guid collectionId) - { - if (collectionId == default) - { - return false; - } - - if (await _currentContext.DeleteAnyCollection(orgId)) - { - return true; - } - - if (await _currentContext.DeleteAssignedCollections(orgId)) - { - var collectionDetails = await _collectionRepository.GetByIdAsync(collectionId, _currentContext.UserId.Value); + var collectionDetails = + await _collectionRepository.GetByIdAsync(collectionId, _currentContext.UserId.Value); return collectionDetails != null; } @@ -315,7 +282,8 @@ public class CollectionsController : Controller if (await _currentContext.ViewAssignedCollections(orgId)) { - var collectionDetails = await _collectionRepository.GetByIdAsync(collectionId, _currentContext.UserId.Value); + var collectionDetails = + await _collectionRepository.GetByIdAsync(collectionId, _currentContext.UserId.Value); return collectionDetails != null; } diff --git a/src/Api/Models/Request/CollectionRequestModel.cs b/src/Api/Models/Request/CollectionRequestModel.cs index abd3d832b6..1ec1ee7960 100644 --- a/src/Api/Models/Request/CollectionRequestModel.cs +++ b/src/Api/Models/Request/CollectionRequestModel.cs @@ -34,7 +34,8 @@ public class CollectionRequestModel public class CollectionBulkDeleteRequestModel { [Required] - public IEnumerable Ids { get; set; } + public IEnumerable Ids { get; set; } + [Obsolete("OrganizationId is no longer required and will be removed in a future release")] public string OrganizationId { get; set; } } diff --git a/src/Api/Utilities/ServiceCollectionExtensions.cs b/src/Api/Utilities/ServiceCollectionExtensions.cs index ef77a32c6a..be6a7a066a 100644 --- a/src/Api/Utilities/ServiceCollectionExtensions.cs +++ b/src/Api/Utilities/ServiceCollectionExtensions.cs @@ -1,4 +1,4 @@ -using Bit.Api.Vault.AuthorizationHandlers; +using Bit.Api.Vault.AuthorizationHandlers.Collections; using Bit.Core.IdentityServer; using Bit.Core.Settings; using Bit.Core.Utilities; diff --git a/src/Api/Vault/AuthorizationHandlers/CollectionAuthorizationHandler.cs b/src/Api/Vault/AuthorizationHandlers/CollectionAuthorizationHandler.cs deleted file mode 100644 index 5434332a6d..0000000000 --- a/src/Api/Vault/AuthorizationHandlers/CollectionAuthorizationHandler.cs +++ /dev/null @@ -1,90 +0,0 @@ -using Bit.Core.Context; -using Bit.Core.Entities; -using Bit.Core.Enums; -using Bit.Core.Exceptions; -using Bit.Core.Repositories; -using Bit.Core.Utilities; -using Microsoft.AspNetCore.Authorization; - -namespace Bit.Api.Vault.AuthorizationHandlers; - -public class CollectionAuthorizationHandler : BulkAuthorizationHandler -{ - private readonly ICurrentContext _currentContext; - private readonly ICollectionRepository _collectionRepository; - private readonly IOrganizationUserRepository _organizationUserRepository; - - public CollectionAuthorizationHandler(ICurrentContext currentContext, ICollectionRepository collectionRepository, IOrganizationUserRepository organizationUserRepository) - { - _currentContext = currentContext; - _collectionRepository = collectionRepository; - _organizationUserRepository = organizationUserRepository; - } - - protected override async Task HandleRequirementAsync(AuthorizationHandlerContext context, CollectionOperationRequirement requirement, - ICollection resources) - { - switch (requirement) - { - case not null when requirement == CollectionOperation.ModifyAccess: - await CanManageCollectionAccessAsync(context, requirement, resources); - break; - } - } - - /// - /// Ensures the acting user is allowed to manage access permissions for the target collections. - /// - private async Task CanManageCollectionAccessAsync(AuthorizationHandlerContext context, IAuthorizationRequirement requirement, ICollection targetCollections) - { - if (!_currentContext.UserId.HasValue) - { - context.Fail(); - return; - } - - var targetOrganizationId = targetCollections.First().OrganizationId; - - // Ensure all target collections belong to the same organization - if (targetCollections.Any(tc => tc.OrganizationId != targetOrganizationId)) - { - throw new BadRequestException("Requested collections must belong to the same organization."); - } - - var org = (await _currentContext.OrganizationMembershipAsync(_organizationUserRepository, _currentContext.UserId.Value)) - .FirstOrDefault(o => targetOrganizationId == o.Id); - - // Acting user is not a member of the target organization, fail - if (org == null) - { - context.Fail(); - return; - } - - // Owners, Admins, Providers, and users with EditAnyCollection permission can always manage collection access - if ( - org.Permissions is { EditAnyCollection: true } || - org.Type is OrganizationUserType.Admin or OrganizationUserType.Owner || - await _currentContext.ProviderUserForOrgAsync(org.Id)) - { - context.Succeed(requirement); - return; - } - - // List of collection Ids the acting user is allowed to manage - var manageableCollectionIds = - (await _collectionRepository.GetManyByUserIdAsync(_currentContext.UserId.Value)) - .Where(c => c.Manage && c.OrganizationId == targetOrganizationId) - .Select(c => c.Id) - .ToHashSet(); - - // The acting user does not have permission to manage all target collections, fail - if (targetCollections.Any(tc => !manageableCollectionIds.Contains(tc.Id))) - { - context.Fail(); - return; - } - - context.Succeed(requirement); - } -} diff --git a/src/Api/Vault/AuthorizationHandlers/Collections/CollectionAuthorizationHandler.cs b/src/Api/Vault/AuthorizationHandlers/Collections/CollectionAuthorizationHandler.cs new file mode 100644 index 0000000000..702f35bb52 --- /dev/null +++ b/src/Api/Vault/AuthorizationHandlers/Collections/CollectionAuthorizationHandler.cs @@ -0,0 +1,162 @@ +using Bit.Core.Context; +using Bit.Core.Entities; +using Bit.Core.Enums; +using Bit.Core.Exceptions; +using Bit.Core.Repositories; +using Bit.Core.Utilities; +using Microsoft.AspNetCore.Authorization; + +namespace Bit.Api.Vault.AuthorizationHandlers.Collections; + +public class CollectionAuthorizationHandler : BulkAuthorizationHandler +{ + private readonly ICurrentContext _currentContext; + private readonly ICollectionRepository _collectionRepository; + + public CollectionAuthorizationHandler(ICurrentContext currentContext, ICollectionRepository collectionRepository) + { + _currentContext = currentContext; + _collectionRepository = collectionRepository; + } + + protected override async Task HandleRequirementAsync(AuthorizationHandlerContext context, + CollectionOperationRequirement requirement, ICollection resources) + { + // Establish pattern of authorization handler null checking passed resources + if (resources == null || !resources.Any()) + { + context.Fail(); + return; + } + + if (!_currentContext.UserId.HasValue) + { + context.Fail(); + return; + } + + var targetOrganizationId = resources.First().OrganizationId; + + // Ensure all target collections belong to the same organization + if (resources.Any(tc => tc.OrganizationId != targetOrganizationId)) + { + throw new BadRequestException("Requested collections must belong to the same organization."); + } + + // Acting user is not a member of the target organization, fail + var org = _currentContext.GetOrganization(targetOrganizationId); + if (org == null) + { + context.Fail(); + return; + } + + switch (requirement) + { + case not null when requirement == CollectionOperations.Create: + await CanCreateAsync(context, requirement, org); + break; + + case not null when requirement == CollectionOperations.Delete: + await CanDeleteAsync(context, requirement, resources, org); + break; + + case not null when requirement == CollectionOperations.ModifyAccess: + await CanManageCollectionAccessAsync(context, requirement, resources, org); + break; + } + } + + private async Task CanCreateAsync(AuthorizationHandlerContext context, CollectionOperationRequirement requirement, + CurrentContextOrganization org) + { + // If false, all organization members are allowed to create collections + if (!org.LimitCollectionCdOwnerAdmin) + { + context.Succeed(requirement); + return; + } + + // Owners, Admins, Providers, and users with CreateNewCollections or EditAnyCollection permission can always create collections + if ( + org.Type is OrganizationUserType.Owner or OrganizationUserType.Admin || + org.Permissions.CreateNewCollections || org.Permissions.EditAnyCollection || + await _currentContext.ProviderUserForOrgAsync(org.Id)) + { + context.Succeed(requirement); + return; + } + + context.Fail(); + } + + private async Task CanDeleteAsync(AuthorizationHandlerContext context, CollectionOperationRequirement requirement, + ICollection resources, CurrentContextOrganization org) + { + // Owners, Admins, Providers, and users with DeleteAnyCollection or EditAnyCollection permission can always delete collections + if ( + org.Type is OrganizationUserType.Owner or OrganizationUserType.Admin || + org.Permissions.DeleteAnyCollection || org.Permissions.EditAnyCollection || + await _currentContext.ProviderUserForOrgAsync(org.Id)) + { + context.Succeed(requirement); + return; + } + + // The limit collection management setting is enabled and we are not an Admin (above condition), fail + if (org.LimitCollectionCdOwnerAdmin) + { + context.Fail(); + return; + } + + // Other members types should have the Manage capability for all collections being deleted + var manageableCollectionIds = + (await _collectionRepository.GetManyByUserIdAsync(_currentContext.UserId!.Value)) + .Where(c => c.Manage && c.OrganizationId == org.Id) + .Select(c => c.Id) + .ToHashSet(); + + // The acting user does not have permission to manage all target collections, fail + if (resources.Any(c => !manageableCollectionIds.Contains(c.Id))) + { + context.Fail(); + return; + } + + context.Succeed(requirement); + } + + /// + /// Ensures the acting user is allowed to manage access permissions for the target collections. + /// + private async Task CanManageCollectionAccessAsync(AuthorizationHandlerContext context, + IAuthorizationRequirement requirement, ICollection targetCollections, CurrentContextOrganization org) + { + // Owners, Admins, Providers, and users with EditAnyCollection permission can always manage collection access + if ( + org.Permissions is { EditAnyCollection: true } || + org.Type is OrganizationUserType.Owner or OrganizationUserType.Admin || + await _currentContext.ProviderUserForOrgAsync(org.Id)) + { + context.Succeed(requirement); + return; + } + + // List of collection Ids the acting user is allowed to manage + var manageableCollectionIds = + (await _collectionRepository.GetManyByUserIdAsync(_currentContext.UserId!.Value)) + .Where(c => c.Manage && c.OrganizationId == org.Id) + .Select(c => c.Id) + .ToHashSet(); + + // The acting user does not have permission to manage all target collections, fail + if (targetCollections.Any(tc => !manageableCollectionIds.Contains(tc.Id))) + { + context.Fail(); + return; + } + + context.Succeed(requirement); + } +} diff --git a/src/Api/Vault/AuthorizationHandlers/CollectionOperation.cs b/src/Api/Vault/AuthorizationHandlers/Collections/CollectionOperations.cs similarity index 64% rename from src/Api/Vault/AuthorizationHandlers/CollectionOperation.cs rename to src/Api/Vault/AuthorizationHandlers/Collections/CollectionOperations.cs index f8b0008240..8fccce4336 100644 --- a/src/Api/Vault/AuthorizationHandlers/CollectionOperation.cs +++ b/src/Api/Vault/AuthorizationHandlers/Collections/CollectionOperations.cs @@ -1,11 +1,13 @@ using Microsoft.AspNetCore.Authorization.Infrastructure; -namespace Bit.Api.Vault.AuthorizationHandlers; +namespace Bit.Api.Vault.AuthorizationHandlers.Collections; public class CollectionOperationRequirement : OperationAuthorizationRequirement { } -public static class CollectionOperation +public static class CollectionOperations { + public static readonly CollectionOperationRequirement Create = new() { Name = nameof(Create) }; + public static readonly CollectionOperationRequirement Delete = new() { Name = nameof(Delete) }; /// /// The operation that represents creating, updating, or removing collection access. /// Combined together to allow for a single requirement to be used for each operation diff --git a/src/Core/Context/CurrentContext.cs b/src/Core/Context/CurrentContext.cs index 4751beb797..be0d3ddf74 100644 --- a/src/Core/Context/CurrentContext.cs +++ b/src/Core/Context/CurrentContext.cs @@ -321,27 +321,16 @@ public class CurrentContext : ICurrentContext && (o.Permissions?.AccessReports ?? false)) ?? false); } - public async Task CreateNewCollections(Guid orgId) - { - return await OrganizationManager(orgId) || (Organizations?.Any(o => o.Id == orgId - && (o.Permissions?.CreateNewCollections ?? false)) ?? false); - } - public async Task EditAnyCollection(Guid orgId) { return await OrganizationAdmin(orgId) || (Organizations?.Any(o => o.Id == orgId && (o.Permissions?.EditAnyCollection ?? false)) ?? false); } - public async Task DeleteAnyCollection(Guid orgId) - { - return await OrganizationAdmin(orgId) || (Organizations?.Any(o => o.Id == orgId - && (o.Permissions?.DeleteAnyCollection ?? false)) ?? false); - } - public async Task ViewAllCollections(Guid orgId) { - return await EditAnyCollection(orgId) || await DeleteAnyCollection(orgId); + var org = GetOrganization(orgId); + return await EditAnyCollection(orgId) || (org != null && org.Permissions.DeleteAnyCollection); } public async Task EditAssignedCollections(Guid orgId) @@ -358,9 +347,20 @@ public class CurrentContext : ICurrentContext public async Task ViewAssignedCollections(Guid orgId) { - return await CreateNewCollections(orgId) // Required to display the existing collections under which the new collection can be nested - || await EditAssignedCollections(orgId) - || await DeleteAssignedCollections(orgId); + /* + * Required to display the existing collections under which the new collection can be nested. + * Owner, Admin, Manager, and Provider checks are handled via the EditAssigned/DeleteAssigned context calls. + * This entire method will be moved to the CollectionAuthorizationHandler in the future + */ + var canCreateNewCollections = false; + var org = GetOrganization(orgId); + if (org != null) + { + canCreateNewCollections = !org.LimitCollectionCdOwnerAdmin || org.Permissions.CreateNewCollections; + } + return await EditAssignedCollections(orgId) + || await DeleteAssignedCollections(orgId) + || canCreateNewCollections; } public async Task ManageGroups(Guid orgId) @@ -509,6 +509,11 @@ public class CurrentContext : ICurrentContext return Providers; } + public CurrentContextOrganization GetOrganization(Guid orgId) + { + return Organizations?.Find(o => o.Id == orgId); + } + private string GetClaimValue(Dictionary> claims, string type) { if (!claims.ContainsKey(type)) diff --git a/src/Core/Context/CurrentContextOrganization.cs b/src/Core/Context/CurrentContextOrganization.cs index cf2c8b40c7..bd66a3c61b 100644 --- a/src/Core/Context/CurrentContextOrganization.cs +++ b/src/Core/Context/CurrentContextOrganization.cs @@ -15,10 +15,12 @@ public class CurrentContextOrganization Type = orgUser.Type; Permissions = CoreHelpers.LoadClassFromJsonData(orgUser.Permissions); AccessSecretsManager = orgUser.AccessSecretsManager && orgUser.UseSecretsManager; + LimitCollectionCdOwnerAdmin = orgUser.LimitCollectionCdOwnerAdmin; } public Guid Id { get; set; } public OrganizationUserType Type { get; set; } - public Permissions Permissions { get; set; } + public Permissions Permissions { get; set; } = new(); public bool AccessSecretsManager { get; set; } + public bool LimitCollectionCdOwnerAdmin { get; set; } } diff --git a/src/Core/Context/ICurrentContext.cs b/src/Core/Context/ICurrentContext.cs index c2e362d435..035a8263fe 100644 --- a/src/Core/Context/ICurrentContext.cs +++ b/src/Core/Context/ICurrentContext.cs @@ -1,4 +1,6 @@ -using System.Security.Claims; +#nullable enable + +using System.Security.Claims; using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Identity; @@ -39,9 +41,7 @@ public interface ICurrentContext Task AccessEventLogs(Guid orgId); Task AccessImportExport(Guid orgId); Task AccessReports(Guid orgId); - Task CreateNewCollections(Guid orgId); Task EditAnyCollection(Guid orgId); - Task DeleteAnyCollection(Guid orgId); Task ViewAllCollections(Guid orgId); Task EditAssignedCollections(Guid orgId); Task DeleteAssignedCollections(Guid orgId); @@ -72,4 +72,5 @@ public interface ICurrentContext Task ProviderIdForOrg(Guid orgId); bool AccessSecretsManager(Guid organizationId); + CurrentContextOrganization? GetOrganization(Guid orgId); } diff --git a/src/Core/Services/Implementations/CollectionService.cs b/src/Core/Services/Implementations/CollectionService.cs index 006c8c5cfc..6525fdc210 100644 --- a/src/Core/Services/Implementations/CollectionService.cs +++ b/src/Core/Services/Implementations/CollectionService.cs @@ -71,7 +71,7 @@ public class CollectionService : ICollectionService { await _collectionRepository.UpdateUsersAsync(collection.Id, new List { - new CollectionAccessSelection { Id = orgUser.Id, ReadOnly = false } }); + new CollectionAccessSelection { Id = orgUser.Id, Manage = true} }); } } diff --git a/src/Core/Services/Implementations/OrganizationService.cs b/src/Core/Services/Implementations/OrganizationService.cs index 10dc2a2056..2cbd017de4 100644 --- a/src/Core/Services/Implementations/OrganizationService.cs +++ b/src/Core/Services/Implementations/OrganizationService.cs @@ -2083,7 +2083,7 @@ public class OrganizationService : IOrganizationService private async Task ValidateCustomPermissionsGrant(Guid organizationId, Permissions permissions) { - if (permissions == null || await _currentContext.OrganizationOwner(organizationId) || await _currentContext.OrganizationAdmin(organizationId)) + if (permissions == null || await _currentContext.OrganizationAdmin(organizationId)) { return true; } @@ -2128,16 +2128,6 @@ public class OrganizationService : IOrganizationService return false; } - if (permissions.CreateNewCollections && !await _currentContext.CreateNewCollections(organizationId)) - { - return false; - } - - if (permissions.DeleteAnyCollection && !await _currentContext.DeleteAnyCollection(organizationId)) - { - return false; - } - if (permissions.DeleteAssignedCollections && !await _currentContext.DeleteAssignedCollections(organizationId)) { return false; @@ -2158,6 +2148,22 @@ public class OrganizationService : IOrganizationService return false; } + var org = _currentContext.GetOrganization(organizationId); + if (org == null) + { + return false; + } + + if (permissions.CreateNewCollections && !org.Permissions.CreateNewCollections) + { + return false; + } + + if (permissions.DeleteAnyCollection && !org.Permissions.DeleteAnyCollection) + { + return false; + } + return true; } diff --git a/test/Api.Test/Controllers/CollectionsControllerTests.cs b/test/Api.Test/Controllers/CollectionsControllerTests.cs index c035a8bc51..564ab4e15b 100644 --- a/test/Api.Test/Controllers/CollectionsControllerTests.cs +++ b/test/Api.Test/Controllers/CollectionsControllerTests.cs @@ -1,5 +1,7 @@ -using Bit.Api.Controllers; +using System.Security.Claims; +using Bit.Api.Controllers; using Bit.Api.Models.Request; +using Bit.Api.Vault.AuthorizationHandlers.Collections; using Bit.Core.Context; using Bit.Core.Entities; using Bit.Core.Exceptions; @@ -9,6 +11,7 @@ using Bit.Core.Repositories; using Bit.Core.Services; using Bit.Test.Common.AutoFixture; using Bit.Test.Common.AutoFixture.Attributes; +using Microsoft.AspNetCore.Authorization; using NSubstitute; using Xunit; @@ -19,27 +22,25 @@ namespace Bit.Api.Test.Controllers; public class CollectionsControllerTests { [Theory, BitAutoData] - public async Task Post_Success(Guid orgId, SutProvider sutProvider) + public async Task Post_Success(Guid orgId, CollectionRequestModel collectionRequest, + SutProvider sutProvider) { - sutProvider.GetDependency() - .CreateNewCollections(orgId) - .Returns(true); + Collection ExpectedCollection() => Arg.Is(c => + c.Name == collectionRequest.Name && c.ExternalId == collectionRequest.ExternalId && + c.OrganizationId == orgId); - sutProvider.GetDependency() - .EditAnyCollection(orgId) - .Returns(false); - - var collectionRequest = new CollectionRequestModel - { - Name = "encrypted_string", - ExternalId = "my_external_id" - }; + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), + ExpectedCollection(), + Arg.Is>(r => r.Contains(CollectionOperations.Create))) + .Returns(AuthorizationResult.Success()); _ = await sutProvider.Sut.Post(orgId, collectionRequest); await sutProvider.GetDependency() .Received(1) - .SaveAsync(Arg.Any(), Arg.Any>(), null); + .SaveAsync(Arg.Any(), Arg.Any>(), + Arg.Any>(), null); } [Theory, BitAutoData] @@ -139,13 +140,12 @@ public class CollectionsControllerTests [Theory, BitAutoData] - public async Task DeleteMany_Success(Guid orgId, User user, Collection collection1, Collection collection2, SutProvider sutProvider) + public async Task DeleteMany_Success(Guid orgId, Collection collection1, Collection collection2, SutProvider sutProvider) { // Arrange var model = new CollectionBulkDeleteRequestModel { - Ids = new[] { collection1.Id.ToString(), collection2.Id.ToString() }, - OrganizationId = orgId.ToString() + Ids = new[] { collection1.Id, collection2.Id } }; var collections = new List @@ -162,18 +162,15 @@ public class CollectionsControllerTests }, }; - sutProvider.GetDependency() - .DeleteAssignedCollections(orgId) - .Returns(true); - - sutProvider.GetDependency() - .UserId - .Returns(user.Id); - - sutProvider.GetDependency() - .GetOrganizationCollectionsAsync(orgId) + sutProvider.GetDependency().GetManyByManyIdsAsync(Arg.Any>()) .Returns(collections); + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), + collections, + Arg.Is>(r => r.Contains(CollectionOperations.Delete))) + .Returns(AuthorizationResult.Success()); + // Act await sutProvider.Sut.DeleteMany(model); @@ -185,18 +182,36 @@ public class CollectionsControllerTests } [Theory, BitAutoData] - public async Task DeleteMany_CanNotDeleteAssignedCollection_ThrowsNotFound(Guid orgId, Collection collection1, Collection collection2, SutProvider sutProvider) + public async Task DeleteMany_PermissionDenied_ThrowsNotFound(Guid orgId, Collection collection1, Collection collection2, SutProvider sutProvider) { // Arrange var model = new CollectionBulkDeleteRequestModel { - Ids = new[] { collection1.Id.ToString(), collection2.Id.ToString() }, - OrganizationId = orgId.ToString() + Ids = new[] { collection1.Id, collection2.Id } }; - sutProvider.GetDependency() - .DeleteAssignedCollections(orgId) - .Returns(false); + var collections = new List + { + new CollectionDetails + { + Id = collection1.Id, + OrganizationId = orgId, + }, + new CollectionDetails + { + Id = collection2.Id, + OrganizationId = orgId, + }, + }; + + sutProvider.GetDependency().GetManyByManyIdsAsync(Arg.Any>()) + .Returns(collections); + + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), + collections, + Arg.Is>(r => r.Contains(CollectionOperations.Delete))) + .Returns(AuthorizationResult.Failed()); // Assert await Assert.ThrowsAsync(() => @@ -205,49 +220,5 @@ public class CollectionsControllerTests await sutProvider.GetDependency() .DidNotReceiveWithAnyArgs() .DeleteManyAsync((IEnumerable)default); - } - - - [Theory, BitAutoData] - public async Task DeleteMany_UserCanNotAccessCollections_FiltersOutInvalid(Guid orgId, User user, Collection collection1, Collection collection2, SutProvider sutProvider) - { - // Arrange - var model = new CollectionBulkDeleteRequestModel - { - Ids = new[] { collection1.Id.ToString(), collection2.Id.ToString() }, - OrganizationId = orgId.ToString() - }; - - var collections = new List - { - new CollectionDetails - { - Id = collection2.Id, - OrganizationId = orgId, - }, - }; - - sutProvider.GetDependency() - .DeleteAssignedCollections(orgId) - .Returns(true); - - sutProvider.GetDependency() - .UserId - .Returns(user.Id); - - sutProvider.GetDependency() - .GetOrganizationCollectionsAsync(orgId) - .Returns(collections); - - // Act - await sutProvider.Sut.DeleteMany(model); - - // Assert - await sutProvider.GetDependency() - .Received(1) - .DeleteManyAsync(Arg.Is>(coll => coll.Select(c => c.Id).SequenceEqual(collections.Select(c => c.Id)))); - } - - } diff --git a/test/Api.Test/Vault/AuthorizationHandlers/CollectionAuthorizationHandlerTests.cs b/test/Api.Test/Vault/AuthorizationHandlers/CollectionAuthorizationHandlerTests.cs index cebf1468d4..16e542d539 100644 --- a/test/Api.Test/Vault/AuthorizationHandlers/CollectionAuthorizationHandlerTests.cs +++ b/test/Api.Test/Vault/AuthorizationHandlers/CollectionAuthorizationHandlerTests.cs @@ -1,5 +1,5 @@ using System.Security.Claims; -using Bit.Api.Vault.AuthorizationHandlers; +using Bit.Api.Vault.AuthorizationHandlers.Collections; using Bit.Core.Context; using Bit.Core.Entities; using Bit.Core.Enums; @@ -41,12 +41,12 @@ public class CollectionAuthorizationHandlerTests organization.Permissions.EditAnyCollection = editAnyCollection; var context = new AuthorizationHandlerContext( - new[] { CollectionOperation.ModifyAccess }, + new[] { CollectionOperations.ModifyAccess }, new ClaimsPrincipal(), collections); sutProvider.GetDependency().UserId.Returns(actingUserId); - sutProvider.GetDependency().OrganizationMembershipAsync(Arg.Any(), actingUserId).Returns(new[] { organization }); + sutProvider.GetDependency().GetOrganization(organization.Id).Returns(organization); sutProvider.GetDependency().GetManyByUserIdAsync(actingUserId).Returns(collectionDetails); await sutProvider.Sut.HandleAsync(context); @@ -54,14 +54,79 @@ public class CollectionAuthorizationHandlerTests Assert.True(context.HasSucceeded); } + [Theory, CollectionCustomization] + [BitAutoData(OrganizationUserType.User, false, false)] + [BitAutoData(OrganizationUserType.Admin, false, true)] + [BitAutoData(OrganizationUserType.Owner, false, true)] + [BitAutoData(OrganizationUserType.Custom, true, true)] + public async Task CanCreateAsync_Success( + OrganizationUserType userType, bool createNewCollection, bool limitCollectionCreateDelete, + SutProvider sutProvider, + ICollection collections, + CurrentContextOrganization organization) + { + var actingUserId = Guid.NewGuid(); + + organization.Type = userType; + organization.Permissions.CreateNewCollections = createNewCollection; + organization.LimitCollectionCdOwnerAdmin = limitCollectionCreateDelete; + + var context = new AuthorizationHandlerContext( + new[] { CollectionOperations.Create }, + new ClaimsPrincipal(), + collections); + + sutProvider.GetDependency().UserId.Returns(actingUserId); + sutProvider.GetDependency().GetOrganization(organization.Id).Returns(organization); + + await sutProvider.Sut.HandleAsync(context); + + Assert.True(context.HasSucceeded); + } + + [Theory, CollectionCustomization] + [BitAutoData(OrganizationUserType.User, false, false, true)] + [BitAutoData(OrganizationUserType.Admin, false, true, false)] + [BitAutoData(OrganizationUserType.Owner, false, true, false)] + [BitAutoData(OrganizationUserType.Custom, true, true, false)] + public async Task CanDeleteAsync_Success( + OrganizationUserType userType, bool deleteAnyCollection, bool limitCollectionCreateDelete, bool manageCollections, + SutProvider sutProvider, + ICollection collections, + ICollection collectionDetails, + CurrentContextOrganization organization) + { + var actingUserId = Guid.NewGuid(); + foreach (var collectionDetail in collectionDetails) + { + collectionDetail.Manage = manageCollections; + } + + organization.Type = userType; + organization.Permissions.DeleteAnyCollection = deleteAnyCollection; + organization.LimitCollectionCdOwnerAdmin = limitCollectionCreateDelete; + + var context = new AuthorizationHandlerContext( + new[] { CollectionOperations.Delete }, + new ClaimsPrincipal(), + collections); + + sutProvider.GetDependency().UserId.Returns(actingUserId); + sutProvider.GetDependency().GetOrganization(organization.Id).Returns(organization); + sutProvider.GetDependency().GetManyByUserIdAsync(actingUserId).Returns(collectionDetails); + + await sutProvider.Sut.HandleAsync(context); + + Assert.True(context.HasSucceeded); + } [Theory, BitAutoData, CollectionCustomization] - public async Task CanManageCollectionAccessAsync_MissingUserId_Failure( + public async Task HandleRequirementAsync_MissingUserId_Failure( SutProvider sutProvider, ICollection collections) { var context = new AuthorizationHandlerContext( - new[] { CollectionOperation.ModifyAccess }, + new[] { CollectionOperations.Create }, new ClaimsPrincipal(), collections ); @@ -75,7 +140,7 @@ public class CollectionAuthorizationHandlerTests } [Theory, BitAutoData, CollectionCustomization] - public async Task CanManageCollectionAccessAsync_TargetCollectionsMultipleOrgs_Failure( + public async Task HandleRequirementAsync_TargetCollectionsMultipleOrgs_Failure( SutProvider sutProvider, IList collections) { @@ -85,7 +150,7 @@ public class CollectionAuthorizationHandlerTests collections.First().OrganizationId = Guid.NewGuid(); var context = new AuthorizationHandlerContext( - new[] { CollectionOperation.ModifyAccess }, + new[] { CollectionOperations.Create }, new ClaimsPrincipal(), collections ); @@ -94,34 +159,27 @@ public class CollectionAuthorizationHandlerTests var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.HandleAsync(context)); Assert.Equal("Requested collections must belong to the same organization.", exception.Message); - await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() - .OrganizationMembershipAsync(default, default); + sutProvider.GetDependency().DidNotReceiveWithAnyArgs().GetOrganization(default); } [Theory, BitAutoData, CollectionCustomization] - public async Task CanManageCollectionAccessAsync_MissingOrgMembership_Failure( + public async Task HandleRequirementAsync_MissingOrg_Failure( SutProvider sutProvider, - ICollection collections, - CurrentContextOrganization organization) + ICollection collections) { var actingUserId = Guid.NewGuid(); var context = new AuthorizationHandlerContext( - new[] { CollectionOperation.ModifyAccess }, + new[] { CollectionOperations.Create }, new ClaimsPrincipal(), collections ); - // Simulate a missing org membership - organization.Id = Guid.NewGuid(); - sutProvider.GetDependency().UserId.Returns(actingUserId); - sutProvider.GetDependency().OrganizationMembershipAsync(Arg.Any(), actingUserId).Returns(new[] { organization }); + sutProvider.GetDependency().GetOrganization(Arg.Any()).Returns((CurrentContextOrganization)null); await sutProvider.Sut.HandleAsync(context); Assert.True(context.HasFailed); - await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() - .GetManyByUserIdAsync(default); } [Theory, BitAutoData, CollectionCustomization] @@ -145,18 +203,18 @@ public class CollectionAuthorizationHandlerTests organization.Permissions.EditAnyCollection = false; var context = new AuthorizationHandlerContext( - new[] { CollectionOperation.ModifyAccess }, + new[] { CollectionOperations.ModifyAccess }, new ClaimsPrincipal(), collections ); sutProvider.GetDependency().UserId.Returns(actingUserId); - sutProvider.GetDependency().OrganizationMembershipAsync(Arg.Any(), actingUserId).Returns(new[] { organization }); + sutProvider.GetDependency().GetOrganization(Arg.Any()).Returns(organization); sutProvider.GetDependency().GetManyByUserIdAsync(actingUserId).Returns(collectionDetails); await sutProvider.Sut.HandleAsync(context); Assert.True(context.HasFailed); - await sutProvider.GetDependency().ReceivedWithAnyArgs().OrganizationMembershipAsync(default, default); + sutProvider.GetDependency().ReceivedWithAnyArgs().GetOrganization(default); await sutProvider.GetDependency().ReceivedWithAnyArgs() .GetManyByUserIdAsync(default); } diff --git a/test/Core.Test/Services/OrganizationServiceTests.cs b/test/Core.Test/Services/OrganizationServiceTests.cs index 4907efafcb..8f1c2b5ee0 100644 --- a/test/Core.Test/Services/OrganizationServiceTests.cs +++ b/test/Core.Test/Services/OrganizationServiceTests.cs @@ -593,12 +593,19 @@ public class OrganizationServiceTests currentContext.ManageSso(organization.Id).Returns(true); currentContext.AccessEventLogs(organization.Id).Returns(true); currentContext.AccessImportExport(organization.Id).Returns(true); - currentContext.CreateNewCollections(organization.Id).Returns(true); - currentContext.DeleteAnyCollection(organization.Id).Returns(true); currentContext.DeleteAssignedCollections(organization.Id).Returns(true); currentContext.EditAnyCollection(organization.Id).Returns(true); currentContext.EditAssignedCollections(organization.Id).Returns(true); currentContext.ManageResetPassword(organization.Id).Returns(true); + currentContext.GetOrganization(organization.Id) + .Returns(new CurrentContextOrganization() + { + Permissions = new Permissions + { + CreateNewCollections = true, + DeleteAnyCollection = true + } + }); await sutProvider.Sut.InviteUsersAsync(organization.Id, invitor.UserId, invites); @@ -928,6 +935,14 @@ public class OrganizationServiceTests currentContext.OrganizationCustom(savingUser.OrganizationId).Returns(true); currentContext.ManageUsers(savingUser.OrganizationId).Returns(true); currentContext.AccessReports(savingUser.OrganizationId).Returns(true); + currentContext.GetOrganization(savingUser.OrganizationId).Returns( + new CurrentContextOrganization() + { + Permissions = new Permissions + { + AccessReports = true + } + }); await sutProvider.Sut.SaveUserAsync(newUserData, savingUser.UserId, collections, groups); }