diff --git a/app/models/person.rb b/app/models/person.rb index 7d0cf8ce9..5b400cd30 100644 --- a/app/models/person.rb +++ b/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 diff --git a/spec/federation_callbacks_spec.rb b/spec/federation_callbacks_spec.rb index b99675c48..4c6a631d0 100644 --- a/spec/federation_callbacks_spec.rb +++ b/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 diff --git a/spec/helpers/people_helper_spec.rb b/spec/helpers/people_helper_spec.rb index 24f0bf445..70c02a199 100644 --- a/spec/helpers/people_helper_spec.rb +++ b/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) - person = @user.person - person.diaspora_handle = "#{@user.username}@#{AppConfig.pod_uri.authority}" - person.save! + @user.person.diaspora_handle = "#{@user.username}@#{AppConfig.pod_uri.authority}" + expect(@user.save(validate: false)).to eq(true) expect(local_or_remote_person_path(@user.person)).to eq(person_path(@user.person)) end diff --git a/spec/models/person_spec.rb b/spec/models/person_spec.rb index 6dfe1530f..c376da2b1 100644 --- a/spec/models/person_spec.rb +++ b/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 - user = FactoryGirl.create(:user, :username => "SaMaNtHa") - person = FactoryGirl.create(:person, :diaspora_handle => "tomtom@tom.joindiaspora.com") - user.person.diaspora_handle = "tom@tom.joindiaspora.com" - user.person.save - expect(Person.by_account_identifier("tom@tom.joindiaspora.com").diaspora_handle).to eq("tom@tom.joindiaspora.com") + it "should only find people who are exact matches (1/2)" do + FactoryGirl.create(:person, diaspora_handle: "tomtom@tom.joindiaspora.com") + FactoryGirl.create(:person, diaspora_handle: "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 - person = FactoryGirl.create(:person, :diaspora_handle => "tomtom@tom.joindiaspora.com") - person1 = FactoryGirl.create(:person, :diaspora_handle => "tom@tom.joindiaspora.comm") - f = Person.by_account_identifier("tom@tom.joindiaspora.com") - expect(f).to be nil + it "should only find people who are exact matches (2/2)" do + FactoryGirl.create(:person, diaspora_handle: "tomtom@tom.joindiaspora.com") + FactoryGirl.create(:person, diaspora_handle: "tom@tom.joindiaspora.comm") + expect(Person.by_account_identifier("tom@tom.joindiaspora.com")).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 diff --git a/spec/models/user/querying_spec.rb b/spec/models/user/querying_spec.rb index fd07454cd..eeb447aa3 100644 --- a/spec/models/user/querying_spec.rb +++ b/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") - asp2 = local_user2.aspects.create(:name => "brb") - asp3 = remote_user.aspects.create(:name => "ttyl") + asp1 = local_user1.aspects.create(name: "lol") + asp2 = local_user2.aspects.create(name: "brb") connect_users(alice, @alices_aspect, local_user1, asp1) connect_users(alice, @alices_aspect, local_user2, asp2) - connect_users(alice, @alices_aspect, remote_user, asp3) - - 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 + alice.contacts.create!(person: remote_person, aspects: [@alices_aspect], sharing: true) 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 => 'local').count).to eq(3) + 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) end it 'does not return people not connected to user on same pod' do