Merge pull request #1643 from nextcloud/ldap_more_caching

cache loginName2UserName and cover the method with unit tests

4 files changed, 180 insertions, 29 deletions

diff --git a/apps/user_ldap/lib/Access.php b/apps/user_ldap/lib/Access.php
index 12d71b1528a..96b6bae64bd 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 00000000000..41fb6c9e713
--- /dev/null
+++ b/apps/user_ldap/lib/Exceptions/NotOnLDAP.php
@@ -0,0 +1,26 @@
+<?php
+/**
+ * @copyright Copyright (c) 2016 Arthur Schiwon <blizzz@arthur-schiwon.de>
+ *
+ * @author Arthur Schiwon <blizzz@arthur-schiwon.de>
+ *
+ * @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 <http://www.gnu.org/licenses/>.
+ *
+ */
+
+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 13e61c63c08..e7658149302 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 f5ffb7f9907..5859e51ec66 100644
--- a/apps/user_ldap/tests/User_LDAPTest.php
+++ b/apps/user_ldap/tests/User_LDAPTest.php
@@ -28,15 +28,22 @@
 
 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\OfflineUser;
+use OCA\User_LDAP\User\User;
 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 +52,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;
@@ -57,24 +64,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->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 +80,7 @@ class User_LDAPTest extends \Test\TestCase {
 			->disableOriginalConstructor()
 			->getMock();
 
-		$um = $this->getMockBuilder('\OCA\User_LDAP\User\Manager')
+		$um = $this->getMockBuilder(Manager::class)
 			->setMethods(['getDeletedUser'])
 			->setConstructorArgs([
 				$this->configMock,
@@ -99,11 +97,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 +134,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
 	 */
@@ -304,7 +303,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) {
@@ -848,4 +847,116 @@ 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(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);
+	}
+
 }