Merge branch '332455-password-expired-error-on-git-fetch-via-ssh-for-ldap-user' into 'master'

Fix Password expired error on git fetch via SSH for LDAP user

See merge request gitlab-org/gitlab!63466

app/models/user.rb

@@ -1869,6 +1869,12 @@ class User < ApplicationRecord
     !!(password_expires_at && password_expires_at < Time.current)
   end
 
+  def password_expired_if_applicable?
+    return false unless allow_password_authentication?
+
+    password_expired?
+  end
+
   def can_be_deactivated?
     active? && no_recent_activity? && !internal?
   end

app/policies/concerns/policy_actor.rb

@@ -85,7 +85,7 @@ module PolicyActor
     false
   end
 
-  def password_expired?
+  def password_expired_if_applicable?
     false
   end
 

app/policies/global_policy.rb

@@ -16,7 +16,7 @@ class GlobalPolicy < BasePolicy
   end
 
   condition(:password_expired, scope: :user) do
-    @user&.password_expired?
+    @user&.password_expired_if_applicable?
   end
 
   condition(:project_bot, scope: :user) { @user&.project_bot? }

ee/spec/requests/api/groups_spec.rb

@@ -10,7 +10,7 @@ RSpec.describe API::Groups do
   let_it_be(:project) { create(:project, group: group) }
   let_it_be(:user) { create(:user) }
   let_it_be(:another_user) { create(:user) }
-  let(:admin) { create(:admin) }
+  let_it_be(:admin) { create(:admin) }
 
   before do
     group.add_owner(user)

lib/api/members.rb

@@ -97,6 +97,8 @@ module API
         end
         # rubocop: disable CodeReuse/ActiveRecord
         post ":id/members" do
+          ::Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/-/issues/333434')
+
           source = find_source(source_type, params[:id])
           authorize_admin_source!(source_type, source)
 

lib/gitlab/auth.rb

@@ -385,7 +385,7 @@ module Gitlab
       end
 
       def can_user_login_with_non_expired_password?(user)
-        user.can?(:log_in) && !user.password_expired?
+        user.can?(:log_in) && !user.password_expired_if_applicable?
       end
     end
   end

lib/gitlab/auth/user_access_denied_reason.rb

@@ -23,6 +23,8 @@ module Gitlab
           "Your primary email address is not confirmed. "\
           "Please check your inbox for the confirmation instructions. "\
           "In case the link is expired, you can request a new confirmation email at #{Rails.application.routes.url_helpers.new_user_confirmation_url}"
+        when :blocked
+          "Your account has been blocked."
         when :password_expired
           "Your password expired. "\
           "Please access GitLab from a web browser to update your password."
@@ -44,6 +46,8 @@ module Gitlab
           :deactivated
         elsif !@user.confirmed?
           :unconfirmed
+        elsif @user.blocked?
+          :blocked
         elsif @user.password_expired?
           :password_expired
         else

lib/gitlab/lfs_token.rb

@@ -52,7 +52,7 @@ module Gitlab
     def valid_user?
       return true unless user?
 
-      !actor.blocked? && (!actor.allow_password_authentication? || !actor.password_expired?)
+      !actor.blocked? && !actor.password_expired_if_applicable?
     end
 
     def authentication_payload(repository_http_path)

spec/lib/gitlab/git_access_spec.rb

@@ -440,6 +440,14 @@ RSpec.describe Gitlab::GitAccess do
       expect { pull_access_check }.to raise_forbidden("Your password expired. Please access GitLab from a web browser to update your password.")
     end
 
+    it 'allows ldap users with expired password to pull' do
+      project.add_maintainer(user)
+      user.update!(password_expires_at: 2.minutes.ago)
+      allow(user).to receive(:ldap_user?).and_return(true)
+
+      expect { pull_access_check }.not_to raise_error
+    end
+
     context 'when the project repository does not exist' do
       before do
         project.add_guest(user)
