From 9fe499fdc1deaeef84dd1a79a1166c78e397c947 Mon Sep 17 00:00:00 2001 From: Zak Burke Date: Thu, 14 Nov 2024 17:04:02 -0500 Subject: [PATCH 1/2] STCOR-907 cautiously evaluate localforage data Be thorough when evaluating data from localforage to determine whether a session exists by checking for `user.id`, `tenant`, and `isAuthenticated` values rather than accepting the existence of a (possibly empty, or mostly empty) object as proof. Without this check, a sparsely-populated value may be passed to `validateUser()`, causing it to throw when evaluating `user.id`, and misleadingly dispatching a server-down message. It is likely that a rogue RTR process is responsible for writing this garbled/sparse session data. We need to research that and resolve that problem too, but at least we have a handle on this from the other end. Refs STCOR-907 --- CHANGELOG.md | 4 ++ src/components/Root/Root.js | 6 +- src/components/Root/token-util.test.js | 12 ++++ src/loginServices.js | 54 +++++++++++----- src/loginServices.test.js | 87 +++++++++++++++++++++----- 5 files changed, 128 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 703291747..23c1ca0b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Change history for stripes-core +## 10.1.3 IN PROGRESS + +* Cautiously evaluate localforage data when restoring a session. Refs STCOR-907. + ## [10.1.2](https://github.com/folio-org/stripes-core/tree/v10.1.2) (2024-10-21) [Full Changelog](https://github.com/folio-org/stripes-core/compare/v10.1.1...v10.1.2) diff --git a/src/components/Root/Root.js b/src/components/Root/Root.js index ecba25a32..912531a39 100644 --- a/src/components/Root/Root.js +++ b/src/components/Root/Root.js @@ -81,7 +81,11 @@ class Root extends Component { componentDidMount() { const { okapi, store, locale, defaultTranslations } = this.props; - if (this.withOkapi) checkOkapiSession(okapi.url, store, okapi.tenant); + if (this.withOkapi) { + // checkOkapiSession is async and will cause a re-render after a delay + // when an API call's .then() handler updates the store. + checkOkapiSession(okapi.url, store, okapi.tenant); + } // TODO: remove this after we load locale and translations at start from a public endpoint loadTranslations(store, locale, defaultTranslations); } diff --git a/src/components/Root/token-util.test.js b/src/components/Root/token-util.test.js index e3686fa36..6c921aa75 100644 --- a/src/components/Root/token-util.test.js +++ b/src/components/Root/token-util.test.js @@ -1,3 +1,5 @@ +import localforage from 'localforage'; + import { RTRError, UnexpectedResourceError } from './Errors'; import { isFolioApiRequest, @@ -12,6 +14,16 @@ import { } from './token-util'; import { RTR_SUCCESS_EVENT } from './Events'; +jest.mock('localforage', () => ({ + getItem: jest.fn(() => Promise.resolve({ + user: { id: 'robert' }, + tenant: 'manhattan', + isAuthenticated: 'i am', + })), + setItem: jest.fn((k, v) => Promise.resolve(v)), + removeItem: jest.fn(() => Promise.resolve()), +})); + describe('isFolioApiRequest', () => { it('accepts requests whose origin matches okapi\'s', () => { const oUrl = 'https://millicent-sounds-kinda-like-malificent.edu'; diff --git a/src/loginServices.js b/src/loginServices.js index baf00569a..223e49bd2 100644 --- a/src/loginServices.js +++ b/src/loginServices.js @@ -76,18 +76,18 @@ const SESSION_NAME = 'okapiSess'; * simple wrapper around access to values stored in localforage * to insulate RTR functions from that API. * - * @returns {object} + * @returns Promise.resolve({object}) */ export const getOkapiSession = async () => { return localforage.getItem(SESSION_NAME); }; /** - * getTokenSess + * getTokenExpiry * simple wrapper around access to values stored in localforage * to insulate RTR functions from that API. * - * @returns {object} shaped like { atExpires, rtExpires }; each is a millisecond timestamp + * @returns Promise.resolve({object}) shaped like { atExpires, rtExpires }; each is a millisecond timestamp */ export const getTokenExpiry = async () => { const sess = await getOkapiSession(); @@ -101,11 +101,15 @@ export const getTokenExpiry = async () => { * session with updated token expiration data. * * @param {object} shaped like { atExpires, rtExpires }; each is a millisecond timestamp - * @returns {object} updated session object + * @returns Promise.resolve({object}) updated session object */ export const setTokenExpiry = async (te) => { const sess = await getOkapiSession(); - return localforage.setItem(SESSION_NAME, { ...sess, tokenExpiration: te }); + if (sess) { + return localforage.setItem(SESSION_NAME, { ...sess, tokenExpiration: te }); + } + + return null; }; @@ -708,18 +712,23 @@ export function validateUser(okapiUrl, store, tenant, session) { /** * checkOkapiSession - * 1. Pull the session from local storage; if non-empty validate it, dispatching load-resources actions. - * 2. Check if SSO (SAML) is enabled, dispatching check-sso actions - * 3. dispatch set-okapi-ready. + * 1 Pull the session from local storage; if non-empty validate it, + * dispatching load-resources actions to populate the store. + * 2 Check if SSO (SAML) is enabled, dispatching check-sso actions + * 3 dispatch set-okapi-ready. * * @param {string} okapiUrl * @param {redux store} store * @param {string} tenant */ export function checkOkapiSession(okapiUrl, store, tenant) { - getOkapiSession() + return getOkapiSession() .then((sess) => { - return sess !== null ? validateUser(okapiUrl, store, tenant, sess) : null; + if (sess?.user?.id && sess?.tenant && sess?.isAuthenticated) { + return validateUser(okapiUrl, store, tenant, sess); + } + + return null; }) .then(() => { return getSSOEnabled(okapiUrl, store, tenant); @@ -821,27 +830,38 @@ export function requestSSOLogin(okapiUrl, tenant) { export function updateUser(store, data) { return getOkapiSession() .then((sess) => { - sess.user = { ...sess.user, ...data }; - return localforage.setItem(SESSION_NAME, sess); + if (sess?.user?.id && sess?.tenant && sess?.isAuthenticated) { + sess.user = { ...sess.user, ...data }; + return localforage.setItem(SESSION_NAME, sess); + } + + throw Error('The user could not be updated because an existing session could not be found'); }) .then(() => { store.dispatch(updateCurrentUser(data)); + }) + .catch((e) => { + console.error(e); // eslint-disable-line no-console }); } /** * updateTenant * 1. prepare user info for requested tenant - * 2. update okapi session + * 2. update okapi session with user and tenant data * @param {object} okapi * @param {string} tenant * * @returns {Promise} */ export async function updateTenant(okapi, tenant) { - const okapiSess = await getOkapiSession(); - const userWithPermsResponse = await fetchUserWithPerms(okapi.url, tenant, okapi.token); - const userWithPerms = await userWithPermsResponse.json(); + const sess = await getOkapiSession(); + if (sess?.user?.id && sess?.tenant && sess?.isAuthenticated) { + const userWithPermsResponse = await fetchUserWithPerms(okapi.url, tenant, okapi.token); + const userWithPerms = await userWithPermsResponse.json(); - await localforage.setItem(SESSION_NAME, { ...okapiSess, tenant, ...spreadUserWithPerms(userWithPerms) }); + await localforage.setItem(SESSION_NAME, { ...sess, tenant, ...spreadUserWithPerms(userWithPerms) }); + } else { + console.error('The tenant could not be updated because an existing session could not be found'); // eslint-disable-line no-console + } } diff --git a/src/loginServices.test.js b/src/loginServices.test.js index c1d2d95b5..47e4426e1 100644 --- a/src/loginServices.test.js +++ b/src/loginServices.test.js @@ -342,7 +342,14 @@ describe('validateUser', () => { }); describe('updateUser', () => { - it('dispatches updateCurrentUser', async () => { + it('dispatches updateCurrentUser when a session exists', async () => { + const s = { + user: { id: 'robert' }, + tenant: 'manhattan', + isAuthenticated: 'i am', + }; + localforage.getItem = () => Promise.resolve(s); + const store = { dispatch: jest.fn(), }; @@ -350,9 +357,26 @@ describe('updateUser', () => { await updateUser(store, data); expect(store.dispatch).toHaveBeenCalledWith(updateCurrentUser(data)); }); + + it('does nothing when session is not found', async () => { + localforage.getItem = () => Promise.resolve(null); + + const store = { + dispatch: jest.fn(), + }; + const data = { thunder: 'chicken' }; + await updateUser(store, data); + expect(store.dispatch).not.toHaveBeenCalled(); + }); }); describe('updateTenant', () => { + const s = { + user: { id: 'robert' }, + tenant: 'manhattan', + isAuthenticated: 'i am', + }; + const okapi = { currentPerms: {}, }; @@ -371,16 +395,30 @@ describe('updateTenant', () => { localforage.setItem.mockClear(); }); - it('should set tenant and updated user in session', async () => { + it('updates tenant and user when session exists', async () => { + localforage.getItem = () => Promise.resolve(s); + localforage.setItem = jest.fn(() => Promise.resolve(s)); + mockFetchSuccess(data); await updateTenant(okapi, tenant); mockFetchCleanUp(); expect(localforage.setItem).toHaveBeenCalledWith('okapiSess', { + ...s, ...spreadUserWithPerms(data), tenant, }); }); + + it('does nothing when session is not found', async () => { + localforage.getItem = () => Promise.resolve(null); + + mockFetchSuccess(data); + await updateTenant(okapi, tenant); + mockFetchCleanUp(); + + expect(localforage.setItem).not.toHaveBeenCalled(); + }); }); describe('localforage session wrapper', () => { @@ -410,22 +448,37 @@ describe('localforage session wrapper', () => { }); }); - it('setTokenExpiry set', async () => { - const o = { - margo: 'timmins', - margot: 'margot with a t looks better', - also: 'i thought we were talking about margot robbie?', - tokenExpiration: 'time out of mind', - }; - localforage.getItem = () => Promise.resolve(o); - localforage.setItem = (k, v) => Promise.resolve(v); + describe('setTokenExpiry', () => { + it('saves data when a session exists', async () => { + const o = { + margo: 'timmins', + margot: 'margot with a t looks better', + also: 'i thought we were talking about margot robbie?', + tokenExpiration: 'time out of mind', + }; + localforage.getItem = () => Promise.resolve(o); + localforage.setItem = (k, v) => Promise.resolve(v); - const te = { - trinity: 'cowboy junkies', - sweet: 'james', - }; + const te = { + trinity: 'cowboy junkies', + sweet: 'james', + }; + + const s = await setTokenExpiry(te); + expect(s).toMatchObject({ ...o, tokenExpiration: te }); + }); + + it('does nothing when session does not exist', async () => { + const o = null; + localforage.getItem = () => Promise.resolve(o); + localforage.setItem = (k, v) => Promise.resolve(v); - const s = await setTokenExpiry(te); - expect(s).toMatchObject({ ...o, tokenExpiration: te }); + const te = { + now: 'i am become death, destroyer of sessions', + }; + + const s = await setTokenExpiry(te); + expect(s).toBeNull(); + }); }); }); From 0f4b867b65b0328387e74a70b6239fd429100913 Mon Sep 17 00:00:00 2001 From: Zak Burke Date: Thu, 14 Nov 2024 21:59:07 -0500 Subject: [PATCH 2/2] additional tests --- src/loginServices.test.js | 82 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/src/loginServices.test.js b/src/loginServices.test.js index 47e4426e1..b34288825 100644 --- a/src/loginServices.test.js +++ b/src/loginServices.test.js @@ -1,6 +1,7 @@ import localforage from 'localforage'; import { + checkOkapiSession, createOkapiSession, getOkapiSession, getTokenExpiry, @@ -482,3 +483,84 @@ describe('localforage session wrapper', () => { }); }); }); + +describe('checkOkapiSession', () => { + it('dispatches setOkapiReady', async () => { + const o = { + user: { id: 'id' }, + tenant: 'tenant', + isAuthenticated: true, + }; + localforage.getItem = jest.fn(() => Promise.resolve(o)); + const store = { + dispatch: jest.fn(), + getState: () => ({ + okapi: { + currentPerms: [], + } + }), + }; + + const data = { data: 'd' }; + + mockFetchSuccess(data); + + await checkOkapiSession('url', store, o.tenant); + expect(store.dispatch).toHaveBeenCalledWith(setOkapiReady()); + + mockFetchCleanUp(); + }); + + it('when getOkapiSession returns full session data, validates it', async () => { + const o = { + user: { id: 'id' }, + tenant: 'tenant', + isAuthenticated: true, + }; + localforage.getItem = jest.fn(() => Promise.resolve(o)); + const store = { + dispatch: jest.fn(), + getState: () => ({ + okapi: { + currentPerms: [], + } + }), + }; + + const data = { data: 'd' }; + + mockFetchSuccess(data); + + await checkOkapiSession('url', store, 'tenant'); + + expect(store.dispatch).toHaveBeenCalledWith(setAuthError(null)); + expect(store.dispatch).toHaveBeenCalledWith(setOkapiReady()); + + mockFetchCleanUp(); + }); + + it('when getOkapiSession returns sparse session data, ignores it', async () => { + const o = { monkey: 'bagel' }; + localforage.getItem = jest.fn(() => Promise.resolve(o)); + const store = { + dispatch: jest.fn(), + getState: () => ({ + okapi: { + currentPerms: [], + } + }), + }; + + const data = { data: 'd' }; + + mockFetchSuccess(data); + + await checkOkapiSession('url', store, o.tenant); + + expect(store.dispatch).not.toHaveBeenCalledWith(setAuthError(null)); + expect(store.dispatch).not.toHaveBeenCalledWith(setLoginData(data)); + expect(store.dispatch).toHaveBeenCalledWith(setOkapiReady(null)); + + mockFetchCleanUp(); + }); +});