From 6b681961e503aa12893218db1ed9e6154c1b750c Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Sun, 28 Apr 2024 12:29:57 +0100
Subject: [PATCH] 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
---
 .env.example.complete      |  2 +-
 app/Access/LdapService.php |  9 +++++++--
 app/Config/services.php    |  2 +-
 tests/Auth/LdapTest.php    | 34 +++++++++++++++++++++++++++++++++-
 4 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/.env.example.complete b/.env.example.complete
index b4beb60cc..e3b2185cc 100644
--- 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 9d2667635..67f3d6f54 100644
--- 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 a035f1056..da07de13e 100644
--- 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 34900ce6f..cb5fc5a87 100644
--- 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);