From c55c9884f6817067dc63294f39dcceddefdfb741 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Mon, 28 Oct 2019 15:11:41 +0100 Subject: [PATCH] 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 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 adjust tests Signed-off-by: Arthur Schiwon 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 ensure that only valid group members are returned Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/Group_LDAP.php | 35 ++++++-- apps/user_ldap/lib/User/User.php | 15 ++++ apps/user_ldap/lib/User_LDAP.php | 52 ++++------- apps/user_ldap/lib/User_Proxy.php | 37 ++++++-- apps/user_ldap/tests/Group_LDAPTest.php | 2 +- apps/user_ldap/tests/User_LDAPTest.php | 109 ++---------------------- 6 files changed, 97 insertions(+), 153 deletions(-) diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index 0c4b7182ec47f..6d9acd79d58cf 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -811,6 +811,7 @@ private function getGroupsByMember($dn, &$seen = null) { * @param int $limit * @param int $offset * @return array with user ids + * @throws \Exception */ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) { if(!$this->enabled) { @@ -862,7 +863,10 @@ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) { //we got uids, need to get their DNs to 'translate' them to user names $filter = $this->access->combineFilterWithAnd(array( str_replace('%uid', trim($member), $this->access->connection->ldapLoginFilter), - $this->access->getFilterPartForUserSearch($search) + $this->access->combineFilterWithAnd([ + $this->access->getFilterPartForUserSearch($search), + $this->access->connection->ldapUserFilter + ]) )); $ldap_users = $this->access->fetchListOfUsers($filter, $attrs, 1); if(count($ldap_users) < 1) { @@ -871,17 +875,32 @@ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) { $groupUsers[] = $this->access->dn2username($ldap_users[0]['dn'][0]); } else { //we got DNs, check if we need to filter by search or we can give back all of them - if ($search !== '') { - if(!$this->access->readAttribute($member, + $uid = $this->access->dn2username($member); + if(!$uid) { + continue; + } + + $cacheKey = 'userExistsOnLDAP' . $uid; + $userExists = $this->access->connection->getFromCache($cacheKey); + if($userExists === false) { + continue; + } + if($userExists === null || $search !== '') { + if (!$this->access->readAttribute($member, $this->access->connection->ldapUserDisplayName, - $this->access->getFilterPartForUserSearch($search))) { + $this->access->combineFilterWithAnd([ + $this->access->getFilterPartForUserSearch($search), + $this->access->connection->ldapUserFilter + ]))) + { + if($search === '') { + $this->access->connection->writeToCache($cacheKey, false); + } continue; } + $this->access->connection->writeToCache($cacheKey, true); } - // dn2username will also check if the users belong to the allowed base - if($ocname = $this->access->dn2username($member)) { - $groupUsers[] = $ocname; - } + $groupUsers[] = $uid; } } diff --git a/apps/user_ldap/lib/User/User.php b/apps/user_ldap/lib/User/User.php index 95e29689224b2..31a33d6aa1d48 100644 --- a/apps/user_ldap/lib/User/User.php +++ b/apps/user_ldap/lib/User/User.php @@ -175,6 +175,21 @@ public function update() { } } + /** + * marks a user as deleted + * + * @throws \OCP\PreConditionNotMetException + */ + public function markUser() { + $curValue = $this->config->getUserValue($this->getUsername(), 'user_ldap', 'isDeleted', '0'); + if($curValue === '1') { + // the user is already marked, do not write to DB again + return; + } + $this->config->setUserValue($this->getUsername(), 'user_ldap', 'isDeleted', '1'); + $this->config->setUserValue($this->getUsername(), 'user_ldap', 'foundDeleted', (string)time()); + } + /** * processes results from LDAP for attributes as returned by getAttributesToRead() * @param array $ldapEntry the user entry as retrieved from LDAP diff --git a/apps/user_ldap/lib/User_LDAP.php b/apps/user_ldap/lib/User_LDAP.php index 85b11acfc5766..eb4589ce31d5d 100644 --- a/apps/user_ldap/lib/User_LDAP.php +++ b/apps/user_ldap/lib/User_LDAP.php @@ -46,7 +46,6 @@ use OCA\User_LDAP\User\User; use OCP\IConfig; use OCP\ILogger; -use OCP\IUser; use OCP\IUserSession; use OCP\Notification\IManager as INotificationManager; use OCP\Util; @@ -58,9 +57,6 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn /** @var INotificationManager */ protected $notificationManager; - /** @var string */ - protected $currentUserInDeletionProcess; - /** @var UserPluginManager */ protected $userPluginManager; @@ -75,20 +71,6 @@ public function __construct(Access $access, IConfig $ocConfig, INotificationMana $this->ocConfig = $ocConfig; $this->notificationManager = $notificationManager; $this->userPluginManager = $userPluginManager; - $this->registerHooks($userSession); - } - - protected function registerHooks(IUserSession $userSession) { - $userSession->listen('\OC\User', 'preDelete', [$this, 'preDeleteUser']); - $userSession->listen('\OC\User', 'postDelete', [$this, 'postDeleteUser']); - } - - public function preDeleteUser(IUser $user) { - $this->currentUserInDeletionProcess = $user->getUID(); - } - - public function postDeleteUser() { - $this->currentUserInDeletionProcess = null; } /** @@ -314,6 +296,12 @@ public function userExistsOnLDAP($user) { if(is_null($user)) { return false; } + $uid = $user instanceof User ? $user->getUsername() : $user->getOCName(); + $cacheKey = 'userExistsOnLDAP' . $uid; + $userExists = $this->access->connection->getFromCache($cacheKey); + if(!is_null($userExists)) { + return (bool)$userExists; + } $dn = $user->getDN(); //check if user really still exists by reading its entry @@ -321,18 +309,22 @@ public function userExistsOnLDAP($user) { try { $uuid = $this->access->getUserMapper()->getUUIDByDN($dn); if (!$uuid) { + $this->access->connection->writeToCache($cacheKey, false); return false; } $newDn = $this->access->getUserDnByUuid($uuid); //check if renamed user is still valid by reapplying the ldap filter if ($newDn === $dn || !is_array($this->access->readAttribute($newDn, '', $this->access->connection->ldapUserFilter))) { + $this->access->connection->writeToCache($cacheKey, false); return false; } $this->access->getUserMapper()->setDNbyUUID($newDn, $uuid); + $this->access->connection->writeToCache($cacheKey, true); return true; } catch (ServerNotAvailableException $e) { throw $e; } catch (\Exception $e) { + $this->access->connection->writeToCache($cacheKey, false); return false; } } @@ -341,6 +333,7 @@ public function userExistsOnLDAP($user) { $user->unmark(); } + $this->access->connection->writeToCache($cacheKey, true); return true; } @@ -363,15 +356,10 @@ public function userExists($uid) { $this->access->connection->ldapHost, ILogger::DEBUG); $this->access->connection->writeToCache('userExists'.$uid, false); return false; - } else if($user instanceof OfflineUser) { - //express check for users marked as deleted. Returning true is - //necessary for cleanup - return true; } - $result = $this->userExistsOnLDAP($user); - $this->access->connection->writeToCache('userExists'.$uid, $result); - return $result; + $this->access->connection->writeToCache('userExists'.$uid, true); + return true; } /** @@ -429,21 +417,13 @@ public function getHome($uid) { // early return path if it is a deleted user $user = $this->access->userManager->get($uid); - if($user instanceof OfflineUser) { - if($this->currentUserInDeletionProcess !== null - && $this->currentUserInDeletionProcess === $user->getOCName() - ) { - return $user->getHomePath(); - } else { - throw new NoUserException($uid . ' is not a valid user anymore'); - } - } else if ($user === null) { + if($user instanceof User || $user instanceof OfflineUser) { + $path = $user->getHomePath() ?: false; + } else { throw new NoUserException($uid . ' is not a valid user anymore'); } - $path = $user->getHomePath(); $this->access->cacheUserHome($uid, $path); - return $path; } diff --git a/apps/user_ldap/lib/User_Proxy.php b/apps/user_ldap/lib/User_Proxy.php index a3f9e67764e1f..359554a2bbc1f 100644 --- a/apps/user_ldap/lib/User_Proxy.php +++ b/apps/user_ldap/lib/User_Proxy.php @@ -37,7 +37,8 @@ use OCP\Notification\IManager as INotificationManager; class User_Proxy extends Proxy implements \OCP\IUserBackend, \OCP\UserInterface, IUserLDAP { - private $backends = array(); + private $backends = []; + /** @var User_LDAP */ private $refBackend = null; /** @@ -49,9 +50,14 @@ class User_Proxy extends Proxy implements \OCP\IUserBackend, \OCP\UserInterface, * @param INotificationManager $notificationManager * @param IUserSession $userSession */ - public function __construct(array $serverConfigPrefixes, ILDAPWrapper $ldap, IConfig $ocConfig, - INotificationManager $notificationManager, IUserSession $userSession, - UserPluginManager $userPluginManager) { + public function __construct( + array $serverConfigPrefixes, + ILDAPWrapper $ldap, + IConfig $ocConfig, + INotificationManager $notificationManager, + IUserSession $userSession, + UserPluginManager $userPluginManager + ) { parent::__construct($ldap); foreach($serverConfigPrefixes as $configPrefix) { $this->backends[$configPrefix] = @@ -105,13 +111,13 @@ protected function callOnLastSeenOn($uid, $method, $parameters, $passOnWhen) { && method_exists($this->getAccess($prefix), $method)) { $instance = $this->getAccess($prefix); } - $result = call_user_func_array(array($instance, $method), $parameters); + $result = call_user_func_array([$instance, $method], $parameters); if($result === $passOnWhen) { //not found here, reset cache to null if user vanished //because sometimes methods return false with a reason $userExists = call_user_func_array( - array($this->backends[$prefix], 'userExists'), - array($uid) + [$this->backends[$prefix], 'userExistsOnLDAP'], + [$uid] ); if(!$userExists) { $this->writeToCache($cacheKey, null); @@ -170,7 +176,22 @@ public function getUsers($search = '', $limit = 10, $offset = 0) { * @return boolean */ public function userExists($uid) { - return $this->handleRequest($uid, 'userExists', array($uid)); + $existsOnLDAP = false; + $existsLocally = $this->handleRequest($uid, 'userExists', array($uid)); + if($existsLocally) { + $existsOnLDAP = $this->userExistsOnLDAP($uid); + } + if($existsLocally && !$existsOnLDAP) { + try { + $user = $this->getLDAPAccess($uid)->userManager->get($uid); + if($user instanceof User) { + $user->markUser(); + } + } catch (\Exception $e) { + // ignore + } + } + return $existsLocally; } /** diff --git a/apps/user_ldap/tests/Group_LDAPTest.php b/apps/user_ldap/tests/Group_LDAPTest.php index a505c403acb3e..f9968af94c94c 100644 --- a/apps/user_ldap/tests/Group_LDAPTest.php +++ b/apps/user_ldap/tests/Group_LDAPTest.php @@ -1054,7 +1054,7 @@ public function testGroupMembers($groupDN, $expectedMembers, $groupsInfo = null) $ldap = new GroupLDAP($access, $pluginManager); $resultingMembers = $this->invokePrivate($ldap, '_groupMembers', [$groupDN]); - $this->assertEquals($expectedMembers, $resultingMembers, '', 0.0, 10, true); + $this->assertEqualsCanonicalizing($expectedMembers, $resultingMembers); } public function displayNameProvider() { diff --git a/apps/user_ldap/tests/User_LDAPTest.php b/apps/user_ldap/tests/User_LDAPTest.php index 7517994b34add..b11ba4a48dd79 100644 --- a/apps/user_ldap/tests/User_LDAPTest.php +++ b/apps/user_ldap/tests/User_LDAPTest.php @@ -314,22 +314,12 @@ public function testDeleteUserSuccess() { $offlineUser->expects($this->once()) ->method('getHomePath') ->willReturn($home); - $offlineUser->expects($this->once()) - ->method('getOCName') - ->willReturn($uid); $this->userManager->expects($this->atLeastOnce()) ->method('get') ->willReturn($offlineUser); $backend = new UserLDAP($this->access, $this->config, $this->notificationManager, $this->session, $this->pluginManager); - /** @var IUser|\PHPUnit_Framework_MockObject_MockObject $user */ - $user = $this->createMock(IUser::class); - $user->expects($this->once()) - ->method('getUID') - ->willReturn($uid); - - $backend->preDeleteUser($user); $result = $backend->deleteUser($uid); $this->assertTrue($result); /** @noinspection PhpUnhandledExceptionInspection */ @@ -509,18 +499,7 @@ public function testUserExists() { $this->prepareMockForUserExists(); $user = $this->createMock(User::class); - $user->expects($this->any()) - ->method('getDN') - ->willReturn('dnOfRoland,dc=test'); - $this->access->expects($this->any()) - ->method('readAttribute') - ->will($this->returnCallback(function($dn) { - if($dn === 'dnOfRoland,dc=test') { - return array(); - } - return false; - })); $this->userManager->expects($this->atLeastOnce()) ->method('get') ->willReturn($user); @@ -544,32 +523,18 @@ public function testUserExistsForDeleted() { ->with('dnOfFormerUser,dc=test') ->willReturn('45673458748'); - $this->access->expects($this->any()) - ->method('readAttribute') - ->will($this->returnCallback(function($dn) { - if($dn === 'dnOfRoland,dc=test') { - return array(); - } - return false; - })); $this->access->expects($this->any()) ->method('getUserMapper') ->willReturn($mapper); - $this->access->expects($this->once()) - ->method('getUserDnByUuid') - ->willThrowException(new \Exception()); $user = $this->createMock(User::class); - $user->expects($this->any()) - ->method('getDN') - ->willReturn('dnOfFormerUser,dc=test'); $this->userManager->expects($this->atLeastOnce()) ->method('get') ->willReturn($user); - //test for deleted user - $this->assertFalse($backend->userExists('formerUser')); + //test for deleted user – always returns true as long as we have the user in DB + $this->assertTrue($backend->userExists('formerUser')); } public function testUserExistsForNeverExisting() { @@ -621,64 +586,6 @@ public function testUserExistsPublicAPI() { $this->assertTrue($result); } - public function testUserExistsPublicAPIForDeleted() { - $backend = new UserLDAP($this->access, $this->config, $this->notificationManager, $this->session, $this->pluginManager); - $this->prepareMockForUserExists(); - \OC_User::useBackend($backend); - - $mapper = $this->createMock(UserMapping::class); - $mapper->expects($this->any()) - ->method('getUUIDByDN') - ->with('dnOfFormerUser,dc=test') - ->willReturn('45673458748'); - - $this->access->expects($this->any()) - ->method('readAttribute') - ->will($this->returnCallback(function($dn) { - if($dn === 'dnOfRoland,dc=test') { - return array(); - } - return false; - })); - $this->access->expects($this->any()) - ->method('getUserMapper') - ->willReturn($mapper); - $this->access->expects($this->once()) - ->method('getUserDnByUuid') - ->willThrowException(new \Exception()); - - $user = $this->createMock(User::class); - $user->expects($this->any()) - ->method('getDN') - ->willReturn('dnOfFormerUser,dc=test'); - - $this->userManager->expects($this->atLeastOnce()) - ->method('get') - ->willReturn($user); - - //test for deleted user - $this->assertFalse(\OC::$server->getUserManager()->userExists('formerUser')); - } - - public function testUserExistsPublicAPIForNeverExisting() { - $backend = new UserLDAP($this->access, $this->config, $this->notificationManager, $this->session, $this->pluginManager); - $this->prepareMockForUserExists(); - \OC_User::useBackend($backend); - - $this->access->expects($this->any()) - ->method('readAttribute') - ->will($this->returnCallback(function($dn) { - if($dn === 'dnOfRoland,dc=test') { - return array(); - } - return false; - })); - - //test for never-existing user - $result = \OC::$server->getUserManager()->userExists('mallory'); - $this->assertFalse($result); - } - public function testDeleteUserExisting() { $backend = new UserLDAP($this->access, $this->config, $this->notificationManager, $this->session, $this->pluginManager); @@ -869,14 +776,16 @@ public function testGetHomeDeletedUser() { ->will($this->returnValue(true)); $offlineUser = $this->createMock(OfflineUser::class); - $offlineUser->expects($this->never()) - ->method('getHomePath'); + $offlineUser->expects($this->atLeastOnce()) + ->method('getHomePath') + ->willReturn(''); $this->userManager->expects($this->atLeastOnce()) ->method('get') ->willReturn($offlineUser); - $backend->getHome($uid); + $result = $backend->getHome($uid); + $this->assertFalse($result); } public function testGetHomeWithPlugin() { @@ -1112,7 +1021,7 @@ public function testCountUsersWithPlugin() { ->willReturn(42); $this->assertEquals($this->backend->countUsers(),42); - } + } public function testLoginName2UserNameSuccess() { $loginName = 'Alice'; @@ -1294,7 +1203,7 @@ public function testSetPasswordInvalid() { $this->assertTrue(\OC_User::setPassword('roland', 'dt')); } - + public function testSetPasswordValid() { $this->prepareAccessForSetPassword($this->access);