diff --git a/migrations/0014_mailbox_state_single_source.down.sql b/migrations/0014_mailbox_state_single_source.down.sql new file mode 100644 index 0000000..9dfb621 --- /dev/null +++ b/migrations/0014_mailbox_state_single_source.down.sql @@ -0,0 +1,4 @@ +-- The reconciliation corrected cached numbers and cannot be undone (and has no +-- reason to be); only the function is removed. Folders it added stay: they are +-- ordinary folders by now and may hold mail. +DROP FUNCTION IF EXISTS provision_default_folders(UUID); diff --git a/migrations/0014_mailbox_state_single_source.up.sql b/migrations/0014_mailbox_state_single_source.up.sql new file mode 100644 index 0000000..98d52d4 --- /dev/null +++ b/migrations/0014_mailbox_state_single_source.up.sql @@ -0,0 +1,77 @@ +-- One definition of "a mailbox's standard folders", and a reconciliation of the +-- cached numbers that two code paths used to keep on their own. +-- +-- 1. provision_default_folders +-- +-- The standard folders were created in two places that had already diverged: +-- the console's createMailbox added Reports, the CLI that creates the first +-- administrator did not, so that account had no folder for DMARC and TLS-RPT +-- reports and they fell into its INBOX. Both now call this function, so the +-- list lives here and nowhere else. It is idempotent: it adds only what is +-- missing, matching by name without regard to case. +CREATE OR REPLACE FUNCTION provision_default_folders(p_mailbox_id UUID) +RETURNS VOID +LANGUAGE sql +AS $$ + INSERT INTO folders (mailbox_id, name, special_use) + SELECT p_mailbox_id, f.name, f.special_use + FROM (VALUES ('INBOX', 'inbox'), + ('Sent', 'sent'), + ('Drafts', 'drafts'), + ('Trash', 'trash'), + ('Junk', 'junk'), + ('Archive', 'archive'), + -- Where delivered DMARC and TLS-RPT reports are filed. IMAP has no + -- special-use role for these, so it is matched by name. + ('Reports', NULL)) AS f(name, special_use) + WHERE NOT EXISTS ( + SELECT 1 FROM folders x + WHERE x.mailbox_id = p_mailbox_id + AND x.parent_id IS NULL + AND LOWER(x.name) = LOWER(f.name)); +$$; + +-- Mailboxes created before the function that lack a standard folder get it now. +SELECT provision_default_folders(id) FROM mailboxes; + +-- 2. Reconcile the cached numbers with the rows. +-- +-- Folder counters, and the mailbox's used bytes, are denormalised caches. IMAP +-- and the webmail each maintained them and each got something wrong (IMAP never +-- moved the unread counter; the webmail never gave the quota back), so existing +-- mailboxes carry the drift. Every path now goes through one MailStore that +-- keeps them true; this corrects what is already stored, once. It only touches +-- rows that are actually wrong, so it is safe to run again. +UPDATE folders f + SET unread_count = c.unread, + total_count = c.total + FROM ( + SELECT f2.id, + COUNT(e.id) FILTER ( + WHERE NOT EXISTS ( + SELECT 1 FROM message_flags mf + WHERE mf.message_id = e.id + AND mf.received_at = e.received_at + AND mf.flag = '\Seen') + )::int AS unread, + COUNT(e.id)::int AS total + FROM folders f2 + LEFT JOIN email_messages e + ON e.folder_id = f2.id AND e.expunged_at IS NULL + GROUP BY f2.id + ) AS c + WHERE f.id = c.id + AND (f.unread_count IS DISTINCT FROM c.unread + OR f.total_count IS DISTINCT FROM c.total); + +UPDATE mailboxes m + SET used_bytes = COALESCE(s.total, 0) + FROM mailboxes m2 + LEFT JOIN ( + SELECT mailbox_id, SUM(size_bytes)::bigint AS total + FROM email_messages + WHERE expunged_at IS NULL + GROUP BY mailbox_id + ) AS s ON s.mailbox_id = m2.id + WHERE m.id = m2.id + AND m.used_bytes IS DISTINCT FROM COALESCE(s.total, 0); diff --git a/services/auth/src/cli.test.ts b/services/auth/src/cli.test.ts new file mode 100644 index 0000000..f538aa6 --- /dev/null +++ b/services/auth/src/cli.test.ts @@ -0,0 +1,46 @@ +import { describe, it, expect, afterAll } from "vitest"; + +process.env.LAMBDAMAIL_MASTER_KEY ||= "test-master-key-at-least-16-chars"; +process.env.DATABASE_URL = process.env.TEST_DATABASE_URL ?? ""; + +const hasDatabase = Boolean(process.env.TEST_DATABASE_URL); +const describeDb = hasDatabase ? describe : describe.skip; + +const { createAdmin, CliError } = await import("./cli.js"); +const { query, closePool } = await import("./db.js"); + +const DOMAIN = `cli-${Date.now()}.test`; +const EMAIL = `root@${DOMAIN}`; + +describeDb("create-admin (command line)", () => { + afterAll(async () => { + await query(`DELETE FROM domains WHERE name = $1`, [DOMAIN]); + await closePool(); + }); + + it("gives the first administrator every standard folder, Reports included", async () => { + const log = console.log; + console.log = () => undefined; + try { + await createAdmin(EMAIL, "a-long-enough-password", true); + } finally { + console.log = log; + } + + // This account used to come out without Reports, so its DMARC and TLS-RPT + // reports fell into the INBOX: the console and this command each had their + // own list of folders and the lists had drifted. + const folders = await query<{ name: string; special_use: string | null }>( + `SELECT f.name, f.special_use FROM folders f JOIN mailboxes m ON m.id = f.mailbox_id WHERE m.email_address = $1`, + [EMAIL], + ); + expect(folders.map((f) => f.name).sort()).toEqual(["Archive", "Drafts", "INBOX", "Junk", "Reports", "Sent", "Trash"]); + expect(folders.find((f) => f.name === "Reports")?.special_use).toBeNull(); + expect(folders.find((f) => f.name === "Sent")?.special_use).toBe("sent"); + }); + + it("refuses with a readable error instead of exiting the process", async () => { + await expect(createAdmin(EMAIL, "short", true)).rejects.toBeInstanceOf(CliError); + await expect(createAdmin(EMAIL, "a-long-enough-password", true)).rejects.toThrow(/already exists/); + }); +}); diff --git a/services/auth/src/cli.ts b/services/auth/src/cli.ts index 57fce04..7f3351c 100644 --- a/services/auth/src/cli.ts +++ b/services/auth/src/cli.ts @@ -13,22 +13,19 @@ * node dist/cli.js reset-mfa */ +import { pathToFileURL } from "node:url"; import { query, queryOne, closePool } from "./db.js"; import { hashPassword } from "./crypto.js"; const SYSTEM_ALIASES = ["postmaster", "abuse", "dmarc", "tlsrpt"]; -const STANDARD_FOLDERS: Array<[string, string]> = [ - ["INBOX", "inbox"], - ["Sent", "sent"], - ["Drafts", "drafts"], - ["Trash", "trash"], - ["Junk", "junk"], - ["Archive", "archive"], -]; +/** A refusal the operator should read, as opposed to a bug. */ +export class CliError extends Error {} + +// Thrown rather than exiting here, so the commands can be exercised from a test; +// main() prints it as "error: ..." and exits 1, exactly as this used to. function fail(message: string): never { - console.error(`error: ${message}`); - process.exit(1); + throw new CliError(message); } function parseAddress(email: string): { localPart: string; domain: string } { @@ -37,7 +34,7 @@ function parseAddress(email: string): { localPart: string; domain: string } { return { localPart: email.slice(0, at).toLowerCase(), domain: email.slice(at + 1).toLowerCase() }; } -async function createAdmin(email: string, password: string, force: boolean): Promise { +export async function createAdmin(email: string, password: string, force: boolean): Promise { if (password.length < 12) fail("the password must be at least 12 characters"); const { localPart, domain } = parseAddress(email); @@ -69,13 +66,10 @@ async function createAdmin(email: string, password: string, force: boolean): Pro ); // Delivery needs these: a mailbox with no INBOX cannot receive at all, and - // one with no Junk used to fail the spam path outright. - for (const [name, specialUse] of STANDARD_FOLDERS) { - await query( - `INSERT INTO folders (mailbox_id, name, special_use) VALUES ($1, $2, $3)`, - [mailbox!.id, name, specialUse], - ); - } + // one with no Junk used to fail the spam path outright. The list lives in the + // database (migration 0014) so the console and this command cannot disagree + // about it, as they once did over the Reports folder. + await query(`SELECT provision_default_folders($1)`, [mailbox!.id]); // PLAN.md section 7.4b: the published DNS records name these, and reports // bounce if they do not resolve. @@ -174,10 +168,13 @@ async function main(): Promise { } } -main() - .then(() => closePool()) - .catch(async (err) => { - console.error(`error: ${err instanceof Error ? err.message : String(err)}`); - await closePool().catch(() => undefined); - process.exit(1); - }); +// Only when run as a command, not when a test imports the functions above +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { + main() + .then(() => closePool()) + .catch(async (err) => { + console.error(`error: ${err instanceof Error ? err.message : String(err)}`); + await closePool().catch(() => undefined); + process.exit(1); + }); +} diff --git a/services/auth/src/repository.ts b/services/auth/src/repository.ts index 45457a2..0560da4 100644 --- a/services/auth/src/repository.ts +++ b/services/auth/src/repository.ts @@ -1004,18 +1004,9 @@ export async function createMailbox(scope: AdminScope, input: CreateMailboxInput [input.domainId, localPart, email, hash, input.role, input.quotaBytes], ); - await query( - `INSERT INTO folders (mailbox_id, name, special_use) - SELECT $1, f.name, f.special_use - FROM (VALUES ('INBOX','inbox'), ('Sent','sent'), ('Drafts','drafts'), - ('Trash','trash'), ('Junk','junk'), ('Archive','archive'), - -- Where delivered DMARC and TLS-RPT reports are filed. - -- IMAP has no special-use role for these, so it is matched - -- by name. Without the folder the delivery path falls back - -- to INBOX, which is the noise this is here to remove. - ('Reports', NULL)) AS f(name, special_use)`, - [created!.id], - ); + // The standard folders come from one definition in the database (migration + // 0014), shared with the command that creates the first administrator. + await query(`SELECT provision_default_folders($1)`, [created!.id]); return { status: "OK", id: created!.id, email }; } diff --git a/services/protocols/internal/infrastructure/postgres/mailbox_state_migration_test.go b/services/protocols/internal/infrastructure/postgres/mailbox_state_migration_test.go new file mode 100644 index 0000000..81be194 --- /dev/null +++ b/services/protocols/internal/infrastructure/postgres/mailbox_state_migration_test.go @@ -0,0 +1,158 @@ +package postgres + +import ( + "context" + "os" + "testing" + + "github.com/google/uuid" + "github.com/jackc/pgx/v5/pgxpool" +) + +// runMigration0014 applies the migration file itself, so the test cannot drift +// from what production runs. It is idempotent by design, which is also tested. +func runMigration0014(t *testing.T, pool *pgxpool.Pool) { + t.Helper() + sql, err := os.ReadFile("../../../../../migrations/0014_mailbox_state_single_source.up.sql") + if err != nil { + t.Fatalf("read migration: %v", err) + } + if _, err := pool.Exec(context.Background(), string(sql)); err != nil { + t.Fatalf("apply migration 0014: %v", err) + } +} + +func folderNames(t *testing.T, pool *pgxpool.Pool, mailboxID string) map[string]string { + t.Helper() + rows, err := pool.Query(context.Background(), `SELECT name, COALESCE(special_use, '') FROM folders WHERE mailbox_id = $1`, mailboxID) + if err != nil { + t.Fatalf("list folders: %v", err) + } + defer rows.Close() + out := map[string]string{} + for rows.Next() { + var name, role string + if err := rows.Scan(&name, &role); err != nil { + t.Fatalf("scan: %v", err) + } + out[name] = role + } + return out +} + +// Numbers that two paths left wrong are corrected from the rows. +func TestMigration0014_ReconcilesDriftedCountersAndQuota(t *testing.T) { + pool := testPool(t) + ctx := context.Background() + mailboxID := seedMailbox(t, pool) + deliver(t, pool, mailboxID, "INBOX", "a", false) + deliver(t, pool, mailboxID, "INBOX", "b", true) + deliver(t, pool, mailboxID, "Sent", "c", true) + + // The drift as it was found: counters and quota that disagree with the rows + if _, err := pool.Exec(ctx, `UPDATE folders SET unread_count = 9, total_count = 7 WHERE mailbox_id = $1`, mailboxID); err != nil { + t.Fatalf("corrupt counters: %v", err) + } + if _, err := pool.Exec(ctx, `UPDATE mailboxes SET used_bytes = 12345 WHERE id = $1`, mailboxID); err != nil { + t.Fatalf("corrupt quota: %v", err) + } + + runMigration0014(t, pool) + assertConsistent(t, pool, mailboxID, "the reconciliation") + + runMigration0014(t, pool) + assertConsistent(t, pool, mailboxID, "running the reconciliation a second time") +} + +// A mailbox that is already right is left exactly as it was. +func TestMigration0014_LeavesATrueMailboxAlone(t *testing.T) { + pool := testPool(t) + ctx := context.Background() + mailboxID := seedMailbox(t, pool) + deliver(t, pool, mailboxID, "INBOX", "a", false) + before, usedBefore, _ := mailState(t, pool, mailboxID) + + runMigration0014(t, pool) + + after, usedAfter, _ := mailState(t, pool, mailboxID) + for name, f := range before { + if after[name].CachedTotal != f.CachedTotal || after[name].CachedUnread != f.CachedUnread || after[name].HighestModSeq != f.HighestModSeq { + t.Errorf("%s changed: %+v -> %+v", name, f, after[name]) + } + } + if usedAfter != usedBefore { + t.Errorf("used_bytes changed from %d to %d", usedBefore, usedAfter) + } + _ = ctx +} + +// The one list of standard folders: what is missing is added, what is there is +// not duplicated, and running it again changes nothing. +func TestProvisionDefaultFolders(t *testing.T) { + pool := testPool(t) + ctx := context.Background() + runMigration0014(t, pool) + + domainID, mailboxID := uuid.New(), uuid.New() + name := "prov" + uuid.NewString()[:8] + ".test" + if _, err := pool.Exec(ctx, `INSERT INTO domains (id, name, punycode_name, is_active) VALUES ($1,$2,$3,true)`, domainID, name, name); err != nil { + t.Fatalf("seed domain: %v", err) + } + t.Cleanup(func() { _, _ = pool.Exec(context.Background(), `DELETE FROM domains WHERE id = $1`, domainID) }) + if _, err := pool.Exec(ctx, `INSERT INTO mailboxes (id, domain_id, local_part, email_address, password_hash, is_active) VALUES ($1,$2,'u',$3,'x',true)`, + mailboxID, domainID, "u@"+name); err != nil { + t.Fatalf("seed mailbox: %v", err) + } + + want := map[string]string{"INBOX": "inbox", "Sent": "sent", "Drafts": "drafts", "Trash": "trash", "Junk": "junk", "Archive": "archive", "Reports": ""} + + if _, err := pool.Exec(ctx, `SELECT provision_default_folders($1)`, mailboxID); err != nil { + t.Fatalf("provision: %v", err) + } + got := folderNames(t, pool, mailboxID.String()) + for n, role := range want { + if r, ok := got[n]; !ok || r != role { + t.Errorf("folder %q: present=%v role=%q, want role %q", n, ok, r, role) + } + } + if len(got) != len(want) { + t.Errorf("got %d folders, want %d: %v", len(got), len(want), got) + } + + if _, err := pool.Exec(ctx, `SELECT provision_default_folders($1)`, mailboxID); err != nil { + t.Fatalf("provision again: %v", err) + } + if again := folderNames(t, pool, mailboxID.String()); len(again) != len(want) { + t.Errorf("a second call changed the folder count to %d", len(again)) + } +} + +// A folder that exists under another capitalisation counts as present, and the +// folder keeps the owner's mail. +func TestProvisionDefaultFolders_DoesNotDuplicateByCase(t *testing.T) { + pool := testPool(t) + ctx := context.Background() + runMigration0014(t, pool) + mailboxID := seedMailbox(t, pool) // INBOX, Sent, Drafts, Trash + + if _, err := pool.Exec(ctx, `INSERT INTO folders (mailbox_id, name, special_use) VALUES ($1, 'reports', NULL)`, mailboxID); err != nil { + t.Fatalf("seed: %v", err) + } + if _, err := pool.Exec(ctx, `SELECT provision_default_folders($1)`, mailboxID); err != nil { + t.Fatalf("provision: %v", err) + } + + got := folderNames(t, pool, mailboxID) + reports := 0 + for n := range got { + if n == "reports" || n == "Reports" { + reports++ + } + } + if reports != 1 { + t.Errorf("found %d reports folders, want 1: %v", reports, got) + } + if len(got) != 7 { + t.Errorf("got %d folders, want 7: %v", len(got), got) + } +}