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()) + } +}