From a0eb0337311601f12f2c7d9f60a5a4fd4d6cf68f Mon Sep 17 00:00:00 2001 From: PieterCK Date: Thu, 19 Dec 2024 00:52:59 +0700 Subject: [PATCH] integrations: Do `check_token_access` only initially. Previously the `check_token_access` is called for every request we get from Slack webhook, this may introduce significant latency. This commit moves `check_token_access` to the same condition for when we need to handle Slack challenge handshake so that we only do API token check once per URL registered. Additionally, we now check for the specific scopes that we need to run the Slack webhook integration (SLACK_INTEGRATION_TOKEN_SCOPES). Fixes part of #30827. (cherry picked from commit f2599bf33d45fff6ca3a764222330694d0b4ad38) --- zerver/webhooks/slack/tests.py | 41 ++++++++++++++++++++++++++++++++++ zerver/webhooks/slack/view.py | 38 ++++++++++++++++++++++++++++++- 2 files changed, 78 insertions(+), 1 deletion(-) diff --git a/zerver/webhooks/slack/tests.py b/zerver/webhooks/slack/tests.py index 172e758f5b..a1b7224177 100644 --- a/zerver/webhooks/slack/tests.py +++ b/zerver/webhooks/slack/tests.py @@ -4,6 +4,7 @@ from unittest.mock import patch from typing_extensions import override from zerver.lib.test_classes import WebhookTestCase +from zerver.webhooks.slack.view import INVALID_SLACK_TOKEN_MESSAGE EXPECTED_TOPIC = "Message from Slack" @@ -311,6 +312,46 @@ class SlackWebhookTests(WebhookTestCase): self.assertFalse(m.called) self.assert_json_success(result) + def test_missing_api_token_scope(self) -> None: + error_message = "Slack token is missing the following required scopes: ['users:read', 'users:read.email']" + user_facing_error_message = INVALID_SLACK_TOKEN_MESSAGE.format(error_message=error_message) + # We tested how `check_token_access` may raise these errors in + # `test_slack_importer.py`. So, for simplicitys sake the function + # is directly mocked here to raise the ValueError we expect. + with ( + patch("zerver.webhooks.slack.view.check_token_access") as e, + patch("zerver.webhooks.slack.view.send_rate_limited_pm_notification_to_bot_owner") as s, + ): + e.side_effect = ValueError(error_message) + self.check_webhook( + "challenge_handshake_payload", + expect_noop=True, + content_type="application/json", + ) + + s.assert_called_once() + _, _, actual_error_message = s.call_args[0] + + self.assertEqual(actual_error_message, user_facing_error_message) + + def test_missing_slack_api_token(self) -> None: + error_message = "slack_app_token is missing." + self.url = self.build_webhook_url(slack_app_token="") + user_facing_error_message = INVALID_SLACK_TOKEN_MESSAGE.format(error_message=error_message) + with ( + patch("zerver.webhooks.slack.view.send_rate_limited_pm_notification_to_bot_owner") as s, + ): + self.check_webhook( + "challenge_handshake_payload", + expect_noop=True, + content_type="application/json", + ) + + s.assert_called_once() + _, _, actual_error_message = s.call_args[0] + + self.assertEqual(actual_error_message, user_facing_error_message) + class SlackLegacyWebhookTests(WebhookTestCase): CHANNEL_NAME = "slack" diff --git a/zerver/webhooks/slack/view.py b/zerver/webhooks/slack/view.py index 0feed11195..5334857471 100644 --- a/zerver/webhooks/slack/view.py +++ b/zerver/webhooks/slack/view.py @@ -5,6 +5,7 @@ from django.http import HttpRequest from django.http.response import HttpResponse from django.utils.translation import gettext as _ +from zerver.actions.message_send import send_rate_limited_pm_notification_to_bot_owner from zerver.data_import.slack import check_token_access, get_slack_api_data from zerver.data_import.slack_message_conversion import ( SLACK_BOLD_REGEX, @@ -169,6 +170,29 @@ def is_retry_call_from_slack(request: HttpRequest) -> bool: return "X-Slack-Retry-Num" in request.headers +SLACK_INTEGRATION_TOKEN_SCOPES = { + "channels:read", + "channels:history", + "users:read", + "emoji:read", + "team:read", + "users:read.email", +} + +INVALID_SLACK_TOKEN_MESSAGE = """ +Hi there! It looks like you're trying to set up a Slack webhook +integration. There seems to be an issue with the Slack app token +you've included in the URL (if any). Please check the error message +below to see if you're missing anything: + +Error: {error_message} + +Feel free to reach out to the [Zulip development community] +(https://chat.zulip.org/#narrow/channel/127-integrations) if you need +further help! +""" + + @webhook_view("Slack", notify_bot_owner_on_invalid_json=False) @typed_endpoint def api_slack_webhook( @@ -211,7 +235,19 @@ def api_slack_webhook( # Handle initial URL verification handshake for Slack Events API. if is_challenge_handshake(payload): challenge = payload.get("challenge").tame(check_string) - check_token_access(slack_app_token) + try: + if slack_app_token == "": + raise ValueError("slack_app_token is missing.") + check_token_access(slack_app_token, SLACK_INTEGRATION_TOKEN_SCOPES) + except (ValueError, Exception) as e: + send_rate_limited_pm_notification_to_bot_owner( + user_profile, + user_profile.realm, + INVALID_SLACK_TOKEN_MESSAGE.format(error_message=e), + ) + # Return json success here as to not trigger retry calls + # from Slack. + return json_success(request) check_send_webhook_message( request, user_profile,