Display an error page during failure of fallback UIA. (#10561)

8 files changed, 65 insertions, 28 deletions

diff --git a/changelog.d/10561.bugfix b/changelog.d/10561.bugfix
new file mode 100644
index 0000000000..2e4f53508c
--- /dev/null
+++ b/changelog.d/10561.bugfix
@@ -0,0 +1 @@
+Display an error on User-Interactive Authentication fallback pages when authentication fails. Contributed by Callum Brown.
diff --git a/docs/upgrade.md b/docs/upgrade.md
index 3ac2387e2a..adbd16fda5 100644
--- a/docs/upgrade.md
+++ b/docs/upgrade.md
@@ -125,6 +125,14 @@ environment variable.
 See [using a forward proxy with Synapse documentation](setup/forward_proxy.md) for
 details.
 
+## User-interactive authentication fallback templates can now display errors
+
+This may affect you if you make use of custom HTML templates for the
+[reCAPTCHA](../synapse/res/templates/recaptcha.html) or
+[terms](../synapse/res/templates/terms.html) fallback pages.
+
+The template is now provided an `error` variable if the authentication
+process failed. See the default templates linked above for an example.
 
 # Upgrading to v1.39.0
 
diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py
index 161b3c933c..98d3d2d97f 100644
--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -627,23 +627,28 @@ class AuthHandler(BaseHandler):
 
     async def add_oob_auth(
         self, stagetype: str, authdict: Dict[str, Any], clientip: str
-    ) -> bool:
+    ) -> None:
         """
         Adds the result of out-of-band authentication into an existing auth
         session. Currently used for adding the result of fallback auth.
+
+        Raises:
+            LoginError if the stagetype is unknown or the session is missing.
+            LoginError is raised by check_auth if authentication fails.
         """
         if stagetype not in self.checkers:
-            raise LoginError(400, "", Codes.MISSING_PARAM)
+            raise LoginError(
+                400, f"Unknown UIA stage type: {stagetype}", Codes.INVALID_PARAM
+            )
         if "session" not in authdict:
-            raise LoginError(400, "", Codes.MISSING_PARAM)
+            raise LoginError(400, "Missing session ID", Codes.MISSING_PARAM)
 
+        # If authentication fails a LoginError is raised. Otherwise, store
+        # the successful result.
         result = await self.checkers[stagetype].check_auth(authdict, clientip)
