Modify `LDAP::Person` to return username value based on attributes

changelogs/unreleased/ldap_username_attributes.yml

+---
+title: Modify `LDAP::Person` to return username value based on attributes
+merge_request:
+author:
+type: fixed

lib/gitlab/ldap/adapter.rb

@@ -74,7 +74,7 @@ module Gitlab
 
       def user_options(fields, value, limit)
         options = {
-          attributes: Gitlab::LDAP::Person.ldap_attributes(config).compact.uniq,
+          attributes: Gitlab::LDAP::Person.ldap_attributes(config),
           base: config.base
         }
 

lib/gitlab/ldap/config.rb

@@ -148,7 +148,7 @@ module Gitlab
 
       def default_attributes
         {
-          'username'    => %w(uid userid sAMAccountName),
+          'username'    => %w(uid sAMAccountName userid),
           'email'       => %w(mail email userPrincipalName),
           'name'        => 'cn',
           'first_name'  => 'givenName',

lib/gitlab/ldap/person.rb

@@ -6,6 +6,8 @@ module Gitlab
       # Source: http://ctogonewild.com/2009/09/03/bitmask-searches-in-ldap/
       AD_USER_DISABLED = Net::LDAP::Filter.ex("userAccountControl:1.2.840.113556.1.4.803", "2")
 
+      InvalidEntryError = Class.new(StandardError)
+
       attr_accessor :entry, :provider
 
       def self.find_by_uid(uid, adapter)
@@ -29,11 +31,12 @@ module Gitlab
 
       def self.ldap_attributes(config)
         [
-          'dn', # Used in `dn`
-          config.uid, # Used in `uid`
-          *config.attributes['name'], # Used in `name`
-          *config.attributes['email'] # Used in `email`
-        ]
+          'dn',
+          config.uid,
+          *config.attributes['name'],
+          *config.attributes['email'],
+          *config.attributes['username']
+        ].compact.uniq
       end
 
       def self.normalize_dn(dn)
@@ -60,6 +63,8 @@ module Gitlab
         Rails.logger.debug { "Instantiating #{self.class.name} with LDIF:\n#{entry.to_ldif}" }
         @entry = entry
         @provider = provider
+
+        validate_entry
       end
 
       def name
@@ -71,7 +76,13 @@ module Gitlab
       end
 
       def username
-        uid
+        username = attribute_value(:username)
+
+        # Depending on the attribute, multiple values may
+        # be returned. We need only one for username.
+        # Ex. `uid` returns only one value but `mail` may
+        # return an array of multiple email addresses.
+        [username].flatten.first
       end
 
       def email
@@ -104,6 +115,19 @@ module Gitlab
 
         entry.public_send(selected_attr) # rubocop:disable GitlabSecurity/PublicSend
       end
+
+      def validate_entry
+        allowed_attrs = self.class.ldap_attributes(config).map(&:downcase)
+
+        # Net::LDAP::Entry transforms keys to symbols. Change to strings to compare.
+        entry_attrs = entry.attribute_names.map { |n| n.to_s.downcase }
+        invalid_attrs = entry_attrs - allowed_attrs
+
+        if invalid_attrs.any?
+          raise InvalidEntryError,
+                "#{self.class.name} initialized with Net::LDAP::Entry containing invalid attributes(s): #{invalid_attrs}"
+        end
+      end
     end
   end
 end

spec/lib/gitlab/ldap/adapter_spec.rb

@@ -16,7 +16,7 @@ describe Gitlab::LDAP::Adapter do
       expect(adapter).to receive(:ldap_search) do |arg|
         expect(arg[:filter].to_s).to eq('(uid=johndoe)')
         expect(arg[:base]).to eq('dc=example,dc=com')
-        expect(arg[:attributes]).to match(%w{dn uid cn mail email userPrincipalName})
+        expect(arg[:attributes]).to match(ldap_attributes)
       end.and_return({})
 
       adapter.users('uid', 'johndoe')
@@ -26,7 +26,7 @@ describe Gitlab::LDAP::Adapter do
       expect(adapter).to receive(:ldap_search).with(
         base: 'uid=johndoe,ou=users,dc=example,dc=com',
         scope: Net::LDAP::SearchScope_BaseObject,
-        attributes: %w{dn uid cn mail email userPrincipalName},
+        attributes: ldap_attributes,
         filter: nil
       ).and_return({})
 
@@ -63,7 +63,7 @@ describe Gitlab::LDAP::Adapter do
     it 'uses the right uid attribute when non-default' do
       stub_ldap_config(uid: 'sAMAccountName')
       expect(adapter).to receive(:ldap_search).with(
-        hash_including(attributes: %w{dn sAMAccountName cn mail email userPrincipalName})
+        hash_including(attributes: ldap_attributes)
       ).and_return({})
 
       adapter.users('sAMAccountName', 'johndoe')
@@ -137,4 +137,8 @@ describe Gitlab::LDAP::Adapter do
       end
     end
   end
+
+  def ldap_attributes
+    Gitlab::LDAP::Person.ldap_attributes(Gitlab::LDAP::Config.new('ldapmain'))
+  end
 end

spec/lib/gitlab/ldap/person_spec.rb

@@ -8,13 +8,16 @@ describe Gitlab::LDAP::Person do
   before do
     stub_ldap_config(
       options: {
+        'uid'        => 'uid',
         'attributes' => {
-          'name' => 'cn',
-          'email' => %w(mail email userPrincipalName)
+          'name'     => 'cn',
+          'email'    => %w(mail email userPrincipalName),
+          'username' => username_attribute
         }
       }
     )
   end
+  let(:username_attribute) { %w(uid sAMAccountName userid) }
 
   describe '.normalize_dn' do
     subject { described_class.normalize_dn(given) }
@@ -44,6 +47,34 @@ describe Gitlab::LDAP::Person do
     end
   end
 
+  describe '.ldap_attributes' do
+    it 'returns a compact and unique array' do
+      stub_ldap_config(
+        options: {
+          'uid'        => nil,
+          'attributes' => {
+            'name'     => 'cn',
+            'email'    => 'mail',
+            'username' => %w(uid mail memberof)
+          }
+        }
+      )
+      config = Gitlab::LDAP::Config.new('ldapmain')
+      ldap_attributes = described_class.ldap_attributes(config)
+
+      expect(ldap_attributes).to match_array(%w(dn uid cn mail memberof))
+    end
+  end
+
+  describe '.validate_entry' do
+    it 'raises InvalidEntryError' do
+      entry['foo'] = 'bar'
+
+      expect { described_class.new(entry, 'ldapmain') }
+        .to raise_error(Gitlab::LDAP::Person::InvalidEntryError)
+    end
+  end
+
   describe '#name' do
     it 'uses the configured name attribute and handles values as an array' do
       name = 'John Doe'
@@ -72,6 +103,44 @@ describe Gitlab::LDAP::Person do
     end
   end
 
+  describe '#username' do
+    context 'with default uid username attribute' do
+      let(:username_attribute) { 'uid' }
+
+      it 'returns the proper username value' do
+        attr_value = 'johndoe'
+        entry[username_attribute] = attr_value
+        person = described_class.new(entry, 'ldapmain')
+
+        expect(person.username).to eq(attr_value)
+      end
+    end
+
+    context 'with a different username attribute' do
+      let(:username_attribute) { 'sAMAccountName' }
+
+      it 'returns the proper username value' do
+        attr_value = 'johndoe'
+        entry[username_attribute] = attr_value
+        person = described_class.new(entry, 'ldapmain')
+
+        expect(person.username).to eq(attr_value)
+      end
+    end
+
+    context 'with a non-standard username attribute' do
+      let(:username_attribute) { 'mail' }
+
+      it 'returns the proper username value' do
+        attr_value = 'john.doe@example.com'
+        entry[username_attribute] = attr_value
+        person = described_class.new(entry, 'ldapmain')
+
+        expect(person.username).to eq(attr_value)
+      end
+    end
+  end
+
   def assert_generic_test(test_description, got, expected)
     test_failure_message = "Failed test description: '#{test_description}'\n\n    expected: #{expected}\n         got: #{got}"
     expect(got).to eq(expected), test_failure_message

spec/lib/gitlab/o_auth/user_spec.rb

@@ -275,6 +275,26 @@ describe Gitlab::OAuth::User do
             end
           end
 
+          context 'and a corresponding LDAP person with a non-default username' do
+            before do
+              allow(ldap_user).to receive(:uid) { uid }
+              allow(ldap_user).to receive(:username) { 'johndoe@example.com' }
+              allow(ldap_user).to receive(:email) { %w(johndoe@example.com john2@example.com) }
+              allow(ldap_user).to receive(:dn) { dn }
+            end
+
+            context 'and no account for the LDAP user' do
+              it 'creates a user favoring the LDAP username and strips email domain' do
+                allow(Gitlab::LDAP::Person).to receive(:find_by_uid).and_return(ldap_user)
+
+                oauth_user.save
+
+                expect(gl_user).to be_valid
+                expect(gl_user.username).to eql 'johndoe'
+              end
+            end
+          end
+
           context "and no corresponding LDAP person" do
             before do
               allow(Gitlab::LDAP::Person).to receive(:find_by_uid).and_return(nil)