coreos / go-oidc

A Go OpenID Connect client.
Apache License 2.0
1.92k stars 393 forks source link

Add access to Provider client from the provider #413

Closed nathenapse closed 6 months ago

nathenapse commented 6 months ago

This is helpful when we want to call Exchange with the client we provided to the Provider

ericchiang commented 6 months ago

I don't see a strong reason the Provider struct should allow accessing it's internal fields like this. If you need to use the same http.Client as the Provider, use the http.Client you passed to the Provider in the first place?

nathenapse commented 6 months ago

Let me explain my reasoning behind it. When passing the *http.client to the NewProvider() I expected it to also affect the Exchange() but that was not the case. So if we also need to pass the *http.client to the Exchange() it would be better if we override the Exchange() method to pass the http.client found in Provider. Or have a way to access it and pass it to Exchange(). Clearly I chose the second method but the first option is also something I can work on. What do you think?

ericchiang commented 6 months ago

Ah. Would you take a look at my comment here https://github.com/coreos/go-oidc/issues/402#issuecomment-1847505771. That goes over some other unexpected behavior with passing config through contexts.

This is unfortunate, but I think the answer remains that you should hold on to the http.Client configuration and reuse it between calls. Having multiple ways to get at the client doesn't seem like it's adding much at the cost of more API surface

nathenapse commented 6 months ago

That means if we have a custom *http.client we have to remember to pass it everytime? Wouldn't that be prone to errors? and sometimes tls configurations are production specific.

ericchiang commented 6 months ago

This is the case for all values passed as context keys. If you use a context, it needs to be configured correctly before passing it to your method. E.g. if you use golang.org/x/oauth2, you also need to do the same.

Again, as I mentioned in #402, if we're going to reduce confusion, we should allow HTTP client configuration without using contexts. I don't think this PR is the correct way to improve things, so going to close out