amadan

carlos / platform Public

console: stop the ops backfill sweeping grant holders in — report them instead

Authorization is "member of the account AND the account claims the app", so a
membership is access to EVERY app in the account. The backfill swept every
pre-accounts grant holder into ops as a member, on the theory that member is
the harmless role. It is not harmless in ops: ops holds console, platform-site,
status and every unmigrated app. On the flagship that handed a user who held
ONE grant for a toy app the ability to promote and roll back kass, keymail and
the console itself. Nobody granted that; the migration invented it, and undoing
it took hand-edited SQL against a live database.

Now: operators (the flag that IS deployment-wide authority) become ops owners;
everyone else is left out and returned as []GrantHolder{address, apps}, which
NewServer logs as a runbook — who, which apps, and the three commands that give
one of them an account of their own. A count and a list would be a to-do; the
difference decides whether it gets done.

The "promote the earliest grant holder so ops has an owner" fallback is gone
too: it was the same widening by a quieter route, and an ownerless ops is now
recoverable box-side with `console member -add <addr> bab -role owner` — the
tool that did not exist when that fallback was written.

Strictly additive: INSERT OR IGNORE only, no UPDATE, no DELETE. A re-run on the
already-deployed flagship — which ran this once under the old semantics and was
cleaned up by hand — cannot resurrect the removed membership, and cannot
re-role a membership an operator has since created deliberately. Reported
holders exclude anyone already in ops, so the log goes quiet once dealt with.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude 15647618974c0a4dd098c3f048f617639dae3864 parent 5e22bad
3 files changed, +243 −103
  • internal/console/account_test.go +120 −40
  • internal/console/server.go +11 −4
  • internal/console/store.go +112 −59
