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

Inherit Provider http client settings in calls #402

Closed
p53 opened this issue Dec 8, 2023 · 2 comments
Closed

Inherit Provider http client settings in calls #402

p53 opened this issue Dec 8, 2023 · 2 comments

Comments

@p53
Copy link

p53 commented Dec 8, 2023

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)
@p53 p53 changed the title Inherit Provider ctx settings in calls Inherit Provider http client settings in calls Dec 8, 2023
@ericchiang
Copy link
Collaborator

ericchiang commented Dec 8, 2023

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: #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
Copy link
Author

p53 commented Dec 8, 2023

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants