diff --git a/changelog.d/8878.bugfix b/changelog.d/8878.bugfix new file mode 100644 index 000000000000..e53005ee1c27 --- /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 dee53b5d40e5..ab2a6489105f 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 55c437789049..c605f7082a46 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -888,7 +888,7 @@ async def oidc_response_to_user_attributes(failures: int) -> UserAttributes: # 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 f42b90e1bcfe..ad8de7fc9b21 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -17,6 +17,7 @@ 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 @@ 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 @@ 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. + + 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 @@ async def get_mxid_from_sso( 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,9 +190,19 @@ async def get_mxid_from_sso( 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. + logger.error( + "Unexpected error when extracting user attributes from SSO response: %s" + % (e,) + ) raise MappingException( - "Could not extract user attributes from SSO response: " + str(e) + "Could not extract user attributes from SSO response." ) logger.debug( diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index d485af52fd04..a308c46da9ef 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -705,8 +705,7 @@ def test_map_userinfo_to_user(self): 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 e1e13a5faffc..45dc17aba5fb 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 @@ def saml_response_to_user_attributes( 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 @@ def test_map_saml_response_to_user_retries(self): 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/")