Closed Bug 981921 Opened 6 years ago Closed 6 years ago

Allow signin in FxAccountsClient to use ?keys=true

Categories

(Firefox :: Firefox Accounts, defect)

29 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- wontfix
firefox30 --- fixed

People

(Reporter: ckarlof, Assigned: whimboo)

References

Details

(Whiteboard: [backport for firefox-29 blocked by bug 943521])

Attachments

(1 file, 1 obsolete file)

This will allow TPS to sign in to Sync via the FxAccountsClient. Currently, the client does not use ?keys=true in the endpoint URL, and TPS is using a hard-coded kB.
Chris, can this be solved by just adding the key property to the data dictionary here?
http://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccountsClient.jsm#100

I can't try this at the moment given that the Firefox is currently build and it will take a while to be finished. Would be great to get this information infront.
Flags: needinfo?(ckarlof)
Looks like it is not the case. So as filed backend work is necessary here.
https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#post-v1accountlogin
Flags: needinfo?(ckarlof)
Close. I believe we just need to change 

> this._request("/account/login", "POST", null, data)

to 

> this._request("/account/login?keys=true", "POST", null, data)

at http://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccountsClient.jsm#104
Alright. That works perfectly. I will come up with a patch for that. Once it is in the product I will update tps to retrieve and decode the keys, so we don't have to hard-code them.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
This patch adds the optional usage of keys=true similar to the web client (https://github.com/mozilla/fxa-js-client/blob/master/client/FxAccountClient.js#L107). I have also added some more tests to ensure we don't regress that.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=cdba8a74bf8d
Attachment #8389730 - Flags: review?(ckarlof)
Attached patch Patch v1.1Splinter Review
Sorry, the last patch missed the latest commit. This one also includes the missing jsdoc, and another enhancements for the xpcshell tests.
Attachment #8389730 - Attachment is obsolete: true
Attachment #8389730 - Flags: review?(ckarlof)
Attachment #8389734 - Flags: review?(ckarlof)
Blocks: 982591
Comment on attachment 8389734 [details] [diff] [review]
Patch v1.1

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

Looks good, and thanks for the cleanup and improvements in the docs and tests!

It might be worth pushing to try to make sure the b2g tests pass.
Attachment #8389734 - Flags: review?(ckarlof) → review+
Chris, thank you for the review. Has anyone else to review or can I get the patch landed? Given that we should get those pieces quickly into m-c, can I land that directly on inbound? Or am I forced to land it in services-central?

(In reply to Chris Karlof [:ckarlof] from comment #7)
> It might be worth pushing to try to make sure the b2g tests pass.

I already did this as mentioned in comment 5. The follow-up fixes were small, and shouldn't change anything. All is green, yay.
Flags: needinfo?(ckarlof)
> Chris, thank you for the review. Has anyone else to review or can I get the patch landed? Given that we should get those pieces quickly into m-c, can I land that directly on inbound? Or am I forced to land it in services-central?

No other review necessary. inbound is fine.
Flags: needinfo?(ckarlof)
inbound is currently closed due to bustage. So I will push early tomorrow.
https://hg.mozilla.org/integration/mozilla-inbound/rev/478d940d4f1d
Flags: in-testsuite+
Target Milestone: --- → mozilla30
https://hg.mozilla.org/mozilla-central/rev/478d940d4f1d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Chris, I think we should get this backported to aurora, right? All the changes regarding tps should get part of beta in a week.
Flags: needinfo?(ckarlof)
> Chris, I think we should get this backported to aurora, right?

yes
Flags: needinfo?(ckarlof)
Comment on attachment 8389734 [details] [diff] [review]
Patch v1.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): TPS for FxA Sync
User impact if declined: reduced Sync test coverage for FxA Sync in 29
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none
Attachment #8389734 - Flags: approval-mozilla-aurora?
Attachment #8389734 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This has some major conflicts on Aurora.
Flags: needinfo?(hskupin)
Looks like bug 943521 is the main source of conflicts, FWIW.
Right. I already commented over there earlier today. Lets see if we can do backport that patch and dependencies. If not I will have to rewrite the code here for the old protocol. :/
Depends on: 943521
Flags: needinfo?(hskupin)
Whiteboard: [backport for firefox-29 blocked by bug 943521]
I was unable to apply Bug 943521 to Aurora either. Rewriting for the old protocol isn't going to work because the old protocol isn't supported in the server. I think your best bet is to manually re-work this patch to apply to aurora. I don't think it would be much work.
Component: Firefox Sync: Backend → FxA
Product: Mozilla Services → Core
Version: unspecified → 29 Branch
Chris, I was talking to jed on IRC today and we wonder if we should really get this backported. Also given that the code is in beta now. It's a small change but if you think we are good in hard-coding the keys for Firefox 29, I would say lets skip this backport and have it fixed in Firefox 30. If we should see major issues with the API and we need tests on beta, we could always request approval. What do you think?
Flags: needinfo?(ckarlof)
More tests are usually a good idea. :) 

The hard-coded keys will certainly not work if you ever change the password on the account, but I think it should be sufficient for testing purposes. What are the downsides from your POV of hard-coding the keys?
Flags: needinfo?(ckarlof)
Ok, so let me figure out first how to get the the retrieval of keys working on bug 982591. We can then consider backporting both of these patches. Given that lack of documentation I can't give a qualified answer at this time. I have to dig into that code and check how it works first.
Lets not track this for Firefox 29. It's lower priority and we might not need a backport here. But once we figure out something is broken in beta, we have to backport. So I will leave the firefox 29 affected flag active.
no more branch-patch-needed
Product: Core → Firefox
Target Milestone: mozilla30 → Firefox 30
You need to log in before you can comment on or make changes to this bug.