Closed Bug 1159641 Opened 5 years ago Closed 4 years ago

pushManager.getSubscription() requests permission

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: matt, Assigned: lina)

References

Details

Attachments

(2 files, 1 obsolete file)

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
Component: Untriaged → DOM: Push Notifications
Product: Firefox → Core
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Unspecified → All
Hardware: Unspecified → All
Version: 40 Branch → Trunk
Assignee: nobody → dougt
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.
Attachment #8609961 - Flags: review?(nsm.nikhil) → review-
Assignee: dougt → nobody
Assignee: nobody → kcambridge
Assignee: kcambridge → nobody
Priority: -- → P3
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.
Blocks: 1219064
Bug 1159641, Part 1 - Skip the permission check in `pushManager.getSubscription()`. r?mt
Attachment #8680971 - Flags: review?(martin.thomson)
Bug 1159641, Part 2 - Use tasks in the Push permissions test. r?mt
Attachment #8680972 - Flags: review?(martin.thomson)
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
Attachment #8680971 - Flags: review?(martin.thomson) → review+
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
Attachment #8680972 - Flags: review?(martin.thomson) → review+
Attachment #8680971 - Attachment description: MozReview Request: Bug 1159641, Part 1 - Skip the permission check in `pushManager.getSubscription()`. r?mt → MozReview Request: Bug 1159641, Part 1 - Skip the permission check in `pushManager.getSubscription()`. r=mt
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 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
Attachment #8680972 - Attachment description: MozReview Request: Bug 1159641, Part 2 - Use tasks in the Push permissions test. r?mt → MozReview Request: Bug 1159641, Part 2 - Use tasks in the Push permissions test. r=mt
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.
Assignee: nobody → kcambridge
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/bda7d1b3f5b2
https://hg.mozilla.org/mozilla-central/rev/34c618cbadca
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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
Attachment #8609961 - Attachment is obsolete: true
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.
Attachment #8680971 - Flags: approval-mozilla-aurora?
(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!
Matt, could you please verify that this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(matt)
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.
Attachment #8680971 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Kit, I would also like to uplift the tests (patch 2) to Aurora44. Any reason why it should not be done? Thanks!
Flags: needinfo?(kcambridge)
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!
Flags: needinfo?(kcambridge)
Attachment #8680972 - Flags: approval-mozilla-aurora?
(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 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.
Attachment #8680972 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(matt)
You need to log in before you can comment on or make changes to this bug.