Improve blocked user tracking and fire some events only once

4 files changed, 23 insertions, 9 deletions

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 73d7b8cb9cf..ce7bfb70c71 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,19 @@ 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.
+  #
+  #  `current_user` call is going to trigger Warden::Proxy authentication
+  #  that is going to invoke warden callbacks, so we use Warden directly here.
+  #
   def auth_user
-    return current_user if current_user.present?
-
-    return try(:authenticated_user)
+    if warden.authenticated?(:user)
+      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..872e82c4df2 100644
--- a/config/initializers/warden.rb
+++ b/config/initializers/warden.rb
@@ -35,7 +35,11 @@ Rails.application.configure do |config|
   Warden::Manager.before_logout(scope: :user) do |user_warden, auth, opts|
     user = user_warden || auth.user
 
+    Gitlab::Auth::Activity.new(user, opts).tap do |activity|
+      activity.user_blocked! if user.blocked?
+      activity.user_session_destroyed!
+    end
+
     ActiveSession.destroy(user, auth.request.session.id)
-    Gitlab::Auth::Activity.new(user, opts).user_session_destroyed!
   end
 end
diff --git a/lib/gitlab/auth/activity.rb b/lib/gitlab/auth/activity.rb
index 9f84c578d4f..711631ccd64 100644
--- a/lib/gitlab/auth/activity.rb
+++ b/lib/gitlab/auth/activity.rb
@@ -32,8 +32,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 +49,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/spec/features/users/login_spec.rb b/spec/features/users/login_spec.rb
index 44758f862a8..7f2d919b4fb 100644
--- a/spec/features/users/login_spec.rb
+++ b/spec/features/users/login_spec.rb
@@ -71,7 +71,7 @@ describe 'Login' do
       expect(authentication_metrics)
         .to increment(:user_blocked_counter)
         .and increment(:user_unauthenticated_counter)
-        .and increment(:user_session_destroyed_counter).twice
+        .and increment(:user_session_destroyed_counter)
 
       user = create(:user, :blocked)
 
@@ -84,7 +84,7 @@ describe 'Login' do
       expect(authentication_metrics)
         .to increment(:user_blocked_counter)
         .and increment(:user_unauthenticated_counter)
-        .and increment(:user_session_destroyed_counter).twice
+        .and increment(:user_session_destroyed_counter)
 
       user = create(:user, :blocked)