listen to deletion hooks for proper handling, adjust and add tests

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

5 files changed, 178 insertions, 26 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_LDAP.php b/apps/user_ldap/lib/User_LDAP.php
index 34bcb9bb268..6c438391380 100644
--- a/apps/user_ldap/lib/User_LDAP.php
+++ b/apps/user_ldap/lib/User_LDAP.php
@@ -41,6 +41,7 @@ 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;
 
@@ -51,6 +52,9 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
 	/** @var INotificationManager */
 	protected $notificationManager;
 
+	/** @var string */
+	protected $currentUserInDeletionProcess;
+
 	/**
 	 * @param Access $access
 	 * @param \OCP\IConfig $ocConfig
@@ -60,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;
 	}
 
 	/**
@@ -357,6 +379,7 @@ 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.
 		$this->access->getUserMapper()->unmap($uid);
+		$this->access->userManager->invalidate($uid);
 		return true;
 	}
 
@@ -383,7 +406,11 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
 		// early return path if it is a deleted user
 		$user = $this->access->userManager->get($uid);
 		if($user instanceof OfflineUser) {
-			return $user->getHomePath();
+			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');
 		}
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) {