diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index df75693e840232739ab20809284035f69fc35fa1..f7e7f04384a4237fb2d46d8d7bd4f6a4d6f5b31f 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -80,9 +80,9 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController identity_linker ||= auth_module::IdentityLinker.new(current_user, oauth) - identity_linker.create_or_update + identity_linker.link - if identity_linker.created? + if identity_linker.changed? redirect_identity_linked elsif identity_linker.error_message.present? redirect_identity_link_failed(identity_linker.error_message) diff --git a/lib/gitlab/auth/o_auth/identity_linker.rb b/lib/gitlab/auth/o_auth/identity_linker.rb index 704a7e1b3c2ebcf8a5cdeeb94971888b023fbb3e..de92d7a214d613bf1f17bf32e9f7d76ef3da2967 100644 --- a/lib/gitlab/auth/o_auth/identity_linker.rb +++ b/lib/gitlab/auth/o_auth/identity_linker.rb @@ -2,25 +2,6 @@ module Gitlab module Auth module OAuth class IdentityLinker < OmniauthIdentityLinkerBase - def create_or_update - if identity.new_record? - @created = identity.save - end - end - - def error_message - identity.validate - - identity.errors.full_messages.join(', ') - end - - private - - def identity - @identity ||= current_user.identities - .with_extern_uid(oauth['provider'], oauth['uid']) - .first_or_initialize(extern_uid: oauth['uid']) - end end end end diff --git a/lib/gitlab/auth/omniauth_identity_linker_base.rb b/lib/gitlab/auth/omniauth_identity_linker_base.rb index 4ed28d6a8be89efc4caa1c9831d0a3db505464c5..ae365fcdfaa50b564a12d08438fd6ef6368e24ee 100644 --- a/lib/gitlab/auth/omniauth_identity_linker_base.rb +++ b/lib/gitlab/auth/omniauth_identity_linker_base.rb @@ -6,19 +6,41 @@ module Gitlab def initialize(current_user, oauth) @current_user = current_user @oauth = oauth - @created = false + @changed = false end - def created? - @created + def link + save if identity.new_record? + end + + def changed? + @changed end def error_message - '' + identity.validate + + identity.errors.full_messages.join(', ') + end + + private + + def save + @changed = identity.save + end + + def identity + @identity ||= current_user.identities + .with_extern_uid(provider, uid) + .first_or_initialize(extern_uid: uid) + end + + def provider + oauth['provider'] end - def create_or_update - raise NotImplementedError + def uid + oauth['uid'] end end end diff --git a/lib/gitlab/auth/saml/identity_linker.rb b/lib/gitlab/auth/saml/identity_linker.rb index d5f97f01df3cbcf5735ff10686e0470642081933..7e4b191d512a9f367194189b1a97531afc2bb9bf 100644 --- a/lib/gitlab/auth/saml/identity_linker.rb +++ b/lib/gitlab/auth/saml/identity_linker.rb @@ -2,25 +2,6 @@ module Gitlab module Auth module Saml class IdentityLinker < OmniauthIdentityLinkerBase - def create_or_update - if find_saml_identity.nil? - create_saml_identity - - @created = true - else - @created = false - end - end - - protected - - def find_saml_identity - current_user.identities.with_extern_uid(:saml, oauth['uid']).take - end - - def create_saml_identity - current_user.identities.create(extern_uid: oauth['uid'], provider: :saml) - end end end end diff --git a/spec/lib/gitlab/auth/o_auth/identity_linker_spec.rb b/spec/lib/gitlab/auth/o_auth/identity_linker_spec.rb index 8d1b0a3cd4b69b5015783c397fcd848dd6b07990..528f1b4ec5753f34c580eb649cbc47394beaa10b 100644 --- a/spec/lib/gitlab/auth/o_auth/identity_linker_spec.rb +++ b/spec/lib/gitlab/auth/o_auth/identity_linker_spec.rb @@ -12,23 +12,23 @@ describe Gitlab::Auth::OAuth::IdentityLinker do let!(:identity) { user.identities.create!(provider: provider, extern_uid: uid) } it "doesn't create new identity" do - expect { subject.create_or_update }.not_to change { Identity.count } + expect { subject.link }.not_to change { Identity.count } end - it "#created? returns false" do - subject.create_or_update + it "sets #changed? to false" do + subject.link - expect(subject).not_to be_created + expect(subject).not_to be_changed end end context 'identity already linked to different user' do let!(:identity) { create(:identity, provider: provider, extern_uid: uid) } - it "#created? returns false" do - subject.create_or_update + it "#changed? returns false" do + subject.link - expect(subject).not_to be_created + expect(subject).not_to be_changed end it 'exposes error message' do @@ -38,25 +38,25 @@ describe Gitlab::Auth::OAuth::IdentityLinker do context 'identity needs to be created' do it 'creates linked identity' do - expect { subject.create_or_update }.to change { user.identities.count } + expect { subject.link }.to change { user.identities.count } end it 'sets identity provider' do - subject.create_or_update + subject.link expect(user.identities.last.provider).to eq provider end it 'sets identity extern_uid' do - subject.create_or_update + subject.link expect(user.identities.last.extern_uid).to eq uid end - it 'sets #created? to true' do - subject.create_or_update + it 'sets #changed? to true' do + subject.link - expect(subject).to be_created + expect(subject).to be_changed end end end diff --git a/spec/lib/gitlab/auth/saml/identity_linker_spec.rb b/spec/lib/gitlab/auth/saml/identity_linker_spec.rb index 11e8469e9f45e7a32782f2f19a207fdcb1d38351..f3305d574ccfb9de4357cdbe2265fd225827b2c2 100644 --- a/spec/lib/gitlab/auth/saml/identity_linker_spec.rb +++ b/spec/lib/gitlab/auth/saml/identity_linker_spec.rb @@ -12,37 +12,37 @@ describe Gitlab::Auth::Saml::IdentityLinker do let!(:identity) { user.identities.create!(provider: provider, extern_uid: uid) } it "doesn't create new identity" do - expect { subject.create_or_update }.not_to change { Identity.count } + expect { subject.link }.not_to change { Identity.count } end - it 'sets #created? to false' do - subject.create_or_update + it "sets #changed? to false" do + subject.link - expect(subject).not_to be_created + expect(subject).not_to be_changed end end context 'identity needs to be created' do it 'creates linked identity' do - expect { subject.create_or_update }.to change { user.identities.count } + expect { subject.link }.to change { user.identities.count } end it 'sets identity provider' do - subject.create_or_update + subject.link expect(user.identities.last.provider).to eq provider end it 'sets identity extern_uid' do - subject.create_or_update + subject.link expect(user.identities.last.extern_uid).to eq uid end - it 'sets #created? to true' do - subject.create_or_update + it 'sets #changed? to true' do + subject.link - expect(subject).to be_created + expect(subject).to be_changed end end end