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 b4beb60cc0e..e3b2185cc29 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 9d266763531..67f3d6f54c6 100644
--- a/app/Access/LdapService.php
+++ b/app/Access/LdapService.php
@@ -249,13 +249,18 @@ protected function parseServerString(string $serverString): string
 
     /**
      * 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 a035f105695..da07de13e9c 100644
--- a/app/Config/services.php
+++ b/app/Config/services.php
@@ -123,7 +123,7 @@
         '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 34900ce6f70..cb5fc5a8725 100644
--- a/tests/Auth/LdapTest.php
+++ b/tests/Auth/LdapTest.php
@@ -32,7 +32,7 @@ protected function setUp(): void
             '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 @@ public function test_a_custom_uid_attribute_can_be_specified_and_is_used_properl
         $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);