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
This commit is contained in:
ThibG 2019-11-07 08:05:07 +01:00 committed by Eugen Rochko
parent 699427777a
commit 66c1fe0495
5 changed files with 126 additions and 6 deletions

View File

@ -8,7 +8,7 @@ class FollowService < BaseService
# @param [Account] source_account From which to follow # @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 [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 # @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? reblogs = true if reblogs.nil?
target_account = ResolveAccountService.new.call(target_account, skip_webfinger: true) target_account = ResolveAccountService.new.call(target_account, skip_webfinger: true)
@ -30,7 +30,7 @@ class FollowService < BaseService
ActivityTracker.increment('activity:interactions') 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) request_follow(source_account, target_account, reblogs: reblogs)
elsif target_account.local? elsif target_account.local?
direct_follow(source_account, target_account, reblogs: reblogs) direct_follow(source_account, target_account, reblogs: reblogs)

View File

@ -7,7 +7,7 @@ class MoveWorker
@source_account = Account.find(source_account_id) @source_account = Account.find(source_account_id)
@target_account = Account.find(target_account_id) @target_account = Account.find(target_account_id)
if @target_account.local? if @target_account.local? && @source_account.local?
rewrite_follows! rewrite_follows!
else else
queue_follow_unfollows! queue_follow_unfollows!
@ -21,13 +21,17 @@ class MoveWorker
def rewrite_follows! def rewrite_follows!
@source_account.passive_relationships @source_account.passive_relationships
.where(account: Account.local) .where(account: Account.local)
.where.not(account: @target_account.followers.local)
.where.not(account_id: @target_account.id)
.in_batches .in_batches
.update_all(target_account_id: @target_account.id) .update_all(target_account_id: @target_account.id)
end end
def queue_follow_unfollows! def queue_follow_unfollows!
bypass_locked = @target_account.local?
@source_account.followers.local.select(:id).find_in_batches do |accounts| @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 end
end end

View File

@ -5,12 +5,15 @@ class UnfollowFollowWorker
sidekiq_options queue: 'pull' 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) follower_account = Account.find(follower_account_id)
old_target_account = Account.find(old_target_account_id) old_target_account = Account.find(old_target_account_id)
new_target_account = Account.find(new_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) UnfollowService.new.call(follower_account, old_target_account, skip_unmerge: true)
rescue ActiveRecord::RecordNotFound, Mastodon::NotPermittedError rescue ActiveRecord::RecordNotFound, Mastodon::NotPermittedError
true true

View File

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

View File

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