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

Pass removed subscriptions as a property of the pushsubscriptionchange event #193

Closed
ghost opened this issue May 24, 2016 · 20 comments
Closed
Assignees

Comments

@ghost
Copy link

ghost commented May 24, 2016

@collimarco points out in http://blog.pushpad.xyz/2016/05/the-push-api-and-its-wild-unsubscription-mechanism/ that we don't currently pass the removed subscription when we fire pushsubscriptionchange. At that point, self.registration.pushManager.getSubscription() will resolve to null, because the old subscription has been removed.

An app can work around this by storing the subscription endpoint in IDB, then retrieving it when pushsubscriptionchange is fired. Alternatively, we can pass the old subscription along, maybe as a property on the event:

interface PushSubscriptionEvent : ExtendableEvent {
  readonly attribute PushSubscription subscription;
}
@martinthomson
Copy link
Member

martinthomson commented May 24, 2016

That might be confused for the new subscription. Maybe call it oldSubscription. Also note that we might not actually return null in response to getSubscription() if we are pre-emptively removing the subscription (the protocol doesn't support a signal of this sort, but we know that no one is currently using the protocol on this leg just yet).

@collimarco
Copy link

+1 (obviously)

I would like to remark that this issue should apply to the following cases:

  • the user blocks the notifications (a single pushsubscriptionchange is triggered passing the old subscription so that it can be immediately removed from the app server)
  • the push service invalidates the endpoint (a single pushsubscriptionchange is triggered passing the old subscription and a new endpoint can be obtained with getSubscription(), so that the old endpoint can be replaced with the new one on the server preserving attached data)

@ghost
Copy link
Author

ghost commented May 27, 2016

Also note that we might not actually return null in response to getSubscription() if we are pre-emptively removing the subscription

Yeah, I think @johnmellor suggested this in #132 (comment). At that point, a "change" could mean a few things:

  1. The subscription is still active, but going away soon. The app should resubscribe soon, and the old subscription will be deactivated once the new subscription is used.
  2. The user revoked permission, or the subscription exceeded the background message quota (in Firefox). The app can't resubscribe, but can still notify its server to clean up the old subscription. It's OK if the app ignores this event.
  3. The server removed the subscription, the user restored a previously-revoked permission, or reset the quota by visiting the origin again. The app should definitely listen for this event so that it can resubscribe.

I feel like these cases are sufficiently different to where a single event doesn't fit them all. Maybe it's worth thinking about bringing pushsubscriptionlost back for case 3, and adding pushsubscriptiongoing for case 1.

OTOH, explaining pushsubscriptionchange is tricky enough as it is, and adding separate events might make subscription management more complex than it needs to be.

@martinthomson
Copy link
Member

Cases 1 and 3 are basically the same. And 2 is exceptional enough that I think we're OK. The subscribe attempt will fail and reject, but I think that's fine. It still gives the app a chance to notify the server that the old subscription is gone, which is one half of the purpose of the event.

@collimarco
Copy link

@martinthomson I agree, I think that pushsubscriptionchange can handle all those cases. Sometimes you have the expired token and you can get a new one, and sometimes you only have the expired token and you cannot resubscribe.

@mohamedhafez
Copy link

mohamedhafez commented Jun 11, 2016

Huge +1: right now a subscription endpoint in essence acts as the 'ID' of a push target. Developers are frequently going to be saving state on their app server associated with that push target ID, e.g. "this push target ID belongs to this user". If the push target ID is going to change that's all good, but any long-term service is going to have to be able to tell its app server "hey, this old ID should now be known as this new ID"

If the pushsubscriptionchange event came with two attributes, oldSubscription AND newSubscription, I think its meaning would be obvious and could be summed up in one simple phrase: oldSubscription is being replaced with newSubscription. Let your app server know.


EDIT: I'd argue @kitcambridge's case #2 doesn't meet this clear definition, so I suggest it alone get its own pushsubscriptiondisabled event: the oldSubscription is being disabled, but we aren't replacing it and the app server might not want to throw away this push target ID's state, in case the subscription gets re-enabled at some point (which I propose should result in a pushsubscriptionchange event getting fired with the old, disabled subscription and the new, enabled one as attributes)

@mohamedhafez
Copy link

mohamedhafez commented Jun 12, 2016

Actually you know what would be a hell of a lot simpler, and solve all the issues brought up in this thread? If each push target had a UUID that didn't change even if the subscription was refreshed, disabled, re-enabled, etc.

That UUID would be used to track state on the app server instead of the subscription endpoint, so as long as we always have access to the UUID we would have no more need to know about the old subscription values in pushsubscriptionchange.

@martinthomson
Copy link
Member

@mohamedhafez, we try really hard not to create super-cookies, which what you describe might cause.

The problem is that you should not be using the push URI as a primary key. For the same reason that you wouldn't use an email address as a primary key (unless you like pain when it comes time to allow users to change their email address). Instead, make another primary key for the user and store the push URI against that primary key.

@mohamedhafez
Copy link

@martinthomson maybe I wasn't clear, I'm already doing what you are suggesting, but the issue is: when the push URI changes to something else because the subscription refreshes, developers are going to need to know that it still belongs to the same user, that it is logically the same subscription just with updated values.

@mohamedhafez
Copy link

mohamedhafez commented Jun 12, 2016

Also I don't think what I'm suggesting counts as a super-cookie: it would get deleted for good when the service worker got deleted for good. I guess what I'm suggesting or what @kitcambridge suggested is already completely possible with things like IDB, but considering that it is something many developers are going to have to do, it would be nice if it was made easy.

@martinthomson
Copy link
Member

Since the subscription is coming from the same user, why is there anything new needed? Do you have multiple users who share the same browser (and service worker)?

@mohamedhafez
Copy link

mohamedhafez commented Jun 14, 2016

@martinthomson No, but with the current spec, unless I save either the user id or the old subscription info in IDB, how would the ServiceWorker know which user the new subscription is for in pushsubscriptionchange?

Storing that info is doable of course - it would just be nice if the browser would take care of that for us considering its going to be necessary for every developer trying to make user-specific push notifications reliable across refreshes. If giving an ID to subscriptions that stays the same across refreshes is out of the question, then simply including the old subscription info in pushsubscriptionchange as @kitcambridge suggested would work just as well at easily letting us know what subscription we are replacing and which user it belongs to

@beverloo
Copy link
Member

This was addressed by #234 - pushsubscriptionchange events now get newSubscription and oldSubscription properties.

@NekR
Copy link

NekR commented Apr 30, 2017

Hi! So in VAPID subscription.endpoint has unique id. Can that (or should?) be reliably used to identify devices? Example:

  1. Device subscribed -- subscription stored on the server with subscript/endpoint as primary key
  2. Device got new subscription, pushsubscriptionchange fired with old/new subscription
  3. ServiceWorker sends to the server 2 subscription info: old one and the new one to replace with

Can server rely on unique endpoint here to performing matching and replace the old subscription with the new one? Or having custom UUID / cookie session is the way to go here?

Thanks.

@martinthomson
Copy link
Member

The endpoint can change. There is no guarantee that anything in the endpoint will remain constant for the same user. For that matter, there is no guarantee that the same endpoint won't be used for a different user either. If you want to identify users, give them an identifier (cookies or anything like that).

@NekR
Copy link

NekR commented Apr 30, 2017

@martinthomson thanks for info, that makes sense!

@NekR
Copy link

NekR commented May 1, 2017

Just came up with this situation:

  • A browser/device (not user, every visited can subscribe to the push notifications) is subscribed
  • We store UUID/subscription in IDB and send it to the server. On server use UUID is primary key
  • On each page load we compare subscription from getSubscription() and the one in IDB -- if changed -- re-send to the server (or just always re-send)
  • At some point user clears IDB data but doesn't revoke push permission / subscriptions
  • Browser/device visits our website again but IDB is empty -- new UUID is being generated and subscription from getSubscription() is stored and sent to the server
  • At this point the server has 2 UUID entries with the same subscription/endpoint
  • Server sends a push notification and the browser/device receives 2 notifications

So, if an endpoint can be used by multiple/different users: how the server should distinguish between the same browser/device or other one?
How custom UUID (website generated) helps in this situation at all?

@NekR
Copy link

NekR commented May 1, 2017

Also spec explicitly says:

A push endpoint must uniquely identify the push subscription.
-- https://w3c.github.io/push-api/#dfn-push-endpoint

@NekR
Copy link

NekR commented May 1, 2017

And the last: if a device/browser cannot be identified by endpoint or IDB entry + endpoint, then the only solution to uniquely mark a device/browser is to generate new applicationServerKey for each device/browser subscribed. But that, khmm.. really weird.

@NekR
Copy link

NekR commented May 1, 2017

Another though: can p256dh or auth keys be used to identify the subscription? As I understand from the spec, it's being generated uniquely by the browser and isn't tied to a endpoint returned by a push service. So if endpoint changes or being used by multiple users, then p256dh or auth should be able to identify the subscription. Is that true/possible?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants