Merge pull request #5209 from jhass/federation_improvements

Federation improvements

Gemfile

@@ -188,6 +188,10 @@ group :development do
 
   gem 'guard-spork', '1.5.1'
   gem 'spork',       '1.0.0rc4'
+
+  # Debugging
+  gem 'pry'
+  gem 'pry-debundle'
 end
 
 group :test do

Gemfile.lock

@@ -317,6 +317,8 @@ GEM
       coderay (~> 1.1.0)
       method_source (~> 0.8.1)
       slop (~> 3.4)
+    pry-debundle (0.8)
+      pry
     rack (1.5.2)
     rack-cors (0.2.9)
     rack-google-analytics (1.2.0)
@@ -551,6 +553,8 @@ DEPENDENCIES
   omniauth-twitter (= 1.0.1)
   omniauth-wordpress (= 0.2.1)
   opengraph_parser (= 0.2.3)
+  pry
+  pry-debundle
   rack-cors (= 0.2.9)
   rack-google-analytics (= 1.2.0)
   rack-piwik (= 0.3.0)

app/models/reshare.rb

@@ -62,7 +62,7 @@ class Reshare < Post
   end
 
   def notification_type(user, person)
-    Notifications::Reshared if root.author == user.person
+    Notifications::Reshared if root.try(:author) == user.person
   end
 
   def absolute_root
@@ -74,35 +74,16 @@ class Reshare < Post
   private
 
   def after_parse
-    root_author = Webfinger.new(@root_diaspora_id).fetch
-    root_author.save! unless root_author.persisted?
-
-    return if Post.exists?(:guid => self.root_guid)
-
-    fetched_post = self.class.fetch_post(root_author, self.root_guid)
-
-    if fetched_post
-      #Why are we checking for this?
-      if root_author.diaspora_handle != fetched_post.diaspora_handle
-        raise "Diaspora ID (#{fetched_post.diaspora_handle}) in the root does not match the Diaspora ID (#{root_author.diaspora_handle}) specified in the reshare!"
+    if root.blank?
+      self.root = Diaspora::Fetcher::Single.find_or_fetch_from_remote root_guid, @root_diaspora_id do |fetched_post, author|
+        # why do we check this?
+        if fetched_post.diaspora_handle != author.diaspora_handle
+          raise Diaspora::PostNotFetchable, "Diaspora ID (#{fetched_post.diaspora_handle}) in the root does not match the Diaspora ID (#{author.diaspora_handle}) specified in the reshare!"
+        end
       end
-
-      fetched_post.save!
     end
   end
 
-  # Fetch a remote public post, used for receiving reshares of unknown posts
-  # @param [Person] author the remote post's author
-  # @param [String] guid the remote post's guid
-  # @return [Post] an unsaved remote post or false if the post was not found
-  def self.fetch_post author, guid
-    url = author.url + "/p/#{guid}.xml"
-    response = Faraday.get(url)
-    return false if response.status == 404 # Old pod, friendika
-    raise "Failed to get #{url}" unless response.success? # Other error, N/A for example
-    Diaspora::Parser.from_xml(response.body)
-  end
-
   def root_must_be_public
     if self.root && !self.root.public
       errors[:base] << "Only posts which are public may be reshared."

app/workers/deferred_dispatch.rb

@@ -17,6 +17,7 @@ module Workers
       end
 
       Postzord::Dispatcher.build(user, object, opts).post
+    rescue ActiveRecord::RecordNotFound # The target got deleted before the job was run
     end
   end
 end

app/workers/process_photo.rb

@@ -16,6 +16,7 @@ module Workers
       photo.processed_image.store!(unprocessed_image)
 
       photo.save!
+    rescue ActiveRecord::RecordNotFound # Deleted before the job was run
     end
   end
 end

app/workers/receive_local_batch.rb

@@ -10,6 +10,7 @@ module Workers
       object = object_class_string.constantize.find(object_id)
       receiver = Postzord::Receiver::LocalBatch.new(object, recipient_user_ids)
       receiver.perform!
+    rescue ActiveRecord::NotFound # Already deleted before the job could run
     end
   end
 end

