diff --git a/app/services/namespaces/groups/adjourned_deletion_service.rb b/app/services/namespaces/groups/adjourned_deletion_service.rb index be12b8973b518371e8b3b2d908050598696701de..3379953d0b2b54fe7fde5c1588b11857672c9f1b 100644 --- a/app/services/namespaces/groups/adjourned_deletion_service.rb +++ b/app/services/namespaces/groups/adjourned_deletion_service.rb @@ -29,7 +29,7 @@ def delete_group end def restore_group - admin_bot = ::Users::Internal.admin_bot + admin_bot = ::Users::Internal.for_organization(group.organization_id).admin_bot Gitlab::Auth::CurrentUserMode.optionally_run_in_admin_mode(admin_bot) do ::Groups::RestoreService.new(group, admin_bot).execute end diff --git a/app/services/projects/adjourned_deletion_service.rb b/app/services/projects/adjourned_deletion_service.rb index 20f57a8252ff6f2d2b95630d0b009b69c7f83f5e..51509a294cdcb60b9e86f97a0cee04c8e5253336 100644 --- a/app/services/projects/adjourned_deletion_service.rb +++ b/app/services/projects/adjourned_deletion_service.rb @@ -30,7 +30,7 @@ def delete_project end def restore_project - admin_bot = ::Users::Internal.admin_bot + admin_bot = ::Users::Internal.for_organization(project.organization_id).admin_bot Gitlab::Auth::CurrentUserMode.optionally_run_in_admin_mode(admin_bot) do ::Projects::RestoreService.new(project, admin_bot).execute end diff --git a/app/services/users/auto_ban_service.rb b/app/services/users/auto_ban_service.rb index 56bf31a3d7821cac3703870442c5ff263ea9d9ab..6d3dd5b07d0ecef2f25433bdbb58a08fd61c3803 100644 --- a/app/services/users/auto_ban_service.rb +++ b/app/services/users/auto_ban_service.rb @@ -32,7 +32,7 @@ def ban_user end def admin_bot - Users::Internal.admin_bot + Users::Internal.for_organization(user.organization_id).admin_bot end def ban_duplicate_users diff --git a/app/workers/projects/inactive_projects_deletion_cron_worker.rb b/app/workers/projects/inactive_projects_deletion_cron_worker.rb index 33145083db7006025ac48ebf5318942cacd0a4eb..802a7873ad4c30a6db4d2ed008e6171b7c48687b 100644 --- a/app/workers/projects/inactive_projects_deletion_cron_worker.rb +++ b/app/workers/projects/inactive_projects_deletion_cron_worker.rb @@ -22,9 +22,6 @@ def perform return unless ::Gitlab::CurrentSettings.delete_inactive_projects? @start_time ||= ::Gitlab::Metrics::System.monotonic_time - admin_bot = ::Users::Internal.admin_bot - - return unless admin_bot notified_inactive_projects = Gitlab::DormantProjectsDeletionWarningTracker.notified_projects @@ -39,15 +36,17 @@ def perform raise TimeoutError end - with_context(project: project, user: admin_bot) do + organization_admin_bot = admin_bot_for_organization_id(project.organization_id) + + with_context(project: project, user: organization_admin_bot) do deletion_warning_email_sent_on = notified_inactive_projects["project:#{project.id}"] if deletion_warning_email_sent_on.blank? send_notification(project) - log_audit_event(project, admin_bot) + log_audit_event(project, organization_admin_bot) elsif grace_period_is_over?(deletion_warning_email_sent_on) Gitlab::DormantProjectsDeletionWarningTracker.new(project.id).reset - delete_project(project, admin_bot) + delete_project(project, organization_admin_bot) end end end @@ -111,6 +110,11 @@ def reset_last_processed_project_id def with_redis(&block) Gitlab::Redis::Cache.with(&block) # rubocop:disable CodeReuse/ActiveRecord end + + def admin_bot_for_organization_id(organization_id) + @admin_bots ||= {} + @admin_bots[organization_id] ||= Users::Internal.for_organization(organization_id).admin_bot + end end end diff --git a/app/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker.rb b/app/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker.rb index a49db473afd820b51423ce274cf18e906556c830..2d93c1cf0c3bb72fd114e17d3d43c4342c3e06af 100644 --- a/app/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker.rb +++ b/app/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker.rb @@ -24,7 +24,7 @@ def perform(cursor = nil) User.project_bot.where('"users"."id" > ?', cursor || 0).each_batch(of: BATCH_SIZE) do |relation| project_bot_users_whose_all_tokens_became_inactive_before_cut_off_date_or_without_tokens = relation - .select(:id, :username) + .select(:id, :username, :organization_id) .where( 'NOT EXISTS (?)', PersonalAccessToken @@ -57,6 +57,7 @@ def initiate_deletion_for(users) DeleteUserWorker.bulk_perform_async_with_contexts( users, arguments_proc: ->(user) { + admin_bot_id = admin_bot_id_for_organization_id(user.organization_id) [ admin_bot_id, user.id, { skip_authorization: true, reason_for_deletion: "No active token assigned" } @@ -66,9 +67,9 @@ def initiate_deletion_for(users) ) end - def admin_bot_id - Users::Internal.admin_bot.id + def admin_bot_id_for_organization_id(organization_id) + @admin_bots ||= {} + @admin_bots[organization_id] ||= Users::Internal.for_organization(organization_id).admin_bot.id end - strong_memoize_attr :admin_bot_id end end diff --git a/app/workers/users/deactivate_dormant_users_worker.rb b/app/workers/users/deactivate_dormant_users_worker.rb index 5cd1d2938ee55bed005a9cae2bb671efa3230147..c0cc7992cdb6852b8984f5ead3ca708f7e7d1cf5 100644 --- a/app/workers/users/deactivate_dormant_users_worker.rb +++ b/app/workers/users/deactivate_dormant_users_worker.rb @@ -15,25 +15,32 @@ def perform return unless ::Gitlab::CurrentSettings.current_application_settings.deactivate_dormant_users - admin_bot = Users::Internal.admin_bot - return unless admin_bot - - Gitlab::Auth::CurrentUserMode.bypass_session!(admin_bot.id) do - deactivate_users(User.dormant, admin_bot) - deactivate_users(User.with_no_activity, admin_bot) - end + deactivate_users(User.dormant) + deactivate_users(User.with_no_activity) end private - def deactivate_users(scope, admin_bot) + def deactivate_users(scope) with_context(caller_id: self.class.name.to_s) do scope.each_batch do |batch| batch.each do |user| - Users::DeactivateService.new(admin_bot).execute(user) + process_user_deactivation(user) end end end end + + def process_user_deactivation(user) + admin_bot = admin_bot_for_organization_id(user.organization_id) + Gitlab::Auth::CurrentUserMode.bypass_session!(admin_bot.id) do + Users::DeactivateService.new(admin_bot).execute(user) + end + end + + def admin_bot_for_organization_id(organization_id) + @admin_bots ||= {} + @admin_bots[organization_id] ||= Users::Internal.for_organization(organization_id).admin_bot + end end end diff --git a/ee/app/models/compliance_management/pipl_user.rb b/ee/app/models/compliance_management/pipl_user.rb index dafbe74719ea836f462e4f1c091ef032e87e20fd..98fa74b8c11089a714fd0e5db034f005347628c8 100644 --- a/ee/app/models/compliance_management/pipl_user.rb +++ b/ee/app/models/compliance_management/pipl_user.rb @@ -38,7 +38,7 @@ class PiplUser < ApplicationRecord scope :pipl_deletable, -> do joins(:user) - .includes(:user) + .includes(user: :organization) .left_outer_joins(user: :ghost_user_migration) .where(ghost_user_migrations: { id: nil }) .where.not(state: "deletion_needs_to_be_reviewed") diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index e7243a29cc498a39858c6853a3dfeb96b340a417..696eaf87cacff3a91468c3faeccd17a423bd8d66 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -902,7 +902,7 @@ def audit_lock_access(reason: nil) ::Gitlab::Audit::Auditor.audit( name: 'user_access_locked', - author: ::Users::Internal.admin_bot, + author: ::Users::Internal.for_organization(organization_id).admin_bot, scope: self, target: self, message: ['User access locked', reason].compact.join(' - ') diff --git a/ee/app/workers/compliance_management/pipl/block_pipl_users_worker.rb b/ee/app/workers/compliance_management/pipl/block_pipl_users_worker.rb index 2c1b2a88a81d94c906f902967a1408d5c7e60c49..0693b1d90f2605b9110b49ba400c5a0ed39f78a0 100644 --- a/ee/app/workers/compliance_management/pipl/block_pipl_users_worker.rb +++ b/ee/app/workers/compliance_management/pipl/block_pipl_users_worker.rb @@ -14,13 +14,11 @@ class BlockPiplUsersWorker queue_namespace :cronjob def perform - Gitlab::Auth::CurrentUserMode.optionally_run_in_admin_mode(admin_bot) do - PiplUser.pipl_blockable.each_batch do |batch| - batch.each do |pipl_user| - with_context(user: pipl_user.user) do - # Ensure that admin mode doesn't break the authorization cycle - block_user(pipl_user) - end + PiplUser.pipl_blockable.each_batch do |batch| + batch.each do |pipl_user| + with_context(user: pipl_user.user) do + # Ensure that admin mode doesn't break the authorization cycle + block_user(pipl_user) end end end @@ -29,6 +27,14 @@ def perform private def block_user(pipl_user) + admin_bot = admin_bot_for_organization_id(pipl_user.user.organization_id) + + Gitlab::Auth::CurrentUserMode.optionally_run_in_admin_mode(admin_bot) do + process_block_user(pipl_user, admin_bot) + end + end + + def process_block_user(pipl_user, admin_bot) result = BlockNonCompliantUserService.new(pipl_user: pipl_user, current_user: admin_bot).execute return unless result.error? @@ -37,10 +43,10 @@ def block_user(pipl_user) .info(message: "Error blocking user: #{pipl_user.user.id} with message #{result.message}", jid: jid) end - def admin_bot - Users::Internal.admin_bot + def admin_bot_for_organization_id(organization_id) + @admin_bots ||= {} + @admin_bots[organization_id] ||= Users::Internal.for_organization(organization_id).admin_bot end - strong_memoize_attr :admin_bot end end end diff --git a/ee/app/workers/compliance_management/pipl/delete_pipl_users_worker.rb b/ee/app/workers/compliance_management/pipl/delete_pipl_users_worker.rb index ea430f9881b892b46b5b8fc5d160cee3075e9fa5..2280996b2e353133951fc4f6c859f153190de084 100644 --- a/ee/app/workers/compliance_management/pipl/delete_pipl_users_worker.rb +++ b/ee/app/workers/compliance_management/pipl/delete_pipl_users_worker.rb @@ -14,13 +14,11 @@ class DeletePiplUsersWorker queue_namespace :cronjob def perform - Gitlab::Auth::CurrentUserMode.optionally_run_in_admin_mode(admin_bot) do - PiplUser.pipl_deletable.each_batch do |batch| - batch.each do |pipl_user| - with_context(user: pipl_user.user) do - # Ensure that admin mode doesn't break the authorization cycle - delete_user(pipl_user) - end + PiplUser.pipl_deletable.each_batch do |batch| + batch.each do |pipl_user| + with_context(user: pipl_user.user) do + # Ensure that admin mode doesn't break the authorization cycle + delete_user(pipl_user) end end end @@ -29,6 +27,14 @@ def perform private def delete_user(pipl_user) + admin_bot = admin_bot_for_organization_id(pipl_user.user.organization_id) + + Gitlab::Auth::CurrentUserMode.optionally_run_in_admin_mode(admin_bot) do + process_delete_user(pipl_user, admin_bot) + end + end + + def process_delete_user(pipl_user, admin_bot) result = DeleteNonCompliantUserService.new(pipl_user: pipl_user, current_user: admin_bot).execute return unless result.error? @@ -37,10 +43,10 @@ def delete_user(pipl_user) .info(message: "Error blocking user: #{pipl_user.user.id} with message #{result.message}", jid: jid) end - def admin_bot - Users::Internal.admin_bot + def admin_bot_for_organization_id(organization_id) + @admin_bots ||= {} + @admin_bots[organization_id] ||= Users::Internal.for_organization(organization_id).admin_bot end - strong_memoize_attr :admin_bot end end end diff --git a/ee/app/workers/compliance_management/update_default_framework_worker.rb b/ee/app/workers/compliance_management/update_default_framework_worker.rb index 7935c5bbd53af98c7a46747a2131386be707e2b4..91f546b03f1d4370c18bcb30a67e4468ae147516 100644 --- a/ee/app/workers/compliance_management/update_default_framework_worker.rb +++ b/ee/app/workers/compliance_management/update_default_framework_worker.rb @@ -10,8 +10,8 @@ class UpdateDefaultFrameworkWorker feature_category :compliance_management def perform(_user_id, project_id, compliance_framework_id) - admin_bot = ::Users::Internal.admin_bot project = Project.find(project_id) + admin_bot = admin_bot_for_organization_id(project.organization_id) Gitlab::Auth::CurrentUserMode.bypass_session!(admin_bot.id) do ::ComplianceManagement::Frameworks::UpdateProjectService @@ -21,5 +21,12 @@ def perform(_user_id, project_id, compliance_framework_id) rescue ActiveRecord::RecordNotFound => e Gitlab::ErrorTracking.log_exception(e) end + + private + + def admin_bot_for_organization_id(organization_id) + @admin_bots ||= {} + @admin_bots[organization_id] ||= Users::Internal.for_organization(organization_id).admin_bot + end end end diff --git a/ee/app/workers/namespaces/remove_dormant_members_worker.rb b/ee/app/workers/namespaces/remove_dormant_members_worker.rb index cb4d32e51e6cbc1ee3358720f5f2650e08cf05fb..37b31ee55ef1bfd65ab5e3abd27f8a95b29526c9 100644 --- a/ee/app/workers/namespaces/remove_dormant_members_worker.rb +++ b/ee/app/workers/namespaces/remove_dormant_members_worker.rb @@ -57,7 +57,7 @@ def namespaces_requiring_dormant_member_removal(limit = 1) def remove_dormant_members(namespace) dormant_period = namespace.namespace_settings.remove_dormant_members_period.days.ago - admin_bot = ::Users::Internal.admin_bot + admin_bot = admin_bot_for_organization_id(namespace.organization_id) dormant_count = 0 ::GitlabSubscriptions::SeatAssignment.dormant_in_namespace(namespace, dormant_period).find_each do |assignment| @@ -88,5 +88,10 @@ def log_monitoring_data(namespace_id, dormant_count) dormant_count: dormant_count ) end + + def admin_bot_for_organization_id(organization_id) + @admin_bots ||= {} + @admin_bots[organization_id] ||= Users::Internal.for_organization(organization_id).admin_bot + end end end diff --git a/ee/app/workers/users/security_policy_bot_cleanup_cron_worker.rb b/ee/app/workers/users/security_policy_bot_cleanup_cron_worker.rb index 39962bed195f464cf18082bb5c5b353e418b2d90..96b495a0639aa8bd609ee572ffcc2c3c5ecb8790 100644 --- a/ee/app/workers/users/security_policy_bot_cleanup_cron_worker.rb +++ b/ee/app/workers/users/security_policy_bot_cleanup_cron_worker.rb @@ -15,10 +15,8 @@ class SecurityPolicyBotCleanupCronWorker def perform return unless Feature.enabled?(:security_policy_bot_cleanup_cron_worker, :instance) - admin_user = Users::Internal.admin_bot options = { skip_authorization: true, hard_delete: false, reason_for_deletion: "Security policy bot no longer associated with any project" } - destroy_service = Users::DestroyService.new(admin_user) users_to_ignore = [] MAX_BATCHES.times do @@ -27,6 +25,9 @@ def perform break if users.empty? users.each do |user| + admin_user = admin_bot_for_organization_id(user.organization_id) + destroy_service = Users::DestroyService.new(admin_user) + with_context(user: user) do destroy_service.execute(user, options) rescue Gitlab::Access::AccessDeniedError, Users::DestroyService::DestroyError => e @@ -36,5 +37,12 @@ def perform end end end + + private + + def admin_bot_for_organization_id(organization_id) + @admin_bots ||= {} + @admin_bots[organization_id] ||= Users::Internal.for_organization(organization_id).admin_bot + end end end diff --git a/ee/app/workers/users/unconfirmed_users_deletion_cron_worker.rb b/ee/app/workers/users/unconfirmed_users_deletion_cron_worker.rb index c26c7ab61dcc664da82464ab0ec9ee1aa32954f7..5a81e1ec9269648c62157a405f3c9dc30e3f6fc3 100644 --- a/ee/app/workers/users/unconfirmed_users_deletion_cron_worker.rb +++ b/ee/app/workers/users/unconfirmed_users_deletion_cron_worker.rb @@ -18,7 +18,6 @@ def perform return if ::Gitlab::CurrentSettings.email_confirmation_setting_off? return unless ::Gitlab::CurrentSettings.delete_unconfirmed_users? return unless License.feature_available?(:delete_unconfirmed_users) - return unless admin_bot_id in_lock(self.class.name.underscore, ttl: Gitlab::Utils::ExecutionTracker::MAX_RUNTIME, retries: 0) do order = Gitlab::Pagination::Keyset::Order.build([ @@ -33,7 +32,11 @@ def perform ) ]) - users = User.unconfirmed_and_created_before(cut_off).select(:created_at, :id, :username).order(order) # rubocop: disable CodeReuse/ActiveRecord + # rubocop:disable CodeReuse/ActiveRecord + users = User.unconfirmed_and_created_before(cut_off) + .select(:created_at, :id, :organization_id, :username) + .order(order) + # rubocop:enable CodeReuse/ActiveRecord delete_users(scope: users) end end @@ -51,7 +54,8 @@ def delete_users(scope:) DeleteUserWorker.bulk_perform_async_with_contexts( relation, arguments_proc: ->(user) { - [admin_bot_id, user.id, { skip_authorization: true, reason_for_deletion: reason_for_deletion }] + admin_bot = admin_bot_for_organization_id(user.organization_id) + [admin_bot.id, user.id, { skip_authorization: true, reason_for_deletion: reason_for_deletion }] }, context_proc: ->(user) { { user: user } } ) @@ -63,15 +67,15 @@ def cut_off end strong_memoize_attr :cut_off - def admin_bot_id - Users::Internal.admin_bot&.id - end - strong_memoize_attr :admin_bot_id - def reason_for_deletion "GitLab automatically deletes unconfirmed users after " \ "#{::Gitlab::CurrentSettings.unconfirmed_users_delete_after_days} days since their creation" end strong_memoize_attr :reason_for_deletion + + def admin_bot_for_organization_id(organization_id) + @admin_bots ||= {} + @admin_bots[organization_id] ||= Users::Internal.for_organization(organization_id).admin_bot + end end end diff --git a/ee/lib/ee/gitlab/background_migration/mark_admin_bot_runners_as_hosted.rb b/ee/lib/ee/gitlab/background_migration/mark_admin_bot_runners_as_hosted.rb index 3332ec1e8d7aa99aaa7f44bfc292cff569c46c0e..13ae78706027416b7dbb55456978727557a60507 100644 --- a/ee/lib/ee/gitlab/background_migration/mark_admin_bot_runners_as_hosted.rb +++ b/ee/lib/ee/gitlab/background_migration/mark_admin_bot_runners_as_hosted.rb @@ -18,7 +18,7 @@ def perform each_sub_batch do |sub_batch| sub_batch.each do |runner| - next unless runner.creator_id == admin_bot.id + next unless ::User.admin_bot.where(id: runner.creator_id).exists? CiHostedRunner.find_or_create_by!(runner_id: runner.id) end @@ -32,12 +32,6 @@ class ApplicationSetting < ApplicationRecord class CiHostedRunner < ::Ci::ApplicationRecord self.table_name = :ci_hosted_runners end - - private - - def admin_bot - @_admin_bot ||= ::Users::Internal.admin_bot - end end end end diff --git a/ee/spec/lib/ee/gitlab/background_migration/mark_admin_bot_runners_as_hosted_spec.rb b/ee/spec/lib/ee/gitlab/background_migration/mark_admin_bot_runners_as_hosted_spec.rb index 4dd7a2113e5f9a1764c3a14a8a1f89d16ea1dad9..2d4ad0c71b3a87a67c4f2a71257775d1c6d98a20 100644 --- a/ee/spec/lib/ee/gitlab/background_migration/mark_admin_bot_runners_as_hosted_spec.rb +++ b/ee/spec/lib/ee/gitlab/background_migration/mark_admin_bot_runners_as_hosted_spec.rb @@ -4,7 +4,7 @@ RSpec.describe Gitlab::BackgroundMigration::MarkAdminBotRunnersAsHosted, feature_category: :hosted_runners do - let!(:admin_bot) { Users::Internal.admin_bot } + let!(:admin_bot) { ::Users::Internal.for_organization(::Organizations::Organization.first).admin_bot } let!(:admin_bot_runner) do table(:ci_runners, database: :ci, primary_key: :id) diff --git a/ee/spec/models/ee/ci/runner_spec.rb b/ee/spec/models/ee/ci/runner_spec.rb index 5a6550419d6876c4c15db199a9b168c1f1e6efcc..a234217c814b180844f921654431021669c4e48b 100644 --- a/ee/spec/models/ee/ci/runner_spec.rb +++ b/ee/spec/models/ee/ci/runner_spec.rb @@ -5,6 +5,7 @@ RSpec.describe Ci::Runner, feature_category: :hosted_runners do let_it_be(:namespace) { create(:group, created_at: Date.new(2021, 7, 16)) } let_it_be(:project) { create(:project) } + let_it_be(:admin_bot) { create(:user, :admin_bot) } let(:shared_runners_minutes) { 400 } @@ -42,7 +43,7 @@ end context 'with an admin bot created runner' do - let_it_be(:runner) { create(:ci_runner, creator: Users::Internal.admin_bot) } + let_it_be(:runner) { create(:ci_runner, creator: admin_bot) } it 'returns true' do expect(runner.dedicated_gitlab_hosted?).to be_truthy @@ -67,7 +68,7 @@ end context 'when not on dedicated installation' do - let_it_be(:runner) { create(:ci_runner, creator: Users::Internal.admin_bot) } + let_it_be(:runner) { create(:ci_runner, creator: admin_bot) } before do allow(Gitlab::CurrentSettings).to receive(:gitlab_dedicated_instance?).and_return(false) diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index e3bec411bc2fad90041b315c4f08dee2ada65982..f14a988b98da935f8c10f141fdd02f1850fd67a8 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -3500,8 +3500,8 @@ end describe '#lock_access!' do - let_it_be(:gitlab_admin_bot) { Users::Internal.admin_bot } let_it_be_with_reload(:user) { create(:user) } + let_it_be(:gitlab_admin_bot) { Users::Internal.for_organization(user.organization).admin_bot } subject { user.lock_access! } diff --git a/ee/spec/policies/compliance_management/pipl_user_policy_spec.rb b/ee/spec/policies/compliance_management/pipl_user_policy_spec.rb index 75007bd62fcaa3a1ce81f391da2beab3d09cd8b0..c101a9ced4e62c874c563dfe210454b9c72cb201 100644 --- a/ee/spec/policies/compliance_management/pipl_user_policy_spec.rb +++ b/ee/spec/policies/compliance_management/pipl_user_policy_spec.rb @@ -6,7 +6,7 @@ let_it_be(:pipl_user) { create(:pipl_user) } let_it_be(:simple_user) { create(:user) } - let_it_be(:admin_user) { Users::Internal.admin_bot } + let_it_be(:admin_user) { create(:user, :admin_bot) } let(:current_user) { admin_user } subject { described_class.new(current_user, pipl_user) } diff --git a/ee/spec/services/compliance_management/pipl/block_non_compliant_user_service_spec.rb b/ee/spec/services/compliance_management/pipl/block_non_compliant_user_service_spec.rb index e10ead806cfa87ca218dcafc33627611153510e2..95f584bbfb4f29778345fa2fb786afb9600c86ef 100644 --- a/ee/spec/services/compliance_management/pipl/block_non_compliant_user_service_spec.rb +++ b/ee/spec/services/compliance_management/pipl/block_non_compliant_user_service_spec.rb @@ -94,7 +94,7 @@ end context 'when the block operation fails' do - let(:pipl_user) { create(:pipl_user, user: Users::Internal.admin_bot, initial_email_sent_at: 60.days.ago) } + let(:pipl_user) { create(:pipl_user, user: create(:user, :admin_bot), initial_email_sent_at: 60.days.ago) } it_behaves_like 'does not block the user' it_behaves_like 'has a validation error', diff --git a/ee/spec/workers/compliance_management/update_default_framework_worker_spec.rb b/ee/spec/workers/compliance_management/update_default_framework_worker_spec.rb index 3c3bb566b96c3933b3643ffeb0a3ffcc53b03985..ab7610e0d07644983c40d89402984300f81e9c8e 100644 --- a/ee/spec/workers/compliance_management/update_default_framework_worker_spec.rb +++ b/ee/spec/workers/compliance_management/update_default_framework_worker_spec.rb @@ -5,10 +5,10 @@ RSpec.describe ComplianceManagement::UpdateDefaultFrameworkWorker, feature_category: :compliance_management do let_it_be(:worker) { described_class.new } let_it_be(:user) { create(:user) } - let_it_be(:admin_bot) { create(:user, :admin_bot, :admin) } let_it_be(:group) { create(:group) } let_it_be_with_reload(:project) { create(:project, namespace: group) } let_it_be(:framework) { create(:compliance_framework, namespace: group, name: 'GDPR') } + let_it_be(:admin_bot) { create(:user, :admin_bot, organization: project.organization) } let(:job_args) { [user.id, project.id, framework.id] } diff --git a/ee/spec/workers/ee/projects/inactive_projects_deletion_cron_worker_spec.rb b/ee/spec/workers/ee/projects/inactive_projects_deletion_cron_worker_spec.rb index 99d590b8fb27172232271f2566d11afdf7a7bf3e..eaa2d7b22f9911ff6728154c2130f27cb2aba28f 100644 --- a/ee/spec/workers/ee/projects/inactive_projects_deletion_cron_worker_spec.rb +++ b/ee/spec/workers/ee/projects/inactive_projects_deletion_cron_worker_spec.rb @@ -8,7 +8,6 @@ describe "#perform", :clean_gitlab_redis_shared_state, :sidekiq_inline do subject(:worker) { described_class.new } - let_it_be(:admin_bot) { ::Users::Internal.admin_bot } let_it_be(:non_admin_user) { create(:user) } let_it_be(:group) { create(:group) } let_it_be(:new_blank_project) do @@ -33,6 +32,8 @@ .tap { |project| project.update!(last_activity_at: 1.month.ago) } end + let_it_be(:admin_bot) { ::Users::Internal.for_organization(new_blank_project.organization).admin_bot } + let_it_be(:delay) { anything } before do diff --git a/ee/spec/workers/namespaces/remove_dormant_members_worker_spec.rb b/ee/spec/workers/namespaces/remove_dormant_members_worker_spec.rb index 7e4554e1b693b265f7a1b67173a943987ee962a1..db6b2c22352d6e24461eb33a327ac9967a64630d 100644 --- a/ee/spec/workers/namespaces/remove_dormant_members_worker_spec.rb +++ b/ee/spec/workers/namespaces/remove_dormant_members_worker_spec.rb @@ -114,7 +114,9 @@ end include_examples 'audit event logging' do - let_it_be(:admin) { create(:admin) } + let_it_be(:admin) do + create(:user, :admin_bot, organization: dormant_enterprise_user.organization) + end let(:operation) { perform_work } @@ -142,7 +144,11 @@ end before do - allow(::Users::Internal).to receive(:admin_bot).and_return(admin) + # this has to be stubbed, otherwise the next_found_instance_of stub above + # will apply to admin_bot instead of the expected test user + allow_next_instance_of(::Users::Internal) do |internal| + allow(internal).to receive(:admin_bot).and_return(admin) + end end end end diff --git a/ee/spec/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker_spec.rb b/ee/spec/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker_spec.rb index 6143a17c2697352bec25d1657ae00ed17cdca1bc..821e0d017f3d6c9c2fd3015902d7c2c842de7eef 100644 --- a/ee/spec/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker_spec.rb +++ b/ee/spec/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker_spec.rb @@ -26,7 +26,7 @@ audit_event = AuditEvent.where("details LIKE ?", "%user_destroyed%").last - author = Users::Internal.admin_bot + author = Users::Internal.for_organization(resource.organization).admin_bot expect(audit_event.author_id).to eq(author.id) expect(audit_event.entity).to eq(resource) expect(audit_event.details).to eq({ diff --git a/ee/spec/workers/users/security_policy_bot_cleanup_cron_worker_spec.rb b/ee/spec/workers/users/security_policy_bot_cleanup_cron_worker_spec.rb index e38000c86cd3c6529d5720d24356effd440eaf29..416c317435ea302691faf7ed2e32ff541b72b0fd 100644 --- a/ee/spec/workers/users/security_policy_bot_cleanup_cron_worker_spec.rb +++ b/ee/spec/workers/users/security_policy_bot_cleanup_cron_worker_spec.rb @@ -17,11 +17,15 @@ } end + let(:destroy_service_double) { Users::DestroyService.new(admin_user) } + + before do + allow(Users::DestroyService).to receive(:new).and_return(destroy_service_double) + end + context 'with no security policy bots' do it 'does not delete any users' do - expect_next_instance_of(Users::DestroyService) do |service| - expect(service).not_to receive(:execute) - end + expect(Users::DestroyService).not_to receive(:new) perform end @@ -36,9 +40,7 @@ end it 'does not delete users with project memberships' do - expect_next_instance_of(Users::DestroyService) do |service| - expect(service).not_to receive(:execute) - end + expect(Users::DestroyService).not_to receive(:new) perform end @@ -57,10 +59,8 @@ end it 'deletes security policy bots without project memberships' do - expect_next_instance_of(Users::DestroyService) do |service| - expect(service).to receive(:execute).with(security_policy_bot_1, expected_service_options) - expect(service).to receive(:execute).with(security_policy_bot_2, expected_service_options) - end + expect(destroy_service_double).to receive(:execute).with(security_policy_bot_1, expected_service_options) + expect(destroy_service_double).to receive(:execute).with(security_policy_bot_2, expected_service_options) perform end @@ -76,10 +76,8 @@ end it 'only deletes security policy bots without project memberships' do - expect_next_instance_of(Users::DestroyService) do |service| - expect(service).to receive(:execute).with(security_policy_bot_without_membership, - expected_service_options).and_call_original - end + expect(destroy_service_double).to receive(:execute).with(security_policy_bot_without_membership, + expected_service_options).and_call_original perform end @@ -103,9 +101,9 @@ let_it_be(:ghost_user_migration) { create(:ghost_user_migration, user: ghost_security_policy_bot) } it 'does not attempt to delete security policy bots with ghost user migration' do - expect_next_instance_of(Users::DestroyService) do |service| - expect(service).to receive(:execute).with(security_policy_bot, expected_service_options).and_call_original - end + expect(destroy_service_double).to receive(:execute).with( + security_policy_bot, expected_service_options + ).and_call_original perform end @@ -120,9 +118,7 @@ end it 'processes up to 2 users in a single execution' do - expect_next_instance_of(Users::DestroyService) do |service| - expect(service).to receive(:execute).twice - end + expect(destroy_service_double).to receive(:execute).twice perform end @@ -133,14 +129,12 @@ let_it_be(:security_policy_bot_2) { create(:user, :security_policy_bot) } before do - allow_next_instance_of(Users::DestroyService) do |service| - allow(service).to receive(:execute).and_call_original - allow(service).to( - receive(:execute) - .with(security_policy_bot_1, expected_service_options) - .and_raise(error_class, 'Service error') - ) - end + allow(destroy_service_double).to receive(:execute).and_call_original + allow(destroy_service_double).to( + receive(:execute) + .with(security_policy_bot_1, expected_service_options) + .and_raise(error_class, 'Service error') + ) end shared_examples 'tracking error and continues processing' do diff --git a/ee/spec/workers/users/unconfirmed_users_deletion_cron_worker_spec.rb b/ee/spec/workers/users/unconfirmed_users_deletion_cron_worker_spec.rb index d2e7d8ba7008428b16b9160f44f404d6e4156a66..a13b23b919f3e9ac3a65a56fa48bd37d3ac59b11 100644 --- a/ee/spec/workers/users/unconfirmed_users_deletion_cron_worker_spec.rb +++ b/ee/spec/workers/users/unconfirmed_users_deletion_cron_worker_spec.rb @@ -10,7 +10,7 @@ let_it_be(:confirmed_user) { create(:user, created_at: cut_off_datetime - 1.day) } let_it_be(:banned_user) { create(:user, :banned, :unconfirmed, created_at: cut_off_datetime - 1.day) } let_it_be(:bot_user) { create(:user, :bot, :unconfirmed, created_at: cut_off_datetime - 1.day) } - let_it_be(:admin_bot) { ::Users::Internal.admin_bot } + let_it_be(:admin_bot) { create(:user, :admin_bot) } let_it_be(:unconfirmed_user_created_after_cut_off) do create(:user, :unconfirmed, created_at: cut_off_datetime + 1.day) @@ -30,7 +30,6 @@ end it 'destroys unconfirmed users who never signed in & signed up before unconfirmed_users_delete_after_days days' do - admin_bot = ::Users::Internal.admin_bot users_to_keep = [ unconfirmed_user_created_after_cut_off, unconfirmed_user_who_signed_in, diff --git a/lib/gitlab/background_migration/delete_orphaned_groups.rb b/lib/gitlab/background_migration/delete_orphaned_groups.rb index 428867ad400257d6a91f48df8eba301b6b5f0e42..e712551510db9e79aa2e75464a5a89e64cf4805b 100644 --- a/lib/gitlab/background_migration/delete_orphaned_groups.rb +++ b/lib/gitlab/background_migration/delete_orphaned_groups.rb @@ -14,16 +14,15 @@ def perform .joins("LEFT JOIN namespaces AS parent ON namespaces.parent_id = parent.id") .where(parent: { id: nil }) .pluck(:id).each do |orphaned_group_id| + organization_id = Group.where(id: orphaned_group_id).includes(:organization).first&.organization_id + + # if organization is nil, returns the default organization admin_bot + admin_bot = Users::Internal.for_organization(organization_id).admin_bot + ::GroupDestroyWorker.perform_async(orphaned_group_id, admin_bot.id) end end end - - private - - def admin_bot - @_admin_bot ||= Users::Internal.admin_bot - end end end end diff --git a/lib/tasks/ci/allowlist_migration.rake b/lib/tasks/ci/allowlist_migration.rake index 5bd7ad540090c99a756acd04f33afd32e26c4005..98ab03075f33e0edfd822906120a679555ebe91b 100644 --- a/lib/tasks/ci/allowlist_migration.rake +++ b/lib/tasks/ci/allowlist_migration.rake @@ -15,7 +15,7 @@ namespace :ci do task = ::Ci::JobToken::AllowlistMigrationTask.new(only_ids: only_ids, exclude_ids: exclude_ids, preview: preview, - user: ::Users::Internal.admin_bot, + user: ::Users::Internal.for_organization(::Organizations::Organization.first).admin_bot, concurrency: concurrency.to_i) task.execute diff --git a/lib/users/internal.rb b/lib/users/internal.rb index bee09537f865979395a19991d505276ba5359d11..0a89b62315ce645032adfc3d2cf49cad4795bdf1 100644 --- a/lib/users/internal.rb +++ b/lib/users/internal.rb @@ -32,7 +32,13 @@ def support_bot_id # rubocop:disable CodeReuse/ActiveRecord -- Need to instantiate a record here def initialize(organization: nil) - @organization = organization + case organization + when ::Organizations::Organization + @organization = organization + @organization_id = organization.id + else + @organization_id = organization + end end # Return (create if necessary) the ghost user. The ghost user @@ -153,28 +159,35 @@ def bot_avatar(image:) private + def organization + return @organization if @organization + + Organizations::Organization.find_by_id(@organization_id) + end + strong_memoize_attr :organization + # NOTE: This method is patched in spec/spec_helper.rb to allow use of exclusive lease in RSpec's # :before_all scope to keep the specs DRY. def unique_internal(scope, username, email_pattern, &block) - if @organization && organization_users_internal_enabled? - scope = scope.joins(:organization_users).where(organization_users: { organization: @organization }) + if @organization_id && organization_users_internal_enabled? # rubocop:disable Style/IfUnlessModifier -- exceeds line length + scope = scope.where(organization_id: @organization_id) end scope.first || create_unique_internal(scope, username, email_pattern, &block) end def username_with_organization_suffix(username) - return username if @organization.nil? || @organization == first_organization + return username if organization.nil? || organization == first_organization return username unless organization_users_internal_enabled? - [username, @organization.path].join('_') + [username, organization.path].join('_') end def display_name_with_organization_suffix(display_name) - return display_name if @organization.nil? || @organization == first_organization + return display_name if organization.nil? || organization == first_organization return display_name unless organization_users_internal_enabled? - "#{display_name} (#{@organization.name})" + "#{display_name} (#{organization.name})" end def first_organization @@ -183,15 +196,15 @@ def first_organization strong_memoize_attr :first_organization def organization_users_internal_enabled? - Feature.enabled?(:organization_users_internal, @organization) + Feature.enabled?(:organization_users_internal, ::Organizations::Organization.actor_from_id(@organization_id)) end strong_memoize_attr :organization_users_internal_enabled? def create_unique_internal(scope, username, email_pattern, &creation_block) # Since we only want a single one of these in an instance, we use an # exclusive lease to ensure than this block is never run concurrently. - lease_key = if @organization - "user:unique_internal:#{@organization.id}:#{username}" + lease_key = if organization + "user:unique_internal:#{@organization_id}:#{username}" else "user:unique_internal:#{username}" end @@ -227,8 +240,8 @@ def create_unique_internal(scope, username, email_pattern, &creation_block) &creation_block ) - user_organization = if @organization && organization_users_internal_enabled? - @organization + user_organization = if organization && organization_users_internal_enabled? + organization else Organizations::Organization.first end diff --git a/spec/factories/ci/runners.rb b/spec/factories/ci/runners.rb index 35ccb1b2f6ed2fc0ddc69e908b9f89d437d4c0f2..bf6223f60ba82d918a6d06b16bd4a435356015a3 100644 --- a/spec/factories/ci/runners.rb +++ b/spec/factories/ci/runners.rb @@ -161,7 +161,7 @@ end trait :hosted_runner do - creator { Users::Internal.admin_bot } + creator { Users::Internal.for_organization(organization_id).admin_bot } end end end diff --git a/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb b/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb index 4d34cb55602792a32ce3add06ddbae0617c4b7a2..f0f39a87acaedb7c9fee503bfe9a4267413a5810 100644 --- a/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb +++ b/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb @@ -8,7 +8,7 @@ let(:organization) { organizations.create!(name: 'Foobar', path: 'path1') } let!(:parent) { namespaces.create!(name: 'Group', type: 'Group', path: 'space1', organization_id: organization.id) } let!(:group) { namespaces.create!(name: 'GitLab', type: 'Group', path: 'group1', organization_id: organization.id) } - let!(:admin_bot) { ::Users::Internal.admin_bot } + let!(:admin_bot) { ::Users::Internal.for_organization(organization).admin_bot } let!(:orphaned_groups) do (1..4).map do |i| namespaces.create!( diff --git a/spec/lib/users/internal_spec.rb b/spec/lib/users/internal_spec.rb index 84d5e274799003377395c93bebe4be0ee2b514ee..62d5b734cbadf94b7020d283625a89983dc955e1 100644 --- a/spec/lib/users/internal_spec.rb +++ b/spec/lib/users/internal_spec.rb @@ -250,6 +250,30 @@ end end + context 'when organization_id is passed instead of organization' do + let_it_be(:organization_id) { organization.id } + + subject(:get_admin_bot) { described_class.for_organization(organization_id).admin_bot } + + context 'and bot exists' do + let_it_be(:admin_bot) { create(:user, :admin_bot, organization: organization) } + + it 'does not load full organization' do + expect(::Organizations::Organization).not_to receive(:find_by_id) + + expect(get_admin_bot).to eq(admin_bot) + end + end + + context 'and bot does not yet exist' do + it 'loads full organization to create new user' do + expect(::Organizations::Organization).to receive(:find_by_id).and_call_original + + expect(get_admin_bot).to be_admin_bot + end + end + end + context 'when organization is not used' do it 'creates user in first organization' do bot = described_class.support_bot diff --git a/spec/models/ci/job_token/allowlist_migration_task_spec.rb b/spec/models/ci/job_token/allowlist_migration_task_spec.rb index b49302016cfac104eea2fc2d901091fb00714893..995c6619bdcb001f9e791b0840f67b68e5bbe2ce 100644 --- a/spec/models/ci/job_token/allowlist_migration_task_spec.rb +++ b/spec/models/ci/job_token/allowlist_migration_task_spec.rb @@ -8,7 +8,7 @@ let(:preview) { nil } let(:output_stream) { StringIO.new } let(:path) { Rails.root.join('tmp/tests/doc/ci/jobs') } - let(:user) { ::Users::Internal.admin_bot } + let(:user) { create(:user, :admin_bot) } let(:concurrency) { 4 } let(:task) do diff --git a/spec/policies/base_policy_spec.rb b/spec/policies/base_policy_spec.rb index 018ec0a80e3956ec5583d7557e19e4585d4de863..060ab3dcf21b97a50d982c652c790003e2b3bd59 100644 --- a/spec/policies/base_policy_spec.rb +++ b/spec/policies/base_policy_spec.rb @@ -66,7 +66,7 @@ def policy end context 'with the admin bot user' do - let(:current_user) { ::Users::Internal.admin_bot } + let(:current_user) { create(:user, :admin_bot) } it { is_expected.to be_allowed(ability) } end diff --git a/spec/services/members/schedule_deletion_service_spec.rb b/spec/services/members/schedule_deletion_service_spec.rb index b84ef2c15ab2a6de2d56b52e6f4409bf3b5162a1..d254d1ddc73fc1408bd33baf3f83d93daa379ca7 100644 --- a/spec/services/members/schedule_deletion_service_spec.rb +++ b/spec/services/members/schedule_deletion_service_spec.rb @@ -81,7 +81,7 @@ end context 'when the user is an admin bot', :enable_admin_mode do - let_it_be(:scheduled_by) { Users::Internal.admin_bot } + let_it_be(:scheduled_by) { create(:user, :admin_bot) } it 'creates a deletion schedule' do result = service.execute diff --git a/spec/services/projects/import_export/relation_import_service_spec.rb b/spec/services/projects/import_export/relation_import_service_spec.rb index a115daa9c0bbeea398e0f15bff5e91d8282e15e4..f9cec9cb04b164c87a961bcbcfc6d5123e063e7e 100644 --- a/spec/services/projects/import_export/relation_import_service_spec.rb +++ b/spec/services/projects/import_export/relation_import_service_spec.rb @@ -71,7 +71,7 @@ end context 'and the user is an admin bot' do - let_it_be(:user) { Users::Internal.admin_bot } + let_it_be(:user) { create(:user, :admin_bot) } include_examples 'returns success response' end diff --git a/spec/tasks/ci/allowlist_migration_rake_spec.rb b/spec/tasks/ci/allowlist_migration_rake_spec.rb index 00bb8f4c477fa2b65b52101b23479a674dce4fcf..09b8fdd02c9949687972e473eb350a59c1d21ac1 100644 --- a/spec/tasks/ci/allowlist_migration_rake_spec.rb +++ b/spec/tasks/ci/allowlist_migration_rake_spec.rb @@ -4,7 +4,7 @@ RSpec.describe 'ci:job_tokens:allowlist rake tasks', feature_category: :secrets_management do let(:task_class) { ::Ci::JobToken::AllowlistMigrationTask } - let(:user) { ::Users::Internal.admin_bot } + let(:user) { create(:user, :admin_bot) } before do Rake.application.rake_require('tasks/ci/allowlist_migration') diff --git a/spec/workers/projects/inactive_projects_deletion_cron_worker_spec.rb b/spec/workers/projects/inactive_projects_deletion_cron_worker_spec.rb index 20eb9fb036869aeb46fce3186d0040cc0213ce20..b54ae1714f4cf76c40fae706b9336af7dc6af6e0 100644 --- a/spec/workers/projects/inactive_projects_deletion_cron_worker_spec.rb +++ b/spec/workers/projects/inactive_projects_deletion_cron_worker_spec.rb @@ -36,7 +36,6 @@ describe "#perform" do subject(:worker) { described_class.new } - let_it_be(:admin_bot) { ::Users::Internal.admin_bot } let_it_be(:non_admin_user) { create(:user) } let_it_be(:new_blank_project) do create_project_with_statistics.tap do |project| @@ -44,6 +43,8 @@ end end + let_it_be(:admin_bot) { ::Users::Internal.for_organization(new_blank_project.organization).admin_bot } + let_it_be(:inactive_blank_project) do create_project_with_statistics.tap do |project| project.update!(last_activity_at: 13.months.ago) diff --git a/spec/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker_spec.rb b/spec/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker_spec.rb index 2cfed36a350b63f9828987a8050e79a8d508edaf..4eef8e1034817c344982ffa3ed916abb3510daec 100644 --- a/spec/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker_spec.rb +++ b/spec/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker_spec.rb @@ -23,8 +23,7 @@ :freeze_time, :sidekiq_inline ) do - cut_off = cut_off_days.days.ago - admin_bot = Users::Internal.admin_bot + cut_off = Gitlab::CurrentSettings.inactive_resource_access_tokens_delete_after_days.days.ago active_personal_access_token = create(:personal_access_token) @@ -141,6 +140,7 @@ worker.perform users_to_keep.each do |user| + admin_bot = Users::Internal.for_organization(user.organization).admin_bot expect( Users::GhostUserMigration.find_by( user: user, @@ -150,6 +150,7 @@ end users_to_delete.each do |user| + admin_bot = Users::Internal.for_organization(user.organization).admin_bot expect( Users::GhostUserMigration.find_by( user: user,