diff --git a/docs/production/security-model.md b/docs/production/security-model.md index 961f670a1f..97d5acc95a 100644 --- a/docs/production/security-model.md +++ b/docs/production/security-model.md @@ -101,14 +101,20 @@ strength allowed is controlled by two settings in without joining the stream. * A private ("invite-only") stream is hidden from users who are not - subscribed to the stream. Users who are not members of a private - stream cannot read messages on the stream, send messages to the - stream, or join the stream, even if they are a Zulip realm - administrator. Users can join private streams only when they are - invited. However, any member of a private stream can invite other - users to the stream. When a new user joins a private stream, they - can see future messages sent to the stream, but they do not receive - access to the stream's message history. + subscribed to the stream. + * Users who are not members of a private stream cannot read messages + on the stream, send messages to the stream, or join the stream, + even if they are a Zulip organization administrator. + * Any member of a private stream can add other users to the stream. + This is the only way that one can join a private stream (even + organization administrators cannot join a private stream without + being added by an existing member). + * When a new user joins a private stream, they can see future + messages sent to the stream, but they do not receive access to the + stream's message history. + * Organization administrators can do some basic management of + private streams that they are not subscribed to: Changing the + stream name and setting its description. * Zulip supports editing the content and topics of messages that have already been sent. As a general philosophy, our policies provide diff --git a/static/js/stream_data.js b/static/js/stream_data.js index e5374022aa..6408885817 100644 --- a/static/js/stream_data.js +++ b/static/js/stream_data.js @@ -194,10 +194,9 @@ exports.render_stream_description = function (sub) { exports.update_calculated_fields = function (sub) { sub.is_admin = page_params.is_admin; - // Admin can change stream name/description either stream is public or - // stream is private and admin is subscribed to private stream. - sub.can_change_name_description = page_params.is_admin && - (!sub.invite_only || (sub.invite_only && sub.subscribed)); + // Admin can change any stream's name & description either stream is public or + // private, subscribed or unsubscribed. + sub.can_change_name_description = page_params.is_admin; // If stream is public then any user can subscribe. If stream is private then only // subscribed users can unsubscribe. sub.should_display_subscription_button = !sub.invite_only || sub.subscribed; diff --git a/zerver/lib/streams.py b/zerver/lib/streams.py index 52681eace6..438aa514f3 100644 --- a/zerver/lib/streams.py +++ b/zerver/lib/streams.py @@ -10,11 +10,11 @@ from zerver.models import UserProfile, Stream, Subscription, \ Realm, Recipient, bulk_get_recipients, get_stream_recipient, get_stream, \ bulk_get_streams, get_realm_stream, DefaultStreamGroup -def access_stream_for_delete(user_profile: UserProfile, stream_id: int) -> Stream: +def access_stream_for_delete_or_update(user_profile: UserProfile, stream_id: int) -> Stream: # We should only ever use this for realm admins, who are allowed - # to delete all streams on their realm, even private streams to - # which they are not subscribed. We do an assert here, because + # to delete or update all streams on their realm, even private streams + # to which they are not subscribed. We do an assert here, because # all callers should have the require_realm_admin decorator. assert(user_profile.is_realm_admin) diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 320c9e2313..9856ef5058 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -401,6 +401,29 @@ class StreamAdminTest(ZulipTestCase): {'new_name': ujson.dumps('stream_name2')}) self.assert_json_error(result, 'Must be an organization administrator') + def test_realm_admin_can_update_unsub_private_stream(self) -> None: + iago = self.example_user('iago') + self.login(iago.email) + result = self.common_subscribe_to_streams(iago.email, ["private_stream"], + dict(principals=ujson.dumps([self.example_email("hamlet")])), + invite_only=True) + self.assert_json_success(result) + + stream_id = get_stream('private_stream', iago.realm).id + result = self.client_patch('/json/streams/%d' % (stream_id,), + {'new_name': ujson.dumps('new_private_stream')}) + self.assert_json_success(result) + + result = self.client_patch('/json/streams/%d' % (stream_id,), + {'new_description': ujson.dumps('new description')}) + self.assert_json_success(result) + + # But can not change stream type. + result = self.client_patch('/json/streams/%d' % (stream_id,), + {'stream_name': ujson.dumps('private_stream'), + 'is_private': ujson.dumps(True)}) + self.assert_json_error(result, "Invalid stream id") + def test_change_stream_description(self) -> None: user_profile = self.example_user('hamlet') email = user_profile.email diff --git a/zerver/views/streams.py b/zerver/views/streams.py index 734eee3982..ee2af13bb9 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -25,7 +25,7 @@ from zerver.lib.actions import bulk_remove_subscriptions, \ 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, check_stream_name_available, filter_stream_authorization, \ - list_to_streams, access_stream_for_delete, access_default_stream_group_by_id + list_to_streams, access_stream_for_delete_or_update, access_default_stream_group_by_id from zerver.lib.validator import check_string, check_int, check_list, check_dict, \ check_bool, check_variable_type from zerver.models import UserProfile, Stream, Realm, Subscription, \ @@ -61,7 +61,7 @@ def principal_to_user_profile(agent: UserProfile, principal: Text) -> UserProfil def deactivate_stream_backend(request: HttpRequest, user_profile: UserProfile, stream_id: int) -> HttpResponse: - stream = access_stream_for_delete(user_profile, stream_id) + stream = access_stream_for_delete_or_update(user_profile, stream_id) do_deactivate_stream(stream) return json_success() @@ -148,8 +148,9 @@ def update_stream_backend( is_private: Optional[bool]=REQ(validator=check_bool, default=None), new_name: Optional[Text]=REQ(validator=check_string, default=None), ) -> HttpResponse: - (stream, recipient, sub) = access_stream_by_id(user_profile, stream_id) - + # We allow realm administrators to to update the stream name and + # description even for private streams. + stream = access_stream_for_delete_or_update(user_profile, stream_id) if description is not None: do_change_stream_description(stream, description) if new_name is not None: @@ -161,7 +162,11 @@ def update_stream_backend( # are only changing the casing of the stream name). check_stream_name_available(user_profile.realm, new_name) do_rename_stream(stream, new_name) + + # But we require even realm administrators to be actually + # subscribed to make a private stream public. if is_private is not None: + (stream, recipient, sub) = access_stream_by_id(user_profile, stream_id) do_change_stream_invite_only(stream, is_private) return json_success()