1159641 - pushManager.getSubscription() requests permission

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect, P3)

Description

[matt]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2384.4 Safari/537.36

Steps to reproduce:

I've been mucking around trying to get a push demo page working with firefox nightly and noticed that I was being asked for permission to enable push before I was expecting it to (i.e. as soon as the page loads).

Demo Code: https://github.com/gauntface/simple-push-demo




Actual results:

The code that seems to be triggering the permission is: serviceWorkerRegistration.pushManager.getSubscription()


Expected results:

In Chrome you can call getSubscription() method without it prompting the user for permission. There is nothing in the spec to suggest that a permission request is required here.

Relevant(ish) part of the spec:
https://w3c.github.io/push-api/#widl-PushManager-getSubscription-Promise-PushSubscription#h-pushmanager-interface

Comment 1

[u279076]

I'm marking this bug as NEW since we're clearly doing something different than Chrome is this case. However, I'll leave it up to Mozilla's DOM engineers to decide whether or not we're operating as intended.

Comment 2

[Doug Turner (:dougt)]

Attachment: 0001-Bug-1159641-allow-getSubscription-to-be-called-witho.patch

Comment 3

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

Comment on attachment 8609961 [details] [diff] [review]
0001-Bug-1159641-allow-getSubscription-to-be-called-witho.patch

Review of attachment 8609961 [details] [diff] [review]:
-----------------------------------------------------------------

r- due to possibility of leaking information.

what happens in this case:
1) User grants permission, push is registered for.
2) Later, user revokes permission via page info dialog or something
3) Even later, page calls getSubscription().
In this case we don't want to leak knowledge of the existing subscription. I think getSubscription() should check for permission (not prompt) and fail with PermissionDeniedError in such a case. I've filed https://github.com/w3c/push-api/issues/150 which I think is the right thing to do, but :mt may have ideas. Could you update the patch to fail for now.

Comment 4

[Martin Thomson [:mt:]]

See https://github.com/w3c/push-api/issues/150

We should just retrieve the current subscription without a permission check.  A registration that doesn't have a subscription should just return null.

Comment 5

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1159641, Part 1 - Skip the permission check in `pushManager.getSubscription()`. r=mt

Bug 1159641, Part 1 - Skip the permission check in `pushManager.getSubscription()`. r?mt

Comment 6

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1159641, Part 2 - Use tasks in the Push permissions test. r=mt

Bug 1159641, Part 2 - Use tasks in the Push permissions test. r?mt

Comment 7

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fd409d49ae4

Comment 8

[Martin Thomson [:mt:]]

Comment on attachment 8680971 [details]
MozReview Request: Bug 1159641, Part 1 - Skip the permission check in `pushManager.getSubscription()`. r=mt

https://reviewboard.mozilla.org/r/23707/#review21217

