Merge branch 'fix-ldap-sync-name-bugs' into 'master'

Fix the bug of LDAP sync_name option

See merge request !112427



Merged-by: Matthias Käppler <mkaeppler@gitlab.com>
Approved-by: Matthias Käppler <mkaeppler@gitlab.com>
Approved-by: Félix Veillette-Potvin <fveillette@gitlab.com>
Approved-by: Drew Blessing <drew@gitlab.com>
Reviewed-by: Drew Blessing <drew@gitlab.com>
Reviewed-by: Song Huang <songhuang@gitlab.cn>
Co-authored-by: Zehua Zhang <zhzhang@jihulab.com>

app/models/user_synced_attributes_metadata.rb

@@ -14,7 +14,7 @@ def read_only?(attribute)
   def read_only_attributes
     return [] unless sync_profile_from_provider?
 
-    self.class.syncable_attributes.select { |key| synced?(key) }
+    SYNCABLE_ATTRIBUTES.select { |key| synced?(key) }
   end
 
   def synced?(attribute)
@@ -26,12 +26,11 @@ def set_attribute_synced(attribute, value)
   end
 
   class << self
-    def syncable_attributes
-      if Gitlab.config.ldap.enabled && !Gitlab.config.ldap.sync_name
-        SYNCABLE_ATTRIBUTES - %i[name]
-      else
-        SYNCABLE_ATTRIBUTES
-      end
+    def syncable_attributes(provider = nil)
+      return SYNCABLE_ATTRIBUTES unless provider && ldap_provider?(provider)
+      return SYNCABLE_ATTRIBUTES if ldap_sync_name?(provider)
+
+      SYNCABLE_ATTRIBUTES - %i[name]
     end
   end
 
@@ -40,4 +39,17 @@ def syncable_attributes
   def sync_profile_from_provider?
     Gitlab::Auth::OAuth::Provider.sync_profile_from_provider?(provider)
   end
+
+  class << self
+    def ldap_provider?(provider)
+      Gitlab::Auth::OAuth::Provider.ldap_provider?(provider)
+    end
+
+    def ldap_sync_name?(provider)
+      return false unless provider
+
+      config = Gitlab::Auth::Ldap::Config.new(provider)
+      config.enabled? && config.sync_name
+    end
+  end
 end

config/initializers/1_settings.rb

@@ -27,7 +27,6 @@
 
 # backwards compatibility, we only have one host
 if Settings.ldap['enabled'] || Rails.env.test?
-  Settings.ldap['sync_name'] = true if Settings.ldap['sync_name'].nil?
   if Settings.ldap['host'].present?
     # We detected old LDAP configuration syntax. Update the config to make it
     # look like it was entered with the new syntax.
@@ -68,6 +67,8 @@
     # `ssl_version` in favor of `tls_options` hash option.
     server['tls_options'] ||= {}
 
+    server['sync_name'] = true if server['sync_name'].nil?
+
     if server['ssl_version'] || server['ca_file']
       Gitlab::AppLogger.warn 'DEPRECATED: LDAP options `ssl_version` and `ca_file` should be nested within `tls_options`'
     end

lib/gitlab/auth/ldap/config.rb

@@ -184,6 +184,10 @@ def lowercase_usernames
           options['lowercase_usernames']
         end
 
+        def sync_name
+          options['sync_name']
+        end
+
         def name_proc
           if allow_username_or_email_login
             proc { |name| name.gsub(/@.*\z/, '') }

lib/gitlab/auth/o_auth/user.rb

@@ -258,7 +258,7 @@ def update_profile
           metadata = gl_user.build_user_synced_attributes_metadata
 
           if sync_profile_from_provider?
-            UserSyncedAttributesMetadata.syncable_attributes.each do |key|
+            UserSyncedAttributesMetadata.syncable_attributes(auth_hash.provider).each do |key|
               if auth_hash.has_attribute?(key) && gl_user.sync_attribute?(key)
                 gl_user.public_send("#{key}=".to_sym, auth_hash.public_send(key)) # rubocop:disable GitlabSecurity/PublicSend
                 metadata.set_attribute_synced(key, true)

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

