Merge pull request #5689 from nextcloud/fix-4117

LDAP: simplify returning the homePath and fixing #4117

6 files changed, 189 insertions, 48 deletions

diff --git a/apps/user_ldap/lib/User/Manager.php b/apps/user_ldap/lib/User/Manager.php
index ea4d071b646..743456d68e2 100644
--- a/apps/user_ldap/lib/User/Manager.php
+++ b/apps/user_ldap/lib/User/Manager.php
@@ -135,6 +135,19 @@ class Manager {
 	}
 
 	/**
+	 * removes a user entry from the cache
+	 * @param $uid
+	 */
+	public function invalidate($uid) {
+		if(!isset($this->usersByUid[$uid])) {
+			return;
+		}
+		$dn = $this->usersByUid[$uid]->getDN();
+		unset($this->usersByUid[$uid]);
+		unset($this->usersByDN[$dn]);
+	}
+
+	/**
 	 * @brief checks whether the Access instance has been set
 	 * @throws \Exception if Access has not been set
 	 * @return null
diff --git a/apps/user_ldap/lib/User/OfflineUser.php b/apps/user_ldap/lib/User/OfflineUser.php
index 0e60a29514e..942eee84cb7 100644
--- a/apps/user_ldap/lib/User/OfflineUser.php
+++ b/apps/user_ldap/lib/User/OfflineUser.php
@@ -25,6 +25,8 @@
 namespace OCA\User_LDAP\User;
 
 use OCA\User_LDAP\Mapping\UserMapping;
+use OCP\IConfig;
+use OCP\IDBConnection;
 
 class OfflineUser {
 	/**
@@ -60,11 +62,11 @@ class OfflineUser {
 	 */
 	protected $hasActiveShares;
 	/**
-	 * @var \OCP\IConfig $config
+	 * @var IConfig $config
 	 */
 	protected $config;
 	/**
-	 * @var \OCP\IDBConnection $db
+	 * @var IDBConnection $db
 	 */
 	protected $db;
 	/**
@@ -74,11 +76,11 @@ class OfflineUser {
 
 	/**
 	 * @param string $ocName
-	 * @param \OCP\IConfig $config
-	 * @param \OCP\IDBConnection $db
+	 * @param IConfig $config
+	 * @param IDBConnection $db
 	 * @param \OCA\User_LDAP\Mapping\UserMapping $mapping
 	 */
