From ca1aba9fc331a3f1874c3c4b2fa56ed4fc5da239 Mon Sep 17 00:00:00 2001 From: Shubham Padia Date: Tue, 28 Jan 2025 09:47:04 +0000 Subject: [PATCH] stream: Allow realm & channel admins to change private channel setting. Previously, realm and channel admins were not able to change settings for a private channel they were not subscribed to. This commit changes that. We have only added the exception for can_add_subscribers_group and not privacy settings. We also need proper functions with proper terminologies for content and metadata access. --- api_docs/changelog.md | 6 ++ zerver/lib/streams.py | 45 ++++++++++-- zerver/models/streams.py | 7 ++ zerver/openapi/zulip.yaml | 30 ++++---- zerver/tests/test_subs.py | 148 +++++++++++++++++++++++++++++++++++--- zerver/views/streams.py | 24 ++++--- 6 files changed, 222 insertions(+), 38 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 588a8a565a..cbaf908918 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -29,6 +29,12 @@ format used by the Zulip server that they are interacting with. administrators can now unsubscribe other users even if they are not an organization administrator or part of `can_remove_subscribers_group`. +* [`PATCH /streams/{stream_id}`](/api/update-stream), + [`DELETE /streams/{stream_id}`](/api/archive-stream): Channel and + organization administrators can modify all the settings requiring + only metadata access without having content access to it. They + cannot add subscribers to the channel or change it's privacy setting + without having content access to it. **Feature level 348** diff --git a/zerver/lib/streams.py b/zerver/lib/streams.py index 1dda236924..b43930954a 100644 --- a/zerver/lib/streams.py +++ b/zerver/lib/streams.py @@ -447,7 +447,32 @@ def check_for_exactly_one_stream_arg(stream_id: int | None, stream: str | None) raise IncompatibleParametersError(["stream_id", "stream"]) -def check_stream_access_for_delete_or_update( +def user_has_content_access( + user_profile: UserProfile, + stream: Stream, + *, + is_subscribed: bool, +) -> bool: + if stream.is_web_public: + return True + + if is_subscribed: + return True + + if not stream.invite_only and not user_profile.is_guest: + return True + + user_recursive_group_ids = set( + get_recursive_membership_groups(user_profile).values_list("id", flat=True) + ) + + if is_user_in_can_add_subscribers_group(stream, user_recursive_group_ids): + return True + + return False + + +def check_stream_access_for_delete_or_update_requiring_metadata_access( user_profile: UserProfile, stream: Stream, sub: Subscription | None = None ) -> None: error = _("Invalid channel ID") @@ -461,16 +486,22 @@ def check_stream_access_for_delete_or_update( if user_profile.is_realm_admin: return - if sub is None and stream.invite_only: - raise JsonableError(error) - if can_administer_accessible_channel(stream, user_profile): return - raise CannotAdministerChannelError + # We only want to reveal that user is not an administrator + # if the user has access to the channel in the first place. + # Ideally, we would be checking if user has metadata access + # to the channel for this block, but since we have ruled out + # the possibility that the user is a channel admin, checking + # for content access will save us valuable DB queries. + if user_has_content_access(user_profile, stream, is_subscribed=sub is not None): + raise CannotAdministerChannelError + + raise JsonableError(error) -def access_stream_for_delete_or_update( +def access_stream_for_delete_or_update_requiring_metadata_access( user_profile: UserProfile, stream_id: int ) -> tuple[Stream, Subscription | None]: try: @@ -485,7 +516,7 @@ def access_stream_for_delete_or_update( except Subscription.DoesNotExist: sub = None - check_stream_access_for_delete_or_update(user_profile, stream, sub) + check_stream_access_for_delete_or_update_requiring_metadata_access(user_profile, stream, sub) return (stream, sub) diff --git a/zerver/models/streams.py b/zerver/models/streams.py index 4fe51e3a93..2a2e626caf 100644 --- a/zerver/models/streams.py +++ b/zerver/models/streams.py @@ -177,6 +177,13 @@ class Stream(models.Model): ), } + stream_permission_group_settings_requiring_content_access = [ + "can_add_subscribers_group", + ] + assert set(stream_permission_group_settings_requiring_content_access).issubset( + stream_permission_group_settings.keys() + ) + class Meta: indexes = [ models.Index(Upper("name"), name="upper_stream_name_idx"), diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 16724bef74..aab11cf8e5 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -20486,12 +20486,16 @@ paths: Administrators can always administer a channel. - Note that a user must also [have access](/help/channel-permissions) to a - channel in order to modify it. The exception to this rule is that - organization administrators can edit channel names and descriptions - without having full access to the channel. + Note that a user must [have access](/help/channel-permissions) to a + channel in order to add other subscribers to the channel. - **Changes**: New in Zulip 10.0 (feature level 325). Prior to this + **Changes**: Prior to Zulip 10.0 (feature level 349) a user needed to + [have content access](/help/channel-permissions) to a channel in order + to modify it. The exception to this rule was that organization + administrators can edit channel names and descriptions without having + full access to the channel. + + New in Zulip 10.0 (feature level 325). Prior to this change, the permission to administer channels was limited to realm administrators. example: @@ -25195,16 +25199,16 @@ components: Administrators can always administer a channel. - Note that a user must also [have access](/help/channel-permissions) to a - channel in order to modify it. + Note that a user must [have access](/help/channel-permissions) to a + channel in order to add other subscribers to the channel. - Users can edit channel name and description without subscribing to the - channel, but they need to be subscribed to edit channel permissions and - add users. The exception to this rule is that organization administrators - can edit channel names and descriptions without having full access to - the channel. + **Changes**: Prior to Zulip 10.0 (feature level 349) a user needed to + [have content access](/help/channel-permissions) to a channel in + order to modify it. The exception to this rule was that organization + administrators can edit channel names and descriptions without + having full access to the channel. - **Changes**: New in Zulip 10.0 (feature level 325). Prior to this + New in Zulip 10.0 (feature level 325). Prior to this change, the permission to administer channels was limited to realm administrators. diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 2cf23f0d69..a042952eb1 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -82,6 +82,7 @@ from zerver.lib.streams import ( ensure_stream, filter_stream_authorization, list_to_streams, + user_has_content_access, ) from zerver.lib.subscription_info import ( bulk_get_subscriber_user_ids, @@ -2125,12 +2126,18 @@ class StreamAdminTest(ZulipTestCase): def test_non_admin_cannot_access_unsub_private_stream(self) -> None: iago = self.example_user("iago") hamlet = self.example_user("hamlet") + nobody_group = NamedUserGroup.objects.get( + name="role:nobody", is_system_group=True, realm=hamlet.realm + ) self.login_user(hamlet) result = self.subscribe_via_post( hamlet, ["private_stream_1"], - dict(principals=orjson.dumps([iago.id]).decode()), + dict( + principals=orjson.dumps([iago.id]).decode(), + can_administer_channel_group=nobody_group.id, + ), invite_only=True, ) self.assert_json_success(result) @@ -2669,26 +2676,81 @@ class StreamAdminTest(ZulipTestCase): f"'{setting_name}' setting cannot be set to 'role:internet' group.", ) - # For private streams, even admins must be subscribed to the - # stream to change the setting. + # For private streams, realm admins need not be subscribed to + # the stream to change the setting as they can administer the + # channel by default. stream = get_stream("stream_name2", realm) params[setting_name] = orjson.dumps({"new": moderators_system_group.id}).decode() result = self.client_patch( f"/json/streams/{stream.id}", params, ) - self.assert_json_error(result, "Invalid channel ID") + if setting_name in Stream.stream_permission_group_settings_requiring_content_access: + self.assert_json_error(result, "Invalid channel ID") + else: + self.assert_json_success(result) + stream = get_stream("stream_name2", realm) + self.assertEqual(getattr(stream, setting_name).id, moderators_system_group.id) - self.subscribe(user_profile, "stream_name2") + # For private streams, channel admins need not be subscribed to + # the stream to change the setting as they can administer the + # channel by default. + shiva_group = self.create_or_update_anonymous_group_for_setting([shiva], []) + do_change_stream_group_based_setting( + stream, + "can_administer_channel_group", + shiva_group, + acting_user=None, + ) + self.assertTrue(is_user_in_group(stream.can_administer_channel_group, shiva)) + params[setting_name] = orjson.dumps({"new": owners_group.id}).decode() + self.login_user(shiva) result = self.client_patch( f"/json/streams/{stream.id}", params, ) - self.assert_json_success(result) - stream = get_stream("stream_name2", realm) - self.assertEqual(getattr(stream, setting_name).id, moderators_system_group.id) - # Unsubscribe user from private stream to test next setting. - self.unsubscribe(user_profile, "stream_name2") + if setting_name in Stream.stream_permission_group_settings_requiring_content_access: + self.assert_json_error(result, "Invalid channel ID") + shiva_group = self.create_or_update_anonymous_group_for_setting([shiva], []) + do_change_stream_group_based_setting( + stream, + "can_add_subscribers_group", + shiva_group, + acting_user=None, + ) + result = self.client_patch( + f"/json/streams/{stream.id}", + params, + ) + self.assert_json_success(result) + stream = get_stream("stream_name2", realm) + self.assertEqual(getattr(stream, setting_name).id, owners_group.id) + else: + self.assert_json_success(result) + stream = get_stream("stream_name2", realm) + self.assertEqual(getattr(stream, setting_name).id, owners_group.id) + + # Guest user cannot be a channel admin for a public channel. + # `user_has_permission_for_group_setting` will not allow a guest + # to be a part of `can_administer_channel_group` since that + # group has `allow_everyone_group` set to false. + stream = get_stream("stream_name1", realm) + polonius = self.example_user("polonius") + polonius_group = self.create_or_update_anonymous_group_for_setting([polonius], []) + do_change_stream_group_based_setting( + stream, + "can_administer_channel_group", + polonius_group, + acting_user=None, + ) + subbed_users = self.users_subscribed_to_stream(stream.name, polonius.realm) + self.assertNotIn(polonius, subbed_users) + self.login_user(polonius) + result = self.client_patch( + f"/json/streams/{stream.id}", + params, + ) + self.assert_json_error(result, "Invalid channel ID") def test_changing_stream_permission_settings(self) -> None: self.make_stream("stream_name1") @@ -7602,6 +7664,72 @@ class AccessStreamTest(ZulipTestCase): assert sub_ret is None self.assertEqual(stream.id, stream_ret.id) + def test_has_content_access(self) -> None: + guest_user = self.example_user("polonius") + aaron = self.example_user("aaron") + realm = guest_user.realm + web_public_stream = self.make_stream("web_public_stream", realm=realm, is_web_public=True) + private_stream = self.make_stream("private_stream", realm=realm, invite_only=True) + public_stream = self.make_stream("public_stream", realm=realm, invite_only=False) + + # Even guest user should have access to web public channel. + self.assertEqual( + user_has_content_access(guest_user, web_public_stream, is_subscribed=False), True + ) + + # User should have access to private channel if they are + # subscribed to it + self.assertEqual(user_has_content_access(aaron, private_stream, is_subscribed=True), True) + self.assertEqual(user_has_content_access(aaron, private_stream, is_subscribed=False), False) + + # Non guest user should have access to public channel + # regardless of their subscription to the channel. + self.assertEqual(user_has_content_access(aaron, public_stream, is_subscribed=True), True) + self.assertEqual(user_has_content_access(aaron, public_stream, is_subscribed=False), True) + + # Guest user should have access to public channel only if they + # are subscribed to it. + self.assertEqual( + user_has_content_access(guest_user, public_stream, is_subscribed=False), False + ) + self.assertEqual( + user_has_content_access(guest_user, public_stream, is_subscribed=True), True + ) + + # User should be able to access private channel if they are + # part of `can_add_subscribers_group` but not subscribed to the + # channel. + aaron_group = self.create_or_update_anonymous_group_for_setting([aaron], []) + do_change_stream_group_based_setting( + private_stream, + "can_add_subscribers_group", + aaron_group, + acting_user=None, + ) + self.assertEqual(user_has_content_access(aaron, private_stream, is_subscribed=False), True) + nobody_group = NamedUserGroup.objects.get( + name="role:nobody", realm=realm, is_system_group=True + ) + do_change_stream_group_based_setting( + private_stream, + "can_add_subscribers_group", + nobody_group, + acting_user=None, + ) + + # User should not be able to access private channel if they are + # part of `can_administer_channel_group` but not subscribed to + # the channel. + aaron_group = self.create_or_update_anonymous_group_for_setting([aaron], []) + do_change_stream_group_based_setting( + private_stream, + "can_administer_channel_group", + aaron_group, + acting_user=None, + ) + self.assertEqual(user_has_content_access(aaron, private_stream, is_subscribed=False), False) + self.assertEqual(user_has_content_access(aaron, private_stream, is_subscribed=True), True) + class StreamTrafficTest(ZulipTestCase): def test_average_weekly_stream_traffic_calculation(self) -> None: diff --git a/zerver/views/streams.py b/zerver/views/streams.py index f1b46db6c5..e3fa3da482 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -65,7 +65,7 @@ from zerver.lib.streams import ( access_default_stream_group_by_id, access_stream_by_id, access_stream_by_name, - access_stream_for_delete_or_update, + access_stream_for_delete_or_update_requiring_metadata_access, access_web_public_stream, check_stream_name_available, do_get_streams, @@ -75,6 +75,7 @@ from zerver.lib.streams import ( get_stream_permission_policy_name, list_to_streams, stream_to_dict, + user_has_content_access, ) from zerver.lib.subscription_info import gather_subscriptions from zerver.lib.topic import ( @@ -142,7 +143,9 @@ def user_directly_controls_user(user_profile: UserProfile, target: UserProfile) def deactivate_stream_backend( request: HttpRequest, user_profile: UserProfile, stream_id: int ) -> HttpResponse: - (stream, sub) = access_stream_for_delete_or_update(user_profile, stream_id) + (stream, sub) = access_stream_for_delete_or_update_requiring_metadata_access( + user_profile, stream_id + ) do_deactivate_stream(stream, acting_user=user_profile) return json_success(request) @@ -266,9 +269,12 @@ def update_stream_backend( can_send_message_group: Json[GroupSettingChangeRequest] | None = None, can_remove_subscribers_group: Json[GroupSettingChangeRequest] | None = None, ) -> HttpResponse: - # We allow realm administrators to update the stream name and - # description even for private streams. - (stream, sub) = access_stream_for_delete_or_update(user_profile, stream_id) + # Most settings updates only require metadata access, not content + # access. We will check for content access further when and where + # required. + (stream, sub) = access_stream_for_delete_or_update_requiring_metadata_access( + user_profile, stream_id + ) # Validate that the proposed state for permissions settings is permitted. if is_private is not None: @@ -325,7 +331,7 @@ def update_stream_backend( raise JsonableError(_("Moderation request channel must be private.")) if is_private is not None: - # We require even realm administrators to be actually + # We require even channel administrators to be actually # subscribed to make a private stream public, via this # stricted access_stream check. access_stream_by_id(user_profile, stream_id) @@ -405,8 +411,10 @@ def update_stream_backend( if validate_group_setting_value_change( current_setting_api_value, new_setting_value, expected_current_setting_value ): - if sub is None and stream.invite_only: - # Admins cannot change this setting for unsubscribed private streams. + if ( + setting_name in Stream.stream_permission_group_settings_requiring_content_access + and not user_has_content_access(user_profile, stream, is_subscribed=sub is not None) + ): raise JsonableError(_("Invalid channel ID")) with transaction.atomic(durable=True): user_group = access_user_group_for_setting(