coreos / go-oidc

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

Inherit Provider http client settings in calls #402

Closed p53 closed 8 months ago

p53 commented 9 months ago

Right now when initializing Provider we are passing to it all settings in context, but when doing Token() calls or e.g. Userinfo calls these settings are totally ignored so we have to do double work and again and again set settings before each call which is quite error prone (you would not expect that you have to do it before each call) + tedious, it would be great if these calls by default would take client from provider and in case client is provided directly in context passed to method call take those

e.g. We are setting up provider but those settings are ignored in calls

restyClient := client.RestyClient()
    restyClient.SetTimeout(r.Config.OpenIDProviderTimeout)
    restyClient.SetTLSClientConfig(
        &tls.Config{
            InsecureSkipVerify: r.Config.SkipOpenIDProviderTLSVerify,
        },
    )

    if r.Config.OpenIDProviderProxy != "" {
        restyClient.SetProxy(r.Config.OpenIDProviderProxy)
    }

    ctx := oidc3.ClientContext(context.Background(), restyClient.GetClient())
    var provider *oidc3.Provider
    var err error

    operation := func() error {
        provider, err = oidc3.NewProvider(ctx, r.Config.DiscoveryURL)
ericchiang commented 9 months ago

Hey @p53

The clients and options are passed through contexts to match Go's OAuth 2.0 package, which uses the same context key/value strategy: https://pkg.go.dev/golang.org/x/oauth2#pkg-variables. If you use go-oidc with the oauth2 package (which was the main goal of go-oidc), you have to remember to construct the context anyway for the oauth2 package.

Design for how the contexts should be used has generally been a really big pain point: https://github.com/coreos/go-oidc/issues/214.

In hindsight I wouldn't use the context keys for something like HTTP Client configuration. But for now, I don't think I want to be silently inferring values or anything clever. So I'm inclined to add some documentation or examples rather than change the context logic (again).

p53 commented 9 months ago

from point of maintainer i understand, usually making too many clever things leads to bugs, but from point of user sad, it is pain for me, already made several times mistake because of forgetting about this, adding any values to context was not good idea anyway, usually misused as always when you make some "global" var available, ok thx anyway :)