Skip to content

Commit

Permalink
address mobile device push notification delivery issue when user had …
Browse files Browse the repository at this point in the history
…> 1 registered device (#2421)

# What this PR does

Address issue where if the user had multiple registered devices w/ FCM,
doing django queries like `.first()` could potentially pick the wrong
device. Do this in two ways:
1. set the `DELETE_INACTIVE_DEVICES` `fcm_django` setting to `True`.
According to the
[docs](https://github.com/xtrinch/fcm-django/blob/20e275618b47cea4e1b4dd3b1a8c640a164a7ec5/README.rst?plain=1#L127-L130),
this works as follows:

> devices to which notifications cannot be sent, are deleted upon
receiving error response from FCM
2. Customizing the `FCMDevice` model provided by `fcm_django`. Add a new
method, `get_active_device_for_user`, so that we can centralize the
logic for this rather than duplicating
`FCMDevice.objects.filter(user=user).first()`

## Which issue(s) this PR fixes

https://raintank-corp.slack.com/archives/C0229FD3CE9/p1688461915752119

## Checklist

- [x] Unit, integration, and e2e (if applicable) tests updated
- [ ] Documentation added (or `pr:no public docs` PR label added if not
required) (N/A)
- [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not
required)
  • Loading branch information
joeyorlando authored Jul 5, 2023
1 parent 01b1be8 commit 425ffbb
Show file tree
Hide file tree
Showing 12 changed files with 80 additions and 33 deletions.
9 changes: 8 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## Unreleased

## v1.3.6 (2023-07-05)

### Fixed

- Address issue where having multiple registered mobile apps for a user could lead to issues in delivering push
notifications by @joeyorlando ([#2421](https://github.com/grafana/oncall/pull/2421))

## v1.3.5 (2023-07-05)

### Fixed
Expand All @@ -23,7 +30,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Changed

- UI drawer updates for webhooks2 ([#2419](https://github.com/grafana/oncall/pull/2419))
- Removed url from sms notification, changed format ([2317](https://github.com/grafana/oncall/pull/2317))
- Removed url from sms notification, changed format ([2317](https://github.com/grafana/oncall/pull/2317))

## v1.3.3 (2023-06-29)

Expand Down
7 changes: 4 additions & 3 deletions engine/apps/mobile_app/backend.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import json

from django.conf import settings
from fcm_django.models import FCMDevice

from apps.base.messaging import BaseMessagingBackend
from apps.mobile_app.tasks import notify_user_async
Expand Down Expand Up @@ -29,13 +28,15 @@ def generate_user_verification_code(self, user):
)

def unlink_user(self, user):
from apps.mobile_app.models import MobileAppAuthToken
from apps.mobile_app.models import FCMDevice, MobileAppAuthToken

token = MobileAppAuthToken.objects.get(user=user)
token.delete()

# delete push notification related info for user
FCMDevice.objects.filter(user=user).delete()
user_active_device = FCMDevice.get_active_device_for_user(user)
if user_active_device is not None:
user_active_device.delete()

def serialize_user(self, user):
from apps.mobile_app.models import MobileAppAuthToken
Expand Down
11 changes: 8 additions & 3 deletions engine/apps/mobile_app/demo_push.py
Original file line number Diff line number Diff line change
@@ -1,25 +1,30 @@
import json
import random
import string
import typing

from fcm_django.models import FCMDevice
from firebase_admin.messaging import APNSPayload, Aps, ApsAlert, CriticalSound, Message

from apps.mobile_app.exceptions import DeviceNotSet
from apps.mobile_app.tasks import FCMMessageData, MessageType, _construct_fcm_message, _send_push_notification, logger
from apps.user_management.models import User

if typing.TYPE_CHECKING:
from apps.mobile_app.models import FCMDevice


def send_test_push(user, critical=False):
device_to_notify = FCMDevice.objects.filter(user=user).first()
from apps.mobile_app.models import FCMDevice

device_to_notify = FCMDevice.get_active_device_for_user(user)
if device_to_notify is None:
logger.info(f"send_test_push: fcm_device not found user_id={user.id}")
raise DeviceNotSet
message = _get_test_escalation_fcm_message(user, device_to_notify, critical)
_send_push_notification(device_to_notify, message)


def _get_test_escalation_fcm_message(user: User, device_to_notify: FCMDevice, critical: bool) -> Message:
def _get_test_escalation_fcm_message(user: User, device_to_notify: "FCMDevice", critical: bool) -> Message:
# TODO: this method is copied from _get_alert_group_escalation_fcm_message
# to have same notification/sound/overrideDND logic. Ideally this logic should be abstracted, not repeated.
from apps.mobile_app.models import MobileAppUserSettings
Expand Down
2 changes: 1 addition & 1 deletion engine/apps/mobile_app/fcm_relay.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

from celery.utils.log import get_task_logger
from django.conf import settings
from fcm_django.models import FCMDevice
from firebase_admin.exceptions import FirebaseError
from firebase_admin.messaging import AndroidConfig, APNSConfig, APNSPayload, Aps, ApsAlert, CriticalSound, Message
from rest_framework import status
Expand All @@ -12,6 +11,7 @@
from rest_framework.views import APIView

from apps.auth_token.auth import ApiTokenAuthentication
from apps.mobile_app.models import FCMDevice
from common.custom_celery_tasks import shared_dedicated_queue_retry_task

task_logger = get_task_logger(__name__)
Expand Down
38 changes: 31 additions & 7 deletions engine/apps/mobile_app/models.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,18 @@
from typing import Tuple
from __future__ import annotations # https://stackoverflow.com/a/33533514

import typing

from django.conf import settings
from django.core import validators
from django.db import models
from django.utils import timezone
from fcm_django.models import FCMDevice as BaseFCMDevice

from apps.auth_token import constants, crypto
from apps.auth_token.models import BaseAuthToken
from apps.user_management.models import Organization, User

if typing.TYPE_CHECKING:
from apps.user_management.models import Organization, User

MOBILE_APP_AUTH_VERIFICATION_TOKEN_TIMEOUT_SECONDS = 60 * (5 if settings.DEBUG else 1)

Expand All @@ -16,6 +21,19 @@ def get_expire_date():
return timezone.now() + timezone.timedelta(seconds=MOBILE_APP_AUTH_VERIFICATION_TOKEN_TIMEOUT_SECONDS)


class ActiveFCMDeviceQuerySet(models.QuerySet):
def filter(self, *args, **kwargs):
return super().filter(*args, **kwargs, active=True)


class FCMDevice(BaseFCMDevice):
active_objects = ActiveFCMDeviceQuerySet.as_manager()

@classmethod
def get_active_device_for_user(cls, user: "User") -> FCMDevice | None:
return cls.active_objects.filter(user=user).first()


class MobileAppVerificationTokenQueryset(models.QuerySet):
def filter(self, *args, **kwargs):
now = timezone.now()
Expand All @@ -38,7 +56,9 @@ class MobileAppVerificationToken(BaseAuthToken):
expire_date = models.DateTimeField(default=get_expire_date)

@classmethod
def create_auth_token(cls, user: User, organization: Organization) -> Tuple["MobileAppVerificationToken", str]:
def create_auth_token(
cls, user: "User", organization: "Organization"
) -> typing.Tuple["MobileAppVerificationToken", str]:
token_string = crypto.generate_short_token_string()
digest = crypto.hash_token_string(token_string)

Expand All @@ -54,13 +74,17 @@ def create_auth_token(cls, user: User, organization: Organization) -> Tuple["Mob
class MobileAppAuthToken(BaseAuthToken):
objects: models.Manager["MobileAppAuthToken"]

user = models.OneToOneField(to=User, null=False, blank=False, on_delete=models.CASCADE)
user = models.OneToOneField(to="user_management.User", null=False, blank=False, on_delete=models.CASCADE)
organization = models.ForeignKey(
to=Organization, null=False, blank=False, related_name="mobile_app_auth_tokens", on_delete=models.CASCADE
to="user_management.Organization",
null=False,
blank=False,
related_name="mobile_app_auth_tokens",
on_delete=models.CASCADE,
)

@classmethod
def create_auth_token(cls, user: User, organization: Organization) -> Tuple["MobileAppAuthToken", str]:
def create_auth_token(cls, user: "User", organization: "Organization") -> typing.Tuple["MobileAppAuthToken", str]:
token_string = crypto.generate_token_string()
digest = crypto.hash_token_string(token_string)

Expand All @@ -82,7 +106,7 @@ class VolumeType(models.TextChoices):
CONSTANT = "constant"
INTENSIFYING = "intensifying"

user = models.OneToOneField(to=User, null=False, on_delete=models.CASCADE)
user = models.OneToOneField(to="user_management.User", null=False, on_delete=models.CASCADE)

# Push notification settings for default notifications
default_notification_sound_name = models.CharField(max_length=100, default="default_sound")
Expand Down
20 changes: 10 additions & 10 deletions engine/apps/mobile_app/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
from django.conf import settings
from django.core.cache import cache
from django.utils import timezone
from fcm_django.models import FCMDevice
from firebase_admin.exceptions import FirebaseError
from firebase_admin.messaging import AndroidConfig, APNSConfig, APNSPayload, Aps, ApsAlert, CriticalSound, Message
from requests import HTTPError
Expand All @@ -29,7 +28,7 @@
from common.timezones import is_valid_timezone

if typing.TYPE_CHECKING:
from apps.mobile_app.models import MobileAppUserSettings
from apps.mobile_app.models import FCMDevice, MobileAppUserSettings


MAX_RETRIES = 1 if settings.DEBUG else 10
Expand Down Expand Up @@ -64,7 +63,7 @@ def send_push_notification_to_fcm_relay(message: Message) -> requests.Response:


def _send_push_notification(
device_to_notify: FCMDevice, message: Message, error_cb: typing.Optional[typing.Callable[..., None]] = None
device_to_notify: "FCMDevice", message: Message, error_cb: typing.Optional[typing.Callable[..., None]] = None
) -> None:
logger.debug(f"Sending push notification to device type {device_to_notify.type} with message: {message}")

Expand Down Expand Up @@ -105,7 +104,7 @@ def _error_cb():

def _construct_fcm_message(
message_type: MessageType,
device_to_notify: FCMDevice,
device_to_notify: "FCMDevice",
thread_id: str,
data: FCMMessageData,
apns_payload: typing.Optional[APNSPayload] = None,
Expand Down Expand Up @@ -151,7 +150,7 @@ def _construct_fcm_message(


def _get_alert_group_escalation_fcm_message(
alert_group: AlertGroup, user: User, device_to_notify: FCMDevice, critical: bool
alert_group: AlertGroup, user: User, device_to_notify: "FCMDevice", critical: bool
) -> Message:
# avoid circular import
from apps.mobile_app.models import MobileAppUserSettings
Expand Down Expand Up @@ -265,7 +264,7 @@ def _format_datetime(dt: datetime.datetime) -> str:
def _get_youre_going_oncall_fcm_message(
user: User,
schedule: OnCallSchedule,
device_to_notify: FCMDevice,
device_to_notify: "FCMDevice",
seconds_until_going_oncall: int,
schedule_event: ScheduleEvent,
) -> Message:
Expand Down Expand Up @@ -314,6 +313,7 @@ def _get_youre_going_oncall_fcm_message(
def notify_user_async(user_pk, alert_group_pk, notification_policy_pk, critical):
# avoid circular import
from apps.base.models import UserNotificationPolicy, UserNotificationPolicyLogRecord
from apps.mobile_app.models import FCMDevice

try:
user = User.objects.get(pk=user_pk)
Expand Down Expand Up @@ -347,7 +347,7 @@ def _create_error_log_record():
notification_channel=notification_policy.notify_by,
)

device_to_notify = FCMDevice.objects.filter(user=user).first()
device_to_notify = FCMDevice.get_active_device_for_user(user)

# create an error log in case user has no devices set up
if not device_to_notify:
Expand Down Expand Up @@ -431,11 +431,11 @@ def _generate_going_oncall_push_notification_cache_key(user_pk: str, schedule_ev
@shared_dedicated_queue_retry_task(autoretry_for=(Exception,), retry_backoff=True, max_retries=MAX_RETRIES)
def conditionally_send_going_oncall_push_notifications_for_schedule(schedule_pk) -> None:
# avoid circular import
from apps.mobile_app.models import MobileAppUserSettings
from apps.mobile_app.models import FCMDevice, MobileAppUserSettings

PUSH_NOTIFICATION_TRACKING_CACHE_KEY_TTL = 60 * 60 # 60 minutes
user_cache: typing.Dict[str, User] = {}
device_cache: typing.Dict[str, FCMDevice] = {}
device_cache: typing.Dict[str, "FCMDevice"] = {}

logger.info(f"Start calculate_going_oncall_push_notifications_for_schedule for schedule {schedule_pk}")

Expand Down Expand Up @@ -473,7 +473,7 @@ def conditionally_send_going_oncall_push_notifications_for_schedule(schedule_pk)

device_to_notify = device_cache.get(user_pk, None)
if device_to_notify is None:
device_to_notify = FCMDevice.objects.filter(user=user).first()
device_to_notify = FCMDevice.get_active_device_for_user(user)

if not device_to_notify:
continue
Expand Down
3 changes: 1 addition & 2 deletions engine/apps/mobile_app/tests/test_demo_push.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import pytest
from fcm_django.models import FCMDevice

from apps.mobile_app.demo_push import _get_test_escalation_fcm_message, get_test_push_title
from apps.mobile_app.models import MobileAppUserSettings
from apps.mobile_app.models import FCMDevice, MobileAppUserSettings


@pytest.mark.django_db
Expand Down
13 changes: 13 additions & 0 deletions engine/apps/mobile_app/tests/test_fcm_device_model.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import pytest

from apps.mobile_app.models import FCMDevice


@pytest.mark.django_db
def test_get_active_device_for_user_works(make_organization_and_user):
_, user = make_organization_and_user()
FCMDevice.objects.create(user=user, registration_id="inactive_device_id", active=False)
active_device = FCMDevice.objects.create(user=user, registration_id="active_device_id", active=True)

assert FCMDevice.objects.filter(user=user).count() == 2
assert FCMDevice.get_active_device_for_user(user) == active_device
2 changes: 1 addition & 1 deletion engine/apps/mobile_app/tests/test_fcm_relay.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@

import pytest
from django.urls import reverse
from fcm_django.models import FCMDevice
from firebase_admin.exceptions import FirebaseError
from rest_framework import status
from rest_framework.test import APIClient

from apps.mobile_app.fcm_relay import FCMRelayThrottler, _get_message_from_request_data, fcm_relay_async
from apps.mobile_app.models import FCMDevice
from apps.mobile_app.tasks import _get_alert_group_escalation_fcm_message


Expand Down
3 changes: 1 addition & 2 deletions engine/apps/mobile_app/tests/test_notify_user.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
from unittest.mock import patch

import pytest
from fcm_django.models import FCMDevice
from firebase_admin.exceptions import FirebaseError

from apps.base.models import UserNotificationPolicy, UserNotificationPolicyLogRecord
from apps.mobile_app.models import MobileAppUserSettings
from apps.mobile_app.models import FCMDevice, MobileAppUserSettings
from apps.mobile_app.tasks import _get_alert_group_escalation_fcm_message, notify_user_async
from apps.oss_installation.models import CloudConnector

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,9 @@
import pytest
from django.core.cache import cache
from django.utils import timezone
from fcm_django.models import FCMDevice

from apps.mobile_app import tasks
from apps.mobile_app.models import MobileAppUserSettings
from apps.mobile_app.models import FCMDevice, MobileAppUserSettings
from apps.schedules.models import OnCallScheduleCalendar, OnCallScheduleICal, OnCallScheduleWeb
from apps.schedules.models.on_call_schedule import ScheduleEvent

Expand Down
2 changes: 1 addition & 1 deletion engine/settings/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -619,7 +619,7 @@ class BrokerTypes:
"DEFAULT_FIREBASE_APP": initialize_app(credential=credential, options={"projectId": FCM_PROJECT_ID}),
"APP_VERBOSE_NAME": "OnCall",
"ONE_DEVICE_PER_USER": True,
"DELETE_INACTIVE_DEVICES": False,
"DELETE_INACTIVE_DEVICES": True,
"UPDATE_ON_DUPLICATE_REG_ID": True,
"USER_MODEL": "user_management.User",
}
Expand Down

0 comments on commit 425ffbb

Please sign in to comment.