Fix grandfathering of SAML users

.buildkite/scripts/test_old_deps.sh

@@ -6,7 +6,7 @@
 set -ex
 
 apt-get update
-apt-get install -y python3.5 python3.5-dev python3-pip libxml2-dev libxslt-dev zlib1g-dev tox
+apt-get install -y python3.5 python3.5-dev python3-pip libxml2-dev libxslt-dev xmlsec1 zlib1g-dev tox
 
 export LANG="C.UTF-8"
 

changelog.d/8800.misc

@@ -0,0 +1 @@
+Add additional error checking for OpenID Connect and SAML mapping providers.

changelog.d/8855.feature

@@ -0,0 +1 @@
+Add support for re-trying generation of a localpart for OpenID Connect mapping providers.

synapse/handlers/oidc_handler.py

@@ -39,7 +39,7 @@
 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
+from synapse.types import JsonDict, UserID, map_username_to_mxid_localpart
 from synapse.util import json_decoder
 
 if TYPE_CHECKING:
@@ -117,7 +117,6 @@ def __init__(self, hs: "HomeServer"):
         self._http_client = hs.get_proxied_http_client()
         self._auth_handler = hs.get_auth_handler()
         self._registration_handler = hs.get_registration_handler()
-        self._server_name = hs.config.server_name  # type: str
         self._macaroon_secret_key = hs.config.macaroon_secret_key
 
         # identifier for the external_ids table
@@ -737,7 +736,7 @@ def _generate_oidc_session_token(
             A signed macaroon token with the session information.
         """
         macaroon = pymacaroons.Macaroon(
-            location=self._server_name, identifier="key", key=self._macaroon_secret_key,
+            location=self.server_name, identifier="key", key=self._macaroon_secret_key,
         )
         macaroon.add_first_party_caveat("gen = 1")
         macaroon.add_first_party_caveat("type = session")
@@ -898,13 +897,40 @@ async def oidc_response_to_user_attributes(failures: int) -> UserAttributes:
 
             return UserAttributes(**attributes)
 
+        async def grandfather_existing_users() -> Optional[str]:
+            if self._allow_existing_users:
+                # If allowing existing users we want to generate a single localpart
+                # and attempt to match it.
+                failures = 0
+                attributes = await oidc_response_to_user_attributes(failures)
+
+                user_id = UserID(attributes.localpart, self.server_name).to_string()
+                users = await self.store.get_users_by_id_case_insensitive(user_id)
+                if 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
+                            )
+                        )
+
+                    return previously_registered_user_id
+
+            return None
+
         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,
+            grandfather_existing_users,
         )
 
 

synapse/handlers/saml_handler.py

@@ -265,10 +265,10 @@ async def saml_response_to_remapped_user_attributes(
             return UserAttributes(
                 localpart=result.get("mxid_localpart"),
                 display_name=result.get("displayname"),
-                emails=result.get("emails"),
+                emails=result.get("emails", []),
             )
 
-        with (await self._mapping_lock.queue(self._auth_provider_id)):
+        async def grandfather_existing_users() -> Optional[str]:
             # backwards-compatibility hack: see if there is an existing user with a
             # suitable mapping from the uid
             if (
@@ -290,17 +290,18 @@ async def saml_response_to_remapped_user_attributes(
                 if users:
                     registered_user_id = list(users.keys())[0]
                     logger.info("Grandfathering mapping to %s", registered_user_id)
-                    await self.store.record_user_external_id(
-                        self._auth_provider_id, remote_user_id, registered_user_id
-                    )
                     return registered_user_id
 
+            return None
+
+        with (await self._mapping_lock.queue(self._auth_provider_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,
+                grandfather_existing_users,
             )
 
     def expire_sessions(self):

synapse/handlers/sso.py

@@ -116,7 +116,7 @@ async def get_mxid_from_sso(
         user_agent: str,
         ip_address: str,
         sso_to_matrix_id_mapper: Callable[[int], Awaitable[UserAttributes]],
-        allow_existing_users: bool = False,
+        grandfather_existing_users: Optional[Callable[[], Awaitable[Optional[str]]]],
     ) -> str:
         """
         Given an SSO ID, retrieve the user ID for it and possibly register the user.
@@ -125,24 +125,17 @@ async def get_mxid_from_sso(
         if it has that matrix ID is returned regardless of the current mapping
         logic.
 
+        If a callable is provided for grandfathering users, it is called and can
+        potentially return a matrix ID to use. If it does, the SSO ID is linked to
+        this matrix ID for subsequent calls.
+
         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.
                 "oidc" or "saml".
@@ -152,8 +145,9 @@ 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.
+            grandfather_existing_users: A callable which can return an previously
+                existing matrix ID. The SSO ID is then linked to the returned
+                matrix ID.
 
         Returns:
              The user ID associated with the SSO response.
@@ -171,6 +165,16 @@ async def get_mxid_from_sso(
         if previously_registered_user_id:
             return previously_registered_user_id
 
+        # Check for grandfathering of users.
+        if grandfather_existing_users:
+            previously_registered_user_id = await grandfather_existing_users()
+            if previously_registered_user_id:
+                # 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
+
         # Otherwise, generate a new user.
         for i in range(self._MAP_USERNAME_RETRIES):
             try:
@@ -194,33 +198,7 @@ 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:
-                    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:
+            if not await self.store.get_users_by_id_case_insensitive(user_id):
                 # This mxid is free
                 break
         else:

tests/handlers/test_oidc.py

@@ -23,7 +23,7 @@
 from twisted.python.failure import Failure
 from twisted.web._newclient import ResponseDone
 
-from synapse.handlers.oidc_handler import OidcError, OidcHandler, OidcMappingProvider
+from synapse.handlers.oidc_handler import OidcError, OidcMappingProvider
 from synapse.handlers.sso import MappingException
 from synapse.types import UserID
 
@@ -127,13 +127,8 @@ async def get_json(url):
 
 
 class OidcHandlerTestCase(HomeserverTestCase):
-    def make_homeserver(self, reactor, clock):
-
-        self.http_client = Mock(spec=["get_json"])
-        self.http_client.get_json.side_effect = get_json
-        self.http_client.user_agent = "Synapse Test"
-
-        config = self.default_config()
+    def default_config(self):
+        config = super().default_config()
         config["public_baseurl"] = BASE_URL
         oidc_config = {
             "enabled": True,
@@ -149,19 +144,24 @@ def make_homeserver(self, reactor, clock):
         oidc_config.update(config.get("oidc_config", {}))
         config["oidc_config"] = oidc_config
 
-        hs = self.setup_test_homeserver(
-            http_client=self.http_client,
-            proxied_http_client=self.http_client,
-            config=config,
-        )
+        return config
+
+    def make_homeserver(self, reactor, clock):
 
-        self.handler = OidcHandler(hs)
+        self.http_client = Mock(spec=["get_json"])
+        self.http_client.get_json.side_effect = get_json
+        self.http_client.user_agent = "Synapse Test"
+
+        hs = self.setup_test_homeserver(proxied_http_client=self.http_client)
+
+        self.handler = hs.get_oidc_handler()
+        sso_handler = hs.get_sso_handler()
         # Mock the render error method.
         self.render_error = Mock(return_value=None)
-        self.handler._sso_handler.render_error = self.render_error
+        sso_handler.render_error = self.render_error
 
         # Reduce the number of attempts when generating MXIDs.
-        self.handler._sso_handler._MAP_USERNAME_RETRIES = 3
+        sso_handler._MAP_USERNAME_RETRIES = 3
 
         return hs
 
@@ -731,6 +731,14 @@ def test_map_userinfo_to_existing_user(self):
         )
         self.assertEqual(mxid, "@test_user:test")
 
+        # Subsequent calls should map to the same mxid.
+        mxid = self.get_success(
+            self.handler._map_userinfo_to_user(
+                userinfo, token, "user-agent", "10.10.10.10"
+            )
+        )
+        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
@@ -832,7 +840,7 @@ def test_map_userinfo_to_user_retries(self):
         # 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.
+        # Register all of the potential mxids for a particular OIDC username.
         self.get_success(
             store.register_user(user_id="@tester:test", password_hash=None)
         )