1419505 - Allow cloning Sync sessions when cloning profiles.

Status: NEW

(Firefox :: Sync, enhancement, P5)

Description

[Dave Townsend [:mossop]]

Bug 1373244 is going to introduce profile per channel. In order to keep users seeing something like the existing behaviour when they first run a non-release build we will create a clone of their release profile but this causes problems if the users is currently signed in to sync. From Ryan Kelly:

> As I undestand it, the main issue here is that cloning the profile will result in two profiles that share a bunch of assumed-to-be-unique-to-a-single-Firefox-instance state from sync:
> 
> * The two profiles will share the same login session with Firefox Accounts
> * The two profiles will share the same sync client ID
> 
> This can result in a bunch of unexpected behviours, such as:
> 
> * The two profiles will show up as a single entry in the "devices and apps" list in FxA
> * The two profiles will clobber each others entries in the "synced tabs" menu
> * Sending a tab to one of the profiles will deliver it to whichever happens to sync first
> * Signing out of one profile will destroy the logged-in state of the other profile
> * And so on...

He proposes a way to get around this:

> * Add a "clone session token" API to the Firefox Accounts server
(currently you can only create a new session if you hold the user's
password)
> * Have the profile clone operation set a flag in the profile's FxA login data
> * When sync starts up in the new profile, have it detect this flag and
clone itself some new credentials

This bug is for looking at the sync and accounts part of this operation. We'd like to understand how much work is involve here.

Comment 2

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

FWIW, the client side of this should be relatively easy - I expect a few days work max (and more likely just 1). I'll let Ryan speak to the accounts side, although he did imply it's relatively sane.

Comment 3

[Alex Davis [:adavis]]

IIUC :mossop and :rt plan to release these new profiles in Fx 59. 59 just landed in Nightly so I think the runway here is short.

Comment 4

[Ryan Kelly [:rfkelly]]

Here's a sketch of an PI we could add on the accounts side to support this:

  https://github.com/mozilla/fxa-auth-server/issues/2236

The upside is I think it should be fairly simple to add.  The downside is it's a very special-purpose API, that I don't think would have any use other than to support this profile-cloning feature.  But, if this feature is important to Firefox then it's important to us.

Comment 5

[Romain Testard [:RT]]

Impacted users are non release users who were signed into sync pre applying 59 - these users would have to re sign into Sync which could be a pretty significant barrier for these users to keep using the service. This would also imply adding UI to inform users they have to sign back in.
We discussed with Alex and it seemed high value to deliver this, if the engineering effort is low and 59 is an achievable target then let's do it!

Comment 6

[Ryan Kelly [:rfkelly]]

The new /session/duplicate API just merged and should go live in FxA train 105, in about a week or so:

  https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#post-sessionduplicate

Once subtlty to keep in mind here: if the user has not verified their sessionToken, and you clone it, then the cloned profile will also have an unverified token.  The two tokens are distinct so *both* profile will have to do an email confirmation loop to verify their token before they can sync.

Comment 7

[Edouard Oger [:eoger]]

Attachment: Bug 1419505 p1 - Add Profile Fingerprint module.

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

Comment 8

[Edouard Oger [:eoger]]

Patch isn't tested yet.

Comment 9

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8948613 [details]
Bug 1419505 p1 - Add Profile Fingerprint module.

https://reviewboard.mozilla.org/r/218032/#review223824

IIUC, didCloneProfile is going to be true only on the very first run of the new profile - and there seems a reasonable risk that sync might not run in that session. ISTM that we might want nsBrowserGlue to just set a pref for us?

Comment 10

[Edouard Oger [:eoger]]

Comment on attachment 8948613 [details]
Bug 1419505 p1 - Add Profile Fingerprint module.

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

Comment 11

[Edouard Oger [:eoger]]

Comment on attachment 8948613 [details]
Bug 1419505 p1 - Add Profile Fingerprint module.

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

Comment 12

[Edouard Oger [:eoger]]

I've done a bunch of manual testing today on my patch and it seems that Push is making things difficult: we receive push messages twice and sync interlocks on the 2nd sync. After a reboot Sync looks like it is working fine however.

Kit, do you think we should wipe the database of push subscriptions after a clone? (more relevant to bug 1373244 probably)

Comment 13

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Edouard Oger [:eoger] from comment #12)
> sync interlocks on the 2nd sync.

That seems odd - what exactly happens (in terms of "interlocking")?

> Kit, do you think we should wipe the database of push subscriptions after a
> clone? (more relevant to bug 1373244 probably)

Ouch - yeah - that sounds like an issue that's bigger than our use of push.

Comment 14

[Edouard Oger [:eoger]]

> That seems odd - what exactly happens (in terms of "interlocking")?

We receive the same push notification twice [0] and end up interlocking here [1]

[0] https://searchfox.org/mozilla-central/rev/84cea84b12145d752e50ddca6be5462c38510e35/services/sync/modules/service.js#432
[1] https://searchfox.org/mozilla-central/rev/84cea84b12145d752e50ddca6be5462c38510e35/services/sync/modules/service.js#1102

Comment 15

[Lina Butler [:lina]]

(In reply to Edouard Oger [:eoger] from comment #12)
> Kit, do you think we should wipe the database of push subscriptions after a
> clone? (more relevant to bug 1373244 probably)

Yup! Resetting the `dom.push.userAgentID` pref should automatically drop all push subscriptions (and fire `pushsubscriptionchange` events for service workers): https://searchfox.org/mozilla-central/rev/a539f8f7fe288d0b4d8d6a32510be20f52bb7a0f/dom/push/PushServiceWebSocket.jsm#590-600

Comment 16

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

I have a vague idea that might also help bug 1234181 - that we should try and detect a change in profile regardless of how it happens. Something like:

* Sync stores in a pref a hashed value of, say, `${profilePath}-$[profileName}-${username}-${computername}'
* If Sync starts and it finds the current value doesn't match the previous value it assumes profile clone and does this migration.

This will not handle a profile being "moved" (ie, where the old profile will never again be run, so no duplication of device/tokens will happen), but I don't think that's a huge problem in practice as everything should end up recovering anyway. It also side-steps the issue that the first run of the new profile ends up crashing or terminating early causing the "just migrated" pref to not be written and causing us to not migrate.

WDYT?

Comment 17

[Julie McCracken (:julie)]

The Firefox team has backlogged the per channel profile effort.

Comment 18

[Alex Davis [:adavis]]

:julie do you know if it was just delayed by a train? (bug # pls)

I ask because I know that Lockbox was hoping that the per channel profile effort would resolve some of their problems caused by users upgrading and downgrading their profile version.

Comment 19

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Ed, what do you think about implementing something like comment 18? That would fix this bug and bug 1234181, and mean we have nothing else to do when per-channel profiles finally lands.

Comment 20

[Edouard Oger [:eoger]]

It seems to be worthwhile, however I wish we had a better computer identifier than the computer's name since some users might modify it on purpose then wonder why they have a duplicated sync device (it's a pretty minor inconvenience tough).
Network adapter's mac address isn't rock-solid either.
I'll post a patch implementing your idea nonetheless.

Comment 21

[Edouard Oger [:eoger]]

Comment on attachment 8948613 [details]
Bug 1419505 p1 - Add Profile Fingerprint module.

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

Comment 22

[Edouard Oger [:eoger]]

Note that we still have the push subscription problem with this patch.

Comment 24

[Edouard Oger [:eoger]]

Attachment: Bug 1419505 p2 - Handle profile fingerprint move in Push.

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

Comment 25

[Edouard Oger [:eoger]]

Attachment: Bug 1419505 p3 - Handle profile fingerprint move in Sync.

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

Comment 26

[Edouard Oger [:eoger]]

Comment on attachment 8948613 [details]
Bug 1419505 p1 - Add Profile Fingerprint module.

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

Comment 27

[Lina Butler [:lina]]

Comment on attachment 8962847 [details]
Bug 1419505 p2 - Handle profile fingerprint move in Push.

https://reviewboard.mozilla.org/r/231672/#review237374

Could I see the patch again with the `_stateChangeProcessEnqueue` change, please? A test would be awesome, but they're kinda tedious to write and run, as the Push service is a singleton...so it's OK if you just manually verified it works.

::: dom/push/PushService.jsm:502
(Diff revision 1)
>          this._changeServerURL(prefs.get("serverURL"), STARTING_SERVICE_EVENT));
>      }
> +
> +    if ('android' != AppConstants.MOZ_WIDGET_TOOLKIT) {
> +      // Drop push subscriptions on profile moved/copied.
> +      (async () => {

Hmm, I think we want to wrap this function in `this._stateChangeProcessEnqueue(...)`, and before we queue the calls to `_changeServerURL` above...but I don't remember offhand. If you tried it, and it broke, feel free to drop this issue. :-)

Comment 28

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8948613 [details]
Bug 1419505 p1 - Add Profile Fingerprint module.

https://reviewboard.mozilla.org/r/218032/#review237394

This looks good to me, thanks! However, as painful as it is, I think we should create a new toolkit bug for just this patch and get Mossop's review. We probably also want a bug in the "push" component for the other patch. Assuming we get that far, you should also open a telemetry bug so they too can handle it (but we don't need to make a patch for that)

::: browser/modules/ProfileFingerprint.jsm:4
(Diff revision 5)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

I think we should add a comment here indicating what the "fingerprint" is to be used for - people might see the name and think "See, I knew Mozilla was fingerprinting users" :)

::: browser/modules/ProfileFingerprint.jsm:9
(Diff revision 5)
> +
> +"use strict";
> +
> +var EXPORTED_SYMBOLS = ["ProfileFingerprint"];
> +
> +ChromeUtils.import("resource://gre/modules/ClientID.jsm");

This appears unused.

Comment 29

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

I believe we no longer clone profiles for per-channel profiles, so cloning isn't supported and isn't a requirement. We aren't going to get to this, but I think we'd probably accept a patch, so P5