Repository: matrix-org/synapse

Issue:
Remove email from registration flows if it’s unsupported
If the HS is not configured to support email (email config block missing and not delegated to an IS), then it should remove email from the registration flows so that clients know they shouldn't show the email field.

Discussion:

Patch:
diff --git a/changelog.d/6107.bugfix b/changelog.d/6107.bugfix
new file mode 100644
index 000000000..d4b9516ac
--- /dev/null
+++ b/changelog.d/6107.bugfix
@@ -0,0 +1,1 @@
+Ensure that servers which are not configured to support email address verification do not offer it in the registration flows.
\ No newline at end of file
diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py
index f920c2f6c..333eb3062 100644
--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -61,7 +61,8 @@ class AuthHandler(BaseHandler):
         self.checkers = {}  # type: dict[str, UserInteractiveAuthChecker]
         for auth_checker_class in INTERACTIVE_AUTH_CHECKERS:
             inst = auth_checker_class(hs)
-            self.checkers[inst.AUTH_TYPE] = inst
+            if inst.is_enabled():
+                self.checkers[inst.AUTH_TYPE] = inst
 
         self.bcrypt_rounds = hs.config.bcrypt_rounds
 
@@ -156,6 +157,14 @@ class AuthHandler(BaseHandler):
 
         return params
 
+    def get_enabled_auth_types(self):
+        """Return the enabled user-interactive authentication types
+
+        Returns the UI-Auth types which are supported by the homeserver's current
+        config.
+        """
+        return self.checkers.keys()
+
     @defer.inlineCallbacks
     def check_auth(self, flows, clientdict, clientip):
         """
diff --git a/synapse/handlers/ui_auth/checkers.py b/synapse/handlers/ui_auth/checkers.py
index fd633b7b0..ee6922324 100644
--- a/synapse/handlers/ui_auth/checkers.py
+++ b/synapse/handlers/ui_auth/checkers.py
@@ -32,6 +32,13 @@ class UserInteractiveAuthChecker:
     def __init__(self, hs):
         pass
 
+    def is_enabled(self):
+        """Check if the configuration of the homeserver allows this checker to work
+
+        Returns:
+            bool: True if this login type is enabled.
+        """
+
     def check_auth(self, authdict, clientip):
         """Given the authentication dict from the client, attempt to check this step
 
@@ -51,6 +58,9 @@ class UserInteractiveAuthChecker:
 class DummyAuthChecker(UserInteractiveAuthChecker):
     AUTH_TYPE = LoginType.DUMMY
 
+    def is_enabled(self):
+        return True
+
     def check_auth(self, authdict, clientip):
         return defer.succeed(True)
 
@@ -58,6 +68,9 @@ class DummyAuthChecker(UserInteractiveAuthChecker):
 class TermsAuthChecker(UserInteractiveAuthChecker):
     AUTH_TYPE = LoginType.TERMS
 
+    def is_enabled(self):
+        return True
+
     def check_auth(self, authdict, clientip):
         return defer.succeed(True)
 
@@ -67,10 +80,14 @@ class RecaptchaAuthChecker(UserInteractiveAuthChecker):
 
     def __init__(self, hs):
         super().__init__(hs)
+        self._enabled = bool(hs.config.recaptcha_private_key)
         self._http_client = hs.get_simple_http_client()
         self._url = hs.config.recaptcha_siteverify_api
         self._secret = hs.config.recaptcha_private_key
 
+    def is_enabled(self):
+        return self._enabled
+
     @defer.inlineCallbacks
     def check_auth(self, authdict, clientip):
         try:
