bots: Do not remove bot from inaccessible streams on owner change.

See
https://chat.zulip.org/#narrow/channel/101-design/topic/manage.20bot.20access.20feature.20removal

api_docs/changelog.md

@@ -20,6 +20,14 @@ format used by the Zulip server that they are interacting with.
 
 ## Changes in Zulip 10.0
 
+**Feature level 359**
+
+* `PATCH /bots/{bot_user_id}`: Previously, changing the owner of a bot
+  unsubscribed the bot from any channels that the new owner was not
+  subscribed to. This behavior was removed in favor of documenting the
+  security trade-off associated with giving bots read access to
+  sensitive channel content.
+
 **Feature level 358**
 
 * `PATCH /realm`, [`GET /events`](/api/get-events): Changed `allow_edit_history`

version.py

@@ -34,7 +34,7 @@ DESKTOP_WARNING_VERSION = "5.9.3"
 # new level means in api_docs/changelog.md, as well as "**Changes**"
 # entries in the endpoint's documentation in `zulip.yaml`.
 
-API_FEATURE_LEVEL = 358
+API_FEATURE_LEVEL = 359  # Last bumped for not adjusting bot subscriptions on owner change.
 
 # Bump the minor PROVISION_VERSION to indicate that folks should provision
 # only when going from an old version of the code to a newer version. Bump

zerver/actions/bots.py

@@ -2,8 +2,6 @@ from django.db import transaction
 from django.utils.timezone import now as timezone_now
 
 from zerver.actions.create_user import created_bot_event
-from zerver.actions.streams import bulk_remove_subscriptions
-from zerver.lib.streams import get_subscribed_private_streams_for_user
 from zerver.models import RealmAuditLog, Stream, UserProfile
 from zerver.models.realm_audit_logs import AuditLogEventType
 from zerver.models.users import active_user_ids, bot_owner_user_ids
