Issue 6900055: code review 6900055: goauth2: Use content-type detection to select codec
Descriptiongoauth2: Use content-type detection to select codec Patch Set 1 #Patch Set 2 : diff -r f77b3584c59b https://code.google.com/p/goauth2/ #Patch Set 3 : diff -r f77b3584c59b https://code.google.com/p/goauth2/ # Total comments: 6 Patch Set 4 : diff -r f77b3584c59b https://code.google.com/p/goauth2/ # Total comments: 2 Patch Set 5 : diff -r f77b3584c59b https://code.google.com/p/goauth2/ # Total comments: 2 Patch Set 6 : diff -r f77b3584c59b https://code.google.com/p/goauth2/ #Patch Set 7 : diff -r f77b3584c59b https://code.google.com/p/goauth2/ #MessagesTotal messages: 15
Hello adg@google.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/goauth2/ Sign in to reply to this message.
I apologize for somehow CC'ing golang-dev. I have no idea how that happend. Sign in to reply to this message.
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... > Sign in to reply to this message.
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? Sign in to reply to this message.
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? Sign in to reply to this message.
Hello adg@google.com, adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look. Sign in to reply to this message.
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. Sign in to reply to this message.
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. Sign in to reply to this message.
Hello adg@google.com, adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look. Sign in to reply to this message.
LGTM Sign in to reply to this message.
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? Sign in to reply to this message.
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. Sign in to reply to this message.
Hello adg@google.com, adg@golang.org, nightlyone@googlemail.com (cc: golang-dev@googlegroups.com), Please take another look. Sign in to reply to this message.
LGTM Sign in to reply to this message.
*** 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> Sign in to reply to this message. |
