[PM-20140] Prevent accidental bulk removal of users without a Master Password (#6173)

src/Core/AdminConsole/OrganizationFeatures/Import/ImportOrganizationUsersAndGroupsCommand.cs

@@ -1,12 +1,7 @@
-#nullable enable
+using Bit.Core.AdminConsole.Entities;
-
-using Bit.Core.AdminConsole.Entities;
 using Bit.Core.AdminConsole.Models.Business;
 using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces;
-using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.InviteUsers;
 using Bit.Core.AdminConsole.Repositories;
-using Bit.Core.Billing.Pricing;
-using Bit.Core.Context;
 using Bit.Core.Entities;
 using Bit.Core.Enums;
 using Bit.Core.Exceptions;
@@ -17,7 +12,7 @@ using Bit.Core.Models.Data.Organizations.OrganizationUsers;
 using Bit.Core.Repositories;
 using Bit.Core.Services;
 
-namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers;
+namespace Bit.Core.AdminConsole.OrganizationFeatures.Import;
 
 public class ImportOrganizationUsersAndGroupsCommand : IImportOrganizationUsersAndGroupsCommand
 {
@@ -26,10 +21,8 @@ public class ImportOrganizationUsersAndGroupsCommand : IImportOrganizationUsersA
     private readonly IPaymentService _paymentService;
     private readonly IGroupRepository _groupRepository;
     private readonly IEventService _eventService;
-    private readonly ICurrentContext _currentContext;
     private readonly IOrganizationService _organizationService;
-    private readonly IInviteOrganizationUsersCommand _inviteOrganizationUsersCommand;
+    private readonly IFeatureService _featureService;
-    private readonly IPricingClient _pricingClient;
 
     private readonly EventSystemUser _EventSystemUser = EventSystemUser.PublicApi;
 
@@ -38,21 +31,16 @@ public class ImportOrganizationUsersAndGroupsCommand : IImportOrganizationUsersA
             IPaymentService paymentService,
             IGroupRepository groupRepository,
             IEventService eventService,
-            ICurrentContext currentContext,
             IOrganizationService organizationService,
-            IInviteOrganizationUsersCommand inviteOrganizationUsersCommand,
+            IFeatureService featureService)
-            IPricingClient pricingClient
-            )
     {
         _organizationRepository = organizationRepository;
         _organizationUserRepository = organizationUserRepository;
         _paymentService = paymentService;
         _groupRepository = groupRepository;
         _eventService = eventService;
-        _currentContext = currentContext;
         _organizationService = organizationService;
-        _inviteOrganizationUsersCommand = inviteOrganizationUsersCommand;
+        _featureService = featureService;
-        _pricingClient = pricingClient;
     }
 
     /// <summary>
