Closed Bug 910479 Opened 7 years ago Closed 7 years ago

Store user credentials after Firefox Account creation/log in

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: zaach, Assigned: zaach)

References

()

Details

(Whiteboard: [qa+])

Attachments

(1 file, 4 obsolete files)

We'll need to call the FxAccounts service to save the user's credentials once we complete the sign-in/create account flows.
Save them where?
(had to ask) 
local storage on the client?
server-side?
Whiteboard: [qa+]
(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.
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.
OS: Mac OS X → All
Hardware: x86 → All
Note: The FxAccounts module currently stores everything to a file in the user's profile directory. See Bug #909967
Attachment #800282 - Flags: feedback?(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">
Attachment #800282 - Flags: feedback?(rnewman) → feedback+
Now uses the XPCOM service to obtain an instance.
Attachment #800282 - Attachment is obsolete: true
Attachment #802604 - Flags: review?(gavin.sharp)
Style and documentation updates.
Attachment #802604 - Attachment is obsolete: true
Attachment #802604 - Flags: review?(gavin.sharp)
Attachment #803327 - Flags: review?(gavin.sharp)
Blocks: 915453
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.
Attachment #803327 - Attachment is obsolete: true
Attachment #803327 - Flags: review?(gavin.sharp)
Attachment #807959 - Flags: review?(gavin.sharp)
No longer blocks: 915453
No longer blocks: 906028
Attachment #807959 - Flags: review?(gavin.sharp) → review+
r=gavin in person.
Attachment #807959 - Attachment is obsolete: true
Attachment #816795 - Flags: review+
https://hg.mozilla.org/projects/elm/rev/258a0a22e62b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Adding to my stack of Resolved bugs to review...
I expect the build will be on the ELM site by tomorrow morning...
Blocks: 951296, 911378
No longer blocks: 905997, 907415
No longer blocks: 911378
Component: Firefox Sync: UI → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.