1249520 - Read FxA/Sync configuration from /.well-known/fxa-client-configuration

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

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

As discussed in bug 1237406, we'd like to move away from reading a bunch of preferences to determine the FxA configuration to reading /.well-known/fxa-client-configuration from a server. Thus, we could end up requiring just a single pref that list the server hosting this config file and do everything else automagically.

This bug tracks the work for desktop.

Support for this file is being done in https://github.com/mozilla/fxa-content-server/pull/3364

Comment 1

[Chris Karlof [:ckarlof]]

Blocked on the server work, so de-prioritizing for now.

Comment 2

[Shane Tomlinson [:stomlinson]]

:rfkelly - you said this week you wanted to see this feature happen, looks like it's already in the works. :D

Comment 3

[Ryan Kelly [:rfkelly]]

I got inspired/hassled-into taking another crack at this on the FxA side, fresh PR at:

  https://github.com/mozilla/fxa-content-server/pull/3919

Comment 4

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

Requesting re-prioritization at our next triage

Comment 5

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: Bug 1249520 - Add client support for a fxa-client-configuration endpoint

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

Comment 6

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

Comment on attachment 8793095 [details]
Bug 1249520 - Add client support for a fxa-client-configuration endpoint

https://reviewboard.mozilla.org/r/79808/#review78654

Looking good, but there's a few changes listed below...

