Merge pull request #7418 from nextcloud/ldap-fix-cache-retrieved-user

ensure that users are cached when they are retrieved

3 files changed, 143 insertions, 12 deletions

diff --git a/apps/user_ldap/lib/Access.php b/apps/user_ldap/lib/Access.php
index 07583798e46..95710cd37f2 100644
--- a/apps/user_ldap/lib/Access.php
+++ b/apps/user_ldap/lib/Access.php
@@ -527,6 +527,7 @@ class Access extends LDAPUtility implements IUserTools {
 	 * @param bool|null $newlyMapped
 	 * @param array|null $record
 	 * @return false|string with with the name to use in Nextcloud
+	 * @throws \Exception
 	 */
 	public function dn2ocname($fdn, $ldapName = null, $isUser = true, &$newlyMapped = null, array $record = null) {
 		$newlyMapped = false;
@@ -586,7 +587,7 @@ class Access extends LDAPUtility implements IUserTools {
 		// outside of core user management will still cache the user as non-existing.
 		$originalTTL = $this->connection->ldapCacheTTL;
 		$this->connection->setConfiguration(array('ldapCacheTTL' => 0));
-		if(($isUser && !\OCP\User::userExists($intName))
+		if(($isUser && $intName !== '' && !\OCP\User::userExists($intName))
 			|| (!$isUser && !\OC::$server->getGroupManager()->groupExists($intName))) {
 			if($mapper->map($fdn, $intName, $uuid)) {
 				$this->connection->setConfiguration(array('ldapCacheTTL' => $originalTTL));
@@ -830,6 +831,9 @@ class Access extends LDAPUtility implements IUserTools {
 			$recordsToUpdate = array_filter($ldapRecords, function($record) use ($isBackgroundJobModeAjax) {
 				$newlyMapped = false;
 				$uid = $this->dn2ocname($record['dn'][0], null, true, $newlyMapped, $record);
+				if(is_string($uid)) {
+					$this->cacheUserExists($uid);
+				}
 				return ($uid !== false) && ($newlyMapped || $isBackgroundJobModeAjax);
 			});
 		}
@@ -854,7 +858,6 @@ class Access extends LDAPUtility implements IUserTools {
 			if($ocName === false) {
 				continue;
 			}
-			$this->cacheUserExists($ocName);
 			$user = $this->userManager->get($ocName);
 			if($user instanceof OfflineUser) {
 				$user->unmark();
@@ -1019,11 +1022,15 @@ class Access extends LDAPUtility implements IUserTools {
 
 	/**
 	 * retrieved. Results will according to the order in the array.
+	 *
+	 * @param $filter
+	 * @param $base
+	 * @param string[]|string|null $attr
 	 * @param int $limit optional, maximum results to be counted
 	 * @param int $offset optional, a starting point
 	 * @return array|false array with the search result as first value and pagedSearchOK as
 	 * second | false if not successful
-	 * @throws \OC\ServerNotAvailableException
+	 * @throws ServerNotAvailableException
 	 */
 	private function executeSearch($filter, $base, &$attr = null, $limit = null, $offset = null) {
 		if(!is_null($attr) && !is_array($attr)) {
@@ -1102,6 +1109,7 @@ class Access extends LDAPUtility implements IUserTools {
 
 	/**
 	 * executes an LDAP search, but counts the results only
+	 *
 	 * @param string $filter the LDAP filter for the search
 	 * @param array $base an array containing the LDAP subtree(s) that shall be searched
 	 * @param string|string[] $attr optional, array, one or more attributes that shall be
@@ -1111,7 +1119,7 @@ class Access extends LDAPUtility implements IUserTools {
 	 * @param bool $skipHandling indicates whether the pages search operation is
 	 * completed
 	 * @return int|false Integer or false if the search could not be initialized
-	 *
+	 * @throws ServerNotAvailableException
 	 */
 	private function count($filter, $base, $attr = null, $limit = null, $offset = null, $skipHandling = false) {
 		\OCP\Util::writeLog('user_ldap', 'Count filter:  '.print_r($filter, true), \OCP\Util::DEBUG);
@@ -1126,8 +1134,7 @@ class Access extends LDAPUtility implements IUserTools {
 		$this->connection->getConnectionResource();
 
 		do {
-			$search = $this->executeSearch($filter, $base, $attr,
-										   $limitPerPage, $offset);
+			$search = $this->executeSearch($filter, $base, $attr, $limitPerPage, $offset);
 			if($search === false) {
 				return $counter > 0 ? $counter : false;
 			}
@@ -1169,6 +1176,7 @@ class Access extends LDAPUtility implements IUserTools {
 
 	/**
 	 * Executes an LDAP search
+	 *
 	 * @param string $filter the LDAP filter for the search
 	 * @param array $base an array containing the LDAP subtree(s) that shall be searched
 	 * @param string|string[] $attr optional, array, one or more attributes that shall be
@@ -1176,6 +1184,7 @@ class Access extends LDAPUtility implements IUserTools {
 	 * @param int $offset
 	 * @param bool $skipHandling
 	 * @return array with the search result
+	 * @throws ServerNotAvailableException
 	 */
 	public function search($filter, $base, $attr = null, $limit = null, $offset = null, $skipHandling = false) {
 		$limitPerPage = intval($this->connection->ldapPagingSize);
@@ -1189,12 +1198,12 @@ class Access extends LDAPUtility implements IUserTools {
 		 * loops through until we get $continue equals true and
 		 * $findings['count'] < $limit
 		 */
-		$findings = array();
+		$findings = [];
 		$savedoffset = $offset;
 		do {
 			$search = $this->executeSearch($filter, $base, $attr, $limitPerPage, $offset);
 			if($search === false) {
-				return array();
+				return [];
 			}
 			list($sr, $pagedSearchOK) = $search;
 			$cr = $this->connection->getConnectionResource();
@@ -1231,7 +1240,7 @@ class Access extends LDAPUtility implements IUserTools {
 		}
 
 		if(!is_null($attr)) {
-			$selection = array();
+			$selection = [];
 			$i = 0;
 			foreach($findings as $item) {
 				if(!is_array($item)) {
@@ -1239,7 +1248,6 @@ class Access extends LDAPUtility implements IUserTools {
 				}
 				$item = \OCP\Util::mb_array_change_key_case($item, MB_CASE_LOWER, 'UTF-8');
 				foreach($attr as $key) {
-					$key = mb_strtolower($key, 'UTF-8');
 					if(isset($item[$key])) {
 						if(is_array($item[$key]) && isset($item[$key]['count'])) {
 							unset($item[$key]['count']);
@@ -1281,7 +1289,7 @@ class Access extends LDAPUtility implements IUserTools {
 	 */
 	public function sanitizeUsername($name) {
 		if($this->connection->ldapIgnoreNamingRules) {
-			return $name;
+			return trim($name);
 		}
 
 		// Transliteration
diff --git a/apps/user_ldap/lib/Helper.php b/apps/user_ldap/lib/Helper.php
index a433ea8e4a7..3157a7ab09d 100644
--- a/apps/user_ldap/lib/Helper.php
+++ b/apps/user_ldap/lib/Helper.php
@@ -229,7 +229,7 @@ class Helper {
 	/**
 	 * sanitizes a DN received from the LDAP server
 	 * @param array $dn the DN in question
-	 * @return array the sanitized DN
+	 * @return array|string the sanitized DN
 	 */
 	public function sanitizeDN($dn) {
 		//treating multiple base DNs
diff --git a/apps/user_ldap/tests/AccessTest.php b/apps/user_ldap/tests/AccessTest.php
index 6c250ff320f..cbb695d779a 100644
--- a/apps/user_ldap/tests/AccessTest.php
+++ b/apps/user_ldap/tests/AccessTest.php
@@ -60,6 +60,8 @@ use Test\TestCase;
  * @package OCA\User_LDAP\Tests
  */
 class AccessTest extends TestCase {
+	/** @var UserMapping|\PHPUnit_Framework_MockObject_MockObject */
+	protected $userMapper;
 	/** @var Connection|\PHPUnit_Framework_MockObject_MockObject */
 	private $connection;
 	/** @var LDAP|\PHPUnit_Framework_MockObject_MockObject */
@@ -79,6 +81,7 @@ class AccessTest extends TestCase {
 		$this->userManager = $this->createMock(Manager::class);
 		$this->helper = $this->createMock(Helper::class);
 		$this->config  = $this->createMock(IConfig::class);
+		$this->userMapper = $this->createMock(UserMapping::class);
 
 		$this->access = new Access(
 			$this->connection,
@@ -87,6 +90,7 @@ class AccessTest extends TestCase {
 			$this->helper,
 			$this->config
 		);
+		$this->access->setUserMapper($this->userMapper);
 	}
 
 	private function getConnectorAndLdapMock() {
@@ -217,6 +221,7 @@ class AccessTest extends TestCase {
 
 	/**
 	 * @dataProvider dnInputDataProvider
+	 * @param array $case
 	 */
 	public function testStringResemblesDN($case) {
 		list($lw, $con, $um, $helper) = $this->getConnectorAndLdapMock();
@@ -440,6 +445,7 @@ class AccessTest extends TestCase {
 			->method('__get')
 			->willReturn(false);
 
+		/** @noinspection PhpUnhandledExceptionInspection */
 		$this->access->setPassword('CN=foo', 'MyPassword');
 	}
 
@@ -458,6 +464,7 @@ class AccessTest extends TestCase {
 			->with($connection)
 			->willReturn(false);
 
+		/** @noinspection PhpUnhandledExceptionInspection */
 		$this->assertFalse($this->access->setPassword('CN=foo', 'MyPassword'));
 	}
 
@@ -485,6 +492,7 @@ class AccessTest extends TestCase {
 			->with($connection, 'CN=foo', 'MyPassword')
 			->willThrowException(new ConstraintViolationException());
 
+		/** @noinspection PhpUnhandledExceptionInspection */
 		$this->access->setPassword('CN=foo', 'MyPassword');
 	}
 
@@ -508,6 +516,121 @@ class AccessTest extends TestCase {
 			->with($connection, 'CN=foo', 'MyPassword')
 			->willReturn(true);
 
+		/** @noinspection PhpUnhandledExceptionInspection */
 		$this->assertTrue($this->access->setPassword('CN=foo', 'MyPassword'));
 	}
+
+	protected function prepareMocksForSearchTests(
+		$base,
+		$fakeConnection,
+		$fakeSearchResultResource,
+		$fakeLdapEntries
+	) {
+		$this->connection
+			->expects($this->any())
+			->method('getConnectionResource')
+			->willReturn($fakeConnection);
+		$this->connection->expects($this->any())
+			->method('__get')
+			->willReturnCallback(function($key) use ($base) {
+				if(stripos($key, 'base') !== false) {
+					return $base;
+				}
+				return null;
+			});
+
+		$this->ldap
+			->expects($this->any())
+			->method('isResource')
+			->willReturnCallback(function ($resource) use ($fakeConnection) {
+				return $resource === $fakeConnection;
+			});
+		$this->ldap
+			->expects($this->any())
+			->method('errno')
+			->willReturn(0);
+		$this->ldap
+			->expects($this->once())
+			->method('search')
+			->willReturn([$fakeSearchResultResource]);
+		$this->ldap
+			->expects($this->exactly(count($base)))
+			->method('getEntries')
+			->willReturn($fakeLdapEntries);
+
+		$this->helper->expects($this->any())
+			->method('sanitizeDN')
+			->willReturnArgument(0);
+	}
+
+	public function testSearchNoPagedSearch() {
+		// scenario: no pages search, 1 search base
+		$filter = 'objectClass=nextcloudUser';
+		$base = ['ou=zombies,dc=foobar,dc=nextcloud,dc=com'];
+
+		$fakeConnection = new \stdClass();
+		$fakeSearchResultResource = new \stdClass();
+		$fakeLdapEntries = [
+			'count' => 2,
+			[
+				'dn' => 'uid=sgarth,' . $base[0],
+			],
+			[
+				'dn' => 'uid=wwilson,' . $base[0],
+			]
+		];
+
+		$expected = $fakeLdapEntries;
+		unset($expected['count']);
+
+		$this->prepareMocksForSearchTests($base, $fakeConnection, $fakeSearchResultResource, $fakeLdapEntries);
+
+		/** @noinspection PhpUnhandledExceptionInspection */
+		$result = $this->access->search($filter, $base);
+		$this->assertSame($expected, $result);
+	}
+
+	public function testFetchListOfUsers() {
+		$filter = 'objectClass=nextcloudUser';
+		$base = ['ou=zombies,dc=foobar,dc=nextcloud,dc=com'];
+		$attrs = ['dn', 'uid'];
+
+		$fakeConnection = new \stdClass();
+		$fakeSearchResultResource = new \stdClass();
+		$fakeLdapEntries = [
+			'count' => 2,
+			[
+				'dn' => 'uid=sgarth,' . $base[0],
+				'uid' => [ 'sgarth' ],
+			],
+			[
+				'dn' => 'uid=wwilson,' . $base[0],
+				'uid' => [ 'wwilson' ],
+			]
+		];
+		$expected = $fakeLdapEntries;
+		unset($expected['count']);
+		array_walk($expected, function(&$v) {
+			$v['dn'] = [$v['dn']];	// dn is translated into an array internally for consistency
+		});
+
+		$this->prepareMocksForSearchTests($base, $fakeConnection, $fakeSearchResultResource, $fakeLdapEntries);
+
+		$this->connection->expects($this->exactly($fakeLdapEntries['count']))
+			->method('writeToCache')
+			->with($this->stringStartsWith('userExists'), true);
+
+		$this->userMapper->expects($this->exactly($fakeLdapEntries['count']))
+			->method('getNameByDN')
+			->willReturnCallback(function($fdn) {
+				$parts = ldap_explode_dn($fdn, false);
+				return $parts[0];
+			});
+
+		/** @noinspection PhpUnhandledExceptionInspection */
+		$list = $this->access->fetchListOfUsers($filter, $attrs);
+		$this->assertSame($expected, $list);
+	}
+
+
 }