Repository: matrix-org/sydent (language: python)

Issue:
Catch failures to contact remote homeserver for `/register`
Make it and the other "the remove homeserver did something silly" cases explicitly return 500. Log a warning message so that we can see the problem in logs, but not in sentry. This addresses the bulk of the errors spotted by matrix-org/matrix-spec#40.

[The spec](https://matrix.org/docs/spec/identity_service/r0.3.0#post-matrix-identity-v2-account-register) has nothing to say about status codes in the failure case. (matrix-org/matrix-doc#3470)
Catch failures to decode JSON in /register
Like #456, but for another error case.

If the remote homeserver fails to give us sensible response, that's not
a problem in our application. We can continue to serve everyone else. We
should tell the requester what happened and log a warning, but not an
error to Sentry.

Most of the examples I saw in Sentry were 404 responses serving up a
generic "page not found" body.


Discussion / hints:
Another sentry error comes from this request timing out. I'll extend this PR to handle that case too.
> I'm not convinced about the 500 error, but I think at need clarification from the spec side there.

FWIW I'm not very happy with it either, but at least this is consistent with the existing behaviour.


Fix patch:
diff --git a/changelog.d/466.misc b/changelog.d/466.misc
new file mode 100644
index 00000000..1635919c
--- /dev/null
+++ b/changelog.d/466.misc
@@ -0,0 +1,1 @@
+Handle errors to contact homeservers in `/unbind`. This returns a better errro message and reduces Sentry spam.
\ No newline at end of file
diff --git a/sydent/http/servlets/threepidunbindservlet.py b/sydent/http/servlets/threepidunbindservlet.py
index 213b98a7..d2b6e729 100644
--- a/sydent/http/servlets/threepidunbindservlet.py
+++ b/sydent/http/servlets/threepidunbindservlet.py
@@ -14,11 +14,14 @@
 # limitations under the License.
 
 import logging
+from http import HTTPStatus
 from typing import TYPE_CHECKING
 
 from signedjson.sign import SignatureVerifyException
 from twisted.internet import defer
+from twisted.internet.error import ConnectError, DNSLookupError
 from twisted.web import server
+from twisted.web.client import ResponseFailed
 from twisted.web.resource import Resource
 from twisted.web.server import Request
 
@@ -60,7 +63,7 @@ async def _async_render_POST(self, request: Request) -> None:
                     request.content.read().decode("UTF-8")
                 )
             except ValueError:
