diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs index b72505c195..2623242ad6 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs @@ -89,7 +89,7 @@ public class UpdateOrganizationUserCommand : IUpdateOrganizationUserCommand if (collectionAccessList.Count != 0) { - await ValidateCollectionAccessAsync(originalOrganizationUser, collectionAccessList); + collectionAccessList = await ValidateAccessAndFilterDefaultUserCollectionsAsync(originalOrganizationUser, collectionAccessList); } if (groupAccess?.Any() == true) @@ -179,11 +179,19 @@ public class UpdateOrganizationUserCommand : IUpdateOrganizationUserCommand throw new BadRequestException("User can only be an admin of one free organization."); } - private async Task ValidateCollectionAccessAsync(OrganizationUser originalUser, - ICollection collectionAccess) + private async Task> ValidateAccessAndFilterDefaultUserCollectionsAsync( + OrganizationUser originalUser, List collectionAccess) { var collections = await _collectionRepository .GetManyByManyIdsAsync(collectionAccess.Select(c => c.Id)); + + ValidateCollections(originalUser, collectionAccess, collections); + + return ExcludeDefaultUserCollections(collectionAccess, collections); + } + + private static void ValidateCollections(OrganizationUser originalUser, List collectionAccess, ICollection collections) + { var collectionIds = collections.Select(c => c.Id); var missingCollection = collectionAccess @@ -199,13 +207,14 @@ public class UpdateOrganizationUserCommand : IUpdateOrganizationUserCommand // Use generic error message to avoid enumeration throw new NotFoundException(); } - - if (collections.Any(c => c.Type == CollectionType.DefaultUserCollection)) - { - throw new BadRequestException("You cannot modify member access for collections with the type as DefaultUserCollection."); - } } + private static List ExcludeDefaultUserCollections( + List collectionAccess, ICollection collections) => + collectionAccess + .Where(cas => collections.Any(c => c.Id == cas.Id && c.Type != CollectionType.DefaultUserCollection)) + .ToList(); + private async Task ValidateGroupAccessAsync(OrganizationUser originalUser, ICollection groupAccess) { diff --git a/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUserControllerTests.cs b/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUserControllerTests.cs index c60c2049a1..08ebcf5de0 100644 --- a/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUserControllerTests.cs +++ b/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUserControllerTests.cs @@ -2,8 +2,10 @@ using Bit.Api.AdminConsole.Models.Request.Organizations; using Bit.Api.IntegrationTest.Factories; using Bit.Api.IntegrationTest.Helpers; +using Bit.Api.Models.Request; using Bit.Core; using Bit.Core.AdminConsole.Entities; +using Bit.Core.AdminConsole.Repositories; using Bit.Core.Billing.Enums; using Bit.Core.Entities; using Bit.Core.Enums; @@ -160,12 +162,138 @@ public class OrganizationUserControllerTests : IClassFixture CreateTestDataAsync() + { + var groupRepository = _factory.GetService(); + var group = await groupRepository.CreateAsync(new Group + { + OrganizationId = _organization.Id, + Name = $"Test Group {Guid.NewGuid()}" + }); + + var collectionRepository = _factory.GetService(); + var sharedCollection = await collectionRepository.CreateAsync(new Collection + { + OrganizationId = _organization.Id, + Name = $"Test Collection {Guid.NewGuid()}", + Type = CollectionType.SharedCollection + }); + + var defaultCollection = await collectionRepository.CreateAsync(new Collection + { + OrganizationId = _organization.Id, + Name = $"My Items {Guid.NewGuid()}", + Type = CollectionType.DefaultUserCollection + }); + + return (group, sharedCollection, defaultCollection); + } + + private async Task AssignDefaultCollectionToUserAsync(OrganizationUser organizationUser, Collection defaultCollection) + { + var organizationUserRepository = _factory.GetService(); + await organizationUserRepository.ReplaceAsync(organizationUser, + new List + { + new CollectionAccessSelection + { + Id = defaultCollection.Id, + ReadOnly = false, + HidePasswords = false, + Manage = true + } + }); + } + + private static OrganizationUserUpdateRequestModel CreateUpdateRequest(Collection sharedCollection, Group group) + { + return new OrganizationUserUpdateRequestModel + { + Type = OrganizationUserType.Custom, + Permissions = new Permissions + { + ManageGroups = true + }, + Collections = new List + { + new SelectionReadOnlyRequestModel + { + Id = sharedCollection.Id, + ReadOnly = true, + HidePasswords = false, + Manage = false + } + }, + Groups = new List { group.Id } + }; + } + + private async Task VerifyUserWasUpdatedCorrectlyAsync( + OrganizationUser organizationUser, + OrganizationUserType expectedType, + bool expectedManageGroups) + { + var organizationUserRepository = _factory.GetService(); + var updatedOrgUser = await organizationUserRepository.GetByIdAsync(organizationUser.Id); + Assert.NotNull(updatedOrgUser); + Assert.Equal(expectedType, updatedOrgUser.Type); + Assert.Equal(expectedManageGroups, updatedOrgUser.GetPermissions().ManageGroups); + } + + private async Task VerifyGroupAccessWasAddedAsync( + OrganizationUser organizationUser, IEnumerable groups) + { + var groupRepository = _factory.GetService(); + var userGroups = await groupRepository.GetManyIdsByUserIdAsync(organizationUser.Id); + Assert.All(groups, group => Assert.Contains(group.Id, userGroups)); + } + + private async Task VerifyCollectionAccessWasUpdatedCorrectlyAsync( + OrganizationUser organizationUser, Guid sharedCollectionId, Guid defaultCollectionId) + { + var organizationUserRepository = _factory.GetService(); + var (_, collectionAccess) = await organizationUserRepository.GetByIdWithCollectionsAsync(organizationUser.Id); + var collectionIds = collectionAccess.Select(c => c.Id).ToHashSet(); + + Assert.Contains(defaultCollectionId, collectionIds); + Assert.Contains(sharedCollectionId, collectionIds); + + var newCollectionAccess = collectionAccess.First(c => c.Id == sharedCollectionId); + Assert.True(newCollectionAccess.ReadOnly); + Assert.False(newCollectionAccess.HidePasswords); + Assert.False(newCollectionAccess.Manage); + } + private async Task> CreateAcceptedUsersAsync(IEnumerable emails) { var acceptedUsers = new List(); diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs index bd112c5d40..ed8d3b2346 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs @@ -245,21 +245,41 @@ public class UpdateOrganizationUserCommandTests } [Theory, BitAutoData] - public async Task UpdateUserAsync_WithDefaultUserCollectionType_Throws(OrganizationUser user, OrganizationUser originalUser, - List collectionAccess, Guid? savingUserId, SutProvider sutProvider, - Organization organization) + public async Task UpdateUserAsync_WithMixedCollectionTypes_FiltersOutDefaultUserCollections( + OrganizationUser user, OrganizationUser originalUser, Collection sharedCollection, Collection defaultUserCollection, + Guid? savingUserId, SutProvider sutProvider, Organization organization) { + user.Permissions = null; + sharedCollection.Type = CollectionType.SharedCollection; + defaultUserCollection.Type = CollectionType.DefaultUserCollection; + sharedCollection.OrganizationId = defaultUserCollection.OrganizationId = organization.Id; + Setup(sutProvider, organization, user, originalUser); - // Return collections with DefaultUserCollection type + var collectionAccess = new List + { + new() { Id = sharedCollection.Id, ReadOnly = true, HidePasswords = false, Manage = false }, + new() { Id = defaultUserCollection.Id, ReadOnly = false, HidePasswords = true, Manage = false } + }; + sutProvider.GetDependency() .GetManyByManyIdsAsync(Arg.Any>()) - .Returns(callInfo => callInfo.Arg>() - .Select(guid => new Collection { Id = guid, OrganizationId = user.OrganizationId, Type = CollectionType.DefaultUserCollection }).ToList()); + .Returns(new List + { + new() { Id = sharedCollection.Id, OrganizationId = user.OrganizationId, Type = CollectionType.SharedCollection }, + new() { Id = defaultUserCollection.Id, OrganizationId = user.OrganizationId, Type = CollectionType.DefaultUserCollection } + }); - var exception = await Assert.ThrowsAsync( - () => sutProvider.Sut.UpdateUserAsync(user, OrganizationUserType.User, savingUserId, collectionAccess, null)); - Assert.Contains("You cannot modify member access for collections with the type as DefaultUserCollection.", exception.Message); + await sutProvider.Sut.UpdateUserAsync(user, OrganizationUserType.User, savingUserId, collectionAccess, null); + + // Verify that ReplaceAsync was called with only the shared collection (default user collection filtered out) + await sutProvider.GetDependency().Received(1).ReplaceAsync( + user, + Arg.Is>(collections => + collections.Count() == 1 && + collections.First().Id == sharedCollection.Id + ) + ); } private void Setup(SutProvider sutProvider, Organization organization,