From 28c0fa57cb4deff4ad408413a6ea495c14a6dc2b Mon Sep 17 00:00:00 2001 From: John Coffey Date: Sun, 23 Aug 2026 21:42:40 -0700 Subject: [PATCH] Restore administrator roles that migrate_v016.py drops Chased down why a migrated instance had no working administrator. The account authenticated fine and was refused every management call, and the cause is that migrate_v016.py assigns every migrated account the User role regardless of what it held before: an account that was `roles: ["admin"]` in v0.15 comes out the far side as `roles: {"@type": "User"}`. Ordinary users were never affected - User is what they had and what they get - and their credentials, mail and mailboxes survive untouched. It is specifically administrators who lose their privileges, which is a bad thing to discover after cutting over. The v0.16 shape came from the server's own schema document rather than the published reference: GET /api/schema defines x:UserRoles as a multi-variant type with variants User, Admin and Custom. Account is itself multi-variant, so an upsert needs its own "@type" too - without it the server rejects the operation outright ("upsert entry is missing `@type`"). applyplan.AccountRoleOperations restores roles from the principals dump, emitting operations only for accounts whose role actually changes. Rewriting every account would be a much larger blast radius for no benefit. Where v0.15 listed several roles, admin wins - under-privileging an administrator locks them out, which is the failure being fixed - and the collapse is reported rather than done silently, as are roles with no known v0.16 equivalent. Verified end to end on the smoke VM: rehearse against the real 0.15.5 put the role operation in the supplement, applying that supplement to a migrated 0.16.14 whose admin was broken restored management access (accounts=3), and alice and bob logged in over IMAPS with unchanged credentials, read their mail, and accepted new SMTP delivery. Also recorded: x:Account.domainId returns an internal id on v0.16, not a domain name, so the post-migration directory comparison would read every domain as missing. Resolving that needs an x:Domain/get call not yet confirmed against the binary. --- ARCHITECTURE.md | 30 ++++++-- cmd/stalwart-migrate/rehearse.go | 22 +++++- internal/applyplan/roles.go | 113 +++++++++++++++++++++++++++++++ internal/applyplan/roles_test.go | 113 +++++++++++++++++++++++++++++++ internal/backup/settingsdump.go | 25 +++++++ 5 files changed, 294 insertions(+), 9 deletions(-) create mode 100644 internal/applyplan/roles.go create mode 100644 internal/applyplan/roles_test.go diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 79052bb..31c9194 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -698,13 +698,29 @@ happens to need them. `preflight.DeploymentKind` is a type alias for it reachable, but the migrated instance refused the call - below), **systemd drop-in** handling, and anything on a **non-RocksDB backend** or a **Docker** deployment. -- **A migrated instance may have no working administrator.** An account - holding the admin role before migration was refused `x:Account/query` - afterwards with `forbidden`. Whether the role failed to carry or v0.16 - requires different permissions was not isolated; the operator's position - is the same either way. The client now explains this rather than - reporting a bare "forbidden", but the underlying question is open and - gates both quota recalculation and any post-migration validation. +- **A migrated instance has no working administrator - diagnosed and + fixed.** `migrate_v016.py` assigns every migrated account the `User` + role regardless of what it held before, so an account that was an + administrator in v0.15 comes out authenticating normally and refused + every management call. Ordinary users are unaffected: `User` is what they + had and what they get, and their credentials, mail and mailboxes all + survive untouched. + + The v0.16 shape came from the server's own schema document + (`GET /api/schema`): `x:UserRoles` is a multi-variant type with variants + `User`, `Admin` and `Custom`, and `Account` is itself multi-variant, so + the upsert needs its own `@type` as well. `applyplan.AccountRoleOperations` + restores it from the principals dump, emitting operations only for + accounts whose role actually changes - rewriting every account would be a + 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. - **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/cmd/stalwart-migrate/rehearse.go b/cmd/stalwart-migrate/rehearse.go index fc209dc..1d7296b 100644 --- a/cmd/stalwart-migrate/rehearse.go +++ b/cmd/stalwart-migrate/rehearse.go @@ -222,7 +222,7 @@ func runRehearse(args []string) (err error) { // worth having precisely because it is honest about how much of the // worklist it does not touch. fmt.Println("\n--- supplemental plan (best-effort) ---") - if err := generateSupplement(store, rs, settingsPath, unmigratedPath, keptSupplement); err != nil { + if err := generateSupplement(store, rs, settingsPath, principalsPath, unmigratedPath, keptSupplement); err != nil { fmt.Fprintf(os.Stderr, "warning: couldn't generate the supplemental plan: %v\n", err) } if err := store.Save(rs); err != nil { @@ -264,7 +264,7 @@ func copyFile(src, dst string) error { // it: the official conversion is the authority on everything it handles, // and a generated plan that overlapped it could silently override a // correct mapping with a guessed one. -func generateSupplement(store *checkpoint.Store, rs *checkpoint.RunState, settingsPath, unmigratedPath, outPath string) error { +func generateSupplement(store *checkpoint.Store, rs *checkpoint.RunState, settingsPath, principalsPath, unmigratedPath, outPath string) error { settings, err := backup.ReadSettingsDump(settingsPath) if err != nil { return err @@ -278,6 +278,24 @@ func generateSupplement(store *checkpoint.Store, rs *checkpoint.RunState, settin if err != nil { return err } + + // Account roles don't live in the settings dump, so they come from the + // principals dump rather than through a settings Generator. Without + // this the migrated instance has no administrator: migrate_v016.py + // gives every account the User role, whatever it had before. + principals, err := backup.ReadPrincipalsDump(principalsPath) + if err != nil { + return err + } + roleOps, _, roleWarnings, err := applyplan.AccountRoleOperations(principals) + if err != nil { + return err + } + plan.Operations = append(plan.Operations, roleOps...) + coverage.Warnings = append(coverage.Warnings, roleWarnings...) + if len(roleOps) > 0 { + coverage.ObjectsByType["Account role"] += len(roleOps) + } if len(plan.Operations) == 0 { fmt.Println("nothing this tool can rebuild automatically yet - the whole worklist is manual") return nil diff --git a/internal/applyplan/roles.go b/internal/applyplan/roles.go new file mode 100644 index 0000000..207b319 --- /dev/null +++ b/internal/applyplan/roles.go @@ -0,0 +1,113 @@ +// SPDX-FileCopyrightText: 2026 LINUXexpert-org +// SPDX-License-Identifier: GPL-3.0-or-later + +package applyplan + +import ( + "fmt" + "sort" + "strings" + + "github.com/LINUXexpert-org/stalwart-migrator/internal/backup" +) + +// AccountRoleOperations restores the account roles a v0.16 migration drops. +// +// migrate_v016.py assigns every migrated account the User role regardless +// of what it had before. Verified on a real migration: an account holding +// the admin role in v0.15 came out the other side with +// `roles: {"@type": "User"}`, authenticated fine, and was refused every +// management call with "forbidden". The instance had no working +// administrator, which is a bad thing to discover after cutting over. +// +// Ordinary users are unaffected - User is what they had and what they get - +// so this deliberately emits operations only for accounts whose role +// actually changes. A plan that rewrote every account would be a much +// larger blast radius for no benefit. +// +// The v0.16 shape comes from the server's own schema document +// (GET /api/schema), where x:UserRoles is a multi-variant type with +// variants User, Admin and Custom, and from confirming the upsert against +// a live 0.16.14: Account is itself multi-variant, so the entry needs its +// own "@type" as well. +func AccountRoleOperations(principals []backup.Principal) (ops []Operation, covered []string, warnings []string, err error) { + type change struct { + name string + variant string + } + var changes []change + + for _, p := range principals { + if !strings.EqualFold(p.Type, "individual") { + continue // domains and groups don't carry these roles + } + variant, note := roleVariant(p.Roles) + if note != "" { + warnings = append(warnings, fmt.Sprintf("account %q: %s", p.Name, note)) + } + if variant == "" || variant == "User" { + // User is the migration's own default; re-asserting it would + // touch every account to no effect. + continue + } + changes = append(changes, change{name: p.Name, variant: variant}) + } + sort.Slice(changes, func(i, j int) bool { return changes[i].name < changes[j].name }) + + for _, c := range changes { + ops = append(ops, Operation{ + Type: "upsert", + Object: "Account", + MatchOn: []string{"name"}, + Value: map[string]map[string]any{ + "role-" + c.name: { + // Account is a multi-variant object: without its own + // @type the upsert is rejected outright. + "@type": "User", + "name": c.name, + "roles": map[string]any{"@type": c.variant}, + }, + }, + }) + covered = append(covered, "principal:"+c.name+":roles") + } + return ops, covered, warnings, nil +} + +// roleVariant maps a v0.15 role list onto v0.16's x:UserRoles variant. +// +// v0.15 carries a list of role names; v0.16 carries one variant. Where an +// account had several, admin wins - under-privileging an administrator +// locks them out, which is the failure this function exists to prevent - +// and the collapse is reported rather than done silently. +func roleVariant(roles []string) (variant, note string) { + if len(roles) == 0 { + return "", "" + } + hasAdmin, hasUser := false, false + var unknown []string + for _, r := range roles { + switch strings.ToLower(strings.TrimSpace(r)) { + case "admin", "administrator": + hasAdmin = true + case "user": + hasUser = true + default: + unknown = append(unknown, r) + } + } + if len(unknown) > 0 { + note = fmt.Sprintf("role(s) %s have no known v0.16 equivalent and are not restored - recreate them by hand", + strings.Join(unknown, ", ")) + } + switch { + case hasAdmin && (hasUser || len(unknown) > 0): + return "Admin", note + " (collapsed several roles to Admin)" + case hasAdmin: + return "Admin", note + case hasUser: + return "User", note + default: + return "", note + } +} diff --git a/internal/applyplan/roles_test.go b/internal/applyplan/roles_test.go new file mode 100644 index 0000000..9284221 --- /dev/null +++ b/internal/applyplan/roles_test.go @@ -0,0 +1,113 @@ +// SPDX-FileCopyrightText: 2026 LINUXexpert-org +// SPDX-License-Identifier: GPL-3.0-or-later + +package applyplan + +import ( + "strings" + "testing" + + "github.com/LINUXexpert-org/stalwart-migrator/internal/backup" +) + +// The exact principal shape a real 0.15.5 reports. +var migratedPrincipals = []backup.Principal{ + {ID: 4, Type: "individual", Name: "alice", Emails: []string{"alice@smoke.test"}, Roles: []string{"user"}}, + {ID: 5, Type: "individual", Name: "bob", Emails: []string{"bob@smoke.test"}, Roles: []string{"user"}}, + {ID: 6, Type: "individual", Name: "sysadmin", Emails: []string{"sysadmin@smoke.test"}, Roles: []string{"admin"}}, + {ID: 1, Type: "domain", Name: "smoke.test"}, +} + +// The failure this exists to prevent: after a real migration the admin +// account authenticated fine and was refused every management call, because +// migrate_v016.py had given it the User role like everyone else. +func TestAccountRolesRestoresTheAdministrator(t *testing.T) { + ops, covered, warnings, err := AccountRoleOperations(migratedPrincipals) + if err != nil { + t.Fatal(err) + } + if len(warnings) != 0 { + t.Errorf("unexpected warnings: %v", warnings) + } + if len(ops) != 1 { + t.Fatalf("generated %d op(s), want 1 - only the admin's role actually changes", len(ops)) + } + v := ops[0].Value["role-sysadmin"] + if v == nil { + t.Fatalf("no operation for sysadmin: %+v", ops[0].Value) + } + // Account is multi-variant; without its own @type the upsert is + // rejected outright by the server. + if v["@type"] != "User" { + t.Errorf("@type = %v, want User (the Account variant)", v["@type"]) + } + roles, ok := v["roles"].(map[string]any) + if !ok || roles["@type"] != "Admin" { + t.Errorf("roles = %v, want {\"@type\": \"Admin\"}", v["roles"]) + } + if len(covered) != 1 { + t.Errorf("covered = %v, want one entry", covered) + } +} + +// Ordinary users already get User from the migration. Rewriting every +// account would be a far larger blast radius for no benefit. +func TestAccountRolesLeavesOrdinaryUsersAlone(t *testing.T) { + ops, _, _, err := AccountRoleOperations([]backup.Principal{ + {Type: "individual", Name: "alice", Roles: []string{"user"}}, + {Type: "individual", Name: "bob", Roles: []string{}}, + }) + if err != nil { + t.Fatal(err) + } + if len(ops) != 0 { + t.Errorf("generated %d op(s) for plain users, want 0", len(ops)) + } +} + +func TestAccountRolesSkipsNonIndividuals(t *testing.T) { + ops, _, _, err := AccountRoleOperations([]backup.Principal{ + {Type: "domain", Name: "smoke.test", Roles: []string{"admin"}}, + {Type: "group", Name: "staff", Roles: []string{"admin"}}, + }) + if err != nil { + t.Fatal(err) + } + if len(ops) != 0 { + t.Errorf("generated %d op(s) for non-individual principals, want 0", len(ops)) + } +} + +// v0.15 carries a list, v0.16 one variant. Under-privileging an +// administrator locks them out, so admin wins - and the collapse is +// reported, not silent. +func TestAccountRolesCollapsesMultipleRolesToAdminAndSaysSo(t *testing.T) { + ops, _, warnings, err := AccountRoleOperations([]backup.Principal{ + {Type: "individual", Name: "boss", Roles: []string{"user", "admin"}}, + }) + if err != nil { + t.Fatal(err) + } + if len(ops) != 1 { + t.Fatalf("generated %d op(s), want 1", len(ops)) + } + roles := ops[0].Value["role-boss"]["roles"].(map[string]any) + if roles["@type"] != "Admin" { + t.Errorf("roles = %v, want Admin to win", roles) + } + if len(warnings) != 1 || !strings.Contains(warnings[0], "collapsed") { + t.Errorf("warnings = %v, want the collapse reported", warnings) + } +} + +func TestAccountRolesReportsRolesItCannotMap(t *testing.T) { + _, _, warnings, err := AccountRoleOperations([]backup.Principal{ + {Type: "individual", Name: "auditor", Roles: []string{"compliance-reviewer"}}, + }) + if err != nil { + t.Fatal(err) + } + if len(warnings) != 1 || !strings.Contains(warnings[0], "compliance-reviewer") { + t.Errorf("warnings = %v, want the unmappable role named", warnings) + } +} diff --git a/internal/backup/settingsdump.go b/internal/backup/settingsdump.go index 079a7e3..dabe0ef 100644 --- a/internal/backup/settingsdump.go +++ b/internal/backup/settingsdump.go @@ -312,3 +312,28 @@ func ReadUnmigratedKeys(reportPath string, settings map[string]string) (map[stri } return keys, nil } + +// Principal is the subset of a v0.15 principals dump this tool needs. The +// shape is what the REST management API returns and what +// migrate_v016.py's dump step writes out verbatim. +type Principal struct { + ID int `json:"id"` + Type string `json:"type"` // "individual", "group", "domain", ... + Name string `json:"name"` + Emails []string `json:"emails"` + Roles []string `json:"roles"` +} + +// ReadPrincipalsDump loads the principals dump written alongside the +// settings dump. +func ReadPrincipalsDump(path string) ([]Principal, error) { + data, err := os.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("backup: read principals dump %s: %w", path, err) + } + var principals []Principal + if err := json.Unmarshal(data, &principals); err != nil { + return nil, fmt.Errorf("backup: parse principals dump %s: %w", path, err) + } + return principals, nil +}