From 3f869f92e2a6b2bac7260d98fd5ce5e78a63c249 Mon Sep 17 00:00:00 2001 From: SleepWalker Date: Mon, 25 Dec 2017 22:03:21 +0200 Subject: [PATCH] #365: improve token api errors handling --- package.json | 2 +- src/components/accounts/actions.js | 11 ++------ src/components/accounts/actions.test.js | 15 ++-------- src/services/api/accounts.js | 37 +++++++++++++++++++++---- src/services/api/authentication.js | 26 +++++++++++++++-- src/services/api/authentication.test.js | 37 +++++++++++++++++++++++++ 6 files changed, 98 insertions(+), 30 deletions(-) diff --git a/package.json b/package.json index a0588eb..a41946d 100644 --- a/package.json +++ b/package.json @@ -8,7 +8,7 @@ "license": "UNLICENSED", "repository": "git@gitlab.ely.by:elyby/accounts.git", "engines": { - "node": "9" + "node": ">=8.0.0" }, "scripts": { "start": "yarn run clean && yarn run build:dll && webpack-dev-server --progress --colors", diff --git a/src/components/accounts/actions.js b/src/components/accounts/actions.js index 5352c0d..00dfa4b 100644 --- a/src/components/accounts/actions.js +++ b/src/components/accounts/actions.js @@ -6,7 +6,6 @@ import { updateUser, setGuest } from 'components/user/actions'; import { setLocale } from 'components/i18n/actions'; import { setAccountSwitcher } from 'components/auth/actions'; import logger from 'services/logger'; -import { InternalServerError } from 'services/request'; import { add, @@ -38,15 +37,9 @@ export function authenticate({token, refreshToken}) { return (dispatch, getState) => authentication.validateToken({token, refreshToken}) .catch((resp = {}) => { - if (resp instanceof InternalServerError) { - // delegate error recovering to the later logic - return Promise.reject(resp); - } - - logger.warn('Error validating token during auth', { - resp - }); + // all the logic to get the valid token was failed, + // we must forget current token, but leave other user's accounts return dispatch(logoutAll()) .then(() => Promise.reject(resp)); }) diff --git a/src/components/accounts/actions.test.js b/src/components/accounts/actions.test.js index c15b750..ae03e73 100644 --- a/src/components/accounts/actions.test.js +++ b/src/components/accounts/actions.test.js @@ -3,7 +3,6 @@ import sinon from 'sinon'; import { browserHistory } from 'services/history'; -import logger from 'services/logger'; import { InternalServerError } from 'services/request'; import { sessionStorage } from 'services/localStorage'; import authentication from 'services/api/authentication'; @@ -58,7 +57,6 @@ describe('components/accounts/actions', () => { }); sinon.stub(authentication, 'validateToken').named('authentication.validateToken'); - sinon.stub(logger, 'warn').named('logger.warn'); authentication.validateToken.returns(Promise.resolve({ token: account.token, refreshToken: account.refreshToken, @@ -68,7 +66,6 @@ describe('components/accounts/actions', () => { afterEach(() => { authentication.validateToken.restore(); - logger.warn.restore(); }); describe('#authenticate()', () => { @@ -122,9 +119,6 @@ describe('components/accounts/actions', () => { authentication.validateToken.returns(Promise.reject({})); return expect(authenticate(account)(dispatch, getState), 'to be rejected').then(() => { - expect(logger.warn, 'to have a call satisfying', [ - 'Error validating token during auth', {} - ]); expect(dispatch, 'to have a call satisfying', [ {payload: {isGuest: true}}, ]); @@ -139,13 +133,10 @@ describe('components/accounts/actions', () => { authentication.validateToken.returns(Promise.reject(resp)); - return expect(authenticate(account)(dispatch, getState), 'to be rejected with', resp).then(() => { - expect(dispatch, 'to have no calls satisfying', [ + return expect(authenticate(account)(dispatch, getState), 'to be rejected with', resp) + .then(() => expect(dispatch, 'to have no calls satisfying', [ {payload: {isGuest: true}}, - ]); - - expect(logger.warn, 'was not called'); - }); + ])); }); it('marks user as stranger, if there is no refreshToken', () => { diff --git a/src/services/api/accounts.js b/src/services/api/accounts.js index 05ce95c..145e51a 100644 --- a/src/services/api/accounts.js +++ b/src/services/api/accounts.js @@ -1,14 +1,30 @@ +// @flow import request from 'services/request'; +type UserResponse = { + elyProfileLink: string, + email: string, + hasMojangUsernameCollision: bool, + id: number, + isActive: bool, + isOtpEnabled: bool, + lang: string, + passwordChangedAt: number, // timestamp + registeredAt: number, // timestamp + shouldAcceptRules: bool, + username: string, + uuid: string, +}; + export default { /** * @param {object} options * @param {object} [options.token] - an optional token to overwrite headers * in middleware and disable token auto-refresh * - * @return {Promise} + * @return {Promise} */ - current(options = {}) { + current(options: { token?: ?string } = {}): Promise { return request.get('/api/accounts/current', {}, { token: options.token }); @@ -19,6 +35,11 @@ export default { newPassword = '', newRePassword = '', logoutAll = true + }: { + password?: string, + newPassword?: string, + newRePassword?: string, + logoutAll?: bool, }) { return request.post( '/api/accounts/change-password', @@ -33,6 +54,9 @@ export default { changeUsername({ username = '', password = '' + }: { + username?: string, + password?: string, }) { return request.post( '/api/accounts/change-username', @@ -40,14 +64,14 @@ export default { ); }, - changeLang(lang) { + changeLang(lang: string) { return request.post( '/api/accounts/change-lang', {lang} ); }, - requestEmailChange({password = ''}) { + requestEmailChange({password = ''}: { password?: string }) { return request.post( '/api/accounts/change-email/initialize', {password} @@ -57,6 +81,9 @@ export default { setNewEmail({ email = '', key = '' + }: { + email?: string, + key?: string, }) { return request.post( '/api/accounts/change-email/submit-new-email', @@ -64,7 +91,7 @@ export default { ); }, - confirmNewEmail({key}) { + confirmNewEmail({key}: { key: string }) { return request.post( '/api/accounts/change-email/confirm-new-email', {key} diff --git a/src/services/api/authentication.js b/src/services/api/authentication.js index d3b7e44..b81806c 100644 --- a/src/services/api/authentication.js +++ b/src/services/api/authentication.js @@ -1,5 +1,6 @@ // @flow -import request from 'services/request'; +import logger from 'services/logger'; +import request, { InternalServerError } from 'services/request'; import accounts from 'services/api/accounts'; const authentication = { @@ -91,12 +92,31 @@ const authentication = { .then(() => accounts.current({token})) .then((user) => ({token, refreshToken, user})) .catch((resp) => { - if (resp.message === 'Token expired') { + if (resp instanceof InternalServerError) { + // delegate error recovering to the bsod middleware + return new Promise(() => {}); + } + + if (['Token expired', 'Incorrect token'].includes(resp.message)) { return authentication.requestToken(refreshToken) .then(({token}) => accounts.current({token}) .then((user) => ({token, refreshToken, user})) - ); + ) + .catch((error) => { + logger.error('Failed refreshing token during token validation', { + error + }); + + return Promise.reject(error); + }); + } + + const errors = resp.errors || {}; + if (errors.refresh_token !== 'error.refresh_token_not_exist') { + logger.error('Unexpected error during token validation', { + resp + }); } return Promise.reject(resp); diff --git a/src/services/api/authentication.test.js b/src/services/api/authentication.test.js index f359347..e0cfbd7 100644 --- a/src/services/api/authentication.test.js +++ b/src/services/api/authentication.test.js @@ -125,6 +125,43 @@ describe('authentication api', () => { ); }); }); + + describe('when token is incorrect', () => { + const expiredResponse = { + name: 'Unauthorized', + message: 'Incorrect token', + code: 0, + status: 401, + type: 'yii\\web\\UnauthorizedHttpException' + }; + const newToken = 'baz'; + + beforeEach(() => { + sinon.stub(authentication, 'requestToken'); + + accounts.current.onCall(0).returns(Promise.reject(expiredResponse)); + authentication.requestToken.returns(Promise.resolve({token: newToken})); + }); + + afterEach(() => { + authentication.requestToken.restore(); + }); + + it('resolves with new token and user object', () => + expect(authentication.validateToken(validTokens), + 'to be fulfilled with', {...validTokens, token: newToken, user} + ) + ); + + it('rejects if token request failed', () => { + const error = 'Something wrong'; + authentication.requestToken.returns(Promise.reject(error)); + + return expect(authentication.validateToken(validTokens), + 'to be rejected with', error + ); + }); + }); }); describe('#logout', () => {