add registrations_require_3pid and allow_local_3pids #2813

ara4n · 2018-01-19T00:20:26Z

Adds allow_local_3pids: to let homeservers specify a whitelist for the format of 3PIDs that users are allowed to register with or add to their HS accounts.

It does not directly impact the 3PIDs which a user can bind to their account on their IS.

It also adds registrations_require_3pid, to determine which registration flows are advertised by the HS in case an admin wants to force users to specify a given 3PID when they sign up.

Typically useful for stopping people from registering with non-work emails.

lets homeservers specify a whitelist for 3PIDs that users are allowed to associate with. Typically useful for stopping people from registering with non-work emails

turt2live · 2018-01-19T00:35:36Z

synapse/config/registration.py

@@ -31,6 +31,7 @@ def read_config(self, config):
                strtobool(str(config["disable_registration"]))
            )

+        self.registrations_require_3pid = config.get("registrations_require_3pid", [])


This doesn't seem to affect just registrations and could be misleading to some people.

Edit for clarity: I'm talking about the config key name

yeah, i've asked @dbkr to clarify the impact on the other 3PID manipulations

this convo ended up in IRL as dave presumably missed this comment: the impact is:

There are three request tokens atm for verifying 3PIDs:

registration

password reset

adding a 3pid to your account for use in login (which in turn tries to go bind it on the IS if asked)

this PR restricts the format of all three with allowed_local_3pids

it does not limit IS bindings (other than in a weak manner given in practice the account/3pid api is used by clients today to trigger bindings)

dbkr

otherwise, yep, lgtm

dbkr · 2018-01-19T10:19:20Z

synapse/rest/client/v2_alpha/register.py

                ])

        auth_result, params, session_id = yield self.auth_handler.check_auth(
            flows, body, self.hs.get_ip_from_request(request)
        )

+        # doublecheck that we're not trying to register an denied 3pid.


dbkr · 2018-01-19T10:20:34Z

synapse/rest/client/v2_alpha/register.py

                ])

        auth_result, params, session_id = yield self.auth_handler.check_auth(
            flows, body, self.hs.get_ip_from_request(request)
        )

+        # doublecheck that we're not trying to register an denied 3pid.
+        # the user-facing checks should already have happened when we requested
+        # a 3PID token to validate them in /register/email/requestToken etc


Not necessarily, since there's nothing to mandate that the user had to proxy their requestToken via the HS, so this check is important.

erikjohnston · 2018-01-19T10:33:35Z

I'm confused what this is trying to do. Is this:

requiring that users register with a 3pid in a whitelist? or
stopping users from adding 3pids outside a whitelist?

erikjohnston · 2018-01-19T10:09:31Z

synapse/handlers/register.py

+                if (
+                    constraint['medium'] == 'email' and
+                    threepid['medium'] == 'email' and
+                    re.match(constraint['pattern'], threepid['address'])


Do you mean and not re.match?

erikjohnston · 2018-01-19T10:44:02Z

synapse/rest/client/v1/register.py

-                ]}
-            )
+                ])
+        return (200, {"flows": flows})


What does require_msisdn mean? We don't seem to add a stage for it.

Also I really can't follow whats going on here, there are two many if and nots and ands and ors. It seems that we're building up flows based on required stages, so something like the following might work better:

required_stages = [] # The stages that need to be in every flow if self.hs.config.enable_registration_captcha: required_stages.append(LoginType.RECAPTCHA) if require_email: required_stages.append(LoginType.EMAIL_IDENTITY) # ... etc ... # Now we work out what flows we need to offer. flows = [] # First we add the base flow with only the required stages: flows.append({"stages": required_stages}) # If we don't require email, then we add a flow to let people optionally specify it if not require_email: flows.append({"stages": required_stages + [LoginType.EMAIL_IDENTITY]})

erikjohnston · 2018-01-19T10:45:32Z

synapse/rest/client/v2_alpha/_base.py

+        for constraint in hs.config.registrations_require_3pid:
+            logger.debug("Checking 3PID %s (%s) against %s (%s)" % (
+                address, medium, constraint['pattern'], constraint['medium']
+            ))


Don't do string building, like most logging libraries it does it for you: logger.debug("foo: %s, bar: %s", foo, bar)

erikjohnston · 2018-01-19T10:47:56Z

synapse/rest/client/v2_alpha/_base.py

+    else:
+        allow = True
+
+    return allow


Nit: Normally I'd do the pattern of returning immediately rather than having an allow variable, e.g.:

def check_3pid_allowed(hs, medium, address): if not hs.config.registrations_require_3pid: return True for constraint in hs.config.registrations_require_3pid: if allowed: return True return False

erikjohnston · 2018-01-19T10:48:11Z

synapse/rest/client/v2_alpha/_base.py

@@ -60,6 +60,27 @@ def set_timeline_upper_limit(filter_json, filter_timeline_limit):
            filter_timeline_limit)


