Merge branch '24462-reduce_ldap_queries_for_lfs' into 'master'

Reduce hits to LDAP on Git HTTP auth by reordering auth mechanisms

Closes #24462

See merge request !8752

3 files changed, 82 insertions, 29 deletions

diff --git a/changelogs/unreleased/24462-reduce_ldap_queries_for_lfs.yml b/changelogs/unreleased/24462-reduce_ldap_queries_for_lfs.yml
new file mode 100644
index 00000000000..05fbd8f0bf2
--- /dev/null
+++ b/changelogs/unreleased/24462-reduce_ldap_queries_for_lfs.yml
@@ -0,0 +1,4 @@
+---
+title: Reduce hits to LDAP on Git HTTP auth by reordering auth mechanisms
+merge_request: 8752
+author:
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 8dda65c71ef..f638905a1e0 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -10,13 +10,16 @@ module Gitlab
       def find_for_git_client(login, password, project:, ip:)
         raise "Must provide an IP for rate limiting" if ip.nil?
 
+        # `user_with_password_for_git` should be the last check
+        # because it's the most expensive, especially when LDAP
+        # is enabled.
         result =
           service_request_check(login, password, project) ||
           build_access_token_check(login, password) ||
-          user_with_password_for_git(login, password) ||
-          oauth_access_token_check(login, password) ||
           lfs_token_check(login, password) ||
+          oauth_access_token_check(login, password) ||
           personal_access_token_check(login, password) ||
+          user_with_password_for_git(login, password) ||
           Gitlab::Auth::Result.new
 
         rate_limit!(ip, success: result.success?, login: login)
@@ -143,7 +146,9 @@ module Gitlab
             read_authentication_abilities
           end
 
-        Result.new(actor, nil, token_handler.type, authentication_abilities) if Devise.secure_compare(token_handler.token, password)
+        if Devise.secure_compare(token_handler.token, password)
+          Gitlab::Auth::Result.new(actor, nil, token_handler.type, authentication_abilities)
+        end
       end
 
       def build_access_token_check(login, password)
diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb
index f251c0dd25a..b234de4c772 100644
--- a/spec/lib/gitlab/auth_spec.rb
+++ b/spec/lib/gitlab/auth_spec.rb
@@ -58,58 +58,102 @@ describe Gitlab::Auth, lib: true do
       expect(gl_auth.find_for_git_client(user.username, 'password', project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(user, nil, :gitlab_or_ldap, full_authentication_abilities))
     end
 
-    it 'recognizes user lfs tokens' do
-      user = create(:user)
-      token = Gitlab::LfsToken.new(user).token
+    context 'while using LFS authenticate' do
+      it 'recognizes user lfs tokens' do
+        user = create(:user)
+        token = Gitlab::LfsToken.new(user).token
 
-      expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: user.username)
-      expect(gl_auth.find_for_git_client(user.username, token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(user, nil, :lfs_token, full_authentication_abilities))
-    end
+        expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: user.username)
+        expect(gl_auth.find_for_git_client(user.username, token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(user, nil, :lfs_token, full_authentication_abilities))
+      end
 
-    it 'recognizes deploy key lfs tokens' do
-      key = create(:deploy_key)
-      token = Gitlab::LfsToken.new(key).token
+      it 'recognizes deploy key lfs tokens' do
+        key = create(:deploy_key)
+        token = Gitlab::LfsToken.new(key).token
 
-      expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: "lfs+deploy-key-#{key.id}")
-      expect(gl_auth.find_for_git_client("lfs+deploy-key-#{key.id}", token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(key, nil, :lfs_deploy_token, read_authentication_abilities))
-    end
+        expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: "lfs+deploy-key-#{key.id}")
+        expect(gl_auth.find_for_git_client("lfs+deploy-key-#{key.id}", token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(key, nil, :lfs_deploy_token, read_authentication_abilities))
+      end
 
-    context "while using OAuth tokens as passwords" do
-      it 'succeeds for OAuth tokens with the `api` scope' do
+      it 'does not try password auth before oauth' do
         user = create(:user)
-        application = Doorkeeper::Application.create!(name: "MyApp", redirect_uri: "https://app.com", owner: user)
-        token = Doorkeeper::AccessToken.create!(application_id: application.id, resource_owner_id: user.id, scopes: "api")
+        token = Gitlab::LfsToken.new(user).token
+
+        expect(gl_auth).not_to receive(:find_with_user_password)
 
+        gl_auth.find_for_git_client(user.username, token, project: nil, ip: 'ip')
+      end
+    end
+
+    context 'while using OAuth tokens as passwords' do
+      let(:user) { create(:user) }
+      let(:token_w_api_scope) { Doorkeeper::AccessToken.create!(application_id: application.id, resource_owner_id: user.id, scopes: 'api') }
+      let(:application) { Doorkeeper::Application.create!(name: 'MyApp', redirect_uri: 'https://app.com', owner: user) }
+
+      it 'succeeds for OAuth tokens with the `api` scope' do
         expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: 'oauth2')
