Mirror
/
zulip
mirror of https://github.com/zulip/zulip.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

zilencer: Delete duplicate remote push registrations. 

This fixes existing instances of the bug fixed in the previous commit.

Fixes #24969.
This commit is contained in:
Mateusz Mandera 2023-04-10 00:55:16 +02:00 committed by Tim Abbott
parent ade2225f08
commit 2a45429a51
2 changed files with 90 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

25
zerver/tests/test_push_notifications.py
View File

@ -336,6 +336,7 @@ class PushBouncerNotificationTest(BouncerTestCase):
server = RemoteZulipServer.objects.get(uuid=self.server_uuid)
token = "aaaa"
android_tokens = []
uuid_android_tokens = []
for i in ["aa", "bb"]:
android_tokens.append(
RemotePushDeviceToken.objects.create(
@ -345,6 +346,19 @@ class PushBouncerNotificationTest(BouncerTestCase):
server=server,
)
)
# Create a duplicate, newer uuid-based registration for the same user to verify
# the bouncer will handle that correctly, without triggering a duplicate notification,
# and will delete the old, legacy registration.
uuid_android_tokens.append(
RemotePushDeviceToken.objects.create(
kind=RemotePushDeviceToken.GCM,
token=hex_to_b64(token + i),
user_uuid=str(hamlet.uuid),
server=server,
)
)
apple_token = RemotePushDeviceToken.objects.create(
kind=RemotePushDeviceToken.APNS,
token=hex_to_b64(token),
@ -354,6 +368,7 @@ class PushBouncerNotificationTest(BouncerTestCase):
many_ids = ",".join(str(i) for i in range(1, 250))
payload = {
"user_id": hamlet.id,
"user_uuid": str(hamlet.uuid),
"gcm_payload": {"event": "remove", "zulip_message_ids": many_ids},
"apns_payload": {
"badge": 0,
@ -383,12 +398,14 @@ class PushBouncerNotificationTest(BouncerTestCase):
logger.output,
[
"INFO:zilencer.views:"
f"Sending mobile push notifications for remote user 6cde5f7a-1f7e-4978-9716-49f69ebfc9fe:<id:{hamlet.id}>: "
"2 via FCM devices, 1 via APNs devices"
f"Deduplicating push registrations for server id:{server.id} user id:{hamlet.id} uuid:{str(hamlet.uuid)} and tokens:{sorted([t.token for t in android_tokens[:]])}",
"INFO:zilencer.views:"
f"Sending mobile push notifications for remote user 6cde5f7a-1f7e-4978-9716-49f69ebfc9fe:<id:{hamlet.id}><uuid:{str(hamlet.uuid)}>: "
"2 via FCM devices, 1 via APNs devices",
],
)
user_identity = UserPushIdentityCompat(user_id=hamlet.id)
user_identity = UserPushIdentityCompat(user_id=hamlet.id, user_uuid=str(hamlet.uuid))
apple_push.assert_called_once_with(
user_identity,
[apple_token],
@ -405,7 +422,7 @@ class PushBouncerNotificationTest(BouncerTestCase):
)
android_push.assert_called_once_with(
user_identity,
list(reversed(android_tokens)),
list(reversed(uuid_android_tokens)),
{"event": "remove", "zulip_message_ids": ",".join(str(i) for i in range(50, 250))},
{},
remote=server,

71
zilencer/views.py
View File

@ -1,5 +1,6 @@
import datetime
import logging
from collections import Counter
from typing import Any, Dict, List, Optional, Type, TypeVar
from uuid import UUID
@ -164,7 +165,7 @@ def register_remote_push_device(
server=server, token=token, kind=token_kind, user_id=user_id
).delete()
else:
# One of these is None, so these will kwargs will leads to a proper registration
# One of these is None, so these kwargs will lead to a proper registration
# of either user_id or user_uuid type
kwargs = {"user_id": user_id, "user_uuid": user_uuid}
try:
@ -219,13 +220,73 @@ def unregister_all_remote_push_devices(
return json_success(request)
def delete_duplicate_registrations(
registrations: List[RemotePushDeviceToken], server_id: int, user_id: int, user_uuid: str
) -> List[RemotePushDeviceToken]:
"""
When migrating to support registration by UUID, we introduced a bug where duplicate
registrations for the same device+user could be created - one by user_id and one by
user_uuid. Given no good way of detecting these duplicates at database level, we need to
take advantage of the fact that when a remote server sends a push notification request
to us, it sends both user_id and user_uuid of the user.
See https://github.com/zulip/zulip/issues/24969 for reference.
This function, knowing the user_id and user_uuid of the user, can detect duplicates
and delete the legacy user_id registration if appropriate.
Return the list of registrations with the user_id-based duplicates removed.
"""
# All registrations passed here should be of the same kind (apple vs android).
assert len({registration.kind for registration in registrations}) == 1
kind = registrations[0].kind
tokens_counter = Counter(device.token for device in registrations)
tokens_to_deduplicate = []
for key in tokens_counter:
if tokens_counter[key] <= 1:
continue
if tokens_counter[key] > 2:
raise AssertionError(
f"More than two registrations for token {key} for user id:{user_id} uuid:{user_uuid}, shouldn't be possible"
)
assert tokens_counter[key] == 2
tokens_to_deduplicate.append(key)
if not tokens_to_deduplicate:
return registrations
logger.info(
"Deduplicating push registrations for server id:%s user id:%s uuid:%s and tokens:%s",
server_id,
user_id,
user_uuid,
sorted(tokens_to_deduplicate),
)
RemotePushDeviceToken.objects.filter(
token__in=tokens_to_deduplicate, kind=kind, server_id=server_id, user_id=user_id
).delete()
deduplicated_registrations_to_return = []
for registration in registrations:
if registration.token in tokens_to_deduplicate and registration.user_id is not None:
# user_id registrations are the ones we deleted
continue
deduplicated_registrations_to_return.append(registration)
return deduplicated_registrations_to_return
@has_request_variables
def remote_server_notify_push(
request: HttpRequest,
server: RemoteZulipServer,
payload: Dict[str, Any] = REQ(argument_type="body"),
) -> HttpResponse:
user_identity = UserPushIdentityCompat(payload.get("user_id"), payload.get("user_uuid"))
user_id = payload.get("user_id")
user_uuid = payload.get("user_uuid")
user_identity = UserPushIdentityCompat(user_id, user_uuid)
gcm_payload = payload["gcm_payload"]
apns_payload = payload["apns_payload"]
@ -238,6 +299,10 @@ def remote_server_notify_push(
server=server,
)
)
if android_devices and user_id is not None and user_uuid is not None:
android_devices = delete_duplicate_registrations(
android_devices, server.id, user_id, user_uuid
)
apple_devices = list(
RemotePushDeviceToken.objects.filter(
@ -246,6 +311,8 @@ def remote_server_notify_push(
server=server,
)
)
if apple_devices and user_id is not None and user_uuid is not None:
apple_devices = delete_duplicate_registrations(apple_devices, server.id, user_id, user_uuid)
logger.info(
"Sending mobile push notifications for remote user %s:%s: %s via FCM devices, %s via APNs devices",