From c225bb17ce8d40fe2f61d83990436a93c1164adf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonne=20Ha=C3=9F?= Date: Sat, 6 Sep 2014 23:13:53 +0200 Subject: [PATCH 1/9] add pry and pry-debundle to the Gemfile If you now install pry-byebug and pry-stackexplorer, you'll get a nice debugger --- Gemfile | 4 ++++ Gemfile.lock | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/Gemfile b/Gemfile index d733084ef..931bd87fc 100644 --- a/Gemfile +++ b/Gemfile @@ -187,6 +187,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 diff --git a/Gemfile.lock b/Gemfile.lock index 27fb15667..648273b2a 100644 --- a/Gemfile.lock +++ b/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) @@ -554,6 +556,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) From 9c88fde82139ee8a094932e35fa49c4e00b6fb0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonne=20Ha=C3=9F?= Date: Sat, 6 Sep 2014 23:15:44 +0200 Subject: [PATCH 2/9] Switch Faraday adapter to typhoeus It uses curl which has less problems connecting to a missconfigured IPv6 host (falls back to v4) --- config/initializers/faraday.rb | 7 +++++++ spec/lib/diaspora/fetcher/public_spec.rb | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/config/initializers/faraday.rb b/config/initializers/faraday.rb index ad1384f6d..d22e92e35 100644 --- a/config/initializers/faraday.rb +++ b/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 diff --git a/spec/lib/diaspora/fetcher/public_spec.rb b/spec/lib/diaspora/fetcher/public_spec.rb index 43f249cab..28726b2e4 100644 --- a/spec/lib/diaspora/fetcher/public_spec.rb +++ b/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) From 777e3123d6968fe4f6c5c2f4032220918c59d3de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonne=20Ha=C3=9F?= Date: Sat, 6 Sep 2014 23:19:57 +0200 Subject: [PATCH 3/9] Try fetching missing parent of relayables * Extract post fetching logic from Reshare into its own module * raise proper error message when fetching fails * raise proper error message when parent is still missing We can't skip fetch failures or missing parents and still need to retry them in case we're sent the parent later on --- app/models/reshare.rb | 31 +++++-------------------- lib/diaspora/exceptions.rb | 7 +++++- lib/diaspora/fetcher.rb | 1 + lib/diaspora/fetcher/single.rb | 41 +++++++++++++++++++++++++++++++++ lib/diaspora/relayable.rb | 17 ++++++++++++-- lib/federated/relayable.rb | 6 ++++- lib/postzord/receiver/public.rb | 9 ++++++++ spec/models/comment_spec.rb | 7 ++++++ spec/models/reshare_spec.rb | 6 ++--- 9 files changed, 93 insertions(+), 32 deletions(-) create mode 100644 lib/diaspora/fetcher/single.rb diff --git a/app/models/reshare.rb b/app/models/reshare.rb index 5bb5326d5..33581d05e 100644 --- a/app/models/reshare.rb +++ b/app/models/reshare.rb @@ -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." diff --git a/lib/diaspora/exceptions.rb b/lib/diaspora/exceptions.rb index eba8366e0..2a9da47a2 100644 --- a/lib/diaspora/exceptions.rb +++ b/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 diff --git a/lib/diaspora/fetcher.rb b/lib/diaspora/fetcher.rb index adb82e552..a27831e2f 100644 --- a/lib/diaspora/fetcher.rb +++ b/lib/diaspora/fetcher.rb @@ -1,5 +1,6 @@ module Diaspora module Fetcher require 'diaspora/fetcher/public' + require 'diaspora/fetcher/single' end end diff --git a/lib/diaspora/fetcher/single.rb b/lib/diaspora/fetcher/single.rb new file mode 100644 index 000000000..5dfeb7f73 --- /dev/null +++ b/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 diff --git a/lib/diaspora/relayable.rb b/lib/diaspora/relayable.rb index d0db312b7..4f43ed402 100644 --- a/lib/diaspora/relayable.rb +++ b/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] @@ -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 diff --git a/lib/federated/relayable.rb b/lib/federated/relayable.rb index d852a4dcd..a6cf6bf83 100644 --- a/lib/federated/relayable.rb +++ b/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 \ No newline at end of file +end diff --git a/lib/postzord/receiver/public.rb b/lib/postzord/receiver/public.rb index 686e9a5b3..0243b72f5 100644 --- a/lib/postzord/receiver/public.rb +++ b/lib/postzord/receiver/public.rb @@ -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 diff --git a/spec/models/comment_spec.rb b/spec/models/comment_spec.rb index a377d142a..efc7eacdc 100644 --- a/spec/models/comment_spec.rb +++ b/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 diff --git a/spec/models/reshare_spec.rb b/spec/models/reshare_spec.rb index 89dfd85f6..ce05d3b6b 100644 --- a/spec/models/reshare_spec.rb +++ b/spec/models/reshare_spec.rb @@ -83,7 +83,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 +194,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 From 619bc3f5379dad2f7f49e3fd2b8a8b4d2e5d296d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonne=20Ha=C3=9F?= Date: Sun, 7 Sep 2014 00:45:12 +0200 Subject: [PATCH 4/9] A reshares root may be already gone when generating the notification --- app/models/reshare.rb | 2 +- spec/models/reshare_spec.rb | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/app/models/reshare.rb b/app/models/reshare.rb index 33581d05e..869d73351 100644 --- a/app/models/reshare.rb +++ b/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 diff --git a/spec/models/reshare_spec.rb b/spec/models/reshare_spec.rb index ce05d3b6b..929ded2b9 100644 --- a/spec/models/reshare_spec.rb +++ b/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 From 4b754b51f9007772eb52177c291dac5811dba25c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonne=20Ha=C3=9F?= Date: Sat, 20 Sep 2014 13:39:09 +0200 Subject: [PATCH 5/9] Discard dispatch jobs of already deleted records --- app/workers/deferred_dispatch.rb | 1 + spec/workers/deferred_dispatch_spec.rb | 9 +++++++++ 2 files changed, 10 insertions(+) create mode 100644 spec/workers/deferred_dispatch_spec.rb diff --git a/app/workers/deferred_dispatch.rb b/app/workers/deferred_dispatch.rb index cbb87581a..46fa894c3 100644 --- a/app/workers/deferred_dispatch.rb +++ b/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 diff --git a/spec/workers/deferred_dispatch_spec.rb b/spec/workers/deferred_dispatch_spec.rb new file mode 100644 index 000000000..f0a20d768 --- /dev/null +++ b/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 From 5ca1c1d2951f35d2443eebc2e7f456eee2fc907d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonne=20Ha=C3=9F?= Date: Sat, 20 Sep 2014 13:47:52 +0200 Subject: [PATCH 6/9] Raise on 404 during Webfinger That's at least readable, return false just causes silly follow up errors --- lib/webfinger.rb | 4 +++- spec/lib/webfinger_spec.rb | 13 +++++++------ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/lib/webfinger.rb b/lib/webfinger.rb index 93e2cf5f6..385c2b40b 100644 --- a/lib/webfinger.rb +++ b/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" diff --git a/spec/lib/webfinger_spec.rb b/spec/lib/webfinger_spec.rb index 25104bb37..c3515bebe 100644 --- a/spec/lib/webfinger_spec.rb +++ b/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) From 499ff6e0f463d7fdd53119d76b1aa444f0ddd7c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonne=20Ha=C3=9F?= Date: Sat, 20 Sep 2014 15:03:53 +0200 Subject: [PATCH 7/9] Fix receiving a relayable retraction through the public route --- lib/diaspora/federated/relayable_retraction.rb | 4 ++++ lib/postzord/receiver/public.rb | 10 +++++----- .../diaspora/federated/relayable_retraction_spec.rb | 7 +++++++ 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/lib/diaspora/federated/relayable_retraction.rb b/lib/diaspora/federated/relayable_retraction.rb index ab7be6528..3d48f6763 100644 --- a/lib/diaspora/federated/relayable_retraction.rb +++ b/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 diff --git a/lib/postzord/receiver/public.rb b/lib/postzord/receiver/public.rb index 0243b72f5..d3f27be77 100644 --- a/lib/postzord/receiver/public.rb +++ b/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 diff --git a/spec/lib/diaspora/federated/relayable_retraction_spec.rb b/spec/lib/diaspora/federated/relayable_retraction_spec.rb index baf88a6eb..6e8635a24 100644 --- a/spec/lib/diaspora/federated/relayable_retraction_spec.rb +++ b/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 From d6f5368474240a394a5c952769480cad69093a89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonne=20Ha=C3=9F?= Date: Sat, 20 Sep 2014 15:12:56 +0200 Subject: [PATCH 8/9] Handle already deleted photos gracefully in process photo job --- app/workers/process_photo.rb | 1 + spec/workers/process_photo_spec.rb | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/app/workers/process_photo.rb b/app/workers/process_photo.rb index 00d959962..2fcdd3943 100644 --- a/app/workers/process_photo.rb +++ b/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 diff --git a/spec/workers/process_photo_spec.rb b/spec/workers/process_photo_spec.rb index 8294902ca..8e28fba74 100644 --- a/spec/workers/process_photo_spec.rb +++ b/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 From 9520d06c7bc1a2e2001820245b8147084f4d7d94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonne=20Ha=C3=9F?= Date: Sat, 20 Sep 2014 15:26:31 +0200 Subject: [PATCH 9/9] Handle already deleted items gracefully in receive local batch job --- app/workers/receive_local_batch.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/workers/receive_local_batch.rb b/app/workers/receive_local_batch.rb index 81d8a9ecf..adcf312aa 100644 --- a/app/workers/receive_local_batch.rb +++ b/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