From 005d264d6031790fae60fd162bbda88d9458870f Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 6 Oct 2016 22:52:45 +0200 Subject: [PATCH] Backport of #1643 to stable10 get rid of test warnings Signed-off-by: Arthur Schiwon cache loginName2UserName and cover the method with unit tests Signed-off-by: Arthur Schiwon adjust tests Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/Access.php | 1 + apps/user_ldap/lib/Exceptions/NotOnLDAP.php | 26 ++++ apps/user_ldap/lib/User_LDAP.php | 21 ++- apps/user_ldap/tests/User_LDAPTest.php | 157 ++++++++++++++++---- 4 files changed, 174 insertions(+), 31 deletions(-) create mode 100644 apps/user_ldap/lib/Exceptions/NotOnLDAP.php diff --git a/apps/user_ldap/lib/Access.php b/apps/user_ldap/lib/Access.php index 299ad58164..3b1370567b 100644 --- a/apps/user_ldap/lib/Access.php +++ b/apps/user_ldap/lib/Access.php @@ -54,6 +54,7 @@ class Access extends LDAPUtility implements IUserTools { * @var \OCA\User_LDAP\Connection */ public $connection; + /** @var Manager */ public $userManager; //never ever check this var directly, always use getPagedSearchResultState protected $pagedSearchedSuccessful; diff --git a/apps/user_ldap/lib/Exceptions/NotOnLDAP.php b/apps/user_ldap/lib/Exceptions/NotOnLDAP.php new file mode 100644 index 0000000000..41fb6c9e71 --- /dev/null +++ b/apps/user_ldap/lib/Exceptions/NotOnLDAP.php @@ -0,0 +1,26 @@ + + * + * @author Arthur Schiwon + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program 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 program. If not, see . + * + */ + +namespace OCA\User_LDAP\Exceptions; + +class NotOnLDAP extends \Exception {} diff --git a/apps/user_ldap/lib/User_LDAP.php b/apps/user_ldap/lib/User_LDAP.php index ce1aafc210..3222c0cb3d 100644 --- a/apps/user_ldap/lib/User_LDAP.php +++ b/apps/user_ldap/lib/User_LDAP.php @@ -36,6 +36,7 @@ namespace OCA\User_LDAP; use OC\User\NoUserException; +use OCA\User_LDAP\Exceptions\NotOnLDAP; use OCA\User_LDAP\User\OfflineUser; use OCA\User_LDAP\User\User; use OCP\IConfig; @@ -80,14 +81,26 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn * @return string|false */ public function loginName2UserName($loginName) { + $cacheKey = 'loginName2UserName-'.$loginName; + $username = $this->access->connection->getFromCache($cacheKey); + if(!is_null($username)) { + return $username; + } + try { $ldapRecord = $this->getLDAPUserByLoginName($loginName); $user = $this->access->userManager->get($ldapRecord['dn'][0]); if($user instanceof OfflineUser) { + // this path is not really possible, however get() is documented + // to return User or OfflineUser so we are very defensive here. + $this->access->connection->writeToCache($cacheKey, false); return false; } - return $user->getUsername(); - } catch (\Exception $e) { + $username = $user->getUsername(); + $this->access->connection->writeToCache($cacheKey, $username); + return $username; + } catch (NotOnLDAP $e) { + $this->access->connection->writeToCache($cacheKey, false); return false; } } @@ -107,14 +120,14 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn * * @param string $loginName * @return array - * @throws \Exception + * @throws NotOnLDAP */ public function getLDAPUserByLoginName($loginName) { //find out dn of the user name $attrs = $this->access->userManager->getAttributes(); $users = $this->access->fetchUsersByLoginName($loginName, $attrs); if(count($users) < 1) { - throw new \Exception('No user available for the given login name on ' . + throw new NotOnLDAP('No user available for the given login name on ' . $this->access->connection->ldapHost . ':' . $this->access->connection->ldapPort); } return $users[0]; diff --git a/apps/user_ldap/tests/User_LDAPTest.php b/apps/user_ldap/tests/User_LDAPTest.php index 47e789142e..13a4f5854c 100644 --- a/apps/user_ldap/tests/User_LDAPTest.php +++ b/apps/user_ldap/tests/User_LDAPTest.php @@ -28,9 +28,10 @@ namespace OCA\User_LDAP\Tests; +use OCA\User_LDAP\Access; +use OCA\User_LDAP\Helper; use OCA\User_LDAP\User_LDAP as UserLDAP; -use \OCA\User_LDAP\Access; -use \OCA\User_LDAP\Connection; +use Test\TestCase; /** * Class Test_User_Ldap_Direct @@ -39,7 +40,7 @@ use \OCA\User_LDAP\Connection; * * @package OCA\User_LDAP\Tests */ -class User_LDAPTest extends \Test\TestCase { +class User_LDAPTest extends TestCase { protected $backend; protected $access; protected $configMock; @@ -51,24 +52,15 @@ class User_LDAPTest extends \Test\TestCase { \OC_Group::clearBackends(); } + /** + * @return \PHPUnit_Framework_MockObject_MockObject|Access + */ private function getAccessMock() { - static $conMethods; - static $accMethods; - static $uMethods; - - if(is_null($conMethods) || is_null($accMethods)) { - $conMethods = get_class_methods('\OCA\User_LDAP\Connection'); - $accMethods = get_class_methods('\OCA\User_LDAP\Access'); - unset($accMethods[array_search('getConnection', $accMethods)]); - $uMethods = get_class_methods('\OCA\User_LDAP\User\User'); - unset($uMethods[array_search('getUsername', $uMethods)]); - unset($uMethods[array_search('getDN', $uMethods)]); - unset($uMethods[array_search('__construct', $uMethods)]); - } $lw = $this->getMock('\OCA\User_LDAP\ILDAPWrapper'); - $connector = $this->getMock('\OCA\User_LDAP\Connection', - $conMethods, - array($lw, null, null)); + $connector = $this->getMockBuilder('\OCA\User_LDAP\Connection') + ->setMethodsExcept(['getConnection']) + ->setConstructorArgs([$lw, null, null]) + ->getMock(); $this->configMock = $this->getMock('\OCP\IConfig'); @@ -76,7 +68,7 @@ class User_LDAPTest extends \Test\TestCase { ->disableOriginalConstructor() ->getMock(); - $um = $this->getMockBuilder('\OCA\User_LDAP\User\Manager') + $um = $this->getMockBuilder('OCA\User_LDAP\User\Manager') ->setMethods(['getDeletedUser']) ->setConstructorArgs([ $this->configMock, @@ -93,11 +85,12 @@ class User_LDAPTest extends \Test\TestCase { ->method('getDeletedUser') ->will($this->returnValue($offlineUser)); - $helper = new \OCA\User_LDAP\Helper(); - - $access = $this->getMock('\OCA\User_LDAP\Access', - $accMethods, - array($connector, $lw, $um, $helper)); + $helper = new Helper(); + + $access = $this->getMockBuilder('OCA\User_LDAP\Access') + ->setMethodsExcept(['getConnection']) + ->setConstructorArgs([$connector, $lw, $um, $helper]) + ->getMock(); $um->setLdapAccess($access); @@ -129,7 +122,7 @@ class User_LDAPTest extends \Test\TestCase { /** * Prepares the Access mock for checkPassword tests - * @param \OCA\User_LDAP\Access $access mock + * @param Access|\PHPUnit_Framework_MockObject_MockObject $access mock * @param bool $noDisplayName * @return void */ @@ -298,7 +291,7 @@ class User_LDAPTest extends \Test\TestCase { /** * Prepares the Access mock for getUsers tests - * @param \OCA\User_LDAP\Access $access mock + * @param Access $access mock * @return void */ private function prepareAccessForGetUsers(&$access) { @@ -842,4 +835,114 @@ class User_LDAPTest extends \Test\TestCase { $result = $backend->countUsers(); $this->assertFalse($result); } + + public function testLoginName2UserNameSuccess() { + $loginName = 'Alice'; + $username = 'alice'; + $dn = 'uid=alice,dc=what,dc=ever'; + + $access = $this->getAccessMock(); + $access->expects($this->once()) + ->method('fetchUsersByLoginName') + ->with($this->equalTo($loginName)) + ->willReturn([['dn' => [$dn]]]); + $access->expects($this->once()) + ->method('stringResemblesDN') + ->with($this->equalTo($dn)) + ->willReturn(true); + $access->expects($this->once()) + ->method('dn2username') + ->with($this->equalTo($dn)) + ->willReturn($username); + + $access->connection->expects($this->exactly(2)) + ->method('getFromCache') + ->with($this->equalTo('loginName2UserName-'.$loginName)) + ->willReturnOnConsecutiveCalls(null, $username); + $access->connection->expects($this->once()) + ->method('writeToCache') + ->with($this->equalTo('loginName2UserName-'.$loginName), $this->equalTo($username)); + + $backend = new UserLDAP($access, $this->createMock('\OCP\IConfig')); + $name = $backend->loginName2UserName($loginName); + $this->assertSame($username, $name); + + // and once again to verify that caching works + $backend->loginName2UserName($loginName); + } + + public function testLoginName2UserNameNoUsersOnLDAP() { + $loginName = 'Loki'; + + $access = $this->getAccessMock(); + $access->expects($this->once()) + ->method('fetchUsersByLoginName') + ->with($this->equalTo($loginName)) + ->willReturn([]); + $access->expects($this->never()) + ->method('stringResemblesDN'); + $access->expects($this->never()) + ->method('dn2username'); + + $access->connection->expects($this->exactly(2)) + ->method('getFromCache') + ->with($this->equalTo('loginName2UserName-'.$loginName)) + ->willReturnOnConsecutiveCalls(null, false); + $access->connection->expects($this->once()) + ->method('writeToCache') + ->with($this->equalTo('loginName2UserName-'.$loginName), false); + + $backend = new UserLDAP($access, $this->createMock('\OCP\IConfig')); + $name = $backend->loginName2UserName($loginName); + $this->assertSame(false, $name); + + // and once again to verify that caching works + $backend->loginName2UserName($loginName); + } + + public function testLoginName2UserNameOfflineUser() { + $loginName = 'Alice'; + $dn = 'uid=alice,dc=what,dc=ever'; + + $offlineUser = $this->getMockBuilder('OCA\User_LDAP\User\OfflineUser') + ->disableOriginalConstructor() + ->getMock(); + + $access = $this->getAccessMock(); + $access->expects($this->once()) + ->method('fetchUsersByLoginName') + ->with($this->equalTo($loginName)) + ->willReturn([['dn' => [$dn]]]); + $access->expects($this->once()) + ->method('stringResemblesDN') + ->with($this->equalTo($dn)) + ->willReturn(true); + $access->expects($this->once()) + ->method('dn2username') + ->willReturn(false); // this is fake, but allows us to force-enter the OfflineUser path + + $access->connection->expects($this->exactly(2)) + ->method('getFromCache') + ->with($this->equalTo('loginName2UserName-'.$loginName)) + ->willReturnOnConsecutiveCalls(null, false); + $access->connection->expects($this->once()) + ->method('writeToCache') + ->with($this->equalTo('loginName2UserName-'.$loginName), $this->equalTo(false)); + + $access->userManager->expects($this->once()) + ->method('getDeletedUser') + ->will($this->returnValue($offlineUser)); + + $this->configMock->expects($this->once()) + ->method('getUserValue') + ->willReturn(1); + + $backend = new UserLDAP($access, $this->createMock('\OCP\IConfig')); + $name = $backend->loginName2UserName($loginName); + $this->assertSame(false, $name); + + // and once again to verify that caching works + $backend->loginName2UserName($loginName); + } + }