From 55b07c7076d3296116714470ea53d75f69cb4387 Mon Sep 17 00:00:00 2001
From: Daniel Fanara <fanardat16@gmail.com>
Date: Fri, 8 Mar 2019 23:55:11 -0500
Subject: [PATCH 1/4] Issue #1306 - Specify display name attribute from LDAP

---
 app/Auth/Access/LdapService.php | 6 ++++--
 config/services.php             | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/app/Auth/Access/LdapService.php b/app/Auth/Access/LdapService.php
index 654ea2f99..c8548b98a 100644
--- a/app/Auth/Access/LdapService.php
+++ b/app/Auth/Access/LdapService.php
@@ -80,7 +80,9 @@ class LdapService
     public function getUserDetails($userName)
     {
         $emailAttr = $this->config['email_attribute'];
-        $user = $this->getUserWithAttributes($userName, ['cn', 'uid', 'dn', $emailAttr]);
+        $displayNameAttr = $this->config['display_name_attribute'];
+
+        $user = $this->getUserWithAttributes($userName, ['cn', 'uid', 'dn', $emailAttr, $displayNameAttr]);
 
         if ($user === null) {
             return null;
@@ -88,7 +90,7 @@ class LdapService
 
         return [
             'uid'   => (isset($user['uid'])) ? $user['uid'][0] : $user['dn'],
-            'name'  => $user['cn'][0],
+            'name'  => (isset($uset[$displayNameAttr])) ? (is_array($user[$displayNameAttr]) ? $user[$displayNameAttr][0] : $user[$displayNameAttr]) : $user['cn'][0],
             'dn'    => $user['dn'],
             'email' => (isset($user[$emailAttr])) ? (is_array($user[$emailAttr]) ? $user[$emailAttr][0] : $user[$emailAttr]) : null
         ];
diff --git a/config/services.php b/config/services.php
index f713f9d38..97cb71ddc 100644
--- a/config/services.php
+++ b/config/services.php
@@ -141,6 +141,7 @@ return [
         'user_filter' => env('LDAP_USER_FILTER', '(&(uid=${user}))'),
         'version' => env('LDAP_VERSION', false),
         'email_attribute' => env('LDAP_EMAIL_ATTRIBUTE', 'mail'),
+        'display_name_attribute' => env('LDAP_DISPLAY_NAME_ATTRIBUTE', 'cn'),
         'follow_referrals' => env('LDAP_FOLLOW_REFERRALS', false),
 		'user_to_groups' => env('LDAP_USER_TO_GROUPS',false),
 		'group_attribute' => env('LDAP_GROUP_ATTRIBUTE', 'memberOf'),

From 502ea608bf2387a0bc928eb1cb521809920309c2 Mon Sep 17 00:00:00 2001
From: Daniel Fanara <fanardat16@gmail.com>
Date: Sat, 9 Mar 2019 01:08:49 -0500
Subject: [PATCH 2/4] Issue #1306 - Unit Tests for LdapService Changes

---
 .env.example.complete           |  1 +
 app/Auth/Access/LdapService.php |  2 +-
 tests/Auth/LdapTest.php         | 76 +++++++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+), 1 deletion(-)

diff --git a/.env.example.complete b/.env.example.complete
index 77a0508dc..911d924df 100644
--- a/.env.example.complete
+++ b/.env.example.complete
@@ -177,6 +177,7 @@ LDAP_USER_FILTER=false
 LDAP_VERSION=false
 LDAP_TLS_INSECURE=false
 LDAP_EMAIL_ATTRIBUTE=mail
+LDAP_DISPLAY_NAME_ATTRIBUTE=cn
 LDAP_FOLLOW_REFERRALS=true
 
 # LDAP group sync configuration
diff --git a/app/Auth/Access/LdapService.php b/app/Auth/Access/LdapService.php
index c8548b98a..c48a72f98 100644
--- a/app/Auth/Access/LdapService.php
+++ b/app/Auth/Access/LdapService.php
@@ -90,7 +90,7 @@ class LdapService
 
         return [
             'uid'   => (isset($user['uid'])) ? $user['uid'][0] : $user['dn'],
-            'name'  => (isset($uset[$displayNameAttr])) ? (is_array($user[$displayNameAttr]) ? $user[$displayNameAttr][0] : $user[$displayNameAttr]) : $user['cn'][0],
+            'name'  => (isset($user[$displayNameAttr])) ? (is_array($user[$displayNameAttr]) ? $user[$displayNameAttr][0] : $user[$displayNameAttr]) : $user['cn'][0],
             'dn'    => $user['dn'],
             'email' => (isset($user[$emailAttr])) ? (is_array($user[$emailAttr]) ? $user[$emailAttr][0] : $user[$emailAttr]) : null
         ];
diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php
index 16ba11358..d23c9ec84 100644
--- a/tests/Auth/LdapTest.php
+++ b/tests/Auth/LdapTest.php
@@ -23,6 +23,7 @@ class LdapTest extends BrowserKitTest
             'auth.method' => 'ldap',
             'services.ldap.base_dn' => 'dc=ldap,dc=local',
             'services.ldap.email_attribute' => 'mail',
+            'services.ldap.display_name_attribute' => 'cn',
             'services.ldap.user_to_groups' => false,
             'auth.providers.users.driver' => 'ldap',
         ]);
