From 9afa4be8cb877e88ade180e48b76fb5b4416b7e8 Mon Sep 17 00:00:00 2001 From: SleepWalker Date: Sat, 30 Dec 2017 21:04:31 +0200 Subject: [PATCH] #365: Redirect user to login page, when token can not be refreshed. Further improvement of auth errors handling --- src/components/accounts/AccountSwitcher.js | 9 +- src/components/accounts/actions.js | 173 +++++++++++++++--- src/components/accounts/actions.test.js | 73 ++++---- .../accounts/actions/pure-actions.js | 20 +- src/components/accounts/index.js | 4 +- src/components/accounts/reducer.js | 116 ++++++++---- src/components/accounts/reducer.test.js | 10 +- src/components/auth/actions.js | 9 +- src/components/user/factory.js | 8 +- .../middlewares/bearerHeaderMiddleware.js | 16 +- .../bearerHeaderMiddleware.test.js | 11 +- .../middlewares/refreshTokenMiddleware.js | 76 ++------ .../refreshTokenMiddleware.test.js | 123 +++++++++---- src/containers/PrivateRoute.js | 16 +- src/services/api/authentication.js | 83 +++++---- src/services/authFlow/AuthFlow.js | 16 +- src/services/authFlow/CompleteState.js | 24 ++- src/services/authFlow/CompleteState.test.js | 24 +-- src/services/authFlow/LoginState.js | 17 +- src/services/authFlow/LoginState.test.js | 4 +- 20 files changed, 519 insertions(+), 313 deletions(-) diff --git a/src/components/accounts/AccountSwitcher.js b/src/components/accounts/AccountSwitcher.js index ce0f4bd..2b1e97c 100644 --- a/src/components/accounts/AccountSwitcher.js +++ b/src/components/accounts/AccountSwitcher.js @@ -21,14 +21,7 @@ export class AccountSwitcher extends Component { removeAccount: PropTypes.func.isRequired, onAfterAction: PropTypes.func, // called after each action performed onSwitch: PropTypes.func, // called after switching an account. The active account will be passed as arg - accounts: PropTypes.shape({ // TODO: accounts shape - active: PropTypes.shape({ - id: PropTypes.number - }), - available: PropTypes.arrayOf(PropTypes.shape({ - id: PropTypes.number - })) - }), + accounts: PropTypes.object, // eslint-disable-line user: userShape, // TODO: remove me, when we will be sure, that accounts.active is always set for user (event after register) skin: PropTypes.oneOf(skins), highlightActiveAccount: PropTypes.bool, // whether active account should be expanded and shown on the top diff --git a/src/components/accounts/actions.js b/src/components/accounts/actions.js index 00dfa4b..ca729ac 100644 --- a/src/components/accounts/actions.js +++ b/src/components/accounts/actions.js @@ -1,10 +1,13 @@ +// @flow +import { getJwtPayload } from 'functions'; import { browserHistory } from 'services/history'; - import { sessionStorage } from 'services/localStorage'; import authentication from 'services/api/authentication'; +import { setLogin } from 'components/auth/actions'; import { updateUser, setGuest } from 'components/user/actions'; import { setLocale } from 'components/i18n/actions'; import { setAccountSwitcher } from 'components/auth/actions'; +import { getActiveAccount } from 'components/accounts/reducer'; import logger from 'services/logger'; import { @@ -13,19 +16,22 @@ import { activate, reset, updateToken -} from 'components/accounts/actions/pure-actions'; +} from './actions/pure-actions'; +import type { Account, State as AccountsState } from './reducer'; + +type Dispatch = (action: Object) => Promise<*>; + +type State = { + accounts: AccountsState, + auth: { + oauth?: { + clientId?: string + }, + }, +}; export { updateToken }; -/** - * @typedef {object} Account - * @property {string} id - * @property {string} username - * @property {string} email - * @property {string} token - * @property {string} refreshToken - */ - /** * @param {Account|object} account * @param {string} account.token @@ -33,15 +39,25 @@ export { updateToken }; * * @return {function} */ -export function authenticate({token, refreshToken}) { - return (dispatch, getState) => +export function authenticate(account: Account | { + token: string, + refreshToken: ?string, +}) { + const {token, refreshToken} = account; + const email = account.email || null; + + return (dispatch: Dispatch, getState: () => State): Promise => authentication.validateToken({token, refreshToken}) .catch((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)); + // looks like we have some problems with token + // lets redirect to login page + if (typeof email === 'string') { + // TODO: we should somehow try to find email by token + dispatch(relogin(email)); + } + + return Promise.reject(resp); }) .then(({token, refreshToken, user}) => ({ user: { @@ -84,6 +100,89 @@ export function authenticate({token, refreshToken}) { }); } +export function ensureToken() { + return (dispatch: Dispatch, getState: () => State): Promise => { + const {token} = getActiveAccount(getState()) || {}; + + try { + const SAFETY_FACTOR = 300; // ask new token earlier to overcome time dissynchronization problem + const jwt = getJwtPayload(token); + + if (jwt.exp - SAFETY_FACTOR < Date.now() / 1000) { + return dispatch(requestNewToken()); + } + } catch (err) { + logger.warn('Refresh token error: bad token', { + token + }); + + dispatch(relogin()); + + return Promise.reject(new Error('Invalid token')); + } + + return Promise.resolve(); + }; +} + +export function recoverFromTokenError(error: ?{ + status: number, + message: string, +}) { + return (dispatch: Dispatch, getState: () => State): Promise => { + if (error && error.status === 401) { + const activeAccount = getActiveAccount(getState()); + + if (activeAccount && activeAccount.refreshToken) { + if ([ + 'Token expired', + 'Incorrect token', + 'You are requesting with an invalid credential.' + ].includes(error.message)) { + // request token and retry + return dispatch(requestNewToken()); + } + + logger.error('Unknown unauthorized response', { + error + }); + } + + // user's access token is outdated and we have no refreshToken + // or something unexpected happend + // in both cases we resetting all the user's state + dispatch(relogin()); + } + + return Promise.reject(error); + }; +} + +export function requestNewToken() { + return (dispatch: Dispatch, getState: () => State): Promise => { + const {refreshToken} = getActiveAccount(getState()) || {}; + + if (!refreshToken) { + dispatch(relogin()); + + return Promise.resolve(); + } + + return authentication.requestToken(refreshToken) + .then(({ token }) => { + dispatch(updateToken(token)); + }) + .catch((resp) => { + // all the logic to get the valid token was failed, + // looks like we have some problems with token + // lets redirect to login page + dispatch(relogin()); + + return Promise.reject(resp); + }); + }; +} + /** * Remove one account from current user's account list * @@ -91,9 +190,9 @@ export function authenticate({token, refreshToken}) { * * @return {function} */ -export function revoke(account) { - return (dispatch, getState) => { - const accountToReplace = getState().accounts.available.find(({id}) => id !== account.id); +export function revoke(account: Account) { + return (dispatch: Dispatch, getState: () => State): Promise => { + const accountToReplace: ?Account = getState().accounts.available.find(({id}) => id !== account.id); if (accountToReplace) { return dispatch(authenticate(accountToReplace)) @@ -107,17 +206,34 @@ export function revoke(account) { }; } +export function relogin(email?: string) { + return (dispatch: Dispatch, getState: () => State) => { + const activeAccount = getActiveAccount(getState()); + + if (!email && activeAccount) { + email = activeAccount.email; + } + + email && dispatch(setLogin(email)); + browserHistory.push('/login'); + }; +} + export function logoutAll() { - return (dispatch, getState) => { + return (dispatch: Dispatch, getState: () => State): Promise => { dispatch(setGuest()); const {accounts: {available}} = getState(); - available.forEach((account) => authentication.logout(account)); + available.forEach((account) => + authentication.logout(account) + .catch(() => { + // we don't care + }) + ); dispatch(reset()); - - browserHistory.push('/login'); + dispatch(relogin()); return Promise.resolve(); }; @@ -132,10 +248,11 @@ export function logoutAll() { * @return {function} */ export function logoutStrangers() { - return (dispatch, getState) => { - const {accounts: {available, active}} = getState(); + return (dispatch: Dispatch, getState: () => State): Promise => { + const {accounts: {available}} = getState(); + const activeAccount = getActiveAccount(getState()); - const isStranger = ({refreshToken, id}) => !refreshToken && !sessionStorage.getItem(`stranger${id}`); + const isStranger = ({refreshToken, id}: Account) => !refreshToken && !sessionStorage.getItem(`stranger${id}`); if (available.some(isStranger)) { const accountToReplace = available.filter((account) => !isStranger(account))[0]; @@ -147,7 +264,7 @@ export function logoutStrangers() { authentication.logout(account); }); - if (isStranger(active)) { + if (activeAccount && isStranger(activeAccount)) { return dispatch(authenticate(accountToReplace)); } } else { diff --git a/src/components/accounts/actions.test.js b/src/components/accounts/actions.test.js index ae03e73..999fc01 100644 --- a/src/components/accounts/actions.test.js +++ b/src/components/accounts/actions.test.js @@ -21,7 +21,7 @@ import { import { SET_LOCALE } from 'components/i18n/actions'; import { updateUser, setUser } from 'components/user/actions'; -import { setAccountSwitcher } from 'components/auth/actions'; +import { setLogin, setAccountSwitcher } from 'components/auth/actions'; const account = { id: 1, @@ -57,6 +57,10 @@ describe('components/accounts/actions', () => { }); sinon.stub(authentication, 'validateToken').named('authentication.validateToken'); + sinon.stub(browserHistory, 'push').named('browserHistory.push'); + sinon.stub(authentication, 'logout').named('authentication.logout'); + + authentication.logout.returns(Promise.resolve()); authentication.validateToken.returns(Promise.resolve({ token: account.token, refreshToken: account.refreshToken, @@ -66,6 +70,8 @@ describe('components/accounts/actions', () => { afterEach(() => { authentication.validateToken.restore(); + authentication.logout.restore(); + browserHistory.push.restore(); }); describe('#authenticate()', () => { @@ -118,14 +124,16 @@ describe('components/accounts/actions', () => { it('rejects when bad auth data', () => { authentication.validateToken.returns(Promise.reject({})); - return expect(authenticate(account)(dispatch, getState), 'to be rejected').then(() => { - expect(dispatch, 'to have a call satisfying', [ - {payload: {isGuest: true}}, - ]); - expect(dispatch, 'to have a call satisfying', [ - reset() - ]); - }); + return expect(authenticate(account)(dispatch, getState), 'to be rejected') + .then(() => { + expect(dispatch, 'to have a call satisfying', [ + setLogin(account.email) + ]); + + expect(browserHistory.push, 'to have a call satisfying', [ + '/login' + ]); + }); }); it('rejects when 5xx without logouting', () => { @@ -182,19 +190,11 @@ describe('components/accounts/actions', () => { }); describe('#revoke()', () => { - beforeEach(() => { - sinon.stub(authentication, 'logout').named('authentication.logout'); - }); - - afterEach(() => { - authentication.logout.restore(); - }); - describe('when one account available', () => { beforeEach(() => { getState.returns({ accounts: { - active: account, + active: account.id, available: [account] }, user @@ -220,8 +220,7 @@ describe('components/accounts/actions', () => { it('should update user state', () => revoke(account)(dispatch, getState).then(() => expect(dispatch, 'to have a call satisfying', [ - {payload: {isGuest: true}} - // updateUser({isGuest: true}) + setUser({isGuest: true}) ]) // expect(dispatch, 'to have calls satisfying', [ // [remove(account)], @@ -238,7 +237,7 @@ describe('components/accounts/actions', () => { beforeEach(() => { getState.returns({ accounts: { - active: account2, + active: account2.id, available: [account, account2] }, user @@ -277,19 +276,11 @@ describe('components/accounts/actions', () => { beforeEach(() => { getState.returns({ accounts: { - active: account2, + active: account2.id, available: [account, account2] }, user }); - - sinon.stub(authentication, 'logout').named('authentication.logout'); - sinon.stub(browserHistory, 'push').named('browserHistory.push'); - }); - - afterEach(() => { - authentication.logout.restore(); - browserHistory.push.restore(); }); it('should call logout api method for each account', () => { @@ -344,17 +335,11 @@ describe('components/accounts/actions', () => { beforeEach(() => { getState.returns({ accounts: { - active: foreignAccount, + active: foreignAccount.id, available: [account, foreignAccount, foreignAccount2] }, user }); - - sinon.stub(authentication, 'logout').named('authentication.logout'); - }); - - afterEach(() => { - authentication.logout.restore(); }); it('should remove stranger accounts', () => { @@ -389,7 +374,7 @@ describe('components/accounts/actions', () => { it('should not activate another account if active account is already not a stranger', () => { getState.returns({ accounts: { - active: account, + active: account.id, available: [account, foreignAccount] }, user @@ -405,7 +390,7 @@ describe('components/accounts/actions', () => { it('should not dispatch if no strangers', () => { getState.returns({ accounts: { - active: account, + active: account.id, available: [account] }, user @@ -421,7 +406,7 @@ describe('components/accounts/actions', () => { beforeEach(() => { getState.returns({ accounts: { - active: foreignAccount, + active: foreignAccount.id, available: [foreignAccount, foreignAccount2] }, user @@ -431,9 +416,13 @@ describe('components/accounts/actions', () => { }); it('logouts all accounts', () => { + expect(authentication.logout, 'to have calls satisfying', [ + [foreignAccount], + [foreignAccount2], + ]); + expect(dispatch, 'to have a call satisfying', [ - {payload: {isGuest: true}} - // updateUser({isGuest: true}) + setUser({isGuest: true}) ]); expect(dispatch, 'to have a call satisfying', [ diff --git a/src/components/accounts/actions/pure-actions.js b/src/components/accounts/actions/pure-actions.js index 4946b95..75f66fd 100644 --- a/src/components/accounts/actions/pure-actions.js +++ b/src/components/accounts/actions/pure-actions.js @@ -1,3 +1,13 @@ +// @flow +import type { + Account, + AddAction, + RemoveAction, + ActivateAction, + UpdateTokenAction, + ResetAction +} from '../reducer'; + export const ADD = 'accounts:add'; /** * @api private @@ -6,7 +16,7 @@ export const ADD = 'accounts:add'; * * @return {object} - action definition */ -export function add(account) { +export function add(account: Account): AddAction { return { type: ADD, payload: account @@ -21,7 +31,7 @@ export const REMOVE = 'accounts:remove'; * * @return {object} - action definition */ -export function remove(account) { +export function remove(account: Account): RemoveAction { return { type: REMOVE, payload: account @@ -36,7 +46,7 @@ export const ACTIVATE = 'accounts:activate'; * * @return {object} - action definition */ -export function activate(account) { +export function activate(account: Account): ActivateAction { return { type: ACTIVATE, payload: account @@ -49,7 +59,7 @@ export const RESET = 'accounts:reset'; * * @return {object} - action definition */ -export function reset() { +export function reset(): ResetAction { return { type: RESET }; @@ -61,7 +71,7 @@ export const UPDATE_TOKEN = 'accounts:updateToken'; * * @return {object} - action definition */ -export function updateToken(token) { +export function updateToken(token: string): UpdateTokenAction { return { type: UPDATE_TOKEN, payload: token diff --git a/src/components/accounts/index.js b/src/components/accounts/index.js index d74b7fa..d9d814c 100644 --- a/src/components/accounts/index.js +++ b/src/components/accounts/index.js @@ -1 +1,3 @@ -export AccountSwitcher from './AccountSwitcher'; +// @flow +export { default as AccountSwitcher } from './AccountSwitcher'; +export type { Account } from './reducer'; diff --git a/src/components/accounts/reducer.js b/src/components/accounts/reducer.js index d712eeb..7cfe908 100644 --- a/src/components/accounts/reducer.js +++ b/src/components/accounts/reducer.js @@ -1,30 +1,52 @@ -import { ADD, REMOVE, ACTIVATE, RESET, UPDATE_TOKEN } from './actions/pure-actions'; +// @flow +export type Account = { + id: number, + username: string, + email: string, + token: string, + refreshToken: ?string, +}; -/** - * @typedef {AccountsState} - * @property {Account} active - * @property {Account[]} available - */ +export type State = { + active: ?number, + available: Array, +}; + + +export type AddAction = { type: 'accounts:add', payload: Account}; +export type RemoveAction = { type: 'accounts:remove', payload: Account}; +export type ActivateAction = { type: 'accounts:activate', payload: Account}; +export type UpdateTokenAction = { type: 'accounts:updateToken', payload: string }; +export type ResetAction = { type: 'accounts:reset' }; + +type Action = + | AddAction + | RemoveAction + | ActivateAction + | UpdateTokenAction + | ResetAction; + +export function getActiveAccount(state: { accounts: State }): ?Account { + const activeAccount = state.accounts.active; + // TODO: remove activeAccount.id, when will be sure, that magor part of users have migrated to new state structure + const accountId: number | void = typeof activeAccount === 'number' ? activeAccount : (activeAccount || {}).id; + + return state.accounts.available.find((account) => account.id === accountId); +} -/** - * @param {AccountsState} state - * @param {string} options.type - * @param {object} options.payload - * - * @return {AccountsState} - */ export default function accounts( - state = { + state: State = { active: null, available: [] }, - {type, payload = {}} -) { - switch (type) { - case ADD: - if (!payload || !payload.id || !payload.token) { + action: Action +): State { + switch (action.type) { + case 'accounts:add': { + if (!action.payload || !action.payload.id || !action.payload.token) { throw new Error('Invalid or empty payload passed for accounts.add'); } + const { payload } = action; state.available = state.available .filter((account) => account.id !== payload.id) @@ -39,44 +61,70 @@ export default function accounts( }); return state; + } - case ACTIVATE: - if (!payload || !payload.id || !payload.token) { + case 'accounts:activate': { + if (!action.payload || !action.payload.id || !action.payload.token) { throw new Error('Invalid or empty payload passed for accounts.add'); } + const { payload } = action; + return { - ...state, - active: payload + available: state.available.map((account) => { + if (account.id === payload.id) { + return {...payload}; + } + + return {...account}; + }), + active: payload.id + }; + } + + case 'accounts:reset': + return { + active: null, + available: [] }; - case RESET: - return accounts(undefined, {}); - - case REMOVE: - if (!payload || !payload.id) { + case 'accounts:remove': { + if (!action.payload || !action.payload.id) { throw new Error('Invalid or empty payload passed for accounts.remove'); } + const { payload } = action; + return { ...state, available: state.available.filter((account) => account.id !== payload.id) }; + } - case UPDATE_TOKEN: - if (typeof payload !== 'string') { + case 'accounts:updateToken': { + if (typeof action.payload !== 'string') { throw new Error('payload must be a jwt token'); } + const { payload } = action; + return { ...state, - active: { - ...state.active, - token: payload - } + available: state.available.map((account) => { + if (account.id === state.active) { + return { + ...account, + token: payload, + }; + } + + return {...account}; + }), }; + } default: + (action: empty); return state; } } diff --git a/src/components/accounts/reducer.test.js b/src/components/accounts/reducer.test.js index ac8e7cf..989f8cf 100644 --- a/src/components/accounts/reducer.test.js +++ b/src/components/accounts/reducer.test.js @@ -35,7 +35,7 @@ describe('Accounts reducer', () => { describe(ACTIVATE, () => { it('sets active account', () => { expect(accounts(initial, activate(account)), 'to satisfy', { - active: account + active: account.id }); }); }); @@ -108,14 +108,14 @@ describe('Accounts reducer', () => { const newToken = 'newToken'; expect(accounts( - {active: account, available: [account]}, + {active: account.id, available: [account]}, updateToken(newToken) ), 'to satisfy', { - active: { + active: account.id, + available: [{ ...account, token: newToken - }, - available: [account] + }] }); }); }); diff --git a/src/components/auth/actions.js b/src/components/auth/actions.js index 2fdf594..51b22c8 100644 --- a/src/components/auth/actions.js +++ b/src/components/auth/actions.js @@ -20,11 +20,16 @@ export { authenticate, logoutAll as logout } from 'components/accounts/actions'; /** * Reoutes user to the previous page if it is possible * - * @param {string} fallbackUrl - an url to route user to if goBack is not possible + * @param {object} options + * @param {string} options.fallbackUrl - an url to route user to if goBack is not possible * * @return {object} - action definition */ -export function goBack(fallbackUrl?: ?string = null) { +export function goBack(options: { + fallbackUrl?: string +}) { + const { fallbackUrl } = options || {}; + if (history.canGoBack()) { browserHistory.goBack(); } else if (fallbackUrl) { diff --git a/src/components/user/factory.js b/src/components/user/factory.js index b6baf10..46bab4d 100644 --- a/src/components/user/factory.js +++ b/src/components/user/factory.js @@ -1,6 +1,6 @@ import { changeLang } from 'components/user/actions'; import { authenticate, logoutStrangers } from 'components/accounts/actions'; - +import { getActiveAccount } from 'components/accounts/reducer'; import request from 'services/request'; import bearerHeaderMiddleware from './middlewares/bearerHeaderMiddleware'; import refreshTokenMiddleware from './middlewares/refreshTokenMiddleware'; @@ -25,11 +25,11 @@ export function factory(store) { promise = Promise.resolve() .then(() => store.dispatch(logoutStrangers())) .then(() => { - const {accounts} = store.getState(); + const activeAccount = getActiveAccount(store.getState()); - if (accounts.active) { + if (activeAccount) { // authorizing user if it is possible - return store.dispatch(authenticate(accounts.active)); + return store.dispatch(authenticate(activeAccount)); } return Promise.reject(); diff --git a/src/components/user/middlewares/bearerHeaderMiddleware.js b/src/components/user/middlewares/bearerHeaderMiddleware.js index 692eee1..ed546ec 100644 --- a/src/components/user/middlewares/bearerHeaderMiddleware.js +++ b/src/components/user/middlewares/bearerHeaderMiddleware.js @@ -1,3 +1,6 @@ +// @flow +import { getActiveAccount } from 'components/accounts/reducer'; + /** * Applies Bearer header for all requests * @@ -9,12 +12,17 @@ * * @return {object} - request middleware */ -export default function bearerHeaderMiddleware({getState}) { +export default function bearerHeaderMiddleware(store: { getState: () => Object }) { return { - before(req) { - const {accounts} = getState(); + before(req: T): T { + const activeAccount = getActiveAccount(store.getState()); - let {token} = accounts.active || {}; + let {token} = activeAccount || {}; if (req.options.token || req.options.token === null) { token = req.options.token; diff --git a/src/components/user/middlewares/bearerHeaderMiddleware.test.js b/src/components/user/middlewares/bearerHeaderMiddleware.test.js index c432759..a75c901 100644 --- a/src/components/user/middlewares/bearerHeaderMiddleware.test.js +++ b/src/components/user/middlewares/bearerHeaderMiddleware.test.js @@ -6,8 +6,9 @@ describe('bearerHeaderMiddleware', () => { const emptyState = { user: {}, accounts: { - active: null - } + active: null, + available: [], + }, }; describe('when token available', () => { @@ -16,7 +17,11 @@ describe('bearerHeaderMiddleware', () => { getState: () => ({ ...emptyState, accounts: { - active: {token} + active: 1, + available: [{ + id: 1, + token, + }], } }) }); diff --git a/src/components/user/middlewares/refreshTokenMiddleware.js b/src/components/user/middlewares/refreshTokenMiddleware.js index b630b73..237bf34 100644 --- a/src/components/user/middlewares/refreshTokenMiddleware.js +++ b/src/components/user/middlewares/refreshTokenMiddleware.js @@ -1,8 +1,6 @@ -import { getJwtPayload } from 'functions'; -import authentication from 'services/api/authentication'; -import logger from 'services/logger'; -import { InternalServerError } from 'services/request'; -import { updateToken, logoutAll } from 'components/accounts/actions'; +// @flow +import { ensureToken, recoverFromTokenError } from 'components/accounts/actions'; +import { getActiveAccount } from 'components/accounts/reducer'; /** * Ensures, that all user's requests have fresh access token @@ -13,75 +11,29 @@ import { updateToken, logoutAll } from 'components/accounts/actions'; * * @return {object} - request middleware */ -export default function refreshTokenMiddleware({dispatch, getState}) { +export default function refreshTokenMiddleware({dispatch, getState}: {dispatch: (Object) => *, getState: () => Object}) { return { - before(req) { - const {accounts} = getState(); - - let refreshToken; - let token; + before(req: T): Promise { + const activeAccount = getActiveAccount(getState()); + const disableMiddleware = !!req.options.token || req.options.token === null; const isRefreshTokenRequest = req.url.includes('refresh-token'); - if (accounts.active) { - token = accounts.active.token; - refreshToken = accounts.active.refreshToken; - } - - if (!token || req.options.token || isRefreshTokenRequest) { + if (!activeAccount || disableMiddleware || isRefreshTokenRequest) { return Promise.resolve(req); } - try { - const SAFETY_FACTOR = 300; // ask new token earlier to overcome time dissynchronization problem - const jwt = getJwtPayload(token); - - if (jwt.exp - SAFETY_FACTOR < Date.now() / 1000) { - return requestAccessToken(refreshToken, dispatch).then(() => req); - } - } catch (err) { - logger.warn('Refresh token error: bad token', { - token - }); - - return dispatch(logoutAll()).then(() => req); - } - - return Promise.resolve(req); + return dispatch(ensureToken()).then(() => req); }, - catch(resp, req, restart) { - if (resp && resp.status === 401 && !req.options.token) { - const {accounts} = getState(); - const {refreshToken} = accounts.active || {}; + catch(resp: {status: number, message: string}, req: {options: { token?: string}}, restart: () => Promise): Promise<*> { + const disableMiddleware = !!req.options.token || req.options.token === null; - if (resp.message === 'Token expired' && refreshToken) { - // request token and retry - return requestAccessToken(refreshToken, dispatch).then(restart); - } - - return dispatch(logoutAll()).then(() => Promise.reject(resp)); + if (disableMiddleware) { + return Promise.reject(resp); } - return Promise.reject(resp); + return dispatch(recoverFromTokenError(resp)).then(restart); } }; } - -function requestAccessToken(refreshToken, dispatch) { - if (refreshToken) { - return authentication.requestToken(refreshToken) - .then(({token}) => dispatch(updateToken(token))) - .catch((resp = {}) => { - if (resp instanceof InternalServerError) { - return Promise.reject(resp); - } - - return dispatch(logoutAll()); - }); - } - - return dispatch(logoutAll()); -} - - diff --git a/src/components/user/middlewares/refreshTokenMiddleware.test.js b/src/components/user/middlewares/refreshTokenMiddleware.test.js index 980d28d..aff16ff 100644 --- a/src/components/user/middlewares/refreshTokenMiddleware.test.js +++ b/src/components/user/middlewares/refreshTokenMiddleware.test.js @@ -2,7 +2,7 @@ import expect from 'unexpected'; import sinon from 'sinon'; import refreshTokenMiddleware from 'components/user/middlewares/refreshTokenMiddleware'; - +import { browserHistory } from 'services/history'; import authentication from 'services/api/authentication'; import { InternalServerError } from 'services/request'; import { updateToken } from 'components/accounts/actions'; @@ -17,9 +17,12 @@ describe('refreshTokenMiddleware', () => { let getState; let dispatch; + const email = 'test@email.com'; + beforeEach(() => { sinon.stub(authentication, 'requestToken').named('authentication.requestToken'); sinon.stub(authentication, 'logout').named('authentication.logout'); + sinon.stub(browserHistory, 'push'); getState = sinon.stub().named('store.getState'); dispatch = sinon.spy((arg) => @@ -32,8 +35,22 @@ describe('refreshTokenMiddleware', () => { afterEach(() => { authentication.requestToken.restore(); authentication.logout.restore(); + browserHistory.push.restore(); }); + function assertRelogin() { + expect(dispatch, 'to have a call satisfying', [ + { + type: 'auth:setCredentials', + payload: {login: email} + } + ]); + + expect(browserHistory.push, 'to have a call satisfying', [ + '/login' + ]); + } + it('must be till 2100 to test with validToken', () => expect(new Date().getFullYear(), 'to be less than', 2100) ); @@ -42,6 +59,7 @@ describe('refreshTokenMiddleware', () => { describe('when token expired', () => { beforeEach(() => { const account = { + email, token: expiredToken, refreshToken }; @@ -111,8 +129,9 @@ describe('refreshTokenMiddleware', () => { ); }); - it('should logout if token can not be parsed', () => { + it('should relogin if token can not be parsed', () => { const account = { + email, token: 'realy bad token', refreshToken }; @@ -126,23 +145,23 @@ describe('refreshTokenMiddleware', () => { const req = {url: 'foo', options: {}}; - return expect(middleware.before(req), 'to be fulfilled with', req).then(() => { - expect(authentication.requestToken, 'was not called'); + return expect(middleware.before(req), 'to be rejected with', { + message: 'Invalid token' + }) + .then(() => { + expect(authentication.requestToken, 'was not called'); - expect(dispatch, 'to have a call satisfying', [ - {payload: {isGuest: true}} - ]); - }); + assertRelogin(); + }); }); - it('should logout if token request failed', () => { + it('should relogin if token request failed', () => { authentication.requestToken.returns(Promise.reject()); - return expect(middleware.before({url: 'foo', options: {}}), 'to be fulfilled').then(() => - expect(dispatch, 'to have a call satisfying', [ - {payload: {isGuest: true}} - ]) - ); + return expect(middleware.before({url: 'foo', options: {}}), 'to be rejected') + .then(() => + assertRelogin() + ); }); it('should not logout if request failed with 5xx', () => { @@ -161,12 +180,16 @@ describe('refreshTokenMiddleware', () => { it('should not be applied if no token', () => { getState.returns({ accounts: { - active: null + active: null, + available: [], }, user: {} }); - const data = {url: 'foo'}; + const data = { + url: 'foo', + options: {}, + }; const resp = middleware.before(data); return expect(resp, 'to be fulfilled with', data) @@ -206,8 +229,13 @@ describe('refreshTokenMiddleware', () => { beforeEach(() => { getState.returns({ accounts: { - active: {refreshToken}, - available: [{refreshToken}] + active: 1, + available: [{ + id: 1, + email, + token: 'old token', + refreshToken, + }] }, user: {} }); @@ -217,37 +245,56 @@ describe('refreshTokenMiddleware', () => { authentication.requestToken.returns(Promise.resolve({token: validToken})); }); + function assertNewTokenRequest() { + expect(authentication.requestToken, 'to have a call satisfying', [ + refreshToken + ]); + expect(restart, 'was called'); + expect(dispatch, 'was called'); + } + it('should request new token if expired', () => - middleware.catch(expiredResponse, {options: {}}, restart).then(() => { - expect(authentication.requestToken, 'to have a call satisfying', [ - refreshToken - ]); - expect(restart, 'was called'); - }) + expect( + middleware.catch(expiredResponse, {options: {}}, restart), + 'to be fulfilled' + ).then(assertNewTokenRequest) ); - it('should logout user if invalid credential', () => + it('should request new token if invalid credential', () => expect( middleware.catch(badTokenReponse, {options: {}}, restart), - 'to be rejected' - ).then(() => - expect(dispatch, 'to have a call satisfying', [ - {payload: {isGuest: true}} - ]) - ) + 'to be fulfilled' + ).then(assertNewTokenRequest) ); - it('should logout user if token is incorrect', () => + it('should request new token if token is incorrect', () => expect( middleware.catch(incorrectTokenReponse, {options: {}}, restart), - 'to be rejected' - ).then(() => - expect(dispatch, 'to have a call satisfying', [ - {payload: {isGuest: true}} - ]) - ) + 'to be fulfilled' + ).then(assertNewTokenRequest) ); + it('should relogin if no refreshToken', () => { + getState.returns({ + accounts: { + active: 1, + available: [{ + id: 1, + email, + refreshToken: null, + }] + }, + user: {} + }); + + return expect( + middleware.catch(incorrectTokenReponse, {options: {}}, restart), + 'to be rejected' + ).then(() => { + assertRelogin(); + }); + }); + it('should pass the request through if options.token specified', () => { const promise = middleware.catch(expiredResponse, { options: { diff --git a/src/containers/PrivateRoute.js b/src/containers/PrivateRoute.js index 8448194..7e48b3f 100644 --- a/src/containers/PrivateRoute.js +++ b/src/containers/PrivateRoute.js @@ -1,17 +1,17 @@ // @flow import React from 'react'; - import { Route, Redirect } from 'react-router-dom'; import { connect } from 'react-redux'; +import { getActiveAccount } from 'components/accounts/reducer'; +import type { ComponentType } from 'react'; +import type { Account } from 'components/accounts'; -import type {User} from 'components/user'; - -const PrivateRoute = ({user, component: Component, ...rest}: { - component: any, - user: User +const PrivateRoute = ({account, component: Component, ...rest}: { + component: ComponentType<*>, + account: ?Account }) => ( ( - user.isGuest ? ( + !account || !account.token ? ( ) : ( @@ -20,5 +20,5 @@ const PrivateRoute = ({user, component: Component, ...rest}: { ); export default connect((state) => ({ - user: state.user + account: getActiveAccount(state) }))(PrivateRoute); diff --git a/src/services/api/authentication.js b/src/services/api/authentication.js index b81806c..86d13b9 100644 --- a/src/services/api/authentication.js +++ b/src/services/api/authentication.js @@ -29,11 +29,11 @@ const authentication = { * * @return {Promise} */ - logout(options: { - token?: string - } = {}) { + logout(options: ?{ + token: string + }) { return request.post('/api/authentication/logout', {}, { - token: options.token + token: options && options.token }); }, @@ -80,7 +80,7 @@ const authentication = { */ validateToken({token, refreshToken}: { token: string, - refreshToken: string + refreshToken: ?string }) { return new Promise((resolve) => { if (typeof token !== 'string') { @@ -91,36 +91,39 @@ const authentication = { }) .then(() => accounts.current({token})) .then((user) => ({token, refreshToken, user})) - .catch((resp) => { - if (resp instanceof InternalServerError) { - // delegate error recovering to the bsod middleware - return new Promise(() => {}); - } + .catch((resp) => + this.handleTokenError(resp, refreshToken) + // TODO: use recursion here + .then(({token}) => + accounts.current({token}) + .then((user) => ({token, refreshToken, user})) + ) + ); + }, - 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 - }); + handleTokenError(resp: Error | { message: string }, refreshToken: ?string): Promise<{ + token: string, + }> { + if (resp instanceof InternalServerError) { + // delegate error recovering to the bsod middleware + return new Promise(() => {}); + } - return Promise.reject(error); - }); - } + if (refreshToken) { + if ([ + 'Token expired', + 'Incorrect token', + 'You are requesting with an invalid credential.' + ].includes(resp.message)) { + return authentication.requestToken(refreshToken); + } - 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); + logger.error('Unexpected error during token validation', { + resp }); + } + + return Promise.reject(resp); }, /** @@ -135,9 +138,21 @@ const authentication = { '/api/authentication/refresh-token', {refresh_token: refreshToken}, // eslint-disable-line {token: null} - ).then((resp: {access_token: string}) => ({ - token: resp.access_token - })); + ) + .then((resp: {access_token: string}) => ({ + token: resp.access_token + })) + .catch((resp) => { + const errors = resp.errors || {}; + + if (errors.refresh_token !== 'error.refresh_token_not_exist') { + logger.error('Failed refreshing token: unknown error', { + resp + }); + } + + return Promise.reject(resp); + }); } }; diff --git a/src/services/authFlow/AuthFlow.js b/src/services/authFlow/AuthFlow.js index b00cd14..5dba009 100644 --- a/src/services/authFlow/AuthFlow.js +++ b/src/services/authFlow/AuthFlow.js @@ -51,7 +51,7 @@ type ActionId = | 'setLoadingState'; export interface AuthContext { - run(actionId: ActionId, payload?: ?Object): *; + run(actionId: ActionId, payload?: mixed): *; setState(newState: AbstractState): Promise<*> | void; getState(): Object; navigate(route: string): void; @@ -59,7 +59,7 @@ export interface AuthContext { } export default class AuthFlow implements AuthContext { - actions: {[key: string]: Function}; + actions: {[key: string]: (mixed) => Object}; state: AbstractState; prevState: AbstractState; /** @@ -125,12 +125,18 @@ export default class AuthFlow implements AuthContext { this.state.goBack(this); } - run(actionId: ActionId, payload?: ?Object): Promise<*> { - if (!this.actions[actionId]) { + run(actionId: ActionId, payload?: mixed): Promise { + const action = this.actions[actionId]; + + if (!action) { throw new Error(`Action ${actionId} does not exists`); } - return Promise.resolve(this.dispatch(this.actions[actionId](payload))); + return Promise.resolve( + this.dispatch( + action(payload) + ) + ); } setState(state: AbstractState) { diff --git a/src/services/authFlow/CompleteState.js b/src/services/authFlow/CompleteState.js index 82d5f93..4f246cc 100644 --- a/src/services/authFlow/CompleteState.js +++ b/src/services/authFlow/CompleteState.js @@ -1,3 +1,5 @@ +// @flow +import { getActiveAccount } from 'components/accounts/reducer'; import AbstractState from './AbstractState'; import LoginState from './LoginState'; import PermissionsState from './PermissionsState'; @@ -5,18 +7,23 @@ import ChooseAccountState from './ChooseAccountState'; import ActivationState from './ActivationState'; import AcceptRulesState from './AcceptRulesState'; import FinishState from './FinishState'; +import type { AuthContext } from './AuthFlow'; const PROMPT_ACCOUNT_CHOOSE = 'select_account'; const PROMPT_PERMISSIONS = 'consent'; export default class CompleteState extends AbstractState { - constructor(options = {}) { - super(options); + isPermissionsAccepted: bool | void; + + constructor(options: { + accept?: bool, + } = {}) { + super(); this.isPermissionsAccepted = options.accept; } - enter(context) { + enter(context: AuthContext) { const {auth = {}, user} = context.getState(); if (user.isGuest) { @@ -32,7 +39,7 @@ export default class CompleteState extends AbstractState { } } - processOAuth(context) { + processOAuth(context: AuthContext) { const {auth = {}, accounts} = context.getState(); let isSwitcherEnabled = auth.isSwitcherEnabled; @@ -44,13 +51,14 @@ export default class CompleteState extends AbstractState { || account.email === loginHint || account.username === loginHint )[0]; + const activeAccount = getActiveAccount(context.getState()); if (account) { // disable switching, because we are know the account, user must be authorized with context.run('setAccountSwitcher', false); isSwitcherEnabled = false; - if (account.id !== accounts.active.id) { + if (!activeAccount || account.id !== activeAccount.id) { // lets switch user to an account, that is needed for auth return context.run('authenticate', account) .then(() => context.setState(new CompleteState())); @@ -75,8 +83,10 @@ export default class CompleteState extends AbstractState { return; } - // TODO: it seams that oAuthComplete may be a separate state - return context.run('oAuthComplete', data).then((resp) => { + // TODO: it seems that oAuthComplete may be a separate state + return context.run('oAuthComplete', data).then((resp: { + redirectUri: string, + }) => { // TODO: пусть в стейт попадает флаг или тип авторизации // вместо волшебства над редирект урлой if (resp.redirectUri.indexOf('static_page') === 0) { diff --git a/src/services/authFlow/CompleteState.test.js b/src/services/authFlow/CompleteState.test.js index 0d1bf4a..5301d9f 100644 --- a/src/services/authFlow/CompleteState.test.js +++ b/src/services/authFlow/CompleteState.test.js @@ -166,9 +166,7 @@ describe('CompleteState', () => { {id: 1}, {id: 2} ], - active: { - id: 1 - } + active: 1 }, auth: { isSwitcherEnabled: true, @@ -195,9 +193,7 @@ describe('CompleteState', () => { {id: 1}, {id: 2} ], - active: { - id: 1 - } + active: 1 }, auth: { isSwitcherEnabled: false, @@ -224,9 +220,7 @@ describe('CompleteState', () => { available: [ {id: 1} ], - active: { - id: 1 - } + active: 1 }, auth: { isSwitcherEnabled: true, @@ -252,9 +246,7 @@ describe('CompleteState', () => { available: [ {id: 1} ], - active: { - id: 1 - } + active: 1 }, auth: { isSwitcherEnabled: false, @@ -416,9 +408,7 @@ describe('CompleteState', () => { available: [ account ], - active: { - id: 100 - } + active: 100 }, auth: { oauth: { @@ -465,7 +455,7 @@ describe('CompleteState', () => { available: [ account ], - active: account + active: account.id, }, auth: { oauth: { @@ -497,7 +487,7 @@ describe('CompleteState', () => { }, accounts: { available: [{id: 1}], - active: {id: 1} + active: 1 }, auth: { oauth: { diff --git a/src/services/authFlow/LoginState.js b/src/services/authFlow/LoginState.js index 98e5239..7e82da9 100644 --- a/src/services/authFlow/LoginState.js +++ b/src/services/authFlow/LoginState.js @@ -1,3 +1,4 @@ +// @flow import logger from 'services/logger'; import { getLogin } from 'components/auth/reducer'; @@ -5,8 +6,10 @@ import AbstractState from './AbstractState'; import PasswordState from './PasswordState'; import RegisterState from './RegisterState'; +import type { AuthContext } from './AuthFlow'; + export default class LoginState extends AbstractState { - enter(context) { + enter(context: AuthContext) { const login = getLogin(context.getState()); const {user} = context.getState(); @@ -24,7 +27,9 @@ export default class LoginState extends AbstractState { } } - resolve(context, payload) { + resolve(context: AuthContext, payload: { + login: string + }) { context.run('login', payload) .then(() => context.setState(new PasswordState())) .catch((err = {}) => @@ -32,11 +37,13 @@ export default class LoginState extends AbstractState { ); } - reject(context) { + reject(context: AuthContext) { context.setState(new RegisterState()); } - goBack(context) { - context.run('goBack', '/'); + goBack(context: AuthContext) { + context.run('goBack', { + fallbackUrl: '/' + }); } } diff --git a/src/services/authFlow/LoginState.test.js b/src/services/authFlow/LoginState.test.js index c57fa2b..ecd264d 100644 --- a/src/services/authFlow/LoginState.test.js +++ b/src/services/authFlow/LoginState.test.js @@ -97,7 +97,9 @@ describe('LoginState', () => { describe('#goBack', () => { it('should return to previous page', () => { - expectRun(mock, 'goBack', '/'); + expectRun(mock, 'goBack', { + fallbackUrl: '/' + }); state.goBack(context); });