Merge pull request #119 from JMR-dev/fix-applock-lifecycle

fix(security): app-lock lifecycle consistency (grace across Back, passphrase eviction limits)
2026-07-02 03:50:14 -05:00
parent 611490a9f6 18f52e1cb9
commit bc1a2b9e11
6 changed files with 189 additions and 9 deletions
@@ -15,6 +15,14 @@ enum class LockState { LOCKED, UNLOCKED }
 *  - Returning to the foreground within [graceMillis] of backgrounding stays unlocked; after the
 *    grace period it re-locks. Foregrounding without a preceding background (e.g. a configuration
 *    change / rotation) does not re-lock an already-unlocked session.
+ *
+ * Lifetime: provided as an application-scoped singleton (see `SecurityModule`) so the grace window
+ * survives Activity recreation. If this state lived in the Activity-scoped [org.libremail.ui.lock
+ * .AppLockViewModel]'s own field, Back on the task root — which finishes the Activity and clears its
+ * ViewModelStore on API 29/30 — would drop the [backgroundedAt] marker and re-arm a fresh LOCKED gate,
+ * so returning within the grace period would wrongly demand re-auth (unlike leaving via Home). Being
+ * process-scoped, the same instance is reused across recreation, while a genuine cold start (process
+ * death) constructs a fresh one that correctly starts [LockState.LOCKED].
 */
 class AppLockGate(private val graceMillis: Long = DEFAULT_GRACE_MILLIS) {

@@ -12,12 +12,25 @@ import javax.inject.Singleton
 *
 * When app-lock is enabled the database passphrase is sealed by an auth-bound Keystore key
 * ([DatabaseKeyCipher]) and is only recoverable after the user passes a `BiometricPrompt`. The
- * unwrapped passphrase is kept here for the lifetime of the unlocked session — never persisted — and
- * cleared on lock, timeout, or when app-lock is disabled. `DatabaseModule` reads it through [await]
- * so the encrypted database is opened only after authentication.
+ * unwrapped passphrase is kept here for the unlocked session — never persisted. `DatabaseModule`
+ * reads it through [await] so the encrypted database is opened only after authentication.
 *
- * The value is an immutable [String] to stay consistent with the rest of the passphrase plumbing; it
- * cannot be zeroed in place, which is an accepted limitation of the existing design.
+ * Eviction — current limitation: the passphrase is NOT guaranteed to leave the process when the app
+ * re-locks. [lock] is called on app-lock disable and cache reset ([DatabaseKeyStore]), but it is
+ * deliberately NOT called on the app-lock UI gate's grace-expiry/timeout re-lock, and even if it were
+ * it would only drop THIS holder's reference:
+ *  - the value is an immutable [String] and cannot be zeroed in place — [lock] merely releases it
+ *    for GC;
+ *  - once the encrypted cache has been opened, SQLCipher keeps the key inside the already-open
+ *    Room/native handle. `DatabaseModule.provideDatabase` runs once per process, so nothing here
+ *    closes that handle; the key stays resident for the process lifetime regardless of [lock];
+ *  - this holder's unlocked-ness also drives [EncryptedCacheGuard], so clearing it while the app is
+ *    merely locked (not exited) would stall background sync/push even though the DB is still open.
+ *
+ * Truly evicting the passphrase on lock/timeout therefore requires a database close/reopen mechanism,
+ * which is owned by the DB-lifecycle work in issues #93 (provideDatabase) and #111 (DB
+ * re-architecture) and is intentionally out of scope here. Until those land, do not rely on [lock]
+ * for cryptographic erasure of the passphrase from the process.
 */
