Leave bad DNs alone instead of raising errors

lib/gitlab/ldap/auth_hash.rb

@@ -4,7 +4,7 @@ module Gitlab
   module LDAP
     class AuthHash < Gitlab::OAuth::AuthHash
       def uid
-        Gitlab::LDAP::DN.new(super).to_normalized_s
+        Gitlab::LDAP::Person.normalize_dn(super)
       end
 
       private

lib/gitlab/ldap/person.rb

@@ -36,6 +36,14 @@ module Gitlab
         ]
       end
 
+      def self.normalize_dn(dn)
+        ::Gitlab::LDAP::DN.new(dn).to_normalized_s
+      rescue ::Gitlab::LDAP::DN::FormatError => e
+        Rails.logger.info("Returning original DN \"#{dn}\" due to error during normalization attempt: #{e.message}")
+
+        dn
+      end
+
       # Returns the UID in a normalized form.
       #
       # 1. Excess spaces are stripped
@@ -44,7 +52,6 @@ module Gitlab
         ::Gitlab::LDAP::DN.normalize_value(uid)
       rescue ::Gitlab::LDAP::DN::FormatError => e
         Rails.logger.info("Returning original UID \"#{uid}\" due to error during normalization attempt: #{e.message}")
-        Rails.logger.info(e.backtrace.join("\n"))
 
         uid
       end
@@ -72,7 +79,7 @@ module Gitlab
       end
 
       def dn
-        DN.new(entry.dn).to_normalized_s
+        self.class.normalize_dn(entry.dn)
       end
 
       private

spec/lib/gitlab/ldap/dn_spec.rb

@@ -78,41 +78,7 @@ describe Gitlab::LDAP::DN do
   describe '#to_normalized_s' do
     subject { described_class.new(given).to_normalized_s }
 
-    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'
-      'unescapes non-reserved, non-special Unicode characters'                                       | 'uid   =  Sebasti\\c3\\a1n\\ C.\\20Smith,   ou=People (aka. \\22humans\\")  ,dc=example, dc=com'      | 'uid=sebastián c. smith,ou=people (aka. \\"humans\\"),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'                                | 'uid=\\ John Smith,ou=People,dc=example,dc=com'                                                       | 'uid=\\ john smith,ou=people,dc=example,dc=com'
-      'does not strip an escaped leading space in the last attribute value'                          | 'uid=\\ John Smith'                                                                                   | 'uid=\\ john smith'
-      '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'
-      'strips extraneous spaces after an escaped trailing space'                                     | 'uid=John Smith\\   ,ou=People,dc=example,dc=com'                                                     | 'uid=john smith\\ ,ou=people,dc=example,dc=com'
-      'strips extraneous spaces after an escaped trailing space at the end of the DN'                | 'uid=John Smith,ou=People,dc=example,dc=com\\   '                                                     | 'uid=john smith,ou=people,dc=example,dc=com\\ '
-      'properly preserves escaped trailing space after unescaped trailing spaces'                    | 'uid=John Smith  \\  ,ou=People,dc=example,dc=com'                                                    | 'uid=john smith  \\ ,ou=people,dc=example,dc=com'
-      'preserves multiple inner spaces in an attribute value'                                        | 'uid=John   Smith,ou=People,dc=example,dc=com'                                                        | 'uid=john   smith,ou=people,dc=example,dc=com'
-      'preserves inner spaces after an escaped space'                                                | 'uid=John\\   Smith,ou=People,dc=example,dc=com'                                                      | 'uid=john   smith,ou=people,dc=example,dc=com'
-      'hex-escapes an escaped leading newline in an attribute value'                                 | "uid=\\\nJohn Smith,ou=People,dc=example,dc=com"                                                      | "uid=\\0ajohn smith,ou=people,dc=example,dc=com"
-      'hex-escapes and does not strip an escaped trailing newline in an attribute value'             | "uid=John Smith\\\n,ou=People,dc=example,dc=com"                                                      | "uid=john smith\\0a,ou=people,dc=example,dc=com"
-      'hex-escapes an unescaped leading newline (actually an invalid DN?)'                           | "uid=\nJohn Smith,ou=People,dc=example,dc=com"                                                        | "uid=\\0ajohn smith,ou=people,dc=example,dc=com"
-      'strips an unescaped trailing newline (actually an invalid DN?)'                               | "uid=John Smith\n,ou=People,dc=example,dc=com"                                                        | "uid=john smith,ou=people,dc=example,dc=com"
-      'does not strip if no extraneous whitespace'                                                   | 'uid=John Smith,ou=People,dc=example,dc=com'                                                          | 'uid=john smith,ou=people,dc=example,dc=com'
-      'does not modify an escaped equal sign in an attribute value'                                  | 'uid= foo  \\=  bar'                                                                                  | 'uid=foo  \\=  bar'
-      'converts an escaped hex equal sign to an escaped equal sign in an attribute value'            | 'uid= foo  \\3D  bar'                                                                                 | 'uid=foo  \\=  bar'
-      'does not modify an escaped comma in an attribute value'                                       | 'uid= John C. Smith, ou=San Francisco\\, CA'                                                          | 'uid=john c. smith,ou=san francisco\\, ca'
-      'converts an escaped hex comma to an escaped comma in an attribute value'                      | 'uid= John C. Smith, ou=San Francisco\\2C CA'                                                         | 'uid=john c. smith,ou=san francisco\\, ca'
-      'does not modify an escaped hex carriage return character in an attribute value'               | 'uid= John C. Smith, ou=San Francisco\\,\\0DCA'                                                       | 'uid=john c. smith,ou=san francisco\\,\\0dca'
-      'does not modify an escaped hex line feed character in an attribute value'                     | 'uid= John C. Smith, ou=San Francisco\\,\\0ACA'                                                       | 'uid=john c. smith,ou=san francisco\\,\\0aca'
-      'does not modify an escaped hex CRLF in an attribute value'                                    | 'uid= John C. Smith, ou=San Francisco\\,\\0D\\0ACA'                                                   | 'uid=john c. smith,ou=san francisco\\,\\0d\\0aca'
-      'allows attribute type name OIDs'                                                              | '0.9.2342.19200300.100.1.25=Example,0.9.2342.19200300.100.1.25=Com'                                   | '0.9.2342.19200300.100.1.25=example,0.9.2342.19200300.100.1.25=com'
-      'strips extraneous whitespace from attribute type name OIDs'                                   | '0.9.2342.19200300.100.1.25 = Example, 0.9.2342.19200300.100.1.25 = Com'                              | '0.9.2342.19200300.100.1.25=example,0.9.2342.19200300.100.1.25=com'
-    end
-
-    with_them do
-      it 'normalizes the DN' do
-        assert_generic_test(test_description, subject, expected)
-      end
-    end
+    it_behaves_like 'normalizes a DN'
 
     context 'when we do not support the given DN format' do
       context 'multivalued RDNs' do

spec/lib/gitlab/ldap/person_spec.rb

@@ -16,6 +16,20 @@ describe Gitlab::LDAP::Person do
     )
   end
 
+  describe '.normalize_dn' do
+    subject { described_class.normalize_dn(given) }
+
+    it_behaves_like 'normalizes a DN'
+
+    context 'with an exception during normalization' do
+      let(:given) { 'John "Smith,' } # just something that will cause an exception
+
+      it 'returns the given DN unmodified' do
+        expect(subject).to eq(given)
+      end
+    end
+  end
+
   describe '.normalize_uid' do
     subject { described_class.normalize_uid(given) }
 

spec/support/ldap_shared_examples.rb

