Fix permission checks for close/reopen from commit (#8875)

* Fix checks for close/reopen from commit

* Fix permission order

diff --git a/models/action.go b/models/action.go
index 1e05a68c398c5f8e2a6cb9c9b1f58436bd3db98b..8673dd2946cdfe59a331375f1e9371c12d0f611f 100644 (file)
--- a/models/action.go
+++ b/models/action.go
@@ -491,32 +491,45 @@ 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(`<a href="%s/commit/%s">%s</a>`, 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
+                       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 {
+                               continue
                        }
 
-                       // Process closing/reopening keywords
-                       if ref.Action != references.XRefActionCloses && ref.Action != references.XRefActionReopens {
+                       message := fmt.Sprintf(`<a href="%s/commit/%s">%s</a>`, repo.Link(), c.Sha1, html.EscapeString(c.Message))
+                       if err = CreateRefComment(doer, refRepo, refIssue, message, c.Sha1); err != nil {
+                               return err
+                       }
+
+                       // 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 e7766eac2f776a2227c8f3239986022246897b4c..2131607fc147b8396b8869f4c7069c3e5dacae38 100644 (file)
--- a/models/action_test.go
+++ b/models/action_test.go
@@ -166,7 +166,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")
@@ -220,7 +220,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))