Refactor group sync in to a more modular layout

lib/ee/gitlab/ldap/sync/admin_users.rb

+module EE
+  module Gitlab
+    module LDAP
+      module Sync
+        class AdminUsers
+          attr_reader :provider, :proxy
+
+          def self.execute(proxy)
+            self.new(proxy).update_permissions
+          end
+
+          def initialize(proxy)
+            @provider = proxy.provider
+            @proxy = proxy
+          end
+
+          def update_permissions
+            return if admin_group.empty?
+
+            admin_group_member_dns = proxy.dns_for_group_cn(admin_group)
+            current_admin_users = ::User.admins.with_provider(provider)
+            verified_admin_users = []
+
+            # Verify existing admin users and add new ones.
+            admin_group_member_dns.each do |member_dn|
+              user = ::Gitlab::LDAP::User.find_by_uid_and_provider(member_dn, provider)
+
+              if user.present?
+                user.admin = true
+                user.save
+                verified_admin_users << user
+              else
+                Rails.logger.debug do
+                  <<-MSG.strip_heredoc.tr("\n", ' ')
+                    #{self.class.name}: User with DN `#{member_dn}` should have admin
+                    access but there is no user in GitLab with that identity.
+                    Membership will be updated once the user signs in for the first time.
+                  MSG
+                end
+              end
+            end
+
+            # Revoke the unverified admins.
+            current_admin_users.each do |user|
+              unless verified_admin_users.include?(user)
+                user.admin = false
+                user.save
+              end
+            end
+          end
+
+          private
+
+          def admin_group
+            proxy.adapter.config.admin_group
+          end
+        end
+      end
+    end
+  end
+end

lib/ee/gitlab/ldap/sync/external_users.rb

+module EE
+  module Gitlab
+    module LDAP
+      module Sync
+        class ExternalUsers
+          attr_reader :provider, :proxy
+
+          def self.execute(proxy)
+            self.new(proxy).update_permissions
+          end
+
+          def initialize(proxy)
+            @provider = proxy.provider
+            @proxy = proxy
+          end
+
+          def update_permissions
+            return unless external_groups.any?
+
+            current_external_users = ::User.external.with_provider(provider)
+            verified_external_users = []
+
+            external_groups.each do |group|
+              group_dns = proxy.dns_for_group_cn(group)
+
+              group_dns.each do |member_dn|
+                user = ::Gitlab::LDAP::User.find_by_uid_and_provider(member_dn, provider)
+
+                if user.present?
+                  user.external = true
+                  user.save
+                  verified_external_users << user
+                else
+                  Rails.logger.debug do
+                    <<-MSG.strip_heredoc.tr("\n", ' ')
+                      #{self.class.name}: User with DN `#{member_dn}` should be marked as
+                      external but there is no user in GitLab with that identity.
+                      Membership will be updated once the user signs in for the first time.
+                    MSG
+                  end
+                end
+              end
+            end
+
+            # Restore normal access to users no longer found in the external groups
+            current_external_users.each do |user|
+              unless verified_external_users.include?(user)
+                user.external = false
+                user.save
+              end
+            end
+          end
+
+          private
+
+          def external_groups
+            proxy.adapter.config.external_groups
+          end
+        end
+      end
+    end
+  end
+end

lib/ee/gitlab/ldap/sync/group.rb

