From 22a5832c01e207e33d815c1542dc26dce33bb063 Mon Sep 17 00:00:00 2001 From: Michael Sofaer and Raphael Sofaer Date: Thu, 23 Dec 2010 23:04:04 -0800 Subject: [PATCH] Fix retraction related failures in mysql --- app/controllers/status_messages_controller.rb | 2 +- app/helpers/sockets_helper.rb | 8 ++++- app/models/retraction.rb | 33 +++++++++---------- lib/diaspora/user/receiving.rb | 17 ++++++---- spec/models/aspect_spec.rb | 10 +++--- spec/models/user/attack_vectors_spec.rb | 26 ++++++++------- 6 files changed, 54 insertions(+), 42 deletions(-) diff --git a/app/controllers/status_messages_controller.rb b/app/controllers/status_messages_controller.rb index 9c3e365af..e4e161dc0 100644 --- a/app/controllers/status_messages_controller.rb +++ b/app/controllers/status_messages_controller.rb @@ -22,7 +22,7 @@ class StatusMessagesController < ApplicationController @status_message = current_user.build_post(:status_message, params[:status_message]) - if photos || @status_message.save!(:safe => true) + if photos || @status_message.save raise 'MongoMapper failed to catch a failed save' unless @status_message.id @status_message.photos += photos unless photos.nil? diff --git a/app/helpers/sockets_helper.rb b/app/helpers/sockets_helper.rb index bf1621615..8a7230ef4 100644 --- a/app/helpers/sockets_helper.rb +++ b/app/helpers/sockets_helper.rb @@ -6,7 +6,13 @@ module SocketsHelper include ApplicationHelper def obj_id(object) - object.respond_to?(:post_id) ? object.post_id : object.id + if object.respond_to?(:post_id) + object.post_id + elsif object.respond_to?(:post_guid) + object.post_guid + else + object.id + end end def action_hash(uid, object, opts={}) diff --git a/app/models/retraction.rb b/app/models/retraction.rb index a31531909..89e7d5985 100644 --- a/app/models/retraction.rb +++ b/app/models/retraction.rb @@ -6,7 +6,7 @@ class Retraction include ROXML include Diaspora::Webhooks - xml_accessor :post_id + xml_accessor :post_guid xml_accessor :diaspora_handle xml_accessor :type @@ -15,31 +15,30 @@ class Retraction def self.for(object) retraction = self.new if object.is_a? User - retraction.post_id = object.person.id + retraction.post_guid = object.person.guid retraction.type = object.person.class.to_s else - retraction.post_id = object.id + retraction.post_guid = object.guid retraction.type = object.class.to_s end retraction.diaspora_handle = object.diaspora_handle retraction end - def perform receiving_user_id - Rails.logger.debug "Performing retraction for #{post_id}" - if self.type.constantize.find_by_id(post_id) - unless Post.where(:diaspora_handle => person.diaspora_handle, :id => post_id).first - Rails.logger.info("event=retraction status=abort reason='no post found authored by retractor' sender=#{person.diaspora_handle} post_id=#{post_id}") - return - end + def target + @target ||= self.type.constantize.where(:guid => post_guid).first + end - begin - Rails.logger.debug("Retracting #{self.type} id: #{self.post_id}") - target = self.type.constantize.where(:id => self.post_id).first - target.unsocket_from_uid receiving_user_id if target.respond_to? :unsocket_from_uid - target.delete - rescue NameError - Rails.logger.info("event=retraction status=abort reason='unknown type'") + def perform receiving_user_id + Rails.logger.debug "Performing retraction for #{post_guid}" + if self.target + if self.target.person != self.person + Rails.logger.info("event=retraction status=abort reason='no post found authored by retractor' sender=#{person.diaspora_handle} post_id=#{post_guid}") + return + else + Rails.logger.info("event=retraction status=complete type=#{self.type} guid=#{self.post_guid}") + self.target.unsocket_from_uid receiving_user_id if target.respond_to? :unsocket_from_uid + self.target.delete end end end diff --git a/lib/diaspora/user/receiving.rb b/lib/diaspora/user/receiving.rb index 67e3f94ed..6a3d899ac 100644 --- a/lib/diaspora/user/receiving.rb +++ b/lib/diaspora/user/receiving.rb @@ -85,15 +85,14 @@ module Diaspora def receive_retraction retraction if retraction.type == 'Person' - unless retraction.person.id.to_s == retraction.post_id.to_s + unless retraction.person.guid.to_s == retraction.post_guid.to_s Rails.logger.info("event=receive status=abort reason='sender is not the person he is trying to retract' recipient=#{self.diaspora_handle} sender=#{retraction.person.diaspora_handle} payload_type=#{retraction.class} retraction_type=person") return end - disconnected_by Person.where(:id => retraction.post_id).first - else - retraction.perform self.id + disconnected_by Person.where(:guid => retraction.post_guid).first + elsif retraction.perform self.id aspects = self.aspects_with_person(retraction.person) - PostVisibility.where(:post_id => retraction.post_id, + PostVisibility.where(:post_id => retraction.target.id, :aspect_id => aspects.map{|a| a.id}).delete_all end retraction @@ -144,6 +143,9 @@ module Diaspora def post_visible?(post) raw_visible_posts.where(:guid => post.guid).first end + def posts_match(first, second) + (first.person == second.person) && (first.guid == second.guid) && (first.diaspora_handle == second.diaspora_handle) + end def receive_post post #exsists locally, but you dont know about it @@ -156,7 +158,10 @@ module Diaspora on_pod = existing_post(post) log_string = "event=receive payload_type=#{post.class} sender=#{post.diaspora_handle} " if on_pod - if post_visible?(post) + if !posts_match(on_pod, post) + Rails.logger.info(log_string << "update=false status=abort reason='incoming post does not patch existing post' existing_post=#{on_pod.id}") + return + elsif post_visible?(post) if post.mutable? on_pod.caption = post.caption on_pod.save! diff --git a/spec/models/aspect_spec.rb b/spec/models/aspect_spec.rb index f036166d4..a80af2750 100644 --- a/spec/models/aspect_spec.rb +++ b/spec/models/aspect_spec.rb @@ -18,29 +18,29 @@ describe Aspect do describe 'creation' do let!(:aspect){user.aspects.create(:name => 'losers')} - it 'should have a name' do + it 'has a name' do aspect.name.should == "losers" end - it 'should not allow duplicate names' do + it 'does not allow duplicate names' do lambda { invalid_aspect = user.aspects.create(:name => "losers ") }.should_not change(Aspect, :count) end - it 'should have a limit of 20 characters' do + it 'has a 20 character limit on names' do aspect = Aspect.new(:name => "this name is really too too too too too long") aspect.valid?.should == false end - it 'should be able to have other users' do + it 'is able to have other users as contacts' do Contact.create(:user => user, :person => user2.person, :aspects => [aspect]) aspect.contacts.where(:person_id => user.person.id).should be_empty aspect.contacts.where(:person_id => user2.person.id).should_not be_empty aspect.contacts.size.should == 1 end - it 'should be able to have users and people' do + it 'is able to have users and people and contacts' do contact1 = Contact.create(:user => user, :person => user2.person, :aspects => [aspect]) contact2 = Contact.create(:user => user, :person => connected_person_2, :aspects => [aspect]) aspect.contacts.include?(contact1).should be_true diff --git a/spec/models/user/attack_vectors_spec.rb b/spec/models/user/attack_vectors_spec.rb index 4968410e2..7c11d7aaf 100644 --- a/spec/models/user/attack_vectors_spec.rb +++ b/spec/models/user/attack_vectors_spec.rb @@ -40,8 +40,9 @@ describe "attack vectors" do original_message = user2.post :status_message, :message => 'store this!', :to => aspect2.id original_message.diaspora_handle = user.diaspora_handle + user3.activate_contact(user2.person, user3.aspects.first) user3.receive_salmon(user.salmon(original_message).xml_for(user3.person)) - user3.reload.visible_posts.should_not include(original_message) + user3.reload.visible_posts.should_not include(StatusMessage.find(original_message.id)) end context 'malicious contact attack vector' do @@ -56,10 +57,8 @@ describe "attack vectors" do user.receive_salmon(user2.salmon(original_message).xml_for(user.person)) - lambda { - malicious_message = Factory.build( :status_message, :id => original_message.id, :message => 'BAD!!!', :person => user3.person) - user.receive_salmon(user3.salmon(malicious_message).xml_for(user.person)) - }.should_not change{user.reload.raw_visible_posts.count} + malicious_message = Factory.build( :status_message, :id => original_message.id, :message => 'BAD!!!', :person => user3.person) + user.receive_salmon(user3.salmon(malicious_message).xml_for(user.person)) original_message.reload.message.should == "store this!" user.raw_visible_posts.first.message.should == "store this!" @@ -94,15 +93,16 @@ describe "attack vectors" do original_message = user2.post :status_message, :message => 'store this!', :to => aspect2.id user.receive_salmon(user2.salmon(original_message).xml_for(user.person)) user.raw_visible_posts.count.should be 1 + StatusMessage.count.should == 1 ret = Retraction.new - ret.post_id = original_message.id + ret.post_guid = original_message.guid ret.diaspora_handle = user3.person.diaspora_handle ret.type = original_message.class.to_s user.receive_salmon(user3.salmon(ret).xml_for(user.person)) StatusMessage.count.should be 1 - user.reload.raw_visible_posts.count.should be 1 + user.raw_visible_posts.count.should be 1 end it "disregards retractions for non-existent posts that are from someone other than the post's author" do @@ -110,14 +110,16 @@ describe "attack vectors" do id = original_message.reload.id ret = Retraction.new - ret.post_id = original_message.id + ret.post_guid = original_message.guid ret.diaspora_handle = user3.person.diaspora_handle ret.type = original_message.class.to_s original_message.delete StatusMessage.count.should be 0 - proc{ user.receive_salmon(user3.salmon(ret).xml_for(user.person)) }.should_not raise_error + proc{ + user.receive_salmon(user3.salmon(ret).xml_for(user.person)) + }.should_not raise_error end it 'should not receive retractions where the retractor and the salmon author do not match' do @@ -126,7 +128,7 @@ describe "attack vectors" do user.raw_visible_posts.count.should == 1 ret = Retraction.new - ret.post_id = original_message.id + ret.post_guid = original_message.guid ret.diaspora_handle = user2.person.diaspora_handle ret.type = original_message.class.to_s @@ -138,7 +140,7 @@ describe "attack vectors" do it 'it should not allow you to send retractions for other people' do ret = Retraction.new - ret.post_id = user2.person.id + ret.post_guid = user2.person.guid ret.diaspora_handle = user3.person.diaspora_handle ret.type = user2.person.class.to_s @@ -149,7 +151,7 @@ describe "attack vectors" do it 'it should not allow you to send retractions with xml and salmon handle mismatch' do ret = Retraction.new - ret.post_id = user2.person.id + ret.post_guid = user2.person.guid ret.diaspora_handle = user2.person.diaspora_handle ret.type = user2.person.class.to_s