diff options
author | Stan Hu <stanhu@gmail.com> | 2018-08-23 15:57:49 +0000 |
---|---|---|
committer | Stan Hu <stanhu@gmail.com> | 2018-08-23 15:57:49 +0000 |
commit | a78c443de279b98726ed6ec5bc79df0e21dfdf41 (patch) | |
tree | c2282414c80c1609de57a781c871f6ebd9a5683c | |
parent | 087cfc6f9dae50e0ecaf34eb0adcf88da4995fda (diff) | |
parent | 5894dfabc5619112b3288f8ac65dbdb62ada713b (diff) | |
download | gitlab-ce-a78c443de279b98726ed6ec5bc79df0e21dfdf41.tar.gz |
Merge branch '6965-backport-to-ce' into 'master'
Backport LDAP changes to CE
See merge request gitlab-org/gitlab-ce!21361
-rw-r--r-- | lib/gitlab/auth/ldap/access.rb | 24 | ||||
-rw-r--r-- | spec/lib/gitlab/auth/ldap/access_spec.rb | 152 | ||||
-rw-r--r-- | spec/support/helpers/ldap_helpers.rb | 17 | ||||
-rw-r--r-- | spec/support/helpers/stub_configuration.rb | 4 |
4 files changed, 118 insertions, 79 deletions
diff --git a/lib/gitlab/auth/ldap/access.rb b/lib/gitlab/auth/ldap/access.rb index 865185eb5db..eeab7791643 100644 --- a/lib/gitlab/auth/ldap/access.rb +++ b/lib/gitlab/auth/ldap/access.rb @@ -19,8 +19,10 @@ module Gitlab # 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 + unless Gitlab::Database.read_only? + access.update_user + Users::UpdateService.new(user, user: user, last_credential_check_at: Time.now).execute + end true else @@ -60,6 +62,12 @@ module Gitlab false end + def update_user + # no-op in CE + end + + private + def adapter @adapter ||= Gitlab::Auth::LDAP::Adapter.new(provider) end @@ -68,16 +76,16 @@ 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 return unless provider @ldap_user ||= find_ldap_user end + def find_ldap_user + Gitlab::Auth::LDAP::Person.find_by_dn(ldap_identity.extern_uid, adapter) + end + def block_user(user, reason) user.ldap_block @@ -102,10 +110,6 @@ module Gitlab "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 eff21985108..7800c543cdb 100644 --- a/spec/lib/gitlab/auth/ldap/access_spec.rb +++ b/spec/lib/gitlab/auth/ldap/access_spec.rb @@ -3,51 +3,61 @@ require 'spec_helper' describe Gitlab::Auth::LDAP::Access do include LdapHelpers - let(:access) { described_class.new user } let(:user) { create(:omniauth_user) } + subject(:access) { described_class.new(user) } + describe '.allowed?' do - it 'updates the users `last_credential_check_at' do + before do allow(access).to receive(:update_user) - expect(access).to receive(:allowed?) { true } - expect(described_class).to receive(:open).and_yield(access) + allow(access).to receive(:allowed?).and_return(true) + allow(described_class).to receive(:open).and_yield(access) + end + it "updates the user's `last_credential_check_at`" do expect { described_class.allowed?(user) } .to change { user.last_credential_check_at } 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) + it "does not update user's `last_credential_check_at` when in a read-only GitLab instance" do + allow(Gitlab::Database).to receive(:read_only?).and_return(true) - access.find_ldap_user + expect { described_class.allowed?(user) } + .not_to change { user.last_credential_check_at } 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) + stub_ldap_person_find_by_dn(nil) + stub_ldap_person_find_by_email(user.email, nil) end - it { is_expected.to be_falsey } + it 'returns false' do + expect(access.allowed?).to be_falsey + end it 'blocks user in GitLab' do - expect(access).to receive(:block_user).with(user, 'does not exist anymore') + access.allowed? + + expect(user).to be_blocked + expect(user).to be_ldap_blocked + end + + it 'logs the reason' do + expect(Gitlab::AppLogger).to receive(:info).with( + "LDAP account \"123456\" does not exist anymore, " \ + "blocking Gitlab user \"#{user.name}\" (#{user.email})" + ) access.allowed? end end context 'when the user is found' do - let(:ldap_user) { Gitlab::Auth::LDAP::Person.new(Net::LDAP::Entry.new, 'ldapmain') } - before do - allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(ldap_user) + stub_ldap_person_find_by_dn(Net::LDAP::Entry.new) end context 'and the user is disabled via active directory' do @@ -55,10 +65,22 @@ describe Gitlab::Auth::LDAP::Access do allow(Gitlab::Auth::LDAP::Person).to receive(:disabled_via_active_directory?).and_return(true) end - it { is_expected.to be_falsey } + it 'returns false' do + expect(access.allowed?).to be_falsey + end it 'blocks user in GitLab' do - expect(access).to receive(:block_user).with(user, 'is disabled in Active Directory') + access.allowed? + + expect(user).to be_blocked + expect(user).to be_ldap_blocked + end + + it 'logs the reason' do + expect(Gitlab::AppLogger).to receive(:info).with( + "LDAP account \"123456\" is disabled in Active Directory, " \ + "blocking Gitlab user \"#{user.name}\" (#{user.email})" + ) access.allowed? end @@ -92,7 +114,17 @@ describe Gitlab::Auth::LDAP::Access do end it 'unblocks user in GitLab' do - expect(access).to receive(:unblock_user).with(user, 'is not disabled anymore') + access.allowed? + + expect(user).not_to be_blocked + expect(user).not_to be_ldap_blocked + end + + it 'logs the reason' do + expect(Gitlab::AppLogger).to receive(:info).with( + "LDAP account \"123456\" is not disabled anymore, " \ + "unblocking Gitlab user \"#{user.name}\" (#{user.email})" + ) access.allowed? end @@ -105,18 +137,32 @@ describe Gitlab::Auth::LDAP::Access do allow_any_instance_of(Gitlab::Auth::LDAP::Config).to receive(:active_directory).and_return(false) end - it { is_expected.to be_truthy } + it 'returns true' do + expect(access.allowed?).to be_truthy + end 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) + stub_ldap_person_find_by_dn(nil) + stub_ldap_person_find_by_email(user.email, nil) end - it { is_expected.to be_falsey } + it 'returns false' do + expect(access.allowed?).to be_falsey + end it 'blocks user in GitLab' do - expect(access).to receive(:block_user).with(user, 'does not exist anymore') + access.allowed? + + expect(user).to be_blocked + expect(user).to be_ldap_blocked + end + + it 'logs the reason' do + expect(Gitlab::AppLogger).to receive(:info).with( + "LDAP account \"123456\" does not exist anymore, " \ + "blocking Gitlab user \"#{user.name}\" (#{user.email})" + ) access.allowed? end @@ -128,7 +174,17 @@ describe Gitlab::Auth::LDAP::Access do end it 'unblocks the user if it exists' do - expect(access).to receive(:unblock_user).with(user, 'is available again') + access.allowed? + + expect(user).not_to be_blocked + expect(user).not_to be_ldap_blocked + end + + it 'logs the reason' do + expect(Gitlab::AppLogger).to receive(:info).with( + "LDAP account \"123456\" is available again, " \ + "unblocking Gitlab user \"#{user.name}\" (#{user.email})" + ) access.allowed? end @@ -152,46 +208,4 @@ describe Gitlab::Auth::LDAP::Access do end end end - - describe '#block_user' do - before do - user.activate - allow(Gitlab::AppLogger).to receive(:info) - - access.block_user user, 'reason' - end - - it 'blocks the user' do - expect(user).to be_blocked - expect(user).to be_ldap_blocked - end - - it 'logs the reason' do - expect(Gitlab::AppLogger).to have_received(:info).with( - "LDAP account \"123456\" reason, " \ - "blocking Gitlab user \"#{user.name}\" (#{user.email})" - ) - end - end - - describe '#unblock_user' do - before do - user.ldap_block - allow(Gitlab::AppLogger).to receive(:info) - - access.unblock_user user, 'reason' - end - - it 'activates the user' do - expect(user).not_to be_blocked - expect(user).not_to be_ldap_blocked - end - - it 'logs the reason' do - Gitlab::AppLogger.info( - "LDAP account \"123456\" reason, " \ - "unblocking Gitlab user \"#{user.name}\" (#{user.email})" - ) - end - end end diff --git a/spec/support/helpers/ldap_helpers.rb b/spec/support/helpers/ldap_helpers.rb index b90bbc4b106..66ca5d7f0a3 100644 --- a/spec/support/helpers/ldap_helpers.rb +++ b/spec/support/helpers/ldap_helpers.rb @@ -37,6 +37,23 @@ module LdapHelpers .to receive(:find_by_uid).with(uid, any_args).and_return(return_value) end + def stub_ldap_person_find_by_dn(entry, provider = 'ldapmain') + person = ::Gitlab::Auth::LDAP::Person.new(entry, provider) if entry.present? + + allow(::Gitlab::Auth::LDAP::Person) + .to receive(:find_by_dn) + .and_return(person) + end + + def stub_ldap_person_find_by_email(email, entry, provider = 'ldapmain') + person = ::Gitlab::Auth::LDAP::Person.new(entry, provider) if entry.present? + + allow(::Gitlab::Auth::LDAP::Person) + .to receive(:find_by_email) + .with(email, anything) + .and_return(person) + end + # Create a simple LDAP user entry. def ldap_user_entry(uid) entry = Net::LDAP::Entry.new diff --git a/spec/support/helpers/stub_configuration.rb b/spec/support/helpers/stub_configuration.rb index 1823099dd9c..8475f91799b 100644 --- a/spec/support/helpers/stub_configuration.rb +++ b/spec/support/helpers/stub_configuration.rb @@ -68,6 +68,10 @@ module StubConfiguration allow(Gitlab.config.repositories).to receive(:storages).and_return(Settingslogic.new(messages)) end + def stub_kerberos_setting(messages) + allow(Gitlab.config.kerberos).to receive_messages(to_settings(messages)) + end + private # Modifies stubbed messages to also stub possible predicate versions |