amadan
Merge pull request #414 from carlosframework/email-domains-rm
email: release a sending-domain claim without deleting the app
9 files changed,
+1393
−110
docs/site/cli.md+13 −0internal/console/apimail.go+129 −7internal/console/apimail_test.go+196 −3internal/console/server.go+1 −1internal/mail/domainclaim.go+23 −11internal/mail/walk.go+335 −83internal/mail/walk_domainremove_test.go+442 −0mail_cli.go+160 −4mail_cli_test.go+94 −1
CI passed — run details
diff --git a/docs/site/cli.md b/docs/site/cli.md| index 6abbbc90f..e9f387255 100644 |
| --- a/docs/site/cli.md |
| +++ b/docs/site/cli.md |
| @@ -1100,6 +1100,19 @@ keeps polling until SES confirms them, giving up after ten minutes unless |
| SES never confirmed. `carlos email domains add` is that same half on its own, |
| for a second domain on an app already declared. |
| +`carlos email domains rm --app hello example.com` is the way back out. A |
| +sending domain can be claimed by exactly one CARLOS account at a time, so a |
| +domain one app holds is unavailable to everybody else — including the |
| +account that actually controls its DNS — until something releases it. `rm` |
| +is that something: it stops the app sending from the domain and, when it was |
| +the last app using it, deletes the SES identity and releases the claim, so |
| +another account can add and verify the domain. It refuses while the domain is |
| +where the app's own From address sits — move that first with `carlos email |
| +enable --app hello --from <addr>`. A custom domain's DNS records are yours, |
| +so `rm` prints them rather than deleting them: the DKIM CNAMEs are dead with |
| +the identity, while the MX, SPF and DMARC records stay correct if you ever |
| +add the domain back. |
| + |
| `carlos email test` sends a real message and reports what SES said about it. |
| It sends through a throwaway standalone credential it mints and revokes |
| around the send, so it never needs the delivered credential's password, and |
diff --git a/internal/console/apimail.go b/internal/console/apimail.go| index 5a22ae60a..baf362816 100644 |
| --- a/internal/console/apimail.go |
| +++ b/internal/console/apimail.go |
| @@ -499,7 +499,7 @@ func (s *Server) mailWalkReply(ctx context.Context, w http.ResponseWriter, acct |
| writeJSON(w, http.StatusOK, out) |
| } |
| -// registerEmailRoutes wires this file's nine doors onto s.mux, through the |
| +// registerEmailRoutes wires this file's ten doors onto s.mux, through the |
| // role gate (cliRoute) like every other /api/cli/ door. It stays a separate |
| // function from registerCLIRoutes only so the email family's reasoning lives |
| // beside the handlers it describes; svcroutes_test's AST pass is |
| @@ -507,12 +507,19 @@ func (s *Server) mailWalkReply(ctx context.Context, w http.ResponseWriter, acct |
| // |
| // THE ROLE TABLE, and why each door reads as it does. |
| // |
| -// The six OPEN doors take Allow(Admin), matching the store family verb for |
| -// verb — declare, ensure, verify, enable, rotate all move an account's |
| -// backend estate, and `enable` mints a path-scoped IAM user exactly as |
| -// `store grant` does. Admin is the union role, so Publish/Instance/Operate |
| -// are refused: a credential that may deploy an app must not thereby be able |
| -// to give it a sending identity on a domain the account owns. |
| +// The seven OPEN doors take Allow(Admin), matching the store family verb |
| +// for verb — declare, ensure, verify, REMOVE, enable, rotate all move an |
| +// account's backend estate, and `enable` mints a path-scoped IAM user |
| +// exactly as `store grant` does. Admin is the union role, so |
| +// Publish/Instance/Operate are refused: a credential that may deploy an app |
| +// must not thereby be able to give it a sending identity on a domain the |
| +// account owns. |
| +// |
| +// `domains rm` is Admin because `domains add` is: the same authority, run |
| +// backwards. It is deliberately NOT closed or operator-gated — the whole |
| +// defect it exists to fix is that releasing a claim required an authority |
| +// (permanent app deletion) far larger than claiming one ever did, which is |
| +// what left real deployments with a domain nobody could ever free. |
| // |
| // GET .../email is Admin rather than a read role for the same reason GET |
| // .../store is: the status reply names the IAM user, the key id and the |
| @@ -561,6 +568,7 @@ func (s *Server) registerEmailRoutes() { |
| s.cliRoute("DELETE /api/cli/apps/{account}/{app}/email/events/push", svcrole.Allow(svcrole.Admin), s.handleAPIEmailPushOff) |
| s.cliRoute("POST /api/cli/apps/{account}/{app}/email/domains", svcrole.Allow(svcrole.Admin), s.handleAPIEmailDomainEnsure) |
| s.cliRoute("POST /api/cli/apps/{account}/{app}/email/domains/{domain}/verify", svcrole.Allow(svcrole.Admin), s.handleAPIEmailDomainVerify) |
| + s.cliRoute("DELETE /api/cli/apps/{account}/{app}/email/domains/{domain}", svcrole.Allow(svcrole.Admin), s.handleAPIEmailDomainRemove) |
| s.cliRoute("POST /api/cli/apps/{account}/{app}/email/enable", svcrole.Allow(svcrole.Admin), s.handleAPIEmailEnable) |
| s.cliRoute("POST /api/cli/apps/{account}/{app}/email/rotate", svcrole.Allow(svcrole.Admin), s.handleAPIEmailRotate) |
| s.cliRoute("POST /api/cli/apps/{account}/{app}/email/credentials", svcrole.Set{}, s.handleAPIEmailCredentialsCreate) |
| @@ -1101,6 +1109,120 @@ func (s *Server) handleAPIEmailDomainVerify(w http.ResponseWriter, r *http.Reque |
| writeJSON(w, http.StatusOK, map[string]any{"domain": rec}) |
| } |
| +// handleAPIEmailDomainRemove answers `carlos email domains rm`: stop this |
| +// app sending from a domain and, when it was the last app doing so, give |
| +// the domain's GLOBAL claim back. |
| +// |
| +// THE DOOR THE CLAIM ALWAYS NEEDED. The claim (internal/mail/domainclaim.go) |
| +// is deliberately a create-only, globally-keyed object so two accounts |
| +// cannot both hold one domain — right, and unchanged by this door. What was |
| +// missing was the way out: until this existed, mail.Teardown, reached only |
| +// by PERMANENTLY DELETING the whole app, was the one caller that could ever |
| +// release a claim, so a domain claimed by the wrong app stayed unreachable |
| +// to every other account — including the account that actually controls its |
| +// DNS — for the life of the deployment. |
| +// |
| +// A MEMBER VERB, Admin like `domains add`, for the same reason: it spends |
| +// nothing that exceeds the account's tier, and it is the exact inverse of a |
| +// door an admin already has. It can never touch an estate outside the |
| +// caller's own account — mail.RemoveDomain reads and releases only through |
| +// the owner-gated releaseDomainClaim, and a claim held by anyone else is |
| +// refused in the same words as a domain that does not exist. |
| +// |
| +// Account-scoped like the record itself (domain.go's package comment), so |
| +// {app} is the membership gate AND — unlike verify — the referrer actually |
| +// being removed. The one exception is a claim with no record at all, where |
| +// there is no referrer set to be in; mail.removeOrphanedClaim documents why |
| +// the gate reading is the right one there. |
| +func (s *Server) handleAPIEmailDomainRemove(w http.ResponseWriter, r *http.Request) { |
| + addr, acct, app, ok := s.apiAccountApp(w, r) |
| + if !ok { |
| + return |
| + } |
| + if !s.mailWriteGates(w, addr) { |
| + return |
| + } |
| + domain := strings.ToLower(strings.TrimSpace(r.PathValue("domain"))) |
| + if !mail.ValidDomain(domain) { |
| + jsonError(w, http.StatusBadRequest, fmt.Sprintf("invalid domain %q", r.PathValue("domain"))) |
| + return |
| + } |
| + ctx, cancel := context.WithTimeout(r.Context(), mailDomainTimeout) |
| + defer cancel() |
| + d, writer := s.mailDeploymentFor() |
| + out, err := mail.RemoveDomain(ctx, s.cfg.Releases, s.cfg.MailProvisioner, writer, d, acct.Sqid, app, domain) |
| + if err != nil { |
| + // The From refusal is a 409, not a 400: it is a conflict with state |
| + // the caller can change (move the From, then remove the domain), |
| + // which is precisely what this message has to say. Contrast the |
| + // ensure door's 409, which is permanent — nothing the caller does |
| + // clears another account's claim. |
| + if errors.Is(err, mail.ErrDomainIsFrom) { |
| + jsonError(w, http.StatusConflict, s.mailFromConflictMessage(ctx, acct, app, domain)) |
| + return |
| + } |
| + // 404 covers both "this app does not use that domain" and "that |
| + // domain's claim belongs to another account", deliberately in the |
| + // same words — see mail.ErrDomainNotReferenced. |
| + if errors.Is(err, mail.ErrDomainNotReferenced) { |
| + jsonError(w, http.StatusNotFound, fmt.Sprintf( |
| + "no sending domain %q for this app — `carlos email status --app %s` lists the ones it has", domain, app)) |
| + return |
| + } |
| + if out != nil { |
| + // The removal LANDED; only the departing app's IAM policy |
| + // shrink failed (mail.RemoveDomain's own doc comment). Saying |
| + // "re-run" here would be wrong — a re-run answers 404 now — |
| + // so name the verb that actually repays the debt. |
| + log.Printf("console: api email domain remove %s/%s (%s): policy refresh: %v", acct.Sqid, app, domain, err) |
| + s.audit(acct, app, addr, "email-domain-remove", domain+" (cli; policy refresh failed)") |
| + jsonError(w, http.StatusBadGateway, |
| + "the domain was removed, but this app's sending policy could not be rewritten — run `carlos email enable --app "+app+"` to repair it") |
| + return |
| + } |
| + log.Printf("console: api email domain remove %s/%s (%s): %v", acct.Sqid, app, domain, err) |
| + jsonError(w, http.StatusBadGateway, |
| + "could not remove the sending domain — nothing unrecorded was left behind; re-run to resume") |
| + return |
| + } |
| + detail := domain + " (" + out.Kind + ")" |
| + switch { |
| + case out.ClaimOnly: |
| + detail += " released a claim with no domain record" |
| + case out.TornDown: |
| + detail += " torn down, claim released" |
| + default: |
| + detail += " still used by " + strings.Join(out.Remaining, ", ") |
| + } |
| + s.audit(acct, app, addr, "email-domain-remove", detail+" (cli)") |
| + // No bumpSerial: nothing in the app's own config changed — the delivered |
| + // SMTP variables are untouched by a domain leaving the granted set, and |
| + // the app's From is guaranteed by the refusal above to be somewhere |
| + // else. |
| + writeJSON(w, http.StatusOK, map[string]any{ |
| + "app": app, |
| + "domain": out.Domain, |
| + "kind": out.Kind, |
| + "torn_down": out.TornDown, |
| + "claim_only": out.ClaimOnly, |
| + "remaining_apps": out.Remaining, |
| + "orphaned": dnsRecordsOut(out.Orphaned), |
| + }) |
| +} |
| + |
| +// mailFromConflictMessage names the From that blocks a domain removal, and |
| +// the command that moves it. Re-reading the record on this one error path |
| +// costs nothing and is the difference between a member knowing what to do |
| +// next and being told only that they may not. |
| +func (s *Server) mailFromConflictMessage(ctx context.Context, acct *Account, app, domain string) string { |
| + msg := fmt.Sprintf("this app still sends from %s", domain) |
| + if rec, _, err := mail.Get(ctx, s.cfg.Releases, acct.Sqid, app); err == nil && rec != nil && rec.Declared != nil { |
| + msg = fmt.Sprintf("this app still sends as %s", rec.Declared.From) |
| + } |
| + return msg + fmt.Sprintf(" — move its From to another verified domain first"+ |
| + " (`carlos email enable --app %s --from <addr>`), then remove %s", app, domain) |
| +} |
| + |
| // ---- enable ----------------------------------------------------------------- |
| // handleAPIEmailEnable answers `carlos email enable`: provision the |
diff --git a/internal/console/apimail_test.go b/internal/console/apimail_test.go| index 0dad941d3..62af15b14 100644 |
| --- a/internal/console/apimail_test.go |
| +++ b/internal/console/apimail_test.go |
| @@ -2,7 +2,7 @@ package console |
| // The member half of the email API (apimail.go, tenant-email task 17). What |
| // is pinned here, in the order it would hurt if lost: a stranger probing any |
| -// of the nine doors learns nothing (the byte-identical 404 the whole |
| +// of the ten doors learns nothing (the byte-identical 404 the whole |
| // /api/cli surface keeps, and — new to this file — the same 404 when this |
| // deployment has no SES estate at all); the standalone-credential reply is |
| // the only one that ever carries a password, and it is uncacheable; a |
| @@ -150,7 +150,7 @@ func emailRecord(t *testing.T, fx *consoleFixture, app string) *mail.Record { |
| // ---- the gate --------------------------------------------------------------- |
| -// TestEmailDoorsAre404ForNonMembers: on all nine doors, a token that belongs |
| +// TestEmailDoorsAre404ForNonMembers: on all ten doors, a token that belongs |
| // to no account, an account segment that doesn't exist, and an app the |
| // account never claimed are one and the same 404 — byte-identical to |
| // http.NotFound's own output. This runs on an UNARMED fixture (no |
| @@ -183,6 +183,10 @@ func TestEmailDoorsAre404ForNonMembers(t *testing.T) { |
| {"POST domains verify", func(base, bearer string) (int, string) { |
| return postRaw(t, base+"/domains/example.com/verify", map[string]any{}, bearer) |
| }}, |
| + {"DELETE domains", func(base, bearer string) (int, string) { |
| + status, _, body := rawReq(t, http.MethodDelete, base+"/domains/example.com", bearer, "") |
| + return status, body |
| + }}, |
| {"POST enable", func(base, bearer string) (int, string) { |
| return postRaw(t, base+"/enable", map[string]any{}, bearer) |
| }}, |
| @@ -253,6 +257,10 @@ func TestEmailDoorsAre404WithNoProvisioner(t *testing.T) { |
| {"POST domains verify", func() (int, string) { |
| return postRaw(t, emailURL(fx, "titogo")+"/domains/example.com/verify", map[string]any{}, token) |
| }}, |
| + {"DELETE domains", func() (int, string) { |
| + status, _, body := rawReq(t, http.MethodDelete, emailURL(fx, "titogo")+"/domains/example.com", token, "") |
| + return status, body |
| + }}, |
| {"POST enable", func() (int, string) { |
| return postRaw(t, emailURL(fx, "titogo")+"/enable", map[string]any{}, token) |
| }}, |
| @@ -284,7 +292,7 @@ func TestEmailDoorsAre404WithNoProvisioner(t *testing.T) { |
| // TestOperatorStatusBuysNothingOnMemberDoors: I3 moved the pause door off |
| // apiAccountApp's member gate onto store.IsOperator — this proves that |
| -// move did not leak into the other eight. A fleet operator (IsOperator |
| +// move did not leak into the other nine. A fleet operator (IsOperator |
| // true) who is NOT a member of the target account gets the exact same |
| // byte-identical 404 an ordinary stranger gets on a WRITE (declare) and a |
| // READ (status) member door. Operator status must buy nothing here. |
| @@ -786,6 +794,191 @@ func TestAPIEmailDomainVerifyRecordsSESVerdict(t *testing.T) { |
| auditedBy(t, fx, app, "email-domain-verify", "alice@keymail.dev", "verified") |
| } |
| +// ---- domains rm: the release door ----------------------------------------- |
| + |
| +// removeEmailDomain drives the DELETE door and hands back the status and |
| +// the decoded reply — a 200 and a refusal are both interesting here, so it |
| +// never fails the test itself. |
| +func removeEmailDomain(t *testing.T, fx *consoleFixture, token, app, domain string) (int, map[string]any) { |
| + t.Helper() |
| + status, _, body := rawReq(t, http.MethodDelete, emailURL(fx, app)+"/domains/"+domain, token, "") |
| + out := map[string]any{} |
| + if err := json.Unmarshal([]byte(body), &out); err != nil { |
| + t.Fatalf("remove %s reply is not JSON (%d): %q", domain, status, body) |
| + } |
| + return status, out |
| +} |
| + |
| +// secondApp claims another app in the same account, so a domain can have |
| +// two referrers — the state the whole sibling rule exists for. |
| +func secondApp(t *testing.T, fx *consoleFixture, name string) string { |
| + t.Helper() |
| + if _, err := fx.st.CreateAppIn(OpsAccountID, name, "paul@keymail.dev", time.Now()); err != nil { |
| + t.Fatal(err) |
| + } |
| + return name |
| +} |
| + |
| +// TestAPIEmailDomainRemoveReleasesTheGlobalClaim: the defect this door |
| +// exists for. A claimed domain was unreachable to every other CARLOS |
| +// account — including the one that actually controls its DNS — until the |
| +// app holding it was PERMANENTLY DELETED, because mail.Teardown was the |
| +// only caller that could ever release a claim. After this, the claim object |
| +// is gone and the domain is free. |
| +func TestAPIEmailDomainRemoveReleasesTheGlobalClaim(t *testing.T) { |
| + fx, _, _, token := emailArmed(t, "code-email-domain-rm-claim") |
| + app, domain := "titogo", "mail.example.com" |
| + declareEmail(t, fx, token, app, "hello@elsewhere.example.com") |
| + ensureEmailDomain(t, fx, token, app, domain) |
| + |
| + claims, err := mail.ListDomainClaims(context.Background(), fx.srv.cfg.Releases) |
| + if err != nil || len(claims) != 1 || claims[0].Domain != domain { |
| + t.Fatalf("fixture broken: claims = %+v (err %v)", claims, err) |
| + } |
| + |
| + status, out := removeEmailDomain(t, fx, token, app, domain) |
| + if status != http.StatusOK { |
| + t.Fatalf("remove = %d %v, want 200", status, out) |
| + } |
| + if out["torn_down"] != true { |
| + t.Fatalf("torn_down = %v, want true — this was the only referrer", out["torn_down"]) |
| + } |
| + claims, err = mail.ListDomainClaims(context.Background(), fx.srv.cfg.Releases) |
| + if err != nil || len(claims) != 0 { |
| + t.Fatalf("the global claim survived the remove: %+v (err %v)", claims, err) |
| + } |
| + auditedBy(t, fx, app, "email-domain-remove", "alice@keymail.dev", domain) |
| + if got := serial(t, fx); got != 0 { |
| + t.Fatalf("serial after removing a domain = %d, want 0 — nothing in the app's own config changed", got) |
| + } |
| +} |
| + |
| +// TestAPIEmailDomainRemoveKeepsADomainASiblingAppUses: with another app |
| +// still referring to the domain, the door removes ONLY the referral. The |
| +// identity, the DNS and the claim all stay — releasing them here would |
| +// revoke a sibling app's verified domain silently. |
| +func TestAPIEmailDomainRemoveKeepsADomainASiblingAppUses(t *testing.T) { |
| + fx, fake, _, token := emailArmed(t, "code-email-domain-rm-sibling") |
| + domain := "shared.example.com" |
| + other := secondApp(t, fx, "marketing") |
| + declareEmail(t, fx, token, "titogo", "hello@elsewhere.example.com") |
| + ensureEmailDomain(t, fx, token, "titogo", domain) |
| + ensureEmailDomain(t, fx, token, other, domain) |
| + |
| + status, out := removeEmailDomain(t, fx, token, "titogo", domain) |
| + if status != http.StatusOK { |
| + t.Fatalf("remove = %d %v, want 200", status, out) |
| + } |
| + if out["torn_down"] != false { |
| + t.Fatalf("torn_down = %v, want false — %s still uses the domain", out["torn_down"], other) |
| + } |
| + if got := strList(t, out, "remaining_apps"); len(got) != 1 || got[0] != other { |
| + t.Fatalf("remaining_apps = %v, want [%s]", got, other) |
| + } |
| + claims, err := mail.ListDomainClaims(context.Background(), fx.srv.cfg.Releases) |
| + if err != nil || len(claims) != 1 { |
| + t.Fatalf("the claim was released while a sibling app still sends from the domain: %+v (err %v)", claims, err) |
| + } |
| + if len(fake.Identities) == 0 { |
| + t.Fatal("the sibling app's SES identity was destroyed") |
| + } |
| +} |
| + |
| +// TestAPIEmailDomainRemoveRefusesTheAppsOwnFromDomain: mail.Enable rejects a |
| +// From that is not at a granted domain, so removing the From's own domain |
| +// would break sending at the app's next re-enable rather than at the |
| +// command that caused it. 409, naming the From and the verb that moves it. |
| +func TestAPIEmailDomainRemoveRefusesTheAppsOwnFromDomain(t *testing.T) { |
| + fx, _, _, token := emailArmed(t, "code-email-domain-rm-from") |
| + app, domain := "titogo", "mail.example.com" |
| + declareEmail(t, fx, token, app, "hello@"+domain) |
| + ensureEmailDomain(t, fx, token, app, domain) |
| + |
| + status, out := removeEmailDomain(t, fx, token, app, domain) |
| + if status != http.StatusConflict { |
| + t.Fatalf("remove of the From's own domain = %d %v, want 409", status, out) |
| + } |
| + msg, _ := out["error"].(string) |
| + if !strings.Contains(msg, "hello@"+domain) { |
| + t.Fatalf("refusal does not name the From address: %q", msg) |
| + } |
| + if !strings.Contains(msg, "--from") { |
| + t.Fatalf("refusal does not name the verb that moves the From: %q", msg) |
| + } |
| + claims, err := mail.ListDomainClaims(context.Background(), fx.srv.cfg.Releases) |
| + if err != nil || len(claims) != 1 { |
| + t.Fatalf("a refused remove still released the claim: %+v (err %v)", claims, err) |
| + } |
| + if rec, _, err := mail.GetDomain(context.Background(), fx.srv.cfg.Releases, fx.sqid, domain); err != nil || rec == nil { |
| + t.Fatalf("a refused remove still deleted the domain record: %+v (err %v)", rec, err) |
| + } |
| +} |
| + |
| +// TestAPIEmailDomainRemoveIs404ForADomainTheAppDoesNotUse: naming a domain |
| +// this app does not send from is a 404, never a quiet 200. The same 404 |
| +// covers a domain claimed by ANOTHER account, deliberately in the same |
| +// words — see mail.ErrDomainNotReferenced. |
| +func TestAPIEmailDomainRemoveIs404ForADomainTheAppDoesNotUse(t *testing.T) { |
| + fx, _, _, token := emailArmed(t, "code-email-domain-rm-404") |
| + app := "titogo" |
| + declareEmail(t, fx, token, app, "hello@elsewhere.example.com") |
| + |
| + status, out := removeEmailDomain(t, fx, token, app, "never-added.example.com") |
| + if status != http.StatusNotFound { |
| + t.Fatalf("remove of an unused domain = %d %v, want 404", status, out) |
| + } |
| + if msg, _ := out["error"].(string); !strings.Contains(msg, "carlos email status") { |
| + t.Fatalf("404 does not point at the verb that lists the app's domains: %q", msg) |
| + } |
| +} |
| + |
| +// TestAPIEmailDomainRemoveReportsACustomDomainsOrphanedRecords: a custom |
| +// domain's records live in the customer's zone and are the customer's to |
| +// remove — mail.Teardown's rule, kept. The door deletes none of them and |
| +// hands the whole set back so the member can see what just went stale. |
| +func TestAPIEmailDomainRemoveReportsACustomDomainsOrphanedRecords(t *testing.T) { |
| + fx, _, writer, token := emailArmed(t, "code-email-domain-rm-custom-dns") |
| + app, domain := "titogo", "mail.example.com" |
| + declareEmail(t, fx, token, app, "hello@elsewhere.example.com") |
| + ensureEmailDomain(t, fx, token, app, domain) |
| + if len(writer.Records) != 0 { |
| + t.Fatalf("fixture broken: CARLOS published a custom domain's records: %v", writer.Records) |
| + } |
| + |
| + status, out := removeEmailDomain(t, fx, token, app, domain) |
| + if status != http.StatusOK { |
| + t.Fatalf("remove = %d %v, want 200", status, out) |
| + } |
| + orphaned, ok := out["orphaned"].([]any) |
| + if !ok || len(orphaned) != 6 { |
| + t.Fatalf("orphaned = %v, want the whole six-record set back", out["orphaned"]) |
| + } |
| +} |
| + |
| +// TestAPIEmailDomainRemoveDeletesAPlatformDomainsRecords: the other half of |
| +// the same rule — records CARLOS published, CARLOS removes, and reports |
| +// nothing orphaned because nothing is. |
| +func TestAPIEmailDomainRemoveDeletesAPlatformDomainsRecords(t *testing.T) { |
| + fx, _, writer, token := emailArmed(t, "code-email-domain-rm-platform-dns") |
| + app, domain := "titogo", "titogo.bab.oncarlos.com" |
| + declareEmail(t, fx, token, app, "hello@elsewhere.example.com") |
| + ensureEmailDomain(t, fx, token, app, domain) |
| + if len(writer.Records) == 0 { |
| + t.Fatal("fixture broken: nothing was published for a platform domain") |
| + } |
| + |
| + status, out := removeEmailDomain(t, fx, token, app, domain) |
| + if status != http.StatusOK { |
| + t.Fatalf("remove = %d %v, want 200", status, out) |
| + } |
| + if len(writer.Records) != 0 { |
| + t.Fatalf("a platform domain's published records survived: %v", writer.Records) |
| + } |
| + if orphaned, _ := out["orphaned"].([]any); len(orphaned) != 0 { |
| + t.Fatalf("orphaned = %v, want empty for a platform domain", orphaned) |
| + } |
| +} |
| + |
| // ---- enable and the F2 grant wiring ---------------------------------------- |
| // TestEnableAppliesTheTierGrantBeforeProvisioning: F2's ruling, pinned. The |
diff --git a/internal/console/server.go b/internal/console/server.go| index ed4352157..b5d5a2706 100644 |
| --- a/internal/console/server.go |
| +++ b/internal/console/server.go |
| @@ -1127,7 +1127,7 @@ func NewServer(cfg Config, st *Store) (*Server, error) { |
| // CLI identity: the device-code login `carlos auth login` drives, the |
| // approval page, and the tokens an owner can see and kill (authcli.go). |
| s.registerCLIRoutes() |
| - // The email API's nine doors (apimail.go, tenant-email task 17) — |
| + // The email API's ten doors (apimail.go, tenant-email task 17) — |
| // registered separately from registerCLIRoutes because this task's file |
| // scope is server.go and apimail.go only. |
| s.registerEmailRoutes() |
diff --git a/internal/mail/domainclaim.go b/internal/mail/domainclaim.go| index 53f2f90ca..90cc59987 100644 |
| --- a/internal/mail/domainclaim.go |
| +++ b/internal/mail/domainclaim.go |
| @@ -147,14 +147,24 @@ func claimDomain(ctx context.Context, s release.Store, domain, acct, app string, |
| // A claim already absent is a quiet no-op: release, like the claim itself, |
| // is idempotent. |
| // |
| -// Callers: ONLY Teardown, at two points (I1, review follow-up) — right |
| -// after the identity/DNS for the LAST referring app is torn down (the |
| -// shared resource is gone by then, so releasing first is safe), and again |
| -// in Teardown's orphan-claim reconciliation pass for a claim this account |
| -// holds with NO backing record at all (reachable when EnsureDomain fails |
| -// after claiming but before ever writing a record). Releasing outside |
| -// either of those two conditions would let a stranger claim a domain still |
| -// backed by a live record. |
| +// Callers: only ever the two CONDITIONS below, never a third. Releasing |
| +// outside them would let a stranger claim a domain still backed by a live |
| +// record. |
| +// |
| +// - right after the identity/DNS for the LAST referring app is torn down |
| +// (the shared resource is gone by then, so releasing first is safe) — |
| +// detachDomainApp, shared by Teardown and RemoveDomain; |
| +// - for a claim this account holds with NO backing record at all |
| +// (reachable when EnsureDomain fails after claiming but before ever |
| +// writing a record) — Teardown's orphan-claim reconciliation pass, and |
| +// RemoveDomain's removeOrphanedClaim for the same state named one |
| +// domain at a time. |
| +// |
| +// It was ONLY Teardown until `carlos email domains rm` (RemoveDomain) was |
| +// built, and that was the defect: Teardown is reached solely by |
| +// PERMANENTLY DELETING an app, so a domain claimed by the wrong app stayed |
| +// unavailable to every other account — its rightful owner included — for |
| +// the life of the deployment. |
| func releaseDomainClaim(ctx context.Context, s release.Store, domain, acct string) error { |
| cur, err := getDomainClaim(ctx, s, domain) |
| if err != nil { |
| @@ -179,9 +189,11 @@ func releaseDomainClaim(ctx context.Context, s release.Store, domain, acct strin |
| // discipline domains.ListClaims applies — one bad claim must never take |
| // the whole listing down. |
| // |
| -// The operator-facing CLI/console verb this enables is filed as owed, not |
| -// built here — this function is the read path a future door calls, not a |
| -// door itself. |
| +// The RELEASE half of the door this was written for now exists — `carlos |
| +// email domains rm` (RemoveDomain), which frees a claim without deleting |
| +// the app that holds it. A READ verb, listing claims an operator does not |
| +// hold, is still owed: this function remains the read path such a door |
| +// would call, and nothing but Teardown's own sweep calls it yet. |
| func ListDomainClaims(ctx context.Context, s release.Store) ([]DomainClaim, error) { |
| keys, err := s.List(ctx, domainClaimsPrefix) |
| if err != nil { |
diff --git a/internal/mail/walk.go b/internal/mail/walk.go| index 60be16b96..f721f61da 100644 |
| --- a/internal/mail/walk.go |
| +++ b/internal/mail/walk.go |
| @@ -38,6 +38,18 @@ var ( |
| // is not a cross-account leak, it is telling the caller which name IS |
| // theirs. |
| ErrNotOwnPlatformName = errors.New("mail: not this account's platform name") |
| + // ErrDomainNotReferenced is RemoveDomain's refusal of a domain this app |
| + // does not use — including one whose global claim belongs to somebody |
| + // else, which reads identically on purpose so the door cannot be used |
| + // as an oracle for who holds a domain the caller does not. |
| + ErrDomainNotReferenced = errors.New("mail: app does not use this domain") |
| + // ErrDomainIsFrom is RemoveDomain's refusal of the domain the app is |
| + // currently sending FROM. Enable rejects a From that is not at a |
| + // granted domain, so allowing this would break sending at the app's |
| + // next re-enable rather than at the command that caused it — the |
| + // failure moves from a refusal a person can read to silence a person |
| + // has to debug. |
| + ErrDomainIsFrom = errors.New("mail: domain is the app's From address") |
| ) |
| // Deployment is the per-deployment configuration the walks need. |
| @@ -1180,6 +1192,140 @@ func RevokeStandalone(ctx context.Context, s release.Store, p Provisioner, acct, |
| return err |
| } |
| +// domainDetach says what detachDomainApp did to one domain record. |
| +type domainDetach int |
| + |
| +const ( |
| + // detachNone decided nothing. It is only ever paired with a non-nil |
| + // error, so a caller that checks the error first never sees it. |
| + detachNone domainDetach = iota |
| + // detachNotAReferrer: this app never referred to the domain, or had |
| + // already stopped. Nothing was written and nothing was destroyed. |
| + detachNotAReferrer |
| + // detachKept: other apps still refer to the domain, so the record was |
| + // rewritten without this app and the shared estate was left alone. |
| + detachKept |
| + // detachTornDown: this app was the LAST referrer, so the published DNS |
| + // (platform domains only), the SES identity, the global claim and the |
| + // account's own domain record are all gone. |
| + detachTornDown |
| +) |
| + |
| +// detachDomainApp stops one app referring to one domain, and — only when |
| +// that leaves NO app referring to it — dismantles the domain. |
| +// |
| +// It is the body of Teardown's per-domain loop, lifted out so `carlos email |
| +// domains rm` (RemoveDomain) executes the identical sequence rather than a |
| +// second copy of it. The order below is load-bearing in three separate |
| +// places, each documented where it happens; two copies of it would be two |
| +// chances to get one of them subtly wrong. |
| +// |
| +// cur must be a FRESH read (with its etag), not a ListDomains snapshot — |
| +// see Teardown's own comment at the call site for the two reasons. |
| +// |
| +// The orphaned record set comes back rather than being deleted for a CUSTOM |
| +// domain: those records live in the customer's zone and are the customer's |
| +// to remove. Returning them is what lets a door tell an operator exactly |
| +// which names are now stale. |
| +func detachDomainApp(ctx context.Context, s release.Store, p Provisioner, w dnsrec.Writer, |
| + d Deployment, acct, app string, cur *DomainRecord, curEtag string) (domainDetach, []dnsrec.Record, error) { |
| + |
| + if !cur.RemoveApp(app) { |
| + return detachNotAReferrer, nil, nil |
| + } |
| + if len(cur.Apps) > 0 { |
| + // A sibling app still sends from this domain. Its identity, its |
| + // DKIM tokens, its DNS and the global claim ALL stay exactly as |
| + // they are — getting this backwards revokes a sibling app's |
| + // verified domain silently, with the customer's own DNS still |
| + // pointing at an identity that no longer exists. |
| + if _, err := PutDomain(ctx, s, acct, cur, curEtag); err != nil { |
| + return detachNone, nil, err |
| + } |
| + return detachKept, nil, nil |
| + } |
| + // Last referrer: remove the identity and, for a platform domain, the |
| + // records CARLOS published. A custom domain's records are the |
| + // customer's to remove, so they come back to the caller instead. |
| + var orphaned []dnsrec.Record |
| + for region, st := range cur.Regions { |
| + records := RecordSet(cur.Domain, region, st.DKIMTokens, d.DMARCRua) |
| + // Same w != nil guard as EnsureDomain's publish branch, for the |
| + // identical reason: a caller with no DNSWriter configured must not |
| + // panic on a nil interface here either. A platform domain with no |
| + // writer degrades to "orphaned" exactly like a custom one — the |
| + // records exist somewhere this process cannot reach, which is the |
| + // honest thing to report. |
| + if cur.Kind == KindPlatform && w != nil { |
| + for _, r := range records { |
| + if err := w.DeleteRecord(ctx, r); err != nil { |
| + return detachNone, nil, err |
| + } |
| + } |
| + } else { |
| + orphaned = append(orphaned, records...) |
| + } |
| + // SES refuses to delete an identity while a tenant association |
| + // exists, so the association is attempted first. The account's |
| + // configuration set is NOT disassociated: it is per account and |
| + // outlives any one app. |
| + // |
| + // A failed disassociate is logged and NOT a hard error (I2, review |
| + // follow-up, reversed from that task's first cut). The control-plane |
| + // role needs ses:DeleteTenantResourceAssociation (spec §4.2) and |
| + // that grant does not live in this repo; if it is missing, |
| + // AccessDenied is neither AlreadyExists nor NotFound, so a |
| + // `return err` here would fail EVERY release where this app is the |
| + // last referrer of a domain, and — worse — leave releaseDomainClaim |
| + // and s.Delete below unreached, which strands the domain's global |
| + // claim: nobody, including this domain's own owner, could ever claim |
| + // it again until the grant landed. That is the exact permanent |
| + // stranding `carlos email domains rm` exists to end, so this door |
| + // must not reintroduce it. DeleteIdentity is the real protection SES |
| + // itself provides (confirmed against the live API, spec §4.1: it |
| + // refuses to delete an identity that still has a tenant |
| + // association), so it stays a hard failure below and does the job |
| + // this disassociate call cannot be trusted to gate. |
| + if st.IdentityARN != "" { |
| + if err := p.DisassociateTenantResource(ctx, TenantName(acct), st.IdentityARN); err != nil { |
| + log.Printf("mail: disassociating tenant resource for %s/%s: %v", acct, cur.Domain, err) |
| + } |
| + } |
| + if err := p.DeleteIdentity(ctx, region, cur.Domain); err != nil { |
| + return detachNone, nil, err |
| + } |
| + } |
| + // The global claim (Task 17a, spec §4a) releases HERE, in the same |
| + // branch as deleting the account's own domain record — the exact |
| + // moment the last app referencing this domain has gone. Releasing any |
| + // earlier (e.g. at the top of Teardown, or keyed off "this app is |
| + // going" rather than "the domain record is going") would let a stranger |
| + // claim a domain a sibling app is still sending from — the same class |
| + // of bug the `len(cur.Apps) > 0` branch above guards. |
| + // |
| + // RELEASE BEFORE DELETE (I1, review follow-up), reversed from this |
| + // task's first cut. The shared resource the claim protects is the SES |
| + // identity, and by this point in the branch it is ALREADY destroyed |
| + // (DeleteIdentity, above) — so releasing the claim before the record is |
| + // gone hands nothing live to a winner. The reverse order |
| + // (delete-then-release, the original cut) has a real defect: if the |
| + // process dies between the two, the record is gone, so |
| + // ListDomains(acct) on any later Teardown run never lists this domain |
| + // again — the release call becomes permanently unreachable, and the |
| + // claim is stranded forever, un-reclaimable even by its own owner's |
| + // account under a different app. Releasing FIRST means a crash before |
| + // the record delete just repeats a no-op release (idempotent) and a |
| + // retriable s.Delete on the next run — self-healing again, matching |
| + // every other step here. |
| + if err := releaseDomainClaim(ctx, s, cur.Domain, acct); err != nil { |
| + return detachNone, nil, err |
| + } |
| + if err := s.Delete(ctx, DomainKey(acct, cur.Domain)); err != nil { |
| + return detachNone, nil, err |
| + } |
| + return detachTornDown, orphaned, nil |
| +} |
| + |
| // Teardown dismantles an app's email estate: credentials, principals, and |
| // its claim on every domain it referenced. |
| // |
| @@ -1254,89 +1400,13 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, w dnsrec.Writ |
| if cur == nil { |
| continue // gone since the list; nothing to do |
| } |
| - if !cur.RemoveApp(app) { |
| - continue // this app was never a referrer, or already removed |
| - } |
| - if len(cur.Apps) > 0 { |
| - if _, err := PutDomain(ctx, s, acct, cur, curEtag); err != nil { |
| - return err |
| - } |
| - continue |
| - } |
| - // Last referrer: remove the identity and, for a platform domain, |
| - // the records CARLOS published. A custom domain's records are the |
| - // customer's to remove. |
| - for region, st := range cur.Regions { |
| - // Same w != nil guard as EnsureDomain's publish branch, for |
| - // the identical reason: a caller with no DNSWriter configured |
| - // must not panic on a nil interface here either, even though |
| - // nothing in this codebase wires Teardown to such a caller |
| - // yet — the guard costs nothing and keeps the two record- |
| - // writing sites in this file from drifting onto different |
| - // nil-safety assumptions. |
| - if cur.Kind == KindPlatform && w != nil { |
| - for _, r := range RecordSet(cur.Domain, region, st.DKIMTokens, d.DMARCRua) { |
| - if err := w.DeleteRecord(ctx, r); err != nil { |
| - return err |
| - } |
| - } |
| - } |
| - // SES refuses to delete an identity while a tenant association |
| - // exists, so the association is attempted first. The account's |
| - // configuration set is NOT disassociated: it is per account and |
| - // outlives any one app. |
| - // |
| - // A failed disassociate is logged and NOT a hard error (I2, |
| - // review follow-up, reversed from this task's first cut). The |
| - // control-plane role needs ses:DeleteTenantResourceAssociation |
| - // (spec §4.2) and that grant does not live in this repo; if it |
| - // is missing, AccessDenied is neither AlreadyExists nor |
| - // NotFound, so a `return err` here would fail EVERY teardown |
| - // where this app is the last referrer of a domain, and — worse |
| - // — leave releaseDomainClaim and s.Delete below unreached, which |
| - // strands the domain's global claim: nobody, including this |
| - // domain's own owner, could ever claim it again until the grant |
| - // landed. DeleteIdentity is the real protection SES itself |
| - // provides (confirmed against the live API, spec §4.1: it |
| - // refuses to delete an identity that still has a tenant |
| - // association), so it stays a hard failure below and does the |
| - // job this disassociate call cannot be trusted to gate. |
| - if st.IdentityARN != "" { |
| - if err := p.DisassociateTenantResource(ctx, TenantName(acct), st.IdentityARN); err != nil { |
| - log.Printf("mail: teardown: disassociating tenant resource for %s/%s: %v", acct, cur.Domain, err) |
| - } |
| - } |
| - if err := p.DeleteIdentity(ctx, region, cur.Domain); err != nil { |
| - return err |
| - } |
| - } |
| - // The global claim (Task 17a, spec §4a) releases HERE, in the same |
| - // branch as deleting the account's own domain record — the exact |
| - // moment the last app referencing this domain has gone. Releasing |
| - // any earlier (e.g. at the top of Teardown, or keyed off "this app |
| - // is going" rather than "the domain record is going") would let a |
| - // stranger claim a domain a sibling app is still sending from — the |
| - // same class of bug RemoveApp's own comment warns about for the |
| - // identity/DNS cleanup above. |
| - // |
| - // RELEASE BEFORE DELETE (I1, review follow-up), reversed from this |
| - // task's first cut. The shared resource the claim protects is the |
| - // SES identity, and by this point in the branch it is ALREADY |
| - // destroyed (DeleteIdentity, above) — so releasing the claim before |
| - // the record is gone hands nothing live to a winner. The reverse |
| - // order (delete-then-release, the original cut) has a real defect: |
| - // if the process dies between the two, the record is gone, so |
| - // ListDomains(acct) on any later Teardown run never lists this |
| - // domain again — the release call becomes permanently unreachable, |
| - // and the claim is stranded forever, un-reclaimable even by its own |
| - // owner's account under a different app. Releasing FIRST means a |
| - // crash before the record delete just repeats a no-op release |
| - // (idempotent) and a retriable s.Delete on the next run — self- |
| - // healing again, matching every other step in this loop. |
| - if err := releaseDomainClaim(ctx, s, cur.Domain, acct); err != nil { |
| - return err |
| - } |
| - if err := s.Delete(ctx, DomainKey(acct, cur.Domain)); err != nil { |
| + // Everything from here — the sibling check, the identity and DNS, |
| + // the claim, the record — is detachDomainApp's, shared verbatim |
| + // with RemoveDomain so the two release paths cannot drift on an |
| + // ordering that is load-bearing and subtle. A domain this app never |
| + // referred to is that function's own no-op, which is what the |
| + // `continue` used to be. |
| + if _, _, err := detachDomainApp(ctx, s, p, w, d, acct, app, cur, curEtag); err != nil { |
| return err |
| } |
| } |
| @@ -1411,3 +1481,185 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, w dnsrec.Writ |
| } |
| return nil |
| } |
| + |
| +// DomainRemoval is what RemoveDomain did, so a door can describe it without |
| +// re-reading anything the walk has just deleted. |
| +type DomainRemoval struct { |
| + Domain string |
| + Kind string |
| + // TornDown is true when this app was the LAST referrer: the SES |
| + // identity, the account's domain record and the global claim are all |
| + // gone, and the domain is free for any account to claim again. |
| + TornDown bool |
| + // Remaining is the apps that still refer to the domain — non-empty |
| + // exactly when TornDown is false, and the reason nothing else was |
| + // touched. |
| + Remaining []string |
| + // Orphaned is the record set nobody deleted: a custom domain's DNS |
| + // lives in the customer's zone and is the customer's to remove |
| + // (detachDomainApp's rule, which this inherits from Teardown). Empty |
| + // for a platform domain, whose records CARLOS published and has now |
| + // deleted itself. |
| + // |
| + // Only the DKIM CNAMEs in here are token-derived and therefore dead: |
| + // the MX, SPF and DMARC records RecordSet builds are deterministic |
| + // functions of the domain and region, so they stay valid if the domain |
| + // is ever re-created. |
| + Orphaned []dnsrec.Record |
| + // ClaimOnly is the orphaned-claim repair: there was no domain record at |
| + // all — only a global claim stranded by an EnsureDomain that failed |
| + // after claimDomain and before PutDomain — and releasing it is the |
| + // whole of what happened. |
| + ClaimOnly bool |
| +} |
| + |
| +// RemoveDomain is the release door the claim always needed: it stops app |
| +// referring to domain and, when that was the last reference, dismantles the |
| +// domain and releases its global claim. |
| +// |
| +// Before this existed, Teardown — the PERMANENT deletion of a whole app — |
| +// was the only thing that could ever release a claim, so a domain claimed |
| +// by the wrong app was unreachable to every other account, including the |
| +// one that actually controls its DNS, for the life of the deployment. |
| +// |
| +// It shares detachDomainApp with Teardown rather than repeating it, so the |
| +// safety properties are the same objects, not the same intentions: |
| +// |
| +// - a domain a sibling app still refers to keeps its identity, its DNS |
| +// and its claim; only the referrer set shrinks; |
| +// - the claim is released only after DeleteIdentity, and only before the |
| +// record delete (see detachDomainApp for why each of those is the way |
| +// round it is); |
| +// - a custom domain's DNS is never deleted, it is reported back. |
| +// |
| +// Two refusals are its own, and neither is Teardown's business: |
| +// |
| +// - ErrDomainIsFrom, the domain the app currently sends FROM. Enable |
| +// refuses a From that is not at a granted domain, so removing it would |
| +// turn a working app into one that fails at its next re-enable — a |
| +// breakage discovered at runtime instead of here. Move the From first. |
| +// - ErrDomainNotReferenced, a domain this app does not use. Teardown |
| +// walks an app's whole estate and a non-referrer is simply skipped; |
| +// here the operator NAMED this domain, so silence would be a lie. |
| +// |
| +// OWNER-GATING IS UNCHANGED. Nothing here reads, probes or releases another |
| +// account's claim: releaseDomainClaim refuses a claim held by anyone else |
| +// as a quiet no-op, the orphan branch below compares the claim's account |
| +// itself, and no error message ever names a holder. |
| +// |
| +// A non-nil *DomainRemoval WITH a non-nil error means one specific thing: |
| +// the removal itself landed, and only the departing app's IAM policy shrink |
| +// (RefreshPolicies) failed. Re-running this verb would then answer |
| +// ErrDomainNotReferenced, correctly — the owed repair is `carlos email |
| +// enable`, which rewrites that policy wholesale on every call. |
| +func RemoveDomain(ctx context.Context, s release.Store, p Provisioner, w dnsrec.Writer, |
| + d Deployment, acct, app, domain string) (*DomainRemoval, error) { |
| + |
| + if !release.ValidAccount(acct) { |
| + return nil, fmt.Errorf("mail: invalid account %q", acct) |
| + } |
| + if !release.ValidApp(app) { |
| + return nil, fmt.Errorf("mail: invalid app %q", app) |
| + } |
| + if !ValidDomain(domain) { |
| + return nil, fmt.Errorf("mail: invalid domain %q", domain) |
| + } |
| + |
| + // The From check runs FIRST, before anything is read about the domain |
| + // and long before anything is written: a refusal must cost the caller |
| + // nothing and leave no half-step behind. It looks at THIS app's From |
| + // only — a sibling app keeps its own referral, so its From is still at |
| + // a domain it is granted. |
| + rec, _, err := Get(ctx, s, acct, app) |
| + if err != nil { |
| + return nil, err |
| + } |
| + if rec != nil && rec.Declared != nil { |
| + if at := strings.LastIndex(rec.Declared.From, "@"); at >= 0 && rec.Declared.From[at+1:] == domain { |
| + return nil, fmt.Errorf("mail: %s still sends as %s: %w", app, rec.Declared.From, ErrDomainIsFrom) |
| + } |
| + } |
| + |
| + cur, curEtag, err := GetDomain(ctx, s, acct, domain) |
| + if err != nil { |
| + return nil, err |
| + } |
| + if cur == nil { |
| + return removeOrphanedClaim(ctx, s, p, d, acct, domain) |
| + } |
| + |
| + res, orphaned, err := detachDomainApp(ctx, s, p, w, d, acct, app, cur, curEtag) |
| + if err != nil { |
| + return nil, err |
| + } |
| + if res == detachNotAReferrer { |
| + return nil, fmt.Errorf("mail: %s does not send from %s: %w", app, domain, ErrDomainNotReferenced) |
| + } |
| + out := &DomainRemoval{Domain: domain, Kind: cur.Kind, TornDown: res == detachTornDown, Orphaned: orphaned} |
| + if res == detachKept { |
| + out.Remaining = append([]string(nil), cur.Apps...) |
| + } |
| + // SHRINK THE DEPARTING APP'S POLICY, the mirror of EnsureDomain's own |
| + // RefreshPolicies call. EnsureUser writes a principal's policy |
| + // wholesale from the CURRENT granted domain set, and nothing else |
| + // recomputes it when a domain LEAVES that set — so without this, an app |
| + // that drops a domain a sibling still keeps alive goes on holding |
| + // ses:SendRawEmail at a live identity ARN it no longer has any claim |
| + // to, indefinitely. That is a real over-grant, not a stale string. |
| + if err := RefreshPolicies(ctx, s, p, acct, []string{app}); err != nil { |
| + return out, err |
| + } |
| + return out, nil |
| +} |
| + |
| +// removeOrphanedClaim is RemoveDomain's branch for a claim with NO backing |
| +// domain record — the state EnsureDomain leaves behind when it fails after |
| +// claimDomain and before PutDomain (an SES throttle, a tenant limit, a bad |
| +// region). Before this, only Teardown could sweep one, and only for the app |
| +// that created it; an operator who hit it on an app they did not want to |
| +// delete had no recourse at all. |
| +// |
| +// The app in the path is a MEMBERSHIP GATE here, not an ownership filter — |
| +// the same reading handleAPIEmailDomainVerify documents, and appropriate |
| +// for the same reason: the claim is account-scoped, not app-scoped, and |
| +// unlike Teardown's blind sweep the operator has NAMED this exact domain, |
| +// so there is no "a sibling app is merely about to retry" ambiguity for the |
| +// App attribution to resolve. |
| +// |
| +// DESTROY THE IDENTITY BEFORE RELEASING (I1 item 2), exactly as Teardown's |
| +// own sweep does: if the crash that orphaned this claim landed after |
| +// EnsureIdentity but before PutDomain, a live SES identity exists that no |
| +// record names, and releasing without deleting it hands a stranger that |
| +// identity's ARN and its already-minted DKIM tokens the moment they claim |
| +// the domain. EnsureDomain only ever mints in d.Region, so that is the only |
| +// region to check, and DeleteIdentity is idempotent. |
| +// |
| +// NO DISASSOCIATE HERE, unlike detachDomainApp's region loop, and that is |
| +// not an omission. EnsureDomain associates the identity with the account's |
| +// tenant only AFTER PutDomain — deliberately, so that a claim with no |
| +// record can never have an association behind it (see the ordering comment |
| +// at that call). An identity reachable from this branch is therefore always |
| +// unassociated, and DeleteIdentity takes it. |
| +func removeOrphanedClaim(ctx context.Context, s release.Store, p Provisioner, |
| + d Deployment, acct, domain string) (*DomainRemoval, error) { |
| + |
| + claim, err := getDomainClaim(ctx, s, domain) |
| + if err != nil { |
| + return nil, err |
| + } |
| + // A claim held by ANOTHER account reads exactly like no claim at all — |
| + // the same refusal, the same words. Anything else would make this door |
| + // an oracle for who holds a domain the caller does not. |
| + if claim == nil || claim.Account != acct { |
| + return nil, fmt.Errorf("mail: no sending domain %s for this account: %w", domain, ErrDomainNotReferenced) |
| + } |
| + if err := p.DeleteIdentity(ctx, d.Region, domain); err != nil { |
| + return nil, err |
| + } |
| + if err := releaseDomainClaim(ctx, s, domain, acct); err != nil { |
| + return nil, err |
| + } |
| + return &DomainRemoval{ |
| + Domain: domain, Kind: DomainKindFor(d, domain), TornDown: true, ClaimOnly: true, |
| + }, nil |
| +} |
diff --git a/internal/mail/walk_domainremove_test.go b/internal/mail/walk_domainremove_test.go| new file mode 100644 |
| index 000000000..3f88795b3 |
| --- /dev/null |
| +++ b/internal/mail/walk_domainremove_test.go |
| @@ -0,0 +1,442 @@ |
| +// RemoveDomain: `carlos email domains rm`, the release door the global |
| +// claim never had. |
| +// |
| +// The claim (domainclaim.go) is a deliberately global, create-only object |
| +// so two accounts cannot both hold one sending domain. Right — and until |
| +// this walk existed, the ONLY thing that ever released one was Teardown, |
| +// reachable only by permanently deleting the app that held it. A real |
| +// deployment hit exactly that: one app held a claim on a domain a second |
| +// app in a second account served and wanted to send from, and there was no |
| +// command in the product that could free it. |
| +// |
| +// What these tests pin is that the door does not reopen any of the holes |
| +// the claim closes: a domain a sibling app still uses keeps everything, the |
| +// release happens in an order a crash cannot strand, a custom domain's DNS |
| +// is never deleted, and no path here can read, disturb or probe another |
| +// account's claim. |
| +package mail |
| + |
| +import ( |
| + "context" |
| + "errors" |
| + "strings" |
| + "testing" |
| + "time" |
| + |
| + "github.com/carlosframework/platform/internal/dnsrec" |
| + "github.com/carlosframework/platform/internal/release" |
| +) |
| + |
| +// removeFixture is one account, one shared CUSTOM domain, two referring |
| +// apps — the shape every interesting question about this verb is asked in. |
| +// Custom rather than platform because the DNS rule (the customer's records |
| +// are the customer's to remove) only has teeth on a custom domain. |
| +func removeFixture(t *testing.T) (context.Context, release.Store, *MemProvisioner, *dnsrec.MemWriter, time.Time) { |
| + t.Helper() |
| + ctx := context.Background() |
| + s, p, w := release.NewDirStore(t.TempDir()), NewMemProvisioner("shared-aws-account"), dnsrec.NewMemWriter() |
| + now := time.Date(2026, 9, 23, 12, 0, 0, 0, time.UTC) |
| + for _, app := range []string{"app1", "app2"} { |
| + if _, _, err := EnsureDomain(ctx, s, p, w, testDeployment(), "aaa", app, "shared.example", now); err != nil { |
| + t.Fatalf("EnsureDomain %s: %v", app, err) |
| + } |
| + } |
| + return ctx, s, p, w, now |
| +} |
| + |
| +// A sibling app still referring to the domain means NOTHING is released: |
| +// not the identity, not the DNS, and above all not the claim. A stranger |
| +// must still be refused — this is the exact failure Teardown's own comment |
| +// warns about ("would let a stranger claim a domain a sibling app is still |
| +// sending from"), and a naive "just delete the claim object" reopens it. |
| +func TestRemoveDomainKeepsADomainASiblingStillUses(t *testing.T) { |
| + ctx, s, p, w, now := removeFixture(t) |
| + |
| + out, err := RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app1", "shared.example") |
| + if err != nil { |
| + t.Fatalf("RemoveDomain: %v", err) |
| + } |
| + if out.TornDown { |
| + t.Fatalf("torn down while app2 still refers to it: %+v", out) |
| + } |
| + if len(out.Remaining) != 1 || out.Remaining[0] != "app2" { |
| + t.Fatalf("remaining = %v, want [app2]", out.Remaining) |
| + } |
| + rec, _, err := GetDomain(ctx, s, "aaa", "shared.example") |
| + if err != nil { |
| + t.Fatalf("GetDomain: %v", err) |
| + } |
| + if rec == nil || len(rec.Apps) != 1 || rec.Apps[0] != "app2" { |
| + t.Fatalf("record = %+v, want one referrer app2", rec) |
| + } |
| + if _, ok := p.Identities[identityKey("eu-west-1", "shared.example")]; !ok { |
| + t.Fatal("removing one referrer destroyed the identity the sibling still sends with") |
| + } |
| + if claim, err := getDomainClaim(ctx, s, "shared.example"); err != nil || claim == nil { |
| + t.Fatalf("claim released while a sibling still uses the domain: claim=%+v err=%v", claim, err) |
| + } |
| + stranger := NewMemProvisioner("shared-aws-account") |
| + if _, _, err := EnsureDomain(ctx, s, stranger, w, testDeployment(), "bbb", "evilapp", "shared.example", now); err == nil { |
| + t.Fatal("a stranger claimed a domain a sibling app is still sending from") |
| + } |
| +} |
| + |
| +// The last referrer leaving is what releases the claim — and the proof is |
| +// that a DIFFERENT account can then complete the whole ensure walk on the |
| +// same domain, which is the outcome the real deployment could not reach. |
| +func TestRemoveDomainLastReferrerFreesTheDomainForAnotherAccount(t *testing.T) { |
| + ctx, s, p, w, now := removeFixture(t) |
| + |
| + if _, err := RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app1", "shared.example"); err != nil { |
| + t.Fatalf("remove app1: %v", err) |
| + } |
| + out, err := RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app2", "shared.example") |
| + if err != nil { |
| + t.Fatalf("remove app2: %v", err) |
| + } |
| + if !out.TornDown || len(out.Remaining) != 0 { |
| + t.Fatalf("last referrer left without a teardown: %+v", out) |
| + } |
| + if _, ok := p.Identities[identityKey("eu-west-1", "shared.example")]; ok { |
| + t.Fatal("the SES identity survived the last referrer leaving") |
| + } |
| + if rec, _, err := GetDomain(ctx, s, "aaa", "shared.example"); err != nil || rec != nil { |
| + t.Fatalf("the domain record survived: %+v (err %v)", rec, err) |
| + } |
| + if claim, err := getDomainClaim(ctx, s, "shared.example"); err != nil || claim != nil { |
| + t.Fatalf("the claim survived the last referrer leaving: %+v (err %v)", claim, err) |
| + } |
| + |
| + // The whole point: another account can now take the domain. |
| + other := NewMemProvisioner("shared-aws-account") |
| + rec, _, err := EnsureDomain(ctx, s, other, w, testDeployment(), "bbb", "jam", "shared.example", now) |
| + if err != nil { |
| + t.Fatalf("a second account still cannot claim the released domain: %v", err) |
| + } |
| + if rec == nil || len(other.Identities) != 1 { |
| + t.Fatal("the second account did not actually get its own identity") |
| + } |
| +} |
| + |
| +// Removing the domain the app's own From sits at is refused. Enable rejects |
| +// a From that is not at a granted domain, so allowing it would move the |
| +// breakage from this command — where a person can read it — to the app's |
| +// next re-enable, where it presents as "email silently stopped working". |
| +func TestRemoveDomainRefusesTheDomainTheAppSendsFrom(t *testing.T) { |
| + ctx, s, p, _, _ := enableFixture(t) |
| + domain := "harbourmail.bag.oncarlos.com" |
| + |
| + _, err := RemoveDomain(ctx, s, p, dnsrec.NewMemWriter(), testDeployment(), "bag", "harbourmail", domain) |
| + if !errors.Is(err, ErrDomainIsFrom) { |
| + t.Fatalf("RemoveDomain err = %v, want ErrDomainIsFrom", err) |
| + } |
| + // A refusal must cost nothing and leave nothing half-done. |
| + if rec, _, err := GetDomain(ctx, s, "bag", domain); err != nil || rec == nil { |
| + t.Fatalf("the refusal still removed the record: %+v (err %v)", rec, err) |
| + } |
| + if claim, err := getDomainClaim(ctx, s, domain); err != nil || claim == nil { |
| + t.Fatalf("the refusal still released the claim: %+v (err %v)", claim, err) |
| + } |
| + if _, ok := p.Identities[identityKey("eu-west-1", domain)]; !ok { |
| + t.Fatal("the refusal still destroyed the identity") |
| + } |
| + // The error names the From, so a member knows what to move. |
| + if !strings.Contains(err.Error(), "noreply@"+domain) { |
| + t.Fatalf("refusal does not name the From address: %v", err) |
| + } |
| +} |
| + |
| +// A CUSTOM domain's records live in the customer's zone: CARLOS never |
| +// published them and must not delete them. They come back instead, so a |
| +// door can tell the operator exactly which names just went stale. |
| +func TestRemoveDomainLeavesACustomDomainsDNSAloneAndNamesIt(t *testing.T) { |
| + ctx := context.Background() |
| + s, p, w := release.NewDirStore(t.TempDir()), NewMemProvisioner("1"), dnsrec.NewMemWriter() |
| + now := time.Date(2026, 9, 23, 12, 0, 0, 0, time.UTC) |
| + |
| + if _, _, err := EnsureDomain(ctx, s, p, w, testDeployment(), "aaa", "app1", "custom.example", now); err != nil { |
| + t.Fatalf("EnsureDomain: %v", err) |
| + } |
| + if len(w.Records) != 0 { |
| + t.Fatalf("fixture broken: CARLOS published records for a custom domain: %v", w.Records) |
| + } |
| + out, err := RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app1", "custom.example") |
| + if err != nil { |
| + t.Fatalf("RemoveDomain: %v", err) |
| + } |
| + if out.Kind != KindCustom { |
| + t.Fatalf("kind = %q, want %q", out.Kind, KindCustom) |
| + } |
| + // Six records: three DKIM CNAMEs, the MAIL FROM MX, its SPF TXT and |
| + // the DMARC TXT — the whole set the member was asked to publish. |
| + if len(out.Orphaned) != 6 { |
| + t.Fatalf("orphaned = %d records, want the whole set of 6: %+v", len(out.Orphaned), out.Orphaned) |
| + } |
| + var cnames int |
| + for _, r := range out.Orphaned { |
| + if r.Type == dnsrec.TypeCNAME { |
| + cnames++ |
| + } |
| + } |
| + if cnames != 3 { |
| + t.Fatalf("token-derived DKIM CNAMEs = %d, want 3", cnames) |
| + } |
| +} |
| + |
| +// A PLATFORM domain's records are CARLOS's own, so they go — the same split |
| +// Teardown makes, for the same reason. |
| +func TestRemoveDomainDeletesAPlatformDomainsPublishedRecords(t *testing.T) { |
| + ctx := context.Background() |
| + s, p, w := release.NewDirStore(t.TempDir()), NewMemProvisioner("1"), dnsrec.NewMemWriter() |
| + now := time.Date(2026, 9, 23, 12, 0, 0, 0, time.UTC) |
| + |
| + if _, _, err := EnsureDomain(ctx, s, p, w, testDeployment(), "bag", "shop", "shop.bag.oncarlos.com", now); err != nil { |
| + t.Fatalf("EnsureDomain: %v", err) |
| + } |
| + if len(w.Records) == 0 { |
| + t.Fatal("fixture broken: nothing was published for a platform domain") |
| + } |
| + out, err := RemoveDomain(ctx, s, p, w, testDeployment(), "bag", "shop", "shop.bag.oncarlos.com") |
| + if err != nil { |
| + t.Fatalf("RemoveDomain: %v", err) |
| + } |
| + if len(w.Records) != 0 { |
| + t.Fatalf("platform records survived the removal: %v", w.Records) |
| + } |
| + if len(out.Orphaned) != 0 { |
| + t.Fatalf("a platform domain reported orphaned records it does not own: %+v", out.Orphaned) |
| + } |
| +} |
| + |
| +// A domain this app does not use is a refusal, not a silent success — and |
| +// running rm twice takes the same path, so "already removed" never reads as |
| +// "removed something else". |
| +func TestRemoveDomainRefusesADomainThisAppDoesNotUse(t *testing.T) { |
| + ctx, s, p, w, _ := removeFixture(t) |
| + |
| + _, err := RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app3", "shared.example") |
| + if !errors.Is(err, ErrDomainNotReferenced) { |
| + t.Fatalf("non-referrer err = %v, want ErrDomainNotReferenced", err) |
| + } |
| + if rec, _, err := GetDomain(ctx, s, "aaa", "shared.example"); err != nil || rec == nil || len(rec.Apps) != 2 { |
| + t.Fatalf("a non-referrer's refusal changed the record: %+v (err %v)", rec, err) |
| + } |
| + |
| + if _, err := RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app1", "shared.example"); err != nil { |
| + t.Fatalf("first remove: %v", err) |
| + } |
| + _, err = RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app1", "shared.example") |
| + if !errors.Is(err, ErrDomainNotReferenced) { |
| + t.Fatalf("second remove err = %v, want the same ErrDomainNotReferenced", err) |
| + } |
| +} |
| + |
| +// OWNER-GATING. A claim held by another account must be untouchable and |
| +// indistinguishable from a domain that was never claimed at all: the same |
| +// error, and nothing in it naming the holder. Anything else turns this door |
| +// into an oracle for who owns what. |
| +func TestRemoveDomainNeverTouchesAnotherAccountsClaim(t *testing.T) { |
| + ctx := context.Background() |
| + s, w := release.NewDirStore(t.TempDir()), dnsrec.NewMemWriter() |
| + p := NewMemProvisioner("shared-aws-account") |
| + now := time.Date(2026, 9, 23, 12, 0, 0, 0, time.UTC) |
| + |
| + if _, _, err := EnsureDomain(ctx, s, p, w, testDeployment(), "secretacct", "victimapp", "victim.example", now); err != nil { |
| + t.Fatalf("EnsureDomain: %v", err) |
| + } |
| + |
| + _, err := RemoveDomain(ctx, s, p, w, testDeployment(), "bbb", "evilapp", "victim.example") |
| + if !errors.Is(err, ErrDomainNotReferenced) { |
| + t.Fatalf("cross-account remove err = %v, want ErrDomainNotReferenced", err) |
| + } |
| + if strings.Contains(err.Error(), "secretacct") { |
| + t.Fatalf("the refusal named the holding account: %v", err) |
| + } |
| + if claim, err := getDomainClaim(ctx, s, "victim.example"); err != nil || claim == nil || claim.Account != "secretacct" { |
| + t.Fatalf("another account's claim was disturbed: %+v (err %v)", claim, err) |
| + } |
| + if _, ok := p.Identities[identityKey("eu-west-1", "victim.example")]; !ok { |
| + t.Fatal("another account's identity was destroyed") |
| + } |
| + // A domain nobody ever claimed answers identically. |
| + _, err2 := RemoveDomain(ctx, s, p, w, testDeployment(), "bbb", "evilapp", "unclaimed.example") |
| + if err.Error() != strings.Replace(err2.Error(), "unclaimed.example", "victim.example", 1) { |
| + t.Fatalf("claimed-by-another and never-claimed read differently:\n %v\n %v", err, err2) |
| + } |
| +} |
| + |
| +// The orphaned claim: EnsureDomain failed after claimDomain and before |
| +// PutDomain, so a global claim exists with NO record behind it. Teardown |
| +// could sweep it, but only by deleting the whole app — an operator who did |
| +// not want to do that had no recourse at all. |
| +func TestRemoveDomainSweepsAClaimWithNoDomainRecord(t *testing.T) { |
| + ctx := context.Background() |
| + s, w := release.NewDirStore(t.TempDir()), dnsrec.NewMemWriter() |
| + p := NewMemProvisioner("shared-aws-account") |
| + now := time.Date(2026, 9, 23, 12, 0, 0, 0, time.UTC) |
| + |
| + p.FailNext["EnsureIdentity"] = errors.New("SES throttled") |
| + if _, _, err := EnsureDomain(ctx, s, p, w, testDeployment(), "aaa", "app1", "stranded.example", now); err == nil { |
| + t.Fatal("expected the injected failure") |
| + } |
| + if claim, err := getDomainClaim(ctx, s, "stranded.example"); err != nil || claim == nil { |
| + t.Fatalf("fixture broken: no stranded claim: %+v (err %v)", claim, err) |
| + } |
| + |
| + out, err := RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app1", "stranded.example") |
| + if err != nil { |
| + t.Fatalf("RemoveDomain: %v", err) |
| + } |
| + if !out.ClaimOnly || !out.TornDown { |
| + t.Fatalf("orphan sweep did not report itself as one: %+v", out) |
| + } |
| + if claim, err := getDomainClaim(ctx, s, "stranded.example"); err != nil || claim != nil { |
| + t.Fatalf("the stranded claim survived: %+v (err %v)", claim, err) |
| + } |
| + other := NewMemProvisioner("shared-aws-account") |
| + if _, _, err := EnsureDomain(ctx, s, other, w, testDeployment(), "bbb", "jam", "stranded.example", now); err != nil { |
| + t.Fatalf("another account still cannot claim the freed domain: %v", err) |
| + } |
| +} |
| + |
| +// THE ORDERING INVARIANT (walk.go's RELEASE BEFORE DELETE comment). A crash |
| +// between releasing the claim and deleting the record must be recoverable — |
| +// which it is precisely because the release came first. The reverse order |
| +// strands the claim forever: the record would be gone, so nothing would |
| +// ever list the domain again to reach the release call. |
| +func TestRemoveDomainClaimReleaseSurvivesAFailedRecordDelete(t *testing.T) { |
| + ctx := context.Background() |
| + inner, w := release.NewDirStore(t.TempDir()), dnsrec.NewMemWriter() |
| + p := NewMemProvisioner("shared-aws-account") |
| + now := time.Date(2026, 9, 23, 12, 0, 0, 0, time.UTC) |
| + |
| + if _, _, err := EnsureDomain(ctx, inner, p, w, testDeployment(), "aaa", "app1", "mine.example", now); err != nil { |
| + t.Fatalf("EnsureDomain: %v", err) |
| + } |
| + s := &failDeleteOnce{Store: inner, key: DomainKey("aaa", "mine.example")} |
| + if _, err := RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app1", "mine.example"); err == nil { |
| + t.Fatal("expected the injected record-delete failure to surface") |
| + } |
| + if claim, err := getDomainClaim(ctx, inner, "mine.example"); err != nil || claim != nil { |
| + t.Fatalf("claim survived a failed record delete: %+v (err %v)", claim, err) |
| + } |
| + // Free immediately, with no retry required of anyone. |
| + other := NewMemProvisioner("shared-aws-account") |
| + if _, _, err := EnsureDomain(ctx, inner, other, w, testDeployment(), "bbb", "jam", "mine.example", now); err != nil { |
| + t.Fatalf("another account cannot claim the released domain: %v", err) |
| + } |
| + // And the stale record is still swept by the app's own teardown, which |
| + // must not error trying to release a claim that is now somebody else's. |
| + if err := Teardown(ctx, inner, p, w, testDeployment(), "aaa", "app1"); err != nil { |
| + t.Fatalf("teardown did not repair the leftover record: %v", err) |
| + } |
| +} |
| + |
| +// The departing app's IAM policy must SHRINK. EnsureUser writes a policy |
| +// wholesale from the current granted set and nothing else recomputes it |
| +// when a domain leaves — so without RemoveDomain's own RefreshPolicies |
| +// call, an app that drops a domain a sibling keeps alive goes on holding |
| +// ses:SendRawEmail at a live identity ARN it no longer has any claim to. |
| +func TestRemoveDomainShrinksTheDepartingAppsPolicy(t *testing.T) { |
| + ctx, s, p, pub, now := enableFixture(t) |
| + extra := "extra.bag.oncarlos.com" |
| + w := dnsrec.NewMemWriter() |
| + |
| + // A second app joins the extra domain too, so removing harbourmail's |
| + // referral leaves the identity ALIVE — the case where a stale policy is |
| + // a live over-grant rather than a dangling string. |
| + for _, app := range []string{"harbourmail", "marketing"} { |
| + if _, _, err := EnsureDomain(ctx, s, p, w, testDeployment(), "bag", app, extra, now); err != nil { |
| + t.Fatalf("EnsureDomain %s: %v", app, err) |
| + } |
| + } |
| + p.SetIdentityStatus("eu-west-1", extra, Status{DKIM: "SUCCESS", MailFrom: "SUCCESS", Sending: "SUCCESS"}) |
| + if _, err := VerifyDomain(ctx, s, p, testDeployment(), "bag", extra, "eu-west-1", now); err != nil { |
| + t.Fatalf("VerifyDomain: %v", err) |
| + } |
| + if _, err := Enable(ctx, s, p, pub, testDeployment(), "bag", "harbourmail", now); err != nil { |
| + t.Fatalf("Enable: %v", err) |
| + } |
| + user := p.Users[UserName("bag", "harbourmail")] |
| + if user == nil || !strings.Contains(user.Policy, extra) { |
| + t.Fatalf("fixture broken: policy does not name %s:\n%v", extra, user) |
| + } |
| + |
| + if _, err := RemoveDomain(ctx, s, p, w, testDeployment(), "bag", "harbourmail", extra); err != nil { |
| + t.Fatalf("RemoveDomain: %v", err) |
| + } |
| + user = p.Users[UserName("bag", "harbourmail")] |
| + if user == nil { |
| + t.Fatal("the delivered principal disappeared") |
| + } |
| + if strings.Contains(user.Policy, extra) { |
| + t.Fatalf("policy still grants sending at a domain this app no longer uses:\n%s", user.Policy) |
| + } |
| + if !strings.Contains(user.Policy, "harbourmail.bag.oncarlos.com") { |
| + t.Fatalf("policy lost the domain this app still sends from:\n%s", user.Policy) |
| + } |
| + if _, ok := p.Identities[identityKey("eu-west-1", extra)]; !ok { |
| + t.Fatal("the identity the sibling app still uses was destroyed") |
| + } |
| +} |
| + |
| +// The claim must NOT be released when the identity could not actually be |
| +// destroyed. detachDomainApp reaches releaseDomainClaim only after |
| +// DeleteIdentity succeeds, and SES refuses that delete while a tenant |
| +// association still exists (spec §4.1) — so a `rm` that trips it leaves the |
| +// domain exactly as it was. Releasing anyway is the one thing this verb |
| +// must never do: it would hand a stranger a live identity's ARN and its |
| +// already-minted DKIM tokens. |
| +func TestRemoveDomainKeepsTheClaimWhenTheIdentityCannotBeDeleted(t *testing.T) { |
| + ctx, s, p, w, _ := removeFixture(t) |
| + if _, err := RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app1", "shared.example"); err != nil { |
| + t.Fatalf("remove app1: %v", err) |
| + } |
| + |
| + // Left associated on purpose: EnsureDomain associated it and nothing |
| + // has removed it, so this is the case DeleteIdentity's guard exists for. |
| + p.FailNext["DisassociateTenantResource"] = errors.New("AccessDenied: ses:DeleteTenantResourceAssociation") |
| + |
| + if _, err := RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app2", "shared.example"); err == nil { |
| + t.Fatal("rm succeeded despite the identity still being associated") |
| + } |
| + if claim, err := getDomainClaim(ctx, s, "shared.example"); err != nil || claim == nil { |
| + t.Fatalf("the claim was released while the identity is still alive: %+v (err %v)", claim, err) |
| + } |
| + if _, ok := p.Identities[identityKey("eu-west-1", "shared.example")]; !ok { |
| + t.Fatal("the identity was deleted despite still being associated") |
| + } |
| + stranger := NewMemProvisioner("shared-aws-account") |
| + now := time.Date(2026, 9, 23, 12, 0, 0, 0, time.UTC) |
| + if _, _, err := EnsureDomain(ctx, s, stranger, w, testDeployment(), "bbb", "evilapp", "shared.example", now); err == nil { |
| + t.Fatal("a stranger claimed a domain whose identity is still live") |
| + } |
| +} |
| + |
| +// The mirror of Teardown's own I2 case: a failed disassociate that does not |
| +// matter (nothing was really associated) must be logged, not fatal, so `rm` |
| +// still reaches DeleteIdentity and releases the claim. A hard error here |
| +// would recreate the permanent stranding this verb exists to end. |
| +func TestRemoveDomainFinishesDespiteAnInconsequentialDisassociateFailure(t *testing.T) { |
| + ctx, s, p, w, _ := removeFixture(t) |
| + if _, err := RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app1", "shared.example"); err != nil { |
| + t.Fatalf("remove app1: %v", err) |
| + } |
| + |
| + // Stand in for a domain that predates the tenant-association release: |
| + // verified, but never actually associated. |
| + ident := p.Identities[identityKey("eu-west-1", "shared.example")] |
| + delete(p.TenantResources[TenantName("aaa")], ident.ARN) |
| + p.FailNext["DisassociateTenantResource"] = errors.New("AccessDenied: ses:DeleteTenantResourceAssociation") |
| + |
| + out, err := RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app2", "shared.example") |
| + if err != nil { |
| + t.Fatalf("RemoveDomain: %v, want the failed disassociate logged and the removal to finish", err) |
| + } |
| + if !out.TornDown { |
| + t.Fatalf("removal did not tear down: %+v", out) |
| + } |
| + if claim, err := getDomainClaim(ctx, s, "shared.example"); err != nil || claim != nil { |
| + t.Fatalf("the claim was stranded by an inconsequential disassociate failure: %+v (err %v)", claim, err) |
| + } |
| +} |
diff --git a/mail_cli.go b/mail_cli.go| index caf2d6517..81711c54c 100644 |
| --- a/mail_cli.go |
| +++ b/mail_cli.go |
| @@ -1,6 +1,6 @@ |
| // mail_cli.go is `carlos email`: the member surface for tenant email sending |
| -// (tenant-email spec, task 18). It calls the nine doors internal/console/ |
| -// apimail.go added in task 17 — API mode only, store.go's sibling and its |
| +// (tenant-email spec, task 18). It calls the ten doors internal/console/ |
| +// apimail.go carries (nine from task 17, plus `domains rm`) — API mode only, store.go's sibling and its |
| // pattern: a package-level flag family, run* verbs that resolve a session |
| // and print* helpers that take an io.Writer so the honesty rules below can |
| // be pinned without a subprocess. |
| @@ -75,6 +75,16 @@ const emailUsage = `usage: carlos email <enable|domains|status|test|credentials| |
| sending domain for an app already declared, print |
| the record set on a custom domain, and poll to |
| verified the same way. |
| + domains rm --app <a> <domain> |
| + stop this app sending from a domain and, when it |
| + was the LAST app using it, tear the SES identity |
| + down and RELEASE the global claim — the only way |
| + short of deleting the app to make a domain |
| + claimable by another CARLOS account again. |
| + Refused while <domain> is where this app's own |
| + From address sits: move that first. A custom |
| + domain's DNS is yours, so the now-stale records |
| + are printed rather than deleted. |
| status --app <a> domains and their per-region verification state, |
| today's counts against the cap, and the pause |
| state. |
| @@ -301,6 +311,21 @@ type apiEmailDomainVerifyReply struct { |
| Domain apiEmailDomain `json:"domain"` |
| } |
| +// apiEmailDomainRemoveReply is handleAPIEmailDomainRemove's reply. It says |
| +// what actually happened rather than just "ok", because the three outcomes |
| +// have different consequences for the member: a sibling app kept the domain |
| +// alive (nothing was released), this app was the last one (the claim is now |
| +// free for any account), or only a stranded claim existed to sweep. |
| +type apiEmailDomainRemoveReply struct { |
| + App string `json:"app"` |
| + Domain string `json:"domain"` |
| + Kind string `json:"kind"` |
| + TornDown bool `json:"torn_down"` |
| + ClaimOnly bool `json:"claim_only"` |
| + RemainingApps []string `json:"remaining_apps"` |
| + Orphaned []apiDNSRecord `json:"orphaned"` |
| +} |
| + |
| // apiEmailCredsReply is handleAPIEmailCredentialsCreate's reply — THE ONLY |
| // one anywhere in this file that carries a password. |
| type apiEmailCredsReply struct { |
| @@ -444,6 +469,21 @@ func (s *apiSession) emailDomainVerify(ctx context.Context, a apiAcct, app, doma |
| return out, nil |
| } |
| +func (s *apiSession) emailDomainRemove(ctx context.Context, a apiAcct, app, domain string) (apiEmailDomainRemoveReply, error) { |
| + status, body, err := s.deleteJSON(ctx, apiPath(a, app, "/email/domains/"+pathEscape(domain))) |
| + if err != nil { |
| + return apiEmailDomainRemoveReply{}, err |
| + } |
| + if status != http.StatusOK { |
| + return apiEmailDomainRemoveReply{}, mailAPIError(status, body) |
| + } |
| + var out apiEmailDomainRemoveReply |
| + if err := json.Unmarshal(body, &out); err != nil { |
| + return apiEmailDomainRemoveReply{}, fmt.Errorf("email domains rm: unreadable response: %w", err) |
| + } |
| + return out, nil |
| +} |
| + |
| func (s *apiSession) emailEnable(ctx context.Context, a apiAcct, app string) (apiEmailReply, error) { |
| status, body, err := s.postJSON(ctx, apiPath(a, app, "/email/enable"), struct{}{}) |
| if err != nil { |
| @@ -739,11 +779,23 @@ func runEmailEnableFlow(ctx context.Context, w io.Writer, sess *apiSession, acct |
| // ---- domains add ------------------------------------------------------------ |
| func runEmailDomains(args []string) { |
| - if len(args) == 0 || args[0] != "add" { |
| + if len(args) == 0 { |
| + fmt.Fprint(os.Stderr, emailUsage) |
| + os.Exit(1) |
| + } |
| + switch args[0] { |
| + case "add": |
| + runEmailDomainsAdd(args[1:]) |
| + // "remove" is accepted alongside "rm" because this is the one verb in |
| + // the family whose effect is irreversible for other people — a domain |
| + // released is a domain a stranger may claim — and a member who spells |
| + // it out in full should not be answered with a usage block. |
| + case "rm", "remove": |
| + runEmailDomainsRemove(args[1:]) |
| + default: |
| fmt.Fprint(os.Stderr, emailUsage) |
| os.Exit(1) |
| } |
| - runEmailDomainsAdd(args[1:]) |
| } |
| func runEmailDomainsAdd(args []string) { |
| @@ -807,6 +859,110 @@ func printPendingRecords(w io.Writer, recs []apiDNSRecord) { |
| tw.Flush() |
| } |
| +// ---- domains rm ------------------------------------------------------------- |
| + |
| +// runEmailDomainsRemove is `carlos email domains rm`: the release door for a |
| +// sending domain's GLOBAL claim. |
| +// |
| +// Why it exists, since the shape is otherwise unremarkable: the claim |
| +// (internal/mail/domainclaim.go) is a deliberately global, create-only |
| +// object, so exactly one CARLOS account can hold a domain. That is right. |
| +// What was missing was the way back out — the only thing that ever released |
| +// a claim was permanently DELETING the app that held it, so a domain |
| +// claimed by the wrong app of the wrong account was unreachable to |
| +// everybody, its rightful owner included, forever. This is that door. |
| +func runEmailDomainsRemove(args []string) { |
| + fs := flag.NewFlagSet("email domains rm", flag.ExitOnError) |
| + app := appFlag(fs, "app to remove the domain from") |
| + acctRaw, acctIDRaw, _ := accountFlags(fs) |
| + consoleFlag := fs.String("console", "", "console origin (API mode only; default: the only logged-in one)") |
| + fs.Usage = flagUsage(fs, "") |
| + fs.Parse(args) |
| + usage := "usage: carlos email domains rm --app <a> [--account <sqid-or-name>] " + |
| + "[--account-id <sqid>] [--console <origin>] <domain>" |
| + if fs.NArg() != 1 { |
| + log.Fatal(usage) |
| + } |
| + domain := strings.ToLower(strings.TrimSpace(fs.Arg(0))) |
| + name := strings.TrimSpace(*app) |
| + if name != "" { |
| + if err := validAppName(name); err != nil { |
| + log.Fatalf("email domains rm: %v", err) |
| + } |
| + } |
| + |
| + sess := requireMailAPISession(*consoleFlag) |
| + printUsing(stdout, sess) |
| + acct := resolvedAccount(sess, acctRaw, acctIDRaw) |
| + name = resolvedApp(sess, acct, name, usage) |
| + |
| + res, err := sess.emailDomainRemove(context.Background(), acct, name, domain) |
| + if err != nil { |
| + // No retry framing and no "permanent" framing either, unlike |
| + // `domains add`: this command's two refusals are both states the |
| + // caller can change (move the From; name a domain the app actually |
| + // uses), and the console's own message already says which and how. |
| + log.Fatalf("email domains rm: %v", err) |
| + } |
| + printDomainRemoval(stdout, res) |
| +} |
| + |
| +// printDomainRemoval says which of the three things happened, because they |
| +// are not interchangeable: a sibling app kept the domain alive, this app was |
| +// the last one and the claim is now free, or there was never a record at all |
| +// and a stranded claim was swept. |
| +func printDomainRemoval(w io.Writer, res apiEmailDomainRemoveReply) { |
| + c := colorFor(w) |
| + fmt.Fprintf(w, "%s %s from %s\n", c.ok("removed"), res.Domain, res.App) |
| + switch { |
| + case !res.TornDown: |
| + mutedf(w, " %s still uses it, so its identity, its DNS and its claim are untouched", |
| + strings.Join(res.RemainingApps, ", ")) |
| + return |
| + case res.ClaimOnly: |
| + mutedf(w, " there was no domain record — only a stranded claim, now released") |
| + default: |
| + mutedf(w, " last app to use it: the SES identity is gone and the domain is claimable again") |
| + } |
| + if len(res.Orphaned) == 0 { |
| + return |
| + } |
| + // A CUSTOM domain's records live in the customer's own zone and are the |
| + // customer's to remove — CARLOS never published them and will not delete |
| + // them. Printing them is the whole point: nothing else in this CLI can |
| + // tell an operator which names in their zone just went stale. |
| + dead, durable := splitDKIMRecords(res.Orphaned) |
| + if len(dead) > 0 { |
| + fmt.Fprintln(w, "these records in your DNS are now dead — the identity that signed with them is gone:") |
| + printPendingRecords(w, dead) |
| + } |
| + if len(durable) > 0 { |
| + fmt.Fprintf(w, "these are unused but still correct — re-creating %s here would want them back unchanged:\n", res.Domain) |
| + printPendingRecords(w, durable) |
| + } |
| +} |
| + |
| +// splitDKIMRecords divides an orphaned record set into the half that is now |
| +// DEAD and the half that is merely unused. |
| +// |
| +// Only the DKIM CNAMEs are derived from the identity's own tokens |
| +// (mail.RecordSet), so only they stop meaning anything the moment that |
| +// identity is deleted. The MAIL FROM MX, its SPF TXT and the DMARC TXT are |
| +// deterministic functions of the domain and the region: they come back |
| +// byte-identical if the domain is ever re-created here, so telling an |
| +// operator to rip them out would be advice that costs them a second |
| +// publish for nothing. |
| +func splitDKIMRecords(recs []apiDNSRecord) (dead, durable []apiDNSRecord) { |
| + for _, r := range recs { |
| + if r.Type == "CNAME" { |
| + dead = append(dead, r) |
| + continue |
| + } |
| + durable = append(durable, r) |
| + } |
| + return dead, durable |
| +} |
| + |
| // ---- status ----------------------------------------------------------------- |
| func runEmailStatus(args []string) { |
diff --git a/mail_cli_test.go b/mail_cli_test.go| index 4e9cb11db..ed278cbb1 100644 |
| --- a/mail_cli_test.go |
| +++ b/mail_cli_test.go |
| @@ -162,6 +162,98 @@ func TestAPISessionEmailDomainVerifyWireShape(t *testing.T) { |
| } |
| } |
| +func TestAPISessionEmailDomainRemoveWireShape(t *testing.T) { |
| + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| + if r.URL.Path != "/api/cli/apps/acct/app/email/domains/x.com" || r.Method != http.MethodDelete { |
| + t.Errorf("path/method = %s %s", r.Method, r.URL.Path) |
| + } |
| + json.NewEncoder(w).Encode(map[string]any{ |
| + "app": "app", "domain": "x.com", "kind": "custom", "torn_down": true, |
| + "orphaned": []map[string]any{ |
| + {"name": "tok._domainkey.x.com", "type": "CNAME", "values": []string{"tok.dkim.amazonses.com"}, "ttl": 300}, |
| + }, |
| + }) |
| + })) |
| + defer ts.Close() |
| + |
| + res, err := newTestSession(ts.URL).emailDomainRemove(context.Background(), apiAcct{Seg: "acct"}, "app", "x.com") |
| + if err != nil { |
| + t.Fatalf("emailDomainRemove: %v", err) |
| + } |
| + if !res.TornDown || len(res.Orphaned) != 1 { |
| + t.Fatalf("reply = %+v, want a torn-down domain with one orphaned record", res) |
| + } |
| +} |
| + |
| +// TestPrintDomainRemovalSaysWhichOfTheThreeThingsHappened: the outcomes are |
| +// not interchangeable — a sibling app kept the domain alive (nothing was |
| +// released), this app was the last one (the claim is free for any account), |
| +// or only a stranded claim existed to sweep. Printing the same line for all |
| +// three would tell a member nothing about whether the domain is now |
| +// claimable. |
| +func TestPrintDomainRemovalSaysWhichOfTheThreeThingsHappened(t *testing.T) { |
| + render := func(res apiEmailDomainRemoveReply) string { |
| + var buf bytes.Buffer |
| + printDomainRemoval(&buf, res) |
| + return buf.String() |
| + } |
| + |
| + kept := render(apiEmailDomainRemoveReply{ |
| + App: "jam", Domain: "jelly.tito.io", Kind: "custom", RemainingApps: []string{"stripe"}, |
| + }) |
| + if !strings.Contains(kept, "stripe") { |
| + t.Fatalf("a kept domain does not name the app still using it:\n%s", kept) |
| + } |
| + if strings.Contains(kept, "claimable") { |
| + t.Fatalf("a kept domain claims to have been released:\n%s", kept) |
| + } |
| + |
| + torn := render(apiEmailDomainRemoveReply{ |
| + App: "jam", Domain: "jelly.tito.io", Kind: "custom", TornDown: true, |
| + }) |
| + if !strings.Contains(torn, "claimable again") { |
| + t.Fatalf("a released domain does not say it is claimable again:\n%s", torn) |
| + } |
| + |
| + swept := render(apiEmailDomainRemoveReply{ |
| + App: "jam", Domain: "jelly.tito.io", Kind: "custom", TornDown: true, ClaimOnly: true, |
| + }) |
| + if !strings.Contains(swept, "no domain record") { |
| + t.Fatalf("an orphaned-claim sweep does not say what it swept:\n%s", swept) |
| + } |
| +} |
| + |
| +// TestPrintDomainRemovalSeparatesDeadDKIMFromDurableRecords: of the six |
| +// records in a sending domain's set, only the three DKIM CNAMEs are derived |
| +// from the identity's own tokens, so only they are dead once it is gone. |
| +// The MAIL FROM MX, its SPF TXT and the DMARC TXT are deterministic |
| +// functions of the domain and region — telling an operator to rip those out |
| +// would cost them a second publish for nothing. |
| +func TestPrintDomainRemovalSeparatesDeadDKIMFromDurableRecords(t *testing.T) { |
| + var buf bytes.Buffer |
| + printDomainRemoval(&buf, apiEmailDomainRemoveReply{ |
| + App: "jam", Domain: "jelly.tito.io", Kind: "custom", TornDown: true, |
| + Orphaned: []apiDNSRecord{ |
| + {Name: "tok1._domainkey.jelly.tito.io", Type: "CNAME", Values: []string{"tok1.dkim.amazonses.com"}}, |
| + {Name: "mail-eu-west-1.jelly.tito.io", Type: "MX", Values: []string{"10 feedback-smtp.eu-west-1.amazonses.com"}}, |
| + {Name: "_dmarc.jelly.tito.io", Type: "TXT", Values: []string{"v=DMARC1; p=none"}}, |
| + }, |
| + }) |
| + out := buf.String() |
| + |
| + deadAt := strings.Index(out, "tok1._domainkey.jelly.tito.io") |
| + durableAt := strings.Index(out, "mail-eu-west-1.jelly.tito.io") |
| + if deadAt < 0 || durableAt < 0 { |
| + t.Fatalf("the orphaned records were not printed at all:\n%s", out) |
| + } |
| + if !strings.Contains(out, "now dead") || !strings.Contains(out, "still correct") { |
| + t.Fatalf("output does not separate the dead records from the durable ones:\n%s", out) |
| + } |
| + if deadAt > durableAt { |
| + t.Fatalf("the dead records must come first — they are the ones to remove:\n%s", out) |
| + } |
| +} |
| + |
| func TestAPISessionEmailEnableWireShape(t *testing.T) { |
| ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| if r.URL.Path != "/api/cli/apps/acct/app/email/enable" || r.Method != http.MethodPost { |
| @@ -989,7 +1081,7 @@ func TestEmailVerbDispatchAndRefusals(t *testing.T) { |
| return string(out), err |
| } |
| - for _, args := range [][]string{{"email"}, {"email", "wat"}} { |
| + for _, args := range [][]string{{"email"}, {"email", "wat"}, {"email", "domains"}, {"email", "domains", "wat"}} { |
| out, err := run(nil, args...) |
| if err == nil { |
| t.Fatalf("carlos %v exited 0:\n%s", args, out) |
| @@ -1008,6 +1100,7 @@ func TestEmailVerbDispatchAndRefusals(t *testing.T) { |
| {"email", "resume", "-app", "keymail"}, |
| {"email", "credentials", "create", "-app", "keymail", "-name", "laptop"}, |
| {"email", "domains", "add", "-app", "keymail", "x.com"}, |
| + {"email", "domains", "rm", "-app", "keymail", "x.com"}, |
| } { |
| out, err := run([]string{"CARLOS_DEPLOYMENT_DIR=" + t.TempDir()}, args...) |
| if err == nil { |