bug #20605 [Ldap] Always have a valid connection when using the Entry…

…Manager (bobvandevijver) This PR was submitted for the 3.2 branch but it was merged into the 3.1 branch instead (closes #20605). Discussion ---------- [Ldap] Always have a valid connection when using the EntryManager | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | They should | Fixed tickets | | License | MIT | Doc PR | The connection might be `null` when calling the `getEntryManager` function as it uses the `$this->connection` instead of retrieved it from it's own method which checks (and constructs if necessary) it first. Commits ------- 7775ec2 [Ldap] Always have a valid connection when using the EntryManager
symfony · Jan 12, 2017 · 5c68c69 · 5c68c69
2 parents 027d588 + 7775ec2
commit 5c68c69
Show file tree

Hide file tree

Showing 8 changed files with 109 additions and 7 deletions.
diff --git a/src/Symfony/Component/Ldap/Adapter/EntryManagerInterface.php b/src/Symfony/Component/Ldap/Adapter/EntryManagerInterface.php
@@ -12,32 +12,44 @@
 namespace Symfony\Component\Ldap\Adapter;
 
 use Symfony\Component\Ldap\Entry;
+use Symfony\Component\Ldap\Exception\LdapException;
+use Symfony\Component\Ldap\Exception\NotBoundException;
 
 /**
  * Entry manager interface.
  *
  * @author Charles Sarrazin <charles@sarraz.in>
+ * @author Bob van de Vijver <bobvandevijver@hotmail.com>
  */
 interface EntryManagerInterface
 {
     /**
      * Adds a new entry in the Ldap server.
      *
      * @param Entry $entry
+     *
+     * @throws NotBoundException
+     * @throws LdapException
      */
     public function add(Entry $entry);
 
     /**
      * Updates an entry from the Ldap server.
      *
      * @param Entry $entry
+     *
+     * @throws NotBoundException
+     * @throws LdapException
      */
     public function update(Entry $entry);
 
     /**
      * Removes an entry from the Ldap server.
      *
      * @param Entry $entry
+     *
+     * @throws NotBoundException
+     * @throws LdapException
      */
     public function remove(Entry $entry);
 }
diff --git a/src/Symfony/Component/Ldap/Adapter/ExtLdap/Adapter.php b/src/Symfony/Component/Ldap/Adapter/ExtLdap/Adapter.php
@@ -50,7 +50,7 @@ public function getConnection()
     public function getEntryManager()
     {
         if (null === $this->entryManager) {
-            $this->entryManager = new EntryManager($this->connection);
+            $this->entryManager = new EntryManager($this->getConnection());
         }
 
         return $this->entryManager;

diff --git a/src/Symfony/Component/Ldap/Adapter/ExtLdap/EntryManager.php b/src/Symfony/Component/Ldap/Adapter/ExtLdap/EntryManager.php
@@ -14,9 +14,11 @@
 use Symfony\Component\Ldap\Adapter\EntryManagerInterface;
 use Symfony\Component\Ldap\Entry;
 use Symfony\Component\Ldap\Exception\LdapException;
+use Symfony\Component\Ldap\Exception\NotBoundException;
 
 /**
  * @author Charles Sarrazin <charles@sarraz.in>
+ * @author Bob van de Vijver <bobvandevijver@hotmail.com>
  */
 class EntryManager implements EntryManagerInterface
 {
@@ -32,7 +34,7 @@ public function __construct(Connection $connection)
      */
     public function add(Entry $entry)
     {
-        $con = $this->connection->getResource();
+        $con = $this->getConnectionResource();
 
         if (!@ldap_add($con, $entry->getDn(), $entry->getAttributes())) {
             throw new LdapException(sprintf('Could not add entry "%s": %s', $entry->getDn(), ldap_error($con)));
@@ -46,7 +48,7 @@ public function add(Entry $entry)
      */
     public function update(Entry $entry)
     {
-        $con = $this->connection->getResource();
+        $con = $this->getConnectionResource();
 
         if (!@ldap_modify($con, $entry->getDn(), $entry->getAttributes())) {
             throw new LdapException(sprintf('Could not update entry "%s": %s', $entry->getDn(), ldap_error($con)));
@@ -58,10 +60,23 @@ public function update(Entry $entry)
      */
     public function remove(Entry $entry)
     {
-        $con = $this->connection->getResource();
+        $con = $this->getConnectionResource();
 
         if (!@ldap_delete($con, $entry->getDn())) {
             throw new LdapException(sprintf('Could not remove entry "%s": %s', $entry->getDn(), ldap_error($con)));
         }
     }
+
+    /**
+     * Get the connection resource, but first check if the connection is bound.
+     */
+    private function getConnectionResource()
+    {
+        // If the connection is not bound, throw an exception. Users should use an explicit bind call first.
+        if (!$this->connection->isBound()) {
+            throw new NotBoundException('Query execution is not possible without binding the connection first.');
+        }
+
+        return $this->connection->getResource();
+    }
 }
diff --git a/src/Symfony/Component/Ldap/Adapter/ExtLdap/Query.php b/src/Symfony/Component/Ldap/Adapter/ExtLdap/Query.php
@@ -13,13 +13,15 @@
 
 use Symfony\Component\Ldap\Adapter\AbstractQuery;
 use Symfony\Component\Ldap\Exception\LdapException;
+use Symfony\Component\Ldap\Exception\NotBoundException;
 
 /**
  * @author Charles Sarrazin <charles@sarraz.in>
+ * @author Bob van de Vijver <bobvandevijver@hotmail.com>
  */
 class Query extends AbstractQuery
 {
-    /** @var  Connection */
+    /** @var Connection */
     protected $connection;
 
     /** @var resource */
@@ -53,9 +55,9 @@ public function __destruct()
     public function execute()
     {
         if (null === $this->search) {
-            // If the connection is not bound, then we try an anonymous bind.
+            // If the connection is not bound, throw an exception. Users should use an explicit bind call first.
             if (!$this->connection->isBound()) {
-                $this->connection->bind();
+                throw new NotBoundException('Query execution is not possible without binding the connection first.');
             }
 
             $con = $this->connection->getResource();

diff --git a/src/Symfony/Component/Ldap/Adapter/QueryInterface.php b/src/Symfony/Component/Ldap/Adapter/QueryInterface.php
@@ -12,9 +12,12 @@
 namespace Symfony\Component\Ldap\Adapter;
 
 use Symfony\Component\Ldap\Entry;
+use Symfony\Component\Ldap\Exception\LdapException;
+use Symfony\Component\Ldap\Exception\NotBoundException;
 
 /**
  * @author Charles Sarrazin <charles@sarraz.in>
+ * @author Bob van de Vijver <bobvandevijver@hotmail.com>
  */
 interface QueryInterface
 {
@@ -27,6 +30,9 @@ interface QueryInterface
      * Executes a query and returns the list of Ldap entries.
      *
      * @return CollectionInterface|Entry[]
+     *
+     * @throws NotBoundException
+     * @throws LdapException
      */
     public function execute();
 }
diff --git a/src/Symfony/Component/Ldap/Exception/NotBoundException.php b/src/Symfony/Component/Ldap/Exception/NotBoundException.php
@@ -0,0 +1,21 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Ldap\Exception;
+
+/**
+ * NotBoundException is thrown if the connection with the LDAP server is not yet bound.
+ *
+ * @author Bob van de Vijver <bobvandevijver@hotmail.com>
+ */
+class NotBoundException extends \RuntimeException implements ExceptionInterface
+{
+}
diff --git a/src/Symfony/Component/Ldap/Tests/Adapter/ExtLdap/AdapterTest.php b/src/Symfony/Component/Ldap/Tests/Adapter/ExtLdap/AdapterTest.php
@@ -14,6 +14,7 @@
 use Symfony\Component\Ldap\Adapter\ExtLdap\Adapter;
 use Symfony\Component\Ldap\Adapter\ExtLdap\Collection;
 use Symfony\Component\Ldap\Entry;
+use Symfony\Component\Ldap\Exception\NotBoundException;
 use Symfony\Component\Ldap\LdapInterface;
 
 /**
@@ -65,4 +66,15 @@ public function testLdapQueryIterator()
         $this->assertEquals(array('Fabien Potencier'), $entry->getAttribute('cn'));
         $this->assertEquals(array('fabpot@symfony.com', 'fabien@potencier.com'), $entry->getAttribute('mail'));
     }
+
+    /**
+     * @group functional
+     */
+    public function testLdapQueryWithoutBind()
+    {
+        $ldap = new Adapter($this->getLdapConfig());
+        $this->setExpectedException(NotBoundException::class);
+        $query = $ldap->createQuery('dc=symfony,dc=com', '(&(objectclass=person)(ou=Maintainers))', array());
+        $query->execute();
+    }
 }
diff --git a/src/Symfony/Component/Ldap/Tests/Adapter/ExtLdap/LdapManagerTest.php b/src/Symfony/Component/Ldap/Tests/Adapter/ExtLdap/LdapManagerTest.php
@@ -15,6 +15,7 @@
 use Symfony\Component\Ldap\Adapter\ExtLdap\Collection;
 use Symfony\Component\Ldap\Entry;
 use Symfony\Component\Ldap\Exception\LdapException;
+use Symfony\Component\Ldap\Exception\NotBoundException;
 
 /**
  * @requires extension ldap
@@ -98,6 +99,39 @@ public function testLdapUpdate()
         $this->assertNull($entry->getAttribute('email'));
     }
 
+    /**
+     * @group functional
+     */
+    public function testLdapUnboundAdd()
+    {
+        $this->adapter = new Adapter($this->getLdapConfig());
+        $this->setExpectedException(NotBoundException::class);
+        $em = $this->adapter->getEntryManager();
+        $em->add(new Entry(''));
+    }
+
+    /**
+     * @group functional
+     */
+    public function testLdapUnboundRemove()
+    {
+        $this->adapter = new Adapter($this->getLdapConfig());
+        $this->setExpectedException(NotBoundException::class);
+        $em = $this->adapter->getEntryManager();
+        $em->remove(new Entry(''));
+    }
+
+    /**
+     * @group functional
+     */
+    public function testLdapUnboundUpdate()
+    {
+        $this->adapter = new Adapter($this->getLdapConfig());
+        $this->setExpectedException(NotBoundException::class);
+        $em = $this->adapter->getEntryManager();
+        $em->update(new Entry(''));
+    }
+
     /**
      * @return Collection|Entry[]
      */