diff options
| -rw-r--r-- | app/lib/activitypub/activity/move.rb | 11 | ||||
| -rw-r--r-- | app/models/account_migration.rb | 14 | ||||
| -rw-r--r-- | spec/controllers/settings/migrations_controller_spec.rb | 37 | ||||
| -rw-r--r-- | spec/lib/activitypub/activity/move_spec.rb | 99 |
4 files changed, 127 insertions, 34 deletions
diff --git a/app/lib/activitypub/activity/move.rb b/app/lib/activitypub/activity/move.rb index 7e073f64d..8576ceccd 100644 --- a/app/lib/activitypub/activity/move.rb +++ b/app/lib/activitypub/activity/move.rb @@ -4,9 +4,8 @@ class ActivityPub::Activity::Move < ActivityPub::Activity PROCESSING_COOLDOWN = 7.days.seconds def perform - return if origin_account.uri != object_uri || processed? - - mark_as_processing! + return if origin_account.uri != object_uri + return unless mark_as_processing! target_account = ActivityPub::FetchRemoteAccountService.new.call(target_uri) @@ -35,12 +34,8 @@ class ActivityPub::Activity::Move < ActivityPub::Activity value_or_id(@json['target']) end - def processed? - redis.exists?("move_in_progress:#{@account.id}") - end - 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 def unmark_as_processing! diff --git a/app/models/account_migration.rb b/app/models/account_migration.rb index 4fae98ed7..ded32c9c6 100644 --- a/app/models/account_migration.rb +++ b/app/models/account_migration.rb @@ -14,6 +14,8 @@ # class AccountMigration < ApplicationRecord + include Redisable + COOLDOWN_PERIOD = 30.days.freeze belongs_to :account @@ -39,7 +41,13 @@ class AccountMigration < ApplicationRecord return false unless errors.empty? - save + RedisLock.acquire(lock_options) do |lock| + if lock.acquired? + save + else + raise Mastodon::RaceConditionError + end + end end def cooldown_at @@ -75,4 +83,8 @@ class AccountMigration < ApplicationRecord def validate_migration_cooldown errors.add(:base, I18n.t('migrations.errors.on_cooldown')) if account.migrations.within_cooldown.exists? end + + def lock_options + { redis: redis, key: "account_migration:#{account.id}" } + end end diff --git a/spec/controllers/settings/migrations_controller_spec.rb b/spec/controllers/settings/migrations_controller_spec.rb index 36e4ba86e..048d9de8d 100644 --- a/spec/controllers/settings/migrations_controller_spec.rb +++ b/spec/controllers/settings/migrations_controller_spec.rb @@ -51,7 +51,7 @@ describe Settings::MigrationsController do it_behaves_like 'authenticate user' 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' } } } let(:user) { Fabricate(:user, password: '12345678') } @@ -67,12 +67,45 @@ describe Settings::MigrationsController do end end - context 'when acct is a current account' do + context 'when acct is the current account' do let(:acct) { user.account } 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 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 diff --git a/spec/lib/activitypub/activity/move_spec.rb b/spec/lib/activitypub/activity/move_spec.rb index 3574f273a..2d1d276c5 100644 --- a/spec/lib/activitypub/activity/move_spec.rb +++ b/spec/lib/activitypub/activity/move_spec.rb @@ -1,23 +1,11 @@ require 'rails_helper' RSpec.describe ActivityPub::Activity::Move do - let(:follower) { Fabricate(:account) } - let(:old_account) { Fabricate(:account) } - let(:new_account) { Fabricate(:account) } - - before do - 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(:follower) { 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, 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] } + let(:returned_account) { new_account } let(:json) do { @@ -30,6 +18,17 @@ RSpec.describe ActivityPub::Activity::Move do }.with_indifferent_access 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 subject { described_class.new(json, old_account) } @@ -37,16 +36,70 @@ RSpec.describe ActivityPub::Activity::Move do subject.perform end - it 'sets moved account on old account' do - expect(old_account.reload.moved_to_account_id).to eq new_account.id + context 'when all conditions are met' do + it 'sets moved account on old account' do + expect(old_account.reload.moved_to_account_id).to eq new_account.id + end + + it 'makes followers unfollow old account' do + expect(follower.following?(old_account)).to be false + end + + it 'makes followers follow-request the new account' do + expect(follower.requested?(new_account)).to be true + end end - it 'makes followers unfollow old account' do - expect(follower.following?(old_account)).to be false + 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 - it 'makes followers follow-request the new account' do - expect(follower.requested?(new_account)).to be true + 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 |