streams: Make sure that unused anonymous groups are not created.

Anonymous groups were being created for stream permission
settings when calling the subscriptions endpoint without
any streams data or when calling it only for subscribing
users to streams and not for creating any new streams.

This commit makes sure that no such unused anonymous groups
are created.

(cherry picked from commit d4d7a8fe2a)

zerver/lib/streams.py

@@ -2,7 +2,7 @@ from collections import defaultdict
 from collections.abc import Collection, Iterable
 from dataclasses import dataclass
 from datetime import datetime, timedelta
-from typing import TypedDict
+from typing import Any, TypedDict
 
 from django.db import transaction
 from django.db.models import Exists, OuterRef, Q, QuerySet
@@ -30,11 +30,13 @@ from zerver.lib.topic import get_topic_display_name
 from zerver.lib.types import APIStreamDict, UserGroupMembersData
 from zerver.lib.user_groups import (
     UserGroupMembershipDetails,
+    access_user_group_for_setting,
     get_group_setting_value_for_register_api,
     get_members_and_subgroups_of_groups,
     get_recursive_membership_groups,
     get_role_based_system_groups_dict,
     get_root_id_annotated_recursive_subgroups_for_groups,
+    parse_group_setting_value,
     user_has_permission_for_group_setting,
 )
 from zerver.models import (
@@ -1511,13 +1513,57 @@ def filter_stream_authorization_for_adding_subscribers(
     )
 
 
+def access_requested_group_permissions(
+    user_profile: UserProfile,
+    realm: Realm,
+    request_settings_dict: dict[str, Any],
+) -> tuple[dict[str, UserGroup], dict[int, UserGroupMembersData]]:
+    anonymous_group_membership = {}
+    group_settings_map = {}
+    system_groups_name_dict = get_role_based_system_groups_dict(realm)
+    for setting_name, permission_configuration in Stream.stream_permission_group_settings.items():
+        assert setting_name in request_settings_dict
+        if request_settings_dict[setting_name] is not None:
+            setting_request_value = request_settings_dict[setting_name]
+            setting_value = parse_group_setting_value(
+                setting_request_value, system_groups_name_dict[SystemGroups.NOBODY]
+            )
+            group_settings_map[setting_name] = access_user_group_for_setting(
+                setting_value,
+                user_profile,
+                setting_name=setting_name,
+                permission_configuration=permission_configuration,
+            )
+            if (
+                setting_name in ["can_delete_any_message_group", "can_delete_own_message_group"]
+                and group_settings_map[setting_name].id
+                != system_groups_name_dict[SystemGroups.NOBODY].id
+                and not user_profile.can_set_delete_message_policy()
+            ):
+                raise JsonableError(_("Insufficient permission"))
+
+            if not isinstance(setting_value, int):
+                anonymous_group_membership[group_settings_map[setting_name].id] = setting_value
+        else:
+            group_settings_map[setting_name] = get_stream_permission_default_group(
+                setting_name, system_groups_name_dict, creator=user_profile
+            )
+            if permission_configuration.default_group_name == "stream_creator_or_nobody":
+                # Default for some settings like "can_administer_channel_group"
+                # is anonymous group with stream creator.
+                anonymous_group_membership[group_settings_map[setting_name].id] = (
+                    UserGroupMembersData(direct_subgroups=[], direct_members=[user_profile.id])
+                )
+    return group_settings_map, anonymous_group_membership
+
+
 def list_to_streams(
     streams_raw: Collection[StreamDict],
     user_profile: UserProfile,
     autocreate: bool = False,
     unsubscribing_others: bool = False,
     is_default_stream: bool = False,
-    anonymous_group_membership: dict[int, UserGroupMembersData] | None = None,
+    request_settings_dict: dict[str, Any] | None = None,
 ) -> tuple[list[Stream], list[Stream]]:
     """Converts list of dicts to a list of Streams, validating input in the process
 
@@ -1572,6 +1618,14 @@ def list_to_streams(
                     ),
                 )
             )
+
+        assert request_settings_dict is not None
+        group_settings_map, anonymous_group_membership = access_requested_group_permissions(
+            user_profile,
+            user_profile.realm,
+            request_settings_dict,
+        )
+
         # autocreate=True path starts here
         for stream_dict in missing_stream_dicts:
             check_channel_creation_permissions(
@@ -1582,6 +1636,31 @@ def list_to_streams(
                 message_retention_days=stream_dict.get("message_retention_days", None),
             )
 
+            stream_dict["can_add_subscribers_group"] = group_settings_map[
+                "can_add_subscribers_group"
+            ]
+            stream_dict["can_administer_channel_group"] = group_settings_map[
+                "can_administer_channel_group"
+            ]
+            stream_dict["can_delete_any_message_group"] = group_settings_map[
+                "can_delete_any_message_group"
+            ]
+            stream_dict["can_delete_own_message_group"] = group_settings_map[
+                "can_delete_own_message_group"
+            ]
+            stream_dict["can_move_messages_out_of_channel_group"] = group_settings_map[
+                "can_move_messages_out_of_channel_group"
+            ]
+            stream_dict["can_move_messages_within_channel_group"] = group_settings_map[
+                "can_move_messages_within_channel_group"
+            ]
+            stream_dict["can_send_message_group"] = group_settings_map["can_send_message_group"]
+            stream_dict["can_remove_subscribers_group"] = group_settings_map[
+                "can_remove_subscribers_group"
+            ]
+            stream_dict["can_resolve_topics_group"] = group_settings_map["can_resolve_topics_group"]
+            stream_dict["can_subscribe_group"] = group_settings_map["can_subscribe_group"]
+
         # We already filtered out existing streams, so dup_streams
         # will normally be an empty list below, but we protect against somebody
         # else racing to create the same stream.  (This is not an entirely

zerver/tests/test_channel_creation.py

@@ -1095,8 +1095,13 @@ class TestCreateStreams(ZulipTestCase):
                 "is_web_public": False,
             }
         ]
+
+        request_settings_dict = dict.fromkeys(Stream.stream_permission_group_settings)
+
         with self.assertRaisesRegex(JsonableError, "Must be an organization owner"):
-            list_to_streams(streams_raw, admin, autocreate=True)
+            list_to_streams(
+                streams_raw, admin, autocreate=True, request_settings_dict=request_settings_dict
+            )
 
         streams_raw = [
             {
@@ -1106,7 +1111,9 @@ class TestCreateStreams(ZulipTestCase):
             }
         ]
         with self.assertRaisesRegex(JsonableError, "Must be an organization owner"):
-            list_to_streams(streams_raw, admin, autocreate=True)
+            list_to_streams(
+                streams_raw, admin, autocreate=True, request_settings_dict=request_settings_dict
+            )
 
         streams_raw = [
             {
@@ -1115,7 +1122,9 @@ class TestCreateStreams(ZulipTestCase):
                 "is_web_public": False,
             }
         ]
-        result = list_to_streams(streams_raw, admin, autocreate=True)
+        result = list_to_streams(
+            streams_raw, admin, autocreate=True, request_settings_dict=request_settings_dict
+        )
         self.assert_length(result[0], 0)
         self.assert_length(result[1], 1)
         self.assertEqual(result[1][0].name, "new_stream")
@@ -1144,10 +1153,14 @@ class TestCreateStreams(ZulipTestCase):
         with self.assertRaisesRegex(
             JsonableError, "Available on Zulip Cloud Standard. Upgrade to access."
         ):
-            list_to_streams(streams_raw, owner, autocreate=True)
+            list_to_streams(
+                streams_raw, owner, autocreate=True, request_settings_dict=request_settings_dict
+            )
 
         do_change_realm_plan_type(realm, Realm.PLAN_TYPE_SELF_HOSTED, acting_user=admin)
-        result = list_to_streams(streams_raw, owner, autocreate=True)
+        result = list_to_streams(
+            streams_raw, owner, autocreate=True, request_settings_dict=request_settings_dict
+        )
         self.assert_length(result[0], 0)
         self.assert_length(result[1], 3)
         self.assertEqual(result[1][0].name, "new_stream1")

zerver/tests/test_channel_fetch.py

@@ -839,7 +839,7 @@ class GetSubscribersTest(ZulipTestCase):
             polonius.id,
         ]
 
-        with self.assert_database_query_count(55):
+        with self.assert_database_query_count(52):
             self.subscribe_via_post(
                 self.user_profile,
                 stream_names,

zerver/tests/test_subs.py

@@ -427,6 +427,8 @@ class StreamAdminTest(ZulipTestCase):
             )
         ]
 
+        request_settings_dict = dict.fromkeys(Stream.stream_permission_group_settings)
+
         self.assertFalse(user_profile.can_create_web_public_streams())
         self.assertTrue(owner.can_create_web_public_streams())
         # As per can_create_web_public_channel_group, only owners
@@ -436,6 +438,7 @@ class StreamAdminTest(ZulipTestCase):
                 streams_raw,
                 user_profile,
                 autocreate=True,
+                request_settings_dict=request_settings_dict,
             )
 
         with self.settings(WEB_PUBLIC_STREAMS_ENABLED=False):
@@ -446,12 +449,14 @@ class StreamAdminTest(ZulipTestCase):
                     streams_raw,
                     owner,
                     autocreate=True,
+                    request_settings_dict=request_settings_dict,
                 )
 
         existing_streams, new_streams = list_to_streams(
             streams_raw,
             owner,
             autocreate=True,
+            request_settings_dict=request_settings_dict,
         )
 
         self.assert_length(new_streams, 3)
@@ -3845,7 +3850,7 @@ class SubscriptionAPITest(ZulipTestCase):
         # Now add ourselves
         with (
             self.capture_send_event_calls(expected_num_events=2) as events,
-            self.assert_database_query_count(20),
+            self.assert_database_query_count(17),
         ):
             self.subscribe_via_post(
                 self.test_user,
@@ -4333,7 +4338,7 @@ class SubscriptionAPITest(ZulipTestCase):
         # Verify that peer_event events are never sent in Zephyr
         # realm. This does generate stream creation events from
         # send_stream_creation_events_for_previously_inaccessible_streams.
-        with self.assert_database_query_count(num_streams + 19):
+        with self.assert_database_query_count(num_streams + 16):
             with self.capture_send_event_calls(expected_num_events=num_streams + 1) as events:
                 self.subscribe_via_post(
                     mit_user,
@@ -4414,7 +4419,7 @@ class SubscriptionAPITest(ZulipTestCase):
         test_user_ids = [user.id for user in test_users]
 
         with (
-            self.assert_database_query_count(23),
+            self.assert_database_query_count(20),
             self.assert_memcached_count(11),
             mock.patch("zerver.views.streams.send_user_subscribed_and_new_channel_notifications"),
         ):

zerver/views/streams.py

@@ -69,6 +69,7 @@ from zerver.lib.stream_traffic import get_streams_traffic
 from zerver.lib.streams import (
     StreamDict,
     access_default_stream_group_by_id,
+    access_requested_group_permissions,
     access_stream_by_id,
     access_stream_by_name,
     access_stream_for_delete_or_update_requiring_metadata_access,
@@ -81,7 +82,6 @@ from zerver.lib.streams import (
     do_get_streams,
     filter_stream_authorization_for_adding_subscribers,
     get_anonymous_group_membership_dict_for_streams,
-    get_stream_permission_default_group,
     get_stream_permission_policy_key,
     list_to_streams,
     stream_to_dict,
@@ -103,9 +103,7 @@ from zerver.lib.user_groups import (
     GroupSettingChangeRequest,
     UserGroupMembershipDetails,
     access_user_group_api_value_for_setting,
-    access_user_group_for_setting,
     get_group_setting_value_for_api,
-    get_role_based_system_groups_dict,
     get_system_user_group_by_name,
     parse_group_setting_value,
     validate_group_setting_value_change,
@@ -113,15 +111,7 @@ from zerver.lib.user_groups import (
 from zerver.lib.user_topics import get_users_with_user_topic_visibility_policy
 from zerver.lib.users import access_bot_by_id, bulk_access_users_by_email, bulk_access_users_by_id
 from zerver.lib.utils import assert_is_not_none
-from zerver.models import (
-    ChannelFolder,
-    Realm,
-    Stream,
-    UserGroup,
-    UserMessage,
-    UserProfile,
-    UserTopic,
-)
+from zerver.models import ChannelFolder, Stream, UserMessage, UserProfile, UserTopic
 from zerver.models.groups import SystemGroups
 from zerver.models.streams import StreamTopicsPolicyEnum
 from zerver.models.users import get_system_bot
@@ -668,50 +658,6 @@ def you_were_just_subscribed_message(
 RETENTION_DEFAULT: str | int = "realm_default"
 
 
-def access_requested_group_permissions(
-    user_profile: UserProfile,
-    realm: Realm,
-    request_settings_dict: dict[str, Any],
-) -> tuple[dict[str, UserGroup], dict[int, UserGroupMembersData]]:
-    anonymous_group_membership = {}
-    group_settings_map = {}
-    system_groups_name_dict = get_role_based_system_groups_dict(realm)
-    for setting_name, permission_configuration in Stream.stream_permission_group_settings.items():
-        assert setting_name in request_settings_dict
-        if request_settings_dict[setting_name] is not None:
-            setting_request_value = request_settings_dict[setting_name]
-            setting_value = parse_group_setting_value(
-                setting_request_value, system_groups_name_dict[SystemGroups.NOBODY]
-            )
-            group_settings_map[setting_name] = access_user_group_for_setting(
-                setting_value,
-                user_profile,
-                setting_name=setting_name,
-                permission_configuration=permission_configuration,
-            )
-            if (
-                setting_name in ["can_delete_any_message_group", "can_delete_own_message_group"]
-                and group_settings_map[setting_name].id
-                != system_groups_name_dict[SystemGroups.NOBODY].id
-                and not user_profile.can_set_delete_message_policy()
-            ):
-                raise JsonableError(_("Insufficient permission"))
-
-            if not isinstance(setting_value, int):
-                anonymous_group_membership[group_settings_map[setting_name].id] = setting_value
-        else:
-            group_settings_map[setting_name] = get_stream_permission_default_group(
-                setting_name, system_groups_name_dict, creator=user_profile
-            )
-            if permission_configuration.default_group_name == "stream_creator_or_nobody":
-                # Default for some settings like "can_administer_channel_group"
-                # is anonymous group with stream creator.
-                anonymous_group_membership[group_settings_map[setting_name].id] = (
-                    UserGroupMembersData(direct_subgroups=[], direct_members=[user_profile.id])
-                )
-    return group_settings_map, anonymous_group_membership
-
-
 @transaction.atomic(savepoint=False)
 @require_non_guest_user
 @typed_endpoint
@@ -879,11 +825,6 @@ def add_subscriptions_backend(
         principals = []
 
     request_settings_dict = locals()
-    group_settings_map, anonymous_group_membership = access_requested_group_permissions(
-        user_profile,
-        realm,
-        request_settings_dict,
-    )
 
     folder: ChannelFolder | None = None
     if folder_id is not None:
@@ -910,32 +851,6 @@ def add_subscriptions_backend(
         validated_topics_policy = validate_topics_policy(topics_policy, user_profile)
         if validated_topics_policy is not None:
             stream_dict_copy["topics_policy"] = validated_topics_policy.value
-        stream_dict_copy["can_add_subscribers_group"] = group_settings_map[
-            "can_add_subscribers_group"
-        ]
-        stream_dict_copy["can_administer_channel_group"] = group_settings_map[
-            "can_administer_channel_group"
-        ]
-        stream_dict_copy["can_delete_any_message_group"] = group_settings_map[
-            "can_delete_any_message_group"
-        ]
-        stream_dict_copy["can_delete_own_message_group"] = group_settings_map[
-            "can_delete_own_message_group"
-        ]
-        stream_dict_copy["can_move_messages_out_of_channel_group"] = group_settings_map[
-            "can_move_messages_out_of_channel_group"
-        ]
-        stream_dict_copy["can_move_messages_within_channel_group"] = group_settings_map[
-            "can_move_messages_within_channel_group"
-        ]
-        stream_dict_copy["can_send_message_group"] = group_settings_map["can_send_message_group"]
-        stream_dict_copy["can_remove_subscribers_group"] = group_settings_map[
-            "can_remove_subscribers_group"
-        ]
-        stream_dict_copy["can_resolve_topics_group"] = group_settings_map[
-            "can_resolve_topics_group"
-        ]
-        stream_dict_copy["can_subscribe_group"] = group_settings_map["can_subscribe_group"]
         stream_dict_copy["folder"] = folder
 
         stream_dicts.append(stream_dict_copy)
@@ -952,7 +867,7 @@ def add_subscriptions_backend(
         user_profile,
         autocreate=True,
         is_default_stream=is_default_stream,
-        anonymous_group_membership=anonymous_group_membership,
+        request_settings_dict=request_settings_dict,
     )
 
     streams_categorized_by_permissions = filter_stream_authorization_for_adding_subscribers(