@@ -243,10 +231,23 @@ public class ImportOrganizationUsersAndGroupsCommand : IImportOrganizationUsersA
             List<(OrganizationUserUserDetails ou, EventType e, DateTime? d)> events,
             OrganizationUserImportData importUserData)
     {
-        var usersToDelete = importUserData.ExistingExternalUsers.Where(u =>
+        var usersToDelete = importUserData.ExistingExternalUsers
-            u.Type != OrganizationUserType.Owner &&
+            .Where(u =>
-            !importUserData.ImportedExternalIds.Contains(u.ExternalId) &&
+                u.Type != OrganizationUserType.Owner &&
-            importUserData.ExistingExternalUsersIdDict.ContainsKey(u.ExternalId));
+                !importUserData.ImportedExternalIds.Contains(u.ExternalId) &&
+                importUserData.ExistingExternalUsersIdDict.ContainsKey(u.ExternalId))
+            .ToList();
+
+        if (_featureService.IsEnabled(FeatureFlagKeys.DirectoryConnectorPreventUserRemoval) &&
+            usersToDelete.Any(u => !u.HasMasterPassword))
+        {
+            // Removing users without an MP will put their account in an unrecoverable state.
+            // We allow this during normal syncs for offboarding, but overwriteExisting risks bricking every user in
+            // the organization, so you don't get to do it here.
+            throw new BadRequestException(
+                "Sync failed. To proceed, disable the 'Remove and re-add users during next sync' setting and try again.");
+        }
+
         await _organizationUserRepository.DeleteManyAsync(usersToDelete.Select(u => u.Id));
         events.AddRange(usersToDelete.Select(u => (
           u,

src/Core/Constants.cs

@@ -116,6 +116,7 @@ public static class FeatureFlagKeys
     public const string ImportAsyncRefactor = "pm-22583-refactor-import-async";
     public const string CreateDefaultLocation = "pm-19467-create-default-location";
     public const string MembersGetEndpointOptimization = "pm-23113-optimize-get-members-endpoint";
+    public const string DirectoryConnectorPreventUserRemoval = "pm-24592-directory-connector-prevent-user-removal";
 
     /* Auth Team */
     public const string PM9112DeviceApprovalPersistence = "pm-9112-device-approval-persistence";

src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs

@@ -2,6 +2,7 @@
 using Bit.Core.AdminConsole.OrganizationAuth.Interfaces;
 using Bit.Core.AdminConsole.OrganizationFeatures.Groups;
 using Bit.Core.AdminConsole.OrganizationFeatures.Groups.Interfaces;
+using Bit.Core.AdminConsole.OrganizationFeatures.Import;
 using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationApiKeys;
 using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationApiKeys.Interfaces;
 using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationConnections;

test/Api.IntegrationTest/AdminConsole/Import/ImportOrganizationUsersAndGroupsCommandTests.cs

@@ -26,8 +26,13 @@ public class ImportOrganizationUsersAndGroupsCommandTests : IClassFixture<ApiApp
     {
         _factory = factory;
         _factory.SubstituteService((IFeatureService featureService)
-            => featureService.IsEnabled(FeatureFlagKeys.ImportAsyncRefactor)
+            =>
-                .Returns(true));
+        {
+            featureService.IsEnabled(FeatureFlagKeys.ImportAsyncRefactor)
+                .Returns(true);
+            featureService.IsEnabled(FeatureFlagKeys.DirectoryConnectorPreventUserRemoval)
+                .Returns(true);
+        });
         _client = _factory.CreateClient();
         _loginHelper = new LoginHelper(_factory, _client);
     }
@@ -309,4 +314,29 @@ public class ImportOrganizationUsersAndGroupsCommandTests : IClassFixture<ApiApp
         Assert.Equal("new-name", existingGroupInDb.Name);
         Assert.Equal(existingGroup.ExternalId, existingGroupInDb.ExternalId);
     }
+
+    [Fact]
+    public async Task Import_Remove_Member_Without_Master_Password_Throws_400_Error()
+    {
+        // ARRANGE: a member without a master password
+        await OrganizationTestHelpers.CreateUserWithoutMasterPasswordAsync(_factory, Guid.NewGuid() + "@example.com",
+            _organization.Id);
+
+        // ACT: an import request that would remove that member
+        var request = new OrganizationImportRequestModel
+        {
+            LargeImport = false,
+            OverwriteExisting = true, // removes all members not in the request
+            Groups = [],
+            Members = []
+        };
+
+        var response = await _client.PostAsync($"/public/organization/import", JsonContent.Create(request));
+
+        // ASSERT: that a 400 error is thrown with the correct error message
+        Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode);
+
+        var responseContent = await response.Content.ReadAsStringAsync();
+        Assert.Contains("Sync failed. To proceed, disable the 'Remove and re-add users during next sync' setting and try again.", responseContent);
+    }
 }

test/Api.IntegrationTest/Helpers/OrganizationTestHelpers.cs

@@ -62,7 +62,8 @@ public static class OrganizationTestHelpers
         OrganizationUserType type,
         bool accessSecretsManager = false,
         Permissions? permissions = null,
