Fix go-gitea#9189 - API Allow only specific Colums to be updated on I…

…ssue (go-gitea#9539) * dont insert "-1" in any case to issue.poster_id * Make sure API cant override importand fields * code format * add Test for IssueEdit * load milestone and return it on IssueEdit via API * extend Test for TestAPIEditIssue
6543-forks · Jan 1, 2020 · eb994a4 · eb994a4
1 parent c63a801
commit eb994a4
Show file tree

Hide file tree

Showing 7 changed files with 117 additions and 26 deletions.
diff --git a/integrations/api_issue_test.go b/integrations/api_issue_test.go
@@ -62,3 +62,61 @@ func TestAPICreateIssue(t *testing.T) {
 		Title:      title,
 	})
 }
+
+func TestAPIEditIssue(t *testing.T) {
+	defer prepareTestEnv(t)()
+
+	issueBefore := models.AssertExistsAndLoadBean(t, &models.Issue{ID: 9}).(*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())
+	assert.Equal(t, int64(1019307200), int64(issueBefore.DeadlineUnix))
+	assert.Equal(t, api.StateOpen, issueBefore.State())
+
+	session := loginUser(t, owner.Name)
+	token := getTokenForLoggedInUser(t, session)
+
+	// update values of issue
+	issueState := "closed"
+	removeDeadline := true
+	milestone := int64(4)
+	body := "new content!"
+	title := "new title from api set"
+
+	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,
+		RemoveDeadline: &removeDeadline,
+		Milestone:      &milestone,
+		Body:           &body,
+		Title:          title,
+
+		// 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)
+
+	// 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)
+
+	// API response
+	assert.Equal(t, api.StateClosed, apiIssue.State)
+	assert.Equal(t, milestone, apiIssue.Milestone.ID)
+	assert.Equal(t, body, apiIssue.Body)
+	assert.True(t, apiIssue.Deadline == nil)
+	assert.Equal(t, title, apiIssue.Title)
+
+	// in database
+	assert.Equal(t, api.StateClosed, issueAfter.State())
+	assert.Equal(t, milestone, issueAfter.MilestoneID)
+	assert.Equal(t, int64(0), int64(issueAfter.DeadlineUnix))
+	assert.Equal(t, body, issueAfter.Content)
+	assert.Equal(t, title, issueAfter.Title)
+}
diff --git a/models/fixtures/issue.yml b/models/fixtures/issue.yml
@@ -96,4 +96,17 @@
   is_closed: false
   is_pull: true
   created_unix: 946684820
-  updated_unix: 978307180
+  updated_unix: 978307180
+
+-
+  id: 9
+  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
+  deadline_unix: 1019307200
diff --git a/models/fixtures/milestone.yml b/models/fixtures/milestone.yml
@@ -21,3 +21,11 @@
   content: content3
   is_closed: true
   num_issues: 0
+
+-
+  id: 4
+  repo_id: 42
+  name: milestone of repo42
+  content: content random
+  is_closed: false
+  num_issues: 0
diff --git a/models/fixtures/repository.yml b/models/fixtures/repository.yml
@@ -547,7 +547,8 @@
   is_private: false
   num_stars: 0
   num_forks: 0
-  num_issues: 0
+  num_issues: 1
+  num_milestones: 1
   is_mirror: false
 
 -

diff --git a/models/issue.go b/models/issue.go
@@ -1,4 +1,5 @@
 // Copyright 2014 The Gogs Authors. All rights reserved.
+// Copyright 2020 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.
 
@@ -238,6 +239,16 @@ func (issue *Issue) loadReactions(e Engine) (err error) {
 	return nil
 }
 
+func (issue *Issue) loadMilestone(e Engine) (err error) {
+	if issue.Milestone == nil && issue.MilestoneID > 0 {
+		issue.Milestone, err = getMilestoneByRepoID(e, issue.RepoID, issue.MilestoneID)
+		if err != nil && !IsErrMilestoneNotExist(err) {
+			return fmt.Errorf("getMilestoneByRepoID [repo_id: %d, milestone_id: %d]: %v", issue.RepoID, issue.MilestoneID, err)
+		}
+	}
+	return nil
+}
+
 func (issue *Issue) loadAttributes(e Engine) (err error) {
 	if err = issue.loadRepo(e); err != nil {
 		return
@@ -251,11 +262,8 @@ func (issue *Issue) loadAttributes(e Engine) (err error) {
 		return
 	}
 
-	if issue.Milestone == nil && issue.MilestoneID > 0 {
-		issue.Milestone, err = getMilestoneByRepoID(e, issue.RepoID, issue.MilestoneID)
-		if err != nil && !IsErrMilestoneNotExist(err) {
-			return fmt.Errorf("getMilestoneByRepoID [repo_id: %d, milestone_id: %d]: %v", issue.RepoID, issue.MilestoneID, err)
-		}
+	if err = issue.loadMilestone(e); err != nil {
+		return
 	}
 
 	if err = issue.loadAssignees(e); err != nil {
@@ -295,6 +303,11 @@ func (issue *Issue) LoadAttributes() error {
 	return issue.loadAttributes(x)
 }
 
+// LoadMilestone load milestone of this issue.
+func (issue *Issue) LoadMilestone() error {
+	return issue.loadMilestone(x)
+}
+
 // GetIsRead load the `IsRead` field of the issue
 func (issue *Issue) GetIsRead(userID int64) error {
 	issueUser := &IssueUser{IssueID: issue.ID, UID: userID}
@@ -1730,30 +1743,24 @@ func SearchIssueIDsByKeyword(kw string, repoID int64, limit, start int) (int64,
 	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
-}
-
-// 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 {
 	sess := x.NewSession()
 	defer sess.Close()
 	if err := sess.Begin(); err != nil {
 		return err
 	}
-	if err := updateIssue(sess, issue); err != nil {
+
+	if _, err := sess.ID(issue.ID).Cols(
+		"name", "is_closed", "content", "milestone_id", "priority",
+		"deadline_unix", "updated_unix", "closed_unix", "is_locked").
+		Update(issue); err != nil {
 		return err
 	}
+
 	if err := issue.neuterCrossReferences(sess); err != nil {
 		return err
 	}
-	if err := issue.loadPoster(sess); err != nil {
-		return err
-	}
 	if err := issue.addCrossReferences(sess, issue.Poster); err != nil {
 		return err
 	}

diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go
@@ -352,8 +352,8 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) {
 		}
 	}
 
-	if err = models.UpdateIssue(issue); err != nil {
-		ctx.Error(500, "UpdateIssue", err)
+	if err = models.UpdateIssueByAPI(issue); err != nil {
+		ctx.InternalServerError(err)
 		return
 	}
 	if form.State != nil {
@@ -372,7 +372,11 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) {
 	// Refetch from database to assign some automatic values
 	issue, err = models.GetIssueByID(issue.ID)
 	if err != nil {
-		ctx.Error(500, "GetIssueByID", err)
+		ctx.InternalServerError(err)
+		return
+	}
+	if err = issue.LoadMilestone(); err != nil {
+		ctx.InternalServerError(err)
 		return
 	}
 	ctx.JSON(201, issue.APIFormat())

diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go
@@ -420,8 +420,8 @@ func EditPullRequest(ctx *context.APIContext, form api.EditPullRequestOption) {
 		}
 	}
 
-	if err = models.UpdateIssue(issue); err != nil {
-		ctx.Error(500, "UpdateIssue", err)
+	if err = models.UpdateIssueByAPI(issue); err != nil {
+		ctx.InternalServerError(err)
 		return
 	}
 	if form.State != nil {