Closed Bug 1436851 Opened 6 years ago Closed 6 years ago

Policy: Disable updates to system 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: bytesized, Assigned: bytesized)

References

Details

Attachments

(4 files)

Bug 1436851 - Implement mechanism to disable system addon updates via enterprise policy
59 bytes, text/x-review-board-request
aswan
: review+
rhelmer
: review+
Felipe
: review+
Details
Bug 1436851 - Prevent AddonTestUtils.jsm from overriding a pref value
59 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1436851 - Create an enterprise policy to disable system addon updates
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1436851 - Add test for enterprise policy to disable system addon updates
59 bytes, text/x-review-board-request
rhelmer
: review+
Felipe
: review+
Details 
Bug 1429150 created a policy to disable App updates. This patch will create a policy to disable the other update mechanism: system addon updates.
Attachment #8951464 - Flags: review?(felipc)
Attachment #8951464 - Flags: review?(aswan)
Attachment #8951465 - Flags: review?(aswan)
Attachment #8951466 - Flags: review?(felipc)
Attachment #8951467 - Flags: review?(felipc)
Attachment #8951467 - Flags: review?(aswan)
Comment on attachment 8951464 [details]
Bug 1436851 - Implement mechanism to disable system addon updates via enterprise policy

https://reviewboard.mozilla.org/r/220332/#review226942

::: toolkit/mozapps/extensions/AddonManager.jsm:1340
(Diff revision 1)
>        let appUpdateEnabled = Services.prefs.getBoolPref(PREF_APP_UPDATE_ENABLED) &&
> -                             Services.prefs.getBoolPref(PREF_APP_UPDATE_AUTO);
> +                             Services.prefs.getBoolPref(PREF_APP_UPDATE_AUTO) &&
> +                             !(Services.policies &&
> +                             !Services.policies.isAllowed("SysAddonUpdate"));

This works but is semantically wrong. This variable here is about app ppdate, not system addons. It is just like this because the two are tied together.

I suggest adding creating a new variable, `sysAddonUpdatesAllowed`, and then where `appUpdateEnabled` is used, you do `appUpdateEnabled && sysAddonUpdatesAllowed`
Attachment #8951464 - Flags: review?(felipc)
Comment on attachment 8951465 [details]
Bug 1436851 - Prevent AddonTestUtils.jsm from overriding a pref value

https://reviewboard.mozilla.org/r/220334/#review226944

Just out of curiosity, why was this necessary here?
Comment on attachment 8951466 [details]
Bug 1436851 - Create an enterprise policy to disable system addon updates

https://reviewboard.mozilla.org/r/220336/#review226946

