From 24265fac17168dbd83eaa7971ed1c05ed1e1678a Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 17 Nov 2020 09:45:18 -0500
Subject: [PATCH 01/16] Support trying multiple localparts for OpenID Connect.

---
 docs/sso_mapping_providers.md    |   9 +++
 synapse/handlers/oidc_handler.py | 120 +++++++++++++++++++------------
 tests/handlers/test_oidc.py      |  64 +++++++++++++++++
 3 files changed, 149 insertions(+), 44 deletions(-)

diff --git a/docs/sso_mapping_providers.md b/docs/sso_mapping_providers.md
index 707dd7397888..712ad9373fe9 100644
--- a/docs/sso_mapping_providers.md
+++ b/docs/sso_mapping_providers.md
@@ -70,6 +70,15 @@ A custom mapping provider must specify the following methods:
                      information from.
       - `token` - A dictionary which includes information necessary to make
                   further requests to the OpenID provider.
+      - `failures` - An `int` that represents the amount of times the returned
+                     mxid localpart mapping has failed.  This should be used
+                     to create a deduplicated mxid localpart which should be
+                     returned instead. For example, if this method returns
+                     `john.doe` as the value of `localpart` in the returned
+                     dict, and that is already taken on the homeserver, this
+                     method will be called again with the same parameters but
+                     with failures=1. The method should then return a different
+                     `localpart` value, such as `john.doe1`.
     - Returns a dictionary with two keys:
       - localpart: A required string, used to generate the Matrix ID.
       - displayname: An optional string, the display name for the user.
diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index 4bfd8d5617be..6068f3b061f7 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -12,6 +12,7 @@
 # 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 inspect
 import logging
 from typing import TYPE_CHECKING, Dict, Generic, List, Optional, Tuple, TypeVar
 from urllib.parse import urlencode
@@ -93,6 +94,9 @@ class OidcHandler(BaseHandler):
     """Handles requests related to the OpenID Connect login flow.
     """
 
+    # The number of attempts to ask the mapping provider for when generating an MXID.
+    _MAP_USERNAME_RETRIES = 1000
+
     def __init__(self, hs: "HomeServer"):
         super().__init__(hs)
         self._callback_url = hs.config.oidc_callback_url  # type: str
