From e259c64bac16d9b47b3a9bd8842d4a9b2e15cf49 Mon Sep 17 00:00:00 2001 From: Alexey Palazhchenko Date: Mon, 19 Dec 2016 08:42:56 +0300 Subject: [PATCH 1/2] Expose OAuth2 errors, avoid redirect loop. Closes #1775. --- remote/bitbucket/bitbucket.go | 27 +++++++++++++++++++++------ remote/bitbucket/bitbucket_test.go | 5 +++++ remote/bitbucket/fixtures/handler.go | 5 +++++ remote/github/github.go | 15 +++++++++++++++ remote/github/github_test.go | 1 + remote/gitlab/gitlab.go | 14 ++++++++++++++ shared/oauth2/oauth2.go | 3 +++ 7 files changed, 64 insertions(+), 6 deletions(-) diff --git a/remote/bitbucket/bitbucket.go b/remote/bitbucket/bitbucket.go index cff6bdfb7f..a03f8ab716 100644 --- a/remote/bitbucket/bitbucket.go +++ b/remote/bitbucket/bitbucket.go @@ -1,6 +1,7 @@ package bitbucket import ( + "errors" "fmt" "net/http" "net/url" @@ -39,13 +40,27 @@ func New(client, secret string) remote.Remote { // Login authenticates an account with Bitbucket using the oauth2 protocol. The // Bitbucket account details are returned when the user is successfully authenticated. -func (c *config) Login(w http.ResponseWriter, r *http.Request) (*model.User, error) { - redirect := httputil.GetURL(r) +func (c *config) Login(w http.ResponseWriter, req *http.Request) (*model.User, error) { + redirect := httputil.GetURL(req) config := c.newConfig(redirect) - code := r.FormValue("code") + // get the OAuth errors + if err := req.FormValue("error"); err != "" { + description := req.FormValue("error_description") + if description != "" { + err += " " + description + } + uri := req.FormValue("error_uri") + if uri != "" { + err += " " + uri + } + return nil, errors.New(err) + } + + // get the OAuth code + code := req.FormValue("code") if len(code) == 0 { - http.Redirect(w, r, config.AuthCodeURL("drone"), http.StatusSeeOther) + http.Redirect(w, req, config.AuthCodeURL("drone"), http.StatusSeeOther) return nil, nil } @@ -237,8 +252,8 @@ func (c *config) Netrc(u *model.User, r *model.Repo) (*model.Netrc, error) { // Hook parses the incoming Bitbucket hook and returns the Repository and // Build details. If the hook is unsupported nil values are returned. -func (c *config) Hook(r *http.Request) (*model.Repo, *model.Build, error) { - return parseHook(r) +func (c *config) Hook(req *http.Request) (*model.Repo, *model.Build, error) { + return parseHook(req) } // helper function to return the bitbucket oauth2 client diff --git a/remote/bitbucket/bitbucket_test.go b/remote/bitbucket/bitbucket_test.go index ea1a8281a6..f00b19875b 100644 --- a/remote/bitbucket/bitbucket_test.go +++ b/remote/bitbucket/bitbucket_test.go @@ -70,6 +70,11 @@ func Test_bitbucket(t *testing.T) { _, err := c.Login(nil, r) g.Assert(err != nil).IsTrue() }) + g.It("Should handle authentication errors", func() { + r, _ := http.NewRequest("GET", "?error=invalid_scope", nil) + _, err := c.Login(nil, r) + g.Assert(err != nil).IsTrue() + }) }) g.Describe("Given an access token", func() { diff --git a/remote/bitbucket/fixtures/handler.go b/remote/bitbucket/fixtures/handler.go index f5cbbb40ec..96aff73e0a 100644 --- a/remote/bitbucket/fixtures/handler.go +++ b/remote/bitbucket/fixtures/handler.go @@ -27,6 +27,11 @@ func Handler() http.Handler { } func getOauth(c *gin.Context) { + switch c.PostForm("error") { + case "invalid_scope": + c.String(500, "") + } + switch c.PostForm("code") { case "code_bad_request": c.String(500, "") diff --git a/remote/github/github.go b/remote/github/github.go index ba69cac563..fec7837037 100644 --- a/remote/github/github.go +++ b/remote/github/github.go @@ -2,6 +2,7 @@ package github import ( "crypto/tls" + "errors" "fmt" "net" "net/http" @@ -92,6 +93,20 @@ type client struct { func (c *client) Login(res http.ResponseWriter, req *http.Request) (*model.User, error) { config := c.newConfig(httputil.GetURL(req)) + // get the OAuth errors + if err := req.FormValue("error"); err != "" { + description := req.FormValue("error_description") + if description != "" { + err += " " + description + } + uri := req.FormValue("error_uri") + if uri != "" { + err += " " + uri + } + return nil, errors.New(err) + } + + // get the OAuth code code := req.FormValue("code") if len(code) == 0 { // TODO(bradrydzewski) we really should be using a random value here and diff --git a/remote/github/github_test.go b/remote/github/github_test.go index 83e41f60d8..8ba0a949bf 100644 --- a/remote/github/github_test.go +++ b/remote/github/github_test.go @@ -140,6 +140,7 @@ func Test_github(t *testing.T) { g.It("Should create an access token") g.It("Should handle an access token error") g.It("Should return the authenticated user") + g.It("Should handle authentication errors") }) }) } diff --git a/remote/gitlab/gitlab.go b/remote/gitlab/gitlab.go index 543b32bb89..c41993ef22 100644 --- a/remote/gitlab/gitlab.go +++ b/remote/gitlab/gitlab.go @@ -2,6 +2,7 @@ package gitlab import ( "crypto/tls" + "errors" "fmt" "io/ioutil" "net" @@ -115,6 +116,19 @@ func (g *Gitlab) Login(res http.ResponseWriter, req *http.Request) (*model.User, TLSClientConfig: &tls.Config{InsecureSkipVerify: g.SkipVerify}, } + // get the OAuth errors + if err := req.FormValue("error"); err != "" { + description := req.FormValue("error_description") + if description != "" { + err += " " + description + } + uri := req.FormValue("error_uri") + if uri != "" { + err += " " + uri + } + return nil, errors.New(err) + } + // get the OAuth code var code = req.FormValue("code") if len(code) == 0 { diff --git a/shared/oauth2/oauth2.go b/shared/oauth2/oauth2.go index 97059c4993..d69d74848a 100644 --- a/shared/oauth2/oauth2.go +++ b/shared/oauth2/oauth2.go @@ -218,6 +218,9 @@ func (c *Config) AuthCodeURL(state string) string { if err != nil { panic("AuthURL malformed: " + err.Error()) } + if err := url_.Query().Get("error"); err != "" { + panic("AuthURL contains error: " + err) + } q := url.Values{ "response_type": {"code"}, "client_id": {c.ClientId}, From b230afe7f56e286a6f7d88e386e835d4a0af405b Mon Sep 17 00:00:00 2001 From: Alexey Palazhchenko Date: Mon, 19 Dec 2016 19:22:11 +0300 Subject: [PATCH 2/2] Add AuthError type, use it. --- remote/bitbucket/bitbucket.go | 13 ++++--------- remote/errors.go | 23 +++++++++++++++++++++++ remote/github/github.go | 13 ++++--------- remote/gitlab/gitlab.go | 13 ++++--------- 4 files changed, 35 insertions(+), 27 deletions(-) create mode 100644 remote/errors.go diff --git a/remote/bitbucket/bitbucket.go b/remote/bitbucket/bitbucket.go index a03f8ab716..a272fa8846 100644 --- a/remote/bitbucket/bitbucket.go +++ b/remote/bitbucket/bitbucket.go @@ -1,7 +1,6 @@ package bitbucket import ( - "errors" "fmt" "net/http" "net/url" @@ -46,15 +45,11 @@ func (c *config) Login(w http.ResponseWriter, req *http.Request) (*model.User, e // get the OAuth errors if err := req.FormValue("error"); err != "" { - description := req.FormValue("error_description") - if description != "" { - err += " " + description + return nil, &remote.AuthError{ + Err: err, + Description: req.FormValue("error_description"), + URI: req.FormValue("error_uri"), } - uri := req.FormValue("error_uri") - if uri != "" { - err += " " + uri - } - return nil, errors.New(err) } // get the OAuth code diff --git a/remote/errors.go b/remote/errors.go new file mode 100644 index 0000000000..0ad31e7691 --- /dev/null +++ b/remote/errors.go @@ -0,0 +1,23 @@ +package remote + +// AuthError represents remote authentication error. +type AuthError struct { + Err string + Description string + URI string +} + +// Error implements error interface. +func (ae *AuthError) Error() string { + err := ae.Err + if ae.Description != "" { + err += " " + ae.Description + } + if ae.URI != "" { + err += " " + ae.URI + } + return err +} + +// check interface +var _ error = new(AuthError) diff --git a/remote/github/github.go b/remote/github/github.go index fec7837037..7b38278b19 100644 --- a/remote/github/github.go +++ b/remote/github/github.go @@ -2,7 +2,6 @@ package github import ( "crypto/tls" - "errors" "fmt" "net" "net/http" @@ -95,15 +94,11 @@ func (c *client) Login(res http.ResponseWriter, req *http.Request) (*model.User, // get the OAuth errors if err := req.FormValue("error"); err != "" { - description := req.FormValue("error_description") - if description != "" { - err += " " + description + return nil, &remote.AuthError{ + Err: err, + Description: req.FormValue("error_description"), + URI: req.FormValue("error_uri"), } - uri := req.FormValue("error_uri") - if uri != "" { - err += " " + uri - } - return nil, errors.New(err) } // get the OAuth code diff --git a/remote/gitlab/gitlab.go b/remote/gitlab/gitlab.go index c41993ef22..5c0192023f 100644 --- a/remote/gitlab/gitlab.go +++ b/remote/gitlab/gitlab.go @@ -2,7 +2,6 @@ package gitlab import ( "crypto/tls" - "errors" "fmt" "io/ioutil" "net" @@ -118,15 +117,11 @@ func (g *Gitlab) Login(res http.ResponseWriter, req *http.Request) (*model.User, // get the OAuth errors if err := req.FormValue("error"); err != "" { - description := req.FormValue("error_description") - if description != "" { - err += " " + description + return nil, &remote.AuthError{ + Err: err, + Description: req.FormValue("error_description"), + URI: req.FormValue("error_uri"), } - uri := req.FormValue("error_uri") - if uri != "" { - err += " " + uri - } - return nil, errors.New(err) } // get the OAuth code