Exempt system Push subscriptions from quota and permissions checks

RESOLVED FIXED in Firefox 46

Status

()

Core
DOM: Push Notifications
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kitcambridge, Assigned: kitcambridge)

Tracking

Trunk
mozilla46
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

We shouldn't enforce the quota or permission checks for subscriptions created by the system principal. This is important for chrome callers like FxA and Hello.
Created attachment 8707747 [details]
MozReview Request: Bug 1239558 - Exempt system Push subscriptions from quota and permissions checks. r?dragana

Review commit: https://reviewboard.mozilla.org/r/30861/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30861/
Attachment #8707747 - Flags: review?(dd.mozilla)
https://reviewboard.mozilla.org/r/30859/#review27967

r=me 

with answer for questions below.

::: dom/push/PushServiceHttp2.jsm:324
(Diff revision 1)
> -      quota: this._subInfo.record.maxQuota,
> +      systemRecord: this._subInfo.record.systemRecord,

Now you are not setting quota any more. This will work fine for systemprincipla because it is not using them, but for others? Are the they always set somewhare?

Looking at this, is there a test that have a custom principal?
Attachment #8707747 - Flags: review?(dd.mozilla)
Attachment #8707747 - Flags: review?(dd.mozilla)
Comment on attachment 8707747 [details]
MozReview Request: Bug 1239558 - Exempt system Push subscriptions from quota and permissions checks. r?dragana

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30861/diff/1-2/
https://reviewboard.mozilla.org/r/30859/#review27967

> Now you are not setting quota any more. This will work fine for systemprincipla because it is not using them, but for others? Are the they always set somewhare?
> 
> Looking at this, is there a test that have a custom principal?

Originally, only chrome callers could set `record.maxQuota`. The `PushRecord` constructor used the value as a suggestion; if it was omitted or invalid, it would use the default quota instead. `suggestedQuota` would probably have been a better name than `maxQuota`.

`test_permissions.js`, `test_quota{exceeded, observer, with_notifications}.js` have some tests for non-system subscriptions, and I added a few to `test_service_child.js`.
https://reviewboard.mozilla.org/r/30861/#review28265

::: dom/push/PushService.jsm:611
(Diff revision 2)
> +    if (this._state <= PUSH_SERVICE_ACTIVATING) {

This is an unrelated change, but `test_register_success_http2.js` crashed a few times on my OS X machine today: https://pastebin.mozilla.org/8857063 Looks like `connOnStop()` tries to set a reconnect alarm as xpcshell shuts down, causing a crash when the `AlarmService` tries to initialize. It's odd that this hasn't happened before, but I can reproduce it on m-c, even without this patch.

Let's see what Try says. It might be something odd about my configuration.
Status: NEW → ASSIGNED
Blocks: 1239584
Comment on attachment 8707747 [details]
MozReview Request: Bug 1239558 - Exempt system Push subscriptions from quota and permissions checks. r?dragana

https://reviewboard.mozilla.org/r/30861/#review28677

Thanks.
Attachment #8707747 - Flags: review?(dd.mozilla) → review+

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1f75c8d8172d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.