From 1684c88877a68e55dd5812ac90851d38bc24817f Mon Sep 17 00:00:00 2001 From: John Coffey Date: Sun, 23 Aug 2026 21:45:55 -0700 Subject: [PATCH] Resolve v0.16 domain ids to names before comparing directories x:Account.domainId is an internal id on v0.16 ("b"), not a domain name. A pre-migration snapshot taken from a v0.15 instance records names ("smoke.test"), so the post-migration directory comparison compared ids against names and would have reported every domain as having vanished - a false alarm on the check whose whole job is proving nothing was lost. The client now resolves them with x:Domain/query + x:Domain/get in a single request, using a JMAP back-reference (RFC 8620 3.7). Confirmed against a live 0.16.14 before being written: ["x:Domain/get", {"list":[{"name":"smoke.test","id":"b"}]}, "g"] An id that can't be resolved is kept as-is - a domain that can't be named is still a domain that exists - but a failure of the resolution call itself is an error rather than a silent fallback, since quietly comparing ids against names is precisely the bug being fixed. Verified against the live migrated instance: the snapshot that reported domains=[b] now reports domains=[smoke.test], matching what the pre-migration snapshot recorded. --- ARCHITECTURE.md | 16 +++-- internal/preflight/checks_test.go | 7 +++ internal/stalwartapi/management.go | 65 ++++++++++++++++++++- internal/stalwartapi/management_test.go | 7 +++ internal/stalwartapi/principal_test.go | 56 ++++++++++++++++++ internal/validate/content_integrity_test.go | 7 +++ internal/validate/main_test.go | 7 +++ 7 files changed, 157 insertions(+), 8 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 31c9194..46245a1 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -715,12 +715,16 @@ happens to need them. `preflight.DeploymentKind` is a type alias for far larger blast radius for no benefit. Where v0.15 listed several roles, admin wins and the collapse is reported; roles with no v0.16 equivalent are named rather than dropped silently. -- **`x:Account.domainId` returns an internal id on v0.16, not a domain - name.** A pre-migration snapshot records domains as names - ("smoke.test"); the same instance after migration reports "b". The - directory comparison in §4.7 would read that as every domain having - vanished. Resolving ids to names needs an `x:Domain/get` call that hasn't - been confirmed against the binary yet. +- **`x:Account.domainId` returns an internal id on v0.16 - resolved.** A + pre-migration snapshot records domains as names ("smoke.test"); the same + instance afterwards reported "b", so the §4.7 directory comparison would + have read every domain as having vanished. The client now resolves ids to + names with `x:Domain/query` + `x:Domain/get` in a single request via a + JMAP back-reference, confirmed against a live 0.16.14. An id that cannot + be resolved is kept as-is rather than dropped, since a domain that can't + be named is still a domain that exists; a failure of the resolution call + itself is an error, because silently comparing ids against names is the + bug this fixes. - **Quota recalculation is grounded but unproven.** The `x:Task` wire format comes from Stalwart's schema reference rather than a live server; §4.5 lists exactly which two details are inferred. A smoke test against a diff --git a/internal/preflight/checks_test.go b/internal/preflight/checks_test.go index b73764a..8bc5aa7 100644 --- a/internal/preflight/checks_test.go +++ b/internal/preflight/checks_test.go @@ -248,6 +248,13 @@ func TestCheckerRunCapturesAccountSnapshotWhenAdminURLSet(t *testing.T) { methodCalls := body["methodCalls"].([]any) name := methodCalls[0].([]any)[0].(string) switch name { + case "x:Domain/query": + json.NewEncoder(w).Encode(map[string]any{"methodResponses": []any{ + []any{"x:Domain/query", map[string]any{"ids": []string{"d1"}}, "q"}, + []any{"x:Domain/get", map[string]any{"list": []map[string]any{ + {"id": "d1", "name": "example.com"}, + }}, "g"}, + }}) case "x:Account/query": json.NewEncoder(w).Encode(map[string]any{"methodResponses": []any{ []any{"x:Account/query", map[string]any{"ids": []string{"a1"}}, "q"}, diff --git a/internal/stalwartapi/management.go b/internal/stalwartapi/management.go index bd9b998..80165d2 100644 --- a/internal/stalwartapi/management.go +++ b/internal/stalwartapi/management.go @@ -179,11 +179,24 @@ func (c *Client) accountSnapshotJMAP(ctx context.Context) (*Snapshot, error) { mailboxCounts[a.Name] = counts } + // Resolve ids to names so this snapshot is comparable with one taken + // from a v0.15 instance, which records names. + domainNames, err := c.domainNames(ctx) + if err != nil { + return nil, fmt.Errorf("stalwartapi: resolve domain names (an unresolved id would make every domain look missing): %w", err) + } domainSet := map[string]bool{} for _, a := range accounts { - if a.DomainID != "" { - domainSet[a.DomainID] = true + if a.DomainID == "" { + continue } + if name, ok := domainNames[a.DomainID]; ok && name != "" { + domainSet[name] = true + continue + } + // Keep the raw id rather than dropping the domain entirely: a + // domain that can't be named is still a domain that exists. + domainSet[a.DomainID] = true } domains := make([]string, 0, len(domainSet)) for d := range domainSet { @@ -237,6 +250,54 @@ func describeJMAPError(method string, args json.RawMessage) error { return fmt.Errorf("stalwartapi: %s error: %s", method, parsed.Type) } +// domainNames resolves v0.16 domain ids to domain names. +// +// x:Account.domainId is an internal id ("b"), not a name. A pre-migration +// snapshot taken from a v0.15 instance records domains as names +// ("smoke.test"), so comparing the two directly reports every domain as +// having vanished - a false alarm on the check that is supposed to prove +// nothing was lost. +// +// The query and the get travel in one request using a JMAP back-reference +// (RFC 8620 §3.7), confirmed against a live 0.16.14: +// +// ["x:Domain/get", {"list":[{"name":"smoke.test","id":"b"}]}, "g"] +func (c *Client) domainNames(ctx context.Context) (map[string]string, error) { + responses, err := c.call(ctx, managementCapabilities, []any{ + []any{"x:Domain/query", map[string]any{"filter": map[string]any{}}, "q"}, + []any{"x:Domain/get", map[string]any{ + "#ids": map[string]any{"resultOf": "q", "name": "x:Domain/query", "path": "/ids"}, + "properties": []string{"id", "name"}, + }, "g"}, + }) + if err != nil { + return nil, fmt.Errorf("stalwartapi: Domain/get: %w", err) + } + for _, r := range responses { + if r.Name == "error" { + return nil, describeJMAPError("Domain/get", r.Args) + } + if r.CallID != "g" { + continue + } + var result struct { + List []struct { + ID string `json:"id"` + Name string `json:"name"` + } `json:"list"` + } + if err := json.Unmarshal(r.Args, &result); err != nil { + return nil, fmt.Errorf("stalwartapi: parse Domain/get response: %w", err) + } + names := make(map[string]string, len(result.List)) + for _, d := range result.List { + names[d.ID] = d.Name + } + return names, nil + } + return nil, fmt.Errorf("stalwartapi: Domain/get returned no matching method response") +} + func accountQueryIDs(responses []methodResponse) ([]string, error) { if len(responses) == 0 { return nil, fmt.Errorf("stalwartapi: Account/query returned no method responses") diff --git a/internal/stalwartapi/management_test.go b/internal/stalwartapi/management_test.go index cfd7d0a..b5275e0 100644 --- a/internal/stalwartapi/management_test.go +++ b/internal/stalwartapi/management_test.go @@ -79,6 +79,13 @@ func accountManagementAndMailboxServer(t *testing.T, mailboxesFor map[string][]m methodName := first[0].(string) switch methodName { + case "x:Domain/query": + json.NewEncoder(w).Encode(jmapEnvelope{MethodResponses: []any{ + []any{"x:Domain/query", map[string]any{"ids": []string{"d1"}}, "q"}, + []any{"x:Domain/get", map[string]any{"list": []map[string]any{ + {"id": "d1", "name": "example.com"}, + }}, "g"}, + }}) case "x:Account/query": json.NewEncoder(w).Encode(jmapEnvelope{MethodResponses: []any{ []any{"x:Account/query", map[string]any{"ids": []string{"a1", "a2"}}, "q"}, diff --git a/internal/stalwartapi/principal_test.go b/internal/stalwartapi/principal_test.go index b35c491..a930ac7 100644 --- a/internal/stalwartapi/principal_test.go +++ b/internal/stalwartapi/principal_test.go @@ -276,3 +276,59 @@ func TestForbiddenExplainsThePostMigrationPermissionTrap(t *testing.T) { } } } + +// x:Account.domainId is an internal id on v0.16, while a v0.15 snapshot +// records domain names. Comparing the two directly reported every domain as +// missing - a false alarm on the check meant to prove nothing was lost. +func TestAccountSnapshotResolvesDomainIdsToNames(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/principal" { + w.WriteHeader(http.StatusNotFound) + return + } + if r.Method == http.MethodGet && r.URL.Path == "/.well-known/jmap" { + user, _, _ := r.BasicAuth() + if strings.Contains(user, "%") { + w.WriteHeader(http.StatusForbidden) // no impersonation here + return + } + json.NewEncoder(w).Encode(map[string]any{"apiUrl": "/jmap/"}) + return + } + var body map[string]any + json.NewDecoder(r.Body).Decode(&body) + name := body["methodCalls"].([]any)[0].([]any)[0].(string) + switch name { + case "x:Domain/query": + json.NewEncoder(w).Encode(jmapEnvelope{MethodResponses: []any{ + []any{"x:Domain/query", map[string]any{"ids": []string{"b"}}, "q"}, + []any{"x:Domain/get", map[string]any{"list": []map[string]any{ + {"id": "b", "name": "smoke.test"}, + }}, "g"}, + }}) + case "x:Account/query": + json.NewEncoder(w).Encode(jmapEnvelope{MethodResponses: []any{ + []any{"x:Account/query", map[string]any{"ids": []string{"e"}}, "q"}, + }}) + case "x:Account/get": + json.NewEncoder(w).Encode(jmapEnvelope{MethodResponses: []any{ + // domainId is the id, exactly as a real 0.16.14 returns it. + []any{"x:Account/get", map[string]any{"list": []map[string]any{ + {"id": "e", "name": "alice@smoke.test", "domainId": "b", "usedDiskQuota": 9207}, + }}, "g"}, + }}) + default: + t.Errorf("unexpected method call %s", name) + } + })) + defer srv.Close() + + client := &Client{BaseURL: srv.URL, Username: "sysadmin", Password: "x"} + snap, err := client.AccountSnapshot(context.Background()) + if err != nil { + t.Fatal(err) + } + if len(snap.Domains) != 1 || snap.Domains[0] != "smoke.test" { + t.Errorf("Domains = %v, want [smoke.test] - ids must be resolved or every domain reads as missing", snap.Domains) + } +} diff --git a/internal/validate/content_integrity_test.go b/internal/validate/content_integrity_test.go index dda5e25..1b3c0d1 100644 --- a/internal/validate/content_integrity_test.go +++ b/internal/validate/content_integrity_test.go @@ -60,6 +60,13 @@ func fakeManagementServer(t *testing.T, accounts []map[string]any, mailboxesByEm call := methodCalls[0].([]any) name := call[0].(string) switch name { + case "x:Domain/query": + json.NewEncoder(w).Encode(jmapEnvelope{MethodResponses: []any{ + []any{"x:Domain/query", map[string]any{"ids": []string{"d1"}}, "q"}, + []any{"x:Domain/get", map[string]any{"list": []map[string]any{ + {"id": "d1", "name": "smoke.test"}, + }}, "g"}, + }}) case "x:Account/query": ids := make([]string, len(accounts)) for i, a := range accounts { diff --git a/internal/validate/main_test.go b/internal/validate/main_test.go index b7b6412..565ef40 100644 --- a/internal/validate/main_test.go +++ b/internal/validate/main_test.go @@ -79,6 +79,13 @@ func runFakeStalwartServer() { call := methodCalls[0].([]any) name := call[0].(string) switch name { + case "x:Domain/query": + json.NewEncoder(w).Encode(map[string]any{"methodResponses": []any{ + []any{"x:Domain/query", map[string]any{"ids": []string{"d1"}}, "q"}, + []any{"x:Domain/get", map[string]any{"list": []map[string]any{ + {"id": "d1", "name": "example.com"}, + }}, "g"}, + }}) case "x:Account/query": json.NewEncoder(w).Encode(map[string]any{"methodResponses": []any{ []any{"x:Account/query", map[string]any{"ids": []string{"a1"}}, "q"},