Read the relay's refusal code, not just the HTTP status #36
@@ -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
|
||||
|
||||
@@ -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()
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user