From 979612fa08caf287641586a372c12c96acdf1c3f Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 6 Oct 2016 22:52:45 +0200 Subject: [PATCH 1/2] get rid of test warnings Signed-off-by: Arthur Schiwon --- apps/user_ldap/tests/User_LDAPTest.php | 43 +++++++++++++------------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/apps/user_ldap/tests/User_LDAPTest.php b/apps/user_ldap/tests/User_LDAPTest.php index f5ffb7f990..f2be36ed12 100644 --- a/apps/user_ldap/tests/User_LDAPTest.php +++ b/apps/user_ldap/tests/User_LDAPTest.php @@ -28,15 +28,20 @@ namespace OCA\User_LDAP\Tests; +use OCA\User_LDAP\Access; +use OCA\User_LDAP\Connection; use OCA\User_LDAP\FilesystemHelper; +use OCA\User_LDAP\Helper; use OCA\User_LDAP\ILDAPWrapper; use OCA\User_LDAP\LogWrapper; +use OCA\User_LDAP\User\Manager; use OCA\User_LDAP\User_LDAP as UserLDAP; use OCP\IAvatarManager; use OCP\IConfig; use OCP\IDBConnection; use OCP\Image; use OCP\IUserManager; +use Test\TestCase; /** * Class Test_User_Ldap_Direct @@ -45,7 +50,7 @@ use OCP\IUserManager; * * @package OCA\User_LDAP\Tests */ -class User_LDAPTest extends \Test\TestCase { +class User_LDAPTest extends TestCase { protected $backend; protected $access; protected $configMock; @@ -58,23 +63,17 @@ class User_LDAPTest extends \Test\TestCase { } 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'); + if(is_null($accMethods)) { + $accMethods = get_class_methods(Access::class); 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->createMock(ILDAPWrapper::class); - $connector = $this->getMock('\OCA\User_LDAP\Connection', - $conMethods, - array($lw, null, null)); + $connector = $this->getMockBuilder(Connection::class) + ->setMethodsExcept(['getConnection']) + ->setConstructorArgs([$lw, null, null]) + ->getMock(); $this->configMock = $this->createMock(IConfig::class); @@ -82,7 +81,8 @@ class User_LDAPTest extends \Test\TestCase { ->disableOriginalConstructor() ->getMock(); - $um = $this->getMockBuilder('\OCA\User_LDAP\User\Manager') + /** @var Manager|\PHPUnit_Framework_MockObject_MockObject $um */ + $um = $this->getMockBuilder(Manager::class) ->setMethods(['getDeletedUser']) ->setConstructorArgs([ $this->configMock, @@ -99,11 +99,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(Access::class) + ->setMethodsExcept(['getConnection']) + ->setConstructorArgs([$connector, $lw, $um, $helper]) + ->getMock(); $um->setLdapAccess($access); @@ -135,7 +136,7 @@ class User_LDAPTest extends \Test\TestCase { /** * Prepares the Access mock for checkPassword tests - * @param \OCA\User_LDAP\Access $access mock + * @param Access $access mock * @param bool $noDisplayName * @return void */ @@ -304,7 +305,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) { From a30341823ed7800e55211e07c0bc62ce6cba2e0b Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 7 Oct 2016 01:39:57 +0200 Subject: [PATCH 2/2] cache loginName2UserName and cover the method with unit 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 | 126 ++++++++++++++++++-- 4 files changed, 162 insertions(+), 12 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 12d71b1528..96b6bae64b 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 13e61c63c0..e765814930 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 f2be36ed12..5859e51ec6 100644 --- a/apps/user_ldap/tests/User_LDAPTest.php +++ b/apps/user_ldap/tests/User_LDAPTest.php @@ -35,6 +35,8 @@ use OCA\User_LDAP\Helper; use OCA\User_LDAP\ILDAPWrapper; use OCA\User_LDAP\LogWrapper; use OCA\User_LDAP\User\Manager; +use OCA\User_LDAP\User\OfflineUser; +use OCA\User_LDAP\User\User; use OCA\User_LDAP\User_LDAP as UserLDAP; use OCP\IAvatarManager; use OCP\IConfig; @@ -62,13 +64,10 @@ class User_LDAPTest extends TestCase { \OC_Group::clearBackends(); } + /** + * @return \PHPUnit_Framework_MockObject_MockObject|Access + */ private function getAccessMock() { - static $accMethods; - - if(is_null($accMethods)) { - $accMethods = get_class_methods(Access::class); - unset($accMethods[array_search('getConnection', $accMethods)]); - } $lw = $this->createMock(ILDAPWrapper::class); $connector = $this->getMockBuilder(Connection::class) ->setMethodsExcept(['getConnection']) @@ -81,7 +80,6 @@ class User_LDAPTest extends TestCase { ->disableOriginalConstructor() ->getMock(); - /** @var Manager|\PHPUnit_Framework_MockObject_MockObject $um */ $um = $this->getMockBuilder(Manager::class) ->setMethods(['getDeletedUser']) ->setConstructorArgs([ @@ -136,7 +134,7 @@ class User_LDAPTest extends TestCase { /** * Prepares the Access mock for checkPassword tests - * @param Access $access mock + * @param Access|\PHPUnit_Framework_MockObject_MockObject $access mock * @param bool $noDisplayName * @return void */ @@ -849,4 +847,116 @@ class User_LDAPTest extends 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(IConfig::class)); + $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(IConfig::class)); + $name = $backend->loginName2UserName($loginName); + $this->assertSame(false, $name); + + // and once again to verify that caching works + $backend->loginName2UserName($loginName); + } + + public function testLoginName2UserNameOfflineUser() { + $loginName = 'Alice'; + $username = 'alice'; + $dn = 'uid=alice,dc=what,dc=ever'; + + $offlineUser = $this->getMockBuilder(OfflineUser::class) + ->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)); + + //$config = $this->createMock(IConfig::class); + $this->configMock->expects($this->once()) + ->method('getUserValue') + ->willReturn(1); + + $backend = new UserLDAP($access, $this->createMock(IConfig::class)); + $name = $backend->loginName2UserName($loginName); + $this->assertSame(false, $name); + + // and once again to verify that caching works + $backend->loginName2UserName($loginName); + } + }