Closed Bug 955953 Opened 6 years ago Closed 6 years ago

FxAccountsClient should set Accept-Language header on all server calls

Categories

(Firefox :: Firefox Accounts, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: spenrose, Assigned: spenrose)

References

Details

Attachments

(1 file, 3 obsolete files)

Summary: FxAccountsClient should set Accept-Language header on server calls that trigger emails → FxAccountsClient should set Accept-Language header on all server calls
Assignee: nobody → spenrose
Attached patch 955953_v0.1.patch (obsolete) — Splinter Review
A dirt-simple patch that reads the general.useragent.locale pref and sets the Accept-Language header accordingly.
Attachment #8355314 - Flags: feedback?(zack.carter)
Again, is this for Desktop or Android or both?
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.
Attachment #8355314 - Flags: feedback-
Blocks: 955951
No longer blocks: 943521
Duplicate of this bug: 938778
Attached patch 955953_v0.2.patch (obsolete) — Splinter Review
Per ttaubert's feedback I read a different preference, and moved it to where the HTTP object is currently assembled.
Attachment #8355314 - Attachment is obsolete: true
Attachment #8355314 - Flags: feedback?(zack.carter)
Attachment #8376594 - Flags: feedback?(jparsons)
Attached patch 955953_v0.2.patch (obsolete) — Splinter Review
Now with actual patch.
Attachment #8376594 - Attachment is obsolete: true
Attachment #8376594 - Flags: feedback?(jparsons)
Attachment #8376601 - Flags: feedback?(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
Attachment #8376601 - Flags: feedback?(jparsons)
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.
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);
Followed suggestion in comment #9, with slight tweaks. Who shall we ask to review this?
Attachment #8376601 - Attachment is obsolete: true
Attachment #8376833 - Flags: feedback?(jparsons)
Tests look good to me with this tweak.

Perhaps rnewman would be a good one to review this?
Attachment #8376833 - Flags: review?(rnewman)
Attachment #8376833 - Flags: feedback?(jparsons) → feedback+
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!)
Attachment #8376833 - Flags: review?(rnewman) → review+
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.
(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.
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.
Flags: needinfo?(spenrose)
Yeah, let's leave it on my plate for now
Flags: needinfo?(spenrose)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/e7c91056d5d5
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Status: NEW → ASSIGNED
Component: Identity → FxA
https://hg.mozilla.org/mozilla-central/rev/e7c91056d5d5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla30
Product: Core → Firefox
Target Milestone: mozilla30 → Firefox 30
You need to log in before you can comment on or make changes to this bug.