increased bitbucket test coverage

remote/bitbucket/bitbucket.go

@@ -11,13 +11,16 @@ import (
 	"github.com/drone/drone/shared/httputil"
 
 	"golang.org/x/oauth2"
-	"golang.org/x/oauth2/bitbucket"
 )
 
-// Bitbucket Server endpoint.
-const Endpoint = "https://api.bitbucket.org"
+// Bitbucket cloud endpoints.
+const (
+	DefaultAPI = "https://api.bitbucket.org"
+	DefaultURL = "https://bitbucket.org"
+)
 
 type config struct {
+	API    string
 	URL    string
 	Client string
 	Secret string
@@ -27,63 +30,42 @@ type config struct {
 // repository hosting service at https://bitbucket.org
 func New(client, secret string) remote.Remote {
 	return &config{
-		URL:    Endpoint,
+		API:    DefaultAPI,
+		URL:    DefaultURL,
 		Client: client,
 		Secret: secret,
 	}
 }
 
-// helper function to return the bitbucket oauth2 client
-func (c *config) newClient(u *model.User) *internal.Client {
-	return internal.NewClientToken(
-		c.URL,
-		c.Client,
-		c.Secret,
-		&oauth2.Token{
-			AccessToken:  u.Token,
-			RefreshToken: u.Secret,
-		},
-	)
-}
+// 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)
+	config := c.newConfig(redirect)
 
-func (c *config) Login(res http.ResponseWriter, req *http.Request) (*model.User, error) {
-	config := &oauth2.Config{
-		ClientID:     c.Client,
-		ClientSecret: c.Secret,
-		Endpoint:     bitbucket.Endpoint,
-		RedirectURL:  fmt.Sprintf("%s/authorize", httputil.GetURL(req)),
-	}
-
-	var code = req.FormValue("code")
+	code := r.FormValue("code")
 	if len(code) == 0 {
-		http.Redirect(res, req, config.AuthCodeURL("drone"), http.StatusSeeOther)
+		http.Redirect(w, r, config.AuthCodeURL("drone"), http.StatusSeeOther)
 		return nil, nil
 	}
 
-	var token, err = config.Exchange(oauth2.NoContext, code)
+	token, err := config.Exchange(oauth2.NoContext, code)
 	if err != nil {
 		return nil, err
 	}
 
-	client := internal.NewClient(c.URL, config.Client(oauth2.NoContext, token))
+	client := internal.NewClient(c.API, config.Client(oauth2.NoContext, token))
 	curr, err := client.FindCurrent()
 	if err != nil {
 		return nil, err
 	}
-
 	return convertUser(curr, token), nil
 }
 
+// Auth uses the Bitbucket oauth2 access token and refresh token to authenticate
+// a session and return the Bitbucket account login.
 func (c *config) Auth(token, secret string) (string, error) {
-	client := internal.NewClientToken(
-		c.URL,
-		c.Client,
-		c.Secret,
-		&oauth2.Token{
-			AccessToken:  token,
-			RefreshToken: secret,
-		},
-	)
+	client := c.newClientToken(token, secret)
 	user, err := client.FindCurrent()
 	if err != nil {
 		return "", err
@@ -91,33 +73,25 @@ func (c *config) Auth(token, secret string) (string, error) {
 	return user.Login, nil
 }
 
+// Refresh refreshes the Bitbucket oauth2 access token. If the token is
+// refreshed the user is updated and a true value is returned.
 func (c *config) Refresh(user *model.User) (bool, error) {
-	config := &oauth2.Config{
-		ClientID:     c.Client,
-		ClientSecret: c.Secret,
-		Endpoint:     bitbucket.Endpoint,
-	}
-
-	// creates a token source with just the refresh token.
-	// this will ensure an access token is automatically
-	// requested.
+	config := c.newConfig("")
 	source := config.TokenSource(
 		oauth2.NoContext, &oauth2.Token{RefreshToken: user.Secret})
 
-	// requesting the token automatically refreshes and
-	// returns a new access token.
 	token, err := source.Token()
 	if err != nil || len(token.AccessToken) == 0 {
 		return false, err
 	}
 
-	// update the user to include tne new access token
 	user.Token = token.AccessToken
 	user.Secret = token.RefreshToken
 	user.Expiry = token.Expiry.UTC().Unix()
 	return true, nil
 }
 
+// Teams returns a list of all team membership for the Bitbucket account.
 func (c *config) Teams(u *model.User) ([]*model.Team, error) {
 	opts := &internal.ListTeamOpts{
 		PageLen: 100,
@@ -130,6 +104,7 @@ func (c *config) Teams(u *model.User) ([]*model.Team, error) {
 	return convertTeamList(resp.Values), nil
 }
 
+// Repo returns the named Bitbucket repository.
 func (c *config) Repo(u *model.User, owner, name string) (*model.Repo, error) {
 	repo, err := c.newClient(u).FindRepo(owner, name)
 	if err != nil {
@@ -138,36 +113,43 @@ func (c *config) Repo(u *model.User, owner, name string) (*model.Repo, error) {
 	return convertRepo(repo), nil
 }
 
+// Repos returns a list of all repositories for Bitbucket account, including
+// organization repositories.
 func (c *config) Repos(u *model.User) ([]*model.RepoLite, error) {
 	client := c.newClient(u)
 
-	var repos []*model.RepoLite
+	var all []*model.RepoLite
 
-	// gets a list of all accounts to query, including the
-	// user's account and all team accounts.
-	logins := []string{u.Login}
-	resp, err := client.ListTeams(&internal.ListTeamOpts{PageLen: 100, Role: "member"})
+	accounts := []string{u.Login}
+	resp, err := client.ListTeams(&internal.ListTeamOpts{
+		PageLen: 100,
+		Role:    "member",
+	})
 	if err != nil {
-		return repos, err
+		return all, err
 	}
 	for _, team := range resp.Values {
-		logins = append(logins, team.Login)
+		accounts = append(accounts, team.Login)
 	}
 
 	// for each account, get the list of repos
-	for _, login := range logins {
-		repos_, err := client.ListReposAll(login)
+	for _, account := range accounts {
+		repos, err := client.ListReposAll(account)
 		if err != nil {
-			return repos, err
+			return all, err
 		}
-		for _, repo := range repos_ {
-			repos = append(repos, convertRepoLite(repo))
+		for _, repo := range repos {
+			all = append(all, convertRepoLite(repo))
 		}
 	}
 
-	return repos, nil
+	return all, nil
 }
 
+// Perm returns the user permissions for the named repository. Because Bitbucket
+// does not have an endpoint to access user permissions, we attempt to fetch
+// the repository hook list, which is restricted to administrators to calculate
+// administrative access to a repository.
 func (c *config) Perm(u *model.User, owner, name string) (*model.Perm, error) {
 	client := c.newClient(u)
 
@@ -177,20 +159,17 @@ func (c *config) Perm(u *model.User, owner, name string) (*model.Perm, error) {
 		return perms, err
 	}
 
-	// if we've gotten this far we know that the user at least has read access
-	// to the repository.
-	perms.Pull = true
-
-	// if the user has access to the repository hooks we can deduce that the user
-	// has push and admin access.
 	_, err = client.ListHooks(owner, name, &internal.ListOpts{})
 	if err == nil {
 		perms.Push = true
 		perms.Admin = true
 	}
+	perms.Pull = true
 	return perms, nil
 }
 
+// File fetches the file from the Bitbucket repository and returns its contents
+// in string format.
 func (c *config) File(u *model.User, r *model.Repo, b *model.Build, f string) ([]byte, error) {
 	config, err := c.newClient(u).FindSource(r.Owner, r.Name, b.Commit, f)
 	if err != nil {
@@ -199,6 +178,7 @@ func (c *config) File(u *model.User, r *model.Repo, b *model.Build, f string) ([
 	return []byte(config.Data), err
 }
 
+// Status creates a build status for the Bitbucket commit.
 func (c *config) Status(u *model.User, r *model.Repo, b *model.Build, link string) error {
 	status := internal.BuildStatus{
 		State: convertStatus(b.Status),
@@ -209,21 +189,14 @@ func (c *config) Status(u *model.User, r *model.Repo, b *model.Build, link strin
 	return c.newClient(u).CreateStatus(r.Owner, r.Name, b.Commit, &status)
 }
 
-func (c *config) Netrc(u *model.User, r *model.Repo) (*model.Netrc, error) {
-	return &model.Netrc{
-		Machine:  "bitbucket.org",
-		Login:    "x-token-auth",
-		Password: u.Token,
-	}, nil
-}
-
-func (c *config) Activate(u *model.User, r *model.Repo, k *model.Key, link string) error {
+// Activate activates the repository by registering repository push hooks with
+// the Bitbucket repository. Prior to registering hook, previously created hooks
+// are deleted.
+func (c *config) Activate(u *model.User, r *model.Repo, link string) error {
 	rawurl, err := url.Parse(link)
 	if err != nil {
 		return err
 	}
-
-	// deletes any previously created hooks
 	c.Deactivate(u, r, link)
 
 	return c.newClient(u).CreateHook(r.Owner, r.Name, &internal.Hook{
@@ -234,32 +207,80 @@ func (c *config) Activate(u *model.User, r *model.Repo, k *model.Key, link strin
 	})
 }
 
+// Deactivate deactives the repository be removing repository push hooks from
+// the Bitbucket repository.
 func (c *config) Deactivate(u *model.User, r *model.Repo, link string) error {
 	client := c.newClient(u)
 
-	linkurl, err := url.Parse(link)
-	if err != nil {
-		return err
-	}
-
 	hooks, err := client.ListHooks(r.Owner, r.Name, &internal.ListOpts{})
 	if err != nil {
 		return err
 	}
-
-	for _, hook := range hooks.Values {
-		hookurl, err := url.Parse(hook.Url)
-		if err == nil && hookurl.Host == linkurl.Host {
-			return client.DeleteHook(r.Owner, r.Name, hook.Uuid)
-		}
+	hook := matchingHooks(hooks.Values, link)
+	if hook != nil {
+		return client.DeleteHook(r.Owner, r.Name, hook.Uuid)
 	}
-
 	return nil
 }
 
+// Netrc returns a netrc file capable of authenticating Bitbucket requests and
+// cloning Bitbucket repositories.
+func (c *config) Netrc(u *model.User, r *model.Repo) (*model.Netrc, error) {
+	return &model.Netrc{
+		Machine:  "bitbucket.org",
+		Login:    "x-token-auth",
+		Password: u.Token,
+	}, nil
+}
+
 // Hook parses the incoming Bitbucket hook and returns the Repository and
-// Build details. If the hook is unsupported nil values are returned and the
-// hook should be skipped.
+// 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)
 }
+
+// helper function to return the bitbucket oauth2 client
+func (c *config) newClient(u *model.User) *internal.Client {
+	return c.newClientToken(u.Token, u.Secret)
+}
+
+// helper function to return the bitbucket oauth2 client
+func (c *config) newClientToken(token, secret string) *internal.Client {
+	return internal.NewClientToken(
+		c.API,
+		c.Client,
+		c.Secret,
+		&oauth2.Token{
+			AccessToken:  token,
+			RefreshToken: secret,
+		},
+	)
+}
+
+// helper function to return the bitbucket oauth2 config
+func (c *config) newConfig(redirect string) *oauth2.Config {
+	return &oauth2.Config{
+		ClientID:     c.Client,
+		ClientSecret: c.Secret,
+		Endpoint: oauth2.Endpoint{
+			AuthURL:  fmt.Sprintf("%s/site/oauth2/authorize", c.URL),
+			TokenURL: fmt.Sprintf("%s/site/oauth2/access_token", c.URL),
+		},
+		RedirectURL: fmt.Sprintf("%s/authorize", redirect),
+	}
+}
+
+// helper function to return matching hooks.
+func matchingHooks(hooks []*internal.Hook, rawurl string) *internal.Hook {
+	link, err := url.Parse(rawurl)
+	if err != nil {
+		return nil
+	}
+	for _, hook := range hooks {
+		hookurl, err := url.Parse(hook.Url)
+		if err == nil && hookurl.Host == link.Host {
+			return hook
+		}
+	}
+	return nil
+}

remote/bitbucket/bitbucket_test.go

@@ -8,6 +8,7 @@ import (
 
 	"github.com/drone/drone/model"
 	"github.com/drone/drone/remote/bitbucket/fixtures"
+	"github.com/drone/drone/remote/bitbucket/internal"
 
 	"github.com/franela/goblin"
 	"github.com/gin-gonic/gin"
@@ -17,7 +18,7 @@ func Test_bitbucket(t *testing.T) {
 	gin.SetMode(gin.TestMode)
 
 	s := httptest.NewServer(fixtures.Handler())
-	c := &config{URL: s.URL}
+	c := &config{URL: s.URL, API: s.URL}
 
 	g := goblin.Goblin(t)
 	g.Describe("Bitbucket client", func() {
@@ -28,10 +29,12 @@ func Test_bitbucket(t *testing.T) {
 
 		g.It("Should return client with default endpoint", func() {
 			remote := New("4vyW6b49Z", "a5012f6c6")
-			g.Assert(remote.(*config).URL).Equal("https://api.bitbucket.org")
+			g.Assert(remote.(*config).URL).Equal(DefaultURL)
+			g.Assert(remote.(*config).API).Equal(DefaultAPI)
 			g.Assert(remote.(*config).Client).Equal("4vyW6b49Z")
 			g.Assert(remote.(*config).Secret).Equal("a5012f6c6")
 		})
+
 		g.It("Should return the netrc file", func() {
 			remote := New("", "")
 			netrc, _ := remote.Netrc(fakeUser, nil)
@@ -40,6 +43,35 @@ func Test_bitbucket(t *testing.T) {
 			g.Assert(netrc.Password).Equal(fakeUser.Token)
 		})
 
+		g.Describe("Given an authorization request", func() {
+			g.It("Should redirect to authorize", func() {
+				w := httptest.NewRecorder()
+				r, _ := http.NewRequest("GET", "", nil)
+				_, err := c.Login(w, r)
+				g.Assert(err == nil).IsTrue()
+				g.Assert(w.Code).Equal(http.StatusSeeOther)
+			})
+			g.It("Should return authenticated user", func() {
+				r, _ := http.NewRequest("GET", "?code=code", nil)
+				u, err := c.Login(nil, r)
+				g.Assert(err == nil).IsTrue()
+				g.Assert(u.Login).Equal(fakeUser.Login)
+				g.Assert(u.Token).Equal("2YotnFZFEjr1zCsicMWpAA")
+				g.Assert(u.Secret).Equal("tGzv3JOkF0XG5Qx2TlKWIA")
+			})
+			g.It("Should handle failure to exchange code", func() {
+				w := httptest.NewRecorder()
+				r, _ := http.NewRequest("GET", "?code=code_bad_request", nil)
+				_, err := c.Login(w, r)
+				g.Assert(err != nil).IsTrue()
+			})
+			g.It("Should handle failure to resolve user", func() {
+				r, _ := http.NewRequest("GET", "?code=code_user_not_found", nil)
+				_, err := c.Login(nil, r)
+				g.Assert(err != nil).IsTrue()
+			})
+		})
+
 		g.Describe("Given an access token", func() {
 			g.It("Should return the authenticated user", func() {
 				login, err := c.Auth(
@@ -49,7 +81,7 @@ func Test_bitbucket(t *testing.T) {
 				g.Assert(err == nil).IsTrue()
 				g.Assert(login).Equal(fakeUser.Login)
 			})
-			g.It("Should return error when request fails", func() {
+			g.It("Should handle a failure to resolve user", func() {
 				_, err := c.Auth(
 					fakeUserNotFound.Token,
 					fakeUserNotFound.Secret,
@@ -58,6 +90,26 @@ func Test_bitbucket(t *testing.T) {
 			})
 		})
 
+		g.Describe("Given a refresh token", func() {
+			g.It("Should return a refresh access token", func() {
+				ok, err := c.Refresh(fakeUserRefresh)
+				g.Assert(err == nil).IsTrue()
+				g.Assert(ok).IsTrue()
+				g.Assert(fakeUserRefresh.Token).Equal("2YotnFZFEjr1zCsicMWpAA")
+				g.Assert(fakeUserRefresh.Secret).Equal("tGzv3JOkF0XG5Qx2TlKWIA")
+			})
+			g.It("Should handle an empty access token", func() {
+				ok, err := c.Refresh(fakeUserRefreshEmpty)
+				g.Assert(err == nil).IsTrue()
+				g.Assert(ok).IsFalse()
+			})
+			g.It("Should handle a failure to refresh", func() {
+				ok, err := c.Refresh(fakeUserRefreshFail)
+				g.Assert(err != nil).IsTrue()
+				g.Assert(ok).IsFalse()
+			})
+		})
+
 		g.Describe("When requesting a repository", func() {
 			g.It("Should return the details", func() {
 				repo, err := c.Repo(
@@ -154,21 +206,16 @@ func Test_bitbucket(t *testing.T) {
 
 		g.Describe("When activating a repository", func() {
 			g.It("Should error when malformed hook", func() {
-				err := c.Activate(fakeUser, fakeRepo, nil, "%gh&%ij")
+				err := c.Activate(fakeUser, fakeRepo, "%gh&%ij")
 				g.Assert(err != nil).IsTrue()
 			})
 			g.It("Should create the hook", func() {
-				err := c.Activate(fakeUser, fakeRepo, nil, "http://127.0.0.1")
+				err := c.Activate(fakeUser, fakeRepo, "http://127.0.0.1")
 				g.Assert(err == nil).IsTrue()
 			})
-			g.It("Should remove previous hooks")
 		})
 
 		g.Describe("When deactivating a repository", func() {
-			g.It("Should error when malformed hook", func() {
-				err := c.Deactivate(fakeUser, fakeRepo, "%gh&%ij")
-				g.Assert(err != nil).IsTrue()
-			})
 			g.It("Should error when listing hooks fails", func() {
 				err := c.Deactivate(fakeUser, fakeRepoNoHooks, "http://127.0.0.1")
 				g.Assert(err != nil).IsTrue()
@@ -183,6 +230,28 @@ func Test_bitbucket(t *testing.T) {
 			})
 		})
 
+		g.Describe("Given a list of hooks", func() {
+			g.It("Should return the matching hook", func() {
+				hooks := []*internal.Hook{
+					{Url: "http://127.0.0.1/hook"},
+				}
+				hook := matchingHooks(hooks, "http://127.0.0.1/")
+				g.Assert(hook).Equal(hooks[0])
+			})
+			g.It("Should handle no matches", func() {
+				hooks := []*internal.Hook{
+					{Url: "http://localhost/hook"},
+				}
+				hook := matchingHooks(hooks, "http://127.0.0.1/")
+				g.Assert(hook == nil).IsTrue()
+			})
+			g.It("Should handle malformed hook urls", func() {
+				var hooks []*internal.Hook
+				hook := matchingHooks(hooks, "%gh&%ij")
+				g.Assert(hook == nil).IsTrue()
+			})
+		})
+
 		g.It("Should update the status", func() {
 			err := c.Status(fakeUser, fakeRepo, fakeBuild, "http://127.0.0.1")
 			g.Assert(err == nil).IsTrue()
@@ -208,6 +277,21 @@ var (
 		Token: "cfcd2084",
 	}
 
+	fakeUserRefresh = &model.User{
+		Login:  "superman",
+		Secret: "cfcd2084",
+	}
+
+	fakeUserRefreshFail = &model.User{
+		Login:  "superman",
+		Secret: "refresh_token_not_found",
+	}
+
+	fakeUserRefreshEmpty = &model.User{
+		Login:  "superman",
+		Secret: "refresh_token_is_empty",
+	}
+
 	fakeUserNotFound = &model.User{
 		Login: "superman",
 		Token: "user_not_found",

remote/bitbucket/fixtures/handler.go

@@ -12,6 +12,7 @@ func Handler() http.Handler {
 	gin.SetMode(gin.TestMode)
 
 	e := gin.New()
+	e.POST("/site/oauth2/access_token", getOauth)
 	e.GET("/2.0/repositories/:owner/:name", getRepo)
 	e.GET("/2.0/repositories/:owner/:name/hooks", getRepoHooks)
 	e.GET("/1.0/repositories/:owner/:name/src/:commit/:file", getRepoFile)
@@ -25,6 +26,27 @@ func Handler() http.Handler {
 	return e
 }
 
+func getOauth(c *gin.Context) {
+	switch c.PostForm("code") {
+	case "code_bad_request":
+		c.String(500, "")
+		return
+	case "code_user_not_found":
+		c.String(200, tokenNotFoundPayload)
+		return
+	}
+	switch c.PostForm("refresh_token") {
+	case "refresh_token_not_found":
+		c.String(404, "")
+	case "refresh_token_is_empty":
+		c.Header("Content-Type", "application/json")
+		c.String(200, "{}")
+	default:
+		c.Header("Content-Type", "application/json")
+		c.String(200, tokenPayload)
+	}
+}
+
 func getRepo(c *gin.Context) {
 	switch c.Param("name") {
 	case "not_found", "repo_unknown", "repo_not_found":
@@ -103,6 +125,24 @@ func getUserRepos(c *gin.Context) {
 	}
 }
 
+const tokenPayload = `
+{
+	"access_token":"2YotnFZFEjr1zCsicMWpAA",
+	"refresh_token":"tGzv3JOkF0XG5Qx2TlKWIA",
+	"token_type":"Bearer",
+	"expires_in":3600
+}
+`
+
+const tokenNotFoundPayload = `
+{
+	"access_token":"user_not_found",
+	"refresh_token":"user_not_found",
+	"token_type":"Bearer",
+	"expires_in":3600
+}
+`
+
 const repoPayload = `
 {
   "full_name": "test_name/repo_name",

remote/bitbucket/parse_test.go

@@ -13,7 +13,7 @@ import (
 func Test_parser(t *testing.T) {
 
 	g := goblin.Goblin(t)
-	g.Describe("Bitbucket hook parser", func() {
+	g.Describe("Bitbucket parser", func() {
 
 		g.It("Should ignore unsupported hook", func() {
 			buf := bytes.NewBufferString(fixtures.HookPush)
@@ -27,7 +27,7 @@ func Test_parser(t *testing.T) {
 			g.Assert(err == nil).IsTrue()
 		})
 
-		g.Describe("Given a pull request hook", func() {
+		g.Describe("Given a pull request hook payload", func() {
 
 			g.It("Should return err when malformed", func() {
 				buf := bytes.NewBufferString("[]")
@@ -64,7 +64,7 @@ func Test_parser(t *testing.T) {
 			})
 		})
 
-		g.Describe("Given a push hook", func() {
+		g.Describe("Given a push hook payload", func() {
 
 			g.It("Should return err when malformed", func() {
 				buf := bytes.NewBufferString("[]")

remote/remote.go

@@ -44,12 +44,11 @@ type Remote interface {
 	// private repositories from a remote system.
 	Netrc(u *model.User, r *model.Repo) (*model.Netrc, error)
 
-	// Activate activates a repository by creating the post-commit hook and
-	// adding the SSH deploy key, if applicable.
-	Activate(u *model.User, r *model.Repo, k *model.Key, link string) error
+	// Activate activates a repository by creating the post-commit hook.
+	Activate(u *model.User, r *model.Repo, link string) error
 
-	// Deactivate removes a repository by removing all the post-commit hooks
-	// which are equal to link and removing the SSH deploy key.
+	// Deactivate deactivates a repository by removing all previously created
+	// post-commit hooks matching the given link.
 	Deactivate(u *model.User, r *model.Repo, link string) error
 
 	// Hook parses the post-commit hook from the Request body and returns the
@@ -116,8 +115,8 @@ func Netrc(c context.Context, u *model.User, r *model.Repo) (*model.Netrc, error
 
 // Activate activates a repository by creating the post-commit hook and
 // adding the SSH deploy key, if applicable.
-func Activate(c context.Context, u *model.User, r *model.Repo, k *model.Key, link string) error {
-	return FromContext(c).Activate(u, r, k, link)
+func Activate(c context.Context, u *model.User, r *model.Repo, link string) error {
+	return FromContext(c).Activate(u, r, link)
 }
 
 // Deactivate removes a repository by removing all the post-commit hooks