From d800ac33a0322b0d3ef327bcebf318a27448bbee Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Wed, 23 Feb 2022 20:27:39 +0100 Subject: [PATCH] push_notifications: Send user_uuid to the push bouncer. Fixes #18017. In previous commits, the change to the bouncer API was introduced to support this and then a series of migrations added .uuid to UserProfiles. Now the code for self-hosted servers that makes requests to the bouncer is changed to make use of it. --- zerver/lib/export.py | 11 ++++- zerver/lib/push_notifications.py | 12 +++++- zerver/tests/test_push_notifications.py | 56 +++++++++++++++++-------- 3 files changed, 59 insertions(+), 20 deletions(-) diff --git a/zerver/lib/export.py b/zerver/lib/export.py index ef5c088ce7..d0ca091659 100644 --- a/zerver/lib/export.py +++ b/zerver/lib/export.py @@ -976,12 +976,19 @@ def add_user_profile_child_configs(user_profile_config: Config) -> None: ) +# We exclude these fields for the following reasons: +# * api_key is a secret. +# * password is a secret. +# * uuid is unlikely to be useful if the domain changes. +EXCLUDED_USER_PROFILE_FIELDS = ["api_key", "password", "uuid"] + + def custom_fetch_user_profile(response: TableData, context: Context) -> None: realm = context["realm"] exportable_user_ids = context["exportable_user_ids"] query = UserProfile.objects.filter(realm_id=realm.id) - exclude = ["password", "api_key"] + exclude = EXCLUDED_USER_PROFILE_FIELDS rows = make_raw(list(query), exclude=exclude) normal_rows: List[Record] = [] @@ -1979,7 +1986,7 @@ def get_single_user_config() -> Config: user_profile_config = Config( table="zerver_userprofile", is_seeded=True, - exclude=["password", "api_key"], + exclude=EXCLUDED_USER_PROFILE_FIELDS, ) # zerver_subscription diff --git a/zerver/lib/push_notifications.py b/zerver/lib/push_notifications.py index b178dda0d3..aba763d07e 100644 --- a/zerver/lib/push_notifications.py +++ b/zerver/lib/push_notifications.py @@ -487,6 +487,9 @@ def send_notifications_to_bouncer( gcm_options: Dict[str, Any], ) -> Tuple[int, int]: post_data = { + "user_uuid": str(get_user_profile_by_id(user_profile_id).uuid), + # user_uuid is the intended future format, but we also need to send user_id + # to avoid breaking old mobile registrations, which were made with user_id. "user_id": user_profile_id, "apns_payload": apns_payload, "gcm_payload": gcm_payload, @@ -539,7 +542,7 @@ def add_push_device_token( if uses_notification_bouncer(): post_data = { "server_uuid": settings.ZULIP_ORG_ID, - "user_id": user_profile.id, + "user_uuid": str(user_profile.uuid), "token": token_str, "token_kind": kind, } @@ -573,6 +576,9 @@ def remove_push_device_token(user_profile: UserProfile, token_str: str, kind: in # TODO: Make this a remove item post_data = { "server_uuid": settings.ZULIP_ORG_ID, + # We don't know here if the token was registered with uuid + # or using the legacy id format, so we need to send both. + "user_uuid": str(user_profile.uuid), "user_id": user_profile.id, "token": token_str, "token_kind": kind, @@ -584,8 +590,12 @@ def remove_push_device_token(user_profile: UserProfile, token_str: str, kind: in def clear_push_device_tokens(user_profile_id: int) -> None: # Deletes all of a user's PushDeviceTokens. if uses_notification_bouncer(): + user_uuid = str(get_user_profile_by_id(user_profile_id).uuid) post_data = { "server_uuid": settings.ZULIP_ORG_ID, + # We want to clear all registered token, and they may have + # been registered with either uuid or id. + "user_uuid": user_uuid, "user_id": user_profile_id, } send_to_push_bouncer("POST", "push/unregister/all", post_data) diff --git a/zerver/tests/test_push_notifications.py b/zerver/tests/test_push_notifications.py index ef4ee4dd37..bbd28c7cbd 100644 --- a/zerver/tests/test_push_notifications.py +++ b/zerver/tests/test_push_notifications.py @@ -77,6 +77,7 @@ from zerver.models import ( get_display_recipient, get_realm, get_stream, + get_user_profile_by_id, ) from zilencer.models import RemoteZulipServerAuditLog @@ -508,13 +509,15 @@ class PushBouncerNotificationTest(BouncerTestCase): self.assert_json_success(result) tokens = list( - RemotePushDeviceToken.objects.filter(user_id=user.id, token=token, server=server) + RemotePushDeviceToken.objects.filter( + user_uuid=user.uuid, token=token, server=server + ) ) self.assert_length(tokens, 1) self.assertEqual(tokens[0].token, token) # User should have tokens for both devices now. - tokens = list(RemotePushDeviceToken.objects.filter(user_id=user.id, server=server)) + tokens = list(RemotePushDeviceToken.objects.filter(user_uuid=user.uuid, server=server)) self.assert_length(tokens, 2) # Remove tokens @@ -524,7 +527,9 @@ class PushBouncerNotificationTest(BouncerTestCase): ) self.assert_json_success(result) tokens = list( - RemotePushDeviceToken.objects.filter(user_id=user.id, token=token, server=server) + RemotePushDeviceToken.objects.filter( + user_uuid=user.uuid, token=token, server=server + ) ) self.assert_length(tokens, 0) @@ -532,7 +537,7 @@ class PushBouncerNotificationTest(BouncerTestCase): for endpoint, token, kind in endpoints: result = self.client_post(endpoint, {"token": token}, subdomain="zulip") self.assert_json_success(result) - tokens = list(RemotePushDeviceToken.objects.filter(user_id=user.id, server=server)) + tokens = list(RemotePushDeviceToken.objects.filter(user_uuid=user.uuid, server=server)) self.assert_length(tokens, 2) # Now we want to remove them using the bouncer after an API key change. @@ -545,12 +550,12 @@ class PushBouncerNotificationTest(BouncerTestCase): mock_retry.assert_called() # We didn't manage to communicate with the bouncer, to the tokens are still there: - tokens = list(RemotePushDeviceToken.objects.filter(user_id=user.id, server=server)) + tokens = list(RemotePushDeviceToken.objects.filter(user_uuid=user.uuid, server=server)) self.assert_length(tokens, 2) # Now we successfully remove them: do_regenerate_api_key(user, user) - tokens = list(RemotePushDeviceToken.objects.filter(user_id=user.id, server=server)) + tokens = list(RemotePushDeviceToken.objects.filter(user_uuid=user.uuid, server=server)) self.assert_length(tokens, 0) @@ -897,14 +902,24 @@ class PushNotificationTest(BouncerTestCase): ios_app_id=settings.ZULIP_IOS_APP_ID, ) - self.remote_tokens = ["cccc"] - for token in self.remote_tokens: + self.remote_tokens = [("cccc", "ffff")] + for id_token, uuid_token in self.remote_tokens: + # We want to set up both types of RemotePushDeviceToken here: + # the legacy one with user_id and the new with user_uuid. + # This allows tests to work with either, without needing to + # do their own setup. RemotePushDeviceToken.objects.create( kind=RemotePushDeviceToken.APNS, - token=hex_to_b64(token), + token=hex_to_b64(id_token), user_id=self.user_profile.id, server=RemoteZulipServer.objects.get(uuid=self.server_uuid), ) + RemotePushDeviceToken.objects.create( + kind=RemotePushDeviceToken.APNS, + token=hex_to_b64(uuid_token), + user_uuid=self.user_profile.uuid, + server=RemoteZulipServer.objects.get(uuid=self.server_uuid), + ) def setup_gcm_tokens(self) -> None: self.gcm_tokens = ["1111", "2222"] @@ -916,14 +931,20 @@ class PushNotificationTest(BouncerTestCase): ios_app_id=None, ) - self.remote_gcm_tokens = ["dddd"] - for token in self.remote_gcm_tokens: + self.remote_gcm_tokens = [("dddd", "eeee")] + for id_token, uuid_token in self.remote_gcm_tokens: RemotePushDeviceToken.objects.create( kind=RemotePushDeviceToken.GCM, - token=hex_to_b64(token), + token=hex_to_b64(id_token), user_id=self.user_profile.id, server=RemoteZulipServer.objects.get(uuid=self.server_uuid), ) + RemotePushDeviceToken.objects.create( + kind=RemotePushDeviceToken.GCM, + token=hex_to_b64(uuid_token), + user_uuid=self.user_profile.uuid, + server=RemoteZulipServer.objects.get(uuid=self.server_uuid), + ) class HandlePushNotificationTest(PushNotificationTest): @@ -985,14 +1006,14 @@ class HandlePushNotificationTest(PushNotificationTest): views_logger.output, [ "INFO:zilencer.views:" - f"Sending mobile push notifications for remote user 6cde5f7a-1f7e-4978-9716-49f69ebfc9fe:id:{self.user_profile.id}: " + f"Sending mobile push notifications for remote user 6cde5f7a-1f7e-4978-9716-49f69ebfc9fe:uuid:{str(self.user_profile.uuid)}: " f"{len(gcm_devices)} via FCM devices, {len(apns_devices)} via APNs devices" ], ) for _, _, token in apns_devices: self.assertIn( "INFO:zerver.lib.push_notifications:" - f"APNs: Success sending for user id:{self.user_profile.id} to device {token}", + f"APNs: Success sending for user uuid:{str(self.user_profile.uuid)} to device {token}", pn_logger.output, ) for _, _, token in gcm_devices: @@ -1046,7 +1067,7 @@ class HandlePushNotificationTest(PushNotificationTest): views_logger.output, [ "INFO:zilencer.views:" - f"Sending mobile push notifications for remote user 6cde5f7a-1f7e-4978-9716-49f69ebfc9fe:id:{self.user_profile.id}: " + f"Sending mobile push notifications for remote user 6cde5f7a-1f7e-4978-9716-49f69ebfc9fe:uuid:{str(self.user_profile.uuid)}: " f"{len(gcm_devices)} via FCM devices, {len(apns_devices)} via APNs devices" ], ) @@ -2094,6 +2115,7 @@ class TestSendNotificationsToBouncer(ZulipTestCase): 1, {"apns": True}, {"gcm": True}, {} ) post_data = { + "user_uuid": get_user_profile_by_id(1).uuid, "user_id": 1, "apns_payload": {"apns": True}, "gcm_payload": {"gcm": True}, @@ -2248,7 +2270,7 @@ class TestPushApi(BouncerTestCase): self.assertEqual(tokens[0].token, token) remote_tokens = list( - RemotePushDeviceToken.objects.filter(user_id=user.id, token=token) + RemotePushDeviceToken.objects.filter(user_uuid=user.uuid, token=token) ) self.assert_length(remote_tokens, 1) self.assertEqual(remote_tokens[0].token, token) @@ -2279,7 +2301,7 @@ class TestPushApi(BouncerTestCase): self.assert_json_success(result) tokens = list(PushDeviceToken.objects.filter(user=user, token=token)) remote_tokens = list( - RemotePushDeviceToken.objects.filter(user_id=user.id, token=token) + RemotePushDeviceToken.objects.filter(user_uuid=user.uuid, token=token) ) self.assert_length(tokens, 0) self.assert_length(remote_tokens, 0)