From e2b6a09ddc12224dd0dcc1c4632b84dbbfe8768e Mon Sep 17 00:00:00 2001 From: Zak Burke Date: Mon, 10 Jun 2024 15:21:33 -0400 Subject: [PATCH] STCOR-776 optionally schedule RTR based on session data (#1488) Not all authentication-related responses are created equal. If the response contains AT expiration data, i.e. it is a response from `/bl-users/login-with-expiry`, use that data to configure RTR. If the response does not contain such data, i.e. it is a response from `/bl-users/_self`, pull AT expiration data from the session and use that. If we still don't have any AT expiration data, cross your fingers and try rotating in 10 seconds. Previously, we did not ever check the session, so RTR always fired 10 seconds into a resumed, which is exactly the situation faced in e2e tests. This doesn't precisely handle the issue faced by e2e tests that stall when the rtr requests fires, but leveraging the session data should push the rtr request far enough into the future that most e2e tests will now avoid it. We should still try to understand that problem and solve it, but in the mean time this may enable us to work around it. Refs STCOR-776 (cherry picked from commit e93a5af762786e53e9f4bc0aa55a8d47ff6b91cf) --- src/components/Root/FFetch.js | 59 ++++++++--- src/components/Root/FFetch.test.js | 163 +++++++++++++++++++++++++++++ src/components/Root/constants.js | 14 +++ src/loginServices.js | 6 +- 4 files changed, 224 insertions(+), 18 deletions(-) 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']) {