From d658af7ff8893d332f1509082a9d919f70af99af Mon Sep 17 00:00:00 2001 From: ThibG Date: Wed, 15 Jul 2020 21:08:19 +0200 Subject: [PATCH] Fix removing allowed domains being done synchronously (#14302) * Fix removing allowed domains being done synchronously * Add tests --- app/services/after_unallow_domain_service.rb | 9 +++ app/services/unallow_domain_service.rb | 5 +- app/workers/after_unallow_domain_worker.rb | 9 +++ spec/services/unallow_domain_service_spec.rb | 64 ++++++++++++++++++++ 4 files changed, 84 insertions(+), 3 deletions(-) create mode 100644 app/services/after_unallow_domain_service.rb create mode 100644 app/workers/after_unallow_domain_worker.rb create mode 100644 spec/services/unallow_domain_service_spec.rb diff --git a/app/services/after_unallow_domain_service.rb b/app/services/after_unallow_domain_service.rb new file mode 100644 index 000000000..ccd0b8ae9 --- /dev/null +++ b/app/services/after_unallow_domain_service.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AfterUnallowDomainService < BaseService + def call(domain) + Account.where(domain: domain).find_each do |account| + SuspendAccountService.new.call(account, reserve_username: false) + end + end +end diff --git a/app/services/unallow_domain_service.rb b/app/services/unallow_domain_service.rb index 870c79951..fc5260761 100644 --- a/app/services/unallow_domain_service.rb +++ b/app/services/unallow_domain_service.rb @@ -12,8 +12,7 @@ class UnallowDomainService < BaseService private def suspend_accounts!(domain) - Account.where(domain: domain).find_each do |account| - SuspendAccountService.new.call(account, reserve_username: false) - end + Account.where(domain: domain).in_batches.update_all(suspended_at: Time.now.utc) + AfterUnallowDomainWorker.perform_async(domain) end end diff --git a/app/workers/after_unallow_domain_worker.rb b/app/workers/after_unallow_domain_worker.rb new file mode 100644 index 000000000..46049cbb2 --- /dev/null +++ b/app/workers/after_unallow_domain_worker.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AfterUnallowDomainWorker + include Sidekiq::Worker + + def perform(domain) + AfterUnallowDomainService.new.call(domain) + end +end diff --git a/spec/services/unallow_domain_service_spec.rb b/spec/services/unallow_domain_service_spec.rb new file mode 100644 index 000000000..559e152fb --- /dev/null +++ b/spec/services/unallow_domain_service_spec.rb @@ -0,0 +1,64 @@ +require 'rails_helper' + +RSpec.describe UnallowDomainService, 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')) } + let!(:already_banned_account) { Fabricate(:account, username: 'badguy', domain: 'evil.org', suspended: true, silenced: true) } + let!(:domain_allow) { Fabricate(:domain_allow, domain: 'evil.org') } + + subject { UnallowDomainService.new } + + context 'in limited federation mode' do + before do + allow(subject).to receive(:whitelist_mode?).and_return(true) + end + + describe '#call' do + before do + subject.call(domain_allow) + end + + it 'removes the allowed domain' do + expect(DomainAllow.allowed?('evil.org')).to be false + end + + it 'removes remote accounts from that domain' do + expect(Account.where(domain: 'evil.org').exists?).to be false + end + + it 'removes the remote accounts\'s statuses and media attachments' do + expect { bad_status1.reload }.to raise_exception ActiveRecord::RecordNotFound + expect { bad_status2.reload }.to raise_exception ActiveRecord::RecordNotFound + expect { bad_attachment.reload }.to raise_exception ActiveRecord::RecordNotFound + end + end + end + + context 'without limited federation mode' do + before do + allow(subject).to receive(:whitelist_mode?).and_return(false) + end + + describe '#call' do + before do + subject.call(domain_allow) + end + + it 'removes the allowed domain' do + expect(DomainAllow.allowed?('evil.org')).to be false + end + + it 'does not remove accounts from that domain' do + expect(Account.where(domain: 'evil.org').exists?).to be true + end + + it 'removes the remote accounts\'s statuses and media attachments' do + expect { bad_status1.reload }.to_not raise_exception ActiveRecord::RecordNotFound + expect { bad_status2.reload }.to_not raise_exception ActiveRecord::RecordNotFound + expect { bad_attachment.reload }.to_not raise_exception ActiveRecord::RecordNotFound + end + end + end +end