1003708 - Pref services.sync.tokenServerURI is reset when user disconnected Fxa.

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P4)

Description

[Pin Zhang [:pzhang] (inactive)]

Steps to reproduce:
 - Deploy your own Fxa services
   Say, sync.fxaexamples.com
 - Change Fxa prefs to *fxaexample.com
   services.sync.tokenServerURI, identity.fxaccounts.auth.uri etc.
 - Register new user and login
 - Open preferences dialog, switch to sync panel, and click 'Disconnect' button to sign out
 - Open about:config, you will find services.sync.tokenServerURI is reset.
   Other changed prefs, like identity.fxaccounts.auth.uri, are not reset.

Comment 1

[Pin Zhang [:pzhang] (inactive)]

Any ideas?

Comment 2

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

All prefs under services.sync are reset on disconnect.  The identity.* prefs are general prefs used for all Firefox Accounts related services, while the tokenserver pref is used just for sync, hence the split.

Comment 3

[Pin Zhang [:pzhang] (inactive)]

(In reply to Mark Hammond [:markh] from comment #2)
> All prefs under services.sync are reset on disconnect.  The identity.* prefs
> are general prefs used for all Firefox Accounts related services, while the
> tokenserver pref is used just for sync, hence the split.

What if user/3rd parties wanna use their own services?

Comment 4

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

They set the prefs up and they are done.  Disconnecting will reset all sync prefs (eg, choices of what to sync etc) - it's not an operation that should be performed repeatedly.  We expect that most users will configure sync once and be done.

Note that disconnecting in the old sync (which was called "unlink this device") also reset custom server prefs that had been set.

Comment 5

[Pin Zhang [:pzhang] (inactive)]

(In reply to Pin Zhang [:pzhang] from comment #3)
> (In reply to Mark Hammond [:markh] from comment #2)
> > All prefs under services.sync are reset on disconnect.  The identity.* prefs
> > are general prefs used for all Firefox Accounts related services, while the
> > tokenserver pref is used just for sync, hence the split.
> 
> What if user/3rd parties wanna use their own services?

If only services.sync.* are reset on disconnect, but identity.* prefs stay with un-reset values, the sync feature will be totally broken, right?

Comment 6

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

It will be "broken" until it is reconfigured correctly, yes.  However, I don't think we want to reset FxAccounts prefs on a sync reset, as that would break any non-sync uses of FxA.

Maybe if you describe your use-case and why this is a problem I might be able to help you with a solution.

Comment 7

[Pin Zhang [:pzhang] (inactive)]

(In reply to Mark Hammond [:markh] from comment #4)
> They set the prefs up and they are done.  Disconnecting will reset all sync
> prefs (eg, choices of what to sync etc) - it's not an operation that should
> be performed repeatedly.  We expect that most users will configure sync once
> and be done.

But there are always some users will disconnect and login again or switch to some other email accounts.

Comment 8

[Pin Zhang [:pzhang] (inactive)]

(In reply to Mark Hammond [:markh] from comment #6)
> It will be "broken" until it is reconfigured correctly, yes.  However, I
> don't think we want to reset FxAccounts prefs on a sync reset, as that would
> break any non-sync uses of FxA.
> 
> Maybe if you describe your use-case and why this is a problem I might be
> able to help you with a solution.

I tried to deploy my own Fxa services, and use them in my Firefox, so I changed several prefs including services.sync.tokenServerURI and identity.* with my own services, I logged in and synced my data successfully, then i found failed to signin again after disconnected, because services.sync.tokenServerURI was reset.


The general use-case is some users/companies/organizations might want to deploy their own Fxa services in Firefox, do you think it makes sense?

Comment 9

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

(In reply to Pin Zhang [:pzhang] from comment #8)
> The general use-case is some users/companies/organizations might want to
> deploy their own Fxa services in Firefox, do you think it makes sense?

That makes sense, but users disconnecting and reconnecting is something I don't understand.  As mentioned, legacy sync also resets custom server prefs on unlink.

Comment 10

[Pin Zhang [:pzhang] (inactive)]

(In reply to Mark Hammond [:markh] from comment #9)
> (In reply to Pin Zhang [:pzhang] from comment #8)
> > The general use-case is some users/companies/organizations might want to
> > deploy their own Fxa services in Firefox, do you think it makes sense?
> 
> That makes sense, but users disconnecting and reconnecting is something I
> don't understand.  

Aha ... user could do anything as long as you provide a disconnect button, :)

> As mentioned, legacy sync also resets custom server prefs
> on unlink.

According to https://support.mozilla.org/en-US/kb/how-to-update-to-the-new-firefox-sync?as=u&utm_source=inproduct:

  "Mozilla will continue to host the old version of Sync for a limited time to allow for migration to Firefox Accounts. Self-hosted users will soon be able to run their own instance."

the legacy sync will retire and we support self-hosted users, to support self-hosted users as we declared, we should fix this asap, but for legacy sync, i think it might not be high prioritized.

Comment 11

[Ben Curtis]

I'd like to +1 this. Resetting all sync preferences is fine, but services.sync.tokenServerURI reverting is not. It took me hours to figure out why testing for a self hosted FxA server wasn't working before I figured this out.

In any environment where an administrator is setting up users to work from the corporate FxA server, this poses an issue. An administrator can't guarantee that a user won't sign in or sign out even if we can't understand why. While this bug is filed for linux, it applies to all platforms.

Comment 12

[u529232]

+1
I think the probability is really high that you forget (or just don't know that you have to) to set services.sync.tokenServerURI after a disconnect (e.g. to use another account) and so accidentally upload your data to the mozilla server.

Comment 13

[jamespatterson]

While trying to get Sync working with my own server, I lost a lot of time because services.sync.tokenServerURI was continually reset. I don't want my data on Mozilla's servers, so to find out that it was there, even after I set it not to be because of some mysterious magic is a disappointment.

Comment 14

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

Attachment: 0001-Bug-1003708-Sync-token-server-pref-now-lives-under-i.patch

I agree this sucks. Richard, what do you think about this for desktop? Android uses the same pref, but I think this patch will let the 2 co-exist, even with migrations. I'm sure there are some docs we'd need to update too somewhere...

Untested beyond xpcshell tests passing, and not sure about s/URL/Url/ - caps never quite works with url ;)

Comment 15

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

Comment on attachment 8620857 [details] [diff] [review]
0001-Bug-1003708-Sync-token-server-pref-now-lives-under-i.patch

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

::: services/sync/modules/service.js
@@ +55,5 @@
>  // The probe will record true if the pref is modified, false otherwise.
>  const TELEMETRY_CUSTOM_SERVER_PREFS = {
>    WEAVE_CUSTOM_LEGACY_SERVER_CONFIGURATION: "services.sync.serverURL",
>    WEAVE_CUSTOM_FXA_SERVER_CONFIGURATION: "identity.fxaccounts.auth.uri",
> +  WEAVE_CUSTOM_TOKEN_SERVER_CONFIGURATION: "identity.sync.tokenserver.uri",

should probably arrange for both to be reported

Comment 16

[Richard Newman [:rnewman]]

I worry that we'll jump from frying pan into fire: that a self-hosted user will hit disconnect, and will subsequently try to set up Sync -- perhaps years later! -- and be frustrated that it doesn't work because some crufty old pref is still hanging around.

That's a weakness of putting transient setup choices into permanent configuration storage, of course.

Most of the problem is that we don't really give any insight into what's happening. I'd be much happier if the setup flow had something like

    You're using a custom Sync server.         [ Change ]

but we do what we can.

Comment 17

[Richard Newman [:rnewman]]

Comment on attachment 8620857 [details] [diff] [review]
0001-Bug-1003708-Sync-token-server-pref-now-lives-under-i.patch

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

This seems sane to me, assuming it passes tests. But there's documentation churn, manual testing, and probably a mailing list post hiding behind the tip of this spear.

::: services/sync/modules/browserid_identity.js
@@ +512,5 @@
> +    // pain-point for people using non-default servers as Sync may auto-reset
> +    // all services.sync prefs. So if that still exists, it wins.
> +    let url = Svc.Prefs.get("tokenServerURI"); // Svc.Prefs "root" is services.sync
> +    if (!url) {
> +      url = Services.prefs.getCharPref("identity.sync.tokenserver.uri");

Are you concerned about this throwing? That's one thing that Svc.Prefs saves you from.

::: services/sync/modules/service.js
@@ +55,5 @@
>  // The probe will record true if the pref is modified, false otherwise.
>  const TELEMETRY_CUSTOM_SERVER_PREFS = {
>    WEAVE_CUSTOM_LEGACY_SERVER_CONFIGURATION: "services.sync.serverURL",
>    WEAVE_CUSTOM_FXA_SERVER_CONFIGURATION: "identity.fxaccounts.auth.uri",
> +  WEAVE_CUSTOM_TOKEN_SERVER_CONFIGURATION: "identity.sync.tokenserver.uri",

yup

Comment 18

[lsof]

(In reply to Richard Newman [:rnewman] from comment #16)
> Most of the problem is that we don't really give any insight into what's
> happening. I'd be much happier if the setup flow had something like
> 
>     You're using a custom Sync server.         [ Change ]
> 
> but we do what we can.

+1 there is no visibility at all and this would fix it.

Comment 19

[Ryan Kelly [:rfkelly]]

> You're using a custom Sync server.         [ Change ]

IIUC, our jelly-donut mix of native code and web content makes this messaging pretty tricky to achieve, right?  We could fairly easily put this messaging on the self-hosted account setup page, but that won't help you if you're trying to connect to a custom server that no longer exists.

Comment 20

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

(In reply to Ryan Kelly [:rfkelly] from comment #19)
> > You're using a custom Sync server.         [ Change ]
> 
> IIUC, our jelly-donut mix of native code and web content makes this
> messaging pretty tricky to achieve, right?

Yeah, let's track that in a different bug if people feel we can actually do it and it has enough value for enough users. My gut feel is that most people who changed the tokenserver pref also already changed the FxA-related prefs (ie, that most self-hosters are running the entire stack), and even users that *only* changed the tokenserver pref are unlikely to expect a Sync reset to flip away from their custom storage server. So I believe this patch is going to help far more users than it hurts (and have zero impact on the vast majority of our users ;)

Comment 21

[Edwin Wong [:edwong]]

triage: p4

Comment 22

[Pochy]

"triage: p4" Whats those this mean?, when fix this? next release?

Comment 23

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

Attachment: 0001-Bug-1003708-Sync-token-server-pref-now-lives-under-i.patch

This patch is largely the same as the last. Main changes are that it now treats both old and new prefs as counting towards "custom server" telemetry, and changes to Sync's head.js - seeing as the tokenserver pref is no longer in services-sync.js, the Sync unittests failed as the pref didn't exist at all there (and the code assumes the pref does exist - there's no sane default it could use otherwise).

For this reason in particular I'm re-requesting review.  Nick, I'm also requesting feedback from you to check this makes sense from Android's POV and in particular whether you think this is something we should end up doing there? And to see if you can change your blog-post - see below.

Re documentation, the places I think that need to be changed, and which I can change, are:

* https://wiki.mozilla.org/User_Services/Sync/SetupSyncNext

* https://docs.services.mozilla.com/howtos/run-sync-1.5.html - apparently changes via https://github.com/mozilla-services/docs/blob/master/source/howtos/run-sync-1.5.rst, but I'm not sure if there's some "deploy" step?

* https://sourcegraph.com/github.com/mozilla-services/syncserver - not sure where this ends up being displayed as docs, or if a "deploy" is needed after a change?

There's also Nick's blog at http://www.ncalexander.net/blog/2014/07/05/how-to-connect-firefox-for-android-to-self-hosted-services/ which talks about this desktop pref - Nick, will you be able to update this once this lands to reflect the new pref name?

There are many other google hits for this pref name which is somewhat unfortunate, but probably not a big deal as we still allow the old prefname to be used and preferred if it exists.

Ryan, does the above sound good to you, and are there any other places you can think of that should be updated?

Comment 24

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

Green looking try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=40ded9117904

Comment 25

[Ryan Kelly [:rfkelly]]

> changes via https://github.com/mozilla-services/docs/blob/master/source/howtos/run-sync-1.5.rst,
> but I'm not sure if there's some "deploy" step?

It should auto-deploy when you commit to master, but there's always the possibility that process has fallen over, so we should double-check.

> https://sourcegraph.com/github.com/mozilla-services/syncserver - not sure where this 
> ends up being displayed as docs, or if a "deploy" is needed after a change?

I don't believe this is deployed as docs anywhere, so much as jsut read directly from the README on github.

Comment 26

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8628587 [details] [diff] [review]
0001-Bug-1003708-Sync-token-server-pref-now-lives-under-i.patch

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

Sadly, I expect this to write sentinels that current Fennec instances will process badly.  Happy to be proved wrong.

::: services/sync/modules/FxaMigrator.jsm
@@ +62,5 @@
>    "identity.fxaccounts.remote.signup.uri",
>    "identity.fxaccounts.remote.signin.uri",
>    "identity.fxaccounts.settings.uri",
> +  "identity.sync.tokenserver.uri",
> +  "services.sync.tokenServerURI", // superceded by the above, but still write it

Can we arrange for this to be set all the time?

We have code in the wild that expects it https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/MigrationSentinelSyncStage.java#51; we can't update that code fast enough to start sending sentinels with different pref values.

Comment 27

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Hammond [:markh] from comment #23)

> There's also Nick's blog at
> http://www.ncalexander.net/blog/2014/07/05/how-to-connect-firefox-for-
> android-to-self-hosted-services/ which talks about this desktop pref - Nick,
> will you be able to update this once this lands to reflect the new pref name?

Happy to update.  Bug 1161223 is going to wreak havoc on self-hosted Fennec configurations anyway.

Comment 28

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

Attachment: 0001-Bug-1003708-Sync-token-server-pref-now-lives-under-i.patch

(In reply to Nick Alexander :nalexander from comment #26)
> Sadly, I expect this to write sentinels that current Fennec instances will
> process badly.  Happy to be proved wrong.

Doh! Yes, thanks.

> > +  "identity.sync.tokenserver.uri",
> > +  "services.sync.tokenServerURI", // superceded by the above, but still write it
> 
> Can we arrange for this to be set all the time?

Yep - I now special-case this pref - regardless of which of the 2 possible prefs are actually set, the sentinel has it written as the "old" services.sync.tokenServerURI pref. When we read the sentinel we only look for the old pref name, but apply it to the new pref name. Includes tests.

Best I can tell this should work fine (but willing to be proven wrong ;)

Comment 29

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8630903 [details] [diff] [review]
0001-Bug-1003708-Sync-token-server-pref-now-lives-under-i.patch

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

lgtm.  Remember to update the reviewer in the commit comment.

::: services/sync/modules/FxaMigrator.jsm
@@ +65,2 @@
>  ];
> +// Note that "identity.sync.tokenserver.uri" and "services.sync.tokenServerURI"

nit: move this up; it looks like it's dangling.

::: services/sync/modules/browserid_identity.js
@@ +514,5 @@
> +    let url = Svc.Prefs.get("tokenServerURI"); // Svc.Prefs "root" is services.sync
> +    if (!url) {
> +      url = Services.prefs.getCharPref("identity.sync.tokenserver.uri");
> +    }
> +    if (url.endsWith("/")) { // trailing slashes cause problems...

Consider using a while loop.

::: services/sync/tests/unit/test_fxa_migration.js
@@ +281,5 @@
> +  Services.prefs.clearUserPref("services.sync.tokenServerURI");
> +  Assert.ok(!Services.prefs.prefHasUserValue("services.sync.tokenServerURI"));
> +  fxaMigrator._applySentinelPrefs(prefs);
> +  // We should have written the pref value to the *new* pref name.
> +  Assert.equal(Services.prefs.getCharPref("identity.sync.tokenserver.uri"), value);

Consider asserting the original value of this pref as well.  To verify it's really written.

Comment 30

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

Attachment: 0001-Bug-1003708-Sync-token-server-pref-now-lives-under-i.patch

(In reply to Nick Alexander :nalexander (PTO July 17-27) from comment #29)
> lgtm.  Remember to update the reviewer in the commit comment.

I'll take that as an invitation to make you the final reviewer, so awesome :)

> nit: move this up; it looks like it's dangling.

Done.

> Consider using a while loop.

Done.

> ::: services/sync/tests/unit/test_fxa_migration.js
> @@ +281,5 @@
> > +  Services.prefs.clearUserPref("services.sync.tokenServerURI");
> > +  Assert.ok(!Services.prefs.prefHasUserValue("services.sync.tokenServerURI"));
> > +  fxaMigrator._applySentinelPrefs(prefs);
> > +  // We should have written the pref value to the *new* pref name.
> > +  Assert.equal(Services.prefs.getCharPref("identity.sync.tokenserver.uri"), value);
> 
> Consider asserting the original value of this pref as well.  To verify it's
> really written.

I was initially unsure what you mean here, but then it dawned on me that you probably mean to check identity.sync.tokenserver.uri had a different value (ie, the default) at the start of the test - which this patch does, but please let me know if you meant something different.

Comment 31

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8633911 [details] [diff] [review]
0001-Bug-1003708-Sync-token-server-pref-now-lives-under-i.patch

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

lgtm.  Just one nit, which might not be relevant to services/sync/ code.

::: services/sync/modules/service.js
@@ +56,5 @@
>  const TELEMETRY_CUSTOM_SERVER_PREFS = {
>    WEAVE_CUSTOM_LEGACY_SERVER_CONFIGURATION: "services.sync.serverURL",
>    WEAVE_CUSTOM_FXA_SERVER_CONFIGURATION: "identity.fxaccounts.auth.uri",
> +  WEAVE_CUSTOM_TOKEN_SERVER_CONFIGURATION: [
> +    "identity.sync.tokenserver.uri",// The new prefname we use for tokenserver

nit: alignment of //, and full sentences.

Comment 32

[Pulsebot]

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

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 34

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

docs need update

Comment 35

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

(In reply to Mark Hammond [:markh] from comment #23)
> Re documentation, the places I think that need to be changed, and which I
> can change, are:
> 
> * https://wiki.mozilla.org/User_Services/Sync/SetupSyncNext

Done.

> * https://docs.services.mozilla.com/howtos/run-sync-1.5.html - apparently
> changes via
> https://github.com/mozilla-services/docs/blob/master/source/howtos/run-sync-
> 1.5.rst, but I'm not sure if there's some "deploy" step?

https://github.com/mozilla-services/docs/pull/53

> * https://sourcegraph.com/github.com/mozilla-services/syncserver - not sure
> where this ends up being displayed as docs, or if a "deploy" is needed after
> a change?

https://github.com/mozilla-services/syncserver/pull/59

> There's also Nick's blog at
> http://www.ncalexander.net/blog/2014/07/05/how-to-connect-firefox-for-
> android-to-self-hosted-services/ which talks about this desktop pref - Nick,
> will you be able to update this once this lands to reflect the new pref name?

Nick, you said you are happy (ok, willing ;) to change that - can you please do that?

Comment 36

[Nick Alexander :nalexander [he/him]]

> > There's also Nick's blog at
> > http://www.ncalexander.net/blog/2014/07/05/how-to-connect-firefox-for-
> > android-to-self-hosted-services/ which talks about this desktop pref - Nick,
> > will you be able to update this once this lands to reflect the new pref name?
> 
> Nick, you said you are happy (ok, willing ;) to change that - can you please
> do that?

I think I said happy, and it's done.  Thanks for following through on the documentation changes!

Comment 37

[Alexis Metaireau (:alexis)]

I've merged both documentation pull requests (for the docs and the tokenserver repositories).

Comment 38

[Ada [:adalucinet]]

Mark, can you please provide some guidance in order to verify this fix? I looked over this - https://docs.services.mozilla.com/howtos/run-fxa.html - How to run your own Firefox Accounts Server documentation, but it looks time consuming. Therefore, any other workaround would be greatly appreciated. Thanks in advance!

Comment 39

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

I don't think you can sanely verify the migration part of this. The general part can be verified by setting the pref "services.sync.log.appender.dump" to "Trace", then doing a Sync - the console will spew Sync logs, and you should see a line like:

> 1445380516199	Sync.BrowserIDManager	INFO	Getting an assertion from: https://token.services.mozilla.com/1.0/sync/1.5

This reflects the default value, but you should be able to change either "services.sync.tokenServerURI" or "identity.sync.tokenserver.uri" to use a different server, and it should be reflected in the log - the Sync obviously will not work, but the log should verify that it tries to use.

Comment 40

[Ryan Kelly [:rfkelly]]

> Mark, can you please provide some guidance in order to verify this fix? I looked over this -
> https://docs.services.mozilla.com/howtos/run-fxa.html - How to run your own Firefox Accounts 
> Server documentation, but it looks time consuming.

You could follow the instructions on that page for setting about:config, but use URLs from one or the FxA deploys rather than actually setting up your own FxA stack:

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Firefox_Accounts/Introduction#Stable_development_%28production_clone%29

Comment 41

[Ada [:adalucinet]]

With 'services.sync.log.appender.dump' pref set to 'Trace', only 'Sync encountered an error - see about:sync-log for the log file.' is visible in Browser Console; applicable for all prefs changes (both comment 39 and comment 40). Any ideas?

Comment 42

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

Sorry, my instructions missed a couple of key points:

* You also need to set the pref "browser.dom.window.dump.enabled" to true
* Start Firefox such that stdout can be seen - on Windows, start it with "-console", on other OS, generally just start it from a shell.

The instructions in comment 39 will cause the log messages to be sent via "dump()" and the above steps are necessary to see any dump statements.

Comment 43

[Ada [:adalucinet]]

With a Nightly build before this fix and the above instructions, ‘services.sync.tokenServerURI’ pref changed to its default value after disconnecting. Using latest 42.0b8 (Build ID: 20151019161651) across platforms [1], the pref *does not change* and ‘1445499114536	Sync.BrowserIDManager	INFO	Getting an assertion from: https://oauth-stable.dev.lcip.org/v1’ is present in terminal. 
Thanks for all the guidance! Marking here accordingly.

[1] Windows 7 64-bit, Mac OS X 10.10.4 and Ubuntu 13.10 64-bit

Comment 44

[Pochy]

I write a addon for myself, the addon change the prefs of custom sync and firefox accounts in desktop version, at this moment it works with this new pref, maybe this help, more in https://addons.mozilla.org/es/firefox/addon/custom-sync-fxa/