Merge branch '11805-support-first-name-and-last-name-attributes-in-ldap-user-sync' into 'master'

Support first_name and last_name attributes in LDAP user sync

Closes #11805

See merge request gitlab-org/gitlab!29542

changelogs/unreleased/11805-support-first-name-and-last-name-attributes-in-ldap-user-sync.yml

+---
+title: Support first_name and last_name attributes in LDAP user sync
+merge_request: 29542
+author:
+type: added

ee/lib/ee/gitlab/auth/ldap/person.rb

@@ -8,6 +8,7 @@ module EE
       module Ldap
         module Person
           extend ActiveSupport::Concern
+          extend ::Gitlab::Utils::Override
 
           class_methods do
             def find_by_email(email, adapter)
@@ -54,9 +55,11 @@ module EE
             def ldap_attributes(config)
               attributes = super + [
                 'memberof',
-                (config.sync_ssh_keys if config.sync_ssh_keys.is_a?(String))
+                (config.sync_ssh_keys if config.sync_ssh_keys.is_a?(String)),
+                *config.attributes['first_name'],
+                *config.attributes['last_name']
               ]
-              attributes.compact.uniq
+              attributes.compact.uniq.reject(&:blank?)
             end
           end
 
@@ -93,6 +96,18 @@ module EE
             # the group name
             memberof.match(/(?:cn=([\w\s-]+))/i)&.captures&.first
           end
+
+          override :name
+          def name
+            name = super
+            return name if name.present?
+
+            first_name = attribute_value(:first_name)&.first
+            last_name = attribute_value(:last_name)&.first
+            return unless first_name.present? || last_name.present?
+
+            "#{first_name} #{last_name}".strip
+          end
         end
       end
     end

ee/spec/lib/gitlab/auth/ldap/access_spec.rb

@@ -194,6 +194,27 @@ RSpec.describe Gitlab::Auth::Ldap::Access do
           expect { access.update_user }.not_to change(user, :email)
         end
       end
+
+      context 'when first and last name attributes passed' do
+        before do
+          entry['givenName'] = ['Jane']
+          entry['sn'] = ['Doe']
+        end
+
+        it 'does not update the name when in a read-only GitLab instance' do
+          allow(Gitlab::Database).to receive(:read_only?).and_return(true)
+
+          expect { access.update_user }.not_to change(user, :name)
+        end
+
+        it 'updates the name if the user name is different' do
+          expect { access.update_user }.to change(user, :name).to('Jane Doe')
+        end
+
+        it 'does not update the email if the user name is different' do
+          expect { access.update_user }.not_to change(user, :email)
+        end
+      end
     end
 
     context 'group memberships' do

ee/spec/lib/gitlab/auth/ldap/person_spec.rb

@@ -16,6 +16,13 @@ RSpec.describe Gitlab::Auth::Ldap::Person do
       stub_ldap_config(sync_ssh_keys: 'sshPublicKey')
       expect(described_class.ldap_attributes(ldap_adapter.config)).to include('sshPublicKey')
     end
+
+    it 'appends first and last name attributes' do
+      stub_ldap_config(options: { 'attributes' => { 'first_name' => 'name', 'last_name' => 'surname' } })
+
+      expect(described_class.ldap_attributes(ldap_adapter.config)).to include('name')
+      expect(described_class.ldap_attributes(ldap_adapter.config)).to include('surname')
+    end
   end
 
   describe '.find_by_email' do
@@ -91,7 +98,9 @@ RSpec.describe Gitlab::Auth::Ldap::Person do
           'attributes'   => {
             'name'       => 'cn',
             'email'      => 'mail',
-            'username'   => %w(uid mail memberof)
+            'username'   => %w(uid mail),
+            'first_name' => 'name',
+            'last_name'  => 'surname'
           },
           'sync_ssh_keys' => value
         }
@@ -100,7 +109,7 @@ RSpec.describe Gitlab::Auth::Ldap::Person do
 
     let(:config) { Gitlab::Auth::Ldap::Config.new('ldapmain') }
     let(:ldap_attributes) { described_class.ldap_attributes(config) }
-    let(:expected_attributes) { %w(dn cn uid mail memberof) }
+    let(:expected_attributes) { %w(dn cn uid mail memberof name surname) }
 
     it 'includes a real attribute name' do
       stub_sync_ssh_keys('my-ssh-attribute')

lib/gitlab/auth/ldap/person.rb

@@ -39,7 +39,7 @@ module Gitlab
             *config.attributes['name'],
             *config.attributes['email'],
             *config.attributes['username']
-          ].compact.uniq
+          ].compact.uniq.reject(&:blank?)
         end
 
         def self.normalize_dn(dn)

spec/lib/gitlab/auth/ldap/person_spec.rb

@@ -57,14 +57,17 @@ describe Gitlab::Auth::Ldap::Person do
           'attributes' => {
             'name'     => 'cn',
             'email'    => 'mail',
-            'username' => %w(uid mail memberof)
+            'username' => %w(uid mail),
+            'first_name' => ''
           }
         }
       )
       config = Gitlab::Auth::Ldap::Config.new('ldapmain')
       ldap_attributes = described_class.ldap_attributes(config)
 
-      expect(ldap_attributes).to match_array(%w(dn uid cn mail memberof))
+      expect(ldap_attributes).to include('dn', 'uid', 'cn', 'mail')
+      expect(ldap_attributes).to be_present
+      expect(ldap_attributes.uniq!).to eq(nil)
     end
   end