Merge branch 'feat/signup-refusal-messages' into 'main'

Read the relay's refusal code, not just the HTTP status

See merge request paulhitt/flamenet-messenger-android!9

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

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

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("<html>a proxy wrote this</html>")
+        )
+        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)
+        }
+    }
+}