From e87e28059826fe0126e737ef3fea38bb34d44a78 Mon Sep 17 00:00:00 2001 From: Markus Koller Date: Mon, 12 Dec 2016 15:13:23 +0100 Subject: [PATCH] Log messages when blocking/unblocking LDAP accounts --- .../feature-log-ldap-to-application-log.yml | 4 ++ doc/administration/auth/ldap.md | 2 +- lib/gitlab/ldap/access.rb | 26 ++++++-- spec/lib/gitlab/ldap/access_spec.rb | 63 ++++++++++++++++--- 4 files changed, 82 insertions(+), 13 deletions(-) create mode 100644 changelogs/unreleased/feature-log-ldap-to-application-log.yml diff --git a/changelogs/unreleased/feature-log-ldap-to-application-log.yml b/changelogs/unreleased/feature-log-ldap-to-application-log.yml new file mode 100644 index 00000000000..4cfbc23edb7 --- /dev/null +++ b/changelogs/unreleased/feature-log-ldap-to-application-log.yml @@ -0,0 +1,4 @@ +--- +title: Log LDAP blocking/unblocking events to application log +merge_request: 8042 +author: Markus Koller diff --git a/doc/administration/auth/ldap.md b/doc/administration/auth/ldap.md index b8b63df091e..f14d2f44dd4 100644 --- a/doc/administration/auth/ldap.md +++ b/doc/administration/auth/ldap.md @@ -302,4 +302,4 @@ GitLab. Common combinations are `method: 'plain'` and `port: 389`, OR If there is an unexpected error while authenticating the user with the LDAP backend, the login is rejected and details about the error are logged to -`production.log`. +`application.log`. diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index 7e06bd2b0fb..f2e53953a37 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -34,21 +34,21 @@ module Gitlab def allowed? if ldap_user unless ldap_config.active_directory - user.activate if user.ldap_blocked? + unblock_user(user, 'is not in Active Directory anymore') if user.ldap_blocked? return true end # Block user in GitLab if he/she was blocked in AD if Gitlab::LDAP::Person.disabled_via_active_directory?(user.ldap_identity.extern_uid, adapter) - user.ldap_block + block_user(user, 'is disabled in Active Directory') false else - user.activate if user.ldap_blocked? + unblock_user(user, 'is not disabled anymore') if user.ldap_blocked? true end else # Block the user if they no longer exist in LDAP/AD - user.ldap_block + block_user(user, 'does not exist anymore') false end end @@ -64,6 +64,24 @@ module Gitlab def ldap_user @ldap_user ||= Gitlab::LDAP::Person.find_by_dn(user.ldap_identity.extern_uid, adapter) 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})" + ) + end + + def unblock_user(user, reason) + user.activate + + Gitlab::AppLogger.info( + "LDAP account \"#{user.ldap_identity.extern_uid}\" #{reason}, " + + "unblocking Gitlab user \"#{user.name}\" (#{user.email})" + ) + end end end end diff --git a/spec/lib/gitlab/ldap/access_spec.rb b/spec/lib/gitlab/ldap/access_spec.rb index 534bcbf39fe..ea64f8a3e0e 100644 --- a/spec/lib/gitlab/ldap/access_spec.rb +++ b/spec/lib/gitlab/ldap/access_spec.rb @@ -15,9 +15,9 @@ describe Gitlab::LDAP::Access, lib: true do it { is_expected.to be_falsey } it 'should block 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 end @@ -34,9 +34,9 @@ describe Gitlab::LDAP::Access, lib: true do it { is_expected.to be_falsey } 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 end @@ -53,7 +53,10 @@ describe Gitlab::LDAP::Access, lib: true do end it 'does not unblock user in GitLab' do + expect(access).not_to receive(:unblock_user) + access.allowed? + expect(user).to be_blocked expect(user).not_to be_ldap_blocked # this block is handled by omniauth not by our internal logic end @@ -65,8 +68,9 @@ describe Gitlab::LDAP::Access, lib: true 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 end end end @@ -87,9 +91,9 @@ describe Gitlab::LDAP::Access, lib: true do it { is_expected.to be_falsey } 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 end @@ -99,11 +103,54 @@ describe Gitlab::LDAP::Access, lib: true do end it 'unblocks the user if it exists' do + expect(access).to receive(:unblock_user).with(user, 'is not in Active Directory anymore') + access.allowed? - expect(user).not_to be_blocked end end 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 -- GitLab