diff options
-rw-r--r-- | app/controllers/application_controller.rb | 13 | ||||
-rw-r--r-- | config/initializers/warden.rb | 32 | ||||
-rw-r--r-- | lib/gitlab/auth/activity.rb | 9 | ||||
-rw-r--r-- | lib/gitlab/auth/blocked_user_tracker.rb | 55 | ||||
-rw-r--r-- | spec/lib/gitlab/auth/blocked_user_tracker_spec.rb | 79 |
5 files changed, 64 insertions, 124 deletions
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 73d7b8cb9cf..783831748a7 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -108,6 +108,7 @@ class ApplicationController < ActionController::Base def append_info_to_payload(payload) super + payload[:remote_ip] = request.remote_ip logged_user = auth_user @@ -122,12 +123,16 @@ class ApplicationController < ActionController::Base end end + ## # Controllers such as GitHttpController may use alternative methods - # (e.g. tokens) to authenticate the user, whereas Devise sets current_user + # (e.g. tokens) to authenticate the user, whereas Devise sets current_user. + # def auth_user - return current_user if current_user.present? - - return try(:authenticated_user) + if user_signed_in? + current_user + else + try(:authenticated_user) + end end # This filter handles personal access tokens, and atom requests with rss tokens diff --git a/config/initializers/warden.rb b/config/initializers/warden.rb index d64b659c6d7..33f55069c3e 100644 --- a/config/initializers/warden.rb +++ b/config/initializers/warden.rb @@ -2,7 +2,7 @@ Rails.application.configure do |config| Warden::Manager.after_set_user(scope: :user) do |user, auth, opts| Gitlab::Auth::UniqueIpsLimiter.limit_user!(user) - activity = Gitlab::Auth::Activity.new(user, opts) + activity = Gitlab::Auth::Activity.new(opts) case opts[:event] when :authentication @@ -26,16 +26,32 @@ Rails.application.configure do |config| end Warden::Manager.before_failure(scope: :user) do |env, opts| - tracker = Gitlab::Auth::BlockedUserTracker.new(env) - tracker.log_blocked_user_activity! if tracker.user_blocked? - - Gitlab::Auth::Activity.new(tracker.user, opts).user_authentication_failed! + Gitlab::Auth::Activity.new(opts).user_authentication_failed! end - Warden::Manager.before_logout(scope: :user) do |user_warden, auth, opts| - user = user_warden || auth.user + Warden::Manager.before_logout(scope: :user) do |user, auth, opts| + user ||= auth.user + activity = Gitlab::Auth::Activity.new(opts) + tracker = Gitlab::Auth::BlockedUserTracker.new(user, auth) ActiveSession.destroy(user, auth.request.session.id) - Gitlab::Auth::Activity.new(user, opts).user_session_destroyed! + activity.user_session_destroyed! + + ## + # It is possible that `before_logout` event is going to be triggered + # multiple times during the request lifecycle. We want to increment + # metrics and write logs only once in that case. + # + # 'warden.auth.*' is our custom hash key that follows usual convention + # of naming keys in the Rack env hash. + # + next if auth.env['warden.auth.user.blocked'] + + if user.blocked? + activity.user_blocked! + tracker.log_activity! + end + + auth.env['warden.auth.user.blocked'] = true end end diff --git a/lib/gitlab/auth/activity.rb b/lib/gitlab/auth/activity.rb index 9f84c578d4f..761f0819c60 100644 --- a/lib/gitlab/auth/activity.rb +++ b/lib/gitlab/auth/activity.rb @@ -18,8 +18,7 @@ module Gitlab user_blocked: 'Counter of sign in attempts when user is blocked' }.freeze - def initialize(user, opts) - @user = user + def initialize(opts) @opts = opts end @@ -32,8 +31,6 @@ module Gitlab when :invalid self.class.user_password_invalid_counter_increment! end - - self.class.user_blocked_counter_increment! if @user&.blocked? end def user_authenticated! @@ -51,6 +48,10 @@ module Gitlab end end + def user_blocked! + self.class.user_blocked_counter_increment! + end + def user_session_destroyed! self.class.user_session_destroyed_counter_increment! end diff --git a/lib/gitlab/auth/blocked_user_tracker.rb b/lib/gitlab/auth/blocked_user_tracker.rb index b6d2adc834b..50712d7eac2 100644 --- a/lib/gitlab/auth/blocked_user_tracker.rb +++ b/lib/gitlab/auth/blocked_user_tracker.rb @@ -2,58 +2,21 @@ module Gitlab module Auth class BlockedUserTracker - include Gitlab::Utils::StrongMemoize - - ACTIVE_RECORD_REQUEST_PARAMS = 'action_dispatch.request.request_parameters' - - def initialize(env) - @env = env - end - - def user_blocked? - user&.blocked? + def initialize(user, auth) + @user = user + @auth = auth end - def user - return unless has_user_blocked_message? + def log_activity! + return unless @user.blocked? - 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') + Gitlab::AppLogger.info <<~INFO + "Failed login for blocked user: user=#{@user.username} ip=#{@auth.request.ip}") + INFO - User.by_login(login) if login.present? - end + SystemHooksService.new.execute_hooks_for(@user, :failed_login) 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: - # https://github.com/plataformatec/devise/blob/v4.2.1/lib/devise/hooks/activatable.rb#L8 - # - # Since Warden doesn't pass the user record to the failure handler, we - # need to do a database lookup with the username. We can limit the - # lookups to happen when the user was blocked by checking the inactive - # message passed along by Warden. - # - def has_user_blocked_message? - strong_memoize(:user_blocked_message) do - message = @env.dig('warden.options', :message) - message == User::BLOCKED_MESSAGE - end - 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 13c09b9cb9b..f39863fdda1 100644 --- a/spec/lib/gitlab/auth/blocked_user_tracker_spec.rb +++ b/spec/lib/gitlab/auth/blocked_user_tracker_spec.rb @@ -1,75 +1,30 @@ require 'spec_helper' describe Gitlab::Auth::BlockedUserTracker do - set(:user) { create(:user) } - 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) - - tracker = described_class.new({}) + context 'when user is not blocked' do + it 'does not log blocked user activity' do + expect_any_instance_of(SystemHooksService) + .not_to receive(:execute_hooks_for) + expect(Gitlab::AppLogger).not_to receive(:info) - expect(tracker.user).to be_nil - expect(tracker.user_blocked?).to be_falsey - expect(tracker.log_blocked_user_activity!).to be_nil - end + user = create(:user) - it 'gracefully handles malformed environment variables' do - tracker = described_class.new({ 'warden.options' => 'test' }) - - 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) } - - before do - expect_any_instance_of(SystemHooksService).to receive(:execute_hooks_for).with(user, :failed_login) + described_class.new(user, spy('auth')).log_activity! end + end - context 'via GitLab login' do - let(:request_env) { { described_class::ACTIVE_RECORD_REQUEST_PARAMS => { 'user' => { 'login' => user.username } } } } - - it 'logs a blocked user' do - user.block! - - 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.user).to be_blocked - expect(subject.log_blocked_user_activity!).to be_truthy - end - end - - context 'via LDAP login' do - let(:request_env) { { described_class::ACTIVE_RECORD_REQUEST_PARAMS => { 'username' => user.username } } } - - it 'logs a blocked user' do - user.block! - - expect(subject.user).to be_blocked - expect(subject.user_blocked?).to be true - expect(subject.log_blocked_user_activity!).to be_truthy - end + context 'when user is not blocked' do + it 'logs blocked user activity' do + user = create(:user, :blocked) - it 'logs a LDAP blocked user' do - user.ldap_block! + expect_any_instance_of(SystemHooksService) + .to receive(:execute_hooks_for) + .with(user, :failed_login) + expect(Gitlab::AppLogger).to receive(:info) + .with(/Failed login for blocked user/) - expect(subject.user).to be_blocked - expect(subject.user_blocked?).to be true - expect(subject.log_blocked_user_activity!).to be_truthy - end + described_class.new(user, spy('auth')).log_activity! end end end |