1429164 - Policy: Don't show the Import Wizard

Status: VERIFIED FIXED

(Firefox :: Enterprise Policies, defect, P1)

Description

[:Felipe Gomes (needinfo for replies!)]

Policy: Don't show the Import Wizard

This is currently controlled via an override.ini file, but it should be simple to add a check to the EnterprisePolicies service.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

So this might not be that easy because gDoMigration is set and used very early, likely earlier than the policies service has had a chance to start.

Gijs, are you interested in looking into this? Or do you have thoughts on how to approach it?

FWIW, startup of the policies service happen here:
https://dxr.mozilla.org/mozilla-central/rev/c4ebc8c28a33b785dfbfa533810517cc707d1ad0/toolkit/xre/nsXREDirProvider.cpp#1015

(Before that, the policies are not available)

Comment 2

[:Gijs (he/him)]

We only ever show the profile migrator on first run (or when the relevant commandline arg is passed, but I assume this is only about first run). At that point there isn't a profile yet. If we wanted to do this we'd need to make the policies service available at that point (perhaps only in the case where first run profile creation (and therefore migration) would happen). And yes, that isn't currently the case, the policy service is started in DoStartup which is called after a profile is sorted out in nsAppRunner.cpp (see callsite in there, and a bit higher up, dealing with fx reset and migration wizard).

Does that help?

Also, should this also remove all the UI entrypoints and disable the commandline arg for the wizard, or not?

Comment 3

[:Felipe Gomes (needinfo for replies!)]

