From 6ff34e00d9bd4e8b0b91347a359b17abeaa22e5a Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 9 Dec 2020 12:23:30 -0500 Subject: Skip the SAML tests if xmlsec1 isn't available. (#8905) --- tests/handlers/test_saml.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) (limited to 'tests/handlers/test_saml.py') diff --git a/tests/handlers/test_saml.py b/tests/handlers/test_saml.py index 45dc17aba5..d21e5588ca 100644 --- a/tests/handlers/test_saml.py +++ b/tests/handlers/test_saml.py @@ -19,6 +19,24 @@ from synapse.handlers.sso import MappingException from tests.unittest import HomeserverTestCase, override_config +# Check if we have the dependencies to run the tests. +try: + import saml2.config + from saml2.sigver import SigverError + + has_saml2 = True + + # pysaml2 can be installed and imported, but might not be able to find xmlsec1. + config = saml2.config.SPConfig() + try: + config.load({"metadata": {}}) + has_xmlsec1 = True + except SigverError: + has_xmlsec1 = False +except ImportError: + has_saml2 = False + has_xmlsec1 = False + # These are a few constants that are used as config parameters in the tests. BASE_URL = "https://synapse/" @@ -86,6 +104,11 @@ class SamlHandlerTestCase(HomeserverTestCase): return hs + if not has_saml2: + skip = "Requires pysaml2" + elif not has_xmlsec1: + skip = "Requires xmlsec1" + 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"}) -- cgit 1.5.1 From 01333681bc3db22541b49c194f5121a5415731c6 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Tue, 15 Dec 2020 20:56:10 +0000 Subject: Preparatory refactoring of the SamlHandlerTestCase (#8938) * move simple_async_mock to test_utils ... so that it can be re-used * Remove references to `SamlHandler._map_saml_response_to_user` from tests This method is going away, so we can no longer use it as a test point. Instead, factor out a higher-level method which takes a SAML object, and verify correct behaviour by mocking out `AuthHandler.complete_sso_login`. * changelog --- changelog.d/8938.feature | 1 + synapse/handlers/saml_handler.py | 23 +++++++ tests/handlers/test_oidc.py | 12 +--- tests/handlers/test_saml.py | 132 ++++++++++++++++++++++++++------------- tests/test_utils/__init__.py | 12 ++++ 5 files changed, 126 insertions(+), 54 deletions(-) create mode 100644 changelog.d/8938.feature (limited to 'tests/handlers/test_saml.py') diff --git a/changelog.d/8938.feature b/changelog.d/8938.feature new file mode 100644 index 0000000000..d450ef4998 --- /dev/null +++ b/changelog.d/8938.feature @@ -0,0 +1 @@ +Add support for allowing users to pick their own user ID during a single-sign-on login. diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py index f2ca1ddb53..6001fe3e27 100644 --- a/synapse/handlers/saml_handler.py +++ b/synapse/handlers/saml_handler.py @@ -163,6 +163,29 @@ class SamlHandler(BaseHandler): return logger.debug("SAML2 response: %s", saml2_auth.origxml) + + await self._handle_authn_response(request, saml2_auth, relay_state) + + async def _handle_authn_response( + self, + request: SynapseRequest, + saml2_auth: saml2.response.AuthnResponse, + relay_state: str, + ) -> None: + """Handle an AuthnResponse, having parsed it from the request params + + Assumes that the signature on the response object has been checked. Maps + the user onto an MXID, registering them if necessary, and returns a response + to the browser. + + Args: + request: the incoming request from the browser. We'll respond to it with an + HTML page or a redirect + saml2_auth: the parsed AuthnResponse object + relay_state: the RelayState query param, which encodes the URI to rediret + back to + """ + for assertion in saml2_auth.assertions: # kibana limits the length of a log field, whereas this is all rather # useful, so split it up. diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index 9878527bab..464e569ac8 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -23,7 +23,7 @@ from synapse.handlers.oidc_handler import OidcError, OidcMappingProvider from synapse.handlers.sso import MappingException from synapse.types import UserID -from tests.test_utils import FakeResponse +from tests.test_utils import FakeResponse, simple_async_mock from tests.unittest import HomeserverTestCase, override_config # These are a few constants that are used as config parameters in the tests. @@ -82,16 +82,6 @@ class TestMappingProviderFailures(TestMappingProvider): } -def simple_async_mock(return_value=None, raises=None) -> Mock: - # AsyncMock is not available in python3.5, this mimics part of its behaviour - async def cb(*args, **kwargs): - if raises: - raise raises - return return_value - - return Mock(side_effect=cb) - - async def get_json(url): # Mock get_json calls to handle jwks & oidc discovery endpoints if url == WELL_KNOWN: diff --git a/tests/handlers/test_saml.py b/tests/handlers/test_saml.py index d21e5588ca..69927cf6be 100644 --- a/tests/handlers/test_saml.py +++ b/tests/handlers/test_saml.py @@ -12,11 +12,15 @@ # See the License for the specific language governing permissions and # limitations under the License. +from typing import Optional + +from mock import Mock + import attr from synapse.api.errors import RedirectException -from synapse.handlers.sso import MappingException +from tests.test_utils import simple_async_mock from tests.unittest import HomeserverTestCase, override_config # Check if we have the dependencies to run the tests. @@ -44,6 +48,8 @@ BASE_URL = "https://synapse/" @attr.s class FakeAuthnResponse: ava = attr.ib(type=dict) + assertions = attr.ib(type=list, factory=list) + in_response_to = attr.ib(type=Optional[str], default=None) class TestMappingProvider: @@ -111,15 +117,22 @@ class SamlHandlerTestCase(HomeserverTestCase): def test_map_saml_response_to_user(self): """Ensure that mapping the SAML response returned from a provider to an MXID works properly.""" + + # stub out the auth handler + auth_handler = self.hs.get_auth_handler() + auth_handler.complete_sso_login = simple_async_mock() + + # send a mocked-up SAML response to the callback 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" - ) + request = _mock_request() + self.get_success( + self.handler._handle_authn_response(request, saml_response, "redirect_uri") + ) + + # check that the auth handler got called as expected + auth_handler.complete_sso_login.assert_called_once_with( + "@test_user:test", request, "redirect_uri" ) - self.assertEqual(mxid, "@test_user:test") @override_config({"saml2_config": {"grandfathered_mxid_source_attribute": "mxid"}}) def test_map_saml_response_to_existing_user(self): @@ -129,53 +142,81 @@ class SamlHandlerTestCase(HomeserverTestCase): store.register_user(user_id="@test_user:test", password_hash=None) ) + # stub out the auth handler + auth_handler = self.hs.get_auth_handler() + auth_handler.complete_sso_login = simple_async_mock() + # 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" - ) + request = _mock_request() + self.get_success( + self.handler._handle_authn_response(request, saml_response, "") + ) + + # check that the auth handler got called as expected + auth_handler.complete_sso_login.assert_called_once_with( + "@test_user:test", request, "" ) - 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" - ) + auth_handler.complete_sso_login.reset_mock() + self.get_success( + self.handler._handle_authn_response(request, saml_response, "") + ) + auth_handler.complete_sso_login.assert_called_once_with( + "@test_user:test", request, "" ) - 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.""" + + # stub out the auth handler + auth_handler = self.hs.get_auth_handler() + auth_handler.complete_sso_login = simple_async_mock() + + # mock out the error renderer too + sso_handler = self.hs.get_sso_handler() + sso_handler.render_error = Mock(return_value=None) + 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, + request = _mock_request() + self.get_success( + self.handler._handle_authn_response(request, saml_response, ""), + ) + sso_handler.render_error.assert_called_once_with( + request, "mapping_error", "localpart is invalid: föö" ) - self.assertEqual(str(e.value), "localpart is invalid: föö") + auth_handler.complete_sso_login.assert_not_called() def test_map_saml_response_to_user_retries(self): """The mapping provider can retry generating an MXID if the MXID is already in use.""" + + # stub out the auth handler and error renderer + auth_handler = self.hs.get_auth_handler() + auth_handler.complete_sso_login = simple_async_mock() + sso_handler = self.hs.get_sso_handler() + sso_handler.render_error = Mock(return_value=None) + + # register a user to occupy the first-choice MXID store = self.hs.get_datastore() self.get_success( store.register_user(user_id="@test_user:test", password_hash=None) ) + + # send the fake SAML response 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" - ) + request = _mock_request() + self.get_success( + self.handler._handle_authn_response(request, saml_response, ""), ) + # test_user is already taken, so test_user1 gets registered instead. - self.assertEqual(mxid, "@test_user1:test") + auth_handler.complete_sso_login.assert_called_once_with( + "@test_user1:test", request, "" + ) + auth_handler.complete_sso_login.reset_mock() # Register all of the potential mxids for a particular SAML username. self.get_success( @@ -188,15 +229,15 @@ class SamlHandlerTestCase(HomeserverTestCase): # 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.get_success( + self.handler._handle_authn_response(request, saml_response, ""), ) - self.assertEqual( - str(e.value), "Unable to generate a Matrix ID from the SSO response" + sso_handler.render_error.assert_called_once_with( + request, + "mapping_error", + "Unable to generate a Matrix ID from the SSO response", ) + auth_handler.complete_sso_login.assert_not_called() @override_config( { @@ -208,12 +249,17 @@ class SamlHandlerTestCase(HomeserverTestCase): } ) def test_map_saml_response_redirect(self): + """Test a mapping provider that raises a RedirectException""" + saml_response = FakeAuthnResponse({"uid": "test", "username": "test_user"}) - redirect_url = "" + request = _mock_request() e = self.get_failure( - self.handler._map_saml_response_to_user( - saml_response, redirect_url, "user-agent", "10.10.10.10" - ), + self.handler._handle_authn_response(request, saml_response, ""), RedirectException, ) self.assertEqual(e.value.location, b"https://custom-saml-redirect/") + + +def _mock_request(): + """Returns a mock which will stand in as a SynapseRequest""" + return Mock(spec=["getClientIP", "get_user_agent"]) diff --git a/tests/test_utils/__init__.py b/tests/test_utils/__init__.py index 6873d45eb6..43898d8142 100644 --- a/tests/test_utils/__init__.py +++ b/tests/test_utils/__init__.py @@ -22,6 +22,8 @@ import warnings from asyncio import Future from typing import Any, Awaitable, Callable, TypeVar +from mock import Mock + import attr from twisted.python.failure import Failure @@ -87,6 +89,16 @@ def setup_awaitable_errors() -> Callable[[], None]: return cleanup +def simple_async_mock(return_value=None, raises=None) -> Mock: + # AsyncMock is not available in python3.5, this mimics part of its behaviour + async def cb(*args, **kwargs): + if raises: + raise raises + return return_value + + return Mock(side_effect=cb) + + @attr.s class FakeResponse: """A fake twisted.web.IResponse object -- cgit 1.5.1 From e1b8e37f936b115e2164d272333c9b15342e6f88 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Wed, 16 Dec 2020 20:01:53 +0000 Subject: Push login completion down into SsoHandler (#8941) This is another part of my work towards fixing #8876. It moves some of the logic currently in the SAML and OIDC handlers - in particular the call to `AuthHandler.complete_sso_login` down into the `SsoHandler`. --- changelog.d/8941.feature | 1 + synapse/handlers/oidc_handler.py | 62 +++++++++++++++++----------------------- synapse/handlers/saml_handler.py | 37 ++++++++---------------- synapse/handlers/sso.py | 58 +++++++++++++++++++++++-------------- tests/handlers/test_saml.py | 8 +++--- 5 files changed, 80 insertions(+), 86 deletions(-) create mode 100644 changelog.d/8941.feature (limited to 'tests/handlers/test_saml.py') diff --git a/changelog.d/8941.feature b/changelog.d/8941.feature new file mode 100644 index 0000000000..d450ef4998 --- /dev/null +++ b/changelog.d/8941.feature @@ -0,0 +1 @@ +Add support for allowing users to pick their own user ID during a single-sign-on login. diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index f626117f76..cbd11a1382 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -115,8 +115,6 @@ class OidcHandler(BaseHandler): self._allow_existing_users = hs.config.oidc_allow_existing_users # type: bool self._http_client = hs.get_proxied_http_client() - self._auth_handler = hs.get_auth_handler() - self._registration_handler = hs.get_registration_handler() self._server_name = hs.config.server_name # type: str self._macaroon_secret_key = hs.config.macaroon_secret_key @@ -689,33 +687,14 @@ class OidcHandler(BaseHandler): # otherwise, it's a login - # Pull out the user-agent and IP from the request. - user_agent = request.get_user_agent("") - ip_address = self.hs.get_ip_from_request(request) - # Call the mapper to register/login the user try: - user_id = await self._map_userinfo_to_user( - userinfo, token, user_agent, ip_address + await self._complete_oidc_login( + userinfo, token, request, client_redirect_url ) except MappingException as e: logger.exception("Could not map user") self._sso_handler.render_error(request, "mapping_error", str(e)) - return - - # Mapping providers might not have get_extra_attributes: only call this - # method if it exists. - extra_attributes = None - get_extra_attributes = getattr( - self._user_mapping_provider, "get_extra_attributes", None - ) - if get_extra_attributes: - extra_attributes = await get_extra_attributes(userinfo, token) - - # and finally complete the login - await self._auth_handler.complete_sso_login( - user_id, request, client_redirect_url, extra_attributes - ) def _generate_oidc_session_token( self, @@ -838,10 +817,14 @@ class OidcHandler(BaseHandler): now = self.clock.time_msec() return now < expiry - async def _map_userinfo_to_user( - self, userinfo: UserInfo, token: Token, user_agent: str, ip_address: str - ) -> str: - """Maps a UserInfo object to a mxid. + async def _complete_oidc_login( + self, + userinfo: UserInfo, + token: Token, + request: SynapseRequest, + client_redirect_url: str, + ) -> None: + """Given a UserInfo response, complete the login flow UserInfo should have a claim that uniquely identifies users. This claim is usually `sub`, but can be configured with `oidc_config.subject_claim`. @@ -853,17 +836,16 @@ class OidcHandler(BaseHandler): If a user already exists with the mxid we've mapped and allow_existing_users is disabled, raise an exception. + Otherwise, render a redirect back to the client_redirect_url with a loginToken. + Args: userinfo: an object representing the user token: a dict with the tokens obtained from the provider - user_agent: The user agent of the client making the request. - ip_address: The IP address of the client making the request. + request: The request to respond to + client_redirect_url: The redirect URL passed in by the client. Raises: MappingException: if there was an error while mapping some properties - - Returns: - The mxid of the user """ try: remote_user_id = self._remote_id_from_userinfo(userinfo) @@ -931,13 +913,23 @@ class OidcHandler(BaseHandler): return None - return await self._sso_handler.get_mxid_from_sso( + # Mapping providers might not have get_extra_attributes: only call this + # method if it exists. + extra_attributes = None + get_extra_attributes = getattr( + self._user_mapping_provider, "get_extra_attributes", None + ) + if get_extra_attributes: + extra_attributes = await get_extra_attributes(userinfo, token) + + await self._sso_handler.complete_sso_login_request( self._auth_provider_id, remote_user_id, - user_agent, - ip_address, + request, + client_redirect_url, oidc_response_to_user_attributes, grandfather_existing_users, + extra_attributes, ) def _remote_id_from_userinfo(self, userinfo: UserInfo) -> str: diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py index 6001fe3e27..5fa7ab3f8b 100644 --- a/synapse/handlers/saml_handler.py +++ b/synapse/handlers/saml_handler.py @@ -58,8 +58,6 @@ class SamlHandler(BaseHandler): super().__init__(hs) self._saml_client = Saml2Client(hs.config.saml2_sp_config) self._saml_idp_entityid = hs.config.saml2_idp_entityid - self._auth_handler = hs.get_auth_handler() - self._registration_handler = hs.get_registration_handler() self._saml2_session_lifetime = hs.config.saml2_session_lifetime self._grandfathered_mxid_source_attribute = ( @@ -229,40 +227,29 @@ class SamlHandler(BaseHandler): ) return - # Pull out the user-agent and IP from the request. - user_agent = request.get_user_agent("") - ip_address = self.hs.get_ip_from_request(request) - # Call the mapper to register/login the user try: - user_id = await self._map_saml_response_to_user( - saml2_auth, relay_state, user_agent, ip_address - ) + await self._complete_saml_login(saml2_auth, request, relay_state) except MappingException as e: logger.exception("Could not map user") self._sso_handler.render_error(request, "mapping_error", str(e)) - return - await self._auth_handler.complete_sso_login(user_id, request, relay_state) - - async def _map_saml_response_to_user( + async def _complete_saml_login( self, saml2_auth: saml2.response.AuthnResponse, + request: SynapseRequest, client_redirect_url: str, - user_agent: str, - ip_address: str, - ) -> str: + ) -> None: """ - Given a SAML response, retrieve the user ID for it and possibly register the user. + Given a SAML response, complete the login flow + + Retrieves the remote user ID, registers the user if necessary, and serves + a redirect back to the client with a login-token. Args: saml2_auth: The parsed SAML2 response. + request: The request to respond to client_redirect_url: The redirect URL passed in by the client. - user_agent: The user agent of the client making the request. - ip_address: The IP address of the client making the request. - - Returns: - The user ID associated with this response. Raises: MappingException if there was a problem mapping the response to a user. @@ -318,11 +305,11 @@ class SamlHandler(BaseHandler): return None - return await self._sso_handler.get_mxid_from_sso( + await self._sso_handler.complete_sso_login_request( self._auth_provider_id, remote_user_id, - user_agent, - ip_address, + request, + client_redirect_url, saml_response_to_remapped_user_attributes, grandfather_existing_users, ) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 112a7d5b2c..f054b66a53 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -21,7 +21,8 @@ from twisted.web.http import Request from synapse.api.errors import RedirectException from synapse.http.server import respond_with_html -from synapse.types import UserID, contains_invalid_mxid_characters +from synapse.http.site import SynapseRequest +from synapse.types import JsonDict, UserID, contains_invalid_mxid_characters from synapse.util.async_helpers import Linearizer if TYPE_CHECKING: @@ -119,15 +120,16 @@ class SsoHandler: # No match. return None - async def get_mxid_from_sso( + async def complete_sso_login_request( self, auth_provider_id: str, remote_user_id: str, - user_agent: str, - ip_address: str, + request: SynapseRequest, + client_redirect_url: str, sso_to_matrix_id_mapper: Callable[[int], Awaitable[UserAttributes]], grandfather_existing_users: Optional[Callable[[], Awaitable[Optional[str]]]], - ) -> str: + extra_login_attributes: Optional[JsonDict] = None, + ) -> None: """ Given an SSO ID, retrieve the user ID for it and possibly register the user. @@ -146,12 +148,18 @@ class SsoHandler: given user-agent and IP address and the SSO ID is linked to this matrix ID for subsequent calls. + Finally, we generate a redirect to the supplied redirect uri, with a login token + Args: auth_provider_id: A unique identifier for this SSO provider, e.g. "oidc" or "saml". + remote_user_id: The unique identifier from the SSO provider. - user_agent: The user agent of the client making the request. - ip_address: The IP address of the client making the request. + + request: The request to respond to + + client_redirect_url: The redirect URL passed in by the client. + 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. @@ -163,12 +171,13 @@ class SsoHandler: 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. - Returns: - The user ID associated with the SSO response. + extra_login_attributes: An optional dictionary of extra + attributes to be provided to the client in the login response. Raises: MappingException if there was a problem mapping the response to a user. @@ -181,28 +190,33 @@ class SsoHandler: # interstitial pages. with await self._mapping_lock.queue(auth_provider_id): # first of all, check if we already have a mapping for this user - previously_registered_user_id = await self.get_sso_user_by_remote_user_id( + user_id = await self.get_sso_user_by_remote_user_id( auth_provider_id, remote_user_id, ) - 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: + if not user_id and grandfather_existing_users: + user_id = await grandfather_existing_users() + if 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 + auth_provider_id, remote_user_id, user_id ) - return previously_registered_user_id # Otherwise, generate a new user. - attributes = await self._call_attribute_mapper(sso_to_matrix_id_mapper) - user_id = await self._register_mapped_user( - attributes, auth_provider_id, remote_user_id, user_agent, ip_address, - ) - return user_id + if not user_id: + attributes = await self._call_attribute_mapper(sso_to_matrix_id_mapper) + user_id = await self._register_mapped_user( + attributes, + auth_provider_id, + remote_user_id, + request.get_user_agent(""), + request.getClientIP(), + ) + + await self._auth_handler.complete_sso_login( + user_id, request, client_redirect_url, extra_login_attributes + ) async def _call_attribute_mapper( self, sso_to_matrix_id_mapper: Callable[[int], Awaitable[UserAttributes]], diff --git a/tests/handlers/test_saml.py b/tests/handlers/test_saml.py index 69927cf6be..548038214b 100644 --- a/tests/handlers/test_saml.py +++ b/tests/handlers/test_saml.py @@ -131,7 +131,7 @@ class SamlHandlerTestCase(HomeserverTestCase): # check that the auth handler got called as expected auth_handler.complete_sso_login.assert_called_once_with( - "@test_user:test", request, "redirect_uri" + "@test_user:test", request, "redirect_uri", None ) @override_config({"saml2_config": {"grandfathered_mxid_source_attribute": "mxid"}}) @@ -157,7 +157,7 @@ class SamlHandlerTestCase(HomeserverTestCase): # check that the auth handler got called as expected auth_handler.complete_sso_login.assert_called_once_with( - "@test_user:test", request, "" + "@test_user:test", request, "", None ) # Subsequent calls should map to the same mxid. @@ -166,7 +166,7 @@ class SamlHandlerTestCase(HomeserverTestCase): self.handler._handle_authn_response(request, saml_response, "") ) auth_handler.complete_sso_login.assert_called_once_with( - "@test_user:test", request, "" + "@test_user:test", request, "", None ) def test_map_saml_response_to_invalid_localpart(self): @@ -214,7 +214,7 @@ class SamlHandlerTestCase(HomeserverTestCase): # test_user is already taken, so test_user1 gets registered instead. auth_handler.complete_sso_login.assert_called_once_with( - "@test_user1:test", request, "" + "@test_user1:test", request, "", None ) auth_handler.complete_sso_login.reset_mock() -- cgit 1.5.1 From 2ec8ca5e6046b207eabe008101631b978758ac6d Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Tue, 12 Jan 2021 12:34:16 +0000 Subject: Remove SynapseRequest.get_user_agent (#9069) SynapseRequest is in danger of becoming a bit of a dumping-ground for "useful stuff relating to Requests", which isn't really its intention (its purpose is to override render, finished and connectionLost to set up the LoggingContext and write the right entries to the request log). Putting utility functions inside SynapseRequest means that lots of our code ends up requiring a SynapseRequest when there is nothing synapse-specific about the Request at all, and any old twisted.web.iweb.IRequest will do. This increases code coupling and makes testing more difficult. In short: move get_user_agent out to a utility function. --- changelog.d/9069.misc | 1 + synapse/api/auth.py | 3 ++- synapse/handlers/auth.py | 6 +++--- synapse/handlers/sso.py | 5 +++-- synapse/http/__init__.py | 15 +++++++++++++++ synapse/http/site.py | 18 ++---------------- tests/handlers/test_cas.py | 2 +- tests/handlers/test_oidc.py | 3 +-- tests/handlers/test_saml.py | 2 +- 9 files changed, 29 insertions(+), 26 deletions(-) create mode 100644 changelog.d/9069.misc (limited to 'tests/handlers/test_saml.py') diff --git a/changelog.d/9069.misc b/changelog.d/9069.misc new file mode 100644 index 0000000000..5e9e62d252 --- /dev/null +++ b/changelog.d/9069.misc @@ -0,0 +1 @@ +Remove `SynapseRequest.get_user_agent`. diff --git a/synapse/api/auth.py b/synapse/api/auth.py index 48c4d7b0be..6d6703250b 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -33,6 +33,7 @@ from synapse.api.errors import ( from synapse.api.room_versions import KNOWN_ROOM_VERSIONS from synapse.appservice import ApplicationService from synapse.events import EventBase +from synapse.http import get_request_user_agent from synapse.http.site import SynapseRequest from synapse.logging import opentracing as opentracing from synapse.storage.databases.main.registration import TokenLookupResult @@ -187,7 +188,7 @@ class Auth: """ try: ip_addr = self.hs.get_ip_from_request(request) - user_agent = request.get_user_agent("") + user_agent = get_request_user_agent(request) access_token = self.get_access_token_from_request(request) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index f4434673dc..5b86ee85c7 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -49,8 +49,10 @@ from synapse.api.errors import ( UserDeactivatedError, ) from synapse.api.ratelimiting import Ratelimiter +from synapse.handlers._base import BaseHandler from synapse.handlers.ui_auth import INTERACTIVE_AUTH_CHECKERS from synapse.handlers.ui_auth.checkers import UserInteractiveAuthChecker +from synapse.http import get_request_user_agent from synapse.http.server import finish_request, respond_with_html from synapse.http.site import SynapseRequest from synapse.logging.context import defer_to_thread @@ -62,8 +64,6 @@ from synapse.util.async_helpers import maybe_awaitable from synapse.util.msisdn import phone_number_to_msisdn from synapse.util.threepids import canonicalise_email -from ._base import BaseHandler - if TYPE_CHECKING: from synapse.app.homeserver import HomeServer @@ -539,7 +539,7 @@ class AuthHandler(BaseHandler): # authentication flow. await self.store.set_ui_auth_clientdict(sid, clientdict) - user_agent = request.get_user_agent("") + user_agent = get_request_user_agent(request) await self.store.add_user_agent_ip_to_ui_auth_session( session.session_id, user_agent, clientip diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 2da1ea2223..740df7e4a0 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -23,6 +23,7 @@ from typing_extensions import NoReturn, Protocol from twisted.web.http import Request from synapse.api.errors import Codes, RedirectException, SynapseError +from synapse.http import get_request_user_agent from synapse.http.server import respond_with_html from synapse.http.site import SynapseRequest from synapse.types import JsonDict, UserID, contains_invalid_mxid_characters @@ -362,7 +363,7 @@ class SsoHandler: attributes, auth_provider_id, remote_user_id, - request.get_user_agent(""), + get_request_user_agent(request), request.getClientIP(), ) @@ -628,7 +629,7 @@ class SsoHandler: attributes, session.auth_provider_id, session.remote_user_id, - request.get_user_agent(""), + get_request_user_agent(request), request.getClientIP(), ) diff --git a/synapse/http/__init__.py b/synapse/http/__init__.py index 59b01b812c..4bc3cb53f0 100644 --- a/synapse/http/__init__.py +++ b/synapse/http/__init__.py @@ -17,6 +17,7 @@ import re from twisted.internet import task from twisted.web.client import FileBodyProducer +from twisted.web.iweb import IRequest from synapse.api.errors import SynapseError @@ -50,3 +51,17 @@ class QuieterFileBodyProducer(FileBodyProducer): FileBodyProducer.stopProducing(self) except task.TaskStopped: pass + + +def get_request_user_agent(request: IRequest, default: str = "") -> str: + """Return the last User-Agent header, or the given default. + """ + # There could be raw utf-8 bytes in the User-Agent header. + + # N.B. if you don't do this, the logger explodes cryptically + # with maximum recursion trying to log errors about + # the charset problem. + # c.f. https://github.com/matrix-org/synapse/issues/3471 + + h = request.getHeader(b"User-Agent") + return h.decode("ascii", "replace") if h else default diff --git a/synapse/http/site.py b/synapse/http/site.py index 5a5790831b..12ec3f851f 100644 --- a/synapse/http/site.py +++ b/synapse/http/site.py @@ -20,7 +20,7 @@ from twisted.python.failure import Failure from twisted.web.server import Request, Site from synapse.config.server import ListenerConfig -from synapse.http import redact_uri +from synapse.http import get_request_user_agent, redact_uri from synapse.http.request_metrics import RequestMetrics, requests_counter from synapse.logging.context import LoggingContext, PreserveLoggingContext from synapse.types import Requester @@ -113,15 +113,6 @@ class SynapseRequest(Request): method = self.method.decode("ascii") return method - def get_user_agent(self, default: str) -> str: - """Return the last User-Agent header, or the given default. - """ - user_agent = self.requestHeaders.getRawHeaders(b"User-Agent", [None])[-1] - if user_agent is None: - return default - - return user_agent.decode("ascii", "replace") - def render(self, resrc): # this is called once a Resource has been found to serve the request; in our # case the Resource in question will normally be a JsonResource. @@ -292,12 +283,7 @@ class SynapseRequest(Request): # and can see that we're doing something wrong. authenticated_entity = repr(self.requester) # type: ignore[unreachable] - # ...or could be raw utf-8 bytes in the User-Agent header. - # N.B. if you don't do this, the logger explodes cryptically - # with maximum recursion trying to log errors about - # the charset problem. - # c.f. https://github.com/matrix-org/synapse/issues/3471 - user_agent = self.get_user_agent("-") + user_agent = get_request_user_agent(self, "-") code = str(self.code) if not self.finished: diff --git a/tests/handlers/test_cas.py b/tests/handlers/test_cas.py index bd7a1b6891..c37bb6440e 100644 --- a/tests/handlers/test_cas.py +++ b/tests/handlers/test_cas.py @@ -118,4 +118,4 @@ class CasHandlerTestCase(HomeserverTestCase): def _mock_request(): """Returns a mock which will stand in as a SynapseRequest""" - return Mock(spec=["getClientIP", "get_user_agent"]) + return Mock(spec=["getClientIP", "getHeader"]) diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index f5df657814..4ce0f74f22 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -1011,7 +1011,7 @@ def _build_callback_request( "addCookie", "requestHeaders", "getClientIP", - "get_user_agent", + "getHeader", ] ) @@ -1020,5 +1020,4 @@ def _build_callback_request( request.args[b"code"] = [code.encode("utf-8")] request.args[b"state"] = [state.encode("utf-8")] request.getClientIP.return_value = ip_address - request.get_user_agent.return_value = user_agent return request diff --git a/tests/handlers/test_saml.py b/tests/handlers/test_saml.py index 548038214b..261c7083d1 100644 --- a/tests/handlers/test_saml.py +++ b/tests/handlers/test_saml.py @@ -262,4 +262,4 @@ class SamlHandlerTestCase(HomeserverTestCase): def _mock_request(): """Returns a mock which will stand in as a SynapseRequest""" - return Mock(spec=["getClientIP", "get_user_agent"]) + return Mock(spec=["getClientIP", "getHeader"]) -- cgit 1.5.1 From 8aed29dc615bee75019fc526a5c91cdc2638b665 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Mon, 1 Feb 2021 15:50:56 +0000 Subject: Improve styling and wording of SSO redirect confirm template (#9272) --- changelog.d/9272.feature | 1 + docs/sample_config.yaml | 14 ++++- synapse/config/sso.py | 14 ++++- synapse/handlers/auth.py | 24 ++++++- synapse/handlers/sso.py | 10 ++- synapse/module_api/__init__.py | 10 ++- synapse/res/templates/sso.css | 83 +++++++++++++++++++++++++ synapse/res/templates/sso_redirect_confirm.html | 34 ++++++++-- tests/handlers/test_cas.py | 8 +-- tests/handlers/test_oidc.py | 24 ++++--- tests/handlers/test_saml.py | 8 +-- 11 files changed, 200 insertions(+), 30 deletions(-) create mode 100644 changelog.d/9272.feature create mode 100644 synapse/res/templates/sso.css (limited to 'tests/handlers/test_saml.py') diff --git a/changelog.d/9272.feature b/changelog.d/9272.feature new file mode 100644 index 0000000000..c21b197ca1 --- /dev/null +++ b/changelog.d/9272.feature @@ -0,0 +1 @@ +Improve the user experience of setting up an account via single-sign on. diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 8777e3254d..05506a7787 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1971,7 +1971,8 @@ sso: # * HTML page for a confirmation step before redirecting back to the client # with the login token: 'sso_redirect_confirm.html'. # - # When rendering, this template is given three variables: + # When rendering, this template is given the following variables: + # # * redirect_url: the URL the user is about to be redirected to. Needs # manual escaping (see # https://jinja.palletsprojects.com/en/2.11.x/templates/#html-escaping). @@ -1984,6 +1985,17 @@ sso: # # * server_name: the homeserver's name. # + # * new_user: a boolean indicating whether this is the user's first time + # logging in. + # + # * user_id: the user's matrix ID. + # + # * user_profile.avatar_url: an MXC URI for the user's avatar, if any. + # None if the user has not set an avatar. + # + # * user_profile.display_name: the user's display name. None if the user + # has not set a display name. + # # * HTML page which notifies the user that they are authenticating to confirm # an operation on their account during the user interactive authentication # process: 'sso_auth_confirm.html'. diff --git a/synapse/config/sso.py b/synapse/config/sso.py index 59be825532..a470112ed4 100644 --- a/synapse/config/sso.py +++ b/synapse/config/sso.py @@ -127,7 +127,8 @@ class SSOConfig(Config): # * HTML page for a confirmation step before redirecting back to the client # with the login token: 'sso_redirect_confirm.html'. # - # When rendering, this template is given three variables: + # When rendering, this template is given the following variables: + # # * redirect_url: the URL the user is about to be redirected to. Needs # manual escaping (see # https://jinja.palletsprojects.com/en/2.11.x/templates/#html-escaping). @@ -140,6 +141,17 @@ class SSOConfig(Config): # # * server_name: the homeserver's name. # + # * new_user: a boolean indicating whether this is the user's first time + # logging in. + # + # * user_id: the user's matrix ID. + # + # * user_profile.avatar_url: an MXC URI for the user's avatar, if any. + # None if the user has not set an avatar. + # + # * user_profile.display_name: the user's display name. None if the user + # has not set a display name. + # # * HTML page which notifies the user that they are authenticating to confirm # an operation on their account during the user interactive authentication # process: 'sso_auth_confirm.html'. diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 0e98db22b3..c722a4afa8 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -61,6 +61,7 @@ from synapse.http.site import SynapseRequest from synapse.logging.context import defer_to_thread from synapse.metrics.background_process_metrics import run_as_background_process from synapse.module_api import ModuleApi +from synapse.storage.roommember import ProfileInfo from synapse.types import JsonDict, Requester, UserID from synapse.util import stringutils as stringutils from synapse.util.async_helpers import maybe_awaitable @@ -1396,6 +1397,7 @@ class AuthHandler(BaseHandler): request: Request, client_redirect_url: str, extra_attributes: Optional[JsonDict] = None, + new_user: bool = False, ): """Having figured out a mxid for this user, complete the HTTP request @@ -1406,6 +1408,8 @@ class AuthHandler(BaseHandler): process. extra_attributes: Extra attributes which will be passed to the client during successful login. Must be JSON serializable. + new_user: True if we should use wording appropriate to a user who has just + registered. """ # If the account has been deactivated, do not proceed with the login # flow. @@ -1414,8 +1418,17 @@ class AuthHandler(BaseHandler): respond_with_html(request, 403, self._sso_account_deactivated_template) return + profile = await self.store.get_profileinfo( + UserID.from_string(registered_user_id).localpart + ) + self._complete_sso_login( - registered_user_id, request, client_redirect_url, extra_attributes + registered_user_id, + request, + client_redirect_url, + extra_attributes, + new_user=new_user, + user_profile_data=profile, ) def _complete_sso_login( @@ -1424,12 +1437,18 @@ class AuthHandler(BaseHandler): request: Request, client_redirect_url: str, extra_attributes: Optional[JsonDict] = None, + new_user: bool = False, + user_profile_data: Optional[ProfileInfo] = None, ): """ The synchronous portion of complete_sso_login. This exists purely for backwards compatibility of synapse.module_api.ModuleApi. """ + + if user_profile_data is None: + user_profile_data = ProfileInfo(None, None) + # Store any extra attributes which will be passed in the login response. # Note that this is per-user so it may overwrite a previous value, this # is considered OK since the newest SSO attributes should be most valid. @@ -1467,6 +1486,9 @@ class AuthHandler(BaseHandler): display_url=redirect_url_no_params, redirect_url=redirect_url, server_name=self._server_name, + new_user=new_user, + user_id=registered_user_id, + user_profile=user_profile_data, ) respond_with_html(request, 200, html) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 50c5ae142a..ceaeb5a376 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -391,6 +391,8 @@ class SsoHandler: to an additional page. (e.g. to prompt for more information) """ + new_user = False + # grab a lock while we try to find a mapping for this user. This seems... # optimistic, especially for implementations that end up redirecting to # interstitial pages. @@ -431,9 +433,14 @@ class SsoHandler: get_request_user_agent(request), request.getClientIP(), ) + new_user = True await self._auth_handler.complete_sso_login( - user_id, request, client_redirect_url, extra_login_attributes + user_id, + request, + client_redirect_url, + extra_login_attributes, + new_user=new_user, ) async def _call_attribute_mapper( @@ -778,6 +785,7 @@ class SsoHandler: request, session.client_redirect_url, session.extra_login_attributes, + new_user=True, ) def _expire_old_sessions(self): diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 72ab5750cc..401d577293 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -279,7 +279,11 @@ class ModuleApi: ) async def complete_sso_login_async( - self, registered_user_id: str, request: SynapseRequest, client_redirect_url: str + self, + registered_user_id: str, + request: SynapseRequest, + client_redirect_url: str, + new_user: bool = False, ): """Complete a SSO login by redirecting the user to a page to confirm whether they want their access token sent to `client_redirect_url`, or redirect them to that @@ -291,9 +295,11 @@ class ModuleApi: request: The request to respond to. client_redirect_url: The URL to which to offer to redirect the user (or to redirect them directly if whitelisted). + new_user: set to true to use wording for the consent appropriate to a user + who has just registered. """ await self._auth_handler.complete_sso_login( - registered_user_id, request, client_redirect_url, + registered_user_id, request, client_redirect_url, new_user=new_user ) @defer.inlineCallbacks diff --git a/synapse/res/templates/sso.css b/synapse/res/templates/sso.css new file mode 100644 index 0000000000..ff9dc94032 --- /dev/null +++ b/synapse/res/templates/sso.css @@ -0,0 +1,83 @@ +body { + font-family: "Inter", "Helvetica", "Arial", sans-serif; + font-size: 14px; + color: #17191C; +} + +header { + max-width: 480px; + width: 100%; + margin: 24px auto; + text-align: center; +} + +header p { + color: #737D8C; + line-height: 24px; +} + +h1 { + font-size: 24px; +} + +h2 { + font-size: 14px; +} + +h2 img { + vertical-align: middle; + margin-right: 8px; + width: 24px; + height: 24px; +} + +label { + cursor: pointer; +} + +main { + max-width: 360px; + width: 100%; + margin: 24px auto; +} + +.primary-button { + border: none; + text-decoration: none; + padding: 12px; + color: white; + background-color: #418DED; + font-weight: bold; + display: block; + border-radius: 12px; + width: 100%; + margin: 16px 0; + cursor: pointer; + text-align: center; +} + +.profile { + display: flex; + justify-content: center; + margin: 24px 0; +} + +.profile .avatar { + width: 36px; + height: 36px; + border-radius: 100%; + display: block; + margin-right: 8px; +} + +.profile .display-name { + font-weight: bold; + margin-bottom: 4px; +} +.profile .user-id { + color: #737D8C; +} + +.profile .display-name, .profile .user-id { + line-height: 18px; +} \ No newline at end of file diff --git a/synapse/res/templates/sso_redirect_confirm.html b/synapse/res/templates/sso_redirect_confirm.html index 20a15e1e74..ce4f573848 100644 --- a/synapse/res/templates/sso_redirect_confirm.html +++ b/synapse/res/templates/sso_redirect_confirm.html @@ -3,12 +3,34 @@ SSO redirect confirmation + + -

