From abf40ee957adc8d072150db8f5ef63a3f0d73aad Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 2 May 2024 10:27:25 +0800 Subject: [PATCH 1/7] Skip gzip for some well-known compressed file types (#30796) Co-authored-by: silverwind (cherry picked from commit be112c1fc30f87a248b30f48e891d1c8c18e8280) Conflicts: routers/web/web.go trivial conflict because of https://codeberg.org/forgejo/forgejo/pulls/1533 (cherry picked from commit 4e35e5b8ae2bf01694bca76bd6fbee9c58934388) --- modules/httplib/serve.go | 8 +++++++ tests/integration/repo_archive_test.go | 31 ++++++++++++++++++++++++++ 2 files changed, 39 insertions(+) create mode 100644 tests/integration/repo_archive_test.go diff --git a/modules/httplib/serve.go b/modules/httplib/serve.go index a193ed901c..6e147d76f5 100644 --- a/modules/httplib/serve.go +++ b/modules/httplib/serve.go @@ -17,11 +17,14 @@ import ( "time" charsetModule "code.gitea.io/gitea/modules/charset" + "code.gitea.io/gitea/modules/container" "code.gitea.io/gitea/modules/httpcache" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/typesniffer" "code.gitea.io/gitea/modules/util" + + "github.com/klauspost/compress/gzhttp" ) type ServeHeaderOptions struct { @@ -38,6 +41,11 @@ type ServeHeaderOptions struct { func ServeSetHeaders(w http.ResponseWriter, opts *ServeHeaderOptions) { header := w.Header() + skipCompressionExts := container.SetOf(".gz", ".bz2", ".zip", ".xz", ".zst", ".deb", ".apk", ".jar", ".png", ".jpg", ".webp") + if skipCompressionExts.Contains(strings.ToLower(path.Ext(opts.Filename))) { + w.Header().Add(gzhttp.HeaderNoCompression, "1") + } + contentType := typesniffer.ApplicationOctetStream if opts.ContentType != "" { if opts.ContentTypeCharset != "" { diff --git a/tests/integration/repo_archive_test.go b/tests/integration/repo_archive_test.go new file mode 100644 index 0000000000..ebc6467378 --- /dev/null +++ b/tests/integration/repo_archive_test.go @@ -0,0 +1,31 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package integration + +import ( + "io" + "net/http" + "testing" + + "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/test" + "code.gitea.io/gitea/routers" + "code.gitea.io/gitea/tests" + + "github.com/stretchr/testify/assert" +) + +func TestRepoDownloadArchive(t *testing.T) { + defer tests.PrepareTestEnv(t)() + defer test.MockVariableValue(&setting.EnableGzip, true)() + defer test.MockVariableValue(&testWebRoutes, routers.NormalRoutes())() + + req := NewRequest(t, "GET", "/user2/repo1/archive/master.zip") + req.Header.Set("Accept-Encoding", "gzip") + resp := MakeRequest(t, req, http.StatusOK) + bs, err := io.ReadAll(resp.Body) + assert.NoError(t, err) + assert.Empty(t, resp.Header().Get("Content-Encoding")) + assert.Equal(t, 320, len(bs)) +} From a7124df0c5bd1786e96421afb1529888f646feca Mon Sep 17 00:00:00 2001 From: silverwind Date: Thu, 2 May 2024 16:56:17 +0200 Subject: [PATCH 2/7] Add hover outline to heatmap squares (#30828) Makes it easier to use because you see which square is currently hovered: Screenshot 2024-05-02 at 15 38 20 I did try a `scoped` style for this, but that did not work for some reason. (cherry picked from commit 6f89d5e3a0886d02ead732005f593ae003f78f78) --- web_src/css/features/heatmap.css | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/web_src/css/features/heatmap.css b/web_src/css/features/heatmap.css index 364754751a..c064590c46 100644 --- a/web_src/css/features/heatmap.css +++ b/web_src/css/features/heatmap.css @@ -31,6 +31,10 @@ padding: 0 5px; } +#user-heatmap .vch__day__square:hover { + outline: 1.5px solid var(--color-text); +} + /* move the "? contributions in the last ? months" text from top to bottom */ #user-heatmap .total-contributions { font-size: 11px; From 5678e9ab205fcc99383a4c4cf2d7e5a2c9d169e7 Mon Sep 17 00:00:00 2001 From: Kemal Zebari <60799661+kemzeb@users.noreply.github.com> Date: Thu, 2 May 2024 09:33:31 -0700 Subject: [PATCH 3/7] Catch and handle unallowed file type errors in issue attachment API (#30791) Before, we would just throw 500 if a user passes an attachment that is not an allowed type. This commit catches this error and throws a 422 instead since this should be considered a validation error. (cherry picked from commit 872caa17c0a30d95f85ab75c068d606e07bd10b3) Conflicts: tests/integration/api_comment_attachment_test.go tests/integration/api_issue_attachment_test.go trivial context conflict because of 'allow setting the update date on issues and comments' (cherry picked from commit 9cd0441cd384f7fbff8e973182c6094270e7f97e) --- routers/api/v1/repo/issue_attachment.go | 9 +++++- .../api/v1/repo/issue_comment_attachment.go | 10 ++++++- templates/swagger/v1_json.tmpl | 6 ++++ .../api_comment_attachment_test.go | 28 +++++++++++++++++++ .../integration/api_issue_attachment_test.go | 27 ++++++++++++++++++ 5 files changed, 78 insertions(+), 2 deletions(-) diff --git a/routers/api/v1/repo/issue_attachment.go b/routers/api/v1/repo/issue_attachment.go index 6f3e4b5e77..658d18094a 100644 --- a/routers/api/v1/repo/issue_attachment.go +++ b/routers/api/v1/repo/issue_attachment.go @@ -15,6 +15,7 @@ import ( "code.gitea.io/gitea/modules/web" "code.gitea.io/gitea/services/attachment" "code.gitea.io/gitea/services/context" + "code.gitea.io/gitea/services/context/upload" "code.gitea.io/gitea/services/convert" issue_service "code.gitea.io/gitea/services/issue" ) @@ -159,6 +160,8 @@ func CreateIssueAttachment(ctx *context.APIContext) { // "$ref": "#/responses/error" // "404": // "$ref": "#/responses/error" + // "422": + // "$ref": "#/responses/validationError" // "423": // "$ref": "#/responses/repoArchivedError" @@ -207,7 +210,11 @@ func CreateIssueAttachment(ctx *context.APIContext) { CreatedUnix: issue.UpdatedUnix, }) if err != nil { - ctx.Error(http.StatusInternalServerError, "UploadAttachment", err) + if upload.IsErrFileTypeForbidden(err) { + ctx.Error(http.StatusUnprocessableEntity, "", err) + } else { + ctx.Error(http.StatusInternalServerError, "UploadAttachment", err) + } return } diff --git a/routers/api/v1/repo/issue_comment_attachment.go b/routers/api/v1/repo/issue_comment_attachment.go index 0f8fc96f08..ed8ea10293 100644 --- a/routers/api/v1/repo/issue_comment_attachment.go +++ b/routers/api/v1/repo/issue_comment_attachment.go @@ -15,6 +15,7 @@ import ( "code.gitea.io/gitea/modules/web" "code.gitea.io/gitea/services/attachment" "code.gitea.io/gitea/services/context" + "code.gitea.io/gitea/services/context/upload" "code.gitea.io/gitea/services/convert" issue_service "code.gitea.io/gitea/services/issue" ) @@ -156,6 +157,8 @@ func CreateIssueCommentAttachment(ctx *context.APIContext) { // "$ref": "#/responses/error" // "404": // "$ref": "#/responses/error" + // "422": + // "$ref": "#/responses/validationError" // "423": // "$ref": "#/responses/repoArchivedError" @@ -209,9 +212,14 @@ func CreateIssueCommentAttachment(ctx *context.APIContext) { CreatedUnix: comment.Issue.UpdatedUnix, }) if err != nil { - ctx.Error(http.StatusInternalServerError, "UploadAttachment", err) + if upload.IsErrFileTypeForbidden(err) { + ctx.Error(http.StatusUnprocessableEntity, "", err) + } else { + ctx.Error(http.StatusInternalServerError, "UploadAttachment", err) + } return } + if err := comment.LoadAttachments(ctx); err != nil { ctx.Error(http.StatusInternalServerError, "LoadAttachments", err) return diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index bcf370b3fb..41c7cc61ef 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -6974,6 +6974,9 @@ "404": { "$ref": "#/responses/error" }, + "422": { + "$ref": "#/responses/validationError" + }, "423": { "$ref": "#/responses/repoArchivedError" } @@ -7600,6 +7603,9 @@ "404": { "$ref": "#/responses/error" }, + "422": { + "$ref": "#/responses/validationError" + }, "423": { "$ref": "#/responses/repoArchivedError" } diff --git a/tests/integration/api_comment_attachment_test.go b/tests/integration/api_comment_attachment_test.go index d4368d51fe..1b3cae91e2 100644 --- a/tests/integration/api_comment_attachment_test.go +++ b/tests/integration/api_comment_attachment_test.go @@ -240,3 +240,31 @@ func TestAPIDeleteCommentAttachment(t *testing.T) { unittest.AssertNotExistsBean(t, &repo_model.Attachment{ID: attachment.ID, CommentID: comment.ID}) } + +func TestAPICreateCommentAttachmentWithUnallowedFile(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 2}) + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: comment.IssueID}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: issue.RepoID}) + repoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) + + session := loginUser(t, repoOwner.Name) + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteIssue) + + filename := "file.bad" + body := &bytes.Buffer{} + + // Setup multi-part. + writer := multipart.NewWriter(body) + _, err := writer.CreateFormFile("attachment", filename) + assert.NoError(t, err) + err = writer.Close() + assert.NoError(t, err) + + req := NewRequestWithBody(t, "POST", fmt.Sprintf("/api/v1/repos/%s/%s/issues/comments/%d/assets", repoOwner.Name, repo.Name, comment.ID), body). + AddTokenAuth(token). + SetHeader("Content-Type", writer.FormDataContentType()) + + session.MakeRequest(t, req, http.StatusUnprocessableEntity) +} diff --git a/tests/integration/api_issue_attachment_test.go b/tests/integration/api_issue_attachment_test.go index b6a0cca6d5..919dea2e62 100644 --- a/tests/integration/api_issue_attachment_test.go +++ b/tests/integration/api_issue_attachment_test.go @@ -173,6 +173,33 @@ func TestAPICreateIssueAttachmentAutoDate(t *testing.T) { }) } +func TestAPICreateIssueAttachmentWithUnallowedFile(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{RepoID: repo.ID}) + repoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) + + session := loginUser(t, repoOwner.Name) + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteIssue) + + filename := "file.bad" + body := &bytes.Buffer{} + + // Setup multi-part. + writer := multipart.NewWriter(body) + _, err := writer.CreateFormFile("attachment", filename) + assert.NoError(t, err) + err = writer.Close() + assert.NoError(t, err) + + req := NewRequestWithBody(t, "POST", fmt.Sprintf("/api/v1/repos/%s/%s/issues/%d/assets", repoOwner.Name, repo.Name, issue.Index), body). + AddTokenAuth(token) + req.Header.Add("Content-Type", writer.FormDataContentType()) + + session.MakeRequest(t, req, http.StatusUnprocessableEntity) +} + func TestAPIEditIssueAttachment(t *testing.T) { defer tests.PrepareTestEnv(t)() From 248a5b8d7a20311e89c7bfa46dd4efb4a0576a95 Mon Sep 17 00:00:00 2001 From: Giteabot Date: Fri, 3 May 2024 03:06:32 +0800 Subject: [PATCH 4/7] Prevent automatic OAuth grants for public clients (#30790) (#30836) Backport #30790 by archer-321 This commit forces the resource owner (user) to always approve OAuth 2.0 authorization requests if the client is public (e.g. native applications). As detailed in [RFC 6749 Section 10.2](https://www.rfc-editor.org/rfc/rfc6749.html#section-10.2), > The authorization server SHOULD NOT process repeated authorization requests automatically (without active resource owner interaction) without authenticating the client or relying on other measures to ensure that the repeated request comes from the original client and not an impersonator. With the implementation prior to this patch, attackers with access to the redirect URI (e.g., the loopback interface for `git-credential-oauth`) can get access to the user account without any user interaction if they can redirect the user to the `/login/oauth/authorize` endpoint somehow (e.g., with `xdg-open` on Linux). Fixes #25061. Co-authored-by: Archer Co-authored-by: wxiaoguang (cherry picked from commit 6d83f5eddc0f394f6386e80b86a3221f6f4925ff) --- routers/web/auth/oauth.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index f5ca0bda5e..ca19d1ea16 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -469,8 +469,9 @@ func AuthorizeOAuth(ctx *context.Context) { return } - // Redirect if user already granted access - if grant != nil { + // Redirect if user already granted access and the application is confidential. + // I.e. always require authorization for public clients as recommended by RFC 6749 Section 10.2 + if app.ConfidentialClient && grant != nil { code, err := grant.GenerateNewAuthorizationCode(ctx, form.RedirectURI, form.CodeChallenge, form.CodeChallengeMethod) if err != nil { handleServerError(ctx, form.State, form.RedirectURI) From f30c648037d0542e965967fb872d7ba63458e53d Mon Sep 17 00:00:00 2001 From: Giteabot Date: Fri, 3 May 2024 12:20:34 +0800 Subject: [PATCH 5/7] Ignore useless error message "broken pipe" (#30801) (#30842) Backport #30801 by wxiaoguang Fix #30792 Co-authored-by: wxiaoguang (cherry picked from commit ab2ef1ae49bc5e81d0debac85aee687a64fde8b3) --- routers/api/packages/maven/maven.go | 4 +--- services/context/base.go | 4 +--- services/context/context_response.go | 3 ++- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/routers/api/packages/maven/maven.go b/routers/api/packages/maven/maven.go index 7571797593..58271e1d43 100644 --- a/routers/api/packages/maven/maven.go +++ b/routers/api/packages/maven/maven.go @@ -143,9 +143,7 @@ func serveMavenMetadata(ctx *context.Context, params parameters) { ctx.Resp.Header().Set("Content-Length", strconv.Itoa(len(xmlMetadataWithHeader))) ctx.Resp.Header().Set("Content-Type", contentTypeXML) - if _, err := ctx.Resp.Write(xmlMetadataWithHeader); err != nil { - log.Error("write bytes failed: %v", err) - } + _, _ = ctx.Resp.Write(xmlMetadataWithHeader) } func servePackageFile(ctx *context.Context, params parameters, serveContent bool) { diff --git a/services/context/base.go b/services/context/base.go index 25ff935055..0259e0d806 100644 --- a/services/context/base.go +++ b/services/context/base.go @@ -234,9 +234,7 @@ func (b *Base) plainTextInternal(skip, status int, bs []byte) { b.Resp.Header().Set("Content-Type", "text/plain;charset=utf-8") b.Resp.Header().Set("X-Content-Type-Options", "nosniff") b.Resp.WriteHeader(status) - if _, err := b.Resp.Write(bs); err != nil { - log.ErrorWithSkip(skip, "plainTextInternal (status=%d): write bytes failed: %v", status, err) - } + _, _ = b.Resp.Write(bs) } // PlainTextBytes renders bytes as plain text diff --git a/services/context/context_response.go b/services/context/context_response.go index 372b4cb38b..4b047c1a70 100644 --- a/services/context/context_response.go +++ b/services/context/context_response.go @@ -13,6 +13,7 @@ import ( "path" "strconv" "strings" + "syscall" "time" user_model "code.gitea.io/gitea/models/user" @@ -77,7 +78,7 @@ func (ctx *Context) HTML(status int, name base.TplName) { } err := ctx.Render.HTML(ctx.Resp, status, string(name), ctx.Data, ctx.TemplateContext) - if err == nil { + if err == nil || errors.Is(err, syscall.EPIPE) { return } From 6ae15bc15ef7775cbaeda23f136aa06a3793f479 Mon Sep 17 00:00:00 2001 From: Kemal Zebari <60799661+kemzeb@users.noreply.github.com> Date: Fri, 3 May 2024 00:58:31 -0700 Subject: [PATCH 6/7] Don't only list code-enabled repositories when using repository API (#30817) We should be listing all repositories by default. Fixes #28483. (cherry picked from commit 9f0ef3621a3b63ccbe93f302a446b67dc54ad725) Conflict: - if ctx.IsSigned && ctx.Doer.IsAdmin || permission.UnitAccessMode(unit_model.TypeCode) >= perm.AccessModeRead { + if ctx.IsSigned && ctx.Doer.IsAdmin || permission.HasAccess() { because of https://codeberg.org/forgejo/forgejo/pulls/2001 (cherry picked from commit e388822e9dc6bfe59df06c7b2008cba517f908fc) --- routers/api/v1/user/repo.go | 4 +--- tests/integration/api_repo_test.go | 34 ++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/routers/api/v1/user/repo.go b/routers/api/v1/user/repo.go index 81f8e0f3fe..9b6701b067 100644 --- a/routers/api/v1/user/repo.go +++ b/routers/api/v1/user/repo.go @@ -6,10 +6,8 @@ package user import ( "net/http" - "code.gitea.io/gitea/models/perm" access_model "code.gitea.io/gitea/models/perm/access" repo_model "code.gitea.io/gitea/models/repo" - unit_model "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/routers/api/v1/utils" @@ -44,7 +42,7 @@ func listUserRepos(ctx *context.APIContext, u *user_model.User, private bool) { ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err) return } - if ctx.IsSigned && ctx.Doer.IsAdmin || permission.UnitAccessMode(unit_model.TypeCode) >= perm.AccessModeRead { + if ctx.IsSigned && ctx.Doer.IsAdmin || permission.HasAccess() { apiRepos = append(apiRepos, convert.ToRepo(ctx, repos[i], permission)) } } diff --git a/tests/integration/api_repo_test.go b/tests/integration/api_repo_test.go index 96f38edc86..a503b201bc 100644 --- a/tests/integration/api_repo_test.go +++ b/tests/integration/api_repo_test.go @@ -13,6 +13,7 @@ import ( "code.gitea.io/gitea/models/db" access_model "code.gitea.io/gitea/models/perm/access" repo_model "code.gitea.io/gitea/models/repo" + unit_model "code.gitea.io/gitea/models/unit" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/setting" @@ -326,6 +327,39 @@ func TestAPIOrgRepos(t *testing.T) { } } +// See issue #28483. Tests to make sure we consider more than just code unit-enabled repositories. +func TestAPIOrgReposWithCodeUnitDisabled(t *testing.T) { + defer tests.PrepareTestEnv(t)() + repo21 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{Name: "repo21"}) + org3 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo21.OwnerID}) + + // Disable code repository unit. + var units []unit_model.Type + units = append(units, unit_model.TypeCode) + + if err := repo_service.UpdateRepositoryUnits(db.DefaultContext, repo21, nil, units); err != nil { + assert.Fail(t, "should have been able to delete code repository unit; failed to %v", err) + } + assert.False(t, repo21.UnitEnabled(db.DefaultContext, unit_model.TypeCode)) + + session := loginUser(t, "user2") + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadOrganization) + + req := NewRequestf(t, "GET", "/api/v1/orgs/%s/repos", org3.Name). + AddTokenAuth(token) + + resp := MakeRequest(t, req, http.StatusOK) + var apiRepos []*api.Repository + DecodeJSON(t, resp, &apiRepos) + + var repoNames []string + for _, r := range apiRepos { + repoNames = append(repoNames, r.Name) + } + + assert.Contains(t, repoNames, repo21.Name) +} + func TestAPIGetRepoByIDUnauthorized(t *testing.T) { defer tests.PrepareTestEnv(t)() user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) From da993b09adf1e1bd5018155f470c4fb4f06a2f9a Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Fri, 3 May 2024 15:11:51 +0900 Subject: [PATCH 7/7] Fix no edit history after editing issue's title and content (#30814) Fix #30807 reuse functions in services (cherry picked from commit a50026e2f30897904704895362da0fb12c7e5b26) Conflicts: models/issues/issue_update.go routers/api/v1/repo/issue.go trivial context conflict because of 'allow setting the update date on issues and comments' (cherry picked from commit 6a4bc0289db5d5d791864f45ed9bb47b6bc8d2fe) --- models/issues/issue_update.go | 59 ----------------------------- modules/structs/pull.go | 2 +- routers/api/v1/repo/issue.go | 36 ++++++++---------- routers/api/v1/repo/pull.go | 37 +++++++++--------- tests/integration/api_issue_test.go | 4 ++ tests/integration/api_pull_test.go | 26 +++++++++---- 6 files changed, 56 insertions(+), 108 deletions(-) diff --git a/models/issues/issue_update.go b/models/issues/issue_update.go index 78e1f8e030..c3debac92e 100644 --- a/models/issues/issue_update.go +++ b/models/issues/issue_update.go @@ -450,65 +450,6 @@ func UpdateIssueMentions(ctx context.Context, issueID int64, mentions []*user_mo return nil } -// UpdateIssueByAPI updates all allowed fields of given issue. -// If the issue status is changed a statusChangeComment is returned -// similarly if the title is changed the titleChanged bool is set to true -func UpdateIssueByAPI(ctx context.Context, issue *Issue, doer *user_model.User) (statusChangeComment *Comment, titleChanged bool, err error) { - ctx, committer, err := db.TxContext(ctx) - if err != nil { - return nil, false, err - } - defer committer.Close() - - if err := issue.LoadRepo(ctx); err != nil { - return nil, false, fmt.Errorf("loadRepo: %w", err) - } - - // Reload the issue - currentIssue, err := GetIssueByID(ctx, issue.ID) - if err != nil { - return nil, false, err - } - - sess := db.GetEngine(ctx).ID(issue.ID) - cols := []string{"name", "content", "milestone_id", "priority", "deadline_unix", "is_locked"} - if issue.NoAutoTime { - cols = append(cols, "updated_unix") - sess.NoAutoTime() - } - if _, err := sess.Cols(cols...).Update(issue); err != nil { - return nil, false, err - } - - titleChanged = currentIssue.Title != issue.Title - if titleChanged { - opts := &CreateCommentOptions{ - Type: CommentTypeChangeTitle, - Doer: doer, - Repo: issue.Repo, - Issue: issue, - OldTitle: currentIssue.Title, - NewTitle: issue.Title, - } - _, err := CreateComment(ctx, opts) - if err != nil { - return nil, false, fmt.Errorf("createComment: %w", err) - } - } - - if currentIssue.IsClosed != issue.IsClosed { - statusChangeComment, err = doChangeIssueStatus(ctx, issue, doer, false) - if err != nil { - return nil, false, err - } - } - - if err := issue.AddCrossReferences(ctx, doer, true); err != nil { - return nil, false, err - } - return statusChangeComment, titleChanged, committer.Commit() -} - // UpdateIssueDeadline updates an issue deadline and adds comments. Setting a deadline to 0 means deleting it. func UpdateIssueDeadline(ctx context.Context, issue *Issue, deadlineUnix timeutil.TimeStamp, doer *user_model.User) (err error) { // if the deadline hasn't changed do nothing diff --git a/modules/structs/pull.go b/modules/structs/pull.go index 05a8d59633..b04def52b8 100644 --- a/modules/structs/pull.go +++ b/modules/structs/pull.go @@ -85,7 +85,7 @@ type CreatePullRequestOption struct { // EditPullRequestOption options when modify pull request type EditPullRequestOption struct { Title string `json:"title"` - Body string `json:"body"` + Body *string `json:"body"` Base string `json:"base"` Assignee string `json:"assignee"` Assignees []string `json:"assignees"` diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index 0d304dd66d..0e977d563a 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -29,7 +29,6 @@ import ( "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/convert" issue_service "code.gitea.io/gitea/services/issue" - notify_service "code.gitea.io/gitea/services/notify" ) // SearchIssues searches for issues across the repositories that the user has access to @@ -810,12 +809,19 @@ func EditIssue(ctx *context.APIContext) { return } - oldTitle := issue.Title if len(form.Title) > 0 { - issue.Title = form.Title + err = issue_service.ChangeTitle(ctx, issue, ctx.Doer, form.Title) + if err != nil { + ctx.Error(http.StatusInternalServerError, "ChangeTitle", err) + return + } } if form.Body != nil { - issue.Content = *form.Body + err = issue_service.ChangeContent(ctx, issue, ctx.Doer, *form.Body) + if err != nil { + ctx.Error(http.StatusInternalServerError, "ChangeContent", err) + return + } } if form.Ref != nil { err = issue_service.ChangeIssueRef(ctx, issue, ctx.Doer, *form.Ref) @@ -883,24 +889,14 @@ func EditIssue(ctx *context.APIContext) { return } } - issue.IsClosed = api.StateClosed == api.StateType(*form.State) - } - statusChangeComment, titleChanged, err := issues_model.UpdateIssueByAPI(ctx, issue, ctx.Doer) - if err != nil { - if issues_model.IsErrDependenciesLeft(err) { - ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this issue because it still has open dependencies") + if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", api.StateClosed == api.StateType(*form.State)); err != nil { + if issues_model.IsErrDependenciesLeft(err) { + ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this issue because it still has open dependencies") + return + } + ctx.Error(http.StatusInternalServerError, "ChangeStatus", err) return } - ctx.Error(http.StatusInternalServerError, "UpdateIssueByAPI", err) - return - } - - if titleChanged { - notify_service.IssueChangeTitle(ctx, ctx.Doer, issue, oldTitle) - } - - if statusChangeComment != nil { - notify_service.IssueChangeStatus(ctx, ctx.Doer, "", issue, statusChangeComment, issue.IsClosed) } // Refetch from database to assign some automatic values diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index eec3c49bc4..62d71875bf 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -601,12 +601,19 @@ func EditPullRequest(ctx *context.APIContext) { return } - oldTitle := issue.Title if len(form.Title) > 0 { - issue.Title = form.Title + err = issue_service.ChangeTitle(ctx, issue, ctx.Doer, form.Title) + if err != nil { + ctx.Error(http.StatusInternalServerError, "ChangeTitle", err) + return + } } - if len(form.Body) > 0 { - issue.Content = form.Body + if form.Body != nil { + err = issue_service.ChangeContent(ctx, issue, ctx.Doer, *form.Body) + if err != nil { + ctx.Error(http.StatusInternalServerError, "ChangeContent", err) + return + } } // Update or remove deadline if set @@ -683,24 +690,14 @@ func EditPullRequest(ctx *context.APIContext) { ctx.Error(http.StatusPreconditionFailed, "MergedPRState", "cannot change state of this pull request, it was already merged") return } - issue.IsClosed = api.StateClosed == api.StateType(*form.State) - } - statusChangeComment, titleChanged, err := issues_model.UpdateIssueByAPI(ctx, issue, ctx.Doer) - if err != nil { - if issues_model.IsErrDependenciesLeft(err) { - ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this pull request because it still has open dependencies") + if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", api.StateClosed == api.StateType(*form.State)); err != nil { + if issues_model.IsErrDependenciesLeft(err) { + ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this pull request because it still has open dependencies") + return + } + ctx.Error(http.StatusInternalServerError, "ChangeStatus", err) return } - ctx.Error(http.StatusInternalServerError, "UpdateIssueByAPI", err) - return - } - - if titleChanged { - notify_service.IssueChangeTitle(ctx, ctx.Doer, issue, oldTitle) - } - - if statusChangeComment != nil { - notify_service.IssueChangeStatus(ctx, ctx.Doer, "", issue, statusChangeComment, issue.IsClosed) } // change pull target branch diff --git a/tests/integration/api_issue_test.go b/tests/integration/api_issue_test.go index 849535ff8e..e6fb62a3a9 100644 --- a/tests/integration/api_issue_test.go +++ b/tests/integration/api_issue_test.go @@ -188,6 +188,10 @@ func TestAPIEditIssue(t *testing.T) { issueAfter := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 10}) repoAfter := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: issueBefore.RepoID}) + // check comment history + unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{IssueID: issueAfter.ID, OldTitle: issueBefore.Title, NewTitle: title}) + unittest.AssertExistsAndLoadBean(t, &issues_model.ContentHistory{IssueID: issueAfter.ID, ContentText: body, IsFirstCreated: false}) + // check deleted user assert.Equal(t, int64(500), issueAfter.PosterID) assert.NoError(t, issueAfter.LoadAttributes(db.DefaultContext)) diff --git a/tests/integration/api_pull_test.go b/tests/integration/api_pull_test.go index 21417bb77e..4f068502f7 100644 --- a/tests/integration/api_pull_test.go +++ b/tests/integration/api_pull_test.go @@ -223,23 +223,33 @@ func TestAPIEditPull(t *testing.T) { session := loginUser(t, owner10.Name) token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) + title := "create a success pr" req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &api.CreatePullRequestOption{ Head: "develop", Base: "master", - Title: "create a success pr", + Title: title, }).AddTokenAuth(token) - pull := new(api.PullRequest) + apiPull := new(api.PullRequest) resp := MakeRequest(t, req, http.StatusCreated) - DecodeJSON(t, resp, pull) - assert.EqualValues(t, "master", pull.Base.Name) + DecodeJSON(t, resp, apiPull) + assert.EqualValues(t, "master", apiPull.Base.Name) - req = NewRequestWithJSON(t, http.MethodPatch, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d", owner10.Name, repo10.Name, pull.Index), &api.EditPullRequestOption{ + newTitle := "edit a this pr" + newBody := "edited body" + req = NewRequestWithJSON(t, http.MethodPatch, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d", owner10.Name, repo10.Name, apiPull.Index), &api.EditPullRequestOption{ Base: "feature/1", - Title: "edit a this pr", + Title: newTitle, + Body: &newBody, }).AddTokenAuth(token) resp = MakeRequest(t, req, http.StatusCreated) - DecodeJSON(t, resp, pull) - assert.EqualValues(t, "feature/1", pull.Base.Name) + DecodeJSON(t, resp, apiPull) + assert.EqualValues(t, "feature/1", apiPull.Base.Name) + // check comment history + pull := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: apiPull.ID}) + err := pull.LoadIssue(db.DefaultContext) + assert.NoError(t, err) + unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{IssueID: pull.Issue.ID, OldTitle: title, NewTitle: newTitle}) + unittest.AssertExistsAndLoadBean(t, &issues_model.ContentHistory{IssueID: pull.Issue.ID, ContentText: newBody, IsFirstCreated: false}) req = NewRequestWithJSON(t, http.MethodPatch, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d", owner10.Name, repo10.Name, pull.Index), &api.EditPullRequestOption{ Base: "not-exist",