Fix close issue but time watcher still running (#17643)

* Fix close issue but time watcher still running

* refactor stopwatch codes

* Fix test

* Fix test

* Fix typo

* Fix test

3 files changed, 508 insertions, 4 deletions

diff --git a/services/issue/commit.go b/services/issue/commit.go
new file mode 100644
index 0000000000..401084639d
--- /dev/null
+++ b/services/issue/commit.go
@@ -0,0 +1,192 @@
+// Copyright 2021 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package issue
+
+import (
+	"fmt"
+	"html"
+	"net/url"
+	"regexp"
+	"strconv"
+	"strings"
+	"time"
+
+	"code.gitea.io/gitea/models"
+	"code.gitea.io/gitea/modules/references"
+	"code.gitea.io/gitea/modules/repository"
+)
+
+const (
+	secondsByMinute = float64(time.Minute / time.Second) // seconds in a minute
+	secondsByHour   = 60 * secondsByMinute               // seconds in an hour
+	secondsByDay    = 8 * secondsByHour                  // seconds in a day
+	secondsByWeek   = 5 * secondsByDay                   // seconds in a week
+	secondsByMonth  = 4 * secondsByWeek                  // seconds in a month
+)
+
+var reDuration = regexp.MustCompile(`(?i)^(?:(\d+([\.,]\d+)?)(?:mo))?(?:(\d+([\.,]\d+)?)(?:w))?(?:(\d+([\.,]\d+)?)(?:d))?(?:(\d+([\.,]\d+)?)(?:h))?(?:(\d+([\.,]\d+)?)(?:m))?$`)
+
+// timeLogToAmount parses time log string and returns amount in seconds
+func timeLogToAmount(str string) int64 {
+	matches := reDuration.FindAllStringSubmatch(str, -1)
+	if len(matches) == 0 {
+		return 0
+	}
+
+	match := matches[0]
+
+	var a int64
+
+	// months
+	if len(match[1]) > 0 {
+		mo, _ := strconv.ParseFloat(strings.Replace(match[1], ",", ".", 1), 64)
+		a += int64(mo * secondsByMonth)
+	}
+
+	// weeks
+	if len(match[3]) > 0 {
+		w, _ := strconv.ParseFloat(strings.Replace(match[3], ",", ".", 1), 64)
+		a += int64(w * secondsByWeek)
+	}
+
+	// days
+	if len(match[5]) > 0 {
+		d, _ := strconv.ParseFloat(strings.Replace(match[5], ",", ".", 1), 64)
+		a += int64(d * secondsByDay)
+	}
+
+	// hours
+	if len(match[7]) > 0 {
+		h, _ := strconv.ParseFloat(strings.Replace(match[7], ",", ".", 1), 64)
+		a += int64(h * secondsByHour)
+	}
+
+	// minutes
+	if len(match[9]) > 0 {
+		d, _ := strconv.ParseFloat(strings.Replace(match[9], ",", ".", 1), 64)
+		a += int64(d * secondsByMinute)
+	}
+
+	return a
+}
+
+func issueAddTime(issue *models.Issue, doer *models.User, time time.Time, timeLog string) error {
+	amount := timeLogToAmount(timeLog)
+	if amount == 0 {
+		return nil
+	}
+
+	_, err := models.AddTime(doer, issue, amount, time)
+	return err
+}
+
+// getIssueFromRef returns the issue referenced by a ref. Returns a nil *Issue
+// if the provided ref references a non-existent issue.
+func getIssueFromRef(repo *models.Repository, index int64) (*models.Issue, error) {
+	issue, err := models.GetIssueByIndex(repo.ID, index)
+	if err != nil {
+		if models.IsErrIssueNotExist(err) {
+			return nil, nil
+		}
+		return nil, err
+	}
+	return issue, nil
+}
+
+// UpdateIssuesCommit checks if issues are manipulated by commit message.
+func UpdateIssuesCommit(doer *models.User, repo *models.Repository, commits []*repository.PushCommit, branchName string) error {
+	// Commits are appended in the reverse order.
+	for i := len(commits) - 1; i >= 0; i-- {
+		c := commits[i]
+
+		type markKey struct {
+			ID     int64
+			Action references.XRefAction
+		}
+
+		refMarked := make(map[markKey]bool)
+		var refRepo *models.Repository
+		var refIssue *models.Issue
+		var err error
+		for _, ref := range references.FindAllIssueReferences(c.Message) {
+
+			// issue is from another repo
+			if len(ref.Owner) > 0 && len(ref.Name) > 0 {
+				refRepo, err = models.GetRepositoryFromMatch(ref.Owner, ref.Name)
+				if err != nil {
+					continue
+				}
+			} else {
+				refRepo = repo
+			}
+			if refIssue, err = getIssueFromRef(refRepo, ref.Index); err != nil {
+				return err
+			}
+			if refIssue == nil {
+				continue
+			}
+
+			perm, err := models.GetUserRepoPermission(refRepo, doer)
+			if err != nil {
+				return err
+			}
+
+			key := markKey{ID: refIssue.ID, Action: ref.Action}
+			if refMarked[key] {
+				continue
+			}
+			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.CanWriteIssuesOrPulls(refIssue.IsPull) || refIssue.PosterID == doer.ID
+			cancomment := canclose || perm.CanReadIssuesOrPulls(refIssue.IsPull)
+
+			// Don't proceed if the user can't comment
+			if !cancomment {
+				continue
+			}
+
+			message := fmt.Sprintf(`<a href="%s/commit/%s">%s</a>`, html.EscapeString(repo.Link()), html.EscapeString(url.PathEscape(c.Sha1)), html.EscapeString(strings.SplitN(c.Message, "\n", 2)[0]))
+			if err = models.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
+			}
+
+			// Only process closing/reopening keywords
+			if ref.Action != references.XRefActionCloses && ref.Action != references.XRefActionReopens {
+				continue
+			}
+
+			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
+				}
+			}
+			close := ref.Action == references.XRefActionCloses
+			if close && len(ref.TimeLog) > 0 {
+				if err := issueAddTime(refIssue, doer, c.Timestamp, ref.TimeLog); err != nil {
+					return err
+				}
+			}
+			if close != refIssue.IsClosed {
+				refIssue.Repo = refRepo
+				if err := ChangeStatus(refIssue, doer, close); err != nil {
+					return err
+				}
+			}
+		}
+	}
+	return nil
+}
diff --git a/services/issue/commit_test.go b/services/issue/commit_test.go
new file mode 100644
index 0000000000..3f8c5f3b42
--- /dev/null
+++ b/services/issue/commit_test.go
@@ -0,0 +1,298 @@
+// Copyright 2019 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package issue
+
+import (
+	"testing"
+
+	"code.gitea.io/gitea/models"
+	"code.gitea.io/gitea/models/unittest"
+	"code.gitea.io/gitea/modules/repository"
+	"code.gitea.io/gitea/modules/setting"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestUpdateIssuesCommit(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+	pushCommits := []*repository.PushCommit{
+		{
+			Sha1:           "abcdef1",
+			CommitterEmail: "user2@example.com",
+			CommitterName:  "User Two",
+			AuthorEmail:    "user4@example.com",
+			AuthorName:     "User Four",
+			Message:        "start working on #FST-1, #1",
+		},
+		{
+			Sha1:           "abcdef2",
+			CommitterEmail: "user2@example.com",
+			CommitterName:  "User Two",
+			AuthorEmail:    "user2@example.com",
+			AuthorName:     "User Two",
+			Message:        "a plain message",
+		},
+		{
+			Sha1:           "abcdef2",
+			CommitterEmail: "user2@example.com",
+			CommitterName:  "User Two",
+			AuthorEmail:    "user2@example.com",
+			AuthorName:     "User Two",
+			Message:        "close #2",
+		},
+	}
+
+	user := unittest.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User)
+	repo := unittest.AssertExistsAndLoadBean(t, &models.Repository{ID: 1}).(*models.Repository)
+	repo.Owner = user
+
+	commentBean := &models.Comment{
+		Type:      models.CommentTypeCommitRef,
+		CommitSHA: "abcdef1",
+		PosterID:  user.ID,
+		IssueID:   1,
+	}
+	issueBean := &models.Issue{RepoID: repo.ID, Index: 4}
+
+	unittest.AssertNotExistsBean(t, commentBean)
+	unittest.AssertNotExistsBean(t, &models.Issue{RepoID: repo.ID, Index: 2}, "is_closed=1")
+	assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, repo.DefaultBranch))
+	unittest.AssertExistsAndLoadBean(t, commentBean)
+	unittest.AssertExistsAndLoadBean(t, issueBean, "is_closed=1")
+	unittest.CheckConsistencyFor(t, &models.Action{})
+
+	// Test that push to a non-default branch closes no issue.
+	pushCommits = []*repository.PushCommit{
+		{
+			Sha1:           "abcdef1",
+			CommitterEmail: "user2@example.com",
+			CommitterName:  "User Two",
+			AuthorEmail:    "user4@example.com",
+			AuthorName:     "User Four",
+			Message:        "close #1",
+		},
+	}
+	repo = unittest.AssertExistsAndLoadBean(t, &models.Repository{ID: 3}).(*models.Repository)
+	commentBean = &models.Comment{
+		Type:      models.CommentTypeCommitRef,
+		CommitSHA: "abcdef1",
+		PosterID:  user.ID,
+		IssueID:   6,
+	}
+	issueBean = &models.Issue{RepoID: repo.ID, Index: 1}
+
+	unittest.AssertNotExistsBean(t, commentBean)
+	unittest.AssertNotExistsBean(t, &models.Issue{RepoID: repo.ID, Index: 1}, "is_closed=1")
+	assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, "non-existing-branch"))
+	unittest.AssertExistsAndLoadBean(t, commentBean)
+	unittest.AssertNotExistsBean(t, issueBean, "is_closed=1")
+	unittest.CheckConsistencyFor(t, &models.Action{})
+
+	pushCommits = []*repository.PushCommit{
+		{
+			Sha1:           "abcdef3",
+			CommitterEmail: "user2@example.com",
+			CommitterName:  "User Two",
+			AuthorEmail:    "user2@example.com",
+			AuthorName:     "User Two",
+			Message:        "close " + setting.AppURL + repo.FullName() + "/pulls/1",
+		},
+	}
+	repo = unittest.AssertExistsAndLoadBean(t, &models.Repository{ID: 3}).(*models.Repository)
+	commentBean = &models.Comment{
+		Type:      models.CommentTypeCommitRef,
+		CommitSHA: "abcdef3",
+		PosterID:  user.ID,
+		IssueID:   6,
+	}
+	issueBean = &models.Issue{RepoID: repo.ID, Index: 1}
+
+	unittest.AssertNotExistsBean(t, commentBean)
+	unittest.AssertNotExistsBean(t, &models.Issue{RepoID: repo.ID, Index: 1}, "is_closed=1")
+	assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, repo.DefaultBranch))
+	unittest.AssertExistsAndLoadBean(t, commentBean)
+	unittest.AssertExistsAndLoadBean(t, issueBean, "is_closed=1")
+	unittest.CheckConsistencyFor(t, &models.Action{})
+}
+
+func TestUpdateIssuesCommit_Colon(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+	pushCommits := []*repository.PushCommit{
+		{
+			Sha1:           "abcdef2",
+			CommitterEmail: "user2@example.com",
+			CommitterName:  "User Two",
+			AuthorEmail:    "user2@example.com",
+			AuthorName:     "User Two",
+			Message:        "close: #2",
+		},
+	}
+
+	user := unittest.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User)
+	repo := unittest.AssertExistsAndLoadBean(t, &models.Repository{ID: 1}).(*models.Repository)
+	repo.Owner = user
+
+	issueBean := &models.Issue{RepoID: repo.ID, Index: 4}
+
+	unittest.AssertNotExistsBean(t, &models.Issue{RepoID: repo.ID, Index: 2}, "is_closed=1")
+	assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, repo.DefaultBranch))
+	unittest.AssertExistsAndLoadBean(t, issueBean, "is_closed=1")
+	unittest.CheckConsistencyFor(t, &models.Action{})
+}
+
+func TestUpdateIssuesCommit_Issue5957(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+	user := unittest.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User)
+
+	// Test that push to a non-default branch closes an issue.
+	pushCommits := []*repository.PushCommit{
+		{
+			Sha1:           "abcdef1",
+			CommitterEmail: "user2@example.com",
+			CommitterName:  "User Two",
+			AuthorEmail:    "user4@example.com",
+			AuthorName:     "User Four",
+			Message:        "close #2",
+		},
+	}
+
+	repo := unittest.AssertExistsAndLoadBean(t, &models.Repository{ID: 2}).(*models.Repository)
+	commentBean := &models.Comment{
+		Type:      models.CommentTypeCommitRef,
+		CommitSHA: "abcdef1",
+		PosterID:  user.ID,
+		IssueID:   7,
+	}
+
+	issueBean := &models.Issue{RepoID: repo.ID, Index: 2, ID: 7}
+
+	unittest.AssertNotExistsBean(t, commentBean)
+	unittest.AssertNotExistsBean(t, issueBean, "is_closed=1")
+	assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, "non-existing-branch"))
+	unittest.AssertExistsAndLoadBean(t, commentBean)
+	unittest.AssertExistsAndLoadBean(t, issueBean, "is_closed=1")
+	unittest.CheckConsistencyFor(t, &models.Action{})
+}
+
+func TestUpdateIssuesCommit_AnotherRepo(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+	user := unittest.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User)
+
+	// Test that a push to default branch closes issue in another repo
+	// If the user also has push permissions to that repo
+	pushCommits := []*repository.PushCommit{
+		{
+			Sha1:           "abcdef1",
+			CommitterEmail: "user2@example.com",
+			CommitterName:  "User Two",
+			AuthorEmail:    "user2@example.com",
+			AuthorName:     "User Two",
+			Message:        "close user2/repo1#1",
+		},
+	}
+
+	repo := unittest.AssertExistsAndLoadBean(t, &models.Repository{ID: 2}).(*models.Repository)
+	commentBean := &models.Comment{
+		Type:      models.CommentTypeCommitRef,
+		CommitSHA: "abcdef1",
+		PosterID:  user.ID,
+		IssueID:   1,
+	}
+
+	issueBean := &models.Issue{RepoID: 1, Index: 1, ID: 1}
+
+	unittest.AssertNotExistsBean(t, commentBean)
+	unittest.AssertNotExistsBean(t, issueBean, "is_closed=1")
+	assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, repo.DefaultBranch))
+	unittest.AssertExistsAndLoadBean(t, commentBean)
+	unittest.AssertExistsAndLoadBean(t, issueBean, "is_closed=1")
+	unittest.CheckConsistencyFor(t, &models.Action{})
+}
+
+func TestUpdateIssuesCommit_AnotherRepo_FullAddress(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+	user := unittest.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User)
+
+	// Test that a push to default branch closes issue in another repo
+	// If the user also has push permissions to that repo
+	pushCommits := []*repository.PushCommit{
+		{
+			Sha1:           "abcdef1",
+			CommitterEmail: "user2@example.com",
+			CommitterName:  "User Two",
+			AuthorEmail:    "user2@example.com",
+			AuthorName:     "User Two",
+			Message:        "close " + setting.AppURL + "user2/repo1/issues/1",
+		},
+	}
+
+	repo := unittest.AssertExistsAndLoadBean(t, &models.Repository{ID: 2}).(*models.Repository)
+	commentBean := &models.Comment{
+		Type:      models.CommentTypeCommitRef,
+		CommitSHA: "abcdef1",
+		PosterID:  user.ID,
+		IssueID:   1,
+	}
+
+	issueBean := &models.Issue{RepoID: 1, Index: 1, ID: 1}
+
+	unittest.AssertNotExistsBean(t, commentBean)
+	unittest.AssertNotExistsBean(t, issueBean, "is_closed=1")
+	assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, repo.DefaultBranch))
+	unittest.AssertExistsAndLoadBean(t, commentBean)
+	unittest.AssertExistsAndLoadBean(t, issueBean, "is_closed=1")
+	unittest.CheckConsistencyFor(t, &models.Action{})
+}
+
+func TestUpdateIssuesCommit_AnotherRepoNoPermission(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+	user := unittest.AssertExistsAndLoadBean(t, &models.User{ID: 10}).(*models.User)
+
+	// Test that a push with close reference *can not* close issue
+	// If the committer doesn't have push rights in that repo
+	pushCommits := []*repository.PushCommit{
+		{
+			Sha1:           "abcdef3",
+			CommitterEmail: "user10@example.com",
+			CommitterName:  "User Ten",
+			AuthorEmail:    "user10@example.com",
+			AuthorName:     "User Ten",
+			Message:        "close user3/repo3#1",
+		},
+		{
+			Sha1:           "abcdef4",
+			CommitterEmail: "user10@example.com",
+			CommitterName:  "User Ten",
+			AuthorEmail:    "user10@example.com",
+			AuthorName:     "User Ten",
+			Message:        "close " + setting.AppURL + "user3/repo3/issues/1",
+		},
+	}
+
+	repo := unittest.AssertExistsAndLoadBean(t, &models.Repository{ID: 6}).(*models.Repository)
+	commentBean := &models.Comment{
+		Type:      models.CommentTypeCommitRef,
+		CommitSHA: "abcdef3",
+		PosterID:  user.ID,
+		IssueID:   6,
+	}
+	commentBean2 := &models.Comment{
+		Type:      models.CommentTypeCommitRef,
+		CommitSHA: "abcdef4",
+		PosterID:  user.ID,
+		IssueID:   6,
+	}
+
+	issueBean := &models.Issue{RepoID: 3, Index: 1, ID: 6}
+
+	unittest.AssertNotExistsBean(t, commentBean)
+	unittest.AssertNotExistsBean(t, commentBean2)
+	unittest.AssertNotExistsBean(t, issueBean, "is_closed=1")
+	assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, repo.DefaultBranch))
+	unittest.AssertNotExistsBean(t, commentBean)
+	unittest.AssertNotExistsBean(t, commentBean2)
+	unittest.AssertNotExistsBean(t, issueBean, "is_closed=1")
+	unittest.CheckConsistencyFor(t, &models.Action{})
+}
diff --git a/services/issue/status.go b/services/issue/status.go
index b01ce4bbb8..0a18169a27 100644
--- a/services/issue/status.go
+++ b/services/issue/status.go
@@ -6,16 +6,30 @@ package issue
 
 import (
 	"code.gitea.io/gitea/models"
+	"code.gitea.io/gitea/models/db"
 	"code.gitea.io/gitea/modules/notification"
 )
 
 // ChangeStatus changes issue status to open or closed.
-func ChangeStatus(issue *models.Issue, doer *models.User, isClosed bool) (err error) {
-	comment, err := issue.ChangeStatus(doer, isClosed)
+func ChangeStatus(issue *models.Issue, doer *models.User, closed bool) error {
+	comment, err := issue.ChangeStatus(doer, closed)
 	if err != nil {
-		return
+		// Don't return an error when dependencies are open as this would let the push fail
+		if models.IsErrDependenciesLeft(err) {
+			if closed {
+				return models.FinishIssueStopwatchIfPossible(db.DefaultContext, doer, issue)
+			}
+		}
+		return err
 	}
 
-	notification.NotifyIssueChangeStatus(doer, issue, comment, isClosed)
+	if closed {
+		if err := models.FinishIssueStopwatchIfPossible(db.DefaultContext, doer, issue); err != nil {
+			return err
+		}
+	}
+
+	notification.NotifyIssueChangeStatus(doer, issue, comment, closed)
+
 	return nil
 }