From a0e424da859a5bf52fe1f08c08a9dcbe3e9d92eb Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Fri, 22 Jan 2021 03:56:19 +0100 Subject: [PATCH] Enhance Ghost comment mitigation Settings (#14392) * refactor models.DeleteComment and delete related reactions too * use deleteComment for UserDeleteWithCommentsMaxDays in DeleteUser * nits * Use time.Duration as other time settings have * docs * Resolve Fixme & fix potential deadlock * Disabled by Default * Update Config Value Description * switch args * Update models/issue_comment.go Co-authored-by: zeripath Co-authored-by: zeripath --- custom/conf/app.example.ini | 5 +- .../doc/advanced/config-cheat-sheet.en-us.md | 2 +- models/admin.go | 2 +- models/issue_assignees.go | 2 +- models/issue_comment.go | 20 +++++--- models/issue_reaction.go | 12 +++-- models/user.go | 47 +++++++++++++------ modules/setting/service.go | 5 +- options/locale/locale_en-US.ini | 2 +- routers/api/v1/repo/issue_comment.go | 2 +- routers/repo/issue.go | 2 +- routers/user/setting/account.go | 6 +-- services/comments/comments.go | 4 +- templates/user/settings/account.tmpl | 2 +- 14 files changed, 72 insertions(+), 41 deletions(-) diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index 8921e3a5de..f2b65a0963 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -688,9 +688,8 @@ AUTO_WATCH_NEW_REPOS = true ; Default value for AutoWatchOnChanges ; Make the user watch a repository When they commit for the first time AUTO_WATCH_ON_CHANGES = false -; Default value for the minimum age a user has to exist before deletion to keep issue comments. -; If a user deletes his account before that amount of days, his comments will be deleted as well. -USER_DELETE_WITH_COMMENTS_MAX_DAYS = 0 +; Minimum amount of time a user must exist before comments are kept when the user is deleted. +USER_DELETE_WITH_COMMENTS_MAX_TIME = 0 [webhook] ; Hook task queue length, increase if webhook shooting starts hanging diff --git a/docs/content/doc/advanced/config-cheat-sheet.en-us.md b/docs/content/doc/advanced/config-cheat-sheet.en-us.md index 50d6a07227..5d2670151c 100644 --- a/docs/content/doc/advanced/config-cheat-sheet.en-us.md +++ b/docs/content/doc/advanced/config-cheat-sheet.en-us.md @@ -474,7 +474,7 @@ relation to port exhaustion. - `ALLOW_ONLY_EXTERNAL_REGISTRATION`: **false** Set to true to force registration only using third-party services. - `NO_REPLY_ADDRESS`: **DOMAIN** Default value for the domain part of the user's email address in the git log if he has set KeepEmailPrivate to true. The user's email will be replaced with a concatenation of the user name in lower case, "@" and NO_REPLY_ADDRESS. -- `USER_DELETE_WITH_COMMENTS_MAX_DAYS`: **0** If a user deletes his account before that amount of days, his comments will be deleted as well. +- `USER_DELETE_WITH_COMMENTS_MAX_TIME`: **0** Minimum amount of time a user must exist before comments are kept when the user is deleted. ## SSH Minimum Key Sizes (`ssh.minimum_key_sizes`) diff --git a/models/admin.go b/models/admin.go index 4635676d0c..77f1c87c02 100644 --- a/models/admin.go +++ b/models/admin.go @@ -75,7 +75,7 @@ func removeStorageWithNotice(e Engine, bucket storage.ObjectStorage, title, path if err := bucket.Delete(path); err != nil { desc := fmt.Sprintf("%s [%s]: %v", title, path, err) log.Warn(title+" [%s]: %v", path, err) - if err = createNotice(x, NoticeRepository, desc); err != nil { + if err = createNotice(e, NoticeRepository, desc); err != nil { log.Error("CreateRepositoryNotice: %v", err) } } diff --git a/models/issue_assignees.go b/models/issue_assignees.go index 05e1797c19..6716f2fc70 100644 --- a/models/issue_assignees.go +++ b/models/issue_assignees.go @@ -82,7 +82,7 @@ func isUserAssignedToIssue(e Engine, issue *Issue, user *User) (isAssigned bool, } // ClearAssigneeByUserID deletes all assignments of an user -func clearAssigneeByUserID(sess *xorm.Session, userID int64) (err error) { +func clearAssigneeByUserID(sess Engine, userID int64) (err error) { _, err = sess.Delete(&IssueAssignees{AssigneeID: userID}) return } diff --git a/models/issue_comment.go b/models/issue_comment.go index dd979edcda..d8f4f0537a 100644 --- a/models/issue_comment.go +++ b/models/issue_comment.go @@ -1037,33 +1037,41 @@ func UpdateComment(c *Comment, doer *User) error { } // DeleteComment deletes the comment -func DeleteComment(comment *Comment, doer *User) error { +func DeleteComment(comment *Comment) error { sess := x.NewSession() defer sess.Close() if err := sess.Begin(); err != nil { return err } - if _, err := sess.Delete(&Comment{ + if err := deleteComment(sess, comment); err != nil { + return err + } + + return sess.Commit() +} + +func deleteComment(e Engine, comment *Comment) error { + if _, err := e.Delete(&Comment{ ID: comment.ID, }); err != nil { return err } if comment.Type == CommentTypeComment { - if _, err := sess.Exec("UPDATE `issue` SET num_comments = num_comments - 1 WHERE id = ?", comment.IssueID); err != nil { + if _, err := e.Exec("UPDATE `issue` SET num_comments = num_comments - 1 WHERE id = ?", comment.IssueID); err != nil { return err } } - if _, err := sess.Where("comment_id = ?", comment.ID).Cols("is_deleted").Update(&Action{IsDeleted: true}); err != nil { + if _, err := e.Where("comment_id = ?", comment.ID).Cols("is_deleted").Update(&Action{IsDeleted: true}); err != nil { return err } - if err := comment.neuterCrossReferences(sess); err != nil { + if err := comment.neuterCrossReferences(e); err != nil { return err } - return sess.Commit() + return deleteReaction(e, &ReactionOptions{Comment: comment}) } // CodeComments represents comments on code by using this structure: FILENAME -> LINE (+ == proposed; - == previous) -> COMMENTS diff --git a/models/issue_reaction.go b/models/issue_reaction.go index 104afce5c1..ad85e5747c 100644 --- a/models/issue_reaction.go +++ b/models/issue_reaction.go @@ -178,11 +178,15 @@ func CreateCommentReaction(doer *User, issue *Issue, comment *Comment, content s }) } -func deleteReaction(e *xorm.Session, opts *ReactionOptions) error { +func deleteReaction(e Engine, opts *ReactionOptions) error { reaction := &Reaction{ - Type: opts.Type, - UserID: opts.Doer.ID, - IssueID: opts.Issue.ID, + Type: opts.Type, + } + if opts.Doer != nil { + reaction.UserID = opts.Doer.ID + } + if opts.Issue != nil { + reaction.IssueID = opts.Issue.ID } if opts.Comment != nil { reaction.CommentID = opts.Comment.ID diff --git a/models/user.go b/models/user.go index 584c9d032d..746608aaa4 100644 --- a/models/user.go +++ b/models/user.go @@ -38,7 +38,6 @@ import ( "golang.org/x/crypto/scrypt" "golang.org/x/crypto/ssh" "xorm.io/builder" - "xorm.io/xorm" ) // UserType defines the user type @@ -1071,8 +1070,7 @@ func deleteBeans(e Engine, beans ...interface{}) (err error) { return nil } -// FIXME: need some kind of mechanism to record failure. HINT: system notice -func deleteUser(e *xorm.Session, u *User) error { +func deleteUser(e Engine, u *User) error { // Note: A user owns any repository or belongs to any organization // cannot perform delete operation. @@ -1151,12 +1149,30 @@ func deleteUser(e *xorm.Session, u *User) error { return fmt.Errorf("deleteBeans: %v", err) } - if setting.Service.UserDeleteWithCommentsMaxDays != 0 && - u.CreatedUnix.AsTime().Add(time.Duration(setting.Service.UserDeleteWithCommentsMaxDays)*24*time.Hour).After(time.Now()) { - if err = deleteBeans(e, - &Comment{PosterID: u.ID}, - ); err != nil { - return fmt.Errorf("deleteBeans: %v", err) + if setting.Service.UserDeleteWithCommentsMaxTime != 0 && + u.CreatedUnix.AsTime().Add(setting.Service.UserDeleteWithCommentsMaxTime).After(time.Now()) { + + // Delete Comments + const batchSize = 50 + for start := 0; ; start += batchSize { + comments := make([]*Comment, 0, batchSize) + if err = e.Where("type=? AND poster_id=?", CommentTypeComment, u.ID).Limit(batchSize, start).Find(&comments); err != nil { + return err + } + if len(comments) == 0 { + break + } + + for _, comment := range comments { + if err = deleteComment(e, comment); err != nil { + return err + } + } + } + + // Delete Reactions + if err = deleteReaction(e, &ReactionOptions{Doer: u}); err != nil { + return err } } @@ -1195,18 +1211,21 @@ func deleteUser(e *xorm.Session, u *User) error { return fmt.Errorf("Delete: %v", err) } - // FIXME: system notice // Note: There are something just cannot be roll back, // so just keep error logs of those operations. path := UserPath(u.Name) - if err := util.RemoveAll(path); err != nil { - return fmt.Errorf("Failed to RemoveAll %s: %v", path, err) + if err = util.RemoveAll(path); err != nil { + err = fmt.Errorf("Failed to RemoveAll %s: %v", path, err) + _ = createNotice(e, NoticeTask, fmt.Sprintf("delete user '%s': %v", u.Name, err)) + return err } if len(u.Avatar) > 0 { avatarPath := u.CustomAvatarRelativePath() - if err := storage.Avatars.Delete(avatarPath); err != nil { - return fmt.Errorf("Failed to remove %s: %v", avatarPath, err) + if err = storage.Avatars.Delete(avatarPath); err != nil { + err = fmt.Errorf("Failed to remove %s: %v", avatarPath, err) + _ = createNotice(e, NoticeTask, fmt.Sprintf("delete user '%s': %v", u.Name, err)) + return err } } diff --git a/modules/setting/service.go b/modules/setting/service.go index 86f46898ac..4410a600dd 100644 --- a/modules/setting/service.go +++ b/modules/setting/service.go @@ -6,6 +6,7 @@ package setting import ( "regexp" + "time" "code.gitea.io/gitea/modules/structs" ) @@ -50,7 +51,7 @@ var Service struct { AutoWatchNewRepos bool AutoWatchOnChanges bool DefaultOrgMemberVisible bool - UserDeleteWithCommentsMaxDays int + UserDeleteWithCommentsMaxTime time.Duration // OpenID settings EnableOpenIDSignIn bool @@ -103,7 +104,7 @@ func newService() { Service.DefaultOrgVisibility = sec.Key("DEFAULT_ORG_VISIBILITY").In("public", structs.ExtractKeysFromMapString(structs.VisibilityModes)) Service.DefaultOrgVisibilityMode = structs.VisibilityModes[Service.DefaultOrgVisibility] Service.DefaultOrgMemberVisible = sec.Key("DEFAULT_ORG_MEMBER_VISIBLE").MustBool() - Service.UserDeleteWithCommentsMaxDays = sec.Key("USER_DELETE_WITH_COMMENTS_MAX_DAYS").MustInt(0) + Service.UserDeleteWithCommentsMaxTime = sec.Key("USER_DELETE_WITH_COMMENTS_MAX_TIME").MustDuration(0) sec = Cfg.Section("openid") Service.EnableOpenIDSignIn = sec.Key("ENABLE_OPENID_SIGNIN").MustBool(!InstallLock) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 53cee057c9..ec133f4b41 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -648,7 +648,7 @@ repos_none = You do not own any repositories delete_account = Delete Your Account delete_prompt = This operation will permanently delete your user account. It CAN NOT be undone. -delete_with_all_comments = Your account is younger than %d days. To avoid ghost comments, all issue/PR comments will be deleted with it. +delete_with_all_comments = Your account is younger than %s. To avoid ghost comments, all issue/PR comments will be deleted with it. confirm_delete_account = Confirm Deletion delete_account_title = Delete User Account delete_account_desc = Are you sure you want to permanently delete this user account? diff --git a/routers/api/v1/repo/issue_comment.go b/routers/api/v1/repo/issue_comment.go index 245c90d49a..af69ae981a 100644 --- a/routers/api/v1/repo/issue_comment.go +++ b/routers/api/v1/repo/issue_comment.go @@ -509,7 +509,7 @@ func deleteIssueComment(ctx *context.APIContext) { return } - if err = comment_service.DeleteComment(comment, ctx.User); err != nil { + if err = comment_service.DeleteComment(ctx.User, comment); err != nil { ctx.Error(http.StatusInternalServerError, "DeleteCommentByID", err) return } diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 6a532dc125..fbeae75ab5 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -2125,7 +2125,7 @@ func DeleteComment(ctx *context.Context) { return } - if err = comment_service.DeleteComment(comment, ctx.User); err != nil { + if err = comment_service.DeleteComment(ctx.User, comment); err != nil { ctx.ServerError("DeleteCommentByID", err) return } diff --git a/routers/user/setting/account.go b/routers/user/setting/account.go index 3b4191f0be..42c2c59b7e 100644 --- a/routers/user/setting/account.go +++ b/routers/user/setting/account.go @@ -302,8 +302,8 @@ func loadAccountData(ctx *context.Context) { ctx.Data["ActivationsPending"] = pendingActivation ctx.Data["CanAddEmails"] = !pendingActivation || !setting.Service.RegisterEmailConfirm - if setting.Service.UserDeleteWithCommentsMaxDays != 0 { - ctx.Data["UserDeleteWithCommentsMaxDays"] = setting.Service.UserDeleteWithCommentsMaxDays - ctx.Data["UserDeleteWithComments"] = ctx.User.CreatedUnix.AsTime().Add(time.Duration(setting.Service.UserDeleteWithCommentsMaxDays) * 24 * time.Hour).After(time.Now()) + if setting.Service.UserDeleteWithCommentsMaxTime != 0 { + ctx.Data["UserDeleteWithCommentsMaxTime"] = setting.Service.UserDeleteWithCommentsMaxTime.String() + ctx.Data["UserDeleteWithComments"] = ctx.User.CreatedUnix.AsTime().Add(setting.Service.UserDeleteWithCommentsMaxTime).After(time.Now()) } } diff --git a/services/comments/comments.go b/services/comments/comments.go index ad79eec4fb..f8bdc8153b 100644 --- a/services/comments/comments.go +++ b/services/comments/comments.go @@ -43,8 +43,8 @@ func UpdateComment(c *models.Comment, doer *models.User, oldContent string) erro } // DeleteComment deletes the comment -func DeleteComment(comment *models.Comment, doer *models.User) error { - if err := models.DeleteComment(comment, doer); err != nil { +func DeleteComment(doer *models.User, comment *models.Comment) error { + if err := models.DeleteComment(comment); err != nil { return err } diff --git a/templates/user/settings/account.tmpl b/templates/user/settings/account.tmpl index 4f7d8a50c7..04ab539088 100644 --- a/templates/user/settings/account.tmpl +++ b/templates/user/settings/account.tmpl @@ -174,7 +174,7 @@

{{svg "octicon-alert"}} {{.i18n.Tr "settings.delete_prompt" | Str2html}}

{{ if .UserDeleteWithComments }} -

{{.i18n.Tr "settings.delete_with_all_comments" .UserDeleteWithCommentsMaxDays | Str2html}}

+

{{.i18n.Tr "settings.delete_with_all_comments" .UserDeleteWithCommentsMaxTime | Str2html}}

{{ end }}