diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 7fc534be25359d937a8bdee8903a2b484779356b..45a7901b2c4039e5a9e8e40f151abba3d84260da 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -96,19 +96,14 @@ def activate end def deactivate - if user.blocked? - return redirect_back_or_admin_user(notice: _("Error occurred. A blocked user cannot be deactivated")) - end - - return redirect_back_or_admin_user(notice: _("Successfully deactivated")) if user.deactivated? - return redirect_back_or_admin_user(notice: _("Internal users cannot be deactivated")) if user.internal? + deactivate_service = Users::DeactivateService.new(current_user, skip_authorization: true) + result = deactivate_service.execute(user) - unless user.can_be_deactivated? - return redirect_back_or_admin_user(notice: format(_("The user you are trying to deactivate has been active in the past %{minimum_inactive_days} days and cannot be deactivated"), minimum_inactive_days: Gitlab::CurrentSettings.deactivate_dormant_users_period)) + if result.success? + redirect_back_or_admin_user(notice: _("Successfully deactivated")) + else + redirect_back_or_admin_user(alert: result.message) end - - user.deactivate - redirect_back_or_admin_user(notice: _("Successfully deactivated")) end def block diff --git a/app/services/users/deactivate_service.rb b/app/services/users/deactivate_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..e69ce13d3cc8242cc6a8585199cf75858513060c --- /dev/null +++ b/app/services/users/deactivate_service.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +module Users + class DeactivateService < BaseService + def initialize(current_user, skip_authorization: false) + @current_user = current_user + @skip_authorization = skip_authorization + end + + def execute(user) + unless allowed? + return ::ServiceResponse.error(message: _('You are not authorized to perform this action'), + reason: :forbidden) + end + + if user.blocked? + return ::ServiceResponse.error(message: _('Error occurred. A blocked user cannot be deactivated'), + reason: :forbidden) + end + + if user.internal? + return ::ServiceResponse.error(message: _('Internal users cannot be deactivated'), + reason: :forbidden) + end + + return ::ServiceResponse.success(message: _('User has already been deactivated')) if user.deactivated? + + unless user.can_be_deactivated? + message = _( + 'The user you are trying to deactivate has been active in the past %{minimum_inactive_days} days ' \ + 'and cannot be deactivated') + + deactivation_error_message = format(message, + minimum_inactive_days: Gitlab::CurrentSettings.deactivate_dormant_users_period) + return ::ServiceResponse.error(message: deactivation_error_message, reason: :forbidden) + end + + unless user.deactivate + return ::ServiceResponse.error(message: user.errors.full_messages.to_sentence, + reason: :bad_request) + end + + log_event(user) + + ::ServiceResponse.success + end + + private + + attr_reader :current_user + + def allowed? + return true if @skip_authorization + + can?(current_user, :admin_all_resources) + end + + def log_event(user) + Gitlab::AppLogger.info(message: 'User deactivated', user: user.username.to_s, email: user.email.to_s, + deactivated_by: current_user.username.to_s, ip_address: current_user.current_sign_in_ip.to_s) + end + end +end + +Users::DeactivateService.prepend_mod_with('Users::DeactivateService') diff --git a/ee/app/services/ee/users/deactivate_service.rb b/ee/app/services/ee/users/deactivate_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..fe0695d942b1991ebc127b721af2ee0e0cc4e3bf --- /dev/null +++ b/ee/app/services/ee/users/deactivate_service.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +module EE + module Users + module DeactivateService + extend ::Gitlab::Utils::Override + + override :execute + def execute(user) + super.tap do |result| + log_audit_event(user) if result[:status] == :success + end + end + + private + + def log_audit_event(user) + audit_context = { + name: 'user_deactivate', + author: current_user, + scope: user, + target: user, + message: "Deactivated user", + target_details: user.username + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end + end + end +end diff --git a/ee/config/audit_events/types/user_deactivate.yml b/ee/config/audit_events/types/user_deactivate.yml new file mode 100644 index 0000000000000000000000000000000000000000..beeaf31c4c388d83d3a9a02359a852044b0edc7d --- /dev/null +++ b/ee/config/audit_events/types/user_deactivate.yml @@ -0,0 +1,8 @@ +name: user_deactivate +description: Event triggered on user deactivate action +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/13473 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/117776 +feature_category: "user_management" +milestone: "16.0" +saved_to_database: true +streamed: true diff --git a/ee/spec/services/ee/users/deactivate_service_spec.rb b/ee/spec/services/ee/users/deactivate_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..46ffe4f0649ec03994bedf62aaf8b6fc43d6c788 --- /dev/null +++ b/ee/spec/services/ee/users/deactivate_service_spec.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::DeactivateService, feature_category: :user_management do + let_it_be(:current_user) { create(:admin) } + + subject(:service) { described_class.new(current_user) } + + describe '#execute' do + let!(:user) { create(:user) } + + subject(:operation) { service.execute(user) } + + context 'for audit events', :enable_admin_mode do + include_examples 'audit event logging' do + let(:operation) { service.execute(user) } + + let(:fail_condition!) do + allow(user).to receive(:deactivate).and_return(false) + end + + let(:attributes) do + { + author_id: current_user.id, + entity_id: user.id, + entity_type: 'User', + details: { + author_class: 'User', + author_name: current_user.name, + custom_message: 'Deactivated user', + target_details: user.username, + target_id: user.id, + target_type: 'User' + } + } + end + end + end + end +end diff --git a/lib/api/users.rb b/lib/api/users.rb index 505fcb2b38e8ecd4f6d9d9dfd30fa1a79d16f91c..3d9af536c3c03c7b60ac89721c0bcf5e9133870c 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -758,16 +758,11 @@ def reorder_users(users) break if user.deactivated? - unless user.can_be_deactivated? - forbidden!('A blocked user cannot be deactivated by the API') if user.blocked? - forbidden!('An internal user cannot be deactivated by the API') if user.internal? - forbidden!("The user you are trying to deactivate has been active in the past #{Gitlab::CurrentSettings.deactivate_dormant_users_period} days and cannot be deactivated") - end - - if user.deactivate + result = ::Users::DeactivateService.new(current_user, skip_authorization: true).execute(user) + if result[:status] == :success true else - render_api_error!(user.errors.full_messages, 400) + render_api_error!(result[:message], result[:reason] || :bad_request) end end # rubocop: enable CodeReuse/ActiveRecord diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 4887fed6ca1771888e5615fcfe546b32ddd82c3b..ba3d349baf0aba5be4ee395d3b4b002db01adfb6 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -47999,6 +47999,9 @@ msgstr "" msgid "User does not have permission to create a Security Policy project." msgstr "" +msgid "User has already been deactivated" +msgstr "" + msgid "User identity was successfully created." msgstr "" diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb index ec2559550c36d48a3f3c75fa2d80a3e86849c779..9b00451de304b3add566d7892cdb474297e716cc 100644 --- a/spec/controllers/admin/users_controller_spec.rb +++ b/spec/controllers/admin/users_controller_spec.rb @@ -388,7 +388,7 @@ put :deactivate, params: { id: user.username } user.reload expect(user.deactivated?).to be_falsey - expect(flash[:notice]).to eq("The user you are trying to deactivate has been active in the past #{Gitlab::CurrentSettings.deactivate_dormant_users_period} days and cannot be deactivated") + expect(flash[:alert]).to eq("The user you are trying to deactivate has been active in the past #{Gitlab::CurrentSettings.deactivate_dormant_users_period} days and cannot be deactivated") end end end @@ -410,7 +410,7 @@ put :deactivate, params: { id: user.username } user.reload expect(user.deactivated?).to be_falsey - expect(flash[:notice]).to eq('Error occurred. A blocked user cannot be deactivated') + expect(flash[:alert]).to eq('Error occurred. A blocked user cannot be deactivated') end end @@ -421,7 +421,7 @@ put :deactivate, params: { id: internal_user.username } expect(internal_user.reload.deactivated?).to be_falsey - expect(flash[:notice]).to eq('Internal users cannot be deactivated') + expect(flash[:alert]).to eq('Internal users cannot be deactivated') end end end diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 3ecd012be129fc74086fa396cf8b5c45ad78c8d0..cc8be312c71a43d2f6ca9c2bfa3fcb74196011a8 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -3558,7 +3558,7 @@ def update_password(user, admin, password = User.random_password) deactivate expect(response).to have_gitlab_http_status(:forbidden) - expect(json_response['message']).to eq("403 Forbidden - The user you are trying to deactivate has been active in the past #{Gitlab::CurrentSettings.deactivate_dormant_users_period} days and cannot be deactivated") + expect(json_response['message']).to eq("The user you are trying to deactivate has been active in the past #{Gitlab::CurrentSettings.deactivate_dormant_users_period} days and cannot be deactivated") expect(user.reload.state).to eq('active') end end @@ -3582,7 +3582,7 @@ def update_password(user, admin, password = User.random_password) deactivate expect(response).to have_gitlab_http_status(:forbidden) - expect(json_response['message']).to eq('403 Forbidden - A blocked user cannot be deactivated by the API') + expect(json_response['message']).to eq('Error occurred. A blocked user cannot be deactivated') expect(blocked_user.reload.state).to eq('blocked') end end @@ -3596,7 +3596,7 @@ def update_password(user, admin, password = User.random_password) deactivate expect(response).to have_gitlab_http_status(:forbidden) - expect(json_response['message']).to eq('403 Forbidden - A blocked user cannot be deactivated by the API') + expect(json_response['message']).to eq('Error occurred. A blocked user cannot be deactivated') expect(user.reload.state).to eq('ldap_blocked') end end @@ -3608,7 +3608,7 @@ def update_password(user, admin, password = User.random_password) deactivate expect(response).to have_gitlab_http_status(:forbidden) - expect(json_response['message']).to eq('403 Forbidden - An internal user cannot be deactivated by the API') + expect(json_response['message']).to eq('Internal users cannot be deactivated') end end diff --git a/spec/services/users/deactivate_service_spec.rb b/spec/services/users/deactivate_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..0bb6e51a3b13f55ea64ebcd7a8e810d0578ab620 --- /dev/null +++ b/spec/services/users/deactivate_service_spec.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::DeactivateService, feature_category: :user_management do + let_it_be(:current_user) { build(:admin) } + let_it_be(:user) { build(:user) } + + subject(:service) { described_class.new(current_user) } + + describe '#execute' do + subject(:operation) { service.execute(user) } + + context 'when successful', :enable_admin_mode do + let(:user) { create(:user) } + + it 'returns success status' do + expect(operation[:status]).to eq(:success) + end + + it "changes the user's state" do + expect { operation }.to change { user.state }.to('deactivated') + end + + it 'creates a log entry' do + expect(Gitlab::AppLogger).to receive(:info).with(message: "User deactivated", user: user.username, + email: user.email, deactivated_by: current_user.username, ip_address: current_user.current_sign_in_ip.to_s) + + operation + end + end + + context 'when the user is already deactivated', :enable_admin_mode do + let(:user) { create(:user, :deactivated) } + + it 'returns error result' do + aggregate_failures 'error result' do + expect(operation[:status]).to eq(:success) + expect(operation[:message]).to eq('User has already been deactivated') + end + end + + it "does not change the user's state" do + expect { operation }.not_to change { user.state } + end + end + + context 'when internal user', :enable_admin_mode do + let(:user) { create(:user, :bot) } + + it 'returns an error message' do + expect(operation[:status]).to eq(:error) + expect(operation[:message]).to eq('Internal users cannot be deactivated') + expect(operation.reason).to eq :forbidden + end + end + + context 'when user is blocked', :enable_admin_mode do + let(:user) { create(:user, :blocked) } + + it 'returns an error message' do + expect(operation[:status]).to eq(:error) + expect(operation[:message]).to eq('Error occurred. A blocked user cannot be deactivated') + expect(operation.reason).to eq :forbidden + end + end + + context 'when user is not an admin' do + it 'returns permissions error message' do + expect(operation[:status]).to eq(:error) + expect(operation[:message]).to eq("You are not authorized to perform this action") + expect(operation.reason).to eq :forbidden + end + end + + context 'when skip_authorization is true' do + let(:non_admin_user) { create(:user) } + let(:user_to_deactivate) { create(:user) } + let(:skip_authorization_service) { described_class.new(non_admin_user, skip_authorization: true) } + + it 'deactivates the user even if the current user is not an admin' do + expect(skip_authorization_service.execute(user_to_deactivate)[:status]).to eq(:success) + end + end + end +end