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

2 files changed, 14 insertions, 50 deletions

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