1317590 - AddonManager support for webextensions permissions

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[Andrew Swan [:aswan]]

The series of blocks blocking bug 1308292 are all about various scenarios in which a user is prompted to accept the permissions required by a webextension when it is installed or upgraded.  Those bugs all involve modifying different parts of the front-end UI code to display prompts but they have a common need from the underlying AddonManager: a way to be notified during the install/upgrade process when the extension's permissions are known and to pause the operation until the user has accepted those permissions (or cancel it if the user opts not to accept the permissions).
This bug is to add that support to AddonManager.

Comment 1

[Andrew Swan [:aswan]]

Attachment: Bug 1317590 AddonManager support for permissions

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

Comment 2

[Robert Helmer [:rhelmer]]

Comment on attachment 8810717 [details]
Bug 1317590 AddonManager support for permissions

https://reviewboard.mozilla.org/r/93018/#review92946

This looks like the right approach.

I think that resuming a system add-on that is a webextension would trigger the UI prompt right now, which we don't want - https://dxr.mozilla.org/mozilla-central/rev/79feeed4293336089590320a9f30a813fade8e3c/toolkit/mozapps/extensions/internal/XPIProvider.jsm#8994 could instead set the state to `AddonManager.STATE_READY` and I think it'd probably work.

I don't believe any of the current system add-on tests install a webextension (and we don't ship any currently), but we probably will at some point. We should probably have a test one to ensure that there is no UI prompt on normal install, resuming a postponed install, and startup.

::: toolkit/components/extensions/Extension.jsm:367
(Diff revision 1)
>        });
>      });
>    }
>  
> +  userPermissions() {
> +    // TODO: Add things like url_overrides here as well

Are you going to land this TODO? It'd be nice to file a bug and mention the number here, makes it easier to figure out if the comment is safe to remove later.

::: toolkit/mozapps/extensions/AddonManager.jsm:1873
(Diff revision 1)
>     *         An optional placeholder name while the add-on is being downloaded
>     * @param  aIcons
>     *         Optional placeholder icons while the add-on is being downloaded
>     * @param  aVersion
>     *         An optional placeholder version while the add-on is being downloaded
> -   * @param  aLoadGroup
> +   * @param  aBrowser

We really need the eslint jsdoc rule :)

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:5440
(Diff revision 1)
>     * @throws if installation cannot proceed from the current state
>     */
>    install() {
>      switch (this.state) {
>      case AddonManager.STATE_DOWNLOADED:
> +      // XXX need to store somewhere non-temp while permissions are pending?

I think a non-temp download area could work exactly like the staging area - a subdirectory of the install location, removed when cleanup is called after all locks have released, etc.

You could make creating+locking/releasing the staging directory into more generic functions, and have `getStagingDir` and `getDownloadDir` methods on the `installLocation` use this.

The difference between download/staging area is that we'd never load add-ons in the download area on startup (or otherwise), I think it's worth having a test to make sure it stays that way.

If you intend to land this and do the download area as a followup then I'd file a bug for this too.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:515
(Diff revision 1)
> +  deepEqual(perms.hosts, ["https://*.example.com/*", "<all_urls>"], "Host permissions are correct");
> +  deepEqual(perms.apis, ["test"], "Experiments permissions are correct");
> +
> +  let addon = yield promiseAddonByID(perminfo.addon.id);
> +  notEqual(addon, null, "Extension was installed");
> +  

stray tab

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:549
(Diff revision 1)
> +
> +  notEqual(perminfo, undefined, "Permission handler was invoked");
> +
> +  let addon = yield promiseAddonByID(perminfo.addon.id);
> +  equal(addon, null, "Extension was not installed");
> +  

stray tab

Comment 3

[Andrew Swan [:aswan]]

Comment on attachment 8810717 [details]
Bug 1317590 AddonManager support for permissions

https://reviewboard.mozilla.org/r/93018/#review92946

The permHandler has to be set by the code that creates the AddonInstall (since the way the prompts are presented is slightly different for different flows).  So, as long as installAddonSet() doesn't set the permHandler property of the AddonInstall objects it creates, you won't see any prompts for system add-ons.
I'm not sure how to write a negative test, it would be something like: if no permHandler is set, verify that the nonexistent permHandler doesn't get called?  That last bit is tricky :-)

> Are you going to land this TODO? It'd be nice to file a bug and mention the number here, makes it easier to figure out if the comment is safe to remove later.

We don't actually support url_overrides yet but I wanted to indicate that things from the manifest other than permissions might eventually be included here.  I'll revise the comment to be clearer.

> I think a non-temp download area could work exactly like the staging area - a subdirectory of the install location, removed when cleanup is called after all locks have released, etc.
> 
> You could make creating+locking/releasing the staging directory into more generic functions, and have `getStagingDir` and `getDownloadDir` methods on the `installLocation` use this.
> 
> The difference between download/staging area is that we'd never load add-ons in the download area on startup (or otherwise), I think it's worth having a test to make sure it stays that way.
> 
> If you intend to land this and do the download area as a followup then I'd file a bug for this too.

Yes, I expect to add this as part of bug 1317470.  I agree with the approach you outlined.  I'll revise the comment to be something more appropriate.

Comment 4

[Andrew Swan [:aswan]]

Comment on attachment 8810717 [details]
Bug 1317590 AddonManager support for permissions

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

Comment 5

[Robert Helmer [:rhelmer]]

Comment on attachment 8810717 [details]
Bug 1317590 AddonManager support for permissions

https://reviewboard.mozilla.org/r/93018/#review93580

Comment 6

[Robert Helmer [:rhelmer]]

(In reply to Andrew Swan [:aswan] from comment #3)
> Comment on attachment 8810717 [details]
> Bug 1317590 AddonManager support for permissions
> 
> https://reviewboard.mozilla.org/r/93018/#review92946
> 
> The permHandler has to be set by the code that creates the AddonInstall
> (since the way the prompts are presented is slightly different for different
> flows).  So, as long as installAddonSet() doesn't set the permHandler
> property of the AddonInstall objects it creates, you won't see any prompts
> for system add-ons.

Ah, I had missed that thanks. I am less concerned.

> I'm not sure how to write a negative test, it would be something like: if no
> permHandler is set, verify that the nonexistent permHandler doesn't get
> called?  That last bit is tricky :-)

I was imagining an xpcshell test that did a restartless install/upgrade of a system add-on, and ensured that it never passed through the unexpected state. Or even a mochitest that ensured that there wasn't a prompt etc.

But as I said I am less concerned about this now.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d35e32293f21
AddonManager support for permissions r=rhelmer

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57f5c7942140
followup, placate eslint

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d35e32293f21
https://hg.mozilla.org/mozilla-central/rev/57f5c7942140