Merge branch '1974-update-user-name-upon-ldap-sync' into 'master'

Update user name upon LDAP sync

Closes #1974

See merge request gitlab-org/gitlab-ee!10316

doc/integration/omniauth.md

@@ -256,7 +256,7 @@ gitlab_rails['omniauth_enabled'] = false
 
 You can enable profile syncing from selected OmniAuth providers and for all or for specific user information.
 
-When authenticating using LDAP, the user's email is always synced.
+When authenticating using LDAP, the user's name and email are always synced.
 
 ```ruby
   gitlab_rails['sync_profile_from_provider'] = ['twitter', 'google_oauth2']

ee/changelogs/unreleased/1974-update-user-name-upon-ldap-sync.yml

+---
+title: Update user name upon LDAP sync
+merge_request: 10316
+author: '@icode1'
+type: added

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

@@ -12,7 +12,7 @@ module EE
           def update_user
             return if ::Gitlab::Database.read_only?
 
-            update_email
+            update_user_attributes
             update_memberships
             update_identity
             update_ssh_keys if sync_ssh_keys?
@@ -89,15 +89,18 @@ module EE
           end
           # rubocop: enable CodeReuse/ActiveRecord
 
-          # Update user email if it changed in LDAP
-          def update_email
-            return false unless ldap_user.try(:email)
+          # Update user attributes (name and email) if they changed in LDAP
+          def update_user_attributes
+            ldap_email = ldap_user.try(:email)&.last.to_s.downcase
+            ldap_name = ldap_user.try(:name)
 
-            ldap_email = ldap_user.email.last.to_s.downcase
+            return if ldap_email.blank? && ldap_name.blank?
 
-            return false if user.email == ldap_email
+            attrs = { user: user }
+            attrs[:email] = ldap_email if ldap_email.present?
+            attrs[:name] = ldap_name if ldap_name.present?
 
-            ::Users::UpdateService.new(user, user: user, email: ldap_email).execute do |user|
+            ::Users::UpdateService.new(user, attrs).execute do |user|
               user.skip_reconfirmation!
             end
           end

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

@@ -57,18 +57,60 @@ describe Gitlab::Auth::LDAP::Access do
         expect { access.update_user }.not_to change(user, :email)
       end
 
-      it 'does not update the email when in a read-only GitLab instance' do
-        allow(Gitlab::Database).to receive(:read_only?).and_return(true)
+      context 'when mail is different' do
+        before do
+          entry['mail'] = ['new_email@example.com']
+        end
+
+        it 'does not update the email when in a read-only GitLab instance' do
+          allow(Gitlab::Database).to receive(:read_only?).and_return(true)
 
-        entry['mail'] = ['new_email@example.com']
+          expect { access.update_user }.not_to change(user, :email)
+        end
 
-        expect { access.update_user }.not_to change(user, :email)
+        it 'updates the email if the user email is different' do
+          expect { access.update_user }.to change(user, :email)
+        end
+
+        it 'does not update the name if the user email is different' do
+          expect { access.update_user }.not_to change(user, :name)
+        end
       end
+    end
 
-      it 'updates the email if the user email is different' do
-        entry['mail'] = ['new_email@example.com']
+    context 'name' do
+      before do
+        stub_ldap_person_find_by_dn(entry, provider)
+      end
 
-        expect { access.update_user }.to change(user, :email)
+      it 'does not update name if name attribute is not set' do
+        expect { access.update_user }.not_to change(user, :name)
+      end
+
+      it 'does not update the name if the user has the same name in GitLab and in LDAP' do
+        entry['cn'] = [user.name]
+
+        expect { access.update_user }.not_to change(user, :name)
+      end
+
+      context 'when cn is different' do
+        before do
+          entry['cn'] = ['New Name']
+        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)
+        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
 

lib/gitlab/auth/o_auth/user.rb

@@ -201,22 +201,19 @@ module Gitlab
           # Give preference to LDAP for sensitive information when creating a linked account
           if creating_linked_ldap_user?
             username = ldap_person.username.presence
+            name = ldap_person.name.presence
             email = ldap_person.email.first.presence
           end
 
           username ||= auth_hash.username
+          name ||= auth_hash.name
           email ||= auth_hash.email
 
           valid_username = ::Namespace.clean_path(username)
-
-          uniquify = Uniquify.new
-          valid_username = uniquify.string(valid_username) { |s| !NamespacePathValidator.valid_path?(s) }
-
-          name = auth_hash.name
-          name = valid_username if name.strip.empty?
+          valid_username = Uniquify.new.string(valid_username) { |s| !NamespacePathValidator.valid_path?(s) }
 
           {
-            name:                       name,
+            name:                       name.strip.presence || valid_username,
             username:                   valid_username,
             email:                      email,
             password:                   auth_hash.password,
@@ -249,8 +246,9 @@ module Gitlab
             metadata.provider = auth_hash.provider
           end
 
-          if creating_linked_ldap_user? && gl_user.email == ldap_person.email.first
-            metadata.set_attribute_synced(:email, true)
+          if creating_linked_ldap_user?
+            metadata.set_attribute_synced(:name, true) if gl_user.name == ldap_person.name
+            metadata.set_attribute_synced(:email, true) if gl_user.email == ldap_person.email.first
             metadata.provider = ldap_person.provider
           end
         end

spec/lib/gitlab/auth/o_auth/user_spec.rb

@@ -207,6 +207,7 @@ describe Gitlab::Auth::OAuth::User do
             before do
               allow(ldap_user).to receive(:uid) { uid }
               allow(ldap_user).to receive(:username) { uid }
+              allow(ldap_user).to receive(:name) { 'John Doe' }
               allow(ldap_user).to receive(:email) { ['johndoe@example.com', 'john2@example.com'] }
               allow(ldap_user).to receive(:dn) { dn }
             end
@@ -221,6 +222,7 @@ describe Gitlab::Auth::OAuth::User do
               it "creates a user with dual LDAP and omniauth identities" do
                 expect(gl_user).to be_valid
                 expect(gl_user.username).to eql uid
+                expect(gl_user.name).to eql 'John Doe'
                 expect(gl_user.email).to eql 'johndoe@example.com'
                 expect(gl_user.identities.length).to be 2
                 identities_as_hash = gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } }
@@ -232,11 +234,13 @@ describe Gitlab::Auth::OAuth::User do
                 )
               end
 
-              it "has email set as synced" do
+              it "has name and email set as synced" do
+                expect(gl_user.user_synced_attributes_metadata.name_synced).to be_truthy
                 expect(gl_user.user_synced_attributes_metadata.email_synced).to be_truthy
               end
 
-              it "has email set as read-only" do
+              it "has name and email set as read-only" do
+                expect(gl_user.read_only_attribute?(:name)).to be_truthy
                 expect(gl_user.read_only_attribute?(:email)).to be_truthy
               end
 
@@ -246,7 +250,7 @@ describe Gitlab::Auth::OAuth::User do
             end
 
             context "and LDAP user has an account already" do
-              let!(:existing_user) { create(:omniauth_user, email: 'john@example.com', extern_uid: dn, provider: 'ldapmain', username: 'john') }
+              let!(:existing_user) { create(:omniauth_user, name: 'John Doe', email: 'john@example.com', extern_uid: dn, provider: 'ldapmain', username: 'john') }
               it "adds the omniauth identity to the LDAP account" do
                 allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_uid).and_return(ldap_user)
 
@@ -254,6 +258,7 @@ describe Gitlab::Auth::OAuth::User do
 
                 expect(gl_user).to be_valid
                 expect(gl_user.username).to eql 'john'
+                expect(gl_user.name).to eql 'John Doe'
                 expect(gl_user.email).to eql 'john@example.com'
                 expect(gl_user.identities.length).to be 2
                 identities_as_hash = gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } }