fix(provisioning): keep the Owner seat single; clash on the operator name stays retryable
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).
This commit is contained in:
6 files changed
+173
-27
No files matched your search
@@ -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
|
||||
|
||||
@@ -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", "[email protected]"); err != nil {
|
||||
t.Fatalf("provisionOwner(reset): %v", err)
|
||||
}
|
||||
if len(f.upserts) != 1 || f.upserts[0].username != "seat-holder" || f.upserts[0].email != "[email protected]" {
|
||||
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 {
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
+16
-9
@@ -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{
|
||||
|
||||
+24
-2
@@ -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
|
||||
}
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in new issue
Block a user