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

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

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
 
+    # 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)
 
 

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

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.
 

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,
         )
+        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,
+        )
+        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, moderators_system_group.id)
-        # Unsubscribe user from private stream to test next setting.
-        self.unsubscribe(user_profile, "stream_name2")
+            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:

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(