Switch to new DN class

for normalizing and parsing DNs

4 files changed, 10 insertions, 74 deletions

diff --git a/lib/gitlab/ldap/auth_hash.rb b/lib/gitlab/ldap/auth_hash.rb
index 3123da17fd9..2ea0a51b18f 100644
--- a/lib/gitlab/ldap/auth_hash.rb
+++ b/lib/gitlab/ldap/auth_hash.rb
@@ -4,7 +4,7 @@ module Gitlab
   module LDAP
     class AuthHash < Gitlab::OAuth::AuthHash
       def uid
-        Gitlab::LDAP::Person.normalize_dn(super)
+        Gitlab::LDAP::DN.new(super).to_s_normalized
       end
 
       private
diff --git a/lib/gitlab/ldap/person.rb b/lib/gitlab/ldap/person.rb
index 6b1a308d521..44cb6a065c9 100644
--- a/lib/gitlab/ldap/person.rb
+++ b/lib/gitlab/ldap/person.rb
@@ -49,21 +49,6 @@ module Gitlab
         uid
       end
 
-      # Returns the DN in a normalized form.
-      #
-      # 1. Excess spaces around attribute names and values are stripped
-      # 2. The string is downcased (for case-insensitivity)
-      def self.normalize_dn(dn)
-        dn.split(/(?<!\\)([,+=])/).map do |part|
-          normalize_dn_part(part)
-        end.join('')
-      rescue StandardError => e
-        Rails.logger.info("Returning original DN \"#{dn}\" due to error during normalization attempt: #{e.message}")
-        Rails.logger.info(e.backtrace.join("\n"))
-
-        dn
-      end
-
       def initialize(entry, provider)
         Rails.logger.debug { "Instantiating #{self.class.name} with LDIF:\n#{entry.to_ldif}" }
         @entry = entry
@@ -87,7 +72,7 @@ module Gitlab
       end
 
       def dn
-        self.class.normalize_dn(entry.dn)
+        DN.new(entry.dn).to_s_normalized
       end
 
       private
diff --git a/spec/lib/gitlab/ldap/person_spec.rb b/spec/lib/gitlab/ldap/person_spec.rb
index 38458549751..02904f1e351 100644
--- a/spec/lib/gitlab/ldap/person_spec.rb
+++ b/spec/lib/gitlab/ldap/person_spec.rb
@@ -17,41 +17,6 @@ describe Gitlab::LDAP::Person do
     )
   end
 
-  shared_examples_for 'normalizes the DN' do
-    # Regarding the telephoneNumber test:
-    #
-    # I am not sure whether a space after the telephoneNumber plus sign is valid,
-    # and I am not sure if this is "proper" behavior under these conditions, and
-    # I am not sure if it matters to us or anyone else, so rather than dig
-    # through RFCs, I am only documenting the behavior here.
-    where(:test_description, :given, :expected) do
-      'strips extraneous whitespace'                                                                 | 'uid     =John Smith ,  ou = People, dc=  example,dc =com'                                            | 'uid=john smith,ou=people,dc=example,dc=com'
-      'strips extraneous whitespace for a DN with a single RDN'                                      | 'uid  =  John Smith'                                                                                  | 'uid=john smith'
-      'strips extraneous whitespace without changing escaped characters'                             | 'uid   =  Sebasti\\c3\\a1n\\ C.\\20Smith\\   ,   ou=People (aka. \\22humans\\")  ,dc=example, dc=com' | 'uid=sebasti\\c3\\a1n\\ c.\\20smith\\ ,ou=people (aka. \\22humans\\"),dc=example,dc=com'
-      'strips extraneous whitespace without modifying the multivalued RDN'                           | 'uid = John Smith  + telephoneNumber  = +1 555-555-5555 , ou = People,dc=example,dc=com'              | 'uid=john smith+telephonenumber=+1 555-555-5555,ou=people,dc=example,dc=com'
-      'strips the space after the plus sign in the telephoneNumber'                                  | 'uid = John Smith  + telephoneNumber  = + 1 555-555-5555 , ou = People,dc=example,dc=com'             | 'uid=john smith+telephonenumber=+1 555-555-5555,ou=people,dc=example,dc=com'
-      'downcases the whole string'                                                                   | 'UID=John Smith,ou=People,dc=example,dc=com'                                                          | 'uid=john smith,ou=people,dc=example,dc=com'
-      'for a null DN (empty string), returns empty string and does not error'                        | ''                                                                                                    | ''
-      'does not strip an escaped leading space in an attribute value (and does not error like Net::LDAP::DN.new does)' | 'uid=\\ John Smith,ou=People,dc=example,dc=com'                                     | 'uid=\\ john smith,ou=people,dc=example,dc=com'
-      'does not strip an escaped trailing space in an attribute value'                               | 'uid=John Smith\\ ,ou=People,dc=example,dc=com'                                                       | 'uid=john smith\\ ,ou=people,dc=example,dc=com'
-      'does not strip an escaped leading newline in an attribute value'                              | 'uid=\\\nJohn Smith,ou=People,dc=example,dc=com'                                                      | 'uid=\\\njohn smith,ou=people,dc=example,dc=com'
-      'does not strip an escaped trailing newline in an attribute value'                             | 'uid=John Smith\\\n,ou=People,dc=example,dc=com'                                                      | 'uid=john smith\\\n,ou=people,dc=example,dc=com'
-      'does not strip an unescaped leading newline (actually an invalid DN)'                         | 'uid=\nJohn Smith,ou=People,dc=example,dc=com'                                                                               | 'uid=\njohn smith,ou=people,dc=example,dc=com'
-      'does not strip an unescaped trailing newline (actually an invalid DN)'                        | 'uid=John Smith\n ,ou=People,dc=example,dc=com'                                                                              | 'uid=john smith\n,ou=people,dc=example,dc=com'
-      'does not strip non whitespace'                                                                | 'uid=John Smith,ou=People,dc=example,dc=com'                                                          | 'uid=john smith,ou=people,dc=example,dc=com'
-      'does not treat escaped equal signs as attribute delimiters'                                   | 'uid= foo  \\=  bar'                                                                                  | 'uid=foo  \\=  bar'
-      'does not treat escaped hex equal signs as attribute delimiters'                               | 'uid= foo  \\3D  bar'                                                                                 | 'uid=foo  \\3d  bar'
-      'does not treat escaped commas as attribute delimiters'                                        | 'uid= John C. Smith, ou=San Francisco\\, CA'                                                          | 'uid=john c. smith,ou=san francisco\\, ca'
-      'does not treat escaped hex commas as attribute delimiters'                                    | 'uid= John C. Smith, ou=San Francisco\\2C CA'                                                         | 'uid=john c. smith,ou=san francisco\\2c ca'
-    end
-
-    with_them do
-      it 'normalizes the DN' do
-        assert_generic_test(test_description, subject, expected)
-      end
-    end
-  end
-
   shared_examples_for 'normalizes the UID' do
     where(:test_description, :given, :expected) do
       'strips extraneous whitespace'                                        | ' John C. Smith   '                     | 'john c. smith'
@@ -91,20 +56,6 @@ describe Gitlab::LDAP::Person do
     end
   end
 
-  describe '.normalize_dn' do
-    subject { described_class.normalize_dn(given) }
-
-    it_behaves_like 'normalizes the DN'
-
-    context 'with an exception during normalization' do
-      let(:given) { described_class } # just something that will cause an exception
-
-      it 'returns the given DN unmodified' do
-        expect(subject).to eq(given)
-      end
-    end
-  end
-
   describe '#name' do
     it 'uses the configured name attribute and handles values as an array' do
       name = 'John Doe'
diff --git a/spec/lib/gitlab/ldap/user_spec.rb b/spec/lib/gitlab/ldap/user_spec.rb
index 6a6e465cea2..9a4705d1cee 100644
--- a/spec/lib/gitlab/ldap/user_spec.rb
+++ b/spec/lib/gitlab/ldap/user_spec.rb
@@ -11,7 +11,7 @@ describe Gitlab::LDAP::User do
     }
   end
   let(:auth_hash) do
