Refactor to distinguish between UIDs and DNs

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::Person.normalize_uid_or_dn(super)
       end
 
       private

lib/gitlab/ldap/person.rb

@@ -36,6 +36,35 @@ module Gitlab
         ]
       end
 
+      # Returns the UID or DN in a normalized form
+      def self.normalize_uid_or_dn(uid_or_dn)
+        if is_dn?(uid_or_dn)
+          normalize_dn(uid_or_dn)
+        else
+          normalize_uid(uid_or_dn)
+        end
+      end
+
+      # Returns true if the string looks like a DN rather than a UID.
+      #
+      # An empty string is technically a valid DN (null DN), although we should
+      # never need to worry about that.
+      def self.is_dn?(uid_or_dn)
+        uid_or_dn.blank? || uid_or_dn.include?('=')
+      end
+
+      # Returns the UID in a normalized form.
+      #
+      # 1. Excess spaces are stripped
+      # 2. The string is downcased (for case-insensitivity)
+      def self.normalize_uid(uid)
+        normalize_dn_part(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)

spec/lib/gitlab/ldap/person_spec.rb

@@ -16,6 +16,146 @@ describe Gitlab::LDAP::Person do
     )
   end
 
+  describe '.normalize_uid_or_dn' do
+    context 'given a DN' do
+      context 'when there is extraneous (but valid) whitespace' do
+        it 'removes the extraneous whitespace' do
+          given    = 'uid     =John Smith ,  ou = People, dc=  example,dc =com'
+          expected = 'uid=John Smith,ou=People,dc=example,dc=com'
+          expect(described_class.normalize_uid_or_dn(given)).to eq(expected)
+        end
+
+        context 'for a DN with a single RDN' do
+          it 'removes the extraneous whitespace' do
+            given    = 'uid  =  John Smith'
+            expected = 'uid=John Smith'
+            expect(described_class.normalize_uid_or_dn(given)).to eq(expected)
+          end
+        end
+
+        context 'when there are escaped characters' do
+          it 'removes extraneous whitespace without changing the escaped characters' do
+            given    = 'uid   =  Sebasti\\c3\\a1n\\ C.\\20Smith\\   ,   ou=People (aka. \\22humans\\")  ,dc=example, dc=com'
+            expected = 'uid=Sebasti\\c3\\a1n\\ C.\\20Smith\\ ,ou=People (aka. \\22humans\\"),dc=example,dc=com'
+            expect(described_class.normalize_uid_or_dn(given)).to eq(expected)
+          end
+        end
+
+        context 'with a multivalued RDN' do
+          it 'removes extraneous whitespace without modifying the multivalued RDN' do
+            given    = 'uid = John Smith  + telephoneNumber  = +1 555-555-5555 , ou = People,dc=example,dc=com'
+            expected = 'uid=John Smith+telephoneNumber=+1 555-555-5555,ou=People,dc=example,dc=com'
+            expect(described_class.normalize_uid_or_dn(given)).to eq(expected)
+          end
+
+          context 'with a telephoneNumber with a space after the plus sign' do
+            # 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.
+            it 'removes the space after the plus sign in the telephoneNumber' do
+              given    = 'uid = John Smith  + telephoneNumber  = + 1 555-555-5555 , ou = People,dc=example,dc=com'
+              expected = 'uid=John Smith+telephoneNumber=+1 555-555-5555,ou=People,dc=example,dc=com'
+              expect(described_class.normalize_uid_or_dn(given)).to eq(expected)
+            end
+          end
+        end
+      end
+
+      context 'for a null DN (empty string)' do
+        it 'returns empty string and does not error' do
+          given    = ''
+          expected = ''
+          expect(described_class.normalize_uid_or_dn(given)).to eq(expected)
+        end
+      end
+
+      context 'when there is an escaped leading space in an attribute value' do
+        it 'does not remove the escaped leading space (and does not error like Net::LDAP::DN.new does)' do
+          given    = 'uid=\\ John Smith,ou=People,dc=example,dc=com'
+          expected = 'uid=\\ John Smith,ou=People,dc=example,dc=com'
+          expect(described_class.normalize_uid_or_dn(given)).to eq(expected)
+        end
+      end
+
+      context 'when there is an escaped trailing space in an attribute value' do
+        it 'does not remove the escaped trailing space' do
+          given    = 'uid=John Smith\\ ,ou=People,dc=example,dc=com'
+          expected = 'uid=John Smith\\ ,ou=People,dc=example,dc=com'
+          expect(described_class.normalize_uid_or_dn(given)).to eq(expected)
+        end
+      end
+
+      context 'when there is an escaped leading newline in an attribute value' do
+        it 'does not remove the escaped leading newline' do
+          given    = 'uid=\\\nJohn Smith,ou=People,dc=example,dc=com'
+          expected = 'uid=\\\nJohn Smith,ou=People,dc=example,dc=com'
+          expect(described_class.normalize_uid_or_dn(given)).to eq(expected)
+        end
+      end
+
+      context 'when there is an escaped trailing newline in an attribute value' do
+        it 'does not remove the escaped trailing newline' do
+          given    = 'uid=John Smith\\\n,ou=People,dc=example,dc=com'
+          expected = 'uid=John Smith\\\n,ou=People,dc=example,dc=com'
+          expect(described_class.normalize_uid_or_dn(given)).to eq(expected)
+        end
+      end
+
+      context 'when there is an unescaped leading newline in an attribute value' do
+        it 'does not remove the unescaped leading newline' do
+          given    = 'uid=\nJohn Smith,ou=People,dc=example,dc=com'
+          expected = 'uid=\nJohn Smith,ou=People,dc=example,dc=com'
+          expect(described_class.normalize_uid_or_dn(given)).to eq(expected)
+        end
+      end
+
+      context 'when there is an unescaped trailing newline in an attribute value' do
+        it 'does not remove the unescaped trailing newline' do
+          given    = 'uid=John Smith\n ,ou=People,dc=example,dc=com'
+          expected = 'uid=John Smith\n,ou=People,dc=example,dc=com'
+          expect(described_class.normalize_uid_or_dn(given)).to eq(expected)
+        end
+      end
+
+      context 'with uppercase characters' do
+        # We may need to normalize casing at some point.
+        # I am just making it explicit that we don't at this time.
+        it 'returns the DN with unmodified casing' do
+          given    = 'UID=John Smith,ou=People,dc=example,dc=com'
+          expected = 'UID=John Smith,ou=People,dc=example,dc=com'
+          expect(described_class.normalize_uid_or_dn(given)).to eq(expected)
+        end
+      end
+
+      context 'with a malformed DN' do
+        context 'when an equal sign is escaped' do
+          it 'returns the DN completely unmodified' do
+            given    = 'uid= foo\\=bar'
+            expected = 'uid= foo\\=bar'
+            expect(described_class.normalize_uid_or_dn(given)).to eq(expected)
+          end
+        end
+      end
+    end
+
+    context 'given a UID' do
+      it 'returns the UID (with whitespace stripped)' do
+        given    = '  John C. Smith '
+        expected = 'John C. Smith'
+        expect(described_class.normalize_uid_or_dn(given)).to eq(expected)
+      end
+    end
+  end
+
+  describe '.normalize_uid' do
+    it 'returns the UID (with whitespace stripped)' do
+      given    = '  John C. Smith '
+      expected = 'John C. Smith'
+      expect(described_class.normalize_uid(given)).to eq(expected)
+    end
+  end
+
   describe '.normalize_dn' do
     context 'when there is extraneous (but valid) whitespace' do
       it 'removes the extraneous whitespace' do
