From 0be977fb29ca215ace2323b3b1ec9292dcc6c8e6 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Mon, 5 Jun 2023 16:55:13 +0100 Subject: [PATCH 1/5] validate settings once + fix Telegram ratelimit issue --- engine/apps/api/tests/test_live_settings.py | 32 +++++++++++++++++++++ engine/apps/api/views/live_setting.py | 3 -- engine/apps/base/models/live_setting.py | 5 ++-- engine/apps/base/utils.py | 8 ++++-- engine/apps/telegram/client.py | 1 + 5 files changed, 40 insertions(+), 9 deletions(-) diff --git a/engine/apps/api/tests/test_live_settings.py b/engine/apps/api/tests/test_live_settings.py index 1cfecebdeb..2f8e073050 100644 --- a/engine/apps/api/tests/test_live_settings.py +++ b/engine/apps/api/tests/test_live_settings.py @@ -5,6 +5,8 @@ from rest_framework.status import HTTP_200_OK from rest_framework.test import APIClient +from apps.base.models import LiveSetting + @pytest.mark.django_db def test_list_live_setting( @@ -98,3 +100,33 @@ def test_live_settings_update_not_trigger_unpopulate_slack_identities( assert not mocked_unpopulate_task.called assert response.status_code == HTTP_200_OK + + +@pytest.mark.django_db +def test_live_settings_telegram_calls_set_webhook_once( + make_organization_and_user_with_plugin_token, + make_user_auth_headers, + make_live_setting, + settings, +): + """ + Check that when TELEGRAM_WEBHOOK_HOST live setting is updated, set_webhook method is called only once. + If set_webhook is called more than once in a short period of time, there will be a rate limit error. + """ + + settings.FEATURE_LIVE_SETTINGS_ENABLED = True + + organization, user, token = make_organization_and_user_with_plugin_token() + LiveSetting.populate_settings_if_needed() + live_setting = LiveSetting.objects.get(name="TELEGRAM_WEBHOOK_HOST") + + client = APIClient() + url = reverse("api-internal:live_settings-detail", kwargs={"pk": live_setting.public_primary_key}) + data = {"id": live_setting.public_primary_key, "value": "TEST_UPDATED_VALUE", "name": "TELEGRAM_WEBHOOK_HOST"} + + with mock.patch("telegram.Bot.get_webhook_info", return_value=mock.Mock(url="TEST_VALUE")): + with mock.patch("telegram.Bot.set_webhook") as mock_set_webhook: + response = client.put(url, data=data, format="json", **make_user_auth_headers(user, token)) + + assert response.status_code == HTTP_200_OK + mock_set_webhook.assert_called_once() diff --git a/engine/apps/api/views/live_setting.py b/engine/apps/api/views/live_setting.py index 02ca047cef..760e3f0ddb 100644 --- a/engine/apps/api/views/live_setting.py +++ b/engine/apps/api/views/live_setting.py @@ -70,9 +70,6 @@ def _post_update_hook(self, name, old_value): self._reset_telegram_integration(old_token=old_value) register_telegram_webhook.delay() - if name == "TELEGRAM_WEBHOOK_HOST": - register_telegram_webhook.delay() - if name in ["SLACK_CLIENT_OAUTH_ID", "SLACK_CLIENT_OAUTH_SECRET"]: organization = self.request.auth.organization slack_team_identity = organization.slack_team_identity diff --git a/engine/apps/base/models/live_setting.py b/engine/apps/base/models/live_setting.py index e6d1e70876..e553265dd2 100644 --- a/engine/apps/base/models/live_setting.py +++ b/engine/apps/base/models/live_setting.py @@ -212,6 +212,7 @@ def populate_settings_if_needed(cls): def validate_settings(cls): settings_to_validate = cls.objects.all() for setting in settings_to_validate: + setting.error = LiveSettingValidator(live_setting=setting).get_error() setting.save(update_fields=["error"]) @staticmethod @@ -220,13 +221,11 @@ def _get_setting_from_setting_file(setting_name): def save(self, *args, **kwargs): """ - Save validates LiveSettings values and save them in database + Save checks that setting name is in list of available names, then saves it. """ if self.name not in self.AVAILABLE_NAMES: raise ValueError( f"Setting with name '{self.name}' is not in list of available names {self.AVAILABLE_NAMES}" ) - self.error = LiveSettingValidator(live_setting=self).get_error() - super().save(*args, **kwargs) diff --git a/engine/apps/base/utils.py b/engine/apps/base/utils.py index 98f593bf58..66f58e15d4 100644 --- a/engine/apps/base/utils.py +++ b/engine/apps/base/utils.py @@ -45,7 +45,7 @@ def __init__(self, live_setting): def get_error(self): check_fn_name = f"_check_{self.live_setting.name.lower()}" - if self.live_setting.value is None and self.live_setting.name not in self.EMPTY_VALID_NAMES: + if self.live_setting.value in (None, "") and self.live_setting.name not in self.EMPTY_VALID_NAMES: return "Empty" # skip validation if there's no handler for it @@ -138,9 +138,11 @@ def _check_telegram_token(cls, telegram_token): @classmethod def _check_telegram_webhook_host(cls, telegram_webhook_host): try: + # avoid circular import + from apps.telegram.client import TelegramClient + url = create_engine_url("/telegram/", override_base=telegram_webhook_host) - bot = Bot(token=live_settings.TELEGRAM_TOKEN) - bot.set_webhook(url) + TelegramClient().register_webhook(url) except Exception as e: return f"Telegram error: {str(e)}" diff --git a/engine/apps/telegram/client.py b/engine/apps/telegram/client.py index 86e2de22fb..b5733bcaf7 100644 --- a/engine/apps/telegram/client.py +++ b/engine/apps/telegram/client.py @@ -39,6 +39,7 @@ def is_chat_member(self, chat_id: Union[int, str]) -> bool: def register_webhook(self, webhook_url: Optional[str] = None) -> None: webhook_url = webhook_url or create_engine_url("/telegram/", override_base=live_settings.TELEGRAM_WEBHOOK_HOST) + # avoid unnecessary set_webhook calls to make sure Telegram rate limits are not exceeded webhook_info = self.api_client.get_webhook_info() if webhook_info.url == webhook_url: return From d1a954559a07022353db5fc44041c3ae2470942b Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Mon, 5 Jun 2023 17:05:38 +0100 Subject: [PATCH 2/5] more tests --- engine/apps/api/tests/test_live_settings.py | 32 ++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/engine/apps/api/tests/test_live_settings.py b/engine/apps/api/tests/test_live_settings.py index 2f8e073050..dce8623677 100644 --- a/engine/apps/api/tests/test_live_settings.py +++ b/engine/apps/api/tests/test_live_settings.py @@ -102,6 +102,34 @@ def test_live_settings_update_not_trigger_unpopulate_slack_identities( assert response.status_code == HTTP_200_OK +@pytest.mark.django_db +def test_live_settings_update_validate_settings_once( + make_organization_and_user_with_plugin_token, + make_user_auth_headers, + make_live_setting, + settings, +): + """ + Check that settings are validated only once per update. + """ + + settings.FEATURE_LIVE_SETTINGS_ENABLED = True + + organization, user, token = make_organization_and_user_with_plugin_token() + LiveSetting.populate_settings_if_needed() + live_setting = LiveSetting.objects.get(name="EMAIL_HOST") # random setting + + client = APIClient() + url = reverse("api-internal:live_settings-detail", kwargs={"pk": live_setting.public_primary_key}) + data = {"id": live_setting.public_primary_key, "value": "TEST_UPDATED_VALUE", "name": "EMAIL_HOST"} + + with mock.patch.object(LiveSetting, "validate_settings") as mock_validate_settings: + response = client.put(url, data=data, format="json", **make_user_auth_headers(user, token)) + + assert response.status_code == HTTP_200_OK + mock_validate_settings.assert_called_once() + + @pytest.mark.django_db def test_live_settings_telegram_calls_set_webhook_once( make_organization_and_user_with_plugin_token, @@ -129,4 +157,6 @@ def test_live_settings_telegram_calls_set_webhook_once( response = client.put(url, data=data, format="json", **make_user_auth_headers(user, token)) assert response.status_code == HTTP_200_OK - mock_set_webhook.assert_called_once() + mock_set_webhook.assert_called_once_with( + "TEST_UPDATED_VALUE/telegram/", allowed_updates=("message", "callback_query") + ) From a8e825bb41cf370cb7ea3e8b0550c4b47dcdf327 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Mon, 5 Jun 2023 17:13:13 +0100 Subject: [PATCH 3/5] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 66c764e69f..f2ac3565bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Do not hide not secret settings in the web plugin UI by @alexintech ([#1964](https://github.com/grafana/oncall/pull/1964)) +- Fix Telegram ratelimit on live setting change by @vadimkerr and @alexintech ([#2100](https://github.com/grafana/oncall/pull/2100)) ## v1.2.36 (2023-06-02) From 879b14f01cd37685d50711c0e2953647ac2c2d27 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Mon, 5 Jun 2023 17:21:11 +0100 Subject: [PATCH 4/5] remove useless comment --- engine/apps/base/models/live_setting.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/engine/apps/base/models/live_setting.py b/engine/apps/base/models/live_setting.py index e553265dd2..34600ddbd2 100644 --- a/engine/apps/base/models/live_setting.py +++ b/engine/apps/base/models/live_setting.py @@ -220,9 +220,6 @@ def _get_setting_from_setting_file(setting_name): return getattr(settings, setting_name) def save(self, *args, **kwargs): - """ - Save checks that setting name is in list of available names, then saves it. - """ if self.name not in self.AVAILABLE_NAMES: raise ValueError( f"Setting with name '{self.name}' is not in list of available names {self.AVAILABLE_NAMES}" From 3abd1bdc900907135b67989c936333fff43fb5a6 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Tue, 6 Jun 2023 16:13:59 +0100 Subject: [PATCH 5/5] changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 416282d1ca..5237128982 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,13 +16,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix + revert [#2057](https://github.com/grafana/oncall/pull/2057) which reverted a change which properly handles `Organization.DoesNotExist` exceptions for Slack events by @joeyorlando ([#TBD](https://github.com/grafana/oncall/pull/TBD)) +- Fix Telegram ratelimit on live setting change by @vadimkerr and @alexintech ([#2100](https://github.com/grafana/oncall/pull/2100)) ## v1.2.39 (2023-06-06) ### Changed - Do not hide not secret settings in the web plugin UI by @alexintech ([#1964](https://github.com/grafana/oncall/pull/1964)) -- Fix Telegram ratelimit on live setting change by @vadimkerr and @alexintech ([#2100](https://github.com/grafana/oncall/pull/2100)) ## v1.2.36 (2023-06-02)