From 180a9cbdcb0194ac357c20fac008629a021b92a7 Mon Sep 17 00:00:00 2001 From: yogesh sirsat Date: Tue, 28 Jun 2022 01:14:12 +0530 Subject: [PATCH] stream_bots: Allow bot owners to unsubscribe their bots from streams. Users who owns bots can unsubscribe their bots from streams. Fixes part of: #21402 --- templates/zerver/api/changelog.md | 5 ++++ version.py | 2 +- zerver/openapi/zulip.yaml | 10 ++++++++ zerver/tests/test_subs.py | 42 ++++++++++++++++++++++++++----- zerver/views/streams.py | 40 ++++++++++++++--------------- 5 files changed, 72 insertions(+), 27 deletions(-) diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md index 8bea4cfbc4..7c2e39edd2 100644 --- a/templates/zerver/api/changelog.md +++ b/templates/zerver/api/changelog.md @@ -20,6 +20,11 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 6.0 +**Feature level 145** + +* [`DELETE users/me/subscriptions`](/api/unsubscribe): Normal users can + now remove bots that they own from streams. + **Feature level 144** * [`GET /messages/{message_id}/read_receipts`](/api/get-read-receipts): diff --git a/version.py b/version.py index 15d8cb169d..d044bb2e17 100644 --- a/version.py +++ b/version.py @@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.4.3" # Changes should be accompanied by documentation explaining what the # new level means in templates/zerver/api/changelog.md, as well as # "**Changes**" entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 144 +API_FEATURE_LEVEL = 145 # 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/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index cdabcd5e93..cf927a0956 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -7736,6 +7736,16 @@ paths: tags: ["streams"] description: | Unsubscribe yourself or other users from one or more streams. + + In addition to managing the current user's subscriptions, this + endpoint can be used by organization administrators to remove + other users from streams, or to remove a bot that the current + user is the `bot_owner` for from any stream that the current + user can access. + + **Changes**: Before Zulip 6.0 (feature level 145), only + organization administrators could remove other users from + streams. x-curl-examples-parameters: oneOf: - type: include diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index f96ca3e322..1319292091 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -11,6 +11,7 @@ from django.core.exceptions import ValidationError from django.http import HttpResponse from django.utils.timezone import now as timezone_now +from zerver.actions.bots import do_change_bot_owner from zerver.actions.create_realm import do_create_realm from zerver.actions.default_streams import ( do_add_default_stream, @@ -2236,12 +2237,12 @@ class StreamAdminTest(ZulipTestCase): return result - def test_cant_remove_others_from_stream(self) -> None: + def test_cant_remove_other_users_from_stream(self) -> None: """ - If you're not an admin, you can't remove other people from streams. + If you're not an admin, you can't remove other people from streams except your own bots. """ result = self.attempt_unsubscribe_of_principal( - query_count=6, + query_count=7, target_users=[self.example_user("cordelia")], is_realm_admin=False, is_subbed=True, @@ -2279,10 +2280,11 @@ class StreamAdminTest(ZulipTestCase): using a queue. """ target_users = [ - self.example_user(name) for name in ["cordelia", "prospero", "iago", "hamlet", "ZOE"] + self.example_user(name) + for name in ["cordelia", "prospero", "iago", "hamlet", "outgoing_webhook_bot"] ] result = self.attempt_unsubscribe_of_principal( - query_count=26, + query_count=27, cache_count=9, target_users=target_users, is_realm_admin=True, @@ -2331,7 +2333,7 @@ class StreamAdminTest(ZulipTestCase): def test_cant_remove_others_from_stream_legacy_emails(self) -> None: result = self.attempt_unsubscribe_of_principal( - query_count=6, + query_count=7, is_realm_admin=False, is_subbed=True, invite_only=False, @@ -2386,6 +2388,34 @@ class StreamAdminTest(ZulipTestCase): self.assert_length(json["removed"], 0) self.assert_length(json["not_removed"], 1) + def test_bot_owner_can_remove_bot_from_stream(self) -> None: + user_profile = self.example_user("hamlet") + webhook_bot = self.example_user("webhook_bot") + do_change_bot_owner(webhook_bot, bot_owner=user_profile, acting_user=user_profile) + result = self.attempt_unsubscribe_of_principal( + query_count=14, + target_users=[webhook_bot], + is_realm_admin=False, + is_subbed=True, + invite_only=False, + target_users_subbed=True, + ) + self.assert_json_success(result) + + def test_non_bot_owner_cannot_remove_bot_from_stream(self) -> None: + other_user = self.example_user("cordelia") + webhook_bot = self.example_user("webhook_bot") + do_change_bot_owner(webhook_bot, bot_owner=other_user, acting_user=other_user) + result = self.attempt_unsubscribe_of_principal( + query_count=8, + target_users=[webhook_bot], + is_realm_admin=False, + is_subbed=True, + invite_only=False, + target_users_subbed=True, + ) + self.assert_json_error(result, "Insufficient permission") + def test_can_remove_subscribers_group(self) -> None: realm = get_realm("zulip") leadership_group = create_user_group( diff --git a/zerver/views/streams.py b/zerver/views/streams.py index 8d1edee01c..cb928e3343 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -130,19 +130,14 @@ def principal_to_user_profile(agent: UserProfile, principal: Union[str, int]) -> raise PrincipalError(principal) -def check_if_removing_someone_else( - user_profile: UserProfile, principals: Optional[Union[List[str], List[int]]] -) -> bool: - if principals is None or len(principals) == 0: - return False - - if len(principals) > 1: +def user_directly_controls_user(user_profile: UserProfile, target: UserProfile) -> bool: + """Returns whether the target user is either the current user or a bot + owned by the current user""" + if user_profile == target: return True - - if isinstance(principals[0], int): - return principals[0] != user_profile.id - else: - return principals[0] != user_profile.email + if target.is_bot and target.bot_owner == user_profile: + return True + return False def deactivate_stream_backend( @@ -464,23 +459,28 @@ def remove_subscriptions_backend( ) -> HttpResponse: realm = user_profile.realm - removing_someone_else = check_if_removing_someone_else(user_profile, principals) streams_as_dict: List[StreamDict] = [] for stream_name in streams_raw: streams_as_dict.append({"name": stream_name.strip()}) - streams, __ = list_to_streams( - streams_as_dict, user_profile, unsubscribing_others=removing_someone_else - ) - + unsubscribing_others = False if principals: - people_to_unsub = { - principal_to_user_profile(user_profile, principal) for principal in principals - } + people_to_unsub = set() + for principal in principals: + target_user = principal_to_user_profile(user_profile, principal) + people_to_unsub.add(target_user) + if not user_directly_controls_user(user_profile, target_user): + unsubscribing_others = True else: people_to_unsub = {user_profile} + streams, __ = list_to_streams( + streams_as_dict, + user_profile, + unsubscribing_others=unsubscribing_others, + ) + result: Dict[str, List[str]] = dict(removed=[], not_removed=[]) (removed, not_subscribed) = bulk_remove_subscriptions( realm, people_to_unsub, streams, acting_user=user_profile