From b0bb942a0240774fdea5f62ee15d7fabede3a388 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 08:24:10 -0500 Subject: [PATCH 1/6] test(imap): measure folder-open round-trip structure (#125) Investigate IMAP folder-open latency (follow-up to #86). Localhost GreenMail has ~0 RTT, so real wall-clock latency can't be measured here; instead this pins the folder-open round-trip STRUCTURE deterministically. Finding: ImapClient.withStore wraps every operation in its own short-lived Store, so each folder-open pays a full CONNECT + TLS + LOGIN + EXAMINE + FETCH + LOGOUT. Only EXAMINE + FETCH is intrinsic to opening a folder; the whole connection-setup group is avoidable on the 2nd+ operation if a connection were reused. Optimistic render-from-cache already exists (selectFolder renders cached rows; the network sync is a background refresh). Adds: - CountingImapProxy: a localhost TCP proxy that forwards a cleartext IMAP session to GreenMail while counting TCP connections and parsing IMAP command words. - ImapFolderOpenLatencyTest: asserts the current no-reuse behaviour (N opens => N connections and N LOGINs; list+read => 2 connections) against a real in-process IMAP server. Doubles as the harness to validate a future connection-reuse fix (flip the counts to assert reuse). - docs/perf/issue-125-imap-folder-open.md: the per-open round-trip sequence, avoidable vs. necessary round-trips, and the recommended per-account connection-reuse/keep-alive mitigation with its IDLE / thread-safety / battery / stale-connection constraints. Analysis + harness only; the connection-reuse fix is deferred pending real-network + real-device measurement (see the doc's measurement plan), so this references #125 without closing it. Co-Authored-By: Claude Fable 5 --- .../org/libremail/mail/CountingImapProxy.kt | 163 ++++++++++++++++ .../mail/ImapFolderOpenLatencyTest.kt | 125 +++++++++++++ docs/perf/issue-125-imap-folder-open.md | 175 ++++++++++++++++++ 3 files changed, 463 insertions(+) create mode 100644 app/src/test/kotlin/org/libremail/mail/CountingImapProxy.kt create mode 100644 app/src/test/kotlin/org/libremail/mail/ImapFolderOpenLatencyTest.kt create mode 100644 docs/perf/issue-125-imap-folder-open.md diff --git a/app/src/test/kotlin/org/libremail/mail/CountingImapProxy.kt b/app/src/test/kotlin/org/libremail/mail/CountingImapProxy.kt new file mode 100644 index 0000000..145754e --- /dev/null +++ b/app/src/test/kotlin/org/libremail/mail/CountingImapProxy.kt @@ -0,0 +1,163 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.mail + +import java.io.IOException +import java.net.InetAddress +import java.net.ServerSocket +import java.net.Socket +import java.util.Collections +import java.util.concurrent.ConcurrentHashMap +import java.util.concurrent.atomic.AtomicInteger + +/** + * A tiny localhost TCP proxy that forwards a **cleartext** IMAP session to a real backend (GreenMail) + * while COUNTING what crosses it, so tests can measure the folder-open round-trip *structure* + * deterministically without a real network (issue #125): + * + * - [connectionCount] — how many separate TCP connections the client established. On a real network + * each new connection is a full CONNECT + TLS handshake + LOGIN/AUTH handshake group (several + * RTTs). [ImapClient] opens one [jakarta.mail.Store] — and therefore one connection — per + * operation today, so this equals the number of operations. Connection reuse / pooling would make + * it diverge (many operations, few connections); that divergence is exactly what a future fix + * should produce and what these tests are wired to detect. + * - [commandCount] — how many times each IMAP command word (LOGIN, EXAMINE, SELECT, FETCH, LOGOUT…) + * the client issued, parsed from the cleartext client → server stream. + * + * Point [ImapClient] at [port] instead of the backend's port. Cleartext only (MailSecurity.NONE): + * command parsing needs to see the bytes. Connection counting alone would work through TLS too, but + * the LibreMail unit tests already exercise the plaintext path, matching the existing GreenMail tests. + */ +class CountingImapProxy(private val backendHost: String, private val backendPort: Int) : AutoCloseable { + + private val server = ServerSocket(0, BACKLOG, InetAddress.getByName("127.0.0.1")) + private val connections = AtomicInteger(0) + private val commands = ConcurrentHashMap() + + /** Client → server pump threads, tracked so tests can wait for the parsed command stream to settle. */ + private val clientPumps = Collections.synchronizedList(mutableListOf()) + + @Volatile private var running = true + + /** The local port to point [ImapClient] at; it forwards to the backend. */ + val port: Int get() = server.localPort + + /** Total TCP connections the client has opened through the proxy. */ + val connectionCount: Int get() = connections.get() + + /** How many times the client issued [command] (case-insensitive), e.g. "LOGIN", "EXAMINE". */ + fun commandCount(command: String): Int = commands[command.uppercase()]?.get() ?: 0 + + /** Authentication round-trips: the `LOGIN` command plus any SASL `AUTHENTICATE` (e.g. XOAUTH2). */ + fun authCommandCount(): Int = commandCount("LOGIN") + commandCount("AUTHENTICATE") + + init { + Thread({ acceptLoop() }, "imap-proxy-accept").apply { isDaemon = true }.start() + } + + /** + * Joins the client → server pump threads so every command line sent before each connection closed + * has been parsed. [ImapClient] closes its store (and thus the socket) when an operation finishes, + * which ends the corresponding pump; call this before asserting on [commandCount]. [connectionCount] + * needs no settling — it is incremented synchronously as each connection is accepted. + */ + fun awaitClientStreamsSettled(timeoutMs: Long = SETTLE_TIMEOUT_MS) { + val deadline = System.currentTimeMillis() + timeoutMs + val snapshot = synchronized(clientPumps) { clientPumps.toList() } + for (thread in snapshot) { + val remaining = deadline - System.currentTimeMillis() + if (remaining > 0) thread.join(remaining) + } + } + + override fun close() { + running = false + runCatching { server.close() } + } + + private fun acceptLoop() { + while (running) { + val client = try { + server.accept() + } catch (_: IOException) { + return // server socket closed by close() + } + connections.incrementAndGet() + val backend = try { + Socket(backendHost, backendPort) + } catch (_: IOException) { + runCatching { client.close() } + continue + } + val upstream = Thread({ pumpCountingCommands(client, backend) }, "imap-proxy-up").apply { isDaemon = true } + val downstream = Thread({ pump(backend, client) }, "imap-proxy-down").apply { isDaemon = true } + clientPumps.add(upstream) + upstream.start() + downstream.start() + } + } + + /** Forwards client → server bytes verbatim while parsing each CRLF-terminated line as a command. */ + private fun pumpCountingCommands(from: Socket, to: Socket) { + val buffer = ByteArray(BUFFER_SIZE) + val line = StringBuilder() + try { + val input = from.getInputStream() + val output = to.getOutputStream() + while (true) { + val read = input.read(buffer) + if (read < 0) break + output.write(buffer, 0, read) + output.flush() + for (i in 0 until read) { + when (val ch = buffer[i].toInt().toChar()) { + '\n' -> { + recordCommand(line.toString()) + line.setLength(0) + } + '\r' -> Unit + else -> line.append(ch) + } + } + } + } catch (_: IOException) { + // Peer closed; fall through to socket cleanup. + } finally { + runCatching { from.close() } + runCatching { to.close() } + } + } + + /** Forwards server → client bytes verbatim (no parsing needed for this direction). */ + private fun pump(from: Socket, to: Socket) { + val buffer = ByteArray(BUFFER_SIZE) + try { + val input = from.getInputStream() + val output = to.getOutputStream() + while (true) { + val read = input.read(buffer) + if (read < 0) break + output.write(buffer, 0, read) + output.flush() + } + } catch (_: IOException) { + // Peer closed; fall through to socket cleanup. + } finally { + runCatching { from.close() } + runCatching { to.close() } + } + } + + /** Records the command word of an IMAP line shaped ` [args]`. */ + private fun recordCommand(rawLine: String) { + val parts = rawLine.trim().split(' ', limit = 3) + if (parts.size < 2) return + val command = parts[1].uppercase() + commands.computeIfAbsent(command) { AtomicInteger(0) }.incrementAndGet() + } + + private companion object { + const val BACKLOG = 50 + const val BUFFER_SIZE = 8192 + const val SETTLE_TIMEOUT_MS = 2_000L + } +} diff --git a/app/src/test/kotlin/org/libremail/mail/ImapFolderOpenLatencyTest.kt b/app/src/test/kotlin/org/libremail/mail/ImapFolderOpenLatencyTest.kt new file mode 100644 index 0000000..62c5dd1 --- /dev/null +++ b/app/src/test/kotlin/org/libremail/mail/ImapFolderOpenLatencyTest.kt @@ -0,0 +1,125 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.mail + +import com.icegreen.greenmail.util.GreenMail +import com.icegreen.greenmail.util.GreenMailUtil +import com.icegreen.greenmail.util.ServerSetupTest +import kotlinx.coroutines.test.runTest +import org.junit.After +import org.junit.Before +import org.junit.Test +import org.libremail.domain.model.ImapConnectionParams +import org.libremail.domain.model.MailSecurity +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +/** + * Measures the folder-open round-trip *structure* over a real in-process IMAP server (issue #125), + * deterministically and without a real network, by routing [ImapClient] through a [CountingImapProxy] + * that counts the TCP connections and IMAP commands it establishes. + * + * The finding these tests pin down: [ImapClient] wraps every operation in its own short-lived + * [jakarta.mail.Store] (`withStore`), so **each folder-open pays a fresh CONNECT + LOGIN + SELECT + + * FETCH + LOGOUT** — nothing is reused between operations. On a real network the CONNECT + TLS + LOGIN + * group is several RTTs of user-perceived latency that a pooled/kept-alive connection would pay only + * once. See `docs/perf/issue-125-imap-folder-open.md`. + * + * These assertions encode the *current* (no-reuse) behaviour. They are also the harness to validate a + * future connection-reuse fix: when the client reuses one authenticated connection across folder + * switches, the connection/auth counts here drop below the operation count — flip the expectations to + * assert reuse and the tests confirm the win against a real IMAP server. + */ +class ImapFolderOpenLatencyTest { + + private lateinit var greenMail: GreenMail + private lateinit var proxy: CountingImapProxy + private val client = ImapClient() + + @Before + fun setUp() { + greenMail = GreenMail(ServerSetupTest.SMTP_IMAP) + greenMail.start() + greenMail.setUser("alice@example.org", "secret") + // All IMAP traffic goes through the proxy so we can count it; the proxy forwards to GreenMail. + proxy = CountingImapProxy(backendHost = "127.0.0.1", backendPort = greenMail.imap.port) + } + + @After + fun tearDown() { + proxy.close() + greenMail.stop() + } + + /** Points [ImapClient] at the counting proxy rather than directly at GreenMail. */ + private fun params() = ImapConnectionParams( + host = "127.0.0.1", + port = proxy.port, + security = MailSecurity.NONE, + username = "alice@example.org", + secret = "secret", + useXoauth2 = false, + ) + + private fun seedInbox(count: Int) { + repeat(count) { i -> + GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Subject $i", "Body $i") + } + greenMail.waitForIncomingEmail(count) + } + + @Test + fun `each folder-open establishes a brand-new IMAP connection (no reuse today)`() = runTest { + seedInbox(2) + + repeat(OPENS) { client.fetchRecent(params(), "INBOX", limit = 50) } + + // One TCP connection per open: nothing is pooled or kept alive across folder-opens. A + // connection-reuse fix would make this strictly less than OPENS. + assertEquals(OPENS, proxy.connectionCount, "expected one fresh connection per folder-open") + } + + @Test + fun `each folder-open pays a fresh LOGIN and its own SELECT`() = runTest { + seedInbox(2) + + repeat(OPENS) { client.fetchRecent(params(), "INBOX", limit = 50) } + proxy.awaitClientStreamsSettled() + + // The avoidable round-trip: a full authentication on every open. Reuse would drop this to 1. + assertEquals(OPENS, proxy.authCommandCount(), "expected one LOGIN per folder-open") + // The necessary per-open work: READ_ONLY open issues EXAMINE. Reuse keeps this at one-per-open. + assertEquals(OPENS, proxy.commandCount("EXAMINE"), "expected one EXAMINE per folder-open") + } + + @Test + fun `a single folder-open's round-trip sequence is CONNECT-LOGIN-EXAMINE-FETCH-LOGOUT`() = runTest { + seedInbox(3) + + client.fetchRecent(params(), "INBOX", limit = 50) + proxy.awaitClientStreamsSettled() + + assertEquals(1, proxy.connectionCount, "one connection") + assertEquals(1, proxy.authCommandCount(), "one LOGIN — the connection-setup cost, avoidable on reuse") + assertEquals(1, proxy.commandCount("EXAMINE"), "one EXAMINE — the necessary per-folder SELECT") + assertTrue(proxy.commandCount("FETCH") >= 1, "at least one FETCH — the necessary header download") + assertEquals(1, proxy.commandCount("LOGOUT"), "one LOGOUT — the connection is torn down, not kept alive") + } + + @Test + fun `opening a folder then reading a message uses two separate connections (compounding cost)`() = runTest { + seedInbox(1) + + val uid = client.fetchRecent(params(), "INBOX", limit = 50).first().uid // open folder -> connection 1 + client.fetchBodyMarkingSeen(params(), "INBOX", uid) // read a message -> connection 2 + proxy.awaitClientStreamsSettled() + + // No session is shared between listing the folder and reading a message in it: the read pays a + // second full CONNECT + LOGIN even though it targets the folder we just had open. + assertEquals(2, proxy.connectionCount, "list + read each open their own connection") + assertEquals(2, proxy.authCommandCount(), "list + read each pay a full LOGIN") + } + + private companion object { + const val OPENS = 3 + } +} diff --git a/docs/perf/issue-125-imap-folder-open.md b/docs/perf/issue-125-imap-folder-open.md new file mode 100644 index 0000000..aebc139 --- /dev/null +++ b/docs/perf/issue-125-imap-folder-open.md @@ -0,0 +1,175 @@ + +# IMAP folder-open latency (issue #125) + +Structural analysis of the IMAP round-trips paid when opening/selecting a folder, a follow-up to the +#86 profiling and distinct from the mailbox cached-render fix (#123) and the fetch policy (#88–#90). + +Performed 2026-07-02 against `main` by reading the folder-open path and confirming the round-trip +*structure* with deterministic GreenMail tests (`ImapFolderOpenLatencyTest` + +`CountingImapProxy`). + +**Verdict.** The folder-open network path re-establishes a **full, freshly-authenticated IMAP +connection on every operation** — there is no connection pooling or keep-alive. Each folder-open pays +`CONNECT + TLS + LOGIN + EXAMINE + FETCH + LOGOUT`; only the `EXAMINE + FETCH` is intrinsic to opening +a folder, and the entire `CONNECT + TLS + LOGIN` setup group (the majority of the round-trips) is +**avoidable on the second and subsequent operations** if a connection were reused. The recommended +mitigation is a per-account connection cache/keep-alive. It is **not implemented here**: the sizing, +eviction, stale-detection, and battery trade-offs are genuine latency/battery decisions that need +real-network + real-device measurement (which localhost GreenMail — ~0 RTT — cannot provide), and a +naïve implementation risks regressing the deliberate concurrency design and the IDLE connection budget. +This is the "spike first, measure before committing" the issue asks for. + +> **Note on numbers.** This document counts *protocol round-trips* (RTTs), which are deterministic and +> measurable in-process. It does **not** quote measured wall-clock latency — there is no real network +> or account in this environment. Where a millisecond figure appears it is explicitly *illustrative +> arithmetic* (`round-trips × RTT`), with RTT a placeholder for a real network's round-trip time. + +## The folder-open path + +Opening/selecting a folder in the UI runs two independent things: + +1. **Render from cache (already optimized, not the subject of #125).** + `MailboxViewModel.selectFolder()` sets `_selectedFolder` synchronously + (`MailboxViewModel.kt:307`). That immediately re-filters the cached Room rows into the list — no + network. #123 optimized this cached render. The network open below is *off* the render path, so its + cost shows up as a background refresh, not a blank screen. + +2. **Network sync (the subject of #125).** + `selectFolder()` then launches `mailSyncer.syncFolder(accountId, folder)`: + + ``` + MailboxViewModel.selectFolder() (MailboxViewModel.kt:307) + └─ MailSyncer.syncFolder() (MailSyncer.kt:80) + └─ syncFolderHeaders() (MailSyncer.kt:87) + ├─ connectionFactory.imapParamsFor(account) (resolves/refreshes credentials) + └─ imapClient.fetchRecent(params, folder, limit) (MailSyncer.kt:93) + └─ ImapClient.withStore { … } (ImapClient.kt:111, 521) + ``` + +`ImapClient.withStore()` is the crux (`ImapClient.kt:521`): + +```kotlin +private inline fun withStore(params: ImapConnectionParams, block: (Store) -> T): T { + val store = Session.getInstance(buildProps(protocol, params)).getStore(protocol) + store.connect(params.host, params.port, params.username, params.secret) // CONNECT + TLS + LOGIN + return try { block(store) } finally { runCatching { store.close() } } // LOGOUT + teardown +} +``` + +**Every** `ImapClient` operation — `fetchRecent`, `fetchOlderThan`, `search`, `fetchBodyMarkingSeen`, +`fetchBodyPeek`, `fetchAttachment`, `setFlag`, `deleteMessage`, `moveMessages`, `fetchForReply` — is a +`withStore { … }`, so each one builds and authenticates its own connection and tears it down. Nothing +is reused between operations. + +## Per-open round-trip sequence + +For one `fetchRecent` (a folder-open), the client → server exchange is: + +| # | Step | RTTs | Necessary to *open a folder*? | +|---|------|------|-------------------------------| +| 1 | TCP handshake | ~1 | Setup — avoidable on reuse | +| 2 | TLS handshake (implicit TLS / `imaps`) | 1 (TLS 1.3) – 2 (TLS 1.2) | Setup — avoidable on reuse | +| 3 | `CAPABILITY` (Angus; reused from greeting when advertised) | 0–1 | Setup — avoidable on reuse | +| 4 | `LOGIN` / `AUTHENTICATE XOAUTH2` | 1 (+1 if challenged) | Setup — avoidable on reuse | +| 5 | `CAPABILITY` post-auth (reused from `LOGIN` response when advertised) | 0–1 | Setup — avoidable on reuse | +| 6 | `EXAMINE` (READ_ONLY select of the folder) | 1 | **Necessary** per folder | +| 7 | `FETCH` recent headers (`ENVELOPE FLAGS UID`) | 1 | **Necessary** header download | +| 8 | `LOGOUT` + socket teardown | ~1 | Setup — avoidable on reuse | + +- **STARTTLS (`imap` on 143)** is worse: it inserts a pre-TLS `CAPABILITY`, the `STARTTLS` command, + then a post-TLS `CAPABILITY` *before* step 4 — roughly **6–8 setup RTTs** instead of 4–6. +- **Setup (steps 1–5, 8): ~4–6 RTT (imaps) / ~6–8 RTT (STARTTLS).** +- **Intrinsic folder work (steps 6–7): 2 RTT.** + +So the connection setup is the **majority** of the round-trips on every open, and it is exactly the +part a reused connection would skip. Illustratively, at an RTT of *R*: a cold open ≈ `(4–6)·R` setup + +`2·R` work; a warm (reused-connection) open ≈ `2·R`. The setup share — everything except the +`EXAMINE + FETCH` — is what a fix removes from the 2nd open onward. + +### Compounding across operations + +Because the pattern is per-operation, costs stack: + +- **Folder switch A → B → A:** 3 folder-opens ⇒ 3 full `CONNECT + TLS + LOGIN` setups. +- **List then open a message:** `fetchRecent` (open) + `fetchBodyMarkingSeen` (read) ⇒ 2 full setups, + even though the read targets the folder just listed (proven by the test below). +- **Prefetch after a sync** (`MailSyncer.prefetchIfEnabled`, FetchPolicy territory #88–#90, *not* + changed here): each unfetched message body is another `withStore` connection, and each attachment + another still. A folder-open that triggers prefetch of *K* messages can open `1 + K + attachments` + separate authenticated connections. This amplifies the motivation for pooling but is out of scope. + +## Deterministic evidence (no real network needed) + +`ImapFolderOpenLatencyTest` routes `ImapClient` through `CountingImapProxy` — a localhost TCP proxy +that forwards a cleartext IMAP session to in-process GreenMail while counting connections and parsing +IMAP command words. This measures the *structure* exactly, without needing real latency: + +- `each folder-open establishes a brand-new IMAP connection (no reuse today)` — N opens ⇒ **N** TCP + connections. +- `each folder-open pays a fresh LOGIN and its own SELECT` — N opens ⇒ **N** `LOGIN` **and** N + `EXAMINE` (the avoidable auth vs. the necessary select). +- `a single folder-open's round-trip sequence is CONNECT-LOGIN-EXAMINE-FETCH-LOGOUT` — pins the + sequence: 1 connection, 1 `LOGIN`, 1 `EXAMINE`, ≥1 `FETCH`, 1 `LOGOUT`. +- `opening a folder then reading a message uses two separate connections (compounding cost)` — list + + read ⇒ **2** connections and **2** `LOGIN`s. + +These assertions encode the *current* (no-reuse) behaviour and double as the **validation harness for a +future fix**: once a connection is reused across folder switches, the connection/auth counts drop below +the operation count — flip the expectations to assert reuse and the same real-IMAP tests confirm the win. + +## Recommended mitigation: per-account connection reuse / keep-alive + +Keep one authenticated `Store` alive per account and reuse it across folder-opens and message +operations instead of `withStore`'s connect-per-call, so only the first operation pays setup and +subsequent ones pay just `EXAMINE + FETCH`. Design constraints that make this **non-trivial** and why +it needs measurement before landing: + +1. **Must not disturb IMAP IDLE (#90).** `IdleService` already holds a *separate*, dedicated + long-lived `Store` per account (`ImapClient.idle`, `IdleService.watchAccount`), blocking on + `INBOX.idle()`. IMAP is serial per connection and IDLE blocks its connection, so folder-opens + cannot be multiplexed onto it. A reuse pool is therefore an **additional** persistent connection + per account (IDLE + pool), which must respect the server's per-account connection limit (Gmail + ~15; many servers 3–5) — a budget `ImapClient.idle`'s own comment already flags. +2. **Thread-safety.** `MailRepositoryImpl`'s UI operations (`openMessage`, `setStarred`, + `deleteMessage`, `moveMessages`, `setFlag`, …) are **not** serialized and can overlap `MailSyncer` + (whose `prefetchIfEnabled` deliberately runs *outside* `syncMutex` so downloads don't block + pull-to-refresh). Today's connect-per-call sidesteps this. A shared connection needs its own + discipline: a single mutex-guarded connection (simplest, but head-of-line-blocks a flag toggle + behind a slow body download — a regression of the current concurrency) **or** a small bounded pool + of N connections (more throughput, needs a size cap + eviction). Choosing between them is a + latency/throughput trade-off that needs real measurement. +3. **Stale-connection handling.** A pooled socket can be dropped by the server's idle timeout + (RFC-permitted), NAT rebinding, or a network change. Reuse must detect staleness — a `NOOP` probe + (adds 1 RTT, partly defeating the point) or catch-and-retry-once on a fresh connection — behaviour + best validated against real servers and real network transitions. +4. **Battery / lifecycle (#88/#89/#90).** Holding a socket open has a battery cost; #90 already tears + IDLE down at low battery. A reuse pool needs an idle-eviction timeout and should likely mirror that + low-battery posture. The right timeout is a battery-vs-latency trade-off that needs device + measurement. + +Because every one of these knobs (mutex vs. pool, eviction timeout, stale-probe strategy, battery +posture) trades latency against battery/complexity and can only be tuned with a real network and a +real device — which this environment cannot provide — forcing an implementation now would be guessing. +Per #125's "investigation/spike first" guidance, this change ships the measurement harness + analysis +and **defers the pool to a measured follow-up**. + +**Already correct — do not redo.** Optimistic render-from-cache is already the architecture +(`selectFolder` renders cached rows instantly; the network sync is a background refresh). #125's +"optimistic render while the network catches up" is satisfied; only connection reuse remains. + +## What a maintainer needs to fully close #125 (real device + real account) + +1. **Instrument the open.** Add timing around `syncFolder → fetchRecent → store.connect / open / fetch + / close` (or enable Angus `mail.imap` debug) and capture on a real Gmail/Outlook account over both + Wi-Fi and cellular. +2. **Attribute the wall-clock.** Break each open into connect (TCP+TLS), login, `EXAMINE`, `FETCH`, + `LOGOUT`; confirm the hypothesis that connection setup dominates and quantify its share. +3. **A/B the pool behind a flag.** Measure folder-switch latency (open A → open B → back to A) and + list-then-open-message latency, cold vs. warm-reuse, on the same accounts/networks. Expect warm + opens to fall by the connection-setup share. +4. **Battery check.** Measure the kept-alive socket's idle cost against candidate eviction timeouts; + confirm no regression versus the #88/#89/#90 posture. +5. **Resilience check.** Force server idle-timeout and network transitions; confirm transparent + reconnect with no user-visible failures, and that IDLE + pool stay within the per-account limit. +6. **Lock it in.** Flip `ImapFolderOpenLatencyTest` to assert reuse (connection/auth counts < operation + count) as the deterministic regression guard. From 9d70bc29320b65498af38f2a992bd26641fea74a Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 03:00:45 -0500 Subject: [PATCH 2/6] fix(security): move accounts/credentials to a non-auth-bound database Accounts, credentials, per-account settings and signatures lived in the same libremail.db that SQLCipher encrypts under the auth-bound passphrase when app-lock + encrypted-cache are on. A genuine key invalidation (biometric re-enrollment or lock removal/re-add) made that file undecryptable, and the "clear + re-sync" recovery wiped the accounts and stored credentials along with the mail cache, dropping the user into onboarding (issue #111). Move those four tables into a new plaintext AccountDatabase (libremail-accounts.db) that is never bound to the auth key. Credentials stay AES-GCM sealed at the column level by the surviving non-auth KeystoreCrypto master key, so the only secret never touches disk in the clear. A cache-key invalidation now wipes only libremail.db; the user stays signed in. - AccountDatabase (v1) + AccountDatabaseModule; the cache DB drops to v15 via MIGRATION_14_15. DAOs are unchanged and re-provided from the new DB, so no injection site changes. - AccountDataMigrator performs the one-time cross-DB copy at startup, before Room opens either database. It attaches the cache (with its resolved passphrase, so an encrypted source is handled) and copies with INSERT OR IGNORE. It is crash-safe and idempotent: the source is dropped only by MIGRATION_14_15 after the copy, a re-run never duplicates or overwrites, and it runs after the clear-pending wipe so an unrecoverable cache degrades to "nothing to move" instead of blocking. - Exported schemas for both databases; MigrationTest asserts the account rows/backfills survive to v14 then are dropped at v15, plus a dedicated 14->15 test. AccountDataMigratorTest covers the plaintext + encrypted copy, idempotency, a DDL-vs-Room drift guard, and end-to-end survival of a simulated cache wipe. Closes #111 Co-Authored-By: Claude Fable 5 --- .../1.json | 234 ++++++++++++++++++ .../data/local/AccountDataMigratorTest.kt | 229 +++++++++++++++++ .../data/local/AccountDatabaseTest.kt | 85 +++++++ .../data/local/LibreMailDatabaseTest.kt | 31 +-- .../org/libremail/data/local/MigrationTest.kt | 85 +++++-- .../libremail/ui/compose/ComposeScreenTest.kt | 6 +- .../ui/settings/AccountSettingsScreenTest.kt | 4 +- .../data/local/AccountDataMigrator.kt | 193 +++++++++++++++ .../libremail/data/local/AccountDatabase.kt | 51 ++++ .../data/local/DatabaseEncryption.kt | 10 +- .../org/libremail/data/local/DatabaseFiles.kt | 7 + .../libremail/data/local/LibreMailDatabase.kt | 28 +-- .../org/libremail/data/local/Migrations.kt | 22 ++ .../org/libremail/di/AccountDatabaseModule.kt | 53 ++++ .../kotlin/org/libremail/di/DatabaseModule.kt | 32 ++- 15 files changed, 979 insertions(+), 91 deletions(-) create mode 100644 app/schemas/org.libremail.data.local.AccountDatabase/1.json create mode 100644 app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt create mode 100644 app/src/androidTest/kotlin/org/libremail/data/local/AccountDatabaseTest.kt create mode 100644 app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt create mode 100644 app/src/main/kotlin/org/libremail/data/local/AccountDatabase.kt create mode 100644 app/src/main/kotlin/org/libremail/di/AccountDatabaseModule.kt diff --git a/app/schemas/org.libremail.data.local.AccountDatabase/1.json b/app/schemas/org.libremail.data.local.AccountDatabase/1.json new file mode 100644 index 0000000..dd0f0fa --- /dev/null +++ b/app/schemas/org.libremail.data.local.AccountDatabase/1.json @@ -0,0 +1,234 @@ +{ + "formatVersion": 1, + "database": { + "version": 1, + "identityHash": "f2bbe80e572de72f50869b14aca4c4bb", + "entities": [ + { + "tableName": "accounts", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `email` TEXT NOT NULL, `displayName` TEXT NOT NULL, `authType` TEXT NOT NULL, `imap_host` TEXT NOT NULL, `imap_port` INTEGER NOT NULL, `imap_security` TEXT NOT NULL, `smtp_host` TEXT NOT NULL, `smtp_port` INTEGER NOT NULL, `smtp_security` TEXT NOT NULL, PRIMARY KEY(`id`))", + "fields": [ + { + "fieldPath": "id", + "columnName": "id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "email", + "columnName": "email", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "displayName", + "columnName": "displayName", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "authType", + "columnName": "authType", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "imap.host", + "columnName": "imap_host", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "imap.port", + "columnName": "imap_port", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "imap.security", + "columnName": "imap_security", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "smtp.host", + "columnName": "smtp_host", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "smtp.port", + "columnName": "smtp_port", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "smtp.security", + "columnName": "smtp_security", + "affinity": "TEXT", + "notNull": true + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "id" + ] + } + }, + { + "tableName": "credentials", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`accountId` TEXT NOT NULL, `encryptedSecret` TEXT NOT NULL, PRIMARY KEY(`accountId`))", + "fields": [ + { + "fieldPath": "accountId", + "columnName": "accountId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "encryptedSecret", + "columnName": "encryptedSecret", + "affinity": "TEXT", + "notNull": true + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "accountId" + ] + } + }, + { + "tableName": "account_settings", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`accountId` TEXT NOT NULL, `signature` TEXT NOT NULL, `signatureEnabled` INTEGER NOT NULL, `notificationsEnabled` INTEGER NOT NULL, `retentionCount` INTEGER, `retentionMonths` INTEGER, PRIMARY KEY(`accountId`), FOREIGN KEY(`accountId`) REFERENCES `accounts`(`id`) ON UPDATE NO ACTION ON DELETE CASCADE )", + "fields": [ + { + "fieldPath": "accountId", + "columnName": "accountId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "signature", + "columnName": "signature", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "signatureEnabled", + "columnName": "signatureEnabled", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "notificationsEnabled", + "columnName": "notificationsEnabled", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "retentionCount", + "columnName": "retentionCount", + "affinity": "INTEGER" + }, + { + "fieldPath": "retentionMonths", + "columnName": "retentionMonths", + "affinity": "INTEGER" + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "accountId" + ] + }, + "foreignKeys": [ + { + "table": "accounts", + "onDelete": "CASCADE", + "onUpdate": "NO ACTION", + "columns": [ + "accountId" + ], + "referencedColumns": [ + "id" + ] + } + ] + }, + { + "tableName": "signatures", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `accountId` TEXT NOT NULL, `name` TEXT NOT NULL, `contentHtml` TEXT NOT NULL, `isDefault` INTEGER NOT NULL, PRIMARY KEY(`id`), FOREIGN KEY(`accountId`) REFERENCES `accounts`(`id`) ON UPDATE NO ACTION ON DELETE CASCADE )", + "fields": [ + { + "fieldPath": "id", + "columnName": "id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "accountId", + "columnName": "accountId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "name", + "columnName": "name", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "contentHtml", + "columnName": "contentHtml", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "isDefault", + "columnName": "isDefault", + "affinity": "INTEGER", + "notNull": true + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "id" + ] + }, + "indices": [ + { + "name": "index_signatures_accountId", + "unique": false, + "columnNames": [ + "accountId" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_signatures_accountId` ON `${TABLE_NAME}` (`accountId`)" + } + ], + "foreignKeys": [ + { + "table": "accounts", + "onDelete": "CASCADE", + "onUpdate": "NO ACTION", + "columns": [ + "accountId" + ], + "referencedColumns": [ + "id" + ] + } + ] + } + ], + "setupQueries": [ + "CREATE TABLE IF NOT EXISTS room_master_table (id INTEGER PRIMARY KEY,identity_hash TEXT)", + "INSERT OR REPLACE INTO room_master_table (id,identity_hash) VALUES(42, 'f2bbe80e572de72f50869b14aca4c4bb')" + ] + } +} \ No newline at end of file diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt new file mode 100644 index 0000000..d610200 --- /dev/null +++ b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt @@ -0,0 +1,229 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.local + +import android.content.Context +import androidx.room.Room +import androidx.room.testing.MigrationTestHelper +import androidx.sqlite.db.framework.FrameworkSQLiteOpenHelperFactory +import androidx.test.core.app.ApplicationProvider +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.platform.app.InstrumentationRegistry +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.runBlocking +import org.json.JSONObject +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.libremail.data.local.entity.CredentialEntity + +/** + * The one-time move performed by [AccountDataMigrator] (issue #111): copying accounts / credentials / + * per-account settings / signatures out of the cache database into the plaintext [AccountDatabase]. + * + * Exercises the [AccountDataMigrator.copyAccountTables] core directly (the full [AccountDataMigrator] + * also resolves the passphrase and flips the done-flag, which need the real DataStore/Keystore). A v14 + * cache is built with [MigrationTestHelper] from the exported schema, so the copy runs against exactly + * the on-disk shape an upgrading user has. + */ +@RunWith(AndroidJUnit4::class) +class AccountDataMigratorTest { + + @get:Rule + val helper = MigrationTestHelper( + InstrumentationRegistry.getInstrumentation(), + LibreMailDatabase::class.java, + emptyList(), + FrameworkSQLiteOpenHelperFactory(), + ) + + private val context = ApplicationProvider.getApplicationContext() + private val cacheName = "acct-migrator-cache-test.db" + private val accountsName = "acct-migrator-accounts-test.db" + private val cacheFile get() = context.getDatabasePath(cacheName) + private val accountsFile get() = context.getDatabasePath(accountsName) + + // 64 hex chars == a 32-byte SQLCipher passphrase. + private val passphrase = "0123456789abcdef".repeat(4) + + @Before + @After + fun clean() { + listOf(cacheName, accountsName).forEach { name -> + context.deleteDatabase(name) + context.getDatabasePath(name).parentFile + ?.listFiles { f -> f.name.startsWith(name) } + ?.forEach { it.delete() } + } + } + + /** Builds a v14 cache holding one fully-populated account plus a mail row. */ + private fun seedVersion14Cache() { + helper.createDatabase(cacheName, 14).apply { + execSQL( + "INSERT INTO accounts (id, email, displayName, authType, imap_host, imap_port, imap_security, " + + "smtp_host, smtp_port, smtp_security) VALUES ('acct', 'ada@example.org', 'Ada', " + + "'PASSWORD_IMAP', 'imap.example.org', 993, 'SSL_TLS', 'smtp.example.org', 465, 'SSL_TLS')", + ) + execSQL("INSERT INTO credentials (accountId, encryptedSecret) VALUES ('acct', 'sealed-secret')") + execSQL( + "INSERT INTO account_settings (accountId, signature, signatureEnabled, notificationsEnabled, " + + "retentionCount, retentionMonths) VALUES ('acct', 'Cheers', 1, 0, NULL, 6)", + ) + execSQL( + "INSERT INTO signatures (id, accountId, name, contentHtml, isDefault) " + + "VALUES ('sig-1', 'acct', 'Work', '

Regards

', 1)", + ) + execSQL( + "INSERT INTO messages (id, accountId, sender, senderEmail, subject, snippet, body, isHtml, " + + "timestampMillis, isRead, isStarred, folder, inInbox, bodyFetched, uid) VALUES " + + "('acct:INBOX:1', 'acct', 'Ada', 'a@x', 'Hi', '', '', 0, 1, 0, 0, 'INBOX', 1, 0, 1)", + ) + close() + } + } + + private fun openAccountsDb(): AccountDatabase = + Room.databaseBuilder(context, AccountDatabase::class.java, accountsName).build() + + @Test + fun movesEveryAccountTableOutOfAPlaintextCache() = runBlocking { + seedVersion14Cache() + + AccountDataMigrator.copyAccountTables(cacheFile, cachePassphrase = "", accountsFile = accountsFile) + + // First open lets Room stamp its identity onto the migrator-created file; reopen so a real + // session's reads run against a fully Room-owned database. + openAccountsDb().apply { + assertEquals("Ada", accountDao().getById("acct")?.displayName) + close() + } + openAccountsDb().apply { + val account = accountDao().getById("acct") + assertEquals("ada@example.org", account?.email) + assertEquals(993, account?.imap?.port) + assertEquals("smtp.example.org", account?.smtp?.host) + assertEquals("sealed-secret", credentialDao().getById("acct")?.encryptedSecret) + val settings = accountSettingsDao().get("acct") + assertEquals(false, settings?.signatureEnabled) + assertEquals(6, settings?.retentionMonths) + assertNull(settings?.retentionCount) + val signatures = signatureDao().observeForAccount("acct").first() + assertEquals(listOf("Work"), signatures.map { it.name }) + assertTrue("the default flag must round-trip", signatures.single().isDefault) + close() + } + } + + @Test + fun movesAccountsOutOfAnEncryptedCache() = runBlocking { + seedVersion14Cache() + // Turn the cache into the SQLCipher form an app-lock + encrypted-cache user has on disk. + DatabaseEncryption.ensureEncrypted(cacheFile, passphrase) + assertTrue("precondition: the source cache is encrypted", DatabaseEncryption.isEncrypted(cacheFile)) + + AccountDataMigrator.copyAccountTables(cacheFile, cachePassphrase = passphrase, accountsFile = accountsFile) + + openAccountsDb().apply { + assertNotNull(accountDao().getById("acct")) + close() + } + openAccountsDb().apply { + assertEquals("ada@example.org", accountDao().getById("acct")?.email) + assertEquals("sealed-secret", credentialDao().getById("acct")?.encryptedSecret) + close() + } + } + + @Test + fun reRunningTheCopyIsIdempotentAndKeepsLaterEdits() = runBlocking { + seedVersion14Cache() + AccountDataMigrator.copyAccountTables(cacheFile, cachePassphrase = "", accountsFile = accountsFile) + + // Simulate the user editing an account AFTER the migration. + openAccountsDb().apply { + val edited = accountDao().getById("acct")!!.copy(displayName = "Ada Lovelace") + accountDao().upsert(edited) + close() + } + + // A re-run (e.g. after a mid-startup crash before the done-flag was set) must not clobber it. + AccountDataMigrator.copyAccountTables(cacheFile, cachePassphrase = "", accountsFile = accountsFile) + + openAccountsDb().apply { + assertEquals(1, accountDao().getAll().size) + assertEquals( + "INSERT OR IGNORE must not overwrite the post-migration edit", + "Ada Lovelace", + accountDao().getById("acct")?.displayName, + ) + close() + } + } + + @Test + fun accountsAndCredentialsSurviveACacheWipe() = runBlocking { + seedVersion14Cache() + AccountDataMigrator.copyAccountTables(cacheFile, cachePassphrase = "", accountsFile = accountsFile) + + // The "clear + re-sync" recovery wipes only the cache file; AccountDatabase is a separate file. + context.deleteDatabase(cacheName) + assertTrue("precondition: the cache file is gone", !cacheFile.exists()) + + openAccountsDb().apply { + assertNotNull("the account must outlive a cache wipe (issue #111)", accountDao().getById("acct")) + assertEquals("sealed-secret", credentialDao().getById("acct")?.encryptedSecret) + // And it is still usable: a fresh credential can be written with no cache present. + credentialDao().upsert(CredentialEntity("acct", "rotated")) + assertEquals("rotated", credentialDao().getById("acct")?.encryptedSecret) + close() + } + } + + @Test + fun migratorDdlMatchesExportedAccountDatabaseSchema() { + val schema = JSONObject( + InstrumentationRegistry.getInstrumentation().context.assets + .open("org.libremail.data.local.AccountDatabase/1.json") + .bufferedReader().use { it.readText() }, + ).getJSONObject("database") + val entities = schema.getJSONArray("entities") + + var checkedIndex = false + for (i in 0 until entities.length()) { + val entity = entities.getJSONObject(i) + val table = entity.getString("tableName") + val expectedCreate = entity.getString("createSql").replace("\${TABLE_NAME}", "`$table`") + assertEquals( + "AccountDataMigrator DDL for `$table` must match the exported AccountDatabase schema", + expectedCreate, + AccountDataMigrator.CREATE_TABLE_SQL[table], + ) + if (entity.has("indices")) { + val indices = entity.getJSONArray("indices") + for (j in 0 until indices.length()) { + val index = indices.getJSONObject(j) + if (index.getString("name") == "index_signatures_accountId") { + assertEquals( + "AccountDataMigrator signatures index must match the exported schema", + index.getString("createSql").replace("\${TABLE_NAME}", "`$table`"), + AccountDataMigrator.SIGNATURES_INDEX_SQL, + ) + checkedIndex = true + } + } + } + } + assertEquals( + "every migrator table DDL must correspond to an exported entity", + AccountDataMigrator.CREATE_TABLE_SQL.keys, + (0 until entities.length()).map { entities.getJSONObject(it).getString("tableName") }.toSet(), + ) + assertTrue("the signatures index must be present in the exported schema", checkedIndex) + } +} diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDatabaseTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDatabaseTest.kt new file mode 100644 index 0000000..8ddae89 --- /dev/null +++ b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDatabaseTest.kt @@ -0,0 +1,85 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.local + +import android.content.Context +import androidx.room.Room +import androidx.test.core.app.ApplicationProvider +import androidx.test.ext.junit.runners.AndroidJUnit4 +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.runBlocking +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.libremail.data.local.entity.AccountEntity +import org.libremail.data.local.entity.AccountSettingsEntity +import org.libremail.data.local.entity.CredentialEntity +import org.libremail.data.local.entity.ServerConfigEmbedded +import org.libremail.data.local.entity.SignatureEntity + +/** + * Behavior of the non-auth [AccountDatabase] that holds accounts, credentials, per-account settings + * and signatures after they were moved out of the cache database (issue #111). Confirms the tables' + * foreign keys still cascade from the account, now that they live together in this database. + */ +@RunWith(AndroidJUnit4::class) +class AccountDatabaseTest { + + private lateinit var db: AccountDatabase + + @Before + fun setUp() { + val context = ApplicationProvider.getApplicationContext() + db = Room.inMemoryDatabaseBuilder(context, AccountDatabase::class.java).build() + } + + @After + fun tearDown() = db.close() + + private fun account(id: String = "acct") = AccountEntity( + id = id, + email = "a@example.org", + displayName = "A", + authType = "PASSWORD_IMAP", + imap = ServerConfigEmbedded("imap.example.org", 993, "SSL_TLS"), + smtp = ServerConfigEmbedded("smtp.example.org", 465, "SSL_TLS"), + ) + + @Test + fun credentialRoundTripsAndIsIndependentOfTheAccountRow() = runBlocking { + db.accountDao().upsert(account()) + db.credentialDao().upsert(CredentialEntity("acct", "sealed-secret")) + + assertEquals("sealed-secret", db.credentialDao().getById("acct")?.encryptedSecret) + } + + @Test + fun accountSettingsRoundTripAndCascadeWithTheirAccount() = runBlocking { + db.accountDao().upsert(account()) + db.accountSettingsDao().upsert( + AccountSettingsEntity("acct", signature = "Hi", signatureEnabled = false, notificationsEnabled = false), + ) + assertEquals("Hi", db.accountSettingsDao().get("acct")?.signature) + + db.accountDao().deleteById("acct") + + assertNull("account_settings must cascade-delete with its account", db.accountSettingsDao().get("acct")) + } + + @Test + fun signaturesCascadeWithTheirAccount() = runBlocking { + db.accountDao().upsert(account()) + db.signatureDao().upsert(SignatureEntity("sig-1", "acct", "Work", "

Regards

", isDefault = true)) + assertEquals(1, db.signatureDao().observeForAccount("acct").first().size) + + db.accountDao().deleteById("acct") + + assertTrue( + "signatures must cascade-delete with their account", + db.signatureDao().observeForAccount("acct").first().isEmpty(), + ) + } +} diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/LibreMailDatabaseTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/LibreMailDatabaseTest.kt index b4ed6cb..5879ba1 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/LibreMailDatabaseTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/LibreMailDatabaseTest.kt @@ -9,22 +9,21 @@ import kotlinx.coroutines.flow.first import kotlinx.coroutines.runBlocking import org.junit.After import org.junit.Assert.assertEquals -import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test import org.junit.runner.RunWith -import org.libremail.data.local.entity.AccountEntity -import org.libremail.data.local.entity.AccountSettingsEntity import org.libremail.data.local.entity.AttachmentEntity import org.libremail.data.local.entity.FolderEntity import org.libremail.data.local.entity.MessageEntity -import org.libremail.data.local.entity.ServerConfigEmbedded /** * Schema-behavior tests on a fresh in-memory database at the current version. The migration DDL * itself is exercised by [MigrationTest], which replays the schema chain exported to app/schemas. * (Migrations from before v7 predate schema export, so they can't be replayed there.) + * + * Account/credential/settings/signature behavior moved to [AccountDatabaseTest] with those tables + * (issue #111). */ @RunWith(AndroidJUnit4::class) class LibreMailDatabaseTest { @@ -97,30 +96,6 @@ class LibreMailDatabaseTest { ) } - @Test - fun accountSettingsRoundTripAndCascadeWithTheirAccount() = runBlocking { - val accountDao = db.accountDao() - val settingsDao = db.accountSettingsDao() - accountDao.upsert( - AccountEntity( - id = "acct", - email = "a@example.org", - displayName = "A", - authType = "PASSWORD_IMAP", - imap = ServerConfigEmbedded("imap.example.org", 993, "SSL_TLS"), - smtp = ServerConfigEmbedded("smtp.example.org", 465, "SSL_TLS"), - ), - ) - settingsDao.upsert( - AccountSettingsEntity("acct", signature = "Hi", signatureEnabled = false, notificationsEnabled = false), - ) - assertEquals("Hi", settingsDao.get("acct")?.signature) - - accountDao.deleteById("acct") - - assertNull("account_settings must cascade-delete with its account", settingsDao.get("acct")) - } - @Test fun searchRowsAreNotInboxAndAreCleared() = runBlocking { val messageDao = db.messageDao() diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/MigrationTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/MigrationTest.kt index 6716829..1d669db 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/MigrationTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/MigrationTest.kt @@ -133,6 +133,9 @@ class MigrationTest { open?.close() val stepDb = helper.runMigrationsAndValidate(TEST_DB, migration.endVersion, true, migration) stepDb.writeMidChainData() + // v16 moves the account tables out to AccountDatabase and drops them, so assert their rows + // and backfills reached v15 intact — just before the move (issue #111). + if (stepDb.version == 15) stepDb.assertAccountDataPresentAtV15() open = stepDb } val db = checkNotNull(open) { "no migration starts at v$OLDEST_EXPORTED_SCHEMA" } @@ -140,6 +143,29 @@ class MigrationTest { assertEquals("the chain must end at the newest exported schema", latestExportedSchemaVersion(), db.version) db.assertVersion7CacheSurvived() db.assertMigrationBackfillsApplied() + db.assertAccountTablesDroppedAtV16() + db.close() + } + + /** v15 -> v16 (issue #111): the moved account tables are dropped and the mail cache is untouched. */ + @Test + fun migrate15To16_dropsMovedAccountTablesAndKeepsCache() { + helper.createDatabase(TEST_DB, 15).apply { + insertAccount() + execSQL("INSERT INTO credentials (accountId, encryptedSecret) VALUES ('acct', 'sealed')") + execSQL( + "INSERT INTO messages (id, accountId, sender, senderEmail, subject, snippet, body, isHtml, " + + "timestampMillis, isRead, isStarred, folder, inInbox, bodyFetched, uid) VALUES " + + "('acct:INBOX:1', 'acct', 'Ada', 'ada@example.org', 'Hi', '', '', 0, 1000, 0, 0, " + + "'INBOX', 1, 0, 1)", + ) + close() + } + + val db = helper.runMigrationsAndValidate(TEST_DB, 16, true, MIGRATION_15_16) + + db.assertAccountTablesDroppedAtV16() + assertEquals("the mail cache must be untouched by 15->16", 1, db.count("messages")) db.close() } @@ -203,9 +229,8 @@ class MigrationTest { } } - /** Every row cached at v7 must still be present and correct at the end of the chain. */ + /** Every mail-cache row cached at v7 must survive to v16 (account tables are checked separately). */ private fun SupportSQLiteDatabase.assertVersion7CacheSurvived() { - assertEquals(1, count("accounts")) assertEquals(2, count("messages")) assertEquals(1, count("outbox")) assertEquals(1, count("drafts")) @@ -215,24 +240,14 @@ class MigrationTest { assertEquals("Analytical engines", c.getString(1)) assertEquals(1, c.getInt(2)) } - query("SELECT encryptedSecret FROM credentials WHERE accountId = 'acct'").use { c -> - assertTrue("stored credentials must never be dropped by a migration", c.moveToFirst()) - assertEquals("sealed-secret", c.getString(0)) - } query("SELECT filename FROM attachments WHERE messageId = 'acct:1'").use { c -> assertTrue("attachment rows must survive the 6->7 style table rebuilds", c.moveToFirst()) assertEquals("notes.pdf", c.getString(0)) } } - /** Columns and rows created by the migrations themselves must hold their documented defaults. */ + /** Cache-table columns/rows the migrations backfill must hold their documented defaults at v16. */ private fun SupportSQLiteDatabase.assertMigrationBackfillsApplied() { - // 8->9 backfills one default settings row per existing account. - query("SELECT signatureEnabled, notificationsEnabled FROM account_settings").use { c -> - assertTrue("8->9 must backfill a settings row for the v7 account", c.moveToFirst()) - assertEquals(1, c.getInt(0)) - assertEquals(1, c.getInt(1)) - } // 9->10 adds bcc columns defaulting to ''; 10->11 adds nullable bodyHtml. query("SELECT bccAddresses, bodyHtml FROM outbox WHERE id = 'out-1'").use { c -> assertTrue(c.moveToFirst()) @@ -244,14 +259,6 @@ class MigrationTest { assertEquals("", c.getString(0)) assertTrue(c.isNull(1)) } - // 10->11 turns the signature written at v9 into that account's default rich-text signature. - query("SELECT name, contentHtml, isDefault FROM signatures WHERE accountId = 'acct'").use { c -> - assertTrue("10->11 must backfill the legacy per-account signature", c.moveToFirst()) - assertEquals("Signature", c.getString(0)) - assertEquals("Cheers,
Ada", c.getString(1)) - assertEquals(1, c.getInt(2)) - assertFalse("exactly one signature row must be backfilled", c.moveToNext()) - } // 11->12 stamps the folder cached at v8 as not special-use. query("SELECT specialUse FROM folders WHERE fullName = 'INBOX'").use { c -> assertTrue("folder cached at v8 must survive to the newest version", c.moveToFirst()) @@ -265,6 +272,42 @@ class MigrationTest { } } + /** + * The account tables' rows + migration backfills must be intact at v15, just before 15->16 moves + * them to [AccountDatabase] and drops them (issue #111). AccountDataMigrator's own copy is + * exercised in `AccountDataMigratorTest`; here we only assert the source rows reach the move point. + */ + private fun SupportSQLiteDatabase.assertAccountDataPresentAtV15() { + assertEquals(1, count("accounts")) + query("SELECT encryptedSecret FROM credentials WHERE accountId = 'acct'").use { c -> + assertTrue("stored credentials must reach v15 before the move", c.moveToFirst()) + assertEquals("sealed-secret", c.getString(0)) + } + // 8->9 backfills one default settings row per existing account. + query("SELECT signatureEnabled, notificationsEnabled FROM account_settings").use { c -> + assertTrue("8->9 must backfill a settings row for the v7 account", c.moveToFirst()) + assertEquals(1, c.getInt(0)) + assertEquals(1, c.getInt(1)) + } + // 10->11 turns the signature written at v9 into that account's default rich-text signature. + query("SELECT name, contentHtml, isDefault FROM signatures WHERE accountId = 'acct'").use { c -> + assertTrue("10->11 must backfill the legacy per-account signature", c.moveToFirst()) + assertEquals("Signature", c.getString(0)) + assertEquals("Cheers,
Ada", c.getString(1)) + assertEquals(1, c.getInt(2)) + assertFalse("exactly one signature row must be backfilled", c.moveToNext()) + } + } + + /** 15->16 drops the account tables from the cache (AccountDataMigrator copies them out first). */ + private fun SupportSQLiteDatabase.assertAccountTablesDroppedAtV16() { + listOf("accounts", "credentials", "account_settings", "signatures").forEach { table -> + query("SELECT name FROM sqlite_master WHERE type = 'table' AND name = '$table'").use { c -> + assertFalse("15->16 must drop `$table` from the cache database", c.moveToFirst()) + } + } + } + private fun SupportSQLiteDatabase.count(table: String): Int = query("SELECT COUNT(*) FROM $table").use { c -> c.moveToFirst() c.getInt(0) diff --git a/app/src/androidTest/kotlin/org/libremail/ui/compose/ComposeScreenTest.kt b/app/src/androidTest/kotlin/org/libremail/ui/compose/ComposeScreenTest.kt index 25355e6..bbd2bbf 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/compose/ComposeScreenTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/compose/ComposeScreenTest.kt @@ -26,7 +26,7 @@ import org.junit.Test import org.junit.runner.RunWith import org.libremail.R import org.libremail.contacts.ContactsRepository -import org.libremail.data.local.LibreMailDatabase +import org.libremail.data.local.AccountDatabase import org.libremail.data.settings.AccountSettingsRepository import org.libremail.data.settings.SignatureRepository import org.libremail.domain.model.Account @@ -56,7 +56,7 @@ class ComposeScreenTest { smtp = ServerConfig("smtp.example.com", 465, MailSecurity.SSL_TLS), ) - private var db: LibreMailDatabase? = null + private var db: AccountDatabase? = null private fun string(resId: Int) = composeTestRule.activity.getString(resId) @@ -79,7 +79,7 @@ class ComposeScreenTest { // Build the view model once and capture it, so recomposition doesn't recreate it. private fun setContent(mailRepository: FakeMailRepository = FakeMailRepository(), onBack: () -> Unit = {}) { val context = InstrumentationRegistry.getInstrumentation().targetContext.applicationContext - val database = Room.inMemoryDatabaseBuilder(context, LibreMailDatabase::class.java).build().also { db = it } + val database = Room.inMemoryDatabaseBuilder(context, AccountDatabase::class.java).build().also { db = it } val viewModel = ComposeViewModel( savedStateHandle = SavedStateHandle(), mailRepository = mailRepository, diff --git a/app/src/androidTest/kotlin/org/libremail/ui/settings/AccountSettingsScreenTest.kt b/app/src/androidTest/kotlin/org/libremail/ui/settings/AccountSettingsScreenTest.kt index 20e1173..85f90c4 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/settings/AccountSettingsScreenTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/settings/AccountSettingsScreenTest.kt @@ -16,7 +16,7 @@ import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import org.libremail.R -import org.libremail.data.local.LibreMailDatabase +import org.libremail.data.local.AccountDatabase import org.libremail.data.local.toEntity import org.libremail.data.settings.AccountSettingsRepository import org.libremail.data.settings.SignatureRepository @@ -59,7 +59,7 @@ class AccountSettingsScreenTest { // stateIn/WhileSubscribed) keeps querying after the test body, so closing the in-memory DB out // from under it races and crashes ("connection pool has been closed"). The DB is reclaimed with // the test process. - val db = Room.inMemoryDatabaseBuilder(context, LibreMailDatabase::class.java).build() + val db = Room.inMemoryDatabaseBuilder(context, AccountDatabase::class.java).build() val repository = AccountSettingsRepository(db.accountSettingsDao()) runBlocking { db.accountDao().upsert(account.toEntity()) // FK parent for the account_settings row diff --git a/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt b/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt new file mode 100644 index 0000000..8e5a50e --- /dev/null +++ b/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt @@ -0,0 +1,193 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.local + +import android.content.Context +import android.util.Log +import androidx.datastore.core.DataStore +import androidx.datastore.preferences.core.Preferences +import androidx.datastore.preferences.core.booleanPreferencesKey +import androidx.datastore.preferences.core.edit +import androidx.datastore.preferences.preferencesDataStore +import dagger.hilt.android.qualifiers.ApplicationContext +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.withContext +import net.zetetic.database.sqlcipher.SQLiteDatabase +import org.libremail.data.security.DatabaseKeyStore +import org.libremail.data.settings.SettingsRepository +import java.io.File +import javax.inject.Inject +import javax.inject.Singleton + +private val Context.accountMigrationDataStore: DataStore by + preferencesDataStore(name = "libremail_account_migration") + +/** + * One-time, crash-safe move of the account tables (`accounts`, `credentials`, `account_settings`, + * `signatures`) out of the auth-bound cache database [LibreMailDatabase] into the non-auth + * [AccountDatabase] (issue #111). Runs at startup, driven by `DatabaseModule.provideDatabase`, BEFORE + * Room opens the cache and its [MIGRATION_15_16] drops the moved tables. + * + * ### Why not a Room migration + * The copy is cross-database, so it needs `ATTACH DATABASE`, which SQLite forbids inside the + * transaction Room wraps every migration in. It therefore runs here on a dedicated SQLCipher + * connection before Room opens either database. + * + * ### Handling the encrypted source + * When the opt-in encrypted cache is on, the source `libremail.db` is SQLCipher-encrypted. The + * caller resolves and hands us its passphrase (the same one Room uses to open it); we attach the + * cache with that passphrase and copy into a plaintext `libremail-accounts.db`. When the cache is + * plaintext the passphrase is empty. Reading the source's schema validates the passphrase, so a + * genuinely wrong key fails loudly here (the same open would fail in Room) rather than losing data. + * + * The unrecoverable-key case does not reach us: `provideDatabase` wipes an undecryptable cache (and + * resets its seals) BEFORE calling us, so we then see a fresh/empty cache with nothing to move — the + * accounts trapped in that already-invalidated cache are lost regardless (the pre-existing bug), but + * no future invalidation can strand them again once they live in [AccountDatabase]. + * + * ### Crash-safety & idempotency + * - We never drop the source here; [MIGRATION_15_16] does that after we return, so if we crash the + * source rows are still intact for the next attempt. + * - The copy uses `INSERT OR IGNORE`, so a re-run after a mid-copy crash converges (existing rows + * are skipped, never duplicated, and never overwrite anything the user changed post-migration). + * - The "done" flag is only set after a successful copy; until then every start retries. Once set we + * return immediately and never touch the cache passphrase again — so after migration the account + * database opens with no Keystore dependency at all. + */ +@Singleton +class AccountDataMigrator @Inject constructor( + @ApplicationContext private val context: Context, + private val keyStore: DatabaseKeyStore, + private val settingsRepository: SettingsRepository, +) { + + /** + * Copy the account tables into [AccountDatabase] if it has not been done yet. Idempotent and + * safe to call from every `provideDatabase` construction. Throws (rather than silently skipping) + * on an unexpected copy failure so the caller does not proceed to drop the source tables — a + * crash-loop that preserves data is strictly safer than a wipe that loses it. + */ + suspend fun migrateIfNeeded() { + if (isDone()) return + val cacheFile = context.getDatabasePath(DatabaseFiles.NAME) + if (cacheFile.exists() && cacheFile.length() > 0L) { + // Read the cache in its CURRENT on-disk form. `provideDatabase` runs us before it converts + // between plaintext and encrypted, so the key is empty unless the file is encrypted now. + val cacheKey = if (DatabaseEncryption.isEncrypted(cacheFile)) { + keyStore.resolvePassphrase(settingsRepository.settings.first().appLock) + } else { + "" + } + val accountsFile = context.getDatabasePath(DatabaseFiles.ACCOUNTS_NAME) + withContext(Dispatchers.IO) { copyAccountTables(cacheFile, cacheKey, accountsFile) } + } + markDone() + } + + private suspend fun isDone(): Boolean = context.accountMigrationDataStore.data.first()[DONE] == true + + private suspend fun markDone() { + context.accountMigrationDataStore.edit { it[DONE] = true } + } + + companion object { + private const val TAG = "LibreMailAcctMigrate" + private val DONE = booleanPreferencesKey("accounts_moved_out_of_cache") + + /** The account tables, parent before children so foreign keys never block an insert. */ + private val TABLES = listOf("accounts", "credentials", "account_settings", "signatures") + + /** + * DDL for the account tables in [AccountDatabase] v1, copied verbatim from the exported Room + * schema (`schemas/org.libremail.data.local.AccountDatabase/1.json`). It MUST stay byte-for-byte + * identical to what Room generates for those entities, or Room silently accepts a subtly wrong + * schema (its identity check only compares the hash it writes, not the pre-existing tables). + * `AccountDataMigratorTest.migratorDdlMatchesExportedAccountDatabaseSchema` guards it against the + * exported schema; `internal` only so that test can read it. + */ + internal val CREATE_TABLE_SQL = mapOf( + "accounts" to + "CREATE TABLE IF NOT EXISTS `accounts` (`id` TEXT NOT NULL, `email` TEXT NOT NULL, " + + "`displayName` TEXT NOT NULL, `authType` TEXT NOT NULL, `imap_host` TEXT NOT NULL, " + + "`imap_port` INTEGER NOT NULL, `imap_security` TEXT NOT NULL, `smtp_host` TEXT NOT NULL, " + + "`smtp_port` INTEGER NOT NULL, `smtp_security` TEXT NOT NULL, PRIMARY KEY(`id`))", + "credentials" to + "CREATE TABLE IF NOT EXISTS `credentials` (`accountId` TEXT NOT NULL, " + + "`encryptedSecret` TEXT NOT NULL, PRIMARY KEY(`accountId`))", + "account_settings" to + "CREATE TABLE IF NOT EXISTS `account_settings` (`accountId` TEXT NOT NULL, " + + "`signature` TEXT NOT NULL, `signatureEnabled` INTEGER NOT NULL, " + + "`notificationsEnabled` INTEGER NOT NULL, `retentionCount` INTEGER, " + + "`retentionMonths` INTEGER, PRIMARY KEY(`accountId`), " + + "FOREIGN KEY(`accountId`) REFERENCES `accounts`(`id`) " + + "ON UPDATE NO ACTION ON DELETE CASCADE )", + "signatures" to + "CREATE TABLE IF NOT EXISTS `signatures` (`id` TEXT NOT NULL, `accountId` TEXT NOT NULL, " + + "`name` TEXT NOT NULL, `contentHtml` TEXT NOT NULL, `isDefault` INTEGER NOT NULL, " + + "PRIMARY KEY(`id`), FOREIGN KEY(`accountId`) REFERENCES `accounts`(`id`) " + + "ON UPDATE NO ACTION ON DELETE CASCADE )", + ) + + internal const val SIGNATURES_INDEX_SQL = + "CREATE INDEX IF NOT EXISTS `index_signatures_accountId` ON `signatures` (`accountId`)" + + /** + * Copies the account tables from [cacheFile] (opened with [cachePassphrase]; empty = plaintext) + * into a plaintext [accountsFile], creating the destination schema first. Opens the destination + * as `main` and attaches the (possibly encrypted) cache as `cache`, so a plaintext connection + * can still read the encrypted source via SQLCipher's per-attach key. Visible for the migrator + * test; call [migrateIfNeeded] in production. + */ + internal fun copyAccountTables(cacheFile: File, cachePassphrase: String, accountsFile: File) { + DatabaseEncryption.ensureNativeLibraryLoaded() + val db = SQLiteDatabase.openOrCreateDatabase( + accountsFile.absolutePath, + "".toByteArray(Charsets.US_ASCII), // destination is plaintext + null, + null, + ) + try { + // No WAL: keep the destination in rollback-journal mode (as DatabaseEncryption does) + // so that after close there is no -wal/-shm holding uncommitted rows for Room to miss. + db.rawExecSQL("PRAGMA journal_mode = DELETE;") + val keyLiteral = cachePassphrase.replace("'", "''") + val cachePath = cacheFile.absolutePath.replace("'", "''") + db.rawExecSQL("ATTACH DATABASE '$cachePath' AS cache KEY '$keyLiteral';") + try { + val present = presentTables(db) + if (present.isEmpty()) return // fresh cache or already dropped: nothing to move + TABLES.forEach { db.rawExecSQL(CREATE_TABLE_SQL.getValue(it)) } + db.rawExecSQL(SIGNATURES_INDEX_SQL) + // Parent first so an enforced foreign key (Room enables them; this raw connection + // does not) would still be satisfied. INSERT OR IGNORE makes each copy idempotent. + TABLES.filter { it in present }.forEach { table -> + db.rawExecSQL("INSERT OR IGNORE INTO `$table` SELECT * FROM cache.`$table`") + } + Log.d(TAG, "moved account tables into the account database: $present") + } finally { + db.rawExecSQL("DETACH DATABASE cache;") + } + } finally { + db.close() + } + // Room opens the destination next; drop any sidecars the copy left so a stale WAL/SHM can't + // confuse its first open. + val dir = accountsFile.parentFile + if (dir != null) { + listOf("-wal", "-shm", "-journal").forEach { File(dir, accountsFile.name + it).delete() } + } + } + + private fun presentTables(db: SQLiteDatabase): Set { + val names = TABLES.joinToString(",") { "'$it'" } + val present = mutableSetOf() + db.rawQuery( + "SELECT name FROM cache.sqlite_master WHERE type = 'table' AND name IN ($names)", + null, + ).use { cursor -> + while (cursor.moveToNext()) present += cursor.getString(0) + } + return present + } + } +} diff --git a/app/src/main/kotlin/org/libremail/data/local/AccountDatabase.kt b/app/src/main/kotlin/org/libremail/data/local/AccountDatabase.kt new file mode 100644 index 0000000..cdf4e47 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/data/local/AccountDatabase.kt @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.local + +import androidx.room.Database +import androidx.room.RoomDatabase +import org.libremail.data.local.dao.AccountDao +import org.libremail.data.local.dao.AccountSettingsDao +import org.libremail.data.local.dao.CredentialDao +import org.libremail.data.local.dao.SignatureDao +import org.libremail.data.local.entity.AccountEntity +import org.libremail.data.local.entity.AccountSettingsEntity +import org.libremail.data.local.entity.CredentialEntity +import org.libremail.data.local.entity.SignatureEntity + +/** + * Durable store for the pieces of an account that must survive a mail-cache wipe (issue #111): the + * account itself, its sealed credential, per-account settings, and saved signatures. + * + * This lives in its OWN database file ([DatabaseFiles.ACCOUNTS_NAME]) that is deliberately NEVER + * bound to the auth-bound SQLCipher key. When app-lock + encrypted-cache are on and that key is + * invalidated (a genuine biometric re-enrollment or lock removal/re-add), only the mail cache + * ([LibreMailDatabase]) becomes undecryptable and is wiped; this database is untouched, so the user + * stays signed in instead of being dropped back into onboarding. + * + * It is plaintext on disk. The only secret it holds is [CredentialEntity.encryptedSecret], which is + * already AES-GCM ciphertext sealed at the column level by the non-auth + * [org.libremail.data.security.KeystoreCrypto] master key (and that key survives an auth-key + * invalidation), so the secret never touches disk in the clear regardless of this file's own + * encryption. Account metadata (email address, server hosts) is not a secret. Keeping the file + * plaintext is what makes it maximally resilient — it can always be opened without any Keystore key, + * so no key invalidation can ever strand it. + * + * Existing installs are migrated into this database once, at startup, by [AccountDataMigrator] + * before [MIGRATION_15_16] drops the moved tables from the cache database. + */ +@Database( + entities = [ + AccountEntity::class, + CredentialEntity::class, + AccountSettingsEntity::class, + SignatureEntity::class, + ], + version = 1, + exportSchema = true, +) +abstract class AccountDatabase : RoomDatabase() { + abstract fun accountDao(): AccountDao + abstract fun credentialDao(): CredentialDao + abstract fun accountSettingsDao(): AccountSettingsDao + abstract fun signatureDao(): SignatureDao +} diff --git a/app/src/main/kotlin/org/libremail/data/local/DatabaseEncryption.kt b/app/src/main/kotlin/org/libremail/data/local/DatabaseEncryption.kt index 515944c..6694a2c 100644 --- a/app/src/main/kotlin/org/libremail/data/local/DatabaseEncryption.kt +++ b/app/src/main/kotlin/org/libremail/data/local/DatabaseEncryption.kt @@ -40,7 +40,7 @@ object DatabaseEncryption { * tables but not that pragma, and a reset version would make Room attempt a bogus migration. */ private fun migrate(dbFile: File, sourcePassphrase: String, targetPassphrase: String) { - ensureLibraryLoaded() + ensureNativeLibraryLoaded() val dir = dbFile.parentFile ?: error("database file has no parent directory") val tmp = File(dir, dbFile.name + ".migrate").apply { delete() } @@ -98,7 +98,13 @@ object DatabaseEncryption { } @Volatile private var libraryLoaded = false - private fun ensureLibraryLoaded() { + + /** + * Load SQLCipher's native library once. Public so other startup helpers that open a database via + * [net.zetetic.database.sqlcipher.SQLiteDatabase] before Room does (e.g. [AccountDataMigrator]) + * can guarantee it is loaded first. + */ + fun ensureNativeLibraryLoaded() { if (libraryLoaded) return synchronized(this) { if (!libraryLoaded) { diff --git a/app/src/main/kotlin/org/libremail/data/local/DatabaseFiles.kt b/app/src/main/kotlin/org/libremail/data/local/DatabaseFiles.kt index 0b18f42..4e52837 100644 --- a/app/src/main/kotlin/org/libremail/data/local/DatabaseFiles.kt +++ b/app/src/main/kotlin/org/libremail/data/local/DatabaseFiles.kt @@ -9,6 +9,13 @@ object DatabaseFiles { const val NAME = "libremail.db" + /** + * The [org.libremail.data.local.AccountDatabase] file — accounts, credentials, per-account + * settings and signatures. Deliberately a DIFFERENT file from [NAME] and NEVER wiped by [clear], + * so a cache-key invalidation keeps the user signed in (issue #111). + */ + const val ACCOUNTS_NAME = "libremail-accounts.db" + /** * Delete the database and any WAL/SHM/journal sidecars. Call only when no connection is open — * used by the "clear + re-sync" path when the encryption key is invalidated and the encrypted diff --git a/app/src/main/kotlin/org/libremail/data/local/LibreMailDatabase.kt b/app/src/main/kotlin/org/libremail/data/local/LibreMailDatabase.kt index 24bf856..567ad08 100644 --- a/app/src/main/kotlin/org/libremail/data/local/LibreMailDatabase.kt +++ b/app/src/main/kotlin/org/libremail/data/local/LibreMailDatabase.kt @@ -3,52 +3,46 @@ package org.libremail.data.local import androidx.room.Database import androidx.room.RoomDatabase -import org.libremail.data.local.dao.AccountDao -import org.libremail.data.local.dao.AccountSettingsDao import org.libremail.data.local.dao.AttachmentDao import org.libremail.data.local.dao.BackfillProgressDao -import org.libremail.data.local.dao.CredentialDao import org.libremail.data.local.dao.DraftDao import org.libremail.data.local.dao.FolderDao import org.libremail.data.local.dao.MessageDao import org.libremail.data.local.dao.OutboxDao -import org.libremail.data.local.dao.SignatureDao -import org.libremail.data.local.entity.AccountEntity -import org.libremail.data.local.entity.AccountSettingsEntity import org.libremail.data.local.entity.AttachmentEntity import org.libremail.data.local.entity.BackfillProgressEntity -import org.libremail.data.local.entity.CredentialEntity import org.libremail.data.local.entity.DraftEntity import org.libremail.data.local.entity.FolderEntity import org.libremail.data.local.entity.MessageEntity import org.libremail.data.local.entity.OutboxEntity -import org.libremail.data.local.entity.SignatureEntity +/** + * The offline mail cache. Everything here is re-derivable from the server on a fresh sync, so it is + * the database that opt-in SQLCipher encryption is applied to and — when the auth-bound key is + * invalidated — the one that "clear + re-sync" wipes. + * + * Account identity and user configuration (accounts, credentials, per-account settings, signatures) + * are deliberately NOT here: they live in [AccountDatabase], a separate non-auth-bound file, so a + * cache-key invalidation can never sign the user out (issue #111). [MIGRATION_15_16] dropped those + * tables from this database; [AccountDataMigrator] copies existing rows into [AccountDatabase] first. + */ @Database( entities = [ - AccountEntity::class, - AccountSettingsEntity::class, MessageEntity::class, - CredentialEntity::class, AttachmentEntity::class, OutboxEntity::class, DraftEntity::class, FolderEntity::class, - SignatureEntity::class, BackfillProgressEntity::class, ], - version = 15, + version = 16, exportSchema = true, ) abstract class LibreMailDatabase : RoomDatabase() { abstract fun messageDao(): MessageDao - abstract fun accountDao(): AccountDao - abstract fun accountSettingsDao(): AccountSettingsDao - abstract fun credentialDao(): CredentialDao abstract fun attachmentDao(): AttachmentDao abstract fun outboxDao(): OutboxDao abstract fun draftDao(): DraftDao abstract fun folderDao(): FolderDao - abstract fun signatureDao(): SignatureDao abstract fun backfillProgressDao(): BackfillProgressDao } diff --git a/app/src/main/kotlin/org/libremail/data/local/Migrations.kt b/app/src/main/kotlin/org/libremail/data/local/Migrations.kt index 1b3a5f5..feadf8a 100644 --- a/app/src/main/kotlin/org/libremail/data/local/Migrations.kt +++ b/app/src/main/kotlin/org/libremail/data/local/Migrations.kt @@ -308,3 +308,25 @@ val MIGRATION_14_15 = object : Migration(14, 15) { db.execSQL("ALTER TABLE `folders` ADD COLUMN `hierarchyDelimiter` TEXT") } } + +/** + * v15 -> v16: move account identity + configuration OUT of the cache database (issue #111). The + * `accounts`, `credentials`, `account_settings` and `signatures` tables now live in [AccountDatabase] + * — a separate file that is never sealed by the auth-bound SQLCipher key — so a cache-key invalidation + * (biometric re-enrollment / lock removal) wipes only mail and can no longer sign the user out. + * + * The rows are copied into [AccountDatabase] by [AccountDataMigrator] at startup BEFORE Room opens the + * cache and runs this migration. The copy CANNOT happen here: Room wraps each migration in a + * transaction and SQLite forbids `ATTACH DATABASE` inside one, so a cross-database copy has to run on + * a separate connection before the cache is opened. This migration therefore only drops the tables + * that were moved. `DROP TABLE IF EXISTS` keeps it idempotent, and children (foreign-keyed to + * `accounts`) are dropped before the parent so the drop never trips a foreign-key check. + */ +val MIGRATION_15_16 = object : Migration(15, 16) { + override fun migrate(db: SupportSQLiteDatabase) { + db.execSQL("DROP TABLE IF EXISTS `signatures`") + db.execSQL("DROP TABLE IF EXISTS `account_settings`") + db.execSQL("DROP TABLE IF EXISTS `credentials`") + db.execSQL("DROP TABLE IF EXISTS `accounts`") + } +} diff --git a/app/src/main/kotlin/org/libremail/di/AccountDatabaseModule.kt b/app/src/main/kotlin/org/libremail/di/AccountDatabaseModule.kt new file mode 100644 index 0000000..435f929 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/di/AccountDatabaseModule.kt @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.di + +import android.content.Context +import androidx.room.Room +import dagger.Module +import dagger.Provides +import dagger.hilt.InstallIn +import dagger.hilt.android.qualifiers.ApplicationContext +import dagger.hilt.components.SingletonComponent +import org.libremail.data.local.AccountDatabase +import org.libremail.data.local.DatabaseFiles.ACCOUNTS_NAME +import org.libremail.data.local.LibreMailDatabase +import org.libremail.data.local.dao.AccountDao +import org.libremail.data.local.dao.AccountSettingsDao +import org.libremail.data.local.dao.CredentialDao +import org.libremail.data.local.dao.SignatureDao +import javax.inject.Singleton + +/** + * Hilt wiring for [AccountDatabase] — the non-auth-bound store for accounts, credentials, per-account + * settings and signatures (issue #111). Kept separate from [DatabaseModule] so each database's + * provides stay cohesive (and neither module grows past detekt's per-object function limit). + */ +@Module +@InstallIn(SingletonComponent::class) +object AccountDatabaseModule { + + /** + * The plaintext account store. Depends on [LibreMailDatabase] purely for construction ordering: + * building the cache runs the one-time [org.libremail.data.local.AccountDataMigrator] (which + * populates this file on a dedicated connection) and then drops the moved tables, so by the time + * Room opens this file the data is already present and no other connection is touching it. + */ + @Provides + @Singleton + fun provideAccountDatabase( + @ApplicationContext context: Context, + @Suppress("UNUSED_PARAMETER") cacheDatabase: LibreMailDatabase, + ): AccountDatabase = Room.databaseBuilder(context, AccountDatabase::class.java, ACCOUNTS_NAME).build() + + @Provides + fun provideAccountDao(database: AccountDatabase): AccountDao = database.accountDao() + + @Provides + fun provideCredentialDao(database: AccountDatabase): CredentialDao = database.credentialDao() + + @Provides + fun provideAccountSettingsDao(database: AccountDatabase): AccountSettingsDao = database.accountSettingsDao() + + @Provides + fun provideSignatureDao(database: AccountDatabase): SignatureDao = database.signatureDao() +} diff --git a/app/src/main/kotlin/org/libremail/di/DatabaseModule.kt b/app/src/main/kotlin/org/libremail/di/DatabaseModule.kt index 792e3ef..d247ad1 100644 --- a/app/src/main/kotlin/org/libremail/di/DatabaseModule.kt +++ b/app/src/main/kotlin/org/libremail/di/DatabaseModule.kt @@ -11,6 +11,7 @@ import dagger.hilt.components.SingletonComponent import kotlinx.coroutines.flow.first import kotlinx.coroutines.runBlocking import net.zetetic.database.sqlcipher.SupportOpenHelperFactory +import org.libremail.data.local.AccountDataMigrator import org.libremail.data.local.DatabaseEncryption import org.libremail.data.local.DatabaseFiles import org.libremail.data.local.LibreMailDatabase @@ -19,6 +20,7 @@ import org.libremail.data.local.MIGRATION_11_12 import org.libremail.data.local.MIGRATION_12_13 import org.libremail.data.local.MIGRATION_13_14 import org.libremail.data.local.MIGRATION_14_15 +import org.libremail.data.local.MIGRATION_15_16 import org.libremail.data.local.MIGRATION_1_2 import org.libremail.data.local.MIGRATION_2_3 import org.libremail.data.local.MIGRATION_3_4 @@ -28,16 +30,12 @@ import org.libremail.data.local.MIGRATION_6_7 import org.libremail.data.local.MIGRATION_7_8 import org.libremail.data.local.MIGRATION_8_9 import org.libremail.data.local.MIGRATION_9_10 -import org.libremail.data.local.dao.AccountDao -import org.libremail.data.local.dao.AccountSettingsDao import org.libremail.data.local.dao.AttachmentDao import org.libremail.data.local.dao.BackfillProgressDao -import org.libremail.data.local.dao.CredentialDao import org.libremail.data.local.dao.DraftDao import org.libremail.data.local.dao.FolderDao import org.libremail.data.local.dao.MessageDao import org.libremail.data.local.dao.OutboxDao -import org.libremail.data.local.dao.SignatureDao import org.libremail.data.security.DatabaseKeyStore import org.libremail.data.settings.SettingsRepository import javax.inject.Singleton @@ -52,6 +50,7 @@ object DatabaseModule { @ApplicationContext context: Context, keyStore: DatabaseKeyStore, settingsRepository: SettingsRepository, + accountDataMigrator: AccountDataMigrator, ): LibreMailDatabase { val builder = Room.databaseBuilder(context, LibreMailDatabase::class.java, DB_NAME) .addMigrations( @@ -69,10 +68,11 @@ object DatabaseModule { MIGRATION_12_13, MIGRATION_13_14, MIGRATION_14_15, + MIGRATION_15_16, ) // No destructive fallback: the migration chain is complete, and silently dropping the - // accounts/credentials/mail tables would lose stored secrets. A missing migration should - // fail loudly in testing instead. + // mail/message tables would lose cached data. A missing migration should fail loudly in + // testing instead. // Opt-in at-rest encryption of the local cache (off by default). The conversion runs here — // before the database is opened — so it never races an open connection; toggling the setting @@ -92,6 +92,8 @@ object DatabaseModule { // restarts the app; we wipe the cache HERE — at cold start, before Room opens — so the file is // never deleted from under an open connection. Crash-safe order: wipe + reset the seals, and // only THEN clear the flag, so a kill mid-wipe just repeats the idempotent wipe next start. + // Only libremail.db is wiped: accounts/credentials live in AccountDatabase (a separate file), + // so the user stays signed in across the wipe (issue #111). if (runBlocking { keyStore.isClearPending() }) { DatabaseFiles.clear(context) runBlocking { @@ -100,6 +102,12 @@ object DatabaseModule { } } + // One-time move of accounts/credentials/settings/signatures into the non-auth AccountDatabase + // (issue #111). MUST run before builder.build() below: opening the cache applies MIGRATION_15_16, + // which drops the moved tables. It runs AFTER the wipe above so an unrecoverable-key cache is + // gone first (nothing left to move) and we never block waiting on a passphrase we can't get. + runBlocking { accountDataMigrator.migrateIfNeeded() } + val settings = runBlocking { settingsRepository.settings.first() } val appLock = settings.appLock if (settings.encryptCache) { @@ -119,15 +127,6 @@ object DatabaseModule { @Provides fun provideMessageDao(database: LibreMailDatabase): MessageDao = database.messageDao() - @Provides - fun provideAccountDao(database: LibreMailDatabase): AccountDao = database.accountDao() - - @Provides - fun provideAccountSettingsDao(database: LibreMailDatabase): AccountSettingsDao = database.accountSettingsDao() - - @Provides - fun provideCredentialDao(database: LibreMailDatabase): CredentialDao = database.credentialDao() - @Provides fun provideAttachmentDao(database: LibreMailDatabase): AttachmentDao = database.attachmentDao() @@ -140,9 +139,6 @@ object DatabaseModule { @Provides fun provideFolderDao(database: LibreMailDatabase): FolderDao = database.folderDao() - @Provides - fun provideSignatureDao(database: LibreMailDatabase): SignatureDao = database.signatureDao() - @Provides fun provideBackfillProgressDao(database: LibreMailDatabase): BackfillProgressDao = database.backfillProgressDao() From d9d50f190356f77fc1dfa4a32480ad3e3f93900d Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 06:37:07 -0500 Subject: [PATCH 3/6] fix(test): make instrumented migrator tests return Unit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `@Test fun x() = runBlocking { ... }` whose block ends in `.apply { }` returns the DB (non-Unit), so JUnit4 rejects the whole class at runtime with InvalidTestClassError ("method should be void") — which compiles fine locally but fails every E2E job on the emulator. Use `runBlocking` (the existing DatabaseEncryptionTest idiom) so the methods are void while keeping the expression body ktlint expects. Co-Authored-By: Claude Fable 5 --- .../org/libremail/data/local/AccountDataMigratorTest.kt | 8 ++++---- .../org/libremail/data/local/AccountDatabaseTest.kt | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt index d610200..4eaf071 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt @@ -92,7 +92,7 @@ class AccountDataMigratorTest { Room.databaseBuilder(context, AccountDatabase::class.java, accountsName).build() @Test - fun movesEveryAccountTableOutOfAPlaintextCache() = runBlocking { + fun movesEveryAccountTableOutOfAPlaintextCache() = runBlocking { seedVersion14Cache() AccountDataMigrator.copyAccountTables(cacheFile, cachePassphrase = "", accountsFile = accountsFile) @@ -121,7 +121,7 @@ class AccountDataMigratorTest { } @Test - fun movesAccountsOutOfAnEncryptedCache() = runBlocking { + fun movesAccountsOutOfAnEncryptedCache() = runBlocking { seedVersion14Cache() // Turn the cache into the SQLCipher form an app-lock + encrypted-cache user has on disk. DatabaseEncryption.ensureEncrypted(cacheFile, passphrase) @@ -141,7 +141,7 @@ class AccountDataMigratorTest { } @Test - fun reRunningTheCopyIsIdempotentAndKeepsLaterEdits() = runBlocking { + fun reRunningTheCopyIsIdempotentAndKeepsLaterEdits() = runBlocking { seedVersion14Cache() AccountDataMigrator.copyAccountTables(cacheFile, cachePassphrase = "", accountsFile = accountsFile) @@ -167,7 +167,7 @@ class AccountDataMigratorTest { } @Test - fun accountsAndCredentialsSurviveACacheWipe() = runBlocking { + fun accountsAndCredentialsSurviveACacheWipe() = runBlocking { seedVersion14Cache() AccountDataMigrator.copyAccountTables(cacheFile, cachePassphrase = "", accountsFile = accountsFile) diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDatabaseTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDatabaseTest.kt index 8ddae89..c8131c5 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDatabaseTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDatabaseTest.kt @@ -49,7 +49,7 @@ class AccountDatabaseTest { ) @Test - fun credentialRoundTripsAndIsIndependentOfTheAccountRow() = runBlocking { + fun credentialRoundTripsAndIsIndependentOfTheAccountRow() = runBlocking { db.accountDao().upsert(account()) db.credentialDao().upsert(CredentialEntity("acct", "sealed-secret")) @@ -57,7 +57,7 @@ class AccountDatabaseTest { } @Test - fun accountSettingsRoundTripAndCascadeWithTheirAccount() = runBlocking { + fun accountSettingsRoundTripAndCascadeWithTheirAccount() = runBlocking { db.accountDao().upsert(account()) db.accountSettingsDao().upsert( AccountSettingsEntity("acct", signature = "Hi", signatureEnabled = false, notificationsEnabled = false), @@ -70,7 +70,7 @@ class AccountDatabaseTest { } @Test - fun signaturesCascadeWithTheirAccount() = runBlocking { + fun signaturesCascadeWithTheirAccount() = runBlocking { db.accountDao().upsert(account()) db.signatureDao().upsert(SignatureEntity("sig-1", "acct", "Work", "

Regards

", isDefault = true)) assertEquals(1, db.signatureDao().observeForAccount("acct").first().size) From 7529a8fa7d2e0079a44a7534d8b7c3c994ac8ac2 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 07:06:23 -0500 Subject: [PATCH 4/6] fix(test): correct AccountDataMigratorTest assertions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two on-device assertion failures (green on JVM compile, red on the emulator): - migratorDdlMatchesExportedAccountDatabaseSchema built its expected DDL by substituting the schema's `${TABLE_NAME}` placeholder with a backtick-wrapped name, but the exported createSql already wraps the placeholder in backticks — producing a double-backticked identifier that never matched the (correct, single-backticked) migrator DDL. Substitute the bare name so the guard compares like-for-like. - movesEveryAccountTableOutOfAPlaintextCache asserted signatureEnabled was false, but the seed row sets it to 1 (true). Assert the seeded values for both booleans so a true and a false each round-trip. The production migrator DDL and drop logic were already correct; only the tests were wrong. Co-Authored-By: Claude Fable 5 --- .../org/libremail/data/local/AccountDataMigratorTest.kt | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt index 4eaf071..66e37d1 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt @@ -110,7 +110,9 @@ class AccountDataMigratorTest { assertEquals("smtp.example.org", account?.smtp?.host) assertEquals("sealed-secret", credentialDao().getById("acct")?.encryptedSecret) val settings = accountSettingsDao().get("acct") - assertEquals(false, settings?.signatureEnabled) + // Seeded signatureEnabled = 1, notificationsEnabled = 0: both booleans must round-trip. + assertEquals(true, settings?.signatureEnabled) + assertEquals(false, settings?.notificationsEnabled) assertEquals(6, settings?.retentionMonths) assertNull(settings?.retentionCount) val signatures = signatureDao().observeForAccount("acct").first() @@ -198,7 +200,7 @@ class AccountDataMigratorTest { for (i in 0 until entities.length()) { val entity = entities.getJSONObject(i) val table = entity.getString("tableName") - val expectedCreate = entity.getString("createSql").replace("\${TABLE_NAME}", "`$table`") + val expectedCreate = entity.getString("createSql").replace("\${TABLE_NAME}", table) assertEquals( "AccountDataMigrator DDL for `$table` must match the exported AccountDatabase schema", expectedCreate, @@ -211,7 +213,7 @@ class AccountDataMigratorTest { if (index.getString("name") == "index_signatures_accountId") { assertEquals( "AccountDataMigrator signatures index must match the exported schema", - index.getString("createSql").replace("\${TABLE_NAME}", "`$table`"), + index.getString("createSql").replace("\${TABLE_NAME}", table), AccountDataMigrator.SIGNATURES_INDEX_SQL, ) checkedIndex = true From ec5e3088c0adc029aea59837d8a7c4f79f635d9c Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 08:27:53 -0500 Subject: [PATCH 5/6] chore(schema): export v16 cache schema after rebase on main Main advanced to @Database v15 (the #66 folder hierarchyDelimiter migration). Renumbered the account-tables-drop migration 14->15 to 15->16 and bumped the cache DB to v16; this exports the v16 schema (main's v15 delimiter schema minus the moved account tables). Main's own 15.json is kept unchanged. Co-Authored-By: Claude Fable 5 --- .../16.json | 455 ++++++++++++++++++ 1 file changed, 455 insertions(+) create mode 100644 app/schemas/org.libremail.data.local.LibreMailDatabase/16.json diff --git a/app/schemas/org.libremail.data.local.LibreMailDatabase/16.json b/app/schemas/org.libremail.data.local.LibreMailDatabase/16.json new file mode 100644 index 0000000..c88d55d --- /dev/null +++ b/app/schemas/org.libremail.data.local.LibreMailDatabase/16.json @@ -0,0 +1,455 @@ +{ + "formatVersion": 1, + "database": { + "version": 16, + "identityHash": "b5c1a38d197cf1335d3092e413d55d0d", + "entities": [ + { + "tableName": "messages", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `accountId` TEXT NOT NULL, `sender` TEXT NOT NULL, `senderEmail` TEXT NOT NULL, `subject` TEXT NOT NULL, `snippet` TEXT NOT NULL, `body` TEXT NOT NULL, `isHtml` INTEGER NOT NULL, `timestampMillis` INTEGER NOT NULL, `isRead` INTEGER NOT NULL, `isStarred` INTEGER NOT NULL, `folder` TEXT NOT NULL DEFAULT 'INBOX', `inInbox` INTEGER NOT NULL, `bodyFetched` INTEGER NOT NULL, `uid` INTEGER NOT NULL DEFAULT 0, PRIMARY KEY(`id`))", + "fields": [ + { + "fieldPath": "id", + "columnName": "id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "accountId", + "columnName": "accountId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "sender", + "columnName": "sender", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "senderEmail", + "columnName": "senderEmail", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "subject", + "columnName": "subject", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "snippet", + "columnName": "snippet", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "body", + "columnName": "body", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "isHtml", + "columnName": "isHtml", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "timestampMillis", + "columnName": "timestampMillis", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "isRead", + "columnName": "isRead", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "isStarred", + "columnName": "isStarred", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "folder", + "columnName": "folder", + "affinity": "TEXT", + "notNull": true, + "defaultValue": "'INBOX'" + }, + { + "fieldPath": "inInbox", + "columnName": "inInbox", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "bodyFetched", + "columnName": "bodyFetched", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "uid", + "columnName": "uid", + "affinity": "INTEGER", + "notNull": true, + "defaultValue": "0" + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "id" + ] + }, + "indices": [ + { + "name": "index_messages_accountId", + "unique": false, + "columnNames": [ + "accountId" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_messages_accountId` ON `${TABLE_NAME}` (`accountId`)" + }, + { + "name": "index_messages_timestampMillis", + "unique": false, + "columnNames": [ + "timestampMillis" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_messages_timestampMillis` ON `${TABLE_NAME}` (`timestampMillis`)" + }, + { + "name": "index_messages_accountId_folder_uid", + "unique": false, + "columnNames": [ + "accountId", + "folder", + "uid" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_messages_accountId_folder_uid` ON `${TABLE_NAME}` (`accountId`, `folder`, `uid`)" + } + ] + }, + { + "tableName": "attachments", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`messageId` TEXT NOT NULL, `partIndex` INTEGER NOT NULL, `filename` TEXT NOT NULL, `mimeType` TEXT NOT NULL, `sizeBytes` INTEGER NOT NULL, PRIMARY KEY(`messageId`, `partIndex`), FOREIGN KEY(`messageId`) REFERENCES `messages`(`id`) ON UPDATE NO ACTION ON DELETE CASCADE )", + "fields": [ + { + "fieldPath": "messageId", + "columnName": "messageId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "partIndex", + "columnName": "partIndex", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "filename", + "columnName": "filename", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "mimeType", + "columnName": "mimeType", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "sizeBytes", + "columnName": "sizeBytes", + "affinity": "INTEGER", + "notNull": true + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "messageId", + "partIndex" + ] + }, + "indices": [ + { + "name": "index_attachments_messageId", + "unique": false, + "columnNames": [ + "messageId" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_attachments_messageId` ON `${TABLE_NAME}` (`messageId`)" + } + ], + "foreignKeys": [ + { + "table": "messages", + "onDelete": "CASCADE", + "onUpdate": "NO ACTION", + "columns": [ + "messageId" + ], + "referencedColumns": [ + "id" + ] + } + ] + }, + { + "tableName": "outbox", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `accountId` TEXT NOT NULL, `toAddresses` TEXT NOT NULL, `ccAddresses` TEXT NOT NULL, `bccAddresses` TEXT NOT NULL DEFAULT '', `subject` TEXT NOT NULL, `body` TEXT NOT NULL, `createdAt` INTEGER NOT NULL, `lastError` TEXT, `bodyHtml` TEXT, PRIMARY KEY(`id`))", + "fields": [ + { + "fieldPath": "id", + "columnName": "id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "accountId", + "columnName": "accountId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "toAddresses", + "columnName": "toAddresses", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "ccAddresses", + "columnName": "ccAddresses", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "bccAddresses", + "columnName": "bccAddresses", + "affinity": "TEXT", + "notNull": true, + "defaultValue": "''" + }, + { + "fieldPath": "subject", + "columnName": "subject", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "body", + "columnName": "body", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "createdAt", + "columnName": "createdAt", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "lastError", + "columnName": "lastError", + "affinity": "TEXT" + }, + { + "fieldPath": "bodyHtml", + "columnName": "bodyHtml", + "affinity": "TEXT" + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "id" + ] + } + }, + { + "tableName": "drafts", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `accountId` TEXT, `toAddresses` TEXT NOT NULL, `ccAddresses` TEXT NOT NULL, `bccAddresses` TEXT NOT NULL DEFAULT '', `subject` TEXT NOT NULL, `body` TEXT NOT NULL, `updatedAt` INTEGER NOT NULL, `attachments` TEXT NOT NULL, `bodyHtml` TEXT, PRIMARY KEY(`id`))", + "fields": [ + { + "fieldPath": "id", + "columnName": "id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "accountId", + "columnName": "accountId", + "affinity": "TEXT" + }, + { + "fieldPath": "toAddresses", + "columnName": "toAddresses", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "ccAddresses", + "columnName": "ccAddresses", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "bccAddresses", + "columnName": "bccAddresses", + "affinity": "TEXT", + "notNull": true, + "defaultValue": "''" + }, + { + "fieldPath": "subject", + "columnName": "subject", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "body", + "columnName": "body", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "updatedAt", + "columnName": "updatedAt", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "attachments", + "columnName": "attachments", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "bodyHtml", + "columnName": "bodyHtml", + "affinity": "TEXT" + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "id" + ] + } + }, + { + "tableName": "folders", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`accountId` TEXT NOT NULL, `fullName` TEXT NOT NULL, `displayName` TEXT NOT NULL, `role` TEXT NOT NULL, `selectable` INTEGER NOT NULL, `sortOrder` INTEGER NOT NULL, `specialUse` INTEGER NOT NULL DEFAULT 0, `hierarchyDelimiter` TEXT, PRIMARY KEY(`accountId`, `fullName`))", + "fields": [ + { + "fieldPath": "accountId", + "columnName": "accountId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "fullName", + "columnName": "fullName", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "displayName", + "columnName": "displayName", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "role", + "columnName": "role", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "selectable", + "columnName": "selectable", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "sortOrder", + "columnName": "sortOrder", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "specialUse", + "columnName": "specialUse", + "affinity": "INTEGER", + "notNull": true, + "defaultValue": "0" + }, + { + "fieldPath": "hierarchyDelimiter", + "columnName": "hierarchyDelimiter", + "affinity": "TEXT" + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "accountId", + "fullName" + ] + } + }, + { + "tableName": "backfill_progress", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`accountId` TEXT NOT NULL, `folder` TEXT NOT NULL, `nextBeforeUid` INTEGER NOT NULL, `complete` INTEGER NOT NULL, PRIMARY KEY(`accountId`, `folder`))", + "fields": [ + { + "fieldPath": "accountId", + "columnName": "accountId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "folder", + "columnName": "folder", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "nextBeforeUid", + "columnName": "nextBeforeUid", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "complete", + "columnName": "complete", + "affinity": "INTEGER", + "notNull": true + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "accountId", + "folder" + ] + } + } + ], + "setupQueries": [ + "CREATE TABLE IF NOT EXISTS room_master_table (id INTEGER PRIMARY KEY,identity_hash TEXT)", + "INSERT OR REPLACE INTO room_master_table (id,identity_hash) VALUES(42, 'b5c1a38d197cf1335d3092e413d55d0d')" + ] + } +} \ No newline at end of file From e53a553398aed9826caad554e1d1f48dbc4b788d Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 08:46:14 -0500 Subject: [PATCH 6/6] fix(security): copy account tables by shared columns, not SELECT * MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Device upgrade testing surfaced a crash: on a cache last written before v13, account_settings has 4 columns (accountId, signature, signatureEnabled, notificationsEnabled) but the destination table has 6 (retentionCount/retentionMonths were added at v13). The migrator ran `INSERT OR IGNORE INTO account_settings SELECT * FROM cache...`, which supplied 4 values for 6 columns and threw SQLiteException — and because the done-flag is only set after a successful copy, every launch re-ran and re-crashed (crash loop). AccountDataMigrator now copies each table by the column names present in BOTH the freshly-created destination and the (possibly older) source, so columns the source lacks take the destination's defaults instead of overflowing the value list. Verified on-device: the upgrade migrates a pre-v13 install cleanly and the account stays signed in (sync/backfill workers run). Regression test seeds a v12 cache and asserts the copy. Co-Authored-By: Claude Fable 5 --- .../data/local/AccountDataMigratorTest.kt | 34 +++++++++++++++++++ .../data/local/AccountDataMigrator.kt | 34 +++++++++++++++++-- 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt index 66e37d1..eda28f0 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt @@ -187,6 +187,40 @@ class AccountDataMigratorTest { } } + @Test + fun copiesFromACacheOlderThanTheCurrentSchema() = runBlocking { + // A cache last written at v12 — before account_settings gained retentionCount/retentionMonths + // (v13). The copy must not choke on the columns the destination has but the source lacks + // (a device upgrade from an old install crashed the migrator here). + helper.createDatabase(cacheName, 12).apply { + execSQL( + "INSERT INTO accounts (id, email, displayName, authType, imap_host, imap_port, imap_security, " + + "smtp_host, smtp_port, smtp_security) VALUES ('acct', 'ada@example.org', 'Ada', " + + "'PASSWORD_IMAP', 'imap.example.org', 993, 'SSL_TLS', 'smtp.example.org', 465, 'SSL_TLS')", + ) + execSQL("INSERT INTO credentials (accountId, encryptedSecret) VALUES ('acct', 'sealed-secret')") + execSQL( + "INSERT INTO account_settings (accountId, signature, signatureEnabled, notificationsEnabled) " + + "VALUES ('acct', 'Sig', 0, 1)", + ) + close() + } + + AccountDataMigrator.copyAccountTables(cacheFile, cachePassphrase = "", accountsFile = accountsFile) + + openAccountsDb().apply { + assertEquals("ada@example.org", accountDao().getById("acct")?.email) + assertEquals("sealed-secret", credentialDao().getById("acct")?.encryptedSecret) + val settings = accountSettingsDao().get("acct") + assertEquals(false, settings?.signatureEnabled) + assertEquals(true, settings?.notificationsEnabled) + // Columns the v12 source lacked come across as the destination's defaults (null). + assertNull("retentionCount absent from a v12 cache must default to null", settings?.retentionCount) + assertNull(settings?.retentionMonths) + close() + } + } + @Test fun migratorDdlMatchesExportedAccountDatabaseSchema() { val schema = JSONObject( diff --git a/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt b/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt index 8e5a50e..521bc81 100644 --- a/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt +++ b/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt @@ -158,10 +158,15 @@ class AccountDataMigrator @Inject constructor( if (present.isEmpty()) return // fresh cache or already dropped: nothing to move TABLES.forEach { db.rawExecSQL(CREATE_TABLE_SQL.getValue(it)) } db.rawExecSQL(SIGNATURES_INDEX_SQL) - // Parent first so an enforced foreign key (Room enables them; this raw connection - // does not) would still be satisfied. INSERT OR IGNORE makes each copy idempotent. + // Copy by explicit shared column names, never SELECT *: the on-disk cache may predate + // columns the current schema added (e.g. account_settings gained retentionCount / + // retentionMonths at v13), and a bare SELECT * would then supply fewer values than the + // destination has columns and fail the whole migration. Listing the columns the source + // actually has lets the destination's newer columns take their defaults (NULL). Parent + // first so an enforced foreign key would still be satisfied; INSERT OR IGNORE is idempotent. TABLES.filter { it in present }.forEach { table -> - db.rawExecSQL("INSERT OR IGNORE INTO `$table` SELECT * FROM cache.`$table`") + val cols = sharedColumns(db, table) + db.rawExecSQL("INSERT OR IGNORE INTO `$table` ($cols) SELECT $cols FROM cache.`$table`") } Log.d(TAG, "moved account tables into the account database: $present") } finally { @@ -189,5 +194,28 @@ class AccountDataMigrator @Inject constructor( } return present } + + /** + * Column names present in BOTH the freshly-created destination `$table` (always the current + * schema) and the source `cache.$table` (possibly an older on-disk schema), backtick-quoted and + * comma-joined for an INSERT/SELECT column list. Destination-only columns are omitted so they + * take their defaults instead of overflowing the value list. + */ + private fun sharedColumns(db: SQLiteDatabase, table: String): String { + val source = tableColumns(db, "cache", table) + return tableColumns(db, "main", table) + .filter { it in source } + .joinToString(", ") { "`$it`" } + } + + /** The column names of `$schema.$table`, in declared order, via `PRAGMA table_info`. */ + private fun tableColumns(db: SQLiteDatabase, schema: String, table: String): List { + val columns = mutableListOf() + db.rawQuery("PRAGMA $schema.table_info(`$table`)", null).use { cursor -> + val nameIndex = cursor.getColumnIndexOrThrow("name") + while (cursor.moveToNext()) columns += cursor.getString(nameIndex) + } + return columns + } } }