Last Comment Bug 774300 - Sync authentication errors if passwords contain non-ASCII characters
: Sync authentication errors if passwords contain non-ASCII characters
Status: VERIFIED FIXED
[qa+]
:
Product: Android Background Services
Classification: Client Software
Component: Android Sync (show other bugs)
: unspecified
: ARM Android
: P1 normal
: mozilla17
Assigned To: Richard Newman [:rnewman]
:
:
Mentors:
: 779190 (view as bug list)
Depends on: 709395 709393
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-16 08:40 PDT by Alexandre Provencio
Modified: 2013-04-04 13:48 PDT (History)
8 users (show)
rnewman: in‑litmus?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
+
verified


Attachments
trying to sync with a fresh firefox profile (68.57 KB, text/plain)
2012-07-16 08:40 PDT, Alexandre Provencio
no flags Details
Proposed patch. v1 (8.42 KB, patch)
2012-08-02 12:42 PDT, Richard Newman [:rnewman]
rnewman: review+
Details | Diff | Splinter Review
Patch for Aurora. v1 (5.43 KB, patch)
2012-08-02 22:09 PDT, Richard Newman [:rnewman]
rnewman: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Alexandre Provencio 2012-07-16 08:40:49 PDT
Created attachment 642601 [details]
trying to sync with a fresh firefox profile

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:13.0) Gecko/20100101 Firefox/13.0.1
Build ID: 20120615112143

Steps to reproduce:

- confirmed wether sync service was ok on https://services.mozilla.com/status/
- confirmed that I'm able to sync the firefox on my laptop (ubuntu) with another firefox on a deskop (mac)
- have never installed other android firefox versions
- have already hit 'clear your sync data' on https://account.services.mozilla.com/
- tried to take the addons off the sync
- tried the recover key method
- tried reinstalling android firefox
- tried to set up sync under two scenarios:
    1) my usual firefox account on my laptop
    2) a fresh new firefox profile on my laptop (logfile attached)


Actual results:

1) Sync seems to be working ok under settings->sync->Firefox and hitting 'sync now' generates no errors, but nothing is really synced
2) Sync generates a syncing problem message when doing the same test above and also nothing is synced


Expected results:

The tabs, bookmarks, history, etc. on my laptop should show on android's firefox
Comment 1 Aaron Train [:aaronmt] 2012-07-16 08:45:50 PDT
I see

W/GlobalSession( 4266): Aborting sync: User password has changed.

Are you sure you have the right password?
Comment 2 Alexandre Provencio 2012-07-17 05:20:48 PDT
This is very odd because the android device never asks me to type a password, I only do that on the laptop. In addiction firefox on laptop was syncing ok with other desktop versions.

However I took you suggestion, and remembered my password had a 'ã' character on it, so I manage to change my password to a less secure but less error prone one, and... it's working! Even with my fully loaded profile.

Maybe you can explain better what really happened here, but my guess really is this accented letter. Hope this can help other users, and thanks a lot for your time :)
Comment 3 Aaron Train [:aaronmt] 2012-07-17 06:13:36 PDT
Alexandre, sounds good, glad to help.

Nick, is there client work to be done here to improve this?
Comment 4 Nick Alexander :nalexander (leave until January 2017) 2012-07-17 08:06:21 PDT
(In reply to Aaron Train [:aaronmt] from comment #3)
> Alexandre, sounds good, glad to help.
> 
> Nick, is there client work to be done here to improve this?

I can think of a few things:

1. we shouldn't create an Account with null credentials. Ever.
2. we could be surfacing Account errors better: prompting for credentials, or at least showing red sync failure.
3. we could QA that usernames/passwords/URLs with "exotic" characters work.

We can probably test problems with 3. in our test code.
Comment 5 Nick Alexander :nalexander (leave until January 2017) 2012-07-17 08:11:04 PDT
I think Bug 709393 is most relevant.
Comment 6 Richard Newman [:rnewman] 2012-07-17 08:58:00 PDT
JSON decoding issue in J-PAKE transfer?
Comment 7 Richard Newman [:rnewman] 2012-07-17 10:08:16 PDT
(In reply to Nick Alexander :nalexander from comment #5)
> I think Bug 709393 is most relevant.

If the user set up with J-PAKE, we should have the correct password. That's why Bug 709393 has not been a high priority: incorrect credentials should be rare.
Comment 8 Richard Newman [:rnewman] 2012-07-31 10:55:21 PDT
Desktop does some really crappy encoding, and I'm sure it's tripping us up along the way. Writing a test now.
Comment 9 Richard Newman [:rnewman] 2012-07-31 10:55:38 PDT
*** Bug 779190 has been marked as a duplicate of this bug. ***
Comment 10 Richard Newman [:rnewman] 2012-07-31 11:47:16 PDT
Firstly, we need to use UTF-8 as the encoding in BasicScheme. I don't know why that would be the case.

Secondly, we need to get the un-mangled desktop password.

*sigh*
Comment 11 Richard Newman [:rnewman] 2012-08-02 12:42:16 PDT
Created attachment 648444 [details] [diff] [review]
Proposed patch. v1

Reviewed by nalexander. Testing now, then I'll land and request uplift.
Comment 12 Richard Newman [:rnewman] 2012-08-02 22:04:16 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f0334d53fd2

I verified this with J-PAKE pairing an inbound build with a non-ASCII password.

QA, please verify this when it lands, and ensure that our test suite includes passwords like "éfoöb£ar".
Comment 13 Richard Newman [:rnewman] 2012-08-02 22:07:19 PDT
And remove a test that I checked in to m-i by accident:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ecf2a75d1e5
Comment 14 Richard Newman [:rnewman] 2012-08-02 22:09:11 PDT
Created attachment 648614 [details] [diff] [review]
Patch for Aurora. v1

Patch for future uplift.
Comment 16 Tracy Walker [:tracy] 2012-08-03 12:53:38 PDT
Just so I am clear, the STR's are:

1) Create an account with a non-ASCII password such as "éfoöb£ar"
2) Pair mobile device, via J-PAKE, to that account

Pairing and sync should just work.
Comment 17 Richard Newman [:rnewman] 2012-08-03 13:20:23 PDT
(In reply to Tracy Walker [:tracy] from comment #16)
> Just so I am clear, the STR's are:
> 
> 1) Create an account with a non-ASCII password such as "éfoöb£ar"
> 2) Pair mobile device, via J-PAKE, to that account
> 
> Pairing and sync should just work.

Exactly that.
Comment 18 Tracy Walker [:tracy] 2012-08-07 13:12:45 PDT
looks good on latest m-c build
Comment 19 Richard Newman [:rnewman] 2012-08-08 10:30:06 PDT
Comment on attachment 648614 [details] [diff] [review]
Patch for Aurora. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Oooold code.

User impact if declined: 
  Users with non-ASCII passwords won't be able to successfully set up Sync.

Testing completed (on m-c, etc.): 
  QA verified on m-c (thanks tracy!). Covered by unit tests.

Risk to taking this patch (and alternatives if risky): 
  Slim. Should have blown up catastrophically if there were any problems.

String or UUID changes made by this patch: 
  None.
Comment 20 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-09 10:09:06 PDT
Comment on attachment 648614 [details] [diff] [review]
Patch for Aurora. v1

letting users use non-ASCII chars in their passwords is pretty important, approving for branches.
Comment 22 Richard Newman [:rnewman] 2012-08-12 23:48:51 PDT
Tracy, would you be so kind as to verify Aurora and Beta when these changes filter through?
Comment 23 Tracy Walker [:tracy] 2012-08-13 10:28:54 PDT
looks good Aurora (desktop) to Aurora (mobile)

Had a little scare with Beta 'til rnewman reminded me beta won't have the fix 'til tomorrow Fx15b5.

Note You need to log in before you can comment on or make changes to this bug.