From a8941a61912d0a36198ea82872530fa041dfb6fa Mon Sep 17 00:00:00 2001 From: Lucas Rayan Date: Fri, 16 Oct 2026 13:19:55 -0300 Subject: [PATCH] refactor(mail): one definition of the standard folders, and reconcile the drift The console created Reports, the CLI that makes the first administrator did not, so that account had no folder for DMARC and TLS-RPT reports. The list now lives in one SQL function, provision_default_folders (migration 0014), which both call. The same migration reconciles what the two paths had left wrong in existing mailboxes: folder counters and the mailbox's used bytes are recomputed from the rows (only where they differ, so it is safe to run again). create-admin can now be tested: fail() throws instead of exiting, and the command runs only when invoked as one. The new test fails on the old code (no Reports). --- .../0014_mailbox_state_single_source.down.sql | 4 + .../0014_mailbox_state_single_source.up.sql | 77 +++++++++ services/auth/src/cli.test.ts | 46 +++++ services/auth/src/cli.ts | 47 +++--- services/auth/src/repository.ts | 15 +- .../postgres/mailbox_state_migration_test.go | 158 ++++++++++++++++++ 6 files changed, 310 insertions(+), 37 deletions(-) create mode 100644 migrations/0014_mailbox_state_single_source.down.sql create mode 100644 migrations/0014_mailbox_state_single_source.up.sql create mode 100644 services/auth/src/cli.test.ts create mode 100644 services/protocols/internal/infrastructure/postgres/mailbox_state_migration_test.go 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) + } +}