Closed Bug 1429123 Opened 7 years ago Closed 6 years ago

Policy: prevent access to the Add-ons Manager (about:addons)

Categories

(Firefox :: Enterprise Policies, enhancement, P1)

Product:
Firefox
Component:
Enterprise Policies
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- verified

People

(Reporter: Felipe, Assigned: bytesized)

References

Details

Attachments

(1 file)

Bug 1429123 - Create an enterprise policy to prevent access to the Add-ons manager (about:addons)
59 bytes, text/x-review-board-request
bzbarsky
: review+
Felipe
: review+
Details 
This can be built on top of bug 1421707, but this will require extra changes, as there are UI entry points to the add-ons manager. That I know of:
  - Customization button in the toolbar palette
  - menu Tools -> Add-ons
  - Shortcut Key  Cmd + Shift + A

and maybe more?

Maybe, if the error notice displayed by bug 1421707 is nice enough, it's acceptable to not remove the UI entries. We should probably talk to someone about this.
Assignee: nobody → ksteuber
Depends on: 1421707
Attachment #8947673 - Flags: review?(felipc)
Attachment #8947673 - Flags: review?(bzbarsky)
Comment on attachment 8947673 [details]
Bug 1429123 - Create an enterprise policy to prevent access to the Add-ons manager (about:addons)

https://reviewboard.mozilla.org/r/217400/#review223140


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/enterprisepolicies/Policies.jsm:14
(Diff revision 1)
>  const Cr = Components.results;
>  const Cu = Components.utils;
>  
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
>  ChromeUtils.import("resource://gre/modules/Services.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "CustomizableUI",

Error: Please use ChromeUtils.defineModuleGetter instead of XPCOMUtils.defineLazyModuleGetter [eslint: mozilla/use-chromeutils-import]
Comment on attachment 8947673 [details]
Bug 1429123 - Create an enterprise policy to prevent access to the Add-ons manager (about:addons)

https://reviewboard.mozilla.org/r/217400/#review223142


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/enterprisepolicies/Policies.jsm:93
(Diff revision 2)
>  
> +  "BlockAboutAddons": {
> +    onBeforeUIStartup(manager, param) {
> +      if (param == true) {
> +        manager.disallowFeature("about:addons", true);
> +        CustomizableUI.destroyWidget("add-ons-button");

Error: 'CustomizableUI' is not defined. [eslint: no-undef]
Comment on attachment 8947673 [details]
Bug 1429123 - Create an enterprise policy to prevent access to the Add-ons manager (about:addons)

https://reviewboard.mozilla.org/r/217400/#review223148

::: browser/components/enterprisepolicies/Policies.jsm:93
(Diff revision 2)
>  
> +  "BlockAboutAddons": {
> +    onBeforeUIStartup(manager, param) {
> +      if (param == true) {
> +        manager.disallowFeature("about:addons", true);
> +        CustomizableUI.destroyWidget("add-ons-button");

Ah, I fell into the same problem today too. The name of the new function in ChromeUtils is `defineModuleGetter`, not `defineLazyModuleGetter`
Comment on attachment 8947673 [details]
Bug 1429123 - Create an enterprise policy to prevent access to the Add-ons manager (about:addons)

https://reviewboard.mozilla.org/r/217400/#review223370

r=me
Attachment #8947673 - Flags: review?(bzbarsky) → review+
Comment on attachment 8947673 [details]
Bug 1429123 - Create an enterprise policy to prevent access to the Add-ons manager (about:addons)

https://reviewboard.mozilla.org/r/217400/#review223376

Can you check with someone from the front-end if CustomizableUI.destroyWidget is the right thing to do here?

::: browser/components/enterprisepolicies/schemas/policies-schema.json:37
(Diff revision 2)
> +    "BlockAboutAddons": {
> +      "description": "Block access to the Add-ons Mananger (about:addons).",
> +      "first_available": "60.0",
> +
> +      "type": "boolean",
> +      "enum": [true]
> +    },
> +

Let's try to keep similar policies close together, so add this nearby the BlockAboutConfig one

::: browser/components/enterprisepolicies/tests/browser/browser.ini:19
(Diff revision 2)
>  [browser_policy_default_browser_check.js]
>  [browser_policy_display_menu.js]
>  [browser_policy_block_about_config.js]
>  [browser_policy_display_bookmarks.js]
>  [browser_policy_block_set_desktop_background.js]
> +[browser_policy_block_about_addons.js]

Please keep this list of tests sorted alphabetically
Attachment #8947673 - Flags: review?(felipc) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s d67f23af27bb061f0126bf384bfa8909bdee4d4b -d 384ba949a896: rebasing 445704:d67f23af27bb "Bug 1429123 - Create an enterprise policy to prevent access to the Add-ons manager (about:addons) r=bz,Felipe" (tip)
merging browser/base/content/browser-menubar.inc
merging browser/base/content/browser.js
merging browser/base/content/utilityOverlay.js
merging browser/components/enterprisepolicies/Policies.jsm
merging browser/components/enterprisepolicies/schemas/policies-schema.json
merging browser/components/enterprisepolicies/tests/browser/browser.ini
warning: conflicts while merging browser/components/enterprisepolicies/tests/browser/browser.ini! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Oop. I just found a little mistake. "Hamburger Menu -> Addons" is disabled only after the Tools menu is opened. I need to fix that...
Comment on attachment 8947673 [details]
Bug 1429123 - Create an enterprise policy to prevent access to the Add-ons manager (about:addons)

Gijs - I think that you are probably the right person to review the changes that I just made. Could you take a look at this interdiff and let me know if it looks ok?

  https://reviewboard.mozilla.org/r/217400/diff/5-6/
Attachment #8947673 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8947673 [details]
Bug 1429123 - Create an enterprise policy to prevent access to the Add-ons manager (about:addons)

https://reviewboard.mozilla.org/r/217400/#review223884

I don't think the button stuff is quite right. I'm also confused about whether we aim to hide the items or disable them - right now the patch does different things for different items, but I don't see this discussed in the bug.

Otherwise... on the one hand, waiting with disabling things until popupshowing is nice, but on the other hand, now we set it to disabled every time you open the menu, which also seems odd and a bit wasteful.

I think I would lean towards doing this stuff in an idle callback off browser-delayed-startup-finished. Then you can just set the command to disabled and/or hidden, and update all the items to use `command="..."` instead of `oncommand`, which should in theory ensure every single button/menuitem gets updated when the command is updated, which will be only once per window, when the window has just opened.

For the toolbarbutton, you can accomplish this by using an `onCreated` callback in CustomizableWidgets.jsm and in there set the `command` attribute on the `aNode` that you get passed, and removing the onCommand JS handler.

Finally, it looks like there's other places that this misses, e.g. https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-pageActions.js#782-785 . Might be useful to just audit the BrowserOpenAddonsMgr callsites in DXR/searchfox.

::: browser/base/content/utilityOverlay.js:844
(Diff revision 6)
>    }
>  }
>  
> +function buildToolsMenu() {
> +  if (!Services.policies.isAllowed("about:addons")) {
> +    document.getElementById("Tools:Addons").setAttribute("disabled", true);

You set the item as hidden in the hamburger menu, but not in here. Is that an intentional decision?

::: browser/components/enterprisepolicies/Policies.jsm:93
(Diff revision 6)
>  
> +  "BlockAboutAddons": {
> +    onBeforeUIStartup(manager, param) {
> +      if (param == true) {
> +        manager.disallowFeature("about:addons", true);
> +        CustomizableUI.destroyWidget("add-ons-button");

Loading CUI so early (but only if this policy is present) is potentially problematic.

Additionally, destroying the widget seems like the wrong thing to do. It would be more consistent to just make it disabled, like the menu item, and/or hide it.
Attachment #8947673 - Flags: review?(gijskruitbosch+bugs) → review-
... though note that some of the BrowserOpenAddonsMgr sites may still need the popupshowing treatment because they can't use the command element directly as they pass a view to open within the add-ons manager... :-(
(In reply to Kirk Steuber [:bytesized] from comment #12)
> Oop. I just found a little mistake. "Hamburger Menu -> Addons" is disabled
> only after the Tools menu is opened. I need to fix that...

And in fact, the patch as-is presumably doesn't disable the command until either menu is shown? This would mean the shortcut would still work...
(In reply to :Gijs from comment #15)
> I'm also confused about
> whether we aim to hide the items or disable them - right now the patch does
> different things for different items, but I don't see this discussed in the
> bug.

I believe that we do want to disable the Tools menu item but hide the entry in the Hamburger menu. Felipe, can you confirm that this is what we want to do?
Flags: needinfo?(felipc)
(In reply to Kirk Steuber [:bytesized] from comment #18)
> (In reply to :Gijs from comment #15)
> > I'm also confused about
> > whether we aim to hide the items or disable them - right now the patch does
> > different things for different items, but I don't see this discussed in the
> > bug.
> 
> I believe that we do want to disable the Tools menu item but hide the entry
> in the Hamburger menu. Felipe, can you confirm that this is what we want to
> do?

We haven't had an official direction on this, so we've been deciding on a case-by-case basis. Most things have been disabled, but when talking to Kirk the other day we thought that having a permanently disabled item in the main menu would be odd, since nothing else there can be disabled (afaict).

Another example of hiding is the policy to disable Firefox Screenshots. It removes the "Take a Screenshot" on the Page Actions menu. (indirectly, by the fact that it disables the entire system add-on)

Amin, what are your thoughts between disabling vs hiding items in menus for when policies are blocking certain features?
Flags: needinfo?(felipc) → needinfo?(aalhazwani)
(In reply to :Felipe Gomes (needinfo me!) from comment #19)
> Amin, what are your thoughts between disabling vs hiding items in menus for
> when policies are blocking certain features?

And a third option that exists specifically in this case is letting about:addons to be opened anyways, since opening it will show the "This page has been blocked" message.
(In reply to :Gijs from comment #15)
> Otherwise... on the one hand, waiting with disabling things until
> popupshowing is nice, but on the other hand, now we set it to disabled every
> time you open the menu, which also seems odd and a bit wasteful.
> 
> I think I would lean towards doing this stuff in an idle callback off
> browser-delayed-startup-finished. Then you can just set the command to
> disabled and/or hidden, and update all the items to use `command="..."`
> instead of `oncommand`, which should in theory ensure every single
> button/menuitem gets updated when the command is updated, which will be only
> once per window, when the window has just opened.

I can make this change if you would like by simply setting up an observer in Policy.jsm. It does, however, make the testing kinda nasty. I guess it would work to use this code in testing to make sure that the policy code has run:
  await new Promise(resolve => newWin.requestIdleCallback(resolve));
Is that what I should do?

> For the toolbarbutton, you can accomplish this by using an `onCreated`
> callback in CustomizableWidgets.jsm and in there set the `command` attribute
> on the `aNode` that you get passed, and removing the onCommand JS handler.
...
> Additionally, destroying the widget seems like the wrong thing to do. It
> would be more consistent to just make it disabled, like the menu item,
> and/or hide it.

I don't really see the reason for this. Why not destroy the widget? The documentation for the function doesn't seem to indicate any problem, and those that I have talked to about this approach have indicated that this is the way to go.

(In reply to :Gijs from comment #16)
> ... though note that some of the BrowserOpenAddonsMgr sites may still need
> the popupshowing treatment because they can't use the command element
> directly as they pass a view to open within the add-ons manager... :-(

So... do I need to fully implement both solutions? That doesn't seem optimal.

(In reply to :Gijs from comment #17)
> (In reply to Kirk Steuber [:bytesized] from comment #12)
> > Oop. I just found a little mistake. "Hamburger Menu -> Addons" is disabled
> > only after the Tools menu is opened. I need to fix that...
> 
> And in fact, the patch as-is presumably doesn't disable the command until
> either menu is shown? This would mean the shortcut would still work...

Incorrect. The early exit that I added to the |BrowserOpenAddonsMgr| function prevents this.
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(jmathies)
(In reply to Kirk Steuber [:bytesized] from comment #21)
> (In reply to :Gijs from comment #15)
> > Otherwise... on the one hand, waiting with disabling things until
> > popupshowing is nice, but on the other hand, now we set it to disabled every
> > time you open the menu, which also seems odd and a bit wasteful.
> > 
> > I think I would lean towards doing this stuff in an idle callback off
> > browser-delayed-startup-finished. Then you can just set the command to
> > disabled and/or hidden, and update all the items to use `command="..."`
> > instead of `oncommand`, which should in theory ensure every single
> > button/menuitem gets updated when the command is updated, which will be only
> > once per window, when the window has just opened.
> 
> I can make this change if you would like by simply setting up an observer in
> Policy.jsm. It does, however, make the testing kinda nasty. I guess it would
> work to use this code in testing to make sure that the policy code has run:
>   await new Promise(resolve => newWin.requestIdleCallback(resolve));
> Is that what I should do?

For the testing, you could just wait for the disabled attribute to be set, see BrowserTestUtils.waitForAttribute.

 > > For the toolbarbutton, you can accomplish this by using an `onCreated`
> > callback in CustomizableWidgets.jsm and in there set the `command` attribute
> > on the `aNode` that you get passed, and removing the onCommand JS handler.
> ...
> > Additionally, destroying the widget seems like the wrong thing to do. It
> > would be more consistent to just make it disabled, like the menu item,
> > and/or hide it.
> 
> I don't really see the reason for this. Why not destroy the widget? The
> documentation for the function doesn't seem to indicate any problem, and
> those that I have talked to about this approach have indicated that this is
> the way to go.

Maybe I'm overly concerned. `destroyWidget` was primarily written to be used by add-ons on uninstall/upgrade. Using it on a builtin widget may cause issues. But I'm also worried about the early load of CUI and hard-to-debug/notice race conditions related to when we load that module. Do we guarantee, for instance, that at the point where the policy manager makes this calls, prefs are available and have the user-defined values? Or do we load the policy code sooner than that? Or might we in the future, depending on what policies are set/required (cf. bug 1429164).

In general though, given that help (sumo), other documentation around the web etc. will refer to UI elements, and removing them outright would be more confusing than disabling them, I think disabling them is likely better than removing them altogether, so then `destroyWidget` isn't going to be what you want anyway.

> (In reply to :Gijs from comment #16)
> > ... though note that some of the BrowserOpenAddonsMgr sites may still need
> > the popupshowing treatment because they can't use the command element
> > directly as they pass a view to open within the add-ons manager... :-(
> 
> So... do I need to fully implement both solutions? That doesn't seem optimal.

No, I think you should implement disabling the XUL <command> when a browser window opens (which should auto-disable the Tools and hamburger menuitems, and the button - if they don't already use command=... then they *should* be using that).

Separately, only for any other entrypoints, you'll need the popupshowing solution. That doesn't really seem any worse than what you have right now - right now you have 3 bits of code that disable 3 different entrypoints. What I'm suggesting is replacing those 3 bits of code with 1 bit of code (and, where necessary, updating the 3 entrypoints to actually defer to the <command> element directly). Then there's 1 bit of code managing those 3 entrypoints.

Any items that call BrowserOpenAddonsMgr without arguments can just be converted to also use the <command> directly, and will thus also benefit from that 1 bit of code.

Any other items (that pass a view to BrowserOpenAddonsMgr) will need custom code -- but they would also need that custom code if you used the popupshowing handlers to update all the other bits of UI. You're avoiding adding lots of those handlers for the sole purpose of disabling the command from all of them.

> (In reply to :Gijs from comment #17)
> > (In reply to Kirk Steuber [:bytesized] from comment #12)
> > > Oop. I just found a little mistake. "Hamburger Menu -> Addons" is disabled
> > > only after the Tools menu is opened. I need to fix that...
> > 
> > And in fact, the patch as-is presumably doesn't disable the command until
> > either menu is shown? This would mean the shortcut would still work...
> 
> Incorrect. The early exit that I added to the |BrowserOpenAddonsMgr|
> function prevents this.

Ah right. But still, the point is that the command isn't disabled, which will mean that e.g. on mac, the menu will still highlight if you press the shortcut (which is hooked up directly to the command) and then no-op, which would be confusing to users (generally the highlight on the menubar implies "I did the think you pressed a shortcut for") and isn't correct behaviour. This is another reason to disable the <command> when the window loads, instead of only when one of the popups that uses it shows up (which it will never do if you use the shortcut).
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #22)
> No, I think you should implement disabling the XUL <command> when a browser
> window opens (which should auto-disable the Tools and hamburger menuitems,
> and the button - if they don't already use command=... then they *should* be
> using that).

FWIW, Kirk: for this, instead of having to setup an observer in Policies.jsm, there's an easier way to do this. Since there's already the disallowFeature("about:addons") being set up, you can directly add some code in _schedulePerWindowIdleTasks that uses the .isAllowed() call to disable the command on new windows

https://searchfox.org/mozilla-central/rev/84cea84b12145d752e50ddca6be5462c38510e35/browser/base/content/browser.js#1689
> Amin, what are your thoughts between disabling vs hiding items in menus for when policies are blocking certain features?

In preferences for example we display disabled options when users can re-enable them. For example a disabled nested checkbox that can be re-enabled with the parent checkbox. Or a disabled button in a dialog that can be re-enabled once something is selected in said dialog. 

We don't have much real estate in a menu to further display information on why a certain menu item is disabled and I wouldn't let disabled menu items be clickable and link somewhere else where we could give more information. 

So, if a menu item points to an about:page I would not disable it and I would take advantage of the real estate of said about:page to explain why people might not be able to access about:addons or about:something. If the menu item is not pointing to an about:page I would simply hide it. What do you think?

If we implement the badge in the tabstrip people will have at least one way to understand why their Firefox looks different from the one they use outside work.
Flags: needinfo?(aalhazwani)
Attachment #8947673 - Flags: review?(gijskruitbosch+bugs)
Removal of UI entry points reversed. This should also address all other concerns with this patch, as the related code is no longer part of the patch.
Flags: needinfo?(jmathies)
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3fef64d3e7a9
Create an enterprise policy to prevent access to the Add-ons manager (about:addons) r=bz,Felipe
https://hg.mozilla.org/mozilla-central/rev/3fef64d3e7a9
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
We tested the disabling ‘about:addons’ policy using JSON file. We verified this manually as fixed.  
When "BlockAboutAddons" policy is in use, about:addons becomes blocked. It also provides information to the user when blocked.

Test steps and runs are available here: https://testrail.stage.mozaws.net/index.php?/plans/view/7734

The bug will also be retested with ADMX files when it is ready for testing.
We retested this on beta builds with ADM and JSON policy formats and it is verified as fixed.

Test cases and runs are here- https://testrail.stage.mozaws.net/index.php?/plans/view/8760
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: