diff --git a/src/Api/Vault/AuthorizationHandlers/Collections/CollectionAuthorizationHandler.cs b/src/Api/Vault/AuthorizationHandlers/Collections/CollectionAuthorizationHandler.cs index 626402f715b3..9bbfc94ff2f9 100644 --- a/src/Api/Vault/AuthorizationHandlers/Collections/CollectionAuthorizationHandler.cs +++ b/src/Api/Vault/AuthorizationHandlers/Collections/CollectionAuthorizationHandler.cs @@ -1,4 +1,5 @@ -using Bit.Core; +#nullable enable +using Bit.Core; using Bit.Core.Context; using Bit.Core.Entities; using Bit.Core.Enums; @@ -19,6 +20,7 @@ public class CollectionAuthorizationHandler : BulkAuthorizationHandler resources) + CollectionOperationRequirement requirement, ICollection? resources) { if (!_featureService.IsEnabled(FeatureFlagKeys.FlexibleCollections, _currentContext)) { @@ -50,21 +52,15 @@ protected override async Task HandleRequirementAsync(AuthorizationHandlerContext return; } - var targetOrganizationId = resources.First().OrganizationId; + _targetOrganizationId = resources.First().OrganizationId; // Ensure all target collections belong to the same organization - if (resources.Any(tc => tc.OrganizationId != targetOrganizationId)) + 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; - } + var org = _currentContext.GetOrganization(_targetOrganizationId); switch (requirement) { @@ -83,95 +79,100 @@ protected override async Task HandleRequirementAsync(AuthorizationHandlerContext } private async Task CanCreateAsync(AuthorizationHandlerContext context, CollectionOperationRequirement requirement, - CurrentContextOrganization org) + CurrentContextOrganization? org) { - // If false, all organization members are allowed to create collections - if (!org.LimitCollectionCreationDeletion) + // If the limit collection management setting is disabled, allow any user to create collections + // Otherwise, Owners, Admins, and users with CreateNewCollections permission can always create collections + if (org is + { LimitCollectionCreationDeletion: false } or + { Type: OrganizationUserType.Owner or OrganizationUserType.Admin } or + { Permissions.CreateNewCollections: true }) { context.Succeed(requirement); return; } - // Owners, Admins, Providers, and users with CreateNewCollections permission can always create collections - if ( - org.Type is OrganizationUserType.Owner or OrganizationUserType.Admin || - org.Permissions is { CreateNewCollections: true } || - await _currentContext.ProviderUserForOrgAsync(org.Id)) + // Allow provider users to create collections if they are a provider for the target organization + if (await _currentContext.ProviderUserForOrgAsync(_targetOrganizationId)) { context.Succeed(requirement); - return; } - - context.Fail(); } private async Task CanDeleteAsync(AuthorizationHandlerContext context, CollectionOperationRequirement requirement, - ICollection resources, CurrentContextOrganization org) + ICollection resources, CurrentContextOrganization? org) { - // Owners, Admins, Providers, and users with DeleteAnyCollection permission can always delete collections - if ( - org.Type is OrganizationUserType.Owner or OrganizationUserType.Admin || - org.Permissions is { DeleteAnyCollection: true } || - await _currentContext.ProviderUserForOrgAsync(org.Id)) + // Owners, Admins, and users with DeleteAnyCollection permission can always delete collections + if (org is + { Type: OrganizationUserType.Owner or OrganizationUserType.Admin } or + { Permissions.DeleteAnyCollection: true }) { context.Succeed(requirement); return; } - // The limit collection management setting is enabled and we are not an Admin (above condition), fail - if (org.LimitCollectionCreationDeletion) + // The limit collection management setting is disabled, + // ensure acting user has manage permissions for all collections being deleted + if (org is { LimitCollectionCreationDeletion: false }) { - context.Fail(); - return; + var manageableCollectionIds = + (await _collectionRepository.GetManyByUserIdAsync(_currentContext.UserId!.Value)) + .Where(c => c.Manage && c.OrganizationId == org.Id) + .Select(c => c.Id) + .ToHashSet(); + + // The acting user has permission to manage all target collections, succeed + if (resources.All(c => manageableCollectionIds.Contains(c.Id))) + { + context.Succeed(requirement); + 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))) + // Allow providers to delete collections if they are a provider for the target organization + if (await _currentContext.ProviderUserForOrgAsync(_targetOrganizationId)) { - context.Fail(); - return; + context.Succeed(requirement); } - - 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) + 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)) + // Owners, Admins, and users with EditAnyCollection permission can always manage collection access + if (org is + { Type: OrganizationUserType.Owner or OrganizationUserType.Admin } or + { Permissions.EditAnyCollection: true }) { 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))) + // Only check collection management permissions if the user is a member of the target organization (org != null) + if (org is not null) { - context.Fail(); - return; + var manageableCollectionIds = + (await _collectionRepository.GetManyByUserIdAsync(_currentContext.UserId!.Value)) + .Where(c => c.Manage && c.OrganizationId == org.Id) + .Select(c => c.Id) + .ToHashSet(); + + // The acting user has permission to manage all target collections, succeed + if (targetCollections.All(c => manageableCollectionIds.Contains(c.Id))) + { + context.Succeed(requirement); + return; + } } - context.Succeed(requirement); + // Allow providers to manage collections if they are a provider for the target organization + if (await _currentContext.ProviderUserForOrgAsync(_targetOrganizationId)) + { + context.Succeed(requirement); + } } } diff --git a/test/Api.Test/Vault/AuthorizationHandlers/CollectionAuthorizationHandlerTests.cs b/test/Api.Test/Vault/AuthorizationHandlers/CollectionAuthorizationHandlerTests.cs index aaaa3076b029..6dc63b69e54f 100644 --- a/test/Api.Test/Vault/AuthorizationHandlers/CollectionAuthorizationHandlerTests.cs +++ b/test/Api.Test/Vault/AuthorizationHandlers/CollectionAuthorizationHandlerTests.cs @@ -143,7 +143,7 @@ public async Task HandleRequirementAsync_MissingUserId_Failure( } [Theory, BitAutoData, CollectionCustomization] - public async Task HandleRequirementAsync_TargetCollectionsMultipleOrgs_Failure( + public async Task HandleRequirementAsync_TargetCollectionsMultipleOrgs_Exception( SutProvider sutProvider, IList collections) { @@ -166,7 +166,7 @@ public async Task HandleRequirementAsync_TargetCollectionsMultipleOrgs_Failure( } [Theory, BitAutoData, CollectionCustomization] - public async Task HandleRequirementAsync_MissingOrg_Failure( + public async Task HandleRequirementAsync_MissingOrg_NoSuccess( SutProvider sutProvider, ICollection collections) { @@ -182,11 +182,44 @@ public async Task HandleRequirementAsync_MissingOrg_Failure( sutProvider.GetDependency().GetOrganization(Arg.Any()).Returns((CurrentContextOrganization)null); await sutProvider.Sut.HandleAsync(context); - Assert.True(context.HasFailed); + Assert.False(context.HasSucceeded); + } + + [Theory, BitAutoData, CollectionCustomization] + public async Task HandleRequirementAsync_Provider_Success( + SutProvider sutProvider, + ICollection collections) + { + var operationsToTest = new[] + { + CollectionOperations.Create, CollectionOperations.Delete, CollectionOperations.ModifyAccess + }; + + foreach (var op in operationsToTest) + { + var actingUserId = Guid.NewGuid(); + var context = new AuthorizationHandlerContext( + new[] { op }, + new ClaimsPrincipal(), + collections + ); + var orgId = collections.First().OrganizationId; + + sutProvider.GetDependency().UserId.Returns(actingUserId); + sutProvider.GetDependency().GetOrganization(orgId).Returns((CurrentContextOrganization)null); + sutProvider.GetDependency().ProviderUserForOrgAsync(Arg.Any()).Returns(true); + + await sutProvider.Sut.HandleAsync(context); + Assert.True(context.HasSucceeded); + await sutProvider.GetDependency().Received().ProviderUserForOrgAsync(orgId); + + // Recreate the SUT to reset the mocks/dependencies between tests + sutProvider.Recreate(); + } } [Theory, BitAutoData, CollectionCustomization] - public async Task CanManageCollectionAccessAsync_MissingManageCollectionPermission_Failure( + public async Task CanManageCollectionAccessAsync_MissingManageCollectionPermission_NoSuccess( SutProvider sutProvider, ICollection collections, ICollection collectionDetails, @@ -216,7 +249,7 @@ public async Task CanManageCollectionAccessAsync_MissingManageCollectionPermissi sutProvider.GetDependency().GetManyByUserIdAsync(actingUserId).Returns(collectionDetails); await sutProvider.Sut.HandleAsync(context); - Assert.True(context.HasFailed); + Assert.False(context.HasSucceeded); sutProvider.GetDependency().ReceivedWithAnyArgs().GetOrganization(default); await sutProvider.GetDependency().ReceivedWithAnyArgs() .GetManyByUserIdAsync(default); diff --git a/test/Common/AutoFixture/SutProvider.cs b/test/Common/AutoFixture/SutProvider.cs index 3a3d6409ba49..ac953965bdfc 100644 --- a/test/Common/AutoFixture/SutProvider.cs +++ b/test/Common/AutoFixture/SutProvider.cs @@ -72,6 +72,12 @@ public void Reset() Sut = default; } + public void Recreate() + { + _dependencies = new Dictionary>(); + Sut = _fixture.Create(); + } + ISutProvider ISutProvider.Create() => Create(); public SutProvider Create() {