From af5d0d6f5e5444332f9f8e565d97f4acdceaa72f Mon Sep 17 00:00:00 2001
From: Mateusz Mandera <mateusz.mandera@zulip.com>
Date: Fri, 7 Jan 2022 21:47:11 +0100
Subject: [PATCH] bots: Don't allow admins to change owner of bot with
 can_create_users.

Ordinary organization administrators shouldn't be allowed to change
ownership of a bot with the can_create_users permission.

This is a special permission that is granted manually by server
administrators to an organization (to a UserProfile of the org owners'
choice) after approval by a server administator. The code comments
provide more detail about why this is sensitive.
---
 zerver/lib/users.py       | 16 +++++++++++++++-
 zerver/tests/test_bots.py | 40 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/zerver/lib/users.py b/zerver/lib/users.py
index 7718fdc7b8..9365a6f8d5 100644
--- a/zerver/lib/users.py
+++ b/zerver/lib/users.py
@@ -19,7 +19,11 @@ from zerver.lib.cache import (
     user_profile_by_id_cache_key,
     user_profile_cache_key_id,
 )
-from zerver.lib.exceptions import JsonableError, OrganizationAdministratorRequired
+from zerver.lib.exceptions import (
+    JsonableError,
+    OrganizationAdministratorRequired,
+    OrganizationOwnerRequired,
+)
 from zerver.lib.timezone import canonicalize_timezone
 from zerver.lib.types import ProfileDataElementValue
 from zerver.models import (
@@ -240,6 +244,16 @@ def access_bot_by_id(user_profile: UserProfile, user_id: int) -> UserProfile:
         raise JsonableError(_("No such bot"))
     if not user_profile.can_admin_user(target):
         raise JsonableError(_("Insufficient permission"))
+
+    if target.can_create_users and not user_profile.is_realm_owner:
+        # Organizations owners are required to administer a bot with
+        # the can_create_users permission. User creation via the API
+        # is a permission not available even to organization owners by
+        # default, because it can be abused to send spam. Requiring an
+        # owner is intended to ensure organizational responsibility
+        # for use of this permission.
+        raise OrganizationOwnerRequired()
+
     return target
 
 
diff --git a/zerver/tests/test_bots.py b/zerver/tests/test_bots.py
index e998cf1cbf..d727e134e3 100644
--- a/zerver/tests/test_bots.py
+++ b/zerver/tests/test_bots.py
@@ -9,6 +9,7 @@ from django.test import override_settings
 from zulip_bots.custom_exceptions import ConfigValidationError
 
 from zerver.lib.actions import (
+    do_change_can_create_users,
     do_change_stream_permission,
     do_deactivate_user,
     do_set_realm_property,
@@ -1028,6 +1029,45 @@ class BotTest(ZulipTestCase, UploadSerializeMixin):
         profile = get_user(email, get_realm("zulip"))
         self.assertEqual(profile.bot_owner, self.example_user("hamlet"))
 
+    def test_patch_bot_owner_of_bot_with_can_create_users(self) -> None:
+        """
+        can_create_users is granted to organizations upon approval, and thus
+        should be thought of as something that only organization owners should
+        have control over.
+        """
+        cordelia = self.example_user("cordelia")
+
+        self.login("hamlet")
+        self.create_bot()
+
+        bot_realm = get_realm("zulip")
+        bot_email = "hambot-bot@zulip.testserver"
+        bot_user = get_user(bot_email, bot_realm)
+
+        do_change_can_create_users(bot_user, True)
+
+        self.logout()
+        # iago is an ordinary organization administrator, and thus doesn't have
+        # sufficient permissions to change ownership of this bot.
+        self.login("iago")
+        bot_info = {
+            "bot_owner_id": cordelia.id,
+        }
+        result = self.client_patch(f"/json/bots/{bot_user.id}", bot_info)
+        self.assert_json_error(
+            result,
+            "Must be an organization owner",
+        )
+
+        self.logout()
+        # desdemona is the organization owner and should be allowed to change the bot's ownership.
+        self.login("desdemona")
+        result = self.client_patch(f"/json/bots/{bot_user.id}", bot_info)
+        self.assert_json_success(result)
+
+        bot_user.refresh_from_db()
+        self.assertEqual(bot_user.bot_owner, cordelia)
+
     def test_patch_bot_avatar(self) -> None:
         self.login("hamlet")
         bot_info = {