From 5f4c0b79c25486548b44f1bc5d65e7527a4843c4 Mon Sep 17 00:00:00 2001 From: Claire Date: Wed, 24 Feb 2021 06:32:13 +0100 Subject: [PATCH] Change ResolveAccountService's handling of skip_webfinger (#15750) * Change ResolveAccountService's handling of skip_webfinger Change it so it never makes any webfinger query, as the name would imply. * Add tests * Change FollowService to not take an URI for target_account * Restore domain-block check in FollowService * Fix tests --- app/services/follow_service.rb | 7 ++-- app/services/resolve_account_service.rb | 5 ++- .../api/v1/follow_requests_controller_spec.rb | 2 +- .../api/v1/notifications_controller_spec.rb | 2 +- .../authorize_interactions_controller_spec.rb | 2 - spec/services/follow_service_spec.rb | 20 ++++----- spec/services/resolve_account_service_spec.rb | 41 +++++++++++++++++++ 7 files changed, 60 insertions(+), 19 deletions(-) diff --git a/app/services/follow_service.rb b/app/services/follow_service.rb index b98f7011d..d3db07a74 100644 --- a/app/services/follow_service.rb +++ b/app/services/follow_service.rb @@ -3,10 +3,11 @@ class FollowService < BaseService include Redisable include Payloadable + include DomainControlHelper # Follow a remote user, notify remote user about the 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 [Account] target_account Account to follow # @param [Hash] options # @option [Boolean] :reblogs Whether or not to show reblogs, defaults to true # @option [Boolean] :notify Whether to create notifications about new posts, defaults to false @@ -15,7 +16,7 @@ class FollowService < BaseService # @option [Boolean] :with_rate_limit def call(source_account, target_account, options = {}) @source_account = source_account - @target_account = ResolveAccountService.new.call(target_account, skip_webfinger: true) + @target_account = target_account @options = { bypass_locked: false, bypass_limit: false, with_rate_limit: false }.merge(options) raise ActiveRecord::RecordNotFound if following_not_possible? @@ -43,7 +44,7 @@ class FollowService < BaseService end def following_not_allowed? - @target_account.blocking?(@source_account) || @source_account.blocking?(@target_account) || @target_account.moved? || (!@target_account.local? && @target_account.ostatus?) || @source_account.domain_blocking?(@target_account.domain) + domain_not_allowed?(@target_account.domain) || @target_account.blocking?(@source_account) || @source_account.blocking?(@target_account) || @target_account.moved? || (!@target_account.local? && @target_account.ostatus?) || @source_account.domain_blocking?(@target_account.domain) end def change_follow_options! diff --git a/app/services/resolve_account_service.rb b/app/services/resolve_account_service.rb index 3301aaf51..b8ddeb2ad 100644 --- a/app/services/resolve_account_service.rb +++ b/app/services/resolve_account_service.rb @@ -10,7 +10,7 @@ class ResolveAccountService < BaseService # @param [String, Account] uri URI in the username@domain format or account record # @param [Hash] options # @option options [Boolean] :redirected Do not follow further Webfinger redirects - # @option options [Boolean] :skip_webfinger Do not attempt to refresh account data + # @option options [Boolean] :skip_webfinger Do not attempt any webfinger query or refreshing account data # @return [Account] def call(uri, options = {}) return if uri.blank? @@ -120,8 +120,9 @@ class ResolveAccountService < BaseService def webfinger_update_due? return false if @options[:check_delivery_availability] && !DeliveryFailureTracker.available?(@domain) + return false if @options[:skip_webfinger] - @account.nil? || ((!@options[:skip_webfinger] || @account.ostatus?) && @account.possibly_stale?) + @account.nil? || (@account.ostatus? && @account.possibly_stale?) end def activitypub_ready? diff --git a/spec/controllers/api/v1/follow_requests_controller_spec.rb b/spec/controllers/api/v1/follow_requests_controller_spec.rb index ae92a9627..1034faa32 100644 --- a/spec/controllers/api/v1/follow_requests_controller_spec.rb +++ b/spec/controllers/api/v1/follow_requests_controller_spec.rb @@ -8,7 +8,7 @@ RSpec.describe Api::V1::FollowRequestsController, type: :controller do let(:follower) { Fabricate(:account, username: 'bob') } before do - FollowService.new.call(follower, user.account.acct) + FollowService.new.call(follower, user.account) allow(controller).to receive(:doorkeeper_token) { token } end diff --git a/spec/controllers/api/v1/notifications_controller_spec.rb b/spec/controllers/api/v1/notifications_controller_spec.rb index db3f4b782..5a0b24bbf 100644 --- a/spec/controllers/api/v1/notifications_controller_spec.rb +++ b/spec/controllers/api/v1/notifications_controller_spec.rb @@ -57,7 +57,7 @@ RSpec.describe Api::V1::NotificationsController, type: :controller do @mention_from_status = mentioning_status.mentions.first @favourite = FavouriteService.new.call(other.account, first_status) @second_favourite = FavouriteService.new.call(third.account, first_status) - @follow = FollowService.new.call(other.account, 'alice') + @follow = FollowService.new.call(other.account, user.account) end describe 'with no options' do diff --git a/spec/controllers/authorize_interactions_controller_spec.rb b/spec/controllers/authorize_interactions_controller_spec.rb index ce4257b68..b4ce30cd7 100644 --- a/spec/controllers/authorize_interactions_controller_spec.rb +++ b/spec/controllers/authorize_interactions_controller_spec.rb @@ -99,12 +99,10 @@ describe AuthorizeInteractionsController do allow(ResolveAccountService).to receive(:new).and_return(service) allow(service).to receive(:call).with('user@hostname').and_return(target_account) - allow(service).to receive(:call).with(target_account, skip_webfinger: true).and_return(target_account) post :create, params: { acct: 'acct:user@hostname' } - expect(service).to have_received(:call).with(target_account, skip_webfinger: true) expect(account.following?(target_account)).to be true expect(response).to render_template(:success) end diff --git a/spec/services/follow_service_spec.rb b/spec/services/follow_service_spec.rb index ae863a9f0..63d6eb3bd 100644 --- a/spec/services/follow_service_spec.rb +++ b/spec/services/follow_service_spec.rb @@ -10,7 +10,7 @@ RSpec.describe FollowService, type: :service do let(:bob) { Fabricate(:user, email: 'bob@example.com', account: Fabricate(:account, locked: true, username: 'bob')).account } before do - subject.call(sender, bob.acct) + subject.call(sender, bob) end it 'creates a follow request with reblogs' do @@ -22,7 +22,7 @@ RSpec.describe FollowService, type: :service do let(:bob) { Fabricate(:user, email: 'bob@example.com', account: Fabricate(:account, locked: true, username: 'bob')).account } before do - subject.call(sender, bob.acct, reblogs: false) + subject.call(sender, bob, reblogs: false) end it 'creates a follow request without reblogs' do @@ -35,7 +35,7 @@ RSpec.describe FollowService, type: :service do before do sender.touch(:silenced_at) - subject.call(sender, bob.acct) + subject.call(sender, bob) end it 'creates a follow request with reblogs' do @@ -48,7 +48,7 @@ RSpec.describe FollowService, type: :service do before do bob.mute!(sender) - subject.call(sender, bob.acct) + subject.call(sender, bob) end it 'creates a following relation with reblogs' do @@ -61,7 +61,7 @@ RSpec.describe FollowService, type: :service do let(:bob) { Fabricate(:user, email: 'bob@example.com', account: Fabricate(:account, username: 'bob')).account } before do - subject.call(sender, bob.acct) + subject.call(sender, bob) end it 'creates a following relation with reblogs' do @@ -74,7 +74,7 @@ RSpec.describe FollowService, type: :service do let(:bob) { Fabricate(:user, email: 'bob@example.com', account: Fabricate(:account, username: 'bob')).account } before do - subject.call(sender, bob.acct, reblogs: false) + subject.call(sender, bob, reblogs: false) end it 'creates a following relation without reblogs' do @@ -88,7 +88,7 @@ RSpec.describe FollowService, type: :service do before do sender.follow!(bob) - subject.call(sender, bob.acct) + subject.call(sender, bob) end it 'keeps a following relation' do @@ -101,7 +101,7 @@ RSpec.describe FollowService, type: :service do before do sender.follow!(bob, reblogs: true) - subject.call(sender, bob.acct, reblogs: false) + subject.call(sender, bob, reblogs: false) end it 'disables reblogs' do @@ -114,7 +114,7 @@ RSpec.describe FollowService, type: :service do before do sender.follow!(bob, reblogs: false) - subject.call(sender, bob.acct, reblogs: true) + subject.call(sender, bob, reblogs: true) end it 'disables reblogs' do @@ -128,7 +128,7 @@ RSpec.describe FollowService, type: :service do before do stub_request(:post, "http://example.com/inbox").to_return(:status => 200, :body => "", :headers => {}) - subject.call(sender, bob.acct) + subject.call(sender, bob) end it 'creates follow request' do diff --git a/spec/services/resolve_account_service_spec.rb b/spec/services/resolve_account_service_spec.rb index a604e90b5..7b1e8885c 100644 --- a/spec/services/resolve_account_service_spec.rb +++ b/spec/services/resolve_account_service_spec.rb @@ -13,6 +13,47 @@ RSpec.describe ResolveAccountService, type: :service do stub_request(:get, 'https://example.com/.well-known/webfinger?resource=acct:hoge@example.com').to_return(status: 410) end + context 'using skip_webfinger' do + context 'when account is known' do + let!(:remote_account) { Fabricate(:account, username: 'foo', domain: 'ap.example.com', protocol: 'activitypub') } + + context 'when domain is banned' do + let!(:domain_block) { Fabricate(:domain_block, domain: 'ap.example.com', severity: :suspend) } + + it 'does not return an account' do + expect(subject.call('foo@ap.example.com', skip_webfinger: true)).to be_nil + end + + it 'does not make a webfinger query' do + subject.call('foo@ap.example.com', skip_webfinger: true) + expect(a_request(:get, 'https://ap.example.com/.well-known/webfinger?resource=acct:foo@ap.example.com')).to_not have_been_made + end + end + + context 'when domain is not banned' do + it 'returns the expected account' do + expect(subject.call('foo@ap.example.com', skip_webfinger: true)).to eq remote_account + end + + it 'does not make a webfinger query' do + subject.call('foo@ap.example.com', skip_webfinger: true) + expect(a_request(:get, 'https://ap.example.com/.well-known/webfinger?resource=acct:foo@ap.example.com')).to_not have_been_made + end + end + end + + context 'when account is not known' do + it 'does not return an account' do + expect(subject.call('foo@ap.example.com', skip_webfinger: true)).to be_nil + end + + it 'does not make a webfinger query' do + subject.call('foo@ap.example.com', skip_webfinger: true) + expect(a_request(:get, 'https://ap.example.com/.well-known/webfinger?resource=acct:foo@ap.example.com')).to_not have_been_made + end + end + end + context 'when there is an LRDD endpoint but no resolvable account' do before do stub_request(:get, "https://quitter.no/.well-known/host-meta").to_return(request_fixture('.host-meta.txt'))