-
Notifications
You must be signed in to change notification settings - Fork 26
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
base: master
Are you sure you want to change the base?
Changes from 10 commits
9eb247e
8313e48
7825038
b1dacce
89629a2
cbd03ca
6e85a91
8fb5642
5c3b6a8
a957eb8
7c63eef
f013931
50c1291
46b503f
51ecc01
9b13e4a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
|
@@ -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, | ||
|
@@ -77,10 +77,27 @@ const OKAPI_FETCH_OPTIONS = { | |
}; | ||
|
||
export class FFetch { | ||
constructor({ logger, store, rtrConfig }) { | ||
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); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
/** | ||
|
@@ -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 | ||
|
@@ -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)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's that, or reassigning the import ( I can:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are two things happening here with the RTR config:
As you noted, we can get around (1) by updating FFetch to leverage stripes-config instead, and call Better, worse, just different but not actually better? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Importing stripes-config for What we can (and should) do is remove the |
||
|
||
this.ffetch = new FFetch({ | ||
logger: this.props.logger, | ||
store, | ||
rtrConfig, | ||
}); | ||
this.ffetch.registerEventListener(); | ||
this.ffetch.replaceFetch(); | ||
this.ffetch.replaceXMLHttpRequest(); | ||
} | ||
|
@@ -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({ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does rely on not calling |
||
|
||
const renderList = []; | ||
|
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.