From 3c00d5ff2465f777a3b0f7e91bb452d8a3609b20 Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Wed, 13 Aug 2025 15:36:06 +0300 Subject: [PATCH] Optimize retrieving PATs for Credentials inventory index page and API Related to https://gitlab.com/gitlab-org/gitlab/-/issues/558112 --- app/finders/personal_access_tokens_finder.rb | 21 +- app/models/personal_access_token.rb | 15 +- .../optimize_credentials_inventory.yml | 10 + ...d_and_user_type_and_last_used_at_and_id.rb | 24 ++ ..._id_and_user_type_and_expires_at_and_id.rb | 24 ++ ..._id_and_user_type_and_created_at_and_id.rb | 24 ++ db/schema_migrations/20250903123949 | 1 + db/schema_migrations/20250903134752 | 1 + db/schema_migrations/20250903135508 | 1 + db/structure.sql | 6 + .../admin/credentials_controller.rb | 4 + .../concerns/credentials_inventory_actions.rb | 24 +- .../ee/personal_access_tokens_finder.rb | 10 +- ee/lib/api/manage/groups.rb | 35 +- ee/spec/requests/api/manage/groups_spec.rb | 237 +++++++++-- .../security/credentials_controller_spec.rb | 391 ++++++++++++------ .../credentials_inventory_shared_examples.rb | 3 + .../credentials_inventory_shared_examples.rb | 2 +- spec/db/schema_spec.rb | 1 + spec/factories/personal_access_tokens.rb | 2 - .../personal_access_tokens_finder_spec.rb | 70 +++- spec/models/personal_access_token_spec.rb | 86 +++- .../settings/access_tokens_controller_spec.rb | 2 +- .../settings/access_tokens_controller_spec.rb | 2 +- ...ccess_tokens_controller_shared_examples.rb | 4 +- 25 files changed, 787 insertions(+), 213 deletions(-) create mode 100644 config/feature_flags/gitlab_com_derisk/optimize_credentials_inventory.yml create mode 100644 db/post_migrate/20250903123949_index_personal_access_tokens_on_group_id_and_user_type_and_last_used_at_and_id.rb create mode 100644 db/post_migrate/20250903134752_index_personal_access_tokens_on_group_id_and_user_type_and_expires_at_and_id.rb create mode 100644 db/post_migrate/20250903135508_index_personal_access_tokens_on_group_id_and_user_type_and_created_at_and_id.rb create mode 100644 db/schema_migrations/20250903123949 create mode 100644 db/schema_migrations/20250903134752 create mode 100644 db/schema_migrations/20250903135508 diff --git a/app/finders/personal_access_tokens_finder.rb b/app/finders/personal_access_tokens_finder.rb index 941da1d9b48d71..52fb5203942bbd 100644 --- a/app/finders/personal_access_tokens_finder.rb +++ b/app/finders/personal_access_tokens_finder.rb @@ -15,6 +15,7 @@ def execute tokens = by_current_user(tokens) tokens = by_user(tokens) tokens = by_users(tokens) + tokens = by_user_types(tokens) tokens = by_impersonation(tokens) tokens = by_state(tokens) tokens = by_owner_type(tokens) @@ -27,6 +28,7 @@ def execute tokens = by_last_used_after(tokens) tokens = by_search(tokens) tokens = by_organization(tokens) + tokens = by_group(tokens) tokens = tokens.allow_cross_joins_across_databases(url: "https://gitlab.com/gitlab-org/gitlab/-/issues/436657") sort(tokens) @@ -49,7 +51,11 @@ def by_current_user(tokens) def by_owner_type(tokens) case @params[:owner_type] when 'human' - tokens.owner_is_human + if Feature.enabled?(:optimize_credentials_inventory, params[:group] || :instance) + tokens.for_user_types(:human) + else + tokens.owner_is_human + end else tokens end @@ -67,6 +73,12 @@ def by_users(tokens) tokens.for_users(@params[:users]) end + def by_user_types(tokens) + return tokens unless @params[:user_types] + + tokens.for_user_types(@params[:user_types]) + end + def sort(tokens) available_sort_orders = PersonalAccessToken.simple_sorts.keys @@ -150,6 +162,13 @@ def by_organization(tokens) tokens.for_organization(params[:organization]) end + + def by_group(tokens) + return tokens unless params[:group] + return tokens unless Feature.enabled?(:optimize_credentials_inventory, params[:group]) + + tokens.for_group(params[:group]) + end end PersonalAccessTokensFinder.prepend_mod_with('PersonalAccessTokensFinder') diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb index f965ad7b3b66a1..f53e51839fd879 100644 --- a/app/models/personal_access_token.rb +++ b/app/models/personal_access_token.rb @@ -70,11 +70,15 @@ class PersonalAccessToken < ApplicationRecord scope :not_revoked, -> { where(revoked: [false, nil]) } scope :for_user, ->(user) { where(user: user) } scope :for_users, ->(users) { where(user: users) } + scope :for_user_types, ->(user_types) { where(user_type: user_types) } scope :for_organization, ->(organization) { where(organization_id: organization) } + scope :for_group, ->(group) { where(group: group) } scope :preload_users, -> { preload(:user) } - scope :order_expires_at_asc_id_desc, -> { reorder(expires_at: :asc, id: :desc) } + scope :order_created_at_asc_id_asc, -> { reorder(created_at: :asc, id: :asc) } + scope :order_created_at_desc_id_desc, -> { reorder(created_at: :desc, id: :desc) } + scope :order_expires_at_asc_id_asc, -> { reorder(expires_at: :asc, id: :asc) } scope :order_expires_at_desc_id_desc, -> { reorder(expires_at: :desc, id: :desc) } - scope :order_last_used_at_asc_id_desc, -> { reorder(last_used_at: :asc, id: :desc) } + scope :order_last_used_at_asc_id_asc, -> { reorder(last_used_at: :asc, id: :asc) } scope :order_last_used_at_desc_id_desc, -> { reorder(last_used_at: :desc, id: :desc) } scope :project_access_token, -> { includes(:user).references(:user).merge(User.project_bot) } scope :owner_is_human, -> { includes(:user).references(:user).merge(User.human) } @@ -109,10 +113,11 @@ def active? def self.simple_sorts super.merge( { - 'expires_asc' => -> { order_expires_at_asc_id_desc }, - 'expires_at_asc_id_desc' => -> { order_expires_at_asc_id_desc }, # Keep for backward compatibility + 'created_asc' => -> { order_created_at_asc_id_asc }, + 'created_desc' => -> { order_created_at_desc_id_desc }, + 'expires_asc' => -> { order_expires_at_asc_id_asc }, 'expires_desc' => -> { order_expires_at_desc_id_desc }, - 'last_used_asc' => -> { order_last_used_at_asc_id_desc }, + 'last_used_asc' => -> { order_last_used_at_asc_id_asc }, 'last_used_desc' => -> { order_last_used_at_desc_id_desc } } ) diff --git a/config/feature_flags/gitlab_com_derisk/optimize_credentials_inventory.yml b/config/feature_flags/gitlab_com_derisk/optimize_credentials_inventory.yml new file mode 100644 index 00000000000000..bbe92a9d3982d5 --- /dev/null +++ b/config/feature_flags/gitlab_com_derisk/optimize_credentials_inventory.yml @@ -0,0 +1,10 @@ +--- +name: optimize_credentials_inventory +description: Optimize retrieving personal and resource access tokens for Credentials inventory index page and API endpoints +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/558112 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/201312 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/561260 +milestone: '18.5' +group: group::authentication +type: gitlab_com_derisk +default_enabled: false diff --git a/db/post_migrate/20250903123949_index_personal_access_tokens_on_group_id_and_user_type_and_last_used_at_and_id.rb b/db/post_migrate/20250903123949_index_personal_access_tokens_on_group_id_and_user_type_and_last_used_at_and_id.rb new file mode 100644 index 00000000000000..f75a2ef87a9993 --- /dev/null +++ b/db/post_migrate/20250903123949_index_personal_access_tokens_on_group_id_and_user_type_and_last_used_at_and_id.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +class IndexPersonalAccessTokensOnGroupIdAndUserTypeAndLastUsedAtAndId < Gitlab::Database::Migration[2.3] + disable_ddl_transaction! + + milestone '18.5' + + INDEX_NAME = 'index_pats_on_group_id_and_user_type_and_last_used_at_and_id' + + def up + # rubocop:disable Migration/PreventIndexCreation -- index is needed for Credentials inventory + add_concurrent_index( + :personal_access_tokens, + [:group_id, :user_type, :last_used_at, :id], + where: 'impersonation = false', + name: INDEX_NAME + ) + # rubocop:enable Migration/PreventIndexCreation + end + + def down + remove_concurrent_index_by_name :personal_access_tokens, INDEX_NAME + end +end diff --git a/db/post_migrate/20250903134752_index_personal_access_tokens_on_group_id_and_user_type_and_expires_at_and_id.rb b/db/post_migrate/20250903134752_index_personal_access_tokens_on_group_id_and_user_type_and_expires_at_and_id.rb new file mode 100644 index 00000000000000..54d1b95bade20e --- /dev/null +++ b/db/post_migrate/20250903134752_index_personal_access_tokens_on_group_id_and_user_type_and_expires_at_and_id.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +class IndexPersonalAccessTokensOnGroupIdAndUserTypeAndExpiresAtAndId < Gitlab::Database::Migration[2.3] + disable_ddl_transaction! + + milestone '18.5' + + INDEX_NAME = 'index_pats_on_group_id_and_user_type_and_expires_at_and_id' + + def up + # rubocop:disable Migration/PreventIndexCreation -- index is needed for Credentials inventory + add_concurrent_index( + :personal_access_tokens, + [:group_id, :user_type, :expires_at, :id], + where: 'impersonation = false', + name: INDEX_NAME + ) + # rubocop:enable Migration/PreventIndexCreation + end + + def down + remove_concurrent_index_by_name :personal_access_tokens, INDEX_NAME + end +end diff --git a/db/post_migrate/20250903135508_index_personal_access_tokens_on_group_id_and_user_type_and_created_at_and_id.rb b/db/post_migrate/20250903135508_index_personal_access_tokens_on_group_id_and_user_type_and_created_at_and_id.rb new file mode 100644 index 00000000000000..af45c24d9dbe28 --- /dev/null +++ b/db/post_migrate/20250903135508_index_personal_access_tokens_on_group_id_and_user_type_and_created_at_and_id.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +class IndexPersonalAccessTokensOnGroupIdAndUserTypeAndCreatedAtAndId < Gitlab::Database::Migration[2.3] + disable_ddl_transaction! + + milestone '18.5' + + INDEX_NAME = 'index_pats_on_group_id_and_user_type_and_created_at_and_id' + + def up + # rubocop:disable Migration/PreventIndexCreation -- index is needed for Credentials inventory + add_concurrent_index( + :personal_access_tokens, + [:group_id, :user_type, :created_at, :id], + where: 'impersonation = false', + name: INDEX_NAME + ) + # rubocop:enable Migration/PreventIndexCreation + end + + def down + remove_concurrent_index_by_name :personal_access_tokens, INDEX_NAME + end +end diff --git a/db/schema_migrations/20250903123949 b/db/schema_migrations/20250903123949 new file mode 100644 index 00000000000000..cd15e65b8a86eb --- /dev/null +++ b/db/schema_migrations/20250903123949 @@ -0,0 +1 @@ +10dd38f16f46379e4b511c149fc2a5f2eb9e1470f7e4455bee94223d1d2286de \ No newline at end of file diff --git a/db/schema_migrations/20250903134752 b/db/schema_migrations/20250903134752 new file mode 100644 index 00000000000000..3e0f94bf727d3d --- /dev/null +++ b/db/schema_migrations/20250903134752 @@ -0,0 +1 @@ +05b4bff357313cd2844f525fd5ca23a5eb4aee77714fcff5f6940fd79fbaa3e5 \ No newline at end of file diff --git a/db/schema_migrations/20250903135508 b/db/schema_migrations/20250903135508 new file mode 100644 index 00000000000000..05f044f7a084b2 --- /dev/null +++ b/db/schema_migrations/20250903135508 @@ -0,0 +1 @@ +f54d1ee2802ff566ee6a50197ffa481343561339d6c525cca4fcb32331f6c068 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 6e9b49a2849ca7..c0682d5d7e1b18 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -40892,6 +40892,12 @@ CREATE INDEX index_pats_on_expiring_at_sixty_days_notification_sent_at ON person CREATE INDEX index_pats_on_expiring_at_thirty_days_notification_sent_at ON personal_access_tokens USING btree (expires_at, id) WHERE ((impersonation = false) AND (revoked = false) AND (thirty_days_notification_sent_at IS NULL)); +CREATE INDEX index_pats_on_group_id_and_user_type_and_created_at_and_id ON personal_access_tokens USING btree (group_id, user_type, created_at, id) WHERE (impersonation = false); + +CREATE INDEX index_pats_on_group_id_and_user_type_and_expires_at_and_id ON personal_access_tokens USING btree (group_id, user_type, expires_at, id) WHERE (impersonation = false); + +CREATE INDEX index_pats_on_group_id_and_user_type_and_last_used_at_and_id ON personal_access_tokens USING btree (group_id, user_type, last_used_at, id) WHERE (impersonation = false); + CREATE INDEX index_pats_on_user_id_and_created_at_and_pat_id ON personal_access_tokens USING btree (user_id, created_at, id) WHERE (impersonation = false); CREATE INDEX index_pats_on_user_id_and_expires_at_and_pat_id ON personal_access_tokens USING btree (user_id, expires_at, id DESC) WHERE (impersonation = false); diff --git a/ee/app/controllers/admin/credentials_controller.rb b/ee/app/controllers/admin/credentials_controller.rb index be6633ecb6dceb..35555b9a05f4e6 100644 --- a/ee/app/controllers/admin/credentials_controller.rb +++ b/ee/app/controllers/admin/credentials_controller.rb @@ -78,4 +78,8 @@ def bot_users def revocable nil end + + def group + nil + end end diff --git a/ee/app/controllers/concerns/credentials_inventory_actions.rb b/ee/app/controllers/concerns/credentials_inventory_actions.rb index e99ea9c5287733..7302ee971efcf7 100644 --- a/ee/app/controllers/concerns/credentials_inventory_actions.rb +++ b/ee/app/controllers/concerns/credentials_inventory_actions.rb @@ -53,17 +53,25 @@ def revoke def filter_credentials if show_personal_access_tokens? - ::Authn::CredentialsInventoryPersonalAccessTokensFinder.new( - pat_params( - users: users, - owner_type: pat_owner_type, - group: revocable - ) - ).execute + if Feature.enabled?(:optimize_credentials_inventory, group || :instance) + PersonalAccessTokensFinder.new(pat_params(group: group, user_types: [:human, :service_account])).execute + else + ::Authn::CredentialsInventoryPersonalAccessTokensFinder.new( + pat_params( + users: users, + owner_type: pat_owner_type, + group: revocable + ) + ).execute + end elsif show_ssh_keys? ::KeysFinder.new({ users: users, key_type: 'ssh' }).execute elsif show_resource_access_tokens? - ::PersonalAccessTokensFinder.new(pat_params(users: bot_users)).execute.project_access_token + if Feature.enabled?(:optimize_credentials_inventory, group || :instance) + PersonalAccessTokensFinder.new(pat_params(group: group, user_types: [:project_bot])).execute + else + ::PersonalAccessTokensFinder.new(pat_params(users: bot_users)).execute.project_access_token + end end end diff --git a/ee/app/finders/ee/personal_access_tokens_finder.rb b/ee/app/finders/ee/personal_access_tokens_finder.rb index e5f8e86485e652..0be25b0153eb3f 100644 --- a/ee/app/finders/ee/personal_access_tokens_finder.rb +++ b/ee/app/finders/ee/personal_access_tokens_finder.rb @@ -7,12 +7,14 @@ module PersonalAccessTokensFinder # rubocop:disable Gitlab/BoundedContexts -- Or override :by_owner_type def by_owner_type(tokens) case params[:owner_type] - when 'human' - tokens.owner_is_human when 'service_account' - tokens.owner_is_service_account + if ::Feature.enabled?(:optimize_credentials_inventory, params[:group] || :instance) + tokens.for_user_types(:service_account) + else + tokens.owner_is_service_account + end else - tokens + super end end end diff --git a/ee/lib/api/manage/groups.rb b/ee/lib/api/manage/groups.rb index 9404aa571f5076..e16afc06c974ed 100644 --- a/ee/lib/api/manage/groups.rb +++ b/ee/lib/api/manage/groups.rb @@ -27,7 +27,7 @@ def ssh_keys_finder_params end def sort_order - params[:sort] + params[:sort] || 'created_asc' end def pat_finder_params @@ -87,7 +87,24 @@ def validate_bot_tokens(token, bot_resource) detail 'This feature was introduced in GitLab 17.8.' end get do - if Feature.enabled?(:credentials_inventory_pat_finder, user_group || :instance) + if Feature.enabled?(:optimize_credentials_inventory, user_group || :instance) + tokens = PersonalAccessTokensFinder.new( + declared( + params, + include_missing: false).merge( + { + group: user_group, + # See bug: https://gitlab.com/gitlab-org/gitlab/-/issues/560151 + # user_types should include :service_account too + user_types: [:human], + impersonation: false, + sort: sort_order + } + ) + ).execute.preload_users + + present paginate(tokens), with: Entities::PersonalAccessToken + elsif Feature.enabled?(:credentials_inventory_pat_finder, user_group || :instance) tokens = ::Authn::CredentialsInventoryPersonalAccessTokensFinder.new(pat_finder_params) .execute.preload_users @@ -152,9 +169,17 @@ def validate_bot_tokens(token, bot_resource) end # rubocop:disable CodeReuse/ActiveRecord -- Specific to this endpoint get do - tokens = PersonalAccessTokensFinder.new(rat_finder_params) - .execute - .includes(user: [:members, { user_detail: :bot_namespace }]) + tokens = + if Feature.enabled?(:optimize_credentials_inventory, user_group || :instance) + PersonalAccessTokensFinder.new( + declared(params, include_missing: false) + .merge({ group: user_group, user_types: [:project_bot], impersonation: false, sort: sort_order }) + ).execute.includes(user: [:members, { user_detail: :bot_namespace }]) + else + PersonalAccessTokensFinder.new(rat_finder_params) + .execute + .includes(user: [:members, { user_detail: :bot_namespace }]) + end present paginate(tokens), with: Entities::ResourceAccessToken end diff --git a/ee/spec/requests/api/manage/groups_spec.rb b/ee/spec/requests/api/manage/groups_spec.rb index aede9b2bf9413e..d8cd275a457f00 100644 --- a/ee/spec/requests/api/manage/groups_spec.rb +++ b/ee/spec/requests/api/manage/groups_spec.rb @@ -190,23 +190,41 @@ end end + # rubocop:disable RSpec/MultipleMemoizedHelpers -- Need helpers for testing multiple scenarios describe 'GET /groups/:id/manage/personal_access_tokens' do let_it_be(:path) { "/groups/#{group.id}/manage/personal_access_tokens" } - let_it_be(:user) { create(:enterprise_user, enterprise_group: group) } + # Tokens that should be returned by the API endpoint - let_it_be(:active_token1) { create(:personal_access_token, user: user, scopes: [:api]) } - let_it_be(:active_token2) { create(:personal_access_token, user: user, scopes: [:api]) } - let_it_be(:expired_token1) { create(:personal_access_token, user: user, expires_at: 1.year.ago) } - let_it_be(:expired_token2) { create(:personal_access_token, user: user, expires_at: 1.year.ago) } - let_it_be(:revoked_token1) { create(:personal_access_token, user: user, revoked: true) } - let_it_be(:revoked_token2) { create(:personal_access_token, user: user, revoked: true) } + let_it_be(:enterprise_user1) { create(:enterprise_user, enterprise_group: group) } + let_it_be(:enterprise_user2) { create(:enterprise_user, enterprise_group: group) } + + let_it_be(:active_token1) { create(:personal_access_token, user: enterprise_user1, group: group, scopes: [:api]) } + let_it_be(:active_token2) { create(:personal_access_token, user: enterprise_user2, group: group, scopes: [:api]) } + + let_it_be(:expired_token1) do + create(:personal_access_token, user: enterprise_user1, group: group, expires_at: 1.year.ago) + end + + let_it_be(:expired_token2) do + create(:personal_access_token, user: enterprise_user2, group: group, expires_at: 1.year.ago) + end + + let_it_be(:revoked_token1) { create(:personal_access_token, user: enterprise_user1, group: group, revoked: true) } + let_it_be(:revoked_token2) { create(:personal_access_token, user: enterprise_user2, group: group, revoked: true) } + + let_it_be(:created_2_days_ago_token) do + create(:personal_access_token, user: enterprise_user1, group: group, created_at: 2.days.ago) + end + + let_it_be(:named_token) { create(:personal_access_token, user: enterprise_user2, group: group, name: 'test_1') } + + let_it_be(:last_used_2_days_ago_token) do + create(:personal_access_token, user: enterprise_user1, group: group, last_used_at: 2.days.ago) + end - let_it_be(:created_2_days_ago_token) { create(:personal_access_token, user: user, created_at: 2.days.ago) } - let_it_be(:named_token) { create(:personal_access_token, user: user, name: 'test_1') } - let_it_be(:last_used_2_days_ago_token) { create(:personal_access_token, user: user, last_used_at: 2.days.ago) } let_it_be(:last_used_2_months_ago_token) do - create(:personal_access_token, user: user, last_used_at: 2.months.ago) + create(:personal_access_token, user: enterprise_user2, group: group, last_used_at: 2.months.ago) end let_it_be(:created_at_asc) do @@ -224,9 +242,40 @@ ] end - let_it_be(:non_enterprise_user) { create(:user) } - # Token which should not be returned in any responses - let_it_be(:non_enterprise_token) { create(:personal_access_token, user: non_enterprise_user, scopes: [:api]) } + # Tokens that should not be returned by the API endpoint + + let_it_be(:another_group) { create(:group) } + + let_it_be(:enterprise_user_of_another_group) { create(:enterprise_user, enterprise_group: another_group) } + + let_it_be(:token_of_enterprise_user_of_another_group) do + create( + :personal_access_token, + user: enterprise_user_of_another_group, + group: another_group, + scopes: [:api] + ) + end + + let_it_be(:token_that_belongs_to_another_group) do + create(:personal_access_token, group: another_group, scopes: [:api]) + end + + let_it_be(:token_that_does_not_belong_to_any_group) do + create(:personal_access_token, scopes: [:api]) + end + + let_it_be(:resource_access_token_that_does_not_belong_to_any_group) do + create(:resource_access_token, scopes: [:api]) + end + + let_it_be(:resource_access_token_that_belongs_to_another_group) do + create(:resource_access_token, resource: another_group, group: another_group, scopes: [:api]) + end + + let_it_be(:resource_access_token_that_belong_to_the_group) do + create(:resource_access_token, resource: group, group: group, scopes: [:api]) + end it_behaves_like 'feature is not available to non saas versions', "get_request" @@ -235,8 +284,6 @@ it_behaves_like 'a manage groups GET endpoint' - it_behaves_like 'a /manage/PAT GET endpoint using the credentials inventory PAT finder' - it 'returns 404 for non-existing group' do get(api( "/groups/#{non_existing_record_id}/manage/personal_access_tokens", @@ -245,8 +292,30 @@ expect(response).to have_gitlab_http_status(:not_found) end + + context 'when optimize_credentials_inventory FF is disabled' do + before do + stub_feature_flags(optimize_credentials_inventory: false) + end + + it_behaves_like 'a /manage/PAT GET endpoint using the credentials inventory PAT finder' + + it_behaves_like 'an access token GET API with access token params' + + it_behaves_like 'a manage groups GET endpoint' + + it 'returns 404 for non-existing group' do + get(api( + "/groups/#{non_existing_record_id}/manage/personal_access_tokens", + personal_access_token: personal_access_token + )) + + expect(response).to have_gitlab_http_status(:not_found) + end + end end end + # rubocop:enable RSpec/MultipleMemoizedHelpers describe 'DELETE /groups/:id/manage/personal_access_tokens/:id' do let_it_be(:enterprise_user) { create(:enterprise_user, enterprise_group: group) } @@ -712,23 +781,43 @@ describe 'GET /groups/:id/manage/resource_access_tokens' do let_it_be(:path) { "/groups/#{group.id}/manage/resource_access_tokens" } - let_it_be(:group_bot) { create(:user, :project_bot, bot_namespace: group, developer_of: group) } + # Tokens that should be returned by the API endpoint + let_it_be(:project) { create(:project, namespace: group) } - let_it_be(:project_bot) do - create(:user, :project_bot, bot_namespace: project.project_namespace, developer_of: project) - end - - let_it_be(:active_token1) { create(:personal_access_token, user: project_bot) } - let_it_be(:active_token2) { create(:personal_access_token, user: group_bot) } - let_it_be(:expired_token1) { create(:personal_access_token, user: group_bot, expires_at: 1.year.ago) } - let_it_be(:expired_token2) { create(:personal_access_token, user: group_bot, expires_at: 1.year.ago) } - let_it_be(:revoked_token1) { create(:personal_access_token, user: group_bot, revoked: true) } - let_it_be(:revoked_token2) { create(:personal_access_token, user: group_bot, revoked: true) } - let_it_be(:created_2_days_ago_token) { create(:personal_access_token, user: project_bot, created_at: 2.days.ago) } - let_it_be(:named_token) { create(:personal_access_token, user: group_bot, name: "Test token") } - let_it_be(:last_used_2_days_ago_token) { create(:personal_access_token, user: group_bot, last_used_at: 2.days.ago) } + + let_it_be(:active_token1) { create(:resource_access_token, resource: project, group: group) } + let_it_be(:active_token2) { create(:resource_access_token, resource: group, group: group) } + + let_it_be(:expired_token1) do + create(:resource_access_token, resource: group, group: group, expires_at: 1.year.ago) + end + + let_it_be(:expired_token2) do + create(:resource_access_token, resource: group, group: group, expires_at: 1.year.ago) + end + + let_it_be(:revoked_token1) do + create(:resource_access_token, resource: group, group: group, revoked: true) + end + + let_it_be(:revoked_token2) do + create(:resource_access_token, resource: group, group: group, revoked: true) + end + + let_it_be(:created_2_days_ago_token) do + create(:resource_access_token, resource: project, group: group, created_at: 2.days.ago) + end + + let_it_be(:named_token) do + create(:resource_access_token, resource: group, group: group, name: "Test token") + end + + let_it_be(:last_used_2_days_ago_token) do + create(:resource_access_token, resource: group, group: group, last_used_at: 2.days.ago) + end + let_it_be(:last_used_2_months_ago_token) do - create(:personal_access_token, user: group_bot, last_used_at: 2.months.ago) + create(:resource_access_token, resource: group, group: group, last_used_at: 2.months.ago) end let_it_be(:created_at_asc) do @@ -746,12 +835,25 @@ ] end - let_it_be(:other_group_bot) { create(:user, :project_bot, bot_namespace: create(:group)) } + # Tokens that should not be returned by the API endpoint + + let_it_be(:another_group) { create(:group) } + + let_it_be(:resource_access_token_that_belongs_to_another_group) do + create(:resource_access_token, resource: another_group, group: another_group, scopes: [:api]) + end + + let_it_be(:resource_access_token_that_does_not_belong_to_any_group) do + create(:resource_access_token, scopes: [:api]) + end + + let_it_be(:personal_access_token_that_belong_to_the_group) do + create(:personal_access_token, group: group, scopes: [:api]) + end - # Tokens which should not be returned in any responses - let_it_be(:excluded_token1) { create(:personal_access_token, user: current_user) } - let_it_be(:excluded_token2) { create(:personal_access_token, user: create(:user, :service_account)) } - let_it_be(:excluded_token3) { create(:personal_access_token, user: other_group_bot) } + let_it_be(:personal_access_token_that_belong_to_another_group) do + create(:personal_access_token, group: another_group, scopes: [:api]) + end it_behaves_like 'feature is not available to non saas versions', "get_request" @@ -806,6 +908,69 @@ get(api(path, personal_access_token: personal_access_token), headers: dpop_header_val) end.not_to exceed_all_query_limit(control) end + + context 'when optimize_credentials_inventory FF is disabled' do + before do + stub_feature_flags(optimize_credentials_inventory: false) + end + + it_behaves_like 'an access token GET API with access token params' + it_behaves_like 'a manage groups GET endpoint' + + it 'returns 404 for non-existing group' do + get(api( + "/groups/#{non_existing_record_id}/manage/resource_access_tokens", + personal_access_token: personal_access_token + ), headers: dpop_headers_for(current_user)) + + expect(response).to have_gitlab_http_status(:not_found) + end + + it 'returns the expected response for group tokens' do + get api(path, personal_access_token: personal_access_token), params: { sort: 'created_at_desc' }, + headers: dpop_headers_for(current_user) + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to match_response_schema('public_api/v4/resource_access_tokens') + expect(json_response[0]['id']).to eq(last_used_2_months_ago_token.id) + expect(json_response[0]['resource_type']).to eq('group') + expect(json_response[0]['resource_id']).to eq(group.id) + end + + it 'returns the expected response for project tokens' do + get( + api( + path, + personal_access_token: personal_access_token + ), + params: { sort: 'created_at_asc' }, + headers: dpop_headers_for(current_user) + ) + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to match_response_schema('public_api/v4/resource_access_tokens') + expect(json_response[0]['id']).to eq(created_2_days_ago_token.id) + expect(json_response[0]['resource_type']).to eq('project') + expect(json_response[0]['resource_id']).to eq(project.id) + end + + it 'avoids N+1 queries' do + dpop_header_val = dpop_headers_for(current_user) + + get(api(path, personal_access_token: personal_access_token), headers: dpop_header_val) + + control = ActiveRecord::QueryRecorder.new(skip_cached: false) do + get(api(path, personal_access_token: personal_access_token), headers: dpop_header_val) + end + + other_bot = create(:user, :project_bot, bot_namespace: group, developer_of: group) + create(:personal_access_token, user: other_bot) + + expect do + get(api(path, personal_access_token: personal_access_token), headers: dpop_header_val) + end.not_to exceed_all_query_limit(control) + end + end end end diff --git a/ee/spec/requests/groups/security/credentials_controller_spec.rb b/ee/spec/requests/groups/security/credentials_controller_spec.rb index 4fd460eebe37da..2e236ee1beb768 100644 --- a/ee/spec/requests/groups/security/credentials_controller_spec.rb +++ b/ee/spec/requests/groups/security/credentials_controller_spec.rb @@ -2,21 +2,86 @@ require 'spec_helper' +# rubocop:disable RSpec/MultipleMemoizedHelpers -- Need helpers for testing multiple scenarios RSpec.describe Groups::Security::CredentialsController, :saas, feature_category: :user_management do let_it_be(:group) { create(:group, :private) } - let_it_be(:enterprise_users) { create_list(:enterprise_user, 2, enterprise_group: group) } - let_it_be(:service_accounts) { create_list(:service_account, 2, provisioned_by_group: group) } - let_it_be(:owner) { enterprise_users.first } - let_it_be(:maintainer) { enterprise_users.last } + let_it_be(:subgroup) { create(:group, :private, parent: group) } + let_it_be(:project) { create(:project, :private, group: subgroup) } + + let_it_be(:enterprise_user1) { create(:enterprise_user, enterprise_group: group) } + let_it_be(:enterprise_user2) { create(:enterprise_user, enterprise_group: group) } + let_it_be(:group_enterprise_users) { [enterprise_user1, enterprise_user2] } + + let_it_be(:service_account1) { create(:service_account, provisioned_by_group: group) } + let_it_be(:service_account2) { create(:service_account, provisioned_by_group: group) } + let_it_be(:group_service_accounts) { [service_account1, service_account2] } + + let_it_be(:owner) { enterprise_user1 } + let_it_be(:maintainer) { enterprise_user2 } + let_it_be(:group_id) { group.to_param } - let_it_be(:personal_access_token) { create(:personal_access_token, user: maintainer) } - let_it_be(:service_account_token) { create(:personal_access_token, user: service_accounts.first) } - before do - allow_next_instance_of(Gitlab::Auth::GroupSaml::SsoEnforcer) do |sso_enforcer| - allow(sso_enforcer).to receive(:active_session?).and_return(true) - end + let_it_be(:enterprise_user1_personal_access_token) do + create(:personal_access_token, user: enterprise_user1, group: group) + end + + let_it_be(:enterprise_user2_personal_access_token) do + create(:personal_access_token, user: enterprise_user2, group: group) + end + + let_it_be(:enterprise_user2_revoked_personal_access_token) do + create(:personal_access_token, revoked: true, user: enterprise_user2, group: group) + end + + let_it_be(:enterprise_user2_expired_personal_access_token) do + create(:personal_access_token, expires_at: 5.days.ago, user: enterprise_user2, group: group) + end + + let_it_be(:service_account1_personal_access_token) do + create(:personal_access_token, user: service_account1, group: group) + end + + let_it_be(:service_account2_personal_access_token) do + create(:personal_access_token, user: service_account2, group: group) + end + + let_it_be(:another_group) { create(:group) } + let_it_be(:enterprise_user_of_another_group) { create(:enterprise_user, enterprise_group: another_group) } + let_it_be(:service_account_of_another_group) { create(:service_account, provisioned_by_group: another_group) } + + let_it_be(:enterprise_user_of_another_group_personal_access_token) do + create(:personal_access_token, user: enterprise_user_of_another_group, group: another_group) + end + let_it_be(:service_account_of_another_group_personal_access_token) do + create(:personal_access_token, user: service_account_of_another_group, group: another_group) + end + + let_it_be(:token_that_does_not_belong_to_any_group) do + create(:personal_access_token) + end + + let_it_be(:resource_access_token_that_belong_to_the_group1) do + create(:resource_access_token, resource: group, group: group, scopes: [:api]) + end + + let_it_be(:resource_access_token_that_belong_to_the_group2) do + create(:resource_access_token, resource: subgroup, group: group, scopes: [:api]) + end + + let_it_be(:resource_access_token_that_belong_to_the_group3) do + create(:resource_access_token, resource: project, group: group, scopes: [:api]) + end + + let_it_be(:resource_access_token_that_belongs_to_another_group) do + create(:resource_access_token, resource: another_group, group: another_group, scopes: [:api]) + end + + let_it_be(:resource_access_token_that_does_not_belong_to_any_group) do + create(:resource_access_token, scopes: [:api]) + end + + before do group.add_owner(owner) group.add_maintainer(maintainer) @@ -27,11 +92,11 @@ let(:filter) {} let(:owner_type) {} - subject(:get_request) { get group_security_credentials_path(group_id: group_id.to_param, filter: filter, owner_type: owner_type) } + subject(:get_request) { get group_security_credentials_path(group_id: group_id, filter: filter, owner_type: owner_type) } context 'when `credentials_inventory` feature is licensed' do before do - stub_licensed_features(credentials_inventory: true, group_saml: true) + stub_licensed_features(credentials_inventory: true) end context 'for a user with access to view credentials inventory' do @@ -51,22 +116,13 @@ end context 'filtering by type of credential' do - before do - enterprise_users.each do |user| - create(:personal_access_token, user: user) - end - service_accounts.each do |user| - create(:personal_access_token, user: user) - end - end - context 'no credential type specified' do let(:filter) { nil } it 'returns all personal access tokens' do get_request - expected_tokens = PersonalAccessToken.where(user: enterprise_users + service_accounts) + expected_tokens = PersonalAccessToken.where(user: group_enterprise_users + group_service_accounts) expect(assigns(:credentials)).to match_array(expected_tokens) end end @@ -77,7 +133,7 @@ it 'returns all personal access tokens' do get_request - expected_tokens = PersonalAccessToken.where(user: enterprise_users + service_accounts) + expected_tokens = PersonalAccessToken.where(user: group_enterprise_users + group_service_accounts) expect(assigns(:credentials)).to match_array(expected_tokens) end end @@ -88,35 +144,19 @@ it 'returns all personal access tokens' do get_request - expected_tokens = PersonalAccessToken.where(user: enterprise_users + service_accounts) + expected_tokens = PersonalAccessToken.where(user: group_enterprise_users + group_service_accounts) expect(assigns(:credentials)).to match_array(expected_tokens) end end - context 'user scope' do - it 'does not show the credentials of a user outside the group' do - personal_access_token = create(:personal_access_token, user: create(:user)) - - get_request - - expect(assigns(:credentials)).not_to include(personal_access_token) - end - - it 'includes service account tokens in the scope' do - get_request - - expect(assigns(:credentials)).to include(service_account_token) - end - end - context 'credential type specified as `ssh_keys`' do let(:filter) { 'ssh_keys' } before do - enterprise_users.each do |user| + group_enterprise_users.each do |user| create(:personal_key, user: user) end - service_accounts.each do |user| + group_service_accounts.each do |user| create(:personal_key, user: user) end end @@ -124,68 +164,23 @@ it 'filters by ssh keys' do get_request - expect(assigns(:credentials)).to match_array(Key.regular_keys.where(user: enterprise_users + service_accounts)) + expect(assigns(:credentials)).to match_array(Key.regular_keys.where(user: group_enterprise_users + group_service_accounts)) end end context 'credential type specified as `resource access tokens`' do let(:filter) { 'resource_access_tokens' } - let_it_be(:subgroup) { create(:group, :private, parent: group) } - let_it_be(:project) { create(:project, :private, group: subgroup) } - let_it_be(:other_group) { create(:group, :private) } - - let_it_be(:group_bot) { create(:user, :project_bot, name: "Group bot", created_by_id: owner.id) } - let_it_be(:subgroup_bot) { create(:user, :project_bot, name: "Subgroup bot", created_by_id: owner.id) } - let_it_be(:project_bot) { create(:user, :project_bot, name: "Project bot", created_by_id: owner.id) } - let_it_be(:nonmember_bot) { create(:user, :project_bot, name: "Non-member bot", created_by_id: owner.id) } - - let_it_be(:group_bot_token) do - create(:personal_access_token, user: group_bot, scopes: %w[read_api]) - end - - let_it_be(:subgroup_bot_token) do - create(:personal_access_token, user: subgroup_bot, scopes: %w[read_api]) - end - - let_it_be(:project_bot_token) do - create(:personal_access_token, user: project_bot, scopes: %w[read_api]) - end - - let_it_be(:nonmember_bot_token) do - create(:personal_access_token, user: nonmember_bot, scopes: %w[read_api]) - end - - before_all do - group_bot.update!(bot_namespace: group) - subgroup_bot.update!(bot_namespace: subgroup) - project_bot.update!(bot_namespace: project.project_namespace) - nonmember_bot.update!(bot_namespace: other_group) - - group.add_developer(group_bot) - subgroup.add_developer(subgroup_bot) - project.add_developer(project_bot) - other_group.add_developer(nonmember_bot) - end - - it 'filters resource access tokens for project bots that belong to the hierarchy' do + it 'returns all resource access tokens that belong to the hierarchy of the group' do get_request - expect(assigns(:credentials)).to match_array([group_bot_token, subgroup_bot_token, project_bot_token]) + expected_tokens = PersonalAccessToken.where(user_type: :project_bot, group: group) + expect(assigns(:credentials)).to match_array(expected_tokens) end end end context 'filtering by owner type' do - before do - enterprise_users.each do |user| - create(:personal_access_token, user: user) - end - service_accounts.each do |user| - create(:personal_access_token, user: user) - end - end - context 'when owner_type is human' do let(:filter) { 'personal_access_tokens' } let(:owner_type) { 'human' } @@ -193,7 +188,7 @@ it 'returns only human user tokens' do get_request - expected_tokens = PersonalAccessToken.where(user: enterprise_users) + expected_tokens = PersonalAccessToken.where(user: group_enterprise_users) expect(assigns(:credentials)).to match_array(expected_tokens) end end @@ -205,7 +200,7 @@ it 'returns only service account tokens' do get_request - expected_tokens = PersonalAccessToken.where(user: service_accounts) + expected_tokens = PersonalAccessToken.where(user: group_service_accounts) expect(assigns(:credentials)).to match_array(expected_tokens) end end @@ -217,7 +212,7 @@ it 'returns all tokens regardless of owner type' do get_request - expected_tokens = PersonalAccessToken.where(user: enterprise_users + service_accounts) + expected_tokens = PersonalAccessToken.where(user: group_enterprise_users + group_service_accounts) expect(assigns(:credentials)).to match_array(expected_tokens) end end @@ -248,6 +243,162 @@ expect(response).to have_gitlab_http_status(:not_found) end end + + context 'when optimize_credentials_inventory FF is disabled' do + before do + stub_feature_flags(optimize_credentials_inventory: false) + end + + context 'when `credentials_inventory` feature is licensed' do + before do + stub_licensed_features(credentials_inventory: true) + end + + context 'for a user with access to view credentials inventory' do + it_behaves_like 'internal event tracking' do + let(:event) { 'visit_authentication_credentials_inventory' } + let(:user) { owner } + let(:project) { nil } + let(:namespace) { group } + + subject(:group_security_credentials_request) { get_request } + end + + it 'responds with 200' do + get_request + + expect(response).to have_gitlab_http_status(:ok) + end + + context 'filtering by type of credential' do + context 'no credential type specified' do + let(:filter) { nil } + + it 'returns all personal access tokens' do + get_request + + expected_tokens = PersonalAccessToken.where(user: group_enterprise_users + group_service_accounts) + expect(assigns(:credentials)).to match_array(expected_tokens) + end + end + + context 'non-existent credential type specified' do + let(:filter) { 'non_existent_credential_type' } + + it 'returns all personal access tokens' do + get_request + + expected_tokens = PersonalAccessToken.where(user: group_enterprise_users + group_service_accounts) + expect(assigns(:credentials)).to match_array(expected_tokens) + end + end + + context 'credential type specified as `personal_access_tokens`' do + let(:filter) { 'personal_access_tokens' } + + it 'returns all personal access tokens' do + get_request + + expected_tokens = PersonalAccessToken.where(user: group_enterprise_users + group_service_accounts) + expect(assigns(:credentials)).to match_array(expected_tokens) + end + end + + context 'credential type specified as `ssh_keys`' do + let(:filter) { 'ssh_keys' } + + before do + group_enterprise_users.each do |user| + create(:personal_key, user: user) + end + group_service_accounts.each do |user| + create(:personal_key, user: user) + end + end + + it 'filters by ssh keys' do + get_request + + expect(assigns(:credentials)).to match_array(Key.regular_keys.where(user: group_enterprise_users + group_service_accounts)) + end + end + + context 'credential type specified as `resource access tokens`' do + let(:filter) { 'resource_access_tokens' } + + it 'returns all resource access tokens that belong to the hierarchy of the group' do + get_request + + expected_tokens = PersonalAccessToken.where(user_type: :project_bot, group: group) + expect(assigns(:credentials)).to match_array(expected_tokens) + end + end + end + + context 'filtering by owner type' do + context 'when owner_type is human' do + let(:filter) { 'personal_access_tokens' } + let(:owner_type) { 'human' } + + it 'returns only human user tokens' do + get_request + + expected_tokens = PersonalAccessToken.where(user: group_enterprise_users) + expect(assigns(:credentials)).to match_array(expected_tokens) + end + end + + context 'when owner_type is service_account' do + let(:filter) { 'personal_access_tokens' } + let(:owner_type) { 'service_account' } + + it 'returns only service account tokens' do + get_request + + expected_tokens = PersonalAccessToken.where(user: group_service_accounts) + expect(assigns(:credentials)).to match_array(expected_tokens) + end + end + + context 'when owner_type is not specified' do + let(:filter) { 'personal_access_tokens' } + let(:owner_type) { nil } + + it 'returns all tokens regardless of owner type' do + get_request + + expected_tokens = PersonalAccessToken.where(user: group_enterprise_users + group_service_accounts) + expect(assigns(:credentials)).to match_array(expected_tokens) + end + end + end + + context 'for a user without access to view credentials inventory' do + before do + sign_in(maintainer) + end + + it 'responds with 404' do + get_request + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + end + + context 'when `credentials_inventory` feature is unlicensed' do + before do + stub_licensed_features(credentials_inventory: false) + end + + it 'returns 404' do + get_request + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end end describe 'DELETE #destroy' do @@ -261,7 +412,7 @@ shared_examples_for 'responds with 404' do it do - put revoke_group_security_credential_path(group_id: group_id.to_param, id: token_id) + put revoke_group_security_credential_path(group_id: group_id, id: token_id) expect(response).to have_gitlab_http_status(:not_found) end @@ -269,7 +420,7 @@ shared_examples_for 'displays the flash success message' do it do - put revoke_group_security_credential_path(group_id: group_id.to_param, id: token_id) + put revoke_group_security_credential_path(group_id: group_id, id: token_id) expect(response).to redirect_to(group_security_credentials_path) expect(flash[:notice]).to start_with 'Revoked personal access token ' @@ -278,7 +429,7 @@ shared_examples_for 'displays the flash error message' do it do - put revoke_group_security_credential_path(group_id: group_id.to_param, id: token_id) + put revoke_group_security_credential_path(group_id: group_id, id: token_id) expect(response).to redirect_to(group_security_credentials_path) expect(flash[:alert]).to eql 'Not permitted to revoke' @@ -287,7 +438,7 @@ context 'when `credentials_inventory` feature is enabled', :saas do before do - stub_licensed_features(credentials_inventory: true, group_saml: true, domain_verification: true) + stub_licensed_features(credentials_inventory: true, domain_verification: true) end context 'for a group with enterprise users' do @@ -300,62 +451,50 @@ describe 'with an existing personal access token' do context 'personal access token is already revoked' do - let_it_be(:token_id) { create(:personal_access_token, revoked: true, user: maintainer).id } + let_it_be(:token_id) { enterprise_user2_revoked_personal_access_token.id } it_behaves_like 'displays the flash success message' end context 'personal access token is already expired' do - let_it_be(:token_id) { create(:personal_access_token, expires_at: 5.days.ago, user: maintainer).id } + let_it_be(:token_id) { enterprise_user2_expired_personal_access_token.id } it_behaves_like 'displays the flash success message' end context 'does not have permissions to revoke the credential' do - let_it_be(:token_id) { create(:personal_access_token, user: create(:user)).id } + let_it_be(:token_id) { enterprise_user_of_another_group_personal_access_token.id } it_behaves_like 'responds with 404' end - context 'personal access token is already revoked' do - let_it_be(:token_id) { create(:personal_access_token, revoked: true, user: maintainer).id } - - it_behaves_like 'displays the flash success message' - end - - context 'personal access token is already expired' do - let_it_be(:token_id) { create(:personal_access_token, expires_at: 5.days.ago, user: maintainer).id } - - it_behaves_like 'displays the flash success message' - end - context 'personal access token is not revoked or expired' do - let_it_be(:token_id) { personal_access_token.id } + let_it_be(:token_id) { enterprise_user2_personal_access_token.id } it_behaves_like 'displays the flash success message' it 'informs the token owner' do expect(CredentialsInventoryMailer).to receive_message_chain(:personal_access_token_revoked_email, :deliver_later) - put revoke_group_security_credential_path(group_id: group_id.to_param, id: personal_access_token.id) + put revoke_group_security_credential_path(group_id: group_id, id: token_id) end end context 'service account personal access token' do - let_it_be(:token_id) { service_account_token.id } + let_it_be(:token_id) { service_account2_personal_access_token.id } it_behaves_like 'displays the flash success message' it 'can revoke service account tokens' do - expect { put revoke_group_security_credential_path(group_id: group_id.to_param, id: service_account_token.id) } - .to change { service_account_token.reload.revoked? }.from(false).to(true) + expect { put revoke_group_security_credential_path(group_id: group_id, id: token_id) } + .to change { service_account2_personal_access_token.reload.revoked? }.from(false).to(true) end end end end context 'for a user without access to view credentials inventory' do - let_it_be(:token_id) { create(:personal_access_token, user: owner).id } + let_it_be(:token_id) { enterprise_user2_personal_access_token.id } before do sign_in(maintainer) @@ -363,22 +502,17 @@ it_behaves_like 'responds with 404' end - end - context 'for non-enterprise user tokens' do - let_it_be(:token_id) { personal_access_token.id } - let_it_be(:group_id) { create(:group).id } + context 'for a token that does not belong to any group' do + let_it_be(:token_id) { token_that_does_not_belong_to_any_group.id } - it 'responds with 404' do - expect do - put revoke_group_security_credential_path(group_id: group_id.to_param, id: token_id) - end.to raise_error(ActionController::RoutingError) + it_behaves_like 'responds with 404' end end end context 'when `credentials_inventory` feature is disabled' do - let_it_be(:token_id) { create(:personal_access_token, user: owner).id } + let_it_be(:token_id) { enterprise_user2_personal_access_token.id } before do stub_licensed_features(credentials_inventory: false) @@ -388,3 +522,4 @@ end end end +# rubocop:enable RSpec/MultipleMemoizedHelpers diff --git a/ee/spec/support/shared_examples/features/credentials_inventory_shared_examples.rb b/ee/spec/support/shared_examples/features/credentials_inventory_shared_examples.rb index c55ae78ea85646..53f0897598c982 100644 --- a/ee/spec/support/shared_examples/features/credentials_inventory_shared_examples.rb +++ b/ee/spec/support/shared_examples/features/credentials_inventory_shared_examples.rb @@ -12,6 +12,7 @@ create( :personal_access_token, user: user, + group: user.enterprise_group, created_at: '2019-12-10', updated_at: '2020-06-22', expires_at: expiry_date @@ -50,6 +51,7 @@ create(:personal_access_token, :revoked, user: user, + group: user.enterprise_group, created_at: '2019-12-10', updated_at: '2020-06-22', expires_at: Time.zone.now) @@ -151,6 +153,7 @@ create( :personal_access_token, user: user, + group: group, created_at: '2019-12-10', updated_at: '2020-06-22', expires_at: expiry_date diff --git a/ee/spec/support/shared_examples/requests/credentials_inventory_shared_examples.rb b/ee/spec/support/shared_examples/requests/credentials_inventory_shared_examples.rb index 430388ac33de5b..1ecf307272feb0 100644 --- a/ee/spec/support/shared_examples/requests/credentials_inventory_shared_examples.rb +++ b/ee/spec/support/shared_examples/requests/credentials_inventory_shared_examples.rb @@ -3,7 +3,7 @@ RSpec.shared_examples_for 'credentials inventory delete SSH key' do |group_credentials_inventory: false| include AdminModeHelper - let_it_be(:user) { group_credentials_inventory ? enterprise_users.last : create(:user, name: 'abc') } + let_it_be(:user) { group_credentials_inventory ? group_enterprise_users.last : create(:user, name: 'abc') } let_it_be(:ssh_key) { create(:personal_key, user: user) } let(:ssh_key_id) { ssh_key.id } diff --git a/spec/db/schema_spec.rb b/spec/db/schema_spec.rb index b43f0873272834..cadf079bd65f37 100644 --- a/spec/db/schema_spec.rb +++ b/spec/db/schema_spec.rb @@ -315,6 +315,7 @@ packages_package_files: 16, packages_packages: 27, project_type_ci_runners: 16, + personal_access_tokens: 17, # We will be able to remove 3 indexes added in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/193375 after https://gitlab.com/gitlab-org/gitlab/-/issues/561260 is complete. projects: 55, sbom_occurrences: 25, users: 34, # Decrement by 1 after the removal of a temporary index https://gitlab.com/gitlab-org/gitlab/-/merge_requests/184848 diff --git a/spec/factories/personal_access_tokens.rb b/spec/factories/personal_access_tokens.rb index a640eaeda37047..7b34f54a6b011c 100644 --- a/spec/factories/personal_access_tokens.rb +++ b/spec/factories/personal_access_tokens.rb @@ -77,9 +77,7 @@ before(:create) do |token, evaluator| bot_namespace = evaluator.resource.is_a?(Group) ? evaluator.resource : evaluator.resource.project_namespace token.user.update!(bot_namespace: bot_namespace) - end - after(:create) do |token, evaluator| evaluator.resource.add_member(token.user, evaluator.access_level) end diff --git a/spec/finders/personal_access_tokens_finder_spec.rb b/spec/finders/personal_access_tokens_finder_spec.rb index 967c8ed4b8517f..2da1ed2d7df7b8 100644 --- a/spec/finders/personal_access_tokens_finder_spec.rb +++ b/spec/finders/personal_access_tokens_finder_spec.rb @@ -21,6 +21,7 @@ let_it_be(:project_bot) { create(:user, :project_bot) } let_it_be(:first_organization) { create(:organization) } let_it_be(:second_organization) { create(:organization) } + let_it_be(:group) { create(:group) } let_it_be(:tokens) do { @@ -31,7 +32,9 @@ active_impersonation: create(:personal_access_token, :impersonation, user: user, organization: organization), expired_impersonation: create(:personal_access_token, :expired, :impersonation, user: user, organization: organization), revoked_impersonation: create(:personal_access_token, :revoked, :impersonation, user: user, organization: organization), - bot: create(:personal_access_token, user: project_bot, organization: organization) + bot: create(:personal_access_token, user: project_bot, organization: organization), + with_group: create(:personal_access_token, organization: organization, group: group), + with_another_group: create(:personal_access_token, organization: organization, group: create(:group)) } end @@ -145,11 +148,28 @@ end end + describe 'by_user_types' do + let(:user_types) { [:project_bot] } + let(:params) { { user_types: user_types } } + + it 'returns tokens by user_types' do + is_expected.to match_array(PersonalAccessToken.where(user_type: user_types)) + end + + context 'when user_types are not specified' do + let(:user_types) { nil } + + it 'returns all tokens' do + is_expected.to match_array(tokens.values) + end + end + end + describe 'by impersonation' do where(:by_impersonation, :expected_tokens) do nil | ref(:tokens_keys) true | [:active_impersonation, :expired_impersonation, :revoked_impersonation] - false | [:active, :active_other, :expired, :revoked, :bot] + false | [:active, :active_other, :expired, :revoked, :bot, :with_group, :with_another_group] 'other' | ref(:tokens_keys) end @@ -165,7 +185,7 @@ describe 'by state' do where(:by_state, :expected_tokens) do nil | ref(:tokens_keys) - 'active' | [:active, :active_other, :active_impersonation, :bot] + 'active' | [:active, :active_other, :active_impersonation, :bot, :with_group, :with_another_group] 'inactive' | [:expired, :revoked, :expired_impersonation, :revoked_impersonation] 'other' | ref(:tokens_keys) end @@ -182,7 +202,7 @@ describe 'by owner type' do where(:by_owner_type, :expected_tokens) do nil | ref(:tokens_keys) - 'human' | [:active, :active_other, :expired, :revoked, :active_impersonation, :expired_impersonation, :revoked_impersonation] + 'human' | [:active, :active_other, :expired, :revoked, :active_impersonation, :expired_impersonation, :revoked_impersonation, :with_group, :with_another_group] 'other' | ref(:tokens_keys) end @@ -192,14 +212,24 @@ it 'returns tokens by owner type' do is_expected.to match_array(tokens.values_at(*expected_tokens)) end + + context 'when optimize_credentials_inventory FF is disabled' do + before do + stub_feature_flags(optimize_credentials_inventory: false) + end + + it 'returns tokens by owner type' do + is_expected.to match_array(tokens.values_at(*expected_tokens)) + end + end end end describe 'by revoked state' do where(:by_revoked_state, :expected_tokens) do - nil | [:active, :active_other, :expired, :active_impersonation, :expired_impersonation, :bot] + nil | [:active, :active_other, :expired, :active_impersonation, :expired_impersonation, :bot, :with_group, :with_another_group] true | [:revoked, :revoked_impersonation] - false | [:active, :active_other, :expired, :active_impersonation, :expired_impersonation, :bot] + false | [:active, :active_other, :expired, :active_impersonation, :expired_impersonation, :bot, :with_group, :with_another_group] end with_them do @@ -235,7 +265,7 @@ describe 'by created after' do where(:by_created_after, :expected_tokens) do 6.days.ago | ref(:tokens_keys) - 2.days.ago | [:active, :expired, :revoked, :active_impersonation, :expired_impersonation, :revoked_impersonation, :bot] + 2.days.ago | [:active, :expired, :revoked, :active_impersonation, :expired_impersonation, :revoked_impersonation, :bot, :with_group, :with_another_group] 2.days.from_now | [] end @@ -268,7 +298,7 @@ describe 'by expires after' do where(:by_expires_after, :expected_tokens) do 2.days.ago | ref(:tokens_keys) - 30.days.from_now | [:active, :active_other, :revoked, :active_impersonation, :revoked_impersonation, :bot] + 30.days.from_now | [:active, :active_other, :revoked, :active_impersonation, :revoked_impersonation, :bot, :with_group, :with_another_group] 31.days.from_now | [] end @@ -306,7 +336,7 @@ describe 'by last used after' do where(:by_last_used_after, :expected_tokens) do 6.days.ago | ref(:tokens_keys) - 2.days.ago | [:active, :expired, :revoked, :active_impersonation, :expired_impersonation, :revoked_impersonation, :bot] + 2.days.ago | [:active, :expired, :revoked, :active_impersonation, :expired_impersonation, :revoked_impersonation, :bot, :with_group, :with_another_group] 2.days.from_now | [] end @@ -346,17 +376,33 @@ context 'when orgnzation is not specified' do let(:params) { { organization: nil } } - it 'returns empty when organization is not specified' do + it 'returns all tokens' do is_expected.to match_array(all_tokens.values) end end end + describe 'by_group' do + let(:params) { { group: group } } + + it 'returns tokens by group' do + is_expected.to match_array(PersonalAccessToken.where(group: group)) + end + + context 'when group is not specified' do + let(:params) { { group: nil } } + + it 'returns all tokens' do + is_expected.to match_array(tokens.values) + end + end + end + describe 'sort' do where(:sort, :expected_tokens) do nil | ref(:tokens_keys) - 'id_asc' | [:active, :active_other, :expired, :revoked, :active_impersonation, :expired_impersonation, :revoked_impersonation, :bot] - 'id_desc' | [:bot, :revoked_impersonation, :expired_impersonation, :active_impersonation, :revoked, :expired, :active_other, :active] + 'id_asc' | [:active, :active_other, :expired, :revoked, :active_impersonation, :expired_impersonation, :revoked_impersonation, :bot, :with_group, :with_another_group] + 'id_desc' | [:with_another_group, :with_group, :bot, :revoked_impersonation, :expired_impersonation, :active_impersonation, :revoked, :expired, :active_other, :active] 'other' | ref(:tokens_keys) end diff --git a/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb index 2b7d6a5fbaca75..b8b555a95fbf85 100644 --- a/spec/models/personal_access_token_spec.rb +++ b/spec/models/personal_access_token_spec.rb @@ -178,6 +178,42 @@ end end + describe '.for_user_types' do + let_it_be(:human_user) { create(:user) } + let_it_be(:service_account_user) { create(:user, :service_account) } + let_it_be(:project_bot_user) { create(:user, :project_bot) } + + let_it_be(:human_user_personal_access_token1) do + create(:personal_access_token, user: human_user) + end + + let_it_be(:human_user_personal_access_token2) do + create(:personal_access_token, user: human_user) + end + + let_it_be(:service_account_user_personal_access_token) do + create(:personal_access_token, user: service_account_user) + end + + let_it_be(:project_bot_user_personal_access_token) do + create(:personal_access_token, user: project_bot_user) + end + + it 'returns personal access tokens for the specified user types only', :aggregate_failures do + expect(described_class.for_user_types([:human, :service_account])) + .to contain_exactly( + human_user_personal_access_token1, + human_user_personal_access_token2, + service_account_user_personal_access_token + ) + + expect(described_class.for_user_types([:project_bot])) + .to contain_exactly( + project_bot_user_personal_access_token + ) + end + end + describe '.created_before' do let(:last_used_at) { 1.month.ago.beginning_of_hour } let!(:new_used_token) do @@ -306,6 +342,21 @@ expect(described_class.for_organization(organization)).to contain_exactly(personal_access_token) end end + + describe ".for_group" do + let_it_be(:group) { create(:group) } + let_it_be(:personal_access_token1) { create(:personal_access_token, group: group) } + let_it_be(:personal_access_token2) { create(:personal_access_token) } + let_it_be(:personal_access_token3) { create(:personal_access_token, group: group) } + let_it_be(:personal_access_token4) { create(:personal_access_token, group: create(:group)) } + + it "returns personal access tokens for the specified group only" do + expect(described_class.for_group(group)).to contain_exactly( + personal_access_token1, + personal_access_token3 + ) + end + end end describe 'PolicyActor methods' do @@ -773,7 +824,7 @@ describe '.simple_sorts' do it 'includes overridden keys' do - expect(described_class.simple_sorts.keys).to include(*%w[expires_asc expires_at_asc_id_desc expires_desc last_used_asc last_used_desc]) + expect(described_class.simple_sorts.keys).to include(*%w[created_asc created_desc expires_asc expires_desc last_used_asc last_used_desc]) end it 'returns a valid ActiveRecord::Relation for each sort' do @@ -799,15 +850,36 @@ end end + describe 'ordering by created_at' do + let_it_be(:earlier_token) { create(:personal_access_token, created_at: 2.days.ago) } + let_it_be(:later_token) { create(:personal_access_token, created_at: 1.day.ago) } + + describe '.order_created_at_asc_id_asc' do + let_it_be(:earlier_token_2) { create(:personal_access_token, created_at: 2.days.ago) } + + it 'returns ordered list in combination of created_at ascending and id ascending' do + expect(described_class.order_created_at_asc_id_asc).to eq [earlier_token, earlier_token_2, later_token] + end + end + + describe '.order_created_at_desc_id_desc' do + let_it_be(:earlier_token_2) { create(:personal_access_token, created_at: 2.days.ago) } + + it 'returns ordered list in combination of created_at descending and id descending' do + expect(described_class.order_created_at_desc_id_desc).to eq [later_token, earlier_token_2, earlier_token] + end + end + end + describe 'ordering by expires_at' do let_it_be(:earlier_token) { create(:personal_access_token, expires_at: 2.days.ago) } let_it_be(:later_token) { create(:personal_access_token, expires_at: 1.day.ago) } - describe '.order_expires_at_asc_id_desc' do + describe '.order_expires_at_asc_id_asc' do let_it_be(:earlier_token_2) { create(:personal_access_token, expires_at: 2.days.ago) } - it 'returns ordered list in combination of expires_at ascending and id descending' do - expect(described_class.order_expires_at_asc_id_desc).to eq [earlier_token_2, earlier_token, later_token] + it 'returns ordered list in combination of expires_at ascending and id ascending' do + expect(described_class.order_expires_at_asc_id_asc).to eq [earlier_token, earlier_token_2, later_token] end end @@ -825,11 +897,11 @@ let_it_be(:earlier_token) { create(:personal_access_token, last_used_at: two_days_ago) } let_it_be(:later_token) { create(:personal_access_token, last_used_at: 1.day.ago) } - describe '.order_last_used_at_asc_id_desc' do + describe '.order_last_used_at_asc_id_asc' do let_it_be(:earlier_token_2) { create(:personal_access_token, last_used_at: two_days_ago) } - it 'returns ordered list in combination of last_used_at ascending and id descending' do - expect(described_class.order_last_used_at_asc_id_desc).to eq [earlier_token_2, earlier_token, later_token] + it 'returns ordered list in combination of last_used_at ascending and id ascending' do + expect(described_class.order_last_used_at_asc_id_asc).to eq [earlier_token, earlier_token_2, later_token] end end diff --git a/spec/requests/groups/settings/access_tokens_controller_spec.rb b/spec/requests/groups/settings/access_tokens_controller_spec.rb index f65e5bcec77e4a..481c52628f0359 100644 --- a/spec/requests/groups/settings/access_tokens_controller_spec.rb +++ b/spec/requests/groups/settings/access_tokens_controller_spec.rb @@ -113,7 +113,7 @@ it 'includes details of the active group access tokens' do active_access_tokens = - ::GroupAccessTokenSerializer.new.represent(resource_access_tokens.reverse, group: resource) + ::GroupAccessTokenSerializer.new.represent(resource_access_tokens, group: resource) expect(assigns(:active_access_tokens).to_json).to eq(active_access_tokens.to_json) end diff --git a/spec/requests/projects/settings/access_tokens_controller_spec.rb b/spec/requests/projects/settings/access_tokens_controller_spec.rb index 68524c94d275cf..9621b506fe5a96 100644 --- a/spec/requests/projects/settings/access_tokens_controller_spec.rb +++ b/spec/requests/projects/settings/access_tokens_controller_spec.rb @@ -116,7 +116,7 @@ it 'includes details of the active project access tokens' do active_access_tokens = - ::ProjectAccessTokenSerializer.new.represent(resource_access_tokens.reverse, project: resource) + ::ProjectAccessTokenSerializer.new.represent(resource_access_tokens, project: resource) expect(assigns(:active_access_tokens).to_json).to eq(active_access_tokens.to_json) end diff --git a/spec/support/shared_examples/requests/access_tokens_controller_shared_examples.rb b/spec/support/shared_examples/requests/access_tokens_controller_shared_examples.rb index 003d73f93ec7f5..3b340f0356a87d 100644 --- a/spec/support/shared_examples/requests/access_tokens_controller_shared_examples.rb +++ b/spec/support/shared_examples/requests/access_tokens_controller_shared_examples.rb @@ -88,11 +88,11 @@ get_access_tokens first_token = assigns(:active_access_tokens).first.as_json - expect(first_token['name']).to eq("Token3") + expect(first_token['name']).to eq("Token1") expect(first_token['expires_at']).to eq(expires_1_day_from_now.iso8601) second_token = assigns(:active_access_tokens).second.as_json - expect(second_token['name']).to eq("Token1") + expect(second_token['name']).to eq("Token3") expect(second_token['expires_at']).to eq(expires_1_day_from_now.iso8601) end end -- GitLab