From 68aca81f71e29463a1dd617aa02a5555e3cfe427 Mon Sep 17 00:00:00 2001 From: Schalli Date: Thu, 6 Aug 2026 16:21:10 +0200 Subject: [PATCH] feat(16-03): wire group reconciliation before membership sync (5a) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - syncUsersForTenant() now calls syncBoundGroupsForTenant() (5a) BEFORE syncGroupMembershipsForTenant() (5b) — the central correctness ordering of Phase 16 (RESEARCH.md Pitfall 1): a rename detected in the same run must be written back before the memberOf filter is built, or the membership sync would misreport a rename as a membership wipeout - Add observable ordering test (call-order spies), a no-op-guard test, and a regression test proving a memberOf search never uses the stale pre-rename DN - Update the D-21 membership-sync test fixtures to resolve step 5a as a deterministic no-op (DN-derived identity GUID), since the wiring now runs 5a ahead of every syncUsersForTenant() call those tests exercise A1 (objectGUID survives an AD rename) and A2 (binary filter escape syntax) remain unverified against a real directory — no reachable AD in this sandbox. Documented as an outstanding live verification in the plan SUMMARY, not silently skipped. --- apps/api/src/ldap/ldap.service.spec.ts | 215 ++++++++++++++++++++++++- apps/api/src/ldap/ldap.service.ts | 15 ++ 2 files changed, 225 insertions(+), 5 deletions(-) diff --git a/apps/api/src/ldap/ldap.service.spec.ts b/apps/api/src/ldap/ldap.service.spec.ts index 253d9d3..98aad17 100644 --- a/apps/api/src/ldap/ldap.service.spec.ts +++ b/apps/api/src/ldap/ldap.service.spec.ts @@ -550,6 +550,46 @@ describe('LdapService.syncUsersForTenant — AD-bound group membership sync (D-1 fieldMappings: [{ ldapField: 'sAMAccountName', tesseraField: 'username' }], }; + // Plan 16-03 step 5a now runs BEFORE this block's own step-5b search in + // every real syncUsersForTenant() call. This describe block only + // exercises step 5b (membership sync), so 5a must resolve as a + // deterministic no-op by default: every fixture group's own DN-derived + // GUID resolves to an identity hit (same cn/dn as already stored) — no + // rename, no delete, nothing added to result.errors. Exposed at + // describe-scope (not just inside beforeEach) so a test that needs to + // ALSO fail one group's step-5b search (see the "records a search + // failure" test below) can still delegate the 5a shapes to this helper + // instead of re-deriving them. + const guidForDn = (dn: string): Buffer => { + const hex = Buffer.from(dn).toString('hex').padEnd(32, '0').slice(0, 32); + return Buffer.from(hex, 'hex'); + }; + + const resolve5aNoOp = (baseDn: string, filter: string) => { + if (filter === '(objectClass=group)') { + // 5a legacy-binding backfill probe: the probed base DN IS the + // group's own stored ldapDn (scope 'base'). + const g = groups.find((x) => x.ldapDn === baseDn); + return { + searchEntries: g + ? [{ dn: g.ldapDn, cn: g.name, objectGUID: guidForDn(g.ldapDn as string) }] + : [], + }; + } + if (filter.startsWith('(objectGUID=')) { + // 5a existence sweep: match whichever fixture group's DN-derived + // GUID produced this escaped filter. + const g = groups.find( + (x) => + x.ldapDn && + filter === + `(objectGUID=${LdapService.escapeLdapFilterBuffer(guidForDn(x.ldapDn))})`, + ); + return { searchEntries: g ? [{ dn: g.ldapDn, cn: g.name }] : [] }; + } + return null; + }; + beforeEach(() => { vi.clearAllMocks(); mockBind.mockResolvedValue(undefined); @@ -564,10 +604,15 @@ describe('LdapService.syncUsersForTenant — AD-bound group membership sync (D-1 ]; groupSearchEntries = []; - mockSearch.mockImplementation((_baseDn: string, opts: any) => { - if (typeof opts.filter === 'string' && opts.filter.includes('memberOf=')) { + mockSearch.mockImplementation((baseDn: string, opts: any) => { + const filter = typeof opts.filter === 'string' ? opts.filter : ''; + if (filter.includes('memberOf=')) { return Promise.resolve({ searchEntries: groupSearchEntries }); } + const fivea = resolve5aNoOp(baseDn, filter); + if (fivea) { + return Promise.resolve(fivea); + } // Plain user-sync search: no entries in this describe block. return Promise.resolve({ searchEntries: [] }); }); @@ -599,6 +644,16 @@ describe('LdapService.syncUsersForTenant — AD-bound group membership sync (D-1 ), ), ), + // 5a's legacy-binding backfill write (ldapObjectGuid only) — never + // a rename/delete write in this block since the mockSearch above + // always resolves an identity hit. + update: vi.fn((args: any) => { + const g = groups.find((x) => x.id === args.where.id); + if (g) { + Object.assign(g, args.data); + } + return Promise.resolve(g); + }), }, groupMembership: { createMany: vi.fn((args: any) => { @@ -848,15 +903,23 @@ describe('LdapService.syncUsersForTenant — AD-bound group membership sync (D-1 { id: 'm0', groupId: 'g-broken', userId: 'u-bob', source: 'MANUAL' }, ]; - mockSearch.mockImplementation((_baseDn: string, opts: any) => { - if (typeof opts.filter === 'string' && opts.filter.includes('CN=Broken')) { + mockSearch.mockImplementation((baseDn: string, opts: any) => { + const filter = typeof opts.filter === 'string' ? opts.filter : ''; + // Only step 5b's memberOf-filtered search for THIS group fails — 5a + // (reconciliation) resolves as a no-op for both groups via the + // shared helper, so this test isolates the 5b failure it targets. + if (filter.includes('memberOf=') && filter.includes('CN=Broken')) { return Promise.reject(new Error('directory unavailable')); } - if (typeof opts.filter === 'string' && opts.filter.includes('memberOf=')) { + if (filter.includes('memberOf=')) { return Promise.resolve({ searchEntries: [{ dn: 'cn=alice,dc=example,dc=com', sAMAccountName: 'alice' }], }); } + const fivea = resolve5aNoOp(baseDn, filter); + if (fivea) { + return Promise.resolve(fivea); + } return Promise.resolve({ searchEntries: [] }); }); @@ -879,6 +942,148 @@ describe('LdapService.syncUsersForTenant — AD-bound group membership sync (D-1 }); }); +describe('LdapService.syncUsersForTenant — Schrittreihenfolge Gruppen vor Mitgliedschaften', () => { + let service: LdapService; + let prisma: any; + let userService: any; + let groupsService: any; + + const cfg = { + 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' }], + }; + + const guidBuffer = Buffer.from('0123456789abcdef'.repeat(2), 'hex'); + const guidHex = guidBuffer.toString('hex'); + + beforeEach(() => { + vi.clearAllMocks(); + mockBind.mockResolvedValue(undefined); + mockUnbind.mockResolvedValue(undefined); + mockSearch.mockResolvedValue({ searchEntries: [] }); + prisma = { + user: { + findFirst: vi.fn().mockResolvedValue(null), + findMany: vi.fn().mockResolvedValue([]), + update: vi.fn().mockResolvedValue({}), + }, + group: { + findMany: vi.fn().mockResolvedValue([]), + update: vi.fn().mockResolvedValue({}), + }, + groupMembership: { + createMany: vi.fn().mockResolvedValue({ count: 0 }), + deleteMany: vi.fn().mockResolvedValue({ count: 0 }), + }, + ldapConfig: { update: vi.fn().mockResolvedValue({}) }, + }; + userService = { create: vi.fn().mockResolvedValue({}) }; + groupsService = { + reassignDefaultBeforeDelete: vi.fn().mockResolvedValue(false), + ensureDefaultGroup: vi.fn().mockResolvedValue(null), + }; + service = new LdapService(prisma, userService, groupsService); + }); + + it('calls syncBoundGroupsForTenant (5a) before syncGroupMembershipsForTenant (5b) — observed via call-order spies, not just asserted', async () => { + const callOrder: string[] = []; + const originalGroups = (service as any).syncBoundGroupsForTenant.bind(service); + const originalMemberships = (service as any).syncGroupMembershipsForTenant.bind(service); + vi.spyOn(service as any, 'syncBoundGroupsForTenant').mockImplementation( + async (...args: any[]) => { + callOrder.push('syncBoundGroupsForTenant'); + return originalGroups(...args); + }, + ); + vi.spyOn(service as any, 'syncGroupMembershipsForTenant').mockImplementation( + async (...args: any[]) => { + callOrder.push('syncGroupMembershipsForTenant'); + return originalMemberships(...args); + }, + ); + + await service.syncUsersForTenant(cfg as any, 't1'); + + expect(callOrder).toEqual([ + 'syncBoundGroupsForTenant', + 'syncGroupMembershipsForTenant', + ]); + }); + + it('sits behind the same Base-DN no-op guard as the rest of the sync — an empty base DN triggers neither step', async () => { + const callOrder: string[] = []; + vi.spyOn(service as any, 'syncBoundGroupsForTenant').mockImplementation( + async () => { + callOrder.push('syncBoundGroupsForTenant'); + }, + ); + vi.spyOn(service as any, 'syncGroupMembershipsForTenant').mockImplementation( + async () => { + callOrder.push('syncGroupMembershipsForTenant'); + }, + ); + + await service.syncUsersForTenant({ ...cfg, baseDn: ' \n ' } as any, 't1'); + + expect(callOrder).toEqual([]); + expect(mockSearch).not.toHaveBeenCalled(); + }); + + it('regression: a rename detected in 5a is written back before 5b builds its memberOf filter — no memberOf search ever uses the stale pre-rename DN', async () => { + const groups = [ + { + id: 'g1', + tenantId: 't1', + name: 'Sales', + ldapDn: 'cn=Sales,dc=example,dc=com', + ldapObjectGuid: guidHex, + isDefault: false, + }, + ]; + prisma.group.findMany = vi.fn(() => Promise.resolve(groups)); + prisma.group.update = vi.fn((args: any) => { + const g = groups.find((x) => x.id === args.where.id); + if (g) { + Object.assign(g, args.data); + } + return Promise.resolve(g); + }); + + mockSearch.mockImplementation((_baseDn: string, opts: any) => { + const filter = typeof opts.filter === 'string' ? opts.filter : ''; + if (filter.startsWith('(objectGUID=')) { + // 5a's existence sweep: AD reports the group renamed — new cn/dn. + return Promise.resolve({ + searchEntries: [ + { dn: 'cn=Vertrieb,dc=example,dc=com', cn: 'Vertrieb' }, + ], + }); + } + // 5b's memberOf search (and the plain user-sync search) — no hits, + // only the filter STRING passed matters for this test. + return Promise.resolve({ searchEntries: [] }); + }); + + await service.syncUsersForTenant(cfg as any, 't1'); + + // The rename was written back (5a ran and updated the in-memory row). + 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='), + ); + expect(memberOfCall).toBeDefined(); + expect(memberOfCall![1].filter).toContain('cn=Vertrieb,dc=example,dc=com'); + expect(memberOfCall![1].filter).not.toContain('cn=Sales,dc=example,dc=com'); + }); +}); + describe('LdapService.syncBoundGroupsForTenant — Rekonziliation gegen das Verzeichnis (SC-3/SC-4/SC-5, D-05/D-06)', () => { 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 ea14e80..3e08a0e 100644 --- a/apps/api/src/ldap/ldap.service.ts +++ b/apps/api/src/ldap/ldap.service.ts @@ -906,9 +906,24 @@ export class LdapService { } } + // 5a. Plan 16-03 (SC-3/SC-4/SC-5, D-05/D-06/D-07): reconcile every + // AD-bound Group's name/ldapDn/existence BEFORE the membership + // reconciliation below. This ordering is the central correctness + // condition of Phase 16, not a style choice (RESEARCH.md Pitfall 1): + // step 5b reads Group.ldapDn from the DB to build its memberOf + // filter. If a rename detected in THIS SAME run hadn't already been + // written back by the time 5b runs, the memberOf filter would still + // use the stale pre-rename DN, AD would return zero hits for it, and + // every LDAP membership of the renamed group would be misreported as + // removed — a rename would look like a membership wipeout that never + // happened in the directory. + await this.syncBoundGroupsForTenant(client, config, tenantId, result); + // 5b. D-21: reconcile GroupMembership rows for every AD-bound Group in // this same run — no separate sync job, no second button. Runs behind // the Base-DN no-op guard above, exactly like the rest of this method. + // Relies on 5a above having already written back any rename in this + // run (see 5a's comment). await this.syncGroupMembershipsForTenant( client, config,