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) + } + } +}