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

VERIFIED FIXED in Firefox 60

Status

()

defect
P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: Felipe, Assigned: bytesized)

Tracking

Trunk
Firefox 61
Points:
---

Firefox Tracking Flags

(firefox59 unaffected, firefox60+ verified, firefox61 verified)

Details

Attachments

(2 attachments)

(Reporter)

Description

a year ago
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
(Assignee)

Updated

a year ago
Assignee: nobody → ksteuber
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
Flags: needinfo?(gijskruitbosch+bugs)

Comment 3

a year ago
(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?
Flags: needinfo?(gijskruitbosch+bugs)

Comment 4

a year ago
(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.
(Reporter)

Comment 5

a year ago
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.
Can't we just simply hide the button and the test would just verify that the button wasn't there?
(Reporter)

Comment 7

a year ago
(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

a year ago
(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 hidden (mozreview-request)
I believe that I got the suggestion from comment #3 working as expected.
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8963223 - Flags: review?(gijskruitbosch+bugs)
Attachment #8963223 - Flags: review?(felipc)
(Assignee)

Updated

a year ago
Attachment #8963223 - Flags: review?(gijskruitbosch+bugs)
Attachment #8963223 - Flags: review?(felipc)
Removing review requests while I quickly add a test to make verify the changes to |PoliciesPrefTracker.restoreDefaultValues|.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8964698 - Flags: review?(felipc)
Attachment #8963223 - Flags: review?(gijskruitbosch+bugs)
Attachment #8963223 - Flags: review?(felipc)
(Reporter)

Comment 15

a year ago
mozreview-review
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
Attachment #8964698 - Flags: review?(felipc) → review+
(Reporter)

Comment 16

a year ago
mozreview-review
Comment on attachment 8963223 [details]
Bug 1429157 - Create an enterprise policy to prevent profile refreshes

https://reviewboard.mozilla.org/r/232112/#review238984
Attachment #8963223 - Flags: review?(felipc) → review+

Comment 17

a year ago
mozreview-review
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.
Attachment #8963223 - Flags: review?(gijskruitbosch+bugs) → review+
Comment hidden (mozreview-request)

Comment 19

a year ago
Pushed by ksteuber@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/33b7339d4168
https://hg.mozilla.org/mozilla-central/rev/109c31691ae8
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
(Reporter)

Comment 21

a year ago
[Tracking Requested - why for this release]:
Enterprise Policies
(Reporter)

Comment 22

a year ago
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
Attachment #8963223 - Flags: approval-mozilla-beta?
(Reporter)

Comment 23

a year ago
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
Attachment #8964698 - Flags: approval-mozilla-beta?
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 on attachment 8963223 [details]
Bug 1429157 - Create an enterprise policy to prevent profile refreshes

enterprise policy, approved for 60.0b11
Attachment #8963223 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8964698 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.