Adapt tests to refactoring

- Use smarter instance methods
- Support multiple LDAP servers

lib/gitlab/ldap/access.rb

@@ -6,19 +6,19 @@
 module Gitlab
   module LDAP
     class Access
-      attr_reader :adapter, :provider
+      attr_reader :adapter, :provider, :user, :ldap_user
 
-      def self.open(provider, &block)
-        Gitlab::LDAP::Adapter.open(provider) do |adapter|
-          block.call(self.new(provider, adapter))
+      def self.open(user, &block)
+        Gitlab::LDAP::Adapter.open(user.provider) do |adapter|
+          block.call(self.new(user, adapter))
         end
       end
 
       def self.allowed?(user)
-        self.open(user.provider) do |access|
-          if access.allowed?(user)
-            access.update_permissions(user)
-            access.update_email(user)
+        self.open(user) do |access|
+          if access.allowed?
+            access.update_permissions
+            access.update_email
             user.last_credential_check_at = Time.now
             user.save
             true
@@ -28,12 +28,13 @@ module Gitlab
         end
       end
 
-      def initialize(provider, adapter=nil)
-        @provider = provider
+      def initialize(user, adapter=nil)
         @adapter = adapter
+        @user = user
+        @provider = user.provider
       end
 
-      def allowed?(user)
+      def allowed?
         if Gitlab::LDAP::Person.find_by_dn(user.extern_uid, adapter)
           !Gitlab::LDAP::Person.disabled_via_active_directory?(user.extern_uid, adapter)
         else
@@ -47,31 +48,28 @@ module Gitlab
         @adapter ||= Gitlab::LDAP::Adapter.new(provider)
       end
 
-      def get_ldap_user(user)
+      def ldap_user
         @ldap_user ||= Gitlab::LDAP::Person.find_by_dn(user.extern_uid, adapter)
       end
 
-      def update_permissions(user)
+      def update_permissions
         if sync_ssh_keys?
-          update_ssh_keys(user)
+          update_ssh_keys
         end
 
         # Skip updating group permissions
         # if instance does not use group_base setting
         return true unless group_base.present?
 
-        update_ldap_group_links(user)
+        update_ldap_group_links
 
         if admin_group.present?
-          update_admin_status(user)
+          update_admin_status
         end
       end
 
       # Update user ssh keys if they changed in LDAP
-      def update_ssh_keys(user)
-        # Get LDAP user entry
-        ldap_user = get_ldap_user(user)
-
+      def update_ssh_keys
         user.keys.ldap.where.not(key: ldap_user.ssh_keys).each do |deleted_key|
           Rails.logger.info "#{self.class.name}: removing LDAP SSH key #{deleted_key.key} from #{user.name} (#{user.id})"
           unless deleted_key.destroy
@@ -81,7 +79,7 @@ module Gitlab
 
         (ldap_user.ssh_keys - user.keys.ldap.pluck(:key)).each do |key|
           Rails.logger.info "#{self.class.name}: adding LDAP SSH key #{key.inspect} to #{user.name} (#{user.id})"
-          new_key = LDAPKey.new(title: "LDAP - #{ldap_config['sync_ssh_keys']}", key: key)
+          new_key = LDAPKey.new(title: "LDAP - #{ldap_config.ssh_sync_key}", key: key)
           new_key.user = user
           unless new_key.save
             Rails.logger.error "#{self.class.name}: failed to add LDAP SSH key #{key.inspect} to #{user.name} (#{user.id})\n"\
@@ -91,16 +89,12 @@ module Gitlab
       end
 
       # Update user email if it changed in LDAP
-      def update_email(user)
-        uid = user.extern_uid
-        ldap_user = get_ldap_user(user)
-        gitlab_user = ::User.where(provider: 'ldap', extern_uid: uid).last
-
-        if gitlab_user && ldap_user && ldap_user.email
+      def update_email
+        if ldap_user.try(:email)
           ldap_email = ldap_user.email.last.to_s.downcase
 
-          if (gitlab_user.email != ldap_email)
-            gitlab_user.update(email: ldap_email)
+          if (user.email != ldap_email)
+            user.update(email: ldap_email)
           else
             false
           end
@@ -109,8 +103,8 @@ module Gitlab
         end
       end
 
