Expose OAuth2 errors, avoid redirect loop.

Closes #1775.
This commit is contained in:
Alexey Palazhchenko 2016-12-19 08:42:56 +03:00
parent b83a719e37
commit e259c64bac
7 changed files with 64 additions and 6 deletions

View file

@ -1,6 +1,7 @@
package bitbucket package bitbucket
import ( import (
"errors"
"fmt" "fmt"
"net/http" "net/http"
"net/url" "net/url"
@ -39,13 +40,27 @@ func New(client, secret string) remote.Remote {
// Login authenticates an account with Bitbucket using the oauth2 protocol. The // Login authenticates an account with Bitbucket using the oauth2 protocol. The
// Bitbucket account details are returned when the user is successfully authenticated. // Bitbucket account details are returned when the user is successfully authenticated.
func (c *config) Login(w http.ResponseWriter, r *http.Request) (*model.User, error) { func (c *config) Login(w http.ResponseWriter, req *http.Request) (*model.User, error) {
redirect := httputil.GetURL(r) redirect := httputil.GetURL(req)
config := c.newConfig(redirect) 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 { 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 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 // Hook parses the incoming Bitbucket hook and returns the Repository and
// Build details. If the hook is unsupported nil values are returned. // Build details. If the hook is unsupported nil values are returned.
func (c *config) Hook(r *http.Request) (*model.Repo, *model.Build, error) { func (c *config) Hook(req *http.Request) (*model.Repo, *model.Build, error) {
return parseHook(r) return parseHook(req)
} }
// helper function to return the bitbucket oauth2 client // helper function to return the bitbucket oauth2 client

View file

@ -70,6 +70,11 @@ func Test_bitbucket(t *testing.T) {
_, err := c.Login(nil, r) _, err := c.Login(nil, r)
g.Assert(err != nil).IsTrue() 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() { g.Describe("Given an access token", func() {

View file

@ -27,6 +27,11 @@ func Handler() http.Handler {
} }
func getOauth(c *gin.Context) { func getOauth(c *gin.Context) {
switch c.PostForm("error") {
case "invalid_scope":
c.String(500, "")
}
switch c.PostForm("code") { switch c.PostForm("code") {
case "code_bad_request": case "code_bad_request":
c.String(500, "") c.String(500, "")

View file

@ -2,6 +2,7 @@ package github
import ( import (
"crypto/tls" "crypto/tls"
"errors"
"fmt" "fmt"
"net" "net"
"net/http" "net/http"
@ -92,6 +93,20 @@ type client struct {
func (c *client) Login(res http.ResponseWriter, req *http.Request) (*model.User, error) { func (c *client) Login(res http.ResponseWriter, req *http.Request) (*model.User, error) {
config := c.newConfig(httputil.GetURL(req)) 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") code := req.FormValue("code")
if len(code) == 0 { if len(code) == 0 {
// TODO(bradrydzewski) we really should be using a random value here and // TODO(bradrydzewski) we really should be using a random value here and

View file

@ -140,6 +140,7 @@ func Test_github(t *testing.T) {
g.It("Should create an access token") g.It("Should create an access token")
g.It("Should handle an access token error") g.It("Should handle an access token error")
g.It("Should return the authenticated user") g.It("Should return the authenticated user")
g.It("Should handle authentication errors")
}) })
}) })
} }

View file

@ -2,6 +2,7 @@ package gitlab
import ( import (
"crypto/tls" "crypto/tls"
"errors"
"fmt" "fmt"
"io/ioutil" "io/ioutil"
"net" "net"
@ -115,6 +116,19 @@ func (g *Gitlab) Login(res http.ResponseWriter, req *http.Request) (*model.User,
TLSClientConfig: &tls.Config{InsecureSkipVerify: g.SkipVerify}, 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 // get the OAuth code
var code = req.FormValue("code") var code = req.FormValue("code")
if len(code) == 0 { if len(code) == 0 {

View file

@ -218,6 +218,9 @@ func (c *Config) AuthCodeURL(state string) string {
if err != nil { if err != nil {
panic("AuthURL malformed: " + err.Error()) panic("AuthURL malformed: " + err.Error())
} }
if err := url_.Query().Get("error"); err != "" {
panic("AuthURL contains error: " + err)
}
q := url.Values{ q := url.Values{
"response_type": {"code"}, "response_type": {"code"},
"client_id": {c.ClientId}, "client_id": {c.ClientId},