diff --git a/lib/gitlab/auth/ldap/access.rb b/lib/gitlab/auth/ldap/access.rb index 34286900e72..865185eb5db 100644 --- a/lib/gitlab/auth/ldap/access.rb +++ b/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 - Gitlab::AppLogger.info( - "LDAP account \"#{user.ldap_identity.extern_uid}\" #{reason}, " \ - "blocking Gitlab user \"#{user.name}\" (#{user.email})" - ) + if provider + Gitlab::AppLogger.info( + "LDAP account \"#{ldap_identity.extern_uid}\" #{reason}, " \ + "blocking Gitlab user \"#{user.name}\" (#{user.email})" + ) + else + Gitlab::AppLogger.info( + "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 diff --git a/spec/lib/gitlab/auth/ldap/access_spec.rb b/spec/lib/gitlab/auth/ldap/access_spec.rb index 6b251d824f7..eff21985108 100644 --- a/spec/lib/gitlab/auth/ldap/access_spec.rb +++ b/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 }