code review 12906045: goauth2: Keep track of the JWT id_token received during...

goauth2: Keep track of the JWT id_token received during access token exchanges. For more details about the JWT id_token, see: http://openid.net/specs/oauth-v2-multiple-response-types-1_0-08.html#id_token And the Google OAuth APIs: https://developers.google.com/accounts/docs/OAuth2Login#exchangecode

[asimshankar, 2013-08-20 07:23:20]

Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/goauth2

[asimshankar, 2013-08-20 07:25:49]

Hello adg@golang.org, golang-dev@googlegroups.com (cc: golang-dev, golang-dev@googlegroups.com), Please take another look.

[bradfitz, 2013-08-20 14:50:28]

R=adg On Tue, Aug 20, 2013 at 12:23 AM, <asimshankar@gmail.com> wrote: > Reviewers: golang-dev1, > > Message: > Hello golang-dev@googlegroups.com, > > I'd like you to review this change to > https://code.google.com/p/**goauth2 <https://code.google.com/p/goauth2> > > > Description: > goauth2: Keep track of the JWT id_token received during access token > exchanges. > > For more details about the JWT id_token, see: > http://openid.net/specs/oauth-**v2-multiple-response-types-1_** > 0-08.html#id_token<http://openid.net/specs/oauth-v2-multiple-response-types-1_0-08.html#id_token> > And the Google OAuth APIs: > https://developers.google.com/**accounts/docs/OAuth2Login#**exchangecode<http... > > Please review this at https://codereview.appspot.**com/12906045/<https://codereview.appspot.com/129... > > Affected files: > M oauth/oauth.go > M oauth/oauth_test.go > > > Index: oauth/oauth.go > ==============================**==============================**======= > --- a/oauth/oauth.go > +++ b/oauth/oauth.go > @@ -133,6 +133,7 @@ > type Token struct { > AccessToken string > RefreshToken string > + IdToken string // A JWT (JSON Web Token). > Expiry time.Time // If zero the token has no (known) expiry > time. > } > > @@ -323,6 +324,7 @@ > var b struct { > Access string `json:"access_token"` > Refresh string `json:"refresh_token"` > + Id string `json:"id_token"` > ExpiresIn time.Duration `json:"expires_in"` > } > > @@ -340,6 +342,7 @@ > > b.Access = vals.Get("access_token") > b.Refresh = vals.Get("refresh_token") > + b.Id = vals.Get("id_token") > b.ExpiresIn, _ = time.ParseDuration(vals.Get("**expires_in") > + "s") > default: > if err = json.NewDecoder(r.Body).**Decode(&b); err != nil > { > @@ -359,5 +362,6 @@ > } else { > tok.Expiry = time.Now().Add(b.ExpiresIn) > } > + tok.IdToken = b.Id > return nil > } > Index: oauth/oauth_test.go > ==============================**==============================**======= > --- a/oauth/oauth_test.go > +++ b/oauth/oauth_test.go > @@ -29,6 +29,7 @@ > { > "access_token":"token1", > "refresh_token":"**refreshtoken1", > + "id_token":"idtoken1", > "expires_in":3600 > } > `, > @@ -42,6 +43,7 @@ > { > "access_token":"token2", > "refresh_token":"**refreshtoken2", > + "id_token":"idtoken2", > "expires_in":3600 > } > `, > @@ -51,7 +53,7 @@ > path: "/token", > query: "grant_type=refresh_token&** > refresh_token=refreshtoken2&**client_id=cl13nt1d", > contenttype: "application/x-www-form-**urlencoded", > - body: "access_token=token3&refresh_** > token=refreshtoken3&expires_**in=3600", > + body: "access_token=token3&refresh_** > token=refreshtoken3&id_token=**idtoken3&expires_in=3600", > }, > {path: "/secure", auth: "Bearer token3", body: "third payload"}, > } > @@ -103,7 +105,7 @@ > if err != nil { > t.Fatalf("Exchange: %v", err) > } > - checkToken(t, transport.Token, "token1", "refreshtoken1") > + checkToken(t, transport.Token, "token1", "refreshtoken1", > "idtoken1") > > c := transport.Client() > resp, err := c.Get(server.URL + "/secure") > @@ -119,7 +121,7 @@ > t.Fatalf("Get: %v", err) > } > checkBody(t, resp, "second payload") > - checkToken(t, transport.Token, "token2", "refreshtoken2") > + checkToken(t, transport.Token, "token2", "refreshtoken2", > "idtoken2") > > // refresh one more time, but get URL-encoded token instead of JSON > transport.Expiry = time.Now() > @@ -128,16 +130,19 @@ > t.Fatalf("Get: %v", err) > } > checkBody(t, resp, "third payload") > - checkToken(t, transport.Token, "token3", "refreshtoken3") > + checkToken(t, transport.Token, "token3", "refreshtoken3", > "idtoken3") > } > > -func checkToken(t *testing.T, tok *Token, access, refresh string) { > +func checkToken(t *testing.T, tok *Token, access, refresh, id string) { > if g, w := tok.AccessToken, access; g != w { > t.Errorf("AccessToken = %q, want %q", g, w) > } > if g, w := tok.RefreshToken, refresh; g != w { > t.Errorf("RefreshToken = %q, want %q", g, w) > } > + if g, w := tok.IdToken, id; g != w { > + t.Errorf("IdToken = %q, want %q", g, w) > + } > exp := tok.Expiry.Sub(time.Now()) > if (time.Hour-time.Second) > exp || exp > time.Hour { > t.Errorf("Expiry = %v, want ~1 hour", exp) > > > > -- > > ---You received this message because you are subscribed to the Google > Groups "golang-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou... > . > For more options, visit https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o... > . > 

[adg, 2013-08-22 07:06:49]

Functionally it looks okay, but if http://golang.org/issue/6213 gets in for 1.2 I'd prefer to take that route. Can we sit on this for a week or two?

[asimshankar, 2013-08-22 07:47:39]

On 2013/08/22 07:06:49, adg wrote: > Functionally it looks okay, but if http://golang.org/issue/6213 gets in for 1.2 > I'd prefer to take that route. > > Can we sit on this for a week or two? Hmm...but even if it does get in for 1.2, it seems 1.2 will only be released in December Ref: https://groups.google.com/d/msg/golang-dev/2swnt6u9pUs/hNk-rsmA-hoJ If the issue you pointed to does make it for the Sept. 1 code freeze AND you're okay with goauth2 not working with the Go release till December, then I'm happy to wait a week or so. But if either of those doesn't hold, I'd request that we proceed with this for now. Thanks!

[adg, 2013-08-26 05:14:48]

Let's explore getting this in, for now. https://codereview.appspot.com/12906045/diff/9001/oauth/oauth.go File oauth/oauth.go (right): https://codereview.appspot.com/12906045/diff/9001/oauth/oauth.go#newcode345 oauth/oauth.go:345: b.Id = vals.Get("id_token") does the google oauth2 server return its values as a urlencoded response, or a JSON response? If the latter, this code path will not get exercised.

[asimshankar, 2013-08-26 06:11:41]

https://codereview.appspot.com/12906045/diff/9001/oauth/oauth.go File oauth/oauth.go (right): https://codereview.appspot.com/12906045/diff/9001/oauth/oauth.go#newcode345 oauth/oauth.go:345: b.Id = vals.Get("id_token") On 2013/08/26 05:14:48, adg wrote: > does the google oauth2 server return its values as a urlencoded response, or a > JSON response? If the latter, this code path will not get exercised. As per: https://developers.google.com/accounts/docs/OAuth2Login#exchangecode it's a JSON response. I just added the url-encoding codepath for consistency.

[adg, 2013-08-27 03:54:01]

I'd like to make this compatible with the future approach, so here it is. https://codereview.appspot.com/12906045/diff/9001/oauth/oauth.go File oauth/oauth.go (right): https://codereview.appspot.com/12906045/diff/9001/oauth/oauth.go#newcode136 oauth/oauth.go:136: IdToken string // A JWT (JSON Web Token). make this a field Extra map[string]string // may be nil https://codereview.appspot.com/12906045/diff/9001/oauth/oauth.go#newcode327 oauth/oauth.go:327: Id string `json:"id_token"` keep this https://codereview.appspot.com/12906045/diff/9001/oauth/oauth.go#newcode345 oauth/oauth.go:345: b.Id = vals.Get("id_token") On 2013/08/26 06:11:41, asimshankar wrote: > On 2013/08/26 05:14:48, adg wrote: > > does the google oauth2 server return its values as a urlencoded response, or a > > JSON response? If the latter, this code path will not get exercised. > > As per: https://developers.google.com/accounts/docs/OAuth2Login#exchangecode > it's a JSON response. > > I just added the url-encoding codepath for consistency. Hm, ok. I guess. https://codereview.appspot.com/12906045/diff/9001/oauth/oauth.go#newcode365 oauth/oauth.go:365: tok.IdToken = b.Id if b.Id != "" { if tok.Extras == nil { tok.Extras = make(map[string]string) } tok.Extras["id_token"] = b.Id }

[asimshankar, 2013-08-27 04:22:43]

Thanks. All comments addressed. https://codereview.appspot.com/12906045/diff/9001/oauth/oauth.go File oauth/oauth.go (right): https://codereview.appspot.com/12906045/diff/9001/oauth/oauth.go#newcode136 oauth/oauth.go:136: IdToken string // A JWT (JSON Web Token). On 2013/08/27 03:54:01, adg wrote: > make this a field > > Extra map[string]string // may be nil Done. https://codereview.appspot.com/12906045/diff/9001/oauth/oauth.go#newcode327 oauth/oauth.go:327: Id string `json:"id_token"` On 2013/08/27 03:54:01, adg wrote: > keep this Done. https://codereview.appspot.com/12906045/diff/9001/oauth/oauth.go#newcode365 oauth/oauth.go:365: tok.IdToken = b.Id On 2013/08/27 03:54:01, adg wrote: > if b.Id != "" { > if tok.Extras == nil { > tok.Extras = make(map[string]string) > } > tok.Extras["id_token"] = b.Id > } Done.

[adg, 2013-08-27 04:42:53]

https://codereview.appspot.com/12906045/diff/18001/oauth/oauth.go File oauth/oauth.go (right): https://codereview.appspot.com/12906045/diff/18001/oauth/oauth.go#newcode136 oauth/oauth.go:136: Extra map[string]string // May be nil. sorry, for the tiny nitpick, but please put this below the Expiry field https://codereview.appspot.com/12906045/diff/18001/oauth/oauth.go#newcode327 oauth/oauth.go:327: Id string `json:"id_token"` move below the expiry field https://codereview.appspot.com/12906045/diff/18001/oauth/oauth.go#newcode345 oauth/oauth.go:345: b.Id = vals.Get("id_token") move down one line

[asimshankar, 2013-08-27 04:56:23]

All done. https://codereview.appspot.com/12906045/diff/18001/oauth/oauth.go File oauth/oauth.go (right): https://codereview.appspot.com/12906045/diff/18001/oauth/oauth.go#newcode136 oauth/oauth.go:136: Extra map[string]string // May be nil. On 2013/08/27 04:42:54, adg wrote: > sorry, for the tiny nitpick, but please put this below the Expiry field Done. https://codereview.appspot.com/12906045/diff/18001/oauth/oauth.go#newcode327 oauth/oauth.go:327: Id string `json:"id_token"` On 2013/08/27 04:42:54, adg wrote: > move below the expiry field Done. https://codereview.appspot.com/12906045/diff/18001/oauth/oauth.go#newcode345 oauth/oauth.go:345: b.Id = vals.Get("id_token") On 2013/08/27 04:42:54, adg wrote: > move down one line Done.

[adg, 2013-08-27 04:58:58]

LGTM

[adg, 2013-08-27 05:00:43]

Have you signed the Contributor License Agreement? http://golang.org/doc/contribute.html#copyright 

[asimshankar, 2013-08-27 05:04:50]

Just did. Though, since I'm currently employed by Google, I'm under the impression that my individual signature was not necessary? Thanks, -- Asim On Mon, Aug 26, 2013 at 10:00 PM, Andrew Gerrand <adg@golang.org> wrote: > Have you signed the Contributor License Agreement? > > http://golang.org/doc/contribute.html#copyright > 

[adg, 2013-08-27 05:14:49]

On 27 August 2013 15:04, Asim Shankar <asimshankar@gmail.com> wrote: > > Though, since I'm currently employed by Google, I'm under the impression > that my individual signature was not necessary? > I didn't realize/remember that. It's not necessary for you to sign the CLA in that case. Do you want to use your google.com or gmail.com email address to contribute to the project? 

[asimshankar, 2013-08-27 05:16:52]

Flipped a coin: gmail it is :) Thanks! On Mon, Aug 26, 2013 at 10:14 PM, Andrew Gerrand <adg@golang.org> wrote: > > On 27 August 2013 15:04, Asim Shankar <asimshankar@gmail.com> wrote: >> >> Though, since I'm currently employed by Google, I'm under the impression >> > that my individual signature was not necessary? >> > > I didn't realize/remember that. It's not necessary for you to sign the CLA > in that case. > > Do you want to use your google.com or gmail.com email address to > contribute to the project? > > > 

[adg, 2013-08-27 23:40:36]

*** Submitted as https://code.google.com/p/goauth2/source/detail?r=aa0ef39a7c24 *** goauth2: Keep track of the JWT id_token received during access token exchanges. For more details about the JWT id_token, see: http://openid.net/specs/oauth-v2-multiple-response-types-1_0-08.html#id_token And the Google OAuth APIs: https://developers.google.com/accounts/docs/OAuth2Login#exchangecode R=adg, golang-dev, bradfitz CC=golang-dev https://codereview.appspot.com/12906045 Committer: Andrew Gerrand <adg@golang.org>