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).

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);

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);

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/);
+  });
+});

services/auth/src/cli.ts

@@ -13,22 +13,19 @@
  *   node dist/cli.js reset-mfa <email>
  */
 
+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}`);
+  throw new CliError(message);
-  process.exit(1);
 }
 
 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<void> {
+export async function createAdmin(email: string, password: string, force: boolean): Promise<void> {
   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.
+  // one with no Junk used to fail the spam path outright. The list lives in the
-  for (const [name, specialUse] of STANDARD_FOLDERS) {
+  // database (migration 0014) so the console and this command cannot disagree
-    await query(
+  // about it, as they once did over the Reports folder.
-      `INSERT INTO folders (mailbox_id, name, special_use) VALUES ($1, $2, $3)`,
+  await query(`SELECT provision_default_folders($1)`, [mailbox!.id]);
-      [mailbox!.id, name, specialUse],
-    );
-  }
 
   // PLAN.md section 7.4b: the published DNS records name these, and reports
   // bounce if they do not resolve.
@@ -174,6 +168,8 @@ async function main(): Promise<void> {
   }
 }
 
+// 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) => {
@@ -181,3 +177,4 @@ main()
       await closePool().catch(() => undefined);
       process.exit(1);
     });
+}

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(
+  // The standard folders come from one definition in the database (migration
-    `INSERT INTO folders (mailbox_id, name, special_use)
+  // 0014), shared with the command that creates the first administrator.
-     SELECT $1, f.name, f.special_use
+  await query(`SELECT provision_default_folders($1)`, [created!.id]);
-       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],
-  );
 
   return { status: "OK", id: created!.id, email };
 }

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)
+	}
+}