diff options
| -rw-r--r-- | config/initializers/warden.rb | 2 | ||||
| -rw-r--r-- | lib/gitlab/auth/activity.rb | 8 | ||||
| -rw-r--r-- | spec/features/users/login_spec.rb | 192 | ||||
| -rw-r--r-- | spec/support/helpers/stub_metrics.rb | 1 | ||||
| -rw-r--r-- | spec/support/prometheus/custom_matchers.rb | 8 |
5 files changed, 180 insertions, 31 deletions
diff --git a/config/initializers/warden.rb b/config/initializers/warden.rb index bd552e2a96b..313604430cd 100644 --- a/config/initializers/warden.rb +++ b/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 diff --git a/lib/gitlab/auth/activity.rb b/lib/gitlab/auth/activity.rb index 334c4794ea7..d364124d74e 100644 --- a/lib/gitlab/auth/activity.rb +++ b/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 diff --git a/spec/features/users/login_spec.rb b/spec/features/users/login_spec.rb index 7f51a2bf52d..e82978102aa 100644 --- a/spec/features/users/login_spec.rb +++ b/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 - end + it 'allows basic login' do + expect(authentication_metrics) + .to increment(:user_authenticated_counter) - 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.') + 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 'blocks invalid login' do - user = create(:user, password: 'not-the-default') + context 'with invalid username and password' do + let(:user) { create(:user, password: 'not-the-default') } - gitlab_sign_in(user) - expect(page).to have_content('Invalid Login or password.') + 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.') + 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 diff --git a/spec/support/helpers/stub_metrics.rb b/spec/support/helpers/stub_metrics.rb index 53abf33f2e5..d882346ec52 100644 --- a/spec/support/helpers/stub_metrics.rb +++ b/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 diff --git a/spec/support/prometheus/custom_matchers.rb b/spec/support/prometheus/custom_matchers.rb index 52ce8d2e48d..22d5cd17e3f 100644 --- a/spec/support/prometheus/custom_matchers.rb +++ b/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 |