From 6042f6f2f617bb1cf041489e167a16ac9f279034 Mon Sep 17 00:00:00 2001 From: marc Date: Sat, 12 Sep 2026 23:04:04 +0200 Subject: [PATCH] test(tags): bleibt ein fremdes Etikett fremd? internal/tags hatte null Tests, und die Rechtepruefung steckt dort nicht in einer if-Zeile, sondern in WHERE-Klauseln. Ein Tippfehler darin faellt beim Lesen nicht auf: Die Abfrage laeuft, sie liefert nur zu viel. Fuenf Tests auf dieselbe Frage -- Liste, Umbenennen, Loeschen, Anhaengen an die eigene Datei, und vor allem das Auflisten der **Dateien** unter einem Etikett. Der letzte Weg gibt Dateinamen, Ordner und Groessen heraus; wer dort eine fremde Etikett-ID erraet, saehe einen Ausschnitt aus fremden Unterlagen. Dazu Gruppen: Mitglied ja, Nichtmitglied nein, und wer die Gruppe verlaesst, verliert den Zugriff im selben Moment. Gegen kaputten Code gegengeprueft: Eigentuemerpruefung aus FilesWithTag entfernt -> "Bert sieht 1 fremde Dateien: Gehaltsabrechnung.pdf". Pruefung in SetFieldsForFile ausgehebelt -> zwei Tests rot. **Ein Verdacht erwies sich als falsch, und das ist der nuetzlichere Ausgang:** List bietet Gruppen-Etiketten an, SetFieldsForFile vergleicht gegen den Prinzipal der Datei -- sah nach einem Widerspruch aus. Ist keiner; die Oberflaeche filtert bereits (TagDialog.tsx:69). Und die Ablehnung ist nicht Foermlichkeit, sondern die tragende Absicherung: FilesWithTag prueft, ob man das *Etikett* sehen darf, nicht die *Datei*. Das ist nur deshalb harmlos, weil ein Etikett stets denselben Prinzipal hat wie die Dateien darunter. Der Test haelt jetzt diesen Zusammenhang fest, nicht bloss eine Fehlermeldung. Nebenbei: tags hat zwei Fehlermarken fuer dieselbe Aussage (ErrNotFound und ErrCorrespondentNotFound). Kein Fehler, aber eine Stolperstelle. Co-Authored-By: Claude Opus 5 --- BEFUNDE.md | 34 ++- TESTING.md | 29 +++ backend/internal/tags/store_test.go | 319 ++++++++++++++++++++++++++++ 3 files changed, 378 insertions(+), 4 deletions(-) create mode 100644 backend/internal/tags/store_test.go diff --git a/BEFUNDE.md b/BEFUNDE.md index cc73c28..8f6fab5 100644 --- a/BEFUNDE.md +++ b/BEFUNDE.md @@ -187,8 +187,35 @@ nur die Laufzeit stieg von 0,2 auf 27 Sekunden. Die Tests schleiften über die Konstante und setzten die Zahl voraus, statt sie zu prüfen. Dagegen steht jetzt ein eigener Test. -**Offen bleibt `internal/tags`:** Etiketten sind je Konto, und dass ein fremdes -Etikett fremd bleibt, prüft bis heute nichts nach. +**`internal/tags` ebenfalls erledigt am 12.09.2026.** Fünf Tests, alle auf +dieselbe Frage: Bleibt ein fremdes Etikett fremd – in der Liste, beim +Umbenennen, beim Löschen, beim Anhängen an die eigene Datei, und vor allem beim +**Auflisten der Dateien darunter**. Der letzte Weg gibt Dateinamen, Ordner und +Größen heraus; wer dort eine fremde Etikett-ID errät, sähe einen Ausschnitt aus +fremden Unterlagen. + +Auch hier gegen kaputten Code gegengeprüft: Eigentümerprüfung aus +`FilesWithTag` entfernt → *„Bert sieht 1 fremde Dateien: Gehaltsabrechnung.pdf"*. +Prüfung in `SetFieldsForFile` ausgehebelt → zwei Tests rot. + +**Ein Verdacht erwies sich dabei als falsch, und das ist der nützlichere +Ausgang:** `List` bietet Gruppen-Etiketten an, `SetFieldsForFile` vergleicht +gegen den Prinzipal der *Datei* – das sah nach einem Widerspruch aus, bei dem +die Oberfläche etwas anbietet, was der Server ablehnt. Ist keiner: Die +Oberfläche filtert bereits (`TagDialog.tsx:69`). Und die Ablehnung ist nicht +Förmlichkeit, sondern die tragende Absicherung – `FilesWithTag` prüft, ob man +das *Etikett* sehen darf, **nicht** die *Datei*. Das ist nur deshalb harmlos, +weil ein Etikett stets denselben Prinzipal hat wie die Dateien darunter. Fiele +die Prüfung weg, hinge an einem Gruppen-Etikett eine private Datei, und jedes +Gruppenmitglied sähe ihren Namen. Der Test hält jetzt diesen Zusammenhang fest, +nicht bloß eine Fehlermeldung. + +Nebenbei: `internal/tags` hat **zwei** Fehlermarken für dieselbe Aussage – +`ErrNotFound` und `ErrCorrespondentNotFound`. Kein Fehler, aber eine +Stolperstelle; jetzt wenigstens festgehalten. + +**Offen bleiben** `internal/sharing`, `internal/rescan` (beide mit +Python-Selbsttest), `internal/audit` und `internal/thumbnails`. --- @@ -234,5 +261,4 @@ gibt: 1. ~~`recover` in die Hintergrund-Goroutinen~~ ✅ 12.09.2026 2. ~~Prüfsummen-Abweichung melden~~ ✅ 12.09.2026 – und dabei fiel auf, dass **alle** kontextgebundenen Logzeilen ins Nichts gingen (2b). -3. ~~Tests für `documents`~~ ✅ 12.09.2026. Offen: `internal/tags` — die - Rechteprüfung dort ist ungeprüft. +3. ~~Tests für `documents`~~ ✅ und ~~`tags`~~ ✅ 12.09.2026. diff --git a/TESTING.md b/TESTING.md index f34dff6..81a82e3 100644 --- a/TESTING.md +++ b/TESTING.md @@ -269,6 +269,29 @@ es auffällt: Dazu: gelöschte Dateien werden nicht mehr genommen, ein erneuter Upload gibt eine neue Chance, und `Complete` schreibt Text und Auftragsende zusammen. +### Etiketten und Absender: bleibt Fremdes fremd? + + ./scripts/db-tests.sh ./internal/tags/ + +Die Rechteprüfung steckt hier nicht in einer `if`-Zeile, sondern in +WHERE-Klauseln. Ein Tippfehler darin fällt beim Lesen nicht auf: Die Abfrage +läuft, sie liefert nur zu viel. + +Fünf Tests auf dieselbe Frage – Liste, Umbenennen, Löschen, Anhängen an die +eigene Datei, und vor allem das **Auflisten der Dateien** unter einem Etikett. +Der letzte Weg gibt Dateinamen, Ordner und Größen heraus. + +Dazu die Gruppen: Ein Mitglied sieht das Gruppen-Etikett, ein Nichtmitglied +nicht, und wer die Gruppe verlässt, verliert den Zugriff im selben Moment. + +**Eine Absicherung, die wie eine Förmlichkeit aussieht:** Ein Gruppen-Etikett +darf nicht an eine persönliche Datei. Der Grund steht nicht im Code, wo die +Prüfung sitzt, sondern zwei Dateien weiter: `FilesWithTag` prüft, ob der +Anfragende das *Etikett* sehen darf – **nicht**, ob er die *Datei* sehen darf. +Harmlos ist das nur, weil ein Etikett stets denselben Prinzipal hat wie die +Dateien darunter. Fiele die Prüfung weg, hinge an einem Gruppen-Etikett eine +private Datei, und jedes Gruppenmitglied bekäme ihren Namen zu sehen. + ### Was der Mutationsversuch gezeigt hat Die Tests wurden gegen **absichtlich kaputten Code** gegengeprüft – sonst weiß @@ -283,6 +306,12 @@ Genau so verschwindet eine Entscheidung unbemerkt aus einer Anwendung. Dagegen steht jetzt ein Test, der die Zahl selbst festhält und sagt, wo die Begründung hingehört, falls sie sich ändert. +Bei den Etiketten dasselbe Vorgehen, mit klarerem Ausgang: Eigentümerprüfung +aus `FilesWithTag` entfernt → *„Bert sieht 1 fremde Dateien: +Gehaltsabrechnung.pdf"*. Prüfung in `SetFieldsForFile` ausgehebelt → zwei Tests +rot. Beide Absicherungen sind also wirklich abgedeckt und nicht bloß +mitgelaufen. + ## Notfall: Migration hängt fest ("Dirty database version") Betrifft jede künftige Migration, nicht nur eine bestimmte. Nachgestellt und diff --git a/backend/internal/tags/store_test.go b/backend/internal/tags/store_test.go new file mode 100644 index 0000000..2edde66 --- /dev/null +++ b/backend/internal/tags/store_test.go @@ -0,0 +1,319 @@ +package tags + +import ( + "context" + "errors" + "os" + "testing" + + "github.com/google/uuid" + "github.com/jackc/pgx/v5/pgxpool" +) + +// Etiketten und Absender gehören einem Konto oder einer Gruppe. Dass ein +// fremdes Etikett fremd **bleibt**, prüfte bis zum 12.09.2026 nichts nach – +// und die Prüfung steckt hier nicht in einer if-Zeile, sondern in +// WHERE-Klauseln. Ein Tippfehler darin fällt beim Lesen nicht auf: Die Abfrage +// läuft, sie liefert nur zu viel. +// +// Der teuerste Fall ist FilesWithTag: Es gibt **Dateinamen und Ordner** +// heraus. Wer dort eine fremde Etikett-ID errät, sähe einen Ausschnitt aus +// fremden Unterlagen. +// +// Läuft nur mit TEST_DSN. Einfachster Weg: ./scripts/db-tests.sh ./internal/tags/ + +func testStore(t *testing.T) (*Store, *pgxpool.Pool, context.Context) { + t.Helper() + dsn := os.Getenv("TEST_DSN") + if dsn == "" { + t.Skip("TEST_DSN nicht gesetzt") + } + ctx := context.Background() + pool, err := pgxpool.New(ctx, dsn) + if err != nil { + t.Fatalf("verbinden: %v", err) + } + t.Cleanup(pool.Close) + return NewStore(pool), pool, ctx +} + +func konto(t *testing.T, pool *pgxpool.Pool, ctx context.Context, name string) uuid.UUID { + t.Helper() + var id uuid.UUID + err := pool.QueryRow(ctx, + `INSERT INTO users (username, display_name, password, is_admin) + VALUES ($1, $1, 'x', false) RETURNING id`, name+"-"+uuid.NewString()[:8], + ).Scan(&id) + if err != nil { + t.Fatalf("konto %s: %v", name, err) + } + t.Cleanup(func() { _, _ = pool.Exec(ctx, `DELETE FROM users WHERE id = $1`, id) }) + return id +} + +func datei(t *testing.T, pool *pgxpool.Pool, ctx context.Context, owner uuid.UUID, name string) uuid.UUID { + t.Helper() + var id uuid.UUID + err := pool.QueryRow(ctx, + `INSERT INTO files (owner_id, folder_id, name, mime_type, size, checksum, storage_key) + VALUES ($1, NULL, $2, 'application/pdf', 1, 'x', $3) RETURNING id`, + owner, name, "k/"+uuid.NewString(), + ).Scan(&id) + if err != nil { + t.Fatalf("datei: %v", err) + } + return id +} + +// ── Ein fremdes Etikett bleibt fremd ──────────────────────────────────────── + +func TestFremdesEtikettBleibtFremd(t *testing.T) { + s, pool, ctx := testStore(t) + a := konto(t, pool, ctx, "anna") + b := konto(t, pool, ctx, "bert") + + annas, err := s.Create(ctx, a, nil, "Steuer 2026", "#ff0000", "") + if err != nil { + t.Fatalf("anlegen: %v", err) + } + + t.Run("taucht nicht in Berts Liste auf", func(t *testing.T) { + liste, err := s.List(ctx, b) + if err != nil { + t.Fatalf("list: %v", err) + } + for _, tg := range liste { + if tg.ID == annas.ID { + t.Fatal("Bert sieht Annas Etikett") + } + } + }) + + t.Run("Bert kann es nicht umbenennen", func(t *testing.T) { + if _, err := s.Update(ctx, b, annas.ID, "Uebernommen", "#00ff00", ""); !errors.Is(err, ErrNotFound) { + t.Fatalf("Update lieferte %v, erwartet ErrNotFound", err) + } + // Und der Name steht wirklich noch da – der Fehlercode allein sagt + // nichts darüber, ob nebenbei geschrieben wurde. + var name string + _ = pool.QueryRow(ctx, `SELECT name FROM tags WHERE id = $1`, annas.ID).Scan(&name) + if name != "Steuer 2026" { + t.Fatalf("Name ist jetzt %q", name) + } + }) + + t.Run("Bert kann es nicht löschen", func(t *testing.T) { + if err := s.Delete(ctx, b, annas.ID); !errors.Is(err, ErrNotFound) { + t.Fatalf("Delete lieferte %v, erwartet ErrNotFound", err) + } + var da bool + _ = pool.QueryRow(ctx, `SELECT EXISTS(SELECT 1 FROM tags WHERE id = $1)`, annas.ID).Scan(&da) + if !da { + t.Fatal("Annas Etikett ist weg") + } + }) + + // Der teuerste Fall: Hier kämen Dateinamen und Ordner heraus. + t.Run("Bert sieht die Dateien darunter nicht", func(t *testing.T) { + f := datei(t, pool, ctx, a, "Gehaltsabrechnung.pdf") + if err := s.SetForFile(ctx, f, a, a, []uuid.UUID{annas.ID}, nil); err != nil { + t.Fatalf("etikettieren: %v", err) + } + // Anna sieht sie – sonst prüfte der nächste Teil eine leere Liste. + meine, err := s.FilesWithTag(ctx, a, annas.ID) + if err != nil || len(meine) != 1 { + t.Fatalf("Anna sieht ihre eigene Datei nicht: %v, %d Treffer", err, len(meine)) + } + fremde, err := s.FilesWithTag(ctx, b, annas.ID) + if err != nil { + t.Fatalf("FilesWithTag: %v", err) + } + if len(fremde) != 0 { + t.Fatalf("Bert sieht %d fremde Dateien: %v", len(fremde), fremde[0].Name) + } + }) + + t.Run("Bert kann es nicht an seine eigene Datei hängen", func(t *testing.T) { + f := datei(t, pool, ctx, b, "Berts Datei.pdf") + err := s.SetForFile(ctx, f, b, b, []uuid.UUID{annas.ID}, nil) + if !errors.Is(err, ErrWrongOwner) { + t.Fatalf("SetForFile lieferte %v, erwartet ErrWrongOwner", err) + } + var n int + _ = pool.QueryRow(ctx, `SELECT COUNT(*) FROM file_tags WHERE file_id = $1`, f).Scan(&n) + if n != 0 { + t.Fatalf("%d Zuordnungen entstanden trotzdem", n) + } + }) +} + +// ── Gruppen ───────────────────────────────────────────────────────────────── + +func gruppeMit(t *testing.T, pool *pgxpool.Pool, ctx context.Context, mitglieder ...uuid.UUID) uuid.UUID { + t.Helper() + var g uuid.UUID + err := pool.QueryRow(ctx, + `INSERT INTO groups (name) VALUES ($1) RETURNING id`, "probe-"+uuid.NewString()[:8], + ).Scan(&g) + if err != nil { + t.Fatalf("gruppe: %v", err) + } + t.Cleanup(func() { _, _ = pool.Exec(ctx, `DELETE FROM groups WHERE id = $1`, g) }) + for _, m := range mitglieder { + if _, err := pool.Exec(ctx, + `INSERT INTO group_members (group_id, user_id) VALUES ($1, $2)`, g, m); err != nil { + t.Fatalf("mitglied: %v", err) + } + } + return g +} + +func TestGruppenEtikettSiehtNurDieGruppe(t *testing.T) { + s, pool, ctx := testStore(t) + drin := konto(t, pool, ctx, "drin") + draussen := konto(t, pool, ctx, "draussen") + g := gruppeMit(t, pool, ctx, drin) + + tag, err := s.Create(ctx, drin, &g, "Buchhaltung", "#0000ff", "") + if err != nil { + t.Fatalf("anlegen: %v", err) + } + + sieht := func(user uuid.UUID) bool { + liste, err := s.List(ctx, user) + if err != nil { + t.Fatalf("list: %v", err) + } + for _, x := range liste { + if x.ID == tag.ID { + return true + } + } + return false + } + if !sieht(drin) { + t.Fatal("das Mitglied sieht das Gruppen-Etikett nicht") + } + if sieht(draussen) { + t.Fatal("ein Nichtmitglied sieht das Gruppen-Etikett") + } + + // Mitgliedschaft ersetzt die Freigabe: Wer die Gruppe verlässt, verliert + // den Zugriff im selben Moment. + if _, err := pool.Exec(ctx, + `DELETE FROM group_members WHERE group_id = $1 AND user_id = $2`, g, drin); err != nil { + t.Fatalf("austragen: %v", err) + } + if sieht(drin) { + t.Fatal("nach dem Austritt weiterhin sichtbar") + } + if _, err := s.Update(ctx, drin, tag.ID, "Umbenannt", "#111111", ""); !errors.Is(err, ErrNotFound) { + t.Fatalf("nach dem Austritt noch änderbar: %v", err) + } +} + +// Ein Gruppen-Etikett gehört **nicht** an eine persönliche Datei – und das ist +// keine Förmlichkeit, sondern die Stelle, an der die Rechte hängen. +// +// Der Anlass war ein Verdacht, der sich als falsch erwies: `List` bietet dem +// Nutzer Gruppen-Etiketten mit an, `SetFieldsForFile` vergleicht aber gegen +// den *Prinzipal der Datei*. Sieht nach Widerspruch aus. Ist keiner – die +// Oberfläche filtert bereits (`TagDialog.tsx`: bei einer persönlichen Datei +// nur Etiketten ohne group_id). +// +// **Warum die Ablehnung bleiben muss:** `FilesWithTag` prüft, ob der +// Anfragende das *Etikett* sehen darf – aber **nicht**, ob er die *Datei* +// sehen darf. Das ist nur deshalb harmlos, weil ein Etikett stets denselben +// Prinzipal hat wie die Dateien darunter: ein persönliches nur an eigenen +// Dateien, ein Gruppen-Etikett nur an Gruppendateien, die alle Mitglieder +// ohnehin sehen. +// +// Fiele diese Prüfung weg, hinge an einem Gruppen-Etikett eine private Datei – +// und jedes Gruppenmitglied bekäme über die Etikettenansicht ihren Namen, +// Ordner und ihre Größe zu sehen. Deshalb steht dieser Test hier: Er hält +// nicht eine Fehlermeldung fest, sondern eine Absicherung. +func TestGruppenEtikettGehoertNichtAnEinePersoenlicheDatei(t *testing.T) { + s, pool, ctx := testStore(t) + u := konto(t, pool, ctx, "mitglied") + g := gruppeMit(t, pool, ctx, u) + + tag, err := s.Create(ctx, u, &g, "Gemeinsam", "#00ffff", "") + if err != nil { + t.Fatalf("anlegen: %v", err) + } + f := datei(t, pool, ctx, u, "Privat.pdf") + + // Prinzipal der Datei ist der Nutzer, nicht die Gruppe. + if err := s.SetForFile(ctx, f, u, u, []uuid.UUID{tag.ID}, nil); !errors.Is(err, ErrWrongOwner) { + t.Fatalf("SetForFile lieferte %v, erwartet ErrWrongOwner – ohne diese "+ + "Prüfung sähe jedes Gruppenmitglied diese private Datei", err) + } + var n int + _ = pool.QueryRow(ctx, `SELECT COUNT(*) FROM file_tags WHERE file_id = $1`, f).Scan(&n) + if n != 0 { + t.Fatalf("%d Zuordnungen entstanden trotzdem", n) + } + + // Die Gegenprobe: An einer **Gruppendatei** geht dasselbe Etikett. + var gf uuid.UUID + err = pool.QueryRow(ctx, + `INSERT INTO files (owner_id, group_id, folder_id, name, mime_type, size, checksum, storage_key) + VALUES (NULL, $1, NULL, 'Gemeinsam.pdf', 'application/pdf', 1, 'x', $2) RETURNING id`, + g, "k/"+uuid.NewString()).Scan(&gf) + if err != nil { + t.Fatalf("gruppendatei: %v", err) + } + if err := s.SetForFile(ctx, gf, g, u, []uuid.UUID{tag.ID}, nil); err != nil { + t.Fatalf("an einer Gruppendatei muss es gehen: %v", err) + } +} + +// ── Absender ──────────────────────────────────────────────────────────────── + +func TestFremderAbsenderBleibtFremd(t *testing.T) { + s, pool, ctx := testStore(t) + a := konto(t, pool, ctx, "anna") + b := konto(t, pool, ctx, "bert") + + k, err := s.CreateCorrespondent(ctx, a, nil, "Finanzamt", "") + if err != nil { + t.Fatalf("anlegen: %v", err) + } + + liste, err := s.ListCorrespondents(ctx, b) + if err != nil { + t.Fatalf("list: %v", err) + } + for _, x := range liste { + if x.ID == k.ID { + t.Fatal("Bert sieht Annas Absender") + } + } + // Absender haben ihre eigene Fehlermarke – ErrCorrespondentNotFound, nicht + // ErrNotFound. Zwei Marken für dieselbe Aussage in einem Paket sind eine + // Stolperstelle; hier festgehalten, damit sie wenigstens nicht unbemerkt + // die Seite wechselt. + if _, err := s.UpdateCorrespondent(ctx, b, k.ID, "Uebernommen", ""); !errors.Is(err, ErrCorrespondentNotFound) { + t.Fatalf("Update lieferte %v, erwartet ErrCorrespondentNotFound", err) + } + if err := s.DeleteCorrespondent(ctx, b, k.ID); !errors.Is(err, ErrCorrespondentNotFound) { + t.Fatalf("Delete lieferte %v, erwartet ErrCorrespondentNotFound", err) + } + + // Auch hier: Der Weg, der Dateinamen herausgibt. + f := datei(t, pool, ctx, a, "Bescheid.pdf") + if err := s.SetFieldsForFile(ctx, f, a, a, FileFields{CorrespondentID: &k.ID}); err != nil { + t.Fatalf("zuordnen: %v", err) + } + meine, err := s.FilesWithCorrespondent(ctx, a, k.ID) + if err != nil || len(meine) != 1 { + t.Fatalf("Anna sieht ihre eigene Datei nicht: %v, %d", err, len(meine)) + } + fremde, err := s.FilesWithCorrespondent(ctx, b, k.ID) + if err != nil { + t.Fatalf("FilesWithCorrespondent: %v", err) + } + if len(fremde) != 0 { + t.Fatalf("Bert sieht %d fremde Dateien", len(fremde)) + } +}