Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check push subscription validity when checking whether notifications are enabled #226

Closed
jasonpang opened this issue Apr 13, 2017 · 2 comments

Comments

@jasonpang
Copy link
Contributor

jasonpang commented Apr 13, 2017

Summary

We only checked whether a stored push token (stored after registration) existed, not whether the stored push token was still valid.

Description

isPushNotificationsEnabled() checks for five different things:

  • Whether there is a stored OneSignal user ID

    We don't make a request to OneSignal to verify this ID, since that can get expensive.

  • Whether there is a stored push token.

    We don't check for the actual push subscription <---- This is the issue.

  • Whether the notification permission is granted.

    This does a real check to the browser's Notification permission API.

  • Whether the user is manually opted out or not.

    Users can be manually opted out using the setSubscription(false) method available on our SDK.

  • Whether a OneSignal service worker is active

    A user who has a service worker from another push provider wouldn't pass the service worker check.

When checking whether there is a valid push token, we should check the actual token for HTTPS sites (e.g. serviceWorkerRegistration.pushManager.getSubscription()). This isn't possible for HTTP sites, since the service worker registration is inaccessible from within an insecure iFrame (top level origin is HTTP), but we can do this for HTTPS sites.

Application

Developers often re-test by clearing notification permissions to prompt themselves again. This causes them to be actually unsubscribed (in terms of the actual subscription, since resetting the permission sends a GCM unregistration event), but it doesn't remove the stored push token we have in IndexedDb that remembers their previous (now unsubscribed) push token.

When isPushNotificationsEnabled() is called, the user gets false until he grants his notification permission again. But the moment the user grants notification permissions, before the re-subscription actually completes (there are more steps involved) the code checked the stored (existing, but now invalid) push token to determine whether the push token existed, and did not check the actual push token to see whether the push token is still valid. This means the user would immediately get a subscriptionChange event of true, and our SDK would fire a welcome notification because of this. However, the user was actually still resubscribing, so the welcome notification would never arrive.

This should also be related to #181.

jasonpang pushed a commit that referenced this issue Apr 13, 2017
We only checked whether a stored push token (stored after registration)
existed, not whether the stored push token was still valid. Fixes issue #226.
@jasonpang
Copy link
Contributor Author

Fixed by: 4afeb5e

@jasonpang
Copy link
Contributor Author

Note: We resubscribe the user anyways on new tab/window page loads so this wasn't a large-impact issue. It just helps people that resubscribe through partially clearing data.

jasonpang pushed a commit that referenced this issue Apr 20, 2017
We only checked whether a stored push token (stored after registration)
existed, not whether the stored push token was still valid. Fixes issue #226.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant