bug #18881 [Security][Ldap] Fixed issue with password attribute conta…

…ining an array of values. (csarrazi) This PR was merged into the 3.1 branch. Discussion ---------- [Security][Ldap] Fixed issue with password attribute containing an array of values. | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #18401 | License | MIT | Doc PR | This PR fixes #18401, as well as other possible issues: * First, the user provider no longer requires a password attribute by default. While this is not mandatory, it is more explicit to not set a password when using the `form_login_ldap` or `http_basic_ldap`, as these two providers don't use a password comparison mechanism, but `ldap_bind()` instead. * Second, the attribute is now configurable. Some implementations actually use different properties to store the user's password attribute. This will enable some users to correctly work with specific configurations. * Third, the user provider normalises the attribute array into a single string. Also, if the attribute has more than one value (which should not be possible), or if is not set, an exception will be thrown, with a clear error message. Commits ------- dbf45e4 [Ldap] Fixed issue with Entry password attribute containing array of values and made password attribute configurable
symfony · May 26, 2016 · 4241413 · 4241413
2 parents 4974a8b + dbf45e4
commit 4241413
Show file tree

Hide file tree

Showing 3 changed files with 177 additions and 5 deletions.
diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/UserProvider/LdapFactory.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/UserProvider/LdapFactory.php
@@ -35,6 +35,7 @@ public function create(ContainerBuilder $container, $id, $config)
             ->replaceArgument(4, $config['default_roles'])
             ->replaceArgument(5, $config['uid_key'])
             ->replaceArgument(6, $config['filter'])
+            ->replaceArgument(7, $config['password_attribute'])
         ;
     }
 
@@ -58,6 +59,7 @@ public function addConfiguration(NodeDefinition $node)
                 ->end()
                 ->scalarNode('uid_key')->defaultValue('sAMAccountName')->end()
                 ->scalarNode('filter')->defaultValue('({uid_key}={username})')->end()
+                ->scalarNode('password_attribute')->defaultNull()->end()
             ->end()
         ;
     }

diff --git a/src/Symfony/Component/Security/Core/Tests/User/LdapUserProviderTest.php b/src/Symfony/Component/Security/Core/Tests/User/LdapUserProviderTest.php
@@ -105,7 +105,10 @@ public function testLoadUserByUsernameFailsIfMoreThanOneLdapEntry()
         $provider->loadUserByUsername('foo');
     }
 
-    public function testSuccessfulLoadUserByUsername()
+    /**
+     * @expectedException \Symfony\Component\Security\Core\Exception\InvalidArgumentException
+     */
+    public function testLoadUserByUsernameFailsIfMoreThanOneLdapPasswordsInEntry()
     {
         $result = $this->getMock(CollectionInterface::class);
         $query = $this->getMock(QueryInterface::class);
@@ -120,8 +123,95 @@ public function testSuccessfulLoadUserByUsername()
             ->method('offsetGet')
             ->with(0)
             ->will($this->returnValue(new Entry('foo', array(
-                    'sAMAccountName' => 'foo',
-                    'userpassword' => 'bar',
+                    'sAMAccountName' => array('foo'),
+                    'userpassword' => array('bar', 'baz'),
+                )
+            )))
+        ;
+        $result
+            ->expects($this->once())
+            ->method('count')
+            ->will($this->returnValue(1))
+        ;
+        $ldap
+            ->expects($this->once())
+            ->method('escape')
+            ->will($this->returnValue('foo'))
+        ;
+        $ldap
+            ->expects($this->once())
+            ->method('query')
+            ->will($this->returnValue($query))
+        ;
+
+        $provider = new LdapUserProvider($ldap, 'ou=MyBusiness,dc=symfony,dc=com', null, null, array(), 'sAMAccountName', '({uid_key}={username})', 'userpassword');
+        $this->assertInstanceOf(
+            'Symfony\Component\Security\Core\User\User',
+            $provider->loadUserByUsername('foo')
+        );
+    }
+
+    /**
+     * @expectedException \Symfony\Component\Security\Core\Exception\InvalidArgumentException
+     */
+    public function testLoadUserByUsernameFailsIfEntryHasNoPasswordAttribute()
+    {
+        $result = $this->getMock(CollectionInterface::class);
+        $query = $this->getMock(QueryInterface::class);
+        $query
+            ->expects($this->once())
+            ->method('execute')
+            ->will($this->returnValue($result))
+        ;
+        $ldap = $this->getMock(LdapInterface::class);
+        $result
+            ->expects($this->once())
+            ->method('offsetGet')
+            ->with(0)
+            ->will($this->returnValue(new Entry('foo', array(
+                    'sAMAccountName' => array('foo'),
+                )
+            )))
+        ;
+        $result
+            ->expects($this->once())
+            ->method('count')
+            ->will($this->returnValue(1))
+        ;
+        $ldap
+            ->expects($this->once())
+            ->method('escape')
+            ->will($this->returnValue('foo'))
+        ;
+        $ldap
+            ->expects($this->once())
+            ->method('query')
+            ->will($this->returnValue($query))
+        ;
+
+        $provider = new LdapUserProvider($ldap, 'ou=MyBusiness,dc=symfony,dc=com', null, null, array(), 'sAMAccountName', '({uid_key}={username})', 'userpassword');
+        $this->assertInstanceOf(
+            'Symfony\Component\Security\Core\User\User',
+            $provider->loadUserByUsername('foo')
+        );
+    }
+
+    public function testLoadUserByUsernameIsSuccessfulWithoutPasswordAttribute()
+    {
+        $result = $this->getMock(CollectionInterface::class);
+        $query = $this->getMock(QueryInterface::class);
+        $query
+            ->expects($this->once())
+            ->method('execute')
+            ->will($this->returnValue($result))
+        ;
+        $ldap = $this->getMock(LdapInterface::class);
+        $result
+            ->expects($this->once())
+            ->method('offsetGet')
+            ->with(0)
+            ->will($this->returnValue(new Entry('foo', array(
+                    'sAMAccountName' => array('foo'),
                 )
             )))
         ;
@@ -147,4 +237,47 @@ public function testSuccessfulLoadUserByUsername()
             $provider->loadUserByUsername('foo')
         );
     }
+
+    public function testLoadUserByUsernameIsSuccessfulWithPasswordAttribute()
+    {
+        $result = $this->getMock(CollectionInterface::class);
+        $query = $this->getMock(QueryInterface::class);
+        $query
+            ->expects($this->once())
+            ->method('execute')
+            ->will($this->returnValue($result))
+        ;
+        $ldap = $this->getMock(LdapInterface::class);
+        $result
+            ->expects($this->once())
+            ->method('offsetGet')
+            ->with(0)
+            ->will($this->returnValue(new Entry('foo', array(
+                    'sAMAccountName' => array('foo'),
+                    'userpassword' => array('bar'),
+                )
+            )))
+        ;
+        $result
+            ->expects($this->once())
+            ->method('count')
+            ->will($this->returnValue(1))
+        ;
+        $ldap
+            ->expects($this->once())
+            ->method('escape')
+            ->will($this->returnValue('foo'))
+        ;
+        $ldap
+            ->expects($this->once())
+            ->method('query')
+            ->will($this->returnValue($query))
+        ;
+
+        $provider = new LdapUserProvider($ldap, 'ou=MyBusiness,dc=symfony,dc=com', null, null, array(), 'sAMAccountName', '({uid_key}={username})', 'userpassword');
+        $this->assertInstanceOf(
+            'Symfony\Component\Security\Core\User\User',
+            $provider->loadUserByUsername('foo')
+        );
+    }
 }
diff --git a/src/Symfony/Component/Security/Core/User/LdapUserProvider.php b/src/Symfony/Component/Security/Core/User/LdapUserProvider.php
@@ -12,6 +12,7 @@
 namespace Symfony\Component\Security\Core\User;
 
 use Symfony\Component\Ldap\Entry;
+use Symfony\Component\Security\Core\Exception\InvalidArgumentException;
 use Symfony\Component\Security\Core\Exception\UnsupportedUserException;
 use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
 use Symfony\Component\Ldap\Exception\ConnectionException;
@@ -31,6 +32,7 @@ class LdapUserProvider implements UserProviderInterface
     private $searchPassword;
     private $defaultRoles;
     private $defaultSearch;
+    private $passwordAttribute;
 
     /**
      * @param LdapInterface $ldap
@@ -41,14 +43,15 @@ class LdapUserProvider implements UserProviderInterface
      * @param string        $uidKey
      * @param string        $filter
      */
-    public function __construct(LdapInterface $ldap, $baseDn, $searchDn = null, $searchPassword = null, array $defaultRoles = array(), $uidKey = 'sAMAccountName', $filter = '({uid_key}={username})')
+    public function __construct(LdapInterface $ldap, $baseDn, $searchDn = null, $searchPassword = null, array $defaultRoles = array(), $uidKey = 'sAMAccountName', $filter = '({uid_key}={username})', $passwordAttribute = null)
     {
         $this->ldap = $ldap;
         $this->baseDn = $baseDn;
         $this->searchDn = $searchDn;
         $this->searchPassword = $searchPassword;
         $this->defaultRoles = $defaultRoles;
         $this->defaultSearch = str_replace('{uid_key}', $uidKey, $filter);
+        $this->passwordAttribute = $passwordAttribute;
     }
 
     /**
@@ -99,8 +102,42 @@ public function supportsClass($class)
         return $class === 'Symfony\Component\Security\Core\User\User';
     }
 
+    /**
+     * Loads a user from an LDAP entry.
+     *
+     * @param string $username
+     * @param Entry  $entry
+     *
+     * @return User
+     */
     private function loadUser($username, Entry $entry)
     {
-        return new User($username, $entry->getAttribute('userpassword'), $this->defaultRoles);
+        $password = $this->getPassword($entry);
+
+        return new User($username, $password, $this->defaultRoles);
+    }
+
+    /**
+     * Fetches the password from an LDAP entry.
+     *
+     * @param null|Entry $entry
+     */
+    private function getPassword(Entry $entry)
+    {
+        if (null === $this->passwordAttribute) {
+            return;
+        }
+
+        if (!$entry->hasAttribute($this->passwordAttribute)) {
+            throw new InvalidArgumentException(sprintf('Missing attribute "%s" for user "%s".', $this->passwordAttribute, $entry->getDn()));
+        }
+
+        $values = $entry->getAttribute($this->passwordAttribute);
+
+        if (1 !== count($values)) {
+            throw new InvalidArgumentException(sprintf('Attribute "%s" has multiple values.', $this->passwordAttribute));
+        }
+
+        return $values[0];
     }
 }