[PM-24582] Bugfix: exclude admins and owners from default user collection creation on confirmation (#6177)

* Update the OrganizationUserController integration Confirm tests to handle the Owner type

* Refactor ConfirmOrganizationUserCommand to simplify side-effect handling in organization user confirmation.
Update IPolicyRequirementQuery to return eligible org user IDs for policy enforcement.
Update tests for method signature changes and default collection creation logic.

src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/ConfirmOrganizationUserCommand.cs

@@ -244,42 +244,40 @@ public class ConfirmOrganizationUserCommand : IConfirmOrganizationUserCommand
             .Select(d => d.Id.ToString());
     }
 
-    private async Task<bool> OrganizationRequiresDefaultCollectionAsync(Guid organizationId, string defaultUserCollectionName)
+    /// <summary>
+    /// Handles the side effects of confirming an organization user.
+    /// Creates a default collection for the user if the organization
+    /// has the OrganizationDataOwnership policy enabled.
+    /// </summary>
+    /// <param name="organizationId">The organization ID.</param>
+    /// <param name="confirmedOrganizationUsers">The confirmed organization users.</param>
+    /// <param name="defaultUserCollectionName">The encrypted default user collection name.</param>
+    private async Task HandleConfirmationSideEffectsAsync(Guid organizationId,
+        IEnumerable<OrganizationUser> confirmedOrganizationUsers, string defaultUserCollectionName)
     {
         if (!_featureService.IsEnabled(FeatureFlagKeys.CreateDefaultLocation))
         {
-            return false;
+            return;
         }
 
         // Skip if no collection name provided (backwards compatibility)
         if (string.IsNullOrWhiteSpace(defaultUserCollectionName))
         {
-            return false;
+            return;
         }
 
-        var organizationPolicyRequirement = await _policyRequirementQuery.GetByOrganizationAsync<OrganizationDataOwnershipPolicyRequirement>(organizationId);
+        var policyEligibleOrganizationUserIds = await _policyRequirementQuery.GetManyByOrganizationIdAsync<OrganizationDataOwnershipPolicyRequirement>(organizationId);
 
-        // Check if the organization requires default collections
-        return organizationPolicyRequirement.RequiresDefaultCollection(organizationId);
-    }
+        var eligibleOrganizationUserIds = confirmedOrganizationUsers
+            .Where(ou => policyEligibleOrganizationUserIds.Contains(ou.Id))
+            .Select(ou => ou.Id)
+            .ToList();
 
-    /// <summary>
-    /// Handles the side effects of confirming an organization user.
-    /// Creates a default collection for the user if the organization
-    /// has the OrganizationDataOwnership policy enabled.
-    /// </summary>
-    /// <param name="organizationId">The organization ID.</param>
-    /// <param name="confirmedOrganizationUsers">The confirmed organization users.</param>
-    /// <param name="defaultUserCollectionName">The encrypted default user collection name.</param>
-    private async Task HandleConfirmationSideEffectsAsync(Guid organizationId, IEnumerable<OrganizationUser> confirmedOrganizationUsers, string defaultUserCollectionName)
-    {
-        var requiresDefaultCollections = await OrganizationRequiresDefaultCollectionAsync(organizationId, defaultUserCollectionName);
-        if (!requiresDefaultCollections)
+        if (eligibleOrganizationUserIds.Count == 0)
         {
             return;
         }
 
-        var organizationUserIds = confirmedOrganizationUsers.Select(u => u.Id).ToList();
-        await _collectionRepository.CreateDefaultCollectionsAsync(organizationId, organizationUserIds, defaultUserCollectionName);
+        await _collectionRepository.CreateDefaultCollectionsAsync(organizationId, eligibleOrganizationUserIds, defaultUserCollectionName);
     }
 }

src/Core/AdminConsole/OrganizationFeatures/Policies/IPolicyRequirementQuery.cs

@@ -17,12 +17,11 @@ public interface IPolicyRequirementQuery
     Task<T> GetAsync<T>(Guid userId) where T : IPolicyRequirement;
 
     /// <summary>
