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>

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)
 		}
 	}

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
 }

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

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

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
 		}
 	}