From 71467f946f1cc9cc1af66b611b766a7a99a59c90 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 15 May 2019 20:18:07 +0100 Subject: [PATCH 1/8] Fix #6946 by checking PullRequest ID on pushing --- cmd/hook.go | 4 ++++ cmd/serv.go | 1 + models/branches.go | 2 ++ models/helper_environment.go | 21 ++++++++++++++------- models/pull.go | 4 ++-- modules/private/branch.go | 30 ++++++++++++++++++++++++++++++ routers/private/branch.go | 29 +++++++++++++++++++++++++++++ routers/private/internal.go | 1 + 8 files changed, 83 insertions(+), 9 deletions(-) diff --git a/cmd/hook.go b/cmd/hook.go index f8bd34c4e995e..0db17212ad9e3 100644 --- a/cmd/hook.go +++ b/cmd/hook.go @@ -68,6 +68,7 @@ func runHookPreReceive(c *cli.Context) error { reponame := os.Getenv(models.EnvRepoName) userIDStr := os.Getenv(models.EnvPusherID) repoPath := models.RepoPath(username, reponame) + prID, _ := strconv.ParseInt(os.Getenv(models.ProtectedBranchPRID), 10, 64) buf := bytes.NewBuffer(nil) scanner := bufio.NewScanner(os.Stdin) @@ -115,6 +116,9 @@ func runHookPreReceive(c *cli.Context) error { userID, _ := strconv.ParseInt(userIDStr, 10, 64) canPush, err := private.CanUserPush(protectBranch.ID, userID) + if err == nil && !canPush { + canPush, err = private.HasEnoughApprovals(protectBranch.ID, prID) + } if err != nil { fail("Internal error", "Fail to detect user can push: %v", err) } else if !canPush { diff --git a/cmd/serv.go b/cmd/serv.go index a30e02e7a264f..393b026079633 100644 --- a/cmd/serv.go +++ b/cmd/serv.go @@ -336,6 +336,7 @@ func runServ(c *cli.Context) error { } os.Setenv(models.ProtectedBranchRepoID, fmt.Sprintf("%d", repo.ID)) + os.Setenv(models.ProtectedBranchPRID, fmt.Sprintf("%d", 0)) gitcmd.Dir = setting.RepoRootPath gitcmd.Stdout = os.Stdout diff --git a/models/branches.go b/models/branches.go index abefa60f48420..1730c752bafb3 100644 --- a/models/branches.go +++ b/models/branches.go @@ -19,6 +19,8 @@ import ( const ( // ProtectedBranchRepoID protected Repo ID ProtectedBranchRepoID = "GITEA_REPO_ID" + // ProtectedBranchPRID protected Repo PR ID + ProtectedBranchPRID = "GITEA_PR_ID" ) // ProtectedBranch struct diff --git a/models/helper_environment.go b/models/helper_environment.go index 283584cc5238d..49236b04b7b7c 100644 --- a/models/helper_environment.go +++ b/models/helper_environment.go @@ -12,24 +12,31 @@ import ( // PushingEnvironment returns an os environment to allow hooks to work on push func PushingEnvironment(doer *User, repo *Repository) []string { + return FullPushingEnvironment(doer, doer, repo, 0) +} + +// FullPushingEnvironment returns an os environment to allow hooks to work on push +func FullPushingEnvironment(author, committer *User, repo *Repository, prID int64) []string { isWiki := "false" if strings.HasSuffix(repo.Name, ".wiki") { isWiki = "true" } - sig := doer.NewGitSig() + authorSig := author.NewGitSig() + committerSig := committer.NewGitSig() return append(os.Environ(), - "GIT_AUTHOR_NAME="+sig.Name, - "GIT_AUTHOR_EMAIL="+sig.Email, - "GIT_COMMITTER_NAME="+sig.Name, - "GIT_COMMITTER_EMAIL="+sig.Email, + "GIT_AUTHOR_NAME="+authorSig.Name, + "GIT_AUTHOR_EMAIL="+authorSig.Email, + "GIT_COMMITTER_NAME="+committerSig.Name, + "GIT_COMMITTER_EMAIL="+committerSig.Email, EnvRepoName+"="+repo.Name, EnvRepoUsername+"="+repo.OwnerName, EnvRepoIsWiki+"="+isWiki, - EnvPusherName+"="+doer.Name, - EnvPusherID+"="+fmt.Sprintf("%d", doer.ID), + EnvPusherName+"="+committer.Name, + EnvPusherID+"="+fmt.Sprintf("%d", committer.ID), ProtectedBranchRepoID+"="+fmt.Sprintf("%d", repo.ID), + ProtectedBranchPRID+"="+fmt.Sprintf("%d", prID), "SSH_ORIGINAL_COMMAND=gitea-internal", ) diff --git a/models/pull.go b/models/pull.go index fe18765fc0c78..4c59ac045a51e 100644 --- a/models/pull.go +++ b/models/pull.go @@ -555,7 +555,7 @@ func (pr *PullRequest) Merge(doer *User, baseGitRepo *git.Repository, mergeStyle return ErrInvalidMergeStyle{pr.BaseRepo.ID, mergeStyle} } - env := PushingEnvironment(doer, pr.BaseRepo) + env := FullPushingEnvironment(doer, doer, pr.BaseRepo, pr.ID) // Push back to upstream. if err := git.NewCommand("push", baseGitRepo.Path, pr.BaseBranch).RunInDirTimeoutEnvPipeline(env, -1, tmpBasePath, nil, &errbuf); err != nil { @@ -1123,7 +1123,7 @@ func (pr *PullRequest) UpdatePatch() (err error) { if err = pr.GetHeadRepo(); err != nil { return fmt.Errorf("GetHeadRepo: %v", err) } else if pr.HeadRepo == nil { - log.Trace("PullRequest[%d].UpdatePatch: ignored cruppted data", pr.ID) + log.Trace("PullRequest[%d].UpdatePatch: ignored corrupted data", pr.ID) return nil } diff --git a/modules/private/branch.go b/modules/private/branch.go index bbd0d4b697382..db079f67eb016 100644 --- a/modules/private/branch.go +++ b/modules/private/branch.go @@ -65,3 +65,33 @@ func CanUserPush(protectedBranchID, userID int64) (bool, error) { return canPush["can_push"].(bool), nil } + +// HasEnoughApprovals returns if true if pr has enough granted approvals. +func HasEnoughApprovals(protectedBranchID, prID int64) (bool, error) { + if prID <= 0 { + return false, nil + } + + // Ask for running deliver hook and test pull request tasks. + reqURL := setting.LocalURL + fmt.Sprintf("api/internal/protectedbranchpr/%d/%d", protectedBranchID, prID) + log.GitLogger.Trace("CanUserPush: %s", reqURL) + + resp, err := newInternalRequest(reqURL, "GET").Response() + if err != nil { + return false, err + } + + var canPush = make(map[string]interface{}) + if err := json.NewDecoder(resp.Body).Decode(&canPush); err != nil { + return false, err + } + + defer resp.Body.Close() + + // All 2XX status codes are accepted and others will return an error + if resp.StatusCode/100 != 2 { + return false, fmt.Errorf("Failed to retrieve push user: %s", decodeJSONError(resp).Err) + } + + return canPush["can_push"].(bool), nil +} diff --git a/routers/private/branch.go b/routers/private/branch.go index 448c61f1dbba4..defc3ee1fcafc 100644 --- a/routers/private/branch.go +++ b/routers/private/branch.go @@ -50,3 +50,32 @@ func CanUserPush(ctx *macaron.Context) { }) } } + +// HasEnoughApprovals return if PR has enough approvals +func HasEnoughApprovals(ctx *macaron.Context) { + pbID := ctx.ParamsInt64(":pbid") + prID := ctx.ParamsInt64(":prid") + + protectBranch, err := models.GetProtectedBranchByID(pbID) + if err != nil { + ctx.JSON(500, map[string]interface{}{ + "err": err.Error(), + }) + return + } else if prID > 0 && protectBranch != nil { + pr, err := models.GetPullRequestByID(prID) + if err != nil { + ctx.JSON(500, map[string]interface{}{ + "err": err.Error(), + }) + return + } + ctx.JSON(200, map[string]interface{}{ + "can_push": protectBranch.HasEnoughApprovals(pr), + }) + } else { + ctx.JSON(200, map[string]interface{}{ + "can_push": false, + }) + } +} diff --git a/routers/private/internal.go b/routers/private/internal.go index ee6e1274c3e64..0cc088fe6b77a 100644 --- a/routers/private/internal.go +++ b/routers/private/internal.go @@ -86,6 +86,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Get("/repositories/:repoid/wiki/init", InitWiki) m.Post("/push/update", PushUpdate) m.Get("/protectedbranch/:pbid/:userid", CanUserPush) + m.Get("/protectedbranchpr/:pbid/:prid", HasEnoughApprovals) m.Get("/repo/:owner/:repo", GetRepositoryByOwnerAndName) m.Get("/branch/:id/*", GetProtectedBranchBy) m.Get("/repository/:rid", GetRepository) From fca3a5103a600b2b4e12b0f180c9f99afc502f60 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 15 May 2019 21:18:58 +0100 Subject: [PATCH 2/8] Ensure we have the owner name, the pr attributes and the the issue --- models/helper_environment.go | 2 +- routers/private/branch.go | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/models/helper_environment.go b/models/helper_environment.go index 49236b04b7b7c..7a2f033a6e3ad 100644 --- a/models/helper_environment.go +++ b/models/helper_environment.go @@ -31,7 +31,7 @@ func FullPushingEnvironment(author, committer *User, repo *Repository, prID int6 "GIT_COMMITTER_NAME="+committerSig.Name, "GIT_COMMITTER_EMAIL="+committerSig.Email, EnvRepoName+"="+repo.Name, - EnvRepoUsername+"="+repo.OwnerName, + EnvRepoUsername+"="+repo.MustOwnerName(), EnvRepoIsWiki+"="+isWiki, EnvPusherName+"="+committer.Name, EnvPusherID+"="+fmt.Sprintf("%d", committer.ID), diff --git a/routers/private/branch.go b/routers/private/branch.go index defc3ee1fcafc..e883607b898e6 100644 --- a/routers/private/branch.go +++ b/routers/private/branch.go @@ -64,6 +64,12 @@ func HasEnoughApprovals(ctx *macaron.Context) { return } else if prID > 0 && protectBranch != nil { pr, err := models.GetPullRequestByID(prID) + if err == nil { + err = pr.LoadAttributes() + } + if err == nil { + err = pr.LoadIssue() + } if err != nil { ctx.JSON(500, map[string]interface{}{ "err": err.Error(), From dde55bd5f9f108032746256749bb79d2659a270f Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 20 May 2019 18:33:31 +0100 Subject: [PATCH 3/8] Fix TestSearchRepo by waiting till indexing is done --- integrations/repo_search_test.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/integrations/repo_search_test.go b/integrations/repo_search_test.go index d7d07ca8d01be..2aee724a439f1 100644 --- a/integrations/repo_search_test.go +++ b/integrations/repo_search_test.go @@ -5,8 +5,12 @@ package integrations import ( + "log" "net/http" "testing" + "time" + + "code.gitea.io/gitea/models" "github.com/PuerkitoBio/goquery" "github.com/stretchr/testify/assert" @@ -27,6 +31,23 @@ func resultFilenames(t testing.TB, doc *HTMLDoc) []string { func TestSearchRepo(t *testing.T) { prepareTestEnv(t) + repo, err := models.GetRepositoryByOwnerAndName("user2", "repo1") + assert.NoError(t, err) + + models.UpdateRepoIndexer(repo) + + log.Printf("Waiting for indexing\n") + + i := 0 + for { + if repo.IndexerStatus != nil && len(repo.IndexerStatus.CommitSha) != 0 && i <= 60 { + break + } + time.Sleep(1 * time.Second) + i++ + } + log.Printf("Indexing took: %d s\n", i) + req := NewRequestf(t, "GET", "/user2/repo1/search?q=Description&page=1") resp := MakeRequest(t, req, http.StatusOK) From f7fba9843bb98518752d12c41c9f7a52f80a4d09 Mon Sep 17 00:00:00 2001 From: zeripath Date: Tue, 21 May 2019 07:07:34 +0100 Subject: [PATCH 4/8] Update integrations/repo_search_test.go --- integrations/repo_search_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integrations/repo_search_test.go b/integrations/repo_search_test.go index 2aee724a439f1..d86a4dda049c2 100644 --- a/integrations/repo_search_test.go +++ b/integrations/repo_search_test.go @@ -40,7 +40,7 @@ func TestSearchRepo(t *testing.T) { i := 0 for { - if repo.IndexerStatus != nil && len(repo.IndexerStatus.CommitSha) != 0 && i <= 60 { + if (repo.IndexerStatus != nil && len(repo.IndexerStatus.CommitSha) != 0) || i > 60 { break } time.Sleep(1 * time.Second) From cb9b7bb9f6f1a76ef9aa1f0e0e6a776f22279f6c Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 21 May 2019 19:26:39 +0100 Subject: [PATCH 5/8] changes as per @mrsdizzie --- integrations/repo_search_test.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/integrations/repo_search_test.go b/integrations/repo_search_test.go index 2aee724a439f1..3422aeaa126d6 100644 --- a/integrations/repo_search_test.go +++ b/integrations/repo_search_test.go @@ -39,14 +39,18 @@ func TestSearchRepo(t *testing.T) { log.Printf("Waiting for indexing\n") i := 0 - for { - if repo.IndexerStatus != nil && len(repo.IndexerStatus.CommitSha) != 0 && i <= 60 { + for i < 60 { + if repo.IndexerStatus != nil && len(repo.IndexerStatus.CommitSha) != 0 { break } time.Sleep(1 * time.Second) i++ } - log.Printf("Indexing took: %d s\n", i) + if i < 60 { + log.Printf("Indexing took: %ds\n", i) + } else { + log.Printf("Waited the limit: %ds for indexing, continuing\n", i) + } req := NewRequestf(t, "GET", "/user2/repo1/search?q=Description&page=1") resp := MakeRequest(t, req, http.StatusOK) From 10773f129cfab6e97bc6f98166f199d5b14e10a1 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 2 Jun 2019 12:53:38 +0100 Subject: [PATCH 6/8] missing comma --- routers/private/hook.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/private/hook.go b/routers/private/hook.go index 25f5720776012..38fb9e64d80f6 100644 --- a/routers/private/hook.go +++ b/routers/private/hook.go @@ -91,7 +91,7 @@ func HookPreReceive(ctx *macaron.Context) { if err != nil { log.Error("Unable to get PullRequest %d Error: %v", prID, err) ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ - "err": fmt.Sprintf("Unable to get PullRequest %d Error: %v", prID, err) + "err": fmt.Sprintf("Unable to get PullRequest %d Error: %v", prID, err), }) return } From 46d8b2e7e72bb437d2cb6bc76e124d50b742da4c Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 2 Jun 2019 16:02:21 +0100 Subject: [PATCH 7/8] Spelling mistake --- cmd/hook.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/hook.go b/cmd/hook.go index 1ef29907bb314..ca876f02a3156 100644 --- a/cmd/hook.go +++ b/cmd/hook.go @@ -96,7 +96,7 @@ func runHookPreReceive(c *cli.Context) error { UserID: userID, GitAlternativeObjectDirectories: os.Getenv(private.GitAlternativeObjectDirectories), GitObjectDirectory: os.Getenv(private.GitObjectDirectory), - ProtectedBranchPRID: prID, + ProtectedBranchID: prID, }) switch statusCode { case http.StatusInternalServerError: From c0158391977a5519a9a3dbfe14f55820ba495656 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Thu, 27 Jun 2019 20:19:17 +0100 Subject: [PATCH 8/8] Fix full pushing environment --- modules/pull/merge.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/modules/pull/merge.go b/modules/pull/merge.go index e915817fd2d5e..5685336a2bb91 100644 --- a/modules/pull/merge.go +++ b/modules/pull/merge.go @@ -231,7 +231,17 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor } } - env := models.FullPushingEnvironment(pr.HeadUserName, doer, pr.BaseRepo) + headUser, err := models.GetUserByName(pr.HeadUserName) + if err != nil { + if !models.IsErrUserNotExist(err) { + log.Error("Can't find user: %s for head repository - %v", pr.HeadUserName, err) + return err + } + log.Error("Can't find user: %s for head repository - defaulting to doer: %s - %v", pr.HeadUserName, doer.Name, err) + headUser = doer + } + + env := models.FullPushingEnvironment(headUser, doer, pr.BaseRepo, pr.ID) // Push back to upstream. if err := git.NewCommand("push", "origin", pr.BaseBranch).RunInDirTimeoutEnvPipeline(env, -1, tmpBasePath, nil, &errbuf); err != nil {