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

Allow to override the http.Client for RequestToken and AccessToken #49

Merged
merged 3 commits into from Jun 9, 2021

Conversation

fdcds
Copy link
Contributor

@fdcds fdcds commented May 17, 2021

Before, the http.DefaultClient was always used. This is e.g. not
sufficient, if the server being authenticated against uses TLS
certificates signed by a custom root CA. In this case, a custom
http.Client needs to be passed.

To avoid passing arbitrary hidden objects through the context,
as can be seen sometimes in similar situations, the client
is passed through the Config struct instead.

@fdcds fdcds changed the title Allow to override the http.Client Allow to override the http.Client for RequestToken and AccessToken May 17, 2021
Before, the `http.DefaultClient` was always used.  This is e.g. not
sufficient, if the server being authenticated against uses TLS
certificates signed by a custom root CA.  In this case, a custom
`http.Client` needs to be passed.

This change follows the design of https://github.com/golang/oauth2
and allows passing the HTTP client via `context.Context`.

Since the user of a function that accepts a `context.Context` would
likely expect it also to be used for HTTP calls, we also pass it to
`http.NewRequest`.
@fdcds fdcds force-pushed the ds/allow-override-httpclient branch from 0fc458e to 75b456f Compare May 17, 2021 16:43
@dghubble
Copy link
Owner

Thanks, but no thanks.

I've never liked how oauth2 uses the context for a client and I also wouldn't change the API for a case like this. If you have this situation, it's probably best you handle it on your own.

@fdcds
Copy link
Contributor Author

fdcds commented May 17, 2021

I've never liked how oauth2 uses the context for a client and I also wouldn't change the API for a case like this. If you have this situation, it's probably best you handle it on your own.

I agree with you that the way oauth2 passes a http.Client through the context does not spark much joy, to put it mildly. Do you have a suggestion how to do it better that would be acceptable to you? AFAIK the general recommendation is also to not use the http.DefaultClient, so that option also seems not a good solution either.

@dghubble
Copy link
Owner

A more ordinary approach is just setting a Client as a Config field and continue defaulting to http.DefaultClient on use. Or just do any approach on your own, where there is no need for compatibility and I'll call this out of scope.

@fdcds
Copy link
Contributor Author

fdcds commented May 18, 2021

A more ordinary approach is just setting a Client as a Config field and continue defaulting to http.DefaultClient on use.

Thanks. I implemented that in 37256b9.

Or just do any approach on your own, where there is no need for compatibility and I'll call this out of scope.

That would mean having to fork this fine library, which would be unfortunate. I would rather prefer to find a solution that is sound from an engineering point of view and merge it into your library.

@dghubble dghubble merged commit 3095820 into dghubble:master Jun 9, 2021
@fdcds fdcds deleted the ds/allow-override-httpclient branch September 6, 2021 14:37
bvuong pushed a commit to fuse-inventory/oauth1 that referenced this pull request Sep 16, 2021
…hubble#49)

* Allow using a custom http.Client for RequestToken and AccessToken. This may be useful when
authenticating against a endpoints that use a custom root CA
* Continue to default to `http.DefaultClient`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants