From 00de6a6f9c7eb937f69418306d0956c149ee8ef5 Mon Sep 17 00:00:00 2001 From: Schalli Date: Thu, 6 Aug 2026 17:00:54 +0200 Subject: [PATCH] fix(16): WR-03 do not delete a bound group merely unobserved under a narrower base DN MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit syncBoundGroupsForTenant()'s existence sweep only searched the configured base DNs, so an AD group MOVED to an OU outside that subtree (still present in the directory) was indistinguishable from a genuine disappearance and got deleted along with its memberships/module grants — a silent access loss from a non-destructive AD operation, and a bigger blast radius than D-05 ("group genuinely gone") was accepted for. Before concluding disappearance, a second (objectGUID=...) sweep now runs against each base DN's own domain root (skipped when a base DN already IS its domain root — the common case, nothing wider to search). A hit there is reported as an error line and the group is left untouched; only when the wide sweep also finds nothing is deletion (SC-4/D-05/D-06) actually established — mirroring the existing conservative stance already taken for a legacy binding whose DN no longer resolves. Deleting on uncertainty was the failure mode; this closes it without widening it. --- apps/api/src/ldap/ldap.service.spec.ts | 113 ++++++++++++++++++++++++ apps/api/src/ldap/ldap.service.ts | 116 ++++++++++++++++++++++++- 2 files changed, 227 insertions(+), 2 deletions(-) diff --git a/apps/api/src/ldap/ldap.service.spec.ts b/apps/api/src/ldap/ldap.service.spec.ts index 814c887..237646e 100644 --- a/apps/api/src/ldap/ldap.service.spec.ts +++ b/apps/api/src/ldap/ldap.service.spec.ts @@ -1657,6 +1657,119 @@ describe('LdapService.syncBoundGroupsForTenant — Rekonziliation gegen das Verz expect(groups[0].name).toBe('Sales'); expect(groups[0].ldapDn).toBe('CN=Sales,DC=example,DC=com'); }); + + it('a bound group not found under the configured (narrower) base DN, but found under the wider domain root, is NOT deleted — reported instead (WR-03, 16-REVIEW.md)', async () => { + // The configured base DN is an OU beneath the domain root — an AD + // group moved OUT of that OU (into ou=Archive, still under the same + // domain) is a directory-internal move, not a deletion. + const narrowCfg = { ...cfg, baseDn: 'ou=Sales,dc=example,dc=com' }; + groups = [ + { + id: 'g1', + tenantId: 't1', + name: 'Sales', + ldapDn: 'cn=Sales,ou=Sales,dc=example,dc=com', + ldapObjectGuid: guidHex, + isDefault: false, + }, + ]; + mockSearch.mockImplementation((baseDn: string) => { + if (baseDn === 'ou=Sales,dc=example,dc=com') { + // Nothing found under the configured (narrow) base DN. + return Promise.resolve({ searchEntries: [] }); + } + if (baseDn === 'dc=example,dc=com') { + // But the group is still there, just moved to a different OU under + // the same domain — the WR-03 wide sweep must catch this. + return Promise.resolve({ + searchEntries: [ + { dn: 'cn=Sales,ou=Archive,dc=example,dc=com', cn: 'Sales' }, + ], + }); + } + throw new Error(`unexpected base DN in test: ${baseDn}`); + }); + + const result = makeResult(); + await (service as any).syncBoundGroupsForTenant( + client, + narrowCfg, + 't1', + result, + ); + + expect(prisma.group.delete).not.toHaveBeenCalled(); + expect(prisma.group.update).not.toHaveBeenCalled(); + expect(result.groupsDeleted).toBe(0); + expect(result.errors).toEqual([ + "Gruppe Sales: nicht mehr unter den konfigurierten Base-DNs gefunden, existiert aber weiterhin unter 'cn=Sales,ou=Archive,dc=example,dc=com' — vermutlich im Verzeichnis verschoben, Base-DN-Konfiguration pruefen. Nicht geloescht.", + ]); + expect(groups).toHaveLength(1); + }); + + it('a bound group not found under the configured base DN AND not found under the wider domain root either is genuinely deleted (WR-03, 16-REVIEW.md)', async () => { + const narrowCfg = { ...cfg, baseDn: 'ou=Sales,dc=example,dc=com' }; + groups = [ + { + id: 'g1', + tenantId: 't1', + name: 'Sales', + ldapDn: 'cn=Sales,ou=Sales,dc=example,dc=com', + ldapObjectGuid: guidHex, + isDefault: false, + }, + { + id: 'g-other', + tenantId: 't1', + name: 'Alle Benutzer', + ldapDn: null, + ldapObjectGuid: null, + isDefault: true, + }, + ]; + // Empty everywhere: under the configured OU AND under the domain root. + mockSearch.mockResolvedValue({ searchEntries: [] }); + + const result = makeResult(); + await (service as any).syncBoundGroupsForTenant( + client, + narrowCfg, + 't1', + result, + ); + + expect(prisma.group.delete).toHaveBeenCalledWith({ where: { id: 'g1' } }); + expect(result.groupsDeleted).toBe(1); + expect(result.errors).toEqual([]); + }); + + it('skips the wide fallback sweep entirely when the configured base DN already IS the domain root — no redundant client.search call (WR-03, 16-REVIEW.md)', async () => { + groups = [ + { + id: 'g1', + tenantId: 't1', + name: 'Sales', + ldapDn: 'cn=Sales,dc=example,dc=com', + ldapObjectGuid: guidHex, + isDefault: false, + }, + { + id: 'g-other', + tenantId: 't1', + name: 'Alle Benutzer', + ldapDn: null, + ldapObjectGuid: null, + isDefault: true, + }, + ]; + mockSearch.mockResolvedValue({ searchEntries: [] }); + + const result = makeResult(); + await run(result); // cfg.baseDn === 'dc=example,dc=com', already the domain root + + expect(mockSearch).toHaveBeenCalledTimes(1); + expect(result.groupsDeleted).toBe(1); + }); }); describe('LdapService — AD group import (SC-1/SC-2, D-01/D-02)', () => { diff --git a/apps/api/src/ldap/ldap.service.ts b/apps/api/src/ldap/ldap.service.ts index ed42805..a61c3e2 100644 --- a/apps/api/src/ldap/ldap.service.ts +++ b/apps/api/src/ldap/ldap.service.ts @@ -1188,7 +1188,25 @@ export class LdapService { * resulting P2002 (the new name collides with an existing local group) * is caught, reported as an error line, and the group is left * unchanged — the run continues with the remaining candidates. - * 4. No hit is a disappearance (SC-4/D-05): the default-marker handoff + * 4. No hit under any configured base DN is NOT automatically treated as a + * disappearance (WR-03, 16-REVIEW.md). The base-DN sweep only proves + * "not found under these specific base DNs" — an AD group that was + * MOVED to an OU outside the configured subtree (still present in the + * directory) would otherwise be misread as gone and deleted along with + * its memberships/module grants, which is a bigger blast radius than + * D-05 ("group genuinely disappeared") was accepted for. Before + * concluding disappearance, a second, wider (objectGUID=...) sweep runs + * against each base DN's own domain root (the trailing DC=... RDN + * chain, e.g. "ou=Sales,dc=example,dc=com" -> "dc=example,dc=com"), + * skipping any root already covered by a configured base DN (the + * common case where baseDn already IS the domain root — nothing wider + * to search). A hit there means the group still exists, just outside + * the configured subtree: reported as an error line, NOT deleted — the + * same conservative "absence must be established, never merely + * unobserved" stance already taken for a legacy binding whose DN no + * longer resolves (case 1 above). Only when the wide sweep ALSO finds + * nothing (or there is no wider root left to search) is disappearance + * (SC-4/D-05) actually established: the default-marker handoff * (reassignDefaultBeforeDelete) runs BEFORE the delete — this ordering * is the actual correctness guarantee of D-06, not a style choice. A * resulting P2025 (already gone, e.g. a concurrent manual delete) is @@ -1236,6 +1254,21 @@ export class LdapService { const baseDns = this.parseBaseDns(config.baseDn); let anyDeleted = false; + // WR-03 (16-REVIEW.md): domain root(s) NOT already covered by a + // configured base DN — the fallback anchor for the wide existence sweep + // below, so a group moved outside every configured base DN is not + // misread as deleted. Computed once per run (identical for every + // candidate), not per-group. + const domainRoots = Array.from( + new Set( + baseDns + .map((dn) => LdapService.domainRootOf(dn)) + .filter( + (dn): dn is string => dn !== null && !baseDns.includes(dn), + ), + ), + ); + for (const group of candidates) { try { let ldapObjectGuid = group.ldapObjectGuid; @@ -1358,7 +1391,35 @@ export class LdapService { } } } else { - // 4. Disappearance (SC-4/D-05/D-06) — handoff BEFORE delete. + // WR-03 (16-REVIEW.md): before concluding disappearance, rule out + // a directory-internal move by widening the SAME (objectGUID=...) + // filter to each base DN's own domain root — outside the + // configured base-DN restriction. A hit means the group still + // exists; report and move on WITHOUT deleting or writing anything + // (deliberately no auto-rename to the found location either — a + // move outside the configured scope is an admin configuration + // signal, not something this sync silently absorbs). + let wideHit: Entry | null = null; + for (const root of domainRoots) { + const { searchEntries } = await client.search(root, { + filter, + attributes: ['cn', 'dn'], + scope: 'sub', + }); + if (searchEntries.length > 0) { + wideHit = searchEntries[0]; + break; + } + } + if (wideHit) { + result.errors.push( + `Gruppe ${group.name}: nicht mehr unter den konfigurierten Base-DNs gefunden, existiert aber weiterhin unter '${wideHit.dn}' — vermutlich im Verzeichnis verschoben, Base-DN-Konfiguration pruefen. Nicht geloescht.`, + ); + continue; + } + + // 4. Disappearance (SC-4/D-05/D-06) established — handoff BEFORE + // delete. const movedDefault = await this.groupsService.reassignDefaultBeforeDelete( tenantId, @@ -1456,4 +1517,55 @@ export class LdapService { .map((b) => '\\' + b.toString(16).padStart(2, '0')) .join(''); } + + /** + * Split a DN into its individual RDN components, respecting a + * backslash-escaped comma inside an RDN value (RFC 4514) so a value like + * "cn=Sales\, Inc.,dc=example,dc=com" is not split in the wrong place. + * Used only by domainRootOf() below — never for LDAP filter construction + * (T-16-01 stays in force there; this is pure DN string bookkeeping). + */ + private static splitDnComponents(dn: string): string[] { + const parts: string[] = []; + let current = ''; + let escaped = false; + for (const ch of dn) { + if (escaped) { + current += ch; + escaped = false; + continue; + } + if (ch === '\\') { + current += ch; + escaped = true; + continue; + } + if (ch === ',') { + parts.push(current); + current = ''; + continue; + } + current += ch; + } + parts.push(current); + return parts.map((p) => p.trim()).filter((p) => p.length > 0); + } + + /** + * Derive the domain root DN — the trailing DC=... component chain — from a + * configured base DN, e.g. "ou=Sales,dc=example,dc=com" -> + * "dc=example,dc=com". WR-03 (16-REVIEW.md) fallback search anchor for + * syncBoundGroupsForTenant()'s existence sweep: widening a "not found" + * result to the domain root before concluding a bound group has actually + * disappeared, rather than merely moved outside a narrower configured + * base DN. Returns null when the DN carries no DC= component at all + * (nothing to widen the search to — e.g. a non-AD directory using a + * different root naming scheme). + */ + private static domainRootOf(dn: string): string | null { + const dcParts = LdapService.splitDnComponents(dn).filter((rdn) => + /^dc=/i.test(rdn), + ); + return dcParts.length > 0 ? dcParts.join(',') : null; + } }