1402850 - Some optional permissions are displayed in the permissions pop-up

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P2)

Description

[CosminB]

Attachment: OptionalPermissionsAndroid.gif

[Affected versions]:
- Firefox 58.0a1 (2017-09-24) 

[Affected platforms]:
- Android 7.1.2

[Prerequisites]
- set extensions.webextPermissionPrompts to true
- set extensions.webextOptionalPermissionPrompts to true
- create extensions.webapi.testing and set it to true
- switch xpinstall.signatures.required to false

[Steps to reproduce]:
1.Toggle the install button for https://addons-dev.allizom.org/en-US/firefox/addon/optionalpermissionsandroid1-1/ 

[Expected results]:
- Only the permissions from “permissions” are displayed in the permissions pop-up.

[Actual results]:
- Some permissions from “optional_permissions” are displayed in the permissions pop-up.
- Once the optional_permissions are allowed on the device, they will be displayed in the permissions pop-up at every prompt for install.
The following permissions are displayed: clipboardRead, clipboardWrite, tabs, topSites, webNavigation.

Please see the attached video.

Comment 1

[CosminB]

This issue is also reproducing on Firefox 58.0a1 (2017-09-25) and Firefox 57.0b3 (20170925150345) Wind 7 64-bit and Ubuntu 16.04 32-bit

I will update the bug for the current issue.

Comment 2

[Andrew Swan [:aswan]]

Ugh how did this slip through so long.  We need to split userPermissions into separate properties for the fixed permissions derived from the manifest and the full permissions including dynamically granted ones.

Comment 4

[Andrew Swan [:aswan]]

Attachment: Bug 1402850 Don't include runtime permissions in prompts for webextension updates

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

Comment 5

[Tomislav Jovanovic :zombie]

Comment on attachment 8927536 [details]
Bug 1402850 Don't include runtime permissions in prompts for webextension updates

https://reviewboard.mozilla.org/r/198842/#review204614

