Skip to content

refactor(ldap)!: switch LDAP server config from host/port to uri #983

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
JohnVillalovos wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

refactor(ldap)!: switch LDAP server config from host/port to uri #983

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 34 additions & 2 deletions docs/source/LDAP-Authentication.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,46 @@ If not existing already, copy the template and edit with your LDAP settings:

The configuration file at ``/config/Ldap.config.php`` contains all available options with detailed comments explaining each setting. You can also view and modify these settings through the web admin interface at **Application Management > Configuration**. Key settings include:

- **host**: LDAP server URL(s)
- **uri**: LDAP URI string. For multiple servers, use a space-separated list of URIs.
- **binddn/bindpw**: Service account credentials for directory searches
- **basedn**: Base DN where users are located
- **user.id.attribute**: LDAP attribute for username lookup (typically ``uid``)
- **attribute.mapping**: Maps LDAP attributes to LibreBooking user fields
- **sync.groups**: Enable group membership synchronization
- **database.auth.when.ldap.user.not.found**: Fallback to database authentication

URI examples:

.. code-block:: php

// single LDAP server (unencrypted LDAP, explicit port)
'uri' => 'ldap://ldap1.example.com:389',

// single LDAP server (unencrypted LDAP, default port 389)
'uri' => 'ldap://ldap1.example.com',

// single LDAP server over LDAPS (TLS, explicit port)
'uri' => 'ldaps://ldap1.example.com:636',

// single LDAP server over LDAPS (TLS, default port 636)
'uri' => 'ldaps://ldap1.example.com',

// multiple LDAP servers (space-separated URIs in one string)
'uri' => 'ldap://ldap1.example.com:389 ldap://ldap2.example.com:389',

// multiple LDAPS servers
'uri' => 'ldaps://ldap1.example.com:636 ldaps://ldap2.example.com:636',

Port defaults:

- ``ldap://`` uses port ``389`` by default when no port is specified.
- ``ldaps://`` uses port ``636`` by default when no port is specified.

Breaking change:

- ``host`` and ``port`` are no longer supported.
- Configure LDAP endpoints only through ``uri``.

Alternatively, configure the plugin through the web admin interface at **Application Configuration** (``/Web/admin/manage_configuration.php``) and select **Authentification-Ldap**.
Refer to ``/plugins/Authentication/Ldap/Ldap.config.dist.php`` for complete documentation of all options.

Expand All @@ -75,7 +107,7 @@ Common Issues
~~~~~~~~~~~~~

**Connection failures**
- Verify server hostname and port accessibility
- Verify LDAP URI hostname and port accessibility
- Check firewall rules
- Test with ``telnet ldap.example.com 389``

