Merge branch 'al-244519-block-users-when-user-cap-exceeded' into 'master'

New users should have admin approval if User Cap setting is exceeded

See merge request gitlab-org/gitlab!47993

app/controllers/registrations_controller.rb

@@ -6,8 +6,6 @@ class RegistrationsController < Devise::RegistrationsController
   include RecaptchaHelper
   include InvisibleCaptchaOnSignup
 
-  BLOCKED_PENDING_APPROVAL_STATE = 'blocked_pending_approval'.freeze
-
   layout 'devise'
 
   prepend_before_action :check_captcha, only: :create
@@ -167,12 +165,18 @@ class RegistrationsController < Devise::RegistrationsController
   end
 
   def set_user_state
-    return unless Gitlab::CurrentSettings.require_admin_approval_after_user_signup
+    return unless set_blocked_pending_approval?
+
+    resource.state = User::BLOCKED_PENDING_APPROVAL_STATE
+  end
 
-    resource.state = BLOCKED_PENDING_APPROVAL_STATE
+  def set_blocked_pending_approval?
+    Gitlab::CurrentSettings.require_admin_approval_after_user_signup
   end
 
   def set_invite_params
     @invite_email = ActionController::Base.helpers.sanitize(params[:invite_email])
   end
 end
+
+RegistrationsController.prepend_if_ee('EE::RegistrationsController')

app/models/user.rb

@@ -30,6 +30,8 @@ class User < ApplicationRecord
 
   INSTANCE_ACCESS_REQUEST_APPROVERS_TO_BE_NOTIFIED_LIMIT = 10
 
+  BLOCKED_PENDING_APPROVAL_STATE = 'blocked_pending_approval'.freeze
+
   add_authentication_token_field :incoming_email_token, token_generator: -> { SecureRandom.hex.to_i(16).to_s(36) }
   add_authentication_token_field :feed_token
   add_authentication_token_field :static_object_token

config/sidekiq_queues.yml

@@ -308,6 +308,8 @@
   - 2
 - - service_desk_email_receiver
   - 1
+- - set_user_status_based_on_user_cap_setting
+  - 1
 - - status_page_publish
   - 1
 - - sync_seat_link_request

ee/app/controllers/ee/registrations_controller.rb

+# frozen_string_literal: true
+
+module EE
+  module RegistrationsController
+    extend ActiveSupport::Concern
+    extend ::Gitlab::Utils::Override
+
+    private
+
+    override :set_blocked_pending_approval?
+    def set_blocked_pending_approval?
+      super || ::Gitlab::CurrentSettings.should_apply_user_signup_cap?
+    end
+  end
+end

ee/app/models/ee/application_setting.rb

@@ -348,6 +348,11 @@ module EE
       write_attribute(:compliance_frameworks, cleaned)
     end
 
+    def should_apply_user_signup_cap?
+      ::Feature.enabled?(:admin_new_user_signups_cap) &&
+        ::Gitlab::CurrentSettings.new_user_signups_cap.present?
+    end
+
     private
 
     def elasticsearch_limited_project_exists?(project)

ee/app/models/ee/user.rb

@@ -28,6 +28,8 @@ module EE
       validate :auditor_requires_license_add_on, if: :auditor
       validate :cannot_be_admin_and_auditor
 
+      after_create :perform_user_cap_check
+
       delegate :shared_runners_minutes_limit, :shared_runners_minutes_limit=,
                :extra_shared_runners_minutes_limit, :extra_shared_runners_minutes_limit=,
                to: :namespace
@@ -413,5 +415,13 @@ module EE
 
       minimal_access_groups.with_feature_available_in_plan(:minimal_access_role)
     end
+
+    def perform_user_cap_check
+      return unless ::Gitlab::CurrentSettings.should_apply_user_signup_cap?
+
+      run_after_commit do
+        SetUserStatusBasedOnUserCapSettingWorker.perform_async(id)
+      end
+    end
   end
 end

ee/app/services/ee/users/build_service.rb

@@ -22,6 +22,7 @@ module EE
         user = super
 
         build_smartcard_identity(user, params) if ::Gitlab::Auth::Smartcard.enabled?
+        set_pending_approval_state(user)
 
         user
       end