@@ -979,12 +987,26 @@ RSpec.describe Gitlab::GitAccess do
       end
 
       it 'disallows users with expired password to push' do
-        project.add_maintainer(user)
         user.update!(password_expires_at: 2.minutes.ago)
 
         expect { push_access_check }.to raise_forbidden("Your password expired. Please access GitLab from a web browser to update your password.")
       end
 
+      it 'allows ldap users with expired password to push' do
+        user.update!(password_expires_at: 2.minutes.ago)
+        allow(user).to receive(:ldap_user?).and_return(true)
+
+        expect { push_access_check }.not_to raise_error
+      end
+
+      it 'disallows blocked ldap users with expired password to push' do
+        user.block
+        user.update!(password_expires_at: 2.minutes.ago)
+        allow(user).to receive(:ldap_user?).and_return(true)
+
+        expect { push_access_check }.to raise_forbidden("Your account has been blocked.")
+      end
+
       it 'cleans up the files' do
         expect(project.repository).to receive(:clean_stale_repository_files).and_call_original
         expect { push_access_check }.not_to raise_error

spec/models/user_spec.rb

@@ -5224,6 +5224,70 @@ RSpec.describe User do
     end
   end
 
+  describe '#password_expired_if_applicable?' do
+    let(:user) { build(:user, password_expires_at: password_expires_at) }
+
+    subject { user.password_expired_if_applicable? }
+
+    context 'when user is not ldap user' do
+      context 'when password_expires_at is not set' do
+        let(:password_expires_at) {}
+
+        it 'returns false' do
+          is_expected.to be_falsey
+        end
+      end
+
+      context 'when password_expires_at is in the past' do
+        let(:password_expires_at) { 1.minute.ago }
+
+        it 'returns true' do
+          is_expected.to be_truthy
+        end
+      end
+
+      context 'when password_expires_at is in the future' do
+        let(:password_expires_at) { 1.minute.from_now }
+
+        it 'returns false' do
+          is_expected.to be_falsey
+        end
+      end
+    end
+
+    context 'when user is ldap user' do
+      let(:user) { build(:user, password_expires_at: password_expires_at) }
+
+      before do
+        allow(user).to receive(:ldap_user?).and_return(true)
+      end
+
+      context 'when password_expires_at is not set' do
+        let(:password_expires_at) {}
+
+        it 'returns false' do
+          is_expected.to be_falsey
+        end
+      end
+
+      context 'when password_expires_at is in the past' do
+        let(:password_expires_at) { 1.minute.ago }
+
+        it 'returns false' do
+          is_expected.to be_falsey
+        end
+      end
+
+      context 'when password_expires_at is in the future' do
+        let(:password_expires_at) { 1.minute.from_now }
+
+        it 'returns false' do
+          is_expected.to be_falsey
+        end
+      end
+    end
+  end
+
   describe '#read_only_attribute?' do
     context 'when synced attributes metadata is present' do
       it 'delegates to synced_attributes_metadata' do

spec/policies/global_policy_spec.rb

@@ -245,6 +245,14 @@ RSpec.describe GlobalPolicy do
       end
 
       it { is_expected.not_to be_allowed(:access_api) }
+
+      context 'when user is using ldap' do
+        before do
+          allow(current_user).to receive(:ldap_user?).and_return(true)
+        end
+
+        it { is_expected.to be_allowed(:access_api) }
+      end
     end
 
     context 'when terms are enforced' do
@@ -433,6 +441,14 @@ RSpec.describe GlobalPolicy do
       end
 
       it { is_expected.not_to be_allowed(:access_git) }
+
+      context 'when user is using ldap' do
+        before do
+          allow(current_user).to receive(:ldap_user?).and_return(true)
+        end
+
+        it { is_expected.to be_allowed(:access_git) }
+      end
     end
   end
 
@@ -517,6 +533,14 @@ RSpec.describe GlobalPolicy do
       end
 
       it { is_expected.not_to be_allowed(:use_slash_commands) }
