From b6ebf143cc2886b11bc9adaac7d1418c1b465f74 Mon Sep 17 00:00:00 2001 From: Shubham Padia Date: Thu, 24 Oct 2024 19:50:49 +0000 Subject: [PATCH] streams: Backend changes to support anonymous groups. can_remove_subscribers_group setting can now be set to anonymous user groups. Co-authored-by: Sahil Batra --- api_docs/changelog.md | 12 +++ version.py | 4 +- zerver/actions/streams.py | 59 ++++++++++-- zerver/lib/event_schema.py | 35 ++++--- zerver/lib/streams.py | 77 ++++++++++++++-- zerver/lib/subscription_info.py | 35 +++++-- zerver/lib/types.py | 4 +- zerver/models/streams.py | 2 +- zerver/openapi/zulip.yaml | 121 +++++++++++++++++-------- zerver/tests/test_event_system.py | 6 +- zerver/tests/test_events.py | 20 ++++ zerver/tests/test_home.py | 6 +- zerver/tests/test_subs.py | 146 ++++++++++++++++++++++++------ zerver/views/streams.py | 64 ++++++++----- 14 files changed, 458 insertions(+), 133 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 7c72e426f5..afe7141728 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,18 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 10.0 +**Feature level 320** + +* [`GET /users/me/subscriptions`](/api/get-subscriptions), + [`GET /streams`](/api/get-streams), [`GET /events`](/api/get-events), + [`POST /register`](/api/register-queue): `can_remove_subscribers_group` + field can now either be an ID of a named user group with the permission, + or an object describing the set of users and groups with the permission. +* [`POST /users/me/subscriptions`](/api/subscribe), + [`PATCH /streams/{stream_id}`](/api/update-stream): The + `can_remove_subscribers_group` parameter can now either be an ID of a + named user group or an object describing a set of users and groups. + **Feature level 319** * [Markdown message diff --git a/version.py b/version.py index 091ac31bce..827cb164a4 100644 --- a/version.py +++ b/version.py @@ -34,7 +34,9 @@ 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 = 319 # Last bumped for message-link class +API_FEATURE_LEVEL = ( + 320 # Last bumped for supporting anonymous groups for can_remove_subscribers_group +) # 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/actions/streams.py b/zerver/actions/streams.py index 2271759614..725bced6c7 100644 --- a/zerver/actions/streams.py +++ b/zerver/actions/streams.py @@ -40,13 +40,18 @@ from zerver.lib.streams import ( can_access_stream_user_ids, check_basic_stream_access, get_occupied_streams, + get_setting_values_for_group_settings, get_stream_permission_policy_name, render_stream_description, send_stream_creation_event, stream_to_dict, ) from zerver.lib.subscription_info import get_subscribers_query -from zerver.lib.types import APISubscriptionDict +from zerver.lib.types import AnonymousSettingGroupDict, APISubscriptionDict +from zerver.lib.user_groups import ( + get_group_setting_value_for_api, + get_group_setting_value_for_audit_log_data, +) from zerver.lib.users import ( get_subscribers_of_target_user_subscriptions, get_users_involved_in_dms_with_target_users, @@ -349,13 +354,21 @@ def send_subscription_add_events( subscribers = list(subscriber_dict[stream.id]) stream_subscribers_dict[stream.id] = subscribers + setting_group_ids = set() + for sub_info in sub_info_list: + stream = sub_info.stream + for setting_name in Stream.stream_permission_group_settings: + setting_group_ids.add(getattr(stream, setting_name + "_id")) + + setting_groups_dict = get_setting_values_for_group_settings(list(setting_group_ids)) + for user_id, sub_infos in info_by_user.items(): sub_dicts: list[APISubscriptionDict] = [] for sub_info in sub_infos: stream = sub_info.stream stream_subscribers = stream_subscribers_dict[stream.id] subscription = sub_info.sub - stream_dict = stream_to_dict(stream, recent_traffic) + stream_dict = stream_to_dict(stream, recent_traffic, setting_groups_dict) # This is verbose as we cannot unpack existing TypedDict # to initialize another TypedDict while making mypy happy. # https://github.com/python/mypy/issues/5382 @@ -447,6 +460,14 @@ def send_stream_creation_events_for_previously_inaccessible_streams( stream_ids = set(altered_user_dict.keys()) recent_traffic = get_streams_traffic(stream_ids, realm) + setting_group_ids = set() + for stream_id in stream_ids: + stream = stream_dict[stream_id] + for setting_name in Stream.stream_permission_group_settings: + setting_group_ids.add(getattr(stream, setting_name + "_id")) + + setting_groups_dict = get_setting_values_for_group_settings(list(setting_group_ids)) + for stream_id, stream_users_ids in altered_user_dict.items(): stream = stream_dict[stream_id] @@ -467,7 +488,9 @@ def send_stream_creation_events_for_previously_inaccessible_streams( notify_user_ids = list(stream_users_ids & altered_guests) if notify_user_ids: - send_stream_creation_event(realm, stream, notify_user_ids, recent_traffic) + send_stream_creation_event( + realm, stream, notify_user_ids, recent_traffic, setting_groups_dict + ) def send_peer_subscriber_events( @@ -1561,16 +1584,30 @@ def do_change_stream_group_based_setting( setting_name: str, user_group: UserGroup, *, + old_setting_api_value: int | AnonymousSettingGroupDict | None = None, acting_user: UserProfile | None = None, ) -> None: old_user_group = getattr(stream, setting_name) - old_user_group_id = None - if old_user_group is not None: - old_user_group_id = old_user_group.id setattr(stream, setting_name, user_group) stream.save() + if old_setting_api_value is None: + # Most production callers will have computed this as part of + # verifying whether there's an actual change to make, but it + # feels quite clumsy to have to pass it from unit tests, so we + # compute it here if not provided by the caller. + old_setting_api_value = get_group_setting_value_for_api(old_user_group) + new_setting_api_value = get_group_setting_value_for_api(user_group) + + if not hasattr(old_user_group, "named_user_group") and hasattr(user_group, "named_user_group"): + # We delete the UserGroup which the setting was set to + # previously if it does not have any linked NamedUserGroup + # object, as it is not used anywhere else. A new UserGroup + # object would be created if the setting is later set to + # a combination of users and groups. + old_user_group.delete() + RealmAuditLog.objects.create( realm=stream.realm, acting_user=acting_user, @@ -1578,8 +1615,12 @@ def do_change_stream_group_based_setting( event_type=AuditLogEventType.CHANNEL_GROUP_BASED_SETTING_CHANGED, event_time=timezone_now(), extra_data={ - RealmAuditLog.OLD_VALUE: old_user_group_id, - RealmAuditLog.NEW_VALUE: user_group.id, + RealmAuditLog.OLD_VALUE: get_group_setting_value_for_audit_log_data( + old_setting_api_value + ), + RealmAuditLog.NEW_VALUE: get_group_setting_value_for_audit_log_data( + new_setting_api_value + ), "property": setting_name, }, ) @@ -1587,7 +1628,7 @@ def do_change_stream_group_based_setting( op="update", type="stream", property=setting_name, - value=user_group.id, + value=new_setting_api_value, stream_id=stream.id, name=stream.name, ) diff --git a/zerver/lib/event_schema.py b/zerver/lib/event_schema.py index 5d47f3505b..7cf965d2ff 100644 --- a/zerver/lib/event_schema.py +++ b/zerver/lib/event_schema.py @@ -45,13 +45,26 @@ from zerver.lib.data_types import ( make_checker, ) from zerver.lib.topic import ORIG_TOPIC, TOPIC_LINKS, TOPIC_NAME +from zerver.lib.types import AnonymousSettingGroupDict from zerver.models import Realm, RealmUserDefault, Stream, UserProfile +group_setting_type = UnionType( + [ + int, + DictType( + required_keys=[ + ("direct_members", ListType(int)), + ("direct_subgroups", ListType(int)), + ] + ), + ] +) + # These fields are used for "stream" events, and are included in the # larger "subscription" events that also contain personal settings. default_stream_fields = [ ("is_archived", bool), - ("can_remove_subscribers_group", int), + ("can_remove_subscribers_group", group_setting_type), ("creator_id", OptionalType(int)), ("date_created", int), ("description", str), @@ -103,6 +116,12 @@ optional_value_type = UnionType( bool, int, str, + DictType( + required_keys=[ + ("direct_members", ListType(int)), + ("direct_subgroups", ListType(int)), + ] + ), Equals(None), ] ) @@ -1046,18 +1065,6 @@ night_logo_data = DictType( ] ) -group_setting_type = UnionType( - [ - int, - DictType( - required_keys=[ - ("direct_members", ListType(int)), - ("direct_subgroups", ListType(int)), - ] - ), - ] -) - group_setting_update_data_type = DictType( required_keys=[], optional_keys=[ @@ -1442,7 +1449,7 @@ def check_stream_update( assert value in Stream.STREAM_POST_POLICY_TYPES elif prop == "can_remove_subscribers_group": assert extra_keys == set() - assert isinstance(value, int) + assert isinstance(value, int | AnonymousSettingGroupDict) elif prop == "first_message_id": assert extra_keys == set() assert isinstance(value, int) diff --git a/zerver/lib/streams.py b/zerver/lib/streams.py index 7c9be859d8..4c047fc5c6 100644 --- a/zerver/lib/streams.py +++ b/zerver/lib/streams.py @@ -21,10 +21,14 @@ from zerver.lib.stream_subscription import ( from zerver.lib.stream_traffic import get_average_weekly_stream_traffic, get_streams_traffic from zerver.lib.string_validation import check_stream_name from zerver.lib.timestamp import datetime_to_timestamp -from zerver.lib.types import APIStreamDict -from zerver.lib.user_groups import user_has_permission_for_group_setting +from zerver.lib.types import AnonymousSettingGroupDict, APIStreamDict +from zerver.lib.user_groups import ( + get_group_setting_value_for_api, + user_has_permission_for_group_setting, +) from zerver.models import ( DefaultStreamGroup, + GroupGroupMembership, NamedUserGroup, Realm, RealmAuditLog, @@ -32,6 +36,7 @@ from zerver.models import ( Stream, Subscription, UserGroup, + UserGroupMembership, UserProfile, ) from zerver.models.groups import SystemGroups @@ -123,8 +128,13 @@ def send_stream_creation_event( stream: Stream, user_ids: list[int], recent_traffic: dict[int, int] | None = None, + setting_groups_dict: dict[int, int | AnonymousSettingGroupDict] | None = None, ) -> None: - event = dict(type="stream", op="create", streams=[stream_to_dict(stream, recent_traffic)]) + event = dict( + type="stream", + op="create", + streams=[stream_to_dict(stream, recent_traffic, setting_groups_dict)], + ) send_event_on_commit(realm, event, user_ids) @@ -870,7 +880,11 @@ def get_occupied_streams(realm: Realm) -> QuerySet[Stream]: return occupied_streams -def stream_to_dict(stream: Stream, recent_traffic: dict[int, int] | None = None) -> APIStreamDict: +def stream_to_dict( + stream: Stream, + recent_traffic: dict[int, int] | None = None, + setting_groups_dict: dict[int, int | AnonymousSettingGroupDict] | None = None, +) -> APIStreamDict: if recent_traffic is not None: stream_weekly_traffic = get_average_weekly_stream_traffic( stream.id, stream.date_created, recent_traffic @@ -884,9 +898,16 @@ def stream_to_dict(stream: Stream, recent_traffic: dict[int, int] | None = None) # passing stream data to spectators. stream_weekly_traffic = None + if setting_groups_dict is not None: + can_remove_subscribers_group = setting_groups_dict[stream.can_remove_subscribers_group_id] + else: + can_remove_subscribers_group = get_group_setting_value_for_api( + stream.can_remove_subscribers_group + ) + return APIStreamDict( is_archived=stream.deactivated, - can_remove_subscribers_group=stream.can_remove_subscribers_group_id, + can_remove_subscribers_group=can_remove_subscribers_group, creator_id=stream.creator_id, date_created=datetime_to_timestamp(stream.date_created), description=stream.description, @@ -978,6 +999,38 @@ def get_streams_for_user( return list(streams) +def get_setting_values_for_group_settings( + group_ids: list[int], +) -> dict[int, int | AnonymousSettingGroupDict]: + setting_value_dict = {} + setting_groups = UserGroup.objects.filter(id__in=group_ids).select_related("named_user_group") + anonymous_groups = [] + for group in setting_groups: + if hasattr(group, "named_user_group"): + setting_value_dict[group.id] = group.id + else: + setting_value_dict[group.id] = AnonymousSettingGroupDict( + direct_members=[], + direct_subgroups=[], + ) + anonymous_groups.append(group.id) + + group_members = ( + UserGroupMembership.objects.filter(user_group_id__in=anonymous_groups) + .exclude(user_profile__is_active=False) + .values_list("user_group_id", "user_profile_id") + ) + group_subgroups = GroupGroupMembership.objects.filter( + supergroup_id__in=anonymous_groups + ).values_list("supergroup_id", "subgroup_id") + for user_group_id, user_profile_id in group_members: + setting_value_dict[user_group_id].direct_members.append(user_profile_id) + for supergroup_id, subgroup_id in group_subgroups: + setting_value_dict[supergroup_id].direct_subgroups.append(subgroup_id) + + return setting_value_dict + + def do_get_streams( user_profile: UserProfile, include_public: bool = True, @@ -1003,14 +1056,22 @@ def do_get_streams( stream_ids = {stream.id for stream in streams} recent_traffic = get_streams_traffic(stream_ids, user_profile.realm) + setting_group_ids = set() + for stream in streams: + for setting_name in Stream.stream_permission_group_settings: + setting_group_ids.add(getattr(stream, setting_name + "_id")) + + setting_groups_dict = get_setting_values_for_group_settings(list(setting_group_ids)) + stream_dicts = sorted( - (stream_to_dict(stream, recent_traffic) for stream in streams), key=lambda elt: elt["name"] + (stream_to_dict(stream, recent_traffic, setting_groups_dict) for stream in streams), + key=lambda elt: elt["name"], ) if include_default: default_stream_ids = get_default_stream_ids_for_realm(user_profile.realm_id) - for stream in stream_dicts: - stream["is_default"] = stream["stream_id"] in default_stream_ids + for stream_dict in stream_dicts: + stream_dict["is_default"] = stream_dict["stream_id"] in default_stream_ids return stream_dicts diff --git a/zerver/lib/subscription_info.py b/zerver/lib/subscription_info.py index f704608c7f..899900e3b7 100644 --- a/zerver/lib/subscription_info.py +++ b/zerver/lib/subscription_info.py @@ -16,9 +16,14 @@ from zerver.lib.stream_subscription import ( get_stream_subscriptions_for_user, ) from zerver.lib.stream_traffic import get_average_weekly_stream_traffic, get_streams_traffic -from zerver.lib.streams import get_web_public_streams_queryset, subscribed_to_stream +from zerver.lib.streams import ( + get_setting_values_for_group_settings, + get_web_public_streams_queryset, + subscribed_to_stream, +) from zerver.lib.timestamp import datetime_to_timestamp from zerver.lib.types import ( + AnonymousSettingGroupDict, APIStreamDict, NeverSubscribedStreamDict, RawStreamDict, @@ -26,6 +31,7 @@ from zerver.lib.types import ( SubscriptionInfo, SubscriptionStreamDict, ) +from zerver.lib.user_groups import get_group_setting_value_for_api from zerver.models import Realm, Stream, Subscription, UserProfile from zerver.models.streams import get_all_streams @@ -43,7 +49,9 @@ def get_web_public_subs(realm: Realm) -> SubscriptionInfo: for stream in get_web_public_streams_queryset(realm): # Add Stream fields. is_archived = stream.deactivated - can_remove_subscribers_group_id = stream.can_remove_subscribers_group_id + can_remove_subscribers_group = get_group_setting_value_for_api( + stream.can_remove_subscribers_group + ) creator_id = stream.creator_id date_created = datetime_to_timestamp(stream.date_created) description = stream.description @@ -76,7 +84,7 @@ def get_web_public_subs(realm: Realm) -> SubscriptionInfo: sub = SubscriptionStreamDict( is_archived=is_archived, audible_notifications=audible_notifications, - can_remove_subscribers_group=can_remove_subscribers_group_id, + can_remove_subscribers_group=can_remove_subscribers_group, color=color, creator_id=creator_id, date_created=date_created, @@ -118,7 +126,9 @@ def build_unsubscribed_sub_from_stream_dict( def build_stream_api_dict( - raw_stream_dict: RawStreamDict, recent_traffic: dict[int, int] | None + raw_stream_dict: RawStreamDict, + recent_traffic: dict[int, int] | None, + setting_groups_dict: dict[int, int | AnonymousSettingGroupDict], ) -> APIStreamDict: # Add a few computed fields not directly from the data models. if recent_traffic is not None: @@ -133,9 +143,13 @@ def build_stream_api_dict( # migration. is_announcement_only = raw_stream_dict["stream_post_policy"] == Stream.STREAM_POST_POLICY_ADMINS + can_remove_subscribers_group = setting_groups_dict[ + raw_stream_dict["can_remove_subscribers_group_id"] + ] + return APIStreamDict( is_archived=raw_stream_dict["deactivated"], - can_remove_subscribers_group=raw_stream_dict["can_remove_subscribers_group_id"], + can_remove_subscribers_group=can_remove_subscribers_group, creator_id=raw_stream_dict["creator_id"], date_created=datetime_to_timestamp(raw_stream_dict["date_created"]), description=raw_stream_dict["description"], @@ -471,6 +485,13 @@ def gather_subscriptions_helper( } all_streams_map: dict[int, RawStreamDict] = {stream["id"]: stream for stream in all_streams} + setting_group_ids = set() + for stream in all_streams: + for setting_name in Stream.stream_permission_group_settings: + setting_group_ids.add(stream[setting_name + "_id"]) + + setting_groups_dict = get_setting_values_for_group_settings(list(setting_group_ids)) + sub_dicts_query: Iterable[RawSubscriptionDict] = ( get_stream_subscriptions_for_user(user_profile) .values( @@ -505,7 +526,9 @@ def gather_subscriptions_helper( stream_id = get_stream_id(sub_dict) sub_unsub_stream_ids.add(stream_id) raw_stream_dict = all_streams_map[stream_id] - stream_api_dict = build_stream_api_dict(raw_stream_dict, recent_traffic) + stream_api_dict = build_stream_api_dict( + raw_stream_dict, recent_traffic, setting_groups_dict + ) stream_dict = build_stream_dict_for_sub( user=user_profile, sub_dict=sub_dict, diff --git a/zerver/lib/types.py b/zerver/lib/types.py index cf001e8180..ce93098a6c 100644 --- a/zerver/lib/types.py +++ b/zerver/lib/types.py @@ -191,7 +191,7 @@ class SubscriptionStreamDict(TypedDict): """ audible_notifications: bool | None - can_remove_subscribers_group: int + can_remove_subscribers_group: int | AnonymousSettingGroupDict color: str creator_id: int | None date_created: int @@ -245,7 +245,7 @@ class DefaultStreamDict(TypedDict): """ is_archived: bool - can_remove_subscribers_group: int + can_remove_subscribers_group: int | AnonymousSettingGroupDict creator_id: int | None date_created: int description: str diff --git a/zerver/models/streams.py b/zerver/models/streams.py index 0036d4592e..c8c4d920b3 100644 --- a/zerver/models/streams.py +++ b/zerver/models/streams.py @@ -143,7 +143,7 @@ class Stream(models.Model): stream_permission_group_settings = { "can_remove_subscribers_group": GroupPermissionSetting( - require_system_group=True, + require_system_group=False, allow_internet_group=False, allow_owners_group=False, allow_nobody_group=False, diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 465168db08..f9f83dcd3b 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -1459,7 +1459,33 @@ paths: value: description: | The new value of the changed property. + + **Changes**: Starting with Zulip 10.0 (feature level 320), this + field can be an object for `can_remove_subscribers_group` property, + which is a [group-setting value][setting-values], when the setting + is set to a combination of users and groups. + + [setting-values]: /api/group-setting-values oneOf: + - type: object + additionalProperties: false + properties: + direct_members: + description: | + The list of IDs of individual users in the collection of users with this permission. + type: array + items: + type: integer + direct_subgroups: + description: | + The list of IDs of the groups in the collection of users with this permission. + type: array + items: + type: integer + description: | + If an object, it will be a [group-setting value][setting-values] with these fields: + + [setting-values]: /api/group-setting-values - type: integer - type: boolean - type: string @@ -10364,7 +10390,7 @@ paths: message_retention_days: $ref: "#/components/schemas/MessageRetentionDays" can_remove_subscribers_group: - $ref: "#/components/schemas/CanRemoveSubscribersGroupId" + $ref: "#/components/schemas/CanRemoveSubscribersGroup" required: - subscriptions encoding: @@ -19958,7 +19984,43 @@ paths: message_retention_days: $ref: "#/components/schemas/MessageRetentionDays" can_remove_subscribers_group: - $ref: "#/components/schemas/CanRemoveSubscribersGroupId" + description: | + The set of users who have permission to unsubscribe others from this + channel expressed as an [update to a group-setting value][update-group-setting]. + + Note that a user who is a member of the specified user group must + also [have access](/help/channel-permissions) to the channel in + order to unsubscribe others. + + **Changes**: Prior to Zulip 10.0 (feature level 320), this value was + always the integer ID of a system group. + + Before Zulip 8.0 (feature level 197), the `can_remove_subscribers_group` + setting was named `can_remove_subscribers_group_id`. + + New in Zulip 7.0 (feature level 161). + type: object + additionalProperties: false + properties: + new: + allOf: + - description: | + The new [group-setting value](/api/group-setting-values) for who would + have the permission to remove subscribers from the channel. + - $ref: "#/components/schemas/GroupSettingValue" + old: + allOf: + - description: | + The expected current [group-setting value](/api/group-setting-values) + for who has the permission to remove subscribers from the channel. + - $ref: "#/components/schemas/GroupSettingValue" + required: + - new + example: + { + "new": {"direct_members": [10], "direct_subgroups": [11]}, + "old": 15, + } encoding: is_private: contentType: application/json @@ -21819,16 +21881,7 @@ components: **Changes**: Deprecated in Zulip 3.0 (feature level 1). Clients should use `stream_post_policy` instead. can_remove_subscribers_group: - type: integer - description: | - ID of the user group whose members are allowed to unsubscribe others - from the channel. - - **Changes**: Before Zulip 8.0 (feature level 197), - the `can_remove_subscribers_group` setting - was named `can_remove_subscribers_group_id`. - - New in Zulip 6.0 (feature level 142). + $ref: "#/components/schemas/CanRemoveSubscribersGroup" BasicBot: allOf: - $ref: "#/components/schemas/BasicBotBase" @@ -22835,16 +22888,7 @@ components: If `null`, the channel was recently created and there is insufficient data to estimate the average traffic. can_remove_subscribers_group: - type: integer - description: | - ID of the user group whose members are allowed to unsubscribe others - from the channel. - - **Changes**: Before Zulip 8.0 (feature level 197), - the `can_remove_subscribers_group` setting - was named `can_remove_subscribers_group_id`. - - New in Zulip 6.0 (feature level 142). + $ref: "#/components/schemas/CanRemoveSubscribersGroup" is_archived: type: boolean description: | @@ -24515,25 +24559,26 @@ components: - type: string - type: integer example: "20" - CanRemoveSubscribersGroupId: - description: | - ID of the [user group](/api/get-user-groups) whose members are - allowed to unsubscribe others from the channel. Note that a user - who is a member of the specified user group must also [have - access](/help/channel-permissions) to the channel in order to - unsubscribe others. + CanRemoveSubscribersGroup: + allOf: + - $ref: "#/components/schemas/GroupSettingValue" + - description: | + A [group-setting value][setting-values] defining the set of users + who have permission to remove subscribers from this channel. - This setting can currently only be set to user groups that are - system groups, except for the system groups named - `"role:internet"` and `"role:owners"`. + Note that a user who is a member of the specified user group must + also [have access](/help/channel-permissions) to the channel in + order to unsubscribe others. - **Changes**: Before Zulip 8.0 (feature level 197), - the `can_remove_subscribers_group` setting - was named `can_remove_subscribers_group_id`. + **Changes**: Prior to Zulip 10.0 (feature level 320), this value was + always the integer ID of a system group. - New in Zulip 7.0 (feature level 161). - type: integer - example: 20 + Before Zulip 8.0 (feature level 197), the `can_remove_subscribers_group` + setting was named `can_remove_subscribers_group_id`. + + New in Zulip 6.0 (feature level 142). + + [setting-values]: /api/group-setting-values LinkifierPattern: description: | The [Python regular diff --git a/zerver/tests/test_event_system.py b/zerver/tests/test_event_system.py index fbb9366d34..ba11604373 100644 --- a/zerver/tests/test_event_system.py +++ b/zerver/tests/test_event_system.py @@ -1192,7 +1192,7 @@ class FetchQueriesTest(ZulipTestCase): realm = get_realm_with_settings(realm_id=user.realm_id) with ( - self.assert_database_query_count(40), + self.assert_database_query_count(42), mock.patch("zerver.lib.events.always_want") as want_mock, ): fetch_initial_state_data(user, realm=realm) @@ -1224,9 +1224,9 @@ class FetchQueriesTest(ZulipTestCase): saved_snippets=1, scheduled_messages=1, starred_messages=1, - stream=3, + stream=4, stop_words=0, - subscription=4, + subscription=5, update_display_settings=0, update_global_notifications=0, update_message_flags=5, diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 01e5fb8eb0..f6e56475e7 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -4538,6 +4538,26 @@ class SubscribeActionTest(BaseAction): acting_user=self.example_user("hamlet"), ) check_stream_update("events[0]", events[0]) + self.assertEqual(events[0]["value"], moderators_group.id) + + setting_group = self.create_or_update_anonymous_group_for_setting( + [self.user_profile], + [moderators_group], + ) + with self.verify_action(include_subscribers=include_subscribers, num_events=1) as events: + do_change_stream_group_based_setting( + stream, + "can_remove_subscribers_group", + setting_group, + acting_user=self.example_user("hamlet"), + ) + check_stream_update("events[0]", events[0]) + self.assertEqual( + events[0]["value"], + AnonymousSettingGroupDict( + direct_members=[self.user_profile.id], direct_subgroups=[moderators_group.id] + ), + ) # Subscribe to a totally new invite-only stream, so it's just Hamlet on it stream = self.make_stream("private", self.user_profile.realm, invite_only=True) diff --git a/zerver/tests/test_home.py b/zerver/tests/test_home.py index 16313727cc..85b5cdffda 100644 --- a/zerver/tests/test_home.py +++ b/zerver/tests/test_home.py @@ -271,7 +271,7 @@ class HomeTest(ZulipTestCase): # Verify succeeds once logged-in with ( - self.assert_database_query_count(51), + self.assert_database_query_count(52), patch("zerver.lib.cache.cache_set") as cache_mock, ): result = self._get_home_page(stream="Denmark") @@ -576,7 +576,7 @@ class HomeTest(ZulipTestCase): # Verify number of queries for Realm admin isn't much higher than for normal users. self.login("iago") with ( - self.assert_database_query_count(51), + self.assert_database_query_count(52), patch("zerver.lib.cache.cache_set") as cache_mock, ): result = self._get_home_page() @@ -608,7 +608,7 @@ class HomeTest(ZulipTestCase): self._get_home_page() # Then for the second page load, measure the number of queries. - with self.assert_database_query_count(46): + with self.assert_database_query_count(47): result = self._get_home_page() # Do a sanity check that our new streams were in the payload. diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 46c8eb6ef9..176b5e2eeb 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -570,10 +570,31 @@ class TestCreateStreams(ZulipTestCase): allow_fail=True, subdomain="zulip", ) - self.assert_json_error( - result, "'can_remove_subscribers_group' must be a system user group." + self.assert_json_success(result) + stream = get_stream("new_stream3", realm) + self.assertEqual(stream.can_remove_subscribers_group.id, hamletcharacters_group.id) + + subscriptions = [{"name": "new_stream4", "description": "Fourth new stream"}] + result = self.common_subscribe_to_streams( + user, + subscriptions, + { + "can_remove_subscribers_group": orjson.dumps( + {"direct_members": [user.id], "direct_subgroups": [moderators_system_group.id]} + ).decode() + }, + allow_fail=True, + subdomain="zulip", + ) + self.assert_json_success(result) + stream = get_stream("new_stream4", realm) + self.assertEqual(list(stream.can_remove_subscribers_group.direct_members.all()), [user]) + self.assertEqual( + list(stream.can_remove_subscribers_group.direct_subgroups.all()), + [moderators_system_group], ) + subscriptions = [{"name": "new_stream5", "description": "Fifth new stream"}] internet_group = NamedUserGroup.objects.get( name="role:internet", is_system_group=True, realm=realm ) @@ -2273,14 +2294,22 @@ class StreamAdminTest(ZulipTestCase): self.login("shiva") result = self.client_patch( f"/json/streams/{stream.id}", - {"can_remove_subscribers_group": orjson.dumps(moderators_system_group.id).decode()}, + { + "can_remove_subscribers_group": orjson.dumps( + {"new": moderators_system_group.id} + ).decode() + }, ) self.assert_json_error(result, "Must be an organization administrator") self.login("iago") result = self.client_patch( f"/json/streams/{stream.id}", - {"can_remove_subscribers_group": orjson.dumps(moderators_system_group.id).decode()}, + { + "can_remove_subscribers_group": orjson.dumps( + {"new": moderators_system_group.id} + ).decode() + }, ) self.assert_json_success(result) stream = get_stream("stream_name1", realm) @@ -2290,10 +2319,56 @@ class StreamAdminTest(ZulipTestCase): hamletcharacters_group = NamedUserGroup.objects.get(name="hamletcharacters", realm=realm) result = self.client_patch( f"/json/streams/{stream.id}", - {"can_remove_subscribers_group": orjson.dumps(hamletcharacters_group.id).decode()}, + { + "can_remove_subscribers_group": orjson.dumps( + {"new": hamletcharacters_group.id} + ).decode() + }, ) - self.assert_json_error( - result, "'can_remove_subscribers_group' must be a system user group." + self.assert_json_success(result) + stream = get_stream("stream_name1", realm) + self.assertEqual(stream.can_remove_subscribers_group.id, hamletcharacters_group.id) + + # Test changing it to anonymous group. + hamlet = self.example_user("hamlet") + + # Test passing incorrect old value. + result = self.client_patch( + f"/json/streams/{stream.id}", + { + "can_remove_subscribers_group": orjson.dumps( + { + "new": { + "direct_members": [hamlet.id], + "direct_subgroups": [moderators_system_group.id], + }, + "old": moderators_system_group.id, + } + ).decode() + }, + ) + self.assert_json_error(result, "'old' value does not match the expected value.") + + result = self.client_patch( + f"/json/streams/{stream.id}", + { + "can_remove_subscribers_group": orjson.dumps( + { + "new": { + "direct_members": [hamlet.id], + "direct_subgroups": [moderators_system_group.id], + }, + "old": hamletcharacters_group.id, + } + ).decode() + }, + ) + self.assert_json_success(result) + stream = get_stream("stream_name1", realm) + self.assertEqual(list(stream.can_remove_subscribers_group.direct_members.all()), [hamlet]) + self.assertEqual( + list(stream.can_remove_subscribers_group.direct_subgroups.all()), + [moderators_system_group], ) internet_group = NamedUserGroup.objects.get( @@ -2301,7 +2376,7 @@ class StreamAdminTest(ZulipTestCase): ) result = self.client_patch( f"/json/streams/{stream.id}", - {"can_remove_subscribers_group": orjson.dumps(internet_group.id).decode()}, + {"can_remove_subscribers_group": orjson.dumps({"new": internet_group.id}).decode()}, ) self.assert_json_error( result, @@ -2313,7 +2388,7 @@ class StreamAdminTest(ZulipTestCase): ) result = self.client_patch( f"/json/streams/{stream.id}", - {"can_remove_subscribers_group": orjson.dumps(owners_group.id).decode()}, + {"can_remove_subscribers_group": orjson.dumps({"new": owners_group.id}).decode()}, ) self.assert_json_error( result, @@ -2325,7 +2400,7 @@ class StreamAdminTest(ZulipTestCase): ) result = self.client_patch( f"/json/streams/{stream.id}", - {"can_remove_subscribers_group": orjson.dumps(nobody_group.id).decode()}, + {"can_remove_subscribers_group": orjson.dumps({"new": nobody_group.id}).decode()}, ) self.assert_json_error( result, @@ -2337,14 +2412,22 @@ class StreamAdminTest(ZulipTestCase): stream = self.make_stream("stream_name2", invite_only=True) result = self.client_patch( f"/json/streams/{stream.id}", - {"can_remove_subscribers_group": orjson.dumps(moderators_system_group.id).decode()}, + { + "can_remove_subscribers_group": orjson.dumps( + {"new": moderators_system_group.id} + ).decode() + }, ) self.assert_json_error(result, "Invalid channel ID") self.subscribe(user_profile, "stream_name2") result = self.client_patch( f"/json/streams/{stream.id}", - {"can_remove_subscribers_group": orjson.dumps(moderators_system_group.id).decode()}, + { + "can_remove_subscribers_group": orjson.dumps( + {"new": moderators_system_group.id} + ).decode() + }, ) self.assert_json_success(result) stream = get_stream("stream_name2", realm) @@ -2676,7 +2759,7 @@ class StreamAdminTest(ZulipTestCase): are on. """ result = self.attempt_unsubscribe_of_principal( - query_count=17, + query_count=18, target_users=[self.example_user("cordelia")], is_realm_admin=True, is_subbed=True, @@ -2693,7 +2776,7 @@ class StreamAdminTest(ZulipTestCase): streams you aren't on. """ result = self.attempt_unsubscribe_of_principal( - query_count=17, + query_count=18, target_users=[self.example_user("cordelia")], is_realm_admin=True, is_subbed=False, @@ -2863,6 +2946,17 @@ class StreamAdminTest(ZulipTestCase): self.subscribe(self.example_user("shiva"), stream.name) check_unsubscribing_user(self.example_user("shiva"), leadership_group) + # Test changing setting to anonymous group. + setting_group = self.create_or_update_anonymous_group_for_setting( + [hamlet], + [leadership_group], + ) + check_unsubscribing_user(self.example_user("othello"), setting_group, expect_fail=True) + check_unsubscribing_user(self.example_user("desdemona"), setting_group, expect_fail=True) + check_unsubscribing_user(self.example_user("hamlet"), setting_group) + check_unsubscribing_user(self.example_user("iago"), setting_group) + check_unsubscribing_user(self.example_user("shiva"), setting_group) + def test_remove_invalid_user(self) -> None: """ Trying to unsubscribe an invalid user from a stream fails gracefully. @@ -4707,7 +4801,7 @@ class SubscriptionAPITest(ZulipTestCase): streams_to_sub = ["multi_user_stream"] with ( self.capture_send_event_calls(expected_num_events=5) as events, - self.assert_database_query_count(37), + self.assert_database_query_count(40), ): self.common_subscribe_to_streams( self.test_user, @@ -4733,7 +4827,7 @@ class SubscriptionAPITest(ZulipTestCase): # Now add ourselves with ( self.capture_send_event_calls(expected_num_events=2) as events, - self.assert_database_query_count(14), + self.assert_database_query_count(16), ): self.common_subscribe_to_streams( self.test_user, @@ -5027,7 +5121,7 @@ class SubscriptionAPITest(ZulipTestCase): # Sends 3 peer-remove events, 2 unsubscribe events # and 2 stream delete events for private streams. with ( - self.assert_database_query_count(16), + self.assert_database_query_count(17), self.assert_memcached_count(3), self.capture_send_event_calls(expected_num_events=7) as events, ): @@ -5098,7 +5192,7 @@ class SubscriptionAPITest(ZulipTestCase): # Verify that peer_event events are never sent in Zephyr # realm. This does generate stream creation events from # send_stream_creation_events_for_previously_inaccessible_streams. - with self.assert_database_query_count(num_streams + 11): + with self.assert_database_query_count(num_streams + 13): with self.capture_send_event_calls(expected_num_events=num_streams + 1) as events: self.common_subscribe_to_streams( mit_user, @@ -5179,7 +5273,7 @@ class SubscriptionAPITest(ZulipTestCase): test_user_ids = [user.id for user in test_users] with ( - self.assert_database_query_count(16), + self.assert_database_query_count(18), self.assert_memcached_count(3), mock.patch("zerver.views.streams.send_messages_for_new_subscribers"), ): @@ -5547,7 +5641,7 @@ class SubscriptionAPITest(ZulipTestCase): ] # Test creating a public stream when realm does not have a notification stream. - with self.assert_database_query_count(37): + with self.assert_database_query_count(40): self.common_subscribe_to_streams( self.test_user, [new_streams[0]], @@ -5555,7 +5649,7 @@ class SubscriptionAPITest(ZulipTestCase): ) # Test creating private stream. - with self.assert_database_query_count(39): + with self.assert_database_query_count(42): self.common_subscribe_to_streams( self.test_user, [new_streams[1]], @@ -5567,7 +5661,7 @@ class SubscriptionAPITest(ZulipTestCase): new_stream_announcements_stream = get_stream(self.streams[0], self.test_realm) self.test_realm.new_stream_announcements_stream_id = new_stream_announcements_stream.id self.test_realm.save() - with self.assert_database_query_count(48): + with self.assert_database_query_count(51): self.common_subscribe_to_streams( self.test_user, [new_streams[2]], @@ -6047,7 +6141,7 @@ class GetSubscribersTest(ZulipTestCase): polonius.id, ] - with self.assert_database_query_count(43): + with self.assert_database_query_count(45): self.common_subscribe_to_streams( self.user_profile, streams, @@ -6095,7 +6189,7 @@ class GetSubscribersTest(ZulipTestCase): for user in [cordelia, othello, polonius]: self.assert_user_got_subscription_notification(user, msg) - with self.assert_database_query_count(4): + with self.assert_database_query_count(5): subscribed_streams, _ = gather_subscriptions( self.user_profile, include_subscribers=True ) @@ -6170,7 +6264,7 @@ class GetSubscribersTest(ZulipTestCase): create_private_streams() def get_never_subscribed() -> list[NeverSubscribedStreamDict]: - with self.assert_database_query_count(4): + with self.assert_database_query_count(5): sub_data = gather_subscriptions_helper(self.user_profile) self.verify_sub_fields(sub_data) never_subscribed = sub_data.never_subscribed @@ -6367,7 +6461,7 @@ class GetSubscribersTest(ZulipTestCase): subdomain="zephyr", ) - with self.assert_database_query_count(3): + with self.assert_database_query_count(4): subscribed_streams, _ = gather_subscriptions(mit_user_profile, include_subscribers=True) self.assertGreaterEqual(len(subscribed_streams), 2) diff --git a/zerver/views/streams.py b/zerver/views/streams.py index 6f0246cba5..382c63e1ef 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -74,7 +74,14 @@ from zerver.lib.topic import ( ) from zerver.lib.typed_endpoint import ApiParamConfig, PathOnly, typed_endpoint from zerver.lib.typed_endpoint_validators import check_color, check_int_in_validator -from zerver.lib.user_groups import access_user_group_for_setting +from zerver.lib.types import AnonymousSettingGroupDict +from zerver.lib.user_groups import ( + GroupSettingChangeRequest, + access_user_group_for_setting, + get_group_setting_value_for_api, + parse_group_setting_value, + validate_group_setting_value_change, +) from zerver.lib.users import bulk_access_users_by_email, bulk_access_users_by_id from zerver.lib.utils import assert_is_not_none from zerver.models import NamedUserGroup, Realm, Stream, UserProfile @@ -249,9 +256,7 @@ def update_stream_backend( is_web_public: Json[bool] | None = None, new_name: str | None = None, message_retention_days: Json[str] | Json[int] | None = None, - can_remove_subscribers_group_id: Annotated[ - Json[int | None], ApiParamConfig("can_remove_subscribers_group") - ] = None, + can_remove_subscribers_group: Json[GroupSettingChangeRequest] | None = None, ) -> HttpResponse: # We allow realm administrators to update the stream name and # description even for private streams. @@ -379,28 +384,42 @@ def update_stream_backend( for setting_name, permission_configuration in Stream.stream_permission_group_settings.items(): request_settings_dict = locals() - setting_group_id_name = permission_configuration.id_field_name - - if setting_group_id_name not in request_settings_dict: # nocoverage + assert setting_name in request_settings_dict + if request_settings_dict[setting_name] is None: continue - if request_settings_dict[setting_group_id_name] is not None and request_settings_dict[ - setting_group_id_name - ] != getattr(stream, setting_group_id_name): + setting_value = request_settings_dict[setting_name] + new_setting_value = parse_group_setting_value(setting_value.new, setting_name) + + expected_current_setting_value = None + if setting_value.old is not None: + expected_current_setting_value = parse_group_setting_value( + setting_value.old, setting_name + ) + + current_value = getattr(stream, setting_name) + current_setting_api_value = get_group_setting_value_for_api(current_value) + + if validate_group_setting_value_change( + current_setting_api_value, new_setting_value, expected_current_setting_value + ): if sub is None and stream.invite_only: # Admins cannot change this setting for unsubscribed private streams. raise JsonableError(_("Invalid channel ID")) - - user_group_id = request_settings_dict[setting_group_id_name] with transaction.atomic(durable=True): user_group = access_user_group_for_setting( - user_group_id, + new_setting_value, user_profile, setting_name=setting_name, permission_configuration=permission_configuration, + current_setting_value=current_value, ) do_change_stream_group_based_setting( - stream, setting_name, user_group, acting_user=user_profile + stream, + setting_name, + user_group, + old_setting_api_value=current_setting_api_value, + acting_user=user_profile, ) return json_success(request) @@ -558,9 +577,7 @@ def add_subscriptions_backend( ] = Stream.STREAM_POST_POLICY_EVERYONE, history_public_to_subscribers: Json[bool] | None = None, message_retention_days: Json[str] | Json[int] = RETENTION_DEFAULT, - can_remove_subscribers_group_id: Annotated[ - Json[int | None], ApiParamConfig("can_remove_subscribers_group") - ] = None, + can_remove_subscribers_group: Json[int | AnonymousSettingGroupDict] | None = None, announce: Json[bool] = False, principals: Json[list[str] | list[int]] | None = None, authorization_errors_fatal: Json[bool] = True, @@ -572,12 +589,15 @@ def add_subscriptions_backend( if principals is None: principals = [] - if can_remove_subscribers_group_id is not None: + if can_remove_subscribers_group is not None: + setting_value = parse_group_setting_value( + can_remove_subscribers_group, "can_remove_subscribers_group" + ) permission_configuration = Stream.stream_permission_group_settings[ "can_remove_subscribers_group" ] - can_remove_subscribers_group = access_user_group_for_setting( - can_remove_subscribers_group_id, + can_remove_subscribers_group_value = access_user_group_for_setting( + setting_value, user_profile, setting_name="can_remove_subscribers_group", permission_configuration=permission_configuration, @@ -586,7 +606,7 @@ def add_subscriptions_backend( can_remove_subscribers_group_default_name = Stream.stream_permission_group_settings[ "can_remove_subscribers_group" ].default_group_name - can_remove_subscribers_group = NamedUserGroup.objects.get( + can_remove_subscribers_group_value = NamedUserGroup.objects.get( name=can_remove_subscribers_group_default_name, realm=user_profile.realm, is_system_group=True, @@ -612,7 +632,7 @@ def add_subscriptions_backend( stream_dict_copy["message_retention_days"] = parse_message_retention_days( message_retention_days, Stream.MESSAGE_RETENTION_SPECIAL_VALUES_MAP ) - stream_dict_copy["can_remove_subscribers_group"] = can_remove_subscribers_group + stream_dict_copy["can_remove_subscribers_group"] = can_remove_subscribers_group_value stream_dicts.append(stream_dict_copy)