Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Commit

Permalink
Fix a regression that mapping providers should be able to redirect us…
Browse files Browse the repository at this point in the history
…ers. (#8878)

This was broken in #8801.
  • Loading branch information
clokep authored Dec 4, 2020
1 parent 295c209 commit 22c6c19
Show file tree
Hide file tree
Showing 6 changed files with 60 additions and 8 deletions.
1 change: 1 addition & 0 deletions changelog.d/8878.bugfix
Original file line number Diff line number Diff line change
@@ -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.
7 changes: 7 additions & 0 deletions docs/sso_mapping_providers.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion synapse/handlers/oidc_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down
27 changes: 22 additions & 5 deletions synapse/handlers/sso.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
"""


Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand All @@ -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)",
Expand Down
3 changes: 1 addition & 2 deletions tests/handlers/test_oidc.py
Original file line number Diff line number Diff line change
Expand Up @@ -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}})
Expand Down
28 changes: 28 additions & 0 deletions tests/handlers/test_saml.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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/")

0 comments on commit 22c6c19

Please sign in to comment.