diff --git a/app/Auth/Access/LdapService.php b/app/Auth/Access/LdapService.php index 9ffbbfbb7..c7415e1f7 100644 --- 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 5ccb1415e..5923ef377 100644 --- 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); + } }