Merge branch 'dm-ldap-adapter-attributes-ee' into 'master'

Support simple string LDAP attribute specifications, and search for name rather…

See merge request !2746

changelogs/unreleased/dm-ldap-adapter-attributes.yml

+---
+title: Fix signing in using LDAP when attribute mapping uses simple strings instead
+  of arrays
+merge_request:
+author:
+type: fixed

ee/lib/ee/gitlab/ldap/adapter.rb

@@ -52,12 +52,6 @@ module EE
             LDAP::Group.new(entry, self)
           end
         end
-
-        def user_attributes
-          attributes = super
-          attributes << config.sync_ssh_keys if config.sync_ssh_keys
-          attributes
-        end
       end
     end
   end

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

@@ -42,6 +42,13 @@ module EE
               .reverse
               .join('.')
           end
+
+          def ldap_attributes(config)
+            super + [
+              'memberof', # Used in `memberof`
+              config.sync_ssh_keys # Used in `ssh_keys`
+            ]
+          end
         end
 
         def ssh_keys

lib/gitlab/ldap/adapter.rb

@@ -79,7 +79,7 @@ module Gitlab
       private
 
       def user_options(field, value, limit)
-        options = { attributes: user_attributes }
+        options = { attributes: Gitlab::LDAP::Person.ldap_attributes(config).compact.uniq }
         options[:size] = limit if limit
 
         if field.to_sym == :dn
@@ -105,10 +105,6 @@ module Gitlab
           filter
         end
       end
-
-      def user_attributes
-        %W(#{config.uid} cn dn memberof) + config.attributes['username'] + config.attributes['email']
-      end
     end
   end
 end

lib/gitlab/ldap/person.rb

@@ -3,7 +3,7 @@
 module Gitlab
   module LDAP
     class Person
-      include ::EE::Gitlab::LDAP::Person
+      prepend ::EE::Gitlab::LDAP::Person
 
       # Active Directory-specific LDAP filter that checks if bit 2 of the
       # userAccountControl attribute is set.
@@ -25,6 +25,15 @@ module Gitlab
         adapter.dn_matches_filter?(dn, AD_USER_DISABLED)
       end
 
+      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`
+        ]
+      end
+
       def initialize(entry, provider)
         Rails.logger.debug { "Instantiating #{self.class.name} with LDIF:\n#{entry.to_ldif}" }
         @entry = entry

spec/ee/spec/lib/ee/gitlab/ldap/adapter_spec.rb

@@ -39,11 +39,4 @@ describe Gitlab::LDAP::Adapter do
       expect(results.first.member_dns).to match_array(%w(john mary))
     end
   end
-
-  describe '#user_attributes' do
-    it 'appends EE-specific attributes' do
-      stub_ldap_config(uid: 'uid', sync_ssh_keys: 'sshPublicKey')
-      expect(adapter.user_attributes).to match_array(%w(uid dn cn email mail memberof sAMAccountName sshPublicKey uid userPrincipalName userid))
-    end
-  end
 end

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

@@ -9,6 +9,13 @@ describe Gitlab::LDAP::Person do
     expect(described_class).to include(EE::Gitlab::LDAP::Person)
   end
 
+  describe '.ldap_attributes' do
+    it 'appends EE-specific attributes' do
+      stub_ldap_config(sync_ssh_keys: 'sshPublicKey')
+      expect(described_class.ldap_attributes(ldap_adapter.config)).to include('sshPublicKey')
+    end
+  end
+
   describe '.find_by_email' do
     let(:adapter) { ldap_adapter }
 

spec/lib/gitlab/ldap/access_spec.rb

@@ -246,14 +246,14 @@ describe Gitlab::LDAP::Access do
     end
 
     it "adds a Kerberos identity if it is in Active Directory but not in GitLab" do
-      allow_any_instance_of(Gitlab::LDAP::Person).to receive_messages(kerberos_principal: "mylogin@FOO.COM")
+      allow_any_instance_of(EE::Gitlab::LDAP::Person).to receive_messages(kerberos_principal: "mylogin@FOO.COM")
 
       expect { access.update_kerberos_identity }.to change(user.identities.where(provider: :kerberos), :count).from(0).to(1)
       expect(user.identities.where(provider: "kerberos").last.extern_uid).to eq("mylogin@FOO.COM")
     end
 
     it "updates existing Kerberos identity in GitLab if Active Directory has a different one" do
-      allow_any_instance_of(Gitlab::LDAP::Person).to receive_messages(kerberos_principal: "otherlogin@BAR.COM")
+      allow_any_instance_of(EE::Gitlab::LDAP::Person).to receive_messages(kerberos_principal: "otherlogin@BAR.COM")
       user.identities.build(provider: "kerberos", extern_uid: "mylogin@FOO.COM").save
 
       expect { access.update_kerberos_identity }.not_to change(user.identities.where(provider: "kerberos"), :count)
@@ -261,7 +261,7 @@ describe Gitlab::LDAP::Access do
     end
 
     it "does not remove Kerberos identities from GitLab if they are none in the LDAP provider" do
-      allow_any_instance_of(Gitlab::LDAP::Person).to receive_messages(kerberos_principal: nil)
+      allow_any_instance_of(EE::Gitlab::LDAP::Person).to receive_messages(kerberos_principal: nil)
       user.identities.build(provider: "kerberos", extern_uid: "otherlogin@BAR.COM").save
 
       expect { access.update_kerberos_identity }.not_to change(user.identities.where(provider: "kerberos"), :count)
@@ -269,7 +269,7 @@ describe Gitlab::LDAP::Access do
     end
 
     it "does not modify identities in GitLab if they are no kerberos principal in the LDAP provider" do
-      allow_any_instance_of(Gitlab::LDAP::Person).to receive_messages(kerberos_principal: nil)
+      allow_any_instance_of(EE::Gitlab::LDAP::Person).to receive_messages(kerberos_principal: nil)
 
       expect { access.update_kerberos_identity }.not_to change(user.identities, :count)
     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{uid cn dn memberof uid userid sAMAccountName mail email userPrincipalName})
+        expect(arg[:attributes]).to match(%w{dn uid cn mail email userPrincipalName memberof})
       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{uid cn dn memberof uid userid sAMAccountName mail email userPrincipalName},
+        attributes: %w{dn uid cn mail email userPrincipalName memberof},
         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{sAMAccountName cn dn memberof uid userid sAMAccountName mail email userPrincipalName})
+        hash_including(attributes: %w{dn sAMAccountName cn mail email userPrincipalName memberof})
       ).and_return({})
 
       adapter.users('sAMAccountName', 'johndoe')