-    /// Get a policy requirement for a specific organization.
-    /// This returns the policy requirement that represents the policy state for the entire organization.
-    /// It will always return a value even if there are no policies that should be enforced.
-    /// This should be used for organization-level policy checks.
+    /// Get all organization user IDs within an organization that are affected by a given policy type.
+    /// Respects role/status/provider exemptions via the policy factory's Enforce predicate.
     /// </summary>
-    /// <param name="organizationId">The organization to check policies for.</param>
-    /// <typeparam name="T">The IPolicyRequirement that corresponds to the policy you want to enforce.</typeparam>
-    Task<T> GetByOrganizationAsync<T>(Guid organizationId) where T : IPolicyRequirement;
+    /// <param name="organizationId">The organization to check.</param>
+    /// <typeparam name="T">The IPolicyRequirement that corresponds to the policy type to evaluate.</typeparam>
+    /// <returns>Organization user IDs for whom the policy applies within the organization.</returns>
+    Task<IEnumerable<Guid>> GetManyByOrganizationIdAsync<T>(Guid organizationId) where T : IPolicyRequirement;
 }

src/Core/AdminConsole/OrganizationFeatures/Policies/Implementations/PolicyRequirementQuery.cs

@@ -28,7 +28,8 @@ public class PolicyRequirementQuery(
         return requirement;
     }
 
-    public async Task<T> GetByOrganizationAsync<T>(Guid organizationId) where T : IPolicyRequirement
+    public async Task<IEnumerable<Guid>> GetManyByOrganizationIdAsync<T>(Guid organizationId)
+        where T : IPolicyRequirement
     {
         var factory = factories.OfType<IPolicyRequirementFactory<T>>().SingleOrDefault();
         if (factory is null)
@@ -37,13 +38,14 @@ public class PolicyRequirementQuery(
         }
 
         var organizationPolicyDetails = await GetOrganizationPolicyDetails(organizationId, factory.PolicyType);
-        var filteredPolicies = organizationPolicyDetails
-            .Cast<PolicyDetails>()
-            .Where(policyDetails => policyDetails.PolicyType == factory.PolicyType)
+
+        var eligibleOrganizationUserIds = organizationPolicyDetails
+            .Where(p => p.PolicyType == factory.PolicyType)
             .Where(factory.Enforce)
+            .Select(p => p.OrganizationUserId)
             .ToList();
-        var requirement = factory.Create(filteredPolicies);
-        return requirement;
+
+        return eligibleOrganizationUserIds;
     }
 
     private Task<IEnumerable<PolicyDetails>> GetPolicyDetails(Guid userId)

test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUserControllerTests.cs

@@ -22,7 +22,6 @@ public class OrganizationUserControllerTests : IClassFixture<ApiApplicationFacto
     private static readonly string _mockEncryptedString =
         "2.AOs41Hd8OQiCPXjyJKCiDA==|O6OHgt2U2hJGBSNGnimJmg==|iD33s8B69C8JhYYhSa4V1tArjvLr8eEaGqOV7BRo5Jk=";
 
