910479 - Store user credentials after Firefox Account creation/log in

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Zachary Carter [:zaach]]

We'll need to call the FxAccounts service to save the user's credentials once we complete the sign-in/create account flows.

Comment 1

[James Bonacci [:jbonacci]]

Save them where?
(had to ask) 
local storage on the client?
server-side?

Comment 2

[Zachary Carter [:zaach]]

(In reply to James Bonacci [:jbonacci] from comment #1)
> Save them where?
> (had to ask) 
> local storage on the client?
> server-side?

Save them to the user's profile on disk. We'll need to access these credentials from chrome to auth with the Sync 2.0 token server.

Comment 3

[Richard Newman [:rnewman]]

Some choices here with historical pointers:

* Password manager: kinda sucks. Some users wipe on shutdown (Bug 553400), you need hacks to unlock it with MP enabled (Bug 671422), even with Master Password it's weak, and it doesn't extend well to sign-in-to-browser.
* Use the system keychain. Bug 638966.
* Flat files. Need to consider if these are encrypted, and whether they live inside a profile directory or outside it (again, for SITB).
* ???

N.B., credentials will be stored in an entirely different manner on Android, so this only applies to desktop, Metro, and perhaps FxOS.

Comment 4

[Zachary Carter [:zaach]]

Attachment: Send credentials from jelly to the FxAccount module for storage

Note: The FxAccounts module currently stores everything to a file in the user's profile directory. See Bug #909967

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 800282 [details] [diff] [review]
Send credentials from jelly to the FxAccount module for storage

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

Looks fine within the context in which the patch lives, but I have some questions about that context inline!

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +6,5 @@
>  
>  const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  
>  Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/FxAccounts.jsm", this);

Doesn't need the scope argument.

@@ +45,5 @@
>      this.injectData("message", { status: "create" });
>    },
>  
>    onVerified: function (data) {
>      log("Received: 'verified'. Data:" + JSON.stringify(data));

Be careful about logging user credentials.

@@ +47,5 @@
>  
>    onVerified: function (data) {
>      log("Received: 'verified'. Data:" + JSON.stringify(data));
> +
> +    FxAccounts.Accounts.setSignedInUser(data).then(function () {

Firstly: I have a preference for not directly using singletons here. (More a comment on Bug 909967 than this bug.) It makes testing harder, makes swapping out implementations more difficult, makes use from C++ more difficult, yadda yadda.

Secondly: I'm probably lacking some context here, but it seems to me that there's a distinction between *recording a valid account in the Accounts system* and *making one of those accounts current*.

I think this API conflates the two -- that is, what does this do with the current account? Does it throw it away, save it for later, or something else?

Might be worth thinking about that in the context of SITB…

@@ +49,5 @@
>      log("Received: 'verified'. Data:" + JSON.stringify(data));
> +
> +    FxAccounts.Accounts.setSignedInUser(data).then(function () {
> +      this.injectData("message", { status: "verified" });
> +    }.bind(this), function(err) {

function () {
  this.injectData("message", {status: "verified"});
}.bind(this);

is equivalent to

  this.injectData.bind(this, "message", {status: "verified"});

which will make this callback hell a lot easier to read!

::: browser/base/content/test/accounts_testRemoteCommands.html
@@ +76,5 @@
>    window.parent.postMessage(data, "*");
>  }
>  
>  function sendToBrowser(type) {
> +  var event = new CustomEvent("FirefoxAccountsCommand", {detail: {command: type, data: {}}, bubbles: true});

You can use modern JS language features such as `let` by using

     <script type="text/javascript;version=1.8">

Comment 6

[Zachary Carter [:zaach]]

Attachment: 910479_store_user_credentials-2.patch

Now uses the XPCOM service to obtain an instance.

Comment 7

[Zachary Carter [:zaach]]

Attachment: 910479_store_user_credentials-3.patch

Style and documentation updates.

Comment 8

[Zachary Carter [:zaach]]

Attachment: 910479_store_user_credentials-4.patch

Updates the wrapper to use the FxAccounts.jsm module instead of the service. Stylistic changes to the communication channel between jelly and wrapper that Gavin brought up in #910844 will be addressed in that bug.

Comment 9

[Zachary Carter [:zaach]]

Attachment: 910479_store_user_credentials-5.patch

r=gavin in person.

Comment 10

[Zachary Carter [:zaach]]

https://hg.mozilla.org/projects/elm/rev/258a0a22e62b

Comment 11

[James Bonacci [:jbonacci]]

Adding to my stack of Resolved bugs to review...
I expect the build will be on the ELM site by tomorrow morning...

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/258a0a22e62b