1339952 - List permissions in descending order of priority in the install prompts

Status: VERIFIED FIXED

(WebExtensions :: General, enhancement, P3)

Description

[krupa raj [:krupa--use this to needinfo]]

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.

Comment 1

[Andy McKay]

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.

Comment 2

[Andrew Swan [:aswan]]

Attachment: Bug 1339952 Sort order of permission prompts

Review commit: https://reviewboard.mozilla.org/r/115128/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/115128/

Comment 3

[Andrew Swan [:aswan]]

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.

Comment 4

[Florian Quèze [: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.)

Comment 5

[Andrew Swan [:aswan]]

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?

Comment 6

[Florian Quèze [: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).

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8840696 [details]
Bug 1339952 Sort order of permission prompts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115128/diff/1-2/

Comment 8

[Andrew Swan [:aswan]]

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...

Comment 9

[Florian Quèze [: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?

Comment 10

[Andrew Swan [:aswan]]

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 11

[Andrew Swan [:aswan]]

Comment on attachment 8840696 [details]
Bug 1339952 Sort order of permission prompts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115128/diff/2-3/

Comment 12

[Florian Quèze [:florian]]

Comment on attachment 8840696 [details]
Bug 1339952 Sort order of permission prompts

https://reviewboard.mozilla.org/r/115128/#review117558

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/478066c98ffb
Sort order of permission prompts r=florian

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/478066c98ffb

Comment 15

[Vasilica Tamas, QA [:vtamas]]

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