diff --git a/app/models/account.rb b/app/models/account.rb index 6f3a9e8883..ca2173e4fe 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -120,6 +120,7 @@ class Account < ApplicationRecord scope :by_domain_and_subdomains, ->(domain) { where(domain: domain).or(where(arel_table[:domain].matches("%.#{domain}"))) } scope :not_excluded_by_account, ->(account) { where.not(id: account.excluded_from_timeline_account_ids) } scope :not_domain_blocked_by_account, ->(account) { where(arel_table[:domain].eq(nil).or(arel_table[:domain].not_in(account.excluded_from_timeline_domains))) } + scope :with_username, ->(value) { value.is_a?(Array) ? where(arel_table[:username].lower.in(value.map { |x| x.to_s.downcase })) : where(arel_table[:username].lower.eq(value.to_s.downcase)) } delegate :email, :unconfirmed_email, diff --git a/app/services/activitypub/synchronize_followers_service.rb b/app/services/activitypub/synchronize_followers_service.rb index 0abca38f3a..3a1db51dd0 100644 --- a/app/services/activitypub/synchronize_followers_service.rb +++ b/app/services/activitypub/synchronize_followers_service.rb @@ -4,32 +4,46 @@ class ActivityPub::SynchronizeFollowersService < BaseService include JsonLdHelper include Payloadable + MAX_COLLECTION_PAGES = 10 + def call(account, partial_collection_url) @account = account + @expected_followers_ids = [] - items = collection_items(partial_collection_url) - return if items.nil? - - # There could be unresolved accounts (hence the call to .compact) but this - # should never happen in practice, since in almost all cases we keep an - # Account record, and should we not do that, we should have sent a Delete. - # In any case there is not much we can do if that occurs. - @expected_followers = items.filter_map { |uri| ActivityPub::TagManager.instance.uri_to_resource(uri, Account) } + return unless process_collection!(partial_collection_url) remove_unexpected_local_followers! - handle_unexpected_outgoing_follows! end private + def process_page!(items) + page_expected_followers = extract_local_followers(items) + @expected_followers_ids.concat(page_expected_followers.pluck(:id)) + + handle_unexpected_outgoing_follows!(page_expected_followers) + end + + def extract_local_followers(items) + # There could be unresolved accounts (hence the call to .filter_map) but this + # should never happen in practice, since in almost all cases we keep an + # Account record, and should we not do that, we should have sent a Delete. + # In any case there is not much we can do if that occurs. + + # TODO: this will need changes when switching to numeric IDs + + usernames = items.filter_map { |uri| ActivityPub::TagManager.instance.uri_to_local_id(uri, :username)&.downcase } + Account.local.with_username(usernames) + end + def remove_unexpected_local_followers! - @account.followers.local.where.not(id: @expected_followers.map(&:id)).each do |unexpected_follower| + @account.followers.local.where.not(id: @expected_followers_ids).reorder(nil).find_each do |unexpected_follower| UnfollowService.new.call(unexpected_follower, @account) end end - def handle_unexpected_outgoing_follows! - @expected_followers.each do |expected_follower| + def handle_unexpected_outgoing_follows!(expected_followers) + expected_followers.each do |expected_follower| next if expected_follower.following?(@account) if expected_follower.requested?(@account) @@ -50,21 +64,33 @@ class ActivityPub::SynchronizeFollowersService < BaseService Oj.dump(serialize_payload(follow, ActivityPub::UndoFollowSerializer)) end - def collection_items(collection_or_uri) - collection = fetch_collection(collection_or_uri) - return unless collection.is_a?(Hash) + # Only returns true if the whole collection has been processed + def process_collection!(collection_uri, max_pages: MAX_COLLECTION_PAGES) + collection = fetch_collection(collection_uri) + return false unless collection.is_a?(Hash) collection = fetch_collection(collection['first']) if collection['first'].present? - return unless collection.is_a?(Hash) - # Abort if we'd have to paginate through more than one page of followers - return if collection['next'].present? + while collection.is_a?(Hash) + process_page!(as_array(collection_page_items(collection))) + max_pages -= 1 + + return true if collection['next'].blank? # We reached the end of the collection + return false if max_pages <= 0 # We reached our pages limit + + collection = fetch_collection(collection['next']) + end + + false + end + + def collection_page_items(collection) case collection['type'] when 'Collection', 'CollectionPage' - as_array(collection['items']) + collection['items'] when 'OrderedCollection', 'OrderedCollectionPage' - as_array(collection['orderedItems']) + collection['orderedItems'] end end diff --git a/spec/services/activitypub/synchronize_followers_service_spec.rb b/spec/services/activitypub/synchronize_followers_service_spec.rb index 8515b1fd4a..70d0501ef8 100644 --- a/spec/services/activitypub/synchronize_followers_service_spec.rb +++ b/spec/services/activitypub/synchronize_followers_service_spec.rb @@ -6,7 +6,7 @@ RSpec.describe ActivityPub::SynchronizeFollowersService, type: :service do let(:bob) { Fabricate(:account, username: 'bob') } let(:eve) { Fabricate(:account, username: 'eve') } let(:mallory) { Fabricate(:account, username: 'mallory') } - let(:collection_uri) { 'http://example.com/partial-followers' } + let(:collection_uri) { 'https://example.com/partial-followers' } let(:items) do [ @@ -104,7 +104,76 @@ RSpec.describe ActivityPub::SynchronizeFollowersService, type: :service do it_behaves_like 'synchronizes followers' end - context 'when the endpoint is a paginated Collection of actor URIs with a next page' do + context 'when the endpoint is a paginated Collection of actor URIs split across multiple pages' do + before do + stub_request(:get, 'https://example.com/partial-followers') + .to_return(status: 200, headers: { 'Content-Type': 'application/activity+json' }, body: Oj.dump({ + '@context': 'https://www.w3.org/ns/activitystreams', + type: 'Collection', + id: 'https://example.com/partial-followers', + first: 'https://example.com/partial-followers/1', + })) + + stub_request(:get, 'https://example.com/partial-followers/1') + .to_return(status: 200, headers: { 'Content-Type': 'application/activity+json' }, body: Oj.dump({ + '@context': 'https://www.w3.org/ns/activitystreams', + type: 'CollectionPage', + id: 'https://example.com/partial-followers/1', + partOf: 'https://example.com/partial-followers', + next: 'https://example.com/partial-followers/2', + items: [alice, eve].map { |account| ActivityPub::TagManager.instance.uri_for(account) }, + })) + + stub_request(:get, 'https://example.com/partial-followers/2') + .to_return(status: 200, headers: { 'Content-Type': 'application/activity+json' }, body: Oj.dump({ + '@context': 'https://www.w3.org/ns/activitystreams', + type: 'CollectionPage', + id: 'https://example.com/partial-followers/2', + partOf: 'https://example.com/partial-followers', + items: ActivityPub::TagManager.instance.uri_for(mallory), + })) + end + + it_behaves_like 'synchronizes followers' + end + + context 'when the endpoint is a paginated Collection of actor URIs split across, but one page errors out' do + before do + stub_request(:get, 'https://example.com/partial-followers') + .to_return(status: 200, headers: { 'Content-Type': 'application/activity+json' }, body: Oj.dump({ + '@context': 'https://www.w3.org/ns/activitystreams', + type: 'Collection', + id: 'https://example.com/partial-followers', + first: 'https://example.com/partial-followers/1', + })) + + stub_request(:get, 'https://example.com/partial-followers/1') + .to_return(status: 200, headers: { 'Content-Type': 'application/activity+json' }, body: Oj.dump({ + '@context': 'https://www.w3.org/ns/activitystreams', + type: 'CollectionPage', + id: 'https://example.com/partial-followers/1', + partOf: 'https://example.com/partial-followers', + next: 'https://example.com/partial-followers/2', + items: [mallory].map { |account| ActivityPub::TagManager.instance.uri_for(account) }, + })) + + stub_request(:get, 'https://example.com/partial-followers/2') + .to_return(status: 404) + end + + it 'confirms pending follow request but does not remove extra followers' do + previous_follower_ids = actor.followers.pluck(:id) + + subject.call(actor, collection_uri) + + expect(previous_follower_ids - actor.followers.reload.pluck(:id)) + .to be_empty + expect(mallory) + .to be_following(actor) + end + end + + context 'when the endpoint is a paginated Collection of actor URIs with more pages than we allow' do let(:payload) do { '@context': 'https://www.w3.org/ns/activitystreams', @@ -120,12 +189,19 @@ RSpec.describe ActivityPub::SynchronizeFollowersService, type: :service do end before do + stub_const('ActivityPub::SynchronizeFollowersService::MAX_COLLECTION_PAGES', 1) stub_request(:get, collection_uri).to_return(status: 200, body: Oj.dump(payload), headers: { 'Content-Type': 'application/activity+json' }) end - it 'does not change followers' do - expect { subject.call(actor, collection_uri) } - .to_not(change { actor.followers.reload.reorder(id: :asc).pluck(:id) }) + it 'confirms pending follow request but does not remove extra followers' do + previous_follower_ids = actor.followers.pluck(:id) + + subject.call(actor, collection_uri) + + expect(previous_follower_ids - actor.followers.reload.pluck(:id)) + .to be_empty + expect(mallory) + .to be_following(actor) end end end