From c34eca98e844f84ec0ee47c2e2de4592b3ed440e Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Thu, 3 Sep 2020 14:01:03 -0400
Subject: [PATCH 01/10] Separate the SAML2 mapping code from the response
 parsing code.

---
 synapse/handlers/saml_handler.py | 93 ++++++++++++++++----------------
 1 file changed, 47 insertions(+), 46 deletions(-)

diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py
index 66b063f9918f..5781cd89091e 100644
--- a/synapse/handlers/saml_handler.py
+++ b/synapse/handlers/saml_handler.py
@@ -134,49 +134,6 @@ async def handle_saml_response(self, request: SynapseRequest) -> None:
         # the dict.
         self.expire_sessions()
 
-        # Pull out the user-agent and IP from the request.
-        user_agent = request.requestHeaders.getRawHeaders(b"User-Agent", default=[b""])[
-            0
-        ].decode("ascii", "surrogateescape")
-        ip_address = self.hs.get_ip_from_request(request)
-
-        user_id, current_session = await self._map_saml_response_to_user(
-            resp_bytes, relay_state, user_agent, ip_address
-        )
-
-        # 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,
-        resp_bytes: str,
-        client_redirect_url: str,
-        user_agent: str,
-        ip_address: str,
-    ) -> Tuple[str, Optional[Saml2SessionData]]:
-        """
-        Given a sample response, retrieve the cached session and user for it.
-
-        Args:
-            resp_bytes: The SAML response.
-            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:
-             Tuple of the user ID and SAML session associated with this response.
-
-        Raises:
-            SynapseError if there was a problem with the response.
-            RedirectException: some mapping providers may raise this if they need
-                to redirect to an interstitial page.
-        """
         try:
             saml2_auth = self._saml_client.parse_authn_request_response(
                 resp_bytes,
@@ -216,6 +173,50 @@ async def _map_saml_response_to_user(
         for requirement in self._saml2_attribute_requirements:
             _check_attribute_requirement(saml2_auth.ava, requirement)
 
+        # Pull out the user-agent and IP from the request.
+        user_agent = request.requestHeaders.getRawHeaders(b"User-Agent", default=[b""])[
+            0
+        ].decode("ascii", "surrogateescape")
+        ip_address = self.hs.get_ip_from_request(request)
+
+        user_id = await self._map_saml_response_to_user(
+            resp_bytes, relay_state, user_agent, ip_address
+        )
+
+        # 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,
+        saml2_auth: saml2.response.AuthnResponse,
+        client_redirect_url: str,
+        user_agent: str,
+        ip_address: str,
+    ) -> str:
+        """
+        Given a SAML response, retrieve the user ID for it and possibly register the user.
+
+        Args:
+            saml2_auth: The parsed SAML2 response.
+            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:
+            SynapseError if there was a problem with the response.
+            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(
             saml2_auth, client_redirect_url
         )
@@ -235,7 +236,7 @@ async def _map_saml_response_to_user(
             )
             if registered_user_id is not None:
                 logger.info("Found existing mapping %s", registered_user_id)
-                return registered_user_id, current_session
+                return registered_user_id
 
             # backwards-compatibility hack: see if there is an existing user with a
             # suitable mapping from the uid
@@ -260,7 +261,7 @@ async def _map_saml_response_to_user(
                     await self._datastore.record_user_external_id(
                         self._auth_provider_id, remote_user_id, registered_user_id
                     )
-                    return registered_user_id, current_session
+                    return registered_user_id
 
             # Map saml response to user attributes using the configured mapping provider
             for i in range(1000):
@@ -310,7 +311,7 @@ async def _map_saml_response_to_user(
             await self._datastore.record_user_external_id(
                 self._auth_provider_id, remote_user_id, registered_user_id
             )
-            return registered_user_id, current_session
+            return registered_user_id
 
     def expire_sessions(self):
         expire_before = self._clock.time_msec() - self._saml2_session_lifetime

From 5171d1f417181ce1d3cd6b641f7c1dcde9784cb8 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Thu, 3 Sep 2020 14:11:18 -0400
Subject: [PATCH 02/10] Render errors for issues handling the SAML response.

---
 synapse/handlers/saml_handler.py | 53 +++++++++++++++++++++++++++-----
 1 file changed, 45 insertions(+), 8 deletions(-)

diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py
index 5781cd89091e..f6779af54b40 100644
--- a/synapse/handlers/saml_handler.py
+++ b/synapse/handlers/saml_handler.py
@@ -21,9 +21,10 @@
 import saml2.response
 from saml2.client import Saml2Client
 
-from synapse.api.errors import AuthError, SynapseError
+from synapse.api.errors import SynapseError
 from synapse.config import ConfigError
 from synapse.config.saml2_config import SamlAttributeRequirement
+from synapse.http.server import respond_with_html
 from synapse.http.servlet import parse_string
 from synapse.http.site import SynapseRequest
 from synapse.module_api import ModuleApi
@@ -68,6 +69,7 @@ def __init__(self, hs: "synapse.server.HomeServer"):
             hs.config.saml2_grandfathered_mxid_source_attribute
         )
         self._saml2_attribute_requirements = hs.config.saml2.attribute_requirements
+        self._error_template = hs.config.sso_error_template
 
         # plugin to do custom mapping from saml response to mxid
         self._user_mapping_provider = hs.config.saml2_user_mapping_provider_class(
@@ -84,6 +86,25 @@ def __init__(self, hs: "synapse.server.HomeServer"):
         # a lock on the mappings
         self._mapping_lock = Linearizer(name="saml_mapping", clock=self._clock)
 
+    def _render_error(
+        self, request, error: str, error_description: Optional[str] = None
+    ) -> None:
+        """Renders the error template and respond with it.
+
+        This is used to show errors to the user. The template of this page can
+        be found under ``synapse/res/templates/sso_error.html``.
+
+        Args:
+            request: The incoming request from the browser.
+                We'll respond with an HTML page describing the error.
+            error: A technical identifier for this error.
+            error_description: A human-readable description of the error.
+        """
+        html = self._error_template.render(
+            error=error, error_description=error_description
+        )
+        respond_with_html(request, 400, html)
+
     def handle_redirect_request(
         self, client_redirect_url: bytes, ui_auth_session_id: Optional[str] = None
     ) -> bytes:
@@ -146,12 +167,23 @@ async def handle_saml_response(self, request: SynapseRequest) -> None:
             # in the (user-visible) exception message, so let's log the exception here
             # so we can track down the session IDs later.
             logger.warning(str(e))
-            raise SynapseError(400, "Unexpected SAML2 login.")
+            self._render_error(
+                request, "unsolicited_response", "Unexpected SAML2 login."
+            )
+            return
         except Exception as e:
-            raise SynapseError(400, "Unable to parse SAML2 response: %s." % (e,))
+            self._render_error(
+                request,
+                "invalid_response",
+                "Unable to parse SAML2 response: %s." % (e,),
+            )
+            return
 
         if saml2_auth.not_signed:
-            raise SynapseError(400, "SAML2 response was not signed.")
+            self._render_error(
+                request, "unsigned_respond", "SAML2 response was not signed."
+            )
+            return
 
         logger.debug("SAML2 response: %s", saml2_auth.origxml)
         for assertion in saml2_auth.assertions:
@@ -171,7 +203,11 @@ async def handle_saml_response(self, request: SynapseRequest) -> None:
         )
 
         for requirement in self._saml2_attribute_requirements:
-            _check_attribute_requirement(saml2_auth.ava, requirement)
+            if not _check_attribute_requirement(saml2_auth.ava, requirement):
+                self._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.requestHeaders.getRawHeaders(b"User-Agent", default=[b""])[
@@ -179,6 +215,7 @@ async def handle_saml_response(self, request: SynapseRequest) -> None:
         ].decode("ascii", "surrogateescape")
         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(
             resp_bytes, relay_state, user_agent, ip_address
         )
@@ -324,11 +361,11 @@ def expire_sessions(self):
             del self._outstanding_requests_dict[reqid]
 
 
-def _check_attribute_requirement(ava: dict, req: SamlAttributeRequirement):
+def _check_attribute_requirement(ava: dict, req: SamlAttributeRequirement) -> bool:
     values = ava.get(req.attribute, [])
     for v in values:
         if v == req.value:
-            return
+            return True
 
     logger.info(
         "SAML2 attribute %s did not match required value '%s' (was '%s')",
@@ -336,7 +373,7 @@ def _check_attribute_requirement(ava: dict, req: SamlAttributeRequirement):
         req.value,
         values,
     )
-    raise AuthError(403, "You are not authorized to log in here.")
+    return False
 
 
 DOT_REPLACE_PATTERN = re.compile(

From a11c9593078c54160c6d5a872e33496f8f84e67c Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Thu, 3 Sep 2020 14:15:47 -0400
Subject: [PATCH 03/10] Render errors for issues mapping to an MXID.

---
 synapse/handlers/saml_handler.py | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py
index f6779af54b40..766d3d5384b2 100644
--- a/synapse/handlers/saml_handler.py
+++ b/synapse/handlers/saml_handler.py
@@ -42,6 +42,10 @@
 logger = logging.getLogger(__name__)
 
 
+class MappingException(Exception):
+    """Used to catch errors when mapping the SAML2 response to a user."""
+
+
 @attr.s
 class Saml2SessionData:
     """Data we track about SAML2 sessions"""
@@ -216,9 +220,14 @@ async def handle_saml_response(self, request: SynapseRequest) -> None:
         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(
-            resp_bytes, relay_state, user_agent, ip_address
-        )
+        try:
+            user_id = await self._map_saml_response_to_user(
+                resp_bytes, relay_state, user_agent, ip_address
+            )
+        except MappingException as e:
+            logger.exception("Could not map user")
+            self._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:
@@ -249,7 +258,7 @@ async def _map_saml_response_to_user(
              The user ID associated with this response.
 
         Raises:
-            SynapseError if there was a problem with the response.
+            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.
         """
@@ -259,7 +268,9 @@ async def _map_saml_response_to_user(
         )
 
         if not remote_user_id:
-            raise Exception("Failed to extract remote user id from SAML response")
+            raise MappingException(
+                "Failed to extract remote user id from SAML response"
+            )
 
         with (await self._mapping_lock.queue(self._auth_provider_id)):
             # first of all, check if we already have a mapping for this user
@@ -315,7 +326,7 @@ async def _map_saml_response_to_user(
 
                 localpart = attribute_dict.get("mxid_localpart")
                 if not localpart:
-                    raise Exception(
+                    raise MappingException(
                         "Error parsing SAML2 response: SAML mapping provider plugin "
                         "did not return a mxid_localpart value"
                     )
@@ -332,8 +343,8 @@ async def _map_saml_response_to_user(
             else:
                 # Unable to generate a username in 1000 iterations
                 # Break and return error to the user
-                raise SynapseError(
-                    500, "Unable to generate a Matrix ID from the SAML response"
+                raise MappingException(
+                    "Unable to generate a Matrix ID from the SAML response"
                 )
 
             logger.info("Mapped SAML user to local part %s", localpart)
@@ -428,7 +439,7 @@ def get_remote_user_id(
             return saml_response.ava["uid"][0]
         except KeyError:
             logger.warning("SAML2 response lacks a 'uid' attestation")
-            raise SynapseError(400, "'uid' not in SAML2 response")
+            raise MappingException("'uid' not in SAML2 response")
 
     def saml_response_to_user_attributes(
         self,

From 0b3f8aaae80634f3324ec2f5a634f519b0472c67 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Thu, 3 Sep 2020 14:17:24 -0400
Subject: [PATCH 04/10] Use the SAML error page for all SSO errors.

---
 synapse/res/templates/sso_error.html | 43 +++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/synapse/res/templates/sso_error.html b/synapse/res/templates/sso_error.html
index 43a211386bec..af8459719ae4 100644
--- a/synapse/res/templates/sso_error.html
+++ b/synapse/res/templates/sso_error.html
@@ -5,14 +5,49 @@
     <title>SSO error</title>
 </head>
 <body>
-    <p>Oops! Something went wrong during authentication.</p>
+{# If an error of unauthorised is returned it means we have actively rejected their login #}
+{% if error == "unauthorised" %}
+    <p>You are not allowed to log in here.</p>
+{% else %}
+    <p>
+        There was an error during authentication:
+    </p>
+    <div id="errormsg" style="margin:20px 80px">{{ error_description }}</div>
+    <p>
+        If you are seeing this page after clicking a link sent to you via email, make
+        sure you only click the confirmation link once, and that you open the
+        validation link in the same client you're logging in from.
+    </p>
     <p>
         Try logging in again from your Matrix client and if the problem persists
         please contact the server's administrator.
     </p>
     <p>Error: <code>{{ error }}</code></p>
-    {% if error_description %}
-    <pre><code>{{ error_description }}</code></pre>
-    {% endif %}
+
+    <script type="text/javascript">
+        // Error handling to support Auth0 errors that we might get through a GET request
+        // to the validation endpoint. If an error is provided, it's either going to be
+        // located in the query string or in a query string-like URI fragment.
+        // We try to locate the error from any of these two locations, but if we can't
+        // we just don't print anything specific.
+        let searchStr = "";
+        if (window.location.search) {
+            // window.location.searchParams isn't always defined when
+            // window.location.search is, so it's more reliable to parse the latter.
+            searchStr = window.location.search;
+        } else if (window.location.hash) {
+            // Replace the # with a ? so that URLSearchParams does the right thing and
+            // doesn't parse the first parameter incorrectly.
+            searchStr = window.location.hash.replace("#", "?");
+        }
+
+        // We might end up with no error in the URL, so we need to check if we have one
+        // to print one.
+        let errorDesc = new URLSearchParams(searchStr).get("error_description")
+        if (errorDesc) {
+            document.getElementById("errormsg").innerText = errorDesc;
+        }
+    </script>
+{% endif %}
 </body>
 </html>

From f6fb379bd8a31fabd0bb442c59a7bccc95118067 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Thu, 3 Sep 2020 14:18:48 -0400
Subject: [PATCH 05/10] Stop using the SAML error page.

---
 docs/sample_config.yaml                 | 30 ++------------
 synapse/config/saml2_config.py          | 34 ++--------------
 synapse/handlers/saml_handler.py        |  4 +-
 synapse/res/templates/saml_error.html   | 52 -------------------------
 synapse/rest/saml2/response_resource.py | 16 ++------
 5 files changed, 15 insertions(+), 121 deletions(-)
 delete mode 100644 synapse/res/templates/saml_error.html

diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml
index 3528d9e11f5c..8d28669f6b08 100644
--- a/docs/sample_config.yaml
+++ b/docs/sample_config.yaml
@@ -1467,11 +1467,14 @@ trusted_key_servers:
 # At least one of `sp_config` or `config_path` must be set in this section to
 # enable SAML login.
 #
-# (You will probably also want to set the following options to `false` to
+# You will probably also want to set the following options to `false` to
 # disable the regular login/registration flows:
 #   * enable_registration
 #   * password_config.enabled
 #
+# You will also want to investigate the settings under the sso configuration
+# section below.
+#
 # Once SAML support is enabled, a metadata file will be exposed at
 # https://<server>:<port>/_matrix/saml2/metadata.xml, which you may be able to
 # use to configure your SAML IdP with. Alternatively, you can manually configure
@@ -1594,31 +1597,6 @@ saml2_config:
   #  - attribute: department
   #    value: "sales"
 
-  # Directory in which Synapse will try to find the template files below.
-  # If not set, default templates from within the Synapse package will be used.
-  #
-  # DO NOT UNCOMMENT THIS SETTING unless you want to customise the templates.
-  # If you *do* uncomment it, you will need to make sure that all the templates
-  # below are in the directory.
-  #
-  # Synapse will look for the following templates in this directory:
-  #
-  # * HTML page to display to users if something goes wrong during the
-  #   authentication process: 'saml_error.html'.
-  #
-  #   When rendering, this template is given the following variables:
-  #     * code: an HTML error code corresponding to the error that is being
-  #       returned (typically 400 or 500)
-  #
-  #     * msg: a textual message describing the error.
-  #
-  #   The variables will automatically be HTML-escaped.
-  #
-  # You can see the default templates at:
-  # https://github.com/matrix-org/synapse/tree/master/synapse/res/templates
-  #
-  #template_dir: "res/templates"
-
 
 # OpenID Connect integration. The following settings can be used to make Synapse
 # use an OpenID Connect Provider for authentication, instead of its internal
diff --git a/synapse/config/saml2_config.py b/synapse/config/saml2_config.py
index cc7401888b24..95c0615db581 100644
--- a/synapse/config/saml2_config.py
+++ b/synapse/config/saml2_config.py
@@ -169,10 +169,6 @@ def read_config(self, config, **kwargs):
             saml2_config.get("saml_session_lifetime", "15m")
         )
 
-        self.saml2_error_html_template = self.read_templates(
-            ["saml_error.html"], saml2_config.get("template_dir")
-        )[0]
-
     def _default_saml_config_dict(
         self, required_attributes: set, optional_attributes: set
     ):
@@ -225,11 +221,14 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs):
         # At least one of `sp_config` or `config_path` must be set in this section to
         # enable SAML login.
         #
-        # (You will probably also want to set the following options to `false` to
+        # You will probably also want to set the following options to `false` to
         # disable the regular login/registration flows:
         #   * enable_registration
         #   * password_config.enabled
         #
+        # You will also want to investigate the settings under the sso configuration
+        # section below.
+        #
         # Once SAML support is enabled, a metadata file will be exposed at
         # https://<server>:<port>/_matrix/saml2/metadata.xml, which you may be able to
         # use to configure your SAML IdP with. Alternatively, you can manually configure
@@ -351,31 +350,6 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs):
           #    value: "staff"
           #  - attribute: department
           #    value: "sales"
-
-          # Directory in which Synapse will try to find the template files below.
-          # If not set, default templates from within the Synapse package will be used.
-          #
-          # DO NOT UNCOMMENT THIS SETTING unless you want to customise the templates.
-          # If you *do* uncomment it, you will need to make sure that all the templates
-          # below are in the directory.
-          #
-          # Synapse will look for the following templates in this directory:
-          #
-          # * HTML page to display to users if something goes wrong during the
-          #   authentication process: 'saml_error.html'.
-          #
-          #   When rendering, this template is given the following variables:
-          #     * code: an HTML error code corresponding to the error that is being
-          #       returned (typically 400 or 500)
-          #
-          #     * msg: a textual message describing the error.
-          #
-          #   The variables will automatically be HTML-escaped.
-          #
-          # You can see the default templates at:
-          # https://github.com/matrix-org/synapse/tree/master/synapse/res/templates
-          #
-          #template_dir: "res/templates"
         """ % {
             "config_dir_path": config_dir_path
         }
diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py
index 766d3d5384b2..2ab3462c4d08 100644
--- a/synapse/handlers/saml_handler.py
+++ b/synapse/handlers/saml_handler.py
@@ -206,6 +206,8 @@ 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._render_error(
@@ -222,7 +224,7 @@ async def handle_saml_response(self, request: SynapseRequest) -> None:
         # Call the mapper to register/login the user
         try:
             user_id = await self._map_saml_response_to_user(
-                resp_bytes, relay_state, user_agent, ip_address
+                saml2_auth, relay_state, user_agent, ip_address
             )
         except MappingException as e:
             logger.exception("Could not map user")
diff --git a/synapse/res/templates/saml_error.html b/synapse/res/templates/saml_error.html
deleted file mode 100644
index 01cd9bdaf3c5..000000000000
--- a/synapse/res/templates/saml_error.html
+++ /dev/null
@@ -1,52 +0,0 @@
-<!DOCTYPE html>
-<html lang="en">
-<head>
-    <meta charset="UTF-8">
-    <title>SSO login error</title>
-</head>
-<body>
-{# a 403 means we have actively rejected their login #}
-{% if code == 403 %}
-    <p>You are not allowed to log in here.</p>
-{% else %}
-    <p>
-        There was an error during authentication:
-    </p>
-    <div id="errormsg" style="margin:20px 80px">{{ msg }}</div>
-    <p>
-        If you are seeing this page after clicking a link sent to you via email, make
-        sure you only click the confirmation link once, and that you open the
-        validation link in the same client you're logging in from.
-    </p>
-    <p>
-        Try logging in again from your Matrix client and if the problem persists
-        please contact the server's administrator.
-    </p>
-
-    <script type="text/javascript">
-        // Error handling to support Auth0 errors that we might get through a GET request
-        // to the validation endpoint. If an error is provided, it's either going to be
-        // located in the query string or in a query string-like URI fragment.
-        // We try to locate the error from any of these two locations, but if we can't
-        // we just don't print anything specific.
-        let searchStr = "";
-        if (window.location.search) {
-            // window.location.searchParams isn't always defined when
-            // window.location.search is, so it's more reliable to parse the latter.
-            searchStr = window.location.search;
-        } else if (window.location.hash) {
-            // Replace the # with a ? so that URLSearchParams does the right thing and
-            // doesn't parse the first parameter incorrectly.
-            searchStr = window.location.hash.replace("#", "?");
-        }
-
-        // We might end up with no error in the URL, so we need to check if we have one
-        // to print one.
-        let errorDesc = new URLSearchParams(searchStr).get("error_description")
-        if (errorDesc) {
-            document.getElementById("errormsg").innerText = errorDesc;
-        }
-    </script>
-{% endif %}
-</body>
-</html>
diff --git a/synapse/rest/saml2/response_resource.py b/synapse/rest/saml2/response_resource.py
index c10188a5d72d..f6668fb5e3bf 100644
--- a/synapse/rest/saml2/response_resource.py
+++ b/synapse/rest/saml2/response_resource.py
@@ -13,10 +13,8 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
-from twisted.python import failure
 
-from synapse.api.errors import SynapseError
-from synapse.http.server import DirectServeHtmlResource, return_html_error
+from synapse.http.server import DirectServeHtmlResource
 
 
 class SAML2ResponseResource(DirectServeHtmlResource):
@@ -27,21 +25,15 @@ class SAML2ResponseResource(DirectServeHtmlResource):
     def __init__(self, hs):
         super().__init__()
         self._saml_handler = hs.get_saml_handler()
-        self._error_html_template = hs.config.saml2.saml2_error_html_template
 
     async def _async_render_GET(self, request):
         # We're not expecting any GET request on that resource if everything goes right,
         # but some IdPs sometimes end up responding with a 302 redirect on this endpoint.
         # In this case, just tell the user that something went wrong and they should
         # try to authenticate again.
-        f = failure.Failure(
-            SynapseError(400, "Unexpected GET request on /saml2/authn_response")
+        self._saml_handler._render_error(
+            request, "unexpected_get", "Unexpected GET request on /saml2/authn_response"
         )
-        return_html_error(f, request, self._error_html_template)
 
     async def _async_render_POST(self, request):
-        try:
-            await self._saml_handler.handle_saml_response(request)
-        except Exception:
-            f = failure.Failure()
-            return_html_error(f, request, self._error_html_template)
+        await self._saml_handler.handle_saml_response(request)

From 01978a29e0ab8e6a40b50237f25b828d99cca1df Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Thu, 3 Sep 2020 15:57:28 -0400
Subject: [PATCH 06/10] Add changelog file.

---
 changelog.d/8248.feature | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 changelog.d/8248.feature

diff --git a/changelog.d/8248.feature b/changelog.d/8248.feature
new file mode 100644
index 000000000000..f3c4a74bc79b
--- /dev/null
+++ b/changelog.d/8248.feature
@@ -0,0 +1 @@
+Consolidate the SSO error template across all configuration.

From 0b51a2054f74a94b8b4875ccf6927e3b56532729 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Wed, 9 Sep 2020 10:33:06 -0400
Subject: [PATCH 07/10] Review feedback.

---
 docs/sample_config.yaml          | 2 +-
 synapse/config/saml2_config.py   | 2 +-
 synapse/handlers/oidc_handler.py | 4 ++--
 synapse/handlers/saml_handler.py | 4 ++--
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml
index 8d28669f6b08..fad9833ccc00 100644
--- a/docs/sample_config.yaml
+++ b/docs/sample_config.yaml
@@ -1472,7 +1472,7 @@ trusted_key_servers:
 #   * enable_registration
 #   * password_config.enabled
 #
-# You will also want to investigate the settings under the sso configuration
+# You will also want to investigate the settings under the "sso" configuration
 # section below.
 #
 # Once SAML support is enabled, a metadata file will be exposed at
diff --git a/synapse/config/saml2_config.py b/synapse/config/saml2_config.py
index 95c0615db581..99aa8b3bf123 100644
--- a/synapse/config/saml2_config.py
+++ b/synapse/config/saml2_config.py
@@ -226,7 +226,7 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs):
         #   * enable_registration
         #   * password_config.enabled
         #
-        # You will also want to investigate the settings under the sso configuration
+        # You will also want to investigate the settings under the "sso" configuration
         # section below.
         #
         # Once SAML support is enabled, a metadata file will be exposed at
diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index 1b06f3173fa0..4230dbaf998b 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -131,10 +131,10 @@ def __init__(self, hs: "HomeServer"):
     def _render_error(
         self, request, error: str, error_description: Optional[str] = None
     ) -> None:
-        """Renders the error template and respond with it.
+        """Render the error template and respond to the request with it.
 
         This is used to show errors to the user. The template of this page can
-        be found under ``synapse/res/templates/sso_error.html``.
+        be found under `synapse/res/templates/sso_error.html`.
 
         Args:
             request: The incoming request from the browser.
diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py
index 2ab3462c4d08..8715abd4d1c7 100644
--- a/synapse/handlers/saml_handler.py
+++ b/synapse/handlers/saml_handler.py
@@ -93,10 +93,10 @@ def __init__(self, hs: "synapse.server.HomeServer"):
     def _render_error(
         self, request, error: str, error_description: Optional[str] = None
     ) -> None:
-        """Renders the error template and respond with it.
+        """Render the error template and respond to the request with it.
 
         This is used to show errors to the user. The template of this page can
-        be found under ``synapse/res/templates/sso_error.html``.
+        be found under `synapse/res/templates/sso_error.html`.
 
         Args:
             request: The incoming request from the browser.

From eb6be88a79e6a42a215d7a218c2be393172b7541 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Thu, 10 Sep 2020 14:24:25 -0400
Subject: [PATCH 08/10] Add upgrade notes.

---
 UPGRADE.rst | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/UPGRADE.rst b/UPGRADE.rst
index 77be1b2952a6..60a70e0a0198 100644
--- a/UPGRADE.rst
+++ b/UPGRADE.rst
@@ -1,3 +1,20 @@
+Upgrading to v1.21.0
+====================
+
+Updated Single Sign-on HTML Templates
+-------------------------------------
+
+The ``saml_error.html`` template was removed from Synapse and replaced with the
+``sso_error.html`` template. If your Synapse is configured to use SAML and a
+custom ``sso_redirect_confirm_template_dir`` configuration then the
+``saml_error.html`` template will need to be renamed to ``sso_error.html`` and
+updated:
+
+* The ``msg`` parameter should be renamed to ``error_description``.
+* There is no longer a ``code`` parameter for the response code.
+* A string ``error`` parameter is available that includes a short hint of why a
+  user is seeing the error page.
+
 Upgrading to v1.20.0
 ====================
 

From 3a9e289764dcb37f41824734a1c1e3b1d211542f Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Fri, 11 Sep 2020 08:44:13 -0400
Subject: [PATCH 09/10] Move upgrade notes.

---
 UPGRADE.rst | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/UPGRADE.rst b/UPGRADE.rst
index 7fbfc9680b12..e7aa540ca527 100644
--- a/UPGRADE.rst
+++ b/UPGRADE.rst
@@ -1,21 +1,3 @@
-Upgrading to v1.21.0
-====================
-
-Updated Single Sign-on HTML Templates
--------------------------------------
-
-The ``saml_error.html`` template was removed from Synapse and replaced with the
-``sso_error.html`` template. If your Synapse is configured to use SAML and a
-custom ``sso_redirect_confirm_template_dir`` configuration then the
-``saml_error.html`` template will need to be renamed to ``sso_error.html`` and
-updated:
-
-* The ``msg`` parameter should be renamed to ``error_description``.
-* There is no longer a ``code`` parameter for the response code.
-* A string ``error`` parameter is available that includes a short hint of why a
-  user is seeing the error page.
-
-
 Upgrading Synapse
 =================
 
@@ -132,6 +114,20 @@ request to
 with the query parameters from the original link, presented as a URL-encoded form. See the file
 itself for more details.
 
+Updated Single Sign-on HTML Templates
+-------------------------------------
+
+The ``saml_error.html`` template was removed from Synapse and replaced with the
+``sso_error.html`` template. If your Synapse is configured to use SAML and a
+custom ``sso_redirect_confirm_template_dir`` configuration then the
+``saml_error.html`` template will need to be renamed to ``sso_error.html`` and
+updated:
+
+* The ``msg`` parameter should be renamed to ``error_description``.
+* There is no longer a ``code`` parameter for the response code.
+* A string ``error`` parameter is available that includes a short hint of why a
+  user is seeing the error page.
+
 Upgrading to v1.18.0
 ====================
 

From d097dba90de895196e3c366e2088a73c2ee7f820 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Mon, 14 Sep 2020 08:40:54 -0400
Subject: [PATCH 10/10] Review feedback.

---
 UPGRADE.rst | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/UPGRADE.rst b/UPGRADE.rst
index e7aa540ca527..49e86e628fa4 100644
--- a/UPGRADE.rst
+++ b/UPGRADE.rst
@@ -119,9 +119,9 @@ Updated Single Sign-on HTML Templates
 
 The ``saml_error.html`` template was removed from Synapse and replaced with the
 ``sso_error.html`` template. If your Synapse is configured to use SAML and a
-custom ``sso_redirect_confirm_template_dir`` configuration then the
-``saml_error.html`` template will need to be renamed to ``sso_error.html`` and
-updated:
+custom ``sso_redirect_confirm_template_dir`` configuration then any customisations
+of the ``saml_error.html`` template will need to be merged into the ``sso_error.html``
+template. These templates are similar, but the parameters are slightly different:
 
 * The ``msg`` parameter should be renamed to ``error_description``.
 * There is no longer a ``code`` parameter for the response code.