-        OrganizationUserStatusType userStatusType = OrganizationUserStatusType.Confirmed
+        OrganizationUserStatusType userStatusType = OrganizationUserStatusType.Confirmed,
+        string? externalId = null
     ) where T : class
     {
         var userRepository = factory.GetService<IUserRepository>();
@@ -78,7 +79,7 @@ public static class OrganizationTestHelpers
             Key = null,
             Type = type,
             Status = userStatusType,
-            ExternalId = null,
+            ExternalId = externalId,
             AccessSecretsManager = accessSecretsManager,
             Email = userEmail
         };
@@ -110,7 +111,7 @@ public static class OrganizationTestHelpers
         await factory.LoginWithNewAccount(email);
 
         // Create organizationUser
-        var organizationUser = await OrganizationTestHelpers.CreateUserAsync(factory, organizationId, email, userType,
+        var organizationUser = await CreateUserAsync(factory, organizationId, email, userType,
             permissions: permissions);
 
         return (email, organizationUser);
@@ -168,4 +169,27 @@ public static class OrganizationTestHelpers
 
         await policyRepository.CreateAsync(policy);
     }
+
+    /// <summary>
+    /// Creates a user account without a Master Password and adds them as a member to the specified organization.
+    /// </summary>
+    public static async Task<(User User, OrganizationUser OrganizationUser)> CreateUserWithoutMasterPasswordAsync(ApiApplicationFactory factory, string email, Guid organizationId)
+    {
+        var userRepository = factory.GetService<IUserRepository>();
+        var user = await userRepository.CreateAsync(new User
+        {
+            Email = email,
+            Culture = "en-US",
+            SecurityStamp = "D7ZH62BWAZ5R5CASKULCDDIQGKDA2EJ6",
+            PublicKey = "MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAwMj7W00xS7H0NWasGn7PfEq8VfH3fa5XuZucsKxLLRAHHZk0xGRZJH2lFIznizv3GpF8vzhHhe9VpmMkrdIa5oWhwHpy+D7Z1QCQxuUXzvMKpa95GOntr89nN/mWKpk6abjgjmDcqFJ0lhDqkKnDfes+d8BBd5oEA8p41/Ykz7OfG7AiktVBpTQFW09MQh1NOvcLxVgiUUVRPwNRKrOeCekWDtOjZhASMETv3kI1ogvhHukOQ3ztDzrxvmwnLQ+cXl1EeD8gQnGDp3QLiJqxPgh2EdmANh4IzjRexoDn6BqhRGqLLIoLAbbkoiNrd6NYujrWW0N8KMMoVEXuJL2g4wIDAQAB",
+            PrivateKey = "2.Ytudv+Qk3ET9hN8whqpuGg==|ijsFhmjaf1aaT9uz+IPhVTzMS+2W/ldAP8LdT5VyJaFdx4HSdLcWSZvz5xWuuW94zfv1Qh+p3iQIuZOr29G4jcx47rYtz4ssiFtB7Ia552ZeF+cb7uuVg40CIe7ycuJQITk00o8gots+wFnaEvk0Vjgycnqutm0jpeBJ1joWJWqTVgSsYdUGLu7PiJywQ9NgY4+bJXqadlcviS3rhPKJXtiXYJhqJqSw+vI0Yxp96MJ0HcFJk/LG22YJPTvL5kzuDq/Wzj40kj8blQ+ag+xHD4P/KJ/MppEB3OpDw3UoJ50Ek+YB9pOqGxZtvqMEzBDsgh0yoz1O992UnhaUqtJ5e9Bxy3PA6cJsdyn9npduNOreEb8vePCidN2XC+chjJpPFpjms9muHLKgfaTIfpiJA2Tz8E9dvSyhHHTE1mY+xEA7P08BYKN3LNoSGIjdiZuouJ1V/KZvCssDfVG1tli2qpnhTIh4m3rAMhbM8WW3B7wCV8N0MpcJJSvndkVcMgRbgWcbivLeXuKdE/K98n01RvOLSJyslhLGCGEQQKw6N3HQ2iELfv84YQZi2fjDK+OqAmXDq1pNcjKX2I8dqBwl31tPC8qSZiWnfinwLdqQTvSQjOIyAHb4sSjAwgdMbCRzUTChRr09l+PAZqGWdMC5N2Bw+bA8WP0l2Wdxuv9Abxl3F7xGeAA9Rw9PU5wGKujaMRmO4V9MFjNyyCcw4D9pzKMW6OUKsHsHE7tsG7KskCzksHzrZGawAt0S41BYQA/JwePCrD3F6dM92anlC1LfA00KJb0tmFdU0yJNmJfR+S78yn8yM6wDgIs2cFB3W1fYfpfUvQm+zzPoEQihNxBxnwFsBtMAOtPy54FjSzKmxsQTrYT9E6NFb8k6ZIIm2gNeOPK9OUJgjw+4g2BXErM6ikHTzM3xcaTq/cQaePZ52emndw1qOtdV06hr2EeuLM8frfLHpsknUe8JeYeW5p9E8QdZjjSN9034usdYNamUdxzmn/Mw/ar8z1xSKS6zcaQoTQ7aYLEX3dWJndc4W64HyiaRkLjO6qLUFeOerfz5UvcxxRY89eAA0KLC2xnGkBMOhXxYzIB3lF8Zxqb4JMhoBGw1n31TDfhRDGDHHEAsZuAIcH7aC5RDVxU08Jxmw4oLmeTDZA5BFcqp2A3fusNVZUnfpmMy6DCJyFprlRl8jSlJMAvhbxVuuLFDZnjl77Z2of796Ur6DgmNwYtMPNEntZPIcZ76VPLWAL8lqiRBm20c4qiwr5rNSr5kry9bR1EfXHwFRjy5pxFQ+5+ilpRl8WPfT/iUuORd8J2wnCmghm7uxiJd9t82kX0s6benhL29dQ1etqt5soX2RnlfKan16GVWoI3xrljIQrCAY4xpdptSpglOnrpSClbN1nhGkDfFPNq2pWhQrDbznDknAJ9MxQaVnLYPhn7I849GMd7EvpSkydwQu7QXn9+H4jxn6UEntNGxcL0xkG+xippvZEe+HBvcDD40efDQW1bDbILLjPb4rNRx4d3xaQnVNaF7L33osm5LgfXAQSwHJiURdkU4zmhtPP4zn0br0OdFlR3mPcrkeNeSvs7FxiKtD6n6s+av+4bKjbLL1OyuwmTnMilL6p+m8ldte0yos/r+zOuxWeI=|euhiXWXehYbFQhlAV6LIECSIPCIRaHbNdr9OI4cTPUM=",
+            ApiKey = "CfGrD4MoJu3NprOBZNL8tu5ocmtnmU",
+            KdfIterations = 600000
+        });
+
+        var organizationUser = await CreateUserAsync(factory, organizationId, user.Email,
+            OrganizationUserType.User, externalId: email);
+
+        return (user, organizationUser);
+    }
 }

test/Core.Test/AdminConsole/OrganizationFeatures/Import/ImportOrganizationUsersAndGroupsCommandTests.cs

@@ -1,9 +1,9 @@
 using Bit.Core.AdminConsole.Models.Business;
-using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers;
+using Bit.Core.AdminConsole.OrganizationFeatures.Import;
 using Bit.Core.Auth.Models.Business.Tokenables;
-using Bit.Core.Context;
 using Bit.Core.Entities;
 using Bit.Core.Enums;
+using Bit.Core.Exceptions;
 using Bit.Core.Models.Business;
 using Bit.Core.Models.Data;
 using Bit.Core.Models.Data.Organizations.OrganizationUsers;
@@ -18,11 +18,10 @@ using NSubstitute;
 using Xunit;
 using Organization = Bit.Core.AdminConsole.Entities.Organization;
 
-namespace Bit.Core.Test.OrganizationFeatures.OrganizationUsers;
+namespace Bit.Core.Test.AdminConsole.OrganizationFeatures.Import;
 
 public class ImportOrganizationUsersAndGroupsCommandTests
 {
-
     private readonly IDataProtectorTokenFactory<OrgUserInviteTokenable> _orgUserInviteTokenDataFactory = new FakeDataProtectorTokenFactory<OrgUserInviteTokenable>();
 
     [Theory, PaidOrganizationCustomize, BitAutoData]
@@ -66,7 +65,6 @@ public class ImportOrganizationUsersAndGroupsCommandTests
                 Users = existingUsers.Count,
                 Sponsored = 0
             });