-                request.setResponseCode(400)
+                request.setResponseCode(HTTPStatus.BAD_REQUEST)
                 request.write(
                     dict_to_json_bytes(
                         {"errcode": "M_BAD_JSON", "error": "Malformed JSON"}
@@ -71,7 +74,7 @@ async def _async_render_POST(self, request: Request) -> None:
 
             missing = [k for k in ("threepid", "mxid") if k not in body]
             if len(missing) > 0:
-                request.setResponseCode(400)
+                request.setResponseCode(HTTPStatus.BAD_REQUEST)
                 msg = "Missing parameters: " + (",".join(missing))
                 request.write(
                     dict_to_json_bytes({"errcode": "M_MISSING_PARAMS", "error": msg})
@@ -83,7 +86,7 @@ async def _async_render_POST(self, request: Request) -> None:
             mxid = body["mxid"]
 
             if "medium" not in threepid or "address" not in threepid:
-                request.setResponseCode(400)
+                request.setResponseCode(HTTPStatus.BAD_REQUEST)
                 request.write(
                     dict_to_json_bytes(
                         {
@@ -114,7 +117,7 @@ async def _async_render_POST(self, request: Request) -> None:
                 client_secret = body["client_secret"]
 
                 if not is_valid_client_secret(client_secret):
-                    request.setResponseCode(400)
+                    request.setResponseCode(HTTPStatus.BAD_REQUEST)
                     request.write(
                         dict_to_json_bytes(
                             {
@@ -131,7 +134,7 @@ async def _async_render_POST(self, request: Request) -> None:
                 try:
                     s = valSessionStore.getValidatedSession(sid, client_secret)
                 except (IncorrectClientSecretException, InvalidSessionIdException):
-                    request.setResponseCode(401)
+                    request.setResponseCode(HTTPStatus.UNAUTHORIZED)
                     request.write(
                         dict_to_json_bytes(
                             {
@@ -143,7 +146,7 @@ async def _async_render_POST(self, request: Request) -> None:
                     request.finish()
                     return
                 except SessionNotValidatedException:
-                    request.setResponseCode(403)
+                    request.setResponseCode(HTTPStatus.FORBIDDEN)
                     request.write(
                         dict_to_json_bytes(
                             {
@@ -155,7 +158,7 @@ async def _async_render_POST(self, request: Request) -> None:
                     return
 
                 if s.medium != threepid["medium"] or s.address != threepid["address"]:
-                    request.setResponseCode(403)
+                    request.setResponseCode(HTTPStatus.FORBIDDEN)
                     request.write(
                         dict_to_json_bytes(
                             {
@@ -174,21 +177,21 @@ async def _async_render_POST(self, request: Request) -> None:
                         )
                     )
                 except SignatureVerifyException as ex:
-                    request.setResponseCode(401)
+                    request.setResponseCode(HTTPStatus.UNAUTHORIZED)
                     request.write(
                         dict_to_json_bytes({"errcode": "M_FORBIDDEN", "error": str(ex)})
                     )
                     request.finish()
                     return
                 except NoAuthenticationError as ex:
-                    request.setResponseCode(401)
+                    request.setResponseCode(HTTPStatus.UNAUTHORIZED)
                     request.write(
                         dict_to_json_bytes({"errcode": "M_FORBIDDEN", "error": str(ex)})
                     )
                     request.finish()
                     return
                 except InvalidServerName as ex:
-                    request.setResponseCode(400)
+                    request.setResponseCode(HTTPStatus.BAD_REQUEST)
                     request.write(
                         dict_to_json_bytes(
                             {"errcode": "M_INVALID_PARAM", "error": str(ex)}
@@ -196,9 +199,26 @@ async def _async_render_POST(self, request: Request) -> None:
                     )
                     request.finish()
                     return
+                except (DNSLookupError, ConnectError, ResponseFailed) as e:
+                    msg = (
+                        f"Unable to contact the Matrix homeserver to "
+                        f"authenticate request ({type(e).__name__})"
+                    )
+                    logger.warning(msg)
+                    request.setResponseCode(HTTPStatus.INTERNAL_SERVER_ERROR)
+                    request.write(
+                        dict_to_json_bytes(
+                            {
+                                "errcode": "M_UNKNOWN",
+                                "error": msg,
+                            }
+                        )
+                    )
+                    request.finish()
+                    return
                 except Exception:
                     logger.exception("Exception whilst authenticating unbind request")
-                    request.setResponseCode(500)
+                    request.setResponseCode(HTTPStatus.INTERNAL_SERVER_ERROR)
                     request.write(
                         dict_to_json_bytes(
                             {"errcode": "M_UNKNOWN", "error": "Internal Server Error"}
@@ -208,7 +228,7 @@ async def _async_render_POST(self, request: Request) -> None:
                     return
 
                 if not mxid.endswith(":" + origin_server_name):
-                    request.setResponseCode(403)
+                    request.setResponseCode(HTTPStatus.FORBIDDEN)
                     request.write(
                         dict_to_json_bytes(
                             {
@@ -226,7 +246,7 @@ async def _async_render_POST(self, request: Request) -> None:
             request.finish()
         except Exception as ex:
             logger.exception("Exception whilst handling unbind")
-            request.setResponseCode(500)
+            request.setResponseCode(HTTPStatus.INTERNAL_SERVER_ERROR)
             request.write(
                 dict_to_json_bytes({"errcode": "M_UNKNOWN", "error": str(ex)})
             )


Test patch:
diff --git a/tests/test_threepidunbind.py b/tests/test_threepidunbind.py
new file mode 100644
index 00000000..fe349744
--- /dev/null
+++ b/tests/test_threepidunbind.py
@@ -0,0 +1,66 @@
+# Copyright 2021 The Matrix.org Foundation C.I.C.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# 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 http import HTTPStatus
+from unittest.mock import patch
+
+import twisted.internet.error
+import twisted.web.client
+from parameterized import parameterized
+from twisted.trial import unittest
+
+from tests.utils import make_request, make_sydent
+
+
+class ThreepidUnbindTestCase(unittest.TestCase):
+    """Tests Sydent's threepidunbind servlet"""
+
+    def setUp(self) -> None:
+        # Create a new sydent
+        self.sydent = make_sydent()
+
+    # Duplicated from TestRegisterServelet. Is there a way for us to keep
+    # ourselves DRY?
+    @parameterized.expand(
+        [
+            (twisted.internet.error.DNSLookupError(),),
+            (twisted.internet.error.TimeoutError(),),
+            (twisted.internet.error.ConnectionRefusedError(),),
+            # Naughty: strictly we're supposed to initialise a ResponseNeverReceived
+            # with a list of 1 or more failures.
+            (twisted.web.client.ResponseNeverReceived([]),),
+        ]
+    )
+    def test_connection_failure(self, exc: Exception) -> None:
+        """Check we respond sensibly if we can't contact the homeserver."""
+        self.sydent.run()
+        request, channel = make_request(
+            self.sydent.reactor,
+            "POST",
+            "/_matrix/identity/v2/3pid/unbind",
+            content={
+                "mxid": "@alice:wonderland",
+                "threepid": {
+                    "address": "alice.cooper@wonderland.biz",
+                    "medium": "email",
+                },
+            },
+        )
+
+        with patch.object(
+            self.sydent.sig_verifier, "authenticate_request", side_effect=exc
+        ):
+            request.render(self.sydent.servlets.threepidUnbind)
+        self.assertEqual(channel.code, HTTPStatus.INTERNAL_SERVER_ERROR)
+        self.assertEqual(channel.json_body["errcode"], "M_UNKNOWN")
+        self.assertIn("contact", channel.json_body["error"])