Refactors SAML identity creation in gl_user.33493-attempt-to-link-saml-users-to-ldap-by-email

8 files changed, 122 insertions, 160 deletions

diff --git a/app/models/user.rb b/app/models/user.rb
index 09c9b3250eb..ed09b6ea67d 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -690,7 +690,11 @@ class User < ActiveRecord::Base
   end
 
   def ldap_user?
-    identities.exists?(["provider LIKE ? AND extern_uid IS NOT NULL", "ldap%"])
+    if identities.loaded?
+      identities.find { |identity| identity.provider.start_with?('ldap') }
+    else
+      identities.exists?(["provider LIKE ? AND extern_uid IS NOT NULL", "ldap%"])
+    end
   end
 
   def ldap_identity
diff --git a/lib/gitlab/ldap/adapter.rb b/lib/gitlab/ldap/adapter.rb
index 8bbd4af58e0..0afaa2306b5 100644
--- a/lib/gitlab/ldap/adapter.rb
+++ b/lib/gitlab/ldap/adapter.rb
@@ -22,8 +22,8 @@ module Gitlab
         Gitlab::LDAP::Config.new(provider)
       end
 
-      def users(field, value, limit = nil)
-        options = user_options(field, value, limit)
+      def users(fields, value, limit = nil)
+        options = user_options(Array(fields), value, limit)
 
         entries = ldap_search(options).select do |entry|
           entry.respond_to? config.uid
@@ -72,8 +72,7 @@ module Gitlab
 
       private
 
-      def user_options(field, value, limit)
-        filter = nil
+      def user_options(fields, value, limit)
         options = {
           attributes: Gitlab::LDAP::Person.ldap_attributes(config).compact.uniq,
           base: config.base
@@ -81,16 +80,13 @@ module Gitlab
 
         options[:size] = limit if limit
 
-        case field.to_sym
-        when :dn
+        if fields.include?('dn')
+          raise ArgumentError, 'It is not currently possible to search the DN and other fields at the same time.' if fields.size > 1
+
           options[:base] = value
           options[:scope] = Net::LDAP::SearchScope_BaseObject
-        when :email
-          filter = config.attributes['email'].map do |field|
-            Net::LDAP::Filter.eq(field, value)
-          end.inject(:|)
         else
-          filter = Net::LDAP::Filter.eq(field, value)
+          filter = fields.map { |field| Net::LDAP::Filter.eq(field, value) }.inject(:|)
         end
 
         options.merge(filter: user_filter(filter))
diff --git a/lib/gitlab/ldap/person.rb b/lib/gitlab/ldap/person.rb
index 7631d9e8b17..9a6f7827b16 100644
--- a/lib/gitlab/ldap/person.rb
+++ b/lib/gitlab/ldap/person.rb
@@ -18,7 +18,9 @@ module Gitlab
       end
 
       def self.find_by_email(email, adapter)
-        adapter.user('email', email)
+        email_fields = adapter.config.attributes['email']
+
+        adapter.user(email_fields, email)
       end
 
       def self.disabled_via_active_directory?(dn, adapter)
diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb
index 3bf27b37ae6..1793097363e 100644
--- a/lib/gitlab/ldap/user.rb
+++ b/lib/gitlab/ldap/user.rb
@@ -17,41 +17,19 @@ module Gitlab
         end
       end
 
-      def initialize(auth_hash)
-        super
-        update_user_attributes
-      end
-
       def save
         super('LDAP')
       end
 
       # instance methods
-      def gl_user
-        @gl_user ||= find_by_uid_and_provider || find_by_email || build_new_user
+      def find_user
+        find_by_uid_and_provider || find_by_email || build_new_user
       end
 
       def find_by_uid_and_provider
         self.class.find_by_uid_and_provider(auth_hash.uid, auth_hash.provider)
       end
 
-      def find_by_email
-        ::User.find_by(email: auth_hash.email.downcase) if auth_hash.has_attribute?(:email)
-      end
-
-      def update_user_attributes
-        if persisted?
-          # 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 identity set extern_uid to the LDAP DN
-          # For an existing identity with matching email but changed DN, update the DN.
-          # For an existing identity with no change in DN, this line changes nothing.
-          identity.extern_uid = auth_hash.uid
-        end
-      end
-
       def changed?
         gl_user.changed? || gl_user.identities.any?(&:changed?)
       end
diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb
index dd318b4242c..a720507245f 100644
--- a/lib/gitlab/o_auth/user.rb
+++ b/lib/gitlab/o_auth/user.rb
@@ -13,6 +13,7 @@ module Gitlab
       def initialize(auth_hash)
         self.auth_hash = auth_hash
         update_profile if sync_profile_from_provider?
+        add_or_update_user_identities
       end
 
       def persisted?
@@ -44,47 +45,55 @@ module Gitlab
       end
 
       def gl_user
-        @user ||= find_by_uid_and_provider
+        return @gl_user if defined?(@gl_user)
 
-        if auto_link_ldap_user?
-          @user ||= find_or_create_ldap_user
-        end
+        @gl_user = find_user
+      end
 
-        if signup_enabled?
-          @user ||= build_new_user
-        end
+      def find_user
+        user = find_by_uid_and_provider
 
-        if external_provider? && @user
-          @user.external = true
-        end
+        user ||= find_or_build_ldap_user if auto_link_ldap_user?
+        user ||= build_new_user if signup_enabled?
+
+        user.external = true if external_provider? && user
 
-        @user
+        user
       end
 
       protected
 
-      def find_or_create_ldap_user
+      def add_or_update_user_identities
+        identity = gl_user.identities.find { |identity| identity.provider == auth_hash.provider }
+
+        identity ||= gl_user.identities.build(provider: auth_hash.provider)
+        identity.extern_uid = auth_hash.uid
+
+        link_user_to_ldap if auto_link_ldap_user? && !gl_user.ldap_user? && ldap_person
+      end
+
+      def link_user_to_ldap
+        log.info "Correct account has been found. identity to user: #{gl_user.username}."
+        gl_user.identities.build(provider: ldap_person.provider, extern_uid: ldap_person.dn)
+      end
+
+      def find_or_build_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 OAuth identity to existing account.
           log.info "LDAP account found for user #{user.username}. Building new #{auth_hash.provider} identity."
-          user.identities.find_or_initialize_by(extern_uid: auth_hash.uid, provider: auth_hash.provider)
-        else
-          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
-          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)
+          return user
         end
 
-        user
+        log.info "No user found using #{auth_hash.provider} provider. Creating a new one."
+        build_new_user
+      end
+
+      def find_by_email
+        return unless auth_hash.has_attribute?(:email)
+
+        ::User.find_by(email: auth_hash.email.downcase)
       end
 
       def auto_link_ldap_user?
@@ -108,12 +117,9 @@ module Gitlab
       end
 
       def find_ldap_person(auth_hash, adapter)
-        person = Gitlab::LDAP::Person.find_by_uid(auth_hash.uid, adapter)
-        # The `uid` might actually be a DN. Try it next.
-        person ||= Gitlab::LDAP::Person.find_by_dn(auth_hash.uid, adapter)
-
-        # The `uid` might actually be a Email. Try it next.
-        person || Gitlab::LDAP::Person.find_by_email(auth_hash.uid, adapter)
+        Gitlab::LDAP::Person.find_by_uid(auth_hash.uid, adapter) ||
+          Gitlab::LDAP::Person.find_by_email(auth_hash.uid, adapter) ||
+          Gitlab::LDAP::Person.find_by_dn(auth_hash.uid, adapter)
       end
 
       def ldap_config
diff --git a/lib/gitlab/saml/user.rb b/lib/gitlab/saml/user.rb
index 2af54f8bb25..e0a9d1dee77 100644
--- a/lib/gitlab/saml/user.rb
+++ b/lib/gitlab/saml/user.rb
@@ -10,45 +10,20 @@ module Gitlab
         super('SAML')
       end
 
-      def gl_user
-        if auto_link_saml_user?
-          @user ||= find_by_email
-        end
-
-        if auto_link_ldap_user? && !@user&.ldap_user?
-          @user ||= find_or_create_ldap_user
-        end
+      def find_user
+        user = find_by_uid_and_provider
 
-        @user ||= find_by_uid_and_provider
-
-        if signup_enabled?
-          @user ||= build_new_user
-        end
+        user ||= find_by_email if auto_link_saml_user?
+        user ||= find_or_build_ldap_user if auto_link_ldap_user?
+        user ||= build_new_user if signup_enabled?
 
-        if external_users_enabled? && @user
+        if external_users_enabled? && user
           # Check if there is overlap between the user's groups and the external groups
           # setting then set user as external or internal.
-          @user.external =
-            if (auth_hash.groups & Gitlab::Saml::Config.external_groups).empty?
-              false
-            else
-              true
-            end
+          user.external = !(auth_hash.groups & Gitlab::Saml::Config.external_groups).empty?
         end
 
-        @user
-      end
-
-      def find_by_email
-        if auth_hash.has_attribute?(:email)
-          user = ::User.find_by(email: auth_hash.email.downcase)
-
-          if user&.identities&.empty?
-            user.identities.new(extern_uid: auth_hash.uid, provider: auth_hash.provider)
-          end
-
-          user
-        end
+        user
       end
 
       def changed?
diff --git a/spec/lib/gitlab/o_auth/user_spec.rb b/spec/lib/gitlab/o_auth/user_spec.rb
index 8aaf320cbf5..db26e16e3b2 100644
--- a/spec/lib/gitlab/o_auth/user_spec.rb
+++ b/spec/lib/gitlab/o_auth/user_spec.rb
@@ -4,6 +4,7 @@ describe Gitlab::OAuth::User do
   let(:oauth_user) { described_class.new(auth_hash) }
   let(:gl_user) { oauth_user.gl_user }
   let(:uid) { 'my-uid' }
+  let(:dn) { 'uid=user1,ou=People,dc=example' }
   let(:provider) { 'my-provider' }
   let(:auth_hash) { OmniAuth::AuthHash.new(uid: uid, provider: provider, info: info_hash) }
   let(:info_hash) do
@@ -197,7 +198,7 @@ describe Gitlab::OAuth::User do
               allow(ldap_user).to receive(:uid) { uid }
               allow(ldap_user).to receive(:username) { uid }
               allow(ldap_user).to receive(:email) { ['johndoe@example.com', 'john2@example.com'] }
-              allow(ldap_user).to receive(:dn) { 'uid=user1,ou=People,dc=example' }
+              allow(ldap_user).to receive(:dn) { dn }
             end
 
             context "and no account for the LDAP user" do
@@ -213,7 +214,7 @@ describe Gitlab::OAuth::User do
                 identities_as_hash = 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: 'ldapmain', extern_uid: dn },
                     { provider: 'twitter', extern_uid: uid }
                   ]
                 )
@@ -221,7 +222,7 @@ describe Gitlab::OAuth::User do
             end
 
             context "and LDAP user has an account already" do
-              let!(:existing_user) { create(:omniauth_user, email: 'john@example.com', extern_uid: 'uid=user1,ou=People,dc=example', provider: 'ldapmain', username: 'john') }
+              let!(:existing_user) { create(:omniauth_user, email: 'john@example.com', extern_uid: dn, provider: 'ldapmain', username: 'john') }
               it "adds the omniauth identity to the LDAP account" do
                 allow(Gitlab::LDAP::Person).to receive(:find_by_uid).and_return(ldap_user)
 
@@ -234,7 +235,7 @@ describe Gitlab::OAuth::User do
                 identities_as_hash = 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: 'ldapmain', extern_uid: dn },
                     { provider: 'twitter', extern_uid: uid }
                   ]
                 )
@@ -252,7 +253,7 @@ describe Gitlab::OAuth::User do
                 expect(identities_as_hash)
                   .to match_array(
                     [
-                      { provider: 'ldapmain', extern_uid: 'uid=user1,ou=People,dc=example' },
+                      { provider: 'ldapmain', extern_uid: dn },
                       { provider: 'twitter', extern_uid: uid }
                     ]
                   )
