Merge branch 'main' into feat-390-imap-disabled-detection

2026-07-08 05:23:50 +00:00
parent 8f7926886c 63c0d5f9f9
commit d6665fe8c3
8 changed files with 261 additions and 6 deletions
@@ -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<MessageDao>(relaxed = true),
+            folderDao = mockk<FolderDao>(relaxed = true),
+            backfillProgressDao = mockk<BackfillProgressDao>(relaxed = true),
+            draftDao = mockk<DraftDao>(relaxed = true),
+            credentialStore = credentialStore,
+            imapClient = imapClient,
+            syncScheduler = mockk<SyncScheduler>(relaxed = true),
+            accountSettingsRepository = mockk<AccountSettingsRepository>(relaxed = true),
+            mailNotifier = mockk<MailNotifier>(relaxed = true),
+            attachmentUriGrants = mockk<AttachmentUriGrants>(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<String?>()
+        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
+    }
+}
@@ -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<List<String>> = 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<List<String>> = 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"
+    }
 }
@@ -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)
@@ -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")
@@ -32,6 +32,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
@@ -315,6 +316,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)
@@ -346,6 +357,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
@@ -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")
@@ -80,7 +80,9 @@ class MailConnectionFactoryTest {
    fun `a missing password credential is a hard error`() = runTest {
        coEvery { credentialStore.loadSecret("acct") } returns null

-        assertFailsWith<IllegalStateException> { 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<MissingCredentialsException> { 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<IllegalStateException> { factory().graphTokenFor(outlookAccount) }
+        assertFailsWith<MissingCredentialsException> { factory().graphTokenFor(outlookAccount) }
    }
 }
@@ -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: