From d054970b0239534d21b5e5c4cee202f56bc2bd30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C4=93teris=20Caune?= Date: Tue, 28 May 2019 14:25:29 +0300 Subject: [PATCH] Webhooks support PUT method. .Webhooks can have different request bodies and headers for "up" and "events". --- CHANGELOG.md | 2 + hc/api/models.py | 70 +++--- hc/api/tests/test_notify.py | 84 ++++++- hc/api/transports.py | 24 +- hc/front/forms.py | 73 +++--- hc/front/templatetags/hc_extras.py | 5 + hc/front/tests/test_add_webhook.py | 95 +++++--- static/css/add_webhook.css | 36 +++ static/js/webhook.js | 32 +-- templates/front/channels.html | 50 +++-- templates/integrations/add_webhook.html | 283 +++++++++++++----------- 11 files changed, 482 insertions(+), 272 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c5122d5..592313e5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ All notable changes to this project will be documented in this file. ### Improvements - Add the `prunetokenbucket` management command - Show check counts in JSON "badges" (#251) +- Webhooks support PUT method (#249) +- Webhooks can have different request bodies and headers for "up" and "events" (#249) ### Bug Fixes - Fix badges for tags containing special characters (#240, #237) diff --git a/hc/api/models.py b/hc/api/models.py index 141d7aae..900045c1 100644 --- a/hc/api/models.py +++ b/hc/api/models.py @@ -360,44 +360,62 @@ class Channel(models.Model): prio = int(parts[1]) return PO_PRIORITIES[prio] - @property - def url_down(self): + def webhook_spec(self, status): assert self.kind == "webhook" - if not self.value.startswith("{"): - parts = self.value.split("\n") - return parts[0] - - doc = json.loads(self.value) - return doc.get("url_down") - @property - def url_up(self): - assert self.kind == "webhook" if not self.value.startswith("{"): parts = self.value.split("\n") - return parts[1] if len(parts) > 1 else "" + url_down = parts[0] + url_up = parts[1] if len(parts) > 1 else "" + post_data = parts[2] if len(parts) > 2 else "" + + return { + "method": "POST" if post_data else "GET", + "url": url_down if status == "down" else url_up, + "body": post_data, + "headers": {}, + } doc = json.loads(self.value) - return doc.get("url_up") + if "post_data" in doc: + # Legacy "post_data" in doc -- use the legacy fields + return { + "method": "POST" if doc["post_data"] else "GET", + "url": doc["url_down"] if status == "down" else doc["url_up"], + "body": doc["post_data"], + "headers": doc["headers"], + } + + if status == "down" and "method_down" in doc: + return { + "method": doc["method_down"], + "url": doc["url_down"], + "body": doc["body_down"], + "headers": doc["headers_down"], + } + elif status == "up" and "method_up" in doc: + return { + "method": doc["method_up"], + "url": doc["url_up"], + "body": doc["body_up"], + "headers": doc["headers_up"], + } @property - def post_data(self): - assert self.kind == "webhook" - if not self.value.startswith("{"): - parts = self.value.split("\n") - return parts[2] if len(parts) > 2 else "" + def down_webhook_spec(self): + return self.webhook_spec("down") - doc = json.loads(self.value) - return doc.get("post_data") + @property + def up_webhook_spec(self): + return self.webhook_spec("up") @property - def headers(self): - assert self.kind == "webhook" - if not self.value.startswith("{"): - return {} + def url_down(self): + return self.down_webhook_spec["url"] - doc = json.loads(self.value) - return doc.get("headers", {}) + @property + def url_up(self): + return self.up_webhook_spec["url"] @property def slack_team(self): diff --git a/hc/api/tests/test_notify.py b/hc/api/tests/test_notify.py index 9f1f034c..24303846 100644 --- a/hc/api/tests/test_notify.py +++ b/hc/api/tests/test_notify.py @@ -146,8 +146,8 @@ class NotifyTestCase(BaseTestCase): self.assertIsInstance(kwargs["data"], bytes) @patch("hc.api.transports.requests.request") - def test_webhooks_handle_json_value(self, mock_request): - definition = {"url_down": "http://foo.com"} + def test_legacy_webhooks_handle_json_value(self, mock_request): + definition = {"url_down": "http://foo.com", "post_data": "", "headers": {}} self._setup_data("webhook", json.dumps(definition)) self.channel.notify(self.check) @@ -156,9 +156,24 @@ class NotifyTestCase(BaseTestCase): "get", "http://foo.com", headers=headers, timeout=5 ) + @patch("hc.api.transports.requests.request") + def test_legacy_webhooks_handle_json_up_event(self, mock_request): + definition = {"url_up": "http://bar", "post_data": "", "headers": {}} + + self._setup_data("webhook", json.dumps(definition), status="up") + self.channel.notify(self.check) + + headers = {"User-Agent": "healthchecks.io"} + mock_request.assert_called_with("get", "http://bar", headers=headers, timeout=5) + @patch("hc.api.transports.requests.request") def test_webhooks_handle_json_up_event(self, mock_request): - definition = {"url_up": "http://bar"} + definition = { + "method_up": "GET", + "url_up": "http://bar", + "body_up": "", + "headers_up": {} + } self._setup_data("webhook", json.dumps(definition), status="up") self.channel.notify(self.check) @@ -167,7 +182,7 @@ class NotifyTestCase(BaseTestCase): mock_request.assert_called_with("get", "http://bar", headers=headers, timeout=5) @patch("hc.api.transports.requests.request") - def test_webhooks_handle_post_headers(self, mock_request): + def test_legacy_webhooks_handle_post_headers(self, mock_request): definition = { "url_down": "http://foo.com", "post_data": "data", @@ -183,9 +198,27 @@ class NotifyTestCase(BaseTestCase): ) @patch("hc.api.transports.requests.request") - def test_webhooks_handle_get_headers(self, mock_request): + def test_webhooks_handle_post_headers(self, mock_request): + definition = { + "method_down": "POST", + "url_down": "http://foo.com", + "body_down": "data", + "headers_down": {"Content-Type": "application/json"}, + } + + self._setup_data("webhook", json.dumps(definition)) + self.channel.notify(self.check) + + headers = {"User-Agent": "healthchecks.io", "Content-Type": "application/json"} + mock_request.assert_called_with( + "post", "http://foo.com", data=b"data", headers=headers, timeout=5 + ) + + @patch("hc.api.transports.requests.request") + def test_legacy_webhooks_handle_get_headers(self, mock_request): definition = { "url_down": "http://foo.com", + "post_data": "", "headers": {"Content-Type": "application/json"}, } @@ -198,9 +231,27 @@ class NotifyTestCase(BaseTestCase): ) @patch("hc.api.transports.requests.request") - def test_webhooks_allow_user_agent_override(self, mock_request): + def test_webhooks_handle_get_headers(self, mock_request): definition = { + "method_down": "GET", "url_down": "http://foo.com", + "body_down": "", + "headers_down": {"Content-Type": "application/json"}, + } + + self._setup_data("webhook", json.dumps(definition)) + self.channel.notify(self.check) + + headers = {"User-Agent": "healthchecks.io", "Content-Type": "application/json"} + mock_request.assert_called_with( + "get", "http://foo.com", headers=headers, timeout=5 + ) + + @patch("hc.api.transports.requests.request") + def test_legacy_webhooks_allow_user_agent_override(self, mock_request): + definition = { + "url_down": "http://foo.com", + "post_data": "", "headers": {"User-Agent": "My-Agent"}, } @@ -212,11 +263,30 @@ class NotifyTestCase(BaseTestCase): "get", "http://foo.com", headers=headers, timeout=5 ) + @patch("hc.api.transports.requests.request") + def test_webhooks_allow_user_agent_override(self, mock_request): + definition = { + "method_down": "GET", + "url_down": "http://foo.com", + "body_down": "", + "headers_down": {"User-Agent": "My-Agent"}, + } + + self._setup_data("webhook", json.dumps(definition)) + self.channel.notify(self.check) + + headers = {"User-Agent": "My-Agent"} + mock_request.assert_called_with( + "get", "http://foo.com", headers=headers, timeout=5 + ) + @patch("hc.api.transports.requests.request") def test_webhooks_support_variables_in_headers(self, mock_request): definition = { + "method_down": "GET", "url_down": "http://foo.com", - "headers": {"X-Message": "$NAME is DOWN"}, + "body_down": "", + "headers_down": {"X-Message": "$NAME is DOWN"}, } self._setup_data("webhook", json.dumps(definition)) diff --git a/hc/api/transports.py b/hc/api/transports.py index b2a6b118..f61b6cf0 100644 --- a/hc/api/transports.py +++ b/hc/api/transports.py @@ -178,22 +178,24 @@ class Webhook(HttpTransport): return False def notify(self, check): - url = self.channel.url_down - if check.status == "up": - url = self.channel.url_up - - assert url + spec = self.channel.webhook_spec(check.status) + assert spec["url"] - url = self.prepare(url, check, urlencode=True) + url = self.prepare(spec["url"], check, urlencode=True) headers = {} - for key, value in self.channel.headers.items(): + for key, value in spec["headers"].items(): headers[key] = self.prepare(value, check) - if self.channel.post_data: - payload = self.prepare(self.channel.post_data, check) - return self.post(url, data=payload.encode(), headers=headers) - else: + body = spec["body"] + if body: + body = self.prepare(body, check) + + if spec["method"] == "GET": return self.get(url, headers=headers) + elif spec["method"] == "POST": + return self.post(url, data=body.encode(), headers=headers) + elif spec["method"] == "PUT": + return self.put(url, data=body.encode(), headers=headers) class Slack(HttpTransport): diff --git a/hc/front/forms.py b/hc/front/forms.py index a43f0856..dda7af49 100644 --- a/hc/front/forms.py +++ b/hc/front/forms.py @@ -1,10 +1,11 @@ from datetime import timedelta as td import json -import re from urllib.parse import quote, urlencode from django import forms +from django.forms import URLField from django.conf import settings +from django.core.exceptions import ValidationError from django.core.validators import RegexValidator from hc.front.validators import ( CronExpressionValidator, @@ -14,6 +15,37 @@ from hc.front.validators import ( import requests +class HeadersField(forms.Field): + message = """Use "Header-Name: value" pairs, one per line.""" + + def to_python(self, value): + if not value: + return {} + + headers = {} + for line in value.split("\n"): + if not line.strip(): + continue + + if ":" not in value: + raise ValidationError(self.message) + + n, v = line.split(":", maxsplit=1) + n, v = n.strip(), v.strip() + if not n or not v: + raise ValidationError(message=self.message) + + headers[n] = v + + return headers + + def validate(self, value): + super().validate(value) + for k, v in value.items(): + if len(k) > 1000 or len(v) > 1000: + raise ValidationError("Value too long") + + class NameTagsForm(forms.Form): name = forms.CharField(max_length=100, required=False) tags = forms.CharField(max_length=500, required=False) @@ -68,49 +100,28 @@ class AddUrlForm(forms.Form): value = forms.URLField(max_length=1000, validators=[WebhookValidator()]) -_valid_header_name = re.compile(r"\A[^:\s][^:\r\n]*\Z").match +METHODS = ("GET", "POST", "PUT") class AddWebhookForm(forms.Form): error_css_class = "has-error" - url_down = forms.URLField( + method_down = forms.ChoiceField(initial="GET", choices=zip(METHODS, METHODS)) + body_down = forms.CharField(max_length=1000, required=False) + headers_down = HeadersField(required=False) + url_down = URLField( max_length=1000, required=False, validators=[WebhookValidator()] ) + method_up = forms.ChoiceField(initial="GET", choices=zip(METHODS, METHODS)) + body_up = forms.CharField(max_length=1000, required=False) + headers_up = HeadersField(required=False) url_up = forms.URLField( max_length=1000, required=False, validators=[WebhookValidator()] ) - post_data = forms.CharField(max_length=1000, required=False) - - def __init__(self, *args, **kwargs): - super(AddWebhookForm, self).__init__(*args, **kwargs) - - self.invalid_header_names = set() - self.headers = {} - if "header_key[]" in self.data and "header_value[]" in self.data: - keys = self.data.getlist("header_key[]") - values = self.data.getlist("header_value[]") - for key, value in zip(keys, values): - if not key: - continue - - if not _valid_header_name(key): - self.invalid_header_names.add(key) - - self.headers[key] = value - - def clean(self): - if self.invalid_header_names: - raise forms.ValidationError("Invalid header names") - - return self.cleaned_data - def get_value(self): - val = dict(self.cleaned_data) - val["headers"] = self.headers - return json.dumps(val, sort_keys=True) + return json.dumps(dict(self.cleaned_data), sort_keys=True) phone_validator = RegexValidator( diff --git a/hc/front/templatetags/hc_extras.py b/hc/front/templatetags/hc_extras.py index 2d20ae34..bff76b5d 100644 --- a/hc/front/templatetags/hc_extras.py +++ b/hc/front/templatetags/hc_extras.py @@ -123,3 +123,8 @@ def fix_asterisks(s): """ Prepend asterisks with "Combining Grapheme Joiner" characters. """ return s.replace("*", "\u034f*") + + +@register.filter +def format_headers(headers): + return "\n".join("%s: %s" % (k, v) for k, v in headers.items()) diff --git a/hc/front/tests/test_add_webhook.py b/hc/front/tests/test_add_webhook.py index cbe26d15..3e83d8e5 100644 --- a/hc/front/tests/test_add_webhook.py +++ b/hc/front/tests/test_add_webhook.py @@ -8,24 +8,32 @@ class AddWebhookTestCase(BaseTestCase): def test_instructions_work(self): self.client.login(username="alice@example.org", password="password") r = self.client.get(self.url) - self.assertContains(r, "Runs a HTTP GET or HTTP POST") + self.assertContains(r, "Executes an HTTP request") def test_it_adds_two_webhook_urls_and_redirects(self): - form = {"url_down": "http://foo.com", "url_up": "https://bar.com"} + form = { + "method_down": "GET", + "url_down": "http://foo.com", + "method_up": "GET", + "url_up": "https://bar.com", + } self.client.login(username="alice@example.org", password="password") r = self.client.post(self.url, form) self.assertRedirects(r, "/integrations/") c = Channel.objects.get() - self.assertEqual( - c.value, - '{"headers": {}, "post_data": "", "url_down": "http://foo.com", "url_up": "https://bar.com"}', - ) self.assertEqual(c.project, self.project) + self.assertEqual(c.down_webhook_spec["url"], "http://foo.com") + self.assertEqual(c.up_webhook_spec["url"], "https://bar.com") def test_it_adds_webhook_using_team_access(self): - form = {"url_down": "http://foo.com", "url_up": "https://bar.com"} + form = { + "method_down": "GET", + "url_down": "http://foo.com", + "method_up": "GET", + "url_up": "https://bar.com", + } # Logging in as bob, not alice. Bob has team access so this # should work. @@ -34,10 +42,8 @@ class AddWebhookTestCase(BaseTestCase): c = Channel.objects.get() self.assertEqual(c.project, self.project) - self.assertEqual( - c.value, - '{"headers": {}, "post_data": "", "url_down": "http://foo.com", "url_up": "https://bar.com"}', - ) + self.assertEqual(c.down_webhook_spec["url"], "http://foo.com") + self.assertEqual(c.up_webhook_spec["url"], "https://bar.com") def test_it_rejects_bad_urls(self): urls = [ @@ -52,7 +58,12 @@ class AddWebhookTestCase(BaseTestCase): self.client.login(username="alice@example.org", password="password") for url in urls: - form = {"url_down": url, "url_up": ""} + form = { + "method_down": "GET", + "url_down": url, + "method_up": "GET", + "url_up": "", + } r = self.client.post(self.url, form) self.assertContains(r, "Enter a valid URL.", msg_prefix=url) @@ -60,35 +71,41 @@ class AddWebhookTestCase(BaseTestCase): self.assertEqual(Channel.objects.count(), 0) def test_it_handles_empty_down_url(self): - form = {"url_down": "", "url_up": "http://foo.com"} + form = { + "method_down": "GET", + "url_down": "", + "method_up": "GET", + "url_up": "http://foo.com", + } self.client.login(username="alice@example.org", password="password") self.client.post(self.url, form) c = Channel.objects.get() - self.assertEqual( - c.value, - '{"headers": {}, "post_data": "", "url_down": "", "url_up": "http://foo.com"}', - ) + self.assertEqual(c.down_webhook_spec["url"], "") + self.assertEqual(c.up_webhook_spec["url"], "http://foo.com") - def test_it_adds_post_data(self): - form = {"url_down": "http://foo.com", "post_data": "hello"} + def test_it_adds_request_body(self): + form = { + "method_down": "POST", + "url_down": "http://foo.com", + "body_down": "hello", + "method_up": "GET", + } self.client.login(username="alice@example.org", password="password") r = self.client.post(self.url, form) self.assertRedirects(r, "/integrations/") c = Channel.objects.get() - self.assertEqual( - c.value, - '{"headers": {}, "post_data": "hello", "url_down": "http://foo.com", "url_up": ""}', - ) + self.assertEqual(c.down_webhook_spec["body"], "hello") def test_it_adds_headers(self): form = { + "method_down": "GET", "url_down": "http://foo.com", - "header_key[]": ["test", "test2"], - "header_value[]": ["123", "abc"], + "headers_down": "test:123\ntest2:abc", + "method_up": "GET", } self.client.login(username="alice@example.org", password="password") @@ -96,16 +113,34 @@ class AddWebhookTestCase(BaseTestCase): self.assertRedirects(r, "/integrations/") c = Channel.objects.get() - self.assertEqual(c.headers, {"test": "123", "test2": "abc"}) + self.assertEqual( + c.down_webhook_spec["headers"], {"test": "123", "test2": "abc"} + ) - def test_it_rejects_bad_header_names(self): + def test_it_rejects_bad_headers(self): self.client.login(username="alice@example.org", password="password") form = { + "method_down": "GET", "url_down": "http://example.org", - "header_key[]": ["ill:egal"], - "header_value[]": ["123"], + "headers_down": "invalid-headers", + "method_up": "GET", } r = self.client.post(self.url, form) - self.assertContains(r, "Please use valid HTTP header names.") + self.assertContains(r, """invalid-headers""") self.assertEqual(Channel.objects.count(), 0) + + def test_it_strips_headers(self): + form = { + "method_down": "GET", + "url_down": "http://foo.com", + "headers_down": " test : 123 ", + "method_up": "GET", + } + + self.client.login(username="alice@example.org", password="password") + r = self.client.post(self.url, form) + self.assertRedirects(r, "/integrations/") + + c = Channel.objects.get() + self.assertEqual(c.down_webhook_spec["headers"], {"test": "123"}) diff --git a/static/css/add_webhook.css b/static/css/add_webhook.css index 16b00681..c90883cb 100644 --- a/static/css/add_webhook.css +++ b/static/css/add_webhook.css @@ -2,3 +2,39 @@ border-color: #a94442; } +#add-webhook-form div.bootstrap-select { + width: 100px; +} + +.method-url-group { + display: flex; +} + +.method-url-group > div.dropdown button { + border-right: 0; + border-top-right-radius: 0; + border-bottom-right-radius: 0; + +} + +.method-url-group input { + z-index: 1; + border-top-left-radius: 0; + border-bottom-left-radius: 0; +} + +#webhook-variables tr:first-child th, #webhook-variables tr:first-child td { + border-top: 0; +} + +#webhook-variables th { + white-space: nowrap; +} + +.label-down { + color: #d9534f; +} + +.label-up { + color: #5cb85c +} \ No newline at end of file diff --git a/static/js/webhook.js b/static/js/webhook.js index 2e7cc9f2..2a9090f5 100644 --- a/static/js/webhook.js +++ b/static/js/webhook.js @@ -1,25 +1,15 @@ $(function() { - function haveBlankHeaderForm() { - return $("#webhook-headers .webhook-header").filter(function() { - var key = $(".key", this).val(); - var value = $(".value", this).val(); - return !key && !value; - }).length; - } + $("#method-down").change(function() { + var method = this.value; + $("#body-down-group").toggle(method != "GET"); + }); - function ensureBlankHeaderForm() { - if (!haveBlankHeaderForm()) { - var tmpl = $("#header-template").html(); - $("#webhook-headers").append(tmpl); - } - } + $("#method-up").change(function() { + var method = this.value; + $("#body-up-group").toggle(method != "GET"); + }); - $("#webhook-headers").on("click", "button", function(e) { - e.preventDefault(); - $(this).closest(".webhook-header").remove(); - ensureBlankHeaderForm(); - }) - - $("#webhook-headers").on("keyup", "input", ensureBlankHeaderForm); - ensureBlankHeaderForm(); + // On page load, check if we need to show "request body" fields + $("#method-down").trigger("change"); + $("#method-up").trigger("change"); }); \ No newline at end of file diff --git a/templates/front/channels.html b/templates/front/channels.html index 74e6fbee..c60585df 100644 --- a/templates/front/channels.html +++ b/templates/front/channels.html @@ -398,30 +398,38 @@ {% if ch.kind == "webhook" %} + {% with ch.down_webhook_spec as spec %} + {% if spec.url %} +

Execute on "down" events:

+
{{ spec.method }} {{ spec.url }}
+ {% if spec.body %} +

Request Body

+
{{ spec.body }}
+ {% endif %} - {% if ch.url_down %} -

URL for "down" events

-
{{ ch.url_down }}
- {% endif %} - - {% if ch.url_up %} -

URL for "up" events

-
{{ ch.url_up }}
- {% endif %} - - {% if ch.post_data %} -

POST data

-
{{ ch.post_data }}
- {% endif %} - + {% if spec.headers %} +

Request Headers

+
{{ spec.headers|format_headers }}
+ {% endif %} + {% endif %} + {% endwith %} + + {% with ch.up_webhook_spec as spec %} + {% if spec.url %} +

Execute on "up" events:

+
{{ spec.method }} {{ spec.url }}
+ {% if spec.body %} +

Request Body

+
{{ spec.body }}
+ {% endif %} - {% for k, v in ch.headers.items %} -

Header {{ k }}

-
{{ v }}
- {% endfor %} + {% if spec.headers %} +

Request Headers

+
{{ spec.headers|format_headers }}
+ {% endif %} + {% endif %} + {% endwith %} {% endif %} - -