From 77d4a28442313390d6f2033d5b2183b335fdd492 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 18 Oct 2022 00:21:11 +0100 Subject: [PATCH] Refactored & split LDAP connection logic Updated ldap connections to be carried via their own class. Extracted connection logic to its own class. Having trouble making this new format testable. --- app/Auth/Access/Guards/LdapSessionGuard.php | 2 +- app/Auth/Access/Ldap.php | 137 ----------- app/Auth/Access/Ldap/LdapConnection.php | 129 ++++++++++ .../Access/Ldap/LdapConnectionManager.php | 155 ++++++++++++ app/Auth/Access/{ => Ldap}/LdapService.php | 224 ++---------------- app/Providers/AuthServiceProvider.php | 2 +- tests/Auth/LdapTest.php | 16 +- 7 files changed, 320 insertions(+), 345 deletions(-) delete mode 100644 app/Auth/Access/Ldap.php create mode 100644 app/Auth/Access/Ldap/LdapConnection.php create mode 100644 app/Auth/Access/Ldap/LdapConnectionManager.php rename app/Auth/Access/{ => Ldap}/LdapService.php (53%) diff --git a/app/Auth/Access/Guards/LdapSessionGuard.php b/app/Auth/Access/Guards/LdapSessionGuard.php index e7ed22704..9f967916a 100644 --- a/app/Auth/Access/Guards/LdapSessionGuard.php +++ b/app/Auth/Access/Guards/LdapSessionGuard.php @@ -2,7 +2,7 @@ namespace BookStack\Auth\Access\Guards; -use BookStack\Auth\Access\LdapService; +use BookStack\Auth\Access\Ldap\LdapService; use BookStack\Auth\Access\RegistrationService; use BookStack\Auth\User; use BookStack\Exceptions\JsonDebugException; diff --git a/app/Auth/Access/Ldap.php b/app/Auth/Access/Ldap.php deleted file mode 100644 index f8f3db062..000000000 --- a/app/Auth/Access/Ldap.php +++ /dev/null @@ -1,137 +0,0 @@ -setOption($ldapConnection, LDAP_OPT_PROTOCOL_VERSION, $version); - } - - /** - * Search LDAP tree using the provided filter. - * - * @param resource $ldapConnection - * @param string $baseDn - * @param string $filter - * @param array|null $attributes - * - * @return resource - */ - public function search($ldapConnection, $baseDn, $filter, array $attributes = null) - { - return ldap_search($ldapConnection, $baseDn, $filter, $attributes); - } - - /** - * Get entries from an ldap search result. - * - * @param resource $ldapConnection - * @param resource $ldapSearchResult - * - * @return array - */ - public function getEntries($ldapConnection, $ldapSearchResult) - { - return ldap_get_entries($ldapConnection, $ldapSearchResult); - } - - /** - * Search and get entries immediately. - * - * @param resource $ldapConnection - * @param string $baseDn - * @param string $filter - * @param array|null $attributes - * - * @return resource - */ - public function searchAndGetEntries($ldapConnection, $baseDn, $filter, array $attributes = null) - { - $search = $this->search($ldapConnection, $baseDn, $filter, $attributes); - - return $this->getEntries($ldapConnection, $search); - } - - /** - * Bind to LDAP directory. - * - * @param resource $ldapConnection - * @throws ErrorException - */ - 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. - * - * @param string $dn - * @param int $withAttrib - * - * @return array - */ - public function explodeDn(string $dn, int $withAttrib) - { - 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) - { - return ldap_escape($value, $ignore, $flags); - } -} diff --git a/app/Auth/Access/Ldap/LdapConnection.php b/app/Auth/Access/Ldap/LdapConnection.php new file mode 100644 index 000000000..f9eb9f17e --- /dev/null +++ b/app/Auth/Access/Ldap/LdapConnection.php @@ -0,0 +1,129 @@ +connection = $this->connect($hostName, $port); + } + + /** + * Connect to an LDAP server. + * + * @return resource + */ + protected function connect(string $hostName, int $port) + { + return ldap_connect($hostName, $port); + } + + /** + * Set the value of a LDAP option for the current connection. + * + * @param mixed $value + */ + public function setOption(int $option, $value): bool + { + return ldap_set_option($this->connection, $option, $value); + } + + /** + * Start TLS for this LDAP connection. + */ + public function startTls(): bool + { + return ldap_start_tls($this->connection); + } + + /** + * Set the version number for this ldap connection. + */ + public function setVersion(int $version): bool + { + return $this->setOption(LDAP_OPT_PROTOCOL_VERSION, $version); + } + + /** + * Search LDAP tree using the provided filter. + * + * @return resource + */ + public function search(string $baseDn, string $filter, array $attributes = null) + { + return ldap_search($this->connection, $baseDn, $filter, $attributes); + } + + /** + * Get entries from an ldap search result. + * + * @param resource $ldapSearchResult + * @return array|false + */ + public function getEntries($ldapSearchResult) + { + return ldap_get_entries($this->connection, $ldapSearchResult); + } + + /** + * Search and get entries immediately. + * + * @return array|false + */ + public function searchAndGetEntries(string $baseDn, string $filter, array $attributes = null) + { + $search = $this->search($baseDn, $filter, $attributes); + + return $this->getEntries($search); + } + + /** + * Bind to LDAP directory. + * + * @throws ErrorException + */ + public function bind(string $bindRdn = null, string $bindPassword = null): bool + { + return ldap_bind($this->connection, $bindRdn, $bindPassword); + } + + /** + * Explode a LDAP dn string into an array of components. + * + * @return array|false + */ + public static function explodeDn(string $dn, int $withAttrib) + { + return ldap_explode_dn($dn, $withAttrib); + } + + /** + * Escape a string for use in an LDAP filter. + */ + public static function escape(string $value, string $ignore = '', int $flags = 0): string + { + return ldap_escape($value, $ignore, $flags); + } + + /** + * Set a non-connection-specific LDAP option. + * @param mixed $value + */ + public static function setGlobalOption(int $option, $value): bool + { + return ldap_set_option(null, $option, $value); + } +} diff --git a/app/Auth/Access/Ldap/LdapConnectionManager.php b/app/Auth/Access/Ldap/LdapConnectionManager.php new file mode 100644 index 000000000..e5720a49a --- /dev/null +++ b/app/Auth/Access/Ldap/LdapConnectionManager.php @@ -0,0 +1,155 @@ +startBind($dn ?: null, $pass ?: null, $config); + } catch (LdapFailedBindException $exception) { + $msg = ($isAnonymous ? trans('errors.ldap_fail_anonymous') : trans('errors.ldap_fail_authed')); + throw new LdapFailedBindException($msg); + } + } + + /** + * Attempt to start and bind to a new LDAP connection. + * Will attempt against multiple defined fail-over hosts if set in the provided config. + * + * Throws a LdapFailedBindException error if the bind connected but failed. + * Otherwise, generic LdapException errors would be thrown. + * + * @throws LdapException + */ + public function startBind(?string $dn, ?string $password, array $config): LdapConnection + { + // Check LDAP extension in installed + if (!function_exists('ldap_connect') && config('app.env') !== 'testing') { + throw new LdapException(trans('errors.ldap_extension_not_installed')); + } + + // Disable certificate verification. + // This option works globally and must be set before a connection is created. + if ($config['tls_insecure']) { + LdapConnection::setGlobalOption(LDAP_OPT_X_TLS_REQUIRE_CERT, LDAP_OPT_X_TLS_NEVER); + } + + $serverDetails = $this->parseMultiServerString($config['server']); + $lastException = null; + + foreach ($serverDetails as $server) { + try { + $connection = $this->startServerConnection($server['host'], $server['port'], $config); + } catch (LdapException $exception) { + $lastException = $exception; + continue; + } + + try { + $bound = $connection->bind($dn, $password); + if (!$bound) { + throw new LdapFailedBindException('Failed to perform LDAP bind'); + } + } catch (ErrorException $exception) { + Log::error('LDAP bind error: ' . $exception->getMessage()); + $lastException = new LdapException('Encountered error during LDAP bind'); + continue; + } + + $this->connectionCache[$server['host'] . ':' . $server['port']] = $connection; + return $connection; + } + + throw $lastException; + } + + /** + * Attempt to start a server connection from the provided details. + * @throws LdapException + */ + protected function startServerConnection(string $host, int $port, array $config): LdapConnection + { + if (isset($this->connectionCache[$host . ':' . $port])) { + return $this->connectionCache[$host . ':' . $port]; + } + + $ldapConnection = new LdapConnection($host, $port); + + if (!$ldapConnection) { + throw new LdapException(trans('errors.ldap_cannot_connect')); + } + + // Set any required options + if ($config['version']) { + $ldapConnection->setVersion($config['version']); + } + + // Start and verify TLS if it's enabled + if ($config['start_tls']) { + try { + $tlsStarted = $ldapConnection->startTls(); + } catch (ErrorException $exception) { + $tlsStarted = false; + } + + if (!$tlsStarted) { + throw new LdapException('Could not start TLS connection'); + } + } + + return $ldapConnection; + } + + /** + * Parse a potentially multi-value LDAP server host string and return an array of host/port detail pairs. + * Multiple hosts are separated with a semicolon, for example: 'ldap.example.com:8069;ldaps://ldap.example.com' + * + * @return array + */ + protected function parseMultiServerString(string $serversString): array + { + $serverStringList = explode(';', $serversString); + + return array_map(fn ($serverStr) => $this->parseSingleServerString($serverStr), $serverStringList); + } + + /** + * Parse an 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'. + * + * @return array{host: string, port: int} + */ + protected function parseSingleServerString(string $serverString): array + { + $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]; + } +} diff --git a/app/Auth/Access/LdapService.php b/app/Auth/Access/Ldap/LdapService.php similarity index 53% rename from app/Auth/Access/LdapService.php rename to app/Auth/Access/Ldap/LdapService.php index 9ae6350c3..502e6099a 100644 --- a/app/Auth/Access/LdapService.php +++ b/app/Auth/Access/Ldap/LdapService.php @@ -1,13 +1,13 @@ ldap = $ldap; $this->userAvatars = $userAvatars; $this->groupSyncService = $groupSyncService; $this->config = config('services.ldap'); - $this->enabled = config('auth.method') === 'ldap'; - } - - /** - * Check if groups should be synced. - */ - public function shouldSyncGroups(): bool - { - return $this->enabled && $this->config['user_to_groups'] !== false; } /** @@ -50,7 +37,7 @@ class LdapService */ protected function getUserWithAttributes(string $userName, array $attributes): ?array { - $ldapConnection = $this->bindConnection(); + $connection = $this->ldap->startSystemBind($this->config); // Clean attributes foreach ($attributes as $index => $attribute) { @@ -64,8 +51,8 @@ class LdapService $baseDn = $this->config['base_dn']; $followReferrals = $this->config['follow_referrals'] ? 1 : 0; - $this->ldap->setOption($ldapConnection, LDAP_OPT_REFERRALS, $followReferrals); - $users = $this->ldap->searchAndGetEntries($ldapConnection, $baseDn, $userFilter, $attributes); + $connection->setOption(LDAP_OPT_REFERRALS, $followReferrals); + $users = $connection->searchAndGetEntries($baseDn, $userFilter, $attributes); if ($users['count'] === 0) { return null; } @@ -151,7 +138,7 @@ class LdapService } try { - $this->bindConnection($ldapUserDetails['dn'], $password); + $this->ldap->startBind($ldapUserDetails['dn'], $password, $this->config); } catch (LdapFailedBindException $e) { return false; } catch (LdapException $e) { @@ -161,179 +148,6 @@ class LdapService return true; } - - /** - * Attempted to start and bind to a new LDAP connection. - * Will attempt against multiple defined fail-over hosts if set. - * - * Throws a LdapFailedBindException error if the bind connected but failed. - * Otherwise, generic LdapException errors would be thrown. - * - * @return resource - * @throws LdapException - */ - protected function bindConnection(string $dn = null, string $password = null) - { - $systemBind = ($dn === null && $password === null); - - // Check LDAP extension in installed - if (!function_exists('ldap_connect') && config('app.env') !== 'testing') { - throw new LdapException(trans('errors.ldap_extension_not_installed')); - } - - // Disable certificate verification. - // This option works globally and must be set before a connection is created. - if ($this->config['tls_insecure']) { - $this->ldap->setOption(null, LDAP_OPT_X_TLS_REQUIRE_CERT, LDAP_OPT_X_TLS_NEVER); - } - - $serverDetails = $this->parseMultiServerString($this->config['server']); - $lastException = null; - - foreach ($serverDetails as $server) { - try { - $connection = $this->startServerConnection($server); - } catch (LdapException $exception) { - $lastException = $exception; - continue; - } - - try { - if ($systemBind) { - $this->bindSystemUser($connection); - } else { - $this->bindGivenUser($connection, $dn, $password); - } - } catch (LdapFailedBindException $exception) { - // Rethrow simply to indicate the importance of handling this exception case - // to indicate auth status. We skip past attempting fail-over hosts in this case since it's - // likely the connection worked here but the bind was unauthorised. - throw $exception; - } catch (ErrorException $exception) { - Log::error('LDAP bind error: ' . $exception->getMessage()); - $lastException = new LdapException('Encountered error during LDAP bind'); - continue; - } - - return $connection; - } - - throw $lastException; - } - - /** - * Bind to the given LDAP connection using the given credentials. - * MUST throw an exception on failure. - * - * @param resource $connection - * - * @throws LdapFailedBindException - */ - protected function bindGivenUser($connection, string $dn = null, string $password = null): void - { - $ldapBind = $this->ldap->bind($connection, $dn, $password); - - if (!$ldapBind) { - throw new LdapFailedBindException('Failed to bind with given user details'); - } - } - - /** - * Bind the system user to the LDAP connection using the configured credentials otherwise anonymous - * access is attempted. MUST throw an exception on failure. - * - * @param resource $connection - * - * @throws LdapFailedBindException - */ - protected function bindSystemUser($connection): void - { - $ldapDn = $this->config['dn']; - $ldapPass = $this->config['pass']; - - $isAnonymous = ($ldapDn === false || $ldapPass === false); - if ($isAnonymous) { - $ldapBind = $this->ldap->bind($connection); - } else { - $ldapBind = $this->ldap->bind($connection, $ldapDn, $ldapPass); - } - - if (!$ldapBind) { - throw new LdapFailedBindException(($isAnonymous ? trans('errors.ldap_fail_anonymous') : trans('errors.ldap_fail_authed'))); - } - } - - /** - * Attempt to start a server connection from the provided details. - * - * @param array{host: string, port: int} $serverDetail - * @return resource - * @throws LdapException - */ - protected function startServerConnection(array $serverDetail) - { - $ldapConnection = $this->ldap->connect($serverDetail['host'], $serverDetail['port']); - - if (!$ldapConnection) { - throw new LdapException(trans('errors.ldap_cannot_connect')); - } - - // Set any required options - if ($this->config['version']) { - $this->ldap->setVersion($ldapConnection, $this->config['version']); - } - - // Start and verify TLS if it's enabled - if ($this->config['start_tls']) { - try { - $tlsStarted = $this->ldap->startTls($ldapConnection); - } catch (ErrorException $exception) { - $tlsStarted = false; - } - - if (!$tlsStarted) { - throw new LdapException('Could not start TLS connection'); - } - } - - return $ldapConnection; - } - - /** - * Parse a potentially multi-value LDAP server host string and return an array of host/port detail pairs. - * Multiple hosts are separated with a semicolon, for example: 'ldap.example.com:8069;ldaps://ldap.example.com' - * - * @return array - */ - protected function parseMultiServerString(string $serversString): array - { - $serverStringList = explode(';', $serversString); - - return array_map(fn ($serverStr) => $this->parseSingleServerString($serverStr), $serverStringList); - } - - /** - * Parse an 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'. - * - * @return array{host: string, port: int} - */ - protected function parseSingleServerString(string $serverString): array - { - $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. */ @@ -342,7 +156,7 @@ class LdapService $newAttrs = []; foreach ($attrs as $key => $attrText) { $newKey = '${' . $key . '}'; - $newAttrs[$newKey] = $this->ldap->escape($attrText); + $newAttrs[$newKey] = LdapConnection::escape($attrText); } return strtr($filterString, $newAttrs); @@ -411,16 +225,16 @@ class LdapService */ private function getGroupGroups(string $groupName): array { - $ldapConnection = $this->bindConnection(); + $connection = $this->ldap->startSystemBind($this->config); $followReferrals = $this->config['follow_referrals'] ? 1 : 0; - $this->ldap->setOption($ldapConnection, LDAP_OPT_REFERRALS, $followReferrals); + $connection->setOption(LDAP_OPT_REFERRALS, $followReferrals); $baseDn = $this->config['base_dn']; $groupsAttr = strtolower($this->config['group_attribute']); - $groupFilter = 'CN=' . $this->ldap->escape($groupName); - $groups = $this->ldap->searchAndGetEntries($ldapConnection, $baseDn, $groupFilter, [$groupsAttr]); + $groupFilter = 'CN=' . $connection->escape($groupName); + $groups = $connection->searchAndGetEntries($baseDn, $groupFilter, [$groupsAttr]); if ($groups['count'] === 0) { return []; } @@ -443,7 +257,7 @@ class LdapService } for ($i = 0; $i < $count; $i++) { - $dnComponents = $this->ldap->explodeDn($userGroupSearchResponse[$groupsAttr][$i], 1); + $dnComponents = LdapConnection::explodeDn($userGroupSearchResponse[$groupsAttr][$i], 1); if (!in_array($dnComponents[0], $ldapGroups)) { $ldapGroups[] = $dnComponents[0]; } @@ -464,13 +278,21 @@ class LdapService $this->groupSyncService->syncUserWithFoundGroups($user, $userLdapGroups, $this->config['remove_from_groups']); } + /** + * Check if groups should be synced. + */ + public function shouldSyncGroups(): bool + { + return $this->config['user_to_groups'] !== false; + } + /** * Save and attach an avatar image, if found in the ldap details, and attach * to the given user model. */ public function saveAndAttachAvatar(User $user, array $ldapUserDetails): void { - if (is_null(config('services.ldap.thumbnail_attribute')) || is_null($ldapUserDetails['avatar'])) { + if (is_null($this->config['thumbnail_attribute']) || is_null($ldapUserDetails['avatar'])) { return; } diff --git a/app/Providers/AuthServiceProvider.php b/app/Providers/AuthServiceProvider.php index 5e16179ab..37ab066eb 100644 --- a/app/Providers/AuthServiceProvider.php +++ b/app/Providers/AuthServiceProvider.php @@ -6,7 +6,7 @@ use BookStack\Api\ApiTokenGuard; use BookStack\Auth\Access\ExternalBaseUserProvider; use BookStack\Auth\Access\Guards\AsyncExternalBaseSessionGuard; use BookStack\Auth\Access\Guards\LdapSessionGuard; -use BookStack\Auth\Access\LdapService; +use BookStack\Auth\Access\Ldap\LdapService; use BookStack\Auth\Access\LoginService; use BookStack\Auth\Access\RegistrationService; use Illuminate\Support\Facades\Auth; diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php index 6ee5e6f81..1c575ec86 100644 --- a/tests/Auth/LdapTest.php +++ b/tests/Auth/LdapTest.php @@ -2,8 +2,9 @@ namespace Tests\Auth; -use BookStack\Auth\Access\Ldap; -use BookStack\Auth\Access\LdapService; +use BookStack\Auth\Access\Ldap\LdapConnection; +use BookStack\Auth\Access\Ldap\LdapConnectionManager; +use BookStack\Auth\Access\Ldap\LdapService; use BookStack\Auth\Role; use BookStack\Auth\User; use Illuminate\Testing\TestResponse; @@ -23,9 +24,11 @@ class LdapTest extends TestCase protected function setUp(): void { parent::setUp(); + if (!defined('LDAP_OPT_REFERRALS')) { define('LDAP_OPT_REFERRALS', 1); } + config()->set([ 'auth.method' => 'ldap', 'auth.defaults.guard' => 'ldap', @@ -41,8 +44,12 @@ class LdapTest extends TestCase 'services.ldap.tls_insecure' => false, 'services.ldap.thumbnail_attribute' => null, ]); - $this->mockLdap = \Mockery::mock(Ldap::class); - $this->app[Ldap::class] = $this->mockLdap; + + $lcm = $this->mock(LdapConnectionManager::class); + // TODO - Properly mock + + $this->mockLdap = \Mockery::mock(LdapConnection::class); + $this->app[LdapConnection::class] = $this->mockLdap; $this->mockUser = User::factory()->make(); } @@ -81,7 +88,6 @@ class LdapTest extends TestCase */ protected function commonLdapMocks(int $connects = 1, int $versions = 1, int $options = 2, int $binds = 4, int $escapes = 2, int $explodes = 0) { - $this->mockLdap->shouldReceive('connect')->times($connects)->andReturn($this->resourceId); $this->mockLdap->shouldReceive('setVersion')->times($versions); $this->mockLdap->shouldReceive('setOption')->times($options); $this->mockLdap->shouldReceive('bind')->times($binds)->andReturn(true);