Improve issue reference on commit (#6694)

* Improve issue reference on commit

Allow commits to properly reference issues in other repositories and also to
close/reopen those issues if user has code permission. Should match
Github behavior described here: https://help.github.com/en/articles/closing-issues-using-keywords

Fixes 6664

* Fix missing return

* Match user/repo directly in regex

models/action.go

@@ -1,4 +1,5 @@
 // Copyright 2014 The Gogs Authors. All rights reserved.
+// 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.
 
@@ -62,7 +63,7 @@ var (
 	issueReferenceKeywordsPat                     *regexp.Regexp
 )
 
-const issueRefRegexpStr = `(?:\S+/\S=)?#\d+`
+const issueRefRegexpStr = `(?:([0-9a-zA-Z-_\.]+)/([0-9a-zA-Z-_\.]+))?(#[0-9]+)+`
 
 func assembleKeywordsPattern(words []string) string {
 	return fmt.Sprintf(`(?i)(?:%s) %s`, strings.Join(words, "|"), issueRefRegexpStr)
@@ -192,6 +193,21 @@ func (a *Action) GetRepoLink() string {
 	return "/" + a.GetRepoPath()
 }
 
+// GetRepositoryFromMatch returns a *Repository from a username and repo strings
+func GetRepositoryFromMatch(ownerName string, repoName string) (*Repository, error) {
+	var err error
+	refRepo, err := GetRepositoryByOwnerAndName(ownerName, repoName)
+	if err != nil {
+		if IsErrRepoNotExist(err) {
+			log.Warn("Repository referenced in commit but does not exist: %v", err)
+			return nil, err
+		}
+		log.Error("GetRepositoryByOwnerAndName: %v", err)
+		return nil, err
+	}
+	return refRepo, nil
+}
+
 // GetCommentLink returns link to action comment.
 func (a *Action) GetCommentLink() string {
 	return a.getCommentLink(x)
@@ -521,8 +537,24 @@ func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit, bra
 		c := commits[i]
 
 		refMarked := make(map[int64]bool)
-		for _, ref := range issueReferenceKeywordsPat.FindAllString(c.Message, -1) {
-			issue, err := getIssueFromRef(repo, ref)
+		var refRepo *Repository
+		var err error
+		for _, m := range issueReferenceKeywordsPat.FindAllStringSubmatch(c.Message, -1) {
+			if len(m[3]) == 0 {
+				continue
+			}
+			ref := m[3]
+
+			// issue is from another repo
+			if len(m[1]) > 0 && len(m[2]) > 0 {
+				refRepo, err = GetRepositoryFromMatch(string(m[1]), string(m[2]))
+				if err != nil {
+					continue
+				}
+			} else {
+				refRepo = repo
+			}
+			issue, err := getIssueFromRef(refRepo, ref)
 			if err != nil {
 				return err
 			}
@@ -533,7 +565,7 @@ func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit, bra
 			refMarked[issue.ID] = true
 
 			message := fmt.Sprintf(`<a href="%s/commit/%s">%s</a>`, repo.Link(), c.Sha1, c.Message)
-			if err = CreateRefComment(doer, repo, issue, message, c.Sha1); err != nil {
+			if err = CreateRefComment(doer, refRepo, issue, message, c.Sha1); err != nil {
 				return err
 			}
 		}
@@ -543,21 +575,65 @@ func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit, bra
 		if repo.DefaultBranch != branchName && !repo.CloseIssuesViaCommitInAnyBranch {
 			continue
 		}
-
 		refMarked = make(map[int64]bool)
-		for _, ref := range issueCloseKeywordsPat.FindAllString(c.Message, -1) {
-			if err := changeIssueStatus(repo, doer, ref, refMarked, true); err != nil {
+		for _, m := range issueCloseKeywordsPat.FindAllStringSubmatch(c.Message, -1) {
+			if len(m[3]) == 0 {
+				continue
+			}
+			ref := m[3]
+
+			// issue is from another repo
+			if len(m[1]) > 0 && len(m[2]) > 0 {
+				refRepo, err = GetRepositoryFromMatch(string(m[1]), string(m[2]))
+				if err != nil {
+					continue
+				}
+			} else {
+				refRepo = repo
+			}
+
+			perm, err := GetUserRepoPermission(refRepo, doer)
+			if err != nil {
 				return err
 			}
+			// only close issues in another repo if user has push access
+			if perm.CanWrite(UnitTypeCode) {
+				if err := changeIssueStatus(refRepo, doer, ref, refMarked, true); err != nil {
+					return err
+				}
+			}
 		}
 
 		// It is conflict to have close and reopen at same time, so refsMarked doesn't need to reinit here.
-		for _, ref := range issueReopenKeywordsPat.FindAllString(c.Message, -1) {
-			if err := changeIssueStatus(repo, doer, ref, refMarked, false); err != nil {
+		for _, m := range issueReopenKeywordsPat.FindAllStringSubmatch(c.Message, -1) {
+			if len(m[3]) == 0 {
+				continue
+			}
+			ref := m[3]
+
+			// issue is from another repo
+			if len(m[1]) > 0 && len(m[2]) > 0 {
+				refRepo, err = GetRepositoryFromMatch(string(m[1]), string(m[2]))
+				if err != nil {
+					continue
+				}
+			} else {
+				refRepo = repo
+			}
+
+			perm, err := GetUserRepoPermission(refRepo, doer)
+			if err != nil {
+				return err
+			}
+
+			// only reopen issues in another repo if user has push access
+			if perm.CanWrite(UnitTypeCode) {
+				if err := changeIssueStatus(refRepo, doer, ref, refMarked, false); err != nil {
 					return err
 				}
 			}
 		}
+	}
 	return nil
 }
 

models/action_test.go

@@ -294,6 +294,76 @@ func TestUpdateIssuesCommit_Issue5957(t *testing.T) {
 	CheckConsistencyFor(t, &Action{})
 }
 
+func TestUpdateIssuesCommit_AnotherRepo(t *testing.T) {
+	assert.NoError(t, PrepareTestDatabase())
+	user := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
+
+	// Test that a push to default branch closes issue in another repo
+	// If the user also has push permissions to that repo
+	pushCommits := []*PushCommit{
+		{
+			Sha1:           "abcdef1",
+			CommitterEmail: "user2@example.com",
+			CommitterName:  "User Two",
+			AuthorEmail:    "user2@example.com",
+			AuthorName:     "User Two",
+			Message:        "close user2/repo1#1",
+		},
+	}
+
+	repo := AssertExistsAndLoadBean(t, &Repository{ID: 2}).(*Repository)
+	commentBean := &Comment{
+		Type:      CommentTypeCommitRef,
+		CommitSHA: "abcdef1",
+		PosterID:  user.ID,
+		IssueID:   1,
+	}
+
+	issueBean := &Issue{RepoID: 1, Index: 1, ID: 1}
+
+	AssertNotExistsBean(t, commentBean)
+	AssertNotExistsBean(t, issueBean, "is_closed=1")
+	assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, repo.DefaultBranch))
+	AssertExistsAndLoadBean(t, commentBean)
+	AssertExistsAndLoadBean(t, issueBean, "is_closed=1")
+	CheckConsistencyFor(t, &Action{})
+}
+
+func TestUpdateIssuesCommit_AnotherRepoNoPermission(t *testing.T) {
+	assert.NoError(t, PrepareTestDatabase())
+	user := AssertExistsAndLoadBean(t, &User{ID: 10}).(*User)
+
+	// Test that a push with close reference *can not* close issue
+	// If the commiter doesn't have push rights in that repo
+	pushCommits := []*PushCommit{
+		{
+			Sha1:           "abcdef3",
+			CommitterEmail: "user10@example.com",
+			CommitterName:  "User Ten",
+			AuthorEmail:    "user10@example.com",
+			AuthorName:     "User Ten",
+			Message:        "close user3/repo3#1",
+		},
+	}
+
+	repo := AssertExistsAndLoadBean(t, &Repository{ID: 6}).(*Repository)
+	commentBean := &Comment{
+		Type:      CommentTypeCommitRef,
+		CommitSHA: "abcdef3",
+		PosterID:  user.ID,
+		IssueID:   6,
+	}
+
+	issueBean := &Issue{RepoID: 3, Index: 1, ID: 6}
+
+	AssertNotExistsBean(t, commentBean)
+	AssertNotExistsBean(t, issueBean, "is_closed=1")
+	assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, repo.DefaultBranch))
+	AssertExistsAndLoadBean(t, commentBean)
+	AssertNotExistsBean(t, issueBean, "is_closed=1")
+	CheckConsistencyFor(t, &Action{})
+}
+
 func testCorrectRepoAction(t *testing.T, opts CommitRepoActionOptions, actionBean *Action) {
 	AssertNotExistsBean(t, actionBean)
 	assert.NoError(t, CommitRepoAction(opts))