From 76de543978c49965b9b5a3b2710a543187859e26 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sat, 6 Mar 2021 17:35:52 +0000 Subject: [PATCH 1/9] Never add labels not from this repository or organisation and remove org labels on transfer Prevent the addition of labels from outside of the repository or organisation and remove organisation labels on transfer. Related #14908 Signed-off-by: Andrew Thornton --- models/issue.go | 14 +++++++++++++- models/issue_label.go | 23 ++++++++++++++++++++--- models/repo_transfer.go | 22 ++++++++++++++++++++++ 3 files changed, 55 insertions(+), 4 deletions(-) diff --git a/models/issue.go b/models/issue.go index 1b634ed9e8d3..3a7a0cd41ae3 100644 --- a/models/issue.go +++ b/models/issue.go @@ -513,6 +513,10 @@ func (issue *Issue) ReplaceLabels(labels []*Label, doer *User) (err error) { return err } + if err = issue.loadRepo(sess); err != nil { + return err + } + if err = issue.loadLabels(sess); err != nil { return err } @@ -527,10 +531,18 @@ func (issue *Issue) ReplaceLabels(labels []*Label, doer *User) (err error) { addLabel := labels[addIndex] removeLabel := issue.Labels[removeIndex] if addLabel.ID == removeLabel.ID { + // Silently drop invalid labels + if removeLabel.RepoID != issue.RepoID && removeLabel.OrgID != issue.Repo.OwnerID { + toRemove = append(toRemove, removeLabel) + } + addIndex++ removeIndex++ } else if addLabel.ID < removeLabel.ID { - toAdd = append(toAdd, addLabel) + // Only add if the label is valid + if addLabel.RepoID == issue.RepoID || addLabel.OrgID == issue.Repo.OwnerID { + toAdd = append(toAdd, addLabel) + } addIndex++ } else { toRemove = append(toRemove, removeLabel) diff --git a/models/issue_label.go b/models/issue_label.go index 54b286fe7e02..3561635d864b 100644 --- a/models/issue_label.go +++ b/models/issue_label.go @@ -632,6 +632,8 @@ func HasIssueLabel(issueID, labelID int64) bool { return hasIssueLabel(x, issueID, labelID) } +// newIssueLabel this function creates a new label it does not check if the label is valid for the issue +// YOU MUST CHECK THIS BEFORE THIS FUNCTION func newIssueLabel(e *xorm.Session, issue *Issue, label *Label, doer *User) (err error) { if _, err = e.Insert(&IssueLabel{ IssueID: issue.ID, @@ -671,6 +673,15 @@ func NewIssueLabel(issue *Issue, label *Label, doer *User) (err error) { return err } + if err = issue.loadRepo(sess); err != nil { + return err + } + + // Do NOT add invalid labels + if issue.RepoID != label.RepoID && issue.Repo.OwnerID != label.OrgID { + return nil + } + if err = newIssueLabel(sess, issue, label, doer); err != nil { return err } @@ -683,13 +694,19 @@ func NewIssueLabel(issue *Issue, label *Label, doer *User) (err error) { return sess.Commit() } +// newIssueLabels add labels to an issue. It will check if the labels are valid for the issue func newIssueLabels(e *xorm.Session, issue *Issue, labels []*Label, doer *User) (err error) { - for i := range labels { - if hasIssueLabel(e, issue.ID, labels[i].ID) { + if err = issue.loadRepo(e); err != nil { + return err + } + for _, label := range labels { + // Don't add already present labels and invalid labels + if hasIssueLabel(e, issue.ID, label.ID) || + (label.RepoID != issue.RepoID && label.OrgID != issue.Repo.OwnerID) { continue } - if err = newIssueLabel(e, issue, labels[i], doer); err != nil { + if err = newIssueLabel(e, issue, label, doer); err != nil { return fmt.Errorf("newIssueLabel: %v", err) } } diff --git a/models/repo_transfer.go b/models/repo_transfer.go index 9f8fd649b6f3..280322cfc0ef 100644 --- a/models/repo_transfer.go +++ b/models/repo_transfer.go @@ -11,6 +11,7 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/util" + "xorm.io/builder" ) // RepoTransfer is used to manage repository transfers @@ -325,6 +326,27 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) (err e } } + // Delete labels that belong to the old organization and comments that added these labels + if oldOwner.IsOrganization() { + if _, err := sess. + In("id", builder.Select("issue_label.id"). + Where(builder.Expr("issue.repo_id =? AND (issue.repo_id != label.repo_id OR (label.repo_id = 0 AND label.org_id != ?))", repo.ID, newOwner.ID)). + From("issue_label"). + Join("inner", "label", "issue_label.id = label.id "). + Join("inner", "issue", "issue.id = issue_label.issue_id ")). + Delete(new(IssueLabel)); err != nil { + return fmt.Errorf("Unable to remove old org labels: %v", err) + } + if _, err := sess.In("id", builder.Select("comment.id"). + Where(builder.Expr("comment.type = ? AND issue.repo_id = ? AND (issue.repo_id != label.repo_id OR (label.repo_id = 0 AND label.org_id != ?))", CommentTypeLabel, repo.ID, newOwner.ID)). + From("comment"). + Join("inner", "label", "label.id = comment.label_id"). + Join("inner", "issue", "issue.id = comment.issue_id ")). + Delete(new(Comment)); err != nil { + return fmt.Errorf("Unable to remove old org label comments: %v", err) + } + } + // Rename remote repository to new path and delete local copy. dir := UserPath(newOwner.Name) From 7baa9cc6c1f3bd17caa9597c14cd7f894042ca26 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 8 Mar 2021 17:29:51 +0000 Subject: [PATCH 2/9] switch to use sql Signed-off-by: Andrew Thornton --- models/repo_transfer.go | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/models/repo_transfer.go b/models/repo_transfer.go index 280322cfc0ef..ef74f133dfcf 100644 --- a/models/repo_transfer.go +++ b/models/repo_transfer.go @@ -11,7 +11,6 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/util" - "xorm.io/builder" ) // RepoTransfer is used to manage repository transfers @@ -328,21 +327,27 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) (err e // Delete labels that belong to the old organization and comments that added these labels if oldOwner.IsOrganization() { - if _, err := sess. - In("id", builder.Select("issue_label.id"). - Where(builder.Expr("issue.repo_id =? AND (issue.repo_id != label.repo_id OR (label.repo_id = 0 AND label.org_id != ?))", repo.ID, newOwner.ID)). - From("issue_label"). - Join("inner", "label", "issue_label.id = label.id "). - Join("inner", "issue", "issue.id = issue_label.issue_id ")). - Delete(new(IssueLabel)); err != nil { + if _, err := sess.Exec(`DELETE FROM issue_label AS il WHERE il.id IN ( + SELECT id FROM ( + SELECT issue_label.id + FROM issue_label + INNER JOIN label ON issue_label.id = label.id + INNER JOIN issue on issue.id = issue_label.issue_id + WHERE + issue.repo_id = ? AND (issue.repo_id != label.repo_id OR (label.repo_id = 0 AND label.org_id != ?)) + ))`, repo.ID, newOwner.ID); err != nil { return fmt.Errorf("Unable to remove old org labels: %v", err) } - if _, err := sess.In("id", builder.Select("comment.id"). - Where(builder.Expr("comment.type = ? AND issue.repo_id = ? AND (issue.repo_id != label.repo_id OR (label.repo_id = 0 AND label.org_id != ?))", CommentTypeLabel, repo.ID, newOwner.ID)). - From("comment"). - Join("inner", "label", "label.id = comment.label_id"). - Join("inner", "issue", "issue.id = comment.issue_id ")). - Delete(new(Comment)); err != nil { + + if _, err := sess.Exec(`DELETE FROM comment AS com WHERE com.id IN ( + SELECT id FROM ( + SELECT comment.id + FROM comment + INNER JOIN label ON comment.label_id = label.id + INNER JOIN issue on issue.id = comment.issue_id + WHERE + comment.type = ? AND issue.repo_id = ? AND (issue.repo_id != label.repo_id OR (label.repo_id = 0 AND label.org_id != ?)) + ))`, CommentTypeLabel, repo.ID, newOwner.ID); err != nil { return fmt.Errorf("Unable to remove old org label comments: %v", err) } } From b83161fa0e42dcd49d8d507e32231cc534913b7f Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 8 Mar 2021 17:51:33 +0000 Subject: [PATCH 3/9] remove AS Signed-off-by: Andrew Thornton --- models/repo_transfer.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/models/repo_transfer.go b/models/repo_transfer.go index ef74f133dfcf..28f9fda97c07 100644 --- a/models/repo_transfer.go +++ b/models/repo_transfer.go @@ -327,7 +327,7 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) (err e // Delete labels that belong to the old organization and comments that added these labels if oldOwner.IsOrganization() { - if _, err := sess.Exec(`DELETE FROM issue_label AS il WHERE il.id IN ( + if _, err := sess.Exec(`DELETE FROM issue_label il WHERE il.id IN ( SELECT id FROM ( SELECT issue_label.id FROM issue_label @@ -339,7 +339,7 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) (err e return fmt.Errorf("Unable to remove old org labels: %v", err) } - if _, err := sess.Exec(`DELETE FROM comment AS com WHERE com.id IN ( + if _, err := sess.Exec(`DELETE FROM comment com WHERE com.id IN ( SELECT id FROM ( SELECT comment.id FROM comment From f1c5131c3d84f624a516e85e44ffc8456e9d8868 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 8 Mar 2021 17:54:21 +0000 Subject: [PATCH 4/9] subquery alias Signed-off-by: Andrew Thornton --- models/repo_transfer.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/models/repo_transfer.go b/models/repo_transfer.go index 28f9fda97c07..e9261a1cf0d8 100644 --- a/models/repo_transfer.go +++ b/models/repo_transfer.go @@ -328,26 +328,26 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) (err e // Delete labels that belong to the old organization and comments that added these labels if oldOwner.IsOrganization() { if _, err := sess.Exec(`DELETE FROM issue_label il WHERE il.id IN ( - SELECT id FROM ( + SELECT il_too.id FROM ( SELECT issue_label.id FROM issue_label INNER JOIN label ON issue_label.id = label.id INNER JOIN issue on issue.id = issue_label.issue_id WHERE issue.repo_id = ? AND (issue.repo_id != label.repo_id OR (label.repo_id = 0 AND label.org_id != ?)) - ))`, repo.ID, newOwner.ID); err != nil { + ) il_too )`, repo.ID, newOwner.ID); err != nil { return fmt.Errorf("Unable to remove old org labels: %v", err) } if _, err := sess.Exec(`DELETE FROM comment com WHERE com.id IN ( - SELECT id FROM ( + SELECT il_too.id FROM ( SELECT comment.id FROM comment INNER JOIN label ON comment.label_id = label.id INNER JOIN issue on issue.id = comment.issue_id WHERE comment.type = ? AND issue.repo_id = ? AND (issue.repo_id != label.repo_id OR (label.repo_id = 0 AND label.org_id != ?)) - ))`, CommentTypeLabel, repo.ID, newOwner.ID); err != nil { + ) il_too)`, CommentTypeLabel, repo.ID, newOwner.ID); err != nil { return fmt.Errorf("Unable to remove old org label comments: %v", err) } } From e669f65bc7794b0ab9c0f82f08277beb21958fa1 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 8 Mar 2021 18:28:44 +0000 Subject: [PATCH 5/9] Give me some AS? Signed-off-by: Andrew Thornton --- models/repo_transfer.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/models/repo_transfer.go b/models/repo_transfer.go index e9261a1cf0d8..baf82dfe2d45 100644 --- a/models/repo_transfer.go +++ b/models/repo_transfer.go @@ -335,7 +335,7 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) (err e INNER JOIN issue on issue.id = issue_label.issue_id WHERE issue.repo_id = ? AND (issue.repo_id != label.repo_id OR (label.repo_id = 0 AND label.org_id != ?)) - ) il_too )`, repo.ID, newOwner.ID); err != nil { + ) AS il_too )`, repo.ID, newOwner.ID); err != nil { return fmt.Errorf("Unable to remove old org labels: %v", err) } @@ -347,7 +347,7 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) (err e INNER JOIN issue on issue.id = comment.issue_id WHERE comment.type = ? AND issue.repo_id = ? AND (issue.repo_id != label.repo_id OR (label.repo_id = 0 AND label.org_id != ?)) - ) il_too)`, CommentTypeLabel, repo.ID, newOwner.ID); err != nil { + ) AS il_too)`, CommentTypeLabel, repo.ID, newOwner.ID); err != nil { return fmt.Errorf("Unable to remove old org label comments: %v", err) } } From 31b0f60cbf0319a82f10e28bdafd6cbf0d458265 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 8 Mar 2021 19:07:48 +0000 Subject: [PATCH 6/9] double AS Signed-off-by: Andrew Thornton --- models/repo_transfer.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/models/repo_transfer.go b/models/repo_transfer.go index baf82dfe2d45..e6d851283bc7 100644 --- a/models/repo_transfer.go +++ b/models/repo_transfer.go @@ -327,7 +327,7 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) (err e // Delete labels that belong to the old organization and comments that added these labels if oldOwner.IsOrganization() { - if _, err := sess.Exec(`DELETE FROM issue_label il WHERE il.id IN ( + if _, err := sess.Exec(`DELETE FROM issue_label AS il WHERE il.id IN ( SELECT il_too.id FROM ( SELECT issue_label.id FROM issue_label @@ -339,7 +339,7 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) (err e return fmt.Errorf("Unable to remove old org labels: %v", err) } - if _, err := sess.Exec(`DELETE FROM comment com WHERE com.id IN ( + if _, err := sess.Exec(`DELETE FROM comment AS com WHERE com.id IN ( SELECT il_too.id FROM ( SELECT comment.id FROM comment From 6264fc44fc8d198bc87c837abc7c5c3507d82d54 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 8 Mar 2021 19:38:27 +0000 Subject: [PATCH 7/9] try try again Signed-off-by: Andrew Thornton --- models/repo_transfer.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/models/repo_transfer.go b/models/repo_transfer.go index e6d851283bc7..b5f16899cb82 100644 --- a/models/repo_transfer.go +++ b/models/repo_transfer.go @@ -327,26 +327,26 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) (err e // Delete labels that belong to the old organization and comments that added these labels if oldOwner.IsOrganization() { - if _, err := sess.Exec(`DELETE FROM issue_label AS il WHERE il.id IN ( + if _, err := sess.Exec(`DELETE FROM issue_label WHERE issue_label.id IN ( SELECT il_too.id FROM ( - SELECT issue_label.id - FROM issue_label - INNER JOIN label ON issue_label.id = label.id - INNER JOIN issue on issue.id = issue_label.issue_id + SELECT il_too_too.id + FROM issue_label AS il_too_too + INNER JOIN label ON il_too_too.id = label.id + INNER JOIN issue on issue.id = il_too_too.issue_id WHERE issue.repo_id = ? AND (issue.repo_id != label.repo_id OR (label.repo_id = 0 AND label.org_id != ?)) ) AS il_too )`, repo.ID, newOwner.ID); err != nil { return fmt.Errorf("Unable to remove old org labels: %v", err) } - if _, err := sess.Exec(`DELETE FROM comment AS com WHERE com.id IN ( + if _, err := sess.Exec(`DELETE FROM comment WHERE comment.id IN ( SELECT il_too.id FROM ( SELECT comment.id - FROM comment - INNER JOIN label ON comment.label_id = label.id - INNER JOIN issue on issue.id = comment.issue_id + FROM comment AS com + INNER JOIN label ON com.label_id = label.id + INNER JOIN issue on issue.id = com.issue_id WHERE - comment.type = ? AND issue.repo_id = ? AND (issue.repo_id != label.repo_id OR (label.repo_id = 0 AND label.org_id != ?)) + com.type = ? AND issue.repo_id = ? AND (issue.repo_id != label.repo_id OR (label.repo_id = 0 AND label.org_id != ?)) ) AS il_too)`, CommentTypeLabel, repo.ID, newOwner.ID); err != nil { return fmt.Errorf("Unable to remove old org label comments: %v", err) } From 5a10bac0db874849ead8a3cb0661443ae88e6664 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 8 Mar 2021 19:55:15 +0000 Subject: [PATCH 8/9] once more around the merry go round Signed-off-by: Andrew Thornton --- models/repo_transfer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/repo_transfer.go b/models/repo_transfer.go index b5f16899cb82..273dca1c5d18 100644 --- a/models/repo_transfer.go +++ b/models/repo_transfer.go @@ -341,7 +341,7 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) (err e if _, err := sess.Exec(`DELETE FROM comment WHERE comment.id IN ( SELECT il_too.id FROM ( - SELECT comment.id + SELECT com.id FROM comment AS com INNER JOIN label ON com.label_id = label.id INNER JOIN issue on issue.id = com.issue_id From cd215d0681756b62304279c3e902d579b55dee7e Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Thu, 11 Mar 2021 11:57:40 +0000 Subject: [PATCH 9/9] fix api problem Signed-off-by: Andrew Thornton --- integrations/api_issue_label_test.go | 8 ++++---- models/issue_label.go | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/integrations/api_issue_label_test.go b/integrations/api_issue_label_test.go index ddcfdd661589..64c3515dd2bb 100644 --- a/integrations/api_issue_label_test.go +++ b/integrations/api_issue_label_test.go @@ -91,22 +91,22 @@ func TestAPIAddIssueLabels(t *testing.T) { repo := models.AssertExistsAndLoadBean(t, &models.Repository{ID: 1}).(*models.Repository) issue := models.AssertExistsAndLoadBean(t, &models.Issue{RepoID: repo.ID}).(*models.Issue) - label := models.AssertExistsAndLoadBean(t, &models.Label{RepoID: repo.ID}).(*models.Label) + _ = models.AssertExistsAndLoadBean(t, &models.Label{RepoID: repo.ID, ID: 2}).(*models.Label) owner := models.AssertExistsAndLoadBean(t, &models.User{ID: repo.OwnerID}).(*models.User) session := loginUser(t, owner.Name) token := getTokenForLoggedInUser(t, session) urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/issues/%d/labels?token=%s", - owner.Name, repo.Name, issue.Index, token) + repo.OwnerName, repo.Name, issue.Index, token) req := NewRequestWithJSON(t, "POST", urlStr, &api.IssueLabelsOption{ - Labels: []int64{label.ID}, + Labels: []int64{1, 2}, }) resp := session.MakeRequest(t, req, http.StatusOK) var apiLabels []*api.Label DecodeJSON(t, resp, &apiLabels) assert.Len(t, apiLabels, models.GetCount(t, &models.IssueLabel{IssueID: issue.ID})) - models.AssertExistsAndLoadBean(t, &models.IssueLabel{IssueID: issue.ID, LabelID: label.ID}) + models.AssertExistsAndLoadBean(t, &models.IssueLabel{IssueID: issue.ID, LabelID: 2}) } func TestAPIReplaceIssueLabels(t *testing.T) { diff --git a/models/issue_label.go b/models/issue_label.go index 3561635d864b..1b5cfd88d5b9 100644 --- a/models/issue_label.go +++ b/models/issue_label.go @@ -321,7 +321,7 @@ func GetLabelsByIDs(labelIDs []int64) ([]*Label, error) { return labels, x.Table("label"). In("id", labelIDs). Asc("name"). - Cols("id"). + Cols("id", "repo_id", "org_id"). Find(&labels) }