@@ -372,4 +373,79 @@ class LdapTest extends BrowserKitTest
         ]);
     }
 
+    public function test_login_uses_specified_display_name_attribute()
+    {
+        app('config')->set([
+            'services.ldap.display_name_attribute' => 'displayName'
+        ]);
+
+        $this->mockLdap->shouldReceive('connect')->once()->andReturn($this->resourceId);
+        $this->mockLdap->shouldReceive('setVersion')->once();
+        $this->mockLdap->shouldReceive('setOption')->times(4);
+        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(4)
+            ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
+            ->andReturn(['count' => 1, 0 => [
+                'uid' => [$this->mockUser->name],
+                'cn' => [$this->mockUser->name],
+                'dn' => ['dc=test' . config('services.ldap.base_dn')],
+                'displayName' => 'displayNameAttribute'
+            ]]);
+        $this->mockLdap->shouldReceive('bind')->times(6)->andReturn(true);
+        $this->mockEscapes(4);
+
+        $this->visit('/login')
+            ->see('Username')
+            ->type($this->mockUser->name, '#username')
+            ->type($this->mockUser->password, '#password')
+            ->press('Log In')
+            ->seePageIs('/login')->see('Please enter an email to use for this account.');
+
+        $this->type($this->mockUser->email, '#email')
+            ->press('Log In')
+            ->seePageIs('/')
+            ->see('displayNameAttribute')
+            ->seeInDatabase('users', ['email' => $this->mockUser->email, 'email_confirmed' => false, 'external_auth_id' => $this->mockUser->name, 'name' => 'displayNameAttribute']);
+        
+        app('config')->set([
+            'services.ldap.display_name_attribute' => 'cn'
+        ]);
+    }
+
+    public function test_login_uses_default_display_name_attribute_if_specified_not_present()
+    {
+        app('config')->set([
+            'services.ldap.display_name_attribute' => 'displayName'
+        ]);
+
+        $this->mockLdap->shouldReceive('connect')->once()->andReturn($this->resourceId);
+        $this->mockLdap->shouldReceive('setVersion')->once();
+        $this->mockLdap->shouldReceive('setOption')->times(4);
+        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(4)
+            ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
+            ->andReturn(['count' => 1, 0 => [
+                'uid' => [$this->mockUser->name],
+                'cn' => [$this->mockUser->name],
+                'dn' => ['dc=test' . config('services.ldap.base_dn')]
+            ]]);
+        $this->mockLdap->shouldReceive('bind')->times(6)->andReturn(true);
+        $this->mockEscapes(4);
+
+        $this->visit('/login')
+            ->see('Username')
+            ->type($this->mockUser->name, '#username')
+            ->type($this->mockUser->password, '#password')
+            ->press('Log In')
+            ->seePageIs('/login')->see('Please enter an email to use for this account.');
+
+        $this->type($this->mockUser->email, '#email')
+            ->press('Log In')
+            ->seePageIs('/')
+            ->see($this->mockUser->name)
+            ->seeInDatabase('users', ['email' => $this->mockUser->email, 'email_confirmed' => false, 'external_auth_id' => $this->mockUser->name, 'name' => $this->mockUser->name]);
+    
+        app('config')->set([
+            'services.ldap.display_name_attribute' => 'cn'
+        ]);
+    }
+
 }

