From 57bc7f96b3808da7e2d8a49e22650989bc8191ae Mon Sep 17 00:00:00 2001 From: Schalli Date: Tue, 28 Jul 2026 15:41:33 +0200 Subject: [PATCH] feat(260728-lih): make LDAP sync strictly selective (empty selection = no-op) - collectSearchEntries() returns [] on empty/undefined groupFilterDns instead of scanning the whole baseDn subtree - syncUsersForTenant() early-returns an empty successful result before any LDAP search or the deactivation loop when groupFilterDns is empty, so an empty selection can never mass-deactivate existing LDAP users - Updated exclude-list tests to use a non-empty groupFilterDns; added a dedicated no-op test proving empty selection performs zero search/ create/update/deactivate operations Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/api/src/ldap/ldap.service.spec.ts | 54 +++++++++++++++++++++++++- apps/api/src/ldap/ldap.service.ts | 25 ++++++++---- 2 files changed, 70 insertions(+), 9 deletions(-) diff --git a/apps/api/src/ldap/ldap.service.spec.ts b/apps/api/src/ldap/ldap.service.spec.ts index e8d9d10..cc3eb90 100644 --- a/apps/api/src/ldap/ldap.service.spec.ts +++ b/apps/api/src/ldap/ldap.service.spec.ts @@ -34,7 +34,10 @@ describe('LdapService.syncUsersForTenant — per-user exclude list', () => { serverUrl: 'ldap://example', baseDn: 'dc=example,dc=com', searchFilter: '(objectClass=person)', - groupFilterDns: [] as string[], + // 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. + groupFilterDns: ['ou=people,dc=example,dc=com'] as string[], userExcludeList: [] as string[], fieldMappings: [ { ldapField: 'sAMAccountName', tesseraField: 'username' }, @@ -115,6 +118,55 @@ describe('LdapService.syncUsersForTenant — per-user exclude list', () => { }); }); +describe('LdapService.syncUsersForTenant — empty selection no-op', () => { + let service: LdapService; + let prisma: any; + let userService: any; + + const emptySelectionConfig = { + id: 'cfg1', + tenantId: 't1', + serverUrl: 'ldap://example', + baseDn: 'dc=example,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([{ id: 'u-existing', ldapDn: 'cn=existing' }]), + update: vi.fn().mockResolvedValue({}), + }, + ldapConfig: { update: vi.fn().mockResolvedValue({}) }, + }; + userService = { create: vi.fn().mockResolvedValue({}) }; + service = new LdapService(prisma, userService); + }); + + it('creates nobody and deactivates nobody when groupFilterDns is empty', async () => { + const result = await service.syncUsersForTenant( + emptySelectionConfig as any, + 't1', + ); + + expect(result).toEqual({ created: 0, updated: 0, deactivated: 0, errors: [] }); + expect(mockSearch).not.toHaveBeenCalled(); + expect(mockBind).not.toHaveBeenCalled(); + expect(userService.create).not.toHaveBeenCalled(); + expect(prisma.user.update).not.toHaveBeenCalled(); + expect(prisma.ldapConfig.update).not.toHaveBeenCalled(); + }); +}); + describe('LdapService — individual user search & import (dedup)', () => { let service: LdapService; let prisma: any; diff --git a/apps/api/src/ldap/ldap.service.ts b/apps/api/src/ldap/ldap.service.ts index 45787a9..f787055 100644 --- a/apps/api/src/ldap/ldap.service.ts +++ b/apps/api/src/ldap/ldap.service.ts @@ -534,6 +534,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"). + // lastSyncAt is intentionally left untouched on this no-op path. + if (!config.groupFilterDns || config.groupFilterDns.length === 0) { + return result; + } + const client = new Client( this.buildClientOptions(config.serverUrl, config.tlsRejectUnauthorized), ); @@ -673,8 +684,11 @@ export class LdapService { * Run the directory search for syncUsersForTenant, applying the selective * group/OU import filter (groupFilterDns) when one is configured. * - * - Empty groupFilterDns: single search of baseDn with sanitizedFilter - * (identical to pre-filter behavior, backward compatible). + * - 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. @@ -686,12 +700,7 @@ export class LdapService { attributes: string[], ) { if (!config.groupFilterDns || config.groupFilterDns.length === 0) { - const { searchEntries } = await client.search(config.baseDn, { - filter: sanitizedFilter, - attributes, - scope: 'sub', - }); - return searchEntries; + return []; } const ouBases = config.groupFilterDns.filter((dn) => /^ou=/i.test(dn));