From 0b58820294c385351b7f29e901358008ab770e38 Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Mon, 23 Sep 2024 19:45:17 +0530 Subject: [PATCH] user_groups: Do not include deactivated users in anonymous group settings. This commit updates code to not include deactivated users in the anonymous group settings data sent to clients, where the setting value is sent as a dict containing members and subgroups of the anonymous group. --- api_docs/changelog.md | 7 ++ zerver/actions/create_user.py | 28 ++++++-- zerver/actions/users.py | 128 ++++++++++++++++++++++++++++++++-- zerver/lib/events.py | 17 +++++ zerver/lib/user_groups.py | 6 +- zerver/openapi/zulip.yaml | 26 +++++-- zerver/tests/test_events.py | 115 ++++++++++++++++++++++++++++-- 7 files changed, 305 insertions(+), 22 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 53c5b4ccc4..e62cd51daa 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -31,6 +31,13 @@ format used by the Zulip server that they are interacting with. [`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. +* [`GET /events`](/api/get-events): `realm/update_dict` and `user_group/update` + events are now also sent when deactivating or reactivating a user, for + settings which are set to anonymous groups having the user being deactivated + or reactivated as direct member. +* [`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. **Feature level 302** diff --git a/zerver/actions/create_user.py b/zerver/actions/create_user.py index 6604aca564..cba54d754c 100644 --- a/zerver/actions/create_user.py +++ b/zerver/actions/create_user.py @@ -18,7 +18,11 @@ from zerver.actions.message_send import ( ) from zerver.actions.streams import bulk_add_subscriptions, send_peer_subscriber_events from zerver.actions.user_groups import do_send_user_group_members_update_event -from zerver.actions.users import change_user_is_active, get_service_dicts_for_bot +from zerver.actions.users import ( + change_user_is_active, + get_service_dicts_for_bot, + send_update_events_for_anonymous_group_settings, +) from zerver.lib.avatar import avatar_url from zerver.lib.create_user import create_user from zerver.lib.default_streams import get_slim_realm_default_streams @@ -60,7 +64,7 @@ from zerver.models import ( ) from zerver.models.groups import SystemGroups from zerver.models.realm_audit_logs import AuditLogEventType -from zerver.models.users import bot_owner_user_ids, get_system_bot +from zerver.models.users import active_user_ids, bot_owner_user_ids, get_system_bot from zerver.tornado.django_api import send_event_on_commit MAX_NUM_RECENT_MESSAGES = 1000 @@ -772,10 +776,22 @@ def do_reactivate_user(user_profile: UserProfile, *, acting_user: UserProfile | 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: + member_user_groups = user_profile.direct_groups.select_related("named_user_group") + named_user_groups = [] + setting_user_groups = [] + for group in member_user_groups: + if hasattr(group, "named_user_group"): + named_user_groups.append(group) + else: + setting_user_groups.append(group) + + for user_group in named_user_groups: do_send_user_group_members_update_event( "add_members", user_group.named_user_group, [user_profile.id] ) + + if setting_user_groups: + notify_user_ids = active_user_ids(user_profile.realm_id) + send_update_events_for_anonymous_group_settings( + setting_user_groups, user_profile.realm, list(notify_user_ids) + ) diff --git a/zerver/actions/users.py b/zerver/actions/users.py index 0f76bc5217..bb7af6461c 100644 --- a/zerver/actions/users.py +++ b/zerver/actions/users.py @@ -6,6 +6,7 @@ from typing import Any from django.conf import settings from django.contrib.auth.tokens import PasswordResetTokenGenerator, default_token_generator from django.db import transaction +from django.db.models import Q from django.http import HttpRequest from django.urls import reverse from django.utils.http import urlsafe_base64_encode @@ -29,7 +30,7 @@ from zerver.lib.stream_subscription import bulk_get_subscriber_peer_info from zerver.lib.stream_traffic import get_streams_traffic from zerver.lib.streams import get_streams_for_user, stream_to_dict from zerver.lib.user_counts import realm_user_count_by_role -from zerver.lib.user_groups import get_system_user_group_for_user +from zerver.lib.user_groups import AnonymousSettingGroupDict, get_system_user_group_for_user from zerver.lib.users import ( get_active_bots_owned_by_user, get_user_ids_who_can_access_user, @@ -37,13 +38,16 @@ from zerver.lib.users import ( user_access_restricted_in_realm, ) from zerver.models import ( + GroupGroupMembership, Message, + NamedUserGroup, Realm, RealmAuditLog, Recipient, Service, Stream, Subscription, + UserGroup, UserGroupMembership, UserProfile, ) @@ -262,6 +266,108 @@ def change_user_is_active(user_profile: UserProfile, value: bool) -> None: Subscription.objects.filter(user_profile=user_profile).update(is_user_active=value) +def send_group_update_event_for_anonymous_group_setting( + setting_group: UserGroup, + group_members_dict: dict[int, list[int]], + group_subgroups_dict: dict[int, list[int]], + named_group: NamedUserGroup, + notify_user_ids: list[int], +) -> None: + realm = setting_group.realm + for setting_name in NamedUserGroup.GROUP_PERMISSION_SETTINGS: + if getattr(named_group, setting_name + "_id") == setting_group.id: + new_setting_value = AnonymousSettingGroupDict( + direct_members=group_members_dict[setting_group.id], + direct_subgroups=group_subgroups_dict[setting_group.id], + ) + event = dict( + type="user_group", + op="update", + group_id=named_group.id, + data={setting_name: new_setting_value}, + ) + send_event_on_commit(realm, event, notify_user_ids) + return + + +def send_realm_update_event_for_anonymous_group_setting( + setting_group: UserGroup, + group_members_dict: dict[int, list[int]], + group_subgroups_dict: dict[int, list[int]], + notify_user_ids: list[int], +) -> None: + realm = setting_group.realm + for setting_name in Realm.REALM_PERMISSION_GROUP_SETTINGS: + if getattr(realm, setting_name + "_id") == setting_group.id: + new_setting_value = AnonymousSettingGroupDict( + direct_members=group_members_dict[setting_group.id], + direct_subgroups=group_subgroups_dict[setting_group.id], + ) + event = dict( + type="realm", + op="update_dict", + property="default", + data={setting_name: new_setting_value}, + ) + send_event_on_commit(realm, event, notify_user_ids) + return + + +def send_update_events_for_anonymous_group_settings( + setting_groups: list[UserGroup], realm: Realm, notify_user_ids: list[int] +) -> None: + setting_group_ids = [group.id for group in setting_groups] + membership = ( + UserGroupMembership.objects.filter(user_group_id__in=setting_group_ids) + .exclude(user_profile__is_active=False) + .values_list("user_group_id", "user_profile_id") + ) + + group_membership = GroupGroupMembership.objects.filter( + supergroup_id__in=setting_group_ids + ).values_list("subgroup_id", "supergroup_id") + + group_members = defaultdict(list) + for user_group_id, user_profile_id in membership: + group_members[user_group_id].append(user_profile_id) + + group_subgroups = defaultdict(list) + for subgroup_id, supergroup_id in group_membership: + group_subgroups[supergroup_id].append(subgroup_id) + + group_setting_query = Q() + for setting_name in NamedUserGroup.GROUP_PERMISSION_SETTINGS: + group_setting_query |= Q(**{f"{setting_name}__in": setting_group_ids}) + + named_groups_using_setting_groups_dict = {} + named_groups_using_setting_groups = NamedUserGroup.objects.filter(realm=realm).filter( + group_setting_query + ) + for group in named_groups_using_setting_groups: + for setting_name in NamedUserGroup.GROUP_PERMISSION_SETTINGS: + setting_value_id = getattr(group, setting_name + "_id") + if setting_value_id in setting_group_ids: + named_groups_using_setting_groups_dict[setting_value_id] = group + + for setting_group in setting_groups: + if setting_group.id in named_groups_using_setting_groups_dict: + named_group = named_groups_using_setting_groups_dict[setting_group.id] + send_group_update_event_for_anonymous_group_setting( + setting_group, + group_members, + group_subgroups, + named_group, + notify_user_ids, + ) + else: + send_realm_update_event_for_anonymous_group_setting( + setting_group, + group_members, + group_subgroups, + notify_user_ids, + ) + + def send_events_for_user_deactivation(user_profile: UserProfile) -> None: event_deactivate_user = dict( type="realm_user", @@ -321,10 +427,15 @@ def send_events_for_user_deactivation(user_profile: UserProfile) -> None: # 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: + deactivated_user_groups = user_profile.direct_groups.select_related("named_user_group") + deactivated_user_named_groups = [] + deactivated_user_setting_groups = [] + for group in deactivated_user_groups: + if not hasattr(group, "named_user_group"): + deactivated_user_setting_groups.append(group) + else: + deactivated_user_named_groups.append(group) + for user_group in deactivated_user_named_groups: event = dict( type="user_group", op="remove_members", @@ -335,6 +446,13 @@ def send_events_for_user_deactivation(user_profile: UserProfile) -> None: user_group.realm, event, list(users_without_access_to_deactivated_user) ) + if deactivated_user_setting_groups: + send_update_events_for_anonymous_group_settings( + deactivated_user_setting_groups, + user_profile.realm, + 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 f042a15a68..6cfcde88d6 100644 --- a/zerver/lib/events.py +++ b/zerver/lib/events.py @@ -80,6 +80,7 @@ from zerver.models import ( Client, CustomProfileField, Draft, + NamedUserGroup, Realm, RealmUserDefault, Recipient, @@ -1112,6 +1113,22 @@ def apply_event( for user_id in user_group["members"] if user_id != person_user_id ] + + for setting_name in Realm.REALM_PERMISSION_GROUP_SETTINGS_WITH_NEW_API_FORMAT: + if not isinstance(state["realm_" + setting_name], int): + state["realm_" + setting_name].direct_members = [ + user_id + for user_id in state["realm_" + setting_name].direct_members + if user_id != person_user_id + ] + for group in state["realm_user_groups"]: + for setting_name in NamedUserGroup.GROUP_PERMISSION_SETTINGS: + if not isinstance(group[setting_name], int): + group[setting_name].direct_members = [ + user_id + for user_id in group[setting_name].direct_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 f11061053f..b2989005b6 100644 --- a/zerver/lib/user_groups.py +++ b/zerver/lib/user_groups.py @@ -516,7 +516,9 @@ def get_group_setting_value_for_api( return setting_value_group.id return AnonymousSettingGroupDict( - direct_members=[member.id for member in setting_value_group.direct_members.all()], + direct_members=[ + member.id for member in setting_value_group.direct_members.filter(is_active=True) + ], direct_subgroups=[subgroup.id for subgroup in setting_value_group.direct_subgroups.all()], ) @@ -565,7 +567,7 @@ def user_groups_in_realm_serialized( membership = ( UserGroupMembership.objects.filter(user_group__realm=realm) - .exclude(user_profile__is_active=False, user_group__named_user_group__isnull=False) + .exclude(user_profile__is_active=False) .values_list("user_group_id", "user_profile_id") ) diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index a6d8f6244d..bb3d174713 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -3182,9 +3182,16 @@ paths: if `include_deactivated_groups` client capability is set to `true`. - **Changes**: Prior to Zulip 10.0 (feature level 294), - this event was sent to all clients when a user group - was deactivated. + This event is also sent when deactivating or reactivating + a user for settings set to anonymous user groups which the + user is direct member of. When deactivating the user, event + is only sent to users who cannot access the deactivated user. + + **Changes**: Starting with Zulip 10.0 (feature level 303), this + event can also be sent when deactivating or reactivating a user. + + Prior to Zulip 10.0 (feature level 294), this event was sent to + all clients when a user group was deactivated. properties: id: $ref: "#/components/schemas/EventIdSchema" @@ -4231,7 +4238,15 @@ paths: event type supports multiple properties being changed in a single event. - **Changes**: In Zulip 7.0 (feature level 163), the realm setting + This event is also sent when deactivating or reactivating a user + for settings set to anonymous user groups which the user is direct + member of. When deactivating the user, event is only sent to users + who cannot access the deactivated user. + + **Changes**: Starting with Zulip 10.0 (feature level 303), this + event can also be sent when deactivating or reactivating a user. + + In Zulip 7.0 (feature level 163), the realm setting `email_address_visibility` was removed. It was replaced by a [user setting](/api/update-settings#parameter-email_address_visibility) with a [realm user default][user-defaults], with the encoding of different @@ -22053,6 +22068,9 @@ components: direct_members: description: | The list of IDs of individual users in the collection of users with this permission. + + **Changes**: Prior to Zulip 10.0 (feature level 303), this list also included + deactivated users. type: array items: type: integer diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index e577f29d22..8e47f27341 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -3050,6 +3050,29 @@ class NormalActionsTest(BaseAction): def test_do_deactivate_user(self) -> None: user_profile = self.example_user("cordelia") + members_group = NamedUserGroup.objects.get( + name=SystemGroups.MEMBERS, realm=user_profile.realm, is_system_group=True + ) + setting_group = self.create_or_update_anonymous_group_for_setting( + [user_profile], [members_group] + ) + do_change_realm_permission_group_setting( + self.user_profile.realm, + "can_create_public_channel_group", + setting_group, + acting_user=None, + ) + hamletcharacters_group = NamedUserGroup.objects.get( + name="hamletcharacters", realm=self.user_profile.realm + ) + hamlet = self.example_user("hamlet") + setting_group = self.create_or_update_anonymous_group_for_setting( + [user_profile, hamlet], [members_group] + ) + do_change_user_group_permission_setting( + hamletcharacters_group, "can_mention_group", setting_group, acting_user=None + ) + 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") @@ -3070,19 +3093,53 @@ class NormalActionsTest(BaseAction): # 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=3) as events: + with self.verify_action(num_events=6) 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]) + check_user_group_update("events[3]", events[3], "can_manage_group") + check_realm_update_dict("events[4]", events[4]) + check_user_group_update("events[5]", events[5], "can_mention_group") + self.assertEqual( + events[3]["data"]["can_manage_group"], + AnonymousSettingGroupDict(direct_members=[], direct_subgroups=[]), + ) + self.assertEqual( + events[4]["data"]["can_create_public_channel_group"], + AnonymousSettingGroupDict(direct_members=[], direct_subgroups=[members_group.id]), + ) + self.assertEqual( + events[5]["data"]["can_mention_group"], + AnonymousSettingGroupDict( + direct_members=[hamlet.id], direct_subgroups=[members_group.id] + ), + ) 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: + with self.verify_action(num_events=6, 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]) + check_user_group_update("events[3]", events[3], "can_manage_group") + check_realm_update_dict("events[4]", events[4]) + check_user_group_update("events[5]", events[5], "can_mention_group") + self.assertEqual( + events[3]["data"]["can_manage_group"], + AnonymousSettingGroupDict(direct_members=[], direct_subgroups=[]), + ) + self.assertEqual( + events[4]["data"]["can_create_public_channel_group"], + AnonymousSettingGroupDict(direct_members=[], direct_subgroups=[members_group.id]), + ) + self.assertEqual( + events[5]["data"]["can_mention_group"], + AnonymousSettingGroupDict( + direct_members=[hamlet.id], direct_subgroups=[members_group.id] + ), + ) user_profile = self.example_user("shiva") with self.verify_action(num_events=1) as events: @@ -3092,12 +3149,17 @@ 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=4) as events: + with self.verify_action(num_events=5) 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]) - check_realm_user_remove("events[3]]", events[3]) + check_user_group_update("events[3]", events[3], "can_mention_group") + check_realm_user_remove("events[4]]", events[4]) + self.assertEqual( + events[3]["data"]["can_mention_group"], + AnonymousSettingGroupDict(direct_members=[], direct_subgroups=[members_group.id]), + ) user_profile = self.example_user("aaron") # One update event is for a deactivating a bot owned by aaron. @@ -3143,6 +3205,30 @@ class NormalActionsTest(BaseAction): check_subscription_peer_remove("events[4]", events[4]) check_stream_delete("events[5]", events[5]) + user_profile = self.example_user("cordelia") + members_group = NamedUserGroup.objects.get( + name=SystemGroups.MEMBERS, realm=user_profile.realm, is_system_group=True + ) + hamletcharacters_group = NamedUserGroup.objects.get( + name="hamletcharacters", realm=self.user_profile.realm + ) + + setting_group = self.create_or_update_anonymous_group_for_setting( + [user_profile], [hamletcharacters_group] + ) + do_change_realm_permission_group_setting( + user_profile.realm, + "can_create_public_channel_group", + setting_group, + acting_user=None, + ) + setting_group = self.create_or_update_anonymous_group_for_setting( + [user_profile], [members_group] + ) + do_change_user_group_permission_setting( + hamletcharacters_group, "can_mention_group", setting_group, acting_user=None + ) + self.set_up_db_for_testing_user_access() # Test that guest users receive realm_user/update event # only if they can access the reactivated user. @@ -3152,11 +3238,30 @@ class NormalActionsTest(BaseAction): self.user_profile = self.example_user("polonius") # 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: + with self.verify_action(num_events=6) 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]) + check_user_group_update("events[3]", events[3], "can_manage_group") + check_realm_update_dict("events[4]", events[4]) + check_user_group_update("events[5]", events[5], "can_mention_group") + self.assertEqual( + events[3]["data"]["can_manage_group"], + AnonymousSettingGroupDict(direct_members=[user_profile.id], direct_subgroups=[]), + ) + self.assertEqual( + events[4]["data"]["can_create_public_channel_group"], + AnonymousSettingGroupDict( + direct_members=[user_profile.id], direct_subgroups=[hamletcharacters_group.id] + ), + ) + self.assertEqual( + events[5]["data"]["can_mention_group"], + AnonymousSettingGroupDict( + direct_members=[user_profile.id], direct_subgroups=[members_group.id] + ), + ) user_profile = self.example_user("shiva") do_deactivate_user(user_profile, acting_user=None)