[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 <[email protected]>

Author: BTreston

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,17 +16,22 @@ 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) {
       const usersRequest = users.map((u) => {
         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 r = () => batchRequestBuilder.buildRequest(mockGroups, mockUsers, options);
+
+    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 requests = singleRequestBuilder.buildRequest(mockGroups, mockUsers, true, true);
+    const options = { ...defaultOptions, removeDisabled: true };
+    const requests = batchRequestBuilder.buildRequest(mockGroups, mockUsers, options);
 
-    expect(requests.length).toEqual(1);
+    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,
       }),
     ];