From 4784c71bf9a658733ee3ca3d75dee60f081df23d Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Thu, 28 Mar 2024 19:04:47 +0530 Subject: [PATCH] user_groups: Do not allow updating memberships of deactivated users. This commit updates backend code to not allow adding deactivated users to groups including when creating groups and also to not allow removing deactivated users from groups. --- api_docs/changelog.md | 2 ++ version.py | 2 +- zerver/lib/user_groups.py | 2 +- zerver/lib/users.py | 13 +++++++------ zerver/openapi/zulip.yaml | 3 +++ zerver/tests/test_user_groups.py | 17 +++++++++++++++++ zerver/tests/test_users.py | 24 ++++++++++++++++++++---- zerver/views/user_groups.py | 11 +++++------ 8 files changed, 56 insertions(+), 18 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index e62cd51daa..5824b41a12 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -38,6 +38,8 @@ format used by the Zulip server that they are interacting with. * [`POST /register`](/api/register-queue): Settings, represented as [group-setting value](/api/group-setting-values), do not include deactivated users in the `direct_members` list for settings set to anonymous groups. +* [`PATCH /user_groups/{user_group_id}/members`](/api/update-user-group-members): + Deactivated users cannot be added or removed from a user group. **Feature level 302** diff --git a/version.py b/version.py index 7f82cc7a1b..3bc9fc6114 100644 --- a/version.py +++ b/version.py @@ -34,7 +34,7 @@ DESKTOP_WARNING_VERSION = "5.9.3" # new level means in api_docs/changelog.md, as well as "**Changes**" # entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 302 # Last bumped for changes to {email} requirements for GET /users/{email} +API_FEATURE_LEVEL = 303 # Last bumped for handling deactivated users in groups. # Bump the minor PROVISION_VERSION to indicate that folks should provision # only when going from an old version of the code to a newer version. Bump diff --git a/zerver/lib/user_groups.py b/zerver/lib/user_groups.py index b2989005b6..2ef2a04e13 100644 --- a/zerver/lib/user_groups.py +++ b/zerver/lib/user_groups.py @@ -427,7 +427,7 @@ def update_or_create_user_group_for_setting( from zerver.lib.users import user_ids_to_users - member_users = user_ids_to_users(direct_members, realm) + member_users = user_ids_to_users(direct_members, realm, allow_deactivated=False) user_group.direct_members.set(member_users) potential_subgroups = NamedUserGroup.objects.select_for_update().filter( diff --git a/zerver/lib/users.py b/zerver/lib/users.py index 06f20eb233..c5e1a9d68e 100644 --- a/zerver/lib/users.py +++ b/zerver/lib/users.py @@ -234,13 +234,14 @@ def bulk_get_cross_realm_bots() -> dict[str, UserProfile]: return {user.email.lower(): user for user in users} -def user_ids_to_users(user_ids: Sequence[int], realm: Realm) -> list[UserProfile]: - # TODO: Consider adding a flag to control whether deactivated - # users should be included. +def user_ids_to_users( + user_ids: Sequence[int], realm: Realm, *, allow_deactivated: bool +) -> list[UserProfile]: + user_query = UserProfile.objects.filter(id__in=user_ids, realm=realm) + if not allow_deactivated: + user_query = user_query.filter(is_active=True) - user_profiles = list( - UserProfile.objects.filter(id__in=user_ids, realm=realm).select_related("realm") - ) + user_profiles = list(user_query.select_related("realm")) found_user_ids = {user_profile.id for user_profile in user_profiles} diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index bb3d174713..bed1a42cb6 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -20288,6 +20288,9 @@ paths: tags: ["users"] description: | Update the members of a [user group](/help/user-groups). + + **Changes**: Prior to Zulip 10.0 (feature level 303), deactivated + users could also be added or removed from a user group. x-curl-examples-parameters: oneOf: - type: exclude diff --git a/zerver/tests/test_user_groups.py b/zerver/tests/test_user_groups.py index 779a7085ee..dc63310886 100644 --- a/zerver/tests/test_user_groups.py +++ b/zerver/tests/test_user_groups.py @@ -7,6 +7,7 @@ import time_machine from django.utils.timezone import now as timezone_now from zerver.actions.create_realm import do_create_realm +from zerver.actions.create_user import do_reactivate_user from zerver.actions.realm_settings import ( do_change_realm_permission_group_setting, do_set_realm_property, @@ -1695,6 +1696,10 @@ class UserGroupAPITestCase(UserGroupTestCase): self.login_user(desdemona) params = {"add": orjson.dumps([desdemona.id, iago.id, webhook_bot.id]).decode()} + result = self.client_post(f"/json/user_groups/{user_group.id}/members", info=params) + self.assert_json_error(result, f"Invalid user ID: {iago.id}") + + params = {"add": orjson.dumps([desdemona.id, webhook_bot.id]).decode()} initial_last_message = self.get_last_message() result = self.client_post(f"/json/user_groups/{user_group.id}/members", info=params) self.assert_json_success(result) @@ -1730,6 +1735,18 @@ class UserGroupAPITestCase(UserGroupTestCase): desdemona = self.example_user("desdemona") self.login_user(desdemona) + # Add user to group after reactivation to test removing deactivated user. + do_reactivate_user(iago, acting_user=None) + self.client_post( + f"/json/user_groups/{user_group.id}/members", + info={"add": orjson.dumps([iago.id]).decode()}, + ) + do_deactivate_user(iago, acting_user=None) + + params = {"delete": orjson.dumps([iago.id, desdemona.id, webhook_bot.id]).decode()} + result = self.client_post(f"/json/user_groups/{user_group.id}/members", info=params) + self.assert_json_error(result, f"Invalid user ID: {iago.id}") + params = {"delete": orjson.dumps([desdemona.id, webhook_bot.id]).decode()} initial_last_message = self.get_last_message() result = self.client_post(f"/json/user_groups/{user_group.id}/members", info=params) diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index 2552315a84..cb51ea802d 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -1327,18 +1327,34 @@ class UserProfileTest(ZulipTestCase): self.example_user("cordelia").id, ] - self.assertEqual(user_ids_to_users([], get_realm("zulip")), []) + self.assertEqual(user_ids_to_users([], get_realm("zulip"), allow_deactivated=False), []) self.assertEqual( { user_profile.id - for user_profile in user_ids_to_users(real_user_ids, get_realm("zulip")) + for user_profile in user_ids_to_users( + real_user_ids, get_realm("zulip"), allow_deactivated=False + ) }, set(real_user_ids), ) with self.assertRaises(JsonableError): - user_ids_to_users([1234], get_realm("zephyr")) + user_ids_to_users([1234], get_realm("zephyr"), allow_deactivated=False) with self.assertRaises(JsonableError): - user_ids_to_users(real_user_ids, get_realm("zephyr")) + user_ids_to_users(real_user_ids, get_realm("zephyr"), allow_deactivated=False) + + do_deactivate_user(self.example_user("hamlet"), acting_user=None) + with self.assertRaises(JsonableError): + user_ids_to_users(real_user_ids, get_realm("zulip"), allow_deactivated=False) + + self.assertEqual( + { + user_profile.id + for user_profile in user_ids_to_users( + real_user_ids, get_realm("zulip"), allow_deactivated=True + ) + }, + set(real_user_ids), + ) def test_get_accounts_for_email(self) -> None: reset_email_visibility_to_everyone_in_zulip_realm() diff --git a/zerver/views/user_groups.py b/zerver/views/user_groups.py index bfac539f31..494261be38 100644 --- a/zerver/views/user_groups.py +++ b/zerver/views/user_groups.py @@ -61,7 +61,7 @@ def add_user_group( can_manage_group: Json[int | AnonymousSettingGroupDict] | None = None, can_mention_group: Json[int | AnonymousSettingGroupDict] | None = None, ) -> HttpResponse: - user_profiles = user_ids_to_users(members, user_profile.realm) + user_profiles = user_ids_to_users(members, user_profile.realm, allow_deactivated=False) name = check_user_group_name(name) group_settings_map = {} @@ -254,9 +254,8 @@ def notify_for_user_group_subscription_changes( if recipient_user.is_bot: # Don't send notification message to bots. continue - if not recipient_user.is_active: - # Don't send notification message to deactivated users. - continue + + assert recipient_user.is_active with override_language(recipient_user.default_language): if send_subscription_message: @@ -306,7 +305,7 @@ def add_members_to_group_backend( user_group_id, user_profile, permission_setting="can_manage_group" ) - member_users = user_ids_to_users(members, user_profile.realm) + member_users = user_ids_to_users(members, user_profile.realm, allow_deactivated=False) existing_member_ids = set( get_direct_memberships_of_users(user_group.usergroup_ptr, member_users) ) @@ -337,7 +336,7 @@ def remove_members_from_group_backend( user_group_id: int, members: list[int], ) -> HttpResponse: - user_profiles = user_ids_to_users(members, user_profile.realm) + user_profiles = user_ids_to_users(members, user_profile.realm, allow_deactivated=False) user_group = access_user_group_for_update( user_group_id, user_profile, permission_setting="can_manage_group" )