-
     public OrganizationUserControllerTests(ApiApplicationFactory apiFactory)
     {
         _factory = apiFactory;
@@ -115,7 +114,29 @@ public class OrganizationUserControllerTests : IClassFixture<ApiApplicationFacto
     {
         await OrganizationTestHelpers.EnableOrganizationDataOwnershipPolicyAsync(_factory, _organization.Id);
 
-        var acceptedOrgUser = (await CreateAcceptedUsersAsync(new[] { "test1@bitwarden.com" })).First();
+        var acceptedOrgUser = (await CreateAcceptedUsersAsync(new[] { ("test1@bitwarden.com", OrganizationUserType.User) })).First();
+
+        await _loginHelper.LoginAsync(_ownerEmail);
+
+        var confirmModel = new OrganizationUserConfirmRequestModel
+        {
+            Key = "test-key",
+            DefaultUserCollectionName = _mockEncryptedString
+        };
+        var confirmResponse = await _client.PostAsJsonAsync($"organizations/{_organization.Id}/users/{acceptedOrgUser.Id}/confirm", confirmModel);
+
+        Assert.Equal(HttpStatusCode.OK, confirmResponse.StatusCode);
+
+        await VerifyUserConfirmedAsync(acceptedOrgUser, "test-key");
+        await VerifyDefaultCollectionCountAsync(acceptedOrgUser, 1);
+    }
+
+    [Fact]
+    public async Task Confirm_WithValidOwner_ReturnsSuccess()
+    {
+        await OrganizationTestHelpers.EnableOrganizationDataOwnershipPolicyAsync(_factory, _organization.Id);
+
+        var acceptedOrgUser = (await CreateAcceptedUsersAsync(new[] { ("owner1@bitwarden.com", OrganizationUserType.Owner) })).First();
 
         await _loginHelper.LoginAsync(_ownerEmail);
 
@@ -129,7 +150,7 @@ public class OrganizationUserControllerTests : IClassFixture<ApiApplicationFacto
         Assert.Equal(HttpStatusCode.OK, confirmResponse.StatusCode);
 
         await VerifyUserConfirmedAsync(acceptedOrgUser, "test-key");
-        await VerifyDefaultCollectionCreatedAsync(acceptedOrgUser);
+        await VerifyDefaultCollectionCountAsync(acceptedOrgUser, 0);
     }
 
     [Fact]
@@ -138,8 +159,11 @@ public class OrganizationUserControllerTests : IClassFixture<ApiApplicationFacto
         const string testKeyFormat = "test-key-{0}";
         await OrganizationTestHelpers.EnableOrganizationDataOwnershipPolicyAsync(_factory, _organization.Id);
 
-        var emails = new[] { "test1@example.com", "test2@example.com", "test3@example.com" };
-        var acceptedUsers = await CreateAcceptedUsersAsync(emails);
+        var acceptedUsers = await CreateAcceptedUsersAsync([
+            ("test1@example.com", OrganizationUserType.User),
+            ("test2@example.com", OrganizationUserType.Owner),
+            ("test3@example.com", OrganizationUserType.User)
+        ]);
 
         await _loginHelper.LoginAsync(_ownerEmail);
 
@@ -159,7 +183,9 @@ public class OrganizationUserControllerTests : IClassFixture<ApiApplicationFacto
 
         await VerifyMultipleUsersConfirmedAsync(acceptedUsers.Select((organizationUser, index) =>
             (organizationUser, string.Format(testKeyFormat, index))).ToList());
-        await VerifyMultipleUsersHaveDefaultCollectionsAsync(acceptedUsers);
+        await VerifyDefaultCollectionCountAsync(acceptedUsers.ElementAt(0), 1);
+        await VerifyDefaultCollectionCountAsync(acceptedUsers.ElementAt(1), 0); // Owner does not get a default collection
+        await VerifyDefaultCollectionCountAsync(acceptedUsers.ElementAt(2), 1);
     }
 
     [Fact]
@@ -294,16 +320,18 @@ public class OrganizationUserControllerTests : IClassFixture<ApiApplicationFacto
         Assert.False(newCollectionAccess.Manage);
     }
 
-    private async Task<List<OrganizationUser>> CreateAcceptedUsersAsync(IEnumerable<string> emails)
+    private async Task<List<OrganizationUser>> CreateAcceptedUsersAsync(
+        IEnumerable<(string email, OrganizationUserType userType)> newUsers)
     {
         var acceptedUsers = new List<OrganizationUser>();
 
-        foreach (var email in emails)
+        foreach (var (email, userType) in newUsers)
         {
             await _factory.LoginWithNewAccount(email);
 
-            var acceptedOrgUser = await OrganizationTestHelpers.CreateUserAsync(_factory, _organization.Id, email,
-                OrganizationUserType.User, userStatusType: OrganizationUserStatusType.Accepted);
+            var acceptedOrgUser = await OrganizationTestHelpers.CreateUserAsync(
+                _factory, _organization.Id, email,
+                userType, userStatusType: OrganizationUserStatusType.Accepted);
 
             acceptedUsers.Add(acceptedOrgUser);
         }
@@ -311,12 +339,11 @@ public class OrganizationUserControllerTests : IClassFixture<ApiApplicationFacto
         return acceptedUsers;
     }
 
