From 5933c0c55f293e8ecf1a6dddd2c74d2362bb6b88 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Tue, 11 Jul 2017 22:43:47 +0200 Subject: [PATCH 1/2] simplify returning the homePath and fixing #4117 homesToKill was not set in runtime since some changes some place else. It required deleteUser() to be called first. The method acts independent of it now. Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/User/OfflineUser.php | 12 +++++++----- apps/user_ldap/lib/User_LDAP.php | 23 +++++------------------ 2 files changed, 12 insertions(+), 23 deletions(-) diff --git a/apps/user_ldap/lib/User/OfflineUser.php b/apps/user_ldap/lib/User/OfflineUser.php index 0e60a29514e76..942eee84cb72f 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 75cdb3951ca47..49b6f09724aea 100644 --- a/apps/user_ldap/lib/User_LDAP.php +++ b/apps/user_ldap/lib/User_LDAP.php @@ -45,9 +45,6 @@ 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; @@ -359,10 +356,7 @@ public function deleteUser($uid) { //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); - return true; } @@ -375,11 +369,6 @@ public function deleteUser($uid) { * @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 +380,14 @@ public function getHome($uid) { 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); + return $user->getHomePath(); + } else if ($user === null) { + throw new NoUserException($uid . ' is not a valid user anymore'); } + $path = $user->getHomePath(); $this->access->cacheUserHome($uid, $path); From 90ebeae512e6a22a337af0d77a951fed54adc16f Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 13 Jul 2017 14:32:52 +0200 Subject: [PATCH 2/2] listen to deletion hooks for proper handling, adjust and add tests Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/User/Manager.php | 13 +++ apps/user_ldap/lib/User_LDAP.php | 29 ++++- .../Lib/User/IntegrationTestUserCleanUp.php | 102 ++++++++++++++++++ apps/user_ldap/tests/User/UserTest.php | 2 +- apps/user_ldap/tests/User_LDAPTest.php | 58 +++++----- 5 files changed, 178 insertions(+), 26 deletions(-) create mode 100644 apps/user_ldap/tests/Integration/Lib/User/IntegrationTestUserCleanUp.php diff --git a/apps/user_ldap/lib/User/Manager.php b/apps/user_ldap/lib/User/Manager.php index ea4d071b646cd..743456d68e2d6 100644 --- a/apps/user_ldap/lib/User/Manager.php +++ b/apps/user_ldap/lib/User/Manager.php @@ -134,6 +134,19 @@ private function createAndCache($dn, $uid) { return $user; } + /** + * 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 diff --git a/apps/user_ldap/lib/User_LDAP.php b/apps/user_ldap/lib/User_LDAP.php index 49b6f09724aea..f677ba77b985d 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\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 @@ public function __construct(Access $access, IConfig $ocConfig, INotificationMana 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 @@ public function deleteUser($uid) { //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 @@ public function getHome($uid) { // 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 0000000000000..7d45ee69fbc7b --- /dev/null +++ b/apps/user_ldap/tests/Integration/Lib/User/IntegrationTestUserCleanUp.php @@ -0,0 +1,102 @@ + + * @author Joas Schilling + * + * @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 + * + */ + +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 0437051b1343c..5563eeef9cfd9 100644 --- a/apps/user_ldap/tests/User/UserTest.php +++ b/apps/user_ldap/tests/User/UserTest.php @@ -295,7 +295,7 @@ public function testUpdateQuotaToDefaultAllProvided() { } 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 aaf67ebcab626..9f1b1d3ffc8ff 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\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 @@ 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 @@ private function getAccessMock() { $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 @@ private function getAccessMock() { $um->expects($this->any()) ->method('getDeletedUser') - ->will($this->returnValue($offlineUser)); + ->will($this->returnValue($this->offlineUser)); $helper = new Helper(\OC::$server->getConfig()); @@ -281,10 +284,11 @@ public function testDeleteUserCancel() { } 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)); @@ -292,18 +296,20 @@ public function testDeleteUserSuccess() { ->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); } /** @@ -571,11 +577,11 @@ public function testUserExistsPublicAPIForNeverExisting() { $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); } @@ -693,8 +699,10 @@ public function testGetHomeNoPath() { * @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()) @@ -710,9 +718,7 @@ public function testGetHomeDeletedUser() { ->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') @@ -722,9 +728,13 @@ public function testGetHomeDeletedUser() { ->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) {