Rotate the bunker URL on every client revoke

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

@@ -0,0 +1,131 @@
+package io.privkey.keep.nip46
+
+import android.content.Context
+import androidx.test.ext.junit.runners.AndroidJUnit4
+import androidx.test.platform.app.InstrumentationRegistry
+import io.privkey.keep.nip55.FrostSignFixture
+import io.privkey.keep.nip55.FrostSignTestSupport
+import io.privkey.keep.uniffi.BunkerApprovalRequest
+import io.privkey.keep.uniffi.BunkerApprovalResult
+import io.privkey.keep.uniffi.BunkerCallbacks
+import io.privkey.keep.uniffi.BunkerHandler
+import io.privkey.keep.uniffi.BunkerLogEvent
+import io.privkey.keep.uniffi.BunkerRememberDuration
+import io.privkey.keep.uniffi.ConnectAuthorization
+import io.privkey.keep.uniffi.KeepMobile
+import io.privkey.keep.uniffi.ParsedBunkerUrl
+import io.privkey.keep.uniffi.parseBunkerUrl
+import kotlinx.coroutines.runBlocking
+import org.junit.After
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertNotEquals
+import org.junit.Assert.assertNotNull
+import org.junit.Assert.assertNull
+import org.junit.Assert.assertTrue
+import org.junit.Assume.assumeTrue
+import org.junit.Test
+import org.junit.runner.RunWith
+
+/**
+ * Revoking a client rotates the bunker credentials even when the bunker service is not
+ * running, so a revoked client that kept the old bunker URL cannot connect again with it.
+ * Starts a real bunker against a public relay, so it is manual-only like the FROST tests,
+ * and it rotates the test device's own bunker URL.
+ */
+@RunWith(AndroidJUnit4::class)
+class BunkerRevokeRotationTest {
+
+    private var mobile: KeepMobile? = null
+
+    @After
+    fun tearDown() {
+        mobile?.destroy()
+        mobile = null
+    }
+
+    @Test(timeout = 180_000L)
+    fun rotatingWithTheServiceStoppedRetiresTheOldBunkerUrl() = runBlocking {
+        val mobile = setUpMobile()
+
+        val before = startedBunkerUrl(mobile)
+        assertNotNull("the bunker URL must carry a connect secret", before.secret)
+        // A restart alone keeps the URL, so the change below comes from the rotation.
+        val restarted = startedBunkerUrl(mobile)
+        assertEquals(before.secret, restarted.secret)
+        assertEquals(before.pubkey, restarted.pubkey)
+
+        BunkerService.rotateBunkerCredentials(mobile)
+        assertNull("the rotation must not have needed the service", BunkerService.current())
+
+        val after = startedBunkerUrl(mobile)
+        assertNotNull(after.secret)
+        assertNotEquals("the old connect secret must be retired", before.secret, after.secret)
+        assertNotEquals("the old bunker key must be retired", before.pubkey, after.pubkey)
+    }
+
+    @Test(timeout = 180_000L)
+    fun aClientRevokedBeforeThisVersionGetsTheUrlRotatedOnce() {
+        val mobile = setUpMobile()
+        val context = InstrumentationRegistry.getInstrumentation().targetContext
+        val markers = context.getSharedPreferences(BunkerService.BUNKER_MARKERS_PREFS, Context.MODE_PRIVATE)
+        val probe = "ab".repeat(32)
+        val alreadyDenylisted = probe in Nip46ClientStore.getDenylist(context)
+        try {
+            markers.edit().remove(BunkerService.KEY_ROTATED_FOR_EARLIER_REVOKES).commit()
+            Nip46ClientStore.addToDenylist(context, probe)
+
+            val before = startedBunkerUrl(mobile)
+            assertTrue(BunkerService.rotateOnceForEarlierRevokes(context, mobile))
+            val after = startedBunkerUrl(mobile)
+            assertNotEquals(before.secret, after.secret)
+            assertNotEquals(before.pubkey, after.pubkey)
+
+            assertFalse("the rotation happens once", BunkerService.rotateOnceForEarlierRevokes(context, mobile))
+            assertEquals(after.secret, startedBunkerUrl(mobile).secret)
+        } finally {
+            if (!alreadyDenylisted) Nip46ClientStore.removeFromDenylist(context, probe)
+        }
+    }
+
+    @Test(timeout = 180_000L)
+    fun withNothingRevokedTheUrlIsLeftAlone() {
+        val mobile = setUpMobile()
+        val context = InstrumentationRegistry.getInstrumentation().targetContext
+        assumeTrue("the device denylist must be empty", Nip46ClientStore.getDenylist(context).isEmpty())
+        context.getSharedPreferences(BunkerService.BUNKER_MARKERS_PREFS, Context.MODE_PRIVATE)
+            .edit().remove(BunkerService.KEY_ROTATED_FOR_EARLIER_REVOKES).commit()
+
+        val before = startedBunkerUrl(mobile)
+        assertFalse(BunkerService.rotateOnceForEarlierRevokes(context, mobile))
+        assertEquals(before.secret, startedBunkerUrl(mobile).secret)
+    }
+
+    private fun setUpMobile(): KeepMobile {
+        val manual = InstrumentationRegistry.getArguments().getString(FrostSignFixture.MANUAL_ARG)
+        assumeTrue("manual-only test; pass -e ${FrostSignFixture.MANUAL_ARG} 1", manual == "1")
+        assumeTrue("the bunker service must not be running", BunkerService.current() == null)
+        val storage = FrostSignTestSupport.noAuthStorage()
+        val mobile = KeepMobile(storage).also { this.mobile = it }
+        FrostSignTestSupport.importShareNoAuth(mobile, storage, FrostSignFixture.SHARE1_EXPORT_DATA, "bunker-rotate-setup")
+        FrostSignTestSupport.initializeWithDecryptContext(mobile, storage, "bunker-rotate-connect")
+        return mobile
+    }
+
+    private fun startedBunkerUrl(mobile: KeepMobile): ParsedBunkerUrl =
+        BunkerHandler(mobile).use { handler ->
+            try {
+                handler.startBunker(listOf(FrostSignFixture.RELAY), RejectAll)
+                parseBunkerUrl(requireNotNull(handler.getBunkerUrl()) { "a started bunker must publish its URL" })
+            } finally {
+                handler.stopBunker()
+            }
+        }
+
+    private object RejectAll : BunkerCallbacks {
+        override fun onLog(event: BunkerLogEvent) = Unit
+        override fun requestApproval(request: BunkerApprovalRequest) =
+            BunkerApprovalResult(approved = false, remember = BunkerRememberDuration.JUST_THIS_TIME)
+        override fun onConnect(pubkey: String, name: String, authorization: ConnectAuthorization) = Unit
+    }
+}

