diff --git a/changelog.d/412.bugfix b/changelog.d/412.bugfix new file mode 100644 index 00000000..eedd6c88 --- /dev/null +++ b/changelog.d/412.bugfix @@ -0,0 +1 @@ +Fix a bug which could cause SMS sending to fail silently. diff --git a/sydent/http/httpclient.py b/sydent/http/httpclient.py index 9b4eaedd..6394378d 100644 --- a/sydent/http/httpclient.py +++ b/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 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 @@ async def get_json(self, uri: str, max_size: Optional[int] = None) -> JsonDict: 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 @@ async def post_json_get_nothing( :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 @@ async def post_json_get_nothing( # 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): diff --git a/sydent/sms/openmarket.py b/sydent/sms/openmarket.py index f2c93686..bfa099ee 100644 --- a/sydent/sms/openmarket.py +++ b/sydent/sms/openmarket.py @@ -94,11 +94,42 @@ async def sendTextSMS( } ) - 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 @@ async def sendTextSMS( 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, + )