diff --git a/apps/api/src/ldap/ldap.service.spec.ts b/apps/api/src/ldap/ldap.service.spec.ts index cc3eb90..5303994 100644 --- a/apps/api/src/ldap/ldap.service.spec.ts +++ b/apps/api/src/ldap/ldap.service.spec.ts @@ -34,9 +34,10 @@ describe('LdapService.syncUsersForTenant — per-user exclude list', () => { serverUrl: 'ldap://example', baseDn: 'dc=example,dc=com', searchFilter: '(objectClass=person)', - // Non-empty selection: an empty groupFilterDns is now a no-op (see the - // dedicated "empty selection no-op" describe block below), so these - // exclude-list tests need a real selection to exercise the search path. + // The Base-DN is the sync scope (an empty parsed base-DN list is the + // sole no-op path — see the dedicated "empty base DN no-op" describe + // block below). groupFilterDns here is an optional extra restriction; + // set to exercise the memberOf-restricted search path. groupFilterDns: ['ou=people,dc=example,dc=com'] as string[], userExcludeList: [] as string[], fieldMappings: [ @@ -118,16 +119,16 @@ describe('LdapService.syncUsersForTenant — per-user exclude list', () => { }); }); -describe('LdapService.syncUsersForTenant — empty selection no-op', () => { +describe('LdapService.syncUsersForTenant — empty base DN no-op', () => { let service: LdapService; let prisma: any; let userService: any; - const emptySelectionConfig = { + const emptyBaseDnConfig = { id: 'cfg1', tenantId: 't1', serverUrl: 'ldap://example', - baseDn: 'dc=example,dc=com', + baseDn: '', searchFilter: '(objectClass=person)', groupFilterDns: [] as string[], userExcludeList: [] as string[], @@ -152,9 +153,9 @@ describe('LdapService.syncUsersForTenant — empty selection no-op', () => { service = new LdapService(prisma, userService); }); - it('creates nobody and deactivates nobody when groupFilterDns is empty', async () => { + it('creates nobody and deactivates nobody when the base DN is empty (whitespace-only)', async () => { const result = await service.syncUsersForTenant( - emptySelectionConfig as any, + { ...emptyBaseDnConfig, baseDn: ' \n \n' } as any, 't1', ); @@ -165,6 +166,94 @@ describe('LdapService.syncUsersForTenant — empty selection no-op', () => { expect(prisma.user.update).not.toHaveBeenCalled(); expect(prisma.ldapConfig.update).not.toHaveBeenCalled(); }); + + it('is NOT a no-op when baseDn is set but groupFilterDns is empty (normal multi-base search)', async () => { + mockSearch.mockResolvedValue({ + searchEntries: [{ dn: 'cn=alice', sAMAccountName: 'alice' }], + }); + + const result = await service.syncUsersForTenant( + { ...emptyBaseDnConfig, baseDn: 'dc=example,dc=com' } as any, + 't1', + ); + + expect(mockBind).toHaveBeenCalled(); + expect(mockSearch).toHaveBeenCalled(); + expect(result.created).toBe(1); + expect(userService.create).toHaveBeenCalledTimes(1); + }); +}); + +describe('LdapService.syncUsersForTenant — multi base DN scope', () => { + let service: LdapService; + let prisma: any; + let userService: any; + + const multiBaseConfig = { + id: 'cfg1', + tenantId: 't1', + serverUrl: 'ldap://example', + baseDn: 'dc=a,dc=com\ndc=b,dc=com', + searchFilter: '(objectClass=person)', + groupFilterDns: [] as string[], + userExcludeList: [] as string[], + fieldMappings: [ + { ldapField: 'sAMAccountName', tesseraField: 'username' }, + ], + }; + + beforeEach(() => { + vi.clearAllMocks(); + mockBind.mockResolvedValue(undefined); + mockUnbind.mockResolvedValue(undefined); + prisma = { + user: { + findFirst: vi.fn().mockResolvedValue(null), + findMany: vi.fn().mockResolvedValue([]), + update: vi.fn().mockResolvedValue({}), + }, + ldapConfig: { update: vi.fn().mockResolvedValue({}) }, + }; + userService = { create: vi.fn().mockResolvedValue({}) }; + service = new LdapService(prisma, userService); + }); + + it('searches every configured base DN and merges/dedupes results by dn', async () => { + mockSearch + .mockResolvedValueOnce({ + searchEntries: [ + { dn: 'cn=shared,dc=a,dc=com', sAMAccountName: 'shared' }, + { dn: 'cn=alice,dc=a,dc=com', sAMAccountName: 'alice' }, + ], + }) + .mockResolvedValueOnce({ + searchEntries: [ + { dn: 'cn=shared,dc=a,dc=com', sAMAccountName: 'shared' }, + { dn: 'cn=bob,dc=b,dc=com', sAMAccountName: 'bob' }, + ], + }); + + const result = await service.syncUsersForTenant( + multiBaseConfig as any, + 't1', + ); + + expect(mockSearch).toHaveBeenCalledTimes(2); + expect(mockSearch).toHaveBeenNthCalledWith( + 1, + 'dc=a,dc=com', + expect.objectContaining({ filter: '(objectClass=person)' }), + ); + expect(mockSearch).toHaveBeenNthCalledWith( + 2, + 'dc=b,dc=com', + expect.objectContaining({ filter: '(objectClass=person)' }), + ); + // 3 distinct dns (shared, alice, bob) — the duplicate "shared" dn from + // the second base is deduped, not double-created. + expect(result.created).toBe(3); + expect(userService.create).toHaveBeenCalledTimes(3); + }); }); describe('LdapService — individual user search & import (dedup)', () => { diff --git a/apps/api/src/ldap/ldap.service.ts b/apps/api/src/ldap/ldap.service.ts index f787055..461f16c 100644 --- a/apps/api/src/ldap/ldap.service.ts +++ b/apps/api/src/ldap/ldap.service.ts @@ -188,9 +188,27 @@ export class LdapService { } /** - * Discover groups and organizational units under the configured base DN. + * Split a `\n`-separated Base-DN admin field into a trimmed, non-empty DN + * list. The baseDn column stays a single String (no schema change) — this + * is the sole place that turns it into the list every search path loops + * over. Blank/whitespace-only lines are dropped; undefined/empty input + * yields []. + */ + private parseBaseDns(baseDn?: string | null): string[] { + if (!baseDn) { + return []; + } + return baseDn + .split(/\r?\n/) + .map((line) => line.trim()) + .filter((line) => line.length > 0); + } + + /** + * Discover groups and organizational units under EVERY configured base DN. * Used by the admin UI to build a selective import filter (groupFilterDns). - * Read-only directory query using the service-account bind. + * Read-only directory query using the service-account bind. Results from + * all base DNs are merged and deduped by entry dn. */ async listGroups(config: { serverUrl: string; @@ -206,13 +224,21 @@ export class LdapService { try { await this.bind(client, config.bindDn, config.bindPassword); - const { searchEntries } = await client.search(config.baseDn, { - filter: '(|(objectClass=group)(objectClass=organizationalUnit))', - attributes: ['cn', 'ou', 'dn'], - scope: 'sub', - }); + const baseDns = this.parseBaseDns(config.baseDn); + const entriesByDn = new Map(); - return searchEntries.map((entry) => { + for (const baseDn of baseDns) { + const { searchEntries } = await client.search(baseDn, { + filter: '(|(objectClass=group)(objectClass=organizationalUnit))', + attributes: ['cn', 'ou', 'dn'], + scope: 'sub', + }); + for (const entry of searchEntries) { + entriesByDn.set(entry.dn, entry); + } + } + + return Array.from(entriesByDn.values()).map((entry) => { const dn = entry.dn; const isOu = /^ou=/i.test(dn); const rawName = isOu ? entry['ou'] : entry['cn']; @@ -350,14 +376,21 @@ export class LdapService { const q = LdapService.escapeLdapFilterValue(trimmed); const filter = `(&(objectClass=person)(|(cn=*${q}*)(sAMAccountName=*${q}*)(displayName=*${q}*)(mail=*${q}*)))`; - const { searchEntries } = await client.search(config.baseDn, { - filter, - attributes: ['cn', 'displayName', 'sAMAccountName', 'mail', 'dn'], - scope: 'sub', - sizeLimit: 50, - }); + const baseDns = this.parseBaseDns(config.baseDn); + const entriesByDn = new Map(); + for (const baseDn of baseDns) { + const { searchEntries } = await client.search(baseDn, { + filter, + attributes: ['cn', 'displayName', 'sAMAccountName', 'mail', 'dn'], + scope: 'sub', + sizeLimit: 50, + }); + for (const entry of searchEntries) { + entriesByDn.set(entry.dn, entry); + } + } - const entries = searchEntries.map((entry) => ({ + const entries = Array.from(entriesByDn.values()).map((entry) => ({ dn: entry.dn, username: first(entry['sAMAccountName']), displayName: first(entry['displayName']) || first(entry['cn']), @@ -534,14 +567,17 @@ export class LdapService { errors: [], }; - // DELIBERATE back-compat break + critical safety guard: an empty/undefined - // groupFilterDns now means "sync nothing", not "import everyone under - // baseDn". Returning here BEFORE any LDAP search and BEFORE the - // deactivation loop below is what prevents an empty selection from - // mass-deactivating every existing LDAP user (there would be no synced - // DNs to compare against, so every local LDAP user would look "removed"). + // CRITICAL SAFETY GUARD: the Base-DN(s) are now the sync scope. Returning + // here BEFORE any LDAP bind/search and BEFORE the deactivation loop below + // is what prevents an unconfigured/blank config from mass-deactivating + // every existing LDAP user (there would be no synced DNs to compare + // against, so every local LDAP user would look "removed"). This is the + // SOLE no-op condition: an empty groupFilterDns no longer short-circuits + // here — with >=1 base DN configured, a normal multi-base search runs + // (see collectSearchEntries), it just applies no memberOf restriction. // lastSyncAt is intentionally left untouched on this no-op path. - if (!config.groupFilterDns || config.groupFilterDns.length === 0) { + const baseDns = this.parseBaseDns(config.baseDn); + if (baseDns.length === 0) { return result; } @@ -681,17 +717,22 @@ export class LdapService { } /** - * Run the directory search for syncUsersForTenant, applying the selective - * group/OU import filter (groupFilterDns) when one is configured. + * Run the directory search for syncUsersForTenant across EVERY configured + * Base DN (the primary sync scope — see syncUsersForTenant's early-return + * guard, which is the sole no-op path and is keyed on the parsed base-DN + * list, not on groupFilterDns). groupFilterDns is an OPTIONAL extra + * restriction: * - * - Empty groupFilterDns: DELIBERATE no-op — returns [] without ever - * searching the directory. A selective sync now means "nothing selected - * = nothing synced", not "import everyone under baseDn" (back-compat - * break, see syncUsersForTenant's early-return guard which normally - * short-circuits before this method is even reached). - * - Non-empty groupFilterDns: split into OU DNs (used as extra search - * bases) and group DNs (matched via memberOf on the base search). - * Results are merged and deduped by entry dn. + * - Empty groupFilterDns: each base DN is searched with the plain + * sanitizedFilter — no memberOf restriction, every user under the base + * DN(s) is synced. + * - Non-empty groupFilterDns: split into OU DNs (searched as ADDITIONAL + * extra bases with the plain filter) and group DNs (turned into a + * memberOf OR-clause ANDed with sanitizedFilter and applied to every + * base DN search). + * + * All results across every base DN (and any extra OU bases) are merged + * and deduped by entry dn. */ private async collectSearchEntries( client: Client, @@ -699,18 +740,33 @@ export class LdapService { sanitizedFilter: string, attributes: string[], ) { - if (!config.groupFilterDns || config.groupFilterDns.length === 0) { - return []; - } - - const ouBases = config.groupFilterDns.filter((dn) => /^ou=/i.test(dn)); - const groupDns = config.groupFilterDns.filter((dn) => !/^ou=/i.test(dn)); + const baseDns = this.parseBaseDns(config.baseDn); + const ouBases = (config.groupFilterDns ?? []).filter((dn) => + /^ou=/i.test(dn), + ); + const groupDns = (config.groupFilterDns ?? []).filter( + (dn) => !/^ou=/i.test(dn), + ); const entriesByDn = new Map(); - for (const ouBase of ouBases) { - const { searchEntries } = await client.search(ouBase, { - filter: sanitizedFilter, + // Build the base-search filter: plain sanitizedFilter, optionally ANDed + // with a memberOf OR-clause when group DNs are configured. + let baseFilter = sanitizedFilter; + if (groupDns.length > 0) { + const memberOfClauses = groupDns + .map( + (dn) => `(memberOf=${LdapService.escapeLdapFilterValue(dn)})`, + ) + .join(''); + baseFilter = `(&${sanitizedFilter}(|${memberOfClauses}))`; + } + + // Search every configured base DN with the (possibly memberOf-restricted) + // base filter. + for (const baseDn of baseDns) { + const { searchEntries } = await client.search(baseDn, { + filter: baseFilter, attributes, scope: 'sub', }); @@ -719,16 +775,11 @@ export class LdapService { } } - if (groupDns.length > 0) { - const memberOfClauses = groupDns - .map( - (dn) => `(memberOf=${LdapService.escapeLdapFilterValue(dn)})`, - ) - .join(''); - const combinedFilter = `(&${sanitizedFilter}(|${memberOfClauses}))`; - - const { searchEntries } = await client.search(config.baseDn, { - filter: combinedFilter, + // ou= group-filter entries are ADDITIONAL search bases, searched with + // the plain sanitizedFilter (no memberOf restriction). + for (const ouBase of ouBases) { + const { searchEntries } = await client.search(ouBase, { + filter: sanitizedFilter, attributes, scope: 'sub', });