Updated ldap server option parsing to work with protocol and port

- Aligns with PHP behaviour where ports is ignore for full LDAP URI.
- Added tests to check format being passed to LDAP is as expected.
- May be related to #1220
- Related to #1386 and #1278

diff --git a/app/Auth/Access/LdapService.php b/app/Auth/Access/LdapService.php
index 9ffbbfbb75b57862c5543c4e4002013bd6485d28..c7415e1f738e723bd18e98ea9a3f5c6a217bb697 100644 (file)
--- a/app/Auth/Access/LdapService.php
+++ b/app/Auth/Access/LdapService.php
@@ -91,7 +91,7 @@ class LdapService
         $userCn = $this->getUserResponseProperty($user, 'cn', null);
         return [
             'uid'   => $this->getUserResponseProperty($user, 'uid', $user['dn']),
-            'name' => $this->getUserResponseProperty($user, $displayNameAttr, $userCn),
+            'name'  => $this->getUserResponseProperty($user, $displayNameAttr, $userCn),
             'dn'    => $user['dn'],
             'email' => $this->getUserResponseProperty($user, $emailAttr, null),
         ];
@@ -116,8 +116,8 @@ class LdapService
 
     /**
      * @param Authenticatable $user
-     * @param string          $username
-     * @param string          $password
+     * @param string $username
+     * @param string $password
      * @return bool
      * @throws LdapException
      */
@@ -182,25 +182,14 @@ class LdapService
             throw new LdapException(trans('errors.ldap_extension_not_installed'));
         }
 
-        // Get port from server string and protocol if specified.
-        $ldapServer = explode(':', $this->config['server']);
-        $hasProtocol = preg_match('/^ldaps{0,1}\:\/\//', $this->config['server']) === 1;
-        if (!$hasProtocol) {
-            array_unshift($ldapServer, '');
-        }
-        $hostName = $ldapServer[0] . ($hasProtocol?':':'') . $ldapServer[1];
-        $defaultPort = $ldapServer[0] === 'ldaps' ? 636 : 389;
-
-        /*
-         * Check if TLS_INSECURE is set. The handle is set to NULL due to the nature of
-         * the LDAP_OPT_X_TLS_REQUIRE_CERT option. It can only be set globally and not
-         * per handle.
-         */
+         // Check if TLS_INSECURE is set. The handle is set to NULL due to the nature of
+         // the LDAP_OPT_X_TLS_REQUIRE_CERT option. It can only be set globally and not per handle.
         if ($this->config['tls_insecure']) {
             $this->ldap->setOption(null, LDAP_OPT_X_TLS_REQUIRE_CERT, LDAP_OPT_X_TLS_NEVER);
         }
 
-        $ldapConnection = $this->ldap->connect($hostName, count($ldapServer) > 2 ? intval($ldapServer[2]) : $defaultPort);
+        $serverDetails = $this->parseServerString($this->config['server']);
+        $ldapConnection = $this->ldap->connect($serverDetails['host'], $serverDetails['port']);
 
         if ($ldapConnection === false) {
             throw new LdapException(trans('errors.ldap_cannot_connect'));
@@ -215,6 +204,27 @@ class LdapService
         return $this->ldapConnection;
     }
 
+    /**
+     * Parse a LDAP server string and return the host and port for
+     * a connection. Is flexible to formats such as 'ldap.example.com:8069' or 'ldaps://ldap.example.com'
+     * @param $serverString
+     * @return array
+     */
+    protected function parseServerString($serverString)
+    {
+        $serverNameParts = explode(':', $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
+        $hostName = $serverNameParts[0];
+        $ldapPort = (count($serverNameParts) > 1) ? intval($serverNameParts[1]) : 389;
+        return ['host' => $hostName, 'port' => $ldapPort];
+    }
+
     /**
      * Build a filter string by injecting common variables.
      * @param string $filterString
@@ -319,10 +329,10 @@ class LdapService
         $count = 0;
 
         if (isset($userGroupSearchResponse[$groupsAttr]['count'])) {
-            $count = (int) $userGroupSearchResponse[$groupsAttr]['count'];
+            $count = (int)$userGroupSearchResponse[$groupsAttr]['count'];
         }
 
-        for ($i=0; $i<$count; $i++) {
+        for ($i = 0; $i < $count; $i++) {
             $dnComponents = $this->ldap->explodeDn($userGroupSearchResponse[$groupsAttr][$i], 1);
             if (!in_array($dnComponents[0], $ldapGroups)) {
                 $ldapGroups[] = $dnComponents[0];

diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php
index 5ccb1415e6fed57e2bced5e33e6d04b966cadf2a..5923ef377700734f931b6a56098bfe14a18e255d 100644 (file)
--- a/tests/Auth/LdapTest.php
+++ b/tests/Auth/LdapTest.php
@@ -409,4 +409,41 @@ class LdapTest extends BrowserKitTest
             ->seeInDatabase('users', ['email' => $this->mockUser->email, 'email_confirmed' => false, 'external_auth_id' => $this->mockUser->name, 'name' => $this->mockUser->name]);
     }
 
+    protected function checkLdapReceivesCorrectDetails($serverString, $expectedHost, $expectedPort)
+    {
+        app('config')->set([
+            'services.ldap.server' => $serverString
+        ]);
+
+        // Standard mocks
+        $this->mockLdap->shouldReceive('setVersion')->once();
+        $this->mockLdap->shouldReceive('setOption')->times(2);
+        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2)->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(3)->andReturn(true);
+        $this->mockEscapes(2);
+
+        $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()
+    {
+        $hostName = 'ldaps://bookstack:8080';
+        $this->checkLdapReceivesCorrectDetails($hostName, $hostName, 389);
+    }
+
+    public function test_ldap_port_parsed_from_server_if_host_is_not_full_uri()
+    {
+        $this->checkLdapReceivesCorrectDetails('ldap.bookstack.com:8080', 'ldap.bookstack.com', 8080);
+    }
+
+    public function test_default_ldap_port_used_if_not_in_server_string_and_not_uri()
+    {
+        $this->checkLdapReceivesCorrectDetails('ldap.bookstack.com', 'ldap.bookstack.com', 389);
+    }
 }