better error message if we webfinger a person with invalid guid

this happens if a remote person changed the diaspora-id manually (renamed
domain or something) in their database.

app/models/person.rb

@@ -51,6 +51,7 @@ class Person < ActiveRecord::Base
   has_many :mentions, :dependent => :destroy
 
   validate :owner_xor_pod
+  validate :other_person_with_same_guid, on: :create
   validates :profile, :presence => true
   validates :serialized_public_key, :presence => true
   validates :diaspora_handle, :uniqueness => true
@@ -301,4 +302,9 @@ class Person < ActiveRecord::Base
   def owner_xor_pod
     errors.add(:base, "Specify an owner or a pod, not both") unless owner.blank? ^ pod.blank?
   end
+
+  def other_person_with_same_guid
+    diaspora_id = Person.where(guid: guid).where.not(diaspora_handle: diaspora_handle).pluck(:diaspora_handle).first
+    errors.add(:base, "Person with same GUID already exists: #{diaspora_id}") if diaspora_id
+  end
 end

spec/federation_callbacks_spec.rb

@@ -130,6 +130,15 @@ describe "diaspora federation callbacks" do
         expect(profile_entity.image_url_small).to eq(profile.image_url_small)
         expect(profile_entity.searchable).to eq(profile.searchable)
       end
+
+      it "raises an error if a person with the same GUID already exists" do
+        person_data = FactoryGirl.attributes_for(:federation_person_from_webfinger).merge(guid: alice.guid)
+        person = DiasporaFederation::Entities::Person.new(person_data)
+
+        expect {
+          DiasporaFederation.callbacks.trigger(:save_person_after_webfinger, person)
+        }.to raise_error ActiveRecord::RecordInvalid, /Person with same GUID already exists: #{alice.diaspora_handle}/
+      end
     end
 
     context "update profile" do

spec/helpers/people_helper_spec.rb

@@ -86,10 +86,8 @@ describe PeopleHelper, :type => :helper do
 
     it "links by id if there is a period in the user's username" do
       @user.username = "invalid.username"
-      expect(@user.save(:validate => false)).to eq(true)
+      @user.person.diaspora_handle = "#{@user.username}@#{AppConfig.pod_uri.authority}"
-      person = @user.person
+      expect(@user.save(validate: false)).to eq(true)
-      person.diaspora_handle = "#{@user.username}@#{AppConfig.pod_uri.authority}"
-      person.save!
 
       expect(local_or_remote_person_path(@user.person)).to eq(person_path(@user.person))
     end

spec/models/person_spec.rb

@@ -430,19 +430,17 @@ describe Person, :type => :model do
         expect(person).to eq(user1.person)
       end
 
-      it 'should only find people who are exact matches (1/2)' do
+      it "should only find people who are exact matches (1/2)" do
-        user = FactoryGirl.create(:user, :username => "SaMaNtHa")
+        FactoryGirl.create(:person, diaspora_handle: "tomtom@tom.joindiaspora.com")
-        person = FactoryGirl.create(:person, :diaspora_handle => "tomtom@tom.joindiaspora.com")
+        FactoryGirl.create(:person, diaspora_handle: "tom@tom.joindiaspora.com")
-        user.person.diaspora_handle = "tom@tom.joindiaspora.com"
+        expect(Person.by_account_identifier("tom@tom.joindiaspora.com").diaspora_handle)
-        user.person.save
+          .to eq("tom@tom.joindiaspora.com")
-        expect(Person.by_account_identifier("tom@tom.joindiaspora.com").diaspora_handle).to eq("tom@tom.joindiaspora.com")
       end
 
-      it 'should only find people who are exact matches (2/2)' do
+      it "should only find people who are exact matches (2/2)" do
-        person = FactoryGirl.create(:person, :diaspora_handle => "tomtom@tom.joindiaspora.com")
+        FactoryGirl.create(:person, diaspora_handle: "tomtom@tom.joindiaspora.com")
-        person1 = FactoryGirl.create(:person, :diaspora_handle => "tom@tom.joindiaspora.comm")
+        FactoryGirl.create(:person, diaspora_handle: "tom@tom.joindiaspora.comm")
-        f = Person.by_account_identifier("tom@tom.joindiaspora.com")
+        expect(Person.by_account_identifier("tom@tom.joindiaspora.com")).to be_nil
-        expect(f).to be nil
       end
     end
   end
@@ -506,4 +504,14 @@ describe Person, :type => :model do
       @person.clear_profile!
     end
   end
+
+  context "validation" do
+    it "validates that no other person with same guid exists" do
+      person = FactoryGirl.build(:person)
+      person.guid = alice.guid
+
+      expect(person.valid?).to be_falsey
+      expect(person.errors.full_messages).to include("Person with same GUID already exists: #{alice.diaspora_handle}")
+    end
+  end
 end

spec/models/user/querying_spec.rb

@@ -186,29 +186,21 @@ describe User::Querying, :type => :model do
         expect(alice.people_in_aspects([@alices_aspect])).to eq([bob.person])
       end
 
-      it 'returns local/remote people objects for a users contact in each aspect' do
+      it "returns local/remote people objects for a users contact in each aspect" do
         local_user1 = FactoryGirl.create(:user)
         local_user2 = FactoryGirl.create(:user)
-        remote_user = FactoryGirl.create(:user)
+        remote_person = FactoryGirl.create(:person)
 
-        asp1 = local_user1.aspects.create(:name => "lol")
+        asp1 = local_user1.aspects.create(name: "lol")
-        asp2 = local_user2.aspects.create(:name => "brb")
+        asp2 = local_user2.aspects.create(name: "brb")
-        asp3 = remote_user.aspects.create(:name => "ttyl")
 
         connect_users(alice, @alices_aspect, local_user1, asp1)
         connect_users(alice, @alices_aspect, local_user2, asp2)
-        connect_users(alice, @alices_aspect, remote_user, asp3)
+        alice.contacts.create!(person: remote_person, aspects: [@alices_aspect], sharing: true)
-
-        local_person = remote_user.person
-        local_person.diaspora_handle = "#{remote_user.username}@example.net"
-        local_person.owner = nil
-        local_person.pod = Pod.find_or_create_by(url: "http://example.net")
-        local_person.save
-        local_person.reload
 
         expect(alice.people_in_aspects([@alices_aspect]).count).to eq(4)
-        expect(alice.people_in_aspects([@alices_aspect], :type => 'remote').count).to eq(1)
+        expect(alice.people_in_aspects([@alices_aspect], type: "remote").count).to eq(1)
-        expect(alice.people_in_aspects([@alices_aspect], :type => 'local').count).to eq(3)
+        expect(alice.people_in_aspects([@alices_aspect], type: "local").count).to eq(3)
       end
 
       it 'does not return people not connected to user on same pod' do