From 1bd814e13fdd95dc74354ce55ccffe5113c7201a Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 17 Nov 2020 09:33:32 -0500
Subject: [PATCH 01/11] Add additional tests for OIDC with existing users.

---
 synapse/handlers/oidc_handler.py |  7 ++---
 tests/handlers/test_oidc.py      | 48 ++++++++++++++++++++++++++++----
 2 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index be8562d47b63..7283be4ceedb 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -38,7 +38,7 @@
 from synapse.handlers.sso import MappingException
 from synapse.http.site import SynapseRequest
 from synapse.logging.context import make_deferred_yieldable
-from synapse.types import JsonDict, UserID, map_username_to_mxid_localpart
+from synapse.types import JsonDict, UserID
 from synapse.util import json_decoder
 
 if TYPE_CHECKING:
@@ -885,11 +885,10 @@ async def _map_userinfo_to_user(
             "Retrieved user attributes from user mapping provider: %r", attributes
         )
 
-        if not attributes["localpart"]:
+        localpart = attributes["localpart"]
+        if not localpart:
             raise MappingException("localpart is empty")
 
-        localpart = map_username_to_mxid_localpart(attributes["localpart"])
-
         user_id = UserID(localpart, self.server_name).to_string()
         users = await self.store.get_users_by_id_case_insensitive(user_id)
         if users:
diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index 630e6da808ef..3552acdbfb10 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -705,13 +705,13 @@ def test_map_userinfo_to_user(self):
     def test_map_userinfo_to_existing_user(self):
         """Existing users can log in with OpenID Connect when allow_existing_users is True."""
         store = self.hs.get_datastore()
-        user4 = UserID.from_string("@test_user_4:test")
+        user = UserID.from_string("@test_user:test")
         self.get_success(
-            store.register_user(user_id=user4.to_string(), password_hash=None)
+            store.register_user(user_id=user.to_string(), password_hash=None)
         )
         userinfo = {
-            "sub": "test4",
-            "username": "test_user_4",
+            "sub": "test",
+            "username": "test_user",
         }
         token = {}
         mxid = self.get_success(
@@ -719,4 +719,42 @@ def test_map_userinfo_to_existing_user(self):
                 userinfo, token, "user-agent", "10.10.10.10"
             )
         )
-        self.assertEqual(mxid, "@test_user_4:test")
+        self.assertEqual(mxid, "@test_user:test")
+
+        # Register some non-exact matching cases.
+        user2 = UserID.from_string("@test_user_2:test")
+        self.get_success(
+            store.register_user(user_id=user2.to_string(), password_hash=None)
+        )
+        user2_caps = UserID.from_string("@TEST_USER_2:test")
+        self.get_success(
+            store.register_user(user_id=user2_caps.to_string(), password_hash=None)
+        )
+
+        # Attempting to login without matching a name exactly is an error.
+        userinfo = {
+            "sub": "test2",
+            "username": "test_USER_2",
+        }
+        e = self.get_failure(
+            self.handler._map_userinfo_to_user(
+                userinfo, token, "user-agent", "10.10.10.10"
+            ),
+            MappingException,
+        )
+        self.assertEqual(
+            str(e.value),
+            "Attempted to login as '@test_USER_2:test' but it matches more than one user inexactly: ['@test_user_2:test', '@TEST_USER_2:test']",
+        )
+
+        # Logging in when matching a name exactly should work.
+        userinfo = {
+            "sub": "test2",
+            "username": "test_user_2",
+        }
+        mxid = self.get_success(
+            self.handler._map_userinfo_to_user(
+                userinfo, token, "user-agent", "10.10.10.10"
+            )
+        )
+        self.assertEqual(mxid, "@test_user_2:test")

From ab04e51ddb8177fea60407cfa5558446e1ea1109 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 17 Nov 2020 10:27:38 -0500
Subject: [PATCH 02/11] Rework this to not change behavior.

---
 synapse/handlers/oidc_handler.py |  5 ++++-
 tests/handlers/test_oidc.py      | 16 +++++++++++-----
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index 7283be4ceedb..30cd785040a1 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -38,7 +38,7 @@
 from synapse.handlers.sso import MappingException
 from synapse.http.site import SynapseRequest
 from synapse.logging.context import make_deferred_yieldable
