Skip to content

Commit

Permalink
feature #21402 [Security] make LdapBindAuthenticationProvider capable…
Browse files Browse the repository at this point in the history
… of searching for the DN (lsmith77, nietonfir)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[Security] make LdapBindAuthenticationProvider capable of searching for the DN

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #16823, #20905
| License       | MIT
| Doc PR        | symfony/symfony-docs#7420

I guess due to the separation between the user and auth provider something like the following isn't ok  (note: the following works just fine for me but if course in the end the username is the DN and not the user provided shorter username):

```diff
diff --git a/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php b/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php
index 5ebb09a..18d7825 100644
--- a/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php
+++ b/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php
@@ -70,7 +70,7 @@ class LdapBindAuthenticationProvider extends UserAuthenticationProvider
      */
     protected function checkAuthentication(UserInterface $user, UsernamePasswordToken $token)
     {
-        $username = $token->getUsername();
+        $username = $user->getUsername();
         $password = $token->getCredentials();

         if ('' === $password) {
@@ -78,10 +78,7 @@ class LdapBindAuthenticationProvider extends UserAuthenticationProvider
         }

         try {
-            $username = $this->ldap->escape($username, '', LdapInterface::ESCAPE_DN);
-            $dn = str_replace('{username}', $username, $this->dnString);
-
-            $this->ldap->bind($dn, $password);
+            $this->ldap->bind($username, $password);
         } catch (ConnectionException $e) {
             throw new BadCredentialsException('The presented password is invalid.');
         }
diff --git a/src/Symfony/Component/Security/Core/User/LdapUserProvider.php b/src/Symfony/Component/Security/Core/User/LdapUserProvider.php
index fc42419..8194c4c 100644
--- a/src/Symfony/Component/Security/Core/User/LdapUserProvider.php
+++ b/src/Symfony/Component/Security/Core/User/LdapUserProvider.php
@@ -115,7 +115,7 @@ class LdapUserProvider implements UserProviderInterface
     {
         $password = $this->getPassword($entry);

-        return new User($username, $password, $this->defaultRoles);
+        return new User($entry->getDn(), $password, $this->defaultRoles);
     }

     /**
```

Therefore I created an entire new auth provider.

Commits
-------

8ddd533 Merge pull request #1 from nietonfir/http_basic_ldap
a783e5c Update HttpBasicLdapFactory
a30191f make LdapBindAuthenticationProvider capable of searching for the DN
  • Loading branch information
fabpot committed Jan 28, 2017
2 parents f025392 + 8ddd533 commit bcf8b68
Show file tree
Hide file tree
Showing 4 changed files with 107 additions and 3 deletions.
Expand Up @@ -27,7 +27,7 @@ class FormLoginLdapFactory extends FormLoginFactory
protected function createAuthProvider(ContainerBuilder $container, $id, $config, $userProviderId)
{
$provider = 'security.authentication.provider.ldap_bind.'.$id;
$container
$definition = $container
->setDefinition($provider, new ChildDefinition('security.authentication.provider.ldap_bind'))
->replaceArgument(0, new Reference($userProviderId))
->replaceArgument(1, new Reference('security.user_checker.'.$id))
Expand All @@ -36,6 +36,10 @@ protected function createAuthProvider(ContainerBuilder $container, $id, $config,
->replaceArgument(4, $config['dn_string'])
;

if (!empty($config['query_string'])) {
$definition->addMethodCall('setQueryString', array($config['query_string']));
}

return $provider;
}

Expand All @@ -47,6 +51,7 @@ public function addConfiguration(NodeDefinition $node)
->children()
->scalarNode('service')->defaultValue('ldap')->end()
->scalarNode('dn_string')->defaultValue('{username}')->end()
->scalarNode('query_string')->end()
->end()
;
}
Expand Down
Expand Up @@ -28,7 +28,7 @@ class HttpBasicLdapFactory extends HttpBasicFactory
public function create(ContainerBuilder $container, $id, $config, $userProvider, $defaultEntryPoint)
{
$provider = 'security.authentication.provider.ldap_bind.'.$id;
$container
$definition = $container
->setDefinition($provider, new ChildDefinition('security.authentication.provider.ldap_bind'))
->replaceArgument(0, new Reference($userProvider))
->replaceArgument(1, new Reference('security.user_checker.'.$id))
Expand All @@ -40,6 +40,10 @@ public function create(ContainerBuilder $container, $id, $config, $userProvider,
// entry point
$entryPointId = $this->createEntryPoint($container, $id, $config, $defaultEntryPoint);

if (!empty($config['query_string'])) {
$definition->addMethodCall('setQueryString', array($config['query_string']));
}

// listener
$listenerId = 'security.authentication.listener.basic.'.$id;
$listener = $container->setDefinition($listenerId, new ChildDefinition('security.authentication.listener.basic'));
Expand All @@ -57,6 +61,7 @@ public function addConfiguration(NodeDefinition $node)
->children()
->scalarNode('service')->defaultValue('ldap')->end()
->scalarNode('dn_string')->defaultValue('{username}')->end()
->scalarNode('query_string')->end()
->end()
;
}
Expand Down
Expand Up @@ -33,6 +33,7 @@ class LdapBindAuthenticationProvider extends UserAuthenticationProvider
private $userProvider;
private $ldap;
private $dnString;
private $queryString;

/**
* Constructor.
Expand All @@ -53,6 +54,16 @@ public function __construct(UserProviderInterface $userProvider, UserCheckerInte
$this->dnString = $dnString;
}

/**
* Set a query string to use in order to find a DN for the username.
*
* @param string $queryString
*/
public function setQueryString($queryString)
{
$this->queryString = $queryString;
}

/**
* {@inheritdoc}
*/
Expand All @@ -79,7 +90,18 @@ protected function checkAuthentication(UserInterface $user, UsernamePasswordToke

try {
$username = $this->ldap->escape($username, '', LdapInterface::ESCAPE_DN);
$dn = str_replace('{username}', $username, $this->dnString);

if ($this->queryString) {
$query = str_replace('{username}', $username, $this->queryString);
$result = $this->ldap->query($this->dnString, $query)->execute();
if (1 !== $result->count()) {
throw new BadCredentialsException('The presented username is invalid.');
}

$dn = $result[0]->getDn();
} else {
$dn = str_replace('{username}', $username, $this->dnString);
}

$this->ldap->bind($dn, $password);
} catch (ConnectionException $e) {
Expand Down
Expand Up @@ -12,6 +12,9 @@
namespace Symfony\Component\Security\Core\Tests\Authentication\Provider;

use Symfony\Component\Ldap\LdapInterface;
use Symfony\Component\Ldap\Entry;
use Symfony\Component\Ldap\Adapter\QueryInterface;
use Symfony\Component\Ldap\Adapter\CollectionInterface;
use Symfony\Component\Security\Core\Authentication\Provider\LdapBindAuthenticationProvider;
use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
use Symfony\Component\Security\Core\User\User;
Expand Down Expand Up @@ -81,4 +84,73 @@ public function testRetrieveUser()

$reflection->invoke($provider, 'foo', new UsernamePasswordToken('foo', 'bar', 'key'));
}

public function testQueryForDn()
{
$userProvider = $this->getMockBuilder(UserProviderInterface::class)->getMock();

$collection = new \ArrayIterator(array(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->once())
->method('query')
->with('{username}', 'foobar')
->will($this->returnValue($query))
;
$userChecker = $this->getMockBuilder(UserCheckerInterface::class)->getMock();

$provider = new LdapBindAuthenticationProvider($userProvider, $userChecker, 'key', $ldap);
$provider->setQueryString('{username}bar');
$reflection = new \ReflectionMethod($provider, 'checkAuthentication');
$reflection->setAccessible(true);

$reflection->invoke($provider, new User('foo', null), new UsernamePasswordToken('foo', 'bar', 'key'));
}

/**
* @expectedException \Symfony\Component\Security\Core\Exception\BadCredentialsException
* @expectedExceptionMessage The presented username is invalid.
*/
public function testEmptyQueryResultShouldThrowAnException()
{
$userProvider = $this->getMockBuilder(UserProviderInterface::class)->getMock();

$collection = $this->getMockBuilder(CollectionInterface::class)->getMock();

$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('query')
->will($this->returnValue($query))
;
$userChecker = $this->getMockBuilder(UserCheckerInterface::class)->getMock();

$provider = new LdapBindAuthenticationProvider($userProvider, $userChecker, 'key', $ldap);
$provider->setQueryString('{username}bar');
$reflection = new \ReflectionMethod($provider, 'checkAuthentication');
$reflection->setAccessible(true);

$reflection->invoke($provider, new User('foo', null), new UsernamePasswordToken('foo', 'bar', 'key'));
}
}

0 comments on commit bcf8b68

Please sign in to comment.