::: toolkit/components/extensions/Extension.jsm:463
(Diff revision 1)
> -  // manifest.  The current implementation just returns the contents
> -  // of the permissions attribute, if we add things like url_overrides,
> -  // they should also be added here.
> +   * has access to based on fixed properties in the manifest.  The result
> +   * includes the contents of the "permissions" property as well as other
> +   * capabilities that are derived from manifest fields that users should
> +   * be informed of (e.g., origins where content scripts are injected).
> +   */
> +  get manifestPermissions() {

`manifest permissions` is still a bit ambiguous, how about `static`, `starting`, `original`, or `fixed`?

::: toolkit/components/extensions/Extension.jsm:468
(Diff revision 1)
> +    let permissions = new Set();
> +    let origins = new Set();
> +    for (let perm of this.manifest.permissions || []) {
> +      let type = classifyPermission(perm);
> +      if (type.origin) {
> +        origins.add(perm);
> +      } else if (!type.api) {
> +        permissions.add(perm);
> +      }
> +    }

This is missing the special handling for the `devtools` permission, and in general seems like duplicate code form `ExtensionData.parseManifest`.  Maybe better to store permissions in two buckets from the start, "original" and "granted optional"?

::: toolkit/components/extensions/Extension.jsm:496
(Diff revision 1)
> +  /**
> +   * Returns an object representing all capabilities this extension has
> +   * access to, including fixed ones from the manifest as well as dynamically
> +   * granted permissions.
> +   */
>    get userPermissions() {

Looking at this code, I'm guessing `ExtensionData.userPermissions` was named so to match the existing property of the `addon` objects?  

But now we are decoupling that (change in `XPInstall.jsm` below), and are left with two different meaning for `userPermissions` (neither of which is intuitive to me).  How about clarifying this by renaming this `activePermissions` or similar?

::: toolkit/components/extensions/Extension.jsm
(Diff revision 1)
>      let result = {
>        origins: this.whiteListedHosts.patterns.map(matcher => matcher.pattern),
>        apis: [...this.apiNames],
>      };
>  
> -    if (Array.isArray(this.manifest.content_scripts)) {

Hm, I don't understand why this change?

::: toolkit/components/extensions/test/xpcshell/test_ext_permissions.js:463
(Diff revision 1)
> +      manifest_version: 2,
> +      version: "1.0",
> +
> +      permissions: ["tabs", "https://test1.example.com/*"],
> +      optional_permissions: ["clipboardWrite", "<all_urls>"],
> +    },

Please dd a `content_script` for completeness.

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8927536 [details]
Bug 1402850 Don't include runtime permissions in prompts for webextension updates

https://reviewboard.mozilla.org/r/198842/#review204614

> `manifest permissions` is still a bit ambiguous, how about `static`, `starting`, `original`, or `fixed`?

It doesn't seem ambiguous to me, it is "permissions declared in the manifest"...

> Looking at this code, I'm guessing `ExtensionData.userPermissions` was named so to match the existing property of the `addon` objects?  
> 
> But now we are decoupling that (change in `XPInstall.jsm` below), and are left with two different meaning for `userPermissions` (neither of which is intuitive to me).  How about clarifying this by renaming this `activePermissions` or similar?

> Looking at this code, I'm guessing ExtensionData.userPermissions was named so to match the existing property of the addon objects?  

The original meaning was "permissions as they should be presented to the user".  This is to avoid confusion with the `permissions` proeprty from the manifest since the former includes things like content script targets, the devtools permission, etc.  But yes, I agree that this can be renamed, I'll do that.

> Hm, I don't understand why this change?

This was only in `userPermissions` so that an extension that has content scripts declared in the manifest includes the domains into which the script is injected in the permission prompt.  That's covered by moving this code to the `manifestPermissions` getter.  The other big user of `userPermissions` is `browser.permissions.getAll()` (and `contains()` etc) which should not report domains for manifest content scripts...

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8927536 [details]
Bug 1402850 Don't include runtime permissions in prompts for webextension updates

https://reviewboard.mozilla.org/r/198842/#review204614

> This is missing the special handling for the `devtools` permission, and in general seems like duplicate code form `ExtensionData.parseManifest`.  Maybe better to store permissions in two buckets from the start, "original" and "granted optional"?

Since this getter is called very infrequently (once at install time and once when an update is available), keeping a separate copy of the original permissions would be wasteful.  Moreover, the important logic here is abstracted away in `classifyPermissions()`, I don't think there's unnecessary duplication here.

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8927536 [details]
Bug 1402850 Don't include runtime permissions in prompts for webextension updates

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

Comment 9

[Tomislav Jovanovic :zombie]

Comment on attachment 8927536 [details]
Bug 1402850 Don't include runtime permissions in prompts for webextension updates

https://reviewboard.mozilla.org/r/198842/#review204614

r=me with a content_script added to the test for completeness.

> It doesn't seem ambiguous to me, it is "permissions declared in the manifest"...

I mean optional_permissions are also declared in the manifest.

Comment 10

[Tomislav Jovanovic :zombie]

Comment on attachment 8927536 [details]
Bug 1402850 Don't include runtime permissions in prompts for webextension updates

https://reviewboard.mozilla.org/r/198842/#review221898

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 8927536 [details]
Bug 1402850 Don't include runtime permissions in prompts for webextension updates

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

Comment 13

[Andrew Swan [:aswan]]

Comment on attachment 8927536 [details]
Bug 1402850 Don't include runtime permissions in prompts for webextension updates

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

Comment 14

[Andrew Swan [:aswan]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85b12303f147a81cbeadd0a37e8418752415e334
Bug 1402850 Don't include runtime permissions in prompts for webextension updates r=zombie

Comment 15

[Andrew Swan [:aswan]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/839b847c943e0908bf75da5915f37ddfe253037f
Bug 1402850 follow-up Remove old unused bit from permission test.  r=me CLOSED TREE

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a464d558ea8b
Backed out 2 changesets for mochitest browser chrome failures at browser_ext_devtools_inspectedWindow.js on a CLOSED TREE

Comment 17

[Cosmin Sabou [:CosminS]]

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=85b12303f147a81cbeadd0a37e8418752415e334&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=164948454

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=164993266&repo=mozilla-inbound&lineNumber=3394

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/a464d558ea8baa3daaba576c8bbed70d69c5af8f

Comment 18

[Andrew Swan [:aswan]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fee788fda968c7f8cefbf3b15ca311b415bf7eb
Bug 1402850 Don't include runtime permissions in prompts for webextension updates r=zombie

Comment 19

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/0fee788fda96

Comment 20

[CosminB]

Attachment: Bug1402850.gif

This issue is verified as fixed on Firefox 60.0a1 (20180301223350) under Windows 7 64-bit, Mac OS X 10.13.2 and Android 8.0.0 (Fennec 20180301223345).

The  granted optional permissions, are not displayed in the permissions pop-up at a second reinstall of the extension.

Please see the attached video.

Comment 21

[CosminB]

Attachment: Bug1402850-2.gif

Once you grant the optional permissions, if you uninstall the extension, then you install the extension again and you request for optional permissions, the optional permissions are automatically granted and the optional permissions pop-up are not displayed anymore for that extension in the current profile.

Is this expected?

Comment 22

[Andrew Swan [:aswan]]

(In reply to CosminB from comment #21)
> Once you grant the optional permissions, if you uninstall the extension,
> then you install the extension again and you request for optional
> permissions, the optional permissions are automatically granted and the
> optional permissions pop-up are not displayed anymore for that extension in
> the current profile.
> 
> Is this expected?

No, that's not expected, can you file a new bug?