Merge pull request #14 from Coffey-Labs/fix/self-delete-lockout

Stop an owner deleting the account they are signed in as

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

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)
+	}
+}

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 {

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")

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())
 	}
 }
 

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