@@ -71,34 +69,6 @@ def send_bot_owner_update_events(
     send_event_on_commit(user_profile.realm, event, active_user_ids(user_profile.realm_id))
 
 
-def remove_bot_from_inaccessible_private_streams(
-    user_profile: UserProfile, *, acting_user: UserProfile | None
-) -> None:
-    assert user_profile.bot_owner is not None
-
-    new_owner_subscribed_private_streams = get_subscribed_private_streams_for_user(
-        user_profile.bot_owner
-    )
-    new_owner_subscribed_private_stream_ids = [
-        stream.id for stream in new_owner_subscribed_private_streams
-    ]
-
-    bot_subscribed_private_streams = get_subscribed_private_streams_for_user(user_profile)
-    bot_subscribed_private_stream_ids = [stream.id for stream in bot_subscribed_private_streams]
-
-    stream_ids_to_unsubscribe = set(bot_subscribed_private_stream_ids) - set(
-        new_owner_subscribed_private_stream_ids
-    )
-    unsubscribed_streams = [
-        stream
-        for stream in bot_subscribed_private_streams
-        if stream.id in stream_ids_to_unsubscribe
-    ]
-    bulk_remove_subscriptions(
-        user_profile.realm, [user_profile], unsubscribed_streams, acting_user=acting_user
-    )
-
-
 @transaction.atomic(durable=True)
 def do_change_bot_owner(
     user_profile: UserProfile, bot_owner: UserProfile, acting_user: UserProfile | None
@@ -117,8 +87,6 @@ def do_change_bot_owner(
 
     send_bot_owner_update_events(user_profile, bot_owner, previous_owner)
 
-    remove_bot_from_inaccessible_private_streams(user_profile, acting_user=acting_user)
-
 
 @transaction.atomic(durable=True)
 def do_change_default_sending_stream(

zerver/actions/create_user.py

@@ -772,11 +772,6 @@ def do_reactivate_user(user_profile: UserProfile, *, acting_user: UserProfile |
             assert acting_user is not None
             send_bot_owner_update_events(user_profile, acting_user, previous_owner)
 
-    if bot_owner_changed:
-        from zerver.actions.bots import remove_bot_from_inaccessible_private_streams
-
-        remove_bot_from_inaccessible_private_streams(user_profile, acting_user=acting_user)
-
     subscribed_recipient_ids = Subscription.objects.filter(
         user_profile_id=user_profile.id, active=True, recipient__type=Recipient.STREAM
     ).values_list("recipient__type_id", flat=True)

zerver/lib/streams.py

@@ -1735,23 +1735,6 @@ def do_get_streams(
     return stream_dicts
 
 
-def get_subscribed_private_streams_for_user(user_profile: UserProfile) -> QuerySet[Stream]:
-    exists_expression = Exists(
-        Subscription.objects.filter(
-            user_profile=user_profile,
-            active=True,
-            is_user_active=True,
-            recipient_id=OuterRef("recipient_id"),
-        ),
-    )
-    subscribed_private_streams = (
-        Stream.objects.filter(realm=user_profile.realm, invite_only=True, deactivated=False)
-        .alias(subscribed=exists_expression)
-        .filter(subscribed=True)
-    )
-    return subscribed_private_streams
-
-
 def notify_stream_is_recently_active_update(stream: Stream, value: bool) -> None:
     event = dict(
         type="stream",

zerver/tests/test_bots.py

@@ -1083,7 +1083,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin):
         assert test_bot.bot_owner is not None
         self.assertEqual(test_bot.bot_owner.id, self.example_user("iago").id)
 
-        self.assertFalse(
+        self.assertTrue(
             Subscription.objects.filter(
                 user_profile=test_bot, recipient__type_id=private_stream.id, active=True
             ).exists()
@@ -1359,7 +1359,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin):
         self.assertEqual(bot_user.bot_owner.id, hamlet.id)
 
         assert private_stream.recipient_id is not None
-        self.assertFalse(
+        self.assertTrue(
             Subscription.objects.filter(
                 user_profile=bot_user, recipient_id=private_stream.recipient_id, active=True
             ).exists()

zerver/tests/test_events.py

@@ -3116,29 +3116,6 @@ class NormalActionsTest(BaseAction):
         check_realm_bot_add("events[0]", events[0])
         check_realm_user_update("events[1]", events[1], "bot_owner_id")
 
-    def test_peer_remove_events_on_changing_bot_owner(self) -> None:
-        previous_owner = self.example_user("aaron")
-        self.user_profile = self.example_user("iago")
-        bot = self.create_test_bot("test2", previous_owner, full_name="Test2 Testerson")
-        private_stream = self.make_stream("private_stream", invite_only=True)
-        self.make_stream("public_stream")
-        self.subscribe(bot, "private_stream")
-        self.subscribe(self.example_user("aaron"), "private_stream")
-        self.subscribe(bot, "public_stream")
-        self.subscribe(self.example_user("aaron"), "public_stream")
-
-        self.make_stream("private_stream_test", invite_only=True)
-        self.subscribe(self.example_user("iago"), "private_stream_test")
-        self.subscribe(bot, "private_stream_test")
-
-        with self.verify_action(num_events=3) as events:
-            do_change_bot_owner(bot, self.user_profile, previous_owner)
-
-        check_realm_bot_update("events[0]", events[0], "owner_id")
-        check_realm_user_update("events[1]", events[1], "bot_owner_id")
-        check_subscription_peer_remove("events[2]", events[2])
-        self.assertEqual(events[2]["stream_ids"], [private_stream.id])
-
     def test_do_update_outgoing_webhook_service(self) -> None:
         self.user_profile = self.example_user("iago")
         bot = self.create_test_bot(
@@ -3320,13 +3297,13 @@ class NormalActionsTest(BaseAction):
         bot.refresh_from_db()
 
         self.user_profile = self.example_user("iago")
-        with self.verify_action(num_events=9) as events:
+        with self.verify_action(num_events=8) 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")
         check_realm_user_update("events[3]", events[3], "bot_owner_id")
-        check_subscription_peer_remove("events[4]", events[4])
-        check_stream_delete("events[5]", events[5])
+        check_subscription_peer_add("events[4]", events[4])
+        check_subscription_peer_add("events[5]", events[5])
 
         user_profile = self.example_user("cordelia")
         members_group = NamedUserGroup.objects.get(