From b7b1ec0aebbf75c2f66a4c81a42bf4da0ac7c9e1 Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Sun, 25 Apr 2021 21:23:52 +0200 Subject: [PATCH] outgoing_webhook: Improve invalid json handling when parsing response. It's better to just raise JsonableError here, as that makes this error processed in the central place for this kind of thing in do_rest_call: --------- except JsonableError as e: response_message = e.msg logging.info("Outhook trigger failed:", stack_info=True) fail_with_message(event, response_message) response_message = f"The outgoing webhook server attempted to send a message in Zulip, but that request resulted in the following error:\n> {e}" notify_bot_owner(event, failure_message=response_message) return None ---------- which does all the things that are supposed to happen - fail_with_message, appropriate logging and notifying the bot owner. --- zerver/lib/outgoing_webhook.py | 3 +- .../tests/test_outgoing_webhook_interfaces.py | 4 +-- zerver/tests/test_outgoing_webhook_system.py | 29 +++++++++++++++++++ 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/zerver/lib/outgoing_webhook.py b/zerver/lib/outgoing_webhook.py index 7fde13c373..45ae7ebe75 100644 --- a/zerver/lib/outgoing_webhook.py +++ b/zerver/lib/outgoing_webhook.py @@ -288,8 +288,7 @@ def process_success_response( try: response_json = json.loads(response.text) except json.JSONDecodeError: - fail_with_message(event, "Invalid JSON in response") - return + raise JsonableError(_("Invalid JSON in response")) if not isinstance(response_json, dict): raise JsonableError(_("Invalid response format")) diff --git a/zerver/tests/test_outgoing_webhook_interfaces.py b/zerver/tests/test_outgoing_webhook_interfaces.py index a7780d5e15..e0ef231af6 100644 --- a/zerver/tests/test_outgoing_webhook_interfaces.py +++ b/zerver/tests/test_outgoing_webhook_interfaces.py @@ -5,6 +5,7 @@ from unittest import mock import requests from zerver.lib.avatar import get_gravatar_url +from zerver.lib.exceptions import JsonableError from zerver.lib.message import MessageDict from zerver.lib.outgoing_webhook import get_service_interface_class, process_success_response from zerver.lib.test_classes import ZulipTestCase @@ -47,13 +48,12 @@ class TestGenericOutgoingWebhookService(ZulipTestCase): response.status_code = 200 response.text = "unparsable text" - with mock.patch("zerver.lib.outgoing_webhook.fail_with_message") as m: + with self.assertRaisesRegex(JsonableError, "Invalid JSON in response"): process_success_response( event=event, service_handler=service_handler, response=response, ) - self.assertTrue(m.called) def test_make_request(self) -> None: othello = self.example_user("othello") diff --git a/zerver/tests/test_outgoing_webhook_system.py b/zerver/tests/test_outgoing_webhook_system.py index e31abab986..85b922cf27 100644 --- a/zerver/tests/test_outgoing_webhook_system.py +++ b/zerver/tests/test_outgoing_webhook_system.py @@ -288,6 +288,35 @@ The outgoing webhook server attempted to send a message in Zulip, but that reque assert bot_user.bot_owner is not None self.assertEqual(bot_owner_notification.recipient_id, bot_user.bot_owner.recipient_id) + def test_invalid_json_in_response(self) -> None: + bot_user = self.example_user("outgoing_webhook_bot") + mock_event = self.mock_event(bot_user) + service_handler = GenericOutgoingWebhookService("token", bot_user, "service") + + expect_logging_info = self.assertLogs(level="INFO") + expect_fail = mock.patch("zerver.lib.outgoing_webhook.fail_with_message") + + with responses.RequestsMock(assert_all_requests_are_fired=True) as requests_mock: + # We mock the endpoint to return response with a body which isn't valid json. + requests_mock.add( + requests_mock.POST, + "https://example.zulip.com", + status=200, + body="this isn't valid json", + ) + with expect_logging_info, expect_fail as mock_fail: + do_rest_call("https://example.zulip.com", mock_event, service_handler) + self.assertTrue(mock_fail.called) + bot_owner_notification = self.get_last_message() + self.assertEqual( + bot_owner_notification.content, + """[A message](http://zulip.testserver/#narrow/stream/999-Verona/topic/Foo/near/) to your bot @_**Outgoing Webhook** triggered an outgoing webhook. +The outgoing webhook server attempted to send a message in Zulip, but that request resulted in the following error: +> Invalid JSON in response""", + ) + assert bot_user.bot_owner is not None + self.assertEqual(bot_owner_notification.recipient_id, bot_user.bot_owner.recipient_id) + class TestOutgoingWebhookMessaging(ZulipTestCase): def create_outgoing_bot(self, bot_owner: UserProfile) -> UserProfile: