diff --git a/src/Api/AdminConsole/Controllers/GroupsController.cs b/src/Api/AdminConsole/Controllers/GroupsController.cs index 9749691583c6..1d5d1a931910 100644 --- a/src/Api/AdminConsole/Controllers/GroupsController.cs +++ b/src/Api/AdminConsole/Controllers/GroupsController.cs @@ -135,7 +135,7 @@ public async Task Post(Guid orgId, [FromBody] GroupRequestMo .Succeeded; if (!authorized) { - throw new NotFoundException("You are not authorized to grant access to these collections."); + throw new NotFoundException(); } } @@ -175,13 +175,20 @@ await _updateGroupCommand.UpdateGroupAsync(model.ToGroup(group), organization, /// private async Task Put_vNext(Guid orgId, Guid id, [FromBody] GroupRequestModel model) { + if (!await _currentContext.ManageGroups(orgId)) + { + throw new NotFoundException(); + } + var (group, currentAccess) = await _groupRepository.GetByIdWithCollectionsAsync(id); - if (group == null || !await _currentContext.ManageGroups(group.OrganizationId)) + if (group == null || group.OrganizationId != orgId) { throw new NotFoundException(); } - // Check whether the user is permitted to add themselves to the group + // Authorization check: + // If admins are not allowed access to all collections, you cannot add yourself to a group. + // No error is thrown for this, we just don't update groups. var orgAbility = await _applicationCacheService.GetOrganizationAbilityAsync(orgId); if (!orgAbility.AllowAdminAccessToAllCollectionItems) { @@ -195,9 +202,23 @@ private async Task Put_vNext(Guid orgId, Guid id, [FromBody] } } + // Authorization check: + // You must have authorization to ModifyUserAccess for all collections being saved + var postedCollections = await _collectionRepository + .GetManyByManyIdsAsync(model.Collections.Select(c => c.Id)); + foreach (var collection in postedCollections) + { + if (!(await _authorizationService.AuthorizeAsync(User, collection, + BulkCollectionOperations.ModifyGroupAccess)) + .Succeeded) + { + throw new NotFoundException(); + } + } + // 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. + // We need to combine these with collections that the user doesn't have permissions for, so that we don't + // accidentally overwrite those var currentCollections = await _collectionRepository .GetManyByManyIdsAsync(currentAccess.Select(cas => cas.Id)); @@ -211,11 +232,6 @@ private async Task Put_vNext(Guid orgId, Guid id, [FromBody] } } - 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 diff --git a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs index 0562b2563145..30308420626c 100644 --- a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs +++ b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs @@ -230,7 +230,7 @@ public async Task Invite(Guid orgId, [FromBody] OrganizationUserInviteRequestMod .Succeeded; if (!authorized) { - throw new NotFoundException("You are not authorized to grant access to these collections."); + throw new NotFoundException(); } } @@ -400,13 +400,15 @@ private async Task Put_vNext(Guid orgId, Guid id, [FromBody] OrganizationUserUpd var userId = _userService.GetProperUserId(User).Value; var editingSelf = userId == organizationUser.UserId; + // Authorization check: // If admins are not allowed access to all collections, you cannot add yourself to a group. - // In this case we just don't update groups. + // No error is thrown for this, we just don't update groups. var organizationAbility = await _applicationCacheService.GetOrganizationAbilityAsync(orgId); var groupsToSave = editingSelf && !organizationAbility.AllowAdminAccessToAllCollectionItems ? null : model.Groups; + // Authorization check: // If admins are not allowed access to all collections, you cannot add yourself to collections. // This is not caught by the requirement below that you can ModifyUserAccess and must be checked separately var currentAccessIds = currentAccess.Select(c => c.Id).ToHashSet(); @@ -417,9 +419,23 @@ private async Task Put_vNext(Guid orgId, Guid id, [FromBody] OrganizationUserUpd throw new BadRequestException("You cannot add yourself to a collection."); } + // Authorization check: + // You must have authorization to ModifyUserAccess for all collections being saved + var postedCollections = await _collectionRepository + .GetManyByManyIdsAsync(model.Collections.Select(c => c.Id)); + foreach (var collection in postedCollections) + { + if (!(await _authorizationService.AuthorizeAsync(User, collection, + BulkCollectionOperations.ModifyUserAccess)) + .Succeeded) + { + throw new NotFoundException(); + } + } + // The client only sends collections that the saving user has permissions to edit. - // On the server side, we need to (1) make sure the user has permissions for these collections, and - // (2) concat these with the collections that the user can't edit before saving to the database. + // We need to combine these with collections that the user doesn't have permissions for, so that we don't + // accidentally overwrite those var currentCollections = await _collectionRepository .GetManyByManyIdsAsync(currentAccess.Select(cas => cas.Id)); @@ -433,11 +449,6 @@ private async Task Put_vNext(Guid orgId, Guid id, [FromBody] OrganizationUserUpd } } - 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 diff --git a/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs index 3a8c6afd8be0..b471b1d88020 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs @@ -1,4 +1,6 @@ -using Bit.Core.AdminConsole.Entities; +#nullable enable + +using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.OrganizationFeatures.Groups.Interfaces; using Bit.Core.AdminConsole.Repositories; using Bit.Core.Enums; @@ -14,48 +16,53 @@ public class UpdateGroupCommand : IUpdateGroupCommand private readonly IEventService _eventService; private readonly IGroupRepository _groupRepository; private readonly IOrganizationUserRepository _organizationUserRepository; + private readonly ICollectionRepository _collectionRepository; public UpdateGroupCommand( IEventService eventService, IGroupRepository groupRepository, - IOrganizationUserRepository organizationUserRepository) + IOrganizationUserRepository organizationUserRepository, + ICollectionRepository collectionRepository) { _eventService = eventService; _groupRepository = groupRepository; _organizationUserRepository = organizationUserRepository; + _collectionRepository = collectionRepository; } public async Task UpdateGroupAsync(Group group, Organization organization, - ICollection collections = null, - IEnumerable userIds = null) + ICollection? collections = null, + IEnumerable? userIds = null) { - Validate(organization, group, collections); - await GroupRepositoryUpdateGroupAsync(group, collections); + await ValidateAsync(organization, group, collections, userIds); + + await SaveGroupWithCollectionsAsync(group, collections); if (userIds != null) { - await GroupRepositoryUpdateUsersAsync(group, userIds); + await SaveGroupUsersAsync(group, userIds); } await _eventService.LogGroupEventAsync(group, Core.Enums.EventType.Group_Updated); } public async Task UpdateGroupAsync(Group group, Organization organization, EventSystemUser systemUser, - ICollection collections = null, - IEnumerable userIds = null) + ICollection? collections = null, + IEnumerable? userIds = null) { - Validate(organization, group, collections); - await GroupRepositoryUpdateGroupAsync(group, collections); + await ValidateAsync(organization, group, collections, userIds); + + await SaveGroupWithCollectionsAsync(group, collections); if (userIds != null) { - await GroupRepositoryUpdateUsersAsync(group, userIds, systemUser); + await SaveGroupUsersAsync(group, userIds, systemUser); } await _eventService.LogGroupEventAsync(group, Core.Enums.EventType.Group_Updated, systemUser); } - private async Task GroupRepositoryUpdateGroupAsync(Group group, IEnumerable collections = null) + private async Task SaveGroupWithCollectionsAsync(Group group, IEnumerable? collections = null) { group.RevisionDate = DateTime.UtcNow; @@ -69,7 +76,7 @@ private async Task GroupRepositoryUpdateGroupAsync(Group group, IEnumerable userIds, EventSystemUser? systemUser = null) + private async Task SaveGroupUsersAsync(Group group, IEnumerable userIds, EventSystemUser? systemUser = null) { var newUserIds = userIds as Guid[] ?? userIds.ToArray(); var originalUserIds = await _groupRepository.GetManyUserIdsByIdAsync(group.Id); @@ -97,11 +104,15 @@ await _eventService.LogOrganizationUserEventsAsync(users.Select(u => } } - private static void Validate(Organization organization, Group group, IEnumerable collections) + private async Task ValidateAsync(Organization organization, Group group, ICollection? collectionAccess, + IEnumerable? memberAccess) { - if (organization == null) + // Avoid multiple enumeration + memberAccess = memberAccess?.ToList(); + + if (organization == null || organization.Id != group.OrganizationId) { - throw new BadRequestException("Organization not found"); + throw new NotFoundException(); } if (!organization.UseGroups) @@ -109,15 +120,73 @@ private static void Validate(Organization organization, Group group, IEnumerable throw new BadRequestException("This organization cannot use groups."); } + var originalGroup = await _groupRepository.GetByIdAsync(group.Id); + if (originalGroup == null || originalGroup.OrganizationId != group.OrganizationId) + { + throw new NotFoundException(); + } + + if (collectionAccess?.Any() == true) + { + await ValidateCollectionAccessAsync(originalGroup, collectionAccess); + } + + if (memberAccess?.Any() == true) + { + await ValidateMemberAccessAsync(originalGroup, memberAccess.ToList()); + } + if (group.AccessAll) { throw new BadRequestException("The AccessAll property has been deprecated by collection enhancements. Assign the group to collections instead."); } - var invalidAssociations = collections?.Where(cas => cas.Manage && (cas.ReadOnly || cas.HidePasswords)); + var invalidAssociations = collectionAccess?.Where(cas => cas.Manage && (cas.ReadOnly || cas.HidePasswords)); if (invalidAssociations?.Any() ?? false) { throw new BadRequestException("The Manage property is mutually exclusive and cannot be true while the ReadOnly or HidePasswords properties are also true."); } } + + private async Task ValidateCollectionAccessAsync(Group originalGroup, + ICollection collectionAccess) + { + var collections = await _collectionRepository + .GetManyByManyIdsAsync(collectionAccess.Select(c => c.Id)); + var collectionIds = collections.Select(c => c.Id); + + var missingCollection = collectionAccess + .FirstOrDefault(cas => !collectionIds.Contains(cas.Id)); + if (missingCollection != default) + { + throw new NotFoundException(); + } + + var invalidCollection = collections.FirstOrDefault(c => c.OrganizationId != originalGroup.OrganizationId); + if (invalidCollection != default) + { + // Use generic error message to avoid enumeration + throw new NotFoundException(); + } + } + + private async Task ValidateMemberAccessAsync(Group originalGroup, + ICollection memberAccess) + { + var members = await _organizationUserRepository.GetManyAsync(memberAccess); + var memberIds = members.Select(g => g.Id); + + var missingMemberId = memberAccess.FirstOrDefault(mId => !memberIds.Contains(mId)); + if (missingMemberId != default) + { + throw new NotFoundException(); + } + + var invalidMember = members.FirstOrDefault(m => m.OrganizationId != originalGroup.OrganizationId); + if (invalidMember != default) + { + // Use generic error message to avoid enumeration + throw new NotFoundException(); + } + } } diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IUpdateOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IUpdateOrganizationUserCommand.cs index 805b4400231a..c7298e1cd90a 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IUpdateOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IUpdateOrganizationUserCommand.cs @@ -6,5 +6,6 @@ namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interface public interface IUpdateOrganizationUserCommand { - Task UpdateUserAsync(OrganizationUser user, Guid? savingUserId, List collections, IEnumerable? groups); + Task UpdateUserAsync(OrganizationUser user, Guid? savingUserId, + List? collectionAccess, IEnumerable? groupAccess); } diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs index 0abae991d0e4..a37d94fb645b 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs @@ -1,5 +1,6 @@ #nullable enable using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; +using Bit.Core.AdminConsole.Repositories; using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Exceptions; @@ -19,6 +20,8 @@ public class UpdateOrganizationUserCommand : IUpdateOrganizationUserCommand private readonly IOrganizationUserRepository _organizationUserRepository; private readonly ICountNewSmSeatsRequiredQuery _countNewSmSeatsRequiredQuery; private readonly IUpdateSecretsManagerSubscriptionCommand _updateSecretsManagerSubscriptionCommand; + private readonly ICollectionRepository _collectionRepository; + private readonly IGroupRepository _groupRepository; public UpdateOrganizationUserCommand( IEventService eventService, @@ -26,7 +29,9 @@ public UpdateOrganizationUserCommand( IOrganizationRepository organizationRepository, IOrganizationUserRepository organizationUserRepository, ICountNewSmSeatsRequiredQuery countNewSmSeatsRequiredQuery, - IUpdateSecretsManagerSubscriptionCommand updateSecretsManagerSubscriptionCommand) + IUpdateSecretsManagerSubscriptionCommand updateSecretsManagerSubscriptionCommand, + ICollectionRepository collectionRepository, + IGroupRepository groupRepository) { _eventService = eventService; _organizationService = organizationService; @@ -34,17 +39,45 @@ public UpdateOrganizationUserCommand( _organizationUserRepository = organizationUserRepository; _countNewSmSeatsRequiredQuery = countNewSmSeatsRequiredQuery; _updateSecretsManagerSubscriptionCommand = updateSecretsManagerSubscriptionCommand; + _collectionRepository = collectionRepository; + _groupRepository = groupRepository; } + /// + /// Update an organization user. + /// + /// The modified user to save. + /// The userId of the currently logged in user who is making the change. + /// The user's updated collection access. If set to null, this removes all collection access. + /// The user's updated group access. If set to null, groups are not updated. + /// public async Task UpdateUserAsync(OrganizationUser user, Guid? savingUserId, - List? collections, IEnumerable? groups) + List? collectionAccess, IEnumerable? groupAccess) { + // Avoid multiple enumeration + collectionAccess = collectionAccess?.ToList(); + groupAccess = groupAccess?.ToList(); + if (user.Id.Equals(default(Guid))) { throw new BadRequestException("Invite the user first."); } var originalUser = await _organizationUserRepository.GetByIdAsync(user.Id); + if (originalUser == null || user.OrganizationId != originalUser.OrganizationId) + { + throw new NotFoundException(); + } + + if (collectionAccess?.Any() == true) + { + await ValidateCollectionAccessAsync(originalUser, collectionAccess.ToList()); + } + + if (groupAccess?.Any() == true) + { + await ValidateGroupAccessAsync(originalUser, groupAccess.ToList()); + } if (savingUserId.HasValue) { @@ -64,9 +97,9 @@ public async Task UpdateUserAsync(OrganizationUser user, Guid? savingUserId, throw new BadRequestException("The AccessAll property has been deprecated by collection enhancements. Assign the user to collections instead."); } - if (collections?.Count > 0) + if (collectionAccess?.Count > 0) { - var invalidAssociations = collections.Where(cas => cas.Manage && (cas.ReadOnly || cas.HidePasswords)); + var invalidAssociations = collectionAccess.Where(cas => cas.Manage && (cas.ReadOnly || cas.HidePasswords)); if (invalidAssociations.Any()) { throw new BadRequestException("The Manage property is mutually exclusive and cannot be true while the ReadOnly or HidePasswords properties are also true."); @@ -87,13 +120,55 @@ public async Task UpdateUserAsync(OrganizationUser user, Guid? savingUserId, } } - await _organizationUserRepository.ReplaceAsync(user, collections); + await _organizationUserRepository.ReplaceAsync(user, collectionAccess); - if (groups != null) + if (groupAccess != null) { - await _organizationUserRepository.UpdateGroupsAsync(user.Id, groups); + await _organizationUserRepository.UpdateGroupsAsync(user.Id, groupAccess); } await _eventService.LogOrganizationUserEventAsync(user, EventType.OrganizationUser_Updated); } + + private async Task ValidateCollectionAccessAsync(OrganizationUser originalUser, + ICollection collectionAccess) + { + var collections = await _collectionRepository + .GetManyByManyIdsAsync(collectionAccess.Select(c => c.Id)); + var collectionIds = collections.Select(c => c.Id); + + var missingCollection = collectionAccess + .FirstOrDefault(cas => !collectionIds.Contains(cas.Id)); + if (missingCollection != default) + { + throw new NotFoundException(); + } + + var invalidCollection = collections.FirstOrDefault(c => c.OrganizationId != originalUser.OrganizationId); + if (invalidCollection != default) + { + // Use generic error message to avoid enumeration + throw new NotFoundException(); + } + } + + private async Task ValidateGroupAccessAsync(OrganizationUser originalUser, + ICollection groupAccess) + { + var groups = await _groupRepository.GetManyByManyIds(groupAccess); + var groupIds = groups.Select(g => g.Id); + + var missingGroupId = groupAccess.FirstOrDefault(gId => !groupIds.Contains(gId)); + if (missingGroupId != default) + { + throw new NotFoundException(); + } + + var invalidGroup = groups.FirstOrDefault(g => g.OrganizationId != originalUser.OrganizationId); + if (invalidGroup != default) + { + // Use generic error message to avoid enumeration + throw new NotFoundException(); + } + } } diff --git a/test/Api.Test/AdminConsole/Controllers/GroupsControllerPutTests.cs b/test/Api.Test/AdminConsole/Controllers/GroupsControllerPutTests.cs new file mode 100644 index 000000000000..3a8e4831afc4 --- /dev/null +++ b/test/Api.Test/AdminConsole/Controllers/GroupsControllerPutTests.cs @@ -0,0 +1,321 @@ +using System.Security.Claims; +using Bit.Api.AdminConsole.Controllers; +using Bit.Api.AdminConsole.Models.Request; +using Bit.Api.Models.Request; +using Bit.Api.Vault.AuthorizationHandlers.Collections; +using Bit.Core; +using Bit.Core.AdminConsole.Entities; +using Bit.Core.AdminConsole.OrganizationFeatures.Groups.Interfaces; +using Bit.Core.AdminConsole.Repositories; +using Bit.Core.Context; +using Bit.Core.Entities; +using Bit.Core.Exceptions; +using Bit.Core.Models.Data; +using Bit.Core.Models.Data.Organizations; +using Bit.Core.Repositories; +using Bit.Core.Services; +using Bit.Core.Utilities; +using Bit.Test.Common.AutoFixture; +using Bit.Test.Common.AutoFixture.Attributes; +using Microsoft.AspNetCore.Authorization; +using NSubstitute; +using Xunit; + +namespace Bit.Api.Test.AdminConsole.Controllers; + +[ControllerCustomize(typeof(GroupsController))] +[SutProviderCustomize] +public class GroupsControllerPutTests +{ + [Theory] + [BitAutoData] + public async Task Put_WithAdminAccess_Success(Organization organization, Group group, + GroupRequestModel groupRequestModel, List existingCollectionAccess, + OrganizationUser savingUser, SutProvider sutProvider) + { + Put_Setup(sutProvider, organization, true, group, savingUser, existingCollectionAccess, []); + + var requestModelCollectionIds = groupRequestModel.Collections.Select(c => c.Id).ToHashSet(); + + // Authorize all changes for basic happy path test + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Any(), + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyGroupAccess))) + .Returns(AuthorizationResult.Success()); + + var response = await sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel); + + 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), + Arg.Is(o => o.Id == organization.Id), + // Should overwrite any existing collections + Arg.Is>(access => + access.All(c => requestModelCollectionIds.Contains(c.Id))), + Arg.Is>(guids => guids.ToHashSet().SetEquals(groupRequestModel.Users.ToHashSet()))); + Assert.Equal(groupRequestModel.Name, response.Name); + Assert.Equal(organization.Id, response.OrganizationId); + } + + [Theory] + [BitAutoData] + public async Task Put_UpdateMembers_NoAdminAccess_CannotAddSelfToGroup(Organization organization, Group group, + GroupRequestModel groupRequestModel, OrganizationUser savingUser, List currentGroupUsers, + SutProvider sutProvider) + { + // Not updating collections + groupRequestModel.Collections = []; + + Put_Setup(sutProvider, organization, false, group, savingUser, + currentCollectionAccess: [], currentGroupUsers); + + // Saving user is trying to add themselves to the group + var updatedUsers = groupRequestModel.Users.ToList(); + updatedUsers.Add(savingUser.Id); + groupRequestModel.Users = updatedUsers; + + var exception = await + Assert.ThrowsAsync(() => sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel)); + + Assert.Contains("You cannot add yourself to groups", exception.Message); + } + + [Theory] + [BitAutoData] + public async Task Put_UpdateMembers_NoAdminAccess_AlreadyInGroup_Success(Organization organization, Group group, + GroupRequestModel groupRequestModel, OrganizationUser savingUser, List currentGroupUsers, + SutProvider sutProvider) + { + // Not changing collection access + groupRequestModel.Collections = []; + + // Saving user is trying to add themselves to the group + var updatedUsers = groupRequestModel.Users.ToList(); + updatedUsers.Add(savingUser.Id); + groupRequestModel.Users = updatedUsers; + + // But! they are already a member of the group + currentGroupUsers.Add(savingUser.Id); + + Put_Setup(sutProvider, organization, false, group, savingUser, currentCollectionAccess: [], currentGroupUsers); + + var response = await sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel); + + 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), + Arg.Is(o => o.Id == organization.Id), + Arg.Any>(), + Arg.Any>()); + Assert.Equal(groupRequestModel.Name, response.Name); + Assert.Equal(organization.Id, response.OrganizationId); + } + + [Theory] + [BitAutoData] + public async Task Put_UpdateMembers_WithAdminAccess_CanAddSelfToGroup(Organization organization, Group group, + GroupRequestModel groupRequestModel, OrganizationUser savingUser, List currentGroupUsers, + SutProvider sutProvider) + { + // Not updating collections + groupRequestModel.Collections = []; + + Put_Setup(sutProvider, organization, true, group, savingUser, + currentCollectionAccess: [], currentGroupUsers); + + // Saving user is trying to add themselves to the group + var updatedUsers = groupRequestModel.Users.ToList(); + updatedUsers.Add(savingUser.Id); + groupRequestModel.Users = updatedUsers; + + var response = await sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel); + + await sutProvider.GetDependency().Received(1).UpdateGroupAsync( + Arg.Is(g => + g.OrganizationId == organization.Id && g.Name == groupRequestModel.Name), + Arg.Is(o => o.Id == organization.Id), + Arg.Any>(), + Arg.Is>(guids => guids.ToHashSet().SetEquals(groupRequestModel.Users.ToHashSet()))); + Assert.Equal(groupRequestModel.Name, response.Name); + Assert.Equal(organization.Id, response.OrganizationId); + } + + [Theory] + [BitAutoData] + public async Task Put_UpdateMembers_NoAdminAccess_ProviderUser_Success(Organization organization, Group group, + GroupRequestModel groupRequestModel, List currentGroupUsers, SutProvider sutProvider) + { + // Make collection authorization pass, it's not being tested here + groupRequestModel.Collections = Array.Empty(); + + Put_Setup(sutProvider, organization, false, group, null, currentCollectionAccess: [], currentGroupUsers); + + var response = await sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel); + + 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), + Arg.Is(o => o.Id == organization.Id), + Arg.Any>(), + Arg.Any>()); + Assert.Equal(groupRequestModel.Name, response.Name); + Assert.Equal(organization.Id, response.OrganizationId); + } + + [Theory] + [BitAutoData] + public async Task Put_UpdateCollections_DoesNotOverwriteUnauthorizedCollections(GroupRequestModel groupRequestModel, + Group group, Organization organization, + SutProvider sutProvider, OrganizationUser savingUser) + { + 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 + }, + }; + + Put_Setup(sutProvider, organization, false, group, savingUser, currentCollectionAccess, currentGroupUsers: []); + + // User is upgrading editedCollectionId to manage + groupRequestModel.Collections = new List + { + new() { Id = editedCollectionId, HidePasswords = false, Manage = true, ReadOnly = false } + }; + + // 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), + 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); + } + + [Theory] + [BitAutoData] + public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotUpdateCollections(GroupRequestModel groupRequestModel, + Group group, Organization organization, + SutProvider sutProvider, OrganizationUser savingUser) + { + // Group is currently assigned to the POSTed collections + Put_Setup(sutProvider, organization, false, group, savingUser, + groupRequestModel.Collections.Select(cas => cas.ToSelectionReadOnly()).ToList(), + []); + + var postedCollectionIds = groupRequestModel.Collections.Select(c => c.Id).ToHashSet(); + + // But the saving user does not have permission to update them + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Is(c => postedCollectionIds.Contains(c.Id)), + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyGroupAccess))) + .Returns(AuthorizationResult.Failed()); + + await Assert.ThrowsAsync(() => sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel)); + } + + [Theory] + [BitAutoData] + public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotAddCollections(GroupRequestModel groupRequestModel, + Group group, Organization organization, + SutProvider sutProvider, OrganizationUser savingUser) + { + // Group is not assigned to the POSTed collections + Put_Setup(sutProvider, organization, false, group, savingUser, [], []); + + var postedCollectionIds = groupRequestModel.Collections.Select(c => c.Id).ToHashSet(); + + // But the saving user does not have permission to update them + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Is(c => postedCollectionIds.Contains(c.Id)), + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyGroupAccess))) + .Returns(AuthorizationResult.Failed()); + + await Assert.ThrowsAsync(() => sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel)); + } + + private void Put_Setup(SutProvider sutProvider, Organization organization, + bool adminAccess, Group group, OrganizationUser? savingUser, List currentCollectionAccess, + List currentGroupUsers) + { + // FCv1 is now fully enabled + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); + + var orgId = organization.Id = group.OrganizationId; + + // Arrange org and orgAbility + sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); + sutProvider.GetDependency().GetOrganizationAbilityAsync(orgId) + .Returns(new OrganizationAbility + { + Id = organization.Id, + AllowAdminAccessToAllCollectionItems = adminAccess + }); + + // Arrange user + // If no savingUser provided, they're not an org user, just return a random guid + sutProvider.GetDependency().GetProperUserId(Arg.Any()).Returns(savingUser?.UserId ?? CoreHelpers.GenerateComb()); + sutProvider.GetDependency().ManageGroups(orgId).Returns(true); + + // Arrange repositories + sutProvider.GetDependency().GetManyUserIdsByIdAsync(group.Id).Returns(currentGroupUsers ?? []); + sutProvider.GetDependency().GetByIdWithCollectionsAsync(group.Id) + .Returns(new Tuple>(group, currentCollectionAccess ?? [])); + if (savingUser != null) + { + sutProvider.GetDependency().GetByOrganizationAsync(orgId, savingUser.UserId.Value) + .Returns(savingUser); + } + + // Collection repository: return mock Collection objects for any ids passed in + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(callInfo => callInfo.Arg>().Select(guid => new Collection { Id = guid }).ToList()); + } +} diff --git a/test/Api.Test/AdminConsole/Controllers/GroupsControllerTests.cs b/test/Api.Test/AdminConsole/Controllers/GroupsControllerTests.cs index 6f077fbd3e90..eb223317f4d6 100644 --- a/test/Api.Test/AdminConsole/Controllers/GroupsControllerTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/GroupsControllerTests.cs @@ -1,12 +1,10 @@ using System.Security.Claims; using Bit.Api.AdminConsole.Controllers; using Bit.Api.AdminConsole.Models.Request; -using Bit.Api.Models.Request; using Bit.Api.Vault.AuthorizationHandlers.Collections; using Bit.Core; using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.OrganizationFeatures.Groups.Interfaces; -using Bit.Core.AdminConsole.Repositories; using Bit.Core.Context; using Bit.Core.Entities; using Bit.Core.Exceptions; @@ -14,7 +12,6 @@ using Bit.Core.Models.Data.Organizations; using Bit.Core.Repositories; using Bit.Core.Services; -using Bit.Core.Utilities; using Bit.Test.Common.AutoFixture; using Bit.Test.Common.AutoFixture.Attributes; using Microsoft.AspNetCore.Authorization; @@ -111,321 +108,9 @@ public async Task Post_NotAuthorizedToGiveAccessToCollections_Throws(Organizatio 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 Assert.ThrowsAsync(() => sutProvider.Sut.Post(organization.Id, groupRequestModel)); 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; - - // Enable FC and v1, set Collection Management Setting - sutProvider.GetDependency().GetOrganizationAbilityAsync(organization.Id).Returns( - new OrganizationAbility { Id = organization.Id, AllowAdminAccessToAllCollectionItems = 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); - - 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), - Arg.Is(o => o.Id == organization.Id), - // 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); - } - - [Theory] - [BitAutoData] - public async Task Put_UpdateMembers_AdminsCannotAccessAllCollections_CannotAddSelfToGroup(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, - }); - 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); - sutProvider.GetDependency().GetProperUserId(Arg.Any()) - .Returns(savingOrganizationUser.UserId); - sutProvider.GetDependency().GetManyUserIdsByIdAsync(group.Id) - .Returns(currentGroupUsers); - - var exception = await - Assert.ThrowsAsync(() => sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel)); - - Assert.Contains("You cannot add yourself to groups", exception.Message); - } - - [Theory] - [BitAutoData] - 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, - }); - 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; - - // But! they are already a member of the group - currentGroupUsers.Add(savingOrganizationUser.Id); - - 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); - sutProvider.GetDependency().GetProperUserId(Arg.Any()) - .Returns(savingOrganizationUser.UserId); - 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); - await sutProvider.GetDependency().Received(1).UpdateGroupAsync( - Arg.Is(g => - g.OrganizationId == organization.Id && g.Name == groupRequestModel.Name), - Arg.Is(o => o.Id == organization.Id), - Arg.Any>(), - Arg.Any>()); - Assert.Equal(groupRequestModel.Name, response.Name); - Assert.Equal(organization.Id, response.OrganizationId); - } - - [Theory] - [BitAutoData] - public async Task Put_UpdateMembers_AdminsCannotAccessAllCollections_ProviderUser_Success(Organization organization, Group group, - GroupRequestModel groupRequestModel, List currentGroupUsers, Guid savingUserId, - SutProvider sutProvider) - { - group.OrganizationId = organization.Id; - - // Enable FC and v1 - sutProvider.GetDependency().GetOrganizationAbilityAsync(organization.Id).Returns( - new OrganizationAbility - { - Id = organization.Id, - AllowAdminAccessToAllCollectionItems = false, - }); - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); - - 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((OrganizationUser)null); // Provider is not an OrganizationUser, so it will always return null - sutProvider.GetDependency().GetProperUserId(Arg.Any()) - .Returns(savingUserId); - 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); - await sutProvider.GetDependency().Received(1).UpdateGroupAsync( - Arg.Is(g => - g.OrganizationId == organization.Id && g.Name == groupRequestModel.Name), - Arg.Is(o => o.Id == organization.Id), - Arg.Any>(), - Arg.Any>()); - Assert.Equal(groupRequestModel.Name, response.Name); - Assert.Equal(organization.Id, response.OrganizationId); - } - - [Theory] - [BitAutoData] - public async Task Put_UpdateCollections_OnlyUpdatesCollectionsTheSavingUserCanUpdate(GroupRequestModel groupRequestModel, - Group group, Organization organization, - SutProvider sutProvider, Guid savingUserId) - { - 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), - 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); - } - - [Theory] - [BitAutoData] - public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotUpdateCollections(GroupRequestModel groupRequestModel, - Group group, Organization organization, - SutProvider sutProvider, Guid savingUserId) - { - 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, - 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); - } } diff --git a/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs b/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs new file mode 100644 index 000000000000..0b9c18c57019 --- /dev/null +++ b/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs @@ -0,0 +1,284 @@ +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.OrganizationFeatures.OrganizationUsers.Interfaces; +using Bit.Core.Context; +using Bit.Core.Entities; +using Bit.Core.Exceptions; +using Bit.Core.Models.Data; +using Bit.Core.Models.Data.Organizations; +using Bit.Core.Repositories; +using Bit.Core.Services; +using Bit.Core.Utilities; +using Bit.Test.Common.AutoFixture; +using Bit.Test.Common.AutoFixture.Attributes; +using Microsoft.AspNetCore.Authorization; +using NSubstitute; +using Xunit; + +namespace Bit.Api.Test.AdminConsole.Controllers; + +[ControllerCustomize(typeof(OrganizationUsersController))] +[SutProviderCustomize] +public class OrganizationUserControllerPutTests +{ + [Theory] + [BitAutoData] + public async Task Put_Success(OrganizationUserUpdateRequestModel model, + OrganizationUser organizationUser, OrganizationAbility organizationAbility, + SutProvider sutProvider, Guid savingUserId) + { + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, currentCollectionAccess: []); + + // Authorize all changes for basic happy path test + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Any(), + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) + .Returns(AuthorizationResult.Success()); + + // Save these for later - organizationUser object will be mutated + var orgUserId = organizationUser.Id; + var orgUserEmail = organizationUser.Email; + + await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); + + 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.All(c => model.Collections.Any(m => m.Id == c.Id))), + model.Groups); + } + + [Theory] + [BitAutoData] + public async Task Put_NoAdminAccess_CannotAddSelfToCollections(OrganizationUserUpdateRequestModel model, + OrganizationUser organizationUser, OrganizationAbility organizationAbility, + SutProvider sutProvider, Guid savingUserId) + { + // Updating self + organizationUser.UserId = savingUserId; + organizationAbility.AllowAdminAccessToAllCollectionItems = false; + + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, currentCollectionAccess: []); + + var exception = await Assert.ThrowsAsync(async () => await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model)); + Assert.Contains("You cannot add yourself to a collection.", exception.Message); + } + [Theory] + [BitAutoData] + public async Task Put_NoAdminAccess_CannotAddSelfToGroups(OrganizationUserUpdateRequestModel model, + OrganizationUser organizationUser, OrganizationAbility organizationAbility, + SutProvider sutProvider, Guid savingUserId) + { + // Updating self + organizationUser.UserId = savingUserId; + organizationAbility.AllowAdminAccessToAllCollectionItems = false; + + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, currentCollectionAccess: []); + + // Not changing any collection access + model.Collections = new List(); + + var orgUserId = organizationUser.Id; + var orgUserEmail = organizationUser.Email; + + await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); + + 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.All(c => model.Collections.Any(m => m.Id == c.Id))), + // Main assertion: groups are not updated (are null) + null); + } + + [Theory] + [BitAutoData] + public async Task Put_WithAdminAccess_CanAddSelfToGroups(OrganizationUserUpdateRequestModel model, + OrganizationUser organizationUser, OrganizationAbility organizationAbility, + SutProvider sutProvider, Guid savingUserId) + { + // Updating self + organizationUser.UserId = savingUserId; + organizationAbility.AllowAdminAccessToAllCollectionItems = true; + + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, currentCollectionAccess: []); + + // Not changing any collection access + model.Collections = new List(); + + var orgUserId = organizationUser.Id; + var orgUserEmail = organizationUser.Email; + + await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); + + 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.All(c => model.Collections.Any(m => m.Id == c.Id))), + model.Groups); + } + + [Theory] + [BitAutoData] + public async Task Put_UpdateCollections_DoesNotOverwriteUnauthorizedCollections(OrganizationUserUpdateRequestModel model, + OrganizationUser organizationUser, OrganizationAbility organizationAbility, + SutProvider sutProvider, Guid 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 + }, + }; + + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, currentCollectionAccess); + + // 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; + + // Authorize the editedCollection + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Is(c => c.Id == editedCollectionId), + 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.ModifyUserAccess))) + .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) + { + // Target user is currently assigned to the POSTed collections + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, + currentCollectionAccess: model.Collections.Select(cas => cas.ToSelectionReadOnly()).ToList()); + + var postedCollectionIds = model.Collections.Select(c => c.Id).ToHashSet(); + + // But the saving user does not have permission to update them + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Is(c => postedCollectionIds.Contains(c.Id)), + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) + .Returns(AuthorizationResult.Failed()); + + await Assert.ThrowsAsync(() => sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model)); + } + + [Theory] + [BitAutoData] + public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotAddCollections(OrganizationUserUpdateRequestModel model, + OrganizationUser organizationUser, OrganizationAbility organizationAbility, + SutProvider sutProvider, Guid savingUserId) + { + // The target user is not currently assigned to any collections, so we're granting access for the first time + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, currentCollectionAccess: []); + + var postedCollectionIds = model.Collections.Select(c => c.Id).ToHashSet(); + // But the saving user does not have permission to assign access to the collections + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Is(c => postedCollectionIds.Contains(c.Id)), + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) + .Returns(AuthorizationResult.Failed()); + + await Assert.ThrowsAsync(() => sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model)); + } + + private void Put_Setup(SutProvider sutProvider, + OrganizationAbility organizationAbility, OrganizationUser organizationUser, Guid savingUserId, + List currentCollectionAccess) + { + // FCv1 is now fully enabled + 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); + + // OrganizationUserRepository: return the user with current collection access + sutProvider.GetDependency() + .GetByIdWithCollectionsAsync(organizationUser.Id) + .Returns(new Tuple>(organizationUser, + currentCollectionAccess ?? [])); + + // Collection repository: return mock Collection objects for any ids passed in + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(callInfo => callInfo.Arg>().Select(guid => new Collection { Id = guid }).ToList()); + } +} diff --git a/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs b/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs index f2898db2e4a2..deb4c6818859 100644 --- a/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs @@ -1,13 +1,11 @@ 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; using Bit.Core.AdminConsole.Models.Data.Organizations.Policies; -using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; using Bit.Core.AdminConsole.Repositories; using Bit.Core.Auth.Entities; using Bit.Core.Auth.Repositories; @@ -183,241 +181,7 @@ public async Task Invite_NotAuthorizedToGiveAccessToCollections_Throws(Organizat .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) - { - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(false); - - Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, true); - - // Save these for later - organizationUser object will be mutated - var orgUserId = organizationUser.Id; - var orgUserEmail = organizationUser.Email; - - await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); - - 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.All(c => model.Collections.Any(m => m.Id == c.Id))), - model.Groups); - } - - [Theory] - [BitAutoData] - public async Task Put_UpdateSelf_WithoutAllowAdminAccessToAllCollectionItems_CannotAddSelfToCollections(OrganizationUserUpdateRequestModel model, - OrganizationUser organizationUser, OrganizationAbility organizationAbility, - SutProvider sutProvider, Guid savingUserId) - { - // Updating self - organizationUser.UserId = savingUserId; - organizationAbility.AllowAdminAccessToAllCollectionItems = false; - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); - - Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, false); - - // User is not currently assigned to any collections, which means they're adding themselves - sutProvider.GetDependency() - .GetByIdWithCollectionsAsync(organizationUser.Id) - .Returns(new Tuple>(organizationUser, - new List())); - sutProvider.GetDependency() - .GetManyByManyIdsAsync(Arg.Any>()) - .Returns(new List()); - - var orgUserId = organizationUser.Id; - var orgUserEmail = organizationUser.Email; - - var exception = await Assert.ThrowsAsync(async () => await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model)); - Assert.Contains("You cannot add yourself to a collection.", exception.Message); - } - - [Theory] - [BitAutoData] - public async Task Put_UpdateSelf_WithoutAllowAdminAccessToAllCollectionItems_DoesNotUpdateGroups(OrganizationUserUpdateRequestModel model, - OrganizationUser organizationUser, OrganizationAbility organizationAbility, - SutProvider sutProvider, Guid savingUserId) - { - // Updating self - organizationUser.UserId = savingUserId; - organizationAbility.AllowAdminAccessToAllCollectionItems = false; - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); - - Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, true); - - var orgUserId = organizationUser.Id; - var orgUserEmail = organizationUser.Email; - - await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); - - 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.All(c => model.Collections.Any(m => m.Id == c.Id))), - null); - } - - [Theory] - [BitAutoData] - public async Task Put_UpdateSelf_WithAllowAdminAccessToAllCollectionItems_DoesUpdateGroups(OrganizationUserUpdateRequestModel model, - OrganizationUser organizationUser, OrganizationAbility organizationAbility, - SutProvider sutProvider, Guid savingUserId) - { - // Updating self - organizationUser.UserId = savingUserId; - organizationAbility.AllowAdminAccessToAllCollectionItems = true; - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); - - Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, true); - - var orgUserId = organizationUser.Id; - var orgUserEmail = organizationUser.Email; - - await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); - - 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.All(c => model.Collections.Any(m => m.Id == c.Id))), - model.Groups); - } - - [Theory] - [BitAutoData] - public async Task Put_UpdateCollections_OnlyUpdatesCollectionsTheSavingUserCanUpdate(OrganizationUserUpdateRequestModel model, - OrganizationUser organizationUser, OrganizationAbility organizationAbility, - SutProvider sutProvider, Guid savingUserId) - { - 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.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.ModifyUserAccess))) - .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) - { - 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.ModifyUserAccess))) - .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); + await Assert.ThrowsAsync(() => sutProvider.Sut.Invite(organizationAbility.Id, model)); } [Theory] @@ -536,36 +300,6 @@ public async Task GetAccountRecoveryDetails_WithoutManageResetPasswordPermission await Assert.ThrowsAsync(async () => await sutProvider.Sut.GetAccountRecoveryDetails(organizationId, bulkRequestModel)); } - 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.ModifyUserAccess))) - .Returns(AuthorizationResult.Success()); - } - } - private void Get_Setup(OrganizationAbility organizationAbility, ICollection organizationUsers, SutProvider sutProvider) diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommandTests.cs index 45749782f4fa..adbc3c43534e 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommandTests.cs @@ -1,11 +1,14 @@ using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.OrganizationFeatures.Groups; using Bit.Core.AdminConsole.Repositories; +using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Exceptions; using Bit.Core.Models.Data; +using Bit.Core.Repositories; using Bit.Core.Services; using Bit.Core.Test.AutoFixture.OrganizationFixtures; +using Bit.Core.Utilities; using Bit.Test.Common.AutoFixture; using Bit.Test.Common.AutoFixture.Attributes; using Bit.Test.Common.Helpers; @@ -18,10 +21,12 @@ namespace Bit.Core.Test.AdminConsole.OrganizationFeatures.Groups; public class UpdateGroupCommandTests { [Theory, OrganizationCustomize(UseGroups = true), BitAutoData] - public async Task UpdateGroup_Success(SutProvider sutProvider, Group group, Organization organization) + public async Task UpdateGroup_Success(SutProvider sutProvider, Group group, Group oldGroup, + Organization organization) { - // Deprecated with Flexible Collections - group.AccessAll = false; + ArrangeGroup(sutProvider, group, oldGroup); + ArrangeUsers(sutProvider, group); + ArrangeCollections(sutProvider, group); await sutProvider.Sut.UpdateGroupAsync(group, organization); @@ -31,10 +36,12 @@ public async Task UpdateGroup_Success(SutProvider sutProvide } [Theory, OrganizationCustomize(UseGroups = true), BitAutoData] - public async Task UpdateGroup_WithCollections_Success(SutProvider sutProvider, Group group, Organization organization, List collections) + public async Task UpdateGroup_WithCollections_Success(SutProvider sutProvider, Group group, + Group oldGroup, Organization organization, List collections) { - // Deprecated with Flexible Collections - group.AccessAll = false; + ArrangeGroup(sutProvider, group, oldGroup); + ArrangeUsers(sutProvider, group); + ArrangeCollections(sutProvider, group); // Arrange list of collections to make sure Manage is mutually exclusive for (var i = 0; i < collections.Count; i++) @@ -53,10 +60,12 @@ public async Task UpdateGroup_WithCollections_Success(SutProvider sutProvider, Group group, Organization organization, EventSystemUser eventSystemUser) + public async Task UpdateGroup_WithEventSystemUser_Success(SutProvider sutProvider, Group group, + Group oldGroup, Organization organization, EventSystemUser eventSystemUser) { - // Deprecated with Flexible Collections - group.AccessAll = false; + ArrangeGroup(sutProvider, group, oldGroup); + ArrangeUsers(sutProvider, group); + ArrangeCollections(sutProvider, group); await sutProvider.Sut.UpdateGroupAsync(group, organization, eventSystemUser); @@ -66,19 +75,27 @@ public async Task UpdateGroup_WithEventSystemUser_Success(SutProvider sutProvider, Group group, EventSystemUser eventSystemUser) + public async Task UpdateGroup_WithNullOrganization_Throws(SutProvider sutProvider, Group group, + Group oldGroup, EventSystemUser eventSystemUser) { - var exception = await Assert.ThrowsAsync(async () => await sutProvider.Sut.UpdateGroupAsync(group, null, eventSystemUser)); + ArrangeGroup(sutProvider, group, oldGroup); + ArrangeUsers(sutProvider, group); + ArrangeCollections(sutProvider, group); - Assert.Contains("Organization not found", exception.Message); + await Assert.ThrowsAsync(async () => await sutProvider.Sut.UpdateGroupAsync(group, null, eventSystemUser)); await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().CreateAsync(default); await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().LogGroupEventAsync(default, default, default); } [Theory, OrganizationCustomize(UseGroups = false), BitAutoData] - public async Task UpdateGroup_WithUseGroupsAsFalse_Throws(SutProvider sutProvider, Organization organization, Group group, EventSystemUser eventSystemUser) + public async Task UpdateGroup_WithUseGroupsAsFalse_Throws(SutProvider sutProvider, + Organization organization, Group group, Group oldGroup, EventSystemUser eventSystemUser) { + ArrangeGroup(sutProvider, group, oldGroup); + ArrangeUsers(sutProvider, group); + ArrangeCollections(sutProvider, group); + var exception = await Assert.ThrowsAsync(async () => await sutProvider.Sut.UpdateGroupAsync(group, organization, eventSystemUser)); Assert.Contains("This organization cannot use groups", exception.Message); @@ -89,8 +106,12 @@ public async Task UpdateGroup_WithUseGroupsAsFalse_Throws(SutProvider sutProvider, Organization organization, Group group) + SutProvider sutProvider, Organization organization, Group group, Group oldGroup) { + ArrangeGroup(sutProvider, group, oldGroup); + ArrangeUsers(sutProvider, group); + ArrangeCollections(sutProvider, group); + group.AccessAll = true; var exception = @@ -100,4 +121,123 @@ public async Task UpdateGroup_WithAccessAll_Throws( await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().CreateAsync(default); await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().LogGroupEventAsync(default, default, default); } + + + [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = true), BitAutoData] + public async Task UpdateGroup_GroupBelongsToDifferentOrganization_Throws(SutProvider sutProvider, + Group group, Group oldGroup, Organization organization) + { + group.AccessAll = false; + ArrangeGroup(sutProvider, group, oldGroup); + ArrangeUsers(sutProvider, group); + ArrangeCollections(sutProvider, group); + + // Mismatching orgId + oldGroup.OrganizationId = CoreHelpers.GenerateComb(); + + await Assert.ThrowsAsync(() => sutProvider.Sut.UpdateGroupAsync(group, organization)); + } + + [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = true), BitAutoData] + public async Task UpdateGroup_CollectionsBelongsToDifferentOrganization_Throws(SutProvider sutProvider, + Group group, Group oldGroup, Organization organization, List collectionAccess) + { + group.AccessAll = false; + ArrangeGroup(sutProvider, group, oldGroup); + ArrangeUsers(sutProvider, group); + + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(callInfo => callInfo.Arg>() + .Select(guid => new Collection { Id = guid, OrganizationId = CoreHelpers.GenerateComb() }).ToList()); + + await Assert.ThrowsAsync( + () => sutProvider.Sut.UpdateGroupAsync(group, organization, collectionAccess)); + } + + [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = true), BitAutoData] + public async Task UpdateGroup_CollectionsDoNotExist_Throws(SutProvider sutProvider, + Group group, Group oldGroup, Organization organization, List collectionAccess) + { + group.AccessAll = false; + ArrangeGroup(sutProvider, group, oldGroup); + ArrangeUsers(sutProvider, group); + + // Return result is missing a collection + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(callInfo => + { + var result = callInfo.Arg>() + .Select(guid => new Collection { Id = guid, OrganizationId = group.OrganizationId }).ToList(); + result.RemoveAt(0); + return result; + }); + + await Assert.ThrowsAsync( + () => sutProvider.Sut.UpdateGroupAsync(group, organization, collectionAccess)); + } + + [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = true), BitAutoData] + public async Task UpdateGroup_MemberBelongsToDifferentOrganization_Throws(SutProvider sutProvider, + Group group, Group oldGroup, Organization organization, IEnumerable userAccess) + { + group.AccessAll = false; + ArrangeGroup(sutProvider, group, oldGroup); + ArrangeCollections(sutProvider, group); + + sutProvider.GetDependency() + .GetManyAsync(Arg.Any>()) + .Returns(callInfo => callInfo.Arg>() + .Select(guid => new OrganizationUser { Id = guid, OrganizationId = CoreHelpers.GenerateComb() }).ToList()); + + await Assert.ThrowsAsync( + () => sutProvider.Sut.UpdateGroupAsync(group, organization, null, userAccess)); + } + + [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = true), BitAutoData] + public async Task UpdateGroup_MemberDoesNotExist_Throws(SutProvider sutProvider, + Group group, Group oldGroup, Organization organization, IEnumerable userAccess) + { + ArrangeGroup(sutProvider, group, oldGroup); + ArrangeCollections(sutProvider, group); + + sutProvider.GetDependency() + .GetManyAsync(Arg.Any>()) + .Returns(callInfo => + { + var result = callInfo.Arg>() + .Select(guid => new OrganizationUser { Id = guid, OrganizationId = group.OrganizationId }) + .ToList(); + result.RemoveAt(0); + return result; + }); + + await Assert.ThrowsAsync( + () => sutProvider.Sut.UpdateGroupAsync(group, organization, null, userAccess)); + } + + private void ArrangeGroup(SutProvider sutProvider, Group group, Group oldGroup) + { + group.AccessAll = false; + oldGroup.OrganizationId = group.OrganizationId; + oldGroup.Id = group.Id; + sutProvider.GetDependency().GetByIdAsync(group.Id).Returns(oldGroup); + } + + private void ArrangeCollections(SutProvider sutProvider, Group group) + { + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(callInfo => callInfo.Arg>() + .Select(guid => new Collection() { Id = guid, OrganizationId = group.OrganizationId }).ToList()); + } + + private void ArrangeUsers(SutProvider sutProvider, Group group) + { + sutProvider.GetDependency() + .GetManyAsync(Arg.Any>()) + .Returns(callInfo => callInfo.Arg>() + .Select(guid => new OrganizationUser { Id = guid, OrganizationId = group.OrganizationId }).ToList()); + } } diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs index 2216b7896a33..4aed8192adbe 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs @@ -1,6 +1,7 @@ using System.Text.Json; using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers; +using Bit.Core.AdminConsole.Repositories; using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Exceptions; @@ -21,7 +22,7 @@ public class UpdateOrganizationUserCommandTests { [Theory, BitAutoData] public async Task UpdateUserAsync_NoUserId_Throws(OrganizationUser user, Guid? savingUserId, - List collections, IEnumerable groups, SutProvider sutProvider) + List collections, List groups, SutProvider sutProvider) { user.Id = default(Guid); var exception = await Assert.ThrowsAsync( @@ -29,22 +30,106 @@ public async Task UpdateUserAsync_NoUserId_Throws(OrganizationUser user, Guid? s Assert.Contains("invite the user first", exception.Message.ToLowerInvariant()); } + [Theory, BitAutoData] + public async Task UpdateUserAsync_DifferentOrganizationId_Throws(OrganizationUser user, OrganizationUser originalUser, + Guid? savingUserId, SutProvider sutProvider) + { + sutProvider.GetDependency().GetByIdAsync(user.Id).Returns(originalUser); + + await Assert.ThrowsAsync( + () => sutProvider.Sut.UpdateUserAsync(user, savingUserId, null, null)); + } + + [Theory, BitAutoData] + public async Task UpdateUserAsync_CollectionsBelongToDifferentOrganization_Throws(OrganizationUser user, OrganizationUser originalUser, + List collectionAccess, Guid? savingUserId, SutProvider sutProvider, + Organization organization) + { + Setup(sutProvider, organization, user, originalUser); + + // Return collections with different organizationIds from the repository + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(callInfo => callInfo.Arg>() + .Select(guid => new Collection { Id = guid, OrganizationId = CoreHelpers.GenerateComb() }).ToList()); + + await Assert.ThrowsAsync( + () => sutProvider.Sut.UpdateUserAsync(user, savingUserId, collectionAccess, null)); + } + + [Theory, BitAutoData] + public async Task UpdateUserAsync_CollectionsDoNotExist_Throws(OrganizationUser user, OrganizationUser originalUser, + List collectionAccess, Guid? savingUserId, SutProvider sutProvider, + Organization organization) + { + Setup(sutProvider, organization, user, originalUser); + + // Return matching collections, except that 1 is missing + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(callInfo => + { + var result = callInfo.Arg>() + .Select(guid => new Collection { Id = guid, OrganizationId = user.OrganizationId }).ToList(); + result.RemoveAt(0); + return result; + }); + + await Assert.ThrowsAsync( + () => sutProvider.Sut.UpdateUserAsync(user, savingUserId, collectionAccess, null)); + } + + [Theory, BitAutoData] + public async Task UpdateUserAsync_GroupsBelongToDifferentOrganization_Throws(OrganizationUser user, OrganizationUser originalUser, + ICollection groupAccess, Guid? savingUserId, SutProvider sutProvider, + Organization organization) + { + Setup(sutProvider, organization, user, originalUser); + + // Return collections with different organizationIds from the repository + sutProvider.GetDependency() + .GetManyByManyIds(Arg.Any>()) + .Returns(callInfo => callInfo.Arg>() + .Select(guid => new Group { Id = guid, OrganizationId = CoreHelpers.GenerateComb() }).ToList()); + + await Assert.ThrowsAsync( + () => sutProvider.Sut.UpdateUserAsync(user, savingUserId, null, groupAccess)); + } + + [Theory, BitAutoData] + public async Task UpdateUserAsync_GroupsDoNotExist_Throws(OrganizationUser user, OrganizationUser originalUser, + ICollection groupAccess, Guid? savingUserId, SutProvider sutProvider, + Organization organization) + { + Setup(sutProvider, organization, user, originalUser); + + // Return matching collections, except that 1 is missing + sutProvider.GetDependency() + .GetManyByManyIds(Arg.Any>()) + .Returns(callInfo => + { + var result = callInfo.Arg>() + .Select(guid => new Group { Id = guid, OrganizationId = CoreHelpers.GenerateComb() }).ToList(); + result.RemoveAt(0); + return result; + }); + + await Assert.ThrowsAsync( + () => sutProvider.Sut.UpdateUserAsync(user, savingUserId, null, groupAccess)); + } + [Theory, BitAutoData] public async Task UpdateUserAsync_Passes( Organization organization, OrganizationUser oldUserData, OrganizationUser newUserData, List collections, - IEnumerable groups, + List groups, Permissions permissions, [OrganizationUser(type: OrganizationUserType.Owner)] OrganizationUser savingUser, SutProvider sutProvider) { - var organizationRepository = sutProvider.GetDependency(); - var organizationUserRepository = sutProvider.GetDependency(); - var organizationService = sutProvider.GetDependency(); - - organizationRepository.GetByIdAsync(organization.Id).Returns(organization); + Setup(sutProvider, organization, newUserData, oldUserData); // Deprecated with Flexible Collections oldUserData.AccessAll = false; @@ -66,17 +151,20 @@ public async Task UpdateUserAsync_Passes( { PropertyNamingPolicy = JsonNamingPolicy.CamelCase, }); - organizationUserRepository.GetByIdAsync(oldUserData.Id).Returns(oldUserData); - organizationUserRepository.GetManyByOrganizationAsync(savingUser.OrganizationId, OrganizationUserType.Owner) - .Returns(new List { savingUser }); - organizationService - .HasConfirmedOwnersExceptAsync( - newUserData.OrganizationId, - Arg.Is>(i => i.Contains(newUserData.Id))) - .Returns(true); + + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(callInfo => callInfo.Arg>() + .Select(guid => new Collection { Id = guid, OrganizationId = oldUserData.OrganizationId }).ToList()); + + sutProvider.GetDependency() + .GetManyByManyIds(Arg.Any>()) + .Returns(callInfo => callInfo.Arg>() + .Select(guid => new Group { Id = guid, OrganizationId = oldUserData.OrganizationId }).ToList()); await sutProvider.Sut.UpdateUserAsync(newUserData, savingUser.UserId, collections, groups); + var organizationService = sutProvider.GetDependency(); await organizationService.Received(1).ValidateOrganizationUserUpdatePermissions( newUserData.OrganizationId, newUserData.Type, @@ -97,7 +185,7 @@ public async Task UpdateUserAsync_WithAccessAll_Throws( [OrganizationUser(type: OrganizationUserType.User)] OrganizationUser newUserData, [OrganizationUser(type: OrganizationUserType.Owner, status: OrganizationUserStatusType.Confirmed)] OrganizationUser savingUser, List collections, - IEnumerable groups, + List groups, SutProvider sutProvider) { newUserData.Id = oldUserData.Id; @@ -106,6 +194,16 @@ public async Task UpdateUserAsync_WithAccessAll_Throws( newUserData.Permissions = CoreHelpers.ClassToJsonData(new Permissions()); newUserData.AccessAll = true; + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(callInfo => callInfo.Arg>() + .Select(guid => new Collection { Id = guid, OrganizationId = oldUserData.OrganizationId }).ToList()); + + sutProvider.GetDependency() + .GetManyByManyIds(Arg.Any>()) + .Returns(callInfo => callInfo.Arg>() + .Select(guid => new Group { Id = guid, OrganizationId = oldUserData.OrganizationId }).ToList()); + sutProvider.GetDependency() .GetByIdAsync(organization.Id) .Returns(organization); @@ -129,4 +227,24 @@ public async Task UpdateUserAsync_WithAccessAll_Throws( Assert.Contains("the accessall property has been deprecated", exception.Message.ToLowerInvariant()); } + + private void Setup(SutProvider sutProvider, Organization organization, + OrganizationUser newUser, OrganizationUser oldUser) + { + var organizationRepository = sutProvider.GetDependency(); + var organizationUserRepository = sutProvider.GetDependency(); + var organizationService = sutProvider.GetDependency(); + + organizationRepository.GetByIdAsync(organization.Id).Returns(organization); + + newUser.Id = oldUser.Id; + newUser.UserId = oldUser.UserId; + newUser.OrganizationId = oldUser.OrganizationId = organization.Id; + organizationUserRepository.GetByIdAsync(oldUser.Id).Returns(oldUser); + organizationService + .HasConfirmedOwnersExceptAsync( + oldUser.OrganizationId, + Arg.Is>(i => i.Contains(oldUser.Id))) + .Returns(true); + } }