Closed Bug 1449021 Opened 7 years ago Closed 7 years ago

Policy: Allow addons to be installed and uninstalled.

Categories

(Firefox :: Enterprise Policies, defect)

Product:
Firefox
Component:
Enterprise Policies
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox60 + verified
firefox61 --- verified

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

Attachments

(2 files)

Bug 1449021 - Policy for install/uninstalling add-ons.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Rollup patch for beta, r=felipe
13.05 KB, patch
Felipe
: review+
RyanVM
: approval-mozilla-beta+
Details | Diff | Splinter Review 
This bug is for a policy that allows extensions to be installed and uninstalled.

You can also specify that extensions should not be able to be removed or disabled.

This policy will be ESR only.
Felipe:

This is a first pass. I still need to add tests.

Based on our conversation, I expanded this a lot and I am putting disabling into the "Extensions" policy.

I also added the ability to uninstall add-ons (which has been requested).
Comment on attachment 8962507 [details]
Bug 1449021 - Policy for install/uninstalling add-ons.

https://reviewboard.mozilla.org/r/231320/#review236788


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

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


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


::: browser/components/enterprisepolicies/Policies.jsm:296
(Diff revision 1)
> +            if (!url.includes("://")) {
> +              let xpiFile = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
> +              try {
> +                xpiFile.initWithPath(location);
> +              } catch (e) {
> +                log.error(`Invalid extension path location - ${location}`)

Error: Missing semicolon. [eslint: semi]
Comment on attachment 8962507 [details]
Bug 1449021 - Policy for install/uninstalling add-ons.

https://reviewboard.mozilla.org/r/231320/#review237130

::: browser/components/enterprisepolicies/Policies.jsm:286
(Diff revision 1)
> +    onBeforeUIStartup(manager, extensions) {
> +      if ("Install" in extensions) {
> +        runOncePerModification("extensions", JSON.stringify(extensions.Install), () => {

just to keep the style with the other policies, please name the second parameter as "param".

and the ifs as: `if (param.Install) {`

::: browser/components/enterprisepolicies/Policies.jsm:286
(Diff revision 1)
> +    onBeforeUIStartup(manager, extensions) {
> +      if ("Install" in extensions) {
> +        runOncePerModification("extensions", JSON.stringify(extensions.Install), () => {

just to match the style with the other policies, please name the second parameter as "param".

and the ifs as: `if (param.Install) {`

::: browser/components/enterprisepolicies/Policies.jsm:290
(Diff revision 1)
> +            let url = location;
> +            if (!url.includes("://")) {
> +              let xpiFile = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
> +              try {
> +                xpiFile.initWithPath(location);
> +              } catch (e) {

I think this is right, but it was a bit confusing to me. I'd rewrite it as:

```
let url;
if (location.includes("://")) {
  // Assume location is an URI
  url = location;
} else {
  // Assume location is a file path
  ...
  url = ..spec
}

::: browser/components/enterprisepolicies/Policies.jsm:301
(Diff revision 1)
> +            AddonManager.getInstallForURL(url, (install) => {
> +              install.install();
> +            }, "application/x-xpinstall");

I'll leave a proper review of this usage for someone from the Addons team, but I will just mention it would be nice to use the listeners available on this install object to log error or successes.

::: browser/components/enterprisepolicies/Policies.jsm:320
(Diff revision 1)
> +          });
> +        });
> +      }
> +      if ("Locked" in extensions) {
> +        for (let ID of extensions.Locked) {
> +          manager.disallowFeature("extension:" + ID);

Since what is disallowed is disabling/uninstalling the extension (and not the extension itself), I'd change this prefix to "modify-extension:"

::: browser/components/enterprisepolicies/schemas/policies-schema.json:226
(Diff revision 1)
>        },
>        "required": ["Value"]
>      },
>  
> +    "Extensions": {
> +      "description": "Force installs extensions. Can be URL or path. If ID is specified, extension cannot be uninstalled or disabled. ID can be specified without URL.",

I couldn't understand the right types by just reading this (I had to read the code). So let's improve this description by saying that the Install options takes URL or file paths as parameters, while the Uninstall and Locked options take extension IDs
Attachment #8962507 - Flags: review?(felipc) → review+
Comment on attachment 8962507 [details]
Bug 1449021 - Policy for install/uninstalling add-ons.

https://reviewboard.mozilla.org/r/231320/#review237218

::: browser/components/enterprisepolicies/Policies.jsm:314
(Diff revision 3)
> +                },
> +                onInstallFailed: () => {
> +                  log.error(`Installation failed - ${location}`);
> +                },
> +                onInstallEnded: () => {
> +                  log.debug("Installation succeeded");

Might as well log ${location} here too

::: browser/components/enterprisepolicies/tests/browser/browser_policy_extensions.js:8
(Diff revision 3)
> +  await setupPolicyEngineWithJson({
> +                                    "policies": {
> +                                      "Extensions": {
> +                                        "Install": [
> +                                          "http://mochi.test:8888/browser/browser/components/enterprisepolicies/tests/browser/policytest.xpi"
> +                                        ]
> +                                      }
> +                                    }
> +                                  });

nit: indent the json object like this: https://searchfox.org/mozilla-central/source/browser/components/enterprisepolicies/tests/browser/browser_policy_display_menu.js#8

::: browser/components/enterprisepolicies/tests/browser/browser_policy_extensions.js:18
(Diff revision 3)
> +                                        ]
> +                                      }
> +                                    }
> +                                  });
> +});
> +

Could you also add a test for the Locked property?

It is easy to do. See how Kirk is opening the Add-ons Manager here and checking that the dropdown for Flash is Disabled:

https://reviewboard.mozilla.org/r/229836/diff/2/

test_flash_status in the browser_policy_disable_flash_plugin.js file
Comment on attachment 8962507 [details]
Bug 1449021 - Policy for install/uninstalling add-ons.

Andrew:

Can you provide some feedback on this just to validate? It touch the add-ons manager in two small places to disable buttons/menus.
Attachment #8962507 - Flags: feedback?(aswan)
Comment on attachment 8962507 [details]
Bug 1449021 - Policy for install/uninstalling add-ons.

https://reviewboard.mozilla.org/r/231320/#review237328

I don't know that much about how the policy engine is implemented.  Is it the case that `Services.policies` is a functional service even if I'm not using the policy engine feature?  And that the calls to `isAllowed()` will all return `true` in that case?
(In reply to Andrew Swan [:aswan] from comment #11)
> Comment on attachment 8962507 [details]
> Bug 1449021 - Policy for install/uninstalling add-ons.
> 
> https://reviewboard.mozilla.org/r/231320/#review237328
> 
> I don't know that much about how the policy engine is implemented.  Is it
> the case that `Services.policies` is a functional service even if I'm not
> using the policy engine feature?  And that the calls to `isAllowed()` will
> all return `true` in that case?

Yep, exactly


(although Services.policies don't exist in Thunderbird/Fennec, so any code not under browser/ first needs to check `if (Services.policies)`. That reminded me I forgot to notice this in the review)
Comment on attachment 8962507 [details]
Bug 1449021 - Policy for install/uninstalling add-ons.

https://reviewboard.mozilla.org/r/231320/#review237334

There are various places in the UI where extensions can be disabled, you might have some whack-a-mole trying to get them all.  (For example, parts of about:preferences where we show that an extension is controlling a preference or the doorhangers on extension-controlled pages)
In theory though, they should all look at whether an addon object's permissions property includes PERM_CAN_DISABLE (or PERM_CAN_UNINSTALL) or whatever.
So, adding the policy logic here:
https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/toolkit/mozapps/extensions/internal/XPIProvider.jsm#5076-5107
should at least cover the cases you cover in this patch, and give you some hope of covering the other cases.
Comment on attachment 8962507 [details]
Bug 1449021 - Policy for install/uninstalling add-ons.

https://reviewboard.mozilla.org/r/231320/#review237334

Brilliant. New patch applied. And as you said, it works for my cases.
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 7a9f8dcb4a06531164031b7d755f00cbbd1df42a -d 21db86504db5: rebasing 454498:7a9f8dcb4a06 "Bug 1449021 - Policy for install/uninstalling add-ons. r=Felipe" (tip)
merging browser/components/enterprisepolicies/Policies.jsm
merging browser/components/enterprisepolicies/schemas/policies-schema.json
merging browser/components/enterprisepolicies/tests/browser/browser.ini
merging toolkit/mozapps/extensions/internal/XPIProvider.jsm
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)
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/160077ddc286
Policy for install/uninstalling add-ons. r=Felipe
[Tracking Requested - why for this release]: Enterprise policy
tracking-firefox60: --- → ?
https://hg.mozilla.org/mozilla-central/rev/160077ddc286
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Depends on: 1449628
Comment on attachment 8962507 [details]
Bug 1449021 - Policy for install/uninstalling add-ons.

Approval Request Comment
[Feature/Bug causing the regression]: Policy for extensions
[User impact if declined]: No policy
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: bug 1449628 (test fix)
[Is the change risky?]: No
[Why is the change risky/not risky?]: Policy only
[String changes made/needed]: None
Attachment #8962507 - Flags: approval-mozilla-beta?
Depends on: 1450097
Comment on attachment 8962507 [details]
Bug 1449021 - Policy for install/uninstalling add-ons.

We should wait until we figure bug 1450097 before uplifting this
Attachment #8962507 - Flags: approval-mozilla-beta?
For beta we should probably just land a rollup patch here that includes all the follow-ups: bug 1450097, bug 1449628 and bug 1449630
Rollup patch for beta. Contains all the follow-ups (bug 1450097, bug 1449628 and bug 1449630)
Attachment #8964303 - Flags: review+
Comment on attachment 8964303 [details] [diff] [review]
Rollup patch for beta, r=felipe

Approval Request Comment
[Feature/Bug causing the regression]: Enterprise Policies
[User impact if declined]: Policy to allow extensions to be installed
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: QA will handle it
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not much
[Why is the change risky/not risky?]: the code is contained to only when this extension is in use, and QA has been verifying it
[String changes made/needed]: none
Attachment #8964303 - Flags: approval-mozilla-beta?
Comment on attachment 8964303 [details] [diff] [review]
Rollup patch for beta, r=felipe

Policy Engine fix needed for ESR60. Approved for 60.0b9.
Attachment #8964303 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1451122
We tested this on Nightly with JSON policy format and it is verified as fixed.
Even though we found bugs, one is verified-fixed and the other one seems not blocking.
With this policy, extensions can be installed, uninstalled or locked.

We will retest this with adm format as well when ready.
Test cases and runs are here- https://testrail.stage.mozaws.net/index.php?/plans/view/8760
We retested this on beta builds[FX60] 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
status-firefox61: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: