code review 6900055: goauth2: Use content-type detection to select codec

goauth2: Use content-type detection to select codec

[Surma, 2012-12-08 20:55:26]

Hello adg@google.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/goauth2/

[Surma, 2012-12-08 20:58:38]

I apologize for somehow CC'ing golang-dev. I have no idea how that happend.

[adg1, 2012-12-10 00:39:58]

It's entirely appropriate.


On 9 December 2012 07:58, <surma@surmair.de> wrote:

> I apologize for somehow CC'ing golang-dev. I have no idea how that
> happend.
>
>
https://codereview.appspot.**com/6900055/<https://codereview.appspot.com/6900...
>

[adg, 2012-12-11 06:32:22]

https://codereview.appspot.com/6900055/diff/5001/oauth/oauth.go
File oauth/oauth.go (right):

https://codereview.appspot.com/6900055/diff/5001/oauth/oauth.go#newcode254
oauth/oauth.go:254: req.Header.Set("Authorization", "Bearer "+t.AccessToken)
I forgot about this header.
Does Facebook accept "Bearer" headers?

[adg, 2012-12-11 06:33:33]

https://codereview.appspot.com/6900055/diff/5001/oauth/oauth.go
File oauth/oauth.go (right):

https://codereview.appspot.com/6900055/diff/5001/oauth/oauth.go#newcode296
oauth/oauth.go:296: content := strings.Split(r.Header.Get("Content-Type"), ";")
content := strings.Split(r.Header.Get("Content-Type"), ";")[0]

https://codereview.appspot.com/6900055/diff/5001/oauth/oauth.go#newcode297
oauth/oauth.go:297: switch content[0] {
switch content {

https://codereview.appspot.com/6900055/diff/5001/oauth/oauth.go#newcode298
oauth/oauth.go:298: case "application/x-www-form-urlencoded", "text/plain":
why text/plain?

[Surma, 2012-12-11 13:12:43]

Hello adg@google.com, adg@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[Surma, 2012-12-11 13:13:39]

https://codereview.appspot.com/6900055/diff/5001/oauth/oauth.go
File oauth/oauth.go (right):

https://codereview.appspot.com/6900055/diff/5001/oauth/oauth.go#newcode254
oauth/oauth.go:254: req.Header.Set("Authorization", "Bearer "+t.AccessToken)
> Does Facebook accept "Bearer" headers?
Short answer: Yes.
Long answer: Yes, it is even the only header which is 
a) in the official spec[1]
b) supported by all of Google, GitHub and Facebook's OAuth providers
[1]: http://tools.ietf.org/html/draft-ietf-oauth-v2-31#section-7.1

https://codereview.appspot.com/6900055/diff/5001/oauth/oauth.go#newcode298
oauth/oauth.go:298: case "application/x-www-form-urlencoded", "text/plain":
On 2012/12/11 06:33:33, adg wrote:
> why text/plain?
“Be liberal in what you accept”, and more hilariously: Because Facebook doesn't
seem to set the `Content-Type` header appropriately. They return urlencoded data
with a `text/plain` content type. Defaulting to urlencode when getting
`text/plain` seems appropriate after browsing the spec.

[adg, 2012-12-12 02:22:41]

https://codereview.appspot.com/6900055/diff/11002/oauth/oauth.go
File oauth/oauth.go (right):

https://codereview.appspot.com/6900055/diff/11002/oauth/oauth.go#newcode308
oauth/oauth.go:308: b.Access = vals.Get("access_token")
what about the refresh token?

https://codereview.appspot.com/6900055/diff/11002/oauth/oauth_test.go
File oauth/oauth_test.go (right):

https://codereview.appspot.com/6900055/diff/11002/oauth/oauth_test.go#newcode44
oauth/oauth_test.go:44: {path: "/secure", auth: "Bearer token2", body: "second
payload"},
Please add a test for a urlencoded payload.

[Surma, 2012-12-13 13:18:18]

Hello adg@google.com, adg@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[adg, 2012-12-14 01:23:23]

LGTM

[ioe, 2012-12-15 02:36:51]

https://codereview.appspot.com/6900055/diff/18001/oauth/oauth.go
File oauth/oauth.go (right):

https://codereview.appspot.com/6900055/diff/18001/oauth/oauth.go#newcode296
oauth/oauth.go:296: content := strings.Split(r.Header.Get("Content-Type"),
";")[0]
What about using the ParseMediaType function from "net/mime" package here?

[Surma, 2012-12-15 08:13:27]

https://codereview.appspot.com/6900055/diff/18001/oauth/oauth.go
File oauth/oauth.go (right):

https://codereview.appspot.com/6900055/diff/18001/oauth/oauth.go#newcode296
oauth/oauth.go:296: content := strings.Split(r.Header.Get("Content-Type"),
";")[0]
On 2012/12/15 02:36:51, ioe wrote:
> What about using the ParseMediaType function from "net/mime" package here?
Good point. Did not think of that package.

[Surma, 2012-12-15 08:13:57]

Hello adg@google.com, adg@golang.org, nightlyone@googlemail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[ioe, 2012-12-15 08:21:13]

LGTM

[adg, 2012-12-16 22:12:06]

*** Submitted as https://code.google.com/p/goauth2/source/detail?r=f3b3146d6c91
***

goauth2: Use content-type detection to select codec

R=adg, adg, nightlyone
CC=golang-dev
https://codereview.appspot.com/6900055

Committer: Andrew Gerrand <adg@golang.org>