From c015630aeef19e25a94833de81582de5c7e2af8a Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 4 Dec 2020 15:46:59 +0000 Subject: [PATCH 1/4] SsoHandler: remove inheritance from BaseHandler --- synapse/handlers/_base.py | 4 ++++ synapse/handlers/sso.py | 16 ++++++++-------- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/synapse/handlers/_base.py b/synapse/handlers/_base.py index bb81c0e81d2c..d29b066a564f 100644 --- a/synapse/handlers/_base.py +++ b/synapse/handlers/_base.py @@ -32,6 +32,10 @@ class BaseHandler: """ Common base class for the event handlers. + + Deprecated: new code should not use this. Instead, Handler classes should define the + fields they actually need. The utility methods should either be factored out to + standalone helper functions, or to different Handler classes. """ def __init__(self, hs: "HomeServer"): diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 47ad96f97e3d..ee76dcfa1167 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -18,7 +18,6 @@ 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 @@ -42,12 +41,13 @@ class UserAttributes: emails = attr.ib(type=List[str], default=attr.Factory(list)) -class SsoHandler(BaseHandler): +class SsoHandler: # The number of attempts to ask the mapping provider for when generating an MXID. _MAP_USERNAME_RETRIES = 1000 def __init__(self, hs: "HomeServer"): - super().__init__(hs) + self._store = hs.get_datastore() + self._server_name = hs.hostname self._registration_handler = hs.get_registration_handler() self._error_template = hs.config.sso_error_template @@ -95,7 +95,7 @@ async def get_sso_user_by_remote_user_id( ) # Check if we already have a mapping for this user. - previously_registered_user_id = await self.store.get_user_by_external_id( + previously_registered_user_id = await self._store.get_user_by_external_id( auth_provider_id, remote_user_id, ) @@ -181,7 +181,7 @@ async def get_mxid_from_sso( 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( + await self._store.record_user_external_id( auth_provider_id, remote_user_id, previously_registered_user_id ) return previously_registered_user_id @@ -214,8 +214,8 @@ async def get_mxid_from_sso( ) # Check if this mxid already exists - user_id = UserID(attributes.localpart, self.server_name).to_string() - if not await self.store.get_users_by_id_case_insensitive(user_id): + user_id = UserID(attributes.localpart, self._server_name).to_string() + if not await self._store.get_users_by_id_case_insensitive(user_id): # This mxid is free break else: @@ -238,7 +238,7 @@ async def get_mxid_from_sso( user_agent_ips=[(user_agent, ip_address)], ) - await self.store.record_user_external_id( + await self._store.record_user_external_id( auth_provider_id, remote_user_id, registered_user_id ) return registered_user_id From ac0cd50cce787e9daad398bb65a19a152cc49de1 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 4 Dec 2020 17:23:58 +0000 Subject: [PATCH 2/4] Simplify the flow for SSO UIA We don't need to do all the magic for mapping users when we are doing UIA, so let's factor that out. --- mypy.ini | 1 + synapse/handlers/auth.py | 11 +++-- synapse/handlers/oidc_handler.py | 81 ++++++++++++++++++------------ synapse/handlers/saml_handler.py | 85 +++++++++++++++++++++----------- synapse/handlers/sso.py | 43 ++++++++++++++++ 5 files changed, 155 insertions(+), 66 deletions(-) diff --git a/mypy.ini b/mypy.ini index 3c8d303064f7..b4f0d2b69752 100644 --- a/mypy.ini +++ b/mypy.ini @@ -43,6 +43,7 @@ files = synapse/handlers/room_member.py, synapse/handlers/room_member_worker.py, synapse/handlers/saml_handler.py, + synapse/handlers/sso.py, synapse/handlers/sync.py, synapse/handlers/ui_auth, synapse/http/client.py, diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 2e72298e05ed..afae6d327240 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -36,6 +36,8 @@ import bcrypt import pymacaroons +from twisted.web.http import Request + from synapse.api.constants import LoginType from synapse.api.errors import ( AuthError, @@ -1331,15 +1333,14 @@ async def start_sso_ui_auth(self, redirect_url: str, session_id: str) -> str: ) async def complete_sso_ui_auth( - self, registered_user_id: str, session_id: str, request: SynapseRequest, + self, registered_user_id: str, session_id: str, request: Request, ): """Having figured out a mxid for this user, complete the HTTP request Args: registered_user_id: The registered user ID to complete SSO login for. + session_id: The ID of the user-interactive auth session. request: The request to complete. - client_redirect_url: The URL to which to redirect the user at the end of the - process. """ # Mark the stage of the authentication as successful. # Save the user who authenticated with SSO, this will be used to ensure @@ -1355,7 +1356,7 @@ async def complete_sso_ui_auth( async def complete_sso_login( self, registered_user_id: str, - request: SynapseRequest, + request: Request, client_redirect_url: str, extra_attributes: Optional[JsonDict] = None, ): @@ -1383,7 +1384,7 @@ async def complete_sso_login( def _complete_sso_login( self, registered_user_id: str, - request: SynapseRequest, + request: Request, client_redirect_url: str, extra_attributes: Optional[JsonDict] = None, ): diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index c605f7082a46..dfc1f0721efc 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -674,38 +674,43 @@ async def handle_oidc_callback(self, request: SynapseRequest) -> None: self._sso_handler.render_error(request, "invalid_token", str(e)) 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: + # first check if we're doing a UIA + if ui_auth_session_id: + return await self._sso_handler.complete_sso_ui_auth_request( + self._auth_provider_id, + self._remote_id_from_userinfo(userinfo), + ui_auth_session_id, + request, + ) + + # 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 user_id = await self._map_userinfo_to_user( userinfo, token, user_agent, ip_address ) - 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 - if ui_auth_session_id: - await self._auth_handler.complete_sso_ui_auth( - user_id, ui_auth_session_id, request + # 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 ) - else: + 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 ) + except MappingException as e: + logger.exception("Could not map user") + self._sso_handler.render_error(request, "mapping_error", str(e)) def _generate_oidc_session_token( self, @@ -855,15 +860,7 @@ async def _map_userinfo_to_user( Returns: The mxid of the user """ - try: - remote_user_id = self._user_mapping_provider.get_remote_user_id(userinfo) - except Exception as e: - raise MappingException( - "Failed to extract subject from OIDC response: %s" % (e,) - ) - # Some OIDC providers use integer IDs, but Synapse expects external IDs - # to be strings. - remote_user_id = str(remote_user_id) + remote_user_id = self._remote_id_from_userinfo(userinfo) # Older mapping providers don't accept the `failures` argument, so we # try and detect support. @@ -933,6 +930,26 @@ async def grandfather_existing_users() -> Optional[str]: grandfather_existing_users, ) + def _remote_id_from_userinfo(self, userinfo: UserInfo) -> str: + """Extract the unique remote id from an OIDC UserInfo block + + Args: + userinfo: An object representing the user given by the OIDC provider + Returns: + remote user id + Raises: + MappingException if there was an error extracting the user id + """ + try: + remote_user_id = self._user_mapping_provider.get_remote_user_id(userinfo) + # Some OIDC providers use integer IDs, but Synapse expects external IDs + # to be strings. + return str(remote_user_id) + except Exception as e: + raise MappingException( + "Failed to extract subject from OIDC response: %s" % (e,) + ) + UserAttributeDict = TypedDict( "UserAttributeDict", {"localpart": str, "display_name": Optional[str]} diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py index 76d4169fe2e6..d87d470a2618 100644 --- a/synapse/handlers/saml_handler.py +++ b/synapse/handlers/saml_handler.py @@ -183,37 +183,41 @@ async def handle_saml_response(self, request: SynapseRequest) -> None: saml2_auth.in_response_to, None ) - # Ensure that the attributes of the logged in user meet the required - # attributes. - for requirement in self._saml2_attribute_requirements: - if not _check_attribute_requirement(saml2_auth.ava, requirement): - self._sso_handler.render_error( - request, "unauthorised", "You are not authorised to log in here." + try: + # first check if we're doing a UIA + if current_session and current_session.ui_auth_session_id: + return await self._sso_handler.complete_sso_ui_auth_request( + self._auth_provider_id, + self._remote_id_from_saml_response(saml2_auth, None), + current_session.ui_auth_session_id, + request, ) - 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) + # otherwise, we're handling a login request. - # Call the mapper to register/login the user - try: + # Ensure that the attributes of the logged in user meet the required + # attributes. + for requirement in self._saml2_attribute_requirements: + if not _check_attribute_requirement(saml2_auth.ava, requirement): + self._sso_handler.render_error( + request, + "unauthorised", + "You are not authorised to log in here.", + ) + 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 user_id = await self._map_saml_response_to_user( saml2_auth, relay_state, user_agent, ip_address ) + await self._auth_handler.complete_sso_login(user_id, request, relay_state) except MappingException as e: logger.exception("Could not map user") self._sso_handler.render_error(request, "mapping_error", str(e)) - return - - # Complete the interactive auth session or the login. - if current_session and current_session.ui_auth_session_id: - await self._auth_handler.complete_sso_ui_auth( - user_id, current_session.ui_auth_session_id, request - ) - - else: - await self._auth_handler.complete_sso_login(user_id, request, relay_state) async def _map_saml_response_to_user( self, @@ -239,16 +243,10 @@ async def _map_saml_response_to_user( RedirectException: some mapping providers may raise this if they need to redirect to an interstitial page. """ - - remote_user_id = self._user_mapping_provider.get_remote_user_id( + remote_user_id = self._remote_id_from_saml_response( saml2_auth, client_redirect_url ) - if not remote_user_id: - raise MappingException( - "Failed to extract remote user id from SAML response" - ) - async def saml_response_to_remapped_user_attributes( failures: int, ) -> UserAttributes: @@ -304,6 +302,35 @@ async def grandfather_existing_users() -> Optional[str]: grandfather_existing_users, ) + def _remote_id_from_saml_response( + self, + saml2_auth: saml2.response.AuthnResponse, + client_redirect_url: Optional[str], + ) -> str: + """Extract the unique remote id from a SAML2 AuthnResponse + + Args: + saml2_auth: The parsed SAML2 response. + client_redirect_url: The redirect URL passed in by the client. + Returns: + remote user id + + Raises: + MappingException if there was an error extracting the user id + """ + # It's not obvious why we need to pass in the redirect URI to the mapping + # provider, but we do :/ + remote_user_id = self._user_mapping_provider.get_remote_user_id( + saml2_auth, client_redirect_url + ) + + if not remote_user_id: + raise MappingException( + "Failed to extract remote user id from SAML response" + ) + + return remote_user_id + def expire_sessions(self): expire_before = self.clock.time_msec() - self._saml2_session_lifetime to_expire = set() diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index ee76dcfa1167..e24767b921db 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -17,6 +17,8 @@ import attr +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 @@ -50,6 +52,7 @@ def __init__(self, hs: "HomeServer"): self._server_name = hs.hostname self._registration_handler = hs.get_registration_handler() self._error_template = hs.config.sso_error_template + self._auth_handler = hs.get_auth_handler() def render_error( self, request, error: str, error_description: Optional[str] = None @@ -242,3 +245,43 @@ async def get_mxid_from_sso( auth_provider_id, remote_user_id, registered_user_id ) return registered_user_id + + async def complete_sso_ui_auth_request( + self, + auth_provider_id: str, + remote_user_id: str, + ui_auth_session_id: str, + request: Request, + ) -> None: + """ + Given an SSO ID, retrieve the user ID for it and complete UIA. + + Note that this requires that the user is mapped in the "user_external_ids" + table. This will be the case if they have ever logged in via SAML or OIDC in + recentish synapse versions, but may not be for older users. + + 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. + ui_auth_session_id: The ID of the user-interactive auth session. + request: The request to complete. + """ + + user_id = await self.get_sso_user_by_remote_user_id( + auth_provider_id, remote_user_id, + ) + + if not user_id: + logger.warning( + "Remote user %s/%s has not previously logged in here: UIA will fail", + auth_provider_id, + remote_user_id, + ) + # Let the UIA flow handle this the same as if they presented creds for a + # different user. + user_id = "" + + await self._auth_handler.complete_sso_ui_auth( + user_id, ui_auth_session_id, request + ) From 8abb7f4a01fd2b35dd8a407f249ff77b29eed578 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 4 Dec 2020 18:03:06 +0000 Subject: [PATCH 3/4] changelog --- changelog.d/8881.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/8881.misc diff --git a/changelog.d/8881.misc b/changelog.d/8881.misc new file mode 100644 index 000000000000..07d3f30fb22b --- /dev/null +++ b/changelog.d/8881.misc @@ -0,0 +1 @@ +Simplify logic for handling user-interactive-auth via single-sign-on servers. From 8795c63e94dcd6c1a73ec296a9f111900c5def3b Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 7 Dec 2020 16:54:33 +0000 Subject: [PATCH 4/4] tighten up exception handling --- synapse/handlers/oidc_handler.py | 83 +++++++++++++++++--------------- synapse/handlers/saml_handler.py | 57 ++++++++++++---------- 2 files changed, 75 insertions(+), 65 deletions(-) diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index dfc1f0721efc..f626117f7645 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -674,43 +674,48 @@ async def handle_oidc_callback(self, request: SynapseRequest) -> None: self._sso_handler.render_error(request, "invalid_token", str(e)) return - try: - # first check if we're doing a UIA - if ui_auth_session_id: - return await self._sso_handler.complete_sso_ui_auth_request( - self._auth_provider_id, - self._remote_id_from_userinfo(userinfo), - ui_auth_session_id, - request, - ) + # first check if we're doing a UIA + if ui_auth_session_id: + try: + remote_user_id = self._remote_id_from_userinfo(userinfo) + except Exception as e: + logger.exception("Could not extract remote user id") + self._sso_handler.render_error(request, "mapping_error", str(e)) + return + + return await self._sso_handler.complete_sso_ui_auth_request( + self._auth_provider_id, remote_user_id, ui_auth_session_id, request + ) - # otherwise, it's a login + # 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) + # 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 + # Call the mapper to register/login the user + try: user_id = await self._map_userinfo_to_user( userinfo, token, user_agent, ip_address ) - - # 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 - ) 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, @@ -860,7 +865,12 @@ async def _map_userinfo_to_user( Returns: The mxid of the user """ - remote_user_id = self._remote_id_from_userinfo(userinfo) + try: + remote_user_id = self._remote_id_from_userinfo(userinfo) + except Exception as e: + raise MappingException( + "Failed to extract subject from OIDC response: %s" % (e,) + ) # Older mapping providers don't accept the `failures` argument, so we # try and detect support. @@ -937,18 +947,11 @@ def _remote_id_from_userinfo(self, userinfo: UserInfo) -> str: userinfo: An object representing the user given by the OIDC provider Returns: remote user id - Raises: - MappingException if there was an error extracting the user id """ - try: - remote_user_id = self._user_mapping_provider.get_remote_user_id(userinfo) - # Some OIDC providers use integer IDs, but Synapse expects external IDs - # to be strings. - return str(remote_user_id) - except Exception as e: - raise MappingException( - "Failed to extract subject from OIDC response: %s" % (e,) - ) + remote_user_id = self._user_mapping_provider.get_remote_user_id(userinfo) + # Some OIDC providers use integer IDs, but Synapse expects external IDs + # to be strings. + return str(remote_user_id) UserAttributeDict = TypedDict( diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py index d87d470a2618..5846f08609b9 100644 --- a/synapse/handlers/saml_handler.py +++ b/synapse/handlers/saml_handler.py @@ -183,41 +183,48 @@ async def handle_saml_response(self, request: SynapseRequest) -> None: saml2_auth.in_response_to, None ) - try: - # first check if we're doing a UIA - if current_session and current_session.ui_auth_session_id: - return await self._sso_handler.complete_sso_ui_auth_request( - self._auth_provider_id, - self._remote_id_from_saml_response(saml2_auth, None), - current_session.ui_auth_session_id, - request, - ) + # first check if we're doing a UIA + if current_session and current_session.ui_auth_session_id: + try: + remote_user_id = self._remote_id_from_saml_response(saml2_auth, None) + except MappingException as e: + logger.exception("Failed to extract remote user id from SAML response") + self._sso_handler.render_error(request, "mapping_error", str(e)) + return + + return await self._sso_handler.complete_sso_ui_auth_request( + self._auth_provider_id, + remote_user_id, + current_session.ui_auth_session_id, + request, + ) - # otherwise, we're handling a login request. + # otherwise, we're handling a login request. - # Ensure that the attributes of the logged in user meet the required - # attributes. - for requirement in self._saml2_attribute_requirements: - if not _check_attribute_requirement(saml2_auth.ava, requirement): - self._sso_handler.render_error( - request, - "unauthorised", - "You are not authorised to log in here.", - ) - return + # Ensure that the attributes of the logged in user meet the required + # attributes. + for requirement in self._saml2_attribute_requirements: + if not _check_attribute_requirement(saml2_auth.ava, requirement): + self._sso_handler.render_error( + request, "unauthorised", "You are not authorised to log in here." + ) + 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) + # 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 + # 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._auth_handler.complete_sso_login(user_id, 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( self,