fix: make changing the PIN require knowing it

Change PIN asked for the current one, and then moved to "Choose a PIN"
without waiting for the answer. The check is asynchronous; the screen
reassigned its step outside the result and passed an empty callback. Any
four digits reached the replacement screen, and setPin enrolls without
verifying anything.

So anyone holding the phone while it was unlocked could change the app's
PIN. Under the no-recovery policy the owner's only way back into her own
history is to erase all of it. The file's own KDoc says this must not be
possible.

The step now advances from inside the verified callback, as "turn the
lock off" already did -- and the ViewModel refuses a replacement that no
successful check authorised. Two guards for one rule on purpose: a screen
is the kind of file that gets rewritten by somebody who has not read the
one behind it, and the place that writes the PIN is the place that has to
refuse. Cancelling withdraws the permission; a successful write spends it.

"Turn the lock off" had the same advance-before-answer shape. It was safe
-- the work was already inside the callback -- but its wrong-PIN message
landed on a screen that had gone, so ConfirmPin.wrong was dead code. Fixed
symmetrically.

Also fixes the lock-out race in the same function (#62). setPin wrote the
PIN and then unlocked the session; the gate is combine(hasPin, unlocked)
and closes on (true, false), so DataStore's emission could arrive in
between. AppLockGate disposes the whole app subtree when it closes, this
ViewModel is scoped to a destination inside it, and the unlock was
cancelled with the scope -- the user was thrown to the lock screen to type
the PIN she had chosen a second earlier. The development log records the
common case as fixed; the fix lived in the scope the race destroyed.

Unlocking first makes the bad pair unobservable: unlock() sets a
MutableStateFlow synchronously on this thread, before the write begins,
and combine always emits with the latest of both. If the write fails there
is no PIN and Unlocked is correct anyway.

The settings ViewModel had no test at all, which is how a wrong PIN
reaching the replacement screen went unnoticed. It has five now, against
the real repository over a host-JVM signing key -- core/security gains a
small public two-argument constructor for that, since AndroidKeyStore
cannot be reached off-device and faking the repository would prove nothing
about it.

Proved with scripts/prove-guard.sh, one red each: spending the
authorisation, and the old write-then-unlock order. Removing the
write-site guard entirely reddens three, which is that guard's whole
surface rather than a coincidence.

closes #60
closes #62

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

app/src/main/kotlin/dev/privacyllc/period/feature/lock/LockSettingsScreen.kt

@@ -69,27 +69,33 @@ fun LockSettingsScreen(viewModel: LockSettingsViewModel = hiltViewModel()) {
         Mode.SET_FIRST, Mode.SET_REPLACEMENT -> PinSetupScreen(
             busy = state.busy,
             failed = state.message == LockSettings.Message.COULD_NOT_SET,
-            onCancel = { mode = Mode.OVERVIEW; viewModel.clearMessage() },
+            onCancel = { mode = Mode.OVERVIEW; viewModel.cancelChange(); viewModel.clearMessage() },
             onConfirmed = { pin -> viewModel.setPin(pin); mode = Mode.OVERVIEW },
         )
 
+        // Both confirmations stay on screen until the PIN is actually right.
+        //
+        // They used to move on in the same breath as asking — the check is
+        // asynchronous and the mode was reassigned outside its result — so the
+        // wrong-PIN message arrived after the screen that would have shown it
+        // had gone, and in the change case any four digits reached "choose a
+        // PIN". Advancing from inside the callback is the whole fix here; the
+        // ViewModel refuses an unauthorised replacement as well, because this
+        // file is exactly the kind that gets rewritten.
         Mode.CONFIRM_TO_REMOVE -> ConfirmPin(
             title = "Enter your PIN to turn the lock off",
             busy = state.busy,
             wrong = state.message == LockSettings.Message.WRONG_PIN,
             onCancel = { mode = Mode.OVERVIEW; viewModel.clearMessage() },
-            onSubmit = { pin -> viewModel.removePin(pin); mode = Mode.OVERVIEW },
+            onSubmit = { pin -> viewModel.removePin(pin) { mode = Mode.OVERVIEW } },
         )
 
         Mode.CONFIRM_TO_CHANGE -> ConfirmPin(
             title = "Enter your current PIN",
             busy = state.busy,
             wrong = state.message == LockSettings.Message.WRONG_PIN,
-            onCancel = { mode = Mode.OVERVIEW; viewModel.clearMessage() },
+            onCancel = { mode = Mode.OVERVIEW; viewModel.cancelChange(); viewModel.clearMessage() },
-            onSubmit = { pin ->
+            onSubmit = { pin -> viewModel.authoriseChange(pin) { mode = Mode.SET_REPLACEMENT } },
-                viewModel.verifyCurrent(pin) { }
-                mode = Mode.SET_REPLACEMENT
-            },
         )
     }
 }

