diff --git a/Doku/flaws.md b/Doku/flaws.md index 9e09788..817b2e4 100644 --- a/Doku/flaws.md +++ b/Doku/flaws.md @@ -23,7 +23,7 @@ Schweregrade: **Hoch** (Datenabfluss oder Rechteumgehung möglich), **Mittel** ( | FLAW-11 | Workspace | ~~Block-Upsert (`ON CONFLICT(id)`) prüft das Dokument nicht~~ | **Behoben** | | FLAW-12 | Benachrichtigungen | ~~`notify_before_minutes` wirkungslos; Alarm feuert nach Umplanung nie wieder; endlose Wiederholung bei Fehlern~~ | **Behoben** | | FLAW-13 | Datenbank | ~~Keine Schema-Versionierung; Migrationsfehler werden verschluckt; `migrations/001_init.sql` ist toter Code~~ | **Behoben** | -| FLAW-14 | Backend | Fehler von Schreiboperationen werden häufig ignoriert (`_, _ =`) | Mittel | +| FLAW-14 | Backend | ~~Fehler von Schreiboperationen werden häufig ignoriert (`_, _ =`)~~ | **Behoben** | | FLAW-15 | Realtime | ~~`WriteTimeout` von 30 s beendet SSE-Streams~~ | **Behoben** | | FLAW-16 | Setup | `/auth/setup` ist bis zum ersten Konto offen; Demo-Modus legt `admin/admin` an | Mittel | | FLAW-17 | Workspace | ~~Bearbeiter-Freigaben für Notizen nicht wählbar~~ | **Behoben** | @@ -132,6 +132,9 @@ Schweregrade: **Hoch** (Datenabfluss oder Rechteumgehung möglich), **Mittel** ( - **Maßnahme:** `schema_migrations`-Tabelle mit nummerierten, einmalig ausgeführten Schritten; die SQL-Datei entweder einbetten oder entfernen. ### FLAW-14: Verschluckte Fehler +- **Status: Behoben.** Mehrstufige Änderungen laufen in einer Transaktion (`Store.execInTx`, ganz oder gar nicht) und geben Fehler zurück: Ordner löschen (inkl. Papierkorb), Ordner verschieben, geteilte Ordner anlegen/abgleichen/löschen, Ordner-Freigaben und -Einhängungen, Aufgabenliste löschen, Konto löschen, Anheften, Passwort-Reset (neues Passwort und Entwerten aller Links zusammen). Einzelne Schreibvorgänge, deren Erfolg die API meldet (Abmelden, neues Passwort, Ersteinrichtung, Freigabe-Modus, Gruppe anlegen, Wiederherstellen, Verknüpfung lösen), geben ihren Fehler an den Client. Folgeaufgaben und Alarm-Markierungen protokollieren Fehler. Test: `TestMultiStepChangesAreAllOrNothing`. +- **Bewusst ohne Fehlerbehandlung** (Ausfall ändert nichts Wichtiges): `updated_at`-Stempel von Listen, „zuletzt genutzt“ bei API-Keys, Aufräumen abgelaufener Tokens, Beispieldaten und Demo-Konten, einzelne Schritte des Markdown-Imports (sie landen als „übersprungen“ im Bericht). +- **Nebenbefund (behoben):** `DeleteTaskList` löste alle Aufgaben aus der Liste, **bevor** geprüft wurde, ob der Aufrufer Eigentümer ist; ein Fremder mit der Listen-ID konnte sie so leeren. Ebenso konnten Leser einer Aufgabenliste die Verknüpfung Notiz ↔ Aufgabe lösen. - **Fundstellen:** u. a. `PatchDocumentBlocks` (Upserts/Deletes), `AddTaskListShare`, `RemoveTaskListShare`, viele `_, _ = s.db.Exec`. - **Problem:** Scheitert ein Schreibzugriff (z. B. `SQLITE_BUSY`), antwortet die API trotzdem mit Erfolg und broadcastet den Stand, der nie gespeichert wurde. Fehler lassen sich nur schwer nachvollziehen. - **Beispiel (behoben):** Das Lösen der Verknüpfung zwischen Notiz-Block und Aufgabe („Verknüpfung aufheben“, Löschen eines verknüpften Blocks) schrieb in nicht existierende Spalten `thoughts.doc_id`/`block_id`. Der Fehler wurde verschluckt, die Funktion war nie wirksam. Korrigiert; im neuen Sync-Weg gleicht `ReconcileDocTaskLinks` Häkchen und gelöschte Blöcke ab. diff --git a/internal/api/alarms.go b/internal/api/alarms.go index 9d2bf55..0a2e65e 100644 --- a/internal/api/alarms.go +++ b/internal/api/alarms.go @@ -150,7 +150,9 @@ func (s *Server) startAlarmTicker() { } if sentAny { - _ = s.store.MarkTaskAlarmSent(a.TaskID) + if err := s.store.MarkTaskAlarmSent(a.TaskID); err != nil { + slog.Error("Alarm als gesendet markieren fehlgeschlagen (wird erneut gesendet)", "task_id", a.TaskID, "error", err) + } } else if gaveUp, _ := s.store.RecordTaskAlarmFailure(a.TaskID); gaveUp { slog.Warn("Aufgaben-Alarm nach 3 Fehlversuchen aufgegeben", "task_id", a.TaskID, "title", a.Title) } diff --git a/internal/api/handlers_admin.go b/internal/api/handlers_admin.go index 1b19db0..1960828 100644 --- a/internal/api/handlers_admin.go +++ b/internal/api/handlers_admin.go @@ -319,7 +319,10 @@ func (s *Server) handleAdminUpdateSettings(w http.ResponseWriter, r *http.Reques } // A stricter mode takes away what is no longer allowed (shares, group memberships, assignments) if settings.SharingMode != previousMode { - _, _ = s.store.PruneSharing(settings.SharingMode, false) + if _, err := s.store.PruneSharing(settings.SharingMode, false); err != nil { + writeError(w, http.StatusInternalServerError, "Freigaben konnten nicht angepasst werden: "+err.Error()) + return + } } settings.SMTPConfiguredViaEnv = strings.TrimSpace(s.cfg.SMTPHost) != "" writeJSON(w, http.StatusOK, settings) diff --git a/internal/api/handlers_auth.go b/internal/api/handlers_auth.go index 8c286de..a6f1f0f 100644 --- a/internal/api/handlers_auth.go +++ b/internal/api/handlers_auth.go @@ -128,7 +128,10 @@ func (s *Server) handleFirstBootSetup(w http.ResponseWriter, r *http.Request) { } sys.AllowRegistration = req.AllowRegistration sys.SharingMode = storage.NormalizeSharingMode(req.SharingMode) - _ = s.store.UpdateSystemSettings(sys) + if err := s.store.UpdateSystemSettings(sys); err != nil { + writeError(w, http.StatusInternalServerError, "Einstellungen konnten nicht gespeichert werden: "+err.Error()) + return + } // In organization mode the first admin starts in an organization named after the instance if sys.SharingMode == storage.SharingOrganizations { if org, err := s.store.CreateOrganization(sys.InstanceName); err == nil { @@ -216,7 +219,10 @@ func (s *Server) handleRegister(w http.ResponseWriter, r *http.Request) { } if validInvite != nil { - _ = s.store.MarkUserInviteUsed(validInvite.ID, user.ID) + // Otherwise the code could be used again + if err := s.store.MarkUserInviteUsed(validInvite.ID, user.ID); err != nil { + slog.Error("Einladungscode konnte nicht als benutzt markiert werden", "invite", validInvite.ID, "error", err) + } if validInvite.OrganizationID != "" { _ = s.store.SetUserOrganization(user.ID, validInvite.OrganizationID) user.OrganizationID = validInvite.OrganizationID @@ -397,7 +403,11 @@ func (s *Server) handleLogout(w http.ResponseWriter, r *http.Request) { } else { exp = time.Now().Add(30 * 24 * time.Hour) } - _ = s.store.RevokeToken(tokenStr, userID, exp) + // The session must really end; the client is told if it did not + if err := s.store.RevokeToken(tokenStr, userID, exp); err != nil { + writeError(w, http.StatusInternalServerError, "Abmelden fehlgeschlagen") + return + } } } // Streams are not tied to one session; other devices reconnect with a new ticket on their own @@ -456,7 +466,10 @@ func (s *Server) handleUpdateUserSettings(w http.ResponseWriter, r *http.Request writeError(w, http.StatusInternalServerError, "Fehler beim Hashen des Passworts") return } - _ = s.store.UpdateUserPassword(userID, hash) + if err := s.store.UpdateUserPassword(userID, hash); err != nil { + writeError(w, http.StatusInternalServerError, "Passwort konnte nicht gespeichert werden") + return + } s.wsHub.DisconnectUser(userID) } u, err := s.store.UpdateUserFull( diff --git a/internal/api/handlers_tasklists.go b/internal/api/handlers_tasklists.go index 8df81d8..8fac112 100644 --- a/internal/api/handlers_tasklists.go +++ b/internal/api/handlers_tasklists.go @@ -72,7 +72,7 @@ func (s *Server) handleDeleteTaskList(w http.ResponseWriter, r *http.Request) { listID := chi.URLParam(r, "id") audienceBefore := s.store.GetTaskListUserIDs(listID) if err := s.store.DeleteTaskList(userID, listID); err != nil { - writeError(w, http.StatusInternalServerError, err.Error()) + writeError(w, http.StatusNotFound, err.Error()) return } s.broadcastTaskListChange(listID, audienceBefore, "TASK_LIST_DELETED") diff --git a/internal/api/handlers_teams.go b/internal/api/handlers_teams.go index f4885e4..db30a56 100644 --- a/internal/api/handlers_teams.go +++ b/internal/api/handlers_teams.go @@ -52,7 +52,10 @@ func (s *Server) handleCreateTeamDirect(w http.ResponseWriter, r *http.Request) return } // Automatically add creator to team as group admin - _ = s.store.AddTeamMember(team.ID, userID, "admin") + if err := s.store.AddTeamMember(team.ID, userID, "admin"); err != nil { + writeError(w, http.StatusInternalServerError, err.Error()) + return + } team.Members, _ = s.store.GetTeamMembers(team.ID) writeJSON(w, http.StatusCreated, team) } diff --git a/internal/api/handlers_thoughts.go b/internal/api/handlers_thoughts.go index 9bed8a6..a0ed90a 100644 --- a/internal/api/handlers_thoughts.go +++ b/internal/api/handlers_thoughts.go @@ -284,7 +284,10 @@ func (s *Server) handleUpdateThought(w http.ResponseWriter, r *http.Request) { func (s *Server) handleUnlinkEnhancedTask(w http.ResponseWriter, r *http.Request) { userID := auth.GetUserID(r.Context()) thoughtID := chi.URLParam(r, "id") - s.store.UnlinkTaskFromDocBlock(userID, thoughtID) + if err := s.store.UnlinkTaskFromDocBlock(userID, thoughtID); err != nil { + writeStoreError(w, http.StatusNotFound, err) + return + } writeJSON(w, http.StatusOK, map[string]bool{"unlinked": true}) } @@ -297,7 +300,6 @@ func (s *Server) handleDeleteThought(w http.ResponseWriter, r *http.Request) { return } audience := s.taskAudience(th) - s.store.UnlinkTaskFromDocBlock(userID, thoughtID) if err := s.store.DeleteThought(userID, thoughtID); err != nil { writeStoreError(w, http.StatusInternalServerError, err) return diff --git a/internal/api/server.go b/internal/api/server.go index 76cbab0..8a1d484 100644 --- a/internal/api/server.go +++ b/internal/api/server.go @@ -32,7 +32,7 @@ type Server struct { demo demoState // periodic reset in demo mode (demo.go) streamTickets streamTickets // one-time tickets for WebSocket/SSE (streamticket.go) - loginGuard *loginGuard // brute-force protection (loginguard.go) + loginGuard *loginGuard // brute-force protection (loginguard.go) } func NewServer(cfg *config.Config, store *storage.Store, wsHub *ws.Hub, staticFS fs.FS) *Server { diff --git a/internal/storage/concurrency_test.go b/internal/storage/concurrency_test.go index 51e1f11..165e22e 100644 --- a/internal/storage/concurrency_test.go +++ b/internal/storage/concurrency_test.go @@ -60,3 +60,36 @@ func TestConcurrentWritersDoNotFail(t *testing.T) { t.Fatalf("%d of %d writes failed", failed, 16*15*2) } } + +func TestMultiStepChangesAreAllOrNothing(t *testing.T) { + store, err := NewStore(filepath.Join(t.TempDir(), "atomic.db")) + if err != nil { + t.Fatal(err) + } + defer store.Close() + anna, _ := store.CreateUser("anna@example.com", "hash", "Anna", "member") + eve, _ := store.CreateUser("eve@example.com", "hash", "Eve", "member") + tl, _ := store.CreateTaskList(anna.ID, "", "", "Haushalt", "", "") + task, _ := store.CreateThoughtWithDetails(anna.ID, "Fenster", "normal", "manual", "", tl.ID, "todo", "") + _, _ = store.UpdateThoughtTriage(anna.ID, task.ID, "keep", "task", "", "", tl.ID) + + // A stranger deleting the list must not empty it (it used to unlink all tasks before checking) + if err := store.DeleteTaskList(eve.ID, tl.ID); err == nil { + t.Fatal("foreign user must not delete the task list") + } + if got, _ := store.GetThoughtByID(anna.ID, task.ID); got.TaskListID != tl.ID { + t.Fatalf("task was taken out of the list by a stranger") + } + + // A failing step rolls back the earlier ones + err = store.execInTx( + stmt(`UPDATE thoughts SET content = 'geändert' WHERE id = ?`, task.ID), + stmt(`UPDATE gibt_es_nicht SET x = 1`), + ) + if err == nil { + t.Fatal("expected an error from the second statement") + } + if got, _ := store.GetThoughtByID(anna.ID, task.ID); got.Content != "Fenster" { + t.Fatalf("first step must be rolled back, content is %q", got.Content) + } +} diff --git a/internal/storage/documents.go b/internal/storage/documents.go index 4601b82..6a989d5 100644 --- a/internal/storage/documents.go +++ b/internal/storage/documents.go @@ -630,16 +630,15 @@ func (s *Store) UpdateDocument(userID, docID string, title, icon, folderPath *st if isPinned != nil { doc.IsPinned = *isPinned + pin := []sqlStmt{stmt(`DELETE FROM doc_pins WHERE user_id = ? AND doc_id = ?`, userID, docID)} if *isPinned { - _, _ = s.db.Exec(`INSERT OR IGNORE INTO doc_pins (user_id, doc_id, created_at) VALUES (?, ?, ?)`, userID, docID, domain.NowISO()) - if doc.IsOwner { - _, _ = s.db.Exec(`UPDATE documents SET is_pinned = 1 WHERE id = ?`, docID) - } - } else { - _, _ = s.db.Exec(`DELETE FROM doc_pins WHERE user_id = ? AND doc_id = ?`, userID, docID) - if doc.IsOwner { - _, _ = s.db.Exec(`UPDATE documents SET is_pinned = 0 WHERE id = ?`, docID) - } + pin = []sqlStmt{stmt(`INSERT OR IGNORE INTO doc_pins (user_id, doc_id, created_at) VALUES (?, ?, ?)`, userID, docID, domain.NowISO())} + } + if doc.IsOwner { + pin = append(pin, stmt(`UPDATE documents SET is_pinned = ? WHERE id = ?`, boolInt(*isPinned), docID)) + } + if err := s.execInTx(pin...); err != nil { + return nil, err } } @@ -678,7 +677,9 @@ func (s *Store) DeleteDocument(userID, docID string) error { } if !doc.IsOwner { // Collaborator removing their shared access - _, _ = s.db.Exec(`DELETE FROM doc_user_folders WHERE user_id = ? AND doc_id = ?`, userID, docID) + if _, err := s.db.Exec(`DELETE FROM doc_user_folders WHERE user_id = ? AND doc_id = ?`, userID, docID); err != nil { + return err + } return s.DeleteDocShare(userID, docID, userID) } if doc.IsArchived { diff --git a/internal/storage/folders.go b/internal/storage/folders.go index 5d0250a..023ec11 100644 --- a/internal/storage/folders.go +++ b/internal/storage/folders.go @@ -105,15 +105,19 @@ func (s *Store) CreateFolder(userID, folderPath string) (string, error) { var existsID string err := s.db.QueryRow(`SELECT id FROM workspace_folders WHERE shared_folder_id = ? AND sub_path = ?`, sfID, subPath).Scan(&existsID) if err == sql.ErrNoRows { - _, _ = s.db.Exec(`INSERT INTO workspace_folders (id, user_id, shared_folder_id, path, sub_path, created_at) VALUES (?, ?, ?, ?, ?, ?)`, - id, userID, sfID, clean, subPath, now) + if _, err := s.db.Exec(`INSERT INTO workspace_folders (id, user_id, shared_folder_id, path, sub_path, created_at) VALUES (?, ?, ?, ?, ?, ?)`, + id, userID, sfID, clean, subPath, now); err != nil { + return "", err + } } } else { var existsID string err := s.db.QueryRow(`SELECT id FROM workspace_folders WHERE user_id = ? AND path = ? AND (shared_folder_id = '' OR shared_folder_id IS NULL)`, userID, clean).Scan(&existsID) if err == sql.ErrNoRows { - _, _ = s.db.Exec(`INSERT INTO workspace_folders (id, user_id, shared_folder_id, path, sub_path, created_at) VALUES (?, ?, '', ?, '', ?)`, - id, userID, clean, now) + if _, err := s.db.Exec(`INSERT INTO workspace_folders (id, user_id, shared_folder_id, path, sub_path, created_at) VALUES (?, ?, '', ?, '', ?)`, + id, userID, clean, now); err != nil { + return "", err + } } } @@ -127,6 +131,7 @@ func (s *Store) DeleteFolder(userID, folderPath string) ([]string, error) { } var deletedDocIDs []string + var writes []sqlStmt seenDocID := make(map[string]bool) mounts, _, err := s.getUserFolderMounts(userID) @@ -138,8 +143,7 @@ func (s *Store) DeleteFolder(userID, folderPath string) ([]string, error) { shFolder, sfErr := s.GetSharedFolderByID(userID, sfID) if sfErr == nil && !shFolder.IsOwner { // Collaborator removing mount/share - _ = s.DeleteSharedFolder(userID, sfID) - return []string{}, nil + return []string{}, s.DeleteSharedFolder(userID, sfID) } rows, qErr := s.db.Query(`SELECT id FROM documents WHERE shared_folder_id = ?`, sfID) if qErr == nil { @@ -152,7 +156,7 @@ func (s *Store) DeleteFolder(userID, folderPath string) ([]string, error) { } _ = rows.Close() } - _, _ = s.db.Exec(`DELETE FROM workspace_folders WHERE shared_folder_id = ?`, sfID) + writes = append(writes, stmt(`DELETE FROM workspace_folders WHERE shared_folder_id = ?`, sfID)) } else { // Subfolder inside a shared folder rows, qErr := s.db.Query( @@ -169,8 +173,8 @@ func (s *Store) DeleteFolder(userID, folderPath string) ([]string, error) { } _ = rows.Close() } - _, _ = s.db.Exec(`DELETE FROM workspace_folders WHERE shared_folder_id = ? AND (sub_path = ? OR sub_path LIKE ?)`, - sfID, subPath, subPath+"/%") + writes = append(writes, stmt(`DELETE FROM workspace_folders WHERE shared_folder_id = ? AND (sub_path = ? OR sub_path LIKE ?)`, + sfID, subPath, subPath+"/%")) } } } @@ -195,13 +199,18 @@ func (s *Store) DeleteFolder(userID, folderPath string) ([]string, error) { // The notes of the folder go to the trash; restoring one creates its folder again for _, id := range deletedDocIDs { - _ = s.trashDocument(userID, id) + writes = append(writes, trashStmts(userID, id)...) } // Also clear any personal folder placements for shared notes in this folder - _, _ = s.db.Exec(`UPDATE doc_user_folders SET folder_path = '' WHERE user_id = ? AND (folder_path = ? OR folder_path LIKE ?)`, userID, clean, clean+"/%") - _, _ = s.db.Exec(`DELETE FROM workspace_folders WHERE user_id = ? AND (path = ? OR path LIKE ?)`, userID, clean, clean+"/%") + writes = append(writes, + stmt(`UPDATE doc_user_folders SET folder_path = '' WHERE user_id = ? AND (folder_path = ? OR folder_path LIKE ?)`, userID, clean, clean+"/%"), + stmt(`DELETE FROM workspace_folders WHERE user_id = ? AND (path = ? OR path LIKE ?)`, userID, clean, clean+"/%")) + // All of it or nothing: no folder disappears while its notes stay behind, or the other way round + if err := s.execInTx(writes...); err != nil { + return nil, err + } return deletedDocIDs, nil } @@ -247,7 +256,9 @@ func (s *Store) MoveDocument(userID, docID, targetFolderPath string) (*domain.Do } // Remove personal override if any - _, _ = s.db.Exec(`DELETE FROM doc_user_folders WHERE user_id = ? AND doc_id = ?`, userID, docID) + if _, err := s.db.Exec(`DELETE FROM doc_user_folders WHERE user_id = ? AND doc_id = ?`, userID, docID); err != nil { + return nil, err + } doc.SharedFolderID = targetFolderID doc.SubFolderPath = subPath @@ -355,10 +366,10 @@ func (s *Store) MoveFolder(userID, sourcePath, targetParentPath string) (string, suffix := strings.TrimPrefix(doc.FolderPath, cleanSource) nextDocFolder = newFolderPath + suffix } - _, moveErr := s.MoveDocument(userID, doc.ID, nextDocFolder) - if moveErr == nil { - movedDocIDs = append(movedDocIDs, doc.ID) + if _, moveErr := s.MoveDocument(userID, doc.ID, nextDocFolder); moveErr != nil { + return "", movedDocIDs, fmt.Errorf("Notiz %q verschieben: %w", doc.Title, moveErr) } + movedDocIDs = append(movedDocIDs, doc.ID) } } @@ -366,8 +377,12 @@ func (s *Store) MoveFolder(userID, sourcePath, targetParentPath string) (string, targetSFID, targetSub, isTargetSF := matchMountPath(newFolderPath, mounts) sourceSFID, sourceSub, isSourceSF := matchMountPath(cleanSource, mounts) + var remap []sqlStmt wfRows, qErr := s.db.Query(`SELECT id, user_id, shared_folder_id, path, sub_path FROM workspace_folders`) - if qErr == nil { + if qErr != nil { + return "", movedDocIDs, qErr + } + { type wfRecord struct { id, uid, sfID, path, subPath string } @@ -402,10 +417,10 @@ func (s *Store) MoveFolder(userID, sourcePath, targetParentPath string) (string, newSub = targetSub + "/" + folderName + suffix } if isTargetSF { - _, _ = s.db.Exec(`UPDATE workspace_folders SET shared_folder_id = ?, sub_path = ? WHERE id = ?`, targetSFID, newSub, r.id) + remap = append(remap, stmt(`UPDATE workspace_folders SET shared_folder_id = ?, sub_path = ? WHERE id = ?`, targetSFID, newSub, r.id)) } else { newP := newFolderPath + suffix - _, _ = s.db.Exec(`UPDATE workspace_folders SET shared_folder_id = '', sub_path = '', path = ? WHERE id = ?`, newP, r.id) + remap = append(remap, stmt(`UPDATE workspace_folders SET shared_folder_id = '', sub_path = '', path = ? WHERE id = ?`, newP, r.id)) } } } else if r.path == cleanSource || strings.HasPrefix(r.path, cleanSource+"/") { @@ -418,24 +433,29 @@ func (s *Store) MoveFolder(userID, sourcePath, targetParentPath string) (string, } else { newSub = targetSub + "/" + folderName + suffix } - _, _ = s.db.Exec(`UPDATE workspace_folders SET shared_folder_id = ?, sub_path = ?, path = ? WHERE id = ?`, targetSFID, newSub, newP, r.id) + remap = append(remap, stmt(`UPDATE workspace_folders SET shared_folder_id = ?, sub_path = ?, path = ? WHERE id = ?`, targetSFID, newSub, newP, r.id)) } else { - _, _ = s.db.Exec(`UPDATE workspace_folders SET path = ? WHERE id = ?`, newP, r.id) + remap = append(remap, stmt(`UPDATE workspace_folders SET path = ? WHERE id = ?`, newP, r.id)) } } } } - _, _ = s.CreateFolder(userID, newFolderPath) - + if err := s.execInTx(remap...); err != nil { + return "", movedDocIDs, err + } + if _, err := s.CreateFolder(userID, newFolderPath); err != nil { + return "", movedDocIDs, err + } return newFolderPath, movedDocIDs, nil } // syncSharedFolderContents ensures all documents and subfolders under cleanMount owned by ownerID are linked to folderID. -func (s *Store) syncSharedFolderContents(ownerID, folderID, cleanMount string) { +func (s *Store) syncSharedFolderContents(ownerID, folderID, cleanMount string) error { if ownerID == "" || folderID == "" || cleanMount == "" { - return + return nil } + var links []sqlStmt // 1. Link all documents owned by ownerID in cleanMount to this shared folder rows, err := s.db.Query(`SELECT id, folder_path FROM documents WHERE user_id = ? AND is_archived = 0 AND (folder_path = ? OR folder_path LIKE ?)`, @@ -458,8 +478,8 @@ func (s *Store) syncSharedFolderContents(ownerID, folderID, cleanMount string) { } else if strings.HasPrefix(de.fp, cleanMount+"/") { subPath = strings.TrimPrefix(de.fp, cleanMount+"/") } - _, _ = s.db.Exec(`UPDATE documents SET shared_folder_id = ?, sub_folder_path = ? WHERE id = ?`, - folderID, subPath, de.id) + links = append(links, stmt(`UPDATE documents SET shared_folder_id = ?, sub_folder_path = ? WHERE id = ?`, + folderID, subPath, de.id)) } } @@ -482,10 +502,11 @@ func (s *Store) syncSharedFolderContents(ownerID, folderID, cleanMount string) { if strings.HasPrefix(wfe.path, cleanMount+"/") { subP = strings.TrimPrefix(wfe.path, cleanMount+"/") } - _, _ = s.db.Exec(`UPDATE workspace_folders SET shared_folder_id = ?, sub_path = ? WHERE id = ?`, - folderID, subP, wfe.id) + links = append(links, stmt(`UPDATE workspace_folders SET shared_folder_id = ?, sub_path = ? WHERE id = ?`, + folderID, subP, wfe.id)) } } + return s.execInTx(links...) } func (s *Store) CreateSharedFolder(userID, folderName, initialMountPath string) (*domain.SharedFolder, error) { @@ -506,7 +527,9 @@ func (s *Store) CreateSharedFolder(userID, folderName, initialMountPath string) WHERE f.owner_id = ? AND (m.mount_path = ? OR f.name = ?) `, userID, userID, cleanMount, cleanName).Scan(&existingID) if err == nil && existingID != "" { - s.syncSharedFolderContents(userID, existingID, cleanMount) + if err := s.syncSharedFolderContents(userID, existingID, cleanMount); err != nil { + return nil, err + } return s.GetSharedFolderByID(userID, existingID) } @@ -529,7 +552,9 @@ func (s *Store) CreateSharedFolder(userID, folderName, initialMountPath string) return nil, err } - s.syncSharedFolderContents(userID, folderID, cleanMount) + if err := s.syncSharedFolderContents(userID, folderID, cleanMount); err != nil { + return nil, err + } var ownerName string _ = s.db.QueryRow(`SELECT display_name FROM users WHERE id = ?`, userID).Scan(&ownerName) @@ -756,13 +781,16 @@ func (s *Store) CreateFolderShare(currentUserID, folderID, targetType, targetIde if ownerMount == "" { ownerMount = f.Name } - s.syncSharedFolderContents(f.OwnerID, folderID, ownerMount) + if err := s.syncSharedFolderContents(f.OwnerID, folderID, ownerMount); err != nil { + return nil, err + } + var mounts []sqlStmt if cleanType == "user" { - _, _ = s.db.Exec(` + mounts = append(mounts, stmt(` INSERT OR IGNORE INTO workspace_folder_mounts (user_id, folder_id, mount_path, created_at, updated_at) VALUES (?, ?, ?, ?, ?) - `, targetID, folderID, f.Name, now, now) + `, targetID, folderID, f.Name, now, now)) } else if cleanType == "team" { mRows, err := s.db.Query(`SELECT user_id FROM team_members WHERE team_id = ?`, targetID) if err == nil { @@ -775,13 +803,16 @@ func (s *Store) CreateFolderShare(currentUserID, folderID, targetType, targetIde } _ = mRows.Close() for _, uid := range memberIDs { - _, _ = s.db.Exec(` + mounts = append(mounts, stmt(` INSERT OR IGNORE INTO workspace_folder_mounts (user_id, folder_id, mount_path, created_at, updated_at) VALUES (?, ?, ?, ?, ?) - `, uid, folderID, f.Name, now, now) + `, uid, folderID, f.Name, now, now)) } } } + if err := s.execInTx(mounts...); err != nil { + return nil, err + } return &domain.FolderShare{ ID: shareID, @@ -806,24 +837,21 @@ func (s *Store) DeleteFolderShare(currentUserID, folderID, shareID string) error // Only the owner manages shares; everyone else can only leave the folder if !f.IsOwner { - _, err = s.db.Exec(`DELETE FROM workspace_folder_shares WHERE folder_id = ? AND target_type = 'user' AND target_id = ? AND (id = ? OR target_id = ?)`, - folderID, currentUserID, cleanID, cleanID) - _, _ = s.db.Exec(`DELETE FROM workspace_folder_mounts WHERE folder_id = ? AND user_id = ?`, folderID, currentUserID) - return err + return s.execInTx( + stmt(`DELETE FROM workspace_folder_shares WHERE folder_id = ? AND target_type = 'user' AND target_id = ? AND (id = ? OR target_id = ?)`, + folderID, currentUserID, cleanID, cleanID), + stmt(`DELETE FROM workspace_folder_mounts WHERE folder_id = ? AND user_id = ?`, folderID, currentUserID)) } var targetType, targetID string _ = s.db.QueryRow(`SELECT target_type, target_id FROM workspace_folder_shares WHERE folder_id = ? AND (id = ? OR target_id = ?)`, folderID, cleanID, cleanID).Scan(&targetType, &targetID) - _, err = s.db.Exec(`DELETE FROM workspace_folder_shares WHERE folder_id = ? AND (id = ? OR target_id = ?)`, folderID, cleanID, cleanID) - if err != nil { - return err - } + writes := []sqlStmt{stmt(`DELETE FROM workspace_folder_shares WHERE folder_id = ? AND (id = ? OR target_id = ?)`, folderID, cleanID, cleanID)} if targetType == "user" && targetID != "" { - _, _ = s.db.Exec(`DELETE FROM workspace_folder_mounts WHERE folder_id = ? AND user_id = ?`, folderID, targetID) + writes = append(writes, stmt(`DELETE FROM workspace_folder_mounts WHERE folder_id = ? AND user_id = ?`, folderID, targetID)) } - return nil + return s.execInTx(writes...) } func (s *Store) DeleteSharedFolder(currentUserID, folderID string) error { @@ -835,12 +863,12 @@ func (s *Store) DeleteSharedFolder(currentUserID, folderID string) error { return s.DeleteFolderShare(currentUserID, folderID, currentUserID) } - _, _ = s.db.Exec(`UPDATE documents SET shared_folder_id = '', sub_folder_path = '' WHERE shared_folder_id = ?`, folderID) - _, _ = s.db.Exec(`DELETE FROM workspace_folder_mounts WHERE folder_id = ?`, folderID) - _, _ = s.db.Exec(`DELETE FROM workspace_folder_shares WHERE folder_id = ?`, folderID) - _, _ = s.db.Exec(`DELETE FROM workspace_folders WHERE shared_folder_id = ?`, folderID) - _, err = s.db.Exec(`DELETE FROM workspace_shared_folders WHERE id = ?`, folderID) - return err + return s.execInTx( + stmt(`UPDATE documents SET shared_folder_id = '', sub_folder_path = '' WHERE shared_folder_id = ?`, folderID), + stmt(`DELETE FROM workspace_folder_mounts WHERE folder_id = ?`, folderID), + stmt(`DELETE FROM workspace_folder_shares WHERE folder_id = ?`, folderID), + stmt(`DELETE FROM workspace_folders WHERE shared_folder_id = ?`, folderID), + stmt(`DELETE FROM workspace_shared_folders WHERE id = ?`, folderID)) } // GetSharedFolderDocIDs returns all non-archived document IDs belonging to a shared folder. diff --git a/internal/storage/password_reset.go b/internal/storage/password_reset.go index 47e630c..6b2303f 100644 --- a/internal/storage/password_reset.go +++ b/internal/storage/password_reset.go @@ -84,6 +84,9 @@ func (s *Store) ResetPasswordWithToken(raw, passwordHash string) (string, error) if n, _ := res.RowsAffected(); n == 0 { return "", ErrResetTokenInvalid // used in parallel } - _, _ = s.db.Exec(`UPDATE password_reset_tokens SET used_at = ? WHERE user_id = ? AND used_at = ''`, now, userID) - return userID, s.UpdateUserPassword(userID, passwordHash) + // New password and the end of every other open link belong together + return userID, s.execInTx( + stmt(`UPDATE password_reset_tokens SET used_at = ? WHERE user_id = ? AND used_at = ''`, now, userID), + stmt(`UPDATE users SET password_hash = ?, token_version = token_version + 1 WHERE id = ?`, passwordHash, userID), + ) } diff --git a/internal/storage/recurrence.go b/internal/storage/recurrence.go index caf1d87..169e8ed 100644 --- a/internal/storage/recurrence.go +++ b/internal/storage/recurrence.go @@ -2,6 +2,7 @@ package storage import ( "errors" + "log/slog" "time" "github.com/sandrodach/mindsift/internal/domain" @@ -111,9 +112,11 @@ func (s *Store) spawnNextOccurrence(taskID string) { due := NextDueDate(t.DueDate, t.Recurrence, time.Now()) next, err := s.CreateThoughtWithDetails(t.UserID, t.Content, t.Priority, t.Source, due, t.TaskListID, column, t.Notes) if err != nil { + slog.Error("Folgeaufgabe konnte nicht angelegt werden", "task_id", taskID, "error", err) return } if _, err := s.UpdateThoughtTriage(t.UserID, next.ID, "keep", "task", "", due, t.TaskListID); err != nil { + slog.Error("Folgeaufgabe konnte nicht als Aufgabe abgelegt werden", "task_id", next.ID, "error", err) return } subs := make([]domain.SubTask, len(t.Subtasks)) @@ -121,10 +124,15 @@ func (s *Store) spawnNextOccurrence(taskID string) { subs[i] = domain.SubTask{ID: domain.NewID("st"), Title: st.Title, CreatedAt: domain.NowISO()} } tags := t.Tags - _, _ = s.UpdateThoughtDetails(t.UserID, next.ID, nil, nil, nil, nil, nil, &column, nil, &subs, &tags) - _, _ = s.db.Exec(`UPDATE thoughts SET recurrence = ?, sort_order = ? WHERE id = ?`, t.Recurrence, t.SortOrder, next.ID) + if _, err := s.UpdateThoughtDetails(t.UserID, next.ID, nil, nil, nil, nil, nil, &column, nil, &subs, &tags); err != nil { + slog.Error("Folgeaufgabe: Unteraufgaben und Tags fehlen", "task_id", next.ID, "error", err) + } + carry := []sqlStmt{stmt(`UPDATE thoughts SET recurrence = ?, sort_order = ? WHERE id = ?`, t.Recurrence, t.SortOrder, next.ID)} for _, uid := range t.Assignees { - _, _ = s.db.Exec(`INSERT OR IGNORE INTO task_assignees (task_id, user_id, created_at) VALUES (?, ?, ?)`, next.ID, uid, domain.NowISO()) + carry = append(carry, stmt(`INSERT OR IGNORE INTO task_assignees (task_id, user_id, created_at) VALUES (?, ?, ?)`, next.ID, uid, domain.NowISO())) + } + if err := s.execInTx(carry...); err != nil { + slog.Error("Folgeaufgabe: Wiederholung und Zuweisungen nicht übernommen", "task_id", next.ID, "error", err) } s.PruneTaskAssignees() diff --git a/internal/storage/sqlite.go b/internal/storage/sqlite.go index b9c2639..5a33117 100644 --- a/internal/storage/sqlite.go +++ b/internal/storage/sqlite.go @@ -5,6 +5,7 @@ import ( "database/sql" "encoding/json" "fmt" + "log/slog" "os" "path/filepath" "strings" @@ -665,7 +666,36 @@ func (s *Store) migrateWorkspaceToPureDB() { } _ = sfRows.Close() for _, info := range sfs { - s.syncSharedFolderContents(info.ownerID, info.id, info.mountPath) + if err := s.syncSharedFolderContents(info.ownerID, info.id, info.mountPath); err != nil { + slog.Error("Geteilte Ordner abgleichen fehlgeschlagen", "folder", info.id, "error", err) + } } } } + +// sqlStmt is one statement of a change with several steps (FLAW-14). +type sqlStmt struct { + query string + args []interface{} +} + +func stmt(query string, args ...interface{}) sqlStmt { return sqlStmt{query, args} } + +// execInTx runs all statements in one transaction: either every step is stored or none, and the +// first error is returned instead of being ignored. +func (s *Store) execInTx(stmts ...sqlStmt) error { + if len(stmts) == 0 { + return nil + } + tx, err := s.db.Begin() + if err != nil { + return err + } + defer tx.Rollback() + for _, st := range stmts { + if _, err := tx.Exec(st.query, st.args...); err != nil { + return err + } + } + return tx.Commit() +} diff --git a/internal/storage/tasklists.go b/internal/storage/tasklists.go index 9eb2f7e..aa15319 100644 --- a/internal/storage/tasklists.go +++ b/internal/storage/tasklists.go @@ -4,6 +4,7 @@ import ( "encoding/json" "errors" "fmt" + "log/slog" "strings" "github.com/sandrodach/mindsift/internal/domain" @@ -52,7 +53,9 @@ func (s *Store) CreateTaskList(userID, spaceID, teamID, title, icon, color strin return nil, err } if teamID != "" && s.CanShareWith(userID, "team", teamID) { - _, _ = s.AddTaskListShare(tl.ID, "team", teamID, "editor", userID) + if _, err := s.AddTaskListShare(tl.ID, "team", teamID, "editor", userID); err != nil { + return nil, err + } } return s.GetTaskListByID(userID, tl.ID) } @@ -323,7 +326,7 @@ func (s *Store) SetTaskAssignees(userID, taskID string, assignees []string) ([]s // left the team, team deleted, task moved to another list or out of any list). Called after each change // that can take access away, so assignments never outlive permissions. func (s *Store) PruneTaskAssignees() { - _, _ = s.db.Exec( + _, err := s.db.Exec( `DELETE FROM task_assignees WHERE rowid IN ( SELECT ta.rowid FROM task_assignees ta LEFT JOIN thoughts t ON t.id = ta.task_id @@ -331,6 +334,9 @@ func (s *Store) PruneTaskAssignees() { OR ta.user_id NOT IN (` + fmt.Sprintf(taskListAccessSQL, "t.task_list_id") + `) )`, ) + if err != nil { + slog.Error("Zuweisungen bereinigen fehlgeschlagen", "error", err) + } } // GetTaskListUserIDs returns every user who can see the task list: owner, direct shares and members of shared teams. @@ -533,9 +539,18 @@ func (s *Store) UpdateTaskList(userID, listID string, title, icon, color, spaceI } func (s *Store) DeleteTaskList(userID, listID string) error { - _, _ = s.db.Exec(`UPDATE thoughts SET task_list_id = '' WHERE task_list_id = ?`, listID) - _, _ = s.db.Exec(`DELETE FROM task_list_shares WHERE task_list_id = ?`, listID) - _, err := s.db.Exec(`DELETE FROM task_lists WHERE id = ? AND user_id = ?`, listID, userID) + var owner string + if err := s.db.QueryRow(`SELECT user_id FROM task_lists WHERE id = ?`, listID).Scan(&owner); err != nil || owner != userID { + return errors.New("Aufgabenliste nicht gefunden") + } + // The tasks stay (as personal tasks of their owners), shares and the list go – all or nothing + if err := s.execInTx( + stmt(`UPDATE thoughts SET task_list_id = '' WHERE task_list_id = ?`, listID), + stmt(`DELETE FROM task_list_shares WHERE task_list_id = ?`, listID), + stmt(`DELETE FROM task_lists WHERE id = ? AND user_id = ?`, listID, userID), + ); err != nil { + return err + } s.PruneTaskAssignees() - return err + return nil } diff --git a/internal/storage/thoughts.go b/internal/storage/thoughts.go index 3b8056f..7c20779 100644 --- a/internal/storage/thoughts.go +++ b/internal/storage/thoughts.go @@ -485,7 +485,9 @@ func (s *Store) UpdateThoughtDetails(userID, thoughtID string, content *string, } // A new due date or a reopened task gets its alarm again if thought.DueDate != oldDue || (wasCompleted && !thought.IsCompleted) { - _, _ = s.db.Exec(`UPDATE thoughts SET alarm_sent = 0, alarm_failures = 0 WHERE id = ?`, thoughtID) + if _, err := s.db.Exec(`UPDATE thoughts SET alarm_sent = 0, alarm_failures = 0 WHERE id = ?`, thoughtID); err != nil { + return nil, err + } thought.AlarmSent = false } if taskListID != nil { @@ -538,8 +540,10 @@ func (s *Store) CreateTaskFromDocBlock(userID, docID, blockID, content string, c doneInt = 1 t.KanbanStatus = "done" } - _, _ = s.db.Exec(`UPDATE thoughts SET converted_type = 'document_block', converted_ref_id = ?, is_completed = ?, kanban_status = ?, status = 'kept' WHERE id = ?`, - t.ConvertedRefID, doneInt, t.KanbanStatus, t.ID) + if _, err := s.db.Exec(`UPDATE thoughts SET converted_type = 'document_block', converted_ref_id = ?, is_completed = ?, kanban_status = ?, status = 'kept' WHERE id = ?`, + t.ConvertedRefID, doneInt, t.KanbanStatus, t.ID); err != nil { + return nil, err + } s.SetBlockChecked(userID, docID, blockID, checked) return t, nil @@ -561,11 +565,16 @@ func (s *Store) SyncDocBlockAndTask(userID, taskID string, isCompleted bool) str // UnlinkTaskFromDocBlock removes the link between a task and its note block; both stay. The block's // task ID is derived from this link (docTaskLinks), so nothing in the note itself needs to change. -func (s *Store) UnlinkTaskFromDocBlock(userID, taskID string) { - if t, err := s.GetThoughtByID(userID, taskID); err != nil || t == nil { - return +func (s *Store) UnlinkTaskFromDocBlock(userID, taskID string) error { + t, err := s.GetThoughtByID(userID, taskID) + if err != nil || t == nil { + return err } - _, _ = s.db.Exec(`UPDATE thoughts SET converted_type = '', converted_ref_id = '' WHERE id = ?`, taskID) + if err := s.ensureTaskListWritable(userID, t.TaskListID); err != nil { + return err + } + _, err = s.db.Exec(`UPDATE thoughts SET converted_type = '', converted_ref_id = '' WHERE id = ?`, taskID) + return err } func (s *Store) SyncTaskFromBlockCheck(userID, taskID string, checked bool) { diff --git a/internal/storage/trash.go b/internal/storage/trash.go index 8a25289..84d037e 100644 --- a/internal/storage/trash.go +++ b/internal/storage/trash.go @@ -30,13 +30,15 @@ type TrashedDocument struct { const trashScope = `is_archived = 1 AND (user_id = ? OR archived_by = ?)` -func (s *Store) trashDocument(userID, docID string) error { - _, err := s.db.Exec(`UPDATE documents SET is_archived = 1, archived_at = ?, archived_by = ? WHERE id = ?`, - domain.NowISO(), userID, docID) - if err == nil { - _, _ = s.db.Exec(`DELETE FROM doc_pins WHERE doc_id = ?`, docID) +func trashStmts(userID, docID string) []sqlStmt { + return []sqlStmt{ + stmt(`UPDATE documents SET is_archived = 1, archived_at = ?, archived_by = ? WHERE id = ?`, domain.NowISO(), userID, docID), + stmt(`DELETE FROM doc_pins WHERE doc_id = ?`, docID), } - return err +} + +func (s *Store) trashDocument(userID, docID string) error { + return s.execInTx(trashStmts(userID, docID)...) } // GetTrash lists the notes in the user's trash, newest deletion first. @@ -90,10 +92,14 @@ func (s *Store) RestoreDocument(userID, docID string) error { var exists int _ = s.db.QueryRow(`SELECT COUNT(*) FROM workspace_shared_folders WHERE id = ?`, sharedFolderID).Scan(&exists) if exists == 0 { - _, _ = s.db.Exec(`UPDATE documents SET shared_folder_id = '', sub_folder_path = '', folder_path = '' WHERE id = ?`, docID) + if _, err := s.db.Exec(`UPDATE documents SET shared_folder_id = '', sub_folder_path = '', folder_path = '' WHERE id = ?`, docID); err != nil { + return err + } } } else if folderPath != "" { - _, _ = s.CreateFolder(owner, folderPath) + if _, err := s.CreateFolder(owner, folderPath); err != nil { + return err + } } _, err = s.db.Exec(`UPDATE documents SET is_archived = 0, archived_at = '', archived_by = '' WHERE id = ?`, docID) return err diff --git a/internal/storage/users.go b/internal/storage/users.go index 0bf2dc4..0196c07 100644 --- a/internal/storage/users.go +++ b/internal/storage/users.go @@ -302,12 +302,17 @@ func (s *Store) DeleteUser(userID string) error { if err := s.PurgeUserData(userID); err != nil { return err } - _, _ = s.db.Exec(`DELETE FROM revoked_tokens WHERE user_id = ?`, userID) - _, _ = s.db.Exec(`DELETE FROM task_assignees WHERE user_id = ?`, userID) - _, err := s.db.Exec(`DELETE FROM users WHERE id = ?`, userID) + if err := s.execInTx( + stmt(`DELETE FROM revoked_tokens WHERE user_id = ?`, userID), + stmt(`DELETE FROM task_assignees WHERE user_id = ?`, userID), + stmt(`DELETE FROM password_reset_tokens WHERE user_id = ?`, userID), + stmt(`DELETE FROM users WHERE id = ?`, userID), + ); err != nil { + return err + } // Shares pointing at the deleted account are no longer visible to anyone s.pruneSharingNow() - return err + return nil } // RevokeToken adds an individual JWT to the blacklist until its expiration date diff --git a/internal/ws/hub.go b/internal/ws/hub.go index 0d2cfbd..c40bbec 100644 --- a/internal/ws/hub.go +++ b/internal/ws/hub.go @@ -319,8 +319,6 @@ func (h *Hub) LeaveSSERoomByID(sseID, userID, roomID string) bool { return false } - - // Close gracefully disconnects all WebSocket and SSE connections so HTTP shutdown completes immediately. func (h *Hub) Close() { h.mu.Lock() @@ -558,11 +556,11 @@ func (h *Hub) ServeWS(w http.ResponseWriter, r *http.Request, roomID, userID str } client := &Client{ - hub: h, - conn: conn, - roomID: roomID, - userID: userID, - send: make(chan []byte, 128), + hub: h, + conn: conn, + roomID: roomID, + userID: userID, + send: make(chan []byte, 128), } h.Register(client)