diff --git a/app/src/main/kotlin/io/flamenet/messenger/app/MainActivity.kt b/app/src/main/kotlin/io/flamenet/messenger/app/MainActivity.kt index aa4d200..a5af607 100644 --- a/app/src/main/kotlin/io/flamenet/messenger/app/MainActivity.kt +++ b/app/src/main/kotlin/io/flamenet/messenger/app/MainActivity.kt @@ -734,16 +734,14 @@ internal fun ColumnScope.SignedIn( ) } - // A call refused because the peer's keys changed offers the review here, - // rather than telling somebody to go and find the conversation (ios#73). - // Compare and accept both live behind this, in the banner the chat already - // draws, so the wording it is judged by is unchanged. - if (phase.callKeyReview != null) { - KitSecondaryButton( - text = "Review their security code", - onClick = { model.reviewBlockedCallKeys() }, - tag = "review-call-keys", - ) + // A call refused because the peer's keys changed is dealt with here, where + // it failed (ios#73). This first opened the conversation instead, which is + // the same gap one screen along: a call placed from the Calls tab, or + // answered from a notification, never passed through a conversation, so + // sending somebody to one to find out why their call failed is the + // complaint rather than the cure. + phase.callKeyReview?.let { review -> + CallKeyReviewDialog(model, review) } if (addingContact) { @@ -827,3 +825,89 @@ private fun ColumnScope.VaultUnreadable( Text("Erase and start fresh") } } + +/** + * The cure for a call the trust gate refused, offered where the call failed. + * + * Accepting keys in a hurry is the risk here, and that is the argument for + * saying plainly what accepting means rather than for hiding it behind another + * screen: an extra tap in front of the same decision slows nobody down, it just + * costs somebody the explanation. The wording matches the conversation banner, + * which is the standard this is held to, and the safety number is on the dialog + * because comparing it is what actually settles the question. + */ +@androidx.compose.runtime.Composable +private fun CallKeyReviewDialog( + model: MessengerViewModel, + review: MessengerViewModel.CallKeyReview, +) { + androidx.compose.material3.AlertDialog( + onDismissRequest = { model.dismissBlockedCallKeys() }, + title = { Text("Security keys changed") }, + text = { + androidx.compose.foundation.layout.Column( + verticalArrangement = androidx.compose.foundation.layout.Arrangement.spacedBy(Space.sm), + ) { + Text( + "${review.peerName}'s keys are not the ones you accepted before, so the " + + "call was not placed.", + style = MaterialTheme.typography.bodyMedium, + ) + // Both readings, in the order that matters, as the chat banner + // has them. + Text( + "That happens when someone reinstalls or adds a device, and it is also " + + "what an intercepted conversation looks like.", + style = MaterialTheme.typography.bodySmall, + ) + review.safetyNumber?.let { number -> + Text( + number, + style = MaterialTheme.typography.bodySmall, + fontFamily = androidx.compose.ui.text.font.FontFamily.Monospace, + modifier = Modifier.testTag("call-safety-number"), + ) + } + if (review.accepted) { + Text( + "New keys accepted. You can call ${review.peerName} again.", + style = MaterialTheme.typography.bodyMedium, + ) + } else { + Text( + "Accepting means this call, and everything you send after it, goes to " + + "whoever holds the new keys. Read the number to them on a channel " + + "this app does not control, and accept only if it matches.", + style = MaterialTheme.typography.bodySmall, + ) + } + } + }, + confirmButton = { + if (review.accepted) { + KitSecondaryButton( + text = "Done", + onClick = { model.dismissBlockedCallKeys() }, + tag = "call-keys-done", + ) + } else { + KitSecondaryButton( + text = "Accept new keys", + onClick = { model.acceptBlockedCallKeys() }, + tag = "call-accept-new-keys", + danger = true, + ) + } + }, + dismissButton = { + if (!review.accepted) { + KitChip( + text = "Not now", + selected = false, + onClick = { model.dismissBlockedCallKeys() }, + tag = "call-keys-not-now", + ) + } + }, + ) +} diff --git a/app/src/main/kotlin/io/flamenet/messenger/app/MessengerViewModel.kt b/app/src/main/kotlin/io/flamenet/messenger/app/MessengerViewModel.kt index f960452..eb1d8d3 100644 --- a/app/src/main/kotlin/io/flamenet/messenger/app/MessengerViewModel.kt +++ b/app/src/main/kotlin/io/flamenet/messenger/app/MessengerViewModel.kt @@ -128,7 +128,7 @@ class MessengerViewModel( * The peer whose key change blocked a call, so the failure can * offer the review instead of describing it (ios#73). */ - val callKeyReview: Pair? = null, + val callKeyReview: CallKeyReview? = null, /** Shown once, before the first call ever placed on this device. */ val callPrivacyPrompt: Long? = null, /** @@ -389,14 +389,33 @@ class MessengerViewModel( } /** - * Open the conversation with the peer whose key change blocked a call, and - * clear the failure (ios#73). + * What a call refused by the trust gate needs in order to be dealt with + * where it failed (ios#73). The safety number rides along because comparing + * it is the thing that settles the question, and a call started from the + * Calls tab never passed through the conversation that would have shown it. */ - fun reviewBlockedCallKeys() { + data class CallKeyReview( + val peerId: Long, + val peerName: String, + val safetyNumber: String?, + val accepted: Boolean = false, + ) + + /** Put the review away, whether or not the keys were accepted. */ + fun dismissBlockedCallKeys() { val current = _phase.value as? Phase.SignedIn ?: return - val (peerId, peerName) = current.callKeyReview ?: return _phase.value = current.copy(error = null, callKeyReview = null) - openChat(peerId, peerName) + } + + /** + * Accept the changed keys from the call failure itself, rather than sending + * somebody to the conversation to find the same two buttons. + */ + fun acceptBlockedCallKeys() { + val current = _phase.value as? Phase.SignedIn ?: return + val review = current.callKeyReview ?: return + acceptIdentityChange(review.peerId) + update { it.copy(callKeyReview = review.copy(accepted = true), error = null) } } fun signIn( @@ -2296,7 +2315,16 @@ class MessengerViewModel( "Their security code changed, so the call was " + "not placed. Review the change before you " + "call again.", - callKeyReview = peerId to nameFor(it, peerId), + callKeyReview = + CallKeyReview( + peerId = peerId, + peerName = nameFor(it, peerId), + safetyNumber = + session + ?.conversations + ?.trust + ?.safetyNumber(peerId), + ), ) } } diff --git a/app/src/test/kotlin/io/flamenet/messenger/app/CallKeyReviewTest.kt b/app/src/test/kotlin/io/flamenet/messenger/app/CallKeyReviewTest.kt index dc86a4d..1f26d90 100644 --- a/app/src/test/kotlin/io/flamenet/messenger/app/CallKeyReviewTest.kt +++ b/app/src/test/kotlin/io/flamenet/messenger/app/CallKeyReviewTest.kt @@ -6,7 +6,14 @@ import org.junit.Assert.assertTrue import org.junit.Test /** - * A call refused because the peer's keys changed offers the review (ios#73). + * A call refused because the peer's keys changed is dealt with where it failed + * (ios#73). + * + * The first pass at this named the reason and offered a button, and the button + * opened the conversation. That is the same gap one screen along: the reason + * the conversation banner does not already cover this case is that a call + * placed from the Calls tab, or answered from a notification, never passes + * through a conversation. */ class CallKeyReviewTest { private fun source(path: String): String { @@ -15,39 +22,78 @@ class CallKeyReviewTest { return file.readText() } - /** The blocked peer is carried, not just named in a sentence. */ + /** + * The blocked peer is carried, with the number that settles the question. + */ @Test fun theBlockedPeerIsCarried() { val vm = source("MessengerViewModel.kt") assertTrue( "the failure must carry the peer", - vm.contains("val callKeyReview: Pair? = null"), + vm.contains("val callKeyReview: CallKeyReview? = null"), ) assertTrue( - "and be set where the trust gate refuses the signal", - vm.contains("callKeyReview = peerId to nameFor(it, peerId)"), + "comparing the number is what settles it, so it travels with the failure", + vm.contains("data class CallKeyReview(") && vm.contains("val safetyNumber: String?"), + ) + assertTrue( + "and it is set where the trust gate refuses the signal", + vm.contains("callKeyReview =") && vm.contains("CallKeyReview("), ) } - /** Tapping it opens that conversation and clears the failure. */ + /** Accepting happens here, not one screen further on. */ @Test - fun reviewOpensTheConversation() { + fun acceptingHappensAtTheFailure() { val vm = source("MessengerViewModel.kt") - assertTrue(vm.contains("fun reviewBlockedCallKeys()")) - assertTrue("must open the chat", vm.contains("openChat(peerId, peerName)")) + assertTrue(vm.contains("fun acceptBlockedCallKeys()")) assertTrue( - "and must not leave the failure on screen", - vm.contains("copy(error = null, callKeyReview = null)"), + "it must actually accept, not just look like it", + vm.contains("acceptIdentityChange(review.peerId)"), + ) + assertTrue("and there must be a way out", vm.contains("fun dismissBlockedCallKeys()")) + assertFalse( + "sending somebody to the conversation is the bug being fixed", + vm.contains("fun reviewBlockedCallKeys()"), ) } - /** The button is on the screen the failure appears on. */ + /** The dialog is on the screen the failure appears on. */ @Test - fun theButtonIsRendered() { + fun theDialogIsRendered() { val main = source("MainActivity.kt") - assertTrue(main.contains("phase.callKeyReview != null")) - assertTrue(main.contains("model.reviewBlockedCallKeys()")) - assertTrue(main.contains("\"review-call-keys\"")) + assertTrue(main.contains("CallKeyReviewDialog(model, review)")) + assertTrue(main.contains("\"call-accept-new-keys\"")) + assertTrue(main.contains("\"call-safety-number\"")) + } + + /** + * Accepting in a hurry is the risk, so the screen says what accepting + * means. The requirement is not "no accept button", it is "no accept button + * without the sentence that says what accepting costs". + */ + @Test + fun acceptingIsWordedToTheStandard() { + val main = source("MainActivity.kt") + // Both readings, as the conversation banner has them. + assertTrue(main.contains("reinstalls or adds a device")) + assertTrue( + "the unlikely reading is the one that matters and must not be dropped", + main.contains("an intercepted conversation looks like"), + ) + assertTrue( + "and what accepting actually commits to", + main.contains("everything you send after it"), + ) + assertTrue(main.contains("accept only if it matches")) + } + + /** The conversation banner is the standard being matched. */ + @Test + fun theBannerIsStillTheStandard() { + val chat = source("ChatScreens.kt") + assertTrue(chat.contains("Accept new keys")) + assertTrue(chat.contains("an intercepted conversation looks like")) } /** @@ -58,11 +104,5 @@ class CallKeyReviewTest { fun theReasonIsStillNamed() { val vm = source("MessengerViewModel.kt") assertTrue(vm.contains("Their security code changed")) - // The quoted form only: the KDoc on reviewBlockedCallKeys describes - // opening a conversation, and that is not copy anybody reads. - assertFalse( - "must not tell somebody to go and find the conversation themselves", - vm.contains("\"Open the conversation with "), - ) } } diff --git a/client/src/main/kotlin/io/flamenet/client/RelayClient.kt b/client/src/main/kotlin/io/flamenet/client/RelayClient.kt index eccf9f2..f970064 100644 --- a/client/src/main/kotlin/io/flamenet/client/RelayClient.kt +++ b/client/src/main/kotlin/io/flamenet/client/RelayClient.kt @@ -785,11 +785,31 @@ class RelayClient( ): Response { val first = client.newCall(build(method, url, logicalPath, body)).execute() if (first.code != 401 || auth == null) return checked(first, logicalPath) - first.close() - auth?.invalidate() + // Read before closing: the reason is in the body, and one of the + // reasons costs a whole round trip to act on wrongly. + val spent = first.use { spentProof(it) } + // A proof is single use, and the request that spent one is often not a + // request this code made: OkHttp retries across a connection failure + // with the headers it already built, which is what a phone moving + // between wi-fi and mobile data produces. The relay refuses the replay, + // correctly. Throwing the token away for it means minting another + // before carrying on, and during a call that lands in front of the + // signalling. `build` signs afresh either way; only the token differs. + if (!spent) auth?.invalidate() return checked(client.newCall(build(method, url, logicalPath, body)).execute(), logicalPath) } + /** + * Whether a 401 says only that the proof of possession had already been + * spent. The relay names it; one too old to name it still says so in words. + */ + private fun spentProof(response: Response): Boolean { + val body = runCatching { response.body?.string() }.getOrNull() ?: return false + val parsed = runCatching { JSONObject(body) }.getOrNull() ?: return false + if (parsed.optString("code") == "proof_replayed") return true + return parsed.optString("message").contains("already used", ignoreCase = true) + } + private fun build( method: String, url: HttpUrl, diff --git a/client/src/test/kotlin/io/flamenet/client/RelayClientTest.kt b/client/src/test/kotlin/io/flamenet/client/RelayClientTest.kt index 0e0dc8f..88731b7 100644 --- a/client/src/test/kotlin/io/flamenet/client/RelayClientTest.kt +++ b/client/src/test/kotlin/io/flamenet/client/RelayClientTest.kt @@ -213,6 +213,83 @@ class RelayClientTest { assertEquals("Bearer tok-2", retried.getHeader("Authorization")) } + /** + * A spent proof is not a token problem. The request that spent one is often + * not a request this code made: OkHttp retries across a connection failure + * with the headers it already built, which is exactly what a phone moving + * between wi-fi and mobile data produces. Minting a replacement token for + * that puts a whole round trip in front of whatever was happening, and + * during a call that is in front of the signalling. + */ + @Test + fun `a spent proof is signed again and costs no new token`() { + val (client, _) = authedClient() + server.enqueue( + MockResponse() + .setResponseCode(401) + .setHeader("Content-Type", "application/json") + .setBody( + """{"code":"proof_replayed","message":"proof-of-possession signature """ + + """already used; sign the request again, the token is fine"}""", + ), + ) + server.enqueue(json("""{"size":4}""")) + + assertEquals(4, client.logHead().getInt("size")) + + next() // the first mint + val rejected = next() + val retried = next() + // No mint between them: three requests in total, not four. + assertEquals(3, server.requestCount) + assertEquals("Bearer tok-1", rejected.getHeader("Authorization")) + assertEquals("Bearer tok-1", retried.getHeader("Authorization")) + // The proof itself must be new, or the relay refuses the retry too. + assertNotEquals( + rejected.getHeader("X-FN-Signature"), + retried.getHeader("X-FN-Signature"), + ) + } + + /** A relay too old to name the code still says so in words. */ + @Test + fun `an older relay is understood from its wording`() { + val (client, _) = authedClient() + server.enqueue( + MockResponse() + .setResponseCode(401) + .setHeader("Content-Type", "application/json") + .setBody( + """{"code":"unauthorized","message":"proof-of-possession """ + + """signature already used"}""", + ), + ) + server.enqueue(json("""{"size":4}""")) + + assertEquals(4, client.logHead().getInt("size")) + assertEquals(3, server.requestCount) + } + + /** Any OTHER 401 still buys the token retry it always had. */ + @Test + fun `an expired token is still replaced`() { + val (client, _) = authedClient() + server.enqueue( + MockResponse() + .setResponseCode(401) + .setHeader("Content-Type", "application/json") + .setBody("""{"code":"unauthorized","message":"token expired"}"""), + ) + server.enqueue(json("""{"token":"tok-2","expires_in":300}""")) + server.enqueue(json("""{"size":4}""")) + + assertEquals(4, client.logHead().getInt("size")) + next() // the first mint + next() // the rejected call + assertEquals("/account/token", next().path) + assertEquals("Bearer tok-2", next().getHeader("Authorization")) + } + @Test fun `a persistent 401 gives up rather than looping`() { val (client, _) = authedClient()