From d891e78652850fdace28a1f53e322b4752ca868a Mon Sep 17 00:00:00 2001 From: Ilya Zhitomirskiy Date: Wed, 5 Oct 2011 20:05:53 -0700 Subject: [PATCH] refactored post receiving; only update cache on verfied received objects --- app/models/post.rb | 108 +++++++++++++-------- lib/postzord/receiver.rb | 5 +- lib/postzord/receiver/local_batch.rb | 2 + lib/postzord/receiver/private.rb | 2 +- lib/postzord/receiver/public.rb | 3 +- spec/lib/postzord/receiver/private_spec.rb | 10 +- spec/lib/postzord/receiver/public_spec.rb | 5 + spec/lib/postzord/receiver_spec.rb | 13 ++- spec/models/post_spec.rb | 82 +++++++++++++++- 9 files changed, 175 insertions(+), 55 deletions(-) diff --git a/app/models/post.rb b/app/models/post.rb index 15718a6c9..d466aae6b 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -96,46 +96,6 @@ class Post < ActiveRecord::Base end end - # @param [User] user The user that is receiving this post. - # @param [Person] person The person who dispatched this post to the - # @return [void] - def receive(user, person) - #exists locally, but you dont know about it - #does not exsist locally, and you dont know about it - #exists_locally? - #you know about it, and it is mutable - #you know about it, and it is not mutable - self.class.transaction do - local_post = self.class.where(:guid => self.guid).first - if local_post && local_post.author_id == self.author_id - known_post = user.find_visible_post_by_id(self.guid, :key => :guid) - if known_post - if known_post.mutable? - known_post.update_attributes(self.attributes) - else - Rails.logger.info("event=receive payload_type=#{self.class} update=true status=abort sender=#{self.diaspora_handle} reason=immutable existing_post=#{known_post.id}") - end - else - user.contact_for(person).receive_post(local_post) - user.notify_if_mentioned(local_post) - Rails.logger.info("event=receive payload_type=#{self.class} update=true status=complete sender=#{self.diaspora_handle} existing_post=#{local_post.id}") - return local_post - end - elsif !local_post - if self.save - user.contact_for(person).receive_post(self) - user.notify_if_mentioned(self) - Rails.logger.info("event=receive payload_type=#{self.class} update=false status=complete sender=#{self.diaspora_handle}") - return self - else - Rails.logger.info("event=receive payload_type=#{self.class} update=false status=abort sender=#{self.diaspora_handle} reason=#{self.errors.full_messages}") - end - else - Rails.logger.info("event=receive payload_type=#{self.class} update=true status=abort sender=#{self.diaspora_handle} reason='update not from post owner' existing_post=#{self.id}") - end - end - end - def activity_streams? false end @@ -173,4 +133,72 @@ class Post < ActiveRecord::Base self.triggers_caching? && RedisCache.configured? && RedisCache.acceptable_types.include?(self.type) && user = self.author.owner end + + + # @param [User] user The user that is receiving this post. + # @param [Person] person The person who dispatched this post to the + # @return [void] + def receive(user, person) + #exists locally, but you dont know about it + #does not exsist locally, and you dont know about it + #exists_locally? + #you know about it, and it is mutable + #you know about it, and it is not mutable + self.class.transaction do + local_post = persisted_post + + if local_post && verify_persisted_post(local_post) + self.receive_persisted(user, person, local_post) + + elsif !local_post + self.receive_non_persisted(user, person) + + else + Rails.logger.info("event=receive payload_type=#{self.class} update=true status=abort sender=#{self.diaspora_handle} reason='update not from post owner' existing_post=#{self.id}") + false + end + end + end + + protected + + # @return [Post,void] + def persisted_post + self.class.where(:guid => self.guid).first + end + + # @return [Boolean] + def verify_persisted_post(persisted_post) + persisted_post.author_id == self.author_id + end + + def receive_persisted(user, person, local_post) + known_post = user.find_visible_post_by_id(self.guid, :key => :guid) + if known_post + if known_post.mutable? + known_post.update_attributes(self.attributes) + true + else + Rails.logger.info("event=receive payload_type=#{self.class} update=true status=abort sender=#{self.diaspora_handle} reason=immutable") #existing_post=#{known_post.id}") + false + end + else + user.contact_for(person).receive_post(local_post) + user.notify_if_mentioned(local_post) + Rails.logger.info("event=receive payload_type=#{self.class} update=true status=complete sender=#{self.diaspora_handle}") #existing_post=#{local_post.id}") + true + end + end + + def receive_non_persisted(user, person) + if self.save + user.contact_for(person).receive_post(self) + user.notify_if_mentioned(self) + Rails.logger.info("event=receive payload_type=#{self.class} update=false status=complete sender=#{self.diaspora_handle}") + true + else + Rails.logger.info("event=receive payload_type=#{self.class} update=false status=abort sender=#{self.diaspora_handle} reason=#{self.errors.full_messages}") + false + end + end end diff --git a/lib/postzord/receiver.rb b/lib/postzord/receiver.rb index 801a66baa..452b4975e 100644 --- a/lib/postzord/receiver.rb +++ b/lib/postzord/receiver.rb @@ -8,8 +8,9 @@ class Postzord::Receiver require File.join(Rails.root, 'lib/postzord/receiver/public') def perform! - receive! - update_cache! if cache? + if self.receive! + self.update_cache! if cache? + end end # @return [Boolean] diff --git a/lib/postzord/receiver/local_batch.rb b/lib/postzord/receiver/local_batch.rb index 240da8309..e1423d02a 100644 --- a/lib/postzord/receiver/local_batch.rb +++ b/lib/postzord/receiver/local_batch.rb @@ -23,6 +23,8 @@ class Postzord::Receiver::LocalBatch < Postzord::Receiver # 09/27/11 this is slow #socket_to_users if @object.respond_to?(:socket_to_user) notify_users + + true end def update_cache! diff --git a/lib/postzord/receiver/private.rb b/lib/postzord/receiver/private.rb index a1db66d96..aa1a8dc76 100644 --- a/lib/postzord/receiver/private.rb +++ b/lib/postzord/receiver/private.rb @@ -23,7 +23,7 @@ class Postzord::Receiver::Private < Postzord::Receiver parse_and_receive(salmon.parsed_data) else Rails.logger.info("event=receive status=abort recipient=#{@user.diaspora_handle} sender=#{@salmon.author_id} reason='not_verified for key'") - nil + false end end diff --git a/lib/postzord/receiver/public.rb b/lib/postzord/receiver/public.rb index a5e55b1a0..90135be8a 100644 --- a/lib/postzord/receiver/public.rb +++ b/lib/postzord/receiver/public.rb @@ -19,12 +19,13 @@ class Postzord::Receiver::Public < Postzord::Receiver # @return [void] def receive! return false unless verified_signature? - return unless save_object + return false unless save_object if @object.respond_to?(:relayable?) receive_relayable else Resque.enqueue(Jobs::ReceiveLocalBatch, @object.class.to_s, @object.id, self.recipient_user_ids) + true end end diff --git a/spec/lib/postzord/receiver/private_spec.rb b/spec/lib/postzord/receiver/private_spec.rb index 8fe6779cc..cc8104662 100644 --- a/spec/lib/postzord/receiver/private_spec.rb +++ b/spec/lib/postzord/receiver/private_spec.rb @@ -53,28 +53,28 @@ describe Postzord::Receiver::Private do @salmon = @zord.instance_variable_get(:@salmon) end - context 'returns nil' do + context 'returns false' do it 'if the salmon author does not exist' do @zord.instance_variable_set(:@sender, nil) - @zord.perform!.should be_nil + @zord.receive!.should == false end it 'if the author does not match the signature' do @zord.instance_variable_set(:@sender, Factory(:person)) - @zord.perform!.should be_nil + @zord.receive!.should == false end end context 'returns the sent object' do it 'returns the received object on success' do - @zord.perform! + @zord.receive! @zord.instance_variable_get(:@object).should respond_to(:to_diaspora_xml) end end it 'parses the salmon object' do Diaspora::Parser.should_receive(:from_xml).with(@salmon.parsed_data).and_return(@original_post) - @zord.perform! + @zord.receive! end end diff --git a/spec/lib/postzord/receiver/public_spec.rb b/spec/lib/postzord/receiver/public_spec.rb index 9b0532149..9fad05edc 100644 --- a/spec/lib/postzord/receiver/public_spec.rb +++ b/spec/lib/postzord/receiver/public_spec.rb @@ -44,6 +44,11 @@ describe Postzord::Receiver::Public do @receiver.perform! end + it 'returns false if signature is not verified' do + @receiver.should_receive(:verified_signature?).and_return(false) + @receiver.perform!.should be_false + end + context 'if signature is valid' do it 'calls recipient_user_ids' do @receiver.should_receive(:recipient_user_ids) diff --git a/spec/lib/postzord/receiver_spec.rb b/spec/lib/postzord/receiver_spec.rb index 669b000fe..163870f19 100644 --- a/spec/lib/postzord/receiver_spec.rb +++ b/spec/lib/postzord/receiver_spec.rb @@ -13,7 +13,7 @@ describe Postzord::Receiver do describe "#perform!" do before do - @receiver.stub(:receive!) + @receiver.stub(:receive!).and_return(true) end it 'calls receive!' do @@ -22,8 +22,11 @@ describe Postzord::Receiver do end context 'update_cache!' do - it "gets called if cache?" do + before do @receiver.stub(:cache?).and_return(true) + end + + it "gets called if cache?" do @receiver.should_receive(:update_cache!) @receiver.perform! end @@ -33,6 +36,12 @@ describe Postzord::Receiver do @receiver.should_not_receive(:update_cache!) @receiver.perform! end + + it 'does not get called if receive! is false' do + @receiver.stub(:receive!).and_return(false) + @receiver.should_not_receive(:update_cache!) + @receiver.perform! + end end end diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb index 48523f180..78bf73f04 100644 --- a/spec/models/post_spec.rb +++ b/spec/models/post_spec.rb @@ -199,9 +199,83 @@ describe Post do @post.should_cache_for_author?.should be_false end end + + describe "#receive" do + it 'returns false if the post does not verify' do + @post = Factory(:status_message, :author => bob.person) + @post.should_receive(:verify_persisted_post).and_return(false) + @post.receive(bob, eve.person).should == false + end + end - #user exists, - # cache configured, - # triggers caching, - # of the cachable type + describe "#receive_persisted" do + before do + @post = Factory.build(:status_message, :author => bob.person) + @known_post = Post.new + bob.stub(:contact_for).with(eve.person).and_return(stub(:receive_post => true)) + end + + context "user knows about the post" do + before do + bob.stub(:find_visible_post_by_id).and_return(@known_post) + end + + it 'updates attributes only if mutable' do + @known_post.stub(:mutable?).and_return(true) + @known_post.should_receive(:update_attributes) + @post.send(:receive_persisted, bob, eve.person, @known_post).should == true + end + + it 'returns false if trying to update a non-mutable object' do + @known_post.stub(:mutable?).and_return(false) + @known_post.should_not_receive(:update_attributes) + @post.send(:receive_persisted, bob, eve.person, @known_post).should == false + end + end + + context "the user does not know about the post" do + before do + bob.stub(:find_visible_post_by_id).and_return(nil) + bob.stub(:notify_if_mentioned).and_return(true) + end + + it "receives the post from the contact of the author" do + @post.send(:receive_persisted, bob, eve.person, @known_post).should == true + end + + it 'notifies the user if they are mentioned' do + bob.stub(:contact_for).with(eve.person).and_return(stub(:receive_post => true)) + bob.should_receive(:notify_if_mentioned).and_return(true) + + @post.send(:receive_persisted, bob, eve.person, @known_post).should == true + end + end + end + + describe '#receive_non_persisted' do + context "the user does not know about the post" do + before do + @post = Factory.build(:status_message, :author => bob.person) + bob.stub(:find_visible_post_by_id).and_return(nil) + bob.stub(:notify_if_mentioned).and_return(true) + end + + it "it receives the post from the contact of the author" do + bob.should_receive(:contact_for).with(eve.person).and_return(stub(:receive_post => true)) + @post.send(:receive_non_persisted, bob, eve.person).should == true + end + + it 'notifies the user if they are mentioned' do + bob.stub(:contact_for).with(eve.person).and_return(stub(:receive_post => true)) + bob.should_receive(:notify_if_mentioned).and_return(true) + + @post.send(:receive_non_persisted, bob, eve.person).should == true + end + + it 'returns false if the post does not save' do + @post.stub(:save).and_return(false) + @post.send(:receive_non_persisted, bob, eve.person).should == false + end + end + end end