From de4eee08947fde4b6b1b21e54329dc8f1d958362 Mon Sep 17 00:00:00 2001
From: Drew Blessing <drew@gitlab.com>
Date: Thu, 5 Jan 2017 16:01:04 -0600
Subject: [PATCH] LDAP attributes needs default values

---
 lib/gitlab/ldap/auth_hash.rb        |  2 +-
 lib/gitlab/ldap/config.rb           | 12 +++++++++++-
 lib/gitlab/ldap/person.rb           |  8 +++-----
 spec/lib/gitlab/ldap/config_spec.rb | 23 +++++++++++++++++++++++
 spec/lib/gitlab/ldap/person_spec.rb | 12 +++++++-----
 5 files changed, 45 insertions(+), 12 deletions(-)

diff --git a/lib/gitlab/ldap/auth_hash.rb b/lib/gitlab/ldap/auth_hash.rb
index bf4dd9542d5d..95378e5a7693 100644
--- a/lib/gitlab/ldap/auth_hash.rb
+++ b/lib/gitlab/ldap/auth_hash.rb
@@ -25,7 +25,7 @@ def get_info(key)
       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 de52ef3fc657..281291984381 100644
--- a/lib/gitlab/ldap/config.rb
+++ b/lib/gitlab/ldap/config.rb
@@ -107,7 +107,7 @@ def block_auto_created_users
       end
 
       def attributes
-        options['attributes']
+        default_attributes.merge(options['attributes'])
       end
 
       def timeout
@@ -130,6 +130,16 @@ def name_proc
         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 333f170a484a..7084fd1767dc 100644
--- a/lib/gitlab/ldap/person.rb
+++ b/lib/gitlab/ldap/person.rb
@@ -28,7 +28,7 @@ def initialize(entry, provider)
       end
 
       def name
-        attribute_value(:name)
+        attribute_value(:name).first
       end
 
       def uid
@@ -62,14 +62,12 @@ def config
       # 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 1a6803e01c32..cab2e9908fff 100644
--- a/spec/lib/gitlab/ldap/config_spec.rb
+++ b/spec/lib/gitlab/ldap/config_spec.rb
@@ -129,4 +129,27 @@
       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 60afe0467888..9a556cde5d56 100644
--- a/spec/lib/gitlab/ldap/person_spec.rb
+++ b/spec/lib/gitlab/ldap/person_spec.rb
@@ -7,9 +7,11 @@
 
   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 @@
       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 @@
       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
-- 
GitLab