From 274c28a9235b010b453964964710143030a91756 Mon Sep 17 00:00:00 2001 From: SleepWalker Date: Wed, 4 Jan 2017 07:52:46 +0200 Subject: [PATCH 1/5] #246: remove logout from user/actions. Use logoutAll from account/actions everywhere --- src/components/accounts/actions.js | 16 ++- src/components/auth/actions.js | 17 +-- src/components/user/actions.js | 39 ++---- .../middlewares/refreshTokenMiddleware.js | 15 +- tests/components/accounts/actions.test.js | 23 ++- tests/components/user/actions.test.js | 132 ------------------ 6 files changed, 67 insertions(+), 175 deletions(-) diff --git a/src/components/accounts/actions.js b/src/components/accounts/actions.js index 8d955a3..a3bb5ab 100644 --- a/src/components/accounts/actions.js +++ b/src/components/accounts/actions.js @@ -1,6 +1,8 @@ +import { routeActions } from 'react-router-redux'; + import authentication from 'services/api/authentication'; import accounts from 'services/api/accounts'; -import { updateUser, logout } from 'components/user/actions'; +import { updateUser, setGuest } from 'components/user/actions'; import { setLocale } from 'components/i18n/actions'; import logger from 'services/logger'; @@ -25,7 +27,7 @@ export function authenticate({token, refreshToken}) { authentication.validateToken({token, refreshToken}) .catch(() => { // TODO: log this case - dispatch(logout()); + dispatch(logoutAll()); return Promise.reject(); }) @@ -80,17 +82,23 @@ export function revoke(account) { }); } - return dispatch(logout()); + return dispatch(logoutAll()); }; } export function logoutAll() { return (dispatch, getState) => { + dispatch(setGuest()); + const {accounts: {available}} = getState(); available.forEach((account) => authentication.logout(account)); dispatch(reset()); + + dispatch(routeActions.push('/login')); + + return Promise.resolve(); }; } @@ -120,7 +128,7 @@ export function logoutStrangers() { return dispatch(authenticate(accountToReplace)); } - dispatch(logout()); + dispatch(logoutAll()); return Promise.resolve(); }; diff --git a/src/components/auth/actions.js b/src/components/auth/actions.js index fb91af0..e5a47af 100644 --- a/src/components/auth/actions.js +++ b/src/components/auth/actions.js @@ -1,12 +1,16 @@ import { routeActions } from 'react-router-redux'; -import { updateUser, logout, acceptRules as userAcceptRules } from 'components/user/actions'; -import { authenticate } from 'components/accounts/actions'; +import logger from 'services/logger'; +import { updateUser, acceptRules as userAcceptRules } from 'components/user/actions'; +import { authenticate, logoutAll } from 'components/accounts/actions'; import authentication from 'services/api/authentication'; import oauth from 'services/api/oauth'; import signup from 'services/api/signup'; import dispatchBsod from 'components/ui/bsod/dispatchBsod'; +export { updateUser } from 'components/user/actions'; +export { authenticate, logoutAll as logout } from 'components/accounts/actions'; + export function login({login = '', password = '', rememberMe = false}) { const PASSWORD_REQUIRED = 'error.password_required'; const LOGIN_REQUIRED = 'error.login_required'; @@ -24,9 +28,9 @@ export function login({login = '', password = '', rememberMe = false}) { } else if (resp.errors.login === ACTIVATION_REQUIRED) { return dispatch(needActivation()); } else if (resp.errors.login === LOGIN_REQUIRED && password) { - // TODO: log this case to backend - // return to the first step - return dispatch(logout()); + logger.warn('No login on password panel'); + + return dispatch(logoutAll()); } } @@ -144,9 +148,6 @@ export function clearErrors() { return setErrors(null); } -export { logout, updateUser } from 'components/user/actions'; -export { authenticate } from 'components/accounts/actions'; - /** * @param {object} oauthData * @param {string} oauthData.clientId diff --git a/src/components/user/actions.js b/src/components/user/actions.js index 0bdcd36..2342dd5 100644 --- a/src/components/user/actions.js +++ b/src/components/user/actions.js @@ -1,7 +1,4 @@ -import { routeActions } from 'react-router-redux'; - import accounts from 'services/api/accounts'; -import { logoutAll } from 'components/accounts/actions'; import { setLocale } from 'components/i18n/actions'; export const UPDATE = 'USER_UPDATE'; @@ -18,6 +15,20 @@ export function updateUser(payload) { }; } +export const SET = 'USER_SET'; +/** + * Replace current user's state with a new one + * + * @param {User} payload + * @return {object} - action definition + */ +export function setUser(payload) { + return { + type: SET, + payload + }; +} + export const CHANGE_LANG = 'USER_CHANGE_LANG'; export function changeLang(lang) { return (dispatch, getState) => dispatch(setLocale(lang)) @@ -37,32 +48,12 @@ export function changeLang(lang) { }); } -export const SET = 'USER_SET'; -/** - * Replace current user's state with a new one - * - * @param {User} payload - * @return {object} - action definition - */ -export function setUser(payload) { - return { - type: SET, - payload - }; -} - -export function logout() { +export function setGuest() { return (dispatch, getState) => { dispatch(setUser({ lang: getState().user.lang, isGuest: true })); - - dispatch(logoutAll()); - - dispatch(routeActions.push('/login')); - - return Promise.resolve(); }; } diff --git a/src/components/user/middlewares/refreshTokenMiddleware.js b/src/components/user/middlewares/refreshTokenMiddleware.js index 491f88a..b48cb04 100644 --- a/src/components/user/middlewares/refreshTokenMiddleware.js +++ b/src/components/user/middlewares/refreshTokenMiddleware.js @@ -1,6 +1,6 @@ import authentication from 'services/api/authentication'; -import { updateToken } from 'components/accounts/actions'; -import { logout } from '../actions'; +import logger from 'services/logger'; +import { updateToken, logoutAll } from 'components/accounts/actions'; /** * Ensures, that all user's requests have fresh access token @@ -41,8 +41,11 @@ export default function refreshTokenMiddleware({dispatch, getState}) { return requestAccessToken(refreshToken, dispatch).then(() => req); } } catch (err) { - // console.error('Bad token', err); // TODO: it would be cool to log such things to backend - return dispatch(logout()).then(() => req); + logger.warn('Refresh token error: bad token', { + token + }); + + return dispatch(logoutAll()).then(() => req); } return Promise.resolve(req); @@ -58,7 +61,7 @@ export default function refreshTokenMiddleware({dispatch, getState}) { return requestAccessToken(refreshToken, dispatch).then(restart); } - return dispatch(logout()).then(() => Promise.reject(resp)); + return dispatch(logoutAll()).then(() => Promise.reject(resp)); } return Promise.reject(resp); @@ -76,7 +79,7 @@ function requestAccessToken(refreshToken, dispatch) { return promise .then(({token}) => dispatch(updateToken(token))) - .catch(() => dispatch(logout())); + .catch(() => dispatch(logoutAll())); } diff --git a/tests/components/accounts/actions.test.js b/tests/components/accounts/actions.test.js index 4544a32..2413ab8 100644 --- a/tests/components/accounts/actions.test.js +++ b/tests/components/accounts/actions.test.js @@ -1,6 +1,8 @@ import expect from 'unexpected'; import sinon from 'sinon'; +import { routeActions } from 'react-router-redux'; + import accounts from 'services/api/accounts'; import authentication from 'services/api/authentication'; import { @@ -15,7 +17,7 @@ import { } from 'components/accounts/actions'; import { SET_LOCALE } from 'components/i18n/actions'; -import { updateUser } from 'components/user/actions'; +import { updateUser, setUser } from 'components/user/actions'; const account = { id: 1, @@ -257,6 +259,25 @@ describe('components/accounts/actions', () => { reset() ]); }); + + it('should redirect to /login', () => + logoutAll()(dispatch, getState).then(() => { + expect(dispatch, 'to have a call satisfying', [ + routeActions.push('/login') + ]); + }) + ); + + it('should change user to guest', () => + logoutAll()(dispatch, getState).then(() => { + expect(dispatch, 'to have a call satisfying', [ + setUser({ + lang: user.lang, + isGuest: true + }) + ]); + }) + ); }); describe('#logoutStrangers', () => { diff --git a/tests/components/user/actions.test.js b/tests/components/user/actions.test.js index 479acdd..94c5805 100644 --- a/tests/components/user/actions.test.js +++ b/tests/components/user/actions.test.js @@ -1,134 +1,2 @@ -import expect from 'unexpected'; -import sinon from 'sinon'; - -import { routeActions } from 'react-router-redux'; - -import request from 'services/request'; -import { reset, RESET } from 'components/accounts/actions'; - -import { - logout, - setUser -} from 'components/user/actions'; - - describe('components/user/actions', () => { - const getState = sinon.stub().named('store.getState'); - const dispatch = sinon.spy((arg) => - typeof arg === 'function' ? arg(dispatch, getState) : arg - ).named('store.dispatch'); - - const callThunk = function(fn, ...args) { - const thunk = fn(...args); - - return thunk(dispatch, getState); - }; - - beforeEach(() => { - dispatch.reset(); - getState.reset(); - getState.returns({}); - sinon.stub(request, 'get').named('request.get'); - sinon.stub(request, 'post').named('request.post'); - }); - - afterEach(() => { - request.get.restore(); - request.post.restore(); - }); - - describe('#logout()', () => { - beforeEach(() => { - request.post.returns(Promise.resolve()); - }); - - describe('user with jwt', () => { - const token = 'iLoveRockNRoll'; - - beforeEach(() => { - getState.returns({ - user: { - lang: 'foo' - }, - accounts: { - active: {token}, - available: [{token}] - } - }); - }); - - it('should post to /api/authentication/logout with user jwt', () => - callThunk(logout).then(() => { - expect(request.post, 'to have a call satisfying', [ - '/api/authentication/logout', {}, { - token: expect.it('not to be empty') - } - ]); - }) - ); - - testChangedToGuest(); - testAccountsReset(); - testRedirectedToLogin(); - }); - - describe('user without jwt', () => { - // (a guest with partially filled user's state) - // DEPRECATED - beforeEach(() => { - getState.returns({ - user: { - lang: 'foo' - }, - accounts: { - active: null, - available: [] - } - }); - }); - - it('should not post to /api/authentication/logout', () => - callThunk(logout).then(() => { - expect(request.post, 'was not called'); - }) - ); - - testChangedToGuest(); - testAccountsReset(); - testRedirectedToLogin(); - }); - - function testChangedToGuest() { - it('should change user to guest', () => - callThunk(logout).then(() => { - expect(dispatch, 'to have a call satisfying', [ - setUser({ - lang: 'foo', - isGuest: true - }) - ]); - }) - ); - } - - function testRedirectedToLogin() { - it('should redirect to /login', () => - callThunk(logout).then(() => { - expect(dispatch, 'to have a call satisfying', [ - routeActions.push('/login') - ]); - }) - ); - } - - function testAccountsReset() { - it(`should dispatch ${RESET}`, () => - callThunk(logout).then(() => { - expect(dispatch, 'to have a call satisfying', [ - reset() - ]); - }) - ); - } - }); }); From 87185b6e9b159268633dd75d3e4cfeca7255aab4 Mon Sep 17 00:00:00 2001 From: SleepWalker Date: Fri, 6 Jan 2017 07:34:39 +0200 Subject: [PATCH 2/5] #246: remove outdated code from refreshTokenMiddleware --- .../middlewares/refreshTokenMiddleware.js | 27 ++------ src/functions.js | 21 ++++++ .../refreshTokenMiddleware.test.js | 68 +++---------------- 3 files changed, 36 insertions(+), 80 deletions(-) diff --git a/src/components/user/middlewares/refreshTokenMiddleware.js b/src/components/user/middlewares/refreshTokenMiddleware.js index b48cb04..426b8cb 100644 --- a/src/components/user/middlewares/refreshTokenMiddleware.js +++ b/src/components/user/middlewares/refreshTokenMiddleware.js @@ -1,3 +1,4 @@ +import { getJwtPayload } from 'functions'; import authentication from 'services/api/authentication'; import logger from 'services/logger'; import { updateToken, logoutAll } from 'components/accounts/actions'; @@ -14,7 +15,7 @@ import { updateToken, logoutAll } from 'components/accounts/actions'; export default function refreshTokenMiddleware({dispatch, getState}) { return { before(req) { - const {user, accounts} = getState(); + const {accounts} = getState(); let refreshToken; let token; @@ -24,18 +25,15 @@ export default function refreshTokenMiddleware({dispatch, getState}) { if (accounts.active) { token = accounts.active.token; refreshToken = accounts.active.refreshToken; - } else { // #legacy token - token = user.token; - refreshToken = user.refreshToken; } if (!token || req.options.token || isRefreshTokenRequest) { - return req; + return Promise.resolve(req); } try { const SAFETY_FACTOR = 300; // ask new token earlier to overcome time dissynchronization problem - const jwt = getJWTPayload(token); + const jwt = getJwtPayload(token); if (jwt.exp - SAFETY_FACTOR < Date.now() / 1000) { return requestAccessToken(refreshToken, dispatch).then(() => req); @@ -53,8 +51,8 @@ export default function refreshTokenMiddleware({dispatch, getState}) { catch(resp, req, restart) { if (resp && resp.status === 401 && !req.options.token) { - const {user, accounts} = getState(); - const {refreshToken} = accounts.active ? accounts.active : user; + const {accounts} = getState(); + const {refreshToken} = accounts.active || {}; if (resp.message === 'Token expired' && refreshToken) { // request token and retry @@ -83,16 +81,3 @@ function requestAccessToken(refreshToken, dispatch) { } -function getJWTPayload(jwt) { - const parts = (jwt || '').split('.'); - - if (parts.length !== 3) { - throw new Error('Invalid jwt token'); - } - - try { - return JSON.parse(atob(parts[1])); - } catch (err) { - throw new Error('Can not decode jwt token'); - } -} diff --git a/src/functions.js b/src/functions.js index f630d1f..5f8ac8b 100644 --- a/src/functions.js +++ b/src/functions.js @@ -61,3 +61,24 @@ export const rAF = window.requestAnimationFrame * @param {bool} [immediate=false] - whether to execute at the beginning */ export debounce from 'debounce'; + +/** + * @param {string} jwt + * + * @throws {Error} If can not decode token + * + * @return {object} - decoded jwt payload + */ +export function getJwtPayload(jwt) { + const parts = (jwt || '').split('.'); + + if (parts.length !== 3) { + throw new Error('Invalid jwt token'); + } + + try { + return JSON.parse(atob(parts[1])); + } catch (err) { + throw new Error('Can not decode jwt token'); + } +} diff --git a/tests/components/user/middlewares/refreshTokenMiddleware.test.js b/tests/components/user/middlewares/refreshTokenMiddleware.test.js index 9b9aa3d..bd241b1 100644 --- a/tests/components/user/middlewares/refreshTokenMiddleware.test.js +++ b/tests/components/user/middlewares/refreshTokenMiddleware.test.js @@ -1,4 +1,5 @@ import expect from 'unexpected'; +import sinon from 'sinon'; import refreshTokenMiddleware from 'components/user/middlewares/refreshTokenMiddleware'; @@ -75,9 +76,11 @@ describe('refreshTokenMiddleware', () => { const data = {url: '/refresh-token', options: {}}; const resp = middleware.before(data); - expect(resp, 'to satisfy', data); - expect(authentication.requestToken, 'was not called'); + return expect(resp, 'to be fulfilled with', data) + .then(() => + expect(authentication.requestToken, 'was not called') + ); }); it('should not auto refresh token if options.token specified', () => { @@ -142,40 +145,6 @@ describe('refreshTokenMiddleware', () => { }); }); - describe('when token expired legacy user', () => { - beforeEach(() => { - getState.returns({ - accounts: { - active: null, - available: [] - }, - user: { - token: expiredToken, - refreshToken - } - }); - }); - - it('should request new token', () => { - const data = { - url: 'foo', - options: { - headers: {} - } - }; - - authentication.requestToken.returns(Promise.resolve({token: validToken})); - - return middleware.before(data).then((resp) => { - expect(resp, 'to satisfy', data); - - expect(authentication.requestToken, 'to have a call satisfying', [ - refreshToken - ]); - }); - }); - }); - it('should not be applied if no token', () => { getState.returns({ accounts: { @@ -187,9 +156,10 @@ describe('refreshTokenMiddleware', () => { const data = {url: 'foo'}; const resp = middleware.before(data); - expect(resp, 'to satisfy', data); - - expect(authentication.requestToken, 'was not called'); + return expect(resp, 'to be fulfilled with', data) + .then(() => + expect(authentication.requestToken, 'was not called') + ); }); }); @@ -290,25 +260,5 @@ describe('refreshTokenMiddleware', () => { expect(authentication.requestToken, 'was not called'); }); }); - - describe('legacy user.refreshToken', () => { - beforeEach(() => { - getState.returns({ - accounts: { - active: null - }, - user: {refreshToken} - }); - }); - - 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'); - }) - ); - }); }); }); From f1110b0067e0e767236f6b457c3cd653778eeeec Mon Sep 17 00:00:00 2001 From: SleepWalker Date: Fri, 6 Jan 2017 08:04:14 +0200 Subject: [PATCH 3/5] #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} ) ); From 85942d27edc16355834488a8b90ab867e862f03a Mon Sep 17 00:00:00 2001 From: SleepWalker Date: Thu, 12 Jan 2017 07:29:39 +0200 Subject: [PATCH 4/5] #246: fixed bug, when active account reset to the first one after page refresh. removed authenticate call, when there are no strangers to logout on app startup. --- src/components/accounts/actions.js | 26 ++++++++------ tests/components/accounts/actions.test.js | 44 ++++++++++++++++++----- 2 files changed, 51 insertions(+), 19 deletions(-) diff --git a/src/components/accounts/actions.js b/src/components/accounts/actions.js index 04af8fc..a76eecb 100644 --- a/src/components/accounts/actions.js +++ b/src/components/accounts/actions.js @@ -111,24 +111,28 @@ export function logoutAll() { */ export function logoutStrangers() { return (dispatch, getState) => { - const {accounts: {available}} = getState(); + const {accounts: {available, active}} = getState(); const isStranger = ({refreshToken, id}) => !refreshToken && !sessionStorage.getItem(`stranger${id}`); - const accountToReplace = available.filter((account) => !isStranger(account))[0]; + if (available.some(isStranger)) { + const accountToReplace = available.filter((account) => !isStranger(account))[0]; - if (accountToReplace) { - available.filter(isStranger) - .forEach((account) => { - dispatch(remove(account)); - authentication.logout(account); - }); + if (accountToReplace) { + available.filter(isStranger) + .forEach((account) => { + dispatch(remove(account)); + authentication.logout(account); + }); - return dispatch(authenticate(accountToReplace)); + if (isStranger(active)) { + return dispatch(authenticate(accountToReplace)); + } + } else { + return dispatch(logoutAll()); + } } - dispatch(logoutAll()); - return Promise.resolve(); }; } diff --git a/tests/components/accounts/actions.test.js b/tests/components/accounts/actions.test.js index 3bd0aee..c8eb9b6 100644 --- a/tests/components/accounts/actions.test.js +++ b/tests/components/accounts/actions.test.js @@ -301,7 +301,7 @@ describe('components/accounts/actions', () => { beforeEach(() => { getState.returns({ accounts: { - active: account, + active: foreignAccount, available: [account, foreignAccount, foreignAccount2] }, user @@ -343,6 +343,37 @@ describe('components/accounts/actions', () => { ) ); + it('should not activate another account if active account is already not a stranger', () => { + getState.returns({ + accounts: { + active: account, + available: [account, foreignAccount] + }, + user + }); + + return logoutStrangers()(dispatch, getState) + .then(() => + expect(dispatch, 'was always called with', + expect.it('not to satisfy', activate(account))) + ); + }); + + it('should not dispatch if no strangers', () => { + getState.returns({ + accounts: { + active: account, + available: [account] + }, + user + }); + + return logoutStrangers()(dispatch, getState) + .then(() => + expect(dispatch, 'was not called') + ); + }); + describe('when all accounts are strangers', () => { beforeEach(() => { getState.returns({ @@ -368,7 +399,7 @@ describe('components/accounts/actions', () => { }); }); - describe('when an stranger has a mark in sessionStorage', () => { + describe('when a stranger has a mark in sessionStorage', () => { const key = `stranger${foreignAccount.id}`; beforeEach(() => { @@ -382,12 +413,9 @@ describe('components/accounts/actions', () => { }); it('should not log out', () => - expect(dispatch, 'to have calls satisfying', [ - [expect.it('not to equal', {payload: foreignAccount})], - // for some reason it says, that dispatch(authenticate(...)) - // must be removed if only one args assertion is listed :( - [expect.it('not to equal', {payload: foreignAccount})] - ]) + expect(dispatch, 'was always called with', + expect.it('not to equal', {payload: foreignAccount}) + ) ); }); }); From c5aa3037935764a425aab4489648940ec81ee527 Mon Sep 17 00:00:00 2001 From: SleepWalker Date: Thu, 12 Jan 2017 08:12:56 +0200 Subject: [PATCH 5/5] #246: remove User class in favor of plain object in reducer --- src/components/accounts/actions.js | 11 ++--- src/components/user/User.js | 74 ++++++------------------------ src/components/user/reducer.js | 43 +++++++++++++---- 3 files changed, 52 insertions(+), 76 deletions(-) diff --git a/src/components/accounts/actions.js b/src/components/accounts/actions.js index a76eecb..a088aac 100644 --- a/src/components/accounts/actions.js +++ b/src/components/accounts/actions.js @@ -1,18 +1,17 @@ import { routeActions } from 'react-router-redux'; import authentication from 'services/api/authentication'; -import accounts from 'services/api/accounts'; import { updateUser, setGuest } from 'components/user/actions'; import { setLocale } from 'components/i18n/actions'; import logger from 'services/logger'; /** * @typedef {object} Account - * @property {string} account.id - * @property {string} account.username - * @property {string} account.email - * @property {string} account.token - * @property {string} account.refreshToken + * @property {string} id + * @property {string} username + * @property {string} email + * @property {string} token + * @property {string} refreshToken */ /** diff --git a/src/components/user/User.js b/src/components/user/User.js index b56c548..6c0ae39 100644 --- a/src/components/user/User.js +++ b/src/components/user/User.js @@ -1,66 +1,18 @@ import { PropTypes } from 'react'; -const KEY_USER = 'user'; - -export default class User { - /** - * @param {object} [data] - plain object or jwt token or empty to load from storage - * - * @return {User} - */ - constructor(data) { - if (!data) { - return this.load(); - } - - // TODO: strict value types validation - - const defaults = { - id: null, - uuid: null, - username: '', - email: '', - // will contain user's email or masked email - // (e.g. ex**ple@em*il.c**) depending on what information user have already provided - maskedEmail: '', - avatar: '', - lang: '', - isActive: false, - shouldAcceptRules: false, // whether user need to review updated rules - passwordChangedAt: null, - hasMojangUsernameCollision: false, - - // frontend app specific attributes - isGuest: true, - goal: null, // the goal with wich user entered site - - // TODO: remove me after migration to multy accs - token: '', - refreshToken: '' - }; - - const user = Object.keys(defaults).reduce((user, key) => { - if (data.hasOwnProperty(key)) { - user[key] = data[key]; - } - - return user; - }, defaults); - - localStorage.setItem(KEY_USER, JSON.stringify(user)); - - return user; - } - - load() { - try { - return new User(JSON.parse(localStorage.getItem(KEY_USER))); - } catch (error) { - return new User({isGuest: true}); - } - } -} - +/** + * @typedef {object} User + * @property {number} id + * @property {string} uuid + * @property {string} token + * @property {string} username + * @property {string} email + * @property {string} avatar + * @property {bool} isGuest + * @property {bool} isActive + * @property {number} passwordChangedAt - timestamp + * @property {bool} hasMojangUsernameCollision + */ export const userShape = PropTypes.shape({ id: PropTypes.number, uuid: PropTypes.string, diff --git a/src/components/user/reducer.js b/src/components/user/reducer.js index b107027..e4e020f 100644 --- a/src/components/user/reducer.js +++ b/src/components/user/reducer.js @@ -1,10 +1,28 @@ import { UPDATE, SET, CHANGE_LANG } from './actions'; -import User from './User'; -// TODO: возможно есть смысл инитить обьект User снаружи, так как редусер не должен столько знать +const defaults = { + id: null, + uuid: null, + username: '', + email: '', + // will contain user's email or masked email + // (e.g. ex**ple@em*il.c**) depending on what information user have already provided + maskedEmail: '', + avatar: '', + lang: '', + isActive: false, + shouldAcceptRules: false, // whether user need to review updated rules + passwordChangedAt: null, + hasMojangUsernameCollision: false, + + // frontend specific attributes + isGuest: true, + goal: null // the goal with wich user entered site +}; + export default function user( - state = new User(), + state = null, {type, payload = null} ) { switch (type) { @@ -13,25 +31,32 @@ export default function user( throw new Error('payload.lang is required for user reducer'); } - return new User({ + return { ...state, lang: payload.lang - }); + }; case UPDATE: if (!payload) { throw new Error('payload is required for user reducer'); } - return new User({ + return { ...state, ...payload - }); + }; case SET: - return new User(payload || {}); + payload = payload || {}; + + return { + ...defaults, + ...payload + }; default: - return state; + return state || { + ...defaults + }; } }