@@ -128,14 +268,6 @@ describe Gitlab::LDAP::Person do
     end
 
     context 'with a malformed DN' do
-      context 'when passed a UID instead of a DN' do
-        it 'returns the UID (with whitespace stripped)' do
-          given    = '  John C. Smith '
-          expected = 'John C. Smith'
-          expect(described_class.normalize_dn(given)).to eq(expected)
-        end
-      end
-
       context 'when an equal sign is escaped' do
         it 'returns the DN completely unmodified' do
           given    = 'uid= foo\\=bar'
@@ -146,6 +278,28 @@ describe Gitlab::LDAP::Person do
     end
   end
 
+  describe '.is_dn?' do
+    context 'given a DN' do
+      context 'with a single RDN' do
+        it 'returns true' do
+          expect(described_class.is_dn?('uid=John Smith')).to be_truthy
+        end
+      end
+
+      context 'with multiple RDNs' do
+        it 'returns true' do
+          expect(described_class.is_dn?('uid=John Smith,ou=People,dc=example,dc=com')).to be_truthy
+        end
+      end
+    end
+
+    context 'given a UID' do
+      it 'returns false' do
+        expect(described_class.is_dn?('John Smith')).to be_falsey
+      end
+    end
+  end
+
   describe '#name' do
     it 'uses the configured name attribute and handles values as an array' do
       name = 'John Doe'