Merge pull request #7837 from owncloud/fix_7530

LDAP: fix determining objectclasses takes long

2 files changed, 237 insertions, 7 deletions

diff --git a/apps/user_ldap/lib/wizard.php b/apps/user_ldap/lib/wizard.php
index e79090febc1..3854af617c1 100644
--- a/apps/user_ldap/lib/wizard.php
+++ b/apps/user_ldap/lib/wizard.php
@@ -869,12 +869,14 @@ class Wizard extends LDAPUtility {
 	 * @param string $attr the attribute of which a list of values shall be returned
 	 * @param $lfw bool, whether the last filter is a wildcard which shall not
 	 * be processed if there were already findings, defaults to true
+	 * @param int $dnReadLimit the amount of how many DNs should be analyzed.
+	 * The lower, the faster
 	 * @param string $maxF string. if not null, this variable will have the filter that
 	 * yields most result entries
 	 * @return mixed, an array with the values on success, false otherwise
 	 *
 	 */
-	private function cumulativeSearchOnAttribute($filters, $attr, $lfw = true, &$maxF = null) {
+	public function cumulativeSearchOnAttribute($filters, $attr, $lfw = true, $dnReadLimit = 3, &$maxF = null) {
 		$dnRead = array();
 		$foundItems = array();
 		$maxEntries = 0;
@@ -884,11 +886,16 @@ class Wizard extends LDAPUtility {
 		}
 		$base = $this->configuration->ldapBase[0];
 		$cr = $this->getConnection();
-		if(!is_resource($cr)) {
+		if(!$this->ldap->isResource($cr)) {
 			return false;
 		}
+		$lastFilter = null;
+		if(isset($filters[count($filters)-1])) {
+			$lastFilter = $filters[count($filters)-1];
+		}
 		foreach($filters as $filter) {
-			if($lfw && count($foundItems) > 0) {
+			if($lfw && $lastFilter === $filter && count($foundItems) > 0) {
+				//skip when the filter is a wildcard and results were found
 				continue;
 			}
 			$rr = $this->ldap->search($cr, $base, $filter, array($attr));
@@ -902,8 +909,10 @@ class Wizard extends LDAPUtility {
 					$maxEntries = $entries;
 					$maxF = $filter;
 				}
+				$dnReadCount = 0;
 				do {
 					$entry = $this->ldap->$getEntryFunc($cr, $rr);
+					$getEntryFunc = 'nextEntry';
 					if(!$this->ldap->isResource($entry)) {
 						continue 2;
 					}
@@ -916,13 +925,14 @@ class Wizard extends LDAPUtility {
 					$state = $this->getAttributeValuesFromEntry($attributes,
 																$attr,
 																$newItems);
+					$dnReadCount++;
 					$foundItems = array_merge($foundItems, $newItems);
 					$this->resultCache[$dn][$attr] = $newItems;
 					$dnRead[] = $dn;
-					$getEntryFunc = 'nextEntry';
 					$rr = $entry; //will be expected by nextEntry next round
-				} while($state === self::LRESULT_PROCESSED_SKIP
-						|| $this->ldap->isResource($entry));
+				} while(($state === self::LRESULT_PROCESSED_SKIP
+						|| $this->ldap->isResource($entry))
+						&& ($dnReadLimit === 0 || $dnReadCount < $dnReadLimit));
 			}
 		}
 
@@ -950,9 +960,19 @@ class Wizard extends LDAPUtility {
 			$objectclasses[$key] = $p.$value;
 		}
 		$maxEntryObjC = '';
+
+		//how deep to dig?
+		//When looking for objectclasses, testing few entries is sufficient,
+		//when looking for group we need to get all names, though.
+		if(strtolower($attr) === 'objectclass') {
+			$dig = 3;
+		} else {
+			$dig = 0;
+		}
+
 		$availableFeatures =
 			$this->cumulativeSearchOnAttribute($objectclasses, $attr,
-											   true, $maxEntryObjC);
+											   true, $dig, $maxEntryObjC);
 		if(is_array($availableFeatures)
 		   && count($availableFeatures) > 0) {
 			natcasesort($availableFeatures);
diff --git a/apps/user_ldap/tests/wizard.php b/apps/user_ldap/tests/wizard.php
new file mode 100644
index 00000000000..2b5cabc705d
--- /dev/null
+++ b/apps/user_ldap/tests/wizard.php
@@ -0,0 +1,210 @@
+<?php
+/**
+* ownCloud
+*
+* @author Arthur Schiwon
+* @copyright 2014 Arthur Schiwon blizzz@owncloud.com
+*
+* This library is free software; you can redistribute it and/or
+* modify it under the terms of the GNU AFFERO GENERAL PUBLIC LICENSE
+* License as published by the Free Software Foundation; either
+* version 3 of the License, or any later version.
+*
+* This library is distributed in the hope that it will be useful,
+* but WITHOUT ANY WARRANTY; without even the implied warranty of
+* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+* GNU AFFERO GENERAL PUBLIC LICENSE for more details.
+*
+* You should have received a copy of the GNU Affero General Public
+* License along with this library.  If not, see <http://www.gnu.org/licenses/>.
+*
+*/
+
+namespace OCA\user_ldap\tests;
+
+use \OCA\user_ldap\lib\Wizard;
+
+// use \OCA\user_ldap\USER_LDAP as UserLDAP;
+// use \OCA\user_ldap\lib\Access;
+// use \OCA\user_ldap\lib\Configuration;
+// use \OCA\user_ldap\lib\ILDAPWrapper;
+
+class Test_Wizard extends \PHPUnit_Framework_TestCase {
+	public function setUp() {
+		//we need to make sure the consts are defined, otherwise tests will fail
+		//on systems without php5_ldap
+		$ldapConsts = array('LDAP_OPT_PROTOCOL_VERSION',
+							'LDAP_OPT_REFERRALS', 'LDAP_OPT_NETWORK_TIMEOUT');
+		foreach($ldapConsts as $const) {
+			if(!defined($const)) {
+				define($const, 42);
+			}
+		}
+	}
+
+	private function getWizardAndMocks() {
+		static $conMethods;
+
+		if(is_null($conMethods)) {
+			$conMethods = get_class_methods('\OCA\user_ldap\lib\Configuration');
+		}
+		$lw   = $this->getMock('\OCA\user_ldap\lib\ILDAPWrapper');
+		$conf = $this->getMock('\OCA\user_ldap\lib\Configuration',
+							   $conMethods,
+							   array($lw, null, null));
+		return array(new Wizard($conf, $lw), $conf, $lw);
+	}
+
+	private function prepareLdapWrapperForConnections(&$ldap) {
+		$ldap->expects($this->once())
+			->method('connect')
+			//dummy value, usually invalid
+			->will($this->returnValue(true));
+
+		$ldap->expects($this->exactly(3))
+			->method('setOption')
+			->will($this->returnValue(true));
+
+		$ldap->expects($this->once())
+			->method('bind')
+			->will($this->returnValue(true));
+
+	}
+
+	public function testCumulativeSearchOnAttributeLimited() {
+		list($wizard, $configuration, $ldap) = $this->getWizardAndMocks();
+
+		$configuration->expects($this->any())
+			->method('__get')
+			->will($this->returnCallback(function($name) {
+					if($name === 'ldapBase') {
+						return array('base');
+					}
+					return null;
+			   }));
+
+		$this->prepareLdapWrapperForConnections($ldap);
+
+		$ldap->expects($this->any())
+			->method('isResource')
+			->will($this->returnValue(true));
+
+		$ldap->expects($this->exactly(2))
+			->method('search')
+			//dummy value, usually invalid
+			->will($this->returnValue(true));
+
+		$ldap->expects($this->exactly(2))
+			->method('countEntries')
+			//an is_resource check will follow, so we need to return a dummy resource
+			->will($this->returnValue(23));
+
+		//5 DNs per filter means 2x firstEntry and 8x nextEntry
+		$ldap->expects($this->exactly(2))
+			->method('firstEntry')
+			//dummy value, usually invalid
+			->will($this->returnValue(true));
+
+		$ldap->expects($this->exactly(8))
+			->method('nextEntry')
+			//dummy value, usually invalid
+			->will($this->returnValue(true));
+
+		$ldap->expects($this->exactly(10))
+			->method('getAttributes')
+			//dummy value, usually invalid
+			->will($this->returnValue(array('cn' => array('foo'), 'count' => 1)));
+
+		global $uidnumber;
+		$uidnumber = 1;
+		$ldap->expects($this->exactly(10))
+			->method('getDN')
+			//dummy value, usually invalid
+			->will($this->returnCallback(function($a, $b) {
+				global $uidnumber;
+				return $uidnumber++;
+			}));
+
+		# The following expectations are the real test #
+		$filters = array('f1', 'f2', '*');
+		$wizard->cumulativeSearchOnAttribute($filters, 'cn', true, 5);
+		unset($uidnumber);
+	}
+
+	public function testCumulativeSearchOnAttributeUnlimited() {
+		list($wizard, $configuration, $ldap) = $this->getWizardAndMocks();
+
+		$configuration->expects($this->any())
+			->method('__get')
+			->will($this->returnCallback(function($name) {
+					if($name === 'ldapBase') {
+						return array('base');
+					}
+					return null;
+			   }));
+
+		$this->prepareLdapWrapperForConnections($ldap);
+
+		$ldap->expects($this->any())
+			->method('isResource')
+			->will($this->returnCallback(function($r) {
+				if($r === true) {
+					return true;
+				}
+				if($r % 24 === 0) {
+					global $uidnumber;
+					$uidnumber++;
+					return false;
+				}
+				return true;
+			}));
+
+		$ldap->expects($this->exactly(2))
+			->method('search')
+			//dummy value, usually invalid
+			->will($this->returnValue(true));
+
+		$ldap->expects($this->exactly(2))
+			->method('countEntries')
+			//an is_resource check will follow, so we need to return a dummy resource
+			->will($this->returnValue(23));
+
+		//5 DNs per filter means 2x firstEntry and 8x nextEntry
+		$ldap->expects($this->exactly(2))
+			->method('firstEntry')
+			//dummy value, usually invalid
+			->will($this->returnCallback(function($r) {
+				global $uidnumber;
+				return $uidnumber;
+			}));
+
+		$ldap->expects($this->exactly(46))
+			->method('nextEntry')
+			//dummy value, usually invalid
+			->will($this->returnCallback(function($r) {
+				global $uidnumber;
+				return $uidnumber;
+			}));
+
+		$ldap->expects($this->exactly(46))
+			->method('getAttributes')
+			//dummy value, usually invalid
+			->will($this->returnValue(array('cn' => array('foo'), 'count' => 1)));
+
+		global $uidnumber;
+		$uidnumber = 1;
+		$ldap->expects($this->exactly(46))
+			->method('getDN')
+			//dummy value, usually invalid
+			->will($this->returnCallback(function($a, $b) {
+				global $uidnumber;
+				return $uidnumber++;
+			}));
+
+		# The following expectations are the real test #
+		$filters = array('f1', 'f2', '*');
+		$wizard->cumulativeSearchOnAttribute($filters, 'cn', true, 0);
+		unset($uidnumber);
+	}
+
+}
\ No newline at end of file