diff --git a/CHANGELOG.md b/CHANGELOG.md index 7eb642c7..be51531d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ All notable changes to this project will be documented in this file. ## Improvements - Django 3.1 +- Handle status callbacks from Twilio, show SMS delivery failures in Integrations ## v1.16.0 - 2020-08-04 diff --git a/hc/api/models.py b/hc/api/models.py index 01a98882..a6d496e1 100644 --- a/hc/api/models.py +++ b/hc/api/models.py @@ -482,16 +482,14 @@ class Channel(models.Model): n.error = "Sending" n.save() - if self.kind == "email": - error = self.transport.notify(check, n.bounce_url()) or "" - else: - error = self.transport.notify(check) or "" + # These are not database fields. It is just a convenient way to pass + # status_url to transport classes. + check.bounce_url = n.bounce_url() + check.status_url = n.status_url() - n.error = error - n.save() - - self.last_error = error - self.save() + error = self.transport.notify(check) or "" + Notification.objects.filter(id=n.id).update(error=error) + Channel.objects.filter(id=self.id).update(last_error=error) return error @@ -765,6 +763,10 @@ class Notification(models.Model): def bounce_url(self): return settings.SITE_ROOT + reverse("hc-api-bounce", args=[self.code]) + def status_url(self): + path = reverse("hc-api-notification-status", args=[self.code]) + return settings.SITE_ROOT + path + class Flip(models.Model): owner = models.ForeignKey(Check, models.CASCADE) diff --git a/hc/api/tests/test_notification_status.py b/hc/api/tests/test_notification_status.py new file mode 100644 index 00000000..b58052cd --- /dev/null +++ b/hc/api/tests/test_notification_status.py @@ -0,0 +1,91 @@ +from datetime import timedelta + +from hc.api.models import Channel, Check, Notification +from hc.test import BaseTestCase + + +class NotificationStatusTestCase(BaseTestCase): + def setUp(self): + super(NotificationStatusTestCase, self).setUp() + + self.check = Check(project=self.project, status="up") + self.check.save() + + self.channel = Channel(project=self.project, kind="email") + self.channel.value = "alice@example.org" + self.channel.email_verified = True + self.channel.save() + + self.n = Notification(owner=self.check, channel=self.channel) + self.n.save() + + self.url = "/api/v1/notifications/%s/status" % self.n.code + + def test_it_handles_twilio_failed_status(self): + r = self.client.post(self.url, {"MessageStatus": "failed"}) + self.assertEqual(r.status_code, 200) + + self.n.refresh_from_db() + self.assertEqual(self.n.error, "Delivery failed (status=failed).") + + self.channel.refresh_from_db() + self.assertEqual(self.channel.last_error, "Delivery failed (status=failed).") + self.assertTrue(self.channel.email_verified) + + def test_it_handles_twilio_undelivered_status(self): + r = self.client.post(self.url, {"MessageStatus": "undelivered"}) + self.assertEqual(r.status_code, 200) + + self.n.refresh_from_db() + self.assertEqual(self.n.error, "Delivery failed (status=undelivered).") + + self.channel.refresh_from_db() + self.assertIn("status=undelivered", self.channel.last_error) + + def test_it_handles_twilio_delivered_status(self): + r = self.client.post(self.url, {"MessageStatus": "delivered"}) + self.assertEqual(r.status_code, 200) + + self.n.refresh_from_db() + self.assertEqual(self.n.error, "") + + self.channel.refresh_from_db() + self.assertEqual(self.channel.last_error, "") + + def test_it_checks_ttl(self): + self.n.created = self.n.created - timedelta(minutes=61) + self.n.save() + + r = self.client.post(self.url, {"MessageStatus": "failed"}) + self.assertEqual(r.status_code, 403) + + def test_it_handles_missing_notification(self): + fake_code = "07c2f548-9850-4b27-af5d-6c9dc157ec02" + url = f"/api/v1/notifications/{fake_code}/status" + r = self.client.post(url, {"MessageStatus": "failed"}) + self.assertEqual(r.status_code, 404) + + def test_it_requires_post(self): + r = self.client.get(self.url) + self.assertEqual(r.status_code, 405) + + def test_it_handles_error_key(self): + r = self.client.post(self.url, {"error": "Something went wrong."}) + self.assertEqual(r.status_code, 200) + + self.n.refresh_from_db() + self.assertEqual(self.n.error, "Something went wrong.") + + self.channel.refresh_from_db() + self.assertEqual(self.channel.last_error, "Something went wrong.") + self.assertTrue(self.channel.email_verified) + + def test_it_handles_mark_not_verified_key(self): + payload = {"error": "Received complaint.", "mark_not_verified": "1"} + + r = self.client.post(self.url, payload) + self.assertEqual(r.status_code, 200) + + self.channel.refresh_from_db() + self.assertEqual(self.channel.last_error, "Received complaint.") + self.assertFalse(self.channel.email_verified) diff --git a/hc/api/tests/test_notify.py b/hc/api/tests/test_notify.py index 2b0c82d7..d0a3c82c 100644 --- a/hc/api/tests/test_notify.py +++ b/hc/api/tests/test_notify.py @@ -60,6 +60,8 @@ class NotifyTestCase(BaseTestCase): n = Notification.objects.get() self.assertEqual(n.error, "Connection timed out") + + self.channel.refresh_from_db() self.assertEqual(self.channel.last_error, "Connection timed out") @patch("hc.api.transports.requests.request", side_effect=ConnectionError) @@ -313,6 +315,7 @@ class NotifyTestCase(BaseTestCase): email = mail.outbox[0] self.assertEqual(email.to[0], "alice@example.org") self.assertTrue("X-Bounce-Url" in email.extra_headers) + self.assertTrue("X-Status-Url" in email.extra_headers) self.assertTrue("List-Unsubscribe" in email.extra_headers) self.assertTrue("List-Unsubscribe-Post" in email.extra_headers) @@ -638,13 +641,17 @@ class NotifyTestCase(BaseTestCase): mock_post.return_value.status_code = 200 self.channel.notify(self.check) - self.assertEqual(Notification.objects.count(), 1) + + n = Notification.objects.get() args, kwargs = mock_post.call_args payload = kwargs["data"] self.assertEqual(payload["To"], "+1234567890") self.assertFalse("\xa0" in payload["Body"]) + callback_path = f"/api/v1/notifications/{n.code}/status" + self.assertTrue(payload["StatusCallback"].endswith(callback_path)) + # sent SMS counter should go up self.profile.refresh_from_db() self.assertEqual(self.profile.sms_sent, 1) diff --git a/hc/api/transports.py b/hc/api/transports.py index 37fe9b6c..81833d32 100644 --- a/hc/api/transports.py +++ b/hc/api/transports.py @@ -55,14 +55,15 @@ class Transport(object): class Email(Transport): - def notify(self, check, bounce_url): + def notify(self, check): if not self.channel.email_verified: return "Email not verified" unsub_link = self.channel.get_unsub_link() headers = { - "X-Bounce-Url": bounce_url, + "X-Bounce-Url": check.bounce_url, + "X-Status-Url": check.status_url, "List-Unsubscribe": "<%s>" % unsub_link, "List-Unsubscribe-Post": "List-Unsubscribe=One-Click", } @@ -473,6 +474,7 @@ class Sms(HttpTransport): "From": settings.TWILIO_FROM, "To": self.channel.phone_number, "Body": text, + "StatusCallback": check.status_url, } return self.post(url, data=data, auth=auth) diff --git a/hc/api/urls.py b/hc/api/urls.py index 09be5cfb..cb69d104 100644 --- a/hc/api/urls.py +++ b/hc/api/urls.py @@ -37,6 +37,11 @@ urlpatterns = [ path("api/v1/checks/", views.get_check_by_unique_key), path("api/v1/checks//pause", views.pause, name="hc-api-pause"), path("api/v1/notifications//bounce", views.bounce, name="hc-api-bounce"), + path( + "api/v1/notifications//status", + views.notification_status, + name="hc-api-notification-status", + ), path("api/v1/checks//pings/", views.pings, name="hc-api-pings"), path("api/v1/checks//flips/", views.flips_by_uuid, name="hc-api-flips"), path("api/v1/checks//flips/", views.flips_by_unique_key), diff --git a/hc/api/views.py b/hc/api/views.py index ed7af4b9..ea6677b3 100644 --- a/hc/api/views.py +++ b/hc/api/views.py @@ -415,6 +415,42 @@ def bounce(request, code): return HttpResponse() +@csrf_exempt +@require_POST +def notification_status(request, code): + """ Handle notification delivery status callbacks. """ + + notification = get_object_or_404(Notification, code=code) + + # If webhook is more than 1 hour late, don't accept it: + td = timezone.now() - notification.created + if td.total_seconds() > 3600: + return HttpResponseForbidden() + + error, mark_not_verified = None, False + + # Look for "error" and "unsub" keys: + if request.POST.get("error"): + error = request.POST["error"][:200] + mark_not_verified = request.POST.get("mark_not_verified") + + # Handle "failed" and "undelivered" callbacks from Twilio + if request.POST.get("MessageStatus") in ("failed", "undelivered"): + status = request.POST["MessageStatus"] + error = f"Delivery failed (status={status})." + + if error: + notification.error = error + notification.save(update_fields=["error"]) + + channel_q = Channel.objects.filter(id=notification.channel_id) + channel_q.update(last_error=error) + if mark_not_verified: + channel_q.update(email_verified=False) + + return HttpResponse() + + def metrics(request): if not settings.METRICS_KEY: return HttpResponseForbidden()