From 4d2f36118a3b21b6bb4ee702b98d032967ba463a Mon Sep 17 00:00:00 2001 From: Joel Koglin Date: Tue, 21 Jul 2015 14:03:26 -0700 Subject: Issue #993: Fixed login failure when extern_uid changes --- CHANGELOG | 1 + app/models/user.rb | 2 +- .../20150817163600_deduplicate_user_identities.rb | 14 ++++++++++++++ db/schema.rb | 2 +- lib/gitlab/ldap/user.rb | 10 +++++++--- spec/lib/gitlab/ldap/user_spec.rb | 22 ++++++++++++++++++++++ 6 files changed, 46 insertions(+), 5 deletions(-) create mode 100644 db/migrate/20150817163600_deduplicate_user_identities.rb diff --git a/CHANGELOG b/CHANGELOG index 6cbe51f1fb0..53c7463ae76 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -75,6 +75,7 @@ v 7.14.0 (unreleased) - Update Flowdock integration to support new Flowdock API (Boyan Tabakov) - Remove author from files view (Sven Strickroth) - Fix infinite loop when SAML was incorrectly configured. + - Fixed login failure when extern_uid changes (Joel Koglin) v 7.13.5 - Satellites reverted diff --git a/app/models/user.rb b/app/models/user.rb index f70761074c5..1f0735a7e7b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -104,7 +104,7 @@ class User < ActiveRecord::Base # Profile has_many :keys, dependent: :destroy has_many :emails, dependent: :destroy - has_many :identities, dependent: :destroy + has_many :identities, dependent: :destroy, autosave: true # Groups has_many :members, dependent: :destroy diff --git a/db/migrate/20150817163600_deduplicate_user_identities.rb b/db/migrate/20150817163600_deduplicate_user_identities.rb new file mode 100644 index 00000000000..fab669c2905 --- /dev/null +++ b/db/migrate/20150817163600_deduplicate_user_identities.rb @@ -0,0 +1,14 @@ +class DeduplicateUserIdentities < ActiveRecord::Migration + def change + execute 'DROP TABLE IF EXISTS tt_migration_DeduplicateUserIdentities;' + execute 'CREATE TEMPORARY TABLE tt_migration_DeduplicateUserIdentities AS SELECT id,provider,user_id FROM identities;' + execute 'DELETE FROM identities WHERE id NOT IN ( SELECT MIN(id) FROM tt_migration_DeduplicateUserIdentities GROUP BY user_id, provider);' + execute 'DROP TABLE IF EXISTS tt_migration_DeduplicateUserIdentities;' + end + + def down + # This is an irreversible migration; + # If someone is trying to rollback for other reasons, we should not throw an Exception. + # raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/schema.rb b/db/schema.rb index fdf09b3afdb..7a0c92bddcd 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20150812080800) do +ActiveRecord::Schema.define(version: 20150817163600) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb index f7f3ba9ad7d..2ffb1241966 100644 --- a/lib/gitlab/ldap/user.rb +++ b/lib/gitlab/ldap/user.rb @@ -44,9 +44,13 @@ module Gitlab gl_user.skip_reconfirmation! gl_user.email = auth_hash.email - # Build new identity only if we dont have have same one - gl_user.identities.find_or_initialize_by(provider: auth_hash.provider, - extern_uid: auth_hash.uid) + # If we don't have an identity for this provider yet, create one + if gl_user.identities.find_by(provider: auth_hash.provider).nil? + gl_user.identities.new(extern_uid: auth_hash.uid, provider: auth_hash.provider) + else # Update the UID attribute for this provider in case it has changed + identity = gl_user.identities.select { |identity| identity.provider == auth_hash.provider } + identity.first.extern_uid = auth_hash.uid + end gl_user end diff --git a/spec/lib/gitlab/ldap/user_spec.rb b/spec/lib/gitlab/ldap/user_spec.rb index 7cfca96f4e0..84d9fb54b61 100644 --- a/spec/lib/gitlab/ldap/user_spec.rb +++ b/spec/lib/gitlab/ldap/user_spec.rb @@ -47,6 +47,28 @@ describe Gitlab::LDAP::User do expect(existing_user.ldap_identity.provider).to eql 'ldapmain' end + it 'connects to existing ldap user if the extern_uid changes' do + existing_user = create(:omniauth_user, email: 'john@example.com', extern_uid: 'old-uid', provider: 'ldapmain') + expect{ ldap_user.save }.not_to change{ User.count } + + existing_user.reload + expect(existing_user.ldap_identity.extern_uid).to eql 'my-uid' + expect(existing_user.ldap_identity.provider).to eql 'ldapmain' + expect(existing_user.id).to eql ldap_user.gl_user.id + end + + it 'maintains an identity per provider' do + existing_user = create(:omniauth_user, email: 'john@example.com', provider: 'twitter') + expect(existing_user.identities.count).to eql(1) + + ldap_user.save + expect(ldap_user.gl_user.identities.count).to eql(2) + + # Expect that find_by provider only returns a single instance of an identity and not an Enumerable + expect(ldap_user.gl_user.identities.find_by(provider: 'twitter')).to be_instance_of Identity + expect(ldap_user.gl_user.identities.find_by(provider: auth_hash.provider)).to be_instance_of Identity + end + it "creates a new user if not found" do expect{ ldap_user.save }.to change{ User.count }.by(1) end -- cgit v1.2.1 From d92f428024b2878682bb23b6b03bc671636b5afe Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 7 Aug 2015 16:09:48 +0200 Subject: Minor refactor --- lib/gitlab/ldap/user.rb | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb index 2ffb1241966..04a22237478 100644 --- a/lib/gitlab/ldap/user.rb +++ b/lib/gitlab/ldap/user.rb @@ -44,13 +44,14 @@ module Gitlab gl_user.skip_reconfirmation! gl_user.email = auth_hash.email - # If we don't have an identity for this provider yet, create one - if gl_user.identities.find_by(provider: auth_hash.provider).nil? - gl_user.identities.new(extern_uid: auth_hash.uid, provider: auth_hash.provider) - else # Update the UID attribute for this provider in case it has changed - identity = gl_user.identities.select { |identity| identity.provider == auth_hash.provider } - identity.first.extern_uid = auth_hash.uid - end + # find_or_initialize_by doesn't update `gl_user.identities`, and isn't autosaved. + identity = gl_user.identities.find { |identity| identity.provider == auth_hash.provider } + identity ||= gl_user.identities.build(provider: auth_hash.provider) + + # For a new user set extern_uid to the LDAP DN + # For an existing user with matching email but changed DN, update the DN. + # For an existing user with no change in DN, this line changes nothing. + identity.extern_uid = auth_hash.uid gl_user end -- cgit v1.2.1 From 12d8b01eb2df532815306db602d116e3177d750e Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sat, 29 Aug 2015 11:49:59 -0700 Subject: Move changelog item --- CHANGELOG | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index 4946c683904..bbf44ca14c9 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -24,6 +24,7 @@ v 8.0.0 (unreleased) - Improve search page usability - Bring more UI consistency in way how projects, snippets and groups lists are rendered - Make all profiles public + - Fixed login failure when extern_uid changes (Joel Koglin) v 7.14.1 - Improve abuse reports management from admin area @@ -98,7 +99,6 @@ v 7.14.0 - Update Flowdock integration to support new Flowdock API (Boyan Tabakov) - Remove author from files view (Sven Strickroth) - Fix infinite loop when SAML was incorrectly configured. - - Fixed login failure when extern_uid changes (Joel Koglin) v 7.13.5 - Satellites reverted -- cgit v1.2.1