Merge pull request #7058 from matrix-org/babolivier/saml_error_html

* commit '6640460d0': Rephrase default message Hopefully mypy is happy now Attempt at appeasing the gods of mypy Lint Changelog Update sample config SAML2: render a comprehensible error page if something goes wrong
matrix-org · Mar 24, 2020 · 42cbe33 · 42cbe33
2 parents 42e1eef + 6640460
commit 42cbe33
Show file tree

Hide file tree

Showing 7 changed files with 81 additions and 6 deletions.
diff --git a/changelog.d/7058.feature b/changelog.d/7058.feature
@@ -0,0 +1 @@
+Render a configurable and comprehensible error page if something goes wrong during the SAML2 authentication process.
diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml
@@ -1497,6 +1497,13 @@ saml2_config:
   #
   #grandfathered_mxid_source_attribute: upn
 
+  # Path to a file containing HTML content to serve in case an error happens
+  # when the user gets redirected from the SAML IdP back to Synapse.
+  # If no file is provided, this defaults to some minimalistic HTML telling the
+  # user that something went wrong and they should try authenticating again.
+  #
+  #error_html_path: /path/to/static/content/saml_error.html
+
 
 
 # Enable CAS for registration and login.

diff --git a/synapse/config/saml2_config.py b/synapse/config/saml2_config.py
@@ -27,6 +27,18 @@
     "synapse.handlers.saml_handler.DefaultSamlMappingProvider"
 )
 
+SAML2_ERROR_DEFAULT_HTML = """
+<html>
+    <body>
+        <p>Oops! Something went wrong</p>
+        <p>
+            Try logging in again from your Matrix client and if the problem persists
+            please contact the server's administrator.
+        </p>
+    </body>
+</html>
+"""
+
 
 def _dict_merge(merge_dict, into_dict):
     """Do a deep merge of two dicts
@@ -160,6 +172,13 @@ def read_config(self, config, **kwargs):
             saml2_config.get("saml_session_lifetime", "5m")
         )
 
+        if "error_html_path" in config:
+            self.saml2_error_html_content = self.read_file(
+                config["error_html_path"], "saml2_config.error_html_path",
+            )
+        else:
+            self.saml2_error_html_content = SAML2_ERROR_DEFAULT_HTML
+
     def _default_saml_config_dict(
         self, required_attributes: set, optional_attributes: set
     ):
@@ -325,6 +344,13 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs):
           # The default is 'uid'.
           #
           #grandfathered_mxid_source_attribute: upn
+
+          # Path to a file containing HTML content to serve in case an error happens
+          # when the user gets redirected from the SAML IdP back to Synapse.
+          # If no file is provided, this defaults to some minimalistic HTML telling the
+          # user that something went wrong and they should try authenticating again.
+          #
+          #error_html_path: /path/to/static/content/saml_error.html
         """ % {
             "config_dir_path": config_dir_path
         }
diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py
@@ -23,6 +23,7 @@
 
 from synapse.api.errors import SynapseError
 from synapse.config import ConfigError
