Commit c388f3db authored by Douwe Maan's avatar Douwe Maan

Merge branch 'joelkoglin/gitlab-ce-feature_fix_ldap_auth_issue_993'

parents a429eb4d 12d8b01e
No related merge requests found
...@@ -24,6 +24,7 @@ v 8.0.0 (unreleased) ...@@ -24,6 +24,7 @@ v 8.0.0 (unreleased)
- Improve search page usability - Improve search page usability
- Bring more UI consistency in way how projects, snippets and groups lists are rendered - Bring more UI consistency in way how projects, snippets and groups lists are rendered
- Make all profiles public - Make all profiles public
- Fixed login failure when extern_uid changes (Joel Koglin)
v 7.14.1 v 7.14.1
- Improve abuse reports management from admin area - Improve abuse reports management from admin area
......
...@@ -104,7 +104,7 @@ class User < ActiveRecord::Base ...@@ -104,7 +104,7 @@ class User < ActiveRecord::Base
# Profile # Profile
has_many :keys, dependent: :destroy has_many :keys, dependent: :destroy
has_many :emails, dependent: :destroy has_many :emails, dependent: :destroy
has_many :identities, dependent: :destroy has_many :identities, dependent: :destroy, autosave: true
# Groups # Groups
has_many :members, dependent: :destroy has_many :members, dependent: :destroy
......
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
...@@ -44,9 +44,14 @@ module Gitlab ...@@ -44,9 +44,14 @@ module Gitlab
gl_user.skip_reconfirmation! gl_user.skip_reconfirmation!
gl_user.email = auth_hash.email gl_user.email = auth_hash.email
# Build new identity only if we dont have have same one # find_or_initialize_by doesn't update `gl_user.identities`, and isn't autosaved.
gl_user.identities.find_or_initialize_by(provider: auth_hash.provider, identity = gl_user.identities.find { |identity| identity.provider == auth_hash.provider }
extern_uid: auth_hash.uid) 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 gl_user
end end
......
...@@ -47,6 +47,28 @@ describe Gitlab::LDAP::User do ...@@ -47,6 +47,28 @@ describe Gitlab::LDAP::User do
expect(existing_user.ldap_identity.provider).to eql 'ldapmain' expect(existing_user.ldap_identity.provider).to eql 'ldapmain'
end 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 it "creates a new user if not found" do
expect{ ldap_user.save }.to change{ User.count }.by(1) expect{ ldap_user.save }.to change{ User.count }.by(1)
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment