From c1d6da0a5c2c6a52b2bb6ac84007dabd2fc9a10a Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Thu, 16 May 2024 19:30:25 +0530 Subject: [PATCH] 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 | 45 ++++++++++--- zerver/tests/test_user_groups.py | 104 +++++++++++++++++++++++++++++++ zerver/views/user_groups.py | 9 ++- 3 files changed, 149 insertions(+), 9 deletions(-) diff --git a/zerver/lib/user_groups.py b/zerver/lib/user_groups.py index 952f542884..fdc4afc647 100644 --- a/zerver/lib/user_groups.py +++ b/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 - # subgroup. This would be caught by the check earlier, so there is - # no coverage here. + # 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, 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, diff --git a/zerver/tests/test_user_groups.py b/zerver/tests/test_user_groups.py index 95163de6ae..0531bcf678 100644 --- a/zerver/tests/test_user_groups.py +++ b/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") diff --git a/zerver/views/user_groups.py b/zerver/views/user_groups.py index 25f316481c..bbbbccac51 100644 --- a/zerver/views/user_groups.py +++ b/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)