Fix circular groups

package.json

@@ -75,13 +75,13 @@
   "devDependencies": {
     "@angular-eslint/eslint-plugin-template": "21.1.0",
     "@angular-eslint/template-parser": "21.1.0",
-    "@angular/build": "21.1.2",
+    "@angular/build": "21.0.5",
     "@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.1.2",
+    "@ngtools/webpack": "21.0.5",
     "@types/inquirer": "8.2.10",
     "@types/jest": "30.0.0",
     "@types/lowdb": "1.0.15",
@@ -147,7 +147,7 @@
   "dependencies": {
     "@angular/animations": "21.1.1",
     "@angular/cdk": "21.1.1",
-    "@angular/cli": "21.1.2",
+    "@angular/cli": "21.0.5",
     "@angular/common": "21.1.1",
     "@angular/compiler": "21.1.1",
     "@angular/core": "21.1.1",

src/services/sync.service.spec.ts

@@ -6,6 +6,8 @@ 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";
@@ -134,4 +136,198 @@ 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,14 +196,27 @@ export class SyncService {
     return users == null ? null : users.filter((u) => u.email?.length <= 256);
   }
 
-  private flattenUsersToGroups(levelGroups: GroupEntry[], allGroups: GroupEntry[]): Set<string> {
+  private flattenUsersToGroups(
+    levelGroups: GroupEntry[],
+    allGroups: GroupEntry[],
+    visitedGroups?: Set<string>,
+  ): 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);
+      const childUsers = this.flattenUsersToGroups(childGroups, allGroups, visited);
       childUsers.forEach((id) => group.userMemberExternalIds.add(id));
       allUsers = new Set([...allUsers, ...group.userMemberExternalIds]);
     }

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

@@ -0,0 +1,308 @@
+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