1282981 - Implement management.get and getAll

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P5)

Description

[Matthew Wein [:mattw]]

Chrome documentation: https://developer.chrome.com/extensions/management#method-getAll

Comment 2

[Kris Maglione [:kmag]]

The last time that we talked about this, there was some question about whether we actually wanted to implement it. As I recall, we decided we wouldn't implement it until we had some idea of what extensions are actually using it for.

Comment 3

[Bob Silverberg [:bsilverberg]]

Some examples of usage from "popular chrome extensions":

- Video Downloader Pro is using it to look for a particular extension (Google Cast) to see if it is installed and/or enabled. If both are true then it uses it for playback. If it is not installed the user is asked to install it, and if it is not enabled then the user is asked to enable it.

- Adblock uses it in a number of ways:
    - It gathers a list of all installed extensions which is then provided to a bug report which can be submitted.
    - It gathers a list of all installed extensions for display on a screen to help diagnose problems.
    - It has a feature that allows a user to disable all other extensions, for which it uses getAll to get the list of those
      extensions. I assume that this is also for troubleshooting purposes.

- Flash Video Downloader uses it in a similar manner to Video Downloader Pro in that it looks for specific extensions to see if they are installed, and if they are it enables features of itself that are only available if said extensions are installed.

Do these seem like valid enough use cases to implement the method? We have flagged it for implementation as part of the project to enable more Chrome extensions to run in Firefox.

Comment 5

[:shell escalante]

should also implement chrome.management.get at same time - putting on one bug (closed bug 1283687 as dupe to this one)

Chrome documentation: https://developer.chrome.com/extensions/management#method-get

Comment 6

[Kris Maglione [:kmag]]

