Ensure LDAP Group Sync by Filter normalizes DNs

Fixes #8241. The newer LDAP Group Sync feature to sync by
LDAP filter did not normalize (ultimately, downcase) the DNs
so membership comparison was not always happening correctly. This
resulted in members be re-added during each resync even if they
were already members. This caused unnecessary writes to the DB
and also reset the joined at/given access at date.

ee/changelogs/unreleased/fix_ldap_group_sync_filter_parsing.yml

+---
+title: Ensure LDAP Group Sync by Filter normalizes DNs
+merge_request:
+author:
+type: fixed

ee/lib/ee/gitlab/auth/ldap/adapter.rb

@@ -56,6 +56,13 @@ module EE
             end
           end
 
+          def filter_search(filter)
+            ldap_search(
+              base: config.base,
+              filter: Net::LDAP::Filter.construct(filter)
+            )
+          end
+
           def user_by_certificate_assertion(certificate_assertion)
             options = user_options_for_cert(certificate_assertion)
             users_search(options).first

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

@@ -123,7 +123,7 @@ module EE
             end
 
             def get_member_dns(group_link)
-              group_link.cn ? dns_for_group_cn(group_link.cn) : UserFilter.filter(@proxy, group_link.filter)
+              group_link.cn ? dns_for_group_cn(group_link.cn) : proxy.dns_for_filter(group_link.filter)
             end
 
             def dns_for_group_cn(group_cn)
@@ -136,10 +136,6 @@ module EE
               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" }
 

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

@@ -35,6 +35,11 @@ module EE
               @dn_for_uid[uid]
             end
 
+            def dns_for_filter(filter)
+              @dns_for_filter ||= Hash.new { |h, k| h[k] = dn_filter_search(k) }
+              @dns_for_filter[filter.downcase]
+            end
+
             private
 
             def ldap_group_member_dns(ldap_group_cn)
@@ -72,7 +77,8 @@ module EE
             def ensure_full_dns!(dns)
               dns.map! do |dn|
                 begin
-                  parsed_dn = ::Gitlab::Auth::LDAP::DN.new(dn).to_a
+                  dn_obj = ::Gitlab::Auth::LDAP::DN.new(dn)
+                  parsed_dn = dn_obj.to_a
                 rescue ::Gitlab::Auth::LDAP::DN::FormatError => e
                   logger.error { "Found malformed DN: '#{dn}'. Skipping. Error: \"#{e.message}\"" }
                   next
@@ -82,7 +88,7 @@ module EE
                   # 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
+                    dn_obj.to_normalized_s
                   elsif parsed_dn.count == 0
                     logger.warn { "Found null DN. Skipping." }
                     nil
@@ -143,6 +149,18 @@ module EE
             end
             # rubocop: enable CodeReuse/ActiveRecord
 
+            def dn_filter_search(filter)
+              logger.debug { "Running filter \"#{filter}\" against #{provider}" }
+
+              dns = adapter.filter_search(filter).map(&:dn)
+
+              ensure_full_dns!(dns)
+
+              logger.debug { "Found #{dns.count} matching users for filter #{filter}" }
+
+              dns
+            end
+
             def logger
               Rails.logger # rubocop:disable Gitlab/RailsLogger
             end

ee/lib/ee/gitlab/auth/ldap/user_filter.rb

-# frozen_string_literal: true
-
-module EE
-  module Gitlab
-    module Auth
-      module LDAP
-        class UserFilter
-          def self.filter(*args)
-            new(*args).filter
-          end
-
-          def initialize(proxy, filter)
-            @proxy = proxy
-            @filter = filter
-          end
-
-          def filter
-            logger.debug "Running filter #{@filter} against #{@proxy.provider}"
-
-            @proxy.adapter.ldap_search(options).map(&:dn).tap do |dns|
-              logger.debug "Found #{dns.count} mathing users for filter #{@filter}"
-            end
-          end
-
-          private
-
-          def options
-            { base: config.base, filter: construct_filter }
-          end
-
-          def construct_filter
-            Net::LDAP::Filter.construct(@filter)
-          end
-
-          def config
-            @proxy.adapter.config
-          end
-
-          def logger
-            Rails.logger # rubocop:disable Gitlab/RailsLogger
-          end
-        end
-      end
-    end
-  end
-end

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

@@ -428,11 +428,18 @@ describe EE::Gitlab::Auth::LDAP::Sync::Group do
     end
 
     describe '#update_permissions' do
+      let(:group) do
+        create(:group_with_ldap_group_filter_link,
+               :access_requestable,
+               group_access: ::Gitlab::Access::DEVELOPER)
+      end
+      let(:sync_group) { described_class.new(group, proxy(adapter)) }
+
       before do
         # Safe-check because some permissions are removed when `Group#ldap_synced?`
         # is true (e.g. in `GroupPolicy`).
         expect(group).to be_ldap_synced
-        allow(EE::Gitlab::Auth::LDAP::UserFilter).to receive(:filter).and_return([user_dn(user.username)])
+        allow(sync_group.proxy).to receive(:dns_for_filter).and_return([user_dn(user.username)])
 
         group.start_ldap_sync
       end
@@ -441,14 +448,6 @@ describe EE::Gitlab::Auth::LDAP::Sync::Group do
         group.finish_ldap_sync
       end
 