@@ -191,6 +208,12 @@ class EmailIdentityAuthChecker(UserInteractiveAuthChecker, _BaseThreepidAuthChec
         UserInteractiveAuthChecker.__init__(self, hs)
         _BaseThreepidAuthChecker.__init__(self, hs)
 
+    def is_enabled(self):
+        return self.hs.config.threepid_behaviour_email in (
+            ThreepidBehaviour.REMOTE,
+            ThreepidBehaviour.LOCAL,
+        )
+
     def check_auth(self, authdict, clientip):
         return self._check_threepid("email", authdict)
 
@@ -202,6 +225,9 @@ class MsisdnAuthChecker(UserInteractiveAuthChecker, _BaseThreepidAuthChecker):
         UserInteractiveAuthChecker.__init__(self, hs)
         _BaseThreepidAuthChecker.__init__(self, hs)
 
+    def is_enabled(self):
+        return bool(self.hs.config.account_threepid_delegate_msisdn)
+
     def check_auth(self, authdict, clientip):
         return self._check_threepid("msisdn", authdict)
 
diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py
index e3f3d9126..4f24a124a 100644
--- a/synapse/rest/client/v2_alpha/register.py
+++ b/synapse/rest/client/v2_alpha/register.py
@@ -32,12 +32,14 @@ from synapse.api.errors import (
     ThreepidValidationError,
     UnrecognizedRequestError,
 )
+from synapse.config import ConfigError
 from synapse.config.captcha import CaptchaConfig
 from synapse.config.consent_config import ConsentConfig
 from synapse.config.emailconfig import ThreepidBehaviour
 from synapse.config.ratelimiting import FederationRateLimitConfig
 from synapse.config.registration import RegistrationConfig
 from synapse.config.server import is_threepid_reserved
+from synapse.handlers.auth import AuthHandler
 from synapse.http.server import finish_request
 from synapse.http.servlet import (
     RestServlet,
@@ -375,7 +377,9 @@ class RegisterRestServlet(RestServlet):
         self.ratelimiter = hs.get_registration_ratelimiter()
         self.clock = hs.get_clock()
 
-        self._registration_flows = _calculate_registration_flows(hs.config)
+        self._registration_flows = _calculate_registration_flows(
+            hs.config, self.auth_handler
+        )
 
     @interactive_auth_handler
     @defer.inlineCallbacks
@@ -664,11 +668,13 @@ class RegisterRestServlet(RestServlet):
 def _calculate_registration_flows(
     # technically `config` has to provide *all* of these interfaces, not just one
     config: Union[RegistrationConfig, ConsentConfig, CaptchaConfig],
+    auth_handler: AuthHandler,
 ) -> List[List[str]]:
     """Get a suitable flows list for registration
 
     Args:
         config: server configuration
+        auth_handler: authorization handler
 
     Returns: a list of supported flows
     """
@@ -678,10 +684,29 @@ def _calculate_registration_flows(
     require_msisdn = "msisdn" in config.registrations_require_3pid
 
     show_msisdn = True
+    show_email = True
+
     if config.disable_msisdn_registration:
         show_msisdn = False
         require_msisdn = False
 
+    enabled_auth_types = auth_handler.get_enabled_auth_types()
+    if LoginType.EMAIL_IDENTITY not in enabled_auth_types:
+        show_email = False
+        if require_email:
+            raise ConfigError(
+                "Configuration requires email address at registration, but email "
+                "validation is not configured"
+            )
+
+    if LoginType.MSISDN not in enabled_auth_types:
+        show_msisdn = False
+        if require_msisdn:
+            raise ConfigError(
+                "Configuration requires msisdn at registration, but msisdn "
+                "validation is not configured"
+            )
+
     flows = []
 
     # only support 3PIDless registration if no 3PIDs are required
@@ -693,14 +718,15 @@ def _calculate_registration_flows(
         flows.append([LoginType.DUMMY])
 
     # only support the email-only flow if we don't require MSISDN 3PIDs
-    if not require_msisdn:
+    if show_email and not require_msisdn:
         flows.append([LoginType.EMAIL_IDENTITY])
 
     # only support the MSISDN-only flow if we don't require email 3PIDs
     if show_msisdn and not require_email:
         flows.append([LoginType.MSISDN])
 
-    if show_msisdn:
+    if show_email and show_msisdn:
+        # always let users provide both MSISDN & email
         flows.append([LoginType.MSISDN, LoginType.EMAIL_IDENTITY])
 
     # Prepend m.login.terms to all flows if we're requiring consent


Test patch:
diff --git a/tests/rest/client/v2_alpha/test_register.py b/tests/rest/client/v2_alpha/test_register.py
index bc2dc4797..dab87e5ed 100644
--- a/tests/rest/client/v2_alpha/test_register.py
+++ b/tests/rest/client/v2_alpha/test_register.py
@@ -198,16 +198,8 @@ class RegisterRestServletTestCase(unittest.HomeserverTestCase):
         self.assertEquals(channel.result["code"], b"401", channel.result)
         flows = channel.json_body["flows"]
 
-        # with the stock config, we expect all four combinations of 3pid
-        self.assertCountEqual(
-            [
-                ["m.login.dummy"],
-                ["m.login.email.identity"],
-                ["m.login.msisdn"],
-                ["m.login.msisdn", "m.login.email.identity"],
-            ],
-            (f["stages"] for f in flows),
-        )
+        # with the stock config, we only expect the dummy flow
+        self.assertCountEqual([["m.login.dummy"]], (f["stages"] for f in flows))
 
     @unittest.override_config(
         {
@@ -217,9 +209,13 @@ class RegisterRestServletTestCase(unittest.HomeserverTestCase):
                 "template_dir": "/",
                 "require_at_registration": True,
             },
+            "account_threepid_delegates": {
+                "email": "https://id_server",
+                "msisdn": "https://id_server",
+            },
         }
     )
-    def test_advertised_flows_captcha_and_terms(self):
+    def test_advertised_flows_captcha_and_terms_and_3pids(self):
         request, channel = self.make_request(b"POST", self.url, b"{}")
         self.render(request)
         self.assertEquals(channel.result["code"], b"401", channel.result)
@@ -241,7 +237,16 @@ class RegisterRestServletTestCase(unittest.HomeserverTestCase):
         )
 
     @unittest.override_config(
-        {"registrations_require_3pid": ["email"], "disable_msisdn_registration": True}
+        {
+            "public_baseurl": "https://test_server",
+            "registrations_require_3pid": ["email"],
+            "disable_msisdn_registration": True,
+            "email": {
+                "smtp_host": "mail_server",
+                "smtp_port": 2525,
+                "notif_from": "sender@host",
+            },
+        }
     )
     def test_advertised_flows_no_msisdn_email_required(self):
         request, channel = self.make_request(b"POST", self.url, b"{}")