From b03e36388814776748d9cd7d1b5f53b691712de5 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 8 Jul 2026 19:39:13 -0500 Subject: [PATCH 1/3] perf(yahoo): respect Yahoo/AOL IMAP limits & avoid the 1-hour auth lockout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Yahoo/AOL trip an automated ~1-hour service lockout after too many rapid or failed authentication attempts. An unguarded login repeater (notably the IDLE reconnect loop, which starts at a 5s backoff) could fire several failed LOGINs within the first minute and lock out a real user for an hour. Add a proactive, Yahoo/AOL-scoped auth circuit-breaker that spaces out login attempts so we never reach the lockout, building on the #360 reactive throttle framework rather than reinventing it. New (org.libremail.mail), all host-keyed so only Yahoo/AOL are gated: - ProviderAuthPolicy: per-host AuthCadencePolicy (Yahoo/AOL enabled, everything else DISABLED). Also exposes the documented 5-connection and 10k-folder caps. - AuthBackoff: pure schedule — exponential equal-jitter ramp (base 60s -> 30s floor after jitter, cap 15m) up to a 4-failure threshold, then a fixed 30m open-circuit window. Every wait stays well under the ~1h lockout. - AuthThrottleGate (@Singleton): per-account state; onAuthFailure / onAuthSuccess / remainingAuthBlockMillis, PII-free logging. Enforcement: - ImapClient guards every real LOGIN (connect-per-op, reuse connect/reconnect, and the IDLE connection): skips the login (AuthBackoffException) while backing off, arms the gate on an AuthenticationFailedException, clears it on success. A transient (non-auth) connect error never arms the backoff. - MailBackfiller skips an auth-blocked account exactly as it skips a reactively throttled one (#360), so BackfillPacer (#356) never spins a cooldown on it. The 5-connection cap is already satisfied by connection reuse (#125/#357: ~1 warm socket + 1 IDLE per account); the 10k folder truncation is respected for free by backfill stopping when the server returns nothing older. Tests: pure-schedule, policy-resolution, gate (virtual time, incl. composition with #360), GreenMail enforcement (auth-fail arms / transient does not / blocked skips), and an on-device instrumented gate test. Closes #362 --- .../mail/AuthThrottleGateInstrumentedTest.kt | 105 ++++++++ .../org/libremail/data/sync/MailBackfiller.kt | 45 +++- .../kotlin/org/libremail/mail/AuthBackoff.kt | 48 ++++ .../org/libremail/mail/AuthThrottleGate.kt | 136 ++++++++++ .../kotlin/org/libremail/mail/ImapClient.kt | 76 +++++- .../org/libremail/mail/ProviderAuthPolicy.kt | 139 ++++++++++ .../libremail/data/sync/MailBackfillerTest.kt | 35 +++ .../data/sync/MailMaintenanceGateTest.kt | 2 + .../data/sync/MailSyncConcurrencyTest.kt | 2 + .../org/libremail/mail/AuthBackoffTest.kt | 104 ++++++++ .../libremail/mail/AuthThrottleGateTest.kt | 237 ++++++++++++++++++ .../mail/ImapClientAuthBackoffTest.kt | 125 +++++++++ .../libremail/mail/ProviderAuthPolicyTest.kt | 82 ++++++ 13 files changed, 1119 insertions(+), 17 deletions(-) create mode 100644 app/src/androidTest/kotlin/org/libremail/mail/AuthThrottleGateInstrumentedTest.kt create mode 100644 app/src/main/kotlin/org/libremail/mail/AuthBackoff.kt create mode 100644 app/src/main/kotlin/org/libremail/mail/AuthThrottleGate.kt create mode 100644 app/src/main/kotlin/org/libremail/mail/ProviderAuthPolicy.kt create mode 100644 app/src/test/kotlin/org/libremail/mail/AuthBackoffTest.kt create mode 100644 app/src/test/kotlin/org/libremail/mail/AuthThrottleGateTest.kt create mode 100644 app/src/test/kotlin/org/libremail/mail/ImapClientAuthBackoffTest.kt create mode 100644 app/src/test/kotlin/org/libremail/mail/ProviderAuthPolicyTest.kt diff --git a/app/src/androidTest/kotlin/org/libremail/mail/AuthThrottleGateInstrumentedTest.kt b/app/src/androidTest/kotlin/org/libremail/mail/AuthThrottleGateInstrumentedTest.kt new file mode 100644 index 0000000..acb11a2 --- /dev/null +++ b/app/src/androidTest/kotlin/org/libremail/mail/AuthThrottleGateInstrumentedTest.kt @@ -0,0 +1,105 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.mail + +import androidx.test.ext.junit.runners.AndroidJUnit4 +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import org.libremail.domain.model.ImapConnectionParams +import org.libremail.domain.model.MailProvider +import org.libremail.domain.model.MailSecurity + +/** + * On-device proof of issue #362's proactive auth circuit-breaker on the REAL Android runtime (not the + * JVM stubs), across the CI API matrix. A real [AuthThrottleGate] with an always-Yahoo policy and a + * manual clock — so it is deterministic and never real-sleeps — must, for a Yahoo/AOL account: block a + * login after an auth failure, escalate rapid failures without reaching the ~1-hour lockout window, open + * a long fixed circuit past the threshold, isolate accounts, and clear on success; and it must be a + * total no-op for a non-gated host. + * + * Deliberately mock-free (no `mockk`, no framework `Context`): the gate's only inputs are plain lambdas + * and value objects, so this exercises the genuine state machine on the device and dodges the + * mockk-on-framework-types landmines. The JVM [AuthThrottleGateTest] covers the same contract under + * coroutines-test virtual time; this proves it survives the real dispatcher and API levels. + */ +@RunWith(AndroidJUnit4::class) +class AuthThrottleGateInstrumentedTest { + + private var now = 0L + private val yahoo = ProviderAuthPolicy.forHost(MailProvider.YAHOO.createAccount("x@yahoo.com").imap.host) + + private fun gate(policyForHost: (String) -> AuthCadencePolicy = { yahoo }) = + AuthThrottleGate(nowMillis = { now }, random = { 0.0 }, policyForHost = policyForHost) + + private fun params(user: String = "user@example.org", host: String = "imap.mail.yahoo.com") = + ImapConnectionParams(host, PORT, MailSecurity.SSL_TLS, user, secret = "secret", useXoauth2 = false) + + @Test + fun anAuthFailureBlocksTheAccountAndEscalatesWithoutReachingTheLockout() { + val gate = gate() + val p = params() + + val first = gate.onAuthFailure(p) + assertTrue("a failure blocks the account", gate.isAuthBlocked(p)) + assertTrue("the first block is positive", first > 0L) + + val second = gate.onAuthFailure(p) + assertTrue("a consecutive failure backs off longer", second > first) + + // Never as long as the ~1-hour lockout the backoff exists to avoid. + repeat(RAPID_FAILURES) { assertTrue(gate.onAuthFailure(p) < ONE_HOUR_MS) } + } + + @Test + fun theCircuitOpensToAFixedWindowPastTheThreshold() { + val gate = gate() + val p = params() + + var last = 0L + repeat(yahoo.circuitOpenThreshold) { last = gate.onAuthFailure(p) } + + assertEquals("the threshold failure opens the fixed circuit window", yahoo.circuitOpenMillis, last) + assertEquals("and it stays open at that window", yahoo.circuitOpenMillis, gate.onAuthFailure(p)) + } + + @Test + fun accountsAreIsolatedAndSuccessClearsTheBackoff() { + val gate = gate() + val blocked = params(user = "blocked@example.org") + val healthy = params(user = "healthy@example.org") + + gate.onAuthFailure(blocked) + assertTrue(gate.isAuthBlocked(blocked)) + assertFalse("one blocked account never stalls another", gate.isAuthBlocked(healthy)) + + gate.onAuthSuccess(blocked) + assertFalse("a successful login clears the backoff", gate.isAuthBlocked(blocked)) + } + + @Test + fun theWindowClearsOnceItElapses() { + val gate = gate() + val p = params() + + val block = gate.onAuthFailure(p) + now += block + assertFalse("the account may retry once the window elapses", gate.isAuthBlocked(p)) + } + + @Test + fun aNonGatedHostIsNeverBlocked() { + val gate = gate(policyForHost = ProviderAuthPolicy::forHost) + val gmail = params(host = "imap.gmail.com") + + assertEquals(0L, gate.onAuthFailure(gmail)) + assertFalse(gate.isAuthBlocked(gmail)) + } + + private companion object { + const val PORT = 993 + const val RAPID_FAILURES = 10 + const val ONE_HOUR_MS = 60 * 60_000L + } +} diff --git a/app/src/main/kotlin/org/libremail/data/sync/MailBackfiller.kt b/app/src/main/kotlin/org/libremail/data/sync/MailBackfiller.kt index 42bb8f0..3be6ab2 100644 --- a/app/src/main/kotlin/org/libremail/data/sync/MailBackfiller.kt +++ b/app/src/main/kotlin/org/libremail/data/sync/MailBackfiller.kt @@ -25,6 +25,7 @@ import org.libremail.data.settings.effectiveRetention import org.libremail.domain.model.Account import org.libremail.domain.model.ImapConnectionParams import org.libremail.domain.repository.MailRepository +import org.libremail.mail.AuthThrottleGate import org.libremail.mail.ImapClient import org.libremail.power.BatteryStatusProvider import org.libremail.reporting.AppLog @@ -59,6 +60,7 @@ class MailBackfiller @Inject constructor( private val maintenanceGate: MailMaintenanceGate, private val throttleGate: AccountThrottleGate, private val interactiveGate: InteractiveImapGate, + private val authGate: AuthThrottleGate, ) { /** One folder's slice outcome: pages fetched, and whether an immediate follow-up slice has work to do. */ private data class FolderResult(val batches: Int, val moreWork: Boolean) @@ -75,19 +77,10 @@ class MailBackfiller @Inject constructor( var remaining = maxBatches var moreWork = false accounts@ for (account in accountDao.getAll().map { it.toDomain() }) { - // Graceful degradation + per-account isolation (#360): an account still inside its throttle - // backoff window is skipped this slice — we don't page a provider that just rate-limited or - // locked us (hammering it makes throttling worse, the on-device perf finding). The window - // elapses on its own, so a later scheduled slice resumes this account automatically. A skip - // deliberately does NOT set moreWork: a slice whose only outstanding work is a throttled - // account reports "done" so the worker's slice-chaining loop stops instead of tight-looping - // over the skip. Other accounts are untouched. - val backoffRemaining = throttleGate.remainingBackoffMillis(account.id) - if (backoffRemaining > 0L) { - AppLog.i(TAG, "backfill skip ${accountLogRef(account.id)}: throttled, remaining=${backoffRemaining}ms") - continue@accounts - } - val params = runCatching { connectionFactory.imapParamsFor(account) }.getOrNull() ?: continue + // Skip an account backing off (throttle #360 / auth #362) or with unresolvable credentials — + // a null return does NOT set moreWork, so a slice whose only work is a backing-off account + // reports "done" instead of tight-looping over the skip. See [paramsForBackfill]. + val params = paramsForBackfill(account) ?: continue@accounts val policy = accountSettingsRepository.effectiveRetention(settingsRepository, account.id) for (folder in messageDao.syncedFolders(account.id)) { if (remaining <= 0) { @@ -111,6 +104,32 @@ class MailBackfiller @Inject constructor( moreWork } + /** + * Resolves [account] to its IMAP connection params for this slice, or **null when the account must be + * skipped without paging** — it is inside a reactive throttle-backoff window (#360) or a proactive + * auth-backoff window (#362), or its credentials could not be resolved. Both backoffs are graceful + * degradation with per-account isolation: we don't page a provider that just rate-limited or locked us + * (hammering makes throttling worse — the on-device perf finding), and we don't drive a login that + * [org.libremail.mail.ImapClient] would only skip anyway, nudging a Yahoo/AOL account toward its + * ~1-hour lockout. Each window elapses on its own so a later scheduled slice resumes; a skip logs a + * PII-free breadcrumb and its caller does NOT set moreWork, so a slice whose only outstanding work is a + * backing-off account reports "done" rather than tight-looping over the skip. + */ + private suspend fun paramsForBackfill(account: Account): ImapConnectionParams? { + val throttleRemaining = throttleGate.remainingBackoffMillis(account.id) + if (throttleRemaining > 0L) { + AppLog.i(TAG, "backfill skip ${accountLogRef(account.id)}: throttled, remaining=${throttleRemaining}ms") + return null + } + val params = runCatching { connectionFactory.imapParamsFor(account) }.getOrNull() ?: return null + val authBlock = authGate.remainingAuthBlockMillis(params) + if (authBlock > 0L) { + AppLog.i(TAG, "backfill skip ${accountLogRef(account.id)}: auth backing off, remaining=${authBlock}ms") + return null + } + return params + } + /** * Pages one folder, translating a failure into the slice's control flow (issue #360). Returns the * [FolderResult] on success — or, for an ordinary transient error, a zero-page result whose diff --git a/app/src/main/kotlin/org/libremail/mail/AuthBackoff.kt b/app/src/main/kotlin/org/libremail/mail/AuthBackoff.kt new file mode 100644 index 0000000..7c79482 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/mail/AuthBackoff.kt @@ -0,0 +1,48 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.mail + +import kotlin.math.min + +/** + * The pure, proactive **auth-backoff schedule** for issue #362: given a provider's [AuthCadencePolicy] + * and how many times in a row an account has failed to authenticate ([consecutiveFailures], 1-based), + * returns how long to block further login attempts. Side-effect- and clock-free (the caller supplies the + * jitter draw) so the whole schedule is deterministically unit-testable; [AuthThrottleGate] owns the + * per-account state, clock, and logging. + * + * Two regimes, both aimed at never tripping Yahoo/AOL's ~1-hour auth lockout: + * - **Ramp** (`failures < circuitOpenThreshold`): exponential in the failure count + * (`base * 2^(failures-1)`), clamped to [AuthCadencePolicy.maxBackoffMillis], with **equal jitter** — + * half the capped target as a floor, the other half spread by [random] — so the result lies in + * `[capped/2, capped]` and a fleet throttled at once doesn't retry in lockstep. Mirrors the equal-jitter + * math of issue #360's [org.libremail.data.sync.ThrottleBackoff]. + * - **Open circuit** (`failures >= circuitOpenThreshold`): a single long, *fixed* block + * ([AuthCadencePolicy.circuitOpenMillis]) — "back off long and stop". Deliberately un-jittered: once we + * give up probing, the window is a firm floor, not something jitter can shorten. + * + * A [disabled][AuthCadencePolicy.enabled] policy always returns `0` (never blocks), so non-Yahoo/AOL + * hosts are unaffected. + */ +object AuthBackoff { + + /** Caps the exponential shift so `base shl (failures-1)` can never overflow before the cap applies. */ + private const val MAX_SHIFT = 16 + + /** + * Block duration in milliseconds for the given 1-based [consecutiveFailures] under [policy], with the + * equal-jitter draw [random] (expected in `[0.0, 1.0)`). See the class doc for the ramp vs. + * open-circuit regimes. + */ + fun blockMillis(policy: AuthCadencePolicy, consecutiveFailures: Int, random: Double): Long { + require(consecutiveFailures >= 1) { "consecutiveFailures must be >= 1" } + if (!policy.enabled) return 0L + if (consecutiveFailures >= policy.circuitOpenThreshold) return policy.circuitOpenMillis + val shift = min(consecutiveFailures - 1, MAX_SHIFT) + val exponential = policy.baseBackoffMillis shl shift + // shl can overflow to <= 0 for a pathological count; treat that as "past the cap". + val capped = if (exponential in 1..policy.maxBackoffMillis) exponential else policy.maxBackoffMillis + val half = capped / 2 + val jitter = (random.coerceIn(0.0, 1.0) * half).toLong() + return half + jitter + } +} diff --git a/app/src/main/kotlin/org/libremail/mail/AuthThrottleGate.kt b/app/src/main/kotlin/org/libremail/mail/AuthThrottleGate.kt new file mode 100644 index 0000000..315f484 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/mail/AuthThrottleGate.kt @@ -0,0 +1,136 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.mail + +import jakarta.mail.MessagingException +import org.libremail.domain.model.ImapConnectionParams +import org.libremail.reporting.AppLog +import org.libremail.reporting.accountLogRef +import java.util.concurrent.ConcurrentHashMap +import java.util.concurrent.ThreadLocalRandom +import javax.inject.Inject +import javax.inject.Singleton + +/** + * Thrown by [ImapClient] instead of attempting a `LOGIN` while an account is inside its proactive + * auth-backoff window (issue #362): the login is *skipped*, not merely retried, so a Yahoo/AOL account + * never accumulates the rapid failed logins that trip the ~1-hour lockout. Extends [MessagingException] + * so existing IMAP error handling catches it, and carries **no PII** — only the remaining wait. Its + * message deliberately avoids any throttle/lock wording so [org.libremail.data.sync.ThrottleClassifier] + * does not misread it as a *reactive* throttle signal, and [ImapConnectionCache] does not treat it as a + * connection drop (its cause is null), so a reused connection is never needlessly rebuilt on it. + */ +class AuthBackoffException(remainingMillis: Long) : + MessagingException("IMAP login paused: backing off ${remainingMillis}ms to protect the account") + +/** + * Per-account **proactive auth circuit-breaker** for issue #362 — the enforcing state machine that turns + * repeated authentication failures into an [AuthBackoff]-scheduled block, so LibreMail never hammers + * Yahoo/AOL's login endpoint into its automated **~1-hour lockout**. + * + * This is the proactive counterpart to issue #360's reactive [org.libremail.data.sync.AccountThrottleGate]: + * that gate reacts to a throttle/lock response the server *already sent*; this one prevents us from ever + * eliciting one. [ImapClient] consults [remainingAuthBlockMillis] before every real `LOGIN` (connect-per-op, + * the reuse cache's connect/reconnect, and the long-lived IDLE connection) and throws [AuthBackoffException] + * instead of connecting while blocked; it records the outcome via [onAuthFailure] / [onAuthSuccess]. The + * full-history backfill ([org.libremail.data.sync.MailBackfiller]) additionally *skips* an auth-blocked + * account, exactly as it skips a reactively-throttled one (#360) — so the two gates compose and the + * [org.libremail.data.sync.BackfillPacer] (#356) never burns cooldowns spinning on a blocked login. + * + * **Only Yahoo/AOL are gated.** State is created solely when [ProviderAuthPolicy.forHost] returns an + * enabled policy, so every other provider (Gmail/iCloud/Outlook, issues #361/#363/#364, and manual + * servers) is a no-op here and unchanged. + * + * **Per-account isolation & PII-free.** State is keyed by the account's connection identity + * (`host|port|username`), so one blocked account never stalls another, and every log line uses + * [accountLogRef] over that key — a salted-looking hash, never the address or host. + * + * State lives only in-process (`@Singleton`); a process restart clears it and simply re-probes, which is + * safe — the first attempt after restart is spaced from the previous run by however long the process was + * down, and any real re-failure immediately re-arms the backoff. + */ +@Singleton +class AuthThrottleGate internal constructor( + private val nowMillis: () -> Long, + private val random: () -> Double, + private val policyForHost: (String) -> AuthCadencePolicy, +) { + /** Production wiring: the real wall clock, a per-thread RNG for jitter, and the host-keyed policy. */ + @Inject + constructor() : this( + nowMillis = System::currentTimeMillis, + random = { ThreadLocalRandom.current().nextDouble() }, + policyForHost = ProviderAuthPolicy::forHost, + ) + + /** One account's auth state: consecutive failures, until when logins are blocked, the last wait, open? */ + private data class State( + val failures: Int, + val blockedUntilMillis: Long, + val lastBlockMillis: Long, + val circuitOpen: Boolean, + ) + + private val states = ConcurrentHashMap() + + /** + * Records a failed authentication for [params]'s account and returns the resulting block in ms (0 when + * the host has no auth-lockout risk, so the call is a no-op). Escalates the consecutive-failure count + * so repeats back off exponentially, then extend into the fixed open-circuit window past the policy's + * threshold, and stamps the account blocked until `now + block`. Atomic per account. Logs a PII-free + * breadcrumb (failure count, block, and whether the circuit is now open). + */ + fun onAuthFailure(params: ImapConnectionParams): Long { + val policy = policyForHost(params.host) + if (!policy.enabled) return 0L + val now = nowMillis() + val updated = states.compute(key(params)) { _, previous -> + val failures = (previous?.failures ?: 0) + 1 + val block = AuthBackoff.blockMillis(policy, failures, random()) + State( + failures = failures, + blockedUntilMillis = now + block, + lastBlockMillis = block, + circuitOpen = failures >= policy.circuitOpenThreshold, + ) + }!! + AppLog.w( + TAG, + "auth backoff ${logRef(params)} failures=${updated.failures} block=${updated.lastBlockMillis}ms" + + if (updated.circuitOpen) " circuit=open" else "", + ) + return updated.lastBlockMillis + } + + /** + * Clears any auth-backoff state for [params]'s account after a successful login, so a recovered + * account resumes at full speed with the failure count reset. Silent no-op when the account was not + * blocked (or the host is not gated), so [ImapClient] can call it on every successful connect. + */ + fun onAuthSuccess(params: ImapConnectionParams) { + val previous = states.remove(key(params)) ?: return + AppLog.i(TAG, "auth recovered ${logRef(params)} after ${previous.failures} failure(s)") + } + + /** + * Milliseconds until [params]'s account may attempt a login again, or 0 when it is not blocked (or the + * window already elapsed). A passed window keeps its failure count until the next [onAuthSuccess], so a + * re-failure before recovery escalates rather than restarting from the base delay. + */ + fun remainingAuthBlockMillis(params: ImapConnectionParams): Long { + val state = states[key(params)] ?: return 0L + return (state.blockedUntilMillis - nowMillis()).coerceAtLeast(0L) + } + + /** True while [params]'s account is inside its auth-backoff window and a login must be skipped. */ + fun isAuthBlocked(params: ImapConnectionParams): Boolean = remainingAuthBlockMillis(params) > 0L + + /** PII-free, stable reference for [params]'s account — a hash of the connection identity, never it. */ + fun logRef(params: ImapConnectionParams): String = accountLogRef(key(params)) + + /** Connection identity keying the state: everything that pins a distinct authenticated login. */ + private fun key(params: ImapConnectionParams): String = "${params.host}|${params.port}|${params.username}" + + private companion object { + const val TAG = "AuthThrottleGate" + } +} diff --git a/app/src/main/kotlin/org/libremail/mail/ImapClient.kt b/app/src/main/kotlin/org/libremail/mail/ImapClient.kt index 4a49550..fd36f72 100644 --- a/app/src/main/kotlin/org/libremail/mail/ImapClient.kt +++ b/app/src/main/kotlin/org/libremail/mail/ImapClient.kt @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-3.0-or-later package org.libremail.mail +import jakarta.mail.AuthenticationFailedException import jakarta.mail.FetchProfile import jakarta.mail.Flags import jakarta.mail.Folder @@ -114,6 +115,12 @@ class ImapClient internal constructor( * no-UIDPLUS fallback path on its own. */ private val supportsUidPlus: (IMAPFolder) -> Boolean = ::probeUidPlusCapability, + /** + * Proactive auth circuit-breaker (issue #362): consulted before every real `LOGIN` so a Yahoo/AOL + * account never accumulates the rapid failed logins that trip its ~1-hour lockout. A no-op for every + * other host. Defaulted here so the test/harness seam constructs one without extra wiring. + */ + private val authGate: AuthThrottleGate = AuthThrottleGate(), ) { /** @@ -125,7 +132,10 @@ class ImapClient internal constructor( * `false` (a build-config change, no code edit) restores connect-per-operation if a server * misbehaves with a kept-alive socket. The internal constructor is the test/harness seam. */ - @Inject constructor() : this(reuseConnections = BuildConfig.IMAP_CONNECTION_REUSE) + @Inject constructor(authGate: AuthThrottleGate) : this( + reuseConnections = BuildConfig.IMAP_CONNECTION_REUSE, + authGate = authGate, + ) /** * Per-account keep-alive cache; allocated only when reuse is enabled, so a reuse-disabled build @@ -540,9 +550,13 @@ class ImapClient internal constructor( * connection to unblock idle()) or a connection error is thrown, leaving reconnection to the caller. */ suspend fun idle(params: ImapConnectionParams, onActivity: suspend () -> Unit) = withContext(Dispatchers.IO) { + // Proactive auth circuit-breaker (issue #362): the IDLE reconnect loop (IdleService) is the fastest + // login repeater — an unguarded auth failure there would storm Yahoo/AOL into their ~1-hour lockout + // — so skip the LOGIN while backing off, and feed the gate exactly as the connect-per-op path does. + guardAuthBackoff(params, op = "IDLE login") val protocol = if (params.security == MailSecurity.SSL_TLS) "imaps" else "imap" val store = Session.getInstance(buildProps(protocol, params)).getStore(protocol) - store.connect(params.host, params.port, params.username, params.secret) + connectRecordingAuth(store, params) // Close the just-connected store if opening the folder fails, so a failed connect in the // IDLE reconnect loop can't leak connections until the server's per-account limit is hit. val inbox = try { @@ -715,14 +729,51 @@ class ImapClient internal constructor( } } - /** Builds and authenticates a fresh [Store] (`CONNECT + TLS + LOGIN`); the caller owns closing it. */ + /** + * Builds and authenticates a fresh [Store] (`CONNECT + TLS + LOGIN`); the caller owns closing it. + * + * Gated by the proactive auth circuit-breaker (issue #362): while the account is inside its + * auth-backoff window the `LOGIN` is *skipped* ([guardAuthBackoff] throws [AuthBackoffException]) + * rather than attempted, so a Yahoo/AOL account never accumulates the rapid failed logins that trip + * its ~1-hour lockout. An authentication failure feeds [AuthThrottleGate.onAuthFailure]; a success + * clears it. A transient (non-auth) connect error is rethrown untouched, never arming the backoff. + */ private fun openConnectedStore(params: ImapConnectionParams): Store { + guardAuthBackoff(params, op = "login") val protocol = if (params.security == MailSecurity.SSL_TLS) "imaps" else "imap" val store = Session.getInstance(buildProps(protocol, params, reuse = reuseConnections)).getStore(protocol) - store.connect(params.host, params.port, params.username, params.secret) + connectRecordingAuth(store, params) return store } + /** + * Skips a login while the account is auth-backing-off (issue #362) by throwing [AuthBackoffException], + * so no `LOGIN` reaches the provider. Logged at INFO — an expected, protective skip, not an error. + * A no-op for non-gated (non-Yahoo/AOL) hosts, whose [AuthThrottleGate.remainingAuthBlockMillis] is 0. + */ + private fun guardAuthBackoff(params: ImapConnectionParams, op: String) { + val remaining = authGate.remainingAuthBlockMillis(params) + if (remaining > 0L) { + AppLog.i(TAG, "$op skipped ${authGate.logRef(params)}: auth backing off ${remaining}ms") + throw AuthBackoffException(remaining) + } + } + + /** + * Runs the actual `store.connect` (`CONNECT + TLS + LOGIN`) and feeds the issue-#362 auth + * circuit-breaker: a rejected LOGIN ([isAuthFailure]) arms the backoff via [AuthThrottleGate], a + * success clears it, and a transient (non-auth) error is rethrown untouched — never arming it. + */ + private fun connectRecordingAuth(store: Store, params: ImapConnectionParams) { + try { + store.connect(params.host, params.port, params.username, params.secret) + } catch (e: Throwable) { + if (isAuthFailure(e)) authGate.onAuthFailure(params) + throw e + } + authGate.onAuthSuccess(params) + } + /** * Tears down every kept-alive reused connection (`LOGOUT` + teardown); a no-op when reuse is * disabled. `IdleService` drives this on the low-battery push-teardown path (#88/#89/#90), mirroring @@ -799,6 +850,23 @@ private fun probeUidPlusCapability(folder: IMAPFolder): Boolean = runCatching { folder.doCommand { protocol -> protocol.hasCapability(CAP_UIDPLUS) } as? Boolean }.getOrNull() ?: false +/** + * True when [error] (or anything in its cause chain) is an [AuthenticationFailedException] — a rejected + * `LOGIN`, the only signal the issue-#362 auth circuit-breaker counts. Deliberately narrow: a transient + * network/socket error is NOT an auth failure and must never arm the backoff. Guards against a cyclic + * cause chain with an identity-based visited check, mirroring [ImapAuthError]'s walk. + */ +private fun isAuthFailure(error: Throwable): Boolean { + val seen = mutableListOf() + var current: Throwable? = error + while (current != null && seen.none { it === current }) { + if (current is AuthenticationFailedException) return true + seen.add(current) + current = current.cause + } + return false +} + /** * True when [part] is a user-facing downloadable attachment: its `Content-Disposition` is * `attachment`, OR it has a filename but no `Content-ID` header. A part with a filename AND a diff --git a/app/src/main/kotlin/org/libremail/mail/ProviderAuthPolicy.kt b/app/src/main/kotlin/org/libremail/mail/ProviderAuthPolicy.kt new file mode 100644 index 0000000..8bd3691 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/mail/ProviderAuthPolicy.kt @@ -0,0 +1,139 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.mail + +import org.libremail.domain.model.MailProvider + +/** + * Per-provider IMAP limits and the **proactive auth cadence** for issue #362, keyed by IMAP host. + * + * This is the *config* half of the Yahoo/AOL work; the enforcing state machine is [AuthThrottleGate] + * and the pure schedule is [AuthBackoff]. It deliberately parallels issue #360's *reactive* family + * ([org.libremail.data.sync.ThrottleBackoff] / [org.libremail.data.sync.AccountThrottleGate]) — that + * family reacts to a throttle/lock response the server *already sent*; this one is proactive, spacing + * out login attempts so we never **reach** the response in the first place. + * + * **Why Yahoo/AOL are special.** Yahoo (and AOL, which shares Yahoo's mail platform) trip an automated + * **~1-hour service lockout** after too many rapid or failed authentication attempts, and cap a + * mailbox at **5 simultaneous IMAP connections** with the folder index truncated to **10,000 + * messages** (issue #362's documented limits). The lockout is the dangerous one: a wrong app-password + * plus a naive retry loop (e.g. the IDLE reconnect loop, which starts at a 5-second backoff) can fire + * several failed `LOGIN`s within the first minute and get a *real user* locked out for an hour. So for + * these hosts the app must back off login attempts long and hard. + * + * **Every other provider is disabled here** ([AuthCadencePolicy.DISABLED]): Gmail, iCloud, Outlook, + * and manually-configured servers have no comparable 1-hour auth lockout, so the proactive + * circuit-breaker is a Yahoo/AOL-scoped no-op for them and their behaviour is unchanged (their own + * limits are issues #361/#363/#364). Keeping the policy host-keyed — rather than refactoring shared + * code — is what makes this change additive and safe to land alongside those siblings. + */ +data class AuthCadencePolicy( + /** When false the whole proactive auth circuit-breaker is inert for this host (records nothing). */ + val enabled: Boolean, + /** First-failure backoff before a retry is permitted; doubles per consecutive failure. */ + val baseBackoffMillis: Long, + /** Ceiling on the exponential ramp, so a single wait never grows without bound. */ + val maxBackoffMillis: Long, + /** Consecutive failures after which the circuit *opens* — retries stop for [circuitOpenMillis]. */ + val circuitOpenThreshold: Int, + /** The long, fixed block applied once the circuit is open: "back off long and stop" (issue #362). */ + val circuitOpenMillis: Long, + /** Documented simultaneous-connection ceiling for this provider (see [ProviderAuthPolicy]). */ + val maxConcurrentConnections: Int, + /** Documented server-side folder-index truncation (messages) for this provider. */ + val folderIndexCap: Int, +) { + companion object { + /** + * The inert policy for every host without a Yahoo-style auth lockout. Every threshold is set so + * the gate can never block ([circuitOpenThreshold] unreachable, caps effectively unbounded), so a + * non-Yahoo/AOL account is never gated and behaves exactly as before issue #362. + */ + val DISABLED = AuthCadencePolicy( + enabled = false, + baseBackoffMillis = 0L, + maxBackoffMillis = 0L, + circuitOpenThreshold = Int.MAX_VALUE, + circuitOpenMillis = 0L, + maxConcurrentConnections = Int.MAX_VALUE, + folderIndexCap = Int.MAX_VALUE, + ) + } +} + +/** + * Resolves the [AuthCadencePolicy] for an IMAP host. Yahoo and AOL (one platform) get the conservative + * lockout-avoiding policy; everything else gets [AuthCadencePolicy.DISABLED]. + */ +object ProviderAuthPolicy { + + private const val MINUTE_MS = 60_000L + + /** + * First-failure auth backoff (1 min → a 30 s floor after equal jitter, see [AuthBackoff]). The whole + * point is that the **second** login attempt lands ≥30 s after the first: Yahoo's lockout keys on + * *rapid* failures (attempts seconds apart, as an unguarded reconnect loop produces), and a ≥30 s + * spacing is decisively not rapid. This floor is well under the ~1-hour lockout window it protects. + */ + const val YAHOO_AUTH_BACKOFF_BASE_MS = MINUTE_MS + + /** + * Ceiling on the exponential ramp (15 min). Comfortably under the ~1-hour lockout, so an account that + * recovers (a transient auth blip clears, or the user fixes the credential) resumes far sooner than a + * self-inflicted hour of silence, while still spacing attempts to at most a few per hour. + */ + const val YAHOO_AUTH_BACKOFF_MAX_MS = 15 * MINUTE_MS + + /** + * Consecutive failed logins after which the circuit opens (4). A wrong app-password does not fix + * itself, so once we have failed this many times in a row we stop *ramping* and switch to the long + * fixed [YAHOO_AUTH_CIRCUIT_OPEN_MS] block — "back off long and stop" — rather than keep probing and + * risk accumulating enough failures to trip the lockout. Reached in ~3.5 min of spaced attempts. + */ + const val YAHOO_AUTH_CIRCUIT_OPEN_THRESHOLD = 4 + + /** + * The block applied once the circuit is open (30 min). Longer than the 15-min ramp cap (we have given + * up probing) yet still **under** the ~1-hour lockout, so recovery beats the lockout — and long + * enough that Yahoo's short rolling failure window fully decays between our probes, holding us to at + * most ~2 failed logins/hour once open: no rolling-window lockout heuristic reads that as rapid. + */ + const val YAHOO_AUTH_CIRCUIT_OPEN_MS = 30 * MINUTE_MS + + /** + * Yahoo/AOL's documented simultaneous-connection ceiling (5). LibreMail stays well under this by + * design: connection reuse (issues #125/#357, ON by default) collapses a whole account to ~1 warm + * IMAP socket plus at most one long-lived IDLE connection — 2 per account, not the `1 + K + + * attachments` sockets the connect-per-operation path once opened per backfill page. Exposed as + * config so the invariant is checkable (see the policy tests) rather than only implicit. + */ + const val YAHOO_MAX_CONCURRENT_CONNECTIONS = 5 + + /** + * Yahoo/AOL's documented folder-index truncation (10,000 messages). The full-history backfill + * (issue #12) already respects this for free: paging older-than-UID simply returns empty once the + * server exposes nothing beyond the truncation point, which the backfiller treats as "folder fully + * backfilled". Exposed as config for visibility and so a future page-cap can reference it. + */ + const val YAHOO_FOLDER_INDEX_CAP = 10_000 + + /** Shared by Yahoo and AOL — one mail platform, one set of limits. */ + private val YAHOO_AOL = AuthCadencePolicy( + enabled = true, + baseBackoffMillis = YAHOO_AUTH_BACKOFF_BASE_MS, + maxBackoffMillis = YAHOO_AUTH_BACKOFF_MAX_MS, + circuitOpenThreshold = YAHOO_AUTH_CIRCUIT_OPEN_THRESHOLD, + circuitOpenMillis = YAHOO_AUTH_CIRCUIT_OPEN_MS, + maxConcurrentConnections = YAHOO_MAX_CONCURRENT_CONNECTIONS, + folderIndexCap = YAHOO_FOLDER_INDEX_CAP, + ) + + /** + * The policy for [host] (the account's IMAP host). Yahoo/AOL — matched via the single source of truth + * [MailProvider.forImapHost], including host aliases — get [YAHOO_AOL]; everything else, including a + * null/blank or unknown host, gets [AuthCadencePolicy.DISABLED]. + */ + fun forHost(host: String): AuthCadencePolicy = when (MailProvider.forImapHost(host)) { + MailProvider.YAHOO, MailProvider.AOL -> YAHOO_AOL + else -> AuthCadencePolicy.DISABLED + } +} diff --git a/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt b/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt index 4a9ad75..16b83b9 100644 --- a/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt +++ b/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt @@ -44,8 +44,10 @@ import org.libremail.domain.model.AccountSettings import org.libremail.domain.model.ImapConnectionParams import org.libremail.domain.model.MailSecurity import org.libremail.domain.repository.MailRepository +import org.libremail.mail.AuthThrottleGate import org.libremail.mail.FetchedMessage import org.libremail.mail.ImapClient +import org.libremail.mail.ProviderAuthPolicy import org.libremail.power.BatteryStatus import org.libremail.power.BatteryStatusProvider import org.libremail.reporting.AppLog @@ -530,6 +532,37 @@ class MailBackfillerTest { assertTrue(logBuffer.snapshot().any { it.message.startsWith("backfill skip acct:") }) } + /** + * Issue #362 composes with #360/#356 by reusing the same skip: an account inside its proactive + * *auth*-backoff window is skipped exactly as a reactively-throttled one is — no server call, no + * `moreWork` (so [BackfillPacer] does not spin a cooldown on it), and the account is left alone so + * backfill never nudges a Yahoo/AOL account toward its ~1-hour lockout. + */ + @Test + fun `an account inside its auth-backoff window is skipped, not paged`() = runTest { + cached += fetchedMessage(uid = "60").toEntity("acct", "INBOX") + val imapClient = mockk(relaxed = true) + // Treat the (127.0.0.1) test host as a gated Yahoo/AOL account and arm the backoff on its identity. + val authGate = AuthThrottleGate( + nowMillis = { 0L }, + random = { 0.0 }, + policyForHost = { ProviderAuthPolicy.forHost("imap.mail.yahoo.com") }, + ) + authGate.onAuthFailure(params()) + + val moreWork = backfiller(AccountSettings("acct"), imapClient = imapClient, authGate = authGate).runBackfill() + + assertFalse(moreWork, "a slice whose only account is auth-blocked reports done, not more-work") + coVerify(exactly = 0) { imapClient.fetchOlderThan(any(), any(), any(), any()) } + assertTrue(authGate.isAuthBlocked(params()), "the account is still auth-backing-off") + assertTrue( + logBuffer.snapshot().any { + it.message.startsWith("backfill skip acct:") && it.message.contains("auth backing off") + }, + "a PII-free auth-skip breadcrumb is recorded", + ) + } + // --- issue #355: interactive-fetch priority ------------------------------------------------- /** @@ -686,6 +719,7 @@ class MailBackfillerTest { imapClient: ImapClient = client, throttleGate: AccountThrottleGate = AccountThrottleGate(), interactiveGate: InteractiveImapGate = InteractiveImapGate(), + authGate: AuthThrottleGate = AuthThrottleGate(), ): MailBackfiller { val accountDao = mockk() coEvery { accountDao.getAll() } returns listOf(accountEntity) @@ -743,6 +777,7 @@ class MailBackfillerTest { maintenanceGate = MailMaintenanceGate(), throttleGate = throttleGate, interactiveGate = interactiveGate, + authGate = authGate, ).also { lastMessageDao = messageDao lastMailRepository = mailRepository diff --git a/app/src/test/kotlin/org/libremail/data/sync/MailMaintenanceGateTest.kt b/app/src/test/kotlin/org/libremail/data/sync/MailMaintenanceGateTest.kt index 410f941..83d6c97 100644 --- a/app/src/test/kotlin/org/libremail/data/sync/MailMaintenanceGateTest.kt +++ b/app/src/test/kotlin/org/libremail/data/sync/MailMaintenanceGateTest.kt @@ -29,6 +29,7 @@ import org.libremail.data.settings.SettingsRepository import org.libremail.domain.model.AccountSettings import org.libremail.domain.model.ImapConnectionParams import org.libremail.domain.model.MailSecurity +import org.libremail.mail.AuthThrottleGate import org.libremail.mail.FetchedMessage import org.libremail.mail.ImapClient import org.libremail.power.BatteryStatus @@ -202,6 +203,7 @@ class MailMaintenanceGateTest { maintenanceGate = gate, throttleGate = AccountThrottleGate(), interactiveGate = InteractiveImapGate(), + authGate = AuthThrottleGate(), ) } diff --git a/app/src/test/kotlin/org/libremail/data/sync/MailSyncConcurrencyTest.kt b/app/src/test/kotlin/org/libremail/data/sync/MailSyncConcurrencyTest.kt index c659e8e..cfe13ab 100644 --- a/app/src/test/kotlin/org/libremail/data/sync/MailSyncConcurrencyTest.kt +++ b/app/src/test/kotlin/org/libremail/data/sync/MailSyncConcurrencyTest.kt @@ -29,6 +29,7 @@ import org.libremail.data.settings.SettingsRepository import org.libremail.domain.model.AccountSettings import org.libremail.domain.model.ImapConnectionParams import org.libremail.domain.model.MailSecurity +import org.libremail.mail.AuthThrottleGate import org.libremail.mail.FetchedMessage import org.libremail.mail.ImapClient import org.libremail.power.BatteryStatus @@ -362,6 +363,7 @@ class MailSyncConcurrencyTest { maintenanceGate = MailMaintenanceGate(), throttleGate = AccountThrottleGate(), interactiveGate = InteractiveImapGate(), + authGate = AuthThrottleGate(), ) } diff --git a/app/src/test/kotlin/org/libremail/mail/AuthBackoffTest.kt b/app/src/test/kotlin/org/libremail/mail/AuthBackoffTest.kt new file mode 100644 index 0000000..417f642 --- /dev/null +++ b/app/src/test/kotlin/org/libremail/mail/AuthBackoffTest.kt @@ -0,0 +1,104 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.mail + +import org.junit.Test +import org.libremail.domain.model.MailProvider +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertTrue + +/** + * The pure proactive auth-backoff schedule (issue #362): an exponential *ramp* (equal jitter, capped) + * up to the circuit-open threshold, then a long *fixed* open-circuit window — "back off long and stop". + * A disabled policy never blocks. Deterministic (the caller supplies the jitter draw), so no clock or + * randomness leaks into the assertions. + */ +class AuthBackoffTest { + + /** A small controlled policy so the ramp/cap arithmetic reads clearly (base 1s, cap 8s, circuit 60s). */ + private val policy = AuthCadencePolicy( + enabled = true, + baseBackoffMillis = 1_000L, + maxBackoffMillis = 8_000L, + circuitOpenThreshold = 4, + circuitOpenMillis = 60_000L, + maxConcurrentConnections = 5, + folderIndexCap = 10_000, + ) + + /** random = 0.0 selects the lower jitter bound (capped/2); random = 1.0 selects the upper (capped). */ + private fun low(failures: Int, p: AuthCadencePolicy = policy) = AuthBackoff.blockMillis(p, failures, random = 0.0) + private fun high(failures: Int, p: AuthCadencePolicy = policy) = AuthBackoff.blockMillis(p, failures, random = 1.0) + + @Test + fun `the ramp doubles per failure at the lower jitter bound`() { + assertEquals(policy.baseBackoffMillis / 2, low(1)) // 500ms + assertEquals(policy.baseBackoffMillis, low(2)) // 1000ms + assertEquals(policy.baseBackoffMillis * 2, low(3)) // 2000ms + } + + @Test + fun `the upper jitter bound of the first failure is the base delay`() { + assertEquals(policy.baseBackoffMillis, high(1)) + } + + @Test + fun `jitter keeps every ramp draw within the exponential half-window`() { + // Failure 2's capped target is 2*base; equal jitter must land in [base, 2*base] for any draw. + val lower = policy.baseBackoffMillis + val upper = policy.baseBackoffMillis * 2 + for (thousandths in 0..1000) { + val delay = AuthBackoff.blockMillis(policy, consecutiveFailures = 2, random = thousandths / 1000.0) + assertTrue(delay in lower..upper, "draw $thousandths gave $delay, outside [$lower,$upper]") + } + } + + @Test + fun `the ramp is capped at the policy maximum`() { + // Raise the threshold so the ramp actually reaches the cap: failure 5 wants 16*base > cap. + val ramped = policy.copy(circuitOpenThreshold = 100) + assertEquals(ramped.maxBackoffMillis / 2, low(5, ramped)) + assertEquals(ramped.maxBackoffMillis, high(5, ramped)) + } + + @Test + fun `at the threshold the circuit opens to a long fixed window with no jitter`() { + // Failure 4 (== threshold) and beyond return the fixed open-circuit window for any jitter draw. + assertEquals(policy.circuitOpenMillis, low(4)) + assertEquals(policy.circuitOpenMillis, high(4)) + assertEquals(policy.circuitOpenMillis, low(9)) + assertTrue( + policy.circuitOpenMillis > policy.maxBackoffMillis, + "the open-circuit window is longer than the ramp cap — we have given up probing", + ) + } + + @Test + fun `a disabled policy never blocks`() { + assertEquals(0L, high(1, AuthCadencePolicy.DISABLED)) + assertEquals(0L, high(50, AuthCadencePolicy.DISABLED)) + } + + @Test + fun `a failure count below one is rejected`() { + assertFailsWith { + AuthBackoff.blockMillis(policy, consecutiveFailures = 0, random = 0.0) + } + } + + @Test + fun `the real yahoo schedule keeps every wait under the one-hour lockout`() { + // The heart of issue #362: however many times in a row auth fails, no single block reaches the + // ~1-hour lockout window — the backoff protects the account without ever matching the punishment. + val yahooHost = MailProvider.YAHOO.createAccount("x@yahoo.com").imap.host + val yahoo = ProviderAuthPolicy.forHost(yahooHost) + for (failures in 1..12) { + assertTrue(low(failures, yahoo) < ONE_HOUR_MS, "failure $failures low bound reached the lockout window") + assertTrue(high(failures, yahoo) < ONE_HOUR_MS, "failure $failures high bound reached the lockout window") + } + } + + private companion object { + const val ONE_HOUR_MS = 60 * 60_000L + } +} diff --git a/app/src/test/kotlin/org/libremail/mail/AuthThrottleGateTest.kt b/app/src/test/kotlin/org/libremail/mail/AuthThrottleGateTest.kt new file mode 100644 index 0000000..42e4e13 --- /dev/null +++ b/app/src/test/kotlin/org/libremail/mail/AuthThrottleGateTest.kt @@ -0,0 +1,237 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.mail + +import io.mockk.every +import io.mockk.mockkStatic +import io.mockk.unmockkAll +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.advanceTimeBy +import kotlinx.coroutines.test.runTest +import org.junit.After +import org.junit.Before +import org.junit.Test +import org.libremail.data.sync.AccountThrottleGate +import org.libremail.data.sync.ThrottleBackoff +import org.libremail.data.sync.ThrottleKind +import org.libremail.data.sync.ThrottleSignal +import org.libremail.domain.model.ImapConnectionParams +import org.libremail.domain.model.MailProvider +import org.libremail.domain.model.MailSecurity +import org.libremail.reporting.AppLog +import org.libremail.reporting.RingLogBuffer +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +/** + * [AuthThrottleGate] (issue #362) must, for a Yahoo/AOL account: block logins after an auth failure, + * escalate rapid consecutive failures without ever reaching the ~1-hour lockout window, open a long + * fixed circuit past the threshold, isolate accounts, reset on success, expose an accurate remaining + * window (proven against coroutines-test virtual time), and log only PII-free breadcrumbs. It must be a + * total no-op for a non-gated host, and it must *compose* with issue #360's reactive gate rather than + * fight it. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class AuthThrottleGateTest { + + private val logBuffer = RingLogBuffer() + + /** A manual virtual clock for the non-timing tests; [gate] reads it live, so tests advance it by hand. */ + private var now = 0L + + /** The real Yahoo policy — the production values under test. */ + private val yahoo = ProviderAuthPolicy.forHost(MailProvider.YAHOO.createAccount("x@yahoo.com").imap.host) + + /** random = 0.0 makes the equal-jitter draw deterministic (always the lower bound of the ramp). */ + private fun gate(random: () -> Double = { 0.0 }, policyForHost: (String) -> AuthCadencePolicy = { yahoo }) = + AuthThrottleGate(nowMillis = { now }, random = random, policyForHost = policyForHost) + + private fun params(user: String = "user@example.org", host: String = "imap.mail.yahoo.com") = + ImapConnectionParams(host, PORT, MailSecurity.SSL_TLS, user, secret = "secret", useXoauth2 = false) + + @Before + fun setUp() { + // AppLog forwards to android.util.Log, a throwing no-op stub under plain JVM unit tests. Fully + // qualified so this file never imports android.util.Log (a detekt-forbidden import, epic #324). + mockkStatic(android.util.Log::class) + every { android.util.Log.i(any(), any()) } returns 0 + every { android.util.Log.w(any(), any()) } returns 0 + AppLog.install(logBuffer) + } + + @After + fun tearDown() = unmockkAll() + + @Test + fun `onAuthFailure blocks the account and returns the backoff`() { + val gate = gate() + val p = params() + + val backoff = gate.onAuthFailure(p) + + assertTrue(backoff > 0L) + assertTrue(gate.isAuthBlocked(p)) + assertEquals(backoff, gate.remainingAuthBlockMillis(p)) + } + + @Test + fun `rapid consecutive auth failures escalate the block within the ramp`() { + val gate = gate() + val p = params() + + val first = gate.onAuthFailure(p) + val second = gate.onAuthFailure(p) + val third = gate.onAuthFailure(p) + + assertTrue(second > first, "a consecutive failure must back off longer ($second !> $first)") + assertTrue(third > second, "and longer again ($third !> $second)") + } + + @Test + fun `rapid auth failures never reach the one-hour lockout window`() { + val gate = gate() + val p = params() + + // Hammer a wrong credential as fast as an unguarded loop would: every resulting block must stay + // well under the ~1-hour lockout it exists to prevent — that is the whole point of issue #362. + repeat(RAPID_FAILURES) { + val block = gate.onAuthFailure(p) + assertTrue(block < ThrottleBackoff.LOCKOUT_BASE_MS, "block $block reached the 1-hour lockout window") + } + } + + @Test + fun `the circuit opens after the threshold to a long fixed window and stays open`() { + val gate = gate() + val p = params() + + var lastBlock = 0L + repeat(yahoo.circuitOpenThreshold) { lastBlock = gate.onAuthFailure(p) } + assertEquals(yahoo.circuitOpenMillis, lastBlock, "the threshold failure opens the fixed circuit window") + + // A further failure stays open at the same fixed window (no runaway escalation). + assertEquals(yahoo.circuitOpenMillis, gate.onAuthFailure(p)) + assertTrue(gate.isAuthBlocked(p)) + } + + @Test + fun `onAuthSuccess clears the backoff and resets the failure count`() { + val gate = gate() + val p = params() + + val first = gate.onAuthFailure(p) + gate.onAuthFailure(p) // escalate to failure 2 + gate.onAuthSuccess(p) + + assertFalse(gate.isAuthBlocked(p)) + // A fresh failure after recovery starts back at the base (failure 1) delay. + assertEquals(first, gate.onAuthFailure(p)) + } + + @Test + fun `a blocked account never stalls another`() { + val gate = gate() + val blocked = params(user = "blocked@example.org") + val healthy = params(user = "healthy@example.org") + + gate.onAuthFailure(blocked) + + assertTrue(gate.isAuthBlocked(blocked)) + assertFalse(gate.isAuthBlocked(healthy)) + assertEquals(0L, gate.remainingAuthBlockMillis(healthy)) + } + + @Test + fun `an elapsed window stops blocking but still escalates a re-failure`() { + val gate = gate() + val p = params() + + val first = gate.onAuthFailure(p) + now += first // the window elapses + assertFalse(gate.isAuthBlocked(p), "the account may attempt a login once its window passes") + + // Re-failing before any success keeps the failure count — it escalates, not restarts. + val next = gate.onAuthFailure(p) + assertTrue(next > first) + } + + @Test + fun `the block window clears exactly when the backoff elapses`() = runTest { + val gate = AuthThrottleGate( + nowMillis = { testScheduler.currentTime }, + random = { 0.0 }, + policyForHost = { yahoo }, + ) + val p = params() + + val backoff = gate.onAuthFailure(p) + assertTrue(gate.isAuthBlocked(p)) + + advanceTimeBy(backoff - 1) + assertTrue(gate.isAuthBlocked(p), "still blocked just before the window elapses") + + advanceTimeBy(1) + assertFalse(gate.isAuthBlocked(p), "cleared the instant the backoff elapses") + assertEquals(0L, gate.remainingAuthBlockMillis(p)) + } + + @Test + fun `a non-gated host is never blocked`() { + // Gmail has no 1-hour auth lockout, so its real policy is DISABLED: failures record nothing. + val gate = gate(policyForHost = ProviderAuthPolicy::forHost) + val gmail = params(host = "imap.gmail.com") + + assertEquals(0L, gate.onAuthFailure(gmail)) + assertFalse(gate.isAuthBlocked(gmail)) + assertTrue(logBuffer.snapshot().isEmpty(), "an ungated host logs no auth-backoff breadcrumb") + } + + @Test + fun `auth backoff and recovery log PII-free breadcrumbs`() { + val gate = gate() + val p = params(user = "secret.user@example.org", host = "imap.mail.yahoo.com") + + gate.onAuthFailure(p) + gate.onAuthSuccess(p) + + val messages = logBuffer.snapshot().map { it.message } + assertTrue(messages.any { it.startsWith("auth backoff acct:") && it.contains("failures=1") }) + assertTrue(messages.any { it.startsWith("auth recovered acct:") }) + messages.forEach { + assertFalse(it.contains("secret.user@example.org"), it) + assertFalse(it.contains("yahoo"), it) + } + } + + @Test + fun `onAuthSuccess on a healthy account is silent`() { + gate().onAuthSuccess(params()) + + assertTrue(logBuffer.snapshot().isEmpty()) + } + + @Test + fun `composes with the reactive throttle gate without interference (issue #360)`() { + val authGate = gate() + val throttleGate = AccountThrottleGate(nowMillis = { now }, random = { 0.0 }) + val p = params() + + // The same account can be BOTH proactively auth-backing-off and reactively throttled; the two + // gates keep independent state and neither clears the other. + val authBlock = authGate.onAuthFailure(p) + val lockout = throttleGate.onThrottle("acct", ThrottleSignal(ThrottleKind.LOCKOUT)) + assertTrue(authGate.isAuthBlocked(p)) + assertTrue(throttleGate.isThrottled("acct")) + authGate.onAuthSuccess(p) + assertFalse(authGate.isAuthBlocked(p)) + assertTrue(throttleGate.isThrottled("acct"), "clearing the auth gate must not clear the reactive one") + + // The proactive auth backoff is always shorter than the reactive lockout it prevents reaching. + assertTrue(authBlock < lockout, "proactive auth backoff ($authBlock) must be shorter than lockout ($lockout)") + } + + private companion object { + const val PORT = 993 + const val RAPID_FAILURES = 10 + } +} diff --git a/app/src/test/kotlin/org/libremail/mail/ImapClientAuthBackoffTest.kt b/app/src/test/kotlin/org/libremail/mail/ImapClientAuthBackoffTest.kt new file mode 100644 index 0000000..b751d42 --- /dev/null +++ b/app/src/test/kotlin/org/libremail/mail/ImapClientAuthBackoffTest.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.ServerSetupTest +import io.mockk.every +import io.mockk.mockkStatic +import io.mockk.unmockkAll +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.MailProvider +import org.libremail.domain.model.MailSecurity +import java.net.ServerSocket +import kotlin.test.assertFailsWith +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +/** + * The issue-#362 auth circuit-breaker's **enforcement** inside [ImapClient], against a real GreenMail + * IMAP server. Proves the load-bearing distinctions end to end: + * - a rejected `LOGIN` (auth failure) **arms** the proactive backoff; + * - a transient connect error (connection refused) **does not** — "transient IMAP error ≠ auth backoff"; + * - while backing off, a subsequent login is **skipped** ([AuthBackoffException]) rather than attempted, + * even with a correct credential, so no failed-login storm can reach the provider; + * - a successful login leaves (or clears) the backoff. + * + * The gate is injected with an always-Yahoo policy and a manual clock, so a GreenMail server on + * `127.0.0.1` is treated as a gated Yahoo/AOL account and the backoff window is controllable without + * real sleeps. Both the connect-per-op and connection-reuse code paths funnel through the same guarded + * `openConnectedStore`, so a reuse-on client is checked too. + */ +class ImapClientAuthBackoffTest { + + private lateinit var greenMail: GreenMail + private var now = 0L + private val yahoo = ProviderAuthPolicy.forHost(MailProvider.YAHOO.createAccount("x@yahoo.com").imap.host) + private val gate = AuthThrottleGate(nowMillis = { now }, random = { 0.0 }, policyForHost = { yahoo }) + + private fun client(reuse: Boolean = false) = ImapClient(reuseConnections = reuse, authGate = gate) + + @Before + fun setUp() { + greenMail = GreenMail(ServerSetupTest.SMTP_IMAP) + greenMail.start() + greenMail.setUser("alice@example.org", "secret") + mockkStatic(android.util.Log::class) + every { android.util.Log.d(any(), any()) } returns 0 + every { android.util.Log.i(any(), any()) } returns 0 + every { android.util.Log.w(any(), any()) } returns 0 + } + + @After + fun tearDown() { + greenMail.stop() + unmockkAll() + } + + private fun params(secret: String = "secret", port: Int = greenMail.imap.port) = ImapConnectionParams( + host = "127.0.0.1", + port = port, + security = MailSecurity.NONE, + username = "alice@example.org", + secret = secret, + useXoauth2 = false, + ) + + @Test + fun `a wrong-password auth failure arms the proactive backoff`() = runTest { + assertFailsWith { client().listFolders(params(secret = "wrong-password")) } + + assertTrue(gate.isAuthBlocked(params()), "a rejected LOGIN must arm the auth circuit-breaker") + } + + @Test + fun `a transient connect error does not arm the backoff`() = runTest { + val closedPort = ServerSocket(0).use { it.localPort } // now free → connection refused, not an auth NO + + assertFailsWith { client().listFolders(params(port = closedPort)) } + + assertFalse( + gate.isAuthBlocked(params(port = closedPort)), + "a transient (non-auth) connect error must NOT arm the auth backoff", + ) + } + + @Test + fun `a successful login leaves the backoff clear`() = runTest { + client().listFolders(params()) + + assertFalse(gate.isAuthBlocked(params())) + } + + @Test + fun `while backing off, a login is skipped even with a correct credential`() = runTest { + // Arm the backoff with one rejected login... + assertFailsWith { client().listFolders(params(secret = "wrong-password")) } + assertTrue(gate.isAuthBlocked(params())) + + // ...then further logins are SKIPPED (AuthBackoffException from the guard), not attempted — even a + // correct credential and a different operation, so no failed-login storm reaches the provider. + assertFailsWith { client().listFolders(params()) } + assertFailsWith { client().fetchRecent(params(), "INBOX", 10) } + } + + @Test + fun `the reuse path is gated too`() = runTest { + // The connection-reuse client establishes its warm socket via the same guarded openConnectedStore. + assertFailsWith { client(reuse = true).listFolders(params(secret = "wrong-password")) } + assertTrue(gate.isAuthBlocked(params())) + assertFailsWith { client(reuse = true).listFolders(params()) } + } + + @Test + fun `a login succeeds again once the backoff window elapses`() = runTest { + assertFailsWith { client().listFolders(params(secret = "wrong-password")) } + now += gate.remainingAuthBlockMillis(params()) // fast-forward past the window + + client().listFolders(params()) // a correct login now goes through and clears the state + + assertFalse(gate.isAuthBlocked(params())) + } +} diff --git a/app/src/test/kotlin/org/libremail/mail/ProviderAuthPolicyTest.kt b/app/src/test/kotlin/org/libremail/mail/ProviderAuthPolicyTest.kt new file mode 100644 index 0000000..dd7155f --- /dev/null +++ b/app/src/test/kotlin/org/libremail/mail/ProviderAuthPolicyTest.kt @@ -0,0 +1,82 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.mail + +import org.junit.Test +import org.libremail.domain.model.MailProvider +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +/** + * [ProviderAuthPolicy] must gate **only** Yahoo/AOL — the providers with the ~1-hour auth lockout + * (issue #362) — with the conservative, lockout-avoiding cadence, and leave every other host + * ([AuthCadencePolicy.DISABLED]) so siblings #361/#363/#364 and manual servers are unaffected. It must + * also expose the documented connection / folder-index ceilings, and its whole schedule must stay under + * the ~1-hour lockout window it protects. + */ +class ProviderAuthPolicyTest { + + /** The real IMAP host a provider's accounts use — the single source of truth, not a hard-coded string. */ + private fun hostOf(provider: MailProvider) = provider.createAccount("user@example.com").imap.host + + @Test + fun `yahoo and aol get the enabled lockout-avoiding policy`() { + for (provider in listOf(MailProvider.YAHOO, MailProvider.AOL)) { + val policy = ProviderAuthPolicy.forHost(hostOf(provider)) + assertTrue(policy.enabled, "${provider.displayName} must be gated against its 1-hour auth lockout") + assertEquals(ProviderAuthPolicy.YAHOO_AUTH_BACKOFF_BASE_MS, policy.baseBackoffMillis) + assertEquals(ProviderAuthPolicy.YAHOO_AUTH_BACKOFF_MAX_MS, policy.maxBackoffMillis) + assertEquals(ProviderAuthPolicy.YAHOO_AUTH_CIRCUIT_OPEN_THRESHOLD, policy.circuitOpenThreshold) + assertEquals(ProviderAuthPolicy.YAHOO_AUTH_CIRCUIT_OPEN_MS, policy.circuitOpenMillis) + assertEquals(ProviderAuthPolicy.YAHOO_MAX_CONCURRENT_CONNECTIONS, policy.maxConcurrentConnections) + assertEquals(ProviderAuthPolicy.YAHOO_FOLDER_INDEX_CAP, policy.folderIndexCap) + } + } + + @Test + fun `gmail and icloud are not gated`() { + for (provider in listOf(MailProvider.GMAIL, MailProvider.ICLOUD)) { + assertFalse( + ProviderAuthPolicy.forHost(hostOf(provider)).enabled, + "${provider.displayName} has no 1-hour auth lockout; issue #362 must leave it ungated", + ) + } + } + + @Test + fun `outlook, unknown, and blank hosts are not gated`() { + assertFalse(ProviderAuthPolicy.forHost("outlook.office365.com").enabled) + assertFalse(ProviderAuthPolicy.forHost("imap.example.com").enabled) + assertFalse(ProviderAuthPolicy.forHost("").enabled) + } + + @Test + fun `the disabled policy can never block a login`() { + // A disabled policy's threshold is unreachable and its windows are zero, so a non-Yahoo/AOL + // account is provably never gated regardless of how [AuthBackoff] is called. + assertFalse(AuthCadencePolicy.DISABLED.enabled) + assertEquals(Int.MAX_VALUE, AuthCadencePolicy.DISABLED.circuitOpenThreshold) + assertEquals(0L, AuthBackoff.blockMillis(AuthCadencePolicy.DISABLED, consecutiveFailures = 1, random = 1.0)) + } + + @Test + fun `the whole yahoo schedule stays under the one-hour lockout window`() { + // Recovery must beat the lockout: every wait the policy can impose is under an hour, so a + // recovered account resumes far sooner than a self-inflicted hour of silence. + assertTrue(ProviderAuthPolicy.YAHOO_AUTH_BACKOFF_BASE_MS < ONE_HOUR_MS) + assertTrue(ProviderAuthPolicy.YAHOO_AUTH_BACKOFF_MAX_MS < ONE_HOUR_MS) + assertTrue(ProviderAuthPolicy.YAHOO_AUTH_CIRCUIT_OPEN_MS < ONE_HOUR_MS) + } + + @Test + fun `the documented connection and folder-index ceilings are exposed`() { + // Reuse (issues #125/#357, ON by default) keeps an account to ~1 warm IMAP socket + at most one + // IDLE connection = 2, comfortably under this documented ceiling of 5. + assertEquals(5, ProviderAuthPolicy.YAHOO_MAX_CONCURRENT_CONNECTIONS) + assertEquals(10_000, ProviderAuthPolicy.YAHOO_FOLDER_INDEX_CAP) + } + + private companion object { + const val ONE_HOUR_MS = 60 * 60_000L + } +} -- 2.47.3 From 4d1f2ae9ca131ddb3e9d8ab445e5e9316779d8ce Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 8 Jul 2026 23:32:56 -0500 Subject: [PATCH 2/3] fix(yahoo): fail loud and error the account after 4 auth failures (#362) Change the Yahoo/AOL proactive auth circuit-breaker's after-threshold behaviour from a silent, self-clearing 30-min open-circuit window to a fail-loud, permanent stop that surfaces to the user. - AuthThrottleGate: past circuitOpenThreshold the circuit now LATCHES (blockedUntilMillis = Long.MAX_VALUE) instead of opening a self-clearing window. onAuthSuccess never clears a latch; only onAccountReadded does. The 1-4 spaced-retry ramp and all thresholds/timings are unchanged, and only genuine AuthenticationFailedExceptions still count. - Persistent, user-visible error state: new nullable AccountEntity.authError (AccountDatabase migration v2 -> v3 + exported schema) + Account domain field + mappers + AccountDao.setAuthError (idempotent conditional write). markAccountErroredIfLatched bridges the in-memory latch to the row from MailSyncer/MailBackfiller (which hold the Account), keeping the DAO out of the mail layer. Both loops also skip a latched/errored account entirely (durable across restarts), and a re-add clears the latch + the row error. - UI: a persistent red indicator + the message on the Settings account row (AccountReorderList) and the drawer switcher (FolderDrawer), plus a persistent banner atop the mailbox for the shown account (MailboxScreen + MailboxViewModel.accountAuthError). String: "Please remove and re-add this account with valid credentials". - All AppLog breadcrumbs stay PII-free (hashed accountLogRef only). Also merges origin/main, composing authGate with #469's GmailBandwidthTracker in MailBackfiller/MailSyncer constructors and every test construction site. Tests: gate latch (no self-clear / success can't clear / re-add clears), markAccountErroredIfLatched, MailSyncer/MailBackfiller errored+latched skips, repository reset-on-re-add, mapper round-trip, v2->v3 migration, DAO set/clear, the mailbox banner + Settings-row Compose renders, and the on-device gate latch. Full fast gate green (JDK 21). --- .../3.json | 246 ++++++++++++++++++ .../libremail/data/local/AccountDaoTest.kt | 21 ++ .../data/local/AccountMigrationTest.kt | 22 ++ ...ntAddCredentialOrderingInstrumentedTest.kt | 2 + .../mail/AuthThrottleGateInstrumentedTest.kt | 25 +- .../libremail/data/local/AccountDatabase.kt | 2 +- .../libremail/data/local/AccountMigrations.kt | 13 + .../org/libremail/data/local/Mappers.kt | 2 + .../libremail/data/local/dao/AccountDao.kt | 10 + .../data/local/entity/AccountEntity.kt | 8 + .../data/repository/AccountRepositoryImpl.kt | 16 +- .../libremail/data/sync/AccountAuthError.kt | 50 ++++ .../org/libremail/data/sync/MailBackfiller.kt | 14 +- .../org/libremail/data/sync/MailSyncer.kt | 138 ++++++---- .../org/libremail/di/AccountDatabaseModule.kt | 3 +- .../org/libremail/domain/model/Account.kt | 7 + .../org/libremail/mail/AuthThrottleGate.kt | 106 ++++++-- .../org/libremail/ui/mailbox/FolderDrawer.kt | 26 ++ .../org/libremail/ui/mailbox/MailboxScreen.kt | 33 +++ .../libremail/ui/mailbox/MailboxViewModel.kt | 13 + .../ui/settings/AccountReorderList.kt | 31 ++- app/src/main/res/values/strings.xml | 6 + .../org/libremail/data/local/MappersTest.kt | 18 ++ .../repository/AccountRepositoryImplTest.kt | 25 ++ .../data/sync/AccountAuthErrorTest.kt | 143 ++++++++++ .../libremail/data/sync/MailBackfillerTest.kt | 53 ++++ .../data/sync/MailSyncConcurrencyTest.kt | 1 + .../org/libremail/data/sync/MailSyncerTest.kt | 73 +++++- .../libremail/mail/AuthThrottleGateTest.kt | 52 +++- .../ui/mailbox/MailboxScreenJvmTest.kt | 15 ++ .../settings/AccountReorderListRenderTest.kt | 74 ++++++ config/detekt/detekt.yml | 7 +- 32 files changed, 1161 insertions(+), 94 deletions(-) create mode 100644 app/schemas/org.libremail.data.local.AccountDatabase/3.json create mode 100644 app/src/main/kotlin/org/libremail/data/sync/AccountAuthError.kt create mode 100644 app/src/test/kotlin/org/libremail/data/sync/AccountAuthErrorTest.kt create mode 100644 app/src/test/kotlin/org/libremail/ui/settings/AccountReorderListRenderTest.kt diff --git a/app/schemas/org.libremail.data.local.AccountDatabase/3.json b/app/schemas/org.libremail.data.local.AccountDatabase/3.json new file mode 100644 index 0000000..6acf521 --- /dev/null +++ b/app/schemas/org.libremail.data.local.AccountDatabase/3.json @@ -0,0 +1,246 @@ +{ + "formatVersion": 1, + "database": { + "version": 3, + "identityHash": "553c7a19d228b3ba8f8a750232b4c7d9", + "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, `sortOrder` INTEGER NOT NULL DEFAULT 0, `authError` TEXT, `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": "sortOrder", + "columnName": "sortOrder", + "affinity": "INTEGER", + "notNull": true, + "defaultValue": "0" + }, + { + "fieldPath": "authError", + "columnName": "authError", + "affinity": "TEXT" + }, + { + "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, '553c7a19d228b3ba8f8a750232b4c7d9')" + ] + } +} \ No newline at end of file diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDaoTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDaoTest.kt index be830a2..225f38b 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDaoTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDaoTest.kt @@ -86,4 +86,25 @@ class AccountDaoTest { assertNull(dao.getById("acct")) } + + @Test + fun setAuthErrorStampsTheMessageIdempotentlyAndAReAddClearsIt() = runBlocking { + dao.upsert(account("acct", "ada@example.org")) + assertNull("a fresh account carries no error", dao.getById("acct")?.authError) + + // The conditional UPDATE stamps the message and reports one row changed (issue #362)... + assertEquals(1, dao.setAuthError("acct", MESSAGE)) + assertEquals(MESSAGE, dao.getById("acct")?.authError) + // ...and is idempotent: re-writing the same message changes nothing (so the caller logs once). + assertEquals(0, dao.setAuthError("acct", MESSAGE)) + + // A re-add rewrites the row from a fresh (null-authError) entity, clearing the error — the + // clear-on-re-add path AccountRepositoryImpl relies on (insertAtEnd's in-place update). + dao.upsert(account("acct", "ada@example.org")) + assertNull("re-adding the account clears the persisted error", dao.getById("acct")?.authError) + } + + private companion object { + const val MESSAGE = "Please remove and re-add this account with valid credentials" + } } diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/AccountMigrationTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/AccountMigrationTest.kt index 7593a44..f5845ab 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/AccountMigrationTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/AccountMigrationTest.kt @@ -63,6 +63,28 @@ class AccountMigrationTest { db.close() } + /** + * v2 -> v3 (issue #362): a nullable `accounts.authError` appears, and existing accounts migrate to NULL + * ("healthy — no error"). runMigrationsAndValidate confirms the resulting schema matches the exported v3 + * JSON, so any drift between the plain ADD COLUMN and the entity would fail here, not at a user's first + * open. + */ + @Test + fun migrate2To3_addsNullableAuthErrorDefaultingToNull() { + helper.createDatabase(TEST_DB, 2).apply { + insertAccount("a", "ada@example.org") + close() + } + + val db = helper.runMigrationsAndValidate(TEST_DB, 3, true, ACCOUNT_MIGRATION_2_3) + + db.query("SELECT authError FROM accounts WHERE id = 'a'").use { c -> + assertTrue(c.moveToFirst()) + assertTrue("an existing account migrates to a null (healthy) authError", c.isNull(0)) + } + db.close() + } + private fun SupportSQLiteDatabase.insertAccount(id: String, email: String) { execSQL( "INSERT INTO accounts (id, email, displayName, authType, imap_host, imap_port, imap_security, " + diff --git a/app/src/androidTest/kotlin/org/libremail/data/repository/AccountAddCredentialOrderingInstrumentedTest.kt b/app/src/androidTest/kotlin/org/libremail/data/repository/AccountAddCredentialOrderingInstrumentedTest.kt index 418d6c7..84675e4 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/repository/AccountAddCredentialOrderingInstrumentedTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/repository/AccountAddCredentialOrderingInstrumentedTest.kt @@ -32,6 +32,7 @@ import org.libremail.domain.model.Account import org.libremail.domain.model.AuthType import org.libremail.domain.model.MailSecurity import org.libremail.domain.model.ServerConfig +import org.libremail.mail.AuthThrottleGate import org.libremail.mail.FetchedFolder import org.libremail.mail.ImapClient import org.libremail.notifications.MailNotifier @@ -78,6 +79,7 @@ class AccountAddCredentialOrderingInstrumentedTest { draftDao = mockk(relaxed = true), credentialStore = credentialStore, imapClient = imapClient, + authGate = mockk(relaxed = true), syncScheduler = mockk(relaxed = true), accountSettingsRepository = mockk(relaxed = true), mailNotifier = mockk(relaxed = true), diff --git a/app/src/androidTest/kotlin/org/libremail/mail/AuthThrottleGateInstrumentedTest.kt b/app/src/androidTest/kotlin/org/libremail/mail/AuthThrottleGateInstrumentedTest.kt index acb11a2..b91be10 100644 --- a/app/src/androidTest/kotlin/org/libremail/mail/AuthThrottleGateInstrumentedTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/mail/AuthThrottleGateInstrumentedTest.kt @@ -53,15 +53,27 @@ class AuthThrottleGateInstrumentedTest { } @Test - fun theCircuitOpensToAFixedWindowPastTheThreshold() { + fun theCircuitLatchesPastTheThresholdAndNeverSelfClears() { val gate = gate() val p = params() - var last = 0L - repeat(yahoo.circuitOpenThreshold) { last = gate.onAuthFailure(p) } + repeat(yahoo.circuitOpenThreshold) { gate.onAuthFailure(p) } + assertTrue("reaching the threshold latches the circuit", gate.isAuthLatched(p)) + assertTrue(gate.isAuthBlocked(p)) - assertEquals("the threshold failure opens the fixed circuit window", yahoo.circuitOpenMillis, last) - assertEquals("and it stays open at that window", yahoo.circuitOpenMillis, gate.onAuthFailure(p)) + // Issue #362 fail-loud stop: the old self-clearing open-circuit window is gone — time never + // unblocks a latch, only a fresh account re-add does. + now += yahoo.circuitOpenMillis * LATCH_ELAPSE_FACTOR + assertTrue("a latched circuit never self-clears with time", gate.isAuthBlocked(p)) + + // A success does not clear a latch either (defensive: no login is attempted while latched). + gate.onAuthSuccess(p) + assertTrue("a success must not clear a latch", gate.isAuthLatched(p)) + + // Only a re-add drops it, letting a fresh credential log in again. + gate.onAccountReadded(p) + assertFalse("a re-add clears the latch", gate.isAuthLatched(p)) + assertFalse(gate.isAuthBlocked(p)) } @Test @@ -101,5 +113,8 @@ class AuthThrottleGateInstrumentedTest { const val PORT = 993 const val RAPID_FAILURES = 10 const val ONE_HOUR_MS = 60 * 60_000L + + /** How many old open-circuit windows to fast-forward to prove a latch never self-clears (#362). */ + const val LATCH_ELAPSE_FACTOR = 10 } } diff --git a/app/src/main/kotlin/org/libremail/data/local/AccountDatabase.kt b/app/src/main/kotlin/org/libremail/data/local/AccountDatabase.kt index 3b0a070..d120b5f 100644 --- a/app/src/main/kotlin/org/libremail/data/local/AccountDatabase.kt +++ b/app/src/main/kotlin/org/libremail/data/local/AccountDatabase.kt @@ -40,7 +40,7 @@ import org.libremail.data.local.entity.SignatureEntity AccountSettingsEntity::class, SignatureEntity::class, ], - version = 2, + version = 3, exportSchema = true, ) abstract class AccountDatabase : RoomDatabase() { diff --git a/app/src/main/kotlin/org/libremail/data/local/AccountMigrations.kt b/app/src/main/kotlin/org/libremail/data/local/AccountMigrations.kt index 0ff0112..bf30d94 100644 --- a/app/src/main/kotlin/org/libremail/data/local/AccountMigrations.kt +++ b/app/src/main/kotlin/org/libremail/data/local/AccountMigrations.kt @@ -31,3 +31,16 @@ val ACCOUNT_MIGRATION_1_2 = object : Migration(1, 2) { ) } } + +/** + * AccountDatabase v2 -> v3 (issue #362): add the nullable [AccountEntity.authError], a user-facing sync/auth + * error surfaced on the account row and as a mailbox banner. The column is nullable with no default, so + * every existing account migrates to NULL ("no error, healthy") — matching the entity's `authError: String? + * = null` — and the proactive auth circuit later stamps the "remove and re-add" message onto an account + * whose Yahoo/AOL login has latched. A plain ADD COLUMN suffices; nothing is backfilled. + */ +val ACCOUNT_MIGRATION_2_3 = object : Migration(2, 3) { + override fun migrate(db: SupportSQLiteDatabase) { + db.execSQL("ALTER TABLE `accounts` ADD COLUMN `authError` TEXT") + } +} diff --git a/app/src/main/kotlin/org/libremail/data/local/Mappers.kt b/app/src/main/kotlin/org/libremail/data/local/Mappers.kt index 2bc7c01..3814ee4 100644 --- a/app/src/main/kotlin/org/libremail/data/local/Mappers.kt +++ b/app/src/main/kotlin/org/libremail/data/local/Mappers.kt @@ -39,6 +39,7 @@ internal fun AccountEntity.toDomain(): Account = Account( authType = runCatching { AuthType.valueOf(authType) }.getOrDefault(AuthType.PASSWORD_IMAP), imap = ServerConfig(imap.host, imap.port, imap.security.toMailSecurity()), smtp = ServerConfig(smtp.host, smtp.port, smtp.security.toMailSecurity()), + authError = authError, ) internal fun Account.toEntity(): AccountEntity = AccountEntity( @@ -48,6 +49,7 @@ internal fun Account.toEntity(): AccountEntity = AccountEntity( authType = authType.name, imap = ServerConfigEmbedded(imap.host, imap.port, imap.security.name), smtp = ServerConfigEmbedded(smtp.host, smtp.port, smtp.security.name), + authError = authError, ) internal fun AccountSettingsEntity.toDomain(): AccountSettings = AccountSettings( diff --git a/app/src/main/kotlin/org/libremail/data/local/dao/AccountDao.kt b/app/src/main/kotlin/org/libremail/data/local/dao/AccountDao.kt index 0db9fd3..adfb96f 100644 --- a/app/src/main/kotlin/org/libremail/data/local/dao/AccountDao.kt +++ b/app/src/main/kotlin/org/libremail/data/local/dao/AccountDao.kt @@ -86,6 +86,16 @@ interface AccountDao { orderedIds.forEachIndexed { index, id -> setSortOrder(id, index) } } + /** + * Records a user-facing [AccountEntity.authError] on the account (issue #362), returning the number of + * rows actually changed. The `authError IS NOT :message` guard makes it a **conditional** write: it + * updates only when the stored value differs (including from NULL), so a caller reconciling the auth + * circuit on every sync slice sets the error — and logs it — exactly once, never re-writing the same + * message. Pass a non-null message to mark errored. + */ + @Query("UPDATE accounts SET authError = :message WHERE id = :id AND authError IS NOT :message") + suspend fun setAuthError(id: String, message: String): Int + @Query("DELETE FROM accounts WHERE id = :id") suspend fun deleteById(id: String) } diff --git a/app/src/main/kotlin/org/libremail/data/local/entity/AccountEntity.kt b/app/src/main/kotlin/org/libremail/data/local/entity/AccountEntity.kt index 7d22516..7e4608e 100644 --- a/app/src/main/kotlin/org/libremail/data/local/entity/AccountEntity.kt +++ b/app/src/main/kotlin/org/libremail/data/local/entity/AccountEntity.kt @@ -23,6 +23,14 @@ data class AccountEntity( * a migrated one (the folders `specialUse` pattern). */ @ColumnInfo(defaultValue = "0") val sortOrder: Int = 0, + /** + * A user-facing sync/auth error that has halted this account, or null when healthy (issue #362). Set + * to the "remove and re-add" message once the proactive auth circuit **latches** — the account's login + * has failed enough consecutive times that a credential fix, not a retry, is required — so the account + * list and the mailbox banner can surface it. Nullable with an implicit NULL default, so existing rows + * migrate to "no error" and a fresh add (which rewrites the row) clears it. + */ + val authError: String? = null, ) /** Embedded host/port/security columns (prefixed per server in [AccountEntity]). */ diff --git a/app/src/main/kotlin/org/libremail/data/repository/AccountRepositoryImpl.kt b/app/src/main/kotlin/org/libremail/data/repository/AccountRepositoryImpl.kt index 07b8f84..321c867 100644 --- a/app/src/main/kotlin/org/libremail/data/repository/AccountRepositoryImpl.kt +++ b/app/src/main/kotlin/org/libremail/data/repository/AccountRepositoryImpl.kt @@ -22,6 +22,7 @@ import org.libremail.data.sync.SyncScheduler import org.libremail.domain.model.Account import org.libremail.domain.model.ImapConnectionParams import org.libremail.domain.repository.AccountRepository +import org.libremail.mail.AuthThrottleGate import org.libremail.mail.ImapClient import org.libremail.notifications.MailNotifier import org.libremail.reporting.AppLog @@ -39,6 +40,7 @@ class AccountRepositoryImpl @Inject constructor( private val draftDao: DraftDao, private val credentialStore: CredentialStore, private val imapClient: ImapClient, + private val authGate: AuthThrottleGate, private val syncScheduler: SyncScheduler, private val accountSettingsRepository: AccountSettingsRepository, private val mailNotifier: MailNotifier, @@ -54,7 +56,13 @@ class AccountRepositoryImpl @Inject constructor( } override suspend fun addImapAccount(account: Account, password: String): Result> = runCatching { - val folders = imapClient.listFolders(account.toImapParams(secret = password, useXoauth2 = false)) + val params = account.toImapParams(secret = password, useXoauth2 = false) + // #362: a re-add is the ONE thing that clears a latched Yahoo/AOL auth circuit. Drop the in-memory + // latch BEFORE the connection test so the fresh credential gets a clean login (an un-reset gate would + // still refuse it); the account-row rewrite below then clears the persisted authError (a fresh domain + // Account carries authError = null, and insertAtEnd's in-place update writes that null). + authGate.onAccountReadded(params) + val folders = imapClient.listFolders(params) // Persist the credential BEFORE inserting the account row (#403). Both LibreMailApplication's // push collector and IdleService.reconcileWatchers react to the *accounts* table; committing the // secret first guarantees any watcher that observes the new row can already resolve it, instead @@ -77,7 +85,11 @@ class AccountRepositoryImpl @Inject constructor( authStateJson: String, ): Result> = runCatching { val account = Account.outlook(email) - val folders = imapClient.listFolders(account.toImapParams(secret = accessToken, useXoauth2 = true)) + val params = account.toImapParams(secret = accessToken, useXoauth2 = true) + // #362: clear any latched auth circuit on re-add before the connection test, exactly as + // addImapAccount does — the account-row rewrite below clears the persisted authError. + authGate.onAccountReadded(params) + val folders = imapClient.listFolders(params) // Persist the durable AuthState BEFORE the account row (#403) — same ordering rationale as // addImapAccount: the push watchers observe the accounts table, so the secret must be committed // first for the newly-observed account to resolve. account_settings' FK still needs the row, so diff --git a/app/src/main/kotlin/org/libremail/data/sync/AccountAuthError.kt b/app/src/main/kotlin/org/libremail/data/sync/AccountAuthError.kt new file mode 100644 index 0000000..c51acc2 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/data/sync/AccountAuthError.kt @@ -0,0 +1,50 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.sync + +import android.content.Context +import org.libremail.R +import org.libremail.data.local.dao.AccountDao +import org.libremail.domain.model.Account +import org.libremail.domain.model.ImapConnectionParams +import org.libremail.mail.AuthThrottleGate +import org.libremail.reporting.AppLog +import org.libremail.reporting.accountLogRef + +/** Tag for the account-auth-error reconciliation breadcrumb (issue #362). PII-free. */ +private const val AUTH_ERROR_TAG = "AccountAuthError" + +/** + * Bridges the in-memory proactive auth circuit ([AuthThrottleGate]) to the **durable, user-visible account + * error** (issue #362). When [params]'s account has *latched* its circuit — the threshold of consecutive + * Yahoo/AOL authentication failures reached, i.e. a wrong app-password that won't fix itself — this persists + * the "remove and re-add this account" message onto the account row (via [AccountDao.setAuthError]) so the + * account list and the mailbox banner surface it, and returns `true` so the caller stops syncing the account + * (fail loud and stop, rather than the old silent self-clearing backoff window). + * + * The write is conditional (idempotent) at the SQL level, so across the many sync/backfill slices that may + * observe the same latch the error is stamped — and the breadcrumb logged — **exactly once**. Returns + * `false` and does nothing for a healthy or still-ramping account, and for every non-Yahoo/AOL host (which + * never latches). PII-free: only a hashed account ref is ever logged; the account id/email/host/credentials + * are not. + * + * Shared by [MailSyncer] and [MailBackfiller] — the two loops that already hold the [Account] and an + * [AccountDao] — rather than injecting a DAO into the mail-layer gate, keeping the persistence write in the + * data layer where it belongs. + */ +internal suspend fun markAccountErroredIfLatched( + authGate: AuthThrottleGate, + accountDao: AccountDao, + context: Context, + account: Account, + params: ImapConnectionParams, +): Boolean { + if (!authGate.isAuthLatched(params)) return false + val message = context.getString(R.string.account_auth_error_remove_readd) + if (accountDao.setAuthError(account.id, message) > 0) { + AppLog.w( + AUTH_ERROR_TAG, + "auth circuit latched ${accountLogRef(account.id)}: account marked errored, sync stopped until re-add", + ) + } + return true +} diff --git a/app/src/main/kotlin/org/libremail/data/sync/MailBackfiller.kt b/app/src/main/kotlin/org/libremail/data/sync/MailBackfiller.kt index 42e0e81..f0fc48c 100644 --- a/app/src/main/kotlin/org/libremail/data/sync/MailBackfiller.kt +++ b/app/src/main/kotlin/org/libremail/data/sync/MailBackfiller.kt @@ -78,6 +78,13 @@ class MailBackfiller @Inject constructor( var remaining = maxBatches var moreWork = false accounts@ for (account in accountDao.getAll().map { it.toDomain() }) { + // #362 fail-loud stop: an account whose proactive auth circuit latched is persisted as errored + // and must not be probed again — skip it entirely (no login), durably across restarts (reads the + // persisted authError, not just the in-memory gate), until the user re-adds it. + if (account.authError != null) { + AppLog.i(TAG, "backfill skip ${accountLogRef(account.id)}: account errored, awaiting re-add") + continue@accounts + } // Skip an account backing off (throttle #360 / auth #362) or with unresolvable credentials — // a null return does NOT set moreWork, so a slice whose only work is a backing-off account // reports "done" instead of tight-looping over the skip. See [paramsForBackfill]. @@ -123,9 +130,14 @@ class MailBackfiller @Inject constructor( return null } val params = runCatching { connectionFactory.imapParamsFor(account) }.getOrNull() ?: return null + // If the auth circuit latched (via any path) since the durable authError check in runBackfill, stamp + // the account error now (once, as a side effect); the block check below then skips it, since a latched + // circuit reports a "forever" remaining block — folding the latch skip into the existing backoff skip. + val latched = markAccountErroredIfLatched(authGate, accountDao, context, account, params) val authBlock = authGate.remainingAuthBlockMillis(params) if (authBlock > 0L) { - AppLog.i(TAG, "backfill skip ${accountLogRef(account.id)}: auth backing off, remaining=${authBlock}ms") + val reason = if (latched) "auth circuit latched" else "auth backing off, remaining=${authBlock}ms" + AppLog.i(TAG, "backfill skip ${accountLogRef(account.id)}: $reason") return null } return params diff --git a/app/src/main/kotlin/org/libremail/data/sync/MailSyncer.kt b/app/src/main/kotlin/org/libremail/data/sync/MailSyncer.kt index 426860c..409b43e 100644 --- a/app/src/main/kotlin/org/libremail/data/sync/MailSyncer.kt +++ b/app/src/main/kotlin/org/libremail/data/sync/MailSyncer.kt @@ -12,13 +12,17 @@ import kotlinx.coroutines.withContext import org.libremail.BuildConfig import org.libremail.data.local.dao.AccountDao import org.libremail.data.local.dao.MessageDao +import org.libremail.data.local.entity.MessageEntity import org.libremail.data.local.toDomain import org.libremail.data.local.toEntity import org.libremail.data.settings.AccountSettingsRepository import org.libremail.data.settings.SettingsRepository import org.libremail.data.settings.effectiveRetention import org.libremail.domain.model.Account +import org.libremail.domain.model.ImapConnectionParams import org.libremail.domain.repository.MailRepository +import org.libremail.mail.AuthThrottleGate +import org.libremail.mail.FetchedMessage import org.libremail.mail.ImapClient import org.libremail.notifications.MailNotifier import org.libremail.power.BatteryStatusProvider @@ -42,6 +46,7 @@ class MailSyncer @Inject constructor( private val mailRepository: MailRepository, private val throttleGate: AccountThrottleGate, private val bandwidthTracker: GmailBandwidthTracker, + private val authGate: AuthThrottleGate, ) : Syncer { // Serializes all syncing: syncAll/syncAccount/syncFolder are invoked concurrently by the periodic // worker, pull-to-refresh, one-shot syncs, folder opens, and one IDLE watcher per account. Without @@ -92,9 +97,26 @@ class MailSyncer @Inject constructor( return result } - private suspend fun syncFolderHeaders(account: Account, folder: String, notify: Boolean): Result = - runCatching { - val params = connectionFactory.imapParamsFor(account) + private suspend fun syncFolderHeaders(account: Account, folder: String, notify: Boolean): Result { + // #362 fail-loud stop: an account whose proactive auth circuit latched is persisted as errored + // (markAccountErroredIfLatched) and must not be probed again — skip it entirely, no login attempt, so + // a locked-out Yahoo/AOL account is never nudged further. Durable across restarts: it reads the + // persisted account error, not just the in-memory gate. + if (account.authError != null) { + AppLog.i(TAG, "sync skip ${accountLogRef(account.id)}: account errored, awaiting re-add") + return Result.success(0) + } + // Captured the moment params resolve so onFailure can reconcile the auth circuit without re-resolving + // them (which for OAuth would redeem a fresh token); null means no login was attempted. + var attemptedParams: ImapConnectionParams? = null + return runCatching { + val params = connectionFactory.imapParamsFor(account).also { attemptedParams = it } + // The circuit may have latched via another path (backfill/IDLE) since the durable check above: + // stamp the account error now and skip rather than drive a login the gate would only refuse. + if (markAccountErroredIfLatched(authGate, accountDao, context, account, params)) { + AppLog.i(TAG, "sync skip ${accountLogRef(account.id)}: auth circuit latched") + return@runCatching 0 + } val policy = accountSettingsRepository.effectiveRetention(settingsRepository, account.id) // Never fetch more of the recent window than device-only retention (#13) would keep. Without // this, a count limit BELOW the window would make foreground sync re-download the same rows @@ -109,52 +131,9 @@ class MailSyncer @Inject constructor( val entities = fetched.map { it.toEntity(account.id, folder) } .let { mapped -> if (cutoff == null) mapped else mapped.filter { it.timestampMillis >= cutoff } } - // Persist and notify atomically with respect to cancellation: an IDLE renewal that cancels - // mid-sync must not drop a notification (the rows would then look "already seen" next time). - withContext(NonCancellable) { - val existingIds = messageDao.getSyncedIds(account.id, folder).toHashSet() - // Don't notify on the very first sync of a folder (would announce everything in it). - val newMessages = if (existingIds.isEmpty()) { - emptyList() - } else { - entities.filter { it.id !in existingIds && !it.isRead } - } - - if (fetched.isEmpty()) { - // An empty recent window means the server folder itself is empty, so nothing (not - // even backfilled history) should remain cached for it. Keyed on the raw fetch, not the - // age-filtered set: a folder holding only mail older than the age cutoff is NOT empty on - // the server, so its stale local rows are left to the pruner rather than wiped here. - messageDao.deleteSyncedByAccountFolder(account.id, folder) - } else { - val ids = entities.map { it.id } - messageDao.insertNew(entities) - // Mark every fetched message as synced (upgrades any former search-only row) and refresh - // its display fields — without touching cached bodies or optimistic read/star flags. The - // per-row refreshes run in a single transaction (issue #310) so a whole recent window - // costs one commit instead of one fsync per message (amplified on the encrypted cache). - messageDao.markSynced(ids) - messageDao.updateHeaderContents(entities) - // Reconcile server-side deletions ONLY within the fetched recent-UID window, so older - // history paged in by the background backfill (issue #12) survives each foreground sync - // instead of being wiped by a whole-folder "not in the recent 50" delete. Bound the - // window by the lowest POSITIVE fetched UID: a message whose UID couldn't be resolved - // (UIDFolder.getUID returns -1) must not collapse the bound to <= 0 and turn this into a - // whole-folder delete that wipes the backfilled history below the window. - val minWindowUid = entities.mapNotNull { entity -> entity.uid.takeIf { it > 0L } }.minOrNull() - if (minWindowUid != null) { - messageDao.deleteSyncedInWindowNotIn(account.id, folder, minWindowUid, ids) - } - } - - val shouldNotify = notify && - newMessages.isNotEmpty() && - settingsRepository.isNewMailNotificationsEnabled() && - accountSettingsRepository.get(account.id).notificationsEnabled - if (shouldNotify) { - notifier.notifyNewMail(account, newMessages.sortedByDescending { it.timestampMillis }) - } - } + // Persist the fetched window and notify about new inbox mail, atomically w.r.t. cancellation + // (extracted to [persistSyncedWindow] to stay under the complexity gate; behavior-preserving). + persistSyncedWindow(account, folder, notify, fetched, entities) val folderLabel = logSafeFolderLabel(folder) AppLog.d(TAG, "sync ${accountLogRef(account.id)} folder=$folderLabel fetched=${fetched.size}") fetched.size @@ -163,7 +142,68 @@ class MailSyncer @Inject constructor( // the background backfill backs this account off — but the interactive sync itself is never // blocked by the gate, so opening/refreshing mail is never queued behind a backfill backoff. ThrottleClassifier.classify(error)?.let { throttleGate.onThrottle(account.id, it) } + // #362: if this very failure crossed the auth-latch threshold, error the account (once) so the + // UI shows the "remove and re-add" state instead of silently retrying a doomed credential. + attemptedParams?.let { markAccountErroredIfLatched(authGate, accountDao, context, account, it) } } + } + + /** + * Persists a freshly-fetched recent window and notifies about genuinely new inbox mail, atomically with + * respect to cancellation: an IDLE renewal that cancels mid-sync must not drop a notification (the rows + * would then look "already seen" next time). Extracted from [syncFolderHeaders] to keep it under the + * complexity gate — behavior-preserving. + */ + private suspend fun persistSyncedWindow( + account: Account, + folder: String, + notify: Boolean, + fetched: List, + entities: List, + ) = withContext(NonCancellable) { + val existingIds = messageDao.getSyncedIds(account.id, folder).toHashSet() + // Don't notify on the very first sync of a folder (would announce everything in it). + val newMessages = if (existingIds.isEmpty()) { + emptyList() + } else { + entities.filter { it.id !in existingIds && !it.isRead } + } + + if (fetched.isEmpty()) { + // An empty recent window means the server folder itself is empty, so nothing (not even + // backfilled history) should remain cached for it. Keyed on the raw fetch, not the age-filtered + // set: a folder holding only mail older than the age cutoff is NOT empty on the server, so its + // stale local rows are left to the pruner rather than wiped here. + messageDao.deleteSyncedByAccountFolder(account.id, folder) + } else { + val ids = entities.map { it.id } + messageDao.insertNew(entities) + // Mark every fetched message as synced (upgrades any former search-only row) and refresh its + // display fields — without touching cached bodies or optimistic read/star flags. The per-row + // refreshes run in a single transaction (issue #310) so a whole recent window costs one commit + // instead of one fsync per message (amplified on the encrypted cache). + messageDao.markSynced(ids) + messageDao.updateHeaderContents(entities) + // Reconcile server-side deletions ONLY within the fetched recent-UID window, so older history + // paged in by the background backfill (issue #12) survives each foreground sync instead of being + // wiped by a whole-folder "not in the recent 50" delete. Bound the window by the lowest POSITIVE + // fetched UID: a message whose UID couldn't be resolved (UIDFolder.getUID returns -1) must not + // collapse the bound to <= 0 and turn this into a whole-folder delete that wipes the backfilled + // history below the window. + val minWindowUid = entities.mapNotNull { entity -> entity.uid.takeIf { it > 0L } }.minOrNull() + if (minWindowUid != null) { + messageDao.deleteSyncedInWindowNotIn(account.id, folder, minWindowUid, ids) + } + } + + val shouldNotify = notify && + newMessages.isNotEmpty() && + settingsRepository.isNewMailNotificationsEnabled() && + accountSettingsRepository.get(account.id).notificationsEnabled + if (shouldNotify) { + notifier.notifyNewMail(account, newMessages.sortedByDescending { it.timestampMillis }) + } + } /** * Aggressively pre-caches each not-yet-fetched message's full content (body + attachments) per the diff --git a/app/src/main/kotlin/org/libremail/di/AccountDatabaseModule.kt b/app/src/main/kotlin/org/libremail/di/AccountDatabaseModule.kt index 620434e..ec91fe2 100644 --- a/app/src/main/kotlin/org/libremail/di/AccountDatabaseModule.kt +++ b/app/src/main/kotlin/org/libremail/di/AccountDatabaseModule.kt @@ -11,6 +11,7 @@ import dagger.hilt.android.qualifiers.ApplicationContext import dagger.hilt.components.SingletonComponent import kotlinx.coroutines.runBlocking import org.libremail.data.local.ACCOUNT_MIGRATION_1_2 +import org.libremail.data.local.ACCOUNT_MIGRATION_2_3 import org.libremail.data.local.AccountDatabase import org.libremail.data.local.CacheEncryptionUnavailableException import org.libremail.data.local.DatabaseFiles.ACCOUNTS_NAME @@ -53,7 +54,7 @@ object AccountDatabaseModule { @ApplicationContext context: Context, provisioner: DatabaseProvisioner, ): AccountDatabase = Room.databaseBuilder(context, AccountDatabase::class.java, ACCOUNTS_NAME) - .addMigrations(ACCOUNT_MIGRATION_1_2) + .addMigrations(ACCOUNT_MIGRATION_1_2, ACCOUNT_MIGRATION_2_3) .openHelperFactory( DeferredOpenHelperFactory { configuration -> try { diff --git a/app/src/main/kotlin/org/libremail/domain/model/Account.kt b/app/src/main/kotlin/org/libremail/domain/model/Account.kt index 0c154e4..fc247e4 100644 --- a/app/src/main/kotlin/org/libremail/domain/model/Account.kt +++ b/app/src/main/kotlin/org/libremail/domain/model/Account.kt @@ -17,6 +17,13 @@ data class Account( val authType: AuthType, val imap: ServerConfig, val smtp: ServerConfig, + /** + * A user-facing error that has halted this account's sync, or null when healthy (issue #362). Carries + * the "remove and re-add this account" message once its Yahoo/AOL auth circuit latches after repeated + * authentication failures; the UI renders it on the account row and as a mailbox banner. Cleared by a + * fresh re-add. Defaulted so it is not a required field at construction (setup, tests). + */ + val authError: String? = null, ) { companion object { private const val OUTLOOK_IMAP_PORT = 993 diff --git a/app/src/main/kotlin/org/libremail/mail/AuthThrottleGate.kt b/app/src/main/kotlin/org/libremail/mail/AuthThrottleGate.kt index 315f484..ccc29ce 100644 --- a/app/src/main/kotlin/org/libremail/mail/AuthThrottleGate.kt +++ b/app/src/main/kotlin/org/libremail/mail/AuthThrottleGate.kt @@ -44,9 +44,17 @@ class AuthBackoffException(remainingMillis: Long) : * (`host|port|username`), so one blocked account never stalls another, and every log line uses * [accountLogRef] over that key — a salted-looking hash, never the address or host. * - * State lives only in-process (`@Singleton`); a process restart clears it and simply re-probes, which is - * safe — the first attempt after restart is spaced from the previous run by however long the process was - * down, and any real re-failure immediately re-arms the backoff. + * **Fail-loud latch (issue #362).** Past [AuthCadencePolicy.circuitOpenThreshold] consecutive failures the + * circuit **latches**: a wrong app-password does not fix itself, so instead of a self-clearing window we + * stop retrying *entirely* and hold the account blocked forever. The latch is surfaced to the user as a + * persisted account error ("remove and re-add") by + * [org.libremail.data.sync.markAccountErroredIfLatched], and is cleared only by a fresh account re-add + * ([onAccountReadded]) — never by time or a stray success. + * + * The in-memory latch itself lives only in-process (`@Singleton`); the durable stop is the persisted + * account error, which the sync/backfill loops honour across restarts, so a process restart does not + * quietly resume probing a latched account. Below the threshold, the in-memory ramp is transient — a + * restart there simply re-probes (spaced from the previous run), and any real re-failure re-arms it. */ @Singleton class AuthThrottleGate internal constructor( @@ -62,53 +70,109 @@ class AuthThrottleGate internal constructor( policyForHost = ProviderAuthPolicy::forHost, ) - /** One account's auth state: consecutive failures, until when logins are blocked, the last wait, open? */ + /** + * One account's auth state: consecutive failures, until when logins are blocked, the last computed + * wait, and whether the circuit has **latched** — a permanent fail-loud stop past the threshold that + * clears only on a fresh account re-add (issue #362), never by time or a success. + */ private data class State( val failures: Int, val blockedUntilMillis: Long, val lastBlockMillis: Long, - val circuitOpen: Boolean, + val latched: Boolean, ) private val states = ConcurrentHashMap() /** * Records a failed authentication for [params]'s account and returns the resulting block in ms (0 when - * the host has no auth-lockout risk, so the call is a no-op). Escalates the consecutive-failure count - * so repeats back off exponentially, then extend into the fixed open-circuit window past the policy's - * threshold, and stamps the account blocked until `now + block`. Atomic per account. Logs a PII-free - * breadcrumb (failure count, block, and whether the circuit is now open). + * the host has no auth-lockout risk, so the call is a no-op). Below the threshold it escalates the + * consecutive-failure count so repeats back off exponentially and stamps the account blocked until + * `now + block`. At the threshold the circuit **latches**: a permanent block (fail-loud stop) that no + * further failure escalates and no success or elapsed time clears — only [onAccountReadded] does + * (issue #362). Atomic per account. Logs a PII-free breadcrumb (the latch transition once, or the + * failure count + block while ramping). */ fun onAuthFailure(params: ImapConnectionParams): Long { val policy = policyForHost(params.host) if (!policy.enabled) return 0L val now = nowMillis() + var alreadyLatched = false val updated = states.compute(key(params)) { _, previous -> + // A latched circuit is a permanent stop: further failures neither escalate nor re-arm it (and + // the failure count stays frozen), so we never spam the log or drift the state once we give up. + if (previous?.latched == true) { + alreadyLatched = true + return@compute previous + } val failures = (previous?.failures ?: 0) + 1 val block = AuthBackoff.blockMillis(policy, failures, random()) + val latched = failures >= policy.circuitOpenThreshold State( failures = failures, - blockedUntilMillis = now + block, + // Latched: block "forever" (Long.MAX_VALUE never elapses) so every subsequent login is + // skipped until a re-add — the fail-loud stop that replaced the old self-clearing window. + blockedUntilMillis = if (latched) Long.MAX_VALUE else now + block, lastBlockMillis = block, - circuitOpen = failures >= policy.circuitOpenThreshold, + latched = latched, ) }!! - AppLog.w( - TAG, - "auth backoff ${logRef(params)} failures=${updated.failures} block=${updated.lastBlockMillis}ms" + - if (updated.circuitOpen) " circuit=open" else "", - ) + when { + alreadyLatched -> Unit // logged once when it first latched; stay silent thereafter + updated.latched -> AppLog.w( + TAG, + "auth circuit latched ${logRef(params)} after ${updated.failures} failure(s): " + + "retries stopped, account will be errored", + ) + else -> AppLog.w( + TAG, + "auth backoff ${logRef(params)} failures=${updated.failures} block=${updated.lastBlockMillis}ms", + ) + } return updated.lastBlockMillis } /** - * Clears any auth-backoff state for [params]'s account after a successful login, so a recovered - * account resumes at full speed with the failure count reset. Silent no-op when the account was not - * blocked (or the host is not gated), so [ImapClient] can call it on every successful connect. + * Clears a *ramping* (not yet latched) auth-backoff for [params]'s account after a successful login, so + * a recovered account resumes at full speed with the failure count reset. A **latched** circuit is + * deliberately left intact — it is cleared only by a fresh account re-add ([onAccountReadded]), never by + * a success (issue #362); while latched no login is even attempted, so this is a defensive guard. Silent + * no-op when the account was not blocked (or the host is not gated), so [ImapClient] can call it on every + * successful connect. */ fun onAuthSuccess(params: ImapConnectionParams) { - val previous = states.remove(key(params)) ?: return - AppLog.i(TAG, "auth recovered ${logRef(params)} after ${previous.failures} failure(s)") + var cleared: State? = null + states.compute(key(params)) { _, current -> + when { + current == null -> null + current.latched -> current // a latched circuit clears only on re-add, never on success + else -> { + cleared = current + null + } + } + } + cleared?.let { AppLog.i(TAG, "auth recovered ${logRef(params)} after ${it.failures} failure(s)") } + } + + /** + * True once [params]'s account has permanently **latched** its auth circuit (issue #362) — the + * threshold of consecutive Yahoo/AOL auth failures reached — so it must be surfaced to the user as + * errored and no login retried until a fresh account re-add. + */ + fun isAuthLatched(params: ImapConnectionParams): Boolean = states[key(params)]?.latched == true + + /** + * Drops ALL auth state for [params]'s account — including a permanently latched circuit — because the + * user re-added the account with fresh credentials (issue #362). This is the single path that escapes a + * latch: the next login is then attempted clean. Called from + * [org.libremail.data.repository.AccountRepositoryImpl] on add (before the connection test), paired with + * clearing the persisted account error, so a re-add fully resumes sync. + */ + fun onAccountReadded(params: ImapConnectionParams) { + if (states.remove(key(params)) != null) { + AppLog.i(TAG, "auth state reset ${logRef(params)}: account re-added, retries resume") + } } /** diff --git a/app/src/main/kotlin/org/libremail/ui/mailbox/FolderDrawer.kt b/app/src/main/kotlin/org/libremail/ui/mailbox/FolderDrawer.kt index f922a87..ab40c67 100644 --- a/app/src/main/kotlin/org/libremail/ui/mailbox/FolderDrawer.kt +++ b/app/src/main/kotlin/org/libremail/ui/mailbox/FolderDrawer.kt @@ -3,8 +3,11 @@ package org.libremail.ui.mailbox import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.Spacer import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.layout.size +import androidx.compose.foundation.layout.width import androidx.compose.foundation.rememberScrollState import androidx.compose.foundation.verticalScroll import androidx.compose.material.icons.Icons @@ -13,6 +16,7 @@ import androidx.compose.material.icons.filled.ArrowDropDown import androidx.compose.material.icons.filled.Delete import androidx.compose.material.icons.filled.Edit import androidx.compose.material.icons.filled.Email +import androidx.compose.material.icons.filled.Warning import androidx.compose.material3.DropdownMenu import androidx.compose.material3.DropdownMenuItem import androidx.compose.material3.HorizontalDivider @@ -138,6 +142,16 @@ private fun AccountSwitcher( overflow = TextOverflow.Ellipsis, fontWeight = if (current.id in accountsWithUnread) FontWeight.Bold else FontWeight.Normal, ) + // #362: flag the switched-to account when its auth circuit has latched (remove-and-re-add). + if (current.authError != null) { + Spacer(Modifier.width(4.dp)) + Icon( + Icons.Filled.Warning, + contentDescription = stringResource(R.string.account_auth_error_icon_description), + tint = MaterialTheme.colorScheme.error, + modifier = Modifier.size(18.dp), + ) + } Icon(Icons.Filled.ArrowDropDown, contentDescription = stringResource(R.string.drawer_switch_account)) } DropdownMenu(expanded = expanded, onDismissRequest = { expanded = false }) { @@ -149,6 +163,18 @@ private fun AccountSwitcher( fontWeight = if (account.id in accountsWithUnread) FontWeight.Bold else FontWeight.Normal, ) }, + // #362: a red warning glyph marks any errored account in the switcher list too. + trailingIcon = if (account.authError != null) { + { + Icon( + Icons.Filled.Warning, + contentDescription = stringResource(R.string.account_auth_error_icon_description), + tint = MaterialTheme.colorScheme.error, + ) + } + } else { + null + }, onClick = { onSelect(account.id) expanded = false diff --git a/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxScreen.kt b/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxScreen.kt index 3e1dff9..a1f5225 100644 --- a/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxScreen.kt +++ b/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxScreen.kt @@ -54,6 +54,7 @@ import androidx.compose.material3.ModalNavigationDrawer import androidx.compose.material3.Scaffold import androidx.compose.material3.SnackbarHost import androidx.compose.material3.SnackbarHostState +import androidx.compose.material3.Surface import androidx.compose.material3.Text import androidx.compose.material3.TextButton import androidx.compose.material3.TopAppBar @@ -121,6 +122,7 @@ fun MailboxScreen( val isRefreshing by viewModel.isRefreshing.collectAsStateWithLifecycle() val isSyncingFolder by viewModel.isSyncingFolder.collectAsStateWithLifecycle() val error by viewModel.error.collectAsStateWithLifecycle() + val accountAuthError by viewModel.accountAuthError.collectAsStateWithLifecycle() val selectedIds by viewModel.selectedIds.collectAsStateWithLifecycle() val pendingConfirm by viewModel.pendingConfirm.collectAsStateWithLifecycle() val currentFolderRole by viewModel.currentFolderRole.collectAsStateWithLifecycle() @@ -265,6 +267,9 @@ fun MailboxScreen( val accountsById = remember(accounts) { accounts.associateBy { it.id } } val showAccount = selectedAccountId == null && accounts.size >= 2 Column(Modifier.fillMaxSize()) { + // #362: a persistent, non-dismissable banner while the shown account's auth circuit + // is latched — mirrors the DB error state, so it clears only when a re-add does. + accountAuthError?.let { AccountErrorBanner(message = it) } if (accounts.size >= 2 && selectedFolder == INBOX) { AccountFilterRow( accounts = accounts, @@ -406,6 +411,34 @@ private fun SearchField(query: String, onQueryChange: (String) -> Unit) { LaunchedEffect(Unit) { focusRequester.requestFocus() } } +/** + * A persistent, non-dismissable error banner shown atop the mailbox when the displayed account's auth + * circuit has latched (issue #362). Renders the "remove and re-add" message from the account's persisted + * error state, so it is present whenever that state is and disappears the moment a re-add clears it — never + * a transient snackbar. Uses the error-container role so it reads as an alert in light and dark themes. + */ +@Composable +private fun AccountErrorBanner(message: String) { + Surface(color = MaterialTheme.colorScheme.errorContainer, modifier = Modifier.fillMaxWidth()) { + Row( + modifier = Modifier.padding(horizontal = 16.dp, vertical = 12.dp), + verticalAlignment = Alignment.CenterVertically, + ) { + Icon( + Icons.Filled.Warning, + contentDescription = null, + tint = MaterialTheme.colorScheme.onErrorContainer, + ) + Spacer(Modifier.width(12.dp)) + Text( + text = message, + style = MaterialTheme.typography.bodyMedium, + color = MaterialTheme.colorScheme.onErrorContainer, + ) + } + } +} + @Composable private fun DraftsEntry(count: Int, onClick: () -> Unit) { Row( diff --git a/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxViewModel.kt b/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxViewModel.kt index c322628..6e03d3e 100644 --- a/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxViewModel.kt +++ b/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxViewModel.kt @@ -116,6 +116,19 @@ class MailboxViewModel @Inject constructor( .map { counts -> counts.filter { it.count > 0 }.map { it.accountId }.toSet() } .stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), emptySet()) + /** + * #362: the persistent auth-error message shown as a banner atop the mailbox, or null when the + * relevant account(s) are healthy. In a per-account view it is the selected account's error; in the + * unified view it surfaces the first errored account's message (identical across accounts), so a + * latched Yahoo/AOL account is visible whichever way the mailbox is filtered. Sourced from the same + * `accounts` flow the row indicators use, so it clears the instant a re-add wipes the error. + */ + val accountAuthError: StateFlow = + combine(accounts, _selectedAccountId) { accts, selectedId -> + val relevant = if (selectedId == null) accts else accts.filter { it.id == selectedId } + relevant.firstNotNullOfOrNull { it.authError } + }.stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), null) + private val _searchActive = MutableStateFlow(false) val searchActive: StateFlow = _searchActive.asStateFlow() diff --git a/app/src/main/kotlin/org/libremail/ui/settings/AccountReorderList.kt b/app/src/main/kotlin/org/libremail/ui/settings/AccountReorderList.kt index 8a996f6..bd67e1a 100644 --- a/app/src/main/kotlin/org/libremail/ui/settings/AccountReorderList.kt +++ b/app/src/main/kotlin/org/libremail/ui/settings/AccountReorderList.kt @@ -6,11 +6,15 @@ import androidx.compose.foundation.clickable import androidx.compose.foundation.gestures.detectDragGesturesAfterLongPress import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.Row +import androidx.compose.foundation.layout.Spacer import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.offset import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.layout.size +import androidx.compose.foundation.layout.width import androidx.compose.material.icons.Icons import androidx.compose.material.icons.filled.Menu +import androidx.compose.material.icons.filled.Warning import androidx.compose.material3.Icon import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Surface @@ -67,6 +71,7 @@ internal fun AccountReorderList( AccountReorderRow( id = account.id, email = account.email, + authError = account.authError, dragging = dragging, offsetY = if (dragging) dragOffsetY.roundToInt() else 0, onMeasured = { rowHeightPx = it }, @@ -107,6 +112,7 @@ internal fun commitDrag(current: List, id: String, dragOffsetY: Float, private fun AccountReorderRow( id: String, email: String, + authError: String?, dragging: Boolean, offsetY: Int, onMeasured: (Int) -> Unit, @@ -146,7 +152,30 @@ private fun AccountReorderRow( .padding(horizontal = 16.dp, vertical = 16.dp), verticalAlignment = Alignment.CenterVertically, ) { - Text(text = email, style = MaterialTheme.typography.bodyLarge, modifier = Modifier.weight(1f)) + Column(modifier = Modifier.weight(1f)) { + Text(text = email, style = MaterialTheme.typography.bodyLarge) + // #362: a latched Yahoo/AOL auth failure surfaces here as a persistent red error line, so + // the user sees which account is broken (and that removing + re-adding it is the fix). + if (authError != null) { + Row( + verticalAlignment = Alignment.CenterVertically, + modifier = Modifier.padding(top = 2.dp), + ) { + Icon( + imageVector = Icons.Filled.Warning, + contentDescription = stringResource(R.string.account_auth_error_icon_description), + tint = MaterialTheme.colorScheme.error, + modifier = Modifier.size(16.dp), + ) + Spacer(Modifier.width(4.dp)) + Text( + text = authError, + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.error, + ) + } + } + } Icon( imageVector = Icons.Filled.Menu, contentDescription = stringResource(R.string.account_reorder_handle), diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 0f9dbb6..17f1b4e 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -288,6 +288,12 @@ Long-press and drag to reorder account Remove account + + Please remove and re-add this account with valid credentials + Account error + Settings Backup Include settings in Android Backup diff --git a/app/src/test/kotlin/org/libremail/data/local/MappersTest.kt b/app/src/test/kotlin/org/libremail/data/local/MappersTest.kt index e3d5f1d..c05fd07 100644 --- a/app/src/test/kotlin/org/libremail/data/local/MappersTest.kt +++ b/app/src/test/kotlin/org/libremail/data/local/MappersTest.kt @@ -45,6 +45,24 @@ class MappersTest { assertEquals(account, account.toEntity().toDomain()) } + @Test + fun `Account authError round-trips through the entity in both directions (issue 362)`() { + val healthy = Account( + id = "acct", + email = "ada@example.org", + displayName = "Ada", + authType = AuthType.PASSWORD_IMAP, + imap = ServerConfig("imap.example.org", 993, MailSecurity.SSL_TLS), + smtp = ServerConfig("smtp.example.org", 465, MailSecurity.SSL_TLS), + ) + assertNull(healthy.toEntity().authError, "a healthy account carries no persisted error") + assertNull(healthy.toEntity().toDomain().authError) + + val errored = healthy.copy(authError = "Please remove and re-add this account with valid credentials") + assertEquals(errored.authError, errored.toEntity().authError, "the error is persisted") + assertEquals(errored, errored.toEntity().toDomain(), "and reads back intact") + } + @Test fun `AccountEntity toDomain falls back to safe defaults for unknown persisted enum names`() { val entity = AccountEntity( diff --git a/app/src/test/kotlin/org/libremail/data/repository/AccountRepositoryImplTest.kt b/app/src/test/kotlin/org/libremail/data/repository/AccountRepositoryImplTest.kt index c348fa5..38d65cf 100644 --- a/app/src/test/kotlin/org/libremail/data/repository/AccountRepositoryImplTest.kt +++ b/app/src/test/kotlin/org/libremail/data/repository/AccountRepositoryImplTest.kt @@ -38,12 +38,14 @@ import org.libremail.domain.model.AuthType import org.libremail.domain.model.ImapConnectionParams import org.libremail.domain.model.MailSecurity import org.libremail.domain.model.ServerConfig +import org.libremail.mail.AuthThrottleGate import org.libremail.mail.FetchedFolder import org.libremail.mail.ImapClient import org.libremail.notifications.MailNotifier import java.io.File import kotlin.test.assertEquals import kotlin.test.assertFalse +import kotlin.test.assertNull import kotlin.test.assertTrue /** @@ -62,6 +64,7 @@ class AccountRepositoryImplTest { private val draftDao = mockk(relaxed = true) private val credentialStore = mockk(relaxed = true) private val imapClient = mockk() + private val authGate = mockk(relaxed = true) private val syncScheduler = mockk(relaxed = true) private val accountSettingsRepository = mockk(relaxed = true) private val mailNotifier = mockk(relaxed = true) @@ -76,6 +79,7 @@ class AccountRepositoryImplTest { draftDao = draftDao, credentialStore = credentialStore, imapClient = imapClient, + authGate = authGate, syncScheduler = syncScheduler, accountSettingsRepository = accountSettingsRepository, mailNotifier = mailNotifier, @@ -170,6 +174,27 @@ class AccountRepositoryImplTest { } } + @Test + fun `addImapAccount resets a latched auth circuit before the test and clears the account error`() = runTest { + val account = account() + val entity = slot() + coEvery { imapClient.listFolders(any()) } returns listOf( + FetchedFolder("INBOX", "INBOX", emptyList(), selectable = true), + ) + coEvery { accountDao.insertAtEnd(capture(entity)) } just Runs + + repository.addImapAccount(account, "app-password").getOrThrow() + + // #362: the in-memory latch is dropped BEFORE the connection test, so a fresh credential logs in + // cleanly instead of being refused by a still-latched gate. + coVerifyOrder { + authGate.onAccountReadded(any()) + imapClient.listFolders(any()) + } + // ...and the (re)written account row carries a null authError, clearing any persisted error state. + assertNull(entity.captured.authError, "a re-add clears the persisted account error") + } + @Test fun `addOutlookAccount persists the credential before the account row (issue 403)`() = runTest { coEvery { imapClient.listFolders(any()) } returns listOf( diff --git a/app/src/test/kotlin/org/libremail/data/sync/AccountAuthErrorTest.kt b/app/src/test/kotlin/org/libremail/data/sync/AccountAuthErrorTest.kt new file mode 100644 index 0000000..6e70ada --- /dev/null +++ b/app/src/test/kotlin/org/libremail/data/sync/AccountAuthErrorTest.kt @@ -0,0 +1,143 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.sync + +import android.content.Context +import io.mockk.coEvery +import io.mockk.coVerify +import io.mockk.every +import io.mockk.mockk +import io.mockk.mockkStatic +import io.mockk.unmockkAll +import kotlinx.coroutines.test.runTest +import org.junit.After +import org.junit.Before +import org.junit.Test +import org.libremail.R +import org.libremail.data.local.dao.AccountDao +import org.libremail.domain.model.Account +import org.libremail.domain.model.AuthType +import org.libremail.domain.model.ImapConnectionParams +import org.libremail.domain.model.MailSecurity +import org.libremail.domain.model.ServerConfig +import org.libremail.mail.AuthCadencePolicy +import org.libremail.mail.AuthThrottleGate +import org.libremail.mail.ProviderAuthPolicy +import org.libremail.reporting.AppLog +import org.libremail.reporting.RingLogBuffer +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +/** + * [markAccountErroredIfLatched] (issue #362) must persist the user-facing "remove and re-add" error onto + * an account whose proactive auth circuit has **latched** — exactly once, PII-free — and report the latch + * so the caller stops syncing; and it must be a no-op for a still-ramping or healthy account. + */ +class AccountAuthErrorTest { + + private val logBuffer = RingLogBuffer() + + /** A gate that treats the test host as a gated Yahoo/AOL account, latching after the threshold. */ + private fun latchingGate(): AuthThrottleGate = AuthThrottleGate( + nowMillis = { 0L }, + random = { 0.0 }, + policyForHost = { ProviderAuthPolicy.forHost(YAHOO_HOST) }, + ) + + private fun params() = ImapConnectionParams(YAHOO_HOST, PORT, MailSecurity.SSL_TLS, "user@yahoo.com", "s", false) + + private val account = Account( + id = "acct", + email = "user@yahoo.com", + displayName = "User", + authType = AuthType.PASSWORD_IMAP, + imap = ServerConfig(YAHOO_HOST, PORT, MailSecurity.SSL_TLS), + smtp = ServerConfig("smtp.mail.yahoo.com", 465, MailSecurity.SSL_TLS), + ) + + private fun context(): Context = mockk().apply { + every { getString(R.string.account_auth_error_remove_readd) } returns MESSAGE + } + + @Before + fun setUp() { + mockkStatic(android.util.Log::class) + every { android.util.Log.w(any(), any()) } returns 0 + AppLog.install(logBuffer) + } + + @After + fun tearDown() = unmockkAll() + + @Test + fun `a latched account is stamped with the remove-and-re-add error and reported latched`() = runTest { + val gate = latchingGate() + val p = params() + repeat(THRESHOLD) { gate.onAuthFailure(p) } + logBuffer.clear() // isolate the helper's breadcrumb from the gate's own latch breadcrumbs + val dao = mockk() + coEvery { dao.setAuthError("acct", MESSAGE) } returns 1 + + val latched = markAccountErroredIfLatched(gate, dao, context(), account, p) + + assertTrue(latched, "a latched account is reported so the caller stops syncing it") + coVerify(exactly = 1) { dao.setAuthError("acct", MESSAGE) } + val messages = logBuffer.snapshot().map { it.message } + assertTrue(messages.any { it.contains("account marked errored") }, "a PII-free breadcrumb is logged") + messages.forEach { + assertFalse(it.contains("user@yahoo.com"), it) + assertFalse(it.contains(YAHOO_HOST), it) + } + } + + @Test + fun `an idempotent no-op write is not logged but still reports latched`() = runTest { + val gate = latchingGate() + val p = params() + repeat(THRESHOLD) { gate.onAuthFailure(p) } + logBuffer.clear() // isolate the helper's (non-)logging from the gate's own latch breadcrumbs + val dao = mockk() + // The conditional UPDATE changed 0 rows (the message is already stored) — no re-log. + coEvery { dao.setAuthError("acct", MESSAGE) } returns 0 + + val latched = markAccountErroredIfLatched(gate, dao, context(), account, p) + + assertTrue(latched) + assertTrue(logBuffer.snapshot().isEmpty(), "a no-op re-write must not re-log the latch") + } + + @Test + fun `a still-ramping account is not errored`() = runTest { + val gate = latchingGate() + val p = params() + gate.onAuthFailure(p) // one failure — below the threshold, still ramping (not latched) + val dao = mockk() + + val latched = markAccountErroredIfLatched(gate, dao, context(), account, p) + + assertFalse(latched, "a ramping account keeps retrying, it is not errored") + coVerify(exactly = 0) { dao.setAuthError(any(), any()) } + } + + @Test + fun `a healthy non-gated account is never errored`() = runTest { + // A disabled policy (non-Yahoo host) never latches, so this is a total no-op. + val gate = AuthThrottleGate( + nowMillis = { 0L }, + random = { 0.0 }, + policyForHost = { AuthCadencePolicy.DISABLED }, + ) + val p = params() + repeat(THRESHOLD * 2) { gate.onAuthFailure(p) } + val dao = mockk() + + assertFalse(markAccountErroredIfLatched(gate, dao, context(), account, p)) + coVerify(exactly = 0) { dao.setAuthError(any(), any()) } + } + + private companion object { + const val YAHOO_HOST = "imap.mail.yahoo.com" + const val PORT = 993 + const val THRESHOLD = 4 + const val MESSAGE = "Please remove and re-add this account with valid credentials" + } +} diff --git a/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt b/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt index e268676..4f92cdf 100644 --- a/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt +++ b/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt @@ -563,6 +563,58 @@ class MailBackfillerTest { ) } + /** + * Issue #362 fail-loud stop: an account already persisted as errored (its Yahoo/AOL auth circuit has + * latched) is skipped entirely — no server call, no `moreWork` — durably across restarts, since the + * skip reads the persisted [AccountEntity.authError] rather than only the in-memory gate. It stays + * skipped until the user re-adds the account, which clears the error. + */ + @Test + fun `an errored account is skipped, not paged`() = runTest { + cached += fetchedMessage(uid = "60").toEntity("acct", "INBOX") + val imapClient = mockk(relaxed = true) + val errored = accountEntity.copy(authError = "Please remove and re-add this account with valid credentials") + + val moreWork = backfiller(AccountSettings("acct"), imapClient = imapClient, account = errored).runBackfill() + + assertFalse(moreWork, "an errored account reports done, not more-work") + coVerify(exactly = 0) { imapClient.fetchOlderThan(any(), any(), any(), any()) } + assertTrue( + logBuffer.snapshot().any { + it.message.startsWith("backfill skip acct:") && it.message.contains("errored") + }, + "a PII-free errored-skip breadcrumb is recorded", + ) + } + + /** + * The mid-slice latch (#362): a gate that has just latched (via any path) but whose account row is not + * yet stamped is marked errored (markAccountErroredIfLatched) and skipped — no page fetched, no more-work. + */ + @Test + fun `a freshly latched account is marked errored and skipped, not paged`() = runTest { + cached += fetchedMessage(uid = "60").toEntity("acct", "INBOX") + val imapClient = mockk(relaxed = true) + val authGate = AuthThrottleGate( + nowMillis = { 0L }, + random = { 0.0 }, + policyForHost = { ProviderAuthPolicy.forHost("imap.mail.yahoo.com") }, + ) + repeat(ProviderAuthPolicy.YAHOO_AUTH_CIRCUIT_OPEN_THRESHOLD) { authGate.onAuthFailure(params()) } + + val moreWork = backfiller(AccountSettings("acct"), imapClient = imapClient, authGate = authGate).runBackfill() + + assertFalse(moreWork, "a latched account reports done, not more-work") + coVerify(exactly = 0) { imapClient.fetchOlderThan(any(), any(), any(), any()) } + assertTrue(authGate.isAuthLatched(params()), "the account is latched") + assertTrue( + logBuffer.snapshot().any { + it.message.startsWith("backfill skip acct:") && it.message.contains("latched") + }, + "a PII-free latched-skip breadcrumb is recorded", + ) + } + // --- issue #355: interactive-fetch priority ------------------------------------------------- /** @@ -776,6 +828,7 @@ class MailBackfillerTest { ): MailBackfiller { val accountDao = mockk() coEvery { accountDao.getAll() } returns listOf(account) + coEvery { accountDao.setAuthError(any(), any()) } returns 1 val messageDao = mockk(relaxed = true) coEvery { messageDao.insertNew(any()) } answers { diff --git a/app/src/test/kotlin/org/libremail/data/sync/MailSyncConcurrencyTest.kt b/app/src/test/kotlin/org/libremail/data/sync/MailSyncConcurrencyTest.kt index 2ff7539..29894c7 100644 --- a/app/src/test/kotlin/org/libremail/data/sync/MailSyncConcurrencyTest.kt +++ b/app/src/test/kotlin/org/libremail/data/sync/MailSyncConcurrencyTest.kt @@ -333,6 +333,7 @@ class MailSyncConcurrencyTest { mailRepository = mockk(relaxed = true), throttleGate = AccountThrottleGate(), bandwidthTracker = GmailBandwidthTracker(), + authGate = AuthThrottleGate(), ) } diff --git a/app/src/test/kotlin/org/libremail/data/sync/MailSyncerTest.kt b/app/src/test/kotlin/org/libremail/data/sync/MailSyncerTest.kt index 0e36516..a4a87fc 100644 --- a/app/src/test/kotlin/org/libremail/data/sync/MailSyncerTest.kt +++ b/app/src/test/kotlin/org/libremail/data/sync/MailSyncerTest.kt @@ -28,9 +28,12 @@ import org.libremail.data.settings.FetchPolicy import org.libremail.data.settings.SettingsRepository import org.libremail.domain.model.AccountSettings import org.libremail.domain.model.ImapConnectionParams +import org.libremail.domain.model.MailSecurity import org.libremail.domain.repository.MailRepository +import org.libremail.mail.AuthThrottleGate import org.libremail.mail.FetchedMessage import org.libremail.mail.ImapClient +import org.libremail.mail.ProviderAuthPolicy import org.libremail.notifications.MailNotifier import org.libremail.power.BatteryStatus import org.libremail.power.BatteryStatusProvider @@ -96,9 +99,10 @@ class MailSyncerTest { fetched: List = emptyList(), throttleGate: AccountThrottleGate = AccountThrottleGate(), bandwidthTracker: GmailBandwidthTracker = GmailBandwidthTracker(), + authGate: AuthThrottleGate = AuthThrottleGate(), accountEntity: AccountEntity = account, ): MailSyncer { - val accountDao = mockk() + val accountDao = mockk(relaxed = true) coEvery { accountDao.getById("acct") } returns accountEntity val messageDao = mockk(relaxed = true) coEvery { messageDao.getSyncedIds(any(), any()) } returns emptyList() @@ -108,7 +112,7 @@ class MailSyncerTest { coEvery { imapClient.fetchRecent(any(), any(), any()) } returns fetched lastImapClient = imapClient val connectionFactory = mockk() - coEvery { connectionFactory.imapParamsFor(any()) } returns mockk() + coEvery { connectionFactory.imapParamsFor(any()) } returns realParams() val settingsRepository = mockk() coEvery { settingsRepository.fetchPolicy() } returns policy every { settingsRepository.settings } returns flowOf(globalSettings) @@ -127,12 +131,71 @@ class MailSyncerTest { mailRepository = mailRepository, throttleGate = throttleGate, bandwidthTracker = bandwidthTracker, + authGate = authGate, ) } + /** A real (non-mock) params object, so the proactive auth gate can key by host|port|username (#362). */ + private fun realParams() = ImapConnectionParams( + host = "imap.example.org", + port = 993, + security = MailSecurity.SSL_TLS, + username = "a@example.org", + secret = "secret", + useXoauth2 = false, + ) + private fun batteryProvider(battery: BatteryStatus): BatteryStatusProvider = mockk { every { current() } returns battery } + /** + * Issue #362 fail-loud stop: an account already persisted as errored (its Yahoo/AOL auth circuit has + * latched) is skipped entirely — no login attempt, no fetch — durably across restarts, since the skip + * reads the persisted [AccountEntity.authError] rather than the in-memory gate. The sync still reports + * success (contributes 0) so a healthy sibling account is unaffected. + */ + @Test + fun `an errored account is skipped without any fetch`() = runTest { + val syncer = syncer( + FetchPolicy.ON_DEMAND, + mockk(relaxed = true), + accountEntity = account.copy(authError = "Please remove and re-add this account with valid credentials"), + ) + + val result = syncer.syncFolder("acct", "INBOX") + + assertEquals(0, result.getOrNull(), "an errored account contributes nothing and does not fail the sync") + coVerify(exactly = 0) { lastImapClient.fetchRecent(any(), any(), any()) } + assertTrue( + logBuffer.snapshot().any { it.message.startsWith("sync skip acct:") && it.message.contains("errored") }, + "a PII-free skip breadcrumb is recorded", + ) + } + + /** + * The mid-sync latch (#362): when the gate has already latched (e.g. via a prior IDLE/backfill failure) + * but the account row is not yet stamped, the next sync marks it errored via [markAccountErroredIfLatched] + * and skips the login rather than driving one the gate would only refuse. + */ + @Test + fun `a freshly latched account is marked errored and skipped without a fetch`() = runTest { + val gate = AuthThrottleGate( + nowMillis = { 0L }, + random = { 0.0 }, + policyForHost = { ProviderAuthPolicy.forHost("imap.mail.yahoo.com") }, + ) + repeat(ProviderAuthPolicy.YAHOO_AUTH_CIRCUIT_OPEN_THRESHOLD) { gate.onAuthFailure(realParams()) } + val syncer = syncer(FetchPolicy.ON_DEMAND, mockk(relaxed = true), authGate = gate) + + val result = syncer.syncFolder("acct", "INBOX") + + assertEquals(0, result.getOrNull()) + coVerify(exactly = 0) { lastImapClient.fetchRecent(any(), any(), any()) } + assertTrue( + logBuffer.snapshot().any { it.message.startsWith("sync skip acct:") && it.message.contains("latched") }, + ) + } + @Test fun `ALWAYS policy prefetches unfetched messages after the header sync`() = runTest { val repo = mockk() @@ -360,7 +423,7 @@ class MailSyncerTest { FetchedMessage("1", "Ada", "ada@example.org", "Hi", 1_000L, isRead = false, isFlagged = false), ) val connectionFactory = mockk() - coEvery { connectionFactory.imapParamsFor(any()) } returns mockk() + coEvery { connectionFactory.imapParamsFor(any()) } returns realParams() val settingsRepository = mockk() coEvery { settingsRepository.isNewMailNotificationsEnabled() } returns globalEnabled coEvery { settingsRepository.fetchPolicy() } returns FetchPolicy.ON_DEMAND @@ -381,6 +444,7 @@ class MailSyncerTest { mailRepository = mockk(relaxed = true), throttleGate = AccountThrottleGate(), bandwidthTracker = GmailBandwidthTracker(), + authGate = AuthThrottleGate(), ) } @@ -445,7 +509,7 @@ class MailSyncerTest { ) val connectionFactory = mockk() coEvery { connectionFactory.imapParamsFor(match { it.id in failingIds }) } throws IOException("no network") - coEvery { connectionFactory.imapParamsFor(match { it.id !in failingIds }) } returns mockk() + coEvery { connectionFactory.imapParamsFor(match { it.id !in failingIds }) } returns realParams() val settingsRepository = mockk() coEvery { settingsRepository.fetchPolicy() } returns FetchPolicy.ON_DEMAND coEvery { settingsRepository.isNewMailNotificationsEnabled() } returns false @@ -465,6 +529,7 @@ class MailSyncerTest { mailRepository = mockk(relaxed = true), throttleGate = AccountThrottleGate(), bandwidthTracker = GmailBandwidthTracker(), + authGate = AuthThrottleGate(), ) } diff --git a/app/src/test/kotlin/org/libremail/mail/AuthThrottleGateTest.kt b/app/src/test/kotlin/org/libremail/mail/AuthThrottleGateTest.kt index 42e4e13..9ce276a 100644 --- a/app/src/test/kotlin/org/libremail/mail/AuthThrottleGateTest.kt +++ b/app/src/test/kotlin/org/libremail/mail/AuthThrottleGateTest.kt @@ -101,17 +101,52 @@ class AuthThrottleGateTest { } @Test - fun `the circuit opens after the threshold to a long fixed window and stays open`() { + fun `the circuit latches after the threshold and never self-clears`() { val gate = gate() val p = params() - var lastBlock = 0L - repeat(yahoo.circuitOpenThreshold) { lastBlock = gate.onAuthFailure(p) } - assertEquals(yahoo.circuitOpenMillis, lastBlock, "the threshold failure opens the fixed circuit window") - - // A further failure stays open at the same fixed window (no runaway escalation). - assertEquals(yahoo.circuitOpenMillis, gate.onAuthFailure(p)) + repeat(yahoo.circuitOpenThreshold) { gate.onAuthFailure(p) } + assertTrue(gate.isAuthLatched(p), "reaching the threshold latches the circuit") assertTrue(gate.isAuthBlocked(p)) + + // The OLD self-clearing open-circuit window is gone (issue #362): advancing far past what used to be + // the 30-min window must NOT unblock — a wrong app-password does not fix itself, so we stop for good. + now += yahoo.circuitOpenMillis * LATCH_ELAPSE_FACTOR + assertTrue(gate.isAuthBlocked(p), "a latched circuit never self-clears with time") + assertTrue(gate.isAuthLatched(p)) + + // A further failure neither escalates nor changes the latch — the state stays frozen. + gate.onAuthFailure(p) + assertTrue(gate.isAuthLatched(p)) + assertTrue(gate.isAuthBlocked(p)) + } + + @Test + fun `a success does not clear a latched circuit`() { + val gate = gate() + val p = params() + + repeat(yahoo.circuitOpenThreshold) { gate.onAuthFailure(p) } + assertTrue(gate.isAuthLatched(p)) + + // Defensive: even if a login somehow succeeded, a latched account stays errored until a re-add. + gate.onAuthSuccess(p) + assertTrue(gate.isAuthLatched(p), "only a re-add clears a latch, never a success") + assertTrue(gate.isAuthBlocked(p)) + } + + @Test + fun `re-adding the account clears a latched circuit`() { + val gate = gate() + val p = params() + + repeat(yahoo.circuitOpenThreshold) { gate.onAuthFailure(p) } + assertTrue(gate.isAuthLatched(p)) + + gate.onAccountReadded(p) + + assertFalse(gate.isAuthLatched(p), "a re-add drops the latch") + assertFalse(gate.isAuthBlocked(p), "and the account may attempt a fresh login again") } @Test @@ -233,5 +268,8 @@ class AuthThrottleGateTest { private companion object { const val PORT = 993 const val RAPID_FAILURES = 10 + + /** How many old open-circuit windows to fast-forward to prove a latch never self-clears. */ + const val LATCH_ELAPSE_FACTOR = 10 } } diff --git a/app/src/test/kotlin/org/libremail/ui/mailbox/MailboxScreenJvmTest.kt b/app/src/test/kotlin/org/libremail/ui/mailbox/MailboxScreenJvmTest.kt index 8530f95..ea01f64 100644 --- a/app/src/test/kotlin/org/libremail/ui/mailbox/MailboxScreenJvmTest.kt +++ b/app/src/test/kotlin/org/libremail/ui/mailbox/MailboxScreenJvmTest.kt @@ -165,6 +165,21 @@ class MailboxScreenJvmTest { assertTrue(composeTestRule.onAllNodesWithText("b@example.org").fetchSemanticsNodes().isNotEmpty()) } + // #362: an account whose auth circuit has latched carries a persisted authError; the mailbox shows a + // persistent (non-dismissable) banner with the "remove and re-add" message while that state holds. + @Test + fun erroredAccount_showsPersistentAuthErrorBanner() { + val errored = account.copy( + authError = string(R.string.account_auth_error_remove_readd), + ) + setContent(accounts = listOf(errored), messages = listOf(message("1", subject = "Msg"))) + waitForText("Msg") + + composeTestRule + .onNodeWithText(string(R.string.account_auth_error_remove_readd)) + .assertIsDisplayed() + } + @Test fun draftsAndOutboxEntries_showAtInbox_andNavigate() { var openedDrafts = false diff --git a/app/src/test/kotlin/org/libremail/ui/settings/AccountReorderListRenderTest.kt b/app/src/test/kotlin/org/libremail/ui/settings/AccountReorderListRenderTest.kt new file mode 100644 index 0000000..cd08214 --- /dev/null +++ b/app/src/test/kotlin/org/libremail/ui/settings/AccountReorderListRenderTest.kt @@ -0,0 +1,74 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.ui.settings + +import androidx.compose.ui.test.assertIsDisplayed +import androidx.compose.ui.test.junit4.v2.createComposeRule +import androidx.compose.ui.test.onAllNodesWithText +import androidx.compose.ui.test.onNodeWithText +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.libremail.R +import org.libremail.domain.model.Account +import org.libremail.domain.model.AuthType +import org.libremail.domain.model.MailSecurity +import org.libremail.domain.model.ServerConfig +import org.libremail.ui.theme.LibreMailTheme +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import org.robolectric.annotation.Config +import org.robolectric.annotation.GraphicsMode +import kotlin.test.assertTrue + +/** + * Renders the Settings account list ([AccountReorderList]) on the JVM under Robolectric to prove the + * per-account auth-error indicator (issue #362): an errored account row shows the "remove and re-add" + * message beneath its address, while a healthy account shows none. + */ +@RunWith(RobolectricTestRunner::class) +@GraphicsMode(GraphicsMode.Mode.NATIVE) +@Config(sdk = [36]) +class AccountReorderListRenderTest { + + @get:Rule + val composeTestRule = createComposeRule() + + private val message: String + get() = RuntimeEnvironment.getApplication().getString(R.string.account_auth_error_remove_readd) + + private fun account(authError: String?) = Account( + id = "acct", + email = "a@example.org", + displayName = "A", + authType = AuthType.PASSWORD_IMAP, + imap = ServerConfig("imap.example.org", 993, MailSecurity.SSL_TLS), + smtp = ServerConfig("smtp.example.org", 465, MailSecurity.SSL_TLS), + authError = authError, + ) + + @Test + fun erroredAccountRow_showsTheAuthErrorMessage() { + composeTestRule.setContent { + LibreMailTheme { + AccountReorderList(accounts = listOf(account(message)), onOpenAccount = {}, onReorder = {}) + } + } + + composeTestRule.onNodeWithText("a@example.org").assertIsDisplayed() + composeTestRule.onNodeWithText(message).assertIsDisplayed() + } + + @Test + fun healthyAccountRow_showsNoAuthError() { + composeTestRule.setContent { + LibreMailTheme { + AccountReorderList(accounts = listOf(account(null)), onOpenAccount = {}, onReorder = {}) + } + } + + assertTrue( + composeTestRule.onAllNodesWithText(message).fetchSemanticsNodes().isEmpty(), + "a healthy account row shows no auth-error message", + ) + } +} diff --git a/config/detekt/detekt.yml b/config/detekt/detekt.yml index 3bb9f85..4bfc97b 100644 --- a/config/detekt/detekt.yml +++ b/config/detekt/detekt.yml @@ -27,8 +27,11 @@ complexity: # plus folder-resolution / spam cases) already at detekt's LLOC boundary; the reader-path perf # logging (issue #358) added its required android.util.Log mock + one breadcrumb test, tipping it # over. Excluded rather than artificially split — same "operation-rich cohesive suite" rationale as - # the TooManyFunctions relaxation above. - excludes: ['**/data/repository/MailRepositoryImplTest.kt'] + # the TooManyFunctions relaxation above. MailBackfillerTest is likewise one cohesive single-SUT + # backfill suite (a test per behaviour — resume, retention floors, throttle/auth/errored skips, + # batched persist); issue #362's fail-loud auth-error skip added its required errored/latched-skip + # tests, tipping it over the same boundary. + excludes: ['**/data/repository/MailRepositoryImplTest.kt', '**/data/sync/MailBackfillerTest.kt'] naming: FunctionNaming: -- 2.47.3 From d18c99280e181c15354b9411809f73d6dafa7df1 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 9 Jul 2026 12:40:42 -0500 Subject: [PATCH 3/3] fix(yahoo): sync AccountDataMigrator DDL to AccountDatabase v3 (authError) AccountDataMigrator hardcoded the v2 accounts DDL (schema/2.json), so copyAccountTables created a table missing the authError column PR #472 added in v3 (schema/3.json). Room then rejected the pre-packaged accounts table as an invalid schema, failing every AccountDataMigratorTest instrumented test on every API level. Add `authError` TEXT to CREATE_TABLE_SQL["accounts"] to match the exported v3 schema exactly, and repoint the DDL-guard test (migratorDdlMatchesExportedAccountDatabaseSchema) at 3.json so it would have caught this drift. --- .../org/libremail/data/local/AccountDataMigratorTest.kt | 2 +- .../org/libremail/data/local/AccountDataMigrator.kt | 8 ++++---- 2 files changed, 5 insertions(+), 5 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 8db0847..6cd365c 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt @@ -275,7 +275,7 @@ class AccountDataMigratorTest { fun migratorDdlMatchesExportedAccountDatabaseSchema() { val schema = JSONObject( InstrumentationRegistry.getInstrumentation().context.assets - .open("org.libremail.data.local.AccountDatabase/2.json") + .open("org.libremail.data.local.AccountDatabase/3.json") .bufferedReader().use { it.readText() }, ).getJSONObject("database") val entities = schema.getJSONArray("entities") 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 af2423e..85d9a84 100644 --- a/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt +++ b/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt @@ -98,9 +98,9 @@ class AccountDataMigrator @Inject constructor( private val TABLES = listOf("accounts", "credentials", "account_settings", "signatures") /** - * DDL for the account tables in [AccountDatabase] v2, copied verbatim from the exported Room - * schema (`schemas/org.libremail.data.local.AccountDatabase/2.json` — v2 added `accounts.sortOrder`, - * issue #164). It MUST stay byte-for-byte identical to what Room generates for those entities, or + * DDL for the account tables in [AccountDatabase] v3, copied verbatim from the exported Room + * schema (`schemas/org.libremail.data.local.AccountDatabase/3.json` — v3 added `accounts.authError`, + * issue #362). 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 @@ -110,7 +110,7 @@ class AccountDataMigrator @Inject constructor( "accounts" to "CREATE TABLE IF NOT EXISTS `accounts` (`id` TEXT NOT NULL, `email` TEXT NOT NULL, " + "`displayName` TEXT NOT NULL, `authType` TEXT NOT NULL, " + - "`sortOrder` INTEGER NOT NULL DEFAULT 0, `imap_host` TEXT NOT NULL, " + + "`sortOrder` INTEGER NOT NULL DEFAULT 0, `authError` TEXT, `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 -- 2.47.3