config/initializers/faraday.rb

@@ -1,6 +1,13 @@
 #   Copyright (c) 2010-2011, Diaspora Inc.  This file is
 #   licensed under the Affero General Public License version 3 or later.  See
 #   the COPYRIGHT file.
+
+# Use net_http in test, that's better supported by webmock
+unless Rails.env.test?
+  require 'typhoeus/adapters/faraday'
+  Faraday.default_adapter = :typhoeus
+end
+
 options = {
   request: {
     timeout: 25

lib/diaspora/exceptions.rb

@@ -16,7 +16,6 @@ module Diaspora
   # that prevents further execution
   class NotMine < StandardError
   end
-  
 
   # Received a message without having a contact
   class ContactRequiredUnlessRequest < StandardError
@@ -30,4 +29,10 @@ module Diaspora
   # original XML message
   class AuthorXMLAuthorMismatch < StandardError
   end
+
+  # Tried to fetch a post but it was deleted, not valid
+  # or the remote end doesn't support post fetching
+  class PostNotFetchable < StandardError
+  end
+
 end

lib/diaspora/federated/relayable_retraction.rb

@@ -60,4 +60,8 @@ class RelayableRetraction < SignedRetraction
   def parent_author_signature_valid?
     verify_signature(self.parent_author_signature, self.parent.author)
   end
+
+  def parent_diaspora_handle
+    target.author.diaspora_handle
+  end
 end

lib/diaspora/fetcher.rb

@@ -1,5 +1,6 @@
 module Diaspora
   module Fetcher
     require 'diaspora/fetcher/public'
+    require 'diaspora/fetcher/single'
   end
 end

lib/diaspora/fetcher/single.rb

@@ -0,0 +1,41 @@
+module Diaspora
+  module Fetcher
+    module Single
+      module_function
+
+      # Fetch and store a remote public post
+      # @param [String] guid the remote posts guid
+      # @param [String] author_id Diaspora ID of a user known to have the post,
+      #                 preferably the author
+      # @yield [Post, Person] If a block is given it is yielded the post
+      #                       and the author prior save
+      # @return a saved post
+      def find_or_fetch_from_remote guid, author_id
+        post = Post.where(guid: guid).first
+        return post if post
+
+        post_author = Webfinger.new(author_id).fetch
+        post_author.save! unless post_author.persisted?
+
+        if fetched_post = fetch_post(post_author, guid)
+          yield fetched_post, post_author if block_given?
+          raise Diaspora::PostNotFetchable unless fetched_post.save
+        end
+
+        fetched_post
+      end
+
+      # Fetch a remote public post, used for receiving of unknown public posts
+      # @param [Person] author the remote post's author
+      # @param [String] guid the remote post's guid
+      # @return [Post] an unsaved remote post or false if the post was not found
+      def fetch_post author, guid
+        url = author.url + "/p/#{guid}.xml"
+        response = Faraday.get(url)
+        raise Diaspora::PostNotFetchable if response.status == 404 # Old pod, Friendika, deleted
+        raise "Failed to get #{url}" unless response.success? # Other error, N/A for example
+        Diaspora::Parser.from_xml(response.body)
+      end
+    end
+  end
+end

lib/diaspora/relayable.rb

@@ -51,7 +51,8 @@ module Diaspora
     end
 
     def parent_guid= new_parent_guid
-      self.parent = parent_class.where(:guid => new_parent_guid).first
+      @parent_guid = new_parent_guid
+      self.parent = parent_class.where(guid: new_parent_guid).first
     end
 
     # @return [Array<Person>]
@@ -66,7 +67,7 @@ module Diaspora
     end
 
     def receive(user, person=nil)
-      comment_or_like = self.class.where(:guid => self.guid).first || self
+      comment_or_like = self.class.where(guid: self.guid).first || self
 
       # Check to make sure the signature of the comment or like comes from the person claiming to author it
       unless comment_or_like.parent_author == user.person || comment_or_like.verify_parent_author_signature
@@ -134,5 +135,17 @@ module Diaspora
     def parent= parent
       raise NotImplementedError.new('you must override parent= in order to enable relayable on this model')
     end
+
+    # ROXML hook ensuring our own hooks are called
+    def after_parse
+      if @parent_guid
+        self.parent ||= fetch_parent(@parent_guid)
+      end
+    end
+
+    # Childs should override this to support fetching a missing parent
+    # @param guid the parents guid
+    def fetch_parent guid
+    end
   end
 end

lib/federated/relayable.rb

@@ -38,5 +38,9 @@ module Federated
     def parent= parent
       self.target = parent
     end
+
+    def fetch_parent guid
+      Diaspora::Fetcher::Single.find_or_fetch_from_remote guid, diaspora_handle
+    end
   end
-end
+end

lib/postzord/receiver/public.rb

@@ -26,15 +26,15 @@ class Postzord::Receiver::Public < Postzord::Receiver
     return false unless save_object
 
     FEDERATION_LOGGER.info("received a #{@object.inspect}")
-    if @object.respond_to?(:relayable?)
-      receive_relayable
-    elsif @object.is_a?(AccountDeletion)
-      #nothing
-    elsif @object.is_a?(SignedRetraction) # feels like a hack
+    if @object.is_a?(SignedRetraction) # feels like a hack
       self.recipient_user_ids.each do |user_id|
         user = User.where(id: user_id).first
         @object.perform user if user
       end
+    elsif @object.respond_to?(:relayable?)
+      receive_relayable
+    elsif @object.is_a?(AccountDeletion)
+      #nothing
     else
       Workers::ReceiveLocalBatch.perform_async(@object.class.to_s, @object.id, self.recipient_user_ids)
       true
@@ -57,6 +57,7 @@ class Postzord::Receiver::Public < Postzord::Receiver
   def save_object
     @object = Diaspora::Parser::from_xml(@salmon.parsed_data)
     raise "Object is not public" if object_can_be_public_and_it_is_not?
+    raise Diaspora::RelayableObjectWithoutParent if object_must_have_parent_and_does_not?
     raise Diaspora::AuthorXMLAuthorMismatch if author_does_not_match_xml_author?
     @object.save! if @object && @object.respond_to?(:save!)
     @object
@@ -88,4 +89,12 @@ class Postzord::Receiver::Public < Postzord::Receiver
   def object_can_be_public_and_it_is_not?
     @object.respond_to?(:public) && !@object.public?
   end
+
+  def object_must_have_parent_and_does_not?
+    if @object.respond_to?(:relayable?) # comment, like
+      @object.parent.nil?
+    else
+      false
+    end
+  end
 end

lib/webfinger.rb

@@ -31,7 +31,9 @@ class Webfinger
     Rails.logger.info("Getting: #{url} for #{account}")
     begin
       res = Faraday.get(url)
-      return false if res.status == 404
+      unless res.success?
+        raise "Failed to fetch #{url}: #{res.status}"
+      end
       res.body
     rescue OpenSSL::SSL::SSLError => e
       Rails.logger.info "Failed to fetch #{url}: SSL setup invalid"

spec/lib/diaspora/federated/relayable_retraction_spec.rb

@@ -98,6 +98,13 @@ describe RelayableRetraction do
         expect(Postzord::Dispatcher).not_to receive(:build)
         @retraction.receive(@recipient, @remote_raphael)
       end
+
+      it 'performs through postzord' do
+        xml = Salmon::Slap.create_by_user_and_activity(@local_luke, @retraction.to_diaspora_xml).xml_for(nil)
+        expect {
+          Postzord::Receiver::Public.new(xml).perform!
+        }.to change(Comment, :count).by(-1)
+      end
     end
   end
 

spec/lib/diaspora/fetcher/public_spec.rb

@@ -19,7 +19,7 @@ describe Diaspora::Fetcher::Public do
 
     stub_request(:get, /remote-testpod.net\/people\/.*/)
       .with(headers: {
-            'Accept'=>'application/json', 
+            'Accept'=>'application/json',
             'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3',
             'User-Agent'=>'diaspora-fetcher'
       }).to_return(:body => @fixture)

spec/lib/webfinger_spec.rb

@@ -36,7 +36,7 @@ describe Webfinger do
       Webfinger.in_background(account)
     end
   end
-  
+
   describe '#fetch' do
     it 'works' do
       finger = Webfinger.new(account_in_fixtures)
@@ -72,14 +72,15 @@ describe Webfinger do
 
       expect(a_request(:get, redirect_url)).to have_been_made
     end
-    
-    it 'returns false on 404' do
+
+    it 'raises on 404' do
       url ="https://bar.com/.well-known/host-meta"
       stub_request(:get, url).
         to_return(:status => 404, :body => nil)
 
-      expect(finger.get(url)).not_to eq(nil)
-      expect(finger.get(url)).to eq(false)
+      expect {
+        expect(finger.get(url)).to eq(false)
+      }.to raise_error
     end
   end
 
@@ -190,7 +191,7 @@ describe Webfinger do
       expect(Person).to receive(:create_from_webfinger).with("webfinger_profile", "hcard")
       finger.make_person_from_webfinger
     end
-    
+
     it 'with an false xrd it does not call Person.create_from_webfinger' do
       allow(finger).to receive(:webfinger_profile_xrd).and_return(false)
       expect(Person).not_to receive(:create_from_webfinger)

spec/models/comment_spec.rb

@@ -96,6 +96,13 @@ describe Comment, :type => :model do
       it 'marshals the post' do
         expect(@marshalled_comment.post).to eq(@post)
       end
+
+      it 'tries to fetch a missing parent' do
+        guid = @post.guid
+        @post.destroy
+        expect_any_instance_of(Comment).to receive(:fetch_parent).with(guid).and_return(nil)
+        Comment.from_xml(@xml)
+      end
     end
   end
 

spec/models/reshare_spec.rb

@@ -75,6 +75,13 @@ describe Reshare, :type => :model do
     it 'returns "Reshared" for the original post author' do
       expect(@reshare.notification_type(alice, @reshare.author)).to eq(Notifications::Reshared)
     end
+
+    it 'does not error out if the root was deleted' do
+      @reshare.root = nil
+      expect {
+        @reshare.notification_type(alice, @reshare.author)
+      }.to_not raise_error
+    end
   end
 
   describe '#absolute_root' do
@@ -83,7 +90,7 @@ describe Reshare, :type => :model do
       rs1 = FactoryGirl.build(:reshare, :root=>@sm)
       rs2 = FactoryGirl.build(:reshare, :root=>rs1)
       @rs3 = FactoryGirl.build(:reshare, :root=>rs2)
-      
+
      sm = FactoryGirl.create(:status_message, :author => alice.person, :public => true)
      rs1 = FactoryGirl.create(:reshare, :root => sm)
      @of_deleted = FactoryGirl.build(:reshare, :root => rs1)
@@ -194,13 +201,13 @@ describe Reshare, :type => :model do
         end
 
         context "fetching post" do
-          it "doesn't error out if the post is not found" do
+          it "raises if the post is not found" do
             allow(@response).to receive(:status).and_return(404)
             expect(Faraday.default_connection).to receive(:get).and_return(@response)
 
             expect {
               Reshare.from_xml(@xml)
-            }.to_not raise_error
+            }.to raise_error(Diaspora::PostNotFetchable)
           end
 
           it "raises if there's another error receiving the post" do

spec/workers/deferred_dispatch_spec.rb

@@ -0,0 +1,9 @@
+require 'spec_helper'
+
+describe Workers::DeferredDispatch do
+  it "handles non existing records gracefully" do
+    expect {
+      described_class.new.perform(alice.id, 'Comment', 0, {})
+    }.to_not raise_error
+  end
+end

spec/workers/process_photo_spec.rb

@@ -61,6 +61,12 @@ describe Workers::ProcessPhoto do
     expect{
       result = Workers::ProcessPhoto.new.perform(p.id)
     }.to_not raise_error
-    
+
+  end
+
+  it 'handles already deleted photos gracefully' do
+    expect {
+      Workers::ProcessPhoto.new.perform(0)
+    }.to_not raise_error
   end
 end