From 6d20bdc1fbb4da20e7f5c40c0beafcafc296a18d Mon Sep 17 00:00:00 2001
From: Daniel Fanara <fanardat16@gmail.com>
Date: Sat, 9 Mar 2019 01:13:30 -0500
Subject: [PATCH 3/4] Preserve original display_name_attribute configuration
 values.

---
 tests/Auth/LdapTest.php | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php
index d23c9ec84..54cf84266 100644
--- a/tests/Auth/LdapTest.php
+++ b/tests/Auth/LdapTest.php
@@ -375,6 +375,7 @@ class LdapTest extends BrowserKitTest
 
     public function test_login_uses_specified_display_name_attribute()
     {
+        $originalAttribute = config('services.ldap.display_name_attribute');
         app('config')->set([
             'services.ldap.display_name_attribute' => 'displayName'
         ]);
@@ -407,12 +408,13 @@ class LdapTest extends BrowserKitTest
             ->seeInDatabase('users', ['email' => $this->mockUser->email, 'email_confirmed' => false, 'external_auth_id' => $this->mockUser->name, 'name' => 'displayNameAttribute']);
         
         app('config')->set([
-            'services.ldap.display_name_attribute' => 'cn'
+            'services.ldap.display_name_attribute' => $originalAttribute
         ]);
     }
 
     public function test_login_uses_default_display_name_attribute_if_specified_not_present()
     {
+        $originalAttribute = config('services.ldap.display_name_attribute');
         app('config')->set([
             'services.ldap.display_name_attribute' => 'displayName'
         ]);
@@ -444,7 +446,7 @@ class LdapTest extends BrowserKitTest
             ->seeInDatabase('users', ['email' => $this->mockUser->email, 'email_confirmed' => false, 'external_auth_id' => $this->mockUser->name, 'name' => $this->mockUser->name]);
     
         app('config')->set([
-            'services.ldap.display_name_attribute' => 'cn'
+            'services.ldap.display_name_attribute' => $originalAttribute
         ]);
     }
 

From 44c537de1a37aeef8e55ac3931875dee48bc512e Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Sun, 10 Mar 2019 10:54:19 +0000
Subject: [PATCH 4/4] Performed some LDAP service/test cleanup

---
 app/Auth/Access/LdapService.php | 24 ++++++++--
 tests/Auth/LdapTest.php         | 77 ++++++++-------------------------
 2 files changed, 39 insertions(+), 62 deletions(-)

diff --git a/app/Auth/Access/LdapService.php b/app/Auth/Access/LdapService.php
index c48a72f98..9ffbbfbb7 100644
--- a/app/Auth/Access/LdapService.php
+++ b/app/Auth/Access/LdapService.php
@@ -88,14 +88,32 @@ class LdapService
             return null;
         }
 
+        $userCn = $this->getUserResponseProperty($user, 'cn', null);
         return [
-            'uid'   => (isset($user['uid'])) ? $user['uid'][0] : $user['dn'],
-            'name'  => (isset($user[$displayNameAttr])) ? (is_array($user[$displayNameAttr]) ? $user[$displayNameAttr][0] : $user[$displayNameAttr]) : $user['cn'][0],
+            'uid'   => $this->getUserResponseProperty($user, 'uid', $user['dn']),
+            'name' => $this->getUserResponseProperty($user, $displayNameAttr, $userCn),
             'dn'    => $user['dn'],
-            'email' => (isset($user[$emailAttr])) ? (is_array($user[$emailAttr]) ? $user[$emailAttr][0] : $user[$emailAttr]) : null
+            'email' => $this->getUserResponseProperty($user, $emailAttr, null),
         ];
     }
 