-    OmniAuth::AuthHash.new(uid: 'my-uid', provider: 'ldapmain', info: info)
+    OmniAuth::AuthHash.new(uid: 'uid=John Smith,ou=People,dc=example,dc=com', provider: 'ldapmain', info: info)
   end
   let(:ldap_user_upper_case) { described_class.new(auth_hash_upper_case) }
   let(:info_upper_case) do
@@ -22,12 +22,12 @@ describe Gitlab::LDAP::User do
     }
   end
   let(:auth_hash_upper_case) do
-    OmniAuth::AuthHash.new(uid: 'my-uid', provider: 'ldapmain', info: info_upper_case)
+    OmniAuth::AuthHash.new(uid: 'uid=John Smith,ou=People,dc=example,dc=com', provider: 'ldapmain', info: info_upper_case)
   end
 
   describe '#changed?' do
     it "marks existing ldap user as changed" do
-      create(:omniauth_user, extern_uid: 'my-uid', provider: 'ldapmain')
+      create(:omniauth_user, extern_uid: 'uid=John Smith,ou=People,dc=example,dc=com', provider: 'ldapmain')
       expect(ldap_user.changed?).to be_truthy
     end
 
@@ -37,7 +37,7 @@ describe Gitlab::LDAP::User do
     end
 
     it "does not mark existing ldap user as changed" do
-      create(:omniauth_user, email: 'john@example.com', extern_uid: 'my-uid', provider: 'ldapmain')
+      create(:omniauth_user, email: 'john@example.com', extern_uid: 'uid=john smith,ou=people,dc=example,dc=com', provider: 'ldapmain')
       ldap_user.gl_user.user_synced_attributes_metadata(provider: 'ldapmain', email: true)
       expect(ldap_user.changed?).to be_falsey
     end
@@ -60,7 +60,7 @@ describe Gitlab::LDAP::User do
 
   describe 'find or create' do
     it "finds the user if already existing" do
-      create(:omniauth_user, extern_uid: 'my-uid', provider: 'ldapmain')
+      create(:omniauth_user, extern_uid: 'uid=John Smith,ou=People,dc=example,dc=com', provider: 'ldapmain')
 
       expect { ldap_user.save }.not_to change { User.count }
     end
@@ -70,7 +70,7 @@ describe Gitlab::LDAP::User do
       expect { ldap_user.save }.not_to change { User.count }
 
       existing_user.reload
-      expect(existing_user.ldap_identity.extern_uid).to eql 'my-uid'
+      expect(existing_user.ldap_identity.extern_uid).to eql 'uid=john smith,ou=people,dc=example,dc=com'
       expect(existing_user.ldap_identity.provider).to eql 'ldapmain'
     end
 
@@ -79,7 +79,7 @@ describe Gitlab::LDAP::User do
       expect { ldap_user.save }.not_to change { User.count }
 
       existing_user.reload
-      expect(existing_user.ldap_identity.extern_uid).to eql 'my-uid'
+      expect(existing_user.ldap_identity.extern_uid).to eql 'uid=john smith,ou=people,dc=example,dc=com'
       expect(existing_user.ldap_identity.provider).to eql 'ldapmain'
       expect(existing_user.id).to eql ldap_user.gl_user.id
     end
@@ -89,7 +89,7 @@ describe Gitlab::LDAP::User do
       expect { ldap_user_upper_case.save }.not_to change { User.count }
 
       existing_user.reload
-      expect(existing_user.ldap_identity.extern_uid).to eql 'my-uid'
+      expect(existing_user.ldap_identity.extern_uid).to eql 'uid=john smith,ou=people,dc=example,dc=com'
       expect(existing_user.ldap_identity.provider).to eql 'ldapmain'
       expect(existing_user.id).to eql ldap_user.gl_user.id
     end