From d5cc29755e34129c13d41f5780e4d7554daff4e8 Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Fri, 28 Feb 2020 17:23:57 +0000 Subject: [PATCH] typing: Accept only user_ids in typing endpoint. The only clients that should use the typing indicators endpoint are our internal clients, and they should send a JSON-formatted list of user_ids. We now enforce this, which removes some complexity surrounding legacy ways of sending users, such as emails and comma-delimited strings of user_ids. There may be a very tiny number of mobile clients that still use the old emails API. This won't have any user-facing effect on the mobile users themselves, but if you type a message to your friend on an old mobile app, the friend will no longer see typing indicators. Also, the mobile team may see some errors in their Sentry logs from the server rejecting posts from the old mobile clients. The error messages we report here are a bit more generic, since we now just use REQ to do validation with this code: validator=check_list(check_int) This also allows us to remove a test hack related to the API documentation. (We changed the docs to reflect the modern API in an earlier commit, but the tests couldn't be fixed while we still had the more complex semantics for the "to" parameter.) --- templates/zerver/api/changelog.md | 2 ++ version.py | 2 +- zerver/lib/actions.py | 36 ++++------------------------ zerver/openapi/zulip.yaml | 4 ++-- zerver/tests/test_openapi.py | 11 --------- zerver/tests/test_typing.py | 39 ++----------------------------- zerver/views/typing.py | 19 ++++++--------- 7 files changed, 18 insertions(+), 95 deletions(-) diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md index 1b0a80c069..58a282631e 100644 --- a/templates/zerver/api/changelog.md +++ b/templates/zerver/api/changelog.md @@ -18,6 +18,8 @@ below features are supported. value means no limit. * [`POST /register`](/api/register-queue): The response now contains a `is_owner`, similar to the existing `is_admin` and `is_guest` fields. +* [`POST /typing`](/api/typing): Removed legacy support for sending email + addresses, rather than user IDs, to encode private message recipients. **Feature level 10** diff --git a/version.py b/version.py index 58616feaf2..28aa9f4448 100644 --- a/version.py +++ b/version.py @@ -29,7 +29,7 @@ DESKTOP_WARNING_VERSION = "5.2.0" # # Changes should be accompanied by documentation explaining what the # new level means in templates/zerver/api/changelog.md. -API_FEATURE_LEVEL = 10 +API_FEATURE_LEVEL = 11 # Bump the minor PROVISION_VERSION to indicate that folks should provision # only when going from an old version of the code to a newer version. Bump diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index a32462f9c7..0dc4d5d9f6 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -113,7 +113,7 @@ from zerver.models import Realm, RealmEmoji, Stream, UserProfile, UserActivity, query_for_ids, get_huddle_recipient, \ UserGroup, UserGroupMembership, get_default_stream_groups, \ get_bot_services, get_bot_dicts_in_realm, \ - get_user_including_cross_realm, get_user_by_id_in_realm_including_cross_realm, \ + get_user_by_id_in_realm_including_cross_realm, \ get_stream_by_id_in_realm from zerver.lib.alert_words import get_alert_word_automaton @@ -1782,11 +1782,12 @@ def do_send_typing_notification( # check_send_typing_notification: # Checks the typing notification and sends it -def check_send_typing_notification(sender: UserProfile, notification_to: Union[Sequence[str], Sequence[int]], +def check_send_typing_notification(sender: UserProfile, + user_ids: List[int], operator: str) -> None: realm = sender.realm - if len(notification_to) == 0: + if len(user_ids) == 0: raise JsonableError(_('Missing parameter: \'to\' (recipient)')) elif operator not in ('start', 'stop'): raise JsonableError(_('Invalid \'op\' value (should be start or stop)')) @@ -1799,16 +1800,6 @@ def check_send_typing_notification(sender: UserProfile, notification_to: Union[S to mostly delete code when we desupport old versions of the app.''' - if isinstance(notification_to[0], int): - user_ids = cast(List[int], notification_to) - else: - try: - emails = cast(Sequence[str], notification_to) - user_ids = user_ids_for_emails(realm, emails) - except ValidationError as e: - assert isinstance(e.messages[0], str) - raise JsonableError(e.messages[0]) - if sender.id not in user_ids: user_ids.append(sender.id) @@ -1908,25 +1899,6 @@ def validate_recipient_user_profiles(user_profiles: Sequence[UserProfile], return list(recipient_profiles_map.values()) -def user_ids_for_emails( - realm: Realm, - emails: Iterable[str], -) -> List[int]: - ''' - This function should only stay around while - we still have to support mobile sending emails - in typing notifications. - ''' - user_ids: List[int] = [] - for email in emails: - try: - user_profile = get_user_including_cross_realm(email, realm) - except UserProfile.DoesNotExist: - raise ValidationError(_("Invalid email '%s'") % (email,)) - user_ids.append(user_profile.id) - - return user_ids - def recipient_for_user_profiles(user_profiles: Sequence[UserProfile], forwarded_mirror_message: bool, forwarder_user_profile: Optional[UserProfile], sender: UserProfile, allow_deactivated: bool=False) -> Recipient: diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 5a2930c869..db0d0297a4 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -3931,8 +3931,8 @@ paths: recipient.). **Changes**: Before Zulip 2.0, this parameter accepted only a JSON-encoded - list of email addresses. The email address-based format is deprecated - and will be removed in Zulip 2.2. + list of email addresses. Support for the email address-based format was + removed in Zulip 2.2 (feature level 11). schema: type: array items: diff --git a/zerver/tests/test_openapi.py b/zerver/tests/test_openapi.py index 9b27c51eba..6bfa44a4a2 100644 --- a/zerver/tests/test_openapi.py +++ b/zerver/tests/test_openapi.py @@ -429,17 +429,6 @@ do not match the types declared in the implementation of {}.\n""".format(functio AssertionError. """ openapi_params: Set[Tuple[str, Union[type, Tuple[type, object]]]] = set() for element in openapi_parameters: - if function.__name__ == 'send_notification_backend': - if element['name'] == 'to': - ''' - We want users to send ints here, but the mypy - types for send_notification_backend are still - str, because we need backward compatible - support for old versions of mobile that still - send emails for typing requests. - ''' - continue - name: str = element["name"] schema = {} if "content" in element: diff --git a/zerver/tests/test_typing.py b/zerver/tests/test_typing.py index f8f5e55c64..01284b942a 100644 --- a/zerver/tests/test_typing.py +++ b/zerver/tests/test_typing.py @@ -52,7 +52,7 @@ class TypingValidateUsersTest(ZulipTestCase): """ sender = self.example_user("hamlet") result = self.api_post(sender, '/api/v1/typing', {'op': 'start'}) - self.assert_json_error(result, "Missing parameter: 'to' (recipient)") + self.assert_json_error(result, "Missing 'to' argument") def test_argument_to_is_not_valid_json(self) -> None: """ @@ -61,7 +61,7 @@ class TypingValidateUsersTest(ZulipTestCase): sender = self.example_user("hamlet") invalid = 'bad email' result = self.api_post(sender, '/api/v1/typing', {'op': 'start', 'to': invalid}) - self.assert_json_error(result, "Invalid email 'bad email'") + self.assert_json_error(result, 'Argument "to" is not valid JSON.') def test_bogus_user_id(self) -> None: """ @@ -282,38 +282,3 @@ class TypingHappyPathTest(ZulipTestCase): self.assertEqual(event['sender']['email'], sender.email) self.assertEqual(event['type'], 'typing') self.assertEqual(event['op'], 'stop') - -class TypingLegacyMobileSupportTest(ZulipTestCase): - def test_legacy_email_interface(self) -> None: - ''' - We are keeping the email interface on life support - for a couple months until we get some of our - mobile users upgraded. - ''' - sender = self.example_user('hamlet') - othello = self.example_user('othello') - cordelia = self.example_user('cordelia') - - emails = [othello.email, cordelia.email] - - params = dict( - to=ujson.dumps(emails), - op='start', - ) - - events: List[Mapping[str, Any]] = [] - with tornado_redirected_to_list(events): - result = self.api_post(sender, '/api/v1/typing', params) - - self.assert_json_success(result) - event = events[0]['event'] - - event_recipient_user_ids = { - user['user_id'] - for user in event['recipients'] - } - - self.assertEqual( - event_recipient_user_ids, - {sender.id, othello.id, cordelia.id} - ) diff --git a/zerver/views/typing.py b/zerver/views/typing.py index 39e317bfb7..08f5ca1b01 100644 --- a/zerver/views/typing.py +++ b/zerver/views/typing.py @@ -1,22 +1,17 @@ from django.http import HttpRequest, HttpResponse -from typing import List, Union +from typing import List from zerver.decorator import has_request_variables, REQ -from zerver.lib.actions import check_send_typing_notification, \ - extract_private_recipients +from zerver.lib.actions import check_send_typing_notification from zerver.lib.response import json_success +from zerver.lib.validator import check_int, check_list from zerver.models import UserProfile -EMPTY_STRS: List[str] = [] -EMPTY_STRS_OR_INTS: Union[List[str], List[int]] = EMPTY_STRS - @has_request_variables def send_notification_backend( - request: HttpRequest, user_profile: UserProfile, - operator: str=REQ('op'), - notification_to: Union[List[str], List[int]]=REQ( - 'to', converter=extract_private_recipients, default=EMPTY_STRS_OR_INTS, - ), -) -> HttpResponse: + request: HttpRequest, + user_profile: UserProfile, + operator: str=REQ('op'), + notification_to: List[int]=REQ('to', validator=check_list(check_int))) -> HttpResponse: check_send_typing_notification(user_profile, notification_to, operator) return json_success()