Make authentication metrics events explicit is specs

config/initializers/warden.rb

@@ -28,6 +28,6 @@ Rails.application.configure do |config|
     user = user_warden || auth.user
 
     ActiveSession.destroy(user, auth.request.session.id)
-    Gitlab::Auth::Activity.new(user, opts).user_signed_out!
+    Gitlab::Auth::Activity.new(user, opts).user_session_destroyed!
   end
 end

lib/gitlab/auth/activity.rb

@@ -12,9 +12,9 @@ module Gitlab
         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_override: 'Counter of manual log-ins and sessions overrides',
+        user_session_destroyed: 'Counter of total user sessions being destroyed',
         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'
+        user_blocked: 'Counter of total sign in attempts when user is blocked'
       }.freeze
 
       def initialize(user, opts)
@@ -50,8 +50,8 @@ module Gitlab
         end
       end
 
-      def user_signed_out!
-        self.class.user_signed_out_counter_increment!
+      def user_session_destroyed!
+        self.class.user_session_destroyed_counter_increment!
       end
 
       def self.each_counter

spec/features/users/login_spec.rb

@@ -68,7 +68,10 @@ 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)
+      expect(authentication_metrics)
+        .to increment(:user_blocked_counter)
+        .and increment(:user_unauthenticated_counter)
+        .and increment(:user_session_destroyed_counter).twice
 
       user = create(:user, :blocked)
 
@@ -78,6 +81,11 @@ describe 'Login' do
     end
 
     it 'does not update Devise trackable attributes', :clean_gitlab_redis_shared_state do
+      expect(authentication_metrics)
+        .to increment(:user_blocked_counter)
+        .and increment(:user_unauthenticated_counter)
+        .and increment(:user_session_destroyed_counter).twice
+
       user = create(:user, :blocked)
 
       expect { gitlab_sign_in(user) }.not_to change { user.reload.sign_in_count }
@@ -88,6 +96,7 @@ describe 'Login' do
     it 'disallows login' do
       expect(authentication_metrics)
         .to increment(:user_unauthenticated_counter)
+        .and increment(:user_password_invalid_counter)
 
       gitlab_sign_in(User.ghost)
 
@@ -95,7 +104,12 @@ describe 'Login' do
     end
 
     it 'does not update Devise trackable attributes', :clean_gitlab_redis_shared_state do
-      expect { gitlab_sign_in(User.ghost) }.not_to change { User.ghost.reload.sign_in_count }
+      expect(authentication_metrics)
+        .to increment(:user_unauthenticated_counter)
+        .and increment(:user_password_invalid_counter)
+
+      expect { gitlab_sign_in(User.ghost) }
+        .not_to change { User.ghost.reload.sign_in_count }
     end
   end
 
@@ -110,11 +124,18 @@ describe 'Login' do
 
       before do
         gitlab_sign_in(user, remember: true)
+
         expect(page).to have_content('Two-Factor Authentication')
       end
 
       it 'does not show a "You are already signed in." error message' 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(page).not_to have_content('You are already signed in.')
       end
 
@@ -137,11 +158,19 @@ describe 'Login' do
         end
 
         it 'blocks login with invalid code' do
+          # TODO invalid 2FA code does not generate any events
+
           enter_code('foo')
+
           expect(page).to have_content('Invalid two-factor code')
         end
 
         it 'allows login with invalid code, then 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('foo')
           expect(page).to have_content('Invalid two-factor code')
 
@@ -162,16 +191,33 @@ describe 'Login' do
 
         context 'with valid code' do
           it 'allows login' do
+            expect(authentication_metrics)
+              .to increment(:user_authenticated_counter)
+              .and increment(:user_session_override_counter)
+              .and increment(:user_two_factor_authenticated_counter)
+
             enter_code(codes.sample)
+
             expect(current_path).to eq root_path
           end
 
           it 'invalidates the used code' do
+            expect(authentication_metrics)
+              .to increment(:user_authenticated_counter)
+              .and increment(:user_session_override_counter)
+              .and increment(:user_two_factor_authenticated_counter)
+
             expect { enter_code(codes.sample) }
               .to change { user.reload.otp_backup_codes.size }.by(-1)
           end
 
           it 'invalidates backup codes twice in a row' do
+            expect(authentication_metrics)
+              .to increment(:user_authenticated_counter).twice
+              .and increment(:user_session_override_counter).twice
+              .and increment(:user_two_factor_authenticated_counter).twice
+              .and increment(:user_session_destroyed_counter)
+
             random_code = codes.delete(codes.sample)
             expect { enter_code(random_code) }
               .to change { user.reload.otp_backup_codes.size }.by(-1)
@@ -186,6 +232,9 @@ describe 'Login' do
 
         context 'with invalid code' do
           it 'blocks login' do
+            # TODO, invalid two factor authentication does not increment
+            # metrics / counters
+
             code = codes.sample
             expect(user.invalidate_otp_backup_code!(code)).to eq true
 
@@ -199,7 +248,7 @@ describe 'Login' do
       end
     end
 
-    context 'logging in via OAuth' do
+    context 'when logging in via OAuth' do
       let(:user) { create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: 'saml')}
       let(:mock_saml_response) do
         File.read('spec/fixtures/authentication/saml_response.xml')
@@ -208,49 +257,79 @@ describe 'Login' do
       before do
         stub_omniauth_saml_config(enabled: true, auto_link_saml_user: true, allow_single_sign_on: ['saml'],
                                   providers: [mock_saml_config_with_upstream_two_factor_authn_contexts])
-        gitlab_sign_in_via('saml', user, 'my-uid', mock_saml_response)
       end
 
       context 'when authn_context is worth two factors' do
         let(:mock_saml_response) do
           File.read('spec/fixtures/authentication/saml_response.xml')
-              .gsub('urn:oasis:names:tc:SAML:2.0:ac:classes:Password', 'urn:oasis:names:tc:SAML:2.0:ac:classes:SecondFactorOTPSMS')
+              .gsub('urn:oasis:names:tc:SAML:2.0:ac:classes:Password',
+                    'urn:oasis:names:tc:SAML:2.0:ac:classes:SecondFactorOTPSMS')
         end
 
         it 'signs user in without prompting for second factor' do
+          # TODO, OAuth authentication does not fire events
+
+          expect(authentication_metrics)
+            .to increment(:user_authenticated_counter)
+            .and increment(:user_session_override_counter)
+
+          sign_in_using_saml!
+
           expect(page).not_to have_content('Two-Factor Authentication')
           expect(current_path).to eq root_path
         end
       end
 
-      context 'when authn_context is not worth two factors' do
+      context 'when two factor authentication is required' do
         it 'shows 2FA prompt after OAuth login' do
+          expect(authentication_metrics)
+            .to increment(:user_authenticated_counter)
+            .and increment(:user_session_override_counter)
+            .and increment(:user_two_factor_authenticated_counter)
+
+          sign_in_using_saml!
+
           expect(page).to have_content('Two-Factor Authentication')
+
           enter_code(user.current_otp)
+
           expect(current_path).to eq root_path
         end
       end
+
+      def sign_in_using_saml!
+        gitlab_sign_in_via('saml', user, 'my-uid', mock_saml_response)
+      end
     end
   end
 
   describe 'without two-factor authentication' do
-    let(:user) { create(:user) }
+    context 'with correct username and password' do
+      let(:user) { create(:user) }
 
-    it 'allows basic login' do
-      gitlab_sign_in(user)
-      expect(current_path).to eq root_path
+      it 'allows basic login' do
+        expect(authentication_metrics)
+          .to increment(:user_authenticated_counter)
+
+        gitlab_sign_in(user)
+
+        expect(current_path).to eq root_path
+        expect(page).not_to have_content('You are already signed in.')
+      end
     end
 
-    it 'does not show a "You are already signed in." error message' do
-      gitlab_sign_in(user)
-      expect(page).not_to have_content('You are already signed in.')
-    end
+    context 'with invalid username and password' do
+      let(:user) { create(:user, password: 'not-the-default') }
 
