notifications: Fix push notifications with multiple realms.

Previously, Zulip did not correctly handle the case of a mobile device
being registered with a push device token being registered for
multiple accounts on the same server (which is a common case on
zulipchat.com).  This was because our database `unique` and
`unique_together` indexes incorrectly enforced the token being unique
on a given server, rather than unique for a given user_id.

We fix this gap, and at the same time remove unnecessary (and
incorrectly racey) logic deleting and recreating the tokens in the
appropriate tables.

There's still an open mobile app bug causing repeated re-registrations
in a loop, but this should fix the fact that the relevant mobile bug
causes the server to 500.

Follow-up work that may be of value includes:
* Removing `ios_app_id`, which may not have much purpose.
* Renaming `last_updated` to `data_created`, since that's what it is now.

But none of those are critical to solving the actual bug here.

Fixes #8841.

zerver/lib/push_notifications.py

@@ -15,6 +15,7 @@ from typing import Any, Dict, List, Optional, SupportsInt, Tuple, Type, Union
 from apns2.client import APNsClient
 from apns2.payload import Payload as APNsPayload
 from django.conf import settings
+from django.db import IntegrityError, transaction
 from django.utils.timezone import now as timezone_now
 from django.utils.translation import ugettext as _
 from gcm import GCM
@@ -345,8 +346,7 @@ def add_push_device_token(user_profile: UserProfile,
                           token_str: bytes,
                           kind: int,
                           ios_app_id: Optional[str]=None) -> None:
-
-    logging.info("New push device: %d %r %d %r",
+    logging.info("Registering push device: %d %r %d %r",
                  user_profile.id, token_str, kind, ios_app_id)
 
     # If we're sending things to the push notification bouncer
@@ -367,22 +367,17 @@ def add_push_device_token(user_profile: UserProfile,
         send_to_push_bouncer('POST', 'register', post_data)
         return
 
-    # If another user was previously logged in on the same device and didn't
-    # properly log out, the token will still be registered to the wrong account
-    PushDeviceToken.objects.filter(token=token_str).exclude(user=user_profile).delete()
-
-    # Overwrite with the latest value
-    token, created = PushDeviceToken.objects.get_or_create(user=user_profile,
-                                                           token=token_str,
-                                                           defaults=dict(
-                                                               kind=kind,
-                                                               ios_app_id=ios_app_id))
-    if not created:
-        logging.info("Existing push device updated.")
-        token.last_updated = timezone_now()
-        token.save(update_fields=['last_updated'])
-    else:
-        logging.info("New push device created.")
+    try:
+        with transaction.atomic():
+            PushDeviceToken.objects.create(
+                user_id=user_profile.id,
+                kind=kind,
+                token=token_str,
+                ios_app_id=ios_app_id,
+                # last_updated is to be renamed to date_created.
+                last_updated=timezone_now())
+    except IntegrityError:
+        pass
 
 def remove_push_device_token(user_profile: UserProfile, token_str: bytes, kind: int) -> None:
 

zerver/migrations/0190_cleanup_pushdevicetoken.py

@@ -0,0 +1,24 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.14 on 2018-10-10 22:52
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('zerver', '0189_userprofile_add_some_emojisets'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='pushdevicetoken',
+            name='token',
+            field=models.CharField(db_index=True, max_length=4096),
+        ),
+        migrations.AlterUniqueTogether(
+            name='pushdevicetoken',
+            unique_together=set([('user', 'kind', 'token')]),
+        ),
+    ]

zerver/models.py

@@ -1039,6 +1039,9 @@ class AbstractPushDeviceToken(models.Model):
     # sent to us from each device:
     #   - APNS token if kind == APNS
     #   - GCM registration id if kind == GCM
+
+    # TODO: last_updated should be renamed date_created, since it is
+    # no longer maintained as a last_updated value.
     last_updated = models.DateTimeField(auto_now=True)  # type: datetime.datetime
 
     # [optional] Contains the app id of the device if it is an iOS device
@@ -1050,7 +1053,10 @@ class AbstractPushDeviceToken(models.Model):
 class PushDeviceToken(AbstractPushDeviceToken):
     # The user who's device this is
     user = models.ForeignKey(UserProfile, db_index=True, on_delete=CASCADE)  # type: UserProfile
-    token = models.CharField(max_length=4096, unique=True)  # type: bytes
+    token = models.CharField(max_length=4096, db_index=True)  # type: bytes
+
+    class Meta:
+        unique_together = ("user", "kind", "token")
 
 def generate_email_token_for_stream() -> str:
     return generate_random_token(32)

zilencer/migrations/0014_cleanup_pushdevicetoken.py

@@ -0,0 +1,19 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.14 on 2018-10-10 22:52
+from __future__ import unicode_literals
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('zilencer', '0013_remove_customer_billing_user'),
+    ]
+
+    operations = [
+        migrations.AlterUniqueTogether(
+            name='remotepushdevicetoken',
+            unique_together=set([('server', 'user_id', 'kind', 'token')]),
+        ),
+    ]

zilencer/models.py

@@ -31,7 +31,7 @@ class RemotePushDeviceToken(AbstractPushDeviceToken):
     token = models.CharField(max_length=4096, db_index=True)  # type: bytes
 
     class Meta:
-        unique_together = ("server", "token")
+        unique_together = ("server", "user_id", "kind", "token")
 
     def __str__(self) -> str:
         return "<RemotePushDeviceToken %s %s>" % (self.server, self.user_id)

zilencer/views.py

@@ -3,6 +3,7 @@ import logging
 
 from django.core.exceptions import ValidationError
 from django.core.validators import validate_email, URLValidator
+from django.db import IntegrityError, transaction
 from django.http import HttpRequest, HttpResponse
 from django.utils import timezone
 from django.utils.translation import ugettext as _, ugettext as err_
@@ -82,21 +83,18 @@ def register_remote_push_device(request: HttpRequest, entity: Union[UserProfile,
     validate_bouncer_token_request(entity, token, token_kind)
     server = cast(RemoteZulipServer, entity)
 
-    # If a user logged out on a device and failed to unregister,
-    # we should delete any other user associations for this token
-    # & RemoteServer pair
-    RemotePushDeviceToken.objects.filter(
-        token=token, kind=token_kind, server=server).exclude(user_id=user_id).delete()
-
-    # Save or update
-    remote_token, created = RemotePushDeviceToken.objects.update_or_create(
-        user_id=user_id,
-        server=server,
-        kind=token_kind,
-        token=token,
-        defaults=dict(
-            ios_app_id=ios_app_id,
-            last_updated=timezone.now()))
+    try:
+        with transaction.atomic():
+            RemotePushDeviceToken.objects.create(
+                user_id=user_id,
+                server=server,
+                kind=token_kind,
+                token=token,
+                ios_app_id=ios_app_id,
+                # last_updated is to be renamed to date_created.
+                last_updated=timezone.now())
+    except IntegrityError:
+        pass
 
     return json_success()