Closed Bug 1429150 Opened 5 years ago Closed 5 years ago

Policy: Disable Firefox updates

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

(3 files, 1 obsolete file)

Bug 1429150 - Change nsIApplicationUpdateService::canCheckForUpdates implementation to respect new enterprise policy preventing app update
59 bytes, text/x-review-board-request
mhowell
: review+
Felipe
: review+
Details
Bug 1429150 - Create an enterprise policy to prevent the application from checking for app updates
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1429150 - Make the path to the root of the test data directory available via a pref
59 bytes, text/x-review-board-request
ahal
: review+
Details 
There's a policy request to disable Firefox updates.

The policy engine could either set app.update.enabled to false, or make the update service consult Services.policies.isAllowed("updates")..  Whatever is easier / cleaner.
Note that app.update.enabled is also used by the addons manager to control checking for system addon updates (not other kinds of addons though), so you'll want to consider whether this policy should affect that as well when deciding how to implement it.
See Also: → 1407820
Assignee: nobody → ksteuber
I plan to implement this as two separate policies: one policy to disable app updates and one policy to disable system addon updates.
Attachment #8949448 - Attachment is obsolete: true
Attachment #8949445 - Flags: review?(mhowell)
Attachment #8949445 - Flags: review?(felipc)
Attachment #8949447 - Flags: review?(ahalberstadt)
Attachment #8949446 - Flags: review?(felipc)
Comment on attachment 8949446 [details]
Bug 1429150 - Create an enterprise policy to prevent the application from checking for app updates

https://reviewboard.mozilla.org/r/218766/#review224562

just nit: the initial policies got out of order, but I've been now trying to keep things sorted alphabetically in policies-schema.json and Policies.jsm.

Once I update the underscore_style policies to CamelCase I'll finish sorting them..

