From 1d8c8bfb97e5a7cabca530f167b5238f531c8d7f Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Thu, 7 Nov 2019 21:36:26 -0300 Subject: [PATCH 1/4] Fix checks for close/reopen from commit --- models/action.go | 46 ++++++++++++++++++++++++++++--------------- models/action_test.go | 4 ++-- 2 files changed, 32 insertions(+), 18 deletions(-) diff --git a/models/action.go b/models/action.go index b651c658d53d..77b5620419a2 100644 --- a/models/action.go +++ b/models/action.go @@ -532,32 +532,46 @@ func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit, bra } refMarked[key] = true - // only create comments for issues if user has permission for it - if perm.IsAdmin() || perm.IsOwner() || perm.CanWrite(UnitTypeIssues) { - message := fmt.Sprintf(`%s`, repo.Link(), c.Sha1, html.EscapeString(c.Message)) - if err = CreateRefComment(doer, refRepo, refIssue, message, c.Sha1); err != nil { - return err - } + // FIXME: this kind of condition is all over the code, it should be consolidated in a single place + cancomment := perm.IsAdmin() || perm.IsOwner() || perm.CanRead(UnitTypeIssues) || refIssue.PosterID == doer.ID + + // Don't proceed if the user can't comment + if !cancomment { + continue } - // Process closing/reopening keywords - if ref.Action != references.XRefActionCloses && ref.Action != references.XRefActionReopens { + message := fmt.Sprintf(`%s`, repo.Link(), c.Sha1, html.EscapeString(c.Message)) + if err = CreateRefComment(doer, refRepo, refIssue, message, c.Sha1); err != nil { + return err + } + + canclose := cancomment || perm.CanWrite(UnitTypeIssues) + + // Only issues can be closed/reopened this way, and user needs the correct permissions + if refIssue.IsPull || !canclose { continue } - // Change issue status only if the commit has been pushed to the default branch. - // and if the repo is configured to allow only that - // FIXME: we should be using Issue.ref if set instead of repo.DefaultBranch - if repo.DefaultBranch != branchName && !repo.CloseIssuesViaCommitInAnyBranch { + // Only process closing/reopening keywords + if ref.Action != references.XRefActionCloses && ref.Action != references.XRefActionReopens { continue } - // only close issues in another repo if user has push access - if perm.IsAdmin() || perm.IsOwner() || perm.CanWrite(UnitTypeCode) { - if err := changeIssueStatus(refRepo, refIssue, doer, ref.Action == references.XRefActionCloses); err != nil { - return err + if !repo.CloseIssuesViaCommitInAnyBranch { + // If the issue was specified to be in a particular branch, don't allow commits in other branches to close it + if refIssue.Ref != "" { + if branchName != refIssue.Ref { + continue + } + // Otherwise, only process commits to the default branch + } else if branchName != repo.DefaultBranch { + continue } } + + if err := changeIssueStatus(refRepo, refIssue, doer, ref.Action == references.XRefActionCloses); err != nil { + return err + } } } return nil diff --git a/models/action_test.go b/models/action_test.go index df41556850d0..25922b2247ca 100644 --- a/models/action_test.go +++ b/models/action_test.go @@ -219,7 +219,7 @@ func TestUpdateIssuesCommit(t *testing.T) { PosterID: user.ID, IssueID: 1, } - issueBean := &Issue{RepoID: repo.ID, Index: 2} + issueBean := &Issue{RepoID: repo.ID, Index: 4} AssertNotExistsBean(t, commentBean) AssertNotExistsBean(t, &Issue{RepoID: repo.ID, Index: 2}, "is_closed=1") @@ -273,7 +273,7 @@ func TestUpdateIssuesCommit_Colon(t *testing.T) { repo := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository) repo.Owner = user - issueBean := &Issue{RepoID: repo.ID, Index: 2} + issueBean := &Issue{RepoID: repo.ID, Index: 4} AssertNotExistsBean(t, &Issue{RepoID: repo.ID, Index: 2}, "is_closed=1") assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, repo.DefaultBranch)) From 9755221ce75c6bedf413736ebefc151b6acf87e3 Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Fri, 8 Nov 2019 12:24:21 -0300 Subject: [PATCH 2/4] Fix permission order --- models/action.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/models/action.go b/models/action.go index 77b5620419a2..c64f6fd00ded 100644 --- a/models/action.go +++ b/models/action.go @@ -533,7 +533,8 @@ func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit, bra refMarked[key] = true // FIXME: this kind of condition is all over the code, it should be consolidated in a single place - cancomment := perm.IsAdmin() || perm.IsOwner() || perm.CanRead(UnitTypeIssues) || refIssue.PosterID == doer.ID + canclose := perm.IsAdmin() || perm.IsOwner() || perm.CanWrite(UnitTypeIssues) || refIssue.PosterID == doer.ID + cancomment := canclose || perm.CanRead(UnitTypeIssues) // Don't proceed if the user can't comment if !cancomment { @@ -545,8 +546,6 @@ func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit, bra return err } - canclose := cancomment || perm.CanWrite(UnitTypeIssues) - // Only issues can be closed/reopened this way, and user needs the correct permissions if refIssue.IsPull || !canclose { continue From 72d0f5fe6fe0dfe4d416774da76adac79e5eb247 Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Sat, 9 Nov 2019 01:07:48 -0300 Subject: [PATCH 3/4] Simplify permission checks --- models/action.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/models/action.go b/models/action.go index 3518c0c01661..530f8e6963fb 100644 --- a/models/action.go +++ b/models/action.go @@ -490,8 +490,8 @@ func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit, bra refMarked[key] = true // FIXME: this kind of condition is all over the code, it should be consolidated in a single place - canclose := perm.IsAdmin() || perm.IsOwner() || perm.CanWrite(UnitTypeIssues) || refIssue.PosterID == doer.ID - cancomment := canclose || perm.CanRead(UnitTypeIssues) + canclose := perm.CanWrite(UnitTypeIssues) || refIssue.PosterID == doer.ID + cancomment := perm.CanRead(UnitTypeIssues) // Don't proceed if the user can't comment if !cancomment { From 7c944851d456f3398d322a1f0633706f370cc578 Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Sat, 9 Nov 2019 01:46:37 -0300 Subject: [PATCH 4/4] Revert "Simplify permission checks" This reverts commit 72d0f5fe6fe0dfe4d416774da76adac79e5eb247. --- models/action.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/models/action.go b/models/action.go index 530f8e6963fb..3518c0c01661 100644 --- a/models/action.go +++ b/models/action.go @@ -490,8 +490,8 @@ func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit, bra refMarked[key] = true // FIXME: this kind of condition is all over the code, it should be consolidated in a single place - canclose := perm.CanWrite(UnitTypeIssues) || refIssue.PosterID == doer.ID - cancomment := perm.CanRead(UnitTypeIssues) + canclose := perm.IsAdmin() || perm.IsOwner() || perm.CanWrite(UnitTypeIssues) || refIssue.PosterID == doer.ID + cancomment := canclose || perm.CanRead(UnitTypeIssues) // Don't proceed if the user can't comment if !cancomment {