From caba2829ef3c6a49385da64c7c8dff348734ce67 Mon Sep 17 00:00:00 2001 From: mrsdizzie Date: Wed, 1 May 2019 12:21:05 -0400 Subject: [PATCH] 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 | 94 ++++++++++++++++++++++++++++++++++++++----- models/action_test.go | 70 ++++++++++++++++++++++++++++++++ 2 files changed, 155 insertions(+), 9 deletions(-) diff --git a/models/action.go b/models/action.go index e496166c4..b089870c7 100644 --- a/models/action.go +++ b/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(`%s`, 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,19 +575,63 @@ 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 diff --git a/models/action_test.go b/models/action_test.go index e30b70680..9ba205731 100644 --- a/models/action_test.go +++ b/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))