From f134662312f5b57570c6733406fb7f95b6f69926 Mon Sep 17 00:00:00 2001 From: Shubham Padia Date: Mon, 7 Oct 2024 19:52:13 +0000 Subject: [PATCH] user_groups: Check can_add_members_group before adding members. Removing members will be controlled by `can_manage_group` until we add `can_remove_members_group` in the future. Users with permission to manage a group can add members to that group by default without being present in `can_add_members_group`. --- zerver/lib/user_groups.py | 22 ++++++++++-- zerver/tests/test_events.py | 57 ++++++++++++++++++++------------ zerver/tests/test_user_groups.py | 18 ++++++---- zerver/views/user_groups.py | 2 +- 4 files changed, 68 insertions(+), 31 deletions(-) diff --git a/zerver/lib/user_groups.py b/zerver/lib/user_groups.py index 2dbc2cc840..ffadfa0166 100644 --- a/zerver/lib/user_groups.py +++ b/zerver/lib/user_groups.py @@ -173,9 +173,10 @@ def access_user_group_for_update( raise JsonableError(_("Insufficient permission")) assert permission_setting in NamedUserGroup.GROUP_PERMISSION_SETTINGS - if permission_setting == "can_manage_group" and check_permission_for_managing_all_groups( - user_group, user_profile - ): + if permission_setting in [ + "can_manage_group", + "can_add_members_group", + ] and check_permission_for_managing_all_groups(user_group, user_profile): return user_group user_has_permission = user_has_permission_for_group_setting( @@ -184,6 +185,21 @@ def access_user_group_for_update( NamedUserGroup.GROUP_PERMISSION_SETTINGS[permission_setting], ) + # Users with permission to manage the group should be able to add members + # to the group. This also takes care of the case where a user creating a group + # with themselves having the permission to manage it don't have to explicitly + # add themselves to can_add_members_group. + if ( + not user_has_permission + and permission_setting == "can_add_members_group" + and permission_setting != "can_manage_group" + ): + user_has_permission = user_has_permission_for_group_setting( + user_group.can_manage_group, + user_profile, + NamedUserGroup.GROUP_PERMISSION_SETTINGS["can_manage_group"], + ) + if not user_has_permission: raise JsonableError(_("Insufficient permission")) diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 8e47f27341..ff0246ba6b 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -3093,24 +3093,29 @@ class NormalActionsTest(BaseAction): # event if they cannot access the deactivated user. user_profile = self.example_user("cordelia") self.user_profile = self.example_user("polonius") - with self.verify_action(num_events=6) as events: + with self.verify_action(num_events=7) as events: do_deactivate_user(user_profile, acting_user=None) check_user_group_remove_members("events[0]", events[0]) check_user_group_remove_members("events[1]", events[1]) check_user_group_remove_members("events[2]", events[2]) - check_user_group_update("events[3]", events[3], "can_manage_group") - check_realm_update_dict("events[4]", events[4]) - check_user_group_update("events[5]", events[5], "can_mention_group") + check_user_group_update("events[3]", events[3], "can_add_members_group") + check_user_group_update("events[4]", events[4], "can_manage_group") + check_realm_update_dict("events[5]", events[5]) + check_user_group_update("events[6]", events[6], "can_mention_group") self.assertEqual( - events[3]["data"]["can_manage_group"], + events[3]["data"]["can_add_members_group"], AnonymousSettingGroupDict(direct_members=[], direct_subgroups=[]), ) self.assertEqual( - events[4]["data"]["can_create_public_channel_group"], + events[4]["data"]["can_manage_group"], + AnonymousSettingGroupDict(direct_members=[], direct_subgroups=[]), + ) + self.assertEqual( + events[5]["data"]["can_create_public_channel_group"], AnonymousSettingGroupDict(direct_members=[], direct_subgroups=[members_group.id]), ) self.assertEqual( - events[5]["data"]["can_mention_group"], + events[6]["data"]["can_mention_group"], AnonymousSettingGroupDict( direct_members=[hamlet.id], direct_subgroups=[members_group.id] ), @@ -3118,24 +3123,29 @@ class NormalActionsTest(BaseAction): user_profile = self.example_user("cordelia") do_reactivate_user(user_profile, acting_user=None) - with self.verify_action(num_events=6, user_list_incomplete=True) as events: + with self.verify_action(num_events=7, user_list_incomplete=True) as events: do_deactivate_user(user_profile, acting_user=None) check_user_group_remove_members("events[0]", events[0]) check_user_group_remove_members("events[1]", events[1]) check_user_group_remove_members("events[2]", events[2]) - check_user_group_update("events[3]", events[3], "can_manage_group") - check_realm_update_dict("events[4]", events[4]) - check_user_group_update("events[5]", events[5], "can_mention_group") + check_user_group_update("events[3]", events[3], "can_add_members_group") + check_user_group_update("events[4]", events[4], "can_manage_group") + check_realm_update_dict("events[5]", events[5]) + check_user_group_update("events[6]", events[6], "can_mention_group") self.assertEqual( - events[3]["data"]["can_manage_group"], + events[3]["data"]["can_add_members_group"], AnonymousSettingGroupDict(direct_members=[], direct_subgroups=[]), ) self.assertEqual( - events[4]["data"]["can_create_public_channel_group"], + events[4]["data"]["can_manage_group"], + AnonymousSettingGroupDict(direct_members=[], direct_subgroups=[]), + ) + self.assertEqual( + events[5]["data"]["can_create_public_channel_group"], AnonymousSettingGroupDict(direct_members=[], direct_subgroups=[members_group.id]), ) self.assertEqual( - events[5]["data"]["can_mention_group"], + events[6]["data"]["can_mention_group"], AnonymousSettingGroupDict( direct_members=[hamlet.id], direct_subgroups=[members_group.id] ), @@ -3238,26 +3248,31 @@ class NormalActionsTest(BaseAction): self.user_profile = self.example_user("polonius") # Guest users receives group members update event for three groups - # members group, full members group and hamletcharacters group. - with self.verify_action(num_events=6) as events: + with self.verify_action(num_events=7) as events: do_reactivate_user(user_profile, acting_user=None) check_user_group_add_members("events[0]", events[0]) check_user_group_add_members("events[1]", events[1]) check_user_group_add_members("events[2]", events[2]) - check_user_group_update("events[3]", events[3], "can_manage_group") - check_realm_update_dict("events[4]", events[4]) - check_user_group_update("events[5]", events[5], "can_mention_group") + check_user_group_update("events[3]", events[3], "can_add_members_group") + check_user_group_update("events[4]", events[4], "can_manage_group") + check_realm_update_dict("events[5]", events[5]) + check_user_group_update("events[6]", events[6], "can_mention_group") self.assertEqual( - events[3]["data"]["can_manage_group"], + events[3]["data"]["can_add_members_group"], AnonymousSettingGroupDict(direct_members=[user_profile.id], direct_subgroups=[]), ) self.assertEqual( - events[4]["data"]["can_create_public_channel_group"], + events[4]["data"]["can_manage_group"], + AnonymousSettingGroupDict(direct_members=[user_profile.id], direct_subgroups=[]), + ) + self.assertEqual( + events[5]["data"]["can_create_public_channel_group"], AnonymousSettingGroupDict( direct_members=[user_profile.id], direct_subgroups=[hamletcharacters_group.id] ), ) self.assertEqual( - events[5]["data"]["can_mention_group"], + events[6]["data"]["can_mention_group"], AnonymousSettingGroupDict( direct_members=[user_profile.id], direct_subgroups=[members_group.id] ), diff --git a/zerver/tests/test_user_groups.py b/zerver/tests/test_user_groups.py index 4343b8ec65..a25ae6271d 100644 --- a/zerver/tests/test_user_groups.py +++ b/zerver/tests/test_user_groups.py @@ -19,7 +19,6 @@ from zerver.actions.streams import ( ) from zerver.actions.user_groups import ( add_subgroups_to_user_group, - bulk_add_members_to_user_groups, bulk_remove_members_from_user_groups, check_add_user_group, create_user_group_in_database, @@ -2355,13 +2354,11 @@ class UserGroupAPITestCase(UserGroupTestCase): acting_user=None, ) - # Default value of can_manage_group is "Nobody system group". + # Default value of can_add_members_group is "group_creator". check_adding_members_to_group("iago", "Insufficient permission") - check_adding_members_to_group("othello", "Insufficient permission") - - # Add aaron to group so that we can test the removal. - bulk_add_members_to_user_groups([user_group], [aaron.id], acting_user=None) + check_adding_members_to_group("desdemona") + # Removing members still uses `can_manage_group`. check_removing_members_from_group("iago", "Insufficient permission") check_removing_members_from_group("othello", "Insufficient permission") @@ -2373,6 +2370,9 @@ class UserGroupAPITestCase(UserGroupTestCase): do_change_user_group_permission_setting( user_group, "can_manage_group", owners_group, acting_user=None ) + do_change_user_group_permission_setting( + user_group, "can_add_members_group", owners_group, acting_user=None + ) check_adding_members_to_group("iago", "Insufficient permission") check_adding_members_to_group("desdemona") @@ -2384,6 +2384,9 @@ class UserGroupAPITestCase(UserGroupTestCase): do_change_user_group_permission_setting( user_group, "can_manage_group", members_group, acting_user=None ) + do_change_user_group_permission_setting( + user_group, "can_add_members_group", members_group, acting_user=None + ) check_adding_members_to_group("polonius", "Not allowed for guest users") check_adding_members_to_group("cordelia") @@ -2396,6 +2399,9 @@ class UserGroupAPITestCase(UserGroupTestCase): do_change_user_group_permission_setting( user_group, "can_manage_group", setting_group, acting_user=None ) + do_change_user_group_permission_setting( + user_group, "can_add_members_group", setting_group, acting_user=None + ) check_adding_members_to_group("iago", "Insufficient permission") check_adding_members_to_group("hamlet") diff --git a/zerver/views/user_groups.py b/zerver/views/user_groups.py index 223d396e95..ec7bb0f89a 100644 --- a/zerver/views/user_groups.py +++ b/zerver/views/user_groups.py @@ -306,7 +306,7 @@ def add_members_to_group_backend( ) else: user_group = access_user_group_for_update( - user_group_id, user_profile, permission_setting="can_manage_group" + user_group_id, user_profile, permission_setting="can_add_members_group" ) member_users = user_ids_to_users(members, user_profile.realm, allow_deactivated=False)