From d877f50fd9c5494dcaa5f908791753be9570d2d9 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 09:37:32 -0500 Subject: [PATCH 1/8] feat(contacts): move contacts permission to onboarding + settings Recipient autocomplete's READ_CONTACTS permission was requested lazily on every compose-screen open (a LaunchedEffect(Unit)), re-prompting users who had declined. Move the request to a dedicated, skippable onboarding step and add a Settings entry to turn it on later, each with an in-context rationale. - #127: new skippable ONBOARDING_CONTACTS step (mirrors the battery step), requested once. ComposeScreen no longer prompts; it only reads the current grant on resume, so a grant made later (e.g. from Settings) still takes effect the next time compose opens. - #128: the onboarding step and the Settings request show a short rationale (contacts are used only for on-device autocomplete, never uploaded) and handle shouldShowRequestPermissionRationale so a re-request explains itself. docs/play-permissions.md updated to match. - #129: Settings -> Contacts -> Recipient autocomplete reflects on / off / blocked-in-settings; requests in-app when grantable, deep-links to the app's system settings when permanently denied. Graceful degradation is preserved: ContactsRepository.search still runCatch-es, ComposeViewModel.searchContacts() still guards on contactsAllowed, and the suggestion list still renders only when non-empty. Adds a pure ContactPermissionDecision (JVM unit-tested), extends the onboarding view-model tests, and adds Compose UI tests for the onboarding step (skip / grant / deny / rationale) and the Settings row states. Co-Authored-By: Claude Fable 5 --- .../libremail/ui/compose/ComposeScreenTest.kt | 5 +- .../onboarding/BatteryOptimizationStepTest.kt | 7 +- .../ui/onboarding/ContactsAccessStepTest.kt | 97 ++++++++++ .../ui/onboarding/OnboardingFlowTest.kt | 2 + .../ui/settings/ContactAutocompleteRowTest.kt | 62 +++++++ .../ui/settings/SettingsScreenTest.kt | 13 ++ .../contacts/ContactPermissionState.kt | 37 ++++ .../contacts/ContactsPermissionManager.kt | 39 ++++ .../data/settings/SettingsRepository.kt | 25 +++ .../kotlin/org/libremail/ui/LibreMailApp.kt | 43 ++++- .../org/libremail/ui/compose/ComposeScreen.kt | 25 ++- .../org/libremail/ui/navigation/Routes.kt | 5 + .../ui/onboarding/ContactsAccessScreen.kt | 172 ++++++++++++++++++ .../ui/onboarding/OnboardingViewModel.kt | 47 ++++- .../libremail/ui/settings/SettingsScreen.kt | 117 +++++++++++- .../ui/settings/SettingsViewModel.kt | 19 ++ app/src/main/res/values/strings.xml | 22 +++ .../contacts/ContactPermissionDecisionTest.kt | 52 ++++++ .../ui/onboarding/OnboardingViewModelTest.kt | 109 ++++++++++- docs/play-permissions.md | 16 +- 20 files changed, 871 insertions(+), 43 deletions(-) create mode 100644 app/src/androidTest/kotlin/org/libremail/ui/onboarding/ContactsAccessStepTest.kt create mode 100644 app/src/androidTest/kotlin/org/libremail/ui/settings/ContactAutocompleteRowTest.kt create mode 100644 app/src/main/kotlin/org/libremail/contacts/ContactPermissionState.kt create mode 100644 app/src/main/kotlin/org/libremail/contacts/ContactsPermissionManager.kt create mode 100644 app/src/main/kotlin/org/libremail/ui/onboarding/ContactsAccessScreen.kt create mode 100644 app/src/test/kotlin/org/libremail/contacts/ContactPermissionDecisionTest.kt 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/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/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/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/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/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 8521a5d..b86f50d 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -151,6 +151,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) @@ -288,6 +298,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/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/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/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` From a7a7c323b51005c6cdb1abd4f1ee9da250ff1be7 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 09:45:54 -0500 Subject: [PATCH 2/8] refactor(security): derive backup exclusion set from DatabaseFiles Make BackupPolicy.EXCLUDED_DATABASE_PATHS the true single source of truth by deriving it from DatabaseFiles.NAME and DatabaseFiles.ACCOUNTS_NAME plus their SQLite sidecars via a new DatabaseFiles.fileNames() helper, instead of a hand-maintained list. This adds libremail-accounts.db (accounts + encrypted credentials, split into their own DB by #118/#111) to the never-back-up set, matching the field's stated intent, so a newly added database can never silently fall out of the exclusions again. Also fix DatabaseFiles.clear to wipe the cache DB via context.deleteDatabase(NAME), which additionally removes the -mj* master-journal temp files the hand-rolled suffix list missed. It still wipes ONLY the cache DB (NAME) and never the accounts DB (ACCOUNTS_NAME), preserving the sign-in-survives-cache-wipe separation from #111. Update the backup XML comments (data_extraction_rules.xml, backup_rules.xml) to note libremail-accounts.db is also kept off-device by the strict include- allowlist, and extend the tests to assert the accounts DB is covered by the exclusion SoT and that the derivation stays in lockstep with the XML resources. There is no active backup leak today: the XML is a strict include-allowlist, so the accounts DB was already excluded by omission. This closes the SoT drift #118 introduced and the -mj* gap, so the security posture no longer depends on the allowlist staying strict by luck. Closes #103 Co-Authored-By: Claude Fable 5 --- .../org/libremail/backup/BackupPolicy.kt | 18 ++++++----- .../org/libremail/data/local/DatabaseFiles.kt | 31 +++++++++++++------ app/src/main/res/xml/backup_rules.xml | 9 +++--- .../main/res/xml/data_extraction_rules.xml | 7 +++-- .../org/libremail/backup/BackupPolicyTest.kt | 28 ++++++++++++++++- .../backup/DataExtractionRulesTest.kt | 10 ++++++ 6 files changed, 79 insertions(+), 24 deletions(-) diff --git a/app/src/main/kotlin/org/libremail/backup/BackupPolicy.kt b/app/src/main/kotlin/org/libremail/backup/BackupPolicy.kt index 2ba34a6..1d9ba2c 100644 --- a/app/src/main/kotlin/org/libremail/backup/BackupPolicy.kt +++ b/app/src/main/kotlin/org/libremail/backup/BackupPolicy.kt @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-3.0-or-later package org.libremail.backup +import org.libremail.data.local.DatabaseFiles import org.libremail.data.settings.AppSettings /** @@ -24,13 +25,16 @@ object BackupPolicy { "datastore/libremail_dbkey.preferences_pb", ) - /** `databases`-dir-relative names that must never leave the device (encrypted credentials + mail cache). */ - val EXCLUDED_DATABASE_PATHS: List = 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/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/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}")) + } } From ed9b9e2742a96bf2e35073c9ccfce2c2f16f8223 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 10:01:46 -0500 Subject: [PATCH 3/8] fix(di): defer database provisioning off the Hilt inject path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DatabaseModule.provideDatabase ran the whole startup sequence with runBlocking while Hilt constructed the singleton database — a DataStore read, a Keystore op, a possible SQLCipher re-key conversion, and (since #111) the cross-database AccountDataMigrator — synchronously on whichever thread first injected it, which can be the main thread (jank / ANR). Move that work behind DatabaseProvisioner.prepareCache(): a memoized, mutex-guarded suspend that runs the same sequence, in the same order, on the IO dispatcher. Both databases' Room builders now open through a DeferredOpenHelperFactory whose delegate — and therefore the gate — is materialised only when Room first OPENS the database, on its background query executor, never at inject time. AccountDatabase's open gates on the same prepareCache(), preserving the #111 migrate-before-open ordering that the old construction-time dependency on LibreMailDatabase enforced. Behaviour, ordering, and crash-safety are unchanged — only where and when the work runs moved off the (possibly main) inject thread. Co-Authored-By: Claude Fable 5 --- .../data/local/DatabaseProvisioner.kt | 136 ++++++++++++ .../data/local/DeferredOpenHelperFactory.kt | 75 +++++++ .../org/libremail/di/AccountDatabaseModule.kt | 26 ++- .../kotlin/org/libremail/di/DatabaseModule.kt | 90 +++----- .../data/local/DatabaseProvisionerTest.kt | 193 ++++++++++++++++++ .../local/DeferredOpenHelperFactoryTest.kt | 107 ++++++++++ 6 files changed, 557 insertions(+), 70 deletions(-) create mode 100644 app/src/main/kotlin/org/libremail/data/local/DatabaseProvisioner.kt create mode 100644 app/src/main/kotlin/org/libremail/data/local/DeferredOpenHelperFactory.kt create mode 100644 app/src/test/kotlin/org/libremail/data/local/DatabaseProvisionerTest.kt create mode 100644 app/src/test/kotlin/org/libremail/data/local/DeferredOpenHelperFactoryTest.kt 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/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..8c0b43b 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 @@ -36,8 +37,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 +45,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, @@ -70,59 +64,29 @@ object DatabaseModule { MIGRATION_14_15, MIGRATION_15_16, ) - // 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/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) + } +} From 15c4cd9ae8928c4e4dee2d47f0575ea752a31ca8 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 10:09:14 -0500 Subject: [PATCH 4/8] fix(security): harden app-lock recovery restart MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The key-invalidation recovery restart was unreliable in two ways, both in AppLockViewModel: 1. Same-process self-restart race: restartProcess() did context.startActivity(...) immediately followed by Runtime.exit(0) in the same process, so ActivityManager could schedule the relaunch into the process being killed and drop it — the app just closed, recovering only on the next manual launch. Fixed with a ProcessPhoenix-style separate-process trampoline (RestartActivity in a distinct ":restart" process, driven by ProcessRestarter): it kills the original process by PID and only then relaunches, so the relaunch is issued from a process that survives the kill. No new dependency; LibreMailApplication early-returns in the ":restart" process so it runs no normal startup work. 2. Lost syncNow() enqueue: clearCacheAndRestart() enqueued the post-wipe re-sync fire-and-forget, but WorkManager persists the WorkSpec asynchronously on its serial task executor, so exiting raced that insert and could drop the re-sync (now user-visible after #118: an empty mailbox until the next periodic sync). syncNow() now returns its enqueue Operation, and clearCacheAndRestart awaits it (bounded by a 5s timeout) before restarting, so the WorkSpec is durably persisted first. CLEAR_PENDING recovery-flag semantics are preserved; the cache wipe still happens at cold start in DatabaseModule (unchanged). Tests: JVM unit tests assert the enqueue Operation is awaited before the restart is triggered (order) and that a timed-out enqueue still restarts; SyncSchedulerTest pins syncNow() returning the enqueue Operation. The separate-process kill/relaunch is device-only and noted for on-device wipe+resync verification. Closes #99 Co-Authored-By: Claude Fable 5 --- app/src/main/AndroidManifest.xml | 13 ++ .../org/libremail/LibreMailApplication.kt | 11 ++ .../org/libremail/data/sync/SyncScheduler.kt | 12 +- .../org/libremail/restart/ProcessRestarter.kt | 44 +++++++ .../org/libremail/restart/RestartActivity.kt | 59 +++++++++ .../org/libremail/ui/lock/AppLockViewModel.kt | 68 +++++++++-- .../libremail/data/sync/SyncSchedulerTest.kt | 15 +++ .../libremail/ui/lock/AppLockViewModelTest.kt | 114 ++++++++++++++++-- 8 files changed, 316 insertions(+), 20 deletions(-) create mode 100644 app/src/main/kotlin/org/libremail/restart/ProcessRestarter.kt create mode 100644 app/src/main/kotlin/org/libremail/restart/RestartActivity.kt 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" /> + + + () .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/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/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/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/ui/lock/AppLockViewModelTest.kt b/app/src/test/kotlin/org/libremail/ui/lock/AppLockViewModelTest.kt index 7efcaa0..61dcf56 100644 --- a/app/src/test/kotlin/org/libremail/ui/lock/AppLockViewModelTest.kt +++ b/app/src/test/kotlin/org/libremail/ui/lock/AppLockViewModelTest.kt @@ -2,8 +2,13 @@ package org.libremail.ui.lock import android.os.SystemClock +import android.util.Log +import androidx.work.Operation +import com.google.common.util.concurrent.ListenableFuture +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 +16,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 +24,14 @@ import org.junit.After import org.junit.Before import org.junit.Test import org.libremail.data.security.AppLockGate +import org.libremail.data.security.DatabaseKeyStore +import org.libremail.data.security.KeyInvalidationPolicy +import org.libremail.data.security.LockAction 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 @@ -29,6 +41,11 @@ import kotlin.test.assertIs * 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. + * + * The recovery-restart tests (#99) 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. */ @OptIn(ExperimentalCoroutinesApi::class) class AppLockViewModelTest { @@ -44,19 +61,27 @@ 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), + ): AppLockViewModel { + every { settingsRepository.settings } returns flowOf(AppSettings(appLock = appLock)) return AppLockViewModel( context = mockk(relaxed = true), - settingsRepository = settings, + settingsRepository = settingsRepository, appLockManager = mockk(relaxed = true), - databaseKeyStore = mockk(relaxed = true), + databaseKeyStore = databaseKeyStore, databaseKeyCipher = mockk(relaxed = true), session = mockk(relaxed = true), - syncScheduler = mockk(relaxed = true), + 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 +109,79 @@ 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() } + } + + /** 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, + ) + } } From 3971e89d1ed445c4b6220e8287da8127f12f4611 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 10:44:05 -0500 Subject: [PATCH 5/8] fix(reader): render inline cid: images in HTML emails Inline images in rich HTML emails (embedded via Content-ID and , e.g. USPS Informed Delivery digests) were listed under Attachments with a download button and never rendered in the body. Two bugs combined; both are fixed here. 1. Misclassification: ImapClient classified any part with a filename as an attachment, sweeping inline images (which carry a filename AND a Content-ID under Content-Disposition: inline) into the list. A part is now a downloadable attachment only when its disposition is attachment, or it has a filename but no Content-ID; an inline image is collected separately and excluded from the displayed list (AttachmentDao filters contentId IS NULL). The Content-ID is read via MimePart.getContentID() so it resolves from IMAP BODYSTRUCTURE rather than a per-part header fetch that Angus leaves unpopulated. 2. No rendering path: HtmlBody's WebViewClient now overrides shouldInterceptRequest to resolve cid: to the matching part's bytes (backing the CSP's existing cid: allowance). Content-ID is threaded end-to-end through AttachmentPart, Attachment, AttachmentEntity, and MailRepository.inlineImages(); ReaderViewModel surfaces the cid->bytes map to the WebView. Schema: adds attachments.contentId (v16 -> v17, MIGRATION_16_17). Tests: MIME-part classification (inline+cid excluded, real/disposition/ filename-only kept), a GreenMail multipart/related round-trip, the cid->bytes resolver, repository inlineImages(), the DAO display filter, and the v16->v17 migration. Closes #133 Co-Authored-By: Claude Fable 5 --- .../17.json | 460 ++++++++++++++++++ .../data/local/LibreMailDatabaseTest.kt | 19 + .../org/libremail/data/local/MigrationTest.kt | 31 ++ .../kotlin/org/libremail/ui/Fakes.kt | 3 + .../libremail/data/local/LibreMailDatabase.kt | 2 +- .../org/libremail/data/local/Mappers.kt | 2 + .../org/libremail/data/local/Migrations.kt | 14 + .../libremail/data/local/dao/AttachmentDao.kt | 12 +- .../data/local/entity/AttachmentEntity.kt | 6 + .../data/repository/MailRepositoryImpl.kt | 10 + .../kotlin/org/libremail/di/DatabaseModule.kt | 2 + .../org/libremail/domain/model/Attachment.kt | 6 + .../org/libremail/domain/model/InlineImage.kt | 11 + .../domain/repository/MailRepository.kt | 8 + .../kotlin/org/libremail/mail/ImapClient.kt | 56 ++- .../org/libremail/ui/reader/HtmlBody.kt | 57 ++- .../org/libremail/ui/reader/ReaderScreen.kt | 4 + .../libremail/ui/reader/ReaderViewModel.kt | 13 +- .../data/repository/MailRepositoryImplTest.kt | 26 + .../org/libremail/mail/ImapClientTest.kt | 88 ++++ .../mail/MimePartClassificationTest.kt | 85 ++++ .../ui/reader/InlineImageResolverTest.kt | 51 ++ 22 files changed, 951 insertions(+), 15 deletions(-) create mode 100644 app/schemas/org.libremail.data.local.LibreMailDatabase/17.json create mode 100644 app/src/main/kotlin/org/libremail/domain/model/InlineImage.kt create mode 100644 app/src/test/kotlin/org/libremail/mail/MimePartClassificationTest.kt create mode 100644 app/src/test/kotlin/org/libremail/ui/reader/InlineImageResolverTest.kt 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/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/di/DatabaseModule.kt b/app/src/main/kotlin/org/libremail/di/DatabaseModule.kt index d247ad1..9bbc8ba 100644 --- a/app/src/main/kotlin/org/libremail/di/DatabaseModule.kt +++ b/app/src/main/kotlin/org/libremail/di/DatabaseModule.kt @@ -21,6 +21,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 @@ -69,6 +70,7 @@ 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 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 001409d..5baed9c 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) @@ -476,7 +488,12 @@ class ImapClient @Inject constructor() { 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) @@ -486,6 +503,7 @@ class ImapClient @Inject constructor() { filename = attachmentName(part) ?: "attachment", mimeType = baseType(part), sizeBytes = part.size.toLong().coerceAtLeast(0L), + contentId = if (isInlineImagePart(part)) inlineContentId(part) else null, ) } } @@ -496,13 +514,10 @@ class ImapClient @Inject constructor() { 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) } @@ -566,3 +581,30 @@ class ImapClient @Inject constructor() { 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/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/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/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/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())) + } +} From 5777967375b0f69e4149846e8ec60f3da63de934 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 11:41:40 -0500 Subject: [PATCH 6/8] test(security): cover the app-lock security core Close the test-coverage gap on the app-lock security core (#100): the branching that decides when to WIPE user data or drop the lock, which shipped largely untested. - AppLockViewModelTest: pin the onAuthenticated unlock/arm classification (OK / UNRECOVERABLE / RETRY) and the onForeground LockAction dispatch -- DISABLE_APP_LOCK persists the setting, CLEAR_* set the pending flag and drop the gate BEFORE the awaited re-sync enqueue and process restart, and CLEAR_AND_REQUIRE_AUTH clears + restarts but keeps app-lock on. - KeyInvalidationPolicyTest: make the exhaustive 16-row decision table a test, with a completeness guard so no row can be dropped. The common (appLock on, encrypt off, secure, valid) -> REQUIRE_AUTH row is now pinned, so a mutation to PROCEED (a silent lock bypass) fails. - DatabaseKeyStoreTest: new JVM tests for the dual-seal exchange (sealWithAuth dropping SEALED_MASTER, sealWithMaster, resetSealedPassphrase, unlockWithAuth, clear-pending) pinning the "never both seals at once" and "not recoverable without auth" invariants. - SettingsViewModelTest: setAppLock reject / reseal / disable branches. To make the device-only DatabaseKeyStore crypto JVM-testable, add a minimal @VisibleForTesting DataStore seam (mirroring AppLockViewModel's injectable dispatcher); production still uses the real per-app DataStore. No crypto plumbing is refactored. Co-Authored-By: Claude Fable 5 --- .../data/security/DatabaseKeyStore.kt | 27 +- .../data/security/DatabaseKeyStoreTest.kt | 256 +++++++++++ .../security/KeyInvalidationPolicyTest.kt | 75 ++- .../libremail/ui/lock/AppLockViewModelTest.kt | 431 +++++++++++++++++- .../ui/settings/SettingsViewModelTest.kt | 144 ++++++ 5 files changed, 893 insertions(+), 40 deletions(-) create mode 100644 app/src/test/kotlin/org/libremail/data/security/DatabaseKeyStoreTest.kt create mode 100644 app/src/test/kotlin/org/libremail/ui/settings/SettingsViewModelTest.kt 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/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/ui/lock/AppLockViewModelTest.kt b/app/src/test/kotlin/org/libremail/ui/lock/AppLockViewModelTest.kt index 61dcf56..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,10 +1,15 @@ // 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 @@ -24,9 +29,13 @@ 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 @@ -40,12 +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 (#99) 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. + * 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 { @@ -68,15 +83,21 @@ class AppLockViewModelTest { 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 { - every { settingsRepository.settings } returns flowOf(AppSettings(appLock = appLock)) + val appSettings = AppSettings(appLock = appLock, encryptCache = encryptCache) + every { settingsRepository.settings } returns flowOf(appSettings) return AppLockViewModel( - context = mockk(relaxed = true), + context = context, settingsRepository = settingsRepository, - appLockManager = mockk(relaxed = true), + appLockManager = appLockManager, databaseKeyStore = databaseKeyStore, - databaseKeyCipher = mockk(relaxed = true), - session = mockk(relaxed = true), + databaseKeyCipher = databaseKeyCipher, + session = session, syncScheduler = syncScheduler, processRestarter = processRestarter, gate = gate, @@ -151,6 +172,358 @@ class AppLockViewModelTest { 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>() @@ -184,4 +557,42 @@ class AppLockViewModelTest { 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/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), + ) + } +} From d424abc6d31f98f9d2414d411b73c84b91272b53 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 12:21:09 -0500 Subject: [PATCH 7/8] refactor(security): de-duplicate AES-GCM Keystore plumbing and unify authenticator policy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract the AES-256-GCM Android Keystore plumbing that `KeystoreCrypto` and `DatabaseKeyCipher` copy-pasted (~60 lines) into a shared alias-parameterized base, `AesGcmKeystoreCipher`: the encrypt/decrypt bodies, existing-key lookup / get-or-create under a lock, key deletion, and the 5 identical GCM constants now live in ONE place. Each cipher keeps only its delta — the `KeyGenParameterSpec` (via `keySpecBuilder()`) and, for the auth-bound key, the invalidation handling. Preserve — deliberately — the two ciphers' different missing-key-on-decrypt behavior via a `generateKeyOnDecrypt` policy parameter, documented on the base: - master key (`KeystoreCrypto`, true): auto-generates on a missing alias, correct for a first-run key with nothing sealed yet. - auth-bound cache key (`DatabaseKeyCipher`, false): fails fast, because a missing auth-bound key means it was INVALIDATED and silently regenerating it would re-arm the lock against a cache that can no longer be decrypted. Also map the opaque `AEADBadTagException` (thrown when the master path generates a fresh key then can't decrypt old data) to a clear `GeneralSecurityException`, while leaving `KeyPermanentlyInvalidatedException` to propagate unwrapped. Unify the accepted-authenticator policy behind one source of truth, `AuthenticatorPolicy.ACCEPTED`, mapped into each API's vocabulary (`AppLockManager.AUTHENTICATORS` for BiometricManager / BiometricPrompt, `DatabaseKeyCipher.keySpec` for KeyProperties / KeyGenParameterSpec) so the two can no longer drift — a drift that yields a prompt that succeeds but a key that throws `UserNotAuthenticatedException` at use. Add JVM tests for the shared base (both `generateKeyOnDecrypt` modes + the AES-GCM error mapping) and for the authenticator mapping. #100's seal-exchange and policy-table safety net stays green. Closes #102 Co-Authored-By: Claude Fable 5 --- .../data/security/AesGcmKeystoreCipher.kt | 143 ++++++++++++++++++ .../libremail/data/security/AppLockManager.kt | 11 +- .../data/security/AuthenticatorPolicy.kt | 48 ++++++ .../data/security/DatabaseKeyCipher.kt | 83 +++------- .../libremail/data/security/KeystoreCrypto.kt | 64 ++------ .../data/security/AesGcmKeystoreCipherTest.kt | 121 +++++++++++++++ .../data/security/AuthenticatorPolicyTest.kt | 60 ++++++++ 7 files changed, 407 insertions(+), 123 deletions(-) create mode 100644 app/src/main/kotlin/org/libremail/data/security/AesGcmKeystoreCipher.kt create mode 100644 app/src/main/kotlin/org/libremail/data/security/AuthenticatorPolicy.kt create mode 100644 app/src/test/kotlin/org/libremail/data/security/AesGcmKeystoreCipherTest.kt create mode 100644 app/src/test/kotlin/org/libremail/data/security/AuthenticatorPolicyTest.kt 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/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/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, + ) + } +} From d0a5ccb10d8333fdc790ccbb7d3575c8ceda5d80 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 12:48:23 -0500 Subject: [PATCH 8/8] ci: auto-update armed PRs; drop dead merge_group trigger Co-Authored-By: Claude Fable 5 --- .github/workflows/autoupdate.yml | 39 ++++++++++++++++++++++++++++++++ .github/workflows/ci.yml | 4 ---- 2 files changed, 39 insertions(+), 4 deletions(-) create mode 100644 .github/workflows/autoupdate.yml 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: