From 8daa26711c1435f9853a53f2603d285e011c3398 Mon Sep 17 00:00:00 2001 From: Zak Burke Date: Mon, 8 Jul 2024 08:22:23 -0400 Subject: [PATCH] STCOR-865 call logout() exclusively from logout-* routes (#1500) Two things happen when idle-session-timeout kicks in: 1. the redux store is updated to clear out the session 2. the URL is updated to `/logout-timeout` It sounds simple, but it gets messy when `` re-renders when the store updates because that's where routes are defined. Previously, with event-handlers separately calling `logout()` to update the store and `history.push()` to update the URL, you could end up in an unexpected situation such as being logged-out before the URL updated to `/logout-timeout`, causing the default route-match handler to kick in and redirect to the login screen. The changes here consolidate calls to `logout()` into the components bound to `/logout` (``) and `/logout-timeout` (``). Event handlers that previously did things like ``` return logout(...) // update redux and other storage .then(history.push(...)) // update URL ``` are now limited to updating the URL. This means directly accessing the routes `/logout` and `/logout-timeout` always terminates a session, and the logic around logout is both simpler and better contained within components whose purpose, by dint of their names, is blindingly clear. The minor changes in `` are just clean-up work, removing cruft that is no longer in use. Refs STCOR-865 --- src/components/LogoutTimeout/LogoutTimeout.js | 33 +++++++++++++++---- .../LogoutTimeout/LogoutTimeout.test.js | 11 +++++-- src/components/MainNav/MainNav.js | 15 +-------- .../SessionEventContainer.js | 22 +++---------- .../SessionEventContainer.test.js | 20 ++++------- test/jest/__mock__/stripesComponents.mock.js | 1 + 6 files changed, 48 insertions(+), 54 deletions(-) diff --git a/src/components/LogoutTimeout/LogoutTimeout.js b/src/components/LogoutTimeout/LogoutTimeout.js index 2eda11421..e0af76f17 100644 --- a/src/components/LogoutTimeout/LogoutTimeout.js +++ b/src/components/LogoutTimeout/LogoutTimeout.js @@ -1,36 +1,55 @@ +import { useEffect, useState } from 'react'; import { FormattedMessage } from 'react-intl'; import { branding } from 'stripes-config'; -import { Redirect } from 'react-router'; import { Button, Col, Headline, + LoadingView, Row, } from '@folio/stripes-components'; import OrganizationLogo from '../OrganizationLogo'; import { useStripes } from '../../StripesContext'; +import { logout } from '../../loginServices'; import styles from './LogoutTimeout.css'; /** * LogoutTimeout - * For unauthenticated users, show a "sorry, your session timed out" message - * with a link to login page. For authenticated users, redirect to / since - * showing such a message would be a misleading lie. + * Show a "sorry, your session timed out message"; if the session is still + * active, call logout() to end it. * * Having a static route to this page allows the logout handler to choose * between redirecting straight to the login page (if the user chose to - * logout) or to this page (if the session timeout out). + * logout) or to this page (if the session timed out). * * This corresponds to the '/logout-timeout' route. */ const LogoutTimeout = () => { const stripes = useStripes(); + const [didLogout, setDidLogout] = useState(false); - if (stripes.okapi.isAuthenticated) { - return ; + useEffect( + () => { + if (stripes.okapi.isAuthenticated) { + // returns a promise, which we ignore + logout(stripes.okapi.url, stripes.store) + .then(setDidLogout(true)); + } else { + setDidLogout(true); + } + }, + // no dependencies because we only want to start the logout process once. + // we don't care about changes to stripes; certainly it'll be updated as + // part of the logout process + // eslint-disable-next-line react-hooks/exhaustive-deps + [] + ); + + if (!didLogout) { + return ; } return ( diff --git a/src/components/LogoutTimeout/LogoutTimeout.test.js b/src/components/LogoutTimeout/LogoutTimeout.test.js index 55c9c9b5b..6efff5e0c 100644 --- a/src/components/LogoutTimeout/LogoutTimeout.test.js +++ b/src/components/LogoutTimeout/LogoutTimeout.test.js @@ -2,6 +2,8 @@ import { render, screen } from '@folio/jest-config-stripes/testing-library/react import LogoutTimeout from './LogoutTimeout'; import { useStripes } from '../../StripesContext'; +import { logout } from '../../loginServices'; + jest.mock('../OrganizationLogo'); @@ -10,6 +12,10 @@ jest.mock('react-router', () => ({ Redirect: () =>
Redirect
, })); +jest.mock('../../loginServices', () => ({ + logout: jest.fn(() => Promise.resolve()), +})); + describe('LogoutTimeout', () => { it('if not authenticated, renders a timeout message', async () => { const mockUseStripes = useStripes; @@ -19,11 +25,12 @@ describe('LogoutTimeout', () => { screen.getByText('stripes-core.rtr.idleSession.sessionExpiredSoSad'); }); - it('if authenticated, renders a redirect', async () => { + it('if authenticated, calls logout then renders a timeout message', async () => { const mockUseStripes = useStripes; mockUseStripes.mockReturnValue({ okapi: { isAuthenticated: true } }); render(); - screen.getByText('Redirect'); + expect(logout).toHaveBeenCalled(); + screen.getByText('stripes-core.rtr.idleSession.sessionExpiredSoSad'); }); }); diff --git a/src/components/MainNav/MainNav.js b/src/components/MainNav/MainNav.js index 1ced15b62..25b473ea3 100644 --- a/src/components/MainNav/MainNav.js +++ b/src/components/MainNav/MainNav.js @@ -11,7 +11,6 @@ import { Icon } from '@folio/stripes-components'; import { withModules } from '../Modules'; import { LastVisitedContext } from '../LastVisited'; -import { getLocale, logout as sessionLogout } from '../../loginServices'; import { updateQueryResource, getLocationQuery, @@ -65,7 +64,6 @@ class MainNav extends Component { userMenuOpen: false, }; this.store = props.stripes.store; - this.logout = this.logout.bind(this); this.getAppList = this.getAppList.bind(this); } @@ -116,14 +114,6 @@ class MainNav extends Component { }); } - // return the user to the login screen, but after logging in they will be brought to the default screen. - logout() { - const { okapi } = this.store.getState(); - - return getLocale(okapi.url, this.store, okapi.tenant) - .then(sessionLogout(okapi.url, this.store, this.props.history)); - } - getAppList(lastVisited) { const { stripes, location: { pathname }, modules, intl: { formatMessage } } = this.props; @@ -213,10 +203,7 @@ class MainNav extends Component { target="_blank" /> - + ); diff --git a/src/components/SessionEventContainer/SessionEventContainer.js b/src/components/SessionEventContainer/SessionEventContainer.js index ff1707764..21ca8f4e1 100644 --- a/src/components/SessionEventContainer/SessionEventContainer.js +++ b/src/components/SessionEventContainer/SessionEventContainer.js @@ -3,7 +3,7 @@ import PropTypes from 'prop-types'; import createInactivityTimer from 'inactivity-timer'; import ms from 'ms'; -import { logout, SESSION_NAME } from '../../loginServices'; +import { SESSION_NAME } from '../../loginServices'; import KeepWorkingModal from './KeepWorkingModal'; import { useStripes } from '../../StripesContext'; import { @@ -21,19 +21,13 @@ import { toggleRtrModal } from '../../okapiActions'; // RTR error in this window: logout export const thisWindowRtrError = (_e, stripes, history) => { console.warn('rtr error; logging out'); // eslint-disable-line no-console - return logout(stripes.okapi.url, stripes.store) - .then(() => { - history.push('/logout-timeout'); - }); + history.push('/logout-timeout'); }; // idle session timeout in this window: logout export const thisWindowRtrTimeout = (_e, stripes, history) => { stripes.logger.log('rtr', 'idle session timeout; logging out'); - return logout(stripes.okapi.url, stripes.store) - .then(() => { - history.push('/logout-timeout'); - }); + history.push('/logout-timeout'); }; // localstorage change in another window: logout? @@ -43,16 +37,10 @@ export const thisWindowRtrTimeout = (_e, stripes, history) => { export const otherWindowStorage = (e, stripes, history) => { if (e.key === RTR_TIMEOUT_EVENT) { stripes.logger.log('rtr', 'idle session timeout; logging out'); - return logout(stripes.okapi.url, stripes.store) - .then(() => { - history.push('/logout-timeout'); - }); + history.push('/logout-timeout'); } else if (!localStorage.getItem(SESSION_NAME)) { stripes.logger.log('rtr', 'external localstorage change; logging out'); - return logout(stripes.okapi.url, stripes.store) - .then(() => { - history.push('/'); - }); + history.push('/logout'); } return Promise.resolve(); }; diff --git a/src/components/SessionEventContainer/SessionEventContainer.test.js b/src/components/SessionEventContainer/SessionEventContainer.test.js index 09b68f19a..24d9fd04c 100644 --- a/src/components/SessionEventContainer/SessionEventContainer.test.js +++ b/src/components/SessionEventContainer/SessionEventContainer.test.js @@ -9,7 +9,7 @@ import SessionEventContainer, { thisWindowRtrError, thisWindowRtrTimeout, } from './SessionEventContainer'; -import { logout, SESSION_NAME } from '../../loginServices'; +import { SESSION_NAME } from '../../loginServices'; import { RTR_TIMEOUT_EVENT } from '../Root/constants'; import { toggleRtrModal } from '../../okapiActions'; @@ -69,11 +69,8 @@ describe('SessionEventContainer', () => { describe('SessionEventContainer event listeners', () => { it('thisWindowRtrError', async () => { const history = { push: jest.fn() }; - const logoutMock = logout; - logoutMock.mockReturnValue(Promise.resolve()); - await thisWindowRtrError(null, { okapi: { url: 'http' } }, history); - expect(logout).toHaveBeenCalled(); + thisWindowRtrError(null, { okapi: { url: 'http' } }, history); expect(history.push).toHaveBeenCalledWith('/logout-timeout'); }); @@ -89,11 +86,8 @@ describe('SessionEventContainer event listeners', () => { }; const history = { push: jest.fn() }; - const logoutMock = logout; - await logoutMock.mockReturnValue(Promise.resolve()); - await thisWindowRtrTimeout(null, s, history); - expect(logout).toHaveBeenCalled(); + thisWindowRtrTimeout(null, s, history); expect(history.push).toHaveBeenCalledWith('/logout-timeout'); }); @@ -115,8 +109,7 @@ describe('SessionEventContainer event listeners', () => { }; const history = { push: jest.fn() }; - await otherWindowStorage(e, s, history); - expect(logout).toHaveBeenCalledWith(s.okapi.url, s.store); + otherWindowStorage(e, s, history); expect(history.push).toHaveBeenCalledWith('/logout-timeout'); }); @@ -133,9 +126,8 @@ describe('SessionEventContainer event listeners', () => { }; const history = { push: jest.fn() }; - await otherWindowStorage(e, s, history); - expect(logout).toHaveBeenCalledWith(s.okapi.url, s.store); - expect(history.push).toHaveBeenCalledWith('/'); + otherWindowStorage(e, s, history); + expect(history.push).toHaveBeenCalledWith('/logout'); }); }); diff --git a/test/jest/__mock__/stripesComponents.mock.js b/test/jest/__mock__/stripesComponents.mock.js index ba14f98ff..49bac0f60 100644 --- a/test/jest/__mock__/stripesComponents.mock.js +++ b/test/jest/__mock__/stripesComponents.mock.js @@ -76,6 +76,7 @@ jest.mock('@folio/stripes-components', () => ({ )), Loading: () =>
Loading
, + LoadingView: () =>
LoadingView
, MessageBanner: jest.fn(({ show, children }) => { return show ? <>{children} : <>; }), // oy, dismissible. we need to pull it out of props so it doesn't