From 1bbf0f9a98a92f6149bb17c49ef2493fdf5a2a49 Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Sun, 29 Jan 2017 22:01:19 -0800 Subject: [PATCH] streams: Consider stream name validation logic. --- zerver/lib/actions.py | 13 ++++--------- zerver/lib/streams.py | 11 +++-------- zerver/models.py | 4 ---- zerver/tests/test_subs.py | 12 ++++++------ zerver/views/streams.py | 11 +++++------ 5 files changed, 18 insertions(+), 33 deletions(-) diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 5308e1e3b8..e4930a4f1f 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -26,7 +26,7 @@ from zerver.lib.message import ( render_markdown, ) from zerver.models import Realm, RealmEmoji, Stream, UserProfile, UserActivity, RealmAlias, \ - Subscription, Recipient, Message, Attachment, UserMessage, valid_stream_name, \ + Subscription, Recipient, Message, Attachment, UserMessage, \ Client, DefaultStream, UserPresence, Referral, PushDeviceToken, MAX_SUBJECT_LENGTH, \ MAX_MESSAGE_LENGTH, get_client, get_stream, get_recipient, get_huddle, \ get_user_profile_by_id, PreregistrationUser, get_display_recipient, \ @@ -1186,12 +1186,10 @@ def check_send_message(sender, client, message_type_name, message_to, def check_stream_name(stream_name): # type: (Text) -> None - if stream_name == "": - raise JsonableError(_("Stream can't be empty")) + if stream_name.strip() == "": + raise JsonableError(_("Invalid stream name '%s'" % (stream_name))) if len(stream_name) > Stream.MAX_NAME_LENGTH: - raise JsonableError(_("Stream name too long")) - if not valid_stream_name(stream_name): - raise JsonableError(_("Invalid stream name")) + raise JsonableError(_("Stream name too long (limit: %s characters)" % (Stream.MAX_NAME_LENGTH))) def send_pm_if_empty_stream(sender, stream, stream_name, realm): # type: (UserProfile, Stream, Text, Realm) -> None @@ -1274,9 +1272,6 @@ def check_message(sender, client, message_type_name, message_to, if subject == "": raise JsonableError(_("Topic can't be empty")) subject = truncate_topic(subject) - ## FIXME: Commented out temporarily while we figure out what we want - # if not valid_stream_name(subject): - # return json_error(_("Invalid subject name")) stream = get_stream(stream_name, realm) diff --git a/zerver/lib/streams.py b/zerver/lib/streams.py index 62711f0edc..bc9a55af7e 100644 --- a/zerver/lib/streams.py +++ b/zerver/lib/streams.py @@ -9,7 +9,7 @@ from zerver.lib.actions import check_stream_name, create_streams_if_needed from zerver.lib.request import JsonableError from zerver.models import UserProfile, Stream, Subscription, \ Realm, Recipient, bulk_get_recipients, get_recipient, get_stream, \ - bulk_get_streams, valid_stream_name + bulk_get_streams def access_stream_common(user_profile, stream, error): # type: (UserProfile, Stream, Text) -> Tuple[Recipient, Subscription] @@ -101,9 +101,7 @@ def list_to_streams(streams_raw, user_profile, autocreate=False): """Converts list of dicts to a list of Streams, validating input in the process For each stream name, we validate it to ensure it meets our - requirements for a proper stream name: that is, that it is shorter - than Stream.MAX_NAME_LENGTH characters and passes - valid_stream_name. + requirements for a proper stream name using check_stream_name. This function in autocreate mode should be atomic: either an exception will be raised during a precheck, or all the streams specified will have been created if applicable. @@ -121,10 +119,7 @@ def list_to_streams(streams_raw, user_profile, autocreate=False): # Stream names should already have been stripped by the # caller, but it makes sense to verify anyway. assert stream_name == stream_name.strip() - if len(stream_name) > Stream.MAX_NAME_LENGTH: - raise JsonableError(_("Stream name (%s) too long.") % (stream_name,)) - if not valid_stream_name(stream_name): - raise JsonableError(_("Invalid stream name (%s).") % (stream_name,)) + check_stream_name(stream_name) existing_streams = [] # type: List[Stream] missing_stream_dicts = [] # type: List[Mapping[str, Any]] diff --git a/zerver/models.py b/zerver/models.py index e37c5ba503..540b7c3d61 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -725,10 +725,6 @@ class Stream(ModelReprMixin, models.Model): post_save.connect(flush_stream, sender=Stream) post_delete.connect(flush_stream, sender=Stream) -def valid_stream_name(name): - # type: (Text) -> bool - return name != "" - # The Recipient table is used to map Messages to the set of users who # received the message. It is implemented as a set of triples (id, # type_id, type). We have 3 types of recipients: Huddles (for group diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 749b653905..23bf41d5e2 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -43,7 +43,7 @@ from zerver.lib.actions import ( do_create_realm, do_remove_default_stream, do_set_realm_create_stream_by_admins_only, gather_subscriptions_helper, bulk_add_subscriptions, bulk_remove_subscriptions, gather_subscriptions, get_default_streams_for_realm, get_realm, get_stream, - get_user_profile_by_email, set_default_streams, + get_user_profile_by_email, set_default_streams, check_stream_name, create_stream_if_needed, create_streams_if_needed, active_user_ids ) @@ -1078,7 +1078,7 @@ class SubscriptionRestApiTest(ZulipTestCase): **self.api_auth(email) ) self.assert_json_error(result, - "Invalid stream name (%s)." % (invalid_stream_name,)) + "Invalid stream name '%s'" % (invalid_stream_name,)) def test_stream_name_too_long(self): # type: () -> None @@ -1095,7 +1095,7 @@ class SubscriptionRestApiTest(ZulipTestCase): **self.api_auth(email) ) self.assert_json_error(result, - "Stream name (%s) too long." % (long_stream_name,)) + "Stream name too long (limit: 60 characters)") def test_compose_views_rollback(self): # type: () -> None @@ -1403,7 +1403,7 @@ class SubscriptionAPITest(ZulipTestCase): long_stream_name = "a" * 61 result = self.common_subscribe_to_streams(self.test_email, [long_stream_name]) self.assert_json_error(result, - "Stream name (%s) too long." % (long_stream_name,)) + "Stream name too long (limit: 60 characters)") def test_user_settings_for_adding_streams(self): # type: () -> None @@ -1431,7 +1431,7 @@ class SubscriptionAPITest(ZulipTestCase): invalid_stream_name = "" result = self.common_subscribe_to_streams(self.test_email, [invalid_stream_name]) self.assert_json_error(result, - "Invalid stream name (%s)." % (invalid_stream_name,)) + "Invalid stream name '%s'" % (invalid_stream_name,)) def assert_adding_subscriptions_for_principal(self, invitee, streams, invite_only=False): # type: (Text, List[Text], bool) -> None @@ -1935,7 +1935,7 @@ class SubscriptionAPITest(ZulipTestCase): invalid_stream_name = "" result = self.client_post("/json/subscriptions/exists", {"stream": invalid_stream_name}) - self.assert_json_error(result, "Invalid characters in stream name") + self.assert_json_error(result, "Invalid stream name ''") def test_existing_subscriptions_autosubscription(self): # type: () -> None diff --git a/zerver/views/streams.py b/zerver/views/streams.py index 68752614b7..99c31474e3 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -19,12 +19,12 @@ from zerver.lib.actions import bulk_remove_subscriptions, \ do_remove_default_stream, get_topic_history_for_stream from zerver.lib.response import json_success, json_error, json_response from zerver.lib.streams import access_stream_by_id, access_stream_by_name, \ - check_stream_name_available, filter_stream_authorization, list_to_streams + check_stream_name, check_stream_name_available, filter_stream_authorization, \ + list_to_streams from zerver.lib.validator import check_string, check_list, check_dict, \ check_bool, check_variable_type from zerver.models import UserProfile, Stream, Realm, Subscription, \ - Recipient, get_recipient, get_stream, valid_stream_name, \ - get_active_user_dicts_in_realm + Recipient, get_recipient, get_stream, get_active_user_dicts_in_realm from collections import defaultdict import ujson @@ -210,7 +210,7 @@ def add_subscriptions_backend(request, user_profile, stream_dicts.append(stream_dict_copy) # Validation of the streams arguments, including enforcement of - # can_create_streams policy and valid_stream_name policy is inside + # can_create_streams policy and check_stream_name policy is inside # list_to_streams. existing_streams, created_streams = \ list_to_streams(stream_dicts, user_profile, autocreate=True) @@ -353,8 +353,7 @@ def get_topics_backend(request, user_profile, def json_stream_exists(request, user_profile, stream_name=REQ("stream"), autosubscribe=REQ(validator=check_bool, default=False)): # type: (HttpRequest, UserProfile, Text, bool) -> HttpResponse - if not valid_stream_name(stream_name): - return json_error(_("Invalid characters in stream name")) + check_stream_name(stream_name) try: (stream, recipient, sub) = access_stream_by_name(user_profile, stream_name)