app/src/androidTest/kotlin/io/privkey/keep/nip55/FrostSignTestSupport.kt

@@ -5,6 +5,7 @@ import io.privkey.keep.storage.AndroidKeystoreStorage
 import io.privkey.keep.uniffi.KeepMobile
 import org.junit.Assert.assertEquals
 import org.junit.AssumptionViolatedException
+import javax.crypto.Cipher
 
 object FrostSignTestSupport {
 
@@ -27,14 +28,8 @@ object FrostSignTestSupport {
                     "delete it. Manually clear the device before running this test."
             )
         }
-        val cipher = storage.getCipherForEncryption()
-        storage.setRequestIdContext(requestId)
-        storage.setPendingCipher(requestId, cipher)
-        try {
+        withCipher(storage, requestId, storage.getCipherForEncryption()) {
             mobile.importShare(exportData, FrostSignFixture.PASSPHRASE, "test")
-        } finally {
-            storage.clearRequestIdContext()
-            storage.clearPendingCipher(requestId)
         }
     }
 
@@ -43,11 +38,17 @@ object FrostSignTestSupport {
         storage: AndroidKeystoreStorage,
         requestId: String,
     ) {
-        val cipher = requireNotNull(storage.getCipherForDecryption()) { "decryption cipher must not be null" }
+        withCipher(storage, requestId, requireNotNull(storage.getCipherForDecryption()) { "decryption cipher must not be null" }) {
+            mobile.initialize(listOf(FrostSignFixture.RELAY))
+        }
+    }
+
+    /** Runs [block] with [cipher] staged as the keystore context the core reads for [requestId]. */
+    fun <T> withCipher(storage: AndroidKeystoreStorage, requestId: String, cipher: Cipher, block: () -> T): T {
         storage.setPendingCipher(requestId, cipher)
         storage.setRequestIdContext(requestId)
         try {
-            mobile.initialize(listOf(FrostSignFixture.RELAY))
+            return block()
         } finally {
             storage.clearRequestIdContext()
             storage.clearPendingCipher(requestId)

app/src/main/kotlin/io/privkey/keep/nip46/BunkerScreen.kt

@@ -16,7 +16,9 @@ import androidx.compose.ui.Modifier
 import androidx.compose.ui.platform.LocalContext
 import androidx.compose.ui.res.stringResource
 import androidx.compose.ui.unit.dp
+import kotlinx.coroutines.CancellationException
 import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.NonCancellable
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.withContext
 import io.privkey.keep.MAX_BUNKER_RELAYS
@@ -115,6 +117,7 @@ fun BunkerScreen(
     var authorizedClients by remember { mutableStateOf<Set<String>>(emptySet()) }
     var showAddDialog by remember { mutableStateOf(false) }
     var showRevokeAllDialog by remember { mutableStateOf(false) }
+    var pendingRevoke by remember { mutableStateOf<String?>(null) }
     var showRotateUrlDialog by remember { mutableStateOf(false) }
     val isEnabled = bunkerStatus == BunkerStatus.RUNNING || bunkerStatus == BunkerStatus.STARTING
 
@@ -207,17 +210,18 @@ fun BunkerScreen(
             }
 
             Spacer(modifier = Modifier.height(8.dp))
+        }
 
-            OutlinedButton(
-                onClick = { showRotateUrlDialog = true },
-                modifier = Modifier.fillMaxWidth()
-            ) {
-                Text(stringResource(R.string.connections_bunker_rotate_url_button))
-            }
-
-            Spacer(modifier = Modifier.height(16.dp))
+        // Offered while stopped too: a URL handed out earlier stays valid until rotated.
+        OutlinedButton(
+            onClick = { showRotateUrlDialog = true },
+            modifier = Modifier.fillMaxWidth()
+        ) {
+            Text(stringResource(R.string.connections_bunker_rotate_url_button))
         }
 
+        Spacer(modifier = Modifier.height(16.dp))
+
         BunkerToggleCard(
             enabled = isEnabled,
             canEnable = relays.isNotEmpty(),
@@ -259,26 +263,7 @@ fun BunkerScreen(
 
         AuthorizedClientsCard(
             clients = authorizedClients,
-            onRevoke = { pubkey ->
-                scope.launch {
-                    runCatching {
-                        withContext(Dispatchers.IO) {
-                            Nip46ClientStore.addToDenylist(context, pubkey)
-                            val saved = BunkerConfigStore.update(keepMobile) { config ->
-                                BunkerConfigInfo(config.enabled, config.authorizedClients.filter { it.lowercase() != pubkey.lowercase() })
-                            }
-                            BunkerService.forgetPendingAuth(pubkey)
-                            BunkerService.revokeClientInEngine(pubkey)
-                            saved
-                        }
-                    }.onSuccess { saved ->
-                        authorizedClients = saved.authorizedClients.toSet()
-                        Toast.makeText(context, toastClientRevoked, Toast.LENGTH_SHORT).show()
-                    }.onFailure {
-                        Toast.makeText(context, toastRevokeFailed, Toast.LENGTH_SHORT).show()
-                    }
-                }
-            },
+            onRevoke = { pubkey -> pendingRevoke = pubkey },
             onRevokeAll = { showRevokeAllDialog = true }
         )
     }
@@ -295,24 +280,47 @@ fun BunkerScreen(
         )
     }
 
+    pendingRevoke?.let { pubkey ->
+        RevokeClientDialog(
+            onConfirm = {
+                pendingRevoke = null
+                scope.launch {
+                    runCatching {
+                        withContext(NonCancellable + Dispatchers.IO) {
+                            BunkerService.revokeClients(context, keepMobile, listOf(pubkey))
+                        }
+                    }.onSuccess { saved ->
+                        authorizedClients = saved.authorizedClients.toSet()
+                        Toast.makeText(context, toastClientRevoked, Toast.LENGTH_SHORT).show()
+                    }.onFailure {
+                        if (it is CancellationException) throw it
+                        // A revoke can fail after it has changed the config, so show what is saved now.
+                        runCatching { withContext(Dispatchers.IO) { keepMobile.getBunkerConfig() } }
+                            .onSuccess { saved -> authorizedClients = saved.authorizedClients.toSet() }
+                        Toast.makeText(context, toastRevokeFailed, Toast.LENGTH_SHORT).show()
+                    }
+                }
+            },
+            onDismiss = { pendingRevoke = null }
+        )
+    }
+
     if (showRevokeAllDialog) {
         RevokeAllClientsDialog(
             onConfirm = {
                 scope.launch {
-                    val revoked = authorizedClients
                     runCatching {
-                        withContext(Dispatchers.IO) {
-                            Nip46ClientStore.addAllToDenylist(context, revoked)
-                            BunkerConfigStore.update(keepMobile) { config ->
-                                BunkerConfigInfo(config.enabled, emptyList())
-                            }
-                            revoked.forEach { BunkerService.forgetPendingAuth(it) }
-                            BunkerService.revokeAllClientsInEngine()
+                        withContext(NonCancellable + Dispatchers.IO) {
+                            BunkerService.revokeClients(context, keepMobile, null)
                         }
-                    }.onSuccess {
-                        authorizedClients = emptySet()
+                    }.onSuccess { saved ->
+                        authorizedClients = saved.authorizedClients.toSet()
                         Toast.makeText(context, toastAllClientsRevoked, Toast.LENGTH_SHORT).show()
                     }.onFailure {
+                        if (it is CancellationException) throw it
+                        // A revoke can fail after it has changed the config, so show what is saved now.
+                        runCatching { withContext(Dispatchers.IO) { keepMobile.getBunkerConfig() } }
+                            .onSuccess { saved -> authorizedClients = saved.authorizedClients.toSet() }
                         Toast.makeText(context, toastRevokeAllFailed, Toast.LENGTH_SHORT).show()
                     }
                 }
@@ -327,12 +335,13 @@ fun BunkerScreen(
                 showRotateUrlDialog = false
                 scope.launch {
                     runCatching {
-                        withContext(Dispatchers.IO) {
-                            BunkerService.rotateBunkerUrlInService()
+                        withContext(NonCancellable + Dispatchers.IO) {
+                            BunkerService.rotateBunkerCredentials(keepMobile)
                         }
                     }.onSuccess {
                         Toast.makeText(context, toastUrlRotated, Toast.LENGTH_SHORT).show()
                     }.onFailure {
+                        if (it is CancellationException) throw it
                         Toast.makeText(context, toastRotateUrlFailed, Toast.LENGTH_SHORT).show()
                     }
                 }
@@ -471,6 +480,35 @@ private fun AddBunkerRelayDialog(
     )
 }
 
+@Composable
+private fun RevokeClientDialog(
+    onConfirm: () -> Unit,
+    onDismiss: () -> Unit
+) {
+    AlertDialog(
+        onDismissRequest = onDismiss,
+        title = { Text(stringResource(R.string.connections_bunker_revoke_client_title)) },
+        text = {
+            Text(stringResource(R.string.connections_bunker_revoke_client_text))
+        },
+        confirmButton = {
+            TextButton(
+                onClick = onConfirm,
+                colors = ButtonDefaults.textButtonColors(
+                    contentColor = MaterialTheme.colorScheme.error
+                )
+            ) {
+                Text(stringResource(R.string.connections_bunker_revoke_client_confirm))
+            }
+        },
+        dismissButton = {
+            TextButton(onClick = onDismiss) {
+                Text(stringResource(R.string.connections_bunker_revoke_all_cancel))
+            }
+        }
+    )
+}
+
 @Composable
 private fun RevokeAllClientsDialog(
     onConfirm: () -> Unit,