Don't offer password login when it is disabled (#8835)

Fix a minor bug where we would offer "m.login.password" login if a custom auth provider supported it, even if password login was disabled.

changelog.d/8835.bugfix

@@ -0,0 +1 @@
+Fix minor long-standing bug in login, where we would offer the `password` login type if a custom auth provider supported it, even if password login was disabled.

synapse/handlers/auth.py

@@ -205,15 +205,23 @@ def __init__(self, hs: "HomeServer"):
         # type in the list. (NB that the spec doesn't require us to do so and
         # clients which favour types that they don't understand over those that
         # they do are technically broken)
+
+        # start out by assuming PASSWORD is enabled; we will remove it later if not.
         login_types = []
-        if self._password_enabled:
+        if hs.config.password_localdb_enabled:
             login_types.append(LoginType.PASSWORD)
+
         for provider in self.password_providers:
             if hasattr(provider, "get_supported_login_types"):
                 for t in provider.get_supported_login_types().keys():
                     if t not in login_types:
                         login_types.append(t)
+
+        if not self._password_enabled:
+            login_types.remove(LoginType.PASSWORD)
+
         self._supported_login_types = login_types
+
         # Login types and UI Auth types have a heavy overlap, but are not
         # necessarily identical. Login types have SSO (and other login types)
         # added in the rest layer, see synapse.rest.client.v1.login.LoginRestServerlet.on_GET.

tests/handlers/test_password_providers.py

@@ -70,6 +70,24 @@ def check_auth(self, *args):
         return mock_password_provider.check_auth(*args)
 
 
+class PasswordCustomAuthProvider:
+    """A password_provider which implements password login via `check_auth`, as well
+    as a custom type."""
+
+    @staticmethod
+    def parse_config(self):
+        pass
+
+    def __init__(self, config, account_handler):
+        pass
+
+    def get_supported_login_types(self):
+        return {"m.login.password": ["password"], "test.login_type": ["test_field"]}
+
+    def check_auth(self, *args):
+        return mock_password_provider.check_auth(*args)
+
+
 def providers_config(*providers: Type[Any]) -> dict:
     """Returns a config dict that will enable the given password auth providers"""
     return {
@@ -246,7 +264,11 @@ def test_no_local_user_fallback_ui_auth(self):
         mock_password_provider.check_password.reset_mock()
 
         # first delete should give a 401
-        session = self._start_delete_device_session(tok1, "dev2")
+        channel = self._delete_device(tok1, "dev2")
+        self.assertEqual(channel.code, 401)
+        # there are no valid flows here!
+        self.assertEqual(channel.json_body["flows"], [])
+        session = channel.json_body["session"]
         mock_password_provider.check_password.assert_not_called()
 
         # now try deleting with the local password
@@ -410,6 +432,88 @@ def test_custom_auth_password_disabled(self):
         self.assertEqual(channel.code, 400, channel.result)
         mock_password_provider.check_auth.assert_not_called()
 
+    @override_config(
+        {
+            **providers_config(PasswordCustomAuthProvider),
+            "password_config": {"enabled": False},
+        }
+    )
+    def test_password_custom_auth_password_disabled_login(self):
+        """log in with a custom auth provider which implements password, but password
+        login is disabled"""
+        self.register_user("localuser", "localpass")
+
+        flows = self._get_login_flows()
+        self.assertEqual(flows, [{"type": "test.login_type"}] + ADDITIONAL_LOGIN_FLOWS)
+
+        # login shouldn't work and should be rejected with a 400 ("unknown login type")
+        channel = self._send_password_login("localuser", "localpass")
+        self.assertEqual(channel.code, 400, channel.result)
+        mock_password_provider.check_auth.assert_not_called()
+
+    @override_config(
+        {
+            **providers_config(PasswordCustomAuthProvider),
+            "password_config": {"enabled": False},
+        }
+    )
+    def test_password_custom_auth_password_disabled_ui_auth(self):
+        """UI Auth with a custom auth provider which implements password, but password
+        login is disabled"""
+        # register the user and log in twice via the test login type to get two devices,
+        self.register_user("localuser", "localpass")
+        mock_password_provider.check_auth.return_value = defer.succeed(
+            "@localuser:test"
+        )
+        channel = self._send_login("test.login_type", "localuser", test_field="")
+        self.assertEqual(channel.code, 200, channel.result)
+        tok1 = channel.json_body["access_token"]
+
+        channel = self._send_login(
+            "test.login_type", "localuser", test_field="", device_id="dev2"
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+
+        # make the initial request which returns a 401
+        channel = self._delete_device(tok1, "dev2")
+        self.assertEqual(channel.code, 401)
+        # Ensure that flows are what is expected. In particular, "password" should *not*
+        # be present.
+        self.assertIn({"stages": ["test.login_type"]}, channel.json_body["flows"])
+        session = channel.json_body["session"]
+
+        mock_password_provider.reset_mock()
+
+        # check that auth with password is rejected
+        body = {
+            "auth": {
+                "type": "m.login.password",
+                "identifier": {"type": "m.id.user", "user": "localuser"},
+                # FIXME "identifier" is ignored
+                #   https://github.com/matrix-org/synapse/issues/5665
+                "user": "localuser",
+                "password": "localpass",
+                "session": session,
+            },
+        }
+
+        channel = self._delete_device(tok1, "dev2", body)
+        self.assertEqual(channel.code, 400)
+        self.assertEqual(
+            "Password login has been disabled.", channel.json_body["error"]
+        )
+        mock_password_provider.check_auth.assert_not_called()
+        mock_password_provider.reset_mock()
+
+        # successful auth
+        body["auth"]["type"] = "test.login_type"
+        body["auth"]["test_field"] = "x"
+        channel = self._delete_device(tok1, "dev2", body)
+        self.assertEqual(channel.code, 200)
+        mock_password_provider.check_auth.assert_called_once_with(
+            "localuser", "test.login_type", {"test_field": "x"}
+        )
+
     @override_config(
         {
             **providers_config(CustomAuthProvider),
@@ -428,8 +532,6 @@ def test_custom_auth_no_local_user_fallback(self):
         channel = self._send_password_login("localuser", "localpass")
         self.assertEqual(channel.code, 400, channel.result)
 
-    test_custom_auth_no_local_user_fallback.skip = "currently broken"
-
     def _get_login_flows(self) -> JsonDict:
         _, channel = self.make_request("GET", "/_matrix/client/r0/login")
         self.assertEqual(channel.code, 200, channel.result)