1429157 - Policy: Don't allow Firefox to be reset in about:support

Status: VERIFIED FIXED

(Firefox :: Enterprise Policies, defect, P1)

Description

[:Felipe Gomes (needinfo for replies!)]

Policy: Don't allow Firefox to be reset in about:support

Hide "reset-box" in about:support and intercept "resetFirefox" actions sent to UITour.onPageEvent. Also set browser.disableResetPrompt

Comment 1

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1429157 - Create an enterprise policy to prevent profile refreshes

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

Comment 2

[Robin Steuber (she/her) [:bytesized]]

So I have the policy written and attached, but the test does not pass. It seems that the test profile cannot be refreshed [1]. Because of this, I do not know if it is possible to write a test for this policy that does not result in false positives.

Gijs - Are you able to provide any insight into whether a test for this is possible and how it might be done?

[1] https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/toolkit/modules/ResetProfile.jsm#32-39

Comment 3

[:Gijs (he/him)]

(In reply to Kirk Steuber [:bytesized] from comment #2)
> So I have the policy written and attached, but the test does not pass. It
> seems that the test profile cannot be refreshed [1]. Because of this, I do
> not know if it is possible to write a test for this policy that does not
> result in false positives.
> 
> Gijs - Are you able to provide any insight into whether a test for this is
> possible and how it might be done?
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> 49cc27555d5b7ed2eb06baf156693dd578daa06f/toolkit/modules/ResetProfile.jsm#32-
> 39

You could force the profile to have a name using the profile service, to make it appear in profiles.ini, which should be enough to let you reset it. See here for how the marionette test for the actual refresh functionality does this:

https://searchfox.org/mozilla-central/source/browser/components/migration/tests/marionette/test_refresh_firefox.py#495-499

I think you wouldn't even need the flush. You'd want to make sure you remove the profile from profiles.ini again afterwards. You'd probably want to do that but make sure that the files aren't removed...

If that fails, you could probably write this as a marionette test, but it'd be simpler to have it be a browser mochitest given that that's what we're using for all the other tests.


Out of interest, I assume that the implementation wouldn't prevent using the commandline parameters and/or env vars, as they get read before there's a profile, and so presumably also before the policy takes effect? Did you test this?

Comment 4

[:Gijs (he/him)]

(In reply to :Gijs from comment #3)
> Out of interest, I assume that the implementation wouldn't prevent using the
> commandline parameters and/or env vars, as they get read before there's a
> profile, and so presumably also before the policy takes effect? Did you test
> this?

I guess comment #0 implies the policy won't prevent this, though I'm not clear on whether that's an oversight or intentional.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Not "intentional" in the sense of "we decided that the policy should do this", but in the sense of "we agreed that part is ok to be left out of scope for the policy".

This is more about preventing users from accidentally resetting their profile because they don't know what that means.

Comment 6

[Mike Kaply [:mkaply]]

Can't we just simply hide the button and the test would just verify that the button wasn't there?

Comment 7

[:Felipe Gomes (needinfo for replies!)]

(In reply to Mike Kaply [:mkaply] from comment #6)
> Can't we just simply hide the button and the test would just verify that the
> button wasn't there?

The problem is that the button is never there, because the profile created by the test harness doesn't support being reset. So the test would not really cover anything.

I think it would be acceptable to land this one without a test first and then figure out something afterwards

Comment 8

[:Gijs (he/him)]

(In reply to :Felipe Gomes (needinfo me!) from comment #7)
> (In reply to Mike Kaply [:mkaply] from comment #6)
> > Can't we just simply hide the button and the test would just verify that the
> > button wasn't there?
> 
> The problem is that the button is never there, because the profile created
> by the test harness doesn't support being reset. So the test would not
> really cover anything.
> 
> I think it would be acceptable to land this one without a test first and
> then figure out something afterwards

I too think that would be OK - though has someone tried the suggestion in comment #3 and does it not work? Because if it does work, writing a test shouldn't be too difficult... (famous last words etc.)

Comment 9

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8963223 [details]
Bug 1429157 - Create an enterprise policy to prevent profile refreshes

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

Comment 10

[Robin Steuber (she/her) [:bytesized]]

I believe that I got the suggestion from comment #3 working as expected.

Comment 11

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8963223 [details]
Bug 1429157 - Create an enterprise policy to prevent profile refreshes

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

Comment 12

[Robin Steuber (she/her) [:bytesized]]

Removing review requests while I quickly add a test to make verify the changes to |PoliciesPrefTracker.restoreDefaultValues|.

Comment 13

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1429157 - Change PoliciesPrefTracker.restoreDefaultValues to delete prefs that were previously non-existant

This is required to write a functional test for the enterprise policy that prevents profile refreshes.

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

Comment 14

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8963223 [details]
Bug 1429157 - Create an enterprise policy to prevent profile refreshes

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

Comment 15

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8964698 [details]
Bug 1429157 - Change PoliciesPrefTracker.restoreDefaultValues to delete prefs that were previously non-existant

https://reviewboard.mozilla.org/r/233406/#review238982

Comment 16

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8963223 [details]
Bug 1429157 - Create an enterprise policy to prevent profile refreshes

https://reviewboard.mozilla.org/r/232112/#review238984

Comment 17

[:Gijs (he/him)]

Comment on attachment 8963223 [details]
Bug 1429157 - Create an enterprise policy to prevent profile refreshes

https://reviewboard.mozilla.org/r/232112/#review239186

Nice, thanks! Just some test nits below.

::: browser/components/enterprisepolicies/tests/browser/browser_policy_disable_profile_reset.js:19
(Diff revision 4)
> +registerCleanupFunction(async function cleanup() {
> +  // Pass false to remove it from the profile service without deleting files.
> +  CREATED_PROFILE.remove(false);
> +});

Nit: if you move this into the `setup` function you don't need to have a global for the CREATED_PROFILE.

That variable probably also doesn't want to be all-caps.

::: browser/components/enterprisepolicies/tests/browser/browser_policy_disable_profile_reset.js:37
(Diff revision 4)
> +    let expectedDisplayValue = "block";
> +    if (disabled) {
> +      expectedDisplayValue = "none";
> +    }

Nit: `expectedDisplayValue = disabled ? "none" : "block";`

::: browser/components/enterprisepolicies/tests/browser/browser_policy_disable_profile_reset.js:44
(Diff revision 4)
> +      expectedDisplayValue = "none";
> +    }
> +    is(elementStyle.display, expectedDisplayValue,
> +       "about:support Reset button box should be hidden");
> +  });
> +  BrowserTestUtils.removeTab(tab);

Nit: should await this to avoid intermittent issues relating to tab closing stuff.

Comment 18

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8963223 [details]
Bug 1429157 - Create an enterprise policy to prevent profile refreshes

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/33b7339d4168
Change PoliciesPrefTracker.restoreDefaultValues to delete prefs that were previously non-existant r=Felipe
https://hg.mozilla.org/integration/autoland/rev/109c31691ae8
Create an enterprise policy to prevent profile refreshes r=Felipe,Gijs

Comment 20

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/33b7339d4168
https://hg.mozilla.org/mozilla-central/rev/109c31691ae8

Comment 21

[:Felipe Gomes (needinfo for replies!)]

[Tracking Requested - why for this release]:
Enterprise Policies

Comment 22

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8963223 [details]
Bug 1429157 - Create an enterprise policy to prevent profile refreshes

Approval Request Comment
[Feature/Bug causing the regression]: Enterprise Policies
[User impact if declined]: Policy to disable the "Refresh Firefox" functionality
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[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?]: no
[Why is the change risky/not risky?]: limited to this policy being used
[String changes made/needed]: none

Comment 23

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8964698 [details]
Bug 1429157 - Change PoliciesPrefTracker.restoreDefaultValues to delete prefs that were previously non-existant

Approval Request Comment
test harness improvement needed to uplift the main patch here

Comment 24

[Abe - QA (:Abe_LV)]

We tested this on latest Nightly with JSON policy format and it is verified as fixed.
We will also test this with adm format.
Using this policy, "Refresh Firefox" button in about:support can be disabled. 

Test cases and runs are here - https://testrail.stage.mozaws.net/index.php?/plans/view/8760

Comment 25

[Julien Cristau [:jcristau]]

Comment on attachment 8963223 [details]
Bug 1429157 - Create an enterprise policy to prevent profile refreshes

enterprise policy, approved for 60.0b11

Comment 26

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/releases/mozilla-beta/rev/733b9c627ffb6c0ccfd173199071b03e762ed1d8
https://hg.mozilla.org/releases/mozilla-beta/rev/6d2939595bbc9a7d4c8b4956c52fe4b8f871b228

Comment 27

[Abe - QA (:Abe_LV)]

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