diff --git a/apps/api/src/ldap/ldap.service.spec.ts b/apps/api/src/ldap/ldap.service.spec.ts index 237646e..84457bd 100644 --- a/apps/api/src/ldap/ldap.service.spec.ts +++ b/apps/api/src/ldap/ldap.service.spec.ts @@ -6,14 +6,51 @@ const mockBind = vi.fn().mockResolvedValue(undefined); const mockSearch = vi.fn(); const mockUnbind = vi.fn().mockResolvedValue(undefined); +// EqualityFilter is mocked as a plain carrier of { attribute, value } — that is +// exactly the contract the existence sweep depends on: the raw GUID Buffer is +// handed to the filter object instead of being escaped into a filter string +// (quick task 260811-f9i). The class is declared INSIDE the factory because +// vi.mock is hoisted above every top-level binding in this file. vi.mock('ldapts', () => ({ Client: vi.fn().mockImplementation(() => ({ bind: mockBind, search: mockSearch, unbind: mockUnbind, })), + EqualityFilter: class { + attribute: string; + value: Buffer | string; + constructor({ + attribute, + value, + }: { + attribute: string; + value: Buffer | string; + }) { + this.attribute = attribute; + this.value = value; + } + }, })); +/** + * The existence sweep passes a FILTER OBJECT carrying the raw GUID bytes, never + * an escaped filter string. Returns the Buffer when a search call is that sweep, + * null otherwise — a string filter of the form "(objectGUID=...)" deliberately + * returns null, so the old broken shape can never satisfy a mock again. + */ +const sweptGuid = (filter: unknown): Buffer | null => { + if ( + filter && + typeof filter === 'object' && + (filter as { attribute?: unknown }).attribute === 'objectGUID' + ) { + const value = (filter as { value?: unknown }).value; + return Buffer.isBuffer(value) ? value : null; + } + return null; +}; + // forTenant just returns the same client in these tests (tenant scoping is not // under test here). vi.mock('../prisma/prisma-tenant.extension', () => ({ @@ -565,7 +602,7 @@ describe('LdapService.syncUsersForTenant — AD-bound group membership sync (D-1 return Buffer.from(hex, 'hex'); }; - const resolve5aNoOp = (baseDn: string, filter: string) => { + const resolve5aNoOp = (baseDn: string, filter: unknown) => { if (filter === '(objectClass=group)') { // 5a legacy-binding backfill probe: the probed base DN IS the // group's own stored ldapDn (scope 'base'). @@ -576,14 +613,12 @@ describe('LdapService.syncUsersForTenant — AD-bound group membership sync (D-1 : [], }; } - if (filter.startsWith('(objectGUID=')) { - // 5a existence sweep: match whichever fixture group's DN-derived - // GUID produced this escaped filter. + const guid = sweptGuid(filter); + if (guid) { + // 5a existence sweep: match whichever fixture group's DN-derived GUID + // the filter object carries. const g = groups.find( - (x) => - x.ldapDn && - filter === - `(objectGUID=${LdapService.escapeLdapFilterBuffer(guidForDn(x.ldapDn))})`, + (x) => x.ldapDn && guidForDn(x.ldapDn as string).equals(guid), ); return { searchEntries: g ? [{ dn: g.ldapDn, cn: g.name }] : [] }; } @@ -609,7 +644,7 @@ describe('LdapService.syncUsersForTenant — AD-bound group membership sync (D-1 if (filter.includes('memberOf=')) { return Promise.resolve({ searchEntries: groupSearchEntries }); } - const fivea = resolve5aNoOp(baseDn, filter); + const fivea = resolve5aNoOp(baseDn, opts.filter); if (fivea) { return Promise.resolve(fivea); } @@ -741,7 +776,7 @@ describe('LdapService.syncUsersForTenant — AD-bound group membership sync (D-1 ); const memberOfCalls = mockSearch.mock.calls.filter(([, opts]) => - (opts.filter as string).includes('memberOf='), + typeof opts.filter === 'string' && opts.filter.includes('memberOf='), ); expect(memberOfCalls).toHaveLength(2); expect(memberOfCalls[0][0]).toBe('dc=a,dc=com'); @@ -764,10 +799,11 @@ describe('LdapService.syncUsersForTenant — AD-bound group membership sync (D-1 await service.syncUsersForTenant(cfg as any, 't1'); const userSyncCall = mockSearch.mock.calls.find( - ([, opts]) => !(opts.filter as string).includes('memberOf='), + ([, opts]) => + typeof opts.filter === 'string' && !opts.filter.includes('memberOf='), ); const groupSyncCall = mockSearch.mock.calls.find(([, opts]) => - (opts.filter as string).includes('memberOf='), + typeof opts.filter === 'string' && opts.filter.includes('memberOf='), ); expect(userSyncCall).toBeDefined(); expect(groupSyncCall).toBeDefined(); @@ -790,7 +826,7 @@ describe('LdapService.syncUsersForTenant — AD-bound group membership sync (D-1 await service.syncUsersForTenant(cfg as any, 't1'); const groupSyncCall = mockSearch.mock.calls.find(([, opts]) => - (opts.filter as string).includes('memberOf='), + typeof opts.filter === 'string' && opts.filter.includes('memberOf='), ); expect(groupSyncCall![1].filter).toBe( '(&(objectClass=person)(memberOf=CN=Sales \\28EMEA\\29\\2a\\5c,DC=ctl,DC=local))', @@ -916,7 +952,7 @@ describe('LdapService.syncUsersForTenant — AD-bound group membership sync (D-1 searchEntries: [{ dn: 'cn=alice,dc=example,dc=com', sAMAccountName: 'alice' }], }); } - const fivea = resolve5aNoOp(baseDn, filter); + const fivea = resolve5aNoOp(baseDn, opts.filter); if (fivea) { return Promise.resolve(fivea); } @@ -1056,8 +1092,7 @@ describe('LdapService.syncUsersForTenant — Schrittreihenfolge Gruppen vor Mitg }); mockSearch.mockImplementation((_baseDn: string, opts: any) => { - const filter = typeof opts.filter === 'string' ? opts.filter : ''; - if (filter.startsWith('(objectGUID=')) { + if (sweptGuid(opts.filter)) { // 5a's existence sweep: AD reports the group renamed — new cn/dn. return Promise.resolve({ searchEntries: [ @@ -1076,7 +1111,7 @@ describe('LdapService.syncUsersForTenant — Schrittreihenfolge Gruppen vor Mitg expect(groups[0].ldapDn).toBe('cn=Vertrieb,dc=example,dc=com'); const memberOfCall = mockSearch.mock.calls.find(([, opts]: any) => - (opts.filter as string).includes('memberOf='), + typeof opts.filter === 'string' && opts.filter.includes('memberOf='), ); expect(memberOfCall).toBeDefined(); expect(memberOfCall![1].filter).toContain('cn=Vertrieb,dc=example,dc=com'); @@ -1584,7 +1619,8 @@ describe('LdapService.syncBoundGroupsForTenant — Rekonziliation gegen das Verz }, ]; mockSearch.mockImplementation((_baseDn: string, opts: any) => { - if (opts.filter.includes(LdapService.escapeLdapFilterBuffer(guidBuffer))) { + const swept = sweptGuid(opts.filter); + if (swept && swept.equals(guidBuffer)) { return Promise.reject(new Error('directory unavailable')); } return Promise.resolve({ @@ -1600,6 +1636,100 @@ describe('LdapService.syncBoundGroupsForTenant — Rekonziliation gegen das Verz expect(groups.find((g) => g.id === 'g-ok')).toBeDefined(); }); + // Quick task 260811-f9i. The sweep used to interpolate a byte-wise `\xx` + // escape into a filter STRING. Measured read-only against a real AD on + // 2026-08-11, that filter returned zero hits for an object whose GUID had + // just been read from that same directory — so every bound group looked + // deleted and would have been removed with its memberships and module grants + // on the first real sync. These two tests pin the shape that actually works. + it('passes the GUID as raw Buffer bytes on a filter object, never as an escaped filter string', async () => { + groups = [ + { + id: 'g1', + tenantId: 't1', + name: 'Sales', + ldapDn: 'cn=Sales,dc=example,dc=com', + ldapObjectGuid: guidHex, + isDefault: false, + }, + ]; + mockSearch.mockResolvedValue({ + searchEntries: [{ dn: 'cn=Sales,dc=example,dc=com', cn: 'Sales' }], + }); + + await run(makeResult()); + + const sweepCalls = mockSearch.mock.calls.filter(([, opts]: any) => + sweptGuid(opts.filter), + ); + expect(sweepCalls.length).toBeGreaterThan(0); + + for (const [, opts] of sweepCalls as any[]) { + const value = opts.filter.value; + expect(Buffer.isBuffer(value)).toBe(true); + // Byte-for-byte the stored hex, not a re-encoding of it. + expect((value as Buffer).equals(guidBuffer)).toBe(true); + } + + // No search in this run may carry a stringly-typed objectGUID filter — + // that is the exact shape the directory refused to match. + const stringGuidFilters = mockSearch.mock.calls.filter( + ([, opts]: any) => + typeof opts.filter === 'string' && opts.filter.includes('objectGUID='), + ); + expect(stringGuidFilters).toEqual([]); + }); + + it('reuses the same raw-Buffer filter for the wider WR-03 move sweep', async () => { + groups = [ + { + id: 'g1', + tenantId: 't1', + name: 'Sales', + ldapDn: 'cn=Sales,ou=Vertrieb,dc=example,dc=com', + ldapObjectGuid: guidHex, + isDefault: false, + }, + ]; + // A COPY — `cfg` is shared across this describe block and never reset in + // beforeEach, so mutating it here would leak into later tests. + const narrowCfg = { ...cfg, baseDn: 'ou=Vertrieb,dc=example,dc=com' }; + // Narrow sweep misses, wide sweep over the domain root finds it — + // the move case WR-03 protects. Both must carry the same Buffer, or the + // protection inherits a broken filter and deletes the group anyway. + mockSearch.mockImplementation((baseDn: string) => + Promise.resolve( + baseDn === 'dc=example,dc=com' + ? { + searchEntries: [ + { dn: 'cn=Sales,ou=Anders,dc=example,dc=com', cn: 'Sales' }, + ], + } + : { searchEntries: [] }, + ), + ); + + const result = makeResult(); + await (service as any).syncBoundGroupsForTenant( + client, + narrowCfg, + 't1', + result, + ); + + const sweepCalls = mockSearch.mock.calls.filter(([, opts]: any) => + sweptGuid(opts.filter), + ); + // Narrow (configured base DN) plus wide (domain root). + expect(sweepCalls.length).toBe(2); + for (const [, opts] of sweepCalls as any[]) { + expect((opts.filter.value as Buffer).equals(guidBuffer)).toBe(true); + } + expect(prisma.group.delete).not.toHaveBeenCalled(); + expect(result.errors.length).toBe(1); + expect(result.errors[0]).toContain('Nicht geloescht'); + }); + it('is idempotent: a second run over an unchanged AD state issues no group.update or group.delete call', async () => { groups = [ { diff --git a/apps/api/src/ldap/ldap.service.ts b/apps/api/src/ldap/ldap.service.ts index a61c3e2..190e0e1 100644 --- a/apps/api/src/ldap/ldap.service.ts +++ b/apps/api/src/ldap/ldap.service.ts @@ -1,5 +1,5 @@ import { Injectable, Logger } from '@nestjs/common'; -import { Client, Entry } from 'ldapts'; +import { Client, EqualityFilter, Entry } from 'ldapts'; import { PrismaService } from '../prisma/prisma.service'; import { forTenant } from '../prisma/prisma-tenant.extension'; import { GroupsService } from '../groups/groups.service'; @@ -1179,9 +1179,9 @@ export class LdapService { * 2. Existence sweep: the stored hex ldapObjectGuid is validated as * exactly 32 [0-9a-f] characters BEFORE it is ever turned into a * filter (T-16-01) — an invalid value is an error line, never a filter - * interpolation. A valid value is turned back into a Buffer and - * byte-wise escaped via escapeLdapFilterBuffer() into an - * (objectGUID=...) filter, searched across every configured base DN. + * interpolation. A valid value is turned back into a Buffer and handed + * to an EqualityFilter over objectGUID — raw bytes, never an escaped + * filter string — searched across every configured base DN. * 3. A hit whose cn/dn differ from the stored name/ldapDn is a rename * (SC-3): Group.name/ldapDn are updated to the AD state, * groupsRenamed++. internalName is NEVER written here (D-04). A @@ -1315,7 +1315,21 @@ export class LdapService { continue; } const guidBuffer = Buffer.from(ldapObjectGuid, 'hex'); - const filter = `(objectGUID=${LdapService.escapeLdapFilterBuffer(guidBuffer)})`; + // The GUID goes onto the wire as raw bytes via an EqualityFilter, NOT + // as a `\xx`-escaped filter string. A string filter is parsed by ldapts + // before it is encoded, and the parser does not turn `\1e\4b...` back + // into the 16 bytes it stands for — the assertion value that reaches the + // directory is then a different value entirely and matches nothing. + // Measured read-only against a real AD on 2026-08-11 (see the quick task + // 260811-f9i): the escaped string returned 0 hits for an object whose + // GUID had just been read from that same directory, while this + // EqualityFilter returned exactly that object. Both sweeps below share + // this value, so the WR-03 move-detection cannot silently inherit a + // broken filter again. + const filter = new EqualityFilter({ + attribute: 'objectGUID', + value: guidBuffer, + }); let hit: Entry | null = null; for (const baseDn of baseDns) { @@ -1503,20 +1517,14 @@ export class LdapService { .replace(/\x00/g, '\\00'); } - /** - * Escape a binary value (e.g. a stored objectGUID) for use in an LDAP - * search filter per RFC 4515 — a byte-wise `\XX` hex escape, distinct from - * escapeLdapFilterValue() which escapes a STRING value. Not yet called - * anywhere in this plan (Plan 16-01 only WRITES ldapObjectGuid); Plan - * 16-03's existence sweep is the first caller, reading it back via a - * binary (objectGUID=...) filter. [ASSUMED — RFC 4515-Praxis, nicht gegen - * ein echtes AD verifiziert, siehe RESEARCH.md Pattern 3/A2.] - */ - static escapeLdapFilterBuffer(buf: Buffer): string { - return Array.from(buf) - .map((b) => '\\' + b.toString(16).padStart(2, '0')) - .join(''); - } + // NOTE: escapeLdapFilterBuffer() used to live here — a byte-wise `\XX` hex + // escape for binary values, written under the assumption (RESEARCH.md A2) + // that ldapts would pass such a string through to the directory unchanged. + // It does not, and the resulting filter matched nothing; the existence sweep + // in syncBoundGroupsForTenant() now builds an EqualityFilter over the raw + // Buffer instead. Do not reintroduce it: escapeLdapFilterValue() below is for + // STRING values and stays correct, but binary values belong in a filter + // object, never in an interpolated filter string. /** * Split a DN into its individual RDN components, respecting a