Merge branch 'fix-login-policy' into 'master'

Prevent login for users that don't have the required permission

See merge request gitlab-org/gitlab!28458

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?
 
@@ -1701,6 +1709,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

@@ -4475,4 +4475,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