Support trying multiple localparts for OpenID Connect.

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.

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()
 

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,10 +773,63 @@ 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"
             ),
             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"
+        )