Merge pull request #5689 from nextcloud/fix-4117

LDAP: simplify returning the homePath and fixing #4117
This commit is contained in:
Morris Jobke 2017-09-14 00:23:01 +02:00 committed by GitHub
commit ba2e1c5db9
6 changed files with 189 additions and 48 deletions

View File

@ -134,6 +134,19 @@ class Manager {
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

View File

@ -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;

View File

@ -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()))) {
if($user instanceof OfflineUser) {
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');
}
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);
}
$path = $user->getHomePath();
$this->access->cacheUserHome($uid, $path);

View File

@ -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();

View File

@ -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) =

View File

@ -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');
$backend = new UserLDAP($access, $this->configMock, $this->createMock(INotificationManager::class));
$result = $backend->deleteUser($uid);
$this->assertTrue($result);
$home = $backend->getHome('jeremy');
$this->assertSame($home, '/var/vhome/jdings/');
$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) {