From 057c2638e4d1c369f03739faad02d9d6c87e83f7 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 20 Mar 2014 13:58:08 +0100 Subject: LDAP Wizard: when determining objectclasses, we realy do not need to look at every entry. Fixes #7530 --- apps/user_ldap/lib/wizard.php | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) (limited to 'apps') diff --git a/apps/user_ldap/lib/wizard.php b/apps/user_ldap/lib/wizard.php index e79090febc1..930db5c4743 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) { + private function cumulativeSearchOnAttribute($filters, $attr, $lfw = true, $dnReadLimit = 5, &$maxF = null) { $dnRead = array(); $foundItems = array(); $maxEntries = 0; @@ -902,6 +904,7 @@ class Wizard extends LDAPUtility { $maxEntries = $entries; $maxF = $filter; } + $dnReadCount = 0; do { $entry = $this->ldap->$getEntryFunc($cr, $rr); if(!$this->ldap->isResource($entry)) { @@ -916,13 +919,15 @@ 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 +955,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 = 5; + } else { + $dig = 0; + } + $availableFeatures = $this->cumulativeSearchOnAttribute($objectclasses, $attr, - true, $maxEntryObjC); + true, $dig, $maxEntryObjC); if(is_array($availableFeatures) && count($availableFeatures) > 0) { natcasesort($availableFeatures); -- cgit v1.2.3 From bd881348e86670332abb0ccb8cd6a71543ee3fd1 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 20 Mar 2014 22:57:50 +0100 Subject: Fix wildcard handling and check even less DNs per filter, enough will be looked at anyway --- apps/user_ldap/lib/wizard.php | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) (limited to 'apps') diff --git a/apps/user_ldap/lib/wizard.php b/apps/user_ldap/lib/wizard.php index 930db5c4743..4f7eee5a989 100644 --- a/apps/user_ldap/lib/wizard.php +++ b/apps/user_ldap/lib/wizard.php @@ -876,7 +876,7 @@ class Wizard extends LDAPUtility { * @return mixed, an array with the values on success, false otherwise * */ - private function cumulativeSearchOnAttribute($filters, $attr, $lfw = true, $dnReadLimit = 5, &$maxF = null) { + public function cumulativeSearchOnAttribute($filters, $attr, $lfw = true, $dnReadLimit = 3, &$maxF = null) { $dnRead = array(); $foundItems = array(); $maxEntries = 0; @@ -889,8 +889,11 @@ class Wizard extends LDAPUtility { if(!is_resource($cr)) { return false; } + 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) { continue; } $rr = $this->ldap->search($cr, $base, $filter, array($attr)); @@ -927,7 +930,7 @@ class Wizard extends LDAPUtility { $rr = $entry; //will be expected by nextEntry next round } while(($state === self::LRESULT_PROCESSED_SKIP || $this->ldap->isResource($entry)) - && ($dnReadLimit === 0 || $dnReadCount <= $dnReadLimit)); + && ($dnReadLimit === 0 || $dnReadCount < $dnReadLimit)); } } @@ -960,7 +963,7 @@ class Wizard extends LDAPUtility { //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 = 5; + $dig = 3; } else { $dig = 0; } -- cgit v1.2.3 From 561d699ca67c0f5d3130b80e238c7ace8ba57e42 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 20 Mar 2014 22:58:57 +0100 Subject: Use the LDAP wrapper for checking resources, needs for proper testing --- apps/user_ldap/lib/wizard.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'apps') diff --git a/apps/user_ldap/lib/wizard.php b/apps/user_ldap/lib/wizard.php index 4f7eee5a989..f97ab51f0f0 100644 --- a/apps/user_ldap/lib/wizard.php +++ b/apps/user_ldap/lib/wizard.php @@ -886,7 +886,7 @@ class Wizard extends LDAPUtility { } $base = $this->configuration->ldapBase[0]; $cr = $this->getConnection(); - if(!is_resource($cr)) { + if(!$this->ldap->isResource($cr)) { return false; } if(isset($filters[count($filters)-1])) { -- cgit v1.2.3 From a085d7715f7fda43f322eb36143b268add3f2061 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 20 Mar 2014 22:59:41 +0100 Subject: fix potential infinite loop when the DN of the first entry was already read. --- apps/user_ldap/lib/wizard.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'apps') diff --git a/apps/user_ldap/lib/wizard.php b/apps/user_ldap/lib/wizard.php index f97ab51f0f0..f6bc586a7a6 100644 --- a/apps/user_ldap/lib/wizard.php +++ b/apps/user_ldap/lib/wizard.php @@ -910,6 +910,7 @@ class Wizard extends LDAPUtility { $dnReadCount = 0; do { $entry = $this->ldap->$getEntryFunc($cr, $rr); + $getEntryFunc = 'nextEntry'; if(!$this->ldap->isResource($entry)) { continue 2; } @@ -926,7 +927,6 @@ class Wizard extends LDAPUtility { $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)) -- cgit v1.2.3 From 3d2ab76347b04b1d7e04684265b5d7ee678a2b38 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 21 Mar 2014 00:17:51 +0100 Subject: add tests for cumulativeSearchOnAttribute --- apps/user_ldap/tests/wizard.php | 198 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 198 insertions(+) create mode 100644 apps/user_ldap/tests/wizard.php (limited to 'apps') diff --git a/apps/user_ldap/tests/wizard.php b/apps/user_ldap/tests/wizard.php new file mode 100644 index 00000000000..f3178332ae5 --- /dev/null +++ b/apps/user_ldap/tests/wizard.php @@ -0,0 +1,198 @@ +. +* +*/ + +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 { + 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 -- cgit v1.2.3 From 6f605ecd671e7e6d5e9734dc74c3b9480613c090 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 21 Mar 2014 10:08:17 +0100 Subject: make tests work on systems without php5_ldap --- apps/user_ldap/tests/wizard.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) (limited to 'apps') diff --git a/apps/user_ldap/tests/wizard.php b/apps/user_ldap/tests/wizard.php index f3178332ae5..2b5cabc705d 100644 --- a/apps/user_ldap/tests/wizard.php +++ b/apps/user_ldap/tests/wizard.php @@ -30,6 +30,18 @@ use \OCA\user_ldap\lib\Wizard; // 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; -- cgit v1.2.3 From 4230983e69906de916c73e06204ff87df72bfce9 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 21 Mar 2014 10:10:49 +0100 Subject: define var --- apps/user_ldap/lib/wizard.php | 1 + 1 file changed, 1 insertion(+) (limited to 'apps') diff --git a/apps/user_ldap/lib/wizard.php b/apps/user_ldap/lib/wizard.php index f6bc586a7a6..54454d2ad22 100644 --- a/apps/user_ldap/lib/wizard.php +++ b/apps/user_ldap/lib/wizard.php @@ -889,6 +889,7 @@ class Wizard extends LDAPUtility { if(!$this->ldap->isResource($cr)) { return false; } + $lastFilter = null; if(isset($filters[count($filters)-1])) { $lastFilter = $filters[count($filters)-1]; } -- cgit v1.2.3 From dbebf6bb5e94615ef8986b812e2ea6ac14862a3f Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 9 Apr 2014 12:25:48 +0200 Subject: add comment to clearify when a skip in the foreach happens --- apps/user_ldap/lib/wizard.php | 1 + 1 file changed, 1 insertion(+) (limited to 'apps') diff --git a/apps/user_ldap/lib/wizard.php b/apps/user_ldap/lib/wizard.php index 54454d2ad22..3854af617c1 100644 --- a/apps/user_ldap/lib/wizard.php +++ b/apps/user_ldap/lib/wizard.php @@ -895,6 +895,7 @@ class Wizard extends LDAPUtility { } foreach($filters as $filter) { 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)); -- cgit v1.2.3