1079835 - Add ability to pre-populate sync credentials from another profile in about:accounts

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Joe Walker [:jwalker]]

We'd like a way to pre-populate sync details from another profile to help the first run experience.

If about:accounts is started with a special query string then we take some extra actions:

    Find the profile marked Default=1 in profiles.ini
    
    If it exists, pre-populate the details into the signup form
    
    If it does not exist, then when the sync setup is complete,
    show a message to the user (text to be decided)

See bug 1079785 for details.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: migrate-sync

This patch brings us most of the way there: looks for the Default=1 profile and copies signedInUser.json to the current profile. This makes sync almost fully configured except for the password. Clicking on "reconnect"/"sign in" once will complete the process.

It doesn't display a message if no such file is found. It also expects to be run from a different app than Firefox, so the path traversal includes an additional hop. It could be simplified further if it is to be used from the same application, while running with a different profile.

It imports the MIT-licensed ini-parser library from https://github.com/isaacs/ini into the tree, for parsing profiles.ini.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Tested on Mac and Linux. The only changes to upstream ini-parser.js was incorporating the license text in the file (which seemed like a good idea) and a one-line change to use Services.appinfo.OS instead of process.platform to inquire the platform type.

Comment 3

[Chris Karlof [:ckarlof]]

Comment on attachment 8501759 [details] [diff] [review]
migrate-sync