@@ -320,6 +320,38 @@ def stub_omniauth_config(messages)
         end
 
         include_examples "to verify compliance with allow_single_sign_on"
+
+        context 'and other providers' do
+          context 'when sync_name is disabled' do
+            before do
+              stub_ldap_config(sync_name: false)
+            end
+
+            let!(:existing_user) { create(:omniauth_user, name: 'John Swift', email: 'john@example.com', extern_uid: dn, provider: 'twitter', username: 'john') }
+
+            it "updates the gl_user name" do
+              oauth_user.save # rubocop:disable Rails/SaveBang
+
+              expect(gl_user).to be_valid
+              expect(gl_user.name).to eql 'John'
+            end
+          end
+
+          context 'when sync_name is enabled' do
+            before do
+              stub_ldap_config(sync_name: true)
+            end
+
+            let!(:existing_user) { create(:omniauth_user, name: 'John Swift', email: 'john@example.com', extern_uid: dn, provider: 'twitter', username: 'john') }
+
+            it "updates the gl_user name" do
+              oauth_user.save # rubocop:disable Rails/SaveBang
+
+              expect(gl_user).to be_valid
+              expect(gl_user.name).to eql 'John'
+            end
+          end
+        end
       end
 
       context "with auto_link_ldap_user enabled" do
@@ -418,54 +450,41 @@ def stub_omniauth_config(messages)
             end
 
             context "and LDAP user has an account already" do
+              let(:provider) { 'ldapmain' }
+
+              before do
+                allow(Gitlab::Auth::Ldap::Person).to receive(:find_by_uid).and_return(ldap_user)
+                stub_omniauth_config(sync_profile_attributes: true)
+                allow(Gitlab.config.ldap).to receive(:enabled).and_return(true)
+              end
+
               context 'when sync_name is disabled' do
                 before do
-                  allow(Gitlab.config.ldap).to receive(:enabled).and_return(true)
-                  allow(Gitlab.config.ldap).to receive(:sync_name).and_return(false)
+                  stub_ldap_config(sync_name: false)
                 end
 
-                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)
+                let!(:existing_user) { create(:omniauth_user, name: 'John Deo', email: 'john@example.com', extern_uid: dn, provider: 'ldapmain', username: 'john') }
 
+                it "does not update the user name" do
                   oauth_user.save # rubocop:disable Rails/SaveBang
 
                   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 } }
-                  expect(identities_as_hash).to match_array(
-                    [
-                      { provider: 'ldapmain', extern_uid: dn },
-                      { provider: 'twitter', extern_uid: uid }
-                    ]
-                  )
+                  expect(gl_user.name).to eql 'John Deo'
                 end
               end
 
               context 'when sync_name is enabled' do
-                let!(:existing_user) { create(:omniauth_user, name: 'John Swift', email: 'john@example.com', extern_uid: dn, provider: 'ldapmain', username: 'john') }
+                before do
+                  stub_ldap_config(sync_name: true)
+                end
 
-                it "adds the omniauth identity to the LDAP account" do
-                  allow(Gitlab::Auth::Ldap::Person).to receive(:find_by_uid).and_return(ldap_user)
+                let!(:existing_user) { create(:omniauth_user, name: 'John Swift', email: 'john@example.com', extern_uid: dn, provider: 'ldapmain', username: 'john') }
 
+                it "updates the user name" do
                   oauth_user.save # rubocop:disable Rails/SaveBang
 
                   expect(gl_user).to be_valid
-                  expect(gl_user.username).to eql 'john'
-                  expect(gl_user.name).to eql 'John Swift'
-                  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 } }
-                  expect(identities_as_hash).to match_array(
-                    [
-                      { provider: 'ldapmain', extern_uid: dn },
-                      { provider: 'twitter', extern_uid: uid }
-                    ]
-                  )
+                  expect(gl_user.name).to eql 'John'
                 end
               end
             end