diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index d51ed9c..79052bb 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -672,12 +672,39 @@ happens to need them. `preflight.DeploymentKind` is a type alias for corpus has been through the converter. Everything in §4.9's rewrite and most of §8's newer entries came from that, not from reading code. - Still unproven against real software: **cutover** (never executed - its - unit rewrite, service control and quota recalculation are tested only - against fakes), the **`x:Task` quota wire format**, **systemd drop-in** - handling, and anything on a **non-RocksDB backend** or a **Docker** - deployment. Cutover is the gap that matters most, since it is the phase - that mutates production. + **Cutover has now run**, in a complete 0.15.5 -> 0.16.14 migration of the + smoke VM: binary verified and installed, the unit rewritten with its + hardening intact, service restarted, health check passed, and checkpoint + resume exercised. All mail survived and was readable afterwards. Three + defects came out of it and are fixed: + + - The converted config was installed root-owned while the service runs as + its own user, so it crash-looped 28 times on "Permission denied". The + ownership trap that retired the rollback implementation (§4.8), in a + new place. Cutover now installs the config itself, copying ownership + and mode from the config being replaced. + - **v0.16.14 does not serve `/api`.** Confirmed against a fully migrated, + fully configured instance, not just a sandbox: `/api`, `/api/principal` + and `/jmap/` all 404, and the JMAP endpoint is the one the session + document advertises. The client now discovers it, re-basing the + advertised path onto the operator's own host - a real instance + advertises a canonical public URL that frequently isn't reachable from + where this tool runs. + - Dispatching on the `urn:stalwart:jmap` capability was wrong, because + *neither* version advertises it. The client probes what the instance + actually serves instead. + + Still unproven: the **`x:Task` quota wire format** (the endpoint fix makes + 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. - **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/cutover/cutover.go b/internal/cutover/cutover.go index 43e5ec8..ca85681 100644 --- a/internal/cutover/cutover.go +++ b/internal/cutover/cutover.go @@ -13,6 +13,7 @@ import ( "os" "path/filepath" "strings" + "syscall" "time" "github.com/LINUXexpert-org/stalwart-migrator/internal/checkpoint" @@ -46,6 +47,16 @@ type Options struct { // becomes the unit's --config argument. ServiceUnitPath string ConfigPath string + // ConfigSource, if set, is installed to ConfigPath before the unit is + // repointed at it - the converted v0.16 config the migration produced. + // Its ownership and mode are copied from ConfigOwnerReference (the old + // config, normally), because the service does not run as root and a + // root-owned config it cannot read fails the service at startup, not at + // install time. That is not hypothetical: a full migration crash-looped + // 28 times on "Failed to read data store settings: Permission denied" + // for exactly this reason. + ConfigSource string + ConfigOwnerReference string // RecoveryPointConfirmed is the operator asserting that a recovery // point exists for this machine. This tool does not take one, verify @@ -88,6 +99,7 @@ type Plan struct { BinaryPath string ServiceUnitPath string ConfigPath string + ConfigSource string RecalculateQuotas bool } @@ -95,7 +107,10 @@ func (p Plan) String() string { var b strings.Builder fmt.Fprintf(&b, "cutover plan for run %s:\n", p.RunID) fmt.Fprintf(&b, " 1. confirm %s really is %s, then install it as %s\n", p.StagedBinaryPath, p.TargetVersion, p.BinaryPath) - fmt.Fprintf(&b, " 2. preserve %s, then point its ExecStart at the new binary and strip any recovery-mode env vars\n", p.ServiceUnitPath) + if p.ConfigSource != "" { + fmt.Fprintf(&b, " 2. install %s as %s, owned so the service user can read it\n", p.ConfigSource, p.ConfigPath) + } + fmt.Fprintf(&b, " 3. preserve %s, then point its ExecStart at the new binary and strip any recovery-mode env vars\n", p.ServiceUnitPath) fmt.Fprintf(&b, " 3. reload the service definition and start %s\n", p.Target) fmt.Fprint(&b, " 4. wait for it to answer an authenticated JMAP session request\n") if p.RecalculateQuotas { @@ -116,6 +131,7 @@ func BuildPlan(rs *checkpoint.RunState, opts Options) (Plan, error) { RunID: rs.RunID, TargetVersion: rs.TargetVersion, StagedBinaryPath: opts.StagedBinaryPath, BinaryPath: opts.BinaryPath, ServiceUnitPath: opts.ServiceUnitPath, ConfigPath: opts.ConfigPath, + ConfigSource: opts.ConfigSource, RecalculateQuotas: opts.RecalculateQuotas, } @@ -239,6 +255,22 @@ func Run(ctx context.Context, store *checkpoint.Store, rs *checkpoint.RunState, return report, err } + if err := step("install-config", func() (checkpoint.StepOutcome, error) { + if plan.ConfigSource == "" { + return checkpoint.StepOutcome{ + Verdict: string(StatusSkipped), + Detail: "no converted config to install - the unit is repointed at whatever is already at ConfigPath", + }, nil + } + owner, err := installConfig(plan.ConfigSource, plan.ConfigPath, opts.ConfigOwnerReference) + if err != nil { + return checkpoint.StepOutcome{}, err + } + return checkpoint.StepOutcome{Detail: fmt.Sprintf("installed %s as %s (%s)", plan.ConfigSource, plan.ConfigPath, owner)}, nil + }); err != nil { + return report, err + } + if err := step("update-service-definition", func() (checkpoint.StepOutcome, error) { preserved, err := preserveUnit(plan.ServiceUnitPath, rs.RunID) if err != nil { @@ -498,3 +530,44 @@ func hashFile(path string) (sha256Hex string, size int64, err error) { } return hex.EncodeToString(h.Sum(nil)), n, nil } + +// installConfig places the converted config at dst, copying ownership and +// mode from reference (normally the config being replaced) so the service +// user can still read it. +// +// Ownership is the whole point of this function. Writing a config as root +// is the natural thing for a tool running as root to do, and it produces a +// service that starts, fails to read its own config, and restarts forever - +// a failure that shows up minutes later in the journal rather than at the +// moment of the mistake. Where no reference is available the file is left +// world-readable, since a config the service cannot read is worse than one +// other local users can. +func installConfig(src, dst, reference string) (ownership string, err error) { + data, err := os.ReadFile(src) + if err != nil { + return "", fmt.Errorf("cutover: read converted config %s: %w", src, err) + } + + perm := os.FileMode(0o644) + uid, gid := -1, -1 + if reference == "" { + reference = dst // fall back to whatever is already in place + } + if info, statErr := os.Stat(reference); statErr == nil { + perm = info.Mode().Perm() + if sys, ok := info.Sys().(*syscall.Stat_t); ok { + uid, gid = int(sys.Uid), int(sys.Gid) + } + } + + if err := writeFileAtomic(dst, data, perm); err != nil { + return "", err + } + if uid >= 0 && gid >= 0 { + if err := os.Chown(dst, uid, gid); err != nil { + return "", fmt.Errorf("cutover: set ownership on %s to %d:%d - the service runs as that user and cannot read a config it does not own: %w", dst, uid, gid, err) + } + return fmt.Sprintf("uid %d, gid %d, mode %v", uid, gid, perm), nil + } + return fmt.Sprintf("mode %v, ownership unchanged (no reference file to copy it from)", perm), nil +} diff --git a/internal/cutover/cutover_test.go b/internal/cutover/cutover_test.go index 2ec2bea..e85fc74 100644 --- a/internal/cutover/cutover_test.go +++ b/internal/cutover/cutover_test.go @@ -388,3 +388,63 @@ func TestRunSchedulesOneQuotaTaskPerAccountAndWaits(t *testing.T) { } } } + +// A full migration crash-looped 28 times on "Failed to read data store +// settings: Permission denied" because the converted config was written as +// root while the service runs as its own user. The failure surfaced minutes +// later in the journal, not at the moment of the mistake, which is what +// makes it worth a test rather than care. +func TestRunInstallsTheConfigWithOwnershipTheServiceCanRead(t *testing.T) { + store, rs, opts := migratedRun(t) + dir := t.TempDir() + + // The config being replaced, standing in for the one the old version + // ran with - restrictive mode, so a naive copy would lock the service + // out of its own config. + oldConfig := filepath.Join(dir, "config.toml") + if err := os.WriteFile(oldConfig, []byte("[server]\n"), 0o640); err != nil { + t.Fatal(err) + } + converted := filepath.Join(dir, "converted.json") + if err := os.WriteFile(converted, []byte(`{"@type":"RocksDb","path":"/opt/stalwart/data"}`), 0o600); err != nil { + t.Fatal(err) + } + installed := filepath.Join(dir, "config.json") + + opts.ConfigSource = converted + opts.ConfigPath = installed + opts.ConfigOwnerReference = oldConfig + + report, err := Run(context.Background(), store, rs, opts) + if err != nil { + t.Fatalf("Run: %v\n%s", err, report) + } + if got := readFile(t, installed); !strings.Contains(got, "RocksDb") { + t.Errorf("installed config = %q, want the converted one", got) + } + info, err := os.Stat(installed) + if err != nil { + t.Fatal(err) + } + // Mode comes from the file being replaced, not from the source's 0600. + if info.Mode().Perm() != 0o640 { + t.Errorf("installed config mode = %v, want 0640 copied from the old config", info.Mode().Perm()) + } + // The unit must point at the installed config, not the scratch copy. + if unit := readFile(t, opts.ServiceUnitPath); !strings.Contains(unit, installed) { + t.Errorf("unit does not reference the installed config:\n%s", unit) + } +} + +func TestRunSkipsConfigInstallWhenThereIsNothingToInstall(t *testing.T) { + store, rs, opts := migratedRun(t) + report, err := Run(context.Background(), store, rs, opts) + if err != nil { + t.Fatal(err) + } + for _, res := range report.Results { + if res.Name == "install-config" && res.Status != StatusSkipped { + t.Errorf("install-config = %s, want skip when no ConfigSource is given", res.Status) + } + } +} diff --git a/internal/preflight/checks_test.go b/internal/preflight/checks_test.go index 56257fb..b73764a 100644 --- a/internal/preflight/checks_test.go +++ b/internal/preflight/checks_test.go @@ -222,6 +222,10 @@ func TestCheckerRunCapturesAccountSnapshotWhenAdminURLSet(t *testing.T) { // x:Account/get, and Mailbox/get. var apiURL string adminSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/principal" { + w.WriteHeader(http.StatusNotFound) // v0.16 shape: no REST management API + return + } switch { case r.Method == http.MethodGet && r.URL.Path == "/.well-known/jmap": user, _, _ := r.BasicAuth() diff --git a/internal/stalwartapi/client.go b/internal/stalwartapi/client.go index f3b15bd..6541181 100644 --- a/internal/stalwartapi/client.go +++ b/internal/stalwartapi/client.go @@ -7,7 +7,9 @@ import ( "context" "fmt" "net/http" + "net/url" "strings" + "sync" "time" ) @@ -24,6 +26,66 @@ type Client struct { Username string Password string HTTPClient *http.Client + + mu sync.Mutex + endpoint string // resolved JMAP endpoint; see apiEndpoint +} + +// apiEndpoint returns the URL to POST JMAP method calls to, discovered from +// the instance's own session document rather than assumed. +// +// This used to be hardcoded as BaseURL + "/api", which is wrong for the +// version this tool migrates *to*: a fully configured, serving 0.16.14 +// returns 404 for /api, and advertises its JMAP endpoint through the +// session document's apiUrl instead (RFC 8620 §2 - discovery is how a +// client is *supposed* to find it). +// +// The path is taken from apiUrl but re-based onto BaseURL's scheme and +// host. A real instance advertises its canonical public URL - observed: +// "https://mail.smoke.test/jmap/" - which frequently isn't reachable from +// where this tool runs, over a hostname that may not resolve or a +// certificate that may not validate. The operator told us how to reach +// this server when they passed --admin-url; the session is only authoritative +// about *where on it* the API lives. +func (c *Client) apiEndpoint(ctx context.Context) (string, error) { + c.mu.Lock() + cached := c.endpoint + c.mu.Unlock() + if cached != "" { + return cached, nil + } + + session, err := c.fetchSession(ctx, c.Username, c.Password) + if err != nil { + return "", fmt.Errorf("stalwartapi: discover the JMAP endpoint: %w", err) + } + resolved, err := c.rebaseOntoBaseURL(session.APIURL) + if err != nil { + return "", err + } + + c.mu.Lock() + c.endpoint = resolved + c.mu.Unlock() + return resolved, nil +} + +// rebaseOntoBaseURL keeps the advertised path but the operator's host. +func (c *Client) rebaseOntoBaseURL(apiURL string) (string, error) { + base, err := url.Parse(strings.TrimRight(c.BaseURL, "/")) + if err != nil { + return "", fmt.Errorf("stalwartapi: parse base URL %q: %w", c.BaseURL, err) + } + if apiURL == "" { + return "", fmt.Errorf("stalwartapi: the instance's session document advertises no apiUrl, so there is no JMAP endpoint to call") + } + advertised, err := url.Parse(apiURL) + if err != nil { + return "", fmt.Errorf("stalwartapi: parse advertised apiUrl %q: %w", apiURL, err) + } + base.Path = advertised.Path + base.RawQuery = advertised.RawQuery + return base.String(), nil } func (c *Client) httpClient() *http.Client { diff --git a/internal/stalwartapi/management.go b/internal/stalwartapi/management.go index 82af84e..bd9b998 100644 --- a/internal/stalwartapi/management.go +++ b/internal/stalwartapi/management.go @@ -39,12 +39,16 @@ type methodResponse struct { CallID string } -// call POSTs one JMAP-style request to the management API (Stalwart's /api -// endpoint - see docs/ref/object/account.md, which is distinct from /jmap) -// using this Client's own credentials, and returns its parsed method -// responses in order. +// call POSTs one JMAP-style request to the management API using this +// Client's own credentials, and returns its parsed method responses in +// order. The endpoint is discovered from the instance's session document - +// see apiEndpoint for why it is not simply BaseURL + "/api". func (c *Client) call(ctx context.Context, using []string, methodCalls []any) ([]methodResponse, error) { - return c.callAs(ctx, c.Username, c.Password, strings.TrimRight(c.BaseURL, "/")+"/api", using, methodCalls) + endpoint, err := c.apiEndpoint(ctx) + if err != nil { + return nil, err + } + return c.callAs(ctx, c.Username, c.Password, endpoint, using, methodCalls) } // callAs is call's underlying primitive: it accepts an explicit @@ -128,14 +132,12 @@ type account struct { // exactly what's missing rather than silently treating an unreachable // account's mailboxes as having zero messages. func (c *Client) AccountSnapshot(ctx context.Context) (*Snapshot, error) { - isJMAP, err := c.hasJMAPManagement(ctx) + isREST, err := c.hasRESTManagement(ctx) if err != nil { return nil, fmt.Errorf("stalwartapi: discover which management API this instance speaks: %w", err) } - if !isJMAP { - // No urn:stalwart:jmap: this is a 0.15.x instance, whose - // management API is REST. Confirmed against a live 0.15.5 server - - // see principal.go. + if isREST { + // v0.15.x: REST at /api/principal. See principal.go. return c.principalSnapshotREST(ctx) } return c.accountSnapshotJMAP(ctx) @@ -205,13 +207,43 @@ func (c *Client) accountSnapshotJMAP(ctx context.Context) (*Snapshot, error) { }, nil } +// describeJMAPError turns a JMAP method-level error into something an +// operator can act on. +// +// "forbidden" gets special handling because of where it shows up: against a +// freshly migrated instance, an account that held the admin role before the +// migration was refused x:Account/query afterwards. Whether the role failed +// to carry over or v0.16 requires different permissions was not isolated, +// but the operator's situation is the same either way - they have an admin +// account that can no longer administer - and a bare "forbidden" gives them +// nothing to go on. +func describeJMAPError(method string, args json.RawMessage) error { + var parsed struct { + Type string `json:"type"` + Description string `json:"description"` + } + if err := json.Unmarshal(args, &parsed); err != nil || parsed.Type == "" { + return fmt.Errorf("stalwartapi: %s error: %s", method, args) + } + if parsed.Type == "forbidden" { + return fmt.Errorf("stalwartapi: %s was refused (forbidden): %s - this account is authenticated but not "+ + "permitted to perform management operations. After a v0.16 migration this is worth checking first: an "+ + "account that held the admin role beforehand may not have it afterwards", + method, parsed.Description) + } + if parsed.Description != "" { + return fmt.Errorf("stalwartapi: %s error (%s): %s", method, parsed.Type, parsed.Description) + } + return fmt.Errorf("stalwartapi: %s error: %s", method, parsed.Type) +} + func accountQueryIDs(responses []methodResponse) ([]string, error) { if len(responses) == 0 { return nil, fmt.Errorf("stalwartapi: Account/query returned no method responses") } r := responses[0] if r.Name == "error" { - return nil, fmt.Errorf("stalwartapi: Account/query error: %s", r.Args) + return nil, describeJMAPError("Account/query", r.Args) } var result struct { IDs []string `json:"ids"` @@ -228,7 +260,7 @@ func accountGetList(responses []methodResponse) ([]account, error) { } r := responses[0] if r.Name == "error" { - return nil, fmt.Errorf("stalwartapi: Account/get error: %s", r.Args) + return nil, describeJMAPError("Account/get", r.Args) } var result struct { List []account `json:"list"` diff --git a/internal/stalwartapi/management_test.go b/internal/stalwartapi/management_test.go index 085d99b..cfd7d0a 100644 --- a/internal/stalwartapi/management_test.go +++ b/internal/stalwartapi/management_test.go @@ -46,6 +46,10 @@ func accountManagementAndMailboxServer(t *testing.T, mailboxesFor map[string][]m var apiURL string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/principal" { + w.WriteHeader(http.StatusNotFound) // v0.16 shape: no REST management API + return + } gotPaths = append(gotPaths, r.URL.Path) if r.Method == http.MethodGet && r.URL.Path == "/.well-known/jmap" { @@ -182,6 +186,10 @@ func TestAccountSnapshotEmptyInstance(t *testing.T) { func TestAccountSnapshotPropagatesJMAPError(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) // v0.16 shape: no REST management API + return + } if serveJMAPSession(w, r, "/api") { return } diff --git a/internal/stalwartapi/principal.go b/internal/stalwartapi/principal.go index d1931f3..0d79209 100644 --- a/internal/stalwartapi/principal.go +++ b/internal/stalwartapi/principal.go @@ -51,15 +51,22 @@ const restPrincipalPageSize = 100 // call. const stalwartManagementCapability = "urn:stalwart:jmap" -// hasJMAPManagement reports whether this instance speaks the 0.16+ JMAP -// management API, by reading the capability list from its session -// document. It deliberately does its own request rather than reusing -// fetchSession: that helper also requires an apiUrl and a mail account, -// which are needed for impersonated mailbox reads but have nothing to do -// with which management API to use - failing dispatch over a missing -// apiUrl would misroute an instance that is perfectly readable. -func (c *Client) hasJMAPManagement(ctx context.Context) (bool, error) { - endpoint := strings.TrimRight(c.BaseURL, "/") + "/.well-known/jmap" +// hasRESTManagement reports whether this instance serves the v0.15.x REST +// management API, by asking it for a single principal. +// +// This replaced a capability check, which cannot work: *neither* version +// advertises urn:stalwart:jmap. A real 0.15.5 doesn't, and a fully +// migrated, fully configured 0.16.14 doesn't either - verified against +// both. Dispatching on the capability sent 0.16 instances down the 0.15 +// REST path, where every call 404s. +// +// So the client asks what the instance actually serves instead. 0.15.x +// answers GET /api/principal with a principal list; 0.16.14 returns 404 +// for that path and serves JMAP management objects at the endpoint its +// session document advertises. A cheap probe is less elegant than a +// declared capability and has the considerable advantage of being true. +func (c *Client) hasRESTManagement(ctx context.Context) (bool, error) { + endpoint := strings.TrimRight(c.BaseURL, "/") + "/api/principal?limit=1" req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) if err != nil { return false, err @@ -67,20 +74,24 @@ func (c *Client) hasJMAPManagement(ctx context.Context) (bool, error) { req.SetBasicAuth(c.Username, c.Password) resp, err := c.httpClient().Do(req) if err != nil { - return false, fmt.Errorf("stalwartapi: reach %s: %w", endpoint, err) + return false, fmt.Errorf("stalwartapi: probe %s: %w", endpoint, err) } defer resp.Body.Close() - if resp.StatusCode != http.StatusOK { - return false, fmt.Errorf("stalwartapi: session discovery at %s returned %s", endpoint, resp.Status) + io.Copy(io.Discard, io.LimitReader(resp.Body, 1<<16)) + + switch resp.StatusCode { + case http.StatusOK: + return true, nil + case http.StatusNotFound: + return false, nil + case http.StatusUnauthorized, http.StatusForbidden: + // The path exists but these credentials can't use it. Say so here + // rather than falling through to the other API and reporting a + // confusing error from there instead. + return false, fmt.Errorf("stalwartapi: %s returned %s - the credentials are not accepted for management operations", endpoint, resp.Status) + default: + return false, nil } - var session struct { - Capabilities map[string]json.RawMessage `json:"capabilities"` - } - if err := json.NewDecoder(resp.Body).Decode(&session); err != nil { - return false, fmt.Errorf("stalwartapi: parse session document from %s: %w", endpoint, err) - } - _, ok := session.Capabilities[stalwartManagementCapability] - return ok, nil } type restPrincipal struct { diff --git a/internal/stalwartapi/principal_test.go b/internal/stalwartapi/principal_test.go index 956eb7e..b35c491 100644 --- a/internal/stalwartapi/principal_test.go +++ b/internal/stalwartapi/principal_test.go @@ -181,3 +181,98 @@ func TestJMAPSnapshotCapturesUsedDiskQuota(t *testing.T) { t.Errorf("UsedQuota = %v, want an entry per account", snap.UsedQuota) } } + +// A fully configured, serving Stalwart 0.16.14 returns 404 for /api - the +// endpoint this client used to assume. It advertises its JMAP endpoint in +// the session document instead. This test pins the discovery so the +// assumption can't creep back. +func TestCallDiscoversTheEndpointRatherThanAssumingSlashApi(t *testing.T) { + var posted []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet && r.URL.Path == "/.well-known/jmap" { + json.NewEncoder(w).Encode(map[string]any{ + "apiUrl": "/jmap/", + "capabilities": map[string]any{"urn:stalwart:jmap": map[string]any{}}, + }) + return + } + posted = append(posted, r.URL.Path) + if r.URL.Path != "/jmap/" { + // Stand in for 0.16.14, which 404s anything else. + w.WriteHeader(http.StatusNotFound) + return + } + json.NewEncoder(w).Encode(jmapEnvelope{MethodResponses: []any{ + []any{"x:Account/query", map[string]any{"ids": []string{}}, "q"}, + }}) + })) + defer srv.Close() + + client := &Client{BaseURL: srv.URL, Username: "admin", Password: "x"} + if _, err := client.AccountIDs(context.Background()); err != nil { + t.Fatalf("AccountIDs against an instance that only serves /jmap/: %v", err) + } + for _, p := range posted { + if p == "/api" { + t.Error("posted to /api, which 0.16.14 does not serve") + } + } + if len(posted) == 0 || posted[0] != "/jmap/" { + t.Errorf("posted to %v, want the advertised /jmap/", posted) + } +} + +// A real instance advertises its canonical public URL, which routinely +// isn't reachable from where this tool runs - a hostname that may not +// resolve, over TLS that may not validate. Observed on a migrated +// instance: "https://mail.smoke.test/jmap/" while the operator reached it +// as http://127.0.0.1:8090. The path is the session's to dictate; the host +// is the operator's. +func TestEndpointKeepsTheOperatorsHostAndTheSessionsPath(t *testing.T) { + client := &Client{BaseURL: "http://127.0.0.1:8090"} + got, err := client.rebaseOntoBaseURL("https://mail.smoke.test/jmap/") + if err != nil { + t.Fatal(err) + } + if got != "http://127.0.0.1:8090/jmap/" { + t.Errorf("endpoint = %q, want the advertised path on the operator's host", got) + } +} + +func TestEndpointRefusesASessionWithNoAPIURL(t *testing.T) { + client := &Client{BaseURL: "http://127.0.0.1:8090"} + if _, err := client.rebaseOntoBaseURL(""); err == nil { + t.Fatal("want an error when the instance advertises no apiUrl") + } +} + +// Observed on a freshly migrated 0.16.14: an account that held the admin +// role before the migration was refused x:Account/query afterwards. A bare +// "forbidden" leaves an operator with nowhere to start. +func TestForbiddenExplainsThePostMigrationPermissionTrap(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) // v0.16 shape: no REST management API + return + } + if r.Method == http.MethodGet && r.URL.Path == "/.well-known/jmap" { + json.NewEncoder(w).Encode(map[string]any{"apiUrl": "/jmap/"}) + return + } + json.NewEncoder(w).Encode(jmapEnvelope{MethodResponses: []any{ + []any{"error", map[string]any{"type": "forbidden", "description": "You are not authorized to perform this action"}, "q"}, + }}) + })) + defer srv.Close() + + client := &Client{BaseURL: srv.URL, Username: "sysadmin", Password: "x"} + _, err := client.AccountSnapshot(context.Background()) + if err == nil { + t.Fatal("want an error for a forbidden management call") + } + for _, want := range []string{"forbidden", "admin role", "not permitted"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q should mention %q", err, want) + } + } +} diff --git a/internal/stalwartapi/task.go b/internal/stalwartapi/task.go index 3d7df2b..0e428fd 100644 --- a/internal/stalwartapi/task.go +++ b/internal/stalwartapi/task.go @@ -132,7 +132,7 @@ func (c *Client) setTasks(ctx context.Context, create map[string]any, subjectFor } r := responses[0] if r.Name == "error" { - return nil, fmt.Errorf("stalwartapi: Task/set error: %s", r.Args) + return nil, describeJMAPError("Task/set", r.Args) } var result struct { Created map[string]struct { @@ -245,7 +245,7 @@ func (c *Client) taskStatuses(ctx context.Context, ids []string) (map[string]tas } r := responses[0] if r.Name == "error" { - return nil, fmt.Errorf("stalwartapi: Task/get error: %s", r.Args) + return nil, describeJMAPError("Task/get", r.Args) } var result struct { List []struct { diff --git a/internal/stalwartapi/task_test.go b/internal/stalwartapi/task_test.go index 5e748f7..dbb77a9 100644 --- a/internal/stalwartapi/task_test.go +++ b/internal/stalwartapi/task_test.go @@ -28,6 +28,18 @@ func newTaskServer(t *testing.T) (*taskServer, *httptest.Server) { t.Helper() ts := &taskServer{queue: map[string]string{}} srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/principal" { + w.WriteHeader(http.StatusNotFound) // v0.16 shape: no REST management API + return + } + if r.Method == http.MethodGet && r.URL.Path == "/.well-known/jmap" { + // The endpoint is discovered, not assumed - see apiEndpoint. + json.NewEncoder(w).Encode(map[string]any{ + "apiUrl": "/api", + "capabilities": map[string]any{"urn:stalwart:jmap": map[string]any{}}, + }) + return + } var body map[string]any json.NewDecoder(r.Body).Decode(&body) call := body["methodCalls"].([]any)[0].([]any) diff --git a/internal/validate/content_integrity_test.go b/internal/validate/content_integrity_test.go index 7039f4a..dda5e25 100644 --- a/internal/validate/content_integrity_test.go +++ b/internal/validate/content_integrity_test.go @@ -27,6 +27,10 @@ func fakeManagementServer(t *testing.T, accounts []map[string]any, mailboxesByEm t.Helper() var apiURL string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/principal" { + w.WriteHeader(http.StatusNotFound) // v0.16 shape: no REST management API + return + } if r.Method == http.MethodGet && r.URL.Path == "/.well-known/jmap" { user, _, _ := r.BasicAuth() if !strings.Contains(user, "%") { diff --git a/internal/validate/main_test.go b/internal/validate/main_test.go index 540de91..b7b6412 100644 --- a/internal/validate/main_test.go +++ b/internal/validate/main_test.go @@ -50,6 +50,10 @@ func runFakeStalwartServer() { os.Exit(1) } srv := &http.Server{Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/principal" { + w.WriteHeader(http.StatusNotFound) // v0.16 shape: no REST management API + return + } switch { case r.Method == http.MethodGet && r.URL.Path == "/.well-known/jmap": user, _, _ := r.BasicAuth()