diff --git a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs index c26d67d2ba..4691518da4 100644 --- a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs +++ b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs @@ -199,7 +199,7 @@ public class OrganizationUsersController : Controller { var collections = await _collectionRepository.GetManyByManyIdsAsync(model.Collections.Select(a => a.Id)); var authorized = - (await _authorizationService.AuthorizeAsync(User, collections, BulkCollectionOperations.ModifyAccess)) + (await _authorizationService.AuthorizeAsync(User, collections, BulkCollectionOperations.ModifyUserAccess)) .Succeeded; if (!authorized) { @@ -390,7 +390,7 @@ public class OrganizationUsersController : Controller var readonlyCollectionIds = new HashSet(); foreach (var collection in currentCollections) { - if (!(await _authorizationService.AuthorizeAsync(User, collection, BulkCollectionOperations.ModifyAccess)) + if (!(await _authorizationService.AuthorizeAsync(User, collection, BulkCollectionOperations.ModifyUserAccess)) .Succeeded) { readonlyCollectionIds.Add(collection.Id); diff --git a/src/Api/Controllers/CollectionsController.cs b/src/Api/Controllers/CollectionsController.cs index 7711e44220..1f320c1617 100644 --- a/src/Api/Controllers/CollectionsController.cs +++ b/src/Api/Controllers/CollectionsController.cs @@ -335,7 +335,8 @@ public class CollectionsController : Controller throw new NotFoundException("One or more collections not found."); } - var result = await _authorizationService.AuthorizeAsync(User, collections, BulkCollectionOperations.ModifyAccess); + var result = await _authorizationService.AuthorizeAsync(User, collections, + new[] { BulkCollectionOperations.ModifyUserAccess, BulkCollectionOperations.ModifyGroupAccess }); if (!result.Succeeded) { @@ -686,7 +687,7 @@ public class CollectionsController : Controller private async Task PutUsers_vNext(Guid id, IEnumerable model) { var collection = await _collectionRepository.GetByIdAsync(id); - var authorized = (await _authorizationService.AuthorizeAsync(User, collection, BulkCollectionOperations.ModifyAccess)).Succeeded; + var authorized = (await _authorizationService.AuthorizeAsync(User, collection, BulkCollectionOperations.ModifyUserAccess)).Succeeded; if (!authorized) { throw new NotFoundException(); @@ -710,7 +711,7 @@ public class CollectionsController : Controller private async Task DeleteUser_vNext(Guid id, Guid orgUserId) { var collection = await _collectionRepository.GetByIdAsync(id); - var authorized = (await _authorizationService.AuthorizeAsync(User, collection, BulkCollectionOperations.ModifyAccess)).Succeeded; + var authorized = (await _authorizationService.AuthorizeAsync(User, collection, BulkCollectionOperations.ModifyUserAccess)).Succeeded; if (!authorized) { throw new NotFoundException(); diff --git a/src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionAuthorizationHandler.cs b/src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionAuthorizationHandler.cs index fa05a71e85..c75e529b6d 100644 --- a/src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionAuthorizationHandler.cs +++ b/src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionAuthorizationHandler.cs @@ -64,61 +64,68 @@ public class BulkCollectionAuthorizationHandler : BulkAuthorizationHandler CanCreateAsync(CurrentContextOrganization? org) { // Owners, Admins, and users with CreateNewCollections permission can always create collections if (org is { Type: OrganizationUserType.Owner or OrganizationUserType.Admin } or { Permissions.CreateNewCollections: true }) { - context.Succeed(requirement); - return; + return true; } // If the limit collection management setting is disabled, allow any user to create collections if (await GetOrganizationAbilityAsync(org) is { LimitCollectionCreationDeletion: false }) { - context.Succeed(requirement); - return; + return true; } // Allow provider users to create collections if they are a provider for the target organization - if (await _currentContext.ProviderUserForOrgAsync(_targetOrganizationId)) - { - context.Succeed(requirement); - } + return await _currentContext.ProviderUserForOrgAsync(_targetOrganizationId); } - private async Task CanReadAsync(AuthorizationHandlerContext context, IAuthorizationRequirement requirement, - ICollection resources, CurrentContextOrganization? org) + private async Task CanReadAsync(ICollection resources, CurrentContextOrganization? org) { // Owners, Admins, and users with EditAnyCollection or DeleteAnyCollection permission can always read a collection if (org is @@ -126,8 +133,7 @@ public class BulkCollectionAuthorizationHandler : BulkAuthorizationHandler resources, CurrentContextOrganization? org) + private async Task CanReadWithAccessAsync(ICollection resources, CurrentContextOrganization? org) { // Owners, Admins, and users with EditAnyCollection, DeleteAnyCollection or ManageUsers permission can always read a collection if (org is @@ -159,8 +160,7 @@ public class BulkCollectionAuthorizationHandler : BulkAuthorizationHandler /// Ensures the acting user is allowed to update the target collections or manage access permissions for them. /// - private async Task CanUpdateCollectionAsync(AuthorizationHandlerContext context, - IAuthorizationRequirement requirement, ICollection resources, - CurrentContextOrganization? org) + private async Task CanUpdateCollectionAsync(ICollection resources, CurrentContextOrganization? org) { // Users with EditAnyCollection permission can always update a collection - if (org is - { Permissions.EditAnyCollection: true }) + if (org is { Permissions.EditAnyCollection: true }) { - context.Succeed(requirement); - return; + return true; } // If V1 is enabled, Owners and Admins can update any collection only if permitted by collection management settings @@ -203,8 +195,7 @@ public class BulkCollectionAuthorizationHandler : BulkAuthorizationHandler resources, CurrentContextOrganization? org) + private async Task CanUpdateUserAccessAsync(ICollection resources, CurrentContextOrganization? org) + { + return await CanUpdateCollectionAsync(resources, org) || org?.Permissions.ManageUsers == true; + } + + private async Task CanUpdateGroupAccessAsync(ICollection resources, CurrentContextOrganization? org) + { + return await CanUpdateCollectionAsync(resources, org) || org?.Permissions.ManageGroups == true; + } + + private async Task CanDeleteAsync(ICollection resources, CurrentContextOrganization? org) { // 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; + return true; } // Check for non-null org here: the user must be apart of the organization for this setting to take affect @@ -246,16 +241,12 @@ public class BulkCollectionAuthorizationHandler : BulkAuthorizationHandler CanManageCollectionsAsync(ICollection targetCollections) diff --git a/src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionOperations.cs b/src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionOperations.cs index c7a01da00a..e27b00f24a 100644 --- a/src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionOperations.cs +++ b/src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionOperations.cs @@ -6,17 +6,31 @@ public class BulkCollectionOperationRequirement : OperationAuthorizationRequirem public static class BulkCollectionOperations { + /// + /// Create a new collection + /// public static readonly BulkCollectionOperationRequirement Create = new() { Name = nameof(Create) }; public static readonly BulkCollectionOperationRequirement Read = new() { Name = nameof(Read) }; public static readonly BulkCollectionOperationRequirement ReadAccess = new() { Name = nameof(ReadAccess) }; public static readonly BulkCollectionOperationRequirement ReadWithAccess = new() { Name = nameof(ReadWithAccess) }; + /// + /// Update a collection, including user and group access + /// public static readonly BulkCollectionOperationRequirement Update = new() { Name = nameof(Update) }; /// - /// The operation that represents creating, updating, or removing collection access. - /// Combined together to allow for a single requirement to be used for each operation - /// as they all currently share the same underlying authorization logic. + /// Delete a collection /// - public static readonly BulkCollectionOperationRequirement ModifyAccess = new() { Name = nameof(ModifyAccess) }; public static readonly BulkCollectionOperationRequirement Delete = new() { Name = nameof(Delete) }; + /// + /// Import ciphers into a collection + /// public static readonly BulkCollectionOperationRequirement ImportCiphers = new() { Name = nameof(ImportCiphers) }; + /// + /// Create, update or delete user access (CollectionUser) + /// + public static readonly BulkCollectionOperationRequirement ModifyUserAccess = new() { Name = nameof(ModifyUserAccess) }; + /// + /// Create, update or delete group access (CollectionGroup) + /// + public static readonly BulkCollectionOperationRequirement ModifyGroupAccess = new() { Name = nameof(ModifyGroupAccess) }; } diff --git a/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs b/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs index ac94b5f514..e4831669d6 100644 --- a/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs @@ -120,7 +120,7 @@ public class OrganizationUsersControllerTests sutProvider.GetDependency() .AuthorizeAsync(Arg.Any(), Arg.Any>(), - Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyAccess))) + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) .Returns(AuthorizationResult.Success()); sutProvider.GetDependency().GetProperUserId(Arg.Any()).Returns(userId); @@ -147,7 +147,7 @@ public class OrganizationUsersControllerTests sutProvider.GetDependency() .AuthorizeAsync(Arg.Any(), Arg.Any>(), - Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyAccess))) + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) .Returns(AuthorizationResult.Failed()); sutProvider.GetDependency().GetProperUserId(Arg.Any()).Returns(userId); @@ -309,13 +309,13 @@ public class OrganizationUsersControllerTests // Authorize the editedCollection sutProvider.GetDependency() .AuthorizeAsync(Arg.Any(), Arg.Is(c => c.Id == editedCollectionId), - Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyAccess))) + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) .Returns(AuthorizationResult.Success()); // Do not authorize the readonly collections sutProvider.GetDependency() .AuthorizeAsync(Arg.Any(), Arg.Is(c => c.Id == readonlyCollectionId1 || c.Id == readonlyCollectionId2), - Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyAccess))) + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) .Returns(AuthorizationResult.Failed()); await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); @@ -357,7 +357,7 @@ public class OrganizationUsersControllerTests sutProvider.GetDependency() .AuthorizeAsync(Arg.Any(), Arg.Is(c => collections.Contains(c)), - Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyAccess))) + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) .Returns(AuthorizationResult.Failed()); var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model)); @@ -466,7 +466,7 @@ public class OrganizationUsersControllerTests sutProvider.GetDependency() .AuthorizeAsync(Arg.Any(), Arg.Is(c => collections.Contains(c)), - Arg.Is>(r => r.Contains(BulkCollectionOperations.ModifyAccess))) + Arg.Is>(r => r.Contains(BulkCollectionOperations.ModifyUserAccess))) .Returns(AuthorizationResult.Success()); } } diff --git a/test/Api.Test/Controllers/CollectionsControllerTests.cs b/test/Api.Test/Controllers/CollectionsControllerTests.cs index 6155ad0f77..5aba61a4db 100644 --- a/test/Api.Test/Controllers/CollectionsControllerTests.cs +++ b/test/Api.Test/Controllers/CollectionsControllerTests.cs @@ -345,7 +345,9 @@ public class CollectionsControllerTests sutProvider.GetDependency().AuthorizeAsync( Arg.Any(), ExpectedCollectionAccess(), Arg.Is>( - r => r.Contains(BulkCollectionOperations.ModifyAccess) + reqs => reqs.All(r => + r == BulkCollectionOperations.ModifyUserAccess || + r == BulkCollectionOperations.ModifyGroupAccess) )) .Returns(AuthorizationResult.Success()); @@ -359,8 +361,9 @@ public class CollectionsControllerTests Arg.Any(), ExpectedCollectionAccess(), Arg.Is>( - r => r.Contains(BulkCollectionOperations.ModifyAccess)) - ); + reqs => reqs.All(r => + r == BulkCollectionOperations.ModifyUserAccess || + r == BulkCollectionOperations.ModifyGroupAccess))); await sutProvider.GetDependency().Received() .AddAccessAsync( Arg.Is>(g => g.SequenceEqual(collections)), @@ -500,7 +503,9 @@ public class CollectionsControllerTests sutProvider.GetDependency().AuthorizeAsync( Arg.Any(), ExpectedCollectionAccess(), Arg.Is>( - r => r.Contains(BulkCollectionOperations.ModifyAccess) + reqs => reqs.All(r => + r == BulkCollectionOperations.ModifyUserAccess || + r == BulkCollectionOperations.ModifyGroupAccess) )) .Returns(AuthorizationResult.Failed()); @@ -511,7 +516,9 @@ public class CollectionsControllerTests Arg.Any(), ExpectedCollectionAccess(), Arg.Is>( - r => r.Contains(BulkCollectionOperations.ModifyAccess)) + reqs => reqs.All(r => + r == BulkCollectionOperations.ModifyUserAccess || + r == BulkCollectionOperations.ModifyGroupAccess)) ); await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() .AddAccessAsync(default, default, default); diff --git a/test/Api.Test/Vault/AuthorizationHandlers/BulkCollectionAuthorizationHandlerTests.cs b/test/Api.Test/Vault/AuthorizationHandlers/BulkCollectionAuthorizationHandlerTests.cs index 92063544ce..dd8bacaa9e 100644 --- a/test/Api.Test/Vault/AuthorizationHandlers/BulkCollectionAuthorizationHandlerTests.cs +++ b/test/Api.Test/Vault/AuthorizationHandlers/BulkCollectionAuthorizationHandlerTests.cs @@ -548,7 +548,9 @@ public class BulkCollectionAuthorizationHandlerTests var operationsToTest = new[] { - BulkCollectionOperations.Update, BulkCollectionOperations.ModifyAccess + BulkCollectionOperations.Update, + BulkCollectionOperations.ModifyUserAccess, + BulkCollectionOperations.ModifyGroupAccess, }; foreach (var op in operationsToTest) @@ -586,7 +588,9 @@ public class BulkCollectionAuthorizationHandlerTests var operationsToTest = new[] { - BulkCollectionOperations.Update, BulkCollectionOperations.ModifyAccess + BulkCollectionOperations.Update, + BulkCollectionOperations.ModifyUserAccess, + BulkCollectionOperations.ModifyGroupAccess, }; foreach (var op in operationsToTest) @@ -627,7 +631,9 @@ public class BulkCollectionAuthorizationHandlerTests var operationsToTest = new[] { - BulkCollectionOperations.Update, BulkCollectionOperations.ModifyAccess + BulkCollectionOperations.Update, + BulkCollectionOperations.ModifyUserAccess, + BulkCollectionOperations.ModifyGroupAccess, }; foreach (var op in operationsToTest) @@ -668,7 +674,9 @@ public class BulkCollectionAuthorizationHandlerTests var operationsToTest = new[] { - BulkCollectionOperations.Update, BulkCollectionOperations.ModifyAccess + BulkCollectionOperations.Update, + BulkCollectionOperations.ModifyUserAccess, + BulkCollectionOperations.ModifyGroupAccess, }; foreach (var op in operationsToTest) @@ -708,7 +716,9 @@ public class BulkCollectionAuthorizationHandlerTests var operationsToTest = new[] { - BulkCollectionOperations.Update, BulkCollectionOperations.ModifyAccess + BulkCollectionOperations.Update, + BulkCollectionOperations.ModifyUserAccess, + BulkCollectionOperations.ModifyGroupAccess, }; foreach (var op in operationsToTest) @@ -760,7 +770,9 @@ public class BulkCollectionAuthorizationHandlerTests var operationsToTest = new[] { - BulkCollectionOperations.Update, BulkCollectionOperations.ModifyAccess + BulkCollectionOperations.Update, + BulkCollectionOperations.ModifyUserAccess, + BulkCollectionOperations.ModifyGroupAccess, }; foreach (var op in operationsToTest) @@ -790,7 +802,9 @@ public class BulkCollectionAuthorizationHandlerTests { var operationsToTest = new[] { - BulkCollectionOperations.Update, BulkCollectionOperations.ModifyAccess + BulkCollectionOperations.Update, + BulkCollectionOperations.ModifyUserAccess, + BulkCollectionOperations.ModifyGroupAccess, }; foreach (var op in operationsToTest) @@ -813,6 +827,82 @@ public class BulkCollectionAuthorizationHandlerTests } } + [Theory, BitAutoData, CollectionCustomization] + public async Task CanUpdateUsers_WithManageUsersCustomPermission_Success( + SutProvider sutProvider, + ICollection collections, + CurrentContextOrganization organization) + { + var actingUserId = Guid.NewGuid(); + + organization.Type = OrganizationUserType.Custom; + organization.Permissions = new Permissions + { + ManageUsers = true + }; + + var operationsToTest = new[] + { + BulkCollectionOperations.ModifyUserAccess, + }; + + foreach (var op in operationsToTest) + { + sutProvider.GetDependency().UserId.Returns(actingUserId); + sutProvider.GetDependency().GetOrganization(organization.Id).Returns(organization); + + var context = new AuthorizationHandlerContext( + new[] { op }, + new ClaimsPrincipal(), + collections); + + await sutProvider.Sut.HandleAsync(context); + + Assert.True(context.HasSucceeded); + + // Recreate the SUT to reset the mocks/dependencies between tests + sutProvider.Recreate(); + } + } + + [Theory, BitAutoData, CollectionCustomization] + public async Task CanUpdateGroups_WithManageGroupsCustomPermission_Success( + SutProvider sutProvider, + ICollection collections, + CurrentContextOrganization organization) + { + var actingUserId = Guid.NewGuid(); + + organization.Type = OrganizationUserType.Custom; + organization.Permissions = new Permissions + { + ManageGroups = true + }; + + var operationsToTest = new[] + { + BulkCollectionOperations.ModifyGroupAccess, + }; + + foreach (var op in operationsToTest) + { + sutProvider.GetDependency().UserId.Returns(actingUserId); + sutProvider.GetDependency().GetOrganization(organization.Id).Returns(organization); + + var context = new AuthorizationHandlerContext( + new[] { op }, + new ClaimsPrincipal(), + collections); + + await sutProvider.Sut.HandleAsync(context); + + Assert.True(context.HasSucceeded); + + // Recreate the SUT to reset the mocks/dependencies between tests + sutProvider.Recreate(); + } + } + [Theory, CollectionCustomization] [BitAutoData(OrganizationUserType.Admin)] [BitAutoData(OrganizationUserType.Owner)] @@ -1023,7 +1113,8 @@ public class BulkCollectionAuthorizationHandlerTests BulkCollectionOperations.Read, BulkCollectionOperations.ReadAccess, BulkCollectionOperations.Update, - BulkCollectionOperations.ModifyAccess, + BulkCollectionOperations.ModifyUserAccess, + BulkCollectionOperations.ModifyGroupAccess, BulkCollectionOperations.Delete, };