-        sutProvider.GetDependency<ICurrentContext>().ManageUsers(org.Id).Returns(true);
         sutProvider.GetDependency<IOrganizationService>().InviteUsersAsync(org.Id, Guid.Empty, EventSystemUser.PublicApi,
                 Arg.Any<IEnumerable<(OrganizationUserInvite, string)>>())
             .Returns(orgUsers);
@@ -92,6 +90,38 @@ public class ImportOrganizationUsersAndGroupsCommandTests
             .LogOrganizationUserEventsAsync(Arg.Any<IEnumerable<(OrganizationUserUserDetails, EventType, EventSystemUser, DateTime?)>>());
     }
 
+    [Theory, PaidOrganizationCustomize, BitAutoData]
+    public async Task OverwriteExistingUsers_WhenRemovingUserWithoutMasterPassword_Throws(
+            SutProvider<ImportOrganizationUsersAndGroupsCommand> sutProvider,
+            Organization org, List<OrganizationUserUserDetails> existingUsers)
+    {
+        SetupOrganizationConfigForImport(sutProvider, org, existingUsers, []);
+
+        // Existing user does not have a master password
+        sutProvider.GetDependency<IFeatureService>().IsEnabled(FeatureFlagKeys.DirectoryConnectorPreventUserRemoval)
+            .Returns(true);
+        existingUsers.First().HasMasterPassword = false;
+
+        sutProvider.GetDependency<IOrganizationRepository>().GetByIdAsync(org.Id).Returns(org);
+        sutProvider.GetDependency<IOrganizationUserRepository>().GetManyDetailsByOrganizationAsync(org.Id).Returns(existingUsers);
+
+        var exception = await Assert.ThrowsAsync<BadRequestException>(() =>
+            sutProvider.Sut.ImportAsync(org.Id, [], [], [], true));
+
+        Assert.Contains("Sync failed. To proceed, disable the 'Remove and re-add users during next sync' setting and try again.", exception.Message);
+
+        await sutProvider.GetDependency<IOrganizationUserRepository>().DidNotReceiveWithAnyArgs()
+            .UpsertAsync(default);
+        await sutProvider.GetDependency<IOrganizationUserRepository>().DidNotReceiveWithAnyArgs()
+            .UpsertManyAsync(default);
+        await sutProvider.GetDependency<IOrganizationUserRepository>().DidNotReceiveWithAnyArgs()
+            .CreateAsync(default);
+        await sutProvider.GetDependency<IOrganizationService>().DidNotReceiveWithAnyArgs()
+            .InviteUsersAsync(default, default, default, default);
+        await sutProvider.GetDependency<IEventService>().DidNotReceiveWithAnyArgs()
+            .LogOrganizationUserEventsAsync(Arg.Any<IEnumerable<(OrganizationUserUserDetails, EventType, EventSystemUser, DateTime?)>>());
+    }
+
     [Theory, PaidOrganizationCustomize, BitAutoData]
     public async Task OrgImportCreateNewUsersAndMarryExistingUser(
             SutProvider<ImportOrganizationUsersAndGroupsCommand> sutProvider,