@Singleton
 class PassphraseSession @Inject constructor() {
@@ -29,7 +42,11 @@ class PassphraseSession @Inject constructor() {
        passphrase.value = value
    }

-    /** Clear the passphrase from memory (on lock, timeout, or app-lock disable). */
+    /**
+     * Drop this holder's reference to the passphrase (on app-lock disable / cache reset). See the
+     * class KDoc: this is a partial eviction only — it does not close the open database, so SQLCipher
+     * keeps the key resident for the process lifetime (full eviction is deferred to #93 / #111).
+     */
    fun lock() {
        passphrase.value = null
    }
@@ -3,9 +3,11 @@ package org.libremail.di

 import dagger.Binds
 import dagger.Module
+import dagger.Provides
 import dagger.hilt.InstallIn
 import dagger.hilt.components.SingletonComponent
 import org.libremail.data.security.AndroidAppLockManager
+import org.libremail.data.security.AppLockGate
 import org.libremail.data.security.AppLockManager
 import javax.inject.Singleton

@@ -16,4 +18,16 @@ abstract class SecurityModule {
    @Binds
    @Singleton
    abstract fun bindAppLockManager(impl: AndroidAppLockManager): AppLockManager
+
+    companion object {
+        /**
+         * The app-lock UI gate is application-scoped, NOT Activity/ViewModel-scoped, so its grace
+         * window survives Activity recreation (e.g. Back finishing the task root on API 29/30, which
+         * clears the Activity's ViewModelStore). See [AppLockGate] for why this must outlive the
+         * ViewModel. A fresh process constructs a new instance that correctly starts LOCKED.
+         */
+        @Provides
+        @Singleton
+        fun provideAppLockGate(): AppLockGate = AppLockGate()
+    }
 }
@@ -73,10 +73,13 @@ class AppLockViewModel @Inject constructor(
    private val databaseKeyCipher: DatabaseKeyCipher,
    private val session: PassphraseSession,
    private val syncScheduler: SyncScheduler,
+    // 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
+    // otherwise drop the grace marker and force a full re-auth on return within the grace period.
+    private val gate: AppLockGate,
 ) : ViewModel() {

-    private val gate = AppLockGate()
-
    private val _uiState = MutableStateFlow<AppLockUiState>(AppLockUiState.Checking)
    val uiState: StateFlow<AppLockUiState> = _uiState.asStateFlow()

@@ -134,7 +137,17 @@ class AppLockViewModel @Inject constructor(

                LockAction.REQUIRE_AUTH -> {
                    val state = gate.onForeground(foregroundAt, appLockEnabled = true)
-                    if (state == LockState.UNLOCKED) _uiState.value = AppLockUiState.Unlocked else emitLocked()
+                    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()
+                    }
                }
            }
        }
@@ -100,4 +100,45 @@ class AppLockGateTest {
        gate.onForeground(now = 1_000L, appLockEnabled = true) // stale -> LOCKED, marker retained
        assertEquals(LockState.LOCKED, gate.onForeground(now = 3_000L, appLockEnabled = true))
    }
+
+    @Test
+    fun `grace survives activity recreation when the gate instance is reused (app-scoped singleton)`() {
+        // #101: Back on the task root (API 29/30) finishes the Activity and clears its ViewModelStore,
+        // so AppLockViewModel is destroyed and re-created. Because the gate is application-scoped the
+        // SAME instance is reused across that recreation: the background marker recorded before the
+        // store was cleared is still present, so returning within grace stays unlocked — identical to
+        // leaving via Home and returning.
+        val survivingGate = AppLockGate(grace)
+        survivingGate.onForeground(now = 0, appLockEnabled = true)
+        survivingGate.onAuthenticated()
+        survivingGate.onBackground(now = 1_000) // ON_STOP as Back finishes the Activity
+        // ...Activity destroyed + re-created; the same singleton gate is handed to the new ViewModel...
+        assertEquals(
+            LockState.UNLOCKED,
+            survivingGate.onForeground(now = 1_000 + grace, appLockEnabled = true),
+        )
+    }
+
+    @Test
+    fun `grace still expires across activity recreation when the reused gate is out of the window`() {
+        val survivingGate = AppLockGate(grace)
+        survivingGate.onForeground(now = 0, appLockEnabled = true)
+        survivingGate.onAuthenticated()
+        survivingGate.onBackground(now = 1_000)
+        assertEquals(
+            LockState.LOCKED,
+            survivingGate.onForeground(now = 1_000 + grace + 1, appLockEnabled = true),
+        )
+    }
+
+    @Test
+    fun `a fresh gate (process death, or the pre-fix Activity-scoped bug) starts locked`() {
+        // Contrast with the reused instance above: if the gate were Activity/ViewModel-scoped (the
+        // pre-fix bug) or after a genuine cold start (process death), recreation builds a FRESH gate.
+        // It must start LOCKED and stay locked on the first foreground even within the grace window —
+        // the re-auth-on-Back inconsistency #101 fixes, and the correct cold-start behavior.
+        val freshGate = AppLockGate(grace)
+        assertEquals(LockState.LOCKED, freshGate.state)
+        assertEquals(LockState.LOCKED, freshGate.onForeground(now = 1_000, appLockEnabled = true))
+    }
 }
