From 7c145c3f684bea340955f15501aa6cfee97ae446 Mon Sep 17 00:00:00 2001 From: "Hemanth V. Alluri" Date: Wed, 5 Jun 2019 18:42:34 +0530 Subject: [PATCH] webhooks: Add a system for storing http headers for integrations. Using this system, we can now associate any fixture of any integration with a particular set of HTTP headers. A helper method called determine_http_headers was introduced, and the test suite was upgraded to use determine_http_headers. Comments and documentation significantly edited by tabbott. --- .../api/incoming-webhooks-walkthrough.md | 18 +++++++ zerver/lib/test_classes.py | 8 ++- zerver/lib/webhooks/common.py | 49 ++++++++++++++++++- zerver/tests/test_webhooks_common.py | 41 +++++++++++++++- 4 files changed, 113 insertions(+), 3 deletions(-) diff --git a/templates/zerver/api/incoming-webhooks-walkthrough.md b/templates/zerver/api/incoming-webhooks-walkthrough.md index dc55a425bf..779f9d3c5e 100644 --- a/templates/zerver/api/incoming-webhooks-walkthrough.md +++ b/templates/zerver/api/incoming-webhooks-walkthrough.md @@ -43,6 +43,24 @@ the 3rd party service sends, your fixture may contain JSON, URL encoded text, or some other kind of data. See [Step 5: Create automated tests](#step-5-create-automated-tests) or [Testing](https://zulip.readthedocs.io/en/latest/testing/testing.html) for further details. +### HTTP Headers + +Some third-party webhook APIs, such as GitHub's, don't encode all the +information about an event in the JSON request body. Instead, they +put key details like the event type in a separate HTTP header +(generally this is clear in their API documentation). In order to +test Zulip's handling of that integration, you will need to record +which HTTP headers are used with each fixture you capture. + +Since this integration-dependent, Zulip offers a simple API for doing +this, which is probably best explained by looking at the example for +GitHub: `zerver/webhooks/github/headers.py`; basically, as part of +writing your integration, you'll write a special function at that path +that maps the filename of the fixture to the set of HTTP headers to +use. Most such functions will use the same strategy as the GitHub +integration: encoding the third party variable header data (usually +just an event type) in the fixture filename. + ## Step 1: Initialize your webhook python package In the `zerver/webhooks/` directory, create new subdirectory that will diff --git a/zerver/lib/test_classes.py b/zerver/lib/test_classes.py index d8194c4b65..26067c7bf6 100644 --- a/zerver/lib/test_classes.py +++ b/zerver/lib/test_classes.py @@ -23,6 +23,7 @@ from zerver.lib.initial_password import initial_password from zerver.lib.utils import is_remote_server from zerver.lib.users import get_api_key from zerver.lib.sessions import get_session_dict_user +from zerver.lib.webhooks.common import get_fixture_http_headers, parse_headers_dict from zerver.lib.actions import ( check_send_message, create_stream_if_needed, bulk_add_subscriptions, @@ -751,6 +752,9 @@ class WebhookTestCase(ZulipTestCase): payload = self.get_body(fixture_name) if content_type is not None: kwargs['content_type'] = content_type + headers = get_fixture_http_headers(self.FIXTURE_DIR_NAME, fixture_name) + headers = parse_headers_dict(headers) + kwargs.update(headers) msg = self.send_json_payload(self.test_user, self.url, payload, self.STREAM_NAME, **kwargs) self.do_test_topic(msg, expected_topic) @@ -764,7 +768,9 @@ class WebhookTestCase(ZulipTestCase): payload = self.get_body(fixture_name) if content_type is not None: kwargs['content_type'] = content_type - + headers = get_fixture_http_headers(self.FIXTURE_DIR_NAME, fixture_name) + headers = parse_headers_dict(headers) + kwargs.update(headers) sender = kwargs.get('sender', self.test_user) msg = self.send_json_payload(sender, self.url, payload, stream_name=None, **kwargs) diff --git a/zerver/lib/webhooks/common.py b/zerver/lib/webhooks/common.py index 7926f8ee13..e4bd016f4d 100644 --- a/zerver/lib/webhooks/common.py +++ b/zerver/lib/webhooks/common.py @@ -1,8 +1,9 @@ +import importlib from urllib.parse import unquote from django.http import HttpRequest from django.utils.translation import ugettext as _ -from typing import Optional +from typing import Optional, Dict, Union, Any from zerver.lib.actions import check_send_stream_message, \ check_send_private_message, send_rate_limited_pm_notification_to_bot_owner @@ -96,6 +97,22 @@ def check_send_webhook_message( # webhook-errors.log pass +def parse_headers_dict(custom_headers: Union[None, Dict[str, Any]]) -> Dict[str, str]: + headers = {} + if not custom_headers: + return {} + for header in custom_headers: + # Ensure what we return is in the Django HTTP headers format, + # with HTTP_ prefixes before most actual HTTP header names. + if header.startswith("HTTP_") or header.startswith("http_"): + headers[header.upper().replace("-", "_")] = str(custom_headers[header]) + else: + new_header = header.upper().replace("-", "_") + if new_header not in ["CONTENT_TYPE", "CONTENT_LENGTH"]: + new_header = "HTTP_" + new_header + headers[new_header] = str(custom_headers[header]) + return headers + def validate_extract_webhook_http_header(request: HttpRequest, header: str, integration_name: str, fatal: Optional[bool]=True) -> Optional[str]: @@ -114,3 +131,33 @@ def validate_extract_webhook_http_header(request: HttpRequest, header: str, raise MissingHTTPEventHeader(header) return extracted_header + + +def get_fixture_http_headers(integration_name: str, + fixture_name: str) -> Dict["str", "str"]: + """For integrations that require custom HTTP headers for some (or all) + of their test fixtures, this method will call a specially named + function from the target integration module to determine what set + of HTTP headers goes with the given test fixture. + """ + + headers_module_name = "zerver.webhooks.{integration_name}.headers".format( + integration_name=integration_name) + try: + # Try to import the headers.py file. If it does not exist, + # then that means that the integration does not have any + # special http headers registered. + # + # TODO: We may want to migrate to a more explicit registration + # strategy for this behavior rather than a try/except import. + headers_module = importlib.import_module(headers_module_name) + except ImportError: + return {} + try: + fixture_to_headers = headers_module.fixture_to_headers # type: ignore # we do extra exception handling in case it does not exist below. + except AttributeError as e: + msg = ". The {module_name} module must contain a fixture_to_headers method.".format( + module_name=headers_module_name + ) + raise AttributeError(str(e) + msg) + return fixture_to_headers(fixture_name) diff --git a/zerver/tests/test_webhooks_common.py b/zerver/tests/test_webhooks_common.py index 8af83cb005..0c5f5579b7 100644 --- a/zerver/tests/test_webhooks_common.py +++ b/zerver/tests/test_webhooks_common.py @@ -1,4 +1,8 @@ # -*- coding: utf-8 -*- +from types import SimpleNamespace +from mock import MagicMock, patch +from typing import Dict + from django.http import HttpRequest from zerver.decorator import api_key_only_webhook_view @@ -7,7 +11,7 @@ from zerver.lib.test_classes import ZulipTestCase, WebhookTestCase from zerver.lib.webhooks.common import \ validate_extract_webhook_http_header, \ MISSING_EVENT_HEADER_MESSAGE, MissingHTTPEventHeader, \ - INVALID_JSON_MESSAGE + INVALID_JSON_MESSAGE, get_fixture_http_headers, parse_headers_dict from zerver.models import get_user, get_realm, UserProfile from zerver.lib.users import get_api_key from zerver.lib.send_email import FromAddress @@ -85,6 +89,41 @@ class WebhooksCommonTestCase(ZulipTestCase): self.assertEqual(msg.sender.email, self.notification_bot().email) self.assertEqual(msg.content, expected_msg.strip()) + @patch("zerver.lib.webhooks.common.importlib.import_module") + def test_get_fixture_http_headers_for_success(self, import_module_mock: MagicMock) -> None: + def fixture_to_headers(fixture_name: str) -> Dict[str, str]: + # A sample function which would normally perform some + # extra operations before returning a dictionary + # corresponding to the fixture name passed. For this test, + # we just return a fixed dictionary. + return {"key": "value"} + + fake_module = SimpleNamespace(fixture_to_headers=fixture_to_headers) + import_module_mock.return_value = fake_module + + headers = get_fixture_http_headers("some_integration", "complex_fixture") + self.assertEqual(headers, {"key": "value"}) + + def test_get_fixture_http_headers_for_non_existant_integration(self) -> None: + headers = get_fixture_http_headers("some_random_nonexistant_integration", "fixture_name") + self.assertEqual(headers, {}) + + @patch("zerver.lib.webhooks.common.importlib.import_module") + def test_get_fixture_http_headers_for_error_handling(self, import_module_mock: MagicMock) -> None: + fake_module = SimpleNamespace() + import_module_mock.return_value = fake_module + + with self.assertRaises(AttributeError): + get_fixture_http_headers("some_integration", "simple_fixture") + + def test_parse_headers_dict(self) -> None: + self.assertEqual(parse_headers_dict({}), {}) + + raw_headers = {"Content-Type": "text/plain", "X-Event-Type": "ping"} + djangoified_headers = parse_headers_dict(raw_headers) + expected_djangoified_headers = {"CONTENT_TYPE": "text/plain", "HTTP_X_EVENT_TYPE": "ping"} + self.assertEqual(djangoified_headers, expected_djangoified_headers) + class MissingEventHeaderTestCase(WebhookTestCase): STREAM_NAME = 'groove' URL_TEMPLATE = '/api/v1/external/groove?stream={stream}&api_key={api_key}'