NewClient documentation possibly contradicts implementation

[stephens2424]

trafficstars

The documentation on NewClient states:

Note that if a custom *http.Client is provided via the Context it is used only for token acquisition and is not used to configure the *http.Client returned from NewClient.

However, the implementation looks like it does use the Context-provided client as the underlying round tripper for the non-token-fetching requests. In fact, since the TokenSource is hardcoded as a ReuseTokenSource, it doesn't seem like the context-provided *http.Client is used at all for token acquisition.

Am I totally misreading this? Or is that contradictory, and thus should the documentation (or implementation) change?

If it's not contradictory, *Config.NewClient, doesn't seem to allow for configuration of the underlying transport for non-token-exchange requests. Its documentation even states The returned client and its Transport should not be modified. , so it's unclear how I would configure a proxy server for the non-token-exchange requests, other than, perhaps, configuring http.DefaultClient.

[adamdecaf]

You can try overriding the client. The context key is a bit hidden, but https://github.com/golang/oauth2/issues/321 talks about that.

[org0000h]

The documentation on NewClient states:

Note that if a custom *http.Client is provided via the Context it is used only for token acquisition and is not used to configure the *http.Client returned from NewClient.

However, the implementation looks like it does use the Context-provided client as the underlying round tripper for the non-token-fetching requests. In fact, since the TokenSource is hardcoded as a ReuseTokenSource, it doesn't seem like the context-provided *http.Client is used at all for token acquisition.

Am I totally misreading this? Or is that contradictory, and thus should the documentation (or implementation) change?

If it's not contradictory, *Config.NewClient, doesn't seem to allow for configuration of the underlying transport for non-token-exchange requests. Its documentation even states The returned client and its Transport should not be modified. , so it's unclear how I would configure a proxy server for the non-token-exchange requests, other than, perhaps, configuring http.DefaultClient.

same question

[levanidev]

I recently opened a similar issue #564 not realizing this has been an open question for 2 years lol.

what should we do if we want to configure client that makes non-token-exchange requests ? can i modify the transport ?

I need to have token exchange transport somehow injected in this:

func NewClient() *http.Client {
	return &http.Client{
		Timeout: time.Second * 60,
		Transport: &http.Transport{
			Proxy: http.ProxyFromEnvironment,
			DialContext: (&net.Dialer{
				Timeout:   30 * time.Second,
				KeepAlive: 30 * time.Second,
			}).DialContext,
			ForceAttemptHTTP2:     true,
			MaxIdleConns:          100,
			MaxConnsPerHost:       100,
			IdleConnTimeout:       90 * time.Second,
			TLSHandshakeTimeout:   10 * time.Second,
			ExpectContinueTimeout: 1 * time.Second,
		},
	}
}

[mitar]

I agree, that whole section is invalid. Config.Client (which calls into NewClient) has better documentation:

// Client returns an HTTP client using the provided token.
// The token will auto-refresh as necessary. The underlying
// HTTP transport will be obtained using the provided context.
// The returned client and its Transport should not be modified.

And in fact Config.Client is the one which uses context-provided client inside tokenRefresher to refresh tokens. But it also sets the underlying HTTP transport (because that is done by NewClient). So my proposal would be to change that section to:

The underlying HTTP transport will be obtained using the provided context.
The returned client and its Transport should not be modified.

In fact, the Config.Client's documentation should maybe include a line stating that context-provided client will also be used for token refreshing.

Hashicorp uses oauth2.HTTPClient to configure the underlying client to cleanhttp.DefaultClient() (cleanhttp is probably what you want to use if you want to fix timeout and other issues of default client).

[jens1205]

