Drop the previous account's bunker clients and grants on an account switch

app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt

@@ -0,0 +1,241 @@
+package io.privkey.keep.nip46
+
+import android.content.Context
+import android.content.SharedPreferences
+import androidx.test.ext.junit.runners.AndroidJUnit4
+import androidx.test.platform.app.InstrumentationRegistry
+import org.junit.After
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertThrows
+import org.junit.Assert.assertTrue
+import org.junit.Before
+import org.junit.Test
+import org.junit.runner.RunWith
+
+@RunWith(AndroidJUnit4::class)
+class RevokeAllMarkTest {
+    private lateinit var prefs: SharedPreferences
+
+    @Before
+    fun setUp() {
+        prefs = InstrumentationRegistry.getInstrumentation().targetContext
+            .getSharedPreferences("revoke_all_mark_test", Context.MODE_PRIVATE)
+        prefs.edit().clear().commit()
+    }
+
+    @After
+    fun tearDown() {
+        prefs.edit().clear().commit()
+    }
+
+    @Test
+    fun aSavedMarkOutlivesTheProcess() {
+        RevokeAllMark(prefs).set()
+        assertTrue(RevokeAllMark(prefs).isSet())
+    }
+
+    @Test
+    fun aMarkThatWasNotSavedStillAppliesInThisProcess() {
+        val mark = RevokeAllMark(prefs)
+        mark.set()
+        prefs.edit().remove(BunkerService.KEY_REVOKE_ALL_ON_START).commit()
+        assertTrue(mark.isSet())
+        assertFalse(RevokeAllMark(prefs).isSet())
+    }
+
+    @Test
+    fun aStartDoesNotClearAMarkSetAfterItLooked() {
+        val mark = RevokeAllMark(prefs)
+        mark.set()
+        val seen = mark.requestsSoFar()
+        mark.set()
+        assertFalse(mark.clearIfUnchanged(seen) { true })
+        assertTrue(mark.isSet())
+        assertTrue(RevokeAllMark(prefs).isSet())
+    }
+
+    @Test
+    fun aStartThatStoppedServingDoesNotClearTheMark() {
+        val mark = RevokeAllMark(prefs)
+        mark.set()
+        assertFalse(mark.clearIfUnchanged(mark.requestsSoFar()) { false })
+        assertTrue(mark.isSet())
+    }
+
+    @Test
+    fun aServingStartClearsTheMarkEverywhere() {
+        val mark = RevokeAllMark(prefs)
+        mark.set()
+        assertTrue(mark.clearIfUnchanged(mark.requestsSoFar()) { true })
+        assertFalse(mark.isSet())
+        assertFalse(RevokeAllMark(prefs).isSet())
+    }
+
+    @Test
+    fun aMarkSetAheadIsSavedBeforeTheChange() {
+        RevokeAllMark(prefs).setAhead()
+        assertTrue(RevokeAllMark(prefs).isSet())
+    }
+
+    @Test
+    fun aStartDuringAnOpenChangeRevokesAndKeepsTheMark() {
+        val mark = RevokeAllMark(prefs)
+        mark.setAhead()
+        val seen = mark.requestsSoFar()
+        assertTrue(mark.isSet())
+        assertTrue(mark.clearIfUnchanged(seen) { true })
+        assertTrue("a later start revokes again while the change is open", mark.isSet())
+        assertTrue(RevokeAllMark(prefs).isSet())
+    }
+
+    @Test
+    fun aMarkThatCouldNotBeSavedAheadLeavesNothingBehind() {
+        var failing = true
+        val mark = RevokeAllMark(failingCommits(prefs) { failing })
+        assertThrows(IllegalStateException::class.java) { mark.setAhead() }
+        assertFalse(mark.isSet())
+        failing = false
+        mark.setAhead()(false)
+        assertFalse("a failed save must not leave a change open", mark.isSet())
+    }
+
+    @Test
+    fun aChangeThatSwitchedKeepsTheMark() {
+        val mark = RevokeAllMark(prefs)
+        val seen = mark.requestsSoFar()
+        mark.setAhead()(true)
+        assertTrue(mark.isSet())
+        assertTrue(RevokeAllMark(prefs).isSet())
+        assertFalse("a start that looked before the switch must not clear it", mark.clearIfUnchanged(seen) { true })
+    }
+
+    @Test
+    fun aChangeThatDidNotSwitchWithdrawsTheMark() {
+        val mark = RevokeAllMark(prefs)
+        mark.setAhead()(false)
+        assertFalse(mark.isSet())
+        assertFalse(RevokeAllMark(prefs).isSet())
+    }
+
+    @Test
+    fun aStartKeepsTheSavedMarkWhileAChangeIsOpen() {
+        val mark = RevokeAllMark(prefs)
+        mark.set()
+        val finish = mark.setAhead()
+        assertTrue(mark.clearIfUnchanged(mark.requestsSoFar()) { true })
+        assertTrue("the open change still needs the saved mark", RevokeAllMark(prefs).isSet())
+        finish(true)
+        assertTrue(mark.isSet())
+        assertTrue(RevokeAllMark(prefs).isSet())
+    }
+
+    @Test
+    fun aChangeThatDidNotSwitchWithdrawsAMarkAStartAlreadyRevokedFor() {
+        val mark = RevokeAllMark(prefs)
+        mark.set()
+        val finish = mark.setAhead()
+        assertTrue(mark.clearIfUnchanged(mark.requestsSoFar()) { true })
+        finish(false)
+        assertFalse(mark.isSet())
+        assertFalse(RevokeAllMark(prefs).isSet())
+    }
+
+    @Test
+    fun anOpenChangeKeepsTheMarkAnotherOneFinishedWithout() {
+        val mark = RevokeAllMark(prefs)
+        val first = mark.setAhead()
+        mark.setAhead()(false)
+        assertTrue(RevokeAllMark(prefs).isSet())
+        first(false)
+        assertFalse(mark.isSet())
+        assertFalse(RevokeAllMark(prefs).isSet())
+    }
+
+    @Test
+    fun aChangeThatDidNotSwitchLeavesAMarkThatWasAlreadyPending() {
+        val mark = RevokeAllMark(prefs)
+        mark.set()
+        mark.setAhead()(false)
+        assertTrue(mark.isSet())
+        assertTrue(RevokeAllMark(prefs).isSet())
+    }
+
+    @Test
+    fun aChangeThatDidNotSwitchLeavesAMarkSavedByAnEarlierProcess() {
+        RevokeAllMark(prefs).set()
+        val mark = RevokeAllMark(prefs)
+        val finish = mark.setAhead()
+        assertTrue(mark.isSet())
+        finish(false)
+        assertTrue(mark.isSet())
+        assertTrue(RevokeAllMark(prefs).isSet())
+    }
+
+    @Test
+    fun aChangeFinishesOnlyOnce() {
+        val mark = RevokeAllMark(prefs)
+        val first = mark.setAhead()
+        val second = mark.setAhead()
+        first(false)
+        first(false)
+        assertTrue("a repeated finish must not close another open change", RevokeAllMark(prefs).isSet())
+        second(false)
+        assertFalse(RevokeAllMark(prefs).isSet())
+    }
+
+    @Test
+    fun aStartThatRevokedCleanlyClearsTheMark() {
+        val mark = RevokeAllMark(prefs)
+        mark.set()
+        var revoked = false
+        mark.revokeIfSet(revoke = { revoked = true }, serving = { true }, faultsSoFar = { 0L })
+        assertTrue(revoked)
+        assertFalse(mark.isSet())
+        assertFalse(RevokeAllMark(prefs).isSet())
+    }
+
+    @Test
+    fun aStorageFaultDuringTheRevokeKeepsTheMark() {
+        val mark = RevokeAllMark(prefs)
+        mark.set()
+        var faults = 0L
+        mark.revokeIfSet(revoke = { faults++ }, serving = { true }, faultsSoFar = { faults })
+        assertTrue(mark.isSet())
+        assertTrue(RevokeAllMark(prefs).isSet())
+    }
+
+    @Test
+    fun aStartThatCannotCountFaultsKeepsTheMark() {
+        val mark = RevokeAllMark(prefs)
+        mark.set()
+        mark.revokeIfSet(revoke = {}, serving = { true }, faultsSoFar = { null })
+        assertTrue(mark.isSet())
+    }
+
+    @Test
+    fun aStartThatStoppedServingKeepsTheMarkAfterItsRevoke() {
+        val mark = RevokeAllMark(prefs)
+        mark.set()
+        mark.revokeIfSet(revoke = {}, serving = { false }, faultsSoFar = { 0L })
+        assertTrue(mark.isSet())
+    }
+
+    @Test
+    fun aStartWithNoMarkRevokesNothing() {
+        var revoked = false
+        RevokeAllMark(prefs).revokeIfSet(revoke = { revoked = true }, serving = { true }, faultsSoFar = { 0L })
+        assertFalse(revoked)
+    }
+
+    // A commit that fails as a disk write does: applied in memory, reported as not saved.
+    private fun failingCommits(prefs: SharedPreferences, failing: () -> Boolean) = object : SharedPreferences by prefs {
+        override fun edit(): SharedPreferences.Editor {
+            val editor = prefs.edit()
+            return object : SharedPreferences.Editor by editor {
+                override fun putBoolean(key: String?, value: Boolean): SharedPreferences.Editor = apply { editor.putBoolean(key, value) }
+                override fun remove(key: String?): SharedPreferences.Editor = apply { editor.remove(key) }
+                override fun commit(): Boolean = if (failing()) false.also { editor.apply() } else editor.commit()
+            }
+        }
+    }
+}