::: browser/components/enterprisepolicies/Policies.jsm:131
(Diff revision 3)
>    },
> +
> +  "DisableAppUpdate": {
> +    onBeforeAddons(manager, param) {
> +      if (param == true) {
> +        manager.disallowFeature("appUpdate", false);

minor nit: we've been ommitting the `false` parameter here, as it is unecessary
Attachment #8949446 - Flags: review?(felipc) → review+
Comment on attachment 8949445 [details]
Bug 1429150 - Change nsIApplicationUpdateService::canCheckForUpdates implementation to respect new enterprise policy preventing app update

https://reviewboard.mozilla.org/r/218764/#review224568
Attachment #8949445 - Flags: review?(felipc) → review+
Comment on attachment 8949445 [details]
Bug 1429150 - Change nsIApplicationUpdateService::canCheckForUpdates implementation to respect new enterprise policy preventing app update

https://reviewboard.mozilla.org/r/218764/#review224594
Attachment #8949445 - Flags: review?(mhowell) → review+
Comment on attachment 8949447 [details]
Bug 1429150 - Make the path to the root of the test data directory available via a pref

https://reviewboard.mozilla.org/r/218768/#review224614
Attachment #8949447 - Flags: review?(ahalberstadt) → review+
Comment on attachment 8949446 [details]
Bug 1429150 - Create an enterprise policy to prevent the application from checking for app updates

https://reviewboard.mozilla.org/r/218766/#review224616

::: browser/components/enterprisepolicies/schemas/policies-schema.json:15
(Diff revision 5)
>        "enum": [true]
>      },
>  
> +    "DisableAppUpdate": {
> +      "description": "Prevent the browser from updating.",
> +      "first_available": "60.0",

btw, can you add here:

"enterprise_only": true


This is not implemented yet in the engine but I'll implement it soon
Originally, I was going to include the policy to disable system addons in the patches for this bug, but I have changed my mind and filed a new bug for that work: Bug 1436851

The patches posted here create the policy to disable app updates, and are ready to merge now that the system addon update patches will be done in Bug 1436851.
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1f2d0e869605
Change nsIApplicationUpdateService::canCheckForUpdates implementation to respect new enterprise policy preventing app update r=Felipe,mhowell
https://hg.mozilla.org/integration/autoland/rev/23a86891fdaf
Make the path to the root of the test data directory available via a pref r=ahal
https://hg.mozilla.org/integration/autoland/rev/dbdd43825f46
Create an enterprise policy to prevent the application from checking for app updates r=Felipe
I'm not very knowledgeable about this area of the code, or Android for that matter. What would be an appropriate way of fixing this problem? Does it make sense for Android devices to have access to this information? It doesn't make much of a difference to me, as there is not enterprise policy support for Android.

If Android should not have this value, I could just put the new code in a try block to catch the possible AttributeError.
Flags: needinfo?(ksteuber) → needinfo?(ahalberstadt)
Ah yeah, the problem is that Android does not have Services.policies. I forgot about it, but the intention is that any code outside of browser/ needs to first check if Services.policies is defined.

However, from the failure log, it looks like even checking this will fail on xpcshell tests.. I need to think a bit more about it.
Flags: needinfo?(ahalberstadt)
The way to make this work right now is like this:
https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/toolkit/modules/tests/xpcshell/test_Services.js#68

You can use it for now to land it.. I'll try to make it less ugly

(Note: if the policies service doesn't exist, this should just work as if it is allowed.. because there'll be no policy engine controlling the feature)
Ah ok. To make this nicer, can you replace this if check here:

https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/toolkit/modules/Services.jsm#121

with this one:
https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/toolkit/modules/tests/xpcshell/test_Services.js#68


Then in your code being added, you can check for Services.policies and it won't throw an error
Most android tests do have self.testRootAbs (and you can see they are passing on your push), but robocop is kind of a weird mini-harness within a harness under the guise of mochitest. It lives here:
https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/runrobocop.py

Though come to think of it, the value of your pref will have no meaning on Android, since it is a path on the host (not on the emulator). Assuming you don't need this value on Android, it's probably best to just say something like:

if getattr(self, 'testRootAbs', None):
    extra_prefs.append(...)

In case you do need this on Android, the path to the remote test root on the device is options.remoteTestRoot.
Attachment #8949445 - Flags: review+ → review?(felipc)
Attachment #8949446 - Flags: review+ → review?(felipc)
Attachment #8949447 - Flags: review+ → review?(ahalberstadt)
Comment on attachment 8949447 [details]
Bug 1429150 - Make the path to the root of the test data directory available via a pref

https://reviewboard.mozilla.org/r/218768/#review225342
Attachment #8949447 - Flags: review?(ahalberstadt) → review+
Comment on attachment 8949445 [details]
Bug 1429150 - Change nsIApplicationUpdateService::canCheckForUpdates implementation to respect new enterprise policy preventing app update

https://reviewboard.mozilla.org/r/218764/#review225766
Attachment #8949445 - Flags: review?(felipc) → review+
Comment on attachment 8949446 [details]
Bug 1429150 - Create an enterprise policy to prevent the application from checking for app updates

https://reviewboard.mozilla.org/r/218766/#review225768
Attachment #8949446 - Flags: review?(felipc) → review+
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/325d132b5921
Change nsIApplicationUpdateService::canCheckForUpdates implementation to respect new enterprise policy preventing app update r=Felipe,mhowell
https://hg.mozilla.org/integration/autoland/rev/121d786078de
Make the path to the root of the test data directory available via a pref r=ahal
https://hg.mozilla.org/integration/autoland/rev/ada56375e684
Create an enterprise policy to prevent the application from checking for app updates r=Felipe
https://hg.mozilla.org/mozilla-central/rev/325d132b5921
https://hg.mozilla.org/mozilla-central/rev/121d786078de
https://hg.mozilla.org/mozilla-central/rev/ada56375e684
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Nightly update is set to "Automatically install updates" in "about:preferences" even when the policy is in use. 
Here is screen capture- https://testing-1.tinytake.com/sf/MjM2MzA1NF83MTk4NjI1

Is this a separate issue or will we fix it here?
Right you are, I hadn't noticed that. I have filed Bug 1438925 to address this.
We tested "Disable Firefox updates” policy using JSON file. We verified this manually as fixed.  When this policy is in use, the user will not able to update Firefox. 

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[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
You need to log in before you can comment on or make changes to this bug.