From aba3a1549d13fc90fbaac6ad4cf0f89a26118fc9 Mon Sep 17 00:00:00 2001 From: Shane Tomlinson Date: Wed, 14 Jun 2017 16:35:14 +0100 Subject: [PATCH] refactor(broker): Extract `finishOAuthFlowIfOriginalTab` to share between methods. (#5141) r=@philbooth This is extracted from #5090 with the intent to make that easier to review. --- .../models/auth_brokers/oauth-redirect.js | 76 ++++++++-------- .../models/auth_brokers/oauth-redirect.js | 89 ++++++++++--------- 2 files changed, 83 insertions(+), 82 deletions(-) diff --git a/app/scripts/models/auth_brokers/oauth-redirect.js b/app/scripts/models/auth_brokers/oauth-redirect.js index cb1c37e41..13f341e84 100644 --- a/app/scripts/models/auth_brokers/oauth-redirect.js +++ b/app/scripts/models/auth_brokers/oauth-redirect.js @@ -12,18 +12,50 @@ define(function (require, exports, module) { const p = require('lib/promise'); const Url = require('lib/url'); - var proto = OAuthAuthenticationBroker.prototype; + const proto = OAuthAuthenticationBroker.prototype; - var RedirectAuthenticationBroker = OAuthAuthenticationBroker.extend({ + /** + * Invoke `brokerMethod` on the broker and finish the OAuth flow by + * invoking `finishMethod` if verifying in the original tab. If verifying + * in another tab, the default behavior is returned. + * + * @param {String} brokerMethod + * @param {String} finishMethod + * @returns {Promise} + */ + function finishOAuthFlowIfOriginalTab (brokerMethod, finishMethod) { + return function (account) { + // The user may have replaced the original tab with the verification + // tab. If this is the case, send the OAuth result to the RP. + // + // The slight delay is to allow the functional tests time to bind + // event handlers before the flow completes. + return proto[brokerMethod].call(this, account) + .delay(this.DELAY_BROKER_RESPONSE_MS) + .then((behavior) => { + if (this.isOriginalTab()) { + return this[finishMethod](account) + .then(() => { + return new HaltBehavior(); + }); + } + return behavior; + }); + }; + } + + module.exports = OAuthAuthenticationBroker.extend({ type: 'redirect', + initialize (options) { options = options || {}; this._metrics = options.metrics; - return proto.initialize.call(this, options); }, + DELAY_BROKER_RESPONSE_MS: 100, + sendOAuthResultToRelier (result) { var win = this.window; return this._metrics.flush() @@ -76,41 +108,7 @@ define(function (require, exports, module) { }); }, - afterCompleteSignUp (account) { - // The user may have replaced the original tab with the verification - // tab. If this is the case, send the OAuth result to the RP. - // - // The slight delay is to allow the functional tests time to bind - // event handlers before the flow completes. - return proto.afterCompleteSignUp.call(this, account) - .delay(100) - .then((behavior) => { - if (this.isOriginalTab()) { - return this.finishOAuthSignUpFlow(account) - .then(() => { - return new HaltBehavior(); - }); - } - return behavior; - }); - }, - - afterCompleteResetPassword (account) { - // The user may have replaced the original tab with the verification - // tab. If this is the case, send the OAuth result to the RP. - return proto.afterCompleteResetPassword.call(this, account) - .then((behavior) => { - if (this.isOriginalTab()) { - return this.finishOAuthSignInFlow(account) - .then(() => { - return new HaltBehavior(); - }); - } - - return behavior; - }); - } + afterCompleteResetPassword: finishOAuthFlowIfOriginalTab('afterCompleteResetPassword', 'finishOAuthSignInFlow'), + afterCompleteSignUp: finishOAuthFlowIfOriginalTab('afterCompleteSignUp', 'finishOAuthSignUpFlow'), }); - - module.exports = RedirectAuthenticationBroker; }); diff --git a/app/tests/spec/models/auth_brokers/oauth-redirect.js b/app/tests/spec/models/auth_brokers/oauth-redirect.js index ad89eac9a..cfb5b383a 100644 --- a/app/tests/spec/models/auth_brokers/oauth-redirect.js +++ b/app/tests/spec/models/auth_brokers/oauth-redirect.js @@ -5,7 +5,7 @@ define(function (require, exports, module) { 'use strict'; - const chai = require('chai'); + const { assert } = require('chai'); const Constants = require('lib/constants'); const p = require('lib/promise'); const RedirectAuthenticationBroker = require('models/auth_brokers/oauth-redirect'); @@ -15,10 +15,9 @@ define(function (require, exports, module) { const User = require('models/user'); const WindowMock = require('../../../mocks/window'); - var assert = chai.assert; var REDIRECT_TO = 'https://redirect.here'; - describe('models/auth_brokers/redirect', function () { + describe('models/auth_brokers/redirect', () => { var account; var broker; var metrics; @@ -26,8 +25,11 @@ define(function (require, exports, module) { var user; var windowMock; - beforeEach(function () { - metrics = { flush: sinon.spy(p) }; + beforeEach(() => { + metrics = { + flush: sinon.spy(p), + logEvent: () => {} + }; relier = new Relier(); user = new User(); windowMock = new WindowMock(); @@ -41,31 +43,32 @@ define(function (require, exports, module) { session: Session, window: windowMock }); + broker.DELAY_BROKER_RESPONSE_MS = 0; - sinon.stub(broker, 'finishOAuthFlow', function () { + sinon.stub(broker, 'finishOAuthFlow', () => { return p(); }); }); - it('has the `signup` capability by default', function () { + it('has the `signup` capability by default', () => { assert.isTrue(broker.hasCapability('signup')); }); - it('does not have the `handleSignedInNotification` capability by default', function () { + it('does not have the `handleSignedInNotification` capability by default', () => { assert.isFalse(broker.hasCapability('handleSignedInNotification')); }); - it('has the `emailVerificationMarketingSnippet` capability by default', function () { + it('has the `emailVerificationMarketingSnippet` capability by default', () => { assert.isTrue(broker.hasCapability('emailVerificationMarketingSnippet')); }); - describe('sendOAuthResultToRelier', function () { - describe('with no error', function () { - it('prepares window to be closed', function () { + describe('sendOAuthResultToRelier', () => { + describe('with no error', () => { + it('prepares window to be closed', () => { return broker.sendOAuthResultToRelier({ redirect: REDIRECT_TO }) - .then(function () { + .then(() => { assert.isTrue(metrics.flush.calledOnce); assert.lengthOf(metrics.flush.getCall(0).args, 0); assert.equal(windowMock.location.href, REDIRECT_TO); @@ -73,13 +76,13 @@ define(function (require, exports, module) { }); }); - describe('with an error', function () { - it('appends an error query parameter', function () { + describe('with an error', () => { + it('appends an error query parameter', () => { return broker.sendOAuthResultToRelier({ error: 'error', redirect: REDIRECT_TO }) - .then(function () { + .then(() => { assert.isTrue(metrics.flush.calledOnce); assert.lengthOf(metrics.flush.getCall(0).args, 0); assert.include(windowMock.location.href, REDIRECT_TO); @@ -88,14 +91,14 @@ define(function (require, exports, module) { }); }); - describe('with an action', function () { - it('appends an action query parameter', function () { + describe('with an action', () => { + it('appends an action query parameter', () => { var action = Constants.OAUTH_ACTION_SIGNIN; return broker.sendOAuthResultToRelier({ action: action, redirect: REDIRECT_TO }) - .then(function () { + .then(() => { assert.isTrue(metrics.flush.calledOnce); assert.lengthOf(metrics.flush.getCall(0).args, 0); assert.include(windowMock.location.href, REDIRECT_TO); @@ -104,13 +107,13 @@ define(function (require, exports, module) { }); }); - describe('with existing query parameters', function () { - it('passes through existing parameters unchanged', function () { + describe('with existing query parameters', () => { + it('passes through existing parameters unchanged', () => { return broker.sendOAuthResultToRelier({ error: 'error', redirect: REDIRECT_TO + '?test=param' }) - .then(function () { + .then(() => { assert.isTrue(metrics.flush.calledOnce); assert.lengthOf(metrics.flush.getCall(0).args, 0); assert.include(windowMock.location.href, REDIRECT_TO); @@ -121,75 +124,75 @@ define(function (require, exports, module) { }); }); - describe('persistVerificationData', function () { - it('sets the Original Tab marker', function () { + describe('persistVerificationData', () => { + it('sets the Original Tab marker', () => { return broker.persistVerificationData(account) - .then(function () { + .then(() => { assert.isTrue(broker.isOriginalTab()); }); }); }); - describe('finishOAuthFlow', function () { - it('clears the original tab marker', function () { + describe('finishOAuthFlow', () => { + it('clears the original tab marker', () => { broker.finishOAuthFlow.restore(); - sinon.stub(broker, 'getOAuthResult', function () { + sinon.stub(broker, 'getOAuthResult', () => { return p({}); }); - sinon.stub(broker, 'sendOAuthResultToRelier', function () { + sinon.stub(broker, 'sendOAuthResultToRelier', () => { return p(); }); return broker.persistVerificationData(account) - .then(function () { + .then(() => { return broker.finishOAuthFlow(account); }) - .then(function () { + .then(() => { assert.isTrue(broker.getOAuthResult.calledWith(account)); assert.isFalse(broker.isOriginalTab()); }); }); }); - describe('afterCompleteSignUp', function () { - it('finishes the oauth flow if the user verifies in the original tab', function () { + describe('afterCompleteSignUp', () => { + it('finishes the oauth flow if the user verifies in the original tab', () => { return broker.persistVerificationData(account) - .then(function () { + .then(() => { return broker.afterCompleteSignUp(account); }) - .then(function () { + .then(() => { assert.isTrue(broker.finishOAuthFlow.calledWith(account, { action: Constants.OAUTH_ACTION_SIGNUP })); }); }); - it('does not finish the oauth flow if the user verifies in another tab', function () { + it('does not finish the oauth flow if the user verifies in another tab', () => { return broker.afterCompleteSignUp(account) - .then(function () { + .then(() => { assert.isFalse(broker.finishOAuthFlow.calledWith(account)); }); }); }); - describe('afterCompleteResetPassword', function () { - it('finishes the oauth flow if the user verifies in the original tab', function () { + describe('afterCompleteResetPassword', () => { + it('finishes the oauth flow if the user verifies in the original tab', () => { return broker.persistVerificationData(account) - .then(function () { + .then(() => { return broker.afterCompleteResetPassword(account); }) - .then(function () { + .then(() => { assert.isTrue(broker.finishOAuthFlow.calledWith(account, { action: Constants.OAUTH_ACTION_SIGNIN })); }); }); - it('does not finish the oauth flow if the user verifies in another tab', function () { + it('does not finish the oauth flow if the user verifies in another tab', () => { return broker.afterCompleteResetPassword(account) - .then(function () { + .then(() => { assert.isFalse(broker.finishOAuthFlow.calledWith(account)); }); });