From c85d3e98c914e3a33234ad863dcbff5dbc425bb8 Mon Sep 17 00:00:00 2001 From: Ggicci Date: Mon, 18 Mar 2019 08:32:44 +0800 Subject: [PATCH] internal: remove fallback parsing for expires_in Facebook has correctted its OAuth2 implementation. The code as a fallback can be removed now. Updates golang/oauth2#51, golang/oauth2#239 Change-Id: Ib5f84bc35c0c4ecbdd25d4169f950410d4ae79a2 Reviewed-on: https://go-review.googlesource.com/c/oauth2/+/168017 Reviewed-by: Brad Fitzpatrick Reviewed-by: JBD Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot --- facebook/facebook.go | 4 ++-- internal/token.go | 10 ---------- oauth2_test.go | 32 ++++++++++++++++++-------------- 3 files changed, 20 insertions(+), 26 deletions(-) diff --git a/facebook/facebook.go b/facebook/facebook.go index 21c49e7..b0054e3 100644 --- a/facebook/facebook.go +++ b/facebook/facebook.go @@ -11,6 +11,6 @@ import ( // Endpoint is Facebook's OAuth 2.0 endpoint. var Endpoint = oauth2.Endpoint{ - AuthURL: "https://www.facebook.com/v3.1/dialog/oauth", - TokenURL: "https://graph.facebook.com/v3.1/oauth/access_token", + AuthURL: "https://www.facebook.com/v3.2/dialog/oauth", + TokenURL: "https://graph.facebook.com/v3.2/oauth/access_token", } diff --git a/internal/token.go b/internal/token.go index 83f7847..355c386 100644 --- a/internal/token.go +++ b/internal/token.go @@ -63,16 +63,12 @@ type tokenJSON struct { TokenType string `json:"token_type"` RefreshToken string `json:"refresh_token"` ExpiresIn expirationTime `json:"expires_in"` // at least PayPal returns string, while most return number - Expires expirationTime `json:"expires"` // broken Facebook spelling of expires_in } func (e *tokenJSON) expiry() (t time.Time) { if v := e.ExpiresIn; v != 0 { return time.Now().Add(time.Duration(v) * time.Second) } - if v := e.Expires; v != 0 { - return time.Now().Add(time.Duration(v) * time.Second) - } return } @@ -264,12 +260,6 @@ func doTokenRoundTrip(ctx context.Context, req *http.Request) (*Token, error) { Raw: vals, } e := vals.Get("expires_in") - if e == "" || e == "null" { - // TODO(jbd): Facebook's OAuth2 implementation is broken and - // returns expires_in field in expires. Remove the fallback to expires, - // when Facebook fixes their implementation. - e = vals.Get("expires") - } expires, _ := strconv.Atoi(e) if expires != 0 { token.Expiry = time.Now().Add(time.Duration(expires) * time.Second) diff --git a/oauth2_test.go b/oauth2_test.go index b76eaae..588600b 100644 --- a/oauth2_test.go +++ b/oauth2_test.go @@ -275,26 +275,26 @@ const day = 24 * time.Hour func TestExchangeRequest_JSONResponse_Expiry(t *testing.T) { seconds := int32(day.Seconds()) for _, c := range []struct { - name string - expires string - want bool + name string + expires string + want bool + nullExpires bool }{ - {"normal", fmt.Sprintf(`"expires_in": %d`, seconds), true}, - {"paypal", fmt.Sprintf(`"expires_in": "%d"`, seconds), true}, - {"facebook", fmt.Sprintf(`"expires": %d`, seconds), true}, - {"issue_239", fmt.Sprintf(`"expires_in": null, "expires": %d`, seconds), true}, + {"normal", fmt.Sprintf(`"expires_in": %d`, seconds), true, false}, + {"paypal", fmt.Sprintf(`"expires_in": "%d"`, seconds), true, false}, + {"issue_239", fmt.Sprintf(`"expires_in": null`), true, true}, - {"wrong_type", `"expires": false`, false}, - {"wrong_type2", `"expires": {}`, false}, - {"wrong_value", `"expires": "zzz"`, false}, + {"wrong_type", `"expires_in": false`, false, false}, + {"wrong_type2", `"expires_in": {}`, false, false}, + {"wrong_value", `"expires_in": "zzz"`, false, false}, } { t.Run(c.name, func(t *testing.T) { - testExchangeRequest_JSONResponse_expiry(t, c.expires, c.want) + testExchangeRequest_JSONResponse_expiry(t, c.expires, c.want, c.nullExpires) }) } } -func testExchangeRequest_JSONResponse_expiry(t *testing.T, exp string, want bool) { +func testExchangeRequest_JSONResponse_expiry(t *testing.T, exp string, want, nullExpires bool) { ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") w.Write([]byte(fmt.Sprintf(`{"access_token": "90d", "scope": "user", "token_type": "bearer", %s}`, exp))) @@ -303,7 +303,7 @@ func testExchangeRequest_JSONResponse_expiry(t *testing.T, exp string, want bool conf := newConf(ts.URL) t1 := time.Now().Add(day) tok, err := conf.Exchange(context.Background(), "exchange-code") - t2 := time.Now().Add(day) + t2 := t1.Add(day) if got := (err == nil); got != want { if want { @@ -319,8 +319,12 @@ func testExchangeRequest_JSONResponse_expiry(t *testing.T, exp string, want bool t.Fatalf("Token invalid. Got: %#v", tok) } expiry := tok.Expiry + + if nullExpires && expiry.IsZero() { + return + } if expiry.Before(t1) || expiry.After(t2) { - t.Errorf("Unexpected value for Expiry: %v (shold be between %v and %v)", expiry, t1, t2) + t.Errorf("Unexpected value for Expiry: %v (should be between %v and %v)", expiry, t1, t2) } }