-        expect(gl_auth.find_for_git_client("oauth2", token.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(user, nil, :oauth, read_authentication_abilities))
+        expect(gl_auth.find_for_git_client("oauth2", token_w_api_scope.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(user, nil, :oauth, read_authentication_abilities))
       end
 
       it 'fails for OAuth tokens with other scopes' do
-        user = create(:user)
-        application = Doorkeeper::Application.create!(name: "MyApp", redirect_uri: "https://app.com", owner: user)
-        token = Doorkeeper::AccessToken.create!(application_id: application.id, resource_owner_id: user.id, scopes: "read_user")
+        token = Doorkeeper::AccessToken.create!(application_id: application.id, resource_owner_id: user.id, scopes: 'read_user')
 
         expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: 'oauth2')
         expect(gl_auth.find_for_git_client("oauth2", token.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(nil, nil))
       end
+
+      it 'does not try password auth before oauth' do
+        expect(gl_auth).not_to receive(:find_with_user_password)
+
+        gl_auth.find_for_git_client("oauth2", token_w_api_scope.token, project: nil, ip: 'ip')
+      end
     end
 
-    context "while using personal access tokens as passwords" do
-      it 'succeeds for personal access tokens with the `api` scope' do
-        user = create(:user)
-        personal_access_token = create(:personal_access_token, user: user, scopes: ['api'])
+    context 'while using personal access tokens as passwords' do
+      let(:user) { create(:user) }
+      let(:token_w_api_scope) { create(:personal_access_token, user: user, scopes: ['api']) }
 
+      it 'succeeds for personal access tokens with the `api` scope' do
         expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: user.email)
-        expect(gl_auth.find_for_git_client(user.email, personal_access_token.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(user, nil, :personal_token, full_authentication_abilities))
+        expect(gl_auth.find_for_git_client(user.email, token_w_api_scope.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(user, nil, :personal_token, full_authentication_abilities))
       end
 
       it 'fails for personal access tokens with other scopes' do
-        user = create(:user)
         personal_access_token = create(:personal_access_token, user: user, scopes: ['read_user'])
 
         expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: user.email)
         expect(gl_auth.find_for_git_client(user.email, personal_access_token.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(nil, nil))
       end
+
+      it 'does not try password auth before personal access tokens' do
+        expect(gl_auth).not_to receive(:find_with_user_password)
+
+        gl_auth.find_for_git_client(user.email, token_w_api_scope.token, project: nil, ip: 'ip')
+      end
+    end
+
+    context 'while using regular user and password' do
+      it 'falls through lfs authentication' do
+        user = create(
+          :user,
+          username: 'normal_user',
+          password: 'my-secret',
+        )
+
+        expect(gl_auth.find_for_git_client(user.username, user.password, project: nil, ip: 'ip'))
+          .to eq(Gitlab::Auth::Result.new(user, nil, :gitlab_or_ldap, full_authentication_abilities))
+      end
+
+      it 'falls through oauth authentication when the username is oauth2' do
+        user = create(
+          :user,
+          username: 'oauth2',
+          password: 'my-secret',
+        )
+
+        expect(gl_auth.find_for_git_client(user.username, user.password, project: nil, ip: 'ip'))
+          .to eq(Gitlab::Auth::Result.new(user, nil, :gitlab_or_ldap, full_authentication_abilities))
+      end
     end
 
     it 'returns double nil for invalid credentials' do