diff --git a/internal/console/account_test.go b/internal/console/account_test.go
index 448a54022..8094256e9 100644
--- a/internal/console/account_test.go
+++ b/internal/console/account_test.go
@@ -3,6 +3,7 @@ package console
import (
"context"
"encoding/json"
+ "strings"
"testing"
"time"
@@ -220,76 +221,130 @@ func TestLastAccountRoundTrips(t *testing.T) {
}
}
-// A per-app grant was never deployment-wide authority. ops contains console,
-// platform-site and status, so backfilling every grant holder as an OWNER of
-// it would hand whoever once shipped one app the right to invite people into
-// the account that runs the platform itself. App-grant holders land as
-// members; the operator flag — which IS deployment-wide authority — is what
-// makes an owner.
-func TestBackfillGrantsAreMembersOperatorsAreOwners(t *testing.T) {
+// A per-app grant was never deployment-wide authority, and membership IS
+// authorization: "member of the account AND the account claims the app" means
+// one membership of ops is access to console, platform-site, status and every
+// other app that lives there. Sweeping grant holders into ops did exactly that
+// on the flagship — someone holding ONE grant for a toy app could promote and
+// roll back kass, keymail and the console. Non-operators must stay OUT, and be
+// reported so they get accounts of their own.
+func TestBackfillLeavesGrantHoldersOutOfOpsAndReportsThem(t *testing.T) {
st := openTestStore(t)
- if err := st.Grant("dev@keymail.dev", "kass"); err != nil {
- t.Fatal(err)
- }
- if err := st.Grant("ops@keymail.dev", "console"); err != nil {
- t.Fatal(err)
+ for _, g := range [][2]string{
+ {"dev@keymail.dev", "toy"},
+ {"dev@keymail.dev", "another-toy"},
+ {"ops@keymail.dev", "console"},
+ } {
+ if err := st.Grant(g[0], g[1]); err != nil {
+ t.Fatal(err)
+ }
}
if err := st.SetOperator("ops@keymail.dev", true); err != nil {
t.Fatal(err)
}
- if err := st.BackfillOpsMemberships(time.Now()); err != nil {
+
+ needs, err := st.BackfillOpsMemberships(time.Now())
+ if err != nil {
t.Fatal(err)
}
- if role, _ := st.Role(OpsAccountID, "dev@keymail.dev"); role != RoleMember {
- t.Errorf("app-grant holder role = %q, want member", role)
+ if role, _ := st.Role(OpsAccountID, "dev@keymail.dev"); role != "" {
+ t.Errorf("app-grant holder got role %q in ops — that is the widening this fixes", role)
}
if role, _ := st.Role(OpsAccountID, "ops@keymail.dev"); role != RoleOwner {
t.Errorf("operator role = %q, want owner", role)
}
- // Both still have their apps — the demotion is of authority, not access.
+ // Left out is not the same as forgotten: the operator has to be told who
+ // needs an account, with their apps, or the migration silently loses them.
+ if len(needs) != 1 || needs[0].Address != "dev@keymail.dev" {
+ t.Fatalf("report = %+v, want just dev@keymail.dev", needs)
+ }
+ if got := strings.Join(needs[0].Apps, ","); got != "another-toy,toy" {
+ t.Errorf("reported apps = %q, want both, sorted", got)
+ }
+ // The app names are still claimed in ops, which is where every
+ // pre-accounts app lived — migrate-app is what moves them out.
apps, _ := st.AppsIn(OpsAccountID)
- if len(apps) != 2 {
- t.Errorf("AppsIn(ops) = %v, want both apps claimed", apps)
+ if len(apps) != 3 {
+ t.Errorf("AppsIn(ops) = %v, want every granted app claimed", apps)
}
- // Idempotent, and it must not DEMOTE an owner on a later run.
- if err := st.BackfillOpsMemberships(time.Now()); err != nil {
+
+ // Idempotent: a re-run changes nothing and reports the same people, so a
+ // deployment that has already run this (with the OLD semantics, cleaned up
+ // by hand) neither loses the cleanup nor re-applies the widening.
+ again, err := st.BackfillOpsMemberships(time.Now())
+ if err != nil {
t.Fatal(err)
}
+ if len(again) != 1 || again[0].Address != "dev@keymail.dev" {
+ t.Errorf("second report = %+v, want the same", again)
+ }
+ if role, _ := st.Role(OpsAccountID, "dev@keymail.dev"); role != "" {
+ t.Errorf("second run put the grant holder into ops as %q", role)
+ }
if role, _ := st.Role(OpsAccountID, "ops@keymail.dev"); role != RoleOwner {
- t.Errorf("second backfill demoted the operator to %q", role)
+ t.Errorf("second run changed the operator to %q", role)
}
}
-// ops must never end up with nobody who can administer it. With grants but no
-// operator, the earliest grant holder is promoted — loudly, because it is a
-// fallback, not the rule.
-func TestBackfillPromotesAnOwnerWhenThereWouldBeNone(t *testing.T) {
+// The flagship shape: the backfill has already run once with the old
+// semantics, and the widened membership was removed by hand. Re-running must
+// not put it back, and must not touch a membership an operator has since
+// created deliberately with `console member`.
+func TestBackfillNeverResurrectsARemovedMembership(t *testing.T) {
st := openTestStore(t)
- base := time.Unix(1700000000, 0)
- if err := st.GrantAt("second@keymail.dev", "b", base.Add(time.Hour)); err != nil {
+ now := time.Now()
+ if err := st.Grant("dev@keymail.dev", "toy"); err != nil {
t.Fatal(err)
}
- if err := st.GrantAt("first@keymail.dev", "a", base); err != nil {
+ if err := st.Grant("paul@keymail.dev", "kass"); err != nil {
+ t.Fatal(err)
+ }
+ if err := st.SetOperator("paul@keymail.dev", true); err != nil {
+ t.Fatal(err)
+ }
+ if _, err := st.BackfillOpsMemberships(now); err != nil {
t.Fatal(err)
}
- if err := st.BackfillOpsMemberships(time.Now()); err != nil {
+ // An operator deliberately admits somebody as a plain member.
+ if err := st.AddMember(OpsAccountID, "trusted@keymail.dev", RoleMember, now); err != nil {
t.Fatal(err)
}
- if role, _ := st.Role(OpsAccountID, "first@keymail.dev"); role != RoleOwner {
- t.Errorf("earliest grant holder role = %q, want owner (nobody else could administer ops)", role)
+
+ needs, err := st.BackfillOpsMemberships(now)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if role, _ := st.Role(OpsAccountID, "dev@keymail.dev"); role != "" {
+ t.Errorf("the removed membership came back as %q", role)
+ }
+ if role, _ := st.Role(OpsAccountID, "trusted@keymail.dev"); role != RoleMember {
+ t.Errorf("deliberate membership changed to %q, want member", role)
}
- if role, _ := st.Role(OpsAccountID, "second@keymail.dev"); role != RoleMember {
- t.Errorf("later grant holder role = %q, want member", role)
+ // Somebody an operator has already dealt with is not reported again.
+ for _, h := range needs {
+ if h.Address == "trusted@keymail.dev" || h.Address == "paul@keymail.dev" {
+ t.Errorf("reported %s, who is already a member of ops", h.Address)
+ }
}
}
-// A deployment with no grants at all and no operator gets an ops account with
-// no members — which is correct: there is nobody to make an owner, and
-// inventing one would be worse than an empty account the first operator
-// claims.
-func TestBackfillLeavesOpsEmptyWhenThereIsNobody(t *testing.T) {
+// With grants but NO operator, nobody is promoted. The old code promoted the
+// earliest grant holder to OWNER of ops so the account would have someone who
+// could administer it — but ops holds the console itself, and that promotion
+// is the same widening by a quieter route. An ownerless ops is recoverable
+// box-side (`console member -add <addr> bab -role owner`), which is precisely
+// the tool that did not exist when that fallback was written.
+func TestBackfillPromotesNobodyWhenThereIsNoOperator(t *testing.T) {
st := openTestStore(t)
- if err := st.BackfillOpsMemberships(time.Now()); err != nil {
+ base := time.Unix(1700000000, 0)
+ if err := st.GrantAt("second@keymail.dev", "b", base.Add(time.Hour)); err != nil {
+ t.Fatal(err)
+ }
+ if err := st.GrantAt("first@keymail.dev", "a", base); err != nil {
+ t.Fatal(err)
+ }
+ needs, err := st.BackfillOpsMemberships(time.Now())
+ if err != nil {
t.Fatal(err)
}
members, err := st.Members(OpsAccountID)
@@ -297,7 +352,32 @@ func TestBackfillLeavesOpsEmptyWhenThereIsNobody(t *testing.T) {
t.Fatal(err)
}
if len(members) != 0 {
- t.Errorf("Members(ops) = %+v, want none", members)
+ t.Errorf("Members(ops) = %+v, want nobody — a grant is not authority over ops", members)
+ }
+ if len(needs) != 2 {
+ t.Errorf("report = %+v, want both grant holders", needs)
+ }
+}
+
+// The advice is the whole point of the report: an operator reading the log
+// must be able to copy three commands per person, not go and design a
+// migration. It names the apps, so nobody is left behind.
+func TestGrantHolderAdviceIsCopyPasteable(t *testing.T) {
+ msg := grantHolderAdvice([]GrantHolder{
+ {Address: "dev@keymail.dev", Apps: []string{"toy", "other"}},
+ {Address: "sam@keymail.dev", Apps: []string{"blog"}},
+ })
+ for _, want := range []string{
+ "2 grant holders need accounts",
+ "dev@keymail.dev", "toy", "other", "sam@keymail.dev", "blog",
+ "carlos accounts create", "console member -add", "-role owner", "console migrate-app",
+ } {
+ if !strings.Contains(msg, want) {
+ t.Errorf("advice does not mention %q: %s", want, msg)
+ }
+ }
+ if grantHolderAdvice(nil) != "" {
+ t.Errorf("advice for nobody = %q, want silence", grantHolderAdvice(nil))
}
}
diff --git a/internal/console/server.go b/internal/console/server.go
index 915d50e1d..4b1401bc8 100644
--- a/internal/console/server.go
+++ b/internal/console/server.go
@@ -165,12 +165,19 @@ func NewServer(cfg Config, st *Store) (*Server, error) {
// all of them. Best-effort; a bucket hiccup must not stop the console
// booting.
materializeQuietly(context.Background(), cfg.Releases, st)
- // One-time backfill: every existing per-app grant becomes a membership of
- // the ops account, which is where every pre-accounts app already lives.
- // Idempotent, and cheap enough to run on every start.
- if err := st.BackfillOpsMemberships(time.Now()); err != nil {
+ // The pre-accounts backfill: operators become owners of ops, every granted
+ // app name is claimed there, and non-operator grant holders are left out
+ // and REPORTED — a per-app grant was never authority over ops. Additive and
+ // idempotent, so it is cheap enough to run on every start. The report is
+ // logged rather than acted on: only an operator can decide who gets an
+ // account of their own, but nobody can decide it if nobody is told.
+ needAccounts, err := st.BackfillOpsMemberships(time.Now())
+ if err != nil {
return nil, err
}
+ if advice := grantHolderAdvice(needAccounts); advice != "" {
+ log.Print(advice)
+ }
return s, nil
}
diff --git a/internal/console/store.go b/internal/console/store.go
index ac9869997..e5a994c2d 100644
--- a/internal/console/store.go
+++ b/internal/console/store.go
@@ -15,7 +15,6 @@ import (
"encoding/base64"
"encoding/hex"
"fmt"
- "log"
"os"
"path/filepath"
"strings"
@@ -296,84 +295,138 @@ func (s *Store) Owners() (map[string][]string, error) {
// ---- accounts, memberships, invites -----------------------------------------
-// BackfillOpsMemberships maps the pre-accounts world onto the new one, once:
-// the built-in ops account exists, every address that held ANY app grant is a
-// member of it, and every granted app name is claimed in it. That is the
-// spec's "every existing grant maps to a membership in the account that owns
-// the app" — before accounts, every app was in ops by definition.
+// GrantHolder is one pre-accounts app-grant holder that the backfill
+// deliberately left OUT of ops, with the apps they held grants for. The
+// backfill cannot decide where these people belong — that is an account per
+// person or per project, which is an operator's judgement — so it reports them
+// instead of guessing.
+type GrantHolder struct {
+ Address string
+ Apps []string
+}
+
+// BackfillOpsMemberships maps the pre-accounts world onto the new one: the
+// built-in ops account exists, every OPERATOR is an owner of it, and every
+// granted app name is claimed in it (before accounts, every app was in ops by
+// definition). It returns the grant holders it left out, so the caller can say
+// who still needs an account of their own.
+//
+// WHY non-operators are excluded — this widened access in production once
+// already, and the reasoning has to survive the next person to read it:
//
-// Grant holders become MEMBERS, not owners. A per-app grant was never
-// deployment-wide authority, and ops is not an ordinary account: it holds
-// console, platform-site and status. Backfilling every grant holder as an
-// owner would hand whoever once shipped one app the right to invite people
-// into the account that runs the platform itself. What IS deployment-wide
-// authority is the operator flag, so that — and only that — backfills as
-// owner.
+// Authorization is "Role(account, address) != "" AND HasAppIn(account, app)".
+// A membership is therefore access to EVERY app in the account, not to the app
+// somebody once held a grant for. The first version of this function swept all
+// grant holders into ops as members, on the theory that a member is the
+// harmless role. It is not harmless in ops: ops holds console, platform-site,
+// status, and every app that has not yet been migrated out. On the flagship
+// that handed a user who held ONE grant — for a toy app — the ability to
+// promote and roll back kass, keymail and the console itself. Nobody granted
+// them that; the migration invented it, and it took hand-edited SQL to undo.
//
-// If that would leave ops with no owner at all, the earliest grant holder is
-// promoted and the promotion is logged loudly: an account nobody can
-// administer is a worse outcome than one operator who has to be told they
-// were given the role. A deployment with no grants and no operators gets an
-// empty ops account, which is correct — there is nobody to promote, and
-// inventing someone would be worse.
+// A per-app grant was never deployment-wide authority. What IS deployment-wide
+// authority is the operator flag, so that — and only that — makes an ops
+// membership here, as owner.
//
-// Idempotent, so it runs on every console start: existing memberships are
-// left exactly as they are (a later run never demotes an owner), and a
-// deployment that has already moved on gains nothing and loses nothing. It
-// deliberately does NOT delete the owners table: the grants stay readable
-// while the migration is being verified.
-func (s *Store) BackfillOpsMemberships(now time.Time) error {
+// There is deliberately no "promote somebody so ops has an owner" fallback any
+// more. The old code promoted the earliest grant holder when no operator
+// existed, reasoning that an unadministrable account is worse than an
+// unexpected owner. But that is the same widening by a quieter route, and it
+// is no longer the only way out: an ownerless ops is recoverable box-side with
+// `console member -add <addr> bab -role owner` — the tool that did not exist
+// when that fallback was written.
+//
+// Strictly ADDITIVE and idempotent, so it can run on every console start: it
+// only ever INSERT OR IGNOREs, so no existing membership is ever demoted,
+// re-roled or removed. That matters concretely — the flagship has already run
+// this once under the old semantics and been cleaned up by hand, and a re-run
+// must not resurrect what was removed. It also deliberately does NOT delete the
+// owners table: the grants stay readable while the migration is verified.
+func (s *Store) BackfillOpsMemberships(now time.Time) ([]GrantHolder, error) {
tx, err := s.db.Begin()
if err != nil {
- return err
+ return nil, err
}
defer tx.Rollback()
if _, err := tx.Exec(`INSERT OR IGNORE INTO accounts (id, sqid, name, created_at)
VALUES (?, ?, 'ops', ?)`, OpsAccountID, opsSqid, now.Unix()); err != nil {
- return err
- }
- // OR IGNORE, never OR REPLACE: an address that is already a member keeps
- // whatever role it has, so a second run cannot demote an owner.
- if _, err := tx.Exec(`INSERT OR IGNORE INTO memberships (account_id, address, role, added_at)
-SELECT DISTINCT ?, address, ?, ? FROM owners`, OpsAccountID, RoleMember, now.Unix()); err != nil {
- return err
+ return nil, err
}
+ // OR IGNORE, never OR REPLACE or UPDATE: an address that is already a
+ // member keeps whatever role it has. This is the only membership write in
+ // the whole function.
if _, err := tx.Exec(`INSERT OR IGNORE INTO memberships (account_id, address, role, added_at)
-SELECT ?, address, ?, ? FROM users WHERE operator != 0`, OpsAccountID, RoleOwner, now.Unix()); err != nil {
- return err
- }
- // The operators that were already members as grant holders: promote them.
- if _, err := tx.Exec(`UPDATE memberships SET role = ? WHERE account_id = ?
-AND address IN (SELECT address FROM users WHERE operator != 0)`, RoleOwner, OpsAccountID); err != nil {
- return err
+SELECT ?, address, ?, ? FROM users WHERE operator != 0`,
+ OpsAccountID, RoleOwner, now.Unix()); err != nil {
+ return nil, err
}
+ // The app NAMES are still claimed in ops: that is where every pre-accounts
+ // app physically lives, and claiming them keeps two accounts from minting
+ // the same name before the migration moves them. `console migrate-app` is
+ // what re-homes them afterwards.
if _, err := tx.Exec(`INSERT OR IGNORE INTO apps (app, account_id, created_by, created_at)
SELECT app, ?, MIN(address), ? FROM owners GROUP BY app`, OpsAccountID, now.Unix()); err != nil {
- return err
+ return nil, err
}
- var owners int
- if err := tx.QueryRow(`SELECT COUNT(*) FROM memberships WHERE account_id = ? AND role = ?`,
- OpsAccountID, RoleOwner).Scan(&owners); err != nil {
- return err
+ // Who is left over: grant holders who are not operators and hold no ops
+ // membership. The membership exclusion is what makes the report quiet once
+ // an operator has dealt with someone — deliberately admitting them, or
+ // giving them their own account — rather than nagging on every restart.
+ rows, err := tx.Query(`SELECT o.address, o.app FROM owners o
+WHERE o.address NOT IN (SELECT address FROM users WHERE operator != 0)
+ AND o.address NOT IN (SELECT address FROM memberships WHERE account_id = ?)
+ORDER BY o.address, o.app`, OpsAccountID)
+ if err != nil {
+ return nil, err
}
- if owners == 0 {
- var first string
- err := tx.QueryRow(`SELECT address FROM owners ORDER BY created_at, address LIMIT 1`).Scan(&first)
- if err == sql.ErrNoRows {
- return tx.Commit() // nobody to promote; an empty ops account is honest
+ var out []GrantHolder
+ for rows.Next() {
+ var addr, app string
+ if err := rows.Scan(&addr, &app); err != nil {
+ rows.Close()
+ return nil, err
}
- if err != nil {
- return err
+ if n := len(out); n > 0 && out[n-1].Address == addr {
+ out[n-1].Apps = append(out[n-1].Apps, app)
+ continue
}
- if _, err := tx.Exec(`UPDATE memberships SET role = ? WHERE account_id = ? AND address = ?`,
- RoleOwner, OpsAccountID, first); err != nil {
- return err
+ out = append(out, GrantHolder{Address: addr, Apps: []string{app}})
+ }
+ rows.Close()
+ if err := rows.Err(); err != nil {
+ return nil, err
+ }
+ if err := tx.Commit(); err != nil {
+ return nil, err
+ }
+ return out, nil
+}
+
+// grantHolderAdvice renders the leftover grant holders as something an
+// operator can act on: who, which apps, and the exact three commands that give
+// one of them an account. A count and a list would be a to-do; this is a
+// runbook, and the difference decides whether it gets done.
+//
+// Returns "" when there is nobody, so the caller logs nothing rather than a
+// line saying nothing happened.
+func grantHolderAdvice(holders []GrantHolder) string {
+ if len(holders) == 0 {
+ return ""
+ }
+ var b strings.Builder
+ fmt.Fprintf(&b, "console: %d grant holders need accounts of their own — they hold pre-accounts "+
+ "app grants and were deliberately NOT made members of ops (a membership there is access to "+
+ "console, platform-site and every unmigrated app): ", len(holders))
+ for i, h := range holders {
+ if i > 0 {
+ b.WriteString("; ")
}
- log.Printf("console: no operator existed, so %s (the earliest app-grant holder) was made "+
- "OWNER of the ops account — every account needs one. Review this: ops holds console, "+
- "platform-site and status.", first)
+ fmt.Fprintf(&b, "%s (%s)", h.Address, strings.Join(h.Apps, " "))
}
- return tx.Commit()
+ b.WriteString(". Give each one their own with: carlos accounts create -name <name> " +
+ "&& console member -add <address> <sqid> -role owner " +
+ "&& console migrate-app -app <app> -to <sqid>")
+ return b.String()
}
// Roles. Two, not a matrix: enough authority to make invites safe, without