From a749387c80b56f4f6aa4a4bd534eb6c0b3bf3d88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=94=A6=E5=BF=83?= <41134017+Lhcfl@users.noreply.github.com> Date: Tue, 23 Jul 2024 15:27:11 +0800 Subject: [PATCH] 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 | 5 +++ app/services/staff_action_logger.rb | 42 +++++++++++-------- config/locales/client.en.yml | 2 + ...506_add_post_id_index_to_user_histories.rb | 13 ++++++ lib/post_destroyer.rb | 15 ++++++- spec/requests/posts_controller_spec.rb | 22 ++++++++++ spec/requests/topics_controller_spec.rb | 19 +++++++++ 7 files changed, 99 insertions(+), 19 deletions(-) create mode 100644 db/migrate/20240723030506_add_post_id_index_to_user_histories.rb diff --git a/app/models/user_history.rb b/app/models/user_history.rb index 85a2ef601b9..b3c41856e5e 100644 --- a/app/models/user_history.rb +++ b/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) diff --git a/app/services/staff_action_logger.rb b/app/services/staff_action_logger.rb index cb222672cc3..c39befbe89f 100644 --- a/app/services/staff_action_logger.rb +++ b/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!( diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index dd6220fde47..a269a08bf37 100644 --- a/config/locales/client.en.yml +++ b/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." diff --git a/db/migrate/20240723030506_add_post_id_index_to_user_histories.rb b/db/migrate/20240723030506_add_post_id_index_to_user_histories.rb new file mode 100644 index 00000000000..4be0e7b76d0 --- /dev/null +++ b/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 diff --git a/lib/post_destroyer.rb b/lib/post_destroyer.rb index 25e3afcc945..e3f206be6e5 100644 --- a/lib/post_destroyer.rb +++ b/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( diff --git a/spec/requests/posts_controller_spec.rb b/spec/requests/posts_controller_spec.rb index 043a625b987..92491f4bcdd 100644 --- a/spec/requests/posts_controller_spec.rb +++ b/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 diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 224a85c2015..ca9b69c4bc8 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/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