diff --git a/.github/workflows/autoupdate.yml b/.github/workflows/autoupdate.yml new file mode 100644 index 0000000..855f3ee --- /dev/null +++ b/.github/workflows/autoupdate.yml @@ -0,0 +1,39 @@ +# SPDX-License-Identifier: GPL-3.0-or-later +name: Auto-update PR branches + +# When main advances, rebase any auto-merge-armed PR that has fallen behind, so the +# "require branches up to date" branch rule doesn't need manual branch updates. Only PRs +# with GitHub auto-merge enabled are touched (PR_FILTER: auto_merge) — held/draft PRs are +# left alone. +# +# IMPORTANT: for the branch update to RE-TRIGGER the PR's CI (so it can pass and merge), +# this must run with a PAT, not the default GITHUB_TOKEN — pushes made by GITHUB_TOKEN do +# not start new workflow runs (GitHub's anti-recursion rule), so the updated PR would sit +# with stale checks. Create a fine-grained PAT scoped to this repo with +# contents:read/write + pull-requests:read/write and add it as the AUTOUPDATE_TOKEN secret. +# Without it this falls back to GITHUB_TOKEN, which updates the branch but will NOT re-run +# the PR's checks. + +on: + push: + branches: [main] + +permissions: + contents: write + pull-requests: write + +concurrency: + group: autoupdate-${{ github.ref }} + cancel-in-progress: true + +jobs: + autoupdate: + name: Auto-update armed PRs + runs-on: ubuntu-latest + steps: + - name: Update behind PRs that have auto-merge enabled + uses: chinthakagodawita/autoupdate@0707656cd062a3b0cf8fa9b2cda1d1404d74437e # v1.7.0 + env: + GITHUB_TOKEN: ${{ secrets.AUTOUPDATE_TOKEN || secrets.GITHUB_TOKEN }} + PR_FILTER: "auto_merge" + MERGE_CONFLICT_ACTION: "ignore" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 753e50f..825d7e1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -4,10 +4,6 @@ name: CI on: pull_request: branches: [main] - # Run the same jobs when a PR is queued in the GitHub merge queue, so the "CI passed" - # gate reports on the up-to-date merge-group ref and the queue can merge in order. - merge_group: - branches: [main] # A new push to a PR cancels any in-flight run for that PR. concurrency: diff --git a/app/schemas/org.libremail.data.local.LibreMailDatabase/17.json b/app/schemas/org.libremail.data.local.LibreMailDatabase/17.json new file mode 100644 index 0000000..c1c9dbe --- /dev/null +++ b/app/schemas/org.libremail.data.local.LibreMailDatabase/17.json @@ -0,0 +1,460 @@ +{ + "formatVersion": 1, + "database": { + "version": 17, + "identityHash": "6fbe947ef0c6133ba5e621251a00fa1b", + "entities": [ + { + "tableName": "messages", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `accountId` TEXT NOT NULL, `sender` TEXT NOT NULL, `senderEmail` TEXT NOT NULL, `subject` TEXT NOT NULL, `snippet` TEXT NOT NULL, `body` TEXT NOT NULL, `isHtml` INTEGER NOT NULL, `timestampMillis` INTEGER NOT NULL, `isRead` INTEGER NOT NULL, `isStarred` INTEGER NOT NULL, `folder` TEXT NOT NULL DEFAULT 'INBOX', `inInbox` INTEGER NOT NULL, `bodyFetched` INTEGER NOT NULL, `uid` INTEGER NOT NULL DEFAULT 0, PRIMARY KEY(`id`))", + "fields": [ + { + "fieldPath": "id", + "columnName": "id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "accountId", + "columnName": "accountId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "sender", + "columnName": "sender", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "senderEmail", + "columnName": "senderEmail", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "subject", + "columnName": "subject", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "snippet", + "columnName": "snippet", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "body", + "columnName": "body", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "isHtml", + "columnName": "isHtml", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "timestampMillis", + "columnName": "timestampMillis", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "isRead", + "columnName": "isRead", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "isStarred", + "columnName": "isStarred", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "folder", + "columnName": "folder", + "affinity": "TEXT", + "notNull": true, + "defaultValue": "'INBOX'" + }, + { + "fieldPath": "inInbox", + "columnName": "inInbox", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "bodyFetched", + "columnName": "bodyFetched", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "uid", + "columnName": "uid", + "affinity": "INTEGER", + "notNull": true, + "defaultValue": "0" + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "id" + ] + }, + "indices": [ + { + "name": "index_messages_accountId", + "unique": false, + "columnNames": [ + "accountId" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_messages_accountId` ON `${TABLE_NAME}` (`accountId`)" + }, + { + "name": "index_messages_timestampMillis", + "unique": false, + "columnNames": [ + "timestampMillis" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_messages_timestampMillis` ON `${TABLE_NAME}` (`timestampMillis`)" + }, + { + "name": "index_messages_accountId_folder_uid", + "unique": false, + "columnNames": [ + "accountId", + "folder", + "uid" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_messages_accountId_folder_uid` ON `${TABLE_NAME}` (`accountId`, `folder`, `uid`)" + } + ] + }, + { + "tableName": "attachments", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`messageId` TEXT NOT NULL, `partIndex` INTEGER NOT NULL, `filename` TEXT NOT NULL, `mimeType` TEXT NOT NULL, `sizeBytes` INTEGER NOT NULL, `contentId` TEXT, PRIMARY KEY(`messageId`, `partIndex`), FOREIGN KEY(`messageId`) REFERENCES `messages`(`id`) ON UPDATE NO ACTION ON DELETE CASCADE )", + "fields": [ + { + "fieldPath": "messageId", + "columnName": "messageId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "partIndex", + "columnName": "partIndex", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "filename", + "columnName": "filename", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "mimeType", + "columnName": "mimeType", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "sizeBytes", + "columnName": "sizeBytes", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "contentId", + "columnName": "contentId", + "affinity": "TEXT" + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "messageId", + "partIndex" + ] + }, + "indices": [ + { + "name": "index_attachments_messageId", + "unique": false, + "columnNames": [ + "messageId" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_attachments_messageId` ON `${TABLE_NAME}` (`messageId`)" + } + ], + "foreignKeys": [ + { + "table": "messages", + "onDelete": "CASCADE", + "onUpdate": "NO ACTION", + "columns": [ + "messageId" + ], + "referencedColumns": [ + "id" + ] + } + ] + }, + { + "tableName": "outbox", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `accountId` TEXT NOT NULL, `toAddresses` TEXT NOT NULL, `ccAddresses` TEXT NOT NULL, `bccAddresses` TEXT NOT NULL DEFAULT '', `subject` TEXT NOT NULL, `body` TEXT NOT NULL, `createdAt` INTEGER NOT NULL, `lastError` TEXT, `bodyHtml` TEXT, PRIMARY KEY(`id`))", + "fields": [ + { + "fieldPath": "id", + "columnName": "id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "accountId", + "columnName": "accountId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "toAddresses", + "columnName": "toAddresses", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "ccAddresses", + "columnName": "ccAddresses", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "bccAddresses", + "columnName": "bccAddresses", + "affinity": "TEXT", + "notNull": true, + "defaultValue": "''" + }, + { + "fieldPath": "subject", + "columnName": "subject", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "body", + "columnName": "body", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "createdAt", + "columnName": "createdAt", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "lastError", + "columnName": "lastError", + "affinity": "TEXT" + }, + { + "fieldPath": "bodyHtml", + "columnName": "bodyHtml", + "affinity": "TEXT" + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "id" + ] + } + }, + { + "tableName": "drafts", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `accountId` TEXT, `toAddresses` TEXT NOT NULL, `ccAddresses` TEXT NOT NULL, `bccAddresses` TEXT NOT NULL DEFAULT '', `subject` TEXT NOT NULL, `body` TEXT NOT NULL, `updatedAt` INTEGER NOT NULL, `attachments` TEXT NOT NULL, `bodyHtml` TEXT, PRIMARY KEY(`id`))", + "fields": [ + { + "fieldPath": "id", + "columnName": "id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "accountId", + "columnName": "accountId", + "affinity": "TEXT" + }, + { + "fieldPath": "toAddresses", + "columnName": "toAddresses", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "ccAddresses", + "columnName": "ccAddresses", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "bccAddresses", + "columnName": "bccAddresses", + "affinity": "TEXT", + "notNull": true, + "defaultValue": "''" + }, + { + "fieldPath": "subject", + "columnName": "subject", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "body", + "columnName": "body", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "updatedAt", + "columnName": "updatedAt", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "attachments", + "columnName": "attachments", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "bodyHtml", + "columnName": "bodyHtml", + "affinity": "TEXT" + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "id" + ] + } + }, + { + "tableName": "folders", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`accountId` TEXT NOT NULL, `fullName` TEXT NOT NULL, `displayName` TEXT NOT NULL, `role` TEXT NOT NULL, `selectable` INTEGER NOT NULL, `sortOrder` INTEGER NOT NULL, `specialUse` INTEGER NOT NULL DEFAULT 0, `hierarchyDelimiter` TEXT, PRIMARY KEY(`accountId`, `fullName`))", + "fields": [ + { + "fieldPath": "accountId", + "columnName": "accountId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "fullName", + "columnName": "fullName", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "displayName", + "columnName": "displayName", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "role", + "columnName": "role", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "selectable", + "columnName": "selectable", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "sortOrder", + "columnName": "sortOrder", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "specialUse", + "columnName": "specialUse", + "affinity": "INTEGER", + "notNull": true, + "defaultValue": "0" + }, + { + "fieldPath": "hierarchyDelimiter", + "columnName": "hierarchyDelimiter", + "affinity": "TEXT" + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "accountId", + "fullName" + ] + } + }, + { + "tableName": "backfill_progress", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`accountId` TEXT NOT NULL, `folder` TEXT NOT NULL, `nextBeforeUid` INTEGER NOT NULL, `complete` INTEGER NOT NULL, PRIMARY KEY(`accountId`, `folder`))", + "fields": [ + { + "fieldPath": "accountId", + "columnName": "accountId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "folder", + "columnName": "folder", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "nextBeforeUid", + "columnName": "nextBeforeUid", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "complete", + "columnName": "complete", + "affinity": "INTEGER", + "notNull": true + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "accountId", + "folder" + ] + } + } + ], + "setupQueries": [ + "CREATE TABLE IF NOT EXISTS room_master_table (id INTEGER PRIMARY KEY,identity_hash TEXT)", + "INSERT OR REPLACE INTO room_master_table (id,identity_hash) VALUES(42, '6fbe947ef0c6133ba5e621251a00fa1b')" + ] + } +} \ No newline at end of file diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/LibreMailDatabaseTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/LibreMailDatabaseTest.kt index 5879ba1..31bbde3 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/LibreMailDatabaseTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/LibreMailDatabaseTest.kt @@ -96,6 +96,25 @@ class LibreMailDatabaseTest { ) } + @Test + fun observeForMessageHidesInlineImagesWhileGetForMessageKeepsThem() = runBlocking { + val messageDao = db.messageDao() + val attachmentDao = db.attachmentDao() + messageDao.insertNew(listOf(message("acct:1"))) + attachmentDao.insert( + listOf( + AttachmentEntity("acct:1", 0, "logo.png", "image/png", 4, contentId = "logo1"), + AttachmentEntity("acct:1", 1, "invoice.pdf", "application/pdf", 10, contentId = null), + ), + ) + + // The displayed list excludes inline cid: images (issue #133) ... + val displayed = attachmentDao.observeForMessage("acct:1").first() + assertEquals(listOf("invoice.pdf"), displayed.map { it.filename }) + // ... while the full read keeps them so their bytes can back a cid: request. + assertEquals(2, attachmentDao.getForMessage("acct:1").size) + } + @Test fun searchRowsAreNotInboxAndAreCleared() = runBlocking { val messageDao = db.messageDao() diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/MigrationTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/MigrationTest.kt index 1d669db..8a4ec5b 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/MigrationTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/MigrationTest.kt @@ -169,6 +169,37 @@ class MigrationTest { db.close() } + /** v16 -> v17 (issue #133): `attachments.contentId` appears defaulting to NULL; cached rows survive. */ + @Test + fun migrate16To17_addsNullContentIdToAttachments() { + helper.createDatabase(TEST_DB, 16).apply { + // v16 dropped the account tables, so a message (no FK to accounts) plus its attachment is + // all that's needed to exercise the attachments table rebuild. + execSQL( + "INSERT INTO messages (id, accountId, sender, senderEmail, subject, snippet, body, isHtml, " + + "timestampMillis, isRead, isStarred, folder, inInbox, bodyFetched, uid) VALUES " + + "('acct:INBOX:1', 'acct', 'Ada', 'ada@example.org', 'Hi', '', '', 0, 1000, 0, 0, " + + "'INBOX', 1, 1, 1)", + ) + execSQL( + "INSERT INTO attachments (messageId, partIndex, filename, mimeType, sizeBytes) " + + "VALUES ('acct:INBOX:1', 0, 'report.pdf', 'application/pdf', 2048)", + ) + close() + } + + val db = helper.runMigrationsAndValidate(TEST_DB, 17, true, MIGRATION_16_17) + + db.query("SELECT filename, contentId FROM attachments WHERE messageId = 'acct:INBOX:1'").use { c -> + assertTrue("the pre-upgrade attachment row must survive", c.moveToFirst()) + assertEquals("report.pdf", c.getString(0)) + assertTrue("existing attachments read a null contentId (treated as ordinary downloads)", c.isNull(1)) + assertFalse("only the one pre-upgrade attachment row must survive", c.moveToNext()) + } + assertEquals("the cached message must be untouched by 16->17", 1, db.count("messages")) + db.close() + } + /** The newest schema JSON exported to app/schemas (shipped to the test APK as assets). */ private fun latestExportedSchemaVersion(): Int { val schemaFolder = checkNotNull(LibreMailDatabase::class.java.canonicalName) diff --git a/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt b/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt index d9a4261..2454909 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt @@ -11,6 +11,7 @@ import org.libremail.domain.model.Attachment import org.libremail.domain.model.Draft import org.libremail.domain.model.Folder import org.libremail.domain.model.ImapConnectionParams +import org.libremail.domain.model.InlineImage import org.libremail.domain.model.Message import org.libremail.domain.model.OutboxMessage import org.libremail.domain.model.OutgoingMessage @@ -120,6 +121,8 @@ class FakeMailRepository( }, ) + override suspend fun inlineImages(messageId: String): List = emptyList() + override suspend fun downloadAttachment(messageId: String, partIndex: Int): Result = Result.failure(UnsupportedOperationException("not used in UI tests")) diff --git a/app/src/androidTest/kotlin/org/libremail/ui/compose/ComposeScreenTest.kt b/app/src/androidTest/kotlin/org/libremail/ui/compose/ComposeScreenTest.kt index bbd2bbf..662f1a8 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/compose/ComposeScreenTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/compose/ComposeScreenTest.kt @@ -67,8 +67,9 @@ class ComposeScreenTest { @Before fun grantContactsPermission() { - // ComposeScreen requests READ_CONTACTS on first composition; pre-grant it (before the test - // calls setContent) so no system permission dialog appears to block the headless run. + // ComposeScreen no longer requests READ_CONTACTS (the request moved to onboarding/#127); it + // only reads the current grant on resume. Pre-grant it (before setContent) so contactsAllowed + // resolves true and the autocomplete path stays exercised — no system dialog is ever shown. val instrumentation = InstrumentationRegistry.getInstrumentation() instrumentation.uiAutomation.grantRuntimePermission( instrumentation.targetContext.packageName, diff --git a/app/src/androidTest/kotlin/org/libremail/ui/onboarding/BatteryOptimizationStepTest.kt b/app/src/androidTest/kotlin/org/libremail/ui/onboarding/BatteryOptimizationStepTest.kt index a32112a..a0b0852 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/onboarding/BatteryOptimizationStepTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/onboarding/BatteryOptimizationStepTest.kt @@ -30,6 +30,7 @@ import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import org.libremail.R +import org.libremail.contacts.ContactsPermissionManager import org.libremail.data.settings.SettingsRepository import org.libremail.push.BatteryOptimizationManager import org.libremail.ui.navigation.Routes @@ -70,7 +71,11 @@ class BatteryOptimizationStepTest { val context = InstrumentationRegistry.getInstrumentation().targetContext.applicationContext settingsRepository = SettingsRepository(context) runBlocking { settingsRepository.setBatteryPromptHandled(handled) } - onboarding = OnboardingViewModel(BatteryOptimizationManager(context), settingsRepository) + onboarding = OnboardingViewModel( + BatteryOptimizationManager(context), + ContactsPermissionManager(context), + settingsRepository, + ) onboarding.onAccountAdded(FIRST_ACCOUNT_ID) composeTestRule.setContent { diff --git a/app/src/androidTest/kotlin/org/libremail/ui/onboarding/ContactsAccessStepTest.kt b/app/src/androidTest/kotlin/org/libremail/ui/onboarding/ContactsAccessStepTest.kt new file mode 100644 index 0000000..529a0f9 --- /dev/null +++ b/app/src/androidTest/kotlin/org/libremail/ui/onboarding/ContactsAccessStepTest.kt @@ -0,0 +1,97 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.ui.onboarding + +import androidx.activity.ComponentActivity +import androidx.compose.ui.test.assertIsDisplayed +import androidx.compose.ui.test.junit4.createAndroidComposeRule +import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.performClick +import androidx.test.ext.junit.runners.AndroidJUnit4 +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 + +/** + * UI tests for the onboarding contacts-access step (#127, #128). They drive the presentational + * [ContactsAccessContent] with explicit signals so the three paths — skip, grant (the "done" state), + * and request (with the re-ask rationale) — run deterministically without a live system permission + * dialog (whose grant state would otherwise leak across the shared instrumentation process). + */ +@RunWith(AndroidJUnit4::class) +class ContactsAccessStepTest { + + @get:Rule + val composeTestRule = createAndroidComposeRule() + + private fun string(resId: Int) = composeTestRule.activity.getString(resId) + + private fun setContent( + granted: Boolean, + showRationale: Boolean = false, + onAllow: () -> Unit = {}, + onSkip: () -> Unit = {}, + onContinue: () -> Unit = {}, + ) { + composeTestRule.setContent { + LibreMailTheme(darkTheme = false, dynamicColor = false) { + ContactsAccessContent( + granted = granted, + showRationale = showRationale, + onAllow = onAllow, + onSkip = onSkip, + onContinue = onContinue, + ) + } + } + } + + @Test + fun notGranted_notNow_skipsTheStep() { + var skipped = false + var allowed = false + setContent(granted = false, onAllow = { allowed = true }, onSkip = { skipped = true }) + + composeTestRule.onNodeWithText(string(R.string.onboarding_contacts_title)).assertIsDisplayed() + composeTestRule.onNodeWithText(string(R.string.onboarding_contacts_not_now)).performClick() + + assertTrue("Not now must invoke the skip callback", skipped) + assertFalse("Skipping must not request the permission", allowed) + } + + @Test + fun notGranted_allow_triggersTheRequest() { + var allowed = false + setContent(granted = false, onAllow = { allowed = true }) + + composeTestRule.onNodeWithText(string(R.string.onboarding_contacts_allow)).performClick() + + assertTrue("Allow must trigger the permission request", allowed) + } + + @Test + fun granted_showsDoneState_andContinues() { + var continued = false + setContent(granted = true, onContinue = { continued = true }) + + // The "done" copy is shown and the request/skip buttons are gone. + composeTestRule.onNodeWithText(string(R.string.onboarding_contacts_done_title)).assertIsDisplayed() + composeTestRule.onNodeWithText(string(R.string.onboarding_contacts_allow)).assertDoesNotExist() + composeTestRule.onNodeWithText(string(R.string.onboarding_contacts_not_now)).assertDoesNotExist() + + composeTestRule.onNodeWithText(string(R.string.onboarding_contacts_continue)).performClick() + assertTrue("Continue must invoke the continue callback", continued) + } + + @Test + fun reRequest_showsRationale() { + setContent(granted = false, showRationale = true) + + // A re-request explains itself (shouldShowRequestPermissionRationale handling, #128). + composeTestRule.onNodeWithText(string(R.string.onboarding_contacts_rationale)).assertIsDisplayed() + composeTestRule.onNodeWithText(string(R.string.onboarding_contacts_allow)).assertIsDisplayed() + } +} diff --git a/app/src/androidTest/kotlin/org/libremail/ui/onboarding/OnboardingFlowTest.kt b/app/src/androidTest/kotlin/org/libremail/ui/onboarding/OnboardingFlowTest.kt index 580543f..5308266 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/onboarding/OnboardingFlowTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/onboarding/OnboardingFlowTest.kt @@ -23,6 +23,7 @@ import org.junit.Test import org.junit.runner.RunWith import org.libremail.R import org.libremail.auth.OutlookAuthManager +import org.libremail.contacts.ContactsPermissionManager import org.libremail.data.settings.SettingsRepository import org.libremail.domain.model.Message import org.libremail.push.BatteryOptimizationManager @@ -85,6 +86,7 @@ class OnboardingFlowTest { val appContext = composeTestRule.activity.applicationContext val onboarding = OnboardingViewModel( BatteryOptimizationManager(appContext), + ContactsPermissionManager(appContext), SettingsRepository(appContext), ) composeTestRule.setContent { diff --git a/app/src/androidTest/kotlin/org/libremail/ui/settings/ContactAutocompleteRowTest.kt b/app/src/androidTest/kotlin/org/libremail/ui/settings/ContactAutocompleteRowTest.kt new file mode 100644 index 0000000..9d55899 --- /dev/null +++ b/app/src/androidTest/kotlin/org/libremail/ui/settings/ContactAutocompleteRowTest.kt @@ -0,0 +1,62 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.ui.settings + +import androidx.activity.ComponentActivity +import androidx.compose.ui.test.assertIsDisplayed +import androidx.compose.ui.test.junit4.createAndroidComposeRule +import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.performClick +import androidx.test.ext.junit.runners.AndroidJUnit4 +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.contacts.ContactPermissionState +import org.libremail.ui.theme.LibreMailTheme + +/** + * UI tests for the Settings contacts-autocomplete row (#129). The row is presentational, so each of + * its three states — on / off / blocked-in-settings — is driven directly and asserted deterministically, + * independent of the process's real `READ_CONTACTS` grant. + */ +@RunWith(AndroidJUnit4::class) +class ContactAutocompleteRowTest { + + @get:Rule + val composeTestRule = createAndroidComposeRule() + + private fun string(resId: Int) = composeTestRule.activity.getString(resId) + + private fun setContent(state: ContactPermissionState, onClick: () -> Unit = {}) { + composeTestRule.setContent { + LibreMailTheme(darkTheme = false, dynamicColor = false) { + ContactAutocompleteRow(state = state, onClick = onClick) + } + } + } + + @Test + fun granted_showsOnSubtitle_andIsClickable() { + var clicked = false + setContent(ContactPermissionState.GRANTED) { clicked = true } + + composeTestRule.onNodeWithText(string(R.string.settings_contacts_autocomplete)).assertIsDisplayed() + composeTestRule.onNodeWithText(string(R.string.settings_contacts_autocomplete_on)).assertIsDisplayed() + + composeTestRule.onNodeWithText(string(R.string.settings_contacts_autocomplete_on)).performClick() + assertTrue("Tapping the row must invoke onClick", clicked) + } + + @Test + fun denied_showsOffSubtitle() { + setContent(ContactPermissionState.DENIED) + composeTestRule.onNodeWithText(string(R.string.settings_contacts_autocomplete_off)).assertIsDisplayed() + } + + @Test + fun blocked_showsBlockedSubtitle() { + setContent(ContactPermissionState.BLOCKED) + composeTestRule.onNodeWithText(string(R.string.settings_contacts_autocomplete_blocked)).assertIsDisplayed() + } +} diff --git a/app/src/androidTest/kotlin/org/libremail/ui/settings/SettingsScreenTest.kt b/app/src/androidTest/kotlin/org/libremail/ui/settings/SettingsScreenTest.kt index 2d986be..e714e22 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/settings/SettingsScreenTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/settings/SettingsScreenTest.kt @@ -2,6 +2,7 @@ package org.libremail.ui.settings 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 @@ -15,6 +16,7 @@ import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import org.libremail.R +import org.libremail.contacts.ContactsPermissionManager import org.libremail.data.security.AppLockManager import org.libremail.data.security.DatabaseKeyCipher import org.libremail.data.security.DatabaseKeyStore @@ -57,6 +59,7 @@ class SettingsScreenTest { insecureDevice, keyStore, BatteryOptimizationManager(context), + ContactsPermissionManager(context), SyncScheduler(Provider { WorkManager.getInstance(context) }), ) } @@ -88,6 +91,16 @@ class SettingsScreenTest { } } + @Test + fun contactsAutocompleteRow_isShown() { + // The contacts entry (#129) is wired into the real screen; it reflects the live permission + // state, so we assert only that the row is present (state-specific rendering is covered by + // ContactAutocompleteRowTest). + setContent(settingsViewModel(SettingsRepository(context))) + composeTestRule.onNodeWithText(string(R.string.settings_contacts_autocomplete)) + .performScrollTo().assertIsDisplayed() + } + @Test fun enablingAppLockWithoutSecureDevice_showsRejectionSnackbar() { val settingsRepository = SettingsRepository(context) diff --git a/app/src/main/AndroidManifest.xml b/app/src/main/AndroidManifest.xml index 5d35ca1..07ca905 100644 --- a/app/src/main/AndroidManifest.xml +++ b/app/src/main/AndroidManifest.xml @@ -91,6 +91,19 @@ android:exported="false" android:foregroundServiceType="dataSync" /> + + + = listOf( - "libremail.db", - "libremail.db-wal", - "libremail.db-shm", - "libremail.db-journal", - ) + /** + * `databases`-dir-relative names that must never leave the device: the encrypted mail cache + * ([DatabaseFiles.NAME]) AND the accounts + encrypted-credentials database + * ([DatabaseFiles.ACCOUNTS_NAME]), each with its SQLite sidecars. Derived from [DatabaseFiles] + * rather than hand-listed, so a newly added database can never silently fall out of the + * never-back-up set (issue #103). + */ + val EXCLUDED_DATABASE_PATHS: List = + DatabaseFiles.fileNames(DatabaseFiles.NAME) + + DatabaseFiles.fileNames(DatabaseFiles.ACCOUNTS_NAME) /** * Whether Android Backup may run for this app. Opt-in and OFF by default: nothing is backed up diff --git a/app/src/main/kotlin/org/libremail/contacts/ContactPermissionState.kt b/app/src/main/kotlin/org/libremail/contacts/ContactPermissionState.kt new file mode 100644 index 0000000..8f6a324 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/contacts/ContactPermissionState.kt @@ -0,0 +1,37 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.contacts + +/** + * Where the optional contacts-autocomplete permission stands, as the Settings entry (#129) shows it. + * - [GRANTED]: on — recipient autocomplete works. + * - [DENIED]: off but re-requestable in-app (never asked, or denied once without "don't ask again"). + * - [BLOCKED]: off and no longer re-requestable — the only way back is the system settings screen. + */ +enum class ContactPermissionState { GRANTED, DENIED, BLOCKED } + +/** + * Pure mapping from the three Android permission signals to a [ContactPermissionState]. Kept free of + * Android types so it is exhaustively unit-testable; the live inputs are read by + * [ContactsPermissionManager] (grant), the Activity (`shouldShowRequestPermissionRationale`), and + * [org.libremail.data.settings.SettingsRepository] (whether the system dialog has ever been shown). + */ +object ContactPermissionDecision { + + /** + * Resolve the current state: + * - [granted]: `READ_CONTACTS` is held → [ContactPermissionState.GRANTED]. + * - [showRationale]: the OS says a rationale should precede a re-request, i.e. the user denied + * once without "don't ask again" → still re-requestable, [ContactPermissionState.DENIED]. + * - [alreadyRequested]: the system dialog has been shown before. Combined with `!showRationale` + * (and not granted) this is the permanently-denied case → [ContactPermissionState.BLOCKED]. + * + * The remaining case — not granted, no rationale, never requested — is a fresh install that has + * simply never asked, so an in-app request will still surface the dialog: [ContactPermissionState.DENIED]. + */ + fun resolve(granted: Boolean, showRationale: Boolean, alreadyRequested: Boolean): ContactPermissionState = when { + granted -> ContactPermissionState.GRANTED + showRationale -> ContactPermissionState.DENIED + alreadyRequested -> ContactPermissionState.BLOCKED + else -> ContactPermissionState.DENIED + } +} diff --git a/app/src/main/kotlin/org/libremail/contacts/ContactsPermissionManager.kt b/app/src/main/kotlin/org/libremail/contacts/ContactsPermissionManager.kt new file mode 100644 index 0000000..49a447f --- /dev/null +++ b/app/src/main/kotlin/org/libremail/contacts/ContactsPermissionManager.kt @@ -0,0 +1,39 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.contacts + +import android.Manifest +import android.content.Context +import android.content.Intent +import android.content.pm.PackageManager +import android.net.Uri +import android.provider.Settings +import androidx.core.content.ContextCompat +import dagger.hilt.android.qualifiers.ApplicationContext +import javax.inject.Inject +import javax.inject.Singleton + +/** + * Reads this app's `READ_CONTACTS` grant and deep-links to the system screen where it can be changed. + * `READ_CONTACTS` powers recipient autocomplete only (see [ContactsRepository]); the whole feature is + * optional and degrades gracefully when the permission is absent. + * + * Deliberately Context-only so it can back both the onboarding opt-in step and the Settings entry. + * The `shouldShowRequestPermissionRationale` signal needs an Activity, so it is read in the Compose + * layer and combined with [ContactPermissionDecision]; this manager stays free of Activity state. + */ +@Singleton +class ContactsPermissionManager @Inject constructor(@ApplicationContext private val context: Context) { + /** True when `READ_CONTACTS` is currently granted to this app. */ + fun hasPermission(): Boolean = ContextCompat.checkSelfPermission(context, Manifest.permission.READ_CONTACTS) == + PackageManager.PERMISSION_GRANTED + + /** + * Intent to this app's system details screen, where **Permissions → Contacts** can be toggled. + * Used to recover the permanently-denied ("Don't allow" / don't-ask-again) case, which can no + * longer be re-requested in-app. Always resolvable since API 9. + */ + fun settingsIntent(): Intent = Intent( + Settings.ACTION_APPLICATION_DETAILS_SETTINGS, + Uri.fromParts("package", context.packageName, null), + ) +} diff --git a/app/src/main/kotlin/org/libremail/data/local/DatabaseFiles.kt b/app/src/main/kotlin/org/libremail/data/local/DatabaseFiles.kt index 4e52837..ab85e54 100644 --- a/app/src/main/kotlin/org/libremail/data/local/DatabaseFiles.kt +++ b/app/src/main/kotlin/org/libremail/data/local/DatabaseFiles.kt @@ -2,9 +2,8 @@ package org.libremail.data.local import android.content.Context -import java.io.File -/** Central name and wipe helper for the Room cache database file (and its SQLite sidecars). */ +/** Central names and wipe helper for the Room database files (and their SQLite sidecars). */ object DatabaseFiles { const val NAME = "libremail.db" @@ -17,15 +16,27 @@ object DatabaseFiles { const val ACCOUNTS_NAME = "libremail-accounts.db" /** - * Delete the database and any WAL/SHM/journal sidecars. Call only when no connection is open — - * used by the "clear + re-sync" path when the encryption key is invalidated and the encrypted - * database can no longer be decrypted. + * The statically-nameable SQLite sidecars that accompany a database file. A transient `-mj*` + * master journal can also exist, but its suffix is random and so can't be listed by name — + * [clear] leans on [Context.deleteDatabase] to sweep that one up. + */ + private val SIDECAR_SUFFIXES = listOf("-wal", "-shm", "-journal") + + /** + * [name] plus each of its statically-nameable sidecars. The single source of truth for which + * on-disk files make up a database file; `BackupPolicy` derives its never-back-up set from this + * so a new database (or a new sidecar suffix) can never silently fall out of the exclusions. + */ + fun fileNames(name: String): List = listOf(name) + SIDECAR_SUFFIXES.map { name + it } + + /** + * Delete the cache database ([NAME]) and every sidecar — including the `-mj*` master journal a + * hand-rolled suffix list would miss — via [Context.deleteDatabase]. NEVER touches + * [ACCOUNTS_NAME], so a cache-key invalidation keeps the user signed in (issue #111). Call only + * when no connection is open — used by the "clear + re-sync" path when the encryption key is + * invalidated and the encrypted database can no longer be decrypted. */ fun clear(context: Context) { - val db = context.getDatabasePath(NAME) - val dir = db.parentFile ?: return - listOf("", "-wal", "-shm", "-journal").forEach { suffix -> - File(dir, db.name + suffix).delete() - } + context.deleteDatabase(NAME) } } diff --git a/app/src/main/kotlin/org/libremail/data/local/DatabaseProvisioner.kt b/app/src/main/kotlin/org/libremail/data/local/DatabaseProvisioner.kt new file mode 100644 index 0000000..9015ce2 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/data/local/DatabaseProvisioner.kt @@ -0,0 +1,136 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.local + +import android.content.Context +import dagger.hilt.android.qualifiers.ApplicationContext +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock +import kotlinx.coroutines.withContext +import org.libremail.data.security.DatabaseKeyStore +import org.libremail.data.settings.SettingsRepository +import javax.inject.Inject +import javax.inject.Singleton + +/** How the cache database ([LibreMailDatabase]) must be opened, decided by [DatabaseProvisioner]. */ +sealed interface CacheOpenMode { + /** Open with SQLCipher, keyed by [passphrase] — the opt-in encrypted cache. */ + data class Encrypted(val passphrase: String) : CacheOpenMode + + /** Open with the default framework helper — the cache is plaintext on disk. */ + data object Plaintext : CacheOpenMode +} + +/** + * Runs the one-time, blocking startup sequence that must complete BEFORE Room opens either database — + * exactly once, memoized, and OFF the Hilt injection path (issue #93). + * + * `DatabaseModule.provideDatabase` used to do this work inline, with `runBlocking`, while Hilt + * constructed the singleton [LibreMailDatabase]: a DataStore read, a Keystore op, a possible SQLCipher + * re-key conversion, and (since #111) the cross-database [AccountDataMigrator]. All of it ran + * synchronously on whichever thread first injected the database — which can be the main thread — so the + * first DB access could jank or ANR (worst with the encrypted cache on). This class moves that work + * behind [prepareCache]; the Hilt providers wire it into a [DeferredOpenHelperFactory] so it runs + * lazily, on Room's background open, never at inject time. + * + * The sequence, its ordering, and its crash-safety are unchanged from the old `provideDatabase` — only + * WHERE and WHEN it runs moved: + * 1. If a screen-lock change flagged the encrypted cache for wiping, wipe it and reset its seals + * (before Room opens the file, so no open connection is deleted underneath it). + * 2. Run [AccountDataMigrator] — the one-time move of accounts/credentials/settings/signatures into + * the non-auth [AccountDatabase] (issue #111). MUST precede opening the cache (whose + * [MIGRATION_15_16] drops the moved tables) AND opening [AccountDatabase] (which reads the copied + * rows). Both databases' open paths gate on [prepareCache], so the migrate-before-open guarantee + * holds regardless of which database Room opens first. + * 3. Resolve the encryption gate: convert the on-disk cache to the form the `encryptCache` setting + * asks for, and report how the cache must be opened. + * + * [prepareCache] is memoized on success and guarded by a [Mutex], so the first database to open runs + * the sequence and any concurrent or later opener awaits the same result. A failure is NOT memoized, so + * it retries on the next open — preserving the migrator's "crash-loop rather than lose data" contract + * (a throw here means the cache never opens, so [MIGRATION_15_16] never drops the not-yet-copied rows). + */ +@Singleton +class DatabaseProvisioner internal constructor( + private val context: Context, + private val keyStore: DatabaseKeyStore, + private val settingsRepository: SettingsRepository, + private val accountDataMigrator: AccountDataMigrator, + private val ioDispatcher: CoroutineDispatcher, +) { + @Inject + constructor( + @ApplicationContext context: Context, + keyStore: DatabaseKeyStore, + settingsRepository: SettingsRepository, + accountDataMigrator: AccountDataMigrator, + ) : this(context, keyStore, settingsRepository, accountDataMigrator, Dispatchers.IO) + + private val mutex = Mutex() + + @Volatile + private var prepared: CacheOpenMode? = null + + /** + * Runs the startup sequence exactly once (on [ioDispatcher]) and returns how the cache must be + * opened. Idempotent and safe to call concurrently from both databases' open paths; the blocking + * work runs on [ioDispatcher], never on the caller's thread past the suspension point. + */ + suspend fun prepareCache(): CacheOpenMode { + prepared?.let { return it } + return mutex.withLock { + prepared ?: withContext(ioDispatcher) { runStartupSequence() }.also { prepared = it } + } + } + + private suspend fun runStartupSequence(): CacheOpenMode { + val dbFile = context.getDatabasePath(DatabaseFiles.NAME) + + // A screen-lock change (biometric re-enrollment / lock removal) can invalidate the auth-bound + // key so the encrypted cache is no longer decryptable. AppLockViewModel records that and + // restarts the app; we wipe the cache HERE — before Room opens it — so the file is never + // deleted from under an open connection. Crash-safe order: wipe + reset the seals, and only THEN + // clear the flag, so a kill mid-wipe just repeats the idempotent wipe next start. Only + // libremail.db is wiped: accounts/credentials live in AccountDatabase (a separate file), so the + // user stays signed in across the wipe (issue #111). + if (keyStore.isClearPending()) { + DatabaseFiles.clear(context) + keyStore.resetSealedPassphrase() + keyStore.clearClearPending() + } + + // One-time move of accounts/credentials/settings/signatures into the non-auth AccountDatabase + // (issue #111). MUST run before the cache opens: opening it applies MIGRATION_15_16, which drops + // the moved tables. Runs AFTER the wipe above so an unrecoverable-key cache is gone first + // (nothing left to move) and we never block waiting on a passphrase we can't get. + accountDataMigrator.migrateIfNeeded() + + // Opt-in at-rest encryption of the local cache (off by default). The conversion runs here — + // before the database is opened — so it never races an open connection; toggling the setting + // therefore takes effect on the next app start. The passphrase source is resolved from which + // seal actually exists (DatabaseKeyStore.resolvePassphrase), NOT from the app-lock setting (a + // separate DataStore that can disagree). When app-lock is ON the sealing key is auth-bound, so + // resolvePassphrase waits on PassphraseSession until the user authenticates — which is why this + // must never run on the main thread while the cache is locked (issue #93). + val settings = settingsRepository.settings.first() + val appLock = settings.appLock + return when { + settings.encryptCache -> { + val passphrase = keyStore.resolvePassphrase(appLock) + DatabaseEncryption.ensureEncrypted(dbFile, passphrase) + CacheOpenMode.Encrypted(passphrase) + } + + DatabaseEncryption.isEncrypted(dbFile) -> { + // Encryption was turned back off — decrypt so the default (unkeyed) open succeeds. + val passphrase = keyStore.resolvePassphrase(appLock) + DatabaseEncryption.ensurePlaintext(dbFile, passphrase) + CacheOpenMode.Plaintext + } + + else -> CacheOpenMode.Plaintext + } + } +} diff --git a/app/src/main/kotlin/org/libremail/data/local/DeferredOpenHelperFactory.kt b/app/src/main/kotlin/org/libremail/data/local/DeferredOpenHelperFactory.kt new file mode 100644 index 0000000..f4bed86 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/data/local/DeferredOpenHelperFactory.kt @@ -0,0 +1,75 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.local + +import androidx.sqlite.db.SupportSQLiteDatabase +import androidx.sqlite.db.SupportSQLiteOpenHelper + +/** + * A [SupportSQLiteOpenHelper.Factory] that defers building the REAL open helper — and any blocking work + * that choosing and creating it entails — from Room's build/inject path to the FIRST actual database + * open (issue #93). + * + * Room calls [create] and [SupportSQLiteOpenHelper.setWriteAheadLoggingEnabled] while it builds the + * database, on whichever thread injected it (possibly the main thread); neither may block. This factory + * hands back a thin handle whose delegate is materialised only when the database is first opened + * (`writableDatabase` / `readableDatabase`), which Room performs on its background query executor. The + * [buildDelegate] lambda is where the caller runs the startup gate (see [DatabaseProvisioner]) and + * picks the concrete factory — so all of that runs off the injection path and off the main thread. + */ +internal class DeferredOpenHelperFactory( + private val buildDelegate: (SupportSQLiteOpenHelper.Configuration) -> SupportSQLiteOpenHelper, +) : SupportSQLiteOpenHelper.Factory { + override fun create(configuration: SupportSQLiteOpenHelper.Configuration): SupportSQLiteOpenHelper = + DeferredOpenHelper(configuration, buildDelegate) +} + +/** + * The lazy handle returned by [DeferredOpenHelperFactory]. Everything Room touches before the first + * open is cheap; [buildDelegate] (which does the blocking work) runs only when [writableDatabase] or + * [readableDatabase] is first read. + */ +private class DeferredOpenHelper( + private val configuration: SupportSQLiteOpenHelper.Configuration, + private val buildDelegate: (SupportSQLiteOpenHelper.Configuration) -> SupportSQLiteOpenHelper, +) : SupportSQLiteOpenHelper { + + private val lock = Any() + + /** Guarded by [lock]. Null until the database is first opened — `create()` must stay non-blocking. */ + private var delegate: SupportSQLiteOpenHelper? = null + + /** + * Guarded by [lock]. Room may set WAL before the first open; we remember the value and apply it when + * the delegate is built, rather than building the delegate early (which would run the gate at inject + * time). Null means "Room never asked", so the delegate keeps the real factory's own default. + */ + private var writeAheadLoggingEnabled: Boolean? = null + + override val databaseName: String? + get() = configuration.name + + override fun setWriteAheadLoggingEnabled(enabled: Boolean) { + synchronized(lock) { + writeAheadLoggingEnabled = enabled + delegate?.setWriteAheadLoggingEnabled(enabled) + } + } + + override val writableDatabase: SupportSQLiteDatabase + get() = delegate().writableDatabase + + override val readableDatabase: SupportSQLiteDatabase + get() = delegate().readableDatabase + + override fun close() { + // Never opened means nothing to close; do NOT build the delegate just to close it. + synchronized(lock) { delegate?.close() } + } + + private fun delegate(): SupportSQLiteOpenHelper = synchronized(lock) { + delegate ?: buildDelegate(configuration).also { built -> + writeAheadLoggingEnabled?.let(built::setWriteAheadLoggingEnabled) + delegate = built + } + } +} diff --git a/app/src/main/kotlin/org/libremail/data/local/LibreMailDatabase.kt b/app/src/main/kotlin/org/libremail/data/local/LibreMailDatabase.kt index 567ad08..2494c04 100644 --- a/app/src/main/kotlin/org/libremail/data/local/LibreMailDatabase.kt +++ b/app/src/main/kotlin/org/libremail/data/local/LibreMailDatabase.kt @@ -35,7 +35,7 @@ import org.libremail.data.local.entity.OutboxEntity FolderEntity::class, BackfillProgressEntity::class, ], - version = 16, + version = 17, exportSchema = true, ) abstract class LibreMailDatabase : RoomDatabase() { diff --git a/app/src/main/kotlin/org/libremail/data/local/Mappers.kt b/app/src/main/kotlin/org/libremail/data/local/Mappers.kt index f2c0171..0854815 100644 --- a/app/src/main/kotlin/org/libremail/data/local/Mappers.kt +++ b/app/src/main/kotlin/org/libremail/data/local/Mappers.kt @@ -184,6 +184,7 @@ internal fun AttachmentEntity.toDomain(): Attachment = Attachment( filename = filename, mimeType = mimeType, sizeBytes = sizeBytes, + contentId = contentId, ) internal fun AttachmentPart.toEntity(messageId: String): AttachmentEntity = AttachmentEntity( @@ -192,6 +193,7 @@ internal fun AttachmentPart.toEntity(messageId: String): AttachmentEntity = Atta filename = filename, mimeType = mimeType, sizeBytes = sizeBytes, + contentId = contentId, ) internal fun DraftEntity.toDomain(): Draft = Draft( diff --git a/app/src/main/kotlin/org/libremail/data/local/Migrations.kt b/app/src/main/kotlin/org/libremail/data/local/Migrations.kt index feadf8a..566d0a4 100644 --- a/app/src/main/kotlin/org/libremail/data/local/Migrations.kt +++ b/app/src/main/kotlin/org/libremail/data/local/Migrations.kt @@ -330,3 +330,17 @@ val MIGRATION_15_16 = object : Migration(15, 16) { db.execSQL("DROP TABLE IF EXISTS `accounts`") } } + +/** + * v16 -> v17: inline-image support in the reader (issue #133; preserves existing data). Adds a + * nullable `contentId` column to `attachments` recording the `Content-ID` of an inline image + * (``) so the reader's WebView can resolve `cid:` requests to the cached bytes, + * and so such parts can be filtered out of the user-facing attachment list. Nullable with no SQL + * default (the MIGRATION_14_15 `hierarchyDelimiter` pattern) so existing attachment rows read back + * null — i.e. treated as ordinary attachments — until the next fetch reclassifies them. + */ +val MIGRATION_16_17 = object : Migration(16, 17) { + override fun migrate(db: SupportSQLiteDatabase) { + db.execSQL("ALTER TABLE `attachments` ADD COLUMN `contentId` TEXT") + } +} diff --git a/app/src/main/kotlin/org/libremail/data/local/dao/AttachmentDao.kt b/app/src/main/kotlin/org/libremail/data/local/dao/AttachmentDao.kt index db6d04a..4356db0 100644 --- a/app/src/main/kotlin/org/libremail/data/local/dao/AttachmentDao.kt +++ b/app/src/main/kotlin/org/libremail/data/local/dao/AttachmentDao.kt @@ -11,10 +11,18 @@ import org.libremail.data.local.entity.AttachmentEntity @Dao interface AttachmentDao { - @Query("SELECT * FROM attachments WHERE messageId = :messageId ORDER BY partIndex") + /** + * The message's user-facing attachments for the reader's attachment list. Inline images + * (`contentId IS NOT NULL`) are excluded — they render in the body via `cid:`, not as downloads + * (issue #133). + */ + @Query("SELECT * FROM attachments WHERE messageId = :messageId AND contentId IS NULL ORDER BY partIndex") fun observeForMessage(messageId: String): Flow> - /** One-shot read of a message's cached attachment metadata (e.g. to pre-download their bytes). */ + /** + * One-shot read of ALL of a message's cached parts — attachments AND inline images — e.g. to + * pre-download their bytes or resolve a `cid:` reference. The reader filters by [AttachmentEntity.contentId]. + */ @Query("SELECT * FROM attachments WHERE messageId = :messageId ORDER BY partIndex") suspend fun getForMessage(messageId: String): List diff --git a/app/src/main/kotlin/org/libremail/data/local/entity/AttachmentEntity.kt b/app/src/main/kotlin/org/libremail/data/local/entity/AttachmentEntity.kt index ecbc181..149a123 100644 --- a/app/src/main/kotlin/org/libremail/data/local/entity/AttachmentEntity.kt +++ b/app/src/main/kotlin/org/libremail/data/local/entity/AttachmentEntity.kt @@ -25,4 +25,10 @@ data class AttachmentEntity( val filename: String, val mimeType: String, val sizeBytes: Long, + /** + * The normalized `Content-ID` when this part is an inline image (``) — null for + * an ordinary attachment. Inline rows are cached so the reader's WebView can resolve `cid:` + * requests offline, but are filtered out of the displayed attachment list (issue #133). + */ + val contentId: String? = null, ) diff --git a/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt b/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt index bcfa1d8..d785cff 100644 --- a/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt +++ b/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt @@ -35,6 +35,7 @@ import org.libremail.domain.model.Draft import org.libremail.domain.model.Folder import org.libremail.domain.model.FolderRole import org.libremail.domain.model.ImapConnectionParams +import org.libremail.domain.model.InlineImage import org.libremail.domain.model.Message import org.libremail.domain.model.OutboxMessage import org.libremail.domain.model.OutgoingAttachment @@ -125,6 +126,15 @@ class MailRepositoryImpl @Inject constructor( rows.map { it.toDomain() } } + override suspend fun inlineImages(messageId: String): List = attachmentDao.getForMessage(messageId) + .filter { it.contentId != null } + .mapNotNull { row -> + // Reuse the on-disk attachment cache (download once, then instant + offline). A failed + // fetch just omits that image, leaving a broken rather than failing the open. + val file = downloadAttachment(messageId, row.partIndex).getOrNull() ?: return@mapNotNull null + InlineImage(contentId = row.contentId!!, mimeType = row.mimeType, bytes = file.readBytes()) + } + override suspend fun downloadAttachment(messageId: String, partIndex: Int): Result = runCatching { val entity = messageDao.getById(messageId) ?: error("Message not found") val meta = attachmentDao.getForMessage(messageId).firstOrNull { it.partIndex == partIndex } diff --git a/app/src/main/kotlin/org/libremail/data/security/AesGcmKeystoreCipher.kt b/app/src/main/kotlin/org/libremail/data/security/AesGcmKeystoreCipher.kt new file mode 100644 index 0000000..be37397 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/data/security/AesGcmKeystoreCipher.kt @@ -0,0 +1,143 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.security + +import android.security.keystore.KeyGenParameterSpec +import android.security.keystore.KeyProperties +import android.util.Base64 +import java.security.GeneralSecurityException +import java.security.KeyStore +import javax.crypto.AEADBadTagException +import javax.crypto.Cipher +import javax.crypto.KeyGenerator +import javax.crypto.SecretKey +import javax.crypto.spec.GCMParameterSpec + +/** + * Shared AES-256-GCM plumbing backed by a non-exportable key in the Android Keystore, parameterized + * by key [alias] and a missing-key-on-decrypt policy ([generateKeyOnDecrypt]). + * + * Encapsulates everything the two Keystore users have in common — the GCM transform and its + * constants, the alias-scoped key lookup/creation guarded by a lock, `Base64(iv || ciphertext)` + * framing, and key deletion — so a change to the crypto (StrongBox opt-in, IV handling, error + * mapping) is made in ONE place instead of being copy-pasted and drifting. Subclasses supply only + * their delta: the [keySpec] that mints the key (extend [keySpecBuilder] for the common + * AES-256-GCM base) and, via [generateKeyOnDecrypt], how a decrypt behaves when the alias is absent. + * + * That missing-key policy is **deliberately different** between the two users and MUST stay + * different: + * - The non-auth master key ([KeystoreCrypto], `generateKeyOnDecrypt = true`) silently generates a + * key on a missing alias — correct for a first-run master key that has nothing sealed yet. + * - The auth-bound cache key ([DatabaseKeyCipher], `generateKeyOnDecrypt = false`) fails fast, + * because a MISSING auth-bound key means it was INVALIDATED (biometric re-enrollment or lock + * removal). Silently regenerating it would defeat the security model — quietly re-arming a lock + * against a cache that can no longer be decrypted — so the absence must surface, not self-heal. + * + * The key operations touch the Android Keystore, so the two concrete ciphers are exercised on-device; + * the alias/policy wiring above is unit-tested against this base with fake keys (see the test seams + * [existingKey], [getOrCreateKey], and [decryptWithKey]). + */ +abstract class AesGcmKeystoreCipher(private val alias: String, private val generateKeyOnDecrypt: Boolean) { + + private val keyLock = Any() + + /** Returns `Base64(iv || ciphertext)`, generating the key under [alias] on first use. */ + open fun encrypt(plaintext: String): String = doEncrypt(getOrCreateKey(), plaintext) + + /** + * Decrypts a blob produced by [encrypt]. Missing-key handling follows [generateKeyOnDecrypt] + * (see the class KDoc). An AES-GCM tag mismatch — the ciphertext no longer matches the key, e.g. + * the alias was cleared and regenerated underneath a still-persisted blob — is surfaced as a + * clear [GeneralSecurityException] instead of an opaque [AEADBadTagException]. A key-invalidation + * failure ([android.security.keystore.KeyPermanentlyInvalidatedException]) is thrown from + * `Cipher.init` and propagates unwrapped, so callers can still classify it. + */ + fun decrypt(encoded: String): String { + val key = decryptionKey() + return try { + decryptWithKey(key, encoded) + } catch (e: AEADBadTagException) { + throw GeneralSecurityException( + "AES-GCM authentication failed for Keystore alias '$alias': the ciphertext no longer " + + "matches the current key (the key was cleared and regenerated, or the data is corrupt)", + e, + ) + } + } + + /** Resolves the key a decrypt should use, applying the [generateKeyOnDecrypt] policy. */ + private fun decryptionKey(): SecretKey = + if (generateKeyOnDecrypt) getOrCreateKey() else existingKey() ?: onMissingDecryptionKey() + + /** + * Invoked when a decrypt finds no key and the policy forbids minting one. The default fails with + * a generic message; auth-bound subclasses override it to explain that the absence means the key + * was invalidated. + */ + protected open fun onMissingDecryptionKey(): Nothing = error("Keystore key for alias '$alias' is missing") + + private fun doEncrypt(key: SecretKey, plaintext: String): String { + val cipher = initEncryptCipher(key) + val iv = cipher.iv + val ciphertext = cipher.doFinal(plaintext.toByteArray(Charsets.UTF_8)) + return Base64.encodeToString(iv + ciphertext, Base64.NO_WRAP) + } + + /** Test seam separating the Keystore-backed cipher call from the decrypt policy/error mapping. */ + protected open fun decryptWithKey(key: SecretKey, encoded: String): String { + val bytes = Base64.decode(encoded, Base64.NO_WRAP) + val iv = bytes.copyOfRange(0, IV_LENGTH) + val ciphertext = bytes.copyOfRange(IV_LENGTH, bytes.size) + val cipher = Cipher.getInstance(TRANSFORMATION) + cipher.init(Cipher.DECRYPT_MODE, key, GCMParameterSpec(TAG_BITS, iv)) + return String(cipher.doFinal(ciphertext), Charsets.UTF_8) + } + + /** + * Initializes an encrypt-mode [Cipher] with [key]. Shared by [doEncrypt] and reused by auth-bound + * subclasses to probe whether a key is still usable (a bare `init` throws if it was invalidated). + */ + protected fun initEncryptCipher(key: SecretKey): Cipher = + Cipher.getInstance(TRANSFORMATION).apply { init(Cipher.ENCRYPT_MODE, key) } + + /** True when a key exists under [alias]. */ + protected fun keyExists(): Boolean = existingKey() != null + + /** Deletes the key so a fresh one is generated on the next [encrypt]. */ + protected fun deleteKeyEntry(): Unit = synchronized(keyLock) { + KeyStore.getInstance(ANDROID_KEYSTORE).apply { load(null) }.deleteEntry(alias) + } + + protected open fun existingKey(): SecretKey? = synchronized(keyLock) { + val keyStore = KeyStore.getInstance(ANDROID_KEYSTORE).apply { load(null) } + (keyStore.getEntry(alias, null) as? KeyStore.SecretKeyEntry)?.secretKey + } + + // Synchronized so two concurrent first-run encrypts can't both generate a key under the same + // alias — the second would overwrite the first, leaving the first secret undecryptable. + protected open fun getOrCreateKey(): SecretKey = synchronized(keyLock) { + existingKey()?.let { return it } + val generator = KeyGenerator.getInstance(KeyProperties.KEY_ALGORITHM_AES, ANDROID_KEYSTORE) + generator.init(keySpec()) + generator.generateKey() + } + + /** The alias-bound [KeyGenParameterSpec] for this key; subclasses extend [keySpecBuilder]. */ + protected abstract fun keySpec(): KeyGenParameterSpec + + /** The common AES-256-GCM builder (encrypt + decrypt, GCM, no padding, 256-bit) to extend. */ + protected fun keySpecBuilder(): KeyGenParameterSpec.Builder = KeyGenParameterSpec.Builder( + alias, + KeyProperties.PURPOSE_ENCRYPT or KeyProperties.PURPOSE_DECRYPT, + ) + .setBlockModes(KeyProperties.BLOCK_MODE_GCM) + .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_NONE) + .setKeySize(AES_KEY_SIZE_BITS) + + private companion object { + const val ANDROID_KEYSTORE = "AndroidKeyStore" + const val TRANSFORMATION = "AES/GCM/NoPadding" + const val IV_LENGTH = 12 + const val TAG_BITS = 128 + const val AES_KEY_SIZE_BITS = 256 + } +} diff --git a/app/src/main/kotlin/org/libremail/data/security/AppLockManager.kt b/app/src/main/kotlin/org/libremail/data/security/AppLockManager.kt index c20a8ae..bdf8278 100644 --- a/app/src/main/kotlin/org/libremail/data/security/AppLockManager.kt +++ b/app/src/main/kotlin/org/libremail/data/security/AppLockManager.kt @@ -3,8 +3,6 @@ package org.libremail.data.security import android.app.KeyguardManager import android.content.Context -import androidx.biometric.BiometricManager.Authenticators.BIOMETRIC_STRONG -import androidx.biometric.BiometricManager.Authenticators.DEVICE_CREDENTIAL import dagger.hilt.android.qualifiers.ApplicationContext import javax.inject.Inject import javax.inject.Singleton @@ -19,8 +17,13 @@ interface AppLockManager { fun isDeviceSecure(): Boolean companion object { - /** Accept a strong biometric OR the device credential (PIN/pattern/password) as fallback. */ - const val AUTHENTICATORS: Int = BIOMETRIC_STRONG or DEVICE_CREDENTIAL + /** + * `BiometricPrompt` authenticators (a strong biometric OR the device credential). Derived + * from the single [AuthenticatorPolicy] source of truth so it can never drift from the + * auth-bound Keystore key's authenticators in [DatabaseKeyCipher] — a drift that would let a + * prompt succeed against an authenticator the key rejects with `UserNotAuthenticatedException`. + */ + val AUTHENTICATORS: Int = AuthenticatorPolicy.biometricPromptAuthenticators } } diff --git a/app/src/main/kotlin/org/libremail/data/security/AuthenticatorPolicy.kt b/app/src/main/kotlin/org/libremail/data/security/AuthenticatorPolicy.kt new file mode 100644 index 0000000..a535914 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/data/security/AuthenticatorPolicy.kt @@ -0,0 +1,48 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.security + +import android.security.keystore.KeyProperties +import androidx.biometric.BiometricManager + +/** A user-presence proof the app accepts to unlock the app lock and authorize the auth-bound key. */ +enum class AppAuthenticator { STRONG_BIOMETRIC, DEVICE_CREDENTIAL } + +/** + * THE single source of truth for which authenticators gate the app lock. The same [ACCEPTED] set is + * mapped into each Android API's own flag vocabulary — androidx [BiometricManager] (for + * `BiometricPrompt.setAllowedAuthenticators`, via [AppLockManager.AUTHENTICATORS]) and platform + * [KeyProperties] (for `KeyGenParameterSpec.setUserAuthenticationParameters`, via + * [DatabaseKeyCipher]) — because the two APIs use *different* bit constants for the same concept + * (`BIOMETRIC_STRONG` is `0xF` here, `AUTH_BIOMETRIC_STRONG` is `1` there). + * + * Deriving both flag sets from one [ACCEPTED] set — with an exhaustive `when` that the compiler forces + * to cover every [AppAuthenticator] — makes it impossible to loosen or tighten one without the other. + * That drift is otherwise invisible until a device hits it: the `BiometricPrompt` would accept an + * authenticator the key does not, so the prompt succeeds but the key then throws + * `UserNotAuthenticatedException` at use. + */ +object AuthenticatorPolicy { + + /** Accept a strong biometric OR the device credential (PIN / pattern / password) as fallback. */ + val ACCEPTED: Set = + setOf(AppAuthenticator.STRONG_BIOMETRIC, AppAuthenticator.DEVICE_CREDENTIAL) + + /** [ACCEPTED] as androidx `BiometricManager.Authenticators` flags for a `BiometricPrompt`. */ + val biometricPromptAuthenticators: Int = ACCEPTED.toFlags { + when (it) { + AppAuthenticator.STRONG_BIOMETRIC -> BiometricManager.Authenticators.BIOMETRIC_STRONG + AppAuthenticator.DEVICE_CREDENTIAL -> BiometricManager.Authenticators.DEVICE_CREDENTIAL + } + } + + /** [ACCEPTED] as platform `KeyProperties.AUTH_*` flags for a `KeyGenParameterSpec`. */ + val keyGenAuthenticators: Int = ACCEPTED.toFlags { + when (it) { + AppAuthenticator.STRONG_BIOMETRIC -> KeyProperties.AUTH_BIOMETRIC_STRONG + AppAuthenticator.DEVICE_CREDENTIAL -> KeyProperties.AUTH_DEVICE_CREDENTIAL + } + } + + private inline fun Set.toFlags(flagOf: (AppAuthenticator) -> Int): Int = + fold(0) { acc, authenticator -> acc or flagOf(authenticator) } +} diff --git a/app/src/main/kotlin/org/libremail/data/security/DatabaseKeyCipher.kt b/app/src/main/kotlin/org/libremail/data/security/DatabaseKeyCipher.kt index f0aa1ec..41fe348 100644 --- a/app/src/main/kotlin/org/libremail/data/security/DatabaseKeyCipher.kt +++ b/app/src/main/kotlin/org/libremail/data/security/DatabaseKeyCipher.kt @@ -4,15 +4,8 @@ package org.libremail.data.security import android.os.Build import android.security.keystore.KeyGenParameterSpec import android.security.keystore.KeyPermanentlyInvalidatedException -import android.security.keystore.KeyProperties import android.security.keystore.UserNotAuthenticatedException -import android.util.Base64 import android.util.Log -import java.security.KeyStore -import javax.crypto.Cipher -import javax.crypto.KeyGenerator -import javax.crypto.SecretKey -import javax.crypto.spec.GCMParameterSpec import javax.inject.Inject import javax.inject.Singleton @@ -30,47 +23,29 @@ import javax.inject.Singleton * lock — permanently invalidates the key; [decrypt] then throws [KeyPermanentlyInvalidatedException], * which the caller treats as "cache unrecoverable -> clear + re-sync". * + * Reuses the shared [AesGcmKeystoreCipher] plumbing; its delta is the auth-bound [keySpec] and the + * invalidation handling below. It is created with `generateKeyOnDecrypt = false` on purpose: a + * missing auth-bound key means it was INVALIDATED, so [decrypt] fails fast (via [onMissingDecryptionKey]) + * rather than silently minting a new key and re-arming the lock against a cache it can never decrypt. + * * DEVICE-ONLY: auth-bound Keystore keys and BiometricPrompt cannot be exercised in JVM unit tests; * this class is covered by on-device instrumentation / manual validation only. */ @Singleton -class DatabaseKeyCipher @Inject constructor() { - - private val keyLock = Any() +class DatabaseKeyCipher @Inject constructor() : + AesGcmKeystoreCipher(alias = KEY_ALIAS, generateKeyOnDecrypt = false) { /** * Returns Base64(iv || ciphertext). Requires a valid auth window (call right after unlock). * Self-heals a stale, permanently-invalidated key by replacing it and retrying once, so sealing a * fresh passphrase after a re-enrollment doesn't fail. */ - fun encrypt(plaintext: String): String = try { - doEncrypt(getOrCreateKey(), plaintext) + override fun encrypt(plaintext: String): String = try { + super.encrypt(plaintext) } catch (e: KeyPermanentlyInvalidatedException) { Log.d(TAG, "replacing invalidated auth-bound key before sealing", e) deleteKey() - doEncrypt(getOrCreateKey(), plaintext) - } - - private fun doEncrypt(key: SecretKey, plaintext: String): String { - val cipher = Cipher.getInstance(TRANSFORMATION) - cipher.init(Cipher.ENCRYPT_MODE, key) - val iv = cipher.iv - val ciphertext = cipher.doFinal(plaintext.toByteArray(Charsets.UTF_8)) - return Base64.encodeToString(iv + ciphertext, Base64.NO_WRAP) - } - - /** - * Decrypts a blob produced by [encrypt]. Requires a valid auth window. Throws - * [KeyPermanentlyInvalidatedException] if the key was invalidated by re-enrollment / lock removal. - */ - fun decrypt(encoded: String): String { - val key = existingKey() ?: error("auth-bound database key is missing") - val bytes = Base64.decode(encoded, Base64.NO_WRAP) - val iv = bytes.copyOfRange(0, IV_LENGTH) - val ciphertext = bytes.copyOfRange(IV_LENGTH, bytes.size) - val cipher = Cipher.getInstance(TRANSFORMATION) - cipher.init(Cipher.DECRYPT_MODE, key, GCMParameterSpec(TAG_BITS, iv)) - return String(cipher.doFinal(ciphertext), Charsets.UTF_8) + super.encrypt(plaintext) } /** @@ -82,7 +57,7 @@ class DatabaseKeyCipher @Inject constructor() { fun isInvalidated(): Boolean { val key = existingKey() ?: return false return try { - Cipher.getInstance(TRANSFORMATION).init(Cipher.ENCRYPT_MODE, key) + initEncryptCipher(key) false } catch (e: KeyPermanentlyInvalidatedException) { Log.d(TAG, "auth-bound database key invalidated", e) @@ -99,39 +74,22 @@ class DatabaseKeyCipher @Inject constructor() { } } - fun hasKey(): Boolean = existingKey() != null + fun hasKey(): Boolean = keyExists() /** Deletes the auth-bound key so a fresh one is generated on the next [encrypt]. */ - fun deleteKey(): Unit = synchronized(keyLock) { - KeyStore.getInstance(ANDROID_KEYSTORE).apply { load(null) }.deleteEntry(KEY_ALIAS) - } + fun deleteKey(): Unit = deleteKeyEntry() - private fun existingKey(): SecretKey? = synchronized(keyLock) { - val keyStore = KeyStore.getInstance(ANDROID_KEYSTORE).apply { load(null) } - (keyStore.getEntry(KEY_ALIAS, null) as? KeyStore.SecretKeyEntry)?.secretKey - } + /** A missing auth-bound key means it was invalidated; surface that instead of regenerating. */ + override fun onMissingDecryptionKey(): Nothing = error("auth-bound database key is missing") - private fun getOrCreateKey(): SecretKey = synchronized(keyLock) { - existingKey()?.let { return it } - val generator = KeyGenerator.getInstance(KeyProperties.KEY_ALGORITHM_AES, ANDROID_KEYSTORE) - generator.init(buildSpec()) - generator.generateKey() - } - - private fun buildSpec(): KeyGenParameterSpec { - val builder = KeyGenParameterSpec.Builder( - KEY_ALIAS, - KeyProperties.PURPOSE_ENCRYPT or KeyProperties.PURPOSE_DECRYPT, - ) - .setBlockModes(KeyProperties.BLOCK_MODE_GCM) - .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_NONE) - .setKeySize(AES_KEY_SIZE_BITS) + override fun keySpec(): KeyGenParameterSpec { + val builder = keySpecBuilder() .setUserAuthenticationRequired(true) .setInvalidatedByBiometricEnrollment(true) if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.R) { builder.setUserAuthenticationParameters( AUTH_VALIDITY_SECONDS, - KeyProperties.AUTH_BIOMETRIC_STRONG or KeyProperties.AUTH_DEVICE_CREDENTIAL, + AuthenticatorPolicy.keyGenAuthenticators, ) } else { @Suppress("DEPRECATION") @@ -141,12 +99,7 @@ class DatabaseKeyCipher @Inject constructor() { } private companion object { - const val ANDROID_KEYSTORE = "AndroidKeyStore" const val KEY_ALIAS = "libremail.dbkey.auth" - const val TRANSFORMATION = "AES/GCM/NoPadding" - const val IV_LENGTH = 12 - const val TAG_BITS = 128 - const val AES_KEY_SIZE_BITS = 256 const val AUTH_VALIDITY_SECONDS = 15 const val TAG = "LibreMailDbKeyAuth" } diff --git a/app/src/main/kotlin/org/libremail/data/security/DatabaseKeyStore.kt b/app/src/main/kotlin/org/libremail/data/security/DatabaseKeyStore.kt index 74c3356..99551b8 100644 --- a/app/src/main/kotlin/org/libremail/data/security/DatabaseKeyStore.kt +++ b/app/src/main/kotlin/org/libremail/data/security/DatabaseKeyStore.kt @@ -2,6 +2,7 @@ package org.libremail.data.security import android.content.Context +import androidx.annotation.VisibleForTesting import androidx.datastore.core.DataStore import androidx.datastore.preferences.core.Preferences import androidx.datastore.preferences.core.booleanPreferencesKey @@ -44,6 +45,16 @@ class DatabaseKeyStore @Inject constructor( ) { private val generationLock = Mutex() + /** + * The DataStore that persists the sealed passphrases (its own `libremail_dbkey` file, never the + * Room DB it protects). Exposed as a [VisibleForTesting] seam — mirroring AppLockViewModel's + * injectable dispatcher — so the dual-seal exchange invariants are exercisable in JVM unit tests + * against an in-memory store, decoupled from the device-only Keystore that produces the sealed + * blobs. Production always uses the real per-app [dbKeyDataStore]. + */ + @VisibleForTesting + internal var dataStore: DataStore = context.dbKeyDataStore + /** * Resolve the passphrase needed to open (or convert) the on-disk cache, keyed off which seal * actually EXISTS — not off the app-lock setting, which lives in a separate DataStore and can be @@ -108,7 +119,7 @@ class DatabaseKeyStore @Inject constructor( suspend fun sealWithAuth(): Unit = generationLock.withLock { val plain = masterSealed() ?: session.current() ?: generateHex() val sealed = authCipher.encrypt(plain) - context.dbKeyDataStore.edit { + dataStore.edit { it[SEALED_AUTH] = sealed it.remove(SEALED_MASTER) } @@ -123,7 +134,7 @@ class DatabaseKeyStore @Inject constructor( */ suspend fun sealWithMaster(): Unit = generationLock.withLock { val plain = session.current() ?: read(SEALED_AUTH)?.let { authCipher.decrypt(it) } ?: return@withLock - context.dbKeyDataStore.edit { + dataStore.edit { it[SEALED_MASTER] = crypto.encrypt(plain) it.remove(SEALED_AUTH) } @@ -140,7 +151,7 @@ class DatabaseKeyStore @Inject constructor( * encrypted database file in the same operation, otherwise it becomes permanently unreadable. */ suspend fun resetSealedPassphrase(): Unit = generationLock.withLock { - context.dbKeyDataStore.edit { + dataStore.edit { it.remove(SEALED_AUTH) it.remove(SEALED_MASTER) } @@ -155,7 +166,7 @@ class DatabaseKeyStore @Inject constructor( * the corruption-safe way to "clear + re-sync" after a screen-lock change invalidates the key. */ suspend fun setClearPending() { - context.dbKeyDataStore.edit { it[CLEAR_PENDING] = true } + dataStore.edit { it[CLEAR_PENDING] = true } } /** @@ -163,20 +174,20 @@ class DatabaseKeyStore @Inject constructor( * perform the wipe (+ [resetSealedPassphrase]) FIRST and then call [clearClearPending], so a crash * mid-wipe simply repeats the idempotent wipe next start instead of stranding an unreadable file. */ - suspend fun isClearPending(): Boolean = context.dbKeyDataStore.data.first()[CLEAR_PENDING] == true + suspend fun isClearPending(): Boolean = dataStore.data.first()[CLEAR_PENDING] == true /** Clear the wipe flag. Call ONLY after the wipe + [resetSealedPassphrase] have completed. */ suspend fun clearClearPending() { - context.dbKeyDataStore.edit { it.remove(CLEAR_PENDING) } + dataStore.edit { it.remove(CLEAR_PENDING) } } private suspend fun masterSealed(): String? = read(SEALED_MASTER)?.let { crypto.decrypt(it) } - private suspend fun read(key: Preferences.Key): String? = context.dbKeyDataStore.data.first()[key] + private suspend fun read(key: Preferences.Key): String? = dataStore.data.first()[key] private suspend fun generateAndSealMaster(): String { val hex = generateHex() - context.dbKeyDataStore.edit { it[SEALED_MASTER] = crypto.encrypt(hex) } + dataStore.edit { it[SEALED_MASTER] = crypto.encrypt(hex) } return hex } diff --git a/app/src/main/kotlin/org/libremail/data/security/KeystoreCrypto.kt b/app/src/main/kotlin/org/libremail/data/security/KeystoreCrypto.kt index 5d8cd05..8844203 100644 --- a/app/src/main/kotlin/org/libremail/data/security/KeystoreCrypto.kt +++ b/app/src/main/kotlin/org/libremail/data/security/KeystoreCrypto.kt @@ -2,69 +2,25 @@ package org.libremail.data.security import android.security.keystore.KeyGenParameterSpec -import android.security.keystore.KeyProperties -import android.util.Base64 -import java.security.KeyStore -import javax.crypto.Cipher -import javax.crypto.KeyGenerator -import javax.crypto.SecretKey -import javax.crypto.spec.GCMParameterSpec import javax.inject.Inject import javax.inject.Singleton /** - * AES-256-GCM encryption backed by a non-exportable key in the Android Keystore. Secrets - * (OAuth tokens, IMAP passwords) are encrypted at rest so they never touch disk in plaintext. + * AES-256-GCM encryption backed by a non-exportable key in the Android Keystore. Secrets (OAuth + * tokens, IMAP passwords) are encrypted at rest so they never touch disk in plaintext. + * + * The non-auth-bound **master** key: usable in the background without a user-presence prompt, so + * credential access keeps working while the app is locked. As the master key it is minted lazily on a + * missing-alias decrypt (`generateKeyOnDecrypt = true`) — correct for a first run that has nothing + * sealed yet. Contrast the auth-bound [DatabaseKeyCipher], whose absent key means invalidation and so + * fails fast; the shared [AesGcmKeystoreCipher] documents why the two must differ. */ @Singleton -class KeystoreCrypto @Inject constructor() { +class KeystoreCrypto @Inject constructor() : AesGcmKeystoreCipher(alias = KEY_ALIAS, generateKeyOnDecrypt = true) { - private val keyLock = Any() - - // Synchronized so two concurrent first-run encrypts can't both generate a key under the same - // alias — the second would overwrite the first, leaving the first secret undecryptable. - private fun secretKey(): SecretKey = synchronized(keyLock) { - val keyStore = KeyStore.getInstance(ANDROID_KEYSTORE).apply { load(null) } - (keyStore.getEntry(KEY_ALIAS, null) as? KeyStore.SecretKeyEntry)?.let { return it.secretKey } - - val generator = KeyGenerator.getInstance(KeyProperties.KEY_ALGORITHM_AES, ANDROID_KEYSTORE) - generator.init( - KeyGenParameterSpec.Builder( - KEY_ALIAS, - KeyProperties.PURPOSE_ENCRYPT or KeyProperties.PURPOSE_DECRYPT, - ) - .setBlockModes(KeyProperties.BLOCK_MODE_GCM) - .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_NONE) - .setKeySize(AES_KEY_SIZE_BITS) - .build(), - ) - generator.generateKey() - } - - /** Returns Base64(iv || ciphertext). */ - fun encrypt(plaintext: String): String { - val cipher = Cipher.getInstance(TRANSFORMATION) - cipher.init(Cipher.ENCRYPT_MODE, secretKey()) - val iv = cipher.iv - val ciphertext = cipher.doFinal(plaintext.toByteArray(Charsets.UTF_8)) - return Base64.encodeToString(iv + ciphertext, Base64.NO_WRAP) - } - - fun decrypt(encoded: String): String { - val bytes = Base64.decode(encoded, Base64.NO_WRAP) - val iv = bytes.copyOfRange(0, IV_LENGTH) - val ciphertext = bytes.copyOfRange(IV_LENGTH, bytes.size) - val cipher = Cipher.getInstance(TRANSFORMATION) - cipher.init(Cipher.DECRYPT_MODE, secretKey(), GCMParameterSpec(TAG_BITS, iv)) - return String(cipher.doFinal(ciphertext), Charsets.UTF_8) - } + override fun keySpec(): KeyGenParameterSpec = keySpecBuilder().build() private companion object { - const val ANDROID_KEYSTORE = "AndroidKeyStore" const val KEY_ALIAS = "libremail.master.key" - const val TRANSFORMATION = "AES/GCM/NoPadding" - const val IV_LENGTH = 12 - const val TAG_BITS = 128 - const val AES_KEY_SIZE_BITS = 256 } } diff --git a/app/src/main/kotlin/org/libremail/data/settings/SettingsRepository.kt b/app/src/main/kotlin/org/libremail/data/settings/SettingsRepository.kt index e048c41..85c2158 100644 --- a/app/src/main/kotlin/org/libremail/data/settings/SettingsRepository.kt +++ b/app/src/main/kotlin/org/libremail/data/settings/SettingsRepository.kt @@ -72,6 +72,8 @@ private object Keys { val RETENTION_COUNT = intPreferencesKey("retention_count") val RETENTION_MONTHS = intPreferencesKey("retention_months") val BATTERY_PROMPT_HANDLED = booleanPreferencesKey("battery_prompt_handled") + val CONTACTS_PROMPT_HANDLED = booleanPreferencesKey("contacts_prompt_handled") + val CONTACTS_PERMISSION_REQUESTED = booleanPreferencesKey("contacts_permission_requested") } /** @@ -118,6 +120,29 @@ class SettingsRepository @Inject constructor(@ApplicationContext private val con suspend fun setBatteryPromptHandled(value: Boolean) = put(Keys.BATTERY_PROMPT_HANDLED, value) + /** + * One-time onboarding flag: whether the user has already seen/acted on the "contacts access" + * opt-in step, so onboarding offers it at most once (see #127). Like [isBatteryPromptHandled] this + * is internal onboarding state, not a user-facing preference — the Settings contacts entry (#129) + * is the way to enable autocomplete later. + */ + suspend fun isContactsPromptHandled(): Boolean = + context.settingsDataStore.data.map { it[Keys.CONTACTS_PROMPT_HANDLED] ?: false }.first() + + suspend fun setContactsPromptHandled(value: Boolean) = put(Keys.CONTACTS_PROMPT_HANDLED, value) + + /** + * Whether the `READ_CONTACTS` system dialog has ever actually been shown (from the onboarding step + * or the Settings entry). It is the only reliable signal — combined with the Activity's + * `shouldShowRequestPermissionRationale` — that separates "never asked yet" from "permanently + * denied", so the Settings entry (#129) can offer an in-app request versus a deep-link to system + * settings. See [ContactPermissionDecision][org.libremail.contacts.ContactPermissionDecision]. + */ + val contactsPermissionRequested: Flow = + context.settingsDataStore.data.map { it[Keys.CONTACTS_PERMISSION_REQUESTED] ?: false } + + suspend fun setContactsPermissionRequested(value: Boolean) = put(Keys.CONTACTS_PERMISSION_REQUESTED, value) + suspend fun setDynamicColor(value: Boolean) = put(Keys.DYNAMIC_COLOR, value) suspend fun setNewMailNotifications(value: Boolean) = put(Keys.NEW_MAIL_NOTIFICATIONS, value) suspend fun setPushIdle(value: Boolean) = put(Keys.PUSH_IDLE, value) diff --git a/app/src/main/kotlin/org/libremail/data/sync/SyncScheduler.kt b/app/src/main/kotlin/org/libremail/data/sync/SyncScheduler.kt index d23806f..fcaa4d0 100644 --- a/app/src/main/kotlin/org/libremail/data/sync/SyncScheduler.kt +++ b/app/src/main/kotlin/org/libremail/data/sync/SyncScheduler.kt @@ -6,6 +6,7 @@ import androidx.work.ExistingPeriodicWorkPolicy import androidx.work.ExistingWorkPolicy import androidx.work.NetworkType import androidx.work.OneTimeWorkRequestBuilder +import androidx.work.Operation import androidx.work.OutOfQuotaPolicy import androidx.work.PeriodicWorkRequestBuilder import androidx.work.WorkManager @@ -48,13 +49,18 @@ class SyncScheduler @Inject constructor( workManager.enqueueUniquePeriodicWork(PERIODIC_WORK, PERIODIC_POLICY, request) } - /** One-shot sync, e.g. right after an account is added. */ - fun syncNow() { + /** + * One-shot sync, e.g. right after an account is added. Returns the enqueue [Operation] so callers + * that must guarantee the WorkSpec is durably persisted before killing the process (the app-lock + * recovery restart) can await it — WorkManager persists the WorkSpec asynchronously on its serial + * task executor, so a fire-and-forget enqueue can be lost to a racing process death. + */ + fun syncNow(): Operation { val request = OneTimeWorkRequestBuilder() .setConstraints(networkConstraint) .setExpedited(OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST) .build() - workManager.enqueueUniqueWork(ONESHOT_WORK, ExistingWorkPolicy.REPLACE, request) + return workManager.enqueueUniqueWork(ONESHOT_WORK, ExistingWorkPolicy.REPLACE, request) } /** diff --git a/app/src/main/kotlin/org/libremail/di/AccountDatabaseModule.kt b/app/src/main/kotlin/org/libremail/di/AccountDatabaseModule.kt index 435f929..60bac01 100644 --- a/app/src/main/kotlin/org/libremail/di/AccountDatabaseModule.kt +++ b/app/src/main/kotlin/org/libremail/di/AccountDatabaseModule.kt @@ -3,14 +3,17 @@ package org.libremail.di import android.content.Context import androidx.room.Room +import androidx.sqlite.db.framework.FrameworkSQLiteOpenHelperFactory import dagger.Module import dagger.Provides import dagger.hilt.InstallIn import dagger.hilt.android.qualifiers.ApplicationContext import dagger.hilt.components.SingletonComponent +import kotlinx.coroutines.runBlocking import org.libremail.data.local.AccountDatabase import org.libremail.data.local.DatabaseFiles.ACCOUNTS_NAME -import org.libremail.data.local.LibreMailDatabase +import org.libremail.data.local.DatabaseProvisioner +import org.libremail.data.local.DeferredOpenHelperFactory import org.libremail.data.local.dao.AccountDao import org.libremail.data.local.dao.AccountSettingsDao import org.libremail.data.local.dao.CredentialDao @@ -27,17 +30,26 @@ import javax.inject.Singleton object AccountDatabaseModule { /** - * The plaintext account store. Depends on [LibreMailDatabase] purely for construction ordering: - * building the cache runs the one-time [org.libremail.data.local.AccountDataMigrator] (which - * populates this file on a dedicated connection) and then drops the moved tables, so by the time - * Room opens this file the data is already present and no other connection is touching it. + * The plaintext account store. Its OPEN is gated on [DatabaseProvisioner.prepareCache] so the + * one-time [org.libremail.data.local.AccountDataMigrator] (which populates this file on a dedicated + * connection, then drops the moved tables from the cache) has finished before Room opens this file — + * the migrate-before-open ordering the old construction-time dependency on `LibreMailDatabase` + * enforced, now moved OFF the injection path (issue #93). This store always opens unkeyed, so it + * ignores the returned cache open-mode and only awaits the shared sequence. */ @Provides @Singleton fun provideAccountDatabase( @ApplicationContext context: Context, - @Suppress("UNUSED_PARAMETER") cacheDatabase: LibreMailDatabase, - ): AccountDatabase = Room.databaseBuilder(context, AccountDatabase::class.java, ACCOUNTS_NAME).build() + provisioner: DatabaseProvisioner, + ): AccountDatabase = Room.databaseBuilder(context, AccountDatabase::class.java, ACCOUNTS_NAME) + .openHelperFactory( + DeferredOpenHelperFactory { configuration -> + runBlocking { provisioner.prepareCache() } + FrameworkSQLiteOpenHelperFactory().create(configuration) + }, + ) + .build() @Provides fun provideAccountDao(database: AccountDatabase): AccountDao = database.accountDao() diff --git a/app/src/main/kotlin/org/libremail/di/DatabaseModule.kt b/app/src/main/kotlin/org/libremail/di/DatabaseModule.kt index d247ad1..30a82f6 100644 --- a/app/src/main/kotlin/org/libremail/di/DatabaseModule.kt +++ b/app/src/main/kotlin/org/libremail/di/DatabaseModule.kt @@ -3,17 +3,18 @@ package org.libremail.di import android.content.Context import androidx.room.Room +import androidx.sqlite.db.framework.FrameworkSQLiteOpenHelperFactory import dagger.Module import dagger.Provides import dagger.hilt.InstallIn import dagger.hilt.android.qualifiers.ApplicationContext import dagger.hilt.components.SingletonComponent -import kotlinx.coroutines.flow.first import kotlinx.coroutines.runBlocking import net.zetetic.database.sqlcipher.SupportOpenHelperFactory -import org.libremail.data.local.AccountDataMigrator -import org.libremail.data.local.DatabaseEncryption +import org.libremail.data.local.CacheOpenMode import org.libremail.data.local.DatabaseFiles +import org.libremail.data.local.DatabaseProvisioner +import org.libremail.data.local.DeferredOpenHelperFactory import org.libremail.data.local.LibreMailDatabase import org.libremail.data.local.MIGRATION_10_11 import org.libremail.data.local.MIGRATION_11_12 @@ -21,6 +22,7 @@ import org.libremail.data.local.MIGRATION_12_13 import org.libremail.data.local.MIGRATION_13_14 import org.libremail.data.local.MIGRATION_14_15 import org.libremail.data.local.MIGRATION_15_16 +import org.libremail.data.local.MIGRATION_16_17 import org.libremail.data.local.MIGRATION_1_2 import org.libremail.data.local.MIGRATION_2_3 import org.libremail.data.local.MIGRATION_3_4 @@ -36,8 +38,6 @@ import org.libremail.data.local.dao.DraftDao import org.libremail.data.local.dao.FolderDao import org.libremail.data.local.dao.MessageDao import org.libremail.data.local.dao.OutboxDao -import org.libremail.data.security.DatabaseKeyStore -import org.libremail.data.settings.SettingsRepository import javax.inject.Singleton @Module @@ -46,13 +46,8 @@ object DatabaseModule { @Provides @Singleton - fun provideDatabase( - @ApplicationContext context: Context, - keyStore: DatabaseKeyStore, - settingsRepository: SettingsRepository, - accountDataMigrator: AccountDataMigrator, - ): LibreMailDatabase { - val builder = Room.databaseBuilder(context, LibreMailDatabase::class.java, DB_NAME) + fun provideDatabase(@ApplicationContext context: Context, provisioner: DatabaseProvisioner): LibreMailDatabase = + Room.databaseBuilder(context, LibreMailDatabase::class.java, DB_NAME) .addMigrations( MIGRATION_1_2, MIGRATION_2_3, @@ -69,60 +64,31 @@ object DatabaseModule { MIGRATION_13_14, MIGRATION_14_15, MIGRATION_15_16, + MIGRATION_16_17, ) - // No destructive fallback: the migration chain is complete, and silently dropping the - // mail/message tables would lose cached data. A missing migration should fail loudly in - // testing instead. + // No destructive fallback: the migration chain is complete, and silently dropping the + // mail/message tables would lose cached data. A missing migration should fail loudly in + // testing instead. + // + // All blocking startup work — the issue-#111 AccountDataMigrator, the encrypted-cache + // conversion, and the Keystore passphrase resolution — is deferred OFF this injection path + // (issue #93). The factory below runs DatabaseProvisioner.prepareCache() lazily, when Room + // first OPENS the cache on its background query executor, never on the (possibly main) + // thread that injects this singleton. prepareCache() still performs that sequence before the + // file opens and in the same order, so the migrate-before-open guarantee and the encryption + // gate are unchanged — only where/when they run moved. + .openHelperFactory( + DeferredOpenHelperFactory { configuration -> + val realFactory = when (val mode = runBlocking { provisioner.prepareCache() }) { + is CacheOpenMode.Encrypted -> + SupportOpenHelperFactory(mode.passphrase.toByteArray(Charsets.US_ASCII), null, false) - // Opt-in at-rest encryption of the local cache (off by default). The conversion runs here — - // before the database is opened — so it never races an open connection; toggling the setting - // therefore takes effect on the next app start. The passphrase is sealed by the Keystore. - // - // The passphrase source is resolved from which seal actually exists - // ([DatabaseKeyStore.resolvePassphrase]), NOT from the app-lock setting (a separate DataStore - // that can disagree). When app-lock is ON the sealing key is auth-bound, so resolvePassphrase - // waits on PassphraseSession until the user authenticates. This provider must therefore never - // be constructed on the main thread while the cache is locked — LibreMailApplication injects - // AccountRepository lazily and the sync/push workers fail fast when locked, and the gate - // composes no DB-backed screen until Unlocked. - val dbFile = context.getDatabasePath(DB_NAME) - - // A screen-lock change (biometric re-enrollment / lock removal) can invalidate the auth-bound - // key so the encrypted cache is no longer decryptable. AppLockViewModel records that and - // restarts the app; we wipe the cache HERE — at cold start, before Room opens — so the file is - // never deleted from under an open connection. Crash-safe order: wipe + reset the seals, and - // only THEN clear the flag, so a kill mid-wipe just repeats the idempotent wipe next start. - // Only libremail.db is wiped: accounts/credentials live in AccountDatabase (a separate file), - // so the user stays signed in across the wipe (issue #111). - if (runBlocking { keyStore.isClearPending() }) { - DatabaseFiles.clear(context) - runBlocking { - keyStore.resetSealedPassphrase() - keyStore.clearClearPending() - } - } - - // One-time move of accounts/credentials/settings/signatures into the non-auth AccountDatabase - // (issue #111). MUST run before builder.build() below: opening the cache applies MIGRATION_15_16, - // which drops the moved tables. It runs AFTER the wipe above so an unrecoverable-key cache is - // gone first (nothing left to move) and we never block waiting on a passphrase we can't get. - runBlocking { accountDataMigrator.migrateIfNeeded() } - - val settings = runBlocking { settingsRepository.settings.first() } - val appLock = settings.appLock - if (settings.encryptCache) { - val passphrase = runBlocking { keyStore.resolvePassphrase(appLock) } - DatabaseEncryption.ensureEncrypted(dbFile, passphrase) - builder.openHelperFactory( - SupportOpenHelperFactory(passphrase.toByteArray(Charsets.US_ASCII), null, false), + CacheOpenMode.Plaintext -> FrameworkSQLiteOpenHelperFactory() + } + realFactory.create(configuration) + }, ) - } else if (DatabaseEncryption.isEncrypted(dbFile)) { - // Encryption was turned back off — decrypt so the default (unkeyed) open succeeds. - val passphrase = runBlocking { keyStore.resolvePassphrase(appLock) } - DatabaseEncryption.ensurePlaintext(dbFile, passphrase) - } - return builder.build() - } + .build() @Provides fun provideMessageDao(database: LibreMailDatabase): MessageDao = database.messageDao() diff --git a/app/src/main/kotlin/org/libremail/domain/model/Attachment.kt b/app/src/main/kotlin/org/libremail/domain/model/Attachment.kt index bcb3932..df1e96c 100644 --- a/app/src/main/kotlin/org/libremail/domain/model/Attachment.kt +++ b/app/src/main/kotlin/org/libremail/domain/model/Attachment.kt @@ -7,4 +7,10 @@ data class Attachment( val filename: String, val mimeType: String, val sizeBytes: Long, + /** + * The normalized `Content-ID` when this part is an inline image referenced from the HTML body via + * `cid:` — null for an ordinary attachment. Inline parts are cached (so the reader can resolve + * `cid:` requests) but filtered out of the user-facing attachment list. + */ + val contentId: String? = null, ) diff --git a/app/src/main/kotlin/org/libremail/domain/model/InlineImage.kt b/app/src/main/kotlin/org/libremail/domain/model/InlineImage.kt new file mode 100644 index 0000000..f86c650 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/domain/model/InlineImage.kt @@ -0,0 +1,11 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.domain.model + +/** + * An inline image embedded in an HTML message body and referenced from it via `cid:` + * (e.g. the mail-piece thumbnails in a USPS Informed Delivery digest). Unlike an [Attachment] it is + * rendered in place by the reader's WebView — which resolves the `cid:` request to these [bytes] — + * rather than being offered as a download. Not a `data class`: [bytes] identity/equality is + * irrelevant and array structural equality would be misleading (matching [DownloadedAttachment]). + */ +class InlineImage(val contentId: String, val mimeType: String, val bytes: ByteArray) diff --git a/app/src/main/kotlin/org/libremail/domain/repository/MailRepository.kt b/app/src/main/kotlin/org/libremail/domain/repository/MailRepository.kt index 8b79e00..e9c92a5 100644 --- a/app/src/main/kotlin/org/libremail/domain/repository/MailRepository.kt +++ b/app/src/main/kotlin/org/libremail/domain/repository/MailRepository.kt @@ -6,6 +6,7 @@ import kotlinx.coroutines.flow.Flow import org.libremail.domain.model.Attachment import org.libremail.domain.model.Draft import org.libremail.domain.model.Folder +import org.libremail.domain.model.InlineImage import org.libremail.domain.model.Message import org.libremail.domain.model.OutboxMessage import org.libremail.domain.model.OutgoingMessage @@ -57,6 +58,13 @@ interface MailRepository { /** Cached attachment metadata for a message, populated when the message is opened. */ fun observeAttachments(messageId: String): Flow> + /** + * The message's inline images (HTML `` parts), each with the bytes the reader's + * WebView serves for its `cid:` request. Downloads and caches any not yet on disk; returns empty + * for a plain-text message or one with no inline parts. + */ + suspend fun inlineImages(messageId: String): List + /** Downloads an attachment's bytes to a local cache file and returns it. */ suspend fun downloadAttachment(messageId: String, partIndex: Int): Result diff --git a/app/src/main/kotlin/org/libremail/mail/ImapClient.kt b/app/src/main/kotlin/org/libremail/mail/ImapClient.kt index 3ce17cd..70a2721 100644 --- a/app/src/main/kotlin/org/libremail/mail/ImapClient.kt +++ b/app/src/main/kotlin/org/libremail/mail/ImapClient.kt @@ -15,6 +15,7 @@ import jakarta.mail.event.MessageCountAdapter import jakarta.mail.event.MessageCountEvent import jakarta.mail.internet.ContentType import jakarta.mail.internet.InternetAddress +import jakarta.mail.internet.MimePart import jakarta.mail.internet.MimeUtility import jakarta.mail.search.BodyTerm import jakarta.mail.search.FromStringTerm @@ -67,8 +68,19 @@ data class FetchedMessage( /** A message body extracted from the server, with metadata for any attachment parts. */ data class MessageContent(val body: String, val isHtml: Boolean, val attachments: List = emptyList()) -/** Metadata for one attachment part. [partIndex] is its position in attachment-tree order. */ -data class AttachmentPart(val partIndex: Int, val filename: String, val mimeType: String, val sizeBytes: Long) +/** + * Metadata for one downloadable part. [partIndex] is its position in attachment-tree order. + * [contentId] is the normalized `Content-ID` (angle brackets stripped) for an inline image referenced + * from the HTML body via `cid:` — null for an ordinary attachment. Inline parts are persisted so the + * reader can resolve `cid:` requests, but excluded from the user-facing attachment list. + */ +data class AttachmentPart( + val partIndex: Int, + val filename: String, + val mimeType: String, + val sizeBytes: Long, + val contentId: String? = null, +) /** A downloaded attachment's bytes plus the metadata needed to open it. */ class DownloadedAttachment(val filename: String, val mimeType: String, val bytes: ByteArray) @@ -492,7 +504,12 @@ class ImapClient(private val reuseConnections: Boolean) { return plain } - /** Walks the MIME tree and returns attachment metadata in a stable, depth-first order. */ + /** + * Walks the MIME tree and returns downloadable-part metadata in a stable, depth-first order. + * Inline images (a `Content-ID` referenced from the HTML via `cid:`) are included so their + * bytes can be fetched by [partIndex] and resolved by the reader, but each carries its + * [AttachmentPart.contentId] so the display layer can filter them out of the attachment list. + */ private fun collectAttachments(message: Part): List { val parts = mutableListOf() collectAttachmentParts(message, parts) @@ -502,6 +519,7 @@ class ImapClient(private val reuseConnections: Boolean) { filename = attachmentName(part) ?: "attachment", mimeType = baseType(part), sizeBytes = part.size.toLong().coerceAtLeast(0L), + contentId = if (isInlineImagePart(part)) inlineContentId(part) else null, ) } } @@ -512,13 +530,10 @@ class ImapClient(private val reuseConnections: Boolean) { val multipart = part.content as? Multipart ?: return for (i in 0 until multipart.count) collectAttachmentParts(multipart.getBodyPart(i), into) } - isAttachment(part) -> into.add(part) + isAttachmentPart(part) || isInlineImagePart(part) -> into.add(part) } } - private fun isAttachment(part: Part): Boolean = - Part.ATTACHMENT.equals(part.disposition, ignoreCase = true) || !part.fileName.isNullOrBlank() - private fun attachmentName(part: Part): String? = part.fileName?.let { runCatching { MimeUtility.decodeText(it) }.getOrDefault(it) } @@ -616,3 +631,30 @@ class ImapClient(private val reuseConnections: Boolean) { const val TAG = "LibreMailIdle" } } + +/** + * 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 + * `Content-ID` (an inline image carried by `Content-Disposition: inline`) is deliberately NOT an + * attachment — it belongs in the message body, not the attachment list (issue #133). + */ +internal fun isAttachmentPart(part: Part): Boolean = Part.ATTACHMENT.equals(part.disposition, ignoreCase = true) || + (!part.fileName.isNullOrBlank() && inlineContentId(part) == null) + +/** + * True when [part] is an inline image embedded in the HTML body and referenced from it via + * `cid:` (e.g. a USPS Informed Delivery digest's mail-piece thumbnails): it has a + * `Content-ID`, is an image, and is not already an [isAttachmentPart]. Such parts are excluded from + * the attachment list and instead served to the reader's WebView by their Content-ID. + */ +internal fun isInlineImagePart(part: Part): Boolean = + inlineContentId(part) != null && part.isMimeType("image/*") && !isAttachmentPart(part) + +/** + * The normalized `Content-ID` of [part] (surrounding angle brackets stripped), or null if it has + * none. Reads it via [MimePart.getContentID] rather than `getHeader("Content-ID")`: over IMAP the + * Content-ID comes from the already-fetched BODYSTRUCTURE, whereas a raw header lookup would force + * (and often miss on) a separate per-part MIME-header fetch. + */ +internal fun inlineContentId(part: Part): String? = runCatching { (part as? MimePart)?.contentID }.getOrNull() + ?.trim()?.trim('<', '>')?.trim()?.takeUnless { it.isBlank() } diff --git a/app/src/main/kotlin/org/libremail/restart/ProcessRestarter.kt b/app/src/main/kotlin/org/libremail/restart/ProcessRestarter.kt new file mode 100644 index 0000000..34edbd6 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/restart/ProcessRestarter.kt @@ -0,0 +1,44 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.restart + +import android.content.Context +import android.content.Intent +import android.os.Process +import dagger.hilt.android.qualifiers.ApplicationContext +import javax.inject.Inject +import javax.inject.Singleton + +/** + * Relaunches the whole app in a brand-new process by handing off to [RestartActivity], a trampoline + * that runs in the separate `:restart` process. Because the trampoline survives the current process + * being killed, the relaunch it issues cannot be dropped by ActivityManager scheduling it into the + * dying process — the failure mode of a same-process "startActivity then exit(0)" restart. + * + * Used by the app-lock key-invalidation recovery to bounce the process so the cache is wiped safely at + * the next cold start (before Room reopens it). + */ +@Singleton +class ProcessRestarter @Inject constructor(@ApplicationContext private val context: Context) { + + /** + * Start the [RestartActivity] trampoline in the `:restart` process, passing it this (main) process + * PID so it can kill us and relaunch from the outside. Returns immediately; the actual kill + + * relaunch happens in the trampoline process moments later. + */ + fun restart() { + val trampoline = Intent(context, RestartActivity::class.java).apply { + // Required because we may be started from a non-Activity (Application) context. + addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) + putExtra(RestartActivity.EXTRA_ORIGINAL_PID, Process.myPid()) + } + context.startActivity(trampoline) + } + + companion object { + /** + * The `android:process` suffix of [RestartActivity] (must match AndroidManifest.xml). The + * Application uses it to skip its normal startup work when it is spun up in this aux process. + */ + const val PROCESS_SUFFIX = ":restart" + } +} diff --git a/app/src/main/kotlin/org/libremail/restart/RestartActivity.kt b/app/src/main/kotlin/org/libremail/restart/RestartActivity.kt new file mode 100644 index 0000000..3144024 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/restart/RestartActivity.kt @@ -0,0 +1,59 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.restart + +import android.app.Activity +import android.content.Intent +import android.os.Bundle +import android.os.Process +import android.util.Log + +/** + * Separate-process trampoline that performs an app relaunch from OUTSIDE the process being killed. + * + * Declared in the manifest with `android:process=":restart"`, so Android runs it in its own process. + * That is the whole point: it kills the original (main) process by PID and only THEN starts the main + * launcher activity, so the relaunch is scheduled from a process that is NOT the one being torn down. + * A same-process "startActivity then Runtime.exit(0)" restart races ActivityManager — the relaunch can + * be scheduled into the dying process and silently dropped, so the app just closes. Issuing it from a + * surviving process (the ProcessPhoenix pattern) makes the relaunch reliable. + * + * DEVICE-ONLY: the multi-process kill/relaunch cannot be exercised in JVM unit tests; see + * [ProcessRestarter] for the (testable) intent/targeting seam and AppLockViewModelTest for the + * ordering guarantees around it. + */ +class RestartActivity : Activity() { + + override fun onCreate(savedInstanceState: Bundle?) { + super.onCreate(savedInstanceState) + + // Kill the original main process FIRST so the relaunch below spins up a genuinely fresh + // process — one whose cold DatabaseModule performs the pending cache wipe before Room opens. + // If we relaunched while the old process were still alive, ActivityManager could route the + // launch back into it and the wipe-on-cold-start would never run. + val originalPid = intent.getIntExtra(EXTRA_ORIGINAL_PID, INVALID_PID) + if (originalPid > INVALID_PID && originalPid != Process.myPid()) { + Process.killProcess(originalPid) + } + + val launchIntent = packageManager.getLaunchIntentForPackage(packageName) + ?.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TASK) + if (launchIntent != null) { + startActivity(launchIntent) + } else { + Log.w(TAG, "no launch intent for $packageName; cannot relaunch after restart") + } + + finish() + // Tear down this trampoline process too: its only job was to issue the relaunch from outside + // the dying main process. + Runtime.getRuntime().exit(0) + } + + companion object { + /** Extra carrying the PID of the main process to kill, so the relaunch starts a fresh one. */ + const val EXTRA_ORIGINAL_PID = "org.libremail.restart.ORIGINAL_PID" + + private const val INVALID_PID = -1 + private const val TAG = "LibreMailRestart" + } +} diff --git a/app/src/main/kotlin/org/libremail/ui/LibreMailApp.kt b/app/src/main/kotlin/org/libremail/ui/LibreMailApp.kt index 0e7882c..1d33c9f 100644 --- a/app/src/main/kotlin/org/libremail/ui/LibreMailApp.kt +++ b/app/src/main/kotlin/org/libremail/ui/LibreMailApp.kt @@ -37,6 +37,7 @@ import org.libremail.ui.mailbox.MailboxScreen import org.libremail.ui.navigation.Routes import org.libremail.ui.onboarding.AddAnotherAccountScreen import org.libremail.ui.onboarding.BatteryOptimizationScreen +import org.libremail.ui.onboarding.ContactsAccessScreen import org.libremail.ui.onboarding.OnboardingViewModel import org.libremail.ui.onboarding.OnboardingWelcomeScreen import org.libremail.ui.outbox.OutboxScreen @@ -327,12 +328,15 @@ private fun NavGraphBuilder.onboardingGraph(navController: NavHostController) { } /** - * The tail of onboarding: the "add another?" prompt and the optional battery opt-in step. Split out of - * [onboardingGraph] so each stays a readable length; both share the graph-scoped [OnboardingViewModel]. + * The tail of onboarding: the "add another?" prompt and the optional contacts + battery opt-in steps. + * Split out of [onboardingGraph] so each stays a readable length; all share the graph-scoped + * [OnboardingViewModel]. The optional steps chain — contacts (#127) then battery (#49) — each shown + * only when needed; any that isn't is skipped, and a still-undecided (null) decision fails open. */ private fun NavGraphBuilder.onboardingFinishDestinations(navController: NavHostController) { composable(Routes.ONBOARDING_ADD_ANOTHER) { entry -> val onboarding = onboardingViewModel(navController, entry) + val contactsPromptNeeded by onboarding.contactsPromptNeeded.collectAsStateWithLifecycle() val batteryPromptNeeded by onboarding.batteryPromptNeeded.collectAsStateWithLifecycle() AddAnotherAccountScreen( onAddAnother = { @@ -341,14 +345,18 @@ private fun NavGraphBuilder.onboardingFinishDestinations(navController: NavHostC popUpTo(Routes.ONBOARDING_PICKER) { inclusive = true } } }, + onFinish = { navController.advanceOnboarding(onboarding, contactsPromptNeeded, batteryPromptNeeded) }, + ) + } + composable(Routes.ONBOARDING_CONTACTS) { entry -> + val onboarding = onboardingViewModel(navController, entry) + val batteryPromptNeeded by onboarding.batteryPromptNeeded.collectAsStateWithLifecycle() + ContactsAccessScreen( + viewModel = onboarding, onFinish = { - // Offer the battery opt-in as a final step when it's needed; otherwise go straight to - // the inbox. A still-undecided (null) decision fails open to finishing. - if (batteryPromptNeeded == true) { - navController.navigate(Routes.ONBOARDING_BATTERY) - } else { - navController.finishOnboarding(onboarding.firstAddedAccountId) - } + onboarding.markContactsPromptHandled() + // Contacts is skipped here (it was the step just shown); only battery may remain. + navController.advanceOnboarding(onboarding, contactsPromptNeeded = false, batteryPromptNeeded) }, ) } @@ -364,6 +372,23 @@ private fun NavGraphBuilder.onboardingFinishDestinations(navController: NavHostC } } +/** + * Advances through the optional onboarding tail: the next still-needed opt-in step (contacts, then + * battery), or the inbox once none remain. Each `*PromptNeeded` is the graph-scoped decision; `null` + * (undecided) is treated as "not needed" so a slow read never blocks the end of onboarding. + */ +private fun NavHostController.advanceOnboarding( + onboarding: OnboardingViewModel, + contactsPromptNeeded: Boolean?, + batteryPromptNeeded: Boolean?, +) { + when { + contactsPromptNeeded == true -> navigate(Routes.ONBOARDING_CONTACTS) + batteryPromptNeeded == true -> navigate(Routes.ONBOARDING_BATTERY) + else -> finishOnboarding(onboarding.firstAddedAccountId) + } +} + /** Leaves onboarding for the inbox — the first account added this session, or the unfiltered mailbox. */ private fun NavController.finishOnboarding(firstAccountId: String?) { val dest = if (firstAccountId != null) Routes.mailboxForAccount(firstAccountId) else Routes.MAILBOX diff --git a/app/src/main/kotlin/org/libremail/ui/compose/ComposeScreen.kt b/app/src/main/kotlin/org/libremail/ui/compose/ComposeScreen.kt index 54c0ae7..ec36b10 100644 --- a/app/src/main/kotlin/org/libremail/ui/compose/ComposeScreen.kt +++ b/app/src/main/kotlin/org/libremail/ui/compose/ComposeScreen.kt @@ -67,6 +67,8 @@ import androidx.compose.ui.text.input.KeyboardType import androidx.compose.ui.unit.dp import androidx.core.content.ContextCompat import androidx.hilt.navigation.compose.hiltViewModel +import androidx.lifecycle.Lifecycle +import androidx.lifecycle.compose.LifecycleEventEffect import androidx.lifecycle.compose.collectAsStateWithLifecycle import kotlinx.coroutines.flow.collect import org.libremail.R @@ -81,10 +83,6 @@ fun ComposeScreen(onBack: () -> Unit, viewModel: ComposeViewModel = hiltViewMode val snackbarHostState = remember { SnackbarHostState() } val context = LocalContext.current - val permissionLauncher = rememberLauncherForActivityResult( - ActivityResultContracts.RequestPermission(), - ) { granted -> viewModel.onContactsPermission(granted) } - val attachmentPicker = rememberLauncherForActivityResult( ActivityResultContracts.OpenMultipleDocuments(), ) { uris -> @@ -98,16 +96,15 @@ fun ComposeScreen(onBack: () -> Unit, viewModel: ComposeViewModel = hiltViewMode ) } - LaunchedEffect(Unit) { - val granted = ContextCompat.checkSelfPermission(context, Manifest.permission.READ_CONTACTS) == - PackageManager.PERMISSION_GRANTED - if (granted) { - viewModel.onContactsPermission( - true, - ) - } else { - permissionLauncher.launch(Manifest.permission.READ_CONTACTS) - } + // Reflect the current READ_CONTACTS grant without ever prompting: the request now lives in the + // onboarding contacts step (#127) and the Settings entry (#129), so compose only reads state. + // Re-checked on resume so enabling autocomplete later (e.g. from Settings) takes effect the next + // time compose is shown. Denial degrades gracefully — searchContacts() guards on this flag. + LifecycleEventEffect(Lifecycle.Event.ON_RESUME) { + viewModel.onContactsPermission( + ContextCompat.checkSelfPermission(context, Manifest.permission.READ_CONTACTS) == + PackageManager.PERMISSION_GRANTED, + ) } LaunchedEffect(Unit) { viewModel.finished.collect { onBack() } } BackHandler { viewModel.onExit() } diff --git a/app/src/main/kotlin/org/libremail/ui/lock/AppLockViewModel.kt b/app/src/main/kotlin/org/libremail/ui/lock/AppLockViewModel.kt index 90129d8..946f76c 100644 --- a/app/src/main/kotlin/org/libremail/ui/lock/AppLockViewModel.kt +++ b/app/src/main/kotlin/org/libremail/ui/lock/AppLockViewModel.kt @@ -2,16 +2,18 @@ package org.libremail.ui.lock import android.content.Context -import android.content.Intent import android.os.SystemClock import android.security.keystore.KeyPermanentlyInvalidatedException import android.security.keystore.UserNotAuthenticatedException import android.util.Log +import androidx.annotation.VisibleForTesting import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope +import androidx.work.Operation import dagger.hilt.android.lifecycle.HiltViewModel import dagger.hilt.android.qualifiers.ApplicationContext import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow @@ -30,6 +32,10 @@ import org.libremail.data.security.LockState import org.libremail.data.security.PassphraseSession import org.libremail.data.settings.SettingsRepository import org.libremail.data.sync.SyncScheduler +import org.libremail.restart.ProcessRestarter +import java.util.concurrent.ExecutionException +import java.util.concurrent.TimeUnit +import java.util.concurrent.TimeoutException import javax.inject.Inject /** UI state of the app-lock gate that wraps the whole app. */ @@ -73,6 +79,10 @@ class AppLockViewModel @Inject constructor( private val databaseKeyCipher: DatabaseKeyCipher, private val session: PassphraseSession, private val syncScheduler: SyncScheduler, + // Issues the key-invalidation recovery relaunch from a separate ":restart" process that survives + // this process being killed, so the relaunch can't be dropped by ActivityManager scheduling it + // into the dying process (the same-process "startActivity then exit(0)" race). + private val processRestarter: ProcessRestarter, // Application-scoped (see SecurityModule): the gate is injected rather than owned by this // Activity-scoped ViewModel so the inactivity grace window survives Activity recreation — Back on // the task root finishes the Activity and clears its ViewModelStore on API 29/30, which would @@ -83,6 +93,12 @@ class AppLockViewModel @Inject constructor( private val _uiState = MutableStateFlow(AppLockUiState.Checking) val uiState: StateFlow = _uiState.asStateFlow() + // The dispatcher for blocking Keystore/DataStore/WorkManager work pushed off the main thread. + // Injectable so the recovery flow (clearCacheAndRestart) runs on the test scheduler and its + // ordering — enqueue durably persisted BEFORE the restart — is deterministically verifiable. + @VisibleForTesting + internal var defaultDispatcher: CoroutineDispatcher = Dispatchers.Default + // Cached so onBackground / onForeground can cover the content synchronously (before the async // settings read) whenever app-lock is on — so no stale mailbox frame renders on resume. @Volatile private var appLockEnabledCached = false @@ -125,7 +141,7 @@ class AppLockViewModel @Inject constructor( // App-lock is on: cover any showing content while we resolve, so no stale mailbox frame // renders before the (async) decision lands. if (_uiState.value == AppLockUiState.Unlocked) _uiState.value = AppLockUiState.Checking - val action = withContext(Dispatchers.Default) { + val action = withContext(defaultDispatcher) { KeyInvalidationPolicy.decide( appLockEnabled = true, encryptCacheEnabled = settings.encryptCache, @@ -173,7 +189,7 @@ class AppLockViewModel @Inject constructor( /** Called by the host after a successful `BiometricPrompt`. */ fun onAuthenticated() { viewModelScope.launch { - when (withContext(Dispatchers.Default) { unlockOrArm() }) { + when (withContext(defaultDispatcher) { unlockOrArm() }) { UnlockResult.OK -> { gate.onAuthenticated() publish() @@ -258,25 +274,52 @@ class AppLockViewModel @Inject constructor( } private suspend fun clearCacheAndRestart(disableAppLock: Boolean) { - withContext(Dispatchers.Default) { + withContext(defaultDispatcher) { // Record the wipe intent BEFORE flipping app-lock off, so a crash between the two writes // leaves the wipe still pending (recoverable) rather than a disabled gate over a stale key. + // Both are DataStore edits that only return once durably committed, so they survive the + // restart below without further ceremony. databaseKeyStore.setClearPending() if (disableAppLock) settingsRepository.setAppLock(false) - syncScheduler.syncNow() // persisted by WorkManager; survives the restart + // Enqueue the post-wipe re-sync and BLOCK until WorkManager has durably persisted its + // WorkSpec before we hand off to the restart. syncNow() only *schedules* the insert on + // WorkManager's serial task executor; killing the process (via restartProcess) can race + // that async insert and drop the re-sync, leaving an empty mailbox after the wipe until the + // next periodic sync. Awaiting the enqueue Operation makes "survives the restart" real. + awaitSyncEnqueue(syncScheduler.syncNow()) } restartProcess() } + /** + * Block until WorkManager confirms the re-sync WorkSpec is durably persisted, bounded by + * [SYNC_ENQUEUE_TIMEOUT_SECONDS] so a stuck insert can never wedge recovery. A timeout/failure is + * logged and we restart anyway: the periodic sync will still eventually refill the wiped cache, so + * a best-effort wait is strictly better than the previous fire-and-forget enqueue. Runs on + * [defaultDispatcher] (never the main thread) because [Operation.result]'s get blocks. + */ + private fun awaitSyncEnqueue(operation: Operation) { + try { + operation.result.get(SYNC_ENQUEUE_TIMEOUT_SECONDS, TimeUnit.SECONDS) + } catch (e: TimeoutException) { + Log.w(TAG, "re-sync enqueue not confirmed within timeout; restarting anyway", e) + } catch (e: ExecutionException) { + Log.w(TAG, "re-sync enqueue failed; restarting anyway", e) + } catch (e: InterruptedException) { + Thread.currentThread().interrupt() + Log.w(TAG, "interrupted awaiting re-sync enqueue; restarting anyway", e) + } + } + /** * Relaunch the app in a fresh process so [org.libremail.di.DatabaseModule] wipes the cache before - * Room reopens it. DEVICE-ONLY: process restart cannot be exercised in JVM unit tests. + * Room reopens it. Delegates to [ProcessRestarter], which issues the relaunch from a separate + * process that survives this one being killed — a same-process "startActivity then exit(0)" is + * unreliable because ActivityManager may schedule the relaunch into the dying process and drop it. + * DEVICE-ONLY end to end: the multi-process kill/relaunch cannot be exercised in JVM unit tests. */ private fun restartProcess() { - val intent = context.packageManager.getLaunchIntentForPackage(context.packageName) - ?.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TASK) - if (intent != null) context.startActivity(intent) - Runtime.getRuntime().exit(0) + processRestarter.restart() } // Monotonic clock so a wall-clock change can't extend the inactivity grace window. @@ -284,5 +327,10 @@ class AppLockViewModel @Inject constructor( private companion object { const val TAG = "LibreMailAppLock" + + // Upper bound on waiting for WorkManager to persist the re-sync WorkSpec. The insert is + // normally sub-second; this only caps a pathological stall so recovery can't hang before the + // restart. On timeout we restart anyway (the periodic sync still refills the cache later). + const val SYNC_ENQUEUE_TIMEOUT_SECONDS = 5L } } diff --git a/app/src/main/kotlin/org/libremail/ui/navigation/Routes.kt b/app/src/main/kotlin/org/libremail/ui/navigation/Routes.kt index 169d8a2..3659bb9 100644 --- a/app/src/main/kotlin/org/libremail/ui/navigation/Routes.kt +++ b/app/src/main/kotlin/org/libremail/ui/navigation/Routes.kt @@ -36,6 +36,11 @@ object Routes { const val ONBOARDING_MANUAL = "onboarding/manual" const val ONBOARDING_ADD_ANOTHER = "onboarding/add_another" + // Optional onboarding step: invites the user to allow contacts access for on-device recipient + // autocomplete (#127). Shown only when the permission isn't already granted and the user hasn't + // handled it before; skippable, and precedes the battery step in the finish tail. + const val ONBOARDING_CONTACTS = "onboarding/contacts" + // Optional final onboarding step: invites the user to allow unrestricted background/battery usage // so push (IMAP IDLE) and periodic sync aren't throttled by Doze (#49). Shown only when the app // isn't already exempt and the user hasn't handled it before; otherwise onboarding skips straight diff --git a/app/src/main/kotlin/org/libremail/ui/onboarding/ContactsAccessScreen.kt b/app/src/main/kotlin/org/libremail/ui/onboarding/ContactsAccessScreen.kt new file mode 100644 index 0000000..66d2404 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/ui/onboarding/ContactsAccessScreen.kt @@ -0,0 +1,172 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.ui.onboarding + +import android.Manifest +import androidx.activity.compose.LocalActivity +import androidx.activity.compose.rememberLauncherForActivityResult +import androidx.activity.result.contract.ActivityResultContracts +import androidx.compose.foundation.layout.Arrangement +import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.Spacer +import androidx.compose.foundation.layout.fillMaxSize +import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.height +import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.layout.size +import androidx.compose.foundation.layout.widthIn +import androidx.compose.material.icons.Icons +import androidx.compose.material.icons.filled.CheckCircle +import androidx.compose.material.icons.filled.Person +import androidx.compose.material3.Button +import androidx.compose.material3.Icon +import androidx.compose.material3.MaterialTheme +import androidx.compose.material3.OutlinedButton +import androidx.compose.material3.Scaffold +import androidx.compose.material3.Text +import androidx.compose.runtime.Composable +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember +import androidx.compose.runtime.setValue +import androidx.compose.ui.Alignment +import androidx.compose.ui.Modifier +import androidx.compose.ui.res.stringResource +import androidx.compose.ui.text.style.TextAlign +import androidx.compose.ui.unit.dp +import androidx.core.app.ActivityCompat +import androidx.lifecycle.Lifecycle +import androidx.lifecycle.compose.LifecycleEventEffect +import androidx.lifecycle.compose.collectAsStateWithLifecycle +import org.libremail.R + +/** + * Optional onboarding step (shown only when needed, see [OnboardingViewModel.contactsPromptNeeded]): + * invites the user to allow contacts access for on-device recipient autocomplete. The rationale is + * on-screen up front — contacts are used **only** for suggesting recipients while composing and are + * never uploaded (#128) — and the step is clearly skippable (#127): **Not now** proceeds without it. + * + * The `READ_CONTACTS` request fires **once**, from here — the compose screen no longer prompts. After + * a grant the screen shows a "done" state; a later change of heart is handled by the Settings entry + * (#129). On returning from anywhere the grant is re-read so the screen reflects the current state. + * + * @param viewModel the graph-scoped onboarding view model (holds the live grant + the handled flag). + * @param onFinish leaves the step for the next destination; the caller also marks the prompt handled. + */ +@Composable +fun ContactsAccessScreen(viewModel: OnboardingViewModel, onFinish: () -> Unit) { + val granted by viewModel.contactsGranted.collectAsStateWithLifecycle() + val activity = LocalActivity.current + var showRationale by remember { mutableStateOf(false) } + + fun refreshRationale() { + showRationale = activity != null && + ActivityCompat.shouldShowRequestPermissionRationale(activity, Manifest.permission.READ_CONTACTS) + } + + val launcher = rememberLauncherForActivityResult(ActivityResultContracts.RequestPermission()) { result -> + viewModel.onContactsPermissionResult(result) + refreshRationale() + } + + // Re-check the grant (and whether a rationale is now owed) on resume so a change made elsewhere — + // e.g. the user granted from system settings — is reflected when this step comes back to the fore. + LifecycleEventEffect(Lifecycle.Event.ON_RESUME) { + viewModel.refreshContactsStatus() + refreshRationale() + } + + ContactsAccessContent( + granted = granted, + showRationale = showRationale, + onAllow = { + // Persist "the dialog was shown" up front so a permanent denial is later distinguishable + // from "never asked" in Settings, even if the process dies before the result arrives. + viewModel.markContactsPermissionRequested() + launcher.launch(Manifest.permission.READ_CONTACTS) + }, + onSkip = onFinish, + onContinue = onFinish, + ) +} + +/** + * Presentational body of the contacts-access step, split out so its three paths — skip, grant (the + * "done" state), and request (with the [showRationale] re-ask explanation) — are driven deterministically + * in tests without a live system permission dialog. + */ +@Composable +fun ContactsAccessContent( + granted: Boolean, + showRationale: Boolean, + onAllow: () -> Unit, + onSkip: () -> Unit, + onContinue: () -> Unit, +) { + Scaffold { padding -> + Column( + modifier = Modifier + .fillMaxSize() + .padding(padding) + .padding(24.dp), + horizontalAlignment = Alignment.CenterHorizontally, + verticalArrangement = Arrangement.Center, + ) { + Icon( + imageVector = if (granted) Icons.Filled.CheckCircle else Icons.Filled.Person, + contentDescription = null, + modifier = Modifier.size(72.dp), + tint = MaterialTheme.colorScheme.primary, + ) + Spacer(Modifier.height(24.dp)) + Text( + text = stringResource( + if (granted) R.string.onboarding_contacts_done_title else R.string.onboarding_contacts_title, + ), + style = MaterialTheme.typography.headlineSmall, + textAlign = TextAlign.Center, + ) + Spacer(Modifier.height(8.dp)) + Text( + text = stringResource( + if (granted) R.string.onboarding_contacts_done_body else R.string.onboarding_contacts_body, + ), + style = MaterialTheme.typography.bodyLarge, + color = MaterialTheme.colorScheme.onSurfaceVariant, + textAlign = TextAlign.Center, + ) + Spacer(Modifier.height(32.dp)) + + if (granted) { + Button( + onClick = onContinue, + modifier = Modifier.fillMaxWidth().widthIn(max = 360.dp), + ) { + Text(stringResource(R.string.onboarding_contacts_continue)) + } + } else { + if (showRationale) { + Text( + text = stringResource(R.string.onboarding_contacts_rationale), + style = MaterialTheme.typography.bodyMedium, + color = MaterialTheme.colorScheme.onSurfaceVariant, + textAlign = TextAlign.Center, + ) + Spacer(Modifier.height(24.dp)) + } + Button( + onClick = onAllow, + modifier = Modifier.fillMaxWidth().widthIn(max = 360.dp), + ) { + Text(stringResource(R.string.onboarding_contacts_allow)) + } + Spacer(Modifier.height(12.dp)) + OutlinedButton( + onClick = onSkip, + modifier = Modifier.fillMaxWidth().widthIn(max = 360.dp), + ) { + Text(stringResource(R.string.onboarding_contacts_not_now)) + } + } + } + } +} diff --git a/app/src/main/kotlin/org/libremail/ui/onboarding/OnboardingViewModel.kt b/app/src/main/kotlin/org/libremail/ui/onboarding/OnboardingViewModel.kt index bc8706b..ab55a2d 100644 --- a/app/src/main/kotlin/org/libremail/ui/onboarding/OnboardingViewModel.kt +++ b/app/src/main/kotlin/org/libremail/ui/onboarding/OnboardingViewModel.kt @@ -9,6 +9,7 @@ import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.launch +import org.libremail.contacts.ContactsPermissionManager import org.libremail.data.settings.SettingsRepository import org.libremail.push.BatteryOptimizationManager import org.libremail.push.BatteryPromptDecision @@ -19,11 +20,13 @@ import javax.inject.Inject * entry, so it is created when onboarding starts and cleared when the graph is popped. * * It remembers the **first** account added this session (so finishing opens that account's inbox, see - * #30) and decides whether to show the "unrestricted battery" opt-in step before finishing (see #49). + * #30) and decides which optional opt-in steps to show before finishing: the "contacts access" step + * for recipient autocomplete (#127) and the "unrestricted battery" step for instant push (#49). */ @HiltViewModel class OnboardingViewModel @Inject constructor( private val batteryOptimizationManager: BatteryOptimizationManager, + private val contactsPermissionManager: ContactsPermissionManager, private val settingsRepository: SettingsRepository, ) : ViewModel() { @@ -45,6 +48,20 @@ class OnboardingViewModel @Inject constructor( /** Live "Unrestricted" status, re-read when the opt-in step resumes (e.g. back from Settings). */ val batteryUnrestricted: StateFlow = _batteryUnrestricted.asStateFlow() + private val _contactsPromptNeeded = MutableStateFlow(null) + + /** + * Whether onboarding should show the optional contacts-access step. `null` until decided; like + * [batteryPromptNeeded] the finish path treats `null` as "skip". Offered only when the permission + * isn't already granted and the user hasn't already handled the step on a previous onboarding run. + */ + val contactsPromptNeeded: StateFlow = _contactsPromptNeeded.asStateFlow() + + private val _contactsGranted = MutableStateFlow(contactsPermissionManager.hasPermission()) + + /** Live `READ_CONTACTS` grant, re-read when the contacts step resumes and after a request result. */ + val contactsGranted: StateFlow = _contactsGranted.asStateFlow() + init { viewModelScope.launch { val unrestricted = batteryOptimizationManager.isIgnoringBatteryOptimizations() @@ -55,6 +72,11 @@ class OnboardingViewModel @Inject constructor( alreadyHandled = settingsRepository.isBatteryPromptHandled(), ) } + viewModelScope.launch { + _contactsPromptNeeded.value = + !contactsPermissionManager.hasPermission() && + !settingsRepository.isContactsPromptHandled() + } } /** Records a freshly added account. Only the first one sticks — later adds don't overwrite it. */ @@ -78,4 +100,27 @@ class OnboardingViewModel @Inject constructor( fun markBatteryPromptHandled() { viewModelScope.launch { settingsRepository.setBatteryPromptHandled(true) } } + + /** Re-read the live `READ_CONTACTS` grant; call when the contacts step resumes. */ + fun refreshContactsStatus() { + _contactsGranted.value = contactsPermissionManager.hasPermission() + } + + /** Fold the result of the system contacts-permission dialog back into [contactsGranted]. */ + fun onContactsPermissionResult(granted: Boolean) { + _contactsGranted.value = granted + } + + /** + * Record that the `READ_CONTACTS` system dialog is about to be (or has been) shown, so a later + * permanent denial is distinguishable from "never asked" in Settings (#129). Call before launching. + */ + fun markContactsPermissionRequested() { + viewModelScope.launch { settingsRepository.setContactsPermissionRequested(true) } + } + + /** Record that the user has seen/acted on the contacts opt-in so onboarding won't ask again. */ + fun markContactsPromptHandled() { + viewModelScope.launch { settingsRepository.setContactsPromptHandled(true) } + } } diff --git a/app/src/main/kotlin/org/libremail/ui/reader/HtmlBody.kt b/app/src/main/kotlin/org/libremail/ui/reader/HtmlBody.kt index 112d9d2..e61ab7c 100644 --- a/app/src/main/kotlin/org/libremail/ui/reader/HtmlBody.kt +++ b/app/src/main/kotlin/org/libremail/ui/reader/HtmlBody.kt @@ -4,6 +4,7 @@ package org.libremail.ui.reader import android.annotation.SuppressLint import android.content.Intent import android.webkit.WebResourceRequest +import android.webkit.WebResourceResponse import android.webkit.WebSettings import android.webkit.WebView import android.webkit.WebViewClient @@ -19,12 +20,18 @@ import androidx.compose.ui.platform.LocalContext import androidx.compose.ui.viewinterop.AndroidView import androidx.webkit.WebSettingsCompat import androidx.webkit.WebViewFeature +import org.libremail.domain.model.InlineImage +import java.io.ByteArrayInputStream /** * Renders an HTML email body in a hardened WebView: JavaScript and file/content access are * disabled, links open in the system browser, and remote content is blocked until the user * opts in (tracking-pixel protection). * + * Inline images the email carries itself (``, resolved via [inlineImages]) are + * always served — they are embedded content, not a remote fetch, so they render even while remote + * images are blocked. + * * The email is wrapped with an explicit background/text/link color drawn from the active Material * theme so it is always readable — in dark mode the previous transparent WebView showed the * near-black app surface through emails whose own CSS left the text at the browser default of @@ -33,7 +40,12 @@ import androidx.webkit.WebViewFeature */ @SuppressLint("SetJavaScriptEnabled") @Composable -fun HtmlBody(html: String, loadRemoteImages: Boolean, modifier: Modifier = Modifier) { +fun HtmlBody( + html: String, + loadRemoteImages: Boolean, + inlineImages: Map, + modifier: Modifier = Modifier, +) { val context = LocalContext.current val colorScheme = MaterialTheme.colorScheme val surface = colorScheme.surface @@ -50,10 +62,15 @@ fun HtmlBody(html: String, loadRemoteImages: Boolean, modifier: Modifier = Modif dark = isDark, ) } + // A mutable holder the WebViewClient reads on the (background) interception thread, kept current + // by the update block so inline images that arrive after the first composition are resolvable. + val imageHolder = remember { InlineImageHolder() } + imageHolder.images = inlineImages // Tracks the content actually loaded so recompositions (star/attachment state changes) don't // reload the page and throw away the user's scroll position. Keyed on the fully wrapped - // document so a theme (light/dark) change still re-renders with the new colors. - val lastLoaded = remember { mutableStateOf?>(null) } + // document so a theme (light/dark) change still re-renders with the new colors, and on the set + // of available cid: keys so the page reloads once when inline images finish resolving. + val lastLoaded = remember { mutableStateOf>?>(null) } AndroidView( modifier = modifier, factory = { ctx -> @@ -72,6 +89,17 @@ fun HtmlBody(html: String, loadRemoteImages: Boolean, modifier: Modifier = Modif applyAlgorithmicDarkening(isDark) isVerticalScrollBarEnabled = true webViewClient = object : WebViewClient() { + override fun shouldInterceptRequest( + view: WebView?, + request: WebResourceRequest?, + ): WebResourceResponse? { + // Serve inline images the email embedded itself (cid:) from the message's own + // parts; everything else falls through to normal (remote-blockable) loading. + val image = request?.url?.toString()?.let { resolveInlineImage(it, imageHolder.images) } + ?: return null + return WebResourceResponse(image.mimeType, null, ByteArrayInputStream(image.bytes)) + } + override fun shouldOverrideUrlLoading(view: WebView?, request: WebResourceRequest?): Boolean { val url = request?.url ?: return false // Only open ordinary web/mail links, and only on an actual user tap — never @@ -96,7 +124,7 @@ fun HtmlBody(html: String, loadRemoteImages: Boolean, modifier: Modifier = Modif webView.setBackgroundColor(surfaceArgb) webView.applyAlgorithmicDarkening(isDark) webView.settings.blockNetworkLoads = !loadRemoteImages - val key = document to loadRemoteImages + val key = Triple(document, loadRemoteImages, inlineImages.keys.toSet()) if (lastLoaded.value != key) { lastLoaded.value = key webView.loadDataWithBaseURL(null, document, "text/html", "UTF-8", null) @@ -105,6 +133,27 @@ fun HtmlBody(html: String, loadRemoteImages: Boolean, modifier: Modifier = Modif ) } +/** Mutable, thread-visible reference to the current inline images (read from the interception thread). */ +private class InlineImageHolder { + @Volatile + var images: Map = emptyMap() +} + +/** + * Extracts and normalizes the `Content-ID` from a `cid:` URL (surrounding angle brackets stripped), + * or null when [url] is not a `cid:` reference. Kept separate from Android types so it is unit-testable. + */ +internal fun cidKey(url: String): String? { + if (!url.startsWith("cid:", ignoreCase = true)) return null + return url.substring(CID_PREFIX_LENGTH).trim().trim('<', '>').trim().takeUnless { it.isBlank() } +} + +/** Resolves a `cid:` [url] to its [InlineImage] among [images] (keyed by normalized Content-ID), or null. */ +internal fun resolveInlineImage(url: String, images: Map): InlineImage? = + cidKey(url)?.let { images[it] } + +private const val CID_PREFIX_LENGTH = 4 + /** * Lets the WebView algorithmically darken email content that does not declare its own dark support, * but only in dark mode and only where the installed WebView supports the feature. This is a diff --git a/app/src/main/kotlin/org/libremail/ui/reader/ReaderScreen.kt b/app/src/main/kotlin/org/libremail/ui/reader/ReaderScreen.kt index 4b3895b..0226fcd 100644 --- a/app/src/main/kotlin/org/libremail/ui/reader/ReaderScreen.kt +++ b/app/src/main/kotlin/org/libremail/ui/reader/ReaderScreen.kt @@ -64,6 +64,7 @@ import androidx.hilt.navigation.compose.hiltViewModel import androidx.lifecycle.compose.collectAsStateWithLifecycle import org.libremail.R import org.libremail.domain.model.Attachment +import org.libremail.domain.model.InlineImage import org.libremail.domain.model.Message import java.io.File @@ -147,6 +148,7 @@ fun ReaderScreen( message != null -> MessageBody( message = message, attachments = state.attachments, + inlineImages = state.inlineImages, downloading = state.downloading, downloaded = state.downloaded, onDownloadAttachment = viewModel::downloadAttachment, @@ -166,6 +168,7 @@ fun ReaderScreen( private fun MessageBody( message: Message, attachments: List, + inlineImages: Map, downloading: Set, downloaded: Set, onDownloadAttachment: (Attachment) -> Unit, @@ -194,6 +197,7 @@ private fun MessageBody( message.isHtml -> HtmlBody( html = message.body, loadRemoteImages = loadRemoteImages, + inlineImages = inlineImages, modifier = Modifier.fillMaxSize(), ) diff --git a/app/src/main/kotlin/org/libremail/ui/reader/ReaderViewModel.kt b/app/src/main/kotlin/org/libremail/ui/reader/ReaderViewModel.kt index 8c90726..f31ff7a 100644 --- a/app/src/main/kotlin/org/libremail/ui/reader/ReaderViewModel.kt +++ b/app/src/main/kotlin/org/libremail/ui/reader/ReaderViewModel.kt @@ -15,6 +15,7 @@ import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch import org.libremail.data.settings.SettingsRepository import org.libremail.domain.model.Attachment +import org.libremail.domain.model.InlineImage import org.libremail.domain.model.Message import org.libremail.domain.repository.MailRepository import org.libremail.ui.navigation.Routes @@ -25,6 +26,8 @@ data class ReaderUiState( val loading: Boolean = true, val message: Message? = null, val attachments: List = emptyList(), + /** Inline `cid:` images for the HTML body, keyed by normalized Content-ID (see [HtmlBody]). */ + val inlineImages: Map = emptyMap(), val downloading: Set = emptySet(), /** Part indexes whose bytes are already cached on disk (openable offline). */ val downloaded: Set = emptySet(), @@ -63,7 +66,15 @@ class ReaderViewModel @Inject constructor( } viewModelScope.launch { repository.openMessage(messageId).fold( - onSuccess = { message -> _state.update { it.copy(loading = false, message = message) } }, + onSuccess = { message -> + _state.update { it.copy(loading = false, message = message) } + // Resolve inline cid: images so the WebView can embed them. Runs after openMessage + // has cached the parts; skipped for plain-text mail and messages with none. + if (message.isHtml) { + val images = repository.inlineImages(messageId).associateBy { it.contentId } + if (images.isNotEmpty()) _state.update { it.copy(inlineImages = images) } + } + }, onFailure = { e -> _state.update { it.copy( diff --git a/app/src/main/kotlin/org/libremail/ui/settings/SettingsScreen.kt b/app/src/main/kotlin/org/libremail/ui/settings/SettingsScreen.kt index d238929..da70170 100644 --- a/app/src/main/kotlin/org/libremail/ui/settings/SettingsScreen.kt +++ b/app/src/main/kotlin/org/libremail/ui/settings/SettingsScreen.kt @@ -1,6 +1,10 @@ // SPDX-License-Identifier: GPL-3.0-or-later package org.libremail.ui.settings +import android.Manifest +import androidx.activity.compose.LocalActivity +import androidx.activity.compose.rememberLauncherForActivityResult +import androidx.activity.result.contract.ActivityResultContracts import androidx.compose.animation.AnimatedVisibility import androidx.compose.foundation.clickable import androidx.compose.foundation.layout.Column @@ -12,6 +16,7 @@ import androidx.compose.foundation.rememberScrollState import androidx.compose.foundation.verticalScroll import androidx.compose.material.icons.Icons import androidx.compose.material.icons.filled.ArrowDropDown +import androidx.compose.material3.AlertDialog import androidx.compose.material3.ExperimentalMaterial3Api import androidx.compose.material3.HorizontalDivider import androidx.compose.material3.Icon @@ -20,11 +25,14 @@ import androidx.compose.material3.Scaffold import androidx.compose.material3.SnackbarHost import androidx.compose.material3.SnackbarHostState import androidx.compose.material3.Text +import androidx.compose.material3.TextButton import androidx.compose.material3.TopAppBar import androidx.compose.runtime.Composable import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember +import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.draw.rotate @@ -32,11 +40,14 @@ import androidx.compose.ui.platform.LocalContext import androidx.compose.ui.platform.LocalResources import androidx.compose.ui.res.stringResource import androidx.compose.ui.unit.dp +import androidx.core.app.ActivityCompat import androidx.hilt.navigation.compose.hiltViewModel import androidx.lifecycle.Lifecycle import androidx.lifecycle.compose.LifecycleEventEffect import androidx.lifecycle.compose.collectAsStateWithLifecycle import org.libremail.R +import org.libremail.contacts.ContactPermissionDecision +import org.libremail.contacts.ContactPermissionState import org.libremail.data.settings.FetchPolicy import org.libremail.ui.LibreMailBottomBar import org.libremail.ui.TopDest @@ -56,9 +67,28 @@ fun SettingsScreen( val appLockMessage by viewModel.appLockMessage.collectAsStateWithLifecycle() val batteryUnrestricted by viewModel.batteryUnrestricted.collectAsStateWithLifecycle() val context = LocalContext.current + val activity = LocalActivity.current val resources = LocalResources.current val snackbarHostState = remember { SnackbarHostState() } + // Contacts-autocomplete entry (#129): its on / off / blocked-in-settings state is derived from the + // live grant, the Activity's rationale signal, and whether the dialog was ever shown — recomputed + // on resume (e.g. back from system settings) and when the "requested" flag flips. + val contactsRequested by viewModel.contactsPermissionRequested.collectAsStateWithLifecycle() + var contactsState by remember { mutableStateOf(ContactPermissionState.DENIED) } + var showContactsRationale by remember { mutableStateOf(false) } + var showContactsBlocked by remember { mutableStateOf(false) } + fun resolveContactsState() = ContactPermissionDecision.resolve( + granted = viewModel.hasContactsPermission(), + showRationale = activity != null && + ActivityCompat.shouldShowRequestPermissionRationale(activity, Manifest.permission.READ_CONTACTS), + alreadyRequested = contactsRequested, + ) + val contactsPermissionLauncher = rememberLauncherForActivityResult( + ActivityResultContracts.RequestPermission(), + ) { contactsState = resolveContactsState() } + LaunchedEffect(contactsRequested) { contactsState = resolveContactsState() } + // Surface a rejected app-lock toggle via the canonical snackbar pattern (matches MailboxScreen). // The ViewModel holds the @StringRes id; resolve it here via LocalResources (so it re-resolves on // configuration changes) at the display boundary, then clear it. @@ -69,8 +99,11 @@ fun SettingsScreen( } } - // Re-read the battery status on resume so it reflects any change made in system settings. - LifecycleEventEffect(Lifecycle.Event.ON_RESUME) { viewModel.refreshBatteryStatus() } + // Re-read the battery + contacts state on resume so both reflect changes made in system settings. + LifecycleEventEffect(Lifecycle.Event.ON_RESUME) { + viewModel.refreshBatteryStatus() + contactsState = resolveContactsState() + } Scaffold( topBar = { TopAppBar(title = { Text(stringResource(R.string.title_settings)) }) }, @@ -108,6 +141,23 @@ fun SettingsScreen( ) HorizontalDivider() + SectionHeader(stringResource(R.string.settings_contacts)) + ContactAutocompleteRow( + state = contactsState, + onClick = { + when (contactsState) { + // Already on: send to system settings, the only place to turn it back off. + ContactPermissionState.GRANTED -> + runCatching { context.startActivity(viewModel.contactsSettingsIntent()) } + // Re-requestable in-app: explain first (#128), then launch the system dialog. + ContactPermissionState.DENIED -> showContactsRationale = true + // Permanently denied: an in-app request is a no-op, so deep-link to settings. + ContactPermissionState.BLOCKED -> showContactsBlocked = true + } + }, + ) + HorizontalDivider() + SectionHeader(stringResource(R.string.settings_appearance)) SwitchRow( title = stringResource(R.string.settings_dynamic_color), @@ -211,6 +261,69 @@ fun SettingsScreen( } } } + + if (showContactsRationale) { + ContactsPermissionDialog( + title = stringResource(R.string.settings_contacts_dialog_title), + body = stringResource(R.string.settings_contacts_rationale), + confirm = stringResource(R.string.settings_contacts_allow), + onConfirm = { + showContactsRationale = false + // Mark the dialog as shown BEFORE launching, so a permanent denial reads as "blocked". + viewModel.markContactsPermissionRequested() + contactsPermissionLauncher.launch(Manifest.permission.READ_CONTACTS) + }, + onDismiss = { showContactsRationale = false }, + ) + } + if (showContactsBlocked) { + ContactsPermissionDialog( + title = stringResource(R.string.settings_contacts_dialog_title), + body = stringResource(R.string.settings_contacts_blocked_body), + confirm = stringResource(R.string.settings_contacts_open_settings), + onConfirm = { + showContactsBlocked = false + runCatching { context.startActivity(viewModel.contactsSettingsIntent()) } + }, + onDismiss = { showContactsBlocked = false }, + ) + } +} + +/** + * The contacts-autocomplete row (#129). Its subtitle reflects the current [state]: on, off (tap to + * turn on), or blocked in system settings. Extracted so each state renders deterministically in tests. + */ +@Composable +internal fun ContactAutocompleteRow(state: ContactPermissionState, onClick: () -> Unit) { + val subtitleRes = when (state) { + ContactPermissionState.GRANTED -> R.string.settings_contacts_autocomplete_on + ContactPermissionState.DENIED -> R.string.settings_contacts_autocomplete_off + ContactPermissionState.BLOCKED -> R.string.settings_contacts_autocomplete_blocked + } + ClickRow( + title = stringResource(R.string.settings_contacts_autocomplete), + subtitle = stringResource(subtitleRes), + onClick = onClick, + ) +} + +/** Shared confirm/cancel dialog for the contacts rationale (before a request) and the blocked case. */ +@Composable +private fun ContactsPermissionDialog( + title: String, + body: String, + confirm: String, + onConfirm: () -> Unit, + onDismiss: () -> Unit, +) { + AlertDialog( + onDismissRequest = onDismiss, + title = { Text(title) }, + text = { Text(body) }, + confirmButton = { TextButton(onClick = onConfirm) { Text(confirm) } }, + dismissButton = { TextButton(onClick = onDismiss) { Text(stringResource(R.string.cancel)) } }, + ) } @Composable diff --git a/app/src/main/kotlin/org/libremail/ui/settings/SettingsViewModel.kt b/app/src/main/kotlin/org/libremail/ui/settings/SettingsViewModel.kt index a838b88..043bb80 100644 --- a/app/src/main/kotlin/org/libremail/ui/settings/SettingsViewModel.kt +++ b/app/src/main/kotlin/org/libremail/ui/settings/SettingsViewModel.kt @@ -13,6 +13,7 @@ import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch import org.libremail.R +import org.libremail.contacts.ContactsPermissionManager import org.libremail.data.security.AppLockManager import org.libremail.data.security.DatabaseKeyStore import org.libremail.data.settings.AppSettings @@ -31,6 +32,7 @@ class SettingsViewModel @Inject constructor( private val appLockManager: AppLockManager, private val databaseKeyStore: DatabaseKeyStore, private val batteryOptimizationManager: BatteryOptimizationManager, + private val contactsPermissionManager: ContactsPermissionManager, private val syncScheduler: SyncScheduler, ) : ViewModel() { @@ -40,6 +42,14 @@ class SettingsViewModel @Inject constructor( val settings: StateFlow = settingsRepository.settings .stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), AppSettings()) + /** + * Whether the `READ_CONTACTS` system dialog has ever been shown, so the contacts entry (#129) can + * tell "never asked" (an in-app request still works) from "permanently denied" (Settings only). + * See [ContactPermissionDecision][org.libremail.contacts.ContactPermissionDecision]. + */ + val contactsPermissionRequested: StateFlow = settingsRepository.contactsPermissionRequested + .stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), false) + private val _advancedExpanded = MutableStateFlow(false) val advancedExpanded: StateFlow = _advancedExpanded.asStateFlow() @@ -62,6 +72,15 @@ class SettingsViewModel @Inject constructor( /** Intent to the system screen where the user flips this app to "Unrestricted". */ fun batterySettingsIntent(): Intent = batteryOptimizationManager.settingsIntent() + /** Whether `READ_CONTACTS` is currently granted (drives the contacts-autocomplete row's state). */ + fun hasContactsPermission(): Boolean = contactsPermissionManager.hasPermission() + + /** Intent to this app's system details screen, to enable contacts when it's permanently denied. */ + fun contactsSettingsIntent(): Intent = contactsPermissionManager.settingsIntent() + + /** Persist that the contacts dialog is being shown, so a later denial reads as "blocked", not "off". */ + fun markContactsPermissionRequested() = update { settingsRepository.setContactsPermissionRequested(true) } + fun setDynamicColor(value: Boolean) = update { settingsRepository.setDynamicColor(value) } fun setNewMailNotifications(value: Boolean) = update { settingsRepository.setNewMailNotifications(value) } fun setPushIdle(value: Boolean) = update { settingsRepository.setPushIdle(value) } diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 687db18..df95327 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -159,6 +159,16 @@ Background usage is unrestricted — new mail will arrive instantly. Continue to inbox + + Suggest recipients as you type + Allow access to your contacts and LibreMail will suggest matching names and email addresses while you compose. This happens entirely on your device — your contacts are never uploaded or shared. It\'s optional; you can skip it and enter addresses yourself. + LibreMail needs the Contacts permission to suggest recipients. It\'s used only for on-device autocomplete — nothing is uploaded. + Allow contacts access + Not now + Autocomplete is on + LibreMail will suggest recipients from your contacts as you compose — all on this device. + Continue + Outlook or Hotmail Other (IMAP/SMTP) @@ -296,6 +306,18 @@ Unrestricted — instant background mail is allowed. Optimized by Android — new mail may be delayed. Tap to allow unrestricted background usage. + + Contacts + Recipient autocomplete + On — suggesting recipients from your contacts as you type. Tap to manage. + Off — tap to suggest recipients from your contacts. On-device only; never uploaded. + Blocked in system settings — tap to open settings and allow Contacts access. + Recipient autocomplete + LibreMail suggests recipients from your device contacts as you compose. This happens entirely on your device — your contacts are never uploaded or shared. + Allow + Contacts access is turned off for LibreMail in Android settings. Open settings and allow Contacts to enable recipient autocomplete. + Open settings + Diagnostics Report a problem diff --git a/app/src/main/res/xml/backup_rules.xml b/app/src/main/res/xml/backup_rules.xml index 0059fd1..ce18508 100644 --- a/app/src/main/res/xml/backup_rules.xml +++ b/app/src/main/res/xml/backup_rules.xml @@ -5,10 +5,11 @@ which applies on API 31+. Backup is still gated by LibreMailBackupAgent (opt-in, OFF by default). makes this a strict allowlist: ONLY the libremail_settings DataStore is backed up. The - Keystore-sealed cache passphrase (datastore/libremail_dbkey.preferences_pb) and the encrypted - credentials + mail-cache database (libremail.db and its -wal/-shm/-journal side files) are kept - off-device by being omitted from the allowlist; the mail cache re-downloads on the next sync and - accounts are re-added on a new device. (Lint's FullBackupContent rule forbids paths + Keystore-sealed cache passphrase (datastore/libremail_dbkey.preferences_pb), the mail-cache + database (libremail.db) and the accounts + encrypted-credentials database (libremail-accounts.db, + which issue #111 split out of libremail.db) — each with their -wal/-shm/-journal side files — are + kept off-device by being omitted from the allowlist; the mail cache re-downloads on the next sync + and accounts are re-added on a new device. (Lint's FullBackupContent rule forbids paths outside an , so exclusion is expressed by omission rather than explicit entries.) --> diff --git a/app/src/main/res/xml/data_extraction_rules.xml b/app/src/main/res/xml/data_extraction_rules.xml index 0f07896..d3ca8b0 100644 --- a/app/src/main/res/xml/data_extraction_rules.xml +++ b/app/src/main/res/xml/data_extraction_rules.xml @@ -11,8 +11,11 @@ - datastore/libremail_dbkey.preferences_pb: the Keystore-sealed SQLCipher passphrase for the encrypted cache. The wrapping Keystore key is non-exportable and device-bound, so the ciphertext is useless anywhere else. - - libremail.db (+ -wal/-shm/-journal): encrypted IMAP/OAuth credentials and the cached mail. - The cache re-downloads on the next sync; accounts are re-added on a new device. + - libremail.db (+ -wal/-shm/-journal): the cached mail; re-downloads on the next sync. + - libremail-accounts.db (+ -wal/-shm/-journal): accounts, encrypted IMAP/OAuth credentials, + per-account settings and signatures (issue #111 moved these out of libremail.db). The + credentials are sealed with a device-bound key, so they would only ever restore as + undecryptable ciphertext; accounts are re-added on a new device. (Lint's FullBackupContent rule forbids paths outside an , so the exclusions are expressed by simply not listing those paths rather than as explicit entries.) --> diff --git a/app/src/test/kotlin/org/libremail/backup/BackupPolicyTest.kt b/app/src/test/kotlin/org/libremail/backup/BackupPolicyTest.kt index c9d0924..c60e33d 100644 --- a/app/src/test/kotlin/org/libremail/backup/BackupPolicyTest.kt +++ b/app/src/test/kotlin/org/libremail/backup/BackupPolicyTest.kt @@ -2,6 +2,7 @@ package org.libremail.backup import org.junit.Test +import org.libremail.data.local.DatabaseFiles import org.libremail.data.settings.AppSettings import kotlin.test.assertEquals import kotlin.test.assertFalse @@ -37,10 +38,35 @@ class BackupPolicyTest { } @Test - fun `the credentials and mail-cache database is never eligible for backup`() { + fun `the mail-cache database is never eligible for backup`() { assertTrue(BackupPolicy.EXCLUDED_DATABASE_PATHS.contains("libremail.db")) // WAL/SHM/journal side-files can hold recently written rows too. assertTrue(BackupPolicy.EXCLUDED_DATABASE_PATHS.contains("libremail.db-wal")) assertTrue(BackupPolicy.EXCLUDED_DATABASE_PATHS.contains("libremail.db-shm")) + assertTrue(BackupPolicy.EXCLUDED_DATABASE_PATHS.contains("libremail.db-journal")) + } + + @Test + fun `the accounts and credentials database is never eligible for backup`() { + // Since #111 the accounts + encrypted IMAP/OAuth credentials live in their OWN database file + // (libremail-accounts.db), so it must be in the never-back-up set just like the cache. + assertTrue(BackupPolicy.EXCLUDED_DATABASE_PATHS.contains("libremail-accounts.db")) + assertTrue(BackupPolicy.EXCLUDED_DATABASE_PATHS.contains("libremail-accounts.db-wal")) + assertTrue(BackupPolicy.EXCLUDED_DATABASE_PATHS.contains("libremail-accounts.db-shm")) + assertTrue(BackupPolicy.EXCLUDED_DATABASE_PATHS.contains("libremail-accounts.db-journal")) + } + + @Test + fun `excluded database paths are derived from DatabaseFiles so none can silently fall out`() { + val derived = DatabaseFiles.fileNames(DatabaseFiles.NAME) + + DatabaseFiles.fileNames(DatabaseFiles.ACCOUNTS_NAME) + // Derived, not hand-maintained: the exclusion set is exactly the DatabaseFiles-known files — + // no more (nothing stale) and no less (every DB + sidecar covered). + assertEquals(derived, BackupPolicy.EXCLUDED_DATABASE_PATHS) + listOf(DatabaseFiles.NAME, DatabaseFiles.ACCOUNTS_NAME).forEach { name -> + DatabaseFiles.fileNames(name).forEach { path -> + assertTrue(path in BackupPolicy.EXCLUDED_DATABASE_PATHS, "$path must never be backed up") + } + } } } diff --git a/app/src/test/kotlin/org/libremail/backup/DataExtractionRulesTest.kt b/app/src/test/kotlin/org/libremail/backup/DataExtractionRulesTest.kt index 2de642c..e70a1c5 100644 --- a/app/src/test/kotlin/org/libremail/backup/DataExtractionRulesTest.kt +++ b/app/src/test/kotlin/org/libremail/backup/DataExtractionRulesTest.kt @@ -2,6 +2,7 @@ package org.libremail.backup import org.junit.Test +import org.libremail.data.local.DatabaseFiles import org.w3c.dom.Element import java.io.File import javax.xml.parsers.DocumentBuilderFactory @@ -73,4 +74,13 @@ class DataExtractionRulesTest { fun `full backup content (API 29-30) mirrors the same exclusions`() { assertSafe(parseSection(resource("backup_rules.xml"), "full-backup-content")) } + + @Test + fun `both the cache and accounts databases are guarded against the backup allowlist`() { + // The exclusion SoT is derived from DatabaseFiles, so both databases flow into secretPaths + // and are asserted-absent from every include section by assertSafe above. Pin that here so + // the accounts + credentials DB added in #111 can't quietly drop out of the guarded set. + assertTrue(secretPaths.contains("database:${DatabaseFiles.NAME}")) + assertTrue(secretPaths.contains("database:${DatabaseFiles.ACCOUNTS_NAME}")) + } } diff --git a/app/src/test/kotlin/org/libremail/contacts/ContactPermissionDecisionTest.kt b/app/src/test/kotlin/org/libremail/contacts/ContactPermissionDecisionTest.kt new file mode 100644 index 0000000..a78077e --- /dev/null +++ b/app/src/test/kotlin/org/libremail/contacts/ContactPermissionDecisionTest.kt @@ -0,0 +1,52 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.contacts + +import org.junit.Test +import kotlin.test.assertEquals + +class ContactPermissionDecisionTest { + + @Test + fun `granted is always ON, regardless of the other signals`() { + for (rationale in listOf(false, true)) { + for (requested in listOf(false, true)) { + val state = ContactPermissionDecision.resolve( + granted = true, + showRationale = rationale, + alreadyRequested = requested, + ) + assertEquals( + ContactPermissionState.GRANTED, + state, + "granted=true must always be GRANTED (rationale=$rationale, requested=$requested)", + ) + } + } + } + + @Test + fun `denied once with a rationale owed is re-requestable (DENIED)`() { + assertEquals( + ContactPermissionState.DENIED, + ContactPermissionDecision.resolve(granted = false, showRationale = true, alreadyRequested = true), + ) + } + + @Test + fun `never asked yet is re-requestable (DENIED), not blocked`() { + // No rationale AND never requested = a fresh install that simply hasn't asked; a request works. + assertEquals( + ContactPermissionState.DENIED, + ContactPermissionDecision.resolve(granted = false, showRationale = false, alreadyRequested = false), + ) + } + + @Test + fun `permanently denied is BLOCKED`() { + // Requested before, no rationale now, still not granted = "don't ask again" — Settings only. + assertEquals( + ContactPermissionState.BLOCKED, + ContactPermissionDecision.resolve(granted = false, showRationale = false, alreadyRequested = true), + ) + } +} diff --git a/app/src/test/kotlin/org/libremail/data/local/DatabaseProvisionerTest.kt b/app/src/test/kotlin/org/libremail/data/local/DatabaseProvisionerTest.kt new file mode 100644 index 0000000..7d2fa26 --- /dev/null +++ b/app/src/test/kotlin/org/libremail/data/local/DatabaseProvisionerTest.kt @@ -0,0 +1,193 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.local + +import android.content.Context +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.mockkObject +import io.mockk.unmockkAll +import io.mockk.verify +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.ExecutorCoroutineDispatcher +import kotlinx.coroutines.asCoroutineDispatcher +import kotlinx.coroutines.async +import kotlinx.coroutines.awaitAll +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.security.DatabaseKeyStore +import org.libremail.data.settings.AppSettings +import org.libremail.data.settings.SettingsRepository +import java.io.File +import java.util.concurrent.Executors +import kotlin.test.assertEquals + +/** + * [DatabaseProvisioner] holds the one-time startup sequence that `DatabaseModule.provideDatabase` used + * to run inline while Hilt constructed the database (a DataStore read, a Keystore op, a possible + * SQLCipher re-key conversion, and the issue-#111 account migrator) — synchronously on whichever thread + * injected it, possibly the main thread. These tests pin down what moving that work behind + * [DatabaseProvisioner.prepareCache] must preserve (issue #93): the same ordering (wipe -> migrate -> + * encryption gate), the same branch behaviour, single-run memoization, and that the blocking work runs + * on the injected IO dispatcher rather than the caller's thread. + * + * The native/file collaborators ([DatabaseEncryption], [DatabaseFiles]) and the suspend collaborators + * are all mocked, so this exercises the orchestration without a device. + */ +class DatabaseProvisionerTest { + + private val context = mockk() + private val keyStore = mockk() + private val settingsRepository = mockk() + private val accountDataMigrator = mockk() + private lateinit var ioDispatcher: ExecutorCoroutineDispatcher + + @Before + fun setUp() { + ioDispatcher = Executors.newSingleThreadExecutor { runnable -> Thread(runnable, IO_THREAD_NAME) } + .asCoroutineDispatcher() + mockkObject(DatabaseEncryption) + mockkObject(DatabaseFiles) + + every { context.getDatabasePath(any()) } returns File("libremail.db") + every { DatabaseFiles.clear(any()) } just Runs + every { DatabaseEncryption.isEncrypted(any()) } returns false + every { DatabaseEncryption.ensureEncrypted(any(), any()) } just Runs + every { DatabaseEncryption.ensurePlaintext(any(), any()) } just Runs + every { settingsRepository.settings } returns flowOf(AppSettings()) + + coEvery { keyStore.isClearPending() } returns false + coEvery { keyStore.resetSealedPassphrase() } just Runs + coEvery { keyStore.clearClearPending() } just Runs + coEvery { keyStore.resolvePassphrase(any()) } returns PASSPHRASE + coEvery { accountDataMigrator.migrateIfNeeded() } just Runs + } + + @After + fun tearDown() { + ioDispatcher.close() + unmockkAll() + } + + private fun provisioner() = + DatabaseProvisioner(context, keyStore, settingsRepository, accountDataMigrator, ioDispatcher) + + @Test + fun `an encrypted cache is converted and reports the SQLCipher passphrase`() = runTest { + every { settingsRepository.settings } returns flowOf(AppSettings(encryptCache = true, appLock = false)) + + val mode = provisioner().prepareCache() + + assertEquals(CacheOpenMode.Encrypted(PASSPHRASE), mode) + coVerify(exactly = 1) { keyStore.resolvePassphrase(false) } + verify(exactly = 1) { DatabaseEncryption.ensureEncrypted(any(), PASSPHRASE) } + verify(exactly = 0) { DatabaseEncryption.ensurePlaintext(any(), any()) } + } + + @Test + fun `a pending clear wipes and resets the seals before the migrator runs`() = runTest { + coEvery { keyStore.isClearPending() } returns true + + provisioner().prepareCache() + + // Crash-safe order preserved from the old provideDatabase: wipe + reset the seals, THEN clear the + // pending flag, THEN migrate — never touching the cache file after an open connection exists. + coVerifyOrder { + keyStore.isClearPending() + DatabaseFiles.clear(any()) + keyStore.resetSealedPassphrase() + keyStore.clearClearPending() + accountDataMigrator.migrateIfNeeded() + } + } + + @Test + fun `the account migrator runs before the cache encryption gate`() = runTest { + every { settingsRepository.settings } returns flowOf(AppSettings(encryptCache = true)) + + provisioner().prepareCache() + + // The #111 migrate-before-open guarantee: the account tables are copied out BEFORE the cache is + // touched (here, before its passphrase is resolved and it is re-keyed). + coVerifyOrder { + accountDataMigrator.migrateIfNeeded() + keyStore.resolvePassphrase(any()) + DatabaseEncryption.ensureEncrypted(any(), any()) + } + } + + @Test + fun `an encrypted file with encryption turned off is decrypted to plaintext`() = runTest { + every { settingsRepository.settings } returns flowOf(AppSettings(encryptCache = false)) + every { DatabaseEncryption.isEncrypted(any()) } returns true + + val mode = provisioner().prepareCache() + + assertEquals(CacheOpenMode.Plaintext, mode) + verify(exactly = 1) { DatabaseEncryption.ensurePlaintext(any(), PASSPHRASE) } + verify(exactly = 0) { DatabaseEncryption.ensureEncrypted(any(), any()) } + } + + @Test + fun `a plaintext cache with encryption off touches neither the passphrase nor a conversion`() = runTest { + val mode = provisioner().prepareCache() // defaults: encryptCache = false, file not encrypted + + assertEquals(CacheOpenMode.Plaintext, mode) + coVerify(exactly = 0) { keyStore.resolvePassphrase(any()) } + verify(exactly = 0) { DatabaseEncryption.ensureEncrypted(any(), any()) } + verify(exactly = 0) { DatabaseEncryption.ensurePlaintext(any(), any()) } + } + + @Test + fun `the startup sequence runs once and is memoized across calls`() = runTest { + val provisioner = provisioner() + + repeat(3) { provisioner.prepareCache() } + + coVerify(exactly = 1) { keyStore.isClearPending() } + coVerify(exactly = 1) { accountDataMigrator.migrateIfNeeded() } + verify(exactly = 1) { settingsRepository.settings } + } + + @Test + fun `concurrent first opens collapse to a single run`() = runTest { + val provisioner = provisioner() + + // Both databases opening at once each gate on prepareCache; the mutex must collapse them to one + // run of the migrator (opening the cache twice would be a correctness bug). + val first = async { provisioner.prepareCache() } + val second = async { provisioner.prepareCache() } + awaitAll(first, second) + + coVerify(exactly = 1) { accountDataMigrator.migrateIfNeeded() } + } + + @Test + fun `the blocking sequence runs on the injected io dispatcher, not the caller`() = runTest { + val migratorThread = CompletableDeferred() + coEvery { accountDataMigrator.migrateIfNeeded() } coAnswers { + migratorThread.complete(Thread.currentThread().name) + } + + provisioner().prepareCache() + + assertEquals( + IO_THREAD_NAME, + migratorThread.await(), + "the startup work must run on the injected IO dispatcher, off the calling thread", + ) + } + + private companion object { + // 64 hex chars == a 32-byte SQLCipher passphrase, matching DatabaseKeyStore's format. + const val PASSPHRASE = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" + const val IO_THREAD_NAME = "test-db-io-dispatcher" + } +} diff --git a/app/src/test/kotlin/org/libremail/data/local/DeferredOpenHelperFactoryTest.kt b/app/src/test/kotlin/org/libremail/data/local/DeferredOpenHelperFactoryTest.kt new file mode 100644 index 0000000..28fb505 --- /dev/null +++ b/app/src/test/kotlin/org/libremail/data/local/DeferredOpenHelperFactoryTest.kt @@ -0,0 +1,107 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.local + +import android.content.Context +import androidx.sqlite.db.SupportSQLiteDatabase +import androidx.sqlite.db.SupportSQLiteOpenHelper +import io.mockk.every +import io.mockk.mockk +import io.mockk.verify +import org.junit.Test +import java.util.concurrent.atomic.AtomicInteger +import kotlin.test.assertEquals +import kotlin.test.assertSame + +/** + * [DeferredOpenHelperFactory] is the seam that keeps the blocking startup gate off Room's build/inject + * path (issue #93): the operations Room performs while BUILDING the database — `create()` and + * `setWriteAheadLoggingEnabled()` — must not touch the real delegate, and therefore must not run the + * gate. The delegate materialises only when the database is first OPENED (`writableDatabase` / + * `readableDatabase`), which Room does on its background query executor. + */ +class DeferredOpenHelperFactoryTest { + + private fun configuration(name: String? = "test.db"): SupportSQLiteOpenHelper.Configuration { + val callback = object : SupportSQLiteOpenHelper.Callback(1) { + override fun onCreate(db: SupportSQLiteDatabase) = Unit + override fun onUpgrade(db: SupportSQLiteDatabase, oldVersion: Int, newVersion: Int) = Unit + } + return SupportSQLiteOpenHelper.Configuration.builder(mockk(relaxed = true)) + .name(name) + .callback(callback) + .build() + } + + @Test + fun `create and the build-time configuration calls never run the deferred gate`() { + val builds = AtomicInteger(0) + val factory = DeferredOpenHelperFactory { + builds.incrementAndGet() + mockk(relaxed = true) + } + + val helper = factory.create(configuration(name = "libremail.db")) + // Everything Room touches while building the database must stay cheap. + assertEquals("libremail.db", helper.databaseName) + helper.setWriteAheadLoggingEnabled(true) + helper.setWriteAheadLoggingEnabled(false) + + assertEquals(0, builds.get(), "building the database must not run the deferred startup gate") + } + + @Test + fun `the delegate is built only on first open and then reused`() { + val builds = AtomicInteger(0) + val delegate = mockk(relaxed = true) + val factory = DeferredOpenHelperFactory { + builds.incrementAndGet() + delegate + } + val helper = factory.create(configuration()) + assertEquals(0, builds.get()) + + // The first open materialises the delegate (and runs the gate exactly once)... + helper.writableDatabase + assertEquals(1, builds.get()) + // ...and every later access reuses it, never re-running the gate. + helper.writableDatabase + helper.readableDatabase + assertEquals(1, builds.get()) + } + + @Test + fun `a WAL setting made before the first open is applied when the delegate is built`() { + val delegate = mockk(relaxed = true) + val factory = DeferredOpenHelperFactory { delegate } + val helper = factory.create(configuration()) + + helper.setWriteAheadLoggingEnabled(true) // recorded, not forwarded — there is no delegate yet + verify(exactly = 0) { delegate.setWriteAheadLoggingEnabled(any()) } + + helper.writableDatabase // builds the delegate + verify(exactly = 1) { delegate.setWriteAheadLoggingEnabled(true) } + } + + @Test + fun `close before any open is a no-op that never builds the delegate`() { + val builds = AtomicInteger(0) + val factory = DeferredOpenHelperFactory { + builds.incrementAndGet() + mockk(relaxed = true) + } + + factory.create(configuration()).close() + + assertEquals(0, builds.get(), "closing a never-opened helper must not build the delegate") + } + + @Test + fun `writableDatabase delegates to the built helper`() { + val db = mockk(relaxed = true) + val delegate = mockk(relaxed = true) + every { delegate.writableDatabase } returns db + val factory = DeferredOpenHelperFactory { delegate } + + assertSame(db, factory.create(configuration()).writableDatabase) + } +} diff --git a/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt b/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt index ad92865..7aaeb95 100644 --- a/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt +++ b/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt @@ -417,6 +417,32 @@ class MailRepositoryImplTest { assertEquals(setOf(0), repository.downloadedAttachmentParts(id)) } + @Test + fun `inlineImages resolves cid parts to their cached bytes and excludes real attachments`() = runTest { + val cache = Files.createTempDirectory("attach").toFile() + every { context.cacheDir } returns cache + val id = "acct:INBOX:30" + coEvery { messageDao.getById(id) } returns messageEntity(id, "INBOX") + coEvery { attachmentDao.getForMessage(id) } returns listOf( + AttachmentEntity(id, 0, "logo.png", "image/png", 4, contentId = "logo1"), + AttachmentEntity(id, 1, "invoice.pdf", "application/pdf", 10, contentId = null), + ) + // The inline part's bytes are already cached, so no network fetch is needed. + File(cache, "attachments/acct_INBOX_30/0/logo.png").apply { + parentFile?.mkdirs() + writeBytes(byteArrayOf(9, 8, 7)) + } + + val images = repository.inlineImages(id) + + assertEquals(1, images.size) + assertEquals("logo1", images.first().contentId) + assertEquals("image/png", images.first().mimeType) + assertTrue(images.first().bytes.contentEquals(byteArrayOf(9, 8, 7))) + // The ordinary attachment (contentId == null) must never be pulled in as an inline image. + coVerify(exactly = 0) { imapClient.fetchAttachment(any(), any(), any(), any()) } + } + @Test fun `prefetchMessage caches the body and downloads attachments`() = runTest { val cache = Files.createTempDirectory("attach").toFile() diff --git a/app/src/test/kotlin/org/libremail/data/security/AesGcmKeystoreCipherTest.kt b/app/src/test/kotlin/org/libremail/data/security/AesGcmKeystoreCipherTest.kt new file mode 100644 index 0000000..b6438e8 --- /dev/null +++ b/app/src/test/kotlin/org/libremail/data/security/AesGcmKeystoreCipherTest.kt @@ -0,0 +1,121 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.security + +import android.security.keystore.KeyGenParameterSpec +import org.junit.Test +import java.security.GeneralSecurityException +import javax.crypto.AEADBadTagException +import javax.crypto.SecretKey +import javax.crypto.spec.SecretKeySpec +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertSame +import kotlin.test.assertTrue + +/** + * JVM coverage for the shared [AesGcmKeystoreCipher] wiring — specifically the deliberately different + * missing-key-on-decrypt policy the two production ciphers depend on, plus the AES-GCM error mapping. + * The Keystore-backed operations (real key generation and the GCM cipher) are device-only, so they + * are replaced here through the [existingKey], [getOrCreateKey], and [decryptWithKey] seams; what is + * pinned is the base's control flow: which key a decrypt resolves under each `generateKeyOnDecrypt` + * mode, and how a tag mismatch is surfaced. + */ +class AesGcmKeystoreCipherTest { + + @Test + fun `generateKeyOnDecrypt true mints a key for a missing alias (master-key behavior)`() { + val cipher = FakeCipher(generateKeyOnDecrypt = true, storedKey = null) + + assertEquals("plain:blob", cipher.decrypt("blob")) + assertEquals(1, cipher.generatedKeys, "a missing master alias is generated on decrypt") + } + + @Test + fun `generateKeyOnDecrypt true reuses an existing key without regenerating`() { + val cipher = FakeCipher(generateKeyOnDecrypt = true, storedKey = newAesKey()) + + assertEquals("plain:blob", cipher.decrypt("blob")) + assertEquals(0, cipher.generatedKeys) + } + + @Test + fun `generateKeyOnDecrypt false fails fast for a missing alias (auth-bound behavior)`() { + val cipher = FakeCipher(generateKeyOnDecrypt = false, storedKey = null) + + val error = assertFailsWith { cipher.decrypt("blob") } + assertTrue(error.message!!.contains("test.alias")) + assertEquals(0, cipher.generatedKeys, "an absent auth-bound key must NOT be silently regenerated") + assertEquals(0, cipher.decryptCalls, "decrypt short-circuits before touching the cipher") + } + + @Test + fun `generateKeyOnDecrypt false decrypts with the existing key`() { + val cipher = FakeCipher(generateKeyOnDecrypt = false, storedKey = newAesKey()) + + assertEquals("plain:blob", cipher.decrypt("blob")) + assertEquals(0, cipher.generatedKeys) + } + + @Test + fun `an AES-GCM tag mismatch is remapped to a clear GeneralSecurityException`() { + val badTag = AEADBadTagException("tag mismatch") + val cipher = FakeCipher( + generateKeyOnDecrypt = true, + storedKey = newAesKey(), + onDecrypt = { _, _ -> throw badTag }, + ) + + val error = assertFailsWith { cipher.decrypt("blob") } + assertSame(badTag, error.cause) + assertTrue(error.message!!.contains("test.alias")) + } + + @Test + fun `a non-AEAD failure propagates unwrapped so key invalidation still surfaces`() { + // Only AEADBadTagException is remapped; every other cipher failure — including the + // KeyPermanentlyInvalidatedException a real init throws on an invalidated key — must propagate + // unchanged so callers can classify it. + val boom = IllegalArgumentException("boom") + val cipher = FakeCipher( + generateKeyOnDecrypt = false, + storedKey = newAesKey(), + onDecrypt = { _, _ -> throw boom }, + ) + + assertSame(boom, assertFailsWith { cipher.decrypt("blob") }) + } + + /** + * A JVM-only [AesGcmKeystoreCipher] whose Keystore seams are replaced by in-memory fakes so the + * base's key-resolution policy and error mapping run without a device. [storedKey] models the key + * present under the alias (null = absent); [onDecrypt] models the GCM cipher operation. + */ + private class FakeCipher( + generateKeyOnDecrypt: Boolean, + private val storedKey: SecretKey?, + private val onDecrypt: (SecretKey, String) -> String = { _, encoded -> "plain:$encoded" }, + ) : AesGcmKeystoreCipher(alias = "test.alias", generateKeyOnDecrypt = generateKeyOnDecrypt) { + + var generatedKeys = 0 + private set + var decryptCalls = 0 + private set + + override fun existingKey(): SecretKey? = storedKey + + override fun getOrCreateKey(): SecretKey = existingKey() ?: newAesKey().also { generatedKeys++ } + + override fun decryptWithKey(key: SecretKey, encoded: String): String { + decryptCalls++ + return onDecrypt(key, encoded) + } + + override fun keySpec(): KeyGenParameterSpec = error("keySpec is not exercised in the JVM base test") + } + + private companion object { + const val KEY_BYTES = 32 + + fun newAesKey(): SecretKey = SecretKeySpec(ByteArray(KEY_BYTES) { it.toByte() }, "AES") + } +} diff --git a/app/src/test/kotlin/org/libremail/data/security/AuthenticatorPolicyTest.kt b/app/src/test/kotlin/org/libremail/data/security/AuthenticatorPolicyTest.kt new file mode 100644 index 0000000..5babb5b --- /dev/null +++ b/app/src/test/kotlin/org/libremail/data/security/AuthenticatorPolicyTest.kt @@ -0,0 +1,60 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.security + +import android.security.keystore.KeyProperties +import androidx.biometric.BiometricManager +import org.junit.Test +import kotlin.test.assertEquals +import kotlin.test.assertNotEquals + +/** + * Pins the single-source-of-truth authenticator mapping. [AuthenticatorPolicy.ACCEPTED] is the one + * definition; the two derived flag sets translate it into each Android API's own vocabulary. If a + * future change loosens or tightens one, both must move together — these assertions catch the drift + * that is otherwise only observable on a device (a BiometricPrompt that succeeds but a Keystore key + * that then throws UserNotAuthenticatedException at use). + * + * The referenced SDK constants are Java compile-time constants, so they inline into this test on the + * plain JVM — no Android runtime is needed to compare the values. + */ +class AuthenticatorPolicyTest { + + @Test + fun `accepted set is a strong biometric or the device credential`() { + assertEquals( + setOf(AppAuthenticator.STRONG_BIOMETRIC, AppAuthenticator.DEVICE_CREDENTIAL), + AuthenticatorPolicy.ACCEPTED, + ) + } + + @Test + fun `maps to the BiometricManager vocabulary for the BiometricPrompt`() { + assertEquals( + BiometricManager.Authenticators.BIOMETRIC_STRONG or BiometricManager.Authenticators.DEVICE_CREDENTIAL, + AuthenticatorPolicy.biometricPromptAuthenticators, + ) + } + + @Test + fun `maps to the KeyProperties vocabulary for the KeyGenParameterSpec`() { + assertEquals( + KeyProperties.AUTH_BIOMETRIC_STRONG or KeyProperties.AUTH_DEVICE_CREDENTIAL, + AuthenticatorPolicy.keyGenAuthenticators, + ) + } + + @Test + fun `AppLockManager AUTHENTICATORS is the biometric-prompt mapping, not an independent copy`() { + assertEquals(AuthenticatorPolicy.biometricPromptAuthenticators, AppLockManager.AUTHENTICATORS) + } + + @Test + fun `the two API vocabularies are genuinely different bit sets`() { + // Why a single shared Int would be a bug: the same concept has different flag values in each + // API, so the policy has to be mapped, not copied. + assertNotEquals( + AuthenticatorPolicy.biometricPromptAuthenticators, + AuthenticatorPolicy.keyGenAuthenticators, + ) + } +} diff --git a/app/src/test/kotlin/org/libremail/data/security/DatabaseKeyStoreTest.kt b/app/src/test/kotlin/org/libremail/data/security/DatabaseKeyStoreTest.kt new file mode 100644 index 0000000..6370ff6 --- /dev/null +++ b/app/src/test/kotlin/org/libremail/data/security/DatabaseKeyStoreTest.kt @@ -0,0 +1,256 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.security + +import androidx.datastore.core.DataStore +import androidx.datastore.preferences.core.Preferences +import androidx.datastore.preferences.core.booleanPreferencesKey +import androidx.datastore.preferences.core.emptyPreferences +import androidx.datastore.preferences.core.stringPreferencesKey +import io.mockk.every +import io.mockk.mockk +import io.mockk.verify +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.test.runTest +import org.junit.Before +import org.junit.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertFalse +import kotlin.test.assertNotNull +import kotlin.test.assertNull +import kotlin.test.assertTrue + +/** + * Pins the [DatabaseKeyStore] dual-seal exchange (issue #100). The device-only Keystore that produces + * the sealed blobs is mocked with a reversible cipher, and the persistence runs against an in-memory + * [DataStore] injected through the [DatabaseKeyStore.dataStore] seam, so the security-critical + * invariants — "never both seals at once" and "an auth-sealed passphrase is not recoverable without + * authentication" — are exercised deterministically on the JVM instead of only on a device. + * + * [crypto] models the non-auth master seal as `m:`; [authCipher] models the auth-bound seal as + * `a:`. Both are reversible so a resealed passphrase round-trips, which is exactly what keeps an + * already-encrypted cache readable across an app-lock toggle. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class DatabaseKeyStoreTest { + + private val store = InMemoryPreferencesDataStore() + private val crypto = mockk(relaxed = true) + private val authCipher = mockk(relaxed = true) + private val session = PassphraseSession() + + @Before + fun setUp() { + every { crypto.encrypt(any()) } answers { "m:" + firstArg() } + every { crypto.decrypt(any()) } answers { firstArg().removePrefix("m:") } + every { authCipher.encrypt(any()) } answers { "a:" + firstArg() } + every { authCipher.decrypt(any()) } answers { firstArg().removePrefix("a:") } + } + + private fun keyStore(): DatabaseKeyStore = + DatabaseKeyStore(mockk(relaxed = true), crypto, authCipher, session).also { it.dataStore = store } + + @Test + fun `passphrase mints a master-sealed key on first use and never alongside an auth seal`() = runTest { + val keyStore = keyStore() + assertEquals(SealState.NONE, keyStore.sealState()) + + val passphrase = keyStore.passphrase() + + assertEquals(SealState.MASTER, keyStore.sealState()) + assertEquals(HEX_LEN, passphrase.length, "the SQLCipher passphrase is 32 bytes rendered as hex") + // Exactly one seal exists: the master copy is present and no auth copy was written. + assertNotNull(store.data.first()[SEALED_MASTER]) + assertNull(store.data.first()[SEALED_AUTH]) + // Idempotent: a second call returns the SAME passphrase rather than regenerating one (a second + // key would strand the DB under a passphrase we could no longer reproduce). + assertEquals(passphrase, keyStore.passphrase()) + } + + @Test + fun `sealWithAuth replaces the master seal with an auth seal and unlocks the session`() = runTest { + val keyStore = keyStore() + val passphrase = keyStore.passphrase() // start master-sealed (app-lock off) + + keyStore.sealWithAuth() + + // Never both seals at once: enabling app-lock drops the master copy so the cache key is no + // longer recoverable without authentication. + assertEquals(SealState.AUTH, keyStore.sealState()) + assertNull(store.data.first()[SEALED_MASTER]) + assertEquals("a:$passphrase", store.data.first()[SEALED_AUTH]) + // The SAME passphrase is resealed (an already-encrypted cache stays readable) and unlocked into + // the session so the DB opens this session. + assertTrue(keyStore.hasAuthSealedPassphrase()) + assertEquals(passphrase, session.current()) + } + + @Test + fun `sealWithAuth mints a fresh passphrase when neither a seal nor a session value exists`() = runTest { + val keyStore = keyStore() + assertEquals(SealState.NONE, keyStore.sealState()) + + keyStore.sealWithAuth() + + assertEquals(SealState.AUTH, keyStore.sealState()) + assertNull(store.data.first()[SEALED_MASTER]) + val minted = session.current() + assertNotNull(minted) + assertEquals(HEX_LEN, minted.length) + assertEquals("a:$minted", store.data.first()[SEALED_AUTH]) + } + + @Test + fun `unlockWithAuth unwraps the auth-sealed passphrase into the session`() = runTest { + val keyStore = keyStore() + keyStore.sealWithAuth() + val passphrase = requireNotNull(session.current()) + session.lock() // simulate a fresh session that must unwrap after the user authenticates + + keyStore.unlockWithAuth() + + assertEquals(passphrase, session.current()) + } + + @Test + fun `unlockWithAuth is a no-op when nothing is auth-sealed`() = runTest { + val keyStore = keyStore() + + keyStore.unlockWithAuth() + + assertNull(session.current()) + verify(exactly = 0) { authCipher.decrypt(any()) } + } + + @Test + fun `sealWithMaster replaces the auth seal with a master seal, deletes the auth key, and locks`() = runTest { + val keyStore = keyStore() + keyStore.sealWithAuth() + val passphrase = requireNotNull(session.current()) + + keyStore.sealWithMaster() + + // Never both seals at once: disabling app-lock drops the auth copy and reseals under the master. + assertEquals(SealState.MASTER, keyStore.sealState()) + assertNull(store.data.first()[SEALED_AUTH]) + assertEquals("m:$passphrase", store.data.first()[SEALED_MASTER]) + // The now-orphaned auth-bound key is deleted so a later invalidation can't trigger a spurious + // wipe, and the session is dropped so the cache opens without auth again. + verify { authCipher.deleteKey() } + assertNull(session.current()) + // Master-sealed value is recoverable WITHOUT authentication (that is the whole point of disable). + assertEquals(passphrase, keyStore.passphrase()) + } + + @Test + fun `sealWithMaster decrypts the auth seal when the session is already locked`() = runTest { + val keyStore = keyStore() + keyStore.sealWithAuth() + val passphrase = requireNotNull(session.current()) + session.lock() // no session value: sealWithMaster must fall back to decrypting the auth seal + + keyStore.sealWithMaster() + + assertEquals(SealState.MASTER, keyStore.sealState()) + assertEquals("m:$passphrase", store.data.first()[SEALED_MASTER]) + assertNull(store.data.first()[SEALED_AUTH]) + } + + @Test + fun `sealWithMaster does nothing when neither a session value nor an auth seal exists`() = runTest { + val keyStore = keyStore() + + keyStore.sealWithMaster() + + assertEquals(SealState.NONE, keyStore.sealState()) + verify(exactly = 0) { authCipher.deleteKey() } + } + + @Test + fun `resetSealedPassphrase drops every seal, deletes the auth key, and locks the session`() = runTest { + val keyStore = keyStore() + keyStore.sealWithAuth() // auth-sealed + session unlocked + + keyStore.resetSealedPassphrase() + + assertEquals(SealState.NONE, keyStore.sealState()) + assertNull(store.data.first()[SEALED_AUTH]) + assertNull(store.data.first()[SEALED_MASTER]) + verify { authCipher.deleteKey() } + assertNull(session.current()) + } + + @Test + fun `passphrase refuses to mint a master key while an auth seal exists`() = runTest { + val keyStore = keyStore() + keyStore.sealWithAuth() // an auth seal now exists + session.lock() + + // Minting a master passphrase now would strand the real (auth-sealed) key and leave the DB + // encrypted under a passphrase we could never reproduce — so it fails loudly instead of quietly + // creating a second, recoverable-without-auth copy. + assertFailsWith { keyStore.passphrase() } + assertEquals(SealState.AUTH, keyStore.sealState()) + assertNull(store.data.first()[SEALED_MASTER]) + } + + @Test + fun `resolvePassphrase returns the master-sealed value without authentication when app-lock is off`() = runTest { + val keyStore = keyStore() + val passphrase = keyStore.passphrase() // master-sealed + + assertEquals(passphrase, keyStore.resolvePassphrase(appLockEnabled = false)) + } + + @Test + fun `resolvePassphrase returns the authenticated session value when an auth seal exists`() = runTest { + val keyStore = keyStore() + keyStore.sealWithAuth() + val passphrase = requireNotNull(session.current()) + + // AUTH seal: the value lives only in the session after the user authenticates — read it there, + // never re-derive or regenerate it. + assertEquals(passphrase, keyStore.resolvePassphrase(appLockEnabled = true)) + } + + @Test + fun `clear-pending flag round-trips through set, query, and clear`() = runTest { + val keyStore = keyStore() + assertFalse(keyStore.isClearPending()) + + keyStore.setClearPending() + assertTrue(keyStore.isClearPending()) + assertEquals(true, store.data.first()[CLEAR_PENDING]) + + keyStore.clearClearPending() + assertFalse(keyStore.isClearPending()) + assertNull(store.data.first()[CLEAR_PENDING]) + } + + private companion object { + // Same key names DatabaseKeyStore persists under, so the raw store can be inspected directly. + val SEALED_MASTER = stringPreferencesKey("sealed_db_key") + val SEALED_AUTH = stringPreferencesKey("sealed_db_key_auth") + val CLEAR_PENDING = booleanPreferencesKey("clear_encrypted_cache_pending") + const val HEX_LEN = 64 // 32 random bytes rendered as hex + } +} + +/** + * A minimal in-memory [DataStore] of [Preferences] backed by a [MutableStateFlow], substituted for the + * device-backed file store so the seal exchange is JVM-testable. `edit { }` routes through [updateData]. + */ +private class InMemoryPreferencesDataStore : DataStore { + private val flow = MutableStateFlow(emptyPreferences()) + override val data: Flow = flow.asStateFlow() + + override suspend fun updateData(transform: suspend (Preferences) -> Preferences): Preferences { + val updated = transform(flow.value) + flow.value = updated + return updated + } +} diff --git a/app/src/test/kotlin/org/libremail/data/security/KeyInvalidationPolicyTest.kt b/app/src/test/kotlin/org/libremail/data/security/KeyInvalidationPolicyTest.kt index 66fab93..47a9994 100644 --- a/app/src/test/kotlin/org/libremail/data/security/KeyInvalidationPolicyTest.kt +++ b/app/src/test/kotlin/org/libremail/data/security/KeyInvalidationPolicyTest.kt @@ -53,28 +53,59 @@ class KeyInvalidationPolicyTest { } @Test - fun `full decision table is pinned`() { - // App-lock off: always proceed, regardless of the other three inputs (all 8 combinations). - for (e in listOf(false, true)) { - for (s in listOf(false, true)) { - for (i in listOf(false, true)) { - assertEquals( - LockAction.PROCEED, - decide(appLock = false, encrypt = e, secure = s, invalidated = i), - ) - } - } + fun `every one of the 16 input combinations maps to its pinned action`() { + // The complete truth table for decide(appLock, encrypt, secure, invalidated): all 2^4 = 16 rows + // listed explicitly, so a mutation of ANY branch is caught — most importantly the common + // (on, *, secure, valid) rows, whose silent flip to PROCEED would be a lock bypass. The + // completeness guard below fails if a row is ever dropped, keeping the table exhaustive. + // + // Columns: appLock, encrypt, secure, invalidated -> expected action. + val table = listOf( + // App-lock OFF: always PROCEED, whatever the other three inputs are. + Case(false, false, false, false, LockAction.PROCEED), + Case(false, false, false, true, LockAction.PROCEED), + Case(false, false, true, false, LockAction.PROCEED), + Case(false, false, true, true, LockAction.PROCEED), + Case(false, true, false, false, LockAction.PROCEED), + Case(false, true, false, true, LockAction.PROCEED), + Case(false, true, true, false, LockAction.PROCEED), + Case(false, true, true, true, LockAction.PROCEED), + // App-lock ON, device NOT secure (lock removed): clear+disable iff a cache exists, else disable. + Case(true, true, false, false, LockAction.CLEAR_AND_DISABLE), + Case(true, true, false, true, LockAction.CLEAR_AND_DISABLE), + Case(true, false, false, false, LockAction.DISABLE_APP_LOCK), + Case(true, false, false, true, LockAction.DISABLE_APP_LOCK), + // App-lock ON, secure, key invalidated: clear+re-auth iff a cache exists, else just re-auth. + Case(true, true, true, true, LockAction.CLEAR_AND_REQUIRE_AUTH), + Case(true, false, true, true, LockAction.REQUIRE_AUTH), + // App-lock ON, secure, key valid: the common case — require auth, no wipe. + Case(true, true, true, false, LockAction.REQUIRE_AUTH), + Case(true, false, true, false, LockAction.REQUIRE_AUTH), + ) + + // Exhaustiveness: exactly the 16 distinct (appLock, encrypt, secure, invalidated) combinations. + assertEquals(16, table.size, "the table must list all 2^4 input combinations") + assertEquals( + 16, + table.map { listOf(it.appLock, it.encrypt, it.secure, it.invalidated) }.toSet().size, + "every row must be a distinct input combination", + ) + + for (case in table) { + assertEquals( + case.expected, + decide(case.appLock, case.encrypt, case.secure, case.invalidated), + "decide(appLock=${case.appLock}, encrypt=${case.encrypt}, " + + "secure=${case.secure}, invalidated=${case.invalidated})", + ) } - // App-lock on, device no longer secure: clear+disable iff there is an encrypted cache to lose. - assertEquals(LockAction.CLEAR_AND_DISABLE, decide(secure = false, encrypt = true, invalidated = false)) - assertEquals(LockAction.CLEAR_AND_DISABLE, decide(secure = false, encrypt = true, invalidated = true)) - assertEquals(LockAction.DISABLE_APP_LOCK, decide(secure = false, encrypt = false, invalidated = false)) - assertEquals(LockAction.DISABLE_APP_LOCK, decide(secure = false, encrypt = false, invalidated = true)) - // App-lock on, secure, key invalidated: clear+re-auth iff encrypted, else just re-auth. - assertEquals(LockAction.CLEAR_AND_REQUIRE_AUTH, decide(secure = true, encrypt = true, invalidated = true)) - assertEquals(LockAction.REQUIRE_AUTH, decide(secure = true, encrypt = false, invalidated = true)) - // App-lock on, secure, key valid: the common case — require auth (previously unpinned rows). - assertEquals(LockAction.REQUIRE_AUTH, decide(secure = true, encrypt = true, invalidated = false)) - assertEquals(LockAction.REQUIRE_AUTH, decide(secure = true, encrypt = false, invalidated = false)) } + + private data class Case( + val appLock: Boolean, + val encrypt: Boolean, + val secure: Boolean, + val invalidated: Boolean, + val expected: LockAction, + ) } diff --git a/app/src/test/kotlin/org/libremail/data/sync/SyncSchedulerTest.kt b/app/src/test/kotlin/org/libremail/data/sync/SyncSchedulerTest.kt index 24092ed..81d267c 100644 --- a/app/src/test/kotlin/org/libremail/data/sync/SyncSchedulerTest.kt +++ b/app/src/test/kotlin/org/libremail/data/sync/SyncSchedulerTest.kt @@ -4,12 +4,15 @@ package org.libremail.data.sync import androidx.work.ExistingPeriodicWorkPolicy import androidx.work.ExistingWorkPolicy import androidx.work.OneTimeWorkRequest +import androidx.work.Operation import androidx.work.PeriodicWorkRequest import androidx.work.WorkManager +import io.mockk.every import io.mockk.mockk import io.mockk.verify import org.junit.Test import javax.inject.Provider +import kotlin.test.assertSame /** * The enqueue methods are thin wrappers over WorkManager, so these tests pin the one thing that carries @@ -79,6 +82,18 @@ class SyncSchedulerTest { } } + // The app-lock key-invalidation recovery restart awaits this Operation before killing the process + // (see AppLockViewModel), so the WorkSpec is durably persisted and the post-wipe re-sync survives. + @Test + fun `syncNow returns the enqueue operation so callers can await durable persistence`() { + val operation = mockk() + every { + workManager.enqueueUniqueWork(any(), any(), any()) + } returns operation + + assertSame(operation, scheduler.syncNow()) + } + @Test fun `backfillNow keeps an already-running backfill`() { scheduler.backfillNow() diff --git a/app/src/test/kotlin/org/libremail/mail/ImapClientTest.kt b/app/src/test/kotlin/org/libremail/mail/ImapClientTest.kt index ca06638..a4c11ed 100644 --- a/app/src/test/kotlin/org/libremail/mail/ImapClientTest.kt +++ b/app/src/test/kotlin/org/libremail/mail/ImapClientTest.kt @@ -4,11 +4,16 @@ package org.libremail.mail import com.icegreen.greenmail.util.GreenMail import com.icegreen.greenmail.util.GreenMailUtil import com.icegreen.greenmail.util.ServerSetupTest +import jakarta.activation.DataHandler import jakarta.mail.Folder import jakarta.mail.Message +import jakarta.mail.Part import jakarta.mail.Session import jakarta.mail.internet.InternetAddress +import jakarta.mail.internet.MimeBodyPart import jakarta.mail.internet.MimeMessage +import jakarta.mail.internet.MimeMultipart +import jakarta.mail.util.ByteArrayDataSource import kotlinx.coroutines.test.runTest import org.junit.After import org.junit.Before @@ -146,6 +151,27 @@ class ImapClientTest { assertFalse(client.fetchRecent(params(), "INBOX", limit = 50).first().isRead, "should stay unread") } + @Test + fun `fetchBodyPeek splits inline cid images from real attachments`() = runTest { + // A digest-style message: multipart/related(html + inline image) alongside a real attachment. + appendInlineImageDigest() + val uid = client.fetchRecent(params(), "INBOX", limit = 50).first().uid + + val content = client.fetchBodyPeek(params(), "INBOX", uid) + + assertTrue(content.isHtml, "the html body must be chosen") + assertTrue(content.body.contains("cid:logo1"), "body=${content.body}") + // Both parts are collected (so the inline image's bytes are fetchable by index), but only the + // inline one carries a Content-ID — the reader filters on that to keep it out of the list. + assertEquals(2, content.attachments.size, "attachments=${content.attachments}") + val inline = content.attachments.single { it.contentId != null } + assertEquals("logo1", inline.contentId) + assertEquals("logo.png", inline.filename) + assertTrue(inline.mimeType.equals("image/png", ignoreCase = true), "mime=${inline.mimeType}") + val attachment = content.attachments.single { it.contentId == null } + assertEquals("invoice.pdf", attachment.filename) + } + @Test fun `fetchRecent reads a non-inbox folder isolated from the inbox`() = runTest { GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Inbox subject", "In the inbox") @@ -161,6 +187,68 @@ class ImapClientTest { assertEquals(setOf("Inbox subject"), inbox.map { it.subject }.toSet()) } + /** + * Appends a rich digest to the INBOX: a `multipart/mixed` of a `multipart/related` (HTML body + * referencing an inline image via `cid:logo1`) plus a genuine PDF attachment — the shape that + * regressed inline images into the attachment list (issue #133). + */ + private fun appendInlineImageDigest() { + val props = Properties().apply { + put("mail.store.protocol", "imap") + put("mail.imap.host", "127.0.0.1") + put("mail.imap.port", greenMail.imap.port.toString()) + } + val session = Session.getInstance(props) + + val htmlPart = MimeBodyPart().apply { + setContent("