-        if result:
-            await self.store.mark_ui_auth_stage_complete(
-                authdict["session"], stagetype, result
-            )
-            return True
-        return False
+        await self.store.mark_ui_auth_stage_complete(
+            authdict["session"], stagetype, result
+        )
 
     def get_session_id(self, clientdict: Dict[str, Any]) -> Optional[str]:
         """
diff --git a/synapse/handlers/ui_auth/checkers.py b/synapse/handlers/ui_auth/checkers.py
index 5414ce77d8..270541cc76 100644
--- a/synapse/handlers/ui_auth/checkers.py
+++ b/synapse/handlers/ui_auth/checkers.py
@@ -49,7 +49,7 @@ class UserInteractiveAuthChecker:
             clientip: The IP address of the client.
 
         Raises:
-            SynapseError if authentication failed
+            LoginError if authentication failed.
 
         Returns:
             The result of authentication (to pass back to the client?)
@@ -131,7 +131,9 @@ class RecaptchaAuthChecker(UserInteractiveAuthChecker):
             )
             if resp_body["success"]:
                 return True
-        raise LoginError(401, "", errcode=Codes.UNAUTHORIZED)
+        raise LoginError(
+            401, "Captcha authentication failed", errcode=Codes.UNAUTHORIZED
+        )
 
 
 class _BaseThreepidAuthChecker:
@@ -191,7 +193,9 @@ class _BaseThreepidAuthChecker:
             raise AssertionError("Unrecognized threepid medium: %s" % (medium,))
 
         if not threepid:
-            raise LoginError(401, "", errcode=Codes.UNAUTHORIZED)
+            raise LoginError(
+                401, "Unable to get validated threepid", errcode=Codes.UNAUTHORIZED
+            )
 
         if threepid["medium"] != medium:
             raise LoginError(
diff --git a/synapse/res/templates/recaptcha.html b/synapse/res/templates/recaptcha.html
index 63944dc608..b3db06ef97 100644
--- a/synapse/res/templates/recaptcha.html
+++ b/synapse/res/templates/recaptcha.html
@@ -16,6 +16,9 @@ function captchaDone() {
 <body>
 <form id="registrationForm" method="post" action="{{ myurl }}">
     <div>
+        {% if error is defined %}
+            <p class="error"><strong>Error: {{ error }}</strong></p>
+        {% endif %}
         <p>
         Hello! We need to prevent computer programs and other automated
         things from creating accounts on this server.
diff --git a/synapse/res/templates/terms.html b/synapse/res/templates/terms.html
index dfef9897ee..369ff446d2 100644
--- a/synapse/res/templates/terms.html
+++ b/synapse/res/templates/terms.html
@@ -8,6 +8,9 @@
 <body>
 <form id="registrationForm" method="post" action="{{ myurl }}">
     <div>
+        {% if error is defined %}
+            <p class="error"><strong>Error: {{ error }}</strong></p>
+        {% endif %}
         <p>
             Please click the button below if you agree to the
             <a href="{{ terms_url }}">privacy policy of this homeserver.</a>
diff --git a/synapse/rest/client/auth.py b/synapse/rest/client/auth.py
index 6ea1b50a62..73284e48ec 100644
--- a/synapse/rest/client/auth.py
+++ b/synapse/rest/client/auth.py
@@ -16,7 +16,7 @@ import logging
 from typing import TYPE_CHECKING
 
 from synapse.api.constants import LoginType
-from synapse.api.errors import SynapseError
+from synapse.api.errors import LoginError, SynapseError
 from synapse.api.urls import CLIENT_API_PREFIX
 from synapse.http.server import respond_with_html
 from synapse.http.servlet import RestServlet, parse_string
@@ -95,29 +95,32 @@ class AuthRestServlet(RestServlet):
 
             authdict = {"response": response, "session": session}
 
-            success = await self.auth_handler.add_oob_auth(
-                LoginType.RECAPTCHA, authdict, request.getClientIP()
-            )
-
-            if success:
-                html = self.success_template.render()
-            else:
+            try:
+                await self.auth_handler.add_oob_auth(
+                    LoginType.RECAPTCHA, authdict, request.getClientIP()
+                )
+            except LoginError as e:
+                # Authentication failed, let user try again
                 html = self.recaptcha_template.render(
                     session=session,
                     myurl="%s/r0/auth/%s/fallback/web"
                     % (CLIENT_API_PREFIX, LoginType.RECAPTCHA),
                     sitekey=self.hs.config.recaptcha_public_key,
+                    error=e.msg,
                 )
+            else:
+                # No LoginError was raised, so authentication was successful
+                html = self.success_template.render()
+
         elif stagetype == LoginType.TERMS:
             authdict = {"session": session}
 
-            success = await self.auth_handler.add_oob_auth(
-                LoginType.TERMS, authdict, request.getClientIP()
-            )
-
-            if success:
-                html = self.success_template.render()
-            else:
+            try:
+                await self.auth_handler.add_oob_auth(
+                    LoginType.TERMS, authdict, request.getClientIP()
+                )
+            except LoginError as e:
+                # Authentication failed, let user try again
                 html = self.terms_template.render(
                     session=session,
                     terms_url="%s_matrix/consent?v=%s"
@@ -127,10 +130,16 @@ class AuthRestServlet(RestServlet):
                     ),
                     myurl="%s/r0/auth/%s/fallback/web"
                     % (CLIENT_API_PREFIX, LoginType.TERMS),
+                    error=e.msg,
                 )
+            else:
+                # No LoginError was raised, so authentication was successful
+                html = self.success_template.render()
+
         elif stagetype == LoginType.SSO:
             # The SSO fallback workflow should not post here,
             raise SynapseError(404, "Fallback SSO auth does not support POST requests.")
+
         else:
             raise SynapseError(404, "Unknown auth stage type")
 
diff --git a/synapse/static/client/register/style.css b/synapse/static/client/register/style.css
index 5a7b6eebf2..8a39b5d0f5 100644
--- a/synapse/static/client/register/style.css
+++ b/synapse/static/client/register/style.css
@@ -57,4 +57,8 @@ textarea, input {
     
     background-color: #f8f8f8;
     border: 1px #ccc solid;
-}
\ No newline at end of file
+}
+
+.error {
+	color: red;
+}