Catch error status codes in OpenMarket API responses (#412)

Fixes https://github.com/matrix-org/sydent/issues/410

Also adding some logging to help debug https://github.com/matrix-org/sydent/issues/411

changelog.d/412.bugfix

@@ -0,0 +1 @@
+Fix a bug which could cause SMS sending to fail silently.

sydent/http/httpclient.py

@@ -15,7 +15,7 @@
 import json
 import logging
 from io import BytesIO
-from typing import TYPE_CHECKING, Any, Dict, Optional
+from typing import TYPE_CHECKING, Any, Dict, Optional, Tuple
 
 from twisted.web.client import Agent, FileBodyProducer
 from twisted.web.http_headers import Headers
@@ -23,7 +23,7 @@ from twisted.web.iweb import IAgent, IResponse
 
 from sydent.http.blacklisting_reactor import BlacklistingReactorWrapper
 from sydent.http.federation_tls_options import ClientTLSOptionsFactory
-from sydent.http.httpcommon import BodyExceededMaxSize, read_body_with_max_size
+from sydent.http.httpcommon import read_body_with_max_size
 from sydent.http.matrixfederationagent import MatrixFederationAgent
 from sydent.types import JsonDict
 from sydent.util import json_decoder
@@ -68,7 +68,7 @@ class HTTPClient:
     async def post_json_get_nothing(
         self, uri: str, post_json: Dict[Any, Any], opts: Dict[str, Any]
     ) -> IResponse:
-        """Make a POST request to an endpoint returning JSON and parse result
+        """Make a POST request to an endpoint returning nothing
 
         :param uri: The URI to make a POST request to.
 
@@ -80,6 +80,32 @@ class HTTPClient:
 
         :return: a response from the remote server.
         """
+        resp, _ = await self.post_json_maybe_get_json(uri, post_json, opts)
+        return resp
+
+    async def post_json_maybe_get_json(
+        self,
+        uri: str,
+        post_json: Dict[Any, Any],
+        opts: Dict[str, Any],
+        max_size: Optional[int] = None,
+    ) -> Tuple[IResponse, Optional[JsonDict]]:
+        """Make a POST request to an endpoint that might be returning JSON and parse
+        result
+
+        :param uri: The URI to make a POST request to.
+
+        :param post_json: A Python object that will be converted to a JSON
+            string and POSTed to the given URI.
+
+        :param opts: A dictionary of request options. Currently only opts.headers
+            is supported.
+
+        :param max_size: The maximum size (in bytes) to allow as a response.
+
+        :return: a response from the remote server, and its decoded JSON body if any (None
+            otherwise).
+        """
         json_bytes = json.dumps(post_json).encode("utf8")
 
         headers = opts.get(
@@ -103,13 +129,17 @@ class HTTPClient:
         # Ensure the body object is read otherwise we'll leak HTTP connections
         # as per
         # https://twistedmatrix.com/documents/current/web/howto/client.html
+        json_body = None
         try:
             # TODO Will this cause the server to think the request was a failure?
-            await read_body_with_max_size(response, 0)
-        except BodyExceededMaxSize:
+            body = await read_body_with_max_size(response, max_size)
+            json_body = json_decoder.decode(body.decode("UTF-8"))
+        except Exception:
+            # We might get an exception here because the body exceeds the max_size, or it
+            # isn't valid JSON. In both cases, we don't care about it.
             pass
 
-        return response
+        return response, json_body
 
 
 class SimpleHttpClient(HTTPClient):

sydent/sms/openmarket.py

@@ -94,11 +94,42 @@ class OpenMarketSMS:
             }
         )
 
-        resp = await self.http_cli.post_json_get_nothing(
+        resp, body = await self.http_cli.post_json_maybe_get_json(
             API_BASE_URL, send_body, {"headers": req_headers}
         )
+
         headers = dict(resp.headers.getAllRawHeaders())
 
+        request_id = None
+        if "X-Request-Id" in headers:
+            request_id = headers["X-Request-Id"][0]
+
+        # Catch errors from the API. The documentation says a success code should be 202
+        # Accepted, but let's be broad here just in case and accept all 2xx response
+        # codes.
+        #
+        # Relevant OpenMarket API documentation:
+        # https://www.openmarket.com/docs/Content/apis/v4http/send-json.htm
+        if resp.code < 200 or resp.code >= 300:
+            if body is None or "error" not in body:
+                raise Exception(
+                    "OpenMarket API responded with status %d (request ID: %s)"
+                    % (
+                        resp.code,
+                        request_id,
+                    ),
+                )
+
+            error = body["error"]
+            raise Exception(
+                "OpenMarket API responded with status %d (request ID: %s): %s"
+                % (
+                    resp.code,
+                    request_id,
+                    error,
+                ),
+            )
+
         if "Location" not in headers:
             raise Exception("Got response from sending SMS with no location header")
         # Nominally we should parse the URL, but we can just split on '/' since
@@ -108,3 +139,11 @@ class OpenMarketSMS:
             raise Exception(
                 "Got response from sending SMS with malformed location header"
             )
+
+        logger.info(
+            "Successfully sent SMS (ticket ID: %s, request ID %s), OpenMarket API"
+            " responded with code %d",
+            parts[-1],
+            request_id,
+            resp.code,
+        )