diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 8d2146cddc5bfdb09c542c36e6886178085f330d..3c96e49499f06d3eb077f57e12e2d6f4061f553a 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -87,12 +87,14 @@ def reject end def activate - if user.blocked? - return redirect_back_or_admin_user(notice: _("Error occurred. A blocked user must be unblocked to be activated")) - end + activate_service = Users::ActivateService.new(current_user) + result = activate_service.execute(user) - user.activate - redirect_back_or_admin_user(notice: _("Successfully activated")) + if result.success? + redirect_back_or_admin_user(notice: _("Successfully activated")) + else + redirect_back_or_admin_user(alert: result.message) + end end def deactivate diff --git a/app/services/users/activate_service.rb b/app/services/users/activate_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..dfc2996bcce9d15227f0c8764a544562c36ae49a --- /dev/null +++ b/app/services/users/activate_service.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +module Users + class ActivateService < BaseService + def initialize(current_user) + @current_user = current_user + end + + def execute(user) + return error(_('You are not authorized to perform this action'), :forbidden) unless allowed? + + return error(_('Error occurred. A blocked user must be unblocked to be activated'), :forbidden) if user.blocked? + + return success(_('Successfully activated')) if user.active? + + if user.activate + after_activate_hook(user) + log_event(user) + success(_('Successfully activated')) + else + error(user.errors.full_messages.to_sentence, :unprocessable_entity) + end + end + + private + + attr_reader :current_user + + def allowed? + can?(current_user, :admin_all_resources) + end + + def after_activate_hook(user) + # overridden by EE module + end + + def log_event(user) + Gitlab::AppLogger.info(message: 'User activated', user: user.username.to_s, email: user.email.to_s, + activated_by: current_user.username.to_s, ip_address: current_user.current_sign_in_ip.to_s) + end + + def success(message) + ::ServiceResponse.success(message: message) + end + + def error(message, reason) + ::ServiceResponse.error(message: message, reason: reason) + end + end +end + +Users::ActivateService.prepend_mod_with('Users::ActivateService') # rubocop: disable Cop/InjectEnterpriseEditionModule diff --git a/doc/administration/audit_events.md b/doc/administration/audit_events.md index 50bd943b8e4ff6d73b78eecb3fa5a221d3d840db..8207918dbbc622bbb2ed6b5a9436a898bfed8971 100644 --- a/doc/administration/audit_events.md +++ b/doc/administration/audit_events.md @@ -393,6 +393,8 @@ The following user actions on a GitLab instance generate instance audit events: - User was unblocked using the Admin Area or API. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/115727) in GitLab 15.11. - User was banned using the Admin Area or API. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/116103) in GitLab 15.11. - User was unbanned using the Admin Area or API. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/116221) in GitLab 15.11. +- User was deactivated using the Admin Area or API. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/117776) in GitLab 16.0. +- User was activated using the Admin Area or API. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/121708) in GitLab 16.1. Instance events can also be accessed using the [Instance Audit Events API](../api/audit_events.md#instance-audit-events). diff --git a/ee/app/services/ee/users/activate_service.rb b/ee/app/services/ee/users/activate_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..79fce38f24711d976dd428d0ff91656dace26147 --- /dev/null +++ b/ee/app/services/ee/users/activate_service.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module EE + module Users + module ActivateService + extend ::Gitlab::Utils::Override + + private + + override :after_activate_hook + def after_activate_hook(user) + log_audit_event(user) + end + + def log_audit_event(user) + audit_context = { + name: 'user_activate', + author: current_user, + scope: user, + target: user, + message: "Activated user", + target_details: user.username + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end + end + end +end diff --git a/ee/config/audit_events/types/user_activate.yml b/ee/config/audit_events/types/user_activate.yml new file mode 100644 index 0000000000000000000000000000000000000000..93d9cfaab2031756095368a57f6bcccf102ca409 --- /dev/null +++ b/ee/config/audit_events/types/user_activate.yml @@ -0,0 +1,8 @@ +name: user_activate +description: Event triggered on user activate action +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/13473 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/121708 +feature_category: "user_management" +milestone: "16.1" +saved_to_database: true +streamed: true diff --git a/ee/spec/services/ee/users/activate_service_spec.rb b/ee/spec/services/ee/users/activate_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..7cd90adb257c53195416dbd90cb1d35b93cd15be --- /dev/null +++ b/ee/spec/services/ee/users/activate_service_spec.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::ActivateService, 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, :deactivated) } + + 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(:activate).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: 'Activated 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 dc77dc5c157df31e2c0ec579b930cef73f406d5f..dbf8f7711ea236a62bcf9aa48bee0fa17e108725 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -707,9 +707,13 @@ def reorder_users(users) user = User.find_by(id: params[:id]) not_found!('User') unless user - forbidden!('A blocked user must be unblocked to be activated') if user.blocked? - user.activate + result = ::Users::ActivateService.new(current_user).execute(user) + if result[:status] == :success + true + else + render_api_error!(result[:message], result[:reason] || :bad_request) + end end desc 'Approve a pending user. Available only for admins.' diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb index a53d200b1e1290f81de1a4f62d7564f5fe3bc124..399b7c02c524344e3d460cc06bfaaf5cb3329029 100644 --- a/spec/controllers/admin/users_controller_spec.rb +++ b/spec/controllers/admin/users_controller_spec.rb @@ -356,7 +356,7 @@ put :activate, params: { id: user.username } user.reload expect(user.active?).to be_falsey - expect(flash[:notice]).to eq('Error occurred. A blocked user must be unblocked to be activated') + expect(flash[:alert]).to eq('Error occurred. A blocked user must be unblocked to be activated') end end end diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index cc8be312c71a43d2f6ca9c2bfa3fcb74196011a8..c1aeb9991cd94f987aacc587f580b6862b51d4d0 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -3480,7 +3480,7 @@ def update_password(user, admin, password = User.random_password) activate expect(response).to have_gitlab_http_status(:forbidden) - expect(json_response['message']).to eq('403 Forbidden - A blocked user must be unblocked to be activated') + expect(json_response['message']).to eq('Error occurred. A blocked user must be unblocked to be activated') expect(blocked_user.reload.state).to eq('blocked') end end @@ -3494,7 +3494,7 @@ def update_password(user, admin, password = User.random_password) activate expect(response).to have_gitlab_http_status(:forbidden) - expect(json_response['message']).to eq('403 Forbidden - A blocked user must be unblocked to be activated') + expect(json_response['message']).to eq('Error occurred. A blocked user must be unblocked to be activated') expect(user.reload.state).to eq('ldap_blocked') end end diff --git a/spec/services/users/activate_service_spec.rb b/spec/services/users/activate_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..8b8c0dbdd3e797fb58c503d5c3f4e818ed3916da --- /dev/null +++ b/spec/services/users/activate_service_spec.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::ActivateService, feature_category: :user_management do + let_it_be(:current_user) { build(:admin) } + + subject(:service) { described_class.new(current_user) } + + describe '#execute' do + let!(:user) { create(:user, :deactivated) } + + subject(:operation) { service.execute(user) } + + context 'when successful', :enable_admin_mode do + 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('active') + end + + it 'creates a log entry' do + expect(Gitlab::AppLogger).to receive(:info).with(message: "User activated", user: user.username, + email: user.email, activated_by: current_user.username, ip_address: current_user.current_sign_in_ip.to_s) + + operation + end + end + + context 'when the user is already active', :enable_admin_mode do + let(:user) { create(:user) } + + it 'returns success result' do + aggregate_failures 'success result' do + expect(operation[:status]).to eq(:success) + expect(operation[:message]).to eq('Successfully activated') + end + end + + it "does not change the user's state" do + expect { operation }.not_to change { user.state } + end + end + + context 'when user activation fails', :enable_admin_mode do + before do + allow(user).to receive(:activate).and_return(false) + end + + it 'returns an unprocessable entity error' do + result = service.execute(user) + + expect(result[:status]).to eq(:error) + expect(result[:reason]).to eq(:unprocessable_entity) + end + end + + context 'when user is not an admin' do + let(:non_admin_user) { build(:user) } + let(:service) { described_class.new(non_admin_user) } + + 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 + end +end