From e5f4397b28fa8ccaf3cfa7be8f769386a5454892 Mon Sep 17 00:00:00 2001 From: Paul Hitt Date: Tue, 1 Sep 2026 20:48:11 -0400 Subject: [PATCH] Read the relay's refusal, not just the HTTP status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🔴 **A 409 is not always a taken handle, and this said it was.** The relay answers 409 for a taken handle, an already-registered email, AND — since flamenet-relay!11 — a handle too close to an existing one. Three problems with three different fixes, all reported as "that handle is already taken". Somebody whose EMAIL was already registered was told to pick a different handle. Doing so cannot work, and nothing anywhere would have told them why. That is a pre-existing bug, not something the new refusals introduced; adding a third meaning to 409 is what made it visible. 400 was worse. Every format refusal — a short passphrase, an unusable email address, a disallowed handle — became "the account service returned 400", which is not a sentence anybody can act on. The status is the transport's opinion. The `code` in the body is the relay's, and it has always been sent; nothing here ever read it. ⚠️ **Unknown codes still refuse.** A relay newer than the app is the normal case, so an unrecognised code falls back to the status rather than to success — an account the server never created would be a much worse failure than a vague message. ⚠️ **An unparseable body cannot throw.** A proxy returning HTML on a 409 must still be a refusal, not a crash. Five tests, proven able to fail: collapsing 409 back to a single meaning turns four of them red. All three Android suites pass. Pairs with flamenet-relay!11 and flamenet-messenger-ios!45. --- .../messenger/app/MessengerViewModel.kt | 17 +++ .../io/flamenet/client/AccountsClient.kt | 59 +++++++- .../client/SignupFailureMappingTest.kt | 129 ++++++++++++++++++ 3 files changed, 204 insertions(+), 1 deletion(-) create mode 100644 client/src/test/kotlin/io/flamenet/client/SignupFailureMappingTest.kt 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 620f1a1..ae1c1b3 100644 --- a/app/src/main/kotlin/io/flamenet/messenger/app/MessengerViewModel.kt +++ b/app/src/main/kotlin/io/flamenet/messenger/app/MessengerViewModel.kt @@ -2525,6 +2525,23 @@ class MessengerViewModel(app: Application) : AndroidViewModel(app) { "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 diff --git a/client/src/main/kotlin/io/flamenet/client/AccountsClient.kt b/client/src/main/kotlin/io/flamenet/client/AccountsClient.kt index 8253949..d780d35 100644 --- a/client/src/main/kotlin/io/flamenet/client/AccountsClient.kt +++ b/client/src/main/kotlin/io/flamenet/client/AccountsClient.kt @@ -81,6 +81,28 @@ class AccountsClient( */ object SessionExpired : Failure("your session has expired — sign in again") object HandleTaken : Failure("that handle is already taken") + + /** + * 🔴 **A 409 is not always a taken handle, and saying so was wrong.** + * The relay answers 409 for a taken handle, a registered email, AND a + * handle too close to an existing one — three different problems with + * three different fixes. Collapsing them told somebody whose EMAIL was + * already registered to pick a different handle, which cannot work and + * gives them no way to discover why. + * + * The status is the transport's opinion; the `code` in the body is the + * relay's, and it is the one that means something. + */ + object EmailTaken : Failure("an account with that email already exists") + + /** The handle folds onto an existing one — see the relay's + * `User.confusableSkeleton`. Changing a digit will not help. */ + object HandleConfusable : Failure("that handle is too easily mistaken for another") + + object DisposableEmail : Failure("that email provider isn't accepted") + object BadHandle : Failure("that handle isn't allowed") + object BadEmail : Failure("that doesn't look like an email address") + object WeakPassword : Failure("that passphrase is too short") object RateLimited : Failure("too many attempts — wait a few minutes") class Service(val status: Int) : Failure("the account service returned $status") class Transport(cause: String) : Failure("couldn't reach the account service: $cause") @@ -168,13 +190,48 @@ class AccountsClient( 401, 403 -> throw if (carriesCredentials) Failure.BadCredentials else Failure.SessionExpired - 409 -> throw Failure.HandleTaken + // 🔴 **Read the relay's own code, not just the status.** A + // 400 became a bare "the account service returned 400" and a + // 409 became "that handle is already taken" whatever the + // actual reason — so a weak passphrase, an unusable email and + // an already-registered address were all reported as something + // the person could not act on, or as the wrong problem + // entirely. The relay has always sent a specific code; nothing + // here ever read it. + 409, 400 -> throw failureFor(code(text), status = it.code) 429 -> throw Failure.RateLimited else -> throw Failure.Service(it.code) } } } + /** + * The relay's error code, or null when the body is not its error shape. + * + * ⚠️ Never throws. A body that cannot be parsed is a transport problem, not + * a reason to lose the status we already have. + */ + private fun code(body: String): String? = runCatching { + JSONObject(body).optString("code").takeIf { it.isNotEmpty() } + }.getOrNull() + + /** + * ⚠️ Falls back to the STATUS when the code is unknown, so a refusal this + * build has never heard of still reports as a refusal rather than as a + * success or a crash. A relay newer than the app is the normal case, not + * the exception. + */ + private fun failureFor(code: String?, status: Int): Failure = when (code) { + "username_taken" -> Failure.HandleTaken + "email_taken" -> Failure.EmailTaken + "username_confusable" -> Failure.HandleConfusable + "disposable_email" -> Failure.DisposableEmail + "bad_username" -> Failure.BadHandle + "bad_email" -> Failure.BadEmail + "weak_password" -> Failure.WeakPassword + else -> if (status == 409) Failure.HandleTaken else Failure.Service(status) + } + private companion object { val JSON = "application/json".toMediaType() } diff --git a/client/src/test/kotlin/io/flamenet/client/SignupFailureMappingTest.kt b/client/src/test/kotlin/io/flamenet/client/SignupFailureMappingTest.kt new file mode 100644 index 0000000..1a0eed1 --- /dev/null +++ b/client/src/test/kotlin/io/flamenet/client/SignupFailureMappingTest.kt @@ -0,0 +1,129 @@ +package io.flamenet.client + +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer +import kotlin.test.AfterTest +import kotlin.test.BeforeTest +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +/** + * What a refused sign-up is reported as. + * + * 🔴 **A 409 is not always a taken handle, and this used to say it was.** The + * relay answers 409 for a taken handle, an already-registered email, and a + * handle too close to an existing one — three problems with three different + * fixes. Collapsing them told somebody whose EMAIL was registered to pick a + * different handle, which cannot work and leaves them no way to find out why. + * + * 400 was worse: every format refusal became "the account service returned + * 400", which is not a sentence anybody can act on. + * + * The status is the transport's opinion. The `code` in the body is the relay's, + * and it is the one that means something. + */ +class SignupFailureMappingTest { + + private lateinit var server: MockWebServer + + @BeforeTest fun start() { + server = MockWebServer() + server.start() + } + + @AfterTest fun stop() { + server.shutdown() + } + + private fun client() = AccountsClient(server.url("/").toString()) + + /** Ask the server to refuse with a given code and status, and see what comes back. */ + private fun refusalFor(code: String, status: Int): AccountsClient.Failure { + server.enqueue( + MockResponse().setResponseCode(status) + .setHeader("Content-Type", "application/json") + .setBody("""{"code":"$code","message":"refused"}""") + ) + return try { + client().register("someone", "passphrase-long-enough") + throw AssertionError("the sign-up was not refused at all") + } catch (failure: AccountsClient.Failure) { + failure + } + } + + @Test + fun `each refusal code maps to its own failure`() { + val cases = listOf( + Triple("username_taken", 409, AccountsClient.Failure.HandleTaken), + Triple("email_taken", 409, AccountsClient.Failure.EmailTaken), + Triple("username_confusable", 409, AccountsClient.Failure.HandleConfusable), + Triple("disposable_email", 400, AccountsClient.Failure.DisposableEmail), + Triple("bad_username", 400, AccountsClient.Failure.BadHandle), + Triple("bad_email", 400, AccountsClient.Failure.BadEmail), + Triple("weak_password", 400, AccountsClient.Failure.WeakPassword), + ) + for ((code, status, expected) in cases) { + assertEquals(expected, refusalFor(code, status), "$code should map to $expected") + } + } + + /** + * 🔴 The regression this whole change exists for. An already-registered + * email must NOT be reported as a taken handle — the person would change + * their handle, be refused again, and have no way to work out why. + */ + @Test + fun `an already registered email is not reported as a taken handle`() { + val failure = refusalFor("email_taken", 409) + assertEquals(AccountsClient.Failure.EmailTaken, failure) + assertTrue( + failure.message!!.contains("email"), + "the message should mention the email, not the handle: ${failure.message}" + ) + } + + /** + * ⚠️ A relay newer than the app is the normal case, not the exception. An + * unknown code must still refuse — falling through to success would create + * an account the server never made. + */ + @Test + fun `an unknown refusal code still refuses`() { + assertEquals(AccountsClient.Failure.HandleTaken, refusalFor("invented_next_year", 409)) + val fourHundred = refusalFor("invented_next_year", 400) + assertTrue( + fourHundred is AccountsClient.Failure.Service, + "an unknown 400 should keep its status: $fourHundred" + ) + } + + /** ⚠️ A body that is not the relay's error shape must not throw a parse error. */ + @Test + fun `an unparseable body falls back to the status`() { + server.enqueue( + MockResponse().setResponseCode(409) + .setHeader("Content-Type", "text/html") + .setBody("a proxy wrote this") + ) + try { + client().register("someone", "passphrase-long-enough") + throw AssertionError("the sign-up was not refused") + } catch (failure: AccountsClient.Failure) { + assertEquals(AccountsClient.Failure.HandleTaken, failure) + } + } + + /** Rate limiting and auth keep their existing meanings. */ + @Test + fun `the other statuses are unchanged`() { + server.enqueue(MockResponse().setResponseCode(429)) + try { + client().register("someone", "passphrase-long-enough") + throw AssertionError("not refused") + } catch (failure: AccountsClient.Failure) { + assertEquals(AccountsClient.Failure.RateLimited, failure) + } + } +} -- 2.54.0