diff --git a/zerver/lib/push_notifications.py b/zerver/lib/push_notifications.py index f4ed87a28e..326fba0bda 100644 --- a/zerver/lib/push_notifications.py +++ b/zerver/lib/push_notifications.py @@ -21,7 +21,7 @@ from django.utils.translation import override as override_language from zerver.decorator import statsd_increment from zerver.lib.avatar import absolute_avatar_url from zerver.lib.exceptions import JsonableError -from zerver.lib.message import access_message, bulk_access_messages_expect_usermessage, huddle_users +from zerver.lib.message import access_message, huddle_users from zerver.lib.outgoing_http import OutgoingSession from zerver.lib.remote_server import send_json_to_push_bouncer, send_to_push_bouncer from zerver.lib.timestamp import datetime_to_timestamp @@ -911,7 +911,16 @@ def handle_remove_push_notification(user_profile_id: int, message_ids: List[int] return user_profile = get_user_profile_by_id(user_profile_id) - message_ids = bulk_access_messages_expect_usermessage(user_profile_id, message_ids) + + # We may no longer have access to the message here; for example, + # the user (1) got a message, (2) read the message in the web UI, + # and then (3) it was deleted. When trying to send the push + # notification for (2), after (3) has happened, there is no + # message to fetch -- but we nonetheless want to remove the mobile + # notification. Because of this, the usual access control of + # `bulk_access_messages_expect_usermessage` is skipped here. + # Because of this, no access to the Message objects should be + # done; they are treated as a list of opaque ints. # APNs has a 4KB limit on the maximum size of messages, which # translated to several hundred message IDs in one of these diff --git a/zerver/tests/test_push_notifications.py b/zerver/tests/test_push_notifications.py index 3083635de2..e0aec5dc7a 100644 --- a/zerver/tests/test_push_notifications.py +++ b/zerver/tests/test_push_notifications.py @@ -1379,6 +1379,28 @@ class HandlePushNotificationTest(PushNotificationTest): ) mock_push_notifications.assert_called_once() + def test_user_message_does_not_exist_remove(self) -> None: + """This simulates a condition that should only be an error if the user is + not long-term idle; we fake it, though, in the sense that the user should + not have received the message in the first place""" + self.setup_apns_tokens() + self.setup_gcm_tokens() + self.make_stream("public_stream") + sender = self.example_user("iago") + self.subscribe(sender, "public_stream") + message_id = self.send_stream_message(sender, "public_stream", "test") + with mock.patch( + "zerver.lib.push_notifications.push_notifications_enabled", return_value=True + ) as mock_push_notifications, mock.patch( + "zerver.lib.push_notifications.send_android_push_notification" + ) as mock_send_android, mock.patch( + "zerver.lib.push_notifications.send_apple_push_notification" + ) as mock_send_apple: + handle_remove_push_notification(self.user_profile.id, [message_id]) + mock_push_notifications.assert_called_once() + mock_send_android.assert_called_once() + mock_send_apple.assert_called_once() + def test_user_message_soft_deactivated(self) -> None: """This simulates a condition that should only be an error if the user is not long-term idle; we fake it, though, in the sense that the user should