From 807a4428f62f1cbb76d369b5581a0bcd71d62afc Mon Sep 17 00:00:00 2001 From: Anders Kaseorg Date: Wed, 27 Mar 2024 17:04:36 -0700 Subject: [PATCH] compose_validate: Remove autosubscribe feature. This was only used in the undocumented narrow_stream mode, and relied on a deprecated synchronous XHR request. Signed-off-by: Anders Kaseorg --- docs/tutorials/life-of-a-request.md | 14 +--- web/src/compose_validate.ts | 58 +------------- web/tests/compose_validate.test.js | 44 +---------- zerver/decorator.py | 4 +- zerver/tests/test_decorators.py | 116 +--------------------------- zerver/tests/test_subs.py | 101 ------------------------ zerver/tests/test_urls.py | 1 - zerver/views/streams.py | 46 +---------- zproject/legacy_urls.py | 21 ----- zproject/urls.py | 6 +- 10 files changed, 12 insertions(+), 399 deletions(-) delete mode 100644 zproject/legacy_urls.py diff --git a/docs/tutorials/life-of-a-request.md b/docs/tutorials/life-of-a-request.md index 2cca460947..640a3e1d4e 100644 --- a/docs/tutorials/life-of-a-request.md +++ b/docs/tutorials/life-of-a-request.md @@ -60,9 +60,9 @@ in [the directory structure doc](../overview/directory-structure.md). The main Zulip Django app is `zerver`. The routes are found in -`zproject/urls.py` and `zproject/legacy_urls.py`. +`zproject/urls.py`. -There are HTML-serving, REST API, legacy, and webhook url patterns. We +There are HTML-serving, REST API, and webhook url patterns. We will look at how each of these types of requests are handled, and focus on how the REST API handles our user creation example. @@ -85,7 +85,7 @@ Note the `zh-hans` prefix--that url pattern gets added by `i18n_patterns`. Our example is a REST API endpoint. It's a PUT to `/users`. With the exception of incoming webhooks (which we do not usually control the -format of), legacy endpoints, and logged-out endpoints, Zulip uses REST +format of) and logged-out endpoints, Zulip uses REST for its API. This means that we use: - POST for creating something new where we don't have a unique @@ -144,14 +144,6 @@ rest_path('users', In this way, the API is partially self-documenting. -### Legacy endpoints are used by the web client - -The endpoints from the legacy JSON API are written without REST in -mind. They are used extensively by the web client, and use POST. - -You can see them in -[zproject/legacy_urls.py](https://github.com/zulip/zulip/blob/main/zproject/legacy_urls.py). - ### Incoming webhook integrations may not be RESTful Zulip endpoints that are called by other services for integrations have diff --git a/web/src/compose_validate.ts b/web/src/compose_validate.ts index eab88a6efd..bd496aef24 100644 --- a/web/src/compose_validate.ts +++ b/web/src/compose_validate.ts @@ -1,6 +1,5 @@ import $ from "jquery"; import assert from "minimalistic-assert"; -import {z} from "zod"; import * as resolved_topic from "../shared/src/resolved_topic"; import render_compose_banner from "../templates/compose_banner/compose_banner.hbs"; @@ -10,7 +9,6 @@ import render_stream_wildcard_warning from "../templates/compose_banner/stream_w import render_wildcard_mention_not_allowed_error from "../templates/compose_banner/wildcard_mention_not_allowed_error.hbs"; import render_compose_limit_indicator from "../templates/compose_limit_indicator.hbs"; -import * as channel from "./channel"; import * as compose_banner from "./compose_banner"; import * as compose_pm_pill from "./compose_pm_pill"; import * as compose_state from "./compose_state"; @@ -18,7 +16,6 @@ import * as compose_ui from "./compose_ui"; import {$t} from "./i18n"; import * as message_store from "./message_store"; import * as narrow_state from "./narrow_state"; -import {page_params} from "./page_params"; import * as peer_data from "./peer_data"; import * as people from "./people"; import * as reactions from "./reactions"; @@ -46,10 +43,6 @@ type StreamWildcardOptions = { export let wildcard_mention_threshold = 15; -const server_subscription_exists_schema = z.object({ - subscribed: z.boolean(), -}); - export function set_upload_in_progress(status: boolean): void { upload_in_progress = status; update_send_button_status(); @@ -397,42 +390,6 @@ export function get_invalid_recipient_emails(): string[] { return invalid_recipients; } -function check_unsubscribed_stream_for_send( - stream_name: string, - autosubscribe: boolean, -): string | undefined { - let result; - if (!autosubscribe) { - return "not-subscribed"; - } - - // In the rare circumstance of the autosubscribe option, we - // *Synchronously* try to subscribe to the stream before sending - // the message. This is deprecated and we hope to remove it; see - // #4650. - void channel.post({ - url: "/json/subscriptions/exists", - data: {stream: stream_name, autosubscribe: true}, - async: false, - success(data) { - const clean_data = server_subscription_exists_schema.parse(data); - if (clean_data.subscribed) { - result = "subscribed"; - } else { - result = "not-subscribed"; - } - }, - error(xhr: JQuery.jqXHR) { - if (xhr.status === 404) { - result = "does-not-exist"; - } else { - result = "error"; - } - }, - }); - return result; -} - function is_recipient_large_stream(): boolean { const stream_id = compose_state.stream_id(); if (stream_id === undefined) { @@ -584,20 +541,12 @@ export function validate_stream_message_mentions(opts: StreamWildcardOptions): b return true; } -export function validation_error(error_type: string, stream_name: string): boolean { +function validation_error(error_type: string, stream_name: string): boolean { const $banner_container = $("#compose_banners"); switch (error_type) { case "does-not-exist": compose_banner.show_stream_does_not_exist_error(stream_name); return false; - case "error": - compose_banner.show_error_message( - $t({defaultMessage: "Error checking subscription."}), - compose_banner.CLASSNAMES.subscription_error, - $banner_container, - $("#compose_select_recipient_widget_wrapper"), - ); - return false; case "not-subscribed": { if ( $(`#compose_banners .${CSS.escape(compose_banner.CLASSNAMES.user_not_subscribed)}`) @@ -633,10 +582,7 @@ export function validate_stream_message_address_info(stream_name: string): boole if (stream_data.is_subscribed_by_name(stream_name)) { return true; } - const autosubscribe = page_params.narrow_stream !== undefined; - const error_type = check_unsubscribed_stream_for_send(stream_name, autosubscribe); - assert(error_type !== undefined); - return validation_error(error_type, stream_name); + return validation_error("not-subscribed", stream_name); } function validate_stream_message(scheduling_message: boolean): boolean { diff --git a/web/tests/compose_validate.test.js b/web/tests/compose_validate.test.js index 28815b3ef8..c56a71593e 100644 --- a/web/tests/compose_validate.test.js +++ b/web/tests/compose_validate.test.js @@ -8,9 +8,8 @@ const {mock_esm, zrequire} = require("./lib/namespace"); const {run_test, noop} = require("./lib/test"); const blueslip = require("./lib/zblueslip"); const $ = require("./lib/zjquery"); -const {current_user, page_params, realm} = require("./lib/zpage_params"); +const {current_user, realm} = require("./lib/zpage_params"); -const channel = mock_esm("../src/channel"); const compose_banner = zrequire("compose_banner"); const compose_pm_pill = zrequire("compose_pm_pill"); const compose_state = zrequire("compose_state"); @@ -105,53 +104,12 @@ test_ui("validate_stream_message_address_info", ({mock_template}) => { assert.ok(!compose_validate.validate_stream_message_address_info("social")); assert.ok(user_not_subscribed_rendered); - page_params.narrow_stream = "social"; - channel.post = (payload) => { - assert.equal(payload.data.stream, "social"); - payload.data.subscribed = true; - payload.success(payload.data); - }; - assert.ok(compose_validate.validate_stream_message_address_info("social")); - party_sub.name = "Frontend"; party_sub.stream_id = 102; stream_data.add_sub(party_sub); - channel.post = (payload) => { - assert.equal(payload.data.stream, "Frontend"); - payload.data.subscribed = false; - payload.success(payload.data); - }; user_not_subscribed_rendered = false; assert.ok(!compose_validate.validate_stream_message_address_info("Frontend")); assert.ok(user_not_subscribed_rendered); - - let stream_does_not_exist_rendered = false; - mock_template("compose_banner/stream_does_not_exist_error.hbs", false, (data) => { - assert.equal(data.classname, compose_banner.CLASSNAMES.stream_does_not_exist); - assert.equal(data.stream_name, "Frontend"); - stream_does_not_exist_rendered = true; - return ""; - }); - channel.post = (payload) => { - assert.equal(payload.data.stream, "Frontend"); - payload.error({status: 404}); - }; - assert.ok(!compose_validate.validate_stream_message_address_info("Frontend")); - assert.ok(stream_does_not_exist_rendered); - - let subscription_error_rendered = false; - mock_template("compose_banner/compose_banner.hbs", false, (data) => { - assert.equal(data.classname, "subscription_error"); - assert.equal(data.banner_text, $t({defaultMessage: "Error checking subscription."})); - subscription_error_rendered = true; - return ""; - }); - channel.post = (payload) => { - assert.equal(payload.data.stream, "social"); - payload.error({status: 500}); - }; - assert.ok(!compose_validate.validate_stream_message_address_info("social")); - assert.ok(subscription_error_rendered); }); test_ui("validate", ({mock_template}) => { diff --git a/zerver/decorator.py b/zerver/decorator.py index e0375fd9e4..feed2de62c 100644 --- a/zerver/decorator.py +++ b/zerver/decorator.py @@ -898,7 +898,7 @@ def authenticated_json_view( *args: ParamT.args, **kwargs: ParamT.kwargs, ) -> HttpResponse: - if not request.user.is_authenticated: + if not request.user.is_authenticated: # nocoverage raise UnauthorizedError user_profile = request.user @@ -907,7 +907,7 @@ def authenticated_json_view( validate_account_and_subdomain(request, user_profile) - if user_profile.is_incoming_webhook: + if user_profile.is_incoming_webhook: # nocoverage raise JsonableError(_("Webhook bots can only access webhooks")) process_client(request, user_profile, is_browser_view=True, query=view_func.__name__) diff --git a/zerver/tests/test_decorators.py b/zerver/tests/test_decorators.py index e61680a8d2..5de42a014b 100644 --- a/zerver/tests/test_decorators.py +++ b/zerver/tests/test_decorators.py @@ -15,7 +15,7 @@ from typing_extensions import override from zerver.actions.create_realm import do_create_realm from zerver.actions.create_user import do_reactivate_user -from zerver.actions.realm_settings import do_deactivate_realm, do_reactivate_realm +from zerver.actions.realm_settings import do_deactivate_realm from zerver.actions.user_settings import do_change_user_setting from zerver.actions.users import change_user_is_active, do_deactivate_user from zerver.decorator import ( @@ -1139,120 +1139,6 @@ class TestAuthenticatedRequirePostDecorator(ZulipTestCase): ["WARNING:root:Method Not Allowed (GET): /api/v1/dev_fetch_api_key"], ) - with self.assertLogs(level="WARNING") as mock_warning: - result = self.client_get(r"/json/subscriptions/exists", {"stream": "Verona"}) - self.assertEqual(result.status_code, 405) - self.assertEqual( - mock_warning.output, - ["WARNING:root:Method Not Allowed (GET): /json/subscriptions/exists"], - ) - - -class TestAuthenticatedJsonPostViewDecorator(ZulipTestCase): - def test_authenticated_json_post_view_if_everything_is_correct(self) -> None: - user = self.example_user("hamlet") - self.login_user(user) - response = self._do_test(user) - self.assert_json_success(response) - - def test_authenticated_json_post_view_if_user_not_logged_in(self) -> None: - user = self.example_user("hamlet") - self.assert_json_error_contains( - self._do_test(user), - "Not logged in: API authentication or user session required", - status_code=401, - ) - - def test_authenticated_json_post_view_with_get_request(self) -> None: - self.login("hamlet") - with self.assertLogs(level="WARNING") as m: - result = self.client_get(r"/json/subscriptions/exists", {"stream": "Verona"}) - self.assertEqual(result.status_code, 405) - self.assertEqual( - m.output, - [ - "WARNING:root:Method Not Allowed ({}): {}".format( - "GET", "/json/subscriptions/exists" - ) - ], - ) - - def test_authenticated_json_post_view_if_subdomain_is_invalid(self) -> None: - user = self.example_user("hamlet") - email = user.delivery_email - self.login_user(user) - with self.assertLogs(level="WARNING") as m, mock.patch( - "zerver.decorator.get_subdomain", return_value="" - ): - self.assert_json_error_contains( - self._do_test(user), "Account is not associated with this subdomain" - ) - self.assertEqual( - m.output, - [ - "WARNING:root:User {} ({}) attempted to access API on wrong subdomain ({})".format( - email, "zulip", "" - ), - "WARNING:root:User {} ({}) attempted to access API on wrong subdomain ({})".format( - email, "zulip", "" - ), - ], - ) - - with self.assertLogs(level="WARNING") as m, mock.patch( - "zerver.decorator.get_subdomain", return_value="acme" - ): - self.assert_json_error_contains( - self._do_test(user), "Account is not associated with this subdomain" - ) - self.assertEqual( - m.output, - [ - "WARNING:root:User {} ({}) attempted to access API on wrong subdomain ({})".format( - email, "zulip", "acme" - ), - "WARNING:root:User {} ({}) attempted to access API on wrong subdomain ({})".format( - email, "zulip", "acme" - ), - ], - ) - - def test_authenticated_json_post_view_if_user_is_incoming_webhook(self) -> None: - bot = self.example_user("webhook_bot") - bot.set_password("test") - bot.save() - self.login_by_email(bot.email, password="test") - self.assert_json_error_contains(self._do_test(bot), "Webhook bots can only access webhooks") - - def test_authenticated_json_post_view_if_user_is_not_active(self) -> None: - user_profile = self.example_user("hamlet") - self.login_user(user_profile) - # we deactivate user manually because do_deactivate_user removes user session - change_user_is_active(user_profile, False) - self.assert_json_error_contains( - self._do_test(user_profile), "Account is deactivated", status_code=401 - ) - do_reactivate_user(user_profile, acting_user=None) - - def test_authenticated_json_post_view_if_user_realm_is_deactivated(self) -> None: - user_profile = self.example_user("hamlet") - self.login_user(user_profile) - # we deactivate user's realm manually because do_deactivate_user removes user session - user_profile.realm.deactivated = True - user_profile.realm.save() - self.assert_json_error_contains( - self._do_test(user_profile), - "This organization has been deactivated", - status_code=401, - ) - do_reactivate_realm(user_profile.realm) - - def _do_test(self, user: UserProfile) -> "TestHttpResponse": - stream_name = "stream name" - self.common_subscribe_to_streams(user, [stream_name], allow_fail=True) - data = {"stream": stream_name} - return self.client_post("/json/subscriptions/exists", data) - class TestAuthenticatedJsonViewDecorator(ZulipTestCase): def test_authenticated_json_view_if_subdomain_is_invalid(self) -> None: diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index a57505cf06..36f1621dc5 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -5253,107 +5253,6 @@ class SubscriptionAPITest(ZulipTestCase): ) self.assert_json_error(result, f"Stream(s) ({random_streams[0]}) do not exist") - def helper_subscriptions_exists( - self, stream: str, expect_success: bool, subscribed: bool - ) -> None: - """ - Call /json/subscriptions/exists on a stream and expect a certain result. - """ - result = self.client_post("/json/subscriptions/exists", {"stream": stream}) - if expect_success: - json = self.assert_json_success(result) - else: - self.assertEqual(result.status_code, 404) - json = result.json() - if subscribed: - self.assertIn("subscribed", json) - self.assertEqual(json["subscribed"], subscribed) - - def test_successful_subscriptions_exists_subbed(self) -> None: - """ - Calling /json/subscriptions/exist on a stream to which you are subbed - should return that it exists and that you are subbed. - """ - self.assertNotEqual(len(self.streams), 0) # necessary for full test coverage - self.helper_subscriptions_exists(self.streams[0], True, True) - - def test_successful_subscriptions_exists_not_subbed(self) -> None: - """ - Calling /json/subscriptions/exist on a stream to which you are not - subbed should return that it exists and that you are not subbed. - """ - all_stream_names = [stream.name for stream in Stream.objects.filter(realm=self.test_realm)] - streams_not_subbed = list(set(all_stream_names) - set(self.streams)) - self.assertNotEqual(len(streams_not_subbed), 0) # necessary for full test coverage - self.helper_subscriptions_exists(streams_not_subbed[0], True, False) - - def test_subscriptions_does_not_exist(self) -> None: - """ - Calling /json/subscriptions/exist on a stream that doesn't exist should - return that it doesn't exist. - """ - random_streams = self.make_random_stream_names(self.streams) - self.assertNotEqual(len(random_streams), 0) # necessary for full test coverage - self.helper_subscriptions_exists(random_streams[0], False, False) - - def test_subscriptions_exist_invalid_name(self) -> None: - """ - Calling /json/subscriptions/exist on a stream whose name is invalid (as - defined by valid_stream_name in zerver/views.py) should return a JSON - error. - """ - # currently, the only invalid stream name is the empty string - invalid_stream_name = "" - result = self.client_post("/json/subscriptions/exists", {"stream": invalid_stream_name}) - self.assert_json_error(result, "Stream name can't be empty!") - - def test_existing_subscriptions_autosubscription(self) -> None: - """ - Call /json/subscriptions/exist on an existing stream and autosubscribe to it. - """ - stream_name = "new_public_stream" - cordelia = self.example_user("cordelia") - self.common_subscribe_to_streams(cordelia, [stream_name], invite_only=False) - result = self.client_post( - "/json/subscriptions/exists", {"stream": stream_name, "autosubscribe": "false"} - ) - response_dict = self.assert_json_success(result) - self.assertIn("subscribed", response_dict) - self.assertFalse(response_dict["subscribed"]) - - result = self.client_post( - "/json/subscriptions/exists", {"stream": stream_name, "autosubscribe": "true"} - ) - response_dict = self.assert_json_success(result) - self.assertIn("subscribed", response_dict) - self.assertTrue(response_dict) - - def test_existing_subscriptions_autosubscription_private_stream(self) -> None: - """Call /json/subscriptions/exist on an existing private stream with - autosubscribe should fail. - """ - stream_name = "Saxony" - cordelia = self.example_user("cordelia") - self.common_subscribe_to_streams(cordelia, [stream_name], invite_only=True) - stream = get_stream(stream_name, self.test_realm) - - result = self.client_post( - "/json/subscriptions/exists", {"stream": stream_name, "autosubscribe": "true"} - ) - # We can't see invite-only streams here - self.assert_json_error(result, "Invalid stream name 'Saxony'", status_code=404) - # Importantly, we are not now subscribed - self.assertEqual(num_subscribers_for_stream_id(stream.id), 1) - - # A user who is subscribed still sees the stream exists - self.login("cordelia") - result = self.client_post( - "/json/subscriptions/exists", {"stream": stream_name, "autosubscribe": "false"} - ) - response_dict = self.assert_json_success(result) - self.assertIn("subscribed", response_dict) - self.assertTrue(response_dict) - def get_subscription(self, user_profile: UserProfile, stream_name: str) -> Subscription: stream = get_stream(stream_name, self.test_realm) return Subscription.objects.get( diff --git a/zerver/tests/test_urls.py b/zerver/tests/test_urls.py index a36fd26b06..2a9dd297e2 100644 --- a/zerver/tests/test_urls.py +++ b/zerver/tests/test_urls.py @@ -106,7 +106,6 @@ class PublicURLTest(ZulipTestCase): 401: [ "/json/messages", "/json/invites", - "/json/subscriptions/exists", "/api/v1/users/me/subscriptions/properties", "/json/fetch_api_key", "/json/users/me/subscriptions", diff --git a/zerver/views/streams.py b/zerver/views/streams.py index c9aa8bbe3a..4f9708186a 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -40,19 +40,10 @@ from zerver.actions.streams import ( get_subscriber_ids, ) from zerver.context_processors import get_valid_realm_from_request -from zerver.decorator import ( - authenticated_json_view, - require_non_guest_user, - require_post, - require_realm_admin, -) +from zerver.decorator import require_non_guest_user, require_realm_admin from zerver.lib.default_streams import get_default_stream_ids_for_realm from zerver.lib.email_mirror_helpers import encode_email_address -from zerver.lib.exceptions import ( - JsonableError, - OrganizationOwnerRequiredError, - ResourceNotFoundError, -) +from zerver.lib.exceptions import JsonableError, OrganizationOwnerRequiredError from zerver.lib.mention import MentionBackend, silent_mention_syntax_for_user from zerver.lib.message import bulk_access_stream_messages_query from zerver.lib.request import REQ, has_request_variables @@ -74,7 +65,6 @@ from zerver.lib.streams import ( list_to_streams, stream_to_dict, ) -from zerver.lib.string_validation import check_stream_name from zerver.lib.subscription_info import gather_subscriptions from zerver.lib.timeout import TimeoutExpiredError, timeout from zerver.lib.topic import ( @@ -959,38 +949,6 @@ def delete_in_topic( return json_success(request, data={"complete": True}) -@require_post -@authenticated_json_view -@has_request_variables -def json_stream_exists( - request: HttpRequest, - user_profile: UserProfile, - stream_name: str = REQ("stream"), - autosubscribe: bool = REQ(json_validator=check_bool, default=False), -) -> HttpResponse: - check_stream_name(stream_name) - - try: - (stream, sub) = access_stream_by_name(user_profile, stream_name) - except JsonableError as e: - raise ResourceNotFoundError(e.msg) - - # access_stream functions return a subscription if and only if we - # are already subscribed. - result = {"subscribed": sub is not None} - - # If we got here, we're either subscribed or the stream is public. - # So if we're not yet subscribed and autosubscribe is enabled, we - # should join. - if sub is None and autosubscribe: - bulk_add_subscriptions( - user_profile.realm, [stream], [user_profile], acting_user=user_profile - ) - result["subscribed"] = True - - return json_success(request, data=result) # results are ignored for HEAD requests - - @has_request_variables def json_get_stream_id( request: HttpRequest, user_profile: UserProfile, stream_name: str = REQ("stream") diff --git a/zproject/legacy_urls.py b/zproject/legacy_urls.py deleted file mode 100644 index ad178bb52d..0000000000 --- a/zproject/legacy_urls.py +++ /dev/null @@ -1,21 +0,0 @@ -from typing import List, Union - -from django.urls import URLPattern, URLResolver, path - -import zerver.views -import zerver.views.auth -import zerver.views.report -import zerver.views.streams -import zerver.views.tutorial - -# Future endpoints should add to urls.py, which includes these legacy URLs - -legacy_urls: List[Union[URLPattern, URLResolver]] = [ - # These are json format views used by the web client. They require a logged in browser. - # We should remove this endpoint and all code related to it. - # It returns a 404 if the stream doesn't exist, which is confusing - # for devs, and I don't think we need to go to the server - # any more to find out about subscriptions, since they are already - # pushed to us via the event system. - path("json/subscriptions/exists", zerver.views.streams.json_stream_exists), -] diff --git a/zproject/urls.py b/zproject/urls.py index c9f61245e1..4bb1bf4f79 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -234,7 +234,6 @@ from zerver.views.video_calls import ( ) from zerver.views.zephyr import webathena_kerberos_login from zproject import dev_urls -from zproject.legacy_urls import legacy_urls if settings.TWO_FACTOR_AUTHENTICATION_ENABLED: # nocoverage from two_factor.gateways.twilio.urls import urlpatterns as tf_twilio_urls @@ -242,9 +241,6 @@ if settings.TWO_FACTOR_AUTHENTICATION_ENABLED: # nocoverage # NB: There are several other pieces of code which route requests by URL: # -# - legacy_urls.py contains API endpoint written before the redesign -# and should not be added to. -# # - runtornado.py has its own URL list for Tornado views. See the # invocation of web.Application in that file. # @@ -866,4 +862,4 @@ urls += [path("health", health)] # The sequence is important; if i18n URLs don't come first then # reverse URL mapping points to i18n URLs which causes the frontend # tests to fail -urlpatterns = i18n_patterns(*i18n_urls) + urls + legacy_urls +urlpatterns = i18n_patterns(*i18n_urls) + urls