::: browser/components/enterprisepolicies/EnterprisePolicies.js:325
(Diff revision 1)
> +    } catch (ex) {
> +      // The above line will fail in xpcshell tests. This should be handled the
> +      // same way as if the configFile simply does not exist.
> +    }
> +    if (configFile) {
> -    configFile.append(POLICIES_FILENAME);
> +      configFile.append(POLICIES_FILENAME);

you can just move this configFile.append line inside the try block, so this `if (configFile) {` won't be needed

::: browser/components/enterprisepolicies/Policies.jsm:115
(Diff revision 1)
>        }
>      }
>    },
>  
> +  "DisableSysAddonUpdate": {
> +    onBeforeUIStartup(manager, param) {

Probably better to use onBeforeAddons
Attachment #8951466 - Flags: review?(felipc) → review+
Comment on attachment 8951467 [details]
Bug 1436851 - Add test for enterprise policy to disable system addon updates

https://reviewboard.mozilla.org/r/220338/#review226948
Attachment #8951467 - Flags: review?(felipc) → review+
Attachment #8951464 - Flags: review?(rhelmer)
Comment on attachment 8951464 [details]
Bug 1436851 - Implement mechanism to disable system addon updates via enterprise policy

https://reviewboard.mozilla.org/r/220332/#review226970

r=me with nits addressed

::: toolkit/mozapps/extensions/AddonManager.jsm:1343
(Diff revision 1)
>  
>      let buPromise = (async () => {
>        let appUpdateEnabled = Services.prefs.getBoolPref(PREF_APP_UPDATE_ENABLED) &&
> -                             Services.prefs.getBoolPref(PREF_APP_UPDATE_AUTO);
> +                             Services.prefs.getBoolPref(PREF_APP_UPDATE_AUTO) &&
> +                             !(Services.policies &&
> +                             !Services.policies.isAllowed("SysAddonUpdate"));

nit: fix the indentation here.

::: toolkit/mozapps/extensions/AddonManager.jsm:3012
(Diff revision 1)
> +                           !(Services.policies &&
> +                           !Services.policies.isAllowed("SysAddonUpdate"));

I don't think you need this check here since this just calls `backgroundUpdateCheck()` which does the same check.  (I realize the existing code has the same issue, sigh...)
Attachment #8951464 - Flags: review?(aswan) → review+
Comment on attachment 8951465 [details]
Bug 1436851 - Prevent AddonTestUtils.jsm from overriding a pref value

https://reviewboard.mozilla.org/r/220334/#review226972

I don't really know what this code is supposed to do, redirecting to Kris
Attachment #8951465 - Flags: review?(aswan) → review?(kmaglione+bmo)
Comment on attachment 8951467 [details]
Bug 1436851 - Add test for enterprise policy to disable system addon updates

https://reviewboard.mozilla.org/r/220338/#review226974

redirect to rob who knows the system addon update test setup better than I do

::: toolkit/mozapps/extensions/test/xpcshell/test_system_update_enterprisepolicy.js:25
(Diff revision 1)
> +  let resolve = null;
> +  let promise = new Promise(r => resolve = r);

Why not just `return new Promise(resolve => {` and then the rest of this function in the nested function?

::: toolkit/mozapps/extensions/test/xpcshell/test_system_update_enterprisepolicy.js:28
(Diff revision 1)
> +  Services.obs.addObserver(function observer() {
> +    Services.obs.removeObserver(observer, "EnterprisePolicies:AllPoliciesApplied");
> +    resolve();
> +  }, "EnterprisePolicies:AllPoliciesApplied");
> +
> +  // Clear any previously used custom schema
> +  Cu.unload("resource:///modules/policies/schema.jsm");
> +
> +  if (customSchema) {
> +    let schemaModule = ChromeUtils.import("resource:///modules/policies/schema.jsm", {});
> +    schemaModule.schema = customSchema;
> +  }
> +
> +  Services.obs.notifyObservers(null, "EnterprisePolicies:Restart");

It looks like this is just copied verbatim from the policies tests' head.js?  Please move it to a test-only .jsm and import that here instead.
Attachment #8951467 - Flags: review?(aswan) → review?(rhelmer)
Comment on attachment 8951464 [details]
Bug 1436851 - Implement mechanism to disable system addon updates via enterprise policy

https://reviewboard.mozilla.org/r/220332/#review226998

::: toolkit/mozapps/extensions/AddonManager.jsm:3010
(Diff revision 1)
>      let appUpdateEnabled = Services.prefs.getBoolPref(PREF_APP_UPDATE_ENABLED) &&
> -                           Services.prefs.getBoolPref(PREF_APP_UPDATE_AUTO);
> +                           Services.prefs.getBoolPref(PREF_APP_UPDATE_AUTO) &&
> +                           !(Services.policies &&
> +                           !Services.policies.isAllowed("SysAddonUpdate"));

Please add a property to AddonManagerPrivate for this. The logic is getting too complicated to be duplicated in multiple places.
Comment on attachment 8951467 [details]
Bug 1436851 - Add test for enterprise policy to disable system addon updates

https://reviewboard.mozilla.org/r/220338/#review227000

::: toolkit/mozapps/extensions/test/xpcshell/test_system_update_enterprisepolicy.js:12
(Diff revision 1)
>  
>  ChromeUtils.import("resource://testing-common/httpd.js");
> +ChromeUtils.import("resource://testing-common/FileTestUtils.jsm");
> +
> +Services.prefs.setBoolPref("browser.policies.enabled", true);
> +Services.policies; // Load policy engine

Why is this necessary?
Comment on attachment 8951465 [details]
Bug 1436851 - Prevent AddonTestUtils.jsm from overriding a pref value

https://reviewboard.mozilla.org/r/220334/#review227002

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:1301
(Diff revision 1)
>        new TextEncoder().encode(JSON.stringify(data)));
>      this.overrideEntry = aomStartup.registerChrome(manifest, [
>        ["override", "chrome://browser/content/built_in_addons.json",
>         Services.io.newFileURI(file).spec],
>      ]);
> -    Services.prefs.setBoolPref(PREF_DISABLE_SECURITY, false);
> +    Services.prefs.setBoolPref(PREF_DISABLE_SECURITY, prevPrefVal);

Why is this necessary? Where is this called where the pref is enabled prior to the call?
Comment on attachment 8951467 [details]
Bug 1436851 - Add test for enterprise policy to disable system addon updates

https://reviewboard.mozilla.org/r/220338/#review227000

> Why is this necessary?

My understanding is that most things are not loaded by default when xpcshell tests run. This line forces the policy engine to load.
Comment on attachment 8951465 [details]
Bug 1436851 - Prevent AddonTestUtils.jsm from overriding a pref value

https://reviewboard.mozilla.org/r/220334/#review227002

> Why is this necessary? Where is this called where the pref is enabled prior to the call?

I believe that the pref is enabled by the testing harness. If this function is allowed to turn the pref on and off arbitrarily, then the policy manager is unable to properly determine whether it is running in a test here: https://searchfox.org/mozilla-central/rev/47cb352984bac15c476dcd75f8360f902673cb98/browser/components/enterprisepolicies/EnterprisePolicies.js#173
Comment on attachment 8951464 [details]
Bug 1436851 - Implement mechanism to disable system addon updates via enterprise policy

https://reviewboard.mozilla.org/r/220332/#review227888
Attachment #8951464 - Flags: review?(felipc) → review+
Comment on attachment 8951464 [details]
Bug 1436851 - Implement mechanism to disable system addon updates via enterprise policy

https://reviewboard.mozilla.org/r/220332/#review227900
Attachment #8951464 - Flags: review?(rhelmer) → review+
Comment on attachment 8951465 [details]
Bug 1436851 - Prevent AddonTestUtils.jsm from overriding a pref value

https://reviewboard.mozilla.org/r/220334/#review227002

> I believe that the pref is enabled by the testing harness. If this function is allowed to turn the pref on and off arbitrarily, then the policy manager is unable to properly determine whether it is running in a test here: https://searchfox.org/mozilla-central/rev/47cb352984bac15c476dcd75f8360f902673cb98/browser/components/enterprisepolicies/EnterprisePolicies.js#173

It's set by the mochitest harness, yes. But this function should only ever be called in xpcshell tests, where this pref should not be set by default.
Comment on attachment 8951467 [details]
Bug 1436851 - Add test for enterprise policy to disable system addon updates

https://reviewboard.mozilla.org/r/220338/#review227000

> My understanding is that most things are not loaded by default when xpcshell tests run. This line forces the policy engine to load.

Services are not loaded by default anywhere, unless they have something like a category manager entry to force them to be loaded. If these tests depend on the service being loaded, then they should depend on whatever logic causes them to be loaded normally. Touching a lazy getter to force a service to load invalidates the test.
The policy module gets loaded by nsXREDirProvider here:
https://searchfox.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#1015

There's a trick to make that load in an xpcshell test, isn't there?
(In reply to :Felipe Gomes (needinfo me!) from comment #25)
> The policy module gets loaded by nsXREDirProvider here:
> https://searchfox.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.
> cpp#1015
> 
> There's a trick to make that load in an xpcshell test, isn't there?

No, the XRE directory provider never runs in xpcshell tests. The policy service initialization should probably be tied into the AOM startup code if it needs to do any sort of initialization of its own.

That said, I don't see how touching the Services.policies getter helps here. It still doesn't trigger the policies-startup observer, and none of the other observer notifications the constructor adds should be triggered in an xpcshell test.
The constructor of that component registers observers, one being to restart itself. (Restart is a test-only feature, and to prevent abuse we check Cu.isInAutomation for that). 
So it works because it listens for the restart notification, and when it receives that notification, it fires all the other notifications that it's listening to, in the proper order.

How is the AOM initialized in xpchsell, as its initialization is right below the policies initialization in the same function in nsXREDirProvider?
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #24)
> Services are not loaded by default anywhere, unless they have something like
> a category manager entry to force them to be loaded. If these tests depend
> on the service being loaded, then they should depend on whatever logic
> causes them to be loaded normally. Touching a lazy getter to force a service
> to load invalidates the test.

It's not a lazy getter in the sense that it has undefined startup timing. It's just a singleton getter that triggers the constructor, exactly how the Add-ons Manager does. In fact, the code is right next to each other, and on xpcshell the AOM is initialized exactly the same:
https://dxr.mozilla.org/mozilla-central/rev/861067332bac96a44bbf41ef366f58a30476057b/toolkit/mozapps/extensions/internal/AddonTestUtils.jsm#580-583

So how about we wrap the policies service initialization in a similar function, which will trigger the constructor and fire the appropriate startup observer, and then the test calls this function?

This function could go in this new EnterprisePolicyTesting.jsm file that is being created. How does that sound?

(or it could go directly inside the setupPolicyEngineWithJson function being created. It already fires the needed notification, so all it needs to do extra is to run the module constructor)
Comment on attachment 8951467 [details]
Bug 1436851 - Add test for enterprise policy to disable system addon updates

https://reviewboard.mozilla.org/r/220338/#review228762
Attachment #8951467 - Flags: review?(rhelmer) → review+
Please see Comment 28
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8951465 [details]
Bug 1436851 - Prevent AddonTestUtils.jsm from overriding a pref value

https://reviewboard.mozilla.org/r/220334/#review227002

> It's set by the mochitest harness, yes. But this function should only ever be called in xpcshell tests, where this pref should not be set by default.

I explicitly set it in the test to address this concern.
Comment on attachment 8951465 [details]
Bug 1436851 - Prevent AddonTestUtils.jsm from overriding a pref value

https://reviewboard.mozilla.org/r/220334/#review231400
Attachment #8951465 - Flags: review?(kmaglione+bmo) → review+
Flags: needinfo?(kmaglione+bmo)
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/da7d8d24de61
Implement mechanism to disable system addon updates via enterprise policy r=aswan,Felipe,rhelmer
https://hg.mozilla.org/integration/autoland/rev/3a38bc5be24c
Prevent AddonTestUtils.jsm from overriding a pref value r=kmag
https://hg.mozilla.org/integration/autoland/rev/c17379600444
Create an enterprise policy to disable system addon updates r=Felipe
https://hg.mozilla.org/integration/autoland/rev/7c7f2203da21
Add test for enterprise policy to disable system addon updates r=Felipe,rhelmer
we tested this on latest nightly with JSON policy format and it is verified as fixed.
We will retest it when it implemented (fixed) using admx file format.
We retested this with adm file and it is verified as fixed. System addon updates can be disabled by this policy.
Test cases and runs are here-https://testrail.stage.mozaws.net/index.php?/plans/view/8760
status-firefox60: fixed → verified
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: