Merge branch 'create-banned-user-table' into 'master'

Create BannedUser table

See merge request gitlab-org/gitlab!64728

app/controllers/admin/users_controller.rb

@@ -136,7 +136,9 @@ class Admin::UsersController < Admin::ApplicationController
   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"))

app/models/user.rb

@@ -205,6 +205,7 @@ class User < ApplicationRecord
   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 @@ class User < ApplicationRecord
       transition deactivated: :blocked
       transition ldap_blocked: :blocked
       transition blocked_pending_approval: :blocked
-      transition banned: :blocked
     end
 
     event :ldap_block do
@@ -380,6 +380,14 @@ class User < ApplicationRecord
       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

app/models/users/banned_user.rb

+# 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

app/services/users/ban_service.rb

 # 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

app/services/users/banned_user_base_service.rb

+# 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

app/services/users/unban_service.rb

+# frozen_string_literal: true
+
+module Users
+  class UnbanService < BannedUserBaseService
+    private
+
+    def update_user(user)
+      user.activate
+    end
+
+    def action
+      :unban
+    end
+  end
+end

db/migrate/20210713211008_create_banned_users.rb

+# 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

db/schema_migrations/20210713211008

+f66d8f3bc32996fe7743cc98cfb96fedd86784d38c8debb5143b7adabdfebd18
\ No newline at end of file

db/structure.sql

@@ -9959,6 +9959,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,
@@ -21092,6 +21098,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);
 
@@ -28235,6 +28244,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;
 

locale/gitlab.pot

@@ -37537,6 +37537,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 ""
 
@@ -38442,6 +38445,9 @@ msgstr ""
 msgid "authored"
 msgstr ""
 
+msgid "banned user already exists"
+msgstr ""
+
 msgid "blocks"
 msgstr ""
 

spec/controllers/admin/users_controller_spec.rb

@@ -359,13 +359,12 @@ RSpec.describe Admin::UsersController do
     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 @@ RSpec.describe Admin::UsersController do
       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

spec/models/user_spec.rb

@@ -89,6 +89,7 @@ RSpec.describe User do
     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) }
@@ -1959,6 +1960,42 @@ RSpec.describe User do
     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 }
 

spec/models/users/banned_user_spec.rb

+# 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

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

spec/services/users/banned_user_base_service_spec.rb

+# 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

spec/services/users/unban_service_spec.rb

+# 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