From 8fac4c11252335b2f2075f7c60a352f9639dcb6e Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Tue, 7 Jul 2026 23:17:52 -0500 Subject: [PATCH 1/5] fix(push): persist credentials before IdleService watches a new account (#403) At account-add both LibreMailApplication's push collector and IdleService.reconcileWatchers react to the accounts table. The account row was inserted before its credential was saved, so a watcher could observe the new account and call MailConnectionFactory.resolveSecret before the secret existed, logging "No stored credentials" on the first IDLE attempt (it self-healed on retry, but fired a failed IDLE + log noise on every add). Primary fix: reorder the writes so the credential is committed before the account row (the credentials table has no FK to accounts; account_settings does, so its ensureDefaults still follows the insert). Any reactive observer of the account row is then guaranteed to see the credential. Defense-in-depth: resolveSecret now throws a typed MissingCredentialsException and the IDLE watcher treats it as a transient miss, deferring quietly (short flat re-check, PII-free info log) instead of the warn + exponential backoff a real connection drop gets. Genuinely-absent credentials keep deferring without noise. Tests: AccountRepositoryImplTest pins the credential-before-row order (coVerifyOrder); MailConnectionFactoryTest asserts the typed exception; a new instrumented test drives the real repository add path against a real AccountDatabase + Keystore-backed CredentialStore and proves the secret is resolvable the instant the account row becomes observable. --- ...ntAddCredentialOrderingInstrumentedTest.kt | 144 ++++++++++++++++++ .../data/repository/AccountRepositoryImpl.kt | 22 ++- .../data/sync/MailConnectionFactory.kt | 4 +- .../data/sync/MissingCredentialsException.kt | 20 +++ .../kotlin/org/libremail/push/IdleService.kt | 17 +++ .../repository/AccountRepositoryImplTest.kt | 51 +++++++ .../data/sync/MailConnectionFactoryTest.kt | 6 +- config/detekt/detekt.yml | 3 + 8 files changed, 261 insertions(+), 6 deletions(-) create mode 100644 app/src/androidTest/kotlin/org/libremail/data/repository/AccountAddCredentialOrderingInstrumentedTest.kt create mode 100644 app/src/main/kotlin/org/libremail/data/sync/MissingCredentialsException.kt diff --git a/app/src/androidTest/kotlin/org/libremail/data/repository/AccountAddCredentialOrderingInstrumentedTest.kt b/app/src/androidTest/kotlin/org/libremail/data/repository/AccountAddCredentialOrderingInstrumentedTest.kt new file mode 100644 index 0000000..418d6c7 --- /dev/null +++ b/app/src/androidTest/kotlin/org/libremail/data/repository/AccountAddCredentialOrderingInstrumentedTest.kt @@ -0,0 +1,144 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.repository + +import android.content.Context +import androidx.room.Room +import androidx.test.core.app.ApplicationProvider +import androidx.test.ext.junit.runners.AndroidJUnit4 +import io.mockk.coEvery +import io.mockk.mockk +import io.mockk.unmockkAll +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.launch +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeout +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.libremail.data.attachment.AttachmentUriGrants +import org.libremail.data.local.AccountDatabase +import org.libremail.data.local.dao.BackfillProgressDao +import org.libremail.data.local.dao.DraftDao +import org.libremail.data.local.dao.FolderDao +import org.libremail.data.local.dao.MessageDao +import org.libremail.data.security.CredentialStore +import org.libremail.data.security.KeystoreCrypto +import org.libremail.data.settings.AccountSettingsRepository +import org.libremail.data.sync.SyncScheduler +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.FetchedFolder +import org.libremail.mail.ImapClient +import org.libremail.notifications.MailNotifier + +/** + * On-device proof of the #403 fix against real SQLite and the real Keystore-backed [CredentialStore]: + * when [AccountRepositoryImpl] adds an account, its credential must be resolvable the instant the new + * account row becomes observable — the exact moment the push watchers (LibreMailApplication's collector + * and [org.libremail.push.IdleService.reconcileWatchers], both keyed on the *accounts* table) react. + * + * The JVM `AccountRepositoryImplTest` pins the call ORDER with `coVerifyOrder`; this drives the same + * production code against a real in-memory [AccountDatabase] (real `accountDao` + real `credentialDao` + * via a real [CredentialStore]/[KeystoreCrypto]) so the transaction-commit ordering — not just the call + * ordering — is exercised. A background collector reads the credential the moment the account first + * appears, reproducing the reactive watcher; with the secret committed before the row it always + * resolves. Non-DB collaborators (IMAP, scheduler, notifier, settings) are mocked the same way + * `WorkerCacheLockDeferralInstrumentedTest` fakes its non-framework collaborators — never a framework + * `Context`, which is the real application context. + */ +@RunWith(AndroidJUnit4::class) +class AccountAddCredentialOrderingInstrumentedTest { + + private val context: Context = ApplicationProvider.getApplicationContext() + + private lateinit var db: AccountDatabase + private lateinit var credentialStore: CredentialStore + private lateinit var imapClient: ImapClient + private lateinit var repository: AccountRepositoryImpl + + @Before + fun setUp() { + db = Room.inMemoryDatabaseBuilder(context, AccountDatabase::class.java).build() + credentialStore = CredentialStore(KeystoreCrypto(), db.credentialDao()) + imapClient = mockk() + coEvery { imapClient.listFolders(any()) } returns listOf( + FetchedFolder("INBOX", "INBOX", emptyList(), selectable = true), + ) + repository = AccountRepositoryImpl( + context = context, + accountDao = db.accountDao(), + messageDao = mockk(relaxed = true), + folderDao = mockk(relaxed = true), + backfillProgressDao = mockk(relaxed = true), + draftDao = mockk(relaxed = true), + credentialStore = credentialStore, + imapClient = imapClient, + syncScheduler = mockk(relaxed = true), + accountSettingsRepository = mockk(relaxed = true), + mailNotifier = mockk(relaxed = true), + attachmentUriGrants = mockk(relaxed = true), + ) + } + + @After + fun tearDown() { + db.close() + unmockkAll() + } + + @Test + fun newAccountRowIsObservableOnlyAfterItsCredentialIsResolvable() = runBlocking { + val account = imapAccount() + + // A reactive watcher: read the stored secret the moment the new account row first appears in the + // observed accounts list — exactly what IdleService.reconcileWatchers does before opening IDLE. + val credentialAtFirstSight = CompletableDeferred() + val observer = launch(Dispatchers.IO) { + repository.observeAccounts().collect { accounts -> + if (accounts.any { it.id == account.id } && !credentialAtFirstSight.isCompleted) { + credentialAtFirstSight.complete(credentialStore.loadSecret(account.id)) + } + } + } + + repository.addImapAccount(account, PASSWORD).getOrThrow() + + assertEquals( + "the credential must be resolvable the instant the account row becomes observable (#403)", + PASSWORD, + withTimeout(TIMEOUT_MS) { credentialAtFirstSight.await() }, + ) + observer.cancel() + } + + @Test + fun addImapAccountLeavesTheCredentialResolvableForTheStoredAccount() = runBlocking { + val account = imapAccount() + + repository.addImapAccount(account, PASSWORD).getOrThrow() + + // Durability backstop: the account is persisted AND its secret round-trips through the real + // Keystore-sealed store, so any later push (re)start resolves it rather than hitting a hard miss. + assertEquals(PASSWORD, credentialStore.loadSecret(account.id)) + assertEquals(account.id, db.accountDao().getById(account.id)?.id) + } + + private fun imapAccount(id: String = "imap:ada@example.org") = Account( + id = id, + email = "ada@example.org", + displayName = "Ada", + authType = AuthType.PASSWORD_IMAP, + imap = ServerConfig("imap.example.org", 993, MailSecurity.SSL_TLS), + smtp = ServerConfig("smtp.example.org", 587, MailSecurity.STARTTLS), + ) + + private companion object { + const val PASSWORD = "app-password" + const val TIMEOUT_MS = 5_000L + } +} 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 61f16be..07b8f84 100644 --- a/app/src/main/kotlin/org/libremail/data/repository/AccountRepositoryImpl.kt +++ b/app/src/main/kotlin/org/libremail/data/repository/AccountRepositoryImpl.kt @@ -24,6 +24,8 @@ import org.libremail.domain.model.ImapConnectionParams import org.libremail.domain.repository.AccountRepository import org.libremail.mail.ImapClient import org.libremail.notifications.MailNotifier +import org.libremail.reporting.AppLog +import org.libremail.reporting.accountLogRef import javax.inject.Inject import javax.inject.Singleton @@ -53,10 +55,17 @@ class AccountRepositoryImpl @Inject constructor( override suspend fun addImapAccount(account: Account, password: String): Result> = runCatching { val folders = imapClient.listFolders(account.toImapParams(secret = password, useXoauth2 = false)) + // 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 + // of firing a transient "No stored credentials" IDLE miss on every account add. The credentials + // table has no foreign key to accounts, so it can be written first; account_settings does (FK), + // so ensureDefaults must still follow the account row. + credentialStore.saveSecret(account.id, password) accountDao.insertAtEnd(account.toEntity()) accountSettingsRepository.ensureDefaults(account.id) - credentialStore.saveSecret(account.id, password) mailNotifier.ensureAccountChannel(account) + AppLog.i(TAG, "IMAP account added ${accountLogRef(account.id)}; credential persisted before account row") syncScheduler.syncNow() syncScheduler.backfillNow() // start caching this account's full history in the background (#12) folders.map { it.fullName } @@ -69,10 +78,15 @@ class AccountRepositoryImpl @Inject constructor( ): Result> = runCatching { val account = Account.outlook(email) val folders = imapClient.listFolders(account.toImapParams(secret = accessToken, useXoauth2 = true)) + // 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 + // ensureDefaults follows the insert. + credentialStore.saveSecret(account.id, authStateJson) accountDao.insertAtEnd(account.toEntity()) accountSettingsRepository.ensureDefaults(account.id) - credentialStore.saveSecret(account.id, authStateJson) mailNotifier.ensureAccountChannel(account) + AppLog.i(TAG, "Outlook account added ${accountLogRef(account.id)}; credential persisted before account row") syncScheduler.syncNow() syncScheduler.backfillNow() // start caching this account's full history in the background (#12) folders.map { it.fullName } @@ -113,4 +127,8 @@ class AccountRepositoryImpl @Inject constructor( if (accountId != null) backfillProgressDao.deleteForAccount(accountId) else backfillProgressDao.deleteAll() syncScheduler.backfillNow() } + + private companion object { + const val TAG = "AccountRepository" + } } diff --git a/app/src/main/kotlin/org/libremail/data/sync/MailConnectionFactory.kt b/app/src/main/kotlin/org/libremail/data/sync/MailConnectionFactory.kt index 26bdf62..ec29fe7 100644 --- a/app/src/main/kotlin/org/libremail/data/sync/MailConnectionFactory.kt +++ b/app/src/main/kotlin/org/libremail/data/sync/MailConnectionFactory.kt @@ -51,7 +51,7 @@ class MailConnectionFactory @Inject constructor( private suspend fun resolveSecret(account: Account): String = when (account.authType) { AuthType.PASSWORD_IMAP -> - credentialStore.loadSecret(account.id) ?: error("No stored credentials for ${account.email}") + credentialStore.loadSecret(account.id) ?: throw MissingCredentialsException() AuthType.OAUTH_OUTLOOK -> cachedAccessToken(account.id, SCOPE_OUTLOOK, outlookAuthManager::freshOutlookToken) } @@ -70,7 +70,7 @@ class MailConnectionFactory @Inject constructor( // computeIfAbsent (not getOrPut) so concurrent first-callers share one mutex per account. return refreshMutexes.computeIfAbsent(accountId) { Mutex() }.withLock { validCachedToken(accountId, scope)?.let { return@withLock it } - val stored = credentialStore.loadSecret(accountId) ?: error("No stored credentials for $accountId") + val stored = credentialStore.loadSecret(accountId) ?: throw MissingCredentialsException() val fresh = refresh(stored) if (fresh.authStateJson != stored) credentialStore.saveSecret(accountId, fresh.authStateJson) tokenCache["$accountId|$scope"] = CachedToken(fresh.accessToken, fresh.accessTokenExpiry) diff --git a/app/src/main/kotlin/org/libremail/data/sync/MissingCredentialsException.kt b/app/src/main/kotlin/org/libremail/data/sync/MissingCredentialsException.kt new file mode 100644 index 0000000..55667cf --- /dev/null +++ b/app/src/main/kotlin/org/libremail/data/sync/MissingCredentialsException.kt @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.sync + +/** + * Thrown by [MailConnectionFactory] when an account has no stored credential to resolve. + * + * It extends [IllegalStateException] — the type [kotlin.error] previously raised here — so existing + * callers that treat a missing credential as a hard failure are unaffected. What it adds is a type a + * caller can catch *specifically*: the IMAP IDLE watcher ([org.libremail.push.IdleService]) tolerates + * the brief account-add write race (#403), where a reactive observer of the accounts table can see a + * newly-added account a beat before its secret has finished persisting, by catching this and deferring + * instead of logging a connection failure. Genuinely-absent credentials still surface as an error to + * every other caller. + * + * The message is deliberately PII-free (no email, host, or account id): an account id embeds the raw + * email address, so it must never appear in an exception message that could reach Logcat. A caller that + * needs to name the account in a log line uses [org.libremail.reporting.accountLogRef] on the id it + * already holds. + */ +class MissingCredentialsException : IllegalStateException("No stored credentials for account") diff --git a/app/src/main/kotlin/org/libremail/push/IdleService.kt b/app/src/main/kotlin/org/libremail/push/IdleService.kt index 6120467..8c3996e 100644 --- a/app/src/main/kotlin/org/libremail/push/IdleService.kt +++ b/app/src/main/kotlin/org/libremail/push/IdleService.kt @@ -31,6 +31,7 @@ import org.libremail.data.local.toDomain import org.libremail.data.security.EncryptedCacheGuard import org.libremail.data.sync.MailConnectionFactory import org.libremail.data.sync.MailSyncer +import org.libremail.data.sync.MissingCredentialsException import org.libremail.data.sync.PushMode import org.libremail.data.sync.SyncResourcePolicy import org.libremail.data.sync.SyncScheduler @@ -301,6 +302,16 @@ class IdleService : Service() { backoffMs = INITIAL_BACKOFF_MS } catch (e: CancellationException) { throw e + } catch (ignored: MissingCredentialsException) { + // #403: a just-added account can be observed here a beat before its secret finishes + // persisting. AccountRepository now commits the secret before the account row, so this is + // rare — but tolerate any residual race as a transient miss: defer quietly and re-check + // soon, WITHOUT the warn + exponential backoff a real connection drop gets. A genuinely + // absent credential simply keeps deferring (no mail, but no error noise) until it appears + // or the account is removed. The sentinel exception carries no diagnostic value beyond the + // message below, so it is intentionally not re-logged with its (empty) trace. + AppLog.i(TAG, "IDLE deferred ${accountLogRef(account.id)}: credentials not yet persisted") + delay(CREDENTIALS_DEFER_RETRY_MS) } catch (e: Exception) { AppLog.w(TAG, "IDLE for ${accountLogRef(account.id)} dropped; retrying in ${backoffMs}ms", e) delay(backoffMs) @@ -332,6 +343,12 @@ class IdleService : Service() { const val INITIAL_BACKOFF_MS = 5_000L const val MAX_BACKOFF_MS = 5 * 60_000L + // #403: how long to wait before re-checking after a transient missing-credential miss on a + // just-added account. Short — the account-add write race resolves in milliseconds once the + // secret commit lands — and deliberately flat (no exponential escalation) because this is an + // expected persist-ordering blip, not a connection failure. + const val CREDENTIALS_DEFER_RETRY_MS = 1_000L + // Cadence of the reuse-cache idle-eviction sweep (issue #357 Part 2). Tighter than the reuse // idle timeout so an idle socket is closed shortly after it crosses it. const val REUSE_EVICTION_SWEEP_MS = 2 * 60_000L 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 3e30517..c348fa5 100644 --- a/app/src/test/kotlin/org/libremail/data/repository/AccountRepositoryImplTest.kt +++ b/app/src/test/kotlin/org/libremail/data/repository/AccountRepositoryImplTest.kt @@ -2,17 +2,23 @@ package org.libremail.data.repository import android.content.Context +import android.util.Log import app.cash.turbine.test import io.mockk.Runs import io.mockk.coEvery import io.mockk.coVerify +import io.mockk.coVerifyOrder import io.mockk.every import io.mockk.just import io.mockk.mockk +import io.mockk.mockkStatic import io.mockk.slot +import io.mockk.unmockkAll import io.mockk.verify import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.test.runTest +import org.junit.After +import org.junit.Before import org.junit.Test import org.libremail.data.attachment.AttachmentUriGrants import org.libremail.data.attachmentCacheDir @@ -76,6 +82,17 @@ class AccountRepositoryImplTest { attachmentUriGrants = attachmentUriGrants, ) + // addImapAccount/addOutlookAccount now breadcrumb via AppLog (#403); android.util.Log is a no-op + // stub under plain JVM tests, so mock it class-wide so no test crashes on the unmocked method. + @Before + fun setUp() { + mockkStatic(Log::class) + every { Log.i(any(), any()) } returns 0 + } + + @After + fun tearDown() = unmockkAll() + @Test fun `observeAccounts maps the stored account rows to domain models`() = runTest { every { accountDao.observeAll() } returns flowOf(listOf(accountEntity())) @@ -134,6 +151,40 @@ class AccountRepositoryImplTest { verify { syncScheduler.backfillNow() } } + @Test + fun `addImapAccount persists the credential before the account row (issue 403)`() = runTest { + val account = account() + coEvery { imapClient.listFolders(any()) } returns listOf( + FetchedFolder("INBOX", "INBOX", emptyList(), selectable = true), + ) + coEvery { accountDao.insertAtEnd(any()) } just Runs + + repository.addImapAccount(account, "app-password").getOrThrow() + + // The push collector (LibreMailApplication) and IdleService.reconcileWatchers both react to the + // accounts table; the secret must be committed FIRST so a watcher observing the new row can + // resolve it, rather than logging a transient "No stored credentials" IDLE miss on every add. + coVerifyOrder { + credentialStore.saveSecret(account.id, "app-password") + accountDao.insertAtEnd(any()) + } + } + + @Test + fun `addOutlookAccount persists the credential before the account row (issue 403)`() = runTest { + coEvery { imapClient.listFolders(any()) } returns listOf( + FetchedFolder("INBOX", "INBOX", emptyList(), selectable = true), + ) + coEvery { accountDao.insertAtEnd(any()) } just Runs + + repository.addOutlookAccount("me@outlook.com", "access-token", "{authstate}").getOrThrow() + + coVerifyOrder { + credentialStore.saveSecret("outlook:me@outlook.com", "{authstate}") + accountDao.insertAtEnd(any()) + } + } + @Test fun `addImapAccount persists nothing when the initial folder list fails`() = runTest { coEvery { imapClient.listFolders(any()) } throws RuntimeException("bad credentials") diff --git a/app/src/test/kotlin/org/libremail/data/sync/MailConnectionFactoryTest.kt b/app/src/test/kotlin/org/libremail/data/sync/MailConnectionFactoryTest.kt index 5256161..5d846f7 100644 --- a/app/src/test/kotlin/org/libremail/data/sync/MailConnectionFactoryTest.kt +++ b/app/src/test/kotlin/org/libremail/data/sync/MailConnectionFactoryTest.kt @@ -80,7 +80,9 @@ class MailConnectionFactoryTest { fun `a missing password credential is a hard error`() = runTest { coEvery { credentialStore.loadSecret("acct") } returns null - assertFailsWith { factory().imapParamsFor(passwordAccount) } + // A typed MissingCredentialsException (#403), not a bare IllegalStateException, so the IMAP IDLE + // watcher can catch it specifically and defer on the account-add write race. + assertFailsWith { factory().imapParamsFor(passwordAccount) } } @Test @@ -170,6 +172,6 @@ class MailConnectionFactoryTest { fun `a missing OAuth credential is a hard error`() = runTest { coEvery { credentialStore.loadSecret(outlookAccount.id) } returns null - assertFailsWith { factory().graphTokenFor(outlookAccount) } + assertFailsWith { factory().graphTokenFor(outlookAccount) } } } diff --git a/config/detekt/detekt.yml b/config/detekt/detekt.yml index 0b34fe2..f190f3c 100644 --- a/config/detekt/detekt.yml +++ b/config/detekt/detekt.yml @@ -74,6 +74,9 @@ style: # log via AppLog, so their unit tests mockkStatic(Log) too. - '**/data/repository/MailRepositoryImplTest.kt' - '**/data/repository/MailRepositoryImplCoverageTest.kt' + # Account-add breadcrumb (issue #403): addImapAccount/addOutlookAccount log via AppLog, so this + # suite mockkStatic(Log) so the calls don't crash on the throwing JVM stub. + - '**/data/repository/AccountRepositoryImplTest.kt' - '**/ui/reader/ReaderViewModelTest.kt' - '**/ui/reader/ReaderViewModelActionsTest.kt' MagicNumber: -- 2.47.3 From 08290dc85f1fa68887b03628f61a14ec955e5680 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Tue, 7 Jul 2026 23:18:40 -0500 Subject: [PATCH 2/5] fix(mail): gracefully fall back when the IMAP server lacks UIDPLUS The targeted UID EXPUNGE (IMAPFolder.expunge(Message[])) from #295/#318 throws "UID EXPUNGE not supported" on a server without the UIDPLUS extension, which broke delete/move entirely on rare self-hosted/legacy IMAP servers (#319). expungeTargeted now probes UIDPLUS from the folder's own already-open protocol (via IMAPFolder.doCommand, so it never opens a second connection + LOGIN and keeps the one-connection-per-batch invariant of #125/#295). With UIDPLUS it still uses the targeted UID EXPUNGE. Without it, it falls back to a plain, untargeted EXPUNGE only when provably safe: the messages we just flagged are the only \Deleted ones in the folder. When unrelated \Deleted mail is present a plain EXPUNGE would destroy it, and there is no UIDPLUS-free way to expunge a single UID, so we refuse and fail loud, preserving the #295 "never touch unrelated \Deleted mail" invariant. PII-free AppLog breadcrumbs record the fallback decision. Covered by GreenMail unit tests for both branches (with UIDPLUS via the default probe; without via an injected capability seam, since GreenMail always advertises UIDPLUS). Closes #319 --- .../kotlin/org/libremail/mail/ImapClient.kt | 89 +++++++++++++++++-- .../org/libremail/mail/ImapClientTest.kt | 71 +++++++++++++++ 2 files changed, 151 insertions(+), 9 deletions(-) diff --git a/app/src/main/kotlin/org/libremail/mail/ImapClient.kt b/app/src/main/kotlin/org/libremail/mail/ImapClient.kt index 2ab1570..4a49550 100644 --- a/app/src/main/kotlin/org/libremail/mail/ImapClient.kt +++ b/app/src/main/kotlin/org/libremail/mail/ImapClient.kt @@ -5,6 +5,7 @@ import jakarta.mail.FetchProfile import jakarta.mail.Flags import jakarta.mail.Folder import jakarta.mail.Message +import jakarta.mail.MessagingException import jakarta.mail.Multipart import jakarta.mail.Part import jakarta.mail.Session @@ -103,6 +104,16 @@ data class ReplyContext( class ImapClient internal constructor( private val reuseConnections: Boolean, private val reuseIdleTimeoutMillis: Long = DEFAULT_REUSE_IDLE_TIMEOUT_MS, + /** + * Reports whether the folder's already-open IMAP connection advertises the UIDPLUS extension (RFC + * 4315), which gates the targeted `UID EXPUNGE` vs. the plain-EXPUNGE fallback in [expungeTargeted] + * (issue #319). Reads the capability from the folder's own protocol so it never opens a second + * connection + LOGIN mid-operation — preserving the one-connection-per-batch invariant of issues + * #125/#295. Defaults to the real probe ([probeUidPlusCapability]); the internal constructor lets a + * test inject a fixed value, because GreenMail always advertises UIDPLUS and so cannot exercise the + * no-UIDPLUS fallback path on its own. + */ + private val supportsUidPlus: (IMAPFolder) -> Boolean = ::probeUidPlusCapability, ) { /** @@ -432,17 +443,61 @@ class ImapClient internal constructor( } /** - * Permanently removes exactly [messages] — which the caller has already flagged `\Deleted` — with a - * **targeted** UID EXPUNGE (RFC 4315, [IMAPFolder.expunge]). Deliberately never the untargeted - * [Folder.expunge], which expunges *every* `\Deleted`-flagged message in [mailbox] — including ones - * a second client, Gmail, or a partial earlier move left flagged — the data-loss bug in issue #295. - * Every folder this client opens is an [IMAPFolder], so the cast holds in production and under - * GreenMail. The server must advertise UIDPLUS (Gmail, Outlook, and GreenMail do); on one that does - * not, Angus raises "UID EXPUNGE not supported" here rather than silently falling back to the - * unrelated-mail-destroying untargeted expunge — a loud failure is the correct, safe outcome. + * Permanently removes exactly [messages] — which the caller has already flagged `\Deleted` — using a + * **targeted** UID EXPUNGE (RFC 4315, [IMAPFolder.expunge]) whenever the server advertises UIDPLUS + * (Gmail, Outlook, and GreenMail do). That never touches other `\Deleted`-flagged mail in [mailbox] + * — ones a second client, Gmail, or a partial earlier move left flagged — the data-loss bug in + * issue #295. + * + * On a server **without** UIDPLUS the targeted UID EXPUNGE throws `UID EXPUNGE not supported`, which + * used to break delete/move entirely (issue #319). There we fall back to a plain, untargeted + * [Folder.expunge] — but **only** when it is provably safe, i.e. the messages we just flagged are the + * *only* `\Deleted` ones in [mailbox] ([countForeignDeletedMessages] is 0). A plain EXPUNGE removes + * **every** `\Deleted` message, so when unrelated `\Deleted` mail is present we refuse and fail loud + * rather than destroy mail the user never selected: there is no UIDPLUS-free way to expunge one + * specific UID while sparing the others (that capability is exactly what UIDPLUS provides), so + * preserving the issue-#295 invariant — never touch unrelated `\Deleted` mail — wins over completing + * the delete. Every folder this client opens is an [IMAPFolder], so the cast holds in production and + * under GreenMail. */ private fun expungeTargeted(mailbox: Folder, messages: Array) { - (mailbox as IMAPFolder).expunge(messages) + val imapFolder = mailbox as IMAPFolder + if (supportsUidPlus(imapFolder)) { + imapFolder.expunge(messages) + return + } + val foreignDeleted = countForeignDeletedMessages(imapFolder, messages) + if (foreignDeleted == 0) { + AppLog.i(EXPUNGE_TAG, "server lacks UIDPLUS; no other \\Deleted mail present, using safe plain EXPUNGE") + imapFolder.expunge() + } else { + AppLog.w( + EXPUNGE_TAG, + "server lacks UIDPLUS and $foreignDeleted other \\Deleted message(s) present; " + + "refusing plain EXPUNGE so unrelated mail is not destroyed", + ) + throw MessagingException( + "Cannot honor a targeted expunge: the server does not support UIDPLUS and other deleted " + + "messages are present, so a plain EXPUNGE would remove mail that was not selected", + ) + } + } + + /** + * Counts messages in [mailbox] flagged `\Deleted` that are **not** among [targets] (the ones the + * caller just flagged). Used only on the no-UIDPLUS fallback in [expungeTargeted] to decide whether a + * plain EXPUNGE is safe — it is safe exactly when this is 0. Fetches FLAGS + UID for the whole folder, + * acceptable on this rare legacy-server path. + */ + private fun countForeignDeletedMessages(mailbox: IMAPFolder, targets: Array): Int { + val targetUids = targets.mapTo(HashSet()) { mailbox.getUID(it) } + val all = mailbox.messages + val profile = FetchProfile().apply { + add(FetchProfile.Item.FLAGS) + add(UIDFolder.FetchProfileItem.UID) + } + mailbox.fetch(all, profile) + return all.count { it.isSet(Flags.Flag.DELETED) && mailbox.getUID(it) !in targetUids } } /** @@ -719,6 +774,7 @@ class ImapClient internal constructor( const val TIMEOUT_MS = "15000" const val TAG = "LibreMailIdle" const val PERF_TAG = "ImapPerf" + const val EXPUNGE_TAG = "ImapExpunge" const val NANOS_PER_MS = 1_000_000L // A reused connection unused for this long is idle-evicted (issue #357 Part 2): long enough to @@ -728,6 +784,21 @@ class ImapClient internal constructor( } } +/** The IMAP UIDPLUS extension name (RFC 4315); its presence is what makes a targeted `UID EXPUNGE` legal. */ +private const val CAP_UIDPLUS = "UIDPLUS" + +/** + * Whether [folder]'s already-open connection advertises the IMAP UIDPLUS extension (RFC 4315) — the + * capability a targeted `UID EXPUNGE` requires. Reads it from the folder's own protocol via + * [IMAPFolder.doCommand] (the capabilities parsed at LOGIN; no extra round trip), reusing the open + * connection rather than borrowing a fresh store protocol — which would open a second connection + LOGIN + * mid-batch and defeat the reuse invariant of issue #125. Any read failure degrades to `false`, so the + * caller takes the safe plain-EXPUNGE fallback. The production default of [ImapClient.supportsUidPlus]. + */ +private fun probeUidPlusCapability(folder: IMAPFolder): Boolean = runCatching { + folder.doCommand { protocol -> protocol.hasCapability(CAP_UIDPLUS) } as? Boolean +}.getOrNull() ?: 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/test/kotlin/org/libremail/mail/ImapClientTest.kt b/app/src/test/kotlin/org/libremail/mail/ImapClientTest.kt index 5c23358..113be5f 100644 --- a/app/src/test/kotlin/org/libremail/mail/ImapClientTest.kt +++ b/app/src/test/kotlin/org/libremail/mail/ImapClientTest.kt @@ -364,6 +364,77 @@ class ImapClientTest { assertTrue(archive.contains("Move me"), "archive=$archive") } + // --- issue #319: graceful fallback when the server lacks the UIDPLUS extension --- + // + // GreenMail always advertises UIDPLUS, so the "with UIDPLUS" case (a targeted UID EXPUNGE that spares + // unrelated \Deleted mail) is exercised by the default-client delete/move tests above. To drive the + // "without UIDPLUS" branch against the same real server, these tests inject a capability probe that + // reports no UIDPLUS while every EXPUNGE below still runs for real against GreenMail. + + private val noUidPlusClient = ImapClient(reuseConnections = false, supportsUidPlus = { false }) + + @Test + fun `deleteMessages falls back to a safe plain expunge when the server lacks UIDPLUS`() = runTest { + val buffer = RingLogBuffer() + AppLog.install(buffer) + GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Target", "delete this") + GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Keep", "no flag at all") + greenMail.waitForIncomingEmail(2) + val bySubject = client.fetchRecent(params(), "INBOX", limit = 50).associateBy { it.subject } + + // No other \Deleted mail is present, so a plain EXPUNGE removes only the target — and must not throw. + noUidPlusClient.deleteMessages(params(), "INBOX", listOf(bySubject.getValue("Target").uid)) + + val remaining = client.fetchRecent(params(), "INBOX", limit = 50).map { it.subject } + assertFalse(remaining.contains("Target"), "the target must be expunged via the fallback, remaining=$remaining") + assertTrue(remaining.contains("Keep"), "an unflagged message must survive, remaining=$remaining") + assertTrue( + buffer.snapshot().any { it.message.contains("using safe plain EXPUNGE") }, + "the no-UIDPLUS fallback decision must be logged, messages=${buffer.snapshot().map { it.message }}", + ) + } + + @Test + fun `deleteMessages without UIDPLUS refuses to expunge when other Deleted mail is present`() = runTest { + val buffer = RingLogBuffer() + AppLog.install(buffer) + GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Target", "delete this") + GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Bystander", "flagged elsewhere") + greenMail.waitForIncomingEmail(2) + val bySubject = client.fetchRecent(params(), "INBOX", limit = 50).associateBy { it.subject } + // A second client left "Bystander" flagged \Deleted but un-expunged; a plain EXPUNGE would drop it. + client.setFlag(params(), "INBOX", bySubject.getValue("Bystander").uid, Flags.Flag.DELETED, value = true) + + // Without UIDPLUS there is no way to expunge only "Target" while sparing "Bystander": refuse loudly + // rather than destroy mail the user never selected (preserves the issue #295 invariant). + assertFailsWith { + noUidPlusClient.deleteMessages(params(), "INBOX", listOf(bySubject.getValue("Target").uid)) + } + + val remaining = client.fetchRecent(params(), "INBOX", limit = 50).map { it.subject } + assertTrue(remaining.contains("Target"), "the refused target must NOT be expunged, remaining=$remaining") + assertTrue(remaining.contains("Bystander"), "unrelated \\Deleted mail must survive, remaining=$remaining") + assertTrue( + buffer.snapshot().any { it.message.contains("refusing plain EXPUNGE") }, + "the refusal must be logged, messages=${buffer.snapshot().map { it.message }}", + ) + } + + @Test + fun `moveMessages falls back to a safe plain expunge when the server lacks UIDPLUS`() = runTest { + GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Move me", "relocate this") + greenMail.waitForIncomingEmail(1) + appendMessage("Archive", "carol@example.org", "Seed", "Creates the Archive folder") + val uid = client.fetchRecent(params(), "INBOX", limit = 50).first { it.subject == "Move me" }.uid + + noUidPlusClient.moveMessages(params(), "INBOX", listOf(uid), "Archive") + + val inbox = client.fetchRecent(params(), "INBOX", limit = 50).map { it.subject } + val archive = client.fetchRecent(params(), "Archive", limit = 50).map { it.subject } + assertFalse(inbox.contains("Move me"), "the moved message must leave the source via the fallback, inbox=$inbox") + assertTrue(archive.contains("Move me"), "archive=$archive") + } + @Test fun `fetchRecent returns empty for an empty folder`() = runTest { createFolder("Empty") -- 2.47.3 From 8632500cf6a1432daf42bbe66b87318227b29c3b Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Tue, 7 Jul 2026 23:42:58 -0500 Subject: [PATCH 3/5] perf(data): route MailBackfiller.persistBatch through batched updateHeaderContents MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit persistBatch refreshed each pre-existing backfilled header with a per-row updateHeaderContent in its own implicit transaction — the same N-commits-per-page anti-pattern #310 fixed in MailSyncer. Route the whole pre-existing subset through the batched MessageDao.updateHeaderContents(List) @Transaction so a page costs one commit instead of one fsync per message (amplified on the encrypted cache). Semantics are unchanged: updateHeaderContents applies updateHeaderContent to each row in list order, so the same rows get the same values (and the same casefold columns); the isNotEmpty guard still skips an empty refresh batch; brand-new rows stay insert-only. No schema change. Adds a PII-free, counts-only AppLog breadcrumb at the persist point. Tests: - MailBackfillerTest: the refresh routes through the batched update and never the per-row one; a partial page refreshes only its pre-existing subset in one batched call; an all-new page skips the batch entirely (empty boundary); breadcrumb counts. - MessageDaoTest (real Room): the batch writes byte-for-byte the same row as the per-row path; an empty batch is a no-op. Closes #322 --- .../libremail/data/local/MessageDaoTest.kt | 56 ++++++++++++ .../org/libremail/data/sync/MailBackfiller.kt | 18 ++-- .../libremail/data/sync/MailBackfillerTest.kt | 85 ++++++++++++++++++- 3 files changed, 146 insertions(+), 13 deletions(-) diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoTest.kt index fdb9956..35d0169 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoTest.kt @@ -320,6 +320,62 @@ class MessageDaoTest { assertEquals("cached-2", two.body) } + /** + * The batched [MessageDao.updateHeaderContents] must write byte-for-byte the same row as the per-row + * [MessageDao.updateHeaderContent] it replaces on the backfill path (issue #322): the same refreshed + * fields and casefold columns, and the same untouched flags/body/membership. Two rows seeded + * identically and refreshed by the two paths with the same values must end up identical. + */ + @Test + fun updateHeaderContentsWritesTheSameResultAsThePerRowUpdate() = runBlocking { + dao.insertNew( + listOf( + message("perRow", isStarred = true, body = "cached"), + message("batch", isStarred = true, body = "cached"), + ), + ) + + // Old path: the single-row update. New path: the batch, carrying identical field values. + dao.updateHeaderContent( + id = "perRow", + sender = "Refreshed", + senderEmail = "refreshed@example.org", + subject = "Fresh", + timestampMillis = 9_000L, + uid = 7L, + ) + dao.updateHeaderContents( + listOf( + message( + "batch", + sender = "Refreshed", + senderEmail = "refreshed@example.org", + subject = "Fresh", + timestampMillis = 9_000L, + uid = 7L, + ), + ), + ) + + // Every stored column — refreshed and preserved alike — matches between the two paths. + val perRow = requireNotNull(dao.getById("perRow")) + val batch = requireNotNull(dao.getById("batch")) + assertEquals(perRow.copy(id = "id"), batch.copy(id = "id")) + } + + /** An empty batch is a no-op — issue #322's empty-batch boundary at the DAO transaction level. */ + @Test + fun updateHeaderContentsWithAnEmptyBatchWritesNothing() = runBlocking { + dao.insertNew(listOf(message("acct:1", subject = "Original", isRead = true, uid = 5))) + + dao.updateHeaderContents(emptyList()) + + val row = requireNotNull(dao.getById("acct:1")) + assertEquals("Original", row.subject) + assertEquals(5L, row.uid) + assertTrue(row.isRead) + } + @Test fun markSyncedPromotesSearchOnlyRowsIntoTheFolder() = runBlocking { dao.insertNew( 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 d0f12b2..48b2a90 100644 --- a/app/src/main/kotlin/org/libremail/data/sync/MailBackfiller.kt +++ b/app/src/main/kotlin/org/libremail/data/sync/MailBackfiller.kt @@ -198,17 +198,15 @@ class MailBackfiller @Inject constructor( val toRefresh = entities.filter { it.id in preexisting } if (toRefresh.isNotEmpty()) { messageDao.markSynced(toRefresh.map { it.id }) - toRefresh.forEach { - messageDao.updateHeaderContent( - id = it.id, - sender = it.sender, - senderEmail = it.senderEmail, - subject = it.subject, - timestampMillis = it.timestampMillis, - uid = it.uid, - ) - } + // Refresh every pre-existing row's header in ONE transaction (issue #322) rather than a per-row + // UPDATE each in its own implicit transaction — the same batched path foreground sync uses + // (issue #310). N per-row commits fsync the journal once per message (amplified on the + // encrypted cache); routing the whole batch through updateHeaderContents collapses them into a + // single commit per page. Semantically identical: it applies updateHeaderContent to each row in + // list order, so the same rows get the same values (and the same casefold columns). + messageDao.updateHeaderContents(toRefresh) } + AppLog.d(TAG, "backfill persist: fetched=${entities.size} refreshed=${toRefresh.size}") } private suspend fun markComplete(accountId: String, folder: String, nextBeforeUid: Long) { 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 0c1ef44..670080d 100644 --- a/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt +++ b/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt @@ -378,10 +378,12 @@ class MailBackfillerTest { /** * A backfilled page whose ids already exist (e.g. former search-only rows) must be *refreshed* * (markSynced + header update), not just IGNORE-inserted — this covers persistBatch's - * pre-existing-row branch, which the all-brand-new happy paths above never hit. + * pre-existing-row branch, which the all-brand-new happy paths above never hit. The refresh is + * routed through the BATCHED [MessageDao.updateHeaderContents] (issue #322), never the per-row + * [MessageDao.updateHeaderContent]. */ @Test - fun `re-inserting a pre-existing header refreshes it rather than only inserting`() = runTest { + fun `re-inserting a pre-existing header refreshes it through the batched update`() = runTest { appendMessages(60) seedForegroundWindow() val backfiller = backfiller(AccountSettings("acct")) @@ -391,11 +393,88 @@ class MailBackfillerTest { backfiller.runBackfill() coVerify(atLeast = 1) { lastMessageDao!!.markSynced(any()) } - coVerify(atLeast = 1) { + // The whole batch is refreshed in one transaction — the per-row path is never taken (issue #322). + coVerify(atLeast = 1) { lastMessageDao!!.updateHeaderContents(any()) } + coVerify(exactly = 0) { lastMessageDao!!.updateHeaderContent(any(), any(), any(), any(), any(), any()) } } + // --- issue #322: batched persist ------------------------------------------------------------ + + /** + * persistBatch routes its pre-existing-row refreshes through the BATCHED + * [MessageDao.updateHeaderContents] — one transaction per page (issue #322) — and refreshes ONLY + * the rows that already existed: brand-new rows are written whole by insertNew, so re-updating them + * would be redundant. A partial page (some ids already present, some brand-new) exercises exactly + * that split, in one batched call, never the per-row [MessageDao.updateHeaderContent]. + */ + @Test + fun `a partial page refreshes only its pre-existing rows through one batched update`() = runTest { + // One cached window row makes INBOX a backfill target with boundary UID 20. + cached += fetchedMessage(uid = "20").toEntity("acct", "INBOX") + // A single older page (UIDs 10..19); the next fetch (boundary 10) returns empty → folder complete. + val page = (10..19).map { fetchedMessage(uid = it.toString()) } + val imapClient = mockk() + coEvery { imapClient.fetchOlderThan(any(), any(), any(), any()) } answers { + if (thirdArg() > 10L) page else emptyList() + } + val backfiller = backfiller(AccountSettings("acct"), imapClient = imapClient) + // Only UIDs 10..13 are reported as already present — the partial pre-existing subset. + val preexistingIds = (10..13).map { "acct:INBOX:$it" }.toSet() + coEvery { lastMessageDao!!.existingIds(any()) } answers { + firstArg>().filter { it in preexistingIds } + } + val refreshedBatches = mutableListOf>() + coEvery { lastMessageDao!!.updateHeaderContents(any()) } answers { + refreshedBatches += firstArg>() + } + + backfiller.runBackfill() + + // Exactly one batched call for the page, carrying exactly the pre-existing subset (not the 6 new rows). + assertEquals(1, refreshedBatches.size, "one batched update per page") + assertEquals(preexistingIds, refreshedBatches.single().mapTo(HashSet()) { it.id }) + // markSynced promotes exactly that subset; the per-row update path is never taken (issue #322). + coVerify(exactly = 1) { lastMessageDao!!.markSynced(match { it.toSet() == preexistingIds }) } + coVerify(exactly = 0) { + lastMessageDao!!.updateHeaderContent(any(), any(), any(), any(), any(), any()) + } + // Counts-only persist breadcrumb (PII-free): 10 fetched, 4 refreshed. + assertTrue( + logBuffer.snapshot().any { it.message == "backfill persist: fetched=10 refreshed=4" }, + "the persist breadcrumb logs page + refresh counts only", + ) + } + + /** + * The empty-refresh boundary: a page whose rows are ALL brand-new must skip the header-refresh + * transaction entirely — insertNew writes them whole, so neither markSynced nor the batched + * updateHeaderContents runs (issue #322 preserves persistBatch's isNotEmpty guard). + */ + @Test + fun `an all-new page skips the batched header update entirely`() = runTest { + cached += fetchedMessage(uid = "20").toEntity("acct", "INBOX") + val page = (10..19).map { fetchedMessage(uid = it.toString()) } + val imapClient = mockk() + coEvery { imapClient.fetchOlderThan(any(), any(), any(), any()) } answers { + if (thirdArg() > 10L) page else emptyList() + } + // existingIds stays at the relaxed default (empty) → every fetched row is brand-new. + backfiller(AccountSettings("acct"), imapClient = imapClient).runBackfill() + + coVerify(exactly = 0) { lastMessageDao!!.updateHeaderContents(any()) } + coVerify(exactly = 0) { lastMessageDao!!.markSynced(any()) } + coVerify(exactly = 0) { + lastMessageDao!!.updateHeaderContent(any(), any(), any(), any(), any(), any()) + } + // The persist breadcrumb still records the page, with zero refreshed. + assertTrue( + logBuffer.snapshot().any { it.message == "backfill persist: fetched=10 refreshed=0" }, + "an all-new page logs refreshed=0", + ) + } + // --- issue #329: AppLog breadcrumbs --------------------------------------------------------- @Test -- 2.47.3 From 60f822a63c7138295517c0ea9c82a2d1076e8846 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Tue, 7 Jul 2026 23:54:57 -0500 Subject: [PATCH 4/5] feat(auth): detect "IMAP disabled" AUTHENTICATE failures and prompt to enable IMAP When account setup obtains a valid credential but the IMAP AUTHENTICATE step is rejected because IMAP access is switched off for the mailbox, surface an actionable "turn on IMAP" dialog (with the provider's enable-IMAP help link) instead of the opaque generic auth error (#390). - ImapAuthError.isImapDisabled classifies the failure on two signals: explicit provider "IMAP is disabled/not enabled" server text (e.g. Gmail's "not enabled for IMAP use"), and -- for the Outlook XOAUTH2 path -- a valid-token AUTHENTICATE rejection, which outlook.office365.com reports only as a generic "AUTHENTICATE failed". Ordinary wrong-password / expired-token / network errors are deliberately not matched, so they keep the generic error. - imapDisabledPromptFor resolves a provider-aware prompt (brand via MailProvider.brandFor; Outlook + Gmail enable-IMAP help URLs, generic otherwise). - Shared ImapDisabledDialog reused by the Outlook picker, the app-password form, and manual setup -- the three points where the auth failure surfaces. The reactive complement to the pre-auth Outlook notice (#411/#426). - PII-free AppLog breadcrumbs at the classification/prompt points (accountLogRef only; never the email/host/token). Tests: ImapAuthErrorTest (provider text + OAuth inference + a real GreenMail wrong-password negative), ImapDisabledPromptTest (brand/URL resolution), ImapDisabledDialogJvmTest (Robolectric), per-view-model + per-screen wiring tests, and an instrumented AppPasswordSetupScreenTest case driving the failure end to end. Closes #390 --- .../AppPasswordSetupScreenTest.kt | 42 ++++ .../org/libremail/mail/ImapAuthError.kt | 77 +++++++ .../ui/accountsetup/AccountPickerScreen.kt | 6 + .../ui/accountsetup/AccountSetupViewModel.kt | 51 +++-- .../ui/accountsetup/AppPasswordSetupScreen.kt | 5 + .../ui/accountsetup/AppPasswordViewModel.kt | 32 ++- .../ui/accountsetup/ImapDisabledDialog.kt | 56 +++++ .../ui/accountsetup/ImapDisabledPrompt.kt | 52 +++++ .../ui/accountsetup/ManualSetupScreen.kt | 5 + .../ui/accountsetup/ManualSetupViewModel.kt | 37 +++- app/src/main/res/values/strings.xml | 8 + .../org/libremail/mail/ImapAuthErrorTest.kt | 194 ++++++++++++++++++ .../AccountPickerScreenJvmTest.kt | 13 ++ .../accountsetup/AccountSetupViewModelTest.kt | 49 +++++ .../AppPasswordSetupScreenJvmTest.kt | 17 ++ .../accountsetup/AppPasswordViewModelTest.kt | 43 +++- .../accountsetup/ImapDisabledDialogJvmTest.kt | 105 ++++++++++ .../ui/accountsetup/ImapDisabledPromptTest.kt | 113 ++++++++++ .../accountsetup/ManualSetupScreenJvmTest.kt | 14 ++ .../accountsetup/ManualSetupViewModelTest.kt | 42 +++- 20 files changed, 931 insertions(+), 30 deletions(-) create mode 100644 app/src/main/kotlin/org/libremail/mail/ImapAuthError.kt create mode 100644 app/src/main/kotlin/org/libremail/ui/accountsetup/ImapDisabledDialog.kt create mode 100644 app/src/main/kotlin/org/libremail/ui/accountsetup/ImapDisabledPrompt.kt create mode 100644 app/src/test/kotlin/org/libremail/mail/ImapAuthErrorTest.kt create mode 100644 app/src/test/kotlin/org/libremail/ui/accountsetup/ImapDisabledDialogJvmTest.kt create mode 100644 app/src/test/kotlin/org/libremail/ui/accountsetup/ImapDisabledPromptTest.kt diff --git a/app/src/androidTest/kotlin/org/libremail/ui/accountsetup/AppPasswordSetupScreenTest.kt b/app/src/androidTest/kotlin/org/libremail/ui/accountsetup/AppPasswordSetupScreenTest.kt index 4118efd..42a53fc 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/accountsetup/AppPasswordSetupScreenTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/accountsetup/AppPasswordSetupScreenTest.kt @@ -7,6 +7,7 @@ import android.content.Intent import androidx.activity.ComponentActivity import androidx.compose.ui.test.assertIsDisplayed import androidx.compose.ui.test.junit4.createAndroidComposeRule +import androidx.compose.ui.test.onAllNodesWithText import androidx.compose.ui.test.onNodeWithText import androidx.compose.ui.test.performClick import androidx.compose.ui.test.performScrollTo @@ -15,8 +16,11 @@ import androidx.lifecycle.SavedStateHandle import androidx.test.espresso.intent.Intents import androidx.test.espresso.intent.matcher.IntentMatchers.hasAction import androidx.test.espresso.intent.matcher.IntentMatchers.hasData +import androidx.test.espresso.intent.matcher.UriMatchers.hasHost import androidx.test.ext.junit.runners.AndroidJUnit4 +import jakarta.mail.AuthenticationFailedException import org.hamcrest.CoreMatchers.allOf +import org.hamcrest.CoreMatchers.equalTo import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith @@ -108,4 +112,42 @@ class AppPasswordSetupScreenTest { Intents.release() } } + + /** + * When the connection test fails specifically because IMAP is disabled (Gmail's "not enabled for + * IMAP use"), the screen surfaces the actionable "turn on IMAP" dialog instead of a generic error, + * and its help link opens the provider's enable-IMAP page (#390). Driving the failure through a + * [FakeAccountRepository] exercises the real classification + dialog wiring end to end on device. + */ + @Test + fun imapDisabledFailure_showsThePrompt_andHelpLinkOpensTheProviderPage() { + setContent( + repository = FakeAccountRepository( + result = Result.failure( + AuthenticationFailedException("Your account is not enabled for IMAP use"), + ), + ), + ) + + composeTestRule.onNodeWithText(string(R.string.app_password_email)).performTextInput("me@gmail.com") + composeTestRule.onNodeWithText(string(R.string.app_password_field)).performTextInput("app-pw-1234") + composeTestRule.onNodeWithText(string(R.string.app_password_test_and_add)).performScrollTo().performClick() + + composeTestRule.waitUntil(5_000) { + composeTestRule.onAllNodesWithText(string(R.string.imap_disabled_title)).fetchSemanticsNodes().isNotEmpty() + } + composeTestRule.onNodeWithText(string(R.string.imap_disabled_message, provider.displayName)).assertIsDisplayed() + + Intents.init() + try { + Intents.intending(hasAction(Intent.ACTION_VIEW)) + .respondWith(Instrumentation.ActivityResult(Activity.RESULT_CANCELED, null)) + + composeTestRule.onNodeWithText(string(R.string.imap_disabled_help)).performClick() + + Intents.intended(allOf(hasAction(Intent.ACTION_VIEW), hasData(hasHost(equalTo("support.google.com"))))) + } finally { + Intents.release() + } + } } diff --git a/app/src/main/kotlin/org/libremail/mail/ImapAuthError.kt b/app/src/main/kotlin/org/libremail/mail/ImapAuthError.kt new file mode 100644 index 0000000..7c820b5 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/mail/ImapAuthError.kt @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.mail + +import jakarta.mail.AuthenticationFailedException + +/** + * Classifies an IMAP authentication failure that surfaced from [ImapClient.openConnectedStore]'s + * `store.connect` (i.e. an `AUTHENTICATE`/`LOGIN` rejection) into the one distinction the account-setup + * UI cares about: **"IMAP access is switched off for this account"** vs any other auth failure (a wrong + * password, an expired/invalid token, a network error, …). Only the former can be fixed by the user + * flipping a provider-side toggle, so only it earns the actionable "turn on IMAP" prompt (issue #390); + * everything else keeps the existing generic error. + * + * The heuristic is deliberately conservative — misclassifying a wrong password as "IMAP disabled" would + * send the user down a dead end — so it fires on only two well-motivated signals (see [isImapDisabled]). + */ +object ImapAuthError { + + /** + * True when [error] (or anything in its cause chain) indicates the account's IMAP access is + * **disabled/not enabled**, rather than a wrong credential or other failure. Two signals, checked + * in order: + * + * 1. **Explicit server text.** The failure message names IMAP as disabled/not-enabled/turned-off — + * e.g. Gmail's `Your account is not enabled for IMAP use`, or a `IMAP access is disabled` + * variant. This is provider-independent and works regardless of [usedOAuth]. + * 2. **OAuth inference.** When the connection authenticated with a **freshly obtained XOAUTH2 + * token** ([usedOAuth] true) and the server still raised an [AuthenticationFailedException], the + * token itself was accepted at consent/exchange time, so an `AUTHENTICATE` rejection here almost + * always means IMAP is off for the mailbox — not a bad token. This is the Outlook case that + * motivated #390: `outlook.office365.com` returns only a generic `AUTHENTICATE failed` with no + * distinctive text, so the text check in (1) cannot catch it. + * + * A password/app-password failure ([usedOAuth] false) with no IMAP-disabled text — the ordinary + * wrong-password case — is deliberately **not** matched, so it is never misclassified. + */ + fun isImapDisabled(error: Throwable, usedOAuth: Boolean): Boolean { + val chain = causeChain(error) + if (chain.any { it.message?.let(::mentionsImapDisabled) == true }) return true + return usedOAuth && chain.any { it is AuthenticationFailedException } + } + + /** + * The exception and its transitive causes, in order, guarding against a self-referential or cyclic + * cause chain (identity-based visited check — [Throwable] does not override `equals`). + */ + private fun causeChain(error: Throwable): List { + val seen = mutableListOf() + var current: Throwable? = error + while (current != null && seen.none { it === current }) { + seen.add(current) + current = current.cause + } + return seen + } + + /** True when [message] names IMAP as disabled/not-enabled, in any of the shapes providers use. */ + private fun mentionsImapDisabled(message: String): Boolean { + val text = message.lowercase() + return DISABLED_PATTERNS.any { it.containsMatchIn(text) } + } + + /** + * Case-insensitive shapes of "IMAP is off" seen across providers (patterns run against a lowercased + * message). Kept broad enough to catch wording variants, but each anchors on both "imap" and an + * explicit off/disabled/not-enabled word so an ordinary "AUTHENTICATE failed" / "Invalid + * credentials" wrong-password message never matches. + */ + private val DISABLED_PATTERNS = listOf( + // "IMAP access is disabled", "IMAP is disabled", "IMAP access is not enabled", "IMAP ... turned off". + Regex("""imap[^\n]{0,40}?(disabled|not enabled|turned off|is off)"""), + // Reversed order, e.g. Gmail's "Your account is not enabled for IMAP use". + Regex("""(disabled|not enabled|turned off)[^\n]{0,20}?for imap"""), + // "please enable IMAP", Gmail's "enable your account for IMAP access". + Regex("""enable[^\n]{0,30}?imap"""), + ) +} diff --git a/app/src/main/kotlin/org/libremail/ui/accountsetup/AccountPickerScreen.kt b/app/src/main/kotlin/org/libremail/ui/accountsetup/AccountPickerScreen.kt index ce3dd1c..7e0d0f9 100644 --- a/app/src/main/kotlin/org/libremail/ui/accountsetup/AccountPickerScreen.kt +++ b/app/src/main/kotlin/org/libremail/ui/accountsetup/AccountPickerScreen.kt @@ -160,6 +160,12 @@ fun AccountPickerScreen( CircularProgressIndicator() } } + // Outlook OAuth can succeed while the IMAP AUTHENTICATE step is rejected because IMAP is + // off for the mailbox (#390); show the actionable "turn on IMAP" prompt instead of a + // generic auth-failure snackbar. + state.imapDisabledPrompt?.let { prompt -> + ImapDisabledDialog(prompt = prompt, onDismiss = viewModel::dismissImapDisabledPrompt) + } } } } diff --git a/app/src/main/kotlin/org/libremail/ui/accountsetup/AccountSetupViewModel.kt b/app/src/main/kotlin/org/libremail/ui/accountsetup/AccountSetupViewModel.kt index 1c975fc..48e63f8 100644 --- a/app/src/main/kotlin/org/libremail/ui/accountsetup/AccountSetupViewModel.kt +++ b/app/src/main/kotlin/org/libremail/ui/accountsetup/AccountSetupViewModel.kt @@ -15,6 +15,7 @@ import org.libremail.auth.OutlookAuthManager import org.libremail.domain.model.Account import org.libremail.domain.repository.AccountRepository import org.libremail.reporting.AppLog +import org.libremail.reporting.accountLogRef import javax.inject.Inject /** Stage of an account-setup attempt, shared by the Outlook and manual flows. */ @@ -25,6 +26,11 @@ data class AccountSetupUiState( val error: String? = null, /** Set alongside [SetupStatus.DONE]: the id of the account that was just added. */ val addedAccountId: String? = null, + /** + * Set instead of [error] when the failure was specifically an "IMAP is disabled" rejection (#390): + * the screen shows the actionable [ImapDisabledDialog] rather than the generic error snackbar. + */ + val imapDisabledPrompt: ImapDisabledPrompt? = null, ) @HiltViewModel @@ -62,34 +68,53 @@ class AccountSetupViewModel @Inject constructor( } viewModelScope.launch { _state.update { it.copy(status = SetupStatus.CONNECTING, error = null) } + // Captured so the failure branch can tell a token/consent failure (account still null) from + // a token-OK-but-IMAP-AUTHENTICATE-rejected one (account set), which is the #390 signal. + var account: Account? = null runCatching { val oauth = outlookAuthManager.exchangeToken(data) + val acct = Account.outlook(oauth.email) + account = acct accountRepository.addOutlookAccount(oauth.email, oauth.accessToken, oauth.authStateJson).getOrThrow() - Account.outlook(oauth.email).id + acct.id }.fold( onSuccess = { accountId -> // No email: the account id embeds it (see accountLogRef) and must never be logged. AppLog.i(TAG, "Outlook account added") _state.update { it.copy(status = SetupStatus.DONE, addedAccountId = accountId) } }, - onFailure = { e -> - // AppLog.d's Logcat mirror is stripped from release builds by the -assumenosideeffects - // Log.d ProGuard rule (keeps any account address / token detail out of shipped - // logcat), but that rule only elides the `Log.d(...)` call inside AppLog.d — the - // buffer.record(...) line right after it is untouched, so this breadcrumb still - // reaches a submitted report. The throwable's message may carry the account - // email/token; AppLog's StackTraceScrubber redacts it before it is recorded. - AppLog.d(TAG, "Outlook sign-in failed after redirect", e) - _state.update { - it.copy(status = SetupStatus.IDLE, error = e.message ?: "Microsoft sign-in failed") - } - }, + onFailure = { e -> onOutlookFailure(e, account) }, ) } } + /** + * Routes a failed Outlook add. When the OAuth token was obtained ([account] set) but the IMAP + * `AUTHENTICATE` step was rejected because IMAP is disabled, surfaces the actionable "turn on IMAP" + * prompt (#390); otherwise (token/consent failure, or any other error) keeps the generic message. + */ + private fun onOutlookFailure(e: Throwable, account: Account?) { + // AppLog.d's Logcat mirror is stripped from release builds by the -assumenosideeffects + // Log.d ProGuard rule (keeps any account address / token detail out of shipped logcat), but + // that rule only elides the `Log.d(...)` call inside AppLog.d — the buffer.record(...) line + // right after it is untouched, so this breadcrumb still reaches a submitted report. The + // throwable's message may carry the account email/token; AppLog's StackTraceScrubber redacts + // it before it is recorded. + AppLog.d(TAG, "Outlook sign-in failed after redirect", e) + val prompt = account?.let { imapDisabledPromptFor(e, it, usedOAuth = true) } + if (prompt != null && account != null) { + AppLog.i(TAG, "IMAP disabled on Outlook sign-in (${accountLogRef(account.id)}); prompting to enable IMAP") + _state.update { it.copy(status = SetupStatus.IDLE, imapDisabledPrompt = prompt, error = null) } + } else { + _state.update { it.copy(status = SetupStatus.IDLE, error = e.message ?: "Microsoft sign-in failed") } + } + } + fun consumeError() = _state.update { it.copy(error = null) } + /** Clears the "IMAP is disabled" prompt after the user acknowledges it (issue #390). */ + fun dismissImapDisabledPrompt() = _state.update { it.copy(imapDisabledPrompt = null) } + private companion object { const val TAG = "AccountSetupVM" } diff --git a/app/src/main/kotlin/org/libremail/ui/accountsetup/AppPasswordSetupScreen.kt b/app/src/main/kotlin/org/libremail/ui/accountsetup/AppPasswordSetupScreen.kt index 82ec211..c36cbbc 100644 --- a/app/src/main/kotlin/org/libremail/ui/accountsetup/AppPasswordSetupScreen.kt +++ b/app/src/main/kotlin/org/libremail/ui/accountsetup/AppPasswordSetupScreen.kt @@ -214,6 +214,11 @@ fun AppPasswordSetupScreen( Text(stringResource(R.string.app_password_test_and_add)) } } + // A wrong app password and "IMAP is turned off" both fail auth, but only the latter is fixed by + // a provider toggle (e.g. Gmail's Enable IMAP); surface it as an actionable prompt (#390). + form.imapDisabledPrompt?.let { prompt -> + ImapDisabledDialog(prompt = prompt, onDismiss = viewModel::dismissImapDisabledPrompt) + } } } diff --git a/app/src/main/kotlin/org/libremail/ui/accountsetup/AppPasswordViewModel.kt b/app/src/main/kotlin/org/libremail/ui/accountsetup/AppPasswordViewModel.kt index 9964c0a..e63a671 100644 --- a/app/src/main/kotlin/org/libremail/ui/accountsetup/AppPasswordViewModel.kt +++ b/app/src/main/kotlin/org/libremail/ui/accountsetup/AppPasswordViewModel.kt @@ -12,6 +12,8 @@ import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch import org.libremail.domain.model.MailProvider import org.libremail.domain.repository.AccountRepository +import org.libremail.reporting.AppLog +import org.libremail.reporting.accountLogRef import org.libremail.ui.navigation.Routes import javax.inject.Inject @@ -23,6 +25,11 @@ data class AppPasswordForm( val error: String? = null, /** Set alongside [SetupStatus.DONE]: the id of the account that was just added. */ val addedAccountId: String? = null, + /** + * Set instead of [error] when the failure was specifically an "IMAP is disabled" rejection (#390): + * the screen shows the actionable [ImapDisabledDialog] rather than the generic error snackbar. + */ + val imapDisabledPrompt: ImapDisabledPrompt? = null, ) { val isValid: Boolean get() = email.isNotBlank() && appPassword.isNotBlank() } @@ -53,6 +60,9 @@ class AppPasswordViewModel @Inject constructor( fun toggleAdvanced() = _form.update { it.copy(advancedExpanded = !it.advancedExpanded) } fun consumeError() = _form.update { it.copy(error = null) } + /** Clears the "IMAP is disabled" prompt after the user acknowledges it (issue #390). */ + fun dismissImapDisabledPrompt() = _form.update { it.copy(imapDisabledPrompt = null) } + fun testAndSave() { val provider = provider if (provider == null) { @@ -72,14 +82,28 @@ class AppPasswordViewModel @Inject constructor( _form.update { it.copy(status = SetupStatus.DONE, addedAccountId = account.id) } }, onFailure = { e -> - _form.update { - it.copy( - status = SetupStatus.IDLE, - error = e.message ?: "Could not connect to the server", + // App passwords are never XOAUTH2, so IMAP-disabled is inferred only from the + // server's message text (e.g. Gmail's "not enabled for IMAP use"), not a token + // signal (#390). + val prompt = imapDisabledPromptFor(e, account, usedOAuth = false) + if (prompt != null) { + AppLog.i( + TAG, + "IMAP disabled on app-password setup (${accountLogRef(account.id)}); " + + "prompting to enable IMAP", ) + _form.update { it.copy(status = SetupStatus.IDLE, imapDisabledPrompt = prompt, error = null) } + } else { + _form.update { + it.copy(status = SetupStatus.IDLE, error = e.message ?: "Could not connect to the server") + } } }, ) } } + + private companion object { + const val TAG = "AppPasswordVM" + } } diff --git a/app/src/main/kotlin/org/libremail/ui/accountsetup/ImapDisabledDialog.kt b/app/src/main/kotlin/org/libremail/ui/accountsetup/ImapDisabledDialog.kt new file mode 100644 index 0000000..c3f0d13 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/ui/accountsetup/ImapDisabledDialog.kt @@ -0,0 +1,56 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.ui.accountsetup + +import androidx.compose.material.icons.Icons +import androidx.compose.material.icons.filled.Email +import androidx.compose.material3.AlertDialog +import androidx.compose.material3.Icon +import androidx.compose.material3.Text +import androidx.compose.material3.TextButton +import androidx.compose.runtime.Composable +import androidx.compose.ui.platform.LocalUriHandler +import androidx.compose.ui.res.stringResource +import org.libremail.R + +/** + * Actionable dialog shown when adding an account fails specifically because IMAP is switched off for it + * (issue #390) — the reactive complement to the pre-auth Outlook notice (#411). Instead of the generic + * "authentication failed" snackbar, it explains that sign-in succeeded but the server rejected IMAP, + * and (when [ImapDisabledPrompt.helpUrl] is known) links the provider's page for turning IMAP on. + * + * A shared surface reused by every setup screen (the Outlook picker, the app-password form, manual + * setup) so the message and link stay consistent wherever the failure occurs. The help link is placed + * as the dialog's *dismiss* action (leading) and "Got it" as the *confirm* action (trailing), so the + * confirm button — the last control — stays the stable click target for E2E. + * + * @param prompt the provider brand + help URL to render. + * @param onDismiss clears the prompt from state (also fired on outside-tap / back). + */ +@Composable +fun ImapDisabledDialog(prompt: ImapDisabledPrompt, onDismiss: () -> Unit) { + val uriHandler = LocalUriHandler.current + val helpUrl = prompt.helpUrl + val message = prompt.brand?.let { stringResource(R.string.imap_disabled_message, it) } + ?: stringResource(R.string.imap_disabled_message_generic) + + AlertDialog( + onDismissRequest = onDismiss, + icon = { Icon(Icons.Filled.Email, contentDescription = null) }, + title = { Text(stringResource(R.string.imap_disabled_title)) }, + text = { Text(message) }, + // The help link is only offered for providers with a known enable-IMAP page. openUri throws + // when no browser is installed; swallow it (a rare case) — the primary "Got it" action, which + // dismisses so the user can fix the toggle and retry, always works. Opening the link + // deliberately leaves the dialog up so it is still there when the user returns from the browser. + dismissButton = { + if (helpUrl != null) { + TextButton(onClick = { runCatching { uriHandler.openUri(helpUrl) } }) { + Text(stringResource(R.string.imap_disabled_help)) + } + } + }, + confirmButton = { + TextButton(onClick = onDismiss) { Text(stringResource(R.string.imap_disabled_dismiss)) } + }, + ) +} diff --git a/app/src/main/kotlin/org/libremail/ui/accountsetup/ImapDisabledPrompt.kt b/app/src/main/kotlin/org/libremail/ui/accountsetup/ImapDisabledPrompt.kt new file mode 100644 index 0000000..07b2719 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/ui/accountsetup/ImapDisabledPrompt.kt @@ -0,0 +1,52 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.ui.accountsetup + +import org.libremail.domain.model.Account +import org.libremail.domain.model.MailProvider +import org.libremail.mail.ImapAuthError + +/** + * The data an "IMAP is turned off" prompt needs (issue #390): the provider [brand] for the message + * copy (null when the host maps to no known brand — a generic message is shown), and the [helpUrl] to + * the provider's enable-IMAP page (null when we have no provider-specific page, so no help link is + * offered). Built by [imapDisabledPromptFor] from a classified auth failure. + */ +data class ImapDisabledPrompt(val brand: String?, val helpUrl: String?) + +/** + * Returns the actionable prompt to show when [error] — the failure from adding [account] — is an + * "IMAP access is disabled" auth rejection ([ImapAuthError.isImapDisabled]), or null for any other + * failure (which keeps the existing generic error). [usedOAuth] is true only on the Outlook XOAUTH2 + * path, where a valid-token `AUTHENTICATE` rejection is itself the IMAP-disabled signal. + * + * Provider awareness comes from [account]: [MailProvider.brandFor] names the brand (Outlook by auth + * type/host, the app-password vendors by IMAP host — so even a manually-configured Gmail account is + * recognised), and [enableImapHelpUrl] maps it to the provider's help page. + */ +fun imapDisabledPromptFor(error: Throwable, account: Account, usedOAuth: Boolean): ImapDisabledPrompt? { + if (!ImapAuthError.isImapDisabled(error, usedOAuth)) return null + val brand = MailProvider.brandFor(account) + return ImapDisabledPrompt(brand = brand, helpUrl = enableImapHelpUrl(brand)) +} + +/** + * The provider's own "how to turn IMAP on" help page for a recognised [brand], or null for brands with + * no user-facing IMAP toggle we can link (Yahoo/iCloud/AOL gate access through app passwords, not an + * IMAP switch) or an unrecognised host — those get the generic message with no link. + */ +private fun enableImapHelpUrl(brand: String?): String? = when (brand) { + MailProvider.OUTLOOK_BRAND -> OUTLOOK_IMAP_HELP_URL + MailProvider.GMAIL.displayName -> GMAIL_IMAP_HELP_URL + else -> null +} + +// Microsoft's canonical "POP, IMAP, and SMTP settings for Outlook.com" support article — the +// authoritative walkthrough for the "Let devices and apps use POP/IMAP" toggle (verified 2026-07, +// issue #390). Matches the pre-auth notice in #411 so both directions point users to one page. +private const val OUTLOOK_IMAP_HELP_URL = + "https://support.microsoft.com/en-us/office/pop-imap-and-smtp-settings-for-outlook-com-" + + "d088b986-291d-42b8-9564-9c414e2aa040" + +// Google's "Check Gmail through other email platforms" article, which documents Settings -> See all +// settings -> Forwarding and POP/IMAP -> Enable IMAP (verified 2026-07, issue #390). +private const val GMAIL_IMAP_HELP_URL = "https://support.google.com/mail/answer/7126229" diff --git a/app/src/main/kotlin/org/libremail/ui/accountsetup/ManualSetupScreen.kt b/app/src/main/kotlin/org/libremail/ui/accountsetup/ManualSetupScreen.kt index 81bb451..a9752c2 100644 --- a/app/src/main/kotlin/org/libremail/ui/accountsetup/ManualSetupScreen.kt +++ b/app/src/main/kotlin/org/libremail/ui/accountsetup/ManualSetupScreen.kt @@ -185,6 +185,11 @@ fun ManualSetupScreen( Text(stringResource(R.string.manual_test_and_add)) } } + // "IMAP is turned off" is one of the auth failures a manual account can hit; when the host maps + // to a known brand it even links that provider's enable-IMAP page (#390). + form.imapDisabledPrompt?.let { prompt -> + ImapDisabledDialog(prompt = prompt, onDismiss = viewModel::dismissImapDisabledPrompt) + } } } diff --git a/app/src/main/kotlin/org/libremail/ui/accountsetup/ManualSetupViewModel.kt b/app/src/main/kotlin/org/libremail/ui/accountsetup/ManualSetupViewModel.kt index b27128a..640bda4 100644 --- a/app/src/main/kotlin/org/libremail/ui/accountsetup/ManualSetupViewModel.kt +++ b/app/src/main/kotlin/org/libremail/ui/accountsetup/ManualSetupViewModel.kt @@ -15,6 +15,8 @@ import org.libremail.domain.model.MailSecurity import org.libremail.domain.model.ServerConfig import org.libremail.domain.model.normalizeEmailForAccountId import org.libremail.domain.repository.AccountRepository +import org.libremail.reporting.AppLog +import org.libremail.reporting.accountLogRef import javax.inject.Inject data class ManualSetupForm( @@ -31,6 +33,11 @@ data class ManualSetupForm( val error: String? = null, /** Set alongside [SetupStatus.DONE]: the id of the account that was just added. */ val addedAccountId: String? = null, + /** + * Set instead of [error] when the failure was specifically an "IMAP is disabled" rejection (#390): + * the screen shows the actionable [ImapDisabledDialog] rather than the generic error snackbar. + */ + val imapDisabledPrompt: ImapDisabledPrompt? = null, ) { val isValid: Boolean get() = email.isNotBlank() && password.isNotBlank() && imapHost.isNotBlank() && smtpHost.isNotBlank() @@ -41,6 +48,7 @@ class ManualSetupViewModel @Inject constructor(private val accountRepository: Ac private companion object { const val MAX_PORT_DIGITS = 5 + const val TAG = "ManualSetupVM" } private val _form = MutableStateFlow(ManualSetupForm()) @@ -59,6 +67,9 @@ class ManualSetupViewModel @Inject constructor(private val accountRepository: Ac fun toggleAdvanced() = _form.update { it.copy(advancedExpanded = !it.advancedExpanded) } fun consumeError() = _form.update { it.copy(error = null) } + /** Clears the "IMAP is disabled" prompt after the user acknowledges it (issue #390). */ + fun dismissImapDisabledPrompt() = _form.update { it.copy(imapDisabledPrompt = null) } + fun testAndSave() { val f = _form.value if (!f.isValid) { @@ -79,16 +90,24 @@ class ManualSetupViewModel @Inject constructor(private val accountRepository: Ac _form.update { it.copy(status = SetupStatus.CONNECTING, error = null) } accountRepository.addImapAccount(account, f.password).fold( onSuccess = { _form.update { it.copy(status = SetupStatus.DONE, addedAccountId = account.id) } }, - onFailure = { e -> - _form.update { - it.copy( - status = SetupStatus.IDLE, - error = - e.message ?: "Could not connect to the server", - ) - } - }, + onFailure = { e -> onAddFailure(e, account) }, ) } } + + /** + * Routes a failed manual add: an "IMAP is disabled" rejection (recognised by server text; #390) + * gets the actionable prompt — provider-aware when the host maps to a known brand, e.g. a + * manually-configured Gmail account still links Gmail's enable-IMAP page — otherwise the generic + * error is kept. + */ + private fun onAddFailure(e: Throwable, account: Account) { + val prompt = imapDisabledPromptFor(e, account, usedOAuth = false) + if (prompt != null) { + AppLog.i(TAG, "IMAP disabled on manual setup (${accountLogRef(account.id)}); prompting to enable IMAP") + _form.update { it.copy(status = SetupStatus.IDLE, imapDisabledPrompt = prompt, error = null) } + } else { + _form.update { it.copy(status = SetupStatus.IDLE, error = e.message ?: "Could not connect to the server") } + } + } } diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index f02be49..e7d7d5b 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -205,6 +205,14 @@ Other (IMAP/SMTP) Choose your email provider to get started. + + Turn on IMAP to continue + You signed in successfully, but %1$s rejected IMAP access for this account. Make sure IMAP is turned on in your account settings, then try again. + You signed in successfully, but the mail server rejected IMAP access for this account. Make sure IMAP is turned on in your account settings, then try again. + How to turn on IMAP + Got it + Connect %1$s Unknown email provider. diff --git a/app/src/test/kotlin/org/libremail/mail/ImapAuthErrorTest.kt b/app/src/test/kotlin/org/libremail/mail/ImapAuthErrorTest.kt new file mode 100644 index 0000000..efc5e51 --- /dev/null +++ b/app/src/test/kotlin/org/libremail/mail/ImapAuthErrorTest.kt @@ -0,0 +1,194 @@ +// 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 jakarta.mail.AuthenticationFailedException +import jakarta.mail.MessagingException +import kotlinx.coroutines.test.runTest +import org.junit.After +import org.junit.Before +import org.junit.Test +import org.libremail.domain.model.ImapConnectionParams +import org.libremail.domain.model.MailSecurity +import java.io.IOException +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +/** + * Unit tests for the #390 error classifier: representative provider "IMAP is disabled" `AUTHENTICATE` + * rejections map to the distinct IMAP-disabled outcome, while ordinary wrong-password / expired-token / + * network failures do not (so the actionable "turn on IMAP" prompt never hijacks a real credential + * error). The provider-text cases use constructed exceptions carrying the real server wording; the + * "wrong password is not misclassified" case is proven end to end against a real GreenMail IMAP server. + */ +class ImapAuthErrorTest { + + // --- Provider "IMAP disabled" server text (works regardless of auth mechanism) --- + + @Test + fun `Gmail not-enabled-for-IMAP text is classified as disabled`() { + // Gmail's verbatim rejection when IMAP is off in its settings. + val error = AuthenticationFailedException( + "[ALERT] Your account is not enabled for IMAP use. Please visit your Gmail settings page " + + "and enable your account for IMAP access. (Failure)", + ) + assertTrue(ImapAuthError.isImapDisabled(error, usedOAuth = false)) + } + + @Test + fun `explicit IMAP access is disabled text is classified as disabled`() { + assertTrue( + ImapAuthError.isImapDisabled( + AuthenticationFailedException("IMAP access is disabled for your account."), + usedOAuth = false, + ), + ) + } + + @Test + fun `IMAP is disabled text is classified as disabled`() { + assertTrue(ImapAuthError.isImapDisabled(MessagingException("IMAP is disabled"), usedOAuth = false)) + } + + @Test + fun `IMAP access not enabled text is classified as disabled`() { + assertTrue( + ImapAuthError.isImapDisabled( + AuthenticationFailedException("IMAP access is not enabled for this account"), + usedOAuth = false, + ), + ) + } + + @Test + fun `please enable IMAP text is classified as disabled`() { + assertTrue( + ImapAuthError.isImapDisabled( + AuthenticationFailedException("Login failed. Please enable IMAP for your mailbox."), + usedOAuth = false, + ), + ) + } + + @Test + fun `disabled text in a wrapped cause is still classified as disabled`() { + val wrapped = RuntimeException("Adding account failed", AuthenticationFailedException("IMAP is disabled")) + assertTrue(ImapAuthError.isImapDisabled(wrapped, usedOAuth = false)) + } + + // --- Outlook OAuth inference: valid token + generic AUTHENTICATE rejection == IMAP off --- + + @Test + fun `a generic AUTHENTICATE failure on the XOAUTH2 path is inferred as disabled`() { + // outlook.office365.com returns only "AUTHENTICATE failed" with no distinctive text; the fresh + // token was already accepted at exchange, so this means IMAP is off (issue #390). + val error = AuthenticationFailedException("AUTHENTICATE failed") + assertTrue(ImapAuthError.isImapDisabled(error, usedOAuth = true)) + } + + @Test + fun `a wrapped AUTHENTICATE failure on the XOAUTH2 path is inferred as disabled`() { + val wrapped = MessagingException("connect failed", AuthenticationFailedException("AUTHENTICATE failed")) + assertTrue(ImapAuthError.isImapDisabled(wrapped, usedOAuth = true)) + } + + // --- Negatives: ordinary auth/other failures must NOT be misclassified --- + + @Test + fun `a generic AUTHENTICATE failure without OAuth is not classified as disabled`() { + // The password/app-password path: a plain "AUTHENTICATE failed" is a wrong password, not + // IMAP-off — misclassifying it would send the user to the wrong fix. + val error = AuthenticationFailedException("AUTHENTICATE failed") + assertFalse(ImapAuthError.isImapDisabled(error, usedOAuth = false)) + } + + @Test + fun `an invalid-credentials failure is not classified as disabled`() { + assertFalse( + ImapAuthError.isImapDisabled( + AuthenticationFailedException("[AUTHENTICATIONFAILED] Invalid credentials (Failure)"), + usedOAuth = false, + ), + ) + // Even on the OAuth path, an explicit invalid-credentials message is a token problem, not IMAP. + assertFalse( + ImapAuthError.isImapDisabled( + MessagingException("Invalid credentials, please re-authenticate"), + usedOAuth = true, + ), + ) + } + + @Test + fun `a token-exchange failure on the OAuth path is not classified as disabled`() { + // A token/consent failure is not a jakarta.mail AuthenticationFailedException, so the inference + // must not fire even with usedOAuth = true (the fix there is re-auth, not enabling IMAP). + assertFalse(ImapAuthError.isImapDisabled(IllegalStateException("Token exchange failed"), usedOAuth = true)) + } + + @Test + fun `a plain AUTHENTICATE-failed string that is not an auth exception is not inferred`() { + // The OAuth inference keys on the AuthenticationFailedException type, not the words + // "AUTHENTICATE failed" appearing in some other exception's message. + assertFalse(ImapAuthError.isImapDisabled(IOException("AUTHENTICATE failed"), usedOAuth = true)) + } + + @Test + fun `a network failure on the OAuth path is not classified as disabled`() { + assertFalse(ImapAuthError.isImapDisabled(IOException("Connection reset"), usedOAuth = true)) + } + + @Test + fun `an IMAP host name in the message alone is not classified as disabled`() { + // "imap.gmail.com" contains "imap" but no disabled/off wording, so it must not match. + assertFalse( + ImapAuthError.isImapDisabled( + AuthenticationFailedException("login to imap.gmail.com failed"), + usedOAuth = false, + ), + ) + } + + // --- End-to-end: a real GreenMail wrong-password rejection is not misclassified --- + + private lateinit var greenMail: GreenMail + private val client = ImapClient(reuseConnections = false) + + @Before + fun setUp() { + greenMail = GreenMail(ServerSetupTest.SMTP_IMAP) + greenMail.start() + greenMail.setUser("alice@example.org", "secret") + // listFolders breadcrumbs via AppLog -> android.util.Log, a throwing stub under plain JVM tests; + // mock it fully-qualified so this file never imports android.util.Log (detekt ForbiddenImport). + 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() + } + + @Test + fun `a real wrong-password IMAP rejection is not classified as disabled`() = runTest { + val params = ImapConnectionParams( + host = "127.0.0.1", + port = greenMail.imap.port, + security = MailSecurity.NONE, + username = "alice@example.org", + secret = "wrong-password", + useXoauth2 = false, + ) + val error = runCatching { client.listFolders(params) }.exceptionOrNull() + assertTrue(error != null, "GreenMail should reject the wrong password") + assertFalse(ImapAuthError.isImapDisabled(error, usedOAuth = false)) + } +} diff --git a/app/src/test/kotlin/org/libremail/ui/accountsetup/AccountPickerScreenJvmTest.kt b/app/src/test/kotlin/org/libremail/ui/accountsetup/AccountPickerScreenJvmTest.kt index 33b1557..aa1198a 100644 --- a/app/src/test/kotlin/org/libremail/ui/accountsetup/AccountPickerScreenJvmTest.kt +++ b/app/src/test/kotlin/org/libremail/ui/accountsetup/AccountPickerScreenJvmTest.kt @@ -213,4 +213,17 @@ class AccountPickerScreenJvmTest { composeTestRule.onNode(hasProgressBarRangeInfo(ProgressBarRangeInfo.Indeterminate)).assertIsDisplayed() } + + @Test + fun imapDisabledPrompt_showsTheDialog_andGotItDismisses() { + // Outlook OAuth can succeed while IMAP is off, surfacing the actionable dialog (#390) instead + // of the generic error snackbar; "Got it" clears the prompt via the view-model. + val vm = viewModel(AccountSetupUiState(imapDisabledPrompt = ImapDisabledPrompt("Outlook", helpUrl = null))) + setContent(vm) + + composeTestRule.onNodeWithText(string(R.string.imap_disabled_title)).assertIsDisplayed() + composeTestRule.onNodeWithText(string(R.string.imap_disabled_dismiss)).performClick() + + verify { vm.dismissImapDisabledPrompt() } + } } diff --git a/app/src/test/kotlin/org/libremail/ui/accountsetup/AccountSetupViewModelTest.kt b/app/src/test/kotlin/org/libremail/ui/accountsetup/AccountSetupViewModelTest.kt index 0033404..332bf07 100644 --- a/app/src/test/kotlin/org/libremail/ui/accountsetup/AccountSetupViewModelTest.kt +++ b/app/src/test/kotlin/org/libremail/ui/accountsetup/AccountSetupViewModelTest.kt @@ -10,6 +10,7 @@ import io.mockk.every import io.mockk.mockk import io.mockk.mockkStatic import io.mockk.unmockkAll +import jakarta.mail.AuthenticationFailedException import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.UnconfinedTestDispatcher @@ -191,6 +192,54 @@ class AccountSetupViewModelTest { assertEquals("IMAP verification failed", vm.state.value.error) } + @Test + fun `a token-OK but IMAP-disabled AUTHENTICATE failure surfaces the enable-IMAP prompt`() = runTest(dispatcher) { + // OAuth succeeds, then the IMAP AUTHENTICATE step is rejected (IMAP off for the mailbox): + // the actionable prompt replaces the generic error, and no account is marked added (#390). + val manager = mockk(relaxed = true) + coEvery { manager.exchangeToken(any()) } returns + OAuthResult(email = "me@outlook.com", accessToken = "tok", authStateJson = "{}") + val accounts = mockk(relaxed = true) + coEvery { accounts.addOutlookAccount(any(), any(), any()) } returns + Result.failure(AuthenticationFailedException("AUTHENTICATE failed")) + val vm = viewModel(outlookAuthManager = manager, accountRepository = accounts) + + vm.onOutlookResult(mockk(relaxed = true)) + advanceUntilIdle() + + val prompt = vm.state.value.imapDisabledPrompt + assertEquals("Outlook", prompt?.brand) + assertTrue(prompt?.helpUrl?.startsWith("https://support.microsoft.com/") == true) + assertEquals(SetupStatus.IDLE, vm.state.value.status) + assertNull(vm.state.value.error) + assertNull(vm.state.value.addedAccountId) + // PII-free breadcrumb: the account is referenced by its hashed log ref, never the email. + val disabledLine = logBuffer.snapshot().single { it.message.contains("IMAP disabled on Outlook sign-in") } + assertEquals('I', disabledLine.level) + assertFalse(disabledLine.message.contains("@"), disabledLine.message) + + vm.dismissImapDisabledPrompt() + assertNull(vm.state.value.imapDisabledPrompt) + } + + @Test + fun `a wrong-password style AUTHENTICATE failure keeps the generic error, not the prompt`() = runTest(dispatcher) { + // A plain OAuth-path failure with no IMAP-disabled signal stays a generic error. + val manager = mockk(relaxed = true) + coEvery { manager.exchangeToken(any()) } returns + OAuthResult(email = "me@outlook.com", accessToken = "tok", authStateJson = "{}") + val accounts = mockk(relaxed = true) + coEvery { accounts.addOutlookAccount(any(), any(), any()) } returns + Result.failure(RuntimeException("Could not reach the server")) + val vm = viewModel(outlookAuthManager = manager, accountRepository = accounts) + + vm.onOutlookResult(mockk(relaxed = true)) + advanceUntilIdle() + + assertNull(vm.state.value.imapDisabledPrompt) + assertEquals("Could not reach the server", vm.state.value.error) + } + @Test fun `a failure whose message carries the account email is scrubbed before it reaches the buffer`() = runTest(dispatcher) { diff --git a/app/src/test/kotlin/org/libremail/ui/accountsetup/AppPasswordSetupScreenJvmTest.kt b/app/src/test/kotlin/org/libremail/ui/accountsetup/AppPasswordSetupScreenJvmTest.kt index d83c866..c8e8b7b 100644 --- a/app/src/test/kotlin/org/libremail/ui/accountsetup/AppPasswordSetupScreenJvmTest.kt +++ b/app/src/test/kotlin/org/libremail/ui/accountsetup/AppPasswordSetupScreenJvmTest.kt @@ -232,4 +232,21 @@ class AppPasswordSetupScreenJvmTest { assertTrue(openedUrls.contains(MailProvider.GMAIL.appPasswordHelpUrl)) assertTrue(openedUrls.contains(MailProvider.GMAIL.twoFactorHelpUrl)) } + + @Test + fun imapDisabledPrompt_showsTheDialog_andGotItDismisses() { + // A wrong app password and "IMAP is off" both fail auth; the latter surfaces the actionable + // dialog (#390) instead of the generic snackbar, and "Got it" clears it via the view-model. + val vm = viewModel( + MailProvider.GMAIL, + AppPasswordForm(imapDisabledPrompt = ImapDisabledPrompt(brand = "Gmail", helpUrl = null)), + ) + setContent(vm) + + composeTestRule.onNodeWithText(string(R.string.imap_disabled_title)).assertIsDisplayed() + composeTestRule.onNodeWithText(string(R.string.imap_disabled_message, "Gmail")).assertIsDisplayed() + composeTestRule.onNodeWithText(string(R.string.imap_disabled_dismiss)).performClick() + + verify { vm.dismissImapDisabledPrompt() } + } } diff --git a/app/src/test/kotlin/org/libremail/ui/accountsetup/AppPasswordViewModelTest.kt b/app/src/test/kotlin/org/libremail/ui/accountsetup/AppPasswordViewModelTest.kt index 81b13ec..c379ab3 100644 --- a/app/src/test/kotlin/org/libremail/ui/accountsetup/AppPasswordViewModelTest.kt +++ b/app/src/test/kotlin/org/libremail/ui/accountsetup/AppPasswordViewModelTest.kt @@ -4,8 +4,12 @@ package org.libremail.ui.accountsetup import androidx.lifecycle.SavedStateHandle import io.mockk.coEvery import io.mockk.coVerify +import io.mockk.every import io.mockk.mockk +import io.mockk.mockkStatic import io.mockk.slot +import io.mockk.unmockkAll +import jakarta.mail.AuthenticationFailedException import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.UnconfinedTestDispatcher @@ -30,10 +34,22 @@ class AppPasswordViewModelTest { private val testDispatcher = UnconfinedTestDispatcher() @Before - fun setUp() = Dispatchers.setMain(testDispatcher) + fun setUp() { + Dispatchers.setMain(testDispatcher) + // The IMAP-disabled branch breadcrumbs via AppLog -> android.util.Log, a throwing stub under + // plain JVM tests. Mock it fully-qualified so this file never imports android.util.Log (which + // detekt's ForbiddenImport guard would flag). + mockkStatic(android.util.Log::class) + every { android.util.Log.i(any(), any()) } returns 0 + every { android.util.Log.d(any(), any()) } returns 0 + every { android.util.Log.w(any(), any()) } returns 0 + } @After - fun tearDown() = Dispatchers.resetMain() + fun tearDown() { + Dispatchers.resetMain() + unmockkAll() + } private fun viewModel(repo: AccountRepository, providerKey: String = MailProvider.GMAIL.key) = AppPasswordViewModel( SavedStateHandle(mapOf(Routes.APP_PASSWORD_ARG_PROVIDER to providerKey)), @@ -101,6 +117,29 @@ class AppPasswordViewModelTest { assertNull(vm.form.value.addedAccountId) } + @Test + fun `an IMAP-disabled failure surfaces the enable-IMAP prompt instead of a generic error`() = + runTest(testDispatcher) { + val repo = mockk() + coEvery { repo.addImapAccount(any(), any()) } returns + Result.failure(AuthenticationFailedException("Your account is not enabled for IMAP use")) + val vm = viewModel(repo) // Gmail preset + + vm.onEmail("user@gmail.com") + vm.onAppPassword("app-pass") + vm.testAndSave() + + val prompt = vm.form.value.imapDisabledPrompt + assertEquals(MailProvider.GMAIL.displayName, prompt?.brand) + assertTrue(prompt?.helpUrl?.startsWith("https://support.google.com/") == true) + assertNull(vm.form.value.error) + assertEquals(SetupStatus.IDLE, vm.form.value.status) + assertNull(vm.form.value.addedAccountId) + + vm.dismissImapDisabledPrompt() + assertNull(vm.form.value.imapDisabledPrompt) + } + @Test fun `an unknown provider key surfaces an error and never contacts the server`() = runTest(testDispatcher) { val repo = mockk(relaxed = true) diff --git a/app/src/test/kotlin/org/libremail/ui/accountsetup/ImapDisabledDialogJvmTest.kt b/app/src/test/kotlin/org/libremail/ui/accountsetup/ImapDisabledDialogJvmTest.kt new file mode 100644 index 0000000..886762b --- /dev/null +++ b/app/src/test/kotlin/org/libremail/ui/accountsetup/ImapDisabledDialogJvmTest.kt @@ -0,0 +1,105 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.ui.accountsetup + +import android.content.Context +import androidx.compose.runtime.CompositionLocalProvider +import androidx.compose.ui.platform.LocalUriHandler +import androidx.compose.ui.platform.UriHandler +import androidx.compose.ui.test.assertIsDisplayed +import androidx.compose.ui.test.junit4.v2.createComposeRule +import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.performClick +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.libremail.R +import org.libremail.ui.theme.LibreMailTheme +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import org.robolectric.annotation.Config +import org.robolectric.annotation.GraphicsMode + +/** + * Robolectric JVM Compose test for the reactive "IMAP is disabled" dialog (#390). Drives the real + * [ImapDisabledDialog] on the JVM via the v2 `createComposeRule()` under [RobolectricTestRunner] — no + * emulator — so the dialog counts toward JaCoCo's JVM-testable surface. A recording [UriHandler] + * captures the help-link launch instead of opening a real browser; the instrumented + * [org.libremail.ui.accountsetup.AppPasswordSetupScreenTest] covers the end-to-end error state on + * device. + */ +@RunWith(RobolectricTestRunner::class) +@GraphicsMode(GraphicsMode.Mode.NATIVE) +@Config(sdk = [36], qualifiers = "+w411dp-h2000dp") +class ImapDisabledDialogJvmTest { + + @get:Rule + val composeTestRule = createComposeRule() + + private val context: Context get() = RuntimeEnvironment.getApplication() + + private fun string(resId: Int, vararg args: Any): String = context.getString(resId, *args) + + private val openedUrls = mutableListOf() + private val recordingUriHandler = object : UriHandler { + override fun openUri(uri: String) { + openedUrls.add(uri) + } + } + + private val outlookPrompt = ImapDisabledPrompt(brand = "Outlook", helpUrl = "https://support.microsoft.com/imap") + + private fun setContent(prompt: ImapDisabledPrompt, onDismiss: () -> Unit = {}) { + composeTestRule.setContent { + CompositionLocalProvider(LocalUriHandler provides recordingUriHandler) { + LibreMailTheme(darkTheme = false, dynamicColor = false) { + ImapDisabledDialog(prompt = prompt, onDismiss = onDismiss) + } + } + } + } + + @Test + fun brandedPrompt_showsTitle_brandMessage_help_andDismiss() { + setContent(outlookPrompt) + + composeTestRule.onNodeWithText(string(R.string.imap_disabled_title)).assertIsDisplayed() + composeTestRule.onNodeWithText(string(R.string.imap_disabled_message, "Outlook")).assertIsDisplayed() + composeTestRule.onNodeWithText(string(R.string.imap_disabled_help)).assertIsDisplayed() + composeTestRule.onNodeWithText(string(R.string.imap_disabled_dismiss)).assertIsDisplayed() + } + + @Test + fun genericPrompt_showsGenericMessage_andNoHelpLink() { + setContent(ImapDisabledPrompt(brand = null, helpUrl = null)) + + composeTestRule.onNodeWithText(string(R.string.imap_disabled_message_generic)).assertIsDisplayed() + // No provider page to link, so the help button is absent — only "Got it" remains. + composeTestRule.onNodeWithText(string(R.string.imap_disabled_help)).assertDoesNotExist() + composeTestRule.onNodeWithText(string(R.string.imap_disabled_dismiss)).assertIsDisplayed() + } + + @Test + fun tappingHelp_opensTheProviderPage_withoutDismissing() { + var dismissed = false + setContent(outlookPrompt, onDismiss = { dismissed = true }) + + composeTestRule.onNodeWithText(string(R.string.imap_disabled_help)).performClick() + + assertEquals(listOf("https://support.microsoft.com/imap"), openedUrls) + // Opening the link leaves the dialog up so it is still there when the user returns. + assertFalse("Opening the help link must not dismiss the dialog", dismissed) + } + + @Test + fun tappingGotIt_dismisses() { + var dismissed = false + setContent(outlookPrompt, onDismiss = { dismissed = true }) + + composeTestRule.onNodeWithText(string(R.string.imap_disabled_dismiss)).performClick() + + assertTrue(dismissed) + } +} diff --git a/app/src/test/kotlin/org/libremail/ui/accountsetup/ImapDisabledPromptTest.kt b/app/src/test/kotlin/org/libremail/ui/accountsetup/ImapDisabledPromptTest.kt new file mode 100644 index 0000000..6bb6e93 --- /dev/null +++ b/app/src/test/kotlin/org/libremail/ui/accountsetup/ImapDisabledPromptTest.kt @@ -0,0 +1,113 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.ui.accountsetup + +import jakarta.mail.AuthenticationFailedException +import org.junit.Test +import org.libremail.domain.model.Account +import org.libremail.domain.model.AuthType +import org.libremail.domain.model.MailProvider +import org.libremail.domain.model.MailSecurity +import org.libremail.domain.model.ServerConfig +import kotlin.test.assertEquals +import kotlin.test.assertNull +import kotlin.test.assertTrue + +/** + * Unit tests for [imapDisabledPromptFor]: an "IMAP disabled" failure is turned into a provider-aware + * prompt (brand copy + the provider's enable-IMAP help link when one is known), while any other failure + * yields null so the caller keeps its generic error. Classification itself is covered by + * [org.libremail.mail.ImapAuthErrorTest]; this pins the brand/URL resolution. + */ +class ImapDisabledPromptTest { + + private fun manualAccount(host: String) = Account( + id = "imap:user@$host", + email = "user@$host", + displayName = "user@$host", + authType = AuthType.PASSWORD_IMAP, + imap = ServerConfig(host, 993, MailSecurity.SSL_TLS), + smtp = ServerConfig(host, 465, MailSecurity.SSL_TLS), + ) + + @Test + fun `outlook OAuth failure resolves to the Outlook brand and Microsoft help link`() { + val prompt = imapDisabledPromptFor( + AuthenticationFailedException("AUTHENTICATE failed"), + Account.outlook("me@outlook.com"), + usedOAuth = true, + ) + + assertEquals(MailProvider.OUTLOOK_BRAND, prompt?.brand) + assertTrue(prompt?.helpUrl?.startsWith("https://support.microsoft.com/") == true, prompt?.helpUrl) + } + + @Test + fun `gmail app-password failure resolves to the Gmail brand and Google help link`() { + val prompt = imapDisabledPromptFor( + AuthenticationFailedException("Your account is not enabled for IMAP use"), + MailProvider.GMAIL.createAccount("me@gmail.com"), + usedOAuth = false, + ) + + assertEquals(MailProvider.GMAIL.displayName, prompt?.brand) + assertTrue(prompt?.helpUrl?.startsWith("https://support.google.com/") == true, prompt?.helpUrl) + } + + @Test + fun `a manually-configured Gmail host is still recognised as Gmail`() { + // brandFor resolves the brand from the IMAP host, so a manual account on imap.gmail.com gets + // the Gmail copy + link even though it was set up through the generic path. + val prompt = imapDisabledPromptFor( + AuthenticationFailedException("IMAP is disabled"), + manualAccount("imap.gmail.com"), + usedOAuth = false, + ) + + assertEquals(MailProvider.GMAIL.displayName, prompt?.brand) + assertTrue(prompt?.helpUrl?.startsWith("https://support.google.com/") == true, prompt?.helpUrl) + } + + @Test + fun `a Yahoo failure keeps the brand but offers no link`() { + // Yahoo/iCloud/AOL gate access via app passwords, not a user-facing IMAP toggle we can deep-link. + val prompt = imapDisabledPromptFor( + AuthenticationFailedException("IMAP access is disabled"), + MailProvider.YAHOO.createAccount("me@yahoo.com"), + usedOAuth = false, + ) + + assertEquals(MailProvider.YAHOO.displayName, prompt?.brand) + assertNull(prompt?.helpUrl) + } + + @Test + fun `an unknown host yields a generic prompt with no brand and no link`() { + val prompt = imapDisabledPromptFor( + AuthenticationFailedException("IMAP access is disabled"), + manualAccount("mail.example.org"), + usedOAuth = false, + ) + + assertNull(prompt?.brand) + assertNull(prompt?.helpUrl) + } + + @Test + fun `a wrong-password failure yields no prompt`() { + val prompt = imapDisabledPromptFor( + AuthenticationFailedException("Invalid credentials"), + MailProvider.GMAIL.createAccount("me@gmail.com"), + usedOAuth = false, + ) + + assertNull(prompt) + } + + @Test + fun `ImapDisabledPrompt value semantics`() { + val prompt = ImapDisabledPrompt(brand = "Outlook", helpUrl = "https://example.test/imap") + assertEquals(prompt, prompt.copy()) + assertEquals(prompt.hashCode(), prompt.copy().hashCode()) + assertTrue(prompt.toString().contains("Outlook")) + } +} diff --git a/app/src/test/kotlin/org/libremail/ui/accountsetup/ManualSetupScreenJvmTest.kt b/app/src/test/kotlin/org/libremail/ui/accountsetup/ManualSetupScreenJvmTest.kt index 6e84ade..c0e8a1f 100644 --- a/app/src/test/kotlin/org/libremail/ui/accountsetup/ManualSetupScreenJvmTest.kt +++ b/app/src/test/kotlin/org/libremail/ui/accountsetup/ManualSetupScreenJvmTest.kt @@ -212,4 +212,18 @@ class ManualSetupScreenJvmTest { composeTestRule.onAllNodesWithText("STARTTLS")[1].performClick() verify { vm.onSmtpSecurity(MailSecurity.STARTTLS) } } + + @Test + fun imapDisabledPrompt_showsTheGenericDialog_andGotItDismisses() { + // A manual account on an unknown host still gets the actionable "IMAP is off" dialog (#390), + // with the generic (brandless) message and no help link; "Got it" clears it via the view-model. + val vm = viewModel(ManualSetupForm(imapDisabledPrompt = ImapDisabledPrompt(brand = null, helpUrl = null))) + setContent(vm) + + composeTestRule.onNodeWithText(string(R.string.imap_disabled_title)).assertIsDisplayed() + composeTestRule.onNodeWithText(string(R.string.imap_disabled_message_generic)).assertIsDisplayed() + composeTestRule.onNodeWithText(string(R.string.imap_disabled_dismiss)).performClick() + + verify { vm.dismissImapDisabledPrompt() } + } } diff --git a/app/src/test/kotlin/org/libremail/ui/accountsetup/ManualSetupViewModelTest.kt b/app/src/test/kotlin/org/libremail/ui/accountsetup/ManualSetupViewModelTest.kt index 882e1de..4134cf1 100644 --- a/app/src/test/kotlin/org/libremail/ui/accountsetup/ManualSetupViewModelTest.kt +++ b/app/src/test/kotlin/org/libremail/ui/accountsetup/ManualSetupViewModelTest.kt @@ -3,8 +3,12 @@ package org.libremail.ui.accountsetup import io.mockk.coEvery import io.mockk.coVerify +import io.mockk.every import io.mockk.mockk +import io.mockk.mockkStatic import io.mockk.slot +import io.mockk.unmockkAll +import jakarta.mail.AuthenticationFailedException import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.UnconfinedTestDispatcher @@ -29,10 +33,22 @@ class ManualSetupViewModelTest { private val dispatcher = UnconfinedTestDispatcher() @Before - fun setUp() = Dispatchers.setMain(dispatcher) + fun setUp() { + Dispatchers.setMain(dispatcher) + // The IMAP-disabled branch breadcrumbs via AppLog -> android.util.Log, a throwing stub under + // plain JVM tests. Mock it fully-qualified so this file never imports android.util.Log (which + // detekt's ForbiddenImport guard would flag). + mockkStatic(android.util.Log::class) + every { android.util.Log.i(any(), any()) } returns 0 + every { android.util.Log.d(any(), any()) } returns 0 + every { android.util.Log.w(any(), any()) } returns 0 + } @After - fun tearDown() = Dispatchers.resetMain() + fun tearDown() { + Dispatchers.resetMain() + unmockkAll() + } private fun filled(vm: ManualSetupViewModel) { vm.onEmail(" user@example.org ") @@ -184,6 +200,28 @@ class ManualSetupViewModelTest { assertNull(vm.form.value.addedAccountId) } + @Test + fun `testAndSave surfaces the enable-IMAP prompt for an IMAP-disabled rejection`() = runTest(dispatcher) { + val repo = mockk() + coEvery { repo.addImapAccount(any(), any()) } returns + Result.failure(AuthenticationFailedException("IMAP access is disabled for this account")) + val vm = ManualSetupViewModel(repo) + filled(vm) // imap.example.org: a generic host with no known brand + + vm.testAndSave() + + val prompt = vm.form.value.imapDisabledPrompt + assertNotEquals(null, prompt) + assertNull(prompt?.brand) // unknown host -> generic message, no help link + assertNull(prompt?.helpUrl) + assertNull(vm.form.value.error) + assertEquals(SetupStatus.IDLE, vm.form.value.status) + assertNull(vm.form.value.addedAccountId) + + vm.dismissImapDisabledPrompt() + assertNull(vm.form.value.imapDisabledPrompt) + } + @Test fun `testAndSave uses a generic message when the failure has none`() = runTest(dispatcher) { val repo = mockk() -- 2.47.3 From 49594da10e9f3d2b6a8d031e086802e2e10f0981 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 8 Jul 2026 07:24:49 -0500 Subject: [PATCH 5/5] fix(mail): below-cut mail/richtext perf & correctness nits (#298) Address the Phase-3 review nits collected in #298: - perf(HtmlToText): hoist the 4 per-call Regex literals in convert() to private vals so each compiles once, not once per fetched HTML body. - fix(ReportStore): write reports via temp-file + atomic rename so a crash mid-write can't truncate a .json that scan() then silently drops. Temp uses a non-.json suffix so it is never scanned. - fix(RichTextEditing): applyLink now splits partially-overlapping links (keeping the non-overlapping remainder) instead of un-linking it whole, mirroring subtractRange. - perf(GraphSender): guard attachment size before readBytes() so an oversized file can't OOM or blow Graph sendMail's ~4 MB request cap; fails mayHaveSent=false so the outbox falls back to SMTP (which streams). - perf(RichText): mergeSameValueSpans is O(n) via a last-run-per-style map instead of O(n^2) indexOfLast; output is identical. - fix(DiagnosticsCollector): bucket provider labels by DNS-label boundary, not raw substring, so mail.notgmail.example no longer reads as Gmail. Adds/updates unit tests for each behavioural change; pure-perf nits keep their existing green coverage plus a direct mergeSameValueSpans equivalence test. --- .../kotlin/org/libremail/mail/GraphSender.kt | 14 +++++++ .../kotlin/org/libremail/mail/HtmlToText.kt | 16 +++++--- .../reporting/DiagnosticsCollector.kt | 20 +++++++--- .../org/libremail/reporting/ReportStore.kt | 35 +++++++++++++++- .../kotlin/org/libremail/richtext/RichText.kt | 10 ++++- .../org/libremail/richtext/RichTextEditing.kt | 20 +++++++++- .../org/libremail/mail/GraphSenderSendTest.kt | 40 ++++++++++++++++++- .../reporting/DiagnosticsCollectorTest.kt | 20 ++++++++++ .../libremail/reporting/ReportStoreTest.kt | 23 +++++++++++ .../libremail/richtext/RichTextEditingTest.kt | 39 +++++++++++++++++- 10 files changed, 218 insertions(+), 19 deletions(-) diff --git a/app/src/main/kotlin/org/libremail/mail/GraphSender.kt b/app/src/main/kotlin/org/libremail/mail/GraphSender.kt index a183458..bbe3762 100644 --- a/app/src/main/kotlin/org/libremail/mail/GraphSender.kt +++ b/app/src/main/kotlin/org/libremail/mail/GraphSender.kt @@ -7,6 +7,7 @@ import kotlinx.coroutines.withContext import org.json.JSONArray import org.json.JSONObject import org.libremail.domain.model.OutgoingMessage +import org.libremail.reporting.AppLog import java.io.IOException import java.net.HttpURLConnection import java.net.URL @@ -34,6 +35,14 @@ class GraphSender @Inject constructor() { message: OutgoingMessage, attachments: List = emptyList(), ) = withContext(Dispatchers.IO) { + // Guard before any attachment is read into memory: Graph sendMail carries attachment bytes inline + // (base64) in a single ~4 MB request, so an oversized file would blow that request limit and risk + // an OOM from readBytes(). Fail with mayHaveSent=false so the outbox falls back to SMTP, which + // streams attachments and handles far larger files (#298). + attachments.firstOrNull { it.file.length() > MAX_ATTACHMENT_BYTES }?.let { + AppLog.w(TAG, "Attachment over Graph sendMail size limit; not sending via Graph") + throw GraphSendException("Attachment exceeds the Graph sendMail size limit", mayHaveSent = false) + } val payload = buildSendMailPayload(message, attachments) val connection = (URL(SEND_MAIL_URL).openConnection() as HttpURLConnection).apply { requestMethod = "POST" @@ -76,8 +85,13 @@ class GraphSender @Inject constructor() { } private companion object { + const val TAG = "GraphSender" const val SEND_MAIL_URL = "https://graph.microsoft.com/v1.0/me/sendMail" const val TIMEOUT_MS = 15_000 + + // Per-file ceiling kept below Graph sendMail's ~4 MB whole-request cap, so one attachment can never + // exceed the request limit or OOM when read into the base64 payload; larger files fall back to SMTP. + const val MAX_ATTACHMENT_BYTES = 3L * 1024 * 1024 const val HTTP_OK_MIN = 200 const val HTTP_OK_MAX = 299 const val ERROR_BODY_LIMIT = 500 diff --git a/app/src/main/kotlin/org/libremail/mail/HtmlToText.kt b/app/src/main/kotlin/org/libremail/mail/HtmlToText.kt index 495368a..0c77768 100644 --- a/app/src/main/kotlin/org/libremail/mail/HtmlToText.kt +++ b/app/src/main/kotlin/org/libremail/mail/HtmlToText.kt @@ -12,23 +12,29 @@ package org.libremail.mail */ object HtmlToText { + // Hoisted out of convert() so each pattern is compiled once, not four times per call — convert() + // runs once per fetched HTML body during sync, so this is a hot path (#298). + private val SCRIPT_STYLE = Regex("(?is)<(script|style)\\b[^>]*>.*?") + private val LIST_ITEM = Regex("(?i)]*>") private val BLOCK_BREAK = Regex( "(?i)]*>|", ) - private val LIST_ITEM = Regex("(?i)]*>") + private val TAG = Regex("<[^>]*>") + private val SPACES_AND_TABS = Regex("[ \\t]+") + private val BLANK_LINES = Regex("\n{3,}") fun convert(html: String): String { var s = html // Drop script/style contents outright so their text never leaks into the output. - s = s.replace(Regex("(?is)<(script|style)\\b[^>]*>.*?"), "") + s = SCRIPT_STYLE.replace(s, "") s = LIST_ITEM.replace(s, "\n• ") s = BLOCK_BREAK.replace(s, "\n") - s = s.replace(Regex("<[^>]*>"), "") + s = TAG.replace(s, "") s = decodeEntities(s) // Collapse runs of spaces/tabs, then trim trailing spaces and cap consecutive blank lines. - s = s.replace(Regex("[ \\t]+"), " ") + s = SPACES_AND_TABS.replace(s, " ") s = s.lineSequence().joinToString("\n") { it.trim() } - s = s.replace(Regex("\n{3,}"), "\n\n") + s = BLANK_LINES.replace(s, "\n\n") return s.trim() } diff --git a/app/src/main/kotlin/org/libremail/reporting/DiagnosticsCollector.kt b/app/src/main/kotlin/org/libremail/reporting/DiagnosticsCollector.kt index 2581525..6fdf54a 100644 --- a/app/src/main/kotlin/org/libremail/reporting/DiagnosticsCollector.kt +++ b/app/src/main/kotlin/org/libremail/reporting/DiagnosticsCollector.kt @@ -99,14 +99,24 @@ private fun providerLabel(account: Account): String = when (account.authType) { AuthType.PASSWORD_IMAP -> imapProviderLabel(account.imap.host) } +/** + * Buckets an IMAP host to a coarse provider by matching brand tokens at DNS-label boundaries rather + * than as raw substrings, so a custom domain that merely contains a brand name — e.g. + * `mail.notgmail.example` — is no longer mislabeled (here it would have read as Gmail) (#298). The + * short, common tokens (`me`/`mac`/`live`) match only as a registrable-domain suffix, never as a bare + * label, so an innocent `me.company.example` doesn't read as iCloud either. + */ private fun imapProviderLabel(host: String): String { val h = host.lowercase() + val labels = h.split('.') + fun hasLabel(vararg brands: String) = brands.any { it in labels } + fun hasDomain(vararg domains: String) = domains.any { h == it || h.endsWith(".$it") } return when { - "gmail" in h || "googlemail" in h -> "Gmail" - "yahoo" in h -> "Yahoo" - "icloud" in h || "me.com" in h || "mac.com" in h -> "iCloud" - "outlook" in h || "office365" in h || "hotmail" in h || "live.com" in h -> "Outlook" - "aol" in h -> "AOL" + hasLabel("gmail", "googlemail") -> "Gmail" + hasLabel("yahoo") -> "Yahoo" + hasLabel("icloud") || hasDomain("me.com", "mac.com") -> "iCloud" + hasLabel("outlook", "office365", "hotmail") || hasDomain("live.com") -> "Outlook" + hasLabel("aol") -> "AOL" else -> "Other" } } diff --git a/app/src/main/kotlin/org/libremail/reporting/ReportStore.kt b/app/src/main/kotlin/org/libremail/reporting/ReportStore.kt index 3fe7b67..d4cfa63 100644 --- a/app/src/main/kotlin/org/libremail/reporting/ReportStore.kt +++ b/app/src/main/kotlin/org/libremail/reporting/ReportStore.kt @@ -9,6 +9,9 @@ import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.launch import java.io.File +import java.nio.file.AtomicMoveNotSupportedException +import java.nio.file.Files +import java.nio.file.StandardCopyOption /** * File-backed store of pending [DebugReport]s — one JSON file per report under [directory]. @@ -52,7 +55,7 @@ class ReportStore( synchronized(lock) { val serialized = serializeForDisk(report) ?: return directory.mkdirs() - File(directory, fileName(report.id)).writeText(serialized) + writeAtomically(File(directory, fileName(report.id)), serialized) _reports.value = scan() } } @@ -69,7 +72,7 @@ class ReportStore( val report = _reports.value.firstOrNull { it.id == id } ?: return if (report.surfaced) return val serialized = serializeForDisk(report.copy(surfaced = true)) ?: return - File(directory, fileName(id)).writeText(serialized) + writeAtomically(File(directory, fileName(id)), serialized) _reports.value = scan() } } @@ -135,10 +138,38 @@ class ReportStore( return runCatching { DebugReport.fromStorageJson(json) }.getOrNull() } + /** + * Writes [content] to [target] via a temp file + atomic rename, so a process death mid-write — the + * crash path that saves a report while the app is dying — can never leave a torn `.json` that [scan] + * would fail to parse and silently drop (#298). The temp file uses a non-`.json` suffix so [scan] + * ignores it (and any orphan left by an interrupted write), and the rename replaces an existing file + * (the [markSurfaced] rewrite) atomically. Falls back to a plain replace on the rare filesystem + * without atomic rename — still safer than an in-place truncate-then-write. + */ + private fun writeAtomically(target: File, content: String) { + val tmp = File(directory, target.name + TMP_SUFFIX) + tmp.writeText(content) + try { + Files.move( + tmp.toPath(), + target.toPath(), + StandardCopyOption.ATOMIC_MOVE, + StandardCopyOption.REPLACE_EXISTING, + ) + } catch (e: AtomicMoveNotSupportedException) { + AppLog.w(TAG, "Atomic report write unsupported here; falling back to a non-atomic replace", e) + Files.move(tmp.toPath(), target.toPath(), StandardCopyOption.REPLACE_EXISTING) + } + } + private fun fileName(id: String) = "$id$SUFFIX" private companion object { const val SUFFIX = ".json" + + // Suffix for the write-and-rename temp file. Deliberately NOT ending in [SUFFIX] so scan() never + // treats a half-written or orphaned temp as a report (#298). + const val TMP_SUFFIX = ".tmp" const val TAG = "ReportStore" /** diff --git a/app/src/main/kotlin/org/libremail/richtext/RichText.kt b/app/src/main/kotlin/org/libremail/richtext/RichText.kt index 4da265e..23029ad 100644 --- a/app/src/main/kotlin/org/libremail/richtext/RichText.kt +++ b/app/src/main/kotlin/org/libremail/richtext/RichText.kt @@ -109,11 +109,17 @@ internal fun lineMarker(line: String): String? = when { */ internal fun mergeSameValueSpans(spans: List): List { val merged = ArrayList() + // Index of the right-most merged run for each style value. Spans are processed in ascending start + // order, and runs of one value stay non-overlapping with strictly increasing ends, so only that + // value's last run can touch the next span — track it directly instead of re-scanning `merged` for + // every span (the old O(n^2) indexOfLast). The produced list is byte-for-byte identical (#298). + val lastRunByStyle = HashMap() for (span in spans.sortedWith(compareBy({ it.start }, { it.end }))) { - val i = merged.indexOfLast { it.style == span.style && span.start <= it.end } - if (i >= 0) { + val i = lastRunByStyle[span.style] + if (i != null && span.start <= merged[i].end) { merged[i] = merged[i].copy(end = maxOf(merged[i].end, span.end)) } else { + lastRunByStyle[span.style] = merged.size merged.add(span) } } diff --git a/app/src/main/kotlin/org/libremail/richtext/RichTextEditing.kt b/app/src/main/kotlin/org/libremail/richtext/RichTextEditing.kt index ed67e10..65bccf9 100644 --- a/app/src/main/kotlin/org/libremail/richtext/RichTextEditing.kt +++ b/app/src/main/kotlin/org/libremail/richtext/RichTextEditing.kt @@ -33,10 +33,15 @@ object RichTextEditing { return content.copy(spans = (otherKinds + updated).sortedBy { it.start }) } - /** Links [[start], [end]) to [url], replacing any links that overlap the range. */ + /** + * Links [[start], [end]) to [url]. A link that only partially overlaps the range keeps its + * non-overlapping remainder — the same split [toggleStyle]/[subtractRange] does for spans — instead + * of being dropped whole, so relinking part of a longer link no longer silently un-links the rest + * of it (#298). A link fully inside the range is replaced outright. + */ fun applyLink(content: RichTextContent, start: Int, end: Int, url: String): RichTextContent { if (start >= end || url.isBlank()) return content - val kept = content.links.filter { it.end <= start || it.start >= end } + val kept = subtractLinkRange(content.links, start, end) return content.copy(links = (kept + RichLink(start, end, url)).sortedBy { it.start }) } @@ -221,6 +226,17 @@ private fun subtractRange(spans: List, start: Int, end: Int): List, start: Int, end: Int): List = links.flatMap { link -> + when { + link.end <= start || link.start >= end -> listOf(link) + else -> buildList { + if (link.start < start) add(link.copy(end = start)) + if (link.end > end) add(link.copy(start = end)) + } + } +} + // --- block marker helpers --- private val ORDERED = Regex("^\\d+\\. ") diff --git a/app/src/test/kotlin/org/libremail/mail/GraphSenderSendTest.kt b/app/src/test/kotlin/org/libremail/mail/GraphSenderSendTest.kt index dbcb6de..c4c02ea 100644 --- a/app/src/test/kotlin/org/libremail/mail/GraphSenderSendTest.kt +++ b/app/src/test/kotlin/org/libremail/mail/GraphSenderSendTest.kt @@ -1,14 +1,20 @@ // 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.test.runTest import org.junit.After +import org.junit.Before import org.junit.Test import org.libremail.domain.model.OutgoingMessage import java.io.ByteArrayOutputStream +import java.io.File import java.io.IOException import java.io.InputStream import java.io.OutputStream +import java.io.RandomAccessFile import java.net.HttpURLConnection import java.net.URL import java.net.URLConnection @@ -18,6 +24,7 @@ import java.util.concurrent.atomic.AtomicReference import kotlin.test.assertEquals import kotlin.test.assertFailsWith import kotlin.test.assertFalse +import kotlin.test.assertNull import kotlin.test.assertTrue /** @@ -30,8 +37,19 @@ import kotlin.test.assertTrue */ class GraphSenderSendTest { + @Before + fun setUp() { + // send() now breadcrumbs through AppLog on the oversized-attachment guard; android.util.Log is a + // no-op stub under plain JVM tests, so mock it (fully qualified, so this file never imports it). + mockkStatic(android.util.Log::class) + every { android.util.Log.w(any(), any()) } returns 0 + } + @After - fun tearDown() = armed.set(null) + fun tearDown() { + armed.set(null) + unmockkAll() + } private val message = OutgoingMessage(accountId = "outlook:me@x.com", to = "bob@example.org", subject = "Hi", body = "Body") @@ -84,6 +102,26 @@ class GraphSenderSendTest { assertFalse(ex.mayHaveSent, "the request never reached Graph, so a retry is safe") } + @Test + fun `an oversized attachment fails safe-to-fall-back before opening a connection`() = runTest { + val last = arm() // armed, but the guard must trip before any connection is opened + val big = File.createTempFile("graph-big", ".bin") + try { + // 4 MiB, over the 3 MiB per-file cap. setLength allocates the size without writing the bytes, + // so the guard (which reads file.length()) trips without the test materializing 4 MiB. + RandomAccessFile(big, "rw").use { it.setLength(4L * 1024 * 1024) } + + val ex = assertFailsWith { + GraphSender().send("token", message, listOf(SendableAttachment(big))) + } + + assertFalse(ex.mayHaveSent, "oversized never reached Graph, so SMTP fallback is safe") + assertNull(last.get(), "the guard must trip before any connection is opened") + } finally { + big.delete() + } + } + @Test fun `GraphSendException carries its message, flag and cause`() { val cause = IOException("boom") diff --git a/app/src/test/kotlin/org/libremail/reporting/DiagnosticsCollectorTest.kt b/app/src/test/kotlin/org/libremail/reporting/DiagnosticsCollectorTest.kt index 859d2fa..2f75273 100644 --- a/app/src/test/kotlin/org/libremail/reporting/DiagnosticsCollectorTest.kt +++ b/app/src/test/kotlin/org/libremail/reporting/DiagnosticsCollectorTest.kt @@ -169,6 +169,26 @@ class DiagnosticsCollectorTest { ) } + @Test + fun `provider label matches brand tokens at label boundaries, not as substrings`() = runTest { + every { settingsRepository.settings } returns flowOf(AppSettings()) + // Each custom host merely CONTAINS a brand name inside a longer DNS label; the old substring match + // mislabeled them (notgmail→Gmail, yahooligans→Yahoo, me.company→iCloud, kaolin→AOL). They must all + // bucket to "Other" now (#298). + every { accountRepository.observeAccounts() } returns flowOf( + listOf( + account("1@x", AuthType.PASSWORD_IMAP, "mail.notgmail.example"), + account("2@x", AuthType.PASSWORD_IMAP, "imap.yahooligans.example"), + account("3@x", AuthType.PASSWORD_IMAP, "me.company.example"), + account("4@x", AuthType.PASSWORD_IMAP, "kaolin.example"), + ), + ) + + val report = collector.collectManual() + + assertEquals(List(4) { "Other (PASSWORD_IMAP)" }, report.accounts) + } + private fun account(email: String, authType: AuthType, imapHost: String) = Account( id = "id:$email", email = email, diff --git a/app/src/test/kotlin/org/libremail/reporting/ReportStoreTest.kt b/app/src/test/kotlin/org/libremail/reporting/ReportStoreTest.kt index 9becacd..9f9c885 100644 --- a/app/src/test/kotlin/org/libremail/reporting/ReportStoreTest.kt +++ b/app/src/test/kotlin/org/libremail/reporting/ReportStoreTest.kt @@ -111,6 +111,29 @@ class ReportStoreTest { assertEquals(listOf("valid"), store.reports.value.map { it.id }) } + @Test + fun `save leaves no temporary file behind (atomic write renames it into place)`() { + val store = newStore() + + store.save(report("a")) + + // The write-and-rename temp must not linger: only the final ".json" remains on disk (#298). + assertEquals(listOf("a.json"), tempFolder.root.listFiles()?.map { it.name }.orEmpty()) + } + + @Test + fun `a stray temp file from an interrupted write is never scanned as a report`() { + // A process death mid-write leaves a ".json.tmp" file, never a torn ".json". scan() filters on + // ".json", so the orphan is ignored and a valid report saved alongside still lists cleanly — the + // old in-place write could instead leave a truncated ".json" that scan() silently dropped (#298). + File(tempFolder.root, "torn.json.tmp").writeText("{ half-written") + val store = newStore() + + store.save(report("valid")) + + assertEquals(listOf("valid"), store.reports.value.map { it.id }) + } + @Test fun `purgeOlderThan deletes reports strictly older than the cutoff`() { val store = newStore() diff --git a/app/src/test/kotlin/org/libremail/richtext/RichTextEditingTest.kt b/app/src/test/kotlin/org/libremail/richtext/RichTextEditingTest.kt index 52553e4..e7707e1 100644 --- a/app/src/test/kotlin/org/libremail/richtext/RichTextEditingTest.kt +++ b/app/src/test/kotlin/org/libremail/richtext/RichTextEditingTest.kt @@ -301,18 +301,53 @@ class RichTextEditingTest { } @Test - fun `applyLink keeps links wholly outside the range and replaces overlapping ones`() { + fun `applyLink keeps links outside the range and splits partial overlaps, keeping the remainder`() { val content = RichTextContent( "0123456789", links = listOf(RichLink(0, 2, "a"), RichLink(3, 6, "b"), RichLink(7, 9, "c")), ) val result = RichTextEditing.applyLink(content, 4, 7, "http://new") assertEquals( - listOf(RichLink(0, 2, "a"), RichLink(4, 7, "http://new"), RichLink(7, 9, "c")), + // "a" is wholly outside; "b" (3,6) overlaps [4,7) so only its (3,4) remainder survives (it is + // no longer dropped whole); the new link takes [4,7); "c" starts at the range end, kept whole. + listOf(RichLink(0, 2, "a"), RichLink(3, 4, "b"), RichLink(4, 7, "http://new"), RichLink(7, 9, "c")), result.links.sortedBy { it.start }, ) } + @Test + fun `applyLink over the middle of a link relinks the middle and keeps both surrounding remainders`() { + val content = RichTextContent("0123456789", links = listOf(RichLink(0, 8, "old"))) + val result = RichTextEditing.applyLink(content, 3, 5, "new") + assertEquals( + listOf(RichLink(0, 3, "old"), RichLink(3, 5, "new"), RichLink(5, 8, "old")), + result.links.sortedBy { it.start }, + ) + } + + // --- mergeSameValueSpans (shared merge used by toggleStyle and the HTML parser) --- + + @Test + fun `mergeSameValueSpans coalesces touching and overlapping runs of the same value only`() { + val merged = mergeSameValueSpans( + listOf( + RichSpan(5, 8, RichStyle.Bold), // out of order, and overlaps the (3,6) run below + RichSpan(0, 3, RichStyle.Bold), + RichSpan(3, 6, RichStyle.Bold), // touches (0,3) and overlaps (5,8) → one 0..8 run + RichSpan(0, 4, RichStyle.Italic), // a different value never folds into the Bold run + RichSpan(10, 12, RichStyle.Bold), // a gap breaks the run into a fresh one + ), + ) + assertEquals( + listOf( + RichSpan(0, 8, RichStyle.Bold), + RichSpan(0, 4, RichStyle.Italic), + RichSpan(10, 12, RichStyle.Bold), + ), + merged, + ) + } + // --- styleAt / isStyled caret edges --- @Test -- 2.47.3