Review of attachment 8501759 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +1311,5 @@
> +        }
> +        let originalFile = OS.Path.join(stableFirefox, defaultProfile, "signedInUser.json");
> +        let copiedFile = OS.Path.join(OS.Constants.Path.profileDir, "signedInUser.json");
> +        try {
> +          OS.File.copy(originalFile, copiedFile);

My understanding is that starting in Fx34, the sync credentials will be stored in the password manager (bug 1013064).

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

How can we retrieve the sync credentials from the password manager in a different profile? Are they stored in a SQLite database?

Comment 5

[Chris Karlof [:ckarlof]]

Someone from the Desktop team could probably help with that. :markh?

Comment 6

[Mark Hammond [:markh] [:mhammond]]

All interaction with the login manager is via the login manager interfaces.  I believe they recently started using JSON - see logins.json in your profile dir and toolkit/components/passwordmgr/LoginStore.jsm and storage-json.js for the implementation.  Note a master-password is going to be a fly in the ointment (ie, you will probably need to prompt for the MP in the old profile and manually use that to decrypt the contents.)

With some refactoring, it might be possible to arrange to create new login related objects and initialize then with the other profile dir.  FTR, dolske is the "owner" of the login manager stuff.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Pre-populate sync credentials from the default Firefox profile

Minor update that migrates from the default profile in the same application, not one in a sibling directory.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Pre-populate sync credentials from the default Firefox profile v2

Properly handle cases where we don't guess the default profile correctly.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: interdiff - Fix exception during jetpack tests

Patch against current gum HEAD to fix some jetpack tests.

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: Pre-populate sync credentials from the default Firefox profile v3

New patch, merged all 3 current patchs related to this bug on gum:
- Pre-populate sync credentials from the default Firefox profile (bug 1079835)
- Don't barf on failed sync credential migrations.
- the patch I just posted: Fix exception during jetpack tests

Comment 11

[Alexandre Poirot [:ochameau]]

With this patch, it looks like jetpack tests become green.

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8504281 [details] [diff] [review]
Pre-populate sync credentials from the default Firefox profile v3

Review of attachment 8504281 [details] [diff] [review]:
-----------------------------------------------------------------

Re ini-parser.js - I think we should investigate using the nsIToolkitProfileService for this.  Currently it doesn't expose whether a given profile is the default, but that looks relatively easy to add - the default flag is read at http://mxr.mozilla.org/mozilla-central/source/toolkit/profile/nsToolkitProfileService.cpp#478, just a couple of lines after each nsToolkitProfile is created, so that flag could be passed to the constructor and exposed via nsIToolkitProfile.

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Mark Hammond [:markh] from comment #12)
> Re ini-parser.js - I think we should investigate using the
> nsIToolkitProfileService for this.  Currently it doesn't expose whether a
> given profile is the default, but that looks relatively easy to add - the
> default flag is read at
> http://mxr.mozilla.org/mozilla-central/source/toolkit/profile/
> nsToolkitProfileService.cpp#478, just a couple of lines after each
> nsToolkitProfile is created, so that flag could be passed to the constructor
> and exposed via nsIToolkitProfile.

I have a patch in bug 1024110 that exposes the default profile in the profile service (see bug 1024110 comment 16 about this). I have thought before about reusing the profiles.ini parsing logic in nsToolkitProfileService.cpp, but I've shied away from doing so because it was reaching out to another app in the previous approach and doing this in C++ would increase the risk, given the time constraints. Now that we've switched to using a profile in the same app, I'll reconsider that decision.

Comment 14

[Alexandre Poirot [:ochameau]]

Attachment: improve sdk logging

Another patch to improve SDK logging when a promise is rejected.
Today we get a useless stack trace...
I'll submit this patch to its own SDK bug,
but that may help figure out our failures.

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: Tentative fix SDK tests

I don't get it, the issue is really fixed for me locally...
Here is a blind tentative to fix it on try.

Comment 16

[Alexandre Poirot [:ochameau]]

Attachment: interdiff - fix sdk failure for good

(hopefully) The tweak to jetpack unit test library helped figuring out what was wrong on try!
Here is an interdiff on top of other ones.
I'll shortly submit a merged'n rebased patch.

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: Pre-populate sync credentials from the default Firefox profile v4

Merged, rebased patch

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Pre-populate sync credentials from the default Firefox profile v5

Here is a patch without the ini-parser library that uses the nsIToolkitProfileService defaultProfile attribute introduced by the patch in bug 1024110. I've also included Alex's fixes, my latest fix from gum, and I've moved the code from the browser startup path to a handler for the about:accounts?action=migrateToDevEdition URL. This way it should be easy to activate the feature from the First Time Experience / UI Tour code.

I've left the identity.fxaccounts.migrateToDevEdition kill-switch in, as an additional guard against accidental use, but perhaps it's too much? Removing it would sure simplify the code a bit.

The other thing that I'm not sure about is the end result from visiting this URL: it's either the "manage" page or the "intro" page, depending on whether migration succeeded or not. It sure looks polished and professional, but perhaps we might want to add some text somewhere to indicate what happened?

Comment 19

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8507894 [details] [diff] [review]
Pre-populate sync credentials from the default Firefox profile v5

Review of attachment 8507894 [details] [diff] [review]:
-----------------------------------------------------------------

I'm missing the context of where this code is called from.

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +330,5 @@
>        fxAccounts.promiseAccountsForceSigninURI().then(url => {
>          show("remote");
>          wrapper.init(url, entryPoint);
>        });
> +    } else if (window.location.href.contains("action=migrateToDevEdition")) {

I think it makes sense to break this out into its own function.

@@ +336,5 @@
> +      let migrateSyncCreds = false;
> +      try {
> +        migrateSyncCreds = Services.prefs.getBoolPref("identity.fxaccounts.migrateToDevEdition");
> +      } catch (e) {}
> +      if (migrateSyncCreds) {

It looks to me that because you aren't grabbing data from the old profile's login manager, the user is going to need to reauthenticate before sync actually works.  Is that the case?  If so, then (a) we probably want to force that auth now and (b) we might be able to just grab the account name from signedInUser.json rather than copying the entire thing, which might be safer in the long term - that reauth will grab everything else we need.

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Add ability to pre-populate sync credentials from the default Firefox profile in about:accounts v6

I've made the suggested changes, except for only reading the account name from the JSON file. I don't know how to make that work. I did however avoid copying the file and just grabbed the important bits and passed them to setSignedInUser(). Now visiting that URL will lead the user right to the login screen, which eliminates all clicking.

The way this feature is going to be used is from the First Time Experience / UI Tour code (see bug 1079785) that will guide the user towards setting up sync in the new profile.

Comment 21

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8508760 [details] [diff] [review]
Add ability to pre-populate sync credentials from the default Firefox profile in about:accounts v6

Review of attachment 8508760 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the review delay and I should have mentioned this before, but I think we want tests for this - see browser_aboutAccounts.  Testing this might require moving the use of the profile service into a global helper (eg, getDefaultProfilePath) which the test can stub out pointing at a directory with a test signedInUser.json.  If timing really is a problem, I'd reluctantly accept a followup bug to add the tests (but given bug 1079785 has no patch yet so there's no code that takes advantage of this yet, I'd hope tests are possible)

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +387,5 @@
> +                          .defaultProfile;
> +    let fxAccountsStorage = OS.Path.join(defaultProfile.rootDir.path, fxAccountsCommon.DEFAULT_STORAGE_FILENAME);
> +    return OS.File.read(fxAccountsStorage, { encoding: "utf-8" }).then(text => {
> +      let accountData = JSON.parse(text).accountData;
> +      fxAccounts.setSignedInUser(accountData);

I think you want to return the promise from setSignedInUser

@@ +393,5 @@
> +      return fxAccounts.promiseAccountsForceSigninURI().then(url => {
> +        show("remote");
> +        wrapper.init(url, entryPoint);
> +      });
> +    }, error => {

A bit of a nit, but this error handler should be at the tail of this chain as it will not catch errors in the matching onResolve (eg, if promiseAccountsForceSigninURI failed.

So it would be something like:
  .read().then(text => {
  }.then(() => {
    return promiseAccountsForce...
  ).then(null, error => {
  }).then(() => {
     // Reset the pref
  });

theoretically there should even be *another* .then(null, error) at the end incase the pref set fails, but that's unlikely enough we can probably live without it.

Comment 22

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Add ability to pre-populate sync credentials from the default Firefox profile in about:accounts v7

Here is the updated patch with the suggested changes. I'll try to write a test early next week.

Comment 23

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v8

Here is an updated version with a test. Try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1b215455970d

Comment 24

[Panos Astithas (he/him) [:past] (please ni?)]

I've added some more logging to see why I'm getting a couple of oranges, but there seem to be a couple of hard to figure out races here: 

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c7cbb8ecc155

One is when I'm mocking the profile service lookup and the other is when I'm using getSignedInUser to verify that migration is finished.

Would you mind landing the actual change and keep working on the test to make it more reliable? Of course any ideas you may have to that effect are more than welcome!

Comment 25

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-test-fixups.patch

These are some test-only patches which should be reliable and cover what we need - they are on-top of your "v8" patch.  Can you please give them a once-over and incorporate them into the main patch for landing?

Try at https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=88b739f89173

Comment 26

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8511957 [details] [diff] [review]
v8

Review of attachment 8511957 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM with requested tweaks and test changes

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +330,5 @@
>        fxAccounts.promiseAccountsForceSigninURI().then(url => {
>          show("remote");
>          wrapper.init(url, entryPoint);
>        });
> +    } else if (window.location.href.contains("action=migrateToDevEdition")) {

I should have picked this up before, but I think we should have a check for "user == null" here - if there already is a user configured we shouldn't overwrite it.

@@ +336,2 @@
>      } else {
>        // No action specified

(so this comment should have appended something like ", or migration request when we already have a user")

@@ +401,5 @@
> +      Services.prefs.setBoolPref("identity.fxaccounts.migrateToDevEdition", false);
> +    });
> +  } else {
> +    if (user) {
> +      show("stage", "manage");

and this block (and the 'if') can be removed, as we'd never call this function when we already have a user configured.  Alternatively, keep the if and cu.reportError() in the "impossible" case.

::: browser/base/content/test/general/content_aboutAccounts.js
@@ +14,5 @@
>  }, true);
>  
>  addEventListener("DOMContentLoaded", function domContentLoaded(event) {
>    removeEventListener("DOMContentLoaded", domContentLoaded, true);
> +  sendAsyncMessage("test:document:domcontentloaded");

oh - the test tweaks I uploaded make this message obsolete but I forgot to remove it - can you please remove it?

Comment 27

[Panos Astithas (he/him) [:past] (please ni?)]

https://tbpl.mozilla.org/?tree=Try&rev=ddbe1d0c209e

Comment 28

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v9

Thanks for the help with the test! I made the requested changes and also removed the promiseNewTabDOMContentLoadedEvent test function, as it was no longer used after your changes.

New try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e3e0329842ab

Comment 29

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/16fe52c3df5f

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/16fe52c3df5f

Comment 31

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8513406 [details] [diff] [review]
v9

Approval Request Comment
[Feature/regressing bug #]: new feature, required for dev-edition
[User impact if declined]: new dev-edition users will have to use a new separate profile and without Sync they won't have access to their profile customizations. This patch streamlines the Sync setup process and will be triggered by the UI Tour and the preferences tab/dialog. 
[Describe test coverage new/current, TBPL]: unit test present and manual testing on gum and fx-team
[Risks and why]: minor risk as it's a fairly contained change (it is triggered only when a specific URL is visited)
[String/UUID change made/needed]: none

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8307f7f2a1df

Comment 33

[Camelia Badau [:cbadau], Desktop Test Engineering]

Does this fix need manual QA coverage?

Comment 34

[:Gavin Sharp [email: [e-mail]]]

(In reply to Camelia Badau, QA [:cbadau] from comment #33)
> Does this fix need manual QA coverage?

This should be verified via the UI that uses it in bug 1086936 (the "Start using Sync..." link).

Comment 35

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed with latest gum build (buildID: 20141105175749).

Comment 36

[Vlad Filippov (:vladikoff)]

I'm getting the following errors in the browser console:
http://v14d.com/i/545c1ad56f0ea.jpg

Which hints at this function:
```
+function getDefaultProfilePath() {
+  let defaultProfile = Cc["@mozilla.org/toolkit/profile-service;1"]
+                        .getService(Ci.nsIToolkitProfileService)
+                        .defaultProfile;
+  return defaultProfile.rootDir.path;
```

Build ID: 20141106040207
OS X 10.9

Comment 37

[Vlad Filippov (:vladikoff)]

More importantly when I go to `about:accounts?action=migrateToDevEdition` from `about:preferences` I get a blank tab. Screenshot: http://v14d.com/i/545c1b6b48340.jpg