user_groups: Update handling of deactivated groups.

We only allow updating name of a deactivated group, and not
allow updating description, members, subgroups and any setting
of a deactivated user group.

Deactivated user groups cannot be a a subgroup of any group
or used as a setting for a group.

zerver/lib/user_groups.py

@@ -72,11 +72,19 @@ class LockedUserGroupContext:
 
 
 def has_user_group_access(
-    user_group: NamedUserGroup, user_profile: UserProfile, *, for_read: bool, as_subgroup: bool
+    user_group: NamedUserGroup,
+    user_profile: UserProfile,
+    *,
+    for_read: bool,
+    as_subgroup: bool,
+    allow_deactivated: bool = False,
 ) -> bool:
     if user_group.realm_id != user_profile.realm_id:
         return False
 
+    if not allow_deactivated and user_group.deactivated:
+        raise JsonableError(_("User group is deactivated."))
+
     if as_subgroup:
         # At this time, we only check for realm ID of a potential subgroup.
         return True
@@ -109,7 +117,12 @@ def has_user_group_access(
 
 
 def access_user_group_by_id(
-    user_group_id: int, user_profile: UserProfile, *, for_read: bool, for_setting: bool = False
+    user_group_id: int,
+    user_profile: UserProfile,
+    *,
+    for_read: bool,
+    for_setting: bool = False,
+    allow_deactivated: bool = False,
 ) -> NamedUserGroup:
     try:
         if for_read and not for_setting:
@@ -121,7 +134,13 @@ def access_user_group_by_id(
     except NamedUserGroup.DoesNotExist:
         raise JsonableError(_("Invalid user group"))
 
-    if not has_user_group_access(user_group, user_profile, for_read=for_read, as_subgroup=False):
+    if not has_user_group_access(
+        user_group,
+        user_profile,
+        for_read=for_read,
+        as_subgroup=False,
+        allow_deactivated=allow_deactivated,
+    ):
         raise JsonableError(_("Insufficient permission"))
 
     return user_group
@@ -246,9 +265,10 @@ def lock_subgroups_with_respect_to_supergroup(
             )
 
         for subgroup in potential_subgroups:
-            # At this time, we only do a check on the realm ID of the fetched
+            # At this time, we only do a check on the realm ID of the subgroup and
-            # subgroup. This would be caught by the check earlier, so there is
+            # whether the group is deactivated or not. Realm ID error would be caught
-            # no coverage here.
+            # above and in case the user group is deactivated the error will be raised
+            # in has_user_group_access itself, so there is no coverage here.
             if not has_user_group_access(subgroup, acting_user, for_read=False, as_subgroup=True):
                 raise JsonableError(_("Insufficient permission"))  # nocoverage
 
@@ -324,11 +344,12 @@ def check_setting_configuration_for_system_groups(
 
 
 def update_or_create_user_group_for_setting(
-    realm: Realm,
+    user_profile: UserProfile,
     direct_members: list[int],
     direct_subgroups: list[int],
     current_setting_value: UserGroup | None,
 ) -> UserGroup:
+    realm = user_profile.realm
     if current_setting_value is not None and not hasattr(current_setting_value, "named_user_group"):
         # We do not create a new group if the setting was already set
         # to an anonymous group. The memberships of existing group
@@ -354,6 +375,14 @@ def update_or_create_user_group_for_setting(
             _("Invalid user group ID: {group_id}").format(group_id=group_ids_not_found[0])
         )
 
+    for subgroup in potential_subgroups:
+        # At this time, we only do a check on the realm ID of the subgroup and
+        # whether the group is deactivated or not. Realm ID error would be caught
+        # above and in case the user group is deactivated the error will be raised
+        # in has_user_group_access itself, so there is no coverage here.
+        if not has_user_group_access(subgroup, user_profile, for_read=False, as_subgroup=True):
+            raise JsonableError(_("Insufficient permission"))  # nocoverage
+
     user_group.direct_subgroups.set(group_ids_found)
 
     return user_group
@@ -380,7 +409,7 @@ def access_user_group_for_setting(
         raise SystemGroupRequiredError(setting_name)
 
     user_group = update_or_create_user_group_for_setting(
-        user_profile.realm,
+        user_profile,
         setting_user_group.direct_members,
         setting_user_group.direct_subgroups,
         current_setting_value,

zerver/tests/test_user_groups.py

@@ -625,6 +625,35 @@ class UserGroupAPITestCase(UserGroupTestCase):
         result = self.client_post("/json/user_groups/create", info=params)
         self.assert_json_error(result, "Invalid user group ID: 1111")
 
+        # Test can_mention_group cannot be set to a deactivated group.
+        do_deactivate_user_group(leadership_group, acting_user=None)
+        params = {
+            "name": "social",
+            "members": orjson.dumps([hamlet.id]).decode(),
+            "description": "Social team",
+            "can_mention_group": orjson.dumps(leadership_group.id).decode(),
+        }
+        result = self.client_post("/json/user_groups/create", info=params)
+        self.assert_json_error(result, "User group is deactivated.")
+
+        params = {
+            "name": "social",
+            "members": orjson.dumps([hamlet.id]).decode(),
+            "description": "Social team",
+            "can_mention_group": orjson.dumps(
+                {
+                    "direct_members": [othello.id],
+                    "direct_subgroups": [leadership_group.id],
+                }
+            ).decode(),
+        }
+        result = self.client_post("/json/user_groups/create", info=params)
+        self.assert_json_error(result, "User group is deactivated.")
+
+        # Reactivate group to use it in further tests.
+        leadership_group.deactivated = False
+        leadership_group.save()
+
         with self.settings(ALLOW_GROUP_VALUED_SETTINGS=False):
             params = {
                 "name": "frontend",
@@ -766,6 +795,17 @@ class UserGroupAPITestCase(UserGroupTestCase):
         result = self.client_patch(f"/json/user_groups/{user_group.id}", info=params)
         self.assert_json_error(result, "User group name cannot start with 'channel:'.")
 
+        do_deactivate_user_group(user_group, acting_user=None)
+        params = {"description": "Troubleshooting and support team"}
+        result = self.client_patch(f"/json/user_groups/{user_group.id}", info=params)
+        self.assert_json_error(result, "You can only change name of deactivated user groups")
+
+        params = {"name": "Support team"}
+        result = self.client_patch(f"/json/user_groups/{user_group.id}", info=params)
+        self.assert_json_success(result)
+        user_group = NamedUserGroup.objects.get(id=user_group.id)
+        self.assertEqual(user_group.name, "Support team")
+
     def do_test_update_user_group_permission_settings(self, setting_name: str) -> None:
         hamlet = self.example_user("hamlet")
         permission_configuration = NamedUserGroup.GROUP_PERMISSION_SETTINGS[setting_name]
@@ -913,6 +953,31 @@ class UserGroupAPITestCase(UserGroupTestCase):
         result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params)
         self.assert_json_error(result, "Invalid user group ID: 1111")
 
+        leadership_group = NamedUserGroup.objects.get(realm=hamlet.realm, name="leadership")
+        do_deactivate_user_group(leadership_group, acting_user=None)
+
+        params[setting_name] = orjson.dumps({"new": leadership_group.id}).decode()
+        result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params)
+        self.assert_json_error(result, "User group is deactivated.")
+
+        params[setting_name] = orjson.dumps(
+            {
+                "new": {
+                    "direct_members": [prospero.id],
+                    "direct_subgroups": [leadership_group.id],
+                }
+            }
+        ).decode()
+        result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params)
+        self.assert_json_error(result, "User group is deactivated.")
+
+        params[setting_name] = orjson.dumps({"new": moderators_group.id}).decode()
+        result = self.client_patch(f"/json/user_groups/{leadership_group.id}", info=params)
+        self.assert_json_error(result, "You can only change name of deactivated user groups")
+
+        leadership_group.deactivated = False
+        leadership_group.save()
+
         # Test case when ALLOW_GROUP_VALUED_SETTINGS is False.
         with self.settings(ALLOW_GROUP_VALUED_SETTINGS=False):
             params[setting_name] = orjson.dumps(
@@ -957,6 +1022,7 @@ class UserGroupAPITestCase(UserGroupTestCase):
         hamlet = self.example_user("hamlet")
         check_add_user_group(hamlet.realm, "support", [hamlet], acting_user=None)
         check_add_user_group(hamlet.realm, "marketing", [hamlet], acting_user=None)
+        check_add_user_group(hamlet.realm, "leadership", [hamlet], acting_user=None)
 
         for setting_name in NamedUserGroup.GROUP_PERMISSION_SETTINGS:
             self.do_test_update_user_group_permission_settings(setting_name)
@@ -1547,6 +1613,19 @@ class UserGroupAPITestCase(UserGroupTestCase):
         self.assert_json_error(result, msg)
         self.assert_user_membership(user_group, [hamlet])
 
+        # Test adding or removing members from a deactivated group.
+        do_deactivate_user_group(user_group, acting_user=None)
+
+        params = {"delete": orjson.dumps([hamlet.id]).decode()}
+        result = self.client_post(f"/json/user_groups/{user_group.id}/members", info=params)
+        self.assert_json_error(result, "User group is deactivated.")
+        self.assert_user_membership(user_group, [hamlet])
+
+        params = {"add": orjson.dumps([iago.id]).decode()}
+        result = self.client_post(f"/json/user_groups/{user_group.id}/members", info=params)
+        self.assert_json_error(result, "User group is deactivated.")
+        self.assert_user_membership(user_group, [hamlet])
+
     def test_mentions(self) -> None:
         cordelia = self.example_user("cordelia")
         hamlet = self.example_user("hamlet")
@@ -2434,6 +2513,31 @@ class UserGroupAPITestCase(UserGroupTestCase):
         self.assert_json_error(result, 'Nothing to do. Specify at least one of "add" or "delete".')
         self.assert_subgroup_membership(support_group, [leadership_group])
 
+        # Do not have support group as subgroup of any group to follow
+        # the condition a group used as a subgroup cannot be deactivated.
+        params = {"delete": orjson.dumps([support_group.id]).decode()}
+        result = self.client_post(f"/json/user_groups/{test_group.id}/subgroups", info=params)
+        self.assert_json_success(result)
+        self.assert_subgroup_membership(test_group, [])
+
+        # Test adding or removing subgroups from a deactivated group.
+        do_deactivate_user_group(support_group, acting_user=None)
+
+        params = {"delete": orjson.dumps([leadership_group.id]).decode()}
+        result = self.client_post(f"/json/user_groups/{support_group.id}/subgroups", info=params)
+        self.assert_json_error(result, "User group is deactivated.")
+        self.assert_subgroup_membership(support_group, [leadership_group])
+
+        params = {"add": orjson.dumps([test_group.id]).decode()}
+        result = self.client_post(f"/json/user_groups/{support_group.id}/subgroups", info=params)
+        self.assert_json_error(result, "User group is deactivated.")
+        self.assert_subgroup_membership(support_group, [leadership_group])
+
+        # Test that a deactivated group cannot be used as a subgroup.
+        params = {"add": orjson.dumps([support_group.id]).decode()}
+        result = self.client_post(f"/json/user_groups/{test_group.id}/subgroups", info=params)
+        self.assert_json_error(result, "User group is deactivated.")
+
     def test_get_is_user_group_member_status(self) -> None:
         self.login("iago")
         realm = get_realm("zulip")

zerver/views/user_groups.py

@@ -120,7 +120,14 @@ def edit_user_group(
     ):
         raise JsonableError(_("No new data supplied"))
 
-    user_group = access_user_group_by_id(user_group_id, user_profile, for_read=False)
+    user_group = access_user_group_by_id(
+        user_group_id, user_profile, for_read=False, allow_deactivated=True
+    )
+
+    if user_group.deactivated and (
+        description is not None or can_mention_group is not None or can_manage_group is not None
+    ):
+        raise JsonableError(_("You can only change name of deactivated user groups"))
 
     if name is not None and name != user_group.name:
         name = check_user_group_name(name)