From f484eb957fb7f62414f76ea5b4a0c9147a5e2a21 Mon Sep 17 00:00:00 2001 From: zhitomirskiyi Date: Thu, 28 Oct 2010 12:13:39 -0700 Subject: [PATCH] comments now don't imbed the person in the xml --- app/models/comment.rb | 15 +++++- app/models/user.rb | 2 +- lib/diaspora/user/receiving.rb | 4 +- spec/lib/diaspora/parser_spec.rb | 25 +++------- spec/models/comment_spec.rb | 44 +++++++----------- spec/models/person_spec.rb | 4 +- spec/models/user/attack_vectors_spec.rb | 24 ---------- spec/models/user/receive_spec.rb | 61 +++++++++++++++---------- 8 files changed, 76 insertions(+), 103 deletions(-) diff --git a/app/models/comment.rb b/app/models/comment.rb index 540b44f2e..e6686efce 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -2,6 +2,14 @@ # licensed under the Affero General Public License version 3 or later. See # the COPYRIGHT file. +class HandleValidator < ActiveModel::Validator + def validate(document) + unless document.diaspora_handle == document.person.diaspora_handle + document.errors[:base] << "Diaspora handle and person handle must match" + end + end +end + class Comment include MongoMapper::Document include ROXML @@ -10,18 +18,21 @@ class Comment include Diaspora::Socketable xml_accessor :text - xml_accessor :person, :as => Person + xml_reader :diaspora_handle xml_accessor :post_id xml_accessor :_id key :text, String key :post_id, ObjectId key :person_id, ObjectId + key :diaspora_handle, String belongs_to :post, :class_name => "Post" belongs_to :person, :class_name => "Person" - validates_presence_of :text + validates_presence_of :text, :diaspora_handle + validates_with HandleValidator + timestamps! diff --git a/app/models/user.rb b/app/models/user.rb index 82d103034..6b183874d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -275,7 +275,7 @@ class User def build_comment(text, options = {}) raise "must comment on something!" unless options[:on] - comment = Comment.new(:person_id => self.person.id, :text => text, :post => options[:on]) + comment = Comment.new(:person_id => self.person.id, :diaspora_handle => self.person.diaspora_handle, :text => text, :post => options[:on]) comment.creator_signature = comment.sign_with_key(encryption_key) if comment.save comment diff --git a/lib/diaspora/user/receiving.rb b/lib/diaspora/user/receiving.rb index bcf8897f1..19732552f 100644 --- a/lib/diaspora/user/receiving.rb +++ b/lib/diaspora/user/receiving.rb @@ -44,6 +44,7 @@ module Diaspora elsif object.is_a? Profile sender = Diaspora::Parser.owner_id_from_xml xml elsif object.is_a?(Comment) + object.person = Person.by_webfinger(object.diaspora_handle) sender = (owns?(object.post))? object.person : object.post.person else sender = object.person @@ -82,14 +83,13 @@ module Diaspora end def receive_comment comment, xml - comment.person = Diaspora::Parser.parse_or_find_person_from_xml( xml ).save if comment.person.nil? raise "In receive for #{self.real_name}, signature was not valid on: #{comment.inspect}" unless comment.post.person == self.person || comment.verify_post_creator_signature self.visible_people = self.visible_people | [comment.person] self.save Rails.logger.debug("The person parsed from comment xml is #{comment.person.inspect}") unless comment.person.nil? comment.person.save Rails.logger.debug("From: #{comment.person.inspect}") if comment.person - comment.save + comment.save! unless owns?(comment) dispatch_comment comment end diff --git a/spec/lib/diaspora/parser_spec.rb b/spec/lib/diaspora/parser_spec.rb index 4ee82da6b..db980cf6f 100644 --- a/spec/lib/diaspora/parser_spec.rb +++ b/spec/lib/diaspora/parser_spec.rb @@ -13,31 +13,18 @@ describe Diaspora::Parser do let(:person) { user3.person } describe "parsing compliant XML object" do - it 'should be able to correctly handle comments with person in db' do + it 'should be able to correctly parse comment fields' do post = user.post :status_message, :message => "hello", :to => aspect.id - comment = Factory.build(:comment, :post => post, :person => person, :text => "Freedom!") + comment = Factory.create(:comment, :post => post, :person => person, :diaspora_handle => person.diaspora_handle, :text => "Freedom!") comment.delete xml = comment.to_diaspora_xml + puts xml comment_from_xml = Diaspora::Parser.from_xml(xml) - comment_from_xml.text.should == "Freedom!" - comment_from_xml.person.should == person + comment_from_xml.diaspora_handle person.diaspora_handle comment_from_xml.post.should == post - end - - it 'should be able to correctly handle person on a comment with person not in db' do - friend_users(user, aspect, user2, aspect2) - post = user.post :status_message, :message => "hello", :to => aspect.id - comment = user2.comment "Fool!", :on => post - - xml = comment.to_diaspora_xml - user2.delete - user2.person.delete - - parsed_person = Diaspora::Parser::parse_or_find_person_from_xml(xml) - parsed_person.save.should be true - parsed_person.diaspora_handle.should == user2.person.diaspora_handle - parsed_person.profile.should_not be_nil + comment_from_xml.text.should == "Freedom!" + comment_from_xml.should_not be comment end it 'should accept retractions' do diff --git a/spec/models/comment_spec.rb b/spec/models/comment_spec.rb index 66056789d..70b99582a 100644 --- a/spec/models/comment_spec.rb +++ b/spec/models/comment_spec.rb @@ -11,6 +11,13 @@ describe Comment do let(:user2) {Factory.create(:user)} let(:aspect2) {user2.aspect(:name => "Lame-faces")} + it 'validates that the handle belongs to the person' do + user_status = user.post(:status_message, :message => "hello", :to => aspect.id) + comment = Comment.new(:person_id => user2.person.id, :text => "hey", :post => user_status) + comment.valid? + comment.errors.full_messages.should include "Diaspora handle and person handle must match" + end + describe 'User#comment' do before do @status = user.post(:status_message, :message => "hello", :to => aspect.id) @@ -52,28 +59,6 @@ describe Comment do user.reload end - it 'should receive a comment from a person not on the pod' do - user3 = Factory.create(:user) - aspect3 = user3.aspect(:name => "blah") - - friend_users(user, aspect, user3, aspect3) - - comment = Comment.new(:person_id => user3.person.id, :text => "hey", :post => @user_status) - comment.creator_signature = comment.sign_with_key(user3.encryption_key) - comment.post_creator_signature = comment.sign_with_key(user.encryption_key) - - xml = user.salmon(comment).xml_for(user2) - - user3.person.delete - user3.delete - - @user_status.reload - @user_status.comments.should == [] - user2.receive_salmon(xml) - @user_status.reload - @user_status.comments.include?(comment).should be true - end - it "should send a user's comment on a person's post to that person" do User::QUEUE.should_receive(:add_post_request) user.comment "yo", :on => @person_status @@ -86,8 +71,9 @@ describe Comment do end it 'should send a comment a person made on your post to all people' do - comment = Comment.new(:person_id => @person.id, :text => "balls", :post => @user_status) + comment = Comment.new(:person_id => @person.id, :diaspora_handle => @person.diaspora_handle, :text => "cats", :post => @user_status) User::QUEUE.should_receive(:add_post_request).twice + Person.should_receive(:by_webfinger).and_return(@person) user.receive comment.to_diaspora_xml, @person end @@ -103,13 +89,13 @@ describe Comment do end it 'should not send a comment a person made on his own post to anyone' do User::QUEUE.should_not_receive(:add_post_request) - comment = Comment.new(:person_id => @person.id, :text => "balls", :post => @person_status) + comment = Comment.new(:person_id => @person.id, :diaspora_handle => @person.diaspora_handle, :text => "cats", :post => @person_status) user.receive comment.to_diaspora_xml, @person end it 'should not send a comment a person made on a person post to anyone' do User::QUEUE.should_not_receive(:add_post_request) - comment = Comment.new(:person_id => @person2.id, :text => "balls", :post => @person_status) + comment = Comment.new(:person_id => @person2.id, :diaspora_handle => @person.diaspora_handle, :text => "cats", :post => @person_status) user.receive comment.to_diaspora_xml, @person end after(:all) do @@ -119,7 +105,7 @@ describe Comment do it 'should not clear the aspect post array on receiving a comment' do aspect.post_ids.include?(@user_status.id).should be true - comment = Comment.new(:person_id => @person.id, :text => "balls", :post => @user_status) + comment = Comment.new(:person_id => @person.id, :diaspora_handle => @person.diaspora_handle, :text => "cats", :post => @user_status) user.receive comment.to_diaspora_xml, @person @@ -128,14 +114,16 @@ describe Comment do end end describe 'serialization' do - it 'should serialize the commenter' do + it 'should serialize the handle and not the sender' do commenter = Factory.create(:user) commenter_aspect = commenter.aspect :name => "bruisers" friend_users(user, aspect, commenter, commenter_aspect) post = user.post :status_message, :message => "hello", :to => aspect.id comment = commenter.comment "Fool!", :on => post comment.person.should_not == user.person - comment.to_diaspora_xml.include?(commenter.person.id.to_s).should be true + xml = comment.to_diaspora_xml + xml.include?(commenter.person.id.to_s).should be false + xml.include?(commenter.diaspora_handle).should be true end end diff --git a/spec/models/person_spec.rb b/spec/models/person_spec.rb index fa89610af..8fe333955 100644 --- a/spec/models/person_spec.rb +++ b/spec/models/person_spec.rb @@ -91,8 +91,8 @@ describe Person do status_message = Factory.create(:status_message, :person => @person) - Factory.create(:comment, :person_id => person.id, :text => "i love you", :post => status_message) - Factory.create(:comment, :person_id => @person.id, :text => "you are creepy", :post => status_message) + Factory.create(:comment, :person_id => person.id, :diaspora_handle => person.diaspora_handle, :text => "i love you", :post => status_message) + Factory.create(:comment, :person_id => @person.id,:diaspora_handle => @person.diaspora_handle, :text => "you are creepy", :post => status_message) lambda {person.destroy}.should_not change(Comment, :count) end diff --git a/spec/models/user/attack_vectors_spec.rb b/spec/models/user/attack_vectors_spec.rb index 36a586647..db5c613a2 100644 --- a/spec/models/user/attack_vectors_spec.rb +++ b/spec/models/user/attack_vectors_spec.rb @@ -79,29 +79,5 @@ describe "attack vectors" do user2.reload user2.profile.first_name.should == first_name end - - it 'should not overwrite another persons profile through comment' do - pending - user_status = user.post(:status_message, :message => "hi", :to => 'all') - comment = Comment.new(:person_id => user3.person.id, :text => "hey", :post => user_status) - - comment.creator_signature = comment.sign_with_key(user3.encryption_key) - comment.post_creator_signature = comment.sign_with_key(user.encryption_key) - - person = user3.person - original_url = person.url - original_id = person.id - puts original_url - - comment.person.url = "http://bad.com/" - user3.delete - person.delete - - comment.to_diaspora_xml.include?("bad.com").should be true - user2.receive_salmon(user.salmon(comment).xml_for(user2.person)) - - comment.person.url.should == original_url - Person.first(:id => original_id).url.should == original_url - end end end diff --git a/spec/models/user/receive_spec.rb b/spec/models/user/receive_spec.rb index e339c56fc..1ff2012e8 100644 --- a/spec/models/user/receive_spec.rb +++ b/spec/models/user/receive_spec.rb @@ -94,36 +94,47 @@ describe User do describe 'comments' do before do friend_users(user, aspect, user3, aspect3) + @post = user.post :status_message, :message => "hello", :to => aspect.id + + user2.receive @post.to_diaspora_xml, user.person + user3.receive @post.to_diaspora_xml, user.person + + @comment = user3.comment('tada',:on => @post) + @comment.post_creator_signature = @comment.sign_with_key(user.encryption_key) + @xml = user.salmon(@comment).xml_for(user2.person) + @comment.delete end + it 'should correctly attach the user already on the pod' do + local_person = user3.person + + user2.reload.raw_visible_posts.size.should == 1 + post_in_db = user2.raw_visible_posts.first + post_in_db.comments.should == [] + user2.receive_salmon(@xml) + post_in_db.reload + + post_in_db.comments.include?(@comment).should be true + post_in_db.comments.first.person.should == local_person + end + it 'should correctly marshal a stranger for the downstream user' do + remote_person = user3.person + remote_person.delete + user3.delete - post = user.post :status_message, :message => "hello", :to => aspect.id + Person.should_receive(:by_webfinger).twice.and_return{ |handle| if handle == user.person.diaspora_handle; user.person.save + user.person; else; remote_person.save; remote_person; end } - user2.receive post.to_diaspora_xml, user.person - user3.receive post.to_diaspora_xml, user.person - - comment = user2.comment('tada',:on => post) - user.receive comment.to_diaspora_xml, user2.person - user.reload - - commenter_id = user2.person.id - - user2.person.delete - user2.delete - comment_id = comment.id - - comment.delete - comment.post_creator_signature = comment.sign_with_key(user.encryption_key) - user3.receive comment.to_diaspora_xml, user.person - user3.reload - - new_comment = Comment.find_by_id(comment_id) - new_comment.should_not be_nil - new_comment.person.should_not be_nil - new_comment.person.profile.should_not be_nil - - user3.visible_person_by_id(commenter_id).should_not be_nil + + user2.reload.raw_visible_posts.size.should == 1 + post_in_db = user2.raw_visible_posts.first + post_in_db.comments.should == [] + user2.receive_salmon(@xml) + post_in_db.reload + + post_in_db.comments.include?(@comment).should be true + post_in_db.comments.first.person.should == remote_person end end