-    private async Task VerifyDefaultCollectionCreatedAsync(OrganizationUser orgUser)
+    private async Task VerifyDefaultCollectionCountAsync(OrganizationUser orgUser, int expectedCount)
     {
         var collectionRepository = _factory.GetService<ICollectionRepository>();
         var collections = await collectionRepository.GetManyByUserIdAsync(orgUser.UserId!.Value);
-        Assert.Single(collections);
-        Assert.Equal(_mockEncryptedString, collections.First().Name);
+        Assert.Equal(expectedCount, collections.Count);
     }
 
     private async Task VerifyUserConfirmedAsync(OrganizationUser orgUser, string expectedKey)
@@ -334,15 +361,4 @@ public class OrganizationUserControllerTests : IClassFixture<ApiApplicationFacto
             Assert.Equal(acceptedOrganizationUsers[i].key, confirmedUser.Key);
         }
     }
-
-    private async Task VerifyMultipleUsersHaveDefaultCollectionsAsync(List<OrganizationUser> acceptedOrganizationUsers)
-    {
-        var collectionRepository = _factory.GetService<ICollectionRepository>();
-        foreach (var acceptedOrganizationUser in acceptedOrganizationUsers)
-        {
-            var collections = await collectionRepository.GetManyByUserIdAsync(acceptedOrganizationUser.UserId!.Value);
-            Assert.Single(collections);
-            Assert.Equal(_mockEncryptedString, collections.First().Name);
-        }
-    }
 }

test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/ConfirmOrganizationUserCommandTests.cs

@@ -10,7 +10,6 @@ using Bit.Core.Billing.Enums;
 using Bit.Core.Entities;
 using Bit.Core.Enums;
 using Bit.Core.Exceptions;
-using Bit.Core.Models.Data;
 using Bit.Core.Models.Data.Organizations.OrganizationUsers;
 using Bit.Core.Platform.Push;
 using Bit.Core.Repositories;
@@ -473,10 +472,8 @@ public class ConfirmOrganizationUserCommandTests
         sutProvider.GetDependency<IFeatureService>().IsEnabled(FeatureFlagKeys.CreateDefaultLocation).Returns(true);
 
         sutProvider.GetDependency<IPolicyRequirementQuery>()
-            .GetByOrganizationAsync<OrganizationDataOwnershipPolicyRequirement>(organization.Id)
-            .Returns(new OrganizationDataOwnershipPolicyRequirement(
-                OrganizationDataOwnershipState.Enabled,
-                [organization.Id]));
+            .GetManyByOrganizationIdAsync<OrganizationDataOwnershipPolicyRequirement>(organization.Id)
+            .Returns(new List<Guid> { orgUser.Id });
 
         await sutProvider.Sut.ConfirmUserAsync(orgUser.OrganizationId, orgUser.Id, key, confirmingUser.Id, collectionName);
 
@@ -504,17 +501,11 @@ public class ConfirmOrganizationUserCommandTests
 
         sutProvider.GetDependency<IFeatureService>().IsEnabled(FeatureFlagKeys.CreateDefaultLocation).Returns(true);
 
-        sutProvider.GetDependency<IPolicyRequirementQuery>()
-            .GetByOrganizationAsync<OrganizationDataOwnershipPolicyRequirement>(org.Id)
-            .Returns(new OrganizationDataOwnershipPolicyRequirement(
-                OrganizationDataOwnershipState.Enabled,
-                [org.Id]));
-
         await sutProvider.Sut.ConfirmUserAsync(orgUser.OrganizationId, orgUser.Id, key, confirmingUser.Id, "");
 
         await sutProvider.GetDependency<ICollectionRepository>()
             .DidNotReceive()
