diff --git a/cmd/td-sync/admin.go b/cmd/td-sync/admin.go index 93bed090..466a4389 100644 --- a/cmd/td-sync/admin.go +++ b/cmd/td-sync/admin.go @@ -44,7 +44,7 @@ func printAdminUsage() { fmt.Fprintln(os.Stderr, `Usage: td-sync admin [flags] Commands: - create-user Create a user (idempotent; stop the server first) + create-user Create a user (idempotent) approve-login Approve a pending td auth login without email grant Grant admin privileges to a user revoke Revoke admin privileges from a user @@ -68,8 +68,8 @@ func openDB(dbPath string) *serverdb.ServerDB { // runAdminCreateUser provisions a user by email, for self-hosted servers and // dev/test setup (e.g. the e2e harness), where the non-enumerating device-login // flow requires the user to already exist. It is idempotent: if the user already -// exists it prints the existing ID and exits 0. Run it while the server is NOT -// holding the DB open (the e2e harness provisions users before server start). +// exists it prints the existing ID and exits 0. Safe to run while the server +// is up. func runAdminCreateUser(args []string) { fs := flag.NewFlagSet("admin create-user", flag.ExitOnError) email := fs.String("email", "", "user email address") diff --git a/cmd/td-sync/admin_test.go b/cmd/td-sync/admin_test.go index ee986224..fe025d5c 100644 --- a/cmd/td-sync/admin_test.go +++ b/cmd/td-sync/admin_test.go @@ -217,70 +217,126 @@ func TestAdminApproveLogin_SinglePendingNeedsConfirmation(t *testing.T) { } } -// TestAdminApproveLogin_WhileServerWrites runs approve-login as a separate -// process while this process holds the database open and keeps writing to it, -// as a running td-sync server would. A real server writes server.db once or -// twice per request; this writer commits hundreds of times a second but, like -// any real server, leaves the lock free between writes. A writer that never -// releases the lock can starve any second process past the busy timeout. -func TestAdminApproveLogin_WhileServerWrites(t *testing.T) { - store, dbPath := openServerStore(t) - - // create-user runs before the load starts: its transaction reads before it - // writes, so a concurrent writer can make it fail with SQLITE_BUSY. The - // docs tell operators to run it while the server is stopped. - stdout, stderr, code := runAdminProcess(t, "create-user", "--email", "live@example.com", "--db", dbPath) - if code != 0 || !strings.Contains(stdout, "created user live@example.com") { - t.Fatalf("create-user: code=%d stdout=%q stderr=%q", code, stdout, stderr) - } - u, err := store.GetUserByEmail("live@example.com") - if err != nil || u == nil { - t.Fatalf("server handle cannot see created user: %v", err) - } - selector := startDeviceLogin(t, store, u, 3) +// serverWriter keeps writing to server.db from this process, as a running +// td-sync server would, while a test runs admin commands as separate +// processes. A real server writes server.db once or twice per request; this +// writer commits hundreds of times a second but, like any real server, leaves +// the lock free between writes. A writer that never releases the lock can +// starve any second process past the busy timeout. +type serverWriter struct { + writes atomic.Int64 + writeErr atomic.Value + stopCh chan struct{} + once sync.Once + wg sync.WaitGroup +} - stop := make(chan struct{}) - var wg sync.WaitGroup - var writes atomic.Int64 - var writeErr atomic.Value - wg.Add(1) +func startServerWriter(t *testing.T, store *serverdb.ServerDB) *serverWriter { + t.Helper() + w := &serverWriter{stopCh: make(chan struct{})} + w.wg.Add(1) go func() { - defer wg.Done() + defer w.wg.Done() for { select { - case <-stop: + case <-w.stopCh: return default: } if err := store.InsertAuthEvent("load", "load@example.com", serverdb.AuthEventStarted, "{}"); err != nil { - writeErr.Store(err.Error()) + w.writeErr.Store(err.Error()) return } - writes.Add(1) + w.writes.Add(1) time.Sleep(time.Millisecond) } }() - var once sync.Once - stopWriter := func() { - once.Do(func() { close(stop) }) - wg.Wait() + t.Cleanup(w.stop) + return w +} + +func (w *serverWriter) stop() { + w.once.Do(func() { close(w.stopCh) }) + w.wg.Wait() +} + +// runWhileWriting runs an admin command as a separate process while w writes, +// and fails the test if the writer made no progress during it. +func (w *serverWriter) runWhileWriting(t *testing.T, args ...string) (stdout, stderr string, code int) { + t.Helper() + before, began := w.writes.Load(), time.Now() + stdout, stderr, code = runAdminProcess(t, args...) + t.Logf("server committed %d writes during the %v %s process", w.writes.Load()-before, time.Since(began).Round(time.Millisecond), args[0]) + if w.writes.Load() == before { + t.Fatalf("server writer made no progress while %s ran", args[0]) } - t.Cleanup(stopWriter) + return stdout, stderr, code +} - before, began := writes.Load(), time.Now() - stdout, stderr, code = runAdminProcess(t, "approve-login", "--email", "live@example.com", "--yes", "--db", dbPath) - t.Logf("server committed %d writes during the %v approve-login process", writes.Load()-before, time.Since(began).Round(time.Millisecond)) - if code != 0 || !strings.Contains(stdout, "selector: "+selector) { - t.Fatalf("approve-login while server writes: code=%d stdout=%q stderr=%q", code, stdout, stderr) +// stopAndCheck stops the writer and fails the test if any of its writes failed. +func (w *serverWriter) stopAndCheck(t *testing.T) { + t.Helper() + w.stop() + if v := w.writeErr.Load(); v != nil { + t.Fatalf("server write failed during the admin command: %v", v) + } +} + +// TestAdminCreateUser_WhileServerWrites runs create-user as separate processes +// while this process keeps writing to the database. A single run hits the +// window between its read and its write only some of the time, so it creates +// several users, then reruns one to check that it stays idempotent. +func TestAdminCreateUser_WhileServerWrites(t *testing.T) { + store, dbPath := openServerStore(t) + w := startServerWriter(t, store) + + const users = 8 + for i := range users { + email := fmt.Sprintf("live%d@example.com", i) + stdout, stderr, code := w.runWhileWriting(t, "create-user", "--email", email, "--db", dbPath) + if code != 0 || !strings.Contains(stdout, "created user "+email) { + t.Fatalf("create-user %s while server writes: code=%d stdout=%q stderr=%q", email, code, stdout, stderr) + } + } + stdout, stderr, code := w.runWhileWriting(t, "create-user", "--email", "live0@example.com", "--db", dbPath) + if code != 0 || !strings.Contains(stdout, "user already exists: live0@example.com") { + t.Fatalf("repeat create-user while server writes: code=%d stdout=%q stderr=%q", code, stdout, stderr) + } + + w.stopAndCheck(t) + for i := range users { + u, err := store.GetUserByEmail(fmt.Sprintf("live%d@example.com", i)) + if err != nil || u == nil { + t.Fatalf("server handle cannot see user %d: %v", i, err) + } + if u.IsAdmin != (i == 0) { + t.Fatalf("user %d: is_admin=%v, want only the first user to be admin", i, u.IsAdmin) + } + } +} + +// TestAdminApproveLogin_WhileServerWrites runs approve-login as a separate +// process while this process keeps writing to the database. +func TestAdminApproveLogin_WhileServerWrites(t *testing.T) { + store, dbPath := openServerStore(t) + + stdout, stderr, code := runAdminProcess(t, "create-user", "--email", "live@example.com", "--db", dbPath) + if code != 0 || !strings.Contains(stdout, "created user live@example.com") { + t.Fatalf("create-user: code=%d stdout=%q stderr=%q", code, stdout, stderr) } - if writes.Load() == before { - t.Fatal("server writer made no progress while approve-login ran") + u, err := store.GetUserByEmail("live@example.com") + if err != nil || u == nil { + t.Fatalf("server handle cannot see created user: %v", err) } + selector := startDeviceLogin(t, store, u, 3) - stopWriter() - if v := writeErr.Load(); v != nil { - t.Fatalf("server write failed while approve-login ran: %v", v) + w := startServerWriter(t, store) + stdout, stderr, code = w.runWhileWriting(t, "approve-login", "--email", "live@example.com", "--yes", "--db", dbPath) + if code != 0 || !strings.Contains(stdout, "selector: "+selector) { + t.Fatalf("approve-login while server writes: code=%d stdout=%q stderr=%q", code, stdout, stderr) } + + w.stopAndCheck(t) if got := challengeStatus(t, store, selector); got != serverdb.ChallengeStatusVerified { t.Fatalf("server handle sees status %s, want verified", got) } diff --git a/docs/sync-server-ops-guide.md b/docs/sync-server-ops-guide.md index 25759459..d99dd238 100644 --- a/docs/sync-server-ops-guide.md +++ b/docs/sync-server-ops-guide.md @@ -164,13 +164,13 @@ SYNC_EMAIL_PROVIDER=memory SYNC_DEV_EMAIL_INSPECT=1 ./td-sync A private server, such as one on a home network, can run without an email provider. Leave `SYNC_EMAIL_PROVIDER` unset (the `log` provider) and approve each login from the server's shell instead of an emailed link. Pass `--db` to the admin commands, or set `SYNC_SERVER_DB_PATH`, when the database is not at `./data/server.db`. -1. Create your account while the server is stopped. `td auth login` never creates accounts. +1. Create your account. `td auth login` never creates accounts. ```bash td-sync admin create-user --email you@example.com ``` -2. Start the server. On the client, run `td auth login` and enter the same email; it waits for approval. +2. Start the server if it is not running. On the client, run `td auth login` and enter the same email; it waits for approval. 3. On the server, review the waiting login and approve it: @@ -199,7 +199,7 @@ A private server, such as one on a home network, can run without an email provid `approve-login` needs write access to `server.db`, which `create-key` and `grant` already require, so it gives nothing to anyone without a shell on the server. The client must still present the PKCE verifier from the login it started. The approval is recorded as a `device_verified` auth event with `{"approved_by":"operator_cli"}` and does not mark the email address verified. -`approve-login` is safe to run while the server is up. If a very busy server holds the database lock past the 5-second busy timeout, the command exits with `database is locked` and changes nothing; run it again. `create-user` is less tolerant of concurrent writes, so run it while the server is stopped. +`create-user` and `approve-login` are safe to run while the server is up. If a very busy server holds the database lock past the 5-second busy timeout, the command exits with `database is locked` and changes nothing; run it again. ## Docker Deployment diff --git a/internal/serverdb/users.go b/internal/serverdb/users.go index 7dc45b81..468652e4 100644 --- a/internal/serverdb/users.go +++ b/internal/serverdb/users.go @@ -1,6 +1,7 @@ package serverdb import ( + "context" "database/sql" "fmt" "strings" @@ -19,6 +20,13 @@ type User struct { // CreateUser inserts a new user with the given email (lowercased). // The first user created is automatically made an admin. +// +// The count and the insert share one transaction so two concurrent creations +// cannot both become admin. It starts with BEGIN IMMEDIATE so the write lock +// is taken up front under the busy timeout, which makes `td-sync admin +// create-user` safe to run while a td-sync server holds the same file. A +// deferred transaction that reads first gets SQLITE_BUSY at once, without +// waiting, if another connection writes in between. func (db *ServerDB) CreateUser(email string) (*User, error) { email = strings.ToLower(strings.TrimSpace(email)) if email == "" { @@ -30,23 +38,32 @@ func (db *ServerDB) CreateUser(email string) (*User, error) { return nil, fmt.Errorf("generate user id: %w", err) } - // Use a transaction to atomically check count and insert - // to prevent TOCTOU race where two concurrent requests both become admin - tx, err := db.conn.Begin() + ctx := context.Background() + conn, err := db.conn.Conn(ctx) if err != nil { + return nil, fmt.Errorf("get connection: %w", err) + } + defer func() { _ = conn.Close() }() + + if _, err := conn.ExecContext(ctx, "BEGIN IMMEDIATE"); err != nil { return nil, fmt.Errorf("begin tx: %w", err) } - defer func() { _ = tx.Rollback() }() + committed := false + defer func() { + if !committed { + _, _ = conn.ExecContext(ctx, "ROLLBACK") + } + }() // First user becomes admin var count int - if err := tx.QueryRow("SELECT COUNT(*) FROM users").Scan(&count); err != nil { + if err := conn.QueryRowContext(ctx, "SELECT COUNT(*) FROM users").Scan(&count); err != nil { return nil, fmt.Errorf("count users: %w", err) } isAdmin := count == 0 now := time.Now().UTC() - _, err = tx.Exec( + _, err = conn.ExecContext(ctx, `INSERT INTO users (id, email, is_admin, created_at, updated_at) VALUES (?, ?, ?, ?, ?)`, id, email, isAdmin, now, now, ) @@ -54,9 +71,10 @@ func (db *ServerDB) CreateUser(email string) (*User, error) { return nil, fmt.Errorf("insert user: %w", err) } - if err := tx.Commit(); err != nil { + if _, err := conn.ExecContext(ctx, "COMMIT"); err != nil { return nil, fmt.Errorf("commit: %w", err) } + committed = true return &User{ID: id, Email: email, IsAdmin: isAdmin, CreatedAt: now, UpdatedAt: now}, nil }