(In reply to Bob Silverberg [:bsilverberg] from comment #3)
> - Video Downloader Pro is using it to look for a particular extension
> (Google Cast) to see if it is installed and/or enabled. If both are true
> then it uses it for playback. If it is not installed the user is asked to
> install it, and if it is not enabled then the user is asked to enable it.

Does Google Cast even work in Firefox? Is it even possible for it to?

> - Adblock uses it in a number of ways:
>     - It gathers a list of all installed extensions which is then provided
> to a bug report which can be submitted.
>     - It gathers a list of all installed extensions for display on a screen
> to help diagnose problems.

We have about:support for this.

>     - It has a feature that allows a user to disable all other extensions,
> for which it uses getAll to get the list of those
>       extensions. I assume that this is also for troubleshooting purposes.

This would require us to also implement that functionality for it to be
useful.

Either way, the lack of this feature doesn't seem like it should prevent
significant parts of the add-on from working.

> - Flash Video Downloader uses it in a similar manner to Video Downloader Pro
> in that it looks for specific extensions to see if they are installed, and
> if they are it enables features of itself that are only available if said
> extensions are installed.

Are any of these extensions relevant to Firefox?

Maybe we should consider initially implementing this as a stub that just
returns the entry for the add-on itself, and decide later how we want to
handle the full implementation.

Comment 7

[Bob Silverberg [:bsilverberg]]

We are going to hold off on implementing this API method until some compelling use cases are presented by the developer community.

Comment 8

[Rob Wu [:robwu]]

Here is another use case (it's not in the current Chromium extension yet, but I'll implement it in the future if Firefox also supports it):

Retrieving a list of installed addons so that their source code can be viewed with my Extension Source Viewer (https://github.com/Rob--W/crxviewer). For this I would need management.getAll + the following of ExtensionInfo (https://developer.chrome.com/extensions/management#type-ExtensionInfo):
- id (to find the listing on AMO)
- name
- version (to select a specific listing on AMO)
- icons (optional, to make the list of extensions prettier)
- installType (to see whether it's probably installed via the addon gallery - e.g. "development" if loaded via about:debugging and "normal" otherwise).

Comment 9

[adam.schenker]

Here is our use case for this feature:

We are an international market research company that recruits volunteers to install software (including a Firefox extension) that automatically reports on their media consumption and web usage. We would like to get information about other installed extensions so that we can report statistics such as marketshare and install base to our clients.

We currently report this information for Firefox using a legacy extension, but will lose this data when switching to WebExtensions. There is a potential for Firefox extension authors and Mozilla to be impacted by this if they happen to be clients as well.

Comment 10

[Klaas Van Parys]

This is a use case that I'm already using in Chrome & Opera.

I have an extension for OGame, a browser game, called UniverseView.
There are also a lot of other extensions and this API helps me to detect which extensions are installed.
When a certain extension is installed my extension doesn't execute certain code to avoid conflicts.

Comment 11

[Michel Gutierrez]

One more use case for this: the weh toolkit (https://github.com/mi-g/weh) is a set of tools to help developing WebExtensions add-ons. It contains an extension, weh-inspector, able to monitor messages, preferences and storage from weh-based installed extensions. The lack of browser.management.getAll API prevents weh-inspector to discover automatically what other add-ons can be inspected.

Note that the lack of runtime.messageExternal support (bug#1258360) is even more critical for weh-inspector to run on Firefox.

Comment 12

[mozilla.utensil]

Another case here: in Adnauseam(https://github.com/dhowe/AdNauseam), we use management.getAll to check for enabled extensions that may conflict (other adblockers making modifications to request headers, etc), and then warn the user that only one should probably be used at the same time.

Comment 14

[Stefany Segovia]

I have a use case for this bug: Persona Switcher (https://github.com/drsjb80/personaswitcher) is an add-on that lets you easily switch between personas/themes in Mozilla Firefox, Thunderbird, and SeaMonkey.  Currently we are working on migrating this add-on to a WebExtension, however, we will lose the functionality and be unable to switch between personas once we do this. The management API would help us access the themes and switch between them in Firefox with the getAll() and setEnabled() functions

Comment 16

[Bob Silverberg [:bsilverberg]]

Bug 1336908 implemented management.getAll but for themes only. Based on all the use cases listed above, and the inclusion of this bug on Andy's list of papercuts, do we want to remove that restriction to allow management.getAll to return all extensions?

Comment 17

[Michel Gutierrez]

I see many good reasons for having APIs and features other browsers don't have, like file access, sockets, user translations, ... but none to skip some, like management.getAll, that are available elsewhere, even more when we have valid use cases.

Comment 18

[Andy McKay]

(In reply to Bob Silverberg [:bsilverberg] from comment #16)
> Bug 1336908 implemented management.getAll but for themes only. Based on all
> the use cases listed above, and the inclusion of this bug on Andy's list of
> papercuts, do we want to remove that restriction to allow management.getAll
> to return all extensions?

Fine by me.

Comment 19

[Bob Silverberg [:bsilverberg]]

Alright, I will implement management.getAll for all extensions, but I am removing management.get from this bug as we're not going to implement that, at least not now.

Comment 20

[Shane Caraveo (:mixedpuppy)]

(In reply to Bob Silverberg [:bsilverberg] from comment #19)
> Alright, I will implement management.getAll for all extensions, but I am
> removing management.get from this bug as we're not going to implement that,
> at least not now.

Why?  get is almost a copy/paste of getSelf, just a couple lines of code.

Comment 21

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1282981 - Implement management.get and getAll,

This updates the existing implementation of browser.management.getAll to allow both themes and extensions
to be returned.

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

Comment 22

[Bob Silverberg [:bsilverberg]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #20)
> (In reply to Bob Silverberg [:bsilverberg] from comment #19)
> > Alright, I will implement management.getAll for all extensions, but I am
> > removing management.get from this bug as we're not going to implement that,
> > at least not now.
> 
> Why?  get is almost a copy/paste of getSelf, just a couple lines of code.

Fair enough. Initially I was thinking that we didn't implement get for a reason, and it doesn't seem like any use cases have presented themselves for it, but I guess if we're implementing getAll that will give developers all the same information, so we might as well implement get too.

Comment 23

[Andrew Swan [:aswan]]

sorry for the drive by but this patch should also include a permissions string for the management permissions

Comment 24

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8876142 [details]
Bug 1282981 - Implement management.get and getAll,

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

Comment 25

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8876142 [details]
Bug 1282981 - Implement management.get and getAll,

https://reviewboard.mozilla.org/r/147560/#review151962

Also ditto on the permission string.

::: toolkit/components/extensions/ext-management.js:92
(Diff revision 2)
>    return extInfo;
>  }
>  
>  const listenerMap = new WeakMap();
> -// Some management APIs are intentionally limited.
> -const allowedTypes = ["theme"];
> +// Some management APIs are intentionally limited to themes.
> +const themeType = "theme";

If we're allowing get/getAll to actually access any, I see no reason to limit the events to themes.  We should just set allowedTypes = [theme,extension].

::: toolkit/components/extensions/ext-management.js:212
(Diff revision 2)
>          async setEnabled(id, enabled) {
>            let addon = await AddonManager.getAddonByID(id);
>            if (!addon) {
>              throw new ExtensionError(`No such addon ${id}`);
>            }
> -          if (!allowedTypes.includes(addon.type)) {
> +          if (addon.type !== themeType) {

Is installType available?  If so, I'm wondering if we should not allow changing enabled state of installType=="admin".  If "admin" is not used, we should have a followup bug, I think system addons should have "admin".

::: toolkit/components/extensions/test/browser/browser_ext_management_themes.js:62
(Diff revision 2)
> +      let themes = addons.filter(addon => addon.type === "theme");
>        // We get the 3 built-in themes plus the lwt and our addon.
> -      browser.test.assertEq(5, addons.length, "got expected addons");
> +      browser.test.assertEq(5, themes.length, "got expected addons");
> +      // We should also get our test extension.
> +      let testExtension = addons.find(addon => { return addon.id === TEST_ID; });
> +      browser.test.assertEq(TEST_ID, testExtension.name,

This is confusing until you read further down.  Why not just assertTrue(!!testExtension)?

Comment 26

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8876142 [details]
Bug 1282981 - Implement management.get and getAll,

https://reviewboard.mozilla.org/r/147560/#review151976

Comment 27

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #25)

> >  const listenerMap = new WeakMap();
> > -// Some management APIs are intentionally limited.
> > -const allowedTypes = ["theme"];
> > +// Some management APIs are intentionally limited to themes.
> > +const themeType = "theme";
> 
> If we're allowing get/getAll to actually access any, I see no reason to
> limit the events to themes.  We should just set allowedTypes =
> [theme,extension].

Just a further comment on this.  If a supported use case is to warn users about incompatible addons per comment 12, using the events to know that one is later installed is important as well.

Comment 28

[Bob Silverberg [:bsilverberg]]

(In reply to Andrew Swan [:aswan] from comment #23)
> sorry for the drive by but this patch should also include a permissions
> string for the management permissions

I wonder what this should say? My initial thought is something along the lines of "Manage add-ons and themes" (or is "themes" redundant if we're saying "add-ons"?), but this API is actually limited to just reading information about and monitoring add-ons in general. It only allows modification to themes, and on in terms of enabling/disabling them. So is "Manage add-ons and themes" too broad, or does it sound okay?

Comment 29

[Bob Silverberg [:bsilverberg]]

My comment above should read "...and only in terms of enabling/disabling them."

Comment 30

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8876142 [details]
Bug 1282981 - Implement management.get and getAll,

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

Comment 31

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8876142 [details]
Bug 1282981 - Implement management.get and getAll,

https://reviewboard.mozilla.org/r/147560/#review151962

I added a proposed permissions string into this patch, but I'm not sure if it is a good string.

> Is installType available?  If so, I'm wondering if we should not allow changing enabled state of installType=="admin".  If "admin" is not used, we should have a followup bug, I think system addons should have "admin".

`installType` is something we calculate in ext-management.js, and it doesn't currently generate a value of "admin". An addon can have a `isSystem` property and if that is set to "true" then we set `installType` to "other". I think `isSystem` corresponds to a system add-on, which you are referring to above as "admin", so we could use that as a test, and prevent changes to enabled for any addon where `isSystem` is "true". Looking at the various themes that are installed while running the test, none of them have an installType of "other". The ones that seem to be pre-packaged, such as "[e-mail]@personas.mozilla.org" have an installType of "normal", so I wonder if "system themes" is even something that will ever exist, and if not, is a check for `isSystem` neccesary? I have added the change to support that to my latest commit, but please consider what I've written above in terms of whether this actually a change we want/need.

Comment 32

[Andrew Swan [:aswan]]

(In reply to Bob Silverberg [:bsilverberg] from comment #28)
> (In reply to Andrew Swan [:aswan] from comment #23)
> > sorry for the drive by but this patch should also include a permissions
> > string for the management permissions
> 
> I wonder what this should say? My initial thought is something along the
> lines of "Manage add-ons and themes" (or is "themes" redundant if we're
> saying "add-ons"?), but this API is actually limited to just reading
> information about and monitoring add-ons in general. It only allows
> modification to themes, and on in terms of enabling/disabling them. So is
> "Manage add-ons and themes" too broad, or does it sound okay?

By making a single api read-only for extensions and read/write for themes, we have made it difficult to describe concisely.
I would avoid a work like "manage" since isn't very precise.  But over to Scott...

Comment 33

[Andrew Swan [:aswan]]

(In reply to Bob Silverberg [:bsilverberg] from comment #31)
> Comment on attachment 8876142 [details]
> Bug 1282981 - Implement management.get and getAll,
> 
> https://reviewboard.mozilla.org/r/147560/#review151962
> 
> I added a proposed permissions string into this patch, but I'm not sure if
> it is a good string.
> 
> > Is installType available?  If so, I'm wondering if we should not allow changing enabled state of installType=="admin".  If "admin" is not used, we should have a followup bug, I think system addons should have "admin".
> 
> `installType` is something we calculate in ext-management.js, and it doesn't
> currently generate a value of "admin". An addon can have a `isSystem`
> property and if that is set to "true" then we set `installType` to "other".
> I think `isSystem` corresponds to a system add-on, which you are referring
> to above as "admin", so we could use that as a test, and prevent changes to
> enabled for any addon where `isSystem` is "true". Looking at the various
> themes that are installed while running the test, none of them have an
> installType of "other". The ones that seem to be pre-packaged, such as
> "[e-mail]@personas.mozilla.org" have an installType
> of "normal", so I wonder if "system themes" is even something that will ever
> exist, and if not, is a check for `isSystem` neccesary? I have added the
> change to support that to my latest commit, but please consider what I've
> written above in terms of whether this actually a change we want/need.

I haven't looked at the patch but we don't display system add-ons in about:addons.  Looking over the use cases that motivated this, none of them seem to require having system add-ons returned and some appear like they would actually be better not seeing them.

Comment 34

[Bob Silverberg [:bsilverberg]]

(In reply to Andrew Swan [:aswan] from comment #33)
> 
> I haven't looked at the patch but we don't display system add-ons in
> about:addons.  Looking over the use cases that motivated this, none of them
> seem to require having system add-ons returned and some appear like they
> would actually be better not seeing them.

Interesting point. Yes, it might be better to filter out system add-ons from the list of add-ons that get returned. It probably makes sense to apply that to the events as well, and not emit an event if the add-on in question is a system add-on. What do you think, Shane?

Comment 35

[Shane Caraveo (:mixedpuppy)]

Yeah, I think that's a good solution.

Just to comment on themes not being system addons, addons can distributed in Firefox without them being system.

Comment 36

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8876142 [details]
Bug 1282981 - Implement management.get and getAll,

https://reviewboard.mozilla.org/r/147560/#review152468

::: browser/locales/en-US/chrome/browser/browser.properties:104
(Diff revision 3)
>  webextPerms.description.clipboardRead=Get data from the clipboard
>  webextPerms.description.clipboardWrite=Input data to the clipboard
>  webextPerms.description.downloads=Download files and read and modify the browser’s download history
>  webextPerms.description.geolocation=Access your location
>  webextPerms.description.history=Access browsing history
> +webextPerms.description.management=Manage add-ons and themes

Use extension, not add-on

::: toolkit/components/extensions/ext-management.js:212
(Diff revision 3)
>          async setEnabled(id, enabled) {
>            let addon = await AddonManager.getAddonByID(id);
>            if (!addon) {
>              throw new ExtensionError(`No such addon ${id}`);
>            }
> -          if (!allowedTypes.includes(addon.type)) {
> +          if (addon.type !== "theme") {

I think we should leave this checking allowedTypes, log who is disabling what.

::: toolkit/components/extensions/ext-management.js:215
(Diff revision 3)
>              throw new ExtensionError(`No such addon ${id}`);
>            }
> -          if (!allowedTypes.includes(addon.type)) {
> +          if (addon.type !== "theme") {
>              throw new ExtensionError("setEnabled applies only to theme addons");
>            }
> +          if (addon.isSystem) {

Per bug discussion, lets filter out system addons everywhere.

Comment 37

[Bob Silverberg [:bsilverberg]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #36)
> Comment on attachment 8876142 [details]
> Bug 1282981 - Implement management.get and getAll,
> 
> https://reviewboard.mozilla.org/r/147560/#review152468
> 
> ::: toolkit/components/extensions/ext-management.js:212
> (Diff revision 3)
> >          async setEnabled(id, enabled) {
> >            let addon = await AddonManager.getAddonByID(id);
> >            if (!addon) {
> >              throw new ExtensionError(`No such addon ${id}`);
> >            }
> > -          if (!allowedTypes.includes(addon.type)) {
> > +          if (addon.type !== "theme") {
> 
> I think we should leave this checking allowedTypes, log who is disabling
> what.
>

I think we decided long ago that we didn't want to allow extensions to disable other extensions. We made an explicit exception for themes, but I don't think we now want to open it up so any extension can disable any other extension, which is what I believe you are suggesting. I'll check with Andy to see what he thinks.

Comment 38

[Andy McKay]

> I think we decided long ago that we didn't want to allow extensions to
> disable other extensions. We made an explicit exception for themes, but I
> don't think we now want to open it up so any extension can disable any other
> extension, which is what I believe you are suggesting. I'll check with Andy
> to see what he thinks.

Most definitely: extensions should not be able to disable other extensions. We've made an exception for themse.

Comment 39

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8876142 [details]
Bug 1282981 - Implement management.get and getAll,

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

Comment 40

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8876142 [details]
Bug 1282981 - Implement management.get and getAll,

https://reviewboard.mozilla.org/r/147560/#review152468

> Use extension, not add-on

I made this change, but I think the string is still very poor. Let's wait to see what Scott suggests.

> I think we should leave this checking allowedTypes, log who is disabling what.

As per Andy's response, we do not want to allow extensions to disable other extensions, so I think this code needs to stay as is.

Comment 41

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8876142 [details]
Bug 1282981 - Implement management.get and getAll,

https://reviewboard.mozilla.org/r/147560/#review152588

Comment 42

[sdevaney]

Yea I'm not sure we can achieve poetry with this message, but do one of these two options ring true?:

Monitors extension usage and manages themes

Reads extensions and manages themes

Comment 43

[Bob Silverberg [:bsilverberg]]

(In reply to sdevaney from comment #42)
> Yea I'm not sure we can achieve poetry with this message, but do one of
> these two options ring true?:
> 
> Monitors extension usage and manages themes
> 
> Reads extensions and manages themes

Thanks Scott. Yeah, neither of those are beautiful either, but not sure what would be better. Andrew suggested that he didn't like using the term "manage" back in comment #32, but I don't have any better ideas.

I'm going to go with "Monitors extension usage and manages themes" for now.

Comment 44

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8876142 [details]
Bug 1282981 - Implement management.get and getAll,

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

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe3ed659f568
Implement management.get and getAll, r=mixedpuppy

Comment 46

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/fe3ed659f568

Comment 47

[Will Bamberg [:wbamberg]]

I've updated the compat data for get() and getAll():
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/management/get
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/management/getAll

I'm marking this as dev-doc-complete, but let me know if we need anything else here.

Comment 48

[Andy McKay]

Does the compat table need to be updated. getAll shows Firefox 55 and get shows it is not implemented.

Comment 49

[Andy McKay]

Oh getAll for themes was implemented in bug 1336908. This bugs allows getAll to work for any extension. I think the getAll page needs this clause removing: "1. Only extensions whose 'type' is 'theme' are returned."

Comment 50

[Will Bamberg [:wbamberg]]

Sorry, I forgot to refresh the page, so the macros didn't get updated.

Comment 51

[Andy McKay]

They both look great now, thanks Will!