I also stumbled about this wrong documentation. Should be improved (together with #368)

[DRK3]

I also came across this. On a similar note: why do the Client method docs state that the returned client should not be modified? Here's the snippet:

// Client returns an HTTP client using the provided token.
// The token will auto-refresh as necessary. The underlying
// HTTP transport will be obtained using the provided context.
// The returned client and its Transport should not be modified.
func (c *Config) Client(ctx context.Context, t *Token) *http.Client {
	return NewClient(ctx, c.TokenSource(ctx, t))
}

Specifically, I want to set the timeout on the returned client, but it's not clear to me why I'm not supposed to modify the client's timeout value, and what the alternative would be.

Edit: For setting a timeout value, I suppose you could use NewRequestWithContext, but I'm wondering if this is the only way...

[dnesting]

Oof, I hit this today too. I did some experiments with every combination of ways you can mix a context and an HttpClient. The cases that might be surprising to users are:

• config.Client(ctx) uses the context's HttpClient for all requests, including token refreshes
• oauth2.NewClient(ctx, ...) returns an HttpClient that chains up to ctx's HttpClient. The ctx HttpClient is not used for token refreshes. If you want to control the HttpClient used for token refreshes, put it in the context passed to either config.Client(ctx), or use config.TokenSource(ctx) and use something that takes a TokenSource.

Here's my data:

config.Client(ctx).Get:
- Token refresh using transport chaining back to config.Client context
- Page request using transport chaining back to config.Client context

oauth2.NewClient(ctx, config.TokenSource(ctx)).Get:
- Token refresh using transport chaining back to config.TokenSource context
- Page request using transport chaining back to oauth2.NewClient context

oauth2.NewClient(ctx, config.TokenSource(background)).Get:
- Token refresh using transport chaining back to http.DefaultClient
- Page request using transport chaining back to oauth2.NewClient context

oauth2.NewClient(background, config.TokenSource(ctx)).Get:
- Token refresh using transport chaining back to config.TokenSource context
- Page request using transport chaining back to http.DefaultClient

oauth2.NewClient(ctx, config.TokenSource(ctx)).Do(Request.WithContext(ctx)):
- Token refresh using transport chaining back to config.TokenSource context
- Page request using transport chaining back to oauth2.NewClient context

clientcredentials.Config.Client(ctx).Get:
- Token refresh using transport chaining back to clientConfig.Client context
- Page request using transport chaining back to clientConfig.Client context

oauth2.NewClient(ctx, clientcredentials.Config.TokenSource(ctx)).Get:
- Token refresh using transport chaining back to clientConfig.TokenSource context
- Page request using transport chaining back to oauth2.NewClient context

oauth2.NewClient(ctx, clientcredentials.Config.TokenSource(background)).Get:
- Token refresh using transport chaining back to http.DefaultClient
- Page request using transport chaining back to oauth2.NewClient context

oauth2.NewClient(background, clientcredentials.Config.TokenSource(ctx)).Get:
- Token refresh using transport chaining back to clientConfig.TokenSource context
- Page request using transport chaining back to http.DefaultClient

[dnesting]

I submitted https://go-review.googlesource.com/c/oauth2/+/493335 to update the documentation to match implementation.

[Av1shay]

also regarding the documentation and what @DRK3 wrote:

// Client returns an HTTP client wrapping the context's
// HTTP transport and adding Authorization headers with tokens
// obtained from c.
//
// The returned client and its Transport should not be modified.

Why the returned client can't be modified? If the client can't be modified, and we can't inject custom client thought the context (because according to the docs, the client that passed from context is only used for token requests, and the actual returned client is different), how can I set timeout on the returned client? e.g

httpClient := cfg.Client(context.Background)
httpClient.Timeout = 30*time.Second // according to docs this is forbidden

[DRK3]

@Av1shay Yes, I had exactly the same question. From my testing, it seems like setting the Timeout does indeed work as expected (at least for my test cases). I'm suspecting it's just a mistake in the docs, but I'm not 100% certain

[Av1shay]

@DRK3 in the meantime I use this workaround

httpClient := &http.Client{
    Transport: jwtCfg.Client(ctx).Transport,
    Timeout:   30 * time.Second,
}

[fosple]

Same issue here. I have to do a HTTP request from a specific IP address. The only way I see to do it is via modifying the DialContext (see example below in which "123.456.789.123" is the IP address I want to use to make the request). Sadly when using oauth2 this is not possible anymore.

Do you know a workaround until this is fixed?

localAddr, _ := net.ResolveIPAddr("ip", string("123.456.789.123"))

localTCPAddr := net.TCPAddr{
	IP: localAddr.IP,
}

httpClient = &http.Client{
	Transport: &http.Transport{
		Proxy: http.ProxyFromEnvironment,
		DialContext: (&net.Dialer{
			LocalAddr: &localTCPAddr,
			Timeout:   30 * time.Second,
			KeepAlive: 30 * time.Second,
			DualStack: true,
		}).DialContext,
		MaxIdleConns:          100,
		IdleConnTimeout:       90 * time.Second,
		TLSHandshakeTimeout:   10 * time.Second,
		ExpectContinueTimeout: 1 * time.Second,
	}
}

[mitar]

@fosple I think you should pass client inside the context and it should work. So I do not remember exactly anymore, it is some time when I was implementing oauth2 using this library, but my issue was more about documentation and not that it is not possible to do it.

[fosple]

Thanks @mitar. Will give it a try. Also stumbled upon https://github.com/golang/oauth2/issues/368 so not sure if it works.

[fosple]

It works. Thanks a lot @mitar!