-      let(:group) do
-        create(:group_with_ldap_group_filter_link,
-               :access_requestable,
-               group_access: ::Gitlab::Access::DEVELOPER)
-      end
-
-      let(:sync_group) { described_class.new(group, proxy(adapter)) }
-
       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)) }

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

@@ -49,6 +49,14 @@ describe EE::Gitlab::Auth::LDAP::Sync::Proxy do
         expect(sync_proxy.dns_for_group_cn('ldap_group1')).to match_array(dns)
       end
 
+      it 'returns normalized DNs' do
+        ldap_group = ldap_group_entry(['UID=JaneDoe,OU=Users,DC=Example,DC=com'])
+        stub_ldap_group_find_by_cn('ldap_group1', ldap_group, adapter)
+
+        expect(sync_proxy.dns_for_group_cn('ldap_group1'))
+          .to match_array(['uid=janedoe,ou=users,dc=example,dc=com'])
+      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)
@@ -123,13 +131,13 @@ describe EE::Gitlab::Auth::LDAP::Sync::Proxy do
 
     context 'when secondary_extern_uid is not stored in the database' do
       before do
-        ldap_user_entry = ldap_user_entry('jane_doe')
+        ldap_user_entry = Net::LDAP::Entry.new
+        ldap_user_entry['dn'] = 'UID=Jane_Doe,OU=Users,DC=Example,DC=com'
         stub_ldap_person_find_by_uid('jane_doe', ldap_user_entry)
       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')
+      it 'returns the normalized 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
@@ -208,4 +216,34 @@ describe EE::Gitlab::Auth::LDAP::Sync::Proxy do
       end
     end
   end
+
+  describe '#dns_for_filter' do
+    let(:filter) { '(ou=people)' }
+
+    it 'returns DNs from an LDAP search' do
+      entry = ldap_user_entry('johndoe')
+
+      allow(adapter).to receive(:ldap_search).and_return([entry])
+
+      expect(sync_proxy.dns_for_filter(filter)).to eq(['uid=johndoe,ou=users,dc=example,dc=com'])
+    end
+
+    it 'normalizes DNs' do
+      entry = Net::LDAP::Entry.new
+      entry['dn'] = 'UID=JaneDoe,OU=Users,DC=Example,DC=com'
+
+      allow(adapter).to receive(:ldap_search).and_return([entry])
+
+      expect(sync_proxy.dns_for_filter(filter)).to eq(['uid=janedoe,ou=users,dc=example,dc=com'])
+    end
+
+    it 'returns cached results after the first lookup' do
+      allow(adapter).to receive(:ldap_search).and_return([]).once
+      sync_proxy.dns_for_filter(filter)
+
+      expect(sync_proxy).not_to receive(:dn_filter_search)
+
+      sync_proxy.dns_for_filter(filter)
+    end
+  end
 end

ee/spec/lib/ee/gitlab/auth/ldap/user_filter_spec.rb

-require 'spec_helper'
-
-describe EE::Gitlab::Auth::LDAP::UserFilter do
-  include LdapHelpers
-
-  let(:auth_hash) do
-    OmniAuth::AuthHash.new(uid: 'uid=john,ou=people,dc=example,dc=com', provider: 'ldapmain')
-  end
-  let!(:fake_proxy) { fake_ldap_sync_proxy(auth_hash.provider) }
-  let(:fake_entry) { ldap_user_entry('user1') }
-
-  before do
-    stub_ldap_config(
-      base: 'dc=example,dc=com',
-      active_directory: false
-    )
-
-    allow(fake_proxy).to receive(:provider)
-  end
-
-  describe '#filter' do
-    it 'returns dns from an LDAP search' do
-      filter = '(ou=people)'
-
-      allow(fake_proxy.adapter).to receive(:ldap_search).and_return([fake_entry])
-
-      expect(described_class.filter(fake_proxy, filter)).to eq(['uid=user1,ou=users,dc=example,dc=com'])
-    end
-
-    it 'errors out with an invalid filter' do
-      filter = ')('
-
-      expect { described_class.filter(fake_proxy, filter) }.to raise_error(Net::LDAP::FilterSyntaxInvalidError, 'Invalid filter syntax.')
-    end
-  end
-end

ee/spec/lib/gitlab/auth/ldap/adapter_spec.rb

@@ -40,6 +40,28 @@ describe Gitlab::Auth::LDAP::Adapter do
     end
   end
 
+  describe '#filter_search' do
+    before do
+      stub_ldap_config(
+        base: 'ou=my_groups,dc=example,dc=com'
+      )
+    end
+
+    it 'searches with the proper options' do
+      expect(adapter).to receive(:ldap_search) do |arg|
+        expect(arg[:filter].to_s).to eq('(ou=people)')
+        expect(arg[:base]).to eq('ou=my_groups,dc=example,dc=com')
+      end.and_return({})
+
+      adapter.filter_search('(ou=people)')
+    end
+
+    it 'errors out with an invalid filter' do
+      expect { adapter.filter_search(')(') }
+        .to raise_error(Net::LDAP::FilterSyntaxInvalidError, 'Invalid filter syntax.')
+    end
+  end
+
   describe '#user_by_certificate_assertion' do
     let(:certificate_assertion) { 'certificate_assertion' }