extant2000/flamenet-messenger-android
1
0
Fork 0
Code Issues 7 Pull Requests Actions Packages Projects Releases Wiki Activity

Keep the token when only the proof was spent, and review keys at the call

Browse Source 
Two things the iPhone got today and this did not.

A proof of possession is single use, and the request that spends one is
frequently 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, and now says so with its own code rather than a
bare "unauthorized". Reading that as a token problem meant discarding a
working token and minting another before carrying on, and during a call
that lands in front of the signalling. The request is signed again
either way; only the token is kept now. A relay too old to name the code
still says so in words, so this works against production as it stands.

And ios#73. A call refused by the trust gate already named the reason
and already offered a button. The button opened the conversation, which
is the same gap one screen along: the reason the conversation banner
does not cover this case is that a call placed from the Calls tab, or
answered from a notification, never passes through a conversation.
Sending somebody to one to find out why their call failed is the
complaint rather than the cure.

So the review is a dialog at the failure. It carries both readings the
chat banner carries, the ordinary one and the one that matters, and it
puts the safety number on screen rather than behind another tap, because
comparing the number is what actually settles the question.

The old test said accepting must not be offered here, on the argument
that a hurried decision is better made elsewhere. The issue argues the
other way and is right: an extra screen in front of the same tap slows
nobody down, it just costs somebody the explanation. The requirement
moves rather than disappears, from "no accept button" to "no accept
button without the sentence saying what accepting costs".

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Paul Hitt
2026-09-10 09:51:55 -04:00
co-authored by Claude Opus 5
parent 6dd8633b08
commit d735b79b86
5 changed files with 290 additions and 41 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/src/main/kotlin/io/flamenet/messenger/app/MainActivity.kt
+94 -10
View File
@@ -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",
)
}
},
)
}
app/src/main/kotlin/io/flamenet/messenger/app/MessengerViewModel.kt
+35 -7
View File
@@ -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<Long, String>? = 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),
),
)
}
}
app/src/test/kotlin/io/flamenet/messenger/app/CallKeyReviewTest.kt
+62 -22
View File
@@ -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<Long, String>? = 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 "),
)
}
}
client/src/main/kotlin/io/flamenet/client/RelayClient.kt
+22 -2
View File
@@ -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,
client/src/test/kotlin/io/flamenet/client/RelayClientTest.kt
+77
View File
@@ -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()