From ba36b2d26a2e3e92f03af418d9ef810990efef5f Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Mon, 29 Apr 2024 11:02:06 +1000 Subject: [PATCH] [AC-2172] Member modal - limit admin access (#3934) * update OrganizationUsersController PUT and POST * enforces new collection access checks when updating members * refactor BulkCollectionAuthorizationHandler to avoid repeated db calls --- .../OrganizationUsersController.cs | 89 ++++++- .../BulkCollectionAuthorizationHandler.cs | 54 ++-- ...ganizationUser_ReadWithCollectionsById.sql | 5 +- .../OrganizationUsersControllerTests.cs | 232 ++++++++++++++++-- ...BulkCollectionAuthorizationHandlerTests.cs | 118 ++++++++- ...rganizationUserReadWithCollectionsById.sql | 21 ++ 6 files changed, 463 insertions(+), 56 deletions(-) create mode 100644 util/Migrator/DbScripts/2024-04-05_00_OrganizationUserReadWithCollectionsById.sql diff --git a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs index 36d266fdf4..c26d67d2ba 100644 --- a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs +++ b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs @@ -3,6 +3,7 @@ using Bit.Api.AdminConsole.Models.Response.Organizations; using Bit.Api.Models.Request.Organizations; using Bit.Api.Models.Response; using Bit.Api.Utilities; +using Bit.Api.Vault.AuthorizationHandlers.Collections; using Bit.Api.Vault.AuthorizationHandlers.OrganizationUsers; using Bit.Core; using Bit.Core.AdminConsole.Enums; @@ -186,16 +187,28 @@ public class OrganizationUsersController : Controller } [HttpPost("invite")] - public async Task Invite(string orgId, [FromBody] OrganizationUserInviteRequestModel model) + public async Task Invite(Guid orgId, [FromBody] OrganizationUserInviteRequestModel model) { - var orgGuidId = new Guid(orgId); - if (!await _currentContext.ManageUsers(orgGuidId)) + if (!await _currentContext.ManageUsers(orgId)) { throw new NotFoundException(); } + // Flexible Collections - check the user has permission to grant access to the collections for the new user + if (await FlexibleCollectionsIsEnabledAsync(orgId) && _featureService.IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1)) + { + var collections = await _collectionRepository.GetManyByManyIdsAsync(model.Collections.Select(a => a.Id)); + var authorized = + (await _authorizationService.AuthorizeAsync(User, collections, BulkCollectionOperations.ModifyAccess)) + .Succeeded; + if (!authorized) + { + throw new NotFoundException("You are not authorized to grant access to these collections."); + } + } + var userId = _userService.GetProperUserId(User); - var result = await _organizationService.InviteUsersAsync(orgGuidId, userId.Value, + await _organizationService.InviteUsersAsync(orgId, userId.Value, new (OrganizationUserInvite, string)[] { (new OrganizationUserInvite(model.ToData()), null) }); } @@ -316,6 +329,35 @@ public class OrganizationUsersController : Controller [HttpPut("{id}")] [HttpPost("{id}")] public async Task Put(Guid orgId, Guid id, [FromBody] OrganizationUserUpdateRequestModel model) + { + if (await FlexibleCollectionsIsEnabledAsync(orgId) && _featureService.IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1)) + { + // Use new Flexible Collections v1 logic + await Put_vNext(orgId, id, model); + return; + } + + // Pre-Flexible Collections v1 code follows + if (!await _currentContext.ManageUsers(orgId)) + { + throw new NotFoundException(); + } + + var organizationUser = await _organizationUserRepository.GetByIdAsync(id); + if (organizationUser == null || organizationUser.OrganizationId != orgId) + { + throw new NotFoundException(); + } + + var userId = _userService.GetProperUserId(User); + await _updateOrganizationUserCommand.UpdateUserAsync(model.ToOrganizationUser(organizationUser), userId.Value, + model.Collections.Select(c => c.ToSelectionReadOnly()).ToList(), model.Groups); + } + + /// + /// Put logic for Flexible Collections v1 + /// + private async Task Put_vNext(Guid orgId, Guid id, [FromBody] OrganizationUserUpdateRequestModel model) { if (!await _currentContext.ManageUsers(orgId)) { @@ -332,17 +374,44 @@ public class OrganizationUsersController : Controller // In this case we just don't update groups var userId = _userService.GetProperUserId(User).Value; var organizationAbility = await _applicationCacheService.GetOrganizationAbilityAsync(orgId); - var restrictEditingGroups = _featureService.IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1) && - organizationAbility.FlexibleCollections && - userId == organizationUser.UserId && - !organizationAbility.AllowAdminAccessToAllCollectionItems; + var editingSelf = userId == organizationUser.UserId; - var groups = restrictEditingGroups + var groups = editingSelf && !organizationAbility.AllowAdminAccessToAllCollectionItems ? null : model.Groups; + // The client only sends collections that the saving user has permissions to edit. + // On the server side, we need to (1) confirm this and (2) concat these with the collections that the user + // can't edit before saving to the database. + var (_, currentAccess) = await _organizationUserRepository.GetByIdWithCollectionsAsync(id); + var currentCollections = await _collectionRepository + .GetManyByManyIdsAsync(currentAccess.Select(cas => cas.Id)); + + var readonlyCollectionIds = new HashSet(); + foreach (var collection in currentCollections) + { + if (!(await _authorizationService.AuthorizeAsync(User, collection, BulkCollectionOperations.ModifyAccess)) + .Succeeded) + { + readonlyCollectionIds.Add(collection.Id); + } + } + + if (model.Collections.Any(c => readonlyCollectionIds.Contains(c.Id))) + { + throw new BadRequestException("You must have Can Manage permissions to edit a collection's membership"); + } + + var editedCollectionAccess = model.Collections + .Select(c => c.ToSelectionReadOnly()); + var readonlyCollectionAccess = currentAccess + .Where(ca => readonlyCollectionIds.Contains(ca.Id)); + var collectionsToSave = editedCollectionAccess + .Concat(readonlyCollectionAccess) + .ToList(); + await _updateOrganizationUserCommand.UpdateUserAsync(model.ToOrganizationUser(organizationUser), userId, - model.Collections?.Select(c => c.ToSelectionReadOnly()).ToList(), groups); + collectionsToSave, groups); } [HttpPut("{userId}/reset-password-enrollment")] diff --git a/src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionAuthorizationHandler.cs b/src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionAuthorizationHandler.cs index afdd7c012a..fa05a71e85 100644 --- a/src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionAuthorizationHandler.cs +++ b/src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionAuthorizationHandler.cs @@ -1,4 +1,5 @@ #nullable enable +using Bit.Core; using Bit.Core.Context; using Bit.Core.Entities; using Bit.Core.Enums; @@ -20,16 +21,20 @@ public class BulkCollectionAuthorizationHandler : BulkAuthorizationHandler? _managedCollectionsIds; public BulkCollectionAuthorizationHandler( ICurrentContext currentContext, ICollectionRepository collectionRepository, - IApplicationCacheService applicationCacheService) + IApplicationCacheService applicationCacheService, + IFeatureService featureService) { _currentContext = currentContext; _collectionRepository = collectionRepository; _applicationCacheService = applicationCacheService; + _featureService = featureService; } protected override async Task HandleRequirementAsync(AuthorizationHandlerContext context, @@ -129,7 +134,7 @@ public class BulkCollectionAuthorizationHandler : BulkAuthorizationHandler resources, CurrentContextOrganization? org) { - // Owners, Admins, and users with EditAnyCollection permission can always manage collection access + // Users with EditAnyCollection permission can always update a collection if (org is - { Type: OrganizationUserType.Owner or OrganizationUserType.Admin } or - { Permissions.EditAnyCollection: true }) + { Permissions.EditAnyCollection: true }) + { + context.Succeed(requirement); + return; + } + + // If V1 is enabled, Owners and Admins can update any collection only if permitted by collection management settings + var organizationAbility = await GetOrganizationAbilityAsync(org); + var allowAdminAccessToAllCollectionItems = !_featureService.IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1) || + organizationAbility is { AllowAdminAccessToAllCollectionItems: true }; + if (allowAdminAccessToAllCollectionItems && org is { Type: OrganizationUserType.Owner or OrganizationUserType.Admin }) { context.Succeed(requirement); return; @@ -197,7 +211,7 @@ public class BulkCollectionAuthorizationHandler : BulkAuthorizationHandler CanManageCollectionsAsync( - ICollection targetCollections, - CurrentContextOrganization org) + private async Task CanManageCollectionsAsync(ICollection targetCollections) { - // List of collection Ids the acting user has access to - var assignedCollectionIds = - (await _collectionRepository.GetManyByUserIdAsync(_currentContext.UserId!.Value, useFlexibleCollections: true)) - .Where(c => - // Check Collections with Manage permission - c.Manage && c.OrganizationId == org.Id) - .Select(c => c.Id) - .ToHashSet(); + if (_managedCollectionsIds == null) + { + var allUserCollections = await _collectionRepository + .GetManyByUserIdAsync(_currentContext.UserId!.Value, useFlexibleCollections: true); + _managedCollectionsIds = allUserCollections + .Where(c => c.Manage) + .Select(c => c.Id) + .ToHashSet(); + } - // Check if the acting user has access to all target collections - return targetCollections.All(tc => assignedCollectionIds.Contains(tc.Id)); + return targetCollections.All(tc => _managedCollectionsIds.Contains(tc.Id)); } private async Task GetOrganizationAbilityAsync(CurrentContextOrganization? organization) diff --git a/src/Sql/dbo/Stored Procedures/OrganizationUser_ReadWithCollectionsById.sql b/src/Sql/dbo/Stored Procedures/OrganizationUser_ReadWithCollectionsById.sql index 6675be82b4..56fb360a7c 100644 --- a/src/Sql/dbo/Stored Procedures/OrganizationUser_ReadWithCollectionsById.sql +++ b/src/Sql/dbo/Stored Procedures/OrganizationUser_ReadWithCollectionsById.sql @@ -9,11 +9,12 @@ BEGIN SELECT CU.[CollectionId] Id, CU.[ReadOnly], - CU.[HidePasswords] + CU.[HidePasswords], + CU.[Manage] FROM [dbo].[OrganizationUser] OU INNER JOIN [dbo].[CollectionUser] CU ON OU.[AccessAll] = 0 AND CU.[OrganizationUserId] = [OU].[Id] WHERE [OrganizationUserId] = @Id -END \ No newline at end of file +END diff --git a/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs b/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs index e64ca09f8a..ac94b5f514 100644 --- a/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs @@ -1,6 +1,8 @@ using System.Security.Claims; using Bit.Api.AdminConsole.Controllers; using Bit.Api.AdminConsole.Models.Request.Organizations; +using Bit.Api.Models.Request; +using Bit.Api.Vault.AuthorizationHandlers.Collections; using Bit.Core; using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.Enums; @@ -10,6 +12,8 @@ using Bit.Core.AdminConsole.Repositories; using Bit.Core.Context; using Bit.Core.Entities; using Bit.Core.Enums; +using Bit.Core.Exceptions; +using Bit.Core.Models.Business; using Bit.Core.Models.Data; using Bit.Core.Models.Data.Organizations; using Bit.Core.Models.Data.Organizations.OrganizationUsers; @@ -104,23 +108,69 @@ public class OrganizationUsersControllerTests .UpdateUserResetPasswordEnrollmentAsync(orgId, user.Id, model.ResetPasswordKey, user.Id); } + [Theory] + [BitAutoData] + public async Task Invite_Success(OrganizationAbility organizationAbility, OrganizationUserInviteRequestModel model, + Guid userId, SutProvider sutProvider) + { + organizationAbility.FlexibleCollections = true; + sutProvider.GetDependency().ManageUsers(organizationAbility.Id).Returns(true); + sutProvider.GetDependency().GetOrganizationAbilityAsync(organizationAbility.Id) + .Returns(organizationAbility); + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), + Arg.Any>(), + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyAccess))) + .Returns(AuthorizationResult.Success()); + sutProvider.GetDependency().GetProperUserId(Arg.Any()).Returns(userId); + + await sutProvider.Sut.Invite(organizationAbility.Id, model); + + await sutProvider.GetDependency().Received(1).InviteUsersAsync(organizationAbility.Id, + userId, Arg.Is>(invites => + invites.Count() == 1 && + invites.First().Item1.Emails.SequenceEqual(model.Emails) && + invites.First().Item1.Type == model.Type && + invites.First().Item1.AccessSecretsManager == model.AccessSecretsManager)); + } + + [Theory] + [BitAutoData] + public async Task Invite_NotAuthorizedToGiveAccessToCollections_Throws(OrganizationAbility organizationAbility, OrganizationUserInviteRequestModel model, + Guid userId, SutProvider sutProvider) + { + organizationAbility.FlexibleCollections = true; + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); + sutProvider.GetDependency().ManageUsers(organizationAbility.Id).Returns(true); + sutProvider.GetDependency().GetOrganizationAbilityAsync(organizationAbility.Id) + .Returns(organizationAbility); + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), + Arg.Any>(), + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyAccess))) + .Returns(AuthorizationResult.Failed()); + sutProvider.GetDependency().GetProperUserId(Arg.Any()).Returns(userId); + + var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.Invite(organizationAbility.Id, model)); + Assert.Contains("You are not authorized", exception.Message); + } + [Theory] [BitAutoData] public async Task Put_Success(OrganizationUserUpdateRequestModel model, OrganizationUser organizationUser, OrganizationAbility organizationAbility, SutProvider sutProvider, Guid savingUserId) { - var orgId = organizationAbility.Id = organizationUser.OrganizationId; - sutProvider.GetDependency().ManageUsers(orgId).Returns(true); - sutProvider.GetDependency().GetByIdAsync(organizationUser.Id).Returns(organizationUser); - sutProvider.GetDependency().GetOrganizationAbilityAsync(orgId) - .Returns(organizationAbility); - sutProvider.GetDependency().GetProperUserId(Arg.Any()).Returns(savingUserId); + organizationAbility.FlexibleCollections = false; + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(false); + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, false); + + // Save these for later - organizationUser object will be mutated var orgUserId = organizationUser.Id; var orgUserEmail = organizationUser.Email; - await sutProvider.Sut.Put(orgId, organizationUser.Id, model); + await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); await sutProvider.GetDependency().Received(1).UpdateUserAsync(Arg.Is(ou => ou.Type == model.Type && @@ -142,21 +192,16 @@ public class OrganizationUsersControllerTests { // Updating self organizationUser.UserId = savingUserId; - organizationAbility.FlexibleCollections = true; organizationAbility.AllowAdminAccessToAllCollectionItems = false; + organizationAbility.FlexibleCollections = true; sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); - var orgId = organizationAbility.Id = organizationUser.OrganizationId; - sutProvider.GetDependency().ManageUsers(orgId).Returns(true); - sutProvider.GetDependency().GetByIdAsync(organizationUser.Id).Returns(organizationUser); - sutProvider.GetDependency().GetOrganizationAbilityAsync(orgId) - .Returns(organizationAbility); - sutProvider.GetDependency().GetProperUserId(Arg.Any()).Returns(savingUserId); + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, true); var orgUserId = organizationUser.Id; var orgUserEmail = organizationUser.Email; - await sutProvider.Sut.Put(orgId, organizationUser.Id, model); + await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); await sutProvider.GetDependency().Received(1).UpdateUserAsync(Arg.Is(ou => ou.Type == model.Type && @@ -182,17 +227,12 @@ public class OrganizationUsersControllerTests organizationAbility.AllowAdminAccessToAllCollectionItems = true; sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); - var orgId = organizationAbility.Id = organizationUser.OrganizationId; - sutProvider.GetDependency().ManageUsers(orgId).Returns(true); - sutProvider.GetDependency().GetByIdAsync(organizationUser.Id).Returns(organizationUser); - sutProvider.GetDependency().GetOrganizationAbilityAsync(orgId) - .Returns(organizationAbility); - sutProvider.GetDependency().GetProperUserId(Arg.Any()).Returns(savingUserId); + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, true); var orgUserId = organizationUser.Id; var orgUserEmail = organizationUser.Email; - await sutProvider.Sut.Put(orgId, organizationUser.Id, model); + await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); await sutProvider.GetDependency().Received(1).UpdateUserAsync(Arg.Is(ou => ou.Type == model.Type && @@ -206,6 +246,124 @@ public class OrganizationUsersControllerTests model.Groups); } + [Theory] + [BitAutoData] + public async Task Put_UpdateCollections_OnlyUpdatesCollectionsTheSavingUserCanUpdate(OrganizationUserUpdateRequestModel model, + OrganizationUser organizationUser, OrganizationAbility organizationAbility, + SutProvider sutProvider, Guid savingUserId) + { + organizationAbility.FlexibleCollections = true; + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, false); + + var editedCollectionId = CoreHelpers.GenerateComb(); + var readonlyCollectionId1 = CoreHelpers.GenerateComb(); + var readonlyCollectionId2 = CoreHelpers.GenerateComb(); + + var currentCollectionAccess = new List + { + new() + { + Id = editedCollectionId, + HidePasswords = true, + Manage = false, + ReadOnly = true + }, + new() + { + Id = readonlyCollectionId1, + HidePasswords = false, + Manage = true, + ReadOnly = false + }, + new() + { + Id = readonlyCollectionId2, + HidePasswords = false, + Manage = false, + ReadOnly = false + }, + }; + + // User is upgrading editedCollectionId to manage + model.Collections = new List + { + new() { Id = editedCollectionId, HidePasswords = false, Manage = true, ReadOnly = false } + }; + + // Save these for later - organizationUser object will be mutated + var orgUserId = organizationUser.Id; + var orgUserEmail = organizationUser.Email; + + sutProvider.GetDependency() + .GetByIdWithCollectionsAsync(organizationUser.Id) + .Returns(new Tuple>(organizationUser, + currentCollectionAccess)); + + var currentCollections = currentCollectionAccess + .Select(cas => new Collection { Id = cas.Id }).ToList(); + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(currentCollections); + + // Authorize the editedCollection + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Is(c => c.Id == editedCollectionId), + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyAccess))) + .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))) + .Returns(AuthorizationResult.Failed()); + + await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); + + // Expect all collection access (modified and unmodified) to be saved + await sutProvider.GetDependency().Received(1).UpdateUserAsync(Arg.Is(ou => + ou.Type == model.Type && + ou.Permissions == CoreHelpers.ClassToJsonData(model.Permissions) && + ou.AccessSecretsManager == model.AccessSecretsManager && + ou.Id == orgUserId && + ou.Email == orgUserEmail), + savingUserId, + Arg.Is>(cas => + cas.Select(c => c.Id).SequenceEqual(currentCollectionAccess.Select(c => c.Id)) && + cas.First(c => c.Id == editedCollectionId).Manage == true && + cas.First(c => c.Id == editedCollectionId).ReadOnly == false && + cas.First(c => c.Id == editedCollectionId).HidePasswords == false), + model.Groups); + } + + [Theory] + [BitAutoData] + public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotUpdateCollections(OrganizationUserUpdateRequestModel model, + OrganizationUser organizationUser, OrganizationAbility organizationAbility, + SutProvider sutProvider, Guid savingUserId) + { + organizationAbility.FlexibleCollections = true; + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, false); + + sutProvider.GetDependency() + .GetByIdWithCollectionsAsync(organizationUser.Id) + .Returns(new Tuple>(organizationUser, + model.Collections.Select(cas => cas.ToSelectionReadOnly()).ToList())); + var collections = model.Collections.Select(cas => new Collection { Id = cas.Id }).ToList(); + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Is>(guids => guids.SequenceEqual(collections.Select(c => c.Id)))) + .Returns(collections); + + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Is(c => collections.Contains(c)), + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyAccess))) + .Returns(AuthorizationResult.Failed()); + + var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model)); + Assert.Contains("You must have Can Manage permission", exception.Message); + } + [Theory] [BitAutoData] public async Task Get_WithFlexibleCollections_ReturnsUsers( @@ -283,6 +441,36 @@ public class OrganizationUsersControllerTests Assert.False(customUserResponse.Permissions.DeleteAssignedCollections); } + private void Put_Setup(SutProvider sutProvider, OrganizationAbility organizationAbility, + OrganizationUser organizationUser, Guid savingUserId, OrganizationUserUpdateRequestModel model, bool authorizeAll) + { + var orgId = organizationAbility.Id = organizationUser.OrganizationId; + + sutProvider.GetDependency().ManageUsers(orgId).Returns(true); + sutProvider.GetDependency().GetByIdAsync(organizationUser.Id).Returns(organizationUser); + sutProvider.GetDependency().GetOrganizationAbilityAsync(orgId) + .Returns(organizationAbility); + sutProvider.GetDependency().GetProperUserId(Arg.Any()).Returns(savingUserId); + + if (authorizeAll) + { + // Simple case: saving user can edit all collections, all collection access is replaced + sutProvider.GetDependency() + .GetByIdWithCollectionsAsync(organizationUser.Id) + .Returns(new Tuple>(organizationUser, + model.Collections.Select(cas => cas.ToSelectionReadOnly()).ToList())); + var collections = model.Collections.Select(cas => new Collection { Id = cas.Id }).ToList(); + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Is>(guids => guids.SequenceEqual(collections.Select(c => c.Id)))) + .Returns(collections); + + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Is(c => collections.Contains(c)), + Arg.Is>(r => r.Contains(BulkCollectionOperations.ModifyAccess))) + .Returns(AuthorizationResult.Success()); + } + } + private void Get_Setup(OrganizationAbility organizationAbility, ICollection organizationUsers, SutProvider sutProvider) diff --git a/test/Api.Test/Vault/AuthorizationHandlers/BulkCollectionAuthorizationHandlerTests.cs b/test/Api.Test/Vault/AuthorizationHandlers/BulkCollectionAuthorizationHandlerTests.cs index 63406c00db..92063544ce 100644 --- a/test/Api.Test/Vault/AuthorizationHandlers/BulkCollectionAuthorizationHandlerTests.cs +++ b/test/Api.Test/Vault/AuthorizationHandlers/BulkCollectionAuthorizationHandlerTests.cs @@ -1,5 +1,6 @@ using System.Security.Claims; using Bit.Api.Vault.AuthorizationHandlers.Collections; +using Bit.Core; using Bit.Core.Context; using Bit.Core.Entities; using Bit.Core.Enums; @@ -536,7 +537,7 @@ public class BulkCollectionAuthorizationHandlerTests [Theory, CollectionCustomization] [BitAutoData(OrganizationUserType.Admin)] [BitAutoData(OrganizationUserType.Owner)] - public async Task CanUpdateCollection_WhenAdminOrOwner_Success( + public async Task CanUpdateCollection_WhenAdminOrOwner_WithoutV1Enabled_Success( OrganizationUserType userType, Guid userId, SutProvider sutProvider, ICollection collections, @@ -569,6 +570,88 @@ public class BulkCollectionAuthorizationHandlerTests } } + [Theory, CollectionCustomization] + [BitAutoData(OrganizationUserType.Admin)] + [BitAutoData(OrganizationUserType.Owner)] + public async Task CanUpdateCollection_WhenAdminOrOwner_WithV1Enabled_PermittedByCollectionManagementSettings_Success( + OrganizationUserType userType, + Guid userId, SutProvider sutProvider, + ICollection collections, CurrentContextOrganization organization, + OrganizationAbility organizationAbility) + { + organization.Type = userType; + organization.Permissions = new Permissions(); + organizationAbility.Id = organization.Id; + organizationAbility.AllowAdminAccessToAllCollectionItems = true; + + var operationsToTest = new[] + { + BulkCollectionOperations.Update, BulkCollectionOperations.ModifyAccess + }; + + foreach (var op in operationsToTest) + { + sutProvider.GetDependency().UserId.Returns(userId); + sutProvider.GetDependency().GetOrganization(organization.Id).Returns(organization); + sutProvider.GetDependency().GetOrganizationAbilityAsync(organization.Id) + .Returns(organizationAbility); + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); + + 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)] + public async Task CanUpdateCollection_WhenAdminOrOwner_WithV1Enabled_NotPermittedByCollectionManagementSettings_Failure( + OrganizationUserType userType, + Guid userId, SutProvider sutProvider, + ICollection collections, CurrentContextOrganization organization, + OrganizationAbility organizationAbility) + { + organization.Type = userType; + organization.Permissions = new Permissions(); + organizationAbility.Id = organization.Id; + organizationAbility.AllowAdminAccessToAllCollectionItems = false; + + var operationsToTest = new[] + { + BulkCollectionOperations.Update, BulkCollectionOperations.ModifyAccess + }; + + foreach (var op in operationsToTest) + { + sutProvider.GetDependency().UserId.Returns(userId); + sutProvider.GetDependency().GetOrganization(organization.Id).Returns(organization); + sutProvider.GetDependency().GetOrganizationAbilityAsync(organization.Id) + .Returns(organizationAbility); + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); + + var context = new AuthorizationHandlerContext( + new[] { op }, + new ClaimsPrincipal(), + collections); + + await sutProvider.Sut.HandleAsync(context); + + Assert.False(context.HasSucceeded); + + // Recreate the SUT to reset the mocks/dependencies between tests + sutProvider.Recreate(); + } + } + [Theory, BitAutoData, CollectionCustomization] public async Task CanUpdateCollection_WithEditAnyCollectionPermission_Success( SutProvider sutProvider, @@ -968,6 +1051,39 @@ public class BulkCollectionAuthorizationHandlerTests } } + [Theory, BitAutoData, CollectionCustomization] + public async Task CachesCollectionsWithCanManagePermissions( + SutProvider sutProvider, + CollectionDetails collection1, CollectionDetails collection2, + CurrentContextOrganization organization, Guid actingUserId) + { + organization.Type = OrganizationUserType.User; + organization.Permissions = new Permissions(); + + sutProvider.GetDependency().UserId.Returns(actingUserId); + sutProvider.GetDependency().GetOrganization(organization.Id).Returns(organization); + sutProvider.GetDependency() + .GetManyByUserIdAsync(actingUserId, Arg.Any()) + .Returns(new List() { collection1, collection2 }); + + var context1 = new AuthorizationHandlerContext( + new[] { BulkCollectionOperations.Update }, + new ClaimsPrincipal(), + collection1); + + await sutProvider.Sut.HandleAsync(context1); + + var context2 = new AuthorizationHandlerContext( + new[] { BulkCollectionOperations.Update }, + new ClaimsPrincipal(), + collection2); + + await sutProvider.Sut.HandleAsync(context2); + + // Expect: only calls the database once + await sutProvider.GetDependency().Received(1).GetManyByUserIdAsync(Arg.Any(), Arg.Any()); + } + private static void ArrangeOrganizationAbility( SutProvider sutProvider, CurrentContextOrganization organization, bool limitCollectionCreationDeletion) diff --git a/util/Migrator/DbScripts/2024-04-05_00_OrganizationUserReadWithCollectionsById.sql b/util/Migrator/DbScripts/2024-04-05_00_OrganizationUserReadWithCollectionsById.sql new file mode 100644 index 0000000000..20c1e7daec --- /dev/null +++ b/util/Migrator/DbScripts/2024-04-05_00_OrganizationUserReadWithCollectionsById.sql @@ -0,0 +1,21 @@ +-- Update to add the Manage column +CREATE OR ALTER PROCEDURE [dbo].[OrganizationUser_ReadWithCollectionsById] + @Id UNIQUEIDENTIFIER +AS +BEGIN + SET NOCOUNT ON + + EXEC [OrganizationUser_ReadById] @Id + + SELECT + CU.[CollectionId] Id, + CU.[ReadOnly], + CU.[HidePasswords], + CU.[Manage] + FROM + [dbo].[OrganizationUser] OU + INNER JOIN + [dbo].[CollectionUser] CU ON OU.[AccessAll] = 0 AND CU.[OrganizationUserId] = [OU].[Id] + WHERE + [OrganizationUserId] = @Id +END