From d33c9ea4ed3c44b518d163b460b4d204411083c2 Mon Sep 17 00:00:00 2001 From: medmunds Date: Tue, 26 Jan 2021 16:29:14 -0800 Subject: [PATCH] Mailgun: improve API error messages --- CHANGELOG.rst | 2 ++ anymail/backends/mailgun.py | 24 ++++++++++++++++++++++++ anymail/webhooks/mailgun.py | 9 +++++++++ tests/mock_requests_backend.py | 14 ++++++++++++-- tests/test_mailgun_backend.py | 25 +++++++++++++++++++++++++ tests/test_mailgun_inbound.py | 23 +++++++++++++++++++++++ 6 files changed, 95 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index e50e8bc..276b1f4 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -40,6 +40,8 @@ Fixes Other ~~~~~ +* **Mailgun:** Improve error messages for some common configuration issues. + * Test against Django 3.2 prerelease (including support for Python 3.9) * Move CI testing to GitHub Actions (and stop using Travis-CI). diff --git a/anymail/backends/mailgun.py b/anymail/backends/mailgun.py index d584e85..76ddabc 100644 --- a/anymail/backends/mailgun.py +++ b/anymail/backends/mailgun.py @@ -47,6 +47,30 @@ class EmailBackend(AnymailRequestsBackend): def build_message_payload(self, message, defaults): return MailgunPayload(message, defaults, self) + def raise_for_status(self, response, payload, message): + # Mailgun issues a terse 404 for unrecognized sender domains. + # Add some context: + if response.status_code == 404 and "Domain not found" in response.text: + raise AnymailRequestsAPIError( + "Unknown sender domain {sender_domain!r}.\n" + "Check the domain is verified with Mailgun, and that the ANYMAIL" + " MAILGUN_API_URL setting {api_url!r} is the correct region.".format( + sender_domain=payload.sender_domain, api_url=self.api_url), + email_message=message, payload=payload, + response=response, backend=self) + + super().raise_for_status(response, payload, message) + + # Mailgun issues a cryptic "Mailgun Magnificent API" success response + # for invalid API endpoints. Convert that to a useful error: + if response.status_code == 200 and "Mailgun Magnificent API" in response.text: + raise AnymailRequestsAPIError( + "Invalid Mailgun API endpoint %r.\n" + "Check your ANYMAIL MAILGUN_SENDER_DOMAIN" + " and MAILGUN_API_URL settings." % response.url, + email_message=message, payload=payload, + response=response, backend=self) + def parse_recipient_status(self, response, payload, message): # The *only* 200 response from Mailgun seems to be: # { diff --git a/anymail/webhooks/mailgun.py b/anymail/webhooks/mailgun.py index 2435f8d..50c6d9a 100644 --- a/anymail/webhooks/mailgun.py +++ b/anymail/webhooks/mailgun.py @@ -352,6 +352,15 @@ class MailgunInboundWebhookView(MailgunBaseWebhookView): "You seem to have set Mailgun's *%s tracking* webhook " "to Anymail's Mailgun *inbound* webhook URL." % request.POST['event']) + if 'attachments' in request.POST: + # Inbound route used store() rather than forward(). + # ("attachments" seems to be the only POST param that differs between + # store and forward; Anymail could support store by handling the JSON + # attachments param in message_from_mailgun_parsed.) + raise AnymailConfigurationError( + "You seem to have configured Mailgun's receiving route using the store()" + " action. Anymail's inbound webhook requires the forward() action.") + if 'body-mime' in request.POST: # Raw-MIME message = AnymailInboundMessage.parse_raw_mime(request.POST['body-mime']) diff --git a/tests/mock_requests_backend.py b/tests/mock_requests_backend.py index 5dd4c39..750da98 100644 --- a/tests/mock_requests_backend.py +++ b/tests/mock_requests_backend.py @@ -21,12 +21,22 @@ class RequestsBackendMockAPITestCase(AnymailTestMixin, SimpleTestCase): class MockResponse(requests.Response): """requests.request return value mock sufficient for testing""" - def __init__(self, status_code=200, raw=b"RESPONSE", encoding='utf-8', reason=None): + def __init__(self, status_code=200, raw=b"RESPONSE", encoding='utf-8', reason=None, test_case=None): super().__init__() self.status_code = status_code self.encoding = encoding self.reason = reason or ("OK" if 200 <= status_code < 300 else "ERROR") self.raw = BytesIO(raw) + self.test_case = test_case + + @property + def url(self): + return self.test_case.get_api_call_arg('url', required=False) + + @url.setter + def url(self, url): + if url is not None: + raise ValueError("MockResponse can't handle url assignment") def setUp(self): super().setUp() @@ -38,7 +48,7 @@ class RequestsBackendMockAPITestCase(AnymailTestMixin, SimpleTestCase): def set_mock_response(self, status_code=DEFAULT_STATUS_CODE, raw=UNSET, encoding='utf-8', reason=None): if raw is UNSET: raw = self.DEFAULT_RAW_RESPONSE - mock_response = self.MockResponse(status_code, raw=raw, encoding=encoding, reason=reason) + mock_response = self.MockResponse(status_code, raw=raw, encoding=encoding, reason=reason, test_case=self) self.mock_request.return_value = mock_response return mock_response diff --git a/tests/test_mailgun_backend.py b/tests/test_mailgun_backend.py index 0b8dcbb..856519b 100644 --- a/tests/test_mailgun_backend.py +++ b/tests/test_mailgun_backend.py @@ -667,6 +667,31 @@ class MailgunBackendAnymailFeatureTests(MailgunBackendMockAPITestCase): self.message.send() self.assert_esp_called('/example.com%20%23%20oops/messages') + def test_unknown_sender_domain(self): + self.set_mock_response(raw=b"""{ + "message": "Domain not found: example.com" + }""", status_code=404) + with self.assertRaisesMessage( + AnymailAPIError, + "Unknown sender domain 'example.com'.\n" + "Check the domain is verified with Mailgun, and that the ANYMAIL MAILGUN_API_URL" + " setting 'https://api.mailgun.net/v3/' is the correct region." + ): + self.message.send() + + @override_settings( + # This is *not* a valid MAILGUN_API_URL setting (it should end at "...v3/"): + ANYMAIL_MAILGUN_API_URL='https://api.mailgun.net/v3/example.com/messages') + def test_magnificent_api(self): + # (Wouldn't a truly "magnificent API" just provide a helpful error message?) + self.set_mock_response(raw=b"Mailgun Magnificent API", status_code=200) + with self.assertRaisesMessage( + AnymailAPIError, + "Invalid Mailgun API endpoint 'https://api.mailgun.net/v3/example.com/messages/example.com/messages'.\n" + "Check your ANYMAIL MAILGUN_SENDER_DOMAIN and MAILGUN_API_URL settings." + ): + self.message.send() + def test_default_omits_options(self): """Make sure by default we don't send any ESP-specific options. diff --git a/tests/test_mailgun_inbound.py b/tests/test_mailgun_inbound.py index e9bf93b..39c2243 100644 --- a/tests/test_mailgun_inbound.py +++ b/tests/test_mailgun_inbound.py @@ -196,6 +196,29 @@ class MailgunInboundTestCase(WebhookTestCase): self.client.post('/anymail/mailgun/inbound/', data=json.dumps(raw_event), content_type='application/json') + def test_misconfigured_store_action(self): + # store() notification includes "attachments" json; forward() includes "attachment-count" + raw_event = mailgun_sign_legacy_payload({ + 'token': '06c96bafc3f42a66b9edd546347a2fe18dc23461fe80dc52f0', + 'timestamp': '1461261330', + 'recipient': 'test@inbound.example.com', + 'sender': 'envelope-from@example.org', + 'body-plain': 'Test body plain', + 'body-html': '
Test body html
', + 'attachments': json.dumps([{ + "url": "https://storage.mailgun.net/v3/domains/example.com/messages/MESSAGE_KEY/attachments/0", + "content-type": "application/pdf", + "name": "attachment.pdf", + "size": 20202 + }]), + }) + with self.assertRaisesMessage( + AnymailConfigurationError, + "You seem to have configured Mailgun's receiving route using the store() action." + " Anymail's inbound webhook requires the forward() action." + ): + self.client.post('/anymail/mailgun/inbound/', data=raw_event) + def test_misconfigured_tracking_legacy(self): raw_event = mailgun_sign_legacy_payload({ 'domain': 'example.com',