diff options
author | Sean McGivern <sean@mcgivern.me.uk> | 2018-07-31 10:44:22 +0000 |
---|---|---|
committer | Sean McGivern <sean@mcgivern.me.uk> | 2018-07-31 10:44:22 +0000 |
commit | e6dd3c527626af1c0f521792360f7c4b29bfee36 (patch) | |
tree | 79dee0a3eb392d42478011af8e3eaa8d94c3e370 | |
parent | eb8597a1b9eb575121f09b8b9904c0ad7cd489cc (diff) | |
parent | 3b81345a730714a94b3e15f0eb91c4f1e8216a44 (diff) | |
download | gitlab-ce-e6dd3c527626af1c0f521792360f7c4b29bfee36.tar.gz |
Merge branch 'feature/gb/login-activity-metrics' into 'master'
Add user authentication activity metrics
Closes #47789
See merge request gitlab-org/gitlab-ce!20668
-rw-r--r-- | app/controllers/application_controller.rb | 2 | ||||
-rw-r--r-- | app/controllers/concerns/authenticates_with_two_factor.rb | 4 | ||||
-rw-r--r-- | app/controllers/sessions_controller.rb | 12 | ||||
-rw-r--r-- | changelogs/unreleased/feature-gb-login-activity-metrics.yml | 5 | ||||
-rw-r--r-- | config/initializers/warden.rb | 30 | ||||
-rw-r--r-- | lib/gitlab/auth/activity.rb | 77 | ||||
-rw-r--r-- | lib/gitlab/auth/blocked_user_tracker.rb | 59 | ||||
-rw-r--r-- | spec/controllers/application_controller_spec.rb | 44 | ||||
-rw-r--r-- | spec/features/users/login_spec.rb | 241 | ||||
-rw-r--r-- | spec/lib/gitlab/auth/activity_spec.rb | 30 | ||||
-rw-r--r-- | spec/lib/gitlab/auth/blocked_user_tracker_spec.rb | 31 | ||||
-rw-r--r-- | spec/support/helpers/stub_metrics.rb | 27 | ||||
-rw-r--r-- | spec/support/matchers/metric_counter_matcher.rb | 11 | ||||
-rw-r--r-- | spec/support/rspec.rb | 2 |
14 files changed, 499 insertions, 76 deletions
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index eeceb99c8d2..73d7b8cb9cf 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -397,7 +397,7 @@ class ApplicationController < ActionController::Base # actually stored in the session and a token is needed # for every request. If you want the token to work as a # sign in token, you can simply remove store: false. - sign_in user, store: false + sign_in(user, store: false, message: :sessionless_sign_in) end end diff --git a/app/controllers/concerns/authenticates_with_two_factor.rb b/app/controllers/concerns/authenticates_with_two_factor.rb index 69a053d4246..dfa1da7872c 100644 --- a/app/controllers/concerns/authenticates_with_two_factor.rb +++ b/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") diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 4ca42e2d4a2..ab8e2e35b98 100644 --- a/app/controllers/sessions_controller.rb +++ b/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 diff --git a/changelogs/unreleased/feature-gb-login-activity-metrics.yml b/changelogs/unreleased/feature-gb-login-activity-metrics.yml new file mode 100644 index 00000000000..5d687b984eb --- /dev/null +++ b/changelogs/unreleased/feature-gb-login-activity-metrics.yml @@ -0,0 +1,5 @@ +--- +title: Add more comprehensive metrics tracking authentication activity +merge_request: 20668 +author: +type: added diff --git a/config/initializers/warden.rb b/config/initializers/warden.rb index 8cc36820d3c..d64b659c6d7 100644 --- a/config/initializers/warden.rb +++ b/config/initializers/warden.rb @@ -1,10 +1,20 @@ Rails.application.configure do |config| Warden::Manager.after_set_user(scope: :user) do |user, auth, opts| Gitlab::Auth::UniqueIpsLimiter.limit_user!(user) - end - Warden::Manager.before_failure(scope: :user) do |env, opts| - Gitlab::Auth::BlockedUserTracker.log_if_user_blocked(env) + activity = Gitlab::Auth::Activity.new(user, opts) + + case opts[:event] + when :authentication + activity.user_authenticated! + when :set_user + activity.user_authenticated! + activity.user_session_override! + when :fetch # rubocop:disable Lint/EmptyWhen + # We ignore session fetch events + else + activity.user_session_override! + end end Warden::Manager.after_authentication(scope: :user) do |user, auth, opts| @@ -15,7 +25,17 @@ Rails.application.configure do |config| ActiveSession.set(user, auth.request) end - Warden::Manager.before_logout(scope: :user) do |user, auth, opts| - ActiveSession.destroy(user || auth.user, auth.request.session.id) + Warden::Manager.before_failure(scope: :user) do |env, opts| + tracker = Gitlab::Auth::BlockedUserTracker.new(env) + tracker.log_blocked_user_activity! if tracker.user_blocked? + + Gitlab::Auth::Activity.new(tracker.user, opts).user_authentication_failed! + 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_session_destroyed! end end diff --git a/lib/gitlab/auth/activity.rb b/lib/gitlab/auth/activity.rb new file mode 100644 index 00000000000..9f84c578d4f --- /dev/null +++ b/lib/gitlab/auth/activity.rb @@ -0,0 +1,77 @@ +module Gitlab + module Auth + ## + # Metrics and logging for user authentication activity. + # + class Activity + extend Gitlab::Utils::StrongMemoize + + COUNTERS = { + user_authenticated: 'Counter of successful authentication events', + user_unauthenticated: 'Counter of authentication failures', + user_not_found: 'Counter of 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 user sessions being destroyed', + user_two_factor_authenticated: 'Counter of two factor authentications', + user_sessionless_authentication: 'Counter of sessionless authentications', + user_blocked: 'Counter of sign in attempts when user is blocked' + }.freeze + + def initialize(user, opts) + @user = user + @opts = opts + end + + def user_authentication_failed! + self.class.user_unauthenticated_counter_increment! + + case @opts[:message] + when :not_found_in_database + self.class.user_not_found_counter_increment! + when :invalid + self.class.user_password_invalid_counter_increment! + end + + self.class.user_blocked_counter_increment! if @user&.blocked? + end + + def user_authenticated! + self.class.user_authenticated_counter_increment! + end + + def user_session_override! + self.class.user_session_override_counter_increment! + + case @opts[:message] + when :two_factor_authenticated + self.class.user_two_factor_authenticated_counter_increment! + when :sessionless_sign_in + self.class.user_sessionless_authentication_counter_increment! + end + end + + def user_session_destroyed! + self.class.user_session_destroyed_counter_increment! + end + + def self.each_counter + COUNTERS.each_pair do |metric, description| + yield "#{metric}_counter", metric, description + end + end + + each_counter do |counter, metric, description| + define_singleton_method(counter) do + strong_memoize(counter) do + Gitlab::Metrics.counter("gitlab_auth_#{metric}_total".to_sym, description) + end + end + + define_singleton_method("#{counter}_increment!") do + public_send(counter).increment # rubocop:disable GitlabSecurity/PublicSend + end + end + end + end +end diff --git a/lib/gitlab/auth/blocked_user_tracker.rb b/lib/gitlab/auth/blocked_user_tracker.rb index 7609a7b04f6..b6d2adc834b 100644 --- a/lib/gitlab/auth/blocked_user_tracker.rb +++ b/lib/gitlab/auth/blocked_user_tracker.rb @@ -2,37 +2,58 @@ module Gitlab module Auth class BlockedUserTracker + include Gitlab::Utils::StrongMemoize + ACTIVE_RECORD_REQUEST_PARAMS = 'action_dispatch.request.request_parameters' - def self.log_if_user_blocked(env) - message = env.dig('warden.options', :message) + def initialize(env) + @env = env + end - # 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. - return unless message == User::BLOCKED_MESSAGE + def user_blocked? + user&.blocked? + end - # 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') + def user + return unless has_user_blocked_message? - return unless login.present? + 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') - user = User.by_login(login) + User.by_login(login) if login.present? + end + rescue TypeError + end - return unless user&.blocked? + def log_blocked_user_activity! + return unless user_blocked? - Gitlab::AppLogger.info("Failed login for blocked user: user=#{user.username} ip=#{env['REMOTE_ADDR']}") + 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 diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index f1165c73847..bad7a28556c 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -57,6 +57,10 @@ describe ApplicationController do end describe "#authenticate_user_from_personal_access_token!" do + before do + stub_authentication_activity_metrics(debug: false) + end + controller(described_class) do def index render text: 'authenticated' @@ -67,7 +71,13 @@ describe ApplicationController do context "when the 'personal_access_token' param is populated with the personal access token" do it "logs the user in" do + expect(authentication_metrics) + .to increment(:user_authenticated_counter) + .and increment(:user_session_override_counter) + .and increment(:user_sessionless_authentication_counter) + get :index, private_token: personal_access_token.token + expect(response).to have_gitlab_http_status(200) expect(response.body).to eq('authenticated') end @@ -75,15 +85,25 @@ describe ApplicationController do context "when the 'PERSONAL_ACCESS_TOKEN' header is populated with the personal access token" do it "logs the user in" do + expect(authentication_metrics) + .to increment(:user_authenticated_counter) + .and increment(:user_session_override_counter) + .and increment(:user_sessionless_authentication_counter) + @request.headers["PRIVATE-TOKEN"] = personal_access_token.token get :index + expect(response).to have_gitlab_http_status(200) expect(response.body).to eq('authenticated') end end it "doesn't log the user in otherwise" do + expect(authentication_metrics) + .to increment(:user_unauthenticated_counter) + get :index, private_token: "token" + expect(response.status).not_to eq(200) expect(response.body).not_to eq('authenticated') end @@ -174,6 +194,10 @@ describe ApplicationController do end describe '#authenticate_sessionless_user!' do + before do + stub_authentication_activity_metrics(debug: false) + end + describe 'authenticating a user from a feed token' do controller(described_class) do def index @@ -184,7 +208,13 @@ describe ApplicationController do context "when the 'feed_token' param is populated with the feed token" do context 'when the request format is atom' do it "logs the user in" do + expect(authentication_metrics) + .to increment(:user_authenticated_counter) + .and increment(:user_session_override_counter) + .and increment(:user_sessionless_authentication_counter) + get :index, feed_token: user.feed_token, format: :atom + expect(response).to have_gitlab_http_status 200 expect(response.body).to eq 'authenticated' end @@ -192,7 +222,13 @@ describe ApplicationController do context 'when the request format is ics' do it "logs the user in" do + expect(authentication_metrics) + .to increment(:user_authenticated_counter) + .and increment(:user_session_override_counter) + .and increment(:user_sessionless_authentication_counter) + get :index, feed_token: user.feed_token, format: :ics + expect(response).to have_gitlab_http_status 200 expect(response.body).to eq 'authenticated' end @@ -200,7 +236,11 @@ describe ApplicationController do context 'when the request format is neither atom nor ics' do it "doesn't log the user in" do + expect(authentication_metrics) + .to increment(:user_unauthenticated_counter) + get :index, feed_token: user.feed_token + expect(response.status).not_to have_gitlab_http_status 200 expect(response.body).not_to eq 'authenticated' end @@ -209,7 +249,11 @@ describe ApplicationController do context "when the 'feed_token' param is populated with an invalid feed token" do it "doesn't log the user" do + expect(authentication_metrics) + .to increment(:user_unauthenticated_counter) + get :index, feed_token: 'token', format: :atom + expect(response.status).not_to eq 200 expect(response.body).not_to eq 'authenticated' end diff --git a/spec/features/users/login_spec.rb b/spec/features/users/login_spec.rb index 21891b9ccda..44758f862a8 100644 --- a/spec/features/users/login_spec.rb +++ b/spec/features/users/login_spec.rb @@ -3,28 +3,40 @@ require 'spec_helper' describe 'Login' do include TermsHelper - it 'Successful user signin invalidates password reset token' do - user = create(:user) + before do + stub_authentication_activity_metrics(debug: true) + end + + 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 + expect(user.reset_password_token).to be_nil - visit new_user_password_path - fill_in 'user_email', with: user.email - click_button 'Reset password' + 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 + 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 + 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 + 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 @@ -56,6 +68,11 @@ 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) + .and increment(:user_unauthenticated_counter) + .and increment(:user_session_destroyed_counter).twice + user = create(:user, :blocked) gitlab_sign_in(user) @@ -64,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 } @@ -72,13 +94,22 @@ describe 'Login' do describe 'with the ghost user' do it 'disallows login' do + expect(authentication_metrics) + .to increment(:user_unauthenticated_counter) + .and increment(:user_password_invalid_counter) + gitlab_sign_in(User.ghost) expect(page).to have_content('Invalid Login or password.') 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 @@ -93,17 +124,30 @@ 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 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 @@ -114,11 +158,20 @@ describe 'Login' do end it 'blocks login with invalid code' do + # TODO invalid 2FA code does not generate any events + # See gitlab-org/gitlab-ce#49785 + 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') @@ -139,16 +192,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) @@ -163,6 +233,9 @@ describe 'Login' do context 'with invalid code' do it 'blocks login' do + # TODO, invalid two factor authentication does not increment + # metrics / counters, see gitlab-org/gitlab-ce#49785 + code = codes.sample expect(user.invalidate_otp_backup_code!(code)).to eq true @@ -176,7 +249,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') @@ -185,49 +258,80 @@ 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, + # see gitlab-org/gitlab-ce#49786 + + 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 @@ -243,18 +347,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 @@ -264,6 +376,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.' @@ -271,6 +388,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 @@ -280,10 +402,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.' @@ -303,11 +429,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 ' \ @@ -316,8 +446,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 @@ -327,6 +461,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 ' \ @@ -335,6 +474,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 @@ -344,10 +488,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 ' \ @@ -431,6 +579,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 @@ -447,6 +598,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 @@ -467,6 +621,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 @@ -495,6 +652,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 @@ -518,6 +680,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 @@ -546,6 +711,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/lib/gitlab/auth/activity_spec.rb b/spec/lib/gitlab/auth/activity_spec.rb new file mode 100644 index 00000000000..07854cb1eba --- /dev/null +++ b/spec/lib/gitlab/auth/activity_spec.rb @@ -0,0 +1,30 @@ +require 'fast_spec_helper' + +describe Gitlab::Auth::Activity 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) + end + end + + it 'has all static incrementers defined' do + described_class.each_counter do |counter| + expect(described_class).to respond_to("#{counter}_increment!") + end + end + + it 'has all counters starting with `user_`' do + described_class.each_counter do |counter| + expect(counter).to start_with('user_') + end + end + + it 'yields counter method, name and description' do + described_class.each_counter do |method, name, description| + expect(method).to eq "#{name}_counter" + expect(description).to start_with('Counter of') + end + end + end +end diff --git a/spec/lib/gitlab/auth/blocked_user_tracker_spec.rb b/spec/lib/gitlab/auth/blocked_user_tracker_spec.rb index 43b68e69131..13c09b9cb9b 100644 --- a/spec/lib/gitlab/auth/blocked_user_tracker_spec.rb +++ b/spec/lib/gitlab/auth/blocked_user_tracker_spec.rb @@ -3,24 +3,30 @@ require 'spec_helper' describe Gitlab::Auth::BlockedUserTracker do set(:user) { create(:user) } - describe '.log_if_user_blocked' do + describe '#log_blocked_user_activity!' do it 'does not log if user failed to login due to undefined reason' do expect_any_instance_of(SystemHooksService).not_to receive(:execute_hooks_for) - expect(described_class.log_if_user_blocked({})).to be_nil + tracker = described_class.new({}) + + expect(tracker.user).to be_nil + expect(tracker.user_blocked?).to be_falsey + expect(tracker.log_blocked_user_activity!).to be_nil end it 'gracefully handles malformed environment variables' do - env = { 'warden.options' => 'test' } + tracker = described_class.new({ 'warden.options' => 'test' }) - expect(described_class.log_if_user_blocked(env)).to be_nil + expect(tracker.user).to be_nil + expect(tracker.user_blocked?).to be_falsey + expect(tracker.log_blocked_user_activity!).to be_nil end context 'failed login due to blocked user' do let(:base_env) { { 'warden.options' => { message: User::BLOCKED_MESSAGE } } } let(:env) { base_env.merge(request_env) } - subject { described_class.log_if_user_blocked(env) } + subject { described_class.new(env) } before do expect_any_instance_of(SystemHooksService).to receive(:execute_hooks_for).with(user, :failed_login) @@ -32,14 +38,17 @@ describe Gitlab::Auth::BlockedUserTracker do it 'logs a blocked user' do user.block! - expect(subject).to be_truthy + expect(subject.user).to be_blocked + expect(subject.user_blocked?).to be true + expect(subject.log_blocked_user_activity!).to be_truthy end it 'logs a blocked user by e-mail' do user.block! env[described_class::ACTIVE_RECORD_REQUEST_PARAMS]['user']['login'] = user.email - expect(subject).to be_truthy + expect(subject.user).to be_blocked + expect(subject.log_blocked_user_activity!).to be_truthy end end @@ -49,13 +58,17 @@ describe Gitlab::Auth::BlockedUserTracker do it 'logs a blocked user' do user.block! - expect(subject).to be_truthy + expect(subject.user).to be_blocked + expect(subject.user_blocked?).to be true + expect(subject.log_blocked_user_activity!).to be_truthy end it 'logs a LDAP blocked user' do user.ldap_block! - expect(subject).to be_truthy + expect(subject.user).to be_blocked + expect(subject.user_blocked?).to be true + expect(subject.log_blocked_user_activity!).to be_truthy end end end diff --git a/spec/support/helpers/stub_metrics.rb b/spec/support/helpers/stub_metrics.rb new file mode 100644 index 00000000000..64983fdf222 --- /dev/null +++ b/spec/support/helpers/stub_metrics.rb @@ -0,0 +1,27 @@ +module StubMetrics + def authentication_metrics + Gitlab::Auth::Activity + end + + def stub_authentication_activity_metrics(debug: false) + authentication_metrics.each_counter do |name, metric, description| + allow(authentication_metrics).to receive(name) + .and_return(double("#{metric} - #{description}")) + end + + debug_authentication_activity_metrics if debug + end + + def debug_authentication_activity_metrics + authentication_metrics.tap do |metrics| + metrics.each_counter do |name, metric| + "#{name}_increment!".tap do |incrementer| + allow(metrics).to receive(incrementer).and_wrap_original do |method| + puts "Authentication activity metric incremented: #{name}" + method.call + end + end + end + end + end +end diff --git a/spec/support/matchers/metric_counter_matcher.rb b/spec/support/matchers/metric_counter_matcher.rb new file mode 100644 index 00000000000..22d5cd17e3f --- /dev/null +++ b/spec/support/matchers/metric_counter_matcher.rb @@ -0,0 +1,11 @@ +RSpec::Matchers.define :increment do |counter| + match do |adapter| + expect(adapter.send(counter)) + .to receive(:increment) + .exactly(@exactly || :once) + end + + chain :twice do + @exactly = :twice + end +end diff --git a/spec/support/rspec.rb b/spec/support/rspec.rb index 54b8df7aa19..9b8bcebcb3a 100644 --- a/spec/support/rspec.rb +++ b/spec/support/rspec.rb @@ -1,4 +1,5 @@ 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 |