Altered ldap_connect usage, cleaned up LDAP classes

Primarily updated ldap_connect to avoid usage of deprecated syntax.
Updated tests and service to handle as expected.
Cleaned up syntax and types in classes while there.

Closes #4274

app/Access/Ldap.php

@@ -12,20 +12,19 @@ class Ldap
     /**
      * Connect to an LDAP server.
      *
-     * @return resource
+     * @return resource|\LDAP\Connection|false
      */
-    public function connect(string $hostName, int $port)
+    public function connect(string $hostName)
     {
-        return ldap_connect($hostName, $port);
+        return ldap_connect($hostName);
     }
 
     /**
-     * Set the value of a LDAP option for the given connection.
+     * Set the value of an LDAP option for the given connection.
      *
-     * @param resource $ldapConnection
+     * @param resource|\LDAP\Connection|null $ldapConnection
-     * @param mixed    $value
      */
-    public function setOption($ldapConnection, int $option, $value): bool
+    public function setOption($ldapConnection, int $option, mixed $value): bool
     {
         return ldap_set_option($ldapConnection, $option, $value);
     }
@@ -39,9 +38,9 @@ class Ldap
     }
 
     /**
-     * Set the version number for the given ldap connection.
+     * Set the version number for the given LDAP connection.
      *
-     * @param resource $ldapConnection
+     * @param resource|\LDAP\Connection $ldapConnection
      */
     public function setVersion($ldapConnection, int $version): bool
     {
@@ -51,27 +50,22 @@ class Ldap
     /**
      * Search LDAP tree using the provided filter.
      *
-     * @param resource   $ldapConnection
+     * @param resource|\LDAP\Connection   $ldapConnection
-     * @param string     $baseDn
-     * @param string     $filter
-     * @param array|null $attributes
      *
-     * @return resource
+     * @return resource|\LDAP\Result
      */
-    public function search($ldapConnection, $baseDn, $filter, array $attributes = null)
+    public function search($ldapConnection, string $baseDn, string $filter, array $attributes = null)
     {
         return ldap_search($ldapConnection, $baseDn, $filter, $attributes);
     }
 
     /**
-     * Get entries from an ldap search result.
+     * Get entries from an LDAP search result.
      *
-     * @param resource $ldapConnection
+     * @param resource|\LDAP\Connection $ldapConnection
-     * @param resource $ldapSearchResult
+     * @param resource|\LDAP\Result $ldapSearchResult
-     *
-     * @return array
      */
-    public function getEntries($ldapConnection, $ldapSearchResult)
+    public function getEntries($ldapConnection, $ldapSearchResult): array|false
     {
         return ldap_get_entries($ldapConnection, $ldapSearchResult);
     }
@@ -79,14 +73,9 @@ class Ldap
     /**
      * Search and get entries immediately.
      *
-     * @param resource   $ldapConnection
+     * @param resource|\LDAP\Connection   $ldapConnection
-     * @param string     $baseDn
-     * @param string     $filter
-     * @param array|null $attributes
-     *
-     * @return resource
      */
-    public function searchAndGetEntries($ldapConnection, $baseDn, $filter, array $attributes = null)
+    public function searchAndGetEntries($ldapConnection, string $baseDn, string $filter, array $attributes = null): array|false
     {
         $search = $this->search($ldapConnection, $baseDn, $filter, $attributes);
 
@@ -96,40 +85,25 @@ class Ldap
     /**
      * Bind to LDAP directory.
      *
-     * @param resource $ldapConnection
+     * @param resource|\LDAP\Connection $ldapConnection
-     * @param string   $bindRdn
-     * @param string   $bindPassword
-     *
-     * @return bool
      */
-    public function bind($ldapConnection, $bindRdn = null, $bindPassword = null)
+    public function bind($ldapConnection, string $bindRdn = null, string $bindPassword = null): bool
     {
         return ldap_bind($ldapConnection, $bindRdn, $bindPassword);
     }
 
     /**
-     * Explode a LDAP dn string into an array of components.
+     * Explode an LDAP dn string into an array of components.
-     *
-     * @param string $dn
-     * @param int    $withAttrib
-     *
-     * @return array
      */
-    public function explodeDn(string $dn, int $withAttrib)
+    public function explodeDn(string $dn, int $withAttrib): array|false
     {
         return ldap_explode_dn($dn, $withAttrib);
     }
 
     /**
      * Escape a string for use in an LDAP filter.
-     *
-     * @param string $value
-     * @param string $ignore
-     * @param int    $flags
-     *
-     * @return string
      */
-    public function escape(string $value, string $ignore = '', int $flags = 0)
+    public function escape(string $value, string $ignore = '', int $flags = 0): string
     {
         return ldap_escape($value, $ignore, $flags);
     }

app/Access/LdapService.php

@@ -15,26 +15,19 @@ use Illuminate\Support\Facades\Log;
  */
 class LdapService
 {
-    protected Ldap $ldap;
-    protected GroupSyncService $groupSyncService;
-    protected UserAvatars $userAvatars;
-
     /**
-     * @var resource
+     * @var resource|\LDAP\Connection
      */
     protected $ldapConnection;
 
     protected array $config;
     protected bool $enabled;
 
-    /**
+    public function __construct(
-     * LdapService constructor.
+        protected Ldap $ldap,
-     */
+        protected UserAvatars $userAvatars,
-    public function __construct(Ldap $ldap, UserAvatars $userAvatars, GroupSyncService $groupSyncService)
+        protected GroupSyncService $groupSyncService
-    {
+    ) {
-        $this->ldap = $ldap;
-        $this->userAvatars = $userAvatars;
-        $this->groupSyncService = $groupSyncService;
         $this->config = config('services.ldap');
         $this->enabled = config('auth.method') === 'ldap';
     }
@@ -59,7 +52,7 @@ class LdapService
 
         // Clean attributes
         foreach ($attributes as $index => $attribute) {
-            if (strpos($attribute, 'BIN;') === 0) {
+            if (str_starts_with($attribute, 'BIN;')) {
                 $attributes[$index] = substr($attribute, strlen('BIN;'));
             }
         }
@@ -82,7 +75,7 @@ class LdapService
      * Get the details of a user from LDAP using the given username.
      * User found via configurable user filter.
      *
-     * @throws LdapException
+     * @throws LdapException|JsonDebugException
      */
     public function getUserDetails(string $userName): ?array
     {
@@ -126,7 +119,7 @@ class LdapService
      */
     protected function getUserResponseProperty(array $userDetails, string $propertyKey, $defaultValue)
     {
-        $isBinary = strpos($propertyKey, 'BIN;') === 0;
+        $isBinary = str_starts_with($propertyKey, 'BIN;');
         $propertyKey = strtolower($propertyKey);
         $value = $defaultValue;
 
@@ -170,11 +163,11 @@ class LdapService
      * Bind the system user to the LDAP connection using the given credentials
      * otherwise anonymous access is attempted.
      *
-     * @param resource $connection
+     * @param resource|\LDAP\Connection $connection
      *
      * @throws LdapException
      */
-    protected function bindSystemUser($connection)
+    protected function bindSystemUser($connection): void
     {
         $ldapDn = $this->config['dn'];
         $ldapPass = $this->config['pass'];
@@ -197,7 +190,7 @@ class LdapService
      *
      * @throws LdapException
      *
-     * @return resource
+     * @return resource|\LDAP\Connection
      */
     protected function getConnection()
     {
@@ -216,8 +209,8 @@ class LdapService
             $this->ldap->setOption(null, LDAP_OPT_X_TLS_REQUIRE_CERT, LDAP_OPT_X_TLS_NEVER);
         }
 
-        $serverDetails = $this->parseServerString($this->config['server']);
+        $ldapHost = $this->parseServerString($this->config['server']);
-        $ldapConnection = $this->ldap->connect($serverDetails['host'], $serverDetails['port']);
+        $ldapConnection = $this->ldap->connect($ldapHost);
 
         if ($ldapConnection === false) {
             throw new LdapException(trans('errors.ldap_cannot_connect'));
@@ -242,23 +235,16 @@ class LdapService
     }
 
     /**
-     * Parse a LDAP server string and return the host and port for a connection.
+     * Parse an LDAP server string and return the host suitable for a connection.
      * Is flexible to formats such as 'ldap.example.com:8069' or 'ldaps://ldap.example.com'.
      */
-    protected function parseServerString(string $serverString): array
+    protected function parseServerString(string $serverString): string
     {
-        $serverNameParts = explode(':', $serverString);
+        if (str_starts_with($serverString, 'ldaps://') || str_starts_with($serverString, 'ldap://')) {
-
+            return $serverString;
-        // If we have a protocol just return the full string since PHP will ignore a separate port.
-        if ($serverNameParts[0] === 'ldaps' || $serverNameParts[0] === 'ldap') {
-            return ['host' => $serverString, 'port' => 389];
         }
 
-        // Otherwise, extract the port out
+        return "ldap://{$serverString}";
-        $hostName = $serverNameParts[0];
-        $ldapPort = (count($serverNameParts) > 1) ? intval($serverNameParts[1]) : 389;
-
-        return ['host' => $hostName, 'port' => $ldapPort];
     }
 
     /**
@@ -386,7 +372,7 @@ class LdapService
      * @throws LdapException
      * @throws JsonDebugException
      */
-    public function syncGroups(User $user, string $username)
+    public function syncGroups(User $user, string $username): void
     {
         $userLdapGroups = $this->getUserGroups($username);
         $this->groupSyncService->syncUserWithFoundGroups($user, $userLdapGroups, $this->config['remove_from_groups']);

tests/Auth/LdapTest.php

@@ -12,13 +12,10 @@ use Tests\TestCase;
 
 class LdapTest extends TestCase
 {
-    /**
+    protected MockInterface $mockLdap;
-     * @var MockInterface
-     */
-    protected $mockLdap;
 
-    protected $mockUser;
+    protected User $mockUser;
-    protected $resourceId = 'resource-test';
+    protected string $resourceId = 'resource-test';
 
     protected function setUp(): void
     {
@@ -40,8 +37,7 @@ class LdapTest extends TestCase
             'services.ldap.tls_insecure'           => false,
             'services.ldap.thumbnail_attribute'    => null,
         ]);
-        $this->mockLdap = \Mockery::mock(Ldap::class);
+        $this->mockLdap = $this->mock(Ldap::class);
-        $this->app[Ldap::class] = $this->mockLdap;
         $this->mockUser = User::factory()->make();
     }
 
@@ -96,7 +92,7 @@ class LdapTest extends TestCase
             ->andReturn(['count' => 1, 0 => [
                 'uid' => [$this->mockUser->name],
                 'cn'  => [$this->mockUser->name],
-                'dn'  => ['dc=test' . config('services.ldap.base_dn')],
+                'dn'  => 'dc=test' . config('services.ldap.base_dn'),
             ]]);
 
         $resp = $this->mockUserLogin();
@@ -127,7 +123,7 @@ class LdapTest extends TestCase
             ->andReturn(['count' => 1, 0 => [
                 'uid' => [$this->mockUser->name],
                 'cn'  => [$this->mockUser->name],
-                'dn'  => ['dc=test' . config('services.ldap.base_dn')],
+                'dn'  => 'dc=test' . config('services.ldap.base_dn'),
             ]]);
 
         $resp = $this->mockUserLogin();
@@ -190,7 +186,7 @@ class LdapTest extends TestCase
             ->andReturn(['count' => 1, 0 => [
                 'uid' => [$this->mockUser->name],
                 'cn'  => [$this->mockUser->name],
-                'dn'  => ['dc=test' . config('services.ldap.base_dn')],
+                'dn'  => 'dc=test' . config('services.ldap.base_dn'),
             ]]);
         $this->mockLdap->shouldReceive('bind')->times(2)->andReturn(true, false);
 
@@ -283,7 +279,7 @@ class LdapTest extends TestCase
             ->andReturn(['count' => 1, 0 => [
                 'uid'      => [$this->mockUser->name],
                 'cn'       => [$this->mockUser->name],
-                'dn'       => ['dc=test' . config('services.ldap.base_dn')],
+                'dn'       => 'dc=test' . config('services.ldap.base_dn'),
                 'mail'     => [$this->mockUser->email],
                 'memberof' => [
                     'count' => 2,
@@ -328,7 +324,7 @@ class LdapTest extends TestCase
             ->andReturn(['count' => 1, 0 => [
                 'uid'      => [$this->mockUser->name],
                 'cn'       => [$this->mockUser->name],
-                'dn'       => ['dc=test' . config('services.ldap.base_dn')],
+                'dn'       => 'dc=test' . config('services.ldap.base_dn'),
                 'mail'     => [$this->mockUser->email],
                 'memberof' => [
                     'count' => 1,
@@ -429,7 +425,7 @@ class LdapTest extends TestCase
             ->andReturn(['count' => 1, 0 => [
                 'uid'      => [$this->mockUser->name],
                 'cn'       => [$this->mockUser->name],
-                'dn'       => ['dc=test' . config('services.ldap.base_dn')],
+                'dn'       => 'dc=test' . config('services.ldap.base_dn'),
                 'mail'     => [$this->mockUser->email],
                 'memberof' => [
                     'count' => 1,
@@ -470,7 +466,7 @@ class LdapTest extends TestCase
             ->andReturn(['count' => 1, 0 => [
                 'uid'      => [$this->mockUser->name],
                 'cn'       => [$this->mockUser->name],
-                'dn'       => ['dc=test' . config('services.ldap.base_dn')],
+                'dn'       => 'dc=test' . config('services.ldap.base_dn'),
                 'mail'     => [$this->mockUser->email],
                 'memberof' => [
                     'count' => 2,
@@ -504,7 +500,7 @@ class LdapTest extends TestCase
             ->andReturn(['count' => 1, 0 => [
                 'uid'         => [$this->mockUser->name],
                 'cn'          => [$this->mockUser->name],
-                'dn'          => ['dc=test' . config('services.ldap.base_dn')],
+                'dn'          => 'dc=test' . config('services.ldap.base_dn'),
                 'displayname' => 'displayNameAttribute',
             ]]);
 
@@ -529,7 +525,7 @@ class LdapTest extends TestCase
             ->andReturn(['count' => 1, 0 => [
                 'uid' => [$this->mockUser->name],
                 'cn'  => [$this->mockUser->name],
-                'dn'  => ['dc=test' . config('services.ldap.base_dn')],
+                'dn'  => 'dc=test' . config('services.ldap.base_dn'),
             ]]);
 
         $this->mockUserLogin()->assertRedirect('/login');
@@ -546,39 +542,33 @@ class LdapTest extends TestCase
         ]);
     }
 
-    protected function checkLdapReceivesCorrectDetails($serverString, $expectedHost, $expectedPort)
+    protected function checkLdapReceivesCorrectDetails($serverString, $expectedHostString): void
     {
-        app('config')->set([
+        app('config')->set(['services.ldap.server' => $serverString]);
-            'services.ldap.server' => $serverString,
-        ]);
 
-        // Standard mocks
+        $this->mockLdap->shouldReceive('connect')
-        $this->commonLdapMocks(0, 1, 1, 2, 1);
+            ->once()
-        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1)->andReturn(['count' => 1, 0 => [
+            ->with($expectedHostString)
-            'uid' => [$this->mockUser->name],
+            ->andReturn(false);
-            'cn'  => [$this->mockUser->name],
-            'dn'  => ['dc=test' . config('services.ldap.base_dn')],
-        ]]);
 
-        $this->mockLdap->shouldReceive('connect')->once()
-            ->with($expectedHost, $expectedPort)->andReturn($this->resourceId);
         $this->mockUserLogin();
     }
 
-    public function test_ldap_port_provided_on_host_if_host_is_full_uri()
+    public function test_ldap_receives_correct_connect_host_from_config()
     {
-        $hostName = 'ldaps://bookstack:8080';
+        $expectedResultByInput = [
-        $this->checkLdapReceivesCorrectDetails($hostName, $hostName, 389);
+            'ldaps://bookstack:8080' => 'ldaps://bookstack:8080',
-    }
+            'ldap.bookstack.com:8080' => 'ldap://ldap.bookstack.com:8080',
+            'ldap.bookstack.com' => 'ldap://ldap.bookstack.com',
+            'ldaps://ldap.bookstack.com' => 'ldaps://ldap.bookstack.com',
+            'ldaps://ldap.bookstack.com ldap://a.b.com' => 'ldaps://ldap.bookstack.com ldap://a.b.com',
+        ];
 
-    public function test_ldap_port_parsed_from_server_if_host_is_not_full_uri()
+        foreach ($expectedResultByInput as $input => $expectedResult) {
-    {
+            $this->checkLdapReceivesCorrectDetails($input, $expectedResult);
-        $this->checkLdapReceivesCorrectDetails('ldap.bookstack.com:8080', 'ldap.bookstack.com', 8080);
+            $this->refreshApplication();
+            $this->setUp();
         }
-
-    public function test_default_ldap_port_used_if_not_in_server_string_and_not_uri()
-    {
-        $this->checkLdapReceivesCorrectDetails('ldap.bookstack.com', 'ldap.bookstack.com', 389);
     }
 
     public function test_forgot_password_routes_inaccessible()
@@ -626,7 +616,7 @@ class LdapTest extends TestCase
                 'cn'  => [$this->mockUser->name],
                 // Test dumping binary data for avatar responses
                 'jpegphoto' => base64_decode('/9j/4AAQSkZJRg=='),
-                'dn'        => ['dc=test' . config('services.ldap.base_dn')],
+                'dn'        => 'dc=test' . config('services.ldap.base_dn'),
             ]]);
 
         $resp = $this->post('/login', [
@@ -665,7 +655,7 @@ class LdapTest extends TestCase
             ->andReturn(['count' => 1, 0 => [
                 'uid' => [hex2bin('FFF8F7')],
                 'cn'  => [$this->mockUser->name],
-                'dn'  => ['dc=test' . config('services.ldap.base_dn')],
+                'dn'  => 'dc=test' . config('services.ldap.base_dn'),
             ]]);
 
         $details = $ldapService->getUserDetails('test');
@@ -680,12 +670,12 @@ class LdapTest extends TestCase
             ->andReturn(['count' => 1, 0 => [
                 'uid'  => [$this->mockUser->name],
                 'cn'   => [$this->mockUser->name],
-                'dn'   => ['dc=test' . config('services.ldap.base_dn')],
+                'dn'   => 'dc=test' . config('services.ldap.base_dn'),
                 'mail' => 'tester@example.com',
             ]], ['count' => 1, 0 => [
                 'uid'  => ['Barry'],
                 'cn'   => ['Scott'],
-                'dn'   => ['dc=bscott' . config('services.ldap.base_dn')],
+                'dn'   => 'dc=bscott' . config('services.ldap.base_dn'),
                 'mail' => 'tester@example.com',
             ]]);
 
@@ -716,7 +706,7 @@ class LdapTest extends TestCase
             ->andReturn(['count' => 1, 0 => [
                 'uid'      => [$user->name],
                 'cn'       => [$user->name],
-                'dn'       => ['dc=test' . config('services.ldap.base_dn')],
+                'dn'       => 'dc=test' . config('services.ldap.base_dn'),
                 'mail'     => [$user->email],
                 'memberof' => [
                     'count' => 1,