From c620eb5b2d0d874da68ebd734d3864c5224f71f7 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Wed, 1 Jan 2020 23:51:10 +0100 Subject: [PATCH] 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 --- integrations/api_issue_test.go | 58 ++++++++++++++++++++++++++++++++++ models/fixtures/issue.yml | 15 ++++++++- models/fixtures/milestone.yml | 8 +++++ models/fixtures/repository.yml | 9 +++--- models/issue.go | 44 +++++++++++++++----------- models/user.go | 8 +++++ routers/api/v1/repo/issue.go | 10 ++++-- routers/api/v1/repo/pull.go | 4 +-- 8 files changed, 127 insertions(+), 29 deletions(-) diff --git a/integrations/api_issue_test.go b/integrations/api_issue_test.go index f412f5af085da..382fe606bf356 100644 --- 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 6b57268a7a027..ecee7499f6f04 100644 --- 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 15f422fc3b5c2..a9ecb4ee6a6ef 100644 --- 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 feec0b5fafb97..c7f4d4d1096ab 100644 --- 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 75f7bd818aa6e..c832b9d014473 100644 --- 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 e832c2ed519b8..a8f2c6fd22336 100644 --- 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 4396e6faaebe2..ad82d53e7a190 100644 --- 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 0392eb8e8c480..d0551320fdbc7 100644 --- 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 {