Skip to content

Commit ba2e1c5

Browse files
MorrisJobkeMorrisJobke
authored
Merge pull request #5689 from nextcloud/fix-4117
LDAP: simplify returning the homePath and fixing #4117
2 parents 8ef4fcb + ab92e2e commit ba2e1c5

6 files changed

Lines changed: 189 additions & 48 deletions

File tree

‎apps/user_ldap/lib/User/Manager.php‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,19 @@ private function createAndCache($dn, $uid) {
134134
return $user;
135135
}
136136

137+
/**
138+
* removes a user entry from the cache
139+
* @param $uid
140+
*/
141+
public function invalidate($uid) {
142+
if(!isset($this->usersByUid[$uid])) {
143+
return;
144+
}
145+
$dn = $this->usersByUid[$uid]->getDN();
146+
unset($this->usersByUid[$uid]);
147+
unset($this->usersByDN[$dn]);
148+
}
149+
137150
/**
138151
* @brief checks whether the Access instance has been set
139152
* @throws \Exception if Access has not been set

‎apps/user_ldap/lib/User/OfflineUser.php‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@
2525
namespace OCA\User_LDAP\User;
2626

2727
use OCA\User_LDAP\Mapping\UserMapping;
28+
use OCP\IConfig;
29+
use OCP\IDBConnection;
2830

2931
class OfflineUser {
3032
/**
@@ -60,11 +62,11 @@ class OfflineUser {
6062
*/
6163
protected $hasActiveShares;
6264
/**
63-
* @var \OCP\IConfig $config
65+
* @var IConfig $config
6466
*/
6567
protected $config;
6668
/**
67-
* @var \OCP\IDBConnection $db
69+
* @var IDBConnection $db
6870
*/
6971
protected $db;
7072
/**
@@ -74,11 +76,11 @@ class OfflineUser {
7476

7577
/**
7678
* @param string $ocName
77-
* @param \OCP\IConfig $config
78-
* @param \OCP\IDBConnection $db
79+
* @param IConfig $config
80+
* @param IDBConnection $db
7981
* @param \OCA\User_LDAP\Mapping\UserMapping $mapping
8082
*/
81-
public function __construct($ocName, \OCP\IConfig $config, \OCP\IDBConnection $db, UserMapping $mapping) {
83+
public function __construct($ocName, IConfig $config, IDBConnection $db, UserMapping $mapping) {
8284
$this->ocName = $ocName;
8385
$this->config = $config;
8486
$this->db = $db;

‎apps/user_ldap/lib/User_LDAP.php‎

Lines changed: 32 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -41,19 +41,20 @@
4141
use OCA\User_LDAP\User\OfflineUser;
4242
use OCA\User_LDAP\User\User;
4343
use OCP\IConfig;
44+
use OCP\IUser;
4445
use OCP\Notification\IManager as INotificationManager;
4546
use OCP\Util;
4647

4748
class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserInterface, IUserLDAP {
48-
/** @var string[] $homesToKill */
49-
protected $homesToKill = array();
50-
5149
/** @var \OCP\IConfig */
5250
protected $ocConfig;
5351

5452
/** @var INotificationManager */
5553
protected $notificationManager;
5654

55+
/** @var string */
56+
protected $currentUserInDeletionProcess;
57+
5758
/**
5859
* @param Access $access
5960
* @param \OCP\IConfig $ocConfig
@@ -63,6 +64,24 @@ public function __construct(Access $access, IConfig $ocConfig, INotificationMana
6364
parent::__construct($access);
6465
$this->ocConfig = $ocConfig;
6566
$this->notificationManager = $notificationManager;
67+
$this->registerHooks();
68+
}
69+
70+
protected function registerHooks() {
71+
Util::connectHook('OC_User','pre_deleteUser', $this, 'preDeleteUser');
72+
Util::connectHook('OC_User','post_deleteUser', $this, 'postDeleteUser');
73+
}
74+
75+
public function preDeleteUser(array $param) {
76+
$user = $param[0];
77+
if(!$user instanceof IUser) {
78+
throw new \RuntimeException('IUser expected');
79+
}
80+
$this->currentUserInDeletionProcess = $user->getUID();
81+
}
82+
83+
public function postDeleteUser() {
84+
$this->currentUserInDeletionProcess = null;
6685
}
6786

6887
/**
@@ -359,10 +378,8 @@ public function deleteUser($uid) {
359378

360379
//Get Home Directory out of user preferences so we can return it later,
361380
//necessary for removing directories as done by OC_User.
362-
$home = $this->ocConfig->getUserValue($uid, 'user_ldap', 'homePath', '');
363-
$this->homesToKill[$uid] = $home;
364381
$this->access->getUserMapper()->unmap($uid);
365-
382+
$this->access->userManager->invalidate($uid);
366383
return true;
367384
}
368385

@@ -375,11 +392,6 @@ public function deleteUser($uid) {
375392
* @throws \Exception
376393
*/
377394
public function getHome($uid) {
378-
if(isset($this->homesToKill[$uid]) && !empty($this->homesToKill[$uid])) {
379-
//a deleted user who needs some clean up
380-
return $this->homesToKill[$uid];
381-
}
382-
383395
// user Exists check required as it is not done in user proxy!
384396
if(!$this->userExists($uid)) {
385397
return false;
@@ -391,16 +403,18 @@ public function getHome($uid) {
391403
return $path;
392404
}
393405

406+
// early return path if it is a deleted user
394407
$user = $this->access->userManager->get($uid);
395-
if(is_null($user) || ($user instanceof OfflineUser && !$this->userExistsOnLDAP($user->getOCName()))) {
396-
throw new NoUserException($uid . ' is not a valid user anymore');
397-
}
398408
if($user instanceof OfflineUser) {
399-
// apparently this user survived the userExistsOnLDAP check,
400-
// we request the user instance again in order to retrieve a User
401-
// instance instead
402-
$user = $this->access->userManager->get($uid);
409+
if($this->currentUserInDeletionProcess === $user->getUID()) {
410+
return $user->getHomePath();
411+
} else {
412+
throw new NoUserException($uid . ' is not a valid user anymore');
413+
}
414+
} else if ($user === null) {
415+
throw new NoUserException($uid . ' is not a valid user anymore');
403416
}
417+
404418
$path = $user->getHomePath();
405419
$this->access->cacheUserHome($uid, $path);
406420

‎apps/user_ldap/tests/Integration/Lib/User/IntegrationTestUserCleanUp.php‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
<?php
2+
/**
3+
* @copyright Copyright (c) 2016, ownCloud, Inc.
4+
*
5+
* @author Arthur Schiwon <blizzz@arthur-schiwon.de>
6+
* @author Joas Schilling <coding@schilljs.com>
7+
*
8+
* @license AGPL-3.0
9+
*
10+
* This code is free software: you can redistribute it and/or modify
11+
* it under the terms of the GNU Affero General Public License, version 3,
12+
* as published by the Free Software Foundation.
13+
*
14+
* This program is distributed in the hope that it will be useful,
15+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
16+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
17+
* GNU Affero General Public License for more details.
18+
*
19+
* You should have received a copy of the GNU Affero General Public License, version 3,
20+
* along with this program. If not, see <http://www.gnu.org/licenses/>
21+
*
22+
*/
23+
24+
namespace OCA\User_LDAP\Tests\Integration\Lib\User;
25+
26+
use OC\User\NoUserException;
27+
use OCA\User_LDAP\Jobs\CleanUp;
28+
use OCA\User_LDAP\Mapping\UserMapping;
29+
use OCA\User_LDAP\Tests\Integration\AbstractIntegrationTest;
30+
use OCA\User_LDAP\User_LDAP;
31+
32+
require_once __DIR__ . '/../../Bootstrap.php';
33+
34+
class IntegrationTestUserCleanUp extends AbstractIntegrationTest {
35+
/** @var UserMapping */
36+
protected $mapping;
37+
38+
/**
39+
* prepares the LDAP environment and sets up a test configuration for
40+
* the LDAP backend.
41+
*/
42+
public function init() {
43+
require(__DIR__ . '/../../setup-scripts/createExplicitUsers.php');
44+
parent::init();
45+
$this->mapping = new UserMapping(\OC::$server->getDatabaseConnection());
46+
$this->mapping->clear();
47+
$this->access->setUserMapper($this->mapping);
48+
49+
$userBackend = new User_LDAP($this->access, \OC::$server->getConfig(), \OC::$server->getNotificationManager());
50+
\OC_User::useBackend($userBackend);
51+
}
52+
53+
/**
54+
* adds a map entry for the user, so we know the username
55+
*
56+
* @param $dn
57+
* @param $username
58+
*/
59+
private function prepareUser($dn, $username) {
60+
// assigns our self-picked oc username to the dn
61+
$this->mapping->map($dn, $username, 'fakeUUID-' . $username);
62+
}
63+
64+
private function deleteUserFromLDAP($dn) {
65+
$cr = $this->connection->getConnectionResource();
66+
ldap_delete($cr, $dn);
67+
}
68+
69+
/**
70+
* tests whether a display name consisting of two parts is created correctly
71+
*
72+
* @return bool
73+
*/
74+
protected function case1() {
75+
$username = 'alice1337';
76+
$dn = 'uid=alice,ou=Users,' . $this->base;
77+
$this->prepareUser($dn, $username);
78+
79+
$user = \OC::$server->getUserManager()->get($username);
80+
if($user === null) {
81+
return false;
82+
}
83+
84+
$this->deleteUserFromLDAP($dn);
85+
86+
$job = new CleanUp();
87+
$job->run([]);
88+
89+
$user->delete();
90+
91+
return null === \OC::$server->getUserManager()->get($username);
92+
}
93+
}
94+
95+
/** @var string $host */
96+
/** @var int $port */
97+
/** @var string $adn */
98+
/** @var string $apwd */
99+
/** @var string $bdn */
100+
$test = new IntegrationTestUserCleanUp($host, $port, $adn, $apwd, $bdn);
101+
$test->init();
102+
$test->run();

‎apps/user_ldap/tests/User/UserTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -295,7 +295,7 @@ public function testUpdateQuotaToDefaultAllProvided() {
295295
}
296296

297297
public function testUpdateQuotaToNoneAllProvided() {
298-
list($access, $config, $filesys, $image, $log, $avaMgr, $dbc, $userMgr, $notiMgr) =
298+
list(, $config, $filesys, $image, $log, $avaMgr, $dbc, $userMgr, $notiMgr) =
299299
$this->getTestInstances();
300300

301301
list($access, $connection) =

0 commit comments

Comments
 (0)