From 019696bce7d04c56a5538080a877bf6e95fc67a0 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Mon, 24 Aug 2026 16:02:31 -0700 Subject: [PATCH] Do not call it data loss when the instance would not show us the accounts The dress rehearsal reported MISSING ACCOUNT postmaster@inbuxa.com after a migration that lost nothing: SMTP on the migrated instance accepts mail for it, while an address that does not exist is refused, so the account is there. Enumeration is permission-scoped. The "before" snapshot was read by an account with full rights; the "after" snapshot by one whose admin role the migration had not carried across - proven one step earlier, where the quota rebuild was refused as unauthorised. It was shown a fraction of the accounts and the comparison called the remainder lost. "Missing" and "not permitted to see" are different findings and only one of them is about the data. When the migrated instance shows fewer accounts than existed, the result now says COULD NOT VERIFY and names the reader, rather than asserting a loss the evidence does not support. It still fails the run: an unverified migration is not a verified one, and the fix is to re-check with an admin account, which --resume makes cheap. Counting accounts is not a strong enough signal to tell the two cases apart, so it does not try to - it reports the ambiguity instead of guessing. Also fixes the test fake, which ignored ?types= and returned domains as individuals, making every account count in these tests wrong. --- internal/validate/content_integrity.go | 21 ++++++++ internal/validate/live.go | 12 +++++ internal/validate/live_test.go | 72 +++++++++++++++++++++++++- 3 files changed, 103 insertions(+), 2 deletions(-) diff --git a/internal/validate/content_integrity.go b/internal/validate/content_integrity.go index 719ad99..9a5132c 100644 --- a/internal/validate/content_integrity.go +++ b/internal/validate/content_integrity.go @@ -41,6 +41,11 @@ type ContentIntegrityResult struct { MessageCountMismatches []MailboxDelta // present both before and after, but with a different message count MissingDomains []string // present before, absent after MessageCountsCompared bool // false when the source version could not report counts + // AccountsVisibleAfter is how many accounts the migrated instance was + // willing to show whoever asked. An account without management + // permission is shown far fewer than exist - so a short list is a + // statement about the reader, not about the data. + AccountsVisibleAfter int } // OK reports whether everything that must match did: no account and no mail @@ -64,6 +69,20 @@ func (r ContentIntegrityResult) DomainsOK() bool { return len(r.MissingDomains) == 0 } +// Inconclusive reports that the migrated instance showed fewer accounts than +// existed before, which makes any "missing" finding unsafe to believe. +// +// Enumeration is permission-scoped: an account that authenticates but holds +// no management role is shown only what it may see, and a v0.16 migration +// does not always carry an admin role across - so the account that read the +// "before" side may not have the same reach afterwards. Observed on a clone +// of production: every account survived, and the comparison called one of +// them lost because the reader could no longer see it. Reporting data loss +// on that evidence is worse than reporting nothing. +func (r ContentIntegrityResult) Inconclusive() bool { + return len(r.MissingAccounts) > 0 && r.AccountsVisibleAfter < r.AccountsChecked +} + func (r ContentIntegrityResult) String() string { var b strings.Builder if r.MessageCountsCompared { @@ -140,6 +159,8 @@ func compareContentIntegrity(ctx context.Context, client *stalwartapi.Client, be afterAccounts[a] = true } + result.AccountsVisibleAfter = len(afterAccounts) + for _, d := range before.Domains { if !containsDomain(after.Domains, d) { result.MissingDomains = append(result.MissingDomains, d) diff --git a/internal/validate/live.go b/internal/validate/live.go index 824bfa5..b3dd723 100644 --- a/internal/validate/live.go +++ b/internal/validate/live.go @@ -72,6 +72,18 @@ func RunLive(ctx context.Context, store *checkpoint.Store, rs *checkpoint.RunSta return checkpoint.StepOutcome{}, err } switch { + case r.Inconclusive(): + // Fewer accounts came back than existed, so "missing" cannot be + // told apart from "not permitted to see". Still a failure - an + // unverified migration is not a verified one - but it must not + // be reported as data loss, which is a different claim and one + // this evidence does not support. + return checkpoint.StepOutcome{Verdict: string(StatusFail), Detail: fmt.Sprintf( + "COULD NOT VERIFY (not the same as data loss): the migrated instance showed %d of %d account(s) to %s. "+ + "Either those accounts are gone, or this account cannot see them - enumeration is permission-scoped, and a "+ + "migration does not always carry an admin role across. Re-check with an account that holds admin on the "+ + "migrated instance before concluding either. Findings: %s", + r.AccountsVisibleAfter, r.AccountsChecked, opts.AdminUser, r.String())}, nil case !r.OK(): // Recorded as a completed step with a failing verdict rather // than an error: the comparison ran, and its answer is the diff --git a/internal/validate/live_test.go b/internal/validate/live_test.go index b965d7b..59c6d9c 100644 --- a/internal/validate/live_test.go +++ b/internal/validate/live_test.go @@ -190,8 +190,76 @@ func fakeInstance(t *testing.T, domains []string, accounts map[string]float64) * for name, quota := range accounts { items = append(items, map[string]any{"type": "individual", "name": name, "usedQuota": quota}) } - return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Honour ?types= the way the real REST API does: asking for + // individuals must not hand back domains, or every count is wrong. + want := r.URL.Query().Get("types") + out := items + if want != "" { + out = nil + for _, it := range items { + if it["type"] == want { + out = append(out, it) + } + } + } w.Header().Set("content-type", "application/json") - _ = json.NewEncoder(w).Encode(map[string]any{"data": map[string]any{"items": items, "total": len(items)}}) + _ = json.NewEncoder(w).Encode(map[string]any{"data": map[string]any{"items": out, "total": len(out)}}) })) } + +// Enumeration is permission-scoped. An account that authenticates but holds +// no management role is shown only what it may see, and a v0.16 migration +// does not always carry an admin role across - so the account that read the +// "before" side may have less reach afterwards. Observed on a clone of +// production: every account survived, SMTP confirmed it, and the comparison +// still called one lost because the reader could no longer see it. +func TestRunLiveWillNotCallItLossWhenItCouldNotLook(t *testing.T) { + // The migrated instance shows only the reader's own account. + srv := fakeInstance(t, []string{"example.org"}, map[string]float64{"ann@example.org": 10}) + defer srv.Close() + + store, rs := newRun(t) + report, err := RunLive(context.Background(), store, rs, LiveOptions{ + AdminURL: srv.URL, AdminUser: "ann@example.org", AdminPassword: "pw", HTTPClient: srv.Client(), + Before: &checkpoint.PreflightSnapshot{ + Domains: []string{"example.org"}, + UsedQuota: map[string]int64{"ann@example.org": 1, "bob@example.org": 2, "postmaster@example.org": 3}, + }, + }) + if err != nil { + t.Fatalf("RunLive: %v", err) + } + // It still fails: an unverified migration is not a verified one. What it + // must not do is call it data loss, which is a different claim. + if !report.Blocking() { + t.Fatal("an unverifiable result must still stop the run") + } + detail := report.Results[0].Detail + for _, want := range []string{"COULD NOT VERIFY", "not the same as data loss", "permission-scoped", "1 of 3"} { + if !strings.Contains(detail, want) { + t.Fatalf("detail should contain %q, got %q", want, detail) + } + } +} + +// The other side of it: when the instance shows everything and an account is +// genuinely absent, that is still a failure. +func TestRunLiveStillFailsWhenTheInstanceShowedEverything(t *testing.T) { + srv := fakeInstance(t, []string{"example.org"}, map[string]float64{ + "ann@example.org": 10, "carol@example.org": 20, "dave@example.org": 30, + }) + defer srv.Close() + + store, rs := newRun(t) + report, _ := RunLive(context.Background(), store, rs, LiveOptions{ + AdminURL: srv.URL, AdminUser: "ann@example.org", AdminPassword: "pw", HTTPClient: srv.Client(), + Before: &checkpoint.PreflightSnapshot{ + Domains: []string{"example.org"}, + UsedQuota: map[string]int64{"ann@example.org": 1, "bob@example.org": 2}, + }, + }) + if !report.Blocking() { + t.Fatalf("a genuine loss must still block, got: %s", report.String()) + } +}