app/src/main/kotlin/dev/privacyllc/period/feature/lock/LockSettingsViewModel.kt

@@ -70,19 +70,56 @@ class LockSettingsViewModel @Inject constructor(
         _message.value = LockSettings.Message.COULD_NOT_SET
     }
 
-    /** First PIN, or a replacement once [verifyCurrent] has passed. */
+    /**
+     * True between a successful check of the current PIN and the replacement
+     * being written or abandoned.
+     *
+     * Not a detail of the screen. The screen is *also* supposed to enforce the
+     * order, and it did not: it fired the check and advanced to "choose a PIN"
+     * in the same breath, without waiting for the answer, so any four digits
+     * reached the replacement screen. The place that writes the PIN is the place
+     * that has to refuse — a screen can be rewritten by somebody who never reads
+     * this file.
+     */
+    private var replacementAuthorised = false
+
+    /** First PIN, or a replacement once [authoriseChange] has passed. */
     fun setPin(pin: CharArray) {
         if (_busy.value) return
         _busy.value = true
         viewModelScope.launch(handler) {
+            // A replacement needs the current PIN first; a first PIN has nothing
+            // to check against. Refusing here rather than trusting the caller is
+            // what makes "you cannot change the PIN without knowing it" a
+            // property of the lock instead of a property of one composable.
+            if (lock.hasPinNow() && !replacementAuthorised) {
+                pin.fill('\u0000')
+                _busy.value = false
+                _message.value = LockSettings.Message.WRONG_PIN
+                return@launch
+            }
+
+            // Open THIS session before the PIN exists, not after.
+            //
+            // The gate is combine(hasPin, unlocked): it closes only on the pair
+            // (true, false). `unlock()` sets a MutableStateFlow synchronously on
+            // this thread, before setPin begins the write that makes hasPin
+            // true, and combine always emits with the latest of both — so that
+            // pair is never observable. The old order wrote first and unlocked
+            // second, and DataStore's emission could reach the gate in between:
+            // AppLockGate disposes the whole app subtree when it closes, this
+            // ViewModel is scoped to a destination inside it, and the unlock was
+            // cancelled with the scope. The user was thrown to the lock screen
+            // to type the PIN she had just chosen.
+            //
+            // If setPin fails there is no PIN, and Unlocked is the right state
+            // anyway — so nothing is undone here. Never call controller.lock()
+            // on failure: during a change that strands the user behind her old
+            // PIN on a screen that has just told her the write did not happen.
+            controller.unlock()
             val ok = lock.setPin(pin)
             pin.fill('\u0000')
-            // Stay unlocked in the session that just set the PIN. Without this
+            replacementAuthorised = false
-            // the gate flips shut the instant `hasPin` turns true, and the user
-            // is asked for a PIN one second after choosing it — which reads as
-            // the app not having understood, and is the first thing they see of
-            // a feature they have just been warned is unrecoverable.
-            if (ok) controller.unlock()
             _busy.value = false
             _message.value =
                 if (ok) LockSettings.Message.PIN_SET else LockSettings.Message.COULD_NOT_SET
@@ -103,6 +140,9 @@ class LockSettingsViewModel @Inject constructor(
             val outcome = lock.check(pin)
             pin.fill('\u0000')
             if (outcome is UnlockResult.Unlocked) {
+                // Clear a stale "that is not the PIN" before moving on, or the
+                // next screen opens still showing the last wrong answer.
+                _message.value = null
                 onVerified()
                 _busy.value = false
             } else {
@@ -112,10 +152,30 @@ class LockSettingsViewModel @Inject constructor(
         }
     }
 
-    fun removePin(current: CharArray) = verifyCurrent(current) {
+    /**
+     * The first step of changing the PIN: prove the current one, and only then
+     * let a replacement be written.
+     *
+     * [onAuthorised] runs after the check passes, so a caller can move to the
+     * "choose a PIN" step from inside it. It is deliberately not the caller's
+     * job to know that — [setPin] refuses a replacement that was not authorised
+     * here, whatever the screen does.
+     */
+    fun authoriseChange(current: CharArray, onAuthorised: () -> Unit) = verifyCurrent(current) {
+        replacementAuthorised = true
+        onAuthorised()
+    }
+
+    /** Abandoning the change withdraws the permission it was granted. */
+    fun cancelChange() {
+        replacementAuthorised = false
+    }
+
+    fun removePin(current: CharArray, onRemoved: () -> Unit = {}) = verifyCurrent(current) {
         lock.clearLock()
         preferences.setBiometricLockEnabled(false)
         _message.value = LockSettings.Message.PIN_REMOVED
+        onRemoved()
     }
 
     fun setBiometricEnabled(enabled: Boolean) {

app/src/main/kotlin/dev/privacyllc/period/lock/AppLockViewModel.kt

@@ -82,16 +82,9 @@ class AppLockViewModel @Inject constructor(
      * The initial value must not be `Unlocked` — that would flash the Today
      * screen, with a forecast on it, before the lock had a chance to close.
      */
-    val state: StateFlow<LockState> = combine(
+    val state: StateFlow<LockState> =
-        lock.hasPin,
+        combine(lock.hasPin, controller.unlocked, ::lockStateOf)
-        controller.unlocked,
+            .stateIn(viewModelScope, SharingStarted.Eagerly, LockState.Unknown)
-    ) { hasPin, unlocked ->
-        when {
-            !hasPin -> LockState.Unlocked
-            unlocked -> LockState.Unlocked
-            else -> LockState.Locked
-        }
-    }.stateIn(viewModelScope, SharingStarted.Eagerly, LockState.Unknown)
 
     /** True only when a fingerprint may stand in for the PIN, which needs a PIN to stand in for. */
     val biometricAllowed: StateFlow<Boolean> = combine(
@@ -178,3 +171,19 @@ class AppLockViewModel @Inject constructor(
         }
     }
 }
+
+/**
+ * The whole rule, in one place: locked exactly when a PIN exists and this
+ * session has not opened it.
+ *
+ * Extracted from the gate's `combine` so a test can assert against the rule the
+ * app actually uses rather than a restatement of it. That matters here more than
+ * usual — the thing worth proving about setting a PIN is that this function
+ * never returns [LockState.Locked] while it happens, and a copy of the rule in
+ * the test would prove that about the copy.
+ */
+internal fun lockStateOf(hasPin: Boolean, unlocked: Boolean): LockState = when {
+    !hasPin -> LockState.Unlocked
+    unlocked -> LockState.Unlocked
+    else -> LockState.Locked
+}

app/src/test/kotlin/dev/privacyllc/period/feature/lock/LockSettingsViewModelTest.kt

@@ -0,0 +1,221 @@
+package dev.privacyllc.period.feature.lock
+
+import androidx.datastore.preferences.core.PreferenceDataStoreFactory
+import dev.privacyllc.period.core.datastore.UserPreferencesRepository
+import dev.privacyllc.period.core.security.AppLockRepository
+import dev.privacyllc.period.core.security.MacProvider
+import dev.privacyllc.period.core.security.UnlockResult
+import dev.privacyllc.period.lock.AppLockController
+import dev.privacyllc.period.lock.LockState
+import dev.privacyllc.period.lock.lockStateOf
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.delay
+import kotlinx.coroutines.flow.combine
+import kotlinx.coroutines.flow.first
+import kotlinx.coroutines.Job
+import kotlinx.coroutines.launch
+import kotlinx.coroutines.runBlocking
+import kotlinx.coroutines.test.UnconfinedTestDispatcher
+import kotlinx.coroutines.test.resetMain
+import kotlinx.coroutines.test.setMain
+import kotlinx.coroutines.withTimeout
+import org.junit.After
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertTrue
+import org.junit.Before
+import org.junit.Rule
+import org.junit.Test
+import org.junit.rules.TemporaryFolder
+import org.junit.runner.RunWith
+import org.robolectric.RobolectricTestRunner
+import org.robolectric.annotation.Config
+import javax.crypto.Mac
+import javax.crypto.spec.SecretKeySpec
+
+/**
+ * The settings side of the app lock, which had no test and two defects.
+ *
+ * One was a security hole: "Change PIN" fired the check of the current PIN and
+ * moved to "choose a PIN" in the same breath, without waiting for the answer, so
+ * any four digits reached the replacement screen and overwrote the PIN. Under
+ * this project's no-recovery policy that costs the owner her entire history.
+ *
+ * The other was a race: the PIN was written first and the session unlocked
+ * second, and the gate could shut in between — disposing the subtree this very
+ * ViewModel lives in, and cancelling the unlock with it. The user was thrown to
+ * the lock screen to type the PIN she had chosen a second earlier.
+ *
+ * Both are asserted here against the real repository over a host-JVM signing
+ * key, not a fake of the thing under test.
+ */
+@RunWith(RobolectricTestRunner::class)
+@Config(sdk = [34])
+class LockSettingsViewModelTest {
+
+    @get:Rule val temp = TemporaryFolder()
+
+    private val dispatcher = UnconfinedTestDispatcher()
+    private lateinit var lock: AppLockRepository
+    private lateinit var controller: AppLockController
+    private lateinit var vm: LockSettingsViewModel
+    private lateinit var subscription: Job
+
+    /**
+     * A signing key that exists off-device.
+     *
+     * `AndroidKeyStore` cannot be reached on the host JVM and Robolectric ships
+     * no crypto shadows, so this is the seam `AppLockRepository`'s two-argument
+     * constructor exists for. Deliberately a duplicate of core/security's own
+     * fake rather than a shared fixture: fifteen lines, against a Gradle
+     * test-fixtures setup whose `internal` visibility rules are their own risk.
+     */
+    private class InMemoryMacProvider : MacProvider {
+        private var key: ByteArray? = null
+        override fun hasKey() = key != null
+        override fun ensureKey() { if (key == null) key = ByteArray(32) { it.toByte() } }
+        override fun mac(data: ByteArray): ByteArray {
+            val k = key ?: error("the verifier key is absent")
+            return Mac.getInstance("HmacSHA256").apply { init(SecretKeySpec(k, "HmacSHA256")) }.doFinal(data)
+        }
+        override fun deleteKey() { key = null }
+    }
+
+    @Before fun setUp() {
+        Dispatchers.setMain(dispatcher)
+        // The stores get a real background dispatcher, not the test one: the
+        // assertions below block the test thread while they wait, and a
+        // DataStore whose own scope is that thread can never finish the write
+        // they are waiting for. Main stays the test dispatcher so viewModelScope
+        // still runs eagerly.
+        val scope = CoroutineScope(Dispatchers.IO)
+        lock = AppLockRepository(
+            PreferenceDataStoreFactory.create(scope = scope) { temp.newFile("app_lock.preferences_pb") },
+            InMemoryMacProvider(),
+        )
+        val prefs = UserPreferencesRepository(
+            PreferenceDataStoreFactory.create(scope = scope) { temp.newFile("prefs.preferences_pb") },
+        )
+        controller = AppLockController()
+        vm = LockSettingsViewModel(lock, prefs, controller)
+
+        // `state` is stateIn(WhileSubscribed): with nobody collecting, the
+        // upstream never runs and `.value` sits on its initial value forever.
+        // The screen is that collector in the app; here it is this.
+        subscription = CoroutineScope(Dispatchers.IO).launch { vm.state.collect {} }
+        await { vm.state.value == LockSettings() }
+    }
+
+    @After fun tearDown() {
+        subscription.cancel()
+        Dispatchers.resetMain()
+    }
+
+    private fun await(predicate: suspend () -> Boolean) = runBlocking {
+        withTimeout(5_000) { while (!predicate()) delay(10) }
+    }
+
+    private fun pin(value: String) = value.toCharArray()
+
+    // -----------------------------------------------------------------------
+    // The race
+    // -----------------------------------------------------------------------
+
+    @Test
+    fun `setting the first PIN never closes the gate`() {
+        val seen = mutableListOf<LockState>()
+        val watching = CoroutineScope(dispatcher).launch {
+            combine(lock.hasPin, controller.unlocked, ::lockStateOf).collect { seen += it }
+        }
+
+        vm.setPin(pin("2468"))
+        await { vm.state.value.message == LockSettings.Message.PIN_SET }
+        watching.cancel()
+
+        // Not "ends unlocked" — never locked, at any instant. The gate disposes
+        // the whole app subtree when it closes, so a single frame of Locked is
+        // the user back at the lock screen typing the PIN she just chose.
+        assertFalse("the gate closed while the PIN was being set: $seen", LockState.Locked in seen)
+        assertTrue(runBlocking { lock.hasPin.first() })
+        assertTrue(controller.unlocked.value)
+    }
+
+    // -----------------------------------------------------------------------
+    // The bypass
+    // -----------------------------------------------------------------------
+
+    @Test
+    fun `changing the PIN needs the current one and cannot be skipped`() {
+        vm.setPin(pin("2468"))
+        await { lock.hasPin.first() }
+
+        var authorised = false
+        vm.authoriseChange(pin("1111")) { authorised = true }
+        await { vm.state.value.message == LockSettings.Message.WRONG_PIN }
+        assertFalse("a wrong current PIN authorised a replacement", authorised)
+
+        // The screen is not the only guard: even a caller that ignored the
+        // callback and asked for the write anyway must be refused.
+        vm.setPin(pin("1357"))
+        await { !vm.state.value.busy }
+
+        // The old PIN still opens the app; the attempted replacement does not.
+        assertTrue(runBlocking { lock.check(pin("2468")) } is UnlockResult.Unlocked)
+        assertTrue(runBlocking { lock.check(pin("1357")) } is UnlockResult.Wrong)
+    }
+
+    @Test
+    fun `the right current PIN authorises exactly one replacement`() {
+        vm.setPin(pin("2468"))
+        await { lock.hasPin.first() }
+
+        var reached = false
+        vm.authoriseChange(pin("2468")) { reached = true }
+        await { reached }
+
+        vm.setPin(pin("1357"))
+        await { vm.state.value.message == LockSettings.Message.PIN_SET }
+        assertTrue(runBlocking { lock.check(pin("1357")) } is UnlockResult.Unlocked)
+
+        // Permission is spent. A second write without asking again is refused.
+        vm.setPin(pin("9999"))
+        await { vm.state.value.message == LockSettings.Message.WRONG_PIN }
+        assertTrue(runBlocking { lock.check(pin("1357")) } is UnlockResult.Unlocked)
+    }
+
+    @Test
+    fun `abandoning a change withdraws the permission it was granted`() {
+        vm.setPin(pin("2468"))
+        await { lock.hasPin.first() }
+
+        vm.authoriseChange(pin("2468")) { }
+        await { !vm.state.value.busy }
+        vm.cancelChange()
+
+        vm.setPin(pin("1357"))
+        await { vm.state.value.message == LockSettings.Message.WRONG_PIN }
+        assertTrue(runBlocking { lock.check(pin("2468")) } is UnlockResult.Unlocked)
+    }
+
+    // -----------------------------------------------------------------------
+    // Turning it off
+    // -----------------------------------------------------------------------
+
+    @Test
+    fun `the lock comes off only for the right PIN`() {
+        vm.setPin(pin("2468"))
+        await { lock.hasPin.first() }
+
+        var removed = false
+        vm.removePin(pin("1111")) { removed = true }
+        await { vm.state.value.message == LockSettings.Message.WRONG_PIN }
+        assertFalse(removed)
+        assertTrue(runBlocking { lock.hasPin.first() })
+
+        vm.removePin(pin("2468")) { removed = true }
+        await { removed }
+        assertFalse(runBlocking { lock.hasPin.first() })
+        assertEquals(LockSettings.Message.PIN_REMOVED, vm.state.value.message)
+    }
+}

core/security/src/main/kotlin/dev/privacyllc/period/core/security/AppLockRepository.kt

@@ -75,6 +75,21 @@ class AppLockRepository internal constructor(
     constructor(store: DataStore<Preferences>) :
         this(store, AndroidKeyStoreMacProvider(), SystemClocks)
 
+    /**
+     * The real repository over a signing key that exists on the host JVM.
+     *
+     * For tests **above** this module. `AndroidKeyStore` cannot be reached
+     * without a device, so an `app`-module test of the settings flow — the one
+     * that proves a PIN cannot be changed without knowing it, and that setting
+     * one does not lock you out of the session — would otherwise have to fake
+     * the whole repository and prove nothing about it.
+     *
+     * [Clocks] stays internal: the backoff timing is this module's own concern
+     * and is tested here, against a fake clock that can be moved and rebooted.
+     */
+    constructor(store: DataStore<Preferences>, macs: MacProvider) :
+        this(store, macs, SystemClocks)
+
     private val verifier: PinVerifier = PinVerifier(macs)
 
     /** True once a PIN exists. Derived, never stored. */

docs/design/README.md

@@ -153,6 +153,26 @@ have replaced it.
 
 ## The lock screen is the one semi-public surface
 
+### A step that asks a question waits for the answer
+
+Both confirmations in App lock — "turn the lock off" and "change PIN" — used to
+move on in the same breath as asking. The check of the current PIN is
+asynchronous, and the screen reassigned its step outside the result, so the
+"that is not the PIN" message arrived after the screen that would have shown it
+had already gone. In the change case it was worse than confusing: any four
+digits reached "Choose a PIN", and the replacement was written. The lock stopped
+protecting the thing it exists to protect, which is somebody holding the phone
+while it is unlocked.
+
+A step now advances from inside the verified callback, and the ViewModel refuses
+a replacement that no successful check authorised. Two guards for one rule, on
+purpose: a screen is the kind of file that gets rewritten by somebody who has not
+read the one behind it, and this is not a rule to leave in a composable's hands.
+
+The same shape applies anywhere a screen has steps: the arrow, the system
+gesture and the on-screen Cancel all mean the same thing at the same step, and
+none of them may skip a question that has not been answered.
+
 ### A button says what it does, and it does what it says
 
 A notification button is chosen twice: once as words on a lock screen, and once