@@ -310,8 +311,8 @@ describe Gitlab::OAuth::User do
           allow(ldap_user).to receive(:uid) { uid }
           allow(ldap_user).to receive(:username) { uid }
           allow(ldap_user).to receive(:email) { ['johndoe@example.com', 'john2@example.com'] }
-          allow(ldap_user).to receive(:dn) { 'uid=user1,ou=People,dc=example' }
-          allow(oauth_user).to receive(:ldap_person).and_return(ldap_user)
+          allow(ldap_user).to receive(:dn) { dn }
+          allow(Gitlab::LDAP::Person).to receive(:find_by_uid).and_return(ldap_user)
         end
 
         context "and no account for the LDAP user" do
@@ -341,7 +342,7 @@ describe Gitlab::OAuth::User do
         end
 
         context 'and LDAP user has an account already' do
-          let!(:existing_user) { create(:omniauth_user, email: 'john@example.com', extern_uid: 'uid=user1,ou=People,dc=example', provider: 'ldapmain', username: 'john') }
+          let!(:existing_user) { create(:omniauth_user, email: 'john@example.com', extern_uid: dn, provider: 'ldapmain', username: 'john') }
 
           context 'dont block on create (LDAP)' do
             before do
diff --git a/spec/lib/gitlab/saml/user_spec.rb b/spec/lib/gitlab/saml/user_spec.rb
index c6cade99daa..59923bfb14d 100644
--- a/spec/lib/gitlab/saml/user_spec.rb
+++ b/spec/lib/gitlab/saml/user_spec.rb
@@ -1,9 +1,12 @@
 require 'spec_helper'
 
 describe Gitlab::Saml::User do
+  include LdapHelpers
+
   let(:saml_user) { described_class.new(auth_hash) }
   let(:gl_user) { saml_user.gl_user }
   let(:uid) { 'my-uid' }
+  let(:dn) { 'uid=user1,ou=People,dc=example' }
   let(:provider) { 'saml' }
   let(:auth_hash) { OmniAuth::AuthHash.new(uid: uid, provider: provider, info: info_hash, extra: { raw_info: OneLogin::RubySaml::Attributes.new({ 'groups' => %w(Developers Freelancers Designers) }) }) }
   let(:info_hash) do
@@ -163,14 +166,17 @@ describe Gitlab::Saml::User do
           end
 
           context 'and a corresponding LDAP person' do
+            let(:adapter) { ldap_adapter('ldapmain') }
+
             before do
               allow(ldap_user).to receive(:uid) { uid }
               allow(ldap_user).to receive(:username) { uid }
               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)
-              allow(Gitlab::LDAP::Person).to receive(:find_by_email).and_return(ldap_user)
+              allow(ldap_user).to receive(:dn) { dn }
+              allow(Gitlab::LDAP::Adapter).to receive(:new).and_return(adapter)
+              allow(Gitlab::LDAP::Person).to receive(:find_by_uid).with(uid, adapter).and_return(ldap_user)
+              allow(Gitlab::LDAP::Person).to receive(:find_by_dn).with(dn, adapter).and_return(ldap_user)
+              allow(Gitlab::LDAP::Person).to receive(:find_by_email).with('john@mail.com', adapter).and_return(ldap_user)
             end
 
             context 'and no account for the LDAP user' do
@@ -182,28 +188,51 @@ describe Gitlab::Saml::User do
                 expect(gl_user.email).to eql 'john@mail.com'
                 expect(gl_user.identities.length).to be 2
                 identities_as_hash = 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' },
+                expect(identities_as_hash).to match_array([{ provider: 'ldapmain', extern_uid: dn },
                                                            { provider: 'saml', extern_uid: uid }])
               end
             end
 
             context 'and LDAP user has an account already' do
+              let(:auth_hash_base_attributes) do
+                {
+                  uid: uid,
+                  provider: provider,
+                  info: info_hash,
+                  extra: {
+                    raw_info: OneLogin::RubySaml::Attributes.new(
+                      { 'groups' => %w(Developers Freelancers Designers) }
+                    )
+                  }
+                }
+              end
+              let(:auth_hash) { OmniAuth::AuthHash.new(auth_hash_base_attributes) }
+              let(:uid_types) { %w(uid dn email) }
+
               before do
                 create(:omniauth_user,
                        email: 'john@mail.com',
-                       extern_uid: 'uid=user1,ou=People,dc=example',
+                       extern_uid: dn,
                        provider: 'ldapmain',
                        username: 'john')
               end
 