-            .CreateAsync(Arg.Any<Collection>(), Arg.Any<IEnumerable<CollectionAccessSelection>>(), Arg.Any<IEnumerable<CollectionAccessSelection>>());
+            .CreateDefaultCollectionsAsync(Arg.Any<Guid>(), Arg.Any<IEnumerable<Guid>>(), Arg.Any<string>());
     }
 
     [Theory, BitAutoData]
@@ -533,15 +524,13 @@ public class ConfirmOrganizationUserCommandTests
         sutProvider.GetDependency<IFeatureService>().IsEnabled(FeatureFlagKeys.CreateDefaultLocation).Returns(true);
 
         sutProvider.GetDependency<IPolicyRequirementQuery>()
-            .GetByOrganizationAsync<OrganizationDataOwnershipPolicyRequirement>(org.Id)
-            .Returns(new OrganizationDataOwnershipPolicyRequirement(
-                OrganizationDataOwnershipState.Enabled,
-                [Guid.NewGuid()]));
+            .GetManyByOrganizationIdAsync<OrganizationDataOwnershipPolicyRequirement>(org.Id)
+            .Returns(new List<Guid> { orgUser.UserId!.Value });
 
         await sutProvider.Sut.ConfirmUserAsync(orgUser.OrganizationId, orgUser.Id, key, confirmingUser.Id, collectionName);
 
         await sutProvider.GetDependency<ICollectionRepository>()
             .DidNotReceive()
-            .CreateAsync(Arg.Any<Collection>(), Arg.Any<IEnumerable<CollectionAccessSelection>>(), Arg.Any<IEnumerable<CollectionAccessSelection>>());
+            .CreateDefaultCollectionsAsync(Arg.Any<Guid>(), Arg.Any<IEnumerable<Guid>>(), Arg.Any<string>());
     }
 }

test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyRequirementQueryTests.cs

@@ -81,11 +81,11 @@ public class PolicyRequirementQueryTests
     }
 
     [Theory, BitAutoData]
-    public async Task GetByOrganizationAsync_IgnoresOtherPolicyTypes(Guid organizationId)
+    public async Task GetManyByOrganizationIdAsync_IgnoresOtherPolicyTypes(Guid organizationId)
     {
         var policyRepository = Substitute.For<IPolicyRepository>();
-        var thisPolicy = new OrganizationPolicyDetails { PolicyType = PolicyType.SingleOrg, UserId = Guid.NewGuid() };
-        var otherPolicy = new OrganizationPolicyDetails { PolicyType = PolicyType.RequireSso, UserId = Guid.NewGuid() };
+        var thisPolicy = new OrganizationPolicyDetails { PolicyType = PolicyType.SingleOrg, OrganizationUserId = Guid.NewGuid() };
+        var otherPolicy = new OrganizationPolicyDetails { PolicyType = PolicyType.RequireSso, OrganizationUserId = Guid.NewGuid() };
         // Force the repository to return both policies even though that is not the expected result
         policyRepository.GetPolicyDetailsByOrganizationIdAsync(organizationId, PolicyType.SingleOrg)
             .Returns([thisPolicy, otherPolicy]);
@@ -93,20 +93,20 @@ public class PolicyRequirementQueryTests
         var factory = new TestPolicyRequirementFactory(_ => true);
         var sut = new PolicyRequirementQuery(policyRepository, [factory]);
 
-        var requirement = await sut.GetByOrganizationAsync<TestPolicyRequirement>(organizationId);
+        var organizationUserIds = await sut.GetManyByOrganizationIdAsync<TestPolicyRequirement>(organizationId);
 
         await policyRepository.Received(1).GetPolicyDetailsByOrganizationIdAsync(organizationId, PolicyType.SingleOrg);
 
-        Assert.Contains(thisPolicy, requirement.Policies.Cast<OrganizationPolicyDetails>());
-        Assert.DoesNotContain(otherPolicy, requirement.Policies.Cast<OrganizationPolicyDetails>());
+        Assert.Contains(thisPolicy.OrganizationUserId, organizationUserIds);
+        Assert.DoesNotContain(otherPolicy.OrganizationUserId, organizationUserIds);
     }
 
     [Theory, BitAutoData]