@@ -98,6 +99,12 @@ module EE
 
         user.scim_identities.build(scim_identity_params.merge(active: true))
       end
+
+      def set_pending_approval_state(user)
+        return unless ::Gitlab::CurrentSettings.should_apply_user_signup_cap?
+
+        user.state = ::User::BLOCKED_PENDING_APPROVAL_STATE
+      end
     end
   end
 end

ee/app/workers/all_queues.yml

@@ -798,6 +798,14 @@
   :idempotent: true
   :tags:
   - :requires_disk_io
+- :name: set_user_status_based_on_user_cap_setting
+  :feature_category: :users
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: status_page_publish
   :feature_category: :incident_management
   :has_external_dependencies: true

ee/app/workers/set_user_status_based_on_user_cap_setting_worker.rb

+# frozen_string_literal: true
+
+class SetUserStatusBasedOnUserCapSettingWorker
+  include ApplicationWorker
+
+  feature_category :users
+
+  idempotent!
+
+  def perform(user_id)
+    user = User.find_by_id(user_id)
+
+    return unless user.blocked_pending_approval?
+    return if user_cap_max.nil?
+    return if current_billable_users_count >= user_cap_max
+
+    if user.activate
+      # Resends confirmation email if the user isn't confirmed yet.
+      # Please see Devise's implementation of `resend_confirmation_instructions` for detail.
+      user.resend_confirmation_instructions
+      user.accept_pending_invitations! if user.active_for_authentication?
+      DeviseMailer.user_admin_approval(user).deliver_later
+    else
+      logger.error(message: "Approval of user id=#{user_id} failed")
+    end
+  end
+
+  private
+
+  def user_cap_max
+    ::Gitlab::CurrentSettings.new_user_signups_cap
+  end
+
+  def current_billable_users_count
+    User.billable.count
+  end
+end

ee/spec/controllers/ee/registrations_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe RegistrationsController do
+  describe '#create' do
+    let_it_be(:base_user_params) { build_stubbed(:user).slice(:first_name, :last_name, :username, :password) }
+    let_it_be(:new_user_email) { 'new@user.com' }
+    let_it_be(:user_params) { { user: base_user_params.merge(email: new_user_email) } }
+
+    subject { post :create, params: user_params }
+
+    shared_examples 'blocked user by default' do
+      it 'registers the user in blocked_pending_approval state' do
+        subject
+        created_user = User.find_by(email: new_user_email)
+
+        expect(created_user).to be_present
+        expect(created_user).to be_blocked_pending_approval
+      end
+
+      it 'does not log in the user after sign up' do
+        subject
+
+        expect(controller.current_user).to be_nil
+      end
+
+      it 'shows flash message after signing up' do
+        subject
+
+        expect(response).to redirect_to(new_user_session_path(anchor: 'login-pane'))
+        expect(flash[:notice])
+          .to match(/your account is awaiting approval from your GitLab administrator/)
+      end
+    end
+
+    shared_examples 'active user by default' do
+      it 'registers the user in active state' do
+        subject
+        created_user = User.find_by(email: new_user_email)
+
+        expect(created_user).to be_present
+        expect(created_user).to be_active
+      end
+
+      it 'does not show any flash message after signing up' do
+        subject
+
+        expect(flash[:notice]).to be_nil
+      end
+    end
+
+    context 'when require admin approval setting is enabled' do
+      before do
+        stub_application_setting(require_admin_approval_after_user_signup: true)
+      end
+
+      it_behaves_like 'blocked user by default'
+    end
+
+    context 'when require admin approval setting is disabled' do
+      before do
+        stub_application_setting(require_admin_approval_after_user_signup: false)
+      end
+
+      it_behaves_like 'active user by default'
+
+      context 'when user signup cap feature is enabled' do
+        before do
+          stub_application_setting(new_user_signups_cap: true)
+        end
+
+        it_behaves_like 'blocked user by default'
+      end
+    end
+
+    context 'when user signup cap setting is enabled' do
+      before do
+        stub_application_setting(new_user_signups_cap: true)
+      end
+
+      it_behaves_like 'blocked user by default'
+
+      context 'when feature flag is disabled' do
+        before do
+          stub_feature_flags(admin_new_user_signups_cap: false)
+        end
+
+        context 'when require admin approval setting is disabled' do
+          before do
+            stub_application_setting(require_admin_approval_after_user_signup: false)
+          end
+
+          it_behaves_like 'active user by default'
+        end
+
+        context 'when require admin approval setting is enabled' do
+          before do
+            stub_application_setting(require_admin_approval_after_user_signup: true)
+          end
+
+          it_behaves_like 'blocked user by default'
+        end
+      end
+    end
+
+    context 'when user signup cap setting is disabled' do
+      before do
+        stub_application_setting(admin_new_user_signups_cap: false)
+      end
+
+      context 'when require admin approval setting is disabled' do
+        before do
+          stub_application_setting(require_admin_approval_after_user_signup: false)
+        end
+
+        it_behaves_like 'active user by default'
+      end
+
+      context 'when require admin approval setting is enabled' do
+        before do
+          stub_application_setting(require_admin_approval_after_user_signup: true)
+        end
+
+        it_behaves_like 'blocked user by default'
+      end
+    end
+  end
+end

ee/spec/models/application_setting_spec.rb

@@ -742,4 +742,34 @@ RSpec.describe ApplicationSetting do
       expect(setting.compliance_frameworks).to eq([])
     end
   end
+
+  describe '#should_apply_user_signup_cap?' do
+    subject { setting.should_apply_user_signup_cap? }
+
+    context 'when feature admin_new_user_signups_cap is disabled' do
+      before do
+        stub_feature_flags(admin_new_user_signups_cap: false)
+      end
+
+      it { is_expected.to be false }
+    end
+
+    context 'when feature admin_new_user_signups_cap is enabled' do
+      before do
+        allow(Gitlab::CurrentSettings).to receive(:new_user_signups_cap).and_return(new_user_signups_cap)
+      end
+
+      context 'when new_user_signups_cap setting is nil' do
+        let(:new_user_signups_cap) { nil }
+
+        it { is_expected.to be false }
+      end
+
+      context 'when new_user_signups_cap setting is set to any number' do
+        let(:new_user_signups_cap) { 10 }
+
+        it { is_expected.to be true }
+      end
+    end
+  end
 end

ee/spec/models/ee/user_spec.rb

@@ -99,6 +99,46 @@ RSpec.describe User do
     end
   end
 
+  describe 'after_create' do
+    describe '#perform_user_cap_check' do
+      let(:new_user_signups_cap) { nil }
+
+      before do
+        allow(Gitlab::CurrentSettings).to receive(:new_user_signups_cap).and_return(new_user_signups_cap)
+      end
+
+      context 'when feature is disabled' do
+        before do
+          stub_feature_flags(admin_new_user_signups_cap: false)
+        end
+
+        it 'does not call SetUserStatusBasedOnUserCapSettingWorker' do
+          expect(SetUserStatusBasedOnUserCapSettingWorker).not_to receive(:perform_async)
+
+          create(:user, state: 'blocked_pending_approval')
+        end
+      end
+
+      context 'when user cap is not set' do
+        it 'does not call SetUserStatusBasedOnUserCapSettingWorker' do
+          expect(SetUserStatusBasedOnUserCapSettingWorker).not_to receive(:perform_async)
+
+          create(:user, state: 'blocked_pending_approval')
+        end
+      end
+
+      context 'when user cap is set' do
+        let(:new_user_signups_cap) { 10 }
+
+        it 'enqueues SetUserStatusBasedOnUserCapSettingWorker' do
+          expect(SetUserStatusBasedOnUserCapSettingWorker).to receive(:perform_async).once
+
+          create(:user, state: 'blocked_pending_approval')
+        end
+      end
+    end
+  end
+
   describe '.find_by_smartcard_identity' do
     let!(:user) { create(:user) }
     let!(:smartcard_identity) { create(:smartcard_identity, user: user) }

ee/spec/services/ee/users/build_service_spec.rb

@@ -78,6 +78,44 @@ RSpec.describe Users::BuildService do
           end
         end
       end
+
+      context 'user signup cap' do
+        let(:new_user_signups_cap) { 10 }
+
+        before do
+          allow(Gitlab::CurrentSettings).to receive(:new_user_signups_cap).and_return(new_user_signups_cap)
+        end
+
+        context 'when user signup cap is set' do
+          it 'sets the user state to blocked_pending_approval' do
+            user = service.execute
+
+            expect(user).to be_blocked_pending_approval
+          end
+        end
+
+        context 'when user signup cap is not set' do
+          let(:new_user_signups_cap) { nil }
+
+          it 'does not set the user state to blocked_pending_approval' do
+            user = service.execute
+
+            expect(user).to be_active
+          end
+        end
+
+        context 'when feature is disabled' do
+          before do
+            stub_feature_flags(admin_new_user_signups_cap: false)
+          end
+
+          it 'does not set the user state to blocked_pending_approval' do
+            user = service.execute
+
+            expect(user).to be_active
+          end
+        end
+      end
     end
   end
 end

ee/spec/workers/set_user_status_based_on_user_cap_setting_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe SetUserStatusBasedOnUserCapSettingWorker, type: :worker do
+  let_it_be(:active_user) { create(:user, state: 'active') }
+
+  describe '#perform' do
+    let_it_be(:user) { create(:user, :blocked_pending_approval) }
+    let(:new_user_signups_cap) { 10 }
+
+    subject { described_class.new.perform(user.id) }
+
+    before do
+      allow(Gitlab::CurrentSettings).to receive(:new_user_signups_cap).and_return(new_user_signups_cap)
+    end
+
+    context 'when user is not blocked_pending_approval' do
+      let(:user) { active_user }
+
+      it 'does nothing to the user state' do
+        subject
+
+        expect(user.reload).to be_active
+      end
+    end
+
+    context 'when user cap is set to nil' do
+      let(:new_user_signups_cap) { nil }
+
+      it 'does nothing to the user state' do
+        subject
+
+        expect(user.reload).to be_blocked_pending_approval
+      end
+    end
+
+    context 'when current billable user count is less than user cap' do
+      it 'activates the user' do
+        subject
+
+        expect(user.reload).to be_active
+      end
+
+      it 'notifies the approval to the user' do
+        expect(DeviseMailer).to receive(:user_admin_approval).with(user).and_call_original
+        expect { subject }.to have_enqueued_mail(DeviseMailer, :user_admin_approval)
+      end
+
+      context 'when user has not confirmed their email yet' do
+        let(:user) { create(:user, :blocked_pending_approval, :unconfirmed) }
+
+        it 'sends confirmation instructions' do
+          expect { subject }
+            .to have_enqueued_mail(DeviseMailer, :confirmation_instructions)
+        end
+      end
+
+      context 'when user has confirmed their email' do
+        it 'does not send a confirmation email' do
+          expect { subject }
+            .not_to have_enqueued_mail(DeviseMailer, :confirmation_instructions)
+        end
+      end
+
+      context 'when pending invitations' do
+        let!(:project_member_invite) { create(:project_member, :invited, invite_email: user.email) }
+        let!(:group_member_invite) { create(:group_member, :invited, invite_email: user.email) }
+
+        context 'when user is unconfirmed' do
+          let(:user) { create(:user, :blocked_pending_approval, :unconfirmed) }
+
+          it 'does not accept pending invites of the user' do
+            subject
+
+            group_member_invite.reload
+            project_member_invite.reload
+
+            expect(group_member_invite).to be_invite
+            expect(project_member_invite).to be_invite
+          end
+        end
+
+        context 'when user is confirmed' do
+          it 'accepts pending invites of the user' do
+            subject
+
+            group_member_invite.reload
+            project_member_invite.reload
+
+            expect(group_member_invite).not_to be_invite
+            expect(project_member_invite).not_to be_invite
+            expect(group_member_invite.user).to eq(user)
+            expect(project_member_invite.user).to eq(user)
+          end
+        end
+      end
+    end
+
+    context 'when current billable user count is equal to user cap' do
+      let(:new_user_signups_cap) { 1 }
+
+      it 'keeps the user in blocked_pending_approval state' do
+        subject
+
+        expect(user.reload).to be_blocked_pending_approval
+      end
+    end
+  end
+end