The application at {{ display_url | e }} is requesting full access to your {{ server_name }} Matrix account.

-

If you don't recognise this address, you should ignore this and close this tab.

-

- I trust this address -

+
+ {% if new_user %} +

Your account is now ready

+

You've made your account on {{ server_name | e }}.

+ {% else %} +

Log in

+ {% endif %} +

Continue to confirm you trust {{ display_url | e }}.

+
+
+ {% if user_profile.avatar_url %} +
+ +
+ {% if user_profile.display_name %} +
{{ user_profile.display_name | e }}
+ {% endif %} +
{{ user_id | e }}
+
+
+ {% endif %} + Continue +
- \ No newline at end of file + diff --git a/tests/handlers/test_cas.py b/tests/handlers/test_cas.py index c37bb6440e..7baf224f7e 100644 --- a/tests/handlers/test_cas.py +++ b/tests/handlers/test_cas.py @@ -62,7 +62,7 @@ class CasHandlerTestCase(HomeserverTestCase): # check that the auth handler got called as expected auth_handler.complete_sso_login.assert_called_once_with( - "@test_user:test", request, "redirect_uri", None + "@test_user:test", request, "redirect_uri", None, new_user=True ) def test_map_cas_user_to_existing_user(self): @@ -85,7 +85,7 @@ class CasHandlerTestCase(HomeserverTestCase): # check that the auth handler got called as expected auth_handler.complete_sso_login.assert_called_once_with( - "@test_user:test", request, "redirect_uri", None + "@test_user:test", request, "redirect_uri", None, new_user=False ) # Subsequent calls should map to the same mxid. @@ -94,7 +94,7 @@ class CasHandlerTestCase(HomeserverTestCase): self.handler._handle_cas_response(request, cas_response, "redirect_uri", "") ) auth_handler.complete_sso_login.assert_called_once_with( - "@test_user:test", request, "redirect_uri", None + "@test_user:test", request, "redirect_uri", None, new_user=False ) def test_map_cas_user_to_invalid_localpart(self): @@ -112,7 +112,7 @@ class CasHandlerTestCase(HomeserverTestCase): # check that the auth handler got called as expected auth_handler.complete_sso_login.assert_called_once_with( - "@f=c3=b6=c3=b6:test", request, "redirect_uri", None + "@f=c3=b6=c3=b6:test", request, "redirect_uri", None, new_user=True ) diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index b3dfa40d25..d8f90b9a80 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -419,7 +419,7 @@ class OidcHandlerTestCase(HomeserverTestCase): self.get_success(self.handler.handle_oidc_callback(request)) auth_handler.complete_sso_login.assert_called_once_with( - expected_user_id, request, client_redirect_url, None, + expected_user_id, request, client_redirect_url, None, new_user=True ) self.provider._exchange_code.assert_called_once_with(code) self.provider._parse_id_token.assert_called_once_with(token, nonce=nonce) @@ -450,7 +450,7 @@ class OidcHandlerTestCase(HomeserverTestCase): self.get_success(self.handler.handle_oidc_callback(request)) auth_handler.complete_sso_login.assert_called_once_with( - expected_user_id, request, client_redirect_url, None, + expected_user_id, request, client_redirect_url, None, new_user=False ) self.provider._exchange_code.assert_called_once_with(code) self.provider._parse_id_token.assert_not_called() @@ -623,7 +623,11 @@ class OidcHandlerTestCase(HomeserverTestCase): self.get_success(self.handler.handle_oidc_callback(request)) auth_handler.complete_sso_login.assert_called_once_with( - "@foo:test", request, client_redirect_url, {"phone": "1234567"}, + "@foo:test", + request, + client_redirect_url, + {"phone": "1234567"}, + new_user=True, ) def test_map_userinfo_to_user(self): @@ -637,7 +641,7 @@ class OidcHandlerTestCase(HomeserverTestCase): } self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) auth_handler.complete_sso_login.assert_called_once_with( - "@test_user:test", ANY, ANY, None, + "@test_user:test", ANY, ANY, None, new_user=True ) auth_handler.complete_sso_login.reset_mock() @@ -648,7 +652,7 @@ class OidcHandlerTestCase(HomeserverTestCase): } self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) auth_handler.complete_sso_login.assert_called_once_with( - "@test_user_2:test", ANY, ANY, None, + "@test_user_2:test", ANY, ANY, None, new_user=True ) auth_handler.complete_sso_login.reset_mock() @@ -685,14 +689,14 @@ class OidcHandlerTestCase(HomeserverTestCase): } self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) auth_handler.complete_sso_login.assert_called_once_with( - user.to_string(), ANY, ANY, None, + user.to_string(), ANY, ANY, None, new_user=False ) auth_handler.complete_sso_login.reset_mock() # Subsequent calls should map to the same mxid. self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) auth_handler.complete_sso_login.assert_called_once_with( - user.to_string(), ANY, ANY, None, + user.to_string(), ANY, ANY, None, new_user=False ) auth_handler.complete_sso_login.reset_mock() @@ -707,7 +711,7 @@ class OidcHandlerTestCase(HomeserverTestCase): } self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) auth_handler.complete_sso_login.assert_called_once_with( - user.to_string(), ANY, ANY, None, + user.to_string(), ANY, ANY, None, new_user=False ) auth_handler.complete_sso_login.reset_mock() @@ -743,7 +747,7 @@ class OidcHandlerTestCase(HomeserverTestCase): self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) auth_handler.complete_sso_login.assert_called_once_with( - "@TEST_USER_2:test", ANY, ANY, None, + "@TEST_USER_2:test", ANY, ANY, None, new_user=False ) def test_map_userinfo_to_invalid_localpart(self): @@ -779,7 +783,7 @@ class OidcHandlerTestCase(HomeserverTestCase): # test_user is already taken, so test_user1 gets registered instead. auth_handler.complete_sso_login.assert_called_once_with( - "@test_user1:test", ANY, ANY, None, + "@test_user1:test", ANY, ANY, None, new_user=True ) auth_handler.complete_sso_login.reset_mock() diff --git a/tests/handlers/test_saml.py b/tests/handlers/test_saml.py index 261c7083d1..a8d6c0f617 100644 --- a/tests/handlers/test_saml.py +++ b/tests/handlers/test_saml.py @@ -131,7 +131,7 @@ class SamlHandlerTestCase(HomeserverTestCase): # check that the auth handler got called as expected auth_handler.complete_sso_login.assert_called_once_with( - "@test_user:test", request, "redirect_uri", None + "@test_user:test", request, "redirect_uri", None, new_user=True ) @override_config({"saml2_config": {"grandfathered_mxid_source_attribute": "mxid"}}) @@ -157,7 +157,7 @@ class SamlHandlerTestCase(HomeserverTestCase): # check that the auth handler got called as expected auth_handler.complete_sso_login.assert_called_once_with( - "@test_user:test", request, "", None + "@test_user:test", request, "", None, new_user=False ) # Subsequent calls should map to the same mxid. @@ -166,7 +166,7 @@ class SamlHandlerTestCase(HomeserverTestCase): self.handler._handle_authn_response(request, saml_response, "") ) auth_handler.complete_sso_login.assert_called_once_with( - "@test_user:test", request, "", None + "@test_user:test", request, "", None, new_user=False ) def test_map_saml_response_to_invalid_localpart(self): @@ -214,7 +214,7 @@ class SamlHandlerTestCase(HomeserverTestCase): # test_user is already taken, so test_user1 gets registered instead. auth_handler.complete_sso_login.assert_called_once_with( - "@test_user1:test", request, "", None + "@test_user1:test", request, "", None, new_user=True ) auth_handler.complete_sso_login.reset_mock() -- cgit 1.5.1