[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
2025-12-17 16:53:23 +00:00 · 2025-08-07 11:12:45 +01:00
parent e61a5cc83a
commit 9d05105dc0
3 changed files with 174 additions and 17 deletions
--- 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<CollectionAccessSelection> collectionAccess)
+    private async Task<List<CollectionAccessSelection>> ValidateAccessAndFilterDefaultUserCollectionsAsync(
+        OrganizationUser originalUser, List<CollectionAccessSelection> 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<CollectionAccessSelection> collectionAccess, ICollection<Collection> collections)
+    {
        var collectionIds = collections.Select(c => c.Id);

        var missingCollection = collectionAccess
@@ -199,12 +207,13 @@ 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<CollectionAccessSelection> ExcludeDefaultUserCollections(
+        List<CollectionAccessSelection> collectionAccess, ICollection<Collection> collections) =>
+            collectionAccess
+                .Where(cas => collections.Any(c => c.Id == cas.Id && c.Type != CollectionType.DefaultUserCollection))
+                .ToList();

    private async Task ValidateGroupAccessAsync(OrganizationUser originalUser,
        ICollection<Guid> groupAccess)
--- 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<ApiApplicationFacto
        await VerifyMultipleUsersHaveDefaultCollectionsAsync(acceptedUsers);
    }

+    [Fact]
+    public async Task Put_WithExistingDefaultCollection_Success()
+    {
+        // Arrange
+        await _loginHelper.LoginAsync(_ownerEmail);
+
+        var (userEmail, organizationUser) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync(_factory,
+            _organization.Id, OrganizationUserType.User);
+
+        var (group, sharedCollection, defaultCollection) = await CreateTestDataAsync();
+        await AssignDefaultCollectionToUserAsync(organizationUser, defaultCollection);
+
+        // Act
+        var updateRequest = CreateUpdateRequest(sharedCollection, group);
+        var httpResponse = await _client.PutAsJsonAsync($"organizations/{_organization.Id}/users/{organizationUser.Id}", updateRequest);
+
+        Assert.Equal(HttpStatusCode.OK, httpResponse.StatusCode);
+
+        // Assert
+        await VerifyUserWasUpdatedCorrectlyAsync(organizationUser, expectedType: OrganizationUserType.Custom, expectedManageGroups: true);
+        await VerifyGroupAccessWasAddedAsync(organizationUser, [group]);
+        await VerifyCollectionAccessWasUpdatedCorrectlyAsync(organizationUser, sharedCollection.Id, defaultCollection.Id);
+    }
+
    public Task DisposeAsync()
    {
        _client.Dispose();
        return Task.CompletedTask;
    }

+    private async Task<(Group group, Collection sharedCollection, Collection defaultCollection)> CreateTestDataAsync()
+    {
+        var groupRepository = _factory.GetService<IGroupRepository>();
+        var group = await groupRepository.CreateAsync(new Group
+        {
+            OrganizationId = _organization.Id,
+            Name = $"Test Group {Guid.NewGuid()}"
+        });
+
+        var collectionRepository = _factory.GetService<ICollectionRepository>();
+        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<IOrganizationUserRepository>();
+        await organizationUserRepository.ReplaceAsync(organizationUser,
+            new List<CollectionAccessSelection>
+            {
+                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<SelectionReadOnlyRequestModel>
+            {
+                new SelectionReadOnlyRequestModel
+                {
+                    Id = sharedCollection.Id,
+                    ReadOnly = true,
+                    HidePasswords = false,
+                    Manage = false
+                }
+            },
+            Groups = new List<Guid> { group.Id }
+        };
+    }
+
+    private async Task VerifyUserWasUpdatedCorrectlyAsync(
+        OrganizationUser organizationUser,
+        OrganizationUserType expectedType,
+        bool expectedManageGroups)
+    {
+        var organizationUserRepository = _factory.GetService<IOrganizationUserRepository>();
+        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<Group> groups)
+    {
+        var groupRepository = _factory.GetService<IGroupRepository>();
+        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<IOrganizationUserRepository>();
+        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<List<OrganizationUser>> CreateAcceptedUsersAsync(IEnumerable<string> emails)
    {
        var acceptedUsers = new List<OrganizationUser>();
--- 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<CollectionAccessSelection> collectionAccess, Guid? savingUserId, SutProvider<UpdateOrganizationUserCommand> sutProvider,
-        Organization organization)
+    public async Task UpdateUserAsync_WithMixedCollectionTypes_FiltersOutDefaultUserCollections(
+        OrganizationUser user, OrganizationUser originalUser, Collection sharedCollection, Collection defaultUserCollection,
+        Guid? savingUserId, SutProvider<UpdateOrganizationUserCommand> 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<CollectionAccessSelection>
+        {
+            new() { Id = sharedCollection.Id, ReadOnly = true, HidePasswords = false, Manage = false },
+            new() { Id = defaultUserCollection.Id, ReadOnly = false, HidePasswords = true, Manage = false }
+        };
+
        sutProvider.GetDependency<ICollectionRepository>()
            .GetManyByManyIdsAsync(Arg.Any<IEnumerable<Guid>>())
-            .Returns(callInfo => callInfo.Arg<IEnumerable<Guid>>()
-                .Select(guid => new Collection { Id = guid, OrganizationId = user.OrganizationId, Type = CollectionType.DefaultUserCollection }).ToList());
+            .Returns(new List<Collection>
+            {
+                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<BadRequestException>(
-            () => 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<IOrganizationUserRepository>().Received(1).ReplaceAsync(
+            user,
+            Arg.Is<IEnumerable<CollectionAccessSelection>>(collections =>
+                collections.Count() == 1 &&
+                collections.First().Id == sharedCollection.Id
+            )
+        );
    }

    private void Setup(SutProvider<UpdateOrganizationUserCommand> sutProvider, Organization organization,