rewrite based on PR feedback:

 * [ ] 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

7 files changed, 102 insertions, 89 deletions

diff --git a/synapse/config/registration.py b/synapse/config/registration.py
index e5e4f77872..336959094b 100644
--- a/synapse/config/registration.py
+++ b/synapse/config/registration.py
@@ -32,6 +32,7 @@ class RegistrationConfig(Config):
             )
 
         self.registrations_require_3pid = config.get("registrations_require_3pid", [])
+        self.allowed_local_3pids = config.get("allowed_local_3pids", [])
         self.registration_shared_secret = config.get("registration_shared_secret")
 
         self.bcrypt_rounds = config.get("bcrypt_rounds", 12)
@@ -53,11 +54,16 @@ class RegistrationConfig(Config):
         # Enable registration for new users.
         enable_registration: False
 
-        # Mandate that registrations require a 3PID which matches one or more
-        # of these 3PIDs.  N.B. regexp escape backslashes are doubled (once for
-        # YAML and once for the regexp itself)
+        # The user must provide all of the below types of 3PID when registering.
         #
         # registrations_require_3pid:
+        #     - email
+        #     - msisdn
+
+        # Mandate that users are only allowed to associate certain formats of
+        # 3PIDs with accounts on this server.
+        #
+        # allowed_local_3pids:
         #     - medium: email
         #       pattern: ".*@matrix\\.org"
         #     - medium: email
diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py
index 157ebaf251..9021d4d57f 100644
--- a/synapse/handlers/register.py
+++ b/synapse/handlers/register.py
@@ -15,7 +15,6 @@
 
 """Contains functions for registering clients."""
 import logging
-import re
 
 from twisted.internet import defer
 
@@ -26,6 +25,7 @@ from synapse.http.client import CaptchaServerHttpClient
 from synapse import types
 from synapse.types import UserID
 from synapse.util.async import run_on_reactor
+from synapse.util.threepids import check_3pid_allowed
 from ._base import BaseHandler
 
 logger = logging.getLogger(__name__)
@@ -308,15 +308,10 @@ class RegistrationHandler(BaseHandler):
             logger.info("got threepid with medium '%s' and address '%s'",
                         threepid['medium'], threepid['address'])
 
-            for constraint in self.hs.config.registrations_require_3pid:
-                if (
-                    constraint['medium'] == 'email' and
-                    threepid['medium'] == 'email' and
-                    re.match(constraint['pattern'], threepid['address'])
-                ):
-                    raise RegistrationError(
-                        403, "Third party identifier is not allowed"
-                    )
+            if not check_3pid_allowed(self.hs, threepid['medium'], threepid['address']):
+                raise RegistrationError(
+                    403, "Third party identifier is not allowed"
+                )
 
     @defer.inlineCallbacks
     def bind_emails(self, user_id, threepidCreds):
diff --git a/synapse/rest/client/v1/register.py b/synapse/rest/client/v1/register.py
index f793542ad6..5c5fa8f7ab 100644
--- a/synapse/rest/client/v1/register.py
+++ b/synapse/rest/client/v1/register.py
@@ -71,22 +71,13 @@ class RegisterRestServlet(ClientV1RestServlet):
 
     def on_GET(self, request):
 
-        require_email = False
-        require_msisdn = False
-        for constraint in self.hs.config.registrations_require_3pid:
-            if constraint['medium'] == 'email':
-                require_email = True
-            elif constraint['medium'] == 'msisdn':
-                require_msisdn = True
-            else:
-                logger.warn(
-                    "Unrecognised 3PID medium %s in registrations_require_3pid" %
-                    constraint['medium']
-                )
+        require_email = 'email' in self.hs.config.registrations_require_3pid
+        require_msisdn = 'msisdn' in self.hs.config.registrations_require_3pid
 
         flows = []
         if self.hs.config.enable_registration_captcha:
-            if require_email or not require_msisdn:
+            # only support the email-only flow if we don't require MSISDN 3PIDs
+            if not require_msisdn:
                 flows.extend([
                     {
                         "type": LoginType.RECAPTCHA,
@@ -97,6 +88,7 @@ class RegisterRestServlet(ClientV1RestServlet):
                         ]
                     },
                 ])
