Allow applications to bypass network request for OpenID configuration #1578
Conversation
|
made some comments, if we can get those resolved and add error handling + tests lets ship it |
| @@ -483,13 +484,13 @@ export class UserAgentApplication { | |||
| * Helper function to acquireToken | |||
| * | |||
| */ | |||
| private acquireTokenHelper(account: Account, interactionType: InteractionType, isLoginCall: boolean, request: AuthenticationParameters, resolve?: any, reject?: any): void { | |||
| private async acquireTokenHelper(account: Account, interactionType: InteractionType, isLoginCall: boolean, request: AuthenticationParameters, resolve?: any, reject?: any): Promise<void> { | |||
There was a problem hiding this comment.
Does this effect performance for calls that don't need this async?
There was a problem hiding this comment.
Potentially, but should be offset when not making the openid-config call at all. Will verify.
…rary-for-js into authority-metadata-openid-perf
…rary-for-js into authority-metadata-openid-perf
| @@ -514,7 +515,14 @@ export class UserAgentApplication { | |||
| } | |||
| } | |||
|
|
|||
| acquireTokenAuthority.resolveEndpointsAsync(this.telemetryManager, request.correlationId).then(async () => { | |||
| try { | |||
| if (!acquireTokenAuthority.hasCachedMetadata()) { | |||
There was a problem hiding this comment.
Instead of checking for this here, why don't we just do this inside resolveAuthorityAsync()? If the cached metadata is available, just return it, otherwise make a network request (similar to acquireTokenSilent)
There was a problem hiding this comment.
I like this too. Though this would prevent intentional refreshes without an additional forceRefresh parameter. Not sure if there's a valid argument for supporting that use case but something to consider.
There was a problem hiding this comment.
We can't do intentional refreshes anyway with the current code.
There was a problem hiding this comment.
One of the goals is to minimize the number of async code paths, which is why the if is done here, so that way if the check fails, we don't enter an async function. If this was factored out, it would enter the async every time.
There was a problem hiding this comment.
Also, I did not consider if forceRefresh should force a network call for openid configuration, in addition to forcing a network call for a new token. Initially, I would think not. @DarylThayil any opinion here?
| @@ -7,7 +7,12 @@ import { Constants } from "../utils/Constants"; | |||
| import { ClientAuthError } from "./ClientAuthError"; | |||
| import { TelemetryOptions } from "../Configuration"; | |||
|
|
|||
| export const ClientConfigurationErrorMessage = { | |||
| interface IClientConfigurationErrorMessage { | |||
There was a problem hiding this comment.
Why are we making this change for error message classes, and should it be done for all error message classes?
There was a problem hiding this comment.
I felt we should codify the pattern of code/desc in the object below. I would actually complete refactor this into a different pattern, but felt this was an incremental improvement.
…rary-for-js into authority-metadata-openid-perf
…rary-for-js into authority-metadata-openid-perf
The allows applications to bypass the network request to retrieve the OpenID configuration that is normally fetched on every request. This data must be retrieved from the openid configuration endpoint and passed wholesale as a JSON string.
Steps:
https://login.microsoftonline.com/common/discovery/instance?api-version=1.0&authorization_endpoint={authorizeEndpoint}for your authority: https://login.microsoftonline.com/common/discovery/instance?api-version=1.0&authorization_endpoint=https://login.microsoftonline.com/common/oauth2/v2.0/authorizetenant_discovery_endpoint: https://login.microsoftonline.com/common/v2.0/.well-known/openid-configurationA side-effect of this change is that the library will cache the response and use that for every call for the current session, regardless if it was provided in the configuration or retrieved via a network call.
Before:
After:
TODO: