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);