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)

enhancement

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.
Blocks: 1342142
Flags: needinfo?(amckay)
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)
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)
(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)
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)
(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)
Priority: -- → P3
Whiteboard: [permissions] triaged
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: nobody → aswan
Attachment #8840696 - Flags: review?(florian)
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)
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 on attachment 8840696 [details]
Bug 1339952 Sort order of permission prompts

https://reviewboard.mozilla.org/r/115128/#review117558
Attachment #8840696 - Flags: review?(florian) → review+
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/478066c98ffb
Sort order of permission prompts r=florian
https://hg.mozilla.org/mozilla-central/rev/478066c98ffb
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.