Prevent login for blocked and internal users

Prevent users who do not have the ability to sign in
(read internal, blocked and inactive users)
from signing in.
Also show appropriate message when such users
try logging in to the system.

app/models/user.rb

@@ -58,6 +58,8 @@ class User < ApplicationRecord
 
   BLOCKED_MESSAGE = "Your account has been blocked. Please contact your GitLab " \
                     "administrator if you think this is an error."
+  LOGIN_FORBIDDEN = "Your account does not have the required permission to login. Please contact your GitLab " \
+                    "administrator if you think this is an error."
 
   MINIMUM_INACTIVE_DAYS = 180
 
@@ -299,14 +301,6 @@ class User < ApplicationRecord
       def blocked?
         true
       end
-
-      def active_for_authentication?
-        false
-      end
-
-      def inactive_message
-        BLOCKED_MESSAGE
-      end
     end
 
     before_transition do
@@ -354,6 +348,20 @@ class User < ApplicationRecord
             .expiring_and_not_notified(at).select(1))
   end
 
+  def active_for_authentication?
+    super && can?(:log_in)
+  end
+
+  def inactive_message
+    if blocked?
+      BLOCKED_MESSAGE
+    elsif internal?
+      LOGIN_FORBIDDEN
+    else
+      super
+    end
+  end
+
   def self.with_visible_profile(user)
     return with_public_profile if user.nil?
 
@@ -1708,6 +1716,10 @@ class User < ApplicationRecord
     members.non_request.maximum(:access_level)
   end
 
+  def confirmation_required_on_sign_in?
+    !confirmed? && !confirmation_period_valid?
+  end
+
   protected
 
   # override, from Devise::Validatable

app/policies/base_policy.rb

@@ -25,8 +25,7 @@ class BasePolicy < DeclarativePolicy::Base
   with_options scope: :user, score: 0
   condition(:inactive) do
     Feature.enabled?(:inactive_policy_condition, default_enabled: true) &&
-      @user &&
-      !@user&.active_for_authentication?
+      @user&.confirmation_required_on_sign_in? || @user&.access_locked?
   end
 
   with_options scope: :user, score: 0

ee/spec/controllers/groups/analytics/productivity_analytics_controller_spec.rb

@@ -39,6 +39,7 @@ describe Groups::Analytics::ProductivityAnalyticsController do
 
     context 'when user is not authorized to view productivity analytics' do
       before do
+        expect(Ability).to receive(:allowed?).with(current_user, :log_in, :global).and_call_original
         expect(Ability).to receive(:allowed?).with(current_user, :read_group, group).and_return(true)
         expect(Ability).to receive(:allowed?).with(current_user, :view_productivity_analytics, group).and_return(false)
       end

ee/spec/controllers/projects/boards_controller_spec.rb

@@ -102,6 +102,7 @@ describe Projects::BoardsController do
 
       context 'with unauthorized user' do
         before do
+          expect(Ability).to receive(:allowed?).with(user, :log_in, :global).and_call_original
           allow(Ability).to receive(:allowed?).with(user, :read_project, project).and_return(true)
           allow(Ability).to receive(:allowed?).with(user, :admin_board, project).and_return(false)
         end
@@ -178,6 +179,7 @@ describe Projects::BoardsController do
 
     context 'with unauthorized user' do
       before do
+        expect(Ability).to receive(:allowed?).with(user, :log_in, :global).and_call_original
         allow(Ability).to receive(:allowed?).with(user, :read_project, project).and_return(true)
         allow(Ability).to receive(:allowed?).with(user, :admin_board, project).and_return(false)
       end
@@ -227,6 +229,7 @@ describe Projects::BoardsController do
 
     context 'with unauthorized user' do
       before do
+        expect(Ability).to receive(:allowed?).with(user, :log_in, :global).and_call_original
         allow(Ability).to receive(:allowed?).with(user, :read_project, project).and_return(true)
         allow(Ability).to receive(:allowed?).with(user, :admin_board, project).and_return(false)
       end

ee/spec/models/user_spec.rb

@@ -1005,4 +1005,28 @@ describe User do
       it { is_expected.to eq [free_group_a, free_group_z] }
     end
   end
+
+  describe '#active_for_authentication?' do
+    subject { user.active_for_authentication? }
+
+    let(:user) { create(:user) }
+
+    context 'based on user type' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:user_type, :expected_result) do
+        'service_user'      | true
+        'support_bot'       | false
+        'visual_review_bot' | false
+      end
+
+      with_them do
+        before do
+          user.update(user_type: user_type)
+        end
+
+        it { is_expected.to be expected_result }
+      end
+    end
+  end
 end

spec/controllers/concerns/controller_with_cross_project_access_check_spec.rb

@@ -14,6 +14,7 @@ describe ControllerWithCrossProjectAccessCheck do
   context 'When reading cross project is not allowed' do
     before do
       allow(Ability).to receive(:allowed).and_call_original
+      expect(Ability).to receive(:allowed?).with(user, :log_in, :global).and_call_original
       allow(Ability).to receive(:allowed?)
                           .with(user, :read_cross_project, :global)
                           .and_return(false)

spec/controllers/graphql_controller_spec.rb

@@ -46,6 +46,7 @@ describe GraphqlController do
         # User cannot access API in a couple of cases
         # * When user is internal(like ghost users)
         # * When user is blocked
