From dba7a79ad248ed6ebb494cffd570b4ea3a6c1b3d Mon Sep 17 00:00:00 2001 From: Shane Melton Date: Tue, 21 Nov 2023 18:09:27 -0800 Subject: [PATCH] [AC-1696] Fix Provider permissions for Flexible Collections (#3381) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [AC-1117] Add manage permission (#3126) * Update sql files to add Manage permission * Add migration script * Rename collection manage migration file to remove duplicate migration date * Migrations * Add manage to models * Add manage to repository * Add constraint to Manage columns * Migration lint fixes * Add manage to OrganizationUserUserDetails_ReadWithCollectionsById * Add missing manage fields * Add 'Manage' to UserCollectionDetails * Use CREATE OR ALTER where possible * [AC-1374] Limit collection creation/deletion to Owner/Admin (#3145) * feat: update org table with new column, write migration, refs AC-1374 * feat: update views with new column, refs AC-1374 * feat: Alter sprocs (org create/update) to include new column, refs AC-1374 * feat: update entity/data/request/response models to handle new column, refs AC-1374 * feat: update necessary Provider related views during migration, refs AC-1374 * fix: update org create to default new column to false, refs AC-1374 * feat: added new API/request model for collection management and removed property from update request model, refs AC-1374 * fix: renamed migration script to be after secrets manage beta column changes, refs AC-1374 * fix: dotnet format, refs AC-1374 * feat: add ef migrations to reflect mssql changes, refs AC-1374 * fix: dotnet format, refs AC-1374 * feat: update API signature to accept Guid and explain Cd verbiage, refs AC-1374 * fix: merge conflict resolution * [AC-1174] CollectionUser and CollectionGroup authorization handlers (#3194) * [AC-1174] Introduce BulkAuthorizationHandler.cs * [AC-1174] Introduce CollectionUserAuthorizationHandler * [AC-1174] Add CreateForNewCollection CollectionUser requirement * [AC-1174] Add some more details to CollectionCustomization * [AC-1174] Formatting * [AC-1174] Add CollectionGroupOperation.cs * [AC-1174] Introduce CollectionGroupAuthorizationHandler.cs * [AC-1174] Cleanup CollectionFixture customization Implement and use re-usable extension method to support seeded Guids * [AC-1174] Introduce WithValueFromList AutoFixtureExtensions Modify CollectionCustomization to use multiple organization Ids for auto generated test data * [AC-1174] Simplify CollectionUserAuthorizationHandler.cs Modify the authorization handler to only perform authorization logic. Validation logic will need to be handled by any calling commands/controllers instead. * [AC-1174] Introduce shared CollectionAccessAuthorizationHandlerBase A shared base authorization handler was created for both CollectionUser and CollectionGroup resources, as they share the same underlying management authorization logic. * [AC-1174] Update CollectionUserAuthorizationHandler and CollectionGroupAuthorizationHandler to use the new CollectionAccessAuthorizationHandlerBase class * [AC-1174] Formatting * [AC-1174] Cleanup typo and redundant ToList() call * [AC-1174] Add check for provider users * [AC-1174] Reduce nested loops * [AC-1174] Introduce ICollectionAccess.cs * [AC-1174] Remove individual CollectionGroup and CollectionUser auth handlers and use base class instead * [AC-1174] Tweak unit test to fail minimally * [AC-1174] Reorganize authorization handlers in Core project * [AC-1174] Introduce new AddCoreAuthorizationHandlers() extension method * [AC-1174] Move CollectionAccessAuthorizationHandler into Api project * [AC-1174] Move CollectionFixture to Vault folder * [AC-1174] Rename operation to CreateUpdateDelete * [AC-1174] Require single organization for collection access authorization handler - Add requirement that all target collections must belong to the same organization - Simplify logic related to multiple organizations - Update tests and helpers - Use ToHashSet to improve lookup time * [AC-1174] Fix null reference exception * [AC-1174] Throw bad request exception when collections belong to different organizations * [AC-1174] Switch to CollectionAuthorizationHandler instead of CollectionAccessAuthorizationHandler to reduce complexity * Fix improper merge conflict resolution * fix: add permission check for collection management api, refs AC-1647 (#3252) * [AC-1125] Enforce org setting for creating/deleting collections (#3241) * [AC-1117] Add manage permission (#3126) * Update sql files to add Manage permission * Add migration script * Rename collection manage migration file to remove duplicate migration date * Migrations * Add manage to models * Add manage to repository * Add constraint to Manage columns * Migration lint fixes * Add manage to OrganizationUserUserDetails_ReadWithCollectionsById * Add missing manage fields * Add 'Manage' to UserCollectionDetails * Use CREATE OR ALTER where possible * [AC-1374] Limit collection creation/deletion to Owner/Admin (#3145) * feat: update org table with new column, write migration, refs AC-1374 * feat: update views with new column, refs AC-1374 * feat: Alter sprocs (org create/update) to include new column, refs AC-1374 * feat: update entity/data/request/response models to handle new column, refs AC-1374 * feat: update necessary Provider related views during migration, refs AC-1374 * fix: update org create to default new column to false, refs AC-1374 * feat: added new API/request model for collection management and removed property from update request model, refs AC-1374 * fix: renamed migration script to be after secrets manage beta column changes, refs AC-1374 * fix: dotnet format, refs AC-1374 * feat: add ef migrations to reflect mssql changes, refs AC-1374 * fix: dotnet format, refs AC-1374 * feat: update API signature to accept Guid and explain Cd verbiage, refs AC-1374 * feat: created collection auth handler/operations, added LimitCollectionCdOwnerAdmin to CurrentContentOrganization, refs AC-1125 * feat: create vault service collection extensions and register with base services, refs AC-1125 * feat: deprecated CurrentContext.CreateNewCollections, refs AC-1125 * feat: deprecate DeleteAnyCollection for single resource usages, refs AC-1125 * feat: move service registration to api, update references, refs AC-1125 * feat: add bulk delete authorization handler, refs AC-1125 * feat: always assign user and give manage access on create, refs AC-1125 * fix: updated CurrentContextOrganization type, refs AC-1125 * feat: combined existing collection authorization handlers/operations, refs AC-1125 * fix: OrganizationServiceTests -> CurrentContentOrganization typo, refs AC-1125 * fix: format, refs AC-1125 * fix: update collection controller tests, refs AC-1125 * fix: dotnet format, refs AC-1125 * feat: removed extra BulkAuthorizationHandler, refs AC-1125 * fix: dotnet format, refs AC-1125 * fix: change string to guid for org id, update bulk delete request model, refs AC-1125 * fix: remove delete many collection check, refs AC-1125 * fix: clean up collection auth handler, refs AC-1125 * fix: format fix for CollectionOperations, refs AC-1125 * fix: removed unnecessary owner check, add org null check to custom permission validation, refs AC-1125 * fix: remove unused methods in CurrentContext, refs AC-1125 * fix: removed obsolete test, fixed failling delete many test, refs AC-1125 * fix: CollectionAuthorizationHandlerTests fixes, refs AC-1125 * fix: OrganizationServiceTests fix broken test by mocking GetOrganization, refs AC-1125 * fix: CollectionAuthorizationHandler - remove unused repository, refs AC-1125 * feat: moved UserId null check to common method, refs AC-1125 * fix: updated auth handler tests to remove dependency on requirement for common code checks, refs AC-1125 * feat: updated conditionals/comments for create/delete methods within colleciton auth handler, refs AC-1125 * feat: added create/delete collection auth handler success methods, refs AC-1125 * fix: new up permissions to prevent excessive null checks, refs AC-1125 * fix: remove old reference to CreateNewCollections, refs AC-1125 * fix: typo within ViewAssignedCollections method, refs AC-1125 --------- Co-authored-by: Robyn MacCallum * refactor: remove organizationId from CollectionBulkDeleteRequestModel, refs AC-1649 (#3282) * [AC-1174] Bulk Collection Management (#3229) * [AC-1174] Update SelectionReadOnlyRequestModel to use Guid for Id property * [AC-1174] Introduce initial bulk-access collection endpoint * [AC-1174] Introduce BulkAddCollectionAccessCommand and validation logic/tests * [AC-1174] Add CreateOrUpdateAccessMany method to CollectionRepository * [AC-1174] Add event logs for bulk add collection access command * [AC-1174] Add User_BumpAccountRevisionDateByCollectionIds and database migration script * [AC-1174] Implement EF repository method * [AC-1174] Improve null checks * [AC-1174] Remove unnecessary BulkCollectionAccessRequestModel helpers * [AC-1174] Add unit tests for new controller endpoint * [AC-1174] Fix formatting * [AC-1174] Remove comment * [AC-1174] Remove redundant organizationId parameter * [AC-1174] Ensure user and group Ids are distinct * [AC-1174] Cleanup tests based on PR feedback * [AC-1174] Formatting * [AC-1174] Update CollectionGroup alias in the sproc * [AC-1174] Add some additional comments to SQL sproc * [AC-1174] Add comment explaining additional SaveChangesAsync call --------- Co-authored-by: Thomas Rittson * [AC-1646] Rename LimitCollectionCdOwnerAdmin column (#3300) * Rename LimitCollectionCdOwnerAdmin -> LimitCollectionCreationDeletion * Rename and bump migration script * [AC-1666] Removed EditAnyCollection from Create/Delete permission checks (#3301) * fix: remove EditAnyCollection from Create/Delete permission check, refs AC-1666 * fix: updated comment, refs AC-1666 * [AC-1669] Bug - Remove obsolete assignUserId from CollectionService.SaveAsync(...) (#3312) * fix: remove AssignUserId from CollectionService.SaveAsync, refs AC-1669 * fix: add manage access conditional before creating collection, refs AC-1669 * fix: move access logic for create/update, fix all tests, refs AC-1669 * fix: add CollectionAccessSelection fixture, update tests, update bad reqeuest message, refs AC-1669 * fix: format, refs AC-1669 * fix: update null params with specific arg.is null checks, refs Ac-1669 * fix: update attribute class name, refs AC-1669 * [AC-1713] [Flexible collections] Add feature flags to server (#3334) * Add feature flags for FlexibleCollections and BulkCollectionAccess * Flag new routes and behaviour --------- Co-authored-by: Rui Tomé <108268980+r-tome@users.noreply.github.com> * Add joint codeownership for auth handlers (#3346) * [AC-1717] Update default values for LimitCollectionCreationDeletion (#3365) * Change default value in organization create sproc to 1 * Drop old column name still present in some QA instances * Set LimitCollectionCreationDeletion value in code based on feature flag * Fix: add missing namespace after merging in master * Fix: add missing namespace after merging in master * [AC-1683] Fix DB migrations for new Manage permission (#3307) * [AC-1683] Update migration script and introduce V2 procedures and types * [AC-1683] Update repository calls to use new V2 procedures / types * [AC-1684] Update bulk add collection migration script to use new V2 type * [AC-1683] Undo Manage changes to more original procedures * [AC-1683] Restore whitespace changes * [AC-1683] Clarify comments regarding explicit column lists * [AC-1683] Update migration script dates * [AC-1683] Split the migration script for readability * [AC-1683] Re-name SelectReadOnlyArray_V2 to CollectionAccessSelectionType * [AC-1648] [Flexible Collections] Bump migration scripts before feature branch merge (#3371) * Bump dates on sql migration scripts * Bump date on ef migrations * [AC-1696] Rework collection auth handler to account for provider users - Add additional check for providers only if the org membership is null, and if the user is a provider, do not early return - Modify conditionals to use pattern matching to allow for null organization contexts from provider users - Save the target organization id as a private field to avoid additional parameter passing because org can now be null * [AC-1696] Add unit tests to collection auth handler for provider users Includes helper test method/enum for creating an xUnit theory for each collection operation. * [AC-1696] Further refactor private methods and remove provider check from public method Updates logic in private methods to only use context.Succeed() to allow for fallback permission checking for provider access. * [AC-1696] Ensure the correct organization id is provided when testing * [AC-1696] Refactor provider test to remove additional operation enum * [AC-1696] Formatting --------- Co-authored-by: Robyn MacCallum Co-authored-by: Vincent Salucci <26154748+vincentsalucci@users.noreply.github.com> Co-authored-by: Vincent Salucci Co-authored-by: Thomas Rittson Co-authored-by: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Co-authored-by: Rui Tomé <108268980+r-tome@users.noreply.github.com> --- .../CollectionAuthorizationHandler.cs | 127 +++++++++--------- .../CollectionAuthorizationHandlerTests.cs | 43 +++++- test/Common/AutoFixture/SutProvider.cs | 6 + 3 files changed, 108 insertions(+), 68 deletions(-) diff --git a/src/Api/Vault/AuthorizationHandlers/Collections/CollectionAuthorizationHandler.cs b/src/Api/Vault/AuthorizationHandlers/Collections/CollectionAuthorizationHandler.cs index 626402f71..9bbfc94ff 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 @@ public class CollectionAuthorizationHandler : BulkAuthorizationHandler 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 @@ public class CollectionAuthorizationHandler : BulkAuthorizationHandler 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 aaaa3076b..6dc63b69e 100644 --- a/test/Api.Test/Vault/AuthorizationHandlers/CollectionAuthorizationHandlerTests.cs +++ b/test/Api.Test/Vault/AuthorizationHandlers/CollectionAuthorizationHandlerTests.cs @@ -143,7 +143,7 @@ public class CollectionAuthorizationHandlerTests } [Theory, BitAutoData, CollectionCustomization] - public async Task HandleRequirementAsync_TargetCollectionsMultipleOrgs_Failure( + public async Task HandleRequirementAsync_TargetCollectionsMultipleOrgs_Exception( SutProvider sutProvider, IList collections) { @@ -166,7 +166,7 @@ public class CollectionAuthorizationHandlerTests } [Theory, BitAutoData, CollectionCustomization] - public async Task HandleRequirementAsync_MissingOrg_Failure( + public async Task HandleRequirementAsync_MissingOrg_NoSuccess( SutProvider sutProvider, ICollection collections) { @@ -182,11 +182,44 @@ public class CollectionAuthorizationHandlerTests 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 CanManageCollectionAccessAsync_MissingManageCollectionPermission_Failure( + 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_NoSuccess( SutProvider sutProvider, ICollection collections, ICollection collectionDetails, @@ -216,7 +249,7 @@ public class CollectionAuthorizationHandlerTests 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 3a3d6409b..ac953965b 100644 --- a/test/Common/AutoFixture/SutProvider.cs +++ b/test/Common/AutoFixture/SutProvider.cs @@ -72,6 +72,12 @@ public class SutProvider : ISutProvider Sut = default; } + public void Recreate() + { + _dependencies = new Dictionary>(); + Sut = _fixture.Create(); + } + ISutProvider ISutProvider.Create() => Create(); public SutProvider Create() {