diff --git a/lib/gitlab/auth/blocked_user_tracker.rb b/lib/gitlab/auth/blocked_user_tracker.rb index 3d2011fb118..b6d2adc834b 100644 --- a/lib/gitlab/auth/blocked_user_tracker.rb +++ b/lib/gitlab/auth/blocked_user_tracker.rb @@ -10,6 +10,34 @@ module Gitlab @env = env end + def user_blocked? + user&.blocked? + end + + def user + return unless has_user_blocked_message? + + strong_memoize(:user) do + # Check for either LDAP or regular GitLab account logins + login = @env.dig(ACTIVE_RECORD_REQUEST_PARAMS, 'username') || + @env.dig(ACTIVE_RECORD_REQUEST_PARAMS, 'user', 'login') + + User.by_login(login) if login.present? + end + rescue TypeError + end + + def log_blocked_user_activity! + return unless user_blocked? + + Gitlab::AppLogger.info("Failed login for blocked user: user=#{user.username} ip=#{@env['REMOTE_ADDR']}") + SystemHooksService.new.execute_hooks_for(user, :failed_login) + true + rescue TypeError + end + + private + ## # Devise calls User#active_for_authentication? on the User model and then # throws an exception to Warden with User#inactive_message: @@ -26,31 +54,6 @@ module Gitlab message == User::BLOCKED_MESSAGE end end - - def user - return unless has_user_blocked_message? - - strong_memoize(:user) do - # Check for either LDAP or regular GitLab account logins - login = @env.dig(ACTIVE_RECORD_REQUEST_PARAMS, 'username') || - @env.dig(ACTIVE_RECORD_REQUEST_PARAMS, 'user', 'login') - - User.by_login(login) if login.present? - end - end - - def user_blocked? - user&.blocked? - end - - def log_blocked_user_activity! - return unless user_blocked? - - Gitlab::AppLogger.info("Failed login for blocked user: user=#{user.username} ip=#{@env['REMOTE_ADDR']}") - SystemHooksService.new.execute_hooks_for(user, :failed_login) - true - rescue TypeError - end end end end diff --git a/spec/lib/gitlab/auth/blocked_user_tracker_spec.rb b/spec/lib/gitlab/auth/blocked_user_tracker_spec.rb index 8f96a5bf414..13c09b9cb9b 100644 --- a/spec/lib/gitlab/auth/blocked_user_tracker_spec.rb +++ b/spec/lib/gitlab/auth/blocked_user_tracker_spec.rb @@ -3,26 +3,30 @@ require 'spec_helper' describe Gitlab::Auth::BlockedUserTracker do set(:user) { create(:user) } - # TODO, add more specs - describe '#log_blocked_user_activity!' do it 'does not log if user failed to login due to undefined reason' do expect_any_instance_of(SystemHooksService).not_to receive(:execute_hooks_for) - expect(described_class.new({}).log_blocked_user_activity!).to be_nil + tracker = described_class.new({}) + + expect(tracker.user).to be_nil + expect(tracker.user_blocked?).to be_falsey + expect(tracker.log_blocked_user_activity!).to be_nil end it 'gracefully handles malformed environment variables' do - env = { 'warden.options' => 'test' } + tracker = described_class.new({ 'warden.options' => 'test' }) - expect(described_class.new(env).log_blocked_user_activity!).to be_nil + expect(tracker.user).to be_nil + expect(tracker.user_blocked?).to be_falsey + expect(tracker.log_blocked_user_activity!).to be_nil end context 'failed login due to blocked user' do let(:base_env) { { 'warden.options' => { message: User::BLOCKED_MESSAGE } } } let(:env) { base_env.merge(request_env) } - subject { described_class.new(env).log_blocked_user_activity! } + subject { described_class.new(env) } before do expect_any_instance_of(SystemHooksService).to receive(:execute_hooks_for).with(user, :failed_login) @@ -34,14 +38,17 @@ describe Gitlab::Auth::BlockedUserTracker do it 'logs a blocked user' do user.block! - expect(subject).to be_truthy + expect(subject.user).to be_blocked + expect(subject.user_blocked?).to be true + expect(subject.log_blocked_user_activity!).to be_truthy end it 'logs a blocked user by e-mail' do user.block! env[described_class::ACTIVE_RECORD_REQUEST_PARAMS]['user']['login'] = user.email - expect(subject).to be_truthy + expect(subject.user).to be_blocked + expect(subject.log_blocked_user_activity!).to be_truthy end end @@ -51,13 +58,17 @@ describe Gitlab::Auth::BlockedUserTracker do it 'logs a blocked user' do user.block! - expect(subject).to be_truthy + expect(subject.user).to be_blocked + expect(subject.user_blocked?).to be true + expect(subject.log_blocked_user_activity!).to be_truthy end it 'logs a LDAP blocked user' do user.ldap_block! - expect(subject).to be_truthy + expect(subject.user).to be_blocked + expect(subject.user_blocked?).to be true + expect(subject.log_blocked_user_activity!).to be_truthy end end end