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 f2f3722..249b501 100644 --- a/app/src/main/kotlin/org/libremail/push/IdleService.kt +++ b/app/src/main/kotlin/org/libremail/push/IdleService.kt @@ -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 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: