Never add labels not from this repository or organisation and remove org labels on transfer

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) {

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)

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)
 }
 
@@ -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)
 		}
 	}

models/repo_transfer.go

@@ -325,6 +325,33 @@ 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 WHERE issue_label.id IN (
+			SELECT il_too.id FROM (
+				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 WHERE comment.id IN (
+			SELECT il_too.id FROM (
+				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
+					WHERE
+						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)
+		}
+	}
+
 	// Rename remote repository to new path and delete local copy.
 	dir := UserPath(newOwner.Name)