From f1110b0067e0e767236f6b457c3cd653778eeeec Mon Sep 17 00:00:00 2001 From: SleepWalker Date: Fri, 6 Jan 2017 08:04:14 +0200 Subject: [PATCH] #246: remove redundant /current calls during account authentication --- src/components/accounts/actions.js | 39 +++++++++++------------ src/services/api/authentication.js | 10 ++++-- tests/components/accounts/actions.test.js | 32 +++++++++++-------- tests/services/api/authentication.test.js | 16 ++++++---- 4 files changed, 55 insertions(+), 42 deletions(-) diff --git a/src/components/accounts/actions.js b/src/components/accounts/actions.js index a3bb5ab..04af8fc 100644 --- a/src/components/accounts/actions.js +++ b/src/components/accounts/actions.js @@ -25,28 +25,27 @@ import logger from 'services/logger'; export function authenticate({token, refreshToken}) { return (dispatch) => authentication.validateToken({token, refreshToken}) - .catch(() => { - // TODO: log this case - dispatch(logoutAll()); + .catch((resp) => { + logger.warn('Error validating token during auth', { + resp + }); - return Promise.reject(); + return dispatch(logoutAll()) + .then(() => Promise.reject()); }) - .then(({token, refreshToken}) => - accounts.current({token}) - .then((user) => ({ - user: { - isGuest: false, - ...user - }, - account: { - id: user.id, - username: user.username, - email: user.email, - token, - refreshToken - } - })) - ) + .then(({token, refreshToken, user}) => ({ + user: { + isGuest: false, + ...user + }, + account: { + id: user.id, + username: user.username, + email: user.email, + token, + refreshToken + } + })) .then(({user, account}) => { dispatch(add(account)); dispatch(activate(account)); diff --git a/src/services/api/authentication.js b/src/services/api/authentication.js index 6ab890f..5c25e40 100644 --- a/src/services/api/authentication.js +++ b/src/services/api/authentication.js @@ -55,7 +55,8 @@ const authentication = { * @param {string} options.refreshToken * * @return {Promise} - resolves with options.token or with a new token - * if it was refreshed + * if it was refreshed. As a side effect the response + * will have a `user` field with current user data */ validateToken({token, refreshToken}) { return new Promise((resolve) => { @@ -66,11 +67,14 @@ const authentication = { resolve(); }) .then(() => accounts.current({token})) - .then(() => ({token, refreshToken})) + .then((user) => ({token, refreshToken, user})) .catch((resp) => { if (resp.message === 'Token expired') { return authentication.requestToken(refreshToken) - .then(({token}) => ({token, refreshToken})); + .then(({token}) => + accounts.current({token}) + .then((user) => ({token, refreshToken, user})) + ); } return Promise.reject(resp); diff --git a/tests/components/accounts/actions.test.js b/tests/components/accounts/actions.test.js index 2413ab8..3bd0aee 100644 --- a/tests/components/accounts/actions.test.js +++ b/tests/components/accounts/actions.test.js @@ -45,30 +45,30 @@ describe('components/accounts/actions', () => { getState = sinon.stub().named('store.getState'); getState.returns({ - accounts: [], + accounts: { + available: [], + active: null + }, user: {} }); sinon.stub(authentication, 'validateToken').named('authentication.validateToken'); authentication.validateToken.returns(Promise.resolve({ token: account.token, - refreshToken: account.refreshToken + refreshToken: account.refreshToken, + user })); - - sinon.stub(accounts, 'current').named('accounts.current'); - accounts.current.returns(Promise.resolve(user)); }); afterEach(() => { authentication.validateToken.restore(); - accounts.current.restore(); }); describe('#authenticate()', () => { it('should request user state using token', () => authenticate(account)(dispatch).then(() => - expect(accounts.current, 'to have a call satisfying', [ - {token: account.token} + expect(authentication.validateToken, 'to have a call satisfying', [ + {token: account.token, refreshToken: account.refreshToken} ]) ) ); @@ -112,17 +112,23 @@ describe('components/accounts/actions', () => { ); it('rejects when bad auth data', () => { - accounts.current.returns(Promise.reject({})); + authentication.validateToken.returns(Promise.reject({})); - return expect(authenticate(account)(dispatch), 'to be rejected').then(() => - expect(dispatch, 'was not called') - ); + return expect(authenticate(account)(dispatch), 'to be rejected').then(() => { + expect(dispatch, 'to have a call satisfying', [ + {payload: {isGuest: true}}, + ]); + expect(dispatch, 'to have a call satisfying', [ + reset() + ]); + }); }); it('marks user as stranger, if there is no refreshToken', () => { const expectedKey = `stranger${account.id}`; authentication.validateToken.returns(Promise.resolve({ - token: account.token + token: account.token, + user })); sessionStorage.removeItem(expectedKey); diff --git a/tests/services/api/authentication.test.js b/tests/services/api/authentication.test.js index 1866351..3e5bc65 100644 --- a/tests/services/api/authentication.test.js +++ b/tests/services/api/authentication.test.js @@ -40,11 +40,12 @@ describe('authentication api', () => { describe('#validateToken()', () => { const validTokens = {token: 'foo', refreshToken: 'bar'}; + const user = {id: 1}; beforeEach(() => { sinon.stub(accounts, 'current'); - accounts.current.returns(Promise.resolve()); + accounts.current.returns(Promise.resolve(user)); }); afterEach(() => { @@ -60,8 +61,11 @@ describe('authentication api', () => { }) ); - it('should resolve with both tokens', () => - expect(authentication.validateToken(validTokens), 'to be fulfilled with', validTokens) + it('should resolve with both tokens and user object', () => + expect(authentication.validateToken(validTokens), 'to be fulfilled with', { + ...validTokens, + user + }) ); it('rejects if token has a bad type', () => @@ -96,7 +100,7 @@ describe('authentication api', () => { beforeEach(() => { sinon.stub(authentication, 'requestToken'); - accounts.current.returns(Promise.reject(expiredResponse)); + accounts.current.onCall(0).returns(Promise.reject(expiredResponse)); authentication.requestToken.returns(Promise.resolve({token: newToken})); }); @@ -104,9 +108,9 @@ describe('authentication api', () => { authentication.requestToken.restore(); }); - it('resolves with new token', () => + it('resolves with new token and user object', () => expect(authentication.validateToken(validTokens), - 'to be fulfilled with', {...validTokens, token: newToken} + 'to be fulfilled with', {...validTokens, token: newToken, user} ) );