@@ -876,56 +880,82 @@ async def _map_userinfo_to_user(
         if previously_registered_user_id:
             return previously_registered_user_id
 
-        # Otherwise, generate a new user.
-        try:
-            attributes = await self._user_mapping_provider.map_user_attributes(
-                userinfo, token
-            )
-        except Exception as e:
-            raise MappingException(
-                "Could not extract user attributes from OIDC response: " + str(e)
-            )
-
-        logger.debug(
-            "Retrieved user attributes from user mapping provider: %r", attributes
+        # Older mapping providers don't accept the `failures` argument, so we
+        # try and detect support.
+        mapper_args = inspect.getfullargspec(
+            self._user_mapping_provider.map_user_attributes
         )
+        supports_failures = "failures" in mapper_args.args
 
-        localpart = attributes["localpart"]
-        if not localpart:
-            raise MappingException(
-                "Error parsing OIDC response: OIDC mapping provider plugin "
-                "did not return a localpart value"
+        # Otherwise, generate a new user.
+        for i in range(self._MAP_USERNAME_RETRIES):
+            try:
+                if supports_failures:
+                    attributes = await self._user_mapping_provider.map_user_attributes(
+                        userinfo, token, i
+                    )
+                else:
+                    attributes = await self._user_mapping_provider.map_user_attributes(  # type: ignore
+                        userinfo, token
+                    )
+            except Exception as e:
+                raise MappingException(
+                    "Could not extract user attributes from OIDC response: " + str(e)
+                )
+
+            logger.debug(
+                "Retrieved user attributes from user mapping provider: %r", attributes
             )
 
-        user_id = UserID(localpart, self.server_name).to_string()
-        users = await self.store.get_users_by_id_case_insensitive(user_id)
-        if users:
-            if self._allow_existing_users:
-                if len(users) == 1:
-                    registered_user_id = next(iter(users))
-                elif user_id in users:
-                    registered_user_id = user_id
+            localpart = attributes["localpart"]
+            if not 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)
+            if users:
+                if self._allow_existing_users:
+                    if len(users) == 1:
+                        registered_user_id = next(iter(users))
+                        break
+                    elif user_id in users:
+                        registered_user_id = user_id
+                        break
+                    else:
+                        raise MappingException(
+                            "Attempted to login as '{}' but it matches more than one user inexactly: {}".format(
+                                user_id, list(users.keys())
+                            )
+                        )
                 else:
-                    raise MappingException(
-                        "Attempted to login as '{}' but it matches more than one user inexactly: {}".format(
-                            user_id, list(users.keys())
+                    # This mxid is taken
+                    if not supports_failures:
+                        raise MappingException(
+                            "mxid '{}' is already taken".format(user_id)
                         )
-                    )
             else:
-                # This mxid is taken
-                raise MappingException("mxid '{}' is already taken".format(user_id))
+                # 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(
+                    localpart=localpart,
+                    default_display_name=attributes["display_name"],
+                    user_agent_ips=(user_agent, ip_address),
+                )
+                break
+
         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(
-                localpart=localpart,
-                default_display_name=attributes["display_name"],
-                user_agent_ips=(user_agent, ip_address),
+            # Unable to generate a username in 1000 iterations
+            # Break and return error to the user
+            raise MappingException(
+                "Unable to generate a Matrix ID from the OpenID response"
             )
 
         await self.store.record_user_external_id(
@@ -978,13 +1008,15 @@ def get_remote_user_id(self, userinfo: UserInfo) -> str:
         raise NotImplementedError()
 
     async def map_user_attributes(
-        self, userinfo: UserInfo, token: Token
+        self, userinfo: UserInfo, token: Token, failures: int
     ) -> UserAttribute:
         """Map a `UserInfo` object into user attributes.
 
         Args:
             userinfo: An object representing the user given by the OIDC provider
             token: A dict with the tokens returned by the provider
+            failures: How many times a call to this function with this
+                UserInfo has resulted in a failure.
 
         Returns:
             A dict containing the ``localpart`` and (optionally) the ``display_name``
@@ -1084,7 +1116,7 @@ def get_remote_user_id(self, userinfo: UserInfo) -> str:
         return userinfo[self._config.subject_claim]
 
     async def map_user_attributes(
-        self, userinfo: UserInfo, token: Token
+        self, userinfo: UserInfo, token: Token, failures: int
     ) -> UserAttribute:
         localpart = self._config.localpart_template.render(user=userinfo).strip()
 
diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index b4fa02acc486..496e7387f43c 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -89,6 +89,14 @@ async def get_extra_attributes(self, userinfo, token):
         return {"phone": userinfo["phone"]}
 
 
+class TestMappingProviderFailures(TestMappingProvider):
+    async def map_user_attributes(self, userinfo, token, failures):
+        return {
+            "localpart": userinfo["username"] + (str(failures) if failures else ""),
+            "display_name": 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):
@@ -152,6 +160,9 @@ def make_homeserver(self, reactor, clock):
         self.render_error = Mock(return_value=None)
         self.handler._sso_handler.render_error = self.render_error
 
+        # Reduce the number of attempts when generating MXIDs.
+        self.handler._MAP_USERNAME_RETRIES = 3
+
         return hs
 
     def metadata_edit(self, values):
@@ -762,6 +773,7 @@ def test_map_userinfo_to_invalid_localpart(self):
             "username": "föö",
         }
         token = {}
+
         e = self.get_failure(
             self.handler._map_userinfo_to_user(
                 userinfo, token, "user-agent", "10.10.10.10"
@@ -769,3 +781,55 @@ def test_map_userinfo_to_invalid_localpart(self):
             MappingException,
         )
         self.assertEqual(str(e.value), "localpart is invalid: föö")
+
+    @override_config(
+        {
+            "oidc_config": {
+                "user_mapping_provider": {
+                    "module": __name__ + ".TestMappingProviderFailures"
+                }
+            }
+        }
+    )
+    def test_map_userinfo_to_user_retries(self):
+        """The mapping provider can retry generating an MXID if the MXID is already in use."""
+        store = self.hs.get_datastore()
+        self.get_success(
+            store.register_user(user_id="@test_user:test", password_hash=None)
+        )
+        userinfo = {
+            "sub": "test",
+            "username": "test_user",
+        }
+        token = {}
+        mxid = self.get_success(
+            self.handler._map_userinfo_to_user(
+                userinfo, token, "user-agent", "10.10.10.10"
+            )
+        )
+        # test_user is already taken, so test_user1 gets registered instead.
+        self.assertEqual(mxid, "@test_user1:test")
+
+        # Register all of the potential users for a particular username.
+        self.get_success(
+            store.register_user(user_id="@tester:test", password_hash=None)
+        )
+        for i in range(1, 3):
+            self.get_success(
+                store.register_user(user_id="@tester%d:test" % i, password_hash=None)
+            )
+
+        # Now attempt to map to a username, this will fail since all potential usernames are taken.
+        userinfo = {
+            "sub": "tester",
+            "username": "tester",
+        }
+        e = self.get_failure(
+            self.handler._map_userinfo_to_user(
+                userinfo, token, "user-agent", "10.10.10.10"
+            ),
+            MappingException,
+        )
+        self.assertEqual(
+            str(e.value), "Unable to generate a Matrix ID from the OpenID response"
+        )

From f4bdc2f55cfec024524bcb6b516e6656438f368f Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Mon, 23 Nov 2020 09:54:42 -0500
Subject: [PATCH 02/16] Add an additional test.

---
 tests/handlers/test_oidc.py | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index 496e7387f43c..ed1042fe3baf 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -714,6 +714,8 @@ def test_map_userinfo_to_existing_user(self):
         self.get_success(
             store.register_user(user_id=user.to_string(), password_hash=None)
         )
+
+        # Map a user via SSO.
         userinfo = {
             "sub": "test",
             "username": "test_user",
@@ -726,6 +728,23 @@ def test_map_userinfo_to_existing_user(self):
         )
         self.assertEqual(mxid, "@test_user:test")
 
+        # Note that a second SSO user can be mapped to the same Matrix ID. (This
+        # requires a unique sub, but something that maps to the same matrix ID,
+        # in this case we'll just use the same username. A more realistic example
+        # would be subs which are email addresses, and mapping from the localpart
+        # of the email, e.g. bob@foo.com and bob@bar.com -> @bob:test.)
+        userinfo = {
+            "sub": "test1",
+            "username": "test_user",
+        }
+        token = {}
+        mxid = self.get_success(
+            self.handler._map_userinfo_to_user(
+                userinfo, token, "user-agent", "10.10.10.10"
+            )
+        )
+        self.assertEqual(mxid, "@test_user:test")
+
         # Register some non-exact matching cases.
         user2 = UserID.from_string("@TEST_user_2:test")
         self.get_success(

From f419ff5b0d5659c72072062834ba8aedb022e9d2 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Mon, 23 Nov 2020 10:21:34 -0500
Subject: [PATCH 03/16] Abstract the shared user mapping code from SAML and
 OIDC.

---
 synapse/handlers/oidc_handler.py | 106 +++++++------------------------
 synapse/handlers/saml_handler.py |  85 ++++++++-----------------
 synapse/handlers/sso.py          | 102 ++++++++++++++++++++++++++++-
 tests/handlers/test_oidc.py      |   8 ++-
 4 files changed, 154 insertions(+), 147 deletions(-)

diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index 6068f3b061f7..8e74ed15ffb5 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -39,12 +39,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,
-    contains_invalid_mxid_characters,
-    map_username_to_mxid_localpart,
-)
+from synapse.types import JsonDict, map_username_to_mxid_localpart
 from synapse.util import json_decoder
 
 if TYPE_CHECKING:
@@ -94,9 +89,6 @@ class OidcHandler(BaseHandler):
     """Handles requests related to the OpenID Connect login flow.
     """
 
-    # The number of attempts to ask the mapping provider for when generating an MXID.
-    _MAP_USERNAME_RETRIES = 1000
-
     def __init__(self, hs: "HomeServer"):
         super().__init__(hs)
         self._callback_url = hs.config.oidc_callback_url  # type: str
@@ -873,13 +865,6 @@ async def _map_userinfo_to_user(
         # to be strings.
         remote_user_id = str(remote_user_id)
 
-        # first of all, check if we already have a mapping for this user
-        previously_registered_user_id = await self._sso_handler.get_sso_user_by_remote_user_id(
-            self._auth_provider_id, remote_user_id,
-        )
-        if previously_registered_user_id:
-            return previously_registered_user_id
-
         # Older mapping providers don't accept the `failures` argument, so we
         # try and detect support.
         mapper_args = inspect.getfullargspec(
@@ -887,81 +872,34 @@ async def _map_userinfo_to_user(
         )
         supports_failures = "failures" in mapper_args.args
 
-        # Otherwise, generate a new user.
-        for i in range(self._MAP_USERNAME_RETRIES):
-            try:
-                if supports_failures:
-                    attributes = await self._user_mapping_provider.map_user_attributes(
-                        userinfo, token, i
-                    )
-                else:
-                    attributes = await self._user_mapping_provider.map_user_attributes(  # type: ignore
-                        userinfo, token
-                    )
-            except Exception as e:
-                raise MappingException(
-                    "Could not extract user attributes from OIDC response: " + str(e)
-                )
-
-            logger.debug(
-                "Retrieved user attributes from user mapping provider: %r", attributes
-            )
+        async def oidc_response_to_user_attributes(userinfo, token, failures):
+            """
+            Call the mapping provider to map the OIDC userinfo and token to user attributes.
 
-            localpart = attributes["localpart"]
-            if not localpart:
-                raise MappingException(
-                    "Error parsing OIDC response: OIDC mapping provider plugin "
-                    "did not return a localpart value"
+            This is backwards compatibility for abstraction for the SSO handler.
+            """
+            if supports_failures:
+                attributes = await self._user_mapping_provider.map_user_attributes(
+                    userinfo, token, failures
                 )
-
-            user_id = UserID(localpart, self.server_name).to_string()
-            users = await self.store.get_users_by_id_case_insensitive(user_id)
-            if users:
-                if self._allow_existing_users:
-                    if len(users) == 1:
-                        registered_user_id = next(iter(users))
-                        break
-                    elif user_id in users:
-                        registered_user_id = user_id
-                        break
-                    else:
-                        raise MappingException(
-                            "Attempted to login as '{}' but it matches more than one user inexactly: {}".format(
-                                user_id, list(users.keys())
-                            )
-                        )
-                else:
-                    # This mxid is taken
-                    if not supports_failures:
-                        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(
-                    localpart=localpart,
-                    default_display_name=attributes["display_name"],
-                    user_agent_ips=(user_agent, ip_address),
+                attributes = await self._user_mapping_provider.map_user_attributes(  # type: ignore
+                    userinfo, token
                 )
-                break
 
-        else:
-            # Unable to generate a username in 1000 iterations
-            # Break and return error to the user
-            raise MappingException(
-                "Unable to generate a Matrix ID from the OpenID response"
-            )
+            return attributes
+
+        # TODO Support existing users.
 
-        await self.store.record_user_external_id(
-            self._auth_provider_id, remote_user_id, registered_user_id,
+        return await self._sso_handler.get_mxid_from_sso(
+            self._auth_provider_id,
+            remote_user_id,
+            user_agent,
+            ip_address,
+            oidc_response_to_user_attributes,
+            userinfo,
+            token,
         )
-        return registered_user_id
 
 
 UserAttribute = TypedDict(
diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py
index f4e8cbeac821..8de18960dabb 100644
--- a/synapse/handlers/saml_handler.py
+++ b/synapse/handlers/saml_handler.py
@@ -31,7 +31,6 @@
 from synapse.module_api import ModuleApi
 from synapse.types import (
     UserID,
-    contains_invalid_mxid_characters,
     map_username_to_mxid_localpart,
     mxid_localpart_allowed_characters,
 )
@@ -250,14 +249,26 @@ async def _map_saml_response_to_user(
                 "Failed to extract remote user id from SAML response"
             )
 
-        with (await self._mapping_lock.queue(self._auth_provider_id)):
-            # first of all, check if we already have a mapping for this user
-            previously_registered_user_id = await self._sso_handler.get_sso_user_by_remote_user_id(
-                self._auth_provider_id, remote_user_id,
+        async def saml_response_to_remapped_user_attributes(
+            saml_response, client_redirect_url, failures
+        ):
+            """
+            Call the mapping provider to map a SAML response to user attributes and coerce the result into the standard form.
+
+            This is backwards compatibility for abstraction for the SSO handler.
+            """
+            # Remap the arguments to the mapping provider.
+            result = self._user_mapping_provider.saml_response_to_user_attributes(
+                saml_response, failures, client_redirect_url
             )
-            if previously_registered_user_id:
-                return previously_registered_user_id
+            # Remap some of the results.
+            if "mxid_localpart" in result:
+                result["localpart"] = result.pop("mxid_localpart")
+            if "displayname" in result:
+                result["display_name"] = result.pop("displayname")
+            return result
 
+        with (await self._mapping_lock.queue(self._auth_provider_id)):
             # backwards-compatibility hack: see if there is an existing user with a
             # suitable mapping from the uid
             if (
@@ -284,59 +295,15 @@ async def _map_saml_response_to_user(
                     )
                     return registered_user_id
 
-            # Map saml response to user attributes using the configured mapping provider
-            for i in range(1000):
-                attribute_dict = self._user_mapping_provider.saml_response_to_user_attributes(
-                    saml2_auth, i, client_redirect_url=client_redirect_url,
-                )
-
-                logger.debug(
-                    "Retrieved SAML attributes from user mapping provider: %s "
-                    "(attempt %d)",
-                    attribute_dict,
-                    i,
-                )
-
-                localpart = attribute_dict.get("mxid_localpart")
-                if not localpart:
-                    raise MappingException(
-                        "Error parsing SAML2 response: SAML mapping provider plugin "
-                        "did not return a mxid_localpart value"
-                    )
-
-                displayname = attribute_dict.get("displayname")
-                emails = attribute_dict.get("emails", [])
-
-                # Check if this mxid already exists
-                if not await self.store.get_users_by_id_case_insensitive(
-                    UserID(localpart, self.server_name).to_string()
-                ):
-                    # This mxid is free
-                    break
-            else:
-                # Unable to generate a username in 1000 iterations
-                # Break and return error to the user
-                raise MappingException(
-                    "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.debug("Mapped SAML user to local part %s", localpart)
-            registered_user_id = await self._registration_handler.register_user(
-                localpart=localpart,
-                default_display_name=displayname,
-                bind_emails=emails,
-                user_agent_ips=(user_agent, ip_address),
-            )
-
-            await self.store.record_user_external_id(
-                self._auth_provider_id, remote_user_id, registered_user_id
+            return await self._sso_handler.get_mxid_from_sso(
+                self._auth_provider_id,
+                remote_user_id,
+                user_agent,
+                ip_address,
+                saml_response_to_remapped_user_attributes,
+                saml2_auth,
+                client_redirect_url,
             )
-            return registered_user_id
 
     def expire_sessions(self):
         expire_before = self.clock.time_msec() - self._saml2_session_lifetime
diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py
index cf7cb7754a95..b152bb26aea4 100644
--- a/synapse/handlers/sso.py
+++ b/synapse/handlers/sso.py
@@ -13,10 +13,11 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 import logging
-from typing import TYPE_CHECKING, Optional
+from typing import TYPE_CHECKING, Any, Awaitable, Callable, Optional
 
 from synapse.handlers._base import BaseHandler
 from synapse.http.server import respond_with_html
+from synapse.types import UserID, contains_invalid_mxid_characters
 
 if TYPE_CHECKING:
     from synapse.server import HomeServer
@@ -30,8 +31,12 @@ class MappingException(Exception):
 
 
 class SsoHandler(BaseHandler):
+    # The number of attempts to ask the mapping provider for when generating an MXID.
+    _MAP_USERNAME_RETRIES = 1000
+
     def __init__(self, hs: "HomeServer"):
         super().__init__(hs)
+        self._registration_handler = hs.get_registration_handler()
         self._error_template = hs.config.sso_error_template
 
     def render_error(
@@ -94,3 +99,98 @@ async def get_sso_user_by_remote_user_id(
 
         # No match.
         return None
+
+    async def get_mxid_from_sso(
+        self,
+        auth_provider_id: str,
+        remote_user_id: str,
+        user_agent: str,
+        ip_address: str,
+        mapper: "Callable[..., Awaitable[str]]",
+        *args: Any,
+        **kwargs: Any,
+    ):
+        """
+
+        Args:
+            auth_provider_id: A unique identifier for this SSO provider, e.g.
+                "oidc" or "saml".
+            remote_user_id: The unique identifier from the SSO provider.
+            user_agent: The user agent of the client making the request.
+            ip_address: The IP address of the client making the request.
+            mapper: A callable to generate the user attributes. Note that the
+                current number of failures is passed in as the last argument.
+            args: Additional arguments for mapper.
+            kwargs: Additional keyword arguments for mapper.
+
+        Returns:
+             The user ID associated with the SSO response.
+
+        Raises:
+            MappingException if there was a problem mapping the response to a user.
+            RedirectException: some mapping providers may raise this if they need
+                to redirect to an interstitial page.
+
+        """
+        # first of all, check if we already have a mapping for this user
+        previously_registered_user_id = await self.get_sso_user_by_remote_user_id(
+            auth_provider_id, remote_user_id,
+        )
+        if previously_registered_user_id:
+            return previously_registered_user_id
+
+        # Otherwise, generate a new user.
+        for i in range(self._MAP_USERNAME_RETRIES):
+            try:
+                attributes = await mapper(*args, i, **kwargs)
+            except Exception as e:
+                raise MappingException(
+                    "Could not extract user attributes from SSO response: " + str(e)
+                )
+
+            logger.debug(
+                "Retrieved user attributes from user mapping provider: %r (attempt %d)",
+                attributes,
+                i,
+            )
+
+            localpart = attributes["localpart"]
+            if not localpart:
+                raise MappingException(
+                    "Error parsing SSO response: SSO mapping provider plugin "
+                    "did not return a localpart value"
+                )
+
+            # Other potential attributes returned by the mapping provider.
+            display_name = attributes.get("display_name")
+            emails = attributes.get("emails", [])
+
+            # Check if this mxid already exists
+            user_id = UserID(localpart, self.server_name).to_string()
+            if not await self.store.get_users_by_id_case_insensitive(user_id):
+                # This mxid is free
+                break
+        else:
+            # Unable to generate a username in 1000 iterations
+            # Break and return error to the user
+            raise MappingException(
+                "Unable to generate a Matrix ID from the SSO 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.debug("Mapped SAML user to local part %s", localpart)
+        registered_user_id = await self._registration_handler.register_user(
+            localpart=localpart,
+            default_display_name=display_name,
+            bind_emails=emails,
+            user_agent_ips=(user_agent, ip_address),
+        )
+
+        await self.store.record_user_external_id(
+            auth_provider_id, remote_user_id, registered_user_id
+        )
+        return registered_user_id
diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index ed1042fe3baf..9180b9aaa623 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -161,7 +161,7 @@ def make_homeserver(self, reactor, clock):
         self.handler._sso_handler.render_error = self.render_error
 
         # Reduce the number of attempts when generating MXIDs.
-        self.handler._MAP_USERNAME_RETRIES = 3
+        self.handler._sso_handler._MAP_USERNAME_RETRIES = 3
 
         return hs
 
@@ -704,7 +704,9 @@ def test_map_userinfo_to_user(self):
             ),
             MappingException,
         )
-        self.assertEqual(str(e.value), "mxid '@test_user_3:test' is already taken")
+        self.assertEqual(
+            str(e.value), "Unable to generate a Matrix ID from the SSO response"
+        )
 
     @override_config({"oidc_config": {"allow_existing_users": True}})
     def test_map_userinfo_to_existing_user(self):
@@ -850,5 +852,5 @@ def test_map_userinfo_to_user_retries(self):
             MappingException,
         )
         self.assertEqual(
-            str(e.value), "Unable to generate a Matrix ID from the OpenID response"
+            str(e.value), "Unable to generate a Matrix ID from the SSO response"
         )

From 7e6a54933008f4c56dc0156900732bbf5ea11200 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Mon, 23 Nov 2020 10:52:13 -0500
Subject: [PATCH 04/16] Do not pass through parameters to the mapper.

---
 synapse/handlers/oidc_handler.py |  4 +---
 synapse/handlers/saml_handler.py |  8 ++------
 synapse/handlers/sso.py          | 15 ++++++---------
 3 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index 8e74ed15ffb5..a98ce652eaf6 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -872,7 +872,7 @@ async def _map_userinfo_to_user(
         )
         supports_failures = "failures" in mapper_args.args
 
-        async def oidc_response_to_user_attributes(userinfo, token, failures):
+        async def oidc_response_to_user_attributes(failures):
             """
             Call the mapping provider to map the OIDC userinfo and token to user attributes.
 
@@ -897,8 +897,6 @@ async def oidc_response_to_user_attributes(userinfo, token, failures):
             user_agent,
             ip_address,
             oidc_response_to_user_attributes,
-            userinfo,
-            token,
         )
 
 
diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py
index 8de18960dabb..87f648e4ead1 100644
--- a/synapse/handlers/saml_handler.py
+++ b/synapse/handlers/saml_handler.py
@@ -249,9 +249,7 @@ async def _map_saml_response_to_user(
                 "Failed to extract remote user id from SAML response"
             )
 
-        async def saml_response_to_remapped_user_attributes(
-            saml_response, client_redirect_url, failures
-        ):
+        async def saml_response_to_remapped_user_attributes(failures):
             """
             Call the mapping provider to map a SAML response to user attributes and coerce the result into the standard form.
 
@@ -259,7 +257,7 @@ async def saml_response_to_remapped_user_attributes(
             """
             # Remap the arguments to the mapping provider.
             result = self._user_mapping_provider.saml_response_to_user_attributes(
-                saml_response, failures, client_redirect_url
+                saml2_auth, failures, client_redirect_url
             )
             # Remap some of the results.
             if "mxid_localpart" in result:
@@ -301,8 +299,6 @@ async def saml_response_to_remapped_user_attributes(
                 user_agent,
                 ip_address,
                 saml_response_to_remapped_user_attributes,
-                saml2_auth,
-                client_redirect_url,
             )
 
     def expire_sessions(self):
diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py
index b152bb26aea4..8a07e97ef2fe 100644
--- a/synapse/handlers/sso.py
+++ b/synapse/handlers/sso.py
@@ -13,7 +13,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 import logging
-from typing import TYPE_CHECKING, Any, Awaitable, Callable, Optional
+from typing import TYPE_CHECKING, Awaitable, Callable, Optional
 
 from synapse.handlers._base import BaseHandler
 from synapse.http.server import respond_with_html
@@ -106,9 +106,7 @@ async def get_mxid_from_sso(
         remote_user_id: str,
         user_agent: str,
         ip_address: str,
-        mapper: "Callable[..., Awaitable[str]]",
-        *args: Any,
-        **kwargs: Any,
+        sso_to_matrix_id_mapper: Callable[[int], Awaitable[dict]],
     ):
         """
 
@@ -118,10 +116,9 @@ async def get_mxid_from_sso(
             remote_user_id: The unique identifier from the SSO provider.
             user_agent: The user agent of the client making the request.
             ip_address: The IP address of the client making the request.
-            mapper: A callable to generate the user attributes. Note that the
-                current number of failures is passed in as the last argument.
-            args: Additional arguments for mapper.
-            kwargs: Additional keyword arguments for mapper.
+            sso_to_matrix_id_mapper: A callable to generate the user attributes.
+                The only parameter is an integer which represents the amount of
+                times the returned mxid localpart mapping has failed.
 
         Returns:
              The user ID associated with the SSO response.
@@ -142,7 +139,7 @@ async def get_mxid_from_sso(
         # Otherwise, generate a new user.
         for i in range(self._MAP_USERNAME_RETRIES):
             try:
-                attributes = await mapper(*args, i, **kwargs)
+                attributes = await sso_to_matrix_id_mapper(i)
             except Exception as e:
                 raise MappingException(
                     "Could not extract user attributes from SSO response: " + str(e)

From c34e97356d867f575bd78e3a4cc90860eb6a1eab Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Mon, 23 Nov 2020 11:05:31 -0500
Subject: [PATCH 05/16] Do not retry Matrix IDs which will keep failing.

---
 synapse/handlers/oidc_handler.py | 6 ++++++
 tests/handlers/test_oidc.py      | 3 ++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index a98ce652eaf6..b3482221b5a3 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -883,6 +883,12 @@ async def oidc_response_to_user_attributes(failures):
                     userinfo, token, failures
                 )
             else:
+                # If the mapping provider does not support processing failures,
+                # do not continually generate the same Matrix ID since this will
+                # likely continue to fail.
+                if failures:
+                    raise RuntimeError("Mapping provider does not support de-duplicating Matrix IDs")
+
                 attributes = await self._user_mapping_provider.map_user_attributes(  # type: ignore
                     userinfo, token
                 )
diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index 9180b9aaa623..e880d32be687 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -705,7 +705,8 @@ def test_map_userinfo_to_user(self):
             MappingException,
         )
         self.assertEqual(
-            str(e.value), "Unable to generate a Matrix ID from the SSO response"
+            str(e.value),
+            "Could not extract user attributes from SSO response: Mapping provider does not support de-duplicating Matrix IDs",
         )
 
     @override_config({"oidc_config": {"allow_existing_users": True}})

From 1f64e022f7ab958050c1557aeb8f1080ee4d5336 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Mon, 23 Nov 2020 11:33:17 -0500
Subject: [PATCH 06/16] Support matching an existing username for OIDC.

---
 synapse/handlers/oidc_handler.py |  7 ++++---
 synapse/handlers/sso.py          | 29 +++++++++++++++++++++++++++--
 2 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index b3482221b5a3..641152058c9a 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -887,7 +887,9 @@ async def oidc_response_to_user_attributes(failures):
                 # do not continually generate the same Matrix ID since this will
                 # likely continue to fail.
                 if failures:
-                    raise RuntimeError("Mapping provider does not support de-duplicating Matrix IDs")
+                    raise RuntimeError(
+                        "Mapping provider does not support de-duplicating Matrix IDs"
+                    )
 
                 attributes = await self._user_mapping_provider.map_user_attributes(  # type: ignore
                     userinfo, token
@@ -895,14 +897,13 @@ async def oidc_response_to_user_attributes(failures):
 
             return attributes
 
-        # TODO Support existing users.
-
         return await self._sso_handler.get_mxid_from_sso(
             self._auth_provider_id,
             remote_user_id,
             user_agent,
             ip_address,
             oidc_response_to_user_attributes,
+            self._allow_existing_users,
         )
 
 
diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py
index 8a07e97ef2fe..b49f25b53ef7 100644
--- a/synapse/handlers/sso.py
+++ b/synapse/handlers/sso.py
@@ -13,7 +13,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 import logging
-from typing import TYPE_CHECKING, Awaitable, Callable, Optional
+from typing import TYPE_CHECKING, Awaitable, Callable, List, Optional
 
 from synapse.handlers._base import BaseHandler
 from synapse.http.server import respond_with_html
@@ -107,6 +107,7 @@ async def get_mxid_from_sso(
         user_agent: str,
         ip_address: str,
         sso_to_matrix_id_mapper: Callable[[int], Awaitable[dict]],
+        allow_existing_users: bool = False,
     ):
         """
 
@@ -119,6 +120,8 @@ async def get_mxid_from_sso(
             sso_to_matrix_id_mapper: A callable to generate the user attributes.
                 The only parameter is an integer which represents the amount of
                 times the returned mxid localpart mapping has failed.
+            allow_existing_users: True if the localpart returned from the
+                mapping provider can be linked to an existing matrix ID.
 
         Returns:
              The user ID associated with the SSO response.
@@ -164,7 +167,29 @@ async def get_mxid_from_sso(
 
             # Check if this mxid already exists
             user_id = UserID(localpart, self.server_name).to_string()
-            if not await self.store.get_users_by_id_case_insensitive(user_id):
+            users = await self.store.get_users_by_id_case_insensitive(user_id)
+            if users and allow_existing_users:
+                # If an existing matrix ID is returned, then use it.
+                if len(users) == 1:
+                    previously_registered_user_id = next(iter(users))
+                elif user_id in users:
+                    previously_registered_user_id = user_id
+                else:
+                    # Do not attempt to continue generating Matrix IDs.
+                    raise MappingException(
+                        "Attempted to login as '{}' but it matches more than one user inexactly: {}".format(
+                            user_id, users
+                        )
+                    )
+
+                # Future logins should also match this user ID.
+                await self.store.record_user_external_id(
+                    auth_provider_id, remote_user_id, previously_registered_user_id
+                )
+
+                return previously_registered_user_id
+
+            elif not users:
                 # This mxid is free
                 break
         else:

From eda3e50c0f70641318ed3bc30845fcdb89e89d85 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Mon, 23 Nov 2020 13:12:27 -0500
Subject: [PATCH 07/16] Clarify some of the type hints.

---
 synapse/handlers/oidc_handler.py | 16 ++++++++--------
 synapse/handlers/saml_handler.py | 16 +++++++++-------
 synapse/handlers/sso.py          | 32 ++++++++++++++++++--------------
 3 files changed, 35 insertions(+), 29 deletions(-)

diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index 641152058c9a..55153daee3a5 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -36,7 +36,7 @@
 
 from synapse.config import ConfigError
 from synapse.handlers._base import BaseHandler
-from synapse.handlers.sso import MappingException
+from synapse.handlers.sso import MappingException, UserAttributes
 from synapse.http.site import SynapseRequest
 from synapse.logging.context import make_deferred_yieldable
 from synapse.types import JsonDict, map_username_to_mxid_localpart
@@ -872,7 +872,7 @@ async def _map_userinfo_to_user(
         )
         supports_failures = "failures" in mapper_args.args
 
-        async def oidc_response_to_user_attributes(failures):
+        async def oidc_response_to_user_attributes(failures: int) -> UserAttributes:
             """
             Call the mapping provider to map the OIDC userinfo and token to user attributes.
 
@@ -895,7 +895,7 @@ async def oidc_response_to_user_attributes(failures):
                     userinfo, token
                 )
 
-            return attributes
+            return UserAttributes(**attributes)
 
         return await self._sso_handler.get_mxid_from_sso(
             self._auth_provider_id,
@@ -907,8 +907,8 @@ async def oidc_response_to_user_attributes(failures):
         )
 
 
-UserAttribute = TypedDict(
-    "UserAttribute", {"localpart": str, "display_name": Optional[str]}
+UserAttributeDict = TypedDict(
+    "UserAttributeDict", {"localpart": str, "display_name": Optional[str]}
 )
 C = TypeVar("C")
 
@@ -952,7 +952,7 @@ def get_remote_user_id(self, userinfo: UserInfo) -> str:
 
     async def map_user_attributes(
         self, userinfo: UserInfo, token: Token, failures: int
-    ) -> UserAttribute:
+    ) -> UserAttributeDict:
         """Map a `UserInfo` object into user attributes.
 
         Args:
@@ -1060,7 +1060,7 @@ def get_remote_user_id(self, userinfo: UserInfo) -> str:
 
     async def map_user_attributes(
         self, userinfo: UserInfo, token: Token, failures: int
-    ) -> UserAttribute:
+    ) -> UserAttributeDict:
         localpart = self._config.localpart_template.render(user=userinfo).strip()
 
         # Ensure only valid characters are included in the MXID.
@@ -1075,7 +1075,7 @@ async def map_user_attributes(
             if display_name == "":
                 display_name = None
 
-        return UserAttribute(localpart=localpart, display_name=display_name)
+        return UserAttributeDict(localpart=localpart, display_name=display_name)
 
     async def get_extra_attributes(self, userinfo: UserInfo, token: Token) -> JsonDict:
         extras = {}  # type: Dict[str, str]
diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py
index 87f648e4ead1..1b6be6a4cfdd 100644
--- a/synapse/handlers/saml_handler.py
+++ b/synapse/handlers/saml_handler.py
@@ -25,7 +25,7 @@
 from synapse.config import ConfigError
 from synapse.config.saml2_config import SamlAttributeRequirement
 from synapse.handlers._base import BaseHandler
-from synapse.handlers.sso import MappingException
+from synapse.handlers.sso import MappingException, UserAttributes
 from synapse.http.servlet import parse_string
 from synapse.http.site import SynapseRequest
 from synapse.module_api import ModuleApi
@@ -249,7 +249,9 @@ async def _map_saml_response_to_user(
                 "Failed to extract remote user id from SAML response"
             )
 
-        async def saml_response_to_remapped_user_attributes(failures):
+        async def saml_response_to_remapped_user_attributes(
+            failures: int,
+        ) -> UserAttributes:
             """
             Call the mapping provider to map a SAML response to user attributes and coerce the result into the standard form.
 
@@ -260,11 +262,11 @@ async def saml_response_to_remapped_user_attributes(failures):
                 saml2_auth, failures, client_redirect_url
             )
             # Remap some of the results.
-            if "mxid_localpart" in result:
-                result["localpart"] = result.pop("mxid_localpart")
-            if "displayname" in result:
-                result["display_name"] = result.pop("displayname")
-            return result
+            return UserAttributes(
+                localpart=result.get("mxid_localpart"),
+                display_name=result.get("displayname"),
+                emails=result.get("emails"),
+            )
 
         with (await self._mapping_lock.queue(self._auth_provider_id)):
             # backwards-compatibility hack: see if there is an existing user with a
diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py
index b49f25b53ef7..a77c4d003a5e 100644
--- a/synapse/handlers/sso.py
+++ b/synapse/handlers/sso.py
@@ -15,6 +15,8 @@
 import logging
 from typing import TYPE_CHECKING, Awaitable, Callable, List, Optional
 
+import attr
+
 from synapse.handlers._base import BaseHandler
 from synapse.http.server import respond_with_html
 from synapse.types import UserID, contains_invalid_mxid_characters
@@ -30,6 +32,13 @@ class MappingException(Exception):
     """
 
 
+@attr.s
+class UserAttributes:
+    localpart = attr.ib(type=str)
+    display_name = attr.ib(type=Optional[str], default=None)
+    emails = attr.ib(type=List[str], default=attr.Factory(list))
+
+
 class SsoHandler(BaseHandler):
     # The number of attempts to ask the mapping provider for when generating an MXID.
     _MAP_USERNAME_RETRIES = 1000
@@ -106,7 +115,7 @@ async def get_mxid_from_sso(
         remote_user_id: str,
         user_agent: str,
         ip_address: str,
-        sso_to_matrix_id_mapper: Callable[[int], Awaitable[dict]],
+        sso_to_matrix_id_mapper: Callable[[int], Awaitable[UserAttributes]],
         allow_existing_users: bool = False,
     ):
         """
@@ -154,19 +163,14 @@ async def get_mxid_from_sso(
                 i,
             )
 
-            localpart = attributes["localpart"]
-            if not localpart:
+            if not attributes.localpart:
                 raise MappingException(
                     "Error parsing SSO response: SSO mapping provider plugin "
                     "did not return a localpart value"
                 )
 
-            # Other potential attributes returned by the mapping provider.
-            display_name = attributes.get("display_name")
-            emails = attributes.get("emails", [])
-
             # Check if this mxid already exists
-            user_id = UserID(localpart, self.server_name).to_string()
+            user_id = UserID(attributes.localpart, self.server_name).to_string()
             users = await self.store.get_users_by_id_case_insensitive(user_id)
             if users and allow_existing_users:
                 # If an existing matrix ID is returned, then use it.
@@ -201,14 +205,14 @@ async def get_mxid_from_sso(
 
         # 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,))
+        if contains_invalid_mxid_characters(attributes.localpart):
+            raise MappingException("localpart is invalid: %s" % (attributes.localpart,))
 
-        logger.debug("Mapped SAML user to local part %s", localpart)
+        logger.debug("Mapped SAML user to local part %s", attributes.localpart)
         registered_user_id = await self._registration_handler.register_user(
-            localpart=localpart,
-            default_display_name=display_name,
-            bind_emails=emails,
+            localpart=attributes.localpart,
+            default_display_name=attributes.display_name,
+            bind_emails=attributes.emails,
             user_agent_ips=(user_agent, ip_address),
         )
 

From d3c4e04dc4a640eb1c22ae0b7a61c7081cddff7d Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Mon, 23 Nov 2020 14:00:12 -0500
Subject: [PATCH 08/16] Add a newsfragment.

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

diff --git a/changelog.d/8801.feature b/changelog.d/8801.feature
new file mode 100644
index 000000000000..13af284d7acf
--- /dev/null
+++ b/changelog.d/8801.feature
@@ -0,0 +1 @@
+Support re-trying generating a localpart for OpenID Connect mapping providers.

From 0ded8b107fc5d120509422de110d99383ae21bd8 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Mon, 23 Nov 2020 14:08:50 -0500
Subject: [PATCH 09/16] Fix some comments.

---
 docs/sso_mapping_providers.md    | 2 +-
 synapse/handlers/saml_handler.py | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/sso_mapping_providers.md b/docs/sso_mapping_providers.md
index 712ad9373fe9..dee53b5d40e5 100644
--- a/docs/sso_mapping_providers.md
+++ b/docs/sso_mapping_providers.md
@@ -63,7 +63,7 @@ A custom mapping provider must specify the following methods:
                      information from.
     - This method must return a string, which is the unique identifier for the
       user. Commonly the ``sub`` claim of the response.
-* `map_user_attributes(self, userinfo, token)`
+* `map_user_attributes(self, userinfo, token, failures)`
     - This method must be async.
     - Arguments:
       - `userinfo` - A `authlib.oidc.core.claims.UserInfo` object to extract user
diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py
index 378f2f90905d..03b7d5374c0f 100644
--- a/synapse/handlers/saml_handler.py
+++ b/synapse/handlers/saml_handler.py
@@ -257,7 +257,7 @@ async def saml_response_to_remapped_user_attributes(
 
             This is backwards compatibility for abstraction for the SSO handler.
             """
-            # Remap the arguments to the mapping provider.
+            # Call the mapping provider.
             result = self._user_mapping_provider.saml_response_to_user_attributes(
                 saml2_auth, failures, client_redirect_url
             )

From c29d8de805332b7c7a48a843810db405fc8c5b5e Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Mon, 23 Nov 2020 14:10:48 -0500
Subject: [PATCH 10/16] Remove a reference to SAML in the abstract code.

---
 synapse/handlers/sso.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py
index c5eaafe8a67f..e02b5b740f50 100644
--- a/synapse/handlers/sso.py
+++ b/synapse/handlers/sso.py
@@ -208,7 +208,7 @@ async def get_mxid_from_sso(
         if contains_invalid_mxid_characters(attributes.localpart):
             raise MappingException("localpart is invalid: %s" % (attributes.localpart,))
 
-        logger.debug("Mapped SAML user to local part %s", attributes.localpart)
+        logger.debug("Mapped SSO user to local part %s", attributes.localpart)
         registered_user_id = await self._registration_handler.register_user(
             localpart=attributes.localpart,
             default_display_name=attributes.display_name,

From 512529fa4fd2d66d0cd5bb1e6619782edca32f24 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <clokep@users.noreply.github.com>
Date: Tue, 24 Nov 2020 07:40:25 -0500
Subject: [PATCH 11/16] Update the changelog based on feedback.

Co-authored-by: Erik Johnston <erik@matrix.org>
---
 changelog.d/8801.feature | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/changelog.d/8801.feature b/changelog.d/8801.feature
index 13af284d7acf..77f7fe4e5d51 100644
--- a/changelog.d/8801.feature
+++ b/changelog.d/8801.feature
@@ -1 +1 @@
-Support re-trying generating a localpart for OpenID Connect mapping providers.
+Add support for re-trying generation of a localpart for OpenID Connect mapping providers.

From 9c5d0f67d64d15f4d630a41a24e89de637a41997 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 24 Nov 2020 08:01:01 -0500
Subject: [PATCH 12/16] Expand comments and add a return type.

---
 synapse/handlers/sso.py | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py
index e02b5b740f50..ba361f14722b 100644
--- a/synapse/handlers/sso.py
+++ b/synapse/handlers/sso.py
@@ -117,8 +117,31 @@ async def get_mxid_from_sso(
         ip_address: str,
         sso_to_matrix_id_mapper: Callable[[int], Awaitable[UserAttributes]],
         allow_existing_users: bool = False,
-    ):
+    ) -> str:
         """
+        Given an SSO ID, retrieve the user ID for it and possibly register the user.
+
+        This first checks if the SSO ID has previously been linked to a matrix ID,
+        if it has that matrix ID is returned regardless of the current mapping
+        logic.
+
+        The mapping function is called (potentially multiple times) to generate
+        a localpart for the user.
+
+        If an unused localpart is generated, the user is registered from the
+        given user-agent and IP address and the SSO ID is linked to this matrix
+        ID for subsequent calls.
+
+        If allow_existing_users is true the mapping function is only called once
+        and results in:
+
+            1. The use of a previously registered matrix ID. In this case, the
+               SSO ID is linked to the matrix ID. (Note it is possible that
+               other SSO IDs are linked to the same matrix ID.)
+            2. An unused localpart, in which case the user is registered (as
+               discussed above).
+            3. An error if the generated localpart matches multiple pre-existing
+               matrix IDs. Generally this should not happen.
 
         Args:
             auth_provider_id: A unique identifier for this SSO provider, e.g.

From 1934c10c0d173026f519e1e32e2da7e976b9378b Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 24 Nov 2020 08:11:20 -0500
Subject: [PATCH 13/16] Append the number of failures by default when mapping
 OIDC IDs.

---
 synapse/handlers/oidc_handler.py | 4 ++++
 synapse/handlers/saml_handler.py | 6 +++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index 55153daee3a5..1705b580ff2c 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -1066,6 +1066,10 @@ async def map_user_attributes(
         # Ensure only valid characters are included in the MXID.
         localpart = map_username_to_mxid_localpart(localpart)
 
+        # Append suffix integer if last call to this function failed to produce
+        # a usable mxid.
+        localpart += str(failures) if failures else ""
+
         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 03b7d5374c0f..34db10ffe43e 100644
--- a/synapse/handlers/saml_handler.py
+++ b/synapse/handlers/saml_handler.py
@@ -416,11 +416,11 @@ def saml_response_to_user_attributes(
             )
 
         # Use the configured mapper for this mxid_source
-        base_mxid_localpart = self._mxid_mapper(mxid_source)
+        localpart = self._mxid_mapper(mxid_source)
 
         # Append suffix integer if last call to this function failed to produce
-        # a usable mxid
-        localpart = base_mxid_localpart + (str(failures) if failures else "")
+        # a usable mxid.
+        localpart += str(failures) if failures else ""
 
         # Retrieve the display name from the saml response
         # If displayname is None, the mxid_localpart will be used instead

From 89f18693673fa602852f2e3ca64c702c7dde71a3 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 24 Nov 2020 08:18:26 -0500
Subject: [PATCH 14/16] Do not use a deprecated function.

---
 synapse/handlers/oidc_handler.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index 1705b580ff2c..74e3ce7920d4 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -867,10 +867,10 @@ async def _map_userinfo_to_user(
 
         # Older mapping providers don't accept the `failures` argument, so we
         # try and detect support.
-        mapper_args = inspect.getfullargspec(
+        mapper_signature = inspect.signature(
             self._user_mapping_provider.map_user_attributes
         )
-        supports_failures = "failures" in mapper_args.args
+        supports_failures = "failures" in mapper_signature.parameters
 
         async def oidc_response_to_user_attributes(failures: int) -> UserAttributes:
             """

From 6d11ff81607bb5f9ad33b4a760ecb4cf55380765 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Wed, 25 Nov 2020 08:40:20 -0500
Subject: [PATCH 15/16] Add an additional comment.

---
 synapse/handlers/sso.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py
index ba361f14722b..d963082210f0 100644
--- a/synapse/handlers/sso.py
+++ b/synapse/handlers/sso.py
@@ -195,6 +195,10 @@ async def get_mxid_from_sso(
             # Check if this mxid already exists
             user_id = UserID(attributes.localpart, self.server_name).to_string()
             users = await self.store.get_users_by_id_case_insensitive(user_id)
+            # Note, if allow_existing_users is true then the loop is guaranteed
+            # to end on the first iteration: either by matching an existing user,
+            # raising an error, or registering a new user. See the docstring for
+            # more in-depth an explanation.
             if users and allow_existing_users:
                 # If an existing matrix ID is returned, then use it.
                 if len(users) == 1:

From 18fcaee858b113adf443db6b12e16f22b6fd3289 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Wed, 25 Nov 2020 08:43:12 -0500
Subject: [PATCH 16/16] Clarify another comment.

---
 synapse/handlers/oidc_handler.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index 74e3ce7920d4..78c4e94a9d1d 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -884,8 +884,9 @@ async def oidc_response_to_user_attributes(failures: int) -> UserAttributes:
                 )
             else:
                 # If the mapping provider does not support processing failures,
-                # do not continually generate the same Matrix ID since this will
-                # likely continue to fail.
+                # do not continually generate the same Matrix ID since it will
+                # continue to already be in use. Note that the error raised is
+                # arbitrary and will get turned into a MappingException.
                 if failures:
                     raise RuntimeError(
                         "Mapping provider does not support de-duplicating Matrix IDs"