From a044ddac5b3c0e2012c0e91bfbc07aa256a0684f Mon Sep 17 00:00:00 2001
From: ThibG <thib@sitedethib.com>
Date: Tue, 2 Feb 2021 14:49:57 +0100
Subject: 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>
---
 spec/lib/activitypub/activity/move_spec.rb | 99 +++++++++++++++++++++++-------
 1 file changed, 76 insertions(+), 23 deletions(-)

(limited to 'spec/lib/activitypub/activity')

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