diff --git a/zerver/lib/outgoing_webhook.py b/zerver/lib/outgoing_webhook.py index 107e0f8724..7e641b82e2 100644 --- a/zerver/lib/outgoing_webhook.py +++ b/zerver/lib/outgoing_webhook.py @@ -32,11 +32,7 @@ class OutgoingWebhookServiceInterface(metaclass=abc.ABCMeta): self.service_name: str = service_name @abc.abstractmethod - def build_bot_request(self, event: Dict[str, Any]) -> Optional[Any]: - raise NotImplementedError - - @abc.abstractmethod - def send_data_to_server(self, base_url: str, request_data: Any) -> Response: + def make_request(self, base_url: str, event: Dict[str, Any]) -> Optional[Response]: raise NotImplementedError @abc.abstractmethod @@ -45,7 +41,7 @@ class OutgoingWebhookServiceInterface(metaclass=abc.ABCMeta): class GenericOutgoingWebhookService(OutgoingWebhookServiceInterface): - def build_bot_request(self, event: Dict[str, Any]) -> Optional[Any]: + def make_request(self, base_url: str, event: Dict[str, Any]) -> Optional[Response]: """ We send a simple version of the message to outgoing webhooks, since most of them really only need @@ -69,9 +65,7 @@ class GenericOutgoingWebhookService(OutgoingWebhookServiceInterface): "token": self.token, "trigger": event["trigger"], } - return json.dumps(request_data) - def send_data_to_server(self, base_url: str, request_data: Any) -> Response: user_agent = "ZulipOutgoingWebhook/" + ZULIP_VERSION headers = { "User-Agent": user_agent, @@ -99,7 +93,7 @@ class GenericOutgoingWebhookService(OutgoingWebhookServiceInterface): class SlackOutgoingWebhookService(OutgoingWebhookServiceInterface): - def build_bot_request(self, event: Dict[str, Any]) -> Optional[Any]: + def make_request(self, base_url: str, event: Dict[str, Any]) -> Optional[Response]: if event["message"]["type"] == "private": failure_message = "Slack outgoing webhooks don't support private messages." fail_with_message(event, failure_message) @@ -118,12 +112,7 @@ class SlackOutgoingWebhookService(OutgoingWebhookServiceInterface): ("trigger_word", event["trigger"]), ("service_id", event["user_profile_id"]), ] - - return request_data - - def send_data_to_server(self, base_url: str, request_data: Any) -> Response: - response = requests.request("POST", base_url, data=request_data) - return response + return requests.request("POST", base_url, data=request_data) def process_success(self, response_json: Dict[str, Any]) -> Optional[Dict[str, Any]]: if "text" in response_json: @@ -311,14 +300,16 @@ def process_success_response( def do_rest_call( - base_url: str, request_data: Any, event: Dict[str, Any], service_handler: Any + base_url: str, event: Dict[str, Any], service_handler: Any ) -> Optional[Response]: """Returns response of call if no exception occurs.""" try: - response = service_handler.send_data_to_server( - base_url=base_url, - request_data=request_data, + response = service_handler.make_request( + base_url, + event, ) + if not response: + return None if str(response.status_code).startswith("2"): process_success_response(event, service_handler, response) else: diff --git a/zerver/tests/test_outgoing_webhook_interfaces.py b/zerver/tests/test_outgoing_webhook_interfaces.py index 5b2ec7bbd8..f7afd788a8 100644 --- a/zerver/tests/test_outgoing_webhook_interfaces.py +++ b/zerver/tests/test_outgoing_webhook_interfaces.py @@ -55,7 +55,7 @@ class TestGenericOutgoingWebhookService(ZulipTestCase): ) self.assertTrue(m.called) - def test_build_bot_request(self) -> None: + def test_make_request(self) -> None: othello = self.example_user("othello") stream = get_stream("Denmark", othello.realm) message_id = self.send_stream_message( @@ -102,8 +102,25 @@ class TestGenericOutgoingWebhookService(ZulipTestCase): "trigger": "mention", } - request_data = self.handler.build_bot_request(event) - request_data = json.loads(request_data) + test_url = "https://example.com/example" + response = mock.Mock(spec=requests.Response) + response.status_code = 200 + expect_200 = mock.patch("requests.request", return_value=response) + with expect_200 as mock_request: + self.handler.make_request( + test_url, + event, + ) + mock_request.assert_called_once() + self.assertEqual( + mock_request.call_args[0], + ( + "POST", + test_url, + ), + ) + request_data = mock_request.call_args[1]["json"] + validate_against_openapi_schema(request_data, "/zulip-outgoing-webhook", "post", "200") self.assertEqual(request_data["data"], "@**test**") self.assertEqual(request_data["token"], "abcdef") @@ -180,8 +197,22 @@ class TestSlackOutgoingWebhookService(ZulipTestCase): service_class = get_service_interface_class(SLACK_INTERFACE) self.handler = service_class(token="abcdef", user_profile=None, service_name="test-service") - def test_build_bot_request_stream_message(self) -> None: - request_data = self.handler.build_bot_request(self.stream_message_event) + def test_make_request_stream_message(self) -> None: + test_url = "https://example.com/example" + with mock.patch("requests.request") as mock_request: + self.handler.make_request( + test_url, + self.stream_message_event, + ) + mock_request.assert_called_once() + self.assertEqual( + mock_request.call_args[0], + ( + "POST", + test_url, + ), + ) + request_data = mock_request.call_args[1]["data"] self.assertEqual(request_data[0][1], "abcdef") # token self.assertEqual(request_data[1][1], "zulip") # team_id @@ -196,10 +227,15 @@ class TestSlackOutgoingWebhookService(ZulipTestCase): self.assertEqual(request_data[10][1], 12) # user_profile_id @mock.patch("zerver.lib.outgoing_webhook.fail_with_message") - def test_build_bot_request_private_message(self, mock_fail_with_message: mock.Mock) -> None: - - request_data = self.handler.build_bot_request(self.private_message_event) - self.assertIsNone(request_data) + def test_make_request_private_message(self, mock_fail_with_message: mock.Mock) -> None: + test_url = "https://example.com/example" + with mock.patch("requests.request") as mock_request: + response = self.handler.make_request( + test_url, + self.private_message_event, + ) + mock_request.assert_not_called() + self.assertIsNone(response) self.assertTrue(mock_fail_with_message.called) def test_process_success(self) -> None: diff --git a/zerver/tests/test_outgoing_webhook_system.py b/zerver/tests/test_outgoing_webhook_system.py index 2355efd9fa..c15db390e6 100644 --- a/zerver/tests/test_outgoing_webhook_system.py +++ b/zerver/tests/test_outgoing_webhook_system.py @@ -47,10 +47,23 @@ class DoRestCallTests(ZulipTestCase): "message": { "display_recipient": "Verona", "stream_id": 999, + "sender_id": bot_user.id, + "sender_email": bot_user.email, + "sender_realm_id": bot_user.realm.id, + "sender_realm_str": bot_user.realm.string_id, + "sender_delivery_email": bot_user.delivery_email, + "sender_full_name": bot_user.full_name, + "sender_avatar_source": UserProfile.AVATAR_FROM_GRAVATAR, + "sender_avatar_version": 1, + "recipient_type": "stream", + "recipient_type_id": 999, + "sender_is_mirror_dummy": False, TOPIC_NAME: "Foo", "id": "", "type": "stream", + "timestamp": 1, }, + "trigger": "mention", "user_profile_id": bot_user.id, "command": "", "service_name": "", @@ -66,14 +79,14 @@ class DoRestCallTests(ZulipTestCase): expect_send_response = mock.patch("zerver.lib.outgoing_webhook.send_response_message") with expect_200, expect_send_response as mock_send: - do_rest_call("", None, mock_event, service_handler) + do_rest_call("", mock_event, service_handler) self.assertTrue(mock_send.called) for service_class in [GenericOutgoingWebhookService, SlackOutgoingWebhookService]: handler = service_class("token", bot_user, "service") with expect_200: - do_rest_call("", None, mock_event, handler) + do_rest_call("", mock_event, handler) # TODO: assert something interesting here? @@ -86,7 +99,7 @@ class DoRestCallTests(ZulipTestCase): with mock.patch("requests.request", return_value=response), self.assertLogs( level="WARNING" ) as m: - final_response = do_rest_call("", None, mock_event, service_handler) + final_response = do_rest_call("", mock_event, service_handler) assert final_response is not None self.assertEqual( @@ -115,7 +128,7 @@ The webhook got a response with status code *500*.""", expect_fail = mock.patch("zerver.lib.outgoing_webhook.fail_with_message") with expect_400, expect_fail as mock_fail, self.assertLogs(level="WARNING") as m: - final_response = do_rest_call("", None, mock_event, service_handler) + final_response = do_rest_call("", mock_event, service_handler) assert final_response is not None self.assertEqual( @@ -144,9 +157,7 @@ The webhook got a response with status code *400*.""", with mock.patch("requests.sessions.Session.send") as mock_send: mock_send.return_value = ResponseMock(200) - final_response = do_rest_call( - "https://example.com/", "payload-stub", mock_event, service_handler - ) + final_response = do_rest_call("https://example.com/", mock_event, service_handler) assert final_response is not None mock_send.assert_called_once() @@ -167,7 +178,7 @@ The webhook got a response with status code *400*.""", def helper(side_effect: Any, error_text: str) -> None: with mock.patch("requests.request", side_effect=side_effect): - do_rest_call("", None, mock_event, service_handler) + do_rest_call("", mock_event, service_handler) bot_owner_notification = self.get_last_message() self.assertIn(error_text, bot_owner_notification.content) @@ -202,7 +213,7 @@ The webhook got a response with status code *400*.""", # Don't think that we should catch and assert whole log output(which is actually a very big error traceback). # We are already asserting bot_owner_notification.content which verifies exception did occur. with expect_request_exception, expect_logging_exception, expect_fail as mock_fail: - do_rest_call("", None, mock_event, service_handler) + do_rest_call("", mock_event, service_handler) self.assertTrue(mock_fail.called) @@ -260,7 +271,8 @@ class TestOutgoingWebhookMessaging(ZulipTestCase): sender = self.example_user("hamlet") - with mock.patch("zerver.worker.queue_processors.do_rest_call") as m: + with mock.patch("requests.request") as mock_request: + mock_request.return_value = ResponseMock(200) self.send_personal_message( sender, bot, @@ -268,10 +280,11 @@ class TestOutgoingWebhookMessaging(ZulipTestCase): ) url_token_tups = set() - for item in m.call_args_list: + for item in mock_request.call_args_list: args = item[0] - base_url = args[0] - request_data = orjson.loads(args[1]) + base_url = args[1] + kwargs = item[1] + request_data = kwargs["json"] tup = (base_url, request_data["token"]) url_token_tups.add(tup) message_data = request_data["message"] diff --git a/zerver/worker/queue_processors.py b/zerver/worker/queue_processors.py index 078831901f..2aa5b0f90a 100644 --- a/zerver/worker/queue_processors.py +++ b/zerver/worker/queue_processors.py @@ -742,9 +742,7 @@ class OutgoingWebhookWorker(QueueProcessingWorker): for service in services: event["service_name"] = str(service.name) service_handler = get_outgoing_webhook_service_handler(service) - request_data = service_handler.build_bot_request(event) - if request_data: - do_rest_call(service.base_url, request_data, event, service_handler) + do_rest_call(service.base_url, event, service_handler) @assign_queue("embedded_bots")