Closed Bug 642660 Opened 9 years ago Closed 9 years ago

Add API for storing oauth tokens and account profiles

Categories

(Cloud Services :: Share: Firefox Client, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: philikon, Assigned: philikon)

References

Details

Attachments

(5 files, 2 obsolete files)

Figure out how to store these securely. This also needs to respect the user's expectations around account clearing (e.g. wipe cookies).
Blocks: 642656
The Services team has been asked to integrate the "F1" add-on into Firefox. F1 is a little bit of chrome code that loads a web client and then talks to it using postMessage. Right now the chrome level code is completely oblivious to the account profiles and oauth tokens. The web client simply stores it in its localStorage (bug 642980 has an example of the kind of data stored there.) There are some problems with this:

* Some add-ons prevent localStorage from working, so right now they prevent F1 from working.
* Clearing your cookies/credentials/... or whatever the right analogy is won't clear your F1 credentials.
* The master password doesn't protect the F1 credentials, so somebody who takes over another person's MP-locked computer, although they can't log into Facebook, can post to their wall using F1.

A good long term solution would be based on bug 643627, but since this is slated for Firefox 5, we need to make due with a short term solution. I see two

* Simply (ab)use the password manager. Sucks because the schema doesn't quite fit, queries are awkward and on top of that it does synchronous I/O. Do not want.
* Write a JSON file to disk, after encrypting it with nsILoginManagerCrypto. That, AFIUI, prompts the MP when needed and does the right thing (nsILoginManager uses it, after all.) Then we can do our own schema and async I/O.

Thoughts?
Just a small correction (not the main issue of this ticket): we have found that clearing cookies, when set to "Everything" does clear the localStorage values.
I have implemented a small component to encrypt the JSON data (any string data, really, but it'll be the JSON that we get passed in from the Web code) and write it to a file in the profile directory: https://github.com/mozilla/f1/commit/15d01c95fd3f62dee3254c62b58d5dee03e6cca9

The API is dead simple: AccountStore.store(data, callback) and AccountStore.fetch(callback). We would then hook this up two new Panel API calls  which the Web bit could then access through postMessage. James & Shane, does that work for you guys?
Moving away from GitHub for the chrome code since we're integrating it into Firefox. This sets up the xpcshell test harness.
Attachment #522744 - Flags: review?(sdwilsh)
Comment on attachment 522744 [details] [diff] [review]
Part 0 (v1): Preq: xpcshell test harness

r=sdwilsh
Attachment #522744 - Flags: review?(sdwilsh) → review+
Comment on attachment 522745 [details] [diff] [review]
Part 1 (v1): Storage component

>+  /**
>+   * Fetch data from the file.
>+   *
>+   * @param callback Function that's called with the following parameters:
>+   *                 - status code
>+   *                 - resulting data as string
>+   */
global-nit: I believe the preferred style for these is like this:
https://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#63

>+    let outputStream = Cc["@mozilla.org/network/safe-file-output-stream;1"]
>+                         .createInstance(Ci.nsIFileOutputStream);
nit: Pretty sure the style in the tree that is encouraged is:
>+    let outputStream = Cc["@mozilla.org/network/safe-file-output-stream;1"].
>+                       createInstance(Ci.nsIFileOutputStream);
I can't say I've seen this one before.

>+++ b/services/share/tests/unit/test_account.js
>+// Test the write functionality. The data is encrypted.
>+gTests.push(function test_store() {
>+  AccountStore.store(TEST_DATA, function (status) {
>+    do_check_true(Components.isSuccessCode(status));
>+    readFile(accountsFile, function(fileContents) {
>+      do_check_eq(loginManagerCrypto.decrypt(fileContents), TEST_DATA);
>+    run_next_test();
nit: indentation oops

>+gTests.push(function test_clear() {
>+  AccountStore.clear();
>+  AccountStore.fetch(function (status, data) {
>+    do_check_eq(status, Cr.NS_ERROR_FILE_NOT_FOUND);
>+    do_check_eq(data, null);
>+    run_next_test();
>+  });
>+  run_next_test();
>+});
You call run_next_test twice here, which I suspect is a bug.

r=sdwilsh
Attachment #522745 - Flags: review?(sdwilsh) → review+
Thanks, sdwilsh. Landed with your comments applied:
* part 0: https://hg.mozilla.org/users/pweitershausen_mozilla.com/fx-share/rev/eeb831d650d6
* part 1: https://hg.mozilla.org/users/pweitershausen_mozilla.com/fx-share/rev/2e38d758a2a1

Still need part 2 which hooks up the AccountStore to the postMessage API, so not resolving this yet.
Blocks: 647312
philikon:

Re: "* Some add-ons prevent localStorage from working, so right now they prevent F1 from working."

Can you point me to some bugs that describe this problem?
(In reply to comment #9)
> philikon:
> 
> Re: "* Some add-ons prevent localStorage from working, so right now they
> prevent F1 from working."
> 
> Can you point me to some bugs that describe this problem?

The BetterPrivacy add-on disables DOM Storage by default, that was the usual cause of the failures that were reported.
Try build at http://tbpl.mozilla.org/?tree=MozillaTry&rev=884231a649d8
Whiteboard: [ETA 2011-04-01] → [part 0-1 landed][part 2-4 need review mixedpuppy]
Attachment #526073 - Flags: review?(mixedpuppy) → review+
Comment on attachment 526074 [details] [diff] [review]
Part 3 (v1): Implement SecureKeyValueStore on top of SecureFileStore

per irc discussion, remove and removeAll will be updated to always notify as a result of the call.  with that r=me
Attachment #526074 - Flags: review?(mixedpuppy) → review+
Comment on attachment 526075 [details] [diff] [review]
Part 4 (v1): Hook up SecureValueStore to the share panel

the observe function will also need to watch for the :store:cleared notification per the change to removeAll that we discussed on irc.  with that r=me
Attachment #526075 - Flags: review?(mixedpuppy) → review+
Incorporated review comments:
* remove() always notifies, even if the key doesn't exist
* removeAll() notifies with a new observer topic
Attachment #526113 - Flags: review?(mixedpuppy)
Attachment #526074 - Attachment is obsolete: true
Incorporate review comments:
* make sure we always notify on storeRemove, even if the key doesn't exist
* send storeNotifyCleared as a response to storeRemoveAll
Attachment #526115 - Flags: review?(mixedpuppy)
Attachment #526075 - Attachment is obsolete: true
Attachment #526115 - Flags: review?(mixedpuppy) → review+
Attachment #526113 - Flags: review?(mixedpuppy) → review+
Depends on: 650200
Depends on: 650201
Depends on: 652629
You need to log in before you can comment on or make changes to this bug.