diff --git a/web/src/stream_data.ts b/web/src/stream_data.ts index be3d2f856f..0f1f002645 100644 --- a/web/src/stream_data.ts +++ b/web/src/stream_data.ts @@ -565,6 +565,10 @@ export function can_unsubscribe_others(sub: StreamSubscription): boolean { return false; } + if (current_user.is_admin) { + return true; + } + return user_groups.is_user_in_setting_group( sub.can_remove_subscribers_group, people.my_current_user_id(), diff --git a/web/tests/stream_data.test.cjs b/web/tests/stream_data.test.cjs index ea55dc36e8..9c2a6728a5 100644 --- a/web/tests/stream_data.test.cjs +++ b/web/tests/stream_data.test.cjs @@ -1154,8 +1154,15 @@ test("can_unsubscribe_others", ({override}) => { is_system_group: true, direct_subgroup_ids: new Set([]), }; + const students = { + name: "Students", + id: 5, + members: new Set([member_user_id]), + is_system_group: false, + direct_subgroup_ids: new Set([]), + }; - user_groups.initialize({realm_user_groups: [admins, moderators, all, nobody]}); + user_groups.initialize({realm_user_groups: [admins, moderators, all, nobody, students]}); const sub = { name: "Denmark", @@ -1187,10 +1194,17 @@ test("can_unsubscribe_others", ({override}) => { people.initialize_current_user(member_user_id); assert.equal(stream_data.can_unsubscribe_others(sub), true); - // With the nobody system group, admins cannot unsubscribe others. - sub.can_remove_subscribers_group = nobody.id; + // With the setting set to user defined group not including admin, + // admin can still unsubscribe others. + sub.can_remove_subscribers_group = students.id; override(current_user, "is_admin", true); + people.initialize_current_user(admin_user_id); + assert.equal(stream_data.can_unsubscribe_others(sub), true); + override(current_user, "is_admin", false); + people.initialize_current_user(moderator_user_id); assert.equal(stream_data.can_unsubscribe_others(sub), false); + people.initialize_current_user(member_user_id); + assert.equal(stream_data.can_unsubscribe_others(sub), true); // This isn't a real state, but we want coverage on !can_view_subscribers. sub.can_remove_subscribers_group = all.id; diff --git a/zerver/lib/streams.py b/zerver/lib/streams.py index 9912e617b0..cfa597871c 100644 --- a/zerver/lib/streams.py +++ b/zerver/lib/streams.py @@ -675,6 +675,9 @@ def can_remove_subscribers_from_stream( if not check_basic_stream_access(user_profile, stream, sub, allow_realm_admin=True): return False + if user_profile.is_realm_admin: + return True + group_allowed_to_remove_subscribers = stream.can_remove_subscribers_group assert group_allowed_to_remove_subscribers is not None return user_has_permission_for_group_setting( diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index bbb6596513..84da178a8d 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -19992,6 +19992,8 @@ paths: The set of users who have permission to unsubscribe others from this channel expressed as an [update to a group-setting value][update-group-setting]. + Administrators can always unsubscribe others from a channel. + Note that a user who is a member of the specified user group must also [have access](/help/channel-permissions) to the channel in order to unsubscribe others. @@ -24570,6 +24572,8 @@ components: A [group-setting value][setting-values] defining the set of users who have permission to remove subscribers from this channel. + Administrators can always unsubscribe others from a channel. + Note that a user who is a member of the specified user group must also [have access](/help/channel-permissions) to the channel in order to unsubscribe others. diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index dea65654bd..ae182b2346 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -2714,7 +2714,7 @@ class StreamAdminTest(ZulipTestCase): those you aren't on. """ result = self.attempt_unsubscribe_of_principal( - query_count=17, + query_count=15, target_users=[self.example_user("cordelia")], is_realm_admin=True, is_subbed=True, @@ -2741,7 +2741,7 @@ class StreamAdminTest(ZulipTestCase): for name in ["cordelia", "prospero", "iago", "hamlet", "outgoing_webhook_bot"] ] result = self.attempt_unsubscribe_of_principal( - query_count=24, + query_count=22, cache_count=8, target_users=target_users, is_realm_admin=True, @@ -2759,7 +2759,7 @@ class StreamAdminTest(ZulipTestCase): are on. """ result = self.attempt_unsubscribe_of_principal( - query_count=18, + query_count=17, target_users=[self.example_user("cordelia")], is_realm_admin=True, is_subbed=True, @@ -2776,7 +2776,7 @@ class StreamAdminTest(ZulipTestCase): streams you aren't on. """ result = self.attempt_unsubscribe_of_principal( - query_count=18, + query_count=17, target_users=[self.example_user("cordelia")], is_realm_admin=True, is_subbed=False, @@ -2802,7 +2802,7 @@ class StreamAdminTest(ZulipTestCase): def test_admin_remove_others_from_stream_legacy_emails(self) -> None: result = self.attempt_unsubscribe_of_principal( - query_count=17, + query_count=15, target_users=[self.example_user("cordelia")], is_realm_admin=True, is_subbed=True, @@ -2816,7 +2816,7 @@ class StreamAdminTest(ZulipTestCase): def test_admin_remove_multiple_users_from_stream_legacy_emails(self) -> None: result = self.attempt_unsubscribe_of_principal( - query_count=19, + query_count=17, target_users=[self.example_user("cordelia"), self.example_user("prospero")], is_realm_admin=True, is_subbed=True, @@ -2830,7 +2830,7 @@ class StreamAdminTest(ZulipTestCase): def test_remove_unsubbed_user_along_with_subbed(self) -> None: result = self.attempt_unsubscribe_of_principal( - query_count=16, + query_count=14, target_users=[self.example_user("cordelia"), self.example_user("iago")], is_realm_admin=True, is_subbed=True, @@ -2847,7 +2847,7 @@ class StreamAdminTest(ZulipTestCase): fails gracefully. """ result = self.attempt_unsubscribe_of_principal( - query_count=9, + query_count=7, target_users=[self.example_user("cordelia")], is_realm_admin=True, is_subbed=False, @@ -2929,8 +2929,10 @@ class StreamAdminTest(ZulipTestCase): self.assert_length(json["not_removed"], 0) check_unsubscribing_user(self.example_user("hamlet"), leadership_group, expect_fail=True) - check_unsubscribing_user(self.example_user("desdemona"), leadership_group, expect_fail=True) check_unsubscribing_user(self.example_user("iago"), leadership_group) + # Owners can always unsubscribe others even when they are not a member + # allowed group. + check_unsubscribing_user(self.example_user("desdemona"), leadership_group) check_unsubscribing_user(self.example_user("othello"), managers_group, expect_fail=True) check_unsubscribing_user(self.example_user("shiva"), managers_group) @@ -2942,7 +2944,9 @@ class StreamAdminTest(ZulipTestCase): # are not subscribed to even if they are member of the allowed group. check_unsubscribing_user(self.example_user("shiva"), leadership_group, expect_fail=True) check_unsubscribing_user(self.example_user("iago"), leadership_group) - + # Owners can always unsubscribe others even when they are not a member + # allowed group. + check_unsubscribing_user(self.example_user("desdemona"), leadership_group) self.subscribe(self.example_user("shiva"), stream.name) check_unsubscribing_user(self.example_user("shiva"), leadership_group) @@ -2952,11 +2956,19 @@ class StreamAdminTest(ZulipTestCase): [leadership_group], ) check_unsubscribing_user(self.example_user("othello"), setting_group, expect_fail=True) - check_unsubscribing_user(self.example_user("desdemona"), setting_group, expect_fail=True) check_unsubscribing_user(self.example_user("hamlet"), setting_group) check_unsubscribing_user(self.example_user("iago"), setting_group) check_unsubscribing_user(self.example_user("shiva"), setting_group) + # Admins can unsubscribe others even when they are not a member of the + # allowed group. + setting_group = self.create_or_update_anonymous_group_for_setting( + [hamlet], + [], + ) + check_unsubscribing_user(self.example_user("desdemona"), setting_group) + check_unsubscribing_user(self.example_user("iago"), setting_group) + def test_remove_invalid_user(self) -> None: """ Trying to unsubscribe an invalid user from a stream fails gracefully.