From 2779d42e6c5a9e72d0190da3c462e4f4003ce320 Mon Sep 17 00:00:00 2001 From: Schalli Date: Thu, 6 Aug 2026 16:58:29 +0200 Subject: [PATCH] fix(16): WR-04 normalize the rename-vs-unchanged comparison MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit syncBoundGroupsForTenant() compared cn/dn for byte equality, so any casing difference AD returns between two runs (e.g. after a domain-controller switch) would look like a rename and re-write name/ldapDn every single sync — violating the 'sync twice over an unchanged AD state = no-op' idempotency guarantee. The comparison used to DECIDE 'is this a rename' is now case-insensitive; the value written on an actual rename is still stored byte-for-byte as the directory reports it, per D-03. --- apps/api/src/ldap/ldap.service.spec.ts | 30 ++++++++++++++++++++++++++ apps/api/src/ldap/ldap.service.ts | 21 +++++++++++++++++- 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/apps/api/src/ldap/ldap.service.spec.ts b/apps/api/src/ldap/ldap.service.spec.ts index f87628e..814c887 100644 --- a/apps/api/src/ldap/ldap.service.spec.ts +++ b/apps/api/src/ldap/ldap.service.spec.ts @@ -1627,6 +1627,36 @@ describe('LdapService.syncBoundGroupsForTenant — Rekonziliation gegen das Verz expect(second.groupsRenamed).toBe(0); expect(second.groupsDeleted).toBe(0); }); + + it('a hit whose cn/dn differ only in casing from the stored values is NOT a rename — no write, no groupsRenamed increment (WR-04, 16-REVIEW.md)', async () => { + // Simulates a domain-controller switch returning different casing for + // the same, unchanged AD group between two syncs — Priority Check 7 + // ("sync twice over an unchanged AD state = no-op") must hold even + // then. + 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' }], + }); + + const result = makeResult(); + await run(result); + + expect(prisma.group.update).not.toHaveBeenCalled(); + expect(result.groupsRenamed).toBe(0); + // The stored value is untouched — it is NOT rewritten to the + // differently-cased AD response either. + expect(groups[0].name).toBe('Sales'); + expect(groups[0].ldapDn).toBe('CN=Sales,DC=example,DC=com'); + }); }); 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 df3d72d..ed42805 100644 --- a/apps/api/src/ldap/ldap.service.ts +++ b/apps/api/src/ldap/ldap.service.ts @@ -1308,8 +1308,27 @@ export class LdapService { : hit.dn; const dn = hit.dn; - if (name !== group.name || dn !== group.ldapDn) { + // WR-04 (16-REVIEW.md): the CHANGE CHECK is case-insensitive — + // only the DECISION "is this a rename" is normalized, never the + // value written below. AD returning the same cn/dn with different + // casing between two runs (e.g. after a domain-controller switch) + // must not look like a rename: that would break the idempotency + // guarantee (Priority Check 7 — sync twice over an unchanged AD + // state = no-op) and increment groupsRenamed / issue an update on + // every subsequent run. This is a plain lowercase compare, not + // full RFC 4514 DN canonicalization (per-attribute-type + // case-sensitivity rules, escaped-character normalization, etc.) + // — a pragmatic simplification, same uncertainty class as A1/A2 in + // RESEARCH.md, not verified against a real AD. + const nameChanged = name.toLowerCase() !== (group.name ?? '').toLowerCase(); + const dnChanged = + dn.toLowerCase() !== (group.ldapDn ?? '').toLowerCase(); + + if (nameChanged || dnChanged) { try { + // The write below stores name/dn EXACTLY as the directory + // reports them, byte-for-byte — never the lowercased + // comparison values above. Group.name stays AD's, per D-03. await tenantPrisma.group.update({ where: { id: group.id }, data: { name, ldapDn: dn },