From f0f02d05ab98f79f4c64564cc9906e5fe900b4ae Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Tue, 28 Feb 2023 00:47:40 +0100 Subject: [PATCH] send_message_backend: Remove the realm_str API param. This already became useless in 6e11754642a59d83d7a575b78e6c3e988d3b6c93, as detailed in the API changelog entry here. At this point, we should eliminate this param and the weird code around it. This commit also deletes the associated tests added in 6e11754642a59d83d7a575b78e6c3e988d3b6c93, since with realm_str removed, they make no sense anymore (and actually fail with an OpenAPI error due to using params not used in the API). Hypothetically they could be translated to use the subdomain= kwarg, but that also doesn't make sense, since at that point they'd be just testing the case of a user making an API request on a different subdomain than their current one and that's just redundant and already tested generally in test_decorators. --- api_docs/changelog.md | 6 +++ version.py | 2 +- zerver/tests/test_message_send.py | 76 ------------------------------- zerver/views/message_send.py | 5 -- 4 files changed, 7 insertions(+), 82 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index b9bcdcfaa5..e25b8e92c9 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,12 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 7.0 +**Feature level 166** +* [`POST /messages`](/api/send-message): Eliminated the `realm_str` parameter. This parameter + was already redundant due to it needing to match the realm of the user making the request, + otherwise returning an authorization error. With this, the parameter is removed, meaning + that if provided in the API request, it'll be ignored. + **Feature level 165** * [`PATCH /user_groups/{user_group_id}`](/api/update-user-group): The diff --git a/version.py b/version.py index b1e57cac2c..b000732667 100644 --- a/version.py +++ b/version.py @@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.4.3" # Changes should be accompanied by documentation explaining what the # new level means in api_docs/changelog.md, as well as "**Changes**" # entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 165 +API_FEATURE_LEVEL = 166 # Bump the minor PROVISION_VERSION to indicate that folks should provision # only when going from an old version of the code to a newer version. Bump diff --git a/zerver/tests/test_message_send.py b/zerver/tests/test_message_send.py index 2d705f49aa..708ffb45d4 100644 --- a/zerver/tests/test_message_send.py +++ b/zerver/tests/test_message_send.py @@ -27,7 +27,6 @@ from zerver.actions.message_send import ( internal_send_stream_message_by_name, send_rate_limited_pm_notification_to_bot_owner, ) -from zerver.actions.realm_domains import do_add_realm_domain from zerver.actions.realm_settings import do_set_realm_property from zerver.actions.streams import do_change_stream_post_policy from zerver.actions.users import do_change_can_forge_sender, do_deactivate_user @@ -1061,81 +1060,6 @@ class MessagePOSTTest(ZulipTestCase): ) self.assert_json_error(result, "User not authorized for this query") - def test_send_message_as_not_superuser_to_different_domain(self) -> None: - self.login("hamlet") - result = self.client_post( - "/json/messages", - { - "type": "stream", - "to": "Verona", - "content": "Test message", - "topic": "Test topic", - "realm_str": "mit", - }, - ) - self.assert_json_error(result, "User not authorized for this query") - - def test_send_message_with_can_forge_sender_to_different_domain(self) -> None: - user = self.example_user("default_bot") - do_change_can_forge_sender(user, True) - # To a non-existing realm: - result = self.api_post( - user, - "/api/v1/messages", - { - "type": "stream", - "to": "Verona", - "content": "Test message", - "topic": "Test topic", - "realm_str": "non-existing", - }, - ) - self.assert_json_error(result, "User not authorized for this query") - - # To an existing realm: - zephyr_realm = get_realm("zephyr") - result = self.api_post( - user, - "/api/v1/messages", - { - "type": "stream", - "to": "Verona", - "content": "Test message", - "topic": "Test topic", - "realm_str": zephyr_realm.string_id, - }, - ) - self.assert_json_error(result, "User not authorized for this query") - - def test_send_message_forging_message_to_another_realm(self) -> None: - """ - Test for a specific vulnerability that allowed a .can_forge_sender - user to forge a message as a cross-realm bot to a stream in another realm, - by setting up an appropriate RealmDomain and specifying JabberMirror as client - to cause the vulnerable codepath to be executed. - """ - user = self.example_user("default_bot") - do_change_can_forge_sender(user, True) - - zephyr_realm = get_realm("zephyr") - self.make_stream("Verona", zephyr_realm) - do_add_realm_domain(zephyr_realm, "zulip.com", False, acting_user=None) - result = self.api_post( - user, - "/api/v1/messages", - { - "type": "stream", - "to": "Verona", - "client": "JabberMirror", - "content": "Test message", - "topic": "Test topic", - "forged": "true", - "sender": "notification-bot@zulip.com", - "realm_str": zephyr_realm.string_id, - }, - ) - self.assert_json_error(result, "User not authorized for this query") - def test_send_message_when_sender_is_not_set(self) -> None: result = self.api_post( self.mit_user("starnine"), diff --git a/zerver/views/message_send.py b/zerver/views/message_send.py index e2ae983dcc..5e989f3263 100644 --- a/zerver/views/message_send.py +++ b/zerver/views/message_send.py @@ -198,7 +198,6 @@ def send_message_backend( topic_name: Optional[str] = REQ_topic(), message_content: str = REQ("content"), widget_content: Optional[str] = REQ(default=None, documentation_pending=True), - realm_str: Optional[str] = REQ("realm_str", default=None, documentation_pending=True), local_id: Optional[str] = REQ(default=None), queue_id: Optional[str] = REQ(default=None), delivery_type: str = REQ("delivery_type", default="send_now", documentation_pending=True), @@ -238,10 +237,6 @@ def send_message_backend( raise JsonableError(_("User not authorized for this query")) realm = user_profile.realm - if realm_str and realm_str != user_profile.realm.string_id: - # The realm_str parameter does nothing, because it has to match - # the user's realm - but we keep it around for backward compatibility. - raise JsonableError(_("User not authorized for this query")) if client.name in ["zephyr_mirror", "irc_mirror", "jabber_mirror", "JabberMirror"]: # Here's how security works for mirroring: