Closed Bug 963835 Opened 10 years ago Closed 10 years ago

clients need to handle for incorrect case email strings

Categories

(Firefox :: Firefox Accounts, defect)

Product:
Firefox
Component:
Firefox Accounts
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 30

People

(Reporter: edwong, Assigned: jedp)

References

Details

(Whiteboard: [qa+])

Attachments

(2 files, 11 obsolete files)

963835-part0.patch
26.36 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
part 2: b2g: handle incorrect email case in fxa client
16.63 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review 
client side bug related to this:
https://github.com/mozilla/fxa-auth-server/issues/455

Clients need to try again with suggested email upon a email normalization error.

I consider this a must fix before we can turn this on by default for all nightly users.
Blocks: 799726, 905997
Whiteboard: [qa+]
Morphing this to be the desktop bug.

Nick's already working on the Android part, though I don't know which bug number it is.
No longer blocks: 799726
Component: Server: Firefox Accounts → Firefox Sync: Backend
OS: Mac OS X → All
QA Contact: jbonacci
Hardware: x86 → All
Ah, Bug 963160 is for Android.
See Also: → 963160
:spenrose, :jedp - we'll need this in FxOS also.
Yes indeed.  If Nick's already got an Android patch going, maybe we can replicate his work on FxOS.
QA Contact: jparsons
Assignee: nobody → jparsons
QA Contact: jparsons
(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #5)
> More reference: https://github.com/mozilla/fxa-auth-server/pull/522

Ah, that makes sense. Thanks Jed.
- Retries once on incorrect email case
- Test case
- Adds missing error and errno constants to fxa-common

Still using the /raw_password endpoints, which I will deal with in a separate patch.

Zach, since you're also looking at Vlad's PR for the desktop jelly, would you mind looking at this as well?
Attachment #8366319 - Flags: feedback?(zack.carter)
After talking with Chris K, once clients are updated to handle the mixed case, users will be able to sign in with whatever mixed case was used.  Users will not be stranded if we turn this on in nightly.
Comment on attachment 8366319 [details] [diff] [review]
handle incorrect email case in fxa client

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

This looks just about identical to the fxa-js-client implementation.

::: services/fxaccounts/tests/xpcshell/test_client.js
@@ +287,5 @@
> +});
> +
> +// End of tests
> +// Utility functions follow
> +

We should probably store these functions somewhere where they can be reused since both the tests and implementation use them.

@@ +311,5 @@
> +  }
> +
> +  for (let i = 0; i < hex.length; i += 2) {
> +    let cur = hex[i] + hex[i + 1];
> +    ret += String.fromCharCode(parseInt(cur, 16));

This works fine for the test string, but for something more robust in practice we might need fromCodePoint: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/fromCodePoint
Attachment #8366319 - Flags: feedback?(zack.carter) → feedback+
(In reply to Zachary Carter [:zaach] from comment #9)
> Comment on attachment 8366319 [details] [diff] [review]
> handle incorrect email case in fxa client

Thanks for taking a look, Zach.

> Review of attachment 8366319 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks just about identical to the fxa-js-client implementation.

Hopefully! :)

> ::: services/fxaccounts/tests/xpcshell/test_client.js
> @@ +287,5 @@
> > +});
> > +
> > +// End of tests
> > +// Utility functions follow
> > +
> 
> We should probably store these functions somewhere where they can be reused
> since both the tests and implementation use them.

Good point.  I've consolidated them in FxAccountsCommon.js.

> @@ +311,5 @@
> > +  }
> > +
> > +  for (let i = 0; i < hex.length; i += 2) {
> > +    let cur = hex[i] + hex[i + 1];
> > +    ret += String.fromCharCode(parseInt(cur, 16));
> 
> This works fine for the test string, but for something more robust in
> practice we might need fromCodePoint:
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/String/fromCodePoint

Perhaps so.  Though my understanding is that ckarlof and callahad are saying emails must be ascii at this point.  So while this might not be good for passwords, it ought to fly for emails.

Thanks!
Hi, Richard,
Can I ask you to review this?
thanks,
j
Attachment #8366319 - Attachment is obsolete: true
Attachment #8367444 - Flags: review?(rnewman)
Comment on attachment 8367444 [details] [diff] [review]
handle incorrect email case in fxa client

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

::: services/fxaccounts/FxAccountsClient.jsm
@@ +88,5 @@
> +        (result) => {
> +          // If the user entered an email with different capitalization from
> +          // what's stored in the database (e.g., Greta.Garbo@gmail.COM as
> +          // opposed to greta.garbo@gmail.com), the server will respond with a
> +          // 400 and the expected lower-casing of the email.  We have one

I thought it responded with a 401? Regardless, don't even bother checking.

@@ +96,5 @@
> +          if (400 === result.code &&
> +              ERRNO_INCORRECT_EMAIL_CASE === result.errno &&
> +              retryOK) {
> +            let emailLowerCase = email.toLowerCase();
> +            if (emailLowerCase == result.email) {

This is not what I expect to happen, and not what Android does.

As far as I can tell, this code does this:

* If we get a 400 with error code 120
* Lowercase the original email address (*klaxons start to fire*)
* If it's equal to the string returned from the server according to JS's string equality (*sirens*)
* Try again.


The server stores the static version of the user's email address, which is not necessarily lowercase.

If your provided email differs from the server's version, your stretched password will be wrong, and you'll get the correct email in the response. The client should use that directly. So this should read:

  if (!retryOK || (result.errno != ERRNO_INCORRECT_EMAIL_CASE)) {
    return result;
  }

  // TODO: Notify the rest of the system that the persisted email should be replaced.
  let correctEmail = result.email;
  return self.signIn(correctEmail, password, false);

::: services/fxaccounts/FxAccountsCommon.js
@@ +28,5 @@
>  
>    return log;
>  });
>  
> +// XXX Sadly, CommonUtils.bytesAsHex doesn't handle typed arrays.

Put this in CommonUtils, and ideally fix bytesAsHex!

@@ +39,5 @@
> +  return hex.join("");
> +};
> +
> +this.stringToHex = function stringToHex(str) {
> +  let encoder = new TextEncoder("utf-8");

CommonUtils.encodeUTF8(str)?

::: services/fxaccounts/tests/xpcshell/test_client.js
@@ +16,5 @@
>      return deferred.promise;
>  }
>  
> +add_task(function test_hexToString() {
> +  do_check_eq("i like pie", hexToString(stringToHex("i like pie")));

Let's be a little more thorough here.

Check against a known-good implementation, or at least another implementation. Note that 

  function stringToHex(x) {
    return x;
  }

passes this test.

You also need to pass in some non-ASCII here.

@@ +236,5 @@
>  });
> +
> +add_task(function test_email_case() {
> +  let dbEmail = "greta.garbo@gmail.com";
> +  let clientEmail = "Greta.Garbo@gmail.COM";

Let's use something that's a little more of an exercise:

services/common/tests/unit/test_utils_convert_string.js
29:  const INPUT = "Árvíztűrő tükörfúrógép いろはにほへとちりぬるを Pijamalı hasta, yağız şoföre çabucak güvendi.";

@@ +259,5 @@
> +        let body = JSON.parse(
> +          CommonUtils.readBytesFromInputStream(request.bodyInputStream));
> +        let email = hexToString(body.email);
> +
> +        // If the client has the wrong case on the email, we return a 400, with

I thought this was a 401? Where's the spec?
Attachment #8367444 - Flags: review?(rnewman) → review-
Sorry for the mess, Richard.  I had misunderstood what I was supposed to do here.  Your comments are helpful, thank you.  Revision to follow shortly.
(In reply to Richard Newman [:rnewman] from comment #12)
> Comment on attachment 8367444 [details] [diff] [review]
> handle incorrect email case in fxa client

Thanks for your review, Richard.  I think I've got it straightened out now.

> Review of attachment 8367444 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: services/fxaccounts/FxAccountsClient.jsm
> @@ +88,5 @@
> > +        (result) => {
> > +          // If the user entered an email with different capitalization from
> > +          // what's stored in the database (e.g., Greta.Garbo@gmail.COM as
> > +          // opposed to greta.garbo@gmail.com), the server will respond with a
> > +          // 400 and the expected lower-casing of the email.  We have one
> 
> I thought it responded with a 401? Regardless, don't even bother checking.

It is a 400 according to the api spec (https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#response-5).

But you're right, it doesn't matter.  I will just check for the errno.

> @@ +96,5 @@
> > +          if (400 === result.code &&
> > +              ERRNO_INCORRECT_EMAIL_CASE === result.errno &&
> > +              retryOK) {
> > +            let emailLowerCase = email.toLowerCase();
> > +            if (emailLowerCase == result.email) {
> 
> This is not what I expect to happen, and not what Android does.
> 
> As far as I can tell, this code does this:
> 
> * If we get a 400 with error code 120
> * Lowercase the original email address (*klaxons start to fire*)
> * If it's equal to the string returned from the server according to JS's
> string equality (*sirens*)
> * Try again.
> 
> 
> The server stores the static version of the user's email address, which is
> not necessarily lowercase.
> 
> If your provided email differs from the server's version, your stretched
> password will be wrong, and you'll get the correct email in the response.
> The client should use that directly. So this should read:
> 
>   if (!retryOK || (result.errno != ERRNO_INCORRECT_EMAIL_CASE)) {
>     return result;
>   }

Right, this is what I had completely misunderstood.  Thanks for the clarification.  I hope I've got it right now.

>   // TODO: Notify the rest of the system that the persisted email should be
> replaced.
>   let correctEmail = result.email;
>   return self.signIn(correctEmail, password, false);
> 
> ::: services/fxaccounts/FxAccountsCommon.js
> @@ +28,5 @@
> >  
> >    return log;
> >  });
> >  
> > +// XXX Sadly, CommonUtils.bytesAsHex doesn't handle typed arrays.
> 
> Put this in CommonUtils, and ideally fix bytesAsHex!

Done, with tests added.

> @@ +39,5 @@
> > +  return hex.join("");
> > +};
> > +
> > +this.stringToHex = function stringToHex(str) {
> > +  let encoder = new TextEncoder("utf-8");
> 
> CommonUtils.encodeUTF8(str)?

Yes, thank you.

> ::: services/fxaccounts/tests/xpcshell/test_client.js
> @@ +16,5 @@
> >      return deferred.promise;
> >  }
> >  
> > +add_task(function test_hexToString() {
> > +  do_check_eq("i like pie", hexToString(stringToHex("i like pie")));
> 
> Let's be a little more thorough here.
> 
> Check against a known-good implementation, or at least another
> implementation. Note that 
> 
>   function stringToHex(x) {
>     return x;
>   }
> 
> passes this test.
> 
> You also need to pass in some non-ASCII here.

I've added hexToString and stringAsHex to CommonUtils.

I've put a little round-trip test case in services/common/.../test_utils_convert_string.js and email-string-to-hex vectors in test_client.js to ensure parity with the fxa-js-client test suite.

Technically, I'm told we don't have to support non-ascii in email addresses, but I'd feel better if we could handle it just the same, and handle it in the same way as the fxa-js-client.

> @@ +236,5 @@
> >  });
> > +
> > +add_task(function test_email_case() {
> > +  let dbEmail = "greta.garbo@gmail.com";
> > +  let clientEmail = "Greta.Garbo@gmail.COM";
> 
> Let's use something that's a little more of an exercise:
> 
> services/common/tests/unit/test_utils_convert_string.js
> 29:  const INPUT = "Árvíztűrő tükörfúrógép いろはにほへとちりぬるを Pijamalı hasta,
> yağız şoföre çabucak güvendi.";

Added this to the CommonUtils tests, as described above.

> @@ +259,5 @@
> > +        let body = JSON.parse(
> > +          CommonUtils.readBytesFromInputStream(request.bodyInputStream));
> > +        let email = hexToString(body.email);
> > +
> > +        // If the client has the wrong case on the email, we return a 400, with
> 
> I thought this was a 401? Where's the spec?

It's a 400 - spec here: https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#response-1

But as you said above, it's more the errno that's really important anyway.
Thanks for your feedback, Richard.

I just need to make sure that the rest of the system gets properly notified of the revised email.  I think it does, if we're coming at this from the b2g accountsmanager, but I need to verify :)
Attachment #8367444 - Attachment is obsolete: true
Attachment #8368924 - Flags: feedback?(rnewman)
btw this is rebased over bug 957863 (the hawk time skew thing), which is landing
Comment on attachment 8368924 [details] [diff] [review]
handle incorrect email case in fxa client

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

::: services/common/utils.js
@@ +194,5 @@
>  
>    byteArrayToString: function byteArrayToString(bytes) {
>      return [String.fromCharCode(byte) for each (byte in bytes)].join("");
>    },
> +  

Trailing whitespace.

@@ +195,5 @@
>    byteArrayToString: function byteArrayToString(bytes) {
>      return [String.fromCharCode(byte) for each (byte in bytes)].join("");
>    },
> +  
> +  stringAsHex: function stringAsHex(str) {

This should probably be utf8StringAsHex, or something like that. Up to you precisely what name, but it ought to have utf8 in there somewhere :D

@@ +215,5 @@
>      }
>      return String.fromCharCode.apply(String, bytes);
>    },
>  
> +  hexToString: function hexToString(hex) {

Similarly.

Also, functions no longer need names:

  hexToString: function (hex) {

::: services/fxaccounts/FxAccountsClient.jsm
@@ +22,5 @@
>  
>  const HOST = _host;
>  const PROTOCOL_VERSION = "identity.mozilla.com/picl/v1/";
>  
> +let stringAsHex = CommonUtils.stringAsHex;

Inline this?

@@ +121,5 @@
> +    let self = this;
> +    return this._request("/raw_password/session/create", "POST", null, options)
> +      .then(
> +        (result) => {
> +          log.debug("signed in: " + JSON.stringify(result));

Don't leak credentials to the log.

@@ +137,5 @@
> +          //
> +          // API reference:
> +          // https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md
> +          if (ERRNO_INCORRECT_EMAIL_CASE === error.errno && retryOK) {
> +            // XXX notify the rest of the system that result.email is correct

File a bug or kill the comment.

@@ +138,5 @@
> +          // API reference:
> +          // https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md
> +          if (ERRNO_INCORRECT_EMAIL_CASE === error.errno && retryOK) {
> +            // XXX notify the rest of the system that result.email is correct
> +            return self.signIn(error.email, password, false);

Validate that error.email is provided first, and error if it's not.
Attachment #8368924 - Flags: feedback?(rnewman) → feedback+
This is a FxOS only bug at this point and should not block Fx29. This issue is handled for Desktop in the hosted page for about:accounts.

This also needs to be rebased on top of bug 943521.
No longer blocks: 969593
Sam, this should be moved as a dependency of FxA on FxOS (bug 941723) somewhere.
Flags: needinfo?(spenrose)
OK, I'm putting under a meta bug for Gecko blockers on FxA/FxOS 1.4. Anyone who is confused by this please holler.
Blocks: 955951
Flags: needinfo?(spenrose)
Rebased over Bug 943521, which is landing.

Responses to comment 17 and further updates coming soon.
Attachment #8368924 - Attachment is obsolete: true
Rebased over patches for Bug 967372 and Bug 969892
Attachment #8372761 - Attachment is obsolete: true
Component: Firefox Sync: Backend → FxA
Product: Mozilla Services → Core
Status: NEW → ASSIGNED
WIP

Added mochitests for browser builds (mochitest does not run on b2g)

Fixing/updating b2g xpcshell tests now
Attachment #8372900 - Attachment is obsolete: true
WIP.  Fixed the test I broke.  One more xpcshell test to work out.
Attachment #8377956 - Attachment is obsolete: true
Adds an xpcshell test for b2g/components to test FxAccountsMgmtService signIn with incorrect email capitalization.
Attachment #8377982 - Attachment is obsolete: true
This patch is part 1 of 2.  It does the following:

- retry signIn with corrected email capitalization
- update and fix some error codes in FxAccountsCommon
- provide mochitest-chrome for signIn test
- add email capitalization test to xpcshell tests

Thanks for your review, Richard
Attachment #8378130 - Attachment is obsolete: true
Attachment #8378381 - Flags: review?(rnewman)
This patch is part 2 of 2.  Apply over the previous.  It does the following:

- add new test_fxaccounts xpcshell test to b2g/components
  - test that signIn correctly handles initially-incorrect email capitalization
- expose the fxAccounts client in the FxAccountsManager
- update test_manager xpcshell test

One particular question I have: Why, when I run this test, do I get a dozen message like the following?

0:02.93 ************************************************************
 0:02.93 * Call to xpconnect wrapped JSObject produced this error:  *
 0:02.93 [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///Users/zeus/code/firefox/build_inbound_b2g/dist/bin/components/nsHandlerService.js :: HandlerService.prototype._ds :: line 891"  data: no]
 0:02.93 ************************************************************

The test passes, and no leaks are detected.  But I don't know what I'm doing to trigger this message.  Thanks for any insight, and thanks as always for your review, Fernando! j
Attachment #8378384 - Flags: review?(ferjmoreno)
The tbpl in Comment 26 is the combination of these patches.  I thought splitting the patch into two would make review easier and more straightforward, but the code is the same.
Comment on attachment 8378381 [details] [diff] [review]
part 1: fxaccounts: handle incorrect email case in fxa client

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

LGTM. Thanks for the thorough tests!

::: services/fxaccounts/FxAccountsClient.jsm
@@ +125,5 @@
> +            if (!error.email) {
> +              log.error("Server returned errno 120 but did not provide email");
> +              throw error;
> +            }
> +            // XXX notify the rest of the system that result.email is correct

Can this comment die, given that we include the email in the result object?

::: services/fxaccounts/tests/mochitest/file_invalidEmailCase.sjs
@@ +18,5 @@
> + *
> + * On success, the client is responsible for updating its sign-in user state
> + * and recording the proper email capitalization.
> + */
> +

use strict

@@ +31,5 @@
> +function handleRequest(request, response) {
> +  var body = new BinaryInputStream(request.bodyInputStream);
> +  var bytes = [];
> +  while ((available = body.available()) > 0)
> +    Array.prototype.push.apply(bytes, body.readByteArray(available));

Style nits: `let`, braces around `while` bodies.

::: services/fxaccounts/tests/mochitest/test_invalidEmailCase.html
@@ +20,5 @@
> +<div id="content" style="display: none">
> +  Test for correction of invalid email case in Fx Accounts signIn
> +</div>
> +<pre id="test">
> +<script class="testbody" type="text/javascript;version=1.7">

1.8 was released in Firefox 3. :P

@@ +31,5 @@
> +Components.utils.import("resource://gre/modules/FxAccountsClient.jsm");
> +Components.utils.import("resource://services-common/hawk.js");
> +
> +const TEST_SERVER =
> +  'http://mochi.test:8888/chrome/services/fxaccounts/tests/mochitest/file_invalidEmailCase.sjs?path=';

Double quotes.
Attachment #8378381 - Flags: review?(rnewman) → review+
(In reply to Richard Newman [:rnewman] from comment #30)
> Comment on attachment 8378381 [details] [diff] [review]
> part 1: fxaccounts: handle incorrect email case in fxa client
> 
> Review of attachment 8378381 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM. Thanks for the thorough tests!

And thank you for your review!

> ::: services/fxaccounts/FxAccountsClient.jsm
> @@ +125,5 @@
> > +            if (!error.email) {
> > +              log.error("Server returned errno 120 but did not provide email");
> > +              throw error;
> > +            }
> > +            // XXX notify the rest of the system that result.email is correct
> 
> Can this comment die, given that we include the email in the result object?

Yes, sorry.  I hadn't intended to leave that there.

> ::: services/fxaccounts/tests/mochitest/file_invalidEmailCase.sjs
> @@ +18,5 @@
> > + *
> > + * On success, the client is responsible for updating its sign-in user state
> > + * and recording the proper email capitalization.
> > + */
> > +
> 
> use strict

I don't think you can actually declare "use strict"; in an sjs file.  I tried, and it causes the server to 500.  I checked mxr, and there are no sjs files that "use strict";, so I think this has to stay as it is.

> @@ +31,5 @@
> > +function handleRequest(request, response) {
> > +  var body = new BinaryInputStream(request.bodyInputStream);
> > +  var bytes = [];
> > +  while ((available = body.available()) > 0)
> > +    Array.prototype.push.apply(bytes, body.readByteArray(available));
> 
> Style nits: `let`, braces around `while` bodies.

Oh gross.  Thanks.

> ::: services/fxaccounts/tests/mochitest/test_invalidEmailCase.html
> @@ +20,5 @@
> > +<div id="content" style="display: none">
> > +  Test for correction of invalid email case in Fx Accounts signIn
> > +</div>
> > +<pre id="test">
> > +<script class="testbody" type="text/javascript;version=1.7">
> 
> 1.8 was released in Firefox 3. :P

Heh.  Yeah.

> @@ +31,5 @@
> > +Components.utils.import("resource://gre/modules/FxAccountsClient.jsm");
> > +Components.utils.import("resource://services-common/hawk.js");
> > +
> > +const TEST_SERVER =
> > +  'http://mochi.test:8888/chrome/services/fxaccounts/tests/mochitest/file_invalidEmailCase.sjs?path=';
> 
> Double quotes.

Fixed here and elsewhere.

Thanks again, Richard!
j
r=rnewman

This patch can be checked in.
Attachment #8378381 - Attachment is obsolete: true
Attachment #8378563 - Flags: checkin?(ryanvm)
Comment on attachment 8378563 [details] [diff] [review]
part 1: fxaccounts: handle incorrect email case in fxa client

Needs rebasing.
Attachment #8378563 - Flags: checkin?(ryanvm) → checkin-
Please use a generic checkin? request too so I don't get CCed :). I promise you I'll still find it!
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #33)
> Comment on attachment 8378563 [details] [diff] [review]
> part 1: fxaccounts: handle incorrect email case in fxa client
> 
> Needs rebasing.

Oh wow! Already? I just rebased like 4 hours ago.  Sorry!
r=rnewman

rebased over mozilla-central
Attachment #8378563 - Attachment is obsolete: true
Attachment #8378600 - Flags: checkin?
What timing.  Just as I did that, Bug 938635, which which it was in conflict. landed in inbound.  So this rebased patch should now apply cleanly on both branches.
Flags: in-testsuite+
Comment on attachment 8378384 [details] [diff] [review]
part 2: b2g: handle incorrect email case in fxa client

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

Sorry for the late review.

Looks great! Thanks Jed, and thanks for the tests!

::: b2g/components/FxAccountsMgmtService.jsm
@@ +38,5 @@
>      log.debug("Chrome event " + JSON.stringify(aMsg));
>      this._shell.sendCustomEvent(aEventName, aMsg);
>    },
>  
> +  _onFulfill: function(aMsgId, aData) {

:) thanks!

::: b2g/components/test/unit/test_fxaccounts.js
@@ +2,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +const {classes: Cc, interfaces: Ci, results: Cr, utils: Cu} = Components;

You are only using 'Cu'
Attachment #8378384 - Flags: review?(ferjmoreno) → review+
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #40)
> Comment on attachment 8378384 [details] [diff] [review]
> part 2: b2g: handle incorrect email case in fxa client
> 
> Review of attachment 8378384 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the late review.

No worries.  Thanks as always for your review, Fernando!

> Looks great! Thanks Jed, and thanks for the tests!

Awesome.  Thanks.

> ::: b2g/components/test/unit/test_fxaccounts.js
> @@ +2,5 @@
> > + * http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +"use strict";
> > +
> > +const {classes: Cc, interfaces: Ci, results: Cr, utils: Cu} = Components;
> 
> You are only using 'Cu'

Oops.  Thanks.

Cheers! j
Comment on attachment 8379790 [details] [diff] [review]
part 2: b2g: handle incorrect email case in fxa client

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

Try to use more descriptive commit messages in the future :)
Attachment #8379790 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/85739150d974
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Comment on attachment 8379790 [details] [diff] [review]
part 2: b2g: handle incorrect email case in fxa client

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined:
  firefox accounts and sync might fail for some users
Testing completed (on m-c, etc.):
  yes; includes automated tests, and has been stable on m-c and m-i
Risk to taking this patch (and alternatives if risky):
  low
String or IDL/UUID changes made by this patch:
  none
Attachment #8379790 - Flags: approval-mozilla-aurora?
Comment on attachment 8379790 [details] [diff] [review]
part 2: b2g: handle incorrect email case in fxa client

Do we really want this for 29?

As I understand it is has no benefits on Desktop (comment 18). Does it risk breaking things there?

I don't think b2g cares about what's on Aurora 29 (though someone correct me if I'm wrong).

Not landing it also potentially increases uplift friction for any future Desktop-related FxAccountsManager.jsm changes, which is an argument in favor of uplift, but I don't know how strong.

Jed, Mark, what do you think?
No immediate benefit, low risk.
Comment on attachment 8379790 [details] [diff] [review]
part 2: b2g: handle incorrect email case in fxa client

Not going to bother with this.
Attachment #8379790 - Flags: approval-mozilla-aurora?
/me rolls sleeves back down
Product: Core → Firefox
Target Milestone: mozilla30 → Firefox 30
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: