comments now don't imbed the person in the xml

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!
 

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

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

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.diaspora_handle person.diaspora_handle
-      comment_from_xml.person.should == person
       comment_from_xml.post.should == post
-    end
+      comment_from_xml.text.should == "Freedom!"
-
+      comment_from_xml.should_not be comment
-    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
     end
 
     it 'should accept retractions' do

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
 

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, :diaspora_handle => person.diaspora_handle, :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 => "you are creepy", :post => status_message)
     
     lambda {person.destroy}.should_not change(Comment, :count)
   end

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

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)
+      user2.reload.raw_visible_posts.size.should == 1
-      user.receive comment.to_diaspora_xml, user2.person
+      post_in_db = user2.raw_visible_posts.first
-      user.reload
+      post_in_db.comments.should == []
+      user2.receive_salmon(@xml)
+      post_in_db.reload
       
-      commenter_id = user2.person.id
+      post_in_db.comments.include?(@comment).should be true
-
+      post_in_db.comments.first.person.should == remote_person
-      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
     end
   end