diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index ef22b993ce..386e3facbe 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -976,13 +976,15 @@ def create_stream_if_needed(realm, stream_name, invite_only=False): send_event(event, active_user_ids(realm)) return stream, created -def create_streams_if_needed(realm, stream_names, invite_only): - # type: (Realm, List[text_type], bool) -> Tuple[List[Stream], List[Stream]] +def create_streams_if_needed(realm, stream_dicts, invite_only): + # type: (Realm, List[Mapping[str, text_type]], bool) -> Tuple[List[Stream], List[Stream]] + """Note that stream_dict["name"] is assumed to already be stripped of + whitespace""" added_streams = [] # type: List[Stream] existing_streams = [] # type: List[Stream] - for stream_name in stream_names: + for stream_dict in stream_dicts: stream, created = create_stream_if_needed(realm, - stream_name, + stream_dict["name"], invite_only=invite_only) if created: added_streams.append(stream) diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index d1bc49f281..32d3a4d805 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -63,7 +63,7 @@ class TestCreateStreams(ZulipTestCase): new_streams, existing_streams = create_streams_if_needed( realm, - stream_names, + [{"name": stream_name} for stream_name in stream_names], invite_only=True) self.assertEqual(len(new_streams), 3) self.assertEqual(len(existing_streams), 0) @@ -73,7 +73,7 @@ class TestCreateStreams(ZulipTestCase): new_streams, existing_streams = create_streams_if_needed( realm, - stream_names, + [{"name": stream_name} for stream_name in stream_names], invite_only=True) self.assertEqual(len(new_streams), 0) self.assertEqual(len(existing_streams), 3) diff --git a/zerver/views/streams.py b/zerver/views/streams.py index 7e9271f304..7fecff1ccc 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -38,8 +38,8 @@ def is_active_subscriber(user_profile, recipient): active=True).exists() def list_to_streams(streams_raw, user_profile, autocreate=False, invite_only=False): - # type: (Iterable[text_type], UserProfile, Optional[bool], Optional[bool]) -> Tuple[List[Stream], List[Stream]] - """Converts plaintext stream names to a list of Streams, validating input in the process + # type: (Iterable[Mapping[str, text_type]], UserProfile, Optional[bool], Optional[bool]) -> Tuple[List[Stream], List[Stream]] + """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 @@ -49,33 +49,39 @@ def list_to_streams(streams_raw, user_profile, autocreate=False, invite_only=Fal 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. - @param streams_raw The list of stream names to process + @param streams_raw The list of stream names to process; should + already be stripped of whitespace by the caller. @param user_profile The user for whom we are retreiving the streams @param autocreate Whether we should create streams if they don't already exist @param invite_only Whether newly created streams should have the invite_only bit set + """ # Validate all streams, getting extant ones, then get-or-creating the rest. - stream_set = set(stream_name.strip() for stream_name in streams_raw) + stream_set = set(stream_dict["name"] for stream_dict in streams_raw) for stream_name in stream_set: + # 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,)) existing_streams = [] # type: List[Stream] - missing_stream_names = [] # type: List[text_type] + missing_stream_dicts = [] # type: List[Mapping[str, text_type]] existing_stream_map = bulk_get_streams(user_profile.realm, stream_set) - for stream_name in stream_set: + for stream_dict in streams_raw: + stream_name = stream_dict["name"] stream = existing_stream_map.get(stream_name.lower()) if stream is None: - missing_stream_names.append(stream_name) + missing_stream_dicts.append(stream_dict) else: existing_streams.append(stream) - if not missing_stream_names: + if len(missing_stream_dicts) == 0: # This is the happy path for callers who expected all of these # streams to exist already. created_streams = [] # type: List[Stream] @@ -84,7 +90,8 @@ def list_to_streams(streams_raw, user_profile, autocreate=False, invite_only=Fal if not user_profile.can_create_streams(): raise JsonableError(_('User cannot create streams.')) elif not autocreate: - raise JsonableError(_("Stream(s) (%s) do not exist") % ", ".join(missing_stream_names)) + raise JsonableError(_("Stream(s) (%s) do not exist") % ", ".join( + stream_dict["name"] for stream_dict in missing_stream_dicts)) # We already filtered out existing streams, so dup_streams # will normally be an empty list below, but we protect against somebody @@ -92,7 +99,7 @@ def list_to_streams(streams_raw, user_profile, autocreate=False, invite_only=Fal # paranoid approach, since often on Zulip two people will discuss # creating a new stream, and both people eagerly do it.) created_streams, dup_streams = create_streams_if_needed(realm=user_profile.realm, - stream_names=missing_stream_names, + stream_dicts=missing_stream_dicts, invite_only=invite_only) existing_streams += dup_streams @@ -241,7 +248,11 @@ def remove_subscriptions_backend(request, user_profile, # admin. return json_error(_("This action requires administrative rights")) - streams, __ = list_to_streams(streams_raw, user_profile) + streams_as_dict = [] + for stream_name in streams_raw: + streams_as_dict.append({"name": stream_name.strip()}) + + streams, __ = list_to_streams(streams_as_dict, user_profile) for stream in streams: if removing_someone_else and stream.invite_only and \ @@ -311,18 +322,20 @@ def add_subscriptions_backend(request, user_profile, principals = REQ(validator=check_list(check_string), default=None), authorization_errors_fatal = REQ(validator=check_bool, default=True)): # type: (HttpRequest, UserProfile, Iterable[Mapping[str, text_type]], bool, bool, Optional[List[text_type]], bool) -> HttpResponse - stream_names = [] + stream_dicts = [] for stream_dict in streams_raw: - stream_name = stream_dict["name"].strip() - if len(stream_name) > Stream.MAX_NAME_LENGTH: - return json_error(_("Stream name (%s) too long.") % (stream_name,)) - if not valid_stream_name(stream_name): - return json_error(_("Invalid stream name (%s).") % (stream_name,)) - stream_names.append(stream_name) + stream_dict_copy = {} # type: Dict[str, Any] + for field in stream_dict: + stream_dict_copy[field] = stream_dict[field] + # Strip the stream name here. + stream_dict_copy['name'] = stream_dict_copy['name'].strip() + stream_dicts.append(stream_dict_copy) - # Enforcement of can_create_streams policy is inside list_to_streams. + # Validation of the streams arguments, including enforcement of + # can_create_streams policy and valid_stream_name policy is inside + # list_to_streams. existing_streams, created_streams = \ - list_to_streams(stream_names, user_profile, autocreate=True, invite_only=invite_only) + list_to_streams(stream_dicts, user_profile, autocreate=True, invite_only=invite_only) authorized_streams, unauthorized_streams = \ filter_stream_authorization(user_profile, existing_streams) if len(unauthorized_streams) > 0 and authorization_errors_fatal: