From ca0921a95aa9a37d8820538458c15fd0a3b0c97c Mon Sep 17 00:00:00 2001 From: metiftikci Date: Mon, 27 May 2024 18:34:18 +0300 Subject: [PATCH] Prevent simultaneous editing of comments and issues (#31053) fixes #22907 Tested: - [x] issue content edit - [x] issue content change tasklist - [x] pull request content edit - [x] pull request change tasklist ![issue-content-edit](https://github.com/go-gitea/gitea/assets/29250154/a0828889-fb96-4bc4-8600-da92e3205812) (cherry picked from commit aa92b13164e84c26be91153b6022220ce0a27720) Conflicts: models/issues/comment.go c7a389f2b2667b7bfc264a0be519b5c85e0a3b94 [FEAT] allow setting the update date on issues and comments options/locale/locale_en-US.ini trivial context conflicts routers/api/v1/repo/issue_comment.go routers/api/v1/repo/issue_comment_attachment.go services/issue/comments.go services/issue/content.go user blocking is implemented differently in Forgejo routers/web/repo/issue.go trivial difference from 6a0750177f Allow to save empty comment user blocking is implemented differently in Forgejo templates/repo/issue/view_content/conversation.tmpl templates changed a lot in Forgejo but the change is trivially ported tests/integration/issue_test.go other tests were added in the same region web_src/js/features/repo-issue-edit.js the code is still web_src/js/features/repo-legacy.js trivially ported --- models/issues/comment.go | 13 +++- models/issues/issue.go | 3 + models/issues/issue_update.go | 21 ++++-- models/migrations/migrations.go | 4 ++ models/migrations/v1_23/v299.go | 18 +++++ options/locale/locale_en-US.ini | 4 ++ routers/api/v1/repo/issue.go | 7 +- routers/api/v1/repo/issue_attachment.go | 2 +- routers/api/v1/repo/issue_comment.go | 2 +- .../api/v1/repo/issue_comment_attachment.go | 2 +- routers/api/v1/repo/pull.go | 7 +- routers/web/repo/issue.go | 35 ++++++--- services/issue/comments.go | 4 +- services/issue/content.go | 4 +- templates/repo/diff/comments.tmpl | 2 +- templates/repo/issue/view_content.tmpl | 2 +- .../repo/issue/view_content/comments.tmpl | 4 +- .../repo/issue/view_content/conversation.tmpl | 2 +- tests/integration/issue_test.go | 71 ++++++++++++++++++- web_src/js/features/repo-legacy.js | 7 ++ web_src/js/markup/tasklist.js | 12 +++- 21 files changed, 190 insertions(+), 36 deletions(-) create mode 100644 models/migrations/v1_23/v299.go diff --git a/models/issues/comment.go b/models/issues/comment.go index b0f7c47a73..d53e5f5949 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -52,6 +52,8 @@ func (err ErrCommentNotExist) Unwrap() error { return util.ErrNotExist } +var ErrCommentAlreadyChanged = util.NewInvalidArgumentErrorf("the comment is already changed") + // CommentType defines whether a comment is just a simple comment, an action (like close) or a reference. type CommentType int @@ -262,6 +264,7 @@ type Comment struct { Line int64 // - previous line / + proposed line TreePath string Content string `xorm:"LONGTEXT"` + ContentVersion int `xorm:"NOT NULL DEFAULT 0"` RenderedContent template.HTML `xorm:"-"` // Path represents the 4 lines of code cemented by this comment @@ -1119,7 +1122,7 @@ func UpdateCommentInvalidate(ctx context.Context, c *Comment) error { } // UpdateComment updates information of comment. -func UpdateComment(ctx context.Context, c *Comment, doer *user_model.User) error { +func UpdateComment(ctx context.Context, c *Comment, contentVersion int, doer *user_model.User) error { ctx, committer, err := db.TxContext(ctx) if err != nil { return err @@ -1139,9 +1142,15 @@ func UpdateComment(ctx context.Context, c *Comment, doer *user_model.User) error // see https://codeberg.org/forgejo/forgejo/pulls/764#issuecomment-1023801 c.UpdatedUnix = c.Issue.UpdatedUnix } - if _, err := sess.Update(c); err != nil { + c.ContentVersion = contentVersion + 1 + + affected, err := sess.Where("content_version = ?", contentVersion).Update(c) + if err != nil { return err } + if affected == 0 { + return ErrCommentAlreadyChanged + } if err := c.AddCrossReferences(ctx, doer, true); err != nil { return err } diff --git a/models/issues/issue.go b/models/issues/issue.go index affd581929..349b7e3d05 100644 --- a/models/issues/issue.go +++ b/models/issues/issue.go @@ -94,6 +94,8 @@ func (err ErrIssueWasClosed) Error() string { return fmt.Sprintf("Issue [%d] %d was already closed", err.ID, err.Index) } +var ErrIssueAlreadyChanged = util.NewInvalidArgumentErrorf("the issue is already changed") + // Issue represents an issue or pull request of repository. type Issue struct { ID int64 `xorm:"pk autoincr"` @@ -107,6 +109,7 @@ type Issue struct { Title string `xorm:"name"` Content string `xorm:"LONGTEXT"` RenderedContent template.HTML `xorm:"-"` + ContentVersion int `xorm:"NOT NULL DEFAULT 0"` Labels []*Label `xorm:"-"` MilestoneID int64 `xorm:"INDEX"` Milestone *Milestone `xorm:"-"` diff --git a/models/issues/issue_update.go b/models/issues/issue_update.go index c3debac92e..dbfd2fc91b 100644 --- a/models/issues/issue_update.go +++ b/models/issues/issue_update.go @@ -25,17 +25,18 @@ import ( "xorm.io/builder" ) -// UpdateIssueCols updates cols of issue func UpdateIssueCols(ctx context.Context, issue *Issue, cols ...string) error { + _, err := UpdateIssueColsWithCond(ctx, issue, builder.NewCond(), cols...) + return err +} + +func UpdateIssueColsWithCond(ctx context.Context, issue *Issue, cond builder.Cond, cols ...string) (int64, error) { sess := db.GetEngine(ctx).ID(issue.ID) if issue.NoAutoTime { cols = append(cols, []string{"updated_unix"}...) sess.NoAutoTime() } - if _, err := sess.Cols(cols...).Update(issue); err != nil { - return err - } - return nil + return sess.Cols(cols...).Where(cond).Update(issue) } func changeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.User, isClosed, isMergePull bool) (*Comment, error) { @@ -250,7 +251,7 @@ func UpdateIssueAttachments(ctx context.Context, issueID int64, uuids []string) } // ChangeIssueContent changes issue content, as the given user. -func ChangeIssueContent(ctx context.Context, issue *Issue, doer *user_model.User, content string) (err error) { +func ChangeIssueContent(ctx context.Context, issue *Issue, doer *user_model.User, content string, contentVersion int) (err error) { ctx, committer, err := db.TxContext(ctx) if err != nil { return err @@ -269,10 +270,16 @@ func ChangeIssueContent(ctx context.Context, issue *Issue, doer *user_model.User } issue.Content = content + issue.ContentVersion = contentVersion + 1 - if err = UpdateIssueCols(ctx, issue, "content"); err != nil { + expectedContentVersion := builder.NewCond().And(builder.Eq{"content_version": contentVersion}) + affected, err := UpdateIssueColsWithCond(ctx, issue, expectedContentVersion, "content", "content_version") + if err != nil { return fmt.Errorf("UpdateIssueCols: %w", err) } + if affected == 0 { + return ErrIssueAlreadyChanged + } historyDate := timeutil.TimeStampNow() if issue.NoAutoTime { diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 5d80f9fd10..2e095c05a4 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -22,6 +22,7 @@ import ( "code.gitea.io/gitea/models/migrations/v1_20" "code.gitea.io/gitea/models/migrations/v1_21" "code.gitea.io/gitea/models/migrations/v1_22" + "code.gitea.io/gitea/models/migrations/v1_23" "code.gitea.io/gitea/models/migrations/v1_6" "code.gitea.io/gitea/models/migrations/v1_7" "code.gitea.io/gitea/models/migrations/v1_8" @@ -589,6 +590,9 @@ var migrations = []Migration{ NewMigration("Drop wrongly created table o_auth2_application", v1_22.DropWronglyCreatedTable), // Gitea 1.22.0-rc1 ends at 299 + + // v299 -> v300 + NewMigration("Add content version to issue and comment table", v1_23.AddContentVersionToIssueAndComment), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v1_23/v299.go b/models/migrations/v1_23/v299.go new file mode 100644 index 0000000000..f6db960c3b --- /dev/null +++ b/models/migrations/v1_23/v299.go @@ -0,0 +1,18 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package v1_23 //nolint + +import "xorm.io/xorm" + +func AddContentVersionToIssueAndComment(x *xorm.Engine) error { + type Issue struct { + ContentVersion int `xorm:"NOT NULL DEFAULT 0"` + } + + type Comment struct { + ContentVersion int `xorm:"NOT NULL DEFAULT 0"` + } + + return x.Sync(new(Comment), new(Issue)) +} diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 1232ad8d5f..96dccccf3f 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1474,6 +1474,7 @@ issues.new.assignees = Assignees issues.new.clear_assignees = Clear assignees issues.new.no_assignees = No assignees issues.new.no_reviewers = No reviewers +issues.edit.already_changed = Unable to save changes to the issue. It appears the content has already been changed by another user. Please refresh the page and try editing again to avoid overwriting their changes issues.choose.get_started = Get started issues.choose.open_external_link = Open issues.choose.blank = Default @@ -1791,6 +1792,7 @@ compare.compare_head = compare pulls.desc = Enable pull requests and code reviews. pulls.new = New pull request pulls.view = View pull request +pulls.edit.already_changed = Unable to save changes to the pull request. It appears the content has already been changed by another user. Please refresh the page and try editing again to avoid overwriting their changes pulls.compare_changes = New pull request pulls.allow_edits_from_maintainers = Allow edits from maintainers pulls.allow_edits_from_maintainers_desc = Users with write access to the base branch can also push to this branch @@ -1946,6 +1948,8 @@ pulls.recently_pushed_new_branches = You pushed on branch
{{.Content}}
-
+
{{if .Attachments}} {{template "repo/issue/view_content/attachments" dict "Attachments" .Attachments "RenderedContent" .RenderedContent}} {{end}} diff --git a/templates/repo/issue/view_content.tmpl b/templates/repo/issue/view_content.tmpl index bb8863a170..9db480bca2 100644 --- a/templates/repo/issue/view_content.tmpl +++ b/templates/repo/issue/view_content.tmpl @@ -60,7 +60,7 @@ {{end}}
{{.Issue.Content}}
-
+
{{if .Issue.Attachments}} {{template "repo/issue/view_content/attachments" dict "Attachments" .Issue.Attachments "RenderedContent" .Issue.RenderedContent}} {{end}} diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl index e9e92db07d..5aeb0ff94f 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -67,7 +67,7 @@ {{end}}
{{.Content}}
-
+
{{if .Attachments}} {{template "repo/issue/view_content/attachments" dict "Attachments" .Attachments "RenderedContent" .RenderedContent}} {{end}} @@ -441,7 +441,7 @@ {{end}}
{{.Content}}
-
+
{{if .Attachments}} {{template "repo/issue/view_content/attachments" dict "Attachments" .Attachments "RenderedContent" .RenderedContent}} {{end}} diff --git a/templates/repo/issue/view_content/conversation.tmpl b/templates/repo/issue/view_content/conversation.tmpl index a28956937b..6ba9e73d3d 100644 --- a/templates/repo/issue/view_content/conversation.tmpl +++ b/templates/repo/issue/view_content/conversation.tmpl @@ -93,7 +93,7 @@ {{end}}
{{.Content}}
-
+
{{if .Attachments}} {{template "repo/issue/view_content/attachments" dict "Attachments" .Attachments "RenderedContent" .RenderedContent}} {{end}} diff --git a/tests/integration/issue_test.go b/tests/integration/issue_test.go index 83d353c935..d5bdea2a19 100644 --- a/tests/integration/issue_test.go +++ b/tests/integration/issue_test.go @@ -282,6 +282,34 @@ func TestIssueDependencies(t *testing.T) { }) } +func TestEditIssue(t *testing.T) { + defer tests.PrepareTestEnv(t)() + session := loginUser(t, "user2") + issueURL := testNewIssue(t, session, "user2", "repo1", "Title", "Description") + + req := NewRequestWithValues(t, "POST", fmt.Sprintf("%s/content", issueURL), map[string]string{ + "_csrf": GetCSRF(t, session, issueURL), + "content": "modified content", + "context": fmt.Sprintf("/%s/%s", "user2", "repo1"), + }) + session.MakeRequest(t, req, http.StatusOK) + + req = NewRequestWithValues(t, "POST", fmt.Sprintf("%s/content", issueURL), map[string]string{ + "_csrf": GetCSRF(t, session, issueURL), + "content": "modified content", + "context": fmt.Sprintf("/%s/%s", "user2", "repo1"), + }) + session.MakeRequest(t, req, http.StatusBadRequest) + + req = NewRequestWithValues(t, "POST", fmt.Sprintf("%s/content", issueURL), map[string]string{ + "_csrf": GetCSRF(t, session, issueURL), + "content": "modified content", + "content_version": "1", + "context": fmt.Sprintf("/%s/%s", "user2", "repo1"), + }) + session.MakeRequest(t, req, http.StatusOK) +} + func TestIssueCommentClose(t *testing.T) { defer tests.PrepareTestEnv(t)() session := loginUser(t, "user2") @@ -399,8 +427,9 @@ func TestIssueCommentUpdate(t *testing.T) { // make the comment empty req = NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/comments/%d", "user2", "repo1", commentID), map[string]string{ - "_csrf": GetCSRF(t, session, issueURL), - "content": "", + "_csrf": GetCSRF(t, session, issueURL), + "content": "", + "content_version": fmt.Sprintf("%d", comment.ContentVersion), }) session.MakeRequest(t, req, http.StatusOK) @@ -408,6 +437,44 @@ func TestIssueCommentUpdate(t *testing.T) { assert.Equal(t, "", comment.Content) } +func TestIssueCommentUpdateSimultaneously(t *testing.T) { + defer tests.PrepareTestEnv(t)() + session := loginUser(t, "user2") + issueURL := testNewIssue(t, session, "user2", "repo1", "Title", "Description") + comment1 := "Test comment 1" + commentID := testIssueAddComment(t, session, issueURL, comment1, "") + + comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: commentID}) + assert.Equal(t, comment1, comment.Content) + + modifiedContent := comment.Content + "MODIFIED" + + req := NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/comments/%d", "user2", "repo1", commentID), map[string]string{ + "_csrf": GetCSRF(t, session, issueURL), + "content": modifiedContent, + }) + session.MakeRequest(t, req, http.StatusOK) + + modifiedContent = comment.Content + "2" + + req = NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/comments/%d", "user2", "repo1", commentID), map[string]string{ + "_csrf": GetCSRF(t, session, issueURL), + "content": modifiedContent, + }) + session.MakeRequest(t, req, http.StatusBadRequest) + + req = NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/comments/%d", "user2", "repo1", commentID), map[string]string{ + "_csrf": GetCSRF(t, session, issueURL), + "content": modifiedContent, + "content_version": "1", + }) + session.MakeRequest(t, req, http.StatusOK) + + comment = unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: commentID}) + assert.Equal(t, modifiedContent, comment.Content) + assert.Equal(t, 2, comment.ContentVersion) +} + func TestIssueReaction(t *testing.T) { defer tests.PrepareTestEnv(t)() session := loginUser(t, "user2") diff --git a/web_src/js/features/repo-legacy.js b/web_src/js/features/repo-legacy.js index f172000842..a14241b5a6 100644 --- a/web_src/js/features/repo-legacy.js +++ b/web_src/js/features/repo-legacy.js @@ -16,6 +16,7 @@ import {initCitationFileCopyContent} from './citation.js'; import {initCompLabelEdit} from './comp/LabelEdit.js'; import {initRepoDiffConversationNav} from './repo-diff.js'; import {createDropzone} from './dropzone.js'; +import {showErrorToast} from '../modules/toast.js'; import {initCommentContent, initMarkupContent} from '../markup/content.js'; import {initCompReactionSelector} from './comp/ReactionSelector.js'; import {initRepoSettingBranches} from './repo-settings.js'; @@ -431,11 +432,17 @@ async function onEditContent(event) { const params = new URLSearchParams({ content: comboMarkdownEditor.value(), context: editContentZone.getAttribute('data-context'), + content_version: editContentZone.getAttribute('data-content-version'), }); for (const fileInput of dropzoneInst?.element.querySelectorAll('.files [name=files]')) params.append('files[]', fileInput.value); const response = await POST(editContentZone.getAttribute('data-update-url'), {data: params}); const data = await response.json(); + if (response.status === 400) { + showErrorToast(data.errorMessage); + return; + } + editContentZone.setAttribute('data-content-version', data.contentVersion); if (!data.content) { renderContent.innerHTML = document.getElementById('no-content').innerHTML; rawContent.textContent = ''; diff --git a/web_src/js/markup/tasklist.js b/web_src/js/markup/tasklist.js index 00076bce58..a40b5e4abd 100644 --- a/web_src/js/markup/tasklist.js +++ b/web_src/js/markup/tasklist.js @@ -1,4 +1,5 @@ import {POST} from '../modules/fetch.js'; +import {showErrorToast} from '../modules/toast.js'; const preventListener = (e) => e.preventDefault(); @@ -54,13 +55,20 @@ export function initMarkupTasklist() { const editContentZone = container.querySelector('.edit-content-zone'); const updateUrl = editContentZone.getAttribute('data-update-url'); const context = editContentZone.getAttribute('data-context'); + const contentVersion = editContentZone.getAttribute('data-content-version'); const requestBody = new FormData(); requestBody.append('ignore_attachments', 'true'); requestBody.append('content', newContent); requestBody.append('context', context); - await POST(updateUrl, {data: requestBody}); - + requestBody.append('content_version', contentVersion); + const response = await POST(updateUrl, {data: requestBody}); + const data = await response.json(); + if (response.status === 400) { + showErrorToast(data.errorMessage); + return; + } + editContentZone.setAttribute('data-content-version', data.contentVersion); rawContent.textContent = newContent; } catch (err) { checkbox.checked = !checkbox.checked;