+            # only support 3PIDless registration if no 3PIDs are required
             if not require_email and not require_msisdn:
                 flows.extend([
                     {
@@ -105,6 +97,7 @@ class RegisterRestServlet(ClientV1RestServlet):
                     }
                 ])
         else:
+            # only support the email-only flow if we don't require MSISDN 3PIDs
             if require_email or not require_msisdn:
                 flows.extend([
                     {
@@ -114,6 +107,7 @@ class RegisterRestServlet(ClientV1RestServlet):
                         ]
                     }
                 ])
+            # only support 3PIDless registration if no 3PIDs are required
             if not require_email and not require_msisdn:
                 flows.extend([
                     {
diff --git a/synapse/rest/client/v2_alpha/_base.py b/synapse/rest/client/v2_alpha/_base.py
index b286ff0d95..77434937ff 100644
--- a/synapse/rest/client/v2_alpha/_base.py
+++ b/synapse/rest/client/v2_alpha/_base.py
@@ -60,27 +60,6 @@ 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
-
-    allow = False
-    if hs.config.registrations_require_3pid:
-        for constraint in hs.config.registrations_require_3pid:
-            logger.debug("Checking 3PID %s (%s) against %s (%s)" % (
-                address, medium, constraint['pattern'], constraint['medium']
-            ))
-            if (
-                medium == constraint['medium'] and
-                re.match(constraint['pattern'], address)
-            ):
-                allow = True
-                break
-    else:
-        allow = True
-
-    return allow
-
-
 def interactive_auth_handler(orig):
     """Wraps an on_POST method to handle InteractiveAuthIncompleteErrors
 
diff --git a/synapse/rest/client/v2_alpha/account.py b/synapse/rest/client/v2_alpha/account.py
index 2977ad439f..514bb37da1 100644
--- a/synapse/rest/client/v2_alpha/account.py
+++ b/synapse/rest/client/v2_alpha/account.py
@@ -26,7 +26,8 @@ from synapse.http.servlet import (
 )
 from synapse.util.async import run_on_reactor
 from synapse.util.msisdn import phone_number_to_msisdn
-from ._base import client_v2_patterns, interactive_auth_handler, check_3pid_allowed
+from synapse.util.threepids import check_3pid_allowed
+from ._base import client_v2_patterns, interactive_auth_handler
 
 logger = logging.getLogger(__name__)
 
diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py
index 898d8b133a..c3479e29de 100644
--- a/synapse/rest/client/v2_alpha/register.py
+++ b/synapse/rest/client/v2_alpha/register.py
@@ -26,11 +26,11 @@ from synapse.http.servlet import (
     RestServlet, parse_json_object_from_request, assert_params_in_request, parse_string
 )
 from synapse.util.msisdn import phone_number_to_msisdn
+from synapse.util.threepids import check_3pid_allowed
 
-from ._base import client_v2_patterns, interactive_auth_handler, check_3pid_allowed
+from ._base import client_v2_patterns, interactive_auth_handler
 
 import logging
-import re
 import hmac
 from hashlib import sha1
 from synapse.util.async import run_on_reactor
@@ -316,41 +316,41 @@ class RegisterRestServlet(RestServlet):
         if 'x_show_msisdn' in body and body['x_show_msisdn']:
             show_msisdn = True
 
-        require_email = False
-        require_msisdn = False
-        for constraint in self.hs.config.registrations_require_3pid:
-            if constraint['medium'] == 'email':
-                require_email = True
-            elif constraint['medium'] == 'msisdn':
-                require_msisdn = True
-            else:
-                logger.warn(
-                    "Unrecognised 3PID medium %s in registrations_require_3pid" %
-                    constraint['medium']
-                )
+        # FIXME: need a better error than "no auth flow found" for scenarios
+        # where we required 3PID for registration but the user didn't give one
+        require_email = 'email' in self.hs.config.registrations_require_3pid
+        require_msisdn = 'msisdn' in self.hs.config.registrations_require_3pid
 
         flows = []
         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]])
-            if require_email or not require_msisdn:
+            # only support the email-only flow if we don't require MSISDN 3PIDs
+            if not require_msisdn:
                 flows.extend([[LoginType.EMAIL_IDENTITY, LoginType.RECAPTCHA]])
 
             if show_msisdn:
-                if not require_email or require_msisdn:
+                # only support the MSISDN-only flow if we don't require email 3PIDs
+                if not require_email:
                     flows.extend([[LoginType.MSISDN, LoginType.RECAPTCHA]])
+                # always let users provide both MSISDN & email
                 flows.extend([
                     [LoginType.MSISDN, LoginType.EMAIL_IDENTITY, LoginType.RECAPTCHA],
                 ])
         else:
+            # only support 3PIDless registration if no 3PIDs are required
             if not require_email and not require_msisdn:
                 flows.extend([[LoginType.DUMMY]])
-            if require_email or not require_msisdn:
+            # only support the email-only flow if we don't require MSISDN 3PIDs
+            if not require_msisdn:
                 flows.extend([[LoginType.EMAIL_IDENTITY]])
 
             if show_msisdn:
+                # only support the MSISDN-only flow if we don't require email 3PIDs
                 if not require_email or require_msisdn:
                     flows.extend([[LoginType.MSISDN]])
+                # always let users provide both MSISDN & email
                 flows.extend([
                     [LoginType.MSISDN, LoginType.EMAIL_IDENTITY]
                 ])
@@ -359,30 +359,23 @@ class RegisterRestServlet(RestServlet):
             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
-
-        for constraint in self.hs.config.registrations_require_3pid:
-            if (
-                constraint['medium'] == 'email' and
-                auth_result and LoginType.EMAIL_IDENTITY in auth_result and
-                re.match(
-                    constraint['pattern'],
-                    auth_result[LoginType.EMAIL_IDENTITY].threepid.address
-                )
-            ):
-                raise SynapseError(
-                    403, "Third party identifier is not allowed", Codes.THREEPID_DENIED
-                )
-            elif (
-                constraint['medium'] == 'msisdn' and
-                auth_result and LoginType.MSISDN in auth_result and
-                re.match(
-                    constraint['pattern'],
-                    auth_result[LoginType.MSISDN].threepid.address
-                )
-            ):
+        # Check that we're not trying to register a denied 3pid.
+        #
+        # the user-facing checks will probably already have happened in
+        # /register/email/requestToken when we requested a 3pid, but that's not
+        # guaranteed.
+
+        if (
+            auth_result and
+            (
+                LoginType.EMAIL_IDENTITY in auth_result or
+                LoginType.EMAIL_MSISDN in auth_result
+            )
+        ):
+            medium = auth_result[LoginType.EMAIL_IDENTITY].threepid['medium']
+            address = auth_result[LoginType.EMAIL_IDENTITY].threepid['address']
+
+            if not check_3pid_allowed(self.hs, medium, address):
                 raise SynapseError(
                     403, "Third party identifier is not allowed", Codes.THREEPID_DENIED
                 )
diff --git a/synapse/util/threepids.py b/synapse/util/threepids.py
new file mode 100644
index 0000000000..e921b97796
--- /dev/null
+++ b/synapse/util/threepids.py
@@ -0,0 +1,45 @@
+# -*- coding: utf-8 -*-
+# Copyright 2018 New Vector Ltd
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import logging
+import re
+
+logger = logging.getLogger(__name__)
+
+
+def check_3pid_allowed(hs, medium, address):
+    """Checks whether a given format of 3PID is allowed to be used on this HS
+
+    Args:
+        hs (synapse.server.HomeServer): server
+        medium (str): 3pid medium - e.g. email, msisdn
+        address (str): address within that medium (e.g. "wotan@matrix.org")
+            msisdns need to first have been canonicalised
+    """
+
+    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']
+            ))
+            if (
+                medium == constraint['medium'] and
+                re.match(constraint['pattern'], address)
+            ):
+                return True
+    else:
+        return True
+
+    return False