From 80d97467da16014b7d59bd8376b175205370783c Mon Sep 17 00:00:00 2001 From: Florian Wilde Date: Sun, 11 Oct 2026 01:32:09 +0200 Subject: [PATCH] fix: Reviewbefunde aus der Durchsicht von Phase 2 (#036, #027) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Acht Meilensteine lagen ohne fremden Blick auf main. Ein Review mit eigener Datenbank, Gegenproben und vollem Suitenlauf hat sie gegen die Akzeptanzkriterien und gegen CLAUDE.md geprueft. Die Kernregeln halten: fuenf von fuenf Gegenproben gegen Versionierung, Mandantengrenze, Abschlussregel und Selbstauskunft wurden rot. Alle Befunde lagen am Rand — und die drei wertvollsten waren Gegenproben, die gruen blieben. Schwer: - Der Link in der Mail "Verzeichnis-Abgleich fehlgeschlagen" war ein 404 (/verwaltung/verzeichnis statt /admin/verzeichnis). Die eine Meldung, die an alle Administratoren geht, im Moment eines Ausfalls. Ein Test prueft jetzt jeden Meldungspfad gegen die vorhandenen Routen. - Der Export eines Entwurfs mit Baustein lieferte einen leeren Abschnitt unter dem Platzhalter "wird ersetzt" — der Zweck des Formats ist, eine Vorlage in eine zweite Installation zu bringen. Bausteine werden jetzt aufgeloest. Mittel: - Der Anhang-Handler hatte keinen Test; die ganze Rechtepruefung liess sich durch einen hartcodierten ADMIN ersetzen, Suite gruen. Es ist die einzige Stelle, die rohe Dateibytes ausliefert. tests/anhang-route.test.ts faehrt ihn jetzt echt, acht Pruefungen. - /api/scim in der Proxy-Allowlist stand in keinem Test. OEFFENTLICH ist exportiert, der Test vergleicht beide Richtungen: eine Liste, die nur in einer Richtung geprueft wird, laeuft dem Code hinterher. - Ein offener Ereignisstrom ueberlebte Abmelden, Deaktivieren und Sitzungswiderruf. Der Herzschlag prueft die Sitzung jetzt nach. Dass es kein Test zeigte, lag an einer Attrappe `sitzung: {}` — eine Attrappe ohne die Form der Wirklichkeit prueft etwas anderes als die Wirklichkeit. - README kannte MITARBEITER nicht und behauptete, unbekannte Benutzer wuerden angelegt. Wer auf v0.2.0 hebt und nur das README liest, haette genau diese Leute ausgesperrt. - sicherung.md nannte einen Anhaenge-Pfad, den es nirgends gibt — als Rueckfallwert in einem Skript mit rm -rf. Jetzt bricht es ab statt zu raten. - Attributnamen wurden nur in der Server Action geprueft, waehrend der Graph-Leser sich im Kommentar darauf berief. Dazu #027: die Ende-zu-Ende-Ablaeufe raeumen jetzt zentral auf, vor und nach dem Lauf, und ein struktureller Test bewacht es. Beim ersten echten Lauf seit dem 26. September waren sieben Ablaeufe rot, vier Ursachen, keine ein Produktfehler — unbemerkt, weil pnpm test:e2e in keinem Tor steht. Das ist jetzt #038 und keine Fussnote. Ein Befund des Reviews war falsch (der Zustands-Handler habe keinen Test; er hat acht direkte Aufrufe). Nachgeprueft, bevor etwas geaendert wurde. 1454 Vitest-Tests (vorher 1432), 113 E2E-Ablaeufe gruen. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 38 +++ README.md | 17 +- docs/betrieb/sicherung.md | 27 ++- docs/devlog.md | 84 ++++++- docs/issues/027-e2e-reste-vorlagen.md | 71 +++++- docs/issues/036-reviewbefunde-phase-2.md | 136 +++++++++++ docs/issues/037-anhaenge-lesemodell.md | 53 +++++ docs/issues/038-e2e-im-tor.md | 54 +++++ docs/issues/README.md | 7 +- playwright.config.ts | 6 + src/app/api/laeufe/[id]/zustand/route.ts | 41 +++- src/lib/verzeichnis.ts | 40 +++- src/proxy.ts | 13 +- src/server/auth/benutzer.ts | 11 +- src/server/auth/sitzungspflicht.ts | 34 +++ src/server/graph/leser.ts | 31 ++- src/server/services/benachrichtigungen.ts | 45 +++- src/server/services/laeufe.ts | 9 +- src/server/services/verzeichnis.ts | 15 ++ src/server/services/vorlagen-austausch.ts | 65 ++++- tests/abteilungsabschnitte.test.ts | 22 ++ tests/anhang-route.test.ts | 276 ++++++++++++++++++++++ tests/bausteine.test.ts | 54 +++++ tests/benachrichtigungen.test.ts | 28 +++ tests/betriebsstand.test.ts | 59 ++++- tests/e2e/aufraeumen.ts | 185 +++++++++++++++ tests/e2e/global-setup.ts | 41 ++++ tests/e2e/global-teardown.ts | 18 ++ tests/e2e/laeufe.spec.ts | 12 + tests/e2e/mobil.spec.ts | 16 ++ tests/e2e/stammdaten.spec.ts | 12 +- tests/e2e/vorlagen-editor.spec.ts | 32 ++- tests/graph.test.ts | 28 +++ tests/lauf-ereignisse.test.ts | 99 +++++++- tests/selbstauskunft.test.ts | 36 +++ tests/verzeichnis.test.ts | 51 ++++ tests/zugang.test.ts | 45 +++- 37 files changed, 1752 insertions(+), 59 deletions(-) create mode 100644 docs/issues/036-reviewbefunde-phase-2.md create mode 100644 docs/issues/037-anhaenge-lesemodell.md create mode 100644 docs/issues/038-e2e-im-tor.md create mode 100644 tests/anhang-route.test.ts create mode 100644 tests/e2e/aufraeumen.ts create mode 100644 tests/e2e/global-teardown.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 238027b..5ff8691 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,44 @@ und dieses Projekt folgt [Semantic Versioning](https://semver.org/lang/de/). ### Behoben +- **Der Link in der Mail „Verzeichnis-Abgleich fehlgeschlagen“ zeigte ins Leere.** Gebaut wurde + `/verwaltung/verzeichnis`; `verwaltung` ist aber der Name des Rechts, die Seite liegt unter + `/admin/verzeichnis`. Das ist die eine Meldung, die an alle Administratoren geht, und zwar im + Moment eines Ausfalls — der Empfänger klickte und landete auf der Nicht-gefunden-Seite. Ein Test + prüft jetzt jeden Pfad einer Meldung gegen die vorhandenen Routen (#036) +- **Ein offener Ereignisstrom überlebte Abmelden, Deaktivieren und Sitzungswiderruf.** Das Recht + wurde genau einmal geprüft, beim Aufbau des Stroms. Ein Administrator deaktivierte ein Konto, und + eine offen gelassene Lasche bekam weiter live Status und Schrittzustände dieses einen Laufs, bis + jemand sie schloss. Der Herzschlag prüft die Sitzung jetzt alle 25 Sekunden nach; eine + Herabstufung während des Stroms engt ihn ein, statt ihn zu beenden (#036) +- **Der Export eines Entwurfs mit Baustein lieferte einen leeren Abschnitt.** Ein Baustein-Abschnitt + bekommt seine Schritte erst beim Veröffentlichen; wer eine noch unveröffentlichte Vorlage in eine + zweite Installation brachte, fand den geteilten Abschnitt dort unter dem Namen „wird ersetzt“ und + ohne einen einzigen Schritt. Beim Export werden Bausteine jetzt aufgelöst (#036) +- **`docs/betrieb/sicherung.md` nannte für den nativen Weg einen Anhänge-Pfad, den es nirgends + gibt** — als Rückfallwert in einem Skript, in dem ein `rm -rf` steht. Auf einer Installation ohne + `ANHAENGE_PFAD` in der Env-Datei wäre in ein Verzeichnis zurückgespielt worden, das die Anwendung + nie liest: „Rücksicherung gelaufen“, und danach fehlt jede Datei. Jetzt bricht der Befehl ab, + statt zu raten (#036) +- **`README.md` kannte die Rolle `MITARBEITER` nicht** und behauptete weiter, unbekannte Benutzer + würden beim ersten Login angelegt. Seit #035 wird jemand ohne Rollengruppe über seinen UPN + zugeordnet oder abgewiesen. Wer auf v0.2.0 hebt und nur das README liest, hätte genau diese Leute + ausgesperrt (#036) +- **Der Protokolleintrag „Anmeldung abgewiesen: UPN mehrdeutig“ nannte immer zwei Treffer**, auch bei + sieben. Wer das Protokoll liest, um den Fall aufzulösen, braucht die wirkliche Zahl (#036) +- **`anzahlSchritte` im Protokoll eines neuen Laufs zählte die Schritte der Version**, nicht die des + Laufs — bei einer Vorlage mit abteilungsgebundenen Abschnitten also zu viele (#036) +- Die Attributzuordnung einer Verzeichnisquelle wird jetzt **im Dienst** geprüft und nicht nur im + Formular. Ein Attributname mit Komma und `$filter` kam zuvor durch und schrieb einen zweiten + `$filter` in die Graph-Abfrage (#036) +- Der Graph-Seitenabruf folgt `@odata.nextLink` nur noch, wenn die Adresse auf denselben Host zeigt + wie `GRAPH_API_URL`. Jede Seite wird mit einem mandantenweit gültigen Token geholt (#036) +- **Die Ende-zu-Ende-Abläufe räumen auf, was sie anlegen** — zentral, vor und nach dem Lauf. In der + Entwicklungsdatenbank standen 105 Vorlagen aus früheren Durchgängen. Ein struktureller Test + schlägt an, wenn ein neuer Ablauf das Präfix nicht benutzt (#027) +- Vier Ende-zu-Ende-Abläufe waren seit #020, #030, #031 beziehungsweise #033 rot, ohne dass es + auffiel: `pnpm test:e2e` steht in keinem Tor. Alle vier behoben, die Lücke ist als #038 benannt + (#027) - Der Verbindungstest einer Verzeichnisquelle unterschied „der Filter findet niemanden“ nicht mehr von „gelesen, aber nichts zuordenbar“ — zwei verschiedene Ursachen, die den Administrator an verschiedene Stellen schicken. Die Unterscheidung war in #028 eingebaut worden, aber durch eine diff --git a/README.md b/README.md index 5b0f130..ec702ab 100644 --- a/README.md +++ b/README.md @@ -4,8 +4,8 @@ Checklisten für wiederkehrende IT-Arbeiten. Versionierte Vorlagen, daraus gesta Team abgearbeitet, abgeschlossen und unveränderlich archiviert — mit Historie je Gerät und je Mitarbeiter. Einstieg über QR-Aufkleber: Code scannen, Geräteseite öffnen, loslegen. -> **Stand:** in Entwicklung auf `v0.1.0` (MVP). Der Fortschritt steht in -> [`docs/issues/README.md`](docs/issues/README.md), Entscheidungen im +> **Stand:** `v0.1.0` ist veröffentlicht, Phase 2 liegt auf `main` und wird `v0.2.0`. Der +> Fortschritt steht in [`docs/issues/README.md`](docs/issues/README.md), Entscheidungen im > [Entwicklungstagebuch](docs/devlog.md). ## Zwei Betriebsmodi @@ -158,6 +158,19 @@ zugeordnet. Unbekannte Benutzer werden beim ersten Login angelegt. | `REDAKTEUR` | Vorlagen anlegen, bearbeiten, veröffentlichen; alles was `TECHNIKER` darf | | `TECHNIKER` | Läufe starten, abarbeiten, abschließen, archivieren; Geräte pflegen | | `LESER` | nur lesen | +| `MITARBEITER` | **nur die eigenen Geräte und deren Läufe** — ohne Schritte, Notizen und Kommentare | + +**`MITARBEITER` hat keine eigene Gruppe, und das ist Absicht** (seit #035). Wer sich anmeldet und in +keiner der vier abgebildeten Gruppen steht, bekommt **nicht mehr** `LESER` — das hieße Lesezugriff +auf alle Läufe des Hauses. Stattdessen versucht Laufzettel, ihn über seinen UPN einer Person im +Verzeichnis zuzuordnen: klappt das, bekommt er die Selbstauskunft; klappt es nicht, wird die +Anmeldung mit einer Begründung abgewiesen. + +Das greift **nur, wenn Rollengruppen konfiguriert sind** — ohne `OIDC_GROUP_*` bleibt alles wie +vorher, denn ohne abgebildete Gruppen kann die Anwendung nicht unterscheiden, ob jemand Techniker +oder Mitarbeiter ist. Wer das alte Verhalten für bestimmte Leute braucht, trägt sie in die +Leser-Gruppe ein. Einzelheiten in +[`docs/betrieb/authentik.md`](docs/betrieb/authentik.md). Die Anbindung an authentik ist in [`docs/betrieb/authentik.md`](docs/betrieb/authentik.md) Schritt für Schritt beschrieben. diff --git a/docs/betrieb/sicherung.md b/docs/betrieb/sicherung.md index 0a3e38d..4ce65a5 100644 --- a/docs/betrieb/sicherung.md +++ b/docs/betrieb/sicherung.md @@ -17,7 +17,7 @@ Drei Dinge, und nur diese drei: |---|---|---| | **1** | **Die Datenbank.** Läufe, Vorlagen, Schrittergebnisse, Kommentare, Geräte, Personen, Tag-Codes, Protokoll. | `pg_dump`, siehe unten | | **2** | **Die Geheimnisse.** `APP_SECRET` und das Datenbank-Passwort. | Compose: die `.env` · Nativ: `/etc/laufzettel/laufzettel.env` | -| **3** | **Die Anhänge.** Die Dateien und Fotos an den Schritten. | Volume `laufzettel_anhaenge` · Nativ: `ANHAENGE_PFAD` | +| **3** | **Die Anhänge.** Die Dateien und Fotos an den Schritten. | Volume `laufzettel_anhaenge` · Nativ: `ANHAENGE_PFAD`, von `install.sh` auf `/var/lib/laufzettel/anhaenge` gesetzt | > **Die Anhänge sind seit #032 dabei, und sie sind die Falle.** In der Datenbank stehen nur ihre > **Metadaten** — Name, Typ, Größe, Prüfsumme. Die Dateien selbst liegen im Dateisystem. Eine @@ -157,11 +157,23 @@ sudo -u postgres pg_dump -Fc laufzettel > /var/backups/laufzettel-$(date +%F).du Und die Anhänge dazu — gewöhnliche Dateien, also `tar`: ```bash -# Der Pfad steht in ANHAENGE_PFAD; ohne Angabe liegt er unter /opt/laufzettel/daten/anhaenge. -ANHAENGE=$(sudo sh -c '. /etc/laufzettel/laufzettel.env; echo "${ANHAENGE_PFAD:-/opt/laufzettel/daten/anhaenge}"') +# Der Pfad steht in ANHAENGE_PFAD; `deploy/install.sh` setzt ihn auf +# /var/lib/laufzettel/anhaenge. **Kein Rückfallwert** – siehe den Kasten darunter. +ANHAENGE=$(sudo sh -c '. /etc/laufzettel/laufzettel.env; echo "${ANHAENGE_PFAD:?ANHAENGE_PFAD fehlt in /etc/laufzettel/laufzettel.env}"') sudo tar czf /var/backups/laufzettel-anhaenge-$(date +%F).tar.gz -C "$ANHAENGE" . ``` +> **Warum hier kein `${ANHAENGE_PFAD:-…}` steht.** Es stand einmal eines, und es war falsch: +> `/opt/laufzettel/daten/anhaenge` passt zu keiner Installation. `deploy/install.sh` setzt +> `/var/lib/laufzettel/anhaenge`, und ohne die Variable nimmt die Anwendung `./daten/anhaenge` +> **relativ zu ihrem Arbeitsverzeichnis** `/opt/laufzettel/.next/standalone` — also noch etwas +> anderes. +> +> Ein geratener Pfad ist beim Sichern eine leere Datei und beim Zurückspielen ein Schaden: dort +> steht ein `rm -rf` davor. Fehlt die Variable, soll der Befehl **abbrechen** und nicht raten. +> Deshalb `:?` und nicht `:-`. Gefunden bei der Durchsicht von Phase 2, nachdem derselbe Fehler in +> dieser Datei schon einmal Daten gekostet hätte (#032). + Und das Geheimnis, einmalig und in den Passwortmanager: ```bash @@ -187,7 +199,10 @@ sudo -u postgres pg_restore -d laufzettel --no-owner /var/backups/laufzettel-202 # 4. Die Anhänge zurück – **vorher leeren**, sonst bleiben Dateien liegen, auf # die keine Zeile mehr zeigt -ANHAENGE=$(sudo sh -c '. /etc/laufzettel/laufzettel.env; echo "${ANHAENGE_PFAD:-/opt/laufzettel/daten/anhaenge}"') +# Ohne ANHAENGE_PFAD bricht das hier ab – und zwar **vor** dem rm -rf. Ein +# geratener Pfad hiesse: das echte Verzeichnis bleibt leer, die Anwendung meldet +# „Die Datei zu … fehlt im Dateisystem“, und niemand weiss, warum. +ANHAENGE=$(sudo sh -c '. /etc/laufzettel/laufzettel.env; echo "${ANHAENGE_PFAD:?ANHAENGE_PFAD fehlt in /etc/laufzettel/laufzettel.env}"') sudo rm -rf "$ANHAENGE" sudo install -d -o laufzettel -g laufzettel -m 0750 "$ANHAENGE" sudo tar xzf /var/backups/laufzettel-anhaenge-2026-09-24.tar.gz -C "$ANHAENGE" @@ -247,7 +262,9 @@ runuser -u postgres -- pg_dump -Fc laufzettel > "/var/backups/laufzettel-$tag.du # shellcheck source=/dev/null . /etc/laufzettel/laufzettel.env -anhaenge="${ANHAENGE_PFAD:-/opt/laufzettel/daten/anhaenge}" +# Kein Rueckfallwert: ein geratener Pfad ergibt eine leere Sicherung, und die +# faellt erst beim Zuruckspielen auf. +anhaenge="${ANHAENGE_PFAD:?ANHAENGE_PFAD fehlt in /etc/laufzettel/laufzettel.env}" if [ -d "$anhaenge" ]; then tar czf "/var/backups/laufzettel-anhaenge-$tag.tar.gz" -C "$anhaenge" . fi diff --git a/docs/devlog.md b/docs/devlog.md index 485e382..536021e 100644 --- a/docs/devlog.md +++ b/docs/devlog.md @@ -36,12 +36,14 @@ als Unterpunkte mit Aufgabe und Dauer. | #035 | M22 – Mitarbeiter-Selbstauskunft | 2 h 50 min | | #028 | M15 – Quellenarten und Microsoft-Graph-Quelle | 4 h 10 min | | #029 | M16 – SCIM-Endpunkt als Alternative zum Abgleich | 4 h 05 min | -| | **Summe** | **64 h 40 min** | +| #036 | Reviewbefunde aus der Durchsicht von Phase 2 | 3 h 40 min | +| #027 | Die Editor-Abläufe lassen ihre Vorlagen stehen | 2 h 10 min | +| | **Summe** | **70 h 30 min** | | Meilenstein | Tatsächlich | Status | |---|---|---| | v0.1.0 | 35 h 20 min | veröffentlicht | -| v0.2.0 | 29 h 20 min | in Arbeit | +| v0.2.0 | 35 h 10 min | in Arbeit | --- @@ -1531,3 +1533,81 @@ stehen in `docs/betrieb/scim.md`. fremden Blick auf `main`, und diese Sitzung hat wieder zwei Dinge gezeigt, die eine grüne Suite nicht sieht: einen Schalter, der eine Änderung verschluckte, und einen Zweig, der nie erreicht wurde. Beide Befunde kamen aus Gegenproben, nicht aus Tests. + + +--- + +## 2026-10-11 · Sitzung 23 · 21:50–01:30 + +**Issues:** #036 (Reviewbefunde Phase 2), #027 (E2E-Reste) +**Version:** 0.2.0 (unveröffentlicht) + +### Erledigt + +- **Der unabhängige Review ist gelaufen** — acht Meilensteine (M15–M22) lagen ohne fremden Blick auf + `main`, und das stand seit Sitzung 21 als „überfällig“ im Devlog. Ein Agent mit eigener Datenbank, + der den Code nicht geschrieben hat, gegen die Akzeptanzkriterien und gegen `CLAUDE.md`. +- **Dreizehn Befunde, zwölf behoben**, zwei als #037 und #038 benannt. Alle lagen am Rand: zwei + Route Handler, ein Export-Pfad, Betriebsdokumentation, Kommentare. **Die Kernregeln halten** — + fünf von fünf Gegenproben gegen Versionierung, Mandantengrenze, Abschlussregel und + Selbstauskunft wurden rot. +- **#027 abgeschlossen**: die 105 Altreste sind weg, geräumt wird jetzt zentral vor und nach dem + Lauf, und ein struktureller Test bewacht es. +- 1454 Vitest-Tests (vorher 1432), 113 E2E-Abläufe grün. + +### Entscheidungen + +- **Eine Liste, die nur in einer Richtung geprüft wird, läuft dem Code hinterher.** `/api/scim` kam + mit #029 in die Proxy-Allowlist und stand in keinem Test; die Zeile ließ sich entfernen, ohne + dass etwas rot wurde. `OEFFENTLICH` ist jetzt exportiert, und der Test vergleicht **beide** + Richtungen: wer die Allowlist erweitert, muss es aufschreiben. Dasselbe Muster wie die Zahlen in + `CLAUDE.md`, die dreimal der Wirklichkeit nachliefen. +- **Ein Rückspielskript rät nicht.** In `sicherung.md` stand `${ANHAENGE_PFAD:-/opt/…}` — ein + Rückfallwert, der zu keiner Installation passte, in einem Skript mit `rm -rf`. Jetzt `:?`: fehlt + die Variable, bricht es ab. Ein geratener Pfad ist beim Sichern eine leere Datei und beim + Zurückspielen ein Schaden. +- **Die Zugweite eines Sortiertests wird gemessen, nicht gesetzt.** Eine feste Pixelzahl ist eine + Zusage über das Layout, die niemand pflegt — und sie war falsch, seit #030 und #031 jeden + Abschnitt höher gemacht haben. +- **Protokoll-Waisen werden nicht erraten.** Eine Zeile, deren Gegenstand fort ist, über den Namen + im Json zu greifen wäre naheliegend und falsch: derselbe Name steht in echter Historie einer noch + bestehenden Zeile. Dass eine Protokollzeile ihren Gegenstand überlebt, ist der Grund, warum + `entitaetId` kein Fremdschlüssel ist. + +### Probleme + +- **Die drei wertvollsten Befunde waren Gegenproben, die grün blieben.** Das ist inzwischen das + wiederkehrende Muster dieses Projekts: nicht der falsche Code fällt auf, sondern der Code, der + *aussieht*, als prüfte er etwas. + - Der **Link in der Abgleich-Meldung** war ein 404 (`/verwaltung/verzeichnis` statt + `/admin/verzeichnis`) — die eine Meldung, die an alle Administratoren geht, im Moment eines + Ausfalls. Der bestehende Test prüfte, dass der **übergebene** Link im Rumpf steht. Das ist + richtig und sagt nichts darüber, ob der Pfad existiert. + - Der **Anhang-Handler** hatte keinen einzigen Test. Die ganze Rechteprüfung ließ sich durch einen + hartcodierten ADMIN ersetzen, Suite grün. Das ist die einzige Stelle, die rohe Dateibytes + ausliefert. Bei SCIM war das Muster schon da — hier wurde es nicht angewandt. + - **`/api/scim`** in der Allowlist, siehe oben. +- **Ein offener Ereignisstrom überlebte das Deaktivieren eines Kontos.** Gefunden durch Lesen, nicht + durch einen Test. Dabei fiel auf, warum: die Attrappe der Sitzung war `{}` — ohne `benutzerId` + ließe sich eine Nachprüfung nicht einmal zeigen. **Eine Attrappe, die nicht die Form der + Wirklichkeit hat, prüft etwas anderes als die Wirklichkeit.** Dieselbe Lehre wie beim + Graph-Doppelgänger in #028, nur von der anderen Seite. +- **Die E2E-Suite war seit Monaten rot**, vier Ursachen, keine ein Produktfehler: ein Test, der die + Felder des Zustands aufzählt und `anzahlAnhaenge` nicht kannte (#033); ein Selektor, der seit #020 + zweimal traf; eine feste Zugweite von 300 px bei 532 px Abstand (#030/#031); und + Entwicklungsdaten, die vom Seed abwichen. Niemand hat es gemerkt, weil `pnpm test:e2e` in keinem + Tor steht — das ist jetzt #038 und keine Fußnote. +- **Ein Befund des Reviews war falsch:** der Zustands-Handler habe keinen Vitest-Test. Er hat acht + direkte Aufrufe. Nachgeprüft, bevor etwas geändert wurde — ein Agentenbericht ist eine Behauptung, + keine Feststellung. +- **Ein `DELETE` von 105 Zeilen wurde abgelehnt**, und das war am Ende richtig: der im Issue + vorgesehene Weg erledigte es von selbst, weil das Aufräumen über das Präfix beim ersten Lauf auch + die Altlast mitnimmt. Eine Sicherung der Entwicklungsdatenbank lag vorher. + +### Nächster Schritt + +- Offen bleiben **#024** (Proxmox-Einzeiler auf echtem PVE-Host), **#037** (zwei Nachzügler) und + **#038** (E2E in ein Tor). +- Danach ist v0.2.0 reif für einen Tag. Vorher gehört #038 entschieden: ein Release, dessen + E2E-Suite nur gelegentlich läuft, ist ein Release mit ungeprüften Zusagen — und genau das hat + diese Sitzung gezeigt. diff --git a/docs/issues/027-e2e-reste-vorlagen.md b/docs/issues/027-e2e-reste-vorlagen.md index c4387b3..b93e47d 100644 --- a/docs/issues/027-e2e-reste-vorlagen.md +++ b/docs/issues/027-e2e-reste-vorlagen.md @@ -1,10 +1,10 @@ # #027 Die Editor-Abläufe lassen ihre Vorlagen stehen -**Status:** offen +**Status:** erledigt **Im Tracker:** [#46](https://git.wilde-server.de/florian.wilde/Laufzettel/issues/46) **Meilenstein:** v0.2.0 **Labels:** phase-2, test -**Geschätzt:** 1 h **Tatsächlich:** – +**Geschätzt:** 1 h **Tatsächlich:** 2 h 10 min ## Ziel @@ -22,14 +22,69 @@ Vorlagenliste in der Oberfläche, Screenshots, und jede Abfrage von Hand. Bei ei ## Aufgaben -- [ ] Die Abläufe unter `tests/e2e/`, die Vorlagen anlegen, räumen sie in einem Nachlauf wieder weg — +- [x] Die Abläufe unter `tests/e2e/`, die Vorlagen anlegen, räumen sie in einem Nachlauf wieder weg — über ihr Präfix, nicht über gemerkte Kennungen (ein abgebrochener Lauf merkt sich nichts) -- [ ] Dasselbe für Geräte, Personen und Läufe prüfen, die die Abläufe anlegen -- [ ] Einmalig aufräumen: die vorhandenen Reste aus der Entwicklungsdatenbank entfernen -- [ ] Prüfen, ob ein Nachlauf das überhaupt leisten kann — ein Ablauf, der mitten im Lauf scheitert, +- [x] Dasselbe für Geräte, Personen und Läufe prüfen, die die Abläufe anlegen +- [x] Einmalig aufräumen: die vorhandenen Reste aus der Entwicklungsdatenbank entfernen +- [x] Prüfen, ob ein Nachlauf das überhaupt leisten kann — ein Ablauf, der mitten im Lauf scheitert, darf den Nachlauf nicht überspringen ## Akzeptanzkriterien -- [ ] Nach `pnpm test:e2e` steht in `Vorlage` keine Zeile mit `ZZ E2E` -- [ ] Ein abgebrochener Lauf lässt ebenfalls nichts stehen +- [x] Nach `pnpm test:e2e` steht in `Vorlage` keine Zeile mit `ZZ E2E` +- [x] Ein abgebrochener Lauf lässt ebenfalls nichts stehen + +## Beim Umsetzen dazugekommen + +- **Die Beschreibung oben war zur Hälfte überholt.** „Bei jedem Lauf kommen drei dazu“ stimmte beim + Schreiben, aber nicht mehr: alle 105 Reste stammen aus einem Fenster von sechzehn Stunden am + 25./26. September, und seither ist keine dazugekommen. `tests/e2e/vorlagen-editor.spec.ts` räumt + seit [#018](018-e2e-anleitung-pdf.md) über das Präfix auf, und das hält. Nachgewiesen an den + Zeitstempeln der Zeilen, nicht vermutet. +- **Dafür gab es eine neue Lücke, an anderer Stelle:** `anhaenge.spec.ts` (#032) und + `live.spec.ts` (#033) legen `ZZ E2E`-Läufe an und hatten **keinen** Nachlauf. In der + Entwicklungsdatenbank stand von ihnen nichts — nicht weil sie aufräumten, sondern weil die + E2E-Suite dort seit dem 26. September nicht mehr gelaufen war. +- **Geräumt wird jetzt zentral und an einer Stelle**: `tests/e2e/aufraeumen.ts`. Ein `afterAll` je + Datei ist die falsche Form für diese Zusage — beim nächsten Meilenstein fehlt wieder einer. +- **Vor *und* nach dem Lauf.** Ein `globalTeardown` läuft bei Testfehlern und bei Zeitabläufen, aber + nicht bei `Strg+C`. Der Vorlauf fängt genau das: ein Vorlauf lässt sich nicht überspringen. Er + **sagt es laut** („Rückstand aus einem früheren Lauf: …“), denn still wegzuräumen hieße, die + Ursache zu verdecken. +- **Das Präfix ist `ZZ` und nicht `ZZ E2E`.** Die Abläufe benutzen es in fünf Schreibweisen + (`ZZ E2E Editor …`, `ZZ-E2E--PC-01` — ein Hostname darf kein Leerzeichen haben —, + `ZZPruef…`, `ZZQuelle…`, `ZZ Abteilung …`). Alle fünf sind sinnvoll, alle fünf beginnen mit `ZZ`. + Darauf zu greifen deckt sie ab, ohne sechzehn Dateien umzubenennen. +- **Ein struktureller Test bewacht das** (`tests/betriebsstand.test.ts`): er liest die Abläufe und + schlägt an, wenn einer Zeilen anlegt, ohne das Präfix zu benutzen — und prüft, dass Vor- und + Nachlauf registriert sind. Er fand beim ersten Durchgang sofort zwei Dateien, die ich übersehen + hätte. +- **Protokoll-Waisen werden nicht erraten.** Eine Zeile, deren Gegenstand schon fort ist, bleibt + stehen; sie über den Namen im Json zu greifen wäre naheliegend und falsch, weil derselbe Name in + echter Historie einer noch bestehenden Zeile steht. Dass eine Protokollzeile ihren Gegenstand + überlebt, ist der Grund, warum `entitaetId` kein Fremdschlüssel ist. + +## Was der erste echte Lauf zutage brachte + +Der E2E-Durchgang war die ganze Zeit **rot**, und niemand hat es gemerkt — `pnpm test:e2e` steht +nicht im Tor vor dem Commit (Reviewbefund zu [#029](029-scim-endpunkt.md)). Sieben Fehlschläge, vier +Ursachen, keine davon ein Produktfehler, alle vier behoben: + +- `laeufe.spec.ts` zählte die Felder des Zustands auf und kannte `anzahlAnhaenge` nicht — das kam mit + #033 dazu. Die Aufzählung ist Absicht und bleibt: der Zustand ist die Antwort, die am häufigsten + hinausgeht, und ein Feld, das sich einschleicht, wandert ungesehen mit. +- `stammdaten.spec.ts` suchte „kommt aus dem Verzeichnis“ — seit #020 steht derselbe Satz auch am + Vorgesetzten-Feld, also zweimal auf der Seite. Playwright wies das als „strict mode violation“ ab. +- `vorlagen-editor.spec.ts` zog am Handy feste 300 px. Der Abstand zwischen den Anfassern ist + **532 px**, seit mit #030 und #031 Bausteinverweis und Abteilungsfilter in jeden Abschnitt kamen — + gemessen, nicht geschätzt. Dazu lag der erste Anfasser bei `y = -115`, also außerhalb des + Sichtfensters: der Touch-Start traf nichts. Jetzt wird erst gescrollt und die Zugweite aus dem + Layout genommen. Eine feste Pixelzahl in einem Sortiertest ist eine Zusage über das Layout, die + niemand pflegt. +- `onboarding.spec.ts` prüft den `New-ADUser`-Befehl zeichengenau und scheiterte an + `DC=firma,DC=de` statt `DC=firma,DC=local`: die **Entwicklungsdaten** wichen vom Seed ab, und der + Seed setzt bestehende Abteilungen absichtlich nicht zurück. Der Vorlauf prüft das jetzt und sagt, + was zu tun ist — statt einer Meldung, die nach einem Fehler im Befehlsbau aussieht. + +**Daraus folgt ein eigenes Issue:** `pnpm test:e2e` gehört ins Tor oder mindestens in eine +nachvollziehbare Regel, sonst wiederholt sich das. Siehe [#038](038-e2e-im-tor.md). diff --git a/docs/issues/036-reviewbefunde-phase-2.md b/docs/issues/036-reviewbefunde-phase-2.md new file mode 100644 index 0000000..40bd73d --- /dev/null +++ b/docs/issues/036-reviewbefunde-phase-2.md @@ -0,0 +1,136 @@ +# #036 Reviewbefunde aus der Durchsicht von Phase 2 + +**Status:** erledigt +**Im Tracker:** – +**Meilenstein:** v0.2.0 +**Labels:** phase-2, backend, test, docs +**Geschätzt:** 4 h **Tatsächlich:** 3 h 40 min + +## Ziel + +Acht Meilensteine von Phase 2 (M15–M22) lagen ohne fremden Blick auf `main`. Ein Review-Agent, der +den Code nicht geschrieben hat, hat sie gegen die Akzeptanzkriterien und gegen `CLAUDE.md` geprüft — +mit eigener Datenbank, Gegenproben und einem vollen Suitenlauf. + +**Ergebnis:** Die Kernregeln halten. Fünf von fünf Gegenproben gegen Versionierung, +Mandantengrenze, Abschlussregel und Selbstauskunft wurden rot. Alle Befunde lagen am Rand — in zwei +Route Handlern, einem Export-Pfad, in Betriebsdokumentation und in Kommentaren. + +**Die drei wertvollsten Befunde waren Gegenproben, die grün blieben.** Das ist das wiederkehrende +Muster dieses Projekts: nicht der falsche Code fällt auf, sondern der Code, der *aussieht*, als +prüfte er etwas. + +## Behoben + +### Schwer + +- [x] **Der Link in der Mail „Verzeichnis-Abgleich fehlgeschlagen“ war ein 404.** Gebaut wurde + `/verwaltung/verzeichnis` — `verwaltung` ist der Name des **Rechts**, die Seite liegt unter + `/admin/verzeichnis`. Das ist die eine Meldung, die an **alle** Administratoren geht, und + zwar im Moment eines Ausfalls: der Empfänger klickt und landet auf der + Nicht-gefunden-Seite. Die Pfade stehen jetzt als `MELDUNGS_PFADE` an einer Stelle, und ein + Test prüft **jeden gegen die Routen unter `src/app/`** — ein Pfad, den es nicht gibt, fällt + beim Testlauf auf und nicht im Postfach. +- [x] **Der Export eines Entwurfs mit Baustein lieferte einen leeren Abschnitt**, unter dem + Editor-Platzhalter „wird ersetzt“, und der Import nahm das stillschweigend an. Ein Redakteur + bringt eine noch unveröffentlichte Vorlage in eine zweite Installation — genau der Zweck des + Formats —, und der geteilte Abschnitt kommt dort ohne einen einzigen Schritt an. Baustein- + Abschnitte werden beim Export jetzt **aufgelöst**, wie beim Veröffentlichen; der Verweis + wandert weiterhin nicht mit. + +### Mittel + +- [x] **Der Route Handler, der Anhänge ausliefert, hatte keinen einzigen Test.** Die ganze + Rechteprüfung ließ sich durch einen hartcodierten ADMIN ersetzen, ohne dass ein Test rot + wurde. Das ist die **einzige** Stelle der Anwendung, die rohe Dateibytes ausliefert. Neu: + `tests/anhang-route.test.ts` fährt den Handler echt, acht Prüfungen — darunter das + Akzeptanzkriterium von #032 („nicht abrufbar **über seine Adresse**“), das bis hierher nur + auf Dienstebene geprüft war. Die Gegenprobe ist jetzt dreifach rot. +- [x] **`/api/scim` in der Proxy-Allowlist stand in keinem Test.** Die Zeile ließ sich entfernen, + ohne dass etwas rot wurde — und ohne sie leitet der Proxy jede Zustellung auf `/anmelden` um: + der Anbieter meldet „Provisioning failed“, in Laufzettel steht nichts, das Provisioning hört + still auf zu arbeiten. `OEFFENTLICH` ist jetzt exportiert, und ein Test prüft **in beide + Richtungen**: wer die Liste erweitert, muss es im Test aufschreiben. Eine Aufzählung, die nur + in einer Richtung geprüft wird, läuft dem Code hinterher. +- [x] **Ein offener Ereignisstrom überlebte Abmelden, Deaktivieren und Sitzungswiderruf.** Das + Recht wurde genau einmal geprüft, beim Aufbau, und Kennung samt Rolle für die Lebensdauer des + Stroms festgehalten. Ein Administrator deaktiviert ein Konto — und eine offen gelassene + Lasche bekommt weiter live Status, Schrittzustände und Zählwerte, bis jemand sie schließt. + `docs/architektur.md` verspricht „ein deaktivierter Benutzer wird auch mit gültigem Cookie + abgewiesen“; für einen Strom galt das nicht. Der Herzschlag prüft jetzt nach + (`sitzungNochGueltig`), eine Herabstufung engt den Strom ein statt ihn zu beenden. +- [x] **`README.md` kannte die Rolle `MITARBEITER` nicht**, und ein Satz daneben war seit #035 + falsch: „Unbekannte Benutzer werden beim ersten Login angelegt.“ Wer auf v0.2.0 hebt, liest + das README, nimmt an, für Leute ohne Rollengruppe bleibe alles wie bisher — und sperrt danach + genau diese Leute aus. #035 hatte den Satz in `docs/betrieb/authentik.md` richtiggestellt und + das README stehengelassen. Dazu: der Stand stand weiter auf „in Entwicklung auf v0.1.0“. +- [x] **`docs/betrieb/sicherung.md` nannte einen Anhänge-Pfad, den es nirgends gibt** + (`/opt/laufzettel/daten/anhaenge`) — als Rückfallwert in einem Skript, in dem ein `rm -rf` + steht. `deploy/install.sh` setzt `/var/lib/laufzettel/anhaenge`, und ohne die Variable nimmt + die Anwendung `./daten/anhaenge` relativ zu ihrem Arbeitsverzeichnis. Der Rückfall passte zu + **keinem** der drei. Jetzt `${ANHAENGE_PFAD:?…}`: fehlt die Variable, **bricht** der Befehl + ab, statt zu raten — vor dem `rm -rf`. Dasselbe Dokument hat denselben Fehler in #032 schon + einmal gehabt. +- [x] **Attributnamen wurden nur in der Server Action geprüft**, während sich + `src/server/graph/leser.ts` im Kommentar darauf berief („feste Feldnamen aus + `ATTRIBUT_MUSTER`, also ist hier nichts zu maskieren“). Nachgewiesen: ein Name mit Komma und + `$filter` kam durch den Dienst und schrieb einen **zweiten `$filter`** in die Graph-Adresse. + Hereinkommen konnte er nur mit `verwaltung` oder per Skript, Seed und `psql`, die + Sicherheitswirkung ist also klein — der Befund ist der **Ort** der Regel. Jetzt + `zuordnungPruefen` im Dienst (wirft) und Verwerfen in `zuordnungLesen` (damit eine kaputte + Zeile nicht die Quellenliste sprengt). + +### Leicht + +- [x] **Drei Kommentare versprachen mehr als der Code hält.** Der Kommentar am Konstanten-Block der + Wartezeiten sagte „nach dem vierten Fehlschlag wird aufgegeben“, während es der fünfte ist — + und das ist der Kommentar, den man zuerst liest. Die Migration der Benachrichtigungen + behauptete „der Dienst prüft dasselbe noch einmal, nur mit besserer Meldung“; tut er nicht, + und er soll es auch nicht (zwei Instanzen hinter einem Lastverteiler fragen beide und + bekommen beide „nein“ — nur der partielle Index hält). **Die Migration selbst bleibt + unverändert:** eine angewandte Migration wird nicht umgeschrieben, so wie eine + veröffentlichte `VorlagenVersion` nicht. Die Richtigstellung steht bei `einreihen`, wo sie + gelesen wird. +- [x] **Der Protokolleintrag „UPN mehrdeutig“ nannte immer „2“**, auch bei sieben Treffern: + `personZuUpn` hatte `take: 2`, und dieselbe Zahl ging in den Eintrag. Wer das Protokoll + liest, liest es, **um den Fall aufzulösen** — er muss wissen, wie viele Zeilen zu bereinigen + sind. `take` entfernt, Test mit vier Treffern. +- [x] **`anzahlSchritte` im Protokoll eines neuen Laufs zählte die Version, nicht den Lauf.** Bei + einer Vorlage mit abteilungsgebundenen Abschnitten (#031) behauptete der Eintrag mehr + Schritte, als der Lauf hat. Das Protokoll ist seit #023 ansehbar. +- [x] **Die Phase-2-Seiten fehlten in der 390-px-Prüfung.** Ergänzt: Bausteinliste, Schnellstart, + Protokoll, Zustellungen — gerade die Tabellen, also die Stellen, an denen horizontales + Scrollen entsteht. Jedes Issue berichtet eine Klickprobe von Hand; eine Probe von Hand ist + kein Regressionstest. +- [x] **Der Graph-Seitenabruf folgte `@odata.nextLink` ohne Host-Prüfung**, mit einem mandantenweit + gültigen Token im Kopf. Graph über TLS zu trauen ist vertretbar — aber `GRAPH_API_URL` ist + absichtlich einstellbar (nationale Clouds, Test-Doppelgänger), und ein Tippfehler darin + schickte das Token woandershin. `naechsteSeite` vergleicht jetzt den Ursprung. + +## Nicht behoben, bewusst + +- **`anhaengeDesLaufs` liegt in `services/` und wendet keine Mandantensicht an.** Beide Aufrufer + prüfen den Lauf vorher, und die Kommentare sagen das auch — aber es steht gegen die Regel aus + `CLAUDE.md` („`queries/*` wenden ihre Sicht selbst an, damit kein Aufrufer sie vergessen kann“). + Zwei Aufrufer in zwei Schichten gibt es schon; der dritte ist der, der es vergisst. Eigenes + Issue: [#037](037-anhaenge-lesemodell.md). +- **`/meine-geraete` fehlt weiter in der 390-px-Prüfung.** Die Seite verlangt `selbstauskunft`, und + das hat nur `MITARBEITER`; der E2E-Ablauf meldet sich als Admin an und würde die + Verweigerungsseite messen. Dafür braucht es ein eigenes Konto samt Personenbindung — siehe + [#037](037-anhaenge-lesemodell.md). +- **Das Graph-Token wird einmal je Abgleich geholt**, vor der Seitenschleife. Bei einem Mandanten, + dessen Abgleich länger als die Token-Lebensdauer läuft, hilft der Sicherheitsabstand nicht. Das + gehört auf die offene Prüfliste in `docs/betrieb/entra-id.md` und ist ohne echten Mandanten nicht + zu beurteilen. + +## Was der Review *nicht* gefunden hat + +Benannt, weil es den Befunden ihr Gewicht gibt: keine `kundeId` oder `quelleId` aus der Eingabe +statt aus Sitzung bzw. Token; kein Lesemodell, das seine Sicht ganz vergisst; keine +Client-Komponente mit einem Wert-Import aus `src/server/*`; kein Geheimnis in einer Sicht, Antwort +oder einem Protokolleintrag; kein gespeichertes `{{generiertes_passwort}}`; kein Codepfad, der +annimmt, dass nur ein Mensch einen Schritt erledigt. + +Ein Befund des Reviews war **falsch**: der Zustands-Handler habe keinen Vitest-Test. Er hat acht +direkte Aufrufe in `tests/lauf-ereignisse.test.ts`, darunter „weist ohne Anmeldung ab“ und „liefert +für einen fremden Kunden keinen Strom“. Nachgeprüft, bevor etwas geändert wurde. diff --git a/docs/issues/037-anhaenge-lesemodell.md b/docs/issues/037-anhaenge-lesemodell.md new file mode 100644 index 0000000..bf66a31 --- /dev/null +++ b/docs/issues/037-anhaenge-lesemodell.md @@ -0,0 +1,53 @@ +# #037 Zwei Nachzügler aus der Durchsicht von Phase 2 + +**Status:** offen +**Im Tracker:** – +**Meilenstein:** v0.3.0 +**Labels:** phase-2, backend, test +**Geschätzt:** 2 h **Tatsächlich:** – + +## Ziel + +Zwei Befunde aus [#036](036-reviewbefunde-phase-2.md), die mehr sind als eine Korrektur: beide +verlangen einen Umbau, und beide sind heute kein Schaden, sondern eine offene Flanke. + +## 1. `anhaengeDesLaufs` ist ein Lesemodell am falschen Ort + +`anhaengeDesLaufs(laufId)` liegt in `src/server/services/anhaenge.ts`, nimmt nur eine Lauf-Kennung +und wendet **weder Rollen- noch Mandantensicht** an. Beide Aufrufer +(`src/app/(app)/laeufe/[id]/page.tsx` und `src/server/queries/druck.ts`) prüfen den Lauf vorher, und +die Kommentare sagen das auch — es ist also heute kein Fehler. + +Es steht aber gegen die Regel aus `CLAUDE.md` und `docs/architektur.md` §2: Lesemodelle liegen in +`queries/*` und **wenden ihre Sicht selbst an, damit kein Aufrufer sie vergessen kann**. Zwei +Aufrufer in zwei Schichten gibt es schon; der dritte ist der, der es vergisst — und er wird nicht +auffallen, weil die zwei bestehenden korrekt sind. + +- [ ] Nach `src/server/queries/anhaenge.ts` ziehen, mit `HandelnderBenutzer` als Parameter +- [ ] Mandanten- und Rollensicht dort anwenden, nicht beim Aufrufer +- [ ] Test: ein Benutzer, der auf andere Kunden beschränkt ist, bekommt eine leere Liste — **ohne** + dass der Aufrufer vorher geprüft hat +- [ ] Gegenprobe: die Sicht entfernen, der Test muss rot werden + +## 2. `/meine-geraete` fehlt in der 390-px-Prüfung + +Die Seite ist das Ergebnis von #035 und ausdrücklich mit „am Handy kein horizontales Scrollen“ +abgenommen — von Hand. In `tests/e2e/mobil.spec.ts` steht sie nicht, und sie kann dort nicht +einfach ergänzt werden: sie verlangt das Recht `selbstauskunft`, und das hat nur `MITARBEITER`. Der +Ablauf meldet sich als Admin an und würde die Verweigerungsseite messen — die scrollt garantiert +nicht. + +- [ ] Ein `MITARBEITER`-Konto für die Abläufe: lokales Passwort, verknüpft mit einer + `Mitarbeiter`-Zeile (der CHECK `Benutzer_mitarbeiter_nur_bei_rolle` verlangt die Verknüpfung) +- [ ] Im Seed oder im E2E-Vorlauf? **Im Vorlauf**, denn ein Konto mit bekanntem Passwort gehört + nicht in einen Seed, der auch produktiv läuft +- [ ] `/meine-geraete` in die Seitenliste von `mobil.spec.ts` +- [ ] Dabei prüfen, was ein `MITARBEITER` auf den **anderen** Seiten sieht: die Rollenmatrix ist in + Vitest vollständig, aber kein Ablauf klickt sie durch + +## Akzeptanzkriterien + +- [ ] `anhaengeDesLaufs` liegt in `queries/`, und ein Test belegt die Sicht ohne Vorprüfung des + Aufrufers +- [ ] `/meine-geraete` wird bei 390 px gemessen, mit einem echten `MITARBEITER` +- [ ] Beide Gegenproben werden rot diff --git a/docs/issues/038-e2e-im-tor.md b/docs/issues/038-e2e-im-tor.md new file mode 100644 index 0000000..73e700d --- /dev/null +++ b/docs/issues/038-e2e-im-tor.md @@ -0,0 +1,54 @@ +# #038 Die E2E-Abläufe laufen in keinem Tor und waren monatelang rot + +**Status:** offen +**Im Tracker:** – +**Meilenstein:** v0.3.0 +**Labels:** phase-2, test, infra +**Geschätzt:** 3 h **Tatsächlich:** – + +## Ziel + +Beim Abschluss von [#027](027-e2e-reste-vorlagen.md) lief `pnpm test:e2e` erstmals seit dem +26. September wieder vollständig. **Sieben Abläufe waren rot**, und niemand hatte es gemerkt. + +Keiner der sieben war ein Produktfehler — und genau das ist das Problem. Vier Ursachen: + +| Ablauf | Ursache | Seit | +|---|---|---| +| `laeufe.spec.ts` | zählt die Felder des Zustands auf, `anzahlAnhaenge` fehlte | #033 | +| `stammdaten.spec.ts` | „kommt aus dem Verzeichnis“ steht seit #020 zweimal auf der Seite | #020 | +| `vorlagen-editor.spec.ts` | zieht am Handy feste 300 px, der Abstand ist 532 px | #030/#031 | +| `onboarding.spec.ts` | die **Entwicklungsdaten** wichen vom Seed ab | unbekannt | + +Alle vier sind behoben. Das Muster bleibt: **die E2E-Schicht trägt Zusagen, die das Commit-Tor nicht +fährt** — Rückfall auf Polling, Herzschlag, 390 px, die Auslieferung eines Anhangs über seine +Adresse. Steht eine Zusage nur in Playwright, und läuft Playwright nur gelegentlich, dann ist sie +eine Notiz und keine Prüfung. + +## Zu entscheiden + +- [ ] **`pnpm test:e2e` ins Tor vor dem Commit?** Dagegen: es verlangt einen **gebauten, + laufenden** Stand (`pnpm build && pnpm start`), dauert über vier Minuten und braucht eine + Datenbank mit Seed-Bestand. Vier Minuten vor jedem Commit ist ein Tor, das umgangen wird. +- [ ] **Oder eine kleinere, verbindliche Regel:** E2E läuft vor jedem Merge nach `main` — das wären + acht Läufe in Phase 2 gewesen und hätte jeden der vier Befunde am Tag seiner Entstehung + gezeigt. Das ist die Empfehlung. +- [ ] **Oder im CI** (`.gitea/workflows/ci.yml`). Dort ist kein Runner registriert; das zu ändern + ist eine eigene Entscheidung und gehört nicht in dieses Issue. + +## Aufgaben + +- [ ] Die gewählte Regel in `CLAUDE.md` aufschreiben, mit **gemessener** Dauer und der Begründung, + warum sie nicht im Commit-Tor steht +- [ ] Die Voraussetzungen, die `tests/e2e/global-setup.ts` prüft, in derselben Reihenfolge + dokumentieren — der Vorlauf ist inzwischen die ehrlichste Beschreibung dessen, was E2E braucht +- [ ] Prüfen, welche Zusagen **ausschließlich** in E2E stehen, und für jede entscheiden: gehört sie + zusätzlich in einen Vitest-Test, der den Handler direkt fährt? (Das Muster steht in + `tests/scim.test.ts`, `tests/anhang-route.test.ts` und `tests/lauf-ereignisse.test.ts`.) +- [ ] Mindestens die Rechteprüfung der beiden Phase-2-Handler ist damit schon abgedeckt — bleiben + Rückfall auf Polling, Herzschlag am echten Browser und 390 px + +## Akzeptanzkriterien + +- [ ] In `CLAUDE.md` steht, wann E2E läuft, und die Angabe ist gemessen, nicht geschätzt +- [ ] Eine Liste der Zusagen, die nur in E2E stehen — und zu jeder eine Entscheidung diff --git a/docs/issues/README.md b/docs/issues/README.md index fc95732..3304ec5 100644 --- a/docs/issues/README.md +++ b/docs/issues/README.md @@ -31,7 +31,7 @@ Diese Dateien sind die Quelle; `pnpm issues:sync` spiegelt sie in den Gitea-Trac | #024 | [Proxmox-Einzeiler auf einem echten PVE-Host gegenprüfen](024-proxmox-echtlauf.md) | offen | v0.2.0 | 2 h | – | | #025 | [Healthcheck gibt die rohe Datenbankfehlermeldung heraus](025-gesundheit-fehlermeldung.md) | erledigt | v0.2.0 | 1 h | 25 min | | #026 | [Zwei Konventionen präzisieren, die strenger klingen als sie gemeint sind](026-konventionen-praezisieren.md) | erledigt | v0.2.0 | 1 h | 20 min | -| #027 | [Die Editor-Abläufe lassen ihre Vorlagen stehen](027-e2e-reste-vorlagen.md) | offen | v0.2.0 | 1 h | – | +| #027 | [Die Editor-Abläufe lassen ihre Vorlagen stehen](027-e2e-reste-vorlagen.md) | erledigt | v0.2.0 | 1 h | 2 h 10 min | | #028 | [M15 – Quellenarten und Microsoft-Graph-Quelle](028-graph-quelle.md) | erledigt | v0.2.0 | 10 h | 4 h 10 min | | #029 | [M16 – SCIM-Endpunkt als Alternative zum Abgleich](029-scim-endpunkt.md) | erledigt | v0.2.0 | 12 h | 4 h 05 min | | #030 | [M17 – Wiederverwendbare Bausteine](030-bausteine.md) | erledigt | v0.2.0 | 12 h | 2 h 25 min | @@ -40,4 +40,7 @@ Diese Dateien sind die Quelle; `pnpm issues:sync` spiegelt sie in den Gitea-Trac | #033 | [M20 – Live-Aktualisierung per SSE statt Polling](033-sse-live.md) | erledigt | v0.2.0 | 6 h | 1 h 40 min | | #034 | [M21 – Benachrichtigungen per Mail und Webhook](034-benachrichtigungen.md) | erledigt | v0.2.0 | 10 h | 3 h 30 min | | #035 | [M22 – Mitarbeiter-Selbstauskunft](035-selbstauskunft.md) | erledigt | v0.2.0 | 8 h | 2 h 50 min | -| | **Summe** | | | **229 h** | **64 h 40 min** | +| #036 | [Reviewbefunde aus der Durchsicht von Phase 2](036-reviewbefunde-phase-2.md) | erledigt | v0.2.0 | 4 h | 3 h 40 min | +| #037 | [Zwei Nachzügler aus der Durchsicht von Phase 2](037-anhaenge-lesemodell.md) | offen | v0.3.0 | 2 h | – | +| #038 | [Die E2E-Abläufe laufen in keinem Tor und waren monatelang rot](038-e2e-im-tor.md) | offen | v0.3.0 | 3 h | – | +| | **Summe** | | | **238 h** | **70 h 30 min** | diff --git a/playwright.config.ts b/playwright.config.ts index ca594dd..85b855f 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -11,6 +11,12 @@ export default defineConfig({ * erst mitten im Lauf auf — als Zeitablauf oder als „0 Treffer“ (#018). */ globalSetup: "./tests/e2e/global-setup.ts", + /* + * Raeumt weg, was die Ablaeufe angelegt haben (#027). Laeuft auch nach + * fehlgeschlagenen Ablaeufen; ein harter Abbruch wird vom Vorlauf gefangen, + * der dasselbe tut. + */ + globalTeardown: "./tests/e2e/global-teardown.ts", // Die Ablaeufe aendern gemeinsame Daten, deshalb nacheinander. fullyParallel: false, workers: 1, diff --git a/src/app/api/laeufe/[id]/zustand/route.ts b/src/app/api/laeufe/[id]/zustand/route.ts index b24ca85..0da73ff 100644 --- a/src/app/api/laeufe/[id]/zustand/route.ts +++ b/src/app/api/laeufe/[id]/zustand/route.ts @@ -1,4 +1,5 @@ -import { RechteFehler, rechtErforderlich } from "@/server/auth/sitzungspflicht"; +import { RechteFehler, rechtErforderlich, sitzungNochGueltig } from "@/server/auth/sitzungspflicht"; +import type { Sitzung } from "@/lib/sitzung"; import { laufZustand } from "@/server/queries/laeufe"; import { HERZSCHLAG, herzschlagMs, maxStroeme, sseEreignis, willStrom } from "@/lib/sse"; import { aufLaufHoeren, stroemeOffen } from "@/server/lauf-ereignisse"; @@ -40,10 +41,14 @@ export async function GET( ): Promise { let benutzerId: string; let rolle: Parameters[1]["rolle"]; + // Die Sitzung wird festgehalten, weil der Strom sie spaeter noch braucht: + // `cookies()` gibt es in seinen Rueckrufen nicht mehr. + let sitzung: Sitzung; try { - const { benutzer } = await rechtErforderlich("lesen"); - benutzerId = benutzer.id; - rolle = benutzer.rolle; + const zugang = await rechtErforderlich("lesen"); + benutzerId = zugang.benutzer.id; + rolle = zugang.benutzer.rolle; + sitzung = zugang.sitzung; } catch (fehler) { if (fehler instanceof RechteFehler) { // 401 und nicht 403: der Client soll die Aktualisierung einstellen und @@ -54,7 +59,7 @@ export async function GET( } const { id } = await params; - const benutzer = { id: benutzerId, rolle }; + let benutzer = { id: benutzerId, rolle }; const zustand = await laufZustand(id, benutzer); if (!zustand) { // Nicht vorhanden und nicht sichtbar sind dieselbe Antwort – die @@ -151,7 +156,31 @@ export async function GET( return; } - herzschlag = setInterval(() => schicken(HERZSCHLAG), herzschlagMs()); + /* + * **Der Herzschlag prueft auch die Sitzung nach** (Reviewbefund zu #029). + * Vorher wurde das Recht genau einmal geprueft, beim Aufbau, und das Paar + * aus Kennung und Rolle fuer die Lebensdauer des Stroms festgehalten — ein + * deaktiviertes Konto bekam weiter live Daten, bis jemand die Lasche + * schloss. Die Pruefung kostet eine Abfrage je offenem Strom und + * Herzschlag (25 s); dieselbe Abfrage macht jede gewoehnliche Anfrage + * ohnehin. + * + * Eine **Herabstufung** beendet den Strom nicht, sondern engt ihn ein: die + * neue Rolle gilt ab dem naechsten Ereignis. Das ist dieselbe Regel wie + * „Rolle pro Anfrage aus der Datenbank“ (CLAUDE.md). + */ + herzschlag = setInterval(() => { + void (async () => { + const noch = await sitzungNochGueltig(sitzung, "lesen"); + if (!noch) { + schicken(sseEreignis("ende", JSON.stringify({ grund: "nicht-angemeldet" }))); + aufraeumen(); + return; + } + if (noch.rolle !== benutzer.rolle) benutzer = { id: benutzerId, rolle: noch.rolle }; + schicken(HERZSCHLAG); + })(); + }, herzschlagMs()); anfrage.signal.addEventListener("abort", aufraeumen); }, diff --git a/src/lib/verzeichnis.ts b/src/lib/verzeichnis.ts index e5958c6..7f9d0c8 100644 --- a/src/lib/verzeichnis.ts +++ b/src/lib/verzeichnis.ts @@ -151,11 +151,49 @@ export function zuordnungLesen(roh: unknown, art: "LDAP" | "GRAPH" = "LDAP"): At const gelesen = { ...ZUORDNUNGS_VORGABE[art] }; for (const schluessel of ZUORDNUNGS_FELDER) { const wert = felder[schluessel]; - if (typeof wert === "string" && wert.trim()) gelesen[schluessel] = wert.trim(); + if (typeof wert !== "string" || !wert.trim()) continue; + /* + * **Was kein Attributname ist, wird beim Lesen verworfen** und nicht + * weitergegeben. Hier zu werfen waere falsch: eine einzige kaputte Zeile in + * der Datenbank machte sonst die ganze Quellenliste unbenutzbar. Dass so + * etwas gar nicht erst hineinkommt, haelt `zuordnungPruefen` beim Schreiben. + * + * Der Grund fuer beides: aus diesen Namen entstehen eine LDAP-Attributliste + * und ein `$select` in der Graph-Adresse. `src/server/graph/leser.ts` berief + * sich im Kommentar auf diese Pruefung, waehrend sie nur in der Server Action + * stand — ein Name mit Komma und `$filter` kam durch den Dienst und landete + * in der Abfrage (Reviewbefund zu #029). + */ + if (!ATTRIBUT_MUSTER.test(wert.trim())) continue; + gelesen[schluessel] = wert.trim(); } return gelesen; } +/** + * Prueft eine Zuordnung, die **geschrieben** werden soll. Wirft nicht, sondern + * meldet — der Aufrufer entscheidet, ob daraus eine Fehlermeldung oder eine + * Ausnahme wird. + * + * Getrennt von `zuordnungAusEingabe`, weil die aus **Formularfeldern** liest und + * dabei leere Felder herausfallen laesst. Diese hier prueft eine fertige + * Zuordnung, egal woher sie kommt: Formular, Skript, Seed oder Import. + */ +export function zuordnungPruefen(zuordnung: Partial>): string | undefined { + for (const [schluessel, wert] of Object.entries(zuordnung)) { + if (wert === undefined || wert === null) continue; + if (typeof wert !== "string" || !ATTRIBUT_MUSTER.test(wert.trim())) { + const beschriftung = + (ZUORDNUNGS_BESCHRIFTUNG as Record)[schluessel] ?? schluessel; + return ( + `„${String(wert)}“ ist kein Attributname (${beschriftung}). ` + + "Erwartet werden Buchstaben, Ziffern und Bindestriche, beginnend mit einem Buchstaben." + ); + } + } + return undefined; +} + /** * Die Attributzuordnung aus den Formularfeldern, geprueft. * diff --git a/src/proxy.ts b/src/proxy.ts index 8cbc3b6..f3c8375 100644 --- a/src/proxy.ts +++ b/src/proxy.ts @@ -18,8 +18,17 @@ import { SITZUNG_COOKIE } from "@/lib/sitzung"; * den Proxy zu haengen. */ -/** Pfade, die ohne Anmeldung erreichbar sein muessen. */ -const OEFFENTLICH: readonly string[] = [ +/** + * Pfade, die ohne Anmeldung erreichbar sein muessen. + * + * **Exportiert, damit der Test die Liste nicht abschreiben muss.** Vorher zaehlte + * `tests/zugang.test.ts` die Pfade von Hand auf, und seit #029 fehlte dort + * `/api/scim` — eine Aufzaehlung laeuft dem Code hinterher (Reviewbefund zu + * #029). Jetzt prueft ein Test, dass **jeder** Eintrag hier auch im Test steht: + * ein neuer oeffentlicher Pfad ist eine Sicherheitsentscheidung und soll jemanden + * zwingen, sie aufzuschreiben. + */ +export const OEFFENTLICH: readonly string[] = [ "/anmelden", // Der Anmeldevorgang selbst – ohne ihn koennte sich niemand anmelden. "/api/auth", diff --git a/src/server/auth/benutzer.ts b/src/server/auth/benutzer.ts index 20c78e7..2480a8a 100644 --- a/src/server/auth/benutzer.ts +++ b/src/server/auth/benutzer.ts @@ -137,10 +137,19 @@ async function personZuUpn( const gesucht = upn.trim(); if (!gesucht) return { treffer: "keiner" }; + /* + * **Ohne `take`, damit die gemeldete Zahl die Zahl ist.** Vorher stand hier + * `take: 2` — fuer die Entscheidung genuegt das, aber `anzahl` ging damit in + * den Protokolleintrag „Anmeldung abgewiesen: UPN mehrdeutig“, und der sagte + * dann immer „2“, auch bei sieben Treffern (Reviewbefund zu #029). Wer das + * Protokoll liest, um den Fall aufzuloesen, bekam eine Zahl, die keine war. + * + * Die Abfrage laeuft gegen einen Index auf `upn` und liefert im Normalfall + * **einen** Treffer; mehrere sind ein Datenfehler, den jemand beheben muss. + */ const passende = await prisma.mitarbeiter.findMany({ where: { upn: { equals: gesucht, mode: "insensitive" }, aktiv: true }, select: { id: true, kundeId: true }, - take: 2, }); const erster = passende[0]; diff --git a/src/server/auth/sitzungspflicht.ts b/src/server/auth/sitzungspflicht.ts index 5d2a6e9..258d0d0 100644 --- a/src/server/auth/sitzungspflicht.ts +++ b/src/server/auth/sitzungspflicht.ts @@ -5,6 +5,7 @@ import type { Benutzer } from "@/generated/prisma/client"; import { prisma } from "@/lib/db"; import { SITZUNG_COOKIE, sitzungLesen, type Sitzung } from "@/lib/sitzung"; import { darf, type Recht } from "@/server/auth/rollen"; +import type { Rolle } from "@/generated/prisma/enums"; /** * Wer ist angemeldet und was darf er (#007). @@ -55,6 +56,39 @@ export type Angemeldet = { readonly sitzung: Sitzung; }; +/** + * Gilt diese Sitzung **jetzt noch**, und darf sie `recht`? + * + * Fuer langlebige Verbindungen. `angemeldet()` liest das Cookie und laeuft + * deshalb nur innerhalb einer Anfrage; ein Ereignisstrom lebt Minuten bis + * Stunden **nach** dem Ende der Anfrage weiter, und in seinen Rueckrufen gibt es + * keinen `cookies()`-Kontext mehr. Die Sitzung selbst ist zu dem Zeitpunkt + * laengst geprueft — nachzusehen ist, was sich seither in der Datenbank + * geaendert haben kann: `aktiv`, die Rolle und `sitzungenUngueltigAb`. + * + * **Warum das noetig ist:** Ohne diese Pruefung bekam eine offen gelassene + * Lasche weiter live Daten, nachdem ein Administrator das Konto deaktiviert und + * die Sitzungen beendet hatte — bis jemand die Lasche schloss. `docs/architektur.md` + * verspricht „ein deaktivierter Benutzer wird auch mit gueltigem Cookie + * abgewiesen“, und fuer einen offenen Strom galt das nicht (Reviewbefund zu #029). + * + * Die Rolle wird **mit**geliefert, nicht nur geprueft: wer waehrend eines Stroms + * herabgestuft wird, soll von da an die Sicht seiner neuen Rolle bekommen — das + * ist dieselbe Regel wie „Rolle pro Anfrage aus der Datenbank“ (CLAUDE.md). + */ +export async function sitzungNochGueltig( + sitzung: Sitzung, + recht: Recht, +): Promise<{ readonly rolle: Rolle } | undefined> { + const benutzer = await prisma.benutzer.findUnique({ + where: { id: sitzung.benutzerId }, + select: { id: true, aktiv: true, rolle: true, sitzungenUngueltigAb: true }, + }); + if (!sitzungBewerten(sitzung, benutzer).gueltig || !benutzer) return undefined; + if (!darf(benutzer.rolle, recht)) return undefined; + return { rolle: benutzer.rolle }; +} + /** * Der angemeldete Benutzer oder `undefined`. Leitet nicht weiter – dafuer gibt es * `sitzungErforderlich`. diff --git a/src/server/graph/leser.ts b/src/server/graph/leser.ts index 331bdaf..3f9bec6 100644 --- a/src/server/graph/leser.ts +++ b/src/server/graph/leser.ts @@ -102,6 +102,35 @@ export function angefragteFelder(zuordnung: AttributZuordnung): string[] { */ export const GRAPH_FILTER = "accountEnabled eq true"; +/** + * Die naechste Seitenadresse — **nur, wenn sie auf denselben Host zeigt**. + * + * Jede Seite wird mit `Authorization: Bearer ` geholt, und das Token gilt + * mandantenweit (`User.Read.All`). Der Antwort von Graph ueber TLS zu trauen ist + * vertretbar, aber `GRAPH_API_URL` ist absichtlich einstellbar — fuer nationale + * Clouds und fuer den Test-Doppelgaenger. Ein Tippfehler darin oder ein + * aufbrechender Firmenproxy dazwischen wuerde das Token sonst an eine fremde + * Adresse schicken (Reviewbefund zu #029). + * + * Die Pruefung kostet eine Zeile und schliesst den Fall. Ein `nextLink`, der + * woandershin zeigt, beendet den Abruf still: er ist kein Fehler des + * Verzeichnisses, und ein Abbruch mitten im Abgleich waere schlimmer als eine + * unvollstaendige Seite — der naechste Lauf holt sie. + */ +export function naechsteSeite(weiter: unknown, basis = graphBasis()): string | undefined { + if (typeof weiter !== "string" || !weiter.trim()) return undefined; + try { + if (new URL(weiter).origin !== new URL(basis).origin) { + console.error(`Graph: @odata.nextLink zeigt auf einen anderen Host als GRAPH_API_URL — Abruf beendet.`); + return undefined; + } + } catch { + // Keine brauchbare Adresse: dasselbe Ergebnis, nur ohne Erklaerung. + return undefined; + } + return weiter; +} + /** * Die erste Adresse der Abfrage. Danach folgt der Abruf `@odata.nextLink`. * @@ -272,7 +301,7 @@ export async function graphEintraegeStroemen( } const weiter = seite["@odata.nextLink"]; - adresse = typeof weiter === "string" && weiter ? weiter : undefined; + adresse = naechsteSeite(weiter); } return { gesamtGelesen, uebersprungen, seiten }; diff --git a/src/server/services/benachrichtigungen.ts b/src/server/services/benachrichtigungen.ts index aa3353f..e8c1891 100644 --- a/src/server/services/benachrichtigungen.ts +++ b/src/server/services/benachrichtigungen.ts @@ -39,10 +39,15 @@ export class MeldungsFehler extends Error { /** * Wie lange zwischen zwei Versuchen gewartet wird, in Minuten. * - * Die Laenge der Liste ist zugleich die Zahl der Versuche: nach dem vierten - * Fehlschlag wird aufgegeben. Wachsend, weil die haeufigste Ursache ein - * Mailserver ist, der gerade neu startet — eine Minute spaeter geht es meist - * wieder, und nach zwei Stunden ist es ein Fall fuer einen Menschen. + * **Vier Wartezeiten ergeben fuenf Versuche**, nicht vier: gewartet wird erst + * *nach* einem Fehlschlag, der fuenfte Versuch findet also keine Wartezeit mehr + * vor und wird aufgegeben. Der Kommentar hier behauptete „nach dem vierten“, + * waehrend `naechsterVersuch` weiter unten das Richtige sagt — und das ist der + * Kommentar, den man zuerst liest (Reviewbefund zu #029). + * + * Wachsend, weil die haeufigste Ursache ein Mailserver ist, der gerade neu + * startet — eine Minute spaeter geht es meist wieder, und nach zwei Stunden ist + * es ein Fall fuer einen Menschen. */ export const WARTEZEITEN_MINUTEN = [1, 5, 30, 120] as const; @@ -332,8 +337,26 @@ function appUrl(): string { return (process.env["APP_URL"]?.trim() || "").replace(/\/+$/, ""); } +/** + * Die Pfade, auf die eine Meldung zeigen kann — **an einer Stelle**. + * + * Sie stehen hier als Konstanten, weil ein Pfad in einer Mail nicht nachgesehen + * wird: er wird geklickt. `/verwaltung/verzeichnis` stand hier und war ein 404 — + * `verwaltung` ist der Name des **Rechts**, die Seite liegt unter `/admin/` + * (Reviewbefund zu #029). Aufgefallen ist das niemandem, weil der Test nur + * prueft, dass der **uebergebene** Link im Rumpf steht. + * + * `tests/benachrichtigungen.test.ts` prueft jeden dieser Pfade jetzt gegen die + * Routen unter `src/app/` — ein Pfad, den es nicht gibt, faellt beim Testlauf + * auf und nicht im Postfach eines Administrators. + */ +export const MELDUNGS_PFADE = { + lauf: "/laeufe", + verzeichnis: "/admin/verzeichnis", +} as const; + function laufLink(laufId: string): string { - return `${appUrl()}/laeufe/${laufId}`; + return `${appUrl()}${MELDUNGS_PFADE.lauf}/${laufId}`; } /** @@ -424,6 +447,16 @@ async function einreihen( * `skipDuplicates`, weil der partielle eindeutige Index fuer „ueberfaellig“ * genau hier zuschlaegt: der Zeitplan sieht jede Minute nach, und ohne das * waere jeder zweite Durchgang ein Fehler statt einer stillen Nichtaenderung. + * + * **Hier haelt nur die Datenbank, und das ist Absicht.** Eine zweite Pruefung + * im Dienst („gibt es die schon?“) waere ein Wettlauf: zwei Instanzen hinter + * einem Lastverteiler fragen beide und bekommen beide „nein“. Die Migration + * `20261005110000_benachrichtigungen` sagt im Kommentar das Gegenteil — sie + * verspricht eine Dienstpruefung „mit besserer Meldung“, die es nicht gibt und + * nicht geben soll (Reviewbefund zu #029). Eine angewandte Migration wird + * nicht nachtraeglich umgeschrieben, so wie eine veroeffentlichte + * `VorlagenVersion` nicht; die Richtigstellung steht deshalb hier, wo sie + * gelesen wird, und in `tests/datenbankregeln.test.ts`. */ const ergebnis = await prisma.zustellung.createMany({ data: zeilen, skipDuplicates: true }); return ergebnis.count; @@ -498,7 +531,7 @@ export async function abgleichMeldungEinreihen(quelleId: string, grund: string): const webhook = await webhookZugang(); if (admins.length === 0 && !webhook) return 0; - const link = `${appUrl()}/verwaltung/verzeichnis`; + const link = `${appUrl()}${MELDUNGS_PFADE.verzeichnis}`; const { betreff, rumpf } = abgleichMeldung(quelle.name, grund, link); return einreihen( Meldeanlass.ABGLEICH_FEHLGESCHLAGEN, diff --git a/src/server/services/laeufe.ts b/src/server/services/laeufe.ts index 4bffdd6..a9c54d5 100644 --- a/src/server/services/laeufe.ts +++ b/src/server/services/laeufe.ts @@ -566,7 +566,14 @@ export async function laufAnlegen(eingabe: NeuerLauf, benutzer: HandelnderBenutz vorlage: version.vorlagenName, vorlagenVersionId: version.id, status: LaufStatus.OFFEN, - anzahlSchritte: version.schrittIds.length, + /* + * Die Schritte **dieses Laufs**, nicht die der Version. Seit #031 + * fallen abteilungsfremde Abschnitte heraus; `version.schrittIds.length` + * behauptete deshalb mehr Schritte, als der Lauf hat (Reviewbefund zu + * #029). Das Protokoll ist seit #023 ansehbar — eine Zahl darin, die + * nicht zur Liste darunter passt, kostet jemanden eine Suche. + */ + anzahlSchritte: anzulegen.length, zugewiesenAnId, }, }, diff --git a/src/server/services/verzeichnis.ts b/src/server/services/verzeichnis.ts index 72b3490..0b958f2 100644 --- a/src/server/services/verzeichnis.ts +++ b/src/server/services/verzeichnis.ts @@ -14,6 +14,7 @@ import { INTERVALL_HOECHSTENS, INTERVALL_MINDESTENS, intervallMeldung, + zuordnungPruefen, } from "@/lib/verzeichnis"; import { eintraegeDerQuelleStroemen } from "@/server/verzeichnis/quellenleser"; import { urlPruefen } from "@/server/ldap/verbindung"; @@ -317,6 +318,18 @@ export async function quelleAnlegen( */ const felder = feldernJeArt(art, eingabe); + /* + * **Die Zuordnung wird hier geprueft, nicht nur in der Server Action.** + * Aus diesen Namen entsteht eine LDAP-Attributliste und ein `$select` in der + * Graph-Adresse; `src/server/graph/leser.ts` berief sich im Kommentar darauf, + * dass sie dem Muster entsprechen — waehrend die Pruefung eine Schicht + * darueber in `zuordnungAusFormular` stand. Ein Name mit Komma und `$filter` + * kam deshalb durch den Dienst und landete in der Abfrage (Reviewbefund zu + * #029). Ein Skript, der Seed und ein Import gehen nicht durch das Formular. + */ + const zuordnungsMangel = zuordnungPruefen(eingabe.attributZuordnung ?? {}); + if (zuordnungsMangel) throw new VerzeichnisFehler(zuordnungsMangel); + const zeile = await prisma.$transaction(async (tx) => { const angelegt = await tx.verzeichnisquelle.create({ data: { @@ -487,6 +500,8 @@ export async function quelleBearbeiten( } } if (aenderung.attributZuordnung !== undefined) { + const mangel = zuordnungPruefen(aenderung.attributZuordnung); + if (mangel) throw new VerzeichnisFehler(mangel); const zuordnung = zuordnungLesen({ ...zuordnungLesen(vorher.attributZuordnung), ...aenderung.attributZuordnung, diff --git a/src/server/services/vorlagen-austausch.ts b/src/server/services/vorlagen-austausch.ts index 60ea6d8..6cda312 100644 --- a/src/server/services/vorlagen-austausch.ts +++ b/src/server/services/vorlagen-austausch.ts @@ -42,7 +42,22 @@ export async function vorlageExportieren(versionId: string): Promise variablen: { orderBy: [{ position: "asc" }, { id: "asc" }] }, abschnitte: { orderBy: [{ position: "asc" }, { id: "asc" }], - include: { schritte: { orderBy: [{ position: "asc" }, { id: "asc" }] } }, + include: { + schritte: { orderBy: [{ position: "asc" }, { id: "asc" }] }, + /* + * Der Baustein mit. Ein Baustein-Abschnitt im **Entwurf** hat keine + * eigenen Schritte — die entstehen erst beim Veroeffentlichen. Ohne + * diese Zeilen exportierte ein Entwurf einen leeren Abschnitt unter + * dem Platzhaltertitel „wird ersetzt“ (Reviewbefund zu #029). + */ + bausteinVersion: { + select: { + titel: true, + baustein: { select: { name: true } }, + schritte: { orderBy: [{ position: "asc" }, { id: "asc" }] }, + }, + }, + }, }, }, }); @@ -68,13 +83,37 @@ export async function vorlageExportieren(versionId: string): Promise optionen: [...variable.optionen], pflicht: variable.pflicht, })), - abschnitte: version.abschnitte.map((abschnitt) => ({ - titel: abschnitt.titel, - // Wandert mit: ein Abteilungsname bedeutet in einer anderen Installation - // dasselbe. Der Baustein-Verweis wandert bewusst **nicht** mit (#030). - nurFuerAbteilungen: [...abschnitt.nurFuerAbteilungen], - schritte: abschnitt.schritte.map(schrittAbbilden), - })), + abschnitte: version.abschnitte.map((abschnitt) => { + /* + * **Ein Baustein-Abschnitt wird aufgeloest, nicht abgeschrieben.** + * + * Der Verweis selbst wandert bewusst nicht mit (#030): in der anderen + * Installation gibt es den Baustein vielleicht nicht, und eine Vorlage, + * die auf etwas Fehlendes zeigt, ist kein Austauschformat. Die **Inhalte** + * muessen aber mitwandern — und bei einem Entwurf stehen sie noch nicht + * am Abschnitt, sondern nur am Baustein. + * + * Vorher exportierte ein Entwurf deshalb `{"titel":"wird ersetzt", + * "schritte":[]}`: ein Redakteur brachte eine unveroeffentlichte Vorlage + * in eine zweite Installation, und der geteilte Abschnitt kam dort als + * leerer Abschnitt mit dem Editor-Platzhalter als Namen an. Die Checkliste + * war unvollstaendig, und nichts sagte es (Reviewbefund zu #029). + * + * Bei einer **veroeffentlichten** Version aendert das nichts: dort sind + * die Schritte beim Veroeffentlichen schon in den Abschnitt geschrieben + * worden, und der Titel kommt ohnehin aus dem Baustein. + */ + const quelle = abschnitt.bausteinVersion; + const ausBaustein = quelle !== null && abschnitt.schritte.length === 0; + + return { + titel: ausBaustein ? quelle.titel : abschnitt.titel, + // Wandert mit: ein Abteilungsname bedeutet in einer anderen Installation + // dasselbe. + nurFuerAbteilungen: [...abschnitt.nurFuerAbteilungen], + schritte: (ausBaustein ? quelle.schritte : abschnitt.schritte).map(schrittAbbilden), + }; + }), }, }; } @@ -90,7 +129,15 @@ export async function vorlageExportieren(versionId: string): Promise * nachsichtige Schema soll. Exportiert wird deshalb der rohe Inhalt; nur was gar * kein Objekt ist, gilt als leer. */ -function schrittAbbilden(schritt: VorlagenSchritt): AustauschSchritt { +function schrittAbbilden( + /* + * Nur die gelesenen Felder statt `VorlagenSchritt`: dieselbe Funktion bildet + * jetzt auch einen `BausteinSchritt` ab (Baustein-Abschnitte werden beim + * Export aufgeloest). Die beiden Modelle unterscheiden sich nur darin, woran + * ein Schritt haengt — und das steht ohnehin in keinem Export. + */ + schritt: Pick, +): AustauschSchritt { const roh = schritt.konfig; const konfig = typeof roh === "object" && roh !== null && !Array.isArray(roh) ? (roh as AustauschSchritt["konfig"]) : {}; diff --git a/tests/abteilungsabschnitte.test.ts b/tests/abteilungsabschnitte.test.ts index df7cfad..74900d0 100644 --- a/tests/abteilungsabschnitte.test.ts +++ b/tests/abteilungsabschnitte.test.ts @@ -170,6 +170,28 @@ describe.runIf(datenbankVerfuegbar)("Abteilungsabschnitte am Lauf", () => { expect(ergebnisse.map((e) => e.vorlagenSchritt.titel).sort()).toEqual(["Papiere", "Übergabe"]); }); + it("schreibt im Protokoll die Schritte dieses Laufs, nicht die der Version", async () => { + /* + * **Die Zahl war die der Version** (Reviewbefund zu #029). Bei einer Vorlage + * mit abteilungsgebundenen Abschnitten behauptete der Eintrag „Lauf angelegt“ + * mehr Schritte, als der Lauf hat. + * + * Das Protokoll ist seit #023 ansehbar: eine Zahl darin, die nicht zur Liste + * darunter passt, schickt jemanden auf die Suche nach Schritten, die es nie + * gab. + */ + const { admin, vertrieb, versionId } = await aufbau(VORLAGE); + const lauf = await laufAnlegen({ vorlagenVersionId: versionId, abteilungId: vertrieb.id }, admin); + + const wirklich = await testPrisma.schrittergebnis.count({ where: { laufId: lauf.id } }); + const eintrag = await testPrisma.protokoll.findFirstOrThrow({ + where: { laufId: lauf.id, aktion: "Lauf angelegt" }, + }); + + expect(wirklich).toBe(2); + expect(eintrag.nachher).toMatchObject({ anzahlSchritte: wirklich }); + }); + it("ändert die Abschnitte nicht, wenn später die Abteilung des Laufs wechselt", async () => { /* * Der Grund für das Festschreiben: ein archivierter Lauf muss genau die diff --git a/tests/anhang-route.test.ts b/tests/anhang-route.test.ts new file mode 100644 index 0000000..993c506 --- /dev/null +++ b/tests/anhang-route.test.ts @@ -0,0 +1,276 @@ +import { mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; +import { Betriebsmodus, VersionStatus } from "@/generated/prisma/enums"; +import type { Rolle } from "@/generated/prisma/enums"; +import { RechteFehler } from "@/server/auth/sitzungspflicht"; +import { anhangSpeichern } from "@/server/services/anhaenge"; +import { datenbankLeeren, datenbankVerfuegbar, testPrisma } from "./helpers/db"; + +/** + * Die Adresse, die einen Anhang ausliefert (#032). + * + * **Warum eine eigene Datei, obwohl `tests/anhaenge.test.ts` schon den Dienst + * prüft:** Das Akzeptanzkriterium von #032 lautet „ein Anhang eines fremden + * Kunden ist **über seine Adresse** nicht abrufbar“. Geprüft war bis hierher nur + * `anhangLesen` — der Dienst. Die ganze Rechteprüfung im Route Handler ließ sich + * durch einen hartcodierten ADMIN ersetzen, ohne dass ein einziger Test rot + * wurde (Reviewbefund zu #029). + * + * Das ist die teuerste Lücke, die eine Prüfung haben kann: dieser Handler ist + * die **einzige** Stelle der Anwendung, die rohe Dateibytes ausliefert. Fällt + * die Prüfung bei einem Umbau heraus, gibt jedes Foto jedem heraus, der eine + * Kennung hat — und das Tor vor dem Commit bleibt grün. + * + * Gefahren wird deshalb der **Handler selbst**, wie bei den SCIM-Handlern und + * beim Ereignisstrom. Nur die Sitzung wird gestellt; alles andere ist echt, bis + * hinunter zur Datei auf der Platte. + */ + +/* Der Handler wird echt gefahren; nur die Sitzung wird gestellt. */ +vi.mock("@/server/auth/sitzungspflicht", async (original) => { + const echt = await original(); + return { ...echt, rechtErforderlich: vi.fn() }; +}); + +const { rechtErforderlich } = await import("@/server/auth/sitzungspflicht"); +const { GET } = await import("@/app/api/anhaenge/[id]/route"); + +const VORHER = process.env["ANHAENGE_PFAD"]; +let ordner: string; + +beforeAll(async () => { + ordner = await mkdtemp(join(tmpdir(), "laufzettel-anhang-route-")); + process.env["ANHAENGE_PFAD"] = ordner; +}); + +afterAll(async () => { + if (VORHER === undefined) delete process.env["ANHAENGE_PFAD"]; + else process.env["ANHAENGE_PFAD"] = VORHER; + await rm(ordner, { recursive: true, force: true }); +}); + +/** Ein winziges, gültiges PNG. */ +const PNG = Buffer.from( + "89504e470d0a1a0a0000000d49484452000000010000000108060000001f15c489" + + "0000000a49444154789c6300010000050001" + + "0d0a2db40000000049454e44ae426082", + "hex", +); + +/** Ein winziges PDF – es wird anders ausgeliefert als ein Bild. */ +const PDF = Buffer.from("%PDF-1.4\n1 0 obj\n<<>>\nendobj\ntrailer\n<<>>\n%%EOF\n", "utf8"); + +async function aufbau() { + const kunde = await testPrisma.kunde.create({ + data: { name: "Eigene Organisation", kuerzel: "EIGEN", istEigeneOrganisation: true }, + }); + await testPrisma.einstellung.create({ data: { id: "global", modus: Betriebsmodus.INTERN } }); + const admin = await testPrisma.benutzer.create({ + data: { email: "admin@firma.example", name: "Admin", rolle: "ADMIN" }, + }); + + const vorlage = await testPrisma.vorlage.create({ data: { name: "PC Neuinstallation" } }); + const version = await testPrisma.vorlagenVersion.create({ + data: { vorlageId: vorlage.id, version: 1, status: VersionStatus.ENTWURF }, + }); + const abschnitt = await testPrisma.vorlagenAbschnitt.create({ + data: { versionId: version.id, titel: "Vorbereitung", position: 0 }, + }); + const schritt = await testPrisma.vorlagenSchritt.create({ + data: { abschnittId: abschnitt.id, titel: "Typenschild fotografieren", position: 0 }, + }); + await testPrisma.vorlagenVersion.update({ + where: { id: version.id }, + data: { status: VersionStatus.VEROEFFENTLICHT, veroeffentlichtAm: new Date() }, + }); + + const lauf = await testPrisma.lauf.create({ + data: { vorlagenVersionId: version.id, titel: "Ein Lauf", kundeId: kunde.id }, + }); + await testPrisma.schrittergebnis.create({ + data: { laufId: lauf.id, vorlagenSchrittId: schritt.id }, + }); + + return { + kunde, + admin: { id: admin.id, rolle: admin.rolle }, + laufId: lauf.id, + schrittId: schritt.id, + }; +} + +/** Legt einen Anhang an und gibt seine Kennung zurück. */ +async function anhangAnlegen( + laufId: string, + schrittId: string, + admin: { id: string; rolle: Rolle }, + abweichend: { dateiname?: string; medientyp?: string; inhalt?: Buffer } = {}, +): Promise { + const { id } = await anhangSpeichern( + { + laufId, + vorlagenSchrittId: schrittId, + dateiname: abweichend.dateiname ?? "typenschild.png", + medientyp: abweichend.medientyp ?? "image/png", + inhalt: new Uint8Array(abweichend.inhalt ?? PNG), + }, + admin, + ); + return id; +} + +const kennung = (id: string) => ({ params: Promise.resolve({ id }) }); +const anfrage = () => new Request("http://127.0.0.1:3300/api/anhaenge/egal"); + +describe.runIf(datenbankVerfuegbar)("Ein Anhang über seine Adresse", () => { + beforeEach(async () => { + await datenbankLeeren(); + vi.mocked(rechtErforderlich).mockReset(); + }); + + it("weist ohne Anmeldung ab, bevor eine Datei gelesen wird", async () => { + const { admin, laufId, schrittId } = await aufbau(); + const id = await anhangAnlegen(laufId, schrittId, admin); + + vi.mocked(rechtErforderlich).mockRejectedValue(new RechteFehler("Nicht angemeldet.")); + const antwort = await GET(anfrage(), kennung(id)); + + expect(antwort.status).toBe(401); + // Und kein Byte der Datei im Rumpf. + expect(await antwort.text()).not.toContain("PNG"); + }); + + it("weist auch ab, wenn die Rolle das Recht lesen nicht hat", async () => { + /* + * `MITARBEITER` hat seit #035 kein `lesen` — die Selbstauskunft ist dessen + * **Ersatz** und kein Zusatz. Dass ein Anhang damit nicht erreichbar ist, + * entscheidet `rechtErforderlich`; hier wird gezeigt, dass der Handler sich + * daran hält und nicht selbst weiterläuft. + */ + const { admin, laufId, schrittId } = await aufbau(); + const id = await anhangAnlegen(laufId, schrittId, admin); + + vi.mocked(rechtErforderlich).mockRejectedValue(new RechteFehler("Dafür fehlt dir die Berechtigung.")); + expect((await GET(anfrage(), kennung(id))).status).toBe(401); + }); + + it("liefert einem berechtigten Benutzer die Datei unverändert", async () => { + const { admin, laufId, schrittId } = await aufbau(); + const id = await anhangAnlegen(laufId, schrittId, admin); + + vi.mocked(rechtErforderlich).mockResolvedValue({ benutzer: admin } as never); + const antwort = await GET(anfrage(), kennung(id)); + + expect(antwort.status).toBe(200); + expect(antwort.headers.get("content-type")).toBe("image/png"); + expect(antwort.headers.get("content-length")).toBe(String(PNG.length)); + // Byte für Byte dasselbe – ein Bild, das unterwegs umkodiert wird, ist kein Beweismittel. + expect(Buffer.from(await antwort.arrayBuffer()).equals(PNG)).toBe(true); + }); + + it("zeigt ein Bild inline und lädt ein PDF herunter", async () => { + /* + * Ein PDF kann Skripte enthalten. Ein Anhang, den jemand aus einem fremden + * Lauf öffnet, soll nichts ausführen — deshalb `attachment` und + * `nosniff`. + */ + const { admin, laufId, schrittId } = await aufbau(); + vi.mocked(rechtErforderlich).mockResolvedValue({ benutzer: admin } as never); + + const bild = await anhangAnlegen(laufId, schrittId, admin); + const bildAntwort = await GET(anfrage(), kennung(bild)); + expect(bildAntwort.headers.get("content-disposition")).toBe('inline; filename="typenschild.png"'); + + const pdf = await anhangAnlegen(laufId, schrittId, admin, { + dateiname: "abnahme.pdf", + medientyp: "application/pdf", + inhalt: PDF, + }); + const pdfAntwort = await GET(anfrage(), kennung(pdf)); + expect(pdfAntwort.headers.get("content-disposition")).toBe('attachment; filename="abnahme.pdf"'); + expect(pdfAntwort.headers.get("x-content-type-options")).toBe("nosniff"); + }); + + it("lässt keinen Zwischenspeicher die Datei weitergeben", async () => { + // Ein Anhang gehört einem Kunden. Ein öffentlicher Cache gäbe ihn an den + // nächsten Betrachter weiter, und hinter einem Reverse Proxy ist das real. + const { admin, laufId, schrittId } = await aufbau(); + const id = await anhangAnlegen(laufId, schrittId, admin); + + vi.mocked(rechtErforderlich).mockResolvedValue({ benutzer: admin } as never); + expect((await GET(anfrage(), kennung(id))).headers.get("cache-control")).toContain("private"); + }); + + it("gibt den Anhang eines fremden Kunden nicht heraus – über seine Adresse", async () => { + /* + * **Das Akzeptanzkriterium von #032, zum ersten Mal an der Adresse + * geprüft.** Der Techniker ist auf einen anderen Kunden beschränkt; die + * Kennung des Anhangs kennt er. Ohne diese Prüfung reicht die Kennung. + */ + const vorher = process.env["OIDC_CUSTOMER_GROUP_PREFIX"]; + process.env["OIDC_CUSTOMER_GROUP_PREFIX"] = "kunde-"; + try { + const { admin, laufId, schrittId } = await aufbau(); + await testPrisma.einstellung.update({ + where: { id: "global" }, + data: { modus: Betriebsmodus.DIENSTLEISTUNG }, + }); + const id = await anhangAnlegen(laufId, schrittId, admin); + + const fremd = await testPrisma.kunde.create({ data: { name: "Fremd GmbH", kuerzel: "FRD" } }); + const techniker = await testPrisma.benutzer.create({ + data: { email: "tina@firma.example", name: "Tina", rolle: "TECHNIKER" }, + }); + await testPrisma.benutzerKunde.create({ + data: { benutzerId: techniker.id, kundeId: fremd.id }, + }); + + vi.mocked(rechtErforderlich).mockResolvedValue({ + benutzer: { id: techniker.id, rolle: techniker.rolle }, + } as never); + const antwort = await GET(anfrage(), kennung(id)); + + /* + * `404` und nicht `403`: nicht vorhanden und nicht sichtbar sind dieselbe + * Antwort. Die Unterscheidung wäre schon eine Auskunft über fremde Kunden. + */ + expect(antwort.status).toBe(404); + expect(antwort.headers.get("content-type")).toContain("application/json"); + const daten = (await antwort.json()) as Record; + // Und die Meldung nennt weder Dateiname noch Kunde. + expect(JSON.stringify(daten)).not.toContain("typenschild"); + expect(JSON.stringify(daten)).not.toContain("Eigene Organisation"); + } finally { + if (vorher === undefined) delete process.env["OIDC_CUSTOMER_GROUP_PREFIX"]; + else process.env["OIDC_CUSTOMER_GROUP_PREFIX"] = vorher; + } + }); + + it("antwortet auf eine Kennung, die es nicht gibt, mit 404", async () => { + const { admin } = await aufbau(); + vi.mocked(rechtErforderlich).mockResolvedValue({ benutzer: admin } as never); + + const antwort = await GET(anfrage(), kennung("gibt-es-nicht")); + expect(antwort.status).toBe(404); + }); + + it("sagt bei fehlender Datei, dass die Zeile steht und die Datei fehlt", async () => { + /* + * Der Fall nach einer Sicherung, die nur die Datenbank mitgenommen hat. Die + * Meldung muss das sagen und nicht „nicht gefunden“ behaupten — sonst sucht + * der Betreiber an der falschen Stelle (#032, und siehe + * `docs/betrieb/sicherung.md`). + */ + const { admin, laufId, schrittId } = await aufbau(); + const id = await anhangAnlegen(laufId, schrittId, admin); + await rm(ordner, { recursive: true, force: true }); + + vi.mocked(rechtErforderlich).mockResolvedValue({ benutzer: admin } as never); + const antwort = await GET(anfrage(), kennung(id)); + + expect(antwort.status).toBe(404); + expect(String((await antwort.json())["fehler"])).toMatch(/fehlt im Dateisystem/u); + }); +}); diff --git a/tests/bausteine.test.ts b/tests/bausteine.test.ts index 7672e3c..8101514 100644 --- a/tests/bausteine.test.ts +++ b/tests/bausteine.test.ts @@ -506,6 +506,60 @@ describe.runIf(datenbankVerfuegbar)("Bausteine", () => { ]); }); + it("exportiert auch einen Entwurf mit seinen Baustein-Schritten", async () => { + /* + * **Der Fall, der still einen leeren Abschnitt lieferte** (Reviewbefund zu + * #029). Ein Baustein-Abschnitt im Entwurf hat keine eigenen Schritte — + * die entstehen erst beim Veröffentlichen. Der Export schrieb deshalb + * `{"titel":"wird ersetzt","schritte":[]}`, und der Import nahm das + * stillschweigend an: das Schema lässt einen leeren Abschnitt zu. + * + * Ein Redakteur bringt eine noch unveröffentlichte Vorlage in eine zweite + * Installation — genau der Zweck des Formats. Dort kam der geteilte + * Abschnitt unter dem Editor-Platzhalter „wird ersetzt“ und ohne einen + * einzigen Schritt an. Die Checkliste war unvollständig, und nichts sagte + * es. + */ + const { adminId } = await grundbestand(); + const { versionId: bausteinVersionId } = await bausteinMitSchritten( + "Datensicherung prüfen", + "Datensicherung", + ["Sicherung läuft", "Rücksicherung geprüft"], + adminId, + ); + + // Wie `vorlageMitBaustein`, aber **ohne** zu veröffentlichen. + const { vorlage } = await vorlageAnlegen({ name: "Noch Entwurf" }, adminId); + const entwurf = await entwurfHolenOderErzeugen(vorlage.id, adminId); + await entwurfSpeichern(entwurf.id, { + variablen: [], + abschnitte: [ + { titel: "Eigener Abschnitt", schritte: [{ titel: "Eigener Schritt" }] }, + { titel: "wird ersetzt", bausteinVersionId, schritte: [] }, + ], + }); + + const austausch = await vorlageExportieren(entwurf.id); + + expect( + austausch.struktur.abschnitte.map((abschnitt) => ({ + titel: abschnitt.titel, + schritte: abschnitt.schritte.map((schritt) => schritt.titel), + })), + ).toEqual([ + { titel: "Eigener Abschnitt", schritte: ["Eigener Schritt"] }, + // Der Titel aus dem Baustein, nicht der Platzhalter – und die Schritte. + { titel: "Datensicherung", schritte: ["Sicherung läuft", "Rücksicherung geprüft"] }, + ]); + // Und weiterhin kein Verweis: in der anderen Installation gibt es den + // Baustein vielleicht nicht. + expect(JSON.stringify(austausch)).not.toContain(bausteinVersionId); + // Kein Abschnitt ohne Schritte – das ist die Zusage, die vorher brach. + for (const abschnitt of austausch.struktur.abschnitte) { + expect(abschnitt.schritte.length, abschnitt.titel).toBeGreaterThan(0); + } + }); + it("übernimmt beim Import keinen mitgeschickten Verweis", async () => { const { adminId } = await grundbestand(); const { versionId: bausteinVersionId } = await bausteinMitSchritten( diff --git a/tests/benachrichtigungen.test.ts b/tests/benachrichtigungen.test.ts index 5a64de3..f222579 100644 --- a/tests/benachrichtigungen.test.ts +++ b/tests/benachrichtigungen.test.ts @@ -1,4 +1,6 @@ import { createServer, type Server } from "node:http"; +import { existsSync } from "node:fs"; +import { join } from "node:path"; import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it } from "vitest"; import { Betriebsmodus, @@ -12,6 +14,7 @@ import { abgleichMeldung, laufMeldung, webhookRumpf } from "@/lib/meldungen"; import { SIGNATUR_KOPF, signaturBauen, signaturPruefen } from "@/lib/signatur"; import { verschluesseln } from "@/lib/geheimnisse"; import { + MELDUNGS_PFADE, WARTEZEITEN_MINUTEN, abgleichMeldungEinreihen, laufMeldungEinreihen, @@ -199,6 +202,31 @@ describe("Die Texte der Meldungen", () => { expect(abgleichMeldung("AD", "Grund", link).rumpf).toContain(link); }); + it("zeigt mit jedem Pfad auf eine Seite, die es wirklich gibt", () => { + /* + * **Der Test, der gefehlt hat.** Oben wird geprüft, dass der *übergebene* + * Link im Rumpf steht — das ist richtig und sagt nichts darüber, ob der + * Pfad existiert. `/verwaltung/verzeichnis` stand im Dienst und war ein + * 404: `verwaltung` ist der Name des Rechts, die Seite liegt unter + * `/admin/verzeichnis` (Reviewbefund zu #029). + * + * Ein Pfad in einer Mail wird nicht nachgesehen, sondern geklickt — und + * genau diese eine Meldung geht an **alle** Administratoren, im Moment + * eines Ausfalls. Geprüft wird deshalb gegen das Dateisystem: zu jedem Pfad + * muss unter `src/app/(app)/` eine `page.tsx` liegen. + */ + const wurzel = new URL("../src/app/(app)", import.meta.url).pathname; + for (const pfad of Object.values(MELDUNGS_PFADE)) { + const teile = pfad.replace(/^\//u, "").split("/"); + // Ein Pfad, der auf eine Kennung endet, liegt unter `[id]`. + const kandidaten = [join(wurzel, ...teile, "page.tsx"), join(wurzel, ...teile, "[id]", "page.tsx")]; + expect( + kandidaten.some((datei) => existsSync(datei)), + `${pfad} zeigt auf keine Seite – gesucht: ${kandidaten.join(" oder ")}`, + ).toBe(true); + } + }); + it("nimmt keinen Schrittinhalt auf — der Typ lässt es nicht zu", () => { /* * Die wichtigste Zusage dieses Meilensteins. Geprueft wird sie hier ueber diff --git a/tests/betriebsstand.test.ts b/tests/betriebsstand.test.ts index 38a7428..6235c46 100644 --- a/tests/betriebsstand.test.ts +++ b/tests/betriebsstand.test.ts @@ -1,5 +1,6 @@ -import { readFileSync } from "node:fs"; +import { readFileSync, readdirSync } from "node:fs"; import { describe, expect, it } from "vitest"; +import { E2E_PRAEFIX } from "./e2e/aufraeumen"; import { parse } from "yaml"; /** @@ -83,6 +84,62 @@ describe("Betriebsstand: docker-compose.yml", () => { }); }); +describe("Die Ende-zu-Ende-Abläufe räumen auf", () => { + /* + * **Der Test, der die Lücke beim nächsten Meilenstein fängt** (#027). + * + * In der Entwicklungsdatenbank standen 105 Vorlagen mit Namen wie + * `ZZ E2E Editor 1790324192596 C`. Der Editor-Ablauf räumt seit #018 selbst + * auf — aber zwei Abläufe aus Phase 2 (`anhaenge`, `live`) legten Zeilen an + * und hatten keinen Nachlauf. Das fiel nicht auf, weil die Suite davon nichts + * sieht: Reste sind kein Testfehler, sie sind ein Schaden an allem, was + * danach in dieselbe Datenbank sieht. + * + * Geräumt wird jetzt zentral in `tests/e2e/aufraeumen.ts`, vor **und** nach + * dem Lauf. Damit das trägt, muss zweierlei gelten, und beides steht hier: + * jeder Ablauf benutzt das Präfix, und der Vor- und Nachlauf sind registriert. + */ + + const ABLAEUFE = readdirSync("tests/e2e").filter((datei) => datei.endsWith(".spec.ts")); + + it("findet überhaupt Abläufe – sonst prüft der Test darunter nichts", () => { + // Ohne diese Zeile wäre eine leere Liste ein grüner Test. + expect(ABLAEUFE.length).toBeGreaterThan(10); + }); + + it("benennt jede angelegte Zeile mit dem Präfix, an dem aufgeräumt wird", () => { + /* + * Geprüft wird die **Umkehrung**: ein Ablauf, der Daten anlegt, ohne das + * Präfix zu benutzen, ist von keinem Aufräumen erfasst. Erkennbar ist das + * daran, dass er ein Formularfeld „Name“ oder „Titel“ füllt. + */ + const ohnePraefix: string[] = []; + for (const datei of ABLAEUFE) { + const inhalt = readFileSync(`tests/e2e/${datei}`, "utf8"); + const legtAn = /getByLabel\((?:"|')(?:Name|Titel|Bezeichnung)/u.test(inhalt); + if (legtAn && !inhalt.includes(E2E_PRAEFIX)) ohnePraefix.push(datei); + } + expect( + ohnePraefix, + `Diese Abläufe legen Zeilen an, ohne „${E2E_PRAEFIX}“ zu benutzen — das zentrale ` + + "Aufräumen findet sie nicht wieder.", + ).toEqual([]); + }); + + it("hat Vorlauf und Nachlauf registriert", () => { + /* + * Beides, und das ist der Punkt: der Nachlauf läuft nach einem Testfehler + * und nach einem Zeitablauf, aber nicht nach `Strg+C`. Dann findet der + * Vorlauf des nächsten Durchgangs die Reste — und ein Vorlauf lässt sich + * nicht überspringen. + */ + const konfiguration = readFileSync("playwright.config.ts", "utf8"); + expect(konfiguration).toContain("global-setup"); + expect(konfiguration).toContain("global-teardown"); + expect(readFileSync("tests/e2e/global-setup.ts", "utf8")).toContain("e2eAufraeumenMitBericht"); + }); +}); + describe("Betriebsstand: Entwicklungsdatenbank", () => { it("bindet nur auf localhost und auf einen eigenen, umstellbaren Port", () => { const stand = compose("docker-compose.dev.yml") as unknown as { diff --git a/tests/e2e/aufraeumen.ts b/tests/e2e/aufraeumen.ts new file mode 100644 index 0000000..34a74de --- /dev/null +++ b/tests/e2e/aufraeumen.ts @@ -0,0 +1,185 @@ +import { prisma } from "@/lib/db"; + +/** + * Was die Ende-zu-Ende-Abläufe hinterlassen, wieder wegräumen (#027). + * + * **Alles, was ein Ablauf anlegt, beginnt mit `ZZ`.** Das Präfix sortiert + * die Zeilen in jeder Liste ans Ende und ist zugleich der Griff, an dem sie + * wieder herausgehen — ohne gemerkte Kennungen, denn ein Lauf, der mitten im + * Ablauf abbricht, merkt sich nichts. + * + * **Warum vor *und* nach dem Lauf geräumt wird.** Ein `afterAll` je Datei und + * ein `globalTeardown` laufen bei einem Testfehler und bei einem Zeitablauf — + * aber nicht, wenn jemand `Strg+C` drückt oder der Prozess hart stirbt. Dann + * bleiben die Zeilen stehen, und der nächste Lauf findet sie vor. Deshalb räumt + * derselbe Code auch im Vorlauf: was vom letzten Mal übrig ist, geht weg, bevor + * der erste Ablauf startet. Ein Vorlauf kann nicht übersprungen werden. + * + * Aufgefallen ist das Ganze, weil in der Entwicklungsdatenbank 105 Vorlagen mit + * Namen wie `ZZ E2E Editor 1790324192596 C` standen — aus einem Fenster von + * sechzehn Stunden, in dem noch kein Ablauf aufräumte. Der Schaden liegt nicht + * bei den Tests, sondern bei allem, was **danach** in dieselbe Datenbank sieht: + * die Vorlagenliste, die Screenshots, jede Abfrage von Hand. Ein Fehler, der + * „zu viele Treffer“ heißt, kostet dann eine Stunde. + */ + +/** + * Das Präfix, das jeder Ablauf seinen Zeilen gibt. + * + * **`ZZ` und nicht `ZZ E2E`**, weil die Abläufe es in fünf Schreibweisen + * benutzen: `ZZ E2E Editor …`, `ZZ-E2E--PC-01` (Gerätenamen dürfen keine + * Leerzeichen haben), `ZZPruef…`, `ZZQuelle…` und `ZZ Abteilung …`. Alle fünf + * sind sinnvoll — ein Hostname sieht anders aus als ein Vorlagenname —, und + * alle fünf beginnen mit `ZZ`. Darauf zu greifen deckt sie ab, ohne sechzehn + * Dateien umzubenennen. + * + * Zwei Buchstaben sind kurz. Das ist hier in Ordnung: geräumt wird nur beim + * Ende-zu-Ende-Lauf und nur gegen die Entwicklungsdatenbank, und `ZZ` am + * **Anfang** eines Namens ist in echten Daten dieses Hauses nichts, was jemand + * tippt. Der Grund für die Wahl ist ohnehin derselbe: `ZZ` sortiert in jeder + * Liste ans Ende. + */ +export const E2E_PRAEFIX = "ZZ"; + +/** Was aufgeräumt wurde — damit der Aufrufer es melden kann. */ +export type Aufgeraeumt = Readonly>; + +/** + * Räumt alles mit `ZZ` weg und meldet, was es war. + * + * Die Reihenfolge ist nicht beliebig: erst was an einem Lauf hängt, dann die + * Läufe, dann woran ein Lauf hängen könnte. Ein Fremdschlüssel, der sich + * beschwert, ist dabei **kein** Fehler dieser Funktion, sondern die Auskunft, + * dass ein Ablauf etwas verknüpft hat, das er nicht aufräumen kann — dann soll + * es auffallen und nicht stillschweigend bleiben. + */ +export async function e2eRestlosAufraeumen(): Promise { + const praefix = { startsWith: E2E_PRAEFIX }; + const gezaehlt: Record = {}; + const merken = (was: string, anzahl: number): void => { + if (anzahl > 0) gezaehlt[was] = (gezaehlt[was] ?? 0) + anzahl; + }; + + /* + * `Protokoll.entitaetId` ist **kein** Fremdschlüssel: die Zeilen überleben + * ihren Gegenstand absichtlich (ein archivierter Lauf soll seine Geschichte + * behalten). Hier heißt das, dass sie als Waisen stehen bleiben, wenn sie + * nicht mitgenommen werden — darum werden die Kennungen vorher gesammelt. + */ + const laeufe = await prisma.lauf.findMany({ where: { titel: praefix }, select: { id: true } }); + const vorlagen = await prisma.vorlage.findMany({ + where: { name: praefix }, + select: { id: true, versionen: { select: { id: true } } }, + }); + const bausteine = await prisma.baustein.findMany({ + where: { name: praefix }, + select: { id: true, versionen: { select: { id: true } } }, + }); + const geraete = await prisma.geraet.findMany({ + where: { OR: [{ bezeichnung: praefix }, { hostname: praefix }] }, + select: { id: true }, + }); + const personen = await prisma.mitarbeiter.findMany({ + where: { anzeigename: praefix }, + select: { id: true }, + }); + const abteilungen = await prisma.abteilung.findMany({ + where: { name: praefix }, + select: { id: true }, + }); + const quellen = await prisma.verzeichnisquelle.findMany({ + where: { name: praefix }, + select: { id: true }, + }); + const benutzer = await prisma.benutzer.findMany({ + where: { name: praefix }, + select: { id: true }, + }); + + /* + * **Waisen werden nicht erraten.** Eine Protokollzeile, deren Gegenstand schon + * fort ist, bleibt stehen — ihre `entitaetId` ist hier nicht mehr zu finden. + * Sie über den Namen im Json zu greifen wäre naheliegend und falsch: derselbe + * Name steht auch in echter Historie einer Zeile, die es weiterhin gibt (etwa + * „Version veröffentlicht“ mit der Liste der eingesetzten Bausteine). Dass + * eine solche Zeile ihren Gegenstand überlebt, ist genau der Grund, warum + * `entitaetId` kein Fremdschlüssel ist. + */ + const laufIds = laeufe.map((zeile) => zeile.id); + const betroffen = [ + ...laufIds, + ...vorlagen.flatMap((v) => [v.id, ...v.versionen.map((version) => version.id)]), + ...bausteine.flatMap((b) => [b.id, ...b.versionen.map((version) => version.id)]), + ...geraete.map((zeile) => zeile.id), + ...personen.map((zeile) => zeile.id), + ...abteilungen.map((zeile) => zeile.id), + ...quellen.map((zeile) => zeile.id), + ...benutzer.map((zeile) => zeile.id), + ]; + + if (laufIds.length > 0) { + merken("Anhänge", (await prisma.anhang.deleteMany({ where: { laufId: { in: laufIds } } })).count); + merken("Kommentare", (await prisma.kommentar.deleteMany({ where: { laufId: { in: laufIds } } })).count); + merken( + "Schrittergebnisse", + (await prisma.schrittergebnis.deleteMany({ where: { laufId: { in: laufIds } } })).count, + ); + merken( + "Zustellungen", + (await prisma.zustellung.deleteMany({ where: { laufId: { in: laufIds } } })).count, + ); + } + + if (betroffen.length > 0) { + merken( + "Protokollzeilen", + ( + await prisma.protokoll.deleteMany({ + where: { OR: [{ entitaetId: { in: betroffen } }, { laufId: { in: laufIds } }] }, + }) + ).count, + ); + } + + merken("Läufe", (await prisma.lauf.deleteMany({ where: { titel: praefix } })).count); + merken("Vorlagen", (await prisma.vorlage.deleteMany({ where: { name: praefix } })).count); + merken("Bausteine", (await prisma.baustein.deleteMany({ where: { name: praefix } })).count); + merken( + "Geräte", + ( + await prisma.geraet.deleteMany({ + where: { OR: [{ bezeichnung: praefix }, { hostname: praefix }] }, + }) + ).count, + ); + merken("Personen", (await prisma.mitarbeiter.deleteMany({ where: { anzeigename: praefix } })).count); + merken("Abteilungen", (await prisma.abteilung.deleteMany({ where: { name: praefix } })).count); + merken( + "Verzeichnisquellen", + (await prisma.verzeichnisquelle.deleteMany({ where: { name: praefix } })).count, + ); + merken("Benutzer", (await prisma.benutzer.deleteMany({ where: { name: praefix } })).count); + + return gezaehlt; +} + +/** Dasselbe mit einer Zeile Ausgabe — für Vor- und Nachlauf. */ +export async function e2eAufraeumenMitBericht(wann: "vor" | "nach"): Promise { + const gezaehlt = await e2eRestlosAufraeumen(); + const summe = Object.values(gezaehlt).reduce((a, b) => a + b, 0); + if (summe === 0) return; + + const liste = Object.entries(gezaehlt) + .map(([was, anzahl]) => `${anzahl} ${was}`) + .join(", "); + /* + * Der Vorlauf **sagt es laut**: Zeilen, die er findet, sind der Rückstand + * eines abgebrochenen Durchgangs. Still wegzuräumen hieße, die Ursache zu + * verdecken — und ein Ablauf, der jedes Mal abbricht, fiele nie auf. + */ + console.info( + wann === "vor" + ? `E2E-Aufräumen (Rückstand aus einem früheren Lauf): ${liste}.` + : `E2E-Aufräumen: ${liste}.`, + ); +} diff --git a/tests/e2e/global-setup.ts b/tests/e2e/global-setup.ts index 1c7eb39..abf308d 100644 --- a/tests/e2e/global-setup.ts +++ b/tests/e2e/global-setup.ts @@ -1,7 +1,9 @@ import { chromium } from "@playwright/test"; import "dotenv/config"; import { prisma } from "@/lib/db"; +import { ABTEILUNGEN } from "@/../prisma/seed-daten/stammdaten"; import { anmelden, SEED_GERAET, SEED_TAG, SEED_VORLAGE } from "./helfer"; +import { e2eAufraeumenMitBericht } from "./aufraeumen"; /** * Was vor dem ersten Ablauf stimmen muss (#018). @@ -23,6 +25,14 @@ function fehlen(was: string, abhilfe: string): never { } export default async function vorbereiten(): Promise { + /* + * **Zuerst aufraeumen, dann pruefen** (#027). Was hier noch steht, ist der + * Rueckstand eines abgebrochenen Durchgangs: `globalTeardown` und die + * `afterAll`-Bloecke laufen bei einem Testfehler, aber nicht bei `Strg+C`. + * Ein Vorlauf laesst sich nicht ueberspringen — das ist der Unterschied. + */ + await e2eAufraeumenMitBericht("vor"); + if (!process.env["SEED_ADMIN_EMAIL"] || !process.env["SEED_ADMIN_PASSWORD"]) { fehlen( "SEED_ADMIN_EMAIL oder SEED_ADMIN_PASSWORD fehlt.", @@ -59,6 +69,37 @@ export default async function vorbereiten(): Promise { fehlen(`Das Beispielgerät ${SEED_GERAET} (${SEED_TAG}) fehlt.`, "pnpm db:seed laufen lassen."); } + /* + * 2c. Stehen die **Werte** des Seed-Bestands noch, nicht nur die Zeilen? + * + * Der Onboarding-Ablauf prueft den `New-ADUser`-Befehl **zeichengenau**, und + * darin steckt die Ziel-OU der Abteilung. In der Entwicklungsdatenbank stand + * an „Buchhaltung“ `DC=firma,DC=de` statt `DC=firma,DC=local` — von Hand + * geaendert oder aus einer aelteren Seed-Fassung. Der Seed setzt bestehende + * Abteilungen absichtlich **nicht** zurueck (gepflegte Daten sollen bleiben), + * also blieb die Abweichung stehen. + * + * Der Ablauf scheiterte daran mit „expected … received …“ — einer Meldung, die + * nach einem Fehler im Befehlsbau aussieht und keiner war. Hier faellt es + * vorher auf, und die Meldung sagt, was zu tun ist (Reviewbefund zu #029). + */ + for (const vorgabe of ABTEILUNGEN) { + const abteilung = await prisma.abteilung.findFirst({ + where: { name: vorgabe.name }, + select: { zielOu: true, standardGruppen: true }, + }); + if (!abteilung) continue; + if (abteilung.zielOu !== vorgabe.zielOu) { + fehlen( + `Die Abteilung „${vorgabe.name}“ hat eine abweichende Ziel-OU: ` + + `„${abteilung.zielOu}“ statt „${vorgabe.zielOu}“.`, + "Der Onboarding-Ablauf prüft den erzeugten Befehl zeichengenau. Entweder die Ziel-OU " + + "unter Admin → Abteilungen zurückstellen, oder gegen eine frische Datenbank fahren: " + + "pnpm db:seed:reset.", + ); + } + } + /* * 2b. In welchem Betriebsmodus laeuft die Installation? * diff --git a/tests/e2e/global-teardown.ts b/tests/e2e/global-teardown.ts new file mode 100644 index 0000000..48ee667 --- /dev/null +++ b/tests/e2e/global-teardown.ts @@ -0,0 +1,18 @@ +import { prisma } from "@/lib/db"; +import { e2eAufraeumenMitBericht } from "./aufraeumen"; + +/** + * Nach dem letzten Ablauf: wegräumen, was die Abläufe angelegt haben (#027). + * + * Läuft auch dann, wenn Abläufe fehlgeschlagen sind — Playwright ruft den + * Nachlauf nach dem Lauf, nicht nach dem Erfolg. Was er **nicht** abdeckt, ist + * ein harter Abbruch (`Strg+C`, `kill`); dafür räumt derselbe Code im Vorlauf, + * siehe `aufraeumen.ts`. + */ +export default async function aufraeumen(): Promise { + try { + await e2eAufraeumenMitBericht("nach"); + } finally { + await prisma.$disconnect(); + } +} diff --git a/tests/e2e/laeufe.spec.ts b/tests/e2e/laeufe.spec.ts index 6e4e524..5ecf245 100644 --- a/tests/e2e/laeufe.spec.ts +++ b/tests/e2e/laeufe.spec.ts @@ -273,7 +273,19 @@ test.describe("Der Zustands-Endpunkt", () => { expect(antwort.headers()["cache-control"]).toBe("no-store"); const zustand: unknown = await antwort.json(); + /* + * Die Felder **namentlich** und vollständig: der Zustand ist die einzige + * Antwort, die alle zehn Sekunden (seit #033 bei jeder Änderung) hinausgeht, + * und ein Feld, das sich hier einschleicht, wandert ungesehen mit. Deshalb + * `toEqual` und nicht `toContain`. + * + * `anzahlAnhaenge` kam mit #033 dazu und fehlte hier — der Ablauf war seitdem + * rot, und niemand hat es gemerkt, weil `pnpm test:e2e` nicht im Tor vor dem + * Commit steht (Reviewbefund zu #029). Wer hier ein Feld ergänzt, trägt es + * ein; genau das ist der Zweck dieser Liste. + */ expect(Object.keys(zustand as object).sort()).toEqual([ + "anzahlAnhaenge", "anzahlKommentare", "anzahlPflichtOffen", "geaendertAm", diff --git a/tests/e2e/mobil.spec.ts b/tests/e2e/mobil.spec.ts index 828c926..f4e4cfb 100644 --- a/tests/e2e/mobil.spec.ts +++ b/tests/e2e/mobil.spec.ts @@ -184,6 +184,22 @@ const SEITEN: readonly Hauptseite[] = [ */ { name: "Admin: Verzeichnis", pfad: () => "/admin/verzeichnis", nurModus: "INTERN" }, { name: "Kunden", pfad: () => "/kunden", nurModus: "DIENSTLEISTUNG" }, + /* + * **Die Seiten aus Phase 2.** Sie fehlten hier, obwohl jedes der Issues eine + * Klickprobe bei 390 px von Hand berichtet — eine Probe von Hand ist kein + * Regressionstest (Reviewbefund zu #029). Gerade die Bausteinliste und das + * Zustellungs-Protokoll sind Tabellen, und Tabellen sind die Stellen, an denen + * horizontales Scrollen entsteht. + * + * Nicht dabei: `/meine-geraete` (#035) — es verlangt das Recht + * `selbstauskunft`, und das hat nur `MITARBEITER`. Dieser Ablauf meldet sich + * als Admin an, der dort „kein Zugriff“ sähe; gemessen würde die + * Verweigerungsseite. Dafür braucht es ein eigenes Konto, siehe #037. + */ + { name: "Vorlagen: Bausteine", pfad: () => "/vorlagen/bausteine" }, + { name: "Admin: Schnellstart", pfad: () => "/admin/schnellstart" }, + { name: "Admin: Protokoll", pfad: () => "/admin/protokoll" }, + { name: "Admin: Zustellungen", pfad: () => "/admin/zustellungen" }, ]; type Messung = { diff --git a/tests/e2e/stammdaten.spec.ts b/tests/e2e/stammdaten.spec.ts index 0143cdf..65fd486 100644 --- a/tests/e2e/stammdaten.spec.ts +++ b/tests/e2e/stammdaten.spec.ts @@ -112,7 +112,17 @@ test.describe("Personen", () => { await treffer.click(); await page.waitForURL(/\/mitarbeiter\/[^/]+$/); - await expect(page.getByText(/kommt aus dem Verzeichnis/)).toBeVisible(); + /* + * **Der ganze Satz, nicht nur der Anfang.** „kommt aus dem Verzeichnis“ + * allein traf seit #020 zwei Stellen: dieselbe Auskunft steht auch am + * Vorgesetzten-Feld. Playwright wies das als „strict mode violation“ ab, und + * der Ablauf war seitdem rot — unbemerkt, weil `pnpm test:e2e` nicht im Tor + * vor dem Commit steht (Reviewbefund zu #029). + * + * Hier zählt die Auskunft an den **Stammdaten**: dass sie dort gepflegt + * werden und eine Änderung hier der nächste Abgleich zurücksetzen würde. + */ + await expect(page.getByText(/kommt aus dem Verzeichnis\. Die Stammdaten werden dort/u)).toBeVisible(); await expect(page.getByLabel("Vorname")).toHaveAttribute("readonly", ""); await expect(page.getByRole("button", { name: "Stammdaten speichern" })).toHaveCount(0); // Deaktivieren muss trotzdem gehen: das Offboarding wartet nicht auf den diff --git a/tests/e2e/vorlagen-editor.spec.ts b/tests/e2e/vorlagen-editor.spec.ts index 374dba4..04f61c3 100644 --- a/tests/e2e/vorlagen-editor.spec.ts +++ b/tests/e2e/vorlagen-editor.spec.ts @@ -132,8 +132,15 @@ test.describe("Vorlagen-Editor", () => { .evaluateAll((felder) => felder.map((feld) => (feld as HTMLInputElement).value)); expect(await werte()).toEqual(["Erster", "Zweiter"]); - // Der Anfasser ist mindestens 44 px hoch – sonst trifft ihn niemand. + /* + * **Erst in den Blick scrollen.** Am Handy stand der erste Anfasser nach dem + * Anlegen zweier Abschnitte bei `y = -115`, also oberhalb des Sichtfensters; + * der Touch-Start landete damit außerhalb und bewegte nichts. Das fiel + * nicht auf, weil `pnpm test:e2e` nicht im Tor vor dem Commit steht + * (Reviewbefund zu #029). + */ const anfasser = page.getByRole("button", { name: "Verschieben" }).first(); + await anfasser.scrollIntoViewIfNeeded(); const kasten = await anfasser.boundingBox(); expect(kasten?.height ?? 0).toBeGreaterThanOrEqual(44); expect(kasten?.width ?? 0).toBeGreaterThanOrEqual(44); @@ -142,13 +149,32 @@ test.describe("Vorlagen-Editor", () => { // eine Wischgeste ein Bildlauf bleibt und kein Verschieben. const x = kasten!.x + kasten!.width / 2; const y = kasten!.y + kasten!.height / 2; + + /* + * **Die Zugweite wird gemessen, nicht gesetzt.** Vorher waren es fest + * 12 × 25 px = 300; der Abstand zwischen den beiden Anfassern ist aber + * 532 px, seit mit #030 und #031 Bausteinverweis und Abteilungsfilter in + * jeden Abschnitt gekommen sind. Der Finger kam damit nicht mehr über den + * zweiten Abschnitt, und die Reihenfolge blieb stehen. + * + * Eine feste Weite in einem Sortiertest ist eine Zusage über das Layout, + * die niemand pflegt. Gemessen wird sie bei jedem Lauf richtig — und wenn + * der Abstand sich wieder ändert, bleibt der Test grün, statt eine + * Pixelzahl nachzuziehen. + */ + const zweiterAnfasser = await page.getByRole("button", { name: "Verschieben" }).nth(1).boundingBox(); + // Ein Stück über die Mitte des zweiten hinaus, sonst entscheidet dnd-kit + // sich fuer „bleibt“. + const weite = (zweiterAnfasser?.y ?? y + 300) - kasten!.y + kasten!.height; + const schritte = 20; + const sitzung = await page.context().newCDPSession(page); await sitzung.send("Input.dispatchTouchEvent", { type: "touchStart", touchPoints: [{ x, y }] }); await page.waitForTimeout(350); - for (let schritt = 1; schritt <= 12; schritt += 1) { + for (let schritt = 1; schritt <= schritte; schritt += 1) { await sitzung.send("Input.dispatchTouchEvent", { type: "touchMove", - touchPoints: [{ x, y: y + schritt * 25 }], + touchPoints: [{ x, y: y + (weite * schritt) / schritte }], }); await page.waitForTimeout(40); } diff --git a/tests/graph.test.ts b/tests/graph.test.ts index f6594c9..7c9634d 100644 --- a/tests/graph.test.ts +++ b/tests/graph.test.ts @@ -8,6 +8,7 @@ import { benutzerZuordnen, ersteAdresse, graphEintraegeStroemen, + naechsteSeite, } from "@/server/graph/leser"; import { GraphFehler, graphZugangAus, tokenHolen, tokenSpeicherLeeren } from "@/server/graph/zugang"; import { vorgesetzteAufloesen } from "@/server/ldap/zuordnung"; @@ -385,6 +386,33 @@ describe("Der Token-Austausch", () => { }); describe("Der Seitenabruf", () => { + it("folgt einem nextLink nur auf denselben Host", () => { + /* + * **Jede Seite wird mit dem Zugangstoken geholt**, und das gilt mandantenweit + * (`User.Read.All`). Der Antwort von Graph über TLS zu trauen ist + * vertretbar — aber `GRAPH_API_URL` ist absichtlich einstellbar, für + * nationale Clouds und für den Doppelgänger in diesem Test. Ein Tippfehler + * darin oder ein aufbrechender Firmenproxy dazwischen schickte das Token + * sonst an eine fremde Adresse (Reviewbefund zu #029). + */ + const basis = "https://graph.microsoft.com"; + expect(naechsteSeite(`${basis}/v1.0/users?$skiptoken=abc`, basis)).toBe( + `${basis}/v1.0/users?$skiptoken=abc`, + ); + + // Anderer Host: Abruf endet, statt das Token mitzuschicken. + expect(naechsteSeite("https://boese.example/v1.0/users", basis)).toBeUndefined(); + // Auch ein anderes Schema oder ein anderer Port ist ein anderer Ursprung. + expect(naechsteSeite("http://graph.microsoft.com/v1.0/users", basis)).toBeUndefined(); + expect(naechsteSeite("https://graph.microsoft.com:8443/v1.0/users", basis)).toBeUndefined(); + + // Und das Übliche: nichts, leer, kein gültiges URL. + expect(naechsteSeite(undefined, basis)).toBeUndefined(); + expect(naechsteSeite("", basis)).toBeUndefined(); + expect(naechsteSeite("kein-url", basis)).toBeUndefined(); + expect(naechsteSeite(42, basis)).toBeUndefined(); + }); + it("folgt @odata.nextLink über drei Seiten", async () => { aufzeichnung.seiten = [[nutzer(1), nutzer(2)], [nutzer(3), nutzer(4)], [nutzer(5)]]; diff --git a/tests/lauf-ereignisse.test.ts b/tests/lauf-ereignisse.test.ts index 19e7ee5..e9bb9a4 100644 --- a/tests/lauf-ereignisse.test.ts +++ b/tests/lauf-ereignisse.test.ts @@ -1,5 +1,6 @@ import { afterAll, afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { Betriebsmodus, LaufStatus, VersionStatus } from "@/generated/prisma/enums"; +import type { Rolle } from "@/generated/prisma/enums"; import { HERZSCHLAG, KANAL, herzschlagMs, maxStroeme, sseEreignis, willStrom } from "@/lib/sse"; import { STROM_GESCHLOSSEN, @@ -7,6 +8,7 @@ import { sollZurueckfallen, } from "@/components/laeufe/lauf-aktualisierung"; import { RechteFehler } from "@/server/auth/sitzungspflicht"; +import type { Sitzung } from "@/lib/sitzung"; import { aufLaufHoeren, ereignisseSchliessen, stroemeOffen } from "@/server/lauf-ereignisse"; import { datenbankLeeren, datenbankVerfuegbar, testPrisma } from "./helpers/db"; @@ -305,10 +307,29 @@ describe.runIf(datenbankVerfuegbar)("Der Route Handler", () => { delete process.env["SSE_MAX_STROEME"]; }); - function angemeldetAls(benutzer: { id: string; rolle: "ADMIN" }): void { + /** + * Die Sitzung wird mit ihrer **echten Form** gestellt, nicht als `{}`. + * + * Vorher stand hier ein leeres Objekt, und genau daran ist ein Befund + * vorbeigekommen: der Strom prueft die Sitzung seit #029 beim Herzschlag nach, + * und mit einer Attrappe ohne `benutzerId` liesse sich das nicht zeigen. Eine + * Attrappe, die nicht die Form der Wirklichkeit hat, prueft etwas anderes als + * die Wirklichkeit — dieselbe Lehre wie beim Graph-Doppelgaenger in #028. + */ + function sitzungFuer(benutzerId: string, ausgestelltAm = Math.floor(Date.now() / 1000)): Sitzung { + return { + benutzerId, + email: "wer@firma.example", + name: "Wer", + herkunft: "passwort", + ausgestelltAm, + }; + } + + function angemeldetAls(benutzer: { id: string; rolle: Rolle }): void { vi.mocked(rechtErforderlich).mockResolvedValue({ benutzer: { id: benutzer.id, rolle: benutzer.rolle }, - sitzung: {}, + sitzung: sitzungFuer(benutzer.id), } as unknown as Awaited>); } @@ -365,7 +386,7 @@ describe.runIf(datenbankVerfuegbar)("Der Route Handler", () => { }); vi.mocked(rechtErforderlich).mockResolvedValue({ benutzer: { id: leser.id, rolle: "LESER" }, - sitzung: {}, + sitzung: sitzungFuer(leser.id), } as unknown as Awaited>); const antwort = await GET(anfrage("text/event-stream"), { @@ -458,6 +479,78 @@ describe.runIf(datenbankVerfuegbar)("Der Route Handler", () => { expect(gelesen.split(HERZSCHLAG).length - 1).toBeGreaterThanOrEqual(3); }); + it("beendet den Strom, wenn das Konto deaktiviert wird", async () => { + /* + * **Ein offener Strom hat das Deaktivieren überlebt** (Reviewbefund zu + * #029). Das Recht wurde genau einmal geprüft, beim Aufbau, und das Paar aus + * Kennung und Rolle für die Lebensdauer des Stroms festgehalten. Ein + * Administrator deaktivierte ein Konto — und eine offen gelassene Lasche auf + * dem Schreibtisch des Betroffenen bekam weiter live Status, Schrittzustände + * und die Zahl der Kommentare, bis jemand sie schloss. + * + * `docs/architektur.md` verspricht „ein deaktivierter Benutzer wird auch mit + * gültigem Cookie abgewiesen“. Für einen offenen Strom galt das nicht. + */ + process.env["SSE_HERZSCHLAG_MS"] = "40"; + const { laufId, admin } = await aufbau(); + angemeldetAls(admin as { id: string; rolle: "ADMIN" }); + + const antwort = await GET(anfrage("text/event-stream"), { + params: Promise.resolve({ id: laufId }), + }); + + await testPrisma.benutzer.update({ where: { id: admin.id }, data: { aktiv: false } }); + + const gelesen = await stromLesen(antwort, (text) => text.includes("nicht-angemeldet")); + expect(gelesen).toContain("nicht-angemeldet"); + // Und der Strom ist auch wirklich freigegeben, nicht nur stillgelegt. + await warteAuf(() => stroemeOffen() === 0, "Der Strom nach dem Deaktivieren"); + }); + + it("beendet den Strom, wenn die Sitzungen widerrufen werden", async () => { + /* + * Derselbe Weg, anderer Anlass: ein Administrator beendet die Sitzungen + * eines Benutzers (`sitzungenUngueltigAb`), ohne das Konto zu deaktivieren — + * etwa nach einem verlorenen Notebook. Dann muss der Strom genauso enden. + */ + process.env["SSE_HERZSCHLAG_MS"] = "40"; + const { laufId, admin } = await aufbau(); + angemeldetAls(admin as { id: string; rolle: "ADMIN" }); + + const antwort = await GET(anfrage("text/event-stream"), { + params: Promise.resolve({ id: laufId }), + }); + + await testPrisma.benutzer.update({ + where: { id: admin.id }, + // Eine Minute in der Zukunft: die Sitzung dieses Tests ist „von vorher“. + data: { sitzungenUngueltigAb: new Date(Date.now() + 60_000) }, + }); + + expect(await stromLesen(antwort, (text) => text.includes("nicht-angemeldet"))).toContain( + "nicht-angemeldet", + ); + }); + + it("hält den Strom, solange das Konto gilt", async () => { + /* + * Die Gegenrichtung, und sie ist nötig: eine Nachprüfung, die **immer** + * beendet, sähe im Test oben genauso aus. Hier bleibt der Strom über + * mehrere Herzschläge offen, ohne dass sich am Konto etwas ändert. + */ + process.env["SSE_HERZSCHLAG_MS"] = "40"; + const { laufId, admin } = await aufbau(); + angemeldetAls(admin as { id: string; rolle: "ADMIN" }); + + const antwort = await GET(anfrage("text/event-stream"), { + params: Promise.resolve({ id: laufId }), + }); + const gelesen = await stromLesen(antwort, (text) => text.split(HERZSCHLAG).length - 1 >= 3); + + expect(gelesen).not.toContain("nicht-angemeldet"); + expect(gelesen.split(HERZSCHLAG).length - 1).toBeGreaterThanOrEqual(3); + }); + it("fällt bei erreichter Obergrenze auf eine gewöhnliche Antwort zurück", async () => { process.env["SSE_MAX_STROEME"] = "1"; const { laufId, admin } = await aufbau(); diff --git a/tests/selbstauskunft.test.ts b/tests/selbstauskunft.test.ts index 6aa0ea3..47fd3b3 100644 --- a/tests/selbstauskunft.test.ts +++ b/tests/selbstauskunft.test.ts @@ -411,6 +411,42 @@ describe.runIf(datenbankVerfuegbar)("Zuordnung bei der Anmeldung", () => { // Ohne Urheber: das Konto gibt es an der Stelle vielleicht noch nicht. expect(protokoll[0]?.benutzerId).toBeNull(); expect(JSON.stringify(protokoll[0]?.nachher)).toContain("mhuber@firma.local"); + expect(protokoll[0]?.nachher).toMatchObject({ treffer: 2 }); + }); + + it("nennt im Protokoll die wirkliche Zahl der Treffer, nicht zwei", async () => { + /* + * **Die Zahl war geraten.** `personZuUpn` hatte `take: 2` — fuer die + * Entscheidung „mehrdeutig“ genuegt das, aber dieselbe Zahl ging in den + * Protokolleintrag und sagte dort immer „2“, auch bei vier Treffern + * (Reviewbefund zu #029). + * + * Wer das Protokoll liest, liest es, **um den Fall aufzuloesen** — er muss + * wissen, wie viele Zeilen er zu bereinigen hat. Eine Zahl, die nicht die + * Zahl ist, schickt ihn nach der zweiten Korrektur zurueck. + */ + await aufbau(); + for (const kuerzel of ["ZWEI", "DREI", "VIER"]) { + const weiterer = await testPrisma.kunde.create({ + data: { name: `${kuerzel} GmbH`, kuerzel }, + }); + await testPrisma.mitarbeiter.create({ + data: { + kundeId: weiterer.id, + vorname: "Maria", + nachname: "Huber", + anzeigename: `Maria Huber (${kuerzel})`, + upn: "mhuber@firma.local", + }, + }); + } + + await expect(benutzerAusOidcUebernehmen(anspruecke())).rejects.toThrow(); + + const eintrag = await testPrisma.protokoll.findFirstOrThrow({ + where: { aktion: { contains: "mehrdeutig" } }, + }); + expect(eintrag.nachher).toMatchObject({ treffer: 4 }); }); it("ordnet keine deaktivierte Person zu", async () => { diff --git a/tests/verzeichnis.test.ts b/tests/verzeichnis.test.ts index f3b634d..978d771 100644 --- a/tests/verzeichnis.test.ts +++ b/tests/verzeichnis.test.ts @@ -436,6 +436,57 @@ describe.runIf(datenbankVerfuegbar)("Verzeichnisquellen", () => { ); }); + it("weist einen Attributnamen ab, der keiner ist – auch am Formular vorbei", async () => { + /* + * **Die Regel sass eine Schicht ueber dem Code, der sich auf sie berief** + * (Reviewbefund zu #029). `src/server/graph/leser.ts` schrieb im Kommentar + * „die Werte selbst sind feste Feldnamen aus ATTRIBUT_MUSTER, also ist hier + * nichts zu maskieren“ — gepruefet wurde das aber nur in + * `zuordnungAusFormular`, also in der Server Action. + * + * Nachgewiesen wurde, dass genau dieser Name durch den Dienst kam und einen + * **zweiten `$filter`** in die Graph-Adresse schrieb. Hereinkommen konnte er + * nur mit `verwaltung` oder per Skript, Seed und `psql` — die + * Sicherheitswirkung ist also klein. Der Befund ist der Ort der Regel: + * CLAUDE.md verlangt sie in der Datenbank, und sie stand nicht einmal im + * Dienst. + */ + const { admin } = await grundbestand(); + const boese = { abteilung: "department,id&$filter=boese eq 1" }; + + await expect(quelleAnlegen(quellenEingabe({ attributZuordnung: boese }), admin)).rejects.toThrow( + /kein Attributname/u, + ); + + // Und beim Bearbeiten genauso. + const quelle = await quelleAnlegen(quellenEingabe(), admin); + await expect(quelleBearbeiten(quelle.id, { attributZuordnung: boese }, admin)).rejects.toThrow( + /kein Attributname/u, + ); + + // Die gespeicherte Zeile ist unberührt. + expect((await quelleHolen(quelle.id, admin))?.attributZuordnung.abteilung).toBe(AD_ZUORDNUNG.abteilung); + }); + + it("verwirft beim Lesen einen unbrauchbaren Namen, statt die Liste zu sprengen", async () => { + /* + * Die Gegenrichtung: eine Zeile, die vor dieser Prüfung angelegt wurde oder + * von Hand in die Datenbank kam. Hier zu werfen wäre falsch — eine einzige + * kaputte Zeile machte die ganze Quellenliste unbenutzbar. Sie bekommt + * deshalb die Vorgabe. + */ + const { admin } = await grundbestand(); + const quelle = await quelleAnlegen(quellenEingabe(), admin); + await testPrisma.verzeichnisquelle.update({ + where: { id: quelle.id }, + data: { attributZuordnung: { abteilung: "department,id&$filter=boese eq 1" } }, + }); + + const gelesen = await quelleHolen(quelle.id, admin); + expect(gelesen?.attributZuordnung.abteilung).toBe(AD_ZUORDNUNG.abteilung); + expect(JSON.stringify(gelesen)).not.toContain("$filter"); + }); + it("weist ein Intervall ausserhalb der Grenzen ab", async () => { const { admin } = await grundbestand(); await expect(quelleAnlegen(quellenEingabe({ abgleichIntervallMinuten: 1 }), admin)).rejects.toThrow( diff --git a/tests/zugang.test.ts b/tests/zugang.test.ts index 1590c6d..25d6ad7 100644 --- a/tests/zugang.test.ts +++ b/tests/zugang.test.ts @@ -3,7 +3,7 @@ import { join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; import { SignJWT } from "jose"; import { sicheresZiel } from "@/server/auth/anmeldung"; -import { istOeffentlich } from "@/proxy"; +import { OEFFENTLICH, istOeffentlich } from "@/proxy"; import { kurzzeitTokenErzeugen, kurzzeitTokenLesen } from "@/lib/kurzzeit-token"; import { sitzungErzeugen, sitzungLesen } from "@/lib/sitzung"; @@ -67,20 +67,55 @@ describe("Ruecksprungziel gegen Steuerzeichen", () => { }); describe("Allowlist des Proxys", () => { + /** + * Jeder oeffentliche Pfad, einzeln aufgezaehlt — **und zwar absichtlich von + * Hand.** Diese Liste ist die Behauptung „genau das soll ohne Anmeldung + * gehen“; aus `OEFFENTLICH` abgeleitet wuerde sie nichts mehr behaupten. + * Dass sie vollstaendig ist, prueft der Test darunter. + */ + const ERWARTET_OEFFENTLICH = [ + "/anmelden", + "/api/auth", + "/api/gesundheit", + // Der SCIM-Endpunkt (#029): ein Identitaetsanbieter hat kein Cookie, er + // weist sich mit einem Bearer-Token aus, und jeder Handler darunter prueft + // es selbst. Ohne diesen Eintrag leitet der Proxy jede Zustellung auf + // /anmelden um, und das Provisioning hoert still auf zu arbeiten. + "/api/scim", + "/manifest.webmanifest", + "/icon.svg", + ] as const; + it("laesst genau die Pfade durch, die ohne Anmeldung gehen muessen", () => { for (const pfad of [ - "/anmelden", + ...ERWARTET_OEFFENTLICH, + // Und die Unterpfade, auf die es im Betrieb ankommt. "/api/auth/oidc/start", "/api/auth/oidc/rueckruf", "/api/auth/abmelden", - "/api/gesundheit", - "/manifest.webmanifest", - "/icon.svg", + "/api/scim/v2/Users", + "/api/scim/v2/ServiceProviderConfig", ]) { expect(istOeffentlich(pfad), pfad).toBe(true); } }); + it("hat keinen oeffentlichen Pfad, den dieser Test nicht kennt", () => { + /* + * **Der Test, der gefehlt hat.** `/api/scim` kam mit #029 in die Allowlist + * und stand in keiner Pruefung — die Zeile liess sich entfernen, ohne dass + * ein einziger Test rot wurde (Reviewbefund). Dabei ist der Eintrag beides: + * noetig, damit das Provisioning ueberhaupt ankommt, und eine + * Sicherheitsentscheidung, weil darunter jeder Handler sein Token selbst + * pruefen **muss**. + * + * Deshalb hier der Vergleich in die andere Richtung: wer `OEFFENTLICH` + * erweitert, muss es oben aufschreiben. Eine Liste, die nur in einer + * Richtung geprueft wird, laeuft dem Code hinterher. + */ + expect([...OEFFENTLICH].sort()).toEqual([...ERWARTET_OEFFENTLICH].sort()); + }); + it("laesst sich nicht durch einen angehaengten Namen austricksen", () => { // Ein reiner startsWith-Vergleich haette /anmeldenX durchgelassen. for (const pfad of [