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

@@ -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**
 

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

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(

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}
 

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

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)

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()

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"
     )