Skip to content

Commit c275018

Browse files
JohnVillalovosJohnVillalovos
committed
refactor(ldap)!: switch LDAP server config from host/port to uri
To work better with PHP's `ldap_connect()` function, we have switched to requiring a URI to specify the LDAP server(s). This allows specifying multiple LDAP servers, if desired. BREAKING CHANGE: LDAP plugin configuration now requires `uri` and no longer supports `host` or `port`. Migrate to: `'uri' => 'ldap://host:389'` or `'uri' => 'ldaps://host:636'` For multiple servers, provide a space-separated URI string.
1 parent 54bcd2c commit c275018

7 files changed

Lines changed: 161 additions & 43 deletions

File tree

‎docs/source/LDAP-Authentication.rst‎

Lines changed: 37 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ options with detailed comments explaining each setting. You can also view and
5252
modify these settings through the web admin interface at **Application
5353
Management > Configuration**. Key settings include:
5454

55-
- **host**: LDAP server URL(s)
55+
- **uri**: LDAP URI string. For multiple servers, use a space-separated list of URIs.
5656
- **binddn/bindpw**: Service account credentials for directory searches
5757
- **basedn**: Base DN where users are located
5858
- **user.id.attribute**: LDAP attribute for username lookup (typically ``uid``)
@@ -62,14 +62,46 @@ Management > Configuration**. Key settings include:
6262

6363
.. important::
6464

65-
The ``host`` value must include the LDAP scheme, for example
65+
The ``uri`` value must include the LDAP scheme, for example
6666
``ldap://ldap.example.com`` or ``ldaps://ldap.example.com``.
6767
A bare hostname such as ``ldap.example.com`` is not enough.
6868

69+
URI examples:
70+
71+
.. code-block:: php
72+
73+
// single LDAP server (unencrypted LDAP, explicit port)
74+
'uri' => 'ldap://ldap1.example.com:389',
75+
76+
// single LDAP server (unencrypted LDAP, default port 389)
77+
'uri' => 'ldap://ldap1.example.com',
78+
79+
// single LDAP server over LDAPS (TLS, explicit port)
80+
'uri' => 'ldaps://ldap1.example.com:636',
81+
82+
// single LDAP server over LDAPS (TLS, default port 636)
83+
'uri' => 'ldaps://ldap1.example.com',
84+
85+
// multiple LDAP servers (space-separated URIs in one string)
86+
'uri' => 'ldap://ldap1.example.com:389 ldap://ldap2.example.com:389',
87+
88+
// multiple LDAPS servers
89+
'uri' => 'ldaps://ldap1.example.com:636 ldaps://ldap2.example.com:636',
90+
91+
Port defaults:
92+
93+
- ``ldap://`` uses port ``389`` by default when no port is specified.
94+
- ``ldaps://`` uses port ``636`` by default when no port is specified.
95+
96+
Breaking change:
97+
98+
- ``host`` and ``port`` are no longer supported.
99+
- Configure LDAP endpoints only through ``uri``.
100+
69101
Alternatively, configure the plugin through the web admin interface at
70102
**Application Configuration** (``/Web/admin/manage_configuration.php``) and
71-
select **Authentification-Ldap**.
72-
Refer to ``/plugins/Authentication/Ldap/Ldap.config.dist.php`` for complete
103+
select **Authentication-Ldap**. Refer to
104+
``/plugins/Authentication/Ldap/Ldap.config.dist.php`` for complete
73105
documentation of all options.
74106

75107
Troubleshooting
@@ -89,7 +121,7 @@ Common Issues
89121
~~~~~~~~~~~~~
90122

91123
**Connection failures**
92-
- Verify server hostname and port accessibility
124+
- Verify LDAP URI hostname and port accessibility
93125
- Check firewall rules
94126
- Test with ``telnet ldap.example.com 389``
95127

