FEATURE: Clean up previously logged information after permanently deleting posts (#28033)

* FEATURE: Clean up previously logged information after permanently deleting posts

When soft deleteing a topic or post, we will log some details in the
staff log, including the raw content of the post. Before this commit, we
will not clear the information in these records. Therefore, after
permanently deleting the post, `UserHistory` still retains copy of the
permanently deleted post. This is an unexpected behaviour and may raise
some potential legal issues.

This commit adds a behavior that when a post is permanently deleted, the
details column of the `UserHistory` associated with the post will be
overwritten to "(permanently deleted)". At the same time, for permanent
deletion, a new `action_id` is introduced to distinguish it from soft
deletion.

Related meta topic: https://meta.discourse.org/t/introduce-a-way-to-also-permanently-delete-the-sensitive-info-from-the-staff-logs/292546

app/models/user_history.rb

@@ -150,6 +150,8 @@ class UserHistory < ActiveRecord::Base
         topic_slow_mode_removed: 111,
         custom_emoji_create: 112,
         custom_emoji_destroy: 113,
+        delete_post_permanently: 114,
+        delete_topic_permanently: 115,
       )
   end
 
@@ -262,6 +264,8 @@ class UserHistory < ActiveRecord::Base
       topic_slow_mode_removed
       custom_emoji_create
       custom_emoji_destroy
+      delete_post_permanently
+      delete_topic_permanently
     ]
   end
 
@@ -364,6 +368,7 @@ end
 #  index_user_histories_on_acting_user_id_and_action_and_id        (acting_user_id,action,id)
 #  index_user_histories_on_action_and_id                           (action,id)
 #  index_user_histories_on_category_id                             (category_id)
+#  index_user_histories_on_post_id                                 (post_id)
 #  index_user_histories_on_subject_and_id                          (subject,id)
 #  index_user_histories_on_target_user_id_and_id                   (target_user_id,id)
 #  index_user_histories_on_topic_id_and_target_user_id_and_action  (topic_id,target_user_id,action)

app/services/staff_action_logger.rb

@@ -74,18 +74,22 @@ class StaffActionLogger
     name = deleted_post.user.try(:name) || I18n.t("staff_action_logs.unknown")
     topic_title = topic.try(:title) || I18n.t("staff_action_logs.not_found")
 
-    details = [
-      "id: #{deleted_post.id}",
-      "created_at: #{deleted_post.created_at}",
-      "user: #{username} (#{name})",
-      "topic: #{topic_title}",
-      "post_number: #{deleted_post.post_number}",
-      "raw: #{truncate(deleted_post.raw)}",
-    ]
+    if opts[:permanent]
+      details = []
+    else
+      details = [
+        "id: #{deleted_post.id}",
+        "created_at: #{deleted_post.created_at}",
+        "user: #{username} (#{name})",
+        "topic: #{topic_title}",
+        "post_number: #{deleted_post.post_number}",
+        "raw: #{truncate(deleted_post.raw)}",
+      ]
+    end
 
     UserHistory.create!(
       params(opts).merge(
-        action: UserHistory.actions[:delete_post],
+        action: UserHistory.actions[opts[:permanent] ? :delete_post_permanently : :delete_post],
         post_id: deleted_post.id,
         details: details.join("\n"),
       ),
@@ -97,15 +101,19 @@ class StaffActionLogger
 
     user = topic.user ? "#{topic.user.username} (#{topic.user.name})" : "(deleted user)"
 
-    details = [
-      "id: #{topic.id}",
-      "created_at: #{topic.created_at}",
-      "user: #{user}",
-      "title: #{topic.title}",
-    ]
+    if action == "delete_topic_permanently"
+      details = []
+    else
+      details = [
+        "id: #{topic.id}",
+        "created_at: #{topic.created_at}",
+        "user: #{user}",
+        "title: #{topic.title}",
+      ]
 
-    if first_post = topic.ordered_posts.with_deleted.first
-      details << "raw: #{truncate(first_post.raw)}"
+      if first_post = topic.ordered_posts.with_deleted.first
+        details << "raw: #{truncate(first_post.raw)}"
+      end
     end
 
     UserHistory.create!(

config/locales/client.en.yml

@@ -6250,6 +6250,8 @@ en:
             topic_slow_mode_removed: "remove topic slow mode"
             custom_emoji_create: "create custom emoji"
             custom_emoji_destroy: "delete custom emoji"
+            delete_post_permanently: "permanently delete post"
+            delete_topic_permanently: "permanently delete topic"
         screened_emails:
           title: "Screened Emails"
           description: "When someone tries to create a new account, the following email addresses will be checked and the registration will be blocked, or some other action performed."

db/migrate/20240723030506_add_post_id_index_to_user_histories.rb

@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+class AddPostIdIndexToUserHistories < ActiveRecord::Migration[7.1]
+  disable_ddl_transaction!
+
+  def up
+    remove_index :user_histories, :post_id, if_exists: true
+    add_index :user_histories, :post_id, algorithm: :concurrently
+  end
+
+  def down
+    remove_index :user_histories, :post_id
+  end
+end

lib/post_destroyer.rb

@@ -193,11 +193,15 @@ class PostDestroyer
         if @post.topic && @post.is_first_post?
           StaffActionLogger.new(@user).log_topic_delete_recover(
             @post.topic,
-            "delete_topic",
+            permanent? ? "delete_topic_permanently" : "delete_topic",
             @opts.slice(:context),
           )
         else
-          StaffActionLogger.new(@user).log_post_deletion(@post, @opts.slice(:context))
+          StaffActionLogger.new(@user).log_post_deletion(
+            @post,
+            **@opts.slice(:context),
+            permanent: permanent?,
+          )
         end
       end
 
@@ -211,6 +215,13 @@ class PostDestroyer
       update_user_counts if !permanent?
       TopicUser.update_post_action_cache(post_id: @post.id)
 
+      if permanent?
+        if @post.topic && @post.is_first_post?
+          UserHistory.where(topic_id: @post.topic.id).update_all(details: "(permanently deleted)")
+        end
+        UserHistory.where(post_id: @post.id).update_all(details: "(permanently deleted)")
+      end
+
       DB.after_commit do
         if @opts[:reviewable]
           notify_deletion(

spec/requests/posts_controller_spec.rb

@@ -300,6 +300,28 @@ RSpec.describe PostsController do
           delete "/posts/#{post.id}.json", params: { force_destroy: true }
           expect(response.status).to eq(403)
         end
+
+        it "creates a log and clean up previously recorded sensitive information" do
+          sign_in(admin)
+
+          delete "/posts/#{post.id}.json"
+          expect(response.status).to eq(200)
+          expect(post.reload.deleted_by_id).to eq(admin.id)
+
+          post.update!(deleted_at: 10.minutes.ago)
+
+          delete "/posts/#{post.id}.json", params: { force_destroy: true }
+          expect(response.status).to eq(200)
+
+          expect(UserHistory.last).to have_attributes(
+            action: UserHistory.actions[:delete_post_permanently],
+            acting_user_id: admin.id,
+          )
+
+          expect(UserHistory.where(post_id: post.id, details: "(permanently deleted)").count).to eq(
+            2,
+          )
+        end
       end
     end
   end

spec/requests/topics_controller_spec.rb

@@ -1535,6 +1535,25 @@ RSpec.describe TopicsController do
         expect(Post.find_by(id: small_action_post.id)).to eq(nil)
       end
 
+      it "creates a log and clean up previously recorded sensitive information" do
+        small_action_post = Fabricate(:small_action, topic: topic)
+        PostDestroyer.new(Discourse.system_user, post).destroy
+        PostDestroyer.new(Discourse.system_user, small_action_post).destroy
+
+        delete "/t/#{topic.id}.json", params: { force_destroy: true }
+
+        expect(response.status).to eq(200)
+
+        expect(UserHistory.last).to have_attributes(
+          action: UserHistory.actions[:delete_topic_permanently],
+          acting_user_id: admin.id,
+        )
+
+        expect(UserHistory.where(topic_id: topic.id, details: "(permanently deleted)").count).to eq(
+          2,
+        )
+      end
+
       it "does not allow to destroy topic if not all posts were force destroyed" do
         _other_post = Fabricate(:post, topic: topic, post_number: 2)
         PostDestroyer.new(Discourse.system_user, post).destroy