-
Notifications
You must be signed in to change notification settings - Fork 68
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 Authority to ClientCredentials options #102
Conversation
If authority is set, we use it to retrieve the discovery document, and use that to configure the token endpoint. Because this is an async operation, we have a new abstraction for retrieval of the token endpoint
var descriptor = new SecurityTokenDescriptor | ||
{ | ||
Issuer = options.ClientId, | ||
Audience = options.TokenEndpoint, | ||
Audience = await _tokenEndpoint.GetAsync(options), |
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.
For most usage, you just set the authority and let discovery happen in the background. But sometimes you do need to know what the token endpoint is - for example, here when we make a client assertion, we need to use the token endpoint as the audience. This is slightly more complex than just reading the option as we used to - notably this uses a new service and is an async operation. I don't think we can really avoid this though, because invoking the discovery endpoint *is async. At one point, we talked about doing this in a PostConfigureOptions, but that doesn't support asynchronicity.
{ | ||
if (!_caches.ContainsKey(authority)) | ||
{ | ||
_caches[authority] = new DiscoveryCache(authority); |
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.
I think we should support DiscoveryPolicy and Timeout here.
Hmm.... ok, so do you think this is too complicated or problematic to pursue? IOW, is this worth all the new goo? |
I'm unsure ... frankly was hoping for strong reactions to this PR 😆 It does feel kind of unnecessary, especially if I add discovery policy and cache timeout. @leastprivilege, @AndersAbel, what do you guys think? |
I think it is not the concern of this library to do discovery... |
Ok, then let's close this and DuendeSoftware/foss#49. |
At least not for this round of updates... |
Okay, I'll leave this as a draft PR for the future |
@josephdecock Will need to re-open this PR against the foss repo, if one want's to keep it. Closing as we're archiving this repository. |
Adds a new authority option. If it is set, we use it to retrieve the discovery document, and use that to configure the token endpoint. Because this is an async operation, we have a new abstraction for retrieval of the token endpoint
Closes DuendeSoftware/foss#49