diff --git a/src/components/Root/FFetch.js b/src/components/Root/FFetch.js index 5312445c0..ef84814b8 100644 --- a/src/components/Root/FFetch.js +++ b/src/components/Root/FFetch.js @@ -47,6 +47,7 @@ import { setRtrTimeout } from '../../okapiActions'; +import { getTokenExpiry } from '../../loginServices'; import { getPromise, isAuthenticationRequest, @@ -58,10 +59,10 @@ import { RTRError, } from './Errors'; import { + RTR_AT_EXPIRY_IF_UNKNOWN, RTR_AT_TTL_FRACTION, RTR_ERROR_EVENT, } from './constants'; - import FXHR from './FXHR'; const OKAPI_FETCH_OPTIONS = { @@ -107,19 +108,42 @@ export class FFetch { rotateCallback = (res) => { this.logger.log('rtr', 'rotation callback setup'); - // set a short rotation interval by default, then inspect the response for - // token-expiration data to use instead if available. not all responses - // (e.g. those from _self) contain token-expiration values, so it is - // necessary to provide a default. - let rotationInterval = 10 * 1000; + const scheduleRotation = (rotationP) => { + rotationP.then((rotationInterval) => { + this.logger.log('rtr', `rotation fired from rotateCallback; next callback in ${ms(rotationInterval)}`); + this.store.dispatch(setRtrTimeout(setTimeout(() => { + rtr(this.nativeFetch, this.logger, this.rotateCallback); + }, rotationInterval))); + }); + }; + + // When starting a new session, the response from /bl-users/login-with-expiry + // will contain AT expiration info, but when restarting an existing session, + // the response from /bl-users/_self will NOT, although that information will + // have been cached in local-storage. + // + // This means there are many places we have to check to figure out when the + // AT is likely to expire, and thus when we want to rotate. First inspect + // the response, otherwise the session. Default to 10 seconds. if (res?.accessTokenExpiration) { - rotationInterval = (new Date(res.accessTokenExpiration).getTime() - Date.now()) * RTR_AT_TTL_FRACTION; + this.logger.log('rtr', 'rotation scheduled with login response data'); + const rotationPromise = Promise.resolve((new Date(res.accessTokenExpiration).getTime() - Date.now()) * RTR_AT_TTL_FRACTION); + + scheduleRotation(rotationPromise); + } else { + const rotationPromise = getTokenExpiry().then((expiry) => { + if (expiry.atExpires) { + this.logger.log('rtr', 'rotation scheduled with cached session data'); + return (new Date(expiry.atExpires).getTime() - Date.now()) * RTR_AT_TTL_FRACTION; + } + + // default: 10 seconds + this.logger.log('rtr', 'rotation scheduled with default value'); + return ms(RTR_AT_EXPIRY_IF_UNKNOWN); + }); + + scheduleRotation(rotationPromise); } - - this.logger.log('rtr', `rotation fired from rotateCallback; next callback in ${ms(rotationInterval)}`); - this.store.dispatch(setRtrTimeout(setTimeout(() => { - rtr(this.nativeFetch, this.logger, this.rotateCallback); - }, rotationInterval))); } /** @@ -158,13 +182,16 @@ export class FFetch { this.logger.log('rtr', 'authn request'); return this.nativeFetch.apply(global, [resource, options && { ...options, ...OKAPI_FETCH_OPTIONS }]) .then(res => { - this.logger.log('rtr', 'authn success!'); // a response can only be read once, so we clone it to grab the // tokenExpiration in order to kick of the rtr cycle, then return // the original - res.clone().json().then(json => { - this.rotateCallback(json.tokenExpiration); - }); + const clone = res.clone(); + if (clone.ok) { + this.logger.log('rtr', 'authn success!'); + clone.json().then(json => { + this.rotateCallback(json.tokenExpiration); + }); + } return res; }); diff --git a/src/components/Root/FFetch.test.js b/src/components/Root/FFetch.test.js index 8013b93ff..8ada9a7c4 100644 --- a/src/components/Root/FFetch.test.js +++ b/src/components/Root/FFetch.test.js @@ -2,9 +2,15 @@ // FFetch for the reassign globals side-effect in its constructor. /* eslint-disable no-unused-vars */ +import ms from 'ms'; + import { getTokenExpiry } from '../../loginServices'; import { FFetch } from './FFetch'; import { RTRError, UnexpectedResourceError } from './Errors'; +import { + RTR_AT_EXPIRY_IF_UNKNOWN, + RTR_AT_TTL_FRACTION, +} from './constants'; jest.mock('../../loginServices', () => ({ ...(jest.requireActual('../../loginServices')), @@ -145,6 +151,163 @@ describe('FFetch class', () => { }); }); + describe('calling authentication resources', () => { + it('handles RTR data in the response', async () => { + // a static timestamp representing "now" + const whatTimeIsItMrFox = 1718042609734; + + // a static timestamp of when the AT will expire, in the future + // this value will be pushed into the response returned from the fetch + const accessTokenExpiration = whatTimeIsItMrFox + 5000; + + const st = jest.spyOn(window, 'setTimeout'); + + // dummy date data: assume session + Date.now = () => whatTimeIsItMrFox; + + const cloneJson = jest.fn(); + const clone = () => ({ + ok: true, + json: () => Promise.resolve({ tokenExpiration: { accessTokenExpiration } }) + }); + + mockFetch.mockResolvedValueOnce({ + ok: false, + clone, + }); + + mockFetch.mockResolvedValueOnce('okapi success'); + const testFfetch = new FFetch({ + logger: { log }, + store: { + dispatch: jest.fn(), + } + }); + testFfetch.replaceFetch(); + testFfetch.replaceXMLHttpRequest(); + + const response = await global.fetch('okapiUrl/bl-users/_self', { testOption: 'test' }); + // why this extra await/setTimeout? Because RTR happens in an un-awaited + // promise in a separate thread fired off by setTimout, and we need to + // give it the chance to complete. on the one hand, this feels super + // gross, but on the other, since we're deliberately pushing rotation + // into a separate thread, I'm note sure of a better way to handle this. + await setTimeout(Promise.resolve(), 2000); + expect(st).toHaveBeenCalledWith(expect.any(Function), (accessTokenExpiration - whatTimeIsItMrFox) * RTR_AT_TTL_FRACTION); + }); + + it('handles RTR data in the session', async () => { + // a static timestamp representing "now" + const whatTimeIsItMrFox = 1718042609734; + + // a static timestamp of when the AT will expire, in the future + // this value will be retrieved from local storage via getTokenExpiry + const atExpires = whatTimeIsItMrFox + 5000; + + const st = jest.spyOn(window, 'setTimeout'); + + getTokenExpiry.mockResolvedValue({ atExpires }); + Date.now = () => whatTimeIsItMrFox; + + const cloneJson = jest.fn(); + const clone = () => ({ + ok: true, + json: () => Promise.resolve({}), + }); + + mockFetch.mockResolvedValueOnce({ + ok: false, + clone, + }); + + mockFetch.mockResolvedValueOnce('okapi success'); + const testFfetch = new FFetch({ + logger: { log }, + store: { + dispatch: jest.fn(), + } + }); + testFfetch.replaceFetch(); + testFfetch.replaceXMLHttpRequest(); + + const response = await global.fetch('okapiUrl/bl-users/_self', { testOption: 'test' }); + // why this extra await/setTimeout? Because RTR happens in an un-awaited + // promise in a separate thread fired off by setTimout, and we need to + // give it the chance to complete. on the one hand, this feels super + // gross, but on the other, since we're deliberately pushing rotation + // into a separate thread, I'm note sure of a better way to handle this. + await setTimeout(Promise.resolve(), 2000); + expect(st).toHaveBeenCalledWith(expect.any(Function), (atExpires - whatTimeIsItMrFox) * RTR_AT_TTL_FRACTION); + }); + + it('handles missing RTR data', async () => { + const st = jest.spyOn(window, 'setTimeout'); + getTokenExpiry.mockResolvedValue({}); + + const cloneJson = jest.fn(); + const clone = () => ({ + ok: true, + json: () => Promise.resolve({}), + }); + + mockFetch.mockResolvedValueOnce({ + ok: false, + clone, + }); + + mockFetch.mockResolvedValueOnce('okapi success'); + const testFfetch = new FFetch({ + logger: { log }, + store: { + dispatch: jest.fn(), + } + }); + testFfetch.replaceFetch(); + testFfetch.replaceXMLHttpRequest(); + + const response = await global.fetch('okapiUrl/bl-users/_self', { testOption: 'test' }); + // why this extra await/setTimeout? Because RTR happens in an un-awaited + // promise in a separate thread fired off by setTimout, and we need to + // give it the chance to complete. on the one hand, this feels super + // gross, but on the other, since we're deliberately pushing rotation + // into a separate thread, I'm note sure of a better way to handle this. + await setTimeout(Promise.resolve(), 2000); + + expect(st).toHaveBeenCalledWith(expect.any(Function), ms(RTR_AT_EXPIRY_IF_UNKNOWN)); + }); + + it('handles unsuccessful responses', async () => { + jest.spyOn(window, 'dispatchEvent'); + jest.spyOn(console, 'error'); + + const cloneJson = jest.fn(); + const clone = () => ({ + ok: false, + json: cloneJson, + }); + + mockFetch.mockResolvedValueOnce({ + ok: false, + clone, + }); + + mockFetch.mockResolvedValueOnce('okapi success'); + const testFfetch = new FFetch({ + logger: { log }, + store: { + dispatch: jest.fn(), + } + }); + testFfetch.replaceFetch(); + testFfetch.replaceXMLHttpRequest(); + + const response = await global.fetch('okapiUrl/bl-users/_self', { testOption: 'test' }); + expect(mockFetch.mock.calls).toHaveLength(1); + expect(cloneJson).not.toHaveBeenCalled(); + }); + }); + + describe('Calling an okapi fetch with missing token...', () => { it('returns the error', async () => { mockFetch.mockResolvedValue('success') diff --git a/src/components/Root/constants.js b/src/components/Root/constants.js index 58ddc16db..771467234 100644 --- a/src/components/Root/constants.js +++ b/src/components/Root/constants.js @@ -47,3 +47,17 @@ export const RTR_IDLE_SESSION_TTL = '60m'; * value must be a string parsable by ms() */ export const RTR_IDLE_MODAL_TTL = '1m'; + +/** + * When resuming an existing session but there is no token-expiration + * data in the session, we can't properly schedule RTR. + * 1. the real expiration data is in the cookie, but it's HTTPOnly + * 2. the resume-session API endpoint, _self, doesn't include + * token-expiration data in its response + * 3. the session _should_ contain a value, but maybe the session + * was corrupt. + * Given the resume-session API call succeeded, we know the AT must have been + * valid at the time, so we punt and schedule rotation in the future by this + * (relatively short) interval. + */ +export const RTR_AT_EXPIRY_IF_UNKNOWN = '10s'; diff --git a/src/loginServices.js b/src/loginServices.js index 58774458c..841bd4ea4 100644 --- a/src/loginServices.js +++ b/src/loginServices.js @@ -762,7 +762,9 @@ export function validateUser(okapiUrl, store, tenant, session) { /** * checkOkapiSession - * 1. Pull the session from local storage; if non-empty validate it, dispatching load-resources actions. + * 1. Pull the session from local storage; if it contains a user id, + * validate it by fetching /_self to verify that it is still active, + * dispatching load-resources actions. * 2. Check if SSO (SAML) is enabled, dispatching check-sso actions * 3. dispatch set-okapi-ready. * @@ -773,7 +775,7 @@ export function validateUser(okapiUrl, store, tenant, session) { export function checkOkapiSession(okapiUrl, store, tenant) { getOkapiSession() .then((sess) => { - return sess !== null ? validateUser(okapiUrl, store, tenant, sess) : null; + return sess?.user?.id ? validateUser(okapiUrl, store, tenant, sess) : null; }) .then(() => { if (store.getState().discovery?.interfaces?.['login-saml']) {