diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 700acc46d8d8a82614c4b0329a26fcd9b3fff0d7..cc6b537870bef358ddf58184770b90fe79a9e326 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -136,7 +136,9 @@ def ban end def unban - if update_user { |user| user.activate } + result = Users::UnbanService.new(current_user).execute(user) + + if result[:status] == :success redirect_back_or_admin_user(notice: _("Successfully unbanned")) else redirect_back_or_admin_user(alert: _("Error occurred. User was not unbanned")) diff --git a/app/models/user.rb b/app/models/user.rb index 80b8c9173d1f86eae9adf552bd93c1870bb6eb6e..928c63bf25e3b893d53bcfdc37b839597571ab07 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -205,6 +205,7 @@ def update_tracked_fields!(request) has_one :user_canonical_email has_one :credit_card_validation, class_name: '::Users::CreditCardValidation' has_one :atlassian_identity, class_name: 'Atlassian::Identity' + has_one :banned_user, class_name: '::Users::BannedUser' has_many :reviews, foreign_key: :author_id, inverse_of: :author @@ -326,7 +327,6 @@ def update_tracked_fields!(request) transition deactivated: :blocked transition ldap_blocked: :blocked transition blocked_pending_approval: :blocked - transition banned: :blocked end event :ldap_block do @@ -380,6 +380,14 @@ def blocked? NotificationService.new.user_deactivated(user.name, user.notification_email) end # rubocop: enable CodeReuse/ServiceClass + + after_transition active: :banned do |user| + user.create_banned_user + end + + after_transition banned: :active do |user| + user.banned_user&.destroy + end end # Scopes diff --git a/app/models/users/banned_user.rb b/app/models/users/banned_user.rb new file mode 100644 index 0000000000000000000000000000000000000000..c52b6d4b7284f00785f2d713e1e114eb4d450d42 --- /dev/null +++ b/app/models/users/banned_user.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +module Users + class BannedUser < ApplicationRecord + self.primary_key = :user_id + + belongs_to :user + + validates :user, presence: true + validates :user_id, uniqueness: { message: _("banned user already exists") } + end +end diff --git a/app/services/users/ban_service.rb b/app/services/users/ban_service.rb index 247ed14966bedb4583b58f90fbd34bdb06fc79b9..88e92ebff9b3a613267ff39050c1a8b5c6d5d94c 100644 --- a/app/services/users/ban_service.rb +++ b/app/services/users/ban_service.rb @@ -1,25 +1,15 @@ # frozen_string_literal: true module Users - class BanService < BaseService - def initialize(current_user) - @current_user = current_user - end + class BanService < BannedUserBaseService + private - def execute(user) - if user.ban - log_event(user) - success - else - messages = user.errors.full_messages - error(messages.uniq.join('. ')) - end + def update_user(user) + user.ban end - private - - def log_event(user) - Gitlab::AppLogger.info(message: "User banned", user: "#{user.username}", email: "#{user.email}", banned_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}") + def action + :ban end end end diff --git a/app/services/users/banned_user_base_service.rb b/app/services/users/banned_user_base_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..160410759414285d1a7ea418c9fffca6e7a961a9 --- /dev/null +++ b/app/services/users/banned_user_base_service.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +module Users + class BannedUserBaseService < BaseService + def initialize(current_user) + @current_user = current_user + end + + def execute(user) + return permission_error unless allowed? + + if update_user(user) + log_event(user) + success + else + messages = user.errors.full_messages + error(messages.uniq.join('. ')) + end + end + + private + + attr_reader :current_user + + def allowed? + can?(current_user, :admin_all_resources) + end + + def permission_error + error(_("You are not allowed to %{action} a user" % { action: action.to_s }), :forbidden) + end + + def log_event(user) + Gitlab::AppLogger.info(message: "User #{action}", user: "#{user.username}", email: "#{user.email}", "#{action}_by": "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}") + end + end +end diff --git a/app/services/users/unban_service.rb b/app/services/users/unban_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..363783cf240a6a731cf80d19d2e775ceb7ab6bbf --- /dev/null +++ b/app/services/users/unban_service.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module Users + class UnbanService < BannedUserBaseService + private + + def update_user(user) + user.activate + end + + def action + :unban + end + end +end diff --git a/db/migrate/20210713211008_create_banned_users.rb b/db/migrate/20210713211008_create_banned_users.rb new file mode 100644 index 0000000000000000000000000000000000000000..7e5eb7f95b84f5bf0c515da9237ccd1941a139bb --- /dev/null +++ b/db/migrate/20210713211008_create_banned_users.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +class CreateBannedUsers < ActiveRecord::Migration[6.1] + include Gitlab::Database::MigrationHelpers + + def up + with_lock_retries do + create_table :banned_users, id: false do |t| + t.timestamps_with_timezone null: false + t.references :user, primary_key: true, default: nil, foreign_key: { on_delete: :cascade }, type: :bigint, index: false, null: false + end + end + end + + def down + with_lock_retries do + drop_table :banned_users + end + end +end diff --git a/db/schema_migrations/20210713211008 b/db/schema_migrations/20210713211008 new file mode 100644 index 0000000000000000000000000000000000000000..75ccad3e3483be2d7944877bb4c1425fde385a38 --- /dev/null +++ b/db/schema_migrations/20210713211008 @@ -0,0 +1 @@ +f66d8f3bc32996fe7743cc98cfb96fedd86784d38c8debb5143b7adabdfebd18 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index da2d4c50068944c6c5efd7af95533585764d899b..7a7e5208e28991183d3367d1233f02d2837c2e19 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -9901,6 +9901,12 @@ CREATE SEQUENCE badges_id_seq ALTER SEQUENCE badges_id_seq OWNED BY badges.id; +CREATE TABLE banned_users ( + created_at timestamp with time zone NOT NULL, + updated_at timestamp with time zone NOT NULL, + user_id bigint NOT NULL +); + CREATE TABLE batched_background_migration_jobs ( id bigint NOT NULL, created_at timestamp with time zone NOT NULL, @@ -20979,6 +20985,9 @@ ALTER TABLE ONLY background_migration_jobs ALTER TABLE ONLY badges ADD CONSTRAINT badges_pkey PRIMARY KEY (id); +ALTER TABLE ONLY banned_users + ADD CONSTRAINT banned_users_pkey PRIMARY KEY (user_id); + ALTER TABLE ONLY batched_background_migration_jobs ADD CONSTRAINT batched_background_migration_jobs_pkey PRIMARY KEY (id); @@ -28082,6 +28091,9 @@ ALTER TABLE ONLY merge_trains ALTER TABLE ONLY ci_runner_namespaces ADD CONSTRAINT fk_rails_f9d9ed3308 FOREIGN KEY (namespace_id) REFERENCES namespaces(id) ON DELETE CASCADE; +ALTER TABLE ONLY banned_users + ADD CONSTRAINT fk_rails_fa5bb598e5 FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE; + ALTER TABLE ONLY requirements_management_test_reports ADD CONSTRAINT fk_rails_fb3308ad55 FOREIGN KEY (requirement_id) REFERENCES requirements(id) ON DELETE CASCADE; diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 5a8c95a0ebff86089f89b01540348cfe812d99d5..9c2c90aaa96b70530c3376f934059fd203479a4c 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -37305,6 +37305,9 @@ msgstr "" msgid "You are going to turn on confidentiality. Only team members with %{strongStart}at least Reporter access%{strongEnd} will be able to see and leave comments on the %{issuableType}." msgstr "" +msgid "You are not allowed to %{action} a user" +msgstr "" + msgid "You are not allowed to approve a user" msgstr "" @@ -38207,6 +38210,9 @@ msgstr "" msgid "authored" msgstr "" +msgid "banned user already exists" +msgstr "" + msgid "blocks" msgstr "" diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb index 6dc5c38cb7638f52acab32a0a68d4c6e084fbe83..be21fffb2966b45ee5ef8cae54b6ce8b59afd851 100644 --- a/spec/controllers/admin/users_controller_spec.rb +++ b/spec/controllers/admin/users_controller_spec.rb @@ -359,13 +359,12 @@ end end - describe 'PUT ban/:id' do + describe 'PUT ban/:id', :aggregate_failures do context 'when ban_user_feature_flag is enabled' do it 'bans user' do put :ban, params: { id: user.username } - user.reload - expect(user.banned?).to be_truthy + expect(user.reload.banned?).to be_truthy expect(flash[:notice]).to eq _('Successfully banned') end @@ -390,21 +389,19 @@ it 'does not ban user, renders 404' do put :ban, params: { id: user.username } - user.reload - expect(user.banned?).to be_falsey + expect(user.reload.banned?).to be_falsey expect(response).to have_gitlab_http_status(:not_found) end end end - describe 'PUT unban/:id' do + describe 'PUT unban/:id', :aggregate_failures do let(:banned_user) { create(:user, :banned) } it 'unbans user' do put :unban, params: { id: banned_user.username } - banned_user.reload - expect(banned_user.banned?).to be_falsey + expect(banned_user.reload.banned?).to be_falsey expect(flash[:notice]).to eq _('Successfully unbanned') end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 0eb769c65cd9f5ee22dd3da01845ed2c44233404..cd68d5e96bb62b283aff40bdbff7ee85d9c87f6d 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -89,6 +89,7 @@ it { is_expected.to have_one(:atlassian_identity) } it { is_expected.to have_one(:user_highest_role) } it { is_expected.to have_one(:credit_card_validation) } + it { is_expected.to have_one(:banned_user) } it { is_expected.to have_many(:snippets).dependent(:destroy) } it { is_expected.to have_many(:members) } it { is_expected.to have_many(:project_members) } @@ -1950,6 +1951,42 @@ end end + describe 'banning and unbanning a user', :aggregate_failures do + let(:user) { create(:user) } + + context 'banning a user' do + it 'bans and blocks the user' do + user.ban + + expect(user.banned?).to eq(true) + expect(user.blocked?).to eq(true) + end + + it 'creates a BannedUser record' do + expect { user.ban }.to change { Users::BannedUser.count }.by(1) + expect(Users::BannedUser.last.user_id).to eq(user.id) + end + end + + context 'unbanning a user' do + before do + user.ban! + end + + it 'activates the user' do + user.activate + + expect(user.banned?).to eq(false) + expect(user.active?).to eq(true) + end + + it 'deletes the BannedUser record' do + expect { user.activate }.to change { Users::BannedUser.count }.by(-1) + expect(Users::BannedUser.where(user_id: user.id)).not_to exist + end + end + end + describe '.filter_items' do let(:user) { double } diff --git a/spec/models/users/banned_user_spec.rb b/spec/models/users/banned_user_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..b55c4821d05ce627e226f5921226a5f453bf73a4 --- /dev/null +++ b/spec/models/users/banned_user_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::BannedUser do + describe 'relationships' do + it { is_expected.to belong_to(:user) } + end + + describe 'validations' do + before do + create(:user, :banned) + end + + it { is_expected.to validate_presence_of(:user) } + + it 'validates uniqueness of banned user id' do + is_expected.to validate_uniqueness_of(:user_id).with_message("banned user already exists") + end + end +end diff --git a/spec/services/users/ban_service_spec.rb b/spec/services/users/ban_service_spec.rb index 0e6ac615da5f4046487774376a2c4dc6d2a71cab..6f49ee08782614e001108444c794c3281174a201 100644 --- a/spec/services/users/ban_service_spec.rb +++ b/spec/services/users/ban_service_spec.rb @@ -3,47 +3,68 @@ require 'spec_helper' RSpec.describe Users::BanService do - let(:current_user) { create(:admin) } + let(:user) { create(:user) } - subject(:service) { described_class.new(current_user) } + let_it_be(:current_user) { create(:admin) } - describe '#execute' do - subject(:operation) { service.execute(user) } + shared_examples 'does not modify the BannedUser record or user state' do + it 'does not modify the BannedUser record or user state' do + expect { ban_user }.not_to change { Users::BannedUser.count } + expect { ban_user }.not_to change { user.state } + end + end - context 'when successful' do - let(:user) { create(:user) } + context 'ban', :aggregate_failures do + subject(:ban_user) { described_class.new(current_user).execute(user) } - it { is_expected.to eq(status: :success) } + context 'when successful', :enable_admin_mode do + it 'returns success status' do + response = ban_user - it "bans the user" do - expect { operation }.to change { user.state }.to('banned') + expect(response[:status]).to eq(:success) end - it "blocks the user" do - expect { operation }.to change { user.blocked? }.from(false).to(true) + it 'bans the user' do + expect { ban_user }.to change { user.state }.from('active').to('banned') end - it 'logs ban in application logs' do - allow(Gitlab::AppLogger).to receive(:info) + it 'creates a BannedUser' do + expect { ban_user }.to change { Users::BannedUser.count }.by(1) + expect(Users::BannedUser.last.user_id).to eq(user.id) + end - operation + it 'logs ban in application logs' do + expect(Gitlab::AppLogger).to receive(:info).with(message: "User ban", user: "#{user.username}", email: "#{user.email}", ban_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}") - expect(Gitlab::AppLogger).to have_received(:info).with(message: "User banned", user: "#{user.username}", email: "#{user.email}", banned_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}") + ban_user end end context 'when failed' do - let(:user) { create(:user, :blocked) } + context 'when user is blocked', :enable_admin_mode do + before do + user.block! + end - it 'returns error result' do - aggregate_failures 'error result' do - expect(operation[:status]).to eq(:error) - expect(operation[:message]).to match(/State cannot transition/) + it 'returns state error message' do + response = ban_user + + expect(response[:status]).to eq(:error) + expect(response[:message]).to match(/State cannot transition/) end + + it_behaves_like 'does not modify the BannedUser record or user state' end - it "does not change the user's state" do - expect { operation }.not_to change { user.state } + context 'when user is not an admin' do + it 'returns permissions error message' do + response = ban_user + + expect(response[:status]).to eq(:error) + expect(response[:message]).to match(/You are not allowed to ban a user/) + end + + it_behaves_like 'does not modify the BannedUser record or user state' end end end diff --git a/spec/services/users/banned_user_base_service_spec.rb b/spec/services/users/banned_user_base_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..29a549f0f4992348a7332ba46b7ed986f7a14678 --- /dev/null +++ b/spec/services/users/banned_user_base_service_spec.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::BannedUserBaseService do + let(:admin) { create(:admin) } + let(:base_service) { described_class.new(admin) } + + describe '#initialize' do + it 'sets the current_user instance value' do + expect(base_service.instance_values["current_user"]).to eq(admin) + end + end +end diff --git a/spec/services/users/unban_service_spec.rb b/spec/services/users/unban_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..b2b3140ccb30efb6033d602ad76bbccb6bf26e98 --- /dev/null +++ b/spec/services/users/unban_service_spec.rb @@ -0,0 +1,75 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::UnbanService do + let(:user) { create(:user) } + + let_it_be(:current_user) { create(:admin) } + + shared_examples 'does not modify the BannedUser record or user state' do + it 'does not modify the BannedUser record or user state' do + expect { unban_user }.not_to change { Users::BannedUser.count } + expect { unban_user }.not_to change { user.state } + end + end + + context 'unban', :aggregate_failures do + subject(:unban_user) { described_class.new(current_user).execute(user) } + + context 'when successful', :enable_admin_mode do + before do + user.ban! + end + + it 'returns success status' do + response = unban_user + + expect(response[:status]).to eq(:success) + end + + it 'unbans the user' do + expect { unban_user }.to change { user.state }.from('banned').to('active') + end + + it 'removes the BannedUser' do + expect { unban_user }.to change { Users::BannedUser.count }.by(-1) + expect(user.reload.banned_user).to be_nil + end + + it 'logs unban in application logs' do + expect(Gitlab::AppLogger).to receive(:info).with(message: "User unban", user: "#{user.username}", email: "#{user.email}", unban_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}") + + unban_user + end + end + + context 'when failed' do + context 'when user is already active', :enable_admin_mode do + it 'returns state error message' do + response = unban_user + + expect(response[:status]).to eq(:error) + expect(response[:message]).to match(/State cannot transition/) + end + + it_behaves_like 'does not modify the BannedUser record or user state' + end + + context 'when user is not an admin' do + before do + user.ban! + end + + it 'returns permissions error message' do + response = unban_user + + expect(response[:status]).to eq(:error) + expect(response[:message]).to match(/You are not allowed to unban a user/) + end + + it_behaves_like 'does not modify the BannedUser record or user state' + end + end + end +end