From 55d515d41ffaf4a6a6b8bf6d6a87492d1349f59d Mon Sep 17 00:00:00 2001 From: Lemon-miaow Date: Wed, 23 Sep 2026 07:48:54 +0800 Subject: [PATCH] fix(provisioning): keep the Owner seat single; clash on the operator name stays retryable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects live-drilled in the break-glass staff provisioning: - An Owner reset that typed any username other than the occupied seat took UpsertOwner's insert arm and silently minted a SECOND owner row, leaving the existing seat — possibly the compromised account the reset was meant to replace — live; every owner row is undeletable through the panel, so the tier could never converge back to one. provisionOwner now refuses with ownerSeatTakenError naming the seat (recoverable: the TUI routes back to the form); bootstrap still mints, and the seat's own username still resets in place. PGRepo gains OwnerUsername for the guard. - InsertOperator returned the raw driver error on a taken username while the console keys its rename prompt off api.ErrConflict — the "choose another name" leg died with SQLSTATE 23505 against real Postgres (the fake encoded the contract; PGRepo had drifted). Map the unique violation to ErrConflict and pin it in pgint. Live (auditfix37): fresh username refused naming the seat; seat reset kept the id/email with still exactly one owner; taken operator name returned to the form with the retry note, and the retyped name succeeded (drill rows cleaned). --- cmd/felis/breakglass.go | 28 ++++++++++++++++++ cmd/felis/breakglass_test.go | 55 ++++++++++++++++++++++++++++++------ cmd/felis/tui_menu_test.go | 41 ++++++++++++++++++++++----- cmd/felis/tui_owner.go | 25 ++++++++++------ internal/api/pgrepo.go | 26 +++++++++++++++-- internal/pgint/pgint_test.go | 25 +++++++++++++++- 6 files changed, 173 insertions(+), 27 deletions(-) diff --git a/cmd/felis/breakglass.go b/cmd/felis/breakglass.go index c801aa2..df22cc5 100644 --- a/cmd/felis/breakglass.go +++ b/cmd/felis/breakglass.go @@ -76,6 +76,12 @@ type ownerStore interface { AdminExists(ctx context.Context) (bool, error) // UserByUsername loads a staff login projection. UserByUsername(ctx context.Context, username string) (*api.StaffUser, error) + // OwnerUsername names the single active Owner seat, or "" when none exists. + // provisionOwner refuses to re-target anything but this username: with the + // seat occupied, a fresh name would mint a second owner row (UpsertOwner's + // insert arm) while the existing — possibly compromised — seat stays live, + // and no supported path can delete an owner row afterwards. + OwnerUsername(ctx context.Context) (string, error) UpsertOwner(ctx context.Context, id, username, email string) error // InsertOperator mints a NEW Operator staff account. Unlike UpsertOwner it is // insert-only: a username already taken is a conflict (api.ErrConflict), never a @@ -278,11 +284,20 @@ func authenticateAdmin(ctx context.Context, s ownerStore, username string) (matc // provisionOwner mints or resets the single Owner account direct-to-Postgres, // passwordless. The account is role=owner with no password — the Owner completes // passwordless login setup via the web setup-token flow after `felis setup`. +// With a seat already occupied the reset must name that seat (ownerSeatTakenError +// otherwise): the upsert's insert arm would silently mint a SECOND owner, and +// every owner row is undeletable through the panel, so the tier could never +// converge back to one. func provisionOwner(ctx context.Context, s ownerStore, username, email string) error { username = strings.TrimSpace(username) if username == "" { return errors.New("owner username is required") } + if seat, err := s.OwnerUsername(ctx); err != nil { + return fmt.Errorf("check the owner seat: %w", err) + } else if seat != "" && seat != username { + return &ownerSeatTakenError{seat: seat} + } id := newOwnerID() if id == "" { return errors.New("generate owner id: entropy source failed") @@ -293,6 +308,19 @@ func provisionOwner(ctx context.Context, s ownerStore, username, email string) e return nil } +// ownerSeatTakenError refuses an Owner reset that names anything but the +// occupied seat, naming it so the operator can retype. Is reports +// api.ErrConflict so the TUI's recoverable-error branch (shared with the +// operator path's taken-name clash) routes back to the form instead of ending +// the console. +type ownerSeatTakenError struct{ seat string } + +func (e *ownerSeatTakenError) Error() string { + return fmt.Sprintf("an Owner already exists as %q — enter that username to reset the Owner", e.seat) +} + +func (e *ownerSeatTakenError) Is(target error) bool { return target == api.ErrConflict } + // provisionOperator mints a NEW Operator staff account direct-to-Postgres. It is // role=admin and passwordless — an additional staff admin below the single // role=owner identity (migrations 0003 + 0011). UNLIKE provisionOwner, which diff --git a/cmd/felis/breakglass_test.go b/cmd/felis/breakglass_test.go index 5efda14..1e8d837 100644 --- a/cmd/felis/breakglass_test.go +++ b/cmd/felis/breakglass_test.go @@ -19,14 +19,15 @@ import ( // terminal. The design is passwordless: accounts carry no credential, and the // Owner completes first-login through the setup-token web flow. type fakeOwnerStore struct { - upserts []upsertCall - inserts []upsertCall - settings map[string][]byte - audits []api.AuditEntry - tokens []setupTokenCall - redeems []redeemCall - users map[string]*api.StaffUser // keyed by username - admins bool // AdminExists answer + upserts []upsertCall + inserts []upsertCall + settings map[string][]byte + audits []api.AuditEntry + tokens []setupTokenCall + redeems []redeemCall + users map[string]*api.StaffUser // keyed by username + admins bool // AdminExists answer + ownerSeat string // OwnerUsername answer: the occupied seat, "" when none // CompleteOwnerSetup's success result. redeemUserID defaults to the fresh id // the caller passes (the unlinked-UUID case) when left empty. @@ -40,6 +41,7 @@ type fakeOwnerStore struct { auditErr error userErr error // non-not-found error from UserByUsername adminErr error + seatErr error redeemErr error createTokenErr error } @@ -80,6 +82,15 @@ func (f *fakeOwnerStore) UserByUsername(_ context.Context, username string) (*ap return nil, api.ErrNotFound } +// OwnerUsername reports the single active Owner seat. Tests set ownerSeat; the +// zero value models a fresh install where bootstrap is free to mint. +func (f *fakeOwnerStore) OwnerUsername(_ context.Context) (string, error) { + if f.seatErr != nil { + return "", f.seatErr + } + return f.ownerSeat, nil +} + func (f *fakeOwnerStore) UpsertOwner(_ context.Context, id, username, email string) error { if f.upsertErr != nil { return f.upsertErr @@ -201,6 +212,34 @@ func TestProvisionOwner(t *testing.T) { } }) + t.Run("an occupied seat refuses any other username", func(t *testing.T) { + // The seat is the single owner row: upserting a fresh name would take the + // insert arm and mint a SECOND owner, while the existing seat — possibly the + // compromised account this reset was meant to replace — stays live, and no + // supported path can delete an owner row. + f := &fakeOwnerStore{ownerSeat: "seat-holder"} + err := provisionOwner(ctx, f, "someone-else", "") + if !errors.Is(err, api.ErrConflict) { + t.Fatalf("error = %v, want it to wrap api.ErrConflict so the TUI routes back to the form", err) + } + if !strings.Contains(err.Error(), `"seat-holder"`) { + t.Errorf("error = %q, want it to name the occupied seat", err) + } + if len(f.upserts) != 0 { + t.Errorf("want no write against an occupied seat, got %d", len(f.upserts)) + } + }) + + t.Run("the occupied seat's own username still resets", func(t *testing.T) { + f := &fakeOwnerStore{ownerSeat: "seat-holder"} + if err := provisionOwner(ctx, f, "seat-holder", "new@example.net"); err != nil { + t.Fatalf("provisionOwner(reset): %v", err) + } + if len(f.upserts) != 1 || f.upserts[0].username != "seat-holder" || f.upserts[0].email != "new@example.net" { + t.Fatalf("want 1 reset upsert for the seat, got %+v", f.upserts) + } + }) + t.Run("propagates a store error", func(t *testing.T) { f := &fakeOwnerStore{upsertErr: errors.New("boom")} if err := provisionOwner(ctx, f, "owner", ""); err == nil { diff --git a/cmd/felis/tui_menu_test.go b/cmd/felis/tui_menu_test.go index 6b16c6f..32d0a15 100644 --- a/cmd/felis/tui_menu_test.go +++ b/cmd/felis/tui_menu_test.go @@ -151,19 +151,46 @@ func TestOwnerModelProvisionErrorRouting(t *testing.T) { } }) - t.Run("a conflict on the Owner path is not a retry", func(t *testing.T) { - // Defensive: the Owner upserts and so never conflicts, but were one ever to - // surface it must end the session rather than loop the form — only the - // insert-only operator path is retryable. + t.Run("the Owner seat refusal returns to the form naming the seat", func(t *testing.T) { + // Upserting a fresh username while a seat is occupied would mint a second + // owner, so provisionOwner refuses with ownerSeatTakenError (Is + // api.ErrConflict) and the console must route back for a retype — the same + // recoverable contract as the operator clash, and the only Owner-path + // conflict there is. m := newOwnerModel(ctx, &fakeOwnerStore{}, "root", true) + seatErr := &ownerSeatTakenError{seat: "seat-holder"} - next, cmd := m.Update(owProvisionMsg{err: conflict}) + next, cmd := m.Update(owProvisionMsg{err: seatErr}) + om := next.(*ownerModel) + if om.step != owProvision { + t.Fatalf("step = %v, want owProvision — the seat refusal is recoverable", om.step) + } + if om.provisionErr == nil || !errors.Is(om.provisionErr, api.ErrConflict) || !strings.Contains(om.provisionErr.Error(), "seat-holder") { + t.Errorf("provisionErr = %v, want the seat refusal naming the seat", om.provisionErr) + } + // Feed the rebuilt form's init message back through so its view renders; + // then the note must carry the seat name (the operator's retype cue). + if cmd != nil { + if msg := cmd(); msg != nil { + if n2, _ := om.Update(msg); n2 != nil { + om = n2.(*ownerModel) + } + } + } + if view := om.form.View(); !strings.Contains(view, "seat-holder") { + t.Errorf("the provision form must surface the seat refusal:\n%s", view) + } + }) + + t.Run("a generic Owner-path fault still tears the console down", func(t *testing.T) { + m := newOwnerModel(ctx, &fakeOwnerStore{}, "root", true) + next, cmd := m.Update(owProvisionMsg{err: errors.New("boom")}) om := next.(*ownerModel) if om.provisionErr != nil { - t.Error("the Owner path recorded a retryable conflict; only the operator path retries") + t.Error("a generic fault must not be treated as a retryable refusal") } if res, ok := cmd().(ownerResultMsg); !ok || res.err == nil { - t.Error("an Owner-path conflict should tear down via an error result") + t.Error("a generic Owner-path fault should tear down via an error result") } }) } diff --git a/cmd/felis/tui_owner.go b/cmd/felis/tui_owner.go index 2ec4019..be6fe55 100644 --- a/cmd/felis/tui_owner.go +++ b/cmd/felis/tui_owner.go @@ -174,12 +174,13 @@ func (m *ownerModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case owProvisionMsg: if msg.err != nil { - // A taken Operator username is the expected, recoverable outcome of the - // insert-only operator path (refusing the clash is the whole reason it is - // insert-only, not an upsert). Route back to the form with a note so the - // operator can pick another name, rather than tearing down the console — - // any other error is a genuine fault and still ends the session. - if m.operation == bgAddOperator && errors.Is(msg.err, api.ErrConflict) { + // api.ErrConflict marks the two recoverable refusals: a taken Operator + // username (insert-only clash) and an Owner reset naming anything but the + // occupied seat (ownerSeatTakenError Is ErrConflict). Route back to the + // form with a note so the operator can retype, rather than tearing down + // the console — any other error is a genuine fault and still ends the + // session. + if errors.Is(msg.err, api.ErrConflict) { m.provisionErr = msg.err m.step = owProvision m.form = m.sized(m.buildProvisionForm()) @@ -364,9 +365,15 @@ func (m *ownerModel) buildProvisionForm() *huh.Form { } } if m.provisionErr != nil { - // The only error routed back to this form is a username clash on the insert-only - // operator path; show a concrete prompt to choose another name. - desc = "That username is already taken — choose a different one.\n\n" + desc + // Recoverable refusals routed back here: the seat refusal already names the + // username to enter, so show it verbatim; the operator-name clash gets the + // generic retry prompt. + note := "That username is already taken — choose a different one." + var seatErr *ownerSeatTakenError + if errors.As(m.provisionErr, &seatErr) { + note = seatErr.Error() + } + desc = note + "\n\n" + desc } fields := []huh.Field{ diff --git a/internal/api/pgrepo.go b/internal/api/pgrepo.go index bc7a7d3..ffd706a 100644 --- a/internal/api/pgrepo.go +++ b/internal/api/pgrepo.go @@ -1037,16 +1037,38 @@ func (p *PGRepo) UpsertOwner(ctx context.Context, id, username, email string) er return err } +// OwnerUsername names the single active Owner seat, or "" when no owner exists. +// It backs the console's single-seat guard: once a seat is occupied only that +// username may be re-targeted (see cmd/felis provisionOwner), because a fresh +// name would take the upsert's insert arm and mint a SECOND owner row that no +// supported path can remove (the panel protects every owner row). +func (p *PGRepo) OwnerUsername(ctx context.Context) (string, error) { + var name string + switch err := p.db.QueryRowContext(ctx, + `SELECT username FROM users WHERE role = 'owner' AND deleted_at IS NULL ORDER BY created_at LIMIT 1`).Scan(&name); { + case errors.Is(err, sql.ErrNoRows): + return "", nil + case err != nil: + return "", err + default: + return name, nil + } +} + // InsertOperator mints a NEW Operator (additional staff admin) account // direct-to-Postgres. role is forced to 'admin'. UNLIKE UpsertOwner this is -// insert-only: a username conflict is left untouched and surfaces as a driver -// error, so adding an Operator can never silently reset the Owner's or another +// insert-only: a username conflict leaves the existing row untouched and +// surfaces as ErrConflict — the console routes a rename off that sentinel — so +// adding an Operator can never silently reset the Owner's or another // Operator's row. The account is passwordless by design. The empty email is // stored as NULL. func (p *PGRepo) InsertOperator(ctx context.Context, id, username, email string) error { _, err := p.db.ExecContext(ctx, `INSERT INTO users (id, username, email, role) VALUES ($1, $2, NULLIF($3, ''), 'admin')`, id, username, email) + if err != nil && isUniqueViolation(err) { + return ErrConflict + } return err } diff --git a/internal/pgint/pgint_test.go b/internal/pgint/pgint_test.go index 552cb8b..d42b0c3 100644 --- a/internal/pgint/pgint_test.go +++ b/internal/pgint/pgint_test.go @@ -666,14 +666,37 @@ func TestOwnerProvisioningWritesOwnerRole(t *testing.T) { if ok, err := repo.AdminExists(ctx); err != nil || !ok { t.Fatalf("AdminExists = (%v, %v), want true (the owner counts as staff)", ok, err) } + // OwnerUsername names the seat the console guard protects. The shared test + // database may hold owner rows from earlier tests, so assert the returned name + // IS an active owner rather than one specific row. + seat, err := repo.OwnerUsername(ctx) + if err != nil { + t.Fatalf("OwnerUsername: %v", err) + } + if seat == "" { + t.Fatal("OwnerUsername = empty, want an active owner seat") + } + var active int + if err := db.QueryRowContext(ctx, + `SELECT count(*) FROM users WHERE username = $1 AND role = 'owner' AND deleted_at IS NULL`, seat). + Scan(&active); err != nil || active != 1 { + t.Fatalf("OwnerUsername returned %q, not an active owner row (count=%d err=%v)", seat, active, err) + } // Operators stay plain admins: the owner tier stays singular. opID := "usr-op-" + suffix(t) - if err := repo.InsertOperator(ctx, opID, "op-"+suffix(t), ""); err != nil { + opName := "op-" + suffix(t) + if err := repo.InsertOperator(ctx, opID, opName, ""); err != nil { t.Fatalf("InsertOperator: %v", err) } if role := userRole(t, opID); role != "admin" { t.Fatalf("InsertOperator role = %q, want admin", role) } + // A taken username must surface as api.ErrConflict: the console routes its + // rename prompt off that sentinel (the cmd fake encoded the contract; PGRepo + // returned the raw driver error until this arm was mapped). + if err := repo.InsertOperator(ctx, "usr-op2-"+suffix(t), opName, ""); !errors.Is(err, api.ErrConflict) { + t.Fatalf("InsertOperator on a taken username = %v, want ErrConflict", err) + } } // The setup wizard's MC-bind path establishes THE Owner, so it writes the same