From 9bbd6a731678fa97a2ee03dfed3a01a02b851b30 Mon Sep 17 00:00:00 2001 From: Shubham Padia Date: Wed, 9 Oct 2024 07:37:35 +0000 Subject: [PATCH] user_groups: Check can_add_members_group when adding members. Fixes #25942. Users with permission to manage the group (either on the group level or realm level) should be able to add members to the group without being present in can_add_members_group. --- web/src/settings_data.ts | 16 +++++ web/src/user_group_edit.js | 6 +- web/tests/settings_data.test.js | 119 +++++++++++++++++++++++++++++++ zerver/tests/test_user_groups.py | 22 ++++-- 4 files changed, 157 insertions(+), 6 deletions(-) diff --git a/web/src/settings_data.ts b/web/src/settings_data.ts index 64e7d89741..b4bbfdfd97 100644 --- a/web/src/settings_data.ts +++ b/web/src/settings_data.ts @@ -215,6 +215,22 @@ export function can_manage_user_group(group_id: number): boolean { ); } +export function can_add_members_to_user_group(group_id: number): boolean { + const group = user_groups.get_user_group_from_id(group_id); + + if ( + user_has_permission_for_group_setting( + group.can_add_members_group, + "can_add_members_group", + "group", + ) + ) { + return true; + } + + return can_manage_user_group(group_id); +} + export function can_join_user_group(group_id: number): boolean { const group = user_groups.get_user_group_from_id(group_id); diff --git a/web/src/user_group_edit.js b/web/src/user_group_edit.js index ffaae2c9d6..bbfb4d1dc3 100644 --- a/web/src/user_group_edit.js +++ b/web/src/user_group_edit.js @@ -87,7 +87,7 @@ function update_add_members_elements(group) { const $input_element = $add_members_container.find(".input").expectOne(); const $button_element = $add_members_container.find('button[name="add_member"]').expectOne(); - if (settings_data.can_manage_user_group(group.id)) { + if (settings_data.can_add_members_to_user_group(group.id)) { $input_element.prop("contenteditable", true); $button_element.prop("disabled", false); $button_element.css("pointer-events", ""); @@ -658,6 +658,10 @@ export function update_group(event) { if (event.data.can_mention_group !== undefined) { sync_group_permission_setting("can_mention_group", group); } + if (event.data.can_manage_group !== undefined) { + sync_group_permission_setting("can_add_members_group", group); + update_group_management_ui(); + } if (event.data.can_manage_group !== undefined) { sync_group_permission_setting("can_manage_group", group); update_group_management_ui(); diff --git a/web/tests/settings_data.test.js b/web/tests/settings_data.test.js index 4389dd8305..b1613bdfc6 100644 --- a/web/tests/settings_data.test.js +++ b/web/tests/settings_data.test.js @@ -585,6 +585,125 @@ run_test("can_join_user_group", ({override}) => { assert.ok(settings_data.can_join_user_group(students.id)); }); +run_test("can_add_members_user_group", () => { + const admins = { + description: "Administrators", + name: "role:administrators", + id: 1, + members: new Set([1]), + is_system_group: true, + direct_subgroup_ids: new Set([]), + can_add_members_group: 4, + can_manage_group: 4, + can_mention_group: 1, + }; + const moderators = { + description: "Moderators", + name: "role:moderators", + id: 2, + members: new Set([2]), + is_system_group: true, + direct_subgroup_ids: new Set([1]), + can_add_members_group: 4, + can_manage_group: 4, + can_mention_group: 1, + }; + const members = { + description: "Members", + name: "role:members", + id: 3, + members: new Set([3, 4]), + is_system_group: true, + direct_subgroup_ids: new Set([1, 2]), + can_add_members_group: 4, + can_manage_group: 4, + can_mention_group: 4, + }; + const nobody = { + description: "Nobody", + name: "role:nobody", + id: 4, + members: new Set([]), + is_system_group: true, + direct_subgroup_ids: new Set([]), + can_add_members_group: 4, + can_manage_group: 4, + can_mention_group: 2, + }; + const students = { + description: "Students group", + name: "Students", + id: 5, + members: new Set([1, 2]), + is_system_group: false, + direct_subgroup_ids: new Set([4, 5]), + can_add_members_group: 1, + can_manage_group: { + direct_members: [6], + direct_subgroups: [], + }, + can_mention_group: 3, + creator_id: 4, + }; + user_groups.initialize({ + realm_user_groups: [admins, moderators, members, nobody, students], + }); + realm.realm_can_manage_all_groups = nobody.id; + + page_params.is_spectator = true; + assert.ok(!settings_data.can_add_members_to_user_group(students.id)); + + page_params.is_spectator = false; + // admin user + current_user.user_id = 1; + assert.ok(settings_data.can_add_members_to_user_group(students.id)); + + // moderator user + current_user.user_id = 2; + assert.ok(!settings_data.can_add_members_to_user_group(students.id)); + + let event = { + group_id: students.id, + data: { + can_add_members_group: moderators.id, + }, + }; + user_groups.update(event); + assert.ok(settings_data.can_add_members_to_user_group(students.id)); + + // Some other user. + current_user.user_id = 5; + assert.ok(!settings_data.can_add_members_to_user_group(students.id)); + + event = { + group_id: students.id, + data: { + can_add_members_group: { + direct_members: [5], + direct_subgroups: [admins.id], + }, + }, + }; + user_groups.update(event); + assert.ok(settings_data.can_add_members_to_user_group(students.id)); + + // Users with permission to manage the group should be able to add + // members to the group without adding themselves to + // can_add_members_group. + current_user.user_id = 4; + assert.ok(!settings_data.can_add_members_to_user_group(students.id)); + event = { + group_id: students.id, + data: { + can_manage_group: { + direct_members: [4], + }, + }, + }; + user_groups.update(event); + assert.ok(settings_data.can_add_members_to_user_group(students.id)); +}); + run_test("type_id_to_string", () => { page_params.bot_types = [ { diff --git a/zerver/tests/test_user_groups.py b/zerver/tests/test_user_groups.py index a25ae6271d..b26e84f601 100644 --- a/zerver/tests/test_user_groups.py +++ b/zerver/tests/test_user_groups.py @@ -2249,8 +2249,7 @@ class UserGroupAPITestCase(UserGroupTestCase): check_adding_members_to_group("othello") check_removing_members_from_group("othello") - # Check only members are allowed to add/remove users in the group and only if belong to the - # user group. + # Check only members are allowed to add/remove users in the group. members_group = NamedUserGroup.objects.get(name=SystemGroups.MEMBERS, realm=realm) do_change_realm_permission_group_setting( realm, @@ -2380,12 +2379,12 @@ class UserGroupAPITestCase(UserGroupTestCase): check_removing_members_from_group("iago", "Insufficient permission") check_removing_members_from_group("desdemona") - members_group = system_group_dict[SystemGroups.MEMBERS] + everyone_group = system_group_dict[SystemGroups.EVERYONE] do_change_user_group_permission_setting( - user_group, "can_manage_group", members_group, acting_user=None + user_group, "can_manage_group", everyone_group, acting_user=None ) do_change_user_group_permission_setting( - user_group, "can_add_members_group", members_group, acting_user=None + user_group, "can_add_members_group", everyone_group, acting_user=None ) check_adding_members_to_group("polonius", "Not allowed for guest users") check_adding_members_to_group("cordelia") @@ -2414,6 +2413,19 @@ class UserGroupAPITestCase(UserGroupTestCase): check_adding_members_to_group("desdemona") check_removing_members_from_group("desdemona") + # If user is part of `can_manage_group`, they need not be part + # of `can_add_members_group` to add members. + othello_group = self.create_or_update_anonymous_group_for_setting([othello], []) + hamlet_group = self.create_or_update_anonymous_group_for_setting([hamlet], []) + do_change_user_group_permission_setting( + user_group, "can_manage_group", othello_group, acting_user=None + ) + do_change_user_group_permission_setting( + user_group, "can_add_members_group", hamlet_group, acting_user=None + ) + check_adding_members_to_group("othello") + check_removing_members_from_group("othello") + def test_adding_yourself_to_group(self) -> None: realm = get_realm("zulip") othello = self.example_user("othello")