Merge branch 'set_ldap_config_attributes_defaults' into 'master'

LDAP attributes needs default values

See merge request !8465

5 files changed, 45 insertions, 12 deletions

diff --git a/lib/gitlab/ldap/auth_hash.rb b/lib/gitlab/ldap/auth_hash.rb
index bf4dd9542d5..95378e5a769 100644
--- a/lib/gitlab/ldap/auth_hash.rb
+++ b/lib/gitlab/ldap/auth_hash.rb
@@ -25,7 +25,7 @@ module Gitlab
       end
 
       def get_raw(key)
-        auth_hash.extra[:raw_info][key]
+        auth_hash.extra[:raw_info][key] if auth_hash.extra
       end
 
       def ldap_config
diff --git a/lib/gitlab/ldap/config.rb b/lib/gitlab/ldap/config.rb
index de52ef3fc65..28129198438 100644
--- a/lib/gitlab/ldap/config.rb
+++ b/lib/gitlab/ldap/config.rb
@@ -107,7 +107,7 @@ module Gitlab
       end
 
       def attributes
-        options['attributes']
+        default_attributes.merge(options['attributes'])
       end
 
       def timeout
@@ -130,6 +130,16 @@ module Gitlab
         end
       end
 
+      def default_attributes
+        {
+          'username'    => %w(uid userid sAMAccountName),
+          'email'       => %w(mail email userPrincipalName),
+          'name'        => 'cn',
+          'first_name'  => 'givenName',
+          'last_name'   => 'sn'
+        }
+      end
+
       protected
 
       def base_options
diff --git a/lib/gitlab/ldap/person.rb b/lib/gitlab/ldap/person.rb
index 333f170a484..7084fd1767d 100644
--- a/lib/gitlab/ldap/person.rb
+++ b/lib/gitlab/ldap/person.rb
@@ -28,7 +28,7 @@ module Gitlab
       end
 
       def name
-        attribute_value(:name)
+        attribute_value(:name).first
       end
 
       def uid
@@ -62,14 +62,12 @@ module Gitlab
       # this method looks for 'mail', 'email' and 'userPrincipalName' and
       # returns the first with a value.
       def attribute_value(attribute)
-        attributes = Array(config.attributes[attribute.to_sym])
+        attributes = Array(config.attributes[attribute.to_s])
         selected_attr = attributes.find { |attr| entry.respond_to?(attr) }
 
         return nil unless selected_attr
 
-        # Some LDAP attributes return an array,
-        # even if it is a single value (like 'cn')
-        Array(entry.public_send(selected_attr)).first
+        entry.public_send(selected_attr)
       end
     end
   end
diff --git a/spec/lib/gitlab/ldap/config_spec.rb b/spec/lib/gitlab/ldap/config_spec.rb
index 1a6803e01c3..cab2e9908ff 100644
--- a/spec/lib/gitlab/ldap/config_spec.rb
+++ b/spec/lib/gitlab/ldap/config_spec.rb
@@ -129,4 +129,27 @@ describe Gitlab::LDAP::Config, lib: true do
       expect(config.has_auth?).to be_falsey
     end
   end
+
+  describe '#attributes' do
+    it 'uses default attributes when no custom attributes are configured' do
+      expect(config.attributes).to eq(config.default_attributes)
+    end
+
+    it 'merges the configuration attributes with default attributes' do
+      stub_ldap_config(
+        options: {
+          'attributes' => {
+            'username' => %w(sAMAccountName),
+            'email'    => %w(userPrincipalName)
+          }
+        }
+      )
+
+      expect(config.attributes).to include({
+        'username' => %w(sAMAccountName),
+        'email'    => %w(userPrincipalName),
+        'name'     => 'cn'
+      })
+    end
+  end
 end
diff --git a/spec/lib/gitlab/ldap/person_spec.rb b/spec/lib/gitlab/ldap/person_spec.rb
index 60afe046788..9a556cde5d5 100644
--- a/spec/lib/gitlab/ldap/person_spec.rb
+++ b/spec/lib/gitlab/ldap/person_spec.rb
@@ -7,9 +7,11 @@ describe Gitlab::LDAP::Person do
 
   before do
     stub_ldap_config(
-      attributes: {
-        name: 'cn',
-        email: %w(mail email userPrincipalName)
+      options: {
+        'attributes' => {
+          'name' => 'cn',
+          'email' => %w(mail email userPrincipalName)
+        }
       }
     )
   end
@@ -30,7 +32,7 @@ describe Gitlab::LDAP::Person do
       entry['mail'] = mail
       person = Gitlab::LDAP::Person.new(entry, 'ldapmain')
 
-      expect(person.email).to eq(mail)
+      expect(person.email).to eq([mail])
     end
 
     it 'returns the value of userPrincipalName, if mail and email are not present' do
@@ -38,7 +40,7 @@ describe Gitlab::LDAP::Person do
       entry['userPrincipalName'] = user_principal_name
       person = Gitlab::LDAP::Person.new(entry, 'ldapmain')
 
-      expect(person.email).to eq(user_principal_name)
+      expect(person.email).to eq([user_principal_name])
     end
   end
 end