From 3f5dc10cd4cf901b44b1cf8c9e2626bf0425d488 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 30 May 2023 13:10:05 +0100 Subject: [PATCH] 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 | 70 ++++++++++--------------------- app/Access/LdapService.php | 54 +++++++++--------------- tests/Auth/LdapTest.php | 84 +++++++++++++++++--------------------- 3 files changed, 79 insertions(+), 129 deletions(-) diff --git a/app/Access/Ldap.php b/app/Access/Ldap.php index d245b3444..12a3d1e71 100644 --- a/app/Access/Ldap.php +++ b/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 mixed $value + * @param resource|\LDAP\Connection|null $ldapConnection */ - 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 string $baseDn - * @param string $filter - * @param array|null $attributes + * @param resource|\LDAP\Connection $ldapConnection * - * @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 $ldapSearchResult - * - * @return array + * @param resource|\LDAP\Connection $ldapConnection + * @param resource|\LDAP\Result $ldapSearchResult */ - 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 string $baseDn - * @param string $filter - * @param array|null $attributes - * - * @return resource + * @param resource|\LDAP\Connection $ldapConnection */ - 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 string $bindRdn - * @param string $bindPassword - * - * @return bool + * @param resource|\LDAP\Connection $ldapConnection */ - 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. - * - * @param string $dn - * @param int $withAttrib - * - * @return array + * Explode an LDAP dn string into an array of components. */ - 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); } diff --git a/app/Access/LdapService.php b/app/Access/LdapService.php index b127b931a..9d2667635 100644 --- a/app/Access/LdapService.php +++ b/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; - /** - * LdapService constructor. - */ - public function __construct(Ldap $ldap, UserAvatars $userAvatars, GroupSyncService $groupSyncService) - { - $this->ldap = $ldap; - $this->userAvatars = $userAvatars; - $this->groupSyncService = $groupSyncService; + public function __construct( + protected Ldap $ldap, + protected UserAvatars $userAvatars, + protected 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']); - $ldapConnection = $this->ldap->connect($serverDetails['host'], $serverDetails['port']); + $ldapHost = $this->parseServerString($this->config['server']); + $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 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]; + if (str_starts_with($serverString, 'ldaps://') || str_starts_with($serverString, 'ldap://')) { + return $serverString; } - // Otherwise, extract the port out - $hostName = $serverNameParts[0]; - $ldapPort = (count($serverNameParts) > 1) ? intval($serverNameParts[1]) : 389; - - return ['host' => $hostName, 'port' => $ldapPort]; + return "ldap://{$serverString}"; } /** @@ -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']); diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php index 97a91aba7..34900ce6f 100644 --- a/tests/Auth/LdapTest.php +++ b/tests/Auth/LdapTest.php @@ -12,13 +12,10 @@ use Tests\TestCase; class LdapTest extends TestCase { - /** - * @var MockInterface - */ - protected $mockLdap; + protected MockInterface $mockLdap; - protected $mockUser; - protected $resourceId = 'resource-test'; + protected User $mockUser; + 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->app[Ldap::class] = $this->mockLdap; + $this->mockLdap = $this->mock(Ldap::class); $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([ - 'services.ldap.server' => $serverString, - ]); + app('config')->set(['services.ldap.server' => $serverString]); - // Standard mocks - $this->commonLdapMocks(0, 1, 1, 2, 1); - $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1)->andReturn(['count' => 1, 0 => [ - 'uid' => [$this->mockUser->name], - 'cn' => [$this->mockUser->name], - 'dn' => ['dc=test' . config('services.ldap.base_dn')], - ]]); + $this->mockLdap->shouldReceive('connect') + ->once() + ->with($expectedHostString) + ->andReturn(false); - $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'; - $this->checkLdapReceivesCorrectDetails($hostName, $hostName, 389); - } + $expectedResultByInput = [ + '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() - { - $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); + foreach ($expectedResultByInput as $input => $expectedResult) { + $this->checkLdapReceivesCorrectDetails($input, $expectedResult); + $this->refreshApplication(); + $this->setUp(); + } } 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,