Track blocked users and two factor authentications

app/controllers/concerns/authenticates_with_two_factor.rb

@@ -60,7 +60,7 @@ module AuthenticatesWithTwoFactor
       remember_me(user) if user_params[:remember_me] == '1'
       user.save!
-      sign_in(user)
+      sign_in(user, message: :two_factor_authenticated)
     else
       user.increment_failed_attempts!
       Gitlab::AppLogger.info("Failed Login: user=#{user.username} ip=#{request.remote_ip} method=OTP")
@@ -77,7 +77,7 @@ module AuthenticatesWithTwoFactor
       session.delete(:challenge)
       remember_me(user) if user_params[:remember_me] == '1'
-      sign_in(user)
+      sign_in(user, message: :two_factor_authenticated)
     else
       user.increment_failed_attempts!
       Gitlab::AppLogger.info("Failed Login: user=#{user.username} ip=#{request.remote_ip} method=U2F")

app/controllers/sessions_controller.rb

@@ -89,6 +89,14 @@ class SessionsController < Devise::SessionsController
     ).increment
   end
+  ##
+  # We do have some duplication between lib/gitlab/auth/activity.rb here, but
+  # leaving this method here because of backwards compatibility.
+  #
+  def login_counter
+    @login_counter ||= Gitlab::Metrics.counter(:user_session_logins_total, 'User sign in count')
+  end
+
   def log_failed_login
     Gitlab::AppLogger.info("Failed Login: username=#{user_params[:login]} ip=#{request.remote_ip}")
   end
@@ -97,10 +105,6 @@ class SessionsController < Devise::SessionsController
     (options = env["warden.options"]) && options[:action] == "unauthenticated"
   end
-  def login_counter
-    @login_counter ||= Gitlab::Metrics.counter(:user_session_logins_total, 'User sign in count')
-  end
-
   # Handle an "initial setup" state, where there's only one user, it's an admin,
   # and they require a password change.
   def check_initial_setup

config/initializers/warden.rb

@@ -18,17 +18,16 @@ Rails.application.configure do |config|
   Warden::Manager.after_set_user(scope: :user, only: :fetch) do |user, auth, opts|
     ActiveSession.set(user, auth.request)
-    Gitlab::Auth::Activity.new(user, opts).user_session_fetched!
   end
   Warden::Manager.after_set_user(scope: :user, only: :set_user) do |user, auth, opts|
     Gitlab::Auth::Activity.new(user, opts).user_session_override!
   end
-  Warden::Manager.before_logout(scope: :user) do |warden_user, auth, opts|
-    (warden_user || auth.user).tap do |user|
-      ActiveSession.destroy(user, auth.request.session.id)
-      Gitlab::Auth::Activity.new(user, opts).user_signed_out!
-    end
+  Warden::Manager.before_logout(scope: :user) do |user_warden, auth, opts|
+    user = user_warden || auth.user
+
+    ActiveSession.destroy(user, auth.request.session.id)
+    Gitlab::Auth::Activity.new(user, opts).user_signed_out!
   end
 end

lib/gitlab/auth/activity.rb

@@ -11,8 +11,9 @@ module Gitlab
         user_unauthenticated: 'Counter of total authentication failures',
         user_not_found: 'Counter of total failed log-ins when user is unknown',
         user_password_invalid: 'Counter of failed log-ins with invalid password',
-        user_session_fetched: 'Counter of total sessions fetched',
         user_session_override: 'Counter of manual log-ins and sessions overrides',
+        user_two_factor_authenticated: 'Counter of two factor authentications',
+        user_blocked: 'Counter of total sign in attempts when user is blocked',
         user_signed_out: 'Counter of total user sign out events'
       }.freeze
@@ -31,19 +32,22 @@ module Gitlab
           self.class.user_password_invalid_counter.increment
         end
-        # case blocked user
+        if @user.present? && @user.blocked?
+          self.class.user_blocked_counter.increment
+        end
       end
       def user_authenticated!
         self.class.user_authenticated_counter.increment
       end
-      def user_session_fetched!
-        self.class.user_session_fetched_counter.increment
-      end
-
       def user_session_override!
+        self.class.user_authenticated_counter.increment
         self.class.user_session_override_counter.increment
+
+        if @opts[:message] == :two_factor_authenticated
+          self.class.user_two_factor_authenticated_counter.increment
+        end
       end
       def user_signed_out!

spec/features/users/login_spec.rb

@@ -4,31 +4,39 @@ describe 'Login' do
   include TermsHelper
   before do
-    stub_authentication_activity_metrics
+    stub_authentication_activity_metrics(debug: true)
   end
-  it 'Successful user signin invalidates password reset token' do
-    user = create(:user)
-    expect(user.reset_password_token).to be_nil
-    visit new_user_password_path
-    fill_in 'user_email', with: user.email
-    click_button 'Reset password'
-    user.reload
-    expect(user.reset_password_token).not_to be_nil
-    find('a[href="#login-pane"]').click
-    gitlab_sign_in(user)
-    expect(current_path).to eq root_path
-    user.reload
-    expect(user.reset_password_token).to be_nil
+  describe 'password reset token after successful sign in' do
+    it 'invalidates password reset token' do
+      expect(authentication_metrics)
+        .to increment(:user_authenticated_counter)
+      user = create(:user)
+      expect(user.reset_password_token).to be_nil
+      visit new_user_password_path
+      fill_in 'user_email', with: user.email
+      click_button 'Reset password'
+      user.reload
+      expect(user.reset_password_token).not_to be_nil
+      find('a[href="#login-pane"]').click
+      gitlab_sign_in(user)
+      expect(current_path).to eq root_path
+
+      user.reload
+      expect(user.reset_password_token).to be_nil
+    end
   end
   describe 'initial login after setup' do
     it 'allows the initial admin to create a password' do
+      expect(authentication_metrics)
+        .to increment(:user_authenticated_counter)
+
       # This behavior is dependent on there only being one user
       User.delete_all
@@ -50,7 +58,6 @@ describe 'Login' do
       click_button 'Sign in'
       expect(current_path).to eq root_path
-      expect(authentication_metrics).to have_incremented(:user_authenticated_counter)
     end
     it 'does not show flash messages when login page' do
@@ -61,6 +68,8 @@ describe 'Login' do
   describe 'with a blocked account' do
     it 'prevents the user from logging in' do
+      expect(authentication_metrics).to increment(:user_blocked_counter)
+
       user = create(:user, :blocked)
       gitlab_sign_in(user)
@@ -77,6 +86,9 @@ describe 'Login' do
   describe 'with the ghost user' do
     it 'disallows login' do
+      expect(authentication_metrics)
+        .to increment(:user_unauthenticated_counter)
+
       gitlab_sign_in(User.ghost)
       expect(page).to have_content('Invalid Login or password.')
@@ -108,7 +120,13 @@ describe 'Login' do
       context 'using one-time code' do
         it 'allows login with valid code' do
+          expect(authentication_metrics)
+            .to increment(:user_authenticated_counter)
+            .and increment(:user_session_override_counter)
+            .and increment(:user_two_factor_authenticated_counter)
+
           enter_code(user.current_otp)
+
           expect(current_path).to eq root_path
         end

spec/lib/gitlab/auth/activity_spec.rb

 require 'spec_helper'
 describe Gitlab::Auth::Activity do
-  describe 'counters' do
+  describe '#each_counter' do
     it 'has all static counters defined' do
       described_class.each_counter do |counter|
         expect(described_class).to respond_to(counter)

spec/support/helpers/stub_configuration.rb

@@ -68,17 +68,6 @@ module StubConfiguration
     allow(Gitlab.config.repositories).to receive(:storages).and_return(Settingslogic.new(messages))
   end
-  def authentication_metrics
-    Gitlab::Auth::Activity
-  end
-
-  def stub_authentication_activity_metrics
-    authentication_metrics.each_counter do |counter, metric, description|
-      allow(authentication_metrics).to receive(counter)
-        .and_return(spy("#{metric} - #{description}"))
-    end
-  end
-
   private
   # Modifies stubbed messages to also stub possible predicate versions

spec/support/helpers/stub_metrics.rb

+module StubMetrics
+  def authentication_metrics
+    Gitlab::Auth::Activity
+  end
+
+  def stub_authentication_activity_metrics(debug: false)
+    authentication_metrics.each_counter do |name, metric, description|
+      double("#{metric} - #{description}").tap do |counter|
+        allow(counter).to receive(:increment) do
+          puts "Authentication activity metric incremented: #{metric}"
+        end
+
+        allow(authentication_metrics).to receive(name).and_return(counter)
+      end
+    end
+  end
+end

spec/support/prometheus/custom_matchers.rb

-RSpec::Matchers.define :have_incremented do |counter|
+RSpec::Matchers.define :increment do |counter|
   match do |adapter|
-    matcher = RSpec::Mocks::Matchers::HaveReceived.new(:increment)
-
-    matcher.matches?(adapter.send(counter))
+    expect(adapter.send(counter)).to receive(:increment)
   end
 end

spec/support/rspec.rb

 require_relative "helpers/stub_configuration"
+require_relative "helpers/stub_metrics"
 require_relative "helpers/stub_object_storage"
 require_relative "helpers/stub_env"
@@ -7,6 +8,7 @@ RSpec.configure do |config|
   config.raise_errors_for_deprecations!
   config.include StubConfiguration
+  config.include StubMetrics
   config.include StubObjectStorage
   config.include StubENV