Simplify the flow for SSO UIA

changelog.d/8881.misc

@@ -0,0 +1 @@
+Simplify logic for handling user-interactive-auth via single-sign-on servers.

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,

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

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,
     ):

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

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