-      def update_admin_status(user)
-        admin_group = Gitlab::LDAP::Group.find_by_cn(ldap_config['admin_group'], adapter)
+      def update_admin_status
+        admin_group = Gitlab::LDAP::Group.find_by_cn(ldap_config.admin_group, adapter)
         if admin_group.has_member?(Gitlab::LDAP::Person.find_by_dn(user.extern_uid, adapter))
           unless user.admin?
             user.admin = true
@@ -125,9 +119,9 @@ module Gitlab
       end
 
       # Loop throug all ldap conneted groups, and update the users link with it
-      def update_ldap_group_links(user)
+      def update_ldap_group_links
         gitlab_groups_with_ldap_link.each do |group|
-          active_group_links = group.ldap_group_links.where(cn: cns_with_access(get_ldap_user(user)))
+          active_group_links = group.ldap_group_links.where(cn: cns_with_access)
 
           if active_group_links.any?
             group.add_users([user.id], fetch_group_access(group, user, active_group_links))
@@ -144,7 +138,7 @@ module Gitlab
       end
 
       # returns a collection of cn strings to which the user has access
-      def cns_with_access(ldap_user)
+      def cns_with_access
         @ldap_groups_with_access ||= ldap_groups.select do |ldap_group|
           ldap_group.has_member?(ldap_user)
         end.map(&:cn)

lib/gitlab/ldap/person.rb

@@ -6,24 +6,24 @@ module Gitlab
       # Source: http://ctogonewild.com/2009/09/03/bitmask-searches-in-ldap/
       AD_USER_DISABLED = Net::LDAP::Filter.ex("userAccountControl:1.2.840.113556.1.4.803", "2")
 
-      def self.find_by_uid(uid, adapter=nil)
-        adapter ||= Gitlab::LDAP::Adapter.new
+      attr_accessor :entry, :provider
+
+      def self.find_by_uid(uid, adapter)
         adapter.user(Gitlab.config.ldap.uid, uid)
       end
 
-      def self.find_by_dn(dn, adapter=nil)
-        adapter ||= Gitlab::LDAP::Adapter.new
+      def self.find_by_dn(dn, adapter)
         adapter.user('dn', dn)
       end
 
-      def self.disabled_via_active_directory?(dn, adapter=nil)
-        adapter ||= Gitlab::LDAP::Adapter.new
+      def self.disabled_via_active_directory?(dn, adapter)
         adapter.dn_matches_filter?(dn, AD_USER_DISABLED)
       end
 
-      def initialize(entry)
+      def initialize(entry, provider)
         Rails.logger.debug { "Instantiating #{self.class.name} with LDIF:\n#{entry.to_ldif}" }
         @entry = entry
+        @provider = provider
       end
 
       def name
@@ -47,9 +47,8 @@ module Gitlab
       end
 
       def ssh_keys
-        ssh_keys_attribute = Gitlab.config.ldap['sync_ssh_keys'].to_sym
-        if entry.respond_to?(ssh_keys_attribute)
-          entry[ssh_keys_attribute]
+        if config.sync_ssh_keys? && entry.respond_to?(config.ssh_sync_key)
+          entry[config.ssh_sync_key.to_sym]
         else
           []
         end
@@ -61,12 +60,12 @@ module Gitlab
         @entry
       end
 
-      def adapter
-        @adapter ||= Gitlab::LDAP::Adapter.new
-      end
+      # def adapter
+      #   @adapter ||= Gitlab::LDAP::Adapter.new
+      # end
 
       def config
-        @config ||= Gitlab.config.ldap
+        @config ||= Gitlab::LDAP::Config.new(provider)
       end
     end
   end

lib/gitlab/ldap/user.rb

@@ -14,8 +14,9 @@ module Gitlab
         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?
+          return nil unless ldap_conf.enabled? && login.present? && password.present?
 
+          binding.pry
           ldap_user = adapter.bind_as(
             filter: user_filter(login),
             size: 1,

spec/factories.rb

@@ -24,6 +24,11 @@ FactoryGirl.define do
       admin true
     end
 
+    trait :ldap do
+      provider 'ldapmain'
+      extern_uid 'my-ldap-id'
+    end
+
     factory :admin, traits: [:admin]
   end