Merge branch 'feature-multiple-ldap-push-authentication' into 'feature-multi-ldap-servers'

Feature multiple ldap push authentication

Add proper multi server authentication

In addition, to reduce complexity, I've moved the authentication to it's own class.

See merge request !194

lib/gitlab/auth.rb

@@ -9,7 +9,7 @@ module Gitlab
         # Second chance - try LDAP authentication
         return nil unless Gitlab::LDAP::Config.enabled?
 
-        Gitlab::LDAP::User.authenticate(login, password)
+        Gitlab::LDAP::Authentication.login(login, password)
       else
         user if user.valid_password?(password)
       end

lib/gitlab/ldap/authentication.rb

+# This calls helps to authenticate to LDAP by providing username and password
+#
+# Since multiple LDAP servers are supported, it will loop through all of them
+# until a valid bind is found
+#
+
+module Gitlab
+  module LDAP
+    class Authentication
+      def self.login(login, password)
+        return unless Gitlab::LDAP::Config.enabled?
+        return unless login.present? && password.present?
+
+        auth = nil
+        # loop through providers until valid bind
+        providers.find do |provider|
+          auth = new(provider)
+          auth.login(login, password) # true will exit the loop
+        end
+
+        auth.user
+      end
+
+      def self.providers
+        Gitlab::LDAP::Config.providers
+      end
+
+      attr_accessor :provider, :ldap_user
+
+      def initialize(provider)
+        @provider = provider
+      end
+
+      def login(login, password)
+        @ldap_user = adapter.bind_as(
+          filter: user_filter(login),
+          size: 1,
+          password: password
+        )
+      end
+
+      def adapter
+        OmniAuth::LDAP::Adaptor.new(config.options)
+      end
+
+      def config
+        Gitlab::LDAP::Config.new(provider)
+      end
+
+      def user_filter(login)
+        Net::LDAP::Filter.eq(config.uid, login).tap do |filter|
+          # Apply LDAP user filter if present
+          if config.user_filter.present?
+            Net::LDAP::Filter.join(
+              filter,
+              Net::LDAP::Filter.construct(config.user_filter)
+            )
+          end
+        end
+      end
+
+      def user
+        return nil unless ldap_user
+        Gitlab::LDAP::User.find_by_uid_and_provider(ldap_user.dn, provider)
+      end
+    end
+  end
+end
\ No newline at end of file

lib/gitlab/ldap/user.rb

@@ -10,52 +10,12 @@ module Gitlab
   module LDAP
     class User < Gitlab::OAuth::User
       class << self
-        # TODO: Look through LDAP servers until valid credentials are found?
-        def authenticate(login, password)
-          # Check user against LDAP backend if user is not authenticated
-          # Only check with valid login and password to prevent anonymous bind results
-          return nil unless ldap_conf.enabled? && login.present? && password.present?
-
-          ldap_user = adapter.bind_as(
-            filter: user_filter(login),
-            size: 1,
-            password: password
-          )
-
-          find_by_uid(ldap_user.dn) if ldap_user
-        end
-
-        def adapter
-          @adapter ||= OmniAuth::LDAP::Adaptor.new(ldap_conf.options)
-        end
-
-        def user_filter(login)
-          filter = Net::LDAP::Filter.eq(adapter.uid, login)
-          # Apply LDAP user filter if present
-          if ldap_conf.user_filter.present?
-            user_filter = Net::LDAP::Filter.construct(ldap_conf.user_filter)
-            filter = Net::LDAP::Filter.join(filter, user_filter)
-          end
-          filter
-        end
-
-        def ldap_conf
-          Gitlab::LDAP::Config.new(provider)
-        end
-
-        def find_by_uid(uid)
+        def find_by_uid_and_provider(uid, provider)
           # LDAP distinguished name is case-insensitive
-          model.
+          ::User.
             where(provider: [provider, :ldap]).
             where('lower(extern_uid) = ?', uid.downcase).last
         end
-
-        def provider
-          # Note: for backwards compatibility we just get the first provider
-          # Later on, we should loop through all servers until a successful
-          # authentication
-          Gitlab::LDAP::Config.servers.first.provider_name
-        end
       end
 
       def initialize(auth_hash)

spec/lib/gitlab/auth_spec.rb

@@ -31,13 +31,13 @@ describe Gitlab::Auth do
       before { Gitlab::LDAP::Config.stub(enabled?: true) }
 
       it "tries to autheticate with db before ldap" do
-        expect(Gitlab::LDAP::User).not_to receive(:authenticate)
+        expect(Gitlab::LDAP::Authentication).not_to receive(:login)
 
         gl_auth.find(username, password)
       end
 
       it "uses ldap as fallback to for authentication" do
-        expect(Gitlab::LDAP::User).to receive(:authenticate)
+        expect(Gitlab::LDAP::Authentication).to receive(:login)
 
         gl_auth.find('ldap_user', 'password')
       end

spec/lib/gitlab/ldap/authentication_spec.rb

+require 'spec_helper'
+
+describe Gitlab::LDAP::Authentication do
+  let(:klass) { Gitlab::LDAP::Authentication }
+  let(:user) { create(:user, :ldap, extern_uid: dn) }
+  let(:dn) { 'uid=john,ou=people,dc=example,dc=com' }
+  let(:login) { 'john' }
+  let(:password) { 'password' }
+
+  describe :login do
+    let(:adapter) { double :adapter }
+    before do
+      Gitlab::LDAP::Config.stub(enabled?: true)
+    end
+
+    it "finds the user if authentication is successful" do
+      user
+      # try only to fake the LDAP call
+      klass.any_instance.stub(adapter: double(:adapter,
+        bind_as: double(:ldap_user, dn: dn)
+      ))
+      expect(klass.login(login, password)).to be_true
+    end
+
+    it "is false if the user does not exist" do
+      # try only to fake the LDAP call
+      klass.any_instance.stub(adapter: double(:adapter,
+        bind_as: double(:ldap_user, dn: dn)
+      ))
+      expect(klass.login(login, password)).to be_false
+    end
+
+    it "is false if authentication fails" do
+      user
+      # try only to fake the LDAP call
+      klass.any_instance.stub(adapter: double(:adapter, bind_as: nil))
+      expect(klass.login(login, password)).to be_false
+    end
+
+    it "fails if ldap is disabled" do
+      Gitlab::LDAP::Config.stub(enabled?: false)
+      expect(klass.login(login, password)).to be_false
+    end
+
+    it "fails if no login is supplied" do
+      expect(klass.login('', password)).to be_false
+    end
+
+    it "fails if no password is supplied" do
+      expect(klass.login(login, '')).to be_false
+    end
+  end
+end
\ No newline at end of file

spec/lib/gitlab/ldap/user_spec.rb

@@ -33,21 +33,4 @@ describe Gitlab::LDAP::User do
       expect{ gl_user.save }.to change{ User.count }.by(1)
     end
   end
-
-  describe "authenticate" do
-    let(:login) { 'john' }
-    let(:password) { 'my-secret' }
-
-    # before {
-    #   Gitlab.config.ldap['enabled'] = true
-    #   Gitlab.config.ldap['user_filter'] = 'employeeType=developer'
-    # }
-    # after  { Gitlab.config.ldap['enabled'] = false }
-
-    it "send an authentication request to ldap" do
-      pending('needs refactoring')
-      expect( Gitlab::LDAP::User.adapter ).to receive(:bind_as)
-      Gitlab::LDAP::User.authenticate(login, password)
-    end
-  end
 end