From 886ba87d309a2381fe409a6eb9adcbb0bb19b6f0 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 27 Mar 2018 07:59:35 +0000 Subject: Merge branch 'fix/ldap_wihtout_user' into 'master' Fix LDAP login without user in DB See merge request gitlab-org/gitlab-ce!17988 (cherry picked from commit ab8f13c3ef6e07eb8d44805dc9eef4b008e1bbe9) 7d017926 Fix LDAP login without user in DB --- ...credentials-causes-a-HTTP-401-Access-denied.yml | 5 +++++ lib/gitlab/auth.rb | 6 +++++- lib/gitlab/auth/database/authentication.rb | 2 +- lib/gitlab/auth/ldap/authentication.rb | 22 ++++++---------------- lib/gitlab/auth/o_auth/authentication.rb | 1 + spec/lib/gitlab/auth_spec.rb | 14 ++++++++++---- 6 files changed, 28 insertions(+), 22 deletions(-) create mode 100644 changelogs/unreleased/44608-Cloning-a-repository-over-HTTPS-with-LDAP-credentials-causes-a-HTTP-401-Access-denied.yml diff --git a/changelogs/unreleased/44608-Cloning-a-repository-over-HTTPS-with-LDAP-credentials-causes-a-HTTP-401-Access-denied.yml b/changelogs/unreleased/44608-Cloning-a-repository-over-HTTPS-with-LDAP-credentials-causes-a-HTTP-401-Access-denied.yml new file mode 100644 index 00000000000..5afb1e3d908 --- /dev/null +++ b/changelogs/unreleased/44608-Cloning-a-repository-over-HTTPS-with-LDAP-credentials-causes-a-HTTP-401-Access-denied.yml @@ -0,0 +1,5 @@ +--- +title: 'Cloning a repository over HTTPS with LDAP credentials causes a HTTP 401 Access denied' +merge_request: !17988 +author: Horatiu Eugen Vlad +type: fixed diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index f5ccf952cf9..6af763faf10 100644 --- a/lib/gitlab/auth.rb +++ b/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 diff --git a/lib/gitlab/auth/database/authentication.rb b/lib/gitlab/auth/database/authentication.rb index 260a77058a4..1234ace0334 100644 --- a/lib/gitlab/auth/database/authentication.rb +++ b/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 diff --git a/lib/gitlab/auth/ldap/authentication.rb b/lib/gitlab/auth/ldap/authentication.rb index e70c3ab6b46..7c134fb6438 100644 --- a/lib/gitlab/auth/ldap/authentication.rb +++ b/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 diff --git a/lib/gitlab/auth/o_auth/authentication.rb b/lib/gitlab/auth/o_auth/authentication.rb index ed03b9f8b40..d4e7f35c857 100644 --- a/lib/gitlab/auth/o_auth/authentication.rb +++ b/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 diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index f969f9e8e38..18cef8ec996 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/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 -- cgit v1.2.1