app/src/androidTest/kotlin/io/privkey/keep/storage/StorageFaultCountTest.kt

@@ -0,0 +1,37 @@
+package io.privkey.keep.storage
+
+import androidx.test.ext.junit.runners.AndroidJUnit4
+import androidx.test.platform.app.InstrumentationRegistry
+import io.privkey.keep.uniffi.KeepMobileException
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertThrows
+import org.junit.Test
+import org.junit.runner.RunWith
+
+@RunWith(AndroidJUnit4::class)
+class StorageFaultCountTest {
+    private val storage = AndroidKeystoreStorage(InstrumentationRegistry.getInstrumentation().targetContext)
+
+    @Test
+    fun aFailedCallIsCounted() {
+        val before = storage.faultsSoFar()
+        assertThrows(KeepMobileException.StorageException::class.java) { storage.loadShareByKey("") }
+        assertEquals(before + 1, storage.faultsSoFar())
+    }
+
+    @Test
+    fun aStorageErrorIsCounted() {
+        val before = storage.faultsSoFar()
+        // A share key outside a request has no cipher to use.
+        assertThrows(KeepMobileException.StorageException::class.java) { storage.loadShareByKey("fault_count_test") }
+        assertEquals(before + 1, storage.faultsSoFar())
+    }
+
+    @Test
+    fun anAbsentRecordIsNotAFault() {
+        val before = storage.faultsSoFar()
+        // Fixed, since each metadata record name gets its own Keystore key.
+        assertThrows(KeepMobileException.StorageNotFound::class.java) { storage.loadShareByKey("__keep_fault_count_test") }
+        assertEquals(before, storage.faultsSoFar())
+    }
+}

