oauth2: do not assume that http.DefaultClient and http.DefaultTransport is always available

oauth2: do not assume that http.DefaultClient and http.DefaultTransport is always available https://github.com/golang/oauth2/issues/27

[adg, 2014-08-31 22:47:06]

LGTM wording tweaks in oauth2.go apply to all other relevant filees https://codereview.appspot.com/138970043/diff/50006/google/appenginevm.go File google/appenginevm.go (right): https://codereview.appspot.com/138970043/diff/50006/google/appenginevm.go#new... google/appenginevm.go:18: context appengine.Context put a blank above this and remove the blank below https://codereview.appspot.com/138970043/diff/50006/oauth2.go File oauth2.go (right): https://codereview.appspot.com/138970043/diff/50006/oauth2.go#newcode114 oauth2.go:114: // Client is the default HTTP client to be used while retrieving s/while retrieving/to retrieve/ s/to be // https://codereview.appspot.com/138970043/diff/50006/oauth2.go#newcode114 oauth2.go:114: // Client is the default HTTP client to be used while retrieving s/default // https://codereview.appspot.com/138970043/diff/50006/oauth2.go#newcode118 oauth2.go:118: // Transport represents the default round tripper to be used s/default // https://codereview.appspot.com/138970043/diff/50006/oauth2.go#newcode118 oauth2.go:118: // Transport represents the default round tripper to be used s/represents/is/ s/to be // https://codereview.appspot.com/138970043/diff/50006/oauth2.go#newcode119 oauth2.go:119: // while constructing new oauth2.Transport instances from s/while constructing/to construct/

[jbd, 2014-08-31 23:09:04]

https://codereview.appspot.com/138970043/diff/50006/google/appenginevm.go File google/appenginevm.go (right): https://codereview.appspot.com/138970043/diff/50006/google/appenginevm.go#new... google/appenginevm.go:18: context appengine.Context On 2014/08/31 22:47:05, adg wrote: > put a blank above this and remove the blank below Done. https://codereview.appspot.com/138970043/diff/50006/oauth2.go File oauth2.go (right): https://codereview.appspot.com/138970043/diff/50006/oauth2.go#newcode114 oauth2.go:114: // Client is the default HTTP client to be used while retrieving On 2014/08/31 22:47:06, adg wrote: > s/while retrieving/to retrieve/ > s/to be // Done. https://codereview.appspot.com/138970043/diff/50006/oauth2.go#newcode114 oauth2.go:114: // Client is the default HTTP client to be used while retrieving On 2014/08/31 22:47:05, adg wrote: > s/default // Done. https://codereview.appspot.com/138970043/diff/50006/oauth2.go#newcode118 oauth2.go:118: // Transport represents the default round tripper to be used On 2014/08/31 22:47:05, adg wrote: > s/default // Done. https://codereview.appspot.com/138970043/diff/50006/oauth2.go#newcode118 oauth2.go:118: // Transport represents the default round tripper to be used On 2014/08/31 22:47:06, adg wrote: > s/represents/is/ > s/to be // Done. https://codereview.appspot.com/138970043/diff/50006/oauth2.go#newcode118 oauth2.go:118: // Transport represents the default round tripper to be used On 2014/08/31 22:47:05, adg wrote: > s/default // Done. https://codereview.appspot.com/138970043/diff/50006/oauth2.go#newcode119 oauth2.go:119: // while constructing new oauth2.Transport instances from On 2014/08/31 22:47:06, adg wrote: > s/while constructing/to construct/ Done.

[jbd, 2014-08-31 23:09:51]

Submitted as 32b45383ad63f4a8bcfb8c6f4b7b4651ec14fcf3.

[bradfitz, 2014-09-01 16:58:15]

https://codereview.appspot.com/138970043/diff/70001/google/appengine.go File google/appengine.go (right): https://codereview.appspot.com/138970043/diff/70001/google/appengine.go#newco... google/appengine.go:27: return &AppEngineConfig{ so you need to create a new config for each HTTP request? I guess that works. A better way in the future is to plumb contexts through (go.net has a Context type and so does Camlistore) and use the appengine.Context to get a RoundTripper later as needed. but we can do that later. https://codereview.appspot.com/138970043/diff/70001/google/appengine.go#newco... google/appengine.go:31: AllowInvalidServerCertificate: false, drop. this one doesn't worth saying explicitly. of course you want it secure by default. https://codereview.appspot.com/138970043/diff/70001/jwt.go File jwt.go (right): https://codereview.appspot.com/138970043/diff/70001/jwt.go#newcode70 jwt.go:70: // Client is the HTTP client to be used to retrieve same comments as other CL https://codereview.appspot.com/138970043/diff/70001/oauth2.go File oauth2.go (right): https://codereview.appspot.com/138970043/diff/70001/oauth2.go#newcode100 oauth2.go:100: Client: http.DefaultClient, drop these. use the zero value. people should be able to make Configs by themselves. https://codereview.appspot.com/138970043/diff/70001/oauth2.go#newcode116 oauth2.go:116: Client *http.Client both of these should be optional with a zero value being http.DefaultClient/http.DefaultTransport. But why are there two of them? Seems like either one (depending on what you need) is sufficient, especially since a Client's Transport is exported. https://codereview.appspot.com/138970043/diff/70001/oauth2.go#newcode118 oauth2.go:118: // Transport is the round tripper to be used RoundTripper or http.RoundTripper. "round tripper" isn't known to anybody just reading English docs. It's not some common expression... just a Go thing. https://codereview.appspot.com/138970043/diff/70001/oauth2.go#newcode169 oauth2.go:169: return NewTransport(c.Transport, c, nil) c.transport() as a method to do the zero value check and return http.DefaultTransport https://codereview.appspot.com/138970043/diff/70001/oauth2.go#newcode234 oauth2.go:234: r, err := c.Client.PostForm(c.tokenURL.String(), v) c.client()

[jbd, 2014-09-02 21:19:10]

https://codereview.appspot.com/138970043/diff/70001/google/appengine.go File google/appengine.go (right): https://codereview.appspot.com/138970043/diff/70001/google/appengine.go#newco... google/appengine.go:27: return &AppEngineConfig{ On 2014/09/01 16:58:14, bradfitz wrote: > so you need to create a new config for each HTTP request? > > I guess that works. A better way in the future is to plumb contexts through > (http://go.net has a Context type and so does Camlistore) and use the appengine.Context > to get a RoundTripper later as needed. > > but we can do that later. Acknowledged. https://codereview.appspot.com/138970043/diff/70001/google/appengine.go#newco... google/appengine.go:31: AllowInvalidServerCertificate: false, On 2014/09/01 16:58:14, bradfitz wrote: > drop. this one doesn't worth saying explicitly. of course you want it secure by > default. Dropping `Deadline` as well. Done. https://codereview.appspot.com/138970043/diff/70001/jwt.go File jwt.go (right): https://codereview.appspot.com/138970043/diff/70001/jwt.go#newcode70 jwt.go:70: // Client is the HTTP client to be used to retrieve On 2014/09/01 16:58:14, bradfitz wrote: > same comments as other CL Done. https://codereview.appspot.com/138970043/diff/70001/oauth2.go File oauth2.go (right): https://codereview.appspot.com/138970043/diff/70001/oauth2.go#newcode100 oauth2.go:100: Client: http.DefaultClient, On 2014/09/01 16:58:15, bradfitz wrote: > drop these. use the zero value. people should be able to make Configs by > themselves. Done. https://codereview.appspot.com/138970043/diff/70001/oauth2.go#newcode116 oauth2.go:116: Client *http.Client > Seems like either one (depending on what you need) is sufficient, There are cases I would like to customize client's RoundTripper to add additional headers, etc. I wouldn't like to reuse a RoundTripper that is supposed to work with auth endpoints to talk to the actual APIs. It's too utilitarian to reuse the client. https://codereview.appspot.com/138970043/diff/70001/oauth2.go#newcode118 oauth2.go:118: // Transport is the round tripper to be used On 2014/09/01 16:58:15, bradfitz wrote: > RoundTripper or http.RoundTripper. > > "round tripper" isn't known to anybody just reading English docs. It's not some > common expression... just a Go thing. Done. https://codereview.appspot.com/138970043/diff/70001/oauth2.go#newcode169 oauth2.go:169: return NewTransport(c.Transport, c, nil) On 2014/09/01 16:58:15, bradfitz wrote: > c.transport() as a method to do the zero value check and return > http.DefaultTransport Done. https://codereview.appspot.com/138970043/diff/70001/oauth2.go#newcode234 oauth2.go:234: r, err := c.Client.PostForm(c.tokenURL.String(), v) On 2014/09/01 16:58:15, bradfitz wrote: > c.client() Done.