+shared_examples_for 'normalizes a DN' do
+  using RSpec::Parameterized::TableSyntax
+
+  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'
+    'unescapes non-reserved, non-special Unicode characters'                                       | 'uid   =  Sebasti\\c3\\a1n\\ C.\\20Smith,   ou=People (aka. \\22humans\\")  ,dc=example, dc=com'      | 'uid=sebastián c. smith,ou=people (aka. \\"humans\\"),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'                                | 'uid=\\ John Smith,ou=People,dc=example,dc=com'                                                       | 'uid=\\ john smith,ou=people,dc=example,dc=com'
+    'does not strip an escaped leading space in the last attribute value'                          | 'uid=\\ John Smith'                                                                                   | 'uid=\\ john smith'
+    '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'
+    'strips extraneous spaces after an escaped trailing space'                                     | 'uid=John Smith\\   ,ou=People,dc=example,dc=com'                                                     | 'uid=john smith\\ ,ou=people,dc=example,dc=com'
+    'strips extraneous spaces after an escaped trailing space at the end of the DN'                | 'uid=John Smith,ou=People,dc=example,dc=com\\   '                                                     | 'uid=john smith,ou=people,dc=example,dc=com\\ '
+    'properly preserves escaped trailing space after unescaped trailing spaces'                    | 'uid=John Smith  \\  ,ou=People,dc=example,dc=com'                                                    | 'uid=john smith  \\ ,ou=people,dc=example,dc=com'
+    'preserves multiple inner spaces in an attribute value'                                        | 'uid=John   Smith,ou=People,dc=example,dc=com'                                                        | 'uid=john   smith,ou=people,dc=example,dc=com'
+    'preserves inner spaces after an escaped space'                                                | 'uid=John\\   Smith,ou=People,dc=example,dc=com'                                                      | 'uid=john   smith,ou=people,dc=example,dc=com'
+    'hex-escapes an escaped leading newline in an attribute value'                                 | "uid=\\\nJohn Smith,ou=People,dc=example,dc=com"                                                      | "uid=\\0ajohn smith,ou=people,dc=example,dc=com"
+    'hex-escapes and does not strip an escaped trailing newline in an attribute value'             | "uid=John Smith\\\n,ou=People,dc=example,dc=com"                                                      | "uid=john smith\\0a,ou=people,dc=example,dc=com"
+    'hex-escapes an unescaped leading newline (actually an invalid DN?)'                           | "uid=\nJohn Smith,ou=People,dc=example,dc=com"                                                        | "uid=\\0ajohn smith,ou=people,dc=example,dc=com"
+    'strips an unescaped trailing newline (actually an invalid DN?)'                               | "uid=John Smith\n,ou=People,dc=example,dc=com"                                                        | "uid=john smith,ou=people,dc=example,dc=com"
+    'does not strip if no extraneous whitespace'                                                   | 'uid=John Smith,ou=People,dc=example,dc=com'                                                          | 'uid=john smith,ou=people,dc=example,dc=com'
+    'does not modify an escaped equal sign in an attribute value'                                  | 'uid= foo  \\=  bar'                                                                                  | 'uid=foo  \\=  bar'
+    'converts an escaped hex equal sign to an escaped equal sign in an attribute value'            | 'uid= foo  \\3D  bar'                                                                                 | 'uid=foo  \\=  bar'
+    'does not modify an escaped comma in an attribute value'                                       | 'uid= John C. Smith, ou=San Francisco\\, CA'                                                          | 'uid=john c. smith,ou=san francisco\\, ca'
+    'converts an escaped hex comma to an escaped comma in an attribute value'                      | 'uid= John C. Smith, ou=San Francisco\\2C CA'                                                         | 'uid=john c. smith,ou=san francisco\\, ca'
+    'does not modify an escaped hex carriage return character in an attribute value'               | 'uid= John C. Smith, ou=San Francisco\\,\\0DCA'                                                       | 'uid=john c. smith,ou=san francisco\\,\\0dca'
+    'does not modify an escaped hex line feed character in an attribute value'                     | 'uid= John C. Smith, ou=San Francisco\\,\\0ACA'                                                       | 'uid=john c. smith,ou=san francisco\\,\\0aca'
+    'does not modify an escaped hex CRLF in an attribute value'                                    | 'uid= John C. Smith, ou=San Francisco\\,\\0D\\0ACA'                                                   | 'uid=john c. smith,ou=san francisco\\,\\0d\\0aca'
+    'allows attribute type name OIDs'                                                              | '0.9.2342.19200300.100.1.25=Example,0.9.2342.19200300.100.1.25=Com'                                   | '0.9.2342.19200300.100.1.25=example,0.9.2342.19200300.100.1.25=com'
+    'strips extraneous whitespace from attribute type name OIDs'                                   | '0.9.2342.19200300.100.1.25 = Example, 0.9.2342.19200300.100.1.25 = Com'                              | '0.9.2342.19200300.100.1.25=example,0.9.2342.19200300.100.1.25=com'
+  end
+
+  with_them do
+    it 'normalizes the DN' do
+      assert_generic_test(test_description, subject, expected)
+    end
+  end
+end
+
 shared_examples_for 'normalizes a DN attribute value' do
   using RSpec::Parameterized::TableSyntax