From 7becb7344d259efcf1e4fc6fda3cbc5afc2b4644 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Fri, 4 Sep 2026 17:39:22 -0700 Subject: [PATCH] Stop an owner deleting the account they are signed in as Deleting your own user succeeded, and logged you out doing it: local_sessions.user_id is ON DELETE CASCADE, so the delete took the caller's own live session with it. Nothing refused this. The last-owner guard is the only thing in the path, and it passes cleanly as soon as a second owner exists -- which is exactly the state you are in just after creating one. The way back in was then whatever other account happened to exist, and -seed-admin could not help: it skipped whenever *any* local user was present, so the command documented as the way to create an administrator refused precisely when there was no usable one, because some other account still existed. It now asks whether the admin account itself is missing, which is what its own help text always claimed, and what makes it useful as recovery rather than only as first-run bootstrap. TestCanDeleteAnOwnerWhenAnotherRemains signed in as admin1 and deleted admin1, asserting 204 -- it encoded the lockout as intended behaviour. It now deletes the other owner, which is what it meant to cover, and a new test holds the refusal in place. runSeedAdmin takes a small interface so the bootstrap path is tested without a Postgres pool; it had no tests before. Signed-off-by: John Coffey --- api/cmd/api/main.go | 37 ++++++++++---- api/cmd/api/main_test.go | 96 +++++++++++++++++++++++++++++++++++ api/localauth/fake_test.go | 4 -- api/localauth/handler.go | 12 +++++ api/localauth/handler_test.go | 41 +++++++++++++-- api/localauth/store.go | 19 ++++--- 6 files changed, 185 insertions(+), 24 deletions(-) create mode 100644 api/cmd/api/main_test.go diff --git a/api/cmd/api/main.go b/api/cmd/api/main.go index 6868c90..6cf07d3 100644 --- a/api/cmd/api/main.go +++ b/api/cmd/api/main.go @@ -79,7 +79,7 @@ func main() { // startup path -- mirrors enterprise-api's -provision-tenant shape // (declare, flag.Parse(), short-circuit before the rest of main's // dependencies matter to it). See runSeedAdmin's doc comment. - seedAdmin := flag.Bool("seed-admin", false, "create the default local-auth admin user with a random password if none exists, print it once, and exit") + seedAdmin := flag.Bool("seed-admin", false, "create the default local-auth admin user with a random password if that account does not exist, print it once, and exit") flag.Parse() ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) @@ -259,21 +259,40 @@ func main() { } } +// seedAdminUsername is the account -seed-admin creates, and the one it +// checks for before deciding it has nothing to do. +const seedAdminUsername = "admin" + +// seedStore is the slice of *localauth.Store that runSeedAdmin uses, +// named here so the bootstrap path can be tested without a Postgres +// pool behind it. +type seedStore interface { + UsernameExists(ctx context.Context, username string) (bool, error) + CreateUser(ctx context.Context, username, passwordHash string, role authz.Role) (*localauth.User, error) +} + // runSeedAdmin is the operator action that bootstraps local login on a -// fresh deployment: idempotent (a no-op if any local user already +// fresh deployment: idempotent (a no-op if the admin account already // exists, safe to run on every deploy per the runbook), so there's no // separate "has this already run" flag to track. The generated // password is printed to stdout exactly once and never stored in // plaintext anywhere -- losing it means resetting it // (POST /auth/users/{id}/reset-password), not recovering it. -func runSeedAdmin(ctx context.Context, logger *slog.Logger, stdout io.Writer, store *localauth.Store) int { - n, err := store.CountLocalUsers(ctx) +// +// The check is specifically for the admin account rather than for any +// local user, which is what it used to be. That older test made this +// command useless in the situation it is most needed: an operator who +// no longer has a working administrator account, but whose deployment +// still contains other users, was told "already provisioned" and left +// with nothing to do. +func runSeedAdmin(ctx context.Context, logger *slog.Logger, stdout io.Writer, store seedStore) int { + exists, err := store.UsernameExists(ctx, seedAdminUsername) if err != nil { - logger.Error("counting local users", "error", err) + logger.Error("checking for an existing admin user", "error", err) return 1 } - if n > 0 { - fmt.Fprintln(stdout, "admin already provisioned, skipping") + if exists { + fmt.Fprintf(stdout, "%q user already exists, skipping\n", seedAdminUsername) return 0 } @@ -289,13 +308,13 @@ func runSeedAdmin(ctx context.Context, logger *slog.Logger, stdout io.Writer, st logger.Error("hashing password", "error", err) return 1 } - if _, err := store.CreateUser(ctx, "admin", hash, authz.RoleOwner); err != nil { + if _, err := store.CreateUser(ctx, seedAdminUsername, hash, authz.RoleOwner); err != nil { logger.Error("creating admin user", "error", err) return 1 } fmt.Fprintln(stdout, "created default admin user:") - fmt.Fprintln(stdout, " username: admin") + fmt.Fprintf(stdout, " username: %s\n", seedAdminUsername) fmt.Fprintf(stdout, " password: %s\n", password) fmt.Fprintln(stdout, "this password will not be shown again -- save it now.") return 0 diff --git a/api/cmd/api/main_test.go b/api/cmd/api/main_test.go new file mode 100644 index 0000000..bc6aab9 --- /dev/null +++ b/api/cmd/api/main_test.go @@ -0,0 +1,96 @@ +package main + +import ( + "bytes" + "context" + "io" + "log/slog" + "strings" + "testing" + + "github.com/cairnobs/cairnobs/api/authz" + "github.com/cairnobs/cairnobs/api/localauth" +) + +type fakeSeedStore struct { + usernames map[string]bool + created []createdUser +} + +type createdUser struct { + username string + role authz.Role +} + +func newFakeSeedStore(existing ...string) *fakeSeedStore { + f := &fakeSeedStore{usernames: map[string]bool{}} + for _, u := range existing { + f.usernames[u] = true + } + return f +} + +func (f *fakeSeedStore) UsernameExists(_ context.Context, username string) (bool, error) { + return f.usernames[username], nil +} + +func (f *fakeSeedStore) CreateUser(_ context.Context, username, _ string, role authz.Role) (*localauth.User, error) { + f.usernames[username] = true + f.created = append(f.created, createdUser{username: username, role: role}) + return &localauth.User{ID: "id-" + username, Username: username, Role: role}, nil +} + +func discardLogger() *slog.Logger { + return slog.New(slog.NewTextHandler(io.Discard, nil)) +} + +func TestSeedAdminCreatesTheAdminOnAFreshDeployment(t *testing.T) { + fs := newFakeSeedStore() + var out bytes.Buffer + + if code := runSeedAdmin(context.Background(), discardLogger(), &out, fs); code != 0 { + t.Fatalf("runSeedAdmin: exit code = %d, want 0", code) + } + if len(fs.created) != 1 || fs.created[0].username != seedAdminUsername { + t.Fatalf("created = %+v, want one %q", fs.created, seedAdminUsername) + } + if fs.created[0].role != authz.RoleOwner { + t.Fatalf("created role = %q, want %q", fs.created[0].role, authz.RoleOwner) + } + if !strings.Contains(out.String(), "password:") { + t.Fatalf("output does not print the generated password: %q", out.String()) + } +} + +func TestSeedAdminSkipsWhenTheAdminAlreadyExists(t *testing.T) { + fs := newFakeSeedStore(seedAdminUsername) + var out bytes.Buffer + + if code := runSeedAdmin(context.Background(), discardLogger(), &out, fs); code != 0 { + t.Fatalf("runSeedAdmin: exit code = %d, want 0", code) + } + if len(fs.created) != 0 { + t.Fatalf("created = %+v, want none -- a second admin must never be minted", fs.created) + } + if !strings.Contains(out.String(), "skipping") { + t.Fatalf("output does not say it skipped: %q", out.String()) + } +} + +// The recovery case this command exists for. It used to refuse here, +// because it asked whether the deployment had *any* local user rather +// than whether the admin account it creates was missing -- so an +// operator whose administrator account was gone, but whose deployment +// still held other accounts, was told "already provisioned" and left +// with no supported way back in. +func TestSeedAdminStillSeedsWhenOtherUsersExist(t *testing.T) { + fs := newFakeSeedStore("someone-else") + var out bytes.Buffer + + if code := runSeedAdmin(context.Background(), discardLogger(), &out, fs); code != 0 { + t.Fatalf("runSeedAdmin: exit code = %d, want 0", code) + } + if len(fs.created) != 1 || fs.created[0].username != seedAdminUsername { + t.Fatalf("created = %+v, want one %q", fs.created, seedAdminUsername) + } +} diff --git a/api/localauth/fake_test.go b/api/localauth/fake_test.go index beab9c1..a3813a8 100644 --- a/api/localauth/fake_test.go +++ b/api/localauth/fake_test.go @@ -129,10 +129,6 @@ func (f *fakeStore) SetDisplayTimezone(_ context.Context, userID, tz string) err return nil } -func (f *fakeStore) CountLocalUsers(_ context.Context) (int, error) { - return len(f.users), nil -} - func (f *fakeStore) CountUsersWithRole(_ context.Context, role authz.Role) (int, error) { n := 0 for _, u := range f.users { diff --git a/api/localauth/handler.go b/api/localauth/handler.go index e28d759..646db21 100644 --- a/api/localauth/handler.go +++ b/api/localauth/handler.go @@ -329,6 +329,18 @@ func (h *Handler) handleCreateUser(w http.ResponseWriter, r *http.Request) { // owner-only -- and anyone at all deleting the last remaining owner. func (h *Handler) handleDeleteUser(w http.ResponseWriter, r *http.Request) { id := r.PathValue("id") + // Deleting yourself is refused before anything else, including the + // last-owner check below -- local_sessions.user_id is ON DELETE + // CASCADE, so a successful self-delete destroys the caller's own + // live session as a side effect, logging them out mid-request with + // no warning. With a second owner present the last-owner guard + // passes cleanly, so nothing else here would have stopped it, and + // the way back in is whatever other account happens to exist. Same + // posture as handleResetPassword's self-target refusal above. + if identity, ok := authz.IdentityFromContext(r.Context()); ok && id == identity.UserID { + writeError(w, http.StatusConflict, "cannot delete the account you are signed in as") + return + } target, err := h.store.GetUserByID(r.Context(), id) if err != nil { h.writeStoreErr(w, err, "deleting user") diff --git a/api/localauth/handler_test.go b/api/localauth/handler_test.go index 089e660..7ba2b6d 100644 --- a/api/localauth/handler_test.go +++ b/api/localauth/handler_test.go @@ -543,16 +543,49 @@ func TestCannotDeleteTheLastOwner(t *testing.T) { func TestCanDeleteAnOwnerWhenAnotherRemains(t *testing.T) { fs := newFakeStore() - owner1 := mustCreateUser(t, fs, "admin1", "adminpass1", authz.RoleOwner) + mustCreateUser(t, fs, "admin1", "adminpass1", authz.RoleOwner) + // The caller deletes the *other* owner, not itself. This test used + // to sign in as admin1 and delete admin1, which passed only because + // self-deletion was unguarded -- it asserted the lockout as if it + // were the intended behaviour. What it means to test is that the + // last-owner guard doesn't fire while a second owner remains, and + // that holds without deleting the caller. + owner2 := mustCreateUser(t, fs, "admin2", "adminpass2", authz.RoleOwner) + _, mux := newTestHandler(t, fs) + + login := doRequest(t, mux, http.MethodPost, "/auth/login", `{"username":"admin1","password":"adminpass1"}`, nil) + cookie := sessionCookieFrom(login) + + rec := doRequest(t, mux, http.MethodDelete, "/auth/users/"+owner2.ID, "", cookie) + if rec.Code != http.StatusNoContent { + t.Fatalf("deleting one of two owners: status = %d, want 204, body=%s", rec.Code, rec.Body.String()) + } +} + +// The lockout this guards against: an owner creates a second owner, +// deletes their own account, and is signed out by the resulting +// local_sessions cascade with no supported way back in unless they +// already know the other account's password. +func TestCannotDeleteYourOwnAccount(t *testing.T) { + fs := newFakeStore() + owner := mustCreateUser(t, fs, "admin1", "adminpass1", authz.RoleOwner) + // A second owner exists, so the last-owner guard is satisfied and + // cannot be what refuses this. mustCreateUser(t, fs, "admin2", "adminpass2", authz.RoleOwner) _, mux := newTestHandler(t, fs) login := doRequest(t, mux, http.MethodPost, "/auth/login", `{"username":"admin1","password":"adminpass1"}`, nil) cookie := sessionCookieFrom(login) - rec := doRequest(t, mux, http.MethodDelete, "/auth/users/"+owner1.ID, "", cookie) - if rec.Code != http.StatusNoContent { - t.Fatalf("deleting one of two owners: status = %d, want 204, body=%s", rec.Code, rec.Body.String()) + rec := doRequest(t, mux, http.MethodDelete, "/auth/users/"+owner.ID, "", cookie) + if rec.Code != http.StatusConflict { + t.Fatalf("deleting your own account: status = %d, want 409, body=%s", rec.Code, rec.Body.String()) + } + + // Still signed in, and the account still exists. + sess := doRequest(t, mux, http.MethodGet, "/auth/session", "", cookie) + if sess.Code != http.StatusOK { + t.Fatalf("session after a refused self-delete: status = %d, want 200, body=%s", sess.Code, sess.Body.String()) } } diff --git a/api/localauth/store.go b/api/localauth/store.go index 6a11dc9..13b9947 100644 --- a/api/localauth/store.go +++ b/api/localauth/store.go @@ -314,13 +314,18 @@ func (s *Store) GetPasswordHashByID(ctx context.Context, id string) (string, err return hash, nil } -// CountLocalUsers backs -seed-admin's idempotency check (see -// cmd/api/main.go's runSeedAdmin): a deployment that already has at -// least one local user never gets a second auto-created admin account. -func (s *Store) CountLocalUsers(ctx context.Context) (int, error) { - var n int - err := s.pool.QueryRow(ctx, `SELECT count(*) FROM users WHERE username IS NOT NULL`).Scan(&n) - return n, err +// UsernameExists backs -seed-admin's idempotency check (see +// cmd/api/main.go's runSeedAdmin). It asks whether the account that +// command would create is already provisioned -- deliberately not +// whether the deployment has any local user at all, which is the +// question it used to ask: an operator who deleted the seeded admin +// account was then refused by the very command documented as the way +// to create one, because some *other* account still existed. +func (s *Store) UsernameExists(ctx context.Context, username string) (bool, error) { + var exists bool + err := s.pool.QueryRow(ctx, + `SELECT EXISTS(SELECT 1 FROM users WHERE username = $1)`, username).Scan(&exists) + return exists, err } // CreateSession mints a fresh opaque token for an already-authenticated