From 72d9ffd8b4a0680a858446c5f753db4f989f3989 Mon Sep 17 00:00:00 2001 From: Matthieu Leboeuf Date: Mon, 28 Oct 2024 22:14:30 +0100 Subject: [PATCH 1/3] Added support for concatenating multiple LDAP attributes in displayName --- app/Access/LdapService.php | 30 ++++++++++++++++++++++++++---- app/Config/services.php | 2 +- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/app/Access/LdapService.php b/app/Access/LdapService.php index 365cb1db0..ef6d33f4d 100644 --- a/app/Access/LdapService.php +++ b/app/Access/LdapService.php @@ -71,6 +71,28 @@ class LdapService return $users[0]; } + /** + * Calculate the display name. + */ + protected function getUserDisplayName(array $displayNameAttr, array $userDetails, string $defaultValue): string + { + $displayName = []; + foreach ($displayNameAttr as $dnAttr) { + $dnComponent = $this->getUserResponseProperty($userDetails, $dnAttr, null); + if ($dnComponent !== null) { + $displayName[] = $dnComponent; + } + } + + if (count($displayName) == 0) { + $displayName = $defaultValue; + } else { + $displayName = implode(' ', $displayName); + } + + return $displayName; + } + /** * Get the details of a user from LDAP using the given username. * User found via configurable user filter. @@ -84,9 +106,9 @@ class LdapService $displayNameAttr = $this->config['display_name_attribute']; $thumbnailAttr = $this->config['thumbnail_attribute']; - $user = $this->getUserWithAttributes($userName, array_filter([ - 'cn', 'dn', $idAttr, $emailAttr, $displayNameAttr, $thumbnailAttr, - ])); + $user = $this->getUserWithAttributes($userName, array_filter(array_merge($displayNameAttr, [ + 'cn', 'dn', $idAttr, $emailAttr, $thumbnailAttr, + ]))); if (is_null($user)) { return null; @@ -95,7 +117,7 @@ class LdapService $userCn = $this->getUserResponseProperty($user, 'cn', null); $formatted = [ 'uid' => $this->getUserResponseProperty($user, $idAttr, $user['dn']), - 'name' => $this->getUserResponseProperty($user, $displayNameAttr, $userCn), + 'name' => $this->getUserDisplayName($displayNameAttr, $user, $userCn), 'dn' => $user['dn'], 'email' => $this->getUserResponseProperty($user, $emailAttr, null), 'avatar' => $thumbnailAttr ? $this->getUserResponseProperty($user, $thumbnailAttr, null) : null, diff --git a/app/Config/services.php b/app/Config/services.php index d73458231..4e2789687 100644 --- a/app/Config/services.php +++ b/app/Config/services.php @@ -127,7 +127,7 @@ return [ 'version' => env('LDAP_VERSION', false), 'id_attribute' => env('LDAP_ID_ATTRIBUTE', 'uid'), 'email_attribute' => env('LDAP_EMAIL_ATTRIBUTE', 'mail'), - 'display_name_attribute' => env('LDAP_DISPLAY_NAME_ATTRIBUTE', 'cn'), + 'display_name_attribute' => explode('|', 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 87242ce6cb462bc933e63d80e514ae5096ee2b67 Mon Sep 17 00:00:00 2001 From: Matthieu Leboeuf Date: Mon, 28 Oct 2024 22:27:15 +0100 Subject: [PATCH 2/3] Adapt tests with displayName array --- tests/Auth/LdapTest.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php index ef95bc2e8..27169a2be 100644 --- a/tests/Auth/LdapTest.php +++ b/tests/Auth/LdapTest.php @@ -29,7 +29,7 @@ class LdapTest extends TestCase 'auth.defaults.guard' => 'ldap', 'services.ldap.base_dn' => 'dc=ldap,dc=local', 'services.ldap.email_attribute' => 'mail', - 'services.ldap.display_name_attribute' => 'cn', + 'services.ldap.display_name_attribute' => ['cn'], 'services.ldap.id_attribute' => 'uid', 'services.ldap.user_to_groups' => false, 'services.ldap.version' => '3', @@ -581,7 +581,7 @@ class LdapTest extends TestCase public function test_login_uses_specified_display_name_attribute() { app('config')->set([ - 'services.ldap.display_name_attribute' => 'displayName', + 'services.ldap.display_name_attribute' => ['displayName'], ]); $this->commonLdapMocks(1, 1, 2, 4, 2); @@ -606,7 +606,7 @@ class LdapTest extends TestCase public function test_login_uses_default_display_name_attribute_if_specified_not_present() { app('config')->set([ - 'services.ldap.display_name_attribute' => 'displayName', + 'services.ldap.display_name_attribute' => ['displayName'], ]); $this->commonLdapMocks(1, 1, 2, 4, 2); From 90341e0e00a832e78df8a394487a073a475003ed Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 1 Dec 2024 18:42:54 +0000 Subject: [PATCH 3/3] LDAP: Review and testing of mulitple-display-name attr support Review of #5295 Added test to cover functionality. Moved splitting from config to service. --- app/Access/LdapService.php | 30 ++++++++++++++---------------- app/Config/services.php | 2 +- tests/Auth/LdapTest.php | 33 ++++++++++++++++++++++++++++++--- 3 files changed, 45 insertions(+), 20 deletions(-) diff --git a/app/Access/LdapService.php b/app/Access/LdapService.php index ef6d33f4d..e5037ad2f 100644 --- a/app/Access/LdapService.php +++ b/app/Access/LdapService.php @@ -72,25 +72,23 @@ class LdapService } /** - * Calculate the display name. + * Build the user display name from the (potentially multiple) attributes defined by the configuration. */ - protected function getUserDisplayName(array $displayNameAttr, array $userDetails, string $defaultValue): string + protected function getUserDisplayName(array $userDetails, array $displayNameAttrs, string $defaultValue): string { - $displayName = []; - foreach ($displayNameAttr as $dnAttr) { + $displayNameParts = []; + foreach ($displayNameAttrs as $dnAttr) { $dnComponent = $this->getUserResponseProperty($userDetails, $dnAttr, null); - if ($dnComponent !== null) { - $displayName[] = $dnComponent; + if ($dnComponent) { + $displayNameParts[] = $dnComponent; } } - if (count($displayName) == 0) { - $displayName = $defaultValue; - } else { - $displayName = implode(' ', $displayName); + if (empty($displayNameParts)) { + return $defaultValue; } - return $displayName; + return implode(' ', $displayNameParts); } /** @@ -103,12 +101,12 @@ class LdapService { $idAttr = $this->config['id_attribute']; $emailAttr = $this->config['email_attribute']; - $displayNameAttr = $this->config['display_name_attribute']; + $displayNameAttrs = explode('|', $this->config['display_name_attribute']); $thumbnailAttr = $this->config['thumbnail_attribute']; - $user = $this->getUserWithAttributes($userName, array_filter(array_merge($displayNameAttr, [ - 'cn', 'dn', $idAttr, $emailAttr, $thumbnailAttr, - ]))); + $user = $this->getUserWithAttributes($userName, array_filter([ + 'cn', 'dn', $idAttr, $emailAttr, ...$displayNameAttrs, $thumbnailAttr, + ])); if (is_null($user)) { return null; @@ -117,7 +115,7 @@ class LdapService $userCn = $this->getUserResponseProperty($user, 'cn', null); $formatted = [ 'uid' => $this->getUserResponseProperty($user, $idAttr, $user['dn']), - 'name' => $this->getUserDisplayName($displayNameAttr, $user, $userCn), + 'name' => $this->getUserDisplayName($user, $displayNameAttrs, $userCn), 'dn' => $user['dn'], 'email' => $this->getUserResponseProperty($user, $emailAttr, null), 'avatar' => $thumbnailAttr ? $this->getUserResponseProperty($user, $thumbnailAttr, null) : null, diff --git a/app/Config/services.php b/app/Config/services.php index 4e2789687..d73458231 100644 --- a/app/Config/services.php +++ b/app/Config/services.php @@ -127,7 +127,7 @@ return [ 'version' => env('LDAP_VERSION', false), 'id_attribute' => env('LDAP_ID_ATTRIBUTE', 'uid'), 'email_attribute' => env('LDAP_EMAIL_ATTRIBUTE', 'mail'), - 'display_name_attribute' => explode('|', env('LDAP_DISPLAY_NAME_ATTRIBUTE', 'cn')), + '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'), diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php index 27169a2be..9a00c983a 100644 --- a/tests/Auth/LdapTest.php +++ b/tests/Auth/LdapTest.php @@ -29,7 +29,7 @@ class LdapTest extends TestCase 'auth.defaults.guard' => 'ldap', 'services.ldap.base_dn' => 'dc=ldap,dc=local', 'services.ldap.email_attribute' => 'mail', - 'services.ldap.display_name_attribute' => ['cn'], + 'services.ldap.display_name_attribute' => 'cn', 'services.ldap.id_attribute' => 'uid', 'services.ldap.user_to_groups' => false, 'services.ldap.version' => '3', @@ -581,7 +581,7 @@ class LdapTest extends TestCase public function test_login_uses_specified_display_name_attribute() { app('config')->set([ - 'services.ldap.display_name_attribute' => ['displayName'], + 'services.ldap.display_name_attribute' => 'displayName', ]); $this->commonLdapMocks(1, 1, 2, 4, 2); @@ -603,10 +603,37 @@ class LdapTest extends TestCase $this->assertDatabaseHas('users', ['email' => $this->mockUser->email, 'email_confirmed' => false, 'external_auth_id' => $this->mockUser->name, 'name' => 'displayNameAttribute']); } + public function test_login_uses_multiple_display_properties_if_defined() + { + app('config')->set([ + 'services.ldap.display_name_attribute' => 'firstname|middlename|noname|lastname', + ]); + + $this->commonLdapMocks(1, 1, 1, 2, 1); + $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1) + ->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'), + 'firstname' => ['Barry'], + 'middlename' => ['Elliott'], + 'lastname' => ['Chuckle'], + 'mail' => [$this->mockUser->email], + ]]); + + $this->mockUserLogin(); + + $this->assertDatabaseHas('users', [ + 'email' => $this->mockUser->email, + 'name' => 'Barry Elliott Chuckle', + ]); + } + public function test_login_uses_default_display_name_attribute_if_specified_not_present() { app('config')->set([ - 'services.ldap.display_name_attribute' => ['displayName'], + 'services.ldap.display_name_attribute' => 'displayName', ]); $this->commonLdapMocks(1, 1, 2, 4, 2);