Merge branch 'fix/gb/improve-blocked-user-tracking' into 'master'

Improve blocked user tracking and fire some events only once

Closes #49784

See merge request gitlab-org/gitlab-ce!20959

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

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

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

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

spec/lib/gitlab/auth/blocked_user_tracker_spec.rb

 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