From fd62e71737390aa08639bcf5bb0b07fdf0686aeb Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Sat, 27 Oct 2018 14:37:29 +0000 Subject: [PATCH] Clean up URLs sent by outgoing webhooks. When you send a message to a bot that wants to talk via an outgoing webhook, and there's an error (e.g. server is down), we send a message to the bot's owner that links to the message that triggered the error. The code to produce those links was out of date. Now we move the important code to the `url_encoding.py` library and fix the PM links to use the more modern style (user_ids instead of emails). We also replace "subject" with "topic" in the stream urls. --- zerver/lib/outgoing_webhook.py | 23 +++---- zerver/lib/url_encoding.py | 63 +++++++++++++++++++- zerver/tests/test_messages.py | 19 +++++- zerver/tests/test_outgoing_webhook_system.py | 6 +- 4 files changed, 90 insertions(+), 21 deletions(-) diff --git a/zerver/lib/outgoing_webhook.py b/zerver/lib/outgoing_webhook.py index 9505dd7fb2..18f8a9de02 100644 --- a/zerver/lib/outgoing_webhook.py +++ b/zerver/lib/outgoing_webhook.py @@ -16,7 +16,7 @@ from zerver.models import Realm, UserProfile, get_user_profile_by_id, get_client GENERIC_INTERFACE, Service, SLACK_INTERFACE, email_to_domain, get_service_profile from zerver.lib.actions import check_send_message from zerver.lib.queue import retry_event -from zerver.lib.url_encoding import encode_stream +from zerver.lib.url_encoding import near_message_url from zerver.lib.validator import check_dict, check_string from zerver.decorator import JsonableError @@ -176,21 +176,12 @@ def fail_with_message(event: Dict[str, Any], failure_message: str) -> None: def get_message_url(event: Dict[str, Any], request_data: Dict[str, Any]) -> str: bot_user = get_user_profile_by_id(event['user_profile_id']) message = event['message'] - if message['type'] == 'stream': - stream_url_frag = encode_stream(message.get('stream_id'), message['display_recipient']) - message_url = ("%(server)s/#narrow/stream/%(stream)s/subject/%(subject)s/near/%(id)s" - % {'server': bot_user.realm.uri, - 'stream': stream_url_frag, - 'subject': message['subject'], - 'id': str(message['id'])}) - else: - recipient_emails = ','.join([recipient['email'] for recipient in message['display_recipient']]) - recipient_email_encoded = urllib.parse.quote(recipient_emails).replace('.', '%2E').replace('%', '.') - message_url = ("%(server)s/#narrow/pm-with/%(recipient_emails)s/near/%(id)s" - % {'server': bot_user.realm.uri, - 'recipient_emails': recipient_email_encoded, - 'id': str(message['id'])}) - return message_url + realm = bot_user.realm + + return near_message_url( + realm=realm, + message=message, + ) def notify_bot_owner(event: Dict[str, Any], request_data: Dict[str, Any], diff --git a/zerver/lib/url_encoding.py b/zerver/lib/url_encoding.py index 702c29bc05..4f50cc5105 100644 --- a/zerver/lib/url_encoding.py +++ b/zerver/lib/url_encoding.py @@ -1,5 +1,5 @@ import urllib -from typing import List +from typing import Any, Dict, List from zerver.models import Realm, Stream @@ -29,3 +29,64 @@ def topic_narrow_url(realm: Realm, stream: Stream, topic: str) -> str: return "%s%s/topic/%s" % (base_url, encode_stream(stream.id, stream.name), hash_util_encode(topic)) + +def near_message_url(realm: Realm, + message: Dict[str, Any]) -> str: + + if message['type'] == 'stream': + url = near_stream_message_url( + realm=realm, + message=message, + ) + return url + + url = near_pm_message_url( + realm=realm, + message=message, + ) + return url + +def near_stream_message_url(realm: Realm, + message: Dict[str, Any]) -> str: + message_id = str(message['id']) + stream_id = message['stream_id'] + stream_name = message['display_recipient'] + topic_name = message['subject'] + encoded_topic = hash_util_encode(topic_name) + encoded_stream = encode_stream(stream_id=stream_id, stream_name=stream_name) + + parts = [ + realm.uri, + '#narrow', + 'stream', + encoded_stream, + 'topic', + encoded_topic, + 'near', + message_id, + ] + full_url = '/'.join(parts) + return full_url + +def near_pm_message_url(realm: Realm, + message: Dict[str, Any]) -> str: + message_id = str(message['id']) + str_user_ids = [ + str(recipient['id']) + for recipient in message['display_recipient'] + ] + + # Use the "perma-link" format here that includes the sender's + # user_id, so they're easier to share between people. + pm_str = ','.join(str_user_ids) + '-pm' + + parts = [ + realm.uri, + '#narrow', + 'pm-with', + pm_str, + 'near', + message_id, + ] + full_url = '/'.join(parts) + return full_url diff --git a/zerver/tests/test_messages.py b/zerver/tests/test_messages.py index 22e96b45ed..6261dbcf60 100644 --- a/zerver/tests/test_messages.py +++ b/zerver/tests/test_messages.py @@ -84,9 +84,10 @@ from zerver.models import ( ) -from zerver.lib.upload import create_attachment from zerver.lib.timestamp import convert_to_UTC, datetime_to_timestamp from zerver.lib.timezone import get_timezone +from zerver.lib.upload import create_attachment +from zerver.lib.url_encoding import near_message_url from zerver.views.messages import create_mirrored_message_users @@ -528,6 +529,22 @@ class ExtractedRecipientsTest(TestCase): class PersonalMessagesTest(ZulipTestCase): + def test_near_pm_message_url(self) -> None: + realm = get_realm('zulip') + message = dict( + type='personal', + id=555, + display_recipient=[ + dict(id=77), + dict(id=80), + ], + ) + url = near_message_url( + realm=realm, + message=message, + ) + self.assertEqual(url, 'http://zulip.testserver/#narrow/pm-with/77,80-pm/near/555') + def test_is_private_flag_not_leaked(self) -> None: """ Make sure `is_private` flag is not leaked to the API. diff --git a/zerver/tests/test_outgoing_webhook_system.py b/zerver/tests/test_outgoing_webhook_system.py index 1068902af4..fc701f2b5c 100644 --- a/zerver/tests/test_outgoing_webhook_system.py +++ b/zerver/tests/test_outgoing_webhook_system.py @@ -78,7 +78,7 @@ class DoRestCallTests(ZulipTestCase): do_rest_call('', None, self.mock_event, service_handler) bot_owner_notification = self.get_last_message() self.assertEqual(bot_owner_notification.content, - '''[A message](http://zulip.testserver/#narrow/stream/999-Verona/subject/Foo/near/) triggered an outgoing webhook. + '''[A message](http://zulip.testserver/#narrow/stream/999-Verona/topic/Foo/near/) triggered an outgoing webhook. The webhook got a response with status code *500*.''') self.assertEqual(bot_owner_notification.recipient_id, self.bot_user.bot_owner.id) self.mock_event['failed_tries'] = 0 @@ -91,7 +91,7 @@ The webhook got a response with status code *500*.''') bot_owner_notification = self.get_last_message() self.assertTrue(mock_fail_with_message.called) self.assertEqual(bot_owner_notification.content, - '''[A message](http://zulip.testserver/#narrow/stream/999-Verona/subject/Foo/near/) triggered an outgoing webhook. + '''[A message](http://zulip.testserver/#narrow/stream/999-Verona/topic/Foo/near/) triggered an outgoing webhook. The webhook got a response with status code *400*.''') self.assertEqual(bot_owner_notification.recipient_id, self.bot_user.bot_owner.id) @@ -117,7 +117,7 @@ The webhook got a response with status code *400*.''') bot_owner_notification = self.get_last_message() self.assertTrue(mock_fail_with_message.called) self.assertEqual(bot_owner_notification.content, - '''[A message](http://zulip.testserver/#narrow/stream/999-Verona/subject/Foo/near/) triggered an outgoing webhook. + '''[A message](http://zulip.testserver/#narrow/stream/999-Verona/topic/Foo/near/) triggered an outgoing webhook. When trying to send a request to the webhook service, an exception of type RequestException occurred: ``` I'm a generic exception :(