Closed Bug 1182071 Opened 5 years ago Closed 5 years ago

Allow System app to fetch Sync keys

Categories

(Firefox OS Graveyard :: FxA, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox42 fixed)

RESOLVED FIXED
Tracking Status
firefox42 --- fixed

People

(Reporter: ferjm, Assigned: ferjm)

References

Details

Attachments

(2 files, 3 obsolete files)

In order to encrypt/decrypt Sync records we need to obtain kA and kB from Firefox Accounts [1]. We are already doing this at [2]. We just need to expose this method to the System app, as we already do with other FxA functionality. But in this case we won't expose this outside of the System app.

[1] https://github.com/mozilla/fxa-auth-server/wiki/onepw-protocol#-fetching-sync-keys
[2] https://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccounts.jsm#856
Assignee: nobody → ferjmoreno
Blocks: 1182001
Attachment #8631644 - Attachment is obsolete: true
Attached patch v1 (obsolete) — Splinter Review
Attached patch v1Splinter Review
Sam, would you mind taking a look at this patch, please?

We need this to do the encryption and decryption of Firefox Sync data at the System app level.

Thanks!
Attachment #8631649 - Attachment is obsolete: true
Attachment #8633465 - Flags: review?(spenrose)
Comment on attachment 8633465 [details] [diff] [review]
v1

This looks good. Thanks very much for the tests. One copy-paste error appears to have snuck in at the end:

+add_test(function(test_getKeys_success) {
+  do_print("= getKeys unverified =");
+  Services.prefs.setBoolPref("services.sync.enabled", true);
+  FakeFxAccountsClient._verified = true;
+  FxAccountsManager.signIn("user@domain.org", "password").then(result => {
+    do_check_false(result.verified);

I believe this should read:

+add_test(function(test_getKeys_success) {
+  do_print("= getKeys success =");
...
+    do_check_true(result.verified);

r+ with that nit addressed. Thanks!
Attachment #8633465 - Flags: review?(spenrose) → review+
Attachment #8631647 - Attachment is obsolete: true
Comment on attachment 8635904 [details] [review]
[gaia] ferjm:bug1182071.synckeys.2 > mozilla-b2g:master

Kevin, would you mind taking a look at this patch, please?

We only want to expose the ability to obtain the Sync keys to the System app.
Attachment #8635904 - Flags: review?(kgrandon)
Comment on attachment 8635904 [details] [review]
[gaia] ferjm:bug1182071.synckeys.2 > mozilla-b2g:master

I don't really know how to test this, but the code seems ok to me. Thanks.
Attachment #8635904 - Flags: review?(kgrandon) → review+
https://hg.mozilla.org/mozilla-central/rev/fc38e238fcb0
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #7)
> Comment on attachment 8635904 [details] [review]
> [gaia] ferjm:bug1182071.synckeys.2 > mozilla-b2g:master
> 
> Kevin, would you mind taking a look at this patch, please?
> 
> We only want to expose the ability to obtain the Sync keys to the System app.

Can you explain how this restriction is implemented - I reviewed the attached patches but I don't see it. I was expecting to see some kind of check in the parent process to ensure that the app using this API is the system app.
(In reply to Paul Theriault [:pauljt] from comment #12)
> (In reply to Fernando Jiménez Moreno [:ferjm] from comment #7)
> > Comment on attachment 8635904 [details] [review]
> > [gaia] ferjm:bug1182071.synckeys.2 > mozilla-b2g:master
> > 
> > Kevin, would you mind taking a look at this patch, please?
> > 
> > We only want to expose the ability to obtain the Sync keys to the System app.
> 
> Can you explain how this restriction is implemented - I reviewed the
> attached patches but I don't see it. I was expecting to see some kind of
> check in the parent process to ensure that the app using this API is the
> system app.

We are using mozChromeEvents to send the keys to the System app. AFAIK no other app other than the System app can receive this kind of events.
Whiteboard: [partner-cherry-pick]
Whiteboard: [partner-cherry-pick]
You need to log in before you can comment on or make changes to this bug.