[PM-20134] Fix overwriteExisting and largeImport causing users to be deleted (#737)

* Fix mixed up bools, use whole object

* disallow overwriteExisting on large syncs

* remove unused file

* add test, always set overwriteExisting to false for batched requests

* add more tests

* wip

* Clean up

---------

Co-authored-by: Thomas Rittson <trittson@bitwarden.com>

jslib/common/src/models/request/organizationImportRequest.ts

@@ -8,16 +8,12 @@ export class OrganizationImportRequest {
   overwriteExisting = false;
   largeImport = false;
 
-  constructor(
-    model:
-      | {
-          groups: Required<OrganizationImportGroupRequest>[];
-          users: Required<OrganizationImportMemberRequest>[];
-          overwriteExisting: boolean;
-          largeImport: boolean;
-        }
-      | ImportDirectoryRequest,
-  ) {
+  constructor(model: {
+    groups: Required<OrganizationImportGroupRequest>[];
+    users: Required<OrganizationImportMemberRequest>[];
+    overwriteExisting: boolean;
+    largeImport: boolean;
+  }) {
     if (model instanceof ImportDirectoryRequest) {
       this.groups = model.groups.map((g) => new OrganizationImportGroupRequest(g));
       this.members = model.users.map((u) => new OrganizationImportMemberRequest(u));

src/abstractions/request-builder.service.ts

@@ -3,11 +3,15 @@ import { OrganizationImportRequest } from "@/jslib/common/src/models/request/org
 import { GroupEntry } from "@/src/models/groupEntry";
 import { UserEntry } from "@/src/models/userEntry";
 
+export interface RequestBuilderOptions {
+  removeDisabled: boolean;
+  overwriteExisting: boolean;
+}
+
 export abstract class RequestBuilder {
   buildRequest: (
     groups: GroupEntry[],
     users: UserEntry[],
-    removeDisabled: boolean,
-    overwriteExisting: boolean,
+    options: RequestBuilderOptions,
   ) => OrganizationImportRequest[];
 }

src/services/batch-request-builder.ts

@@ -3,7 +3,7 @@ import { OrganizationImportRequest } from "@/jslib/common/src/models/request/org
 import { GroupEntry } from "@/src/models/groupEntry";
 import { UserEntry } from "@/src/models/userEntry";
 
-import { RequestBuilder } from "../abstractions/request-builder.service";
+import { RequestBuilder, RequestBuilderOptions } from "../abstractions/request-builder.service";
 
 import { batchSize } from "./sync.service";
 
@@ -16,9 +16,14 @@ export class BatchRequestBuilder implements RequestBuilder {
   buildRequest(
     groups: GroupEntry[],
     users: UserEntry[],
-    removeDisabled: boolean,
-    overwriteExisting: boolean,
+    options: RequestBuilderOptions,
   ): OrganizationImportRequest[] {
+    if (options.overwriteExisting) {
+      throw new Error(
+        "You cannot use the 'Remove and re-add organization users during the next sync' option with large imports.",
+      );
+    }
+
     const requests: OrganizationImportRequest[] = [];
 
     if (users.length > 0) {
@@ -26,7 +31,7 @@ export class BatchRequestBuilder implements RequestBuilder {
         return {
           email: u.email,
           externalId: u.externalId,
-          deleted: u.deleted || (removeDisabled && u.disabled),
+          deleted: u.deleted || (options.removeDisabled && u.disabled),
         };
       });
 
@@ -37,7 +42,7 @@ export class BatchRequestBuilder implements RequestBuilder {
           groups: [],
           users: u,
           largeImport: true,
-          overwriteExisting,
+          overwriteExisting: false,
         });
         requests.push(req);
       }
@@ -59,7 +64,7 @@ export class BatchRequestBuilder implements RequestBuilder {
           groups: g,
           users: [],
           largeImport: true,
-          overwriteExisting,
+          overwriteExisting: false,
         });
         requests.push(req);
       }

src/services/batch-requests-builder.spec.ts

@@ -1,46 +1,74 @@
-import { GroupEntry } from "@/src/models/groupEntry";
+import { GetUniqueString } from "@/jslib/common/spec/utils";
+
 import { UserEntry } from "@/src/models/userEntry";
 
+import { RequestBuilderOptions } from "../abstractions/request-builder.service";
+import { groupSimulator, userSimulator } from "../utils/request-builder-helper";
+
 import { BatchRequestBuilder } from "./batch-request-builder";
-import { SingleRequestBuilder } from "./single-request-builder";
 
 describe("BatchRequestBuilder", () => {
   let batchRequestBuilder: BatchRequestBuilder;
-  let singleRequestBuilder: SingleRequestBuilder;
-
-  function userSimulator(userCount: number) {
-    return Array(userCount).fill(new UserEntry());
-  }
-
-  function groupSimulator(groupCount: number) {
-    return Array(groupCount).fill(new GroupEntry());
-  }
 
   beforeEach(async () => {
     batchRequestBuilder = new BatchRequestBuilder();
-    singleRequestBuilder = new SingleRequestBuilder();
+  });
+
+  const defaultOptions: RequestBuilderOptions = Object.freeze({
+    overwriteExisting: false,
+    removeDisabled: false,
   });
 
   it("BatchRequestBuilder batches requests for > 2000 users", () => {
     const mockGroups = groupSimulator(11000);
     const mockUsers = userSimulator(11000);
-
-    const requests = batchRequestBuilder.buildRequest(mockGroups, mockUsers, true, true);
+    const requests = batchRequestBuilder.buildRequest(mockGroups, mockUsers, defaultOptions);
 
     expect(requests.length).toEqual(12);
   });
 
-  it("SingleRequestBuilder returns single request for 200 users", () => {
-    const mockGroups = groupSimulator(200);
-    const mockUsers = userSimulator(200);
+  it("BatchRequestBuilder throws error when overwriteExisting is true", () => {
+    const mockGroups = groupSimulator(11000);
+    const mockUsers = userSimulator(11000);
+    const options = { ...defaultOptions, overwriteExisting: true };
 
-    const requests = singleRequestBuilder.buildRequest(mockGroups, mockUsers, true, true);
+    const r = () => batchRequestBuilder.buildRequest(mockGroups, mockUsers, options);
 
-    expect(requests.length).toEqual(1);
+    expect(r).toThrow(
+      "You cannot use the 'Remove and re-add organization users during the next sync' option with large imports.",
+    );
+  });
+
+  it("BatchRequestBuilder returns requests with deleted users when removeDisabled is true", () => {
+    const mockGroups = groupSimulator(11000);
+    const mockUsers = userSimulator(11000);
+
+    const disabledUser1 = new UserEntry();
+    const disabledUserEmail1 = GetUniqueString() + "@email.com";
+
+    const disabledUser2 = new UserEntry();
+    const disabledUserEmail2 = GetUniqueString() + "@email.com";
+
+    disabledUser1.disabled = true;
+    disabledUser1.email = disabledUserEmail1;
+    disabledUser2.disabled = true;
+    disabledUser2.email = disabledUserEmail2;
+
+    mockUsers[0] = disabledUser1;
+    mockUsers.push(disabledUser2);
+
+    const options = { ...defaultOptions, removeDisabled: true };
+    const requests = batchRequestBuilder.buildRequest(mockGroups, mockUsers, options);
+
+    expect(requests[0].members).toContainEqual({ email: disabledUserEmail1, deleted: true });
+    expect(requests[1].members.find((m) => m.deleted)).toBeUndefined();
+    expect(requests[3].members.find((m) => m.deleted)).toBeUndefined();
+    expect(requests[4].members.find((m) => m.deleted)).toBeUndefined();
+    expect(requests[5].members).toContainEqual({ email: disabledUserEmail2, deleted: true });
   });
 
   it("BatchRequestBuilder retuns an empty array when there are no users or groups", () => {
-    const requests = batchRequestBuilder.buildRequest([], [], true, true);
+    const requests = batchRequestBuilder.buildRequest([], [], defaultOptions);
 
     expect(requests).toEqual([]);
   });

src/services/single-request-builder.spec.ts

@@ -0,0 +1,79 @@
+import { GetUniqueString } from "@/jslib/common/spec/utils";
+
+import { UserEntry } from "@/src/models/userEntry";
+
+import { RequestBuilderOptions } from "../abstractions/request-builder.service";
+import { groupSimulator, userSimulator } from "../utils/request-builder-helper";
+
+import { SingleRequestBuilder } from "./single-request-builder";
+
+describe("SingleRequestBuilder", () => {
+  let singleRequestBuilder: SingleRequestBuilder;
+
+  beforeEach(async () => {
+    singleRequestBuilder = new SingleRequestBuilder();
+  });
+
+  const defaultOptions: RequestBuilderOptions = Object.freeze({
+    overwriteExisting: false,
+    removeDisabled: false,
+  });
+
+  it("SingleRequestBuilder returns single request for 200 users", () => {
+    const mockGroups = groupSimulator(200);
+    const mockUsers = userSimulator(200);
+
+    const requests = singleRequestBuilder.buildRequest(mockGroups, mockUsers, defaultOptions);
+
+    expect(requests.length).toEqual(1);
+  });
+
+  it("SingleRequestBuilder returns request with overwriteExisting enabled", () => {
+    const mockGroups = groupSimulator(200);
+    const mockUsers = userSimulator(200);
+
+    const options = { ...defaultOptions, overwriteExisting: true };
+    const request = singleRequestBuilder.buildRequest(mockGroups, mockUsers, options)[0];
+
+    expect(request.overwriteExisting).toBe(true);
+  });
+
+  it("SingleRequestBuilder returns request with deleted user when removeDisabled is true", () => {
+    const mockGroups = groupSimulator(200);
+    const mockUsers = userSimulator(200);
+
+    const disabledUser = new UserEntry();
+    const disabledUserEmail = GetUniqueString() + "@example.com";
+    disabledUser.disabled = true;
+    disabledUser.email = disabledUserEmail;
+    mockUsers.push(disabledUser);
+
+    const options = { ...defaultOptions, removeDisabled: true };
+    const request = singleRequestBuilder.buildRequest(mockGroups, mockUsers, options)[0];
+
+    expect(request.members.length).toEqual(201);
+    expect(request.members.pop()).toEqual(
+      expect.objectContaining({ email: disabledUserEmail, deleted: true }),
+    );
+    expect(request.overwriteExisting).toBe(false);
+  });
+
+  it("SingleRequestBuilder returns request with deleted user and overwriteExisting enabled when overwriteExisting and removeDisabled are true", () => {
+    const mockGroups = groupSimulator(200);
+    const mockUsers = userSimulator(200);
+
+    const disabledUser = new UserEntry();
+    const disabledUserEmail = GetUniqueString() + "@example.com";
+    disabledUser.disabled = true;
+    disabledUser.email = disabledUserEmail;
+    mockUsers.push(disabledUser);
+
+    const options = { overwriteExisting: true, removeDisabled: true };
+    const request = singleRequestBuilder.buildRequest(mockGroups, mockUsers, options)[0];
+
+    expect(request.members.pop()).toEqual(
+      expect.objectContaining({ email: disabledUserEmail, deleted: true }),
+    );
+    expect(request.overwriteExisting).toBe(true);
+  });
+});

src/services/single-request-builder.ts

@@ -3,7 +3,7 @@ import { OrganizationImportRequest } from "@/jslib/common/src/models/request/org
 import { GroupEntry } from "@/src/models/groupEntry";
 import { UserEntry } from "@/src/models/userEntry";
 
-import { RequestBuilder } from "../abstractions/request-builder.service";
+import { RequestBuilder, RequestBuilderOptions } from "../abstractions/request-builder.service";
 
 /**
  * This class is responsible for building small (<2k users) syncs as a single
@@ -15,8 +15,7 @@ export class SingleRequestBuilder implements RequestBuilder {
   buildRequest(
     groups: GroupEntry[],
     users: UserEntry[],
-    removeDisabled: boolean,
-    overwriteExisting: boolean,
+    options: RequestBuilderOptions,
   ): OrganizationImportRequest[] {
     return [
       new OrganizationImportRequest({
@@ -31,10 +30,10 @@ export class SingleRequestBuilder implements RequestBuilder {
           return {
             email: u.email,
             externalId: u.externalId,
-            deleted: u.deleted || (removeDisabled && u.disabled),
+            deleted: u.deleted || (options.removeDisabled && u.disabled),
           };
         }),
-        overwriteExisting: overwriteExisting,
+        overwriteExisting: options.overwriteExisting,
         largeImport: false,
       }),
     ];

src/services/sync.service.integration.spec.ts

@@ -0,0 +1,132 @@
+import { mock, MockProxy } from "jest-mock-extended";
+
+import { ApiService } from "@/jslib/common/src/abstractions/api.service";
+import { CryptoFunctionService } from "@/jslib/common/src/abstractions/cryptoFunction.service";
+import { MessagingService } from "@/jslib/common/src/abstractions/messaging.service";
+import { EnvironmentService } from "@/jslib/common/src/services/environment.service";
+
+import { I18nService } from "../../jslib/common/src/abstractions/i18n.service";
+import { LogService } from "../../jslib/common/src/abstractions/log.service";
+import { groupFixtures } from "../../openldap/group-fixtures";
+import { userFixtures } from "../../openldap/user-fixtures";
+import { DirectoryFactoryService } from "../abstractions/directory-factory.service";
+import { DirectoryType } from "../enums/directoryType";
+import { getLdapConfiguration, getSyncConfiguration } from "../utils/test-fixtures";
+
+import { BatchRequestBuilder } from "./batch-request-builder";
+import { LdapDirectoryService } from "./ldap-directory.service";
+import { SingleRequestBuilder } from "./single-request-builder";
+import { StateService } from "./state.service";
+import { SyncService } from "./sync.service";
+import * as constants from "./sync.service";
+
+describe("SyncService", () => {
+  let logService: MockProxy<LogService>;
+  let i18nService: MockProxy<I18nService>;
+  let stateService: MockProxy<StateService>;
+  let cryptoFunctionService: MockProxy<CryptoFunctionService>;
+  let apiService: MockProxy<ApiService>;
+  let messagingService: MockProxy<MessagingService>;
+  let environmentService: MockProxy<EnvironmentService>;
+  let directoryFactory: MockProxy<DirectoryFactoryService>;
+
+  let batchRequestBuilder: BatchRequestBuilder;
+  let singleRequestBuilder: SingleRequestBuilder;
+  let syncService: SyncService;
+  let directoryService: LdapDirectoryService;
+
+  const originalBatchSize = constants.batchSize;
+
+  beforeEach(() => {
+    logService = mock();
+    i18nService = mock();
+    stateService = mock();
+    cryptoFunctionService = mock();
+    apiService = mock();
+    messagingService = mock();
+    environmentService = mock();
+    directoryFactory = mock();
+
+    stateService.getDirectoryType.mockResolvedValue(DirectoryType.Ldap);
+    stateService.getOrganizationId.mockResolvedValue("fakeId");
+
+    directoryService = new LdapDirectoryService(logService, i18nService, stateService);
+    directoryFactory.createService.mockReturnValue(directoryService);
+
+    batchRequestBuilder = new BatchRequestBuilder();
+    singleRequestBuilder = new SingleRequestBuilder();
+
+    syncService = new SyncService(
+      cryptoFunctionService,
+      apiService,
+      messagingService,
+      i18nService,
+      environmentService,
+      stateService,
+      batchRequestBuilder,
+      singleRequestBuilder,
+      directoryFactory,
+    );
+  });
+
+  describe("OpenLdap integration: ", () => {
+    it("with largeImport disabled matches directory fixture data", async () => {
+      stateService.getDirectory
+        .calledWith(DirectoryType.Ldap)
+        .mockResolvedValue(getLdapConfiguration());
+      stateService.getSync.mockResolvedValue(
+        getSyncConfiguration({
+          users: true,
+          groups: true,
+          largeImport: false,
+          overwriteExisting: false,
+        }),
+      );
+
+      cryptoFunctionService.hash.mockResolvedValue(new ArrayBuffer(1));
+      // This arranges the last hash to be differet from the ArrayBuffer after it is converted to b64
+      stateService.getLastSyncHash.mockResolvedValue("unique hash");
+
+      const syncResult = await syncService.sync(false, false);
+
+      expect(syncResult).toEqual([groupFixtures, userFixtures]);
+
+      expect(apiService.postPublicImportDirectory).toHaveBeenCalledWith(
+        expect.objectContaining({ overwriteExisting: false }),
+      );
+      expect(apiService.postPublicImportDirectory).toHaveBeenCalledTimes(1);
+    });
+
+    it("with largeImport enabled matches directory fixture data", async () => {
+      stateService.getDirectory
+        .calledWith(DirectoryType.Ldap)
+        .mockResolvedValue(getLdapConfiguration());
+      stateService.getSync.mockResolvedValue(
+        getSyncConfiguration({
+          users: true,
+          groups: true,
+          largeImport: true,
+          overwriteExisting: false,
+        }),
+      );
+
+      cryptoFunctionService.hash.mockResolvedValue(new ArrayBuffer(1));
+      // This arranges the last hash to be differet from the ArrayBuffer after it is converted to b64
+      stateService.getLastSyncHash.mockResolvedValue("unique hash");
+
+      // @ts-expect-error This is a workaround to make the batchsize smaller to trigger the batching logic since its a const.
+      constants.batchSize = 4;
+
+      const syncResult = await syncService.sync(false, false);
+
+      expect(syncResult).toEqual([groupFixtures, userFixtures]);
+      expect(apiService.postPublicImportDirectory).toHaveBeenCalledWith(
+        expect.objectContaining({ overwriteExisting: false }),
+      );
+      expect(apiService.postPublicImportDirectory).toHaveBeenCalledTimes(6);
+
+      // @ts-expect-error Reset batch size to original state.
+      constants.batchSize = originalBatchSize;
+    });
+  });
+});

src/services/sync.service.spec.ts

@@ -34,6 +34,8 @@ describe("SyncService", () => {
 
   let syncService: SyncService;
 
+  const originalBatchSize = constants.batchSize;
+
   beforeEach(() => {
     cryptoFunctionService = mock();
     apiService = mock();
@@ -115,11 +117,12 @@ describe("SyncService", () => {
     expect(apiService.postPublicImportDirectory).toHaveBeenCalledWith(mockRequests[3]);
     expect(apiService.postPublicImportDirectory).toHaveBeenCalledWith(mockRequests[4]);
     expect(apiService.postPublicImportDirectory).toHaveBeenCalledWith(mockRequests[5]);
+
+    // @ts-expect-error Reset batch size back to original value.
+    constants.batchSize = originalBatchSize;
   });
 
   it("does not post for the same hash", async () => {
-    // @ts-expect-error this sets the batch size back to its expexted value for this test.
-    constants.batchSize = 2000;
     stateService.getSync.mockResolvedValue(getSyncConfiguration({ groups: true, users: true }));
     cryptoFunctionService.hash.mockResolvedValue(new ArrayBuffer(1));
     // This arranges the last hash to be the same as the ArrayBuffer after it is converted to b64

src/services/sync.service.ts

@@ -83,13 +83,7 @@ export class SyncService {
         return [groups, users];
       }
 
-      const reqs = this.buildRequest(
-        groups,
-        users,
-        syncConfig.removeDisabled,
-        syncConfig.overwriteExisting,
-        syncConfig.largeImport,
-      );
+      const reqs = this.buildRequest(groups, users, syncConfig);
 
       const result: HashResult = await this.generateHash(reqs);
 
@@ -219,24 +213,12 @@ export class SyncService {
   private buildRequest(
     groups: GroupEntry[],
     users: UserEntry[],
-    removeDisabled: boolean,
-    overwriteExisting: boolean,
-    largeImport = false,
+    syncConfig: SyncConfiguration,
   ): OrganizationImportRequest[] {
-    if (largeImport && groups.length + users.length > batchSize) {
-      return this.batchRequestBuilder.buildRequest(
-        groups,
-        users,
-        overwriteExisting,
-        removeDisabled,
-      );
+    if (syncConfig.largeImport && groups.length + users.length > batchSize) {
+      return this.batchRequestBuilder.buildRequest(groups, users, syncConfig);
     } else {
-      return this.singleRequestBuilder.buildRequest(
-        groups,
-        users,
-        overwriteExisting,
-        removeDisabled,
-      );
+      return this.singleRequestBuilder.buildRequest(groups, users, syncConfig);
     }
   }
 

src/utils/request-builder-helper.ts

@@ -0,0 +1,26 @@
+import { GetUniqueString } from "@/jslib/common/spec/utils";
+
+import { GroupEntry } from "../models/groupEntry";
+import { UserEntry } from "../models/userEntry";
+
+export function userSimulator(userCount: number): UserEntry[] {
+  const users: UserEntry[] = [];
+  while (userCount > 0) {
+    const userEntry = new UserEntry();
+    userEntry.email = GetUniqueString() + "@example.com";
+    users.push(userEntry);
+    userCount--;
+  }
+  return users;
+}
+
+export function groupSimulator(groupCount: number): GroupEntry[] {
+  const groups: GroupEntry[] = [];
+  while (groupCount > 0) {
+    const groupEntry = new GroupEntry();
+    groupEntry.name = GetUniqueString();
+    groups.push(groupEntry);
+    groupCount--;
+  }
+  return groups;
+}