Refactor ResolveRemoteAccountService (#4258)

* Refactor ResolveRemoteAccountService

* Remove trailing whitespace

* Use redis locks around critical ResolveRemoteAccountService code

* Add test for race condition of lock
pull/4270/head
Eugen Rochko 2017-07-19 14:44:04 +02:00 committed by GitHub
parent bc1f9dc24b
commit 8400bee3b1
4 changed files with 141 additions and 58 deletions

View File

@ -52,6 +52,7 @@ gem 'rack-timeout', '~> 0.4'
gem 'rails-i18n', '~> 5.0' gem 'rails-i18n', '~> 5.0'
gem 'rails-settings-cached', '~> 0.6' gem 'rails-settings-cached', '~> 0.6'
gem 'redis', '~> 3.3', require: ['redis', 'redis/connection/hiredis'] gem 'redis', '~> 3.3', require: ['redis', 'redis/connection/hiredis']
gem 'mario-redis-lock', '~> 1.2', require: 'redis_lock'
gem 'rqrcode', '~> 0.10' gem 'rqrcode', '~> 0.10'
gem 'ruby-oembed', '~> 0.12', require: 'oembed' gem 'ruby-oembed', '~> 0.12', require: 'oembed'
gem 'sanitize', '~> 4.4' gem 'sanitize', '~> 4.4'

View File

@ -242,6 +242,8 @@ GEM
nokogiri (>= 1.5.9) nokogiri (>= 1.5.9)
mail (2.6.6) mail (2.6.6)
mime-types (>= 1.16, < 4) mime-types (>= 1.16, < 4)
mario-redis-lock (1.2.0)
redis (~> 3, >= 3.0.5)
method_source (0.8.2) method_source (0.8.2)
microformats (4.0.7) microformats (4.0.7)
json json
@ -535,6 +537,7 @@ DEPENDENCIES
letter_opener_web (~> 1.3) letter_opener_web (~> 1.3)
link_header (~> 0.0) link_header (~> 0.0)
lograge (~> 0.5) lograge (~> 0.5)
mario-redis-lock (~> 1.2)
microformats (~> 4.0) microformats (~> 4.0)
mime-types (~> 3.1) mime-types (~> 3.1)
nokogiri (~> 1.7) nokogiri (~> 1.7)

View File

@ -11,97 +11,153 @@ class ResolveRemoteAccountService < BaseService
# @param [String] uri User URI in the form of username@domain # @param [String] uri User URI in the form of username@domain
# @return [Account] # @return [Account]
def call(uri, update_profile = true, redirected = nil) def call(uri, update_profile = true, redirected = nil)
username, domain = uri.split('@') @username, @domain = uri.split('@')
return Account.find_local(username) if TagManager.instance.local_domain?(domain) return Account.find_local(@username) if TagManager.instance.local_domain?(@domain)
account = Account.find_remote(username, domain) @account = Account.find_remote(@username, @domain)
return account unless account_needs_webfinger_update?(account)
return @account unless webfinger_update_due?
Rails.logger.debug "Looking up webfinger for #{uri}" Rails.logger.debug "Looking up webfinger for #{uri}"
data = Goldfinger.finger("acct:#{uri}") @webfinger = Goldfinger.finger("acct:#{uri}")
raise Goldfinger::Error, 'Missing resource links' if data.link('http://schemas.google.com/g/2010#updates-from').nil? || data.link('salmon').nil? || data.link('http://webfinger.net/rel/profile-page').nil? || data.link('magic-public-key').nil? raise Goldfinger::Error, 'Missing resource links' if links_missing?
# Disallow account hijacking confirmed_username, confirmed_domain = @webfinger.subject.gsub(/\Aacct:/, '').split('@')
confirmed_username, confirmed_domain = data.subject.gsub(/\Aacct:/, '').split('@')
unless confirmed_username.casecmp(username).zero? && confirmed_domain.casecmp(domain).zero? if confirmed_username.casecmp(@username).zero? && confirmed_domain.casecmp(@domain).zero?
return call("#{confirmed_username}@#{confirmed_domain}", update_profile, true) if redirected.nil? @username = confirmed_username
raise Goldfinger::Error, 'Requested and returned acct URI do not match' @domain = confirmed_domain
end
return Account.find_local(confirmed_username) if TagManager.instance.local_domain?(confirmed_domain)
confirmed_account = Account.find_remote(confirmed_username, confirmed_domain)
if confirmed_account.nil?
Rails.logger.debug "Creating new remote account for #{uri}"
domain_block = DomainBlock.find_by(domain: domain)
account = Account.new(username: confirmed_username, domain: confirmed_domain)
account.suspended = true if domain_block && domain_block.suspend?
account.silenced = true if domain_block && domain_block.silence?
account.private_key = nil
else else
account = confirmed_account return call("#{confirmed_username}@#{confirmed_domain}", update_profile, true) if redirected.nil?
raise Goldfinger::Error, 'Requested and returned acct URIs do not match'
end end
account.last_webfingered_at = Time.now.utc return Account.find_local(@username) if TagManager.instance.local_domain?(@domain)
account.remote_url = data.link('http://schemas.google.com/g/2010#updates-from').href RedisLock.acquire(lock_options) do |lock|
account.salmon_url = data.link('salmon').href if lock.acquired?
account.url = data.link('http://webfinger.net/rel/profile-page').href @account = Account.find_remote(@username, @domain)
account.public_key = magic_key_to_pem(data.link('magic-public-key').href)
body, xml = get_feed(account.remote_url) create_account if @account.nil?
hubs = get_hubs(xml) update_account
account.uri = get_account_uri(xml) update_account_profile if update_profile
account.hub_url = hubs.first.attribute('href').value end
begin
account.save!
get_profile(body, account) if update_profile
rescue ActiveRecord::RecordNotUnique
# The account has been added by another worker!
return Account.find_remote(confirmed_username, confirmed_domain)
end end
account @account
end end
private private
def account_needs_webfinger_update?(account) def links_missing?
account&.last_webfingered_at.nil? || account.last_webfingered_at <= 1.day.ago @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?
end end
def get_feed(url) def webfinger_update_due?
response = Request.new(:get, url).perform @account.nil? || @account.last_webfingered_at.nil? || @account.last_webfingered_at <= 1.day.ago
raise Goldfinger::Error, "Feed attempt failed for #{url}: HTTP #{response.code}" unless response.code == 200
[response.to_s, Nokogiri::XML(response)]
end end
def get_hubs(xml) def create_account
hubs = xml.xpath('//xmlns:link[@rel="hub"]') Rails.logger.debug "Creating new remote account for #{@username}@#{@domain}"
raise Goldfinger::Error, 'No PubSubHubbub hubs found' if hubs.empty? || hubs.first.attribute('href').nil?
hubs @account = Account.new(username: @username, domain: @domain)
@account.suspended = true if auto_suspend?
@account.silenced = true if auto_silence?
@account.private_key = nil
end end
def get_account_uri(xml) def update_account
author_uri = xml.at_xpath('/xmlns:feed/xmlns:author/xmlns:uri') @account.last_webfingered_at = Time.now.utc
@account.remote_url = atom_url
@account.salmon_url = salmon_url
@account.url = url
@account.public_key = public_key
@account.uri = canonical_uri
@account.hub_url = hub_url
@account.save!
end
def auto_suspend?
domain_block && domain_block.suspend?
end
def auto_silence?
domain_block && domain_block.silence?
end
def domain_block
return @domain_block if defined?(@domain_block)
@domain_block = DomainBlock.find_by(domain: @domain)
end
def atom_url
@atom_url ||= @webfinger.link('http://schemas.google.com/g/2010#updates-from').href
end
def salmon_url
@salmon_url ||= @webfinger.link('salmon').href
end
def url
@url ||= @webfinger.link('http://webfinger.net/rel/profile-page').href
end
def public_key
@public_key ||= magic_key_to_pem(@webfinger.link('magic-public-key').href)
end
def canonical_uri
return @canonical_uri if defined?(@canonical_uri)
author_uri = atom.at_xpath('/xmlns:feed/xmlns:author/xmlns:uri')
if author_uri.nil? if author_uri.nil?
owner = xml.at_xpath('/xmlns:feed').at_xpath('./dfrn:owner', dfrn: DFRN_NS) owner = atom.at_xpath('/xmlns:feed').at_xpath('./dfrn:owner', dfrn: DFRN_NS)
author_uri = owner.at_xpath('./xmlns:uri') unless owner.nil? author_uri = owner.at_xpath('./xmlns:uri') unless owner.nil?
end end
raise Goldfinger::Error, 'Author URI could not be found' if author_uri.nil? raise Goldfinger::Error, 'Author URI could not be found' if author_uri.nil?
author_uri.content
@canonical_uri = author_uri.content
end end
def get_profile(body, account) def hub_url
RemoteProfileUpdateWorker.perform_async(account.id, body.force_encoding('UTF-8'), false) 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']
end
def atom_body
return @atom_body if defined?(@atom_body)
response = Request.new(:get, atom_url).perform
raise Goldfinger::Error, "Feed attempt failed for #{atom_url}: HTTP #{response.code}" unless response.code == 200
@atom_body = response.to_s
end
def atom
return @atom if defined?(@atom)
@atom = Nokogiri::XML(atom_body)
end
def update_account_profile
RemoteProfileUpdateWorker.perform_async(@account.id, atom_body.force_encoding('UTF-8'), false)
end
def lock_options
{ redis: Redis.current, key: "resolve:#{@username}@#{@domain}" }
end end
end end

View File

@ -68,4 +68,27 @@ RSpec.describe ResolveRemoteAccountService do
expect(account.domain).to eq 'localdomain.com' expect(account.domain).to eq 'localdomain.com'
expect(account.remote_url).to eq 'https://webdomain.com/users/foo.atom' expect(account.remote_url).to eq 'https://webdomain.com/users/foo.atom'
end end
it 'processes one remote account at a time using locks' do
wait_for_start = true
fail_occurred = false
return_values = []
threads = Array.new(5) do
Thread.new do
true while wait_for_start
begin
return_values << subject.call('foo@localdomain.com')
rescue ActiveRecord::RecordNotUnique
fail_occurred = true
end
end
end
wait_for_start = false
threads.each(&:join)
expect(fail_occurred).to be false
expect(return_values).to_not include(nil)
end
end end