LDAP: Review and testing of mulitple-display-name attr support

Review of #5295 Added test to cover functionality. Moved splitting from config to service.
BookStackApp · Dec 1, 2024 · 90341e0 · 90341e0
1 parent 87242ce
commit 90341e0
Show file tree

Hide file tree

Showing 3 changed files with 45 additions and 20 deletions.
diff --git a/app/Access/LdapService.php b/app/Access/LdapService.php
@@ -72,25 +72,23 @@ private function getUserWithAttributes(string $userName, array $attributes): ?ar
     }
 
     /**
-     * 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 @@ public function getUserDetails(string $userName): ?array
     {
         $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 @@ public function getUserDetails(string $userName): ?array
         $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
@@ -127,7 +127,7 @@
         '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
@@ -29,7 +29,7 @@ protected function setUp(): void
             '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 @@ public function test_login_group_mapping_does_not_conflict_with_default_role()
     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 @@ public function test_login_uses_specified_display_name_attribute()
         $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);