From 2a954f5cee7f36367a3579a2acc5cb88b49087c6 Mon Sep 17 00:00:00 2001 From: Schalli Date: Thu, 6 Aug 2026 17:02:35 +0200 Subject: [PATCH] docs(16): record WR-01..WR-04 fix resolution in review and 16-03 summary Marks all four Warning findings from 16-REVIEW.md as resolved with their fix commit hashes; the two Info findings (IN-01/IN-02) remain open and out of scope for this fix pass. Appends a note to 16-03-SUMMARY.md recording the WR-02/WR-03/WR-04 behavior change in syncBoundGroupsForTenant(), since that summary still described the pre-fix behavior. --- .../16-03-SUMMARY.md | 12 ++ .../16-REVIEW.md | 166 ++++++++++++++++++ 2 files changed, 178 insertions(+) create mode 100644 .planning/phases/16-ad-gruppen-synchronisation/16-REVIEW.md diff --git a/.planning/phases/16-ad-gruppen-synchronisation/16-03-SUMMARY.md b/.planning/phases/16-ad-gruppen-synchronisation/16-03-SUMMARY.md index b5d79f6..076566f 100644 --- a/.planning/phases/16-ad-gruppen-synchronisation/16-03-SUMMARY.md +++ b/.planning/phases/16-ad-gruppen-synchronisation/16-03-SUMMARY.md @@ -216,6 +216,18 @@ None — keine externe Service-Konfiguration erforderlich. Der laufende Docker-S - Plan 16-04 (Dialog-Umbau) und 16-05 (Anzeige-Fallback Frontend, Requirement-Abschluss PERM-02) koennen auf dieser Grundlage aufsetzen - **Offener Punkt bleibt bestehen (WINDOWS.md #4):** A1/A2-Live-Pruefung gegen ein echtes Active Directory (ViCoTest) muss nachgeholt werden, bevor die Loeschsemantik aus D-05 als produktionsreif gilt — siehe Abschnitt „Outstanding Live Verification" oben +## Post-Review Fix (16-REVIEW.md, 2026-08-06) + +Der Code-Review dieser Phase (`16-REVIEW.md`) fand drei Warnungen an genau der in diesem Plan gebauten `syncBoundGroupsForTenant()`-Methode. Alle drei sind seither behoben — die obige Beschreibung dieses Plans spiegelt noch den Vor-Fix-Stand wider, daher dieser Nachtrag: + +- **WR-02 (Commit `1971795`):** Der Rename-Zweig meldete jeden P2002 beim Zurückschreiben von `name`/`ldapDn` pauschal als Namenskollision, ohne `updateError.meta.target` zu prüfen — obwohl der Aufruf zwei verschiedene Unique-Indizes treffen kann. Behoben durch dieselbe Ziel-Prüfung, die `importGroupsByDn()` bereits nutzte. +- **WR-03 (Commit `00de6a6`, der wichtigste der drei Punkte):** Der Existenz-Sweep vor einer Löschung suchte ausschließlich unter den konfigurierten Base-DNs. Eine AD-Gruppe, die in eine andere OU verschoben (nicht gelöscht) wurde, lieferte dort `searchEntries.length === 0` und wurde fälschlich als „verschwunden" gelesen und samt Mitgliedschaften/Modulfreigaben gelöscht — Zugriffsverlust durch eine reine Verzeichnis-Umstrukturierung, kein tatsächliches AD-Löschen. Die Methode führt jetzt vor jeder Löschung einen zweiten, weiter gefassten Sweep gegen die Domänen-Wurzel jeder konfigurierten Base-DN aus; ein Treffer dort verhindert die Löschung und wird stattdessen als Fehlerzeile gemeldet. Erst wenn auch dieser weite Sweep leer bleibt, gilt SC-4/D-05 als tatsächlich erfüllt. +- **WR-04 (Commit `2779d42`):** Die Umbenennungs-Erkennung verglich `cn`/`dn` byteweise. Eine reine Groß-/Kleinschreibungs-Abweichung zwischen zwei Sync-Läufen (z. B. nach einem Domain-Controller-Wechsel) hätte jedes Mal fälschlich eine Umbenennung ausgelöst und die Idempotenz-Garantie verletzt. Der Vergleich zur Entscheidung „ist das eine Umbenennung" ist jetzt case-insensitive; der tatsächlich gespeicherte Wert bleibt weiterhin exakt der vom Verzeichnis gemeldete (D-03 unangetastet). + +(WR-01, in `groups.service.ts`, betrifft nicht diese Methode — siehe `16-02-SUMMARY.md`. Commit `dd59bf5`.) + +Alle vier Fixes sind mit neuen Tests in `ldap.service.spec.ts` bzw. `groups.service.spec.ts` abgesichert; die volle API-Suite lief zuletzt gruen. Details siehe `16-REVIEW.md`. + --- *Phase: 16-ad-gruppen-synchronisation* *Completed: 2026-08-06* diff --git a/.planning/phases/16-ad-gruppen-synchronisation/16-REVIEW.md b/.planning/phases/16-ad-gruppen-synchronisation/16-REVIEW.md new file mode 100644 index 0000000..795a30d --- /dev/null +++ b/.planning/phases/16-ad-gruppen-synchronisation/16-REVIEW.md @@ -0,0 +1,166 @@ +--- +phase: 16-ad-gruppen-synchronisation +reviewed: 2026-08-06T14:50:13Z +depth: standard +files_reviewed: 21 +files_reviewed_list: + - apps/api/prisma/schema.prisma + - apps/api/prisma/migrations/20260806133916_add_group_internal_name_and_object_guid/migration.sql + - apps/api/src/ldap/ldap.service.ts + - apps/api/src/ldap/ldap.controller.ts + - apps/api/src/ldap/dto/ldap-config.dto.ts + - apps/api/src/ldap/ldap.module.ts + - apps/api/src/ldap/ldap.service.spec.ts + - apps/api/src/groups/groups.service.ts + - apps/api/src/groups/groups.controller.ts + - apps/api/src/groups/groups.module.ts + - apps/api/src/groups/dto/update-group.dto.ts + - apps/api/src/groups/module-grants.service.ts + - apps/api/src/groups/groups.service.spec.ts + - apps/api/src/groups/module-grants.service.spec.ts + - apps/api/src/groups/migration-sql.spec.ts + - apps/web/src/app/(portal)/admin/ldap/page.tsx + - apps/web/src/app/(portal)/admin/groups/page.tsx + - apps/web/src/app/(portal)/admin/groups/components/GroupFormModal.tsx + - apps/web/src/app/(portal)/admin/groups/groups-page.test.tsx + - apps/web/src/app/(portal)/admin/modules/grants/page.tsx + - apps/web/src/messages/de.json + - apps/web/src/messages/en.json +findings: + critical: 0 + warning: 4 + info: 2 + total: 6 +status: warnings_fixed_info_open +fixed_at: 2026-08-06T17:01:00Z +fixed_findings: + - id: WR-01 + commit: dd59bf5 + - id: WR-02 + commit: 1971795 + - id: WR-04 + commit: 2779d42 + - id: WR-03 + commit: 00de6a6 +open_findings: + - IN-01 + - IN-02 +--- + +# Phase 16: Code Review Report + +**Reviewed:** 2026-08-06T14:50:13Z +**Depth:** standard (mit gezielter Cross-File-Verifikation entlang der neun Priority Checks) +**Files Reviewed:** 21 +**Status:** issues_found + +## Summary + +Geprüft wurde der vollständige Diff von Phase 16 (AD-Gruppen-Synchronisation, `c4a2551..HEAD`, 21 Commits über 5 Pläne). Die tragenden Korrektheitsbedingungen aus dem Auftrag sind eingehalten: `syncBoundGroupsForTenant()` (Schritt 5a) läuft nachweislich vor `syncGroupMembershipsForTenant()` (Schritt 5b) — sowohl im Code als auch durch einen beobachtenden Call-Order-Test abgesichert; die Rename-vs-Delete-Unterscheidung läuft ausschließlich über den 32-Zeichen-validierten `ldapObjectGuid`, niemals über eine leere Suche, die durch einen Bind-Fehler, eine Exception oder einen Such-Timeout hätte simuliert werden können — jeder Suchfehler landet im per-Gruppe-`catch`, nie im Lösch-Zweig; eine Alt-Bindung mit nicht mehr auflösbarem `ldapDn` wird nachweislich NICHT gelöscht, sondern nur als Fehlerzeile gemeldet; der Default-Gruppen-Handoff läuft strikt vor jedem `group.delete()` und `ensureDefaultGroup()` schließt das Zero-Default-Fenster auch bei Löschung der letzten Gruppe eines Mandanten; jede LDAP-Filter-Interpolation (String wie binär) ist escaped, admin-gewählte DNs werden ausschließlich als Such-Base, nie als Filter-Fragment verwendet; die neuen Fehlerpfade in `/admin/ldap` (Gruppenimport, Sync-Anfrage) und im `GroupFormModal`-Speicherpfad zeigen sichtbare Fehler statt stillem `catch {}`; die sechs verwaisten `ldapBind.*`-i18n-Schlüssel sind aus beiden Sprachdateien vollständig und parallel entfernt. + +Trotzdem bleiben vier Warnungen und zwei Hinweise: Die serverseitige Namenssperre aus D-03 prüft ausschließlich `ldapObjectGuid`, nicht `ldapDn` — für eine Alt-Bindung aus Plan 15-06, die die neue Rekonziliation noch nicht durchlaufen hat, ist die im Summary behauptete „echte Backend-Invariante" bis zum ersten Sync-Lauf nur eine UI-Konvention. Ein P2002-Konflikt bei Umbenennung/Import wird ungeprüft als Namenskollision gemeldet, auch wenn technisch der `ldapDn`-Unique-Index getroffen wurde. Der Existenz-Sweep ist an die konfigurierten Base-DNs gebunden — eine AD-Gruppe, die aus diesem Teilbaum heraus verschoben, aber nicht gelöscht wird, würde als „verschwunden" gelesen und samt Mitgliedschaften/Freigaben entfernt. Und zwei kleinere Qualitätspunkte (veralteter Controller-Kommentar, fehlende Längenbegrenzung auf `internalName`) runden das Bild ab. Keiner dieser Punkte ist ein Blocker. + +## Warnings + +### WR-01: Namenssperre (D-03) prüft nur `ldapObjectGuid`, nicht `ldapDn` — Lücke für Alt-Bindungen vor dem ersten Sync + +**Status: BEHOBEN** — Commit `dd59bf5`. `GroupsService.update()` lehnt `name` jetzt ab, wenn `existing.ldapObjectGuid` ODER `existing.ldapDn` gesetzt ist. Neuer Test `groups.service.spec.ts`: „lehnt name fuer eine Alt-Bindung (ldapDn gesetzt, ldapObjectGuid noch null) mit BadRequestException ab". + +**File:** `apps/api/src/groups/groups.service.ts:134-139` +**Issue:** `GroupsService.update()` lehnt einen `name`-Wert nur ab, wenn `existing.ldapObjectGuid` gesetzt ist: +```ts +if (data.name !== undefined) { + if (existing.ldapObjectGuid) { + throw new BadRequestException(...) + } + ... +} +``` +D-07 legt jedoch ausdrücklich fest: „Bestehende Gruppen mit gesetztem `ldapDn` gelten als importiert und werden vom Sync verwaltet." Eine Gruppe aus der alten Plan-15-06-Bindung hat `ldapDn` gesetzt, aber `ldapObjectGuid` bleibt `null`, bis der neue Rekonziliations-Schritt (`syncBoundGroupsForTenant`, Legacy-Backfill) sie zum ersten Mal durchläuft. In diesem Zeitfenster — zwischen Deploy von Phase 16 und dem nächsten Sync-Lauf — lehnt der Server einen direkten `PATCH /groups/:id`-Aufruf mit `{ name: "..." }` NICHT ab, obwohl `GroupFormModal.tsx` (Frontend) dieselbe Gruppe bereits als „importiert" behandelt (`isImported = group?.ldapDn != null`) und das Namensfeld sperrt. Das 16-02-Summary behauptet ausdrücklich „echte Backend-Invariante ... ein direkter API-Aufruf kommt an ihr nicht vorbei" — für Alt-Bindungen stimmt das bis zum ersten Sync-Lauf nicht. Der zugehörige Test (`groups.service.spec.ts:402`, „erlaubt name fuer eine Gruppe ohne gesetzten ldapObjectGuid") deckt exakt diesen unveränderten Umgehungspfad ab, ohne ihn als Lücke zu benennen. Der Schaden begrenzt sich selbst (der nächste Sync-Lauf schreibt den AD-Namen wieder zurück), ist aber ein widersprüchlicher Zustand: eine Umbenennung „hält" bis zum nächsten Sync, obwohl D-03 sie kategorisch ausschließen soll. +**Fix:** +```ts +if (data.name !== undefined) { + if (existing.ldapObjectGuid || existing.ldapDn) { + throw new BadRequestException( + 'Der Name einer aus dem Verzeichnis übernommenen Gruppe wird dort gepflegt und kann hier nicht geändert werden', + ); + } + ... +} +``` + +### WR-02: P2002 bei Umbenennung/Import wird pauschal als Namenskollision gemeldet, ohne das tatsächliche Zielfeld zu prüfen + +**Status: BEHOBEN** — Commit `1971795`. Der Rename-Zweig in `syncBoundGroupsForTenant()` prüft jetzt `updateError.meta.target` genau wie `importGroupsByDn()` und meldet eine `ldapDn`-Kollision separat als „Aktualisierung kollidiert mit einer bestehenden Bindung". Zwei neue Tests in `ldap.service.spec.ts` decken beide Zweige ab (Name-Kollision mit realistischem `meta.target`, sowie eine Nicht-Name-Kollision, die nicht mehr fehlbeschriftet wird). + +**File:** `apps/api/src/ldap/ldap.service.ts:1318-1326` (Rename-Zweig in `syncBoundGroupsForTenant`) +**Issue:** Beim Zurückschreiben eines erkannten Umbenennungs-Treffers fängt der Code jeden `P2002` ab und meldet ihn immer als Namenskollision: +```ts +} catch (updateError: any) { + if (updateError?.code === 'P2002') { + result.errors.push( + `Gruppe ${group.name}: Umbenennung nach '${name}' kollidiert mit einer bestehenden Gruppe`, + ); + } else { throw updateError; } +} +``` +Der `update()`-Aufruf schreibt aber sowohl `name` als auch `ldapDn` in einem Schritt — `@@unique([tenantId, name])` UND `@@unique([tenantId, ldapDn])` sind beide potenzielle Auslöser eines P2002 auf genau diesem Aufruf. `importGroupsByDn()` (Zeile 711-728) macht es an der vergleichbaren Stelle bereits richtig vor: dort wird `createError?.meta?.target` geprüft, um zwischen `ldapObjectGuid`- und Namens-Kollision zu unterscheiden (allerdings auch dort ohne den dritten möglichen Index `ldapDn` zu berücksichtigen). Der Rename-Zweig prüft `target` überhaupt nicht. Praktisch ist eine `ldapDn`-Kollision durch zwei unterschiedliche AD-Gruppenobjekte mit identischer DN ausgeschlossen — der Fall ist selten, aber die Fehlermeldung wäre bei einem seltenen Edge-Case (z. B. gleichzeitiger Alt-Bindungs-Nachtrag einer anderen Gruppe auf dieselbe DN) irreführend und würde den Admin auf die falsche Fährte (Namenskonflikt statt DN-Konflikt) schicken. +**Fix:** +```ts +} catch (updateError: any) { + if (updateError?.code === 'P2002') { + const target = updateError?.meta?.target; + const targetsName = Array.isArray(target) + ? target.includes('name') + : String(target ?? '').includes('name'); + result.errors.push( + targetsName + ? `Gruppe ${group.name}: Umbenennung nach '${name}' kollidiert mit einer bestehenden Gruppe` + : `Gruppe ${group.name}: Aktualisierung kollidiert mit einer bestehenden Bindung (${JSON.stringify(target)})`, + ); + } else { throw updateError; } +} +``` + +### WR-03: Existenz-Sweep ist an die konfigurierten Base-DNs gebunden — eine im AD verschobene (nicht gelöschte) Gruppe wird fälschlich als „verschwunden" gelesen und samt Mitgliedschaften/Freigaben gelöscht + +**Status: BEHOBEN** — Commit `00de6a6`. Bevor `syncBoundGroupsForTenant()` eine Löschung ausführt, läuft jetzt ein zweiter, weiter gefasster `(objectGUID=...)`-Sweep gegen die jeweilige Domänen-Wurzel jeder konfigurierten Base-DN (der abgeleitete `dc=...,dc=...`-Teilbaum) — übersprungen, wenn eine Base-DN bereits selbst die Domänen-Wurzel ist. Ein Treffer dort wird als Fehlerzeile gemeldet, NICHT gelöscht; erst wenn auch der weite Sweep leer bleibt, gilt das Verschwinden als etabliert (SC-4/D-05/D-06 laufen dann unverändert weiter). Drei neue Tests in `ldap.service.spec.ts` decken den Verschiebungsfall (nicht löschen + Fehlerzeile), den echten Verschwindensfall (weiterhin löschen) und die Kein-Zusatzaufruf-Optimierung ab, wenn Base-DN bereits Domänen-Wurzel ist. + +**File:** `apps/api/src/ldap/ldap.service.ts:1236, 1287-1298` +**Issue:** Priority Check 2 verlangt eine harte Prüfung, ob irgendein Pfad eine echte Existenz fälschlich als „gone" liest. Bind-Fehler, Exceptions und leere Suchergebnisse durch einen echten, verifizierten Sync-Fehler sind sauber abgesichert (jeder Suchfehler landet im per-Gruppe-`catch`, nie im Lösch-Zweig — siehe `ldap.service.spec.ts:1527`). Es bleibt aber ein struktureller Pfad, der NICHT abgesichert ist: die Existenz-Suche läuft ausschließlich `for (const baseDn of baseDns)` — also nur unter den im LDAP-Konfigurationsfeld eingetragenen Base-DNs (`config.baseDn`, `scope: 'sub'`). Wird eine bereits gebundene AD-Gruppe im Verzeichnis in eine andere OU verschoben, die außerhalb dieses konfigurierten Teilbaums liegt (die Gruppe existiert im AD also weiterhin, nur nicht mehr unter den konfigurierten Suchwurzeln), liefert jede Sub-Tree-Suche unter den konfigurierten Base-DNs `searchEntries.length === 0` — kein Fehler, kein Timeout, eine technisch korrekte, aber fachlich irreführende leere Antwort. Der Code interpretiert das nach der bestehenden Logik zwangsläufig als „Gruppe im AD verschwunden" (SC-4/D-05) und löscht sie inklusive `GroupMembership`/`ModuleGrant` per Cascade — ein Zugriffsverlust, der durch eine reine Verzeichnis-Umstrukturierung ausgelöst wird, nicht durch ein tatsächliches Löschen der Gruppe. Dasselbe Scoping-Verhalten existiert bereits für die Benutzer-Deaktivierung (vorbestehendes Verhalten, hier nicht neu bewertet) — bei Gruppen ist der Blast-Radius über die Modul-Freigaben-Kaskade aber größer als bei einer einzelnen Benutzer-Deaktivierung, und D-05 wurde als „Löschung bei echtem AD-Verschwinden" akzeptiert, nicht ausdrücklich als „Löschung bei Verzeichnis-Umzug". +**Fix:** Mindestens dokumentieren (Betriebsanleitung/Admin-Hinweis: Base-DN-Feld muss jede OU abdecken, in die eine importierte Gruppe jemals verschoben werden könnte). Optional: vor einer Löschung eine zusätzliche, ungefilterte Existenzprüfung ohne Base-DN-Einschränkung durchführen (z. B. eine Suche ab dem LDAP-Root/naming context) und nur löschen, wenn auch diese null Treffer liefert — das würde False Positives durch reine Umzüge ausschließen, ohne die Kernlogik zu ändern. + +### WR-04: Rename-Erkennung vergleicht `cn`/`dn` als reine String-Gleichheit ohne Normalisierung + +**Status: BEHOBEN** — Commit `2779d42`. Die Entscheidung „ist das eine Umbenennung" vergleicht `cn`/`dn` jetzt case-insensitive (einfacher Kleinschreibungs-Vergleich, keine vollständige RFC-4514-Kanonisierung — als bewusste Vereinfachung dokumentiert, gleiche Unsicherheitsklasse wie A1/A2). Der geschriebene Wert bleibt weiterhin exakt der vom Verzeichnis gemeldete (D-03). Neuer Test in `ldap.service.spec.ts`: unterschiedliche Groß-/Kleinschreibung zwischen zwei Sync-Läufen löst keine Umbenennung mehr aus. + +**File:** `apps/api/src/ldap/ldap.service.ts:1311` +**Issue:** `if (name !== group.name || dn !== group.ldapDn)` löst bei jeder Byte-Abweichung eine Umbenennungs-Schreibaktion aus (`groupsRenamed++`, `group.update()`). Sollte ein AD-Server die Groß-/Kleinschreibung eines DN-Bestandteils zwischen zwei Sync-Läufen unterschiedlich zurückliefern (z. B. durch einen Domain-Controller-Wechsel mit abweichender Normalisierung), würde jeder Sync-Lauf fälschlich eine „Umbenennung" melden und schreiben, obwohl im Verzeichnis nichts geändert wurde — ein Verstoß gegen die in Priority Check 7 geforderte Idempotenz „Sync zweimal über unveränderten AD-Zustand = No-Op". Der bestehende Idempotenz-Test (`ldap.service.spec.ts:1563`) deckt genau diesen Fall nicht ab, weil der Mock in beiden Läufen byteidentische Strings liefert. Da dies von realem AD-Laufzeitverhalten abhängt (dieselbe Unsicherheitskategorie wie die bereits dokumentierten offenen Annahmen A1/A2), wird der Schaden als gering eingeschätzt (kein Datenverlust, nur ein wiederholt inkrementierter `groupsRenamed`-Zähler und unnötige Schreibzugriffe), aber die fehlende Normalisierung ist im Code nicht einmal als bewusste Entscheidung dokumentiert. +**Fix:** Optional Case-insensitive-Vergleich für `dn` (RFC-4514-DNs sind attributweise oft case-insensitive) erwägen, oder zumindest im Kommentar festhalten, dass exakte Byte-Gleichheit vom AD als gegeben vorausgesetzt wird. + +## Info + +### IN-01: Veralteter Controller-Kommentar behauptet weiterhin eine AD-Bindungsfunktion, die D-07 entfernt hat + +**File:** `apps/api/src/groups/groups.controller.ts:64-66` +**Issue:** +```ts +/** + * PATCH /groups/:id + * Umbenennen, Standardmarkierung setzen/entfernen, AD-Bindung setzen/lösen. + */ +``` +`UpdateGroupDto` besitzt seit Plan 16-02 kein `ldapDn`-Feld mehr — „AD-Bindung setzen/lösen" über diese Route ist seit D-07 nicht mehr möglich. Der zugehörige Kommentar im DTO selbst (`update-group.dto.ts`) wurde korrekt aktualisiert, der Controller-Kommentar wurde übersehen. +**Fix:** Kommentar auf „Umbenennen (nur lokale Gruppen), Standardmarkierung setzen/entfernen, internen Namen setzen/löschen." aktualisieren. + +### IN-02: `internalName` ohne serverseitige Längenbegrenzung + +**File:** `apps/api/src/groups/dto/update-group.dto.ts:28-30` +**Issue:** `internalName?: string | null` trägt nur `@IsOptional()`/`@IsString()`, keine `@MaxLength()`. Die Spalte ist als `TEXT` ohne DB-seitiges Limit angelegt, die Anzeige in Gruppenliste/Matrix-Spaltenkopf/Chips kürzt visuell per CSS-Truncate, aber ein sehr langer Wert würde unbegrenzt persistiert und z. B. in `title`-Tooltips oder Log-Zeilen unhandlich. +**Fix:** `@MaxLength(255)` (oder passender Grenzwert) ergänzen, analog zu üblichen Namensfeld-Konventionen im Projekt. + +--- + +_Reviewed: 2026-08-06T14:50:13Z_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_