diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 3e1412ff08..53c5b4ccc4 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -24,6 +24,13 @@ format used by the Zulip server that they are interacting with. * [`GET /events`](/api/get-events): User reactivation event is not sent to users who cannot access the reactivated user anymore. +* [`GET /events`](/api/get-events): `add_members` and `remove_members` + events with `user_group` type can now also be sent when reactivating + and deactivating a user respectively. +* [`POST /register`](/api/register-queue), [`GET /user_groups`](/api/get-user-groups), + [`GET /user_groups/{user_group_id}/members/{user_id}`](/api/get-is-user-group-member), + [`GET /user_groups/{user_group_id}/members`](/api/get-user-group-members): + Deactivated users are not considered as members of a user group anymore. **Feature level 302** diff --git a/zerver/actions/create_user.py b/zerver/actions/create_user.py index 9b6527690a..6604aca564 100644 --- a/zerver/actions/create_user.py +++ b/zerver/actions/create_user.py @@ -771,3 +771,11 @@ def do_reactivate_user(user_profile: UserProfile, *, acting_user: UserProfile | stream_dict=stream_dict, subscriber_peer_info=subscriber_peer_info, ) + + member_user_groups = user_profile.direct_groups.exclude(named_user_group=None).select_related( + "named_user_group" + ) + for user_group in member_user_groups: + do_send_user_group_members_update_event( + "add_members", user_group.named_user_group, [user_profile.id] + ) diff --git a/zerver/actions/users.py b/zerver/actions/users.py index 84fae51ddd..0f76bc5217 100644 --- a/zerver/actions/users.py +++ b/zerver/actions/users.py @@ -311,6 +311,30 @@ def send_events_for_user_deactivation(user_profile: UserProfile) -> None: realm, event_deactivate_user, list(users_with_access_to_deactivated_user) ) + all_active_user_ids = active_user_ids(realm.id) + users_without_access_to_deactivated_user = ( + set(all_active_user_ids) - users_with_access_to_deactivated_user + ) + if users_without_access_to_deactivated_user: + # Guests who have access to the deactivated user receive + # 'realm_user/update' event and can update the user groups + # data, but guests who cannot access the deactivated user + # need an explicit 'user_group/remove_members' event to + # update the user groups data. + deactivated_user_groups = user_profile.direct_groups.exclude( + named_user_group=None + ).select_related("named_user_group") + for user_group in deactivated_user_groups: + event = dict( + type="user_group", + op="remove_members", + group_id=user_group.id, + user_ids=[user_profile.id], + ) + send_event_on_commit( + user_group.realm, event, list(users_without_access_to_deactivated_user) + ) + users_losing_access_to_deactivated_user = ( peer_stream_subscribers - users_with_access_to_deactivated_user ) diff --git a/zerver/lib/events.py b/zerver/lib/events.py index fb0b5eb382..f042a15a68 100644 --- a/zerver/lib/events.py +++ b/zerver/lib/events.py @@ -1092,18 +1092,26 @@ def apply_event( if "new_email" in person: p["email"] = person["new_email"] - if "is_active" in person and not person["is_active"] and include_subscribers: - for sub_dict in [ - state["subscriptions"], - state["unsubscribed"], - state["never_subscribed"], - ]: - for sub in sub_dict: - sub["subscribers"] = [ - user_id - for user_id in sub["subscribers"] - if user_id != person_user_id - ] + if "is_active" in person and not person["is_active"]: + if include_subscribers: + for sub_dict in [ + state["subscriptions"], + state["unsubscribed"], + state["never_subscribed"], + ]: + for sub in sub_dict: + sub["subscribers"] = [ + user_id + for user_id in sub["subscribers"] + if user_id != person_user_id + ] + + for user_group in state["realm_user_groups"]: + user_group["members"] = [ + user_id + for user_id in user_group["members"] + if user_id != person_user_id + ] elif event["op"] == "remove": if person_user_id in state["raw_users"]: if user_list_incomplete: diff --git a/zerver/lib/user_groups.py b/zerver/lib/user_groups.py index d4609ba563..f11061053f 100644 --- a/zerver/lib/user_groups.py +++ b/zerver/lib/user_groups.py @@ -563,8 +563,10 @@ def user_groups_in_realm_serialized( if not include_deactivated_groups: realm_groups = realm_groups.filter(deactivated=False) - membership = UserGroupMembership.objects.filter(user_group__realm=realm).values_list( - "user_group_id", "user_profile_id" + membership = ( + UserGroupMembership.objects.filter(user_group__realm=realm) + .exclude(user_profile__is_active=False, user_group__named_user_group__isnull=False) + .values_list("user_group_id", "user_profile_id") ) group_membership = GroupGroupMembership.objects.filter(subgroup__realm=realm).values_list( @@ -632,16 +634,19 @@ def get_direct_user_groups(user_profile: UserProfile) -> list[UserGroup]: def get_user_group_direct_member_ids( user_group: UserGroup, ) -> QuerySet[UserGroupMembership, int]: - return UserGroupMembership.objects.filter(user_group=user_group).values_list( - "user_profile_id", flat=True - ) + return UserGroupMembership.objects.filter( + user_group=user_group, user_profile__is_active=True + ).values_list("user_profile_id", flat=True) def get_user_group_direct_members(user_group: UserGroup) -> QuerySet[UserProfile]: - return user_group.direct_members.all() + return user_group.direct_members.filter(is_active=True) def get_direct_memberships_of_users(user_group: UserGroup, members: list[UserProfile]) -> list[int]: + # Returns the subset of the provided members list who are direct subscribers of the group. + # If a deactivated user is passed, it will be returned if the user was a member of the + # group when deactivated. return list( UserGroupMembership.objects.filter( user_group=user_group, user_profile__in=members @@ -684,7 +689,9 @@ def get_recursive_strict_subgroups(user_group: UserGroup) -> QuerySet[NamedUserG def get_recursive_group_members(user_group: UserGroup) -> QuerySet[UserProfile]: - return UserProfile.objects.filter(direct_groups__in=get_recursive_subgroups(user_group)) + return UserProfile.objects.filter( + is_active=True, direct_groups__in=get_recursive_subgroups(user_group) + ) def get_recursive_membership_groups(user_profile: UserProfile) -> QuerySet[UserGroup]: diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index bdaec6e2b2..a6d8f6244d 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -3288,6 +3288,12 @@ paths: additionalProperties: false description: | Event sent to all users when users have been added to a user group. + + This event is also sent when reactivating a user for all the user + groups the reactivated user was a member of before being deactivated. + + **Changes**: Starting with Zulip 10.0 (feature level 303), this + event can also be sent when reactivating a user. properties: id: $ref: "#/components/schemas/EventIdSchema" @@ -3324,6 +3330,13 @@ paths: description: | Event sent to all users when users have been removed from a user group. + + This event is also sent when deactivating a user, for all + the user groups the deactivated user is a member of, but only + to the users who cannot access the deactivated user. + + **Changes**: Starting with Zulip 10.0 (feature level 303), + this event can also be sent when deactivating a user. properties: id: $ref: "#/components/schemas/EventIdSchema" @@ -20596,6 +20609,10 @@ paths: type: array description: | The integer user IDs of the user group members. + + **Changes**: Prior to Zulip 10.0 (feature level 303), this + list also included deactivated users who were members of + the user group before being deactivated. items: type: integer direct_subgroup_ids: @@ -20818,7 +20835,11 @@ paths: description: | Check whether a user is member of user group. - **Changes**: New in Zulip 6.0 (feature level 127). + **Changes**: Prior to Zulip 10.0 (feature level 303), + this returned true when the user was deactivated but + was member of the user group before being deactivated. + + New in Zulip 6.0 (feature level 127). parameters: - $ref: "#/components/parameters/UserGroupId" - $ref: "#/components/parameters/UserId" @@ -21940,6 +21961,10 @@ components: description: | Array containing the ID of the users who are members of this user group. + + **Changes**: Prior to Zulip 10.0 (feature level 303), this + list also included deactivated users who were members of + the user group before being deactivated. direct_subgroup_ids: type: array items: diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index a8e2fb3c8d..e577f29d22 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -3057,12 +3057,32 @@ class NormalActionsTest(BaseAction): do_reactivate_user(user_profile, acting_user=None) self.set_up_db_for_testing_user_access() - # Test that guest users receive event only - # if they can access the deactivated user. + # Test that users who can access the deactivated user + # do not receive the 'user_group/remove_members' event. + user_profile = self.example_user("cordelia") + with self.verify_action(num_events=1) as events: + do_deactivate_user(user_profile, acting_user=None) + check_realm_user_update("events[0]", events[0], "is_active") + + do_reactivate_user(user_profile, acting_user=None) + + # Test that guest users receive 'user_group/remove_members' + # event if they cannot access the deactivated user. user_profile = self.example_user("cordelia") self.user_profile = self.example_user("polonius") - with self.verify_action(num_events=0, state_change_expected=False) as events: + with self.verify_action(num_events=3) as events: do_deactivate_user(user_profile, acting_user=None) + check_user_group_remove_members("events[0]", events[0]) + check_user_group_remove_members("events[1]", events[1]) + check_user_group_remove_members("events[2]", events[2]) + + user_profile = self.example_user("cordelia") + do_reactivate_user(user_profile, acting_user=None) + with self.verify_action(num_events=3, user_list_incomplete=True) as events: + do_deactivate_user(user_profile, acting_user=None) + check_user_group_remove_members("events[0]", events[0]) + check_user_group_remove_members("events[1]", events[1]) + check_user_group_remove_members("events[2]", events[2]) user_profile = self.example_user("shiva") with self.verify_action(num_events=1) as events: @@ -3072,9 +3092,12 @@ class NormalActionsTest(BaseAction): # Guest loses access to deactivated user if the user # was not involved in DMs. user_profile = self.example_user("hamlet") - with self.verify_action(num_events=1) as events: + with self.verify_action(num_events=4) as events: do_deactivate_user(user_profile, acting_user=None) - check_realm_user_remove("events[0]", events[0]) + check_user_group_remove_members("events[0]", events[0]) + check_user_group_remove_members("events[1]", events[1]) + check_user_group_remove_members("events[2]", events[2]) + check_realm_user_remove("events[3]]", events[3]) user_profile = self.example_user("aaron") # One update event is for a deactivating a bot owned by aaron. @@ -3089,15 +3112,17 @@ class NormalActionsTest(BaseAction): self.make_stream("Test private stream", invite_only=True) self.subscribe(bot, "Test private stream") do_deactivate_user(bot, acting_user=None) - with self.verify_action(num_events=3) as events: + with self.verify_action(num_events=5) as events: do_reactivate_user(bot, acting_user=None) check_realm_bot_update("events[1]", events[1], "is_active") check_subscription_peer_add("events[2]", events[2]) + check_user_group_add_members("events[3]", events[3]) + check_user_group_add_members("events[4]", events[4]) # Test 'peer_add' event for private stream is received only if user is subscribed to it. do_deactivate_user(bot, acting_user=None) self.subscribe(self.example_user("hamlet"), "Test private stream") - with self.verify_action(num_events=4) as events: + with self.verify_action(num_events=6) as events: do_reactivate_user(bot, acting_user=None) check_realm_bot_update("events[1]", events[1], "is_active") check_subscription_peer_add("events[2]", events[2]) @@ -3110,7 +3135,7 @@ class NormalActionsTest(BaseAction): bot.refresh_from_db() self.user_profile = self.example_user("iago") - with self.verify_action(num_events=7) as events: + with self.verify_action(num_events=9) as events: do_reactivate_user(bot, acting_user=self.example_user("iago")) check_realm_bot_update("events[1]", events[1], "is_active") check_realm_bot_update("events[2]", events[2], "owner_id") @@ -3119,20 +3144,26 @@ class NormalActionsTest(BaseAction): check_stream_delete("events[5]", events[5]) self.set_up_db_for_testing_user_access() - # Test that guest users receive event only - # if they can access the reactivated user. + # Test that guest users receive realm_user/update event + # only if they can access the reactivated user. user_profile = self.example_user("cordelia") do_deactivate_user(user_profile, acting_user=None) self.user_profile = self.example_user("polonius") - with self.verify_action(num_events=0, state_change_expected=False) as events: + # Guest users receives group members update event for three groups - + # members group, full members group and hamletcharacters group. + with self.verify_action(num_events=3) as events: do_reactivate_user(user_profile, acting_user=None) + check_user_group_add_members("events[0]", events[0]) + check_user_group_add_members("events[1]", events[1]) + check_user_group_add_members("events[2]", events[2]) user_profile = self.example_user("shiva") do_deactivate_user(user_profile, acting_user=None) - with self.verify_action(num_events=1) as events: + with self.verify_action(num_events=2) as events: do_reactivate_user(user_profile, acting_user=None) check_realm_user_update("events[0]", events[0], "is_active") + check_user_group_add_members("events[1]", events[1]) def test_do_deactivate_realm(self) -> None: realm = self.user_profile.realm diff --git a/zerver/tests/test_user_groups.py b/zerver/tests/test_user_groups.py index 7ad85c380b..779a7085ee 100644 --- a/zerver/tests/test_user_groups.py +++ b/zerver/tests/test_user_groups.py @@ -91,6 +91,7 @@ class UserGroupTestCase(ZulipTestCase): user_group = NamedUserGroup.objects.filter(realm=realm).first() assert user_group is not None empty_user_group = check_add_user_group(realm, "newgroup", [], acting_user=user) + do_deactivate_user(self.example_user("hamlet"), acting_user=None) user_groups = user_groups_in_realm_serialized(realm, include_deactivated_groups=False) self.assert_length(user_groups, 10) @@ -133,6 +134,10 @@ class UserGroupTestCase(ZulipTestCase): # Check that owners system group is present in "direct_subgroup_ids" self.assertEqual(user_groups[2]["direct_subgroup_ids"], [owners_system_group.id]) + self.assertEqual(user_groups[8]["name"], "hamletcharacters") + # Test deactivated user is not included in the members list. + self.assertEqual(user_groups[8]["members"], [self.example_user("cordelia").id]) + everyone_group = NamedUserGroup.objects.get( name=SystemGroups.EVERYONE, realm=realm, is_system_group=True ) @@ -286,6 +291,10 @@ class UserGroupTestCase(ZulipTestCase): self.assertIn(everyone_group.usergroup_ptr, get_recursive_membership_groups(shiva)) + do_deactivate_user(iago, acting_user=None) + self.assertCountEqual(list(get_recursive_group_members(staff_group)), [desdemona]) + self.assertCountEqual(list(get_recursive_group_members(everyone_group)), [desdemona, shiva]) + def test_subgroups_of_role_based_system_groups(self) -> None: realm = get_realm("zulip") owners_group = NamedUserGroup.objects.get( @@ -371,6 +380,10 @@ class UserGroupTestCase(ZulipTestCase): self.assertFalse(is_user_in_group(moderators_group, hamlet)) self.assertFalse(is_user_in_group(moderators_group, hamlet, direct_member_only=True)) + do_deactivate_user(iago, acting_user=None) + self.assertFalse(is_user_in_group(moderators_group, iago)) + self.assertFalse(is_user_in_group(administrators_group, iago, direct_member_only=True)) + def test_is_any_user_in_group(self) -> None: realm = get_realm("zulip") shiva = self.example_user("shiva").id @@ -1685,7 +1698,7 @@ class UserGroupAPITestCase(UserGroupTestCase): 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) - self.assert_user_membership(user_group, [hamlet, othello, desdemona, iago, webhook_bot]) + self.assert_user_membership(user_group, [hamlet, othello, desdemona, webhook_bot]) # No notification message is sent for adding to user group. self.assertEqual(self.get_last_message(), initial_last_message) @@ -1698,7 +1711,7 @@ class UserGroupAPITestCase(UserGroupTestCase): 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) - self.assert_user_membership(user_group, [hamlet, desdemona, iago, webhook_bot]) + self.assert_user_membership(user_group, [hamlet, desdemona, webhook_bot]) # A notification message is sent for removing from user group. self.assertNotEqual(self.get_last_message(), initial_last_message) @@ -1711,13 +1724,13 @@ class UserGroupAPITestCase(UserGroupTestCase): params = {"delete": orjson.dumps([othello.id]).decode()} result = self.client_post(f"/json/user_groups/{user_group.id}/members", info=params) self.assert_json_error(result, f"There is no member '{othello.id}' in this user group") - self.assert_user_membership(user_group, [hamlet, desdemona, iago, webhook_bot]) + self.assert_user_membership(user_group, [hamlet, desdemona, webhook_bot]) # Test user remove itself,bot and deactivated user from user group. desdemona = self.example_user("desdemona") self.login_user(desdemona) - params = {"delete": orjson.dumps([desdemona.id, iago.id, webhook_bot.id]).decode()} + 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) self.assert_json_success(result) @@ -2765,6 +2778,11 @@ class UserGroupAPITestCase(UserGroupTestCase): ) self.assertFalse(result_dict["is_user_group_member"]) + # Check membership of deactivated user. + do_deactivate_user(iago, acting_user=None) + result = self.client_get(f"/json/user_groups/{admins_group.id}/members/{iago.id}") + self.assert_json_error(result, "User is deactivated") + def test_get_user_group_members(self) -> None: realm = get_realm("zulip") iago = self.example_user("iago") @@ -2811,6 +2829,19 @@ class UserGroupAPITestCase(UserGroupTestCase): ) self.assertCountEqual(result_dict["members"], [shiva.id]) + # Check deactivated users are not returned in members list. + do_deactivate_user(shiva, acting_user=None) + result_dict = orjson.loads( + self.client_get(f"/json/user_groups/{moderators_group.id}/members", info=params).content + ) + self.assertCountEqual(result_dict["members"], []) + + params = {"direct_member_only": orjson.dumps(False).decode()} + result_dict = orjson.loads( + self.client_get(f"/json/user_groups/{moderators_group.id}/members", info=params).content + ) + self.assertCountEqual(result_dict["members"], [desdemona.id, iago.id]) + def test_get_subgroups_of_user_group(self) -> None: realm = get_realm("zulip") owners_group = NamedUserGroup.objects.get(