From cb2d97f92b37e097d58c81556c0f12ffc3ec97b8 Mon Sep 17 00:00:00 2001 From: Amrouche Hamza Date: Tue, 21 May 2019 08:08:02 +0200 Subject: [PATCH] [Ldap][Security] LdapBindAuthenticationProvider does not bind before search query --- .../Bundle/SecurityBundle/CHANGELOG.md | 4 ++ .../Security/Factory/FormLoginLdapFactory.php | 7 +++ .../Security/Factory/HttpBasicLdapFactory.php | 7 +++ .../Security/Factory/JsonLoginLdapFactory.php | 7 +++ .../Resources/config/security_listeners.xml | 2 + .../LdapBindAuthenticationProvider.php | 11 +++- .../LdapBindAuthenticationProviderTest.php | 53 ++++++++++++++++++- 7 files changed, 88 insertions(+), 3 deletions(-) diff --git a/src/Symfony/Bundle/SecurityBundle/CHANGELOG.md b/src/Symfony/Bundle/SecurityBundle/CHANGELOG.md index e26a56b78884..1a6f7169a2cc 100644 --- a/src/Symfony/Bundle/SecurityBundle/CHANGELOG.md +++ b/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 ----- diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/FormLoginLdapFactory.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/FormLoginLdapFactory.php index 3d9d4b218631..0a3f92f402ed 100644 --- a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/FormLoginLdapFactory.php +++ b/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() ; } diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/HttpBasicLdapFactory.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/HttpBasicLdapFactory.php index 8ae020156886..309d114fab3f 100644 --- a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/HttpBasicLdapFactory.php +++ b/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() ; } diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/JsonLoginLdapFactory.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/JsonLoginLdapFactory.php index bd0e8115e93b..09b99dd3a2ac 100644 --- a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/JsonLoginLdapFactory.php +++ b/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() ; } diff --git a/src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml b/src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml index 9e3f96c366bf..55044986e310 100644 --- a/src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml +++ b/src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml @@ -195,6 +195,8 @@ + + %security.authentication.hide_user_not_found% diff --git a/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php b/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php index 2dadc05012e1..2caf1417cf79 100644 --- a/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php +++ b/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()) { diff --git a/src/Symfony/Component/Security/Core/Tests/Authentication/Provider/LdapBindAuthenticationProviderTest.php b/src/Symfony/Component/Security/Core/Tests/Authentication/Provider/LdapBindAuthenticationProviderTest.php index bad3072f4a9a..0043649028e0 100644 --- a/src/Symfony/Component/Security/Core/Tests/Authentication/Provider/LdapBindAuthenticationProviderTest.php +++ b/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,6 +202,10 @@ 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') @@ -164,7 +213,7 @@ public function testEmptyQueryResultShouldThrowAnException() ; $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);