From 9aaf34c95c44161bb244ab32de435275b0e6bf7a Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 04:38:22 -0500 Subject: [PATCH] refactor(security): app-lock UI plumbing cleanup (#104) Behavior-preserving cleanup of the app-lock UI plumbing: - SettingsScreen: replace the app's only Toast with the canonical SnackbarHostState + Scaffold(snackbarHost) + consume pattern for the app-lock rejection message (matches MailboxScreen); the ViewModel keeps the @StringRes id, resolved via LocalResources at the display boundary. - AppLockGateHost: replace the hand-rolled DisposableEffect + LifecycleEventObserver with LifecycleEventEffect, and the ContextWrapper findFragmentActivity() walk with LocalActivity; remember the derived activity and the authenticate lambda. - AppLockManager: delete the dead availability() API and the four-value AppLockAvailability enum (no production caller; the BIOMETRIC_STRONG or DEVICE_CREDENTIAL canAuthenticate combo is unsupported on minSdk 29). Keep isDeviceSecure() and AUTHENTICATORS. - AppLockViewModel: derive the gated uiState from the injected gate via a single publish() helper instead of hand-mirroring gate.state at each auth site; the transient Checking cover and app-lock-off unlocked states stay explicit (settings/lifecycle-driven, not session-gate-driven). Extend SettingsScreenTest with a Compose test for the rejection snackbar (now visible to Compose semantics) and drop availability() from its fake. Co-Authored-By: Claude Fable 5 --- .../ui/settings/SettingsScreenTest.kt | 54 ++++++++++++++----- .../libremail/data/security/AppLockManager.kt | 33 +----------- .../org/libremail/ui/lock/AppLockGateHost.kt | 54 +++++++------------ .../org/libremail/ui/lock/AppLockViewModel.kt | 41 ++++++++------ .../libremail/ui/settings/SettingsScreen.kt | 13 +++-- 5 files changed, 96 insertions(+), 99 deletions(-) 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 4db2142..2d986be 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/settings/SettingsScreenTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/settings/SettingsScreenTest.kt @@ -3,6 +3,7 @@ package org.libremail.ui.settings import androidx.activity.ComponentActivity import androidx.compose.ui.test.junit4.createAndroidComposeRule +import androidx.compose.ui.test.onAllNodesWithText import androidx.compose.ui.test.onNodeWithText import androidx.compose.ui.test.performClick import androidx.compose.ui.test.performScrollTo @@ -14,7 +15,6 @@ import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import org.libremail.R -import org.libremail.data.security.AppLockAvailability import org.libremail.data.security.AppLockManager import org.libremail.data.security.DatabaseKeyCipher import org.libremail.data.security.DatabaseKeyStore @@ -29,8 +29,10 @@ import org.libremail.ui.theme.LibreMailTheme import javax.inject.Provider /** - * End-to-end test for the top-level message-downloading setting: tapping a policy must round-trip - * through the real [SettingsRepository] (DataStore), proving the UI → ViewModel → persistence wiring. + * End-to-end tests for the global settings screen. Tapping a policy must round-trip through the real + * [SettingsRepository] (DataStore), proving the UI → ViewModel → persistence wiring; and enabling + * app-lock on a device with no secure lock must surface the rejection message via a snackbar — now + * visible to Compose semantics, unlike the former Toast it replaced. */ @RunWith(AndroidJUnit4::class) class SettingsScreenTest { @@ -38,27 +40,28 @@ class SettingsScreenTest { @get:Rule val composeTestRule = createAndroidComposeRule() + private val context = InstrumentationRegistry.getInstrumentation().targetContext.applicationContext + + // Device reports no secure lock, so enabling app-lock is rejected with a message. + private val insecureDevice = object : AppLockManager { + override fun isDeviceSecure() = false + } + private fun string(resId: Int) = composeTestRule.activity.getString(resId) - @Test - fun selectingFetchPolicy_persistsThroughTheRepository() { - val context = InstrumentationRegistry.getInstrumentation().targetContext.applicationContext - val settingsRepository = SettingsRepository(context) - runBlocking { settingsRepository.setFetchPolicy(FetchPolicy.ALWAYS) } // known starting state - val appLockManager = object : AppLockManager { - override fun isDeviceSecure() = false - override fun availability() = AppLockAvailability.NONE_ENROLLED - } + private fun settingsViewModel(settingsRepository: SettingsRepository): SettingsViewModel { val keyStore = DatabaseKeyStore(context, KeystoreCrypto(), DatabaseKeyCipher(), PassphraseSession()) - val viewModel = SettingsViewModel( + return SettingsViewModel( FakeAccountRepository(), settingsRepository, - appLockManager, + insecureDevice, keyStore, BatteryOptimizationManager(context), SyncScheduler(Provider { WorkManager.getInstance(context) }), ) + } + private fun setContent(viewModel: SettingsViewModel) { composeTestRule.setContent { LibreMailTheme(darkTheme = false, dynamicColor = false) { SettingsScreen( @@ -70,6 +73,13 @@ class SettingsScreenTest { ) } } + } + + @Test + fun selectingFetchPolicy_persistsThroughTheRepository() { + val settingsRepository = SettingsRepository(context) + runBlocking { settingsRepository.setFetchPolicy(FetchPolicy.ALWAYS) } // known starting state + setContent(settingsViewModel(settingsRepository)) composeTestRule.onNodeWithText(string(R.string.fetch_on_demand)).performScrollTo().performClick() @@ -77,4 +87,20 @@ class SettingsScreenTest { runBlocking { settingsRepository.fetchPolicy() } == FetchPolicy.ON_DEMAND } } + + @Test + fun enablingAppLockWithoutSecureDevice_showsRejectionSnackbar() { + val settingsRepository = SettingsRepository(context) + runBlocking { settingsRepository.setAppLock(false) } // known starting state: off + setContent(settingsViewModel(settingsRepository)) + + // App-lock lives under the collapsed "Advanced" section: expand it, then toggle the switch on. + composeTestRule.onNodeWithText(string(R.string.settings_advanced)).performScrollTo().performClick() + composeTestRule.onNodeWithText(string(R.string.settings_adv_app_lock)).performScrollTo().performClick() + + val message = string(R.string.app_lock_needs_device_lock) + composeTestRule.waitUntil(5_000) { + composeTestRule.onAllNodesWithText(message).fetchSemanticsNodes().isNotEmpty() + } + } } 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 192a041..c20a8ae 100644 --- a/app/src/main/kotlin/org/libremail/data/security/AppLockManager.kt +++ b/app/src/main/kotlin/org/libremail/data/security/AppLockManager.kt @@ -3,40 +3,21 @@ package org.libremail.data.security import android.app.KeyguardManager import android.content.Context -import androidx.biometric.BiometricManager 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 -/** Whether a usable device authenticator (strong biometric or device credential) is available. */ -enum class AppLockAvailability { - /** A biometric or device credential can be presented right now. */ - AVAILABLE, - - /** Hardware exists but nothing (biometric or PIN/pattern/password) is enrolled. */ - NONE_ENROLLED, - - /** No suitable authentication hardware, or it is temporarily unavailable. */ - NO_HARDWARE, - - /** Availability could not be determined (unknown status / security update required). */ - UNAVAILABLE, -} - /** - * Queries device-lock and biometric availability for the app-lock feature. Real behavior depends on - * the device keyguard and BiometricManager, so it is validated on-device, not in JVM tests. + * Reports whether the device has a secure lock, for the app-lock feature. Real behavior depends on + * the device keyguard, so it is validated on-device, not in JVM tests. */ interface AppLockManager { /** True when the device has a secure lock screen (PIN, pattern, password, or biometric). */ fun isDeviceSecure(): Boolean - /** Whether a biometric or device-credential prompt can currently be shown. */ - fun availability(): AppLockAvailability - companion object { /** Accept a strong biometric OR the device credential (PIN/pattern/password) as fallback. */ const val AUTHENTICATORS: Int = BIOMETRIC_STRONG or DEVICE_CREDENTIAL @@ -50,14 +31,4 @@ class AndroidAppLockManager @Inject constructor(@ApplicationContext private val get() = context.getSystemService(Context.KEYGUARD_SERVICE) as? KeyguardManager override fun isDeviceSecure(): Boolean = keyguardManager?.isDeviceSecure == true - - override fun availability(): AppLockAvailability = - when (BiometricManager.from(context).canAuthenticate(AppLockManager.AUTHENTICATORS)) { - BiometricManager.BIOMETRIC_SUCCESS -> AppLockAvailability.AVAILABLE - BiometricManager.BIOMETRIC_ERROR_NONE_ENROLLED -> AppLockAvailability.NONE_ENROLLED - BiometricManager.BIOMETRIC_ERROR_NO_HARDWARE, - BiometricManager.BIOMETRIC_ERROR_HW_UNAVAILABLE, - -> AppLockAvailability.NO_HARDWARE - else -> AppLockAvailability.UNAVAILABLE - } } diff --git a/app/src/main/kotlin/org/libremail/ui/lock/AppLockGateHost.kt b/app/src/main/kotlin/org/libremail/ui/lock/AppLockGateHost.kt index 5fbe375..0038671 100644 --- a/app/src/main/kotlin/org/libremail/ui/lock/AppLockGateHost.kt +++ b/app/src/main/kotlin/org/libremail/ui/lock/AppLockGateHost.kt @@ -1,13 +1,13 @@ // SPDX-License-Identifier: GPL-3.0-or-later package org.libremail.ui.lock +import androidx.activity.compose.LocalActivity import androidx.biometric.BiometricPrompt import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Surface import androidx.compose.runtime.Composable -import androidx.compose.runtime.DisposableEffect import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf @@ -15,14 +15,12 @@ import androidx.compose.runtime.remember import androidx.compose.runtime.setValue import androidx.compose.ui.Modifier import androidx.compose.ui.input.pointer.pointerInput -import androidx.compose.ui.platform.LocalContext import androidx.compose.ui.res.stringResource import androidx.core.content.ContextCompat import androidx.fragment.app.FragmentActivity import androidx.hilt.navigation.compose.hiltViewModel import androidx.lifecycle.Lifecycle -import androidx.lifecycle.LifecycleEventObserver -import androidx.lifecycle.compose.LocalLifecycleOwner +import androidx.lifecycle.compose.LifecycleEventEffect import androidx.lifecycle.compose.collectAsStateWithLifecycle import org.libremail.R import org.libremail.data.security.AppLockManager @@ -38,35 +36,29 @@ import org.libremail.data.security.AppLockManager @Composable fun AppLockGateHost(viewModel: AppLockViewModel = hiltViewModel(), content: @Composable () -> Unit) { val uiState by viewModel.uiState.collectAsStateWithLifecycle() - val lifecycleOwner = LocalLifecycleOwner.current - val context = LocalContext.current - val activity = context.findFragmentActivity() - DisposableEffect(lifecycleOwner) { - val observer = LifecycleEventObserver { _, event -> - when (event) { - Lifecycle.Event.ON_START -> viewModel.onForeground() - Lifecycle.Event.ON_STOP -> viewModel.onBackground() - else -> Unit - } - } - lifecycleOwner.lifecycle.addObserver(observer) - onDispose { lifecycleOwner.lifecycle.removeObserver(observer) } - } + LifecycleEventEffect(Lifecycle.Event.ON_START) { viewModel.onForeground() } + LifecycleEventEffect(Lifecycle.Event.ON_STOP) { viewModel.onBackground() } + + // BiometricPrompt must be hosted by a FragmentActivity; LocalActivity is the hosting Activity. + val activity = LocalActivity.current + val fragmentActivity = remember(activity) { activity as? FragmentActivity } val promptTitle = stringResource(R.string.app_lock_prompt_title) val promptSubtitle = stringResource(R.string.app_lock_prompt_subtitle) - val authenticate: () -> Unit = authenticate@{ - val host = activity ?: run { - viewModel.onAuthError(null) - return@authenticate + val authenticate: () -> Unit = remember(fragmentActivity, viewModel, promptTitle, promptSubtitle) { + authenticate@{ + val host = fragmentActivity ?: run { + viewModel.onAuthError(null) + return@authenticate + } + host.showAppLockPrompt( + title = promptTitle, + subtitle = promptSubtitle, + onSuccess = viewModel::onAuthenticated, + onError = { message -> viewModel.onAuthError(message) }, + ) } - host.showAppLockPrompt( - title = promptTitle, - subtitle = promptSubtitle, - onSuccess = viewModel::onAuthenticated, - onError = { message -> viewModel.onAuthError(message) }, - ) } // Once unlocked, keep [content] in the composition across later re-locks so its state (navigation @@ -149,9 +141,3 @@ private fun FragmentActivity.showAppLockPrompt( .build() prompt.authenticate(info) } - -private tailrec fun android.content.Context.findFragmentActivity(): FragmentActivity? = when (this) { - is FragmentActivity -> this - is android.content.ContextWrapper -> baseContext.findFragmentActivity() - else -> null -} 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 88670c5..90129d8 100644 --- a/app/src/main/kotlin/org/libremail/ui/lock/AppLockViewModel.kt +++ b/app/src/main/kotlin/org/libremail/ui/lock/AppLockViewModel.kt @@ -97,8 +97,18 @@ class AppLockViewModel @Inject constructor( } } - private fun emitLocked(error: String? = null) { - _uiState.value = AppLockUiState.Locked(error, ++lockSeq) + /** + * Single writer for the gated UI state: derive it from [AppLockGate.state] — the source of truth + * for whether the session is locked — instead of hand-mirroring the gate at each call site. [error] + * is a human-readable reason the previous unlock attempt failed. The transient + * [AppLockUiState.Checking] cover and the "app-lock off / just disabled" unlocked states are driven + * by settings/lifecycle rather than the session gate, so their sites set [_uiState] directly. + */ + private fun publish(error: String? = null) { + _uiState.value = when (gate.state) { + LockState.UNLOCKED -> AppLockUiState.Unlocked + LockState.LOCKED -> AppLockUiState.Locked(error, ++lockSeq) + } } /** Recompute the lock state when the app comes to the foreground (lifecycle ON_START). */ @@ -136,18 +146,15 @@ class AppLockViewModel @Inject constructor( LockAction.CLEAR_AND_REQUIRE_AUTH -> clearCacheAndRestart(disableAppLock = false) LockAction.REQUIRE_AUTH -> { - val state = gate.onForeground(foregroundAt, appLockEnabled = true) - if (state == LockState.UNLOCKED) { - _uiState.value = AppLockUiState.Unlocked - } else { - // Re-lock on grace expiry (timeout). The SQLCipher passphrase is intentionally - // NOT evicted here: PassphraseSession is the only separately-held copy, but - // clearing it flips EncryptedCacheGuard to "locked" and would stall background - // sync/push while locked, and the already-open Room handle keeps the key - // resident regardless. Full eviction needs the DB close/reopen owned by #93 / - // #111 — see PassphraseSession's KDoc. - emitLocked() - } + // Advance the gate for this foreground pass, then publish its decision (UNLOCKED + // within the grace window, otherwise LOCKED). Re-locking on grace expiry + // intentionally does NOT evict the SQLCipher passphrase: PassphraseSession is the + // only separately-held copy, but clearing it flips EncryptedCacheGuard to "locked" + // and would stall background sync/push while locked, and the already-open Room handle + // keeps the key resident regardless. Full eviction needs the DB close/reopen owned by + // #93 / #111 — see PassphraseSession's KDoc. + gate.onForeground(foregroundAt, appLockEnabled = true) + publish() } } } @@ -169,7 +176,7 @@ class AppLockViewModel @Inject constructor( when (withContext(Dispatchers.Default) { unlockOrArm() }) { UnlockResult.OK -> { gate.onAuthenticated() - _uiState.value = AppLockUiState.Unlocked + publish() } UnlockResult.UNRECOVERABLE -> { @@ -181,7 +188,7 @@ class AppLockViewModel @Inject constructor( UnlockResult.RETRY -> { gate.lock() - emitLocked(context.getString(R.string.app_lock_unlock_failed)) + publish(context.getString(R.string.app_lock_unlock_failed)) } } } @@ -190,7 +197,7 @@ class AppLockViewModel @Inject constructor( /** Called by the host when the prompt is cancelled or errors. */ fun onAuthError(message: String?) { gate.lock() - emitLocked(message) + publish(message) } /** Outcome of unwrapping/arming the auth-bound passphrase after a successful device auth. */ 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 f07281d..d238929 100644 --- a/app/src/main/kotlin/org/libremail/ui/settings/SettingsScreen.kt +++ b/app/src/main/kotlin/org/libremail/ui/settings/SettingsScreen.kt @@ -1,7 +1,6 @@ // SPDX-License-Identifier: GPL-3.0-or-later package org.libremail.ui.settings -import android.widget.Toast import androidx.compose.animation.AnimatedVisibility import androidx.compose.foundation.clickable import androidx.compose.foundation.layout.Column @@ -18,11 +17,14 @@ import androidx.compose.material3.HorizontalDivider import androidx.compose.material3.Icon import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Scaffold +import androidx.compose.material3.SnackbarHost +import androidx.compose.material3.SnackbarHostState import androidx.compose.material3.Text import androidx.compose.material3.TopAppBar import androidx.compose.runtime.Composable import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.getValue +import androidx.compose.runtime.remember import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.draw.rotate @@ -55,10 +57,14 @@ fun SettingsScreen( val batteryUnrestricted by viewModel.batteryUnrestricted.collectAsStateWithLifecycle() val context = LocalContext.current val resources = LocalResources.current + val snackbarHostState = remember { SnackbarHostState() } + // 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. LaunchedEffect(appLockMessage) { - appLockMessage?.let { - Toast.makeText(context, resources.getString(it), Toast.LENGTH_LONG).show() + appLockMessage?.let { messageId -> + snackbarHostState.showSnackbar(resources.getString(messageId)) viewModel.clearAppLockMessage() } } @@ -69,6 +75,7 @@ fun SettingsScreen( Scaffold( topBar = { TopAppBar(title = { Text(stringResource(R.string.title_settings)) }) }, bottomBar = { LibreMailBottomBar(current = TopDest.SETTINGS, onSelect = onSelectTab) }, + snackbarHost = { SnackbarHost(snackbarHostState) }, ) { padding -> Column( modifier = Modifier -- 2.47.3