Merge branch '5913-extract-ee-specific-lines-for-lib-gitlab-auth' into 'master'

[CE] Resolve "Extract EE specific files/lines for lib/gitlab/auth"

See merge request gitlab-org/gitlab-ce!19037

lib/gitlab/auth/ldap/access.rb

@@ -6,7 +6,7 @@ module Gitlab
   module Auth
     module LDAP
       class Access
-        attr_reader :provider, :user
+        attr_reader :provider, :user, :ldap_identity
 
         def self.open(user, &block)
           Gitlab::Auth::LDAP::Adapter.open(user.ldap_identity.provider) do |adapter|
@@ -14,9 +14,12 @@ module Gitlab
           end
         end
 
-        def self.allowed?(user)
+        def self.allowed?(user, options = {})
           self.open(user) do |access|
+            # Whether user is allowed, or not, we should update
+            # permissions to keep things clean
             if access.allowed?
+              access.update_user
               Users::UpdateService.new(user, user: user, last_credential_check_at: Time.now).execute
 
               true
@@ -29,7 +32,8 @@ module Gitlab
         def initialize(user, adapter = nil)
           @adapter = adapter
           @user = user
-          @provider = user.ldap_identity.provider
+          @ldap_identity = user.ldap_identity
+          @provider = adapter&.provider || ldap_identity&.provider
         end
 
         def allowed?
@@ -40,7 +44,7 @@ module Gitlab
             end
 
             # Block user in GitLab if he/she was blocked in AD
-            if Gitlab::Auth::LDAP::Person.disabled_via_active_directory?(user.ldap_identity.extern_uid, adapter)
+            if Gitlab::Auth::LDAP::Person.disabled_via_active_directory?(ldap_identity.extern_uid, adapter)
               block_user(user, 'is disabled in Active Directory')
               false
             else
@@ -64,27 +68,44 @@ module Gitlab
           Gitlab::Auth::LDAP::Config.new(provider)
         end
 
+        def find_ldap_user
+          Gitlab::Auth::LDAP::Person.find_by_dn(ldap_identity.extern_uid, adapter)
+        end
+
         def ldap_user
-          @ldap_user ||= Gitlab::Auth::LDAP::Person.find_by_dn(user.ldap_identity.extern_uid, adapter)
+          return unless provider
+
+          @ldap_user ||= find_ldap_user
         end
 
         def block_user(user, reason)
           user.ldap_block
 
+          if provider
+            Gitlab::AppLogger.info(
+              "LDAP account \"#{ldap_identity.extern_uid}\" #{reason}, " \
+              "blocking Gitlab user \"#{user.name}\" (#{user.email})"
+            )
+          else
             Gitlab::AppLogger.info(
-            "LDAP account \"#{user.ldap_identity.extern_uid}\" #{reason}, " \
+              "Account is not provided by LDAP, " \
               "blocking Gitlab user \"#{user.name}\" (#{user.email})"
             )
           end
+        end
 
         def unblock_user(user, reason)
           user.activate
 
           Gitlab::AppLogger.info(
-            "LDAP account \"#{user.ldap_identity.extern_uid}\" #{reason}, " \
+            "LDAP account \"#{ldap_identity.extern_uid}\" #{reason}, " \
             "unblocking Gitlab user \"#{user.name}\" (#{user.email})"
           )
         end
+
+        def update_user
+          # no-op in CE
+        end
       end
     end
   end

lib/gitlab/auth/ldap/config.rb

@@ -11,6 +11,8 @@ module Gitlab
 
         attr_accessor :provider, :options
 
+        InvalidProvider = Class.new(StandardError)
+
         def self.enabled?
           Gitlab.config.ldap.enabled
         end
@@ -22,6 +24,10 @@ module Gitlab
         def self.available_servers
           return [] unless enabled?
 
+          _available_servers
+        end
+
+        def self._available_servers
           Array.wrap(servers.first)
         end
 
@@ -34,7 +40,7 @@ module Gitlab
         end
 
         def self.invalid_provider(provider)
-          raise "Unknown provider (#{provider}). Available providers: #{providers}"
+          raise InvalidProvider.new("Unknown provider (#{provider}). Available providers: #{providers}")
         end
 
         def initialize(provider)
@@ -84,13 +90,17 @@ module Gitlab
         end
 
         def base
-          options['base']
+          @base ||= Person.normalize_dn(options['base'])
         end
 
         def uid
           options['uid']
         end
 
+        def label
+          options['label']
+        end
+
         def sync_ssh_keys?
           sync_ssh_keys.present?
         end
@@ -132,6 +142,10 @@ module Gitlab
           options['timeout'].to_i
         end
 
+        def external_groups
+          options['external_groups'] || []
+        end
+
         def has_auth?
           options['password'] || options['bind_dn']
         end

lib/gitlab/auth/saml/config.rb

@@ -14,6 +14,10 @@ module Gitlab
           def external_groups
             options[:external_groups]
           end
+
+          def admin_groups
+            options[:admin_groups]
+          end
         end
       end
     end

lib/gitlab/auth/saml/user.rb

@@ -20,10 +20,8 @@ module Gitlab
           user ||= find_or_build_ldap_user if auto_link_ldap_user?
           user ||= build_new_user if signup_enabled?
 
-          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 = !(auth_hash.groups & saml_config.external_groups).empty?
+          if user
+            user.external = !(auth_hash.groups & saml_config.external_groups).empty? if external_users_enabled?
           end
 
           user

lib/gitlab/auth/user_auth_finders.rb

 module Gitlab
   module Auth
-    #
-    # Exceptions
-    #
-
     AuthenticationError = Class.new(StandardError)
     MissingTokenError = Class.new(AuthenticationError)
     TokenNotFoundError = Class.new(AuthenticationError)
@@ -61,6 +57,12 @@ module Gitlab
 
       private
 
+      def route_authentication_setting
+        return {} unless respond_to?(:route_setting)
+
+        route_setting(:authentication) || {}
+      end
+
       def access_token
         strong_memoize(:access_token) do
           find_oauth_access_token || find_personal_access_token

spec/lib/gitlab/auth/ldap/access_spec.rb

@@ -8,6 +8,7 @@ describe Gitlab::Auth::LDAP::Access do
 
   describe '.allowed?' do
     it 'updates the users `last_credential_check_at' do
+      allow(access).to receive(:update_user)
       expect(access).to receive(:allowed?) { true }
       expect(described_class).to receive(:open).and_yield(access)
 
@@ -16,12 +17,21 @@ describe Gitlab::Auth::LDAP::Access do
     end
   end
 
+  describe '#find_ldap_user' do
+    it 'finds a user by dn first' do
+      expect(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(:ldap_user)
+
+      access.find_ldap_user
+    end
+  end
+
   describe '#allowed?' do
     subject { access.allowed? }
 
     context 'when the user cannot be found' do
       before do
         allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(nil)
+        allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_email).and_return(nil)
       end
 
       it { is_expected.to be_falsey }
@@ -54,7 +64,7 @@ describe Gitlab::Auth::LDAP::Access do
         end
       end
 
-      context 'and has no disabled flag in active diretory' do
+      context 'and has no disabled flag in active directory' do
         before do
           allow(Gitlab::Auth::LDAP::Person).to receive(:disabled_via_active_directory?).and_return(false)
         end
@@ -100,6 +110,7 @@ describe Gitlab::Auth::LDAP::Access do
         context 'when user cannot be found' do
           before do
             allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(nil)
+            allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_email).and_return(nil)
           end
 
           it { is_expected.to be_falsey }

spec/lib/gitlab/auth/ldap/config_spec.rb

@@ -23,7 +23,7 @@ describe Gitlab::Auth::LDAP::Config do
     end
 
     it 'raises an error if a unknown provider is used' do
-      expect { described_class.new 'unknown' }.to raise_error(RuntimeError)
+      expect { described_class.new 'unknown' }.to raise_error(described_class::InvalidProvider)
     end
   end
 
@@ -370,4 +370,38 @@ describe Gitlab::Auth::LDAP::Config do
       })
     end
   end
+
+  describe '#base' do
+    context 'when the configured base is not normalized' do
+      it 'returns the normalized base' do
+        stub_ldap_config(options: { 'base' => 'DC=example, DC= com' })
+
+        expect(config.base).to eq('dc=example,dc=com')
+      end
+    end
+
+    context 'when the configured base is normalized' do
+      it 'returns the base unaltered' do
+        stub_ldap_config(options: { 'base' => 'dc=example,dc=com' })
+
+        expect(config.base).to eq('dc=example,dc=com')
+      end
+    end
+
+    context 'when the configured base is malformed' do
+      it 'returns the base unaltered' do
+        stub_ldap_config(options: { 'base' => 'invalid,dc=example,dc=com' })
+
+        expect(config.base).to eq('invalid,dc=example,dc=com')
+      end
+    end
+
+    context 'when the configured base is blank' do
+      it 'returns the base unaltered' do
+        stub_ldap_config(options: { 'base' => '' })
+
+        expect(config.base).to eq('')
+      end
+    end
+  end
 end

spec/lib/gitlab/auth/ldap/user_spec.rb

 require 'spec_helper'
 
 describe Gitlab::Auth::LDAP::User do
+  include LdapHelpers
+
   let(:ldap_user) { described_class.new(auth_hash) }
   let(:gl_user) { ldap_user.gl_user }
   let(:info) do
@@ -177,8 +179,7 @@ describe Gitlab::Auth::LDAP::User do
 
   describe 'blocking' do
     def configure_block(value)
-      allow_any_instance_of(Gitlab::Auth::LDAP::Config)
-          .to receive(:block_auto_created_users).and_return(value)
+      stub_ldap_config(block_auto_created_users: value)
     end
 
     context 'signup' do