Merge branch 'fix/ldap_wihtout_user' into 'master'

changelogs/unreleased/44608-Cloning-a-repository-over-HTTPS-with-LDAP-credentials-causes-a-HTTP-401-Access-denied.yml

+---
+title: 'Cloning a repository over HTTPS with LDAP credentials causes a HTTP 401 Access denied'
+merge_request: !17988
+author: Horatiu Eugen Vlad
+type: fixed

lib/gitlab/auth.rb

@@ -69,7 +69,11 @@ module Gitlab
 
           authenticators.compact!
 
-          user if authenticators.find { |auth| auth.login(login, password) }
+          # return found user that was authenticated first for given login credentials
+          authenticators.find do |auth|
+            authenticated_user = auth.login(login, password)
+            break authenticated_user if authenticated_user
+          end
         end
       end
 

lib/gitlab/auth/database/authentication.rb

@@ -8,7 +8,7 @@ module Gitlab
         def login(login, password)
           return false unless Gitlab::CurrentSettings.password_authentication_enabled_for_git?
 
-          user&.valid_password?(password)
+          return user if user&.valid_password?(password)
         end
       end
     end

lib/gitlab/auth/ldap/authentication.rb

@@ -12,30 +12,26 @@ module Gitlab
           return unless Gitlab::Auth::LDAP::Config.enabled?
           return unless login.present? && password.present?
 
-          auth = nil
-          # loop through providers until valid bind
+          # return found user that was authenticated by first provider for given login credentials
           providers.find do |provider|
             auth = new(provider)
-            auth.login(login, password) # true will exit the loop
+            break auth.user if auth.login(login, password) # true will exit the loop
           end
-
-          # If (login, password) was invalid for all providers, the value of auth is now the last
-          # Gitlab::Auth::LDAP::Authentication instance we tried.
-          auth.user
         end
 
         def self.providers
           Gitlab::Auth::LDAP::Config.providers
         end
 
-        attr_accessor :ldap_user
-
         def login(login, password)
-          @ldap_user = adapter.bind_as(
+          result = adapter.bind_as(
             filter: user_filter(login),
             size: 1,
             password: password
           )
+          return unless result
+
+          @user = Gitlab::Auth::LDAP::User.find_by_uid_and_provider(result.dn, provider)
         end
 
         def adapter
@@ -56,12 +52,6 @@ module Gitlab
 
           filter
         end
-
-        def user
-          return unless ldap_user
-
-          Gitlab::Auth::LDAP::User.find_by_uid_and_provider(ldap_user.dn, provider)
-        end
       end
     end
   end

lib/gitlab/auth/o_auth/authentication.rb

@@ -12,6 +12,7 @@ module Gitlab
           @user = user
         end
 
+        # Implementation must return user object if login successful
         def login(login, password)
           raise NotImplementedError
         end

spec/lib/gitlab/auth_spec.rb

@@ -315,13 +315,19 @@ describe Gitlab::Auth do
       it "tries to autheticate with db before ldap" do
         expect(Gitlab::Auth::LDAP::Authentication).not_to receive(:login)
 
-        gl_auth.find_with_user_password(username, password)
+        expect(gl_auth.find_with_user_password(username, password)).to eq(user)
+      end
+
+      it "does not find user by using ldap as fallback to for authentication" do
+        expect(Gitlab::Auth::LDAP::Authentication).to receive(:login).and_return(nil)
+
+        expect(gl_auth.find_with_user_password('ldap_user', 'password')).to be_nil
       end
 
-      it "uses ldap as fallback to for authentication" do
-        expect(Gitlab::Auth::LDAP::Authentication).to receive(:login)
+      it "find new user by using ldap as fallback to for authentication" do
+        expect(Gitlab::Auth::LDAP::Authentication).to receive(:login).and_return(user)
 
-        gl_auth.find_with_user_password('ldap_user', 'password')
+        expect(gl_auth.find_with_user_password('ldap_user', 'password')).to eq(user)
       end
     end