Expand Down
15 changes: 10 additions & 5 deletions plugins/Authentication/Ldap/Ldap.config.dist.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,16 @@
return [
'settings' => [
'ldap' => [
// comma separated list of ldap servers such as ldap://mydomain1,ldap://localhost
'host' => 'ldap://localhost',

// default ldap port 389 or 636 for ssl.
'port' => 389,
// LDAP URI(s). For multiple servers, separate each URI with spaces.
// If no port is specified, defaults are: ldap:// = 389, ldaps:// = 636.
// examples:
// 'ldap://ldap1.example.com'
// 'ldap://ldap1.example.com:389'
// 'ldaps://ldap1.example.com'
// 'ldaps://ldap1.example.com:636'
// 'ldap://ldap1.example.com:389 ldap://ldap2.example.com:389'
// 'ldaps://ldap1.example.com:636 ldaps://ldap2.example.com:636'
'uri' => 'ldap://localhost:389',

// LDAP protocol version
'version' => 3,
Expand Down
17 changes: 4 additions & 13 deletions plugins/Authentication/Ldap/LdapConfigKeys.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,12 @@ class LdapConfigKeys extends PluginConfigKeys
{
public const CONFIG_ID = 'ldap';

public const HOST = [
'key' => 'host',
public const URI = [
'key' => 'uri',
'type' => 'string',
'default' => '',
'label' => 'LDAP Host',
'description' => 'Hostname or IP address of LDAP server. Should start with ldap:// or ldaps://',
'section' => 'ldap'
];

public const PORT = [
'key' => 'port',
'type' => 'integer',
'default' => 389,
'label' => 'LDAP Port',
'description' => 'Port of LDAP server (usually 389 or 636 for SSL)',
'label' => 'LDAP server URI(s)',
'description' => 'LDAP server URI(s). Use ldap:// or ldaps://. For multiple servers, separate URIs with spaces.',
'section' => 'ldap'
];

Expand Down
60 changes: 51 additions & 9 deletions plugins/Authentication/Ldap/LdapOptions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@
class LdapOptions
{
private $_options = [];
/**
* @var array<string, mixed>
*/
private $rawLdapSettings = [];

public function __construct()
{
Expand All @@ -13,7 +17,10 @@ public function __construct()
$configPath = dirname(__FILE__) . '/Ldap.config.dist.php';
}

require_once($configPath);
$loadedConfig = @require $configPath;
if (is_array($loadedConfig) && isset($loadedConfig['settings']['ldap']) && is_array($loadedConfig['settings']['ldap'])) {
$this->rawLdapSettings = $loadedConfig['settings']['ldap'];
}
Comment on lines +20 to +23
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__construct() loads the config file via @require $configPath and then immediately calls Configuration::Instance()->Register($configPath, ...), which itself @requires the same file again. This double-executes any code in Ldap.config.php (especially problematic for legacy-style configs that mutate $conf or define constants), and makes config loading harder to reason about. Consider avoiding the pre-require and instead capturing the raw settings from the same load path used by Register() (e.g., refactor Register() to return the loaded array, or add a method to access the loaded settings for this config id).

Copilot uses AI. Check for mistakes.

Configuration::Instance()->Register(
$configPath,
Expand All @@ -26,9 +33,8 @@ public function __construct()

public function Ldap2Config()
{
$hosts = $this->GetHosts();
$hosts = $this->GetUris();
$this->SetOption('host', $hosts);
$this->SetOption('port', $this->GetConfig(LdapConfigKeys::PORT, new IntConverter()));
$this->SetOption('starttls', $this->GetConfig(LdapConfigKeys::STARTTLS, new BooleanConverter()));
$this->SetOption('version', $this->GetConfig(LdapConfigKeys::VERSION, new IntConverter()));
$this->SetOption('binddn', $this->GetConfig(LdapConfigKeys::BINDDN));
Expand All @@ -47,7 +53,7 @@ public function RetryAgainstDatabase()

public function Controllers()
{
return $this->GetHosts();
return $this->GetUris();
}

private function SetOption($key, $value)
Expand All @@ -64,15 +70,51 @@ private function GetConfig($configDef, $converter = null)
return Configuration::Instance()->File(LdapConfigKeys::CONFIG_ID)->GetKey($configDef, $converter);
}

private function GetHosts()
private function GetUris()
{
$hosts = explode(',', $this->GetConfig(LdapConfigKeys::HOST));
$this->AssertLegacyHostPortNotConfigured();

for ($i = 0; $i < count($hosts); $i++) {
$hosts[$i] = trim($hosts[$i]);
$uriConfig = trim($this->GetConfig(LdapConfigKeys::URI));
if (empty($uriConfig)) {
throw new RuntimeException("LDAP setting 'uri' is required and must contain at least one ldap:// or ldaps:// URI.");
}

$uris = preg_split('/\s+/', $uriConfig) ?: [];
foreach ($uris as $uri) {
$scheme = parse_url($uri, PHP_URL_SCHEME);
$host = parse_url($uri, PHP_URL_HOST);

if (!in_array($scheme, ['ldap', 'ldaps'], true) || empty($host)) {
throw new RuntimeException(
sprintf("Invalid LDAP URI '%s'. Use ldap:// or ldaps:// with a hostname. For multiple servers, separate URIs with spaces.", $uri)
);
}
}

return $hosts;
return $uris;
}

private function AssertLegacyHostPortNotConfigured()
{
if (array_key_exists('host', $this->rawLdapSettings) || array_key_exists('port', $this->rawLdapSettings)) {
throw new RuntimeException("LDAP settings 'host' and 'port' have been removed. Use only the 'uri' setting.");
}

// Also enforce legacy key removal when values are provided via env vars or tests.
$legacyHost = trim($this->GetConfig([
'key' => 'host',
'section' => 'ldap',
'type' => 'string'
]));
$legacyPort = trim($this->GetConfig([
'key' => 'port',
'section' => 'ldap',
'type' => 'string'
]));

if (!empty($legacyHost) || !empty($legacyPort)) {
throw new RuntimeException("LDAP settings 'host' and 'port' have been removed. Use only the 'uri' setting.");
}
}

public function BaseDn()
Expand Down
49 changes: 40 additions & 9 deletions tests/Plugins/Authentication/Ldap/LdapTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,17 +184,15 @@ public function testDoesNotSyncIfUserWasNotFoundInLdap()

public function testConstructsOptionsCorrectly()
{
$hosts = 'localhost, localhost.2';
$port = '389';
$uris = 'ldap://localhost:389 ldap://localhost.2:389';
$binddn = 'cn=admin,ou=users,dc=example,dc=org';
$password = 'pw';
$base = 'dc=example,dc=org';
$starttls = 'false';
$version = '3';

$configFile = new FakeConfigFile();
$configFile->SetKey(LdapConfigKeys::HOST, $hosts);
$configFile->SetKey(LdapConfigKeys::PORT, $port);
$configFile->SetKey(LdapConfigKeys::URI, $uris);
$configFile->SetKey(LdapConfigKeys::BINDDN, $binddn);
$configFile->SetKey(LdapConfigKeys::BINDPW, $password);
$configFile->SetKey(LdapConfigKeys::BASEDN, $base);
Expand All @@ -207,8 +205,7 @@ public function testConstructsOptionsCorrectly()
$options = $ldapOptions->Ldap2Config();

$this->assertNotNull($this->fakeConfig->_RegisteredFiles[LdapConfigKeys::CONFIG_ID]);
$this->assertEquals('localhost', $options['host'][0], 'domain_controllers must be an array');
$this->assertEquals(intval($port), $options['port'], 'port should be int');
$this->assertEquals('ldap://localhost:389', $options['host'][0], 'controllers must be an array of URIs');
$this->assertEquals($binddn, $options['binddn']);
$this->assertEquals($password, $options['bindpw']);
$this->assertEquals($base, $options['basedn']);
Expand All @@ -218,15 +215,49 @@ public function testConstructsOptionsCorrectly()

public function testGetAllHosts()
{
$controllers = 'localhost, localhost.2';
$controllers = 'ldap://localhost:389 ldap://localhost.2:389';

$configFile = new FakeConfigFile();
$configFile->SetKey(LdapConfigKeys::HOST, $controllers);
$configFile->SetKey(LdapConfigKeys::URI, $controllers);
$this->fakeConfig->SetFile(LdapConfigKeys::CONFIG_ID, $configFile);

$options = new LdapOptions();

$this->assertEquals(['localhost', 'localhost.2'], $options->Controllers(), "comma separated values should become array");
$this->assertEquals(['ldap://localhost:389', 'ldap://localhost.2:389'], $options->Controllers(), "space separated uris should become array");
}

public function testThrowsIfLegacyHostIsConfigured()
{
$configFile = new FakeConfigFile();
$configFile->SetKey([
'key' => 'host',
'section' => 'ldap',
'type' => 'string',
], 'ldap://localhost');
$this->fakeConfig->SetFile(LdapConfigKeys::CONFIG_ID, $configFile);

$this->expectException(RuntimeException::class);
$this->expectExceptionMessage("LDAP settings 'host' and 'port' have been removed");

$options = new LdapOptions();
$options->Ldap2Config();
}

public function testThrowsIfLegacyPortIsConfigured()
{
$configFile = new FakeConfigFile();
$configFile->SetKey([
'key' => 'port',
'section' => 'ldap',
'type' => 'string',
], '389');
$this->fakeConfig->SetFile(LdapConfigKeys::CONFIG_ID, $configFile);

$this->expectException(RuntimeException::class);
$this->expectExceptionMessage("LDAP settings 'host' and 'port' have been removed");

$options = new LdapOptions();
$options->Ldap2Config();
}

public function testUserHandlesArraysAsAttribute()
Expand Down