From 3bce060cbf076b00fcbf068f14d53f27cf39e20b Mon Sep 17 00:00:00 2001
From: Alper Akgun <aakgun@gitlab.com>
Date: Tue, 5 Dec 2023 12:30:04 +0000
Subject: [PATCH] Merge branch 'codyw-fix-dormantuser-cron' into 'master'
Fix Admin Mode bug in DeactivateDormantUsersWorker
See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/136907
Merged-by: Alper Akgun <aakgun@gitlab.com>
Approved-by: Josianne Hyson <jhyson@gitlab.com>
Approved-by: Smriti Garg <sgarg@gitlab.com>
Reviewed-by: Josianne Hyson <jhyson@gitlab.com>
Reviewed-by: Huzaifa Iftikhar <hiftikhar@gitlab.com>
Co-authored-by: Cody West <cwest@gitlab.com>
Co-authored-by: Josianne Hyson <jhyson@gitlab.com>
(cherry picked from commit 460c393db3e558176cd0af3ad71782d8e93a2628)
c889133c Skip DeactivateService authorization in cron
e9f51558 Switch to session bypass
0db4f4bc Add admin mode user deactivation tests
5f3e44ad Call original for better testing
67d4689b Remove authorization skip
efda2a7a Apply review suggestions
---
.../users/deactivate_dormant_users_worker.rb | 6 +-
.../deactivate_dormant_users_worker_spec.rb | 91 +++++++++++++------
2 files changed, 65 insertions(+), 32 deletions(-)
diff --git a/app/workers/users/deactivate_dormant_users_worker.rb b/app/workers/users/deactivate_dormant_users_worker.rb
index 33c54f07521b..5cd1d2938ee5 100644
--- a/app/workers/users/deactivate_dormant_users_worker.rb
+++ b/app/workers/users/deactivate_dormant_users_worker.rb
@@ -18,8 +18,10 @@ def perform
admin_bot = Users::Internal.admin_bot
return unless admin_bot
- deactivate_users(User.dormant, admin_bot)
- deactivate_users(User.with_no_activity, 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
end
private
diff --git a/spec/workers/users/deactivate_dormant_users_worker_spec.rb b/spec/workers/users/deactivate_dormant_users_worker_spec.rb
index c28be165fd71..574dc922a363 100644
--- a/spec/workers/users/deactivate_dormant_users_worker_spec.rb
+++ b/spec/workers/users/deactivate_dormant_users_worker_spec.rb
@@ -10,34 +10,27 @@
let_it_be(:inactive) { create(:user, last_activity_on: nil, created_at: User::MINIMUM_DAYS_CREATED.days.ago.to_date) }
let_it_be(:inactive_recently_created) { create(:user, last_activity_on: nil, created_at: (User::MINIMUM_DAYS_CREATED - 1).days.ago.to_date) }
- let(:admin_bot) { create(:user, :admin_bot) }
- let(:deactivation_service) { instance_spy(Users::DeactivateService) }
-
- before do
- allow(Users::DeactivateService).to receive(:new).and_return(deactivation_service)
- end
-
subject(:worker) { described_class.new }
it 'does not run for SaaS', :saas do
- # Now makes a call to current settings to determine period of dormancy
-
worker.perform
- expect(deactivation_service).not_to have_received(:execute)
- end
-
- context 'when automatic deactivation of dormant users is enabled' do
- before do
- stub_application_setting(deactivate_dormant_users: true)
+ expect_any_instance_of(::Users::DeactivateService) do |deactivation_service|
+ expect(deactivation_service).not_to have_received(:execute)
end
+ end
- it 'deactivates dormant users' do
- worker.perform
-
- expect(deactivation_service).to have_received(:execute).twice
+ shared_examples 'deactivates dormant users' do
+ specify do
+ expect { worker.perform }
+ .to change { dormant.reload.state }
+ .to('deactivated')
+ .and change { inactive.reload.state }
+ .to('deactivated')
end
+ end
+ shared_examples 'deactivates certain user types' do
where(:user_type, :expected_state) do
:human | 'deactivated'
:support_bot | 'active'
@@ -52,33 +45,69 @@
end
with_them do
- it 'deactivates certain user types' do
+ specify do
user = create(:user, user_type: user_type, state: :active, last_activity_on: Gitlab::CurrentSettings.deactivate_dormant_users_period.days.ago.to_date)
worker.perform
- if expected_state == 'deactivated'
- expect(deactivation_service).to have_received(:execute).with(user)
- else
- expect(deactivation_service).not_to have_received(:execute).with(user)
+ expect_any_instance_of(::Users::DeactivateService) do |deactivation_service|
+ if expected_state == 'deactivated'
+ expect(deactivation_service).to receive(:execute).with(user).and_call_original
+ else
+ expect(deactivation_service).not_to have_received(:execute).with(user)
+ end
end
+
+ expect(user.reload.state).to eq expected_state
end
end
+ end
- it 'does not deactivate non-active users' do
+ shared_examples 'does not deactivate non-active users' do
+ specify do
human_user = create(:user, user_type: :human, state: :blocked, last_activity_on: Gitlab::CurrentSettings.deactivate_dormant_users_period.days.ago.to_date)
service_user = create(:user, user_type: :service_user, state: :blocked, last_activity_on: Gitlab::CurrentSettings.deactivate_dormant_users_period.days.ago.to_date)
worker.perform
- expect(deactivation_service).not_to have_received(:execute).with(human_user)
- expect(deactivation_service).not_to have_received(:execute).with(service_user)
+ expect_any_instance_of(::Users::DeactivateService) do |deactivation_service|
+ expect(deactivation_service).not_to have_received(:execute).with(human_user)
+ expect(deactivation_service).not_to have_received(:execute).with(service_user)
+ end
end
+ end
- it 'does not deactivate recently created users' do
+ shared_examples 'does not deactivate recently created users' do
+ specify do
worker.perform
- expect(deactivation_service).not_to have_received(:execute).with(inactive_recently_created)
+ expect_any_instance_of(::Users::DeactivateService) do |deactivation_service|
+ expect(deactivation_service).not_to have_received(:execute).with(inactive_recently_created)
+ end
+ end
+ end
+
+ context 'when automatic deactivation of dormant users is enabled' do
+ before do
+ stub_application_setting(deactivate_dormant_users: true)
+ end
+
+ context 'when admin mode is not enabled', :do_not_mock_admin_mode_setting do
+ include_examples 'deactivates dormant users'
+ include_examples 'deactivates certain user types'
+ include_examples 'does not deactivate non-active users'
+ include_examples 'does not deactivate recently created users'
+ end
+
+ context 'when admin mode is enabled', :request_store do
+ before do
+ stub_application_setting(admin_mode: true)
+ end
+
+ include_examples 'deactivates dormant users'
+ include_examples 'deactivates certain user types'
+ include_examples 'does not deactivate non-active users'
+ include_examples 'does not deactivate recently created users'
end
end
@@ -90,7 +119,9 @@
it 'does nothing' do
worker.perform
- expect(deactivation_service).not_to have_received(:execute)
+ expect_any_instance_of(::Users::DeactivateService) do |deactivation_service|
+ expect(deactivation_service).not_to have_received(:execute)
+ end
end
end
end
--
GitLab