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 75e50c8..c2d73ce 100644 --- a/app/src/main/kotlin/io/flamenet/messenger/app/MessengerViewModel.kt +++ b/app/src/main/kotlin/io/flamenet/messenger/app/MessengerViewModel.kt @@ -4,6 +4,7 @@ import android.app.Application import androidx.lifecycle.AndroidViewModel import androidx.lifecycle.viewModelScope import io.flamenet.client.ChainRpc +import io.flamenet.client.IdentityChangedException import io.flamenet.client.DeviceClock import io.flamenet.client.AccountsClient import io.flamenet.client.RelayClient @@ -2214,6 +2215,33 @@ class MessengerViewModel(app: Application) : AndroidViewModel(app) { // WebRTC callback thread it is raised from. viewModelScope.launch(Dispatchers.IO) { runCatching { session?.sendCallSignal(peerId, signal) } + // 🔴 **This was `runCatching { }` and nothing else, so + // every failure here vanished.** The one that matters + // is a changed key: the send path REFUSES to encrypt to + // an unreviewed identity — TOFU working exactly as + // intended — and the exception was swallowed, so the + // offer never left and the screen sat in DIALLING until + // it timed out. Reported as "calls wouldn't connect and + // it wasn't obvious why", which is precisely right. + // + // ⚠️ The fix is one tap and it is on a screen the + // person is not looking at: the accept-the-new-key + // banner lives in the CHAT, and somebody calling from + // the Calls tab never sees it. So say where to go. + .onFailure { failure -> + if (failure is IdentityChangedException) { + calls?.hangUp(notifyPeer = false) + update { + it.copy( + call = null, + error = "Their security code changed, so the call " + + "was not placed. Open the conversation with " + + "${nameFor(it, peerId)} to review it, then " + + "call again.", + ) + } + } + } } }, onStateChanged = { state -> @@ -3511,86 +3539,16 @@ class MessengerViewModel(app: Application) : AndroidViewModel(app) { * service answers them identically on purpose, and a message here that * distinguishes them would undo that at the last step. */ - private fun describe(failure: Throwable): String = when (failure) { - is AccountsClient.Failure.BadCredentials -> "That handle or passphrase isn't right." - is AccountsClient.Failure.SessionExpired -> - "Your session expired. Sign out and back in to keep going." + private fun describe(failure: Throwable): String = describeFailure(failure) - is AccountsClient.Failure.HandleTaken -> "That handle is already taken." - // ⚠️ Each of these says what to DO. A refusal somebody cannot act on is - // the same as no explanation: they retry the same thing and are refused - // again. "Too similar" without "changing a digit won't help" sends - // people round that loop deliberately. - is AccountsClient.Failure.EmailTaken -> - "An account with that email already exists. Try signing in instead." - is AccountsClient.Failure.HandleConfusable -> - "That handle looks too much like one that already exists. " + - "Changing a digit won't help — try a different word." - is AccountsClient.Failure.DisposableEmail -> - "That email provider isn't accepted. Use an address you'll still be " + - "able to read later — it's how you get back in." - is AccountsClient.Failure.BadHandle -> - "Handles can only contain lowercase letters and numbers." - is AccountsClient.Failure.BadEmail -> "That doesn't look like a valid email address." - is AccountsClient.Failure.WeakPassword -> - "Your passphrase needs to be at least 8 characters." - is AccountsClient.Failure.RateLimited -> "Too many attempts. Wait a few minutes." - // ⚠️ Not the underlying message: at this layer it is a Java networking - // string ("Failed to connect to /10.0.2.2:443"), which names an address - // the person never typed. - is AccountsClient.Failure.Transport -> - "Couldn't reach that server. Check the address and your connection." - // ⚠️ NOT "that passphrase is wrong". The relay rejects a TOKEN, which - // the user never typed and cannot check. Telling them their passphrase - // is wrong when their session merely aged out sends them off to change - // a password that was fine — observed on a send ten hours after a - // successful sign-in. - is RelayException.Unauthorized -> - "Your session expired. Sign out and back in to keep going." - is RelayException.NotFound -> "This server doesn't look like a Flamenet relay." - // The relay now says WHEN (Retry-After); a refusal with a clock is - // the difference between a person who waits and one who hammers. - is RelayException.RateLimited -> failure.retryAfterSeconds?.let { s -> - val mins = ((s + 59) / 60).coerceAtLeast(1) - "Too many attempts. Try again in about $mins minute" + - if (mins == 1) "." else "s." - } ?: "Too many attempts. Wait a few minutes." - is Vault.Corrupt -> "This phone's stored keys could not be read." - - // 🔴 **Anything not named above used to reach the screen verbatim.** - // `RelayException.Http` carries "relay returned 409: {body}" as its - // message, so somebody choosing a handle that was already in use was - // shown a status code and a JSON fragment. Nothing in that tells them - // to pick a different name, which is the entire content of the answer. - // - // ⚠️ The body is deliberately dropped rather than appended. It is - // written for whoever is reading the logs, and half of these carry the - // handle back in it. - is RelayException.Http -> when (failure.status) { - 409 -> "That handle is already taken. Try a different one." - 400, 422 -> "That server couldn't accept those details. " + - "Check the handle and try again." - // 🔴 **Say what the relay said.** A 403 is a rule refusing this - // specific thing, and the relay writes a sentence that names it - // and what to do — "this account already has 3 registered devices, - // remove one from the Devices screen". "That isn't allowed on this - // server" replaces an answer somebody can act on with one they - // cannot, and leaves them guessing at a rule nobody stated. - // - // ⚠️ Only when it really is a sentence. `body` falls back to the - // raw payload when there is no `message`, so anything that looks - // like JSON or a bare code keeps the generic line rather than - // putting a fragment on screen. - 403 -> failure.body - .takeIf { it.isNotBlank() && !it.trimStart().startsWith("{") && " " in it } - ?: "That isn't allowed on this server." - in 500..599 -> "That server is having trouble right now. " + - "Try again in a moment." - else -> "That server couldn't complete this. Try again in a moment." - } - is RelayException.Malformed -> "That server sent back something unexpected." - else -> "Something went wrong reaching that server." - } + /** + * The failure-to-sentence mapping, as a pure function of its argument. + * + * ⚠️ Lifted out of the view model so it can be tested without an + * `Application`. It never touched instance state — it reads only the + * throwable — so this is a move, not a rewrite, and the alternative was + * a Robolectric run to assert a string. + */ /** * ⚠️ A recorder outlives the view model unless something stops it, and a @@ -3632,3 +3590,93 @@ object FileLevels { LOCAL_LEVELS.firstOrNull { it.first == classification }?.second ?: "Level $classification" } + +internal fun describeFailure(failure: Throwable): String = when (failure) { + is AccountsClient.Failure.BadCredentials -> "That handle or passphrase isn't right." + is AccountsClient.Failure.SessionExpired -> + "Your session expired. Sign out and back in to keep going." + + is AccountsClient.Failure.HandleTaken -> "That handle is already taken." + // ⚠️ Each of these says what to DO. A refusal somebody cannot act on is + // the same as no explanation: they retry the same thing and are refused + // again. "Too similar" without "changing a digit won't help" sends + // people round that loop deliberately. + is AccountsClient.Failure.EmailTaken -> + "An account with that email already exists. Try signing in instead." + is AccountsClient.Failure.HandleConfusable -> + "That handle looks too much like one that already exists. " + + "Changing a digit won't help — try a different word." + is AccountsClient.Failure.DisposableEmail -> + "That email provider isn't accepted. Use an address you'll still be " + + "able to read later — it's how you get back in." + is AccountsClient.Failure.BadHandle -> + "Handles can only contain lowercase letters and numbers." + is AccountsClient.Failure.BadEmail -> "That doesn't look like a valid email address." + is AccountsClient.Failure.WeakPassword -> + "Your passphrase needs to be at least 8 characters." + is AccountsClient.Failure.RateLimited -> "Too many attempts. Wait a few minutes." + // ⚠️ Not the underlying message: at this layer it is a Java networking + // string ("Failed to connect to /10.0.2.2:443"), which names an address + // the person never typed. + is AccountsClient.Failure.Transport -> + "Couldn't reach that server. Check the address and your connection." + // ⚠️ NOT "that passphrase is wrong". The relay rejects a TOKEN, which + // the user never typed and cannot check. Telling them their passphrase + // is wrong when their session merely aged out sends them off to change + // a password that was fine — observed on a send ten hours after a + // successful sign-in. + is RelayException.Unauthorized -> + "Your session expired. Sign out and back in to keep going." + is RelayException.NotFound -> "This server doesn't look like a Flamenet relay." + // The relay now says WHEN (Retry-After); a refusal with a clock is + // the difference between a person who waits and one who hammers. + is RelayException.RateLimited -> failure.retryAfterSeconds?.let { s -> + val mins = ((s + 59) / 60).coerceAtLeast(1) + "Too many attempts. Try again in about $mins minute" + + if (mins == 1) "." else "s." + } ?: "Too many attempts. Wait a few minutes." + // 🔴 **Named, because the generic fallback sent people to the network.** + // "Something went wrong reaching that server" is what this used to + // produce for a changed key, and the server is fine — the refusal is + // this phone declining to encrypt to an identity nobody has reviewed. + // iOS names it ("Their security code changed"); this said nothing + // anywhere, in the call path or the message path. + is IdentityChangedException -> + "Their security code changed. Open the conversation to review it " + + "before sending anything else." + is Vault.Corrupt -> "This phone's stored keys could not be read." + + // 🔴 **Anything not named above used to reach the screen verbatim.** + // `RelayException.Http` carries "relay returned 409: {body}" as its + // message, so somebody choosing a handle that was already in use was + // shown a status code and a JSON fragment. Nothing in that tells them + // to pick a different name, which is the entire content of the answer. + // + // ⚠️ The body is deliberately dropped rather than appended. It is + // written for whoever is reading the logs, and half of these carry the + // handle back in it. + is RelayException.Http -> when (failure.status) { + 409 -> "That handle is already taken. Try a different one." + 400, 422 -> "That server couldn't accept those details. " + + "Check the handle and try again." + // 🔴 **Say what the relay said.** A 403 is a rule refusing this + // specific thing, and the relay writes a sentence that names it + // and what to do — "this account already has 3 registered devices, + // remove one from the Devices screen". "That isn't allowed on this + // server" replaces an answer somebody can act on with one they + // cannot, and leaves them guessing at a rule nobody stated. + // + // ⚠️ Only when it really is a sentence. `body` falls back to the + // raw payload when there is no `message`, so anything that looks + // like JSON or a bare code keeps the generic line rather than + // putting a fragment on screen. + 403 -> failure.body + .takeIf { it.isNotBlank() && !it.trimStart().startsWith("{") && " " in it } + ?: "That isn't allowed on this server." + in 500..599 -> "That server is having trouble right now. " + + "Try again in a moment." + else -> "That server couldn't complete this. Try again in a moment." + } + is RelayException.Malformed -> "That server sent back something unexpected." + else -> "Something went wrong reaching that server." +} diff --git a/app/src/test/kotlin/io/flamenet/messenger/app/ChangedKeyIsExplainedTest.kt b/app/src/test/kotlin/io/flamenet/messenger/app/ChangedKeyIsExplainedTest.kt new file mode 100644 index 0000000..81a8584 --- /dev/null +++ b/app/src/test/kotlin/io/flamenet/messenger/app/ChangedKeyIsExplainedTest.kt @@ -0,0 +1,64 @@ +package io.flamenet.messenger.app + +import io.flamenet.client.IdentityChangedException +import kotlin.test.Test +import kotlin.test.assertTrue + +/** + * A refused-because-the-key-changed failure has to say so (android#20). + * + * 🔴 **Reported from a real session: "the calls wouldn't connect because the + * keys were changed. it wasn't obvious why the call failed."** It was not + * obvious because nothing said it. The call's signalling lambda was + * `runCatching { … }` with no `onFailure`, so `IdentityChangedException` — the + * send path refusing to encrypt to an unreviewed identity, which is TOFU + * working — vanished. The offer never left and the screen sat in DIALLING. + * + * ⚠️ **And the fix is on a screen the caller is not looking at.** Accepting a + * changed key is a banner in the CHAT; somebody calling from the Calls tab + * never sees it. So the message has to name the conversation, not just the + * problem. + * + * ⚠️ iOS has said "Their security code changed" on this path since it was + * built. This is a parity gap, not a new idea — which is why the assertion is + * about the substance of the sentence rather than its exact wording. + */ +class ChangedKeyIsExplainedTest { + + private val message = describeFailure(IdentityChangedException(7L)) + + @Test + fun `it names the security code rather than the network`() { + assertTrue( + message.contains("security code", ignoreCase = true), + "a changed key was described as \"$message\" — the person is sent " + + "hunting a network problem that does not exist", + ) + } + + /** + * 🔴 **The actionable half.** A refusal somebody cannot act on is the same + * as no explanation: they retry the same thing and are refused again. This + * file's neighbours in `describe()` all say what to DO, and that rule is + * written down there — this one has to keep it. + */ + @Test + fun `it says where to go and what to do`() { + assertTrue( + message.contains("conversation", ignoreCase = true) && + message.contains("review", ignoreCase = true), + "\"$message\" does not point at the conversation, which is the only " + + "place the key can be accepted", + ) + } + + /** ⚠️ Not the generic fallback, which is what this used to produce. */ + @Test + fun `it is not the generic something-went-wrong line`() { + assertTrue( + !message.contains("Something went wrong", ignoreCase = true) && + !message.contains("reaching that server", ignoreCase = true), + "a changed key still falls through to the generic server line: \"$message\"", + ) + } +}