From 49388d5d3d49c97b4b7e69b5c66ad7612816ecde Mon Sep 17 00:00:00 2001 From: Prakhar Pratyush Date: Tue, 21 Nov 2023 15:09:13 +0530 Subject: [PATCH] topic_mentions: Fix restriction rule for @-topic mentions. Now, the topic wildcard mention follows the following rules: * If the topic has less than 15 participants , anyone can use @ topic mentions. * For more than 15, the org setting 'wildcard_mention_policy' determines who can use @ topic mentions. Earlier, topic wildcard mentions followed the same restriction as stream wildcard mentions, which was incorrect. Fixes part of #27700. --- api_docs/changelog.md | 10 +++ version.py | 2 +- web/src/compose.js | 29 ++++++--- web/src/echo.js | 2 +- web/src/markdown.js | 11 +++- web/src/message_edit.js | 17 ++++- web/src/transmit.js | 4 +- .../wildcard_mention_not_allowed_error.hbs | 6 +- .../organization_permissions_admin.hbs | 2 +- web/tests/transmit.test.js | 28 ++++++++ zerver/actions/message_edit.py | 28 ++++++-- zerver/actions/message_send.py | 21 ++++-- zerver/lib/exceptions.py | 26 ++++++++ zerver/lib/markdown/__init__.py | 3 - zerver/lib/message.py | 33 +++++++--- zerver/openapi/zulip.yaml | 64 +++++++++++++++++++ zerver/tests/test_message_edit.py | 39 +++++++---- zerver/tests/test_message_send.py | 25 ++++---- 18 files changed, 282 insertions(+), 68 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 6922244052..c2eb725d76 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,16 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 8.0 +**Feature level 229** + +* [`PATCH /messages/{message_id}`](/api/update-message), [`POST + /messages`](/api/send-message): Topic wildcard mentions involving + large numbers of participants are now restricted by + `wildcard_mention_policy`. The server now uses the + `STREAM_WILDCARD_MENTION_NOT_ALLOWED` and + `TOPIC_WILDCARD_MENTION_NOT_ALLOWED` error codes when a message is + rejected because of `wildcard_mention_policy`. + **Feature level 228** * [`GET /events`](/api/get-events): `realm_user` events with `op: "update"` diff --git a/version.py b/version.py index a4e1f6c726..7f77715976 100644 --- a/version.py +++ b/version.py @@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.9.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 = 228 +API_FEATURE_LEVEL = 229 # 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/web/src/compose.js b/web/src/compose.js index 263df67f8d..574c613f0e 100644 --- a/web/src/compose.js +++ b/web/src/compose.js @@ -5,6 +5,7 @@ import $ from "jquery"; import _ from "lodash"; import render_success_message_scheduled_banner from "../templates/compose_banner/success_message_scheduled_banner.hbs"; +import render_wildcard_mention_not_allowed_error from "../templates/compose_banner/wildcard_mention_not_allowed_error.hbs"; import * as channel from "./channel"; import * as compose_banner from "./compose_banner"; @@ -180,16 +181,26 @@ export function send_message(request = create_message_object()) { send_message_success(request, data); } - function error(response) { - // If we're not local echo'ing messages, or if this message was not - // locally echoed, show error in compose box + function error(response, server_error_code) { + // Error callback for failed message send attempts. if (!locally_echoed) { - compose_banner.show_error_message( - response, - compose_banner.CLASSNAMES.generic_compose_error, - $("#compose_banners"), - $("textarea#compose-textarea"), - ); + if (server_error_code === "TOPIC_WILDCARD_MENTION_NOT_ALLOWED") { + // The topic wildcard mention permission code path has + // a special error. + const new_row = render_wildcard_mention_not_allowed_error({ + banner_type: compose_banner.ERROR, + classname: compose_banner.CLASSNAMES.wildcards_not_allowed, + }); + compose_banner.append_compose_banner_to_banner_list(new_row, $("#compose_banners")); + } else { + compose_banner.show_error_message( + response, + compose_banner.CLASSNAMES.generic_compose_error, + $("#compose_banners"), + $("textarea#compose-textarea"), + ); + } + // For messages that were not locally echoed, we're // responsible for hiding the compose spinner to restore // the compose box so one can send a next message. diff --git a/web/src/echo.js b/web/src/echo.js index bd337cf288..cb062dc85f 100644 --- a/web/src/echo.js +++ b/web/src/echo.js @@ -96,7 +96,7 @@ function resend_message(message, $row, {on_send_message_success, send_message}) failed_message_success(message_id); } - function on_error(response) { + function on_error(response, _server_error_code) { message_send_error(message.id, response); setTimeout(() => { hide_retry_spinner($row); diff --git a/web/src/markdown.js b/web/src/markdown.js index c89e5de21a..b2c804fbb6 100644 --- a/web/src/markdown.js +++ b/web/src/markdown.js @@ -85,13 +85,22 @@ function contains_problematic_linkifier({content, get_linkifier_map}) { return false; } +function contains_topic_wildcard_mention(content) { + // If the content has topic wildcard mention (@**topic**) then don't + // render it locally. We have only server-side restriction check for + // @topic mention. This helps to show the error message (no permission) + // via the compose banner and not to local-echo then fail due to restriction. + return content.includes("@**topic**"); +} + function content_contains_backend_only_syntax({content, get_linkifier_map}) { // Try to guess whether or not a message contains syntax that only the // backend Markdown processor can correctly handle. // If it doesn't, we can immediately render it client-side for local echo. return ( contains_preview_link(content) || - contains_problematic_linkifier({content, get_linkifier_map}) + contains_problematic_linkifier({content, get_linkifier_map}) || + contains_topic_wildcard_mention(content) ); } diff --git a/web/src/message_edit.js b/web/src/message_edit.js index 7426d20dcf..8af05d5988 100644 --- a/web/src/message_edit.js +++ b/web/src/message_edit.js @@ -2,6 +2,7 @@ import ClipboardJS from "clipboard"; import $ from "jquery"; import * as resolved_topic from "../shared/src/resolved_topic"; +import render_wildcard_mention_not_allowed_error from "../templates/compose_banner/wildcard_mention_not_allowed_error.hbs"; import render_delete_message_modal from "../templates/confirm_dialog/confirm_delete_message.hbs"; import render_confirm_moving_messages_modal from "../templates/confirm_dialog/confirm_moving_messages.hbs"; import render_message_edit_form from "../templates/message_edit_form.hbs"; @@ -1070,13 +1071,23 @@ export function save_message_row_edit($row) { hide_message_edit_spinner($row); if (xhr.readyState !== 0) { + const $container = compose_banner.get_compose_banner_container( + $row.find("textarea"), + ); + + if (xhr.responseJSON?.code === "TOPIC_WILDCARD_MENTION_NOT_ALLOWED") { + const new_row = render_wildcard_mention_not_allowed_error({ + banner_type: compose_banner.ERROR, + classname: compose_banner.CLASSNAMES.wildcards_not_allowed, + }); + compose_banner.append_compose_banner_to_banner_list(new_row, $container); + return; + } + const message = channel.xhr_error_message( $t({defaultMessage: "Error editing message"}), xhr, ); - const $container = compose_banner.get_compose_banner_container( - $row.find("textarea"), - ); compose_banner.show_error_message( message, compose_banner.CLASSNAMES.generic_compose_error, diff --git a/web/src/transmit.js b/web/src/transmit.js index 48efd18d07..29180cb4df 100644 --- a/web/src/transmit.js +++ b/web/src/transmit.js @@ -71,7 +71,7 @@ export function send_message(request, on_success, error) { } const response = channel.xhr_error_message("Error sending message", xhr); - error(response); + error(response, xhr.responseJSON?.code); }, }); } finally { @@ -97,7 +97,7 @@ export function reply_message(opts) { // already handles things like reporting times to the server.) } - function error() { + function error(_response, _server_error_code) { // TODO: In our current use case, which is widgets, to meaningfully // handle errors, we would want the widget to provide some // kind of callback to us so it can do some appropriate UI. diff --git a/web/templates/compose_banner/wildcard_mention_not_allowed_error.hbs b/web/templates/compose_banner/wildcard_mention_not_allowed_error.hbs index 3b1d861cd3..b05ba39e23 100644 --- a/web/templates/compose_banner/wildcard_mention_not_allowed_error.hbs +++ b/web/templates/compose_banner/wildcard_mention_not_allowed_error.hbs @@ -1,5 +1,9 @@ {{#> compose_banner }} {{/compose_banner}} diff --git a/web/templates/settings/organization_permissions_admin.hbs b/web/templates/settings/organization_permissions_admin.hbs index 12cf4b3582..dae4518a10 100644 --- a/web/templates/settings/organization_permissions_admin.hbs +++ b/web/templates/settings/organization_permissions_admin.hbs @@ -101,7 +101,7 @@
-