From 9f13953879b90611c37547e9c7237286a6f09eaf Mon Sep 17 00:00:00 2001 From: Drew Blessing <drew@blessing.io> Date: Fri, 11 Sep 2020 12:11:49 -0500 Subject: [PATCH] Report auth events in manage stage usage ping Provide aggregate auth event details in usage ping to help identify how often particular authentication methods are being used. This will help inform decision making about improvements and fixes. --- app/models/authentication_event.rb | 9 +++++ .../dblessing-auth-events-usage-ping.yml | 5 +++ ...d_result_index_to_authentication_events.rb | 18 ++++++++++ db/schema_migrations/20200916151442 | 1 + db/structure.sql | 2 ++ lib/gitlab/usage_data.rb | 35 +++++++++++++++++++ spec/factories/authentication_event.rb | 11 ++++++ spec/lib/gitlab/usage_data_spec.rb | 21 ++++++++--- spec/models/authentication_event_spec.rb | 31 ++++++++++++++++ 9 files changed, 128 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/dblessing-auth-events-usage-ping.yml create mode 100644 db/migrate/20200916151442_add_result_index_to_authentication_events.rb create mode 100644 db/schema_migrations/20200916151442 create mode 100644 spec/factories/authentication_event.rb diff --git a/app/models/authentication_event.rb b/app/models/authentication_event.rb index 1ac3c5fbd9ce10d1..16b132c7ed5534ad 100644 --- a/app/models/authentication_event.rb +++ b/app/models/authentication_event.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class AuthenticationEvent < ApplicationRecord + include UsageStatistics + belongs_to :user, optional: true validates :provider, :user_name, :result, presence: true @@ -9,4 +11,11 @@ class AuthenticationEvent < ApplicationRecord failed: 0, success: 1 } + + scope :for_provider, ->(provider) { where(provider: provider) } + scope :ldap, -> { where('provider LIKE ?', 'ldap%')} + + def self.providers + distinct.pluck(:provider) + end end diff --git a/changelogs/unreleased/dblessing-auth-events-usage-ping.yml b/changelogs/unreleased/dblessing-auth-events-usage-ping.yml new file mode 100644 index 0000000000000000..26117006f1c38419 --- /dev/null +++ b/changelogs/unreleased/dblessing-auth-events-usage-ping.yml @@ -0,0 +1,5 @@ +--- +title: Report auth events in manage stage usage ping +merge_request: 39747 +author: +type: added diff --git a/db/migrate/20200916151442_add_result_index_to_authentication_events.rb b/db/migrate/20200916151442_add_result_index_to_authentication_events.rb new file mode 100644 index 0000000000000000..13b0521038ed3d3b --- /dev/null +++ b/db/migrate/20200916151442_add_result_index_to_authentication_events.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +class AddResultIndexToAuthenticationEvents < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + INDEX_NAME = 'index_authentication_events_on_provider_user_id_created_at' + + disable_ddl_transaction! + + def up + add_concurrent_index :authentication_events, [:provider, :user_id, :created_at], where: 'result = 1', name: INDEX_NAME + end + + def down + remove_concurrent_index_by_name :authentication_events, INDEX_NAME + end +end diff --git a/db/schema_migrations/20200916151442 b/db/schema_migrations/20200916151442 new file mode 100644 index 0000000000000000..36b2a4e9962490fa --- /dev/null +++ b/db/schema_migrations/20200916151442 @@ -0,0 +1 @@ +aef52404e6ce83d5d4b3de65ad00b665334f5ff2e5b7b6c3c622f79313657f26 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 753f44a018070557..67738c8623ed0847 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -19478,6 +19478,8 @@ CREATE UNIQUE INDEX index_atlassian_identities_on_extern_uid ON atlassian_identi CREATE INDEX index_authentication_events_on_provider ON authentication_events USING btree (provider); +CREATE INDEX index_authentication_events_on_provider_user_id_created_at ON authentication_events USING btree (provider, user_id, created_at) WHERE (result = 1); + CREATE INDEX index_authentication_events_on_user_id ON authentication_events USING btree (user_id); CREATE INDEX index_award_emoji_on_awardable_type_and_awardable_id ON award_emoji USING btree (awardable_type, awardable_id); diff --git a/lib/gitlab/usage_data.rb b/lib/gitlab/usage_data.rb index 6bb651bbae5cfd09..ea3e2ea064befe9e 100644 --- a/lib/gitlab/usage_data.rb +++ b/lib/gitlab/usage_data.rb @@ -542,6 +542,7 @@ def usage_activity_by_stage_manage(time_period) groups: distinct_count(::GroupMember.where(time_period), :user_id), users_created: count(::User.where(time_period), start: user_minimum_id, finish: user_maximum_id), omniauth_providers: filtered_omniauth_provider_names.reject { |name| name == 'group_saml' }, + user_auth_by_provider: distinct_count_user_auth_by_provider(time_period), projects_imported: { gitlab_project: projects_imported_count('gitlab_project', time_period), gitlab: projects_imported_count('gitlab', time_period), @@ -813,6 +814,7 @@ def clear_memoized clear_memoization(:approval_merge_request_rule_maximum_id) clear_memoization(:project_minimum_id) clear_memoization(:project_maximum_id) + clear_memoization(:auth_providers) end # rubocop: disable CodeReuse/ActiveRecord @@ -844,6 +846,39 @@ def deployment_count(relation) def projects_imported_count(from, time_period) distinct_count(::Project.imported_from(from).where(time_period), :creator_id) # rubocop: disable CodeReuse/ActiveRecord end + + # rubocop:disable CodeReuse/ActiveRecord + def distinct_count_user_auth_by_provider(time_period) + counts = auth_providers_except_ldap.each_with_object({}) do |provider, hash| + hash[provider] = distinct_count( + ::AuthenticationEvent.success.for_provider(provider).where(time_period), :user_id) + end + + if any_ldap_auth_providers? + counts['ldap'] = distinct_count( + ::AuthenticationEvent.success.ldap.where(time_period), :user_id + ) + end + + counts + end + # rubocop:enable CodeReuse/ActiveRecord + + # rubocop:disable UsageData/LargeTable + def auth_providers + strong_memoize(:auth_providers) do + ::AuthenticationEvent.providers + end + end + # rubocop:enable UsageData/LargeTable + + def auth_providers_except_ldap + auth_providers.reject { |provider| provider.starts_with?('ldap') } + end + + def any_ldap_auth_providers? + auth_providers.any? { |provider| provider.starts_with?('ldap') } + end end end end diff --git a/spec/factories/authentication_event.rb b/spec/factories/authentication_event.rb new file mode 100644 index 0000000000000000..ff539c6f5c4aea71 --- /dev/null +++ b/spec/factories/authentication_event.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :authentication_event do + user + provider { :standard } + user_name { 'Jane Doe' } + ip_address { '127.0.0.1' } + result { :failed } + end +end diff --git a/spec/lib/gitlab/usage_data_spec.rb b/spec/lib/gitlab/usage_data_spec.rb index 7c413f3bccab79c9..22c6ef7346b33838 100644 --- a/spec/lib/gitlab/usage_data_spec.rb +++ b/spec/lib/gitlab/usage_data_spec.rb @@ -29,7 +29,8 @@ user_minimum_id user_maximum_id unique_visit_service deployment_minimum_id deployment_maximum_id approval_merge_request_rule_minimum_id - approval_merge_request_rule_maximum_id) + approval_merge_request_rule_maximum_id + auth_providers) values.each do |key| expect(described_class).to receive(:clear_memoization).with(key) end @@ -167,6 +168,8 @@ describe 'usage_activity_by_stage_manage' do it 'includes accurate usage_activity_by_stage data' do + described_class.clear_memoization(:auth_providers) + stub_config( omniauth: { providers: omniauth_providers } @@ -174,21 +177,29 @@ for_defined_days_back do user = create(:user) + user2 = create(:user) create(:event, author: user) create(:group_member, user: user) + create(:authentication_event, user: user, provider: :ldapmain, result: :success) + create(:authentication_event, user: user2, provider: :ldapsecondary, result: :success) + create(:authentication_event, user: user2, provider: :group_saml, result: :success) + create(:authentication_event, user: user2, provider: :group_saml, result: :success) + create(:authentication_event, user: user, provider: :group_saml, result: :failed) end expect(described_class.usage_activity_by_stage_manage({})).to include( events: 2, groups: 2, - users_created: 4, - omniauth_providers: ['google_oauth2'] + users_created: 6, + omniauth_providers: ['google_oauth2'], + user_auth_by_provider: { 'group_saml' => 2, 'ldap' => 4 } ) expect(described_class.usage_activity_by_stage_manage(described_class.last_28_days_time_period)).to include( events: 1, groups: 1, - users_created: 2, - omniauth_providers: ['google_oauth2'] + users_created: 3, + omniauth_providers: ['google_oauth2'], + user_auth_by_provider: { 'group_saml' => 1, 'ldap' => 2 } ) end diff --git a/spec/models/authentication_event_spec.rb b/spec/models/authentication_event_spec.rb index 56b0111f2c7c3d44..7390fde74a66ced4 100644 --- a/spec/models/authentication_event_spec.rb +++ b/spec/models/authentication_event_spec.rb @@ -12,4 +12,35 @@ it { is_expected.to validate_presence_of(:user_name) } it { is_expected.to validate_presence_of(:result) } end + + describe 'scopes' do + let_it_be(:ldap_event) { create(:authentication_event, provider: :ldapmain, result: :failed) } + let_it_be(:google_oauth2) { create(:authentication_event, provider: :google_oauth2, result: :success) } + + describe '.for_provider' do + it 'returns events only for the specified provider' do + expect(described_class.for_provider(:ldapmain)).to match_array ldap_event + end + end + + describe '.ldap' do + it 'returns all events for an LDAP provider' do + expect(described_class.ldap).to match_array ldap_event + end + end + end + + describe '.providers' do + before do + create(:authentication_event, provider: :ldapmain) + create(:authentication_event, provider: :google_oauth2) + create(:authentication_event, provider: :standard) + create(:authentication_event, provider: :standard) + create(:authentication_event, provider: :standard) + end + + it 'returns an array of distinct providers' do + expect(described_class.providers).to match_array %w(ldapmain google_oauth2 standard) + end + end end -- GitLab