app/src/main/kotlin/io/privkey/keep/AccountActions.kt

@@ -7,6 +7,7 @@ import java.util.concurrent.atomic.AtomicBoolean
 import java.util.concurrent.atomic.AtomicReference
 import android.util.Log
 import android.widget.Toast
+import io.privkey.keep.nip46.BunkerService
 import io.privkey.keep.storage.AndroidKeystoreStorage
 import io.privkey.keep.uniffi.DkgConfig
 import io.privkey.keep.uniffi.DkgProgressCallback
@@ -63,10 +64,11 @@ internal class AccountActions(
     private val coroutineScope: CoroutineScope,
     private val appContext: Context,
     private val onBiometricRequest: (String, String, Cipher, (Cipher?) -> Unit) -> Unit,
-    private val onAccountSwitched: suspend () -> Unit,
+    private val onAccountSwitched: suspend (stopIfBunkerResetFails: Boolean) -> Boolean,
     private val onAccountDeleted: suspend () -> Unit,
     private val onLiveNodeDropped: () -> Unit,
-    private val onStateChanged: (AccountState) -> Unit
+    private val onStateChanged: (AccountState) -> Unit,
+    private val markBunkerRevokeAhead: () -> (switched: Boolean) -> Unit = { BunkerService.markRevokeAllAhead(appContext) }
 ) {
     data class AccountState(
         val hasShare: Boolean,
@@ -129,12 +131,20 @@ internal class AccountActions(
      * failed attempt that changed nothing, or a first account, wipes nothing. A pointer or
      * share list that cannot be read counts as a change. A store that succeeded also
      * reports the core's node drop.
+     *
+     * When a share already exists, or that cannot be read, the bunker's revoke-all mark is
+     * saved before [store] runs, since nothing can stop the switch once the core has made it;
+     * when the mark cannot be saved, [store] does not run. It becomes the switch's mark when
+     * the store switched; otherwise it is withdrawn unless another change or switch needs it.
      */
     private suspend fun <T> storingNewActiveShare(store: suspend () -> T): T {
         val before = withContext(Dispatchers.IO) {
             val hadShare = runCatching { keepMobile.hasShare() }.getOrDefault(true)
             hadShare to runCatching { storage.getActiveShareKey() }.getOrNull()
         }
+        // Not cancellable: a cancel landing after the save would lose what finishes it. Nested,
+        // since a withContext that also changes dispatcher returns cancellably.
+        val finishMark = if (before.first) withContext(NonCancellable) { withContext(Dispatchers.IO) { markBunkerRevokeAhead() } } else null
         var stored = false
         try {
             return withContext(Dispatchers.IO) { store().also { stored = true } }
@@ -148,7 +158,12 @@ internal class AccountActions(
                 val activeAfter = withContext(Dispatchers.IO) {
                     runCatching { storage.getActiveShareKey() }.getOrNull()
                 }
-                if (storeSwitchedAccount(hadShare, activeBefore, activeAfter)) onAccountSwitched()
+                val switched = storeSwitchedAccount(hadShare, activeBefore, activeAfter)
+                if (finishMark != null) {
+                    withContext(Dispatchers.IO) { runCatching { finishMark(switched) } }
+                        .onFailure { if (BuildConfig.DEBUG) Log.e("AccountActions", "Revoke mark withdraw failed: ${it::class.simpleName}") }
+                }
+                if (switched) onAccountSwitched(false)
             }
         }
     }
@@ -226,7 +241,9 @@ internal class AccountActions(
                     // the transition window fail-safe (no grants + old key -> the
                     // request re-prompts). onAccountSwitched clears state account-
                     // agnostically and does not depend on the new active share.
-                    onAccountSwitched()
+                    // A bunker that could not be reset would serve the previous account's
+                    // clients as this one, so the switch stops here, before activation.
+                    check(onAccountSwitched(true)) { "bunker reset failed" }
                     activateShare(authedCipher, account.groupPubkeyHex)
                     refreshAccountState()
                 } catch (e: Exception) {
@@ -254,8 +271,14 @@ internal class AccountActions(
                     // Before the delete, not after the follow-up switch prompt. The core's
                     // teardown leaves a running bunker serving with the deleted share, and
                     // this account's grants would otherwise stand until that prompt is
-                    // answered, which it may never be if the activity goes away first.
-                    onAccountSwitched()
+                    // answered, which it may never be if the activity goes away first. Stops
+                    // the delete when the bunker cannot be reset: the next account would
+                    // otherwise take over its clients, with one left even without a switch.
+                    if (!onAccountSwitched(true)) {
+                        logAndToast("Delete failed", appContext.getString(R.string.account_delete_failed), IllegalStateException("bunker reset failed"))
+                        onDismiss()
+                        return@withLock
+                    }
                 }
                 try {
                     withContext(Dispatchers.IO) {
@@ -301,7 +324,7 @@ internal class AccountActions(
             switchToNextAccountAfterDelete(remainingAccounts.first(), onDismiss)
         } else if (wasActive) {
             try {
-                onAccountSwitched()
+                onAccountSwitched(false)
             } finally {
                 refreshAccountState()
                 onDismiss()
@@ -329,8 +352,12 @@ internal class AccountActions(
                             // activating the next account's key, so a signing request
                             // racing this switch cannot resolve an old grant against
                             // the new key (revoke-before-activate; see switchAccount).
-                            onAccountSwitched()
-                            if (switchAuthed != null) {
+                            // Not a stop: the account is already deleted, so the cleanup
+                            // runs whatever the bunker reset does. Activation still waits on
+                            // the reset (see switchAccount); with one account left, which is
+                            // then in use anyway, the reset before the delete already held.
+                            val bunkerReset = onAccountSwitched(false)
+                            if (switchAuthed != null && bunkerReset) {
                                 try {
                                     activateShare(switchAuthed, nextAccount.groupPubkeyHex)
                                 } catch (e: Exception) {
@@ -346,7 +373,7 @@ internal class AccountActions(
             }
         } else {
             try {
-                onAccountSwitched()
+                onAccountSwitched(false)
                 refreshAccountState()
             } finally {
                 onDismiss()