webapp/bookstack
1
0
Fork 0
mirror of https://github.com/BookStackApp/BookStack.git synced 2026-10-23 06:49:01 +03:00
Code Activity

LDAP: Migrated code to use classes instead of resource

Browse Source 
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
This commit is contained in:
Dan Brown
2026-10-15 16:23:13 +01:00
parent f462704506
commit b04ad353fb
4 changed files with 68 additions and 77 deletions
Download Patch File Download Diff File Expand all files Collapse all files

53
app/Access/Ldap.php
View File

@@ -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);
}

22
app/Access/LdapService.php
View File

@@ -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}");
}
}

3
composer.json
View File

@@ -51,6 +51,9 @@
"squizlabs/php_codesniffer": "^4.0.1",
"ssddanbrown/asserthtml": "^3.1"
},
"suggest": {
"ext-ldap": "*"
},
"autoload": {
"psr-4": {
"BookStack\\": "app/",

67
tests/Auth/LdapTest.php
View File

@@ -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,