Fix #2706 - Always respond with 200 to PuSH payloads (#2733)

Fix #2196 - Respond with 201 when Salmon accepted, 400 when unverified
Fix #2629 - Correctly handle confirm_domain? for local accounts
Unify rules for extracting author acct from XML, prefer <email>, fall back
to <name> + <uri> (see also #2017, #2172)

9 files changed, 77 insertions, 85 deletions

diff --git a/app/controllers/api/salmon_controller.rb b/app/controllers/api/salmon_controller.rb
index a7872d542..7fc5e548d 100644
--- a/app/controllers/api/salmon_controller.rb
+++ b/app/controllers/api/salmon_controller.rb
@@ -5,13 +5,13 @@ class Api::SalmonController < ApiController
   respond_to :txt
 
   def update
-    body = request.body.read
+    payload = request.body.read
 
-    if body.nil?
-      head 200
-    else
-      SalmonWorker.perform_async(@account.id, body.force_encoding('UTF-8'))
+    if !payload.nil? && verify?(payload)
+      SalmonWorker.perform_async(@account.id, payload.force_encoding('UTF-8'))
       head 201
+    else
+      head 202
     end
   end
 
@@ -20,4 +20,8 @@ class Api::SalmonController < ApiController
   def set_account
     @account = Account.find(params[:id])
   end
+
+  def verify?(payload)
+    VerifySalmonService.new.call(payload)
+  end
 end
diff --git a/app/controllers/api/subscriptions_controller.rb b/app/controllers/api/subscriptions_controller.rb
index 51c476436..135a5632e 100644
--- a/app/controllers/api/subscriptions_controller.rb
+++ b/app/controllers/api/subscriptions_controller.rb
@@ -19,10 +19,9 @@ class Api::SubscriptionsController < ApiController
 
     if subscription.verify(body, request.headers['HTTP_X_HUB_SIGNATURE'])
       ProcessingWorker.perform_async(@account.id, body.force_encoding('UTF-8'))
-      head 201
-    else
-      head 202
     end
+
+    head 200
   end
 
   private
diff --git a/app/services/concerns/author_extractor.rb b/app/services/concerns/author_extractor.rb
new file mode 100644
index 000000000..d99780e7d
--- /dev/null
+++ b/app/services/concerns/author_extractor.rb
@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+module AuthorExtractor
+  def author_from_xml(xml)
+    # Try <email> for acct
+    acct = xml.at_xpath('./xmlns:author/xmlns:email', xmlns: TagManager::XMLNS)&.content
+
+    # Try <name> + <uri>
+    if acct.blank?
+      username = xml.at_xpath('./xmlns:author/xmlns:name', xmlns: TagManager::XMLNS)&.content
+      uri      = xml.at_xpath('./xmlns:author/xmlns:uri', xmlns: TagManager::XMLNS)&.content
+
+      return nil if username.blank? || uri.blank?
+
+      domain = Addressable::URI.parse(uri).normalize.host
+      acct   = "#{username}@#{domain}"
+    end
+
+    FollowRemoteAccountService.new.call(acct)
+  end
+end
diff --git a/app/services/fetch_remote_account_service.rb b/app/services/fetch_remote_account_service.rb
index 7c2fb0ef1..98fdf3dfe 100644
--- a/app/services/fetch_remote_account_service.rb
+++ b/app/services/fetch_remote_account_service.rb
@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 class FetchRemoteAccountService < BaseService
+  include AuthorExtractor
+
   def call(url, prefetched_body = nil)
     if prefetched_body.nil?
       atom_url, body = FetchAtomService.new.call(url)
@@ -19,21 +21,10 @@ class FetchRemoteAccountService < BaseService
     xml = Nokogiri::XML(body)
     xml.encoding = 'utf-8'
 
-    email = xml.at_xpath('//xmlns:author/xmlns:email').try(:content)
-    if email.nil?
-      url_parts = Addressable::URI.parse(url).normalize
-      username  = xml.at_xpath('//xmlns:author/xmlns:name').try(:content)
-      domain    = url_parts.host
-    else
-      username, domain = email.split('@')
-    end
-
-    return nil if username.nil? || domain.nil?
-
-    Rails.logger.debug "Going to webfinger #{username}@#{domain}"
+    account = author_from_xml(xml.at_xpath('/xmlns:feed', xmlns: TagManager::XMLNS))
 
-    account = FollowRemoteAccountService.new.call("#{username}@#{domain}")
     UpdateRemoteProfileService.new.call(xml, account) unless account.nil?
+
     account
   rescue TypeError
     Rails.logger.debug "Unparseable URL given: #{url}"
diff --git a/app/services/fetch_remote_status_service.rb b/app/services/fetch_remote_status_service.rb
index 5a454808e..f414813ad 100644
--- a/app/services/fetch_remote_status_service.rb
+++ b/app/services/fetch_remote_status_service.rb
@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 class FetchRemoteStatusService < BaseService
+  include AuthorExtractor
+
   def call(url, prefetched_body = nil)
     if prefetched_body.nil?
       atom_url, body = FetchAtomService.new.call(url)
@@ -21,37 +23,19 @@ class FetchRemoteStatusService < BaseService
     xml = Nokogiri::XML(body)
     xml.encoding = 'utf-8'
 
-    account = extract_author(url, xml)
+    account = author_from_xml(xml.at_xpath('/xmlns:entry', xmlns: TagManager::XMLNS))
+    domain  = Addressable::URI.parse(url).normalize.host
 
-    return nil if account.nil?
+    return nil unless !account.nil? && confirmed_domain?(domain, account)
 
     statuses = ProcessFeedService.new.call(body, account)
-
     statuses.first
-  end
-
-  def extract_author(url, xml)
-    url_parts = Addressable::URI.parse(url).normalize
-    username  = xml.at_xpath('//xmlns:author/xmlns:name').try(:content)
-    domain    = url_parts.host
-
-    return nil if username.nil?
-
-    Rails.logger.debug "Going to webfinger #{username}@#{domain}"
-
-    account = FollowRemoteAccountService.new.call("#{username}@#{domain}")
-
-    # If the author's confirmed URLs do not match the domain of the URL
-    # we are reading this from, abort
-    return nil unless confirmed_domain?(domain, account)
-
-    account
   rescue Nokogiri::XML::XPath::SyntaxError
     Rails.logger.debug 'Invalid XML or missing namespace'
     nil
   end
 
   def confirmed_domain?(domain, account)
-    domain.casecmp(account.domain).zero? || domain.casecmp(Addressable::URI.parse(account.remote_url).normalize.host).zero?
+    account.domain.nil? || domain.casecmp(account.domain).zero? || domain.casecmp(Addressable::URI.parse(account.remote_url).normalize.host).zero?
   end
 end
diff --git a/app/services/process_feed_service.rb b/app/services/process_feed_service.rb
index 7a27b7b29..4d23a6262 100644
--- a/app/services/process_feed_service.rb
+++ b/app/services/process_feed_service.rb
@@ -20,6 +20,8 @@ class ProcessFeedService < BaseService
   end
 
   class ProcessEntry
+    include AuthorExtractor
+
     def call(xml, account)
       @account = account
       @xml     = xml
@@ -108,7 +110,7 @@ class ProcessFeedService < BaseService
       # If that author cannot be found, don't record the status (do not misattribute)
       if account?(entry)
         begin
-          account = find_or_resolve_account(acct(entry))
+          account = author_from_xml(entry)
           return [nil, false] if account.nil?
         rescue Goldfinger::Error
           return [nil, false]
@@ -143,10 +145,6 @@ class ProcessFeedService < BaseService
       [status, true]
     end
 
-    def find_or_resolve_account(acct)
-      FollowRemoteAccountService.new.call(acct)
-    end
-
     def find_or_resolve_status(parent, uri, url)
       status = find_status(uri)
 
@@ -275,13 +273,5 @@ class ProcessFeedService < BaseService
     def account?(xml = @xml)
       !xml.at_xpath('./xmlns:author', xmlns: TagManager::XMLNS).nil?
     end
-
-    def acct(xml = @xml)
-      username = xml.at_xpath('./xmlns:author/xmlns:name', xmlns: TagManager::XMLNS).content
-      url      = xml.at_xpath('./xmlns:author/xmlns:uri', xmlns: TagManager::XMLNS).content
-      domain   = Addressable::URI.parse(url).normalize.host
-
-      "#{username}@#{domain}"
-    end
   end
 end
diff --git a/app/services/process_interaction_service.rb b/app/services/process_interaction_service.rb
index 410e805d3..1f15a265d 100644
--- a/app/services/process_interaction_service.rb
+++ b/app/services/process_interaction_service.rb
@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 class ProcessInteractionService < BaseService
+  include AuthorExtractor
+
   # Record locally the remote interaction with our user
   # @param [String] envelope Salmon envelope
   # @param [Account] target_account Account the Salmon was addressed to
@@ -10,18 +12,9 @@ class ProcessInteractionService < BaseService
     xml = Nokogiri::XML(body)
     xml.encoding = 'utf-8'
 
-    return unless contains_author?(xml)
-
-    username = xml.at_xpath('/xmlns:entry/xmlns:author/xmlns:name', xmlns: TagManager::XMLNS).content
-    url      = xml.at_xpath('/xmlns:entry/xmlns:author/xmlns:uri', xmlns: TagManager::XMLNS).content
-    domain   = Addressable::URI.parse(url).normalize.host
-    account  = Account.find_by(username: username, domain: domain)
-
-    if account.nil?
-      account = follow_remote_account_service.call("#{username}@#{domain}")
-    end
+    account = author_from_xml(xml.at_xpath('/xmlns:entry', xmlns: TagManager::XMLNS))
 
-    return if account.suspended?
+    return if account.nil? || account.suspended?
 
     if salmon.verify(envelope, account.keypair)
       RemoteProfileUpdateWorker.perform_async(account.id, body.force_encoding('UTF-8'), true)
@@ -59,10 +52,6 @@ class ProcessInteractionService < BaseService
 
   private
 
-  def contains_author?(xml)
-    !(xml.at_xpath('/xmlns:entry/xmlns:author/xmlns:name', xmlns: TagManager::XMLNS).nil? || xml.at_xpath('/xmlns:entry/xmlns:author/xmlns:uri', xmlns: TagManager::XMLNS).nil?)
-  end
-
   def mentions_account?(xml, account)
     xml.xpath('/xmlns:entry/xmlns:link[@rel="mentioned"]', xmlns: TagManager::XMLNS).each { |mention_link| return true if [TagManager.instance.uri_for(account), TagManager.instance.url_for(account)].include?(mention_link.attribute('href').value) }
     false
@@ -144,16 +133,4 @@ class ProcessInteractionService < BaseService
   def salmon
     @salmon ||= OStatus2::Salmon.new
   end
-
-  def follow_remote_account_service
-    @follow_remote_account_service ||= FollowRemoteAccountService.new
-  end
-
-  def process_feed_service
-    @process_feed_service ||= ProcessFeedService.new
-  end
-
-  def remove_status_service
-    @remove_status_service ||= RemoveStatusService.new
-  end
 end
diff --git a/app/services/verify_salmon_service.rb b/app/services/verify_salmon_service.rb
new file mode 100644
index 000000000..cd674837d
--- /dev/null
+++ b/app/services/verify_salmon_service.rb
@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+class VerifySalmonService < BaseService
+  include AuthorExtractor
+
+  def call(payload)
+    body = salmon.unpack(payload)
+
+    xml = Nokogiri::XML(body)
+    xml.encoding = 'utf-8'
+
+    account = author_from_xml(xml.at_xpath('/xmlns:entry', xmlns: TagManager::XMLNS))
+
+    if account.nil?
+      false
+    else
+      salmon.verify(payload, account.keypair)
+    end
+  end
+
+  private
+
+  def salmon
+    @salmon ||= OStatus2::Salmon.new
+  end
+end
diff --git a/spec/controllers/api/subscriptions_controller_spec.rb b/spec/controllers/api/subscriptions_controller_spec.rb
index 44841176a..97e36420d 100644
--- a/spec/controllers/api/subscriptions_controller_spec.rb
+++ b/spec/controllers/api/subscriptions_controller_spec.rb
@@ -45,8 +45,8 @@ RSpec.describe Api::SubscriptionsController, type: :controller do
       post :update, params: { id: account.id }
     end
 
-    it 'returns http created' do
-      expect(response).to have_http_status(:created)
+    it 'returns http success' do
+      expect(response).to have_http_status(:success)
     end
 
     it 'creates statuses for feed' do