diff --git a/app/src/main/kotlin/dev/privacyllc/period/feature/lock/LockSettingsScreen.kt b/app/src/main/kotlin/dev/privacyllc/period/feature/lock/LockSettingsScreen.kt index e8b117a..27e74da 100644 --- a/app/src/main/kotlin/dev/privacyllc/period/feature/lock/LockSettingsScreen.kt +++ b/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() }, - onSubmit = { pin -> - viewModel.verifyCurrent(pin) { } - mode = Mode.SET_REPLACEMENT - }, + onCancel = { mode = Mode.OVERVIEW; viewModel.cancelChange(); viewModel.clearMessage() }, + onSubmit = { pin -> viewModel.authoriseChange(pin) { mode = Mode.SET_REPLACEMENT } }, ) } } diff --git a/app/src/main/kotlin/dev/privacyllc/period/feature/lock/LockSettingsViewModel.kt b/app/src/main/kotlin/dev/privacyllc/period/feature/lock/LockSettingsViewModel.kt index 750f6e2..0b81501 100644 --- a/app/src/main/kotlin/dev/privacyllc/period/feature/lock/LockSettingsViewModel.kt +++ b/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 - // 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() + replacementAuthorised = false _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) { diff --git a/app/src/main/kotlin/dev/privacyllc/period/lock/AppLockViewModel.kt b/app/src/main/kotlin/dev/privacyllc/period/lock/AppLockViewModel.kt index f353a78..7cb26c8 100644 --- a/app/src/main/kotlin/dev/privacyllc/period/lock/AppLockViewModel.kt +++ b/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 = combine( - lock.hasPin, - controller.unlocked, - ) { hasPin, unlocked -> - when { - !hasPin -> LockState.Unlocked - unlocked -> LockState.Unlocked - else -> LockState.Locked - } - }.stateIn(viewModelScope, SharingStarted.Eagerly, LockState.Unknown) + val state: StateFlow = + combine(lock.hasPin, controller.unlocked, ::lockStateOf) + .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 = 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 +} diff --git a/app/src/test/kotlin/dev/privacyllc/period/feature/lock/LockSettingsViewModelTest.kt b/app/src/test/kotlin/dev/privacyllc/period/feature/lock/LockSettingsViewModelTest.kt new file mode 100644 index 0000000..d1157a6 --- /dev/null +++ b/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() + 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) + } +} diff --git a/core/security/src/main/kotlin/dev/privacyllc/period/core/security/AppLockRepository.kt b/core/security/src/main/kotlin/dev/privacyllc/period/core/security/AppLockRepository.kt index 755aa5e..58b3a4d 100644 --- a/core/security/src/main/kotlin/dev/privacyllc/period/core/security/AppLockRepository.kt +++ b/core/security/src/main/kotlin/dev/privacyllc/period/core/security/AppLockRepository.kt @@ -75,6 +75,21 @@ class AppLockRepository internal constructor( constructor(store: DataStore) : 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, macs: MacProvider) : + this(store, macs, SystemClocks) + private val verifier: PinVerifier = PinVerifier(macs) /** True once a PIN exists. Derived, never stored. */ diff --git a/docs/design/README.md b/docs/design/README.md index 9dc54d7..c158336 100644 --- a/docs/design/README.md +++ b/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