From 9d05105dc0a6c06b381d46be1a5d228b97fe9297 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rui=20Tom=C3=A9?= <108268980+r-tome@users.noreply.github.com> Date: Thu, 7 Aug 2025 11:12:45 +0100 Subject: [PATCH] [PM-23981] Fix DefaultUserCollection filtering in organization user updates (#6161) * Refactor UpdateOrganizationUserCommand to validate and filter out DefaultUserCollections during user updates. * Enhance UpdateOrganizationUserCommandTests to filter out DefaultUserCollections during user updates, ensuring only shared collections are processed. Updated test logic to reflect new filtering behavior. * Add integration test for updating organization user with existing default collection. The test verifies successful updates to user permissions, group access, and collection access, ensuring correct handling of shared and default collections. * Refactor UpdateOrganizationUserCommand to separate the collection validation and DefaultUserCollection filtering * Refactored integration test setup/assertion for clarity --- .../UpdateOrganizationUserCommand.cs | 25 ++-- .../OrganizationUserControllerTests.cs | 128 ++++++++++++++++++ .../UpdateOrganizationUserCommandTests.cs | 38 ++++-- 3 files changed, 174 insertions(+), 17 deletions(-) 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,