(In reply to :Gijs (lower availability until Jan 27) from comment #2)
> We only ever show the profile migrator on first run (or when the relevant
> commandline arg is passed, but I assume this is only about first run). At
> that point there isn't a profile yet. If we wanted to do this we'd need to
> make the policies service available at that point (perhaps only in the case
> where first run profile creation (and therefore migration) would happen).
> And yes, that isn't currently the case, the policy service is started in
> DoStartup which is called after a profile is sorted out in nsAppRunner.cpp
> (see callsite in there, and a bit higher up, dealing with fx reset and
> migration wizard).

Hmm, to clarify: you mentioned that the profile isn't there yet, but I suspect you're saying when the commandline/env var is processed.. but by the time the migrator actually runs, a profile must already exist, right?

If there's at least access to prefs, I think we can refactor things a little bit to allow the policy service to be started up earlier than that in that one specific case.

> 
> Does that help?
> 
> Also, should this also remove all the UI entrypoints and disable the
> commandline arg for the wizard, or not?

Yeah, likely.. But that's easier, and a secondary concern.. I think the main point is for sysadmins to provide a fresh install that has never run before, and not have to teach users to skip the import, etc..

But there's this line here: https://searchfox.org/mozilla-central/rev/e3cba77cee3ff1be38313abe9c804d13c51bd95b/toolkit/xre/nsAppRunner.cpp#2622
which confuses my understanding about when the import wizard is actually shown to the user.
Gijs, can you explain how it usually works in practice?


I'll also needinfo mkaply to see how people have requested this in the past, and how this was handled in CCK2 or other solutions.

It's my understanding that this is already possible with an override.ini file, so maybe we can/should leave this out of the policy manager if we can't come up with a good solution here.

Comment 4

[Mike Kaply [:mkaply]]

CCK2 uses the override.ini to make this happen because I couldn't find any other way.

But as you pointed out, I believe we don't show the import dialog on fresh installs anymore.

Comment 5

[:Gijs (he/him)]

(In reply to :Felipe Gomes (needinfo me!) from comment #3)
> (In reply to :Gijs (lower availability until Jan 27) from comment #2)
> > We only ever show the profile migrator on first run (or when the relevant
> > commandline arg is passed, but I assume this is only about first run). At
> > that point there isn't a profile yet. If we wanted to do this we'd need to
> > make the policies service available at that point (perhaps only in the case
> > where first run profile creation (and therefore migration) would happen).
> > And yes, that isn't currently the case, the policy service is started in
> > DoStartup which is called after a profile is sorted out in nsAppRunner.cpp
> > (see callsite in there, and a bit higher up, dealing with fx reset and
> > migration wizard).
> 
> Hmm, to clarify: you mentioned that the profile isn't there yet, but I
> suspect you're saying when the commandline/env var is processed.. but by the
> time the migrator actually runs, a profile must already exist, right?

The directory is there when the migrator runs, yes. But ProfD is not defined, only ProfDS, and e.g. when resetting, Services.prefs doesn't know where to save a pref file when calling savePrefFile without a file argument doesn't work (x-ref https://dxr.mozilla.org/mozilla-central/rev/e22e2c9eb81686e958a9448ea3d1e8cd766743e2/browser/components/migration/FirefoxProfileMigrator.js#133 ). Likewise for homepage saving code: https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/content/migration.js#454 .

> If there's at least access to prefs, I think we can refactor things a little
> bit to allow the policy service to be started up earlier than that in that
> one specific case.

I'm not sure off-hand when we start prefs.

> > Does that help?
> > 
> > Also, should this also remove all the UI entrypoints and disable the
> > commandline arg for the wizard, or not?
> 
> Yeah, likely.. But that's easier, and a secondary concern.. I think the main
> point is for sysadmins to provide a fresh install that has never run before,
> and not have to teach users to skip the import, etc..
> 
> But there's this line here:
> https://searchfox.org/mozilla-central/rev/
> e3cba77cee3ff1be38313abe9c804d13c51bd95b/toolkit/xre/nsAppRunner.cpp#2622
> which confuses my understanding about when the import wizard is actually
> shown to the user.
> Gijs, can you explain how it usually works in practice?

This changed for 57, and I didn't realize that happened. See https://hg.mozilla.org/integration/autoland/rev/19185f801cdd / bug 1403402. I think this means we never show the wizard on startup automatically anymore, unless a commandline arg / env var is explicitly passed in (overriding that I think isn't something we should bother offering a specific policy thing for).

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Ah, so that makes things a lot simpler.

What I think we should do with this policy then is to remove all the UI entry points for importing when it's in effect.

Also, since the commandline arg will be rare, I think we can start the policies earlier in that rare case just to check for it. But I'm not sure if it's necessary.


How does that sound?

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Also, what's AutoMigrate? Does this policy need to worry about that code?

Comment 8

[:Gijs (he/him)]

(In reply to :Felipe Gomes (needinfo me!) from comment #7)
> Also, what's AutoMigrate? Does this policy need to worry about that code?

It's a replacement for the wizard that just automatically imports data from the default profile of the default browser. It was tested repeatedly in funnelcakes but there were issues with it, both in terms of user confusion (how did Firefox get this data / wait, I didn't want Firefox to get all that data) and performance (which Doug Thayer has been working on generally, which also helps the import wizard).

In this particular case, the changes from bug 1403402 will also disable automigration, I think, as it is only invoked from the wizard ( https://dxr.mozilla.org/mozilla-central/rev/e22e2c9eb81686e958a9448ea3d1e8cd766743e2/browser/components/migration/MigrationUtils.jsm#978 ; invoked from startupMigration which is bound here: https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/ProfileMigrator.js#14 which in turn is run from https://dxr.mozilla.org/mozilla-central/rev/e22e2c9eb81686e958a9448ea3d1e8cd766743e2/toolkit/xre/nsAppRunner.cpp which is now disabled because gDoMigration isn't set to true when running on a clean install).

So generally, I think you're fine until/unless we end up re-enabling automigration in some cases. I assume Doug knows more about our plans here. TBH, I'm surprised that the gDoMigration patch for 57 from bug 1403402 just blanket turned it off, which will make it impossible to experiment with migration OR automigration against release in 57+ without changing that in the binary (rather than by flipping a pref).

Comment 9

[Alex Thayer [:alexical] (she/her)]

Last we talked, Verdi was hoping to experiment with different migration approaches, including automigration, but also just with other, non-wizard approaches. Verdi, could you talk a little more to this? We'll just need to sort out a less hard-coded way to disable clean install migration soon if we want to run these tests.

Comment 10

[Verdi [:verdi] Best to slack me]

Yes we did want to experiment with automigration on start up. We've just been waiting for the performance issues to be straightened out. We also want to experiment with a manual migration process that is not tied to the migration wizard and incorporates other processes (e.g. installing add-ons and/or signing in to Sync in addition to migrating browsing data).

Examples:
* This is a prototype we tested for automgration before running into the performance issues: https://mozilla.michaelverdi.com/public/am6.framer/
* Here's an idea for incorporating signing into sync: https://mozilla.invisionapp.com/share/F2F9X8B4W#/268576311_New_Tab_New
* And here's an idea for incorporating "migrating" extensions: https://mozilla.invisionapp.com/share/RFF9X9V9T#/268527474_New_Tab_New

Comment 11

[:Gijs (he/him)]

Sounds like we should have a separate bug to allow re-enabling this type of migration, and then in this bug add a way to disable that type of re-enabling. Not that that's confusing or anything...

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Note: set browser.newtabpage.activity-stream.migrationExpired to true to make Activity Stream not show the migration offer.

I think as a short-term work we can just set this pref and also disable the "Import Data from Another Browser" in the Bookmarks Manager

Comment 13

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

Attachment: Bug 1429164 - Add enterprise policy to prevent showing the profile import wizard

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

Comment 14

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8965005 [details]
Bug 1429164 - Add enterprise policy to prevent showing the profile import wizard

https://reviewboard.mozilla.org/r/233750/#review239392

::: browser/components/enterprisepolicies/tests/browser/browser_policy_disable_profile_import.js:42
(Diff revision 1)
> +  is(Services.prefs.getBoolPref("browser.newtabpage.activity-stream.migrationExpired", false),
> +     true, "Activity stream should not show migration offer");
> +  is(Services.prefs.prefIsLocked("browser.newtabpage.activity-stream.migrationExpired"), true,
> +     "Activity stream migration pref should be locked.");

nit: you can simplify this with checkLockedPref (a function available from head.js on this folder)

::: browser/components/places/content/places.js:192
(Diff revision 1)
> +    if (Services.policies && !Services.policies.isAllowed("profileImport")) {
> +      document.getElementById("OrganizerCommand_browserImport").setAttribute("disabled", true);
> +    }
> +

nit: for files under browser/, it's not necessary to first check for the existence of Services.policies

Comment 15

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

Comment on attachment 8965005 [details]
Bug 1429164 - Add enterprise policy to prevent showing the profile import wizard

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

Comment 16

[:Felipe Gomes (needinfo for replies!)]

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

Comment 17

[:Gijs (he/him)]

Comment on attachment 8965005 [details]
Bug 1429164 - Add enterprise policy to prevent showing the profile import wizard

https://reviewboard.mozilla.org/r/233750/#review239492

r=me, though obviously we'll need to make sure that if we ever end up in a place where we restart showing this dialog for new users, that we update the policy thingy to disable that.

::: browser/components/enterprisepolicies/tests/browser/browser_policy_disable_profile_import.js:14
(Diff revision 2)
> +  return new Promise(resolve => {
> +    library.addEventListener("unload", () => executeSoon(resolve), {once: true});
> +    library.close();
> +  });

I think you can probably use `BrowserTestUtils.closeWindow` ?

Comment 18

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

Comment on attachment 8965005 [details]
Bug 1429164 - Add enterprise policy to prevent showing the profile import wizard

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/be2c7141c619
Add enterprise policy to prevent showing the profile import wizard r=Felipe,Gijs

Comment 20

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/be2c7141c619

Comment 21

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8965005 [details]
Bug 1429164 - Add enterprise policy to prevent showing the profile import wizard

Approval Request Comment
[Feature/Bug causing the regression]: Enterprise Policies
[User impact if declined]: Policy to prevent showing the profile import wizard
[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 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8965005 [details]
Bug 1429164 - Add enterprise policy to prevent showing the profile import wizard

Needed for the policy engine work. Approved for 60.0b11.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1a77d1f0ddc2

Comment 24

[Abe - QA (:Abe_LV)]

We tested this on Nightly using JSON policy format and it is verified as fixed.

Import wizard is not available when this policy is in use.
 
We will also test this with adm policy format when ready.
Test cases and runs are here- https://testrail.stage.mozaws.net/index.php?/plans/view/8760

Comment 25

[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