Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[STCOR-888] RTR dynamic configuration, debugging event #1535

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
1 change: 1 addition & 0 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ export { userLocaleConfig } from './src/loginServices';
export * from './src/consortiaServices';
export { default as queryLimit } from './src/queryLimit';
export { default as init } from './src/init';
export * as RTR_CONSTANTS from './src/components/Root/constants';

/* localforage wrappers hide the session key */
export { getOkapiSession, getTokenExpiry, setTokenExpiry } from './src/loginServices';
Expand Down
31 changes: 24 additions & 7 deletions src/components/Root/FFetch.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
*/

import ms from 'ms';
import { okapi as okapiConfig } from 'stripes-config';
import { okapi as okapiConfig, config } from 'stripes-config';
import {
setRtrTimeout,
setRtrFlsTimeout,
Expand All @@ -62,8 +62,8 @@ import {
} from './Errors';
import {
RTR_AT_EXPIRY_IF_UNKNOWN,
RTR_AT_TTL_FRACTION,
RTR_ERROR_EVENT,
RTR_FORCE_REFRESH_EVENT,
RTR_FLS_TIMEOUT_EVENT,
RTR_TIME_MARGIN_IN_MS,
RTR_FLS_WARNING_EVENT,
Expand All @@ -77,10 +77,27 @@ const OKAPI_FETCH_OPTIONS = {
};

export class FFetch {
constructor({ logger, store, rtrConfig }) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RTR config is exposed as part of the main stripes-config. Why should FFetch get passed it in a non-standard way?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strikes me as 'better testability' if you're able to just pass the config in vs having to mock it out/provide different mocks through some other way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair critique. @JohnC-80, do you remember if there is a particular reason we did it this way? My bet is that we were thinking "We need to pass it through the constructor because this is a regular class, not a React component, so it won't have access to the stripes object" which is true, but Noah is correct that since we can just grab the import ... we might as well just grab the import.

constructor({ logger, store }) {
this.logger = logger;
this.store = store;
this.rtrConfig = rtrConfig;
}

/**
* registers a listener for the RTR_FORCE_REFRESH_EVENT
*/
registerEventListener = () => {
this.globalEventCallback = () => {
this.logger.log('rtr', 'forcing rotation due to RTR_FORCE_REFRESH_EVENT');
rtr(this.nativeFetch, this.logger, this.rotateCallback, this.store.getState().okapi);
};
window.addEventListener(RTR_FORCE_REFRESH_EVENT, this.globalEventCallback);
}

/**
* unregister the listener for the RTR_FORCE_REFRESH_EVENT
*/
unregisterEventListener = () => {
window.removeEventListener(RTR_FORCE_REFRESH_EVENT, this.globalEventCallback);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All our other event handlers are consolidated within SessionEventContainer. Can we move this there too? If not why not?

Functionally it should not matter either way, but it would be nice to keep similar things in the same place.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I don't think this can be consolidated; this handler specifically needs access to those callbacks and fields within FFetch, which SessionEventContainer has no way to access


/**
Expand Down Expand Up @@ -112,11 +129,11 @@ export class FFetch {
scheduleRotation = (rotationP) => {
rotationP.then((rotationInterval) => {
// AT refresh interval: a large fraction of the actual AT TTL
const atInterval = (rotationInterval.accessTokenExpiration - Date.now()) * RTR_AT_TTL_FRACTION;
const atInterval = (rotationInterval.accessTokenExpiration - Date.now()) * config.rtr.rotationIntervalFraction;

// RT timeout interval (session will end) and warning interval (warning that session will end)
const rtTimeoutInterval = (rotationInterval.refreshTokenExpiration - Date.now());
const rtWarningInterval = (rotationInterval.refreshTokenExpiration - Date.now()) - ms(this.rtrConfig.fixedLengthSessionWarningTTL);
const rtWarningInterval = (rotationInterval.refreshTokenExpiration - Date.now()) - ms(config.rtr.fixedLengthSessionWarningTTL);

// schedule AT rotation IFF the AT will expire before the RT. this avoids
// refresh-thrashing near the end of the FLS with progressively shorter
Expand All @@ -132,7 +149,7 @@ export class FFetch {
}

// schedule FLS end-of-session warning
this.logger.log('rtr-fls', `end-of-session warning at ${new Date(rotationInterval.refreshTokenExpiration - ms(this.rtrConfig.fixedLengthSessionWarningTTL))}`);
this.logger.log('rtr-fls', `end-of-session warning at ${new Date(rotationInterval.refreshTokenExpiration - ms(config.rtr.fixedLengthSessionWarningTTL))}`);
this.store.dispatch(setRtrFlsWarningTimeout(setTimeout(() => {
this.logger.log('rtr-fls', 'emitting RTR_FLS_WARNING_EVENT');
window.dispatchEvent(new Event(RTR_FLS_WARNING_EVENT));
Expand Down
56 changes: 39 additions & 17 deletions src/components/Root/FFetch.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,15 @@
/* eslint-disable no-unused-vars */

import ms from 'ms';
import { waitFor } from '@testing-library/react';
import { okapi } from 'stripes-config';

import { getTokenExpiry } from '../../loginServices';
import { FFetch } from './FFetch';
import { RTRError, UnexpectedResourceError } from './Errors';
import {
RTR_AT_EXPIRY_IF_UNKNOWN,
RTR_AT_TTL_FRACTION,
RTR_FORCE_REFRESH_EVENT,
RTR_FLS_WARNING_TTL,
RTR_TIME_MARGIN_IN_MS,
} from './constants';
Expand All @@ -26,6 +28,12 @@ jest.mock('stripes-config', () => ({
okapi: {
url: 'okapiUrl',
tenant: 'okapiTenant'
},
config: {
rtr: {
rotationIntervalFraction: 0.5,
fixedLengthSessionWarningTTL: '1m',
}
}
}),
{ virtual: true });
Expand All @@ -34,13 +42,18 @@ const log = jest.fn();

const mockFetch = jest.fn();

// to ensure we cleanup after each test
const instancesWithEventListeners = [];

describe('FFetch class', () => {
beforeEach(() => {
global.fetch = mockFetch;
getTokenExpiry.mockResolvedValue({
atExpires: Date.now() + (10 * 60 * 1000),
rtExpires: Date.now() + (10 * 60 * 1000),
});
instancesWithEventListeners.forEach(instance => instance.unregisterEventListener());
instancesWithEventListeners.length = 0;
});

afterEach(() => {
Expand Down Expand Up @@ -153,6 +166,23 @@ describe('FFetch class', () => {
});
});

describe('force refresh event', () => {
it('Invokes a refresh on RTR_FORCE_REFRESH_EVENT...', async () => {
mockFetch.mockResolvedValueOnce('okapi success');

const instance = new FFetch({ logger: { log }, store: { getState: () => ({ okapi }) } });
instance.replaceFetch();
instance.replaceXMLHttpRequest();

instance.registerEventListener();
instancesWithEventListeners.push(instance);

window.dispatchEvent(new Event(RTR_FORCE_REFRESH_EVENT));

await waitFor(() => expect(mockFetch.mock.calls).toHaveLength(1));
});
});

describe('calling authentication resources', () => {
it('handles RTR data in the response', async () => {
// a static timestamp representing "now"
Expand Down Expand Up @@ -185,9 +215,7 @@ describe('FFetch class', () => {
store: {
dispatch: jest.fn(),
},
rtrConfig: {
fixedLengthSessionWarningTTL: '1m',
},

});
testFfetch.replaceFetch();
testFfetch.replaceXMLHttpRequest();
Expand All @@ -201,7 +229,7 @@ describe('FFetch class', () => {
await setTimeout(Promise.resolve(), 2000);

// AT rotation
expect(st).toHaveBeenCalledWith(expect.any(Function), (accessTokenExpiration - whatTimeIsItMrFox) * RTR_AT_TTL_FRACTION);
expect(st).toHaveBeenCalledWith(expect.any(Function), (accessTokenExpiration - whatTimeIsItMrFox) * 0.5);
ncovercash marked this conversation as resolved.
Show resolved Hide resolved

// FLS warning
expect(st).toHaveBeenCalledWith(expect.any(Function), (refreshTokenExpiration - whatTimeIsItMrFox) - ms(RTR_FLS_WARNING_TTL));
Expand Down Expand Up @@ -241,9 +269,7 @@ describe('FFetch class', () => {
store: {
dispatch: jest.fn(),
},
rtrConfig: {
fixedLengthSessionWarningTTL: '1m',
},

});
testFfetch.replaceFetch();
testFfetch.replaceXMLHttpRequest();
Expand All @@ -255,7 +281,7 @@ describe('FFetch class', () => {
// 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);
expect(st).toHaveBeenCalledWith(expect.any(Function), (atExpires - whatTimeIsItMrFox) * 0.5);
});

it('handles missing RTR data', async () => {
Expand All @@ -279,9 +305,7 @@ describe('FFetch class', () => {
store: {
dispatch: jest.fn(),
},
rtrConfig: {
fixedLengthSessionWarningTTL: '1m',
},

});
testFfetch.replaceFetch();
testFfetch.replaceXMLHttpRequest();
Expand All @@ -294,7 +318,7 @@ describe('FFetch class', () => {
// into a separate thread, I'm not 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) * RTR_AT_TTL_FRACTION);
expect(st).toHaveBeenCalledWith(expect.any(Function), ms(RTR_AT_EXPIRY_IF_UNKNOWN) * 0.5);
});

it('handles unsuccessful responses', async () => {
Expand Down Expand Up @@ -358,9 +382,7 @@ describe('FFetch class', () => {
store: {
dispatch: jest.fn(),
},
rtrConfig: {
fixedLengthSessionWarningTTL: '1m',
},

});
testFfetch.replaceFetch();
testFfetch.replaceXMLHttpRequest();
Expand All @@ -374,7 +396,7 @@ describe('FFetch class', () => {
await setTimeout(Promise.resolve(), 2000);

// AT rotation
expect(st).not.toHaveBeenCalledWith(expect.any(Function), (accessTokenExpiration - whatTimeIsItMrFox) * RTR_AT_TTL_FRACTION);
expect(st).not.toHaveBeenCalledWith(expect.any(Function), (accessTokenExpiration - whatTimeIsItMrFox) * 0.5);

// FLS warning
expect(st).toHaveBeenCalledWith(expect.any(Function), (refreshTokenExpiration - whatTimeIsItMrFox) - ms(RTR_FLS_WARNING_TTL));
Expand Down
7 changes: 5 additions & 2 deletions src/components/Root/Root.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,13 +67,15 @@ class Root extends Component {
// * configure fetch and xhr interceptors to conduct RTR
// * see SessionEventContainer for RTR handling
if (this.props.config.useSecureTokens) {
const rtrConfig = configureRtr(this.props.config.rtr);
// FFetch relies on some of these properties, so we must ensure
// they are filled before initialization
this.props.config.rtr = configureRtr(this.props.config.rtr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reassigning something on props feels icky. Why was it necessary to change this line?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's that, or reassigning the import (configureRtr doesn't change the input). This was changed because my FFetch imported config via stripes-config, whereas before it was passed in.

I can:

  • revert it to being passed in instead, if preferred, but IMO it's best for the shared config to match the filled-in version from configureRtr (and we already do that in render(), where we have gross: this overwrites whatever is currently stored at config.rtr)
  • make configureRtr mutate its input

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two things happening here with the RTR config:

  1. passing it to FFetch (in the constructor)
  2. passing it Stripes (in render)

As you noted, we can get around (1) by updating FFetch to leverage stripes-config instead, and call configureRtr() in the constructor there. Should we just do the same in render() here? Part of me screams, "You should only have to call that function once! More than once means you're doing it wrong!" The other part of me is like, "Eh, if the value in stripes-config may be sparse, think of configureRtr() as getSafeRtrValues(). Does that still bug ya? No? Great."

Better, worse, just different but not actually better?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Importing stripes-config for render here might make sense, but I'm not familiar enough with the rest of this to know if that's a bad idea.

What we can (and should) do is remove the configureRtr call inside render() here. It just populates the sparse values (as you said), and there's no reason any values should be removed between renders.


this.ffetch = new FFetch({
logger: this.props.logger,
store,
rtrConfig,
});
this.ffetch.registerEventListener();
this.ffetch.replaceFetch();
this.ffetch.replaceXMLHttpRequest();
}
Expand Down Expand Up @@ -135,6 +137,7 @@ class Root extends Component {
// render runs when props change. realistically, that'll never happen
// since config values are read only once from a static file at build
// time, but still, props are props so technically it's possible.
// Also, ui-developer provides facilities to change some of this
config.rtr = configureRtr(this.props.config.rtr);

const stripes = new Stripes({
Expand Down
4 changes: 4 additions & 0 deletions src/components/Root/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ export const RTR_SUCCESS_EVENT = '@folio/stripes/core::RTRSuccess';
/** dispatched during RTR if RTR itself fails */
export const RTR_ERROR_EVENT = '@folio/stripes/core::RTRError';

/** dispatched by ui-developer to force a token rotation */
export const RTR_FORCE_REFRESH_EVENT = '@folio/stripes/core::RTRForceRefresh';

/**
* dispatched if the session is idle (without activity) for too long
*/
Expand Down Expand Up @@ -36,6 +39,7 @@ export const RTR_ACTIVITY_CHANNEL = '@folio/stripes/core::RTRActivityChannel';
* the RT is still good at that point. Since rotation happens in the background
* (i.e. it isn't a user-visible feature), rotating early has no user-visible
* impact.
* overridden in stripes.config.js::config.rtr.rotationIntervalFraction.
*/
export const RTR_AT_TTL_FRACTION = 0.8;

Expand Down
6 changes: 6 additions & 0 deletions src/components/Root/token-util.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { getTokenExpiry, setTokenExpiry } from '../../loginServices';
import { RTRError, UnexpectedResourceError } from './Errors';
import {
RTR_ACTIVITY_EVENTS,
RTR_AT_TTL_FRACTION,
RTR_ERROR_EVENT,
RTR_FLS_WARNING_TTL,
RTR_IDLE_MODAL_TTL,
Expand Down Expand Up @@ -322,6 +323,11 @@ export const configureRtr = (config = {}) => {
conf.idleModalTTL = RTR_IDLE_MODAL_TTL;
}

// what fraction of the way through the session should we rotate?
if (!conf.rotationIntervalFraction) {
conf.rotationIntervalFraction = RTR_AT_TTL_FRACTION;
}

// what events constitute activity?
if (isEmpty(conf.activityEvents)) {
conf.activityEvents = RTR_ACTIVITY_EVENTS;
Expand Down
30 changes: 16 additions & 14 deletions src/components/Root/token-util.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -342,19 +342,21 @@ describe('getPromise', () => {
});

describe('configureRtr', () => {
it('sets idleSessionTTL and idleModalTTL', () => {
const res = configureRtr({});
expect(res.idleSessionTTL).toBe('60m');
expect(res.idleModalTTL).toBe('1m');
});

it('leaves existing settings in place', () => {
const res = configureRtr({
idleSessionTTL: '5m',
idleModalTTL: '5m',
});

expect(res.idleSessionTTL).toBe('5m');
expect(res.idleModalTTL).toBe('5m');
it.each([
[
{},
{ idleSessionTTL: '60m', idleModalTTL: '1m', rotationIntervalFraction: 0.8, activityEvents: ['keydown', 'mousedown'] }
],
[
{ idleSessionTTL: '1s', idleModalTTL: '2m' },
{ idleSessionTTL: '1s', idleModalTTL: '2m', rotationIntervalFraction: 0.8, activityEvents: ['keydown', 'mousedown'] }
],
[
{ idleSessionTTL: '1s', idleModalTTL: '2m', rotationIntervalFraction: -1, activityEvents: ['cha-cha-slide'] },
{ idleSessionTTL: '1s', idleModalTTL: '2m', rotationIntervalFraction: -1, activityEvents: ['cha-cha-slide'] }
],
])('sets default values as applicable', (config, expected) => {
const res = configureRtr(config);
expect(res).toMatchObject(expected);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@ const SessionEventContainer = ({ history }) => {
// We only want to configure the event listeners once, not every time
// there is a change to stripes or history. Hence, an empty dependency
// array.
// should `stripes.rtr` go here?
}, []); // eslint-disable-line react-hooks/exhaustive-deps
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zburke can you comment on this? As-is, this configuration cannot be modified at runtime by ui-developer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably I started with a non-empty array and it didn't work as expected, so I added the empty array as a crutch and never looked back. Your observation/criticism is fair, but I don't have time to debug this further since the only thing "broken" about it is that it can't be modified at runtime via dev-tools.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested it, and it works beautifully. Only the RTR config in ui-developer causes the effect to re-run, nothing else (not even the other config forms in developer)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does rely on not calling configureRtr on render in Root, though.


const renderList = [];
Expand Down
Loading