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

@@ -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(
+    if permission_setting in [
-        user_group, user_profile
+        "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"))
 

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_user_group_update("events[3]", events[3], "can_add_members_group")
-        check_realm_update_dict("events[4]", events[4])
+        check_user_group_update("events[4]", events[4], "can_manage_group")
-        check_user_group_update("events[5]", events[5], "can_mention_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_user_group_update("events[3]", events[3], "can_add_members_group")
-        check_realm_update_dict("events[4]", events[4])
+        check_user_group_update("events[4]", events[4], "can_manage_group")
-        check_user_group_update("events[5]", events[5], "can_mention_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_user_group_update("events[3]", events[3], "can_add_members_group")
-        check_realm_update_dict("events[4]", events[4])
+        check_user_group_update("events[4]", events[4], "can_manage_group")
-        check_user_group_update("events[5]", events[5], "can_mention_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]
             ),

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")
+        check_adding_members_to_group("desdemona")
-
-        # Add aaron to group so that we can test the removal.
-        bulk_add_members_to_user_groups([user_group], [aaron.id], acting_user=None)
 
+        # 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")
 

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)