Refactor error handling in LdapDirectoryService to ensure proper unbinding and error propagation

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

.github/PULL_REQUEST_TEMPLATE.md

@@ -9,26 +9,3 @@
 ## 📸 Screenshots
 
 <!-- Required for any UI changes; delete if not applicable. Use fixed width images for better display. -->
-
-## ⏰ Reminders before review
-
-- Contributor guidelines followed
-- All formatters and local linters executed and passed
-- Written new unit and / or integration tests where applicable
-- Used internationalization (i18n) for all UI strings
-- CI builds passed
-- Communicated to DevOps any deployment requirements
-- Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team
-
-## 🦮 Reviewer guidelines
-
-<!-- Suggested interactions but feel free to use (or not) as you desire! -->
-
-- 👍 (`:+1:`) or similar for great changes
-- 📝 (`:memo:`) or ℹ️ (`:information_source:`) for notes or general info
-- ❓ (`:question:`) for questions
-- 🤔 (`:thinking:`) or 💭 (`:thought_balloon:`) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
-- 🎨 (`:art:`) for suggestions / improvements
-- ❌ (`:x:`) or ⚠️ (`:warning:`) for more significant problems or concerns needing attention
-- 🌱 (`:seedling:`) or ♻️ (`:recycle:`) for future improvements or indications of technical debt
-- ⛏ (`:pick:`) for minor or nitpick changes

jslib/common/spec/domain/encString.spec.ts

@@ -1,195 +0,0 @@
-import { Substitute, Arg } from "@fluffy-spoon/substitute";
-
-import { CryptoService } from "@/jslib/common/src/abstractions/crypto.service";
-import { EncryptionType } from "@/jslib/common/src/enums/encryptionType";
-import { EncString } from "@/jslib/common/src/models/domain/encString";
-import { SymmetricCryptoKey } from "@/jslib/common/src/models/domain/symmetricCryptoKey";
-import { ContainerService } from "@/jslib/common/src/services/container.service";
-
-describe("EncString", () => {
-  afterEach(() => {
-    (window as any).bitwardenContainerService = undefined;
-  });
-
-  describe("Rsa2048_OaepSha256_B64", () => {
-    it("constructor", () => {
-      const encString = new EncString(EncryptionType.Rsa2048_OaepSha256_B64, "data");
-
-      expect(encString).toEqual({
-        data: "data",
-        encryptedString: "3.data",
-        encryptionType: 3,
-      });
-    });
-
-    describe("parse existing", () => {
-      it("valid", () => {
-        const encString = new EncString("3.data");
-
-        expect(encString).toEqual({
-          data: "data",
-          encryptedString: "3.data",
-          encryptionType: 3,
-        });
-      });
-
-      it("invalid", () => {
-        const encString = new EncString("3.data|test");
-
-        expect(encString).toEqual({
-          encryptedString: "3.data|test",
-          encryptionType: 3,
-        });
-      });
-    });
-
-    describe("decrypt", () => {
-      const encString = new EncString(EncryptionType.Rsa2048_OaepSha256_B64, "data");
-
-      const cryptoService = Substitute.for<CryptoService>();
-      cryptoService.getOrgKey(null).resolves(null);
-      cryptoService.decryptToUtf8(encString, Arg.any()).resolves("decrypted");
-
-      beforeEach(() => {
-        (window as any).bitwardenContainerService = new ContainerService(cryptoService);
-      });
-
-      it("decrypts correctly", async () => {
-        const decrypted = await encString.decrypt(null);
-
-        expect(decrypted).toBe("decrypted");
-      });
-
-      it("result should be cached", async () => {
-        const decrypted = await encString.decrypt(null);
-        cryptoService.received(1).decryptToUtf8(Arg.any(), Arg.any());
-
-        expect(decrypted).toBe("decrypted");
-      });
-    });
-  });
-
-  describe("AesCbc256_B64", () => {
-    it("constructor", () => {
-      const encString = new EncString(EncryptionType.AesCbc256_B64, "data", "iv");
-
-      expect(encString).toEqual({
-        data: "data",
-        encryptedString: "0.iv|data",
-        encryptionType: 0,
-        iv: "iv",
-      });
-    });
-
-    describe("parse existing", () => {
-      it("valid", () => {
-        const encString = new EncString("0.iv|data");
-
-        expect(encString).toEqual({
-          data: "data",
-          encryptedString: "0.iv|data",
-          encryptionType: 0,
-          iv: "iv",
-        });
-      });
-
-      it("invalid", () => {
-        const encString = new EncString("0.iv|data|mac");
-
-        expect(encString).toEqual({
-          encryptedString: "0.iv|data|mac",
-          encryptionType: 0,
-        });
-      });
-    });
-  });
-
-  describe("AesCbc256_HmacSha256_B64", () => {
-    it("constructor", () => {
-      const encString = new EncString(EncryptionType.AesCbc256_HmacSha256_B64, "data", "iv", "mac");
-
-      expect(encString).toEqual({
-        data: "data",
-        encryptedString: "2.iv|data|mac",
-        encryptionType: 2,
-        iv: "iv",
-        mac: "mac",
-      });
-    });
-
-    it("valid", () => {
-      const encString = new EncString("2.iv|data|mac");
-
-      expect(encString).toEqual({
-        data: "data",
-        encryptedString: "2.iv|data|mac",
-        encryptionType: 2,
-        iv: "iv",
-        mac: "mac",
-      });
-    });
-
-    it("invalid", () => {
-      const encString = new EncString("2.iv|data");
-
-      expect(encString).toEqual({
-        encryptedString: "2.iv|data",
-        encryptionType: 2,
-      });
-    });
-  });
-
-  it("Exit early if null", () => {
-    const encString = new EncString(null);
-
-    expect(encString).toEqual({
-      encryptedString: null,
-    });
-  });
-
-  describe("decrypt", () => {
-    it("throws exception when bitwarden container not initialized", async () => {
-      const encString = new EncString(null);
-
-      expect.assertions(1);
-      try {
-        await encString.decrypt(null);
-      } catch (e) {
-        expect(e.message).toEqual("global bitwardenContainerService not initialized.");
-      }
-    });
-
-    it("handles value it can't decrypt", async () => {
-      const encString = new EncString(null);
-
-      const cryptoService = Substitute.for<CryptoService>();
-      cryptoService.getOrgKey(null).resolves(null);
-      cryptoService.decryptToUtf8(encString, Arg.any()).throws("error");
-
-      (window as any).bitwardenContainerService = new ContainerService(cryptoService);
-
-      const decrypted = await encString.decrypt(null);
-
-      expect(decrypted).toBe("[error: cannot decrypt]");
-
-      expect(encString).toEqual({
-        decryptedValue: "[error: cannot decrypt]",
-        encryptedString: null,
-      });
-    });
-
-    it("passes along key", async () => {
-      const encString = new EncString(null);
-      const key = Substitute.for<SymmetricCryptoKey>();
-
-      const cryptoService = Substitute.for<CryptoService>();
-      cryptoService.getOrgKey(null).resolves(null);
-
-      (window as any).bitwardenContainerService = new ContainerService(cryptoService);
-
-      await encString.decrypt(null, key);
-
-      cryptoService.received().decryptToUtf8(encString, key);
-    });
-  });
-});

