From bd9a1dc9710293e36d2d47d970d7afb95100c2e6 Mon Sep 17 00:00:00 2001 From: Anders Kaseorg Date: Mon, 12 Sep 2022 23:39:44 -0700 Subject: [PATCH] =?UTF-8?q?tests:=20Consistently=20JSON-encode=20=E2=80=98?= =?UTF-8?q?to=E2=80=99=20parameter?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Although our POST /messages handler accepts the ‘to’ parameter with or without JSON encoding, there are two problems with passing it as an unencoded string. Firstly, you’d fail to send a message to a stream named ‘true’ or ‘false’ or ‘null’ or ‘2022’, as the JSON interpretation is prioritized over the plain string interpretation. Secondly, and more importantly for our tests, it violates our OpenAPI schema, which requires the parameter to be JSON-encoded. This is because OpenAPI has no concept of a parameter that’s “optionally JSON-encoded”, nor should it: such a parameter cannot be unambiguously decoded for the reason above. Our version of openapi-core doesn’t currently detect this schema violation, but after the next upgrade it will. Signed-off-by: Anders Kaseorg --- templates/zerver/api/send-message.md | 2 +- zerver/tests/test_decorators.py | 2 +- zerver/tests/test_email_mirror.py | 12 +++---- zerver/tests/test_email_notifications.py | 2 +- zerver/tests/test_external.py | 2 +- zerver/tests/test_legacy_subject.py | 4 ++- zerver/tests/test_message_send.py | 46 ++++++++++++------------ zerver/tests/test_reactions.py | 30 +++++++++++++--- zerver/tests/test_user_groups.py | 2 +- zerver/tests/test_widgets.py | 16 ++++----- 10 files changed, 71 insertions(+), 47 deletions(-) diff --git a/templates/zerver/api/send-message.md b/templates/zerver/api/send-message.md index 89c4a84d9d..0c3b90bc47 100644 --- a/templates/zerver/api/send-message.md +++ b/templates/zerver/api/send-message.md @@ -15,7 +15,7 @@ curl -X POST {{ api_url }}/v1/messages \ -u BOT_EMAIL_ADDRESS:BOT_API_KEY \ --data-urlencode type=stream \ - --data-urlencode to=Denmark \ + --data-urlencode 'to="Denmark"' \ --data-urlencode topic=Castle \ --data-urlencode 'content=I come not, friends, to steal away your hearts.' diff --git a/zerver/tests/test_decorators.py b/zerver/tests/test_decorators.py index 0f7b8d29f3..1616353562 100644 --- a/zerver/tests/test_decorators.py +++ b/zerver/tests/test_decorators.py @@ -1401,7 +1401,7 @@ class TestIncomingWebhookBot(ZulipTestCase): payload = dict( type="private", content="Test message", - to=othello.email, + to=orjson.dumps([othello.email]).decode(), ) result = self.api_post(webhook_bot, "/api/v1/messages", payload) diff --git a/zerver/tests/test_email_mirror.py b/zerver/tests/test_email_mirror.py index b252ffed79..72d94c16ec 100644 --- a/zerver/tests/test_email_mirror.py +++ b/zerver/tests/test_email_mirror.py @@ -1028,7 +1028,7 @@ class TestMissedMessageEmailMessages(ZulipTestCase): "type": "stream", "topic": "test topic", "content": "test_receive_missed_stream_message_email_messages", - "to": "Denmark", + "to": orjson.dumps("Denmark").decode(), }, ) self.assert_json_success(result) @@ -1067,7 +1067,7 @@ class TestMissedMessageEmailMessages(ZulipTestCase): "type": "stream", "topic": "test topic", "content": "test_receive_email_response_for_auth_failures", - "to": "announce", + "to": orjson.dumps("announce").decode(), }, ) self.assert_json_success(result) @@ -1111,7 +1111,7 @@ class TestMissedMessageEmailMessages(ZulipTestCase): "type": "stream", "topic": "test topic", "content": "test_receive_missed_stream_message_email_messages", - "to": "Denmark", + "to": orjson.dumps("Denmark").decode(), }, ) self.assert_json_success(result) @@ -1156,7 +1156,7 @@ class TestMissedMessageEmailMessages(ZulipTestCase): "type": "stream", "topic": "test topic", "content": "test_receive_missed_stream_message_email_messages", - "to": "Denmark", + "to": orjson.dumps("Denmark").decode(), }, ) self.assert_json_success(result) @@ -1192,7 +1192,7 @@ class TestMissedMessageEmailMessages(ZulipTestCase): "type": "stream", "topic": "test topic", "content": "test_receive_missed_stream_message_email_messages", - "to": "Denmark", + "to": orjson.dumps("Denmark").decode(), }, ) self.assert_json_success(result) @@ -1229,7 +1229,7 @@ class TestMissedMessageEmailMessages(ZulipTestCase): "type": "stream", "topic": "test topic", "content": "test_receive_missed_stream_message_email_messages", - "to": "Denmark", + "to": orjson.dumps("Denmark").decode(), }, ) self.assert_json_success(result) diff --git a/zerver/tests/test_email_notifications.py b/zerver/tests/test_email_notifications.py index ee8673bcd5..72b434447d 100644 --- a/zerver/tests/test_email_notifications.py +++ b/zerver/tests/test_email_notifications.py @@ -385,7 +385,7 @@ class TestMissedMessages(ZulipTestCase): { "type": "private", "content": "Test message", - "to": hamlet.email, + "to": orjson.dumps([hamlet.email]).decode(), }, ) self.assert_json_success(result) diff --git a/zerver/tests/test_external.py b/zerver/tests/test_external.py index d32ede0630..2b4bbe2c15 100644 --- a/zerver/tests/test_external.py +++ b/zerver/tests/test_external.py @@ -117,7 +117,7 @@ class RateLimitTests(ZulipTestCase): "/api/v1/messages", { "type": "stream", - "to": "Verona", + "to": orjson.dumps("Verona").decode(), "content": content, "topic": "whatever", }, diff --git a/zerver/tests/test_legacy_subject.py b/zerver/tests/test_legacy_subject.py index e6b45c1b72..81988dee71 100644 --- a/zerver/tests/test_legacy_subject.py +++ b/zerver/tests/test_legacy_subject.py @@ -1,3 +1,5 @@ +import orjson + from zerver.lib.test_classes import ZulipTestCase @@ -7,7 +9,7 @@ class LegacySubjectTest(ZulipTestCase): payload = dict( type="stream", - to="Verona", + to=orjson.dumps("Verona").decode(), content="Test message", ) diff --git a/zerver/tests/test_message_send.py b/zerver/tests/test_message_send.py index b26d7374a9..6e93cd7203 100644 --- a/zerver/tests/test_message_send.py +++ b/zerver/tests/test_message_send.py @@ -1,7 +1,7 @@ import datetime import sys from email.headerregistry import Address -from typing import TYPE_CHECKING, Any, List, Mapping, Optional, Set +from typing import TYPE_CHECKING, Any, List, Mapping, Optional, Set, Union from unittest import mock import orjson @@ -97,7 +97,7 @@ class MessagePOSTTest(ZulipTestCase): "/json/messages", { "type": "stream", - "to": "Verona", + "to": orjson.dumps("Verona").decode(), "content": "Test message", "topic": "Test topic", }, @@ -114,7 +114,7 @@ class MessagePOSTTest(ZulipTestCase): "/api/v1/messages", { "type": "stream", - "to": "Verona", + "to": orjson.dumps("Verona").decode(), "content": "Test message", "topic": "Test topic", }, @@ -519,7 +519,7 @@ class MessagePOSTTest(ZulipTestCase): { "type": "private", "content": "Test message", - "to": othello.email, + "to": orjson.dumps([othello.email]).decode(), }, ) self.assert_json_success(result) @@ -538,7 +538,7 @@ class MessagePOSTTest(ZulipTestCase): { "type": "private", "content": "Test message", - "to": user_profile.email, + "to": orjson.dumps([user_profile.email]).decode(), }, ) self.assert_json_success(result) @@ -814,7 +814,7 @@ class MessagePOSTTest(ZulipTestCase): "sender": self.mit_email("sipbtest"), "content": "Test message", "client": "zephyr_mirror", - "to": self.mit_email("starnine"), + "to": orjson.dumps([self.mit_email("starnine")]).decode(), }, subdomain="zephyr", ) @@ -912,7 +912,7 @@ class MessagePOSTTest(ZulipTestCase): self.login("hamlet") post_data = { "type": "stream", - "to": "Verona", + "to": orjson.dumps("Verona").decode(), "content": " I like whitespace at the end! \n\n \n", "topic": "Test topic", } @@ -924,7 +924,7 @@ class MessagePOSTTest(ZulipTestCase): # Test if it removes the new line from the beginning of the message. post_data = { "type": "stream", - "to": "Verona", + "to": orjson.dumps("Verona").decode(), "content": "\nAvoid the new line at the beginning of the message.", "topic": "Test topic", } @@ -946,7 +946,7 @@ class MessagePOSTTest(ZulipTestCase): long_message = "A" * (MAX_MESSAGE_LENGTH + 1) post_data = { "type": "stream", - "to": "Verona", + "to": orjson.dumps("Verona").decode(), "content": long_message, "topic": "Test topic", } @@ -967,7 +967,7 @@ class MessagePOSTTest(ZulipTestCase): long_topic = "A" * (MAX_TOPIC_NAME_LENGTH + 1) post_data = { "type": "stream", - "to": "Verona", + "to": orjson.dumps("Verona").decode(), "content": "test content", "topic": long_topic, } @@ -1189,7 +1189,7 @@ class MessagePOSTTest(ZulipTestCase): "content": "Test message", "client": "irc_mirror", "topic": "from irc", - "to": "IRCLand", + "to": orjson.dumps("IRCLand").decode(), }, ) self.assert_json_success(result) @@ -1212,7 +1212,7 @@ class MessagePOSTTest(ZulipTestCase): "content": "Test message", "client": "irc_mirror", "topic": "from irc", - "to": "IRCLand", + "to": orjson.dumps("IRCLand").decode(), }, ) self.assert_json_success(result) @@ -1236,7 +1236,7 @@ class MessagePOSTTest(ZulipTestCase): def test_with(sender_email: str, client: str, forged: bool) -> None: payload = dict( type="stream", - to=stream_name, + to=orjson.dumps(stream_name).decode(), client=client, topic="whatever", content="whatever", @@ -1283,7 +1283,7 @@ class MessagePOSTTest(ZulipTestCase): payload = dict( type="stream", - to=stream_name, + to=orjson.dumps(stream_name).decode(), topic="whatever", content="whatever", ) @@ -1309,7 +1309,7 @@ class MessagePOSTTest(ZulipTestCase): "/api/v1/messages", { "type": "stream", - "to": "notify_channel", + "to": orjson.dumps("notify_channel").decode(), "content": "Test message", "topic": "Test topic", }, @@ -1330,7 +1330,7 @@ class MessagePOSTTest(ZulipTestCase): self.make_stream(stream_name, invite_only=False) payload = dict( type="stream", - to=stream_name, + to=orjson.dumps(stream_name).decode(), topic="whatever", content="whatever", ) @@ -1352,7 +1352,7 @@ class ScheduledMessageTest(ZulipTestCase): def do_schedule_message( self, msg_type: str, - to: str, + to: Union[str, int, List[str], List[int]], msg: str, defer_until: str = "", tz_guess: str = "", @@ -1367,7 +1367,7 @@ class ScheduledMessageTest(ZulipTestCase): payload = { "type": msg_type, - "to": to, + "to": orjson.dumps(to).decode(), "content": msg, "topic": topic_name, "realm_str": realm_str, @@ -1407,7 +1407,9 @@ class ScheduledMessageTest(ZulipTestCase): # Scheduling a private message is successful. othello = self.example_user("othello") hamlet = self.example_user("hamlet") - result = self.do_schedule_message("private", othello.email, content + " 3", defer_until_str) + result = self.do_schedule_message( + "private", [othello.email], content + " 3", defer_until_str + ) message = self.last_scheduled_message() self.assert_json_success(result) self.assertEqual(message.content, "Test message 3") @@ -1416,14 +1418,14 @@ class ScheduledMessageTest(ZulipTestCase): # Setting a reminder in PM's to other users causes a error. result = self.do_schedule_message( - "private", othello.email, content + " 4", defer_until_str, delivery_type="remind" + "private", [othello.email], content + " 4", defer_until_str, delivery_type="remind" ) self.assert_json_error(result, "Reminders can only be set for streams.") # Setting a reminder in PM's to ourself is successful. # Required by reminders from message actions popover caret feature. result = self.do_schedule_message( - "private", hamlet.email, content + " 5", defer_until_str, delivery_type="remind" + "private", [hamlet.email], content + " 5", defer_until_str, delivery_type="remind" ) message = self.last_scheduled_message() self.assert_json_success(result) @@ -1891,7 +1893,7 @@ class StreamMessagesTest(ZulipTestCase): "/api/v1/messages", { "type": "stream", - "to": "Verona", + "to": orjson.dumps("Verona").decode(), "sender": self.mit_email("sipbtest"), "client": "zephyr_mirror", "topic": "announcement", diff --git a/zerver/tests/test_reactions.py b/zerver/tests/test_reactions.py index bc7681e1bd..ab82f949db 100644 --- a/zerver/tests/test_reactions.py +++ b/zerver/tests/test_reactions.py @@ -281,7 +281,11 @@ class ReactionMessageIDTest(ZulipTestCase): result = self.api_post( pm_sender, "/api/v1/messages", - {"type": "private", "content": "Test message", "to": pm_recipient.email}, + { + "type": "private", + "content": "Test message", + "to": orjson.dumps([pm_recipient.email]).decode(), + }, ) pm_id = self.assert_json_success(result)["id"] reaction_info = { @@ -306,7 +310,11 @@ class ReactionTest(ZulipTestCase): pm = self.api_post( pm_sender, "/api/v1/messages", - {"type": "private", "content": "Test message", "to": pm_recipient.email}, + { + "type": "private", + "content": "Test message", + "to": orjson.dumps([pm_recipient.email]).decode(), + }, ) self.assert_json_success(pm) content = orjson.loads(pm.content) @@ -336,7 +344,11 @@ class ReactionTest(ZulipTestCase): pm = self.api_post( pm_sender, "/api/v1/messages", - {"type": "private", "content": "Test message", "to": pm_recipient.email}, + { + "type": "private", + "content": "Test message", + "to": orjson.dumps([pm_recipient.email]).decode(), + }, ) self.assert_json_success(pm) @@ -417,7 +429,11 @@ class ReactionEventTest(ZulipTestCase): result = self.api_post( pm_sender, "/api/v1/messages", - {"type": "private", "content": "Test message", "to": pm_recipient.email}, + { + "type": "private", + "content": "Test message", + "to": orjson.dumps([pm_recipient.email]).decode(), + }, ) pm_id = self.assert_json_success(result)["id"] @@ -456,7 +472,11 @@ class ReactionEventTest(ZulipTestCase): result = self.api_post( pm_sender, "/api/v1/messages", - {"type": "private", "content": "Test message", "to": pm_recipient.email}, + { + "type": "private", + "content": "Test message", + "to": orjson.dumps([pm_recipient.email]).decode(), + }, ) content = self.assert_json_success(result) pm_id = content["id"] diff --git a/zerver/tests/test_user_groups.py b/zerver/tests/test_user_groups.py index d4485844af..f2c59c2f52 100644 --- a/zerver/tests/test_user_groups.py +++ b/zerver/tests/test_user_groups.py @@ -418,7 +418,7 @@ class UserGroupAPITestCase(UserGroupTestCase): payload = dict( type="stream", - to=stream_name, + to=orjson.dumps(stream_name).decode(), topic="whatever", content=content_with_group_mention, ) diff --git a/zerver/tests/test_widgets.py b/zerver/tests/test_widgets.py index 9121e71ceb..7bbca54ce6 100644 --- a/zerver/tests/test_widgets.py +++ b/zerver/tests/test_widgets.py @@ -66,7 +66,7 @@ class WidgetContentTestCase(ZulipTestCase): payload = dict( type="stream", - to=stream_name, + to=orjson.dumps(stream_name).decode(), topic="whatever", content="whatever", ) @@ -122,7 +122,7 @@ class WidgetContentTestCase(ZulipTestCase): payload = dict( type="stream", - to=stream_name, + to=orjson.dumps(stream_name).decode(), topic="whatever", content=content, widget_content=orjson.dumps(widget_content).decode(), @@ -152,7 +152,7 @@ class WidgetContentTestCase(ZulipTestCase): payload = dict( type="stream", - to=stream_name, + to=orjson.dumps(stream_name).decode(), topic="whatever", content=content, ) @@ -180,7 +180,7 @@ class WidgetContentTestCase(ZulipTestCase): payload = dict( type="stream", - to=stream_name, + to=orjson.dumps(stream_name).decode(), topic="whatever", content=content, ) @@ -231,7 +231,7 @@ class WidgetContentTestCase(ZulipTestCase): payload = dict( type="stream", - to=stream_name, + to=orjson.dumps(stream_name).decode(), topic="whatever", content=content, ) @@ -259,7 +259,7 @@ class WidgetContentTestCase(ZulipTestCase): payload = dict( type="stream", - to=stream_name, + to=orjson.dumps(stream_name).decode(), topic="whatever", content=content, ) @@ -317,7 +317,7 @@ class WidgetContentTestCase(ZulipTestCase): payload = dict( type="stream", - to=stream_name, + to=orjson.dumps(stream_name).decode(), topic="whatever", content=content, ) @@ -374,7 +374,7 @@ class WidgetContentTestCase(ZulipTestCase): payload = dict( type="stream", - to=stream_name, + to=orjson.dumps(stream_name).decode(), topic="whatever", content=content, )