-    it 'blocks invalid login' do
-      user = create(:user, password: 'not-the-default')
+      it 'blocks invalid login' do
+        expect(authentication_metrics)
+          .to increment(:user_unauthenticated_counter)
+          .and increment(:user_password_invalid_counter)
 
-      gitlab_sign_in(user)
-      expect(page).to have_content('Invalid Login or password.')
+        gitlab_sign_in(user)
+
+        expect(page).to have_content('Invalid Login or password.')
+      end
     end
   end
 
@@ -266,18 +345,26 @@ describe 'Login' do
       context 'with grace period defined' do
         before do
           stub_application_setting(two_factor_grace_period: 48)
-          gitlab_sign_in(user)
         end
 
         context 'within the grace period' do
           it 'redirects to two-factor configuration page' do
+            expect(authentication_metrics)
+              .to increment(:user_authenticated_counter)
+
+            gitlab_sign_in(user)
+
             expect(current_path).to eq profile_two_factor_auth_path
             expect(page).to have_content('The global settings require you to enable Two-Factor Authentication for your account. You need to do this before ')
           end
 
           it 'allows skipping two-factor configuration', :js do
-            expect(current_path).to eq profile_two_factor_auth_path
+            expect(authentication_metrics)
+              .to increment(:user_authenticated_counter)
 
+            gitlab_sign_in(user)
+
+            expect(current_path).to eq profile_two_factor_auth_path
             click_link 'Configure it later'
             expect(current_path).to eq root_path
           end
@@ -287,6 +374,11 @@ describe 'Login' do
           let(:user) { create(:user, otp_grace_period_started_at: 9999.hours.ago) }
 
           it 'redirects to two-factor configuration page' do
