LDAP: Updated default user filter placeholder format

To not conflict with env variables, and to align with placeholders used
for PDF gen command.
Added test to cover, including old format supported for
back-compatibility.
For #4967

diff --git a/.env.example.complete b/.env.example.complete
index b4beb60cc0ed99a77c03e49f74c27160a252e168..e3b2185cc29bf99265039184efc9b058bfef669e 100644 (file)
--- a/.env.example.complete
+++ b/.env.example.complete
@@ -215,7 +215,7 @@ LDAP_SERVER=false
 LDAP_BASE_DN=false
 LDAP_DN=false
 LDAP_PASS=false
-LDAP_USER_FILTER=false
+LDAP_USER_FILTER="(&(uid={user}))"
 LDAP_VERSION=false
 LDAP_START_TLS=false
 LDAP_TLS_INSECURE=false

diff --git a/app/Access/LdapService.php b/app/Access/LdapService.php
index 9d266763531685377e41d7d9775d61c9803b5383..67f3d6f54c6ed70c652516feb0512796a34fc94f 100644 (file)
--- a/app/Access/LdapService.php
+++ b/app/Access/LdapService.php
@@ -249,13 +249,18 @@ class LdapService
 
     /**
      * Build a filter string by injecting common variables.
+     * Both "${var}" and "{var}" style placeholders are supported.
+     * Dollar based are old format but supported for compatibility.
      */
     protected function buildFilter(string $filterString, array $attrs): string
     {
         $newAttrs = [];
         foreach ($attrs as $key => $attrText) {
-            $newKey = '${' . $key . '}';
-            $newAttrs[$newKey] = $this->ldap->escape($attrText);
+            $escapedText = $this->ldap->escape($attrText);
+            $oldVarKey = '${' . $key . '}';
+            $newVarKey = '{' . $key . '}';
+            $newAttrs[$oldVarKey] = $escapedText;
+            $newAttrs[$newVarKey] = $escapedText;
         }
 
         return strtr($filterString, $newAttrs);

diff --git a/app/Config/services.php b/app/Config/services.php
index a035f10569500d5c05c552c351e7c5d512117cd3..da07de13e9cbb61ce6c259de71979f4dcf9bf0a0 100644 (file)
--- a/app/Config/services.php
+++ b/app/Config/services.php
@@ -123,7 +123,7 @@ return [
         'dn'                     => env('LDAP_DN', false),
         'pass'                   => env('LDAP_PASS', false),
         'base_dn'                => env('LDAP_BASE_DN', false),
-        'user_filter'            => env('LDAP_USER_FILTER', '(&(uid=${user}))'),
+        'user_filter'            => env('LDAP_USER_FILTER', '(&(uid={user}))'),
         'version'                => env('LDAP_VERSION', false),
         'id_attribute'           => env('LDAP_ID_ATTRIBUTE', 'uid'),
         'email_attribute'        => env('LDAP_EMAIL_ATTRIBUTE', 'mail'),

diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php
index 34900ce6f70a1d7356f6f45d822a207c73e48870..cb5fc5a8725f953efa686f1e572ed14638fb1a21 100644 (file)
--- a/tests/Auth/LdapTest.php
+++ b/tests/Auth/LdapTest.php
@@ -32,7 +32,7 @@ class LdapTest extends TestCase
             'services.ldap.id_attribute'           => 'uid',
             'services.ldap.user_to_groups'         => false,
             'services.ldap.version'                => '3',
-            'services.ldap.user_filter'            => '(&(uid=${user}))',
+            'services.ldap.user_filter'            => '(&(uid={user}))',
             'services.ldap.follow_referrals'       => false,
             'services.ldap.tls_insecure'           => false,
             'services.ldap.thumbnail_attribute'    => null,
@@ -178,6 +178,38 @@ class LdapTest extends TestCase
         $this->assertDatabaseHas('users', ['email' => $this->mockUser->email, 'email_confirmed' => false, 'external_auth_id' => 'cooluser456']);
     }
 
+    public function test_user_filter_default_placeholder_format()
+    {
+        config()->set('services.ldap.user_filter', '(&(uid={user}))');
+        $this->mockUser->name = 'barryldapuser';
+        $expectedFilter = '(&(uid=\62\61\72\72\79\6c\64\61\70\75\73\65\72))';
+
+        $this->commonLdapMocks(1, 1, 1, 1, 1);
+        $this->mockLdap->shouldReceive('searchAndGetEntries')
+            ->once()
+            ->with($this->resourceId, config('services.ldap.base_dn'), $expectedFilter, \Mockery::type('array'))
+            ->andReturn(['count' => 0, 0 => []]);
+
+        $resp = $this->mockUserLogin();
+        $resp->assertRedirect('/login');
+    }
+
+    public function test_user_filter_old_placeholder_format()
+    {
+        config()->set('services.ldap.user_filter', '(&(username=${user}))');
+        $this->mockUser->name = 'barryldapuser';
+        $expectedFilter = '(&(username=\62\61\72\72\79\6c\64\61\70\75\73\65\72))';
+
+        $this->commonLdapMocks(1, 1, 1, 1, 1);
+        $this->mockLdap->shouldReceive('searchAndGetEntries')
+            ->once()
+            ->with($this->resourceId, config('services.ldap.base_dn'), $expectedFilter, \Mockery::type('array'))
+            ->andReturn(['count' => 0, 0 => []]);
+
+        $resp = $this->mockUserLogin();
+        $resp->assertRedirect('/login');
+    }
+
     public function test_initial_incorrect_credentials()
     {
         $this->commonLdapMocks(1, 1, 1, 0, 1);