From 9da79a15b45ddd8896aee6f8484cc2baca739498 Mon Sep 17 00:00:00 2001 From: SleepWalker Date: Mon, 5 Dec 2016 21:14:38 +0200 Subject: [PATCH] #232: fix remember me logic on frontend --- src/components/accounts/actions.js | 56 ++++++++-- src/components/user/factory.js | 31 +++--- tests/components/accounts/actions.test.js | 124 +++++++++++++++++++++- tests/index.js | 2 +- 4 files changed, 188 insertions(+), 25 deletions(-) diff --git a/src/components/accounts/actions.js b/src/components/accounts/actions.js index fba3b0f..0d69005 100644 --- a/src/components/accounts/actions.js +++ b/src/components/accounts/actions.js @@ -49,6 +49,11 @@ export function authenticate({token, refreshToken}) { ...user })); + if (!account.refreshToken) { + // mark user as stranger (user does not want us to remember his account) + sessionStorage.setItem(`stranger${account.id}`, 1); + } + return dispatch(setLocale(user.lang)) .then(() => account); }); @@ -75,6 +80,48 @@ export function revoke(account) { }; } +export function logoutAll() { + return (dispatch, getState) => { + const {accounts: {available}} = getState(); + + available.forEach((account) => authentication.logout(account)); + + dispatch(reset()); + }; +} + +/** + * Logouts accounts, that was marked as "do not remember me" + * + * We detecting foreign accounts by the absence of refreshToken. The account + * won't be removed, until key `stranger${account.id}` is present in sessionStorage + * + * @return {function} + */ +export function logoutStrangers() { + return (dispatch, getState) => { + const {accounts: {available}} = getState(); + + const isStranger = ({refreshToken, id}) => !refreshToken && !sessionStorage.getItem(`stranger${id}`); + + const accountToReplace = available.filter((account) => !isStranger(account))[0]; + + if (accountToReplace) { + available.filter(isStranger) + .forEach((account) => { + dispatch(remove(account)); + authentication.logout(account); + }); + + return dispatch(authenticate(accountToReplace)); + } + + dispatch(logout()); + + return Promise.resolve(); + }; +} + export const ADD = 'accounts:add'; /** * @api private @@ -120,15 +167,6 @@ export function activate(account) { }; } -export function logoutAll() { - return (dispatch, getState) => { - const {accounts: {available}} = getState(); - - available.forEach((account) => authentication.logout(account)); - - dispatch(reset()); - }; -} export const RESET = 'accounts:reset'; /** * @api private diff --git a/src/components/user/factory.js b/src/components/user/factory.js index cc71574..58af5cf 100644 --- a/src/components/user/factory.js +++ b/src/components/user/factory.js @@ -1,5 +1,5 @@ import { changeLang } from 'components/user/actions'; -import { authenticate } from 'components/accounts/actions'; +import { authenticate, logoutStrangers } from 'components/accounts/actions'; import request from 'services/request'; import bearerHeaderMiddleware from './middlewares/bearerHeaderMiddleware'; @@ -22,22 +22,25 @@ export function factory(store) { request.addMiddleware(refreshTokenMiddleware(store)); request.addMiddleware(bearerHeaderMiddleware(store)); - promise = Promise.resolve().then(() => { - const {user, accounts} = store.getState(); + promise = Promise.resolve() + .then(() => store.dispatch(logoutStrangers())) + .then(() => { + const {user, accounts} = store.getState(); - if (accounts.active || user.token) { - // authorizing user if it is possible - return store.dispatch(authenticate(accounts.active || user)); - } + if (accounts.active || user.token) { + // authorizing user if it is possible + return store.dispatch(authenticate(accounts.active || user)); + } - return Promise.reject(); - }).catch(() => { - // the user is guest or user authentication failed - const {user} = store.getState(); + return Promise.reject(); + }) + .catch(() => { + // the user is guest or user authentication failed + const {user} = store.getState(); - // auto-detect guest language - return store.dispatch(changeLang(user.lang)); - }); + // auto-detect guest language + return store.dispatch(changeLang(user.lang)); + }); return promise; } diff --git a/tests/components/accounts/actions.test.js b/tests/components/accounts/actions.test.js index 297dbdd..29c404e 100644 --- a/tests/components/accounts/actions.test.js +++ b/tests/components/accounts/actions.test.js @@ -1,4 +1,5 @@ import expect from 'unexpected'; +import sinon from 'sinon'; import accounts from 'services/api/accounts'; import authentication from 'services/api/authentication'; @@ -9,7 +10,8 @@ import { activate, ACTIVATE, remove, reset, - logoutAll + logoutAll, + logoutStrangers } from 'components/accounts/actions'; import { SET_LOCALE } from 'components/i18n/actions'; @@ -114,6 +116,20 @@ describe('components/accounts/actions', () => { expect(dispatch, 'was not called') ); }); + + it('marks user as stranger, if there is no refreshToken', () => { + const expectedKey = `stranger${account.id}`; + authentication.validateToken.returns(Promise.resolve({ + token: account.token + })); + + sessionStorage.removeItem(expectedKey); + + return authenticate(account)(dispatch).then(() => { + expect(sessionStorage.getItem(expectedKey), 'not to be null') + sessionStorage.removeItem(expectedKey); + }); + }); }); describe('#revoke()', () => { @@ -242,4 +258,110 @@ describe('components/accounts/actions', () => { ]); }); }); + + describe('#logoutStrangers', () => { + const foreignAccount = { + ...account, + id: 2, + refreshToken: undefined + }; + + const foreignAccount2 = { + ...foreignAccount, + id: 3 + }; + + beforeEach(() => { + getState.returns({ + accounts: { + active: account, + available: [account, foreignAccount, foreignAccount2] + }, + user + }); + + sinon.stub(authentication, 'logout').named('authentication.logout'); + }); + + afterEach(() => { + authentication.logout.restore(); + }); + + it('should remove stranger accounts', () => { + logoutStrangers()(dispatch, getState); + + expect(dispatch, 'to have a call satisfying', [ + remove(foreignAccount) + ]); + expect(dispatch, 'to have a call satisfying', [ + remove(foreignAccount2) + ]); + }); + + it('should logout stranger accounts', () => { + logoutStrangers()(dispatch, getState); + + expect(authentication.logout, 'to have calls satisfying', [ + [foreignAccount], + [foreignAccount2] + ]); + }); + + it('should activate another account if available', () => + logoutStrangers()(dispatch, getState) + .then(() => + expect(dispatch, 'to have a call satisfying', [ + activate(account) + ]) + ) + ); + + describe('when all accounts are strangers', () => { + beforeEach(() => { + getState.returns({ + accounts: { + active: foreignAccount, + available: [foreignAccount, foreignAccount2] + }, + user + }); + + logoutStrangers()(dispatch, getState); + }); + + it('logouts all accounts', () => { + expect(dispatch, 'to have a call satisfying', [ + {payload: {isGuest: true}} + // updateUser({isGuest: true}) + ]); + + expect(dispatch, 'to have a call satisfying', [ + reset() + ]); + }); + }); + + describe('when an stranger has a mark in sessionStorage', () => { + const key = `stranger${foreignAccount.id}`; + + beforeEach(() => { + sessionStorage.setItem(key, 1); + + logoutStrangers()(dispatch, getState); + }); + + afterEach(() => { + sessionStorage.removeItem(key); + }); + + 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})] + ]) + ); + }); + }); }); diff --git a/tests/index.js b/tests/index.js index 05f13ad..beb5975 100644 --- a/tests/index.js +++ b/tests/index.js @@ -6,7 +6,7 @@ expect.use(require('unexpected-sinon')); if (!window.localStorage) { window.localStorage = { getItem(key) { - return this[key]; + return this[key] || null; }, setItem(key, value) { this[key] = value;