From 6c2535fe463c706bf6a201355626a61315d4858f Mon Sep 17 00:00:00 2001 From: Sahil Singh Date: Wed, 29 Mar 2023 22:31:19 +0530 Subject: [PATCH] bots: Avoid multiple active bots with the same name. Creating a bot with a name that is already in use will raise an error. However, by deactivating the existing bot, creating a new bot with the same name, and then reactivating the original bot, it is possible to have multiple bots with the same name. To fix this, we check if the bot name is already in use in the active bots list. If it is, an error will be raised, prompting either the name of the existing bot to be changed or the bot to be deactivated. Co-authored-by: Sujal Shah --- zerver/actions/user_settings.py | 1 + zerver/lib/users.py | 9 +++++++-- zerver/tests/test_bots.py | 30 ++++++++++++++++++++++++++++++ zerver/views/users.py | 2 ++ 4 files changed, 40 insertions(+), 2 deletions(-) diff --git a/zerver/actions/user_settings.py b/zerver/actions/user_settings.py index b948c844b3..eee0e0f04c 100644 --- a/zerver/actions/user_settings.py +++ b/zerver/actions/user_settings.py @@ -272,6 +272,7 @@ def check_change_bot_full_name( check_bot_name_available( realm_id=user_profile.realm_id, full_name=new_full_name, + is_activation=False, ) do_change_full_name(user_profile, new_full_name, acting_user) diff --git a/zerver/lib/users.py b/zerver/lib/users.py index 3d16dfec74..78b4f684ac 100644 --- a/zerver/lib/users.py +++ b/zerver/lib/users.py @@ -94,7 +94,7 @@ def check_full_name( # name (e.g. you can get there by reactivating a deactivated bot after # making a new bot with the same name). This is just a check designed # to make it unlikely to happen by accident. -def check_bot_name_available(realm_id: int, full_name: str) -> None: +def check_bot_name_available(realm_id: int, full_name: str, *, is_activation: bool) -> None: dup_exists = UserProfile.objects.filter( realm_id=realm_id, full_name=full_name.strip(), @@ -102,7 +102,12 @@ def check_bot_name_available(realm_id: int, full_name: str) -> None: ).exists() if dup_exists: - raise JsonableError(_("Name is already in use!")) + if is_activation: + raise JsonableError( + f'There is already an active bot named "{full_name}" in this organization. To reactivate this bot, you must rename or deactivate the other one first.' + ) + else: + raise JsonableError(_("Name is already in use!")) def check_short_name(short_name_raw: str) -> str: diff --git a/zerver/tests/test_bots.py b/zerver/tests/test_bots.py index eab39c9095..fb89ac5a84 100644 --- a/zerver/tests/test_bots.py +++ b/zerver/tests/test_bots.py @@ -669,6 +669,36 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): self.assert_json_error(result, "No such user") self.assert_num_bots_equal(1) + def test_activate_bot_with_duplicate_name(self) -> None: + self.login("iago") + # Create a bot and then deactivate it + original_name = "Hamlet" + bot_info = { + "full_name": original_name, + "short_name": "hambot", + } + result = self.client_post("/json/bots", bot_info) + self.assert_json_success(result) + bot_email = "hambot-bot@zulip.testserver" + bot = self.get_bot_user(bot_email) + do_deactivate_user(bot, False, acting_user=None) + self.assertFalse( + UserProfile.objects.filter(is_bot=True, id=bot.id, is_active=True).exists() + ) + + # Create another bot with the same name + bot_info2 = { + "full_name": original_name, + "short_name": "hambot2", + } + result = self.client_post("/json/bots", bot_info2) + self.assert_json_success(result) + result = self.client_post(f"/json/users/{bot.id}/reactivate") + self.assert_json_error( + result, + 'There is already an active bot named "Hamlet" in this organization. To reactivate this bot, you must rename or deactivate the other one first.', + ) + def test_bot_permissions(self) -> None: self.login("hamlet") self.assert_num_bots_equal(0) diff --git a/zerver/views/users.py b/zerver/views/users.py index 319461f203..37780238b5 100644 --- a/zerver/views/users.py +++ b/zerver/views/users.py @@ -184,6 +184,7 @@ def reactivate_user_backend( if target.is_bot: assert target.bot_type is not None check_bot_creation_policy(user_profile, target.bot_type) + check_bot_name_available(user_profile.realm_id, target.full_name, is_activation=True) do_reactivate_user(target, acting_user=user_profile) return json_success(request) @@ -526,6 +527,7 @@ def add_bot_backend( check_bot_name_available( realm_id=user_profile.realm_id, full_name=full_name, + is_activation=False, ) check_bot_creation_policy(user_profile, bot_type)