-	public function __construct($ocName, \OCP\IConfig $config, \OCP\IDBConnection $db, UserMapping $mapping) {
+	public function __construct($ocName, IConfig $config, IDBConnection $db, UserMapping $mapping) {
 		$this->ocName = $ocName;
 		$this->config = $config;
 		$this->db = $db;
diff --git a/apps/user_ldap/lib/User_LDAP.php b/apps/user_ldap/lib/User_LDAP.php
index 3cc2fec740e..6c438391380 100644
--- a/apps/user_ldap/lib/User_LDAP.php
+++ b/apps/user_ldap/lib/User_LDAP.php
@@ -41,19 +41,20 @@ use OCA\User_LDAP\Exceptions\NotOnLDAP;
 use OCA\User_LDAP\User\OfflineUser;
 use OCA\User_LDAP\User\User;
 use OCP\IConfig;
+use OCP\IUser;
 use OCP\Notification\IManager as INotificationManager;
 use OCP\Util;
 
 class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserInterface, IUserLDAP {
-	/** @var string[] $homesToKill */
-	protected $homesToKill = array();
-
 	/** @var \OCP\IConfig */
 	protected $ocConfig;
 
 	/** @var INotificationManager */
 	protected $notificationManager;
 
+	/** @var string */
+	protected $currentUserInDeletionProcess;
+
 	/**
 	 * @param Access $access
 	 * @param \OCP\IConfig $ocConfig
@@ -63,6 +64,24 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
 		parent::__construct($access);
 		$this->ocConfig = $ocConfig;
 		$this->notificationManager = $notificationManager;
+		$this->registerHooks();
+	}
+
+	protected function registerHooks() {
+		Util::connectHook('OC_User','pre_deleteUser', $this, 'preDeleteUser');
+		Util::connectHook('OC_User','post_deleteUser', $this, 'postDeleteUser');
+	}
+
+	public function preDeleteUser(array $param) {
+		$user = $param[0];
+		if(!$user instanceof IUser) {
+			throw new \RuntimeException('IUser expected');
+		}
+		$this->currentUserInDeletionProcess = $user->getUID();
+	}
+
+	public function postDeleteUser() {
+		$this->currentUserInDeletionProcess = null;
 	}
 
 	/**
@@ -359,10 +378,8 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
 
 		//Get Home Directory out of user preferences so we can return it later,
 		//necessary for removing directories as done by OC_User.
-		$home = $this->ocConfig->getUserValue($uid, 'user_ldap', 'homePath', '');
-		$this->homesToKill[$uid] = $home;
 		$this->access->getUserMapper()->unmap($uid);
-
+		$this->access->userManager->invalidate($uid);
 		return true;
 	}
 
@@ -375,11 +392,6 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
 	 * @throws \Exception
 	 */
 	public function getHome($uid) {
-		if(isset($this->homesToKill[$uid]) && !empty($this->homesToKill[$uid])) {
-			//a deleted user who needs some clean up
-			return $this->homesToKill[$uid];
-		}
-
 		// user Exists check required as it is not done in user proxy!
 		if(!$this->userExists($uid)) {
 			return false;
@@ -391,16 +403,18 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
 			return $path;
 		}
 
+		// early return path if it is a deleted user
 		$user = $this->access->userManager->get($uid);
-		if(is_null($user) || ($user instanceof OfflineUser && !$this->userExistsOnLDAP($user->getOCName()))) {
-			throw new NoUserException($uid . ' is not a valid user anymore');
-		}
 		if($user instanceof OfflineUser) {
-			// apparently this user survived the userExistsOnLDAP check,
-			// we request the user instance again in order to retrieve a User
-			// instance instead
-			$user = $this->access->userManager->get($uid);
+			if($this->currentUserInDeletionProcess === $user->getUID()) {
+				return $user->getHomePath();
+			} else {
+				throw new NoUserException($uid . ' is not a valid user anymore');
+			}
+		} else if ($user === null) {
+			throw new NoUserException($uid . ' is not a valid user anymore');
 		}
+
 		$path = $user->getHomePath();
 		$this->access->cacheUserHome($uid, $path);
 
diff --git a/apps/user_ldap/tests/Integration/Lib/User/IntegrationTestUserCleanUp.php b/apps/user_ldap/tests/Integration/Lib/User/IntegrationTestUserCleanUp.php
new file mode 100644
index 00000000000..7d45ee69fbc
--- /dev/null
+++ b/apps/user_ldap/tests/Integration/Lib/User/IntegrationTestUserCleanUp.php
@@ -0,0 +1,102 @@
+<?php
+/**
+ * @copyright Copyright (c) 2016, ownCloud, Inc.
+ *
+ * @author Arthur Schiwon <blizzz@arthur-schiwon.de>
+ * @author Joas Schilling <coding@schilljs.com>
+ *
+ * @license AGPL-3.0
+ *
+ * This code is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
+ *
+ * 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, version 3,
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>
+ *
+ */
+
+namespace OCA\User_LDAP\Tests\Integration\Lib\User;
+
+use OC\User\NoUserException;
+use OCA\User_LDAP\Jobs\CleanUp;
+use OCA\User_LDAP\Mapping\UserMapping;
+use OCA\User_LDAP\Tests\Integration\AbstractIntegrationTest;
+use OCA\User_LDAP\User_LDAP;
+
+require_once __DIR__ . '/../../Bootstrap.php';
+
+class IntegrationTestUserCleanUp extends AbstractIntegrationTest {
+	/** @var  UserMapping */
+	protected $mapping;
+
+	/**
+	 * prepares the LDAP environment and sets up a test configuration for
+	 * the LDAP backend.
+	 */
+	public function init() {
+		require(__DIR__ . '/../../setup-scripts/createExplicitUsers.php');
+		parent::init();
+		$this->mapping = new UserMapping(\OC::$server->getDatabaseConnection());
+		$this->mapping->clear();
+		$this->access->setUserMapper($this->mapping);
+
+		$userBackend  = new User_LDAP($this->access, \OC::$server->getConfig(), \OC::$server->getNotificationManager());
+		\OC_User::useBackend($userBackend);
+	}
+
+	/**
+	 * adds a map entry for the user, so we know the username
+	 *
+	 * @param $dn
+	 * @param $username
+	 */
+	private function prepareUser($dn, $username) {
+		// assigns our self-picked oc username to the dn
+		$this->mapping->map($dn, $username, 'fakeUUID-' . $username);
+	}
+
+	private function deleteUserFromLDAP($dn) {
+		$cr = $this->connection->getConnectionResource();
+		ldap_delete($cr, $dn);
+	}
+
+	/**
+	 * tests whether a display name consisting of two parts is created correctly
+	 *
+	 * @return bool
+	 */
+	protected function case1() {
+		$username = 'alice1337';
+		$dn = 'uid=alice,ou=Users,' . $this->base;
+		$this->prepareUser($dn, $username);
+
+		$user = \OC::$server->getUserManager()->get($username);
+		if($user === null) {
+			return false;
+		}
+
+		$this->deleteUserFromLDAP($dn);
+
+		$job = new CleanUp();
+		$job->run([]);
+
+		$user->delete();
+
+		return null === \OC::$server->getUserManager()->get($username);
+	}
+}
+
+/** @var string $host */
+/** @var int $port */
+/** @var string $adn */
+/** @var string $apwd */
+/** @var string $bdn */
+$test = new IntegrationTestUserCleanUp($host, $port, $adn, $apwd, $bdn);
+$test->init();
+$test->run();
diff --git a/apps/user_ldap/tests/User/UserTest.php b/apps/user_ldap/tests/User/UserTest.php
index f48abc9ce4a..637842d9ac7 100644
--- a/apps/user_ldap/tests/User/UserTest.php
+++ b/apps/user_ldap/tests/User/UserTest.php
@@ -295,7 +295,7 @@ class UserTest extends \Test\TestCase {
 	}
 
 	public function testUpdateQuotaToNoneAllProvided() {
-		list($access, $config, $filesys, $image, $log, $avaMgr, $dbc, $userMgr, $notiMgr) =
+		list(, $config, $filesys, $image, $log, $avaMgr, $dbc, $userMgr, $notiMgr) =
 			$this->getTestInstances();
 
 		list($access, $connection) =
diff --git a/apps/user_ldap/tests/User_LDAPTest.php b/apps/user_ldap/tests/User_LDAPTest.php
index ced5009148d..f74a57e25eb 100644
--- a/apps/user_ldap/tests/User_LDAPTest.php
+++ b/apps/user_ldap/tests/User_LDAPTest.php
@@ -35,6 +35,7 @@ use OCA\User_LDAP\FilesystemHelper;
 use OCA\User_LDAP\Helper;
 use OCA\User_LDAP\ILDAPWrapper;
 use OCA\User_LDAP\LogWrapper;
+use OCA\User_LDAP\Mapping\UserMapping;
 use OCA\User_LDAP\User\Manager;
 use OCA\User_LDAP\User\OfflineUser;
 use OC\HintException;
@@ -59,7 +60,10 @@ use OCP\Notification\IManager as INotificationManager;
 class User_LDAPTest extends TestCase {
 	protected $backend;
 	protected $access;
+	/** @var  IConfig|\PHPUnit_Framework_MockObject_MockObject */
 	protected $configMock;
+	/** @var  OfflineUser|\PHPUnit_Framework_MockObject_MockObject */
+	protected $offlineUser;
 
 	protected function setUp() {
 		parent::setUp();
@@ -80,10 +84,9 @@ class User_LDAPTest extends TestCase {
 
 		$this->configMock = $this->createMock(IConfig::class);
 
-		$offlineUser = $this->getMockBuilder('\OCA\User_LDAP\User\OfflineUser')
-			->disableOriginalConstructor()
-			->getMock();
+		$this->offlineUser = $this->createMock(OfflineUser::class);
 
+		/** @var Manager|\PHPUnit_Framework_MockObject_MockObject $um */
 		$um = $this->getMockBuilder(Manager::class)
 			->setMethods(['getDeletedUser'])
 			->setConstructorArgs([
@@ -100,7 +103,7 @@ class User_LDAPTest extends TestCase {
 
 		$um->expects($this->any())
 			->method('getDeletedUser')
-			->will($this->returnValue($offlineUser));
+			->will($this->returnValue($this->offlineUser));
 
 		$helper = new Helper(\OC::$server->getConfig());
 
@@ -284,10 +287,11 @@ class User_LDAPTest extends TestCase {
 	}
 
 	public function testDeleteUserSuccess() {
+		$uid = 'jeremy';
+		$home = '/var/vhome/jdings/';
+
 		$access = $this->getAccessMock();
-		$mapping = $this->getMockBuilder('\OCA\User_LDAP\Mapping\UserMapping')
-			->disableOriginalConstructor()
-			->getMock();
+		$mapping = $this->createMock(UserMapping::class);
 		$mapping->expects($this->once())
 			->method('unmap')
 			->will($this->returnValue(true));
@@ -295,18 +299,20 @@ class User_LDAPTest extends TestCase {
 			->method('getUserMapper')
 			->will($this->returnValue($mapping));
 
-		$config = $this->createMock(IConfig::class);
-		$config->expects($this->exactly(2))
+		$this->configMock->expects($this->any())
 			->method('getUserValue')
-			->will($this->onConsecutiveCalls('1', '/var/vhome/jdings/'));
+			->with($uid, 'user_ldap', 'isDeleted')
+			->willReturn('1');
 
-		$backend = new UserLDAP($access, $config, $this->createMock(INotificationManager::class));
+		$this->offlineUser->expects($this->once())
+			->method('getHomePath')
+			->willReturn($home);
 
-		$result = $backend->deleteUser('jeremy');
-		$this->assertTrue($result);
+		$backend = new UserLDAP($access, $this->configMock, $this->createMock(INotificationManager::class));
 
-		$home = $backend->getHome('jeremy');
-		$this->assertSame($home, '/var/vhome/jdings/');
+		$result = $backend->deleteUser($uid);
+		$this->assertTrue($result);
+		$this->assertSame($backend->getHome($uid), $home);
 	}
 
 	/**
@@ -577,11 +583,11 @@ class User_LDAPTest extends TestCase {
 		$this->assertFalse($result);
 	}
 
-	public function testDeleteUser() {
+	public function testDeleteUserExisting() {
 		$access = $this->getAccessMock();
 		$backend = new UserLDAP($access, $this->createMock(IConfig::class), $this->createMock(INotificationManager::class));
 
-		//we do not support deleting users at all
+		//we do not support deleting existing users at all
 		$result = $backend->deleteUser('gunslinger');
 		$this->assertFalse($result);
 	}
@@ -699,8 +705,10 @@ class User_LDAPTest extends TestCase {
 	 * @expectedException \OC\User\NoUserException
 	 */
 	public function testGetHomeDeletedUser() {
+		$uid = 'newyorker';
+
 		$access = $this->getAccessMock();
-		$backend = new UserLDAP($access, $this->createMock(IConfig::class), $this->createMock(INotificationManager::class));
+		$backend = new UserLDAP($access, $this->configMock, $this->createMock(INotificationManager::class));
 		$this->prepareMockForUserExists($access);
 
 		$access->connection->expects($this->any())
@@ -716,9 +724,7 @@ class User_LDAPTest extends TestCase {
 				->method('readAttribute')
 				->will($this->returnValue([]));
 
-		$userMapper = $this->getMockBuilder('\OCA\User_LDAP\Mapping\UserMapping')
-				->disableOriginalConstructor()
-				->getMock();
+		$userMapper = $this->createMock(UserMapping::class);
 
 		$access->expects($this->any())
 				->method('getUserMapper')
@@ -728,9 +734,13 @@ class User_LDAPTest extends TestCase {
 			->method('getUserValue')
 			->will($this->returnValue(true));
 
-		//no path at all – triggers OC default behaviour
-		$result = $backend->getHome('newyorker');
-		$this->assertFalse($result);
+		$this->offlineUser->expects($this->never())
+			->method('getHomePath');
+		$this->offlineUser->expects($this->once())
+			->method('getUID')
+			->willReturn($uid);
+
+		$backend->getHome($uid);
 	}
 
 	private function prepareAccessForGetDisplayName(&$access) {