[jbd, 2014-09-03 17:53:25]

+proppy for GAE. PTAL?

[proppy, 2014-09-03 23:45:53]

https://codereview.appspot.com/138970043/diff/90001/google/appengine.go File google/appengine.go (right): https://codereview.appspot.com/138970043/diff/90001/google/appengine.go#newco... google/appengine.go:20: Transport *urlfetch.Transport Sounds like this should be a http.RoundTripper if we want to allow user to override it with their own.

[jbd, 2014-09-03 23:51:48]

https://codereview.appspot.com/138970043/diff/90001/google/appengine.go File google/appengine.go (right): https://codereview.appspot.com/138970043/diff/90001/google/appengine.go#newco... google/appengine.go:20: Transport *urlfetch.Transport On 2014/09/03 23:45:53, proppy wrote: > Sounds like this should be a http.RoundTripper if we want to allow user to > override it with their own. On GAE, you should always override it with a urlfetch.Transport. So, why not to enforce it? https://codereview.appspot.com/138970043/diff/90001/google/appengine.go#newco... google/appengine.go:53: func (c *AppEngineConfig) transport() http.RoundTripper { I should return a urlfetch.Transport.

[proppy, 2014-09-03 23:56:34]

https://codereview.appspot.com/138970043/diff/90001/google/appengine.go File google/appengine.go (right): https://codereview.appspot.com/138970043/diff/90001/google/appengine.go#newco... google/appengine.go:20: Transport *urlfetch.Transport On 2014/09/03 23:51:47, jbd wrote: > On 2014/09/03 23:45:53, proppy wrote: > > Sounds like this should be a http.RoundTripper if we want to allow user to > > override it with their own. > > On GAE, you should always override it with a urlfetch.Transport. So, why not to > enforce it? If you want to implement caching, you might end up wrapping the urlfetch.Transport w/ your own type. The oauth2 lib shound care as long as it satisfy it needs: http.RoundTripper.

[jbd, 2014-09-04 00:20:46]

https://codereview.appspot.com/138970043/diff/90001/google/appengine.go File google/appengine.go (right): https://codereview.appspot.com/138970043/diff/90001/google/appengine.go#newco... google/appengine.go:20: Transport *urlfetch.Transport > If you want to implement caching, you might end up wrapping the > urlfetch.Transport w/ your own type. > > The oauth2 lib shound care as long as it satisfy it needs: http.RoundTripper. > Yes, that was the whole point to work with RoundTrippers. Done. https://codereview.appspot.com/138970043/diff/90001/google/appengine.go#newco... google/appengine.go:53: func (c *AppEngineConfig) transport() http.RoundTripper { On 2014/09/03 23:51:47, jbd wrote: > I should return a urlfetch.Transport. Keeping it as http.RoundTripper.

[proppy, 2014-09-04 00:30:28]

On 2014/09/04 00:20:46, jbd wrote: > https://codereview.appspot.com/138970043/diff/90001/google/appengine.go > File google/appengine.go (right): > > https://codereview.appspot.com/138970043/diff/90001/google/appengine.go#newco... > google/appengine.go:20: Transport *urlfetch.Transport > > > If you want to implement caching, you might end up wrapping the > > urlfetch.Transport w/ your own type. > > > > The oauth2 lib shound care as long as it satisfy it needs: http.RoundTripper. > > > > Yes, that was the whole point to work with RoundTrippers. Done. > > https://codereview.appspot.com/138970043/diff/90001/google/appengine.go#newco... > google/appengine.go:53: func (c *AppEngineConfig) transport() http.RoundTripper > { > On 2014/09/03 23:51:47, jbd wrote: > > I should return a urlfetch.Transport. > > Keeping it as http.RoundTripper. LGTM

[jbd, 2014-09-04 01:05:35]

Submitted as 948919caa0fda0e1e6aaaf613dc68cad65da28b5.