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,