diff --git a/BUILD_PLAN.md b/BUILD_PLAN.md index 359b081..d05e470 100644 --- a/BUILD_PLAN.md +++ b/BUILD_PLAN.md @@ -178,10 +178,12 @@ Option B ratified. `go-oidc` + `x/oauth2`; config fields already existed. The `R - **Two bugs deploying caught that the whole test suite could not**, both fatal before the login page ever renders: (1) the code trimmed the issuer's **trailing slash**, and Authentik's issuer has one — OIDC requires a byte-for-byte match, so discovery failed every time while the stub IdP (which advertised a slashless issuer) kept passing. Fixed, and the stub's issuer is now a knob with a regression test that ends in a slash. (2) A provider created through `ak shell` rather than the admin UI comes up with **`grant_types = []`**, which authentik reads as "no grant type is permitted here" and answers with `invalid_request` / *The request is otherwise malformed*. Both are written up in `deploy/README.md` §4. - **Allowlist is currently `prosolis@proton.me` only.** That Authentik instance fronts ~40 accounts across several applications, so an empty list was not an option, and guessing which account is hers would either lock her out or let a stranger in. Adding her is one line in `.env` plus a restart. Note that an Authentik account with **no email set** (e.g. `akadmin`) can't match an email-based entry — use its subject id. -### Phase 17 — Migrate the `local` user -Script, app stopped, backup first (OPEN #4). Depends on: she logs in once so her OIDC `sub` exists. +### Phase 17 — Migrate the `local` user ✅ script (2026-07-27) +Script, app stopped, backup first (OPEN #4). **The "she logs in once first" dependency turned out not to exist**: authentik's default `hashed_user_id` sub mode makes the subject `User.uid`, which is derived from her user id and the instance secret — stable, and readable before she has ever signed in (`ak shell -c "…User.objects.get(username='claire').uid"`). So the data can move *first*, and she signs in to find her writing already there rather than to an empty Petal that fills in later. - [ ] `scripts/migrate_local_user.*`: single transaction, `PRAGMA foreign_keys=OFF`, re-point `documents`/`tags`/`vocab_words` **and `images`** (versions/suggestions follow parents; `images` is new in Phase 16 and carries `user_id` directly — miss it and every pasted picture 404s), delete the empty provisioned row, verify row counts before commit; refuses to run if the app is up or the target has data -- [ ] Runbook documented in the script header; dry-run mode +- [x] `scripts/migrate_local_user.py` — dry-run by default, `VACUUM INTO` backup before touching anything, one transaction with `PRAGMA foreign_keys=OFF`, re-points `documents`/`tags`/`vocab_words`/`images`, deletes the old user row, and **verifies every expected row actually moved (and that the source is left owning nothing) before it commits**, rolling back otherwise. Refuses to merge into an account that already owns writing. Runbook in the script header. +- **The "is the app stopped?" guard needed a second attempt.** `BEGIN EXCLUSIVE` — the obvious check — passes straight through against a *running but idle* Petal, because in WAL mode it only conflicts with another writer. That is exactly the case the guard exists to catch, and it would have failed silently. `PRAGMA locking_mode = EXCLUSIVE` conflicts with any connection at all, since it locks the shared-memory index every WAL reader maps; verified against a live server. +- **Startup crash averted while sequencing this**: the image backfill claims unowned files for `local`, which stops existing after the migration — a foreign-key error inside `images.New`, which `main.go` treats as fatal. Petal would have entered a crash loop the first time it started on a migrated database. The backfill now skips a missing owner (there is nothing to claim in that case anyway; the migration moves the image rows itself). ### Phase 18 — Per-user, per-language client state - [ ] Namespace `localStorage` keys by user id once known: `petal.spell.personal`, `petal.companion`, sound/petals prefs diff --git a/internal/images/handler.go b/internal/images/handler.go index 921fc2a..b9d184a 100644 --- a/internal/images/handler.go +++ b/internal/images/handler.go @@ -77,6 +77,21 @@ func (h *Handler) backfill(owner string) error { if owner == "" { return nil } + // The owner may not exist — after the `local` account has been migrated onto + // a real one, it doesn't. Claiming for a missing user would violate the + // foreign key, and this runs during startup, so the error would take the + // whole app down. There is nothing left to claim in that case anyway: the + // migration moves the image rows along with everything else. + var ownerExists bool + if err := h.db.QueryRow( + `SELECT EXISTS(SELECT 1 FROM users WHERE id = ?)`, owner, + ).Scan(&ownerExists); err != nil { + return err + } + if !ownerExists { + return nil + } + entries, err := os.ReadDir(h.dir) if err != nil { return err diff --git a/internal/images/handler_test.go b/internal/images/handler_test.go index 426ef39..6132bf2 100644 --- a/internal/images/handler_test.go +++ b/internal/images/handler_test.go @@ -209,6 +209,13 @@ func TestBackfillClaimsExistingFiles(t *testing.T) { if _, err := New(dir, database.DB, "bob"); err != nil { t.Fatalf("second backfill: %v", err) } + + // And an owner who no longer exists — which is what the `local` account + // becomes once it has been migrated onto a real one — must be skipped, not + // turned into a foreign-key error that takes startup down with it. + if _, err := New(dir, database.DB, "nobody-at-all"); err != nil { + t.Fatalf("backfill for a missing owner should be a no-op, got: %v", err) + } var owners int if err := database.QueryRow(`SELECT COUNT(*) FROM images WHERE name = ?`, orphan).Scan(&owners); err != nil { t.Fatal(err) diff --git a/scripts/migrate_local_user.py b/scripts/migrate_local_user.py new file mode 100755 index 0000000..373c248 --- /dev/null +++ b/scripts/migrate_local_user.py @@ -0,0 +1,222 @@ +#!/usr/bin/env python3 +"""Move the pre-auth `local` user's writing onto a real account. + +Petal ran as a single hardcoded user (`users.id = 'local'`) for its whole life +before sign-in existed. Everything she has written is owned by that row. This +script re-points it at the account she now signs in as. + +Why a script and not a startup migration: the destination is an OIDC subject +id, which is not knowable from inside the app — it belongs to the identity +provider. Running it deliberately, with the app stopped and a backup taken, is +also the only way to be sure nothing is writing to the database halfway through. + +What moves: `documents`, `tags`, `vocab_words` and `images` carry `user_id` +directly. `document_versions`, `suggestions` and `document_tags` hang off their +parents and follow without being touched — which is exactly why this must be one +transaction with foreign keys off: re-pointing a parent while its children are +enforced would either fail or cascade. + +Sessions belonging to the old identity are deleted rather than moved. A session +is proof that *someone signed in*, and nobody ever signed in as `local`. + +Safety: dry-run unless --apply; refuses to run while anything else has the +database open; refuses if the destination already owns writing of its own; takes +a `VACUUM INTO` backup first; and verifies every row it expected to move +actually moved before it commits. + +Usage: + # look at what would happen + python3 scripts/migrate_local_user.py data/petal.db --to + + # do it + python3 scripts/migrate_local_user.py data/petal.db --to \\ + --email her@example.com --name "Her Name" --apply + +The subject id comes from the identity provider. For authentik with the default +`hashed_user_id` sub mode it is the user's `uid`, which is stable and knowable +before she has ever logged in: + + docker exec authentik-server-1 ak shell -c \\ + "from authentik.core.models import User; print(User.objects.get(username='claire').uid)" +""" +import argparse +import os +import sqlite3 +import sys +import time + +# The tables that name an owner directly. Everything else in the schema reaches +# its owner through one of these. +OWNED_TABLES = ("documents", "tags", "vocab_words", "images") + +LOCAL_USER = "local" + + +def die(msg: str) -> None: + print(f"error: {msg}", file=sys.stderr) + sys.exit(1) + + +def open_db(path: str) -> sqlite3.Connection: + if not os.path.exists(path): + die(f"no database at {path}") + conn = sqlite3.connect(path, isolation_level=None) + conn.row_factory = sqlite3.Row + return conn + + +def table_exists(conn: sqlite3.Connection, name: str) -> bool: + row = conn.execute( + "SELECT 1 FROM sqlite_master WHERE type='table' AND name=?", (name,) + ).fetchone() + return row is not None + + +def counts(conn: sqlite3.Connection, user_id: str) -> dict[str, int]: + out = {} + for table in OWNED_TABLES: + if not table_exists(conn, table): + continue + out[table] = conn.execute( + f"SELECT COUNT(*) FROM {table} WHERE user_id = ?", (user_id,) + ).fetchone()[0] + return out + + +def require_app_stopped(conn: sqlite3.Connection, path: str) -> None: + """Fail unless nothing else has the database open. + + `BEGIN EXCLUSIVE` is not enough, and quietly so: in WAL mode it only + conflicts with another *writer*, so an idle-but-running Petal sails straight + past it — which is precisely the case this guard exists to catch. Taking + `locking_mode = EXCLUSIVE` conflicts with any other connection at all, + because it locks the shared-memory index every WAL reader must map. + + Checking for a `-wal` file would be no use either: WAL mode leaves one + behind whether or not anything is running. + """ + try: + conn.execute("PRAGMA locking_mode = EXCLUSIVE") + conn.execute("BEGIN IMMEDIATE") + conn.execute("COMMIT") + except sqlite3.OperationalError as err: + die( + f"{path} is in use ({err}). Stop Petal first:\n" + " docker compose stop petal # VPS\n" + " systemctl --user stop petal # millenia" + ) + finally: + # Back to normal locking so the rest of the run behaves like any other + # client, and so a dry run leaves the file exactly as it found it. + conn.execute("PRAGMA locking_mode = NORMAL") + + +def backup(path: str) -> str: + """Snapshot the database with VACUUM INTO — one coherent file including the + WAL, taken without a write lock, and it refuses to overwrite.""" + dest = f"{path}.pre-migrate-{time.strftime('%Y%m%d-%H%M%S')}" + conn = sqlite3.connect(path) + try: + conn.execute("VACUUM INTO ?", (dest,)) + finally: + conn.close() + return dest + + +def main() -> None: + ap = argparse.ArgumentParser(description=__doc__.split("\n")[0]) + ap.add_argument("database", help="path to petal.db") + ap.add_argument("--to", required=True, metavar="SUB", + help="OIDC subject id of the destination account") + ap.add_argument("--from", dest="source", default=LOCAL_USER, + help=f"account to move from (default: {LOCAL_USER})") + ap.add_argument("--email", default="", help="email for the destination account, if it doesn't exist yet") + ap.add_argument("--name", default="", help="display name for the destination account") + ap.add_argument("--pair-lang", default="zh", help="language pair for a newly created account (default: zh)") + ap.add_argument("--apply", action="store_true", help="actually commit (default: dry run)") + ap.add_argument("--no-backup", action="store_true", help="skip the pre-migration snapshot") + args = ap.parse_args() + + if args.to == args.source: + die("source and destination are the same account") + + conn = open_db(args.database) + require_app_stopped(conn, args.database) + + src_counts = counts(conn, args.source) + if not any(src_counts.values()): + die(f"account {args.source!r} owns nothing in this database — wrong file, or already migrated?") + + dst_counts = counts(conn, args.to) + if any(dst_counts.values()): + die( + f"account {args.to!r} already owns writing here " + f"({', '.join(f'{k}={v}' for k, v in dst_counts.items() if v)}). " + "Refusing to merge two accounts — that is not something this script can undo." + ) + + dst = conn.execute("SELECT id, email, display_name FROM users WHERE id = ?", (args.to,)).fetchone() + + print(f"database: {args.database}") + print(f"moving from: {args.source}") + print(f"moving to: {args.to}" + ("" if dst else " (will be created)")) + for table, n in src_counts.items(): + print(f" {table:<14} {n}") + sessions = 0 + if table_exists(conn, "sessions"): + sessions = conn.execute( + "SELECT COUNT(*) FROM sessions WHERE user_id = ?", (args.source,) + ).fetchone()[0] + print(f" {'sessions':<14} {sessions} (deleted, not moved)") + + if not args.apply: + print("\ndry run — nothing changed. Re-run with --apply to commit.") + return + + if not args.no_backup: + dest = backup(args.database) + print(f"\nbackup: {dest}") + + # Foreign keys off for the duration: children reference the parents being + # re-pointed, and this is one atomic swap of an identity, not a data change. + conn.execute("PRAGMA foreign_keys = OFF") + conn.execute("BEGIN EXCLUSIVE") + try: + if not dst: + conn.execute( + "INSERT INTO users (id, email, display_name, pair_lang) VALUES (?, ?, ?, ?)", + (args.to, args.email, args.name or args.email, args.pair_lang), + ) + + for table in src_counts: + conn.execute( + f"UPDATE {table} SET user_id = ? WHERE user_id = ?", (args.to, args.source) + ) + if table_exists(conn, "sessions"): + conn.execute("DELETE FROM sessions WHERE user_id = ?", (args.source,)) + + # Verify before committing: every row that was the source's is now the + # destination's, and the source owns nothing. + moved = counts(conn, args.to) + left = counts(conn, args.source) + if moved != src_counts or any(left.values()): + raise RuntimeError( + f"row counts do not match after the move (expected {src_counts}, " + f"got {moved}, source still holds {left})" + ) + + conn.execute("DELETE FROM users WHERE id = ?", (args.source,)) + conn.execute("COMMIT") + except Exception as err: # noqa: BLE001 — any failure must roll the whole thing back + conn.execute("ROLLBACK") + die(f"migration rolled back: {err}") + finally: + conn.execute("PRAGMA foreign_keys = ON") + + print("\nmigrated. Start Petal and sign in as the destination account.") + for table, n in src_counts.items(): + print(f" {table:<14} {n}") + + +if __name__ == "__main__": + main()