mirror of
https://github.com/zulip/zulip.git
synced 2025-11-03 05:23:35 +00:00
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 f2599bf33d)
This commit is contained in:
@@ -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"
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user