Fix race conditions on account migration creation (#15597)

* Atomically check for processing lock in Move handler

* Prevent race condition when creating account migrations

Fixes #15595

* Add tests

Co-authored-by: Claire <claire.github-309c@sitedethib.com>
pull/17389/head
ThibG 2021-02-02 14:49:57 +01:00 committed by Claire
parent 4b025cf7e6
commit 78d5bda973
4 changed files with 127 additions and 34 deletions

View File

@ -4,9 +4,8 @@ class ActivityPub::Activity::Move < ActivityPub::Activity
PROCESSING_COOLDOWN = 7.days.seconds PROCESSING_COOLDOWN = 7.days.seconds
def perform def perform
return if origin_account.uri != object_uri || processed? return if origin_account.uri != object_uri
return unless mark_as_processing!
mark_as_processing!
target_account = ActivityPub::FetchRemoteAccountService.new.call(target_uri) target_account = ActivityPub::FetchRemoteAccountService.new.call(target_uri)
@ -35,12 +34,8 @@ class ActivityPub::Activity::Move < ActivityPub::Activity
value_or_id(@json['target']) value_or_id(@json['target'])
end end
def processed?
redis.exists?("move_in_progress:#{@account.id}")
end
def mark_as_processing! def mark_as_processing!
redis.setex("move_in_progress:#{@account.id}", PROCESSING_COOLDOWN, true) redis.set("move_in_progress:#{@account.id}", true, nx: true, ex: PROCESSING_COOLDOWN)
end end
def unmark_as_processing! def unmark_as_processing!

View File

@ -14,6 +14,8 @@
# #
class AccountMigration < ApplicationRecord class AccountMigration < ApplicationRecord
include Redisable
COOLDOWN_PERIOD = 30.days.freeze COOLDOWN_PERIOD = 30.days.freeze
belongs_to :account belongs_to :account
@ -39,7 +41,13 @@ class AccountMigration < ApplicationRecord
return false unless errors.empty? return false unless errors.empty?
RedisLock.acquire(lock_options) do |lock|
if lock.acquired?
save save
else
raise Mastodon::RaceConditionError
end
end
end end
def cooldown_at def cooldown_at
@ -75,4 +83,8 @@ class AccountMigration < ApplicationRecord
def validate_migration_cooldown def validate_migration_cooldown
errors.add(:base, I18n.t('migrations.errors.on_cooldown')) if account.migrations.within_cooldown.exists? errors.add(:base, I18n.t('migrations.errors.on_cooldown')) if account.migrations.within_cooldown.exists?
end end
def lock_options
{ redis: redis, key: "account_migration:#{account.id}" }
end
end end

View File

@ -51,7 +51,7 @@ describe Settings::MigrationsController do
it_behaves_like 'authenticate user' it_behaves_like 'authenticate user'
end end
context 'when user is sign in' do context 'when user is signed in' do
subject { post :create, params: { account_migration: { acct: acct, current_password: '12345678' } } } subject { post :create, params: { account_migration: { acct: acct, current_password: '12345678' } } }
let(:user) { Fabricate(:user, password: '12345678') } let(:user) { Fabricate(:user, password: '12345678') }
@ -67,12 +67,45 @@ describe Settings::MigrationsController do
end end
end end
context 'when acct is a current account' do context 'when acct is the current account' do
let(:acct) { user.account } let(:acct) { user.account }
it 'renders show' do it 'renders show' do
is_expected.to render_template :show is_expected.to render_template :show
end end
it 'does not update the moved account' do
expect(user.account.reload.moved_to_account_id).to be_nil
end
end
context 'when target account does not reference the account being moved from' do
let(:acct) { Fabricate(:account, also_known_as: []) }
it 'renders show' do
is_expected.to render_template :show
end
it 'does not update the moved account' do
expect(user.account.reload.moved_to_account_id).to be_nil
end
end
context 'when a recent migration already exists ' do
let(:acct) { Fabricate(:account, also_known_as: [ActivityPub::TagManager.instance.uri_for(user.account)]) }
before do
moved_to = Fabricate(:account, also_known_as: [ActivityPub::TagManager.instance.uri_for(user.account)])
user.account.migrations.create!(acct: moved_to.acct)
end
it 'renders show' do
is_expected.to render_template :show
end
it 'does not update the moved account' do
expect(user.account.reload.moved_to_account_id).to be_nil
end
end end
end end
end end

View File

@ -2,22 +2,10 @@ require 'rails_helper'
RSpec.describe ActivityPub::Activity::Move do RSpec.describe ActivityPub::Activity::Move do
let(:follower) { Fabricate(:account) } let(:follower) { Fabricate(:account) }
let(:old_account) { Fabricate(:account) } let(:old_account) { Fabricate(:account, uri: 'https://example.org/alice', domain: 'example.org', protocol: :activitypub, inbox_url: 'https://example.org/inbox') }
let(:new_account) { Fabricate(:account) } let(:new_account) { Fabricate(:account, uri: 'https://example.com/alice', domain: 'example.com', protocol: :activitypub, inbox_url: 'https://example.com/inbox', also_known_as: also_known_as) }
let(:also_known_as) { [old_account.uri] }
before do let(:returned_account) { new_account }
follower.follow!(old_account)
old_account.update!(uri: 'https://example.org/alice', domain: 'example.org', protocol: :activitypub, inbox_url: 'https://example.org/inbox')
new_account.update!(uri: 'https://example.com/alice', domain: 'example.com', protocol: :activitypub, inbox_url: 'https://example.com/inbox', also_known_as: [old_account.uri])
stub_request(:post, 'https://example.org/inbox').to_return(status: 200)
stub_request(:post, 'https://example.com/inbox').to_return(status: 200)
service_stub = double
allow(ActivityPub::FetchRemoteAccountService).to receive(:new).and_return(service_stub)
allow(service_stub).to receive(:call).and_return(new_account)
end
let(:json) do let(:json) do
{ {
@ -30,6 +18,17 @@ RSpec.describe ActivityPub::Activity::Move do
}.with_indifferent_access }.with_indifferent_access
end end
before do
follower.follow!(old_account)
stub_request(:post, old_account.inbox_url).to_return(status: 200)
stub_request(:post, new_account.inbox_url).to_return(status: 200)
service_stub = double
allow(ActivityPub::FetchRemoteAccountService).to receive(:new).and_return(service_stub)
allow(service_stub).to receive(:call).and_return(returned_account)
end
describe '#perform' do describe '#perform' do
subject { described_class.new(json, old_account) } subject { described_class.new(json, old_account) }
@ -37,6 +36,7 @@ RSpec.describe ActivityPub::Activity::Move do
subject.perform subject.perform
end end
context 'when all conditions are met' do
it 'sets moved account on old account' do it 'sets moved account on old account' do
expect(old_account.reload.moved_to_account_id).to eq new_account.id expect(old_account.reload.moved_to_account_id).to eq new_account.id
end end
@ -49,4 +49,57 @@ RSpec.describe ActivityPub::Activity::Move do
expect(follower.requested?(new_account)).to be true expect(follower.requested?(new_account)).to be true
end end
end end
context "when the new account can't be resolved" do
let(:returned_account) { nil }
it 'does not set moved account on old account' do
expect(old_account.reload.moved_to_account_id).to be_nil
end
it 'does not make followers unfollow old account' do
expect(follower.following?(old_account)).to be true
end
it 'does not make followers follow-request the new account' do
expect(follower.requested?(new_account)).to be false
end
end
context 'when the new account does not references the old account' do
let(:also_known_as) { [] }
it 'does not set moved account on old account' do
expect(old_account.reload.moved_to_account_id).to be_nil
end
it 'does not make followers unfollow old account' do
expect(follower.following?(old_account)).to be true
end
it 'does not make followers follow-request the new account' do
expect(follower.requested?(new_account)).to be false
end
end
context 'when a Move has been recently processed' do
around do |example|
Redis.current.set("move_in_progress:#{old_account.id}", true, nx: true, ex: 7.days.seconds)
example.run
Redis.current.del("move_in_progress:#{old_account.id}")
end
it 'does not set moved account on old account' do
expect(old_account.reload.moved_to_account_id).to be_nil
end
it 'does not make followers unfollow old account' do
expect(follower.following?(old_account)).to be true
end
it 'does not make followers follow-request the new account' do
expect(follower.requested?(new_account)).to be false
end
end
end
end end