diff --git a/src/Api/AdminConsole/Controllers/GroupsController.cs b/src/Api/AdminConsole/Controllers/GroupsController.cs index dfe927ede6..9e5b8ec264 100644 --- a/src/Api/AdminConsole/Controllers/GroupsController.cs +++ b/src/Api/AdminConsole/Controllers/GroupsController.cs @@ -2,6 +2,7 @@ using Bit.Api.AdminConsole.Models.Response; using Bit.Api.Models.Response; using Bit.Api.Utilities; +using Bit.Api.Vault.AuthorizationHandlers.Collections; using Bit.Api.Vault.AuthorizationHandlers.Groups; using Bit.Core; using Bit.Core.AdminConsole.OrganizationFeatures.Groups.Interfaces; @@ -32,6 +33,7 @@ public class GroupsController : Controller private readonly IUserService _userService; private readonly IFeatureService _featureService; private readonly IOrganizationUserRepository _organizationUserRepository; + private readonly ICollectionRepository _collectionRepository; public GroupsController( IGroupRepository groupRepository, @@ -45,7 +47,8 @@ public class GroupsController : Controller IApplicationCacheService applicationCacheService, IUserService userService, IFeatureService featureService, - IOrganizationUserRepository organizationUserRepository) + IOrganizationUserRepository organizationUserRepository, + ICollectionRepository collectionRepository) { _groupRepository = groupRepository; _groupService = groupService; @@ -59,6 +62,7 @@ public class GroupsController : Controller _userService = userService; _featureService = featureService; _organizationUserRepository = organizationUserRepository; + _collectionRepository = collectionRepository; } [HttpGet("{id}")] @@ -125,16 +129,28 @@ public class GroupsController : Controller } [HttpPost("")] - public async Task Post(string orgId, [FromBody] GroupRequestModel model) + public async Task Post(Guid orgId, [FromBody] GroupRequestModel model) { - var orgIdGuid = new Guid(orgId); - if (!await _currentContext.ManageGroups(orgIdGuid)) + if (!await _currentContext.ManageGroups(orgId)) { throw new NotFoundException(); } - var organization = await _organizationRepository.GetByIdAsync(orgIdGuid); - var group = model.ToGroup(orgIdGuid); + // Flexible Collections - check the user has permission to grant access to the collections for the new group + 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.ModifyGroupAccess)) + .Succeeded; + if (!authorized) + { + throw new NotFoundException("You are not authorized to grant access to these collections."); + } + } + + var organization = await _organizationRepository.GetByIdAsync(orgId); + var group = model.ToGroup(orgId); await _createGroupCommand.CreateGroupAsync(group, organization, model.Collections?.Select(c => c.ToSelectionReadOnly()).ToList(), model.Users); return new GroupResponseModel(group); @@ -144,16 +160,40 @@ public class GroupsController : Controller [HttpPost("{id}")] public async Task Put(Guid orgId, Guid id, [FromBody] GroupRequestModel model) { + if (await FlexibleCollectionsIsEnabledAsync(orgId) && _featureService.IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1)) + { + // Use new Flexible Collections v1 logic + return await Put_vNext(orgId, id, model); + } + + // Pre-Flexible Collections v1 logic follows var group = await _groupRepository.GetByIdAsync(id); if (group == null || !await _currentContext.ManageGroups(group.OrganizationId)) { throw new NotFoundException(); } - // Flexible Collections v1 - a user may not be able to add themselves to a group + var organization = await _organizationRepository.GetByIdAsync(orgId); + + await _updateGroupCommand.UpdateGroupAsync(model.ToGroup(group), organization, + model.Collections.Select(c => c.ToSelectionReadOnly()).ToList(), model.Users); + return new GroupResponseModel(group); + } + + /// + /// Put logic for Flexible Collections v1 + /// + private async Task Put_vNext(Guid orgId, Guid id, [FromBody] GroupRequestModel model) + { + var (group, currentAccess) = await _groupRepository.GetByIdWithCollectionsAsync(id); + if (group == null || !await _currentContext.ManageGroups(group.OrganizationId)) + { + throw new NotFoundException(); + } + + // Check whether the user is permitted to add themselves to the group var orgAbility = await _applicationCacheService.GetOrganizationAbilityAsync(orgId); - var flexibleCollectionsV1Enabled = _featureService.IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1); - if (flexibleCollectionsV1Enabled && orgAbility.FlexibleCollections && !orgAbility.AllowAdminAccessToAllCollectionItems) + if (!orgAbility.AllowAdminAccessToAllCollectionItems) { var userId = _userService.GetProperUserId(User).Value; var organizationUser = await _organizationUserRepository.GetByOrganizationAsync(orgId, userId); @@ -164,9 +204,38 @@ public class GroupsController : Controller } } + // 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 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.ModifyGroupAccess)) + .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(); + var organization = await _organizationRepository.GetByIdAsync(orgId); - await _updateGroupCommand.UpdateGroupAsync(model.ToGroup(group), organization, model.Collections?.Select(c => c.ToSelectionReadOnly()).ToList(), model.Users); + await _updateGroupCommand.UpdateGroupAsync(model.ToGroup(group), organization, collectionsToSave, model.Users); return new GroupResponseModel(group); } diff --git a/src/Api/Vault/AuthorizationHandlers/Collections/CollectionAuthorizationHandler.cs b/src/Api/Vault/AuthorizationHandlers/Collections/CollectionAuthorizationHandler.cs index 64cb1919d8..997cb80191 100644 --- a/src/Api/Vault/AuthorizationHandlers/Collections/CollectionAuthorizationHandler.cs +++ b/src/Api/Vault/AuthorizationHandlers/Collections/CollectionAuthorizationHandler.cs @@ -85,7 +85,8 @@ public class CollectionAuthorizationHandler : AuthorizationHandler sutProvider) + public async Task Post_PreFCv1_Success(Organization organization, GroupRequestModel groupRequestModel, SutProvider sutProvider) { sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); sutProvider.GetDependency().ManageGroups(organization.Id).Returns(true); - var response = await sutProvider.Sut.Post(organization.Id.ToString(), groupRequestModel); + var response = await sutProvider.Sut.Post(organization.Id, groupRequestModel); await sutProvider.GetDependency().Received(1).ManageGroups(organization.Id); await sutProvider.GetDependency().Received(1).CreateGroupAsync( @@ -47,15 +51,100 @@ public class GroupsControllerTests [Theory] [BitAutoData] - public async Task Put_AdminsCanAccessAllCollections_Success(Organization organization, Group group, GroupRequestModel groupRequestModel, SutProvider sutProvider) + public async Task Post_AuthorizedToGiveAccessToCollections_Success(Organization organization, + GroupRequestModel groupRequestModel, SutProvider sutProvider) + { + // Enable FC and v1 + sutProvider.GetDependency().GetOrganizationAbilityAsync(organization.Id).Returns( + new OrganizationAbility { Id = organization.Id, FlexibleCollections = true, AllowAdminAccessToAllCollectionItems = false }); + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); + + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), + Arg.Any>(), + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyGroupAccess))) + .Returns(AuthorizationResult.Success()); + + sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); + sutProvider.GetDependency().ManageGroups(organization.Id).Returns(true); + + var response = await sutProvider.Sut.Post(organization.Id, groupRequestModel); + + var requestModelCollectionIds = groupRequestModel.Collections.Select(c => c.Id).ToHashSet(); + + // Assert that it checked permissions + await sutProvider.GetDependency().Received(1).ManageGroups(organization.Id); + await sutProvider.GetDependency() + .Received(1) + .AuthorizeAsync(Arg.Any(), + Arg.Is>(collections => + collections.All(c => requestModelCollectionIds.Contains(c.Id))), + Arg.Is>(reqs => + reqs.Single() == BulkCollectionOperations.ModifyGroupAccess)); + + // Assert that it saved the data + await sutProvider.GetDependency().Received(1).CreateGroupAsync( + Arg.Is(g => + g.OrganizationId == organization.Id && g.Name == groupRequestModel.Name && + g.AccessAll == groupRequestModel.AccessAll), + organization, + Arg.Is>(access => + access.All(c => requestModelCollectionIds.Contains(c.Id))), + Arg.Any>()); + Assert.Equal(groupRequestModel.Name, response.Name); + Assert.Equal(organization.Id, response.OrganizationId); + Assert.Equal(groupRequestModel.AccessAll, response.AccessAll); + } + + [Theory] + [BitAutoData] + public async Task Post_NotAuthorizedToGiveAccessToCollections_Throws(Organization organization, GroupRequestModel groupRequestModel, SutProvider sutProvider) + { + // Enable FC and v1 + sutProvider.GetDependency().GetOrganizationAbilityAsync(organization.Id).Returns( + new OrganizationAbility { Id = organization.Id, FlexibleCollections = true, AllowAdminAccessToAllCollectionItems = false }); + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); + + sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); + sutProvider.GetDependency().ManageGroups(organization.Id).Returns(true); + + var requestModelCollectionIds = groupRequestModel.Collections.Select(c => c.Id).ToHashSet(); + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), + Arg.Is>(collections => collections.All(c => requestModelCollectionIds.Contains(c.Id))), + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyGroupAccess))) + .Returns(AuthorizationResult.Failed()); + + var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.Post(organization.Id, groupRequestModel)); + + Assert.Contains("You are not authorized to grant access to these collections.", exception.Message); + + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .CreateGroupAsync(default, default, default, default); + } + + [Theory] + [BitAutoData] + public async Task Put_AdminsCanAccessAllCollections_Success(Organization organization, Group group, + GroupRequestModel groupRequestModel, List existingCollectionAccess, + SutProvider sutProvider) { group.OrganizationId = organization.Id; - sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); - sutProvider.GetDependency().GetByIdAsync(group.Id).Returns(group); - sutProvider.GetDependency().ManageGroups(organization.Id).Returns(true); + // Enable FC and v1, set Collection Management Setting sutProvider.GetDependency().GetOrganizationAbilityAsync(organization.Id).Returns( - new OrganizationAbility { Id = organization.Id, AllowAdminAccessToAllCollectionItems = true }); + new OrganizationAbility { Id = organization.Id, AllowAdminAccessToAllCollectionItems = true, FlexibleCollections = true }); + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); + + sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); + sutProvider.GetDependency().GetByIdWithCollectionsAsync(group.Id) + .Returns(new Tuple>(group, existingCollectionAccess)); + sutProvider.GetDependency() + .GetManyByManyIdsAsync(existingCollectionAccess.Select(c => c.Id)) + .Returns(existingCollectionAccess.Select(c => new Collection { Id = c.Id }).ToList()); + sutProvider.GetDependency().ManageGroups(organization.Id).Returns(true); + + var requestModelCollectionIds = groupRequestModel.Collections.Select(c => c.Id).ToHashSet(); var response = await sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel); @@ -65,7 +154,9 @@ public class GroupsControllerTests g.OrganizationId == organization.Id && g.Name == groupRequestModel.Name && g.AccessAll == groupRequestModel.AccessAll), Arg.Is(o => o.Id == organization.Id), - Arg.Any>(), + // Should overwrite any existing collections + Arg.Is>(access => + access.All(c => requestModelCollectionIds.Contains(c.Id))), Arg.Any>()); Assert.Equal(groupRequestModel.Name, response.Name); Assert.Equal(organization.Id, response.OrganizationId); @@ -74,20 +165,13 @@ public class GroupsControllerTests [Theory] [BitAutoData] - public async Task Put_AdminsCannotAccessAllCollections_CannotAddSelfToGroup(Organization organization, Group group, + public async Task Put_UpdateMembers_AdminsCannotAccessAllCollections_CannotAddSelfToGroup(Organization organization, Group group, GroupRequestModel groupRequestModel, OrganizationUser savingOrganizationUser, List currentGroupUsers, SutProvider sutProvider) { group.OrganizationId = organization.Id; - // Saving user is trying to add themselves to the group - var updatedUsers = groupRequestModel.Users.ToList(); - updatedUsers.Add(savingOrganizationUser.Id); - groupRequestModel.Users = updatedUsers; - - sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); - sutProvider.GetDependency().GetByIdAsync(group.Id).Returns(group); - sutProvider.GetDependency().ManageGroups(organization.Id).Returns(true); + // Enable FC and v1 sutProvider.GetDependency().GetOrganizationAbilityAsync(organization.Id).Returns( new OrganizationAbility { @@ -96,6 +180,16 @@ public class GroupsControllerTests FlexibleCollections = true }); sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); + + // Saving user is trying to add themselves to the group + var updatedUsers = groupRequestModel.Users.ToList(); + updatedUsers.Add(savingOrganizationUser.Id); + groupRequestModel.Users = updatedUsers; + + sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); + sutProvider.GetDependency().GetByIdWithCollectionsAsync(group.Id) + .Returns(new Tuple>(group, new List())); + sutProvider.GetDependency().ManageGroups(organization.Id).Returns(true); sutProvider.GetDependency() .GetByOrganizationAsync(organization.Id, Arg.Any()) .Returns(savingOrganizationUser); @@ -112,12 +206,22 @@ public class GroupsControllerTests [Theory] [BitAutoData] - public async Task Put_AdminsCannotAccessAllCollections_Success(Organization organization, Group group, + public async Task Put_UpdateMembers_AdminsCannotAccessAllCollections_AlreadyInGroup_Success(Organization organization, Group group, GroupRequestModel groupRequestModel, OrganizationUser savingOrganizationUser, List currentGroupUsers, SutProvider sutProvider) { group.OrganizationId = organization.Id; + // Enable FC and v1 + sutProvider.GetDependency().GetOrganizationAbilityAsync(organization.Id).Returns( + new OrganizationAbility + { + Id = organization.Id, + AllowAdminAccessToAllCollectionItems = false, + FlexibleCollections = true + }); + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); + // Saving user is trying to add themselves to the group var updatedUsers = groupRequestModel.Users.ToList(); updatedUsers.Add(savingOrganizationUser.Id); @@ -127,16 +231,9 @@ public class GroupsControllerTests currentGroupUsers.Add(savingOrganizationUser.Id); sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); - sutProvider.GetDependency().GetByIdAsync(group.Id).Returns(group); + sutProvider.GetDependency().GetByIdWithCollectionsAsync(group.Id) + .Returns(new Tuple>(group, new List())); sutProvider.GetDependency().ManageGroups(organization.Id).Returns(true); - sutProvider.GetDependency().GetOrganizationAbilityAsync(organization.Id).Returns( - new OrganizationAbility - { - Id = organization.Id, - AllowAdminAccessToAllCollectionItems = false, - FlexibleCollections = true - }); - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); sutProvider.GetDependency() .GetByOrganizationAsync(organization.Id, Arg.Any()) .Returns(savingOrganizationUser); @@ -145,6 +242,9 @@ public class GroupsControllerTests sutProvider.GetDependency().GetManyUserIdsByIdAsync(group.Id) .Returns(currentGroupUsers); + // Make collection authorization pass, it's not being tested here + groupRequestModel.Collections = Array.Empty(); + var response = await sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel); await sutProvider.GetDependency().Received(1).ManageGroups(organization.Id); @@ -159,4 +259,143 @@ public class GroupsControllerTests Assert.Equal(organization.Id, response.OrganizationId); Assert.Equal(groupRequestModel.AccessAll, response.AccessAll); } + + [Theory] + [BitAutoData] + public async Task Put_UpdateCollections_OnlyUpdatesCollectionsTheSavingUserCanUpdate(GroupRequestModel groupRequestModel, + Group group, Organization organization, + SutProvider sutProvider, Guid savingUserId) + { + organization.FlexibleCollections = true; + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); + Put_Setup(sutProvider, organization, group, savingUserId); + + 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 + groupRequestModel.Collections = new List + { + new() { Id = editedCollectionId, HidePasswords = false, Manage = true, ReadOnly = false } + }; + + sutProvider.GetDependency() + .GetByIdWithCollectionsAsync(group.Id) + .Returns(new Tuple>(group, + 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.ModifyGroupAccess))) + .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.ModifyGroupAccess))) + .Returns(AuthorizationResult.Failed()); + + var response = await sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel); + + // Expect all collection access (modified and unmodified) to be saved + await sutProvider.GetDependency().Received(1).ManageGroups(organization.Id); + await sutProvider.GetDependency().Received(1).UpdateGroupAsync( + Arg.Is(g => + g.OrganizationId == organization.Id && g.Name == groupRequestModel.Name && + g.AccessAll == groupRequestModel.AccessAll), + Arg.Is(o => o.Id == organization.Id), + 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), + Arg.Any>()); + Assert.Equal(groupRequestModel.Name, response.Name); + Assert.Equal(organization.Id, response.OrganizationId); + Assert.Equal(groupRequestModel.AccessAll, response.AccessAll); + } + + [Theory] + [BitAutoData] + public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotUpdateCollections(GroupRequestModel groupRequestModel, + Group group, Organization organization, + SutProvider sutProvider, Guid savingUserId) + { + organization.FlexibleCollections = true; + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); + Put_Setup(sutProvider, organization, group, savingUserId); + + sutProvider.GetDependency() + .GetByIdWithCollectionsAsync(group.Id) + .Returns(new Tuple>(group, + groupRequestModel.Collections.Select(cas => cas.ToSelectionReadOnly()).ToList())); + var collections = groupRequestModel.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.ModifyGroupAccess))) + .Returns(AuthorizationResult.Failed()); + + var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel)); + Assert.Contains("You must have Can Manage permission", exception.Message); + } + + private void Put_Setup(SutProvider sutProvider, Organization organization, + Group group, Guid savingUserId) + { + var orgId = organization.Id = group.OrganizationId; + + sutProvider.GetDependency().ManageGroups(orgId).Returns(true); + sutProvider.GetDependency().GetOrganizationAbilityAsync(orgId) + .Returns(new OrganizationAbility + { + Id = organization.Id, + FlexibleCollections = true, + AllowAdminAccessToAllCollectionItems = false + }); + + sutProvider.GetDependency().GetManyUserIdsByIdAsync(group.Id).Returns(new List()); + sutProvider.GetDependency().GetProperUserId(Arg.Any()).Returns(savingUserId); + sutProvider.GetDependency().GetByOrganizationAsync(orgId, savingUserId).Returns(new OrganizationUser + { + Id = savingUserId + }); + sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); + } }