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