From 18fb01ef7ca3829b07bb8ea101bdd073da72cfbc Mon Sep 17 00:00:00 2001 From: Claire Date: Thu, 5 Jan 2023 13:47:21 +0100 Subject: [PATCH] Fix possible race conditions when suspending/unsuspending accounts (#22363) * Fix possible race conditions when suspending/unsuspending accounts * Fix tests Tests were assuming SuspensionWorker and UnsuspensionWorker would do the suspending/unsuspending themselves, but this has changed. --- app/services/suspend_account_service.rb | 9 ++++----- app/services/unsuspend_account_service.rb | 8 +++----- spec/services/suspend_account_service_spec.rb | 6 ++++-- spec/services/unsuspend_account_service_spec.rb | 14 +++++++------- 4 files changed, 18 insertions(+), 19 deletions(-) diff --git a/app/services/suspend_account_service.rb b/app/services/suspend_account_service.rb index 6856c2c51..211544fea 100644 --- a/app/services/suspend_account_service.rb +++ b/app/services/suspend_account_service.rb @@ -3,10 +3,13 @@ class SuspendAccountService < BaseService include Payloadable + # Carry out the suspension of a recently-suspended account + # @param [Account] account Account to suspend def call(account) + return unless account.suspended? + @account = account - suspend! reject_remote_follows! distribute_update_actor! unmerge_from_home_timelines! @@ -16,10 +19,6 @@ class SuspendAccountService < BaseService private - def suspend! - @account.suspend! unless @account.suspended? - end - def reject_remote_follows! return if @account.local? || !@account.activitypub? diff --git a/app/services/unsuspend_account_service.rb b/app/services/unsuspend_account_service.rb index 534203dce..70667308e 100644 --- a/app/services/unsuspend_account_service.rb +++ b/app/services/unsuspend_account_service.rb @@ -2,10 +2,12 @@ class UnsuspendAccountService < BaseService include Payloadable + + # Restores a recently-unsuspended account + # @param [Account] account Account to restore def call(account) @account = account - unsuspend! refresh_remote_account! return if @account.nil? || @account.suspended? @@ -18,10 +20,6 @@ class UnsuspendAccountService < BaseService private - def unsuspend! - @account.unsuspend! if @account.suspended? - end - def refresh_remote_account! return if @account.local? diff --git a/spec/services/suspend_account_service_spec.rb b/spec/services/suspend_account_service_spec.rb index 5d45e4ffd..126b13986 100644 --- a/spec/services/suspend_account_service_spec.rb +++ b/spec/services/suspend_account_service_spec.rb @@ -13,6 +13,8 @@ RSpec.describe SuspendAccountService, type: :service do local_follower.follow!(account) list.accounts << account + + account.suspend! end it "unmerges from local followers' feeds" do @@ -21,8 +23,8 @@ RSpec.describe SuspendAccountService, type: :service do expect(FeedManager.instance).to have_received(:unmerge_from_list).with(account, list) end - it 'marks account as suspended' do - expect { subject }.to change { account.suspended? }.from(false).to(true) + it 'does not change the “suspended” flag' do + expect { subject }.to_not change { account.suspended? } end end diff --git a/spec/services/unsuspend_account_service_spec.rb b/spec/services/unsuspend_account_service_spec.rb index 3ac4cc085..987eb09e2 100644 --- a/spec/services/unsuspend_account_service_spec.rb +++ b/spec/services/unsuspend_account_service_spec.rb @@ -14,7 +14,7 @@ RSpec.describe UnsuspendAccountService, type: :service do local_follower.follow!(account) list.accounts << account - account.suspend!(origin: :local) + account.unsuspend! end end @@ -30,8 +30,8 @@ RSpec.describe UnsuspendAccountService, type: :service do stub_request(:post, 'https://bob.com/inbox').to_return(status: 201) end - it 'marks account as unsuspended' do - expect { subject }.to change { account.suspended? }.from(true).to(false) + it 'does not change the “suspended” flag' do + expect { subject }.to_not change { account.suspended? } end include_examples 'common behavior' do @@ -83,8 +83,8 @@ RSpec.describe UnsuspendAccountService, type: :service do expect(FeedManager.instance).to have_received(:merge_into_list).with(account, list) end - it 'marks account as unsuspended' do - expect { subject }.to change { account.suspended? }.from(true).to(false) + it 'does not change the “suspended” flag' do + expect { subject }.to_not change { account.suspended? } end end @@ -107,8 +107,8 @@ RSpec.describe UnsuspendAccountService, type: :service do expect(FeedManager.instance).to_not have_received(:merge_into_list).with(account, list) end - it 'does not mark the account as unsuspended' do - expect { subject }.not_to change { account.suspended? } + it 'marks account as suspended' do + expect { subject }.to change { account.suspended? }.from(false).to(true) end end