Skip to content

Commit

Permalink
STCOR-865 call logout() exclusively from logout-* routes
Browse files Browse the repository at this point in the history
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 `<RootWithIntl>` 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` (`<Logout>`) and `/logout-timeout`
(`<LogoutTimeout>`). 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 `<MainNav>` are just clean-up work, removing cruft
that is no longer in use.

Refs STCOR-865
  • Loading branch information
zburke committed Jul 3, 2024
1 parent 6201292 commit 7fa9b80
Show file tree
Hide file tree
Showing 6 changed files with 48 additions and 54 deletions.
33 changes: 26 additions & 7 deletions src/components/LogoutTimeout/LogoutTimeout.js
Original file line number Diff line number Diff line change
@@ -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 <Redirect to="/" />;
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 <LoadingView />;
}

return (
Expand Down
11 changes: 9 additions & 2 deletions src/components/LogoutTimeout/LogoutTimeout.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -10,6 +12,10 @@ jest.mock('react-router', () => ({
Redirect: () => <div>Redirect</div>,
}));

jest.mock('../../loginServices', () => ({
logout: jest.fn(() => Promise.resolve()),
}));

describe('LogoutTimeout', () => {
it('if not authenticated, renders a timeout message', async () => {
const mockUseStripes = useStripes;
Expand All @@ -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(<LogoutTimeout />);
screen.getByText('Redirect');
expect(logout).toHaveBeenCalled();
screen.getByText('stripes-core.rtr.idleSession.sessionExpiredSoSad');
});
});
15 changes: 1 addition & 14 deletions src/components/MainNav/MainNav.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -213,10 +203,7 @@ class MainNav extends Component {
target="_blank"
/>
<NavDivider md="hide" />
<ProfileDropdown
onLogout={this.logout}
stripes={stripes}
/>
<ProfileDropdown stripes={stripes} />
</nav>
</header>
);
Expand Down
22 changes: 5 additions & 17 deletions src/components/SessionEventContainer/SessionEventContainer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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?
Expand All @@ -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();
};
Expand Down
20 changes: 6 additions & 14 deletions src/components/SessionEventContainer/SessionEventContainer.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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');
});

Expand All @@ -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');
});

Expand All @@ -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');
});

Expand All @@ -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');
});
});

Expand Down
1 change: 1 addition & 0 deletions test/jest/__mock__/stripesComponents.mock.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ jest.mock('@folio/stripes-components', () => ({
</ul>
)),
Loading: () => <div>Loading</div>,
LoadingView: () => <div>LoadingView</div>,
MessageBanner: jest.fn(({ show, children }) => { return show ? <>{children}</> : <></>; }),

// oy, dismissible. we need to pull it out of props so it doesn't
Expand Down

0 comments on commit 7fa9b80

Please sign in to comment.