From ecdb4c1750d1b2f2e8e41bdc87ffec5458cd8f13 Mon Sep 17 00:00:00 2001
From: guillep2k <18600385+guillep2k@users.noreply.github.com>
Date: Fri, 15 Nov 2019 19:11:40 -0300
Subject: [PATCH] Fix permission checks for close/reopen from commit (#8875)
(#9033)
* Fix checks for close/reopen from commit
* Fix permission order
---
models/action.go | 45 ++++++++++++++++++++++++++++---------------
models/action_test.go | 4 ++--
2 files changed, 31 insertions(+), 18 deletions(-)
diff --git a/models/action.go b/models/action.go
index 2d2999f880..626ae1913d 100644
--- a/models/action.go
+++ b/models/action.go
@@ -533,32 +533,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(`%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
+ 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(`%s`, 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 df41556850..25922b2247 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))
--
2.39.5