Merge branch 'fix_ldap_group_sync_filter_parsing' into 'master'

Ensure LDAP Group Sync by Filter sanitizes DNs

Closes #8241 and #8285

See merge request gitlab-org/gitlab-ee!8241

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' }