Skip to content

Commit aa92b13

Browse files
metiftikcimetiftikci
authored
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)
1 parent 1ed8e6a commit aa92b13

File tree

21 files changed

+172
-27
lines changed

21 files changed

+172
-27
lines changed

‎models/issues/comment.go

+11-2
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,8 @@ func (err ErrCommentNotExist) Unwrap() error {
5252
return util.ErrNotExist
5353
}
5454

55+
var ErrCommentAlreadyChanged = util.NewInvalidArgumentErrorf("the comment is already changed")
56+
5557
// CommentType defines whether a comment is just a simple comment, an action (like close) or a reference.
5658
type CommentType int
5759

@@ -262,6 +264,7 @@ type Comment struct {
262264
Line int64 // - previous line / + proposed line
263265
TreePath string
264266
Content string `xorm:"LONGTEXT"`
267+
ContentVersion int `xorm:"NOT NULL DEFAULT 0"`
265268
RenderedContent template.HTML `xorm:"-"`
266269

267270
// Path represents the 4 lines of code cemented by this comment
@@ -1111,17 +1114,23 @@ func UpdateCommentInvalidate(ctx context.Context, c *Comment) error {
11111114
}
11121115

11131116
// UpdateComment updates information of comment.
1114-
func UpdateComment(ctx context.Context, c *Comment, doer *user_model.User) error {
1117+
func UpdateComment(ctx context.Context, c *Comment, contentVersion int, doer *user_model.User) error {
11151118
ctx, committer, err := db.TxContext(ctx)
11161119
if err != nil {
11171120
return err
11181121
}
11191122
defer committer.Close()
11201123
sess := db.GetEngine(ctx)
11211124

1122-
if _, err := sess.ID(c.ID).AllCols().Update(c); err != nil {
1125+
c.ContentVersion = contentVersion + 1
1126+
1127+
affected, err := sess.ID(c.ID).AllCols().Where("content_version = ?", contentVersion).Update(c)
1128+
if err != nil {
11231129
return err
11241130
}
1131+
if affected == 0 {
1132+
return ErrCommentAlreadyChanged
1133+
}
11251134
if err := c.LoadIssue(ctx); err != nil {
11261135
return err
11271136
}

‎models/issues/issue.go

+3
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,8 @@ func (err ErrIssueWasClosed) Error() string {
9494
return fmt.Sprintf("Issue [%d] %d was already closed", err.ID, err.Index)
9595
}
9696

97+
var ErrIssueAlreadyChanged = util.NewInvalidArgumentErrorf("the issue is already changed")
98+
9799
// Issue represents an issue or pull request of repository.
98100
type Issue struct {
99101
ID int64 `xorm:"pk autoincr"`
@@ -107,6 +109,7 @@ type Issue struct {
107109
Title string `xorm:"name"`
108110
Content string `xorm:"LONGTEXT"`
109111
RenderedContent template.HTML `xorm:"-"`
112+
ContentVersion int `xorm:"NOT NULL DEFAULT 0"`
110113
Labels []*Label `xorm:"-"`
111114
MilestoneID int64 `xorm:"INDEX"`
112115
Milestone *Milestone `xorm:"-"`

‎models/issues/issue_update.go

+8-3
Original file line numberDiff line numberDiff line change
@@ -235,7 +235,7 @@ func UpdateIssueAttachments(ctx context.Context, issueID int64, uuids []string)
235235
}
236236

237237
// ChangeIssueContent changes issue content, as the given user.
238-
func ChangeIssueContent(ctx context.Context, issue *Issue, doer *user_model.User, content string) (err error) {
238+
func ChangeIssueContent(ctx context.Context, issue *Issue, doer *user_model.User, content string, contentVersion int) (err error) {
239239
ctx, committer, err := db.TxContext(ctx)
240240
if err != nil {
241241
return err
@@ -254,9 +254,14 @@ func ChangeIssueContent(ctx context.Context, issue *Issue, doer *user_model.User
254254
}
255255

256256
issue.Content = content
257+
issue.ContentVersion = contentVersion + 1
257258

258-
if err = UpdateIssueCols(ctx, issue, "content"); err != nil {
259-
return fmt.Errorf("UpdateIssueCols: %w", err)
259+
affected, err := db.GetEngine(ctx).ID(issue.ID).Cols("content", "content_version").Where("content_version = ?", contentVersion).Update(issue)
260+
if err != nil {
261+
return err
262+
}
263+
if affected == 0 {
264+
return ErrIssueAlreadyChanged
260265
}
261266

262267
if err = SaveIssueContentHistory(ctx, doer.ID, issue.ID, 0,

‎models/migrations/migrations.go

+4
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"code.gitea.io/gitea/models/migrations/v1_20"
2222
"code.gitea.io/gitea/models/migrations/v1_21"
2323
"code.gitea.io/gitea/models/migrations/v1_22"
24+
"code.gitea.io/gitea/models/migrations/v1_23"
2425
"code.gitea.io/gitea/models/migrations/v1_6"
2526
"code.gitea.io/gitea/models/migrations/v1_7"
2627
"code.gitea.io/gitea/models/migrations/v1_8"
@@ -587,6 +588,9 @@ var migrations = []Migration{
587588
NewMigration("Drop wrongly created table o_auth2_application", v1_22.DropWronglyCreatedTable),
588589

589590
// Gitea 1.22.0-rc1 ends at 299
591+
592+
// v299 -> v300
593+
NewMigration("Add content version to issue and comment table", v1_23.AddContentVersionToIssueAndComment),
590594
}
591595

592596
// GetCurrentDBVersion returns the current db version

‎models/migrations/v1_23/v299.go

+18
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
// Copyright 2024 The Gitea Authors. All rights reserved.
2+
// SPDX-License-Identifier: MIT
3+
4+
package v1_23 //nolint
5+
6+
import "xorm.io/xorm"
7+
8+
func AddContentVersionToIssueAndComment(x *xorm.Engine) error {
9+
type Issue struct {
10+
ContentVersion int `xorm:"NOT NULL DEFAULT 0"`
11+
}
12+
13+
type Comment struct {
14+
ContentVersion int `xorm:"NOT NULL DEFAULT 0"`
15+
}
16+
17+
return x.Sync(new(Comment), new(Issue))
18+
}

‎options/locale/locale_en-US.ini

+4
Original file line numberDiff line numberDiff line change
@@ -1443,6 +1443,7 @@ issues.new.clear_assignees = Clear assignees
14431443
issues.new.no_assignees = No Assignees
14441444
issues.new.no_reviewers = No reviewers
14451445
issues.new.blocked_user = Cannot create issue because you are blocked by the repository owner.
1446+
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
14461447
issues.edit.blocked_user = Cannot edit content because you are blocked by the poster or repository owner.
14471448
issues.choose.get_started = Get Started
14481449
issues.choose.open_external_link = Open
@@ -1758,6 +1759,7 @@ compare.compare_head = compare
17581759
pulls.desc = Enable pull requests and code reviews.
17591760
pulls.new = New Pull Request
17601761
pulls.new.blocked_user = Cannot create pull request because you are blocked by the repository owner.
1762+
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
17611763
pulls.view = View Pull Request
17621764
pulls.compare_changes = New Pull Request
17631765
pulls.allow_edits_from_maintainers = Allow edits from maintainers
@@ -1903,6 +1905,8 @@ pulls.recently_pushed_new_branches = You pushed on branch <strong>%[1]s</strong>
19031905

19041906
pull.deleted_branch = (deleted):%s
19051907

1908+
comments.edit.already_changed = Unable to save changes to the comment. It appears the content has already been changed by another user. Please refresh the page and try editing again to avoid overwriting their changes
1909+
19061910
milestones.new = New Milestone
19071911
milestones.closed = Closed %s
19081912
milestones.update_ago = Updated %s

‎routers/api/v1/repo/issue.go

+6-1
Original file line numberDiff line numberDiff line change
@@ -810,8 +810,13 @@ func EditIssue(ctx *context.APIContext) {
810810
}
811811
}
812812
if form.Body != nil {
813-
err = issue_service.ChangeContent(ctx, issue, ctx.Doer, *form.Body)
813+
err = issue_service.ChangeContent(ctx, issue, ctx.Doer, *form.Body, issue.ContentVersion)
814814
if err != nil {
815+
if errors.Is(err, issues_model.ErrIssueAlreadyChanged) {
816+
ctx.Error(http.StatusBadRequest, "ChangeContent", err)
817+
return
818+
}
819+
815820
ctx.Error(http.StatusInternalServerError, "ChangeContent", err)
816821
return
817822
}

‎routers/api/v1/repo/issue_attachment.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,7 @@ func CreateIssueAttachment(ctx *context.APIContext) {
198198

199199
issue.Attachments = append(issue.Attachments, attachment)
200200

201-
if err := issue_service.ChangeContent(ctx, issue, ctx.Doer, issue.Content); err != nil {
201+
if err := issue_service.ChangeContent(ctx, issue, ctx.Doer, issue.Content, issue.ContentVersion); err != nil {
202202
ctx.Error(http.StatusInternalServerError, "ChangeContent", err)
203203
return
204204
}

‎routers/api/v1/repo/issue_comment.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -611,7 +611,7 @@ func editIssueComment(ctx *context.APIContext, form api.EditIssueCommentOption)
611611

612612
oldContent := comment.Content
613613
comment.Content = form.Body
614-
if err := issue_service.UpdateComment(ctx, comment, ctx.Doer, oldContent); err != nil {
614+
if err := issue_service.UpdateComment(ctx, comment, comment.ContentVersion, ctx.Doer, oldContent); err != nil {
615615
if errors.Is(err, user_model.ErrBlockedUser) {
616616
ctx.Error(http.StatusForbidden, "UpdateComment", err)
617617
} else {

‎routers/api/v1/repo/issue_comment_attachment.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,7 @@ func CreateIssueCommentAttachment(ctx *context.APIContext) {
210210
return
211211
}
212212

213-
if err = issue_service.UpdateComment(ctx, comment, ctx.Doer, comment.Content); err != nil {
213+
if err = issue_service.UpdateComment(ctx, comment, comment.ContentVersion, ctx.Doer, comment.Content); err != nil {
214214
if errors.Is(err, user_model.ErrBlockedUser) {
215215
ctx.Error(http.StatusForbidden, "UpdateComment", err)
216216
} else {

‎routers/api/v1/repo/pull.go

+6-1
Original file line numberDiff line numberDiff line change
@@ -610,8 +610,13 @@ func EditPullRequest(ctx *context.APIContext) {
610610
}
611611
}
612612
if form.Body != nil {
613-
err = issue_service.ChangeContent(ctx, issue, ctx.Doer, *form.Body)
613+
err = issue_service.ChangeContent(ctx, issue, ctx.Doer, *form.Body, issue.ContentVersion)
614614
if err != nil {
615+
if errors.Is(err, issues_model.ErrIssueAlreadyChanged) {
616+
ctx.Error(http.StatusBadRequest, "ChangeContent", err)
617+
return
618+
}
619+
615620
ctx.Error(http.StatusInternalServerError, "ChangeContent", err)
616621
return
617622
}

‎routers/web/repo/issue.go

+17-6
Original file line numberDiff line numberDiff line change
@@ -2247,9 +2247,15 @@ func UpdateIssueContent(ctx *context.Context) {
22472247
return
22482248
}
22492249

2250-
if err := issue_service.ChangeContent(ctx, issue, ctx.Doer, ctx.Req.FormValue("content")); err != nil {
2250+
if err := issue_service.ChangeContent(ctx, issue, ctx.Doer, ctx.Req.FormValue("content"), ctx.FormInt("content_version")); err != nil {
22512251
if errors.Is(err, user_model.ErrBlockedUser) {
22522252
ctx.JSONError(ctx.Tr("repo.issues.edit.blocked_user"))
2253+
} else if errors.Is(err, issues_model.ErrIssueAlreadyChanged) {
2254+
if issue.IsPull {
2255+
ctx.JSONError(ctx.Tr("repo.pulls.edit.already_changed"))
2256+
} else {
2257+
ctx.JSONError(ctx.Tr("repo.issues.edit.already_changed"))
2258+
}
22532259
} else {
22542260
ctx.ServerError("ChangeContent", err)
22552261
}
@@ -2278,8 +2284,9 @@ func UpdateIssueContent(ctx *context.Context) {
22782284
}
22792285

22802286
ctx.JSON(http.StatusOK, map[string]any{
2281-
"content": content,
2282-
"attachments": attachmentsHTML(ctx, issue.Attachments, issue.Content),
2287+
"content": content,
2288+
"contentVersion": issue.ContentVersion,
2289+
"attachments": attachmentsHTML(ctx, issue.Attachments, issue.Content),
22832290
})
22842291
}
22852292

@@ -3153,12 +3160,15 @@ func UpdateCommentContent(ctx *context.Context) {
31533160

31543161
oldContent := comment.Content
31553162
newContent := ctx.FormString("content")
3163+
contentVersion := ctx.FormInt("content_version")
31563164

31573165
// allow to save empty content
31583166
comment.Content = newContent
3159-
if err = issue_service.UpdateComment(ctx, comment, ctx.Doer, oldContent); err != nil {
3167+
if err = issue_service.UpdateComment(ctx, comment, contentVersion, ctx.Doer, oldContent); err != nil {
31603168
if errors.Is(err, user_model.ErrBlockedUser) {
31613169
ctx.JSONError(ctx.Tr("repo.issues.comment.blocked_user"))
3170+
} else if errors.Is(err, issues_model.ErrCommentAlreadyChanged) {
3171+
ctx.JSONError(ctx.Tr("repo.comments.edit.already_changed"))
31623172
} else {
31633173
ctx.ServerError("UpdateComment", err)
31643174
}
@@ -3198,8 +3208,9 @@ func UpdateCommentContent(ctx *context.Context) {
31983208
}
31993209

32003210
ctx.JSON(http.StatusOK, map[string]any{
3201-
"content": renderedContent,
3202-
"attachments": attachmentsHTML(ctx, comment.Attachments, comment.Content),
3211+
"content": renderedContent,
3212+
"contentVersion": comment.ContentVersion,
3213+
"attachments": attachmentsHTML(ctx, comment.Attachments, comment.Content),
32033214
})
32043215
}
32053216

‎services/issue/comments.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ func CreateIssueComment(ctx context.Context, doer *user_model.User, repo *repo_m
8282
}
8383

8484
// UpdateComment updates information of comment.
85-
func UpdateComment(ctx context.Context, c *issues_model.Comment, doer *user_model.User, oldContent string) error {
85+
func UpdateComment(ctx context.Context, c *issues_model.Comment, contentVersion int, doer *user_model.User, oldContent string) error {
8686
if err := c.LoadIssue(ctx); err != nil {
8787
return err
8888
}
@@ -110,7 +110,7 @@ func UpdateComment(ctx context.Context, c *issues_model.Comment, doer *user_mode
110110
}
111111
}
112112

113-
if err := issues_model.UpdateComment(ctx, c, doer); err != nil {
113+
if err := issues_model.UpdateComment(ctx, c, contentVersion, doer); err != nil {
114114
return err
115115
}
116116

‎services/issue/content.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import (
1313
)
1414

1515
// ChangeContent changes issue content, as the given user.
16-
func ChangeContent(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, content string) error {
16+
func ChangeContent(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, content string, contentVersion int) error {
1717
if err := issue.LoadRepo(ctx); err != nil {
1818
return err
1919
}
@@ -26,7 +26,7 @@ func ChangeContent(ctx context.Context, issue *issues_model.Issue, doer *user_mo
2626

2727
oldContent := issue.Content
2828

29-
if err := issues_model.ChangeIssueContent(ctx, issue, doer, content); err != nil {
29+
if err := issues_model.ChangeIssueContent(ctx, issue, doer, content, contentVersion); err != nil {
3030
return err
3131
}
3232

‎templates/repo/diff/comments.tmpl

+1-1
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@
6161
{{end}}
6262
</div>
6363
<div id="issuecomment-{{.ID}}-raw" class="raw-content tw-hidden">{{.Content}}</div>
64-
<div class="edit-content-zone tw-hidden" data-update-url="{{$.root.RepoLink}}/comments/{{.ID}}" data-context="{{$.root.RepoLink}}" data-attachment-url="{{$.root.RepoLink}}/comments/{{.ID}}/attachments"></div>
64+
<div class="edit-content-zone tw-hidden" data-update-url="{{$.root.RepoLink}}/comments/{{.ID}}" data-content-version="{{.ContentVersion}}" data-context="{{$.root.RepoLink}}" data-attachment-url="{{$.root.RepoLink}}/comments/{{.ID}}/attachments"></div>
6565
{{if .Attachments}}
6666
{{template "repo/issue/view_content/attachments" dict "Attachments" .Attachments "RenderedContent" .RenderedContent}}
6767
{{end}}

‎templates/repo/issue/view_content.tmpl

+1-1
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@
6060
{{end}}
6161
</div>
6262
<div id="issue-{{.Issue.ID}}-raw" class="raw-content tw-hidden">{{.Issue.Content}}</div>
63-
<div class="edit-content-zone tw-hidden" data-update-url="{{$.RepoLink}}/issues/{{.Issue.Index}}/content" data-context="{{.RepoLink}}" data-attachment-url="{{$.RepoLink}}/issues/{{.Issue.Index}}/attachments" data-view-attachment-url="{{$.RepoLink}}/issues/{{.Issue.Index}}/view-attachments"></div>
63+
<div class="edit-content-zone tw-hidden" data-update-url="{{$.RepoLink}}/issues/{{.Issue.Index}}/content" data-content-version="{{.Issue.ContentVersion}}" data-context="{{.RepoLink}}" data-attachment-url="{{$.RepoLink}}/issues/{{.Issue.Index}}/attachments" data-view-attachment-url="{{$.RepoLink}}/issues/{{.Issue.Index}}/view-attachments"></div>
6464
{{if .Issue.Attachments}}
6565
{{template "repo/issue/view_content/attachments" dict "Attachments" .Issue.Attachments "RenderedContent" .Issue.RenderedContent}}
6666
{{end}}

‎templates/repo/issue/view_content/comments.tmpl

+2-2
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@
6767
{{end}}
6868
</div>
6969
<div id="issuecomment-{{.ID}}-raw" class="raw-content tw-hidden">{{.Content}}</div>
70-
<div class="edit-content-zone tw-hidden" data-update-url="{{$.RepoLink}}/comments/{{.ID}}" data-context="{{$.RepoLink}}" data-attachment-url="{{$.RepoLink}}/comments/{{.ID}}/attachments"></div>
70+
<div class="edit-content-zone tw-hidden" data-update-url="{{$.RepoLink}}/comments/{{.ID}}" data-content-version="{{.ContentVersion}}" data-context="{{$.RepoLink}}" data-attachment-url="{{$.RepoLink}}/comments/{{.ID}}/attachments"></div>
7171
{{if .Attachments}}
7272
{{template "repo/issue/view_content/attachments" dict "Attachments" .Attachments "RenderedContent" .RenderedContent}}
7373
{{end}}
@@ -441,7 +441,7 @@
441441
{{end}}
442442
</div>
443443
<div id="issuecomment-{{.ID}}-raw" class="raw-content tw-hidden">{{.Content}}</div>
444-
<div class="edit-content-zone tw-hidden" data-update-url="{{$.RepoLink}}/comments/{{.ID}}" data-context="{{$.RepoLink}}" data-attachment-url="{{$.RepoLink}}/comments/{{.ID}}/attachments"></div>
444+
<div class="edit-content-zone tw-hidden" data-update-url="{{$.RepoLink}}/comments/{{.ID}}" data-content-version="{{.ContentVersion}}" data-context="{{$.RepoLink}}" data-attachment-url="{{$.RepoLink}}/comments/{{.ID}}/attachments"></div>
445445
{{if .Attachments}}
446446
{{template "repo/issue/view_content/attachments" dict "Attachments" .Attachments "RenderedContent" .RenderedContent}}
447447
{{end}}

‎templates/repo/issue/view_content/conversation.tmpl

+1-1
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@
9696
{{end}}
9797
</div>
9898
<div id="issuecomment-{{.ID}}-raw" class="raw-content tw-hidden">{{.Content}}</div>
99-
<div class="edit-content-zone tw-hidden" data-update-url="{{$.RepoLink}}/comments/{{.ID}}" data-context="{{$.RepoLink}}" data-attachment-url="{{$.RepoLink}}/comments/{{.ID}}/attachments"></div>
99+
<div class="edit-content-zone tw-hidden" data-update-url="{{$.RepoLink}}/comments/{{.ID}}" data-content-version="{{.ContentVersion}}" data-context="{{$.RepoLink}}" data-attachment-url="{{$.RepoLink}}/comments/{{.ID}}/attachments"></div>
100100
{{if .Attachments}}
101101
{{template "repo/issue/view_content/attachments" dict "Attachments" .Attachments "RenderedContent" .RenderedContent}}
102102
{{end}}

0 commit comments

Comments
 (0)