push_notifications: Remove access control on "remove" notifications.

When removing notifications, we skip the access control on if the user
still can read them -- they should not have a notification of them,
both because they currently cannot read the message, as well as
because they have already done so.

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

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