Mirror
/
zulip
mirror of https://github.com/zulip/zulip.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

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 <anders@zulip.com>
This commit is contained in:
Anders Kaseorg 2024-03-27 17:04:36 -07:00 committed by Tim Abbott
parent a7dc7c0734
commit 807a4428f6
10 changed files with 12 additions and 399 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
docs/tutorials/life-of-a-request.md
View File

@ -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

58
web/src/compose_validate.ts
View File

@ -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 {

44
web/tests/compose_validate.test.js
View File

@ -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 "<banner-stub>";
});
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 "<banner-stub>";
});
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}) => {

4
zerver/decorator.py
View File

@ -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__)

116
zerver/tests/test_decorators.py
View File

@ -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:

101
zerver/tests/test_subs.py
View File

@ -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(

1
zerver/tests/test_urls.py
View File

@ -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",

46
zerver/views/streams.py
View File

@ -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")

21
zproject/legacy_urls.py
View File

@ -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),
]

6
zproject/urls.py
View File

@ -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