Refactors SAML identity creation in gl_user.

5 files changed, 58 insertions, 102 deletions

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..197dd3f97aa 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,54 @@ 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
+        # 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)
+        identity.extern_uid = auth_hash.uid
+
+        if auto_link_ldap_user? && !gl_user.ldap_user? && ldap_person
+          log.info "Correct LDAP account has been found. identity to user: #{gl_user.username}."
+          gl_user.identities.build(provider: ldap_person.provider, extern_uid: ldap_person.dn)
+        end
+      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 +116,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
@@ -155,7 +160,7 @@ module Gitlab
       end
 
       def build_new_user
-        user_params = user_attributes.merge(extern_uid: auth_hash.uid, provider: auth_hash.provider, skip_confirmation: true)
+        user_params = user_attributes.merge(skip_confirmation: true)
         Users::BuildService.new(nil, user_params).execute(skip_authorization: true)
       end
 
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?