+from synapse.http.server import finish_request
 from synapse.http.servlet import parse_string
 from synapse.module_api import ModuleApi
 from synapse.types import (
@@ -73,6 +74,8 @@ def __init__(self, hs):
         # a lock on the mappings
         self._mapping_lock = Linearizer(name="saml_mapping", clock=self._clock)
 
+        self._error_html_content = hs.config.saml2_error_html_content
+
     def handle_redirect_request(self, client_redirect_url):
         """Handle an incoming request to /login/sso/redirect
 
@@ -114,7 +117,22 @@ async def handle_saml_response(self, request):
         # the dict.
         self.expire_sessions()
 
-        user_id = await self._map_saml_response_to_user(resp_bytes, relay_state)
+        try:
+            user_id = await self._map_saml_response_to_user(resp_bytes, relay_state)
+        except Exception as e:
+            # If decoding the response or mapping it to a user failed, then log the
+            # error and tell the user that something went wrong.
+            logger.error(e)
+
+            request.setResponseCode(400)
+            request.setHeader(b"Content-Type", b"text/html; charset=utf-8")
+            request.setHeader(
+                b"Content-Length", b"%d" % (len(self._error_html_content),)
+            )
+            request.write(self._error_html_content.encode("utf8"))
+            finish_request(request)
+            return
+
         self._auth_handler.complete_sso_login(user_id, request, relay_state)
 
     async def _map_saml_response_to_user(self, resp_bytes, client_redirect_url):

diff --git a/synapse/logging/context.py b/synapse/logging/context.py
@@ -210,14 +210,15 @@ class LoggingContext(object):
     class Sentinel(object):
         """Sentinel to represent the root context"""
 
-        __slots__ = ["previous_context", "alive", "request", "scope"]
+        __slots__ = ["previous_context", "alive", "request", "scope", "tag"]
 
         def __init__(self) -> None:
             # Minimal set for compatibility with LoggingContext
             self.previous_context = None
             self.alive = None
             self.request = None
             self.scope = None
+            self.tag = None
 
         def __str__(self):
             return "sentinel"
@@ -511,7 +512,7 @@ class PreserveLoggingContext(object):
 
     __slots__ = ["current_context", "new_context", "has_parent"]
 
-    def __init__(self, new_context: Optional[LoggingContext] = None) -> None:
+    def __init__(self, new_context: Optional[LoggingContextOrSentinel] = None) -> None:
         if new_context is None:
             self.new_context = LoggingContext.sentinel  # type: LoggingContextOrSentinel
         else:

diff --git a/synapse/rest/saml2/response_resource.py b/synapse/rest/saml2/response_resource.py
@@ -14,7 +14,11 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from synapse.http.server import DirectServeResource, wrap_html_request_handler
+from synapse.http.server import (
+    DirectServeResource,
+    finish_request,
+    wrap_html_request_handler,
+)
 
 
 class SAML2ResponseResource(DirectServeResource):
@@ -24,8 +28,20 @@ class SAML2ResponseResource(DirectServeResource):
 
     def __init__(self, hs):
         super().__init__()
+        self._error_html_content = hs.config.saml2_error_html_content
         self._saml_handler = hs.get_saml_handler()
 
+    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.
+        request.setResponseCode(400)
+        request.setHeader(b"Content-Type", b"text/html; charset=utf-8")
+        request.setHeader(b"Content-Length", b"%d" % (len(self._error_html_content),))
+        request.write(self._error_html_content.encode("utf8"))
+        finish_request(request)
+
     @wrap_html_request_handler
     async def _async_render_POST(self, request):
         return await self._saml_handler.handle_saml_response(request)
diff --git a/synapse/storage/database.py b/synapse/storage/database.py
@@ -29,7 +29,11 @@
 
 from synapse.api.errors import StoreError
 from synapse.config.database import DatabaseConnectionConfig
-from synapse.logging.context import LoggingContext, make_deferred_yieldable
+from synapse.logging.context import (
+    LoggingContext,
+    LoggingContextOrSentinel,
+    make_deferred_yieldable,
+)
 from synapse.metrics.background_process_metrics import run_as_background_process
 from synapse.storage.background_updates import BackgroundUpdater
 from synapse.storage.engines import BaseDatabaseEngine, PostgresEngine, Sqlite3Engine
@@ -543,7 +547,9 @@ def runWithConnection(self, func: Callable, *args: Any, **kwargs: Any):
         Returns:
             Deferred: The result of func
         """
-        parent_context = LoggingContext.current_context()
+        parent_context = (
+            LoggingContext.current_context()
+        )  # type: Optional[LoggingContextOrSentinel]
         if parent_context == LoggingContext.sentinel:
             logger.warning(
                 "Starting db connection from sentinel context: metrics will be lost"