From 1b22acca3032cd832c52f5859871ef1c8937171e Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 12 Aug 2014 14:03:22 -0700 Subject: [PATCH] Update goauth2 to f06a85362aa5 Change-Id: I581d449099b6201dc78593c3394fa3ae0954e0c3 changeset: 75:f06a85362aa5 tag: tip user: Brad Fitzpatrick date: Tue Aug 12 13:58:32 2014 -0700 summary: oauth: clean up docs, code, fix data race, don't send client_secret in two places --- .../code.google.com/p/goauth2/oauth/oauth.go | 120 ++++++++++++------ .../p/goauth2/oauth/oauth_test.go | 19 ++- 2 files changed, 95 insertions(+), 44 deletions(-) diff --git a/third_party/code.google.com/p/goauth2/oauth/oauth.go b/third_party/code.google.com/p/goauth2/oauth/oauth.go index 25ff2a09a..317d88e91 100644 --- a/third_party/code.google.com/p/goauth2/oauth/oauth.go +++ b/third_party/code.google.com/p/goauth2/oauth/oauth.go @@ -2,8 +2,7 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -// The oauth package provides support for making -// OAuth2-authenticated HTTP requests. +// Package oauth supports making OAuth2-authenticated HTTP requests. // // Example usage: // @@ -39,15 +38,23 @@ package oauth import ( "encoding/json" + "errors" + "fmt" + "io" "io/ioutil" "mime" "net/http" "net/url" "os" + "strconv" "strings" + "sync" "time" ) +// OAuthError is the error type returned by many operations. +// +// In retrospect it should not exist. Don't depend on it. type OAuthError struct { prefix string msg string @@ -123,7 +130,16 @@ type Config struct { // TokenCache allows tokens to be cached for subsequent requests. TokenCache Cache - AccessType string // Optional, "online" (default) or "offline", no refresh token if "online" + // AccessType is an OAuth extension that gets sent as the + // "access_type" field in the URL from AuthCodeURL. + // See https://developers.google.com/accounts/docs/OAuth2WebServer. + // It may be "online" (the default) or "offline". + // If your application needs to refresh access tokens when the + // user is not present at the browser, then use offline. This + // will result in your application obtaining a refresh token + // the first time your application exchanges an authorization + // code for a user. + AccessType string // ApprovalPrompt indicates whether the user should be // re-prompted for consent. If set to "auto" (default) the @@ -140,12 +156,18 @@ type Config struct { type Token struct { AccessToken string RefreshToken string - Expiry time.Time // If zero the token has no (known) expiry time. - Extra map[string]string // May be nil. + Expiry time.Time // If zero the token has no (known) expiry time. + + // Extra optionally contains extra metadata from the server + // when updating a token. The only current key that may be + // populated is "id_token". It may be nil and will be + // initialized as needed. + Extra map[string]string } +// Expired reports whether the token has expired or is invalid. func (t *Token) Expired() bool { - if t.Expiry.IsZero() { + if t.Expiry.IsZero() || t.AccessToken == "" { return false } return t.Expiry.Before(time.Now()) @@ -165,6 +187,9 @@ type Transport struct { *Config *Token + // mu guards modifying the token. + mu sync.Mutex + // Transport is the HTTP transport to use when making requests. // It will default to http.DefaultTransport if nil. // (It should never be an oauth.Transport.) @@ -247,35 +272,48 @@ func (t *Transport) Exchange(code string) (*Token, error) { // If the Token is invalid callers should expect HTTP-level errors, // as indicated by the Response's StatusCode. func (t *Transport) RoundTrip(req *http.Request) (*http.Response, error) { + accessToken, err := t.getAccessToken() + if err != nil { + return nil, err + } + // To set the Authorization header, we must make a copy of the Request + // so that we don't modify the Request we were given. + // This is required by the specification of http.RoundTripper. + req = cloneRequest(req) + req.Header.Set("Authorization", "Bearer "+accessToken) + + // Make the HTTP request. + return t.transport().RoundTrip(req) +} + +func (t *Transport) getAccessToken() (string, error) { + t.mu.Lock() + defer t.mu.Unlock() + if t.Token == nil { if t.Config == nil { - return nil, OAuthError{"RoundTrip", "no Config supplied"} + return "", OAuthError{"RoundTrip", "no Config supplied"} } if t.TokenCache == nil { - return nil, OAuthError{"RoundTrip", "no Token supplied"} + return "", OAuthError{"RoundTrip", "no Token supplied"} } var err error t.Token, err = t.TokenCache.Token() if err != nil { - return nil, err + return "", err } } // Refresh the Token if it has expired. if t.Expired() { if err := t.Refresh(); err != nil { - return nil, err + return "", err } } - - // To set the Authorization header, we must make a copy of the Request - // so that we don't modify the Request we were given. - // This is required by the specification of http.RoundTripper. - req = cloneRequest(req) - req.Header.Set("Authorization", "Bearer "+t.AccessToken) - - // Make the HTTP request. - return t.transport().RoundTrip(req) + if t.AccessToken == "" { + return "", errors.New("no access token obtained from refresh") + } + return t.AccessToken, nil } // cloneRequest returns a clone of the provided *http.Request. @@ -329,9 +367,16 @@ func (t *Transport) AuthenticateClient() error { return t.updateToken(t.Token, url.Values{"grant_type": {"client_credentials"}}) } +// updateToken mutates both tok and v. func (t *Transport) updateToken(tok *Token, v url.Values) error { v.Set("client_id", t.ClientId) - v.Set("client_secret", t.ClientSecret) + // Note that we're not setting v's client_secret to t.ClientSecret, due + // to https://code.google.com/p/goauth2/issues/detail?id=31 + // Reddit only accepts client_secret in Authorization header. + // Dropbox accepts either, but not both. + // The spec requires servers to always support the Authorization header, + // so that's all we use. + client := &http.Client{Transport: t.transport()} req, err := http.NewRequest("POST", t.TokenURL, strings.NewReader(v.Encode())) if err != nil { @@ -345,22 +390,23 @@ func (t *Transport) updateToken(tok *Token, v url.Values) error { } defer r.Body.Close() if r.StatusCode != 200 { - return OAuthError{"updateToken", r.Status} + return OAuthError{"updateToken", "Unexpected HTTP status " + r.Status} } var b struct { - Access string `json:"access_token"` - Refresh string `json:"refresh_token"` - ExpiresIn time.Duration `json:"expires_in"` - Id string `json:"id_token"` + Access string `json:"access_token"` + Refresh string `json:"refresh_token"` + ExpiresIn int64 `json:"expires_in"` // seconds + Id string `json:"id_token"` + } + + body, err := ioutil.ReadAll(io.LimitReader(r.Body, 1<<20)) + if err != nil { + return err } content, _, _ := mime.ParseMediaType(r.Header.Get("Content-Type")) switch content { case "application/x-www-form-urlencoded", "text/plain": - body, err := ioutil.ReadAll(r.Body) - if err != nil { - return err - } vals, err := url.ParseQuery(string(body)) if err != nil { return err @@ -368,25 +414,25 @@ func (t *Transport) updateToken(tok *Token, v url.Values) error { b.Access = vals.Get("access_token") b.Refresh = vals.Get("refresh_token") - b.ExpiresIn, _ = time.ParseDuration(vals.Get("expires_in") + "s") + b.ExpiresIn, _ = strconv.ParseInt(vals.Get("expires_in"), 10, 64) b.Id = vals.Get("id_token") default: - if err = json.NewDecoder(r.Body).Decode(&b); err != nil { - return err + if err = json.Unmarshal(body, &b); err != nil { + return fmt.Errorf("got bad response from server: %q", body) } - // The JSON parser treats the unitless ExpiresIn like 'ns' instead of 's' as above, - // so compensate here. - b.ExpiresIn *= time.Second + } + if b.Access == "" { + return errors.New("received empty access token from authorization server") } tok.AccessToken = b.Access // Don't overwrite `RefreshToken` with an empty value - if len(b.Refresh) > 0 { + if b.Refresh != "" { tok.RefreshToken = b.Refresh } if b.ExpiresIn == 0 { tok.Expiry = time.Time{} } else { - tok.Expiry = time.Now().Add(b.ExpiresIn) + tok.Expiry = time.Now().Add(time.Duration(b.ExpiresIn) * time.Second) } if b.Id != "" { if tok.Extra == nil { diff --git a/third_party/code.google.com/p/goauth2/oauth/oauth_test.go b/third_party/code.google.com/p/goauth2/oauth/oauth_test.go index b903c1607..81d4992a1 100644 --- a/third_party/code.google.com/p/goauth2/oauth/oauth_test.go +++ b/third_party/code.google.com/p/goauth2/oauth/oauth_test.go @@ -23,7 +23,7 @@ var requests = []struct { }{ { path: "/token", - query: "grant_type=authorization_code&code=c0d3&client_id=cl13nt1d&client_secret=s3cr3t", + query: "grant_type=authorization_code&code=c0d3&client_id=cl13nt1d", contenttype: "application/json", auth: "Basic Y2wxM250MWQ6czNjcjN0", body: ` @@ -38,7 +38,7 @@ var requests = []struct { {path: "/secure", auth: "Bearer token1", body: "first payload"}, { path: "/token", - query: "grant_type=refresh_token&refresh_token=refreshtoken1&client_id=cl13nt1d&client_secret=s3cr3t", + query: "grant_type=refresh_token&refresh_token=refreshtoken1&client_id=cl13nt1d", contenttype: "application/json", auth: "Basic Y2wxM250MWQ6czNjcjN0", body: ` @@ -53,7 +53,7 @@ var requests = []struct { {path: "/secure", auth: "Bearer token2", body: "second payload"}, { path: "/token", - query: "grant_type=refresh_token&refresh_token=refreshtoken2&client_id=cl13nt1d&client_secret=s3cr3t", + query: "grant_type=refresh_token&refresh_token=refreshtoken2&client_id=cl13nt1d", contenttype: "application/x-www-form-urlencoded", body: "access_token=token3&refresh_token=refreshtoken3&id_token=idtoken3&expires_in=3600", auth: "Basic Y2wxM250MWQ6czNjcjN0", @@ -61,7 +61,7 @@ var requests = []struct { {path: "/secure", auth: "Bearer token3", body: "third payload"}, { path: "/token", - query: "grant_type=client_credentials&client_id=cl13nt1d&client_secret=s3cr3t", + query: "grant_type=client_credentials&client_id=cl13nt1d", contenttype: "application/json", auth: "Basic Y2wxM250MWQ6czNjcjN0", body: ` @@ -171,9 +171,14 @@ func checkToken(t *testing.T, tok *Token, access, refresh, id string) { if g, w := tok.Extra["id_token"], id; g != w { t.Errorf("Extra['id_token'] = %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) + if tok.Expiry.IsZero() { + t.Errorf("Expiry is zero; want ~1 hour") + } else { + exp := tok.Expiry.Sub(time.Now()) + const slop = 3 * time.Second // time moving during test + if (time.Hour-slop) > exp || exp > time.Hour { + t.Errorf("Expiry = %v, want ~1 hour", exp) + } } }