Managed Identity: ResponseHandler can now accept an ISO 8601 date in the ServerAuthorizationTokenResponse's "expires_in" value #7529
Conversation
github-actions
bot
added
documentation
Robbie-Microsoft
requested review from
sameerag,
tnorling,
hectormmg,
jo-arroyo,
peterzenz,
konstantin-msft,
lalimasharda and
shylasummers
as code owners
| * @param returnValueIfUndefined - The value to return if the tokenExpirationTimestamp is undefined. | ||
| * @returns The number of seconds until the token expires, or the returnValueIfUndefined if the tokenExpirationTimestamp is undefined. | ||
| */ | ||
| private getSecondsUntilTokenExpires( |
There was a problem hiding this comment.
Can you move this to TimeUtils as a pure function?
| private getSecondsUntilTokenExpires( | ||
| reqTimestamp: number, | ||
| tokenExpirationTimestamp: number | string | undefined, | ||
| returnValueIfUndefined: number | undefined |
There was a problem hiding this comment.
Do we need this as a param if we only ever pass 0?
There was a problem hiding this comment.
I added that in there because I originally coded this PR to also handle (future-proof?) ext_expires_in, refresh_in, and refresh_token_expires_in. However, I'm not sure if those are necessary, and if they are, maybe it should be another PR. I've posed this question to Bogdan and Gladwin and am awaiting feedback.
There was a problem hiding this comment.
I think even for those the fallback would be 0, no?
There was a problem hiding this comment.
refreshOnSeconds (derived from refresh_in) can be passed as undefined to CacheHelpers.createAccessTokenEntity, and rtExpiresOn (derived from refresh_token_expires_in) can be passed as undefined to CacheHelpers.createRefreshTokenEntity.
| reqTimestamp: number, | ||
| tokenExpirationTimestamp: number | string | undefined, | ||
| returnValueIfUndefined: number | undefined | ||
| ): number | undefined { |
There was a problem hiding this comment.
| ): number | undefined { | |
| ): number { |
| ? Math.floor( | ||
| new Date(tokenExpirationTimestamp).getTime() / 1000 | ||
| ) - reqTimestamp | ||
| : parseInt(tokenExpirationTimestamp, 10); |
There was a problem hiding this comment.
Sorry, I don't understand what you're asking.
But to explain this: If the expires_in value is an ISO 8601 string, it's in the future - say 3599 seconds (an hour minus 1 second). This equation will subtract the current time, as of the moment of the request, from the ISO 8601 string and end up with what's left (3599 seconds).
There was a problem hiding this comment.
I'm confused by the fact that we seem to be mixing and matching expiresAT with expiresIN values. It's not clear what these variables represent. We have a variable called tokenExpirationTimestamp which suggests it represents expiresAT and would need a subtraction of reqTimestamp in order to obtain expiresIN but we're only doing that subtraction if the value is ISO8601 format otherwise treating it as if it's already expiresIN.
This might be poor API design on the server side if they're sending back values that represent different things on the same parameter but if that's the case we should write our logic in way that clears up this confusion. e.g.
const expiresIn = isIso8601(serverResponse.expires_in) ? getExpiresInFromExpiresAt(serverResponse.expires_in, reqTimestamp) : serverResponse.expires_in || 0;
const expiresAt = isIso8601(serverResponse.expires_in) ? serverResponse.expiresIn : getExpiresAtFromExpiresIn(serverResponse.expires_in, reqTimestamp);
| ) - reqTimestamp | ||
| : parseInt(tokenExpirationTimestamp, 10); | ||
| } else { | ||
| return tokenExpirationTimestamp || returnValueIfUndefined; |
Fixes #7393