From b04ad353fb1576f028887b61496cb05ed0fdb7ae Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Thu, 15 Oct 2026 16:23:13 +0100 Subject: [PATCH] LDAP: Migrated code to use classes instead of resource Added readAndGetEntries, and made those individual methods protected, in the LDAP class since these were always used together, and this limits exposure of internal opaque LDAP values/classes. Closes #6203 --- app/Access/Ldap.php | 53 ++++++++++++++---------------- app/Access/LdapService.php | 22 +++++-------- composer.json | 3 ++ tests/Auth/LdapTest.php | 67 +++++++++++++++++++------------------- 4 files changed, 68 insertions(+), 77 deletions(-) diff --git a/app/Access/Ldap.php b/app/Access/Ldap.php index 54d2af72d..9eb01734a 100644 --- a/app/Access/Ldap.php +++ b/app/Access/Ldap.php @@ -2,6 +2,9 @@ namespace BookStack\Access; +use LDAP\Connection; +use LDAP\Result; + /** * Class Ldap * An object-orientated thin abstraction wrapper for common PHP LDAP functions. @@ -11,20 +14,16 @@ class Ldap { /** * Connect to an LDAP server. - * - * @return resource|\LDAP\Connection|false */ - public function connect(string $hostName) + public function connect(string $hostName): Connection|false { return ldap_connect($hostName); } /** * Set the value of an LDAP option for the given connection. - * - * @param resource|\LDAP\Connection|null $ldapConnection */ - public function setOption($ldapConnection, int $option, mixed $value): bool + public function setOption(Connection|null $ldapConnection, int $option, mixed $value): bool { return ldap_set_option($ldapConnection, $option, $value); } @@ -32,17 +31,15 @@ class Ldap /** * Start TLS on the given LDAP connection. */ - public function startTls($ldapConnection): bool + public function startTls(Connection $ldapConnection): bool { return ldap_start_tls($ldapConnection); } /** * Set the version number for the given LDAP connection. - * - * @param resource|\LDAP\Connection $ldapConnection */ - public function setVersion($ldapConnection, int $version): bool + public function setVersion(Connection $ldapConnection, int $version): bool { return $this->setOption($ldapConnection, LDAP_OPT_PROTOCOL_VERSION, $version); } @@ -50,11 +47,9 @@ class Ldap /** * Search the LDAP tree using the provided filter. * - * @param resource|\LDAP\Connection $ldapConnection - * - * @return \LDAP\Result|array|false + * @return Result|Result[]|false */ - public function search($ldapConnection, string $baseDn, string $filter, array $attributes = []) + public function search(Connection $ldapConnection, string $baseDn, string $filter, array $attributes = []) { return ldap_search($ldapConnection, $baseDn, $filter, $attributes); } @@ -62,32 +57,34 @@ class Ldap /** * Read an entry from the LDAP tree. * - * @param resource|\Ldap\Connection $ldapConnection - * - * @return \LDAP\Result|array|false + * @return Result|false */ - public function read($ldapConnection, string $baseDn, string $filter, array $attributes = []) + protected function read(Connection $ldapConnection, string $baseDn, string $filter, array $attributes = []) { return ldap_read($ldapConnection, $baseDn, $filter, $attributes); } /** * Get entries from an LDAP search result. - * - * @param resource|\LDAP\Connection $ldapConnection - * @param resource|\LDAP\Result $ldapSearchResult */ - public function getEntries($ldapConnection, $ldapSearchResult): array|false + protected function getEntries(Connection $ldapConnection, Result $ldapSearchResult): array|false { return ldap_get_entries($ldapConnection, $ldapSearchResult); } /** - * Search and get entries immediately. - * - * @param resource|\LDAP\Connection $ldapConnection + * Read from the LDAP system, and get the entries immediately. */ - public function searchAndGetEntries($ldapConnection, string $baseDn, string $filter, array $attributes = []): array|false + public function readAndGetEntries(Connection $ldapConnection, string $baseDn, string $filter, array $attributes = []): array|false + { + $read = $this->read($ldapConnection, $baseDn, $filter, $attributes); + return $this->getEntries($ldapConnection, $read); + } + + /** + * Search and get entries immediately. + */ + public function searchAndGetEntries(Connection $ldapConnection, string $baseDn, string $filter, array $attributes = []): array|false { $search = $this->search($ldapConnection, $baseDn, $filter, $attributes); @@ -96,10 +93,8 @@ class Ldap /** * Bind to the LDAP directory. - * - * @param resource|\LDAP\Connection $ldapConnection */ - public function bind($ldapConnection, ?string $bindRdn = null, ?string $bindPassword = null): bool + public function bind(Connection $ldapConnection, ?string $bindRdn = null, ?string $bindPassword = null): bool { return ldap_bind($ldapConnection, $bindRdn, $bindPassword); } diff --git a/app/Access/LdapService.php b/app/Access/LdapService.php index 648262508..218bf1a0c 100644 --- a/app/Access/LdapService.php +++ b/app/Access/LdapService.php @@ -7,7 +7,9 @@ use BookStack\Exceptions\LdapException; use BookStack\Uploads\UserAvatars; use BookStack\Users\Models\User; use ErrorException; +use Exception; use Illuminate\Support\Facades\Log; +use LDAP\Connection; /** * Class LdapService @@ -15,10 +17,7 @@ use Illuminate\Support\Facades\Log; */ class LdapService { - /** - * @var resource|\LDAP\Connection - */ - protected $ldapConnection; + protected Connection|null $ldapConnection = null; protected array $config; protected bool $enabled; @@ -187,11 +186,9 @@ class LdapService * Bind the system user to the LDAP connection using the given credentials * otherwise anonymous access is attempted. * - * @param resource|\LDAP\Connection $connection - * * @throws LdapException */ - protected function bindSystemUser($connection): void + protected function bindSystemUser(Connection $connection): void { $ldapDn = $this->config['dn']; $ldapPass = $this->config['pass']; @@ -213,10 +210,8 @@ class LdapService * Creates a new connection if one does not exist. * * @throws LdapException - * - * @return resource|\LDAP\Connection */ - protected function getConnection() + protected function getConnection(): Connection { if ($this->ldapConnection !== null) { return $this->ldapConnection; @@ -255,7 +250,7 @@ class LdapService if ($this->config['start_tls']) { try { $started = $this->ldap->startTls($ldapConnection); - } catch (\Exception $exception) { + } catch (Exception $exception) { $error = $exception->getMessage() . ' :: ' . ldap_error($ldapConnection); ldap_get_option($ldapConnection, LDAP_OPT_DIAGNOSTIC_MESSAGE, $detail); Log::info("LDAP STARTTLS failure: {$error} {$detail}"); @@ -431,8 +426,7 @@ class LdapService $followReferrals = $this->config['follow_referrals'] ? 1 : 0; $this->ldap->setOption($ldapConnection, LDAP_OPT_REFERRALS, $followReferrals); - $read = $this->ldap->read($ldapConnection, $groupDN, '(objectClass=*)', [$groupsAttr]); - $results = $this->ldap->getEntries($ldapConnection, $read); + $results = $this->ldap->readAndGetEntries($ldapConnection, $groupDN, '(objectClass=*)', [$groupsAttr]); if ($results['count'] === 0) { return []; } @@ -488,7 +482,7 @@ class LdapService try { $imageData = $ldapUserDetails['avatar']; $this->userAvatars->assignToUserFromExistingData($user, $imageData, 'jpg'); - } catch (\Exception $exception) { + } catch (Exception) { Log::info("Failed to use avatar image from LDAP data for user id {$user->id}"); } } diff --git a/composer.json b/composer.json index 696a37db4..88f32a0bb 100644 --- a/composer.json +++ b/composer.json @@ -51,6 +51,9 @@ "squizlabs/php_codesniffer": "^4.0.1", "ssddanbrown/asserthtml": "^3.1" }, + "suggest": { + "ext-ldap": "*" + }, "autoload": { "psr-4": { "BookStack\\": "app/", diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php index 9bf15ae2b..0b1f1d852 100644 --- a/tests/Auth/LdapTest.php +++ b/tests/Auth/LdapTest.php @@ -8,6 +8,7 @@ use BookStack\Exceptions\LdapException; use BookStack\Users\Models\Role; use BookStack\Users\Models\User; use Illuminate\Testing\TestResponse; +use LDAP\Connection; use Mockery\MockInterface; use Tests\TestCase; @@ -18,6 +19,8 @@ class LdapTest extends TestCase protected User $mockUser; protected string $resourceId = 'resource-test'; + protected Connection $connection; + protected function setUp(): void { parent::setUp(); @@ -78,7 +81,8 @@ 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, int $groups = 0): void { - $this->mockLdap->shouldReceive('connect')->times($connects)->andReturn($this->resourceId); + $this->connection = ldap_connect('example.com'); + $this->mockLdap->shouldReceive('connect')->times($connects)->andReturn($this->connection); $this->mockLdap->shouldReceive('setVersion')->times($versions); $this->mockLdap->shouldReceive('setOption')->times($options); $this->mockLdap->shouldReceive('bind')->times($binds)->andReturn(true); @@ -89,15 +93,14 @@ class LdapTest extends TestCase protected function mockGroupLookups(int $times = 1): void { - $this->mockLdap->shouldReceive('read')->times($times)->andReturn(['count' => 0]); - $this->mockLdap->shouldReceive('getEntries')->times($times)->andReturn(['count' => 0]); + $this->mockLdap->shouldReceive('readAndGetEntries')->times($times)->andReturn(['count' => 0]); } public function test_login() { $this->commonLdapMocks(1, 1, 2, 4, 2); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2) - ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) + ->with($this->connection, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ 'uid' => [$this->mockUser->name], 'cn' => [$this->mockUser->name], @@ -128,7 +131,7 @@ class LdapTest extends TestCase $this->commonLdapMocks(1, 1, 2, 4, 2); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2) - ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) + ->with($this->connection, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ 'uid' => [$this->mockUser->name], 'cn' => [$this->mockUser->name], @@ -153,7 +156,7 @@ class LdapTest extends TestCase $this->commonLdapMocks(1, 1, 1, 2, 1); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1) - ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) + ->with($this->connection, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ 'cn' => [$this->mockUser->name], 'dn' => $ldapDn, @@ -172,7 +175,7 @@ class LdapTest extends TestCase $this->commonLdapMocks(1, 1, 1, 2, 1); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1) - ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) + ->with($this->connection, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ 'dn' => $ldapDn, 'mail' => [$this->mockUser->email], @@ -193,7 +196,7 @@ class LdapTest extends TestCase $this->commonLdapMocks(1, 1, 1, 2, 1); $ldapDn = 'cn=test-user,dc=test' . config('services.ldap.base_dn'); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1) - ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) + ->with($this->connection, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ 'cn' => [$this->mockUser->name], 'dn' => $ldapDn, @@ -221,7 +224,7 @@ class LdapTest extends TestCase $this->commonLdapMocks(1, 1, 1, 2, 1); $ldapDn = 'cn=test-user,dc=test' . config('services.ldap.base_dn'); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1) - ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) + ->with($this->connection, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ 'cn' => [$this->mockUser->name], 'dn' => $ldapDn, @@ -246,7 +249,7 @@ class LdapTest extends TestCase $this->commonLdapMocks(1, 1, 1, 1, 1); $this->mockLdap->shouldReceive('searchAndGetEntries') ->once() - ->with($this->resourceId, config('services.ldap.base_dn'), $expectedFilter, \Mockery::type('array')) + ->with($this->connection, config('services.ldap.base_dn'), $expectedFilter, \Mockery::type('array')) ->andReturn(['count' => 0, 0 => []]); $resp = $this->mockUserLogin(); @@ -262,7 +265,7 @@ class LdapTest extends TestCase $this->commonLdapMocks(1, 1, 1, 1, 1); $this->mockLdap->shouldReceive('searchAndGetEntries') ->once() - ->with($this->resourceId, config('services.ldap.base_dn'), $expectedFilter, \Mockery::type('array')) + ->with($this->connection, config('services.ldap.base_dn'), $expectedFilter, \Mockery::type('array')) ->andReturn(['count' => 0, 0 => []]); $resp = $this->mockUserLogin(); @@ -273,7 +276,7 @@ class LdapTest extends TestCase { $this->commonLdapMocks(1, 1, 1, 0, 1); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1) - ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) + ->with($this->connection, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ 'uid' => [$this->mockUser->name], 'cn' => [$this->mockUser->name], @@ -291,7 +294,7 @@ class LdapTest extends TestCase { $this->commonLdapMocks(1, 1, 1, 1, 1); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1) - ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) + ->with($this->connection, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 0]); $resp = $this->mockUserLogin(); @@ -366,7 +369,7 @@ class LdapTest extends TestCase $this->commonLdapMocks(1, 1, 4, 5, 2, 4, 2); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2) - ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) + ->with($this->connection, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ 'uid' => [$this->mockUser->name], 'cn' => [$this->mockUser->name], @@ -411,7 +414,7 @@ class LdapTest extends TestCase $this->commonLdapMocks(1, 1, 3, 4, 2, 2, 1); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2) - ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) + ->with($this->connection, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ 'uid' => [$this->mockUser->name], 'cn' => [$this->mockUser->name], @@ -453,7 +456,7 @@ class LdapTest extends TestCase ]]; $this->commonLdapMocks(1, 1, 4, 5, 2, 4, 0); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2) - ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) + ->with($this->connection, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn($userResp, ['count' => 1, 0 => [ 'dn' => 'dc=test,' . config('services.ldap.base_dn'), @@ -464,8 +467,7 @@ class LdapTest extends TestCase ], ]); - $this->mockLdap->shouldReceive('read')->times(2); - $this->mockLdap->shouldReceive('getEntries')->times(2) + $this->mockLdap->shouldReceive('readAndGetEntries')->times(2) ->andReturn([ 'count' => 1, 0 => [ @@ -527,14 +529,11 @@ class LdapTest extends TestCase $escapedName = ldap_escape($this->mockUser->name); $this->mockLdap->shouldReceive('searchAndGetEntries')->twice() - ->with($this->resourceId, config('services.ldap.base_dn'), "(&(uid={$escapedName}))", \Mockery::type('array')) + ->with($this->connection, config('services.ldap.base_dn'), "(&(uid={$escapedName}))", \Mockery::type('array')) ->andReturn($userResp, $groupResp); - $this->mockLdap->shouldReceive('read')->times(1) - ->with($this->resourceId, 'cn=ldaptester,ou=groups,dc=example,dc=com', '(objectClass=*)', ['memberof']) - ->andReturn(['count' => 0]); - $this->mockLdap->shouldReceive('getEntries')->times(1) - ->with($this->resourceId, ['count' => 0]) + $this->mockLdap->shouldReceive('readAndGetEntries')->times(1) + ->with($this->connection, 'cn=ldaptester,ou=groups,dc=example,dc=com', '(objectClass=*)', ['memberof']) ->andReturn(['count' => 0]); $resp = $this->mockUserLogin(); @@ -561,7 +560,7 @@ class LdapTest extends TestCase $this->commonLdapMocks(1, 1, 3, 4, 2, 2, 1); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2) - ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) + ->with($this->connection, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ 'uid' => [$this->mockUser->name], 'cn' => [$this->mockUser->name], @@ -602,7 +601,7 @@ class LdapTest extends TestCase $this->commonLdapMocks(1, 1, 4, 5, 2, 4, 2); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2) - ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) + ->with($this->connection, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ 'uid' => [$this->mockUser->name], 'cn' => [$this->mockUser->name], @@ -643,7 +642,7 @@ class LdapTest extends TestCase $this->commonLdapMocks(1, 1, 4, 5, 2, 6, 2); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2) - ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) + ->with($this->connection, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ 'uid' => [$this->mockUser->name], 'cn' => [$this->mockUser->name], @@ -674,7 +673,7 @@ class LdapTest extends TestCase $this->commonLdapMocks(1, 1, 2, 4, 2); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2) - ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) + ->with($this->connection, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ 'uid' => [$this->mockUser->name], 'cn' => [$this->mockUser->name], @@ -699,7 +698,7 @@ class LdapTest extends TestCase $this->commonLdapMocks(1, 1, 1, 2, 1); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1) - ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) + ->with($this->connection, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ 'uid' => [$this->mockUser->name], 'cn' => [$this->mockUser->name], @@ -726,7 +725,7 @@ class LdapTest extends TestCase $this->commonLdapMocks(1, 1, 2, 4, 2); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2) - ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) + ->with($this->connection, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ 'uid' => [$this->mockUser->name], 'cn' => [$this->mockUser->name], @@ -815,7 +814,7 @@ class LdapTest extends TestCase $this->commonLdapMocks(1, 1, 1, 1, 1); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1) - ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) + ->with($this->connection, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ 'uid' => [$this->mockUser->name], 'cn' => [$this->mockUser->name], @@ -856,7 +855,7 @@ class LdapTest extends TestCase $ldapService = app()->make(LdapService::class); $this->commonLdapMocks(1, 1, 1, 1, 1); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1) - ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), ['cn', 'dn', 'uid', 'mail', 'cn']) + ->with($this->connection, config('services.ldap.base_dn'), \Mockery::type('string'), ['cn', 'dn', 'uid', 'mail', 'cn']) ->andReturn(['count' => 1, 0 => [ 'uid' => [hex2bin('FFF8F7')], 'cn' => [$this->mockUser->name], @@ -871,7 +870,7 @@ class LdapTest extends TestCase { $this->commonLdapMocks(1, 1, 2, 4, 2); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2) - ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) + ->with($this->connection, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ 'uid' => [$this->mockUser->name], 'cn' => [$this->mockUser->name], @@ -956,7 +955,7 @@ class LdapTest extends TestCase $this->commonLdapMocks(1, 1, 1, 2, 1); $ldapDn = 'cn=test-user,dc=test' . config('services.ldap.base_dn'); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1) - ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) + ->with($this->connection, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ 'cn' => [$this->mockUser->name], 'dn' => $ldapDn,