diff --git a/zerver/actions/streams.py b/zerver/actions/streams.py index 2f4d7708ec..178d40af56 100644 --- a/zerver/actions/streams.py +++ b/zerver/actions/streams.py @@ -37,7 +37,7 @@ from zerver.lib.stream_subscription import ( ) from zerver.lib.stream_traffic import get_streams_traffic from zerver.lib.streams import ( - can_access_stream_user_ids, + can_access_stream_metadata_user_ids, check_basic_stream_access, get_group_setting_value_dict_for_streams, get_occupied_streams, @@ -118,7 +118,7 @@ def do_deactivate_stream(stream: Stream, *, acting_user: UserProfile | None) -> raise JsonableError(_("Channel is already deactivated")) # Get the affected user ids *before* we deactivate everybody. - affected_user_ids = can_access_stream_user_ids(stream) + affected_user_ids = can_access_stream_metadata_user_ids(stream) was_public = stream.is_public() was_web_public = stream.is_web_public @@ -1273,11 +1273,11 @@ def do_change_stream_permission( stream.id, include_deactivated_users=False ).values_list("user_profile_id", flat=True) - old_can_access_stream_user_ids = set(stream_subscriber_user_ids) | { + old_can_access_stream_metadata_user_ids = set(stream_subscriber_user_ids) | { user.id for user in stream.realm.get_admin_users_and_bots() } non_guest_user_ids = set(active_non_guest_user_ids(stream.realm_id)) - notify_stream_creation_ids = non_guest_user_ids - old_can_access_stream_user_ids + notify_stream_creation_ids = non_guest_user_ids - old_can_access_stream_metadata_user_ids recent_traffic = get_streams_traffic({stream.id}, realm) setting_groups_dict = get_group_setting_value_dict_for_streams([stream]) @@ -1312,7 +1312,9 @@ def do_change_stream_permission( ) # we do not need to send update events to the users who received creation event # since they already have the updated stream info. - notify_stream_update_ids = can_access_stream_user_ids(stream) - notify_stream_creation_ids + notify_stream_update_ids = ( + can_access_stream_metadata_user_ids(stream) - notify_stream_creation_ids + ) send_event_on_commit(stream.realm, event, notify_stream_update_ids) old_policy_name = get_stream_permission_policy_name( @@ -1431,7 +1433,7 @@ def do_rename_stream(stream: Stream, new_name: str, user_profile: UserProfile) - stream_id=stream.id, name=old_name, ) - send_event_on_commit(stream.realm, event, can_access_stream_user_ids(stream)) + send_event_on_commit(stream.realm, event, can_access_stream_metadata_user_ids(stream)) sender = get_system_bot(settings.NOTIFICATION_BOT, stream.realm_id) with override_language(stream.realm.default_language): internal_send_stream_message( @@ -1505,7 +1507,7 @@ def do_change_stream_description( value=new_description, rendered_description=stream.rendered_description, ) - send_event_on_commit(stream.realm, event, can_access_stream_user_ids(stream)) + send_event_on_commit(stream.realm, event, can_access_stream_metadata_user_ids(stream)) send_change_stream_description_notification( stream, @@ -1590,7 +1592,7 @@ def do_change_stream_message_retention_days( stream_id=stream.id, name=stream.name, ) - send_event_on_commit(stream.realm, event, can_access_stream_user_ids(stream)) + send_event_on_commit(stream.realm, event, can_access_stream_metadata_user_ids(stream)) send_change_stream_message_retention_days_notification( user_profile=acting_user, stream=stream, @@ -1644,7 +1646,7 @@ def do_change_stream_group_based_setting( stream_id=stream.id, name=stream.name, ) - send_event_on_commit(stream.realm, event, can_access_stream_user_ids(stream)) + send_event_on_commit(stream.realm, event, can_access_stream_metadata_user_ids(stream)) if setting_name == "can_send_message_group": old_stream_post_policy = get_stream_post_policy_value_based_on_group_setting(old_user_group) @@ -1659,7 +1661,7 @@ def do_change_stream_group_based_setting( stream_id=stream.id, name=stream.name, ) - send_event_on_commit(stream.realm, event, can_access_stream_user_ids(stream)) + send_event_on_commit(stream.realm, event, can_access_stream_metadata_user_ids(stream)) # Backwards-compatibility code: We removed the # is_announcement_only property in early 2020, but we send a @@ -1673,7 +1675,7 @@ def do_change_stream_group_based_setting( stream_id=stream.id, name=stream.name, ) - send_event_on_commit(stream.realm, event, can_access_stream_user_ids(stream)) + send_event_on_commit(stream.realm, event, can_access_stream_metadata_user_ids(stream)) assert acting_user is not None send_stream_posting_permission_update_notification( diff --git a/zerver/lib/streams.py b/zerver/lib/streams.py index b43930954a..93b55b6469 100644 --- a/zerver/lib/streams.py +++ b/zerver/lib/streams.py @@ -24,6 +24,7 @@ from zerver.lib.string_validation import check_stream_name from zerver.lib.timestamp import datetime_to_timestamp from zerver.lib.types import AnonymousSettingGroupDict, APIStreamDict from zerver.lib.user_groups import ( + get_recursive_group_members, get_recursive_membership_groups, get_role_based_system_groups_dict, user_has_permission_for_group_setting, @@ -179,6 +180,18 @@ def get_default_values_for_stream_permission_group_settings( return group_setting_values +def get_user_ids_with_metadata_access_via_permission_groups(stream: Stream) -> set[int]: + stream_admin_user_ids = set( + get_recursive_group_members(stream.can_administer_channel_group).values_list( + "id", flat=True + ) + ) + stream_add_subscribers_group_user_ids = set( + get_recursive_group_members(stream.can_add_subscribers_group).values_list("id", flat=True) + ) + return stream_admin_user_ids | stream_add_subscribers_group_user_ids + + @transaction.atomic(savepoint=False) def create_stream_if_needed( realm: Realm, @@ -264,9 +277,15 @@ def create_stream_if_needed( realm, stream, notify_user_ids, setting_groups_dict=setting_groups_dict ) else: - realm_admin_ids = [user.id for user in stream.realm.get_admin_users_and_bots()] + realm_admin_ids = {user.id for user in stream.realm.get_admin_users_and_bots()} send_stream_creation_event( - realm, stream, realm_admin_ids, setting_groups_dict=setting_groups_dict + realm, + stream, + list( + realm_admin_ids + | get_user_ids_with_metadata_access_via_permission_groups(stream) + ), + setting_groups_dict=setting_groups_dict, ) return stream, created @@ -746,7 +765,7 @@ def public_stream_user_ids(stream: Stream) -> set[int]: return set(active_non_guest_user_ids(stream.realm_id)) | guest_subscriptions_ids -def can_access_stream_user_ids(stream: Stream) -> set[int]: +def can_access_stream_metadata_user_ids(stream: Stream) -> set[int]: # return user ids of users who can access the attributes of a # stream, such as its name/description. Useful for sending events # to all users with access to a stream's attributes. @@ -755,10 +774,13 @@ def can_access_stream_user_ids(stream: Stream) -> set[int]: # except unsubscribed guest users return public_stream_user_ids(stream) else: - # for a private stream, it's subscribers plus realm admins. - return private_stream_user_ids(stream.id) | { - user.id for user in stream.realm.get_admin_users_and_bots() - } + # for a private stream, it's subscribers plus channel admins + # and users belonging to `can_add_subscribers_group`. + return ( + private_stream_user_ids(stream.id) + | {user.id for user in stream.realm.get_admin_users_and_bots()} + | get_user_ids_with_metadata_access_via_permission_groups(stream) + ) def can_access_stream_history(user_profile: UserProfile, stream: Stream) -> bool: diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index a042952eb1..bf8eb82973 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -75,7 +75,7 @@ from zerver.lib.streams import ( access_stream_by_id, access_stream_by_name, can_access_stream_history, - can_access_stream_user_ids, + can_access_stream_metadata_user_ids, create_stream_if_needed, create_streams_if_needed, do_get_streams, @@ -286,6 +286,7 @@ class TestCreateStreams(ZulipTestCase): stream_names = ["new1", "new2", "new3"] stream_descriptions = ["des1", "des2", "des3"] realm = get_realm("zulip") + iago = self.example_user("iago") # Test stream creation events. with self.capture_send_event_calls(expected_num_events=1) as events: @@ -298,15 +299,34 @@ class TestCreateStreams(ZulipTestCase): self.assertEqual(events[0]["event"]["streams"][0]["name"], "Public stream") self.assertEqual(events[0]["event"]["streams"][0]["stream_weekly_traffic"], None) + aaron_group = check_add_user_group( + realm, "aaron_group", [self.example_user("aaron")], acting_user=iago + ) + prospero_group = check_add_user_group( + realm, "prospero_group", [self.example_user("prospero")], acting_user=iago + ) with self.capture_send_event_calls(expected_num_events=1) as events: - ensure_stream(realm, "Private stream", invite_only=True, acting_user=None) + create_stream_if_needed( + realm, + "Private stream", + invite_only=True, + can_administer_channel_group=aaron_group, + can_add_subscribers_group=prospero_group + ) self.assertEqual(events[0]["event"]["type"], "stream") self.assertEqual(events[0]["event"]["op"], "create") # Send private stream creation event to only realm admins. - self.assert_length(events[0]["users"], 2) - self.assertTrue(self.example_user("iago").id in events[0]["users"]) - self.assertTrue(self.example_user("desdemona").id in events[0]["users"]) + self.assert_length(events[0]["users"], 4) + self.assertCountEqual( + [ + iago.id, + self.example_user("desdemona").id, + self.example_user("aaron").id, + self.example_user("prospero").id, + ], + events[0]["users"], + ) self.assertEqual(events[0]["event"]["streams"][0]["name"], "Private stream") self.assertEqual(events[0]["event"]["streams"][0]["stream_weekly_traffic"], None) @@ -2042,6 +2062,24 @@ class StreamAdminTest(ZulipTestCase): stream_private = self.make_stream( "stream_private_name1", realm=user_profile.realm, invite_only=True ) + aaron_group = check_add_user_group( + realm, "aaron_group", [self.example_user("aaron")], acting_user=user_profile + ) + do_change_stream_group_based_setting( + stream_private, + "can_administer_channel_group", + aaron_group, + acting_user=None, + ) + prospero_group = check_add_user_group( + realm, "prospero_group", [self.example_user("prospero")], acting_user=user_profile + ) + do_change_stream_group_based_setting( + stream_private, + "can_add_subscribers_group", + prospero_group, + acting_user=None, + ) self.subscribe(self.example_user("cordelia"), "stream_private_name1") with self.capture_send_event_calls(expected_num_events=2) as events: stream_id = get_stream("stream_private_name1", realm).id @@ -2051,13 +2089,16 @@ class StreamAdminTest(ZulipTestCase): ) self.assert_json_success(result) notified_user_ids = set(events[0]["users"]) - self.assertEqual(notified_user_ids, can_access_stream_user_ids(stream_private)) + self.assertEqual(notified_user_ids, can_access_stream_metadata_user_ids(stream_private)) self.assertIn(self.example_user("cordelia").id, notified_user_ids) # An important corner case is that all organization admins are notified. self.assertIn(self.example_user("iago").id, notified_user_ids) # The current user, Hamlet was made an admin and thus should be notified too. self.assertIn(user_profile.id, notified_user_ids) - self.assertNotIn(self.example_user("prospero").id, notified_user_ids) + # Channel admin should be notified. + self.assertIn(self.example_user("aaron").id, notified_user_ids) + # User belonging to `can_add_subscribers_group` should be notified. + self.assertIn(self.example_user("prospero").id, notified_user_ids) def test_rename_stream_requires_admin(self) -> None: user_profile = self.example_user("hamlet") @@ -2297,15 +2338,6 @@ class StreamAdminTest(ZulipTestCase): '

See https://zulip.com/team/

', ) - user_profile_group = check_add_user_group( - realm, "user_profile_group", [user_profile], acting_user=user_profile - ) - do_change_stream_group_based_setting( - stream, - "can_administer_channel_group", - user_profile_group, - acting_user=None, - ) do_change_user_role(user_profile, UserProfile.ROLE_MEMBER, acting_user=None) result = self.client_patch( f"/json/streams/{stream_id}", {"description": "Test description"} @@ -6463,7 +6495,7 @@ class SubscriptionAPITest(ZulipTestCase): ) # Test creating private stream. - with self.assert_database_query_count(46): + with self.assert_database_query_count(48): self.subscribe_via_post( self.test_user, [new_streams[1]],