Gracefully handle malformed DNs in LDAP group sync

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
 

lib/gitlab/ldap/group_sync.rb

@@ -180,7 +180,14 @@ module Gitlab
       # 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 @@ module Gitlab
             dn
           end
         end
+
+        # Remove `nil` values generated by the rescue above.
+        dns.compact!
       end
 
       def member_uid_to_dn(uid)

spec/lib/gitlab/ldap/group_sync_spec.rb

@@ -548,6 +548,40 @@ describe Gitlab::LDAP::GroupSync, lib: true do
             }.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