From 0149fc59d64124a1d3b5d8d62bd23a9528cf74ea Mon Sep 17 00:00:00 2001 From: SleepWalker Date: Sun, 7 Aug 2016 16:54:59 +0300 Subject: [PATCH] Show 404 for unknown authFlow requests. Pass {path, query, params} as first argument to authFlow#handleRequest --- src/routes.js | 3 +- src/services/authFlow/ActivationState.js | 4 +- src/services/authFlow/AuthFlow.js | 37 ++++++++++++------- src/services/authFlow/OAuthState.js | 2 +- src/services/authFlow/RecoverPasswordState.js | 4 +- .../services/authFlow/ActivationState.test.js | 4 +- .../authFlow/AuthFlow.functional.test.js | 10 ++--- tests/services/authFlow/AuthFlow.test.js | 35 ++++++++++++++---- tests/services/authFlow/OAuthState.test.js | 4 +- .../authFlow/RecoverPasswordState.test.js | 4 +- tests/services/authFlow/helpers.js | 3 +- 11 files changed, 70 insertions(+), 40 deletions(-) diff --git a/src/routes.js b/src/routes.js index d6aa0a7..11d1a8e 100644 --- a/src/routes.js +++ b/src/routes.js @@ -31,7 +31,8 @@ export default function routesFactory(store) { authFlow.setStore(store); const startAuthFlow = { - onEnter: ({location}, replace, callback) => authFlow.handleRequest(location.pathname, replace, callback) + onEnter: ({location: {query, pathname: path}, params}, replace, callback) => + authFlow.handleRequest({path, params, query}, replace, callback) }; const userOnly = { diff --git a/src/services/authFlow/ActivationState.js b/src/services/authFlow/ActivationState.js index 4718544..491c98c 100644 --- a/src/services/authFlow/ActivationState.js +++ b/src/services/authFlow/ActivationState.js @@ -9,8 +9,8 @@ export default class ActivationState extends AbstractState { if (user.isActive) { context.setState(new CompleteState()); } else { - const url = context.getCurrentPath().includes('/activation') - ? context.getCurrentPath() + const url = context.getRequest().path.includes('/activation') + ? context.getRequest().path : '/activation'; context.navigate(url); } diff --git a/src/services/authFlow/AuthFlow.js b/src/services/authFlow/AuthFlow.js index 92438d2..2272b02 100644 --- a/src/services/authFlow/AuthFlow.js +++ b/src/services/authFlow/AuthFlow.js @@ -25,8 +25,10 @@ export default class AuthFlow { setStore(store) { this.navigate = (route) => { - if (this.currentPath !== route) { - this.currentPath = route; + if (this.getRequest().path !== route) { + this.currentRequest = { + path: route + }; if (this.replace) { this.replace(route); @@ -89,33 +91,40 @@ export default class AuthFlow { } } - getCurrentPath() { - return this.currentPath; - } - - getQuery() { - return this.getState().routing.location.query; + /** + * @return {object} - current request object + */ + getRequest() { + return this.currentRequest ? {...this.currentRequest} : {}; } /** * This should be called from onEnter prop of react-router Route component * - * @param {string} path + * @param {object} request + * @param {string} request.path + * @param {object} request.params + * @param {object} request.query * @param {function} replace * @param {function} [callback = function() {}] - an optional callback function to be called, when state will be stabilized * (state's enter function's promise resolved) */ - handleRequest(path, replace, callback = function() {}) { + handleRequest(request, replace, callback = function() {}) { + const {path, params = {}, query = {}} = request; this.replace = replace; this.onReady = callback; - if (this.currentPath === path) { + if (!path) { + throw new Error('The request.path is required'); + } + + if (this.getRequest().path === path) { // we are already handling this path this.onReady(); return; } - this.currentPath = path; + this.currentRequest = request; if (path === '/') { // reset oauth data if user tried to navigate to index route @@ -159,7 +168,9 @@ export default class AuthFlow { break; default: - throw new Error(`Unsupported request: ${path}`); + console.log('Unsupported request', {path, query, params}); + replace('/404'); + break; } } diff --git a/src/services/authFlow/OAuthState.js b/src/services/authFlow/OAuthState.js index cec47c6..64c007b 100644 --- a/src/services/authFlow/OAuthState.js +++ b/src/services/authFlow/OAuthState.js @@ -3,7 +3,7 @@ import CompleteState from './CompleteState'; export default class OAuthState extends AbstractState { enter(context) { - const query = context.getQuery(); + const {query} = context.getRequest(); return context.run('oAuthValidate', { clientId: query.client_id, diff --git a/src/services/authFlow/RecoverPasswordState.js b/src/services/authFlow/RecoverPasswordState.js index c274d4e..008fdd3 100644 --- a/src/services/authFlow/RecoverPasswordState.js +++ b/src/services/authFlow/RecoverPasswordState.js @@ -7,8 +7,8 @@ export default class RecoverPasswordState extends AbstractState { const {user} = context.getState(); if (user.isGuest) { - const url = context.getCurrentPath().includes('/recover-password') - ? context.getCurrentPath() + const url = context.getRequest().path.includes('/recover-password') + ? context.getRequest().path : '/recover-password'; context.navigate(url); } else { diff --git a/tests/services/authFlow/ActivationState.test.js b/tests/services/authFlow/ActivationState.test.js index c56e149..999bc31 100644 --- a/tests/services/authFlow/ActivationState.test.js +++ b/tests/services/authFlow/ActivationState.test.js @@ -30,7 +30,7 @@ describe('ActivationState', () => { } }); - context.getCurrentPath.returns(expectedPath); + context.getRequest.returns({path: expectedPath}); expectNavigate(mock, '/activation'); @@ -45,7 +45,7 @@ describe('ActivationState', () => { } }); - context.getCurrentPath.returns(expectedPath); + context.getRequest.returns({path: expectedPath}); expectNavigate(mock, expectedPath); diff --git a/tests/services/authFlow/AuthFlow.functional.test.js b/tests/services/authFlow/AuthFlow.functional.test.js index 652f448..d5d6b9f 100644 --- a/tests/services/authFlow/AuthFlow.functional.test.js +++ b/tests/services/authFlow/AuthFlow.functional.test.js @@ -30,10 +30,10 @@ describe('AuthFlow.functional', () => { flow = new AuthFlow(actions); flow.setStore(store); - navigate = function navigate(url) { // emulates router behaviour - if (navigate.lastUrl !== url) { - navigate.lastUrl = url; - flow.handleRequest(url, navigate); + navigate = function navigate(path, extra = {}) { // emulates router behaviour + if (navigate.lastUrl !== path) { + navigate.lastUrl = path; + flow.handleRequest({path, ...extra}, navigate); } }; @@ -97,7 +97,7 @@ describe('AuthFlow.functional', () => { redirectUri: expectedRedirect })}); - navigate('/oauth2'); + navigate('/oauth2', {query: {}}); expect(flow.run, 'to have calls satisfying', [ ['oAuthValidate', {}], diff --git a/tests/services/authFlow/AuthFlow.test.js b/tests/services/authFlow/AuthFlow.test.js index b98dc96..9ae4013 100644 --- a/tests/services/authFlow/AuthFlow.test.js +++ b/tests/services/authFlow/AuthFlow.test.js @@ -5,7 +5,6 @@ import AbstractState from 'services/authFlow/AbstractState'; import OAuthState from 'services/authFlow/OAuthState'; import RegisterState from 'services/authFlow/RegisterState'; -import AcceptRulesState from 'services/authFlow/AcceptRulesState'; import RecoverPasswordState from 'services/authFlow/RecoverPasswordState'; import ForgotPasswordState from 'services/authFlow/ForgotPasswordState'; import ActivationState from 'services/authFlow/ActivationState'; @@ -189,7 +188,7 @@ describe('AuthFlow', () => { '/resend-activation': ResendActivationState }).forEach(([path, type]) => { it(`should transition to ${type.name} if ${path}`, () => { - flow.handleRequest(path); + flow.handleRequest({path}); expect(flow.setState, 'was called once'); expect(flow.setState, 'to have a call satisfying', [ @@ -199,7 +198,7 @@ describe('AuthFlow', () => { }); it('should run setOAuthRequest if /', () => { - flow.handleRequest('/'); + flow.handleRequest({path: '/'}); expect(flow.run, 'was called once'); expect(flow.run, 'to have a call satisfying', ['setOAuthRequest', {}]); @@ -208,7 +207,7 @@ describe('AuthFlow', () => { it('should call callback', () => { const callback = sinon.stub().named('callback'); - flow.handleRequest('/', () => {}, callback); + flow.handleRequest({path: '/'}, () => {}, callback); expect(callback, 'was called once'); }); @@ -223,7 +222,7 @@ describe('AuthFlow', () => { flow.setState = AuthFlow.prototype.setState.bind(flow, state); - flow.handleRequest('/', () => {}, callback); + flow.handleRequest({path: '/'}, () => {}, callback); expect(resolve, 'to be', callback); @@ -236,8 +235,8 @@ describe('AuthFlow', () => { const path = '/oauth2'; const callback = sinon.stub(); - flow.handleRequest(path, () => {}, callback); - flow.handleRequest(path, () => {}, callback); + flow.handleRequest({path}, () => {}, callback); + flow.handleRequest({path}, () => {}, callback); expect(flow.setState, 'was called once'); expect(flow.setState, 'to have a call satisfying', [ @@ -247,7 +246,27 @@ describe('AuthFlow', () => { }); it('throws if unsupported request', () => { - expect(() => flow.handleRequest('/foo/bar'), 'to throw', 'Unsupported request: /foo/bar'); + const replace = sinon.stub().named('replace'); + + flow.handleRequest({path: '/foo/bar'}, replace); + + expect(replace, 'to have a call satisfying', ['/404']); + }); + }); + + describe('#getRequest()', () => { + beforeEach(() => { + sinon.stub(flow, 'setState').named('flow.setState'); + sinon.stub(flow, 'run').named('flow.run'); + }); + + it('should return a copy of current request', () => { + const request = {path: '/', query: {foo: 'bar'}, params: {baz: 'bud'}}; + + flow.handleRequest(request); + + expect(flow.getRequest(), 'to equal', request); + expect(flow.getRequest(), 'not to be', request); }); }); }); diff --git a/tests/services/authFlow/OAuthState.test.js b/tests/services/authFlow/OAuthState.test.js index 561440f..5e240a7 100644 --- a/tests/services/authFlow/OAuthState.test.js +++ b/tests/services/authFlow/OAuthState.test.js @@ -30,7 +30,7 @@ describe('OAuthState', () => { state: 'state' }; - context.getQuery.returns(query); + context.getRequest.returns({query}); expectRun( mock, @@ -50,7 +50,7 @@ describe('OAuthState', () => { it('should transition to complete state on success', () => { const promise = Promise.resolve(); - context.getQuery.returns({}); + context.getRequest.returns({query: {}}); mock.expects('run').returns(promise); expectState(mock, CompleteState); diff --git a/tests/services/authFlow/RecoverPasswordState.test.js b/tests/services/authFlow/RecoverPasswordState.test.js index da042cb..a4930e8 100644 --- a/tests/services/authFlow/RecoverPasswordState.test.js +++ b/tests/services/authFlow/RecoverPasswordState.test.js @@ -28,7 +28,7 @@ describe('RecoverPasswordState', () => { user: {isGuest: true} }); - context.getCurrentPath.returns(expectedPath); + context.getRequest.returns({path: expectedPath}); expectNavigate(mock, expectedPath); @@ -41,7 +41,7 @@ describe('RecoverPasswordState', () => { user: {isGuest: true} }); - context.getCurrentPath.returns(expectedPath); + context.getRequest.returns({path: expectedPath}); expectNavigate(mock, expectedPath); diff --git a/tests/services/authFlow/helpers.js b/tests/services/authFlow/helpers.js index 6ee896b..387c506 100644 --- a/tests/services/authFlow/helpers.js +++ b/tests/services/authFlow/helpers.js @@ -7,8 +7,7 @@ export function bootstrap() { getState: sinon.stub(), run() {}, setState() {}, - getCurrentPath: sinon.stub(), - getQuery: sinon.stub(), + getRequest: sinon.stub(), navigate() {} };