-              shared_examples 'find ldap person' do |uid_type, uid|
+              shared_examples 'find LDAP person' do |uid_type, uid|
+                let(:auth_hash) { OmniAuth::AuthHash.new(auth_hash_base_attributes.merge(uid: extern_uid)) }
+
                 before do
+                  nil_types = uid_types - [uid_type]
+
+                  nil_types.each do |type|
+                    allow(Gitlab::LDAP::Person).to receive(:"find_by_#{type}").and_return(nil)
+                  end
+
                   allow(Gitlab::LDAP::Person).to receive(:"find_by_#{uid_type}").and_return(ldap_user)
                 end
 
                 it 'adds the omniauth identity to the LDAP account' do
                   identities = [
-                    { provider: 'ldapmain', extern_uid: 'uid=user1,ou=People,dc=example' },
+                    { provider: 'ldapmain', extern_uid: dn },
                     { provider: 'saml', extern_uid: extern_uid }
                   ]
 
@@ -222,53 +251,20 @@ describe Gitlab::Saml::User do
               end
 
               context 'when uid is an uid' do
-                it_behaves_like 'find ldap person', 'uid' do
+                it_behaves_like 'find LDAP person', 'uid' do
                   let(:extern_uid) { uid }
-                  let(:auth_hash) do
-                    OmniAuth::AuthHash.new(
-                      uid: uid,
-                      provider: provider,
-                      info: info_hash,
-                      extra: {
-                        raw_info: OneLogin::RubySaml::Attributes.new(
-                          { 'groups' => %w(Developers Freelancers Designers) }
-                        )
-                      })
-                  end
                 end
               end
 
               context 'when uid is a dn' do
-                it_behaves_like 'find ldap person', 'email' do
-                  let(:extern_uid) { 'uid=user1,ou=People,dc=example' }
-                  let(:auth_hash) do
-                    OmniAuth::AuthHash.new(
-                      uid: extern_uid,
-                      provider: provider,
-                      info: info_hash,
-                      extra: {
-                        raw_info: OneLogin::RubySaml::Attributes.new(
-                          { 'groups' => %w(Developers Freelancers Designers) }
-                        )
-                      })
-                  end
+                it_behaves_like 'find LDAP person', 'dn' do
+                  let(:extern_uid) { dn }
                 end
               end
 
               context 'when uid is an email' do
-                it_behaves_like 'find ldap person', 'email' do
+                it_behaves_like 'find LDAP person', 'email' do
                   let(:extern_uid) { 'john@mail.com' }
-                  let(:auth_hash) do
-                    OmniAuth::AuthHash.new(
-                      uid: extern_uid,
-                      provider: provider,
-                      info: info_hash,
-                      extra: {
-                        raw_info: OneLogin::RubySaml::Attributes.new(
-                          { 'groups' => %w(Developers Freelancers Designers) }
-                        )
-                      })
-                  end
                 end
               end
 
@@ -280,7 +276,7 @@ describe Gitlab::Saml::User do
                 expect(gl_user.email).to eql 'john@mail.com'
                 expect(gl_user.identities.length).to be 2
                 identities_as_hash = 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' },
+                expect(identities_as_hash).to match_array([{ provider: 'ldapmain', extern_uid: dn },
                                                            { provider: 'saml', extern_uid: uid }])
               end
 
@@ -296,17 +292,21 @@ describe Gitlab::Saml::User 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)
+                create(:omniauth_user, email: 'john@mail.com', extern_uid: dn, provider: 'saml', username: 'john')
+
+                allow(Gitlab::LDAP::Person).to receive(:find_by_uid).with(dn, adapter).and_return(ldap_user)
+
+                local_hash = OmniAuth::AuthHash.new(uid: dn, 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 be 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' }])
+                expect(identities_as_hash).to match_array([{ provider: 'ldapmain', extern_uid: dn },
+                                                           { provider: 'saml', extern_uid: dn }])
               end
             end
           end