+
+      context 'when user is using ldap' do
+        before do
+          allow(current_user).to receive(:ldap_user?).and_return(true)
+        end
+
+        it { is_expected.to be_allowed(:use_slash_commands) }
+      end
     end
   end
 

spec/requests/api/graphql/mutations/merge_requests/set_assignees_spec.rb

@@ -68,7 +68,7 @@ RSpec.describe 'Setting assignees of a merge request', :assume_throttled do
 
   context 'when the current user does not have permission to add assignees' do
     let(:current_user) { create(:user) }
-    let(:db_query_limit) { 27 }
+    let(:db_query_limit) { 28 }
 
     it 'does not change the assignees' do
       project.add_guest(current_user)
@@ -80,7 +80,7 @@ RSpec.describe 'Setting assignees of a merge request', :assume_throttled do
   end
 
   context 'with assignees already assigned' do
-    let(:db_query_limit) { 39 }
+    let(:db_query_limit) { 46 }
 
     before do
       merge_request.assignees = [assignee2]
@@ -96,7 +96,7 @@ RSpec.describe 'Setting assignees of a merge request', :assume_throttled do
   end
 
   context 'when passing an empty list of assignees' do
-    let(:db_query_limit) { 31 }
+    let(:db_query_limit) { 32 }
     let(:input) { { assignee_usernames: [] } }
 
     before do
@@ -115,7 +115,7 @@ RSpec.describe 'Setting assignees of a merge request', :assume_throttled do
   context 'when passing append as true' do
     let(:mode) { Types::MutationOperationModeEnum.enum[:append] }
     let(:input) { { assignee_usernames: [assignee2.username], operation_mode: mode } }
-    let(:db_query_limit) { 20 }
+    let(:db_query_limit) { 22 }
 
     before do
       # In CE, APPEND is a NOOP as you can't have multiple assignees
@@ -135,7 +135,7 @@ RSpec.describe 'Setting assignees of a merge request', :assume_throttled do
   end
 
   context 'when passing remove as true' do
-    let(:db_query_limit) { 31 }
+    let(:db_query_limit) { 32 }
     let(:mode) { Types::MutationOperationModeEnum.enum[:remove] }
     let(:input) { { assignee_usernames: [assignee.username], operation_mode: mode } }
     let(:expected_result) { [] }

spec/requests/api/import_bitbucket_server_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 RSpec.describe API::ImportBitbucketServer do
   let(:base_uri) { "https://test:7990" }
-  let(:user) { create(:user) }
+  let(:user) { create(:user, bio: 'test') }
   let(:token) { "asdasd12345" }
   let(:secret) { "sekrettt" }
   let(:project_key) { 'TES' }

spec/requests/git_http_spec.rb

@@ -36,16 +36,6 @@ RSpec.describe 'Git HTTP requests' do
           end
         end
 
-        context "when password is expired" do
-          it "responds to downloads with status 401 Unauthorized" do
-            user.update!(password_expires_at: 2.days.ago)
-
-            download(path, user: user.username, password: user.password) do |response|
-              expect(response).to have_gitlab_http_status(:unauthorized)
-            end
-          end
-        end
-
         context "when user is blocked" do
           let(:user) { create(:user, :blocked) }
 
@@ -68,6 +58,26 @@ RSpec.describe 'Git HTTP requests' do
     end
   end
 
+  shared_examples 'operations are not allowed with expired password' do
+    context "when password is expired" do
+      it "responds to downloads with status 401 Unauthorized" do
+        user.update!(password_expires_at: 2.days.ago)
+
+        download(path, user: user.username, password: user.password) do |response|
+          expect(response).to have_gitlab_http_status(:unauthorized)
+        end
+      end
+
+      it "responds to uploads with status 401 Unauthorized" do
+        user.update!(password_expires_at: 2.days.ago)
+
+        upload(path, user: user.username, password: user.password) do |response|
+          expect(response).to have_gitlab_http_status(:unauthorized)
+        end
+      end
+    end
+  end
+
   shared_examples 'pushes require Basic HTTP Authentication' do
     context "when no credentials are provided" do
       it "responds to uploads with status 401 Unauthorized (no project existence information leak)" do
