-
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
Changes from all 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-browser", | ||
"email": "[email protected]", | ||
"dependentChangeType": "patch" | ||
} |
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 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. Just because it can be doesn't mean it needs to be. |
||||||
): 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 | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,9 +9,9 @@ import { AuthenticationScheme } from "../utils/Constants.js"; | |
* Deserialized response object from server authorization code request. | ||
* - token_type: Indicates the token type value. Can be either Bearer or pop. | ||
* - scope: The scopes that the access_token is valid for. | ||
* - expires_in: How long the access token is valid (in seconds). | ||
* - refresh_in: Duration afer which a token should be renewed, regardless of expiration. | ||
* - expires_in: How long the access token is valid (in seconds, or an ISO 8601 string). | ||
* - ext_expires_in: How long the access token is valid (in seconds) if the server isn't responding. | ||
* - refresh_in: Duration afer which a token should be renewed, regardless of expiration. | ||
* - access_token: The requested access token. The app can use this token to authenticate to the secured resource, such as a web API. | ||
* - refresh_token: An OAuth 2.0 refresh token. The app can use this token acquire additional access tokens after the current access token expires. | ||
* - id_token: A JSON Web Token (JWT). The app can decode the segments of this token to request information about the user who signed in. | ||
|
@@ -31,9 +31,9 @@ export type ServerAuthorizationTokenResponse = { | |
// Success | ||
token_type?: AuthenticationScheme; | ||
scope?: string; | ||
expires_in?: number; | ||
refresh_in?: number; | ||
expires_in?: number | string; // Managed Identity can send an ISO 8601 string instead of a number | ||
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 really need to make changes in this base layer of MSAL? Can't you treat this in the MSI layer and adapt the ISO date to expires_in ? and leave this layer unaffected? |
||
ext_expires_in?: number; | ||
refresh_in?: number; | ||
access_token?: string; | ||
refresh_token?: string; | ||
refresh_token_expires_in?: number; | ||
|
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?