fix(oauth): Ensure we can derive relier keys during the signup flow. #2385

rfk · 2015-05-06T06:56:55Z

This is a rather brutal approach to ensuring that we can derive oauth relier keys during the signup flow. It persists the keys=true state in the oauth session data, and persists keyFetchToken and unwrapBKey in the account data so that they can be used on the complete_sign_up page.

I'm putting it up here as a proof of concept. We should find a way to do it without persisting the key-fetching material any more than necessary.

Fixes #2384. But not in a way that I'm happy about.

shane-tomlinson · 2015-05-07T07:44:39Z

app/scripts/models/auth_brokers/web-channel.js

+      // If the relier wants keys, the signup verification tab will need
+      // to be able to fetch them in order to complete the flow.
+      // Send them as part of the oauth session data.
+      if (this.relier.wantsKeys()) {


Maybe this should be done in persist so this same logic can be used for account unlock and password reset as well?

Makes sense, although IIRC the account is not current passed into persist().

Looking into this, I get the impression that:

the password reset case should work already, because of the existing cross-tab dance that goes on to coordinate completion of that flow in the original tab

the account-unlock flow only completes signin on the original tab, not the tab from the email link, and hence should work already.

If I can confirm that this is the case, I think it's better to special-case this to the sign-up flow for now, then deal with a more generalied version when we go to clean it up with e.g. tab-mutex or similar.

shane-tomlinson · 2015-05-07T07:45:30Z

app/scripts/lib/session.js

@@ -41,7 +41,22 @@ define([
        // ignore the parse error.
      }

-      this.set(values);
+      // Update new values, without overwriting items on the prototype.


Maybe this logic should live in a new function "sync" - which could call "load", then perform your logic - that way the semantics of the original function are not changed and we don't break expectations inadvertently, but the intent is clear?

On reflection, "sync" suggests a two-way synchronisation between the session and its backing store. How about "reload"?

On reflection, "sync" suggests a two-way synchronisation between the session and its backing store. How about "reload"?

I thought that was the intent, no?

no, this is more about just sucking in any changes that were made to the backing store

Ah, then reload makes sense. Carry on.

shane-tomlinson · 2015-05-07T07:49:11Z

app/scripts/models/auth_brokers/web-channel.js

+        }
+        // The slight delay is to allow the functional tests time to bind
+        // event handlers before the flow completes.
+        return p().delay(100)


This leaves open a 100ms window where the original tab could see that this.session.oauth exists, then finish the oauth flow, and the verification tab also finishes the oauth flow just afterwards.

I would wrap all of your new logic inside of:

var self = this; return p().delay(100) .then(function () { self.session.load(); // or sync! if (self.session.oauth) { .... return self.finishOAuthFlow(account); } });

shane-tomlinson · 2015-05-08T08:30:19Z

app/scripts/lib/session.js

+
+    /**
+     * Reload in-memory values based on what's currently in storage.
+     * @method sync


you use sync in the docs and reload in the signature.

rfk · 2015-05-08T11:44:52Z

OK, the latest push is moving a lot closer to something we might actually run with. Most importantly, it's got a bunch of tests! I get a clean run of the mocha tests and of npm run test-functional-oauth, apart from some force_auth tests that seem to be broken on master.

I coped the existing suite of oauth_webchannel tests into a new oauth_webchannel_keys suite and tweaked them to request and check key derivation. This was a bit fiddly because with the change, the original tab will often skip completing the oauth flow. So in e.g. the sign-up-and-verify-in-same-browser test, we have to check for the WebChannel events in the verification page rather than the main browser page.

This ickiness will go away if we move to a cross-tab mutex solution to ensure that the original tab is deterministically chosen as the one to complete the flow. But I've no idea how much work that will turn out to be, and it's nice to have some sort of fix ready in the meantime.

rfk · 2015-05-08T11:55:54Z

apart from some force_auth tests that seem to be broken on master.

Aha, I did not have mozilla/123done@65ac3b1 in my local dev setup; the whole oauth functional suite is now passing cleaning for me.

shane-tomlinson · 2015-05-09T19:05:18Z

apart from some force_auth tests that seem to be broken on master.
...
Aha, I did not have mozilla/123done@65ac3b1 in my local dev setup; the whole oauth functional suite is now passing cleaning for me.

We don't check in code if those tests fail. ;)

shane-tomlinson · 2015-05-09T20:19:51Z

app/scripts/lib/session.js

+    /**
+     * Reload in-memory values based on what's currently in storage.
+     * @method reload
+     * This method can be used to pull in any changes to localStorage


Super nit, can you move this note above the @method

Also, can you make a note that this operation is destructive, as in, it'll remove any values currently in Session that are not also in sessionStorage or localStorage?

shane-tomlinson · 2015-05-09T20:22:18Z

app/scripts/lib/session.js

-      this.set(values);
+      // Update new values, without overwriting items on the prototype.
+      _.each(values, function (value, key) {
+        if (! Session.prototype.hasOwnProperty(key)) {


I was thinking about this yesterday. If Session were an actual Backbone model, we wouldn't have to do this unnatural hasOwnProperty stuff because models store their data in its own field. It's a bit moot because I want to get rid of Session, and converting to a model is well out of the scope of this PR.

shane-tomlinson · 2015-05-09T20:51:59Z

app/scripts/models/auth_brokers/web-channel.js

+    afterSignUpConfirmationPoll: function (account) {
+      // The original tab can finish the OAuth flow if it is still open,
+      // but not if the verification tab has already finished it.
+      this.session.reload();


Can this and the next line be combined (in all 4 places) into a descriptively named helper function that reduces the need for the comment, something like if (this.canFinishOAuthFlow()) { or if (! this.hasOAuthFlowFinished()) {

shane-tomlinson · 2015-05-09T21:03:46Z

app/tests/spec/models/auth_brokers/web-channel.js

+          });
+      });
+
+      it('doesnt call sendOAuthResultToRelier if there is no session data', function () {


You can add a ' between the nt with doesn\'t. It looks funny in the JS, but renders well in HTML.

Heh, yeah I deliberately didn't do that because it looked funny in the JS; will add it.

shane-tomlinson · 2015-05-09T21:58:49Z

app/scripts/models/auth_brokers/web-channel.js

+    },
+
+    afterResetPasswordConfirmationPoll: function (account) {
+      // The original tab can finish the OAuth flow if it is still open,


Is the extra work needed for the reset password flow? In the reset password flow, the updated keys are generated once the user enters their new password in the verification page. The verification page can send the keys/web channel message to the browser w/o the original page being involved.

So, what I'm asking, if you haven't done so already, can you verify there is a problem w/ reset password before adding the extra logic to have the original tab send the message?

never mind, I found my own answer.

For completeness: thanks to the existing session-token-juggling logic between the two tabs here, they were both already capable of finishing the flow with keys. The logic here is just to prevent them both from doing so, and hence triggering an error by trying to use the same keyFetchToken twice.

shane-tomlinson · 2015-05-09T22:14:07Z

app/scripts/models/auth_brokers/web-channel.js

+      // but not if the verification tab has already finished it.
+      this.session.reload();
+      if (this.session.oauth) {
+        return this.finishOAuthFlow(account);


Does this account already have the keyFetchToken and unwrapBKey?

Yes, thanks to wantsKeys() being true and the user already typing their password into that tab. It's the same account that was passed into beforeSignUpConfirmationPoll above.

rfk · 2015-05-10T23:16:48Z

It's a shame we can't fetch keys until after the account is verified; if that were possible, we could do the key-derivation ahead of time and store just the relier keys in the session, rather than the master unwrapKBey...

rfk · 2015-05-10T23:26:23Z

@vladikoff this PR adds some more functional tests for the oauth flow with keys, and shuffles the existing tests around a bit. Can you please give it a look over to sanity-check?

shane-tomlinson · 2015-05-11T13:10:34Z

Thanks for working through this with me @rfk. I give this my r+.
@vladikoff, yer turn!

vladikoff · 2015-05-11T14:29:34Z

tests/functional/oauth_webchannel_keys.js

+    });
+  }
+
+  function openFxaFromRp(context, page, additionalQueryParams) {


Nit: rename this to avoid confusion with the original openFxaFromRp?

vladikoff · 2015-05-11T14:35:37Z

app/scripts/models/reliers/oauth.js

@@ -17,7 +17,7 @@ define([
  'lib/relier-keys',
  'lib/url'
 ], function (_, Relier, ResumeToken, p, OAuthErrors, RelierKeys, Url) {
-  var RELIER_FIELDS_IN_RESUME_TOKEN = ['state'];
+  var RELIER_FIELDS_IN_RESUME_TOKEN = ['state', 'keys'];


Adding keys here doesn't do anything, removing functional tests still pass. Currently we are not parsing the resume token anywhere AFAIK. I'm adding parsing in https://github.com/mozilla/fxa-content-server/pull/2366/files#diff-ca4d7096469c2a76694bb8214760cbaaR104

Not sure if we want to keep this here. Is this just for a the "different browser" flow to know that the relier wanted keys?

After talking this over with @shane-tomlinson, I will just remove it from here.

vladikoff · 2015-05-11T14:49:50Z

tests/functional/lib/helpers.js

+            // Hook up the new window to listen for WebChannel messages.
+            // XXX TODO: this is pretty gross to do universally like this...
+            // XXX TODO: it will go away if we can make the original tab
+            //           reliably be the one to complete the oauth flow.


@rfk @shane-tomlinson can we use https://developer.mozilla.org/en-US/docs/Web/Guide/User_experience/Using_the_Page_Visibility_API to reliably finish flow in the active tab?
Should that be part of this PR or leave it for later?

https://github.com/dcherman/jquery.pagevisibility

https://github.com/dcherman/jquery.pagevisibility

Why'd he have to tie that to jQuery?

@rfk @shane-tomlinson can we use https://developer.mozilla.org/en-US/docs/Web/Guide/User_experience/Using_the_Page_Visibility_API to reliably finish flow in the active tab?
Should that be part of this PR or leave it for later?

@vladikoff - at first blush this makes sense, but the preference is for the original tab to finish the flow, if it is still open. @rfk has already started on a PR to make it so.

" but the preference is for the original tab" why is that?

Why'd he have to tie that to jQuery?

Does not have to be, I saw the brutal example on MDN and looked for a contained solution.

We prefer the original sign-up tab to finish the flow, since it's the one holding the encryption material. It doesn't come into play in this PR, but in a future iteration we want to avoid writing the encryption material to localStorage. In that case, the verification link would have to re-prompt for your password to complete the flow, while the originating tab could do it automatically. Hence the (future-looking) preference for completing in the original tab.

vladikoff · 2015-05-11T14:55:13Z

tests/functional/oauth_webchannel_keys.js

+      });
+    },
+
+    'signup, verify same browser with original tab open': function () {


Nit: signup, verify same browser with original tab open' ->
signup, verify same browser, in a different tab, with original tab open' ?

My fault. @rfk copied my test names from elsewhere.

vladikoff · 2015-05-11T15:03:31Z

tests/functional/oauth_webchannel_keys.js

+        .end();
+    },
+
+    'signup, verify same browser with original tab closed': function () {


nit: naming, with vs adding a , related to https://github.com/mozilla/fxa-content-server/pull/2385/files#r30045417

vladikoff · 2015-05-11T15:08:24Z

tests/functional/oauth_webchannel_keys.js

+        .findByCssSelector('#fxa-confirm-header')
+        .end()
+
+        .then(FunctionalHelpers.openExternalSite(self))


nit: this maybe needs documentation, original tab closed is actually -> "navigated away somewhere else" (to example.com in this case).

vladikoff · 2015-05-11T15:08:49Z

tests/functional/oauth_webchannel_keys.js

+
+        .findById('fxa-sign-up-complete-header')
+        .end();
+    },


The 2 tests above: Is original tab closed vs replace original tab a bit redundant?
@shane-tomlinson thoughts?

The 2 tests above: Is original tab closed vs replace original tab a bit redundant?

For this case, possibly, for other cases they are not. For example, with the redirect flow, if the user replaces the tab they signed up in with the verification tab, then the user should be redirected back to the relier automatically. If they verify in a 2nd tab and the original tab has closed, then no automatic redirection.

I'd like to leave it in unless you object strongly, since it might highlight weirdness when we're trying to do "complete in the original tab" in the future.

vladikoff · 2015-05-11T15:30:02Z

tests/functional/oauth_webchannel_keys.js

+
+        .findByCssSelector('#fxa-reset-password-complete-header')
+        .end();
+    },


2 tests above, see https://github.com/mozilla/fxa-content-server/pull/2385/files#r30046801

vladikoff · 2015-05-11T15:31:15Z

tests/functional/oauth_webchannel_keys.js

+        // It should not try to generate keys and complete the flow.
+        .findByCssSelector('#fxa-sign-up-complete-header')
+        .end();
+    },


All different browser tests, see comment https://github.com/mozilla/fxa-content-server/pull/2385/files#r30043457

Yeah, I guess these "from new browser P.O.V." tests aren't really testing anything. I'm going to remove them here, they're equally well covered by their equivalents in the non-keys webchannel case.

rfk · 2015-05-13T00:39:01Z

OK, functional tests updated per @vladikoff feedback, and keys removed from the resume token. AFAICT this should be good to land for train-37, and we can continue iterating toward a better solution for train-38.

rfk · 2015-05-13T00:39:21Z

(But I won't be around to watch it so I'll leave the merge button for someone else...)

coveralls · 2015-05-13T13:00:58Z

Coverage increased (+0.01%) to 97.93% when pulling fd2fc72 on rfk/relier-keys-signup-flow into a52d6fb on master.

@rfk

fix(client): Ensure WebChannel reliers can be sent derived relier keys during the signup flow. @rfk - this is a great start, we can iterate towards a more optimal solution next train. r=@shane-tomlinson

rfk added WIP waffle:needs review and removed WIP labels May 6, 2015

rfk mentioned this pull request May 6, 2015

Relier keys don't work after signup and verify in same browser #2384

Closed

rfk force-pushed the rfk/relier-keys-signup-flow branch from 85529ef to 88dbdff May 7, 2015

rfk changed the title ~~fix(oauth): Properly derive relier keys during the signup flow.~~ fix(oauth): Ensure we can derive relier keys during the signup flow. May 7, 2015

rfk force-pushed the rfk/relier-keys-signup-flow branch 2 times, most recently from 4c95163 to b2ffb60 May 7, 2015

shane-tomlinson reviewed May 7, 2015
View changes

shane-tomlinson self-assigned this May 7, 2015

rfk force-pushed the rfk/relier-keys-signup-flow branch 2 times, most recently from c0d1a43 to 6d9e883 May 8, 2015

shane-tomlinson reviewed May 8, 2015
View changes

app/scripts/lib/session.js

/**

* Reload in-memory values based on what's currently in storage.

* @method sync

This comment has been minimized.

Sign in to view

shane-tomlinson May 8, 2015
Member

you use sync in the docs and reload in the signature.

rfk force-pushed the rfk/relier-keys-signup-flow branch 2 times, most recently from 8837344 to 6e9478b May 8, 2015

shane-tomlinson reviewed May 9, 2015
View changes

rfk force-pushed the rfk/relier-keys-signup-flow branch from 6e9478b to e38d587 May 10, 2015

rfk force-pushed the rfk/relier-keys-signup-flow branch from e38d587 to 2f31784 May 11, 2015

shane-tomlinson force-pushed the rfk/relier-keys-signup-flow branch 2 times, most recently from 07b098d to 585a1d0 May 11, 2015

shane-tomlinson assigned vladikoff and unassigned shane-tomlinson May 11, 2015

vladikoff reviewed May 11, 2015
View changes

tests/functional/oauth_webchannel_keys.js

});

}

function openFxaFromRp(context, page, additionalQueryParams) {

This comment has been minimized.

Sign in to view

vladikoff May 11, 2015
Contributor

Nit: rename this to avoid confusion with the original openFxaFromRp?

vladikoff reviewed May 11, 2015
View changes

tests/functional/oauth_webchannel_keys.js

.findByCssSelector('#fxa-reset-password-complete-header')

.end();

},

This comment has been minimized.

Sign in to view

vladikoff May 11, 2015
Contributor

2 tests above, see https://github.com/mozilla/fxa-content-server/pull/2385/files#r30046801

vladikoff reviewed May 11, 2015
View changes

rfk added this to the train 37 milestone May 12, 2015

rfk force-pushed the rfk/relier-keys-signup-flow branch from 585a1d0 to 4da4f5b May 12, 2015

fix(oauth): Ensure we can derive relier keys during the signup flow.

fd2fc72

shane-tomlinson force-pushed the rfk/relier-keys-signup-flow branch from 4da4f5b to fd2fc72 May 13, 2015

shane-tomlinson merged commit 52c081e into master May 13, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

continuous-integration/travis-ci/push The Travis CI build passed
Details

shane-tomlinson removed the waffle:needs review label May 13, 2015

shane-tomlinson deleted the rfk/relier-keys-signup-flow branch May 13, 2015

This was referenced May 14, 2015

Add functional tests for oauth webchannel keys #2175

Closed

Allow verification_redirect from the verification tab but *not * the original tab? #2436

Closed

mozilla / fxa-content-server Archived

fix(oauth): Ensure we can derive relier keys during the signup flow. #2385

fix(oauth): Ensure we can derive relier keys during the signup flow. #2385

Conversation

rfk commented May 6, 2015

This comment has been minimized.

shane-tomlinson May 7, 2015 Member

This comment has been minimized.

rfk May 7, 2015 Author Member

This comment has been minimized.

rfk May 8, 2015 Author Member

This comment has been minimized.

shane-tomlinson May 7, 2015 Member

This comment has been minimized.

rfk May 8, 2015 Author Member

This comment has been minimized.

shane-tomlinson May 8, 2015 Member

This comment has been minimized.

rfk May 8, 2015 Author Member

This comment has been minimized.

shane-tomlinson May 9, 2015 Member

This comment has been minimized.

shane-tomlinson May 7, 2015 Member

This comment has been minimized.

rfk May 8, 2015 Author Member

This comment has been minimized.

shane-tomlinson May 8, 2015 Member

rfk commented May 8, 2015

rfk commented May 8, 2015

shane-tomlinson commented May 9, 2015

This comment has been minimized.

shane-tomlinson May 9, 2015 Member

This comment has been minimized.

shane-tomlinson May 9, 2015 Member

This comment has been minimized.

shane-tomlinson May 9, 2015 Member

This comment has been minimized.

shane-tomlinson May 9, 2015 Member

This comment has been minimized.

shane-tomlinson May 9, 2015 Member

This comment has been minimized.

rfk May 10, 2015 Author Member

This comment has been minimized.

shane-tomlinson May 9, 2015 Member

This comment has been minimized.

shane-tomlinson May 9, 2015 Member

This comment has been minimized.

shane-tomlinson May 9, 2015 Member

This comment has been minimized.

rfk May 10, 2015 Author Member

This comment has been minimized.

shane-tomlinson May 9, 2015 Member

This comment has been minimized.

rfk May 10, 2015 Author Member

rfk commented May 10, 2015

rfk commented May 10, 2015

shane-tomlinson commented May 11, 2015

This comment has been minimized.

vladikoff May 11, 2015 Contributor

This comment has been minimized.

vladikoff May 11, 2015 Contributor

This comment has been minimized.

rfk May 12, 2015 Author Member

This comment has been minimized.

vladikoff May 11, 2015 Contributor

This comment has been minimized.

vladikoff May 11, 2015 Contributor

This comment has been minimized.

shane-tomlinson May 11, 2015 Member

This comment has been minimized.

shane-tomlinson May 11, 2015 Member

This comment has been minimized.

vladikoff May 11, 2015 Contributor

This comment has been minimized.

vladikoff May 11, 2015 Contributor

This comment has been minimized.

rfk May 12, 2015 Author Member

This comment has been minimized.

vladikoff May 11, 2015 Contributor

This comment has been minimized.

shane-tomlinson May 7, 2015
Member

rfk May 7, 2015
Author Member

rfk May 8, 2015
Author Member

shane-tomlinson May 7, 2015
Member

rfk May 8, 2015
Author Member

shane-tomlinson May 8, 2015
Member

rfk May 8, 2015
Author Member

shane-tomlinson May 9, 2015
Member

shane-tomlinson May 7, 2015
Member

rfk May 8, 2015
Author Member

shane-tomlinson May 8, 2015
Member

shane-tomlinson May 9, 2015
Member

shane-tomlinson May 9, 2015
Member

shane-tomlinson May 9, 2015
Member

shane-tomlinson May 9, 2015
Member

shane-tomlinson May 9, 2015
Member

rfk May 10, 2015
Author Member

shane-tomlinson May 9, 2015
Member

shane-tomlinson May 9, 2015
Member

shane-tomlinson May 9, 2015
Member

rfk May 10, 2015
Author Member

shane-tomlinson May 9, 2015
Member

rfk May 10, 2015
Author Member

vladikoff May 11, 2015
Contributor

vladikoff May 11, 2015
Contributor

rfk May 12, 2015
Author Member

vladikoff May 11, 2015
Contributor

vladikoff May 11, 2015
Contributor

shane-tomlinson May 11, 2015
Member

shane-tomlinson May 11, 2015
Member

vladikoff May 11, 2015
Contributor

vladikoff May 11, 2015
Contributor

rfk May 12, 2015
Author Member

vladikoff May 11, 2015
Contributor

shane-tomlinson May 11, 2015
Member

vladikoff May 11, 2015
Contributor

vladikoff May 11, 2015
Contributor

vladikoff May 11, 2015
Contributor

shane-tomlinson May 11, 2015
Member

rfk May 12, 2015
Author Member

vladikoff May 11, 2015
Contributor

vladikoff May 11, 2015
Contributor

rfk May 12, 2015
Author Member