-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Managed Identity: ResponseHandler can now accept an ISO 8601 date in the ServerAuthorizationTokenResponse's "expires_in" value #7529
base: dev
Are you sure you want to change the base?
Changes from 5 commits
9535ebe
218077e
b3429bf
df6af37
7bb7be1
ee34178
1ac790d
9abe753
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 |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"type": "patch", | ||
"comment": "Managed Identity: ResponseHandler can now accept an ISO 8601 date in the ServerAuthorizationTokenResponse expires_in value #7529", | ||
"packageName": "@azure/msal-common", | ||
"email": "[email protected]", | ||
"dependentChangeType": "patch" | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -52,6 +52,7 @@ import { | |||||
updateAccountTenantProfileData, | ||||||
} from "../account/AccountInfo.js"; | ||||||
import * as CacheHelpers from "../cache/utils/CacheHelpers.js"; | ||||||
import { isIso8601 } from "../utils/TimeUtils.js"; | ||||||
|
||||||
function parseServerErrorNo( | ||||||
serverResponse: ServerAuthorizationCodeResponse | ||||||
|
@@ -425,6 +426,30 @@ export class ResponseHandler { | |||||
); | ||||||
} | ||||||
|
||||||
/** | ||||||
* Calculates the number of seconds until the token expires. | ||||||
* | ||||||
* @param reqTimestamp - The timestamp when the request was made, in seconds. | ||||||
* @param tokenExpirationTimestamp - The expiration timestamp of the token, which can be a number (or a number as a string), a string in ISO 8601 format, or undefined. | ||||||
* @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( | ||||||
reqTimestamp: number, | ||||||
tokenExpirationTimestamp: number | string | undefined, | ||||||
returnValueIfUndefined: number | undefined | ||||||
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. Do we need this as a param if we only ever pass 0? 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. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think even for those the fallback would be 0, no? 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. refreshOnSeconds (derived from |
||||||
): number | undefined { | ||||||
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.
Suggested change
|
||||||
if (typeof tokenExpirationTimestamp === "string") { | ||||||
return isIso8601(tokenExpirationTimestamp) | ||||||
? Math.floor( | ||||||
new Date(tokenExpirationTimestamp).getTime() / 1000 | ||||||
) - reqTimestamp | ||||||
: parseInt(tokenExpirationTimestamp, 10); | ||||||
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. minus reqTimestamp? 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. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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); |
||||||
} else { | ||||||
return tokenExpirationTimestamp || returnValueIfUndefined; | ||||||
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. minus reqTimestamp? |
||||||
} | ||||||
} | ||||||
|
||||||
/** | ||||||
* Generates CacheRecord | ||||||
* @param serverTokenResponse | ||||||
|
@@ -486,23 +511,26 @@ export class ResponseHandler { | |||||
|
||||||
/* | ||||||
* Use timestamp calculated before request | ||||||
* Server may return timestamps as strings, parse to numbers if so. | ||||||
* Server may return timestamps as an ISO 8601 date string or a numeric string, parse to numbers if so. | ||||||
*/ | ||||||
const expiresIn: number = | ||||||
(typeof serverTokenResponse.expires_in === "string" | ||||||
? parseInt(serverTokenResponse.expires_in, 10) | ||||||
: serverTokenResponse.expires_in) || 0; | ||||||
const expiresIn: number = this.getSecondsUntilTokenExpires( | ||||||
reqTimestamp, | ||||||
serverTokenResponse.expires_in, | ||||||
0 | ||||||
) as number; | ||||||
const tokenExpirationSeconds = reqTimestamp + expiresIn; | ||||||
|
||||||
const extExpiresIn: number = | ||||||
(typeof serverTokenResponse.ext_expires_in === "string" | ||||||
? parseInt(serverTokenResponse.ext_expires_in, 10) | ||||||
: serverTokenResponse.ext_expires_in) || 0; | ||||||
const extendedTokenExpirationSeconds = | ||||||
tokenExpirationSeconds + extExpiresIn; | ||||||
|
||||||
const refreshIn: number | undefined = | ||||||
(typeof serverTokenResponse.refresh_in === "string" | ||||||
? parseInt(serverTokenResponse.refresh_in, 10) | ||||||
: serverTokenResponse.refresh_in) || undefined; | ||||||
const tokenExpirationSeconds = reqTimestamp + expiresIn; | ||||||
const extendedTokenExpirationSeconds = | ||||||
tokenExpirationSeconds + extExpiresIn; | ||||||
const refreshOnSeconds = | ||||||
refreshIn && refreshIn > 0 | ||||||
? reqTimestamp + refreshIn | ||||||
|
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.
Can you move this to TimeUtils as a pure function?