958546 - s/isVerified/verified in all FxAccounts related code

Status: RESOLVED FIXED

(Firefox OS Graveyard :: FxA, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

I'm not sure when this changed or if it always was like this, but it seems that the server is not sending 'isVerified' but 'verified' in responses like [1].

Among other potential errors, this makes every login request poll for email verification even if the email was already verified:

Gecko  I  1389366295720	FirefoxAccounts	DEBUG	(Response) Code: 200 - Status text: OK - Response text: {"uid":"9b6e44b8e67a4647bfe13595c06d2f9b","verified":true,"sessionToken":"a17640dfd68afdb2df7aeaf32341d44eb97fc3bab8fda68443d910ce3335410d"}
Gecko  I  1389366295746	FirefoxAccounts	DEBUG	setSignedInUser - aborting any existing flows
Gecko  I  1389366295747	FirefoxAccounts	DEBUG	generationCount: 9
Gecko  I  1389366295774	FirefoxAccounts	DEBUG	Notifying observers of fxaccounts:onlogin
Gecko  I  1389366295776	FirefoxAccounts	DEBUG	startVerifiedCheck {"uid":"9b6e44b8e67a4647bfe13595c06d2f9b","verified":true,"sessionToken":"a17640dfd68afdb2df7aeaf32341d44eb97fc3bab8fda68443d910ce3335410d","email":"[e-mail]"}
Gecko  I  1389366295778	FirefoxAccounts	DEBUG	whenVerified promise starts polling for verified email
Gecko  I  1389366295779	FirefoxAccounts	DEBUG	entering pollEmailStatus: start 9

[1] https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#response-1

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Comment 2

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

I don't quite get how this fixes the problem.  After an initial login, the data I see coming back from the login page *does not* include either 'verified' or 'isVerified' keys - just keys of "sessionToken", "email","keyFetchToken" and ,"unwrapBKey".  In your patch, you touch the block:

        if (response && response.verified) {
...
              data.isVerified = true;

so that code already is expecting that response to have .verified and translating that to isVerified.

IOW, while I could agree this might be nice from a consistency POV, I fail to see how it fixes the bug - and the patch doesn't add a test that demonstrates one.  Asking warner and ckarlof for moreinfo incase they have additional insights...

Comment 3

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

(In reply to Mark Hammond [:markh] from comment #2)
> so that code already is expecting that response to have .verified and
> translating that to isVerified.

To clarify: that code is expecting *that other* response to have .verified...

Comment 4

[Chris Karlof [:ckarlof]]

A couple of points:

1) :ferjm is developing for FxOS, which uses the login API directly and not the hosted login page, and has access to the verified flag in the /account/login response. Desktop login relies on the hosted login page to do this. :ferjm is likely seeing the polling because of these naming mismatch (code looks for "isVerified", but "verified" gets written to the accounts file after login). If I understand this correctly, the :ferjm's patch should address this. 

2) We could add the verified flag to the information provided by the hosted login page to Desktop Firefox. This would save it a call to /recovery_email/status.

3) :ferjm's patch is touching some of the tests for the remote command from the hosted login page. Those tests have bitrotted, it seems. In particular, the "login" event doesn't provide kA or kB anymore and seems to have dropped "(is)verified".

Comment 5

[Chris Karlof [:ckarlof]]

In summary, I think :ferjm's patch is reasonable, but I think we should update /browser/base/content/test/general/accounts_testRemoteCommands.html to more reflect reality of the about:accounts hosted page <-> browser communication.

Comment 6

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

Comment on attachment 8358429 [details] [diff] [review]
v1

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

OK, I see no harm that can come from this patch and I'll take it as read that it solves a problem for FxOS

Comment 7

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Chris Karlof [:ckarlof] from comment #5)
> In summary, I think :ferjm's patch is reasonable, but I think we should
> update /browser/base/content/test/general/accounts_testRemoteCommands.html
> to more reflect reality of the about:accounts hosted page <-> browser
> communication.

Thanks Chris! I filed bug 959626 for the tests.

And thanks Mark for the review!

https://hg.mozilla.org/integration/b2g-inbound/rev/cdb827187aa8

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Try was green btw https://tbpl.mozilla.org/?tree=Try&rev=ce0c6ecc0572

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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