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.
rebase/4.0.0rc2
ThibG 2020-06-09 10:32:00 +02:00 committed by GitHub
parent 384d64894a
commit 89f40b6c3e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 142 additions and 55 deletions

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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