jslib/common/spec/services/stateMigration.service.ts

@@ -1,84 +0,0 @@
-import { Arg, Substitute, SubstituteOf } from "@fluffy-spoon/substitute";
-
-import { StorageService } from "@/jslib/common/src/abstractions/storage.service";
-import { StateVersion } from "@/jslib/common/src/enums/stateVersion";
-import { StateFactory } from "@/jslib/common/src/factories/stateFactory";
-import { Account } from "@/jslib/common/src/models/domain/account";
-import { GlobalState } from "@/jslib/common/src/models/domain/globalState";
-import { StateMigrationService } from "@/jslib/common/src/services/stateMigration.service";
-
-const userId = "USER_ID";
-
-describe("State Migration Service", () => {
-  let storageService: SubstituteOf<StorageService>;
-  let secureStorageService: SubstituteOf<StorageService>;
-  let stateFactory: SubstituteOf<StateFactory>;
-
-  let stateMigrationService: StateMigrationService;
-
-  beforeEach(() => {
-    storageService = Substitute.for<StorageService>();
-    secureStorageService = Substitute.for<StorageService>();
-    stateFactory = Substitute.for<StateFactory>();
-
-    stateMigrationService = new StateMigrationService(
-      storageService,
-      secureStorageService,
-      stateFactory,
-    );
-  });
-
-  describe("StateVersion 3 to 4 migration", async () => {
-    beforeEach(() => {
-      const globalVersion3: Partial<GlobalState> = {
-        stateVersion: StateVersion.Three,
-      };
-
-      storageService.get("global", Arg.any()).resolves(globalVersion3);
-      storageService.get("authenticatedAccounts", Arg.any()).resolves([userId]);
-    });
-
-    it("clears everBeenUnlocked", async () => {
-      const accountVersion3: Account = {
-        profile: {
-          apiKeyClientId: null,
-          convertAccountToKeyConnector: null,
-          email: "EMAIL",
-          emailVerified: true,
-          everBeenUnlocked: true,
-          hasPremiumPersonally: false,
-          kdfIterations: 100000,
-          kdfType: 0,
-          keyHash: "KEY_HASH",
-          lastSync: "LAST_SYNC",
-          userId: userId,
-          usesKeyConnector: false,
-          forcePasswordReset: false,
-        },
-      };
-
-      const expectedAccountVersion4: Account = {
-        profile: {
-          ...accountVersion3.profile,
-        },
-      };
-      delete expectedAccountVersion4.profile.everBeenUnlocked;
-
-      storageService.get(userId, Arg.any()).resolves(accountVersion3);
-
-      await stateMigrationService.migrate();
-
-      storageService.received(1).save(userId, expectedAccountVersion4, Arg.any());
-    });
-
-    it("updates StateVersion number", async () => {
-      await stateMigrationService.migrate();
-
-      storageService.received(1).save(
-        "global",
-        Arg.is((globals: GlobalState) => globals.stateVersion === StateVersion.Four),
-        Arg.any(),
-      );
-    });
-  });
-});

