Fix various issues with account migration (#12301)

* Fix being able to follow oneself by moving to an account that was following the old one

* Add specs

* Add spec to catch MoveWorker issue with local followers following both accounts

* Fix move worker breaking when a local account follows both source and target accounts

* Fix migration from remote to local account not sending Undo Follow

* Fix show_reblogs not being preserved for moved account's followers

diff --git a/app/services/follow_service.rb b/app/services/follow_service.rb
index 1941c2e2d8864a352e2de04239391ea2ee5bdee2..dc47804c05bc4b1e6dd385356aa8d43ad807a8ed 100644 (file)
--- a/app/services/follow_service.rb
+++ b/app/services/follow_service.rb
@@ -8,7 +8,7 @@ class FollowService < BaseService
   # @param [Account] source_account From which to follow
   # @param [String, Account] uri User URI to follow in the form of username@domain (or account record)
   # @param [true, false, nil] reblogs Whether or not to show reblogs, defaults to true
-  def call(source_account, target_account, reblogs: nil)
+  def call(source_account, target_account, reblogs: nil, bypass_locked: false)
     reblogs = true if reblogs.nil?
     target_account = ResolveAccountService.new.call(target_account, skip_webfinger: true)
 
@@ -30,7 +30,7 @@ class FollowService < BaseService
 
     ActivityTracker.increment('activity:interactions')
 
-    if target_account.locked? || source_account.silenced? || target_account.activitypub?
+    if (target_account.locked? && !bypass_locked) || source_account.silenced? || target_account.activitypub?
       request_follow(source_account, target_account, reblogs: reblogs)
     elsif target_account.local?
       direct_follow(source_account, target_account, reblogs: reblogs)

diff --git a/app/workers/move_worker.rb b/app/workers/move_worker.rb
index 83f7090eec1198f0d1d9472524304e769171cadb..59573022633e026c0786347cd3c8117871512a95 100644 (file)
--- a/app/workers/move_worker.rb
+++ b/app/workers/move_worker.rb
@@ -7,7 +7,7 @@ class MoveWorker
     @source_account = Account.find(source_account_id)
     @target_account = Account.find(target_account_id)
 
-    if @target_account.local?
+    if @target_account.local? && @source_account.local?
       rewrite_follows!
     else
       queue_follow_unfollows!
@@ -21,13 +21,17 @@ class MoveWorker
   def rewrite_follows!
     @source_account.passive_relationships
                    .where(account: Account.local)
+                   .where.not(account: @target_account.followers.local)
+                   .where.not(account_id: @target_account.id)
                    .in_batches
                    .update_all(target_account_id: @target_account.id)
   end
 
   def queue_follow_unfollows!
+    bypass_locked = @target_account.local?
+
     @source_account.followers.local.select(:id).find_in_batches do |accounts|
-      UnfollowFollowWorker.push_bulk(accounts.map(&:id)) { |follower_id| [follower_id, @source_account.id, @target_account.id] }
+      UnfollowFollowWorker.push_bulk(accounts.map(&:id)) { |follower_id| [follower_id, @source_account.id, @target_account.id, bypass_locked] }
     end
   end
 end

diff --git a/app/workers/unfollow_follow_worker.rb b/app/workers/unfollow_follow_worker.rb
index 95549e107ad70c760ea04d492e71574f8d2bad51..b6e665a41fbd67bceb6ce00b14f3fdfa6cca1a62 100644 (file)
--- a/app/workers/unfollow_follow_worker.rb
+++ b/app/workers/unfollow_follow_worker.rb
@@ -5,12 +5,15 @@ class UnfollowFollowWorker
 
   sidekiq_options queue: 'pull'
 
-  def perform(follower_account_id, old_target_account_id, new_target_account_id)
+  def perform(follower_account_id, old_target_account_id, new_target_account_id, bypass_locked = false)
     follower_account   = Account.find(follower_account_id)
     old_target_account = Account.find(old_target_account_id)
     new_target_account = Account.find(new_target_account_id)
 
-    FollowService.new.call(follower_account, new_target_account)
+    follow = follower_account.active_relationships.find_by(target_account: old_target_account)
+    reblogs = follow&.show_reblogs?
+
+    FollowService.new.call(follower_account, new_target_account, reblogs: reblogs, bypass_locked: bypass_locked)
     UnfollowService.new.call(follower_account, old_target_account, skip_unmerge: true)
   rescue ActiveRecord::RecordNotFound, Mastodon::NotPermittedError
     true

diff --git a/spec/workers/move_worker_spec.rb b/spec/workers/move_worker_spec.rb
new file mode 100644 (file)
index 0000000..b8f4d99
--- /dev/null
+++ b/spec/workers/move_worker_spec.rb
@@ -0,0 +1,63 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe MoveWorker do
+  let(:local_follower)   { Fabricate(:user, email: 'bob@example.com', account: Fabricate(:account, username: 'bob')).account }
+  let(:source_account)   { Fabricate(:account, protocol: :activitypub, domain: 'example.com') }
+  let(:target_account)   { Fabricate(:account, protocol: :activitypub, domain: 'example.com') }
+
+  subject { described_class.new }
+
+  before do
+    local_follower.follow!(source_account)
+  end
+
+  context 'both accounts are distant' do
+    describe 'perform' do
+      it 'calls UnfollowFollowWorker' do
+        allow(UnfollowFollowWorker).to receive(:push_bulk)
+        subject.perform(source_account.id, target_account.id)
+        expect(UnfollowFollowWorker).to have_received(:push_bulk).with([local_follower.id])
+      end
+    end
+  end
+
+  context 'target account is local' do
+    let(:target_account) { Fabricate(:user, email: 'alice@example.com', account: Fabricate(:account, username: 'alice')).account }
+
+    describe 'perform' do
+      it 'calls UnfollowFollowWorker' do
+        allow(UnfollowFollowWorker).to receive(:push_bulk)
+        subject.perform(source_account.id, target_account.id)
+        expect(UnfollowFollowWorker).to have_received(:push_bulk).with([local_follower.id])
+      end
+    end
+  end
+
+  context 'both target and source accounts are local' do
+    let(:target_account) { Fabricate(:user, email: 'alice@example.com', account: Fabricate(:account, username: 'alice')).account }
+    let(:source_account) { Fabricate(:user, email: 'alice_@example.com', account: Fabricate(:account, username: 'alice_')).account }
+
+    describe 'perform' do
+      it 'calls makes local followers follow the target account' do
+        subject.perform(source_account.id, target_account.id)
+        expect(local_follower.following?(target_account)).to be true
+      end
+
+      it 'does not fail when a local user is already following both accounts' do
+        double_follower = Fabricate(:user, email: 'eve@example.com', account: Fabricate(:account, username: 'eve')).account
+        double_follower.follow!(source_account)
+        double_follower.follow!(target_account)
+        subject.perform(source_account.id, target_account.id)
+        expect(local_follower.following?(target_account)).to be true
+      end
+
+      it 'does not allow the moved account to follow themselves' do
+        source_account.follow!(target_account)
+        subject.perform(source_account.id, target_account.id)
+        expect(target_account.following?(target_account)).to be false
+      end
+    end
+  end
+end

diff --git a/spec/workers/unfollow_follow_worker_spec.rb b/spec/workers/unfollow_follow_worker_spec.rb
new file mode 100644 (file)
index 0000000..5052c56
--- /dev/null
+++ b/spec/workers/unfollow_follow_worker_spec.rb
@@ -0,0 +1,50 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe UnfollowFollowWorker do
+  let(:local_follower)   { Fabricate(:user, email: 'bob@example.com', account: Fabricate(:account, username: 'bob')).account }
+  let(:source_account)   { Fabricate(:account) }
+  let(:target_account)   { Fabricate(:account) }
+  let(:show_reblogs)     { true }
+
+  subject { described_class.new }
+
+  before do
+    local_follower.follow!(source_account, reblogs: show_reblogs)
+  end
+
+  context 'when show_reblogs is true' do
+    let(:show_reblogs) { true }
+
+    describe 'perform' do
+      it 'unfollows source account and follows target account' do
+        subject.perform(local_follower.id, source_account.id, target_account.id)
+        expect(local_follower.following?(source_account)).to be false
+        expect(local_follower.following?(target_account)).to be true
+      end
+
+      it 'preserves show_reblogs' do
+        subject.perform(local_follower.id, source_account.id, target_account.id)
+        expect(Follow.find_by(account: local_follower, target_account: target_account).show_reblogs?).to be show_reblogs
+      end
+    end
+  end
+
+  context 'when show_reblogs is false' do
+    let(:show_reblogs) { false }
+
+    describe 'perform' do
+      it 'unfollows source account and follows target account' do
+        subject.perform(local_follower.id, source_account.id, target_account.id)
+        expect(local_follower.following?(source_account)).to be false
+        expect(local_follower.following?(target_account)).to be true
+      end
+
+      it 'preserves show_reblogs' do
+        subject.perform(local_follower.id, source_account.id, target_account.id)
+        expect(Follow.find_by(account: local_follower, target_account: target_account).show_reblogs?).to be show_reblogs
+      end
+    end
+  end
+end