Skip to content
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

Add support in DiscoveryCache to HttpClients with BaseAddress specified #43

Open
josephdecock opened this issue Nov 13, 2024 · 0 comments
Labels
area/identity-model Issues related to Identity Model impact/non-breaking The fix or change will not be a breaking one priority/3 Medium

Comments

@josephdecock
Copy link
Member

Migrated from IdentityModel/IdentityModel#599

Original Description

In my application, I have a HttpClientFactory configured to point at the authority

builder.Services.AddHttpClient("idp", http =>
{
    http.BaseAddress = new Uri("https://idp");
});

I want to use that client configuration to request the discovery document from the cache.

Currently all DiscoveryCache constructors require the authority as string but it's not required to have a value if the HttpClient produced by the delegate has a BaseAddress.

The snippet below works, but the null! passed as argument really hurts my eyes.

builder.Services.AddSingleton<IDiscoveryCache>(sp =>
{
    var policy = new DiscoveryPolicy
    {
       // some policy settings
    };
    
    return new DiscoveryCache(null!, () => sp.GetRequiredService<IHttpClientFactory>().CreateClient("idp"), policy);
});

I'm open to work on a PR if there is an agreed-upon solution to work on!
In my application, I have a HttpClientFactory configured to point at the authority

builder.Services.AddHttpClient("idp", http =>
{
    http.BaseAddress = new Uri("https://idp");
});

I want to use that client configuration to request the discovery document from the cache.

Currently all DiscoveryCache constructors require the authority as string but it's not required to have a value if the HttpClient produced by the delegate has a BaseAddress.

The snippet below works, but the null! passed as argument really hurts my eyes.

builder.Services.AddSingleton<IDiscoveryCache>(sp =>
{
    var policy = new DiscoveryPolicy
    {
       // some policy settings
    };
    
    return new DiscoveryCache(null!, () => sp.GetRequiredService<IHttpClientFactory>().CreateClient("idp"), policy);
});

I'm open to work on a PR if there is an agreed-upon solution to work on!

My thoughts

My first thought is to have a constructor overload like this:

public DiscoveryCache(Func<HttpMessageInvoker> httpClientFunc, DiscoveryPolicy? policy = null)

And then _authority's type would change to allow nulls.

The only downside that I see is that it is possible to misuse this API, but we have a runtime check already to ensure that there is a base address, and a helpful error message. We should just make sure to include xmldoc that explains the requirements of the new constructor really clearly.

@josephdecock josephdecock added area/identity-model Issues related to Identity Model impact/non-breaking The fix or change will not be a breaking one priority/3 Medium labels Nov 13, 2024
@Erwinvandervalk Erwinvandervalk added area/access-token-management Issues related to Access Token Management and removed area/access-token-management Issues related to Access Token Management labels Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/identity-model Issues related to Identity Model impact/non-breaking The fix or change will not be a breaking one priority/3 Medium
Projects
None yet
Development

No branches or pull requests

2 participants