+            expect(authentication_metrics)
+              .to increment(:user_authenticated_counter)
+
+            gitlab_sign_in(user)
+
             expect(current_path).to eq profile_two_factor_auth_path
             expect(page).to have_content(
               'The global settings require you to enable Two-Factor Authentication for your account.'
@@ -294,6 +386,11 @@ describe 'Login' do
           end
 
           it 'disallows skipping two-factor configuration', :js do
+            expect(authentication_metrics)
+              .to increment(:user_authenticated_counter)
+
+            gitlab_sign_in(user)
+
             expect(current_path).to eq profile_two_factor_auth_path
             expect(page).not_to have_link('Configure it later')
           end
@@ -303,10 +400,14 @@ describe 'Login' do
       context 'without grace period defined' do
         before do
           stub_application_setting(two_factor_grace_period: 0)
-          gitlab_sign_in(user)
         end
 
         it 'redirects to two-factor configuration page' do
+          expect(authentication_metrics)
+            .to increment(:user_authenticated_counter)
+
+          gitlab_sign_in(user)
+
           expect(current_path).to eq profile_two_factor_auth_path
           expect(page).to have_content(
             'The global settings require you to enable Two-Factor Authentication for your account.'
@@ -326,11 +427,15 @@ describe 'Login' do
       context 'with grace period defined' do
         before do
           stub_application_setting(two_factor_grace_period: 48)
-          gitlab_sign_in(user)
         end
 
         context 'within the grace period' do
           it 'redirects to two-factor configuration page' do
+            expect(authentication_metrics)
+              .to increment(:user_authenticated_counter)
+
+            gitlab_sign_in(user)
+
             expect(current_path).to eq profile_two_factor_auth_path
             expect(page).to have_content(
               'The group settings for Group 1 and Group 2 require you to enable ' \
@@ -339,8 +444,12 @@ describe 'Login' do
           end
 
           it 'allows skipping two-factor configuration', :js do
-            expect(current_path).to eq profile_two_factor_auth_path
+            expect(authentication_metrics)
+              .to increment(:user_authenticated_counter)
 
+            gitlab_sign_in(user)
+
+            expect(current_path).to eq profile_two_factor_auth_path
             click_link 'Configure it later'
             expect(current_path).to eq root_path
           end
@@ -350,6 +459,11 @@ describe 'Login' do
           let(:user) { create(:user, otp_grace_period_started_at: 9999.hours.ago) }
 
           it 'redirects to two-factor configuration page' do
+            expect(authentication_metrics)
+              .to increment(:user_authenticated_counter)
+
+            gitlab_sign_in(user)
+
             expect(current_path).to eq profile_two_factor_auth_path
             expect(page).to have_content(
               'The group settings for Group 1 and Group 2 require you to enable ' \
@@ -358,6 +472,11 @@ describe 'Login' do
           end
 
           it 'disallows skipping two-factor configuration', :js do
+            expect(authentication_metrics)
+              .to increment(:user_authenticated_counter)
+
+            gitlab_sign_in(user)
+
             expect(current_path).to eq profile_two_factor_auth_path
             expect(page).not_to have_link('Configure it later')
           end
@@ -367,10 +486,14 @@ describe 'Login' do
       context 'without grace period defined' do
         before do
           stub_application_setting(two_factor_grace_period: 0)
-          gitlab_sign_in(user)
         end
 
         it 'redirects to two-factor configuration page' do
+          expect(authentication_metrics)
+            .to increment(:user_authenticated_counter)
+
+          gitlab_sign_in(user)
+
           expect(current_path).to eq profile_two_factor_auth_path
           expect(page).to have_content(
             'The group settings for Group 1 and Group 2 require you to enable ' \
@@ -454,6 +577,9 @@ describe 'Login' do
     end
 
     it 'asks to accept the terms on first login' do
+      expect(authentication_metrics)
+        .to increment(:user_authenticated_counter)
+
       visit new_user_session_path
 
       fill_in 'user_login', with: user.email
@@ -470,6 +596,9 @@ describe 'Login' do
     end
 
     it 'does not ask for terms when the user already accepted them' do
+      expect(authentication_metrics)
+        .to increment(:user_authenticated_counter)
+
       accept_terms(user)
 
       visit new_user_session_path
@@ -490,6 +619,9 @@ describe 'Login' do
 
       context 'when the user did not enable 2FA' do
         it 'asks to set 2FA before asking to accept the terms' do
+          expect(authentication_metrics)
+            .to increment(:user_authenticated_counter)
+
           visit new_user_session_path
 
           fill_in 'user_login', with: user.email
@@ -518,6 +650,11 @@ describe 'Login' do
         end
 
         it 'asks the user to accept the terms' do
+          expect(authentication_metrics)
+            .to increment(:user_authenticated_counter)
+            .and increment(:user_session_override_counter)
+            .and increment(:user_two_factor_authenticated_counter)
+
           visit new_user_session_path
 
           fill_in 'user_login', with: user.email
@@ -541,6 +678,9 @@ describe 'Login' do
       end
 
       it 'asks the user to accept the terms before setting a new password' do
+        expect(authentication_metrics)
+          .to increment(:user_authenticated_counter)
+
         visit new_user_session_path
 
         fill_in 'user_login', with: user.email
@@ -569,6 +709,10 @@ describe 'Login' do
       end
 
       it 'asks the user to accept the terms before setting an email' do
+        expect(authentication_metrics)
+          .to increment(:user_authenticated_counter)
+          .and increment(:user_session_override_counter)
+
         gitlab_sign_in_via('saml', user, 'my-uid')
 
         expect_to_be_on_terms_page

spec/support/helpers/stub_metrics.rb

@@ -7,7 +7,6 @@ module StubMetrics
     authentication_metrics.each_counter do |name, metric, description|
       double("#{metric} - #{description}").tap do |counter|
         allow(authentication_metrics).to receive(name).and_return(counter)
-        allow(counter).to receive(:increment) # TODO, require expectations
       end
     end
 

spec/support/prometheus/custom_matchers.rb

@@ -1,5 +1,11 @@
 RSpec::Matchers.define :increment do |counter|
   match do |adapter|
-    expect(adapter.send(counter)).to receive(:increment)
+    expect(adapter.send(counter))
+      .to receive(:increment)
+      .exactly(@exactly || :once)
+  end
+
+  chain :twice do
+    @exactly = :twice
   end
 end