From 8d5f05e76af627231933c33124192c5639013158 Mon Sep 17 00:00:00 2001
From: Drew Blessing <drew@gitlab.com>
Date: Wed, 13 Apr 2016 16:48:03 -0500
Subject: [PATCH] Sanitize DNs before storing in extern_uid

---
 .../20160413213231_clean_ldap_extern_uids.rb  | 30 +++++++++++++++++++
 db/schema.rb                                  |  2 +-
 lib/gitlab/ldap/group_sync.rb                 |  8 +++++
 lib/gitlab/ldap/person.rb                     | 13 ++++++++
 lib/gitlab/ldap/user.rb                       |  3 +-
 spec/lib/gitlab/ldap/person_spec.rb           | 13 ++++++++
 6 files changed, 67 insertions(+), 2 deletions(-)
 create mode 100644 db/migrate/20160413213231_clean_ldap_extern_uids.rb

diff --git a/db/migrate/20160413213231_clean_ldap_extern_uids.rb b/db/migrate/20160413213231_clean_ldap_extern_uids.rb
new file mode 100644
index 00000000000000..78aee310fd741b
--- /dev/null
+++ b/db/migrate/20160413213231_clean_ldap_extern_uids.rb
@@ -0,0 +1,30 @@
+require 'net/ldap/dn'
+
+class CleanLdapExternUids < ActiveRecord::Migration
+  def up
+    select_all("SELECT id, extern_uid FROM identities WHERE identities.provider LIKE 'ldap%'").each do |ldap_identity|
+      id = ldap_identity['id']
+      extern_uid_was = ldap_identity['extern_uid'].clone
+      extern_uid = clean_dn(ldap_identity['extern_uid'])
+
+      if extern_uid != extern_uid_was
+        execute("UPDATE identities SET extern_uid = '#{quote_string(extern_uid)}' WHERE id = #{id}")
+      end
+    end
+  end
+
+  def down
+    # There's no going back! Shouldn't be a big deal.
+  end
+
+  private
+
+  def clean_dn(dn)
+    clean_pairs = []
+    Net::LDAP::DN.new(dn).each_pair do |k,v|
+      # No need to escape the value. `each_pair` already escapes for us.
+      clean_pairs << "#{k}=#{v}"
+    end
+    Net::LDAP::DN.new(clean_pairs.join(',')).to_s
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 199f3e0249fae1..70755f816ef935 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20160331223143) do
+ActiveRecord::Schema.define(version: 20160413213231) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
diff --git a/lib/gitlab/ldap/group_sync.rb b/lib/gitlab/ldap/group_sync.rb
index 38207d7f077a3e..26e72c3ac659a4 100644
--- a/lib/gitlab/ldap/group_sync.rb
+++ b/lib/gitlab/ldap/group_sync.rb
@@ -168,6 +168,10 @@ def ldap_group_member_dns(ldap_group_cn)
           member_dns = members.map { |uid| dn_for_uid(uid) }.compact
         end
 
+        # Sanitize the DNs to avoid problems when looking up identities later.
+        # See gitlab-ee#365
+        clean_dns!(member_dns)
+
         logger.debug { "Members in '#{ldap_group.name}' LDAP group: #{member_dns}" }
 
         member_dns
@@ -192,6 +196,10 @@ def member_uid_to_dn(uid)
         end
       end
 
+      def clean_dns!(dns)
+        dns.map! { |dn| Gitlab::LDAP::Person.clean_dn(dn) }
+      end
+
       def update_identity(dn, uid)
         identity =
           Identity.find_by(provider: provider, extern_uid: dn)
diff --git a/lib/gitlab/ldap/person.rb b/lib/gitlab/ldap/person.rb
index 4cdaf30cd9ce83..3cb2a908c0d19e 100644
--- a/lib/gitlab/ldap/person.rb
+++ b/lib/gitlab/ldap/person.rb
@@ -1,3 +1,5 @@
+require 'net/ldap/dn'
+
 module Gitlab
   module LDAP
     class Person
@@ -21,6 +23,17 @@ def self.disabled_via_active_directory?(dn, adapter)
         adapter.dn_matches_filter?(dn, AD_USER_DISABLED)
       end
 
+      # Sometimes LDAP contains dirty DNs. `Net::LDAP::DN.each_pair` does some
+      # magical clean up and then we put it all back together. See gitlab-ee#365
+      def self.clean_dn(dn)
+        clean_pairs = []
+        Net::LDAP::DN.new(dn).each_pair do |k,v|
+          # No need to escape the value. `each_pair` already escapes for us.
+          clean_pairs << "#{k}=#{v}"
+        end
+        Net::LDAP::DN.new(clean_pairs.join(',')).to_s
+      end
+
       def initialize(entry, provider)
         Rails.logger.debug { "Instantiating #{self.class.name} with LDIF:\n#{entry.to_ldif}" }
         @entry = entry
diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb
index b84c81f1a6cd09..c127291e72c978 100644
--- a/lib/gitlab/ldap/user.rb
+++ b/lib/gitlab/ldap/user.rb
@@ -52,10 +52,11 @@ def update_user_attributes
           identity = gl_user.identities.find { |identity|  identity.provider == auth_hash.provider }
           identity ||= gl_user.identities.build(provider: auth_hash.provider)
 
+          # Store a sanitized DN to avoid data consistency problems later. See gitlab-ee#365
           # For a new identity set extern_uid to the LDAP DN
           # For an existing identity with matching email but changed DN, update the DN.
           # For an existing identity with no change in DN, this line changes nothing.
-          identity.extern_uid = auth_hash.uid
+          identity.extern_uid = Gitlab::LDAP::Person.clean_dn(auth_hash.uid)
         end
 
         gl_user.ldap_email = auth_hash.has_email?
diff --git a/spec/lib/gitlab/ldap/person_spec.rb b/spec/lib/gitlab/ldap/person_spec.rb
index ac5b4dc3acb868..b51c179d600340 100644
--- a/spec/lib/gitlab/ldap/person_spec.rb
+++ b/spec/lib/gitlab/ldap/person_spec.rb
@@ -1,6 +1,19 @@
 require "spec_helper"
 
 describe Gitlab::LDAP::Person do
+  describe '.clean_dn' do
+    it 'strips weird whitespace' do
+      dn = 'uid=johndoe, ou=people , dc=example,dc=com'
+      expect(Gitlab::LDAP::Person.clean_dn(dn)).
+        to eq('uid=johndoe,ou=people,dc=example,dc=com')
+    end
+
+    it 'does not change good DNs' do
+      dn = 'uid=janedoe,ou=people,dc=example,dc=com'
+      expect(Gitlab::LDAP::Person.clean_dn(dn)).
+        to eq('uid=janedoe,ou=people,dc=example,dc=com')
+    end
+  end
 
   describe "#kerberos_principal" do
 
-- 
GitLab