From 411a47cadb078987f1354dfcae0852154df42a06 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Mon, 28 Oct 2019 15:11:41 +0100 Subject: [PATCH 1/5] 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 Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/User_LDAP.php | 32 +++----------------------- apps/user_ldap/tests/User_LDAPTest.php | 21 ++++------------- 2 files changed, 8 insertions(+), 45 deletions(-) diff --git a/apps/user_ldap/lib/User_LDAP.php b/apps/user_ldap/lib/User_LDAP.php index b62a5f95a42dc..c3f25f098d76a 100644 --- a/apps/user_ldap/lib/User_LDAP.php +++ b/apps/user_ldap/lib/User_LDAP.php @@ -47,7 +47,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; @@ -59,9 +58,6 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn /** @var INotificationManager */ protected $notificationManager; - /** @var string */ - protected $currentUserInDeletionProcess; - /** @var UserPluginManager */ protected $userPluginManager; @@ -76,20 +72,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; } /** @@ -430,21 +412,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/tests/User_LDAPTest.php b/apps/user_ldap/tests/User_LDAPTest.php index 7982ef0f42dbd..3717ecd8f3890 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 */ @@ -836,10 +826,7 @@ public function testGetHomeNoPath() { $this->assertFalse($result); } - public function testGetHomeDeletedUser() { - $this->expectException(\OC\User\NoUserException::class); - $uid = 'newyorker'; $backend = new UserLDAP($this->access, $this->config, $this->notificationManager, $this->session, $this->pluginManager); @@ -869,14 +856,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() { From 5cae135b94d495449948978d5e2207335843697b Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 29 Nov 2019 13:21:45 +0100 Subject: [PATCH 2/5] 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 --- apps/user_ldap/lib/User/User.php | 15 +++++++++++++ apps/user_ldap/lib/User_LDAP.php | 20 +++++++++++------ apps/user_ldap/lib/User_Proxy.php | 37 ++++++++++++++++++++++++------- 3 files changed, 57 insertions(+), 15 deletions(-) diff --git a/apps/user_ldap/lib/User/User.php b/apps/user_ldap/lib/User/User.php index 364f5f8cfabfe..dea5d91c0ce0b 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 c3f25f098d76a..323e59860f30a 100644 --- a/apps/user_ldap/lib/User_LDAP.php +++ b/apps/user_ldap/lib/User_LDAP.php @@ -297,6 +297,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 @@ -304,18 +310,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; } } @@ -324,6 +334,7 @@ public function userExistsOnLDAP($user) { $user->unmark(); } + $this->access->connection->writeToCache($cacheKey, true); return true; } @@ -346,15 +357,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; } /** diff --git a/apps/user_ldap/lib/User_Proxy.php b/apps/user_ldap/lib/User_Proxy.php index 6fdaa2998ecb0..96be4a7529f13 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; } /** From 1068b860ff8cfabedee78f86d9655e8c0c206170 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 29 Nov 2019 13:30:25 +0100 Subject: [PATCH 3/5] adjust tests Signed-off-by: Arthur Schiwon --- apps/user_ldap/tests/Group_LDAPTest.php | 2 +- apps/user_ldap/tests/User_LDAPTest.php | 45 ++++++------------------- 2 files changed, 11 insertions(+), 36 deletions(-) diff --git a/apps/user_ldap/tests/Group_LDAPTest.php b/apps/user_ldap/tests/Group_LDAPTest.php index f80b043016d2b..7ea4cb463d96f 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 3717ecd8f3890..3b473cc8de449 100644 --- a/apps/user_ldap/tests/User_LDAPTest.php +++ b/apps/user_ldap/tests/User_LDAPTest.php @@ -499,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); @@ -534,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() { @@ -646,8 +621,8 @@ public function testUserExistsPublicAPIForDeleted() { ->method('get') ->willReturn($user); - //test for deleted user - $this->assertFalse(\OC::$server->getUserManager()->userExists('formerUser')); + //test for deleted user – always returns true as long as we have the user in DB + $this->assertTrue(\OC::$server->getUserManager()->userExists('formerUser')); } public function testUserExistsPublicAPIForNeverExisting() { @@ -777,7 +752,7 @@ public function testGetHomeRelative() { $this->assertEquals($dataDir.'/susannah/', $result); } - + public function testGetHomeNoPath() { $this->expectException(\Exception::class); @@ -1101,7 +1076,7 @@ public function testCountUsersWithPlugin() { ->willReturn(42); $this->assertEquals($this->backend->countUsers(),42); - } + } public function testLoginName2UserNameSuccess() { $loginName = 'Alice'; @@ -1269,7 +1244,7 @@ private function prepareAccessForSetPassword($enablePasswordChange = true) { })); } - + public function testSetPasswordInvalid() { $this->expectException(\OC\HintException::class); $this->expectExceptionMessage('Password fails quality checking policy'); @@ -1283,7 +1258,7 @@ public function testSetPasswordInvalid() { $this->assertTrue(\OC_User::setPassword('roland', 'dt')); } - + public function testSetPasswordValid() { $this->prepareAccessForSetPassword($this->access); @@ -1313,7 +1288,7 @@ public function testSetPasswordValidDisabled() { $this->assertFalse(\OC_User::setPassword('roland', 'dt12234$')); } - + public function testSetPasswordWithInvalidUser() { $this->expectException(\Exception::class); $this->expectExceptionMessage('LDAP setPassword: Could not get user object for uid NotExistingUser. Maybe the LDAP entry has no set display name attribute?'); @@ -1414,7 +1389,7 @@ public function testSetDisplayNameWithPlugin() { $this->assertEquals($newDisplayName, $this->backend->setDisplayName('uid', $newDisplayName)); } - + public function testSetDisplayNameErrorWithPlugin() { $this->expectException(\OC\HintException::class); From f657ded6ec93de95eec33e19f3d6d528aa397f2d Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 5 Dec 2019 23:31:21 +0100 Subject: [PATCH 4/5] 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 --- apps/user_ldap/tests/User_LDAPTest.php | 58 -------------------------- 1 file changed, 58 deletions(-) diff --git a/apps/user_ldap/tests/User_LDAPTest.php b/apps/user_ldap/tests/User_LDAPTest.php index 3b473cc8de449..e4a105b2bd64d 100644 --- a/apps/user_ldap/tests/User_LDAPTest.php +++ b/apps/user_ldap/tests/User_LDAPTest.php @@ -586,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 – always returns true as long as we have the user in DB - $this->assertTrue(\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); From 489ed878e15a986e30ec1ea70b4459e6b22fbaa9 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Mon, 13 Jan 2020 12:10:29 +0100 Subject: [PATCH 5/5] ensure that only valid group members are returned Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/Group_LDAP.php | 35 ++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index 30d37c13ba265..a38c42035f698 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -812,6 +812,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) { @@ -863,7 +864,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) { @@ -872,17 +876,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; } }