From ef0dfaf3059d356a46600dffdae38255d378c44e Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Sun, 13 Oct 2019 01:11:21 -0300 Subject: [PATCH 1/2] Ignore mentions for users with no access --- models/issue.go | 156 +++++++++++++++++++++++++++++++--------- models/issue_comment.go | 13 +++- models/issue_mail.go | 15 ++-- models/issue_test.go | 32 +++++++++ models/org_team.go | 2 +- 5 files changed, 177 insertions(+), 41 deletions(-) diff --git a/models/issue.go b/models/issue.go index 33e13bb180c55..5a3f2d5ea0c64 100644 --- a/models/issue.go +++ b/models/issue.go @@ -1462,46 +1462,18 @@ func getParticipantsByIssueID(e Engine, issueID int64) ([]*User, error) { return users, e.In("id", userIDs).Find(&users) } -// UpdateIssueMentions extracts mentioned people from content and -// updates issue-user relations for them. -func UpdateIssueMentions(e Engine, issueID int64, mentions []string) error { +// UpdateIssueMentions updates issue-user relations for mentioned users. +func UpdateIssueMentions(e Engine, issueID int64, mentions []*User) error { if len(mentions) == 0 { return nil } - - for i := range mentions { - mentions[i] = strings.ToLower(mentions[i]) - } - users := make([]*User, 0, len(mentions)) - - if err := e.In("lower_name", mentions).Asc("lower_name").Find(&users); err != nil { - return fmt.Errorf("find mentioned users: %v", err) + ids := make([]int64, len(mentions)) + for i, u := range mentions { + ids[i] = u.ID } - - ids := make([]int64, 0, len(mentions)) - for _, user := range users { - ids = append(ids, user.ID) - if !user.IsOrganization() || user.NumMembers == 0 { - continue - } - - memberIDs := make([]int64, 0, user.NumMembers) - orgUsers, err := getOrgUsersByOrgID(e, user.ID) - if err != nil { - return fmt.Errorf("GetOrgUsersByOrgID [%d]: %v", user.ID, err) - } - - for _, orgUser := range orgUsers { - memberIDs = append(memberIDs, orgUser.ID) - } - - ids = append(ids, memberIDs...) - } - if err := UpdateIssueUsersByMentions(e, issueID, ids); err != nil { return fmt.Errorf("UpdateIssueUsersByMentions: %v", err) } - return nil } @@ -1854,3 +1826,121 @@ func (issue *Issue) updateClosedNum(e Engine) (err error) { } return } + + +// ResolveMentionsByVisibility returns the users mentioned in an issue, removing those that +// don't have access to reading it. Teams are expanded into their users, but organizations are ignored. +func (issue *Issue) ResolveMentionsByVisibility(e Engine, doer *User, mentions []string) (users []*User, err error) { + if len(mentions) == 0 { + return + } + if err = issue.loadRepo(e); err != nil { + return + } + resolved := make(map[string]bool, 20) + names := make([]string, 0, 20) + resolved[doer.LowerName] = true + for _, name := range mentions { + name := strings.ToLower(name) + if _, ok := resolved[name]; ok { + continue + } + resolved[name] = false + names = append(names, name) + } + + if err := issue.Repo.getOwner(e); err != nil { + return nil, err + } + + if issue.Repo.Owner.IsOrganization() { + // Since there can be users with names that match the name of a team, + // if the team exists and can read the issue, the team takes precedence. + teams := make([]*Team, 0, len(names)) + if err := e. + Join("INNER", "team_repo", "team_repo.team_id = team.id"). + Where("team_repo.repo_id=?", issue.Repo.ID). + In("team.lower_name", names). + Find(&teams); err != nil { + return nil, fmt.Errorf("find mentioned teams: %v", err) + } + if len(teams) != 0 { + checked := make([]int64, 0, len(teams)) + unittype := UnitTypeIssues + if issue.IsPull { + unittype = UnitTypePullRequests + } + for _, team := range teams { + if team.Authorize >= AccessModeOwner { + checked = append(checked, team.ID) + resolved[team.LowerName] = true + continue + } + has, err := e.Get(&TeamUnit{OrgID: issue.Repo.Owner.ID, TeamID: team.ID, Type: unittype}) + if err != nil { + return nil, fmt.Errorf("get team units (%d): %v", team.ID, err) + } + if has { + checked = append(checked, team.ID) + resolved[team.LowerName] = true + } + } + if len(checked) != 0 { + teamusers := make([]*User, 0, 20) + if err := e. + Join("INNER", "team_user", "team_user.uid = `user`.id"). + In("`team_user`.team_id", checked). + And("`user`.is_active = ?", true). + And("`user`.prohibit_login = ?", false). + Find(&teamusers); err != nil { + return nil, fmt.Errorf("get teams users: %v", err) + } + if len(teamusers) > 0 { + users = make([]*User, 0, len(teamusers)) + for _, user := range teamusers { + if already, ok := resolved[user.LowerName]; !ok || !already { + users = append(users, user) + resolved[user.LowerName] = true + } + } + } + } + } + + // Remove names already in the list to avoid querying the database if pending names remain + names = make([]string, 0, len(resolved)) + for name, already := range resolved { + if !already { + names = append(names, name) + } + } + if len(names) == 0 { + return + } + } + + unchecked := make([]*User, 0, len(names)) + if err := e. + Where("`user`.is_active = ?", true). + And("`user`.prohibit_login = ?", false). + In("`user`.lower_name", names). + Find(&unchecked); err != nil { + return nil, fmt.Errorf("find mentioned users: %v", err) + } + for _, user := range unchecked { + if already := resolved[user.LowerName]; already || user.IsOrganization() { + continue + } + // Normal users must have read access to the referencing issue + perm, err := getUserRepoPermission(e, issue.Repo, user) + if err != nil { + return nil, fmt.Errorf("getUserRepoPermission [%d]: %v", user.ID, err) + } + if !perm.CanReadIssuesOrPulls(issue.IsPull) { + continue + } + users = append(users, user) + } + + return +} \ No newline at end of file diff --git a/models/issue_comment.go b/models/issue_comment.go index d5e10fa64506d..0a1d9852cf2fd 100644 --- a/models/issue_comment.go +++ b/models/issue_comment.go @@ -387,11 +387,18 @@ func (c *Comment) MailParticipants(opType ActionType, issue *Issue) (err error) } func (c *Comment) mailParticipants(e Engine, opType ActionType, issue *Issue) (err error) { - mentions := markup.FindAllMentions(c.Content) - if err = UpdateIssueMentions(e, c.IssueID, mentions); err != nil { + rawMentions := markup.FindAllMentions(c.Content) + userMentions, err := issue.ResolveMentionsByVisibility(e, c.Poster, rawMentions) + if err != nil { + return fmt.Errorf("ResolveMentionsByVisibility [%d]: %v", c.IssueID, err) + } + if err = UpdateIssueMentions(e, c.IssueID, userMentions); err != nil { return fmt.Errorf("UpdateIssueMentions [%d]: %v", c.IssueID, err) } - + mentions := make([]string, len(userMentions)) + for i, u := range userMentions { + mentions[i] = u.LowerName + } if len(c.Content) > 0 { if err = mailIssueCommentToParticipants(e, issue, c.Poster, c.Content, c, mentions); err != nil { log.Error("mailIssueCommentToParticipants: %v", err) diff --git a/models/issue_mail.go b/models/issue_mail.go index 01a12b16d2a8e..0023413338cc4 100644 --- a/models/issue_mail.go +++ b/models/issue_mail.go @@ -123,12 +123,19 @@ func (issue *Issue) MailParticipants(doer *User, opType ActionType) (err error) } func (issue *Issue) mailParticipants(e Engine, doer *User, opType ActionType) (err error) { - mentions := markup.FindAllMentions(issue.Content) - - if err = UpdateIssueMentions(e, issue.ID, mentions); err != nil { + rawMentions := markup.FindAllMentions(issue.Content) + userMentions, err := issue.ResolveMentionsByVisibility(e, doer, rawMentions) + if err != nil { + return fmt.Errorf("ResolveMentionsByVisibility [%d]: %v", issue.ID, err) + } + if err = UpdateIssueMentions(e, issue.ID, userMentions); err != nil { return fmt.Errorf("UpdateIssueMentions [%d]: %v", issue.ID, err) } - + mentions := make([]string, len(userMentions)) + for i, u := range userMentions { + mentions[i] = u.LowerName + } + if len(issue.Content) > 0 { if err = mailIssueCommentToParticipants(e, issue, doer, issue.Content, nil, mentions); err != nil { log.Error("mailIssueCommentToParticipants: %v", err) diff --git a/models/issue_test.go b/models/issue_test.go index 1a7e45ae02bda..0108504b4b00c 100644 --- a/models/issue_test.go +++ b/models/issue_test.go @@ -320,3 +320,35 @@ func TestIssue_SearchIssueIDsByKeyword(t *testing.T) { assert.EqualValues(t, 1, total) assert.EqualValues(t, []int64{1}, ids) } + +func TestIssue_ResolveMentions(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + + testSuccess := func(owner, repo, doer string, mentions []string, expected []int64) { + o := AssertExistsAndLoadBean(t, &User{LowerName: owner}).(*User) + r := AssertExistsAndLoadBean(t, &Repository{OwnerID: o.ID, LowerName: repo}).(*Repository) + issue := &Issue{RepoID: r.ID} + d := AssertExistsAndLoadBean(t, &User{LowerName: doer}).(*User) + resolved, err := issue.ResolveMentionsByVisibility(x, d, mentions) + assert.NoError(t, err) + ids := make([]int64, len(resolved)) + for i, user := range resolved { + ids[i] = user.ID + } + sort.Slice(ids, func(i, j int) bool { return ids[i] < ids[j] }) + assert.EqualValues(t, expected, ids) + } + + // Public repo, existing user + testSuccess("user2", "repo1", "user1", []string{"user5"}, []int64{5}) + // Public repo, non-existing user + testSuccess("user2", "repo1", "user1", []string{"nonexisting"}, []int64{}) + // Public repo, doer + testSuccess("user2", "repo1", "user1", []string{"user1"}, []int64{}) + // Private repo, team member + testSuccess("user17", "big_test_private_4", "user20", []string{"user2"}, []int64{2}) + // Private repo, not a team member + testSuccess("user17", "big_test_private_4", "user20", []string{"user5"}, []int64{}) + // Private repo, whole team + testSuccess("user17", "big_test_private_4", "user15", []string{"owners"}, []int64{18}) +} diff --git a/models/org_team.go b/models/org_team.go index 531d235dcd092..371501d216534 100644 --- a/models/org_team.go +++ b/models/org_team.go @@ -252,7 +252,7 @@ func (t *Team) UnitEnabled(tp UnitType) bool { func (t *Team) unitEnabled(e Engine, tp UnitType) bool { if err := t.getUnits(e); err != nil { - log.Warn("Error loading repository (ID: %d) units: %s", t.ID, err.Error()) + log.Warn("Error loading team (ID: %d) units: %s", t.ID, err.Error()) } for _, unit := range t.Units { From ab4c7c1e05de97d35e9c3ce930c53cd14b156c63 Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Sun, 13 Oct 2019 01:35:08 -0300 Subject: [PATCH 2/2] Fix fmt --- models/issue.go | 3 +-- models/issue_mail.go | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/models/issue.go b/models/issue.go index 5a3f2d5ea0c64..ea2e8d0d92ac9 100644 --- a/models/issue.go +++ b/models/issue.go @@ -1827,7 +1827,6 @@ func (issue *Issue) updateClosedNum(e Engine) (err error) { return } - // ResolveMentionsByVisibility returns the users mentioned in an issue, removing those that // don't have access to reading it. Teams are expanded into their users, but organizations are ignored. func (issue *Issue) ResolveMentionsByVisibility(e Engine, doer *User, mentions []string) (users []*User, err error) { @@ -1943,4 +1942,4 @@ func (issue *Issue) ResolveMentionsByVisibility(e Engine, doer *User, mentions [ } return -} \ No newline at end of file +} diff --git a/models/issue_mail.go b/models/issue_mail.go index 0023413338cc4..d429e60629bbb 100644 --- a/models/issue_mail.go +++ b/models/issue_mail.go @@ -135,7 +135,7 @@ func (issue *Issue) mailParticipants(e Engine, doer *User, opType ActionType) (e for i, u := range userMentions { mentions[i] = u.LowerName } - + if len(issue.Content) > 0 { if err = mailIssueCommentToParticipants(e, issue, doer, issue.Content, nil, mentions); err != nil { log.Error("mailIssueCommentToParticipants: %v", err)