initial commit for adding aadIssuerValidatorOptions #696
Conversation
| /// Sets the name of the HttpClientFactory to use with the configuration manager. | ||
| /// Needed when setting up a proxy. | ||
| /// </summary> | ||
| public string? HttpClientFactoryName { get; set; } = null!; |
There was a problem hiding this comment.
| /// Sets the name of the HttpClientFactory to use with the configuration manager. | |
| /// Needed when setting up a proxy. | |
| /// </summary> | |
| public string? HttpClientFactoryName { get; set; } = null!; | |
| /// Sets the name of the HttpClient to get from the IHttpClientFactory for use with the configuration manager. | |
| /// Needed when customizing the client such as configuring a proxy. | |
| /// </summary> | |
| public string? HttpClientName { get; set; } = null!; |
There was a problem hiding this comment.
@Tratcher: the thing I don't fully get is the proxy configuration is a static property of HttpClient?
But I understand that for the default headers etc ... this is important.
There was a problem hiding this comment.
Yeah, you don't want the static, you want to set the instance property on SocketsHttpHandler when creating the HttpClient.
https://docs.microsoft.com/en-us/dotnet/api/system.net.http.socketshttphandler.proxy?view=netcore-3.1#System_Net_Http_SocketsHttpHandler_Proxy
There was a problem hiding this comment.
@Tratcher i see. makes sense. will add to our documentation when we release.
| new ConfigurationManager<IssuerMetadata>( | ||
| Constants.AzureADIssuerMetadataUrl, | ||
| new IssuerConfigurationRetriever(), | ||
| httpClientFactory?.CreateClient(aadIssuerValidatorOptions?.Value?.HttpClientFactoryName)); |
There was a problem hiding this comment.
What's with all the ?? None of these things should be null when activated from DI.
There was a problem hiding this comment.
that's for the unit test to pass, otherwise i have to configure all the service, etc..., which i might do or leave as an integration test instead.
| @@ -15,9 +17,20 @@ namespace Microsoft.Identity.Web.Resource | |||
| /// </summary> | |||
| internal class MicrosoftIdentityIssuerValidatorFactory | |||
| { | |||
| public MicrosoftIdentityIssuerValidatorFactory( | |||
| IOptions<AadIssuerValidatorOptions> aadIssuerValidatorOptions, | |||
| IHttpClientFactory httpClientFactory) | |||
There was a problem hiding this comment.
Did you need to add a package dependency for this? You at least need to register it in DI.
There was a problem hiding this comment.
no need to add a package dependency. IHttpClientFactory is already used by Microsoft.Identity.Web
There was a problem hiding this comment.
right, what @jmprieur said.
|
@jmprieur i need to do some more testing, so will update accordingly. |
tests/Microsoft.Identity.Web.Test/Resource/HttpClientFactoryTest.cs
Outdated
Show resolved
Hide resolved
#551