Skip to content

Commit c55c988

Browse files
blizzzblizzz
committed
relax strict getHome behaviour for LDAP users in a shadow state
* simplifies deletion process * less strange behaviour when looking up home storage (as long as it is local) * thus could enable transfer ownerships after user went invisible on ldap backport of #17717 Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de> decouple userExists from userExistsOnLDAP check allows to mark users as offline right away, avoids a gap of being not a user and causing weird side effects Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de> adjust tests Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de> remove superfluous tests - user_ldap is not exposed to public api, it is always behind ldap_proxy - this is too much for a unit test - integration tests cover userExists implicitly Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de> ensure that only valid group members are returned Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
1 parent 2ecedea commit c55c988

File tree

6 files changed

+97
-153
lines changed

6 files changed

+97
-153
lines changed

‎apps/user_ldap/lib/Group_LDAP.php‎

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -811,6 +811,7 @@ private function getGroupsByMember($dn, &$seen = null) {
811811
* @param int $limit
812812
* @param int $offset
813813
* @return array with user ids
814+
* @throws \Exception
814815
*/
815816
public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
816817
if(!$this->enabled) {
@@ -862,7 +863,10 @@ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
862863
//we got uids, need to get their DNs to 'translate' them to user names
863864
$filter = $this->access->combineFilterWithAnd(array(
864865
str_replace('%uid', trim($member), $this->access->connection->ldapLoginFilter),
865-
$this->access->getFilterPartForUserSearch($search)
866+
$this->access->combineFilterWithAnd([
867+
$this->access->getFilterPartForUserSearch($search),
868+
$this->access->connection->ldapUserFilter
869+
])
866870
));
867871
$ldap_users = $this->access->fetchListOfUsers($filter, $attrs, 1);
868872
if(count($ldap_users) < 1) {
@@ -871,17 +875,32 @@ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
871875
$groupUsers[] = $this->access->dn2username($ldap_users[0]['dn'][0]);
872876
} else {
873877
//we got DNs, check if we need to filter by search or we can give back all of them
874-
if ($search !== '') {
875-
if(!$this->access->readAttribute($member,
878+
$uid = $this->access->dn2username($member);
879+
if(!$uid) {
880+
continue;
881+
}
882+
883+
$cacheKey = 'userExistsOnLDAP' . $uid;
884+
$userExists = $this->access->connection->getFromCache($cacheKey);
885+
if($userExists === false) {
886+
continue;
887+
}
888+
if($userExists === null || $search !== '') {
889+
if (!$this->access->readAttribute($member,
876890
$this->access->connection->ldapUserDisplayName,
877-
$this->access->getFilterPartForUserSearch($search))) {
891+
$this->access->combineFilterWithAnd([
892+
$this->access->getFilterPartForUserSearch($search),
893+
$this->access->connection->ldapUserFilter
894+
])))
895+
{
896+
if($search === '') {
897+
$this->access->connection->writeToCache($cacheKey, false);
898+
}
878899
continue;
879900
}
901+
$this->access->connection->writeToCache($cacheKey, true);
880902
}
881-
// dn2username will also check if the users belong to the allowed base
882-
if($ocname = $this->access->dn2username($member)) {
883-
$groupUsers[] = $ocname;
884-
}
903+
$groupUsers[] = $uid;
885904
}
886905
}
887906

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,21 @@ public function update() {
175175
}
176176
}
177177

178+
/**
179+
* marks a user as deleted
180+
*
181+
* @throws \OCP\PreConditionNotMetException
182+
*/
183+
public function markUser() {
184+
$curValue = $this->config->getUserValue($this->getUsername(), 'user_ldap', 'isDeleted', '0');
185+
if($curValue === '1') {
186+
// the user is already marked, do not write to DB again
187+
return;
188+
}
189+
$this->config->setUserValue($this->getUsername(), 'user_ldap', 'isDeleted', '1');
190+
$this->config->setUserValue($this->getUsername(), 'user_ldap', 'foundDeleted', (string)time());
191+
}
192+
178193
/**
179194
* processes results from LDAP for attributes as returned by getAttributesToRead()
180195
* @param array $ldapEntry the user entry as retrieved from LDAP

‎apps/user_ldap/lib/User_LDAP.php‎

Lines changed: 16 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,6 @@
4646
use OCA\User_LDAP\User\User;
4747
use OCP\IConfig;
4848
use OCP\ILogger;
49-
use OCP\IUser;
5049
use OCP\IUserSession;
5150
use OCP\Notification\IManager as INotificationManager;
5251
use OCP\Util;
@@ -58,9 +57,6 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
5857
/** @var INotificationManager */
5958
protected $notificationManager;
6059

61-
/** @var string */
62-
protected $currentUserInDeletionProcess;
63-
6460
/** @var UserPluginManager */
6561
protected $userPluginManager;
6662

@@ -75,20 +71,6 @@ public function __construct(Access $access, IConfig $ocConfig, INotificationMana
7571
$this->ocConfig = $ocConfig;
7672
$this->notificationManager = $notificationManager;
7773
$this->userPluginManager = $userPluginManager;
78-
$this->registerHooks($userSession);
79-
}
80-
81-
protected function registerHooks(IUserSession $userSession) {
82-
$userSession->listen('\OC\User', 'preDelete', [$this, 'preDeleteUser']);
83-
$userSession->listen('\OC\User', 'postDelete', [$this, 'postDeleteUser']);
84-
}
85-
86-
public function preDeleteUser(IUser $user) {
87-
$this->currentUserInDeletionProcess = $user->getUID();
88-
}
89-
90-
public function postDeleteUser() {
91-
$this->currentUserInDeletionProcess = null;
9274
}
9375

9476
/**
@@ -314,25 +296,35 @@ public function userExistsOnLDAP($user) {
314296
if(is_null($user)) {
315297
return false;
316298
}
299+
$uid = $user instanceof User ? $user->getUsername() : $user->getOCName();
300+
$cacheKey = 'userExistsOnLDAP' . $uid;
301+
$userExists = $this->access->connection->getFromCache($cacheKey);
302+
if(!is_null($userExists)) {
303+
return (bool)$userExists;
304+
}
317305

318306
$dn = $user->getDN();
319307
//check if user really still exists by reading its entry
320308
if(!is_array($this->access->readAttribute($dn, '', $this->access->connection->ldapUserFilter))) {
321309
try {
322310
$uuid = $this->access->getUserMapper()->getUUIDByDN($dn);
323311
if (!$uuid) {
312+
$this->access->connection->writeToCache($cacheKey, false);
324313
return false;
325314
}
326315
$newDn = $this->access->getUserDnByUuid($uuid);
327316
//check if renamed user is still valid by reapplying the ldap filter
328317
if ($newDn === $dn || !is_array($this->access->readAttribute($newDn, '', $this->access->connection->ldapUserFilter))) {
318+
$this->access->connection->writeToCache($cacheKey, false);
329319
return false;
330320
}
331321
$this->access->getUserMapper()->setDNbyUUID($newDn, $uuid);
322+
$this->access->connection->writeToCache($cacheKey, true);
332323
return true;
333324
} catch (ServerNotAvailableException $e) {
334325
throw $e;
335326
} catch (\Exception $e) {
327+
$this->access->connection->writeToCache($cacheKey, false);
336328
return false;
337329
}
338330
}
@@ -341,6 +333,7 @@ public function userExistsOnLDAP($user) {
341333
$user->unmark();
342334
}
343335

336+
$this->access->connection->writeToCache($cacheKey, true);
344337
return true;
345338
}
346339

@@ -363,15 +356,10 @@ public function userExists($uid) {
363356
$this->access->connection->ldapHost, ILogger::DEBUG);
364357
$this->access->connection->writeToCache('userExists'.$uid, false);
365358
return false;
366-
} else if($user instanceof OfflineUser) {
367-
//express check for users marked as deleted. Returning true is
368-
//necessary for cleanup
369-
return true;
370359
}
371360

372-
$result = $this->userExistsOnLDAP($user);
373-
$this->access->connection->writeToCache('userExists'.$uid, $result);
374-
return $result;
361+
$this->access->connection->writeToCache('userExists'.$uid, true);
362+
return true;
375363
}
376364

377365
/**
@@ -429,21 +417,13 @@ public function getHome($uid) {
429417

430418
// early return path if it is a deleted user
431419
$user = $this->access->userManager->get($uid);
432-
if($user instanceof OfflineUser) {
433-
if($this->currentUserInDeletionProcess !== null
434-
&& $this->currentUserInDeletionProcess === $user->getOCName()
435-
) {
436-
return $user->getHomePath();
437-
} else {
438-
throw new NoUserException($uid . ' is not a valid user anymore');
439-
}
440-
} else if ($user === null) {
420+
if($user instanceof User || $user instanceof OfflineUser) {
421+
$path = $user->getHomePath() ?: false;
422+
} else {
441423
throw new NoUserException($uid . ' is not a valid user anymore');
442424
}
443425

444-
$path = $user->getHomePath();
445426
$this->access->cacheUserHome($uid, $path);
446-
447427
return $path;
448428
}
449429

‎apps/user_ldap/lib/User_Proxy.php‎

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,8 @@
3737
use OCP\Notification\IManager as INotificationManager;
3838

3939
class User_Proxy extends Proxy implements \OCP\IUserBackend, \OCP\UserInterface, IUserLDAP {
40-
private $backends = array();
40+
private $backends = [];
41+
/** @var User_LDAP */
4142
private $refBackend = null;
4243

4344
/**
@@ -49,9 +50,14 @@ class User_Proxy extends Proxy implements \OCP\IUserBackend, \OCP\UserInterface,
4950
* @param INotificationManager $notificationManager
5051
* @param IUserSession $userSession
5152
*/
52-
public function __construct(array $serverConfigPrefixes, ILDAPWrapper $ldap, IConfig $ocConfig,
53-
INotificationManager $notificationManager, IUserSession $userSession,
54-
UserPluginManager $userPluginManager) {
53+
public function __construct(
54+
array $serverConfigPrefixes,
55+
ILDAPWrapper $ldap,
56+
IConfig $ocConfig,
57+
INotificationManager $notificationManager,
58+
IUserSession $userSession,
59+
UserPluginManager $userPluginManager
60+
) {
5561
parent::__construct($ldap);
5662
foreach($serverConfigPrefixes as $configPrefix) {
5763
$this->backends[$configPrefix] =
@@ -105,13 +111,13 @@ protected function callOnLastSeenOn($uid, $method, $parameters, $passOnWhen) {
105111
&& method_exists($this->getAccess($prefix), $method)) {
106112
$instance = $this->getAccess($prefix);
107113
}
108-
$result = call_user_func_array(array($instance, $method), $parameters);
114+
$result = call_user_func_array([$instance, $method], $parameters);
109115
if($result === $passOnWhen) {
110116
//not found here, reset cache to null if user vanished
111117
//because sometimes methods return false with a reason
112118
$userExists = call_user_func_array(
113-
array($this->backends[$prefix], 'userExists'),
114-
array($uid)
119+
[$this->backends[$prefix], 'userExistsOnLDAP'],
120+
[$uid]
115121
);
116122
if(!$userExists) {
117123
$this->writeToCache($cacheKey, null);
@@ -170,7 +176,22 @@ public function getUsers($search = '', $limit = 10, $offset = 0) {
170176
* @return boolean
171177
*/
172178
public function userExists($uid) {
173-
return $this->handleRequest($uid, 'userExists', array($uid));
179+
$existsOnLDAP = false;
180+
$existsLocally = $this->handleRequest($uid, 'userExists', array($uid));
181+
if($existsLocally) {
182+
$existsOnLDAP = $this->userExistsOnLDAP($uid);
183+
}
184+
if($existsLocally && !$existsOnLDAP) {
185+
try {
186+
$user = $this->getLDAPAccess($uid)->userManager->get($uid);
187+
if($user instanceof User) {
188+
$user->markUser();
189+
}
190+
} catch (\Exception $e) {
191+
// ignore
192+
}
193+
}
194+
return $existsLocally;
174195
}
175196

176197
/**

‎apps/user_ldap/tests/Group_LDAPTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1054,7 +1054,7 @@ public function testGroupMembers($groupDN, $expectedMembers, $groupsInfo = null)
10541054
$ldap = new GroupLDAP($access, $pluginManager);
10551055
$resultingMembers = $this->invokePrivate($ldap, '_groupMembers', [$groupDN]);
10561056

1057-
$this->assertEquals($expectedMembers, $resultingMembers, '', 0.0, 10, true);
1057+
$this->assertEqualsCanonicalizing($expectedMembers, $resultingMembers);
10581058
}
10591059

10601060
public function displayNameProvider() {

0 commit comments

Comments
 (0)