diff --git a/CHANGELOG-EE b/CHANGELOG-EE index 3603f62566c50ab370344b3a40ad6a5f1b942665..77227daf0356412740c1f4276b3075f92a49939c 100644 --- a/CHANGELOG-EE +++ b/CHANGELOG-EE @@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.9.0 (unreleased) v 8.8.3 + - Gracefully handle malformed DNs in LDAP group sync - Make it clear the license overusage is visible only to admins - Reduce load on DB for license upgrade check diff --git a/lib/gitlab/ldap/group_sync.rb b/lib/gitlab/ldap/group_sync.rb index 44775d08dc24f0eb2dfaa6c1fcbddf8b5c852795..203328cff5fba0f7824e1af194092046f0024b78 100644 --- a/lib/gitlab/ldap/group_sync.rb +++ b/lib/gitlab/ldap/group_sync.rb @@ -180,7 +180,14 @@ def ldap_group_member_dns(ldap_group_cn) # account for that. See gitlab-ee#442 def ensure_full_dns!(dns) dns.map! do |dn| - parsed_dn = Net::LDAP::DN.new(dn).to_a + 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 @@ -192,6 +199,9 @@ def ensure_full_dns!(dns) dn end end + + # Remove `nil` values generated by the rescue above. + dns.compact! end def member_uid_to_dn(uid) diff --git a/spec/lib/gitlab/ldap/group_sync_spec.rb b/spec/lib/gitlab/ldap/group_sync_spec.rb index 197327820e7c9b8d03d24562e2235b36798f9196..f6a1f9a20841e7db9859d238abe6dbb64e9ab68f 100644 --- a/spec/lib/gitlab/ldap/group_sync_spec.rb +++ b/spec/lib/gitlab/ldap/group_sync_spec.rb @@ -548,6 +548,40 @@ }.from(nil).to(user1.username) end end + + context 'with invalid DNs in the LDAP group' do + let(:ldap_group) do + Gitlab::LDAP::Group.new( + Net::LDAP::Entry.from_single_ldif_string( + <<-EOS.strip_heredoc + dn: cn=ldap_group1,ou=groups,dc=example,dc=com + cn: ldap_group1 + member: + member: uid=#{user1.username},ou=users,dc=example,dc=com + member: foo, bar + description: LDAP Group 1 + objectclass: top + objectclass: groupOfUniqueNames + EOS + ) + ) + end + + # Check that the blank member and malformed member logged an error + it 'expects two errors to be logged' do + expect(Rails.logger).to receive(:error) do |&block| + expect(block.call).to match /^Found malformed DN:/ + end.twice + + group_sync.sync_groups + end + + it 'expects the valid user to be added' do + expect { group_sync.sync_groups } + .to change { group1.members.where(user_id: user1.id).any? } + .from(false).to(true) + end + end end end