feat(oauth): 'verification_redirect' option for OAuth reliers #2366

vladikoff · 2015-04-30T22:42:31Z

Fixes #2343

vladikoff · 2015-04-30T22:42:57Z

app/scripts/models/reliers/oauth.js

-      keys: false
+      keys: false,
+      // always redirect to the rp, useful during email verification signup flow
+      always: false


WIP: need to rename this

Have other alternatives been considered, like making use of redirectTo or allowing the relier to specify an URL other than the redirectURI?

redirectTo is conventionally used by reliers to dynamically change the redirect URL, which we don't support now but haven't ruled out for the future. Let's not conflate that with configuring the UX around redirection.

rfk · 2015-05-01T03:22:12Z

app/scripts/lib/url.js

+     * @param {String} uri
+     * @returns {boolean}
+     */
+    isHTTP: function (uri) {


Not sure I have a better naming suggestion, but when I saw the name of this method, I immediately thought "oh, it's checking whether the URL is plain http instead of https".

isUri()? I'm guessing a uri couldn't be protocol-relative URL, like "//mozilla.com/" (http://www.paulirish.com/2010/the-protocol-relative-url/)?

vladikoff · 2015-05-01T22:16:47Z

app/scripts/models/auth_brokers/redirect.js

@@ -76,7 +76,7 @@ define([
      // event handlers before the flow completes.
      var self = this;
      return p().delay(100).then(function () {
-        if (self.isOriginalTab()) {
+        if (self.isOriginalTab() || account.get('sessionToken') && alwaysRedirect) {


Any better way to do this, is there anything else we can use instead of sessionToken to signal that this is SAME_BROWSER?

Passing in alwaysRedirect feels janky and like an implementation detail the view doesn't need to know about. The broker already has access to the relier, so the flag can be fetched by the broker directly from the relier.

Any better way to do this, is there anything else we can use instead of sessionToken to signal that this is SAME_BROWSER?

We need to know this already in app-start - see

fxa-content-server/app/scripts/lib/app-start.js

Line 501 in 4898dc0

var savedClientId = Session.oauth && Session.oauth.client_id;

We have a notion of "verification info models" in https://github.com/mozilla/fxa-content-server/tree/master/app/scripts/models/verification. Logically, knowing whether the user is verifying in the same browser seems like it is "verification info". Maybe we can add a bit onto the verification info models and pass the model in to the broker?

shane-tomlinson · 2015-05-03T09:15:43Z

app/scripts/models/reliers/oauth.js

-      keys: false
+      keys: false,
+      // always redirect to the rp, useful during email verification signup flow
+      alwaysRedirect: false


Maybe forceRedirect is more clear? Can you move this closer to redirectTo so there is slight bit more local context?

I have to admit that I'm still a little confused by the name. Does alwaysRedirect mean the user is automatically redirected, or always given the option to redirect? If the second, could it be called allowRedirect or something like that? IIUC, always_redirect is specified by the relier?

Have other alternatives been considered, like making use of redirectTo or allowing the relier to specify an URL other than the redirectURI?

I'm trying to wrap my head around the relier HTTP API and am trying to avoid confusion (because I myself am confused).

shane-tomlinson · 2015-05-03T09:16:18Z

app/scripts/views/complete_sign_up.js

@@ -12,12 +12,13 @@ define([
  'lib/auth-errors',
  'lib/validate',
  'lib/promise',
+  'lib/resume-token',


It doesn't look like the resume token is actually used anywhere.

shane-tomlinson · 2015-05-03T09:18:28Z

app/tests/spec/lib/url.js

+      it('detects HTTP and HTTPS properly', function () {
+        assert.isFalse(Url.isNavigable('urn:ietf:wg:oauth:2.0:fx:webchannel'));
+        assert.isTrue(Url.isNavigable('https://find.firefox.com'));
+        assert.isTrue(Url.isNavigable('http://find.firefox.com'));


Can you add a test for telnet:// or ftp:// or something with a protocol:// that is not http/https?

shane-tomlinson · 2015-05-03T09:19:10Z

app/scripts/views/ready.js

+      var alwaysRedirect = relier.get('alwaysRedirect');
+
+      // if this relier wants to always redirect the user then create a link to the service
+      if (alwaysRedirect && redirectUri && Url.isNavigable(redirectUri)) {


alwaysRedirect implies that the user would be redirected automatically, is that not the case?

Not the case for the signup flow in a different browser

shane-tomlinson · 2015-05-03T09:19:43Z

app/scripts/views/complete_sign_up.js

@@ -62,7 +63,7 @@ function (Cocktail, FormView, BaseView, CompleteSignUpTemplate,
      return self.fxaClient.verifyCode(uid, code)
          .then(function () {
            self.logScreenEvent('verification.success');
-            return self.broker.afterCompleteSignUp(self.getAccount());
+            return self.broker.afterCompleteSignUp(self.getAccount(), self.relier.get('alwaysRedirect'));


Again, I would not pass in self.relier.get('alwaysRedirect'), but let the broker fetch this value from the relier directly.

shane-tomlinson · 2015-05-03T09:21:39Z

app/tests/spec/lib/url.js

+        assert.isTrue(Url.isNavigable('https://find.firefox.com'));
+        assert.isTrue(Url.isNavigable('http://find.firefox.com'));
+        assert.isFalse(Url.isNavigable(''));
+        assert.isFalse(Url.isNavigable());


A brief look at the code - it also looks like both the strings http:// and https:// (without any sort of domain name) are considered navigable. I think they should not be.

shane-tomlinson · 2015-05-03T09:24:41Z

app/scripts/views/ready.js

@@ -18,12 +18,14 @@ define([
  'lib/strings',
  'lib/auth-errors',
  'lib/promise',
+  'lib/url',
+  'lib/resume-token',


It doesn't look like ResumeToken is used in this module.

shane-tomlinson · 2015-05-03T09:27:23Z

@vladikoff - can you explain a bit what you are trying to do? I think some context got shoved out of my head between last Thursday and today.

vladikoff · 2015-05-04T15:27:19Z

@shane-tomlinson woops I think I forgot to link the bug, see #2343

shane-tomlinson · 2015-05-12T20:44:53Z

I have a feeling, though I have not tested, the above behavior will affect account lockout too.

zaach · 2015-05-12T20:56:55Z

Don't we want the button after a reset? Otherwise it would be another dead end.

shane-tomlinson · 2015-05-12T20:58:06Z

app/scripts/views/ready.js

+      var showProceedButton = false;
+
+      // if this relier wants to use verification redirect in the same browser then create a link to the service
+      if (verificationRedirect === Constants.VERIFICATION_REDIRECT_ALWAYS &&


Perhaps to avoid last minute confusion and unexpected behavior for reset password, you should add && this.is('sign_up'). We can iterate on the other cases next train.

We can iterate on the other cases next train.

Agreed.

vladikoff · 2015-05-12T20:58:29Z

The only sad thing is that 123done explodes, but maybe we should fix that and in cases of password reset / account lockout we should be able to "Proceed"

shane-tomlinson · 2015-05-12T21:09:36Z

Don't we want the button after a reset? Otherwise it would be another dead end.

For sure. I think for this train it's sufficient to enable the feature for sign up verification, and then add the other two flows in the next train when we have more time to work through any hidden goblins.

shane-tomlinson · 2015-05-13T13:06:14Z

@vladikoff - can you rebase & limit Proceed's visibility to the sign-up flow?

r+ once addressed!

shane-tomlinson · 2015-05-13T21:14:18Z

tests/functional/oauth_sign_up_verification_redirect.js

+
+    beforeEach: function () {
+      email = TestHelpers.createEmail();
+      bouncedEmail = TestHelpers.createEmail();


nit: this isn't actually used anywhere.

shane-tomlinson · 2015-05-13T21:15:28Z

tests/functional/oauth_sign_up_verification_redirect.js

+    beforeEach: function () {
+      email = TestHelpers.createEmail();
+      bouncedEmail = TestHelpers.createEmail();
+      user = TestHelpers.emailToUser(email);


nit: nor is this.

shane-tomlinson · 2015-05-13T21:15:55Z

tests/functional/oauth_sign_up_verification_redirect.js

+      email = TestHelpers.createEmail();
+      bouncedEmail = TestHelpers.createEmail();
+      user = TestHelpers.emailToUser(email);
+      fxaClient = new FxaClient(AUTH_SERVER_ROOT, {


same here. ;)

shane-tomlinson · 2015-05-13T21:17:36Z

tests/functional/oauth_sign_up_verification_redirect.js

+
+        // new browser provides a proceed link to the relier
+        .findByCssSelector('#proceedButton')
+        .click()


nit: indent.

coveralls · 2015-05-13T22:00:04Z

Coverage increased (+0.01%) to 97.95% when pulling a5fdaee on vladikoff:issue-2343-dead-end into 176d17d on mozilla:master.

zaach · 2015-05-13T22:41:15Z

🚢

feat(oauth): 'verification_redirect' option for OAuth reliers r=shane-tomlinson,zaach

shane-tomlinson · 2015-05-18T09:09:58Z

ref #2402

vladikoff added WIP oauth labels Apr 30, 2015

vladikoff added this to the train 37 milestone Apr 30, 2015

vladikoff added waffle:needs review and removed WIP labels Apr 30, 2015

vladikoff reviewed Apr 30, 2015
View changes

vladikoff force-pushed the vladikoff:issue-2343-dead-end branch from 5eb857c to c3ae53a Apr 30, 2015

rfk reviewed May 1, 2015
View changes

vladikoff force-pushed the vladikoff:issue-2343-dead-end branch 2 times, most recently from 2e94ef5 to 23850a9 May 1, 2015

vladikoff changed the title ~~WIP feat(oauth): 'always_redirect' option for OAuth reliers~~ feat(oauth): 'always_redirect' option for OAuth reliers May 1, 2015

vladikoff assigned zaach May 1, 2015

vladikoff reviewed May 1, 2015
View changes

shane-tomlinson changed the title ~~feat(oauth): 'always_redirect' option for OAuth reliers~~ WIP feat(oauth): 'always_redirect' option for OAuth reliers May 3, 2015

shane-tomlinson added WIP and removed waffle:needs review labels May 3, 2015

shane-tomlinson reviewed May 3, 2015
View changes

app/scripts/views/complete_sign_up.js

@@ -12,12 +12,13 @@ define([

'lib/auth-errors',

'lib/validate',

'lib/promise',

'lib/resume-token',

This comment has been minimized.

Sign in to view

shane-tomlinson May 3, 2015
Member

It doesn't look like the resume token is actually used anywhere.

shane-tomlinson reviewed May 3, 2015
View changes

app/scripts/views/ready.js

@@ -18,12 +18,14 @@ define([

'lib/strings',

'lib/auth-errors',

'lib/promise',

'lib/url',

'lib/resume-token',

This comment has been minimized.

Sign in to view

shane-tomlinson May 3, 2015
Member

It doesn't look like ResumeToken is used in this module.

vladikoff changed the title ~~WIP feat(oauth): 'always_redirect' option for OAuth reliers~~ feat(oauth): 'always_redirect' option for OAuth reliers May 4, 2015

vladikoff added waffle:needs review and removed WIP labels May 4, 2015

vladikoff force-pushed the vladikoff:issue-2343-dead-end branch from 23850a9 to 77ed459 May 4, 2015

shane-tomlinson reviewed May 12, 2015
View changes

vladikoff mentioned this pull request May 12, 2015

OAuth: support verification_redirect for password reset and account reset #2401

Closed

pdehaan mentioned this pull request May 13, 2015

Odd login flow when trying to sign in to Pocket (and you close original tab) #2402

Closed

vladikoff mentioned this pull request May 13, 2015

OAuth: remove 'state' from RELIER_FIELDS_IN_RESUME_TOKEN #2405

Closed

vladikoff force-pushed the vladikoff:issue-2343-dead-end branch 3 times, most recently from 5ae66f7 to 687f79d May 13, 2015

shane-tomlinson reviewed May 13, 2015
View changes

tests/functional/oauth_sign_up_verification_redirect.js

beforeEach: function () {

email = TestHelpers.createEmail();

bouncedEmail = TestHelpers.createEmail();

This comment has been minimized.

Sign in to view

shane-tomlinson May 13, 2015
Member

nit: this isn't actually used anywhere.

shane-tomlinson reviewed May 13, 2015
View changes

tests/functional/oauth_sign_up_verification_redirect.js

// new browser provides a proceed link to the relier

.findByCssSelector('#proceedButton')

.click()

This comment has been minimized.

Sign in to view

shane-tomlinson May 13, 2015
Member

nit: indent.

vladikoff force-pushed the vladikoff:issue-2343-dead-end branch 2 times, most recently from 31f1000 to c4acf4f May 13, 2015

feat(oauth): 'verification_redirect' option for OAuth reliers

a5fdaee

vladikoff force-pushed the vladikoff:issue-2343-dead-end branch from c4acf4f to a5fdaee May 13, 2015

zaach added a commit that referenced this pull request May 13, 2015

Merge pull request #2366 from vladikoff/issue-2343-dead-end

a63443e

feat(oauth): 'verification_redirect' option for OAuth reliers r=shane-tomlinson,zaach

zaach merged commit a63443e into mozilla:master May 13, 2015
1 check passed

1 check passed

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

zaach removed the waffle:needs review label May 13, 2015

vladikoff deleted the vladikoff:issue-2343-dead-end branch May 13, 2015

rfk mentioned this pull request May 19, 2015

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

Closed

This was referenced May 19, 2015

Flow doc needs updated with verification_redirect behaviors. #2437

Closed

query-params.md needs updated with verification_redirect #2438

Closed

mozilla / fxa-content-server Archived

feat(oauth): 'verification_redirect' option for OAuth reliers #2366

feat(oauth): 'verification_redirect' option for OAuth reliers #2366

vladikoff commented Apr 30, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

shane-tomlinson commented May 3, 2015

vladikoff commented May 4, 2015

shane-tomlinson commented May 12, 2015

zaach commented May 12, 2015

This comment has been minimized.

This comment has been minimized.

vladikoff commented May 12, 2015

shane-tomlinson commented May 12, 2015

shane-tomlinson commented May 13, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

coveralls commented May 13, 2015

zaach commented May 13, 2015

shane-tomlinson commented May 18, 2015

mozilla / fxa-content-server Archived

feat(oauth): 'verification_redirect' option for OAuth reliers #2366

feat(oauth): 'verification_redirect' option for OAuth reliers #2366

Conversation

vladikoff commented Apr 30, 2015

Fixes #2343

This comment has been minimized.

vladikoff Apr 30, 2015 Author Contributor

This comment has been minimized.

zaach May 6, 2015 Contributor

This comment has been minimized.

rfk May 1, 2015 Member

This comment has been minimized.

vladikoff May 1, 2015 Author Contributor

This comment has been minimized.

pdehaan May 1, 2015 Contributor

This comment has been minimized.

vladikoff May 1, 2015 Author Contributor

This comment has been minimized.

shane-tomlinson May 3, 2015 Member

This comment has been minimized.

shane-tomlinson May 3, 2015 Member

This comment has been minimized.

shane-tomlinson May 6, 2015 Member

This comment has been minimized.

shane-tomlinson May 3, 2015 Member

This comment has been minimized.

shane-tomlinson May 3, 2015 Member

This comment has been minimized.

vladikoff May 4, 2015 Author Contributor

This comment has been minimized.

shane-tomlinson May 3, 2015 Member

This comment has been minimized.

vladikoff May 3, 2015 Author Contributor

This comment has been minimized.

shane-tomlinson May 3, 2015 Member

This comment has been minimized.

shane-tomlinson May 3, 2015 Member

This comment has been minimized.

shane-tomlinson May 3, 2015 Member

shane-tomlinson commented May 3, 2015

vladikoff commented May 4, 2015

shane-tomlinson commented May 12, 2015

zaach commented May 12, 2015

This comment has been minimized.

shane-tomlinson May 12, 2015 Member

This comment has been minimized.

vladikoff May 12, 2015 Author Contributor

vladikoff commented May 12, 2015

shane-tomlinson commented May 12, 2015

shane-tomlinson commented May 13, 2015

This comment has been minimized.

shane-tomlinson May 13, 2015 Member

This comment has been minimized.

shane-tomlinson May 13, 2015 Member

This comment has been minimized.

shane-tomlinson May 13, 2015 Member

This comment has been minimized.

shane-tomlinson May 13, 2015 Member

coveralls commented May 13, 2015

zaach commented May 13, 2015

shane-tomlinson commented May 18, 2015

vladikoff Apr 30, 2015
Author Contributor

zaach May 6, 2015
Contributor

rfk May 1, 2015
Member

vladikoff May 1, 2015
Author Contributor

pdehaan May 1, 2015
Contributor

vladikoff May 1, 2015
Author Contributor

shane-tomlinson May 3, 2015
Member

shane-tomlinson May 3, 2015
Member

shane-tomlinson May 6, 2015
Member

shane-tomlinson May 3, 2015
Member

shane-tomlinson May 3, 2015
Member

vladikoff May 4, 2015
Author Contributor

shane-tomlinson May 3, 2015
Member

vladikoff May 3, 2015
Author Contributor

shane-tomlinson May 3, 2015
Member

shane-tomlinson May 3, 2015
Member

shane-tomlinson May 3, 2015
Member

shane-tomlinson May 12, 2015
Member

vladikoff May 12, 2015
Author Contributor

shane-tomlinson May 13, 2015
Member

shane-tomlinson May 13, 2015
Member

shane-tomlinson May 13, 2015
Member

shane-tomlinson May 13, 2015
Member