@@ -95,15 +105,6 @@ RSpec.describe 'Git HTTP requests' do
             expect(response.header['WWW-Authenticate']).to start_with('Basic ')
           end
         end
-
-        context "when password is expired" do
-          it "responds to uploads with status 401 Unauthorized" do
-            user.update!(password_expires_at: 2.days.ago)
-            upload(path, user: user.username, password: user.password) do |response|
-              expect(response).to have_gitlab_http_status(:unauthorized)
-            end
-          end
-        end
       end
 
       context "when authentication succeeds" do
@@ -212,6 +213,7 @@ RSpec.describe 'Git HTTP requests' do
 
         it_behaves_like 'pulls require Basic HTTP Authentication'
         it_behaves_like 'pushes require Basic HTTP Authentication'
+        it_behaves_like 'operations are not allowed with expired password'
 
         context 'when authenticated' do
           it 'rejects downloads and uploads with 404 Not Found' do
@@ -306,6 +308,7 @@ RSpec.describe 'Git HTTP requests' do
 
         it_behaves_like 'pulls require Basic HTTP Authentication'
         it_behaves_like 'pushes require Basic HTTP Authentication'
+        it_behaves_like 'operations are not allowed with expired password'
 
         context 'when authenticated' do
           context 'and as a developer on the team' do
@@ -473,6 +476,7 @@ RSpec.describe 'Git HTTP requests' do
 
             it_behaves_like 'pulls require Basic HTTP Authentication'
             it_behaves_like 'pushes require Basic HTTP Authentication'
+            it_behaves_like 'operations are not allowed with expired password'
           end
 
           context 'but the repo is enabled' do
@@ -488,6 +492,7 @@ RSpec.describe 'Git HTTP requests' do
 
             it_behaves_like 'pulls require Basic HTTP Authentication'
             it_behaves_like 'pushes require Basic HTTP Authentication'
+            it_behaves_like 'operations are not allowed with expired password'
           end
         end
 
@@ -508,6 +513,7 @@ RSpec.describe 'Git HTTP requests' do
 
         it_behaves_like 'pulls require Basic HTTP Authentication'
         it_behaves_like 'pushes require Basic HTTP Authentication'
+        it_behaves_like 'operations are not allowed with expired password'
 
         context "when username and password are provided" do
           let(:env) { { user: user.username, password: 'nope' } }
@@ -1003,6 +1009,24 @@ RSpec.describe 'Git HTTP requests' do
 
           it_behaves_like 'pulls are allowed'
           it_behaves_like 'pushes are allowed'
+
+          context "when password is expired" do
+            it "responds to downloads with status 200" do
+              user.update!(password_expires_at: 2.days.ago)
+
+              download(path, user: user.username, password: user.password) do |response|
+                expect(response).to have_gitlab_http_status(:ok)
+              end
+            end
+
+            it "responds to uploads with status 200" do
+              user.update!(password_expires_at: 2.days.ago)
+
+              upload(path, user: user.username, password: user.password) do |response|
+                expect(response).to have_gitlab_http_status(:ok)
+              end
+            end
+          end
         end
       end
     end

spec/services/repositories/changelog_service_spec.rb

@@ -76,7 +76,7 @@ RSpec.describe Repositories::ChangelogService do
       recorder = ActiveRecord::QueryRecorder.new { service.execute }
       changelog = project.repository.blob_at('master', 'CHANGELOG.md')&.data
 
-      expect(recorder.count).to eq(11)
+      expect(recorder.count).to eq(12)
       expect(changelog).to include('Title 1', 'Title 2')
     end