@@ -0,0 +1,87 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+package org.libremail.ui.lock
+
+import android.os.SystemClock
+import io.mockk.every
+import io.mockk.mockk
+import io.mockk.mockkStatic
+import io.mockk.unmockkAll
+import io.mockk.verify
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.ExperimentalCoroutinesApi
+import kotlinx.coroutines.flow.flowOf
+import kotlinx.coroutines.test.UnconfinedTestDispatcher
+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.data.security.AppLockGate
+import org.libremail.data.settings.AppSettings
+import org.libremail.data.settings.SettingsRepository
+import kotlin.test.assertEquals
+import kotlin.test.assertIs
+
+/**
+ * Wiring guard for #101: the app-lock gate must be an INJECTED, application-scoped dependency so its
+ * 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.
+ */
+@OptIn(ExperimentalCoroutinesApi::class)
+class AppLockViewModelTest {
+
+    private val dispatcher = UnconfinedTestDispatcher()
+
+    @Before
+    fun setUp() = Dispatchers.setMain(dispatcher)
+
+    @After
+    fun tearDown() {
+        Dispatchers.resetMain()
+        unmockkAll()
+    }
+
+    private fun viewModel(gate: AppLockGate, appLock: Boolean = true): AppLockViewModel {
+        val settings = mockk<SettingsRepository>()
+        every { settings.settings } returns flowOf(AppSettings(appLock = appLock))
+        return AppLockViewModel(
+            context = mockk(relaxed = true),
+            settingsRepository = settings,
+            appLockManager = mockk(relaxed = true),
+            databaseKeyStore = mockk(relaxed = true),
+            databaseKeyCipher = mockk(relaxed = true),
+            session = mockk(relaxed = true),
+            syncScheduler = mockk(relaxed = true),
+            gate = gate,
+        )
+    }
+
+    @Test
+    fun `onBackground records the background on the injected gate`() = runTest(dispatcher) {
+        mockkStatic(SystemClock::class)
+        every { SystemClock.elapsedRealtime() } returns 5_000L
+        val gate = mockk<AppLockGate>(relaxed = true)
+
+        viewModel(gate).onBackground()
+
+        // Delegating to the gate passed into the constructor (an injected, shareable singleton) rather
+        // than an internally-constructed one is the whole point of the #101 hoist: the same instance
+        // must survive Activity recreation so the grace marker persists.
+        verify { gate.onBackground(5_000L) }
+    }
+
+    @Test
+    fun `onAuthError re-locks through the injected gate and surfaces the error`() = runTest(dispatcher) {
+        val gate = mockk<AppLockGate>(relaxed = true)
+        val vm = viewModel(gate)
+
+        vm.onAuthError("boom")
+
+        verify { gate.lock() }
+        val state = assertIs<AppLockUiState.Locked>(vm.uiState.value)
+        assertEquals("boom", state.error)
+    }
+}