diff --git a/zerver/lib/user_groups.py b/zerver/lib/user_groups.py index 2960b72b55..7132abcbc1 100644 --- a/zerver/lib/user_groups.py +++ b/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 diff --git a/zerver/tests/test_user_groups.py b/zerver/tests/test_user_groups.py index 323b04ef1d..1409761329 100644 --- a/zerver/tests/test_user_groups.py +++ b/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 diff --git a/zerver/views/user_groups.py b/zerver/views/user_groups.py index 8c459b6fec..75539284b0 100644 --- a/zerver/views/user_groups.py +++ b/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)