Commit 30ee4ea6 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'saml-ldap-link-flow' into 'master'

Adjust the SAML control flow to allow LDAP identities to be added to an existing SAML user.

## What does this MR do?

It correctly lets an existing SAML user to add their LDAP identity automatically at login.

## Why was this MR needed?

A customer had issues with the `auto_link_ldap_user` feature. The flow was not working if there was an account with a SAML identity, but no LDAP identity. GitLab would pick up the correct LDAP person, but due to the order of the flow, that LDAP person was never associated with the user.

## What are the relevant issue numbers?

Fixes #17346 

/cc @dblessing @balameb @stanhu 

See merge request !4498
parents 1e4db9ed 9282810f
...@@ -69,13 +69,20 @@ module Gitlab ...@@ -69,13 +69,20 @@ module Gitlab
return unless ldap_person return unless ldap_person
# If a corresponding person exists with same uid in a LDAP server, # If a corresponding person exists with same uid in a LDAP server,
# set up a Gitlab user with dual LDAP and Omniauth identities. # check if the user already has a GitLab account.
if user = Gitlab::LDAP::User.find_by_uid_and_provider(ldap_person.dn, ldap_person.provider) user = Gitlab::LDAP::User.find_by_uid_and_provider(ldap_person.dn, ldap_person.provider)
# Case when a LDAP user already exists in Gitlab. Add the Omniauth identity to existing account. if user
# Case when a LDAP user already exists in Gitlab. Add the OAuth identity to existing account.
log.info "LDAP account found for user #{user.username}. Building new #{auth_hash.provider} identity."
user.identities.build(extern_uid: auth_hash.uid, provider: auth_hash.provider) user.identities.build(extern_uid: auth_hash.uid, provider: auth_hash.provider)
else else
# No account in Gitlab yet: create it and add the LDAP identity log.info "No existing LDAP account was found in GitLab. Checking for #{auth_hash.provider} account."
user = find_by_uid_and_provider
if user.nil?
log.info "No user found using #{auth_hash.provider} provider. Creating a new one."
user = build_new_user user = build_new_user
end
log.info "Correct account has been found. Adding LDAP identity to user: #{user.username}."
user.identities.new(provider: ldap_person.provider, extern_uid: ldap_person.dn) user.identities.new(provider: ldap_person.provider, extern_uid: ldap_person.dn)
end end
......
...@@ -12,12 +12,12 @@ module Gitlab ...@@ -12,12 +12,12 @@ module Gitlab
end end
def gl_user def gl_user
@user ||= find_by_uid_and_provider
if auto_link_ldap_user? if auto_link_ldap_user?
@user ||= find_or_create_ldap_user @user ||= find_or_create_ldap_user
end end
@user ||= find_by_uid_and_provider
if auto_link_saml_user? if auto_link_saml_user?
@user ||= find_by_email @user ||= find_by_email
end end
......
...@@ -145,6 +145,7 @@ describe Gitlab::Saml::User, lib: true do ...@@ -145,6 +145,7 @@ describe Gitlab::Saml::User, lib: true do
allow(ldap_user).to receive(:email) { %w(john@mail.com john2@example.com) } allow(ldap_user).to receive(:email) { %w(john@mail.com john2@example.com) }
allow(ldap_user).to receive(:dn) { 'uid=user1,ou=People,dc=example' } allow(ldap_user).to receive(:dn) { 'uid=user1,ou=People,dc=example' }
allow(Gitlab::LDAP::Person).to receive(:find_by_uid).and_return(ldap_user) allow(Gitlab::LDAP::Person).to receive(:find_by_uid).and_return(ldap_user)
allow(Gitlab::LDAP::Person).to receive(:find_by_dn).and_return(ldap_user)
end end
context 'and no account for the LDAP user' do context 'and no account for the LDAP user' do
...@@ -177,6 +178,23 @@ describe Gitlab::Saml::User, lib: true do ...@@ -177,6 +178,23 @@ describe Gitlab::Saml::User, lib: true do
]) ])
end end
end end
context 'user has SAML user, and wants to add their LDAP identity' do
it 'adds the LDAP identity to the existing SAML user' do
create(:omniauth_user, email: 'john@mail.com', extern_uid: 'uid=user1,ou=People,dc=example', provider: 'saml', username: 'john')
local_hash = OmniAuth::AuthHash.new(uid: 'uid=user1,ou=People,dc=example', provider: provider, info: info_hash)
local_saml_user = described_class.new(local_hash)
local_saml_user.save
local_gl_user = local_saml_user.gl_user
expect(local_gl_user).to be_valid
expect(local_gl_user.identities.length).to eql 2
identities_as_hash = local_gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } }
expect(identities_as_hash).to match_array([ { provider: 'ldapmain', extern_uid: 'uid=user1,ou=People,dc=example' },
{ provider: 'saml', extern_uid: 'uid=user1,ou=People,dc=example' }
])
end
end
end end
end end
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