From fb3df28a3d7de578ca4a11bc288ff8d12f74afd3 Mon Sep 17 00:00:00 2001
From: Drew Blessing <drew@gitlab.com>
Date: Wed, 11 May 2016 14:39:38 -0500
Subject: [PATCH] Gracefully handle malformed DNs in LDAP group sync

---
 CHANGELOG-EE                            |  1 +
 lib/gitlab/ldap/group_sync.rb           | 12 ++++++++-
 spec/lib/gitlab/ldap/group_sync_spec.rb | 34 +++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/CHANGELOG-EE b/CHANGELOG-EE
index 3603f62566c50a..77227daf035641 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 44775d08dc24f0..203328cff5fba0 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 197327820e7c9b..f6a1f9a20841e7 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
 
-- 
GitLab