From 3573e201a6affb7efc4ec6f1b65fb502cea75e1d Mon Sep 17 00:00:00 2001 From: Brandon Treston Date: Wed, 30 Apr 2025 09:26:15 -0400 Subject: [PATCH] [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 --- .../request/organizationImportRequest.ts | 16 +-- src/abstractions/request-builder.service.ts | 8 +- src/services/batch-request-builder.ts | 17 ++- src/services/batch-requests-builder.spec.ts | 68 ++++++--- src/services/single-request-builder.spec.ts | 79 +++++++++++ src/services/single-request-builder.ts | 9 +- src/services/sync.service.integration.spec.ts | 132 ++++++++++++++++++ src/services/sync.service.spec.ts | 7 +- src/services/sync.service.ts | 28 +--- src/utils/request-builder-helper.ts | 26 ++++ 10 files changed, 322 insertions(+), 68 deletions(-) create mode 100644 src/services/single-request-builder.spec.ts create mode 100644 src/services/sync.service.integration.spec.ts create mode 100644 src/utils/request-builder-helper.ts diff --git a/jslib/common/src/models/request/organizationImportRequest.ts b/jslib/common/src/models/request/organizationImportRequest.ts index feb00cac..a11ecc4b 100644 --- a/jslib/common/src/models/request/organizationImportRequest.ts +++ b/jslib/common/src/models/request/organizationImportRequest.ts @@ -8,16 +8,12 @@ export class OrganizationImportRequest { overwriteExisting = false; largeImport = false; - constructor( - model: - | { - groups: Required[]; - users: Required[]; - overwriteExisting: boolean; - largeImport: boolean; - } - | ImportDirectoryRequest, - ) { + constructor(model: { + groups: Required[]; + users: Required[]; + 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)); diff --git a/src/abstractions/request-builder.service.ts b/src/abstractions/request-builder.service.ts index 1edce99b..d8e8573e 100644 --- a/src/abstractions/request-builder.service.ts +++ b/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[]; } diff --git a/src/services/batch-request-builder.ts b/src/services/batch-request-builder.ts index 7034642d..a782b56c 100644 --- a/src/services/batch-request-builder.ts +++ b/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); } diff --git a/src/services/batch-requests-builder.spec.ts b/src/services/batch-requests-builder.spec.ts index 4cd0c1c8..79cd80cf 100644 --- a/src/services/batch-requests-builder.spec.ts +++ b/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([]); }); diff --git a/src/services/single-request-builder.spec.ts b/src/services/single-request-builder.spec.ts new file mode 100644 index 00000000..20eba07b --- /dev/null +++ b/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); + }); +}); diff --git a/src/services/single-request-builder.ts b/src/services/single-request-builder.ts index 4026d93e..749704e0 100644 --- a/src/services/single-request-builder.ts +++ b/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, }), ]; diff --git a/src/services/sync.service.integration.spec.ts b/src/services/sync.service.integration.spec.ts new file mode 100644 index 00000000..39cd6ac2 --- /dev/null +++ b/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; + let i18nService: MockProxy; + let stateService: MockProxy; + let cryptoFunctionService: MockProxy; + let apiService: MockProxy; + let messagingService: MockProxy; + let environmentService: MockProxy; + let directoryFactory: MockProxy; + + 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; + }); + }); +}); diff --git a/src/services/sync.service.spec.ts b/src/services/sync.service.spec.ts index 5f67346c..b9ac26b0 100644 --- a/src/services/sync.service.spec.ts +++ b/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 diff --git a/src/services/sync.service.ts b/src/services/sync.service.ts index ca46c5f0..8e3172fc 100644 --- a/src/services/sync.service.ts +++ b/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); } } diff --git a/src/utils/request-builder-helper.ts b/src/utils/request-builder-helper.ts new file mode 100644 index 00000000..f83c3321 --- /dev/null +++ b/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; +}