s/isVerified/verified in all FxAccounts related code

RESOLVED FIXED in 1.3 C2/1.4 S2(17jan)

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ferjm, Assigned: ferjm)

Tracking

unspecified
1.3 C2/1.4 S2(17jan)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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":"c4134836@drdrb.com"}
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
(Assignee)

Updated

5 years ago
Assignee: nobody → ferjmoreno
(Assignee)

Updated

5 years ago
Summary: s/isVerified/verified → s/isVerified/verified in all FxAccounts related code
(Assignee)

Updated

5 years ago
Attachment #8358429 - Flags: review?(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...
Flags: needinfo?(warner-bugzilla)
Flags: needinfo?(ckarlof)
(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...
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".
Flags: needinfo?(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 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
Attachment #8358429 - Flags: review?(mhammond) → review+

Updated

5 years ago
Flags: needinfo?(warner-bugzilla)
(Assignee)

Comment 7

5 years ago
(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
https://hg.mozilla.org/mozilla-central/rev/cdb827187aa8
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
You need to log in before you can comment on or make changes to this bug.