webhooks: Stop raising an exception if stream does not exist.

webhook-errors.log file is cluttered with Stream.DoesNotExist
errors, which hides the errors that we actually need to see. So,
since check_message already sends the bot_owner a PM if the webhook
bot tries to send a message to a non-existent stream, we can ignore
such exceptions.
This commit is contained in:
Eeshan Garg
2018-03-22 18:13:28 -02:30
committed by Tim Abbott
parent 4b8bd0bc3b
commit 538746fc65
6 changed files with 44 additions and 7 deletions

View File

@@ -313,9 +313,10 @@
"var_name": "content" "var_name": "content"
}, },
"nonexistent-stream-error": { "nonexistent-stream-error": {
"code": "BAD_REQUEST", "code": "STREAM_DOES_NOT_EXIST",
"msg": "Stream 'nonexistent_stream' does not exist", "msg": "Stream 'nonexistent_stream' does not exist",
"result": "error" "result": "error",
"stream": "nonexistent_stream"
}, },
"private-message": { "private-message": {
"id": 134, "id": 134,

View File

@@ -36,6 +36,7 @@ from zerver.lib.cache import (
) )
from zerver.lib.context_managers import lockfile from zerver.lib.context_managers import lockfile
from zerver.lib.emoji import emoji_name_to_emoji_code, get_emoji_file_name from zerver.lib.emoji import emoji_name_to_emoji_code, get_emoji_file_name
from zerver.lib.exceptions import StreamDoesNotExistError
from zerver.lib.hotspots import get_next_hotspots from zerver.lib.hotspots import get_next_hotspots
from zerver.lib.message import ( from zerver.lib.message import (
access_message, access_message,
@@ -1869,8 +1870,7 @@ def check_message(sender: UserProfile, client: Client, addressee: Addressee,
except Stream.DoesNotExist: except Stream.DoesNotExist:
send_pm_if_empty_stream(sender, None, stream_name, realm) send_pm_if_empty_stream(sender, None, stream_name, realm)
raise JsonableError(_("Stream '%(stream_name)s' " raise StreamDoesNotExistError(escape(stream_name))
"does not exist") % {'stream_name': escape(stream_name)})
recipient = get_stream_recipient(stream.id) recipient = get_stream_recipient(stream.id)
if not stream.invite_only: if not stream.invite_only:

View File

@@ -2,6 +2,7 @@ from enum import Enum
from typing import Any, Dict, List, Optional, Text, Type from typing import Any, Dict, List, Optional, Text, Type
from django.core.exceptions import PermissionDenied from django.core.exceptions import PermissionDenied
from django.utils.translation import ugettext as _
class AbstractEnum(Enum): class AbstractEnum(Enum):
'''An enumeration whose members are used strictly for their names.''' '''An enumeration whose members are used strictly for their names.'''
@@ -29,6 +30,7 @@ class ErrorCode(AbstractEnum):
BAD_IMAGE = () BAD_IMAGE = ()
REALM_UPLOAD_QUOTA = () REALM_UPLOAD_QUOTA = ()
BAD_NARROW = () BAD_NARROW = ()
STREAM_DOES_NOT_EXIST = ()
UNAUTHORIZED_PRINCIPAL = () UNAUTHORIZED_PRINCIPAL = ()
BAD_EVENT_QUEUE_ID = () BAD_EVENT_QUEUE_ID = ()
CSRF_FAILED = () CSRF_FAILED = ()
@@ -134,6 +136,17 @@ class JsonableError(Exception):
def __str__(self) -> str: def __str__(self) -> str:
return self.msg return self.msg
class StreamDoesNotExistError(JsonableError):
code = ErrorCode.STREAM_DOES_NOT_EXIST
data_fields = ['stream']
def __init__(self, stream: str) -> None:
self.stream = stream
@staticmethod
def msg_format() -> str:
return _("Stream '{stream}' does not exist")
class RateLimited(PermissionDenied): class RateLimited(PermissionDenied):
def __init__(self, msg: str="") -> None: def __init__(self, msg: str="") -> None:
super().__init__(msg) super().__init__(msg)

View File

@@ -652,7 +652,8 @@ class WebhookTestCase(ZulipTestCase):
if content_type is not None: if content_type is not None:
kwargs['content_type'] = content_type kwargs['content_type'] = content_type
msg = self.send_json_payload(self.test_user, self.url, payload, sender = kwargs.get('sender', self.test_user)
msg = self.send_json_payload(sender, self.url, payload,
stream_name=None, **kwargs) stream_name=None, **kwargs)
self.do_test_message(msg, expected_message) self.do_test_message(msg, expected_message)

View File

@@ -3,9 +3,11 @@ from typing import Optional, Text
from zerver.lib.actions import check_send_stream_message, \ from zerver.lib.actions import check_send_stream_message, \
check_send_private_message check_send_private_message
from zerver.lib.exceptions import StreamDoesNotExistError
from zerver.lib.request import REQ, has_request_variables from zerver.lib.request import REQ, has_request_variables
from zerver.models import UserProfile from zerver.models import UserProfile
@has_request_variables @has_request_variables
def check_send_webhook_message( def check_send_webhook_message(
request: HttpRequest, user_profile: UserProfile, request: HttpRequest, user_profile: UserProfile,
@@ -20,5 +22,13 @@ def check_send_webhook_message(
else: else:
if user_specified_topic is not None: if user_specified_topic is not None:
topic = user_specified_topic topic = user_specified_topic
check_send_stream_message(user_profile, request.client,
stream, topic, body) try:
check_send_stream_message(user_profile, request.client,
stream, topic, body)
except StreamDoesNotExistError:
# A PM will be sent to the bot_owner by check_message, notifying
# that the webhook bot just tried to send a message to a non-existent
# stream, so we don't need to re-raise it since it clutters up
# webhook-errors.log
pass

View File

@@ -1,7 +1,9 @@
# -*- coding: utf-8 -*- # -*- coding: utf-8 -*-
from django.conf import settings
from typing import Text from typing import Text
from zerver.lib.test_classes import WebhookTestCase from zerver.lib.test_classes import WebhookTestCase
from zerver.models import get_system_bot
class HelloWorldHookTests(WebhookTestCase): class HelloWorldHookTests(WebhookTestCase):
STREAM_NAME = 'test' STREAM_NAME = 'test'
@@ -35,6 +37,16 @@ class HelloWorldHookTests(WebhookTestCase):
self.send_and_test_private_message('goodbye', expected_message=expected_message, self.send_and_test_private_message('goodbye', expected_message=expected_message,
content_type="application/x-www-form-urlencoded") content_type="application/x-www-form-urlencoded")
def test_stream_error_pm_to_bot_owner(self) -> None:
# Note taht this is really just a test for check_send_webhook_message
self.STREAM_NAME = 'nonexistent'
self.url = self.build_webhook_url()
notification_bot = get_system_bot(settings.NOTIFICATION_BOT)
expected_message = "Hi there! We thought you'd like to know that your bot **Zulip Webhook Bot** just tried to send a message to stream `nonexistent`, but that stream does not yet exist. To create it, click the gear in the left-side stream list."
self.send_and_test_private_message('goodbye', expected_message=expected_message,
content_type='application/x-www-form-urlencoded',
sender=notification_bot)
def test_custom_topic(self) -> None: def test_custom_topic(self) -> None:
# Note that this is really just a test for check_send_webhook_message # Note that this is really just a test for check_send_webhook_message
expected_subject = u"Custom Topic" expected_subject = u"Custom Topic"