From 89f40b6c3ec525b09d02f21e9b45276084167d8d Mon Sep 17 00:00:00 2001 From: ThibG Date: Tue, 9 Jun 2020 10:32:00 +0200 Subject: [PATCH] Make domain block/silence/reject-media code more robust (#13424) * Split media cleanup from reject-media domain blocks to its own service * Slightly improve ClearDomainMediaService error handling * Lower DomainClearMediaWorker to lowest-priority queue * Do not catch ActiveRecord::RecordNotFound in domain block workers * Fix DomainBlockWorker spec labels * Add some specs * Change domain blocks to immediately mark accounts as suspended Rather than doing so sequentially, account after account, while cleaning their data. This doesn't change much about the time the block takes to complete, but it immediately prevents interaction with the blocked domain, while up to now, it would only be guaranteed when the process ends. --- app/services/block_domain_service.rb | 53 +------------- app/services/clear_domain_media_service.rb | 70 +++++++++++++++++++ app/workers/domain_block_worker.rb | 7 +- app/workers/domain_clear_media_worker.rb | 14 ++++ .../clear_domain_media_service_spec.rb | 23 ++++++ spec/workers/domain_block_worker_spec.rb | 4 +- .../workers/domain_clear_media_worker_spec.rb | 26 +++++++ 7 files changed, 142 insertions(+), 55 deletions(-) create mode 100644 app/services/clear_domain_media_service.rb create mode 100644 app/workers/domain_clear_media_worker.rb create mode 100644 spec/services/clear_domain_media_service_spec.rb create mode 100644 spec/workers/domain_clear_media_worker_spec.rb diff --git a/app/services/block_domain_service.rb b/app/services/block_domain_service.rb index 9f086067476..dc23ef8d8a8 100644 --- a/app/services/block_domain_service.rb +++ b/app/services/block_domain_service.rb @@ -26,59 +26,20 @@ class BlockDomainService < BaseService suspend_accounts! end - clear_media! if domain_block.reject_media? - end - - def invalidate_association_caches! - # Normally, associated models of a status are immutable (except for accounts) - # so they are aggressively cached. After updating the media attachments to no - # longer point to a local file, we need to clear the cache to make those - # changes appear in the API and UI - @affected_status_ids.each { |id| Rails.cache.delete_matched("statuses/#{id}-*") } + DomainClearMediaWorker.perform_async(domain_block.id) if domain_block.reject_media? end def silence_accounts! blocked_domain_accounts.without_silenced.in_batches.update_all(silenced_at: @domain_block.created_at) end - def clear_media! - @affected_status_ids = [] - - clear_account_images! - clear_account_attachments! - clear_emojos! - - invalidate_association_caches! - end - def suspend_accounts! - blocked_domain_accounts.without_suspended.reorder(nil).find_each do |account| + blocked_domain_accounts.without_suspended.in_batches.update_all(suspended_at: @domain_block.created_at) + blocked_domain_accounts.where(suspended_at: @domain_block.created_at).reorder(nil).find_each do |account| SuspendAccountService.new.call(account, reserve_username: true, suspended_at: @domain_block.created_at) end end - def clear_account_images! - blocked_domain_accounts.reorder(nil).find_each do |account| - account.avatar.destroy if account.avatar.exists? - account.header.destroy if account.header.exists? - account.save - end - end - - def clear_account_attachments! - media_from_blocked_domain.reorder(nil).find_each do |attachment| - @affected_status_ids << attachment.status_id if attachment.status_id.present? - - attachment.file.destroy if attachment.file.exists? - attachment.type = :unknown - attachment.save - end - end - - def clear_emojos! - emojis_from_blocked_domains.destroy_all - end - def blocked_domain domain_block.domain end @@ -86,12 +47,4 @@ class BlockDomainService < BaseService def blocked_domain_accounts Account.by_domain_and_subdomains(blocked_domain) end - - def media_from_blocked_domain - MediaAttachment.joins(:account).merge(blocked_domain_accounts).reorder(nil) - end - - def emojis_from_blocked_domains - CustomEmoji.by_domain_and_subdomains(blocked_domain) - end end diff --git a/app/services/clear_domain_media_service.rb b/app/services/clear_domain_media_service.rb new file mode 100644 index 00000000000..704cfb71a6e --- /dev/null +++ b/app/services/clear_domain_media_service.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +class ClearDomainMediaService < BaseService + attr_reader :domain_block + + def call(domain_block) + @domain_block = domain_block + clear_media! if domain_block.reject_media? + end + + private + + def invalidate_association_caches! + # Normally, associated models of a status are immutable (except for accounts) + # so they are aggressively cached. After updating the media attachments to no + # longer point to a local file, we need to clear the cache to make those + # changes appear in the API and UI + @affected_status_ids.each { |id| Rails.cache.delete_matched("statuses/#{id}-*") } + end + + def clear_media! + @affected_status_ids = [] + + begin + clear_account_images! + clear_account_attachments! + clear_emojos! + ensure + invalidate_association_caches! + end + end + + def clear_account_images! + blocked_domain_accounts.reorder(nil).find_each do |account| + account.avatar.destroy if account.avatar&.exists? + account.header.destroy if account.header&.exists? + account.save + end + end + + def clear_account_attachments! + media_from_blocked_domain.reorder(nil).find_each do |attachment| + @affected_status_ids << attachment.status_id if attachment.status_id.present? + + attachment.file.destroy if attachment.file&.exists? + attachment.type = :unknown + attachment.save + end + end + + def clear_emojos! + emojis_from_blocked_domains.destroy_all + end + + def blocked_domain + domain_block.domain + end + + def blocked_domain_accounts + Account.by_domain_and_subdomains(blocked_domain) + end + + def media_from_blocked_domain + MediaAttachment.joins(:account).merge(blocked_domain_accounts).reorder(nil) + end + + def emojis_from_blocked_domains + CustomEmoji.by_domain_and_subdomains(blocked_domain) + end +end diff --git a/app/workers/domain_block_worker.rb b/app/workers/domain_block_worker.rb index 35518d6b5e8..3c601cd8321 100644 --- a/app/workers/domain_block_worker.rb +++ b/app/workers/domain_block_worker.rb @@ -4,8 +4,9 @@ class DomainBlockWorker include Sidekiq::Worker def perform(domain_block_id, update = false) - BlockDomainService.new.call(DomainBlock.find(domain_block_id), update) - rescue ActiveRecord::RecordNotFound - true + domain_block = DomainBlock.find_by(id: domain_block_id) + return true if domain_block.nil? + + BlockDomainService.new.call(domain_block, update) end end diff --git a/app/workers/domain_clear_media_worker.rb b/app/workers/domain_clear_media_worker.rb new file mode 100644 index 00000000000..971934a0882 --- /dev/null +++ b/app/workers/domain_clear_media_worker.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class DomainClearMediaWorker + include Sidekiq::Worker + + sidekiq_options queue: 'pull' + + def perform(domain_block_id) + domain_block = DomainBlock.find_by(id: domain_block_id) + return true if domain_block.nil? + + ClearDomainMediaService.new.call(domain_block) + end +end diff --git a/spec/services/clear_domain_media_service_spec.rb b/spec/services/clear_domain_media_service_spec.rb new file mode 100644 index 00000000000..8e58c603977 --- /dev/null +++ b/spec/services/clear_domain_media_service_spec.rb @@ -0,0 +1,23 @@ +require 'rails_helper' + +RSpec.describe ClearDomainMediaService, type: :service do + let!(:bad_account) { Fabricate(:account, username: 'badguy666', domain: 'evil.org') } + let!(:bad_status1) { Fabricate(:status, account: bad_account, text: 'You suck') } + let!(:bad_status2) { Fabricate(:status, account: bad_account, text: 'Hahaha') } + let!(:bad_attachment) { Fabricate(:media_attachment, account: bad_account, status: bad_status2, file: attachment_fixture('attachment.jpg')) } + + subject { ClearDomainMediaService.new } + + describe 'for a silence with reject media' do + before do + subject.call(DomainBlock.create!(domain: 'evil.org', severity: :silence, reject_media: true)) + end + + it 'leaves the domains status and attachements, but clears media' do + expect { bad_status1.reload }.not_to raise_error + expect { bad_status2.reload }.not_to raise_error + expect { bad_attachment.reload }.not_to raise_error + expect(bad_attachment.file.exists?).to be false + end + end +end diff --git a/spec/workers/domain_block_worker_spec.rb b/spec/workers/domain_block_worker_spec.rb index 48b3e38c40c..bd8fc4a6206 100644 --- a/spec/workers/domain_block_worker_spec.rb +++ b/spec/workers/domain_block_worker_spec.rb @@ -8,7 +8,7 @@ describe DomainBlockWorker do describe 'perform' do let(:domain_block) { Fabricate(:domain_block) } - it 'returns true for non-existent domain block' do + it 'calls domain block service for relevant domain block' do service = double(call: nil) allow(BlockDomainService).to receive(:new).and_return(service) result = subject.perform(domain_block.id) @@ -17,7 +17,7 @@ describe DomainBlockWorker do expect(service).to have_received(:call).with(domain_block, false) end - it 'calls domain block service for relevant domain block' do + it 'returns true for non-existent domain block' do result = subject.perform('aaa') expect(result).to eq(true) diff --git a/spec/workers/domain_clear_media_worker_spec.rb b/spec/workers/domain_clear_media_worker_spec.rb new file mode 100644 index 00000000000..36251b1ec56 --- /dev/null +++ b/spec/workers/domain_clear_media_worker_spec.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe DomainClearMediaWorker do + subject { described_class.new } + + describe 'perform' do + let(:domain_block) { Fabricate(:domain_block, severity: :silence, reject_media: true) } + + it 'calls domain clear media service for relevant domain block' do + service = double(call: nil) + allow(ClearDomainMediaService).to receive(:new).and_return(service) + result = subject.perform(domain_block.id) + + expect(result).to be_nil + expect(service).to have_received(:call).with(domain_block) + end + + it 'returns true for non-existent domain block' do + result = subject.perform('aaa') + + expect(result).to eq(true) + end + end +end