From 1fcdaafa6fbe6d746a096c33263d76e6819da46d Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Thu, 20 Jul 2017 01:59:07 +0200 Subject: Fix webfinger retries (#4275) * Do not raise unretryable exceptions in ResolveRemoteAccountService * Removed fatal exceptions from ResolveRemoteAccountService Exceptions that cannot be retried should not be raised. New exception class for those that can be retried (Mastodon::UnexpectedResponseError) --- app/controllers/api/base_controller.rb | 6 +---- app/lib/exceptions.rb | 10 ++++++++ app/services/fetch_remote_account_service.rb | 3 --- app/services/fetch_remote_status_service.rb | 3 --- app/services/process_interaction_service.rb | 2 +- app/services/resolve_remote_account_service.rb | 29 +++++++++++----------- app/services/send_interaction_service.rb | 2 +- app/services/subscribe_service.rb | 2 +- app/workers/import_worker.rb | 6 ++--- app/workers/pubsubhubbub/delivery_worker.rb | 2 +- spec/controllers/api/base_controller_spec.rb | 2 +- .../resolve_remote_account_service_spec.rb | 6 ++--- spec/services/subscribe_service_spec.rb | 4 +-- spec/workers/pubsubhubbub/delivery_worker_spec.rb | 2 +- 14 files changed, 40 insertions(+), 39 deletions(-) diff --git a/app/controllers/api/base_controller.rb b/app/controllers/api/base_controller.rb index c1b2ec3cf..105a2859d 100644 --- a/app/controllers/api/base_controller.rb +++ b/app/controllers/api/base_controller.rb @@ -17,11 +17,7 @@ class Api::BaseController < ApplicationController render json: { error: 'Record not found' }, status: 404 end - rescue_from Goldfinger::Error do - render json: { error: 'Remote account could not be resolved' }, status: 422 - end - - rescue_from HTTP::Error do + rescue_from HTTP::Error, Mastodon::UnexpectedResponseError do render json: { error: 'Remote data could not be fetched' }, status: 503 end diff --git a/app/lib/exceptions.rb b/app/lib/exceptions.rb index 9bc802c12..34d84a34f 100644 --- a/app/lib/exceptions.rb +++ b/app/lib/exceptions.rb @@ -5,4 +5,14 @@ module Mastodon class NotPermittedError < Error; end class ValidationError < Error; end class RaceConditionError < Error; end + + class UnexpectedResponseError < Error + def initialize(response = nil) + @response = response + end + + def to_s + "#{@response.uri} returned code #{@response.code}" + end + end end diff --git a/app/services/fetch_remote_account_service.rb b/app/services/fetch_remote_account_service.rb index 1efac365b..8eed0d454 100644 --- a/app/services/fetch_remote_account_service.rb +++ b/app/services/fetch_remote_account_service.rb @@ -32,8 +32,5 @@ class FetchRemoteAccountService < BaseService rescue Nokogiri::XML::XPath::SyntaxError Rails.logger.debug 'Invalid XML or missing namespace' nil - rescue Goldfinger::NotFoundError, Goldfinger::Error - Rails.logger.debug 'Exceptions related to Goldfinger occurs' - nil end end diff --git a/app/services/fetch_remote_status_service.rb b/app/services/fetch_remote_status_service.rb index 6ac31e4d8..b9f5f97b1 100644 --- a/app/services/fetch_remote_status_service.rb +++ b/app/services/fetch_remote_status_service.rb @@ -33,9 +33,6 @@ class FetchRemoteStatusService < BaseService rescue Nokogiri::XML::XPath::SyntaxError Rails.logger.debug 'Invalid XML or missing namespace' nil - rescue Goldfinger::NotFoundError, Goldfinger::Error - Rails.logger.debug 'Exceptions related to Goldfinger occurs' - nil end def confirmed_domain?(domain, account) diff --git a/app/services/process_interaction_service.rb b/app/services/process_interaction_service.rb index 584a109ad..cc99cde03 100644 --- a/app/services/process_interaction_service.rb +++ b/app/services/process_interaction_service.rb @@ -47,7 +47,7 @@ class ProcessInteractionService < BaseService reflect_unblock!(account, target_account) end end - rescue Goldfinger::Error, HTTP::Error, OStatus2::BadSalmonError, Mastodon::NotPermittedError + rescue HTTP::Error, OStatus2::BadSalmonError, Mastodon::NotPermittedError nil end diff --git a/app/services/resolve_remote_account_service.rb b/app/services/resolve_remote_account_service.rb index c948243ec..e0e2ebc83 100644 --- a/app/services/resolve_remote_account_service.rb +++ b/app/services/resolve_remote_account_service.rb @@ -23,18 +23,19 @@ class ResolveRemoteAccountService < BaseService @webfinger = Goldfinger.finger("acct:#{uri}") - raise Goldfinger::Error, 'Missing resource links' if links_missing? - confirmed_username, confirmed_domain = @webfinger.subject.gsub(/\Aacct:/, '').split('@') if confirmed_username.casecmp(@username).zero? && confirmed_domain.casecmp(@domain).zero? @username = confirmed_username @domain = confirmed_domain + elsif redirected.nil? + return call("#{confirmed_username}@#{confirmed_domain}", update_profile, true) else - return call("#{confirmed_username}@#{confirmed_domain}", update_profile, true) if redirected.nil? - raise Goldfinger::Error, 'Requested and returned acct URIs do not match' + Rails.logger.debug 'Requested and returned acct URIs do not match' + return end + return if links_missing? return Account.find_local(@username) if TagManager.instance.local_domain?(@domain) RedisLock.acquire(lock_options) do |lock| @@ -49,6 +50,9 @@ class ResolveRemoteAccountService < BaseService end @account + rescue Goldfinger::Error => e + Rails.logger.debug "Webfinger query for #{uri} unsuccessful: #{e}" + nil end private @@ -57,7 +61,9 @@ class ResolveRemoteAccountService < BaseService @webfinger.link('http://schemas.google.com/g/2010#updates-from').nil? || @webfinger.link('salmon').nil? || @webfinger.link('http://webfinger.net/rel/profile-page').nil? || - @webfinger.link('magic-public-key').nil? + @webfinger.link('magic-public-key').nil? || + canonical_uri.nil? || + hub_url.nil? end def webfinger_update_due? @@ -123,19 +129,14 @@ class ResolveRemoteAccountService < BaseService author_uri = owner.at_xpath('./xmlns:uri') unless owner.nil? end - raise Goldfinger::Error, 'Author URI could not be found' if author_uri.nil? - - @canonical_uri = author_uri.content + @canonical_uri = author_uri.nil? ? nil : author_uri.content end def hub_url return @hub_url if defined?(@hub_url) - hubs = atom.xpath('//xmlns:link[@rel="hub"]') - - raise Goldfinger::Error, 'No PubSubHubbub hubs found' if hubs.empty? || hubs.first['href'].nil? - - @hub_url = hubs.first['href'] + hubs = atom.xpath('//xmlns:link[@rel="hub"]') + @hub_url = hubs.empty? || hubs.first['href'].nil? ? nil : hubs.first['href'] end def atom_body @@ -143,7 +144,7 @@ class ResolveRemoteAccountService < BaseService response = Request.new(:get, atom_url).perform - raise Goldfinger::Error, "Feed attempt failed for #{atom_url}: HTTP #{response.code}" unless response.code == 200 + raise Mastodon::UnexpectedResponseError, response unless response.code == 200 @atom_body = response.to_s end diff --git a/app/services/send_interaction_service.rb b/app/services/send_interaction_service.rb index ab0d3aeed..c11813abc 100644 --- a/app/services/send_interaction_service.rb +++ b/app/services/send_interaction_service.rb @@ -14,7 +14,7 @@ class SendInteractionService < BaseService delivery = build_request.perform - raise "Delivery failed for #{target_account.salmon_url}: HTTP #{delivery.code}" unless delivery.code > 199 && delivery.code < 300 + raise Mastodon::UnexpectedResponseError, delivery unless delivery.code > 199 && delivery.code < 300 end private diff --git a/app/services/subscribe_service.rb b/app/services/subscribe_service.rb index c1c0a4c8b..d3e41e691 100644 --- a/app/services/subscribe_service.rb +++ b/app/services/subscribe_service.rb @@ -18,7 +18,7 @@ class SubscribeService < BaseService else # The response was either a 429 rate limit, or a 5xx error. # We need to retry at a later time. Fail loudly! - raise "Subscription attempt failed for #{@account.acct} (#{@account.hub_url}): HTTP #{@response.code}" + raise Mastodon::UnexpectedResponseError, @response end end diff --git a/app/workers/import_worker.rb b/app/workers/import_worker.rb index 90a226206..27cc6b365 100644 --- a/app/workers/import_worker.rb +++ b/app/workers/import_worker.rb @@ -44,7 +44,7 @@ class ImportWorker target_account = ResolveRemoteAccountService.new.call(row.first) next if target_account.nil? MuteService.new.call(from_account, target_account) - rescue Goldfinger::Error, HTTP::Error, OpenSSL::SSL::SSLError + rescue Mastodon::UnexpectedResponseError, HTTP::Error, OpenSSL::SSL::SSLError next end end @@ -56,7 +56,7 @@ class ImportWorker target_account = ResolveRemoteAccountService.new.call(row.first) next if target_account.nil? BlockService.new.call(from_account, target_account) - rescue Goldfinger::Error, HTTP::Error, OpenSSL::SSL::SSLError + rescue Mastodon::UnexpectedResponseError, HTTP::Error, OpenSSL::SSL::SSLError next end end @@ -66,7 +66,7 @@ class ImportWorker import_rows.each do |row| begin FollowService.new.call(from_account, row.first) - rescue Mastodon::NotPermittedError, ActiveRecord::RecordNotFound, Goldfinger::Error, HTTP::Error, OpenSSL::SSL::SSLError + rescue Mastodon::NotPermittedError, ActiveRecord::RecordNotFound, Mastodon::UnexpectedResponseError, HTTP::Error, OpenSSL::SSL::SSLError next end end diff --git a/app/workers/pubsubhubbub/delivery_worker.rb b/app/workers/pubsubhubbub/delivery_worker.rb index 2e1101b93..035a59048 100644 --- a/app/workers/pubsubhubbub/delivery_worker.rb +++ b/app/workers/pubsubhubbub/delivery_worker.rb @@ -23,7 +23,7 @@ class Pubsubhubbub::DeliveryWorker def process_delivery payload_delivery - raise "Delivery failed for #{subscription.callback_url}: HTTP #{payload_delivery.code}" unless response_successful? + raise Mastodon::UnexpectedResponseError, payload_delivery unless response_successful? subscription.touch(:last_successful_delivery_at) end diff --git a/spec/controllers/api/base_controller_spec.rb b/spec/controllers/api/base_controller_spec.rb index 7d5e0116c..0c7ca8990 100644 --- a/spec/controllers/api/base_controller_spec.rb +++ b/spec/controllers/api/base_controller_spec.rb @@ -32,7 +32,7 @@ describe Api::BaseController do ActiveRecord::RecordInvalid => 422, Mastodon::ValidationError => 422, ActiveRecord::RecordNotFound => 404, - Goldfinger::Error => 422, + Mastodon::UnexpectedResponseError => 503, HTTP::Error => 503, OpenSSL::SSL::SSLError => 503, Mastodon::NotPermittedError => 403, diff --git a/spec/services/resolve_remote_account_service_spec.rb b/spec/services/resolve_remote_account_service_spec.rb index c51588210..ab5d3c6e5 100644 --- a/spec/services/resolve_remote_account_service_spec.rb +++ b/spec/services/resolve_remote_account_service_spec.rb @@ -22,11 +22,11 @@ RSpec.describe ResolveRemoteAccountService do end it 'raises error if no such user can be resolved via webfinger' do - expect { subject.call('catsrgr8@quitter.no') }.to raise_error Goldfinger::Error + expect(subject.call('catsrgr8@quitter.no')).to be_nil end it 'raises error if the domain does not have webfinger' do - expect { subject.call('catsrgr8@example.com') }.to raise_error Goldfinger::Error + expect(subject.call('catsrgr8@example.com')).to be_nil end it 'returns an already existing remote account' do @@ -58,7 +58,7 @@ RSpec.describe ResolveRemoteAccountService do end it 'prevents hijacking inexisting accounts' do - expect { subject.call('hacker2@redirected.com') }.to raise_error Goldfinger::Error + expect(subject.call('hacker2@redirected.com')).to be_nil end it 'returns a new remote account' do diff --git a/spec/services/subscribe_service_spec.rb b/spec/services/subscribe_service_spec.rb index 5db91ad99..835be5ec5 100644 --- a/spec/services/subscribe_service_spec.rb +++ b/spec/services/subscribe_service_spec.rb @@ -33,11 +33,11 @@ RSpec.describe SubscribeService do it 'fails loudly if PuSH hub is unavailable' do stub_request(:post, 'http://hub.example.com/').to_return(status: 503) - expect { subject.call(account) }.to raise_error(/Subscription attempt failed/) + expect { subject.call(account) }.to raise_error Mastodon::UnexpectedResponseError end it 'fails loudly if rate limited' do stub_request(:post, 'http://hub.example.com/').to_return(status: 429) - expect { subject.call(account) }.to raise_error(/Subscription attempt failed/) + expect { subject.call(account) }.to raise_error Mastodon::UnexpectedResponseError end end diff --git a/spec/workers/pubsubhubbub/delivery_worker_spec.rb b/spec/workers/pubsubhubbub/delivery_worker_spec.rb index a83245786..b72001568 100644 --- a/spec/workers/pubsubhubbub/delivery_worker_spec.rb +++ b/spec/workers/pubsubhubbub/delivery_worker_spec.rb @@ -26,7 +26,7 @@ describe Pubsubhubbub::DeliveryWorker do subscription = Fabricate(:subscription) stub_request_to_respond_with(subscription, 500) - expect { subject.perform(subscription.id, payload) }.to raise_error(/Delivery failed/) + expect { subject.perform(subscription.id, payload) }.to raise_error Mastodon::UnexpectedResponseError end it 'updates subscriptions when delivery succeeds' do -- cgit