::: dom/push/Push.js:67
(Diff revision 1)
>    askPermission: function (aAllowCallback, aCancelCallback) {

This would be a whole lot less ugly if this returned a promise.

::: dom/push/Push.js:140
(Diff revision 1)
> +      if (this._testPermission() != Ci.nsIPermissionManager.ALLOW_ACTION) {

Is there any case where the permission will not be == allow and there is a live subscription?  If not, this check is not needed.

::: dom/push/Push.js:140
(Diff revision 1)
> +      if (this._testPermission() != Ci.nsIPermissionManager.ALLOW_ACTION) {
> +        resolve(null);
> +        return;
> +      }

Is there any case where the permission will not be == allow but there is a live subscription?  I'm not suggesting that you remove the check, but I'd like to be sure we're safe.

::: dom/push/Push.js:202
(Diff revision 1)
> +      sub = new pushManager._window.PushSubscription(endpoint,
> +                                                     pushManager._scope,
> +                                                     publicKey);
> +    } else {
> +      sub = new pushManager._window.PushSubscription(endpoint,
> +                                                     pushManager._scope,
> +                                                     null);

Maybe move the setter for publicKey into the branch rather than duplicating the assignment to sub

Comment 9

[Martin Thomson [:mt:]]

Comment on attachment 8680972 [details]
MozReview Request: Bug 1159641, Part 2 - Use tasks in the Push permissions test. r=mt

https://reviewboard.mozilla.org/r/23709/#review21227

Comment 10

[Lina Butler [:lina]]

Comment on attachment 8680971 [details]
MozReview Request: Bug 1159641, Part 1 - Skip the permission check in `pushManager.getSubscription()`. r=mt

Bug 1159641, Part 1 - Skip the permission check in `pushManager.getSubscription()`. r=mt

Comment 11

[Lina Butler [:lina]]

Comment on attachment 8680972 [details]
MozReview Request: Bug 1159641, Part 2 - Use tasks in the Push permissions test. r=mt

Bug 1159641, Part 2 - Use tasks in the Push permissions test. r=mt

Comment 12

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/23707/#review21217

> This would be a whole lot less ugly if this returned a promise.

Indeed, good call. Fixed.

> Is there any case where the permission will not be == allow and there is a live subscription?  If not, this check is not needed.

It's possible to have an expired subscription if the user revoked the permission, but the correct place to handle that is `PushService.registration()`. Fixed.

Comment 13

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=622a016c7c79

Comment 14

[Lina Butler [:lina]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bda7d1b3f5b2188d82e51fca69eba584ea64edae
Bug 1159641, Part 1 - Skip the permission check in `pushManager.getSubscription()`. r=mt

https://hg.mozilla.org/integration/mozilla-inbound/rev/34c618cbadca3b92003136715dd497ca7694fd61
Bug 1159641, Part 2 - Use tasks in the Push permissions test. r=mt

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bda7d1b3f5b2
https://hg.mozilla.org/mozilla-central/rev/34c618cbadca

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/bda7d1b3f5b2
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/34c618cbadca

Comment 17

[neil]

I'm still seeing the original behavior in 44.0a2 (2015-11-30), specifically: a call to serviceWorkerRegistration.pushManager.getSubscription() triggers the permission dialog rather than just returning null when no subscription exists

Comment 18

[Lina Butler [:lina]]

Comment on attachment 8680971 [details]
MozReview Request: Bug 1159641, Part 1 - Skip the permission check in `pushManager.getSubscription()`. r=mt

Approval Request Comment
[Feature/regressing bug #]: This bug.
[User impact if declined]: Web compatibility issue (the current behavior in Auora doesn't match the spec, or Chrome's implementation). Potentially disruptive user experience due to the extra permission dialog.
[Describe test coverage new/current, TreeHerder]: Part 2 includes new tests for this fix.
[Risks and why]: Low risk, due to additional test coverage and time spent on Nightly.
[String/UUID change made/needed]: None.

Comment 19

[Lina Butler [:lina]]

(In reply to neil from comment #17)
> I'm still seeing the original behavior in 44.0a2 (2015-11-30), specifically:
> a call to serviceWorkerRegistration.pushManager.getSubscription() triggers
> the permission dialog rather than just returning null when no subscription
> exists

Right, this is only fixed in Nightly (45). Since it's a compatibility issue, and a low-risk fix, I've requested an uplift to 44. Thanks, Neil!

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Matt, could you please verify that this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8680971 [details]
MozReview Request: Bug 1159641, Part 1 - Skip the permission check in `pushManager.getSubscription()`. r=mt

This fix has been in Nightly for several weeks now, seems safe to uplift to Aurora44. Push notifications is planned for 44.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Kit, I would also like to uplift the tests (patch 2) to Aurora44. Any reason why it should not be done? Thanks!

Comment 23

[Lina Butler [:lina]]

Comment on attachment 8680972 [details]
MozReview Request: Bug 1159641, Part 2 - Use tasks in the Push permissions test. r=mt

Sure, sounds good. Thanks, Ritu!

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Kit Cambridge [:kitcambridge] from comment #23)
> Comment on attachment 8680972 [details]
> MozReview Request: Bug 1159641, Part 2 - Use tasks in the Push permissions
> test. r=mt
> 
> Sure, sounds good. Thanks, Ritu!

Awesome! Thank you.

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8680972 [details]
MozReview Request: Bug 1159641, Part 2 - Use tasks in the Push permissions test. r=mt

Let's uplift tests to Aurora as well so we can catch regressions easily.

Comment 26

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/6dae0b0bdb8c
https://hg.mozilla.org/releases/mozilla-aurora/rev/3b528ca2a11a

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/6dae0b0bdb8c
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/3b528ca2a11a