oauth2: NewClient should copy settings from the context client

[abeltay]

OAuth2 client creation currently doesn't faithfully reuse the client passed into the context.

Instead, it simply takes the Transport of the context client and puts it into a DefaultClient. This causes config settings such as timeout to be set to Default and we know default timeouts are bad #24138. This may end up to be a gotcha for anyone who sends in a context client with timeout set assuming that the timeout will be copied to the new client.

Propose to update the NewClient code as follows:

 func NewClient(ctx context.Context, src TokenSource) *http.Client {
        if src == nil {
                return internal.ContextClient(ctx)
        }
+       cc := internal.ContextClient(ctx)
        return &http.Client{
                Transport: &Transport{
-                       Base:   internal.ContextClient(ctx).Transport,
+                       Base:   cc.Transport,
                        Source: ReuseTokenSource(nil, src),
                },
+               CheckRedirect: cc.CheckRedirect,
+               Jar:           cc.Jar,
+               Timeout:       cc.Timeout,
        }
 }

References: #321

[amuttsch]

As a workaround you can do the following:

    // get client
    token, err := oauth.PasswordCredentialsToken(ctx, user, pass)
    oauthClient2 := oauth.Client(ctx, token)
    if err != nil {
        return nil, err
    }

    client := &http.Client{
        Transport: oauthClient.Transport,
        Timeout: 15 * time.Second,
    }

But I support the change suggested by @abeltay for the NewClient to reuse configurations from the httpClient provided by the context.

[DRK3]

+1 for this change too!

[DRK3]

I've also noticed that the docs on the Client method state that:

// The returned client and its Transport should not be modified.

Is there a reason that the client's fields shouldn't be modified? Specifically, I want to set the client timeout field which doesn't get copied over from the context client (see the OP's post regarding this).

[phil-f]

I've also noticed that the docs on the Client method state that:

// The returned client and its Transport should not be modified.

Is there a reason that the client's fields shouldn't be modified? Specifically, I want to set the client timeout field which doesn't get copied over from the context client (see the OP's post regarding this).

+1 on this, would be great to get some clarification.