From d4d7a8fe2afc4b64862c5ddbd0b5daba08851396 Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Mon, 8 Sep 2025 21:03:44 +0530 Subject: [PATCH] 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. --- zerver/lib/streams.py | 83 +++++++++++++++++++++++- zerver/tests/test_channel_creation.py | 23 +++++-- zerver/tests/test_channel_fetch.py | 2 +- zerver/tests/test_subs.py | 9 ++- zerver/views/streams.py | 91 +-------------------------- 5 files changed, 110 insertions(+), 98 deletions(-) diff --git a/zerver/lib/streams.py b/zerver/lib/streams.py index 2bdd1a992e..ccf561da81 100644 --- a/zerver/lib/streams.py +++ b/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 ( @@ -1486,13 +1488,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 == "channel_creator": + # 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 @@ -1547,6 +1593,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( @@ -1557,6 +1611,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 diff --git a/zerver/tests/test_channel_creation.py b/zerver/tests/test_channel_creation.py index a2e0af3f54..51fcff7f2c 100644 --- a/zerver/tests/test_channel_creation.py +++ b/zerver/tests/test_channel_creation.py @@ -1056,8 +1056,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 = [ { @@ -1067,7 +1072,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 = [ { @@ -1076,7 +1083,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") @@ -1105,10 +1114,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") diff --git a/zerver/tests/test_channel_fetch.py b/zerver/tests/test_channel_fetch.py index 34b8d023a6..f62f59a8c7 100644 --- a/zerver/tests/test_channel_fetch.py +++ b/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, diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 6b7000e1a9..a71f235862 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -426,6 +426,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 @@ -435,6 +437,7 @@ class StreamAdminTest(ZulipTestCase): streams_raw, user_profile, autocreate=True, + request_settings_dict=request_settings_dict, ) with self.settings(WEB_PUBLIC_STREAMS_ENABLED=False): @@ -445,12 +448,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) @@ -3804,7 +3809,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, @@ -4309,7 +4314,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"), ): diff --git a/zerver/views/streams.py b/zerver/views/streams.py index b97748f712..5cd3c557fd 100644 --- a/zerver/views/streams.py +++ b/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, @@ -80,7 +81,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, @@ -102,9 +102,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, @@ -112,15 +110,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 @@ -659,50 +649,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 == "channel_creator": - # 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 @@ -868,11 +814,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: @@ -899,32 +840,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) @@ -941,7 +856,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(