feature #31560 [Ldap][Security] LdapBindAuthenticationProvider does n…

…ot bind before search query (Simperfit)

This PR was merged into the 4.4 branch.

Discussion
----------

[Ldap][Security] LdapBindAuthenticationProvider does not bind before search query

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yesish
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | yes <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #24210   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

<!--
Replace this notice by a short README for your feature/bugfix. This will help people
understand your PR and can be used as a start for the documentation.

Additionally (see https://symfony.com/roadmap):
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the master branch.
-->

This is tagged as a bug but since we need to add new params to the differents login providers and a deprecation I think we need to consider it as a new feature, maybe this could go in 3.4 too but without the deprecation.

This allow using a searchDn and a sarchPassword when passing a queryString in order to do the query authenticated.

Commits
-------

cb2d97f [Ldap][Security] LdapBindAuthenticationProvider does not bind before search query

src/Symfony/Bundle/SecurityBundle/CHANGELOG.md

@@ -1,6 +1,10 @@
 CHANGELOG
 =========
 
+4.4.0
+
+* Deprecated the usage of "query_string" without a "search_dn" and a "search_password" config key in Ldap factories.
+
 4.3.0
 -----
 

src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/FormLoginLdapFactory.php

@@ -34,9 +34,14 @@ protected function createAuthProvider(ContainerBuilder $container, $id, $config,
             ->replaceArgument(2, $id)
             ->replaceArgument(3, new Reference($config['service']))
             ->replaceArgument(4, $config['dn_string'])
+            ->replaceArgument(5, $config['search_dn'])
+            ->replaceArgument(6, $config['search_password'])
         ;
 
         if (!empty($config['query_string'])) {
+            if ('' === $config['search_dn'] || '' === $config['search_password']) {
+                @trigger_error('Using the "query_string" config without using a "search_dn" and a "search_password" is deprecated since Symfony 4.4 and will throw in Symfony 5.0.', E_USER_DEPRECATED);
+            }
             $definition->addMethodCall('setQueryString', [$config['query_string']]);
         }
 
@@ -52,6 +57,8 @@ public function addConfiguration(NodeDefinition $node)
                 ->scalarNode('service')->defaultValue('ldap')->end()
                 ->scalarNode('dn_string')->defaultValue('{username}')->end()
                 ->scalarNode('query_string')->end()
+                ->scalarNode('search_dn')->defaultValue('')->end()
+                ->scalarNode('search_password')->defaultValue('')->end()
             ->end()
         ;
     }

src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/HttpBasicLdapFactory.php

@@ -35,12 +35,17 @@ public function create(ContainerBuilder $container, $id, $config, $userProvider,
             ->replaceArgument(2, $id)
             ->replaceArgument(3, new Reference($config['service']))
             ->replaceArgument(4, $config['dn_string'])
+            ->replaceArgument(5, $config['search_dn'])
+            ->replaceArgument(6, $config['search_password'])
         ;
 
         // entry point
         $entryPointId = $this->createEntryPoint($container, $id, $config, $defaultEntryPoint);
 
         if (!empty($config['query_string'])) {
+            if ('' === $config['search_dn'] || '' === $config['search_password']) {
+                @trigger_error('Using the "query_string" config without using a "search_dn" and a "search_password" is deprecated since Symfony 4.4 and will throw in Symfony 5.0.', E_USER_DEPRECATED);
+            }
             $definition->addMethodCall('setQueryString', [$config['query_string']]);
         }
 
@@ -62,6 +67,8 @@ public function addConfiguration(NodeDefinition $node)
                 ->scalarNode('service')->defaultValue('ldap')->end()
                 ->scalarNode('dn_string')->defaultValue('{username}')->end()
                 ->scalarNode('query_string')->end()
+                ->scalarNode('search_dn')->defaultValue('')->end()
+                ->scalarNode('search_password')->defaultValue('')->end()
             ->end()
         ;
     }

src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/JsonLoginLdapFactory.php

@@ -36,9 +36,14 @@ protected function createAuthProvider(ContainerBuilder $container, $id, $config,
             ->replaceArgument(2, $id)
             ->replaceArgument(3, new Reference($config['service']))
             ->replaceArgument(4, $config['dn_string'])
+            ->replaceArgument(5, $config['search_dn'])
+            ->replaceArgument(6, $config['search_password'])
         ;
 
         if (!empty($config['query_string'])) {
+            if ('' === $config['search_dn'] || '' === $config['search_password']) {
+                @trigger_error('Using the "query_string" config without using a "search_dn" and a "search_password" is deprecated since Symfony 4.4 and will throw in Symfony 5.0.', E_USER_DEPRECATED);
+            }
             $definition->addMethodCall('setQueryString', [$config['query_string']]);
         }
 
@@ -54,6 +59,8 @@ public function addConfiguration(NodeDefinition $node)
                 ->scalarNode('service')->defaultValue('ldap')->end()
                 ->scalarNode('dn_string')->defaultValue('{username}')->end()
                 ->scalarNode('query_string')->end()
+                ->scalarNode('search_dn')->defaultValue('')->end()
+                ->scalarNode('search_password')->defaultValue('')->end()
             ->end()
         ;
     }

src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml

@@ -195,6 +195,8 @@
             <argument /> <!-- UserChecker -->
             <argument /> <!-- Provider-shared Key -->
             <argument /> <!-- LDAP -->
+            <argument /> <!-- search dn -->
+            <argument /> <!-- search password -->
             <argument /> <!-- Base DN -->
             <argument>%security.authentication.hide_user_not_found%</argument>
         </service>

src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php

@@ -34,14 +34,18 @@ class LdapBindAuthenticationProvider extends UserAuthenticationProvider
     private $ldap;
     private $dnString;
     private $queryString;
+    private $searchDn;
+    private $searchPassword;
 
-    public function __construct(UserProviderInterface $userProvider, UserCheckerInterface $userChecker, string $providerKey, LdapInterface $ldap, string $dnString = '{username}', bool $hideUserNotFoundExceptions = true)
+    public function __construct(UserProviderInterface $userProvider, UserCheckerInterface $userChecker, string $providerKey, LdapInterface $ldap, string $dnString = '{username}', bool $hideUserNotFoundExceptions = true, string $searchDn = '', string $searchPassword = '')
     {
         parent::__construct($userChecker, $providerKey, $hideUserNotFoundExceptions);
 
         $this->userProvider = $userProvider;
         $this->ldap = $ldap;
         $this->dnString = $dnString;
+        $this->searchDn = $searchDn;
+        $this->searchPassword = $searchPassword;
     }
 
     /**
@@ -82,6 +86,11 @@ protected function checkAuthentication(UserInterface $user, UsernamePasswordToke
             $username = $this->ldap->escape($username, '', LdapInterface::ESCAPE_DN);
 
             if ($this->queryString) {
+                if ('' !== $this->searchDn && '' !== $this->searchPassword) {
+                    $this->ldap->bind($this->searchDn, $this->searchPassword);
+                } else {
+                    @trigger_error('Using the "query_string" config without using a "search_dn" and a "search_password" is deprecated since Symfony 4.4 and will throw in Symfony 5.0.', E_USER_DEPRECATED);
+                }
                 $query = str_replace('{username}', $username, $this->queryString);
                 $result = $this->ldap->query($this->dnString, $query)->execute();
                 if (1 !== $result->count()) {

src/Symfony/Component/Security/Core/Tests/Authentication/Provider/LdapBindAuthenticationProviderTest.php

@@ -123,6 +123,10 @@ public function testQueryForDn()
             ->with('foo', '')
             ->willReturn('foo')
         ;
+        $ldap
+            ->expects($this->at(1))
+            ->method('bind')
+            ->with('elsa', 'test1234A$');
         $ldap
             ->expects($this->once())
             ->method('query')
@@ -131,7 +135,48 @@ public function testQueryForDn()
         ;
         $userChecker = $this->getMockBuilder(UserCheckerInterface::class)->getMock();
 
-        $provider = new LdapBindAuthenticationProvider($userProvider, $userChecker, 'key', $ldap);
+        $provider = new LdapBindAuthenticationProvider($userProvider, $userChecker, 'key', $ldap, '{username}', true, 'elsa', 'test1234A$');
+        $provider->setQueryString('{username}bar');
+        $reflection = new \ReflectionMethod($provider, 'checkAuthentication');
+        $reflection->setAccessible(true);
+
+        $reflection->invoke($provider, new User('foo', null), new UsernamePasswordToken('foo', 'bar', 'key'));
+    }
+
+    public function testQueryWithUserForDn()
+    {
+        $userProvider = $this->getMockBuilder(UserProviderInterface::class)->getMock();
+
+        $collection = new \ArrayIterator([new Entry('')]);
+
+        $query = $this->getMockBuilder(QueryInterface::class)->getMock();
+        $query
+            ->expects($this->once())
+            ->method('execute')
+            ->will($this->returnValue($collection))
+        ;
+
+        $ldap = $this->getMockBuilder(LdapInterface::class)->getMock();
+        $ldap
+            ->expects($this->once())
+            ->method('escape')
+            ->with('foo', '')
+            ->will($this->returnValue('foo'))
+        ;
+        $ldap
+            ->expects($this->at(1))
+            ->method('bind')
+            ->with('elsa', 'test1234A$');
+        $ldap
+            ->expects($this->once())
+            ->method('query')
+            ->with('{username}', 'foobar')
+            ->will($this->returnValue($query))
+        ;
+
+        $userChecker = $this->getMockBuilder(UserCheckerInterface::class)->getMock();
+
+        $provider = new LdapBindAuthenticationProvider($userProvider, $userChecker, 'key', $ldap, '{username}', true, 'elsa', 'test1234A$');
         $provider->setQueryString('{username}bar');
         $reflection = new \ReflectionMethod($provider, 'checkAuthentication');
         $reflection->setAccessible(true);
@@ -157,14 +202,18 @@ public function testEmptyQueryResultShouldThrowAnException()
         ;
 
         $ldap = $this->getMockBuilder(LdapInterface::class)->getMock();
+        $ldap
+            ->expects($this->at(1))
+            ->method('bind')
+            ->with('elsa', 'test1234A$');
         $ldap
             ->expects($this->once())
             ->method('query')
             ->willReturn($query)
         ;
         $userChecker = $this->getMockBuilder(UserCheckerInterface::class)->getMock();
 
-        $provider = new LdapBindAuthenticationProvider($userProvider, $userChecker, 'key', $ldap);
+        $provider = new LdapBindAuthenticationProvider($userProvider, $userChecker, 'key', $ldap, '{username}', true, 'elsa', 'test1234A$');
         $provider->setQueryString('{username}bar');
         $reflection = new \ReflectionMethod($provider, 'checkAuthentication');
         $reflection->setAccessible(true);