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

* dont insert "-1" in any case to issue.poster_id

* Make sure API cant override importand fields

* code format

* fix lint

* WIP test

* add missing poster_id

* fix test

* user.IsGhost handle nil

* CI.restart()

* make sure no -1 is realy added

* CI.restart()

* @lunny suggestion remove some not allowed fields

* seperate issue.LoadMilestone

* load milestone and return it on IssueEdit via API

* extend Test for TestAPIEditIssue

* fix fixtures

* declare allowedColumnsUpdateIssueByAPI only once

* Update Year

* no var just write id drecty into func cal

Co-authored-by: Lauris BH <lauris@nix.lv>

diff --git a/integrations/api_issue_test.go b/integrations/api_issue_test.go
index f412f5af085da2e3097c5ecf736d0d8849f3d912..382fe606bf3563cbb7e699a42b518f2ae133a2f3 100644 (file)
--- 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: 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())
+       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
index 6b57268a7a027c4dcb8fc34adc030ac3d91ad850..ecee7499f6f047e07509225ddc53f3a269bf7a0b 100644 (file)
--- a/models/fixtures/issue.yml
+++ b/models/fixtures/issue.yml
@@ -108,4 +108,17 @@
   is_closed: false
   is_pull: true
   created_unix: 946684820
-  updated_unix: 978307180
\ No newline at end of file
+  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
+  deadline_unix: 1019307200

diff --git a/models/fixtures/milestone.yml b/models/fixtures/milestone.yml
index 15f422fc3b5c200ab5a8bcd7103163c87eaab291..a9ecb4ee6a6efb41437081aea98b322277977254 100644 (file)
--- 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
index feec0b5fafb97230f184a791794895cc711805fa..c7f4d4d1096ab478cabaaf76676a1d1619ea81b7 100644 (file)
--- 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
 
 -
@@ -588,7 +589,7 @@
   is_mirror: false
   status: 0
 
-- 
+-
   id: 46
   owner_id: 26
   lower_name: repo_external_tracker
@@ -600,7 +601,7 @@
   is_mirror: false
   status: 0
 
-- 
+-
   id: 47
   owner_id: 26
   lower_name: repo_external_tracker_numeric
@@ -612,7 +613,7 @@
   is_mirror: false
   status: 0
 
-- 
+-
   id: 48
   owner_id: 26
   lower_name: repo_external_tracker_alpha

diff --git a/models/issue.go b/models/issue.go
index 75f7bd818aa6eed09ef6b0b42ea06504afddf7ef..c832b9d014473222f1c0f53d4dd25e0b6eaf24ee 100644 (file)
--- 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.
 
@@ -239,6 +240,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
@@ -252,11 +263,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 {
@@ -296,6 +304,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}
@@ -1568,25 +1581,18 @@ 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
-}
-
-// 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 {
-               return err
-       }
-       if err := issue.loadPoster(sess); 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.addCrossReferences(sess, issue.Poster, true); err != nil {

diff --git a/models/user.go b/models/user.go
index e832c2ed519b838fa9ef6d8b2e33aeb7e263ad28..a8f2c6fd223362d6fc19330e98f8fb88bcb0cbcd 100644 (file)
--- a/models/user.go
+++ b/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",

diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go
index 4396e6faaebe23c58e0cb2b6cf665f6e19fe1530..ad82d53e7a1909cea02f160da35d9e0ac3173929 100644 (file)
--- a/routers/api/v1/repo/issue.go
+++ b/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 {
@@ -542,7 +542,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(http.StatusInternalServerError, "GetIssueByID", err)
+               ctx.InternalServerError(err)
+               return
+       }
+       if err = issue.LoadMilestone(); err != nil {
+               ctx.InternalServerError(err)
                return
        }
        ctx.JSON(http.StatusCreated, issue.APIFormat())

diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go
index 0392eb8e8c480ef0c39313e3091598521911ebb2..d0551320fdbc7fcf6f1ce0b3fbffc4ff8cdf3403 100644 (file)
--- a/routers/api/v1/repo/pull.go
+++ b/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 {