+def check_3pid_allowed(hs, medium, address):
+    # check whether the HS has whitelisted the given 3PID


Proper docstring please.

erikjohnston · 2018-01-19T10:48:38Z

synapse/rest/client/v2_alpha/account.py

@@ -47,6 +47,11 @@ def on_POST(self, request):
            'id_server', 'client_secret', 'email', 'send_attempt'
        ])

+        if not check_3pid_allowed(self.hs, "email", body['email']):
+            raise SynapseError(
+                403, "Third party identifier is not allowed", Codes.THREEPID_DENIED


Nit: Generally we always try and use trailing commas

erikjohnston · 2018-01-19T10:50:28Z

synapse/rest/client/v2_alpha/register.py

+            else:
+                logger.warn(
+                    "Unrecognised 3PID medium %s in registrations_require_3pid" %
+                    constraint['medium']


Don't do string building.

erikjohnston · 2018-01-19T10:51:06Z

synapse/rest/client/v2_alpha/register.py

                flows.extend([
-                    [LoginType.MSISDN],
-                    [LoginType.MSISDN, LoginType.EMAIL_IDENTITY],
+                    [LoginType.MSISDN, LoginType.EMAIL_IDENTITY]


(See comment where we do this in v1 register)

erikjohnston · 2018-01-19T10:52:16Z

synapse/rest/client/v2_alpha/register.py

+            ):
+                raise SynapseError(
+                    403, "Third party identifier is not allowed", Codes.THREEPID_DENIED
+                )


Can we not use check_3pid_allowed here?

erikjohnston · 2018-01-19T10:55:40Z

I think this does what you want, but I think we need to be a lot more explicit that we're requiring and restricting 3pids here. I think it may be better to split up the config (and PR) into "require 3pid on register" and "disallow 3pids outside a whitelist"

@erikjohnston

* [ ] split config options into allowed_local_3pids and registrations_require_3pid * [ ] simplify and comment logic for picking registration flows * [ ] fix docstring and move check_3pid_allowed into a new util module * [ ] use check_3pid_allowed everywhere @erikjohnston PTAL

ara4n · 2018-01-19T15:40:07Z

Rewritten based on the above to:

split config options into allowed_local_3pids and registrations_require_3pid
simplify and comment logic for picking registration flows
fix docstring and move check_3pid_allowed into a new util module
use check_3pid_allowed everywhere
add trailing commas when we wrap args...

erikjohnston · 2018-01-19T16:18:22Z

synapse/util/threepids.py

+    if hs.config.allowed_local_3pids:
+        for constraint in hs.config.allowed_local_3pids:
+            logger.debug("Checking 3PID %s (%s) against %s (%s)" % (
+                address, medium, constraint['pattern'], constraint['medium']


Don't need explicit string formatting, logger.debug does it for you.

erikjohnston · 2018-01-19T16:19:21Z

synapse/rest/client/v2_alpha/register.py

+            )
+        ):
+            medium = auth_result[LoginType.EMAIL_IDENTITY].threepid['medium']
+            address = auth_result[LoginType.EMAIL_IDENTITY].threepid['address']


Doesn't this need to check both msisdn and email?

oops, thanks

erikjohnston · 2018-01-19T16:24:10Z

synapse/config/registration.py

+        #
+        # registrations_require_3pid:
+        #     - email
+        #     - msisdn


Personally I would have the two config option entirely separate, as this doesn't support the use case of "require a work email but let people bind personal emails"

we're not trying to solve for that use case here, plus having talked to @dbkr it's hard to do anyway - as /account/3pid goes and adds the email to your account (letting you log in with it) as well as optionally binding it on the identity server... so you can't separate the two easily anyway.

Hence me splitting it into:

What 3PID media do we mandate at registration?

What format 3PIDs are allowed to be added to accounts on this HS?
...with no restrictions on formats valid on ISes.

btw, this is a good example of a review comment where I'm staring at it thinking "does he really want me to rewrite it again? or is he just voicing a random comment?". It might be worth clearly spelling out which comments actually block accepting the PR from your POV, otherwise the committer has no option than assume everything is a request for change...

Ok, in this case it was an observation that this feels odd and questioning whether this is the right thing to do. The synapse review process is not just about whether functional changes are right, but if we should even be doing it like that.

erikjohnston · 2018-01-19T16:25:44Z

synapse/util/threepids.py

+        medium (str): 3pid medium - e.g. email, msisdn
+        address (str): address within that medium (e.g. "[email protected]")
+            msisdns need to first have been canonicalised
+    """


Should have a:

Returns: bool: ...

especially since I think there are other check_ functions raise rather than return a bool

ara4n · 2018-01-19T19:23:06Z

I'm confused what this is trying to do. Is this:

requiring that users register with a 3pid in a whitelist? or

stopping users from adding 3pids outside a whitelist?

Both. It's trying to limit both the 3PIDs which a user can use to register, and add to their HS account. The main focus is on requiring the user has a valid email address for a domain at the point that they register; however if this restriction is in place, it seems unwise to let them then go and add other emails to let them log into the account (such that they might still be able to login and reset passwords etc, even if their official address is no more).

In other words: it feels needlessly confusing to expose a different whitelist for 3PID formats which may be added to an account, than that which is used to register in the first place.

ara4n · 2018-01-19T19:24:03Z

I think this does what you want, but I think we need to be a lot more explicit that we're requiring and restricting 3pids here. I think it may be better to split up the config (and PR) into "require 3pid on register" and "disallow 3pids outside a whitelist"

Totally agreed, which is why I rewrote the PR to do this.

ara4n · 2018-01-19T20:53:08Z

@erikjohnston ptal again if you can face it.

richvdh · 2018-01-20T01:01:16Z

@erikjohnston did you mean to assign this to me?

erikjohnston · 2018-01-22T13:53:04Z

@erikjohnston did you mean to assign this to me?

woops, sorry, must have been muscle memory

ara4n · 2018-01-22T13:57:13Z

thanks :)

ara4n added 3 commits January 19, 2018 00:19

add registrations_require_3pid

28a6ccb

lets homeservers specify a whitelist for 3PIDs that users are allowed to associate with. Typically useful for stopping people from registering with non-work emails

mock registrations_require_3pid

81d037d

fix pep8

0af58f1

turt2live reviewed Jan 19, 2018

View reviewed changes

fix up v1, and improve errors

9d332e0

erikjohnston self-assigned this Jan 19, 2018

dbkr approved these changes Jan 19, 2018

View reviewed changes

erikjohnston suggested changes Jan 19, 2018

View reviewed changes

erikjohnston assigned ara4n and unassigned erikjohnston Jan 19, 2018

ara4n added 2 commits January 19, 2018 15:33

trailing commas

293380b

erikjohnston suggested changes Jan 19, 2018

View reviewed changes

erikjohnston assigned richvdh and unassigned ara4n Jan 19, 2018

ara4n added 2 commits January 19, 2018 18:23

fix PR nitpicking

8fe253f

oops, check all login types

62d7d66

ara4n changed the title ~~add registrations_require_3pid~~ add registrations_require_3pid and allow_local_3pids Jan 19, 2018

fix typo (thanks sytest)

49fce04

erikjohnston approved these changes Jan 22, 2018

View reviewed changes

erikjohnston assigned ara4n and unassigned richvdh Jan 22, 2018

ara4n merged commit d84f652 into develop Jan 22, 2018

hawkowl deleted the matthew/registrations_require_3pid branch September 20, 2018 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add registrations_require_3pid and allow_local_3pids #2813

add registrations_require_3pid and allow_local_3pids #2813

ara4n commented Jan 19, 2018 •

edited

Loading

turt2live Jan 19, 2018 •

edited

Loading

ara4n Jan 19, 2018

ara4n Jan 19, 2018

dbkr left a comment

dbkr Jan 19, 2018

dbkr Jan 19, 2018

erikjohnston commented Jan 19, 2018

erikjohnston Jan 19, 2018

erikjohnston Jan 19, 2018

erikjohnston Jan 19, 2018

erikjohnston Jan 19, 2018

erikjohnston Jan 19, 2018

erikjohnston Jan 19, 2018

erikjohnston Jan 19, 2018

erikjohnston Jan 19, 2018

erikjohnston Jan 19, 2018

erikjohnston commented Jan 19, 2018

ara4n commented Jan 19, 2018 •

edited

Loading

erikjohnston Jan 19, 2018

erikjohnston Jan 19, 2018

ara4n Jan 19, 2018

erikjohnston Jan 19, 2018

ara4n Jan 19, 2018

ara4n Jan 19, 2018

erikjohnston Jan 19, 2018

erikjohnston Jan 19, 2018

ara4n commented Jan 19, 2018

ara4n commented Jan 19, 2018

ara4n commented Jan 19, 2018

richvdh commented Jan 20, 2018

erikjohnston commented Jan 22, 2018

ara4n commented Jan 22, 2018

add registrations_require_3pid and allow_local_3pids #2813

add registrations_require_3pid and allow_local_3pids #2813

Conversation

ara4n commented Jan 19, 2018 • edited Loading

turt2live Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikjohnston commented Jan 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikjohnston commented Jan 19, 2018

ara4n commented Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ara4n commented Jan 19, 2018

ara4n commented Jan 19, 2018

ara4n commented Jan 19, 2018

richvdh commented Jan 20, 2018

erikjohnston commented Jan 22, 2018

ara4n commented Jan 22, 2018

ara4n commented Jan 19, 2018 •

edited

Loading

turt2live Jan 19, 2018 •

edited

Loading

ara4n commented Jan 19, 2018 •

edited

Loading