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 <art27@cantab.net>
Co-authored-by: zeripath <art27@cantab.net>

diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini
index 8921e3a5decef37dfae3836282a59d933f0f9a92..f2b65a0963db6221f17bfcf489266c7f76e123d9 100644 (file)
--- 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 50d6a072276f28a9cdc01491030647e2fe88972b..5d2670151cb55b62e279281dbdd4314c1279eebb 100644 (file)
--- 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 4635676d0cbc8be66df6e1fb5ef5492f0afd710c..77f1c87c026cb948c4f52a2f873d935e1b606ae4 100644 (file)
--- 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 05e1797c19969ad31d34c5d91e1c3de873150942..6716f2fc70f80778aab161dca80b459f82d52b27 100644 (file)
--- 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 dd979edcda52675f342d335b39702dd14ac519e0..d8f4f0537ac907635a34119c451554ac4d7ca519 100644 (file)
--- 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 104afce5c192939bfca1871c640b097cdbc07fbd..ad85e5747c3db83d6e9e098c8482bb6f344e426e 100644 (file)
--- 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 584c9d032d74c698d12b0150e3dff655a70a2fe5..746608aaa47cfbff9efcc5df1cb65faff211ade1 100644 (file)
--- 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 86f46898ac109f96c2a1200f98496e8ef6d704c6..4410a600dd86e08482629ba43eccbed817b1b4d1 100644 (file)
--- 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 53cee057c97bf6782c9c959df5b71552c04dd729..ec133f4b41df8028ce4c9f957c5c1d9d8fac1f45 100644 (file)
--- 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 <strong>CAN NOT</strong> 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 245c90d49aab0903ecb1c52e7d2987675a8e2272..af69ae981ad3b2e9e01984133d55411d97643822 100644 (file)
--- 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 6a532dc125a92b802a7d44b3b50b3c1bb3eda8ba..fbeae75ab5218792f69c4cb708d6ed07f73e3bb5 100644 (file)
--- 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 3b4191f0beeb28468f40c71abd7090ce2bf7b4fb..42c2c59b7e75235cca3464c6aa8ce47878539477 100644 (file)
--- 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 ad79eec4fb91db46edb0dc3f5b3c0360be448655..f8bdc8153b47492169b97e536f77382c098466f9 100644 (file)
--- 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 4f7d8a50c705a8ee510dbf337829b325c5d47247..04ab539088397b78e4b0672bac2c99d7115053c2 100644 (file)
--- a/templates/user/settings/account.tmpl
+++ b/templates/user/settings/account.tmpl
@@ -174,7 +174,7 @@
                        <div class="ui red message">
                                <p class="text left">{{svg "octicon-alert"}} {{.i18n.Tr "settings.delete_prompt" | Str2html}}</p>
                                {{ if .UserDeleteWithComments }}
-                               <p class="text left" style="font-weight: bold;">{{.i18n.Tr "settings.delete_with_all_comments" .UserDeleteWithCommentsMaxDays | Str2html}}</p>
+                               <p class="text left" style="font-weight: bold;">{{.i18n.Tr "settings.delete_with_all_comments" .UserDeleteWithCommentsMaxTime | Str2html}}</p>
                                {{ end }}
                        </div>
                        <form class="ui form ignore-dirty" id="delete-form" action="{{AppSubUrl}}/user/settings/account/delete" method="post">