From 0ce4547d34f418b272fe58ebdb6ad648997938d0 Mon Sep 17 00:00:00 2001 From: Sugianto BW Date: Fri, 26 Sep 2025 14:13:54 +0800 Subject: [PATCH] suggestion changes --- openldap/group-fixtures.ts | 10 +- openldap/ldifs/directory-20.ldif | 164 ++---------------- openldap/user-fixtures.ts | 35 ---- ...ldap-directory.service.integration.spec.ts | 84 --------- src/services/ldap-directory.service.ts | 18 +- src/services/sync.service.ts | 9 +- 6 files changed, 33 insertions(+), 287 deletions(-) diff --git a/openldap/group-fixtures.ts b/openldap/group-fixtures.ts index f02351de..a6298840 100644 --- a/openldap/group-fixtures.ts +++ b/openldap/group-fixtures.ts @@ -37,9 +37,9 @@ const data: Jsonify[] = [ }, { userMemberExternalIds: [ - "cn=Benjamin Chen,ou=Product Development,dc=bitwarden,dc=com", - "cn=Karen Smith,ou=Product Development,dc=bitwarden,dc=com", - "cn=Robert Johnson,ou=Product Development,dc=bitwarden,dc=com", + "cn=Painterson Miki,ou=Product Development,dc=bitwarden,dc=com", + "cn=Virgina Pichocki,ou=Product Development,dc=bitwarden,dc=com", + "cn=Steffen Carsten,ou=Product Development,dc=bitwarden,dc=com", ], groupMemberReferenceIds: [], users: [], @@ -49,8 +49,8 @@ const data: Jsonify[] = [ }, { userMemberExternalIds: [ - "cn=Thomas Williams,ou=Management,dc=bitwarden,dc=com", - "cn=Michelle Brown,ou=Management,dc=bitwarden,dc=com", + "cn=Angus Merizzi,ou=Management,dc=bitwarden,dc=com", + "cn=Grissel Currer,ou=Management,dc=bitwarden,dc=com", ], groupMemberReferenceIds: [], users: [], diff --git a/openldap/ldifs/directory-20.ldif b/openldap/ldifs/directory-20.ldif index 81655718..ee939c4a 100644 --- a/openldap/ldifs/directory-20.ldif +++ b/openldap/ldifs/directory-20.ldif @@ -690,13 +690,17 @@ roomNumber: 9273 manager: cn=Inga Schnirer,ou=Product Testing,dc=bitwarden, dc=com secretary: cn=Keven Gilleland,ou=Administrative,dc=bitwarden, dc=com +# DevOps Team and Security Team identify their members by the member uid attribute, +# instead of the member Dn attribute. +# These test that group membership by uid works correctly. + dn: cn=DevOps Team,dc=bitwarden,dc=com changetype: add cn: DevOps Team gidnumber: 800 -memberuid: ChenB -memberuid: SmithK -memberuid: JohnsonR +memberuid: mikip +memberuid: pichockv +memberuid: carstens objectclass: posixGroup objectclass: top @@ -704,157 +708,9 @@ dn: cn=Security Team,dc=bitwarden,dc=com changetype: add cn: Security Team gidnumber: 900 -memberuid: WilliamsT -memberuid: BrownM +memberuid: merizzia +memberuid: currerg objectclass: posixGroup objectclass: top -dn: cn=Benjamin Chen,ou=Product Development,dc=bitwarden, dc=com -changetype: add -objectClass: top -objectClass: person -objectClass: organizationalPerson -objectClass: inetOrgPerson -cn: Benjamin Chen -sn: Chen -description: This is Benjamin Chen's description -facsimileTelephoneNumber: +1 408 555-1234 -l: San Jose -ou: Product Development -postalAddress: Product Development$San Jose -telephoneNumber: +1 408 555-5678 -title: Senior DevOps Engineer -userPassword: Password1 -uid: ChenB -givenName: Benjamin -mail: ChenB@9f8a7b6c5d4e3f2a1b0c9d8e7f6a5b4c3.bitwarden.com -carLicense: 2K8N9L -departmentNumber: 1001 -employeeType: Employee -homePhone: +1 408 555-9876 -initials: B. C. -mobile: +1 408 555-4321 -pager: +1 408 555-7890 -roomNumber: 7125 -manager: cn=Roland Dyke,ou=Human Resources,dc=bitwarden, dc=com -secretary: cn=Keven Gilleland,ou=Administrative,dc=bitwarden, dc=com - -dn: cn=Karen Smith,ou=Product Development,dc=bitwarden, dc=com -changetype: add -objectClass: top -objectClass: person -objectClass: organizationalPerson -objectClass: inetOrgPerson -cn: Karen Smith -sn: Smith -description: This is Karen Smith's description -facsimileTelephoneNumber: +1 415 555-2345 -l: San Francisco -ou: Product Development -postalAddress: Product Development$San Francisco -telephoneNumber: +1 415 555-6789 -title: Senior Systems Administrator -userPassword: Password1 -uid: SmithK -givenName: Karen -mail: SmithK@3e4f5a6b7c8d9e0f1a2b3c4d5e6f7a8b9.bitwarden.com -carLicense: 5M3P7Q -departmentNumber: 1002 -employeeType: Employee -homePhone: +1 415 555-0987 -initials: K. S. -mobile: +1 415 555-5432 -pager: +1 415 555-8901 -roomNumber: 7126 -manager: cn=Roland Dyke,ou=Human Resources,dc=bitwarden, dc=com -secretary: cn=Keven Gilleland,ou=Administrative,dc=bitwarden, dc=com - -dn: cn=Robert Johnson,ou=Product Development,dc=bitwarden, dc=com -changetype: add -objectClass: top -objectClass: person -objectClass: organizationalPerson -objectClass: inetOrgPerson -cn: Robert Johnson -sn: Johnson -description: This is Robert Johnson's description -facsimileTelephoneNumber: +1 510 555-3456 -l: Oakland -ou: Product Development -postalAddress: Product Development$Oakland -telephoneNumber: +1 510 555-7890 -title: Cloud Infrastructure Engineer -userPassword: Password1 -uid: JohnsonR -givenName: Robert -mail: JohnsonR@7c8d9e0f1a2b3c4d5e6f7a8b9c0d1e2f3.bitwarden.com -carLicense: 8X2Y4Z -departmentNumber: 1003 -employeeType: Employee -homePhone: +1 510 555-1098 -initials: R. J. -mobile: +1 510 555-6543 -pager: +1 510 555-9012 -roomNumber: 7127 -manager: cn=Roland Dyke,ou=Human Resources,dc=bitwarden, dc=com -secretary: cn=Keven Gilleland,ou=Administrative,dc=bitwarden, dc=com - -dn: cn=Thomas Williams,ou=Management,dc=bitwarden, dc=com -changetype: add -objectClass: top -objectClass: person -objectClass: organizationalPerson -objectClass: inetOrgPerson -cn: Thomas Williams -sn: Williams -description: This is Thomas Williams's description -facsimileTelephoneNumber: +1 650 555-4567 -l: Palo Alto -ou: Management -postalAddress: Management$Palo Alto -telephoneNumber: +1 650 555-8901 -title: Chief Security Officer -userPassword: Password1 -uid: WilliamsT -givenName: Thomas -mail: WilliamsT@1f2a3b4c5d6e7f8a9b0c1d2e3f4a5b6c7.bitwarden.com -carLicense: 6H9J2K -departmentNumber: 1004 -employeeType: Employee -homePhone: +1 650 555-2109 -initials: T. W. -mobile: +1 650 555-7654 -pager: +1 650 555-0123 -roomNumber: 7128 -manager: cn=Roland Dyke,ou=Human Resources,dc=bitwarden, dc=com -secretary: cn=Keven Gilleland,ou=Administrative,dc=bitwarden, dc=com - -dn: cn=Michelle Brown,ou=Management,dc=bitwarden, dc=com -changetype: add -objectClass: top -objectClass: person -objectClass: organizationalPerson -objectClass: inetOrgPerson -cn: Michelle Brown -sn: Brown -description: This is Michelle Brown's description -facsimileTelephoneNumber: +1 408 555-5678 -l: Santa Clara -ou: Management -postalAddress: Management$Santa Clara -telephoneNumber: +1 408 555-9012 -title: Security Analyst -userPassword: Password1 -uid: BrownM -givenName: Michelle -mail: BrownM@5d6e7f8a9b0c1d2e3f4a5b6c7d8e9f0a1.bitwarden.com -carLicense: 4T6U8V -departmentNumber: 1005 -employeeType: Employee -homePhone: +1 408 555-3210 -initials: M. B. -mobile: +1 408 555-8765 -pager: +1 408 555-1234 -roomNumber: 7129 -manager: cn=Roland Dyke,ou=Human Resources,dc=bitwarden, dc=com -secretary: cn=Keven Gilleland,ou=Administrative,dc=bitwarden, dc=com + diff --git a/openldap/user-fixtures.ts b/openldap/user-fixtures.ts index 168d81f4..d34a2eec 100644 --- a/openldap/user-fixtures.ts +++ b/openldap/user-fixtures.ts @@ -144,41 +144,6 @@ const data: Jsonify[] = [ externalId: "cn=Loella Mak,ou=Payroll,dc=bitwarden,dc=com", email: "makl@6ab3e25ca49d4d64aaf44844288a8ef7.bitwarden.com", }, - { - disabled: false, - deleted: false, - referenceId: "cn=Benjamin Chen,ou=Product Development,dc=bitwarden,dc=com", - externalId: "cn=Benjamin Chen,ou=Product Development,dc=bitwarden,dc=com", - email: "chenb@9f8a7b6c5d4e3f2a1b0c9d8e7f6a5b4c3.bitwarden.com", - }, - { - disabled: false, - deleted: false, - referenceId: "cn=Karen Smith,ou=Product Development,dc=bitwarden,dc=com", - externalId: "cn=Karen Smith,ou=Product Development,dc=bitwarden,dc=com", - email: "smithk@3e4f5a6b7c8d9e0f1a2b3c4d5e6f7a8b9.bitwarden.com", - }, - { - disabled: false, - deleted: false, - referenceId: "cn=Robert Johnson,ou=Product Development,dc=bitwarden,dc=com", - externalId: "cn=Robert Johnson,ou=Product Development,dc=bitwarden,dc=com", - email: "johnsonr@7c8d9e0f1a2b3c4d5e6f7a8b9c0d1e2f3.bitwarden.com", - }, - { - disabled: false, - deleted: false, - referenceId: "cn=Thomas Williams,ou=Management,dc=bitwarden,dc=com", - externalId: "cn=Thomas Williams,ou=Management,dc=bitwarden,dc=com", - email: "williamst@1f2a3b4c5d6e7f8a9b0c1d2e3f4a5b6c7.bitwarden.com", - }, - { - disabled: false, - deleted: false, - referenceId: "cn=Michelle Brown,ou=Management,dc=bitwarden,dc=com", - externalId: "cn=Michelle Brown,ou=Management,dc=bitwarden,dc=com", - email: "brownm@5d6e7f8a9b0c1d2e3f4a5b6c7d8e9f0a1.bitwarden.com", - }, ]; export const userFixtures = data.map((v) => UserEntry.fromJSON(v)); diff --git a/src/services/ldap-directory.service.integration.spec.ts b/src/services/ldap-directory.service.integration.spec.ts index 0ec25074..7cbf9252 100644 --- a/src/services/ldap-directory.service.integration.spec.ts +++ b/src/services/ldap-directory.service.integration.spec.ts @@ -152,88 +152,4 @@ describe("ldapDirectoryService", () => { expect(result).toEqual([[redTeam], undefined]); }); }); - - describe("new groups and users", () => { - it("fetches DevOps Team and Security Team groups", async () => { - stateService.getDirectory - .calledWith(DirectoryType.Ldap) - .mockResolvedValue(getLdapConfiguration()); - stateService.getSync.mockResolvedValue( - getSyncConfiguration({ - groups: true, - groupFilter: "(|(cn=DevOps Team)(cn=Security Team))", - }), - ); - - const devOpsTeam = groupFixtures.find( - (g) => g.referenceId === "cn=DevOps Team,dc=bitwarden,dc=com", - ); - const securityTeam = groupFixtures.find( - (g) => g.referenceId === "cn=Security Team,dc=bitwarden,dc=com", - ); - - const result = await directoryService.getEntries(true, true); - expect(result[0]).toEqual(expect.arrayContaining([devOpsTeam, securityTeam])); - expect(result[0].length).toEqual(2); - }); - - it("fetches new users with correct group memberships", async () => { - stateService.getDirectory - .calledWith(DirectoryType.Ldap) - .mockResolvedValue(getLdapConfiguration()); - stateService.getSync.mockResolvedValue( - getSyncConfiguration({ - users: true, - groups: true, - userFilter: "(|(uid=ChenB)(uid=SmithK)(uid=JohnsonR)(uid=WilliamsT)(uid=BrownM))", - }), - ); - - const newUsers = userFixtures.filter( - (u) => - u.referenceId === "cn=Benjamin Chen,ou=Product Development,dc=bitwarden,dc=com" || - u.referenceId === "cn=Karen Smith,ou=Product Development,dc=bitwarden,dc=com" || - u.referenceId === "cn=Robert Johnson,ou=Product Development,dc=bitwarden,dc=com" || - u.referenceId === "cn=Thomas Williams,ou=Management,dc=bitwarden,dc=com" || - u.referenceId === "cn=Michelle Brown,ou=Management,dc=bitwarden,dc=com", - ); - - const devOpsTeam = groupFixtures.find( - (g) => g.referenceId === "cn=DevOps Team,dc=bitwarden,dc=com", - ); - const securityTeam = groupFixtures.find( - (g) => g.referenceId === "cn=Security Team,dc=bitwarden,dc=com", - ); - - const result = await directoryService.getEntries(true, true); - - // Verify users are fetched - expect(result[1]).toEqual(expect.arrayContaining(newUsers)); - expect(result[1].length).toEqual(newUsers.length); - - // Verify groups are fetched with correct membership - expect(result[0]).toEqual(expect.arrayContaining([devOpsTeam, securityTeam])); - - // Verify DevOps Team has 3 members - const fetchedDevOpsTeam = result[0].find((g) => g.name === "DevOps Team"); - expect(fetchedDevOpsTeam.userMemberExternalIds.size).toEqual(3); - expect(Array.from(fetchedDevOpsTeam.userMemberExternalIds)).toEqual( - expect.arrayContaining([ - "cn=Benjamin Chen,ou=Product Development,dc=bitwarden,dc=com", - "cn=Karen Smith,ou=Product Development,dc=bitwarden,dc=com", - "cn=Robert Johnson,ou=Product Development,dc=bitwarden,dc=com", - ]), - ); - - // Verify Security Team has 2 members - const fetchedSecurityTeam = result[0].find((g) => g.name === "Security Team"); - expect(fetchedSecurityTeam.userMemberExternalIds.size).toEqual(2); - expect(Array.from(fetchedSecurityTeam.userMemberExternalIds)).toEqual( - expect.arrayContaining([ - "cn=Thomas Williams,ou=Management,dc=bitwarden,dc=com", - "cn=Michelle Brown,ou=Management,dc=bitwarden,dc=com", - ]), - ); - }); - }); }); diff --git a/src/services/ldap-directory.service.ts b/src/services/ldap-directory.service.ts index 47e21640..150fd8d3 100644 --- a/src/services/ldap-directory.service.ts +++ b/src/services/ldap-directory.service.ts @@ -200,7 +200,7 @@ export class LdapDirectoryService implements IDirectoryService { const externalId = this.getExternalId(se, dn); userDnMap.set(dn, externalId); if (uid != null) { - userUidMap.set(uid, externalId); + userUidMap.set(uid.toLowerCase(), externalId); } return se; }); @@ -215,6 +215,19 @@ export class LdapDirectoryService implements IDirectoryService { return entries; } + /** + * Builds a GroupEntry from an LDAP search entry, processing group members and determining member types. + * + * This method creates a group object with its basic properties (referenceId, externalId, name) and + * processes its members to categorize them as either user members (DN or UID format) or nested groups. + * Members are identified by their format: DN-like strings containing "=" and "," are checked against + * user maps to determine if they're user DNs or group DNs, while other strings are treated as UIDs. + * + * @param searchEntry - The LDAP search entry containing group data + * @param userDnMap - Map of user distinguished names to their external IDs + * @param userUidMap - Map of user UIDs to their external IDs + * @returns A populated GroupEntry object, or null if the group lacks required properties + */ private buildGroup( searchEntry: any, userDnMap: Map, @@ -239,6 +252,7 @@ export class LdapDirectoryService implements IDirectoryService { const members = this.getAttrVals(searchEntry, this.syncConfig.memberAttribute); if (members != null) { + // Parses a group member attribute and identifies it as a member DN, member Uid, or a group Dn const getMemberAttributeType = (member: string): "memberDn" | "memberUid" | "groupDn" => { const isDnLike = member.includes("=") && member.includes(","); if (isDnLike) { @@ -257,7 +271,7 @@ export class LdapDirectoryService implements IDirectoryService { break; } case "memberUid": { - const externalId = userUidMap.get(member); + const externalId = userUidMap.get(member.toLowerCase()); if (externalId != null) { group.userMemberExternalIds.add(externalId); } diff --git a/src/services/sync.service.ts b/src/services/sync.service.ts index c7c55d48..9896d82b 100644 --- a/src/services/sync.service.ts +++ b/src/services/sync.service.ts @@ -164,13 +164,8 @@ export class SyncService { } } else { if (!u.deleted) { - // Check that active UserEntry does not conflict with a deleted UserEntry - if (processedDeletedUsers.has(u.email)) { - duplicateEmails.push(u.email); - } else { - processedActiveUsers.set(u.email, JSON.stringify(u)); - uniqueUsers.push(u); - } + processedActiveUsers.set(u.email, JSON.stringify(u)); + uniqueUsers.push(u); } else { // UserEntrys with duplicate email will not throw an error if they are all deleted. They will be synced. processedDeletedUsers.set(u.email, JSON.stringify(u));