jslib/common/spec/utils.ts

@@ -1,7 +1,3 @@
-import { Substitute, Arg } from "@fluffy-spoon/substitute";
-
-import { EncString } from "@/jslib/common/src/models/domain/encString";
-
 function newGuid() {
   return "xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx".replace(/[xy]/g, (c) => {
     const r = (Math.random() * 16) | 0;
@@ -21,13 +17,6 @@ export function BuildTestObject<T, K extends keyof T = keyof T>(
   return Object.assign(constructor === null ? {} : new constructor(), def) as T;
 }
 
-export function mockEnc(s: string): EncString {
-  const mock = Substitute.for<EncString>();
-  mock.decrypt(Arg.any(), Arg.any()).resolves(s);
-
-  return mock;
-}
-
 export function makeStaticByteArray(length: number, start = 0) {
   const arr = new Uint8Array(length);
   for (let i = 0; i < length; i++) {

package.json

@@ -2,7 +2,7 @@
   "name": "@bitwarden/directory-connector",
   "productName": "Bitwarden Directory Connector",
   "description": "Sync your user directory to your Bitwarden organization.",
-  "version": "2026.1.0",
+  "version": "2026.2.0",
   "keywords": [
     "bitwarden",
     "password",
@@ -75,13 +75,12 @@
   "devDependencies": {
     "@angular-eslint/eslint-plugin-template": "21.1.0",
     "@angular-eslint/template-parser": "21.1.0",
-    "@angular/build": "21.0.5",
+    "@angular/build": "21.1.2",
     "@angular/compiler-cli": "21.1.1",
     "@electron/notarize": "2.5.0",
     "@electron/rebuild": "4.0.1",
-    "@fluffy-spoon/substitute": "1.208.0",
     "@microsoft/microsoft-graph-types": "2.43.1",
-    "@ngtools/webpack": "21.0.5",
+    "@ngtools/webpack": "21.1.2",
     "@types/inquirer": "8.2.10",
     "@types/jest": "30.0.0",
     "@types/lowdb": "1.0.15",
@@ -91,10 +90,10 @@
     "@types/proper-lockfile": "4.1.4",
     "@types/semver": "7.7.1",
     "@types/tldjs": "2.3.4",
-    "@typescript-eslint/eslint-plugin": "8.50.0",
-    "@typescript-eslint/parser": "8.50.0",
+    "@typescript-eslint/eslint-plugin": "8.54.0",
+    "@typescript-eslint/parser": "8.54.0",
     "@yao-pkg/pkg": "5.16.1",
-    "babel-loader": "9.2.1",
+    "babel-loader": "10.0.0",
     "clean-webpack-plugin": "4.0.0",
     "jest-environment-jsdom": "30.2.0",
     "concurrently": "9.2.0",
@@ -147,7 +146,7 @@
   "dependencies": {
     "@angular/animations": "21.1.1",
     "@angular/cdk": "21.1.1",
-    "@angular/cli": "21.0.5",
+    "@angular/cli": "21.1.2",
     "@angular/common": "21.1.1",
     "@angular/compiler": "21.1.1",
     "@angular/core": "21.1.1",

src/services/authService.spec.ts

@@ -1,7 +1,8 @@
-import { Arg, Substitute, SubstituteOf } from "@fluffy-spoon/substitute";
+import { mock } from "jest-mock-extended";
 
 import { ApiService } from "@/jslib/common/src/abstractions/api.service";
 import { AppIdService } from "@/jslib/common/src/abstractions/appId.service";
+import { MessagingService } from "@/jslib/common/src/abstractions/messaging.service";
 import { PlatformUtilsService } from "@/jslib/common/src/abstractions/platformUtils.service";
 import { Utils } from "@/jslib/common/src/misc/utils";
 import {
@@ -11,7 +12,6 @@ import {
 } from "@/jslib/common/src/models/domain/account";
 import { IdentityTokenResponse } from "@/jslib/common/src/models/response/identityTokenResponse";
 
-import { MessagingService } from "../../jslib/common/src/abstractions/messaging.service";
 import { Account, DirectoryConfigurations, DirectorySettings } from "../models/account";
 
 import { AuthService } from "./auth.service";
@@ -35,22 +35,22 @@ export function identityTokenResponseFactory() {
 }
 
 describe("AuthService", () => {
-  let apiService: SubstituteOf<ApiService>;
-  let appIdService: SubstituteOf<AppIdService>;
-  let platformUtilsService: SubstituteOf<PlatformUtilsService>;
-  let messagingService: SubstituteOf<MessagingService>;
-  let stateService: SubstituteOf<StateService>;
+  let apiService: jest.Mocked<ApiService>;
+  let appIdService: jest.Mocked<AppIdService>;
+  let platformUtilsService: jest.Mocked<PlatformUtilsService>;
+  let messagingService: jest.Mocked<MessagingService>;
+  let stateService: jest.Mocked<StateService>;
 
   let authService: AuthService;
 
   beforeEach(async () => {
-    apiService = Substitute.for();
-    appIdService = Substitute.for();
-    platformUtilsService = Substitute.for();
-    stateService = Substitute.for();
-    messagingService = Substitute.for();
+    apiService = mock<ApiService>();
+    appIdService = mock<AppIdService>();
+    platformUtilsService = mock<PlatformUtilsService>();
+    stateService = mock<StateService>();
+    messagingService = mock<MessagingService>();
 
-    appIdService.getAppId().resolves(deviceId);
+    appIdService.getAppId.mockResolvedValue(deviceId);
 
     authService = new AuthService(
       apiService,
@@ -62,11 +62,12 @@ describe("AuthService", () => {
   });
 
   it("sets the local environment after a successful login", async () => {
-    apiService.postIdentityToken(Arg.any()).resolves(identityTokenResponseFactory());
+    apiService.postIdentityToken.mockResolvedValue(identityTokenResponseFactory());
 
     await authService.logIn({ clientId, clientSecret });
 
-    stateService.received(1).addAccount(
+    expect(stateService.addAccount).toHaveBeenCalledTimes(1);
+    expect(stateService.addAccount).toHaveBeenCalledWith(
       new Account({
         profile: {
           ...new AccountProfile(),

src/services/directory-services/ldap-directory.service.ts

@@ -68,10 +68,12 @@ export class LdapDirectoryService implements IDirectoryService {
         }
         groups = await this.getGroups(groupForce);
       }
-    } finally {
+    } catch (e) {
       await this.client.unbind();
+      throw e;
     }
 
+    await this.client.unbind();
     return [groups, users];
   }
 
@@ -453,8 +455,9 @@ export class LdapDirectoryService implements IDirectoryService {
 
     try {
       await this.client.bind(user, pass);
-    } catch {
+    } catch (error) {
       await this.client.unbind();
+      throw error;
     }
   }
 

src/services/sync.service.spec.ts

@@ -6,8 +6,6 @@ import { MessagingService } from "@/jslib/common/src/abstractions/messaging.serv
 import { OrganizationImportRequest } from "@/jslib/common/src/models/request/organizationImportRequest";
 import { ApiService } from "@/jslib/common/src/services/api.service";
 
-import { GroupEntry } from "@/src/models/groupEntry";
-
 import { getSyncConfiguration } from "../../utils/openldap/config-fixtures";
 import { DirectoryFactoryService } from "../abstractions/directory-factory.service";
 import { DirectoryType } from "../enums/directoryType";
@@ -136,198 +134,4 @@ describe("SyncService", () => {
 
     expect(apiService.postPublicImportDirectory).not.toHaveBeenCalled();
   });
-
-  describe("nested and circular group handling", () => {
-    function createGroup(
-      name: string,
-      userExternalIds: string[] = [],
-      groupMemberReferenceIds: string[] = [],
-    ) {
-      return GroupEntry.fromJSON({
-        name,
-        referenceId: name,
-        externalId: name,
-        userMemberExternalIds: userExternalIds,
-        groupMemberReferenceIds: groupMemberReferenceIds,
-        users: [],
-      });
-    }
-
-    it("should handle simple circular reference (A ↔ B) without stack overflow", async () => {
-      const groupA = createGroup("GroupA", ["userA"], ["GroupB"]);
-      const groupB = createGroup("GroupB", ["userB"], ["GroupA"]);
-      const circularGroups = [groupA, groupB];
-
-      const mockDirectoryService = mock<LdapDirectoryService>();
-      mockDirectoryService.getEntries.mockResolvedValue([circularGroups, []]);
-      directoryFactory.createService.mockReturnValue(mockDirectoryService);
-
-      stateService.getSync.mockResolvedValue(getSyncConfiguration({ groups: true, users: true }));
-      cryptoFunctionService.hash.mockResolvedValue(new ArrayBuffer(1));
-      stateService.getLastSyncHash.mockResolvedValue("unique hash");
-      singleRequestBuilder.buildRequest.mockReturnValue([
-        { members: [], groups: [], overwriteExisting: true, largeImport: false },
-      ]);
-
-      const [groups] = await syncService.sync(true, true);
-
-      // Both groups should have both users after flattening
-      expect(groups[0].userMemberExternalIds).toContain("userA");
-      expect(groups[0].userMemberExternalIds).toContain("userB");
-      expect(groups[1].userMemberExternalIds).toContain("userA");
-      expect(groups[1].userMemberExternalIds).toContain("userB");
-    });
-
-    it("should handle longer circular chain (A → B → C → A) without stack overflow", async () => {
-      const groupA = createGroup("GroupA", ["userA"], ["GroupB"]);
-      const groupB = createGroup("GroupB", ["userB"], ["GroupC"]);
-      const groupC = createGroup("GroupC", ["userC"], ["GroupA"]);
-      const circularGroups = [groupA, groupB, groupC];
-
-      const mockDirectoryService = mock<LdapDirectoryService>();
-      mockDirectoryService.getEntries.mockResolvedValue([circularGroups, []]);
-      directoryFactory.createService.mockReturnValue(mockDirectoryService);
-
-      stateService.getSync.mockResolvedValue(getSyncConfiguration({ groups: true, users: true }));
-      cryptoFunctionService.hash.mockResolvedValue(new ArrayBuffer(1));
-      stateService.getLastSyncHash.mockResolvedValue("unique hash");
-      singleRequestBuilder.buildRequest.mockReturnValue([
-        { members: [], groups: [], overwriteExisting: true, largeImport: false },
-      ]);
-
-      const [groups] = await syncService.sync(true, true);
-
-      // All groups should have all users after flattening
-      for (const group of groups) {
-        expect(group.userMemberExternalIds).toContain("userA");
-        expect(group.userMemberExternalIds).toContain("userB");
-        expect(group.userMemberExternalIds).toContain("userC");
-      }
-    });
-
-    it("should handle diamond structure (A → [B, C] → D)", async () => {
-      const groupA = createGroup("GroupA", ["userA"], ["GroupB", "GroupC"]);
-      const groupB = createGroup("GroupB", ["userB"], ["GroupD"]);
-      const groupC = createGroup("GroupC", ["userC"], ["GroupD"]);
-      const groupD = createGroup("GroupD", ["userD"], []);
-      const diamondGroups = [groupA, groupB, groupC, groupD];
-
-      const mockDirectoryService = mock<LdapDirectoryService>();
-      mockDirectoryService.getEntries.mockResolvedValue([diamondGroups, []]);
-      directoryFactory.createService.mockReturnValue(mockDirectoryService);
-
-      stateService.getSync.mockResolvedValue(getSyncConfiguration({ groups: true, users: true }));
-      cryptoFunctionService.hash.mockResolvedValue(new ArrayBuffer(1));
-      stateService.getLastSyncHash.mockResolvedValue("unique hash");
-      singleRequestBuilder.buildRequest.mockReturnValue([
-        { members: [], groups: [], overwriteExisting: true, largeImport: false },
-      ]);
-
-      const [groups] = await syncService.sync(true, true);
-
-      const [a, b, c, d] = groups;
-
-      // A should have all users (through B and C, both containing D)
-      expect(a.userMemberExternalIds).toContain("userA");
-      expect(a.userMemberExternalIds).toContain("userB");
-      expect(a.userMemberExternalIds).toContain("userC");
-      expect(a.userMemberExternalIds).toContain("userD");
-
-      // B should have its own user plus D's user
-      expect(b.userMemberExternalIds).toContain("userB");
-      expect(b.userMemberExternalIds).toContain("userD");
-
-      // C should have its own user plus D's user
-      expect(c.userMemberExternalIds).toContain("userC");
-      expect(c.userMemberExternalIds).toContain("userD");
-
-      // D should only have its own user
-      expect(d.userMemberExternalIds).toContain("userD");
-      expect(d.userMemberExternalIds.size).toBe(1);
-    });
-
-    it("should handle deep nesting with circular reference at leaf", async () => {
-      // Structure: A → B → C → D → B (cycle back to B)
-      const groupA = createGroup("GroupA", ["userA"], ["GroupB"]);
-      const groupB = createGroup("GroupB", ["userB"], ["GroupC"]);
-      const groupC = createGroup("GroupC", ["userC"], ["GroupD"]);
-      const groupD = createGroup("GroupD", ["userD"], ["GroupB"]); // cycles back to B
-      const deepGroups = [groupA, groupB, groupC, groupD];
-
-      const mockDirectoryService = mock<LdapDirectoryService>();
-      mockDirectoryService.getEntries.mockResolvedValue([deepGroups, []]);
-      directoryFactory.createService.mockReturnValue(mockDirectoryService);
-
-      stateService.getSync.mockResolvedValue(getSyncConfiguration({ groups: true, users: true }));
-      cryptoFunctionService.hash.mockResolvedValue(new ArrayBuffer(1));
-      stateService.getLastSyncHash.mockResolvedValue("unique hash");
-      singleRequestBuilder.buildRequest.mockReturnValue([
-        { members: [], groups: [], overwriteExisting: true, largeImport: false },
-      ]);
-
-      const [groups] = await syncService.sync(true, true);
-
-      const [a, b, c, d] = groups;
-
-      // A should have all users
-      expect(a.userMemberExternalIds.size).toBe(4);
-
-      // B, C, D form a cycle, so they should all have each other's users
-      expect(b.userMemberExternalIds).toContain("userB");
-      expect(b.userMemberExternalIds).toContain("userC");
-      expect(b.userMemberExternalIds).toContain("userD");
-
-      expect(c.userMemberExternalIds).toContain("userB");
-      expect(c.userMemberExternalIds).toContain("userC");
-      expect(c.userMemberExternalIds).toContain("userD");
-
-      expect(d.userMemberExternalIds).toContain("userB");
-      expect(d.userMemberExternalIds).toContain("userC");
-      expect(d.userMemberExternalIds).toContain("userD");
-    });
-
-    it("should handle complex structure with multiple cycles and shared members", async () => {
-      // Structure:
-      // A → [B, C]
-      // B → [D, E]
-      // C → [E, F]
-      // D → A (cycle)
-      // E → C (cycle)
-      // F → (leaf)
-      const groupA = createGroup("GroupA", ["userA"], ["GroupB", "GroupC"]);
-      const groupB = createGroup("GroupB", ["userB"], ["GroupD", "GroupE"]);
-      const groupC = createGroup("GroupC", ["userC"], ["GroupE", "GroupF"]);
-      const groupD = createGroup("GroupD", ["userD"], ["GroupA"]); // cycle to A
-      const groupE = createGroup("GroupE", ["userE"], ["GroupC"]); // cycle to C
-      const groupF = createGroup("GroupF", ["userF"], []);
-      const complexGroups = [groupA, groupB, groupC, groupD, groupE, groupF];
-
-      const mockDirectoryService = mock<LdapDirectoryService>();
-      mockDirectoryService.getEntries.mockResolvedValue([complexGroups, []]);
-      directoryFactory.createService.mockReturnValue(mockDirectoryService);
-
-      stateService.getSync.mockResolvedValue(getSyncConfiguration({ groups: true, users: true }));
-      cryptoFunctionService.hash.mockResolvedValue(new ArrayBuffer(1));
-      stateService.getLastSyncHash.mockResolvedValue("unique hash");
-      singleRequestBuilder.buildRequest.mockReturnValue([
-        { members: [], groups: [], overwriteExisting: true, largeImport: false },
-      ]);
-
-      // Should complete without stack overflow
-      const [groups] = await syncService.sync(true, true);
-
-      expect(groups).toHaveLength(6);
-
-      // Verify A gets users from its descendants
-      const a = groups.find((g) => g.name === "GroupA");
-      expect(a.userMemberExternalIds).toContain("userA");
-      expect(a.userMemberExternalIds).toContain("userB");
-      expect(a.userMemberExternalIds).toContain("userC");
-
-      // F should only have its own user (it's a leaf)
-      const f = groups.find((g) => g.name === "GroupF");
-      expect(f.userMemberExternalIds).toContain("userF");
-      expect(f.userMemberExternalIds.size).toBe(1);
-    });
-  });
 });

src/services/sync.service.ts

@@ -196,27 +196,14 @@ export class SyncService {
     return users == null ? null : users.filter((u) => u.email?.length <= 256);
   }
 
-  private flattenUsersToGroups(
-    levelGroups: GroupEntry[],
-    allGroups: GroupEntry[],
-    visitedGroups?: Set<string>,
-  ): Set<string> {
+  private flattenUsersToGroups(levelGroups: GroupEntry[], allGroups: GroupEntry[]): Set<string> {
     let allUsers = new Set<string>();
     if (allGroups == null) {
       return allUsers;
     }
-
     for (const group of levelGroups) {
-      const visited = visitedGroups ?? new Set<string>();
-
-      if (visited.has(group.referenceId)) {
-        continue;
-      }
-
-      visited.add(group.referenceId);
-
       const childGroups = allGroups.filter((g) => group.groupMemberReferenceIds.has(g.referenceId));
-      const childUsers = this.flattenUsersToGroups(childGroups, allGroups, visited);
+      const childUsers = this.flattenUsersToGroups(childGroups, allGroups);
       childUsers.forEach((id) => group.userMemberExternalIds.add(id));
       allUsers = new Set([...allUsers, ...group.userMemberExternalIds]);
     }

utils/openldap/example-ldifs/directory-circular-groups.ldif

@@ -1,308 +0,0 @@
-version: 1
-
-dn: dc=bitwarden,dc=com
-dc: bitwarden
-objectClass: dcObject
-objectClass: organization
-o: Bitwarden
-
-# Organizational Units
-dn: ou=Human Resources,dc=bitwarden,dc=com
-changetype: add
-ou: Human Resources
-objectClass: top
-objectClass: organizationalUnit
-
-dn: ou=Engineering,dc=bitwarden,dc=com
-changetype: add
-ou: Engineering
-objectClass: top
-objectClass: organizationalUnit
-
-dn: ou=Marketing,dc=bitwarden,dc=com
-changetype: add
-ou: Marketing
-objectClass: top
-objectClass: organizationalUnit
-
-# Users - Human Resources
-dn: cn=Roland Dyke,ou=Human Resources,dc=bitwarden,dc=com
-changetype: add
-objectClass: top
-objectClass: person
-objectClass: organizationalPerson
-objectClass: inetOrgPerson
-cn: Roland Dyke
-sn: Dyke
-description: This is Roland Dyke's description
-facsimileTelephoneNumber: +1 804 674-5794
-l: San Francisco
-ou: Human Resources
-postalAddress: Human Resources$San Francisco
-telephoneNumber: +1 804 831-5121
-title: Supreme Human Resources Writer
-userPassword: Password1
-uid: DykeR
-givenName: Roland
-mail: DykeR@220af87272f04218bb8dd81d50fb19f5.bitwarden.com
-carLicense: 4CMGOJ
-departmentNumber: 2838
-employeeType: Contract
-homePhone: +1 804 936-4965
-initials: R. D.
-mobile: +1 804 592-3734
-pager: +1 804 285-2962
-roomNumber: 9890
-
-dn: cn=Teirtza Kara,ou=Human Resources,dc=bitwarden,dc=com
-changetype: add
-objectClass: top
-objectClass: person
-objectClass: organizationalPerson
-objectClass: inetOrgPerson
-cn: Teirtza Kara
-sn: Kara
-description: This is Teirtza Kara's description
-facsimileTelephoneNumber: +1 206 759-2040
-l: San Francisco
-ou: Human Resources
-postalAddress: Human Resources$San Francisco
-telephoneNumber: +1 206 562-1407
-title: Junior Human Resources President
-userPassword: Password1
-uid: KaraT
-givenName: Teirtza
-mail: KaraT@c2afe8b3509f4a20b2b784841685bd74.bitwarden.com
-carLicense: O9GAN2
-departmentNumber: 3880
-employeeType: Employee
-homePhone: +1 206 154-4842
-initials: T. K.
-mobile: +1 206 860-1835
-pager: +1 206 684-1438
-roomNumber: 9079
-
-# Users - Engineering
-dn: cn=Alice Chen,ou=Engineering,dc=bitwarden,dc=com
-changetype: add
-objectClass: top
-objectClass: person
-objectClass: organizationalPerson
-objectClass: inetOrgPerson
-cn: Alice Chen
-sn: Chen
-description: Senior DevOps Engineer
-l: Seattle
-ou: Engineering
-telephoneNumber: +1 206 555-0101
-title: Senior DevOps Engineer
-userPassword: Password1
-uid: ChenA
-givenName: Alice
-mail: ChenA@bitwarden.com
-employeeType: Employee
-
-dn: cn=Bob Martinez,ou=Engineering,dc=bitwarden,dc=com
-changetype: add
-objectClass: top
-objectClass: person
-objectClass: organizationalPerson
-objectClass: inetOrgPerson
-cn: Bob Martinez
-sn: Martinez
-description: Platform Engineer
-l: Austin
-ou: Engineering
-telephoneNumber: +1 512 555-0102
-title: Platform Engineer
-userPassword: Password1
-uid: MartinezB
-givenName: Bob
-mail: MartinezB@bitwarden.com
-employeeType: Employee
-
-dn: cn=Carol Williams,ou=Engineering,dc=bitwarden,dc=com
-changetype: add
-objectClass: top
-objectClass: person
-objectClass: organizationalPerson
-objectClass: inetOrgPerson
-cn: Carol Williams
-sn: Williams
-description: QA Lead
-l: Denver
-ou: Engineering
-telephoneNumber: +1 303 555-0103
-title: QA Lead
-userPassword: Password1
-uid: WilliamsC
-givenName: Carol
-mail: WilliamsC@bitwarden.com
-employeeType: Employee
-
-dn: cn=David Kim,ou=Engineering,dc=bitwarden,dc=com
-changetype: add
-objectClass: top
-objectClass: person
-objectClass: organizationalPerson
-objectClass: inetOrgPerson
-cn: David Kim
-sn: Kim
-description: QA Engineer
-l: Portland
-ou: Engineering
-telephoneNumber: +1 503 555-0104
-title: QA Engineer
-userPassword: Password1
-uid: KimD
-givenName: David
-mail: KimD@bitwarden.com
-employeeType: Contractor
-
-# Users - Marketing
-dn: cn=Eva Johnson,ou=Marketing,dc=bitwarden,dc=com
-changetype: add
-objectClass: top
-objectClass: person
-objectClass: organizationalPerson
-objectClass: inetOrgPerson
-cn: Eva Johnson
-sn: Johnson
-description: Marketing Director
-l: New York
-ou: Marketing
-telephoneNumber: +1 212 555-0105
-title: Marketing Director
-userPassword: Password1
-uid: JohnsonE
-givenName: Eva
-mail: JohnsonE@bitwarden.com
-employeeType: Employee
-
-dn: cn=Frank Lee,ou=Marketing,dc=bitwarden,dc=com
-changetype: add
-objectClass: top
-objectClass: person
-objectClass: organizationalPerson
-objectClass: inetOrgPerson
-cn: Frank Lee
-sn: Lee
-description: Content Strategist
-l: Chicago
-ou: Marketing
-telephoneNumber: +1 312 555-0106
-title: Content Strategist
-userPassword: Password1
-uid: LeeF
-givenName: Frank
-mail: LeeF@bitwarden.com
-employeeType: Employee
-
-# ============================================================
-# GROUP HIERARCHY
-# ============================================================
-# Structure (arrows show "contains" relationship):
-#
-#   AllStaff
-#     ├── Engineering ◄────────────────┐ (CYCLE from Platform)
-#     │     ├── DevOps                 │
-#     │     │     └── Platform ────────┘
-#     │     └── QA
-#     ├── Marketing
-#     └── HR
-#
-#   Contractors ─── DevOps (diamond: second path to Platform)
-#
-#   TestNestA ◄──► TestNestB (simple bidirectional cycle)
-#
-# ============================================================
-
-# Leaf group - Platform team (CYCLES BACK to Engineering)
-dn: cn=Platform,dc=bitwarden,dc=com
-changetype: add
-cn: Platform
-member: cn=Bob Martinez,ou=Engineering,dc=bitwarden,dc=com
-member: cn=Engineering,dc=bitwarden,dc=com
-objectclass: groupOfNames
-objectclass: top
-
-# DevOps group - contains Platform subgroup
-dn: cn=DevOps,dc=bitwarden,dc=com
-changetype: add
-cn: DevOps
-member: cn=Alice Chen,ou=Engineering,dc=bitwarden,dc=com
-member: cn=Platform,dc=bitwarden,dc=com
-objectclass: groupOfNames
-objectclass: top
-
-# QA group
-dn: cn=QA,dc=bitwarden,dc=com
-changetype: add
-cn: QA
-member: cn=Carol Williams,ou=Engineering,dc=bitwarden,dc=com
-member: cn=David Kim,ou=Engineering,dc=bitwarden,dc=com
-objectclass: groupOfNames
-objectclass: top
-
-# Engineering group - contains DevOps and QA subgroups
-dn: cn=Engineering,dc=bitwarden,dc=com
-changetype: add
-cn: Engineering
-member: cn=DevOps,dc=bitwarden,dc=com
-member: cn=QA,dc=bitwarden,dc=com
-objectclass: groupOfNames
-objectclass: top
-
-# Marketing group
-dn: cn=Marketing,dc=bitwarden,dc=com
-changetype: add
-cn: Marketing
-member: cn=Eva Johnson,ou=Marketing,dc=bitwarden,dc=com
-member: cn=Frank Lee,ou=Marketing,dc=bitwarden,dc=com
-objectclass: groupOfNames
-objectclass: top
-
-# HR group
-dn: cn=HR,dc=bitwarden,dc=com
-changetype: add
-cn: HR
-member: cn=Roland Dyke,ou=Human Resources,dc=bitwarden,dc=com
-member: cn=Teirtza Kara,ou=Human Resources,dc=bitwarden,dc=com
-objectclass: groupOfNames
-objectclass: top
-
-# AllStaff - top-level group containing all departments
-dn: cn=AllStaff,dc=bitwarden,dc=com
-changetype: add
-cn: AllStaff
-member: cn=Engineering,dc=bitwarden,dc=com
-member: cn=Marketing,dc=bitwarden,dc=com
-member: cn=HR,dc=bitwarden,dc=com
-objectclass: groupOfNames
-objectclass: top
-
-# Contractors group - creates diamond pattern (second path to Platform via DevOps)
-dn: cn=Contractors,dc=bitwarden,dc=com
-changetype: add
-cn: Contractors
-member: cn=DevOps,dc=bitwarden,dc=com
-member: cn=David Kim,ou=Engineering,dc=bitwarden,dc=com
-objectclass: groupOfNames
-objectclass: top
-
-# Simple bidirectional cycle test groups (preserved from original)
-dn: cn=TestNestA,dc=bitwarden,dc=com
-changetype: add
-cn: TestNestA
-member: cn=TestNestB,dc=bitwarden,dc=com
-member: cn=Roland Dyke,ou=Human Resources,dc=bitwarden,dc=com
-objectclass: groupOfNames
-objectclass: top
-
-dn: cn=TestNestB,dc=bitwarden,dc=com
-changetype: add
-cn: TestNestB
-member: cn=TestNestA,dc=bitwarden,dc=com
-member: cn=Teirtza Kara,ou=Human Resources,dc=bitwarden,dc=com
-objectclass: groupOfNames
-objectclass: top