955953 - FxAccountsClient should set Accept-Language header on all server calls

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, defect)

Description

[Sam Penrose]

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

Comment 1

[Sam Penrose]

Attachment: 955953_v0.1.patch

A dirt-simple patch that reads the general.useragent.locale pref and sets the Accept-Language header accordingly.

Comment 2

[James Bonacci [:jbonacci]]

Again, is this for Desktop or Android or both?

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8355314 [details] [diff] [review]
955953_v0.1.patch

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

::: services/fxaccounts/FxAccountsClient.jsm
@@ +328,5 @@
>                          });
>        xhr.setRequestHeader("authorization", header.field);
>      }
>  
> +    let locale = Services.prefs.getCharPref("general.useragent.locale");

general.useragent.locale might be set to something weird for people that spoof their User-Agent header. nsHttpHandler [1] gets its accepted languages from intl.accept_languages.

In order to read that pref correctly you'd need to use Services.prefs.getComplexValue("intl.accept_languages", Ci.nsIPrefLocalizedString).data.

[1] https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpHandler.cpp#1274

@@ +329,5 @@
>        xhr.setRequestHeader("authorization", header.field);
>      }
>  
> +    let locale = Services.prefs.getCharPref("general.useragent.locale");
> +    if (!!locale) {

if (locale) is sufficient.

Comment 5

[Sam Penrose]

Attachment: 955953_v0.2.patch

Per ttaubert's feedback I read a different preference, and moved it to where the HTTP object is currently assembled.

Comment 6

[Sam Penrose]

Attachment: 955953_v0.2.patch

Now with actual patch.

Comment 7

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Comment on attachment 8376601 [details] [diff] [review]
955953_v0.2.patch

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

Hrm ... I'm getting a failure.  It doesn't like that indexOf test:

 0:23.86 TEST-UNEXPECTED-FAIL | /Users/zeus/code/firefox/build_inbound/_tests/xpcshell/services/common/tests/unit/test_restrequest.js | -1 != -1 - See following stack:
JS frame :: /Users/zeus/code/firefox/build_inbound/_tests/xpcshell/services/common/tests/unit/test_restrequest.js :: test_hawk_authenticated_request/server<["/elysium"] :: line 888
JS frame :: resource://testing-common/httpd.js :: ServerHandler.prototype.handleResponse :: line 2374
JS frame :: resource://testing-common/httpd.js :: Connection.prototype.process :: line 1225
JS frame :: resource://testing-common/httpd.js :: RequestReader.prototype._handleResponse :: line 1679
JS frame :: resource://testing-common/httpd.js :: RequestReader.prototype._processBody :: line 1527
JS frame :: resource://testing-common/httpd.js :: RequestReader.prototype.onInputStreamReady :: line 1395
native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

and since it fails, it returns a 500, so the following is kind of superfluous:

 0:23.89 TEST-UNEXPECTED-FAIL | /Users/zeus/code/firefox/build_inbound/_tests/xpcshell/services/common/tests/unit/test_restrequest.js | 200 == 500 - See following stack:
JS frame :: /Users/zeus/code/firefox/build_inbound/_tests/xpcshell/services/common/tests/unit/test_restrequest.js :: onComplete :: line 902
JS frame :: resource://services-common/rest.js :: onStopRequest :: line 459
native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

Comment 8

[Sam Penrose]

Hhmm. I'm only doing indexOf to work around black-box rewriting of the value. Also it Works On My Box™ . Maybe I'll switch to a regular expression as see if that passes for you.

Comment 9

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

You might initially set the pref to a value you control and that is highly unlikely to be a default.  This way I think you'll also be able to save yourself the regex tests, as it seems you and I have different whitespace in our prefs for some reason; there could be other differences with other testers, I imagine.

So for example:

  let str = Cc['@mozilla.org/supports-string;1'].createInstance(Ci.nsISupportsString);

  // Set the accept-languages pref to the Nepalese dialect of Zulu, falling back
  // on Volapük where that is not available.
  str.data = 'zu-NP;vo';
  Services.prefs.setComplexValue('intl.accept_languages', Ci.nsISupportsString, str);

  // ... do some tests ...

  // at the end of the tests, restore defaults
  Services.prefs.resetUserPrefs();

  // maybe a sanity check to be sure that got changed back
  let pref = Services.prefs.getComplexValue('intl.accept_languages', Ci.nsIPrefLocalizedString);
  do_check_neq(str.data, pref.data);

Comment 10

[Sam Penrose]

Attachment: 955953_v0.4.patch

Followed suggestion in comment #9, with slight tweaks. Who shall we ask to review this?

Comment 11

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Tests look good to me with this tweak.

Perhaps rnewman would be a good one to review this?

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 8376833 [details] [diff] [review]
955953_v0.4.patch

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

This is simple, which is nice.

I don't like that we attach A-L on every authenticated request -- it's wasteful. I do understand why, though.

I'm assuming that this prefs hit is cheap enough to do on every request without caching and using an observer to invalidate the cache. (I could imagine that this more complex access to prefs is less cheap than usual.) If you can spend two minutes to add a couple of log statements and verify that during a sync, please do.

Thanks for the test!

::: services/common/rest.js
@@ +788,5 @@
> +      if (acceptLanguage) {
> +        this.setHeader("Accept-Language", acceptLanguage);
> +      }
> +    } catch (err) {
> +      this._log.error("Error reading intl.accept_languages pref: " + CommonUtils.exceptionStr(err));

Concern: if this fails once, it'll probably fail over and over again, spamming the shit out of the user's console and sync log. Please add a flag in an outer scope to ensure that this only logs once per session, and update the log message accordingly.

(Even better if we skip doing the work altogether if it ever failed!)

Comment 13

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Perhaps in a follow-up bug we could read the pref the first time a RESTRequest is instantiated, and then subsequently only when we observe "nsPref:changed" for this pref?  That would also take care of the potential log spam on error.

Comment 14

[Richard Newman [:rnewman]]

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #13)
> Perhaps in a follow-up bug we could read the pref the first time a
> RESTRequest is instantiated, and then subsequently only when we observe
> "nsPref:changed" for this pref?  That would also take care of the potential
> log spam on error.

WFM.

Comment 15

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

I've filed Bug 974990 for the follow-up.

Sam, do you have time to address :rnewman's suggestion from Comment 12?  I'd be happy to tweak your patch and check it in for you if you don't have time.

Comment 16

[Sam Penrose]

Yeah, let's leave it on my plate for now

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Carsten Book [:Tomcat]]

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