From c21bdc813f5c21153cded05bcd0a57b5836f09fe Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 2 Dec 2020 07:09:21 -0500 Subject: Add basic SAML tests for mapping users. (#8800) --- tests/handlers/test_saml.py | 136 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 136 insertions(+) create mode 100644 tests/handlers/test_saml.py (limited to 'tests/handlers/test_saml.py') diff --git a/tests/handlers/test_saml.py b/tests/handlers/test_saml.py new file mode 100644 index 0000000000..79fd47036f --- /dev/null +++ b/tests/handlers/test_saml.py @@ -0,0 +1,136 @@ +# 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. + +import attr + +from synapse.handlers.sso import MappingException + +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 + ): + localpart = saml_response.ava["username"] + (str(failures) if failures else "") + return {"mxid_localpart": localpart, "displayname": None} + + +class SamlHandlerTestCase(HomeserverTestCase): + def default_config(self): + config = super().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 + + return config + + def make_homeserver(self, reactor, clock): + hs = self.setup_test_homeserver() + + self.handler = hs.get_saml_handler() + + # Reduce the number of attempts when generating MXIDs. + sso_handler = hs.get_sso_handler() + sso_handler._MAP_USERNAME_RETRIES = 3 + + 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") + + 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": "test", "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öö") + + def test_map_saml_response_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) + ) + saml_response = FakeAuthnResponse({"uid": "test", "username": "test_user"}) + redirect_url = "" + mxid = self.get_success( + self.handler._map_saml_response_to_user( + saml_response, redirect_url, "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 mxids for a particular SAML 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. + saml_response = FakeAuthnResponse({"uid": "tester", "username": "tester"}) + 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 SSO response" + ) -- cgit 1.5.1 From 8388384a640d3381b5858d3fb1d2ea0a8c9c059c Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 2 Dec 2020 07:45:42 -0500 Subject: Fix a regression when grandfathering SAML users. (#8855) This was broken in #8801 when abstracting code shared with OIDC. After this change both SAML and OIDC have a concept of grandfathering users, but with different implementations. --- changelog.d/8855.feature | 1 + synapse/handlers/oidc_handler.py | 30 ++++++++++++++++++-- synapse/handlers/saml_handler.py | 9 +++--- synapse/handlers/sso.py | 60 +++++++++++++--------------------------- tests/handlers/test_oidc.py | 8 ++++++ tests/handlers/test_saml.py | 34 ++++++++++++++++++++++- 6 files changed, 94 insertions(+), 48 deletions(-) create mode 100644 changelog.d/8855.feature (limited to 'tests/handlers/test_saml.py') diff --git a/changelog.d/8855.feature b/changelog.d/8855.feature new file mode 100644 index 0000000000..77f7fe4e5d --- /dev/null +++ b/changelog.d/8855.feature @@ -0,0 +1 @@ +Add support for re-trying generation of a localpart for OpenID Connect mapping providers. diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index 78c4e94a9d..55c4377890 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -39,7 +39,7 @@ from synapse.handlers._base import BaseHandler 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: @@ -898,13 +898,39 @@ class OidcHandler(BaseHandler): 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. + attributes = await oidc_response_to_user_attributes(failures=0) + + 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, ) diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py index 7ffad7d8af..76d4169fe2 100644 --- a/synapse/handlers/saml_handler.py +++ b/synapse/handlers/saml_handler.py @@ -268,7 +268,7 @@ class SamlHandler(BaseHandler): 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 @@ class SamlHandler(BaseHandler): 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): diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index d963082210..f42b90e1bc 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -116,7 +116,7 @@ class SsoHandler(BaseHandler): 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,6 +125,10 @@ class SsoHandler(BaseHandler): 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. @@ -132,17 +136,6 @@ class SsoHandler(BaseHandler): 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 @@ class SsoHandler(BaseHandler): 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 @@ class SsoHandler(BaseHandler): 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 @@ class SsoHandler(BaseHandler): # 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: diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index c9807a7b73..d485af52fd 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -731,6 +731,14 @@ class OidcHandlerTestCase(HomeserverTestCase): ) 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 diff --git a/tests/handlers/test_saml.py b/tests/handlers/test_saml.py index 79fd47036f..e1e13a5faf 100644 --- a/tests/handlers/test_saml.py +++ b/tests/handlers/test_saml.py @@ -16,7 +16,7 @@ import attr from synapse.handlers.sso import MappingException -from tests.unittest import HomeserverTestCase +from tests.unittest import HomeserverTestCase, override_config # These are a few constants that are used as config parameters in the tests. BASE_URL = "https://synapse/" @@ -59,6 +59,10 @@ class SamlHandlerTestCase(HomeserverTestCase): "grandfathered_mxid_source_attribute": None, "user_mapping_provider": {"module": __name__ + ".TestMappingProvider"}, } + + # Update this config with what's in the default config so that + # override_config works as expected. + saml_config.update(config.get("saml2_config", {})) config["saml2_config"] = saml_config return config @@ -86,6 +90,34 @@ class SamlHandlerTestCase(HomeserverTestCase): ) self.assertEqual(mxid, "@test_user:test") + @override_config({"saml2_config": {"grandfathered_mxid_source_attribute": "mxid"}}) + def test_map_saml_response_to_existing_user(self): + """Existing users can log in with SAML account.""" + store = self.hs.get_datastore() + self.get_success( + store.register_user(user_id="@test_user:test", password_hash=None) + ) + + # Map a user via SSO. + saml_response = FakeAuthnResponse( + {"uid": "tester", "mxid": ["test_user"], "username": "test_user"} + ) + 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") + + # Subsequent calls should map to the same mxid. + 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") + 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": "test", "username": "föö"}) -- cgit 1.5.1 From 22c6c19f91d7325c82eddfada696826adad69e5b Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 4 Dec 2020 08:25:15 -0500 Subject: Fix a regression that mapping providers should be able to redirect users. (#8878) This was broken in #8801. --- changelog.d/8878.bugfix | 1 + docs/sso_mapping_providers.md | 7 +++++++ synapse/handlers/oidc_handler.py | 2 +- synapse/handlers/sso.py | 27 ++++++++++++++++++++++----- tests/handlers/test_oidc.py | 3 +-- tests/handlers/test_saml.py | 28 ++++++++++++++++++++++++++++ 6 files changed, 60 insertions(+), 8 deletions(-) create mode 100644 changelog.d/8878.bugfix (limited to 'tests/handlers/test_saml.py') diff --git a/changelog.d/8878.bugfix b/changelog.d/8878.bugfix new file mode 100644 index 0000000000..e53005ee1c --- /dev/null +++ b/changelog.d/8878.bugfix @@ -0,0 +1 @@ +Fix a regression in v1.24.0rc1 which failed to allow SAML mapping providers which were unable to redirect users to an additional page. diff --git a/docs/sso_mapping_providers.md b/docs/sso_mapping_providers.md index dee53b5d40..ab2a648910 100644 --- a/docs/sso_mapping_providers.md +++ b/docs/sso_mapping_providers.md @@ -168,6 +168,13 @@ A custom mapping provider must specify the following methods: the value of `mxid_localpart`. * `emails` - A list of emails for the new user. If not provided, will default to an empty list. + + Alternatively it can raise a `synapse.api.errors.RedirectException` to + redirect the user to another page. This is useful to prompt the user for + additional information, e.g. if you want them to provide their own username. + It is the responsibility of the mapping provider to either redirect back + to `client_redirect_url` (including any additional information) or to + complete registration using methods from the `ModuleApi`. ### Default SAML Mapping Provider diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index 55c4377890..c605f7082a 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -888,7 +888,7 @@ class OidcHandler(BaseHandler): # continue to already be in use. Note that the error raised is # arbitrary and will get turned into a MappingException. if failures: - raise RuntimeError( + raise MappingException( "Mapping provider does not support de-duplicating Matrix IDs" ) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index f42b90e1bc..47ad96f97e 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -17,6 +17,7 @@ from typing import TYPE_CHECKING, Awaitable, Callable, List, Optional import attr +from synapse.api.errors import RedirectException from synapse.handlers._base import BaseHandler from synapse.http.server import respond_with_html from synapse.types import UserID, contains_invalid_mxid_characters @@ -28,7 +29,9 @@ logger = logging.getLogger(__name__) class MappingException(Exception): - """Used to catch errors when mapping the UserInfo object + """Used to catch errors when mapping an SSO response to user attributes. + + Note that the msg that is raised is shown to end-users. """ @@ -145,6 +148,14 @@ class SsoHandler(BaseHandler): 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. + + It is expected that the mapper can raise two exceptions, which + will get passed through to the caller: + + MappingException if there was a problem mapping the response + to the user. + RedirectException to redirect to an additional page (e.g. + to prompt the user for more information). grandfather_existing_users: A callable which can return an previously existing matrix ID. The SSO ID is then linked to the returned matrix ID. @@ -154,8 +165,8 @@ class SsoHandler(BaseHandler): 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. + RedirectException: if the mapping provider needs to redirect the user + to an additional page. (e.g. to prompt for more information) """ # first of all, check if we already have a mapping for this user @@ -179,10 +190,16 @@ class SsoHandler(BaseHandler): for i in range(self._MAP_USERNAME_RETRIES): try: attributes = await sso_to_matrix_id_mapper(i) + except (RedirectException, MappingException): + # Mapping providers are allowed to issue a redirect (e.g. to ask + # the user for more information) and can issue a mapping exception + # if a name cannot be generated. + raise except Exception as e: + # Any other exception is unexpected. raise MappingException( - "Could not extract user attributes from SSO response: " + str(e) - ) + "Could not extract user attributes from SSO response." + ) from e logger.debug( "Retrieved user attributes from user mapping provider: %r (attempt %d)", diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index d485af52fd..a308c46da9 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -705,8 +705,7 @@ class OidcHandlerTestCase(HomeserverTestCase): MappingException, ) self.assertEqual( - str(e.value), - "Could not extract user attributes from SSO response: Mapping provider does not support de-duplicating Matrix IDs", + str(e.value), "Mapping provider does not support de-duplicating Matrix IDs", ) @override_config({"oidc_config": {"allow_existing_users": True}}) diff --git a/tests/handlers/test_saml.py b/tests/handlers/test_saml.py index e1e13a5faf..45dc17aba5 100644 --- a/tests/handlers/test_saml.py +++ b/tests/handlers/test_saml.py @@ -14,6 +14,7 @@ import attr +from synapse.api.errors import RedirectException from synapse.handlers.sso import MappingException from tests.unittest import HomeserverTestCase, override_config @@ -49,6 +50,13 @@ class TestMappingProvider: return {"mxid_localpart": localpart, "displayname": None} +class TestRedirectMappingProvider(TestMappingProvider): + def saml_response_to_user_attributes( + self, saml_response, failures, client_redirect_url + ): + raise RedirectException(b"https://custom-saml-redirect/") + + class SamlHandlerTestCase(HomeserverTestCase): def default_config(self): config = super().default_config() @@ -166,3 +174,23 @@ class SamlHandlerTestCase(HomeserverTestCase): self.assertEqual( str(e.value), "Unable to generate a Matrix ID from the SSO response" ) + + @override_config( + { + "saml2_config": { + "user_mapping_provider": { + "module": __name__ + ".TestRedirectMappingProvider" + }, + } + } + ) + def test_map_saml_response_redirect(self): + saml_response = FakeAuthnResponse({"uid": "test", "username": "test_user"}) + redirect_url = "" + e = self.get_failure( + self.handler._map_saml_response_to_user( + saml_response, redirect_url, "user-agent", "10.10.10.10" + ), + RedirectException, + ) + self.assertEqual(e.value.location, b"https://custom-saml-redirect/") -- cgit 1.5.1