summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSean McGivern <sean@mcgivern.me.uk>2018-07-31 10:44:22 +0000
committerSean McGivern <sean@mcgivern.me.uk>2018-07-31 10:44:22 +0000
commite6dd3c527626af1c0f521792360f7c4b29bfee36 (patch)
tree79dee0a3eb392d42478011af8e3eaa8d94c3e370
parenteb8597a1b9eb575121f09b8b9904c0ad7cd489cc (diff)
parent3b81345a730714a94b3e15f0eb91c4f1e8216a44 (diff)
downloadgitlab-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.rb2
-rw-r--r--app/controllers/concerns/authenticates_with_two_factor.rb4
-rw-r--r--app/controllers/sessions_controller.rb12
-rw-r--r--changelogs/unreleased/feature-gb-login-activity-metrics.yml5
-rw-r--r--config/initializers/warden.rb30
-rw-r--r--lib/gitlab/auth/activity.rb77
-rw-r--r--lib/gitlab/auth/blocked_user_tracker.rb59
-rw-r--r--spec/controllers/application_controller_spec.rb44
-rw-r--r--spec/features/users/login_spec.rb241
-rw-r--r--spec/lib/gitlab/auth/activity_spec.rb30
-rw-r--r--spec/lib/gitlab/auth/blocked_user_tracker_spec.rb31
-rw-r--r--spec/support/helpers/stub_metrics.rb27
-rw-r--r--spec/support/matchers/metric_counter_matcher.rb11
-rw-r--r--spec/support/rspec.rb2
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