+        expect(Ability).to receive(:allowed?).with(user, :log_in, :global).and_call_original
         expect(Ability).to receive(:allowed?).with(user, :access_api, :global).and_return(false)
 
         post :execute

spec/controllers/groups/boards_controller_spec.rb

@@ -26,6 +26,7 @@ describe Groups::BoardsController do
 
       context 'with unauthorized user' do
         before do
+          expect(Ability).to receive(:allowed?).with(user, :log_in, :global).and_call_original
           allow(Ability).to receive(:allowed?).with(user, :read_cross_project, :global).and_return(true)
           allow(Ability).to receive(:allowed?).with(user, :read_group, group).and_return(true)
           allow(Ability).to receive(:allowed?).with(user, :read_board, group).and_return(false)
@@ -70,6 +71,7 @@ describe Groups::BoardsController do
 
       context 'with unauthorized user' do
         before do
+          expect(Ability).to receive(:allowed?).with(user, :log_in, :global).and_call_original
           allow(Ability).to receive(:allowed?).with(user, :read_cross_project, :global).and_return(true)
           allow(Ability).to receive(:allowed?).with(user, :read_group, group).and_return(true)
           allow(Ability).to receive(:allowed?).with(user, :read_board, group).and_return(false)
@@ -106,6 +108,7 @@ describe Groups::BoardsController do
 
       context 'with unauthorized user' do
         before do
+          expect(Ability).to receive(:allowed?).with(user, :log_in, :global).and_call_original
           allow(Ability).to receive(:allowed?).with(user, :read_cross_project, :global).and_return(true)
           allow(Ability).to receive(:allowed?).with(user, :read_group, group).and_return(true)
           allow(Ability).to receive(:allowed?).with(user, :read_board, group).and_return(false)
@@ -144,6 +147,7 @@ describe Groups::BoardsController do
 
       context 'with unauthorized user' do
         before do
+          expect(Ability).to receive(:allowed?).with(user, :log_in, :global).and_call_original
           allow(Ability).to receive(:allowed?).with(user, :read_cross_project, :global).and_return(true)
           allow(Ability).to receive(:allowed?).with(user, :read_group, group).and_return(true)
           allow(Ability).to receive(:allowed?).with(user, :read_group, group).and_return(false)

spec/controllers/projects/boards_controller_spec.rb

@@ -32,6 +32,7 @@ describe Projects::BoardsController do
 
       context 'with unauthorized user' do
         before do
+          expect(Ability).to receive(:allowed?).with(user, :log_in, :global).and_call_original
           allow(Ability).to receive(:allowed?).with(user, :read_project, project).and_return(true)
           allow(Ability).to receive(:allowed?).with(user, :read_board, project).and_return(false)
         end
@@ -75,6 +76,7 @@ describe Projects::BoardsController do
 
       context 'with unauthorized user' do
         before do
+          expect(Ability).to receive(:allowed?).with(user, :log_in, :global).and_call_original
           allow(Ability).to receive(:allowed?).with(user, :read_project, project).and_return(true)
           allow(Ability).to receive(:allowed?).with(user, :read_board, project).and_return(false)
         end
@@ -130,6 +132,7 @@ describe Projects::BoardsController do
 
       context 'with unauthorized user' do
         before do
+          expect(Ability).to receive(:allowed?).with(user, :log_in, :global).and_call_original
           allow(Ability).to receive(:allowed?).with(user, :read_project, project).and_return(true)
           allow(Ability).to receive(:allowed?).with(user, :read_board, project).and_return(false)
         end
@@ -167,6 +170,7 @@ describe Projects::BoardsController do
 
       context 'with unauthorized user' do
         before do
+          expect(Ability).to receive(:allowed?).with(user, :log_in, :global).and_call_original
           allow(Ability).to receive(:allowed?).with(user, :read_project, project).and_return(true)
           allow(Ability).to receive(:allowed?).with(user, :read_board, project).and_return(false)
         end

spec/models/user_spec.rb

@@ -4493,4 +4493,73 @@ describe User, :do_not_mock_admin_mode do
       end
     end
   end
+
+  describe '#active_for_authentication?' do
+    subject { user.active_for_authentication? }
+
+    let(:user) { create(:user) }
+
+    context 'when user is blocked' do
+      before do
+        user.block
+      end
+
+      it { is_expected.to be false }
+    end
+
+    context 'when user is a ghost user' do
+      before do
+        user.update(ghost: true)
+      end
+
+      it { is_expected.to be false }
+    end
+
+    context 'based on user type' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:user_type, :expected_result) do
+        'human'             | true
+        'alert_bot'         | false
+      end
+
+      with_them do
+        before do
+          user.update(user_type: user_type)
+        end
+
+        it { is_expected.to be expected_result }
+      end
+    end
+  end
+
+  describe '#inactive_message' do
+    subject { user.inactive_message }
+
+    let(:user) { create(:user) }
+
+    context 'when user is blocked' do
+      before do
+        user.block
+      end
+
+      it { is_expected.to eq User::BLOCKED_MESSAGE }
+    end
+
+    context 'when user is an internal user' do
+      before do
+        user.update(ghost: true)
+      end
+
+      it { is_expected.to be User::LOGIN_FORBIDDEN }
+    end
+
+    context 'when user is locked' do
+      before do
+        user.lock_access!
+      end
+
+      it { is_expected.to be :locked }
+    end
+  end
 end