-from synapse.types import JsonDict, UserID
+from synapse.types import JsonDict, UserID, map_username_to_mxid_localpart
 from synapse.util import json_decoder
 
 if TYPE_CHECKING:
@@ -889,6 +889,9 @@ async def _map_userinfo_to_user(
         if not localpart:
             raise MappingException("localpart is empty")
 
+        # Ensure only valid characters are included in the MXID.
+        localpart = map_username_to_mxid_localpart(localpart)
+
         user_id = UserID(localpart, self.server_name).to_string()
         users = await self.store.get_users_by_id_case_insensitive(user_id)
         if users:
diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index 3552acdbfb10..9eb191114997 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -722,11 +722,11 @@ def test_map_userinfo_to_existing_user(self):
         self.assertEqual(mxid, "@test_user:test")
 
         # Register some non-exact matching cases.
-        user2 = UserID.from_string("@test_user_2:test")
+        user2 = UserID.from_string("@TEST_user_2:test")
         self.get_success(
             store.register_user(user_id=user2.to_string(), password_hash=None)
         )
-        user2_caps = UserID.from_string("@TEST_USER_2:test")
+        user2_caps = UserID.from_string("@test_USER_2:test")
         self.get_success(
             store.register_user(user_id=user2_caps.to_string(), password_hash=None)
         )
@@ -734,7 +734,7 @@ def test_map_userinfo_to_existing_user(self):
         # Attempting to login without matching a name exactly is an error.
         userinfo = {
             "sub": "test2",
-            "username": "test_USER_2",
+            "username": "test_user_2",
         }
         e = self.get_failure(
             self.handler._map_userinfo_to_user(
@@ -744,10 +744,16 @@ def test_map_userinfo_to_existing_user(self):
         )
         self.assertEqual(
             str(e.value),
-            "Attempted to login as '@test_USER_2:test' but it matches more than one user inexactly: ['@test_user_2:test', '@TEST_USER_2:test']",
+            "Attempted to login as '@test_user_2:test' but it matches more than one user inexactly: ['@TEST_user_2:test', '@test_USER_2:test']",
+        )
+
+        # Logging in when matching a name exactly should work. (Note that this
+        # essentially means matching an all lowercase name.)
+        user2 = UserID.from_string("@test_user_2:test")
+        self.get_success(
+            store.register_user(user_id=user2.to_string(), password_hash=None)
         )
 
-        # Logging in when matching a name exactly should work.
         userinfo = {
             "sub": "test2",
             "username": "test_user_2",

From f9524dd8bbdb5355a481e9b258d39de27022864f Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 17 Nov 2020 10:35:38 -0500
Subject: [PATCH 03/11] Add a newsfragment.

---
 changelog.d/8774.misc | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 changelog.d/8774.misc

diff --git a/changelog.d/8774.misc b/changelog.d/8774.misc
new file mode 100644
index 000000000000..221bb1894940
--- /dev/null
+++ b/changelog.d/8774.misc
@@ -0,0 +1 @@
+Expand tests for OpenID Connect.

From 8501613934cb86be779d03711541c320b04ffdce Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 17 Nov 2020 10:36:52 -0500
Subject: [PATCH 04/11] Remove duplicated variable.

---
 tests/handlers/test_oidc.py | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index 9eb191114997..46b55000b2ee 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -754,10 +754,6 @@ def test_map_userinfo_to_existing_user(self):
             store.register_user(user_id=user2.to_string(), password_hash=None)
         )
 
-        userinfo = {
-            "sub": "test2",
-            "username": "test_user_2",
-        }
         mxid = self.get_success(
             self.handler._map_userinfo_to_user(
                 userinfo, token, "user-agent", "10.10.10.10"

From 592bd12131f27d11383bb9d56a262d2012f094d9 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 17 Nov 2020 10:39:42 -0500
Subject: [PATCH 05/11] Clarify tests a bit more.

---
 tests/handlers/test_oidc.py | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index 46b55000b2ee..dba98db1db3b 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -734,7 +734,7 @@ def test_map_userinfo_to_existing_user(self):
         # Attempting to login without matching a name exactly is an error.
         userinfo = {
             "sub": "test2",
-            "username": "test_user_2",
+            "username": "test_USER_2",
         }
         e = self.get_failure(
             self.handler._map_userinfo_to_user(
@@ -747,8 +747,15 @@ def test_map_userinfo_to_existing_user(self):
             "Attempted to login as '@test_user_2:test' but it matches more than one user inexactly: ['@TEST_user_2:test', '@test_USER_2:test']",
         )
 
-        # Logging in when matching a name exactly should work. (Note that this
-        # essentially means matching an all lowercase name.)
+        # Logging in when matching a name exactly should work.
+        #
+        # Note that this essentially means matching an existing all lowercase
+        # name, regardless of the case of the localpart providing by the mapping
+        # provider.
+        #
+        # Above we return test_USER_2, which you'd expect to match the user
+        # test_USER_2, which was already created, but it actually ends up
+        # matching test_user_2.
         user2 = UserID.from_string("@test_user_2:test")
         self.get_success(
             store.register_user(user_id=user2.to_string(), password_hash=None)

From 9436837b470fcac0421c6667a0cc171928bd7c15 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 17 Nov 2020 13:56:23 -0500
Subject: [PATCH 06/11] Move the normalization to the mapper.

---
 docs/sso_mapping_providers.md    |  2 +-
 synapse/handlers/oidc_handler.py | 23 ++++++++++++++++++-----
 synapse/handlers/saml_handler.py |  6 ++++++
 synapse/types.py                 |  6 +++---
 tests/handlers/test_oidc.py      | 31 +++++++++++++++++++------------
 5 files changed, 47 insertions(+), 21 deletions(-)

diff --git a/docs/sso_mapping_providers.md b/docs/sso_mapping_providers.md
index 32b06aa2c570..e9075c475bac 100644
--- a/docs/sso_mapping_providers.md
+++ b/docs/sso_mapping_providers.md
@@ -16,7 +16,7 @@ SSO mapping providers are currently supported for OpenID and SAML SSO
 configurations. Please see the details below for how to implement your own.
 
 External mapping providers are provided to Synapse in the form of an external
-Python module. You can retrieve this module from [PyPi](https://pypi.org) or elsewhere,
+Python module. You can retrieve this module from [PyPI](https://pypi.org) or elsewhere,
 but it must be importable via Synapse (e.g. it must be in the same virtualenv
 as Synapse). The Synapse config is then modified to point to the mapping provider
 (and optionally provide additional configuration for it).
diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index 30cd785040a1..4bfd8d5617be 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -38,7 +38,12 @@
 from synapse.handlers.sso import MappingException
 from synapse.http.site import SynapseRequest
 from synapse.logging.context import make_deferred_yieldable
-from synapse.types import JsonDict, UserID, map_username_to_mxid_localpart
+from synapse.types import (
+    JsonDict,
+    UserID,
+    contains_invalid_mxid_characters,
+    map_username_to_mxid_localpart,
+)
 from synapse.util import json_decoder
 
 if TYPE_CHECKING:
@@ -887,10 +892,10 @@ async def _map_userinfo_to_user(
 
         localpart = attributes["localpart"]
         if not localpart:
-            raise MappingException("localpart is empty")
-
-        # Ensure only valid characters are included in the MXID.
-        localpart = map_username_to_mxid_localpart(localpart)
+            raise MappingException(
+                "Error parsing OIDC response: OIDC mapping provider plugin "
+                "did not return a localpart value"
+            )
 
         user_id = UserID(localpart, self.server_name).to_string()
         users = await self.store.get_users_by_id_case_insensitive(user_id)
@@ -910,6 +915,11 @@ async def _map_userinfo_to_user(
                 # This mxid is taken
                 raise MappingException("mxid '{}' is already taken".format(user_id))
         else:
+            # Since the localpart is provided via a potentially untrusted module,
+            # ensure the MXID is valid before registering.
+            if contains_invalid_mxid_characters(localpart):
+                raise MappingException("localpart is invalid: %s" % (localpart,))
+
             # It's the first time this user is logging in and the mapped mxid was
             # not taken, register the user
             registered_user_id = await self._registration_handler.register_user(
@@ -1078,6 +1088,9 @@ async def map_user_attributes(
     ) -> UserAttribute:
         localpart = self._config.localpart_template.render(user=userinfo).strip()
 
+        # Ensure only valid characters are included in the MXID.
+        localpart = map_username_to_mxid_localpart(localpart)
+
         display_name = None  # type: Optional[str]
         if self._config.display_name_template is not None:
             display_name = self._config.display_name_template.render(
diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py
index aee772239a55..21b2c29c72ce 100644
--- a/synapse/handlers/saml_handler.py
+++ b/synapse/handlers/saml_handler.py
@@ -31,6 +31,7 @@
 from synapse.module_api import ModuleApi
 from synapse.types import (
     UserID,
+    contains_invalid_mxid_characters,
     map_username_to_mxid_localpart,
     mxid_localpart_allowed_characters,
 )
@@ -317,6 +318,11 @@ async def _map_saml_response_to_user(
                     "Unable to generate a Matrix ID from the SAML response"
                 )
 
+            # Since the localpart is provided via a potentially untrusted module,
+            # ensure the MXID is valid before registering.
+            if contains_invalid_mxid_characters(localpart):
+                raise MappingException("localpart is invalid: %s" % (localpart,))
+
             logger.info("Mapped SAML user to local part %s", localpart)
             registered_user_id = await self._registration_handler.register_user(
                 localpart=localpart,
diff --git a/synapse/types.py b/synapse/types.py
index 66bb5bac8dc2..3ab6bdbe0626 100644
--- a/synapse/types.py
+++ b/synapse/types.py
@@ -317,14 +317,14 @@ def from_string(cls: Type[DS], s: str) -> DS:
 )
 
 
-def contains_invalid_mxid_characters(localpart):
+def contains_invalid_mxid_characters(localpart: str) -> bool:
     """Check for characters not allowed in an mxid or groupid localpart
 
     Args:
-        localpart (basestring): the localpart to be checked
+        localpart: the localpart to be checked
 
     Returns:
-        bool: True if there are any naughty characters
+        True if there are any naughty characters
     """
     return any(c not in mxid_localpart_allowed_characters for c in localpart)
 
diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index dba98db1db3b..7e033073a44a 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -734,7 +734,7 @@ def test_map_userinfo_to_existing_user(self):
         # Attempting to login without matching a name exactly is an error.
         userinfo = {
             "sub": "test2",
-            "username": "test_USER_2",
+            "username": "TEST_USER_2",
         }
         e = self.get_failure(
             self.handler._map_userinfo_to_user(
@@ -744,19 +744,11 @@ def test_map_userinfo_to_existing_user(self):
         )
         self.assertEqual(
             str(e.value),
-            "Attempted to login as '@test_user_2:test' but it matches more than one user inexactly: ['@TEST_user_2:test', '@test_USER_2:test']",
+            "Attempted to login as '@TEST_USER_2:test' but it matches more than one user inexactly: ['@TEST_user_2:test', '@test_USER_2:test']",
         )
 
         # Logging in when matching a name exactly should work.
-        #
-        # Note that this essentially means matching an existing all lowercase
-        # name, regardless of the case of the localpart providing by the mapping
-        # provider.
-        #
-        # Above we return test_USER_2, which you'd expect to match the user
-        # test_USER_2, which was already created, but it actually ends up
-        # matching test_user_2.
-        user2 = UserID.from_string("@test_user_2:test")
+        user2 = UserID.from_string("@TEST_USER_2:test")
         self.get_success(
             store.register_user(user_id=user2.to_string(), password_hash=None)
         )
@@ -766,4 +758,19 @@ def test_map_userinfo_to_existing_user(self):
                 userinfo, token, "user-agent", "10.10.10.10"
             )
         )
-        self.assertEqual(mxid, "@test_user_2:test")
+        self.assertEqual(mxid, "@TEST_USER_2:test")
+
+    def test_map_userinfo_to_invalid_localpart(self):
+        """If the mapping provider generates an invalid localpart it should be rejected."""
+        userinfo = {
+            "sub": "test2",
+            "username": "föö",
+        }
+        token = {}
+        e = self.get_failure(
+            self.handler._map_userinfo_to_user(
+                userinfo, token, "user-agent", "10.10.10.10"
+            ),
+            MappingException,
+        )
+        self.assertEqual(str(e.value), "localpart is invalid: föö")

From 613eb4df20d0575ed97032ab4ffff27b60618216 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 17 Nov 2020 14:45:44 -0500
Subject: [PATCH 07/11] Add some basic SAML tests.

---
 tests/handlers/test_oidc.py |  24 +++----
 tests/handlers/test_saml.py | 139 ++++++++++++++++++++++++++++++++++++
 2 files changed, 148 insertions(+), 15 deletions(-)
 create mode 100644 tests/handlers/test_saml.py

diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index 7e033073a44a..cc0ebc0de58e 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -12,7 +12,6 @@
 # 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 json
 from urllib.parse import parse_qs, urlparse
 
@@ -24,12 +23,8 @@
 from twisted.python.failure import Failure
 from twisted.web._newclient import ResponseDone
 
-from synapse.handlers.oidc_handler import (
-    MappingException,
-    OidcError,
-    OidcHandler,
-    OidcMappingProvider,
-)
+from synapse.handlers.oidc_handler import OidcError, OidcHandler, OidcMappingProvider
+from synapse.handlers.sso import MappingException
 from synapse.types import UserID
 
 from tests.unittest import HomeserverTestCase, override_config
@@ -132,14 +127,13 @@ def make_homeserver(self, reactor, clock):
 
         config = self.default_config()
         config["public_baseurl"] = BASE_URL
-        oidc_config = {}
-        oidc_config["enabled"] = True
-        oidc_config["client_id"] = CLIENT_ID
-        oidc_config["client_secret"] = CLIENT_SECRET
-        oidc_config["issuer"] = ISSUER
-        oidc_config["scopes"] = SCOPES
-        oidc_config["user_mapping_provider"] = {
-            "module": __name__ + ".TestMappingProvider",
+        oidc_config = {
+            "enabled": True,
+            "client_id": CLIENT_ID,
+            "client_secret": CLIENT_SECRET,
+            "issuer": ISSUER,
+            "scopes": SCOPES,
+            "user_mapping_provider": {"module": __name__ + ".TestMappingProvider"},
         }
 
         # Update this config with what's in the default config so that
diff --git a/tests/handlers/test_saml.py b/tests/handlers/test_saml.py
new file mode 100644
index 000000000000..d4fd33343ad6
--- /dev/null
+++ b/tests/handlers/test_saml.py
@@ -0,0 +1,139 @@
+#  Copyright 2020 The Matrix.org Foundation C.I.C.
+#
+#  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.
+
+from mock import Mock
+
+import attr
+
+from synapse.handlers.saml_handler import SamlHandler
+from synapse.handlers.sso import MappingException
+from synapse.types import UserID
+
+from tests.unittest import HomeserverTestCase
+
+# These are a few constants that are used as config parameters in the tests.
+BASE_URL = "https://synapse/"
+
+
+@attr.s
+class FakeAuthnResponse:
+    ava = attr.ib(type=dict)
+
+
+class TestMappingProvider:
+    def __init__(self, config, module):
+        pass
+
+    @staticmethod
+    def parse_config(config):
+        return
+
+    @staticmethod
+    def get_saml_attributes(config):
+        return {"uid"}, {"displayName"}
+
+    def get_remote_user_id(self, saml_response, client_redirect_url):
+        return saml_response.ava["uid"]
+
+    def saml_response_to_user_attributes(
+        self, saml_response, failures, client_redirect_url
+    ):
+        return {"mxid_localpart": saml_response.ava["username"], "displayname": None}
+
+
+def simple_async_mock(return_value=None, raises=None):
+    # AsyncMock is not available in python3.5, this mimics part of its behaviour
+    async def cb(*args, **kwargs):
+        if raises:
+            raise raises
+        return return_value
+
+    return Mock(side_effect=cb)
+
+
+class SamlHandlerTestCase(HomeserverTestCase):
+    def make_homeserver(self, reactor, clock):
+
+        self.http_client = Mock(spec=["get_json"])
+        self.http_client.user_agent = "Synapse Test"
+
+        config = self.default_config()
+        config["public_baseurl"] = BASE_URL
+        saml_config = {
+            "sp_config": {"metadata": {}},
+            # Disable grandfathering.
+            "grandfathered_mxid_source_attribute": None,
+            "user_mapping_provider": {"module": __name__ + ".TestMappingProvider"},
+        }
+        config["saml2_config"] = saml_config
+
+        hs = self.setup_test_homeserver(
+            http_client=self.http_client,
+            proxied_http_client=self.http_client,
+            config=config,
+        )
+
+        self.handler = SamlHandler(hs)
+
+        return hs
+
+    def test_map_saml_response_to_user(self):
+        """Ensure that mapping the SAML response returned from a provider to an MXID works properly."""
+        saml_response = FakeAuthnResponse({"uid": "test_user", "username": "test_user"})
+        # The redirect_url doesn't matter with the default user mapping provider.
+        redirect_url = ""
+        mxid = self.get_success(
+            self.handler._map_saml_response_to_user(
+                saml_response, redirect_url, "user-agent", "10.10.10.10"
+            )
+        )
+        self.assertEqual(mxid, "@test_user:test")
+
+        # Some providers return an integer ID.
+        saml_response = FakeAuthnResponse({"uid": 1234, "username": "test_user_2"})
+        mxid = self.get_success(
+            self.handler._map_saml_response_to_user(
+                saml_response, redirect_url, "user-agent", "10.10.10.10"
+            )
+        )
+        self.assertEqual(mxid, "@test_user_2:test")
+
+        # Test if the mxid is already taken
+        store = self.hs.get_datastore()
+        user3 = UserID.from_string("@test_user_3:test")
+        self.get_success(
+            store.register_user(user_id=user3.to_string(), password_hash=None)
+        )
+        saml_response = FakeAuthnResponse({"uid": "test3", "username": "test_user_3"})
+        e = self.get_failure(
+            self.handler._map_saml_response_to_user(
+                saml_response, redirect_url, "user-agent", "10.10.10.10"
+            ),
+            MappingException,
+        )
+        self.assertEqual(
+            str(e.value), "Unable to generate a Matrix ID from the SAML response"
+        )
+
+    def test_map_saml_response_to_invalid_localpart(self):
+        """If the mapping provider generates an invalid localpart it should be rejected."""
+        saml_response = FakeAuthnResponse({"uid": "test2", "username": "föö"})
+        redirect_url = ""
+        e = self.get_failure(
+            self.handler._map_saml_response_to_user(
+                saml_response, redirect_url, "user-agent", "10.10.10.10"
+            ),
+            MappingException,
+        )
+        self.assertEqual(str(e.value), "localpart is invalid: föö")

From 07ba01d48e3763318db2b94945c2bab6c6e59c6e Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 17 Nov 2020 14:48:16 -0500
Subject: [PATCH 08/11] Please Python 3.5.

---
 tests/handlers/test_oidc.py | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index cc0ebc0de58e..b4fa02acc486 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -736,9 +736,10 @@ def test_map_userinfo_to_existing_user(self):
             ),
             MappingException,
         )
-        self.assertEqual(
-            str(e.value),
-            "Attempted to login as '@TEST_USER_2:test' but it matches more than one user inexactly: ['@TEST_user_2:test', '@test_USER_2:test']",
+        self.assertTrue(
+            str(e.value).startswith(
+                "Attempted to login as '@TEST_USER_2:test' but it matches more than one user inexactly:"
+            )
         )
 
         # Logging in when matching a name exactly should work.

From edf350e3acb48a026ac9b4b15315904deb7c95e7 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 17 Nov 2020 14:49:18 -0500
Subject: [PATCH 09/11] Update the newsfragment.

---
 changelog.d/8774.misc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/changelog.d/8774.misc b/changelog.d/8774.misc
index 221bb1894940..57cca8fee51c 100644
--- a/changelog.d/8774.misc
+++ b/changelog.d/8774.misc
@@ -1 +1 @@
-Expand tests for OpenID Connect.
+Add additional error checking for OpenID Connect and SAML mapping providers.

From ffafd999b1a3e7302de0a5013925bb58c1ef23af Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Wed, 18 Nov 2020 14:09:06 -0500
Subject: [PATCH 10/11] Remove SAML tests since the Docker image cannot run
 them.

---
 tests/handlers/test_saml.py | 139 ------------------------------------
 1 file changed, 139 deletions(-)
 delete mode 100644 tests/handlers/test_saml.py

diff --git a/tests/handlers/test_saml.py b/tests/handlers/test_saml.py
deleted file mode 100644
index d4fd33343ad6..000000000000
--- a/tests/handlers/test_saml.py
+++ /dev/null
@@ -1,139 +0,0 @@
-#  Copyright 2020 The Matrix.org Foundation C.I.C.
-#
-#  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.
-
-from mock import Mock
-
-import attr
-
-from synapse.handlers.saml_handler import SamlHandler
-from synapse.handlers.sso import MappingException
-from synapse.types import UserID
-
-from tests.unittest import HomeserverTestCase
-
-# These are a few constants that are used as config parameters in the tests.
-BASE_URL = "https://synapse/"
-
-
-@attr.s
-class FakeAuthnResponse:
-    ava = attr.ib(type=dict)
-
-
-class TestMappingProvider:
-    def __init__(self, config, module):
-        pass
-
-    @staticmethod
-    def parse_config(config):
-        return
-
-    @staticmethod
-    def get_saml_attributes(config):
-        return {"uid"}, {"displayName"}
-
-    def get_remote_user_id(self, saml_response, client_redirect_url):
-        return saml_response.ava["uid"]
-
-    def saml_response_to_user_attributes(
-        self, saml_response, failures, client_redirect_url
-    ):
-        return {"mxid_localpart": saml_response.ava["username"], "displayname": None}
-
-
-def simple_async_mock(return_value=None, raises=None):
-    # AsyncMock is not available in python3.5, this mimics part of its behaviour
-    async def cb(*args, **kwargs):
-        if raises:
-            raise raises
-        return return_value
-
-    return Mock(side_effect=cb)
-
-
-class SamlHandlerTestCase(HomeserverTestCase):
-    def make_homeserver(self, reactor, clock):
-
-        self.http_client = Mock(spec=["get_json"])
-        self.http_client.user_agent = "Synapse Test"
-
-        config = self.default_config()
-        config["public_baseurl"] = BASE_URL
-        saml_config = {
-            "sp_config": {"metadata": {}},
-            # Disable grandfathering.
-            "grandfathered_mxid_source_attribute": None,
-            "user_mapping_provider": {"module": __name__ + ".TestMappingProvider"},
-        }
-        config["saml2_config"] = saml_config
-
-        hs = self.setup_test_homeserver(
-            http_client=self.http_client,
-            proxied_http_client=self.http_client,
-            config=config,
-        )
-
-        self.handler = SamlHandler(hs)
-
-        return hs
-
-    def test_map_saml_response_to_user(self):
-        """Ensure that mapping the SAML response returned from a provider to an MXID works properly."""
-        saml_response = FakeAuthnResponse({"uid": "test_user", "username": "test_user"})
-        # The redirect_url doesn't matter with the default user mapping provider.
-        redirect_url = ""
-        mxid = self.get_success(
-            self.handler._map_saml_response_to_user(
-                saml_response, redirect_url, "user-agent", "10.10.10.10"
-            )
-        )
-        self.assertEqual(mxid, "@test_user:test")
-
-        # Some providers return an integer ID.
-        saml_response = FakeAuthnResponse({"uid": 1234, "username": "test_user_2"})
-        mxid = self.get_success(
-            self.handler._map_saml_response_to_user(
-                saml_response, redirect_url, "user-agent", "10.10.10.10"
-            )
-        )
-        self.assertEqual(mxid, "@test_user_2:test")
-
-        # Test if the mxid is already taken
-        store = self.hs.get_datastore()
-        user3 = UserID.from_string("@test_user_3:test")
-        self.get_success(
-            store.register_user(user_id=user3.to_string(), password_hash=None)
-        )
-        saml_response = FakeAuthnResponse({"uid": "test3", "username": "test_user_3"})
-        e = self.get_failure(
-            self.handler._map_saml_response_to_user(
-                saml_response, redirect_url, "user-agent", "10.10.10.10"
-            ),
-            MappingException,
-        )
-        self.assertEqual(
-            str(e.value), "Unable to generate a Matrix ID from the SAML response"
-        )
-
-    def test_map_saml_response_to_invalid_localpart(self):
-        """If the mapping provider generates an invalid localpart it should be rejected."""
-        saml_response = FakeAuthnResponse({"uid": "test2", "username": "föö"})
-        redirect_url = ""
-        e = self.get_failure(
-            self.handler._map_saml_response_to_user(
-                saml_response, redirect_url, "user-agent", "10.10.10.10"
-            ),
-            MappingException,
-        )
-        self.assertEqual(str(e.value), "localpart is invalid: föö")

From 8d21b256976504ed8021bdc79e0ac293fc972460 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Thu, 19 Nov 2020 10:44:24 -0500
Subject: [PATCH 11/11] Add upgrade note and improve documentation.

---
 UPGRADE.rst                   | 30 ++++++++++++++++++++++++++++++
 docs/sso_mapping_providers.md |  7 +++++++
 2 files changed, 37 insertions(+)

diff --git a/UPGRADE.rst b/UPGRADE.rst
index 960c2aeb2bdc..2a7ee305bd8e 100644
--- a/UPGRADE.rst
+++ b/UPGRADE.rst
@@ -75,6 +75,36 @@ for example:
      wget https://packages.matrix.org/debian/pool/main/m/matrix-synapse-py3/matrix-synapse-py3_1.3.0+stretch1_amd64.deb
      dpkg -i matrix-synapse-py3_1.3.0+stretch1_amd64.deb
 
+Upgrading to v1.24.0
+====================
+
+Custom OpenID Connect mapping provider breaking change
+------------------------------------------------------
+
+This release allows the OpenID Connect mapping provider to perform normalisation
+of the localpart of the Matrix ID. This allows for the mapping provider to
+specify different algorithms, instead of the [default way](https://matrix.org/docs/spec/appendices#mapping-from-other-character-sets).
+
+If your Synapse configuration uses a custom mapping provider
+(`oidc_config.user_mapping_provider.module` is specified and not equal to
+`synapse.handlers.oidc_handler.JinjaOidcMappingProvider`) then you *must* ensure
+that `map_user_attributes` of the mapping provider performs some normalisation
+of the `localpart` returned. To match previous behaviour you can use the
+`map_username_to_mxid_localpart` function provided by Synapse. An example is
+shown below:
+
+.. code-block:: python
+
+  from synapse.types import map_username_to_mxid_localpart
+
+  class MyMappingProvider:
+      def map_user_attributes(self, userinfo, token):
+          # ... your custom logic ...
+          sso_user_id = ...
+          localpart = map_username_to_mxid_localpart(sso_user_id)
+
+          return {"localpart": localpart}
+
 Upgrading to v1.23.0
 ====================
 
diff --git a/docs/sso_mapping_providers.md b/docs/sso_mapping_providers.md
index e9075c475bac..707dd7397888 100644
--- a/docs/sso_mapping_providers.md
+++ b/docs/sso_mapping_providers.md
@@ -15,6 +15,13 @@ where SAML mapping providers come into play.
 SSO mapping providers are currently supported for OpenID and SAML SSO
 configurations. Please see the details below for how to implement your own.
 
+It is the responsibility of the mapping provider to normalise the SSO attributes
+and map them to a valid Matrix ID. The
+[specification for Matrix IDs](https://matrix.org/docs/spec/appendices#user-identifiers)
+has some information about what is considered valid. Alternately an easy way to
+ensure it is valid is to use a Synapse utility function:
+`synapse.types.map_username_to_mxid_localpart`.
+
 External mapping providers are provided to Synapse in the form of an external
 Python module. You can retrieve this module from [PyPI](https://pypi.org) or elsewhere,
 but it must be importable via Synapse (e.g. it must be in the same virtualenv