+    /**
+     * Get a property from an LDAP user response fetch.
+     * Handles properties potentially being part of an array.
+     * @param array $userDetails
+     * @param string $propertyKey
+     * @param $defaultValue
+     * @return mixed
+     */
+    protected function getUserResponseProperty(array $userDetails, string $propertyKey, $defaultValue)
+    {
+        if (isset($userDetails[$propertyKey])) {
+            return (is_array($userDetails[$propertyKey]) ? $userDetails[$propertyKey][0] : $userDetails[$propertyKey]);
+        }
+
+        return $defaultValue;
+    }
+
     /**
      * @param Authenticatable $user
      * @param string          $username
diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php
index 54cf84266..5ccb1415e 100644
--- a/tests/Auth/LdapTest.php
+++ b/tests/Auth/LdapTest.php
@@ -46,6 +46,15 @@ class LdapTest extends BrowserKitTest
         });
     }
 
+    protected function mockUserLogin()
+    {
+        return $this->visit('/login')
+            ->see('Username')
+            ->type($this->mockUser->name, '#username')
+            ->type($this->mockUser->password, '#password')
+            ->press('Log In');
+    }
+
     public function test_login()
     {
         $this->mockLdap->shouldReceive('connect')->once()->andReturn($this->resourceId);
@@ -61,11 +70,7 @@ class LdapTest extends BrowserKitTest
         $this->mockLdap->shouldReceive('bind')->times(6)->andReturn(true);
         $this->mockEscapes(4);
 
-        $this->visit('/login')
-            ->see('Username')
-            ->type($this->mockUser->name, '#username')
-            ->type($this->mockUser->password, '#password')
-            ->press('Log In')
+        $this->mockUserLogin()
             ->seePageIs('/login')->see('Please enter an email to use for this account.');
 
         $this->type($this->mockUser->email, '#email')
@@ -91,11 +96,7 @@ class LdapTest extends BrowserKitTest
         $this->mockLdap->shouldReceive('bind')->times(3)->andReturn(true);
         $this->mockEscapes(2);
 
-        $this->visit('/login')
-            ->see('Username')
-            ->type($this->mockUser->name, '#username')
-            ->type($this->mockUser->password, '#password')
-            ->press('Log In')
+        $this->mockUserLogin()
             ->seePageIs('/')
             ->see($this->mockUser->name)
             ->seeInDatabase('users', ['email' => $this->mockUser->email, 'email_confirmed' => false, 'external_auth_id' => $ldapDn]);
@@ -116,11 +117,7 @@ class LdapTest extends BrowserKitTest
         $this->mockLdap->shouldReceive('bind')->times(3)->andReturn(true, true, false);
         $this->mockEscapes(2);
 
-        $this->visit('/login')
-            ->see('Username')
-            ->type($this->mockUser->name, '#username')
-            ->type($this->mockUser->password, '#password')
-            ->press('Log In')
+        $this->mockUserLogin()
             ->seePageIs('/login')->see('These credentials do not match our records.')
             ->dontSeeInDatabase('users', ['external_auth_id' => $this->mockUser->name]);
     }
@@ -197,12 +194,7 @@ class LdapTest extends BrowserKitTest
         $this->mockEscapes(5);
         $this->mockExplodes(6);
 
-        $this->visit('/login')
-            ->see('Username')
-            ->type($this->mockUser->name, '#username')
-            ->type($this->mockUser->password, '#password')
-            ->press('Log In')
-            ->seePageIs('/');
+        $this->mockUserLogin()->seePageIs('/');
 
         $user = User::where('email', $this->mockUser->email)->first();
         $this->seeInDatabase('role_user', [
@@ -250,12 +242,7 @@ class LdapTest extends BrowserKitTest
         $this->mockEscapes(4);
         $this->mockExplodes(2);
 
-        $this->visit('/login')
-            ->see('Username')
-            ->type($this->mockUser->name, '#username')
-            ->type($this->mockUser->password, '#password')
-            ->press('Log In')
-            ->seePageIs('/');
+        $this->mockUserLogin()->seePageIs('/');
 
         $user = User::where('email', $this->mockUser->email)->first();
         $this->seeInDatabase('role_user', [
@@ -304,12 +291,7 @@ class LdapTest extends BrowserKitTest
         $this->mockEscapes(4);
         $this->mockExplodes(2);
 
-        $this->visit('/login')
-            ->see('Username')
-            ->type($this->mockUser->name, '#username')
-            ->type($this->mockUser->password, '#password')
-            ->press('Log In')
-            ->seePageIs('/');
+        $this->mockUserLogin()->seePageIs('/');
 
         $user = User::where('email', $this->mockUser->email)->first();
         $this->seeInDatabase('role_user', [
@@ -355,12 +337,7 @@ class LdapTest extends BrowserKitTest
         $this->mockEscapes(5);
         $this->mockExplodes(6);
 
-        $this->visit('/login')
-            ->see('Username')
-            ->type($this->mockUser->name, '#username')
-            ->type($this->mockUser->password, '#password')
-            ->press('Log In')
-            ->seePageIs('/');
+        $this->mockUserLogin()->seePageIs('/');
 
         $user = User::where('email', $this->mockUser->email)->first();
         $this->seeInDatabase('role_user', [
@@ -375,7 +352,6 @@ class LdapTest extends BrowserKitTest
 
     public function test_login_uses_specified_display_name_attribute()
     {
-        $originalAttribute = config('services.ldap.display_name_attribute');
         app('config')->set([
             'services.ldap.display_name_attribute' => 'displayName'
         ]);
@@ -394,11 +370,7 @@ class LdapTest extends BrowserKitTest
         $this->mockLdap->shouldReceive('bind')->times(6)->andReturn(true);
         $this->mockEscapes(4);
 
-        $this->visit('/login')
-            ->see('Username')
-            ->type($this->mockUser->name, '#username')
-            ->type($this->mockUser->password, '#password')
-            ->press('Log In')
+        $this->mockUserLogin()
             ->seePageIs('/login')->see('Please enter an email to use for this account.');
 
         $this->type($this->mockUser->email, '#email')
@@ -406,15 +378,10 @@ class LdapTest extends BrowserKitTest
             ->seePageIs('/')
             ->see('displayNameAttribute')
             ->seeInDatabase('users', ['email' => $this->mockUser->email, 'email_confirmed' => false, 'external_auth_id' => $this->mockUser->name, 'name' => 'displayNameAttribute']);
-        
-        app('config')->set([
-            'services.ldap.display_name_attribute' => $originalAttribute
-        ]);
     }
 
     public function test_login_uses_default_display_name_attribute_if_specified_not_present()
     {
-        $originalAttribute = config('services.ldap.display_name_attribute');
         app('config')->set([
             'services.ldap.display_name_attribute' => 'displayName'
         ]);
@@ -432,11 +399,7 @@ class LdapTest extends BrowserKitTest
         $this->mockLdap->shouldReceive('bind')->times(6)->andReturn(true);
         $this->mockEscapes(4);
 
-        $this->visit('/login')
-            ->see('Username')
-            ->type($this->mockUser->name, '#username')
-            ->type($this->mockUser->password, '#password')
-            ->press('Log In')
+        $this->mockUserLogin()
             ->seePageIs('/login')->see('Please enter an email to use for this account.');
 
         $this->type($this->mockUser->email, '#email')
@@ -444,10 +407,6 @@ class LdapTest extends BrowserKitTest
             ->seePageIs('/')
             ->see($this->mockUser->name)
             ->seeInDatabase('users', ['email' => $this->mockUser->email, 'email_confirmed' => false, 'external_auth_id' => $this->mockUser->name, 'name' => $this->mockUser->name]);
-    
-        app('config')->set([
-            'services.ldap.display_name_attribute' => $originalAttribute
-        ]);
     }
 
 }