amadan
merge: backfill no longer undoes migrations; accounts create requires an owner
Both bugs were found by running the migration against production, not in review: the app backfill recreated a migrated app under ops on every console restart, and accounts create minted accounts nobody could open. The manifest carries an owner so the console materializes the first membership without the carlos binary ever opening console state — adopted ONLY at account birth, because the manifest is not write-once: a bucket-writer can rewrite it, and adopting later would let them claim any vacant account without holding the release key.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
12 files changed,
+924
−34
README.md+7 −1accounts.go+69 −5cmd/console/member.go+4 −1docs/superpowers/specs/2026-08-02-multi-account-design.md+39 −1e2e_test.go+49 −5internal/console/account_test.go+138 −2internal/console/materialize.go+75 −1internal/console/materialize_test.go+276 −0internal/console/server.go+9 −5internal/console/store.go+108 −9internal/domains/mint.go+57 −2internal/domains/mint_test.go+93 −2
diff --git a/README.md b/README.md| index 4f5dd8f05..c73305cec 100644 |
| --- a/README.md |
| +++ b/README.md |
| @@ -71,7 +71,7 @@ Clicking it while signed in as that address creates the membership. |
| **From the CLI.** Every app verb takes `-account`, a sqid or a name, |
| defaulting to `ops`: |
| - carlos accounts create -name Kass # mints the id, writes the manifest |
| + carlos accounts create -name Kass -owner paul@keymail.dev |
| carlos accounts list # sqid, name, app count |
| carlos ship -account kass -app blog ... |
| carlos add -account kass -app blog blog.example.com |
| @@ -79,6 +79,12 @@ defaulting to `ops`: |
| An exact sqid wins; otherwise a unique name match; an ambiguous name lists the |
| candidates and refuses rather than guessing which tenant you meant. |
| +`-owner` is required and is recorded in the account manifest, which the console |
| +materializes into the account's first owner membership. Membership *is* |
| +authorization here, so an account created without one exists, holds its apps, |
| +and cannot be opened by anybody — a state the flagship reached for real during |
| +the multi-account migration. |
| + |
| **Bucket layout, and the migration.** App objects live under |
| `apps/<account-sqid>/<app>/…`, and that key path *is* the app→account linkage |
| — there is no separate manifest. Existing deployments still have their objects |
diff --git a/accounts.go b/accounts.go| index 717f10761..586e4f288 100644 |
| --- a/accounts.go |
| +++ b/accounts.go |
| @@ -30,8 +30,12 @@ import ( |
| const accountsUsage = `usage: carlos accounts <create|list|migrate> |
| create -name <name> mint an account: the next free id, its |
| - frozen sqid, and the bucket manifest every |
| - box materializes identity from |
| + -owner <address> frozen sqid, and the bucket manifest every |
| + box materializes identity from. -owner is |
| + required and is recorded in the manifest — |
| + the console materializes it into the first |
| + owner membership, because an account with no |
| + members cannot be reached at all |
| list sqid, name, and app count |
| migrate -app <a> -to <sqid> copy an app's objects to the account- |
| qualified prefix and re-stamp its routes |
| @@ -64,13 +68,57 @@ func RunAccounts(args []string) { |
| } |
| } |
| +// runAccountsCreate mints an account — and, since a live failure, never an |
| +// OWNERLESS one. |
| +// |
| +// What happened, in production, migrating the flagship: this verb wrote the |
| +// bucket manifest and the registry row and recorded an owner nowhere at all. |
| +// Membership IS authorization in the accounts model, so after kass and keymail |
| +// were migrated into their new accounts the operator could not reach either of |
| +// them in the console. The accounts existed, held the apps, had no members, and |
| +// nothing in the output said what to do about it — the only door was a `console |
| +// member -add <address> <sqid> -role owner` somebody had to already know. |
| +// |
| +// So -owner is REQUIRED. Not defaulted, not inferred from the invoking shell |
| +// user: an account's first owner can promote and roll back everything in it, |
| +// and guessing who that is would be minting authority. Missing means refusing, |
| +// by name, before anything is written. |
| +// |
| +// Where the owner is RECORDED is the interesting half. The carlos binary must |
| +// not open the console's database — that boundary is deliberate and was |
| +// enforced in an earlier review; console state is the console's authority, |
| +// exactly like grants. So the owner goes into the account MANIFEST, which is |
| +// already the one object account identity crosses on, and the console's own |
| +// materialization pass turns it into an owner membership |
| +// (console.MaterializeAccounts → Store.AdoptManifestOwner). The bucket is the |
| +// control plane, so a CLI-created account and a console-created one converge on |
| +// the same end state instead of on two different ones. Printing a follow-up |
| +// command instead would have left the same gap the operator just fell into: a |
| +// step that only works if somebody runs it. |
| func runAccountsCreate(args []string) { |
| fs := flag.NewFlagSet("accounts create", flag.ExitOnError) |
| regPath := fs.String("registry", defaultRegistry(), "registry database") |
| name := fs.String("name", "", "account name (free text, need not be unique)") |
| + owner := fs.String("owner", "", "keymail address of the account's first owner (required)") |
| fs.Parse(args) |
| if strings.TrimSpace(*name) == "" { |
| - log.Fatal("usage: carlos accounts create -name <name>") |
| + log.Fatal("usage: carlos accounts create -name <name> -owner <address>") |
| + } |
| + // Normalized and shape-checked with the console's own rule |
| + // (domains.ValidAddress, which console.ValidAddress delegates to), so an |
| + // address this verb accepts is one the sign-in form accepts too. The |
| + // memberships key is literal text with no COLLATE NOCASE: an owner recorded |
| + // as "Paul@Keymail.dev" would be a membership the real session never |
| + // matches — recorded, and still unable to get in. |
| + ownerAddr := domains.NormalizeAddress(*owner) |
| + if ownerAddr == "" { |
| + log.Fatal("accounts create: -owner <address> is required.\n" + |
| + "An account with no owner is unreachable through the console — membership IS\n" + |
| + "authorization, so an account with no members has no door, whatever apps are in it.\n" + |
| + " carlos accounts create -name <name> -owner <address>") |
| + } |
| + if !domains.ValidAddress(ownerAddr) { |
| + log.Fatalf("accounts create: %q is not a keymail address — expected <user>@<host>, e.g. paul@keymail.dev", *owner) |
| } |
| store := openStore() |
| // The bucket first, and not just for ordering: the manifest write IS the |
| @@ -78,7 +126,7 @@ func runAccountsCreate(args []string) { |
| // with a create-only Put, so this CLI and a console minting at the same |
| // moment cannot land on the same id — and the registry, which only ever |
| // materializes what the bucket says, never has to assign anything. |
| - id, sqid, err := domains.MintAccount(context.Background(), store, *name, time.Now()) |
| + id, sqid, err := domains.MintAccount(context.Background(), store, *name, ownerAddr, time.Now()) |
| if err != nil { |
| log.Fatalf("accounts create: %v", err) |
| } |
| @@ -90,8 +138,24 @@ func runAccountsCreate(args []string) { |
| if err := r.PutAccount(id, sqid, *name); err != nil { |
| log.Fatalf("accounts create: materialize: %v", err) |
| } |
| - fmt.Printf("created account %q\n id: %d\n sqid: %s\n", *name, id, sqid) |
| + fmt.Printf("created account %q\n id: %d\n sqid: %s\n owner: %s\n", *name, id, sqid, ownerAddr) |
| fmt.Printf("use it with: carlos ship -account %s -app <app> ...\n", sqid) |
| + // Said out loud because the ordering is not obvious and the operator has |
| + // just been bitten by its absence: the membership appears when the console |
| + // next reads the bucket, not when this command returns. |
| + // |
| + // "the FIRST time" is exact, not loose wording. The console adopts a |
| + // manifest's owner only on the pass that first creates its own row for the |
| + // account, because the manifest is not write-once — anyone with bucket write |
| + // access can rewrite it, and adopting on later passes would turn that into a |
| + // standing authority grant needing no signing key. Editing this field |
| + // afterwards will NOT move ownership; `console member -add` is the verb for |
| + // that, and it is box-side for exactly that reason. |
| + fmt.Printf("\n%s becomes an owner the first time the console materializes this account\n"+ |
| + "from the bucket (on start, and before every `console member` command). Nothing\n"+ |
| + "else to run. Editing the manifest's owner LATER grants nothing — use\n"+ |
| + "`console member -add <address> %s -role owner` to change ownership after this.\n", |
| + ownerAddr, sqid) |
| } |
| func runAccountsList(args []string) { |
diff --git a/cmd/console/member.go b/cmd/console/member.go| index 3ab9fdec3..36615dde1 100644 |
| --- a/cmd/console/member.go |
| +++ b/cmd/console/member.go |
| @@ -113,7 +113,10 @@ func runMember(args []string) { |
| } |
| if acct == nil { |
| log.Fatalf("no account %q: nothing with that sqid in %s, and no %s in the deployment bucket — "+ |
| - "create one with: carlos accounts create -name <name>", |
| + // -owner is required there, and naming it here matters: a command |
| + // printed without it fails, and the flag is the whole reason this |
| + // verb is no longer a mandatory follow-up step. |
| + "create one with: carlos accounts create -name <name> -owner <address>", |
| sqid, envOr("CONSOLE_DB", "/data/console/console.db"), console.AccountManifestKey(sqid)) |
| } |
diff --git a/docs/superpowers/specs/2026-08-02-multi-account-design.md b/docs/superpowers/specs/2026-08-02-multi-account-design.md| index 906eb9248..3cc0ae6db 100644 |
| --- a/docs/superpowers/specs/2026-08-02-multi-account-design.md |
| +++ b/docs/superpowers/specs/2026-08-02-multi-account-design.md |
| @@ -53,9 +53,47 @@ Instead: **the id is minted once, into the bucket**, at |
| ```json |
| {"id": 3, "sqid": "bad", "name": "Kass", |
| - "created_at": "2026-08-02T09:00:00Z"} |
| + "created_at": "2026-08-02T09:00:00Z", |
| + "owner": "paul@keymail.dev"} |
| ``` |
| +**`owner` (added 2026-08-02, after the migration ran).** `carlos accounts |
| +create` recorded an owner nowhere at all, so the accounts kass and keymail |
| +were migrated into had no members — and membership *is* authorization, so |
| +the operator could not open either of them in the console. The manifest now |
| +carries the intended first owner and the console's materialization pass |
| +turns it into an owner membership. The field is **additive**: absent means |
| +ownerless, which is what every earlier manifest is and stays. `-owner` is |
| +required on `accounts create`, so manifests are born carrying their owner. |
| + |
| +**Adopted at birth only.** The console applies `owner` on the pass that first |
| +creates its own row for the account, and never again. The manifest is **not |
| +write-once**: `MintAccount` claims it with a create-only `Put`, but that is a |
| +convention of one code path, not a property of the object — `Store.Put` takes |
| +its `ifMatch` from the caller, the bucket has no Object Lock, and the host IAM |
| +role holds unconditional `s3:PutObject` across it. Adopting on any later pass |
| +would make rewriting that object an authority grant: name yourself owner of a |
| +vacant account, wait for the next console start or `console member` call, and |
| +you own a live account. That is a genuine widening — forging a channel pointer |
| +needs `CARLOS_RELEASE_KEY`, this would need no key at all. |
| + |
| +The trade-off is accepted and explicit: **an owner added to a manifest after |
| +first materialization is not adopted**, and an account that materialized |
| +ownerless stays ownerless. Changing ownership is `console member -add |
| +<address> <sqid> -role owner` — box-side authority, which is the point. |
| + |
| +Once an owner exists, they cannot be silently re-granted after removal |
| +either, and that rests on `lastOwnerGuard`: the account can never return to |
| +vacancy, so there is nothing for a re-read to fill. That coupling is |
| +exercised end to end by a test, so a future change to member removal or |
| +account deletion cannot quietly reopen the path. |
| + |
| +This is also why the owner lives in the bucket rather than in a flag that |
| +tells the operator to run `console member -add` afterwards: the carlos binary |
| +must never open the console's database (console state is the console's |
| +authority, like grants), and a follow-up step only works if somebody runs it |
| +— which is precisely the step that got skipped. |
| + |
| The registry only ever *materializes* what the bucket says — it never |
| assigns. The bucket is the control plane; identity belongs there like |
| everything else. **Durability comes from minted-once-and-recorded, not |
diff --git a/e2e_test.go b/e2e_test.go| index f74431d1e..4c886df5b 100644 |
| --- a/e2e_test.go |
| +++ b/e2e_test.go |
| @@ -696,8 +696,36 @@ func TestAccountsCLI(t *testing.T) { |
| } |
| reg.Close() |
| + // FOUND IN PRODUCTION: create used to record an owner NOWHERE, so the |
| + // accounts kass and keymail were migrated into had no members — and |
| + // membership is authorization, so the operator could not open either one in |
| + // the console. -owner is required, and its absence has to name the flag |
| + // rather than print a shrug. |
| + failed := exec.Command(carlosBin, "accounts", "create", "-registry", regPath, "-name", "Kass") |
| + failed.Env = env |
| + refusal, err := failed.CombinedOutput() |
| + if err == nil { |
| + t.Fatalf("accounts create with no -owner succeeded — that is the ownerless account bug:\n%s", refusal) |
| + } |
| + if !strings.Contains(string(refusal), "-owner") { |
| + t.Errorf("refusal does not name the flag to pass:\n%s", refusal) |
| + } |
| + // And it refused BEFORE minting: a rejected create must not burn an id or |
| + // leave a half-made account in the bucket. |
| + if keys, err := store.List(ctx, "accounts/"); err != nil || len(keys) != 0 { |
| + t.Errorf("refused create left %v in the bucket", keys) |
| + } |
| + // A typo'd address is refused with the same rule the console's sign-in form |
| + // uses — an owner keymail could never authenticate is a door that does not |
| + // open either. |
| + failed = exec.Command(carlosBin, "accounts", "create", "-registry", regPath, "-name", "Kass", "-owner", "paul") |
| + failed.Env = env |
| + if refusal, err := failed.CombinedOutput(); err == nil { |
| + t.Errorf("accounts create accepted the non-address owner %q:\n%s", "paul", refusal) |
| + } |
| + |
| // create: a minted id, its frozen sqid printed for the operator to use. |
| - out := run("accounts", "create", "-registry", regPath, "-name", "Kass") |
| + out := run("accounts", "create", "-registry", regPath, "-name", "Kass", "-owner", "Paul@Keymail.dev") |
| m := regexp.MustCompile(`(?m)^\s*sqid:\s*(\S+)`).FindStringSubmatch(out) |
| if m == nil { |
| t.Fatalf("accounts create printed no sqid:\n%s", out) |
| @@ -706,10 +734,26 @@ func TestAccountsCLI(t *testing.T) { |
| if !regexp.MustCompile(`^[a-z]{3,16}$`).MatchString(sqid) { |
| t.Fatalf("created sqid = %q, want the frozen encoder's alphabet", sqid) |
| } |
| - // The manifest is in the bucket — that is where identity lives. |
| - if _, _, err := store.Get(ctx, "accounts/"+sqid+"/account.json"); err != nil { |
| + // The manifest is in the bucket — that is where identity lives, and now |
| + // where the OWNER lives too: the console materializes it into an owner |
| + // membership, so the carlos binary never has to open console state. |
| + body, _, err := store.Get(ctx, "accounts/"+sqid+"/account.json") |
| + if err != nil { |
| t.Fatalf("create wrote no account manifest: %v", err) |
| } |
| + var manifest domains.AccountManifest |
| + if err := json.Unmarshal(body, &manifest); err != nil { |
| + t.Fatal(err) |
| + } |
| + if manifest.Owner != "paul@keymail.dev" { |
| + t.Errorf("manifest owner = %q, want the normalized address — the console's memberships "+ |
| + "key is literal text, so a mixed-case owner is an owner who cannot sign in", manifest.Owner) |
| + } |
| + // The operator has to be able to see, from the output alone, that the |
| + // account is reachable and by whom. |
| + if !strings.Contains(out, "paul@keymail.dev") { |
| + t.Errorf("create does not say who owns the account:\n%s", out) |
| + } |
| if out := run("accounts", "list", "-registry", regPath); !strings.Contains(out, sqid) || |
| !strings.Contains(out, "Kass") || !strings.Contains(out, "ops") { |
| @@ -896,7 +940,7 @@ func TestAccountsMigrateTakesTheLiveObjects(t *testing.T) { |
| } |
| reg.Close() |
| - out, err := run("accounts", "create", "-registry", regPath, "-name", "Kass") |
| + out, err := run("accounts", "create", "-registry", regPath, "-name", "Kass", "-owner", "paul@keymail.dev") |
| if err != nil { |
| t.Fatalf("accounts create: %v\n%s", err, out) |
| } |
| @@ -1005,7 +1049,7 @@ func TestAccountsMigrateConvergesADivergedPointer(t *testing.T) { |
| } |
| reg.Close() |
| - out, err := run("accounts", "create", "-registry", regPath, "-name", "Kass") |
| + out, err := run("accounts", "create", "-registry", regPath, "-name", "Kass", "-owner", "paul@keymail.dev") |
| if err != nil { |
| t.Fatalf("accounts create: %v\n%s", err, out) |
| } |
diff --git a/internal/console/account_test.go b/internal/console/account_test.go| index 06ca4b103..9da1ff54e 100644 |
| --- a/internal/console/account_test.go |
| +++ b/internal/console/account_test.go |
| @@ -74,7 +74,7 @@ func TestConsoleMintsSequentialIdsIntoTheBucket(t *testing.T) { |
| ctx := t.Context() |
| var last int64 |
| for i := 0; i < 3; i++ { |
| - id, sqid, err := domains.MintAccount(ctx, rel, "Account", time.Now()) |
| + id, sqid, err := domains.MintAccount(ctx, rel, "Account", "", time.Now()) |
| if err != nil { |
| t.Fatal(err) |
| } |
| @@ -367,6 +367,137 @@ func TestBackfillPromotesNobodyWhenThereIsNoOperator(t *testing.T) { |
| } |
| } |
| +// AdoptManifestOwner is how a bucket manifest's `owner` becomes a membership. |
| +// It only ever fills a VACANCY: the manifest names the account's first owner, |
| +// and once an account has one, membership is the console's business and a |
| +// re-read of a write-once object must not touch it. |
| +func TestAdoptManifestOwnerOnlyEverFillsAVacancy(t *testing.T) { |
| + st := openTestStore(t) |
| + now := time.Unix(1700000000, 0) |
| + |
| + // The shape `carlos accounts create` leaves: a row with no members at all. |
| + if _, err := st.EnsureAccount(3, "bac", "Kass", now); err != nil { |
| + t.Fatal(err) |
| + } |
| + adopted, err := st.AdoptManifestOwner(3, "paul@keymail.dev", now) |
| + if err != nil || !adopted { |
| + t.Fatalf("AdoptManifestOwner = %v %v, want it to fill the vacancy", adopted, err) |
| + } |
| + if role, _ := st.Role(3, "paul@keymail.dev"); role != RoleOwner { |
| + t.Errorf("role after adoption = %q, want owner", role) |
| + } |
| + // Idempotent: every console start re-reads the same manifest, so a second |
| + // pass must report "nothing to do" rather than re-writing. |
| + if adopted, err := st.AdoptManifestOwner(3, "paul@keymail.dev", now); err != nil || adopted { |
| + t.Errorf("second adoption = %v %v, want false", adopted, err) |
| + } |
| + |
| + // An account that already has an owner is administered by the console now. |
| + // A manifest naming somebody else must NOT hand them authority — that would |
| + // make a write-once bucket object a standing grant, re-applied on every |
| + // restart, exactly the shape of the backfill bug above. |
| + if adopted, err := st.AdoptManifestOwner(3, "someone@keymail.dev", now); err != nil || adopted { |
| + t.Errorf("adoption against an owned account = %v %v, want false", adopted, err) |
| + } |
| + if role, _ := st.Role(3, "someone@keymail.dev"); role != "" { |
| + t.Errorf("a manifest gave %q to a stranger on an already-owned account", role) |
| + } |
| + if role, _ := st.Role(3, "paul@keymail.dev"); role != RoleOwner { |
| + t.Errorf("the existing owner was changed to %q", role) |
| + } |
| +} |
| + |
| +// The account has members but no OWNER — an invite was accepted (invites always |
| +// grant member) into an account `carlos accounts create` left ownerless. The |
| +// manifest's owner is the one the operator named, so they are promoted rather |
| +// than added twice. |
| +func TestAdoptManifestOwnerPromotesTheNamedMember(t *testing.T) { |
| + st := openTestStore(t) |
| + now := time.Unix(1700000000, 0) |
| + if _, err := st.EnsureAccount(3, "bac", "Kass", now); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := st.AddMember(3, "paul@keymail.dev", RoleMember, now); err != nil { |
| + t.Fatal(err) |
| + } |
| + adopted, err := st.AdoptManifestOwner(3, "paul@keymail.dev", now) |
| + if err != nil || !adopted { |
| + t.Fatalf("AdoptManifestOwner = %v %v, want the named member promoted", adopted, err) |
| + } |
| + if role, _ := st.Role(3, "paul@keymail.dev"); role != RoleOwner { |
| + t.Errorf("role = %q, want owner", role) |
| + } |
| +} |
| + |
| +// FOUND IN PRODUCTION, not in review — and it was undoing the migration it |
| +// ran alongside. |
| +// |
| +// `carlos accounts migrate` + `console migrate-app` moved jelly out of ops and |
| +// into account bac. The legacy `owners` grant for jelly stays readable on |
| +// purpose (the backfill's comment says so: the grants stay while the migration |
| +// is verified). The very next console RESTART re-ran this backfill, saw a grant |
| +// for "jelly", saw no jelly row in OPS, and created one — so the same app |
| +// existed in two accounts at once, and kass and keymail were queued to get the |
| +// same treatment on the restart after that. Every migration would have been |
| +// undone, indefinitely, by the ordinary act of restarting the console. |
| +// |
| +// The rule the SQL was missing: the backfill claims a name that nothing claims |
| +// yet. An app with a row in ANY account has already been placed — by the |
| +// migration, by the console, by a previous backfill — and placing it again is |
| +// not a claim, it is a duplicate. |
| +func TestBackfillNeverRecreatesAnAppThatLivesInAnotherAccount(t *testing.T) { |
| + st := openTestStore(t) |
| + now := time.Unix(1700000000, 0) |
| + const migrated = int64(3) |
| + if err := st.CreateAccount(migrated, "bac", "Jelly", "paul@keymail.dev", now); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := st.CreateAppIn(migrated, "jelly", "paul@keymail.dev", now); err != nil { |
| + t.Fatal(err) |
| + } |
| + // Exactly the live shape: the app is migrated, and its pre-accounts grant |
| + // is still sitting in the owners table waiting to be revoked. |
| + if err := st.GrantAt("dev@keymail.dev", "jelly", now); err != nil { |
| + t.Fatal(err) |
| + } |
| + // A grant-only app that was never migrated still has to be claimed in ops — |
| + // that is the job this function exists for, and the fix must not cost it. |
| + if err := st.GrantAt("dev@keymail.dev", "toy", now); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + if _, err := st.BackfillOpsMemberships(now.Add(time.Hour)); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + if ok, err := st.HasAppIn(OpsAccountID, "jelly"); err != nil || ok { |
| + t.Error("the backfill recreated jelly in ops — the migration is undone on every restart") |
| + } |
| + holders, err := st.AppAccounts("jelly") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if len(holders) != 1 || holders[0] != migrated { |
| + t.Errorf("jelly is claimed by accounts %v, want only %d", holders, migrated) |
| + } |
| + // The surviving row is the migrated one, unchanged: same owner, same |
| + // creation time. A backfill that "fixed" it by rewriting the row would be |
| + // the same bug wearing a different hat. |
| + var createdBy string |
| + var createdAt int64 |
| + if err := st.db.QueryRow(`SELECT created_by, created_at FROM apps WHERE app = 'jelly'`). |
| + Scan(&createdBy, &createdAt); err != nil { |
| + t.Fatal(err) |
| + } |
| + if createdBy != "paul@keymail.dev" || createdAt != now.Unix() { |
| + t.Errorf("migrated jelly row = (%s, %d), want it untouched (paul@keymail.dev, %d)", |
| + createdBy, createdAt, now.Unix()) |
| + } |
| + if ok, err := st.HasAppIn(OpsAccountID, "toy"); err != nil || !ok { |
| + t.Error("a grant-only app that was never migrated lost its ops claim") |
| + } |
| +} |
| + |
| // 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. |
| @@ -378,7 +509,12 @@ func TestGrantHolderAdviceIsCopyPasteable(t *testing.T) { |
| 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", |
| + // The -owner flag is required, so advice that omits it is advice that |
| + // fails when pasted. And the `console member -add … -role owner` step is |
| + // gone: the manifest owner materializes into the membership, which is |
| + // the point — a step an operator can skip is a step that produces an |
| + // account nobody can open, as it did in production. |
| + "carlos accounts create -name <name> -owner <address>", "console migrate-app", |
| } { |
| if !strings.Contains(msg, want) { |
| t.Errorf("advice does not mention %q: %s", want, msg) |
diff --git a/internal/console/materialize.go b/internal/console/materialize.go| index a8ec794a6..69f5df4e1 100644 |
| --- a/internal/console/materialize.go |
| +++ b/internal/console/materialize.go |
| @@ -51,7 +51,12 @@ func MaterializeAccounts(ctx context.Context, s release.Store, st *Store) (int, |
| continue |
| } |
| // Cheap short-circuit, and the reason a steady-state pass does no |
| - // writes at all: the console usually already knows every account. |
| + // writes and no object reads at all: the console usually already knows |
| + // every account. |
| + // |
| + // This is ALSO the security boundary for the owner field, so it must |
| + // stay an unconditional "already known, stop here". See materializeOwner |
| + // for why an account the console has seen before is never revisited. |
| if a, err := st.AccountBySqid(sqid); err != nil { |
| return n, err |
| } else if a != nil { |
| @@ -89,10 +94,79 @@ func MaterializeAccounts(ctx context.Context, s release.Store, st *Store) (int, |
| } |
| log.Printf("console: materialized account %s (%s) from the bucket", m.Sqid, m.Name) |
| n++ |
| + // ONLY here — on the pass that created the row, never on a later one. |
| + materializeOwner(st, key, m, created) |
| } |
| return n, nil |
| } |
| +// materializeOwner turns the manifest's `owner` into the account's first owner |
| +// membership — the console half of the fix for `carlos accounts create` making |
| +// unreachable accounts (see Store.AdoptManifestOwner for the production failure |
| +// this comes from). |
| +// |
| +// IT RUNS EXACTLY ONCE PER ACCOUNT: on the pass that first creates the console's |
| +// row, and never again. That is the whole security argument for the field, and |
| +// it is deliberately structural rather than a check inside this function. |
| +// |
| +// The reason is that the manifest is NOT write-once. MintAccount claims the |
| +// object with a create-only Put, but that is a convention of one code path, not |
| +// a property of the object: release.Store.Put takes its ifMatch from the caller, |
| +// the deployment bucket has no Object Lock, and the host IAM role holds |
| +// unconditional s3:PutObject across the bucket with nothing excluding |
| +// accounts/*/account.json. Anyone who can write to the bucket can rewrite a live |
| +// account's manifest. |
| +// |
| +// If adoption happened on any pass, that rewrite would BE an authority grant: |
| +// name yourself owner of a vacant account, wait for the next console start or |
| +// `console member` invocation — both run this pass — and you own a live account |
| +// and everything deployed in it. That is a real widening of the trust model, not |
| +// something bucket write access already implied: forging a channel pointer needs |
| +// CARLOS_RELEASE_KEY, which most bucket-writers do not hold, and this path needs |
| +// no key at all. AdoptManifestOwner's transactional vacancy check does not cover |
| +// it either — it is TOCTOU-safe, but the account was LEGITIMATELY vacant and it |
| +// is the manifest that changed afterwards. |
| +// |
| +// Adopting only at birth is safe precisely because `-owner` is now REQUIRED on |
| +// `carlos accounts create`: manifests are born carrying their owner, so there is |
| +// no legitimate case of one arriving later. |
| +// |
| +// THE TRADE-OFF, stated plainly: an owner added to a manifest AFTER the account |
| +// first materialized is deliberately NOT adopted, however well-meant the edit. |
| +// Editing the bucket is not how you grant console authority; `console member |
| +// -add <address> <sqid> -role owner` is, and it is box-side for exactly this |
| +// reason. The same applies to an account that materialized ownerless from a |
| +// pre-`owner` manifest — it stays ownerless until an operator says otherwise. |
| +// |
| +// Identity and authority stay SEPARATE decisions, which is why this runs after |
| +// the account row rather than inside EnsureAccount: a manifest with an owner |
| +// keymail could never authenticate still yields a perfectly good account. One |
| +// unusable string must not hide an account from the console entirely. |
| +// |
| +// Best-effort and never fatal, like the rest of this pass: an account that |
| +// materialized but could not be given its owner is recoverable with `console |
| +// member -add`, whereas a console that refuses to boot is not. |
| +func materializeOwner(st *Store, key string, m accountManifest, created time.Time) { |
| + owner := domains.NormalizeAddress(m.Owner) |
| + if owner == "" { |
| + return // ownerless by design: every manifest written before the field existed |
| + } |
| + if !domains.ValidAddress(owner) { |
| + log.Printf("console: %s names owner %q, which is not a keymail address — "+ |
| + "materializing the account without it; give it one with `console member -add <address> %s -role owner`", |
| + key, m.Owner, m.Sqid) |
| + return |
| + } |
| + adopted, err := st.AdoptManifestOwner(m.ID, owner, created) |
| + if err != nil { |
| + log.Printf("console: could not adopt %s as owner of %s: %v", owner, m.Sqid, err) |
| + return |
| + } |
| + if adopted { |
| + log.Printf("console: %s is the owner of %s (%s), from the account manifest", owner, m.Sqid, m.Name) |
| + } |
| +} |
| + |
| // materializeQuietly is the call site every entry point actually wants: catch |
| // up if we can, log and carry on if we cannot. Separated from |
| // MaterializeAccounts so the failure policy lives in ONE place rather than |
diff --git a/internal/console/materialize_test.go b/internal/console/materialize_test.go| index 302724968..1206fbc76 100644 |
| --- a/internal/console/materialize_test.go |
| +++ b/internal/console/materialize_test.go |
| @@ -1,6 +1,7 @@ |
| package console |
| import ( |
| + "encoding/json" |
| "testing" |
| "time" |
| @@ -132,6 +133,281 @@ func TestMaterializeNeverRewritesAKnownAccount(t *testing.T) { |
| } |
| } |
| +// FOUND IN PRODUCTION: `carlos accounts create` recorded an owner NOWHERE, so |
| +// the accounts kass and keymail were migrated into had no members — and |
| +// membership IS authorization, so the operator could not reach either of them |
| +// in the console at all. The account existed, the apps were in it, and the only |
| +// door was a `console member -add … -role owner` that nothing told anybody to |
| +// run. |
| +// |
| +// The fix keeps the boundary the carlos binary is held to (it must never open |
| +// this database): `create` names the owner in the BUCKET manifest, and this |
| +// pass — the console's own reader of that manifest — turns it into the owner |
| +// membership. Same object, same reader, on every box. |
| +func TestMaterializeGivesANewAccountTheManifestOwner(t *testing.T) { |
| + st := openTestStore(t) |
| + rel := release.NewDirStore(t.TempDir()) |
| + ctx := t.Context() |
| + |
| + // Exactly what `carlos accounts create -name Kass -owner paul@keymail.dev` |
| + // leaves behind, minted through the same allocator. |
| + id, sqid, err := domains.MintAccount(ctx, rel, "Kass", "paul@keymail.dev", time.Unix(1700000000, 0)) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if n, err := MaterializeAccounts(ctx, rel, st); err != nil || n != 1 { |
| + t.Fatalf("materialized %d (%v), want 1", n, err) |
| + } |
| + if role, _ := st.Role(id, "paul@keymail.dev"); role != RoleOwner { |
| + t.Errorf("owner role after materialize = %q, want owner — the account is unreachable without it", role) |
| + } |
| + // And it lands where the console looks it up from: the switcher. |
| + accts, err := st.AccountsFor("paul@keymail.dev") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if len(accts) != 1 || accts[0].Sqid != sqid || accts[0].Role != RoleOwner { |
| + t.Errorf("AccountsFor(owner) = %+v, want the new account as owner", accts) |
| + } |
| + // Idempotent: the account is known and owned, so a second pass does nothing. |
| + if n, err := MaterializeAccounts(ctx, rel, st); err != nil || n != 0 { |
| + t.Errorf("second pass materialized %d (%v), want 0", n, err) |
| + } |
| +} |
| + |
| +// THE MANIFEST IS NOT WRITE-ONCE, and the owner is only adopted at BIRTH. |
| +// |
| +// `MintAccount` claims the object with a create-only Put, but that is a |
| +// convention of one code path, not a property of the object: release.Store.Put |
| +// takes its ifMatch from the caller, the deployment bucket has no Object Lock, |
| +// and the host IAM role holds unconditional s3:PutObject over the whole bucket |
| +// with nothing excluding accounts/*/account.json. So anyone who can write to |
| +// the bucket can REWRITE a live account's manifest. |
| +// |
| +// If materialization adopted an owner on any pass, that rewrite would be an |
| +// authority grant: name yourself owner of a vacant account, wait for the next |
| +// console start or `console member` invocation — both run this pass — and you |
| +// hold owner over a live account with its apps in it. That is a genuine |
| +// widening of the trust model, not something bucket write access already |
| +// implied: forging a channel pointer needs CARLOS_RELEASE_KEY, which most |
| +// bucket-writers do not have, and this path needs no key at all. |
| +// |
| +// The transactional vacancy check in AdoptManifestOwner does not help here. It |
| +// is TOCTOU-safe, but the account was LEGITIMATELY vacant; the manifest is what |
| +// changed afterwards. |
| +// |
| +// So adoption happens once, when the console first creates the account row, and |
| +// never again. Safe now that -owner is required at create time: manifests are |
| +// born with their owner. |
| +func TestMaterializeNeverAdoptsAnOwnerAddedToAManifestAfterTheFact(t *testing.T) { |
| + st := openTestStore(t) |
| + rel := release.NewDirStore(t.TempDir()) |
| + ctx := t.Context() |
| + now := time.Unix(1700000000, 0) |
| + |
| + // A legitimately vacant account: an old manifest, from before `owner`. |
| + if err := WriteAccountManifest(ctx, rel, 42, domains.Sqid(42), "Kass", now); err != nil { |
| + t.Fatal(err) |
| + } |
| + if n, err := MaterializeAccounts(ctx, rel, st); err != nil || n != 1 { |
| + t.Fatalf("materialized %d (%v), want 1", n, err) |
| + } |
| + if members, _ := st.Members(42); len(members) != 0 { |
| + t.Fatalf("Members = %+v, want the account vacant to begin with", members) |
| + } |
| + |
| + // Somebody with plain bucket write access — no signing key — rewrites the |
| + // manifest naming themselves. |
| + rewriteManifest(t, rel, 42, "attacker@keymail.dev") |
| + // This pass runs on every console start and before every `console member`. |
| + if _, err := MaterializeAccounts(ctx, rel, st); err != nil { |
| + t.Fatal(err) |
| + } |
| + if role, _ := st.Role(42, "attacker@keymail.dev"); role != "" { |
| + t.Errorf("a rewritten manifest granted %q over a live account — bucket write became "+ |
| + "console authority, with no signing key involved", role) |
| + } |
| + if members, _ := st.Members(42); len(members) != 0 { |
| + t.Errorf("Members = %+v, want the account still vacant", members) |
| + } |
| +} |
| + |
| +// The same rule stated the other way, and the case an operator will actually |
| +// hit: an owner added to a manifest AFTER the account first materialized is |
| +// deliberately not adopted, however well-meant. The operator path for that is |
| +// `console member -add <address> <sqid> -role owner`, which is box-side |
| +// authority rather than bucket-side. |
| +func TestMaterializeLeavesALateManifestOwnerToTheOperator(t *testing.T) { |
| + st := openTestStore(t) |
| + rel := release.NewDirStore(t.TempDir()) |
| + ctx := t.Context() |
| + now := time.Unix(1700000000, 0) |
| + |
| + if err := WriteAccountManifest(ctx, rel, 42, domains.Sqid(42), "Kass", now); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := MaterializeAccounts(ctx, rel, st); err != nil { |
| + t.Fatal(err) |
| + } |
| + rewriteManifest(t, rel, 42, "paul@keymail.dev") |
| + if _, err := MaterializeAccounts(ctx, rel, st); err != nil { |
| + t.Fatal(err) |
| + } |
| + if role, _ := st.Role(42, "paul@keymail.dev"); role != RoleOwner { |
| + // The box-side verb is what fills it, and it still works. |
| + if err := st.AddMember(42, "paul@keymail.dev", RoleOwner, now); err != nil { |
| + t.Fatal(err) |
| + } |
| + } else { |
| + t.Error("a late manifest owner was adopted — the manifest is not write-once, " + |
| + "so that is a standing grant anyone with bucket write can issue") |
| + } |
| +} |
| + |
| +// THE COUPLING TEST. Once an owner exists, the only thing standing between "the |
| +// operator removed them" and "the manifest puts them back" is that the account |
| +// never becomes vacant again — and that is enforced by lastOwnerGuard, which |
| +// lives in member removal, not here. This diff leans on that guard implicitly; |
| +// exercising it end to end means a future change to removal (or to account |
| +// deletion) cannot silently reopen the adoption path. |
| +func TestARemovedOwnerIsNeverRegrantedByTheManifest(t *testing.T) { |
| + st := openTestStore(t) |
| + rel := release.NewDirStore(t.TempDir()) |
| + ctx := t.Context() |
| + now := time.Unix(1700000000, 0) |
| + |
| + id, _, err := domains.MintAccount(ctx, rel, "Kass", "paul@keymail.dev", now) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := MaterializeAccounts(ctx, rel, st); err != nil { |
| + t.Fatal(err) |
| + } |
| + if role, _ := st.Role(id, "paul@keymail.dev"); role != RoleOwner { |
| + t.Fatalf("role = %q, want the manifest owner adopted at birth", role) |
| + } |
| + |
| + // Link one: the account cannot be emptied. Removing the last owner is |
| + // refused, so there is no vacancy for a re-read to fill. |
| + if err := st.RemoveMember(id, "paul@keymail.dev"); err == nil { |
| + t.Fatal("removed the last owner — the account is now vacant and the manifest can refill it") |
| + } |
| + |
| + // Link two: with ownership transferred, the original CAN be removed — and |
| + // must stay removed across every subsequent pass, forever. |
| + if err := st.AddMember(id, "successor@keymail.dev", RoleOwner, now); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := st.RemoveMember(id, "paul@keymail.dev"); err != nil { |
| + t.Fatal(err) |
| + } |
| + for i := 0; i < 3; i++ { |
| + if _, err := MaterializeAccounts(ctx, rel, st); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + if role, _ := st.Role(id, "paul@keymail.dev"); role != "" { |
| + t.Errorf("the removed owner came back as %q — the manifest is behaving as a standing grant", role) |
| + } |
| + if role, _ := st.Role(id, "successor@keymail.dev"); role != RoleOwner { |
| + t.Errorf("successor role = %q, want owner", role) |
| + } |
| +} |
| + |
| +// The manifest is written ONCE and read on every start, so it must never be a |
| +// standing grant. An account that has an owner is administered by the console; |
| +// re-reading the object must not demote them, replace them, or add the |
| +// manifest's address alongside them. |
| +func TestMaterializeNeverReplacesAnExistingOwner(t *testing.T) { |
| + st := openTestStore(t) |
| + rel := release.NewDirStore(t.TempDir()) |
| + ctx := t.Context() |
| + now := time.Unix(1700000000, 0) |
| + |
| + id, sqid, err := domains.MintAccount(ctx, rel, "Kass", "original@keymail.dev", now) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + // Ownership was transferred in the console after creation. |
| + if err := st.CreateAccount(id, sqid, "Kass", "successor@keymail.dev", now); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := MaterializeAccounts(ctx, rel, st); err != nil { |
| + t.Fatal(err) |
| + } |
| + if role, _ := st.Role(id, "successor@keymail.dev"); role != RoleOwner { |
| + t.Errorf("the console's owner became %q after a materialize pass", role) |
| + } |
| + if role, _ := st.Role(id, "original@keymail.dev"); role != "" { |
| + t.Errorf("the manifest re-granted %q to the address it named at creation — "+ |
| + "a write-once object must not be a standing grant", role) |
| + } |
| +} |
| + |
| +// An owner address the sign-in form would refuse is a membership nobody can |
| +// ever claim. Skipped loudly, and the ACCOUNT still materializes: the identity |
| +// is sound even when the owner field is not, and refusing the whole manifest |
| +// would make one bad address hide an account from the console entirely. |
| +func TestMaterializeSkipsAnUnusableManifestOwner(t *testing.T) { |
| + st := openTestStore(t) |
| + rel := release.NewDirStore(t.TempDir()) |
| + ctx := t.Context() |
| + |
| + sqid := domains.Sqid(42) |
| + if _, err := rel.Put(ctx, domains.AccountManifestKey(sqid), |
| + []byte(`{"id":42,"sqid":"`+sqid+`","name":"Kass","owner":"not-an-address"}`), ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + if n, err := MaterializeAccounts(ctx, rel, st); err != nil || n != 1 { |
| + t.Fatalf("materialized %d (%v), want the account itself", n, err) |
| + } |
| + if members, _ := st.Members(42); len(members) != 0 { |
| + t.Errorf("Members = %+v, want nobody — an address keymail cannot authenticate is not an owner", members) |
| + } |
| +} |
| + |
| +// An old manifest — written before the field existed — has no owner and stays |
| +// ownerless. Additive means the pre-owner world is unchanged. |
| +func TestMaterializeLeavesAnOwnerlessManifestOwnerless(t *testing.T) { |
| + st := openTestStore(t) |
| + rel := release.NewDirStore(t.TempDir()) |
| + ctx := t.Context() |
| + if err := WriteAccountManifest(ctx, rel, 42, domains.Sqid(42), "Legacy", time.Unix(1700000000, 0)); err != nil { |
| + t.Fatal(err) |
| + } |
| + if n, err := MaterializeAccounts(ctx, rel, st); err != nil || n != 1 { |
| + t.Fatalf("materialized %d (%v), want 1", n, err) |
| + } |
| + if members, _ := st.Members(42); len(members) != 0 { |
| + t.Errorf("Members = %+v, want nobody — inventing an owner is minting authority from nothing", members) |
| + } |
| +} |
| + |
| +// rewriteManifest overwrites a live account manifest with one naming owner — |
| +// what a bucket-writer does, and the thing MintAccount's create-only Put does |
| +// NOT prevent anyone else from doing. The etag is read and passed because the |
| +// DirStore emulates compare-and-swap; S3 has no such constraint at all, so a |
| +// caller holding s3:PutObject simply writes the object. |
| +func rewriteManifest(t *testing.T, rel release.Store, id int64, owner string) { |
| + t.Helper() |
| + ctx := t.Context() |
| + sqid := domains.Sqid(id) |
| + key := domains.AccountManifestKey(sqid) |
| + _, etag, err := rel.Get(ctx, key) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + body, err := json.Marshal(domains.AccountManifest{ |
| + ID: id, Sqid: sqid, Name: "Kass", Owner: owner, CreatedAt: time.Unix(1700000000, 0).UTC(), |
| + }) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := rel.Put(ctx, key, body, etag); err != nil { |
| + t.Fatal(err) |
| + } |
| +} |
| + |
| // A deployment with no bucket configured at all still has to work: this is |
| // best-effort catch-up, not a precondition. |
| func TestMaterializeWithNoStoreIsANoOp(t *testing.T) { |
diff --git a/internal/console/server.go b/internal/console/server.go| index 011662a43..cd252ba3f 100644 |
| --- a/internal/console/server.go |
| +++ b/internal/console/server.go |
| @@ -378,7 +378,7 @@ func (s *Server) currentAccount(ctx context.Context, addr string) (string, error |
| // next rebuild — worse than a "new account" button that reports the outage. |
| func (s *Server) mintAccount(ctx context.Context, name, ownerAddr string) (*Account, error) { |
| now := time.Now() |
| - id, sqid, err := domains.MintAccount(ctx, s.cfg.Releases, name, now) |
| + id, sqid, err := domains.MintAccount(ctx, s.cfg.Releases, name, ownerAddr, now) |
| if err != nil { |
| return nil, err |
| } |
| @@ -668,10 +668,14 @@ func (s *Server) handleContinue(w http.ResponseWriter, r *http.Request) { |
| // (`console member`) must reject a typo with the SAME rule the web surface |
| // uses — a membership created from the command line for an address the sign-in |
| // form would refuse is a grant nobody can ever claim. |
| -func ValidAddress(a string) bool { |
| - i := strings.LastIndex(a, "@") |
| - return i > 0 && i < len(a)-1 && !strings.ContainsAny(a, " \t\r\n") |
| -} |
| +// |
| +// The rule itself now lives in internal/domains, next to the account manifest's |
| +// `owner` field, so `carlos accounts create -owner` can apply it too. The carlos |
| +// binary must never open the console's database, but it must not invent a |
| +// second definition of a valid address either: an address one tool accepts and |
| +// another refuses is exactly how an owner ends up recorded and unable to sign |
| +// in. Delegating keeps one rule with two doors, not two rules. |
| +func ValidAddress(a string) bool { return domains.ValidAddress(a) } |
| // handleSignin starts the one-time OAuth ceremony: state + PKCE verifier in |
| // a short-lived cookie, browser off to Keymail's consent page. |
diff --git a/internal/console/store.go b/internal/console/store.go| index 7fb7cb82f..e4c3e08ec 100644 |
| --- a/internal/console/store.go |
| +++ b/internal/console/store.go |
| @@ -365,8 +365,30 @@ SELECT ?, address, ?, ? FROM users WHERE operator != 0`, |
| // 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. |
| + // |
| + // The NOT IN is not a tidy-up — it is the fix for a bug that was live on the |
| + // flagship, and it was found by running the migration in production, not by |
| + // reading this code. `carlos accounts migrate` + `console migrate-app` moved |
| + // jelly out of ops into account bac. The migration deliberately leaves the |
| + // legacy grant readable (see the note above about not deleting the owners |
| + // table). The next console RESTART ran this statement, found a grant for |
| + // "jelly", found no jelly row in OPS — because it had just been moved out — |
| + // and inserted one. The same app then existed in two accounts at once, and |
| + // kass and keymail were lined up for the same on the following restart. The |
| + // migration was being undone, indefinitely, by restarting the console; it |
| + // took a hand-deleted row and hand-revoked grants to stop it. |
| + // |
| + // ON CONFLICT could never have caught this: the primary key is |
| + // (account_id, app), so an ops row and a bac row for the same app do not |
| + // conflict — they are, by the schema's own rules, two different apps. The |
| + // claim has to be conditioned on the app name being unclaimed EVERYWHERE, |
| + // which is what the backfill actually means: take the names nothing has |
| + // placed yet. An app that already has a row somewhere has been placed, and |
| + // the row that placed it is the authority — never this sweep. |
| 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 { |
| +SELECT o.app, ?, MIN(o.address), ? FROM owners o |
| +WHERE o.app NOT IN (SELECT app FROM apps) |
| +GROUP BY o.app`, OpsAccountID, now.Unix()); err != nil { |
| return nil, err |
| } |
| // Who is left over: grant holders who are not operators and hold no ops |
| @@ -424,8 +446,15 @@ func grantHolderAdvice(holders []GrantHolder) string { |
| } |
| fmt.Fprintf(&b, "%s (%s)", h.Address, strings.Join(h.Apps, " ")) |
| } |
| - b.WriteString(". Give each one their own with: carlos accounts create -name <name> " + |
| - "&& console member -add <address> <sqid> -role owner " + |
| + // Two commands, not three: `accounts create -owner` now records the owner in |
| + // the account manifest and the console materializes it into an owner |
| + // membership, so the `console member -add … -role owner` step this advice |
| + // used to carry is gone. It was never optional — an operator who skipped it |
| + // got an account nobody could open, which is exactly what happened on the |
| + // flagship — and the surest way to stop a step being skipped is to not need |
| + // it. The -owner flag is REQUIRED, so it has to appear here or the copied |
| + // command fails. |
| + b.WriteString(". Give each one their own with: carlos accounts create -name <name> -owner <address> " + |
| "&& console migrate-app -app <app> -to <sqid>") |
| return b.String() |
| } |
| @@ -492,12 +521,17 @@ func (s *Store) CreateAccount(id int64, sqid, name, ownerAddress string, now tim |
| // created the row. |
| // |
| // Unlike CreateAccount it deliberately creates no membership, and that is not |
| -// an oversight: `carlos accounts create` records no owner anywhere, so the |
| -// console has nobody to name. Inventing one — the invoking operator, the first |
| -// user, anyone — would be minting authority out of thin air over an account |
| -// this box knows nothing about. An ownerless row is honest, harmless (no |
| -// membership means no access), and exactly what `console member -add <addr> |
| -// <sqid> -role owner` is for. |
| +// an oversight: this call knows only what a manifest's identity fields say. |
| +// Inventing an owner — the invoking operator, the first user, anyone — would be |
| +// minting authority out of thin air over an account this box knows nothing |
| +// about. An ownerless row is honest and harmless (no membership means no |
| +// access). |
| +// |
| +// Where the owner DOES come from is the manifest's own `owner` field, applied |
| +// separately by AdoptManifestOwner, so identity and authority stay two distinct |
| +// decisions: a manifest with a broken owner still materializes an account, and |
| +// an old manifest with no owner at all still materializes exactly as it did |
| +// before the field existed. |
| // |
| // INSERT OR IGNORE, never REPLACE: an account the console already knows must |
| // keep its id, its name and its members. |
| @@ -511,6 +545,71 @@ func (s *Store) EnsureAccount(id int64, sqid, name string, created time.Time) (a |
| return n > 0, err |
| } |
| +// AdoptManifestOwner turns an account manifest's `owner` field into the owner |
| +// membership — the console half of the fix for a PRODUCTION failure, found by |
| +// running the migration on the flagship and not by reading this code. |
| +// |
| +// What happened: `carlos accounts create` recorded an owner nowhere at all. The |
| +// bucket manifest and the registry row both landed, and the account had no |
| +// members. Membership IS authorization here, so after kass and keymail were |
| +// migrated into fresh accounts the operator could not open either of them in |
| +// the console — the account existed, held the apps, and had no door. The only |
| +// way in was a `console member -add … -role owner` that nothing told anybody to |
| +// run. |
| +// |
| +// The carlos binary still does not open this database — that boundary is |
| +// deliberate and was enforced in an earlier review. Instead the manifest, which |
| +// is already the one place account identity crosses between the CLI, the |
| +// console and every box, now carries the intended owner, and this is where the |
| +// console reads it. |
| +// |
| +// It fills a VACANCY and nothing else. The manifest is re-read on every console |
| +// start, so treating it as an instruction would make it a standing grant: an |
| +// address removed by an operator would come back on the next restart, which is |
| +// precisely the shape of the app-backfill bug this same production run turned |
| +// up. So: an account that already has ANY owner is left completely alone, and |
| +// only an ownerless one is filled. Returns whether it wrote anything. |
| +// |
| +// This is the SECOND of two independent guards, and deliberately not the load |
| +// bearing one. The manifest is not write-once — it can be rewritten by anyone |
| +// with bucket write access — so the caller only ever invokes this on the pass |
| +// that first creates the account row (see materializeOwner). That structural |
| +// rule is what stops a rewritten manifest granting authority; this vacancy |
| +// check is what stops a race between the row insert and this write, and what |
| +// keeps the function safe to reason about on its own. |
| +// |
| +// The named address may already be a plain member (an accepted invite always |
| +// grants member) — then they are promoted, rather than the account gaining a |
| +// second row for the same person. |
| +func (s *Store) AdoptManifestOwner(accountID int64, address string, now time.Time) (bool, error) { |
| + tx, err := s.db.Begin() |
| + if err != nil { |
| + return false, err |
| + } |
| + defer tx.Rollback() |
| + // Re-counted INSIDE the transaction rather than trusting HasOwner: that one |
| + // is an optimization, this one is the rule, and a concurrent `console member |
| + // -add … -role owner` must not be able to slip between the two. |
| + var owners int |
| + if err := tx.QueryRow(`SELECT COUNT(*) FROM memberships WHERE account_id = ? AND role = ?`, |
| + accountID, RoleOwner).Scan(&owners); err != nil { |
| + return false, err |
| + } |
| + if owners > 0 { |
| + return false, nil |
| + } |
| + if _, err := tx.Exec(`INSERT INTO memberships (account_id, address, role, added_at) |
| +VALUES (?, ?, ?, ?) |
| +ON CONFLICT(account_id, address) DO UPDATE SET role = excluded.role`, |
| + accountID, address, RoleOwner, now.Unix()); err != nil { |
| + return false, err |
| + } |
| + if err := tx.Commit(); err != nil { |
| + return false, err |
| + } |
| + return true, nil |
| +} |
| + |
| // AccountBySqid returns the account with that public identity, or (nil, nil) |
| // when there is none — "no such account" is an answer, not an error, and the |
| // caller renders it as the same 404 a nonexistent page gets. |
diff --git a/internal/domains/mint.go b/internal/domains/mint.go| index 41ab52da8..b3109ee02 100644 |
| --- a/internal/domains/mint.go |
| +++ b/internal/domains/mint.go |
| @@ -11,6 +11,7 @@ import ( |
| "encoding/json" |
| "errors" |
| "fmt" |
| + "strings" |
| "time" |
| "github.com/carlosframework/platform/internal/release" |
| @@ -26,6 +27,46 @@ type AccountManifest struct { |
| Sqid string `json:"sqid"` |
| Name string `json:"name"` |
| CreatedAt time.Time `json:"created_at"` |
| + // Owner is the keymail address that should hold the first owner membership |
| + // of this account. ADDITIVE and omitempty: every manifest written before |
| + // this field existed has no owner and stays ownerless, exactly as it |
| + // materialized before. |
| + // |
| + // It is here — in the bucket, not in a console table — because the bucket is |
| + // the control plane, and this field exists to fix a PRODUCTION failure. |
| + // `carlos accounts create` recorded an owner nowhere at all, so after kass |
| + // and keymail were migrated into fresh accounts the operator could not reach |
| + // either of them in the console: the account existed, had no members, and |
| + // membership IS authorization. The only way in was hand-running `console |
| + // member -add … -role owner`, which nothing told anybody to do. |
| + // |
| + // Putting it in the manifest rather than teaching the carlos binary to open |
| + // the console database keeps a boundary that was deliberately drawn and |
| + // enforced in review: console state is the console's authority. Every box |
| + // that materializes this object converges on the same owner, so a |
| + // CLI-created account and a console-created one end up in the same state |
| + // instead of two. |
| + // |
| + // Always NORMALIZED (NormalizeAddress) before it is written: the console's |
| + // memberships primary key is literal text, so an unnormalized address here |
| + // would materialize a membership the real session can never match. |
| + Owner string `json:"owner,omitempty"` |
| +} |
| + |
| +// NormalizeAddress is the one normalization every keymail address gets before |
| +// it is stored or compared: trimmed and lowercased. Defined here, next to the |
| +// manifest field that carries one, so the CLI can apply the console's rule |
| +// without importing — or opening — console state. |
| +func NormalizeAddress(a string) string { return strings.ToLower(strings.TrimSpace(a)) } |
| + |
| +// ValidAddress is the platform's one shape check on a keymail address: |
| +// <local>@<host>, no whitespace. console.ValidAddress delegates to it, so the |
| +// sign-in form, `console member -add` and `carlos accounts create -owner` |
| +// cannot drift into accepting different strings — an address one of them takes |
| +// and another refuses is a membership somebody can create and nobody can use. |
| +func ValidAddress(a string) bool { |
| + i := strings.LastIndex(a, "@") |
| + return i > 0 && i < len(a)-1 && !strings.ContainsAny(a, " \t\r\n") |
| } |
| // AccountManifestKey is where an account's identity lives in the bucket. |
| @@ -112,14 +153,28 @@ func NextAccountID(ctx context.Context, s release.Store) (int64, error) { |
| // and that ordering is deliberate: a local row whose manifest never landed is |
| // an account that vanishes on the next rebuild, while a manifest with no local |
| // row is materialized back on the next pass. |
| -func MintAccount(ctx context.Context, s release.Store, name string, created time.Time) (int64, string, error) { |
| +// |
| +// owner is the address that should hold the account's first owner membership. |
| +// "" mints an ownerless account, which is what every pre-owner manifest is and |
| +// stays; anything else must be a usable keymail address, checked HERE rather |
| +// than at each caller because the manifest is written once and read forever — |
| +// a malformed owner would be permanent, and the id it consumed is not |
| +// reusable. The check happens before the first listing, so a refused mint |
| +// writes nothing at all. |
| +func MintAccount(ctx context.Context, s release.Store, name, owner string, created time.Time) (int64, string, error) { |
| + owner = NormalizeAddress(owner) |
| + if owner != "" && !ValidAddress(owner) { |
| + return 0, "", fmt.Errorf("domains: %q is not a keymail address — expected <user>@<host>, e.g. paul@keymail.dev", owner) |
| + } |
| for attempt := 0; attempt < mintAttempts; attempt++ { |
| id, err := NextAccountID(ctx, s) |
| if err != nil { |
| return 0, "", err |
| } |
| sqid := Sqid(id) |
| - body, err := json.Marshal(AccountManifest{ID: id, Sqid: sqid, Name: name, CreatedAt: created.UTC()}) |
| + body, err := json.Marshal(AccountManifest{ |
| + ID: id, Sqid: sqid, Name: name, CreatedAt: created.UTC(), Owner: owner, |
| + }) |
| if err != nil { |
| return 0, "", err |
| } |
diff --git a/internal/domains/mint_test.go b/internal/domains/mint_test.go| index b27ed74c2..5114b31f2 100644 |
| --- a/internal/domains/mint_test.go |
| +++ b/internal/domains/mint_test.go |
| @@ -76,7 +76,7 @@ func TestMintAccountLosesTheRaceAndTakesTheNextID(t *testing.T) { |
| writeManifest(t, inner, 2, domains.Sqid(2)) |
| }} |
| - id, sqid, err := domains.MintAccount(ctx, s, "Kass", time.Unix(1700000000, 0)) |
| + id, sqid, err := domains.MintAccount(ctx, s, "Kass", "", time.Unix(1700000000, 0)) |
| if err != nil { |
| t.Fatal(err) |
| } |
| @@ -122,7 +122,7 @@ func TestMintAccountGivesUpLoudly(t *testing.T) { |
| writeManifest(t, inner, next, domains.Sqid(next)) |
| next++ |
| }} |
| - _, _, err := domains.MintAccount(ctx, s, "Kass", time.Now()) |
| + _, _, err := domains.MintAccount(ctx, s, "Kass", "", time.Now()) |
| if err == nil { |
| t.Fatal("minting against a permanently racing store succeeded") |
| } |
| @@ -131,6 +131,97 @@ func TestMintAccountGivesUpLoudly(t *testing.T) { |
| } |
| } |
| +// FOUND IN PRODUCTION: `carlos accounts create` recorded an owner NOWHERE, so |
| +// the accounts kass and keymail were migrated into were unreachable through the |
| +// console until someone ran `console member -add … -role owner` by hand. The |
| +// operator hit exactly that and could not open either account. |
| +// |
| +// The manifest is where the fix lives, because the bucket is the control plane: |
| +// whoever materializes the account — this box's console, or any other box's — |
| +// reads the same object and lands on the same owner. An account created from |
| +// the CLI and one created from the console now converge on the same end state. |
| +func TestMintAccountRecordsTheOwnerInTheManifest(t *testing.T) { |
| + ctx := t.Context() |
| + s := release.NewDirStore(t.TempDir()) |
| + _, sqid, err := domains.MintAccount(ctx, s, "Kass", " Paul@Keymail.dev ", time.Unix(1700000000, 0)) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + body, _, err := s.Get(ctx, domains.AccountManifestKey(sqid)) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + var m domains.AccountManifest |
| + if err := json.Unmarshal(body, &m); err != nil { |
| + t.Fatal(err) |
| + } |
| + // NORMALIZED at the mint, not at the reader: the console's memberships key |
| + // is literal text with no COLLATE NOCASE, so a manifest carrying |
| + // "Paul@Keymail.dev" would materialize a membership the real session can |
| + // never match — an owner who still cannot get in, which is the very failure |
| + // this field exists to end. |
| + if m.Owner != "paul@keymail.dev" { |
| + t.Errorf("manifest owner = %q, want the trimmed, lowercased address", m.Owner) |
| + } |
| +} |
| + |
| +// Absent is legal and means ownerless: every manifest written before this field |
| +// existed has no owner, and they must keep materializing exactly as they did. |
| +func TestMintAccountLeavesTheOwnerOutWhenThereIsNone(t *testing.T) { |
| + ctx := t.Context() |
| + s := release.NewDirStore(t.TempDir()) |
| + _, sqid, err := domains.MintAccount(ctx, s, "Ownerless", "", time.Unix(1700000000, 0)) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + body, _, err := s.Get(ctx, domains.AccountManifestKey(sqid)) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if strings.Contains(string(body), "owner") { |
| + t.Errorf("manifest carries an owner key for an ownerless account: %s", body) |
| + } |
| +} |
| + |
| +// A manifest owner that keymail could never authenticate is a grant nobody can |
| +// claim — the same reason `console member` refuses a malformed address. Caught |
| +// at the MINT, because the manifest is written once and read forever: a bad |
| +// address here is permanent, and the id it consumed is not reusable. |
| +func TestMintAccountRefusesAnUnusableOwner(t *testing.T) { |
| + ctx := t.Context() |
| + for _, bad := range []string{"paul", "@keymail.dev", "paul@", "paul @keymail.dev"} { |
| + s := release.NewDirStore(t.TempDir()) |
| + if _, _, err := domains.MintAccount(ctx, s, "Kass", bad, time.Now()); err == nil { |
| + t.Errorf("MintAccount with owner %q succeeded", bad) |
| + } |
| + // And nothing was written: a refused mint must not burn an id or leave |
| + // an account behind. |
| + if keys, err := s.List(ctx, "accounts/"); err != nil || len(keys) != 0 { |
| + t.Errorf("owner %q left %v in the bucket", bad, keys) |
| + } |
| + } |
| +} |
| + |
| +// One rule, in one place. The console's sign-in form, `console member -add` and |
| +// now `carlos accounts create` must all accept and reject the same strings — |
| +// an address one of them takes and another refuses is a membership somebody |
| +// can create and nobody can use. |
| +func TestAddressNormalizationAndValidity(t *testing.T) { |
| + if got := domains.NormalizeAddress(" Paul@Keymail.dev\n"); got != "paul@keymail.dev" { |
| + t.Errorf("NormalizeAddress = %q", got) |
| + } |
| + for _, ok := range []string{"paul@keymail.dev", "a@b"} { |
| + if !domains.ValidAddress(ok) { |
| + t.Errorf("ValidAddress(%q) = false", ok) |
| + } |
| + } |
| + for _, bad := range []string{"", "paul", "@keymail.dev", "paul@", "paul@key mail.dev"} { |
| + if domains.ValidAddress(bad) { |
| + t.Errorf("ValidAddress(%q) = true", bad) |
| + } |
| + } |
| +} |
| + |
| func writeManifest(t *testing.T, s release.Store, id int64, sqid string) { |
| t.Helper() |
| body := []byte(`{"id":` + itoa(id) + `,"sqid":"` + sqid + `","name":"Competitor",` + |