Closed
Bug 1339952
Opened 7 years ago
Closed 7 years ago
List permissions in descending order of priority in the install prompts
Categories
(WebExtensions :: General, enhancement, P3)
WebExtensions
General
Tracking
(firefox54 verified)
VERIFIED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | verified |
People
(Reporter: krupa--use, Assigned: aswan)
References
Details
(Whiteboard: [permissions] triaged)
Attachments
(1 file)
We show users permissions which the webextension will have access listed in a doorhanger when the user clicks install. It would make sense to list the most important/invasive permissions at the top so that someone skimming for information doesn't miss what permissions they are granting to a webextension.
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(amckay)
Comment 1•7 years ago
|
||
In order: * the hosts permission: because this is probably the most understandable risk. It's related directly to what people use firefox for, browsing. * native messaging: because it means you are getting out of the Firefox sandbox. * the rest (no order) There's probably a lot of bikeshedding we can do on the rest, I don't think its worth doing right now though and should be easy to change later.
Flags: needinfo?(amckay)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
Florian, any thoughts on how to test the attached patch? Testing from a mochitest seems awkward with the localized strings. Or I could rearrange the code a whole bunch to make it possible to write an xpcshell test of _buildStrings but that seems like overkill. I'll go with "manual testing by QA" unless you have some other clever idea.
Flags: needinfo?(florian)
Comment 4•7 years ago
|
||
(In reply to Andrew Swan [:aswan] from comment #3) > Florian, any thoughts on how to test the attached patch? Testing from a > mochitest seems awkward with the localized strings. How are the localized strings a problem? The test should be able to use the string bundle, and figure out what the expected localized strings are. (If the localized strings are actually a problem here, we may find a way to override the file of the bundle for the duration of the test; but that's likely more work.)
Flags: needinfo?(florian)
Assignee | ||
Comment 5•7 years ago
|
||
Sorry I should have been more precise, they are localized and formatted (i.e., host names and domain names are inserted into some permissions, the application name into another). Re-implementing all that logic in the test sounds like a bad idea. What would be nice is a function that determines whether a particular string looks like a valid formatted version of some raw string. E.g.: ``` let unformatted = "Some string containing %S or something"; // this would come from bundle.getString() let formatted = "Some string containing a substring or something"; // this would come from the DOM ok(isFormattedFrom(formatted, unformatted); ``` Do you know if such a thing exists?
Flags: needinfo?(florian)
Comment 6•7 years ago
|
||
(In reply to Andrew Swan [:aswan] from comment #5) > Sorry I should have been more precise, they are localized and formatted > (i.e., host names and domain names are inserted into some permissions, the > application name into another). Re-implementing all that logic in the test > sounds like a bad idea. What would be nice is a function that determines > whether a particular string looks like a valid formatted version of some raw > string. E.g.: > > ``` > let unformatted = "Some string containing %S or something"; // this would > come from bundle.getString() > let formatted = "Some string containing a substring or something"; // this > would come from the DOM > ok(isFormattedFrom(formatted, unformatted); > > ``` > > Do you know if such a thing exists? I'm not aware of any such function, but it seems easy to replace "%S" (and variations of it with numbered parameters) with ".*" and then build a regexp. If you decide to go this way, don't forget to escape regexp special characters, eg: let exp = /[[\]{}()*+?.\\^$|]/g; string.replace(exp, "\\$&")); I'm not convinced that makes a better test than just 're-implementing' some of the logic in the test (I'm saying 'some' because you would hardcode a few specific cases for the test; not have a copy/paste of loops we have in the actual code).
Flags: needinfo?(florian)
Updated•7 years ago
|
Priority: -- → P3
Whiteboard: [permissions] triaged
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
I compromised a bit, I built exact strings for the host permissions but for the native messaging permission which is "Exchange messages with programs other than %S" where %S can be Firefox, Nightly, etc., I just compare the string up to the %S. Its a bit of a shortcut but I think its adequate for this bug...
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → aswan
Assignee | ||
Updated•7 years ago
|
Attachment #8840696 -
Flags: review?(florian)
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8840696 [details] Bug 1339952 Sort order of permission prompts https://reviewboard.mozilla.org/r/115128/#review117406 ::: browser/base/content/test/webextensions/head.js:139 (Diff revision 2) > + * @param {string} msg > + * The message to be emitted as part of the actual test. > + */ > +function checkPermissionString(string, key, param, msg) { > + if (!_stringBundle) { > + _stringBundle = Services.strings.createBundle("chrome://browser/locale/browser.properties"); Is gBrowserBundle usable here? ::: browser/base/content/test/webextensions/head.js:144 (Diff revision 2) > + _stringBundle = Services.strings.createBundle("chrome://browser/locale/browser.properties"); > + } > + > + let localizedString = _stringBundle.GetStringFromName(key); > + if (param) { > + localizedString = localizedString.replace("%S", param); Is this really easier than calling formatStringFromName? ::: browser/base/content/test/webextensions/head.js:153 (Diff revision 2) > + // If this is a localized string and the parameter is supplied, just > + // substitute it and do a simple compare. If the parameter is not > + // supplied, cheat a bit and just compare everything before the %S. > + if (localizedString.includes("%S")) { > + let i = localizedString.indexOf("%S"); > + ok(string.startsWith(localizedString.slice(0, i)), msg); This will always pass if we ever get strings that start with %S. ::: browser/base/content/test/webextensions/head.js:253 (Diff revision 2) > + "Third permission is nativeMessaging"); > + checkPermissionString(ul.children[3].textContent, > + "webextPerms.description.tabs", null, > + "Fourth permission is tabs"); > + checkPermissionString(ul.children[4].textContent, > + "webextPerms.description.history", null, Is the order between 'tabs' and 'history' enforced anywhere in the code?
Attachment #8840696 -
Flags: review?(florian)
Assignee | ||
Comment 10•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8840696 [details] Bug 1339952 Sort order of permission prompts https://reviewboard.mozilla.org/r/115128/#review117406 > Is the order between 'tabs' and 'history' enforced anywhere in the code? The current implementation displays them in the order they appear in the manifest.
Comment hidden (mozreview-request) |
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8840696 [details] Bug 1339952 Sort order of permission prompts https://reviewboard.mozilla.org/r/115128/#review117558
Attachment #8840696 -
Flags: review?(florian) → review+
Comment 13•7 years ago
|
||
Pushed by aswan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/478066c98ffb Sort order of permission prompts r=florian
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/478066c98ffb
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 15•7 years ago
|
||
Verified fixed on Firefox 54.0a1 (2017-03-03) under Windows 10 64-bit, Ubuntu 16.04 32-bit and Mac OS X 10.12.1. Permissions displayed in installation doorhangers are successfully listed in descending order of priority: https://www.screencast.com/t/c0BPPd4Hy
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•