Merge branch '338980-oauth-and-user-cap' into 'master'

Fix user cap evaluation for OAuth and LDAP login

See merge request gitlab-org/gitlab!81272

ee/app/models/ee/user.rb

@@ -409,13 +409,13 @@ module EE
     end
 
     def activate_based_on_user_cap?
-      !blocked_auto_created_omniauth_user? &&
+      !blocked_auto_created_oauth_ldap_user? &&
         blocked_pending_approval? &&
         self.class.user_cap_max.present?
     end
 
-    def blocked_auto_created_omniauth_user?
-      ::Gitlab.config.omniauth.block_auto_created_users && identities.any?
+    def blocked_auto_created_oauth_ldap_user?
+      identities.any? && block_auto_created_users?
     end
 
     def has_valid_credit_card?
@@ -439,6 +439,17 @@ module EE
 
     private
 
+    def block_auto_created_users?
+      if ldap_user?
+        provider = ldap_identity.provider
+        return false unless provider
+
+        ::Gitlab::Auth::Ldap::Config.new(provider).block_auto_created_users
+      else
+        ::Gitlab.config.omniauth.block_auto_created_users
+      end
+    end
+
     def created_after_credit_card_release_day?(project)
       created_at >= ::Users::CreditCardValidation::RELEASE_DAY ||
         ::Feature.enabled?(:ci_require_credit_card_for_old_users, project, default_enabled: :yaml)

ee/lib/ee/gitlab/auth/ldap/user.rb

@@ -9,26 +9,12 @@ module EE
     module Auth
       module Ldap
         module User
-          extend ::Gitlab::Utils::Override
-
           def initialize(auth_hash)
             super
 
             set_external_with_external_groups
           end
 
-          override :find_user
-          def find_user
-            user = super
-
-            if activate_user_based_on_user_cap?(user)
-              user.activate
-              log_user_changes(user, 'LDAP', "user cap not reached yet, unblocking")
-            end
-
-            user
-          end
-
           private
 
           # Intended to be called during #initialize, and #save should be called

ee/lib/ee/gitlab/auth/o_auth/user.rb

@@ -7,6 +7,13 @@ module EE
         module User
           protected
 
+          def activate_user_if_user_cap_not_reached
+            if activate_user_based_on_user_cap?(gl_user)
+              gl_user.activate
+              log_user_changes(gl_user, protocol_name, "user cap not reached yet, unblocking")
+            end
+          end
+
           def find_ldap_person(auth_hash, adapter)
             if auth_hash.provider == 'kerberos'
               ::Gitlab::Auth::Ldap::Person.find_by_kerberos_principal(auth_hash.uid, adapter)

ee/lib/ee/gitlab/auth/saml/user.rb

@@ -13,7 +13,6 @@ module EE
 
             if user_in_required_group?
               unblock_user(user, "in required group") if user&.persisted? && user&.ldap_blocked?
-              unblock_user(user, "user cap not reached yet") if activate_user_based_on_user_cap?(user)
             elsif user&.persisted?
               block_user(user, "not in required group") unless user.blocked?
             else

ee/spec/lib/gitlab/auth/o_auth/user_spec.rb

@@ -44,5 +44,66 @@ RSpec.describe Gitlab::Auth::OAuth::User do
 
       expect(gl_user.email).to eq(real_email)
     end
+
+    describe '#save' do
+      let(:user) { build(:omniauth_user, :blocked_pending_approval) }
+
+      before do
+        allow(oauth_user).to receive(:gl_user).and_return(user)
+      end
+
+      subject(:save_user) { oauth_user.save } # rubocop: disable Rails/SaveBang
+
+      describe '#activate_user_if_user_cap_not_reached' do
+        context 'when a user can be activated based on user cap' do
+          before do
+            allow(user).to receive(:activate_based_on_user_cap?).and_return(true)
+          end
+
+          context 'when the user cap has not been reached yet' do
+            it 'activates the user' do
+              allow(::User).to receive(:user_cap_reached?).and_return(false)
+              expect(oauth_user).to receive(:log_user_changes).with(
+                user, 'OAuth', 'user cap not reached yet, unblocking'
+              )
+
+              expect do
+                save_user
+                user.reload
+              end.to change { user.state }.from('blocked_pending_approval').to('active')
+            end
+          end
+
+          context 'when the user cap has been reached' do
+            it 'leaves the user as blocked' do
+              allow(::User).to receive(:user_cap_reached?).and_return(true)
+              expect(oauth_user).not_to receive(:log_user_changes)
+
+              expect do
+                save_user
+                user.reload
+              end.not_to change { user.state }
+              expect(user.state).to eq('blocked_pending_approval')
+            end
+          end
+        end
+
+        context 'when a user cannot be activated based on user cap' do
+          before do
+            allow(user).to receive(:activate_based_on_user_cap?).and_return(false)
+          end
+
+          it 'leaves the user as blocked' do
+            expect(oauth_user).not_to receive(:log_user_changes)
+
+            expect do
+              save_user
+              user.reload
+            end.not_to change { user.state }
+            expect(user.state).to eq('blocked_pending_approval')
+          end
+        end
+      end
+    end
   end
 end

ee/spec/models/ee/user_spec.rb

@@ -1923,7 +1923,7 @@ RSpec.describe User do
     end
   end
 
-  describe '#blocked_auto_created_omniauth_user?' do
+  describe '#blocked_auto_created_oauth_ldap_user?' do
     context 'when the auto-creation of an omniauth user is blocked' do
       before do
         stub_omniauth_setting(block_auto_created_users: true)
@@ -1933,7 +1933,7 @@ RSpec.describe User do
         it 'is true' do
           omniauth_user = create(:omniauth_user)
 
-          expect(omniauth_user.blocked_auto_created_omniauth_user?).to be_truthy
+          expect(omniauth_user.blocked_auto_created_oauth_ldap_user?).to be_truthy
         end
       end
 
@@ -1941,7 +1941,37 @@ RSpec.describe User do
         it 'is false' do
           user = build(:user)
 
-          expect(user.blocked_auto_created_omniauth_user?).to be_falsey
+          expect(user.blocked_auto_created_oauth_ldap_user?).to be_falsey
+        end
+      end
+
+      context 'when the config for auto-creation of LDAP user is set' do
+        let(:ldap_user) { create(:omniauth_user, :ldap) }
+
+        before do
+          allow_next_instance_of(::Gitlab::Auth::Ldap::Config) do |config|
+            allow(config).to receive_messages(block_auto_created_users: ldap_auto_create_blocked)
+          end
+        end
+
+        subject(:blocked_user?) { ldap_user.blocked_auto_created_oauth_ldap_user? }
+
+        context 'when it blocks the creation of a LDAP user' do
+          let(:ldap_auto_create_blocked) { true }
+
+          it { is_expected.to be_truthy }
+
+          context 'when no provider is linked to the user' do
+            let(:ldap_user) { create(:user) }
+
+            it { is_expected.to be_falsey }
+          end
+        end
+
+        context 'when it does not block the creation of a LDAP user' do
+          let(:ldap_auto_create_blocked) { false }
+
+          it { is_expected.to be_falsey }
         end
       end
     end
@@ -2041,7 +2071,7 @@ RSpec.describe User do
 
     with_them do
       before do
-        allow(user).to receive(:blocked_auto_created_omniauth_user?).and_return(blocked_auto_created_omniauth)
+        allow(user).to receive(:blocked_auto_created_oauth_ldap_user?).and_return(blocked_auto_created_omniauth)
         allow(user).to receive(:blocked_pending_approval?).and_return(blocked_pending_approval)
         allow(described_class.user_cap_max).to receive(:present?).and_return(user_cap_max_present)
       end

ee/spec/support/shared_examples/auth/access_protocol_examples.rb

@@ -22,12 +22,10 @@ RSpec.shared_examples 'finding user when user cap is set' do
     context 'when the user cap has not been reached' do
       let(:new_user_signups_cap) { 100 }
 
-      before do
-        stub_omniauth_setting(block_auto_created_users: block)
-      end
-
       context 'when the user can be activated based on user cap' do
-        let(:block) { false }
+        before do
+          stub_omniauth_setting(block_auto_created_users: false)
+        end
 
         it 'activates the user' do
           o_auth_user.save # rubocop:disable Rails/SaveBang
@@ -39,19 +37,23 @@ RSpec.shared_examples 'finding user when user cap is set' do
           it 'does not activate the user' do
             allow(::User).to receive(:user_cap_reached?).and_raise(ActiveRecord::QueryAborted)
 
-            o_auth_user.save # rubocop:disable Rails/SaveBang
-
             expect(::Gitlab::ErrorTracking).to receive(:track_exception).with(
               instance_of(ActiveRecord::QueryAborted),
               user_email: o_auth_user.gl_user.email
             )
             expect(o_auth_user.find_user).to be_blocked
+
+            o_auth_user.save # rubocop:disable Rails/SaveBang
           end
         end
       end
 
       context 'when the user cannot be activated based on user cap' do
-        let(:block) { true }
+        before do
+          allow_next_instance_of(::Gitlab::Auth::Ldap::Config) do |config|
+            allow(config).to receive_messages(block_auto_created_users: true)
+          end
+        end
 
         it 'does not activate the user' do
           o_auth_user.save # rubocop:disable Rails/SaveBang

lib/gitlab/auth/o_auth/user.rb

@@ -55,6 +55,7 @@ module Gitlab
           Users::UpdateService.new(gl_user, user: gl_user).execute!
 
           gl_user.block_pending_approval if block_after_save
+          activate_user_if_user_cap_not_reached
 
           log.info "(#{provider}) saving user #{auth_hash.email} from login with admin => #{gl_user.admin}, extern_uid => #{auth_hash.uid}"
           gl_user
@@ -102,6 +103,10 @@ module Gitlab
 
         protected
 
+        def activate_user_if_user_cap_not_reached
+          nil
+        end
+
         def should_save?
           true
         end