Fix #9189 - API Allow only specific Colums to be updated on Issue

integrations/api_issue_test.go

@@ -62,3 +62,38 @@ func TestAPICreateIssue(t *testing.T) {
 		Title:      title,
 	})
 }
+
+func TestAPIEditIssue(t *testing.T) {
+	defer prepareTestEnv(t)()
+
+	issueBefore := models.AssertExistsAndLoadBean(t, &models.Issue{ID: 10}).(*models.Issue)
+	repo := models.AssertExistsAndLoadBean(t, &models.Repository{ID: issueBefore.RepoID}).(*models.Repository)
+	owner := models.AssertExistsAndLoadBean(t, &models.User{ID: repo.OwnerID}).(*models.User)
+	assert.NoError(t, issueBefore.LoadAttributes())
+
+	session := loginUser(t, owner.Name)
+	token := getTokenForLoggedInUser(t, session)
+
+	issueState := "closed"
+
+	urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/issues/%d?token=%s", owner.Name, repo.Name, issueBefore.Index, token)
+	req := NewRequestWithJSON(t, "PATCH", urlStr, api.EditIssueOption{
+		State: &issueState,
+		// ToDo change more
+	})
+	resp := session.MakeRequest(t, req, http.StatusCreated)
+	var apiIssue api.Issue
+	DecodeJSON(t, resp, &apiIssue)
+
+	issueAfter := models.AssertExistsAndLoadBean(t, &models.Issue{ID: 10}).(*models.Issue)
+
+	assert.Equal(t, api.StateOpen, issueBefore.State())
+	assert.Equal(t, api.StateClosed, issueAfter.State())
+	// check deleted user
+	assert.Equal(t, int64(500), issueAfter.PosterID)
+	assert.NoError(t, issueAfter.LoadAttributes())
+	assert.Equal(t, int64(-1), issueAfter.PosterID)
+	assert.Equal(t, int64(-1), issueBefore.PosterID)
+	assert.Equal(t, int64(-1), apiIssue.Poster.ID)
+
+}

models/fixtures/issue.yml

@@ -108,4 +108,16 @@
   is_closed: false
   is_pull: true
   created_unix: 946684820
-  updated_unix: 978307180
+  updated_unix: 978307180
+
+-
+  id: 10
+  repo_id: 42
+  index: 1
+  poster_id: 500
+  name: issue from deleted account
+  content: content from deleted account
+  is_closed: false
+  is_pull: false
+  created_unix: 946684830
+  updated_unix: 999307200

models/fixtures/repository.yml

@@ -547,7 +547,7 @@
   is_private: false
   num_stars: 0
   num_forks: 0
-  num_issues: 0
+  num_issues: 1
   is_mirror: false
 
 -
@@ -588,7 +588,7 @@
   is_mirror: false
   status: 0
 
-- 
+-
   id: 46
   owner_id: 26
   lower_name: repo_external_tracker
@@ -600,7 +600,7 @@
   is_mirror: false
   status: 0
 
-- 
+-
   id: 47
   owner_id: 26
   lower_name: repo_external_tracker_numeric
@@ -612,7 +612,7 @@
   is_mirror: false
   status: 0
 
-- 
+-
   id: 48
   owner_id: 26
   lower_name: repo_external_tracker_alpha

models/issue.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.
 
@@ -1568,25 +1569,34 @@ func SearchIssueIDsByKeyword(kw string, repoIDs []int64, limit, start int) (int6
 	return total, ids, nil
 }
 
-func updateIssue(e Engine, issue *Issue) error {
-	_, err := e.ID(issue.ID).AllCols().Update(issue)
-	if err != nil {
-		return err
-	}
-	return nil
+func updateIssueByCols(e Engine, issue *Issue, columns ...string) (err error) {
+	_, err = e.ID(issue.ID).Cols(columns...).Update(issue)
+	return
 }
 
-// UpdateIssue updates all fields of given issue.
-func UpdateIssue(issue *Issue) error {
+// UpdateIssueByAPI updates all allowed fields of given issue.
+func UpdateIssueByAPI(issue *Issue) error {
+	// allowed fields to update
+	columns := []string{"name", "is_closed", "content", "milestone_id",
+		"priority", "num_comments", "ref", "deadline_unix", "created_unix",
+		"updated_unix", "closed_unix", "is_locked"}
+
 	sess := x.NewSession()
 	defer sess.Close()
 	if err := sess.Begin(); err != nil {
 		return err
 	}
-	if err := updateIssue(sess, issue); err != nil {
+	if err := issue.loadPoster(sess); err != nil {
 		return err
 	}
-	if err := issue.loadPoster(sess); err != nil {
+	if !issue.Poster.IsGhost() {
+		if issue.PosterID <= 0 {
+			return fmt.Errorf("Issue %d can't be updated with PosterID %d", issue.Index, issue.PosterID)
+		}
+		columns = append(columns, "poster_id")
+	}
+
+	if err := updateIssueByCols(sess, issue, columns...); err != nil {
 		return err
 	}
 	if err := issue.addCrossReferences(sess, issue.Poster, true); err != nil {

models/user.go

@@ -791,6 +791,14 @@ func NewGhostUser() *User {
 	}
 }
 
+// IsGhost check if user is fake user for a deleted account
+func (u *User) IsGhost() bool {
+	if u == nil {
+		return false
+	}
+	return u.ID == -1 && u.Name == "Ghost"
+}
+
 var (
 	reservedUsernames = []string{
 		"attachments",

routers/api/v1/repo/issue.go

@@ -524,8 +524,8 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) {
 		}
 	}
 
-	if err = models.UpdateIssue(issue); err != nil {
-		ctx.Error(http.StatusInternalServerError, "UpdateIssue", err)
+	if err = models.UpdateIssueByAPI(issue); err != nil {
+		ctx.Error(http.StatusInternalServerError, "UpdateIssueByAPI", err)
 		return
 	}
 	if form.State != nil {

routers/api/v1/repo/pull.go

@@ -450,8 +450,8 @@ func EditPullRequest(ctx *context.APIContext, form api.EditPullRequestOption) {
 		}
 	}
 
-	if err = models.UpdateIssue(issue); err != nil {
-		ctx.Error(http.StatusInternalServerError, "UpdateIssue", err)
+	if err = models.UpdateIssueByAPI(issue); err != nil {
+		ctx.Error(http.StatusInternalServerError, "UpdateIssueByAPI", err)
 		return
 	}
 	if form.State != nil {