::: browser/base/content/aboutaccounts/aboutaccounts.js:362
(Diff revision 1)
>      case "signin":
>        if (user) {
>          // asking to sign-in when already signed in just shows manage.
>          show("stage", "manage");
>        } else {
> +        fxAccounts.promiseAccountsSignInURI().then(url => {

While we didn't do this before in all cases, I wonder if we should consider returning these promises so the final .catch() is used on failure?

Also, I think we need some story for failure (which is much more likely here now than before). I suspect we might want a new panel to .show() on errors here - this could be a generic page which assumes the config URL is wrong - ie, something like "Unable to determine your Firefox Account server configuration. Please try again later" with exception details written to the console would probably be sufficient.

::: services/fxaccounts/FxAccounts.jsm:40
(Diff revision 1)
> +  "resource://gre/modules/FxAccountsWebChannel.jsm");
> +
>  XPCOMUtils.defineLazyModuleGetter(this, "Utils",
>    "resource://services-sync/util.js");
>  
> +const CONFIG_PREFS = [

this module is getting very big - what do you think about a new, say, FxAccountsConfig module with some of these changes? This new module could also include the `promiseAccounts*()` functions which reduces the size of this module even more. If we do that, I'd be fine with either keeping them public here with a 1-line impl that delegates to the new module, or just removing them completely and having aboutaccounts import and use them directly.

::: services/fxaccounts/FxAccounts.jsm:450
(Diff revision 1)
> +  resetConfigUrls() {
> +    let customRootURL = this.getCustomRootURL();
> +    if (!customRootURL) {
> +      return;
> +    }
> +    // they have the root uri pref set, so we clear all the prefs that we will have initialized.

expand this comment to note that the default values for all these prefs are configured to point at production.

::: services/fxaccounts/FxAccounts.jsm:460
(Diff revision 1)
> +      return;
> +    }
> +    let whitelistValue = Services.prefs.getCharPref("webchannel.allowObject.urlWhitelist");
> +    if (whitelistValue.startsWith(customRootURL + " ")) {
> +      whitelistValue = whitelistValue.slice(customRootURL.length + 1);
> +      Services.prefs.setCharPref("webchannel.allowObject.urlWhitelist", whitelistValue);

It might be nice to check here if the value you are setting is now the default, and if so, clearUserPref() - it's not a big deal, but it would then leave about:config showing the pref hasn't changed.

::: services/fxaccounts/FxAccounts.jsm:467
(Diff revision 1)
> +  },
> +
> +  getCustomRootURL() {
> +    let pref;
> +    try {
> +      pref = Services.prefs.getCharPref("identity.fxaccounts.root.uri");

I think having this preference name include "autoconfig" or similar might make it clearer (eg, s/root/autoconfig/)?

::: services/fxaccounts/FxAccounts.jsm:527
(Diff revision 1)
> +    if (config.sync_tokenserver_base_url) {
> +      Services.prefs.setCharPref("identity.sync.tokenserver.uri", config.sync_tokenserver_base_url + "/1.0/sync/1.5");
> +    }
> +    // Update the prefs that are based off of the rootURL
> +    Services.prefs.setCharPref("identity.fxaccounts.remote.webchannel.uri", rootURL);
> +    Services.prefs.setCharPref("identity.fxaccounts.settings.uri", rootURL + "/settings?service=sync&context=fx_desktop_v3");

Thinking about our IRC chat, I think a reasonable compromise here might be to create a new pref, identity-fxaccounts.contextparam (or similar) with "fx_desktop_v3" and use that pref here. Later we can consider Ryan's suggestion of fixing up existing custom prefs and/or work out a better strategy for these existing prefs.

::: services/fxaccounts/FxAccountsClient.jsm:13
(Diff revision 1)
>  
>  Cu.import("resource://gre/modules/Log.jsm");
>  Cu.import("resource://gre/modules/Promise.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://services-common/utils.js");
> +Cu.import("resource://services-common/rest.js");

this import doesn't look necessary given the changes you made here?

::: services/fxaccounts/FxAccountsWebChannel.jsm:469
(Diff revision 1)
>          log.warn("FxA WebChannel functionaly is disabled due to no URI pref.");
>        }
>      } catch (ex) {
>        log.error("Failed to create FxA WebChannel", ex);
>      }
> +  } else {

A bit of a nit, but I think this would read slightly better if this new block was at the top:
```
if (singleton) {
 if (...) {
   singleton.teardown();
   singleton = null
 }
}
if (!singleton) {
  existing code
}
```

However, are you sure this will work as expected? IIUC, this is called when a browser window is created - it's not clear to me that a new webchannel will be setup until that happens. A pref observer might be more reliable.

::: services/fxaccounts/tests/xpcshell/test_accounts.js:213
(Diff revision 1)
>  
> -add_test(function test_non_https_remote_server_uri() {
> +add_task(function* test_non_https_remote_server_uri() {
>    Services.prefs.setCharPref(
>      "identity.fxaccounts.remote.signup.uri",
>      "http://example.com/browser/browser/base/content/test/general/accounts_testRemoteCommands.html");
> -  do_check_throws_message(function () {
> +  let message;

This should probably use rejects() from Assert.jsm - IIRC, it would look something like:

yield rejects(fxAccounts.promiseBlah(), "Firefox Accounts server ...")

::: services/sync/modules/service.js:923
(Diff revision 1)
>  
>      Svc.Prefs.set("lastversion", WEAVE_VERSION);
>  
>      this.identity.deleteSyncCredentials();
>  
> +    this.identity.resetConfigURLs();

I think we can probably do this in the identity's finalize() method, which would save the new method on the identity.

The other alternative is that we could do it directly in fxa's signOut method, which means it doesn't need to bleed into browserid_identity at all?

Comment 7

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8793095 [details]
Bug 1249520 - Add client support for a fxa-client-configuration endpoint

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

Comment 8

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8793095 [details]
Bug 1249520 - Add client support for a fxa-client-configuration endpoint

https://reviewboard.mozilla.org/r/79808/#review78654

> While we didn't do this before in all cases, I wonder if we should consider returning these promises so the final .catch() is used on failure?
> 
> Also, I think we need some story for failure (which is much more likely here now than before). I suspect we might want a new panel to .show() on errors here - this could be a generic page which assumes the config URL is wrong - ie, something like "Unable to determine your Firefox Account server configuration. Please try again later" with exception details written to the console would probably be sufficient.

I've added this, although I'm just going to hope that we weren't ever relying on an error for something benign to happen and not change the page.

One issue I *did* run into that I'm not sure what to do about is the fact that if this page is open, we'll fail to reset the pref on signout, or rather, we'll reset them and then immediately set them back. E.g. we reset in signout, the signout listener on this page will run, it will move us to the signin page, which to get it, we ensure we're configured.

I'm unsure what the best thing to do in this case is.

> this module is getting very big - what do you think about a new, say, FxAccountsConfig module with some of these changes? This new module could also include the `promiseAccounts*()` functions which reduces the size of this module even more. If we do that, I'd be fine with either keeping them public here with a 1-line impl that delegates to the new module, or just removing them completely and having aboutaccounts import and use them directly.

This ended up being a bit hairier than I like due to cyclic dependency issues between various functions that were in the FxAccounts.jsm module. It's probably better to have this out of there, but it's still a little messy.

> Thinking about our IRC chat, I think a reasonable compromise here might be to create a new pref, identity-fxaccounts.contextparam (or similar) with "fx_desktop_v3" and use that pref here. Later we can consider Ryan's suggestion of fixing up existing custom prefs and/or work out a better strategy for these existing prefs.

Should this be an actual pref with a default? I went with "no", since none of the default prefs use it, but I could be convinced otherwise.

> this import doesn't look necessary given the changes you made here?

It's needed to get access to RESTRequest (used to download the config)

> A bit of a nit, but I think this would read slightly better if this new block was at the top:
> ```
> if (singleton) {
>  if (...) {
>    singleton.teardown();
>    singleton = null
>  }
> }
> if (!singleton) {
>   existing code
> }
> ```
> 
> However, are you sure this will work as expected? IIUC, this is called when a browser window is created - it's not clear to me that a new webchannel will be setup until that happens. A pref observer might be more reliable.

I explicitly call this function after changing the pref so that it works. If you think I should replace it with a listener, I will though.

I guess I didn't realize that it was only called when the browser window was created.

> I think we can probably do this in the identity's finalize() method, which would save the new method on the identity.
> 
> The other alternative is that we could do it directly in fxa's signOut method, which means it doesn't need to bleed into browserid_identity at all?

See my comment above. On the one hand, having it in signOut allows us to clear them *after* all the oauth tokens are destroyed, which is good, but I'm a bit concerned that this could cause timing issues since it happens asynchronously and isn't, err, synchronized with anything else.

Not sure what the best thing to do here is either, thoughts?

Comment 9

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

(In reply to Thom Chiovoloni [:tcsc] from comment #8)
> I've added this, although I'm just going to hope that we weren't ever
> relying on an error for something benign to happen and not change the page.

I'm not sure what you mean here. In theory, the code was already broken as anything can throw. However, in practice, the only thing that could fail in reality was getSignedInUser(), which basically never does throw even though it could in practice.

With your patch a simple network error will cause failure.

What are you concerned about here?

> One issue I *did* run into that I'm not sure what to do about is the fact
> that if this page is open, we'll fail to reset the pref on signout, or
> rather, we'll reset them and then immediately set them back. E.g. we reset
> in signout, the signout listener on this page will run, it will move us to
> the signin page, which to get it, we ensure we're configured.

To ensure I understand: these items will be configured as soon as about:accounts is opened. If about:accounts is open while the user logs out, the fact it shifts to a login page immediately is the same basic scenario, which I don't have a problem with. What it means (IIUC) is that if a user starts a login process, then aborts it, they remain configured with the values from the config URL.

While that's fine in practice, it does raise an interesting point - the config URL might change between those 2 events. We could mitigate that by reconfiguring every time promiseAccountsSignInURI() is called and no user is signed in. Given a config URL isn't our default behaviour, I think that's probably fine.

It seems this might be as easy as having ensureConfigured() see if an auto-config URL exists rather than checking the other URLs haven't been modified. Given this is only called by the signIn/signUp functions, it might work OK? This would rely on the fact that aboutAccounts doesn't call these functions until it actually needs the URLs, which I think it true. It's not ideal and might not be good enough if we were relying on autoconfig as the default behaviour, but I think it might be OK for the use-cases we are trying to solve here. WDYT?

> This ended up being a bit hairier than I like due to cyclic dependency
> issues between various functions that were in the FxAccounts.jsm module.
> It's probably better to have this out of there, but it's still a little
> messy.

I think it looks good - and we can go further - I'll comment on the patch.

> > I think we can probably do this in the identity's finalize() method, which would save the new method on the identity.
> > 
> > The other alternative is that we could do it directly in fxa's signOut method, which means it doesn't need to bleed into browserid_identity at all?
> 
> See my comment above. On the one hand, having it in signOut allows us to
> clear them *after* all the oauth tokens are destroyed, which is good, but
> I'm a bit concerned that this could cause timing issues since it happens
> asynchronously and isn't, err, synchronized with anything else.

I think that looks OK. As I'll comment in the bug, an additional safety check would be to check currentAccountState.isCurrent before resetting - if that returns false then another user has started/completed login. Unless I misunderstand your concern here?

Comment 10

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

Comment on attachment 8793095 [details]
Bug 1249520 - Add client support for a fxa-client-configuration endpoint

https://reviewboard.mozilla.org/r/79808/#review78966

I think this looks great with all my comments addressed - including my comment in the bug about a possibility of a "stale" config being used. If that seems problematic, let's discuss it further before landing.

Just to make sure - have you tested it *actually* works? :)

::: services/fxaccounts/FxAccounts.jsm:793
(Diff revision 2)
>            this.notifyObservers("testhelper-fxa-signout-complete");
> -        });
> +        })
> +      } else {
> +        // We want to do this either way -- but if we're signing out remotely we
> +        // need to wait until we destroy the oauth tokens if we want that to succeed.
> +        FxAccountsConfig.resetConfigURLs();

As I mentioned in the other comment, an additional currentAccountState.isCurrent check might be worthwhile (probably doing log.warn if it is false)

::: services/fxaccounts/FxAccounts.jsm:1261
(Diff revision 2)
> -    return url;
>    },
>  
>    // Returns a promise that resolves with the URL to use to force a re-signin
>    // of the current account.
> -  promiseAccountsForceSigninURI: function() {
> +  promiseAccountsForceSigninURI: Task.async(function *() {

I think we should go further here and move all these `promise*URI` functions into the config object - that allows us to keep `_requireHttps` as private in that module.

::: services/fxaccounts/FxAccountsConfig.jsm:20
(Diff revision 2)
> +                                  "resource://gre/modules/FxAccounts.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "EnsureFxAccountsWebChannel",
> +                                  "resource://gre/modules/FxAccountsWebChannel.jsm");
> +
> +const DEFAULT_CONTEXT_VERSION = "fx_desktop_v3";

I think we should add this as a real pref, next to the other `identity.*` items. It will be far more likely to be seen when the other items with this param are changed, and it seems nicer than just having a comment for someone to notice and follow. That should mean you can just kill this default and the exception handler below too.

::: services/fxaccounts/FxAccountsConfig.jsm:91
(Diff revision 2)
> +        Services.prefs.setCharPref("webchannel.allowObject.urlWhitelist", whitelistValue);
> +      }
> +    }
> +  },
> +
> +  getAutoconfigURL() {

nit: I think getAutoConfigURL works better.

::: services/fxaccounts/FxAccountsConfig.jsm:133
(Diff revision 2)
> +      request.get(error => {
> +        if (error) {
> +          return reject(error);
> +        }
> +        if (!request.response.success) {
> +          return reject(request.response.status);

let's log.error here with the status, and maybe an additional .debug with the entire body (if one exists) - importing FxAccountsCommon.js will give you a log to use.

::: services/fxaccounts/FxAccountsConfig.jsm:142
(Diff revision 2)
> +    });
> +    let config = JSON.parse(jsonStr);
> +    // Update the prefs directly specified by the config.
> +
> +
> +    if (config.auth_server_base_url) {

and similarly, maybe a .trace or .debug of the entire body. Something like:

`log.debug("Got successful configuration response", config)`

will JSON.stringify for you.

Also, I'm not sure the checks for the elements in config are worthwhile and may do more harm than good. Eg, if a response doesn't have, say, the tokenserver URL, this is almost certainly going to leave a config that doesn't actually work. 

I think we should just wrap this in a try/catch and attempt to unconditionally set every pref, then log and re-throw on failure. Attempting to pass undefined to setCharPref fails.

Comment 11

[Thom Chiovoloni [:tcsc] (ex-moco)]

(In reply to Mark Hammond [:markh] from comment #9)
> (In reply to Thom Chiovoloni [:tcsc] from comment #8)
> > I've added this, although I'm just going to hope that we weren't ever
> > relying on an error for something benign to happen and not change the page.
> 
> I'm not sure what you mean here. In theory, the code was already broken as
> anything can throw. However, in practice, the only thing that could fail in
> reality was getSignedInUser(), which basically never does throw even though
> it could in practice.
> 
> With your patch a simple network error will cause failure.
> 
> What are you concerned about here?

Showing the wrong error message if getSignedInUser() threw -- which is probably not important.


> To ensure I understand: these items will be configured as soon as
> about:accounts is opened. If about:accounts is open while the user logs out,
> the fact it shifts to a login page immediately is the same basic scenario,
> which I don't have a problem with. What it means (IIUC) is that if a user
> starts a login process, then aborts it, they remain configured with the
> values from the config URL.

Yep, but also if they had the manage page open in the background and signed out, or similar.

> It seems this might be as easy as having ensureConfigured() see if an
> auto-config URL exists rather than checking the other URLs haven't been
> modified. Given this is only called by the signIn/signUp functions, it might
> work OK? This would rely on the fact that aboutAccounts doesn't call these
> functions until it actually needs the URLs, which I think it true. It's not
> ideal and might not be good enough if we were relying on autoconfig as the
> default behaviour, but I think it might be OK for the use-cases we are
> trying to solve here. WDYT?
> 

The only issue I can think of is that we'd be reconfiguring multiple times without resetting in between. Similarly, since that function is asynchronous, trying to do a second reconfigure while waiting for the request from the first.

But AFAICT these shouldn't be an issue with the code as it's written now, and the second one might be possible already anyway.

> I think that looks OK. As I'll comment in the bug, an additional safety
> check would be to check currentAccountState.isCurrent before resetting - if
> that returns false then another user has started/completed login. Unless I
> misunderstand your concern here?

Well, my concern is possibly that FxAccountsClient.signOut is going to be happening at the same time as this function, roughly, and we might clear the prefs while it's still using them, or before it has a chance to. Checking isCurrent does seem like a smart idea though.

Comment 12

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8793095 [details]
Bug 1249520 - Add client support for a fxa-client-configuration endpoint

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

Comment 13

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8793095 [details]
Bug 1249520 - Add client support for a fxa-client-configuration endpoint

https://reviewboard.mozilla.org/r/79808/#review78966

Yep, it does actually work.

> As I mentioned in the other comment, an additional currentAccountState.isCurrent check might be worthwhile (probably doing log.warn if it is false)

Turns out we can't do this. We do a local signout before here, which calls currentAccountState.signOut(), which means that the currentAccountState.isCurrent is always false.

> I think we should go further here and move all these `promise*URI` functions into the config object - that allows us to keep `_requireHttps` as private in that module.

`currentAccountState`, is a private property of the FxAccounts.jsm object, and is needed for `promiseAccountsForceSigninURI`, so I don't see how that one can be moved into `FxAccountsConfig`. Given that, it seemed cleaner to keep them all available on the `FxAccounts` object.

> let's log.error here with the status, and maybe an additional .debug with the entire body (if one exists) - importing FxAccountsCommon.js will give you a log to use.

Sounds good -- I added some additional error logging as well.

> and similarly, maybe a .trace or .debug of the entire body. Something like:
> 
> `log.debug("Got successful configuration response", config)`
> 
> will JSON.stringify for you.
> 
> Also, I'm not sure the checks for the elements in config are worthwhile and may do more harm than good. Eg, if a response doesn't have, say, the tokenserver URL, this is almost certainly going to leave a config that doesn't actually work. 
> 
> I think we should just wrap this in a try/catch and attempt to unconditionally set every pref, then log and re-throw on failure. Attempting to pass undefined to setCharPref fails.

We already have it as a string, and logging that string seems a bit more useful given that JSON.stringify could always fail on us (leaving us wondering what the request possibly could have returned)

Comment 14

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8793095 [details]
Bug 1249520 - Add client support for a fxa-client-configuration endpoint

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

Comment 15

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8793095 [details]
Bug 1249520 - Add client support for a fxa-client-configuration endpoint

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

Comment 16

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

Comment on attachment 8793095 [details]
Bug 1249520 - Add client support for a fxa-client-configuration endpoint

https://reviewboard.mozilla.org/r/79808/#review79302

Awesome, thanks!

::: browser/base/content/aboutaccounts/aboutaccounts.js:479
(Diff revision 5)
>      log("Failed to migrate FX Account: " + error);
>      show("stage", "intro");
>      // load the remote frame in the background
> -    wrapper.init(fxAccounts.getAccountsSignUpURI(), urlParams);
> +    fxAccounts.promiseAccountsSignUpURI().then(uri => {
> +      wrapper.init(uri, urlParams)
> +    }, e => {

I think this should be in a final .catch - IIUC, an exception in wrapper.init here will not call that error handler - not a big deal as we care more about promiseAccountsSignUpURL failing, but still seems worthwhile.

Comment 17

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8793095 [details]
Bug 1249520 - Add client support for a fxa-client-configuration endpoint

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

Comment 18

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8793095 [details]
Bug 1249520 - Add client support for a fxa-client-configuration endpoint

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79808/diff/6-7/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d08f86205057
Add client support for a fxa-client-configuration endpoint r=markh

Comment 20

[Carsten Book [:Tomcat]]

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