Fix a regression that mapping providers should be able to redirect us…

…ers. (#8878)

This was broken in #8801.

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.

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
 

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

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,10 +190,16 @@ 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.
  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)",

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

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/")