Hello

", "text/html; charset=utf-8") + } + val inlineImage = MimeBodyPart().apply { + dataHandler = DataHandler(ByteArrayDataSource(byteArrayOf(1, 2, 3, 4), "image/png")) + contentID = "" + disposition = Part.INLINE + fileName = "logo.png" + } + val related = MimeBodyPart().apply { + setContent( + MimeMultipart("related").apply { + addBodyPart(htmlPart) + addBodyPart(inlineImage) + }, + ) + } + val attachment = MimeBodyPart().apply { + dataHandler = DataHandler(ByteArrayDataSource(byteArrayOf(5, 6, 7), "application/pdf")) + disposition = Part.ATTACHMENT + fileName = "invoice.pdf" + } + val message = MimeMessage(session).apply { + setFrom(InternetAddress("bob@example.org")) + setRecipient(Message.RecipientType.TO, InternetAddress("alice@example.org")) + subject = "Daily Digest" + setContent( + MimeMultipart("mixed").apply { + addBodyPart(related) + addBodyPart(attachment) + }, + ) + // Flush each part's Content-Type/Content-ID/Content-Disposition into headers so the + // appended raw MIME round-trips them (without this the Content-ID is dropped). + saveChanges() + } + + val store = session.getStore("imap") + store.connect("127.0.0.1", greenMail.imap.port, "alice@example.org", "secret") + try { + val inbox = store.getFolder("INBOX") + inbox.open(Folder.READ_WRITE) + inbox.appendMessages(arrayOf(message)) + inbox.close(false) + } finally { + store.close() + } + } + /** Creates [folderName] if needed and appends a message to it, via Jakarta Mail directly. */ private fun appendMessage(folderName: String, from: String, subject: String, body: String) { val props = Properties().apply { diff --git a/app/src/test/kotlin/org/libremail/mail/MimePartClassificationTest.kt b/app/src/test/kotlin/org/libremail/mail/MimePartClassificationTest.kt new file mode 100644 index 0000000..651b5e9 --- /dev/null +++ b/app/src/test/kotlin/org/libremail/mail/MimePartClassificationTest.kt @@ -0,0 +1,85 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.mail + +import jakarta.mail.Part +import jakarta.mail.internet.MimeBodyPart +import org.junit.Test +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertNull +import kotlin.test.assertTrue + +/** + * Unit tests for the MIME-part classification behind issue #133. An inline image (a `Content-ID` that + * the HTML body references via `cid:`) carries a filename AND a `Content-ID` under + * `Content-Disposition: inline`; it must NOT be swept into the downloadable-attachment list, while + * genuine attachments and filename-only parts still must. + */ +class MimePartClassificationTest { + + private fun part( + contentType: String, + disposition: String? = null, + filename: String? = null, + contentId: String? = null, + ): MimeBodyPart = MimeBodyPart().apply { + setHeader("Content-Type", contentType) + if (disposition != null || filename != null) { + val header = buildString { + append(disposition ?: Part.INLINE) + if (filename != null) append("; filename=\"").append(filename).append("\"") + } + setHeader("Content-Disposition", header) + } + if (contentId != null) setHeader("Content-ID", contentId) + } + + @Test + fun `an inline image with a Content-ID is not a downloadable attachment`() { + val inline = part("image/jpeg", disposition = "inline", filename = "mailer-1.jpg", contentId = "") + + assertFalse(isAttachmentPart(inline), "inline+Content-ID must be excluded from attachments") + assertTrue(isInlineImagePart(inline), "inline+Content-ID image must be collected for cid: rendering") + } + + @Test + fun `a real attachment with a filename is kept as an attachment`() { + val attachment = part("application/pdf", disposition = "attachment", filename = "invoice.pdf") + + assertTrue(isAttachmentPart(attachment)) + assertFalse(isInlineImagePart(attachment)) + } + + @Test + fun `a part with attachment disposition and no filename is kept as an attachment`() { + val attachment = part("application/octet-stream", disposition = "attachment") + + assertTrue(isAttachmentPart(attachment)) + assertFalse(isInlineImagePart(attachment)) + } + + @Test + fun `an image with a filename but no Content-ID is kept as an attachment`() { + // No cid means nothing references it from the body, so it is a genuine download, not inline. + val image = part("image/png", disposition = "inline", filename = "photo.png") + + assertTrue(isAttachmentPart(image), "filename without a Content-ID stays a downloadable attachment") + assertFalse(isInlineImagePart(image)) + } + + @Test + fun `an inline image without an explicit disposition is still classified inline by its Content-ID`() { + // Some mailers omit Content-Disposition entirely and rely on the Content-ID + cid: reference. + val inline = part("image/gif", contentId = "logo@example.com") + + assertFalse(isAttachmentPart(inline)) + assertTrue(isInlineImagePart(inline)) + } + + @Test + fun `Content-ID is normalized by stripping surrounding angle brackets`() { + assertEquals("cid-1@usps", inlineContentId(part("image/jpeg", contentId = ""))) + assertEquals("bare@id", inlineContentId(part("image/jpeg", contentId = "bare@id"))) + assertNull(inlineContentId(part("application/pdf", disposition = "attachment", filename = "x.pdf"))) + } +} diff --git a/app/src/test/kotlin/org/libremail/ui/lock/AppLockViewModelTest.kt b/app/src/test/kotlin/org/libremail/ui/lock/AppLockViewModelTest.kt index 7efcaa0..49f16fd 100644 --- a/app/src/test/kotlin/org/libremail/ui/lock/AppLockViewModelTest.kt +++ b/app/src/test/kotlin/org/libremail/ui/lock/AppLockViewModelTest.kt @@ -1,9 +1,19 @@ // SPDX-License-Identifier: GPL-3.0-or-later package org.libremail.ui.lock +import android.content.Context import android.os.SystemClock +import android.security.keystore.KeyPermanentlyInvalidatedException +import android.security.keystore.UserNotAuthenticatedException +import android.util.Log +import androidx.work.Operation +import com.google.common.util.concurrent.ListenableFuture +import io.mockk.coEvery +import io.mockk.coVerify +import io.mockk.coVerifyOrder import io.mockk.every import io.mockk.mockk +import io.mockk.mockkObject import io.mockk.mockkStatic import io.mockk.unmockkAll import io.mockk.verify @@ -11,6 +21,7 @@ import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.test.UnconfinedTestDispatcher +import kotlinx.coroutines.test.advanceUntilIdle import kotlinx.coroutines.test.resetMain import kotlinx.coroutines.test.runTest import kotlinx.coroutines.test.setMain @@ -18,8 +29,18 @@ import org.junit.After import org.junit.Before import org.junit.Test import org.libremail.data.security.AppLockGate +import org.libremail.data.security.AppLockManager +import org.libremail.data.security.DatabaseKeyCipher +import org.libremail.data.security.DatabaseKeyStore +import org.libremail.data.security.KeyInvalidationPolicy +import org.libremail.data.security.LockAction +import org.libremail.data.security.LockState +import org.libremail.data.security.PassphraseSession import org.libremail.data.settings.AppSettings import org.libremail.data.settings.SettingsRepository +import org.libremail.data.sync.SyncScheduler +import org.libremail.restart.ProcessRestarter +import java.util.concurrent.TimeoutException import kotlin.test.assertEquals import kotlin.test.assertIs @@ -28,7 +49,18 @@ import kotlin.test.assertIs * grace window survives Activity recreation — NOT a field the Activity-scoped ViewModel constructs * itself (which Back on the task root would drop on API 29/30). These tests exercise the synchronous * paths that delegate to the injected gate; the grace math itself is covered exhaustively — and - * deterministically — by AppLockGateTest. Broader ViewModel coverage is issue #100. + * deterministically — by AppLockGateTest. + * + * The recovery-restart tests pin the ordering that makes the key-invalidation "clear + re-sync" safe: + * the re-sync enqueue must be durably persisted (its WorkManager Operation awaited) BEFORE the process + * is restarted, and a stuck enqueue must never wedge recovery. The separate-process relaunch itself is + * device-only; here we assert the ViewModel's orchestration around ProcessRestarter. + * + * Issue #100 broadens this to the ViewModel's security-critical branching: the `onForeground` + * [LockAction] dispatch (that DISABLE_APP_LOCK persists the setting, CLEAR_* set the pending flag and + * restart, and CLEAR_AND_REQUIRE_AUTH keeps app-lock on) and the `onAuthenticated` unlock/arm + * classification (OK / UNRECOVERABLE / RETRY), so a mutation that wipes user data or drops the lock is + * caught here rather than only on a device. */ @OptIn(ExperimentalCoroutinesApi::class) class AppLockViewModelTest { @@ -44,19 +76,33 @@ class AppLockViewModelTest { unmockkAll() } - private fun viewModel(gate: AppLockGate, appLock: Boolean = true): AppLockViewModel { - val settings = mockk() - every { settings.settings } returns flowOf(AppSettings(appLock = appLock)) + private fun viewModel( + gate: AppLockGate, + appLock: Boolean = true, + settingsRepository: SettingsRepository = mockk(relaxed = true), + databaseKeyStore: DatabaseKeyStore = mockk(relaxed = true), + syncScheduler: SyncScheduler = mockk(relaxed = true), + processRestarter: ProcessRestarter = mockk(relaxed = true), + encryptCache: Boolean = false, + databaseKeyCipher: DatabaseKeyCipher = mockk(relaxed = true), + session: PassphraseSession = mockk(relaxed = true), + appLockManager: AppLockManager = mockk(relaxed = true), + context: Context = mockk(relaxed = true), + ): AppLockViewModel { + val appSettings = AppSettings(appLock = appLock, encryptCache = encryptCache) + every { settingsRepository.settings } returns flowOf(appSettings) return AppLockViewModel( - context = mockk(relaxed = true), - settingsRepository = settings, - appLockManager = mockk(relaxed = true), - databaseKeyStore = mockk(relaxed = true), - databaseKeyCipher = mockk(relaxed = true), - session = mockk(relaxed = true), - syncScheduler = mockk(relaxed = true), + context = context, + settingsRepository = settingsRepository, + appLockManager = appLockManager, + databaseKeyStore = databaseKeyStore, + databaseKeyCipher = databaseKeyCipher, + session = session, + syncScheduler = syncScheduler, + processRestarter = processRestarter, gate = gate, - ) + // Run the off-main recovery work on the test scheduler so its ordering is deterministic. + ).also { it.defaultDispatcher = dispatcher } } @Test @@ -84,4 +130,469 @@ class AppLockViewModelTest { val state = assertIs(vm.uiState.value) assertEquals("boom", state.error) } + + @Test + fun `recovery persists the re-sync enqueue before restarting`() = runTest(dispatcher) { + val (future, syncScheduler) = enqueueingScheduler() + val databaseKeyStore = mockk(relaxed = true) + val processRestarter = mockk(relaxed = true) + val vm = clearOnForegroundViewModel( + databaseKeyStore = databaseKeyStore, + syncScheduler = syncScheduler, + processRestarter = processRestarter, + ) + + vm.onForeground() + advanceUntilIdle() + + // The re-sync WorkSpec must be durably persisted (the enqueue Operation awaited) BEFORE the + // process is torn down. Otherwise WorkManager's async insert races the process death, the + // enqueue is lost, and the just-wiped cache never refills until the next periodic sync. + coVerifyOrder { + databaseKeyStore.setClearPending() + syncScheduler.syncNow() + future.get(any(), any()) + processRestarter.restart() + } + } + + @Test + fun `recovery still restarts when the re-sync enqueue await times out`() = runTest(dispatcher) { + val (future, syncScheduler) = enqueueingScheduler() + every { future.get(any(), any()) } throws TimeoutException("stuck insert") + val processRestarter = mockk(relaxed = true) + val vm = clearOnForegroundViewModel(syncScheduler = syncScheduler, processRestarter = processRestarter) + + vm.onForeground() + advanceUntilIdle() + + // A stuck WorkManager insert must not wedge recovery: the timeout is swallowed and we restart + // anyway (the periodic sync will still refill the wiped cache later). + verify { future.get(any(), any()) } + verify { processRestarter.restart() } + } + + // --- onForeground: LockAction dispatch (issue #100) ------------------------------------------ + + @Test + fun `onForeground with app-lock off shows the app`() = runTest(dispatcher) { + mockkStatic(SystemClock::class) + every { SystemClock.elapsedRealtime() } returns FOREGROUND_AT + val vm = viewModel(gate = mockk(relaxed = true), appLock = false) + + vm.onForeground() + advanceUntilIdle() + + assertIs(vm.uiState.value) + } + + @Test + fun `onForeground DISABLE_APP_LOCK persists app-lock off and shows the app`() = runTest(dispatcher) { + val settingsRepository = mockk(relaxed = true) + val processRestarter = mockk(relaxed = true) + val vm = foregroundResolving( + LockAction.DISABLE_APP_LOCK, + settingsRepository = settingsRepository, + processRestarter = processRestarter, + ) + + vm.onForeground() + advanceUntilIdle() + + // The lock was silently dropped (device no longer secure, nothing encrypted to lose): the + // setting is persisted off and the app is shown, with no cache wipe / restart. + coVerify { settingsRepository.setAppLock(false) } + assertIs(vm.uiState.value) + verify(exactly = 0) { processRestarter.restart() } + } + + @Test + fun `onForeground CLEAR_AND_DISABLE clears, disables app-lock, then restarts in order`() = runTest(dispatcher) { + val (future, syncScheduler) = enqueueingScheduler() + val settingsRepository = mockk(relaxed = true) + val databaseKeyStore = mockk(relaxed = true) + val processRestarter = mockk(relaxed = true) + val vm = foregroundResolving( + LockAction.CLEAR_AND_DISABLE, + settingsRepository = settingsRepository, + databaseKeyStore = databaseKeyStore, + syncScheduler = syncScheduler, + processRestarter = processRestarter, + ) + + vm.onForeground() + advanceUntilIdle() + + // Crash-safe recovery order: record the wipe intent and drop the gate BEFORE the durable + // re-sync enqueue is awaited and the process is torn down. + coVerifyOrder { + databaseKeyStore.setClearPending() + settingsRepository.setAppLock(false) + syncScheduler.syncNow() + future.get(any(), any()) + processRestarter.restart() + } + } + + @Test + fun `onForeground CLEAR_AND_REQUIRE_AUTH clears and restarts but keeps app-lock on`() = runTest(dispatcher) { + val (future, syncScheduler) = enqueueingScheduler() + val settingsRepository = mockk(relaxed = true) + val databaseKeyStore = mockk(relaxed = true) + val processRestarter = mockk(relaxed = true) + val vm = foregroundResolving( + LockAction.CLEAR_AND_REQUIRE_AUTH, + settingsRepository = settingsRepository, + databaseKeyStore = databaseKeyStore, + syncScheduler = syncScheduler, + processRestarter = processRestarter, + ) + + vm.onForeground() + advanceUntilIdle() + + // Same clear + durable re-sync + restart, but app-lock stays ON: the wipe re-arms a fresh key + // on the next authentication, so setAppLock(false) must NOT be called. + coVerifyOrder { + databaseKeyStore.setClearPending() + future.get(any(), any()) + processRestarter.restart() + } + coVerify(exactly = 0) { settingsRepository.setAppLock(false) } + } + + @Test + fun `onForeground PROCEED shows the app without restarting`() = runTest(dispatcher) { + val processRestarter = mockk(relaxed = true) + val vm = foregroundResolving(LockAction.PROCEED, processRestarter = processRestarter) + + vm.onForeground() + advanceUntilIdle() + + assertIs(vm.uiState.value) + verify(exactly = 0) { processRestarter.restart() } + } + + @Test + fun `onForeground REQUIRE_AUTH advances the gate and publishes its locked decision`() = runTest(dispatcher) { + val gate = mockk(relaxed = true) + every { gate.state } returns LockState.LOCKED + val vm = foregroundResolving(LockAction.REQUIRE_AUTH, gate = gate) + + vm.onForeground() + advanceUntilIdle() + + verify { gate.onForeground(FOREGROUND_AT, appLockEnabled = true) } + assertIs(vm.uiState.value) + } + + // --- onAuthenticated: unlockOrArm / unwrapSealedPassphrase classification (issue #100) -------- + + @Test + fun `onAuthenticated with an already-unlocked session unlocks the gate without unwrapping`() = runTest(dispatcher) { + val gate = mockk(relaxed = true) + every { gate.state } returns LockState.UNLOCKED + val session = mockk(relaxed = true) + every { session.isUnlocked() } returns true + val databaseKeyStore = mockk(relaxed = true) + val vm = viewModel(gate = gate, session = session, databaseKeyStore = databaseKeyStore) + + vm.onAuthenticated() + advanceUntilIdle() + + verify { gate.onAuthenticated() } + assertIs(vm.uiState.value) + coVerify(exactly = 0) { databaseKeyStore.unlockWithAuth() } + coVerify(exactly = 0) { databaseKeyStore.sealWithAuth() } + } + + @Test + fun `onAuthenticated unwraps a sealed passphrase and unlocks the gate`() = runTest(dispatcher) { + val gate = mockk(relaxed = true) + every { gate.state } returns LockState.UNLOCKED + val session = mockk(relaxed = true) + every { session.isUnlocked() } returns false + val databaseKeyStore = mockk(relaxed = true) + coEvery { databaseKeyStore.hasAuthSealedPassphrase() } returns true + val databaseKeyCipher = mockk(relaxed = true) + every { databaseKeyCipher.hasKey() } returns true + val vm = viewModel( + gate = gate, + session = session, + databaseKeyStore = databaseKeyStore, + databaseKeyCipher = databaseKeyCipher, + ) + + vm.onAuthenticated() + advanceUntilIdle() + + coVerify { databaseKeyStore.unlockWithAuth() } + verify { gate.onAuthenticated() } + assertIs(vm.uiState.value) + } + + @Test + fun `onAuthenticated clears the cache when the auth-bound key was deleted`() = runTest(dispatcher) { + stubLog() + val (future, syncScheduler) = enqueueingScheduler() + val session = mockk(relaxed = true) + every { session.isUnlocked() } returns false + val databaseKeyStore = mockk(relaxed = true) + coEvery { databaseKeyStore.hasAuthSealedPassphrase() } returns true + val databaseKeyCipher = mockk(relaxed = true) + every { databaseKeyCipher.hasKey() } returns false // key gone entirely -> unrecoverable + val processRestarter = mockk(relaxed = true) + val vm = viewModel( + gate = mockk(relaxed = true), + session = session, + databaseKeyStore = databaseKeyStore, + databaseKeyCipher = databaseKeyCipher, + syncScheduler = syncScheduler, + processRestarter = processRestarter, + ) + + vm.onAuthenticated() + advanceUntilIdle() + + // Unrecoverable: never attempt the unwrap; wipe the cache and restart (app-lock stays on). + coVerify(exactly = 0) { databaseKeyStore.unlockWithAuth() } + coVerifyOrder { + databaseKeyStore.setClearPending() + future.get(any(), any()) + processRestarter.restart() + } + } + + @Test + fun `onAuthenticated clears the cache when the auth-bound key was permanently invalidated`() = runTest(dispatcher) { + stubLog() + val (_, syncScheduler) = enqueueingScheduler() + val session = mockk(relaxed = true) + every { session.isUnlocked() } returns false + val databaseKeyStore = mockk(relaxed = true) + coEvery { databaseKeyStore.hasAuthSealedPassphrase() } returns true + coEvery { databaseKeyStore.unlockWithAuth() } throws mockk(relaxed = true) + val databaseKeyCipher = mockk(relaxed = true) + every { databaseKeyCipher.hasKey() } returns true + val processRestarter = mockk(relaxed = true) + val vm = viewModel( + gate = mockk(relaxed = true), + session = session, + databaseKeyStore = databaseKeyStore, + databaseKeyCipher = databaseKeyCipher, + syncScheduler = syncScheduler, + processRestarter = processRestarter, + ) + + vm.onAuthenticated() + advanceUntilIdle() + + coVerify { databaseKeyStore.setClearPending() } + verify { processRestarter.restart() } + } + + @Test + fun `onAuthenticated re-locks for a retry when the auth window elapsed`() = runTest(dispatcher) { + stubLog() + val gate = mockk(relaxed = true) + every { gate.state } returns LockState.LOCKED + val session = mockk(relaxed = true) + every { session.isUnlocked() } returns false + val databaseKeyStore = mockk(relaxed = true) + coEvery { databaseKeyStore.hasAuthSealedPassphrase() } returns true + coEvery { databaseKeyStore.unlockWithAuth() } throws mockk(relaxed = true) + val databaseKeyCipher = mockk(relaxed = true) + every { databaseKeyCipher.hasKey() } returns true + val processRestarter = mockk(relaxed = true) + val vm = viewModel( + gate = gate, + session = session, + databaseKeyStore = databaseKeyStore, + databaseKeyCipher = databaseKeyCipher, + processRestarter = processRestarter, + ) + + vm.onAuthenticated() + advanceUntilIdle() + + // A lapsed auth window is transient: re-lock and let the user retry — never wipe the cache. + verify { gate.lock() } + assertIs(vm.uiState.value) + verify(exactly = 0) { processRestarter.restart() } + } + + @Test + fun `onAuthenticated re-locks for a retry on an ambiguous unwrap failure`() = runTest(dispatcher) { + stubLog() + val gate = mockk(relaxed = true) + every { gate.state } returns LockState.LOCKED + val session = mockk(relaxed = true) + every { session.isUnlocked() } returns false + val databaseKeyStore = mockk(relaxed = true) + coEvery { databaseKeyStore.hasAuthSealedPassphrase() } returns true + coEvery { databaseKeyStore.unlockWithAuth() } throws IllegalStateException("corrupt sealed blob") + val databaseKeyCipher = mockk(relaxed = true) + every { databaseKeyCipher.hasKey() } returns true + val processRestarter = mockk(relaxed = true) + val vm = viewModel( + gate = gate, + session = session, + databaseKeyStore = databaseKeyStore, + databaseKeyCipher = databaseKeyCipher, + processRestarter = processRestarter, + ) + + vm.onAuthenticated() + advanceUntilIdle() + + // An ambiguous error must NOT wipe the cache on an otherwise-successful auth: re-lock + retry. + verify { gate.lock() } + verify(exactly = 0) { processRestarter.restart() } + } + + @Test + fun `onAuthenticated with no seal and encryption off unlocks without arming`() = runTest(dispatcher) { + val gate = mockk(relaxed = true) + every { gate.state } returns LockState.UNLOCKED + val session = mockk(relaxed = true) + every { session.isUnlocked() } returns false + val databaseKeyStore = mockk(relaxed = true) + coEvery { databaseKeyStore.hasAuthSealedPassphrase() } returns false + val vm = viewModel( + gate = gate, + session = session, + databaseKeyStore = databaseKeyStore, + encryptCache = false, + ) + + vm.onAuthenticated() + advanceUntilIdle() + + // App-lock is a pure UI gate this session: there is no encrypted cache to arm. + coVerify(exactly = 0) { databaseKeyStore.sealWithAuth() } + verify { gate.onAuthenticated() } + assertIs(vm.uiState.value) + } + + @Test + fun `onAuthenticated arms a fresh auth seal when encryption is on and none exists`() = runTest(dispatcher) { + val gate = mockk(relaxed = true) + every { gate.state } returns LockState.UNLOCKED + val session = mockk(relaxed = true) + every { session.isUnlocked() } returns false + val databaseKeyStore = mockk(relaxed = true) + coEvery { databaseKeyStore.hasAuthSealedPassphrase() } returns false + val vm = viewModel( + gate = gate, + session = session, + databaseKeyStore = databaseKeyStore, + encryptCache = true, + ) + + vm.onAuthenticated() + advanceUntilIdle() + + coVerify { databaseKeyStore.sealWithAuth() } + verify { gate.onAuthenticated() } + assertIs(vm.uiState.value) + } + + @Test + fun `onAuthenticated re-locks for a retry when arming the auth seal fails`() = runTest(dispatcher) { + stubLog() + val gate = mockk(relaxed = true) + every { gate.state } returns LockState.LOCKED + val session = mockk(relaxed = true) + every { session.isUnlocked() } returns false + val databaseKeyStore = mockk(relaxed = true) + coEvery { databaseKeyStore.hasAuthSealedPassphrase() } returns false + coEvery { databaseKeyStore.sealWithAuth() } throws IllegalStateException("keystore busy") + val processRestarter = mockk(relaxed = true) + val vm = viewModel( + gate = gate, + session = session, + databaseKeyStore = databaseKeyStore, + encryptCache = true, + processRestarter = processRestarter, + ) + + vm.onAuthenticated() + advanceUntilIdle() + + verify { gate.lock() } + assertIs(vm.uiState.value) + verify(exactly = 0) { processRestarter.restart() } + } + + /** A [SyncScheduler] whose `syncNow()` returns an [Operation] whose result future can be stubbed. */ + private fun enqueueingScheduler(): Pair, SyncScheduler> { + val future = mockk>() + every { future.get(any(), any()) } returns Operation.SUCCESS + val operation = mockk { every { result } returns future } + val syncScheduler = mockk { every { syncNow() } returns operation } + return future to syncScheduler + } + + /** + * A ViewModel whose next `onForeground()` resolves to a cache-clear + restart: the pure decision + * table is stubbed to CLEAR_AND_REQUIRE_AUTH so the test drives the recovery path deterministically + * without reproducing the full key-invalidation device state (that logic is KeyInvalidationPolicyTest). + */ + private fun clearOnForegroundViewModel( + databaseKeyStore: DatabaseKeyStore = mockk(relaxed = true), + syncScheduler: SyncScheduler = mockk(relaxed = true), + processRestarter: ProcessRestarter = mockk(relaxed = true), + ): AppLockViewModel { + mockkStatic(SystemClock::class) + every { SystemClock.elapsedRealtime() } returns 1_000L + // android.util.Log is a no-op stub that throws "not mocked" in JVM tests; the timeout path logs. + mockkStatic(Log::class) + every { Log.w(any(), any(), any()) } returns 0 + mockkObject(KeyInvalidationPolicy) + every { KeyInvalidationPolicy.decide(any(), any(), any(), any()) } returns LockAction.CLEAR_AND_REQUIRE_AUTH + return viewModel( + gate = mockk(relaxed = true), + databaseKeyStore = databaseKeyStore, + syncScheduler = syncScheduler, + processRestarter = processRestarter, + ) + } + + /** + * Builds a ViewModel whose next `onForeground()` resolves to [action] by stubbing the pure decision + * table directly (its own exhaustive coverage is KeyInvalidationPolicyTest) plus the Android statics + * `onForeground` touches, so each [LockAction] branch is driven without reproducing device state. + */ + private fun foregroundResolving( + action: LockAction, + gate: AppLockGate = mockk(relaxed = true), + settingsRepository: SettingsRepository = mockk(relaxed = true), + databaseKeyStore: DatabaseKeyStore = mockk(relaxed = true), + syncScheduler: SyncScheduler = mockk(relaxed = true), + processRestarter: ProcessRestarter = mockk(relaxed = true), + ): AppLockViewModel { + mockkStatic(SystemClock::class) + every { SystemClock.elapsedRealtime() } returns FOREGROUND_AT + stubLog() + mockkObject(KeyInvalidationPolicy) + every { KeyInvalidationPolicy.decide(any(), any(), any(), any()) } returns action + return viewModel( + gate = gate, + settingsRepository = settingsRepository, + databaseKeyStore = databaseKeyStore, + syncScheduler = syncScheduler, + processRestarter = processRestarter, + ) + } + + /** android.util.Log is a no-op stub that throws "not mocked" in JVM tests; the recovery paths log. */ + private fun stubLog() { + mockkStatic(Log::class) + every { Log.w(any(), any()) } returns 0 + every { Log.w(any(), any(), any()) } returns 0 + } + + private companion object { + const val FOREGROUND_AT = 1_000L + } } diff --git a/app/src/test/kotlin/org/libremail/ui/onboarding/OnboardingViewModelTest.kt b/app/src/test/kotlin/org/libremail/ui/onboarding/OnboardingViewModelTest.kt index 4000aa5..dcf8d00 100644 --- a/app/src/test/kotlin/org/libremail/ui/onboarding/OnboardingViewModelTest.kt +++ b/app/src/test/kotlin/org/libremail/ui/onboarding/OnboardingViewModelTest.kt @@ -16,6 +16,7 @@ import kotlinx.coroutines.test.setMain import org.junit.After import org.junit.Before import org.junit.Test +import org.libremail.contacts.ContactsPermissionManager import org.libremail.data.settings.SettingsRepository import org.libremail.push.BatteryOptimizationManager import kotlin.test.assertEquals @@ -40,13 +41,25 @@ class OnboardingViewModelTest { every { isIgnoringBatteryOptimizations() } returns unrestricted } - private fun settingsRepository(handled: Boolean = false) = mockk { - coEvery { isBatteryPromptHandled() } returns handled + private fun contactsManager(granted: Boolean = false) = mockk { + every { hasPermission() } returns granted } + private fun settingsRepository(batteryHandled: Boolean = false, contactsHandled: Boolean = false) = + mockk { + coEvery { isBatteryPromptHandled() } returns batteryHandled + coEvery { isContactsPromptHandled() } returns contactsHandled + } + + private fun viewModel( + battery: BatteryOptimizationManager = batteryManager(), + contacts: ContactsPermissionManager = contactsManager(), + settings: SettingsRepository = settingsRepository(), + ) = OnboardingViewModel(battery, contacts, settings) + @Test fun `battery prompt is needed when not unrestricted and not handled`() = runTest(testDispatcher) { - val vm = OnboardingViewModel(batteryManager(unrestricted = false), settingsRepository(handled = false)) + val vm = viewModel(battery = batteryManager(unrestricted = false), settings = settingsRepository()) assertEquals(true, vm.batteryPromptNeeded.value) assertFalse(vm.batteryUnrestricted.value) @@ -54,7 +67,7 @@ class OnboardingViewModelTest { @Test fun `battery prompt is skipped when the app is already unrestricted`() = runTest(testDispatcher) { - val vm = OnboardingViewModel(batteryManager(unrestricted = true), settingsRepository(handled = false)) + val vm = viewModel(battery = batteryManager(unrestricted = true)) assertEquals(false, vm.batteryPromptNeeded.value) assertTrue(vm.batteryUnrestricted.value) @@ -62,14 +75,46 @@ class OnboardingViewModelTest { @Test fun `battery prompt is skipped once it has been handled`() = runTest(testDispatcher) { - val vm = OnboardingViewModel(batteryManager(unrestricted = false), settingsRepository(handled = true)) + val vm = viewModel( + battery = batteryManager(unrestricted = false), + settings = settingsRepository(batteryHandled = true), + ) assertEquals(false, vm.batteryPromptNeeded.value) } + @Test + fun `contacts prompt is needed when not granted and not handled`() = runTest(testDispatcher) { + val vm = viewModel( + contacts = contactsManager(granted = false), + settings = settingsRepository(contactsHandled = false), + ) + + assertEquals(true, vm.contactsPromptNeeded.value) + assertFalse(vm.contactsGranted.value) + } + + @Test + fun `contacts prompt is skipped when the permission is already granted`() = runTest(testDispatcher) { + val vm = viewModel(contacts = contactsManager(granted = true)) + + assertEquals(false, vm.contactsPromptNeeded.value) + assertTrue(vm.contactsGranted.value) + } + + @Test + fun `contacts prompt is skipped once it has been handled`() = runTest(testDispatcher) { + val vm = viewModel( + contacts = contactsManager(granted = false), + settings = settingsRepository(contactsHandled = true), + ) + + assertEquals(false, vm.contactsPromptNeeded.value) + } + @Test fun `only the first added account id is remembered`() = runTest(testDispatcher) { - val vm = OnboardingViewModel(batteryManager(), settingsRepository()) + val vm = viewModel() assertNull(vm.firstAddedAccountId) vm.onAccountAdded("imap:first@example.com") @@ -79,16 +124,48 @@ class OnboardingViewModelTest { } @Test - fun `marking the prompt handled persists the flag`() = runTest(testDispatcher) { + fun `marking the battery prompt handled persists the flag`() = runTest(testDispatcher) { val repo = settingsRepository() coEvery { repo.setBatteryPromptHandled(any()) } just Runs - val vm = OnboardingViewModel(batteryManager(), repo) + val vm = viewModel(settings = repo) vm.markBatteryPromptHandled() coVerify { repo.setBatteryPromptHandled(true) } } + @Test + fun `marking the contacts prompt handled persists the flag`() = runTest(testDispatcher) { + val repo = settingsRepository() + coEvery { repo.setContactsPromptHandled(any()) } just Runs + val vm = viewModel(settings = repo) + + vm.markContactsPromptHandled() + + coVerify { repo.setContactsPromptHandled(true) } + } + + @Test + fun `marking the contacts permission requested persists the flag`() = runTest(testDispatcher) { + val repo = settingsRepository() + coEvery { repo.setContactsPermissionRequested(any()) } just Runs + val vm = viewModel(settings = repo) + + vm.markContactsPermissionRequested() + + coVerify { repo.setContactsPermissionRequested(true) } + } + + @Test + fun `a granted permission result flips contactsGranted on`() = runTest(testDispatcher) { + val vm = viewModel(contacts = contactsManager(granted = false)) + assertFalse(vm.contactsGranted.value) + + vm.onContactsPermissionResult(true) + + assertTrue(vm.contactsGranted.value) + } + @Test fun `refresh re-reads the live battery status`() = runTest(testDispatcher) { val manager = mockk { @@ -96,11 +173,25 @@ class OnboardingViewModelTest { // First read (init) is not-unrestricted; the second (refresh) reflects the user's change. every { isIgnoringBatteryOptimizations() } returnsMany listOf(false, true) } - val vm = OnboardingViewModel(manager, settingsRepository()) + val vm = viewModel(battery = manager) assertFalse(vm.batteryUnrestricted.value) vm.refreshBatteryStatus() assertTrue(vm.batteryUnrestricted.value) } + + @Test + fun `refresh re-reads the live contacts grant`() = runTest(testDispatcher) { + val manager = mockk { + // First reads (init) report not-granted; a later read reflects the user granting it. + every { hasPermission() } returnsMany listOf(false, false, true) + } + val vm = viewModel(contacts = manager) + assertFalse(vm.contactsGranted.value) + + vm.refreshContactsStatus() + + assertTrue(vm.contactsGranted.value) + } } diff --git a/app/src/test/kotlin/org/libremail/ui/reader/InlineImageResolverTest.kt b/app/src/test/kotlin/org/libremail/ui/reader/InlineImageResolverTest.kt new file mode 100644 index 0000000..c753a4b --- /dev/null +++ b/app/src/test/kotlin/org/libremail/ui/reader/InlineImageResolverTest.kt @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.ui.reader + +import org.junit.Test +import org.libremail.domain.model.InlineImage +import kotlin.test.assertEquals +import kotlin.test.assertNull +import kotlin.test.assertSame + +/** + * Unit tests for the reader WebView's `cid:` resolution (issue #133): the pure logic that turns an + * `` request URL into the matching inline image's bytes, factored out of + * [HtmlBody] so it needs no WebView. Only `cid:` URLs are served; anything else falls through to the + * WebView's normal (remote-blockable) loading. + */ +class InlineImageResolverTest { + + private fun image(contentId: String) = InlineImage(contentId, "image/png", byteArrayOf(1, 2, 3)) + + @Test + fun `cidKey extracts and normalizes the Content-ID from a cid URL`() { + assertEquals("logo1", cidKey("cid:logo1")) + assertEquals("logo1", cidKey("cid:")) + assertEquals("a@b.example", cidKey("CID:a@b.example")) // scheme is case-insensitive + } + + @Test + fun `cidKey rejects non-cid and empty references`() { + assertNull(cidKey("https://example.com/tracker.png")) + assertNull(cidKey("data:image/png;base64,AAAA")) + assertNull(cidKey("cid:")) + } + + @Test + fun `resolveInlineImage returns the matching image for a cid reference`() { + val logo = image("logo1") + val images = mapOf("logo1" to logo, "banner" to image("banner")) + + assertSame(logo, resolveInlineImage("cid:logo1", images)) + assertSame(logo, resolveInlineImage("cid:", images)) + } + + @Test + fun `resolveInlineImage returns null for remote urls and unknown cids`() { + val images = mapOf("logo1" to image("logo1")) + + assertNull(resolveInlineImage("https://example.com/pixel.gif", images)) + assertNull(resolveInlineImage("cid:does-not-exist", images)) + assertNull(resolveInlineImage("cid:logo1", emptyMap())) + } +} diff --git a/app/src/test/kotlin/org/libremail/ui/settings/SettingsViewModelTest.kt b/app/src/test/kotlin/org/libremail/ui/settings/SettingsViewModelTest.kt new file mode 100644 index 0000000..00e50c6 --- /dev/null +++ b/app/src/test/kotlin/org/libremail/ui/settings/SettingsViewModelTest.kt @@ -0,0 +1,144 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.ui.settings + +import io.mockk.coEvery +import io.mockk.coVerify +import io.mockk.coVerifyOrder +import io.mockk.every +import io.mockk.mockk +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.test.UnconfinedTestDispatcher +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.resetMain +import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.test.setMain +import org.junit.After +import org.junit.Before +import org.junit.Test +import org.libremail.R +import org.libremail.data.security.AppLockManager +import org.libremail.data.security.DatabaseKeyStore +import org.libremail.data.settings.AppSettings +import org.libremail.data.settings.SettingsRepository +import org.libremail.domain.repository.AccountRepository +import kotlin.test.assertEquals +import kotlin.test.assertNull + +/** + * Covers [SettingsViewModel.setAppLock]'s security-critical branches (issue #100): enabling is rejected + * without a secure device lock; disabling reseals the cache passphrase under the non-auth master key + * BEFORE dropping the gate, and keeps app-lock on if that reseal fails (so the passphrase is never + * stranded). JVM-testable with the repo's existing MockK pattern — no device needed. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class SettingsViewModelTest { + + private val dispatcher = UnconfinedTestDispatcher() + + @Before + fun setUp() = Dispatchers.setMain(dispatcher) + + @After + fun tearDown() = Dispatchers.resetMain() + + @Test + fun `enabling app-lock without a secure device is rejected and does not persist`() = runTest(dispatcher) { + val appLockManager = mockk() + every { appLockManager.isDeviceSecure() } returns false + val settingsRepository = mockk(relaxed = true) + val vm = viewModel(appLockManager = appLockManager, settingsRepository = settingsRepository) + + vm.setAppLock(true) + advanceUntilIdle() + + // No secure lock means nothing to authenticate against: reject with a message, persist nothing. + assertEquals(R.string.app_lock_needs_device_lock, vm.appLockMessage.value) + coVerify(exactly = 0) { settingsRepository.setAppLock(any()) } + } + + @Test + fun `enabling app-lock on a secure device persists the setting`() = runTest(dispatcher) { + val appLockManager = mockk() + every { appLockManager.isDeviceSecure() } returns true + val settingsRepository = mockk(relaxed = true) + val vm = viewModel(appLockManager = appLockManager, settingsRepository = settingsRepository) + + vm.setAppLock(true) + advanceUntilIdle() + + coVerify { settingsRepository.setAppLock(true) } + assertNull(vm.appLockMessage.value) + } + + @Test + fun `disabling app-lock reseals under the master key before dropping the gate`() = runTest(dispatcher) { + val databaseKeyStore = mockk(relaxed = true) + coEvery { databaseKeyStore.hasAuthSealedPassphrase() } returns true + val settingsRepository = mockk(relaxed = true) + val vm = viewModel(databaseKeyStore = databaseKeyStore, settingsRepository = settingsRepository) + + vm.setAppLock(false) + advanceUntilIdle() + + // Reseal so the cache opens without auth again, THEN drop the gate — reversing the order would + // leave the next launch unable to open a still-auth-sealed cache. + coVerifyOrder { + databaseKeyStore.sealWithMaster() + settingsRepository.setAppLock(false) + } + } + + @Test + fun `disabling app-lock keeps the lock on when resealing fails`() = runTest(dispatcher) { + val databaseKeyStore = mockk(relaxed = true) + coEvery { databaseKeyStore.hasAuthSealedPassphrase() } returns true + coEvery { databaseKeyStore.sealWithMaster() } throws IllegalStateException("keystore busy") + val settingsRepository = mockk(relaxed = true) + val vm = viewModel(databaseKeyStore = databaseKeyStore, settingsRepository = settingsRepository) + + vm.setAppLock(false) + advanceUntilIdle() + + // Resealing failed: surface a message and keep app-lock ON rather than strand the passphrase + // under a gate we just dropped. + assertEquals(R.string.app_lock_disable_failed, vm.appLockMessage.value) + coVerify(exactly = 0) { settingsRepository.setAppLock(false) } + } + + @Test + fun `disabling app-lock with no auth seal just drops the gate`() = runTest(dispatcher) { + val databaseKeyStore = mockk(relaxed = true) + coEvery { databaseKeyStore.hasAuthSealedPassphrase() } returns false + val settingsRepository = mockk(relaxed = true) + val vm = viewModel(databaseKeyStore = databaseKeyStore, settingsRepository = settingsRepository) + + vm.setAppLock(false) + advanceUntilIdle() + + // Nothing auth-sealed to reseal: skip the master reseal and simply drop the gate. + coVerify(exactly = 0) { databaseKeyStore.sealWithMaster() } + coVerify { settingsRepository.setAppLock(false) } + } + + private fun viewModel( + appLockManager: AppLockManager = mockk(relaxed = true), + databaseKeyStore: DatabaseKeyStore = mockk(relaxed = true), + settingsRepository: SettingsRepository = mockk(relaxed = true), + ): SettingsViewModel { + every { settingsRepository.settings } returns flowOf(AppSettings()) + every { settingsRepository.contactsPermissionRequested } returns flowOf(false) + val accountRepository = mockk(relaxed = true) + every { accountRepository.observeAccounts() } returns flowOf(emptyList()) + return SettingsViewModel( + accountRepository = accountRepository, + settingsRepository = settingsRepository, + appLockManager = appLockManager, + databaseKeyStore = databaseKeyStore, + batteryOptimizationManager = mockk(relaxed = true), + contactsPermissionManager = mockk(relaxed = true), + syncScheduler = mockk(relaxed = true), + ) + } +} diff --git a/docs/play-permissions.md b/docs/play-permissions.md index a5b4568..833d2d9 100644 --- a/docs/play-permissions.md +++ b/docs/play-permissions.md @@ -39,16 +39,22 @@ Nothing else. Notably **absent** (worth stating in any review exchange): - **Data handling:** query and results are entirely **on-device** (results live in memory for the suggestion dropdown). Nothing from the contacts provider is stored, logged, or transmitted; an address reaches the network only if the user puts it on an email they send. -- **Request flow:** first composition of the compose screen (`ui/compose/ComposeScreen.kt:101`); - denial is handled gracefully — `ContactsRepository.search` returns empty and composing works - normally (manual address entry). +- **Request flow:** a dedicated, skippable **onboarding step** (`ui/onboarding/ContactsAccessScreen.kt`, + route `ONBOARDING_CONTACTS`) requests it **once**, showing an in-context rationale up front — + contacts are used only for on-device autocomplete and never uploaded (#127, #128). The compose + screen no longer prompts; it only reads the current grant. If declined, recipient autocomplete can + be enabled later from **Settings → Contacts → Recipient autocomplete** (`ui/settings/SettingsScreen.kt`), + which re-requests in-app when possible or deep-links to the app's system settings when the + permission is permanently denied (#129). Denial is handled gracefully throughout — + `ContactsRepository.search` returns empty and composing works normally (manual address entry). - **Play-Console justification text (if asked in review):** > LibreMail is an email client. READ_CONTACTS powers recipient autocomplete on the compose > screen only: the app queries the on-device contacts provider for names/email addresses > matching what the user typed and shows up to 8 suggestions. Contact data is processed > entirely on the device — it is never uploaded, stored outside the suggestion list, or shared. - > The permission is requested in context (first open of the compose screen) and the feature - > degrades gracefully if denied. + > The permission is requested once, in context, from a skippable onboarding step that explains + > the on-device autocomplete use before asking (and can be enabled later from Settings); the + > feature degrades gracefully if denied. ## `POST_NOTIFICATIONS`