From 07a9163ea8ad4ffc4faa25bd0a03e2847b7f1690 Mon Sep 17 00:00:00 2001 From: Roman Rizzi Date: Mon, 28 Nov 2022 13:32:57 -0300 Subject: [PATCH] FEATURE: Deleting a user with their posts also deletes chat messages. (#19194) This commit introduce a new API for registering callbacks, which we'll execute when a user gets destroyed, and the `delete_posts` opt is true. The chat plugin registers one callback and queues a job to destroy every message from that user in batches. --- app/models/reviewable_flagged_post.rb | 2 +- app/services/user_destroyer.rb | 4 ++ lib/discourse_plugin_registry.rb | 2 + lib/plugin/instance.rb | 11 ++++ .../app/jobs/regular/delete_user_messages.rb | 12 +++++ .../scheduled/delete_old_chat_messages.rb | 43 ++++++---------- .../app/services/chat_message_destroyer.rb | 23 +++++++++ plugins/chat/plugin.rb | 6 +++ .../jobs/delete_old_chat_messages_spec.rb | 50 ------------------- .../jobs/regular/delete_user_messages_spec.rb | 32 ++++++++++++ plugins/chat/spec/plugin_spec.rb | 13 +++++ .../services/chat_message_destroyer_spec.rb | 50 +++++++++++++++++++ spec/lib/plugin/instance_spec.rb | 32 ++++++++++++ spec/models/reviewable_flagged_post_spec.rb | 19 +++++++ 14 files changed, 219 insertions(+), 80 deletions(-) create mode 100644 plugins/chat/app/jobs/regular/delete_user_messages.rb create mode 100644 plugins/chat/app/services/chat_message_destroyer.rb create mode 100644 plugins/chat/spec/jobs/regular/delete_user_messages_spec.rb create mode 100644 plugins/chat/spec/services/chat_message_destroyer_spec.rb diff --git a/app/models/reviewable_flagged_post.rb b/app/models/reviewable_flagged_post.rb index 032f6bed900..44011bf6fbf 100644 --- a/app/models/reviewable_flagged_post.rb +++ b/app/models/reviewable_flagged_post.rb @@ -72,7 +72,7 @@ class ReviewableFlaggedPost < Reviewable build_action(actions, :ignore, icon: 'external-link-alt') - if potential_spam? && guardian.can_delete_all_posts?(target_created_by) + if potential_spam? && guardian.can_delete_user?(target_created_by) delete_user_actions(actions) end diff --git a/app/services/user_destroyer.rb b/app/services/user_destroyer.rb index 408dffcb9ef..7458e9dc538 100644 --- a/app/services/user_destroyer.rb +++ b/app/services/user_destroyer.rb @@ -35,6 +35,10 @@ class UserDestroyer category_topic_ids = Category.where("topic_id IS NOT NULL").pluck(:topic_id) if opts[:delete_posts] + DiscoursePluginRegistry.user_destroyer_on_content_deletion_callbacks.each do |cb| + cb.call(user, @guardian, opts) + end + agree_with_flags(user) if opts[:delete_as_spammer] block_external_urls(user) if opts[:block_urls] delete_posts(user, category_topic_ids, opts) diff --git a/lib/discourse_plugin_registry.rb b/lib/discourse_plugin_registry.rb index e82d640e294..c9d0baf5915 100644 --- a/lib/discourse_plugin_registry.rb +++ b/lib/discourse_plugin_registry.rb @@ -97,6 +97,8 @@ class DiscoursePluginRegistry define_filtered_register :email_unsubscribers + define_filtered_register :user_destroyer_on_content_deletion_callbacks + def self.register_auth_provider(auth_provider) self.auth_providers << auth_provider end diff --git a/lib/plugin/instance.rb b/lib/plugin/instance.rb index 980f94889e0..09ceac7804e 100644 --- a/lib/plugin/instance.rb +++ b/lib/plugin/instance.rb @@ -1138,6 +1138,17 @@ class Plugin::Instance HashtagAutocompleteService.register_type_in_context(type, context, priority) end + ## + # Register a block that will be called when the UserDestroyer runs + # with the :delete_posts opt set to true. It's important to note that the block will + # execute before any other :delete_posts actions, it allows us to manipulate flags + # before agreeing with them. For example, discourse-akismet makes use of this + # + # @param {Block} callback to be called with the user, guardian, and the destroyer opts as arguments + def register_user_destroyer_on_content_deletion_callback(callback) + DiscoursePluginRegistry.register_user_destroyer_on_content_deletion_callback(callback, self) + end + protected def self.js_path diff --git a/plugins/chat/app/jobs/regular/delete_user_messages.rb b/plugins/chat/app/jobs/regular/delete_user_messages.rb new file mode 100644 index 00000000000..ca52ba73860 --- /dev/null +++ b/plugins/chat/app/jobs/regular/delete_user_messages.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +module Jobs + class DeleteUserMessages < ::Jobs::Base + def execute(args) + return if args[:user_id].nil? + + ChatMessageDestroyer.new + .destroy_in_batches(ChatMessage.with_deleted.where(user_id: args[:user_id])) + end + end +end diff --git a/plugins/chat/app/jobs/scheduled/delete_old_chat_messages.rb b/plugins/chat/app/jobs/scheduled/delete_old_chat_messages.rb index 07999155281..6fa85197197 100644 --- a/plugins/chat/app/jobs/scheduled/delete_old_chat_messages.rb +++ b/plugins/chat/app/jobs/scheduled/delete_old_chat_messages.rb @@ -9,47 +9,32 @@ module Jobs delete_dm_channel_messages end + private + def delete_public_channel_messages return unless valid_day_value?(:chat_channel_retention_days) - ChatMessage - .in_public_channel - .with_deleted - .created_before(SiteSetting.chat_channel_retention_days.days.ago) - .in_batches(of: 200) - .each do |relation| - destroyed_ids = relation.destroy_all.pluck(:id) - reset_last_read_message_id(destroyed_ids) - delete_flags(destroyed_ids) - end + ChatMessageDestroyer.new.destroy_in_batches( + ChatMessage + .in_public_channel + .with_deleted + .created_before(SiteSetting.chat_channel_retention_days.days.ago) + ) end def delete_dm_channel_messages return unless valid_day_value?(:chat_dm_retention_days) - ChatMessage - .in_dm_channel - .with_deleted - .created_before(SiteSetting.chat_dm_retention_days.days.ago) - .in_batches(of: 200) - .each do |relation| - destroyed_ids = relation.destroy_all.pluck(:id) - reset_last_read_message_id(destroyed_ids) - end + ChatMessageDestroyer.new.destroy_in_batches( + ChatMessage + .in_dm_channel + .with_deleted + .created_before(SiteSetting.chat_dm_retention_days.days.ago) + ) end def valid_day_value?(setting_name) (SiteSetting.public_send(setting_name) || 0).positive? end - - def reset_last_read_message_id(ids) - UserChatChannelMembership.where(last_read_message_id: ids).update_all( - last_read_message_id: nil, - ) - end - - def delete_flags(message_ids) - ReviewableChatMessage.where(target_id: message_ids).destroy_all - end end end diff --git a/plugins/chat/app/services/chat_message_destroyer.rb b/plugins/chat/app/services/chat_message_destroyer.rb new file mode 100644 index 00000000000..311e1f96110 --- /dev/null +++ b/plugins/chat/app/services/chat_message_destroyer.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +class ChatMessageDestroyer + def destroy_in_batches(chat_messages_query, batch_size: 200) + chat_messages_query.in_batches(of: batch_size).each do |relation| + destroyed_ids = relation.destroy_all.pluck(:id) + reset_last_read(destroyed_ids) + delete_flags(destroyed_ids) + end + end + + private + + def reset_last_read(message_ids) + UserChatChannelMembership.where(last_read_message_id: message_ids).update_all( + last_read_message_id: nil, + ) + end + + def delete_flags(message_ids) + ReviewableChatMessage.where(target_id: message_ids).destroy_all + end +end diff --git a/plugins/chat/plugin.rb b/plugins/chat/plugin.rb index 1544fba4f57..10416b4a3c1 100644 --- a/plugins/chat/plugin.rb +++ b/plugins/chat/plugin.rb @@ -192,11 +192,13 @@ after_initialize do load File.expand_path("../app/jobs/regular/chat_notify_mentioned.rb", __FILE__) load File.expand_path("../app/jobs/regular/chat_notify_watching.rb", __FILE__) load File.expand_path("../app/jobs/regular/update_channel_user_count.rb", __FILE__) + load File.expand_path("../app/jobs/regular/delete_user_messages.rb", __FILE__) load File.expand_path("../app/jobs/scheduled/delete_old_chat_messages.rb", __FILE__) load File.expand_path("../app/jobs/scheduled/update_user_counts_for_chat_channels.rb", __FILE__) load File.expand_path("../app/jobs/scheduled/email_chat_notifications.rb", __FILE__) load File.expand_path("../app/jobs/scheduled/auto_join_users.rb", __FILE__) load File.expand_path("../app/services/chat_publisher.rb", __FILE__) + load File.expand_path("../app/services/chat_message_destroyer.rb", __FILE__) load File.expand_path("../app/controllers/api_controller.rb", __FILE__) load File.expand_path("../app/controllers/api/chat_channels_controller.rb", __FILE__) load File.expand_path("../app/controllers/api/chat_channel_memberships_controller.rb", __FILE__) @@ -729,6 +731,10 @@ after_initialize do limited_pretty_text_markdown_rules: ChatMessage::MARKDOWN_IT_RULES, hashtag_configurations: HashtagAutocompleteService.contexts_with_ordered_types, } + + register_user_destroyer_on_content_deletion_callback( + Proc.new { |user| Jobs.enqueue(:delete_user_messages, user_id: user.id) } + ) end if Rails.env == "test" diff --git a/plugins/chat/spec/jobs/delete_old_chat_messages_spec.rb b/plugins/chat/spec/jobs/delete_old_chat_messages_spec.rb index 6aafe1984ad..79b0ea94c21 100644 --- a/plugins/chat/spec/jobs/delete_old_chat_messages_spec.rb +++ b/plugins/chat/spec/jobs/delete_old_chat_messages_spec.rb @@ -109,40 +109,6 @@ describe Jobs::DeleteOldChatMessages do SiteSetting.chat_channel_retention_days = 800 expect { described_class.new.execute }.not_to change { ChatMessage.in_public_channel.count } end - - it "resets last_read_message_id from memberships" do - SiteSetting.chat_channel_retention_days = 20 - membership = - UserChatChannelMembership.create!( - user: Fabricate(:user), - chat_channel: public_channel, - last_read_message_id: public_days_old_30.id, - following: true, - desktop_notification_level: 2, - mobile_notification_level: 2, - ) - described_class.new.execute - - expect(membership.reload.last_read_message_id).to be_nil - end - - it "deletes flags associated to deleted chat messages" do - SiteSetting.chat_channel_retention_days = 10 - guardian = Guardian.new(Discourse.system_user) - Chat::ChatReviewQueue.new.flag_message( - public_days_old_20, - guardian, - ReviewableScore.types[:off_topic], - ) - - reviewable = ReviewableChatMessage.last - expect(reviewable).to be_present - - described_class.new.execute - - expect { public_days_old_20.reload }.to raise_exception(ActiveRecord::RecordNotFound) - expect { reviewable.reload }.to raise_exception(ActiveRecord::RecordNotFound) - end end describe "dm channels" do @@ -166,21 +132,5 @@ describe Jobs::DeleteOldChatMessages do SiteSetting.chat_dm_retention_days = 800 expect { described_class.new.execute }.not_to change { ChatMessage.in_dm_channel.count } end - - it "resets last_read_message_id from memberships" do - SiteSetting.chat_dm_retention_days = 20 - membership = - UserChatChannelMembership.create!( - user: Fabricate(:user), - chat_channel: dm_channel, - last_read_message_id: dm_days_old_30.id, - following: true, - desktop_notification_level: 2, - mobile_notification_level: 2, - ) - described_class.new.execute - - expect(membership.reload.last_read_message_id).to be_nil - end end end diff --git a/plugins/chat/spec/jobs/regular/delete_user_messages_spec.rb b/plugins/chat/spec/jobs/regular/delete_user_messages_spec.rb new file mode 100644 index 00000000000..26242bae9c7 --- /dev/null +++ b/plugins/chat/spec/jobs/regular/delete_user_messages_spec.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +RSpec.describe Jobs::DeleteUserMessages do + describe "#execute" do + fab!(:user_1) { Fabricate(:user) } + fab!(:channel) { Fabricate(:chat_channel) } + fab!(:chat_message) { Fabricate(:chat_message, chat_channel: channel, user: user_1) } + + it "deletes messages from the user" do + subject.execute(user_id: user_1) + + expect { chat_message.reload }.to raise_error(ActiveRecord::RecordNotFound) + end + + it "doesn't delete messages from other users" do + user_2 = Fabricate(:user) + user_2_message = Fabricate(:chat_message, chat_channel: channel, user: user_2) + + subject.execute(user_id: user_1) + + expect(user_2_message.reload).to be_present + end + + it "deletes trashed messages" do + chat_message.trash! + + subject.execute(user_id: user_1) + + expect(ChatMessage.with_deleted.where(id: chat_message.id)).to be_empty + end + end +end diff --git a/plugins/chat/spec/plugin_spec.rb b/plugins/chat/spec/plugin_spec.rb index fdf17580d0c..64bbde077bb 100644 --- a/plugins/chat/spec/plugin_spec.rb +++ b/plugins/chat/spec/plugin_spec.rb @@ -421,4 +421,17 @@ describe Chat do end end end + + describe "Deleting posts while deleting a user" do + fab!(:user) { Fabricate(:user) } + + it "queues a job to also delete chat messages" do + deletion_opts = { delete_posts: true } + + expect { UserDestroyer.new(Discourse.system_user).destroy(user, deletion_opts) }.to change( + Jobs::DeleteUserMessages.jobs, + :size, + ).by(1) + end + end end diff --git a/plugins/chat/spec/services/chat_message_destroyer_spec.rb b/plugins/chat/spec/services/chat_message_destroyer_spec.rb new file mode 100644 index 00000000000..66e58a85aad --- /dev/null +++ b/plugins/chat/spec/services/chat_message_destroyer_spec.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +RSpec.describe ChatMessageDestroyer do + describe "#destroy_in_batches" do + fab!(:message_1) { Fabricate(:chat_message) } + fab!(:user_1) { Fabricate(:user) } + + it "resets last_read_message_id from memberships" do + membership = + UserChatChannelMembership.create!( + user: user_1, + chat_channel: message_1.chat_channel, + last_read_message: message_1, + following: true, + desktop_notification_level: 2, + mobile_notification_level: 2, + ) + + described_class.new.destroy_in_batches(ChatMessage.where(id: message_1.id)) + + expect(membership.reload.last_read_message_id).to be_nil + end + + it "deletes flags associated to deleted chat messages" do + guardian = Guardian.new(Discourse.system_user) + Chat::ChatReviewQueue.new.flag_message( + message_1, + guardian, + ReviewableScore.types[:off_topic], + ) + + reviewable = ReviewableChatMessage.last + expect(reviewable).to be_present + + described_class.new.destroy_in_batches(ChatMessage.where(id: message_1.id)) + + expect { message_1.reload }.to raise_exception(ActiveRecord::RecordNotFound) + expect { reviewable.reload }.to raise_exception(ActiveRecord::RecordNotFound) + end + + it "doesn't delete other messages" do + message_2 = Fabricate(:chat_message, chat_channel: message_1.chat_channel) + + described_class.new.destroy_in_batches(ChatMessage.where(id: message_1.id)) + + expect { message_1.reload }.to raise_exception(ActiveRecord::RecordNotFound) + expect(message_2.reload).to be_present + end + end +end diff --git a/spec/lib/plugin/instance_spec.rb b/spec/lib/plugin/instance_spec.rb index b22f10c8fd3..e5bc2df288e 100644 --- a/spec/lib/plugin/instance_spec.rb +++ b/spec/lib/plugin/instance_spec.rb @@ -770,4 +770,36 @@ RSpec.describe Plugin::Instance do expect(About.displayed_plugin_stat_groups).to eq([]) end end + + describe "#register_user_destroyer_on_content_deletion_callback" do + let(:plugin) { Plugin::Instance.new } + + after do + DiscoursePluginRegistry.reset_register!(:user_destroyer_on_content_deletion_callbacks) + end + + fab!(:user) { Fabricate(:user) } + + it "calls the callback when the UserDestroyer runs with the delete_posts opt set to true" do + callback_called = false + + cb = Proc.new { callback_called = true } + plugin.register_user_destroyer_on_content_deletion_callback(cb) + + UserDestroyer.new(Discourse.system_user).destroy(user, { delete_posts: true }) + + expect(callback_called).to eq(true) + end + + it "doesn't run the callback when delete_posts opt is not true" do + callback_called = false + + cb = Proc.new { callback_called = true } + plugin.register_user_destroyer_on_content_deletion_callback(cb) + + UserDestroyer.new(Discourse.system_user).destroy(user, {}) + + expect(callback_called).to eq(false) + end + end end diff --git a/spec/models/reviewable_flagged_post_spec.rb b/spec/models/reviewable_flagged_post_spec.rb index 691276e5687..8a7c81f2e66 100644 --- a/spec/models/reviewable_flagged_post_spec.rb +++ b/spec/models/reviewable_flagged_post_spec.rb @@ -83,6 +83,25 @@ RSpec.describe ReviewableFlaggedPost, type: :model do expect(reviewable.actions_for(guardian).has?(:agree_and_silence)).to eq(false) expect(reviewable.actions_for(guardian).has?(:agree_and_suspend)).to eq(false) end + + context "when flagged as potential_spam" do + before { reviewable.update!(potential_spam: true) } + + it "excludes delete action if the reviewer cannot delete the user" do + post.user.user_stat.update!( + first_post_created_at: 1.year.ago, + post_count: User::MAX_STAFF_DELETE_POST_COUNT + 1 + ) + + expect(reviewable.actions_for(guardian).has?(:delete_user)).to be false + expect(reviewable.actions_for(guardian).has?(:delete_user_block)).to be false + end + + it "includes delete actions if the reviewer can delete the user" do + expect(reviewable.actions_for(guardian).has?(:delete_user)).to be true + expect(reviewable.actions_for(guardian).has?(:delete_user_block)).to be true + end + end end it "agree_and_keep agrees with the flags and keeps the post" do