-    public async Task GetByOrganizationAsync_CallsEnforceCallback(Guid organizationId)
+    public async Task GetManyByOrganizationIdAsync_CallsEnforceCallback(Guid organizationId)
     {
         var policyRepository = Substitute.For<IPolicyRepository>();
-        var thisPolicy = new OrganizationPolicyDetails { PolicyType = PolicyType.SingleOrg, UserId = Guid.NewGuid() };
-        var otherPolicy = new OrganizationPolicyDetails { PolicyType = PolicyType.SingleOrg, UserId = Guid.NewGuid() };
+        var thisPolicy = new OrganizationPolicyDetails { PolicyType = PolicyType.SingleOrg, OrganizationUserId = Guid.NewGuid() };
+        var otherPolicy = new OrganizationPolicyDetails { PolicyType = PolicyType.SingleOrg, OrganizationUserId = Guid.NewGuid() };
         policyRepository.GetPolicyDetailsByOrganizationIdAsync(organizationId, PolicyType.SingleOrg).Returns([thisPolicy, otherPolicy]);
 
         var callback = Substitute.For<Func<PolicyDetails, bool>>();
@@ -115,28 +115,28 @@ public class PolicyRequirementQueryTests
         var factory = new TestPolicyRequirementFactory(callback);
         var sut = new PolicyRequirementQuery(policyRepository, [factory]);
 
-        var requirement = await sut.GetByOrganizationAsync<TestPolicyRequirement>(organizationId);
+        var organizationUserIds = await sut.GetManyByOrganizationIdAsync<TestPolicyRequirement>(organizationId);
 
-        Assert.Contains(thisPolicy, requirement.Policies.Cast<OrganizationPolicyDetails>());
-        Assert.DoesNotContain(otherPolicy, requirement.Policies.Cast<OrganizationPolicyDetails>());
+        Assert.Contains(thisPolicy.OrganizationUserId, organizationUserIds);
+        Assert.DoesNotContain(otherPolicy.OrganizationUserId, organizationUserIds);
         callback.Received()(Arg.Is<PolicyDetails>(p => p == thisPolicy));
         callback.Received()(Arg.Is<PolicyDetails>(p => p == otherPolicy));
     }
 
     [Theory, BitAutoData]
-    public async Task GetByOrganizationAsync_ThrowsIfNoFactoryRegistered(Guid organizationId)
+    public async Task GetManyByOrganizationIdAsync_ThrowsIfNoFactoryRegistered(Guid organizationId)
     {
         var policyRepository = Substitute.For<IPolicyRepository>();
         var sut = new PolicyRequirementQuery(policyRepository, []);
 
         var exception = await Assert.ThrowsAsync<NotImplementedException>(()
-            => sut.GetByOrganizationAsync<TestPolicyRequirement>(organizationId));
+            => sut.GetManyByOrganizationIdAsync<TestPolicyRequirement>(organizationId));
 
         Assert.Contains("No Requirement Factory found", exception.Message);
     }
 
     [Theory, BitAutoData]
-    public async Task GetByOrganizationAsync_HandlesNoPolicies(Guid organizationId)
+    public async Task GetManyByOrganizationIdAsync_HandlesNoPolicies(Guid organizationId)
     {
         var policyRepository = Substitute.For<IPolicyRepository>();
         policyRepository.GetPolicyDetailsByOrganizationIdAsync(organizationId, PolicyType.SingleOrg).Returns([]);
@@ -144,8 +144,8 @@ public class PolicyRequirementQueryTests
         var factory = new TestPolicyRequirementFactory(x => x.IsProvider);
         var sut = new PolicyRequirementQuery(policyRepository, [factory]);
 
-        var requirement = await sut.GetByOrganizationAsync<TestPolicyRequirement>(organizationId);
+        var organizationUserIds = await sut.GetManyByOrganizationIdAsync<TestPolicyRequirement>(organizationId);
 
-        Assert.Empty(requirement.Policies);
+        Assert.Empty(organizationUserIds);
     }
 }