+module EE
+  module Gitlab
+    module LDAP
+      module Sync
+        class Group
+          attr_reader :provider, :group, :proxy
+
+          def self.execute(group, proxy)
+            self.new(group, proxy).update_permissions
+          end
+
+          def initialize(group, proxy)
+            @provider = proxy.provider
+            @group = group
+            @proxy = proxy
+          end
+
+          def update_permissions
+            lease = ::Gitlab::ExclusiveLease.new(
+              "ldap_group_sync:#{provider}:#{group.id}",
+              timeout: 3600
+            )
+            return unless lease.try_obtain
+
+            logger.debug { "Syncing '#{group.name}' group" }
+
+            access_levels = AccessLevels.new
+            # Only iterate over group links for the current provider
+            group.ldap_group_links.with_provider(provider).each do |group_link|
+              if member_dns = dns_for_group_cn(group_link.cn)
+                access_levels.set(member_dns, to: group_link.group_access)
+                logger.debug do
+                  "Resolved '#{group.name}' group member access: #{access_levels.to_hash}"
+                end
+              end
+            end
+
+            update_existing_group_membership(group, access_levels)
+            add_new_members(group, access_levels)
+
+            group.update(last_ldap_sync_at: Time.now)
+
+            logger.debug { "Finished syncing '#{group.name}' group" }
+          end
+
+          private
+
+          def dns_for_group_cn(group_cn)
+            proxy.dns_for_group_cn(group_cn)
+          end
+
+          def dn_for_uid(uid)
+            proxy.dn_for_uid(uid)
+          end
+
+          def update_existing_group_membership(group, access_levels)
+            logger.debug { "Updating existing membership for '#{group.name}' group" }
+
+            select_and_preload_group_members(group).each do |member|
+              user = member.user
+              identity = user.identities.select(:id, :extern_uid)
+                           .with_provider(provider).first
+              member_dn = identity.extern_uid
+
+              # Skip if this is not an LDAP user with a valid `extern_uid`.
+              next unless member_dn.present?
+
+              # Prevent shifting group membership, in case where user is a member
+              # of two LDAP groups from different providers linked to the same
+              # GitLab group. This is not ideal, but preserves existing behavior.
+              if user.ldap_identity.id != identity.id
+                access_levels.delete(member_dn)
+                next
+              end
+
+              desired_access = access_levels[member_dn]
+
+              # Don't do anything if the user already has the desired access level
+              if member.access_level == desired_access
+                access_levels.delete(member_dn)
+                next
+              end
+
+              # Check and update the access level. If `desired_access` is `nil`
+              # we need to delete the user from the group.
+              if desired_access.present?
+                add_or_update_user_membership(user, group, desired_access)
+
+                # Delete this entry from the hash now that we've acted on it
+                access_levels.delete(member_dn)
+              elsif group.last_owner?(user)
+                warn_cannot_remove_last_owner(user, group)
+              else
+                group.users.delete(user)
+              end
+            end
+          end
+
+          def add_new_members(group, access_levels)
+            logger.debug { "Adding new members to '#{group.name}' group" }
+
+            access_levels.each do |member_dn, access_level|
+              user = ::Gitlab::LDAP::User.find_by_uid_and_provider(member_dn, provider)
+
+              if user.present?
+                add_or_update_user_membership(user, group, access_level)
+              else
+                logger.debug do
+                  <<-MSG.strip_heredoc.tr("\n", ' ')
+                    #{self.class.name}: User with DN `#{member_dn}` should have access
+                    to '#{group.name}' group but there is no user in GitLab with that
+                    identity. Membership will be updated once the user signs in for
+                    the first time.
+                  MSG
+                end
+              end
+            end
+          end
+
+          def add_or_update_user_membership(user, group, access)
+            # Prevent the last owner of a group from being demoted
+            if access < ::Gitlab::Access::OWNER && group.last_owner?(user)
+              warn_cannot_remove_last_owner(user, group)
+            else
+              # If you pass the user object, instead of just user ID,
+              # it saves an extra user database query.
+              group.add_users([user], access, skip_notification: true)
+            end
+          end
+
+          def warn_cannot_remove_last_owner(user, group)
+            logger.warn do
+              <<-MSG.strip_heredoc.tr("\n", ' ')
+                #{self.class.name}: LDAP group sync cannot remove #{user.name}
+                (#{user.id}) from group #{group.name} (#{group.id}) as this is
+                the group's last owner
+              MSG
+            end
+          end
+
+          def select_and_preload_group_members(group)
+            group.members.select_access_level_and_user
+              .with_identity_provider(provider).preload(:user)
+          end
+
+          def logger
+            Rails.logger
+          end
+        end
+      end
+    end
+  end
+end

lib/ee/gitlab/ldap/sync/groups.rb

+module EE
+  module Gitlab
+    module LDAP
+      module Sync
+        class Groups
+          attr_reader :provider, :proxy
+
+          def self.execute
+            # Shuffle providers to prevent a scenario where sync fails after a time
+            # and only the first provider or two get synced. This shuffles the order
+            # so subsequent syncs should eventually get to all providers. Obviously
+            # we should avoid failure, but this is an additional safeguard.
+            ::Gitlab::LDAP::Config.providers.shuffle.each do |provider|
+              Sync::Proxy.open(provider) do |proxy|
+                group_sync = self.new(proxy)
+                group_sync.update_permissions
+              end
+            end
+
+            true
+          end
+
+          def initialize(proxy)
+            @provider = proxy.provider
+            @proxy = proxy
+          end
+
+          def update_permissions
+            unless config.group_base.present?
+              logger.debug { "No `group_base` configured for '#{provider}' provider. Skipping" }
+              return nil
+            end
+
+            logger.debug { "Performing LDAP group sync for '#{provider}' provider" }
+            sync_groups
+            logger.debug { "Finished LDAP group sync for '#{provider}' provider" }
+
+            if config.admin_group.present?
+              logger.debug { "Syncing admin users for '#{provider}' provider" }
+              sync_admin_users
+              logger.debug { "Finished syncing admin users for '#{provider}' provider" }
+            else
+              logger.debug { "No `admin_group` configured for '#{provider}' provider. Skipping" }
+            end
+
+            if config.external_groups.empty?
+              logger.debug { "No `external_groups` configured for '#{provider}' provider. Skipping" }
+            else
+              logger.debug { "Syncing external users for '#{provider}' provider" }
+              sync_external_users
+              logger.debug { "Finished syncing external users for '#{provider}' provider" }
+            end
+
+            nil
+          end
+
+          private
+
+          def sync_groups
+            groups_where_group_links_with_provider_ordered.each do |group|
+              Sync::Group.execute(group, proxy)
+            end
+          end
+
+          def sync_admin_users
+            Sync::AdminUsers.execute(proxy)
+          end
+
+          def sync_external_users
+            Sync::ExternalUsers.execute(proxy)
+          end
+
+          def groups_where_group_links_with_provider_ordered
+            ::Group.where_group_links_with_provider(provider)
+              .preload(:ldap_group_links)
+              .reorder('last_ldap_sync_at ASC, namespaces.id ASC')
+              .distinct
+          end
+
+          def config
+            proxy.adapter.config
+          end
+
+          def logger
+            Rails.logger
+          end
+        end
+      end
+    end
+  end
+end

lib/ee/gitlab/ldap/sync/proxy.rb

+require 'net/ldap/dn'
+
+module EE
+  module Gitlab
+    module LDAP
+      module Sync
+        class Proxy
+          attr_reader :provider, :adapter
+
+          # Open a connection and run all queries through it.
+          # It's more efficient than the default of opening/closing per LDAP query.
+          def self.open(provider, &block)
+            ::Gitlab::LDAP::Adapter.open(provider) do |adapter|
+              block.call(self.new(provider, adapter))
+            end
+          end
+
+          def initialize(provider, adapter)
+            @adapter = adapter
+            @provider = provider
+          end
+
+          # Cache LDAP group member DNs so we don't query LDAP groups more than once.
+          def dns_for_group_cn(group_cn)
+            @dns_for_group_cn ||= Hash.new { |h, k| h[k] = ldap_group_member_dns(k) }
+            @dns_for_group_cn[group_cn]
+          end
+
+          # Cache user DN so we don't generate excess queries to map UID to DN
+          def dn_for_uid(uid)
+            @dn_for_uid ||= Hash.new { |h, k| h[k] = member_uid_to_dn(k) }
+            @dn_for_uid[uid]
+          end
+
+          private
+
+          def ldap_group_member_dns(ldap_group_cn)
+            ldap_group = LDAP::Group.find_by_cn(ldap_group_cn, adapter)
+            unless ldap_group.present?
+              logger.warn { "Cannot find LDAP group with CN '#{ldap_group_cn}'. Skipping" }
+              return []
+            end
+
+            member_dns = ldap_group.member_dns
+            if member_dns.empty?
+              # Group must be empty
+              return [] unless ldap_group.memberuid?
+
+              members = ldap_group.member_uids
+              member_dns = members.map { |uid| dn_for_uid(uid) }
+            end
+
+            # Various lookups in this method could return `nil` values.
+            # Compact the array to remove those entries
+            member_dns.compact!
+
+            ensure_full_dns!(member_dns)
+
+            logger.debug { "Members in '#{ldap_group.name}' LDAP group: #{member_dns}" }
+
+            # Various lookups in this method could return `nil` values.
+            # Compact the array to remove those entries
+            member_dns
+          end
+
+          # At least one customer reported that their LDAP `member` values contain
+          # only `uid=username` and not the full DN. This method allows us to
+          # account for that. See gitlab-ee#442
+          def ensure_full_dns!(dns)
+            dns.map! do |dn|
+              begin
+                parsed_dn = Net::LDAP::DN.new(dn).to_a
+              rescue RuntimeError => e
+                # Net::LDAP raises a generic RuntimeError. Bad library! Bad!
+                logger.error { "Found malformed DN: '#{dn}'. Skipping. #{e.message}" }
+                next
+              end
+
+              # If there is more than one key/value set we must have a full DN,
+              # or at least the probability is higher.
+              if parsed_dn.count > 2
+                dn
+              elsif parsed_dn[0] == 'uid'
+                dn_for_uid(parsed_dn[1])
+              else
+                logger.warn { "Found potentially malformed/incomplete DN: '#{dn}'" }
+                dn
+              end
+            end
+
+            # Remove `nil` values generated by the rescue above.
+            dns.compact!
+          end
+
+          def member_uid_to_dn(uid)
+            identity = Identity.find_by(provider: provider, secondary_extern_uid: uid)
+
+            if identity.present?
+              # Use the DN on record in GitLab when it's available
+              identity.extern_uid
+            else
+              ldap_user = ::Gitlab::LDAP::Person.find_by_uid(uid, adapter)
+
+              # Can't find a matching user
+              return nil unless ldap_user.present?
+
+              # Update user identity so we don't have to go through this again
+              update_identity(ldap_user.dn, uid)
+
+              ldap_user.dn
+            end
+          end
+
+          def update_identity(dn, uid)
+            identity =
+              Identity.find_by(provider: provider, extern_uid: dn)
+
+            # User may not exist in GitLab yet. Skip.
+            return unless identity.present?
+
+            identity.secondary_extern_uid = uid
+            identity.save
+          end
+
+          def logger
+            Rails.logger
+          end
+        end
+      end
+    end
+  end
+end

spec/factories/groups.rb

@@ -15,5 +15,22 @@ FactoryGirl.define do
     trait :private do
       visibility_level Gitlab::VisibilityLevel::PRIVATE
     end
+
+    factory :group_with_ldap_group_link do
+      transient do
+        cn 'group1'
+        group_access Gitlab::Access::GUEST
+        provider 'ldapmain'
+      end
+
+      after(:create) do |group, evaluator|
+        group.ldap_group_links << create(
+          :ldap_group_link,
+          cn: evaluator.cn,
+          group_access: evaluator.group_access,
+          provider: evaluator.provider
+        )
+      end
+    end
   end
 end

spec/factories/users.rb

@@ -14,6 +14,10 @@ FactoryGirl.define do
       admin true
     end
 
+    trait :external do
+      external true
+    end
+
     trait :two_factor do
       two_factor_via_otp
     end
@@ -51,5 +55,6 @@ FactoryGirl.define do
     end
 
     factory :admin, traits: [:admin]
+    factory :external_user, traits: [:external]
   end
 end

spec/lib/ee/gitlab/ldap/sync/admin_users_spec.rb

+require 'spec_helper'
+
+describe EE::Gitlab::LDAP::Sync::AdminUsers, lib: true do
+  include LdapHelpers
+
+  describe '#update_permissions' do
+    let(:sync_admin) { described_class.new(proxy(adapter)) }
+
+    let(:user) { create(:user) }
+
+    let(:admin_group) do
+      ldap_group_entry(user_dn(user.username), cn: 'admin_group')
+    end
+
+    before do
+      stub_ldap_config(admin_group: 'admin_group', active_directory: false)
+      stub_ldap_group_find_by_cn('admin_group', admin_group, adapter)
+    end
+
+    it 'adds user as admin' do
+      create(:identity, user: user, extern_uid: user_dn(user.username))
+
+      expect { sync_admin.update_permissions }
+        .to change { user.reload.admin? }.from(false).to(true)
+    end
+
+    it 'removes users that are not in the LDAP group' do
+      admin = create(:admin)
+      create(:identity, user: admin, extern_uid: user_dn(admin.username))
+
+      expect { sync_admin.update_permissions }
+        .to change { admin.reload.admin? }.from(true).to(false)
+    end
+
+    it 'leaves admin users that do not have the LDAP provider' do
+      admin = create(:admin)
+
+      expect { sync_admin.update_permissions }
+        .not_to change { admin.reload.admin? }
+    end
+
+    it 'leaves admin users that have a different provider identity' do
+      admin = create(:admin)
+      create(:identity,
+             user: admin,
+             provider: 'ldapsecondary',
+             extern_uid: user_dn(admin.username))
+
+      expect { sync_admin.update_permissions }
+        .not_to change { admin.reload.admin? }
+    end
+  end
+end

spec/lib/ee/gitlab/ldap/sync/external_users_spec.rb

+require 'spec_helper'
+
+describe EE::Gitlab::LDAP::Sync::ExternalUsers, lib: true do
+  include LdapHelpers
+
+  describe '#update_permissions' do
+    let(:sync_external) { described_class.new(proxy(adapter)) }
+    let(:user1) { create(:user) }
+    let(:user2) { create(:user) }
+    let(:external_group1) { ldap_group_entry(user_dn(user1.username), cn: 'external_group1') }
+    let(:external_group2) { ldap_group_entry(user_dn(user2.username), cn: 'external_group2') }
+
+    before do
+      stub_ldap_config(
+        external_groups: %w(external_group1 external_group2),
+        active_directory: false
+      )
+      stub_ldap_group_find_by_cn('external_group1', external_group1, adapter)
+      stub_ldap_group_find_by_cn('external_group2', external_group2, adapter)
+    end
+
+    it 'adds users from both external LDAP groups as external users' do
+      create(:identity, user: user1, extern_uid: user_dn(user1.username))
+      create(:identity, user: user2, extern_uid: user_dn(user2.username))
+
+      sync_external.update_permissions
+
+      expect(user1.reload.external?).to be true
+      expect(user2.reload.external?).to be true
+    end
+
+    it 'removes users that are not in the LDAP group' do
+      user = create(:external_user)
+      create(:identity, user: user, extern_uid: user_dn(user.username))
+
+      expect { sync_external.update_permissions }
+        .to change { user.reload.external? }.from(true).to(false)
+    end
+
+    it 'leaves external users that do not have the LDAP provider' do
+      user = create(:external_user)
+
+      expect { sync_external.update_permissions }
+        .not_to change { user.reload.external? }
+    end
+
+    it 'leaves external users that have a different provider identity' do
+      user = create(:external_user)
+      create(:identity, user: user, provider: 'ldapsecondary', extern_uid: user_dn(user.username))
+
+      expect { sync_external.update_permissions }
+        .not_to change { user.reload.external? }
+    end
+  end
+end

spec/lib/ee/gitlab/ldap/sync/group_spec.rb

+require 'spec_helper'
+
+describe EE::Gitlab::LDAP::Sync::Group, lib: true do
+  include LdapHelpers
+
+  let(:sync_group) { described_class.new(group, proxy(adapter)) }
+  let(:user) { create(:user) }
+
+  before do
+    allow_any_instance_of(::Gitlab::ExclusiveLease)
+      .to receive(:try_obtain).and_return(true)
+
+    create(:identity, user: user, extern_uid: user_dn(user.username))
+
+    stub_ldap_config(active_directory: false)
+    stub_ldap_group_find_by_cn('ldap_group1', ldap_group1, adapter)
+  end
+
+  describe '#update_permissions' do
+    let(:group) do
+      create(:group_with_ldap_group_link,
+             cn: 'ldap_group1',
+             group_access: ::Gitlab::Access::DEVELOPER)
+    end
+
+    context 'with all functionality against one LDAP group type' do
+      context 'with basic add/update actions' do
+        let(:ldap_group1) { ldap_group_entry(user_dn(user.username)) }
+
+        it 'adds new members' do
+          sync_group.update_permissions
+
+          expect(group.members.pluck(:user_id)).to include(user.id)
+        end
+
+        it 'downgrades existing member access' do
+          # Create user with higher access
+          group.add_users([user],
+                          ::Gitlab::Access::MASTER, skip_notification: true)
+
+          sync_group.update_permissions
+
+          expect(group.members.find_by(user_id: user.id).access_level)
+            .to eq(::Gitlab::Access::DEVELOPER)
+        end
+
+        it 'upgrades existing member access' do
+          # Create user with lower access
+          group.add_users([user],
+                          ::Gitlab::Access::GUEST, skip_notification: true)
+
+          sync_group.update_permissions
+
+          expect(group.members.find_by(user_id: user.id).access_level)
+            .to eq(::Gitlab::Access::DEVELOPER)
+        end
+      end
+
+      context 'when existing user is no longer in LDAP group' do
+        let(:ldap_group1) do
+          ldap_group_entry(user_dn('some_user'))
+        end
+
+        it 'removes the user from the group' do
+          group.add_users([user],
+                          Gitlab::Access::MASTER, skip_notification: true)
+
+          sync_group.update_permissions
+
+          expect(group.members.find_by(user_id: user.id)).to be_nil
+        end
+
+        it 'refuses to delete the last owner' do
+          group.add_users([user],
+                          Gitlab::Access::OWNER, skip_notification: true)
+
+          sync_group.update_permissions
+
+          expect(group.members.find_by(user_id: user.id).access_level)
+            .to eq(::Gitlab::Access::OWNER)
+        end
+      end
+
+      context 'when the user is the last owner' do
+        let(:user1) { create(:user) }
+        let(:user2) { create(:user) }
+        let(:ldap_group1) do
+          ldap_group_entry(%W(#{user_dn(user1.username)} #{user_dn(user2.username)}))
+        end
+
+        it 'downgrades one user but not the other' do
+          create(:identity, user: user1, extern_uid: user_dn(user1.username))
+          create(:identity, user: user2, extern_uid: user_dn(user2.username))
+          group.add_users([user1, user2],
+                          Gitlab::Access::OWNER, skip_notification: true)
+
+          sync_group.update_permissions
+
+          expect(group.members.pluck(:access_level).sort)
+            .to eq([::Gitlab::Access::DEVELOPER, ::Gitlab::Access::OWNER])
+        end
+      end
+    end
+
+    # Test that membership can be resolved for all different type of LDAP groups
+    context 'with different LDAP group types' do
+      # GroupOfNames - OpenLDAP
+      context 'with groupOfNames style LDAP group' do
+        let(:ldap_group1) do
+          ldap_group_entry(
+            user_dn(user.username),
+            objectclass: 'groupOfNames',
+            member_attr: 'uniqueMember'
+          )
+        end
+
+        it 'adds the user to the group' do
+          sync_group.update_permissions
+
+          expect(group.members.pluck(:user_id)).to include(user.id)
+        end
+      end
+
+      # posixGroup - Apple Open Directory
+      context 'with posixGroup style LDAP group' do
+        let(:ldap_group1) do
+          ldap_group_entry(
+            user.username,
+            objectclass: 'posixGroup',
+            member_attr: 'memberUid'
+          )
+        end
+        let(:ldap_user) do
+          ldap_user_entry(user.username)
+        end
+
+        it 'adds the user to the group' do
+          stub_ldap_person_find_by_uid(user.username, ldap_user)
+
+          sync_group.update_permissions
+
+          expect(group.members.pluck(:user_id)).to include(user.id)
+        end
+      end
+
+      context 'with groupOfUniqueNames style LDAP group' do
+        let(:ldap_group1) do
+          ldap_group_entry(
+            user_dn(user.username),
+            objectclass: 'groupOfUniqueNames',
+            member_attr: 'uniqueMember'
+          )
+        end
+
+        it 'adds the user to the group' do
+          sync_group.update_permissions
+
+          expect(group.members.pluck(:user_id)).to include(user.id)
+        end
+      end
+
+      context 'with an empty LDAP group' do
+        let(:ldap_group1) do
+          ldap_group_entry(nil)
+        end
+
+        it 'does nothing, without failure' do
+          expect { sync_group.update_permissions }
+            .not_to change { group.members.count }
+        end
+      end
+    end
+  end
+end

spec/lib/ee/gitlab/ldap/sync/groups_spec.rb

+require 'spec_helper'
+
+describe EE::Gitlab::LDAP::Sync::Groups, lib: true do
+  include LdapHelpers
+
+  let(:group_sync) { described_class.new(proxy(adapter)) }
+
+  describe '#update_permissions' do
+    before do
+      allow(EE::Gitlab::LDAP::Sync::Group).to receive(:execute)
+      allow(EE::Gitlab::LDAP::Sync::AdminUsers).to receive(:execute)
+      allow(EE::Gitlab::LDAP::Sync::ExternalUsers).to receive(:execute)
+
+      2.times { create(:group_with_ldap_group_link) }
+    end
+
+    after { group_sync.update_permissions }
+
+    context 'when group_base is not present' do
+      before { stub_ldap_config(group_base: nil) }
+
+      it 'does not call EE::Gitlab::LDAP::Sync::Group#execute' do
+        expect(EE::Gitlab::LDAP::Sync::Group).not_to receive(:execute)
+      end
+
+      it 'does not call EE::Gitlab::LDAP::Sync::AdminUsers#execute' do
+        expect(EE::Gitlab::LDAP::Sync::AdminUsers).not_to receive(:execute)
+      end
+
+      it 'does not call EE::Gitlab::LDAP::Sync::ExternalUsers#execute' do
+        expect(EE::Gitlab::LDAP::Sync::ExternalUsers).not_to receive(:execute)
+      end
+    end
+
+    context 'when group_base is present' do
+
+      context 'and admin_group and external_groups are not present' do
+        before { stub_ldap_config(group_base: 'dc=example,dc=com') }
+
+        it 'should call EE::Gitlab::LDAP::Sync::Group#execute' do
+          expect(EE::Gitlab::LDAP::Sync::Group).to receive(:execute).twice
+        end
+
+        it 'does not call EE::Gitlab::LDAP::Sync::AdminUsers#execute' do
+          expect(EE::Gitlab::LDAP::Sync::AdminUsers).not_to receive(:execute)
+        end
+
+        it 'does not call EE::Gitlab::LDAP::Sync::ExternalUsers#execute' do
+          expect(EE::Gitlab::LDAP::Sync::ExternalUsers).not_to receive(:execute)
+        end
+      end
+
+      context 'and admin_group is present' do
+        before do
+          stub_ldap_config(
+            group_base: 'dc=example,dc=com',
+            admin_group: 'my-admin-group'
+          )
+        end
+
+        it 'should call EE::Gitlab::LDAP::Sync::Group#execute' do
+          expect(EE::Gitlab::LDAP::Sync::Group).to receive(:execute).twice
+        end
+
+        it 'does not call EE::Gitlab::LDAP::Sync::AdminUsers#execute' do
+          expect(EE::Gitlab::LDAP::Sync::AdminUsers).to receive(:execute).once
+        end
+
+        it 'does not call EE::Gitlab::LDAP::Sync::ExternalUsers#execute' do
+          expect(EE::Gitlab::LDAP::Sync::ExternalUsers).not_to receive(:execute)
+        end
+      end
+
+      context 'and external_groups is present' do
+        before do
+          stub_ldap_config(
+            group_base: 'dc=example,dc=com',
+            external_groups: %w(external_group)
+          )
+        end
+
+        it 'should call EE::Gitlab::LDAP::Sync::Group#execute' do
+          expect(EE::Gitlab::LDAP::Sync::Group).to receive(:execute).twice
+        end
+
+        it 'does not call EE::Gitlab::LDAP::Sync::AdminUsers#execute' do
+          expect(EE::Gitlab::LDAP::Sync::AdminUsers).not_to receive(:execute)
+        end
+
+        it 'does not call EE::Gitlab::LDAP::Sync::ExternalUsers#execute' do
+          expect(EE::Gitlab::LDAP::Sync::ExternalUsers).to receive(:execute).once
+        end
+      end
+    end
+  end
+end

spec/lib/ee/gitlab/ldap/sync/proxy_spec.rb

+require 'spec_helper'
+require 'net/ldap/dn'
+
+describe EE::Gitlab::LDAP::Sync::Proxy, lib: true do
+  include LdapHelpers
+
+  let(:sync_proxy) { EE::Gitlab::LDAP::Sync::Proxy.new('ldapmain', adapter) }
+
+  before do
+    stub_ldap_config(active_directory: false)
+  end
+
+  describe '#dns_for_group_cn' do
+    it 'returns an empty array when LDAP group cannot be found' do
+      stub_ldap_group_find_by_cn('ldap_group1', nil, adapter)
+
+      expect(sync_proxy.dns_for_group_cn('ldap_group1')).to eq([])
+    end
+
+    it 'returns an empty array when LDAP group has no members' do
+      ldap_group = ldap_group_entry(nil)
+      stub_ldap_group_find_by_cn('ldap_group1', ldap_group, adapter)
+
+      expect(sync_proxy.dns_for_group_cn('ldap_group1')).to eq([])
+    end
+
+    context 'with a valid LDAP group that contains members' do
+      # Create some random usernames and DNs
+      let(:usernames) { (1..4).map { FFaker::Internet.user_name } }
+      let(:dns) { usernames.map { |u| user_dn(u) } }
+
+      it 'returns member DNs' do
+        ldap_group = ldap_group_entry(dns)
+        stub_ldap_group_find_by_cn('ldap_group1', ldap_group, adapter)
+
+        expect(sync_proxy.dns_for_group_cn('ldap_group1')).to match_array(dns)
+      end
+
+      it 'returns cached results after the first lookup' do
+        ldap_group = ldap_group_entry(dns)
+        stub_ldap_group_find_by_cn('ldap_group1', ldap_group, adapter)
+        # Do the first lookup to build the cache
+        sync_proxy.dns_for_group_cn('ldap_group1')
+
+        expect(sync_proxy).not_to receive(:ldap_group_member_dns)
+        expect(EE::Gitlab::LDAP::Group).not_to receive(:find_by_cn)
+
+        sync_proxy.dns_for_group_cn('ldap_group1')
+      end
+
+      # posixGroup - Apple Open Directory
+      it 'returns member DNs for posixGroup' do
+        ldap_group = ldap_group_entry(
+          usernames,
+          objectclass: 'posixGroup',
+          member_attr: 'memberUid'
+        )
+        stub_ldap_group_find_by_cn('ldap_group1', ldap_group, adapter)
+        usernames.each do |username|
+          stub_ldap_person_find_by_uid(username, ldap_user_entry(username))
+        end
+
+        expect(sync_proxy.dns_for_group_cn('ldap_group1')).to match_array(dns)
+      end
+
+      it 'returns member DNs when member value is in uid=<user> format' do
+        ldap_group = ldap_group_entry(usernames.map { |u| "uid=#{u}" })
+        stub_ldap_group_find_by_cn('ldap_group1', ldap_group, adapter)
+        usernames.each do |username|
+          stub_ldap_person_find_by_uid(username, ldap_user_entry(username))
+        end
+
+        expect(sync_proxy.dns_for_group_cn('ldap_group1')).to match_array(dns)
+      end
+
+      it 'returns valid DNs while gracefully skipping malformed DNs' do
+        mixed_dns = dns.dup << 'invalid_dn'
+        ldap_group = ldap_group_entry(mixed_dns)
+        stub_ldap_group_find_by_cn('ldap_group1', ldap_group, adapter)
+
+        expect(sync_proxy.dns_for_group_cn('ldap_group1')).to match_array(dns)
+      end
+
+      it 'returns valid DNs while gracefully handling empty entries' do
+        mixed_dns = dns.dup << ''
+        ldap_group = ldap_group_entry(mixed_dns)
+        stub_ldap_group_find_by_cn('ldap_group1', ldap_group, adapter)
+
+        expect(sync_proxy.dns_for_group_cn('ldap_group1')).to match_array(dns)
+      end
+    end
+  end
+
+  describe '#dn_for_uid' do
+    it 'returns nil when no user is found' do
+      stub_ldap_person_find_by_uid('john_doe', nil)
+
+      expect(sync_proxy.dn_for_uid('john_doe')).to be_nil
+    end
+
+    context 'when secondary_extern_uid is not stored in the database' do
+      before do
+        ldap_user_entry = ldap_user_entry('jane_doe')
+        stub_ldap_person_find_by_uid('jane_doe', ldap_user_entry, adapter)
+      end
+
+      it 'returns the user DN' do
+        expect(sync_proxy.dn_for_uid('jane_doe'))
+          .to eq('uid=jane_doe,ou=users,dc=example,dc=com')
+      end
+
+      it 'retrieves the user from LDAP' do
+        expect(::Gitlab::LDAP::Person).to receive(:find_by_uid)
+
+        sync_proxy.dn_for_uid('jane_doe')
+      end
+
+      it 'returns cached results after the first lookup' do
+        sync_proxy.dn_for_uid('jane_doe')
+
+        expect(sync_proxy).not_to receive(:member_uid_to_dn)
+        expect(Identity).not_to receive(:find_by)
+        expect(::Gitlab::LDAP::Person).not_to receive(:find_by_uid)
+
+        sync_proxy.dn_for_uid('jane_doe')
+      end
+
+      it 'saves the secondary_extern_uid' do
+        user = create(:user)
+        create(:identity, user: user, extern_uid: user_dn(user.username))
+        ldap_user_entry = ldap_user_entry(user.username)
+        stub_ldap_person_find_by_uid(user.username, ldap_user_entry, adapter)
+
+        expect { sync_proxy.dn_for_uid(user.username) }
+          .to change {
+            user.identities.first.secondary_extern_uid
+          }.from(nil).to(user.username)
+      end
+
+      it 'is graceful when no user with LDAP identity is found' do
+        # Create a user with no LDAP identity
+        user = create(:user)
+        ldap_user_entry = ldap_user_entry(user.username)
+        stub_ldap_person_find_by_uid(user.username, ldap_user_entry, adapter)
+
+        expect { sync_proxy.dn_for_uid(user.username) }.not_to raise_error
+      end
+    end
+
+    context 'when secondary_extern_uid is stored in the database' do
+      let(:user) { create(:user) }
+
+      before do
+        create(
+          :identity,
+          user: user,
+          extern_uid: user_dn(user.username),
+          secondary_extern_uid: user.username
+        )
+      end
+
+      after { sync_proxy.dn_for_uid(user.username) }
+
+      it 'does not query LDAP' do
+        expect(::Gitlab::LDAP::Person).not_to receive(:find_by_uid)
+      end
+
+      it 'retrieves the DN from the identity' do
+        expect(Identity)
+          .to receive(:find_by)
+                .with(
+                  provider: sync_proxy.provider,
+                  secondary_extern_uid: user.username
+                ).once.and_call_original
+      end
+    end
+  end
+end

spec/support/ldap_helpers.rb

+module LdapHelpers
+  def adapter(provider = 'ldapmain')
+    ::Gitlab::LDAP::Adapter.new(provider, double(:ldap))
+  end
+
+  def proxy(adapter, provider = 'ldapmain')
+    EE::Gitlab::LDAP::Sync::Proxy.new(provider, adapter)
+  end
+
+  def user_dn(uid)
+    "uid=#{uid},ou=users,dc=example,dc=com"
+  end
+
+  # Accepts a hash of Gitlab::LDAP::Config keys and values.
+  #
+  # Example:
+  #   stub_ldap_config(
+  #     group_base: 'ou=groups,dc=example,dc=com',
+  #     admin_group: 'my-admin-group'
+  #   )
+  def stub_ldap_config(messages)
+    messages.each do |config, value|
+      allow_any_instance_of(::Gitlab::LDAP::Config)
+        .to receive(config.to_sym).and_return(value)
+    end
+  end
+
+  # Stub an LDAP person search and provide the return entry. Specify `nil` for
+  # `entry` to simulate when an LDAP person is not found
+  #
+  # Example:
+  #  adapter = ::Gitlab::LDAP::Adapter.new('ldapmain', double(:ldap))
+  #  ldap_user_entry = ldap_user_entry('john_doe')
+  #
+  #  stub_ldap_person_find_by_uid('john_doe', ldap_user_entry, adapter)
+  def stub_ldap_person_find_by_uid(uid, entry, provider = 'ldapmain')
+    return_value = if entry.present?
+                     ::Gitlab::LDAP::Person.new(entry, provider)
+                   else
+                     nil
+                   end
+
+    allow(::Gitlab::LDAP::Person)
+      .to receive(:find_by_uid).with(uid, any_args).and_return(return_value)
+  end
+
+  # Stub an LDAP group search and provide the return entry. Specify `nil` for
+  # `entry` to simulate when an LDAP group is not found
+  #
+  # Example:
+  #  adapter = ::Gitlab::LDAP::Adapter.new('ldapmain', double(:ldap))
+  #  ldap_group1 = ldap_group_entry('uid=user,ou=users,dc=example,dc=com')
+  #
+  #  stub_ldap_group_find_by_cn('ldap_group1', ldap_group1, adapter)
+  def stub_ldap_group_find_by_cn(cn, entry, adapter = nil)
+    return_value = if entry.present?
+                     EE::Gitlab::LDAP::Group.new(entry, adapter)
+                   else
+                     nil
+                   end
+
+    allow(EE::Gitlab::LDAP::Group)
+      .to receive(:find_by_cn)
+        .with(cn, kind_of(::Gitlab::LDAP::Adapter)).and_return(return_value)
+  end
+
+  # Create a simple LDAP user entry.
+  def ldap_user_entry(uid)
+    Net::LDAP::Entry.from_single_ldif_string("dn: #{user_dn(uid)}")
+  end
+
+  # Create an LDAP group entry with any number of members. By default, creates
+  # a groupOfNames style entry. Change the style by specifying the object class
+  # and member attribute name. The last example below shows how to specify a
+  # posixGroup (Apple Open Directory) entry. `members` can be nil to create
+  # an empty group.
+  #
+  # Example:
+  #   ldap_group_entry('uid=user,ou=users,dc=example,dc=com')
+  #
+  #   ldap_group_entry(
+  #     'uid=user1,ou=users,dc=example,dc=com',
+  #     'uid=user2,ou=users,dc=example,dc=com'
+  #   )
+  #
+  #   ldap_group_entry(
+  #     [ 'user1', 'user2' ],
+  #     cn: 'my_group'
+  #     objectclass: 'posixGroup',
+  #     member_attr: 'memberUid'
+  #   )
+  def ldap_group_entry(
+    members,
+    cn: 'ldap_group1',
+    objectclass: 'groupOfNames',
+    member_attr: 'uniqueMember'
+  )
+    entry = Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc)
+      dn: cn=#{cn},ou=groups,dc=example,dc=com
+      cn: #{cn}
+      description: LDAP Group #{cn}
+      objectclass: top
+      objectclass: #{objectclass}
+    EOS
+
+    members = [members].flatten
+    entry[member_attr] = members if members.any?
+    entry
+  end
+end