‎plugins/Authentication/Ldap/Ldap.config.dist.php‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,16 @@
66
return [
77
'settings' => [
88
'ldap' => [
9-
// comma separated list of ldap servers such as ldap://mydomain1,ldap://localhost
10-
'host' => 'ldap://localhost',
11-
12-
// default ldap port 389 or 636 for ssl.
13-
'port' => 389,
9+
// LDAP URI(s). For multiple servers, separate each URI with spaces.
10+
// If no port is specified, defaults are: ldap:// = 389, ldaps:// = 636.
11+
// examples:
12+
// 'ldap://ldap1.example.com'
13+
// 'ldap://ldap1.example.com:389'
14+
// 'ldaps://ldap1.example.com'
15+
// 'ldaps://ldap1.example.com:636'
16+
// 'ldap://ldap1.example.com:389 ldap://ldap2.example.com:389'
17+
// 'ldaps://ldap1.example.com:636 ldaps://ldap2.example.com:636'
18+
'uri' => 'ldap://localhost:389',
1419

1520
// LDAP protocol version
1621
'version' => 3,

‎plugins/Authentication/Ldap/Ldap.php‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,13 @@ public function Validate($username, $password)
108108
$this->password = $password;
109109

110110
$username = $this->CleanUsername($username);
111-
$connected = $this->ldap->Connect();
111+
112+
try {
113+
$connected = $this->ldap->Connect();
114+
} catch (RuntimeException $e) {
115+
Log::Error('LDAP configuration error: %s', $e->getMessage());
116+
throw $e;
117+
}
112118

113119
if (!$connected) {
114120
throw new Exception('Could not connect to LDAP server. Please check your LDAP configuration settings');

‎plugins/Authentication/Ldap/LdapConfigKeys.php‎

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -6,21 +6,12 @@ class LdapConfigKeys extends PluginConfigKeys
66
{
77
public const CONFIG_ID = 'ldap';
88

9-
public const HOST = [
10-
'key' => 'host',
9+
public const URI = [
10+
'key' => 'uri',
1111
'type' => 'string',
1212
'default' => '',
13-
'label' => 'LDAP Host',
14-
'description' => 'Hostname or IP address of LDAP server. Should start with ldap:// or ldaps://',
15-
'section' => 'ldap'
16-
];
17-
18-
public const PORT = [
19-
'key' => 'port',
20-
'type' => 'integer',
21-
'default' => 389,
22-
'label' => 'LDAP Port',
23-
'description' => 'Port of LDAP server (usually 389 or 636 for SSL)',
13+
'label' => 'LDAP server URI(s)',
14+
'description' => 'LDAP server URI(s). Use ldap:// or ldaps://. For multiple servers, separate URIs with spaces.',
2415
'section' => 'ldap'
2516
];
2617

‎plugins/Authentication/Ldap/LdapOptions.php‎

Lines changed: 34 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,10 @@
55
class LdapOptions
66
{
77
private $_options = [];
8+
/**
9+
* @var array<string, mixed>
10+
*/
11+
private $rawLdapSettings = [];
812

913
public function __construct()
1014
{
@@ -13,22 +17,22 @@ public function __construct()
1317
$configPath = dirname(__FILE__) . '/Ldap.config.dist.php';
1418
}
1519

16-
require_once($configPath);
17-
1820
Configuration::Instance()->Register(
1921
$configPath,
2022
'',
2123
LdapConfigKeys::CONFIG_ID,
2224
false,
2325
LdapConfigKeys::class
2426
);
27+
28+
$allValues = Configuration::Instance()->File(LdapConfigKeys::CONFIG_ID)->GetValues();
29+
$this->rawLdapSettings = $allValues['ldap'] ?? [];
2530
}
2631

2732
public function Ldap2Config()
2833
{
29-
$hosts = $this->GetHosts();
34+
$hosts = $this->GetUris();
3035
$this->SetOption('host', $hosts);
31-
$this->SetOption('port', $this->GetConfig(LdapConfigKeys::PORT, new IntConverter()));
3236
$this->SetOption('starttls', $this->GetConfig(LdapConfigKeys::STARTTLS, new BooleanConverter()));
3337
$this->SetOption('version', $this->GetConfig(LdapConfigKeys::VERSION, new IntConverter()));
3438
$this->SetOption('binddn', $this->GetConfig(LdapConfigKeys::BINDDN));
@@ -47,7 +51,7 @@ public function RetryAgainstDatabase()
4751

4852
public function Controllers()
4953
{
50-
return $this->GetHosts();
54+
return $this->GetUris();
5155
}
5256

5357
private function SetOption($key, $value)
@@ -64,15 +68,35 @@ private function GetConfig($configDef, $converter = null)
6468
return Configuration::Instance()->File(LdapConfigKeys::CONFIG_ID)->GetKey($configDef, $converter);
6569
}
6670

67-
private function GetHosts()
71+
private function GetUris()
6872
{
69-
$hosts = explode(',', $this->GetConfig(LdapConfigKeys::HOST));
73+
$this->AssertLegacyHostPortNotConfigured();
74+
75+
$uriConfig = trim($this->GetConfig(LdapConfigKeys::URI));
76+
if (empty($uriConfig)) {
77+
throw new RuntimeException("LDAP setting 'uri' is required and must contain at least one ldap:// or ldaps:// URI.");
78+
}
7079

71-
for ($i = 0; $i < count($hosts); $i++) {
72-
$hosts[$i] = trim($hosts[$i]);
80+
$uris = preg_split('/\s+/', $uriConfig) ?: [];
81+
foreach ($uris as $uri) {
82+
$scheme = parse_url($uri, PHP_URL_SCHEME);
83+
$host = parse_url($uri, PHP_URL_HOST);
84+
85+
if (!in_array($scheme, ['ldap', 'ldaps'], true) || empty($host)) {
86+
throw new RuntimeException(
87+
sprintf("Invalid LDAP URI '%s'. Use ldap:// or ldaps:// with a hostname. For multiple servers, separate URIs with spaces.", $uri)
88+
);
89+
}
7390
}
7491

75-
return $hosts;
92+
return $uris;
93+
}
94+
95+
private function AssertLegacyHostPortNotConfigured()
96+
{
97+
if (array_key_exists('host', $this->rawLdapSettings) || array_key_exists('port', $this->rawLdapSettings)) {
98+
throw new RuntimeException("LDAP settings 'host' and 'port' have been removed. Use only the 'uri' setting.");
99+
}
76100
}
77101

78102
public function BaseDn()

‎tests/Plugins/Authentication/Ldap/LdapTest.php‎

Lines changed: 64 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,19 @@ public function testNotValidIfCannotFindUser()
9898
$this->assertTrue($this->fakeLdap->_AuthenticateCalled);
9999
}
100100

101+
public function testValidateRethrowsRuntimeExceptionFromConnect()
102+
{
103+
$exception = new RuntimeException("LDAP settings 'host' and 'port' have been removed. Use only the 'uri' setting.");
104+
$this->fakeLdap->_ConnectException = $exception;
105+
106+
$auth = new Ldap($this->fakeAuth, $this->fakeLdap, $this->fakeLdapOptions);
107+
108+
$this->expectException(RuntimeException::class);
109+
$this->expectExceptionMessage("LDAP settings 'host' and 'port' have been removed");
110+
111+
$auth->Validate($this->username, $this->password);
112+
}
113+
101114
public function testDoesNotTryToLoadUserDetailsIfNotValid()
102115
{
103116
$this->fakeLdap->_ExpectedAuthenticate = false;
@@ -186,17 +199,15 @@ public function testDoesNotSyncIfUserWasNotFoundInLdap()
186199

187200
public function testConstructsOptionsCorrectly()
188201
{
189-
$hosts = 'localhost, localhost.2';
190-
$port = '389';
202+
$uris = 'ldap://localhost:389 ldap://localhost.2:389';
191203
$binddn = 'cn=admin,ou=users,dc=example,dc=org';
192204
$password = 'pw';
193205
$base = 'dc=example,dc=org';
194206
$starttls = 'false';
195207
$version = '3';
196208

197209
$configFile = new FakeConfigFile();
198-
$configFile->SetKey(LdapConfigKeys::HOST, $hosts);
199-
$configFile->SetKey(LdapConfigKeys::PORT, $port);
210+
$configFile->SetKey(LdapConfigKeys::URI, $uris);
200211
$configFile->SetKey(LdapConfigKeys::BINDDN, $binddn);
201212
$configFile->SetKey(LdapConfigKeys::BINDPW, $password);
202213
$configFile->SetKey(LdapConfigKeys::BASEDN, $base);
@@ -209,8 +220,7 @@ public function testConstructsOptionsCorrectly()
209220
$options = $ldapOptions->Ldap2Config();
210221

211222
$this->assertNotNull($this->fakeConfig->_RegisteredFiles[LdapConfigKeys::CONFIG_ID]);
212-
$this->assertEquals('localhost', $options['host'][0], 'domain_controllers must be an array');
213-
$this->assertEquals(intval($port), $options['port'], 'port should be int');
223+
$this->assertEquals('ldap://localhost:389', $options['host'][0], 'controllers must be an array of URIs');
214224
$this->assertEquals($binddn, $options['binddn']);
215225
$this->assertEquals($password, $options['bindpw']);
216226
$this->assertEquals($base, $options['basedn']);
@@ -220,15 +230,51 @@ public function testConstructsOptionsCorrectly()
220230

221231
public function testGetAllHosts()
222232
{
223-
$controllers = 'localhost, localhost.2';
233+
$controllers = 'ldap://localhost:389 ldap://localhost.2:389';
224234

225235
$configFile = new FakeConfigFile();
226-
$configFile->SetKey(LdapConfigKeys::HOST, $controllers);
236+
$configFile->SetKey(LdapConfigKeys::URI, $controllers);
227237
$this->fakeConfig->SetFile(LdapConfigKeys::CONFIG_ID, $configFile);
228238

229239
$options = new LdapOptions();
230240

231-
$this->assertEquals(['localhost', 'localhost.2'], $options->Controllers(), 'comma separated values should become array');
241+
$this->assertEquals(['ldap://localhost:389', 'ldap://localhost.2:389'], $options->Controllers(), 'space separated uris should become array');
242+
}
243+
244+
public function testThrowsIfLegacyHostIsConfigured()
245+
{
246+
$configFile = new FakeConfigFile();
247+
$configFile->SetKey([
248+
'key' => 'host',
249+
'section' => 'ldap',
250+
'type' => 'string',
251+
], 'ldap://localhost');
252+
$this->fakeConfig->SetFile(LdapConfigKeys::CONFIG_ID, $configFile);
253+
254+
$options = new LdapOptions();
255+
256+
$this->expectException(RuntimeException::class);
257+
$this->expectExceptionMessage("LDAP settings 'host' and 'port' have been removed");
258+
259+
$options->Ldap2Config();
260+
}
261+
262+
public function testThrowsIfLegacyPortIsConfigured()
263+
{
264+
$configFile = new FakeConfigFile();
265+
$configFile->SetKey([
266+
'key' => 'port',
267+
'section' => 'ldap',
268+
'type' => 'string',
269+
], '389');
270+
$this->fakeConfig->SetFile(LdapConfigKeys::CONFIG_ID, $configFile);
271+
272+
$options = new LdapOptions();
273+
274+
$this->expectException(RuntimeException::class);
275+
$this->expectExceptionMessage("LDAP settings 'host' and 'port' have been removed");
276+
277+
$options->Ldap2Config();
232278
}
233279

234280
public function testUserHandlesArraysAsAttribute()
@@ -345,6 +391,10 @@ public function testAuthRealLdap()
345391

346392
class FakeLdapOptions extends LdapOptions
347393
{
394+
public function __construct()
395+
{
396+
}
397+
348398
public $_RetryAgainstDatabase = false;
349399

350400
public function RetryAgainstDatabase()
@@ -391,9 +441,14 @@ public function __construct()
391441

392442
public $_ExpectedLdapUser;
393443

444+
public ?\RuntimeException $_ConnectException = null;
445+
394446
public function Connect()
395447
{
396448
$this->_ConnectCalled = true;
449+
if ($this->_ConnectException !== null) {
450+
throw $this->_ConnectException;
451+
}
397452
return $this->_ExpectedConnect;
398453
}
399454

‎tests/fakes/FakeConfig.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,11 @@ public function SetKey($configDef, $value)
141141
}
142142
}
143143

144+
public function GetValues(): array
145+
{
146+
return $this->_values;
147+
}
148+
144149
public function EnableSubscription()
145150
{
146151
}

0 commit comments

Comments
 (0)