paulmataruso/zulip
1
0
Fork 0
mirror of https://github.com/zulip/zulip.git synced 2025-10-23 04:52:12 +00:00
Code Issues Packages Projects Releases Wiki Activity

outgoing webhooks: Fix inconsistencies with Slack's API.

Browse Source 
Apparently, our slack compatible outgoing webhook format didn't
exactly match Slack, especially in the types used for values.  Fix
this by using a much more consistent format, where we preserve their
pattern of prefixing IDs with letters.

This fixes a bug where Zulip's team_id could be the empty string,
which tripped up using GitLab's slash commands with Zulip.

Fixes #19588.
This commit is contained in:
Tim Abbott
2021-09-17 12:22:23 -07:00
parent df8b8b9836
commit 24277a144e
3 changed files with 63 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files

23
templates/zerver/api/outgoing-webhooks.md
View File

@@ -119,27 +119,31 @@ Here's how we fill in the fields that a Slack-format webhook expects:
</tr> </tr>
<tr> <tr>
<td><code>team_id</code></td> <td><code>team_id</code></td>
<td>String ID of the Zulip organization</td> <td>ID of the Zulip organization prefixed by "T".</td>
</tr> </tr>
<tr> <tr>
<td><code>team_domain</code></td> <td><code>team_domain</code></td>
<td>Domain of the Zulip organization</td> <td>Hostname of the Zulip organization</td>
</tr> </tr>
<tr> <tr>
<td><code>channel_id</code></td> <td><code>channel_id</code></td>
<td>Stream ID</td> <td>Stream ID prefixed by "C"</td>
</tr> </tr>
<tr> <tr>
<td><code>channel_name</code></td> <td><code>channel_name</code></td>
<td>Stream name</td> <td>Stream name</td>
</tr> </tr>
<tr>
<td><code>thread_ts</code></td>
<td>Timestamp for when message was sent</td>
</tr>
<tr> <tr>
<td><code>timestamp</code></td> <td><code>timestamp</code></td>
<td>Timestamp for when message was sent</td> <td>Timestamp for when message was sent</td>
</tr> </tr>
<tr> <tr>
<td><code>user_id</code></td> <td><code>user_id</code></td>
<td>ID of the user who sent the message</td> <td>ID of the user who sent the message prefixed by "U"</td>
</tr> </tr>
<tr> <tr>
<td><code>user_name</code></td> <td><code>user_name</code></td>
@@ -164,13 +168,14 @@ The above data is posted as list of tuples (not JSON), here's an example:
``` ```
[('token', 'v9fpCdldZIej2bco3uoUvGp06PowKFOf'), [('token', 'v9fpCdldZIej2bco3uoUvGp06PowKFOf'),
('team_id', 'zulip'), ('team_id', 'T1512'),
('team_domain', 'zulip.com'), ('team_domain', 'zulip.example.com'),
('channel_id', '123'), ('channel_id', 'C123'),
('channel_name', 'integrations'), ('channel_name', 'integrations'),
('thread_ts', 1532078950),
('timestamp', 1532078950), ('timestamp', 1532078950),
('user_id', 21), ('user_id', 'U21'),
('user_name', 'Sample User'), ('user_name', 'Full Name'),
('text', '@**test**'), ('text', '@**test**'),
('trigger_word', 'mention'), ('trigger_word', 'mention'),
('service_id', 27)] ('service_id', 27)]

41
zerver/lib/outgoing_webhook.py
View File

@@ -20,9 +20,9 @@ from zerver.lib.url_encoding import near_message_url
from zerver.models import ( from zerver.models import (
GENERIC_INTERFACE, GENERIC_INTERFACE,
SLACK_INTERFACE, SLACK_INTERFACE,
Realm,
Service, Service,
UserProfile, UserProfile,
email_to_domain,
get_client, get_client,
get_user_profile_by_id, get_user_profile_by_id,
) )
@@ -40,7 +40,9 @@ class OutgoingWebhookServiceInterface(metaclass=abc.ABCMeta):
) )
@abc.abstractmethod @abc.abstractmethod
def make_request(self, base_url: str, event: Dict[str, Any]) -> Optional[Response]: def make_request(
self, base_url: str, event: Dict[str, Any], realm: Realm
) -> Optional[Response]:
raise NotImplementedError raise NotImplementedError
@abc.abstractmethod @abc.abstractmethod
@@ -49,7 +51,9 @@ class OutgoingWebhookServiceInterface(metaclass=abc.ABCMeta):
class GenericOutgoingWebhookService(OutgoingWebhookServiceInterface): class GenericOutgoingWebhookService(OutgoingWebhookServiceInterface):
def make_request(self, base_url: str, event: Dict[str, Any]) -> Optional[Response]: def make_request(
self, base_url: str, event: Dict[str, Any], realm: Realm
) -> Optional[Response]:
""" """
We send a simple version of the message to outgoing We send a simple version of the message to outgoing
webhooks, since most of them really only need webhooks, since most of them really only need
@@ -98,20 +102,38 @@ class GenericOutgoingWebhookService(OutgoingWebhookServiceInterface):
class SlackOutgoingWebhookService(OutgoingWebhookServiceInterface): class SlackOutgoingWebhookService(OutgoingWebhookServiceInterface):
def make_request(self, base_url: str, event: Dict[str, Any]) -> Optional[Response]: def make_request(
self, base_url: str, event: Dict[str, Any], realm: Realm
) -> Optional[Response]:
if event["message"]["type"] == "private": if event["message"]["type"] == "private":
failure_message = "Slack outgoing webhooks don't support private messages." failure_message = "Slack outgoing webhooks don't support private messages."
fail_with_message(event, failure_message) fail_with_message(event, failure_message)
return None return None
# https://api.slack.com/legacy/custom-integrations/outgoing-webhooks#legacy-info__post-data
# documents the Slack outgoing webhook format:
#
# token=XXXXXXXXXXXXXXXXXX
# team_id=T0001
# team_domain=example
# channel_id=C2147483705
# channel_name=test
# thread_ts=1504640714.003543
# timestamp=1504640775.000005
# user_id=U2147483697
# user_name=Steve
# text=googlebot: What is the air-speed velocity of an unladen swallow?
# trigger_word=googlebot:
request_data = [ request_data = [
("token", self.token), ("token", self.token),
("team_id", event["message"]["sender_realm_str"]), ("team_id", f"T{realm.id}"),
("team_domain", email_to_domain(event["message"]["sender_email"])), ("team_domain", realm.host),
("channel_id", event["message"]["stream_id"]), ("channel_id", f"C{event['message']['stream_id']}"),
("channel_name", event["message"]["display_recipient"]), ("channel_name", event["message"]["display_recipient"]),
("thread_ts", event["message"]["timestamp"]),
("timestamp", event["message"]["timestamp"]), ("timestamp", event["message"]["timestamp"]),
("user_id", event["message"]["sender_id"]), ("user_id", f"U{event['message']['sender_id']}"),
("user_name", event["message"]["sender_full_name"]), ("user_name", event["message"]["sender_full_name"]),
("text", event["command"]), ("text", event["command"]),
("trigger_word", event["trigger"]), ("trigger_word", event["trigger"]),
@@ -326,11 +348,12 @@ def do_rest_call(
"""Returns response of call if no exception occurs.""" """Returns response of call if no exception occurs."""
try: try:
start_time = perf_counter() start_time = perf_counter()
bot_profile = service_handler.user_profile
response = service_handler.make_request( response = service_handler.make_request(
base_url, base_url,
event, event,
bot_profile.realm,
) )
bot_profile = service_handler.user_profile
logging.info( logging.info(
"Outgoing webhook request from %s@%s took %f seconds", "Outgoing webhook request from %s@%s took %f seconds",
bot_profile.id, bot_profile.id,

27
zerver/tests/test_outgoing_webhook_interfaces.py
View File

@@ -107,6 +107,7 @@ class TestGenericOutgoingWebhookService(ZulipTestCase):
self.handler.make_request( self.handler.make_request(
test_url, test_url,
event, event,
othello.realm,
) )
session.post.assert_called_once() session.post.assert_called_once()
self.assertEqual(session.post.call_args[0], (test_url,)) self.assertEqual(session.post.call_args[0], (test_url,))
@@ -150,6 +151,7 @@ class TestGenericOutgoingWebhookService(ZulipTestCase):
class TestSlackOutgoingWebhookService(ZulipTestCase): class TestSlackOutgoingWebhookService(ZulipTestCase):
def setUp(self) -> None: def setUp(self) -> None:
super().setUp() super().setUp()
self.bot_user = get_user("outgoing-webhook@zulip.com", get_realm("zulip"))
self.stream_message_event = { self.stream_message_event = {
"command": "@**test**", "command": "@**test**",
"user_profile_id": 12, "user_profile_id": 12,
@@ -187,7 +189,9 @@ class TestSlackOutgoingWebhookService(ZulipTestCase):
} }
service_class = get_service_interface_class(SLACK_INTERFACE) service_class = get_service_interface_class(SLACK_INTERFACE)
self.handler = service_class(token="abcdef", user_profile=None, service_name="test-service") self.handler = service_class(
token="abcdef", user_profile=self.bot_user, service_name="test-service"
)
def test_make_request_stream_message(self) -> None: def test_make_request_stream_message(self) -> None:
test_url = "https://example.com/example" test_url = "https://example.com/example"
@@ -195,22 +199,24 @@ class TestSlackOutgoingWebhookService(ZulipTestCase):
self.handler.make_request( self.handler.make_request(
test_url, test_url,
self.stream_message_event, self.stream_message_event,
self.bot_user.realm,
) )
session.post.assert_called_once() session.post.assert_called_once()
self.assertEqual(session.post.call_args[0], (test_url,)) self.assertEqual(session.post.call_args[0], (test_url,))
request_data = session.post.call_args[1]["data"] request_data = session.post.call_args[1]["data"]
self.assertEqual(request_data[0][1], "abcdef") # token self.assertEqual(request_data[0][1], "abcdef") # token
self.assertEqual(request_data[1][1], "zulip") # team_id self.assertEqual(request_data[1][1], "T2") # team_id
self.assertEqual(request_data[2][1], "zulip.com") # team_domain self.assertEqual(request_data[2][1], "zulip.testserver") # team_domain
self.assertEqual(request_data[3][1], "123") # channel_id self.assertEqual(request_data[3][1], "C123") # channel_id
self.assertEqual(request_data[4][1], "integrations") # channel_name self.assertEqual(request_data[4][1], "integrations") # channel_name
self.assertEqual(request_data[5][1], 123456) # timestamp self.assertEqual(request_data[5][1], 123456) # thread_id
self.assertEqual(request_data[6][1], 21) # user_id self.assertEqual(request_data[6][1], 123456) # timestamp
self.assertEqual(request_data[7][1], "Sample User") # user_name self.assertEqual(request_data[7][1], "U21") # user_id
self.assertEqual(request_data[8][1], "@**test**") # text self.assertEqual(request_data[8][1], "Sample User") # user_name
self.assertEqual(request_data[9][1], "mention") # trigger_word self.assertEqual(request_data[9][1], "@**test**") # text
self.assertEqual(request_data[10][1], 12) # user_profile_id self.assertEqual(request_data[10][1], "mention") # trigger_word
self.assertEqual(request_data[11][1], 12) # user_profile_id
@mock.patch("zerver.lib.outgoing_webhook.fail_with_message") @mock.patch("zerver.lib.outgoing_webhook.fail_with_message")
def test_make_request_private_message(self, mock_fail_with_message: mock.Mock) -> None: def test_make_request_private_message(self, mock_fail_with_message: mock.Mock) -> None:
@@ -219,6 +225,7 @@ class TestSlackOutgoingWebhookService(ZulipTestCase):
response = self.handler.make_request( response = self.handler.make_request(
test_url, test_url,
self.private_message_event, self.private_message_event,
self.bot_user.realm,
) )
session.post.assert_not_called() session.post.assert_not_called()
self.assertIsNone(response) self.assertIsNone(response)