Re-whitelist domains for users of PermissionsUtils.importFromPrefs

RESOLVED FIXED in Firefox 35

Status

()

RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: MattN, Assigned: markh)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {addon-compat})

unspecified
Firefox 35
addon-compat
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

Due to bug 771630 and bug 506446, users may have lost some of their default whitelists in the permission manager. This breaks things like UITour, LWT previews on AMO, and adds another warning for add-on and app installation. We should redo the whitelisting to help affected users who currently have broken experiences. We may want to do this soon (e.g. for the next UI Tour usage) and again once the two bugs are resolved.

This may be as simple as bumping the version number in the pref names to one in the future.

Of course this will affect users who explicitly removed the permissions but I don't see how we can detect that case. We should probably send an email to dev.platform and the enterprise mailing list to notify people if we do this.

MXR search: https://mxr.mozilla.org/mozilla-central/search?string=whitelist.add
Flags: firefox-backlog+
QA Whiteboard: [qa+]
Points: --- → 3
QA Whiteboard: [qa+]
Flags: qe-verify+
Depends on: 1058433, 1058435, 1058438, 1058442
No longer depends on: 771630
(Assignee)

Comment 1

4 years ago
Created attachment 8488427 [details] [diff] [review]
0001-Bug-1050080-add-default-permissions-for-uitour-and-a.patch

This patch previously had f+ in bug 506446, and that bug just landed on fx-team - assuming it makes it to central, this is the last piece of the puzzle to get these defaults back.

The try from bug 506446 - https://tbpl.mozilla.org/?tree=Try&rev=a3a9210acc3e - included this patch.

MattN, in that other bug you mentioned:

> Once this lands we should send an email to the enterprise mailing list 
> to let organizations who currently remove our default permissions to
> know about the new way.

I'm not aware of that list - are you able to send that mail (or point me at the list) once this lands?
Assignee: nobody → mhammond
Attachment #8488427 - Flags: review?(MattN+bmo)
Iteration: --- → 35.2
Status: NEW → ASSIGNED
Comment on attachment 8488427 [details] [diff] [review]
0001-Bug-1050080-add-default-permissions-for-uitour-and-a.patch

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

(In reply to Mark Hammond [:markh] from comment #1)
> MattN, in that other bug you mentioned:
> 
> > Once this lands we should send an email to the enterprise mailing list 
> > to let organizations who currently remove our default permissions to
> > know about the new way.
> 
> I'm not aware of that list - are you able to send that mail (or point me at
> the list) once this lands?

https://mail.mozilla.org/listinfo/enterprise

CC'ing firefox-dev would probably be good too.
Attachment #8488427 - Flags: review?(MattN+bmo) → review+

Updated

4 years ago
QA Contact: jbecerra
(Assignee)

Comment 3

4 years ago
While drafting the email, I noticed default_permissions also needs the line:

host	uitour	1	about:home

Given the patch to firefox.js:
-pref("browser.uitour.whitelist.add.340", "about:home");

I don't think this is worth re-review, but if you disagree, speak now or forever hold your peace ;)
(In reply to Mark Hammond [:markh] from comment #3)
> I don't think this is worth re-review, but if you disagree, speak now or
> forever hold your peace ;)

I agree but you can have r=me again regardless.
(Assignee)

Comment 5

4 years ago
Thanks :)

https://hg.mozilla.org/integration/fx-team/rev/1e0d3035de88

Mail to enterprise@ and firefox-dev@ sent.
https://hg.mozilla.org/mozilla-central/rev/1e0d3035de88
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Marking addon-compat in case an add-on used PermissionUtils (now removed) or needs to deal with the new behaviour for managing permissions.
Keywords: addon-compat
Mark, any objections to backing this out for causing bug 1072744, bug 1072748, and bug 1072751? 

Will a backout cause any problems with the permissions due to the permissions prefs being cleared upon being previously imported? i.e. if the defaults are removed, do they also get lost from profiles that used this patch because the default permissions aren't stored in the database anymore? If this will cause the permissions to get lost until it re-lands then maybe we shouldn't backout fully and instead file a new bug to revert only PermissionsUtils.jsm and XPIProvider.jsm but make sure that reverting XPIProvider.jsm won't mess up desktop permissions. What do you think?
Flags: needinfo?(mhammond)
(Assignee)

Comment 9

4 years ago
(In reply to Matthew N. [:MattN] from comment #8)
> Mark, any objections to backing this out for causing bug 1072744, bug
> 1072748, and bug 1072751? 

It's a shame, but no.

> Will a backout cause any problems with the permissions due to the
> permissions prefs being cleared upon being previously imported? i.e. if the
> defaults are removed, do they also get lost from profiles that used this
> patch because the default permissions aren't stored in the database anymore?

This shouldn't be a problem - we never removed those old defaults from the database - so if a user still had the defaults written by the pref system they should remain.

> If this will cause the permissions to get lost until it re-lands then maybe
> we shouldn't backout fully and instead file a new bug to revert only
> PermissionsUtils.jsm and XPIProvider.jsm but make sure that reverting
> XPIProvider.jsm won't mess up desktop permissions. What do you think?

That would certainly make relanding this less of a priority and leave us with working permissions for uitour in the scenarios that caused us to take this approach in the first place.  OTOH though, if we felt these problems were solvable in the next iteration I think it would be fine to back out fully and re-land.
Flags: needinfo?(mhammond)
(Assignee)

Updated

4 years ago
Depends on: 1073359
Depends on: 1298630
You need to log in before you can comment on or make changes to this bug.