1191453 - Drop subscriptions for a site when the user revokes push permissions

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Lina Butler [:lina]]

When the user revokes Push permissions for a site, we should drop all subscriptions for that site, and send an `unregister` message to the Push server.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Should we drop subscriptions or simply not deliver them to the application as we do right now?

Comment 2

[Lina Butler [:lina]]

I think we should at least notify the Push server, like we do for quotas. That way, the sender knows the endpoint is no longer valid, and the server won't need to buffer all those messages. In Facebook's case, maybe they could use this to determine your preferred devices for receiving notifications. What do you think?

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Kit Cambridge [:kitcambridge] from comment #2)
> I think we should at least notify the Push server, like we do for quotas.
> That way, the sender knows the endpoint is no longer valid, and the server
> won't need to buffer all those messages. In Facebook's case, maybe they
> could use this to determine your preferred devices for receiving
> notifications. What do you think?

I had no idea we notified the server when quotas expired. Is that a special message or is it treated as an unregister? The problem is if the user re-enables push (especially on non-desktop UIs, where for example, the re-enabling may be in settings app and not in the page info page or something), they won't actually receive pushes till they visit the specific page on the website that will register for push. That sounds unintuitive.

Comment 4

[Lina Butler [:lina]]

>(In reply to Nikhil Marathe [:nsm] (please needinfo?) from comment #3)
> I had no idea we notified the server when quotas expired. Is that a special
> message or is it treated as an unregister?

It's just an unregister.

> That sounds unintuitive.

That makes sense; I hadn't thought of that. We'd need to broadcast a `pushsubscriptionchange` to all service workers with push subscriptions for that host if the user re-enables push. And we'd need to mark those push subscriptions as "expired" instead of deleting them, so we know which workers to notify. That seems like a lot of extra complexity, and I can see why keeping the current behavior is preferable.

Comment 5

[Lina Butler [:lina]]

Martin, Ben...would like to hear your thoughts, too. Are we okay with keeping subscriptions around when the user revokes permissions for a site, and just dropping incoming messages?

Comment 6

[Martin Thomson [:mt:]]

I've done a lot of back-and-forth on what is the right thing to do for this situation.  If we have no quota, clearly we need to stop delivering events, but how much feedback do we need to provide the application?

Notifying the service worker is fairly straightforward; I believe that I proposed we include a timeRemaining attribute on the global.  We need to be careful about different service workers for the same origin, but I don't have any problem in principle with only notifying the one that runs the quota down in this way.  Activating other service workers just to deliver this news doesn't seem like a good use of limited resources.

Notifying the application server is harder.  If we delete the subscription, then the site will have to re-acquire the push subscription when the user visits the site and redo any signaling.  On the other hand, unless we delete the subscription, we don't have any way of letting the application server know.  They will be sending messages into the aether with no awareness that they will never receive a response (I wonder if we would acknowledge them).  If we unregister, the application server receives a 404 and can mark that subscription as bad.  It also ensures that these application servers can't continue to drain battery by waking the device over time.

On this basis, I think that the right plan here is to unsubscribe and then automatically re-trigger a subscription when the site is revisited.  That means that we would surface a `pushsubscriptionchange` event for the service workers on that origin that had push subscriptions.

Comment 7

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1191453 - Drop subscriptions for a site when the user revokes push permissions. r=mt r?MattN

Bug 1191453 - Drop subscriptions for a site when the user revokes push permissions. r?mt,nsm

Comment 8

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7462fae8c60c

Comment 9

[Lina Butler [:lina]]

Here's a first cut.

When the user revokes push permissions for a site, we unregister all subscriptions, and set their quotas to 0. If the block is lifted (set to either "allow" or "ask permission"), we drop the subscriptions, then notify the service worker. There's lots of overlap with how we handle expired subscriptions, so I treated permission revocation identical to "quota expired." Not sure if it makes more sense to use a separate flag, though.

It also looks like we don't receive `perm-changed` notifications when a permission expires, so I added a call to drop expired subscriptions at startup.

Comment 10

[Lina Butler [:lina]]

Linking to a potentially relevant spec discussion: https://github.com/w3c/push-api/issues/116

Comment 11

[Martin Thomson [:mt:]]

Comment on attachment 8644682 [details]
MozReview Request: Bug 1191453 - Drop subscriptions for a site when the user revokes push permissions. r=mt r?MattN

https://reviewboard.mozilla.org/r/15289/#review13741

We should have a discussion about what permissions look like when notifications and push are combined.

::: dom/push/PushRecord.jsm:154
(Diff revision 1)
> +    return this.getLastVisit().then(lastVisit => {
> +      return lastVisit > this.lastPush;
> +    });

```
return this.getLastVisit()
  .then(lastVisit => lastVisit > this.lastPush);
```

::: dom/push/PushService.jsm:275
(Diff revision 1)
>                  .then(_ => {
> -                  // courtesy, but don't establish a connection
> +                  this._unregisterIfConnected(record);
> -                  // just for it
> -                  if (this._ws) {
> -                    debug("Had a connection, so telling the server");
> -                    this._sendRequest("unregister", {channelID: record.channelID})
> -                        .catch(function(e) {
> -                          debug("Unregister errored " + e);
> -                        });
> -                  }
>                  }, err => {

```
.then(_ => this.unregisterIfConnected(record),
      err  => {
```

::: dom/push/PushService.jsm:877
(Diff revision 1)
>          if (record.isExpired()) {
> -          return record.getLastVisit().then(lastVisit => {
> -            if (lastVisit > record.lastPush) {
> +          return record.quotaChanged().then(isChanged => {
> +            if (isChanged) {
>                // If the user revisited the site, drop the expired push
>                // registration and re-register.

Move the comment under the if statement, and maybe tweak it, since the registration is now unconditional.

::: dom/push/PushService.jsm:1116
(Diff revision 1)
>    _registration: function(aPageRecord) {

Lots of code duplication here between _register and _registration.  I won't ask you to fix it now, but keep it in mind.

::: dom/push/PushService.jsm:1160
(Diff revision 1)
>        return Promise.all(records.map(record => {
> -        return record.getLastVisit().then(lastVisit => {
> -          if (lastVisit > record.lastPush) {
> +        return this._dropExpiredRegistration(record).catch(error => {
> +          debug("dropExpiredRegistrations: Error dropping registration " +
> +            record.keyID + ": " + error);
> +        });
> +      }));
> +    }).catch(error => {
> +      debug("dropExpiredRegistrations: Error dropping registrations: " +
> +        error);
> +    });

A single catch statement should suffice here (the outer one).  Note that the error will propagate so all you have to do is add record details to the error you generate in _dropExpiredRegistration

::: dom/push/PushService.jsm:1203
(Diff revision 1)
> +      return;

delete this, or return Promise.resolve();

Why do you need to return true/false here?

::: dom/push/test/xpcshell/test_permissions.js:43
(Diff revision 1)
> +    channelID: '10731540-5ce2-49c3-83ab-80b1552bdae0',

Can you make these channelID values more self-explanatory?  a UUID doesn't stick in the mind very well.

::: dom/push/test/xpcshell/test_permissions.js:114
(Diff revision 1)
> +    quota: Infinity,
> +    quota: 16,

Oops

::: dom/push/test/xpcshell/test_permissions.js:39
(Diff revision 1)
> +add_task(function* setUp() {
> +  // Two active registrations for a visited site. These will expire when we
> +  // revoke permissions.
> +  yield db.put({
> +    channelID: '10731540-5ce2-49c3-83ab-80b1552bdae0',
> +    pushEndpoint: 'https://example.org/push/1',
> +    scope: 'https://example.net/ham',
> +    pushCount: 0,
> +    lastPush: 0,
> +    version: null,
> +    originAttributes: '',
> +    quota: 16,
> +  });

Is there any reason that you can't condense this a little?  The only important fields are the channelID, the URL and the quota.  The rest is just noise.

::: dom/push/test/xpcshell/test_permissions.js:91
(Diff revision 1)
> +  // A registration not subject to quota. Permission list changes should
> +  // not affect this entry.

Please add the reason to the comment: ... because the quota is set to Infinity.

::: dom/push/test/xpcshell/test_permissions.js:227
(Diff revision 1)
> +  yield addVisit({
> +    uri: 'https://example.com/recipes',
> +    title: 'Spam musubi recipes',
> +    visits: [{
> +      visitDate: Date.now() * 1000,
> +      transitionType: Ci.nsINavHistoryService.TRANSITION_LINK,
> +    }],
> +  });
> +  Services.obs.notifyObservers(null, 'idle-daily', '');

I don't think that this is complete.  If a site has permission, but an expired registration, then it should create a new registration when the visit is registered, but you aren't testing for that new record being present.

Comment 12

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8644682 [details]
MozReview Request: Bug 1191453 - Drop subscriptions for a site when the user revokes push permissions. r=mt r?MattN

https://reviewboard.mozilla.org/r/15289/#review14223

::: dom/push/PushDB.jsm:253
(Diff revision 1)
> +            [aPageRecord.scope + "\x7f", aPageRecord.originAttributes + "\x7f"]

Shouldn't the originAttributes be checked for a precise match?

Comment 13

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/15289/#review18281

::: dom/push/PushService.jsm:1210
(Diff revision 1)
> +    // We only expire registrations if push permissions are revoked (the "block"
> +    // case). Adding or changing a grant, or deleting a revocation, drops all
> +    // expired registrations (the "always ask" and "allow" case).
> +    let isBlocked = (data == "added" || data == "changed") &&
> +      permission.capability == Ci.nsIPermissionManager.DENY_ACTION;

Does this patch properly handle switching a permission from allow to always ask? Without knowing much context or terminology, I think it should be treated like getting blocked but it doesn't seem to be here.

Comment 14

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/15289/#review14223

> Shouldn't the originAttributes be checked for a precise match?

Indeed. Thanks!

Comment 15

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/15289/#review13741

> A single catch statement should suffice here (the outer one).  Note that the error will propagate so all you have to do is add record details to the error you generate in _dropExpiredRegistration

The inner catch is intentional; if there's an error dropping one, it shouldn't affect the others. But that makes the outer catch redundant, so I removed that. Good catch!

> I don't think that this is complete.  If a site has permission, but an expired registration, then it should create a new registration when the visit is registered, but you aren't testing for that new record being present.

I'm not sure why I thought it was a good idea to entangle quotas and permissions, but I removed it. Granting permission after a prior revocation will now drop all expired records, even if the user hasn't visited the site recently. Easier to implement and understand.

Comment 16

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/15289/#review18281

> Does this patch properly handle switching a permission from allow to always ask? Without knowing much context or terminology, I think it should be treated like getting blocked but it doesn't seem to be here.

Ouch, it doesn't. The latest patch restructures `_onPermissionChange` to behave like this:

* If "allow" is set, we drop all expired subscriptions and notify the service worker.
* If "deny" is set, we mark all subscriptions as expired. The worker isn't notified until the permission is granted again, since it can't get a new subscription.
* If "allow" is removed, we treat it the same as "denied," per your comment. I think removing "allow" is the same as "always ask." Is that correct?
* If "deny" is removed, we don't do anything. I think that covers switching from "deny" to "always ask."

Does that seem reasonable?

Comment 17

[Lina Butler [:lina]]

Comment on attachment 8644682 [details]
MozReview Request: Bug 1191453 - Drop subscriptions for a site when the user revokes push permissions. r=mt r?MattN

Bug 1191453 - Drop subscriptions for a site when the user revokes push permissions. r?MattN,mt

Comment 18

[Martin Thomson [:mt:]]

Comment on attachment 8644682 [details]
MozReview Request: Bug 1191453 - Drop subscriptions for a site when the user revokes push permissions. r=mt r?MattN

https://reviewboard.mozilla.org/r/15289/#review18921

This looks fine.  I worry that this module is getting more complex though.  I see some serious refactoring in our future.

::: dom/push/PushService.jsm:1304
(Diff revisions 1 - 2)
> +      if (data == "added" || data == "changed") {
> +        if (capability == Ci.nsIPermissionManager.ALLOW_ACTION) {
> +          // Permission set to "allow". Drop all expired registrations for this
> +          // site, notify the associated service workers, and reset the quota
> +          // for active registrations.
> +          return this._permissionAllowed(records);
> +        }
> +        // Permission set to "block" or "always ask". Expire all registrations
> +        // for this site.
> +        return this._permissionDenied(records);
> +      }
> +      if (data == "deleted") {
> +        if (capability == Ci.nsIPermissionManager.ALLOW_ACTION) {
> +          // "Allow" permission removed. Treat this as a "block" and expire all
> +          // registrations for this site.
> +          return this._permissionDenied(records);
> +        }
> +        // "Block" or "always ask" permission removed. At this point, all
> +        // registrations have expired, so we don't need to call
> +        // `_permissionDenied`. We don't notify the service worker yet; that
> +        // happens when the permission is set to "allow".
> +        return null;

This could be a lot simpler.
In a new function:

```
// if the permission changes to allow
if (capability == ALLOW && (data == "added" || data == "changed")) {
  return this._db.getAllByOrigin(...) // factor this out if you like.
    .then(records => this._permissionAllowed(records));
// if the permissions changes to something other than allow,
// or if the permission to allow is removed
} else if (data == "added" || data == "changed" || (capability == ALLOW && data == "deleted")) {
  return this._db.getAllByOrigin(...) // factor this out if you like.
    .then(records => this._permissionDenied(records));
}
```
Note that this means that you don't query if there is no change to make.

::: dom/push/PushService.jsm:1365
(Diff revisions 1 - 2)
> +        return this._db.delete(record.keyID).then(() => {
> +          this._notifySubscriptionChangeObservers(record);
>          });

What are the rules for notifying change observers?  Because this ad hoc approach we've adopted isn't very reliable.

It would be better if there were wrapper functions for all these critical updates.

::: dom/push/PushService.jsm:1383
(Diff revisions 1 - 2)
> +      debug("expireRegistration: Error dropping expired registration " +
> +        record.keyID + ": " + error);

A lot of these debug statements are catching real problems.  You can and probably should log them to console.  Maybe open a bug to track doing that.

::: dom/push/test/xpcshell/test_permissions.js:70
(Diff revisions 1 - 2)
> +    '50fffd33-e016-4c9f-8ad1-4e348352a3be',

Can you use nicer strings for the channel ID please?

::: dom/push/test/xpcshell/test_permissions.js:252
(Diff revisions 1 - 2)
> +  ok(!droppedRecords.some(Boolean),

clever

::: dom/push/test/xpcshell/test_permissions.js:296
(Diff revisions 1 - 2)
> +add_task(function* tearDown() {
> +  yield db.drop();
> +  db.close();
> +});

Why isn't this a cleanup function like it was previously?

Comment 19

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/15289/#review18921

> Why isn't this a cleanup function like it was previously?

A failure in the final test caused it to run the cleanup function prematurely, but, somehow, the test still ran to completion...so it failed with another error, instead of the one actually responsible. Breaking this out into a separate task revealed the real failure. Reverted to the cleanup function.

Comment 20

[Lina Butler [:lina]]

Comment on attachment 8644682 [details]
MozReview Request: Bug 1191453 - Drop subscriptions for a site when the user revokes push permissions. r=mt r?MattN

Bug 1191453 - Drop subscriptions for a site when the user revokes push permissions. r=mt r?MattN

Comment 21

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ab652f44bbd

Comment 22

[Matthew N. [:MattN]]

Comment on attachment 8644682 [details]
MozReview Request: Bug 1191453 - Drop subscriptions for a site when the user revokes push permissions. r=mt r?MattN

https://reviewboard.mozilla.org/r/15289/#review19141

I'm only reviewing the parts that I previously commented on since I don't have enough context on the push backend and registrations to do a thorough review. My assumption is that mt covered that.

::: dom/push/PushService.jsm:1351
(Diff revisions 1 - 3)
> -      // registrations for this site.
> -      return Promise.all(records.filter(record =>
> -        record.isExpired()
> +   * Drops all expired registrations, notifies the associated service
> +   * workers, and resets the quota for active registrations if the push
> +   * permission is granted.

Why do we "Drop all expired registrations" when permission is granted? Shouldn't that have happened when the permission was revoked? Is this just some extra handling in case we somehow missed handling the revocation of the permission?

::: dom/push/PushService.jsm:1315
(Diff revision 3)
> +    } else if (isChange || isAllow && type == "deleted") {

Can you put in braces to make this easier to read i.e. making the operator precedence explicit?

Comment 23

[Lina Butler [:lina]]

(In reply to Matthew N. [:MattN] (behind on mail) from comment #22)
> Why do we "Drop all expired registrations" when permission is granted?
> Shouldn't that have happened when the permission was revoked? Is this just
> some extra handling in case we somehow missed handling the revocation of the
> permission?

It's tricky. When the permission is revoked, we mark all registrations as expired, but leave them in IndexedDB. When the permission is granted again, we drop the expired registrations and fire `pushsubscriptionchange` events. This addresses the case in comment 3, where the user enables the permission without revisiting the site.

> Can you put in braces to make this easier to read i.e. making the operator
> precedence explicit?

Certainly. I'll push the change directly once m-i reopens.

Comment 24

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=14372dacf2d5

Comment 25

[Lina Butler [:lina]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd18686b904dde3589b682fc934aba47f5cc2bb1
Bug 1191453 - Drop subscriptions for a site when the user revokes push permissions. r=mt,MattN

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cd18686b904d