From 7038440e342a521807b1e5ffb6d47d4c0b13048d Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Mon, 6 Jun 2016 18:47:49 -0500 Subject: Adjust the SAML control flow to allow LDAP identities to be added to an existing SAML user. --- lib/gitlab/o_auth/user.rb | 2 +- lib/gitlab/saml/user.rb | 28 ++++++++++++++++++++++++++-- spec/lib/gitlab/saml/user_spec.rb | 19 +++++++++++++++++++ 3 files changed, 46 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb index 356e96fcbab..268ee115028 100644 --- a/lib/gitlab/o_auth/user.rb +++ b/lib/gitlab/o_auth/user.rb @@ -96,7 +96,7 @@ module Gitlab # Look for a corresponding person with same uid in any of the configured LDAP providers Gitlab::LDAP::Config.providers.each do |provider| adapter = Gitlab::LDAP::Adapter.new(provider) - @ldap_person = Gitlab::LDAP::Person.find_by_uid(auth_hash.uid, adapter) + @ldap_person = Gitlab::LDAP::Person.find_by_dn(auth_hash.uid, adapter) break if @ldap_person end @ldap_person diff --git a/lib/gitlab/saml/user.rb b/lib/gitlab/saml/user.rb index dba4bbfc899..6f7d4825ae5 100644 --- a/lib/gitlab/saml/user.rb +++ b/lib/gitlab/saml/user.rb @@ -12,12 +12,12 @@ module Gitlab end def gl_user - @user ||= find_by_uid_and_provider - if auto_link_ldap_user? @user ||= find_or_create_ldap_user end + @user ||= find_by_uid_and_provider + if auto_link_saml_user? @user ||= find_by_email end @@ -62,6 +62,30 @@ module Gitlab !Gitlab::Saml::Config.external_groups.nil? end + def find_or_create_ldap_user + return unless ldap_person + + # If a corresponding person exists with same uid in a LDAP server, + # check if the user already has a GitLab account + user = Gitlab::LDAP::User.find_by_uid_and_provider(ldap_person.dn, ldap_person.provider) + if user + # Case when a LDAP user already exists in Gitlab. Add the SAML identity to existing account. + user.identities.build(extern_uid: auth_hash.uid, provider: auth_hash.provider) + else + # No account found using LDAP in Gitlab yet: check if there is a SAML account with + # the passed uid and provider + user = find_by_uid_and_provider + if user.nil? + # No SAML account found, build a new user. + user = build_new_user + end + # Correct account is present, add the LDAP Identity to the user. + user.identities.new(provider: ldap_person.provider, extern_uid: ldap_person.dn) + end + + user + end + def auth_hash=(auth_hash) @auth_hash = Gitlab::Saml::AuthHash.new(auth_hash) end diff --git a/spec/lib/gitlab/saml/user_spec.rb b/spec/lib/gitlab/saml/user_spec.rb index c2a51d9249c..f0a17244ff6 100644 --- a/spec/lib/gitlab/saml/user_spec.rb +++ b/spec/lib/gitlab/saml/user_spec.rb @@ -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(: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_dn).and_return(ldap_user) end context 'and no account for the LDAP user' do @@ -177,6 +178,24 @@ describe Gitlab::Saml::User, lib: true do ]) 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, extra: { raw_info: OneLogin::RubySaml::Attributes.new({ 'groups' => %w(Developers Freelancers Designers) }) }) + 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 -- cgit v1.2.1 From c593154cb4f0215851fbbae1dde753dacbaa6713 Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Tue, 7 Jun 2016 11:19:19 -0500 Subject: Moved `find_or_create_ldap_user` method to parent class and added logging. --- lib/gitlab/o_auth/user.rb | 18 ++++++++++++------ lib/gitlab/saml/user.rb | 24 ------------------------ spec/lib/gitlab/saml/user_spec.rb | 2 +- 3 files changed, 13 insertions(+), 31 deletions(-) diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb index 268ee115028..5e52979093d 100644 --- a/lib/gitlab/o_auth/user.rb +++ b/lib/gitlab/o_auth/user.rb @@ -69,13 +69,19 @@ module Gitlab return unless ldap_person # If a corresponding person exists with same uid in a LDAP server, - # set up a Gitlab user with dual LDAP and Omniauth identities. - if 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. + # 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)) + # 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 identity." user.identities.build(extern_uid: auth_hash.uid, provider: auth_hash.provider) else - # No account in Gitlab yet: create it and add the LDAP identity - user = build_new_user + log.info 'No existing LDAP account was found in GitLab. Checking for OAuth account.' + user = find_by_uid_and_provider + if user.nil? + log.info 'No user found with the specified OAuth provider. Creating a new one.' + 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) end @@ -96,7 +102,7 @@ module Gitlab # Look for a corresponding person with same uid in any of the configured LDAP providers Gitlab::LDAP::Config.providers.each do |provider| adapter = Gitlab::LDAP::Adapter.new(provider) - @ldap_person = Gitlab::LDAP::Person.find_by_dn(auth_hash.uid, adapter) + @ldap_person = Gitlab::LDAP::Person.find_by_uid(auth_hash.uid, adapter) break if @ldap_person end @ldap_person diff --git a/lib/gitlab/saml/user.rb b/lib/gitlab/saml/user.rb index 6f7d4825ae5..8943022612c 100644 --- a/lib/gitlab/saml/user.rb +++ b/lib/gitlab/saml/user.rb @@ -62,30 +62,6 @@ module Gitlab !Gitlab::Saml::Config.external_groups.nil? end - def find_or_create_ldap_user - return unless ldap_person - - # If a corresponding person exists with same uid in a LDAP server, - # check if the user already has a GitLab account - user = Gitlab::LDAP::User.find_by_uid_and_provider(ldap_person.dn, ldap_person.provider) - if user - # Case when a LDAP user already exists in Gitlab. Add the SAML identity to existing account. - user.identities.build(extern_uid: auth_hash.uid, provider: auth_hash.provider) - else - # No account found using LDAP in Gitlab yet: check if there is a SAML account with - # the passed uid and provider - user = find_by_uid_and_provider - if user.nil? - # No SAML account found, build a new user. - user = build_new_user - end - # Correct account is present, add the LDAP Identity to the user. - user.identities.new(provider: ldap_person.provider, extern_uid: ldap_person.dn) - end - - user - end - def auth_hash=(auth_hash) @auth_hash = Gitlab::Saml::AuthHash.new(auth_hash) end diff --git a/spec/lib/gitlab/saml/user_spec.rb b/spec/lib/gitlab/saml/user_spec.rb index f0a17244ff6..5957998e0f3 100644 --- a/spec/lib/gitlab/saml/user_spec.rb +++ b/spec/lib/gitlab/saml/user_spec.rb @@ -182,7 +182,7 @@ describe Gitlab::Saml::User, lib: true do 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, extra: { raw_info: OneLogin::RubySaml::Attributes.new({ 'groups' => %w(Developers Freelancers Designers) }) }) + 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 -- cgit v1.2.1 From 9282810fb7b6102657a0ddb2a02f71b6da22067f Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Wed, 8 Jun 2016 18:09:43 -0500 Subject: Syntax fixes and better logging around the `ldap_person` method. --- lib/gitlab/o_auth/user.rb | 9 +++++---- spec/lib/gitlab/saml/user_spec.rb | 3 +-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb index 5e52979093d..78f3ecb4cb4 100644 --- a/lib/gitlab/o_auth/user.rb +++ b/lib/gitlab/o_auth/user.rb @@ -70,15 +70,16 @@ module Gitlab # If a corresponding person exists with same uid in a LDAP server, # 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) + 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 identity." + 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) else - log.info 'No existing LDAP account was found in GitLab. Checking for OAuth account.' + 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 with the specified OAuth provider. Creating a new one.' + log.info "No user found using #{auth_hash.provider} provider. Creating a new one." user = build_new_user end log.info "Correct account has been found. Adding LDAP identity to user: #{user.username}." diff --git a/spec/lib/gitlab/saml/user_spec.rb b/spec/lib/gitlab/saml/user_spec.rb index 5957998e0f3..84c21ceefd9 100644 --- a/spec/lib/gitlab/saml/user_spec.rb +++ b/spec/lib/gitlab/saml/user_spec.rb @@ -184,9 +184,9 @@ describe Gitlab::Saml::User, lib: true 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 } } @@ -194,7 +194,6 @@ describe Gitlab::Saml::User, lib: true do { provider: 'saml', extern_uid: 'uid=user1,ou=People,dc=example' } ]) end - end end end -- cgit v1.2.1