Support registration & login with phone number by dbkr · Pull Request #1971 · matrix-org/synapse

dbkr · 2017-03-07T17:03:22Z

Adds the msisdn form of the proxy requestToken APIs
Changes the call to the ID servers to use JSON rather than x-www-form-urlencoded
Adds support for binding msisdns at registration time
Adds support for m.login.msisdn UI auth type and offers flows that use it in the registration API

defer.returnValue doth not maketh a generator: it would need a yield to be a generator, and this doesn't need a yield.

and support binding them with the bind_msisdn param

https://docs.google.com/document/d/1-6ZSSW5YvCGhVFDyD2QExAUAdpCWjccvJT5xiyTTG2Y/edit#

richvdh · 2017-03-08T09:26:32Z

FTR: this implements https://docs.google.com/document/d/1-6ZSSW5YvCGhVFDyD2QExAUAdpCWjccvJT5xiyTTG2Y/edit

richvdh · 2017-03-08T09:28:19Z

+        defer.returnValue(True)
+
+    @defer.inlineCallbacks
+    def _check_threepid(self, medium, authdict, ):


random trailing ,?

richvdh · 2017-03-08T09:55:33Z

+        del submission["medium"]
+        del submission["address"]
+
+    return submission


since you're modifying the input, I'd suggest not returning it, to make that clear.

richvdh · 2017-03-08T09:56:24Z

+        if "type" not in identifier:
+            raise SynapseError(400, "Login identifier has no type")
+
+        # convert phone type identifiers to geberic threepids


richvdh · 2017-03-08T10:07:25Z

+            raise SynapseError(400, "Unable to parse phone number")
+        msisdn = phonenumbers.format_number(
+            phoneNumber, phonenumbers.PhoneNumberFormat.E164
+        )[1:]


let's factor out the (phone_number, country) -> msisdn_or_throw_400 conversion to a utility, since it appears in several places.

richvdh · 2017-03-08T10:10:47Z

+        if existingUid is None:
+            raise SynapseError(400, "MSISDN not found", Codes.THREEPID_NOT_FOUND)
+
+        ret = yield self.identity_handler.requestEmailToken(**body)


should be requestMsisdnToken?

richvdh · 2017-03-08T10:20:32Z

+                absent.append(k)
+
+        if len(absent) > 0:
+            raise SynapseError(400, "Missing params: %r" % absent, Codes.MISSING_PARAM)


could we not have an assert_params_in_request(body, required) utility rather than copying this pattern twenty times?

suggest sticking it alongside parse_json_object_from_request in servlet.py

richvdh · 2017-03-08T10:21:51Z

@@ -226,6 +281,7 @@ def on_POST(self, request):
            )
            # don't re-register the email address


s/email address/threepids/

richvdh · 2017-03-08T10:22:19Z

            logger.info("bind_email not specified: not binding email")

+    @defer.inlineCallbacks
+    def _register_msisdn_threepid(self, user_id, threepid, token, bind_msisdn):


richvdh · 2017-03-08T10:26:07Z

+            defer.Deferred:
+        """
+        reqd = ('medium', 'address', 'validated_at')
+        if any(x not in threepid for x in reqd):


when would this happen?

This would have to be the ID server giving an invalid response

comment would help, then

erikjohnston

I've only looked at the synapse/python part of it, not the functionality. Looks mostly good, other than factoring a few bits out to reduce code duplication

erikjohnston · 2017-03-08T10:10:51Z

+    @defer.inlineCallbacks
+    def _check_dummy_auth(self, authdict, _):
+        yield run_on_reactor()
+        defer.returnValue(True)


Why do we do wait a reactor tick?

Because these functions are all specified to return a deferred, so we need to yield somewhere in the function for it to actually return a deferred.

you could do that with return success(True).

A better answer might be: because it did before we got here, and changing it is scary.

erikjohnston · 2017-03-08T10:11:47Z

+
        identity_handler = self.hs.get_handlers().identity_handler

        logger.info("Getting validated threepid. threepidcreds: %r" % (threepid_creds,))


I know this isn't you, but this should be logger.info("..", threepid_creds)

It probably was me in a former life

erikjohnston · 2017-03-08T10:13:05Z

+    """
+    Convert a phone login identifier type to a generic threepid identifier
+    Args:
+        identifier: Login identifier dict of type 'm.id.phone'


If you're listing args please add type, e.g. identifier(str): ...

erikjohnston · 2017-03-08T10:16:16Z

+                absent.append(k)
+
+        if absent:
+            raise SynapseError(400, "Missing params: %r" % absent, Codes.MISSING_PARAM)


This checking of required keys should probably be factored out as its shared between at least this and the email one

erikjohnston · 2017-03-08T10:17:34Z

+            raise SynapseError(400, "Unable to parse phone number")
+        msisdn = phonenumbers.format_number(
+            phoneNumber, phonenumbers.PhoneNumberFormat.E164
+        )[1:]


Is this the same block of code as in login_id_thirdparty_from_phone? Its probably worth factoring out as a) DRY and b) it'll probably be easier to see whats going on in this function

erikjohnston · 2017-03-08T10:18:43Z

+            phoneNumber, phonenumbers.PhoneNumberFormat.E164
+        )[1:]
+
+        existingUid = yield self.hs.get_datastore().get_user_id_by_threepid(


We should be avoiding using self.hs.get_*() outside of initialisers and instead pull out all the dependencies from the HS we need in the initialiser

erikjohnston · 2017-03-08T10:20:30Z

+        """
+        reqd = ('medium', 'address', 'validated_at')
+        if any(x not in threepid for x in reqd):
+            logger.info("Can't add incomplete 3pid")


This will likely fail as it doesn't have a parameter? If you just want to return None you can actually just do return

can you? really? I thought it was naughty to have yield and return in the same function.

ah, you can return without a value.

erikjohnston · 2017-03-08T10:21:18Z

+        if bind_msisdn:
+            logger.info("bind_msisdn specified: binding")
+            logger.debug("Binding msisdn %s to %s" % (
+                threepid, user_id


What's the reasoning for not just having this as one info log line?

hmm, it probably used to be longer. fixed.

I assumed it was something to do with not wanting to put the email/msisdn in the log.

erikjohnston · 2017-03-08T10:31:05Z

+            raise SynapseError(400, "Unable to parse phone number")
+        msisdn = phonenumbers.format_number(
+            phoneNumber, phonenumbers.PhoneNumberFormat.E164
+        )[1:]


Yeah, this would be much nicer to read if factored out a bit:

def on_POST(self, request): body = parse_json_object_from_request(request) check_required_keys(request, ('id_server', 'client_secret', ..)) msisdn = format_msisdn(body['phone_number'], body['country']) existingUid = yield self.store.get_user_id_by_threepid('msisdn', msisdn) if existingUid: raise ret = yield self.identity_handler.requestEmailToken(**body) defer.returnValue((200, ret))

Plus a couple of other minor fixes

and replace requestEmailToken where we meant requestMsisdnToken

dbkr · 2017-03-08T13:24:40Z

@erikjohnston @richvdh ptal

richvdh · 2017-03-08T14:56:40Z

+    If the input login submission is an old style object
+    (ie. with top-level user / medium / address) convert it
+    to a typed object.
+    Returns: Typed-object style login identifier


richvdh · 2017-03-08T14:57:50Z

@@ -0,0 +1,28 @@
+# -*- coding: utf-8 -*-
+# Copyright 2015, 2016 OpenMarket Ltd


technically not

richvdh · 2017-03-08T14:58:43Z

+from synapse.api.errors import SynapseError
+
+
+def phone_number_to_msisdn(country, number):


can haz doc pls kthxbye

richvdh · 2017-03-08T15:01:49Z

+            defer.Deferred:
+        """
+        reqd = ('medium', 'address', 'validated_at')
+        if any(x not in threepid for x in reqd):


comment would help, then

This file post-dates OM

richvdh

lgtm

Changes from #1971

dbkr added 5 commits February 14, 2017 15:05

WIP support for msisdn 3pid proxy methods

ce3e583

Merge remote-tracking branch 'origin/develop' into dbkr/msisdn_signin

3557cf3

Just return the deferred straight off

ad882cd

defer.returnValue doth not maketh a generator: it would need a yield to be a generator, and this doesn't need a yield.

Add msisdns as 3pids during registration

b0effa2

and support binding them with the bind_msisdn param

Support new login format

00466e2

https://docs.google.com/document/d/1-6ZSSW5YvCGhVFDyD2QExAUAdpCWjccvJT5xiyTTG2Y/edit#

dbkr assigned richvdh Mar 7, 2017

dbkr requested review from erikjohnston and richvdh March 7, 2017 17:03

dbkr mentioned this pull request Mar 7, 2017

Support registration & login with phone number matrix-org/matrix-react-sdk#742

Merged

Fix pep8

402a7bf

richvdh suggested changes Mar 8, 2017

View reviewed changes

richvdh assigned dbkr and unassigned richvdh Mar 8, 2017

erikjohnston reviewed Mar 8, 2017

View reviewed changes

dbkr added 8 commits March 8, 2017 11:03

Factor out msisdn canonicalisation

88df6c0

Plus a couple of other minor fixes

Refector out assert_params_in_request

2e27339

and replace requestEmailToken where we meant requestMsisdnToken

Typos

82c5e7d

Fix log line

0e0aee2

Minor fixes from PR feedback

65d43f3

Pull out datastore in initialiser

85bb322

Add msisdn util file

a9e2b9e

pep8

1c99934

dbkr assigned richvdh and erikjohnston and unassigned dbkr Mar 8, 2017

richvdh reviewed Mar 8, 2017

View reviewed changes

richvdh unassigned erikjohnston Mar 8, 2017

richvdh assigned dbkr and erikjohnston and unassigned richvdh Mar 8, 2017

dbkr added 5 commits March 8, 2017 17:01

Better error message

d4d3629

Not any more, it doesn't

727124a

Incorrectly copied copyright

3edc572

This file post-dates OM

Docs

9d0d40f

Comment when our 3pids would be incomplete

ece7e00

dbkr assigned richvdh and unassigned dbkr Mar 8, 2017

richvdh approved these changes Mar 8, 2017

View reviewed changes

dbkr merged commit 663396e into develop Mar 9, 2017

erikjohnston mentioned this pull request Mar 13, 2017

Revert "Support registration & login with phone number" #1988

Merged

dbkr added a commit that referenced this pull request Mar 13, 2017

Support registration / login with phone number

73a5f06

Changes from #1971

dbkr mentioned this pull request Mar 13, 2017

Phone number registration / login support v2 #1994

Merged

dbkr deleted the dbkr/msisdn_signin branch October 17, 2017 12:48

		@@ -226,6 +281,7 @@ def on_POST(self, request):
		)
		# don't re-register the email address


		identity_handler = self.hs.get_handlers().identity_handler

		logger.info("Getting validated threepid. threepidcreds: %r" % (threepid_creds,))

		@@ -0,0 +1,28 @@
		# -- coding: utf-8 --
		# Copyright 2015, 2016 OpenMarket Ltd

		from synapse.api.errors import SynapseError


		def phone_number_to_msisdn(country, number):

Uh oh!

Conversation

dbkr commented Mar 7, 2017

Uh oh!

richvdh commented Mar 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikjohnston left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment