user_groups: Prevent cycles when adding subgroups for a user group.

The user group depedency graph should always be a DAG.
This commit adds code to make sure we keep the graph DAG
while adding subgroups to a user group.

Fixes #25913.

zerver/lib/user_groups.py

@@ -216,6 +216,16 @@ def get_subgroup_ids(user_group: UserGroup, *, direct_subgroup_only: bool = Fals
     return list(subgroup_ids)
 
 
+def get_recursive_subgroups_for_groups(user_group_ids: List[int]) -> List[int]:
+    cte = With.recursive(
+        lambda cte: UserGroup.objects.filter(id__in=user_group_ids)
+        .values(group_id=F("id"))
+        .union(cte.join(UserGroup, direct_supergroups=cte.col.group_id).values(group_id=F("id")))
+    )
+    recursive_subgroups = cte.join(UserGroup, id=cte.col.group_id).with_cte(cte)
+    return list(recursive_subgroups.values_list("id", flat=True))
+
+
 @transaction.atomic(savepoint=False)
 def create_system_user_groups_for_realm(realm: Realm) -> Dict[int, UserGroup]:
     """Any changes to this function likely require a migration to adjust

zerver/tests/test_user_groups.py

@@ -905,6 +905,7 @@ class UserGroupAPITestCase(UserGroupTestCase):
             realm, "leadership", [desdemona, iago, hamlet], acting_user=None
         )
         support_group = check_add_user_group(realm, "support", [hamlet, othello], acting_user=None)
+        test_group = check_add_user_group(realm, "test", [hamlet], acting_user=None)
 
         self.login("cordelia")
         # Non-admin and non-moderators who are not a member of group cannot add or remove subgroups.
@@ -968,6 +969,28 @@ class UserGroupAPITestCase(UserGroupTestCase):
             ),
         )
 
+        self.login("iago")
+        params = {"add": orjson.dumps([support_group.id]).decode()}
+        result = self.client_post(f"/json/user_groups/{leadership_group.id}/subgroups", info=params)
+        self.assert_json_error(
+            result,
+            (
+                "User group {user_group_id} is already a subgroup of one of the passed subgroups."
+            ).format(user_group_id=leadership_group.id),
+        )
+
+        params = {"add": orjson.dumps([support_group.id]).decode()}
+        result = self.client_post(f"/json/user_groups/{test_group.id}/subgroups", info=params)
+
+        params = {"add": orjson.dumps([test_group.id]).decode()}
+        result = self.client_post(f"/json/user_groups/{leadership_group.id}/subgroups", info=params)
+        self.assert_json_error(
+            result,
+            (
+                "User group {user_group_id} is already a subgroup of one of the passed subgroups."
+            ).format(user_group_id=leadership_group.id),
+        )
+
         lear_realm = get_realm("lear")
         lear_test_group = check_add_user_group(
             lear_realm, "test", [self.lear_user("cordelia")], acting_user=None

zerver/views/user_groups.py

@@ -25,6 +25,7 @@ from zerver.lib.user_groups import (
     access_user_group_by_id,
     access_user_groups_as_potential_subgroups,
     get_direct_memberships_of_users,
+    get_recursive_subgroups_for_groups,
     get_subgroup_ids,
     get_user_group_direct_member_ids,
     get_user_group_member_ids,
@@ -238,6 +239,14 @@ def add_subgroups_to_group_backend(
                 )
             )
 
+    subgroup_ids = [group.id for group in subgroups]
+    if user_group_id in get_recursive_subgroups_for_groups(subgroup_ids):
+        raise JsonableError(
+            _(
+                "User group {user_group_id} is already a subgroup of one of the passed subgroups."
+            ).format(user_group_id=user_group_id)
+        )
+
     add_subgroups_to_user_group(user_group, subgroups, acting_user=user_profile)
     return json_success(request)