From 8714ff6d515ab0fb937518d4ba7d1d3b1593617d Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 10 May 2019 11:09:53 +0100 Subject: [PATCH 1/8] Add a DUMMY stage to captcha-only registration flow This allows the client to complete the email last which is more natual for the user. Without this stage, if the client would complete the recaptcha (and terms, if enabled) stages and then the registration request would complete because you've now completed a flow, even if you were intending to complete the flow that's the same except has email auth at the end. Adding a dummy auth stage to the recaptcha-only flow means it's always unambiguous which flow the client was trying to complete. Longer term we should think about changing the protocol so the client explicitly says which flow it's trying to complete. vector-im/riot-web#9586 --- synapse/rest/client/v2_alpha/register.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py index dc3e265bcd..95578b76ae 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -345,7 +345,7 @@ class RegisterRestServlet(RestServlet): if self.hs.config.enable_registration_captcha: # only support 3PIDless registration if no 3PIDs are required if not require_email and not require_msisdn: - flows.extend([[LoginType.RECAPTCHA]]) + flows.extend([[LoginType.RECAPTCHA, LoginType.DUMMY]]) # only support the email-only flow if we don't require MSISDN 3PIDs if not require_msisdn: flows.extend([[LoginType.EMAIL_IDENTITY, LoginType.RECAPTCHA]]) From a18f93279eb7b8505db63af062695e8c7fe263c4 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 10 May 2019 11:11:59 +0100 Subject: [PATCH 2/8] Add changelog entry --- changelog.d/5174.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/5174.bugfix diff --git a/changelog.d/5174.bugfix b/changelog.d/5174.bugfix new file mode 100644 index 0000000000..f4d3192bd9 --- /dev/null +++ b/changelog.d/5174.bugfix @@ -0,0 +1 @@ +Allow clients to complete email auth first when registering From 9c61dce3c81283f9c7bb47b634512dd848eb116e Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 10 May 2019 11:14:55 +0100 Subject: [PATCH 3/8] Comment --- synapse/rest/client/v2_alpha/register.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py index 95578b76ae..5c4c8dca28 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -345,6 +345,10 @@ class RegisterRestServlet(RestServlet): if self.hs.config.enable_registration_captcha: # only support 3PIDless registration if no 3PIDs are required if not require_email and not require_msisdn: + # Also add a dummy flow here, otherwise if a client completes + # recaptcha first we'll assume they were going for this flow + # and complete the request, when they could have been trying to + # complete one of the flows with email/msisdn auth. flows.extend([[LoginType.RECAPTCHA, LoginType.DUMMY]]) # only support the email-only flow if we don't require MSISDN 3PIDs if not require_msisdn: From 7a3eb8657d66bf0ae479b5a0db9529df5d0226c1 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 10 May 2019 11:18:35 +0100 Subject: [PATCH 4/8] Thanks, automated grammar pedantry. --- changelog.d/5174.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/5174.bugfix b/changelog.d/5174.bugfix index f4d3192bd9..2d7c71739b 100644 --- a/changelog.d/5174.bugfix +++ b/changelog.d/5174.bugfix @@ -1 +1 @@ -Allow clients to complete email auth first when registering +Allow clients to complete email auth first when registering. From 04299132af8dee4047a28f9d92ccd7ba2d1c10a0 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 10 May 2019 13:58:03 +0100 Subject: [PATCH 5/8] Re-order flows so that email auth is done last It's more natural for the user if the bit that takes them away from the registration flow comes last. Adding the dummy stage allows us to do the stages in this order without the ambiguity. --- synapse/rest/client/v2_alpha/register.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py index 5c4c8dca28..c51241fa01 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -352,15 +352,15 @@ class RegisterRestServlet(RestServlet): flows.extend([[LoginType.RECAPTCHA, LoginType.DUMMY]]) # only support the email-only flow if we don't require MSISDN 3PIDs if not require_msisdn: - flows.extend([[LoginType.EMAIL_IDENTITY, LoginType.RECAPTCHA]]) + flows.extend([[LoginType.RECAPTCHA, LoginType.EMAIL_IDENTITY]]) if show_msisdn: # only support the MSISDN-only flow if we don't require email 3PIDs if not require_email: - flows.extend([[LoginType.MSISDN, LoginType.RECAPTCHA]]) + flows.extend([[LoginType.RECAPTCHA, LoginType.MSISDN]]) # always let users provide both MSISDN & email flows.extend([ - [LoginType.MSISDN, LoginType.EMAIL_IDENTITY, LoginType.RECAPTCHA], + [LoginType.RECAPTCHA, LoginType.MSISDN, LoginType.EMAIL_IDENTITY], ]) else: # only support 3PIDless registration if no 3PIDs are required @@ -383,7 +383,15 @@ class RegisterRestServlet(RestServlet): if self.hs.config.user_consent_at_registration: new_flows = [] for flow in flows: - flow.append(LoginType.TERMS) + inserted = False + # m.login.terms should go near the end but before msisdn or email auth + for i, stage in enumerate(flow): + if stage == LoginType.EMAIL_IDENTITY or stage == LoginType.MSISDN: + flow.insert(i, LoginType.TERMS) + inserted = True + break + if not inserted: + flow.append(LoginType.TERMS) flows.extend(new_flows) auth_result, params, session_id = yield self.auth_handler.check_auth( From c9f811c5d45ac52d4b55ed74d2919b0d41790983 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 10 May 2019 14:01:19 +0100 Subject: [PATCH 6/8] Update changelog --- changelog.d/5174.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/5174.bugfix b/changelog.d/5174.bugfix index 2d7c71739b..0f26d46b2c 100644 --- a/changelog.d/5174.bugfix +++ b/changelog.d/5174.bugfix @@ -1 +1 @@ -Allow clients to complete email auth first when registering. +Re-order stages in registration flows such that msisdn and email verification are done last. From 8782bfb783ef4da3882951e56d8911a1d81eada5 Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 13 May 2019 15:34:11 +0100 Subject: [PATCH 7/8] And now I realise why the test is failing... --- tests/rest/client/v2_alpha/test_auth.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/rest/client/v2_alpha/test_auth.py b/tests/rest/client/v2_alpha/test_auth.py index 0ca3c4657b..fa833ba5a2 100644 --- a/tests/rest/client/v2_alpha/test_auth.py +++ b/tests/rest/client/v2_alpha/test_auth.py @@ -92,7 +92,14 @@ class FallbackAuthTests(unittest.HomeserverTestCase): self.assertEqual(len(self.recaptcha_attempts), 1) self.assertEqual(self.recaptcha_attempts[0][0]["response"], "a") - # Now we have fufilled the recaptcha fallback step, we can then send a + # also complete the dummy auth + request, channel = self.make_request( + "POST", "register", {"auth": {"session": session, "type": "m.login.dummy"}} + ) + self.render(request) + + # Now we should have fufilled a complete auth flow, including + # the recaptcha fallback step, we can then send a # request to the register API with the session in the authdict. request, channel = self.make_request( "POST", "register", {"auth": {"session": session}} From 822072b1bb8c68006dffff196885202d80099361 Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 13 May 2019 16:10:26 +0100 Subject: [PATCH 8/8] Terms might not be the last stage --- tests/test_terms_auth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_terms_auth.py b/tests/test_terms_auth.py index f412985d2c..52739fbabc 100644 --- a/tests/test_terms_auth.py +++ b/tests/test_terms_auth.py @@ -59,7 +59,7 @@ class TermsTestCase(unittest.HomeserverTestCase): for flow in channel.json_body["flows"]: self.assertIsInstance(flow["stages"], list) self.assertTrue(len(flow["stages"]) > 0) - self.assertEquals(flow["stages"][-1], "m.login.terms") + self.assertTrue("m.login.terms" in flow["stages"]) expected_params = { "m.login.terms": {